diff --git a/.changelog/14298.txt b/.changelog/14298.txt new file mode 100644 index 000000000000..1072f7bebf2d --- /dev/null +++ b/.changelog/14298.txt @@ -0,0 +1,7 @@ +```release-note:bug +vault: Fixed a bug where changing the Vault configuration `namespace` field was not detected as a change during server configuration reload. +``` + +```release-note:bug +vault: Fixed a bug where Vault clients were recreated when the server configuration was reloaded, even if there were no changes to the Vault configuration. +``` diff --git a/command/agent/agent_test.go b/command/agent/agent_test.go index 711b2aac20fc..315ff86d6aac 100644 --- a/command/agent/agent_test.go +++ b/command/agent/agent_test.go @@ -980,6 +980,39 @@ func TestServer_Reload_TLS_DowngradeFromTLS(t *testing.T) { assert.True(agentConfig.TLSConfig.IsEmpty()) } +func TestServer_Reload_VaultConfig(t *testing.T) { + ci.Parallel(t) + + agent := NewTestAgent(t, t.Name(), func(c *Config) { + c.Server.NumSchedulers = pointer.Of(0) + c.Vault = &config.VaultConfig{ + Enabled: pointer.Of(true), + Token: "vault-token", + Namespace: "vault-namespace", + Addr: "https://vault.consul:8200", + } + }) + defer agent.Shutdown() + + newConfig := agent.GetConfig().Copy() + newConfig.Vault = &config.VaultConfig{ + Enabled: pointer.Of(true), + Token: "vault-token", + Namespace: "another-namespace", + Addr: "https://vault.consul:8200", + } + + sconf, err := convertServerConfig(newConfig) + must.NoError(t, err) + agent.finalizeServerConfig(sconf) + + // TODO: the vault client isn't accessible here, and we don't actually + // overwrite the agent's server config on reload. We probably should? See + // tests in nomad/server_test.go for verification of this code path's + // behavior on the VaultClient + must.NoError(t, agent.server.Reload(sconf)) +} + func TestServer_ShouldReload_ReturnFalseForNoChanges(t *testing.T) { ci.Parallel(t) assert := assert.New(t) diff --git a/nomad/server_test.go b/nomad/server_test.go index 7e4cddd4ef3f..01bb6f931ef0 100644 --- a/nomad/server_test.go +++ b/nomad/server_test.go @@ -226,6 +226,7 @@ func TestServer_Reload_Vault(t *testing.T) { config := DefaultConfig() config.VaultConfig.Enabled = &tr config.VaultConfig.Token = uuid.Generate() + config.VaultConfig.Namespace = "nondefault" if err := s1.Reload(config); err != nil { t.Fatalf("Reload failed: %v", err) @@ -234,6 +235,10 @@ func TestServer_Reload_Vault(t *testing.T) { if !s1.vault.Running() { t.Fatalf("Vault client should be running") } + + if s1.vault.GetConfig().Namespace != "nondefault" { + t.Fatalf("Vault client did not get new namespace") + } } func connectionReset(msg string) bool { diff --git a/nomad/structs/config/vault.go b/nomad/structs/config/vault.go index 3f229b6c9da9..835a53a92a74 100644 --- a/nomad/structs/config/vault.go +++ b/nomad/structs/config/vault.go @@ -106,14 +106,20 @@ func (a *VaultConfig) AllowsUnauthenticated() bool { func (a *VaultConfig) Merge(b *VaultConfig) *VaultConfig { result := *a + if b.Enabled != nil { + result.Enabled = b.Enabled + } if b.Token != "" { result.Token = b.Token } + if b.Role != "" { + result.Role = b.Role + } if b.Namespace != "" { result.Namespace = b.Namespace } - if b.Role != "" { - result.Role = b.Role + if b.AllowUnauthenticated != nil { + result.AllowUnauthenticated = b.AllowUnauthenticated } if b.TaskTokenTTL != "" { result.TaskTokenTTL = b.TaskTokenTTL @@ -136,17 +142,11 @@ func (a *VaultConfig) Merge(b *VaultConfig) *VaultConfig { if b.TLSKeyFile != "" { result.TLSKeyFile = b.TLSKeyFile } - if b.TLSServerName != "" { - result.TLSServerName = b.TLSServerName - } - if b.AllowUnauthenticated != nil { - result.AllowUnauthenticated = b.AllowUnauthenticated - } if b.TLSSkipVerify != nil { result.TLSSkipVerify = b.TLSSkipVerify } - if b.Enabled != nil { - result.Enabled = b.Enabled + if b.TLSServerName != "" { + result.TLSServerName = b.TLSServerName } return &result @@ -188,54 +188,77 @@ func (c *VaultConfig) Copy() *VaultConfig { return nc } -// IsEqual compares two Vault configurations and returns a boolean indicating +// Equals compares two Vault configurations and returns a boolean indicating +// if they are equal. +// Equals 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 { +func (c *VaultConfig) Equals(b *VaultConfig) bool { + if c == nil && b != nil { + return false + } + if c != nil && b == nil { return false } - if a != nil && b == nil { + + if c.Enabled == nil || b.Enabled == nil { + if c.Enabled != b.Enabled { + return false + } + } else if *c.Enabled != *b.Enabled { return false } - if a.Token != b.Token { + if c.Token != b.Token { return false } - if a.Role != b.Role { + if c.Role != b.Role { return false } - if a.TaskTokenTTL != b.TaskTokenTTL { + if c.Namespace != b.Namespace { return false } - if a.Addr != b.Addr { + + if c.AllowUnauthenticated == nil || b.AllowUnauthenticated == nil { + if c.AllowUnauthenticated != b.AllowUnauthenticated { + return false + } + } else if *c.AllowUnauthenticated != *b.AllowUnauthenticated { return false } - if a.ConnectionRetryIntv.Nanoseconds() != b.ConnectionRetryIntv.Nanoseconds() { + + if c.TaskTokenTTL != b.TaskTokenTTL { return false } - if a.TLSCaFile != b.TLSCaFile { + if c.Addr != b.Addr { return false } - if a.TLSCaPath != b.TLSCaPath { + if c.ConnectionRetryIntv.Nanoseconds() != b.ConnectionRetryIntv.Nanoseconds() { return false } - if a.TLSCertFile != b.TLSCertFile { + if c.TLSCaFile != b.TLSCaFile { return false } - if a.TLSKeyFile != b.TLSKeyFile { + if c.TLSCaPath != b.TLSCaPath { return false } - if a.TLSServerName != b.TLSServerName { + if c.TLSCertFile != b.TLSCertFile { return false } - if a.AllowUnauthenticated != b.AllowUnauthenticated { + if c.TLSKeyFile != b.TLSKeyFile { return false } - if a.TLSSkipVerify != b.TLSSkipVerify { + + if c.TLSSkipVerify == nil || b.TLSSkipVerify == nil { + if c.TLSSkipVerify != b.TLSSkipVerify { + return false + } + } else if *c.TLSSkipVerify != *b.TLSSkipVerify { return false } - if a.Enabled != b.Enabled { + + if c.TLSServerName != b.TLSServerName { return false } + return true } diff --git a/nomad/structs/config/vault_test.go b/nomad/structs/config/vault_test.go index c4eda801c755..c5669a2051cb 100644 --- a/nomad/structs/config/vault_test.go +++ b/nomad/structs/config/vault_test.go @@ -3,35 +3,36 @@ package config import ( "reflect" "testing" + "time" "github.com/hashicorp/nomad/ci" + "github.com/hashicorp/nomad/helper" "github.com/stretchr/testify/require" ) func TestVaultConfig_Merge(t *testing.T) { ci.Parallel(t) - trueValue, falseValue := true, false c1 := &VaultConfig{ - Enabled: &falseValue, + Enabled: helper.BoolToPtr(false), Token: "1", Role: "1", - AllowUnauthenticated: &trueValue, + AllowUnauthenticated: helper.BoolToPtr(true), TaskTokenTTL: "1", Addr: "1", TLSCaFile: "1", TLSCaPath: "1", TLSCertFile: "1", TLSKeyFile: "1", - TLSSkipVerify: &trueValue, + TLSSkipVerify: helper.BoolToPtr(true), TLSServerName: "1", } c2 := &VaultConfig{ - Enabled: &trueValue, + Enabled: helper.BoolToPtr(true), Token: "2", Role: "2", - AllowUnauthenticated: &falseValue, + AllowUnauthenticated: helper.BoolToPtr(false), TaskTokenTTL: "2", Addr: "2", TLSCaFile: "2", @@ -43,17 +44,17 @@ func TestVaultConfig_Merge(t *testing.T) { } e := &VaultConfig{ - Enabled: &trueValue, + Enabled: helper.BoolToPtr(true), Token: "2", Role: "2", - AllowUnauthenticated: &falseValue, + AllowUnauthenticated: helper.BoolToPtr(false), TaskTokenTTL: "2", Addr: "2", TLSCaFile: "2", TLSCaPath: "2", TLSCertFile: "2", TLSKeyFile: "2", - TLSSkipVerify: &trueValue, + TLSSkipVerify: helper.BoolToPtr(true), TLSServerName: "2", } @@ -63,72 +64,78 @@ func TestVaultConfig_Merge(t *testing.T) { } } -func TestVaultConfig_IsEqual(t *testing.T) { +func TestVaultConfig_Equals(t *testing.T) { ci.Parallel(t) - - require := require.New(t) - trueValue, falseValue := true, false c1 := &VaultConfig{ - Enabled: &falseValue, + Enabled: helper.BoolToPtr(false), Token: "1", Role: "1", - AllowUnauthenticated: &trueValue, + Namespace: "1", + AllowUnauthenticated: helper.BoolToPtr(true), TaskTokenTTL: "1", Addr: "1", + ConnectionRetryIntv: time.Second, TLSCaFile: "1", TLSCaPath: "1", TLSCertFile: "1", TLSKeyFile: "1", - TLSSkipVerify: &trueValue, + TLSSkipVerify: helper.BoolToPtr(true), TLSServerName: "1", } c2 := &VaultConfig{ - Enabled: &falseValue, + Enabled: helper.BoolToPtr(false), Token: "1", Role: "1", - AllowUnauthenticated: &trueValue, + Namespace: "1", + AllowUnauthenticated: helper.BoolToPtr(true), TaskTokenTTL: "1", Addr: "1", + ConnectionRetryIntv: time.Second, TLSCaFile: "1", TLSCaPath: "1", TLSCertFile: "1", TLSKeyFile: "1", - TLSSkipVerify: &trueValue, + TLSSkipVerify: helper.BoolToPtr(true), TLSServerName: "1", } - require.True(c1.IsEqual(c2)) + require.True(t, c1.Equals(c2)) c3 := &VaultConfig{ - Enabled: &trueValue, + Enabled: helper.BoolToPtr(true), Token: "1", Role: "1", - AllowUnauthenticated: &trueValue, + Namespace: "1", + AllowUnauthenticated: helper.BoolToPtr(true), TaskTokenTTL: "1", Addr: "1", + ConnectionRetryIntv: time.Second, TLSCaFile: "1", TLSCaPath: "1", TLSCertFile: "1", TLSKeyFile: "1", - TLSSkipVerify: &trueValue, + TLSSkipVerify: helper.BoolToPtr(true), TLSServerName: "1", } c4 := &VaultConfig{ - Enabled: &falseValue, + Enabled: helper.BoolToPtr(false), Token: "1", Role: "1", - AllowUnauthenticated: &trueValue, + Namespace: "1", + AllowUnauthenticated: helper.BoolToPtr(true), TaskTokenTTL: "1", Addr: "1", + ConnectionRetryIntv: time.Second, TLSCaFile: "1", TLSCaPath: "1", TLSCertFile: "1", TLSKeyFile: "1", - TLSSkipVerify: &trueValue, + TLSSkipVerify: helper.BoolToPtr(true), TLSServerName: "1", } - require.False(c3.IsEqual(c4)) + + require.False(t, c3.Equals(c4)) } diff --git a/nomad/vault.go b/nomad/vault.go index 482be89817b3..9a2c9bd68f5e 100644 --- a/nomad/vault.go +++ b/nomad/vault.go @@ -112,6 +112,10 @@ type VaultClient interface { // SetConfig updates the config used by the Vault client SetConfig(config *config.VaultConfig) error + // GetConfig returns a copy of the config used by the Vault client, for + // testing + GetConfig() *config.VaultConfig + // CreateToken takes an allocation and task and returns an appropriate Vault // Secret CreateToken(ctx context.Context, a *structs.Allocation, task string) (*vapi.Secret, error) @@ -352,6 +356,13 @@ func (v *vaultClient) flush() { v.tomb = &tomb.Tomb{} } +// GetConfig returns a copy of this vault client's configuration, for testing. +func (v *vaultClient) GetConfig() *config.VaultConfig { + v.setConfigLock.Lock() + defer v.setConfigLock.Unlock() + return v.config.Copy() +} + // SetConfig is used to update the Vault config being used. A temporary outage // may occur after calling as it re-establishes a connection to Vault func (v *vaultClient) SetConfig(config *config.VaultConfig) error { @@ -365,7 +376,7 @@ func (v *vaultClient) SetConfig(config *config.VaultConfig) error { defer v.l.Unlock() // If reloading the same config, no-op - if v.config.IsEqual(config) { + if v.config.Equals(config) { return nil } diff --git a/nomad/vault_testing.go b/nomad/vault_testing.go index e3555173701e..8f00d2688b4c 100644 --- a/nomad/vault_testing.go +++ b/nomad/vault_testing.go @@ -142,6 +142,7 @@ func (v *TestVaultClient) MarkForRevocation(accessors []*structs.VaultAccessor) func (v *TestVaultClient) Stop() {} func (v *TestVaultClient) SetActive(enabled bool) {} +func (v *TestVaultClient) GetConfig() *config.VaultConfig { return nil } func (v *TestVaultClient) SetConfig(config *config.VaultConfig) error { return nil } func (v *TestVaultClient) Running() bool { return true } func (v *TestVaultClient) Stats() map[string]string { return map[string]string{} }