Merge pull request #1086 from hashicorp/b-pph

terraform: catch scenario where both "foo" and "foo.0" are in state

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")
@@ -3097,6 +3155,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 +3212,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")

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
 }

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

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 = `
 <no state>
 `
@@ -796,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:
 

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"
-}