terraform: prune destroy nodes for resources not in diff

terraform/graph_config_node.go

@@ -336,7 +336,59 @@ func (n *graphNodeResourceDestroy) CreateNode() dag.Vertex {
 }
 
 func (n *graphNodeResourceDestroy) DiffId() string {
-	return ""
+	// Get the count, and specifically the raw value of the count
+	// (with interpolations and all). If the count is NOT a static "1",
+	// then we keep the destroy node no matter what.
+	//
+	// The reasoning for this is complicated and not intuitively obvious,
+	// but I attempt to explain it below.
+	//
+	// The destroy transform works by generating the worst case graph,
+	// with worst case being the case that every resource already exists
+	// and needs to be destroy/created (force-new). There is a single important
+	// edge case where this actually results in a real-life cycle: if a
+	// create-before-destroy (CBD) resource depends on a non-CBD resource.
+	// Imagine a EC2 instance "foo" with CBD depending on a security
+	// group "bar" without CBD, and conceptualize the worst case destroy
+	// order:
+	//
+	//   1.) SG must be destroyed (non-CBD)
+	//   2.) SG must be created/updated
+	//   3.) EC2 instance must be created (CBD, requires the SG be made)
+	//   4.) EC2 instance must be destroyed (requires SG be destroyed)
+	//
+	// Except, #1 depends on #4, since the SG can't be destroyed while
+	// an EC2 instance is using it (AWS API requirements). As you can see,
+	// this is a real life cycle that can't be automatically reconciled
+	// except under two conditions:
+	//
+	//   1.) SG is also CBD. This doesn't work 100% of the time though
+	//       since the non-CBD resource might not support CBD. To make matters
+	//       worse, the entire transitive closure of dependencies must be
+	//       CBD (if the SG depends on a VPC, you have the same problem).
+	//   2.) EC2 must not CBD. This can't happen automatically because CBD
+	//       is used as a way to ensure zero (or minimal) downtime Terraform
+	//       applies, and it isn't acceptable for TF to ignore this request,
+	//       since it can result in unexpected downtime.
+	//
+	// Therefore, we compromise with this edge case here: if there is
+	// a static count of "1", we prune the diff to remove cycles during a
+	// graph optimization path if we don't see the resource in the diff.
+	// If the count is set to ANYTHING other than a static "1" (variable,
+	// computed attribute, static number greater than 1), then we keep the
+	// destroy, since it is required for dynamic graph expansion to find
+	// orphan/tainted count objects.
+	//
+	// This isn't ideal logic, but its strictly better without introducing
+	// new impossibilities. It breaks the cycle in practical cases, and the
+	// cycle comes back in no cases we've found to be practical, but just
+	// as the cycle would already exist without this anyways.
+	count := n.Original.Resource.RawCount
+	if raw := count.Raw[count.Key]; raw != "1" {
+		return ""
+	}
+
+	return n.Original.Resource.Id()
 }
 
 // graphNodeModuleExpanded represents a module where the graph has

terraform/test-fixtures/transform-destroy-prune-count/main.tf

@@ -0,0 +1,6 @@
+resource "aws_instance" "foo" {}
+
+resource "aws_instance" "bar" {
+    value = "${aws_instance.foo.value}"
+    count = "5"
+}

terraform/transform_destroy.go

@@ -30,8 +30,13 @@ type GraphNodeDestroy interface {
 	// CreateNode returns the node used for the create side of this
 	// destroy. This must already exist within the graph.
 	CreateNode() dag.Vertex
+}
 
-	// Not used right now
+// GraphNodeDiffPrunable is the interface that can be implemented to
+// signal that this node can be pruned depending on what is in the diff.
+type GraphNodeDiffPrunable interface {
+	// DiffId is used to return the ID that should be checked for
+	// pruning this resource. If this is empty, pruning won't be done.
 	DiffId() string
 }
 
