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

When locking was enabled with the Consul backend and the lock not properly
released, the `terraform force-unlock <lock_id>` command would do nothing as
its implementation would exit early in that case.

It now destroys the session that created the lock and clean both the lock and
the lock-info keys.

A regression test is added to TestConsul_destroyLock() to catch the issue if it
happends again.

Closes https://github.com/hashicorp/terraform/issues/22174

backend/remote-state/consul/backend_test.go

@@ -1,6 +1,7 @@
 package consul
 
 import (
+	"flag"
 	"fmt"
 	"io/ioutil"
 	"os"
@@ -39,6 +40,10 @@ func newConsulTestServer() (*testutil.TestServer, error) {
 	srv, err := testutil.NewTestServerConfig(func(c *testutil.TestServerConfig) {
 		c.LogLevel = "warn"
 
+		if !flag.Parsed() {
+			flag.Parse()
+		}
+
 		if !testing.Verbose() {
 			c.Stdout = ioutil.Discard
 			c.Stderr = ioutil.Discard

backend/remote-state/consul/client.go

@@ -233,6 +233,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.Path + lockSuffix,
 		Session: lockSession,
@@ -391,8 +396,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.Path+lockSuffix, nil)
+		kv.Delete(c.Path+lockInfoSuffix, nil)
+
 		return nil
 	}
 

backend/remote-state/consul/client_test.go

@@ -95,6 +95,17 @@ func TestConsul_stateLock(t *testing.T) {
 }
 
 func TestConsul_destroyLock(t *testing.T) {
+	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)
+		}
+	}
+
 	// Get the backend
 	b := backend.TestBackendConfig(t, New(), backend.TestWrapConfig(map[string]interface{}{
 		"address": srv.HTTPAddr,
@@ -107,27 +118,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)
 	}
 }