From f305849be813134997afa2c06df5388daaf7cfdd Mon Sep 17 00:00:00 2001 From: Tim Gross Date: Wed, 24 Aug 2022 14:53:38 -0400 Subject: [PATCH] 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 | 62 +++++++++++++++++------------- nomad/vault.go | 2 +- 6 files changed, 111 insertions(+), 47 deletions(-) create mode 100644 .changelog/14298.txt 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..f2f39dcb4e72 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 6e73e5c11c08..c4ea2cfc7424 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 f3e9b290f3fe..13ffc449a72c 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 f6fa2c6bd4f5..bc8824812001 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/shoenig/test/must" "github.com/hashicorp/nomad/ci" - "github.com/stretchr/testify/require" + "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,75 @@ 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)) + 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..7e2550b15801 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 }