@@ -178,7 +183,7 @@ func (t *PruneDestroyTransformer) Transform(g *Graph) error {
 
 	for _, v := range g.Vertices() {
 		// If it is not a destroyer, we don't care
-		dn, ok := v.(GraphNodeDestroy)
+		dn, ok := v.(GraphNodeDiffPrunable)
 		if !ok {
 			continue
 		}

terraform/transform_destroy_test.go

@@ -119,6 +119,116 @@ func TestCreateBeforeDestroyTransformer_twice(t *testing.T) {
 	}
 }
 
+func TestPruneDestroyTransformer(t *testing.T) {
+	var diff *Diff
+	mod := testModule(t, "transform-destroy-basic")
+
+	g := Graph{Path: RootModulePath}
+	{
+		tf := &ConfigTransformer{Module: mod}
+		if err := tf.Transform(&g); err != nil {
+			t.Fatalf("err: %s", err)
+		}
+	}
+
+	{
+		tf := &DestroyTransformer{}
+		if err := tf.Transform(&g); err != nil {
+			t.Fatalf("err: %s", err)
+		}
+	}
+
+	{
+		tf := &PruneDestroyTransformer{Diff: diff}
+		if err := tf.Transform(&g); err != nil {
+			t.Fatalf("err: %s", err)
+		}
+	}
+
+	actual := strings.TrimSpace(g.String())
+	expected := strings.TrimSpace(testTransformPruneDestroyBasicStr)
+	if actual != expected {
+		t.Fatalf("bad:\n\n%s", actual)
+	}
+}
+
+func TestPruneDestroyTransformer_diff(t *testing.T) {
+	mod := testModule(t, "transform-destroy-basic")
+
+	diff := &Diff{
+		Modules: []*ModuleDiff{
+			&ModuleDiff{
+				Path: RootModulePath,
+				Resources: map[string]*InstanceDiff{
+					"aws_instance.bar": &InstanceDiff{},
+				},
+			},
+		},
+	}
+
+	g := Graph{Path: RootModulePath}
+	{
+		tf := &ConfigTransformer{Module: mod}
+		if err := tf.Transform(&g); err != nil {
+			t.Fatalf("err: %s", err)
+		}
+	}
+
+	{
+		tf := &DestroyTransformer{}
+		if err := tf.Transform(&g); err != nil {
+			t.Fatalf("err: %s", err)
+		}
+	}
+
+	{
+		tf := &PruneDestroyTransformer{Diff: diff}
+		if err := tf.Transform(&g); err != nil {
+			t.Fatalf("err: %s", err)
+		}
+	}
+
+	actual := strings.TrimSpace(g.String())
+	expected := strings.TrimSpace(testTransformPruneDestroyBasicDiffStr)
+	if actual != expected {
+		t.Fatalf("bad:\n\n%s", actual)
+	}
+}
+
+func TestPruneDestroyTransformer_count(t *testing.T) {
+	mod := testModule(t, "transform-destroy-prune-count")
+
+	diff := &Diff{}
+
+	g := Graph{Path: RootModulePath}
+	{
+		tf := &ConfigTransformer{Module: mod}
+		if err := tf.Transform(&g); err != nil {
+			t.Fatalf("err: %s", err)
+		}
+	}
+
+	{
+		tf := &DestroyTransformer{}
+		if err := tf.Transform(&g); err != nil {
+			t.Fatalf("err: %s", err)
+		}
+	}
+
+	{
+		tf := &PruneDestroyTransformer{Diff: diff}
+		if err := tf.Transform(&g); err != nil {
+			t.Fatalf("err: %s", err)
+		}
+	}
+
+	actual := strings.TrimSpace(g.String())
+	expected := strings.TrimSpace(testTransformPruneDestroyCountStr)
+	if actual != expected {
+		t.Fatalf("bad:\n\n%s", actual)
+	}
+}
+
 const testTransformDestroyBasicStr = `
 aws_instance.bar
   aws_instance.bar (destroy)
@@ -141,6 +251,28 @@ aws_lc.foo (destroy)
   aws_asg.bar (destroy)
 `
 
+const testTransformPruneDestroyBasicStr = `
+aws_instance.bar
+  aws_instance.foo
+aws_instance.foo
+`
+
+const testTransformPruneDestroyBasicDiffStr = `
+aws_instance.bar
+  aws_instance.bar (destroy)
+  aws_instance.foo
+aws_instance.bar (destroy)
+aws_instance.foo
+`
+
+const testTransformPruneDestroyCountStr = `
+aws_instance.bar
+  aws_instance.bar (destroy)
+  aws_instance.foo
+aws_instance.bar (destroy)
+aws_instance.foo
+`
+
 const testTransformCreateBeforeDestroyBasicStr = `
 aws_instance.web
 aws_instance.web (destroy)