From 3e224df3794e9f21a015a749e7163f9be3896e64 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 23 Dec 2020 12:25:44 -0500 Subject: [PATCH 1/4] destroy plan with provider config from data --- terraform/context_apply_test.go | 65 ++++++++++++++++++- .../testdata/apply-destroy-data-cycle/main.tf | 4 ++ 2 files changed, 66 insertions(+), 3 deletions(-) diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index a29ba8730..4d1c80f7c 100644 --- a/terraform/context_apply_test.go +++ b/terraform/context_apply_test.go @@ -1275,6 +1275,7 @@ func TestContext2Apply_destroyDependsOnStateOnly(t *testing.T) { } func testContext2Apply_destroyDependsOnStateOnly(t *testing.T, state *states.State) { + state = state.DeepCopy() m := testModule(t, "empty") p := testProvider("aws") p.ApplyResourceChangeFn = testApplyFn @@ -1371,6 +1372,7 @@ func TestContext2Apply_destroyDependsOnStateOnlyModule(t *testing.T) { } func testContext2Apply_destroyDependsOnStateOnlyModule(t *testing.T, state *states.State) { + state = state.DeepCopy() m := testModule(t, "empty") p := testProvider("aws") p.ApplyResourceChangeFn = testApplyFn @@ -1451,6 +1453,12 @@ func TestContext2Apply_destroyData(t *testing.T) { p := testProvider("null") p.ApplyResourceChangeFn = testApplyFn p.PlanResourceChangeFn = testDiffFn + p.ReadDataSourceFn = func(req providers.ReadDataSourceRequest) providers.ReadDataSourceResponse { + return providers.ReadDataSourceResponse{ + State: req.Config, + } + } + state := states.NewState() root := state.EnsureModule(addrs.RootModuleInstance) root.SetResourceInstanceCurrent( @@ -1493,6 +1501,8 @@ func TestContext2Apply_destroyData(t *testing.T) { } wantHookCalls := []*testHookCall{ + {"PreDiff", "data.null_data_source.testing"}, + {"PostDiff", "data.null_data_source.testing"}, {"PreDiff", "data.null_data_source.testing"}, {"PostDiff", "data.null_data_source.testing"}, {"PostStateUpdate", ""}, @@ -9561,6 +9571,18 @@ func TestContext2Apply_destroyDataCycle(t *testing.T) { p := testProvider("null") p.ApplyResourceChangeFn = testApplyFn p.PlanResourceChangeFn = testDiffFn + p.ReadDataSourceFn = func(req providers.ReadDataSourceRequest) providers.ReadDataSourceResponse { + return providers.ReadDataSourceResponse{ + State: cty.ObjectVal(map[string]cty.Value{ + "id": cty.StringVal("new"), + "foo": cty.NullVal(cty.String), + }), + } + } + + tp := testProvider("test") + tp.ApplyResourceChangeFn = testApplyFn + tp.PlanResourceChangeFn = testDiffFn state := states.NewState() root := state.EnsureModule(addrs.RootModuleInstance) @@ -9579,6 +9601,31 @@ func TestContext2Apply_destroyDataCycle(t *testing.T) { Module: addrs.RootModule, }, ) + root.SetResourceInstanceCurrent( + addrs.Resource{ + Mode: addrs.ManagedResourceMode, + Type: "test_resource", + Name: "a", + }.Instance(addrs.IntKey(0)), + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"id":"a"}`), + Dependencies: []addrs.ConfigResource{ + addrs.ConfigResource{ + Resource: addrs.Resource{ + Mode: addrs.DataResourceMode, + Type: "null_data_source", + Name: "d", + }, + Module: addrs.RootModule, + }, + }, + }, + addrs.AbsProviderConfig{ + Provider: addrs.NewDefaultProvider("test"), + Module: addrs.RootModule, + }, + ) root.SetResourceInstanceCurrent( addrs.Resource{ Mode: addrs.DataResourceMode, @@ -9587,7 +9634,7 @@ func TestContext2Apply_destroyDataCycle(t *testing.T) { }.Instance(addrs.NoKey), &states.ResourceInstanceObjectSrc{ Status: states.ObjectReady, - AttrsJSON: []byte(`{"id":"data"}`), + AttrsJSON: []byte(`{"id":"old"}`), }, addrs.AbsProviderConfig{ Provider: addrs.NewDefaultProvider("null"), @@ -9597,15 +9644,14 @@ func TestContext2Apply_destroyDataCycle(t *testing.T) { Providers := map[addrs.Provider]providers.Factory{ addrs.NewDefaultProvider("null"): testProviderFuncFixed(p), + addrs.NewDefaultProvider("test"): testProviderFuncFixed(tp), } - hook := &testHook{} ctx := testContext2(t, &ContextOpts{ Config: m, Providers: Providers, State: state, Destroy: true, - Hooks: []Hook{hook}, }) plan, diags := ctx.Plan() @@ -9627,6 +9673,19 @@ func TestContext2Apply_destroyDataCycle(t *testing.T) { t.Fatalf("failed to create context for plan: %s", diags.Err()) } + tp.ConfigureFn = func(req providers.ConfigureRequest) (resp providers.ConfigureResponse) { + foo := req.Config.GetAttr("foo") + if !foo.IsKnown() { + resp.Diagnostics = resp.Diagnostics.Append(fmt.Errorf("unknown config value foo")) + return resp + } + + if foo.AsString() != "new" { + resp.Diagnostics = resp.Diagnostics.Append(fmt.Errorf("wrong config value: %q", foo.AsString())) + } + return resp + } + _, diags = ctx.Apply() if diags.HasErrors() { t.Fatalf("diags: %s", diags.Err()) diff --git a/terraform/testdata/apply-destroy-data-cycle/main.tf b/terraform/testdata/apply-destroy-data-cycle/main.tf index bd72a47e3..591af8200 100644 --- a/terraform/testdata/apply-destroy-data-cycle/main.tf +++ b/terraform/testdata/apply-destroy-data-cycle/main.tf @@ -8,3 +8,7 @@ data "null_data_source" "d" { resource "null_resource" "a" { count = local.l == "NONE" ? 1 : 0 } + +provider "test" { + foo = data.null_data_source.d.id +} From 0b3b84acc1ade2e394fcb7e3b956bf2ea0789eb1 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Mon, 4 Jan 2021 17:17:35 -0500 Subject: [PATCH 2/4] refresh state during a destroy plan Because the destroy plan only creates the necessary changes for apply to remove all the resources, it does no reading of resources or data sources, leading to stale data in the state. In most cases this is not a problem, but when a provider configuration is using resource values, the provider may not be able to run correctly during apply. In prior versions of terraform, the implicit refresh that happened during `terraform destroy` would update the data sources and remove missing resources from state as required. The destroy plan graph has a minimal amount of information, so it is not feasible to work the reading of resources into the operation without completely replicating the normal plan graph, and updating the plan graph and all destroy node implementation is also a considerable amount of refactoring. Instead, we can run a normal plan which is used to refresh the state before creating the destroy plan. This brings back similar behavior to core versions prior to 0.14, and the refresh can still be skipped using the `-refresh=false` cli flag. --- terraform/context.go | 78 +++++++++++++++---- terraform/graph_builder_destroy_plan.go | 8 +- terraform/node_resource_abstract_instance.go | 7 ++ .../apply-destroy-data-resource/main.tf | 4 +- .../plan-module-destroy-gh-1835/b/main.tf | 2 +- 5 files changed, 79 insertions(+), 20 deletions(-) diff --git a/terraform/context.go b/terraform/context.go index 7e24de341..34dea4c87 100644 --- a/terraform/context.go +++ b/terraform/context.go @@ -547,44 +547,96 @@ The -target option is not for routine use, and is provided only for exceptional varVals[k] = dv } - p := &plans.Plan{ + plan := &plans.Plan{ VariableValues: varVals, TargetAddrs: c.targets, ProviderSHA256s: c.providerSHA256s, } - operation := walkPlan - graphType := GraphTypePlan - if c.destroy { - operation = walkPlanDestroy - graphType = GraphTypePlanDestroy + switch { + case c.destroy: + diags = diags.Append(c.destroyPlan(plan)) + default: + diags = diags.Append(c.plan(plan)) } - graph, graphDiags := c.Graph(graphType, nil) + return plan, diags +} + +func (c *Context) plan(plan *plans.Plan) tfdiags.Diagnostics { + var diags tfdiags.Diagnostics + + graph, graphDiags := c.Graph(GraphTypePlan, nil) diags = diags.Append(graphDiags) if graphDiags.HasErrors() { - return nil, diags + return diags } // Do the walk - walker, walkDiags := c.walk(graph, operation) + walker, walkDiags := c.walk(graph, walkPlan) diags = diags.Append(walker.NonFatalDiagnostics) diags = diags.Append(walkDiags) if walkDiags.HasErrors() { - return nil, diags + return diags } - p.Changes = c.changes + plan.Changes = c.changes c.refreshState.SyncWrapper().RemovePlannedResourceInstanceObjects() refreshedState := c.refreshState.DeepCopy() - p.State = refreshedState + plan.State = refreshedState // replace the working state with the updated state, so that immediate calls // to Apply work as expected. c.state = refreshedState - return p, diags + return diags +} + +func (c *Context) destroyPlan(destroyPlan *plans.Plan) tfdiags.Diagnostics { + var diags tfdiags.Diagnostics + c.changes = plans.NewChanges() + + // A destroy plan starts by running Refresh to read any pending data + // sources, and remove missing managed resources. This is required because + // a "destroy plan" is only creating delete changes, and is essentially a + // local operation. + if !c.skipRefresh { + refreshPlan := &plans.Plan{ + VariableValues: destroyPlan.VariableValues, + TargetAddrs: c.targets, + ProviderSHA256s: c.providerSHA256s, + } + + refreshDiags := c.plan(refreshPlan) + + diags = diags.Append(refreshDiags) + if diags.HasErrors() { + return diags + } + + // insert the refreshed state into the destroy plan result, and discard + // the changes recorded from the refresh. + destroyPlan.State = refreshPlan.State + c.changes = plans.NewChanges() + } + + graph, graphDiags := c.Graph(GraphTypePlanDestroy, nil) + diags = diags.Append(graphDiags) + if graphDiags.HasErrors() { + return diags + } + + // Do the walk + walker, walkDiags := c.walk(graph, walkPlan) + diags = diags.Append(walker.NonFatalDiagnostics) + diags = diags.Append(walkDiags) + if walkDiags.HasErrors() { + return diags + } + + destroyPlan.Changes = c.changes + return diags } // Refresh goes through all the resources in the state and refreshes them diff --git a/terraform/graph_builder_destroy_plan.go b/terraform/graph_builder_destroy_plan.go index d82162282..a16729507 100644 --- a/terraform/graph_builder_destroy_plan.go +++ b/terraform/graph_builder_destroy_plan.go @@ -12,7 +12,10 @@ import ( // planning a pure-destroy. // // Planning a pure destroy operation is simple because we can ignore most -// ordering configuration and simply reverse the state. +// ordering configuration and simply reverse the state. This graph mainly +// exists for targeting, because we need to walk the destroy dependencies to +// ensure we plan the required resources. Without the requirement for +// targeting, the plan could theoretically be created directly from the state. type DestroyPlanGraphBuilder struct { // Config is the configuration tree to build the plan from. Config *configs.Config @@ -72,6 +75,7 @@ func (b *DestroyPlanGraphBuilder) Steps() []GraphTransformer { State: b.State, }, + // Create the delete changes for root module outputs. &OutputTransformer{ Config: b.Config, Destroy: true, @@ -93,8 +97,6 @@ func (b *DestroyPlanGraphBuilder) Steps() []GraphTransformer { Schemas: b.Schemas, }, - // Target. Note we don't set "Destroy: true" here since we already - // created proper destroy ordering. &TargetsTransformer{Targets: b.Targets}, // Close opened plugin connections diff --git a/terraform/node_resource_abstract_instance.go b/terraform/node_resource_abstract_instance.go index 417d94bf1..3278759e5 100644 --- a/terraform/node_resource_abstract_instance.go +++ b/terraform/node_resource_abstract_instance.go @@ -1401,6 +1401,13 @@ func (n *NodeAbstractResourceInstance) planDataSource(ctx EvalContext, currentSt return plannedChange, plannedNewState, diags } + // While this isn't a "diff", continue to call this for data sources. + diags = diags.Append(ctx.Hook(func(h Hook) (HookAction, error) { + return h.PreDiff(n.Addr, states.CurrentGen, priorVal, configVal) + })) + if diags.HasErrors() { + return nil, nil, diags + } // We have a complete configuration with no dependencies to wait on, so we // can read the data source into the state. newVal, readDiags := n.readDataSource(ctx, configVal) diff --git a/terraform/testdata/apply-destroy-data-resource/main.tf b/terraform/testdata/apply-destroy-data-resource/main.tf index cb16d9f34..0d941a707 100644 --- a/terraform/testdata/apply-destroy-data-resource/main.tf +++ b/terraform/testdata/apply-destroy-data-resource/main.tf @@ -1,5 +1,3 @@ data "null_data_source" "testing" { - inputs = { - test = "yes" - } + foo = "yes" } diff --git a/terraform/testdata/plan-module-destroy-gh-1835/b/main.tf b/terraform/testdata/plan-module-destroy-gh-1835/b/main.tf index c3b0270b0..3b0cc6664 100644 --- a/terraform/testdata/plan-module-destroy-gh-1835/b/main.tf +++ b/terraform/testdata/plan-module-destroy-gh-1835/b/main.tf @@ -1,5 +1,5 @@ variable "a_id" {} resource "aws_instance" "b" { - command = "echo ${var.a_id}" + foo = "echo ${var.a_id}" } From e614fb9aed597252e6ca18244ca779e0d61d3683 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Mon, 4 Jan 2021 18:27:08 -0500 Subject: [PATCH 3/4] refresh is expected for destroy These tests were not previously running a refresh, and hence did not expect the resources to be read. --- backend/local/backend_plan_test.go | 16 ++-------------- backend/local/testdata/destroy-with-ds/main.tf | 1 + 2 files changed, 3 insertions(+), 14 deletions(-) diff --git a/backend/local/backend_plan_test.go b/backend/local/backend_plan_test.go index 5c0545380..3f72168f1 100644 --- a/backend/local/backend_plan_test.go +++ b/backend/local/backend_plan_test.go @@ -559,7 +559,7 @@ func TestLocal_planDestroy(t *testing.T) { b, cleanup := TestLocal(t) defer cleanup() - p := TestLocalProvider(t, b, "test", planFixtureSchema()) + TestLocalProvider(t, b, "test", planFixtureSchema()) testStateFile(t, b.StatePath, testPlanState()) outDir := testTempDir(t) @@ -593,10 +593,6 @@ func TestLocal_planDestroy(t *testing.T) { t.Fatalf("plan operation failed") } - if p.ReadResourceCalled { - t.Fatal("ReadResource should not be called") - } - if run.PlanEmpty { t.Fatal("plan should not be empty") } @@ -613,7 +609,7 @@ func TestLocal_planDestroy_withDataSources(t *testing.T) { b, cleanup := TestLocal(t) defer cleanup() - p := TestLocalProvider(t, b, "test", planFixtureSchema()) + TestLocalProvider(t, b, "test", planFixtureSchema()) testStateFile(t, b.StatePath, testPlanState_withDataSource()) b.CLI = cli.NewMockUi() @@ -649,14 +645,6 @@ func TestLocal_planDestroy_withDataSources(t *testing.T) { t.Fatalf("plan operation failed") } - if p.ReadResourceCalled { - t.Fatal("ReadResource should not be called") - } - - if p.ReadDataSourceCalled { - t.Fatal("ReadDataSourceCalled should not be called") - } - if run.PlanEmpty { t.Fatal("plan should not be empty") } diff --git a/backend/local/testdata/destroy-with-ds/main.tf b/backend/local/testdata/destroy-with-ds/main.tf index 4ee80ea3d..7062d896b 100644 --- a/backend/local/testdata/destroy-with-ds/main.tf +++ b/backend/local/testdata/destroy-with-ds/main.tf @@ -1,4 +1,5 @@ resource "test_instance" "foo" { + count = 1 ami = "bar" } From fb2208a6d9f9927179475e808f87068e49b2a81f Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 5 Jan 2021 17:40:41 -0500 Subject: [PATCH 4/4] refactor context plan get rid of the mutation of the plans.Plan and return values. --- terraform/context.go | 63 +++++++++++++++++++++++--------------------- 1 file changed, 33 insertions(+), 30 deletions(-) diff --git a/terraform/context.go b/terraform/context.go index 34dea4c87..a7fa0fb3e 100644 --- a/terraform/context.go +++ b/terraform/context.go @@ -530,6 +530,20 @@ The -target option is not for routine use, and is provided only for exceptional )) } + var plan *plans.Plan + var planDiags tfdiags.Diagnostics + switch { + case c.destroy: + plan, planDiags = c.destroyPlan() + default: + plan, planDiags = c.plan() + } + diags = diags.Append(planDiags) + if diags.HasErrors() { + return nil, diags + } + + // convert the variables into the format expected for the plan varVals := make(map[string]plans.DynamicValue, len(c.variables)) for k, iv := range c.variables { // We use cty.DynamicPseudoType here so that we'll save both the @@ -547,29 +561,22 @@ The -target option is not for routine use, and is provided only for exceptional varVals[k] = dv } - plan := &plans.Plan{ - VariableValues: varVals, - TargetAddrs: c.targets, - ProviderSHA256s: c.providerSHA256s, - } - - switch { - case c.destroy: - diags = diags.Append(c.destroyPlan(plan)) - default: - diags = diags.Append(c.plan(plan)) - } + // insert the run-specific data from the context into the plan; variables, + // targets and provider SHAs. + plan.VariableValues = varVals + plan.TargetAddrs = c.targets + plan.ProviderSHA256s = c.providerSHA256s return plan, diags } -func (c *Context) plan(plan *plans.Plan) tfdiags.Diagnostics { +func (c *Context) plan() (*plans.Plan, tfdiags.Diagnostics) { var diags tfdiags.Diagnostics graph, graphDiags := c.Graph(GraphTypePlan, nil) diags = diags.Append(graphDiags) if graphDiags.HasErrors() { - return diags + return nil, diags } // Do the walk @@ -577,9 +584,11 @@ func (c *Context) plan(plan *plans.Plan) tfdiags.Diagnostics { diags = diags.Append(walker.NonFatalDiagnostics) diags = diags.Append(walkDiags) if walkDiags.HasErrors() { - return diags + return nil, diags + } + plan := &plans.Plan{ + Changes: c.changes, } - plan.Changes = c.changes c.refreshState.SyncWrapper().RemovePlannedResourceInstanceObjects() @@ -590,11 +599,12 @@ func (c *Context) plan(plan *plans.Plan) tfdiags.Diagnostics { // to Apply work as expected. c.state = refreshedState - return diags + return plan, diags } -func (c *Context) destroyPlan(destroyPlan *plans.Plan) tfdiags.Diagnostics { +func (c *Context) destroyPlan() (*plans.Plan, tfdiags.Diagnostics) { var diags tfdiags.Diagnostics + destroyPlan := &plans.Plan{} c.changes = plans.NewChanges() // A destroy plan starts by running Refresh to read any pending data @@ -602,17 +612,10 @@ func (c *Context) destroyPlan(destroyPlan *plans.Plan) tfdiags.Diagnostics { // a "destroy plan" is only creating delete changes, and is essentially a // local operation. if !c.skipRefresh { - refreshPlan := &plans.Plan{ - VariableValues: destroyPlan.VariableValues, - TargetAddrs: c.targets, - ProviderSHA256s: c.providerSHA256s, - } - - refreshDiags := c.plan(refreshPlan) - + refreshPlan, refreshDiags := c.plan() diags = diags.Append(refreshDiags) if diags.HasErrors() { - return diags + return nil, diags } // insert the refreshed state into the destroy plan result, and discard @@ -624,7 +627,7 @@ func (c *Context) destroyPlan(destroyPlan *plans.Plan) tfdiags.Diagnostics { graph, graphDiags := c.Graph(GraphTypePlanDestroy, nil) diags = diags.Append(graphDiags) if graphDiags.HasErrors() { - return diags + return nil, diags } // Do the walk @@ -632,11 +635,11 @@ func (c *Context) destroyPlan(destroyPlan *plans.Plan) tfdiags.Diagnostics { diags = diags.Append(walker.NonFatalDiagnostics) diags = diags.Append(walkDiags) if walkDiags.HasErrors() { - return diags + return nil, diags } destroyPlan.Changes = c.changes - return diags + return destroyPlan, diags } // Refresh goes through all the resources in the state and refreshes them