Merge pull request #14680 from hashicorp/jbardin/unlock-s3

Check that a named state in s3 doesn't exist before acquiring a lock

backend/remote-state/s3/backend_state.go

@@ -101,9 +101,29 @@ func (b *Backend) State(name string) (state.State, error) {
 
 	stateMgr := &remote.State{Client: client}
 
-	//if this isn't the default state name, we need to create the object so
+	// Check to see if this state already exists.
-	//it's listed by States.
+	// If we're trying to force-unlock a state, we can't take the lock before
-	if name != backend.DefaultStateName {
+	// fetching the state. If the state doesn't exist, we have to assume this
+	// is a normal create operation, and take the lock at that point.
+	//
+	// If we need to force-unlock, but for some reason the state no longer
+	// exists, the user will have to use aws tools to manually fix the
+	// situation.
+	existing, err := b.States()
+	if err != nil {
+		return nil, err
+	}
+
+	exists := false
+	for _, s := range existing {
+		if s == name {
+			exists = true
+			break
+		}
+	}
+
+	// We need to create the object so it's listed by States.
+	if !exists {
 		// take a lock on this state while we write it
 		lockInfo := state.NewLockInfo()
 		lockInfo.Operation = "init"
@@ -121,6 +141,8 @@ func (b *Backend) State(name string) (state.State, error) {
 		}
 
 		// Grab the value
+		// This is to ensure that no one beat us to writing a state between
+		// the `exists` check and taking the lock.
 		if err := stateMgr.RefreshState(); err != nil {
 			err = lockUnlock(err)
 			return nil, err

backend/remote-state/s3/client_test.go

@@ -6,6 +6,7 @@ import (
 	"time"
 
 	"github.com/hashicorp/terraform/backend"
+	"github.com/hashicorp/terraform/state"
 	"github.com/hashicorp/terraform/state/remote"
 )
 
@@ -16,7 +17,6 @@ func TestRemoteClient_impl(t *testing.T) {
 
 func TestRemoteClient(t *testing.T) {
 	testACC(t)
-
 	bucketName := fmt.Sprintf("terraform-remote-s3-test-%x", time.Now().Unix())
 	keyName := "testState"
 
@@ -26,20 +26,19 @@ func TestRemoteClient(t *testing.T) {
 		"encrypt": true,
 	}).(*Backend)
 
+	createS3Bucket(t, b.s3Client, bucketName)
+	defer deleteS3Bucket(t, b.s3Client, bucketName)
+
 	state, err := b.State(backend.DefaultStateName)
 	if err != nil {
 		t.Fatal(err)
 	}
 
-	createS3Bucket(t, b.s3Client, bucketName)
-	defer deleteS3Bucket(t, b.s3Client, bucketName)
-
 	remote.TestClient(t, state.(*remote.State).Client)
 }
 
 func TestRemoteClientLocks(t *testing.T) {
 	testACC(t)
-
 	bucketName := fmt.Sprintf("terraform-remote-s3-test-%x", time.Now().Unix())
 	keyName := "testState"
 
@@ -57,6 +56,11 @@ func TestRemoteClientLocks(t *testing.T) {
 		"lock_table": bucketName,
 	}).(*Backend)
 
+	createS3Bucket(t, b1.s3Client, bucketName)
+	defer deleteS3Bucket(t, b1.s3Client, bucketName)
+	createDynamoDBTable(t, b1.dynClient, bucketName)
+	defer deleteDynamoDBTable(t, b1.dynClient, bucketName)
+
 	s1, err := b1.State(backend.DefaultStateName)
 	if err != nil {
 		t.Fatal(err)
@@ -67,10 +71,82 @@ func TestRemoteClientLocks(t *testing.T) {
 		t.Fatal(err)
 	}
 
+	remote.TestRemoteLocks(t, s1.(*remote.State).Client, s2.(*remote.State).Client)
+}
+
+// verify that we can unlock a state with an existing lock
+func TestForceUnlock(t *testing.T) {
+	testACC(t)
+	bucketName := fmt.Sprintf("terraform-remote-s3-test-force-%x", time.Now().Unix())
+	keyName := "testState"
+
+	b1 := backend.TestBackendConfig(t, New(), map[string]interface{}{
+		"bucket":     bucketName,
+		"key":        keyName,
+		"encrypt":    true,
+		"lock_table": bucketName,
+	}).(*Backend)
+
+	b2 := backend.TestBackendConfig(t, New(), map[string]interface{}{
+		"bucket":     bucketName,
+		"key":        keyName,
+		"encrypt":    true,
+		"lock_table": bucketName,
+	}).(*Backend)
+
 	createS3Bucket(t, b1.s3Client, bucketName)
 	defer deleteS3Bucket(t, b1.s3Client, bucketName)
 	createDynamoDBTable(t, b1.dynClient, bucketName)
 	defer deleteDynamoDBTable(t, b1.dynClient, bucketName)
 
-	remote.TestRemoteLocks(t, s1.(*remote.State).Client, s2.(*remote.State).Client)
+	// first test with default
+	s1, err := b1.State(backend.DefaultStateName)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	info := state.NewLockInfo()
+	info.Operation = "test"
+	info.Who = "clientA"
+
+	lockID, err := s1.Lock(info)
+	if err != nil {
+		t.Fatal("unable to get initial lock:", err)
+	}
+
+	// s1 is now locked, get the same state through s2 and unlock it
+	s2, err := b2.State(backend.DefaultStateName)
+	if err != nil {
+		t.Fatal("failed to get default state to force unlock:", err)
+	}
+
+	if err := s2.Unlock(lockID); err != nil {
+		t.Fatal("failed to force-unlock default state")
+	}
+
+	// now try the same thing with a named state
+	// first test with default
+	s1, err = b1.State("test")
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	info = state.NewLockInfo()
+	info.Operation = "test"
+	info.Who = "clientA"
+
+	lockID, err = s1.Lock(info)
+	if err != nil {
+		t.Fatal("unable to get initial lock:", err)
+	}
+
+	// s1 is now locked, get the same state through s2 and unlock it
+	s2, err = b2.State("test")
+	if err != nil {
+		t.Fatal("failed to get named state to force unlock:", err)
+	}
+
+	if err = s2.Unlock(lockID); err != nil {
+		t.Fatal("failed to force-unlock named state")
+	}
 }