From 6ee3a08799dc4d13174bde099797122d57d06e33 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Mon, 28 Nov 2016 13:35:29 -0800 Subject: [PATCH 1/4] terraform: deposed shows up in plan, tests --- terraform/context_plan_test.go | 54 ++++++++++++++++++++++++ terraform/diff.go | 28 +++++++++--- terraform/eval_diff_deposed.go | 37 ++++++++++++++++ terraform/node_resource_plan_instance.go | 4 ++ terraform/test-fixtures/plan-cbd/main.tf | 3 ++ 5 files changed, 121 insertions(+), 5 deletions(-) create mode 100644 terraform/eval_diff_deposed.go create mode 100644 terraform/test-fixtures/plan-cbd/main.tf diff --git a/terraform/context_plan_test.go b/terraform/context_plan_test.go index 9c92b2f43e..aafb74b076 100644 --- a/terraform/context_plan_test.go +++ b/terraform/context_plan_test.go @@ -38,6 +38,60 @@ func TestContext2Plan_basic(t *testing.T) { } } +func TestContext2Plan_createBefore_deposed(t *testing.T) { + m := testModule(t, "plan-cbd") + p := testProvider("aws") + p.DiffFn = testDiffFn + + s := &State{ + Modules: []*ModuleState{ + &ModuleState{ + Path: []string{"root"}, + Resources: map[string]*ResourceState{ + "aws_instance.foo": &ResourceState{ + Type: "aws_instance", + Primary: &InstanceState{ + ID: "baz", + }, + Deposed: []*InstanceState{ + &InstanceState{ID: "foo"}, + }, + }, + }, + }, + }, + } + + ctx := testContext2(t, &ContextOpts{ + Module: m, + Providers: map[string]ResourceProviderFactory{ + "aws": testProviderFuncFixed(p), + }, + State: s, + }) + + plan, err := ctx.Plan() + if err != nil { + t.Fatalf("err: %s", err) + } + + actual := strings.TrimSpace(plan.String()) + expected := strings.TrimSpace(` +DIFF: + +DESTROY: aws_instance.foo (deposed only) + +STATE: + +aws_instance.foo: (1 deposed) + ID = baz + Deposed ID 1 = foo + `) + if actual != expected { + t.Fatalf("expected:\n%s, got:\n%s", expected, actual) + } +} + func TestContext2Plan_createBefore_maintainRoot(t *testing.T) { m := testModule(t, "plan-cbd-maintain-root") p := testProvider("aws") diff --git a/terraform/diff.go b/terraform/diff.go index 002f729215..75fd1778af 100644 --- a/terraform/diff.go +++ b/terraform/diff.go @@ -291,16 +291,22 @@ func (d *ModuleDiff) String() string { switch { case rdiff.RequiresNew() && (rdiff.GetDestroy() || rdiff.GetDestroyTainted()): crud = "DESTROY/CREATE" - case rdiff.GetDestroy(): + case rdiff.GetDestroy() || rdiff.GetDestroyDeposed(): crud = "DESTROY" case rdiff.RequiresNew(): crud = "CREATE" } + extra := "" + if !rdiff.GetDestroy() && rdiff.GetDestroyDeposed() { + extra = " (deposed only)" + } + buf.WriteString(fmt.Sprintf( - "%s: %s\n", + "%s: %s%s\n", crud, - name)) + name, + extra)) keyLen := 0 rdiffAttrs := rdiff.CopyAttributes() @@ -356,6 +362,7 @@ type InstanceDiff struct { mu sync.Mutex Attributes map[string]*ResourceAttrDiff Destroy bool + DestroyDeposed bool DestroyTainted bool } @@ -430,7 +437,7 @@ func (d *InstanceDiff) ChangeType() DiffChangeType { return DiffDestroyCreate } - if d.GetDestroy() { + if d.GetDestroy() || d.GetDestroyDeposed() { return DiffDestroy } @@ -449,7 +456,10 @@ func (d *InstanceDiff) Empty() bool { d.mu.Lock() defer d.mu.Unlock() - return !d.Destroy && !d.DestroyTainted && len(d.Attributes) == 0 + return !d.Destroy && + !d.DestroyTainted && + !d.DestroyDeposed && + len(d.Attributes) == 0 } // Equal compares two diffs for exact equality. @@ -482,6 +492,7 @@ func (d *InstanceDiff) GoString() string { Attributes: d.Attributes, Destroy: d.Destroy, DestroyTainted: d.DestroyTainted, + DestroyDeposed: d.DestroyDeposed, }) } @@ -516,6 +527,13 @@ func (d *InstanceDiff) requiresNew() bool { return false } +func (d *InstanceDiff) GetDestroyDeposed() bool { + d.mu.Lock() + defer d.mu.Unlock() + + return d.DestroyDeposed +} + // These methods are properly locked, for use outside other InstanceDiff // methods but everywhere else within in the terraform package. // TODO refactor the locking scheme diff --git a/terraform/eval_diff_deposed.go b/terraform/eval_diff_deposed.go new file mode 100644 index 0000000000..c873690905 --- /dev/null +++ b/terraform/eval_diff_deposed.go @@ -0,0 +1,37 @@ +package terraform + +// EvalDiffDeposed is an EvalNode implementation that marks DestroyDeposed +// in a diff if a resource has deposed instances that need destruction. +type EvalDiffDeposed struct { + Name string + Diff **InstanceDiff +} + +// TODO: test +func (n *EvalDiffDeposed) Eval(ctx EvalContext) (interface{}, error) { + // Check if there are any deposed items in the state + deposed := false + _, err := readInstanceFromState(ctx, n.Name, nil, func(rs *ResourceState) (*InstanceState, error) { + if len(rs.Deposed) > 0 { + deposed = true + } + + return nil, nil + }) + if err != nil { + return nil, err + } + + // If no deposed items, just return + if !deposed { + return nil, nil + } + + // Set the flag to true + if *n.Diff == nil { + *n.Diff = new(InstanceDiff) + } + (*n.Diff).DestroyDeposed = true + + return nil, nil +} diff --git a/terraform/node_resource_plan_instance.go b/terraform/node_resource_plan_instance.go index 9dafd22bad..b220e69e7d 100644 --- a/terraform/node_resource_plan_instance.go +++ b/terraform/node_resource_plan_instance.go @@ -177,6 +177,10 @@ func (n *NodePlannableResourceInstance) evalTreeManagedResource( OutputDiff: &diff, OutputState: &state, }, + &EvalDiffDeposed{ + Name: stateId, + Diff: &diff, + }, &EvalCheckPreventDestroy{ Resource: n.Config, Diff: &diff, diff --git a/terraform/test-fixtures/plan-cbd/main.tf b/terraform/test-fixtures/plan-cbd/main.tf new file mode 100644 index 0000000000..6edd78aef6 --- /dev/null +++ b/terraform/test-fixtures/plan-cbd/main.tf @@ -0,0 +1,3 @@ +resource "aws_instance" "foo" { + lifecycle { create_before_destroy = true } +} From 41c56ad002cee526e9fd77946720e18c77eb5d5a Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Mon, 28 Nov 2016 13:41:22 -0800 Subject: [PATCH 2/4] terraform: new apply graph understands destroying deposed only --- terraform/context_apply_test.go | 55 +++++++++++++++++++ .../apply-cbd-deposed-only/main.tf | 3 + .../apply-good-create-before-count/main.tf | 7 +++ terraform/transform_diff.go | 2 +- test/foo/main.tf | 9 +++ test/index.html | 1 + test/main.tf | 13 +++++ test/real.tfstate | 45 +++++++++++++++ 8 files changed, 134 insertions(+), 1 deletion(-) create mode 100644 terraform/test-fixtures/apply-cbd-deposed-only/main.tf create mode 100644 terraform/test-fixtures/apply-good-create-before-count/main.tf create mode 100644 test/foo/main.tf create mode 100644 test/index.html create mode 100644 test/main.tf create mode 100644 test/real.tfstate diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index f933301bca..27f89da187 100644 --- a/terraform/context_apply_test.go +++ b/terraform/context_apply_test.go @@ -1014,6 +1014,61 @@ aws_instance.bar.1: `) } +// Test that when we have a deposed instance but a good primary, we still +// destroy the deposed instance. +func TestContext2Apply_createBeforeDestroy_deposedOnly(t *testing.T) { + m := testModule(t, "apply-cbd-deposed-only") + p := testProvider("aws") + p.ApplyFn = testApplyFn + p.DiffFn = testDiffFn + + state := &State{ + Modules: []*ModuleState{ + &ModuleState{ + Path: rootModulePath, + Resources: map[string]*ResourceState{ + "aws_instance.bar": &ResourceState{ + Type: "aws_instance", + Primary: &InstanceState{ + ID: "bar", + }, + + Deposed: []*InstanceState{ + &InstanceState{ + ID: "foo", + }, + }, + }, + }, + }, + }, + } + + ctx := testContext2(t, &ContextOpts{ + Module: m, + Providers: map[string]ResourceProviderFactory{ + "aws": testProviderFuncFixed(p), + }, + State: state, + }) + + if p, err := ctx.Plan(); err != nil { + t.Fatalf("err: %s", err) + } else { + t.Logf(p.String()) + } + + state, err := ctx.Apply() + if err != nil { + t.Fatalf("err: %s", err) + } + + checkStateString(t, state, ` +aws_instance.bar: + ID = bar + `) +} + func TestContext2Apply_destroyComputed(t *testing.T) { m := testModule(t, "apply-destroy-computed") p := testProvider("aws") diff --git a/terraform/test-fixtures/apply-cbd-deposed-only/main.tf b/terraform/test-fixtures/apply-cbd-deposed-only/main.tf new file mode 100644 index 0000000000..4e5f481c54 --- /dev/null +++ b/terraform/test-fixtures/apply-cbd-deposed-only/main.tf @@ -0,0 +1,3 @@ +resource "aws_instance" "bar" { + lifecycle { create_before_destroy = true } +} diff --git a/terraform/test-fixtures/apply-good-create-before-count/main.tf b/terraform/test-fixtures/apply-good-create-before-count/main.tf new file mode 100644 index 0000000000..324ad52857 --- /dev/null +++ b/terraform/test-fixtures/apply-good-create-before-count/main.tf @@ -0,0 +1,7 @@ +resource "aws_instance" "bar" { + count = 2 + require_new = "xyz" + lifecycle { + create_before_destroy = true + } +} diff --git a/terraform/transform_diff.go b/terraform/transform_diff.go index 68e9053461..ad46d3c612 100644 --- a/terraform/transform_diff.go +++ b/terraform/transform_diff.go @@ -58,7 +58,7 @@ func (t *DiffTransformer) Transform(g *Graph) error { addr.Path = m.Path[1:] // If we're destroying, add the destroy node - if inst.Destroy { + if inst.Destroy || inst.GetDestroyDeposed() { abstract := &NodeAbstractResource{Addr: addr} g.Add(&NodeDestroyResource{NodeAbstractResource: abstract}) } diff --git a/test/foo/main.tf b/test/foo/main.tf new file mode 100644 index 0000000000..ce7a57d4ca --- /dev/null +++ b/test/foo/main.tf @@ -0,0 +1,9 @@ +resource "null_resource" "foo" { + count = 2 + + provisioner "local-exec" { command = "sleep ${count.index*3}" } + + //provisioner "local-exec" { command = "exit 1" } + + lifecycle { create_before_destroy = true } +} diff --git a/test/index.html b/test/index.html new file mode 100644 index 0000000000..3fa0d4b982 --- /dev/null +++ b/test/index.html @@ -0,0 +1 @@ +Hello, World diff --git a/test/main.tf b/test/main.tf new file mode 100644 index 0000000000..09f29ef134 --- /dev/null +++ b/test/main.tf @@ -0,0 +1,13 @@ +variable "username" { + default = "bob" +} + +data "template_file" "user" { + template = "$${USERNAME}" + vars { + USERNAME = "${var.username}" + } + provisioner "local-exec" { + command = "echo ${self.rendered} > user.txt" + } +} diff --git a/test/real.tfstate b/test/real.tfstate new file mode 100644 index 0000000000..807ef69637 --- /dev/null +++ b/test/real.tfstate @@ -0,0 +1,45 @@ +{ + "version": 3, + "terraform_version": "0.7.2", + "serial": 393, + "lineage": "e8e8cc31-ebe6-4260-bb6a-eed53258cc08", + "modules": [ + { + "path": [ + "root" + ], + "outputs": {}, + "resources": { + "aws_route53_record.rslc-xxx-test-route53-record": { + "type": "aws_route53_record", + "depends_on": [ + ], + "primary": { + "id": "ZKBENUPLDUDMJ_xxx-test.rslcare.com.au_A", + "attributes": { + "alias.#": "1", + "alias.2624757427.evaluate_target_health": "false", + "alias.2624757427.name": "awseb-e-3-AWSEBLoa-1OO1X7V7IRMMF-999999999.ap-southeast-2.elb.amazonaws.com.", + "alias.2624757427.zone_id": "Z2999QAZ9SRTIC", + "fqdn": "hws-test.rslcare.com.au", + "health_check_id": "", + "id": "ZKBENUPLDUDMJ_hws-test.rslcare.com.au_A", + "name": "xxx-test.example.com", + "records.#": "0", + "set_identifier": "", + "ttl": "0", + "type": "A", + "zone_id": "ZKBENUPLDUDMJ" + }, + "meta": { + "schema_version": "2" + }, + "tainted": false + }, + "deposed": [], + "provider": "" + } + } + } + ] +} From 8701434b4d5fa042e8924f5aecfff416a4837c74 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Mon, 28 Nov 2016 13:49:10 -0800 Subject: [PATCH 3/4] command: plan formatting for deposed destroy --- command/format_plan.go | 13 ++++++++++--- command/format_plan_test.go | 35 +++++++++++++++++++++++++++++++++++ 2 files changed, 45 insertions(+), 3 deletions(-) diff --git a/command/format_plan.go b/command/format_plan.go index e0020e648a..98a07f63c7 100644 --- a/command/format_plan.go +++ b/command/format_plan.go @@ -114,14 +114,21 @@ func formatPlanModuleExpand( symbol = "-" } - taintStr := "" + var extraAttr []string if rdiff.DestroyTainted { - taintStr = " (tainted)" + extraAttr = append(extraAttr, "tainted") + } + if rdiff.DestroyDeposed { + extraAttr = append(extraAttr, "deposed") + } + var extraStr string + if len(extraAttr) > 0 { + extraStr = fmt.Sprintf(" (%s)", strings.Join(extraAttr, ", ")) } buf.WriteString(opts.Color.Color(fmt.Sprintf( "[%s]%s %s%s\n", - color, symbol, name, taintStr))) + color, symbol, name, extraStr))) // Get all the attributes that are changing, and sort them. Also // determine the longest key so that we can align them all. diff --git a/command/format_plan_test.go b/command/format_plan_test.go index a79c873eb4..3784f18c5f 100644 --- a/command/format_plan_test.go +++ b/command/format_plan_test.go @@ -8,6 +8,41 @@ import ( "github.com/mitchellh/colorstring" ) +// Test that a root level data source gets a special plan output on create +func TestFormatPlan_destroyDeposed(t *testing.T) { + plan := &terraform.Plan{ + Diff: &terraform.Diff{ + Modules: []*terraform.ModuleDiff{ + &terraform.ModuleDiff{ + Path: []string{"root"}, + Resources: map[string]*terraform.InstanceDiff{ + "aws_instance.foo": &terraform.InstanceDiff{ + DestroyDeposed: true, + }, + }, + }, + }, + }, + } + opts := &FormatPlanOpts{ + Plan: plan, + Color: &colorstring.Colorize{ + Colors: colorstring.DefaultColors, + Disable: true, + }, + ModuleDepth: 1, + } + + actual := FormatPlan(opts) + + expected := strings.TrimSpace(` +- aws_instance.foo (deposed) + `) + if actual != expected { + t.Fatalf("expected:\n\n%s\n\ngot:\n\n%s", expected, actual) + } +} + // Test that a root level data source gets a special plan output on create func TestFormatPlan_rootDataSource(t *testing.T) { plan := &terraform.Plan{ From 80457b689c9500eb0936137190868097ca5ae5ef Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Mon, 28 Nov 2016 13:58:12 -0800 Subject: [PATCH 4/4] terraform: do the deposed check within EvalDiff There is never any reason to separate the two. --- command/hook_count_test.go | 25 ++++++++++++++++ terraform/diff.go | 7 +++++ terraform/eval_diff.go | 13 +++++++++ terraform/eval_diff_deposed.go | 37 ------------------------ terraform/node_resource_plan_instance.go | 5 +--- 5 files changed, 46 insertions(+), 41 deletions(-) delete mode 100644 terraform/eval_diff_deposed.go diff --git a/command/hook_count_test.go b/command/hook_count_test.go index 5f0b000e88..457460637f 100644 --- a/command/hook_count_test.go +++ b/command/hook_count_test.go @@ -11,6 +11,31 @@ func TestCountHook_impl(t *testing.T) { var _ terraform.Hook = new(CountHook) } +func TestCountHookPostDiff_DestroyDeposed(t *testing.T) { + h := new(CountHook) + + resources := map[string]*terraform.InstanceDiff{ + "lorem": &terraform.InstanceDiff{DestroyDeposed: true}, + } + + n := &terraform.InstanceInfo{} // TODO + + for _, d := range resources { + h.PostDiff(n, d) + } + + expected := new(CountHook) + expected.ToAdd = 0 + expected.ToChange = 0 + expected.ToRemoveAndAdd = 0 + expected.ToRemove = 1 + + if !reflect.DeepEqual(expected, h) { + t.Fatalf("Expected %#v, got %#v instead.", + expected, h) + } +} + func TestCountHookPostDiff_DestroyOnly(t *testing.T) { h := new(CountHook) diff --git a/terraform/diff.go b/terraform/diff.go index 75fd1778af..c50d3cedb3 100644 --- a/terraform/diff.go +++ b/terraform/diff.go @@ -534,6 +534,13 @@ func (d *InstanceDiff) GetDestroyDeposed() bool { return d.DestroyDeposed } +func (d *InstanceDiff) SetDestroyDeposed(b bool) { + d.mu.Lock() + defer d.mu.Unlock() + + d.DestroyDeposed = b +} + // These methods are properly locked, for use outside other InstanceDiff // methods but everywhere else within in the terraform package. // TODO refactor the locking scheme diff --git a/terraform/eval_diff.go b/terraform/eval_diff.go index 7152ea6f34..717d951053 100644 --- a/terraform/eval_diff.go +++ b/terraform/eval_diff.go @@ -69,6 +69,7 @@ func (n *EvalCompareDiff) Eval(ctx EvalContext) (interface{}, error) { // EvalDiff is an EvalNode implementation that does a refresh for // a resource. type EvalDiff struct { + Name string Info *InstanceInfo Config **ResourceConfig Provider *ResourceProvider @@ -112,6 +113,18 @@ func (n *EvalDiff) Eval(ctx EvalContext) (interface{}, error) { diff = new(InstanceDiff) } + // Set DestroyDeposed if we have deposed instances + _, err = readInstanceFromState(ctx, n.Name, nil, func(rs *ResourceState) (*InstanceState, error) { + if len(rs.Deposed) > 0 { + diff.DestroyDeposed = true + } + + return nil, nil + }) + if err != nil { + return nil, err + } + // Preserve the DestroyTainted flag if n.Diff != nil { diff.SetTainted((*n.Diff).GetDestroyTainted()) diff --git a/terraform/eval_diff_deposed.go b/terraform/eval_diff_deposed.go deleted file mode 100644 index c873690905..0000000000 --- a/terraform/eval_diff_deposed.go +++ /dev/null @@ -1,37 +0,0 @@ -package terraform - -// EvalDiffDeposed is an EvalNode implementation that marks DestroyDeposed -// in a diff if a resource has deposed instances that need destruction. -type EvalDiffDeposed struct { - Name string - Diff **InstanceDiff -} - -// TODO: test -func (n *EvalDiffDeposed) Eval(ctx EvalContext) (interface{}, error) { - // Check if there are any deposed items in the state - deposed := false - _, err := readInstanceFromState(ctx, n.Name, nil, func(rs *ResourceState) (*InstanceState, error) { - if len(rs.Deposed) > 0 { - deposed = true - } - - return nil, nil - }) - if err != nil { - return nil, err - } - - // If no deposed items, just return - if !deposed { - return nil, nil - } - - // Set the flag to true - if *n.Diff == nil { - *n.Diff = new(InstanceDiff) - } - (*n.Diff).DestroyDeposed = true - - return nil, nil -} diff --git a/terraform/node_resource_plan_instance.go b/terraform/node_resource_plan_instance.go index b220e69e7d..aeb57bbd1a 100644 --- a/terraform/node_resource_plan_instance.go +++ b/terraform/node_resource_plan_instance.go @@ -169,6 +169,7 @@ func (n *NodePlannableResourceInstance) evalTreeManagedResource( Output: &state, }, &EvalDiff{ + Name: stateId, Info: info, Config: &resourceConfig, Resource: n.Config, @@ -177,10 +178,6 @@ func (n *NodePlannableResourceInstance) evalTreeManagedResource( OutputDiff: &diff, OutputState: &state, }, - &EvalDiffDeposed{ - Name: stateId, - Diff: &diff, - }, &EvalCheckPreventDestroy{ Resource: n.Config, Diff: &diff,