diff --git a/nomad/vault.go b/nomad/vault.go index cbe1ac273d0..44a61973e09 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 @@ -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 } 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()