From 5621d97925a8673825f0517eeb83f3a919d74189 Mon Sep 17 00:00:00 2001
From: James Bardin <j.bardin@gmail.com>
Date: Thu, 6 Apr 2017 14:04:50 -0400
Subject: [PATCH] cleanup consul lock entries

This matches the consul cli behavior, where locks are cleaned up after
use.

Return an error from re-locking the state. This isn't required by the
Locker interface, but it's an added sanity check for state operations.
What was incorrect here was returning an empty ID and error, which would
indicate that Lock/Unlock isn't supported.
---
 backend/remote-state/consul/client.go      | 11 +++---
 backend/remote-state/consul/client_test.go | 41 ++++++++++++++++++++++
 2 files changed, 48 insertions(+), 4 deletions(-)

diff --git a/backend/remote-state/consul/client.go b/backend/remote-state/consul/client.go
index cd5971163..b11f31ba1 100644
--- a/backend/remote-state/consul/client.go
+++ b/backend/remote-state/consul/client.go
@@ -121,16 +121,15 @@ func (c *RemoteClient) Lock(info *state.LockInfo) (string, error) {
 	default:
 		if c.lockCh != nil {
 			// we have an active lock already
-			return "", nil
+			return "", fmt.Errorf("state %q already locked", c.Path)
 		}
 	}
 
 	if c.consulLock == nil {
 		opts := &consulapi.LockOptions{
 			Key: c.Path + lockSuffix,
-			// We currently don't procide any options to block terraform and
-			// retry lock acquisition, but we can wait briefly in case the
-			// lock is about to be freed.
+			// only wait briefly, so terraform has the choice to fail fast or
+			// retry as needed.
 			LockWaitTime: time.Second,
 			LockTryOnce:  true,
 		}
@@ -191,6 +190,10 @@ func (c *RemoteClient) Unlock(id string) error {
 	err := c.consulLock.Unlock()
 	c.lockCh = nil
 
+	// This is only cleanup, and will fail if the lock was immediately taken by
+	// another client, so we don't report an error to the user here.
+	c.consulLock.Destroy()
+
 	kv := c.Client.KV()
 	_, delErr := kv.Delete(c.Path+lockInfoSuffix, nil)
 	if delErr != nil {
diff --git a/backend/remote-state/consul/client_test.go b/backend/remote-state/consul/client_test.go
index 57b7c452e..dc7988135 100644
--- a/backend/remote-state/consul/client_test.go
+++ b/backend/remote-state/consul/client_test.go
@@ -6,6 +6,7 @@ import (
 	"time"
 
 	"github.com/hashicorp/terraform/backend"
+	"github.com/hashicorp/terraform/state"
 	"github.com/hashicorp/terraform/state/remote"
 )
 
@@ -98,3 +99,43 @@ func TestConsul_stateLock(t *testing.T) {
 
 	remote.TestRemoteLocks(t, sA.(*remote.State).Client, sB.(*remote.State).Client)
 }
+
+func TestConsul_destroyLock(t *testing.T) {
+	srv := newConsulTestServer(t)
+	defer srv.Stop()
+
+	// Get the backend
+	b := backend.TestBackendConfig(t, New(), map[string]interface{}{
+		"address": srv.HTTPAddr,
+		"path":    fmt.Sprintf("tf-unit/%s", time.Now().String()),
+	})
+
+	// Grab the client
+	s, err := b.State(backend.DefaultStateName)
+	if err != nil {
+		t.Fatalf("err: %s", err)
+	}
+
+	c := s.(*remote.State).Client.(*RemoteClient)
+
+	info := state.NewLockInfo()
+	id, err := c.Lock(info)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	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)
+	}
+}