Merge pull request #11753 from hashicorp/b-cbd-count

core: non-CBD depending on CBD won't depend on destroy

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) {

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

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}"]
+}

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}"
+}

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

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)