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

@@ -1181,6 +1181,7 @@ func TestApply_backup(t *testing.T) {
 			},
 		},
 	}
+	originalState.Init()
 
 	statePath := testStateFile(t, originalState)
 	backupPath := testTempFile(t)

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 {

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"
 `

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": []
         }
     ]
 }

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
+	initial.Init()
-	terraform.WriteState(initial, &scratch)
+
 	return initial
 }

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
 

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"},
@@ -1280,6 +1290,7 @@ func TestInstanceState_MergeDiff_nilDiff(t *testing.T) {
 func TestReadWriteState(t *testing.T) {
 	state := &State{
 		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)
 	}
 }
 

terraform/state_upgrade_v1_to_v2.go

@@ -38,6 +38,7 @@ func upgradeStateV1ToV2(old *stateV1) (*State, error) {
 	}
 
 	newState.sort()
+	newState.init()
 
 	return newState, nil
 }

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