Merge pull request #13222 from hashicorp/jbardin/init-locks

Fix some backend locking issues

backend/remote-state/s3/client.go

@@ -113,8 +113,7 @@ func (c *RemoteClient) Lock(info *state.LockInfo) (string, error) {
 		return "", nil
 	}
 
-	stateName := fmt.Sprintf("%s/%s", c.bucketName, c.path)
-	info.Path = stateName
+	info.Path = c.lockPath()
 
 	if info.ID == "" {
 		lockID, err := uuid.GenerateUUID()
@@ -127,7 +126,7 @@ func (c *RemoteClient) Lock(info *state.LockInfo) (string, error) {
 
 	putParams := &dynamodb.PutItemInput{
 		Item: map[string]*dynamodb.AttributeValue{
-			"LockID": {S: aws.String(stateName)},
+			"LockID": {S: aws.String(c.lockPath())},
 			"Info":   {S: aws.String(string(info.Marshal()))},
 		},
 		TableName:           aws.String(c.lockTable),
@@ -153,7 +152,7 @@ func (c *RemoteClient) Lock(info *state.LockInfo) (string, error) {
 func (c *RemoteClient) getLockInfo() (*state.LockInfo, error) {
 	getParams := &dynamodb.GetItemInput{
 		Key: map[string]*dynamodb.AttributeValue{
-			"LockID": {S: aws.String(fmt.Sprintf("%s/%s", c.bucketName, c.path))},
+			"LockID": {S: aws.String(c.lockPath())},
 		},
 		ProjectionExpression: aws.String("LockID, Info"),
 		TableName:            aws.String(c.lockTable),
@@ -202,7 +201,7 @@ func (c *RemoteClient) Unlock(id string) error {
 
 	params := &dynamodb.DeleteItemInput{
 		Key: map[string]*dynamodb.AttributeValue{
-			"LockID": {S: aws.String(fmt.Sprintf("%s/%s", c.bucketName, c.path))},
+			"LockID": {S: aws.String(c.lockPath())},
 		},
 		TableName: aws.String(c.lockTable),
 	}
@@ -214,3 +213,7 @@ func (c *RemoteClient) Unlock(id string) error {
 	}
 	return nil
 }
+
+func (c *RemoteClient) lockPath() string {
+	return fmt.Sprintf("%s/%s", c.bucketName, c.path)
+}

command/init.go

@@ -237,7 +237,6 @@ Options:
   -force-copy          Suppress prompts about copying state data. This is
                        equivalent to providing a "yes" to all confirmation
                        prompts.
-
 `
 	return strings.TrimSpace(helpText)
 }

command/meta.go

@@ -264,6 +264,10 @@ func (m *Meta) flagSet(n string) *flag.FlagSet {
 	// Set the default Usage to empty
 	f.Usage = func() {}
 
+	// command that bypass locking will supply their own flag on this var, but
+	// set the initial meta value to true as a failsafe.
+	m.stateLock = true
+
 	return f
 }
 

command/meta_backend_migrate.go

@@ -2,6 +2,7 @@ package command
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"io/ioutil"
 	"os"
@@ -53,7 +54,7 @@ func (m *Meta) backendMigrateState(opts *backendMigrateOpts) error {
 	// Setup defaults
 	opts.oneEnv = backend.DefaultStateName
 	opts.twoEnv = backend.DefaultStateName
-	opts.force = false
+	opts.force = m.forceInitCopy
 
 	// Determine migration behavior based on whether the source/destionation
 	// supports multi-state.
@@ -163,7 +164,7 @@ func (m *Meta) backendMigrateState_S_S(opts *backendMigrateOpts) error {
 func (m *Meta) backendMigrateState_S_s(opts *backendMigrateOpts) error {
 	currentEnv := m.Env()
 
-	migrate := m.forceInitCopy
+	migrate := opts.force
 	if !migrate {
 		var err error
 		// Ask the user if they want to migrate their existing remote state
@@ -218,6 +219,19 @@ func (m *Meta) backendMigrateState_s_s(opts *backendMigrateOpts) error {
 			errMigrateSingleLoadDefault), opts.TwoType, err)
 	}
 
+	// Check if we need migration at all.
+	// This is before taking a lock, because they may also correspond to the same lock.
+	one := stateOne.State()
+	two := stateTwo.State()
+
+	// no reason to migrate if the state is already there
+	if one.Equal(two) {
+		// Equal isn't identical; it doesn't check lineage.
+		if one != nil && two != nil && one.Lineage == two.Lineage {
+			return nil
+		}
+	}
+
 	if m.stateLock {
 		lockCtx, cancel := context.WithTimeout(context.Background(), m.stateLockTimeout)
 		defer cancel()
@@ -241,10 +255,21 @@ func (m *Meta) backendMigrateState_s_s(opts *backendMigrateOpts) error {
 			return fmt.Errorf("Error locking destination state: %s", err)
 		}
 		defer clistate.Unlock(stateTwo, lockIDTwo, m.Ui, m.Colorize())
-	}
 
-	one := stateOne.State()
-	two := stateTwo.State()
+		// We now own a lock, so double check that we have the version
+		// corresponding to the lock.
+		if err := stateOne.RefreshState(); err != nil {
+			return fmt.Errorf(strings.TrimSpace(
+				errMigrateSingleLoadDefault), opts.OneType, err)
+		}
+		if err := stateTwo.RefreshState(); err != nil {
+			return fmt.Errorf(strings.TrimSpace(
+				errMigrateSingleLoadDefault), opts.OneType, err)
+		}
+
+		one = stateOne.State()
+		two = stateTwo.State()
+	}
 
 	// Clear the legacy remote state in both cases. If we're at the migration
 	// step then this won't be used anymore.
@@ -281,6 +306,11 @@ func (m *Meta) backendMigrateState_s_s(opts *backendMigrateOpts) error {
 	}
 
 	if !opts.force {
+		// Abort if we can't ask for input.
+		if !m.input {
+			return errors.New("error asking for state migration action: inptut disabled")
+		}
+
 		// Confirm with the user whether we want to copy state over
 		confirm, err := confirmFunc(stateOne, stateTwo, opts)
 		if err != nil {
@@ -306,10 +336,6 @@ func (m *Meta) backendMigrateState_s_s(opts *backendMigrateOpts) error {
 }
 
 func (m *Meta) backendMigrateEmptyConfirm(one, two state.State, opts *backendMigrateOpts) (bool, error) {
-	if m.forceInitCopy {
-		return true, nil
-	}
-
 	inputOpts := &terraform.InputOpts{
 		Id: "backend-migrate-copy-to-empty",
 		Query: fmt.Sprintf(
@@ -372,10 +398,6 @@ func (m *Meta) backendMigrateNonEmptyConfirm(
 		return false, fmt.Errorf("Error saving temporary state: %s", err)
 	}
 
-	if m.forceInitCopy {
-		return true, nil
-	}
-
 	// Ask for confirmation
 	inputOpts := &terraform.InputOpts{
 		Id: "backend-migrate-to-backend",

command/meta_backend_test.go

@@ -426,6 +426,57 @@ func TestMetaBackend_configureNewWithState(t *testing.T) {
 	}
 }
 
+// Newly configured backend with matching local and remote state doesn't prompt
+// for copy.
+func TestMetaBackend_configureNewWithoutCopy(t *testing.T) {
+	// Create a temporary working directory that is empty
+	td := tempDir(t)
+	copy.CopyDir(testFixturePath("backend-new-migrate"), td)
+	defer os.RemoveAll(td)
+	defer testChdir(t, td)()
+
+	if err := copy.CopyFile(DefaultStateFilename, "local-state.tfstate"); err != nil {
+		t.Fatal(err)
+	}
+
+	// Setup the meta
+	m := testMetaBackend(t, nil)
+	m.input = false
+
+	// init the backend
+	_, err := m.Backend(&BackendOpts{Init: true})
+	if err != nil {
+		t.Fatalf("bad: %s", err)
+	}
+
+	// Verify the state is where we expect
+	f, err := os.Open("local-state.tfstate")
+	if err != nil {
+		t.Fatalf("err: %s", err)
+	}
+	actual, err := terraform.ReadState(f)
+	f.Close()
+	if err != nil {
+		t.Fatalf("err: %s", err)
+	}
+
+	if actual.Lineage != "backend-new-migrate" {
+		t.Fatalf("incorrect state lineage: %q", actual.Lineage)
+	}
+
+	// Verify the default paths don't exist
+	if !isEmptyState(DefaultStateFilename) {
+		data, _ := ioutil.ReadFile(DefaultStateFilename)
+
+		t.Fatal("state should not exist, but contains:\n", string(data))
+	}
+
+	// Verify a backup does exist
+	if isEmptyState(DefaultStateFilename + DefaultBackupExtension) {
+		t.Fatal("backup state is empty or missing")
+	}
+}
+
 // Newly configured backend with prior local state and no remote state,
 // but opting to not migrate.
 func TestMetaBackend_configureNewWithStateNoMigrate(t *testing.T) {