terraform: NodeDestroyResourceInstance refactor (#26246)

* terraform: remove unused eval node

* add UpdateStateHook function to replace EvalUpdateStateHook

* early exit error isn't

* terraform: NodeDestroyResourceInstance refactor

This PR refactor's NodeDestroyResourceInstance EvalTree() into an
Execute() node. EvalRequireState and evalWriteEmptyState were used only
by this node, and they have been removed in favor of straight code.

There are still many calls to someEvalNode.Eval() in NodeDestroyResourceInstance: I plan on refactoring the remaining EvalTree()s before tacking those Eval()s (all of which are used by many graph nodes)

I've added a new function, UpdateStateHook, that is effectively the same
as EvalUpdateStateHook. The latter will be removed when the larger
EvalNode refactor project is complete.

terraform/eval_state.go

@@ -143,25 +143,6 @@ func (n *EvalReadStateDeposed) Eval(ctx EvalContext) (interface{}, error) {
 	return obj, nil
 }
 
-// EvalRequireState is an EvalNode implementation that exits early if the given
-// object is null.
-type EvalRequireState struct {
-	State **states.ResourceInstanceObject
-}
-
-func (n *EvalRequireState) Eval(ctx EvalContext) (interface{}, error) {
-	if n.State == nil {
-		return nil, EvalEarlyExitError{}
-	}
-
-	state := *n.State
-	if state == nil || state.Value.IsNull() {
-		return nil, EvalEarlyExitError{}
-	}
-
-	return nil, nil
-}
-
 // EvalUpdateStateHook is an EvalNode implementation that calls the
 // PostStateUpdate hook with the current state.
 type EvalUpdateStateHook struct{}
@@ -187,6 +168,27 @@ func (n *EvalUpdateStateHook) Eval(ctx EvalContext) (interface{}, error) {
 	return nil, nil
 }
 
+// UpdateStateHook calls the PostStateUpdate hook with the current state.
+//
+// TODO: UpdateStateHook will eventually replace EvalUpdateStateHook, at which
+// point EvalUpdateStateHook can be removed and this comment updated.
+func UpdateStateHook(ctx EvalContext) error {
+	// In principle we could grab the lock here just long enough to take a
+	// deep copy and then pass that to our hooks below, but we'll instead
+	// hold the hook for the duration to avoid the potential confusing
+	// situation of us racing to call PostStateUpdate concurrently with
+	// different state snapshots.
+	stateSync := ctx.State()
+	state := stateSync.Lock().DeepCopy()
+	defer stateSync.Unlock()
+
+	// Call the hook
+	err := ctx.Hook(func(h Hook) (HookAction, error) {
+		return h.PostStateUpdate(state)
+	})
+	return err
+}
+
 // evalWriteEmptyState wraps EvalWriteState to specifically record an empty
 // state for a particular object.
 type evalWriteEmptyState struct {
@@ -510,34 +512,6 @@ func (n *EvalWriteResourceState) Eval(ctx EvalContext) (interface{}, error) {
 	return nil, nil
 }
 
-// EvalForgetResourceState is an EvalNode implementation that prunes out an
-// empty resource-level state for a given resource address, or produces an
-// error if it isn't empty after all.
-//
-// This should be the last action taken for a resource that has been removed
-// from the configuration altogether, to clean up the leftover husk of the
-// resource in the state after other EvalNodes have destroyed and removed
-// all of the instances and instance objects beneath it.
-type EvalForgetResourceState struct {
-	Addr addrs.Resource
-}
-
-func (n *EvalForgetResourceState) Eval(ctx EvalContext) (interface{}, error) {
-	absAddr := n.Addr.Absolute(ctx.Path())
-	state := ctx.State()
-
-	pruned := state.RemoveResourceIfEmpty(absAddr)
-	if !pruned {
-		// If this produces an error, it indicates a bug elsewhere in Terraform
-		// -- probably missing graph nodes, graph edges, or
-		// incorrectly-implemented evaluation steps.
-		return nil, fmt.Errorf("orphan resource %s still has a non-empty state after apply; this is a bug in Terraform", absAddr)
-	}
-	log.Printf("[TRACE] EvalForgetResourceState: Pruned husk of %s from state", absAddr)
-
-	return nil, nil
-}
-
 // EvalRefreshDependencies is an EvalNode implementation that appends any newly
 // found dependencies to those saved in the state. The existing dependencies
 // are retained, as they may be missing from the config, and will be required

terraform/eval_state_test.go

@@ -12,57 +12,6 @@ import (
 	"github.com/hashicorp/terraform/states"
 )
 
-func TestEvalRequireState(t *testing.T) {
-	ctx := new(MockEvalContext)
-
-	cases := []struct {
-		State *states.ResourceInstanceObject
-		Exit  bool
-	}{
-		{
-			nil,
-			true,
-		},
-		{
-			&states.ResourceInstanceObject{
-				Value: cty.NullVal(cty.Object(map[string]cty.Type{
-					"id": cty.String,
-				})),
-				Status: states.ObjectReady,
-			},
-			true,
-		},
-		{
-			&states.ResourceInstanceObject{
-				Value: cty.ObjectVal(map[string]cty.Value{
-					"id": cty.StringVal("foo"),
-				}),
-				Status: states.ObjectReady,
-			},
-			false,
-		},
-	}
-
-	var exitVal EvalEarlyExitError
-	for _, tc := range cases {
-		node := &EvalRequireState{State: &tc.State}
-		_, err := node.Eval(ctx)
-		if tc.Exit {
-			if err != exitVal {
-				t.Fatalf("should've exited: %#v", tc.State)
-			}
-
-			continue
-		}
-		if !tc.Exit && err != nil {
-			t.Fatalf("shouldn't exit: %#v", tc.State)
-		}
-		if err != nil {
-			t.Fatalf("err: %s", err)
-		}
-	}
-}
-
 func TestEvalUpdateStateHook(t *testing.T) {
 	mockHook := new(MockHook)
 
@@ -275,3 +224,28 @@ aws_instance.foo: (1 deposed)
   Deposed ID 1 = i-abc123
 	`)
 }
+
+// Same test as TestEvalUpdateStateHook, similar function, slightly different
+// signature. The EvalUpdateStateHook test and function will be removed when the
+// EvalNode Removal is complete.
+func TestUpdateStateHook(t *testing.T) {
+	mockHook := new(MockHook)
+
+	state := states.NewState()
+	state.Module(addrs.RootModuleInstance).SetLocalValue("foo", cty.StringVal("hello"))
+
+	ctx := new(MockEvalContext)
+	ctx.HookHook = mockHook
+	ctx.StateState = state.SyncWrapper()
+
+	if err := UpdateStateHook(ctx); err != nil {
+		t.Fatalf("err: %s", err)
+	}
+
+	if !mockHook.PostStateUpdateCalled {
+		t.Fatal("should call PostStateUpdate")
+	}
+	if mockHook.PostStateUpdateState.LocalValue(addrs.LocalValue{Name: "foo"}.Absolute(addrs.RootModuleInstance)) != cty.StringVal("hello") {
+		t.Fatalf("wrong state passed to hook: %s", spew.Sdump(mockHook.PostStateUpdateState))
+	}
+}

terraform/graph_walk_context.go

@@ -188,6 +188,11 @@ func (w *ContextGraphWalker) Execute(ctx EvalContext, n GraphNodeExecutable) tfd
 		return nil
 	}
 
+	//  If we early exit, it isn't an error.
+	if _, isEarlyExit := err.(EvalEarlyExitError); isEarlyExit {
+		return nil
+	}
+
 	// Otherwise, we'll let our usual diagnostics machinery figure out how to
 	// unpack this as one or more diagnostic messages and return that. If we
 	// get down here then the returned diagnostics will contain at least one

terraform/node_data_refresh.go

@@ -251,12 +251,11 @@ func (n *NodeRefreshableDataResourceInstance) Execute(ctx EvalContext, op walkOp
 			return err
 		}
 
-		// EvalUpdateStateHook
-		updateStateHookReq := EvalUpdateStateHook{}
-		_, err = updateStateHookReq.Eval(ctx)
+		err = UpdateStateHook(ctx)
 		if err != nil {
 			return err
 		}
+
 	} else {
 		// EvalWriteDiff
 		writeDiffReq := &EvalWriteDiff{

terraform/node_resource_destroy.go

@@ -5,7 +5,6 @@ import (
 	"log"
 
 	"github.com/hashicorp/terraform/plans"
-	"github.com/hashicorp/terraform/providers"
 
 	"github.com/hashicorp/terraform/addrs"
 	"github.com/hashicorp/terraform/configs"
@@ -31,7 +30,7 @@ var (
 	_ GraphNodeDestroyerCBD        = (*NodeDestroyResourceInstance)(nil)
 	_ GraphNodeReferenceable       = (*NodeDestroyResourceInstance)(nil)
 	_ GraphNodeReferencer          = (*NodeDestroyResourceInstance)(nil)
-	_ GraphNodeEvalable            = (*NodeDestroyResourceInstance)(nil)
+	_ GraphNodeExecutable          = (*NodeDestroyResourceInstance)(nil)
 	_ GraphNodeProviderConsumer    = (*NodeDestroyResourceInstance)(nil)
 	_ GraphNodeProvisionerConsumer = (*NodeDestroyResourceInstance)(nil)
 )
@@ -122,8 +121,8 @@ func (n *NodeDestroyResourceInstance) References() []*addrs.Reference {
 	return nil
 }
 
-// GraphNodeEvalable
-func (n *NodeDestroyResourceInstance) EvalTree() EvalNode {
+// GraphNodeExecutable
+func (n *NodeDestroyResourceInstance) Execute(ctx EvalContext, op walkOperation) error {
 	addr := n.ResourceInstanceAddr()
 
 	// Get our state
@@ -132,142 +131,150 @@ func (n *NodeDestroyResourceInstance) EvalTree() EvalNode {
 		log.Printf("[WARN] NodeDestroyResourceInstance for %s with no state", addr)
 	}
 
+	// These vars are updated through pointers at various stages below.
 	var changeApply *plans.ResourceInstanceChange
-	var provider providers.Interface
-	var providerSchema *ProviderSchema
 	var state *states.ResourceInstanceObject
-	var err error
-	return &EvalOpFilter{
-		Ops: []walkOperation{walkApply, walkDestroy},
-		Node: &EvalSequence{
-			Nodes: []EvalNode{
-				&EvalGetProvider{
-					Addr:   n.ResolvedProvider,
-					Output: &provider,
-					Schema: &providerSchema,
-				},
+	var provisionerErr error
 
-				// Get the saved diff for apply
-				&EvalReadDiff{
-					Addr:           addr.Resource,
-					ProviderSchema: &providerSchema,
-					Change:         &changeApply,
-				},
+	switch op {
+	case walkApply, walkDestroy:
+		provider, providerSchema, err := GetProvider(ctx, n.ResolvedProvider)
+		if err != nil {
+			return err
+		}
 
-				&EvalReduceDiff{
-					Addr:      addr.Resource,
-					InChange:  &changeApply,
-					Destroy:   true,
-					OutChange: &changeApply,
-				},
+		evalReadDiff := &EvalReadDiff{
+			Addr:           addr.Resource,
+			ProviderSchema: &providerSchema,
+			Change:         &changeApply,
+		}
+		_, err = evalReadDiff.Eval(ctx)
+		if err != nil {
+			return err
+		}
 
-				// EvalReduceDiff may have simplified our planned change
-				// into a NoOp if it does not require destroying.
-				&EvalIf{
-					If: func(ctx EvalContext) (bool, error) {
-						if changeApply == nil || changeApply.Action == plans.NoOp {
-							return true, EvalEarlyExitError{}
-						}
-						return true, nil
-					},
-					Then: EvalNoop{},
-				},
+		evalReduceDiff := &EvalReduceDiff{
+			Addr:      addr.Resource,
+			InChange:  &changeApply,
+			Destroy:   true,
+			OutChange: &changeApply,
+		}
+		_, err = evalReduceDiff.Eval(ctx)
+		if err != nil {
+			return err
+		}
 
-				&EvalReadState{
-					Addr:           addr.Resource,
-					Output:         &state,
-					Provider:       &provider,
-					ProviderSchema: &providerSchema,
-				},
-				&EvalRequireState{
-					State: &state,
-				},
+		// EvalReduceDiff may have simplified our planned change
+		// into a NoOp if it does not require destroying.
+		if changeApply == nil || changeApply.Action == plans.NoOp {
+			return EvalEarlyExitError{}
+		}
 
-				// Call pre-apply hook
-				&EvalApplyPre{
-					Addr:   addr.Resource,
-					State:  &state,
-					Change: &changeApply,
-				},
+		evalReadState := &EvalReadState{
+			Addr:           addr.Resource,
+			Output:         &state,
+			Provider:       &provider,
+			ProviderSchema: &providerSchema,
+		}
+		_, err = evalReadState.Eval(ctx)
+		if err != nil {
+			return err
+		}
 
-				// Run destroy provisioners if not tainted
-				&EvalIf{
-					If: func(ctx EvalContext) (bool, error) {
-						if state != nil && state.Status == states.ObjectTainted {
-							return false, nil
-						}
+		// Exit early if the state object is null after reading the state
+		if state == nil || state.Value.IsNull() {
+			return EvalEarlyExitError{}
+		}
 
-						return true, nil
-					},
-
-					Then: &EvalApplyProvisioners{
-						Addr:           addr.Resource,
-						State:          &state,
-						ResourceConfig: n.Config,
-						Error:          &err,
-						When:           configs.ProvisionerWhenDestroy,
-					},
-				},
+		evalApplyPre := &EvalApplyPre{
+			Addr:   addr.Resource,
+			State:  &state,
+			Change: &changeApply,
+		}
+		_, err = evalApplyPre.Eval(ctx)
+		if err != nil {
+			return err
+		}
 
+		// Run destroy provisioners if not tainted
+		if state != nil && state.Status != states.ObjectTainted {
+			evalApplyProvisioners := &EvalApplyProvisioners{
+				Addr:           addr.Resource,
+				State:          &state,
+				ResourceConfig: n.Config,
+				Error:          &provisionerErr,
+				When:           configs.ProvisionerWhenDestroy,
+			}
+			_, err := evalApplyProvisioners.Eval(ctx)
+			if err != nil {
+				return err
+			}
+			if provisionerErr != nil {
 				// If we have a provisioning error, then we just call
 				// the post-apply hook now.
-				&EvalIf{
-					If: func(ctx EvalContext) (bool, error) {
-						return err != nil, nil
-					},
-
-					Then: &EvalApplyPost{
-						Addr:  addr.Resource,
-						State: &state,
-						Error: &err,
-					},
-				},
-
-				// Managed resources need to be destroyed, while data sources
-				// are only removed from state.
-				&EvalIf{
-					If: func(ctx EvalContext) (bool, error) {
-						return addr.Resource.Resource.Mode == addrs.ManagedResourceMode, nil
-					},
-
-					Then: &EvalSequence{
-						Nodes: []EvalNode{
-							&EvalApply{
-								Addr:           addr.Resource,
-								Config:         nil, // No configuration because we are destroying
-								State:          &state,
-								Change:         &changeApply,
-								Provider:       &provider,
-								ProviderAddr:   n.ResolvedProvider,
-								ProviderMetas:  n.ProviderMetas,
-								ProviderSchema: &providerSchema,
-								Output:         &state,
-								Error:          &err,
-							},
-							&EvalWriteState{
-								Addr:           addr.Resource,
-								ProviderAddr:   n.ResolvedProvider,
-								ProviderSchema: &providerSchema,
-								State:          &state,
-							},
-						},
-					},
-					Else: &evalWriteEmptyState{
-						EvalWriteState{
-							Addr:           addr.Resource,
-							ProviderAddr:   n.ResolvedProvider,
-							ProviderSchema: &providerSchema,
-						},
-					},
-				},
-
-				&EvalApplyPost{
+				evalApplyPost := &EvalApplyPost{
 					Addr:  addr.Resource,
 					State: &state,
-					Error: &err,
-				},
-				&EvalUpdateStateHook{},
-			},
-		},
+					Error: &provisionerErr,
+				}
+				_, err = evalApplyPost.Eval(ctx)
+				if err != nil {
+					return err
+				}
+			}
+		}
+
+		// Managed resources need to be destroyed, while data sources
+		// are only removed from state.
+		if addr.Resource.Resource.Mode == addrs.ManagedResourceMode {
+			evalApply := &EvalApply{
+				Addr:           addr.Resource,
+				Config:         nil, // No configuration because we are destroying
+				State:          &state,
+				Change:         &changeApply,
+				Provider:       &provider,
+				ProviderAddr:   n.ResolvedProvider,
+				ProviderMetas:  n.ProviderMetas,
+				ProviderSchema: &providerSchema,
+				Output:         &state,
+				Error:          &provisionerErr,
+			}
+			_, err = evalApply.Eval(ctx)
+			if err != nil {
+				return err
+			}
+
+			evalWriteState := &EvalWriteState{
+				Addr:           addr.Resource,
+				ProviderAddr:   n.ResolvedProvider,
+				ProviderSchema: &providerSchema,
+				State:          &state,
+			}
+			_, err = evalWriteState.Eval(ctx)
+			if err != nil {
+				return err
+			}
+		} else {
+			log.Printf("[TRACE] NodeDestroyResourceInstance: removing state object for %s", n.Addr)
+			state := ctx.State()
+			state.SetResourceInstanceCurrent(n.Addr, nil, n.ResolvedProvider)
+		}
+
+		evalApplyPost := &EvalApplyPost{
+			Addr:  addr.Resource,
+			State: &state,
+			Error: &provisionerErr,
+		}
+		_, err = evalApplyPost.Eval(ctx)
+		if err != nil {
+			return err
+		}
+
+		err = UpdateStateHook(ctx)
+		if err != nil {
+			return err
+		}
 	}
+
+	return nil
 }