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 diffs when only some rules are changed #4697

Merged
merged 5 commits into from
Dec 9, 2024

Conversation

zakcutner
Copy link
Contributor

@zakcutner zakcutner commented Dec 3, 2024

Remove version in ruleset rule action parameters

This action parameter is used within the execute action, but cannot
actually be set to anything other than latest.

Removing this field from Terraform prevents an issue where it shows up
in diffs, due to it being a computed attribute.

Prevent ruleset rule IDs incorrectly showing as changed in diffs

In the Rulesets API, it is possible to prevent rule IDs from changing
across updates by using rule refs. If a rule's ref does not change as
part of an update, then neither will its ID.

However, Terraform does not know this and always reports that rule IDs
may change, which creates very large diffs. In this change, we use a
resource plan modifier to "teach" Terraform this rule.

Remove version and last_updated fields from ruleset rules

These fields frequently change, leading to verbose diffs in plans. Since
these fields are not really useful within Terraform, we remove them from
the schema completely to prevent them from "polluting" diffs.

This also matches the behavior we have for the ruleset-level version
and last_updated fields, which are also not represented in the
Terraform schema.

Remove logic to preserve ruleset rule refs

This logic tries to detect which rules are the same as before during an
update, and preserve the refs (and therefore IDs) of these rules.

However, this logic is fallible, since we cannot know the user's true
intention, and makes it difficult for us to remove the version and
last_updated fields from rules.

If users want to preserve rule refs across updates, they can now
actually use the ref field directly—and there is no need for this
remapping logic anymore.

Closes #2690

Copy link
Contributor

github-actions bot commented Dec 3, 2024

changelog detected ✅

@zakcutner zakcutner force-pushed the rulesets branch 3 times, most recently from 3e07a47 to a00a466 Compare December 3, 2024 22:48
@jacobbednarz
Copy link
Member

we'll want to remove the TestAccCloudflareRuleset_PreserveRuleRefs acceptance test too given that no longer exists.

@jacobbednarz
Copy link
Member

looks like this test has also started failing (on master) so we should update it while we're here.

