Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Impossible to not have a drift with an empty cookie block in cloudflare_page_rule since 3.34.0 #2225

Closed
2 tasks done
lra opened this issue Feb 8, 2023 · 12 comments
Closed
2 tasks done
Labels
service/page_rules Categorizes issue or PR as related to the Page Rules service. working-as-intended Indicates an issue is working as designed.

Comments

@lra
Copy link

lra commented Feb 8, 2023

Confirmation

  • My issue isn't already found on the issue tracker.
  • I have replicated my issue using the latest version of the provider and it is still present.

Terraform and Cloudflare provider version

Terraform v1.3.7
on darwin_arm64

  • provider registry.terraform.io/cloudflare/cloudflare v3.34.0

Affected resource(s)

  • cloudflare_page_rule

Terraform configuration files

resource "cloudflare_page_rule" "page_rule" {
    id       = "..."
    priority = 29
    status   = "active"
    target   = "..."
    zone_id  = "..."

    actions {
        always_use_https     = false
        browser_cache_ttl    = "1800"
        cache_level          = "bypass"
        disable_apps         = false
        disable_performance  = false
        disable_railgun      = false
        disable_security     = false
        disable_zaraz        = false
        edge_cache_ttl       = 604800
        host_header_override = "....blob.core.windows.net"

        cache_key_fields {

            header {
                check_presence = []
                exclude        = [
                    "origin",
                ]
                include        = []
            }

            host {
                resolved = false
            }

            query_string {
                exclude = []
                ignore  = true
                include = []
            }

            user {
                device_type = false
                geo         = false
                lang        = false
            }
        }
    }
}

Link to debug output

https://gist.github.com/lra/5241499210278c1c1e8aca383529ebf3

Panic output

No response

Expected output

No changes. Your infrastructure matches the configuration.

Actual output

Terraform will perform the following actions:

  # module.seismic.cloudflare_page_rule.page_rule["..."] will be updated in-place
  ~ resource "cloudflare_page_rule" "page_rule" {
        id       = "..."
        # (4 unchanged attributes hidden)

      ~ actions {
            # (10 unchanged attributes hidden)

          ~ cache_key_fields {
              + cookie {
                  + check_presence = (known after apply)
                  + include        = (known after apply)
                }

                # (4 unchanged blocks hidden)
            }
        }
    }

Plan: 0 to add, 1 to change, 0 to destroy.

Steps to reproduce

  1. Don't define a cookie block under cloudflare_page_rule.cache_key_fields or define an empty cookie block, or define a block with empty array as check_presence and include, I tried everything I could.
  2. Run terraform apply
  3. Run terraform plan -> you get a drift

Additional factoids

This is not happening with 3.33.1

References

No response

@lra lra added kind/bug Categorizes issue or PR as related to a bug. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Feb 8, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Feb 8, 2023

Community Note

Voting for Prioritization

  • Please vote on this issue by adding a 👍 reaction to the original post to help the community and maintainers prioritize this request.
  • Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for issue followers and do not help prioritize the request.

Volunteering to Work on This Issue

  • If you are interested in working on this issue, please leave a comment.
  • If this would be your first contribution, please review the contribution guide.

@github-actions github-actions bot added the triage/debug-log-attached Indicates an issue or PR has a complete Terraform debug log. label Feb 8, 2023
@lra lra changed the title Impossible to not specify an empty cookie block cloudflare_page_rule since 3.34.0 Impossible to not have a drift with an empty cookie block in cloudflare_page_rule since 3.34.0 Feb 8, 2023
@jacobbednarz
Copy link
Member

have you tried removing this field from your state file? i suspect this has something to do with it still being an empty value and trying to keep it in sync.

@jacobbednarz
Copy link
Member

this relies on a module too which may make this harder to diagnose as it doesn't happen on a fresh or minimal repro.

@lra
Copy link
Author

lra commented Feb 9, 2023

have you tried removing this field from your state file? i suspect this has something to do with it still being an empty value and trying to keep it in sync.

It's a good idea, I just did 2 ways: Doing a state rm followed by an import, and touching the json state directly: same behavior.

this relies on a module too which may make this harder to diagnose as it doesn't happen on a fresh or minimal repro.

This is a very simple internal module, I'm touching the resource directly to do my tests.

I think I nailed down the problem:

If I don't state a cookie block at all:

cache_key_fields {
}

I get no drift: No changes. Your infrastructure matches the configuration.
But if I set a cookie block, empty or with empty lists, I can never get the provider to a clean state:

