diff --git a/CHANGELOG.md b/CHANGELOG.md index 38e27f80ea14..40f17ff6cb50 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -42,6 +42,7 @@ IMPROVEMENTS: BUG FIXES: * core: Clean up leaked deployments on restoration [[GH-4329](https://github.com/hashicorp/nomad/issues/4329)] + * core: Fix regression to allow for dynamic Vault configuration reload [[GH-4395](https://github.com/hashicorp/nomad/issues/4395)] * core: Fix bug where older failed allocations of jobs that have been updated to a newer version were not being garbage collected [[GH-4313](https://github.com/hashicorp/nomad/issues/4313)] * core: Fix bug when upgrading an existing server to Raft protocol 3 that diff --git a/command/agent/command.go b/command/agent/command.go index 9b9fb9ad4364..1fa22734cf65 100644 --- a/command/agent/command.go +++ b/command/agent/command.go @@ -740,20 +740,21 @@ func (c *Command) handleReload() { } } - if shouldReloadRPC { - if s := c.agent.Server(); s != nil { - sconf, err := convertServerConfig(newConf, c.logOutput) - c.agent.logger.Printf("[DEBUG] agent: starting reload of server config") - if err != nil { - c.agent.logger.Printf("[ERR] agent: failed to convert server config: %v", err) + c.agent.logger.Printf("[DEBUG] agent: starting reload of server config") + if s := c.agent.Server(); s != nil { + sconf, err := convertServerConfig(newConf, c.logOutput) + if err != nil { + c.agent.logger.Printf("[ERR] agent: failed to convert server config: %v", err) + return + } else { + if err := s.Reload(sconf); err != nil { + c.agent.logger.Printf("[ERR] agent: reloading server config failed: %v", err) return - } else { - if err := s.Reload(sconf); err != nil { - c.agent.logger.Printf("[ERR] agent: reloading server config failed: %v", err) - return - } } } + } + + if shouldReloadRPC { if s := c.agent.Client(); s != nil { clientConfig, err := c.agent.clientConfig() diff --git a/nomad/server_test.go b/nomad/server_test.go index ef1439047599..30633b9e87b4 100644 --- a/nomad/server_test.go +++ b/nomad/server_test.go @@ -219,7 +219,7 @@ func TestServer_Reload_Vault(t *testing.T) { } tr := true - config := s1.config + config := DefaultConfig() config.VaultConfig.Enabled = &tr config.VaultConfig.Token = uuid.Generate() diff --git a/nomad/structs/config/vault.go b/nomad/structs/config/vault.go index 42115c1a9132..bfb515623f54 100644 --- a/nomad/structs/config/vault.go +++ b/nomad/structs/config/vault.go @@ -181,3 +181,55 @@ func (c *VaultConfig) Copy() *VaultConfig { *nc = *c return nc } + +// IsEqual compares two Vault configurations and returns a boolean indicating +// if they are equal. +func (a *VaultConfig) IsEqual(b *VaultConfig) bool { + if a == nil && b != nil { + return false + } + if a != nil && b == nil { + return false + } + + if a.Token != b.Token { + return false + } + if a.Role != b.Role { + return false + } + if a.TaskTokenTTL != b.TaskTokenTTL { + return false + } + if a.Addr != b.Addr { + return false + } + if a.ConnectionRetryIntv.Nanoseconds() != b.ConnectionRetryIntv.Nanoseconds() { + return false + } + if a.TLSCaFile != b.TLSCaFile { + return false + } + if a.TLSCaPath != b.TLSCaPath { + return false + } + if a.TLSCertFile != b.TLSCertFile { + return false + } + if a.TLSKeyFile != b.TLSKeyFile { + return false + } + if a.TLSServerName != b.TLSServerName { + return false + } + if a.AllowUnauthenticated != b.AllowUnauthenticated { + return false + } + if a.TLSSkipVerify != b.TLSSkipVerify { + return false + } + if a.Enabled != b.Enabled { + return false + } + return true +} diff --git a/nomad/structs/config/vault_test.go b/nomad/structs/config/vault_test.go index f92f1dde75cc..e48b6ef02782 100644 --- a/nomad/structs/config/vault_test.go +++ b/nomad/structs/config/vault_test.go @@ -3,6 +3,8 @@ package config import ( "reflect" "testing" + + "github.com/stretchr/testify/require" ) func TestVaultConfig_Merge(t *testing.T) { @@ -57,3 +59,71 @@ func TestVaultConfig_Merge(t *testing.T) { t.Fatalf("bad:\n%#v\n%#v", result, e) } } + +func TestVaultConfig_IsEqual(t *testing.T) { + require := require.New(t) + + trueValue, falseValue := true, false + c1 := &VaultConfig{ + Enabled: &falseValue, + Token: "1", + Role: "1", + AllowUnauthenticated: &trueValue, + TaskTokenTTL: "1", + Addr: "1", + TLSCaFile: "1", + TLSCaPath: "1", + TLSCertFile: "1", + TLSKeyFile: "1", + TLSSkipVerify: &trueValue, + TLSServerName: "1", + } + + c2 := &VaultConfig{ + Enabled: &falseValue, + Token: "1", + Role: "1", + AllowUnauthenticated: &trueValue, + TaskTokenTTL: "1", + Addr: "1", + TLSCaFile: "1", + TLSCaPath: "1", + TLSCertFile: "1", + TLSKeyFile: "1", + TLSSkipVerify: &trueValue, + TLSServerName: "1", + } + + require.True(c1.IsEqual(c2)) + + c3 := &VaultConfig{ + Enabled: &trueValue, + Token: "1", + Role: "1", + AllowUnauthenticated: &trueValue, + TaskTokenTTL: "1", + Addr: "1", + TLSCaFile: "1", + TLSCaPath: "1", + TLSCertFile: "1", + TLSKeyFile: "1", + TLSSkipVerify: &trueValue, + TLSServerName: "1", + } + + c4 := &VaultConfig{ + Enabled: &falseValue, + Token: "1", + Role: "1", + AllowUnauthenticated: &trueValue, + TaskTokenTTL: "1", + Addr: "1", + TLSCaFile: "1", + TLSCaPath: "1", + TLSCertFile: "1", + TLSKeyFile: "1", + TLSSkipVerify: &trueValue, + TLSServerName: "1", + } + require.False(c3.IsEqual(c4)) +} diff --git a/nomad/vault.go b/nomad/vault.go index ca1486ebf93e..fc60075fb097 100644 --- a/nomad/vault.go +++ b/nomad/vault.go @@ -316,6 +316,11 @@ func (v *vaultClient) SetConfig(config *config.VaultConfig) error { v.l.Lock() defer v.l.Unlock() + // If reloading the same config, no-op + if v.config.IsEqual(config) { + return nil + } + // Kill any background routines if v.running { // Stop accepting any new request diff --git a/nomad/vault_test.go b/nomad/vault_test.go index 687395a0dc20..5ba75f4da8d0 100644 --- a/nomad/vault_test.go +++ b/nomad/vault_test.go @@ -437,6 +437,31 @@ func TestVaultClient_SetConfig(t *testing.T) { if client.tokenData == nil || len(client.tokenData.Policies) != 3 { t.Fatalf("unexpected token: %v", client.tokenData) } + + // Test that when SetConfig is called with the same configuration, it is a + // no-op + failCh := make(chan struct{}, 1) + go func() { + tomb := client.tomb + select { + case <-tomb.Dying(): + close(failCh) + case <-time.After(1 * time.Second): + return + } + }() + + // Update the config + if err := client.SetConfig(v2.Config); err != nil { + t.Fatalf("SetConfig failed: %v", err) + } + + select { + case <-failCh: + t.Fatalf("Tomb shouldn't have exited") + case <-time.After(1 * time.Second): + return + } } // Test that we can disable vault