cloudposse / terraform-cloudflare-zone

Terraform module to provision a CloudFlare zone with DNS records, Argo, Firewall filters and rules
https://cloudposse.com/accelerate
Apache License 2.0
27 stars 19 forks source link

Allow creating mixed type page_rules and firewall_rules #28

Closed bryanhorstmann closed 7 months ago

bryanhorstmann commented 10 months ago

what

Change the type of the page_rules variable from list(any) to any. See https://github.com/hashicorp/terraform/issues/26265 for more information on why this is happening

why

When creating a list of page rules where some rules contain any of cache_ttl_by_status, forwarding_url, minify or cache_key_fields but others do not you receive the following error:

╷
│ Error: Invalid value for input variable
│ 
│   on main.tf line 8, in module "zone":
│    8:   page_rules = [
│    9:     {
│   10:       priority = 2
│   11:       status   = "active"
│   12:       target   = "foo.example.com"
│   13:       actions = {
│   14:         minify = {
│   15:           css  = "off"
│   16:           html = "off"
│   17:           js   = "off"
│   18:         }
│   19:       }
│   20:     },
│   21:     {
│   22:       priority = 1
│   23:       status   = "active"
│   24:       target   = "bar.example.com"
│   25:       actions = {
│   26:         browser_check = "off"
│   27:         cache_level   = "bypass"
│   28:         waf           = "on"
│   29:       }
│   30:     },
│   31:   ]
│ 
│ The given value is not suitable for module.zone.var.page_rules declared at .terraform/modules/zone/variables.tf:125,1-22: all list elements must have the same type.

Steps to reproduce:

module "zone" {
  source = "cloudposse/zone/cloudflare"
  # Cloud Posse recommends pinning every module to a specific version
  # version = "x.x.x"

  account_id = "example-account-id"
  zone       = "cloudposse.co"
  page_rules = [
    {
      priority = 2
      status   = "active"
      target   = "foo.example.com"
      actions = {
        minify = {
          css  = "off"
          html = "off"
          js   = "off"
        }
      }
    },
    {
      priority = 1
      status   = "active"
      target   = "bar.example.com"
      actions = {
        browser_check = "off"
        cache_level   = "bypass"
        waf           = "on"
      }
    },
  ]
}
kevcube commented 10 months ago

@bryanhorstmann can you instead turn it to type = list(object(blah blah)) and make a complex object type to represent what the provider expects? See here https://developer.hashicorp.com/terraform/language/expressions/type-constraints#optional-object-type-attributes

bryanhorstmann commented 10 months ago

@bryanhorstmann can you instead turn it to type = list(object(blah blah)) and make a complex object type to represent what the provider expects? See here https://developer.hashicorp.com/terraform/language/expressions/type-constraints#optional-object-type-attributes

@kevcube, I did start down this path, but then ran into a minor stumbling block. The cloudflare provider is not always clear on type. See https://registry.terraform.io/providers/cloudflare/cloudflare/latest/docs/resources/page_rule#actions. I can make an educated guess for all of them. eg all *_ttl should be int.

I'd need to increase the required version of Terraform to > 1.3.0 in order to support the optional() fields.

I've also just run into this same issue with firewall_rules. Happy to extend this change to include them as well.

kevcube commented 10 months ago

@kevcube, I did start down this path, but then ran into a minor stumbling block. The cloudflare provider is not always clear on type. See https://registry.terraform.io/providers/cloudflare/cloudflare/latest/docs/resources/page_rule#actions. I can make an educated guess for all of them. eg all *_ttl should be int.

Yeah I think educated guess is good. They look descriptive enough to guess most or all.

I'd need to increase the required version of Terraform to > 1.3.0 in order to support the optional() fields.

No problem

I've also just run into this same issue with firewall_rules. Happy to extend this change to include them as well.

👍

bryanhorstmann commented 10 months ago

Hey @kevcube, PR has been updated as requested. I've done the following:

docs/terraform.md still needs to be updated. I can see they're generated with terraform-docs but I'm not sure if this is meant to be done manually or the CI will handle this.

kevcube commented 10 months ago

/terratest

kevcube commented 10 months ago

@bryanhorstmann see failing tests

mergify[bot] commented 7 months ago

This pull request now has conflicts. Could you fix it @bryanhorstmann? 🙏

mergify[bot] commented 7 months ago

This PR has been closed due to inactivity and merge conflicts. Please resolve the conflicts and reopen if necessary.

mergify[bot] commented 7 months ago

Thanks @bryanhorstmann for creating this pull request!

A maintainer will review your changes shortly. Please don't be discouraged if it takes a while.

While you wait, make sure to review our contributor guidelines.

[!TIP]

Need help or want to ask for a PR review to be expedited?

Join us on Slack in the #pr-reviews channel.