From 7e08a2fe4589026ea521e3aea5fa22f6186e73c4 Mon Sep 17 00:00:00 2001
From: Michael Schurter <mschurter@hashicorp.com>
Date: Tue, 6 Aug 2019 10:40:14 -0700
Subject: [PATCH 1/2] vault: fix deadlock in SetConfig

This seems to be the minimum viable patch for fixing a deadlock between
establishConnection and SetConfig.

SetConfig calls tomb.Kill+tomb.Wait while holding v.lock.
establishConnection needs to acquire v.lock to exit but SetConfig is
holding v.lock until tomb.Wait exits. tomb.Wait can't exit until
establishConnect does!

```
  SetConfig -> tomb.Wait
     ^              |
     |              v
  v.lock <- establishConnection
```
---
 nomad/vault.go      | 13 +++++++++----
 nomad/vault_test.go | 32 ++++++++++++++++++++++++++++++++
 2 files changed, 41 insertions(+), 4 deletions(-)

diff --git a/nomad/vault.go b/nomad/vault.go
index b3fe6efde98..209d0176a29 100644
--- a/nomad/vault.go
+++ b/nomad/vault.go
@@ -341,12 +341,17 @@ func (v *vaultClient) SetConfig(config *config.VaultConfig) error {
 
 	// Kill any background routines
 	if v.running {
-		// Stop accepting any new request
-		v.connEstablished = false
-
-		// Kill any background routine and create a new tomb
+		// Kill any background routine
 		v.tomb.Kill(nil)
+
+		// Locking around tomb.Wait can deadlock with
+		// establishConnection exiting, so we must unlock here.
+		v.l.Unlock()
 		v.tomb.Wait()
+		v.l.Lock()
+
+		// Stop accepting any new requests
+		v.connEstablished = false
 		v.tomb = &tomb.Tomb{}
 		v.running = false
 	}
diff --git a/nomad/vault_test.go b/nomad/vault_test.go
index e3f9b8692ef..0664ac26b8a 100644
--- a/nomad/vault_test.go
+++ b/nomad/vault_test.go
@@ -519,6 +519,38 @@ func TestVaultClient_SetConfig(t *testing.T) {
 	}
 }
 
+// TestVaultClient_SetConfig_Deadlock asserts that calling SetConfig
+// concurrently with establishConnection does not deadlock.
+func TestVaultClient_SetConfig_Deadlock(t *testing.T) {
+	t.Parallel()
+	v := testutil.NewTestVault(t)
+	defer v.Stop()
+
+	v2 := testutil.NewTestVault(t)
+	defer v2.Stop()
+
+	// Set the configs token in a new test role
+	v2.Config.Token = defaultTestVaultWhitelistRoleAndToken(v2, t, 20)
+
+	logger := testlog.HCLogger(t)
+	client, err := NewVaultClient(v.Config, logger, nil)
+	if err != nil {
+		t.Fatalf("failed to build vault client: %v", err)
+	}
+	defer client.Stop()
+
+	for i := 0; i < 100; i++ {
+		// Alternate configs to cause updates
+		conf := v.Config
+		if i%2 == 0 {
+			conf = v2.Config
+		}
+		if err := client.SetConfig(conf); err != nil {
+			t.Fatalf("SetConfig failed: %v", err)
+		}
+	}
+}
+
 // Test that we can disable vault
 func TestVaultClient_SetConfig_Disable(t *testing.T) {
 	t.Parallel()

From 21753013be2e29cf58f90113293d35b70a45d938 Mon Sep 17 00:00:00 2001
From: Michael Schurter <mschurter@hashicorp.com>
Date: Tue, 6 Aug 2019 11:17:10 -0700
Subject: [PATCH 2/2] vault: ensure SetConfig calls are serialized

This is a defensive measure as SetConfig should only be called serially.
---
 nomad/vault.go | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/nomad/vault.go b/nomad/vault.go
index 209d0176a29..186524065cc 100644
--- a/nomad/vault.go
+++ b/nomad/vault.go
@@ -233,6 +233,9 @@ type vaultClient struct {
 	// l is used to lock the configuration aspects of the client such that
 	// multiple callers can't cause conflicting config updates
 	l sync.Mutex
+
+	// setConfigLock serializes access to the SetConfig method
+	setConfigLock sync.Mutex
 }
 
 // NewVaultClient returns a Vault client from the given config. If the client
@@ -330,6 +333,8 @@ func (v *vaultClient) SetConfig(config *config.VaultConfig) error {
 	if config == nil {
 		return fmt.Errorf("must pass valid VaultConfig")
 	}
+	v.setConfigLock.Lock()
+	defer v.setConfigLock.Unlock()
 
 	v.l.Lock()
 	defer v.l.Unlock()