diff --git a/terraform/graph_builder_apply_test.go b/terraform/graph_builder_apply_test.go index 659f8e141..4dbb87a44 100644 --- a/terraform/graph_builder_apply_test.go +++ b/terraform/graph_builder_apply_test.go @@ -88,6 +88,72 @@ func TestApplyGraphBuilder(t *testing.T) { } } +// This tests the ordering of two resources where a non-CBD depends +// on a CBD. GH-11349. +func TestApplyGraphBuilder_depCbd(t *testing.T) { + diff := &Diff{ + Modules: []*ModuleDiff{ + &ModuleDiff{ + Path: []string{"root"}, + Resources: map[string]*InstanceDiff{ + "aws_instance.A": &InstanceDiff{ + Destroy: true, + Attributes: map[string]*ResourceAttrDiff{ + "name": &ResourceAttrDiff{ + Old: "", + New: "foo", + RequiresNew: true, + }, + }, + }, + + "aws_instance.B": &InstanceDiff{ + Attributes: map[string]*ResourceAttrDiff{ + "name": &ResourceAttrDiff{ + Old: "", + New: "foo", + }, + }, + }, + }, + }, + }, + } + + b := &ApplyGraphBuilder{ + Module: testModule(t, "graph-builder-apply-dep-cbd"), + Diff: diff, + Providers: []string{"aws"}, + Provisioners: []string{"exec"}, + DisableReduce: true, + } + + g, err := b.Build(RootModulePath) + if err != nil { + t.Fatalf("err: %s", err) + } + t.Logf("Graph: %s", g.String()) + + if !reflect.DeepEqual(g.Path, RootModulePath) { + t.Fatalf("bad: %#v", g.Path) + } + + // Create A, Modify B, Destroy A + + testGraphHappensBefore( + t, g, + "aws_instance.A", + "aws_instance.A (destroy)") + testGraphHappensBefore( + t, g, + "aws_instance.A", + "aws_instance.B") + testGraphHappensBefore( + t, g, + "aws_instance.B", + "aws_instance.A (destroy)") +} + // This tests the ordering of two resources that are both CBD that // require destroy/create. func TestApplyGraphBuilder_doubleCBD(t *testing.T) { diff --git a/terraform/node_resource_destroy.go b/terraform/node_resource_destroy.go index 0bf424a39..9a0bc8c2d 100644 --- a/terraform/node_resource_destroy.go +++ b/terraform/node_resource_destroy.go @@ -46,9 +46,21 @@ func (n *NodeDestroyResource) ModifyCreateBeforeDestroy(v bool) error { // GraphNodeReferenceable, overriding NodeAbstractResource func (n *NodeDestroyResource) ReferenceableName() []string { + // We modify our referenceable name to have the suffix of ".destroy" + // since depending on the creation side doesn't necessarilly mean + // depending on destruction. + suffix := ".destroy" + + // If we're CBD, we also append "-cbd". This is because CBD will setup + // its own edges (in CBDEdgeTransformer). Depending on the "destroy" + // side generally doesn't mean depending on CBD as well. See GH-11349 + if n.CreateBeforeDestroy() { + suffix += "-cbd" + } + result := n.NodeAbstractResource.ReferenceableName() for i, v := range result { - result[i] = v + ".destroy" + result[i] = v + suffix } return result diff --git a/terraform/test-fixtures/graph-builder-apply-dep-cbd/main.tf b/terraform/test-fixtures/graph-builder-apply-dep-cbd/main.tf new file mode 100644 index 000000000..0a25da772 --- /dev/null +++ b/terraform/test-fixtures/graph-builder-apply-dep-cbd/main.tf @@ -0,0 +1,7 @@ +resource "aws_instance" "A" { + lifecycle { create_before_destroy = true } +} + +resource "aws_instance" "B" { + value = ["${aws_instance.A.*.id}"] +} diff --git a/terraform/test-fixtures/transform-destroy-edge-splat/main.tf b/terraform/test-fixtures/transform-destroy-edge-splat/main.tf new file mode 100644 index 000000000..3ed06ae1b --- /dev/null +++ b/terraform/test-fixtures/transform-destroy-edge-splat/main.tf @@ -0,0 +1,5 @@ +resource "test" "A" {} +resource "test" "B" { + count = 2 + value = "${test.A.*.value}" +} diff --git a/terraform/transform_destroy_cbd.go b/terraform/transform_destroy_cbd.go index e1a525b88..edfb460bf 100644 --- a/terraform/transform_destroy_cbd.go +++ b/terraform/transform_destroy_cbd.go @@ -89,9 +89,20 @@ func (t *CBDEdgeTransformer) Transform(g *Graph) error { g.Connect(&DestroyEdge{S: de.Target(), T: de.Source()}) } + // If the address has an index, we strip that. Our depMap creation + // graph doesn't expand counts so we don't currently get _exact_ + // dependencies. One day when we limit dependencies more exactly + // this will have to change. We have a test case covering this + // (depNonCBDCountBoth) so it'll be caught. + addr := dn.DestroyAddr() + if addr.Index >= 0 { + addr = addr.Copy() // Copy so that we don't modify any pointers + addr.Index = -1 + } + // Add this to the list of nodes that we need to fix up // the edges for (step 2 above in the docs). - key := dn.DestroyAddr().String() + key := addr.String() destroyMap[key] = append(destroyMap[key], v) } @@ -134,9 +145,19 @@ func (t *CBDEdgeTransformer) Transform(g *Graph) error { // Get the address addr := rn.CreateAddr() - key := addr.String() + + // If the address has an index, we strip that. Our depMap creation + // graph doesn't expand counts so we don't currently get _exact_ + // dependencies. One day when we limit dependencies more exactly + // this will have to change. We have a test case covering this + // (depNonCBDCount) so it'll be caught. + if addr.Index >= 0 { + addr = addr.Copy() // Copy so that we don't modify any pointers + addr.Index = -1 + } // If there is nothing this resource should depend on, ignore it + key := addr.String() dns, ok := depMap[key] if !ok { continue diff --git a/terraform/transform_destroy_cbd_test.go b/terraform/transform_destroy_cbd_test.go index bb8fc24f8..0f79a1987 100644 --- a/terraform/transform_destroy_cbd_test.go +++ b/terraform/transform_destroy_cbd_test.go @@ -68,6 +68,93 @@ func TestCBDEdgeTransformer_depNonCBD(t *testing.T) { } } +func TestCBDEdgeTransformer_depNonCBDCount(t *testing.T) { + g := Graph{Path: RootModulePath} + g.Add(&graphNodeCreatorTest{AddrString: "test.A"}) + g.Add(&graphNodeCreatorTest{AddrString: "test.B[0]"}) + g.Add(&graphNodeCreatorTest{AddrString: "test.B[1]"}) + g.Add(&graphNodeDestroyerTest{AddrString: "test.A", CBD: true}) + + module := testModule(t, "transform-destroy-edge-splat") + + { + tf := &DestroyEdgeTransformer{ + Module: module, + } + if err := tf.Transform(&g); err != nil { + t.Fatalf("err: %s", err) + } + } + + { + tf := &CBDEdgeTransformer{Module: module} + if err := tf.Transform(&g); err != nil { + t.Fatalf("err: %s", err) + } + } + + actual := strings.TrimSpace(g.String()) + expected := strings.TrimSpace(` +test.A +test.A (destroy) + test.A + test.B[0] + test.B[1] +test.B[0] +test.B[1] + `) + if actual != expected { + t.Fatalf("bad:\n\n%s", actual) + } +} + +func TestCBDEdgeTransformer_depNonCBDCountBoth(t *testing.T) { + g := Graph{Path: RootModulePath} + g.Add(&graphNodeCreatorTest{AddrString: "test.A[0]"}) + g.Add(&graphNodeCreatorTest{AddrString: "test.A[1]"}) + g.Add(&graphNodeCreatorTest{AddrString: "test.B[0]"}) + g.Add(&graphNodeCreatorTest{AddrString: "test.B[1]"}) + g.Add(&graphNodeDestroyerTest{AddrString: "test.A[0]", CBD: true}) + g.Add(&graphNodeDestroyerTest{AddrString: "test.A[1]", CBD: true}) + + module := testModule(t, "transform-destroy-edge-splat") + + { + tf := &DestroyEdgeTransformer{ + Module: module, + } + if err := tf.Transform(&g); err != nil { + t.Fatalf("err: %s", err) + } + } + + { + tf := &CBDEdgeTransformer{Module: module} + if err := tf.Transform(&g); err != nil { + t.Fatalf("err: %s", err) + } + } + + actual := strings.TrimSpace(g.String()) + expected := strings.TrimSpace(` +test.A[0] +test.A[0] (destroy) + test.A[0] + test.B[0] + test.B[1] +test.A[1] +test.A[1] (destroy) + test.A[1] + test.B[0] + test.B[1] +test.B[0] +test.B[1] + `) + if actual != expected { + t.Fatalf("bad:\n\n%s", actual) + } +} + const testTransformCBDEdgeBasicStr = ` test.A test.A (destroy)