From fe4799bd68eb2169cf861b316f6a1d67c2acad7e Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 13 Oct 2016 11:16:03 -0400 Subject: [PATCH 1/3] Add failing test for nil IsntanceState in State A nil InstanceState within State/Modules/Resources/Deposed will panic during a deep copy. The panic needs to be fixed in copystructure, but the nil probably should have been normalized out before we got here too. --- terraform/state_test.go | 54 +++++++++++++++++++++++++++++++++-------- 1 file changed, 44 insertions(+), 10 deletions(-) diff --git a/terraform/state_test.go b/terraform/state_test.go index 569f5ef941c9..cae8751f0726 100644 --- a/terraform/state_test.go +++ b/terraform/state_test.go @@ -254,30 +254,64 @@ func TestStateModuleOrphans_deepNestedNilConfig(t *testing.T) { func TestStateDeepCopy(t *testing.T) { cases := []struct { - One, Two *State - F func(*State) interface{} + State *State }{ // Version { &State{Version: 5}, - &State{Version: 5}, - func(s *State) interface{} { return s.Version }, }, - // TFVersion { &State{TFVersion: "5"}, - &State{TFVersion: "5"}, - func(s *State) interface{} { return s.TFVersion }, + }, + // Modules + { + &State{ + Version: 6, + Modules: []*ModuleState{ + &ModuleState{ + Path: rootModulePath, + Resources: map[string]*ResourceState{ + "test_instance.foo": &ResourceState{ + Primary: &InstanceState{ + Meta: map[string]string{}, + }, + }, + }, + }, + }, + }, + }, + // Deposed + { + &State{ + Version: 6, + Modules: []*ModuleState{ + &ModuleState{ + Path: rootModulePath, + Resources: map[string]*ResourceState{ + "test_instance.foo": &ResourceState{ + Primary: &InstanceState{ + Meta: map[string]string{}, + }, + Deposed: []*InstanceState{ + {ID: "test"}, + nil, + }, + }, + }, + }, + }, + }, }, } for i, tc := range cases { t.Run(fmt.Sprintf("copy-%d", i), func(t *testing.T) { - actual := tc.F(tc.One.DeepCopy()) - expected := tc.F(tc.Two) + actual := tc.State.DeepCopy() + expected := tc.State if !reflect.DeepEqual(actual, expected) { - t.Fatalf("Bad: %d\n\n%s\n\n%s", i, actual, expected) + t.Fatalf("Expected: %#v\nRecevied: %#v\n", expected, actual) } }) } From 816c04309c5e9ab0a08b43140781213ef29c452b Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 13 Oct 2016 12:10:57 -0400 Subject: [PATCH 2/3] Filter nil Deposed values during State init The Deposed slice wasn't being normalized and nil values could be read in from a state file. Filter out the nils during init. There is still a bug in copystructure, but that will be addressed separately. --- terraform/state.go | 13 +++++++++++-- terraform/state_test.go | 2 ++ 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/terraform/state.go b/terraform/state.go index 2ab56ba6ceea..00609c7826a0 100644 --- a/terraform/state.go +++ b/terraform/state.go @@ -1403,9 +1403,18 @@ func (s *ResourceState) init() { s.Deposed = make([]*InstanceState, 0) } - for _, dep := range s.Deposed { - dep.init() + // clean out any possible nil values read in from the state file + end := len(s.Deposed) - 1 + for i := 0; i <= end; i++ { + if s.Deposed[i] == nil { + s.Deposed[i], s.Deposed[end] = s.Deposed[end], s.Deposed[i] + end-- + i-- + } else { + s.Deposed[i].init() + } } + s.Deposed = s.Deposed[:end+1] } func (s *ResourceState) deepcopy() *ResourceState { diff --git a/terraform/state_test.go b/terraform/state_test.go index cae8751f0726..24513200ef56 100644 --- a/terraform/state_test.go +++ b/terraform/state_test.go @@ -283,6 +283,8 @@ func TestStateDeepCopy(t *testing.T) { }, }, // Deposed + // The nil values shouldn't be there if the State was properly init'ed, + // but the Copy should still work anyway. { &State{ Version: 6, From 95786c5090c47c712bb2dfb914ce5b2f670614a0 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 13 Oct 2016 15:56:33 -0400 Subject: [PATCH 3/3] update copystructure --- vendor/github.com/mitchellh/copystructure/copystructure.go | 2 +- vendor/vendor.json | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/vendor/github.com/mitchellh/copystructure/copystructure.go b/vendor/github.com/mitchellh/copystructure/copystructure.go index 3f390764e46e..0e725ea72370 100644 --- a/vendor/github.com/mitchellh/copystructure/copystructure.go +++ b/vendor/github.com/mitchellh/copystructure/copystructure.go @@ -177,8 +177,8 @@ func (w *walker) Exit(l reflectwalk.Location) error { case reflectwalk.SliceElem: // Pop off the value and the index and set it on the slice v := w.valPop() + i := w.valPop().Interface().(int) if v.IsValid() { - i := w.valPop().Interface().(int) s := w.cs[len(w.cs)-1] se := s.Index(i) if se.CanSet() { diff --git a/vendor/vendor.json b/vendor/vendor.json index d3dde0756eda..a04d20b26fc5 100644 --- a/vendor/vendor.json +++ b/vendor/vendor.json @@ -1474,10 +1474,10 @@ "revision": "8631ce90f28644f54aeedcb3e389a85174e067d1" }, { - "checksumSHA1": "BsIMq23KDyxdhQC7g2dDz/9oHbA=", + "checksumSHA1": "guxbLo8KHHBeM0rzou4OTzzpDNs=", "path": "github.com/mitchellh/copystructure", - "revision": "c4815f984fb5c5486f6db1c9a5660e7605fd4c20", - "revisionTime": "2016-10-03T18:23:19Z" + "revision": "5af94aef99f597e6a9e1f6ac6be6ce0f3c96b49d", + "revisionTime": "2016-10-13T19:53:42Z" }, { "path": "github.com/mitchellh/go-homedir",