Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

vault: fix deadlock in SetConfig #6082

Merged
merged 2 commits into from
Aug 6, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 14 additions & 4 deletions nomad/vault.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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()
Expand All @@ -341,12 +346,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
}
Expand Down
32 changes: 32 additions & 0 deletions nomad/vault_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down