From ddb2bbf4e9952fceae0a0bdbc1e268f988302730 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 22 Oct 2020 09:13:02 -0400 Subject: [PATCH 1/3] Read orphaned resources during plan This forces orphaned resources to be re-read during planning, removing them from the state if they no longer exist. This needs to be done for a bare `refresh` execution, since Terraform should remove instances that don't exist and are not in the configuration from the state. They should also be removed from state so there is no Delete change planned, as not all providers will gracefully handle a delete operation on a resource that does not exist. --- terraform/graph_builder_plan.go | 1 + terraform/node_resource_plan_orphan.go | 38 +++++++++++++++++++++++++- 2 files changed, 38 insertions(+), 1 deletion(-) diff --git a/terraform/graph_builder_plan.go b/terraform/graph_builder_plan.go index 503ce1b61b..49184c2e2b 100644 --- a/terraform/graph_builder_plan.go +++ b/terraform/graph_builder_plan.go @@ -191,6 +191,7 @@ func (b *PlanGraphBuilder) init() { b.ConcreteResourceOrphan = func(a *NodeAbstractResourceInstance) dag.Vertex { return &NodePlannableResourceInstanceOrphan{ NodeAbstractResourceInstance: a, + skipRefresh: b.skipRefresh, } } } diff --git a/terraform/node_resource_plan_orphan.go b/terraform/node_resource_plan_orphan.go index ca02000545..7469473e30 100644 --- a/terraform/node_resource_plan_orphan.go +++ b/terraform/node_resource_plan_orphan.go @@ -9,6 +9,8 @@ import ( // it is ready to be applied and is represented by a diff. type NodePlannableResourceInstanceOrphan struct { *NodeAbstractResourceInstance + + skipRefresh bool } var ( @@ -35,7 +37,7 @@ func (n *NodePlannableResourceInstanceOrphan) Execute(ctx EvalContext, op walkOp var change *plans.ResourceInstanceChange var state *states.ResourceInstanceObject - _, providerSchema, err := GetProvider(ctx, n.ResolvedProvider) + provider, providerSchema, err := GetProvider(ctx, n.ResolvedProvider) if err != nil { return err } @@ -45,6 +47,40 @@ func (n *NodePlannableResourceInstanceOrphan) Execute(ctx EvalContext, op walkOp return err } + if !n.skipRefresh { + // Refresh this instance even though it is going to be destroyed, in + // order to catch missing resources. If this is a normal plan, + // providers expect a Read request to remove missing resources from the + // plan before apply, and may not handle a missing resource during + // Delete correctly. If this is a simple refresh, Terraform is + // expected to remove the missing resource from the state entirely + refresh := &EvalRefresh{ + Addr: addr.Resource, + ProviderAddr: n.ResolvedProvider, + Provider: &provider, + ProviderMetas: n.ProviderMetas, + ProviderSchema: &providerSchema, + State: &state, + Output: &state, + } + _, err = refresh.Eval(ctx) + if err != nil { + return err + } + + writeRefreshState := &EvalWriteState{ + Addr: addr.Resource, + ProviderAddr: n.ResolvedProvider, + ProviderSchema: &providerSchema, + State: &state, + targetState: refreshState, + } + _, err = writeRefreshState.Eval(ctx) + if err != nil { + return err + } + } + diffDestroy := &EvalDiffDestroy{ Addr: addr.Resource, State: &state, From 72e81de0fce0c467b61a3a7c99c2110424d6cfd8 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 22 Oct 2020 09:32:47 -0400 Subject: [PATCH 2/3] update tests Some tests could not handle reading orphaned resources. It also turns out the ReadResource mock never returned the correct state in the default case at all. --- terraform/context_apply_test.go | 1 - terraform/node_resource_plan_orphan_test.go | 1 + terraform/provider_mock.go | 18 ++++++++++++------ 3 files changed, 13 insertions(+), 7 deletions(-) diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index f5b28da2b4..071c80db9e 100644 --- a/terraform/context_apply_test.go +++ b/terraform/context_apply_test.go @@ -267,7 +267,6 @@ func TestContext2Apply_resourceDependsOnModule(t *testing.T) { } if !reflect.DeepEqual(order, []string{"child", "parent"}) { - fmt.Println("ORDER:", order) t.Fatal("resources applied out of order") } diff --git a/terraform/node_resource_plan_orphan_test.go b/terraform/node_resource_plan_orphan_test.go index 45078cd436..f4396448ed 100644 --- a/terraform/node_resource_plan_orphan_test.go +++ b/terraform/node_resource_plan_orphan_test.go @@ -33,6 +33,7 @@ func TestNodeResourcePlanOrphanExecute(t *testing.T) { p := simpleMockProvider() ctx := &MockEvalContext{ StateState: state.SyncWrapper(), + RefreshStateState: state.SyncWrapper(), InstanceExpanderExpander: instances.NewExpander(), ProviderProvider: p, ProviderSchemaSchema: &ProviderSchema{ diff --git a/terraform/provider_mock.go b/terraform/provider_mock.go index 45397fe22a..bf3aca3274 100644 --- a/terraform/provider_mock.go +++ b/terraform/provider_mock.go @@ -242,14 +242,20 @@ func (p *MockProvider) ReadResource(r providers.ReadResourceRequest) providers.R return p.ReadResourceFn(r) } - // make sure the NewState fits the schema - newState, err := p.GetSchemaReturn.ResourceTypes[r.TypeName].CoerceValue(p.ReadResourceResponse.NewState) - if err != nil { - panic(err) - } resp := p.ReadResourceResponse - resp.NewState = newState + if resp.NewState != cty.NilVal { + // make sure the NewState fits the schema + // This isn't always the case for the existing tests + newState, err := p.GetSchemaReturn.ResourceTypes[r.TypeName].CoerceValue(resp.NewState) + if err != nil { + panic(err) + } + resp.NewState = newState + return resp + } + // just return the same state we received + resp.NewState = r.PriorState return resp } From 73627e4dc37b88dffc5b28db8c73377732ec15cf Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 22 Oct 2020 10:15:22 -0400 Subject: [PATCH 3/3] don't read data sources that are to be removed We can directly remove orphaned data sources from the refesh state. --- terraform/context_refresh_test.go | 51 ++++++++++++++++++++++++ terraform/node_resource_plan_instance.go | 6 +-- terraform/node_resource_plan_orphan.go | 26 ++++++++++++ 3 files changed, 80 insertions(+), 3 deletions(-) diff --git a/terraform/context_refresh_test.go b/terraform/context_refresh_test.go index 77535d7355..b8efe480c0 100644 --- a/terraform/context_refresh_test.go +++ b/terraform/context_refresh_test.go @@ -1743,3 +1743,54 @@ resource "aws_instance" "bar" { t.Fatal("create_before_destroy not updated in instance state") } } + +func TestContext2Refresh_dataSourceOrphan(t *testing.T) { + m := testModuleInline(t, map[string]string{ + "main.tf": ``, + }) + + state := states.NewState() + root := state.EnsureModule(addrs.RootModuleInstance) + root.SetResourceInstanceCurrent( + addrs.Resource{ + Mode: addrs.DataResourceMode, + Type: "test_data_source", + Name: "foo", + }.Instance(addrs.NoKey), + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"id":"foo"}`), + Dependencies: []addrs.ConfigResource{}, + }, + addrs.AbsProviderConfig{ + Provider: addrs.NewDefaultProvider("test"), + Module: addrs.RootModule, + }, + ) + p := testProvider("test") + p.ReadDataSourceFn = func(req providers.ReadDataSourceRequest) (resp providers.ReadDataSourceResponse) { + resp.State = cty.NullVal(req.Config.Type()) + return + } + + ctx := testContext2(t, &ContextOpts{ + Config: m, + Providers: map[addrs.Provider]providers.Factory{ + addrs.NewDefaultProvider("test"): testProviderFuncFixed(p), + }, + State: state, + }) + + _, diags := ctx.Refresh() + if diags.HasErrors() { + t.Fatal(diags.Err()) + } + + if p.ReadResourceCalled { + t.Fatal("there are no managed resources to read") + } + + if p.ReadDataSourceCalled { + t.Fatal("orphaned data source instance should not be read") + } +} diff --git a/terraform/node_resource_plan_instance.go b/terraform/node_resource_plan_instance.go index 3ffe994600..3810484d05 100644 --- a/terraform/node_resource_plan_instance.go +++ b/terraform/node_resource_plan_instance.go @@ -36,7 +36,7 @@ func (n *NodePlannableResourceInstance) Execute(ctx EvalContext, op walkOperatio // Eval info is different depending on what kind of resource this is switch addr.Resource.Resource.Mode { case addrs.ManagedResourceMode: - return n.managedResourceExecute(ctx, n.skipRefresh) + return n.managedResourceExecute(ctx) case addrs.DataResourceMode: return n.dataResourceExecute(ctx) default: @@ -123,7 +123,7 @@ func (n *NodePlannableResourceInstance) dataResourceExecute(ctx EvalContext) err return err } -func (n *NodePlannableResourceInstance) managedResourceExecute(ctx EvalContext, skipRefresh bool) error { +func (n *NodePlannableResourceInstance) managedResourceExecute(ctx EvalContext) error { config := n.Config addr := n.ResourceInstanceAddr() @@ -162,7 +162,7 @@ func (n *NodePlannableResourceInstance) managedResourceExecute(ctx EvalContext, } // Refresh, maybe - if !skipRefresh { + if !n.skipRefresh { refresh := &EvalRefresh{ Addr: addr.Resource, ProviderAddr: n.ResolvedProvider, diff --git a/terraform/node_resource_plan_orphan.go b/terraform/node_resource_plan_orphan.go index 7469473e30..515caec58d 100644 --- a/terraform/node_resource_plan_orphan.go +++ b/terraform/node_resource_plan_orphan.go @@ -1,6 +1,10 @@ package terraform import ( + "fmt" + "log" + + "github.com/hashicorp/terraform/addrs" "github.com/hashicorp/terraform/plans" "github.com/hashicorp/terraform/states" ) @@ -32,6 +36,28 @@ func (n *NodePlannableResourceInstanceOrphan) Name() string { func (n *NodePlannableResourceInstanceOrphan) Execute(ctx EvalContext, op walkOperation) error { addr := n.ResourceInstanceAddr() + // Eval info is different depending on what kind of resource this is + switch addr.Resource.Resource.Mode { + case addrs.ManagedResourceMode: + return n.managedResourceExecute(ctx) + case addrs.DataResourceMode: + return n.dataResourceExecute(ctx) + default: + panic(fmt.Errorf("unsupported resource mode %s", n.Config.Mode)) + } +} + +func (n *NodePlannableResourceInstanceOrphan) dataResourceExecute(ctx EvalContext) error { + // A data source that is no longer in the config is removed from the state + log.Printf("[TRACE] NodePlannableResourceInstanceOrphan: removing state object for %s", n.Addr) + state := ctx.RefreshState() + state.SetResourceInstanceCurrent(n.Addr, nil, n.ResolvedProvider) + return nil +} + +func (n *NodePlannableResourceInstanceOrphan) managedResourceExecute(ctx EvalContext) error { + addr := n.ResourceInstanceAddr() + // Declare a bunch of variables that are used for state during // evaluation. These are written to by-address below. var change *plans.ResourceInstanceChange