Merge pull request #10233 from hashicorp/jbardin/GH-10229

An empty module in state can panic

state/cache_test.go

@@ -1,6 +1,7 @@
 package state
 
 import (
+	"fmt"
 	"os"
 	"reflect"
 	"testing"
@@ -70,36 +71,42 @@ 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",
+							},
+						},
 					},
 				},
 			},
 			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())
+			}
+		})
 	}
 }
 

terraform/state.go

@@ -644,6 +644,10 @@ func (s *State) init() {
 	}
 	s.ensureHasLineage()
 
+	// 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()
 	}
@@ -651,6 +655,7 @@ func (s *State) init() {
 	if s.Remote != nil {
 		s.Remote.init()
 	}
+
 }
 
 func (s *State) EnsureHasLineage() {
@@ -695,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()
 	}
@@ -1074,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 {
@@ -1420,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 {

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