From 817a59328004075405091b654c0627283ebfed9d Mon Sep 17 00:00:00 2001
From: Mitchell Hashimoto <mitchell.hashimoto@gmail.com>
Date: Wed, 14 Dec 2016 21:39:13 -0800
Subject: [PATCH] terraform: destroy edges should take into account module
 variables

Fixes #10729

Destruction ordering wasn't taking into account ordering implied through
variables across module boundaries.

This is because to build the destruction ordering we create a
non-destruction graph to determine the _creation_ ordering (to properly
flip edges). This creation graph we create wasn't including module
variables. This PR adds that transform to the graph.
---
 terraform/graph_builder_apply_test.go         | 40 +++++++++++++++++++
 terraform/graph_test.go                       | 38 ++++++++++++++++++
 .../A/main.tf                                 |  7 ++++
 .../main.tf                                   | 11 +++++
 terraform/transform_destroy_edge.go           |  6 +++
 5 files changed, 102 insertions(+)
 create mode 100644 terraform/test-fixtures/graph-builder-apply-module-destroy/A/main.tf
 create mode 100644 terraform/test-fixtures/graph-builder-apply-module-destroy/main.tf

diff --git a/terraform/graph_builder_apply_test.go b/terraform/graph_builder_apply_test.go
index 57307a8f3..f6420154e 100644
--- a/terraform/graph_builder_apply_test.go
+++ b/terraform/graph_builder_apply_test.go
@@ -192,6 +192,46 @@ func TestApplyGraphBuilder_destroyCount(t *testing.T) {
 	}
 }
 
+func TestApplyGraphBuilder_moduleDestroy(t *testing.T) {
+	diff := &Diff{
+		Modules: []*ModuleDiff{
+			&ModuleDiff{
+				Path: []string{"root", "A"},
+				Resources: map[string]*InstanceDiff{
+					"null_resource.foo": &InstanceDiff{
+						Destroy: true,
+					},
+				},
+			},
+
+			&ModuleDiff{
+				Path: []string{"root", "B"},
+				Resources: map[string]*InstanceDiff{
+					"null_resource.foo": &InstanceDiff{
+						Destroy: true,
+					},
+				},
+			},
+		},
+	}
+
+	b := &ApplyGraphBuilder{
+		Module:    testModule(t, "graph-builder-apply-module-destroy"),
+		Diff:      diff,
+		Providers: []string{"null"},
+	}
+
+	g, err := b.Build(RootModulePath)
+	if err != nil {
+		t.Fatalf("err: %s", err)
+	}
+
+	testGraphHappensBefore(
+		t, g,
+		"module.B.null_resource.foo (destroy)",
+		"module.A.null_resource.foo (destroy)")
+}
+
 const testApplyGraphBuilderStr = `
 aws_instance.create
   provider.aws
diff --git a/terraform/graph_test.go b/terraform/graph_test.go
index 45770e213..3d5ae9aab 100644
--- a/terraform/graph_test.go
+++ b/terraform/graph_test.go
@@ -4,6 +4,8 @@ import (
 	"reflect"
 	"strings"
 	"testing"
+
+	"github.com/hashicorp/terraform/dag"
 )
 
 func TestGraphAdd(t *testing.T) {
@@ -86,6 +88,42 @@ func TestGraphWalk_panicWrap(t *testing.T) {
 	}
 }
 
+// testGraphHappensBefore is an assertion helper that tests that node
+// A (dag.VertexName value) happens before node B.
+func testGraphHappensBefore(t *testing.T, g *Graph, A, B string) {
+	// Find the B vertex
+	var vertexB dag.Vertex
+	for _, v := range g.Vertices() {
+		if dag.VertexName(v) == B {
+			vertexB = v
+			break
+		}
+	}
+	if vertexB == nil {
+		t.Fatalf(
+			"Expected %q before %q. Couldn't find %q in:\n\n%s",
+			A, B, B, g.String())
+	}
+
+	// Look at ancestors
+	deps, err := g.Ancestors(vertexB)
+	if err != nil {
+		t.Fatalf("Error: %s in graph:\n\n%s", err, g.String())
+	}
+
+	// Make sure B is in there
+	for _, v := range deps.List() {
+		if dag.VertexName(v) == A {
+			// Success
+			return
+		}
+	}
+
+	t.Fatalf(
+		"Expected %q before %q in:\n\n%s",
+		A, B, g.String())
+}
+
 type testGraphSubPath struct {
 	PathFn func() []string
 }
diff --git a/terraform/test-fixtures/graph-builder-apply-module-destroy/A/main.tf b/terraform/test-fixtures/graph-builder-apply-module-destroy/A/main.tf
new file mode 100644
index 000000000..656540d80
--- /dev/null
+++ b/terraform/test-fixtures/graph-builder-apply-module-destroy/A/main.tf
@@ -0,0 +1,7 @@
+variable "input" {}
+
+resource "null_resource" "foo" {
+    triggers { input = "${var.input}" }
+}
+
+output "output" { value = "${null_resource.foo.id}" }
diff --git a/terraform/test-fixtures/graph-builder-apply-module-destroy/main.tf b/terraform/test-fixtures/graph-builder-apply-module-destroy/main.tf
new file mode 100644
index 000000000..b374934bf
--- /dev/null
+++ b/terraform/test-fixtures/graph-builder-apply-module-destroy/main.tf
@@ -0,0 +1,11 @@
+variable "input" { default = "value" }
+
+module "A" {
+    source = "./A"
+    input  = "${var.input}"
+}
+
+module "B" {
+    source = "./A"
+    input  = "${module.A.output}"
+}
diff --git a/terraform/transform_destroy_edge.go b/terraform/transform_destroy_edge.go
index 8c067892c..c837cf172 100644
--- a/terraform/transform_destroy_edge.go
+++ b/terraform/transform_destroy_edge.go
@@ -131,6 +131,12 @@ func (t *DestroyEdgeTransformer) Transform(g *Graph) error {
 		&ParentProviderTransformer{},
 		&AttachProviderConfigTransformer{Module: t.Module},
 
+		// Add all the variables. We can depend on resources through
+		// variables due to module parameters, and we need to properly
+		// determine that.
+		&RootVariableTransformer{Module: t.Module},
+		&ModuleVariableTransformer{Module: t.Module},
+
 		&ReferenceTransformer{},
 	}