vault: detect namespace change in config reload (#14298)

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

@@ -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.
+```

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)

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 {

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
 }

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))
 }

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
 	}