From cdb80f68a8e20369d57e0f9c61abe1a18681cdec Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 10 Aug 2016 15:47:25 -0400 Subject: [PATCH] Ensure better state normalization Fix checksum issue with remote state If we read a state file with "null" objects in a module and they become initialized to an empty map the state file may be written out with empty objects rather than "null", changing the checksum. If we can detect this, increment the serial number to prevent a conflict in atlas. Our fakeAtlas test server now needs to decode the state directly rather than using the ReadState function, so as to be able to read the state unaltered. The terraform.State data structures have initialization spread out throughout the package. More thoroughly initialize State during ReadState, and add a call to init() during WriteState as another normalization safeguard. Expose State.init through an exported Init() method, so that a new State can be completely realized outside of the terraform package. Additionally, the internal init now completely walks all internal state structures ensuring that all maps and slices are initialized. While it was mentioned before that the `init()` methods are problematic with too many call sites, expanding this out better exposes the entry points that will need to be refactored later for improved concurrency handling. The State structures had a mix of `omitempty` fields. Remove omitempty for all maps and slices as part of this normalization process. Make Lineage mandatory, which is now explicitly set in some tests. --- command/apply_test.go | 1 + command/command_test.go | 4 +- command/refresh_test.go | 25 +++++-- state/remote/atlas_test.go | 22 ++++-- state/testing.go | 11 ++- terraform/state.go | 104 +++++++++++++++++++++++++--- terraform/state_test.go | 20 +++++- terraform/state_upgrade_v1_to_v2.go | 1 + terraform/upgrade_state_v1_test.go | 3 +- 9 files changed, 162 insertions(+), 29 deletions(-) diff --git a/command/apply_test.go b/command/apply_test.go index ca0816ba7..1b6e87069 100644 --- a/command/apply_test.go +++ b/command/apply_test.go @@ -1181,6 +1181,7 @@ func TestApply_backup(t *testing.T) { }, }, } + originalState.Init() statePath := testStateFile(t, originalState) backupPath := testTempFile(t) diff --git a/command/command_test.go b/command/command_test.go index 174d439ac..01863ee39 100644 --- a/command/command_test.go +++ b/command/command_test.go @@ -131,7 +131,7 @@ func testReadPlan(t *testing.T, path string) *terraform.Plan { // testState returns a test State structure that we use for a lot of tests. func testState() *terraform.State { - return &terraform.State{ + state := &terraform.State{ Version: 2, Modules: []*terraform.ModuleState{ &terraform.ModuleState{ @@ -148,6 +148,8 @@ func testState() *terraform.State { }, }, } + state.Init() + return state } func testStateFile(t *testing.T, s *terraform.State) string { diff --git a/command/refresh_test.go b/command/refresh_test.go index 91ef22b17..c7d0ea6eb 100644 --- a/command/refresh_test.go +++ b/command/refresh_test.go @@ -173,7 +173,7 @@ func TestRefresh_defaultState(t *testing.T) { } p.RefreshFn = nil - p.RefreshReturn = &terraform.InstanceState{ID: "yes"} + p.RefreshReturn = newInstanceState("yes") args := []string{ testFixturePath("refresh"), @@ -200,7 +200,8 @@ func TestRefresh_defaultState(t *testing.T) { actual := newState.RootModule().Resources["test_instance.foo"].Primary expected := p.RefreshReturn if !reflect.DeepEqual(actual, expected) { - t.Fatalf("bad: %#v", actual) + t.Logf("expected:\n%#v", expected) + t.Fatalf("bad:\n%#v", actual) } f, err = os.Open(statePath + DefaultBackupExtension) @@ -347,7 +348,7 @@ func TestRefresh_outPath(t *testing.T) { } p.RefreshFn = nil - p.RefreshReturn = &terraform.InstanceState{ID: "yes"} + p.RefreshReturn = newInstanceState("yes") args := []string{ "-state", statePath, @@ -577,7 +578,7 @@ func TestRefresh_backup(t *testing.T) { } p.RefreshFn = nil - p.RefreshReturn = &terraform.InstanceState{ID: "yes"} + p.RefreshReturn = newInstanceState("yes") args := []string{ "-state", statePath, @@ -662,7 +663,7 @@ func TestRefresh_disableBackup(t *testing.T) { } p.RefreshFn = nil - p.RefreshReturn = &terraform.InstanceState{ID: "yes"} + p.RefreshReturn = newInstanceState("yes") args := []string{ "-state", statePath, @@ -742,6 +743,20 @@ func TestRefresh_displaysOutputs(t *testing.T) { } } +// When creating an InstaneState for direct comparison to one contained in +// terraform.State, all fields must be inintialized (duplicating the +// InstanceState.init() method) +func newInstanceState(id string) *terraform.InstanceState { + return &terraform.InstanceState{ + ID: id, + Attributes: make(map[string]string), + Ephemeral: terraform.EphemeralState{ + ConnInfo: make(map[string]string), + }, + Meta: make(map[string]string), + } +} + const refreshVarFile = ` foo = "bar" ` diff --git a/state/remote/atlas_test.go b/state/remote/atlas_test.go index 060d79455..f5fe127d6 100644 --- a/state/remote/atlas_test.go +++ b/state/remote/atlas_test.go @@ -5,6 +5,7 @@ import ( "crypto/md5" "crypto/tls" "crypto/x509" + "encoding/json" "net/http" "net/http/httptest" "net/url" @@ -161,6 +162,9 @@ func TestAtlasClient_LegitimateConflict(t *testing.T) { t.Fatalf("err: %s", err) } + var buf bytes.Buffer + terraform.WriteState(state, &buf) + // Changing the state but not the serial. Should generate a conflict. state.RootModule().Outputs["drift"] = &terraform.OutputState{ Type: "string", @@ -244,7 +248,10 @@ func (f *fakeAtlas) Server() *httptest.Server { } func (f *fakeAtlas) CurrentState() *terraform.State { - currentState, err := terraform.ReadState(bytes.NewReader(f.state)) + // we read the state manually here, because terraform may alter state + // during read + currentState := &terraform.State{} + err := json.Unmarshal(f.state, currentState) if err != nil { f.t.Fatalf("err: %s", err) } @@ -288,10 +295,15 @@ func (f *fakeAtlas) handler(resp http.ResponseWriter, req *http.Request) { var buf bytes.Buffer buf.ReadFrom(req.Body) sum := md5.Sum(buf.Bytes()) - state, err := terraform.ReadState(&buf) + + // we read the state manually here, because terraform may alter state + // during read + state := &terraform.State{} + err := json.Unmarshal(buf.Bytes(), state) if err != nil { f.t.Fatalf("err: %s", err) } + conflict := f.CurrentSerial() == state.Serial && f.CurrentSum() != sum conflict = conflict || f.alwaysConflict if conflict { @@ -351,7 +363,8 @@ var testStateModuleOrderChange = []byte( var testStateSimple = []byte( `{ "version": 3, - "serial": 1, + "serial": 2, + "lineage": "c00ad9ac-9b35-42fe-846e-b06f0ef877e9", "modules": [ { "path": [ @@ -364,7 +377,8 @@ var testStateSimple = []byte( "value": "bar" } }, - "resources": null + "resources": {}, + "depends_on": [] } ] } diff --git a/state/testing.go b/state/testing.go index 207dda7d7..bf73ba4ff 100644 --- a/state/testing.go +++ b/state/testing.go @@ -1,7 +1,6 @@ package state import ( - "bytes" "reflect" "testing" @@ -29,12 +28,12 @@ func TestState(t *testing.T, s interface{}) { // Check that the initial state is correct if state := reader.State(); !current.Equal(state) { - t.Fatalf("not initial: %#v\n\n%#v", state, current) + t.Fatalf("not initial:\n%#v\n\n%#v", state, current) } // Write a new state and verify that we have it if ws, ok := s.(StateWriter); ok { - current.Modules = append(current.Modules, &terraform.ModuleState{ + current.AddModuleState(&terraform.ModuleState{ Path: []string{"root"}, Outputs: map[string]*terraform.OutputState{ "bar": &terraform.OutputState{ @@ -50,7 +49,7 @@ func TestState(t *testing.T, s interface{}) { } if actual := reader.State(); !actual.Equal(current) { - t.Fatalf("bad: %#v\n\n%#v", actual, current) + t.Fatalf("bad:\n%#v\n\n%#v", actual, current) } } @@ -146,7 +145,7 @@ func TestStateInitial() *terraform.State { }, } - var scratch bytes.Buffer - terraform.WriteState(initial, &scratch) + initial.Init() + return initial } diff --git a/terraform/state.go b/terraform/state.go index 473a8adbc..d6dc17194 100644 --- a/terraform/state.go +++ b/terraform/state.go @@ -70,7 +70,7 @@ type State struct { // Apart from the guarantee that collisions between two lineages // are very unlikely, this value is opaque and external callers // should only compare lineage strings byte-for-byte for equality. - Lineage string `json:"lineage,omitempty"` + Lineage string `json:"lineage"` // Remote is used to track the metadata required to // pull and push state files from a remote storage endpoint. @@ -113,7 +113,13 @@ func (s *State) Children(path []string) []*ModuleState { // This should be the preferred method to add module states since it // allows us to optimize lookups later as well as control sorting. func (s *State) AddModule(path []string) *ModuleState { - m := &ModuleState{Path: path} + // check if the module exists first + m := s.ModuleByPath(path) + if m != nil { + return m + } + + m = &ModuleState{Path: path} m.init() s.Modules = append(s.Modules, m) s.sort() @@ -498,6 +504,10 @@ func (s *State) FromFutureTerraform() bool { return SemVersion.LessThan(v) } +func (s *State) Init() { + s.init() +} + func (s *State) init() { if s.Version == 0 { s.Version = StateVersion @@ -506,6 +516,14 @@ func (s *State) init() { s.AddModule(rootModulePath) } s.EnsureHasLineage() + + for _, mod := range s.Modules { + mod.init() + } + + if s.Remote != nil { + s.Remote.init() + } } func (s *State) EnsureHasLineage() { @@ -517,6 +535,21 @@ func (s *State) EnsureHasLineage() { } } +// AddModuleState insert this module state and override any existing ModuleState +func (s *State) AddModuleState(mod *ModuleState) { + mod.init() + + for i, m := range s.Modules { + if reflect.DeepEqual(m.Path, mod.Path) { + s.Modules[i] = mod + return + } + } + + s.Modules = append(s.Modules, mod) + s.sort() +} + // prune is used to remove any resources that are no longer required func (s *State) prune() { if s == nil { @@ -586,6 +619,12 @@ type RemoteState struct { Config map[string]string `json:"config"` } +func (r *RemoteState) init() { + if r.Config == nil { + r.Config = make(map[string]string) + } +} + func (r *RemoteState) deepcopy() *RemoteState { confCopy := make(map[string]string, len(r.Config)) for k, v := range r.Config { @@ -713,7 +752,7 @@ type ModuleState struct { // Terraform. If Terraform doesn't find a matching ID in the // overall state, then it assumes it isn't managed and doesn't // worry about it. - Dependencies []string `json:"depends_on,omitempty"` + Dependencies []string `json:"depends_on"` } // Equal tests whether one module state is equal to another. @@ -817,12 +856,23 @@ func (m *ModuleState) View(id string) *ModuleState { } func (m *ModuleState) init() { + if m.Path == nil { + m.Path = []string{} + } if m.Outputs == nil { m.Outputs = make(map[string]*OutputState) } if m.Resources == nil { m.Resources = make(map[string]*ResourceState) } + + if m.Dependencies == nil { + m.Dependencies = make([]string, 0) + } + + for _, rs := range m.Resources { + rs.init() + } } func (m *ModuleState) deepcopy() *ModuleState { @@ -1095,7 +1145,7 @@ type ResourceState struct { // Terraform. If Terraform doesn't find a matching ID in the // overall state, then it assumes it isn't managed and doesn't // worry about it. - Dependencies []string `json:"depends_on,omitempty"` + Dependencies []string `json:"depends_on"` // Primary is the current active instance for this resource. // It can be replaced but only after a successful creation. @@ -1113,13 +1163,13 @@ type ResourceState struct { // // An instance will remain in the Deposed list until it is successfully // destroyed and purged. - Deposed []*InstanceState `json:"deposed,omitempty"` + Deposed []*InstanceState `json:"deposed"` // Provider is used when a resource is connected to a provider with an alias. // If this string is empty, the resource is connected to the default provider, // e.g. "aws_instance" goes with the "aws" provider. // If the resource block contained a "provider" key, that value will be set here. - Provider string `json:"provider,omitempty"` + Provider string `json:"provider"` } // Equal tests whether two ResourceStates are equal. @@ -1171,6 +1221,18 @@ func (r *ResourceState) init() { r.Primary = &InstanceState{} } r.Primary.init() + + if r.Dependencies == nil { + r.Dependencies = []string{} + } + + if r.Deposed == nil { + r.Deposed = make([]*InstanceState, 0) + } + + for _, dep := range r.Deposed { + dep.init() + } } func (r *ResourceState) deepcopy() *ResourceState { @@ -1222,7 +1284,7 @@ type InstanceState struct { // Attributes are basic information about the resource. Any keys here // are accessible in variable format within Terraform configurations: // ${resourcetype.name.attribute}. - Attributes map[string]string `json:"attributes,omitempty"` + Attributes map[string]string `json:"attributes"` // Ephemeral is used to store any state associated with this instance // that is necessary for the Terraform run to complete, but is not @@ -1232,10 +1294,10 @@ type InstanceState struct { // Meta is a simple K/V map that is persisted to the State but otherwise // ignored by Terraform core. It's meant to be used for accounting by // external client code. - Meta map[string]string `json:"meta,omitempty"` + Meta map[string]string `json:"meta"` // Tainted is used to mark a resource for recreation. - Tainted bool `json:"tainted,omitempty"` + Tainted bool `json:"tainted"` } func (i *InstanceState) init() { @@ -1498,6 +1560,7 @@ func ReadState(src io.Reader) (*State, error) { return nil, fmt.Errorf("Terraform %s does not support state version %d, please update.", SemVersion.String(), versionIdentifier.Version) } + } func ReadStateV1(jsonBytes []byte) (*stateV1, error) { @@ -1543,6 +1606,9 @@ func ReadStateV2(jsonBytes []byte) (*State, error) { // Sort it state.sort() + // catch any unitialized fields in the state + state.init() + return state, nil } @@ -1575,6 +1641,23 @@ func ReadStateV3(jsonBytes []byte) (*State, error) { // Sort it state.sort() + // catch any unitialized fields in the state + state.init() + + // Now we write the state back out to detect any changes in normaliztion. + // If our state is now written out differently, bump the serial number to + // prevent conflicts. + var buf bytes.Buffer + err := WriteState(state, &buf) + if err != nil { + return nil, err + } + + if !bytes.Equal(jsonBytes, buf.Bytes()) { + log.Println("[INFO] state modified during read or write. incrementing serial number") + state.Serial++ + } + return state, nil } @@ -1583,6 +1666,9 @@ func WriteState(d *State, dst io.Writer) error { // Make sure it is sorted d.sort() + // make sure we have no uninitialized fields + d.init() + // Ensure the version is set d.Version = StateVersion diff --git a/terraform/state_test.go b/terraform/state_test.go index b9e727457..41e5f6bcf 100644 --- a/terraform/state_test.go +++ b/terraform/state_test.go @@ -90,6 +90,7 @@ func TestStateOutputTypeRoundTrip(t *testing.T) { }, }, } + state.init() buf := new(bytes.Buffer) if err := WriteState(state, buf); err != nil { @@ -102,7 +103,8 @@ func TestStateOutputTypeRoundTrip(t *testing.T) { } if !reflect.DeepEqual(state, roundTripped) { - t.Fatalf("bad: %#v", roundTripped) + t.Logf("expected:\n%#v", state) + t.Fatalf("got:\n%#v", roundTripped) } } @@ -121,6 +123,8 @@ func TestStateModuleOrphans(t *testing.T) { }, } + state.init() + config := testModule(t, "state-module-orphans").Config() actual := state.ModuleOrphans(RootModulePath, config) expected := [][]string{ @@ -144,6 +148,8 @@ func TestStateModuleOrphans_nested(t *testing.T) { }, } + state.init() + actual := state.ModuleOrphans(RootModulePath, nil) expected := [][]string{ []string{RootModuleName, "foo"}, @@ -169,6 +175,8 @@ func TestStateModuleOrphans_nilConfig(t *testing.T) { }, } + state.init() + actual := state.ModuleOrphans(RootModulePath, nil) expected := [][]string{ []string{RootModuleName, "foo"}, @@ -195,6 +203,8 @@ func TestStateModuleOrphans_deepNestedNilConfig(t *testing.T) { }, } + state.init() + actual := state.ModuleOrphans(RootModulePath, nil) expected := [][]string{ []string{RootModuleName, "parent"}, @@ -1279,7 +1289,8 @@ func TestInstanceState_MergeDiff_nilDiff(t *testing.T) { func TestReadWriteState(t *testing.T) { state := &State{ - Serial: 9, + Serial: 9, + Lineage: "5d1ad1a1-4027-4665-a908-dbe6adff11d8", Remote: &RemoteState{ Type: "http", Config: map[string]string{ @@ -1309,6 +1320,7 @@ func TestReadWriteState(t *testing.T) { }, }, } + state.init() buf := new(bytes.Buffer) if err := WriteState(state, buf); err != nil { @@ -1328,9 +1340,11 @@ func TestReadWriteState(t *testing.T) { // ReadState should not restore sensitive information! mod := state.RootModule() mod.Resources["foo"].Primary.Ephemeral = EphemeralState{} + mod.Resources["foo"].Primary.Ephemeral.init() if !reflect.DeepEqual(actual, state) { - t.Fatalf("bad: %#v", actual) + t.Logf("expected:\n%#v", state) + t.Fatalf("got:\n%#v", actual) } } diff --git a/terraform/state_upgrade_v1_to_v2.go b/terraform/state_upgrade_v1_to_v2.go index d4bf9f8f0..038615336 100644 --- a/terraform/state_upgrade_v1_to_v2.go +++ b/terraform/state_upgrade_v1_to_v2.go @@ -38,6 +38,7 @@ func upgradeStateV1ToV2(old *stateV1) (*State, error) { } newState.sort() + newState.init() return newState, nil } diff --git a/terraform/upgrade_state_v1_test.go b/terraform/upgrade_state_v1_test.go index ad6d57409..405cba949 100644 --- a/terraform/upgrade_state_v1_test.go +++ b/terraform/upgrade_state_v1_test.go @@ -35,7 +35,8 @@ func TestReadUpgradeStateV1toV3(t *testing.T) { } if !reflect.DeepEqual(actual, roundTripped) { - t.Fatalf("bad: %#v", actual) + t.Logf("actual:\n%#v", actual) + t.Fatalf("roundTripped:\n%#v", roundTripped) } }