cache_key_fields {
    cookie {}
}

Lead to:

      ~ actions {
            # (10 unchanged attributes hidden)

          ~ cache_key_fields {
              + cookie {
                  + check_presence = (known after apply)
                  + include        = (known after apply)
                }

                # (4 unchanged blocks hidden)
            }
        }
    }

or

        cookie {
          check_presence = []
          include = []
        }

Lead to:

  ~ resource "cloudflare_page_rule" "page_rule" {
        id       = "..."
        # (4 unchanged attributes hidden)

      ~ actions {
            # (10 unchanged attributes hidden)

          ~ cache_key_fields {
              + cookie {}

                # (4 unchanged blocks hidden)
            }
        }
    }

This one is weird, it wants to set an empty object cookie {} when I set empty lists. While it tried to actually set the lists when I gave it an empty cookie {} object on the previous try.

There is something fishy here.

The big problem is that with those nested objects (cookie and others under cache_key_fields), when you have tons of cloudflare_page_rule, you create a module and set variables to feed dynamic cache_key_fields blocks, and there is no way to have a condition to not state at least cookie {} as this is already inside a dynamic block. If there is no cookie parameter set, you have to set at least cookie {}, so this behavior is blocking on <3.34.0 anyone wanting to loop over different cloudflare_page_rule. I guess modules maintainers will have the same issue when will start to show up soon.

@tamas-jozsa
Copy link
Contributor

I think this was me here: #2208
I'll try to repro this and come up with something for you shortly.

@tamas-jozsa
Copy link
Contributor

tamas-jozsa commented Feb 10, 2023

@irA I'm not so sure why the provider doesn't remove these empty cookie fields altogether, I was looking at the code, but I wasn't able to figure it out YET.

These should be completely removed by the provider, but they are not.

cookie {
   check_presence = []
   include = []
}

Is it a viable workaround if you remove these empty cookie fields / cookie fields with empty arrays from your configuration?

@lra
Copy link
Author

lra commented Feb 10, 2023

For a single resource yes but:

Sadly not for a module that optionally propose to set cookie settings, AFAIK there is no way in a dynamic cache_key_fields block to optionally state a cookie block (which means making it dynamic too), that's what I tried to explain at the bottom of #2225 (comment)

