From b45b6a5c20de0fa9b3363e0b9f99dc8fddce6f1d Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 7 Apr 2017 21:12:21 -0400 Subject: [PATCH 1/3] remove duplicates in Dependencies duplicate entries could end up in "depends_on" in the state, which could possible lead to erroneous state comparisons. Remove them when walking the graph, and remove existing duplicates when pruning the state. --- terraform/node_resource_abstract.go | 2 +- terraform/state.go | 5 ++++- terraform/util.go | 18 ++++++++++++++++++ 3 files changed, 23 insertions(+), 2 deletions(-) diff --git a/terraform/node_resource_abstract.go b/terraform/node_resource_abstract.go index e4577e9db..50bb70792 100644 --- a/terraform/node_resource_abstract.go +++ b/terraform/node_resource_abstract.go @@ -102,7 +102,7 @@ func (n *NodeAbstractResource) References() []string { } } - return result + return uniqueStrings(result) } // If we have state, that is our next source diff --git a/terraform/state.go b/terraform/state.go index 905ec3dcb..78725e88d 100644 --- a/terraform/state.go +++ b/terraform/state.go @@ -1163,6 +1163,8 @@ func (m *ModuleState) prune() { delete(m.Outputs, k) } } + + m.Dependencies = uniqueStrings(m.Dependencies) } func (m *ModuleState) sort() { @@ -1526,8 +1528,9 @@ func (s *ResourceState) prune() { i-- } } - s.Deposed = s.Deposed[:n] + + s.Dependencies = uniqueStrings(s.Dependencies) } func (s *ResourceState) sort() { diff --git a/terraform/util.go b/terraform/util.go index e1d951c01..f41f0d7d6 100644 --- a/terraform/util.go +++ b/terraform/util.go @@ -1,6 +1,7 @@ package terraform import ( + "sort" "strings" ) @@ -73,3 +74,20 @@ func strSliceContains(haystack []string, needle string) bool { } return false } + +// deduplicate a slice of strings +func uniqueStrings(s []string) []string { + if len(s) < 2 { + return s + } + + sort.Strings(s) + result := make([]string, 1, len(s)) + result[0] = s[0] + for i := 1; i < len(s); i++ { + if s[i] != result[len(result)-1] { + result = append(result, s[i]) + } + } + return result +} From bd983f6cba9b09d25ceaf12e35ea6a8807669d14 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Sat, 8 Apr 2017 14:24:00 -0400 Subject: [PATCH 2/3] don't forget to test the simple things --- terraform/util_test.go | 43 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/terraform/util_test.go b/terraform/util_test.go index 3c95e361a..9c5712cfa 100644 --- a/terraform/util_test.go +++ b/terraform/util_test.go @@ -1,6 +1,8 @@ package terraform import ( + "fmt" + "reflect" "testing" "time" ) @@ -96,3 +98,44 @@ func TestUtilResourceProvider(t *testing.T) { } } } + +func TestUniqueStrings(t *testing.T) { + cases := []struct { + Input []string + Expected []string + }{ + { + []string{}, + []string{}, + }, + { + []string{"x"}, + []string{"x"}, + }, + { + []string{"a", "b", "c"}, + []string{"a", "b", "c"}, + }, + { + []string{"a", "a", "a"}, + []string{"a"}, + }, + { + []string{"a", "b", "a", "b", "a", "a"}, + []string{"a", "b"}, + }, + { + []string{"c", "b", "a", "c", "b"}, + []string{"a", "b", "c"}, + }, + } + + for i, tc := range cases { + t.Run(fmt.Sprintf("unique-%d", i), func(t *testing.T) { + actual := uniqueStrings(tc.Input) + if !reflect.DeepEqual(tc.Expected, actual) { + t.Fatalf("Expected: %q\nGot: %q", tc.Expected, actual) + } + }) + } +} From 3f49227b729df73cdff7a13e5654f1d24c8373fe Mon Sep 17 00:00:00 2001 From: James Bardin Date: Sat, 8 Apr 2017 15:15:18 -0400 Subject: [PATCH 3/3] add state an context tests Make sure duplicate depends_on entries are pruned from existing states on read. Make sure new state built from configs with multiple references to the same resource only add it once to the Dependencies. --- terraform/context_apply_test.go | 29 ++++++++++ terraform/state_test.go | 56 +++++++++++++++++++ .../test-fixtures/apply-multi-ref/main.tf | 8 +++ 3 files changed, 93 insertions(+) create mode 100644 terraform/test-fixtures/apply-multi-ref/main.tf diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index afe2f85a7..7d5bbf025 100644 --- a/terraform/context_apply_test.go +++ b/terraform/context_apply_test.go @@ -8100,3 +8100,32 @@ func TestContext2Apply_terraformEnv(t *testing.T) { t.Fatalf("bad: \n%s", actual) } } + +// verify that multiple config references only create a single depends_on entry +func TestContext2Apply_multiRef(t *testing.T) { + m := testModule(t, "apply-multi-ref") + p := testProvider("aws") + p.ApplyFn = testApplyFn + p.DiffFn = testDiffFn + + ctx := testContext2(t, &ContextOpts{ + Module: m, + Providers: map[string]ResourceProviderFactory{ + "aws": testProviderFuncFixed(p), + }, + }) + + if _, err := ctx.Plan(); err != nil { + t.Fatalf("err: %s", err) + } + + state, err := ctx.Apply() + if err != nil { + t.Fatalf("err: %s", err) + } + + deps := state.Modules[0].Resources["aws_instance.other"].Dependencies + if len(deps) > 1 || deps[0] != "aws_instance.create" { + t.Fatalf("expected 1 depends_on entry for aws_instance.create, got %q", deps) + } +} diff --git a/terraform/state_test.go b/terraform/state_test.go index c10ebf133..324ab7970 100644 --- a/terraform/state_test.go +++ b/terraform/state_test.go @@ -1898,6 +1898,62 @@ func TestReadState_prune(t *testing.T) { } } +func TestReadState_pruneDependencies(t *testing.T) { + state := &State{ + Serial: 9, + Lineage: "5d1ad1a1-4027-4665-a908-dbe6adff11d8", + Remote: &RemoteState{ + Type: "http", + Config: map[string]string{ + "url": "http://my-cool-server.com/", + }, + }, + Modules: []*ModuleState{ + &ModuleState{ + Path: rootModulePath, + Dependencies: []string{ + "aws_instance.bar", + "aws_instance.bar", + }, + Resources: map[string]*ResourceState{ + "foo": &ResourceState{ + Dependencies: []string{ + "aws_instance.baz", + "aws_instance.baz", + }, + Primary: &InstanceState{ + ID: "bar", + }, + }, + }, + }, + }, + } + state.init() + + buf := new(bytes.Buffer) + if err := WriteState(state, buf); err != nil { + t.Fatalf("err: %s", err) + } + + actual, err := ReadState(buf) + if err != nil { + t.Fatalf("err: %s", err) + } + + // make sure the duplicate Dependencies are filtered + modDeps := actual.Modules[0].Dependencies + resourceDeps := actual.Modules[0].Resources["foo"].Dependencies + + if len(modDeps) > 1 || modDeps[0] != "aws_instance.bar" { + t.Fatalf("expected 1 module depends_on entry, got %q", modDeps) + } + + if len(resourceDeps) > 1 || resourceDeps[0] != "aws_instance.baz" { + t.Fatalf("expected 1 resource depends_on entry, got %q", resourceDeps) + } +} + func TestResourceNameSort(t *testing.T) { names := []string{ "a", diff --git a/terraform/test-fixtures/apply-multi-ref/main.tf b/terraform/test-fixtures/apply-multi-ref/main.tf new file mode 100644 index 000000000..2a6a67152 --- /dev/null +++ b/terraform/test-fixtures/apply-multi-ref/main.tf @@ -0,0 +1,8 @@ +resource "aws_instance" "create" { + bar = "abc" +} + +resource "aws_instance" "other" { + var = "${aws_instance.create.id}" + foo = "${aws_instance.create.bar}" +}