From 8d4d333efe95231ea288def73bd29582421a4ae9 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Thu, 6 May 2021 11:35:05 -0700 Subject: [PATCH] core: If we refresh during orphan processing, use the result for planning If we don't do this then we can create a situation where refresh detects that an object already doesn't exist but we plan to destroy it anyway, rather than returning "no changes" as expected. --- terraform/node_resource_abstract_instance.go | 17 ++++- terraform/node_resource_plan_orphan.go | 4 + terraform/node_resource_plan_orphan_test.go | 80 ++++++++++++++++++++ 3 files changed, 100 insertions(+), 1 deletion(-) diff --git a/terraform/node_resource_abstract_instance.go b/terraform/node_resource_abstract_instance.go index 3b5ae4997..5ecc709b8 100644 --- a/terraform/node_resource_abstract_instance.go +++ b/terraform/node_resource_abstract_instance.go @@ -350,7 +350,22 @@ func (n *NodeAbstractResourceInstance) planDestroy(ctx EvalContext, currentState // If there is no state or our attributes object is null then we're already // destroyed. if currentState == nil || currentState.Value.IsNull() { - return nil, nil + // We still need to generate a NoOp change, because that allows + // outside consumers of the plan to distinguish between us affirming + // that we checked something and concluded no changes were needed + // vs. that something being entirely excluded e.g. due to -target. + noop := &plans.ResourceInstanceChange{ + Addr: absAddr, + DeposedKey: deposedKey, + Change: plans.Change{ + Action: plans.NoOp, + Before: cty.NullVal(cty.DynamicPseudoType), + After: cty.NullVal(cty.DynamicPseudoType), + }, + Private: currentState.Private, + ProviderAddr: n.ResolvedProvider, + } + return noop, nil } // Call pre-diff hook diff --git a/terraform/node_resource_plan_orphan.go b/terraform/node_resource_plan_orphan.go index 884bd647d..74a4bbb77 100644 --- a/terraform/node_resource_plan_orphan.go +++ b/terraform/node_resource_plan_orphan.go @@ -114,6 +114,10 @@ func (n *NodePlannableResourceInstanceOrphan) managedResourceExecute(ctx EvalCon if diags.HasErrors() { return diags } + + // If we refreshed then our subsequent planning should be in terms of + // the new object, not the original object. + oldState = refreshedState } if !n.skipPlanChanges { diff --git a/terraform/node_resource_plan_orphan_test.go b/terraform/node_resource_plan_orphan_test.go index e89fd1e4b..df663abac 100644 --- a/terraform/node_resource_plan_orphan_test.go +++ b/terraform/node_resource_plan_orphan_test.go @@ -7,7 +7,9 @@ import ( "github.com/hashicorp/terraform/configs/configschema" "github.com/hashicorp/terraform/instances" "github.com/hashicorp/terraform/plans" + "github.com/hashicorp/terraform/providers" "github.com/hashicorp/terraform/states" + "github.com/zclconf/go-cty/cty" ) func TestNodeResourcePlanOrphanExecute(t *testing.T) { @@ -64,3 +66,81 @@ func TestNodeResourcePlanOrphanExecute(t *testing.T) { t.Fatalf("expected empty state, got %s", state.String()) } } + +func TestNodeResourcePlanOrphanExecute_alreadyDeleted(t *testing.T) { + addr := addrs.Resource{ + Mode: addrs.ManagedResourceMode, + Type: "test_object", + Name: "foo", + }.Instance(addrs.NoKey).Absolute(addrs.RootModuleInstance) + + state := states.NewState() + state.Module(addrs.RootModuleInstance).SetResourceInstanceCurrent( + addr.Resource, + &states.ResourceInstanceObjectSrc{ + AttrsFlat: map[string]string{ + "test_string": "foo", + }, + Status: states.ObjectReady, + }, + addrs.AbsProviderConfig{ + Provider: addrs.NewDefaultProvider("test"), + Module: addrs.RootModule, + }, + ) + refreshState := state.DeepCopy() + prevRunState := state.DeepCopy() + changes := plans.NewChanges() + + p := simpleMockProvider() + p.ReadResourceResponse = &providers.ReadResourceResponse{ + NewState: cty.NullVal(p.GetProviderSchemaResponse.ResourceTypes["test_string"].Block.ImpliedType()), + } + ctx := &MockEvalContext{ + StateState: state.SyncWrapper(), + RefreshStateState: refreshState.SyncWrapper(), + PrevRunStateState: prevRunState.SyncWrapper(), + InstanceExpanderExpander: instances.NewExpander(), + ProviderProvider: p, + ProviderSchemaSchema: &ProviderSchema{ + ResourceTypes: map[string]*configschema.Block{ + "test_object": simpleTestSchema(), + }, + }, + ChangesChanges: changes.SyncWrapper(), + } + + node := NodePlannableResourceInstanceOrphan{ + NodeAbstractResourceInstance: &NodeAbstractResourceInstance{ + NodeAbstractResource: NodeAbstractResource{ + ResolvedProvider: addrs.AbsProviderConfig{ + Provider: addrs.NewDefaultProvider("test"), + Module: addrs.RootModule, + }, + }, + Addr: mustResourceInstanceAddr("test_object.foo"), + }, + } + diags := node.Execute(ctx, walkPlan) + if diags.HasErrors() { + t.Fatalf("unexpected error: %s", diags.Err()) + } + if !state.Empty() { + t.Fatalf("expected empty state, got %s", state.String()) + } + + if got := prevRunState.ResourceInstance(addr); got == nil { + t.Errorf("no entry for %s in the prev run state; should still be present", addr) + } + if got := refreshState.ResourceInstance(addr); got != nil { + t.Errorf("refresh state has entry for %s; should've been removed", addr) + } + if got := changes.ResourceInstance(addr); got == nil { + t.Errorf("no entry for %s in the planned changes; should have a NoOp change", addr) + } else { + if got, want := got.Action, plans.NoOp; got != want { + t.Errorf("planned change for %s has wrong action\ngot: %s\nwant: %s", addr, got, want) + } + } + +}