Merge pull request hashicorp#35890 from hashicorp/jbardin/unused-ephe…

…meral-destroy

core: update destroy graph pruning to handle ephemeral resources

internal/terraform/context_apply2_test.go

@@ -2357,21 +2357,13 @@ locals {
 	// The local value should remain in the graph because the external
 	// reference uses it.
 	gotGraph := g.String()
-	wantGraph := `<external ref to local.local_value>
-  local.local_value (expand)
-local.local_value (expand)
-  test_object.a (expand)
-provider["registry.terraform.io/hashicorp/test"]
+	wantGraph := `provider["registry.terraform.io/hashicorp/test"]
 provider["registry.terraform.io/hashicorp/test"] (close)
   test_object.a (destroy)
-  test_object.a (expand)
 root
-  <external ref to local.local_value>
   provider["registry.terraform.io/hashicorp/test"] (close)
 test_object.a (destroy)
   provider["registry.terraform.io/hashicorp/test"]
-test_object.a (expand)
-  provider["registry.terraform.io/hashicorp/test"]
 `
 	if diff := cmp.Diff(wantGraph, gotGraph); diff != "" {
 		t.Errorf("wrong graph\n%s", diff)

internal/terraform/context_plan2_test.go

@@ -5942,7 +5942,7 @@ resource "test_object" "a" {
 		},
 	})
 	if diags.HasErrors() {
-		t.Errorf("expected no errors, but got %s", diags)
+		t.Fatalf("expected no errors, but got %s", diags.ErrWithWarnings())
 	}
 
 	planResult := plan.Checks.GetObjectResult(addrs.AbsInputVariableInstance{
@@ -5952,7 +5952,7 @@ resource "test_object" "a" {
 		Module: addrs.RootModuleInstance,
 	})
 
-	if planResult.Status != checks.StatusUnknown {
+	if planResult != nil && planResult.Status != checks.StatusUnknown {
 		// checks should not have been evaluated, because the variable is not required for destroy.
 		t.Errorf("expected checks to be pass but was %s", planResult.Status)
 	}

internal/terraform/node_resource_destroy_deposed.go

@@ -61,8 +61,13 @@ var (
 	_ GraphNodeExecutable                    = (*NodePlanDeposedResourceInstanceObject)(nil)
 	_ GraphNodeProviderConsumer              = (*NodePlanDeposedResourceInstanceObject)(nil)
 	_ GraphNodeProvisionerConsumer           = (*NodePlanDeposedResourceInstanceObject)(nil)
+	_ GraphNodeDestroyer                     = (*NodePlanDeposedResourceInstanceObject)(nil)
 )
 
+func (n *NodePlanDeposedResourceInstanceObject) DestroyAddr() *addrs.AbsResourceInstance {
+	return &n.Addr
+}
+
 func (n *NodePlanDeposedResourceInstanceObject) Name() string {
 	return fmt.Sprintf("%s (deposed %s)", n.ResourceInstanceAddr().String(), n.DeposedKey)
 }
@@ -226,6 +231,7 @@ var (
 	_ GraphNodeExecutable                    = (*NodeDestroyDeposedResourceInstanceObject)(nil)
 	_ GraphNodeProviderConsumer              = (*NodeDestroyDeposedResourceInstanceObject)(nil)
 	_ GraphNodeProvisionerConsumer           = (*NodeDestroyDeposedResourceInstanceObject)(nil)
+	_ GraphNodeDestroyer                     = (*NodeDestroyDeposedResourceInstanceObject)(nil)
 )
 
 func (n *NodeDestroyDeposedResourceInstanceObject) Name() string {
@@ -344,12 +350,17 @@ var (
 	_ GraphNodeExecutable                    = (*NodeForgetDeposedResourceInstanceObject)(nil)
 	_ GraphNodeProviderConsumer              = (*NodeForgetDeposedResourceInstanceObject)(nil)
 	_ GraphNodeProvisionerConsumer           = (*NodeForgetDeposedResourceInstanceObject)(nil)
+	_ GraphNodeDestroyer                     = (*NodeForgetDeposedResourceInstanceObject)(nil)
 )
 
 func (n *NodeForgetDeposedResourceInstanceObject) Name() string {
 	return fmt.Sprintf("%s (forget deposed %s)", n.ResourceInstanceAddr(), n.DeposedKey)
 }
 
+func (n *NodeForgetDeposedResourceInstanceObject) DestroyAddr() *addrs.AbsResourceInstance {
+	return &n.Addr
+}
+
 func (n *NodeForgetDeposedResourceInstanceObject) DeposedInstanceObjectKey() states.DeposedKey {
 	return n.DeposedKey
 }

internal/terraform/node_resource_forget.go

@@ -27,8 +27,13 @@ var (
 	_ GraphNodeExecutable          = (*NodeForgetResourceInstance)(nil)
 	_ GraphNodeProviderConsumer    = (*NodeForgetResourceInstance)(nil)
 	_ GraphNodeProvisionerConsumer = (*NodeForgetResourceInstance)(nil)
+	_ GraphNodeDestroyer           = (*NodeForgetResourceInstance)(nil)
 )
 
+func (n *NodeForgetResourceInstance) DestroyAddr() *addrs.AbsResourceInstance {
+	return &n.Addr
+}
+
 func (n *NodeForgetResourceInstance) Name() string {
 	return n.ResourceInstanceAddr().String() + " (forget)"
 }

internal/terraform/node_resource_plan_destroy.go

@@ -37,6 +37,10 @@ var (
 	_ GraphNodeProviderConsumer     = (*NodePlanDestroyableResourceInstance)(nil)
 )
 
+func (n *NodePlanDestroyableResourceInstance) Name() string {
+	return n.NodeAbstractResource.Name() + " (destroy)"
+}
+
 // GraphNodeDestroyer
 func (n *NodePlanDestroyableResourceInstance) DestroyAddr() *addrs.AbsResourceInstance {
 	addr := n.ResourceInstanceAddr()

internal/terraform/transform_destroy_edge.go

@@ -311,105 +311,43 @@ func (t *pruneUnusedNodesTransformer) Transform(g *Graph) error {
 		return nil
 	}
 
-	// We need a reverse depth first walk of modules, processing them in order
-	// from the leaf modules to the root. This allows us to remove unneeded
-	// dependencies from child modules, freeing up nodes in the parent module
-	// to also be removed.
-
-	nodes := g.Vertices()
-
-	for removed := true; removed; {
-		removed = false
-
-		for i := 0; i < len(nodes); i++ {
-			// run this in a closure, so we can return early rather than
-			// dealing with complex looping and labels
-			func() {
-				n := nodes[i]
-				switch n := n.(type) {
-
-				case graphNodeTemporaryValue:
-					// root module outputs indicate they are not temporary by
-					// returning false here.
-					if !n.temporaryValue() {
-						return
-					}
-
-					// temporary values, which consist of variables, locals,
-					// and outputs, must be kept if anything refers to them.
-					for _, v := range g.UpEdges(n) {
-						// keep any value which is connected through a
-						// reference
-						if _, ok := v.(GraphNodeReferencer); ok {
-							return
-						}
-					}
-
-				case graphNodeExpandsInstances:
-					// FIXME: Ephemeral resources have kind of broken this
-					// transformer. They work like temporary values in that they
-					// should not exist when there are no dependencies, but also
-					// share expansion nodes with all other resource modes. We
-					// can't use graphNodeTemporaryValue because then all
-					// resources will be caught by the previous case here.
-					if n, ok := n.(GraphNodeConfigResource); ok && n.ResourceAddr().Resource.Mode == addrs.EphemeralResourceMode {
-						return
-					}
-
-					// Any nodes that expand instances are kept when their
-					// instances may need to be evaluated.
-					for _, v := range g.UpEdges(n) {
-						switch v.(type) {
-						case graphNodeExpandsInstances:
-							// Root module output values (which the following
-							// condition matches) are exempt because we know
-							// there is only ever exactly one instance of the
-							// root module, and so it's not actually important
-							// to expand it and so this lets us do a bit more
-							// pruning than we'd be able to do otherwise.
-							if tmp, ok := v.(graphNodeTemporaryValue); ok && !tmp.temporaryValue() {
-								continue
-							}
-
-							// expanders can always depend on module expansion
-							// themselves
-							return
-						case GraphNodeResourceInstance:
-							// resource instances always depend on their
-							// resource node, which is an expander
-							return
-						}
-					}
+	// we need to track nodes to keep, because the dependency trees can overlap,
+	// so we can't just remove all dependencies of nodes we don't want.
+	keep := make(dag.Set)
+
+	// Only keep destroyers, their providers, and anything the providers need
+	// for configuration. Since the destroyer should already be hooked up to the
+	// provider, keeping all the destroyer dependencies should suffice.
+	for _, n := range g.Vertices() {
+		// a special case of destroyer, is that by convention Terraform expects
+		// root outputs to be "destroyed", and the output node is what writes
+		// the nil state. A root module output currently identifies itself as a
+		// temporary value which is not temporary for that reason.
+		if tmp, ok := n.(graphNodeTemporaryValue); ok && !tmp.temporaryValue() {
+			log.Printf("[TRACE] pruneUnusedNodesTransformer: keeping root output %s", dag.VertexName(n))
+			keep.Add(n)
+			continue
+		}
 
-				case GraphNodeProvider:
-					// Only keep providers for evaluation if they have
-					// resources to handle.
-					// The provider transformers removed most unused providers
-					// earlier, however there may be more to prune now based on
-					// targeting or a destroy with no related instances in the
-					// state.
-					if g.MatchDescendant(n, func(v dag.Vertex) bool {
-						if _, ok := v.(GraphNodeProviderConsumer); ok {
-							return true
-						}
-						return false
-					}) {
-						return
-					}
+		// from here we only search for managed resource destroy nodes
+		n, ok := n.(GraphNodeDestroyer)
+		if !ok {
+			continue
+		}
 
-				default:
-					return
-				}
+		log.Printf("[TRACE] pruneUnusedNodesTransformer: keeping destroy node %s", dag.VertexName(n))
+		keep.Add(n)
 
-				log.Printf("[DEBUG] pruneUnusedNodes: %s is no longer needed, removing", dag.VertexName(n))
-				g.Remove(n)
-				removed = true
+		for _, anc := range g.Ancestors(n) {
+			log.Printf("[TRACE] pruneUnusedNodesTransformer: keeping %s as dependency of %s", dag.VertexName(anc), dag.VertexName(n))
+			keep.Add(anc)
+		}
+	}
 
-				// remove the node from our iteration as well
-				last := len(nodes) - 1
-				nodes[i], nodes[last] = nodes[last], nodes[i]
-				nodes = nodes[:last]
-			}()
+	for _, n := range g.Vertices() {
+		if !keep.Include(n) {
+			log.Printf("[TRACE] pruneUnusedNodesTransformer: removing %s", dag.VertexName(n))
+			g.Remove(n)
 		}
 	}