=== RUN   TestAccCloudflareRuleset_ActionParametersHTTPDDoSOverride
    resource_test.go:1464: Step 1/1 error: Error running apply: exit status 1
        
        Error: error creating ruleset multiple skips for managed rulesets
        
          with cloudflare_ruleset.xmqibwlrcj,
          on terraform_plugin_test.tf line 12, in resource "cloudflare_ruleset" "xmqibwlrcj":
          12:   resource "cloudflare_ruleset" "xmqibwlrcj" {
        
        'fdfdac75430c4c47a959592f0aa5e68a' is not a valid value for rules because it
        does not exist in ruleset 4d21379b4f9f4bb088e0729962c8b3cf (20233)
--- FAIL: TestAccCloudflareRuleset_ActionParametersHTTPDDoSOverride (1.34s)

otherwise, lgtm!

@jacobbednarz
Copy link
Member

tests all passing here (ignore those other failures for the moment while we fix them)

TF_ACC=1 go test ./internal/framework/service/rulesets/ -run "^TestAcc" -count 1 -v
=== RUN   TestAccCloudflareRuleset_WAFBasic
--- PASS: TestAccCloudflareRuleset_WAFBasic (4.67s)
=== RUN   TestAccCloudflareRuleset_WAFManagedRuleset
--- PASS: TestAccCloudflareRuleset_WAFManagedRuleset (3.88s)
=== RUN   TestAccCloudflareRuleset_WAFManagedRulesetWithoutDescription
--- PASS: TestAccCloudflareRuleset_WAFManagedRulesetWithoutDescription (3.87s)
=== RUN   TestAccCloudflareRuleset_WAFManagedRulesetOWASP
--- PASS: TestAccCloudflareRuleset_WAFManagedRulesetOWASP (4.03s)
=== RUN   TestAccCloudflareRuleset_WAFManagedRulesetOWASPBlockXSSWithAnomalyOver60
--- PASS: TestAccCloudflareRuleset_WAFManagedRulesetOWASPBlockXSSWithAnomalyOver60 (4.33s)
=== RUN   TestAccCloudflareRuleset_WAFManagedRulesetOWASPOnlyPL1
--- PASS: TestAccCloudflareRuleset_WAFManagedRulesetOWASPOnlyPL1 (4.14s)
=== RUN   TestAccCloudflareRuleset_WAFManagedRulesetDeployMultiple
--- PASS: TestAccCloudflareRuleset_WAFManagedRulesetDeployMultiple (4.66s)
=== RUN   TestAccCloudflareRuleset_WAFManagedRulesetDeployMultipleWithSkip
--- PASS: TestAccCloudflareRuleset_WAFManagedRulesetDeployMultipleWithSkip (5.04s)
=== RUN   TestAccCloudflareRuleset_WAFManagedRulesetDeployMultipleWithTopSkipAndLastSkip
--- PASS: TestAccCloudflareRuleset_WAFManagedRulesetDeployMultipleWithTopSkipAndLastSkip (5.01s)
=== RUN   TestAccCloudflareRuleset_SkipPhaseAndProducts
--- PASS: TestAccCloudflareRuleset_SkipPhaseAndProducts (4.09s)
=== RUN   TestAccCloudflareRuleset_WAFManagedRulesetWithCategoryAndRuleBasedOverrides
--- PASS: TestAccCloudflareRuleset_WAFManagedRulesetWithCategoryAndRuleBasedOverrides (3.85s)
=== RUN   TestAccCloudflareRuleset_WAFManagedRulesetWithIDBasedOverrides
--- PASS: TestAccCloudflareRuleset_WAFManagedRulesetWithIDBasedOverrides (4.02s)
=== RUN   TestAccCloudflareRuleset_MagicTransitUpdateWithHigherPriority
    acctest.go:112: Skipping acceptance test for default account (f037e56e89293a057740de681ac9abbe). Default account is not configured for Magic Transit.
--- SKIP: TestAccCloudflareRuleset_MagicTransitUpdateWithHigherPriority (0.00s)
=== RUN   TestAccCloudflareRuleset_WAFManagedRulesetWithPayloadLogging
--- PASS: TestAccCloudflareRuleset_WAFManagedRulesetWithPayloadLogging (3.95s)
=== RUN   TestAccCloudflareRuleset_RateLimit
--- PASS: TestAccCloudflareRuleset_RateLimit (5.44s)
=== RUN   TestAccCloudflareRuleset_RateLimitScorePerPeriod
--- PASS: TestAccCloudflareRuleset_RateLimitScorePerPeriod (3.72s)
=== RUN   TestAccCloudflareRuleset_RateLimitMitigationTimeoutOfZero
=== PAUSE TestAccCloudflareRuleset_RateLimitMitigationTimeoutOfZero
=== RUN   TestAccCloudflareRuleset_CustomErrors
--- PASS: TestAccCloudflareRuleset_CustomErrors (3.86s)
=== RUN   TestAccCloudflareRuleset_RequestOrigin
--- PASS: TestAccCloudflareRuleset_RequestOrigin (3.92s)
=== RUN   TestAccCloudflareRuleset_RequestOriginPortWithoutHost
--- PASS: TestAccCloudflareRuleset_RequestOriginPortWithoutHost (3.77s)
=== RUN   TestAccCloudflareRuleset_TransformationRuleURIPath
--- PASS: TestAccCloudflareRuleset_TransformationRuleURIPath (3.57s)
=== RUN   TestAccCloudflareRuleset_TransformationRuleURIQuery
--- PASS: TestAccCloudflareRuleset_TransformationRuleURIQuery (4.34s)
=== RUN   TestAccCloudflareRuleset_TransformationRuleURIPathAndQueryCombination
--- PASS: TestAccCloudflareRuleset_TransformationRuleURIPathAndQueryCombination (3.69s)
=== RUN   TestAccCloudflareRuleset_TransformationRuleRequestHeaders
--- PASS: TestAccCloudflareRuleset_TransformationRuleRequestHeaders (4.24s)
=== RUN   TestAccCloudflareRuleset_TransformationRuleResponseHeaders
--- PASS: TestAccCloudflareRuleset_TransformationRuleResponseHeaders (4.23s)
=== RUN   TestAccCloudflareRuleset_ResponseCompression
--- PASS: TestAccCloudflareRuleset_ResponseCompression (3.47s)
=== RUN   TestAccCloudflareRuleset_ActionParametersMultipleSkips
--- PASS: TestAccCloudflareRuleset_ActionParametersMultipleSkips (4.94s)
=== RUN   TestAccCloudflareRuleset_ActionParametersOverridesAction
--- PASS: TestAccCloudflareRuleset_ActionParametersOverridesAction (4.13s)
=== RUN   TestAccCloudflareRuleset_ActionParametersHTTPDDoSOverride
--- PASS: TestAccCloudflareRuleset_ActionParametersHTTPDDoSOverride (3.75s)
=== RUN   TestAccCloudflareRuleset_ActionParametersOverrideAllRulesetRules
--- PASS: TestAccCloudflareRuleset_ActionParametersOverrideAllRulesetRules (5.02s)
=== RUN   TestAccCloudflareRuleset_AccountLevelCustomWAFRule
--- PASS: TestAccCloudflareRuleset_AccountLevelCustomWAFRule (5.56s)
=== RUN   TestAccCloudflareRuleset_ExposedCredentialCheck
--- PASS: TestAccCloudflareRuleset_ExposedCredentialCheck (3.05s)
=== RUN   TestAccCloudflareRuleset_Logging
--- PASS: TestAccCloudflareRuleset_Logging (3.53s)
=== RUN   TestAccCloudflareRuleset_ConditionallySetActionParameterVersion
--- PASS: TestAccCloudflareRuleset_ConditionallySetActionParameterVersion (6.89s)
=== RUN   TestAccCloudflareRuleset_WAFManagedRulesetWithActionManagedChallenge
--- PASS: TestAccCloudflareRuleset_WAFManagedRulesetWithActionManagedChallenge (8.88s)
=== RUN   TestAccCloudflareRuleset_LogCustomField
--- PASS: TestAccCloudflareRuleset_LogCustomField (3.57s)
=== RUN   TestAccCloudflareRuleset_ActionParametersOverridesThrashingStatus
--- PASS: TestAccCloudflareRuleset_ActionParametersOverridesThrashingStatus (21.58s)
=== RUN   TestAccCloudflareRuleset_CacheSettingsAllEnabled
--- PASS: TestAccCloudflareRuleset_CacheSettingsAllEnabled (4.36s)
=== RUN   TestAccCloudflareRuleset_CacheSettingsOptionalsEmpty
--- PASS: TestAccCloudflareRuleset_CacheSettingsOptionalsEmpty (4.53s)
=== RUN   TestAccCloudflareRuleset_CacheSettingsCacheReserveFalse
--- PASS: TestAccCloudflareRuleset_CacheSettingsCacheReserveFalse (3.60s)
=== RUN   TestAccCloudflareRuleset_CacheSettingsOnlyExludeOrigin
--- PASS: TestAccCloudflareRuleset_CacheSettingsOnlyExludeOrigin (3.56s)
=== RUN   TestAccCloudflareRuleset_CacheSettingsEdgeTTLRespectOrigin
--- PASS: TestAccCloudflareRuleset_CacheSettingsEdgeTTLRespectOrigin (4.09s)
=== RUN   TestAccCloudflareRuleset_CacheSettingsNoCacheForStatus
--- PASS: TestAccCloudflareRuleset_CacheSettingsNoCacheForStatus (4.18s)
=== RUN   TestAccCloudflareRuleset_CacheSettingsStatusRangeGreaterThan
--- PASS: TestAccCloudflareRuleset_CacheSettingsStatusRangeGreaterThan (4.06s)
=== RUN   TestAccCloudflareRuleset_CacheSettingsStatusRangeLessThan
--- PASS: TestAccCloudflareRuleset_CacheSettingsStatusRangeLessThan (3.93s)
=== RUN   TestAccCloudflareRuleset_CacheSettingsFalse
--- PASS: TestAccCloudflareRuleset_CacheSettingsFalse (3.69s)
=== RUN   TestAccCloudflareRuleset_Config
--- PASS: TestAccCloudflareRuleset_Config (3.62s)
=== RUN   TestAccCloudflareRuleset_Redirect
--- PASS: TestAccCloudflareRuleset_Redirect (4.80s)
=== RUN   TestAccCloudflareRuleset_DynamicRedirect
--- PASS: TestAccCloudflareRuleset_DynamicRedirect (3.34s)
=== RUN   TestAccCloudflareRuleset_DynamicRedirectWithoutPreservingQueryString
--- PASS: TestAccCloudflareRuleset_DynamicRedirectWithoutPreservingQueryString (3.69s)
=== RUN   TestAccCloudflareRuleset_TransformationRuleURIStripOffQueryString
--- PASS: TestAccCloudflareRuleset_TransformationRuleURIStripOffQueryString (3.63s)
=== RUN   TestAccCloudflareRuleset_TransformationRuleURIStripOffPath
--- PASS: TestAccCloudflareRuleset_TransformationRuleURIStripOffPath (3.83s)
=== RUN   TestAccCloudflareRuleset_ConfigSingleFalseyValue
--- PASS: TestAccCloudflareRuleset_ConfigSingleFalseyValue (3.96s)
=== RUN   TestAccCloudflareRuleset_ConfigConflictingCacheByDevice
--- PASS: TestAccCloudflareRuleset_ConfigConflictingCacheByDevice (0.29s)
=== RUN   TestAccCloudflareRuleset_CacheSettingsMissingEdgeTTLWithOverrideOrigin
--- PASS: TestAccCloudflareRuleset_CacheSettingsMissingEdgeTTLWithOverrideOrigin (0.18s)
=== RUN   TestAccCloudflareRuleset_CacheSettingsMissingBrowserTTLWithOverrideOrigin
--- PASS: TestAccCloudflareRuleset_CacheSettingsMissingBrowserTTLWithOverrideOrigin (0.18s)
=== RUN   TestAccCloudflareRuleset_CacheSettingsInvalidEdgeTTLWithOverrideOrigin
--- PASS: TestAccCloudflareRuleset_CacheSettingsInvalidEdgeTTLWithOverrideOrigin (0.18s)
=== RUN   TestAccCloudflareRuleset_CacheSettingsEdgeTTLWithBypassByDefault
--- PASS: TestAccCloudflareRuleset_CacheSettingsEdgeTTLWithBypassByDefault (4.07s)
=== RUN   TestAccCloudflareRuleset_CacheSettingsInvalidEdgeTTLWithBypassByDefault
--- PASS: TestAccCloudflareRuleset_CacheSettingsInvalidEdgeTTLWithBypassByDefault (0.26s)
=== RUN   TestAccCloudflareRuleset_CacheSettingsBrowserTTLWithBypass
--- PASS: TestAccCloudflareRuleset_CacheSettingsBrowserTTLWithBypass (3.68s)
=== RUN   TestAccCloudflareRuleset_CacheSettingsInvalidBrowserTTLWithBypass
--- PASS: TestAccCloudflareRuleset_CacheSettingsInvalidBrowserTTLWithBypass (0.24s)
=== RUN   TestAccCloudflareRuleset_CacheSettingsInvalidBrowserTTLWithOverrideOrigin
--- PASS: TestAccCloudflareRuleset_CacheSettingsInvalidBrowserTTLWithOverrideOrigin (0.18s)
=== RUN   TestAccCloudflareRuleset_CacheSettingsDefinedQueryStringExcludeKeys
--- PASS: TestAccCloudflareRuleset_CacheSettingsDefinedQueryStringExcludeKeys (4.68s)
=== RUN   TestAccCloudflareRuleset_CacheSettingsDefinedQueryStringIncludeKeys
--- PASS: TestAccCloudflareRuleset_CacheSettingsDefinedQueryStringIncludeKeys (3.96s)
=== RUN   TestAccCloudflareRuleset_CacheSettingsHandleDefaultHeaderExcludeOrigin
--- PASS: TestAccCloudflareRuleset_CacheSettingsHandleDefaultHeaderExcludeOrigin (13.00s)
=== RUN   TestAccCloudflareRuleset_ImportHandlesMissingValues
--- PASS: TestAccCloudflareRuleset_ImportHandlesMissingValues (0.35s)
=== CONT  TestAccCloudflareRuleset_RateLimitMitigationTimeoutOfZero
--- PASS: TestAccCloudflareRuleset_RateLimitMitigationTimeoutOfZero (3.80s)
PASS
ok  	github.com/cloudflare/terraform-provider-cloudflare/internal/framework/service/rulesets	269.358s

This logic tries to detect which rules are the same as before during an
update, and preserve the refs (and therefore IDs) of these rules.

However, this logic is fallible, since we cannot know the user's true
intention, and makes it difficult for us to remove the `version` and
`last_updated` fields from rules.

If users want to preserve rule refs across updates, they can now
actually use the `ref` field directly—and there is no need for this
remapping logic anymore.
These fields frequently change, leading to verbose diffs in plans. Since
these fields are not really useful within Terraform, we remove them from
the schema completely to prevent them from "polluting" diffs.

This also matches the behavior we have for the ruleset-level `version`
and `last_updated` fields, which are also not represented in the
Terraform schema.
In the Rulesets API, it is possible to prevent rule IDs from changing
across updates by using rule refs. If a rule's ref does not change as
part of an update, then neither will its ID.

However, Terraform does not know this and always reports that rule IDs
may change, which creates very large diffs. In this change, we use a
resource plan modifier to "teach" Terraform this rule.
This action parameter is used within the execute action, but cannot
actually be set to anything other than `latest`.

Removing this field from Terraform prevents an issue where it shows up
in diffs, due to it being a computed attribute.
@jacobbednarz jacobbednarz merged commit 3938c66 into cloudflare:master Dec 9, 2024
7 checks passed
@github-actions github-actions bot added this to the v4.48.0 milestone Dec 9, 2024
@zakcutner zakcutner deleted the rulesets branch December 9, 2024 21:21
jacobbednarz added a commit that referenced this pull request Dec 10, 2024
Follows on from #4697 to cleanup the state automatically.

Signed-off-by: Jacob Bednarz <[email protected]>
jacobbednarz added a commit that referenced this pull request Dec 10, 2024
Follows on from #4697 to cleanup the state automatically.

Signed-off-by: Jacob Bednarz <[email protected]>
jacobbednarz added a commit that referenced this pull request Dec 10, 2024
Follows on from #4697 to cleanup the state automatically.

Signed-off-by: Jacob Bednarz <[email protected]>
Copy link
Contributor

This functionality has been released in v4.48.0 of the Terraform Cloudflare Provider.

Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
2 participants