Merge pull request #6082 from hashicorp/b-vault-deadlock

vault: fix deadlock in SetConfig

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
@@ -332,6 +335,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()
@@ -343,12 +348,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
 	}

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()