Add plan-based gates to cloudflare_zone_settings_override or improve docs

[lukiffer]

We use modules in our terraform infrastructure, and in creating a module for our various zones, we included the cloudflare_zone_settings_override resource to both set sane defaults but allow customisation in our "live" module.

Some of the settings are only available at certain plan levels (for example, mirage is only available at Pro and above. Even when setting the value to "off" (and its existing state is "off") running an apply, the API still responds with an error as the setting is "read only" (see related #559 and #599):

Error: invalid zone setting "mirage" (value: off) found - cannot be set as it is read only

We can gate the setting in our module, i.e.:

resource "cloudflare_zone_settings_override" "settings" {
  zone_id = cloudflare_zone.zone.id

  settings {
    # ...
    mirage = var.plan != "free" ? var.mirage : null
  }
}

IMHO this should be handled internally within the provider, though I could see arguments against its inclusion as well. My thoughts here are that we either need to poll remote state of the cloudflare_zone to which this is being applied in order to read the plan for the zone, or have the module include both the cloudflare_zone and cloudflare_zone_settings_override resources and use a common var.

At a minimum, I feel it would be good to include these caveats as part of the documentation as it took a not-insignificant amount of debugging to figure out why some of our zones were fine and others were having this error.

Terraform Version

Terraform v0.12.24

Affected Resource(s)

• cloudflare_zone_settings_override

Terraform Configuration Files

Fails on Free Plan

resource "cloudflare_zone_settings_override" "settings" {
  settings {
    mirage = "off"
  }
}

Succeeds on Free Plan

resource "cloudflare_zone_settings_override" "settings" {
  settings {
    mirage = null
  }
}

Expected Behavior

The provider should either swallow the error if the values are already consistent or output an error if they're inconsistent. Alternatively, it could just read the plan from the target zone and not attempt to set them (again optionally warning the user) if the plan doesn't support an update of the property. This gets a bit tricky if the plan changes separately underneath the override resource.

Actual Behavior

The terraform plan output:

mirage = "off"

Intimating that there is no change to this value.

When running a terraform apply, the following error was output:

Error: invalid zone setting "mirage" (value: off) found - cannot be set as it is read only

Steps to Reproduce

1. Create a zone and assign it the Free plan
2. Create a cloudflare_zone_settings_override resource setting the mirage property to "off"
3. terraform plan
4. terraform apply

[jacobbednarz]

Right now, the provider doesn’t have any concept of the existing rate plan for features and it’s probably not something that we would add in due to the complexities of keeping it in sync and ability to toggle entitlements on Cloudflare’s side without it mapping cleanly to an existing plan.

Something to note here is that you shouldn’t be setting all of the options in the cloudflare_zone_settings_override resource. If you aren’t overriding the default value, you can omit it.

[lukiffer]

I think it's completely reasonable to avoid the complexities of entitlements. After further consideration, swallowing the error probably isn't appropriate either given its cascading affect on the tfstate artifacts. And while the workarounds are a bit cumbersome it's probably the best option, all things considered. Which leaves a subtle docs update.

Speaking of which, your point about not updating everything wasn't immediately clear from the docs. Additionally, I'm not sure how I feel about the notion of only updating non-default settings – when I think of Terraform I think of infrastructure that is both deterministic and idempotent.

resource "cloudflare_zone_settings_override" "settings" {
  settings {
    mirage = "off"
  }
}

Given the above code, I would expect the feature to be turned off regardless of the default state. So if Cloudflare decides to change the default state, newly provisioned infrastructure is still provisioned correctly. Similarly, if I want to explicitly inherit the default, setting it to null seems to work.

All that said, this doesn't seem as unruly a quirk as it seemed at first – documentation seems sufficient. If that seems reasonable, I'll float a PR across.

[jacobbednarz]

when I think of Terraform I think of infrastructure that is both deterministic and idempotent.

To be clear here, you don't compromise either of these attributes by only defining what you override. I believe the point you're trying to make is better articulated in the sentence below it.

Given the above code, I would expect the feature to be turned off regardless of the default state.

The terminology for this is more about coverage and having all infrastructure managed as code than either of the other two attributes you've mentioned. This assumption seems totally reasonable 👍

All that said, this doesn't seem as unruly a quirk as it seemed at first – documentation seems sufficient. If that seems reasonable, I'll float a PR across.

Happy to review a PR that will help iron out any confusion here.

[lukiffer]

@jacobbednarz apologies this took a while for me to get back around to – I've opened a PR https://github.com/terraform-providers/terraform-provider-cloudflare/pull/689

To me, the only questionable bit is dividing them into groups based on plan support. While I would find it helpful now, if Cloudflare does change the composition of their plans, it would then be decidedly unhelpful. Let me know if you have feedback. Cheers.

[jacobbednarz]

I'm on the fence whether the documentation should be split like this as it is introducing an extra maintenance burden into this project to keep it in sync. I'd be more comfortable with some sort of disclaimer paragraph stating that you should first consult the Cloudflare source for the feature you're looking to manage and ensure it's available within your current plan. I think that hits the balance of providing information without making this project a source of truth for something outside its realm.

[lukiffer]

Agree. I reverted and just added a disclaimer paragraph with a pseudo-exact error message to ensure it's searchable/discoverable. This should have zero maintenance impact that isn't the result of an internal change.

CI checks are running, but the PR is up to date.

[jacobbednarz]

Closed by #689