diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index 1601a0acf..6458b82d7 100644 --- a/terraform/context_apply_test.go +++ b/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") diff --git a/terraform/graph_builder_apply_test.go b/terraform/graph_builder_apply_test.go index 28599602e..57307a8f3 100644 --- a/terraform/graph_builder_apply_test.go +++ b/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 +` diff --git a/terraform/node_resource_apply.go b/terraform/node_resource_apply.go index 811aa55eb..a16683642 100644 --- a/terraform/node_resource_apply.go +++ b/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 diff --git a/terraform/test-fixtures/apply-multi-var-count-dec/main.tf b/terraform/test-fixtures/apply-multi-var-count-dec/main.tf new file mode 100644 index 000000000..ab820b5cf --- /dev/null +++ b/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)}" +} diff --git a/terraform/test-fixtures/graph-builder-apply-count/main.tf b/terraform/test-fixtures/graph-builder-apply-count/main.tf new file mode 100644 index 000000000..982021bf9 --- /dev/null +++ b/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}"] +} diff --git a/terraform/test-fixtures/graph-builder-apply-double-cbd/main.tf b/terraform/test-fixtures/graph-builder-apply-double-cbd/main.tf new file mode 100644 index 000000000..bc121744d --- /dev/null +++ b/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 } +} diff --git a/terraform/transform_destroy_edge_test.go b/terraform/transform_destroy_edge_test.go index cf8e4c704..1f7b1a5d6 100644 --- a/terraform/transform_destroy_edge_test.go +++ b/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