From c5b43b9f1a6cfca53190fe6578f503041fd42d85 Mon Sep 17 00:00:00 2001 From: Oleksandr Levchenkov Date: Fri, 20 Dec 2024 10:47:00 +0200 Subject: [PATCH] fix: unused config's create_before_destroy on resource change with no refresh (#2248) Signed-off-by: ollevche --- CHANGELOG.md | 2 + internal/command/apply_test.go | 77 +++++++++++++++++++ .../apply_cbd_with_refresh_false/main.tf | 3 + internal/tofu/context_apply.go | 7 +- internal/tofu/context_plan.go | 2 +- internal/tofu/graph_builder_apply.go | 5 +- internal/tofu/node_resource_plan_instance.go | 14 ++++ internal/tofu/transform_destroy_cbd.go | 9 +-- 8 files changed, 102 insertions(+), 17 deletions(-) create mode 100644 internal/command/testdata/apply_cbd_with_refresh_false/main.tf diff --git a/CHANGELOG.md b/CHANGELOG.md index 730c2b3cff..0005c70524 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,8 @@ ENHANCEMENTS: BUG FIXES: * `tofu init` command does not attempt to read encryption keys when `-backend=false` flag is set. (https://github.com/opentofu/opentofu/pull/2293) +* Changes in `create_before_destroy` for resources which require replacement are now properly handled when refresh is disabled. ([#2248](https://github.com/opentofu/opentofu/pull/2248)) + ## Previous Releases For information on prior major and minor releases, see their changelogs: diff --git a/internal/command/apply_test.go b/internal/command/apply_test.go index 06f48d9652..8e377e8cf4 100644 --- a/internal/command/apply_test.go +++ b/internal/command/apply_test.go @@ -2394,6 +2394,83 @@ func TestApply_showSensitiveArg(t *testing.T) { } } +func TestApply_CreateBeforeDestroyWithRefreshFalse(t *testing.T) { + td := t.TempDir() + testCopyDir(t, testFixturePath("apply_cbd_with_refresh_false"), td) + defer testChdir(t, td)() + + originalState := states.BuildState(func(s *states.SyncState) { + s.SetResourceInstanceCurrent( + addrs.Resource{ + Mode: addrs.ManagedResourceMode, + Type: "test_instance", + Name: "a", + }.Instance(addrs.NoKey).Absolute(addrs.RootModuleInstance), + &states.ResourceInstanceObjectSrc{ + AttrsJSON: []byte(`{"id":"foo"}`), + Status: states.ObjectReady, + // Create before destroy is set in the state and + // omitted in the configuration. + CreateBeforeDestroy: true, + }, + addrs.AbsProviderConfig{ + Provider: addrs.NewDefaultProvider("test"), + Module: addrs.RootModule, + }, + addrs.NoKey, + ) + }) + statePath := testStateFile(t, originalState) + + p := testProvider() + p.GetProviderSchemaResponse = &providers.GetProviderSchemaResponse{ + ResourceTypes: map[string]providers.Schema{ + "test_instance": { + Block: &configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "id": {Type: cty.String, Computed: true}, + }, + }, + }, + }, + } + // Resource must be replaced. + p.PlanResourceChangeFn = func(req providers.PlanResourceChangeRequest) providers.PlanResourceChangeResponse { + return providers.PlanResourceChangeResponse{ + PlannedState: req.ProposedNewState, + RequiresReplace: []cty.Path{{cty.GetAttrStep{Name: "id"}}}, + } + } + + view, done := testView(t) + c := &ApplyCommand{ + Meta: Meta{ + testingOverrides: metaOverridesForProvider(p), + View: view, + }, + } + + args := []string{ + "-state", statePath, + "-auto-approve", + // We are disabling refresh to see if cbd will be updated. + "-refresh=false", + } + + code := c.Run(args) + output := done(t) + if code != 0 { + t.Fatalf("Failed to run: \n%s", output.Stderr()) + } + + // If cbd is not updated, resource becomes detached and + // apply results in 1 added, 0 changed, 0 destroyed. + stdout := output.Stdout() + if !strings.Contains(stdout, "Apply complete! Resources: 1 added, 0 changed, 1 destroyed.") { + t.Fatalf("bad: output should contain 'notsensitive' output\n%s", stdout) + } +} + // applyFixtureSchema returns a schema suitable for processing the // configuration in testdata/apply . This schema should be // assigned to a mock provider named "test". diff --git a/internal/command/testdata/apply_cbd_with_refresh_false/main.tf b/internal/command/testdata/apply_cbd_with_refresh_false/main.tf new file mode 100644 index 0000000000..92d99ddaa6 --- /dev/null +++ b/internal/command/testdata/apply_cbd_with_refresh_false/main.tf @@ -0,0 +1,3 @@ +resource "test_instance" "a" { + id = "bar" +} diff --git a/internal/tofu/context_apply.go b/internal/tofu/context_apply.go index 5b572f919c..c4dfce6434 100644 --- a/internal/tofu/context_apply.go +++ b/internal/tofu/context_apply.go @@ -57,7 +57,7 @@ func (c *Context) Apply(ctx context.Context, plan *plans.Plan, config *configs.C providerFunctionTracker := make(ProviderFunctionMapping) - graph, operation, diags := c.applyGraph(plan, config, true, providerFunctionTracker) + graph, operation, diags := c.applyGraph(plan, config, providerFunctionTracker) if diags.HasErrors() { return nil, diags } @@ -123,8 +123,7 @@ Note that the -target and -exclude options are not suitable for routine use, and return newState, diags } -//nolint:revive,unparam // TODO remove validate bool as it's not used -func (c *Context) applyGraph(plan *plans.Plan, config *configs.Config, validate bool, providerFunctionTracker ProviderFunctionMapping) (*Graph, walkOperation, tfdiags.Diagnostics) { +func (c *Context) applyGraph(plan *plans.Plan, config *configs.Config, providerFunctionTracker ProviderFunctionMapping) (*Graph, walkOperation, tfdiags.Diagnostics) { var diags tfdiags.Diagnostics variables := InputValues{} @@ -211,7 +210,7 @@ func (c *Context) ApplyGraphForUI(plan *plans.Plan, config *configs.Config) (*Gr var diags tfdiags.Diagnostics - graph, _, moreDiags := c.applyGraph(plan, config, false, make(ProviderFunctionMapping)) + graph, _, moreDiags := c.applyGraph(plan, config, make(ProviderFunctionMapping)) diags = diags.Append(moreDiags) return graph, diags } diff --git a/internal/tofu/context_plan.go b/internal/tofu/context_plan.go index 4896eae0b9..586c0c26d5 100644 --- a/internal/tofu/context_plan.go +++ b/internal/tofu/context_plan.go @@ -291,7 +291,7 @@ func (c *Context) checkApplyGraph(plan *plans.Plan, config *configs.Config) tfdi return nil } log.Println("[DEBUG] building apply graph to check for errors") - _, _, diags := c.applyGraph(plan, config, true, make(ProviderFunctionMapping)) + _, _, diags := c.applyGraph(plan, config, make(ProviderFunctionMapping)) return diags } diff --git a/internal/tofu/graph_builder_apply.go b/internal/tofu/graph_builder_apply.go index c78a50ec9f..57d1dab958 100644 --- a/internal/tofu/graph_builder_apply.go +++ b/internal/tofu/graph_builder_apply.go @@ -187,10 +187,7 @@ func (b *ApplyGraphBuilder) Steps() []GraphTransformer { Changes: b.Changes, Operation: b.Operation, }, - &CBDEdgeTransformer{ - Config: b.Config, - State: b.State, - }, + &CBDEdgeTransformer{}, // We need to remove configuration nodes that are not used at all, as // they may not be able to evaluate, especially during destroy. diff --git a/internal/tofu/node_resource_plan_instance.go b/internal/tofu/node_resource_plan_instance.go index d330fd2cb3..37ce946f34 100644 --- a/internal/tofu/node_resource_plan_instance.go +++ b/internal/tofu/node_resource_plan_instance.go @@ -237,7 +237,21 @@ func (n *NodePlannableResourceInstance) managedResourceExecute(ctx EvalContext) log.Printf("[WARN] managedResourceExecute: no Managed config value found in instance state for %q", n.Addr) } else { if instanceRefreshState != nil { + prevCreateBeforeDestroy := instanceRefreshState.CreateBeforeDestroy + + // This change is usually written to the refreshState and then + // updated value used for further graph execution. However, with + // "refresh=false", refreshState is not being written and then + // some resources with updated configuration could be detached + // due to missaligned create_before_destroy in different graph nodes. instanceRefreshState.CreateBeforeDestroy = n.Config.Managed.CreateBeforeDestroy || n.ForceCreateBeforeDestroy + + if prevCreateBeforeDestroy != instanceRefreshState.CreateBeforeDestroy && n.skipRefresh { + diags = diags.Append(n.writeResourceInstanceState(ctx, instanceRefreshState, refreshState)) + if diags.HasErrors() { + return diags + } + } } } diff --git a/internal/tofu/transform_destroy_cbd.go b/internal/tofu/transform_destroy_cbd.go index 84a5d7719a..9348dc7a2c 100644 --- a/internal/tofu/transform_destroy_cbd.go +++ b/internal/tofu/transform_destroy_cbd.go @@ -9,9 +9,7 @@ import ( "fmt" "log" - "github.com/opentofu/opentofu/internal/configs" "github.com/opentofu/opentofu/internal/dag" - "github.com/opentofu/opentofu/internal/states" ) // GraphNodeDestroyerCBD must be implemented by nodes that might be @@ -115,12 +113,7 @@ func (t *ForcedCBDTransformer) hasCBDDescendent(g *Graph, v dag.Vertex) bool { // will get here by recording the CBD-ness of each change in the plan during // the plan walk and then forcing the nodes into the appropriate setting during // DiffTransformer when building the apply graph. -type CBDEdgeTransformer struct { - // Module and State are only needed to look up dependencies in - // any way possible. Either can be nil if not available. - Config *configs.Config - State *states.State -} +type CBDEdgeTransformer struct{} func (t *CBDEdgeTransformer) Transform(g *Graph) error { // Go through and reverse any destroy edges