From f6161a7dc9dc56ab84e52cb4c6436073c4ed45c7 Mon Sep 17 00:00:00 2001
From: Mitchell Hashimoto <mitchell.hashimoto@gmail.com>
Date: Fri, 11 Nov 2016 14:29:19 -0800
Subject: [PATCH] terraform: destroy edge must include resources through
 outputs

This fixes: `TestContext2Apply_moduleDestroyOrder`

The new destroy graph wasn't properly creating edges that happened
_through_ an output, it was only created the edges for _direct_
dependents.

To fix this, the DestroyEdgeTransformer now creates the full transitive
list of destroy edges by walking all ancestors. This will create more
edges than are necessary but also will no longer miss resources through
an output.
---
 .../child/main.tf                             |  5 ++++
 .../transform-destroy-edge-module/main.tf     |  7 ++++++
 terraform/transform_destroy_edge.go           | 22 ++++++++++++----
 terraform/transform_destroy_edge_test.go      | 25 +++++++++++++++++++
 4 files changed, 54 insertions(+), 5 deletions(-)
 create mode 100644 terraform/test-fixtures/transform-destroy-edge-module/child/main.tf
 create mode 100644 terraform/test-fixtures/transform-destroy-edge-module/main.tf

diff --git a/terraform/test-fixtures/transform-destroy-edge-module/child/main.tf b/terraform/test-fixtures/transform-destroy-edge-module/child/main.tf
new file mode 100644
index 000000000..cc2f3b7ff
--- /dev/null
+++ b/terraform/test-fixtures/transform-destroy-edge-module/child/main.tf
@@ -0,0 +1,5 @@
+resource "aws_instance" "b" {
+    value = "foo"
+}
+
+output "output" { value = "${aws_instance.b.value}" }
diff --git a/terraform/test-fixtures/transform-destroy-edge-module/main.tf b/terraform/test-fixtures/transform-destroy-edge-module/main.tf
new file mode 100644
index 000000000..207e6d043
--- /dev/null
+++ b/terraform/test-fixtures/transform-destroy-edge-module/main.tf
@@ -0,0 +1,7 @@
+resource "aws_instance" "a" {
+    value = "${module.child.output}"
+}
+
+module "child" {
+    source = "./child"
+}
diff --git a/terraform/transform_destroy_edge.go b/terraform/transform_destroy_edge.go
index dd6ed114c..194048c92 100644
--- a/terraform/transform_destroy_edge.go
+++ b/terraform/transform_destroy_edge.go
@@ -116,8 +116,10 @@ func (t *DestroyEdgeTransformer) Transform(g *Graph) error {
 	// in the graph. BUT if resource A is just pure destroy, then only
 	// destroy A is in the graph, and create A is not.
 	steps := []GraphTransformer{
+		&OutputTransformer{Module: t.Module},
 		&AttachResourceConfigTransformer{Module: t.Module},
 		&AttachStateTransformer{State: t.State},
+		&ReferenceTransformer{},
 	}
 
 	// Go through all the nodes being destroyed and create a graph.
@@ -127,6 +129,7 @@ func (t *DestroyEdgeTransformer) Transform(g *Graph) error {
 	//   A, B (with no edges)
 	//
 	var tempG Graph
+	var tempDestroyed []dag.Vertex
 	for d, _ := range destroyers {
 		// d is what is being destroyed. We parse the resource address
 		// which it came from it is a panic if this fails.
@@ -140,6 +143,7 @@ func (t *DestroyEdgeTransformer) Transform(g *Graph) error {
 		// attach config and state transformers then ask for references.
 		node := &NodeAbstractResource{Addr: addr}
 		tempG.Add(node)
+		tempDestroyed = append(tempDestroyed, node)
 	}
 
 	// Run the graph transforms so we have the information we need to
@@ -150,14 +154,22 @@ func (t *DestroyEdgeTransformer) Transform(g *Graph) error {
 		}
 	}
 
-	// Create a reference map for easy lookup
-	refMap := NewReferenceMap(tempG.Vertices())
+	log.Printf("[TRACE] DestroyEdgeTransformer: reference graph: %s", tempG.String())
 
 	// Go through all the nodes in the graph and determine what they
 	// depend on.
-	for _, v := range tempG.Vertices() {
-		// Find all the references
-		refs, _ := refMap.References(v)
+	for _, v := range tempDestroyed {
+		// Find all descendents of this to determine the edges we'll depend on
+		vs, err := tempG.Ancestors(v)
+		if err != nil {
+			return err
+		}
+
+		refs := make([]dag.Vertex, 0, vs.Len())
+		for _, raw := range vs.List() {
+			refs = append(refs, raw.(dag.Vertex))
+		}
+
 		log.Printf(
 			"[TRACE] DestroyEdgeTransformer: creation node %q references %v",
 			dag.VertexName(v), refs)
diff --git a/terraform/transform_destroy_edge_test.go b/terraform/transform_destroy_edge_test.go
index 9475e5165..873b01eac 100644
--- a/terraform/transform_destroy_edge_test.go
+++ b/terraform/transform_destroy_edge_test.go
@@ -78,6 +78,24 @@ func TestDestroyEdgeTransformer_selfRef(t *testing.T) {
 	}
 }
 
+func TestDestroyEdgeTransformer_module(t *testing.T) {
+	g := Graph{Path: RootModulePath}
+	g.Add(&graphNodeDestroyerTest{AddrString: "module.child.aws_instance.b"})
+	g.Add(&graphNodeDestroyerTest{AddrString: "aws_instance.a"})
+	tf := &DestroyEdgeTransformer{
+		Module: testModule(t, "transform-destroy-edge-module"),
+	}
+	if err := tf.Transform(&g); err != nil {
+		t.Fatalf("err: %s", err)
+	}
+
+	actual := strings.TrimSpace(g.String())
+	expected := strings.TrimSpace(testTransformDestroyEdgeModuleStr)
+	if actual != expected {
+		t.Fatalf("bad:\n\n%s", actual)
+	}
+}
+
 type graphNodeCreatorTest struct {
 	AddrString string
 }
@@ -125,6 +143,7 @@ test.B (destroy)
 const testTransformDestroyEdgeMultiStr = `
 test.A (destroy)
   test.B (destroy)
+  test.C (destroy)
 test.B (destroy)
   test.C (destroy)
 test.C (destroy)
@@ -133,3 +152,9 @@ test.C (destroy)
 const testTransformDestroyEdgeSelfRefStr = `
 test.A (destroy)
 `
+
+const testTransformDestroyEdgeModuleStr = `
+aws_instance.a (destroy)
+module.child.aws_instance.b (destroy)
+  aws_instance.a (destroy)
+`