From 064e7f823d800c48f2dc221020b5acc7b63d7672 Mon Sep 17 00:00:00 2001 From: Daniel Walsh Date: Mon, 9 Dec 2024 13:15:03 +0000 Subject: [PATCH 1/9] Update issue sync owners of Pages/Workers --- tools/cmd/sync-github-issue-to-jira/main.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tools/cmd/sync-github-issue-to-jira/main.go b/tools/cmd/sync-github-issue-to-jira/main.go index b9d54a8d88..ecd8dac62c 100644 --- a/tools/cmd/sync-github-issue-to-jira/main.go +++ b/tools/cmd/sync-github-issue-to-jira/main.go @@ -128,7 +128,7 @@ var ( owner: "opayne", }, "service/workers": { - teamName: "Workers Core Platform", + teamName: "Workers Deploy & Config", owner: "laszlo", }, "service/tunnel": { @@ -152,8 +152,8 @@ var ( owner: "njones", }, "service/pages": { - teamName: "Workers Builds & Automation", - owner: "nrogers", + teamName: "Workers Deploy & Config", + owner: "laszlo", }, "service/bot_management": { teamName: "Bot Management", From 895f6c01720460da300e432e92245ae5a8018fac Mon Sep 17 00:00:00 2001 From: Zak Cutner Date: Tue, 3 Dec 2024 10:57:39 -0500 Subject: [PATCH 2/9] Remove logic to preserve ruleset rule refs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../framework/service/rulesets/resource.go | 159 +--------- .../service/rulesets/resource_test.go | 287 +----------------- 2 files changed, 9 insertions(+), 437 deletions(-) diff --git a/internal/framework/service/rulesets/resource.go b/internal/framework/service/rulesets/resource.go index ab0f93edfa..0357735baa 100644 --- a/internal/framework/service/rulesets/resource.go +++ b/internal/framework/service/rulesets/resource.go @@ -2,7 +2,6 @@ package rulesets import ( "context" - "encoding/json" "errors" "fmt" "reflect" @@ -130,10 +129,9 @@ func (r *RulesetResource) Create(ctx context.Context, req resource.CreateRequest Phase: rulesetPhase, } - rulesetData := data.toRuleset(ctx) - - if len(rulesetData.Rules) > 0 { - rs.Rules = rulesetData.Rules + rulesetRules := data.toRulesetRules(ctx) + if len(rulesetRules) > 0 { + rs.Rules = rulesetRules } ruleset, rulesetCreateErr := r.client.V1.CreateRuleset(ctx, identifier, rs) @@ -146,7 +144,7 @@ func (r *RulesetResource) Create(ctx context.Context, req resource.CreateRequest params := cfv1.UpdateEntrypointRulesetParams{ Phase: rulesetPhase, Description: rulesetDescription, - Rules: rulesetData.Rules, + Rules: rulesetRules, } var err error @@ -225,12 +223,6 @@ func (r *RulesetResource) Update(ctx context.Context, req resource.UpdateRequest accountID := plan.AccountID zoneID := plan.ZoneID.ValueString() - remappedRules, e := remapPreservedRuleRefs(ctx, state, plan) - if e != nil { - resp.Diagnostics.AddError("failed to remap rule IDs from state", e.Error()) - return - } - var identifier *cfv1.ResourceContainer if accountID.ValueString() != "" { identifier = cfv1.AccountIdentifier(accountID.ValueString()) @@ -241,7 +233,7 @@ func (r *RulesetResource) Update(ctx context.Context, req resource.UpdateRequest params := cfv1.UpdateRulesetParams{ ID: state.ID.ValueString(), Description: plan.Description.ValueString(), - Rules: remappedRules, + Rules: plan.toRulesetRules(ctx), } rs, err := r.client.V1.UpdateRuleset(ctx, identifier, params) if err != nil { @@ -790,18 +782,13 @@ func toRulesetResourceModel(ctx context.Context, zoneID, accountID basetypes.Str // // The reverse of this method is `toRulesetResourceModel` which handles building // a state representation using the API response. -func (r *RulesetResourceModel) toRuleset(ctx context.Context) cfv1.Ruleset { - var rs cfv1.Ruleset +func (r *RulesetResourceModel) toRulesetRules(ctx context.Context) []cfv1.RulesetRule { var rules []cfv1.RulesetRule - - rs.ID = r.ID.ValueString() for _, rule := range r.Rules { rules = append(rules, rule.toRulesetRule(ctx)) } - rs.Rules = rules - - return rs + return rules } // toRulesetRule takes a state representation of a Ruleset Rule and transforms @@ -1384,135 +1371,3 @@ func (r *RulesModel) toRulesetRule(ctx context.Context) cfv1.RulesetRule { return rr } - -// ruleRefs is a lookup table for rule IDs with two operations, add and pop. - -// We use add to populate the table from the old value of rules. We use pop to -// look up the ref for the new value of a rule (and remove it from the table). -// -// Internally, both operations serialize the rule to JSON and use the resulting -// string as the lookup key; the ref itself and other computed fields are -// excluded from the JSON. -// -// If a ruleset has multiple copies of the same rule, the copies have a single -// lookup key associated with multiple refs; we preserve order when adding and -// popping the refs. -type ruleRefs struct { - refs map[string][]string -} - -// newRuleRefs creates a new ruleRefs. -func newRuleRefs(rulesetRules []cfv1.RulesetRule, explicitRefs map[string]struct{}) (ruleRefs, error) { - r := ruleRefs{make(map[string][]string)} - for _, rule := range rulesetRules { - if rule.Ref == "" { - // This is unexpected. We only invoke this function for the old - // values of rules, which have their refs populated. - return ruleRefs{}, errors.New("unable to determine ID or ref of existing rule") - } - - if _, ok := explicitRefs[rule.Ref]; ok { - // We should not add explicitly-set refs, to avoid them being - // "stolen" by other rules. - continue - } - - if err := r.add(rule); err != nil { - return ruleRefs{}, err - } - } - - return r, nil -} - -// add stores a ref for the given rule. -func (r *ruleRefs) add(rule cfv1.RulesetRule) error { - key, err := ruleToKey(rule) - if err != nil { - return err - } - - r.refs[key] = append(r.refs[key], rule.Ref) - return nil -} - -// pop removes a ref for the given rule and returns it. If no ref was found for -// the rule, pop returns an empty string. -func (r *ruleRefs) pop(rule cfv1.RulesetRule) (string, error) { - key, err := ruleToKey(rule) - if err != nil { - return "", err - } - - refs := r.refs[key] - if len(refs) == 0 { - return "", nil - } - - ref, refs := refs[0], refs[1:] - r.refs[key] = refs - - return ref, nil -} - -// isEmpty returns true if the store does not contain any rule refs. -func (r *ruleRefs) isEmpty() bool { - return len(r.refs) == 0 -} - -// ruleToKey converts a ruleset rule to a key that can be used to track -// equivalent rules. Internally, it serializes the rule to JSON after removing -// computed fields. -func ruleToKey(rule cfv1.RulesetRule) (string, error) { - // For the purposes of preserving existing rule refs, we don't want to - // include computed fields as a part of the key value. - rule.ID = "" - rule.Ref = "" - rule.Version = nil - rule.LastUpdated = nil - - data, err := json.Marshal(rule) - if err != nil { - return "", err - } - - return string(data), nil -} - -// remapPreservedRuleRefs tries to preserve the refs of rules that have not -// changed in the ruleset, while also allowing users to explicitly set the ref -// if they choose to. -func remapPreservedRuleRefs(ctx context.Context, state, plan *RulesetResourceModel) ([]cfv1.RulesetRule, error) { - currentRuleset := state.toRuleset(ctx) - plannedRuleset := plan.toRuleset(ctx) - - plannedExplicitRefs := make(map[string]struct{}) - for _, rule := range plannedRuleset.Rules { - if rule.Ref != "" { - plannedExplicitRefs[rule.Ref] = struct{}{} - } - } - - refs, err := newRuleRefs(currentRuleset.Rules, plannedExplicitRefs) - if err != nil { - return nil, err - } - - if refs.isEmpty() { - // There are no rule refs when the ruleset is first created. - return plannedRuleset.Rules, nil - } - - for i := range plannedRuleset.Rules { - rule := &plannedRuleset.Rules[i] - - // We should not override refs that have been explicitly set. - if rule.Ref == "" { - if rule.Ref, err = refs.pop(*rule); err != nil { - return nil, err - } - } - } - - return plannedRuleset.Rules, nil -} diff --git a/internal/framework/service/rulesets/resource_test.go b/internal/framework/service/rulesets/resource_test.go index a6646520c3..49ea8be9a2 100644 --- a/internal/framework/service/rulesets/resource_test.go +++ b/internal/framework/service/rulesets/resource_test.go @@ -11,7 +11,6 @@ import ( "github.com/cloudflare/terraform-provider-cloudflare/internal/acctest" "github.com/cloudflare/terraform-provider-cloudflare/internal/utils" "github.com/hashicorp/terraform-plugin-testing/helper/resource" - "github.com/hashicorp/terraform-plugin-testing/terraform" ) func TestMain(m *testing.M) { @@ -851,167 +850,6 @@ func TestAccCloudflareRuleset_RateLimitMitigationTimeoutOfZero(t *testing.T) { }) } -func TestAccCloudflareRuleset_PreserveRuleRefs(t *testing.T) { - // Temporarily unset CLOUDFLARE_API_TOKEN if it is set as the WAF - // service does not yet support the API tokens and it results in - // misleading state error messages. - if os.Getenv("CLOUDFLARE_API_TOKEN") != "" { - t.Setenv("CLOUDFLARE_API_TOKEN", "") - } - - rnd := utils.GenerateRandomResourceName() - zoneID := os.Getenv("CLOUDFLARE_ZONE_ID") - resourceName := "cloudflare_ruleset." + rnd - - var adminRuleRef, loginRuleRef, adminRuleCopyRef, adminRuleExplicitRef string - - resource.Test(t, resource.TestCase{ - PreCheck: func() { acctest.TestAccPreCheck(t) }, - ProtoV6ProviderFactories: acctest.TestAccProtoV6ProviderFactories, - Steps: []resource.TestStep{ - { - // Create a ruleset with two rules (one for /admin, one for - // /login) and get their refs. - Config: testAccCheckCloudflareRulesetTwoCustomRules(rnd, zoneID), - Check: resource.ComposeTestCheckFunc( - resource.TestCheckResourceAttrWith(resourceName, "rules.0.ref", getValue(&adminRuleRef)), - resource.TestCheckResourceAttrWith(resourceName, "rules.1.ref", getValue(&loginRuleRef)), - ), - }, - { - // Reverse the order of rules. The refs should remain the same, - // just in reverse order. - Config: testAccCheckCloudflareRulesetTwoCustomRulesReversed(rnd, zoneID), - Check: resource.ComposeTestCheckFunc( - resource.TestCheckResourceAttrWith(resourceName, "rules.0.ref", equalsValue(&loginRuleRef)), - resource.TestCheckResourceAttrWith(resourceName, "rules.1.ref", equalsValue(&adminRuleRef)), - ), - }, - { - // Revert to the original version. - Config: testAccCheckCloudflareRulesetTwoCustomRules(rnd, zoneID), - Check: resource.ComposeTestCheckFunc( - resource.TestCheckResourceAttrWith(resourceName, "rules.0.ref", equalsValue(&adminRuleRef)), - resource.TestCheckResourceAttrWith(resourceName, "rules.1.ref", equalsValue(&loginRuleRef)), - ), - }, - { - // Append a copy of the admin rule. The first two refs should - // not change. - Config: testAccCheckCloudflareRulesetThreeCustomRules(rnd, zoneID, true), - Check: resource.ComposeTestCheckFunc( - resource.TestCheckResourceAttrWith(resourceName, "rules.0.ref", equalsValue(&adminRuleRef)), - resource.TestCheckResourceAttrWith(resourceName, "rules.1.ref", equalsValue(&loginRuleRef)), - resource.TestCheckResourceAttrWith(resourceName, "rules.2.ref", notEqualsValue(&adminRuleRef)), - resource.TestCheckResourceAttrWith(resourceName, "rules.2.ref", getValue(&adminRuleCopyRef)), - ), - }, - { - // Disable the login rule. Its ref will change, but the admin - // rule refs should remain the same. - Config: testAccCheckCloudflareRulesetThreeCustomRules(rnd, zoneID, false), - Check: resource.ComposeTestCheckFunc( - resource.TestCheckResourceAttrWith(resourceName, "rules.0.ref", equalsValue(&adminRuleRef)), - resource.TestCheckResourceAttrWith(resourceName, "rules.1.ref", notEqualsValue(&loginRuleRef)), - resource.TestCheckResourceAttrWith(resourceName, "rules.2.ref", equalsValue(&adminRuleCopyRef)), - ), - }, - { - // Revert to the original version. The preserved admin rule ref - // should stay the same, and the login rule ref should change. - Config: testAccCheckCloudflareRulesetTwoCustomRules(rnd, zoneID), - Check: resource.ComposeTestCheckFunc( - resource.TestCheckResourceAttrWith(resourceName, "rules.0.ref", equalsValue(&adminRuleRef)), - resource.TestCheckResourceAttrWith(resourceName, "rules.1.ref", notEqualsValue(&loginRuleRef)), - resource.TestCheckResourceAttrWith(resourceName, "rules.1.ref", getValue(&loginRuleRef)), - ), - }, - { - // Give the admin rule a ref. - Config: testAccCheckCloudflareRulesetTwoCustomRulesWithRef(rnd, zoneID, true), - Check: resource.ComposeTestCheckFunc( - resource.TestCheckResourceAttr(resourceName, "rules.0.ref", "foo"), - resource.TestCheckResourceAttrWith(resourceName, "rules.0.ref", getValue(&adminRuleExplicitRef)), - resource.TestCheckResourceAttrWith(resourceName, "rules.1.ref", equalsValue(&loginRuleRef)), - ), - }, - { - // Disable the admin rule. Its ref should stay the same. - Config: testAccCheckCloudflareRulesetTwoCustomRulesWithRef(rnd, zoneID, false), - Check: resource.ComposeTestCheckFunc( - resource.TestCheckResourceAttrWith(resourceName, "rules.0.ref", equalsValue(&adminRuleExplicitRef)), - resource.TestCheckResourceAttrWith(resourceName, "rules.1.ref", equalsValue(&loginRuleRef)), - ), - }, - { - // Prepend a copy of the admin rule without an explicit ref. The - // original rule should keep its explicit ref and the new rule - // should get a new ref. - Config: testAccCheckCloudflareRulesetThreeCustomRulesWithRef(rnd, zoneID), - Check: resource.ComposeTestCheckFunc( - resource.TestCheckResourceAttrWith(resourceName, "rules.0.ref", notEqualsValue(&adminRuleRef)), - resource.TestCheckResourceAttrWith(resourceName, "rules.0.ref", notEqualsValue(&adminRuleCopyRef)), - resource.TestCheckResourceAttrWith(resourceName, "rules.0.ref", notEqualsValue(&adminRuleExplicitRef)), - resource.TestCheckResourceAttrWith(resourceName, "rules.1.ref", equalsValue(&adminRuleExplicitRef)), - resource.TestCheckResourceAttrWith(resourceName, "rules.2.ref", equalsValue(&loginRuleRef)), - ), - }, - { - // Remove the prepended admin rule and re-enable the original - // admin rule. - Config: testAccCheckCloudflareRulesetTwoCustomRulesWithRef(rnd, zoneID, true), - Check: resource.ComposeTestCheckFunc( - resource.TestCheckResourceAttrWith(resourceName, "rules.0.ref", equalsValue(&adminRuleExplicitRef)), - resource.TestCheckResourceAttrWith(resourceName, "rules.1.ref", equalsValue(&loginRuleRef)), - ), - }, - { - // Revert to the original version. The refs should remain - // exactly the same. - Config: testAccCheckCloudflareRulesetTwoCustomRules(rnd, zoneID), - Check: resource.ComposeTestCheckFunc( - resource.TestCheckResourceAttrWith(resourceName, "rules.0.ref", equalsValue(&adminRuleExplicitRef)), - resource.TestCheckResourceAttrWith(resourceName, "rules.1.ref", equalsValue(&loginRuleRef)), - ), - }, - { - // Reverse the order of rules. The refs should remain the same, - // just in reverse order. - Config: testAccCheckCloudflareRulesetTwoCustomRulesReversed(rnd, zoneID), - Check: resource.ComposeTestCheckFunc( - resource.TestCheckResourceAttrWith(resourceName, "rules.0.ref", equalsValue(&loginRuleRef)), - resource.TestCheckResourceAttrWith(resourceName, "rules.1.ref", equalsValue(&adminRuleExplicitRef)), - ), - }, - }, - }) -} - -func getValue(result *string) func(string) error { - return func(value string) error { - *result = value - return nil - } -} - -func equalsValue(expected *string) func(string) error { - return func(value string) error { - if value != *expected { - return fmt.Errorf("expected '%s' got '%s'", *expected, value) - } - return nil - } -} - -func notEqualsValue(expected *string) func(string) error { - return func(value string) error { - if value == *expected { - return fmt.Errorf("expected != '%s'", *expected) - } - return nil - } -} - func TestAccCloudflareRuleset_CustomErrors(t *testing.T) { // Temporarily unset CLOUDFLARE_API_TOKEN if it is set as the WAF // service does not yet support the API tokens and it results in @@ -1477,7 +1315,7 @@ func TestAccCloudflareRuleset_ActionParametersHTTPDDoSOverride(t *testing.T) { resource.TestCheckResourceAttr(resourceName, "rules.0.action", "execute"), resource.TestCheckResourceAttr(resourceName, "rules.0.action_parameters.0.id", "4d21379b4f9f4bb088e0729962c8b3cf"), - resource.TestCheckResourceAttr(resourceName, "rules.0.action_parameters.0.overrides.0.rules.0.id", "fdfdac75430c4c47a959592f0aa5e68a"), + resource.TestCheckResourceAttr(resourceName, "rules.0.action_parameters.0.overrides.0.rules.0.id", "dd4d0a93c065441fb7c99729d96c7c08"), resource.TestCheckResourceAttr(resourceName, "rules.0.action_parameters.0.overrides.0.rules.0.sensitivity_level", "low"), resource.TestCheckResourceAttr(resourceName, "rules.0.expression", "true"), resource.TestCheckResourceAttr(resourceName, "rules.0.description", "override HTTP DDoS ruleset rule"), @@ -3560,123 +3398,6 @@ func testAccCheckCloudflareRulesetRateLimitWithMitigationTimeoutOfZero(rnd, name }`, rnd, name, zoneID, zoneName) } -func testAccCheckCloudflareRulesetTwoCustomRules(rnd, zoneID string) string { - return fmt.Sprintf(` - resource "cloudflare_ruleset" "%[1]s" { - zone_id = "%[2]s" - name = "Terraform provider test" - description = "%[1]s ruleset description" - kind = "zone" - phase = "http_request_firewall_custom" - rules { - action = "log" - enabled = true - expression = "(http.request.uri.path eq \"/admin\")" - } - rules { - action = "challenge" - enabled = true - expression = "(http.request.uri.path eq \"/login\")" - } - }`, rnd, zoneID) -} - -func testAccCheckCloudflareRulesetTwoCustomRulesReversed(rnd, zoneID string) string { - return fmt.Sprintf(` - resource "cloudflare_ruleset" "%[1]s" { - zone_id = "%[2]s" - name = "Terraform provider test" - description = "%[1]s ruleset description" - kind = "zone" - phase = "http_request_firewall_custom" - rules { - action = "challenge" - enabled = true - expression = "(http.request.uri.path eq \"/login\")" - } - rules { - action = "log" - enabled = true - expression = "(http.request.uri.path eq \"/admin\")" - } - }`, rnd, zoneID) -} - -func testAccCheckCloudflareRulesetThreeCustomRules(rnd, zoneID string, enableLoginRule bool) string { - return fmt.Sprintf(` - resource "cloudflare_ruleset" "%[1]s" { - zone_id = "%[2]s" - name = "Terraform provider test" - description = "%[1]s ruleset description" - kind = "zone" - phase = "http_request_firewall_custom" - rules { - action = "log" - enabled = true - expression = "(http.request.uri.path eq \"/admin\")" - } - rules { - action = "challenge" - enabled = %[3]t - expression = "(http.request.uri.path eq \"/login\")" - } - rules { - action = "log" - enabled = true - expression = "(http.request.uri.path eq \"/admin\")" - } - }`, rnd, zoneID, enableLoginRule) -} - -func testAccCheckCloudflareRulesetTwoCustomRulesWithRef(rnd, zoneID string, enableAdminRule bool) string { - return fmt.Sprintf(` - resource "cloudflare_ruleset" "%[1]s" { - zone_id = "%[2]s" - name = "Terraform provider test" - description = "%[1]s ruleset description" - kind = "zone" - phase = "http_request_firewall_custom" - rules { - action = "log" - enabled = %[3]t - expression = "(http.request.uri.path eq \"/admin\")" - ref = "foo" - } - rules { - action = "challenge" - enabled = true - expression = "(http.request.uri.path eq \"/login\")" - } - }`, rnd, zoneID, enableAdminRule) -} - -func testAccCheckCloudflareRulesetThreeCustomRulesWithRef(rnd, zoneID string) string { - return fmt.Sprintf(` - resource "cloudflare_ruleset" "%[1]s" { - zone_id = "%[2]s" - name = "Terraform provider test" - description = "%[1]s ruleset description" - kind = "zone" - phase = "http_request_firewall_custom" - rules { - action = "log" - enabled = false - expression = "(http.request.uri.path eq \"/admin\")" - } - rules { - action = "log" - enabled = false - expression = "(http.request.uri.path eq \"/admin\")" - ref = "foo" - } - rules { - action = "challenge" - enabled = true - expression = "(http.request.uri.path eq \"/login\")" - } - }`, rnd, zoneID) -} - func testAccCheckCloudflareRulesetActionParametersOverridesActionEnabled(rnd, name, zoneID, zoneName string) string { return fmt.Sprintf(` resource "cloudflare_ruleset" "%[1]s" { @@ -3784,7 +3505,7 @@ func testAccCheckCloudflareRulesetActionParametersHTTPDDosOverride(rnd, name, zo id = "4d21379b4f9f4bb088e0729962c8b3cf" overrides { rules { - id = "fdfdac75430c4c47a959592f0aa5e68a" # requests with odd HTTP headers or URI path + id = "dd4d0a93c065441fb7c99729d96c7c08" # HTTP requests from known botnet sensitivity_level = "low" } } @@ -5057,7 +4778,3 @@ func testAccCloudflareRulesetCacheSettingsBypassBrowserInvalid(rnd, zoneID strin } `, rnd, zoneID) } - -func testAccCheckCloudflareRulesetDestroy(s *terraform.State) error { - return nil -} From 4fd67ce413f0f3a4bbb975474f36fcaafb642547 Mon Sep 17 00:00:00 2001 From: Zak Cutner Date: Tue, 3 Dec 2024 13:29:41 -0500 Subject: [PATCH 3/9] 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. --- internal/framework/service/rulesets/model.go | 2 -- .../framework/service/rulesets/resource.go | 18 ------------------ internal/framework/service/rulesets/schema.go | 8 -------- 3 files changed, 28 deletions(-) diff --git a/internal/framework/service/rulesets/model.go b/internal/framework/service/rulesets/model.go index 82083cf054..e39c30ddd5 100644 --- a/internal/framework/service/rulesets/model.go +++ b/internal/framework/service/rulesets/model.go @@ -14,7 +14,6 @@ type RulesetResourceModel struct { } type RulesModel struct { - Version types.String `tfsdk:"version"` Action types.String `tfsdk:"action"` ActionParameters []*ActionParametersModel `tfsdk:"action_parameters"` Description types.String `tfsdk:"description"` @@ -22,7 +21,6 @@ type RulesModel struct { ExposedCredentialCheck []*ExposedCredentialCheckModel `tfsdk:"exposed_credential_check"` Expression types.String `tfsdk:"expression"` ID types.String `tfsdk:"id"` - LastUpdated types.String `tfsdk:"last_updated"` Logging []*LoggingModel `tfsdk:"logging"` Ratelimit []*RatelimitModel `tfsdk:"ratelimit"` Ref types.String `tfsdk:"ref"` diff --git a/internal/framework/service/rulesets/resource.go b/internal/framework/service/rulesets/resource.go index 0357735baa..54842794ef 100644 --- a/internal/framework/service/rulesets/resource.go +++ b/internal/framework/service/rulesets/resource.go @@ -7,7 +7,6 @@ import ( "reflect" "sort" "strings" - "time" cfv1 "github.com/cloudflare/cloudflare-go" "github.com/cloudflare/terraform-provider-cloudflare/internal/framework/expanders" @@ -313,13 +312,6 @@ func toRulesetResourceModel(ctx context.Context, zoneID, accountID basetypes.Str Expression: flatteners.String(ruleResponse.Expression), Description: types.StringValue(ruleResponse.Description), Enabled: flatteners.Bool(ruleResponse.Enabled), - Version: flatteners.String(cfv1.String(ruleResponse.Version)), - } - - if ruleResponse.LastUpdated != nil { - rule.LastUpdated = types.StringValue(ruleResponse.LastUpdated.String()) - } else { - rule.LastUpdated = types.StringNull() } // action_parameters @@ -797,7 +789,6 @@ func (r *RulesModel) toRulesetRule(ctx context.Context) cfv1.RulesetRule { rr := cfv1.RulesetRule{ ID: r.ID.ValueString(), Ref: r.Ref.ValueString(), - Version: r.Version.ValueStringPointer(), Action: r.Action.ValueString(), Expression: r.Expression.ValueString(), Description: r.Description.ValueString(), @@ -1360,14 +1351,5 @@ func (r *RulesModel) toRulesetRule(ctx context.Context) cfv1.RulesetRule { } } - if !r.LastUpdated.IsNull() { - if lastUpdated, err := time.Parse( - "2006-01-02 15:04:05.999999999 -0700 MST", - r.LastUpdated.ValueString(), - ); err == nil { - rr.LastUpdated = &lastUpdated - } - } - return rr } diff --git a/internal/framework/service/rulesets/schema.go b/internal/framework/service/rulesets/schema.go index 928813301c..21d94a79fc 100644 --- a/internal/framework/service/rulesets/schema.go +++ b/internal/framework/service/rulesets/schema.go @@ -109,10 +109,6 @@ func (r *RulesetResource) Schema(ctx context.Context, req resource.SchemaRequest Computed: true, MarkdownDescription: "Unique rule identifier.", }, - "version": schema.StringAttribute{ - Computed: true, - MarkdownDescription: "Version of the ruleset to deploy.", - }, "ref": schema.StringAttribute{ Optional: true, Computed: true, @@ -145,10 +141,6 @@ func (r *RulesetResource) Schema(ctx context.Context, req resource.SchemaRequest }, Optional: true, }, - "last_updated": schema.StringAttribute{ - Computed: true, - MarkdownDescription: "The most recent update to this rule.", - }, }, Blocks: map[string]schema.Block{ "action_parameters": schema.ListNestedBlock{ From 34060ffd2c38bd6a17424c5671ee50334bb7d47a Mon Sep 17 00:00:00 2001 From: Zak Cutner Date: Tue, 3 Dec 2024 13:35:47 -0500 Subject: [PATCH 4/9] 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. --- .../framework/service/rulesets/resource.go | 43 +++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/internal/framework/service/rulesets/resource.go b/internal/framework/service/rulesets/resource.go index 54842794ef..f2c6fe7f86 100644 --- a/internal/framework/service/rulesets/resource.go +++ b/internal/framework/service/rulesets/resource.go @@ -1353,3 +1353,46 @@ func (r *RulesModel) toRulesetRule(ctx context.Context) cfv1.RulesetRule { return rr } + +func (r *RulesetResource) ModifyPlan(ctx context.Context, req resource.ModifyPlanRequest, resp *resource.ModifyPlanResponse) { + var state *RulesetResourceModel + resp.Diagnostics.Append(req.State.Get(ctx, &state)...) + if resp.Diagnostics.HasError() { + return + } + + var plan *RulesetResourceModel + resp.Diagnostics.Append(req.Plan.Get(ctx, &plan)...) + if resp.Diagnostics.HasError() { + return + } + + // Do nothing if there is no state or no plan. + if state == nil || plan == nil { + return + } + + ruleIDsByRef := make(map[string]types.String) + for _, rule := range state.Rules { + if ref := rule.Ref.ValueString(); ref != "" { + ruleIDsByRef[ref] = rule.ID + } + } + + for _, rule := range plan.Rules { + // Do nothing if the rule's ID is a known planned value. + if !rule.ID.IsUnknown() { + continue + } + + // If the rule's ref matches a rule in the state, populate the planned + // value of its ID with the corresponding ID from the state. + if ref := rule.Ref.ValueString(); ref != "" { + if id, ok := ruleIDsByRef[ref]; ok { + rule.ID = id + } + } + } + + resp.Diagnostics.Append(resp.Plan.Set(ctx, plan)...) +} From 9088d8d37cb0bc3ab57cdd825e452ee12bfb5d81 Mon Sep 17 00:00:00 2001 From: Zak Cutner Date: Tue, 3 Dec 2024 13:39:26 -0500 Subject: [PATCH 5/9] 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. --- internal/framework/service/rulesets/model.go | 1 - internal/framework/service/rulesets/resource.go | 7 ------- internal/framework/service/rulesets/schema.go | 5 ----- 3 files changed, 13 deletions(-) diff --git a/internal/framework/service/rulesets/model.go b/internal/framework/service/rulesets/model.go index e39c30ddd5..36c27b1f1e 100644 --- a/internal/framework/service/rulesets/model.go +++ b/internal/framework/service/rulesets/model.go @@ -27,7 +27,6 @@ type RulesModel struct { } type ActionParametersModel struct { - Version types.String `tfsdk:"version"` AdditionalCacheablePorts types.Set `tfsdk:"additional_cacheable_ports"` AutomaticHTTPSRewrites types.Bool `tfsdk:"automatic_https_rewrites"` AutoMinify []*ActionParameterAutoMinifyModel `tfsdk:"autominify"` diff --git a/internal/framework/service/rulesets/resource.go b/internal/framework/service/rulesets/resource.go index f2c6fe7f86..84902ef3dc 100644 --- a/internal/framework/service/rulesets/resource.go +++ b/internal/framework/service/rulesets/resource.go @@ -343,7 +343,6 @@ func toRulesetResourceModel(ctx context.Context, zoneID, accountID basetypes.Str OriginErrorPagePassthru: flatteners.Bool(ruleResponse.ActionParameters.OriginErrorPagePassthru), RespectStrongEtags: flatteners.Bool(ruleResponse.ActionParameters.RespectStrongETags), ReadTimeout: flatteners.Int64(int64(cfv1.Uint(ruleResponse.ActionParameters.ReadTimeout))), - Version: flatteners.String(cfv1.String(ruleResponse.ActionParameters.Version)), }) if !reflect.ValueOf(ruleResponse.ActionParameters.Polish).IsNil() { @@ -842,12 +841,6 @@ func (r *RulesModel) toRulesetRule(ctx context.Context) cfv1.RulesetRule { rr.ActionParameters.Ruleset = ap.Ruleset.ValueString() } - if !ap.Version.IsNull() { - if ap.Version.ValueString() != "" { - rr.ActionParameters.Version = cfv1.StringPtr(ap.Version.ValueString()) - } - } - if !ap.Increment.IsNull() { rr.ActionParameters.Increment = int(ap.Increment.ValueInt64()) } diff --git a/internal/framework/service/rulesets/schema.go b/internal/framework/service/rulesets/schema.go index 21d94a79fc..dafa268ffb 100644 --- a/internal/framework/service/rulesets/schema.go +++ b/internal/framework/service/rulesets/schema.go @@ -303,11 +303,6 @@ func (r *RulesetResource) Schema(ctx context.Context, req resource.SchemaRequest Optional: true, MarkdownDescription: "Pass-through error page for origin.", }, - "version": schema.StringAttribute{ - Computed: true, - Optional: true, - MarkdownDescription: "Version of the ruleset to deploy.", - }, }, Blocks: map[string]schema.Block{ "algorithms": schema.ListNestedBlock{ From 9894c3a34312d0f3257a82381d4bfa8ed250f579 Mon Sep 17 00:00:00 2001 From: Zak Cutner Date: Tue, 3 Dec 2024 15:00:05 -0500 Subject: [PATCH 6/9] Add changelog --- .changelog/4697.txt | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 .changelog/4697.txt diff --git a/.changelog/4697.txt b/.changelog/4697.txt new file mode 100644 index 0000000000..2fd951a01f --- /dev/null +++ b/.changelog/4697.txt @@ -0,0 +1,7 @@ +```release-note:enhancement +resource/cloudflare_ruleset: improve diffs when only some rules are changed +``` + +```release-note:note +resource/cloudflare_ruleset: rules must now be given an explicit `ref` to avoid their IDs changing across ruleset updates, see https://developers.cloudflare.com/terraform/troubleshooting/rule-id-changes/ +``` From 9669bf92151686ff2698b01bd04263eff5ee25bd Mon Sep 17 00:00:00 2001 From: Michele Russo Date: Tue, 10 Dec 2024 10:27:05 +0100 Subject: [PATCH 7/9] Fix update method --- .../service/leaked_credential_check_rule/resource.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/internal/framework/service/leaked_credential_check_rule/resource.go b/internal/framework/service/leaked_credential_check_rule/resource.go index 454fc00701..1e90cf7625 100644 --- a/internal/framework/service/leaked_credential_check_rule/resource.go +++ b/internal/framework/service/leaked_credential_check_rule/resource.go @@ -113,6 +113,14 @@ func (r *LeakedCredentialCheckRuleResource) Update(ctx context.Context, req reso if resp.Diagnostics.HasError() { return } + var state LeakedCredentialCheckRulesModel + diags = req.State.Get(ctx, &state) + resp.Diagnostics.Append(diags...) + if resp.Diagnostics.HasError() { + return + } + data.ID = state.ID + zoneID := cloudflare.ZoneIdentifier(data.ZoneID.ValueString()) _, err := r.client.V1.LeakedCredentialCheckUpdateDetection(ctx, zoneID, cloudflare.LeakedCredentialCheckUpdateDetectionParams{ LeakedCredentialCheckDetectionEntry: cloudflare.LeakedCredentialCheckDetectionEntry{ From 4595f238114d802563ef2d6b63855c1a6569d046 Mon Sep 17 00:00:00 2001 From: Michele Russo Date: Tue, 10 Dec 2024 10:27:23 +0100 Subject: [PATCH 8/9] Expand acceptance test --- .../resource_test.go | 29 ++++++++++++++++++- 1 file changed, 28 insertions(+), 1 deletion(-) diff --git a/internal/framework/service/leaked_credential_check_rule/resource_test.go b/internal/framework/service/leaked_credential_check_rule/resource_test.go index 62714a892d..14643192bf 100644 --- a/internal/framework/service/leaked_credential_check_rule/resource_test.go +++ b/internal/framework/service/leaked_credential_check_rule/resource_test.go @@ -46,7 +46,7 @@ func testSweepCloudflareLCCRules(r string) error { tflog.Error(ctx, fmt.Sprintf("Error deleting a user-defined detection patter for Leaked Credential Check: %s", err)) } } - + return nil } @@ -68,6 +68,18 @@ func TestAccCloudflareLeakedCredentialCheckRule_Basic(t *testing.T) { resource.TestCheckResourceAttr(name+"_first", "username", "lookup_json_string(http.request.body.raw, \"user\")"), resource.TestCheckResourceAttr(name+"_first", "password", "lookup_json_string(http.request.body.raw, \"pass\")"), + resource.TestCheckResourceAttr(name+"_second", "zone_id", zoneID), + resource.TestCheckResourceAttr(name+"_second", "username", "lookup_json_string(http.request.body.raw, \"id\")"), + resource.TestCheckResourceAttr(name+"_second", "password", "lookup_json_string(http.request.body.raw, \"secret\")"), + ), + }, + { + Config: testAccConfigAddHeader(rnd, zoneID, testAccLCCUpdateOneRule(rnd)), + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr(name+"_first", "zone_id", zoneID), + resource.TestCheckResourceAttr(name+"_first", "username", "lookup_json_string(http.request.body.raw, \"username\")"), + resource.TestCheckResourceAttr(name+"_first", "password", "lookup_json_string(http.request.body.raw, \"password\")"), + resource.TestCheckResourceAttr(name+"_second", "zone_id", zoneID), resource.TestCheckResourceAttr(name+"_second", "username", "lookup_json_string(http.request.body.raw, \"id\")"), resource.TestCheckResourceAttr(name+"_second", "password", "lookup_json_string(http.request.body.raw, \"secret\")"), @@ -100,3 +112,18 @@ func testAccLCCTwoSimpleRules(name string) string { password = "lookup_json_string(http.request.body.raw, \"secret\")" }`, name) } + +func testAccLCCUpdateOneRule(name string) string { + return fmt.Sprintf(` + resource "cloudflare_leaked_credential_check_rule" "%[1]s_first" { + zone_id = cloudflare_leaked_credential_check.%[1]s.zone_id + username = "lookup_json_string(http.request.body.raw, \"username\")" + password = "lookup_json_string(http.request.body.raw, \"password\")" + } + + resource "cloudflare_leaked_credential_check_rule" "%[1]s_second" { + zone_id = cloudflare_leaked_credential_check.%[1]s.zone_id + username = "lookup_json_string(http.request.body.raw, \"id\")" + password = "lookup_json_string(http.request.body.raw, \"secret\")" + }`, name) +} From 01883bcb5fd36123375d8ec574c8ed84e1d6e24e Mon Sep 17 00:00:00 2001 From: Michele Russo Date: Tue, 10 Dec 2024 10:36:57 +0100 Subject: [PATCH 9/9] Add changelog --- .changelog/4741.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 .changelog/4741.txt diff --git a/.changelog/4741.txt b/.changelog/4741.txt new file mode 100644 index 0000000000..4c9de3b447 --- /dev/null +++ b/.changelog/4741.txt @@ -0,0 +1,3 @@ +```release-note:bug +resource/cloudflare_leaked_credential_check_rule: Fix bug in update method +``` \ No newline at end of file