Merge pull request #11836 from hashicorp/jbardin/state-locking

Add locking during backend configuration

command/meta_backend.go

@@ -530,6 +530,13 @@ func (m *Meta) backendFromPlan(opts *BackendOpts) (backend.Backend, error) {
 	if err != nil {
 		return nil, fmt.Errorf("Error reading state: %s", err)
 	}
+
+	unlock, err := lockState(realMgr, "backend from plan")
+	if err != nil {
+		return nil, err
+	}
+	defer unlock()
+
 	if err := realMgr.RefreshState(); err != nil {
 		return nil, fmt.Errorf("Error reading state: %s", err)
 	}
@@ -574,6 +581,8 @@ func (m *Meta) backendFromPlan(opts *BackendOpts) (backend.Backend, error) {
 		newState.Remote = nil
 		newState.Backend = nil
 	}
+
+	// realMgr locked above
 	if err := realMgr.WriteState(newState); err != nil {
 		return nil, fmt.Errorf("Error writing state: %s", err)
 	}
@@ -974,6 +983,12 @@ func (m *Meta) backend_C_r_s(
 		}
 	}
 
+	unlock, err := lockState(sMgr, "backend_C_r_s")
+	if err != nil {
+		return nil, err
+	}
+	defer unlock()
+
 	// Store the metadata in our saved state location
 	s := sMgr.State()
 	if s == nil {
@@ -984,6 +999,7 @@ func (m *Meta) backend_C_r_s(
 		Config: c.RawConfig.Raw,
 		Hash:   c.Hash,
 	}
+
 	if err := sMgr.WriteState(s); err != nil {
 		return nil, fmt.Errorf(errBackendWriteSaved, err)
 	}
@@ -1009,6 +1025,9 @@ func (m *Meta) backend_C_r_S_changed(
 			strings.TrimSpace(outputBackendReconfigure))))
 	}
 
