-
Notifications
You must be signed in to change notification settings - Fork 599
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
Conversation
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
353203d
to
8d7389b
Compare
changelog detected ✅ |
need to dig into the test that handles preserving the references which is currently failing.
right now, this approach isn't feasible |
This comment has been minimized.
This comment has been minimized.
I tested this change yesterday, and looks like everything works as expected - rules updated in CF and have correct |
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 |
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:
To a clearer and more concise output:
The second part of this confusing output is the
last_updated
andversion
output. Thelast_updated
is pretty self explanatory however, there are some minor but important details about theversion
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 viaListNestedObject
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 thelast_updated
andversion
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 managerules
as their own entities.A further improvement consideration here is if we don't have active uses for
last_updated
andversion
, 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
andlast_updated
as known values that will always be changing due to the way we update all the resources.