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 `<nil>`. 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.
This commit is contained in:
Mitchell Hashimoto 2016-12-02 11:48:34 -05:00
parent 08a56304bb
commit cfb440ea60
No known key found for this signature in database
GPG Key ID: 744E147AA52F5B0A
4 changed files with 87 additions and 16 deletions

View File

@ -108,6 +108,10 @@ func (s *State) Children(path []string) []*ModuleState {
func (s *State) children(path []string) []*ModuleState { func (s *State) children(path []string) []*ModuleState {
result := make([]*ModuleState, 0) result := make([]*ModuleState, 0)
for _, m := range s.Modules { for _, m := range s.Modules {
if m == nil {
continue
}
if len(m.Path) != len(path)+1 { if len(m.Path) != len(path)+1 {
continue continue
} }
@ -161,6 +165,9 @@ func (s *State) ModuleByPath(path []string) *ModuleState {
func (s *State) moduleByPath(path []string) *ModuleState { func (s *State) moduleByPath(path []string) *ModuleState {
for _, mod := range s.Modules { for _, mod := range s.Modules {
if mod == nil {
continue
}
if mod.Path == nil { if mod.Path == nil {
panic("missing module path") panic("missing module path")
} }
@ -213,6 +220,10 @@ func (s *State) moduleOrphans(path []string, c *config.Config) [][]string {
// Find the orphans that are nested... // Find the orphans that are nested...
for _, m := range s.Modules { for _, m := range s.Modules {
if m == nil {
continue
}
// We only want modules that are at least grandchildren // We only want modules that are at least grandchildren
if len(m.Path) < len(path)+2 { if len(m.Path) < len(path)+2 {
continue continue
@ -328,6 +339,10 @@ func (s *State) Validate() error {
{ {
found := make(map[string]struct{}) found := make(map[string]struct{})
for _, ms := range s.Modules { for _, ms := range s.Modules {
if ms == nil {
continue
}
key := strings.Join(ms.Path, ".") key := strings.Join(ms.Path, ".")
if _, ok := found[key]; ok { if _, ok := found[key]; ok {
result = multierror.Append(result, fmt.Errorf( result = multierror.Append(result, fmt.Errorf(
@ -644,13 +659,11 @@ func (s *State) init() {
} }
s.ensureHasLineage() 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 { for _, mod := range s.Modules {
if mod != nil {
mod.init() mod.init()
} }
}
if s.Remote != nil { if s.Remote != nil {
s.Remote.init() s.Remote.init()
@ -726,8 +739,10 @@ func (s *State) sort() {
// Allow modules to be sorted // Allow modules to be sorted
for _, m := range s.Modules { for _, m := range s.Modules {
if m != nil {
m.sort() m.sort()
} }
}
} }
func (s *State) String() string { func (s *State) String() string {
@ -1810,6 +1825,10 @@ func ReadState(src io.Reader) (*State, error) {
panic("resulting state in load not set, assertion failed") 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 // Validate the state file is valid
if err := result.Validate(); err != nil { if err := result.Validate(); err != nil {
return nil, err return nil, err
@ -1968,6 +1987,11 @@ func (s moduleStateSort) Less(i, j int) bool {
a := s[i] a := s[i]
b := s[j] 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 the lengths are different, then the shorter one always wins
if len(a.Path) != len(b.Path) { if len(a.Path) != len(b.Path) {
return len(a.Path) < len(b.Path) return len(a.Path) < len(b.Path)

View File

@ -1695,16 +1695,34 @@ func TestStateModuleOrphans_empty(t *testing.T) {
// just calling this to check for panic // just calling this to check for panic
state.ModuleOrphans(RootModulePath, nil) state.ModuleOrphans(RootModulePath, nil)
}
for _, mod := range state.Modules { func TestReadState_prune(t *testing.T) {
if mod == nil { state := &State{
t.Fatal("found nil module") Modules: []*ModuleState{
&ModuleState{Path: rootModulePath},
nil,
},
} }
if mod.Path == nil { state.init()
t.Fatal("found nil module path")
buf := new(bytes.Buffer)
if err := WriteState(state, buf); err != nil {
t.Fatalf("err: %s", err)
} }
if len(mod.Path) == 0 {
t.Fatal("found empty module path") 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)
} }
} }

View File

@ -42,6 +42,10 @@ func (t *OrphanResourceTransformer) Transform(g *Graph) error {
} }
func (t *OrphanResourceTransformer) transform(g *Graph, ms *ModuleState) 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 // Get the configuration for this path. The configuration might be
// nil if the module was removed from the configuration. This is okay, // nil if the module was removed from the configuration. This is okay,
// this just means that every resource is an orphan. // this just means that every resource is an orphan.

View File

@ -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) { func TestOrphanResourceTransformer_countGood(t *testing.T) {
mod := testModule(t, "transform-orphan-count") mod := testModule(t, "transform-orphan-count")
state := &State{ state := &State{