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

resource/cloudflare_ruleset: improve unknown value handling #4692

Closed
wants to merge 1 commit into from

Conversation

jacobbednarz
Copy link
Member

Within the rulesets schema, we have a handful of fields that are
Computed. The expectation of this directive is that it is a
value not known at run time and may be provided by the remote.
However, this premise will break down and not work correctly if
the value is ever provided to the plan since the value is no
longer "unknown". This subtle bug is one part of what is
contributing to the additional output toil in #2690. To address
this, I've removed the lines that modified these values and
were being supplied to the plan operation.

This takes the confusing and bloated output from looking like this:

Terraform will perform the following actions:

  # cloudflare_ruleset.rate_limiting_example will be updated in-place
  ~ resource "cloudflare_ruleset" "rate_limiting_example" {
        id          = "87e1099f077f4e49bbfbe159217ff605"
        name        = "restrict API requests count"
        # (4 unchanged attributes hidden)

      ~ rules {
          ~ id           = "e3b97fdf354c4c24a7ac091c3537fbc5" -> (known after apply)
          ~ last_updated = "2024-01-31 04:02:55.651275 +0000 UTC" -> (known after apply)
          ~ ref          = "e3b97fdf354c4c24a7ac091c3537fbc5" -> (known after apply)
          ~ version      = "1" -> (known after apply)
            # (4 unchanged attributes hidden)

            # (1 unchanged block hidden)
        }
      + rules {
          + action       = "block"
          + description  = "rate limit for API"
          + enabled      = true
          + expression   = "(http.request.uri.path matches \"^/api0/\")"
          + id           = (known after apply)
          + last_updated = (known after apply)
          + ref          = (known after apply)
          + version      = (known after apply)

          + ratelimit {
              + characteristics     = [
                  + "cf.colo.id",
                  + "ip.src",
                ]
              + mitigation_timeout  = 600
              + period              = 60
              + requests_per_period = 100
              + requests_to_origin  = false
            }
        }
    }

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

To a clearer and more concise output:

Terraform will perform the following actions:

  # cloudflare_ruleset.rate_limiting_example will be updated in-place
  ~ resource "cloudflare_ruleset" "rate_limiting_example" {
        id          = "87e1099f077f4e49bbfbe159217ff605"
        name        = "restrict API requests count"
        # (4 unchanged attributes hidden)

      ~ rules {
            id           = "39ed6015367444e3905d838cadc1b9c2"
          + last_updated = (known after apply)
          + version      = (known after apply)
            # (5 unchanged attributes hidden)

            # (1 unchanged block hidden)
        }
      + rules {
          + action       = "block"
          + description  = "rate limit for API"
          + enabled      = true
          + expression   = "(http.request.uri.path matches \"^/api0/\")"
          + id           = (known after apply)
          + last_updated = (known after apply)
          + ref          = (known after apply)
          + version      = (known after apply)

          + ratelimit {
              + characteristics     = [
                  + "cf.colo.id",
                  + "ip.src",
                ]
              + mitigation_timeout  = 600
              + period              = 60
              + requests_per_period = 100
              + requests_to_origin  = false
            }
        }
    }

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

The second part of this confusing output is the last_updated and
version output. The last_updated is pretty self explanatory
however, there are some minor but important details about the version
field here. Within ERE, it captures and is tied to a particular state
of the ruleset rule at any point and is incremented when changes are
detected to any parts of the rule. The nuance here is that ERE does
not perform a diff of the current state and what is proposed.
Instead, it autoincrements this field even if the payload is identical.
So why is this important for the Terraform resource? Well, since we
use explicit ordering via ListNestedObject schema attribute, we are
sending all rules to the API even when only a single one changes to
ensure we maintain the correctness the end user expects. Doing this
causes the last_updated and version fields to always show as
unknown values and result in updates.

While the last_updated and version fields are useful in a context
where you may reuse the version, value, it's not in Terraform. To
remove that part of the diff, we're going to just remove them entirely
from the schema. Given that users couldn't be using them in a
configuration, we're going to release this within a minor version
handling the cleanup internally to the provider.

Supersedes #3091

Closes #2690

