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 49cd81f5a6b6..56df743e1a08 100644 --- a/command/agent/agent_test.go +++ b/command/agent/agent_test.go @@ -1085,6 +1085,39 @@ func TestServer_Reload_TLS_DowngradeFromTLS(t *testing.T) { assert.True(agent.config.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 6e73e5c11c08..16f99c2c3e8b 100644 --- a/nomad/server_test.go +++ b/nomad/server_test.go @@ -214,6 +214,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) @@ -222,6 +223,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 f3e9b290f3fe..4deb4c2f743c 100644 --- a/nomad/structs/config/vault.go +++ b/nomad/structs/config/vault.go @@ -106,14 +106,20 @@ func (c *VaultConfig) AllowsUnauthenticated() bool { func (c *VaultConfig) Merge(b *VaultConfig) *VaultConfig { result := *c + 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 (c *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,9 +188,9 @@ 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. -func (c *VaultConfig) IsEqual(b *VaultConfig) bool { +func (c *VaultConfig) Equals(b *VaultConfig) bool { if c == nil && b != nil { return false } @@ -198,12 +198,32 @@ func (c *VaultConfig) IsEqual(b *VaultConfig) bool { return false } + if c.Enabled == nil || b.Enabled == nil { + if c.Enabled != b.Enabled { + return false + } + } else if *c.Enabled != *b.Enabled { + return false + } + if c.Token != b.Token { return false } if c.Role != b.Role { return false } + if c.Namespace != b.Namespace { + return false + } + + if c.AllowUnauthenticated == nil || b.AllowUnauthenticated == nil { + if c.AllowUnauthenticated != b.AllowUnauthenticated { + return false + } + } else if *c.AllowUnauthenticated != *b.AllowUnauthenticated { + return false + } + if c.TaskTokenTTL != b.TaskTokenTTL { return false } @@ -225,17 +245,18 @@ func (c *VaultConfig) IsEqual(b *VaultConfig) bool { if c.TLSKeyFile != b.TLSKeyFile { return false } - if c.TLSServerName != b.TLSServerName { - return false - } - if c.AllowUnauthenticated != b.AllowUnauthenticated { - return false - } - if c.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 c.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 f6fa2c6bd4f5..6f5e2909c98e 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/stretchr/testify/require" + "github.com/hashicorp/nomad/helper/pointer" + "github.com/shoenig/test/must" ) func TestVaultConfig_Merge(t *testing.T) { ci.Parallel(t) - trueValue, falseValue := true, false c1 := &VaultConfig{ - Enabled: &falseValue, + Enabled: pointer.Of(false), Token: "1", Role: "1", - AllowUnauthenticated: &trueValue, + AllowUnauthenticated: pointer.Of(true), TaskTokenTTL: "1", Addr: "1", TLSCaFile: "1", TLSCaPath: "1", TLSCertFile: "1", TLSKeyFile: "1", - TLSSkipVerify: &trueValue, + TLSSkipVerify: pointer.Of(true), TLSServerName: "1", } c2 := &VaultConfig{ - Enabled: &trueValue, + Enabled: pointer.Of(true), Token: "2", Role: "2", - AllowUnauthenticated: &falseValue, + AllowUnauthenticated: pointer.Of(false), TaskTokenTTL: "2", Addr: "2", TLSCaFile: "2", @@ -43,17 +44,17 @@ func TestVaultConfig_Merge(t *testing.T) { } e := &VaultConfig{ - Enabled: &trueValue, + Enabled: pointer.Of(true), Token: "2", Role: "2", - AllowUnauthenticated: &falseValue, + AllowUnauthenticated: pointer.Of(false), TaskTokenTTL: "2", Addr: "2", TLSCaFile: "2", TLSCaPath: "2", TLSCertFile: "2", TLSKeyFile: "2", - TLSSkipVerify: &trueValue, + TLSSkipVerify: pointer.Of(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: pointer.Of(false), Token: "1", Role: "1", - AllowUnauthenticated: &trueValue, + Namespace: "1", + AllowUnauthenticated: pointer.Of(true), TaskTokenTTL: "1", Addr: "1", + ConnectionRetryIntv: time.Second, TLSCaFile: "1", TLSCaPath: "1", TLSCertFile: "1", TLSKeyFile: "1", - TLSSkipVerify: &trueValue, + TLSSkipVerify: pointer.Of(true), TLSServerName: "1", } c2 := &VaultConfig{ - Enabled: &falseValue, + Enabled: pointer.Of(false), Token: "1", Role: "1", - AllowUnauthenticated: &trueValue, + Namespace: "1", + AllowUnauthenticated: pointer.Of(true), TaskTokenTTL: "1", Addr: "1", + ConnectionRetryIntv: time.Second, TLSCaFile: "1", TLSCaPath: "1", TLSCertFile: "1", TLSKeyFile: "1", - TLSSkipVerify: &trueValue, + TLSSkipVerify: pointer.Of(true), TLSServerName: "1", } - require.True(c1.IsEqual(c2)) + must.Equals(t, c1, c2) c3 := &VaultConfig{ - Enabled: &trueValue, + Enabled: pointer.Of(true), Token: "1", Role: "1", - AllowUnauthenticated: &trueValue, + Namespace: "1", + AllowUnauthenticated: pointer.Of(true), TaskTokenTTL: "1", Addr: "1", + ConnectionRetryIntv: time.Second, TLSCaFile: "1", TLSCaPath: "1", TLSCertFile: "1", TLSKeyFile: "1", - TLSSkipVerify: &trueValue, + TLSSkipVerify: pointer.Of(true), TLSServerName: "1", } c4 := &VaultConfig{ - Enabled: &falseValue, + Enabled: pointer.Of(false), Token: "1", Role: "1", - AllowUnauthenticated: &trueValue, + Namespace: "1", + AllowUnauthenticated: pointer.Of(true), TaskTokenTTL: "1", Addr: "1", + ConnectionRetryIntv: time.Second, TLSCaFile: "1", TLSCaPath: "1", TLSCertFile: "1", TLSKeyFile: "1", - TLSSkipVerify: &trueValue, + TLSSkipVerify: pointer.Of(true), TLSServerName: "1", } - require.False(c3.IsEqual(c4)) + + must.NotEquals(t, c3, c4) } diff --git a/nomad/vault.go b/nomad/vault.go index 4e3ac0d577b4..0410b53597ef 100644 --- a/nomad/vault.go +++ b/nomad/vault.go @@ -111,6 +111,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) @@ -350,6 +354,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 { @@ -363,7 +374,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 857cd52150f7..b81c0f197d1e 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{} }