From 11eb88753d64f99936ee00547fca29d6bbbab512 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9mi=20Lapeyre?= Date: Thu, 13 Aug 2020 15:15:46 +0200 Subject: [PATCH] Fix `terraform force-unlock ` for Consul backend When locking was enabled with the Consul backend and the lock not properly released, the `terraform force-unlock ` 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 | 5 +++ backend/remote-state/consul/client.go | 24 +++++++++- backend/remote-state/consul/client_test.go | 50 +++++++++++++++++---- 3 files changed, 70 insertions(+), 9 deletions(-) diff --git a/backend/remote-state/consul/backend_test.go b/backend/remote-state/consul/backend_test.go index 6aa43349ab..394bf428e5 100644 --- a/backend/remote-state/consul/backend_test.go +++ b/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 diff --git a/backend/remote-state/consul/client.go b/backend/remote-state/consul/client.go index 6007761eec..b9d2412ea0 100644 --- a/backend/remote-state/consul/client.go +++ b/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 ` in which case + // we only need to release the lock. + if c.consulLock == nil || c.lockCh == nil { + // The user called `terraform force-unlock `, 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 } diff --git a/backend/remote-state/consul/client_test.go b/backend/remote-state/consul/client_test.go index 9b5d2a53f0..3e4c6b0c2a 100644 --- a/backend/remote-state/consul/client_test.go +++ b/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 - pair, _, err := c.Client.KV().Get(lockPath, nil) + testLock(clientA, lockPath) + + // The release the lock from a second client to test the + // `terraform force-unlock ` 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) } }