BTW, this same problem seem to apply to all optional blocks within cache_key_fields. So the only workaround would have to write a different resource for each combination of cache_key_fields block possible, we are talking 5x4x3x2 = 120 combinations =(

@jacobbednarz
Copy link
Member

if this is for a module, it very well could be the module causing the issue which involves its own logic for when to set or define these fields.

for now, I'd suggest coming up with a minimal reproduction case without modules or dynamics that demonstrates the bug. as it stands, this isn't reproducible using just the resource so I'm hesitant to say this is a bug in the provider until we can rule those out.

@lra
Copy link
Author

lra commented Feb 11, 2023

Again, the module has nothing to do with it, I was editing the resource directly.

Anyway, here's a self-contained explicit exhaustive reproductive case. You'll only have to change the zone_id:

terraform {
  required_providers {
    cloudflare = {
      source = "cloudflare/cloudflare"
    }
  }
}

provider "cloudflare" {
}

resource "cloudflare_page_rule" "this" {
  target  = "whatever/*"
  zone_id = "82592f37298b1a37a3fb743431a53c1c"

  actions {
    cache_key_fields {
      query_string {
        exclude = ["api_token"]
      }
      user {
        lang = true
      }
      host {}
      cookie {}
    }
  }
}

Run terraform apply:

Terraform will perform the following actions:

  # cloudflare_page_rule.this will be created
  + resource "cloudflare_page_rule" "this" {
      + id       = (known after apply)
      + priority = 1
      + status   = "active"
      + target   = "whatever/*"
      + zone_id  = "82592f37298b1a37a3fb743431a53c1c"

      + actions {
          + always_use_https    = false
          + disable_apps        = false
          + disable_performance = false
          + disable_railgun     = false
          + disable_security    = false
          + disable_zaraz       = false

          + cache_key_fields {
              + cookie {
                  + check_presence = (known after apply)
                  + include        = (known after apply)
                }

              + host {
                  + resolved = false
                }

              + query_string {
                  + exclude = [
                      + "api_token",
                    ]
                  + ignore  = (known after apply)
                  + include = (known after apply)
                }

              + user {
                  + device_type = (known after apply)
                  + geo         = (known after apply)
                  + lang        = true
                }
            }
        }
    }

Plan: 1 to add, 0 to change, 0 to destroy.

Do you want to perform these actions?
  Terraform will perform the actions described above.
  Only 'yes' will be accepted to approve.

  Enter a value: yes

cloudflare_page_rule.this: Creating...
cloudflare_page_rule.this: Creation complete after 2s [id=11e475e98dd96670b35d3512adf0644c]

Apply complete! Resources: 1 added, 0 changed, 0 destroyed.

Good.
Now run terraform apply again:

Terraform will perform the following actions:

  # cloudflare_page_rule.this will be updated in-place
  ~ resource "cloudflare_page_rule" "this" {
        id       = "11e475e98dd96670b35d3512adf0644c"
        # (4 unchanged attributes hidden)

      ~ actions {
            # (7 unchanged attributes hidden)

          ~ cache_key_fields {
              + cookie {
                  + check_presence = (known after apply)
                  + include        = (known after apply)
                }

                # (3 unchanged blocks hidden)
            }
        }
    }

Plan: 0 to add, 1 to change, 0 to destroy.

Do you want to perform these actions?
  Terraform will perform the actions described above.
  Only 'yes' will be accepted to approve.

  Enter a value: yes

cloudflare_page_rule.this: Modifying... [id=11e475e98dd96670b35d3512adf0644c]
cloudflare_page_rule.this: Modifications complete after 8s [id=11e475e98dd96670b35d3512adf0644c]

Apply complete! Resources: 0 added, 1 changed, 0 destroyed.

And again:

Terraform will perform the following actions:

  # cloudflare_page_rule.this will be updated in-place
  ~ resource "cloudflare_page_rule" "this" {
        id       = "11e475e98dd96670b35d3512adf0644c"
        # (4 unchanged attributes hidden)

      ~ actions {
            # (7 unchanged attributes hidden)

          ~ cache_key_fields {
              + cookie {
                  + check_presence = (known after apply)
                  + include        = (known after apply)
                }

                # (3 unchanged blocks hidden)
            }
        }
    }

Plan: 0 to add, 1 to change, 0 to destroy.

Do you want to perform these actions?
  Terraform will perform the actions described above.
  Only 'yes' will be accepted to approve.

  Enter a value: yes

cloudflare_page_rule.this: Modifying... [id=11e475e98dd96670b35d3512adf0644c]
cloudflare_page_rule.this: Modifications complete after 2s [id=11e475e98dd96670b35d3512adf0644c]

Apply complete! Resources: 0 added, 1 changed, 0 destroyed.

etc...

And yes if I don't specify any cookie {} I won't get the infinite drift, but as I can't make the block dynamic when we parametrize the resource, because it's already in a dynamic block, I have to state a cookie {} block, be it empty or not, and this leads to the infinite drifts.

@jacobbednarz
Copy link
Member

And yes if I don't specify any cookie {} I won't get the infinite drift

this is what #2208 removed the need for and the what you should be doing when you don't have configuration options for that. this has been a weird empty block since it's inception (and caused some internals issues) but has since been fixed to be correct.

i'm sorry but this is an issue with the syntax used to make the resource dynamic. as such, we don't directly support it and I'd recommend you open a feature request with terraform core to achieve this. alternatively, you can migrate this functionality to the rulesets equivalent and use that resource instead.

@jacobbednarz jacobbednarz closed this as not planned Won't fix, can't repro, duplicate, stale Feb 11, 2023
@jacobbednarz jacobbednarz added working-as-intended Indicates an issue is working as designed. service/page_rules Categorizes issue or PR as related to the Page Rules service. and removed kind/bug Categorizes issue or PR as related to a bug. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. triage/debug-log-attached Indicates an issue or PR has a complete Terraform debug log. labels Feb 11, 2023
@lra
Copy link
Author

lra commented Feb 11, 2023

I understand, what confused me I guess is that the doc still states it's required but allowed to be empty. Same behavior for the header block.

We'll workaround it by adding more dynamic blocks.

@jacobbednarz
Copy link
Member

if the docs are adding confusion here, we can get those updated to reflect the true state now. you're welcome to send over a PR otherwise someone will get to it later on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
service/page_rules Categorizes issue or PR as related to the Page Rules service. working-as-intended Indicates an issue is working as designed.
Projects
None yet
Development

No branches or pull requests

3 participants