Within the rulesets schema, we have a handful of fields that are
`Computed`. The expectation of this directive is that it is a
value not known at run time and may be provided by the remote.
However, this premise will break down and not work correctly if
the value is ever provided to the plan since the value is no
longer "unknown". This subtle bug is one part of what is
contributing to the additional output toil in #2690. To address
this, I've removed the lines that modified these values and
were being supplied to the plan operation.

This takes the confusing and bloated output from looking like this:

```
Terraform will perform the following actions:

  # cloudflare_ruleset.rate_limiting_example will be updated in-place
  ~ resource "cloudflare_ruleset" "rate_limiting_example" {
        id          = "87e1099f077f4e49bbfbe159217ff605"
        name        = "restrict API requests count"
        # (4 unchanged attributes hidden)

      ~ rules {
          ~ id           = "e3b97fdf354c4c24a7ac091c3537fbc5" -> (known after apply)
          ~ last_updated = "2024-01-31 04:02:55.651275 +0000 UTC" -> (known after apply)
          ~ ref          = "e3b97fdf354c4c24a7ac091c3537fbc5" -> (known after apply)
          ~ version      = "1" -> (known after apply)
            # (4 unchanged attributes hidden)

            # (1 unchanged block hidden)
        }
      + rules {
          + action       = "block"
          + description  = "rate limit for API"
          + enabled      = true
          + expression   = "(http.request.uri.path matches \"^/api0/\")"
          + id           = (known after apply)
          + last_updated = (known after apply)
          + ref          = (known after apply)
          + version      = (known after apply)

          + ratelimit {
              + characteristics     = [
                  + "cf.colo.id",
                  + "ip.src",
                ]
              + mitigation_timeout  = 600
              + period              = 60
              + requests_per_period = 100
              + requests_to_origin  = false
            }
        }
    }

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

To a clearer and more concise output:

```
Terraform will perform the following actions:

  # cloudflare_ruleset.rate_limiting_example will be updated in-place
  ~ resource "cloudflare_ruleset" "rate_limiting_example" {
        id          = "87e1099f077f4e49bbfbe159217ff605"
        name        = "restrict API requests count"
        # (4 unchanged attributes hidden)

      ~ rules {
            id           = "39ed6015367444e3905d838cadc1b9c2"
          + last_updated = (known after apply)
          + version      = (known after apply)
            # (5 unchanged attributes hidden)

            # (1 unchanged block hidden)
        }
      + rules {
          + action       = "block"
          + description  = "rate limit for API"
          + enabled      = true
          + expression   = "(http.request.uri.path matches \"^/api0/\")"
          + id           = (known after apply)
          + last_updated = (known after apply)
          + ref          = (known after apply)
          + version      = (known after apply)

          + ratelimit {
              + characteristics     = [
                  + "cf.colo.id",
                  + "ip.src",
                ]
              + mitigation_timeout  = 600
              + period              = 60
              + requests_per_period = 100
              + requests_to_origin  = false
            }
        }
    }

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

The second part of this confusing output is the `last_updated` and
`version` output. The `last_updated` is pretty self explanatory
however, there are some minor but important details about the version
field here. Within ERE, it captures and is tied to a particular state
of the ruleset rule at any point and is incremented when changes are
detected to any parts of the rule. The nuance here is that ERE does
not perform a diff of the current state and what is proposed.
Instead, it autoincrements this field even if the payload is identical.
So why is this important for the Terraform resource? Well, since we
use explicit ordering via `ListNestedObject` schema attribute, we are
sending all rules to the API even when only a single one changes to
ensure we maintain the correctness the end user expects. Doing this
causes the `last_updated` and `version` fields to always show as
unknown values and result in updates.

While the `last_updated` and `version` fields are useful in a context
where you may reuse the `version`, value, it's not in Terraform. To
remove that part of the diff, we're going to just remove them entirely
from the schema. Given that users couldn't be using them in a
configuration, we're going to release this within a minor version
handling the cleanup internally to the provider.

Supersedes #3091

Closes #2690
@cloudflare cloudflare deleted a comment from github-actions bot Dec 3, 2024
@william-woodhead
Copy link

could we also target nested version fields, like in action_parameters? @zakcutner

@jacobbednarz
Copy link
Member Author

@zakcutner has more improvements in #4697 so let's use that one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants