Merge pull request #25922 from hashicorp/jbardin/destroy-pruning

Fix node-pruning during destroy

terraform/context_apply_test.go

@@ -11648,3 +11648,112 @@ output "output" {
 		t.Fatalf("destroy apply errors: %s", diags.Err())
 	}
 }
+
+// Destroying properly requires pruning out all unneeded config nodes to
+// prevent incorrect expansion evaluation.
+func TestContext2Apply_destroyInterModuleExpansion(t *testing.T) {
+	m := testModuleInline(t, map[string]string{
+		"main.tf": `
+data "test_data_source" "a" {
+  for_each = {
+    one = "thing"
+  }
+}
+
+locals {
+  module_input = {
+    for k, v in data.test_data_source.a : k => v.id
+  }
+}
+
+module "mod1" {
+  source = "./mod"
+  input = local.module_input
+}
+
+module "mod2" {
+  source = "./mod"
+  input = module.mod1.outputs
+}
+
+resource "test_instance" "bar" {
+  for_each = module.mod2.outputs
+}
+
+output "module_output" {
+  value = module.mod2.outputs
+}
+output "test_instances" {
+  value = test_instance.bar
+}
+`,
+		"mod/main.tf": `
+variable "input" {
+}
+
+data "test_data_source" "foo" {
+  for_each = var.input
+}
+
+output "outputs" {
+  value = data.test_data_source.foo
+}
+`})
+
+	p := testProvider("test")
+	p.ApplyFn = testApplyFn
+	p.DiffFn = testDiffFn
+	p.ReadDataSourceFn = func(req providers.ReadDataSourceRequest) providers.ReadDataSourceResponse {
+		return providers.ReadDataSourceResponse{
+			State: cty.ObjectVal(map[string]cty.Value{
+				"id":  cty.StringVal("data_source"),
+				"foo": cty.StringVal("output"),
+			}),
+		}
+	}
+
+	ctx := testContext2(t, &ContextOpts{
+		Config: m,
+		Providers: map[addrs.Provider]providers.Factory{
+			addrs.NewDefaultProvider("test"): testProviderFuncFixed(p),
+		},
+	})
+
+	if _, diags := ctx.Plan(); diags.HasErrors() {
+		t.Fatalf("plan errors: %s", diags.Err())
+	}
+
+	state, diags := ctx.Apply()
+	if diags.HasErrors() {
+		t.Fatalf("apply errors: %s", diags.Err())
+	}
+
+	destroy := func() {
+		ctx = testContext2(t, &ContextOpts{
+			Config: m,
+			Providers: map[addrs.Provider]providers.Factory{
+				addrs.NewDefaultProvider("test"): testProviderFuncFixed(p),
+			},
+
+			State:   state,
+			Destroy: true,
+		})
+
+		if _, diags := ctx.Refresh(); diags.HasErrors() {
+			t.Fatalf("destroy plan errors: %s", diags.Err())
+		}
+
+		if _, diags := ctx.Plan(); diags.HasErrors() {
+			t.Fatalf("destroy plan errors: %s", diags.Err())
+		}
+
+		state, diags = ctx.Apply()
+		if diags.HasErrors() {
+			t.Fatalf("destroy apply errors: %s", diags.Err())
+		}
+	}
+
+	destroy()
+	// Destroying again from the empty state should not cause any errors either
+	destroy()
+}

terraform/transform_destroy_edge.go

@@ -2,7 +2,6 @@ package terraform
 
 import (
 	"log"
-	"sort"
 
 	"github.com/hashicorp/terraform/addrs"
 	"github.com/hashicorp/terraform/states"
@@ -188,69 +187,9 @@ func (t *pruneUnusedNodesTransformer) Transform(g *Graph) error {
 	// dependencies from child modules, freeing up nodes in the parent module
 	// to also be removed.
 
-	// First collect the nodes into their respective modules based on
-	// configuration path.
-	moduleMap := make(map[string]pruneUnusedNodesMod)
-	for _, v := range g.Vertices() {
-		var path addrs.Module
-		switch v := v.(type) {
-		case GraphNodeModulePath:
-			path = v.ModulePath()
-		default:
-			continue
-		}
-		m := moduleMap[path.String()]
-		m.addr = path
-		m.nodes = append(m.nodes, v)
+	nodes := g.Vertices()
 
-		moduleMap[path.String()] = m
-	}
-
-	// now we need to restructure the modules so we can sort them
-	var modules []pruneUnusedNodesMod
-
-	for _, mod := range moduleMap {
-		modules = append(modules, mod)
-	}
-
-	// Sort them by path length, longest first, so that we start with the
-	// deepest modules. The order of modules at the same tree level doesn't
-	// matter, we just need to ensure that child modules are processed before
-	// parent modules.
-	sort.Slice(modules, func(i, j int) bool {
-		return len(modules[i].addr) > len(modules[j].addr)
-	})
-
-	for _, mod := range modules {
-		mod.removeUnused(g)
-	}
-
-	return nil
-}
-
-// pruneUnusedNodesMod is a container to hold the nodes that belong to a
-// particular configuration module for the pruneUnusedNodesTransformer
-type pruneUnusedNodesMod struct {
-	addr  addrs.Module
-	nodes []dag.Vertex
-}
-
-// Remove any unused locals, variables, outputs and expanders.  Since module
-// closers can also lookup expansion info to detect orphaned instances, disable
-// them if their associated expander is removed.
-func (m *pruneUnusedNodesMod) removeUnused(g *Graph) {
-	// We modify the nodes slice during processing here.
-	// Make a copy so no one is surprised by this changing in the future.
-	nodes := make([]dag.Vertex, len(m.nodes))
-	copy(nodes, m.nodes)
-
-	// since we have no defined structure within the module, just cycle through
-	// the nodes in each module until there are no more removals
-	removed := true
-	for {
-		if !removed {
-			return
-		}
+	for removed := true; removed; {
 		removed = false
 
 		for i := 0; i < len(nodes); i++ {
@@ -307,4 +246,6 @@ func (m *pruneUnusedNodesMod) removeUnused(g *Graph) {
 			}()
 		}
 	}
+
+	return nil
 }