From cfb440ea6069a350b5fe5a4db09e9c153c2cff8b Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Fri, 2 Dec 2016 11:48:34 -0500 Subject: [PATCH] terraform: don't prune state on init() Init should only _add_ values, not remove them. During graph execution, there are steps that expect that a state isn't being actively pruned out from under it. Namely: writing deposed states. Writing deposed states has no way to handle if a state changes underneath it because the only way to uniquely identify a deposed state is its index in the deposed array. When destroying deposed resources, we set the value to ``. If the array is pruned before the next deposed destroy, then the indexes have changed, and this can cause a crash. This PR does the following (with more details below): * `init()` no longer prunes. * `ReadState()` always prunes before returning. I can't think of a scenario where this is unsafe since generally we can always START from a pruned state, its just causing problems to prune mid-execution. * Exported State APIs updated to be robust against nil ModuleStates. Instead, I think we should adopt the following semantics for init/prune in our structures that support it (Diff, for example). By having consistent semantics around these functions, we can avoid this in the future and have set expectations working with them. * `init()` (in anything) will only ever be additive, and won't change ordering or existing values. It won't remove values. * `prune()` is destructive, expectedly. * Functions on a structure must not assume a pruned structure 100% of the time. They must be robust to handle nils. This is especially important because in many cases values such as `Modules` in state are exported so end users can simply modify them outside of the exported APIs. This PR may expose us to unknown crashes but I've tried to cover our cases in exposed APIs by checking for nil. --- terraform/state.go | 36 +++++++++++++++---- terraform/state_test.go | 38 +++++++++++++++------ terraform/transform_orphan_resource.go | 4 +++ terraform/transform_orphan_resource_test.go | 25 ++++++++++++++ 4 files changed, 87 insertions(+), 16 deletions(-) diff --git a/terraform/state.go b/terraform/state.go index 90b8d4dbc..472fac0d5 100644 --- a/terraform/state.go +++ b/terraform/state.go @@ -108,6 +108,10 @@ func (s *State) Children(path []string) []*ModuleState { func (s *State) children(path []string) []*ModuleState { result := make([]*ModuleState, 0) for _, m := range s.Modules { + if m == nil { + continue + } + if len(m.Path) != len(path)+1 { continue } @@ -161,6 +165,9 @@ func (s *State) ModuleByPath(path []string) *ModuleState { func (s *State) moduleByPath(path []string) *ModuleState { for _, mod := range s.Modules { + if mod == nil { + continue + } if mod.Path == nil { panic("missing module path") } @@ -213,6 +220,10 @@ func (s *State) moduleOrphans(path []string, c *config.Config) [][]string { // Find the orphans that are nested... for _, m := range s.Modules { + if m == nil { + continue + } + // We only want modules that are at least grandchildren if len(m.Path) < len(path)+2 { continue @@ -328,6 +339,10 @@ func (s *State) Validate() error { { found := make(map[string]struct{}) for _, ms := range s.Modules { + if ms == nil { + continue + } + key := strings.Join(ms.Path, ".") if _, ok := found[key]; ok { result = multierror.Append(result, fmt.Errorf( @@ -644,12 +659,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() + if mod != nil { + mod.init() + } } if s.Remote != nil { @@ -726,7 +739,9 @@ func (s *State) sort() { // Allow modules to be sorted for _, m := range s.Modules { - m.sort() + if m != nil { + m.sort() + } } } @@ -1810,6 +1825,10 @@ func ReadState(src io.Reader) (*State, error) { panic("resulting state in load not set, assertion failed") } + // Prune the state when read it. Its possible to write unpruned states or + // for a user to make a state unpruned (nil-ing a module state for example). + result.prune() + // Validate the state file is valid if err := result.Validate(); err != nil { return nil, err @@ -1968,6 +1987,11 @@ func (s moduleStateSort) Less(i, j int) bool { a := s[i] b := s[j] + // If either is nil, then the nil one is "less" than + if a == nil || b == nil { + return a == nil + } + // If the lengths are different, then the shorter one always wins if len(a.Path) != len(b.Path) { return len(a.Path) < len(b.Path) diff --git a/terraform/state_test.go b/terraform/state_test.go index f66c2b324..507c0c4a7 100644 --- a/terraform/state_test.go +++ b/terraform/state_test.go @@ -1695,16 +1695,34 @@ func TestStateModuleOrphans_empty(t *testing.T) { // 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") - } +func TestReadState_prune(t *testing.T) { + state := &State{ + Modules: []*ModuleState{ + &ModuleState{Path: rootModulePath}, + nil, + }, + } + 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) + } + + expected := &State{ + Version: state.Version, + Lineage: state.Lineage, + } + expected.init() + + if !reflect.DeepEqual(actual, expected) { + t.Fatalf("got:\n%#v", actual) } } diff --git a/terraform/transform_orphan_resource.go b/terraform/transform_orphan_resource.go index 11721462e..e42d3c849 100644 --- a/terraform/transform_orphan_resource.go +++ b/terraform/transform_orphan_resource.go @@ -42,6 +42,10 @@ func (t *OrphanResourceTransformer) Transform(g *Graph) error { } func (t *OrphanResourceTransformer) transform(g *Graph, ms *ModuleState) error { + if ms == nil { + return nil + } + // Get the configuration for this path. The configuration might be // nil if the module was removed from the configuration. This is okay, // this just means that every resource is an orphan. diff --git a/terraform/transform_orphan_resource_test.go b/terraform/transform_orphan_resource_test.go index ce29eecf1..c26f056a1 100644 --- a/terraform/transform_orphan_resource_test.go +++ b/terraform/transform_orphan_resource_test.go @@ -59,6 +59,31 @@ func TestOrphanResourceTransformer(t *testing.T) { } } +func TestOrphanResourceTransformer_nilModule(t *testing.T) { + mod := testModule(t, "transform-orphan-basic") + state := &State{ + Modules: []*ModuleState{nil}, + } + + g := Graph{Path: RootModulePath} + { + tf := &ConfigTransformer{Module: mod} + if err := tf.Transform(&g); err != nil { + t.Fatalf("err: %s", err) + } + } + + { + tf := &OrphanResourceTransformer{ + Concrete: testOrphanResourceConcreteFunc, + State: state, Module: mod, + } + if err := tf.Transform(&g); err != nil { + t.Fatalf("err: %s", err) + } + } +} + func TestOrphanResourceTransformer_countGood(t *testing.T) { mod := testModule(t, "transform-orphan-count") state := &State{