From c995fb6b80ca5e446883ec6996b16f8af09487cb Mon Sep 17 00:00:00 2001 From: Vaishak Dinesh Date: Fri, 9 Oct 2026 16:56:51 -0700 Subject: [PATCH] feat(e2e): heal corrupted leaked_credential_check_rule state; clean up create-only resources Two related e2e harness robustness fixes, both live-tested against v5.19.0 (all 5 steps passed, 0 unmatched drift): 1. healCorruptedLeakedCredentialCheckRuleState: detects and repairs a known v4 provider bug where cloudflare_leaked_credential_check_rule's Read() silently zeroes id/username/password to null instead of erroring when the tracked resource no longer matches reality (e.g. deleted outside Terraform), permanently breaking every subsequent Update() with "required missing detection ID". Runs after v4 init, before v4 plan; no-op if the resource is healthy or not in state. The identical bug exists in content_scanning_expression's Read() too, but that resource has no tf-migrate migrator/testdata so it's out of scope here. 2. cleanupCreateOnlyResources (Step 5): after a fully successful run, parses the already-captured v5 plan text for resources with a pure "will be created" action (no moved {} or import {} block protecting them) and destroys exactly those, leaving the real v4 base and every adopted v5 resource untouched. Insurance against resources with no v4 counterpart silently accumulating duplicates across repeated runs, since v5-side state is never persisted between invocations. Opt out with --keep-created. Deliberately parses plan TEXT rather than `terraform show -json `: the JSON form always marshals the full prior state, so one unrelated resource elsewhere in state with a stale schema makes the whole call fail even though the actual -target plan succeeded. Confirmed working live: destroyed 7 genuinely create-only resources (zone/account singletons and conditional zone_setting instances) while correctly leaving leaked_credential_check_rule and all moved/import-protected resources untouched. Both include unit tests for their safety-critical pure logic (text parsing, import-block protection, state corruption detection). --- cmd/e2e-runner/main.go | 2 + internal/e2e-runner/cleanup_test.go | 209 +++++++++++++++++++++ internal/e2e-runner/runner.go | 269 ++++++++++++++++++++++++++++ 3 files changed, 480 insertions(+) create mode 100644 internal/e2e-runner/cleanup_test.go diff --git a/cmd/e2e-runner/main.go b/cmd/e2e-runner/main.go index afcaa072..24434a79 100644 --- a/cmd/e2e-runner/main.go +++ b/cmd/e2e-runner/main.go @@ -60,6 +60,7 @@ var runCmd = &cobra.Command{ TargetProviderVersion: cmd.Flag("target-provider-version").Value.String(), VersionSuffix: cmd.Flag("version-suffix").Value.String(), Clean: cmd.Flag("clean").Changed, + KeepCreated: cmd.Flag("keep-created").Changed, } return e2e.RunE2ETests(cfg) }, @@ -191,6 +192,7 @@ func init() { runCmd.Flags().String("target-provider-version", "", "Explicit provider version to set in required_providers (e.g. 5.19.0-beta.3); skips GitHub API lookup") runCmd.Flags().String("version-suffix", "", "Isolate local dirs + R2 state key from the shared, unversioned ones used by default runs. Defaults to --target-provider-version if not set — pass this only to override that default (e.g. to share one isolated slot across two versions, or to use a suffix unrelated to the provider version).") runCmd.Flags().Bool("clean", false, "Destroy test infrastructure (v4 and v5 sides) on exit, success or failure. Recommended for version-isolated supportability-matrix runs against a shared test account/zone.") + runCmd.Flags().Bool("keep-created", false, "Skip the automatic cleanup of resources this run newly created with no prior real-world identity (no moved {} or import {} block protecting them). That cleanup runs by default on every successful run to prevent unbounded duplicate accumulation across repeated runs (see e2e/SUPPORTABILITY_MATRIX.md §10). Pass this only to deliberately inspect freshly-created resources after a local debug run.") // Clean command flags cleanCmd.Flags().String("modules", "", "Modules to remove from state (comma-separated)") diff --git a/internal/e2e-runner/cleanup_test.go b/internal/e2e-runner/cleanup_test.go new file mode 100644 index 00000000..87cce3ac --- /dev/null +++ b/internal/e2e-runner/cleanup_test.go @@ -0,0 +1,209 @@ +package e2e + +import ( + "os" + "path/filepath" + "testing" +) + +func TestIsStateShowIDNull(t *testing.T) { + tests := []struct { + name string + output string + want bool + }{ + { + name: "corrupted state (known bug signature)", + output: `# module.leaked_credential_check_rule.cloudflare_leaked_credential_check_rule.basic: +resource "cloudflare_leaked_credential_check_rule" "basic" { + id = null + password = null + username = null + zone_id = "cd581854c1f59f8c686ee796d0eddce2" +} +`, + want: true, + }, + { + name: "healthy state with a real id", + output: `# module.leaked_credential_check_rule.cloudflare_leaked_credential_check_rule.basic: +resource "cloudflare_leaked_credential_check_rule" "basic" { + id = "53d55269c789408baa1b052e2239eacd" + password = (sensitive value) + username = (sensitive value) + zone_id = "cd581854c1f59f8c686ee796d0eddce2" +} +`, + want: false, + }, + { + name: "unrelated null field does not false-positive", + output: "resource \"x\" \"y\" {\n name = null\n id = \"real-id\"\n}\n", + want: false, + }, + { + name: "empty output", + output: "", + want: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := isStateShowIDNull(tt.output) + if got != tt.want { + t.Errorf("got %v, want %v", got, tt.want) + } + }) + } +} + +func TestExtractPureCreateAddresses(t *testing.T) { + tests := []struct { + name string + output string + want []string + }{ + { + name: "pure create is included", + output: " # module.queue.cloudflare_queue.minimal will be created\n", + want: []string{"module.queue.cloudflare_queue.minimal"}, + }, + { + name: "update in-place, destroy, and import are excluded", + output: "" + + " # module.list.cloudflare_list.redirect_list will be updated in-place\n" + + " # module.x.cloudflare_x.y will be destroyed\n" + + " # module.argo.cloudflare_argo_tiered_caching.both_with_lifecycle_tiered will be imported\n", + want: nil, + }, + { + name: "replace (must be replaced) is excluded — a real resource already exists", + output: " # module.certificate_pack.cloudflare_certificate_pack.by_config[\"google-90d\"] must be replaced\n", + want: nil, + }, + { + name: "mixed set returns only the pure creates, in order", + output: "" + + " # module.a.t.one will be created\n" + + " # module.a.t.two will be updated in-place\n" + + " # module.a.t.three must be replaced\n" + + " # module.a.t.four will be created\n", + want: []string{"module.a.t.one", "module.a.t.four"}, + }, + { + name: "a line merely containing \"will be created\" as trailing prose is not matched without the leading '#' header", + output: "This resource will be created automatically.\n", + want: nil, + }, + { + name: "empty output", + output: "", + want: nil, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := extractPureCreateAddresses(tt.output) + if len(got) != len(tt.want) { + t.Fatalf("got %v, want %v", got, tt.want) + } + for i := range got { + if got[i] != tt.want[i] { + t.Errorf("index %d: got %q, want %q", i, got[i], tt.want[i]) + } + } + }) + } +} + +func TestFindImportToAddresses(t *testing.T) { + t.Run("root-hoisted import block (already module-qualified) is used as-is", func(t *testing.T) { + dir := t.TempDir() + writeTFFile(t, dir, "main.tf", ` +import { + to = module.argo.cloudflare_argo_tiered_caching.both_with_lifecycle_tiered + id = "cd581854c1f59f8c686ee796d0eddce2" +} +`) + got, err := findImportToAddresses(dir) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + want := "module.argo.cloudflare_argo_tiered_caching.both_with_lifecycle_tiered" + if !got[want] { + t.Errorf("expected %q to be protected, got %v", want, got) + } + }) + + t.Run("module-local import block gets the module prefix reconstructed", func(t *testing.T) { + dir := t.TempDir() + mustMkdir(t, filepath.Join(dir, "leaked_credential_check_rule")) + writeTFFile(t, filepath.Join(dir, "leaked_credential_check_rule"), "leaked_credential_check_rule.tf", ` +import { + to = cloudflare_leaked_credential_check_rule.basic + id = "zone123/detection456" +} +`) + got, err := findImportToAddresses(dir) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + want := "module.leaked_credential_check_rule.cloudflare_leaked_credential_check_rule.basic" + if !got[want] { + t.Errorf("expected %q to be protected, got %v", want, got) + } + }) + + t.Run("no import blocks returns an empty, non-nil-error result", func(t *testing.T) { + dir := t.TempDir() + writeTFFile(t, dir, "main.tf", ` +resource "cloudflare_queue" "minimal" { + account_id = var.cloudflare_account_id + name = "test" +} +`) + got, err := findImportToAddresses(dir) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(got) != 0 { + t.Errorf("expected no protected addresses, got %v", got) + } + }) + + t.Run("moved blocks are ignored by the import scanner", func(t *testing.T) { + dir := t.TempDir() + writeTFFile(t, dir, "main.tf", ` +moved { + from = cloudflare_record.example + to = cloudflare_dns_record.example +} +`) + got, err := findImportToAddresses(dir) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(got) != 0 { + t.Errorf("expected moved blocks to be ignored, got %v", got) + } + }) +} + +// writeTFFile is a small helper shared by this file's tests to write a .tf +// file under dir, creating parent directories if needed. +func writeTFFile(t *testing.T, dir, filename, content string) { + t.Helper() + path := filepath.Join(dir, filename) + if err := os.WriteFile(path, []byte(content), 0644); err != nil { + t.Fatalf("failed to write %s: %v", path, err) + } +} + +func mustMkdir(t *testing.T, dir string) { + t.Helper() + if err := os.MkdirAll(dir, 0755); err != nil { + t.Fatalf("failed to create dir %s: %v", dir, err) + } +} diff --git a/internal/e2e-runner/runner.go b/internal/e2e-runner/runner.go index 3bf4818b..a809757d 100644 --- a/internal/e2e-runner/runner.go +++ b/internal/e2e-runner/runner.go @@ -16,11 +16,13 @@ package e2e import ( + "bufio" "encoding/json" "fmt" "os" "os/exec" "path/filepath" + "regexp" "strings" "github.com/hashicorp/hcl/v2" @@ -59,6 +61,15 @@ type RunConfig struct { // limited resources (ruleset phase slots, spectrum IPv4 quota, unique // hostnames, etc.), where leftover resources from one leg break the next. Clean bool + // KeepCreated, when true, skips the automatic cleanup of resources this + // run newly created with no prior real-world identity (no moved {} or + // import {} block protecting them — see cleanupCreateOnlyResources). + // That cleanup runs by default on every successful run specifically to + // prevent the kind of unbounded duplicate accumulation documented in + // e2e/SUPPORTABILITY_MATRIX.md §10 (the Access Policy and Gateway + // Certificate quota incidents). Pass this only to deliberately inspect + // freshly-created resources after a local debug run. + KeepCreated bool } // testContext holds shared state for e2e test execution @@ -741,6 +752,28 @@ func RunE2ETests(cfg *RunConfig) error { return fmt.Errorf("migration produced drift") } + // Step 5: Clean up create-only resources (see cleanupCreateOnlyResources). + // Only reached on this fully-successful path — a failed run's resources + // are left in place for debugging, and --keep-created opts out entirely. + if !cfg.KeepCreated { + fmt.Println() + printCyan("Step 5: Cleaning up create-only test resources") + destroyed, cleanupErr := cleanupCreateOnlyResources(v5TF, v5Dir, ctx.v5PlanOutput, tmpDir, cfg) + if cleanupErr != nil { + printYellow("Warning: cleanup did not fully succeed: %v", cleanupErr) + printYellow("This does not affect the test result above, which already passed.") + printYellow("See %s for details. If this keeps recurring for the same resource,", filepath.Join(tmpDir, "v5-cleanup-destroy.log")) + printYellow("it may need to be excluded from cleanup or investigated manually.") + } else if len(destroyed) == 0 { + printSuccess("No create-only resources needed cleanup") + } else { + printSuccess("Destroyed %d create-only resource(s):", len(destroyed)) + for _, addr := range destroyed { + printYellow(" - %s", addr) + } + } + } + return nil } @@ -1048,6 +1081,64 @@ func checkAndDisplayDrift(planOutput string, cfg *RunConfig, stage string, resou return result } +// healCorruptedLeakedCredentialCheckRuleState detects and repairs a known v4 +// provider bug: cloudflare_leaked_credential_check_rule's Read() lists all +// detection patterns and loops looking for one matching state's id, but if +// none match (e.g. because the real pattern was deleted outside Terraform) +// it does not error — it silently writes a zero-value result back to state, +// leaving id/username/password all null. Every subsequent Update() then +// fails with "required missing detection ID", and the resource can never +// recover on its own (confirmed directly in the v4 provider source, +// internal/framework/service/leaked_credential_check_rule/resource.go). +// +// The identical anti-pattern also exists in +// internal/framework/service/content_scanning_expression/resource.go (same +// "doesn't offer a single get operation" comment, same no-match-found +// handling), but that resource has no tf-migrate migrator or testdata, so +// it's not in scope here — only leaked_credential_check_rule is actually +// exercised by this harness today. +// +// This runs after v4 init, before v4 plan, looking for that corruption +// signature (id == null) in the module's state and removing the entry so +// the next plan/apply cleanly recreates it instead of repeatedly failing. +// Safe to run every time: if the resource is healthy or not present in +// state at all, this is a no-op. +func healCorruptedLeakedCredentialCheckRuleState(v4TF *TerraformRunner) error { + const addr = "module.leaked_credential_check_rule.cloudflare_leaked_credential_check_rule.basic" + + showOutput, err := v4TF.Run("state", "show", addr) + if err != nil { + // Not in state (not targeted this run, or never created yet) — nothing to heal. + return nil + } + + if !isStateShowIDNull(showOutput) { + return nil + } + + printYellow("Detected corrupted state for %s (id is null — known v4 Read() bug after out-of-band deletion). Removing so it recreates cleanly...", addr) + if _, err := v4TF.Run("state", "rm", addr); err != nil { + return fmt.Errorf("failed to remove corrupted state for %s: %w", addr, err) + } + printSuccess("Removed corrupted state entry for %s", addr) + return nil +} + +// isStateShowIDNull reports whether `terraform state show ` output has +// a top-level `id = null` line — the exact corruption signature left by the +// leaked_credential_check_rule v4 provider bug (a healthy resource always +// shows `id = ""`). +func isStateShowIDNull(stateShowOutput string) bool { + scanner := bufio.NewScanner(strings.NewReader(stateShowOutput)) + for scanner.Scan() { + line := strings.TrimSpace(scanner.Text()) + if strings.HasPrefix(line, "id") && strings.Contains(line, "= null") { + return true + } + } + return false +} + // findMovedFromAddresses scans every .tf file under v5Dir for `moved { from = ... }` // blocks and returns the set of full state addresses (e.g. // "module.zero_trust_access_policy.cloudflare_access_policy.example") that those @@ -1119,6 +1210,180 @@ func findMovedFromAddresses(v5Dir string) (map[string]bool, error) { return protected, nil } +// findImportToAddresses scans every .tf file under v5Dir for `import { to = ... }` +// blocks and returns the set of resource addresses they target. +// +// This exists so the create-only-resource cleanup step (see cleanupCreateOnlyResources) +// can never destroy a resource that an import {} block says already exists for real — +// Terraform's plan JSON reports an import-driven resource with the same "create" action +// as a genuinely brand-new one, so action alone can't distinguish "safe to destroy test +// fixture" from "do not touch, this is a real pre-existing resource" (e.g. +// cloudflare_argo_tiered_caching, a DLP predefined profile, zero_trust_organization). +// +// tf-migrate normally hoists import {} blocks to the root main.tf, where "to" is already +// fully module-qualified (module...