From 032d339915a85e19d8bc288c91060f9891449e32 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9mi=20Lapeyre?= Date: Thu, 13 Aug 2020 16:29:43 +0200 Subject: [PATCH] Sanitize lock path for the Consul backend when it ends with a / When the path ends with / (e.g. `path = "tfstate/"), the lock path used will contain two consecutive slashes (e.g. `tfstate//.lock`) which Consul does not accept. This change the lock path so it is sanitized to `tfstate/.lock`. If the user has two different Terraform project, one with `path = "tfstate"` and the other with `path = "tfstate/"`, the paths for the locks will be the same which will be confusing as locking one project will lock both. I wish it were possible to forbid ending slashes altogether but doing so would require all users currently having an ending slash in the path to manually move their Terraform state and would be a poor user experience. Closes https://github.com/hashicorp/terraform/issues/15747 --- backend/remote-state/consul/backend_test.go | 5 + backend/remote-state/consul/client.go | 15 ++- backend/remote-state/consul/client_test.go | 137 ++++++++++++-------- 3 files changed, 97 insertions(+), 60 deletions(-) diff --git a/backend/remote-state/consul/backend_test.go b/backend/remote-state/consul/backend_test.go index 6aa43349a..394bf428e 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 6007761ee..43dbdce5a 100644 --- a/backend/remote-state/consul/client.go +++ b/backend/remote-state/consul/client.go @@ -9,6 +9,7 @@ import ( "errors" "fmt" "log" + "strings" "sync" "time" @@ -161,13 +162,19 @@ func (c *RemoteClient) Delete() error { return err } +func (c *RemoteClient) lockPath() string { + // we sanitize the path for the lock as Consul does not like having + // two consecutive slashes for the lock path + return strings.TrimRight(c.Path, "/") +} + func (c *RemoteClient) putLockInfo(info *statemgr.LockInfo) error { info.Path = c.Path info.Created = time.Now().UTC() kv := c.Client.KV() _, err := kv.Put(&consulapi.KVPair{ - Key: c.Path + lockInfoSuffix, + Key: c.lockPath() + lockInfoSuffix, Value: info.Marshal(), }, nil) @@ -175,7 +182,7 @@ func (c *RemoteClient) putLockInfo(info *statemgr.LockInfo) error { } func (c *RemoteClient) getLockInfo() (*statemgr.LockInfo, error) { - path := c.Path + lockInfoSuffix + path := c.lockPath() + lockInfoSuffix pair, _, err := c.Client.KV().Get(path, nil) if err != nil { return nil, err @@ -234,7 +241,7 @@ func (c *RemoteClient) lock() (string, error) { c.info.Info = "consul session: " + lockSession opts := &consulapi.LockOptions{ - Key: c.Path + lockSuffix, + Key: c.lockPath() + lockSuffix, Session: lockSession, // only wait briefly, so terraform has the choice to fail fast or @@ -419,7 +426,7 @@ func (c *RemoteClient) unlock(id string) error { var errs error - if _, err := kv.Delete(c.Path+lockInfoSuffix, nil); err != nil { + if _, err := kv.Delete(c.lockPath()+lockInfoSuffix, nil); err != nil { errs = multierror.Append(errs, err) } diff --git a/backend/remote-state/consul/client_test.go b/backend/remote-state/consul/client_test.go index 9b5d2a53f..ff38f0120 100644 --- a/backend/remote-state/consul/client_test.go +++ b/backend/remote-state/consul/client_test.go @@ -19,20 +19,29 @@ func TestRemoteClient_impl(t *testing.T) { } func TestRemoteClient(t *testing.T) { - // Get the backend - b := backend.TestBackendConfig(t, New(), backend.TestWrapConfig(map[string]interface{}{ - "address": srv.HTTPAddr, - "path": fmt.Sprintf("tf-unit/%s", time.Now().String()), - })) - - // Grab the client - state, err := b.StateMgr(backend.DefaultStateName) - if err != nil { - t.Fatalf("err: %s", err) + testCases := []string{ + fmt.Sprintf("tf-unit/%s", time.Now().String()), + fmt.Sprintf("tf-unit/%s/", time.Now().String()), } - // Test - remote.TestClient(t, state.(*remote.State).Client) + for _, path := range testCases { + t.Run(path, func(*testing.T) { + // Get the backend + b := backend.TestBackendConfig(t, New(), backend.TestWrapConfig(map[string]interface{}{ + "address": srv.HTTPAddr, + "path": path, + })) + + // Grab the client + state, err := b.StateMgr(backend.DefaultStateName) + if err != nil { + t.Fatalf("err: %s", err) + } + + // Test + remote.TestClient(t, state.(*remote.State).Client) + }) + } } // test the gzip functionality of the client @@ -72,62 +81,78 @@ func TestRemoteClient_gzipUpgrade(t *testing.T) { } func TestConsul_stateLock(t *testing.T) { - path := fmt.Sprintf("tf-unit/%s", time.Now().String()) - - // create 2 instances to get 2 remote.Clients - sA, err := backend.TestBackendConfig(t, New(), backend.TestWrapConfig(map[string]interface{}{ - "address": srv.HTTPAddr, - "path": path, - })).StateMgr(backend.DefaultStateName) - if err != nil { - t.Fatal(err) + testCases := []string{ + fmt.Sprintf("tf-unit/%s", time.Now().String()), + fmt.Sprintf("tf-unit/%s/", time.Now().String()), } - sB, err := backend.TestBackendConfig(t, New(), backend.TestWrapConfig(map[string]interface{}{ - "address": srv.HTTPAddr, - "path": path, - })).StateMgr(backend.DefaultStateName) - if err != nil { - t.Fatal(err) - } + for _, path := range testCases { + t.Run(path, func(*testing.T) { + // create 2 instances to get 2 remote.Clients + sA, err := backend.TestBackendConfig(t, New(), backend.TestWrapConfig(map[string]interface{}{ + "address": srv.HTTPAddr, + "path": path, + })).StateMgr(backend.DefaultStateName) + if err != nil { + t.Fatal(err) + } - remote.TestRemoteLocks(t, sA.(*remote.State).Client, sB.(*remote.State).Client) + sB, err := backend.TestBackendConfig(t, New(), backend.TestWrapConfig(map[string]interface{}{ + "address": srv.HTTPAddr, + "path": path, + })).StateMgr(backend.DefaultStateName) + if err != nil { + t.Fatal(err) + } + + remote.TestRemoteLocks(t, sA.(*remote.State).Client, sB.(*remote.State).Client) + }) + } } func TestConsul_destroyLock(t *testing.T) { - // Get the backend - b := backend.TestBackendConfig(t, New(), backend.TestWrapConfig(map[string]interface{}{ - "address": srv.HTTPAddr, - "path": fmt.Sprintf("tf-unit/%s", time.Now().String()), - })) - - // Grab the client - s, err := b.StateMgr(backend.DefaultStateName) - if err != nil { - t.Fatalf("err: %s", err) + testCases := []string{ + fmt.Sprintf("tf-unit/%s", time.Now().String()), + fmt.Sprintf("tf-unit/%s/", time.Now().String()), } - c := s.(*remote.State).Client.(*RemoteClient) + for _, path := range testCases { + t.Run(path, func(*testing.T) { + // Get the backend + b := backend.TestBackendConfig(t, New(), backend.TestWrapConfig(map[string]interface{}{ + "address": srv.HTTPAddr, + "path": path, + })) - info := statemgr.NewLockInfo() - id, err := c.Lock(info) - if err != nil { - t.Fatal(err) - } + // Grab the client + s, err := b.StateMgr(backend.DefaultStateName) + if err != nil { + t.Fatalf("err: %s", err) + } - lockPath := c.Path + lockSuffix + c := s.(*remote.State).Client.(*RemoteClient) - if err := c.Unlock(id); err != nil { - t.Fatal(err) - } + info := statemgr.NewLockInfo() + id, err := c.Lock(info) + if err != nil { + t.Fatal(err) + } - // get the lock val - pair, _, err := c.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) + lockPath := c.Path + lockSuffix + + if err := c.Unlock(id); err != nil { + t.Fatal(err) + } + + // get the lock val + pair, _, err := c.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) + } + }) } }