From fe210e6da4d8541de32f6655f85bdba62d980bce Mon Sep 17 00:00:00 2001
From: Paul Hinze <phinze@phinze.com>
Date: Mon, 9 May 2016 12:18:57 -0500
Subject: [PATCH] core: Fix interp error msgs on module vars during destroy

Wow this one was tricky!

This bug presents itself only when using planfiles, because when doing a
straight `terraform apply` the interpolations are left in place from the
Plan graph walk and paper over the issue. (This detail is what made it
so hard to reproduce initially.)

Basically, graph nodes for module variables are visited during the apply
walk and attempt to interpolate. During a destroy walk, no attributes
are interpolated from resource nodes, so these interpolations fail.

This scenario is supposed to be handled by the `PruneNoopTransformer` -
in fact it's described as the example use case in the comment above it!

So the bug had to do with the actual behavor of the Noop transformer.
The resource nodes were not properly reporting themselves as Noops
during a destroy, so they were being left in the graph.

This in turn triggered the module variable nodes to see that they had
another node depending on them, so they also reported that they could
not be pruned.

Therefore we had two nodes in the graph that were effectively noops but
were being visited anyways. The module variable nodes were already graph
leaves, which is why this error presented itself as just stray messages
instead of actual failure to destroy.

Fixes #5440
Fixes #5708
Fixes #4988
Fixes #3268
---
 terraform/context_apply_test.go               | 88 +++++++++++++++++++
 terraform/graph_config_node_resource.go       |  6 ++
 .../child/main.tf                             |  5 ++
 .../apply-destroy-module-with-attrs/main.tf   |  6 ++
 4 files changed, 105 insertions(+)
 create mode 100644 terraform/test-fixtures/apply-destroy-module-with-attrs/child/main.tf
 create mode 100644 terraform/test-fixtures/apply-destroy-module-with-attrs/main.tf

diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go
index 9e390455b..c58ee803f 100644
--- a/terraform/context_apply_test.go
+++ b/terraform/context_apply_test.go
@@ -2495,6 +2495,94 @@ module.child.subchild.subsubchild:
 	}
 }
 
+// https://github.com/hashicorp/terraform/issues/5440
+func TestContext2Apply_destroyModuleWithAttrsReferencingResource(t *testing.T) {
+	m := testModule(t, "apply-destroy-module-with-attrs")
+	p := testProvider("aws")
+	p.ApplyFn = testApplyFn
+	p.DiffFn = testDiffFn
+
+	var state *State
+	var err error
+	{
+		ctx := testContext2(t, &ContextOpts{
+			Module: m,
+			Providers: map[string]ResourceProviderFactory{
+				"aws": testProviderFuncFixed(p),
+			},
+			Variables: map[string]string{
+				"key_name": "foobarkey",
+			},
+		})
+
+		// First plan and apply a create operation
+		if _, err := ctx.Plan(); err != nil {
+			t.Fatalf("err: %s", err)
+		}
+
+		state, err = ctx.Apply()
+		if err != nil {
+			t.Fatalf("err: %s", err)
+		}
+	}
+
+	h := new(HookRecordApplyOrder)
+	h.Active = true
+
+	{
+		ctx := testContext2(t, &ContextOpts{
+			Destroy: true,
+			Module:  m,
+			State:   state,
+			Hooks:   []Hook{h},
+			Providers: map[string]ResourceProviderFactory{
+				"aws": testProviderFuncFixed(p),
+			},
+			Variables: map[string]string{
+				"key_name": "foobarkey",
+			},
+		})
+
+		// First plan and apply a create operation
+		plan, err := ctx.Plan()
+		if err != nil {
+			t.Fatalf("err: %s", err)
+		}
+
+		var buf bytes.Buffer
+		if err := WritePlan(plan, &buf); err != nil {
+			t.Fatalf("err: %s", err)
+		}
+
+		planFromFile, err := ReadPlan(&buf)
+		if err != nil {
+			t.Fatalf("err: %s", err)
+		}
+
+		ctx = planFromFile.Context(&ContextOpts{
+			Providers: map[string]ResourceProviderFactory{
+				"aws": testProviderFuncFixed(p),
+			},
+		})
+
+		state, err = ctx.Apply()
+		if err != nil {
+			t.Fatalf("err: %s", err)
+		}
+	}
+
+	//Test that things were destroyed
+	actual := strings.TrimSpace(state.String())
+	expected := strings.TrimSpace(`
+<no state>
+module.child:
+  <no state>
+		`)
+	if actual != expected {
+		t.Fatalf("expected: \n%s\n\nbad: \n%s", expected, actual)
+	}
+}
+
 func TestContext2Apply_destroyOutputs(t *testing.T) {
 	m := testModule(t, "apply-destroy-outputs")
 	h := new(HookRecordApplyOrder)
diff --git a/terraform/graph_config_node_resource.go b/terraform/graph_config_node_resource.go
index 8abcd9156..41fc7e515 100644
--- a/terraform/graph_config_node_resource.go
+++ b/terraform/graph_config_node_resource.go
@@ -297,6 +297,12 @@ func (n *GraphNodeConfigResource) Noop(opts *NoopOpts) bool {
 		return true
 	}
 
+	// If the whole module is being destroyed, then the resource nodes in that
+	// module are irrelevant - we only need to keep the destroy nodes.
+	if opts.ModDiff != nil && opts.ModDiff.Destroy == true {
+		return true
+	}
+
 	// Grab the ID which is the prefix (in the case count > 0 at some point)
 	prefix := n.Resource.Id()
 
diff --git a/terraform/test-fixtures/apply-destroy-module-with-attrs/child/main.tf b/terraform/test-fixtures/apply-destroy-module-with-attrs/child/main.tf
new file mode 100644
index 000000000..2aa1e1045
--- /dev/null
+++ b/terraform/test-fixtures/apply-destroy-module-with-attrs/child/main.tf
@@ -0,0 +1,5 @@
+variable "vpc_id" {}
+
+resource "aws_instance" "child" {
+  vpc_id = "${var.vpc_id}"
+}
diff --git a/terraform/test-fixtures/apply-destroy-module-with-attrs/main.tf b/terraform/test-fixtures/apply-destroy-module-with-attrs/main.tf
new file mode 100644
index 000000000..0f26089ff
--- /dev/null
+++ b/terraform/test-fixtures/apply-destroy-module-with-attrs/main.tf
@@ -0,0 +1,6 @@
+resource "aws_instance" "vpc"   { }
+
+module "child" {
+  source = "./child"
+  vpc_id = "${aws_instance.vpc.id}"
+}