From f33ef43195512e72befda2c2b28c77a97487b808 Mon Sep 17 00:00:00 2001
From: Paul Hinze <phinze@phinze.com>
Date: Mon, 16 May 2016 14:39:38 -0700
Subject: [PATCH] core: Fix destroy when modules vars are used in resource
 counts

A new problem was introduced by the prior fixes for destroy
interpolation messages when resources depend on module variables with
a _count_ attribute, this makes the variable crucial for properly
building the graph - even in destroys. So removing all module variables
from the graph as noops was overzealous.

By borrowing the logic in `DestroyEdgeInclude` we are able to determine
if we need to keep a given module variable relatively easily.

I'd like to overhaul the `Destroy: true` implementation so that it does
not depend on config at all, but I want to continue for now with the
targeted fixes that we can backport into the 0.6 series.
---
 terraform/context_apply_test.go               | 84 +++++++++++++++++++
 terraform/graph_config_node_variable.go       | 14 +++-
 .../child/main.tf                             |  5 ++
 .../apply-destroy-mod-var-and-count/main.tf   |  4 +
 4 files changed, 105 insertions(+), 2 deletions(-)
 create mode 100644 terraform/test-fixtures/apply-destroy-mod-var-and-count/child/main.tf
 create mode 100644 terraform/test-fixtures/apply-destroy-mod-var-and-count/main.tf

diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go
index ca172ca1f7..4be4769824 100644
--- a/terraform/context_apply_test.go
+++ b/terraform/context_apply_test.go
@@ -2622,6 +2622,90 @@ module.child:
 	}
 }
 
+func TestContext2Apply_destroyWithModuleVariableAndCount(t *testing.T) {
+	m := testModule(t, "apply-destroy-mod-var-and-count")
+	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),
+			},
+		})
+
+		// First plan and apply a create operation
+		if _, err := ctx.Plan(); err != nil {
+			t.Fatalf("plan err: %s", err)
+		}
+
+		state, err = ctx.Apply()
+		if err != nil {
+			t.Fatalf("apply 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),
+			},
+		})
+
+		// First plan and apply a create operation
+		plan, err := ctx.Plan()
+		if err != nil {
+			t.Fatalf("destroy plan err: %s", err)
+		}
+
+		var buf bytes.Buffer
+		if err := WritePlan(plan, &buf); err != nil {
+			t.Fatalf("plan write err: %s", err)
+		}
+
+		planFromFile, err := ReadPlan(&buf)
+		if err != nil {
+			t.Fatalf("plan read err: %s", err)
+		}
+
+		ctx, err = planFromFile.Context(&ContextOpts{
+			Providers: map[string]ResourceProviderFactory{
+				"aws": testProviderFuncFixed(p),
+			},
+		})
+		if err != nil {
+			t.Fatalf("err: %s", err)
+		}
+
+		state, err = ctx.Apply()
+		if err != nil {
+			t.Fatalf("destroy apply 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_variable.go b/terraform/graph_config_node_variable.go
index f085205322..0c8a3ded0c 100644
--- a/terraform/graph_config_node_variable.go
+++ b/terraform/graph_config_node_variable.go
@@ -94,9 +94,19 @@ func (n *GraphNodeConfigVariable) Noop(opts *NoopOpts) bool {
 	// the flat node's implementation of Path() below.
 	modDiff := opts.Diff.ModuleByPath(n.ModulePath)
 
-	// If we're destroying, we have no need of variables.
+	// If we're destroying, we have no need of variables unless they are depended
+	// on by the count of a resource.
 	if modDiff != nil && modDiff.Destroy {
-		log.Printf("[DEBUG] Destroy diff, treating variable as a noop")
+		for _, v := range opts.Graph.UpEdges(opts.Vertex).List() {
+			// Here we borrow the implementation of DestroyEdgeInclude, whose logic
+			// and semantics are exactly what we want here.
+			if n.DestroyEdgeInclude(v) {
+				log.Printf("[DEBUG] Variable has destroy edge from %s, not a noop",
+					dag.VertexName(v))
+				return false
+			}
+		}
+		log.Printf("[DEBUG] Variable has no included destroy edges: noop!")
 		return true
 	}
 
diff --git a/terraform/test-fixtures/apply-destroy-mod-var-and-count/child/main.tf b/terraform/test-fixtures/apply-destroy-mod-var-and-count/child/main.tf
new file mode 100644
index 0000000000..67dac02a27
--- /dev/null
+++ b/terraform/test-fixtures/apply-destroy-mod-var-and-count/child/main.tf
@@ -0,0 +1,5 @@
+variable "mod_count" { }
+
+resource "aws_instance" "foo" {
+  count = "${var.mod_count}"
+}
diff --git a/terraform/test-fixtures/apply-destroy-mod-var-and-count/main.tf b/terraform/test-fixtures/apply-destroy-mod-var-and-count/main.tf
new file mode 100644
index 0000000000..918b40d067
--- /dev/null
+++ b/terraform/test-fixtures/apply-destroy-mod-var-and-count/main.tf
@@ -0,0 +1,4 @@
+module "child" {
+  source    = "./child"
+  mod_count = "3"
+}