From 4efd0ececa19b0634e9e429a117caf254b10d9ff Mon Sep 17 00:00:00 2001 From: Ilia Gogotchuri Date: Wed, 12 Feb 2025 16:43:06 +0400 Subject: [PATCH 1/5] implement GraphNodeDestroyerCBD for nodeExpandApplyableResource Signed-off-by: Ilia Gogotchuri --- internal/tofu/node_resource_apply.go | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/internal/tofu/node_resource_apply.go b/internal/tofu/node_resource_apply.go index 956e59235c..4bf6f1699b 100644 --- a/internal/tofu/node_resource_apply.go +++ b/internal/tofu/node_resource_apply.go @@ -19,6 +19,7 @@ type nodeExpandApplyableResource struct { } var ( + _ GraphNodeDestroyerCBD = (*nodeExpandApplyableResource)(nil) _ GraphNodeReferenceable = (*nodeExpandApplyableResource)(nil) _ GraphNodeReferencer = (*nodeExpandApplyableResource)(nil) _ GraphNodeConfigResource = (*nodeExpandApplyableResource)(nil) @@ -30,6 +31,21 @@ var ( func (n *nodeExpandApplyableResource) expandsInstances() { } +// CreateBeforeDestroy implementation for GraphNodeDestroyerCBD +func (n *nodeExpandApplyableResource) CreateBeforeDestroy() bool { + // If we have no config, we just assume no + if n.Config == nil || n.Config.Managed == nil { + return false + } + + return n.Config.Managed.CreateBeforeDestroy +} + +// ModifyCreateBeforeDestroy implementation for GraphNodeDestroyerCBD actually does nothing for this node type. +func (n *nodeExpandApplyableResource) ModifyCreateBeforeDestroy(_ bool) error { + return nil +} + func (n *nodeExpandApplyableResource) References() []*addrs.Reference { refs := n.NodeAbstractResource.References() From 02320f245c778e85d60889eee6561e0342dd644e Mon Sep 17 00:00:00 2001 From: Ilia Gogotchuri Date: Mon, 17 Feb 2025 10:15:20 +0400 Subject: [PATCH 2/5] Adds tests for create_before_destroy behaviour checking Signed-off-by: Ilia Gogotchuri --- internal/tofu/context_apply_test.go | 92 +++++++++++++++++++ internal/tofu/context_test.go | 81 ++++++++++++++++ .../apply-cbd-depends-non-cbd2/main.tf | 24 +++++ 3 files changed, 197 insertions(+) create mode 100644 internal/tofu/testdata/apply-cbd-depends-non-cbd2/main.tf diff --git a/internal/tofu/context_apply_test.go b/internal/tofu/context_apply_test.go index 01c44f1eb1..d12e53eaec 100644 --- a/internal/tofu/context_apply_test.go +++ b/internal/tofu/context_apply_test.go @@ -1124,6 +1124,98 @@ aws_instance.foo: require_new = yes type = aws_instance `) + + // Check that create_before_destroy was set on the foo resource + foo := state.RootModule().Resources["aws_instance.foo"].Instances[addrs.NoKey].Current + if !foo.CreateBeforeDestroy { + t.Fatalf("foo resource should have create_before_destroy set") + } +} + +// This tests that when a CBD (C) resource depends on a non-CBD (B) resource that depends on another CBD resource (A) +// Check that create_before_destroy is still set on the B resource after only the B resource is updated +func TestContext2Apply_createBeforeDestroy_dependsNonCBD2(t *testing.T) { + m := testModule(t, "apply-cbd-depends-non-cbd2") + p := testProviderBuiltin() + + state := states.NewState() + root := state.EnsureModule(addrs.RootModuleInstance) + root.SetResourceInstanceCurrent( + mustResourceInstanceAddr("terraform_data.A").Resource, + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(` + { + "id": "a1", + "triggers_replace": { + "value": { "version": 0 }, + "type": ["object", { "version": "number" }] + } + }`), + CreateBeforeDestroy: true, + }, + mustProviderConfig(`provider["terraform.io/builtin/terraform"]`), + addrs.NoKey, + ) + root.SetResourceInstanceCurrent( + mustResourceInstanceAddr("terraform_data.B").Resource, + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(` + { + "id": "b1", + "triggers_replace": { + "value": { + "base": "a1", + "version": 0 + }, + "type": ["object", { "base": "string", "version": "number" }] + } + }`), + Dependencies: []addrs.ConfigResource{mustConfigResourceAddr("terraform_data.A")}, + }, + mustProviderConfig(`provider["terraform.io/builtin/terraform"]`), + addrs.NoKey, + ) + root.SetResourceInstanceCurrent( + mustResourceInstanceAddr("terraform_data.C").Resource, + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(` + { + "id": "c1", + "triggers_replace": null + }`), + Dependencies: []addrs.ConfigResource{mustConfigResourceAddr("terraform_data.A"), mustConfigResourceAddr("terraform_data.B")}, + CreateBeforeDestroy: true, + }, + mustProviderConfig(`provider["terraform.io/builtin/terraform"]`), + addrs.NoKey, + ) + + ctx := testContext2(t, &ContextOpts{ + Providers: map[addrs.Provider]providers.Factory{ + addrs.NewBuiltInProvider("terraform"): testProviderFuncFixed(p), + }, + }) + + plan, diags := ctx.Plan(context.Background(), m, state, DefaultPlanOpts) + if diags.HasErrors() { + t.Fatalf("diags: %s", diags.Err()) + } else { + t.Logf(legacyDiffComparisonString(plan.Changes)) + } + + state, diags = ctx.Apply(context.Background(), plan, m) + if diags.HasErrors() { + t.Fatalf("diags: %s", diags.Err()) + } + + // Check that create_before_destroy was set on the foo resource + foo := state.RootModule().Resources["terraform_data.B"].Instances[addrs.NoKey].Current + if !foo.CreateBeforeDestroy { + t.Fatalf("foo resource should have create_before_destroy set") + } } func TestContext2Apply_createBeforeDestroy_hook(t *testing.T) { diff --git a/internal/tofu/context_test.go b/internal/tofu/context_test.go index a1244ebfdb..54292b8eea 100644 --- a/internal/tofu/context_test.go +++ b/internal/tofu/context_test.go @@ -10,6 +10,7 @@ import ( "bytes" "context" "fmt" + "github.com/hashicorp/go-uuid" "path/filepath" "sort" "strings" @@ -369,6 +370,86 @@ func testDiffFn(req providers.PlanResourceChangeRequest) (resp providers.PlanRes return } +// testProviderBuiltin returns a mock provider that implements the relevant parts of builtin provider schema for testing. +// Used to test with scenarios that are used for actual configs. Currently, ignores input and output attr handling. +func testProviderBuiltin() *MockProvider { + p := new(MockProvider) + p.GetProviderSchemaResponse = &providers.GetProviderSchemaResponse{ + ResourceTypes: map[string]providers.Schema{ + "terraform_data": { + Block: &configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "triggers_replace": {Type: cty.DynamicPseudoType, Optional: true}, + "id": {Type: cty.String, Computed: true}, + }, + }, + }, + }, + } + p.PlanResourceChangeFn = func(req providers.PlanResourceChangeRequest) providers.PlanResourceChangeResponse { + var resp providers.PlanResourceChangeResponse + if req.ProposedNewState.IsNull() { + // destroy op + resp.PlannedState = req.ProposedNewState + return resp + } + + planned := req.ProposedNewState.AsValueMap() + trigger := req.ProposedNewState.GetAttr("triggers_replace") + + switch { + case req.PriorState.IsNull(): + // Create + // Set the id value to unknown. + planned["id"] = cty.UnknownVal(cty.String).RefineNotNull() + + resp.PlannedState = cty.ObjectVal(planned) + return resp + + case !req.PriorState.GetAttr("triggers_replace").RawEquals(trigger): + // trigger changed, so we need to replace the entire instance + resp.RequiresReplace = append(resp.RequiresReplace, cty.GetAttrPath("triggers_replace")) + planned["id"] = cty.UnknownVal(cty.String).RefineNotNull() + } + + resp.PlannedState = cty.ObjectVal(planned) + return resp + } + + p.ApplyResourceChangeFn = func(req providers.ApplyResourceChangeRequest) providers.ApplyResourceChangeResponse { + var resp providers.ApplyResourceChangeResponse + if req.PlannedState.IsNull() { + resp.NewState = req.PlannedState + return resp + } + + newState := req.PlannedState.AsValueMap() + + if !req.PlannedState.GetAttr("id").IsKnown() { + idString, err := uuid.GenerateUUID() + // OpenTofu would probably never get this far without a good random + // source, but catch the error anyway. + if err != nil { + diag := tfdiags.AttributeValue( + tfdiags.Error, + "Error generating id", + err.Error(), + cty.GetAttrPath("id"), + ) + + resp.Diagnostics = resp.Diagnostics.Append(diag) + } + + newState["id"] = cty.StringVal(idString) + } + + resp.NewState = cty.ObjectVal(newState) + + return resp + } + return p +} + func testProvider(prefix string) *MockProvider { p := new(MockProvider) p.GetProviderSchemaResponse = testProviderSchema(prefix) diff --git a/internal/tofu/testdata/apply-cbd-depends-non-cbd2/main.tf b/internal/tofu/testdata/apply-cbd-depends-non-cbd2/main.tf new file mode 100644 index 0000000000..5bb4ba118a --- /dev/null +++ b/internal/tofu/testdata/apply-cbd-depends-non-cbd2/main.tf @@ -0,0 +1,24 @@ +resource "terraform_data" "A" { + triggers_replace = { + version = 0 + } + + lifecycle { + create_before_destroy = true + } +} + +resource "terraform_data" "B" { + triggers_replace = { + base = terraform_data.A.id + version = 1 + } +} + +resource "terraform_data" "C" { + depends_on = [terraform_data.B] + + lifecycle { + create_before_destroy = true + } +} From 9a902010e9381c77737aa23c2d0678463e9e1a2a Mon Sep 17 00:00:00 2001 From: Ilia Gogotchuri Date: Mon, 17 Feb 2025 10:18:37 +0400 Subject: [PATCH 3/5] Adds tests for create_before_destroy behaviour checking Signed-off-by: Ilia Gogotchuri --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 47a7ec0b11..603649a13f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -33,6 +33,7 @@ BUG FIXES: - `pg` backend doesn't fail on workspace creation for paralel runs, when the database is shared across multiple projects. ([#2411](https://github.com/opentofu/opentofu/pull/2411)) - Generating an OpenTofu configuration from an `import` block that is referencing a resource with nested attributes now works correctly, instead of giving an error that the nested computed attribute is required. ([#2372](https://github.com/opentofu/opentofu/issues/2372)) - `base64gunzip` now doesn't expose sensitive values if it fails during the base64 decoding. ([#2503](https://github.com/opentofu/opentofu/pull/2503)) +- Fix the issue with unexpected `create_before_destroy` (CBD) behavior, when CBD resource was depending on a non-CBD resource. ([#2508](https://github.com/opentofu/opentofu/pull/2508)) ## Previous Releases From 670d7a754f71c3629ad746f9725f8adba6ce0aac Mon Sep 17 00:00:00 2001 From: Ilia Gogotchuri Date: Mon, 17 Feb 2025 10:24:16 +0400 Subject: [PATCH 4/5] lint fix Signed-off-by: Ilia Gogotchuri --- internal/tofu/context_apply_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/tofu/context_apply_test.go b/internal/tofu/context_apply_test.go index d12e53eaec..7a7a8cf3b4 100644 --- a/internal/tofu/context_apply_test.go +++ b/internal/tofu/context_apply_test.go @@ -1203,7 +1203,7 @@ func TestContext2Apply_createBeforeDestroy_dependsNonCBD2(t *testing.T) { if diags.HasErrors() { t.Fatalf("diags: %s", diags.Err()) } else { - t.Logf(legacyDiffComparisonString(plan.Changes)) + t.Log(legacyDiffComparisonString(plan.Changes)) } state, diags = ctx.Apply(context.Background(), plan, m) From c9857aa81868fbbf6bba7e1c0cced57b53ff6e35 Mon Sep 17 00:00:00 2001 From: Ilia Gogotchuri Date: Mon, 17 Feb 2025 10:42:25 +0400 Subject: [PATCH 5/5] lint fix Signed-off-by: Ilia Gogotchuri --- internal/tofu/context_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/tofu/context_test.go b/internal/tofu/context_test.go index 54292b8eea..9d1a75c925 100644 --- a/internal/tofu/context_test.go +++ b/internal/tofu/context_test.go @@ -10,7 +10,6 @@ import ( "bytes" "context" "fmt" - "github.com/hashicorp/go-uuid" "path/filepath" "sort" "strings" @@ -19,6 +18,7 @@ import ( "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" + "github.com/hashicorp/go-uuid" "github.com/hashicorp/go-version" "github.com/opentofu/opentofu/internal/configs" "github.com/opentofu/opentofu/internal/configs/configload"