From b2c2ab5b838fb825b29aaf7ec3566898d3a738a3 Mon Sep 17 00:00:00 2001 From: Tim Gross Date: Wed, 24 Aug 2022 14:53:38 -0400 Subject: [PATCH 1/3] vault: detect namespace change in config reload The `namespace` field was not included in the equality check between old and new Vault configurations, which meant that a Vault config change that only changed the namespace would not be detected as a change and the clients would not be reloaded. Also, the comparison for boolean fields such as `enabled` and `allow_unauthenticated` was on the pointer and not the value of that pointer, which results in spurious reloads in real config reload that is easily missed in typical test scenarios. Includes a minor refactor of the order of fields for `Copy` and `Merge` to match the struct fields in hopes it makes it harder to make this mistake in the future, as well as additional test coverage. --- .changelog/14298.txt | 7 ++++ command/agent/agent_test.go | 33 +++++++++++++++ nomad/server_test.go | 1 + nomad/structs/config/vault.go | 53 ++++++++++++++++--------- nomad/structs/config/vault_test.go | 64 +++++++++++++++++------------- nomad/vault.go | 2 +- 6 files changed, 113 insertions(+), 47 deletions(-) create mode 100644 .changelog/14298.txt diff --git a/.changelog/14298.txt b/.changelog/14298.txt new file mode 100644 index 00000000000..1072f7bebf2 --- /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 49cd81f5a6b..f2f39dcb4e7 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) + + logger := testlog.HCLogger(t) + + agentConfig := &Config{ + TLSConfig: &config.TLSConfig{}, + Vault: &config.VaultConfig{ + Enabled: pointer.Of(true), + Token: "vault-token", + Namespace: "vault-namespace", + }, + } + + agent := &Agent{ + auditor: &noOpAuditor{}, + logger: logger, + config: agentConfig, + } + + newConfig := &Config{ + TLSConfig: &config.TLSConfig{}, + Vault: &config.VaultConfig{ + Enabled: pointer.Of(true), + Token: "vault-token", + Namespace: "vault-namespace", + }, + } + + must.NoError(t, agent.Reload(newConfig)) + must.Equals(t, agent.config.Vault, newConfig.Vault) +} + 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 6e73e5c11c0..c4ea2cfc742 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) diff --git a/nomad/structs/config/vault.go b/nomad/structs/config/vault.go index f3e9b290f3f..13ffc449a72 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,12 @@ 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 { 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 f6fa2c6bd4f..4e8420d9886 100644 --- a/nomad/structs/config/vault_test.go +++ b/nomad/structs/config/vault_test.go @@ -3,35 +3,37 @@ package config import ( "reflect" "testing" + "time" - "github.com/hashicorp/nomad/ci" "github.com/stretchr/testify/require" + + "github.com/hashicorp/nomad/ci" + "github.com/hashicorp/nomad/helper/pointer" ) 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 +45,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", } @@ -66,69 +68,77 @@ func TestVaultConfig_Merge(t *testing.T) { func TestVaultConfig_IsEqual(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)) + // TODO: must.Equals(t, c, c2) should work here? + require.True(t, c.Equals(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)) + + // TODO: must.NotEquals(t, c3, c4) should work here? + require.False(t, c3.Equals(c4)) } diff --git a/nomad/vault.go b/nomad/vault.go index 4e3ac0d577b..7e2550b1580 100644 --- a/nomad/vault.go +++ b/nomad/vault.go @@ -363,7 +363,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 } From 9692586e77c3121737f6fe2c14fd6037520410c4 Mon Sep 17 00:00:00 2001 From: Tim Gross Date: Wed, 24 Aug 2022 16:14:29 -0400 Subject: [PATCH 2/3] add accessor method for testing --- command/agent/agent_test.go | 42 +++++++++++++++--------------- nomad/server_test.go | 4 +++ nomad/structs/config/vault_test.go | 2 +- nomad/vault.go | 11 ++++++++ nomad/vault_testing.go | 1 + 5 files changed, 38 insertions(+), 22 deletions(-) diff --git a/command/agent/agent_test.go b/command/agent/agent_test.go index f2f39dcb4e7..56df743e1a0 100644 --- a/command/agent/agent_test.go +++ b/command/agent/agent_test.go @@ -1088,34 +1088,34 @@ func TestServer_Reload_TLS_DowngradeFromTLS(t *testing.T) { func TestServer_Reload_VaultConfig(t *testing.T) { ci.Parallel(t) - logger := testlog.HCLogger(t) - - agentConfig := &Config{ - TLSConfig: &config.TLSConfig{}, - Vault: &config.VaultConfig{ + 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() - agent := &Agent{ - auditor: &noOpAuditor{}, - logger: logger, - config: agentConfig, + newConfig := agent.GetConfig().Copy() + newConfig.Vault = &config.VaultConfig{ + Enabled: pointer.Of(true), + Token: "vault-token", + Namespace: "another-namespace", + Addr: "https://vault.consul:8200", } - newConfig := &Config{ - TLSConfig: &config.TLSConfig{}, - Vault: &config.VaultConfig{ - Enabled: pointer.Of(true), - Token: "vault-token", - Namespace: "vault-namespace", - }, - } + sconf, err := convertServerConfig(newConfig) + must.NoError(t, err) + agent.finalizeServerConfig(sconf) - must.NoError(t, agent.Reload(newConfig)) - must.Equals(t, agent.config.Vault, newConfig.Vault) + // 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) { diff --git a/nomad/server_test.go b/nomad/server_test.go index c4ea2cfc742..16f99c2c3e8 100644 --- a/nomad/server_test.go +++ b/nomad/server_test.go @@ -223,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_test.go b/nomad/structs/config/vault_test.go index 4e8420d9886..83739c4c9b7 100644 --- a/nomad/structs/config/vault_test.go +++ b/nomad/structs/config/vault_test.go @@ -65,7 +65,7 @@ func TestVaultConfig_Merge(t *testing.T) { } } -func TestVaultConfig_IsEqual(t *testing.T) { +func TestVaultConfig_Equals(t *testing.T) { ci.Parallel(t) c1 := &VaultConfig{ diff --git a/nomad/vault.go b/nomad/vault.go index 7e2550b1580..0410b53597e 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 { diff --git a/nomad/vault_testing.go b/nomad/vault_testing.go index 857cd52150f..b81c0f197d1 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{} } From 3f6349922eab6c6c77ec925cf36095ffeb4f5aa1 Mon Sep 17 00:00:00 2001 From: Tim Gross Date: Wed, 24 Aug 2022 16:23:35 -0400 Subject: [PATCH 3/3] fix TLSSkipVerify comparison too --- nomad/structs/config/vault.go | 8 +++++++- nomad/structs/config/vault_test.go | 9 +++------ 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/nomad/structs/config/vault.go b/nomad/structs/config/vault.go index 13ffc449a72..4deb4c2f743 100644 --- a/nomad/structs/config/vault.go +++ b/nomad/structs/config/vault.go @@ -245,9 +245,15 @@ func (c *VaultConfig) Equals(b *VaultConfig) bool { if c.TLSKeyFile != b.TLSKeyFile { 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.TLSServerName != b.TLSServerName { return false } diff --git a/nomad/structs/config/vault_test.go b/nomad/structs/config/vault_test.go index 83739c4c9b7..6f5e2909c98 100644 --- a/nomad/structs/config/vault_test.go +++ b/nomad/structs/config/vault_test.go @@ -5,10 +5,9 @@ import ( "testing" "time" - "github.com/stretchr/testify/require" - "github.com/hashicorp/nomad/ci" "github.com/hashicorp/nomad/helper/pointer" + "github.com/shoenig/test/must" ) func TestVaultConfig_Merge(t *testing.T) { @@ -102,8 +101,7 @@ func TestVaultConfig_Equals(t *testing.T) { TLSServerName: "1", } - // TODO: must.Equals(t, c, c2) should work here? - require.True(t, c.Equals(c2)) + must.Equals(t, c1, c2) c3 := &VaultConfig{ Enabled: pointer.Of(true), @@ -139,6 +137,5 @@ func TestVaultConfig_Equals(t *testing.T) { TLSServerName: "1", } - // TODO: must.NotEquals(t, c3, c4) should work here? - require.False(t, c3.Equals(c4)) + must.NotEquals(t, c3, c4) }