Merge pull request #10522 from hashicorp/b-destroy-cbd

terraform: apply resource must depend on destroy deps

terraform/context_apply_test.go

@@ -2885,6 +2885,107 @@ func TestContext2Apply_multiVarOrderInterp(t *testing.T) {
 	}
 }
 
+// Based on GH-10440 where a graph edge wasn't properly being created
+// between a modified resource and a count instance being destroyed.
+func TestContext2Apply_multiVarCountDec(t *testing.T) {
+	var s *State
+
+	// First create resources. Nothing sneaky here.
+	{
+		m := testModule(t, "apply-multi-var-count-dec")
+		p := testProvider("aws")
+		p.ApplyFn = testApplyFn
+		p.DiffFn = testDiffFn
+		ctx := testContext2(t, &ContextOpts{
+			Module: m,
+			Providers: map[string]ResourceProviderFactory{
+				"aws": testProviderFuncFixed(p),
+			},
+			Variables: map[string]interface{}{
+				"count": "2",
+			},
+		})
+
+		if _, err := ctx.Plan(); err != nil {
+			t.Fatalf("err: %s", err)
+		}
+
+		state, err := ctx.Apply()
+		if err != nil {
+			t.Fatalf("err: %s", err)
+		}
+
+		t.Logf("Step 1 state: %s", state)
+
+		s = state
+	}
+
+	// Decrease the count by 1 and verify that everything happens in the
+	// right order.
+	{
+		m := testModule(t, "apply-multi-var-count-dec")
+		p := testProvider("aws")
+		p.ApplyFn = testApplyFn
+		p.DiffFn = testDiffFn
+
+		// Verify that aws_instance.bar is modified first and nothing
+		// else happens at the same time.
+		var checked bool
+		var called int32
+		var lock sync.Mutex
+		p.ApplyFn = func(
+			info *InstanceInfo,
+			is *InstanceState,
+			id *InstanceDiff) (*InstanceState, error) {
+			lock.Lock()
+			defer lock.Unlock()
+
+			if info.HumanId() == "aws_instance.bar" {
+				checked = true
+
+				// Sleep to allow parallel execution
+				time.Sleep(50 * time.Millisecond)
+
+				// Verify that called is 0 (dep not called)
+				if atomic.LoadInt32(&called) != 1 {
+					return nil, fmt.Errorf("nothing else should be called")
+				}
+			}
+
+			atomic.AddInt32(&called, 1)
+			return testApplyFn(info, is, id)
+		}
+
+		ctx := testContext2(t, &ContextOpts{
+			State:  s,
+			Module: m,
+			Providers: map[string]ResourceProviderFactory{
+				"aws": testProviderFuncFixed(p),
+			},
+			Variables: map[string]interface{}{
+				"count": "1",
+			},
+		})
+
+		if _, err := ctx.Plan(); err != nil {
+			t.Fatalf("err: %s", err)
+		}
+
+		state, err := ctx.Apply()
+		if err != nil {
+			t.Fatalf("err: %s", err)
+		}
+
+		if !checked {
+			t.Fatal("apply never called")
+		}
+
+		t.Logf("Step 2 state: %s", state)
+
+		s = state
+	}
+}
+
 func TestContext2Apply_nilDiff(t *testing.T) {
 	m := testModule(t, "apply-good")
 	p := testProvider("aws")

terraform/graph_builder_apply_test.go

@@ -88,6 +88,110 @@ func TestApplyGraphBuilder(t *testing.T) {
 	}
 }
 
+// This tests the ordering of two resources that are both CBD that
+// require destroy/create.
+func TestApplyGraphBuilder_doubleCBD(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",
+							},
+						},
+					},
+
+					"aws_instance.B": &InstanceDiff{
+						Destroy: true,
+						Attributes: map[string]*ResourceAttrDiff{
+							"name": &ResourceAttrDiff{
+								Old: "",
+								New: "foo",
+							},
+						},
+					},
+				},
+			},
+		},
+	}
+
+	b := &ApplyGraphBuilder{
+		Module:        testModule(t, "graph-builder-apply-double-cbd"),
+		Diff:          diff,
+		Providers:     []string{"aws"},
+		Provisioners:  []string{"exec"},
+		DisableReduce: true,
+	}
+
+	g, err := b.Build(RootModulePath)
+	if err != nil {
+		t.Fatalf("err: %s", err)
+	}
+
+	if !reflect.DeepEqual(g.Path, RootModulePath) {
+		t.Fatalf("bad: %#v", g.Path)
+	}
+
+	actual := strings.TrimSpace(g.String())
+	expected := strings.TrimSpace(testApplyGraphBuilderDoubleCBDStr)
+	if actual != expected {
+		t.Fatalf("bad: %s", actual)
+	}
+}
+
+// This tests the ordering of destroying a single count of a resource.
+func TestApplyGraphBuilder_destroyCount(t *testing.T) {
+	diff := &Diff{
+		Modules: []*ModuleDiff{
+			&ModuleDiff{
+				Path: []string{"root"},
+				Resources: map[string]*InstanceDiff{
+					"aws_instance.A.1": &InstanceDiff{
+						Destroy: true,
+					},
+
+					"aws_instance.B": &InstanceDiff{
+						Attributes: map[string]*ResourceAttrDiff{
+							"name": &ResourceAttrDiff{
+								Old: "",
+								New: "foo",
+							},
+						},
+					},
+				},
+			},
+		},
+	}
+
+	b := &ApplyGraphBuilder{
+		Module:        testModule(t, "graph-builder-apply-count"),
+		Diff:          diff,
+		Providers:     []string{"aws"},
+		Provisioners:  []string{"exec"},
+		DisableReduce: true,
+	}
+
+	g, err := b.Build(RootModulePath)
+	if err != nil {
+		t.Fatalf("err: %s", err)
+	}
+
+	if !reflect.DeepEqual(g.Path, RootModulePath) {
+		t.Fatalf("bad: %#v", g.Path)
+	}
+
+	actual := strings.TrimSpace(g.String())
+	expected := strings.TrimSpace(testApplyGraphBuilderDestroyCountStr)
+	if actual != expected {
+		t.Fatalf("bad: %s", actual)
+	}
+}
+
 const testApplyGraphBuilderStr = `
 aws_instance.create
   provider.aws
@@ -113,3 +217,39 @@ module.child.provider.aws
 module.child.provisioner.exec
 provider.aws
 `
