From def55e52ca6273d92014dabf8a08162b7c8f2c83 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 18 Nov 2016 17:59:07 -0500 Subject: [PATCH 1/4] An empty module in state can panic An empty module, or a module with an empty path can panic during graph traversal. Make sure we clear these out when reading and writing the state. --- terraform/state.go | 11 +++++++++++ terraform/state_test.go | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 45 insertions(+) diff --git a/terraform/state.go b/terraform/state.go index 30023fb297..a969357fa9 100644 --- a/terraform/state.go +++ b/terraform/state.go @@ -644,6 +644,17 @@ func (s *State) init() { } s.ensureHasLineage() + // Filter out empty modules. + // A module is always assumed to have a path, and it's length isn't always + // bounds checked later on. Modules may be "emptied" during destroy, but we + // never want to store those in the state. + for i := 0; i < len(s.Modules); i++ { + if s.Modules[i] == nil || len(s.Modules[i].Path) == 0 { + s.Modules = append(s.Modules[:i], s.Modules[i+1:]...) + i-- + } + } + for _, mod := range s.Modules { mod.init() } diff --git a/terraform/state_test.go b/terraform/state_test.go index de84b8431f..f66c2b3242 100644 --- a/terraform/state_test.go +++ b/terraform/state_test.go @@ -1674,3 +1674,37 @@ func TestParseResourceStateKey(t *testing.T) { } } } + +func TestStateModuleOrphans_empty(t *testing.T) { + state := &State{ + Modules: []*ModuleState{ + &ModuleState{ + Path: RootModulePath, + }, + &ModuleState{ + Path: []string{RootModuleName, "foo", "bar"}, + }, + &ModuleState{ + Path: []string{}, + }, + nil, + }, + } + + state.init() + + // just calling this to check for panic + state.ModuleOrphans(RootModulePath, nil) + + for _, mod := range state.Modules { + if mod == nil { + t.Fatal("found nil module") + } + if mod.Path == nil { + t.Fatal("found nil module path") + } + if len(mod.Path) == 0 { + t.Fatal("found empty module path") + } + } +} From a5cb5305714090879c4a4a751efb23abeb8b884b Mon Sep 17 00:00:00 2001 From: James Bardin Date: Sun, 20 Nov 2016 11:31:24 -0500 Subject: [PATCH 2/4] Move the state module cleanup from init to prune It makes for sense for this to happen in State.prune(). Also move a redundant pruning from ResourceState.init, and make sure ResourceState.prune is called from the parent's prune method. --- terraform/state.go | 46 ++++++++++++++++++++-------------------------- 1 file changed, 20 insertions(+), 26 deletions(-) diff --git a/terraform/state.go b/terraform/state.go index a969357fa9..90b8d4dbc0 100644 --- a/terraform/state.go +++ b/terraform/state.go @@ -644,16 +644,9 @@ func (s *State) init() { } s.ensureHasLineage() - // Filter out empty modules. - // A module is always assumed to have a path, and it's length isn't always - // bounds checked later on. Modules may be "emptied" during destroy, but we - // never want to store those in the state. - for i := 0; i < len(s.Modules); i++ { - if s.Modules[i] == nil || len(s.Modules[i].Path) == 0 { - s.Modules = append(s.Modules[:i], s.Modules[i+1:]...) - i-- - } - } + // We can't trust that state read from a file doesn't have nil/empty + // modules + s.prune() for _, mod := range s.Modules { mod.init() @@ -662,6 +655,7 @@ func (s *State) init() { if s.Remote != nil { s.Remote.init() } + } func (s *State) EnsureHasLineage() { @@ -706,6 +700,18 @@ func (s *State) prune() { if s == nil { return } + + // Filter out empty modules. + // A module is always assumed to have a path, and it's length isn't always + // bounds checked later on. Modules may be "emptied" during destroy, but we + // never want to store those in the state. + for i := 0; i < len(s.Modules); i++ { + if s.Modules[i] == nil || len(s.Modules[i].Path) == 0 { + s.Modules = append(s.Modules[:i], s.Modules[i+1:]...) + i-- + } + } + for _, mod := range s.Modules { mod.prune() } @@ -1085,11 +1091,12 @@ func (m *ModuleState) prune() { defer m.Unlock() for k, v := range m.Resources { - v.prune() - - if (v.Primary == nil || v.Primary.ID == "") && len(v.Deposed) == 0 { + if v == nil || (v.Primary == nil || v.Primary.ID == "") && len(v.Deposed) == 0 { delete(m.Resources, k) + continue } + + v.prune() } for k, v := range m.Outputs { @@ -1431,19 +1438,6 @@ func (s *ResourceState) init() { if s.Deposed == nil { s.Deposed = make([]*InstanceState, 0) } - - // 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 { From 7715bc84233edd8fbf81deb9fb0d2b98256572fe Mon Sep 17 00:00:00 2001 From: James Bardin Date: Sun, 20 Nov 2016 12:37:51 -0500 Subject: [PATCH 3/4] change failing test to use subtests --- state/cache_test.go | 41 ++++++++++++++++++++++------------------- 1 file changed, 22 insertions(+), 19 deletions(-) diff --git a/state/cache_test.go b/state/cache_test.go index 0edfd85643..44ac341773 100644 --- a/state/cache_test.go +++ b/state/cache_test.go @@ -1,6 +1,7 @@ package state import ( + "fmt" "os" "reflect" "testing" @@ -77,29 +78,31 @@ func TestCacheState_RefreshState(t *testing.T) { expected: CacheRefreshLocalNewer, }, } { - cache := testLocalState(t) - durable := testLocalState(t) - defer os.Remove(cache.Path) - defer os.Remove(durable.Path) + t.Run(fmt.Sprintf("%d", i), func(t *testing.T) { + cache := testLocalState(t) + durable := testLocalState(t) + defer os.Remove(cache.Path) + defer os.Remove(durable.Path) - cs := &CacheState{ - Cache: cache, - Durable: durable, - } + cs := &CacheState{ + Cache: cache, + Durable: durable, + } - state := cache.State() - state.Modules = test.cacheModules - if err := cs.WriteState(state); err != nil { - t.Fatalf("err: %s", err) - } + state := cache.State() + state.Modules = test.cacheModules + if err := cs.WriteState(state); err != nil { + t.Fatalf("err: %s", err) + } - if err := cs.RefreshState(); err != nil { - t.Fatalf("err: %s", err) - } + if err := cs.RefreshState(); err != nil { + t.Fatalf("err: %s", err) + } - if cs.RefreshResult() != test.expected { - t.Fatalf("bad %d: %v", i, cs.RefreshResult()) - } + if cs.RefreshResult() != test.expected { + t.Fatalf("bad %d: %v", i, cs.RefreshResult()) + } + }) } } From 9616618de1dae9a38667f1b64030c6e8af5b7464 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Sun, 20 Nov 2016 13:13:43 -0500 Subject: [PATCH 4/4] Make sure test has a valid ResourceState Empty resources are now pruned more aggressively, so make sure there is a valid ResourceState in the test ModuleState. --- state/cache_test.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/state/cache_test.go b/state/cache_test.go index 44ac341773..7252637e00 100644 --- a/state/cache_test.go +++ b/state/cache_test.go @@ -71,7 +71,11 @@ func TestCacheState_RefreshState(t *testing.T) { &terraform.ModuleState{ Path: terraform.RootModulePath, Resources: map[string]*terraform.ResourceState{ - "foo.foo": &terraform.ResourceState{}, + "foo.foo": &terraform.ResourceState{ + Primary: &terraform.InstanceState{ + ID: "ID", + }, + }, }, }, },