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

@@ -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

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

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)

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,22 +188,42 @@ 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
 	}
 	if c != nil && b == nil {
 		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
 }

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

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
 	}