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

@@ -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

@@ -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)
 	}
 

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
+	testCases := []string{
-	b := backend.TestBackendConfig(t, New(), backend.TestWrapConfig(map[string]interface{}{
+		fmt.Sprintf("tf-unit/%s", time.Now().String()),
-		"address": srv.HTTPAddr,
+		fmt.Sprintf("tf-unit/%s/", time.Now().String()),
-		"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)
 	}
 
-	// Test
+	for _, path := range testCases {
-	remote.TestClient(t, state.(*remote.State).Client)
+		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())
+	testCases := []string{
-
+		fmt.Sprintf("tf-unit/%s", time.Now().String()),
-	// create 2 instances to get 2 remote.Clients
+		fmt.Sprintf("tf-unit/%s/", time.Now().String()),
-	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)
 	}
 
-	sB, err := backend.TestBackendConfig(t, New(), backend.TestWrapConfig(map[string]interface{}{
+	for _, path := range testCases {
-		"address": srv.HTTPAddr,
+		t.Run(path, func(*testing.T) {
-		"path":    path,
+			// create 2 instances to get 2 remote.Clients
-	})).StateMgr(backend.DefaultStateName)
+			sA, err := backend.TestBackendConfig(t, New(), backend.TestWrapConfig(map[string]interface{}{
-	if err != nil {
+				"address": srv.HTTPAddr,
-		t.Fatal(err)
+				"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
+	testCases := []string{
-	b := backend.TestBackendConfig(t, New(), backend.TestWrapConfig(map[string]interface{}{
+		fmt.Sprintf("tf-unit/%s", time.Now().String()),
-		"address": srv.HTTPAddr,
+		fmt.Sprintf("tf-unit/%s/", time.Now().String()),
-		"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)
 	}
 
-	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()
+			// Grab the client
-	id, err := c.Lock(info)
+			s, err := b.StateMgr(backend.DefaultStateName)
-	if err != nil {
+			if err != nil {
-		t.Fatal(err)
+				t.Fatalf("err: %s", err)
-	}
+			}
 
-	lockPath := c.Path + lockSuffix
+			c := s.(*remote.State).Client.(*RemoteClient)
 
-	if err := c.Unlock(id); err != nil {
+			info := statemgr.NewLockInfo()
-		t.Fatal(err)
+			id, err := c.Lock(info)
-	}
+			if err != nil {
+				t.Fatal(err)
+			}
 
-	// get the lock val
+			lockPath := c.Path + lockSuffix
-	pair, _, err := c.Client.KV().Get(lockPath, nil)
+
-	if err != nil {
+			if err := c.Unlock(id); err != nil {
-		t.Fatal(err)
+				t.Fatal(err)
-	}
+			}
-	if pair != nil {
+
-		t.Fatalf("lock key not cleaned up at: %s", pair.Key)
+			// 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)
+			}
+		})
 	}
 }