Merge pull request #25842 from remilapeyre/consul-path-slash

Sanitize lock path for the Consul backend when it ends with a /

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