diff --git a/terraform/graph_config_node.go b/terraform/graph_config_node.go index 9904b59e0..06af6c0fe 100644 --- a/terraform/graph_config_node.go +++ b/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 diff --git a/terraform/test-fixtures/transform-destroy-prune-count/main.tf b/terraform/test-fixtures/transform-destroy-prune-count/main.tf new file mode 100644 index 000000000..756ae10d5 --- /dev/null +++ b/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" +} diff --git a/terraform/transform_destroy.go b/terraform/transform_destroy.go index c5025e55d..44252bc60 100644 --- a/terraform/transform_destroy.go +++ b/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 } diff --git a/terraform/transform_destroy_test.go b/terraform/transform_destroy_test.go index af8a64ba9..416d5de38 100644 --- a/terraform/transform_destroy_test.go +++ b/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)