+	// Get the old state
+	s := sMgr.State()
+
 	// Get the backend
 	b, err := m.backendInitFromConfig(c)
 	if err != nil {
@@ -1020,7 +1039,7 @@ func (m *Meta) backend_C_r_S_changed(
 	copy, err := m.confirm(&terraform.InputOpts{
 		Id:          "backend-migrate-to-new",
 		Query:       fmt.Sprintf("Do you want to copy the state from %q?", c.Type),
-		Description: strings.TrimSpace(inputBackendMigrateChange),
+		Description: strings.TrimSpace(fmt.Sprintf(inputBackendMigrateChange, c.Type, s.Backend.Type)),
 	})
 	if err != nil {
 		return nil, fmt.Errorf(
@@ -1037,8 +1056,6 @@ func (m *Meta) backend_C_r_S_changed(
 				"Error loading previously configured backend: %s", err)
 		}
 
-		// Get the old state
-		s := sMgr.State()
 		oldState, err := oldB.State()
 		if err != nil {
 			return nil, fmt.Errorf(
@@ -1070,8 +1087,14 @@ func (m *Meta) backend_C_r_S_changed(
 		}
 	}
 
+	unlock, err := lockState(sMgr, "backend_C_r_S_changed")
+	if err != nil {
+		return nil, err
+	}
+	defer unlock()
+
 	// Update the backend state
-	s := sMgr.State()
+	s = sMgr.State()
 	if s == nil {
 		s = terraform.NewState()
 	}
@@ -1080,6 +1103,7 @@ func (m *Meta) backend_C_r_S_changed(
 		Config: c.RawConfig.Raw,
 		Hash:   c.Hash,
 	}
+
 	if err := sMgr.WriteState(s); err != nil {
 		return nil, fmt.Errorf(errBackendWriteSaved, err)
 	}
@@ -1220,12 +1244,19 @@ func (m *Meta) backend_C_R_S_unchanged(
 		}
 	}
 
+	unlock, err := lockState(sMgr, "backend_C_R_S_unchanged")
+	if err != nil {
+		return nil, err
+	}
+	defer unlock()
+
 	// Unset the remote state
 	s = sMgr.State()
 	if s == nil {
 		s = terraform.NewState()
 	}
 	s.Remote = nil
+
 	if err := sMgr.WriteState(s); err != nil {
 		return nil, fmt.Errorf(strings.TrimSpace(errBackendClearLegacy), err)
 	}
@@ -1368,6 +1399,21 @@ func init() {
 	backendlegacy.Init(Backends)
 }
 
+// simple wrapper to check for a state.Locker and always provide an unlock
+// function to defer.
+func lockState(s state.State, info string) (func() error, error) {
+	l, ok := s.(state.Locker)
+	if !ok {
+		return func() error { return nil }, nil
+	}
+
+	if err := l.Lock(info); err != nil {
+		return nil, err
+	}
+
+	return l.Unlock, nil
+}
+
 // errBackendInitRequired is the final error message shown when reinit
 // is required for some reason. The error message includes the reason.
 var errBackendInitRequired = errors.New(
@@ -1572,7 +1618,7 @@ Current Serial: %[2]d
 
 const inputBackendMigrateChange = `
 Would you like to copy the state from your prior backend %q to the
-newly configured backend %q? If you're reconfiguring the same backend,
+newly configured %q backend? If you're reconfiguring the same backend,
 answering "yes" or "no" shouldn't make a difference. Please answer exactly
 "yes" or "no".
 `

command/meta_backend_migrate.go

@@ -20,7 +20,21 @@ import (
 //
 // After migrating the state, the existing state in the first backend
 // remains untouched.
+//
+// This will attempt to lock both states for the migration.
 func (m *Meta) backendMigrateState(opts *backendMigrateOpts) error {
+	unlockOne, err := lockState(opts.One, "migrate from")
+	if err != nil {
+		return err
+	}
+	defer unlockOne()
+
+	unlockTwo, err := lockState(opts.Two, "migrate to")
+	if err != nil {
+		return err
+	}
+	defer unlockTwo()
+
 	one := opts.One.State()
 	two := opts.Two.State()
 

command/meta_backend_test.go

@@ -354,6 +354,7 @@ func TestMetaBackend_configureNewWithState(t *testing.T) {
 	if state == nil {
 		t.Fatal("state is nil")
 	}
+
 	if state.Lineage != "backend-new-migrate" {
 		t.Fatalf("bad: %#v", state)
 	}

state/local.go

@@ -97,6 +97,7 @@ func (s *LocalState) Unlock() error {
 	fileName := s.stateFileOut.Name()
 
 	unlockErr := s.unlock()
+
 	s.stateFileOut.Close()
 	s.stateFileOut = nil
 
@@ -201,6 +202,11 @@ func (s *LocalState) RefreshState() error {
 			reader = f
 		}
 	} else {
+		// no state to refresh
+		if s.stateFileOut == nil {
+			return nil
+		}
+
 		// we have a state file, make sure we're at the start
 		s.stateFileOut.Seek(0, os.SEEK_SET)
 		reader = s.stateFileOut

state/remote/s3.go

@@ -236,7 +236,7 @@ func (c *S3Client) Lock(info string) error {
 
 		resp, err := c.dynClient.GetItem(getParams)
 		if err != nil {
-			return fmt.Errorf("s3 state file %q locked, failed to retrive info: %s", stateName, err)
+			return fmt.Errorf("s3 state file %q locked, failed to retrieve info: %s", stateName, err)
 		}
 
 		var infoData string

terraform/state.go

@@ -1808,7 +1808,9 @@ var ErrNoState = errors.New("no state")
 // was written by WriteState.
 func ReadState(src io.Reader) (*State, error) {
 	buf := bufio.NewReader(src)
-	if _, err := buf.Peek(1); err == io.EOF {
+	if _, err := buf.Peek(1); err != nil {
+		// the error is either io.EOF or "invalid argument", and both are from
+		// an empty state.
 		return nil, ErrNoState
 	}
 

terraform/state_test.go

@@ -4,6 +4,7 @@ import (
 	"bytes"
 	"encoding/json"
 	"fmt"
+	"os"
 	"reflect"
 	"strings"
 	"testing"
@@ -1577,6 +1578,20 @@ func TestReadStateNewVersion(t *testing.T) {
 	}
 }
 
+func TestReadStateEmptyOrNilFile(t *testing.T) {
+	var emptyState bytes.Buffer
+	_, err := ReadState(&emptyState)
+	if err != ErrNoState {
+		t.Fatal("expected ErrNostate, got", err)
+	}
+
+	var nilFile *os.File
+	_, err = ReadState(nilFile)
+	if err != ErrNoState {
+		t.Fatal("expected ErrNostate, got", err)
+	}
+}
+
 func TestReadStateTFVersion(t *testing.T) {
 	type tfVersion struct {
 		Version   int    `json:"version"`