terraform: apply resource must depend on destroy deps

Fixes #10440

This updates the behavior of "apply" resources to depend on the
destroy versions of their dependencies.

We make an exception to this behavior when the "apply" resource is CBD.
This is odd and not 100% correct, but it mimics the behavior of the
legacy graphs and avoids us having to do major core work to support the
100% correct solution.

I'll explain this in examples...

Given the following configuration:

    resource "null_resource" "a" {
       count = "${var.count}"
    }

    resource "null_resource" "b" {
      triggers { key = "${join(",", null_resource.a.*.id)}" }
    }

Assume we've successfully created this configuration with count = 2.
When going from count = 2 to count = 1, `null_resource.b` should wait
for `null_resource.a.1` to destroy.

If it doesn't, then it is a race: depending when we interpolate the
`triggers.key` attribute of `null_resource.b`, we may get 1 value or 2.
If `null_resource.a.1` is destroyed, we'll get 1. Otherwise, we'll get
2. This was the root cause of #10440

In the legacy graphs, `null_resource.b` would depend on the destruction
of any `null_resource.a` (orphans, tainted, anything!). This would
ensure proper ordering. We mimic that behavior here.

The difference is CBD. If `null_resource.b` has CBD enabled, then the
ordering **in the legacy graph** becomes:

  1. null_resource.b (create)
  2. null_resource.b (destroy)
  3. null_resource.a (destroy)

In this case, the update would always have 2 values for `triggers.key`,
even though we were destroying a resource later! This scenario required
two `terraform apply` operations.

This is what the CBD check is for in this PR. We do this to mimic the
behavior of the legacy graph.

The correct solution to do one day is to allow splat references
(`null_resource.a.*.id`) to happen in parallel and only read up to to
the `count` amount in the state. This requires some fairly significant
work close to the 0.8 release date, so we can defer this to later and
adopt the 0.7.x behavior for now.

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