+
+const testApplyGraphBuilderDoubleCBDStr = `
+aws_instance.A
+  provider.aws
+aws_instance.A (destroy)
+  aws_instance.A
+  aws_instance.B
+  aws_instance.B (destroy)
+  provider.aws
+aws_instance.B
+  aws_instance.A
+  provider.aws
+aws_instance.B (destroy)
+  aws_instance.B
+  provider.aws
+meta.count-boundary (count boundary fixup)
+  aws_instance.A
+  aws_instance.A (destroy)
+  aws_instance.B
+  aws_instance.B (destroy)
+  provider.aws
+provider.aws
+`
+
+const testApplyGraphBuilderDestroyCountStr = `
+aws_instance.A[1] (destroy)
+  provider.aws
+aws_instance.B
+  aws_instance.A[1] (destroy)
+  provider.aws
+meta.count-boundary (count boundary fixup)
+  aws_instance.A[1] (destroy)
+  aws_instance.B
+  provider.aws
+provider.aws
+`

terraform/node_resource_apply.go

@@ -17,6 +17,36 @@ func (n *NodeApplyableResource) CreateAddr() *ResourceAddress {
 	return n.NodeAbstractResource.Addr
 }
 
+// GraphNodeReferencer, overriding NodeAbstractResource
+func (n *NodeApplyableResource) References() []string {
+	result := n.NodeAbstractResource.References()
+
+	// The "apply" side of a resource generally also depends on the
+	// destruction of its dependencies as well. For example, if a LB
+	// references a set of VMs with ${vm.foo.*.id}, then we must wait for
+	// the destruction so we get the newly updated list of VMs.
+	//
+	// The exception here is CBD. When CBD is set, we don't do this since
+	// it would create a cycle. By not creating a cycle, we require two
+	// applies since the first apply the creation step will use the OLD
+	// values (pre-destroy) and the second step will update.
+	//
+	// This is how Terraform behaved with "legacy" graphs (TF <= 0.7.x).
+	// We mimic that behavior here now and can improve upon it in the future.
+	//
+	// This behavior is tested in graph_build_apply_test.go to test ordering.
+	cbd := n.Config != nil && n.Config.Lifecycle.CreateBeforeDestroy
+	if !cbd {
+		// The "apply" side of a resource always depends on the destruction
+		// of all its dependencies in addition to the creation.
+		for _, v := range result {
+			result = append(result, v+".destroy")
+		}
+	}
+
+	return result
+}
+
 // GraphNodeEvalable
 func (n *NodeApplyableResource) EvalTree() EvalNode {
 	addr := n.NodeAbstractResource.Addr

terraform/test-fixtures/apply-multi-var-count-dec/main.tf

@@ -0,0 +1,10 @@
+variable "count" {}
+
+resource "aws_instance" "foo" {
+    count = "${var.count}"
+    value = "foo"
+}
+
+resource "aws_instance" "bar" {
+    value = "${join(",", aws_instance.foo.*.id)}"
+}

terraform/test-fixtures/graph-builder-apply-count/main.tf

@@ -0,0 +1,7 @@
+resource "aws_instance" "A" {
+  count = 1
+}
+
+resource "aws_instance" "B" {
+  value = ["${aws_instance.A.*.id}"]
+}

terraform/test-fixtures/graph-builder-apply-double-cbd/main.tf

@@ -0,0 +1,9 @@
+resource "aws_instance" "A" {
+  lifecycle { create_before_destroy = true }
+}
+
+resource "aws_instance" "B" {
+  value = ["${aws_instance.A.*.id}"]
+
+  lifecycle { create_before_destroy = true }
+}

terraform/transform_destroy_edge_test.go

@@ -98,6 +98,7 @@ func TestDestroyEdgeTransformer_module(t *testing.T) {
 
 type graphNodeCreatorTest struct {
 	AddrString string
+	Refs       []string
 }
 
 func (n *graphNodeCreatorTest) Name() string { return n.CreateAddr().String() }
@@ -110,6 +111,8 @@ func (n *graphNodeCreatorTest) CreateAddr() *ResourceAddress {
 	return addr
 }
 
+func (n *graphNodeCreatorTest) References() []string { return n.Refs }
+
 type graphNodeDestroyerTest struct {
 	AddrString string
 	CBD        bool