From 2389251c3849e4cf55409448759540ea927d51fd Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Sun, 1 Mar 2015 21:28:41 -0800 Subject: [PATCH 1/2] terraform: catch scenario where both "foo" and "foo.0" are in state --- terraform/context_test.go | 65 +++++++++++++++++++ terraform/eval_count.go | 14 +++- terraform/graph_config_node.go | 8 +++ terraform/terraform_test.go | 33 ++++++++++ .../test-fixtures/apply-count-dec-one/main.tf | 4 -- 5 files changed, 117 insertions(+), 7 deletions(-) diff --git a/terraform/context_test.go b/terraform/context_test.go index 63eb9c283a7d..06de7312d707 100644 --- a/terraform/context_test.go +++ b/terraform/context_test.go @@ -3097,6 +3097,7 @@ func TestContext2Apply_countDecrease(t *testing.T) { func TestContext2Apply_countDecreaseToOne(t *testing.T) { m := testModule(t, "apply-count-dec-one") p := testProvider("aws") + p.ApplyFn = testApplyFn p.DiffFn = testDiffFn s := &State{ Modules: []*ModuleState{ @@ -3153,6 +3154,70 @@ func TestContext2Apply_countDecreaseToOne(t *testing.T) { } } +// https://github.com/PeoplePerHour/terraform/pull/11 +// +// This tests a case where both a "resource" and "resource.0" are in +// the state file, which apparently is a reasonable backwards compatibility +// concern found in the above 3rd party repo. +func TestContext2Apply_countDecreaseToOneCorrupted(t *testing.T) { + m := testModule(t, "apply-count-dec-one") + p := testProvider("aws") + p.ApplyFn = testApplyFn + p.DiffFn = testDiffFn + s := &State{ + Modules: []*ModuleState{ + &ModuleState{ + Path: rootModulePath, + Resources: map[string]*ResourceState{ + "aws_instance.foo": &ResourceState{ + Type: "aws_instance", + Primary: &InstanceState{ + ID: "bar", + Attributes: map[string]string{ + "foo": "foo", + "type": "aws_instance", + }, + }, + }, + "aws_instance.foo.0": &ResourceState{ + Type: "aws_instance", + Primary: &InstanceState{ + ID: "baz", + Attributes: map[string]string{ + "type": "aws_instance", + }, + }, + }, + }, + }, + }, + } + ctx := testContext2(t, &ContextOpts{ + Module: m, + Providers: map[string]ResourceProviderFactory{ + "aws": testProviderFuncFixed(p), + }, + State: s, + }) + + if p, err := ctx.Plan(nil); err != nil { + t.Fatalf("err: %s", err) + } else { + testStringMatch(t, p, testTerraformApplyCountDecToOneCorruptedPlanStr) + } + + state, err := ctx.Apply() + if err != nil { + t.Fatalf("err: %s", err) + } + + actual := strings.TrimSpace(state.String()) + expected := strings.TrimSpace(testTerraformApplyCountDecToOneCorruptedStr) + if actual != expected { + t.Fatalf("bad: \n%s", actual) + } +} + func TestContext2Apply_countTainted(t *testing.T) { m := testModule(t, "apply-count-tainted") p := testProvider("aws") diff --git a/terraform/eval_count.go b/terraform/eval_count.go index f7886b8da95c..2ae56a751cfd 100644 --- a/terraform/eval_count.go +++ b/terraform/eval_count.go @@ -41,10 +41,18 @@ func (n *EvalCountFixZeroOneBoundary) Eval(ctx EvalContext) (interface{}, error) } // Look for the resource state. If we don't have one, then it is okay. - if rs, ok := mod.Resources[hunt]; ok { - mod.Resources[replace] = rs - delete(mod.Resources, hunt) + rs, ok := mod.Resources[hunt] + if !ok { + return nil, nil } + // If the replacement key exists, we just keep both + if _, ok := mod.Resources[replace]; ok { + return nil, nil + } + + mod.Resources[replace] = rs + delete(mod.Resources, hunt) + return nil, nil } diff --git a/terraform/graph_config_node.go b/terraform/graph_config_node.go index baa862b2daf3..07e53ef09c9c 100644 --- a/terraform/graph_config_node.go +++ b/terraform/graph_config_node.go @@ -481,6 +481,14 @@ func (n *graphNodeResourceDestroy) DestroyInclude(d *ModuleDiff, s *ModuleState) return true } } + + // If we're in the state as _both_ "foo" and "foo.0", then + // keep it, since we treat the latter as an orphan. + _, okOne := s.Resources[prefix] + _, okTwo := s.Resources[prefix+".0"] + if okOne && okTwo { + return true + } } return false diff --git a/terraform/terraform_test.go b/terraform/terraform_test.go index d063555bfbd4..815a1dd98d16 100644 --- a/terraform/terraform_test.go +++ b/terraform/terraform_test.go @@ -5,9 +5,11 @@ import ( "crypto/sha1" "encoding/gob" "encoding/hex" + "fmt" "io/ioutil" "os" "path/filepath" + "strings" "sync" "testing" @@ -68,6 +70,14 @@ func testModule(t *testing.T, name string) *module.Tree { return mod } +func testStringMatch(t *testing.T, s fmt.Stringer, expected string) { + actual := strings.TrimSpace(s.String()) + expected = strings.TrimSpace(expected) + if actual != expected { + t.Fatalf("Actual\n\n%s\n\nExpected:\n\n%s", actual, expected) + } +} + func testProviderFuncFixed(rp ResourceProvider) ResourceProviderFactory { return func() (ResourceProvider, error) { return rp, nil @@ -246,6 +256,29 @@ aws_instance.foo: type = aws_instance ` +const testTerraformApplyCountDecToOneCorruptedStr = ` +aws_instance.foo: + ID = bar + foo = foo + type = aws_instance +` + +const testTerraformApplyCountDecToOneCorruptedPlanStr = ` +DIFF: + +DESTROY: aws_instance.foo.0 + +STATE: + +aws_instance.foo: + ID = bar + foo = foo + type = aws_instance +aws_instance.foo.0: + ID = baz + type = aws_instance +` + const testTerraformApplyCountTaintedStr = ` ` diff --git a/terraform/test-fixtures/apply-count-dec-one/main.tf b/terraform/test-fixtures/apply-count-dec-one/main.tf index 7837f58655f7..3b0fd9428595 100644 --- a/terraform/test-fixtures/apply-count-dec-one/main.tf +++ b/terraform/test-fixtures/apply-count-dec-one/main.tf @@ -1,7 +1,3 @@ resource "aws_instance" "foo" { foo = "foo" } - -resource "aws_instance" "bar" { - foo = "bar" -} From 11d073f7d489ba1b7c0c2cede45a43b1e9d57231 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Sun, 1 Mar 2015 21:39:48 -0800 Subject: [PATCH 2/2] terraform: test the increase from one case --- terraform/context_test.go | 58 +++++++++++++++++++++++++++++++++++++ terraform/terraform_test.go | 26 +++++++++++++++++ 2 files changed, 84 insertions(+) diff --git a/terraform/context_test.go b/terraform/context_test.go index 06de7312d707..aaeda915939e 100644 --- a/terraform/context_test.go +++ b/terraform/context_test.go @@ -829,6 +829,64 @@ func TestContext2Plan_countIncreaseFromOne(t *testing.T) { } } +// https://github.com/PeoplePerHour/terraform/pull/11 +// +// This tests a case where both a "resource" and "resource.0" are in +// the state file, which apparently is a reasonable backwards compatibility +// concern found in the above 3rd party repo. +func TestContext2Plan_countIncreaseFromOneCorrupted(t *testing.T) { + m := testModule(t, "plan-count-inc") + p := testProvider("aws") + p.DiffFn = testDiffFn + s := &State{ + Modules: []*ModuleState{ + &ModuleState{ + Path: rootModulePath, + Resources: map[string]*ResourceState{ + "aws_instance.foo": &ResourceState{ + Type: "aws_instance", + Primary: &InstanceState{ + ID: "bar", + Attributes: map[string]string{ + "foo": "foo", + "type": "aws_instance", + }, + }, + }, + "aws_instance.foo.0": &ResourceState{ + Type: "aws_instance", + Primary: &InstanceState{ + ID: "bar", + Attributes: map[string]string{ + "foo": "foo", + "type": "aws_instance", + }, + }, + }, + }, + }, + }, + } + ctx := testContext2(t, &ContextOpts{ + Module: m, + Providers: map[string]ResourceProviderFactory{ + "aws": testProviderFuncFixed(p), + }, + State: s, + }) + + plan, err := ctx.Plan(nil) + if err != nil { + t.Fatalf("err: %s", err) + } + + actual := strings.TrimSpace(plan.String()) + expected := strings.TrimSpace(testTerraformPlanCountIncreaseFromOneCorruptedStr) + if actual != expected { + t.Fatalf("bad:\n%s", actual) + } +} + func TestContext2Plan_destroy(t *testing.T) { m := testModule(t, "plan-destroy") p := testProvider("aws") diff --git a/terraform/terraform_test.go b/terraform/terraform_test.go index 815a1dd98d16..4070475c09ce 100644 --- a/terraform/terraform_test.go +++ b/terraform/terraform_test.go @@ -829,6 +829,32 @@ aws_instance.foo.0: type = aws_instance ` +const testTerraformPlanCountIncreaseFromOneCorruptedStr = ` +DIFF: + +CREATE: aws_instance.bar + foo: "" => "bar" + type: "" => "aws_instance" +DESTROY: aws_instance.foo +CREATE: aws_instance.foo.1 + foo: "" => "foo" + type: "" => "aws_instance" +CREATE: aws_instance.foo.2 + foo: "" => "foo" + type: "" => "aws_instance" + +STATE: + +aws_instance.foo: + ID = bar + foo = foo + type = aws_instance +aws_instance.foo.0: + ID = bar + foo = foo + type = aws_instance +` + const testTerraformPlanDestroyStr = ` DIFF: