Merge pull request #8120 from hashicorp/jbardin/null-state-module

Fix panic from a null resources module in a state file

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"},
@@ -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)
 	}
 }
 

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