Merge pull request #25837 from remilapeyre/consul-force-unlock

Fix `terraform force-unlock <lock_id>` for Consul backend

backend/remote-state/consul/client.go

@@ -366,6 +366,11 @@ func (c *RemoteClient) lock() (string, error) {
 	// store the session ID for correlation with consul logs
 	c.info.Info = "consul session: " + lockSession
 
+	// A random lock ID has been generated but we override it with the session
+	// ID as this will make it easier to manually invalidate the session
+	// if needed.
+	c.info.ID = lockSession
+
 	opts := &consulapi.LockOptions{
 		Key:     c.lockPath() + lockSuffix,
 		Session: lockSession,
@@ -524,8 +529,25 @@ func (c *RemoteClient) Unlock(id string) error {
 // the unlock implementation.
 // Only to be called while holding Client.mu
 func (c *RemoteClient) unlock(id string) error {
-	// this doesn't use the lock id, because the lock is tied to the consul client.
+	// This method can be called in two circumstances:
+	// - when the plan apply or destroy operation finishes and the lock needs to be released,
+	// the watchdog stopped and the session closed
+	// - when the user calls `terraform force-unlock <lock_id>` in which case
+	// we only need to release the lock.
+
 	if c.consulLock == nil || c.lockCh == nil {
+		// The user called `terraform force-unlock <lock_id>`, we just destroy
+		// the session which will release the lock, clean the KV store and quit.
+
+		_, err := c.Client.Session().Destroy(id, nil)
+		if err != nil {
+			return err
+		}
+		// We ignore the errors that may happen during cleanup
+		kv := c.Client.KV()
+		kv.Delete(c.lockPath()+lockSuffix, nil)
+		kv.Delete(c.lockPath()+lockInfoSuffix, nil)
+
 		return nil
 	}
 

backend/remote-state/consul/client_test.go

@@ -255,6 +255,17 @@ func TestConsul_destroyLock(t *testing.T) {
 		fmt.Sprintf("tf-unit/%s/", time.Now().String()),
 	}
 
+	testLock := func(client *RemoteClient, lockPath string) {
+		// get the lock val
+		pair, _, err := client.Client.KV().Get(lockPath, nil)
+		if err != nil {
+			t.Fatal(err)
+		}
+		if pair != nil {
+			t.Fatalf("lock key not cleaned up at: %s", pair.Key)
+		}
+	}
+
 	for _, path := range testCases {
 		t.Run(path, func(*testing.T) {
 			// Get the backend
@@ -269,27 +280,50 @@ func TestConsul_destroyLock(t *testing.T) {
 				t.Fatalf("err: %s", err)
 			}
 
-			c := s.(*remote.State).Client.(*RemoteClient)
+			clientA := s.(*remote.State).Client.(*RemoteClient)
 
 			info := statemgr.NewLockInfo()
-			id, err := c.Lock(info)
+			id, err := clientA.Lock(info)
 			if err != nil {
 				t.Fatal(err)
 			}
 
-			lockPath := c.Path + lockSuffix
+			lockPath := clientA.Path + lockSuffix
 
-			if err := c.Unlock(id); err != nil {
+			if err := clientA.Unlock(id); err != nil {
 				t.Fatal(err)
 			}
 
-			// get the lock val
+			testLock(clientA, lockPath)
-			pair, _, err := c.Client.KV().Get(lockPath, nil)
+
+			// The release the lock from a second client to test the
+			// `terraform force-unlock <lock_id>` functionnality
+			s, err = b.StateMgr(backend.DefaultStateName)
+			if err != nil {
+				t.Fatalf("err: %s", err)
+			}
+
+			clientB := s.(*remote.State).Client.(*RemoteClient)
+
+			info = statemgr.NewLockInfo()
+			id, err = clientA.Lock(info)
 			if err != nil {
 				t.Fatal(err)
 			}
-			if pair != nil {
+
-				t.Fatalf("lock key not cleaned up at: %s", pair.Key)
+			if err := clientB.Unlock(id); err != nil {
+				t.Fatal(err)
+			}
+
+			testLock(clientA, lockPath)
+
+			err = clientA.Unlock(id)
+
+			if err == nil {
+				t.Fatal("consul lock should have been lost")
+			}
+			if err.Error() != "consul lock was lost" {
+				t.Fatal("got wrong error", err)
 			}
 		})
 	}