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 #3091

Closed
wants to merge 1 commit into from

Conversation

jacobbednarz
Copy link
Member

@jacobbednarz jacobbednarz commented Jan 31, 2024

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 naunce 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. There are some plans to potentially look at de-duping the payloads to prevent spurious versions being created however, that won't help here until such a time that we individually manage rules as their own entities.

A further improvement consideration here is if we don't have active uses for last_updated and version, consider removing them entirely from the schema. This will have downstream impacts but may clear up the output.

Closes #3082 as it is superseded by this PR.

Closes #2690 by improving the output and marking the version and last_updated as known values that will always be changing due to the way we update all the resources.

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 naunce
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`][1] 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. There are some plans to potentially look at de-duping the payloads
to prevent spurious versions being created however, that won't help here until
such a time that we individually manage `rules` as their own entities.

Closes #2690 by improving the output and marking the `version` and
`last_updated` as known values that will always be changing due to the way we
update all the resources.

[1]: https://github.com/cloudflare/terraform-provider-cloudflare/blob/f7c05af8be05e0ef9da72c10baa5a5bf8440e5d7/internal/framework/service/rulesets/schema.go#L104
@jacobbednarz jacobbednarz force-pushed the improve-ruleset-unknown-values-handling branch from 353203d to 8d7389b Compare January 31, 2024 04:18
Copy link
Contributor

changelog detected ✅

@jacobbednarz
Copy link
Member Author

need to dig into the test that handles preserving the references which is currently failing.

TF_ACC=1 go test ./internal/framework/service/rulesets -v -run "^TestAccCloudflareRuleset_PreserveRuleRefs" -count 1 -timeout 120m -parallel 1
=== RUN   TestAccCloudflareRuleset_PreserveRuleRefs
    resource_test.go:868: Step 2/12 error: Check failed: Check 1/2 error: cloudflare_ruleset.wvwfudwszz: Attribute "rules.0.ref" value: expected '4ac8be4f5f4944e4a15933e3823e2ef7' got 'ca9a6c1240a04c83898080e8b82252cb'
--- FAIL: TestAccCloudflareRuleset_PreserveRuleRefs (12.13s)
FAIL
FAIL	github.com/cloudflare/terraform-provider-cloudflare/internal/framework/service/rulesets	13.884s
FAIL
make: *** [testacc] Error 1

@pranit-p

This comment has been minimized.

@Nmishin
Copy link
Contributor

Nmishin commented Feb 21, 2024

I tested this change yesterday, and looks like everything works as expected - rules updated in CF and have correct ref (the same as id). And in the state the same.
Need to deep into the test itself.

Copy link
Contributor

github-actions bot commented Mar 7, 2024

Marking this pull request as stale due to 14 days of inactivity. This helps our maintainers find and focus on the active pull requests. If this pull request receives no comments in the next 7 days it will automatically be closed. Maintainers can also remove the lifecycle/stale label.
If this pull request was automatically closed and you feel this pull request should be reopened, we encourage creating a new pull request linking back to this one for added context. Thank you!

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