Fix for dynamically reloading vault

CHANGELOG.md

@@ -42,6 +42,7 @@ IMPROVEMENTS:
 
 BUG FIXES:
  * core: Clean up leaked deployments on restoration [[GH-4329](https://github.com/hashicorp/nomad/issues/4329)]
+ * core: Fix regression to allow for dynamic Vault configuration reload [[GH-4395](https://github.com/hashicorp/nomad/issues/4395)]
  * core: Fix bug where older failed allocations of jobs that have been updated to a newer version were
    not being garbage collected [[GH-4313](https://github.com/hashicorp/nomad/issues/4313)]
  * core: Fix bug when upgrading an existing server to Raft protocol 3 that

command/agent/command.go

@@ -740,20 +740,21 @@ func (c *Command) handleReload() {
 		}
 	}
 
-	if shouldReloadRPC {
-		if s := c.agent.Server(); s != nil {
-			sconf, err := convertServerConfig(newConf, c.logOutput)
-			c.agent.logger.Printf("[DEBUG] agent: starting reload of server config")
-			if err != nil {
-				c.agent.logger.Printf("[ERR] agent: failed to convert server config: %v", err)
+	c.agent.logger.Printf("[DEBUG] agent: starting reload of server config")
+	if s := c.agent.Server(); s != nil {
+		sconf, err := convertServerConfig(newConf, c.logOutput)
+		if err != nil {
+			c.agent.logger.Printf("[ERR] agent: failed to convert server config: %v", err)
+			return
+		} else {
+			if err := s.Reload(sconf); err != nil {
+				c.agent.logger.Printf("[ERR] agent: reloading server config failed: %v", err)
 				return
-			} else {
-				if err := s.Reload(sconf); err != nil {
-					c.agent.logger.Printf("[ERR] agent: reloading server config failed: %v", err)
-					return
-				}
 			}
 		}
+	}
+
+	if shouldReloadRPC {
 
 		if s := c.agent.Client(); s != nil {
 			clientConfig, err := c.agent.clientConfig()

nomad/server_test.go

@@ -219,7 +219,7 @@ func TestServer_Reload_Vault(t *testing.T) {
 	}
 
 	tr := true
-	config := s1.config
+	config := DefaultConfig()
 	config.VaultConfig.Enabled = &tr
 	config.VaultConfig.Token = uuid.Generate()
 

nomad/structs/config/vault.go

@@ -181,3 +181,55 @@ func (c *VaultConfig) Copy() *VaultConfig {
 	*nc = *c
 	return nc
 }
+
+// IsEqual 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 {
+		return false
+	}
+	if a != nil && b == nil {
+		return false
+	}
+
+	if a.Token != b.Token {
+		return false
+	}
+	if a.Role != b.Role {
+		return false
+	}
+	if a.TaskTokenTTL != b.TaskTokenTTL {
+		return false
+	}
+	if a.Addr != b.Addr {
+		return false
+	}
+	if a.ConnectionRetryIntv.Nanoseconds() != b.ConnectionRetryIntv.Nanoseconds() {
+		return false
+	}
+	if a.TLSCaFile != b.TLSCaFile {
+		return false
+	}
+	if a.TLSCaPath != b.TLSCaPath {
+		return false
+	}
+	if a.TLSCertFile != b.TLSCertFile {
+		return false
+	}
+	if a.TLSKeyFile != b.TLSKeyFile {
+		return false
+	}
+	if a.TLSServerName != b.TLSServerName {
+		return false
+	}
+	if a.AllowUnauthenticated != b.AllowUnauthenticated {
+		return false
+	}
+	if a.TLSSkipVerify != b.TLSSkipVerify {
+		return false
+	}
+	if a.Enabled != b.Enabled {
+		return false
+	}
+	return true
+}

nomad/structs/config/vault_test.go

@@ -3,6 +3,8 @@ package config
 import (
 	"reflect"
 	"testing"
+
+	"github.com/stretchr/testify/require"
 )
 
 func TestVaultConfig_Merge(t *testing.T) {
@@ -57,3 +59,71 @@ func TestVaultConfig_Merge(t *testing.T) {
 		t.Fatalf("bad:\n%#v\n%#v", result, e)
 	}
 }
+
+func TestVaultConfig_IsEqual(t *testing.T) {
+	require := require.New(t)
+
+	trueValue, falseValue := true, false
+	c1 := &VaultConfig{
+		Enabled:              &falseValue,
+		Token:                "1",
+		Role:                 "1",
+		AllowUnauthenticated: &trueValue,
+		TaskTokenTTL:         "1",
+		Addr:                 "1",
+		TLSCaFile:            "1",
+		TLSCaPath:            "1",
+		TLSCertFile:          "1",
+		TLSKeyFile:           "1",
+		TLSSkipVerify:        &trueValue,
+		TLSServerName:        "1",
+	}
+
+	c2 := &VaultConfig{
+		Enabled:              &falseValue,
+		Token:                "1",
+		Role:                 "1",
+		AllowUnauthenticated: &trueValue,
+		TaskTokenTTL:         "1",
+		Addr:                 "1",
+		TLSCaFile:            "1",
+		TLSCaPath:            "1",
+		TLSCertFile:          "1",
+		TLSKeyFile:           "1",
+		TLSSkipVerify:        &trueValue,
+		TLSServerName:        "1",
+	}
+
+	require.True(c1.IsEqual(c2))
+
+	c3 := &VaultConfig{
+		Enabled:              &trueValue,
+		Token:                "1",
+		Role:                 "1",
+		AllowUnauthenticated: &trueValue,
+		TaskTokenTTL:         "1",
+		Addr:                 "1",
+		TLSCaFile:            "1",
+		TLSCaPath:            "1",
+		TLSCertFile:          "1",
+		TLSKeyFile:           "1",
+		TLSSkipVerify:        &trueValue,
+		TLSServerName:        "1",
+	}
+
+	c4 := &VaultConfig{
+		Enabled:              &falseValue,
+		Token:                "1",
+		Role:                 "1",
+		AllowUnauthenticated: &trueValue,
+		TaskTokenTTL:         "1",
+		Addr:                 "1",
+		TLSCaFile:            "1",
+		TLSCaPath:            "1",
+		TLSCertFile:          "1",
+		TLSKeyFile:           "1",
+		TLSSkipVerify:        &trueValue,
+		TLSServerName:        "1",
+	}
+	require.False(c3.IsEqual(c4))
+}

nomad/vault.go

@@ -316,6 +316,11 @@ func (v *vaultClient) SetConfig(config *config.VaultConfig) error {
 	v.l.Lock()
 	defer v.l.Unlock()
 
+	// If reloading the same config, no-op
+	if v.config.IsEqual(config) {
+		return nil
+	}
+
 	// Kill any background routines
 	if v.running {
 		// Stop accepting any new request

nomad/vault_test.go

@@ -437,6 +437,31 @@ func TestVaultClient_SetConfig(t *testing.T) {
 	if client.tokenData == nil || len(client.tokenData.Policies) != 3 {
 		t.Fatalf("unexpected token: %v", client.tokenData)
 	}
+
+	// Test that when SetConfig is called with the same configuration, it is a
+	// no-op
+	failCh := make(chan struct{}, 1)
+	go func() {
+		tomb := client.tomb
+		select {
+		case <-tomb.Dying():
+			close(failCh)
+		case <-time.After(1 * time.Second):
+			return
+		}
+	}()
+
+	// Update the config
+	if err := client.SetConfig(v2.Config); err != nil {
+		t.Fatalf("SetConfig failed: %v", err)
+	}
+
+	select {
+	case <-failCh:
+		t.Fatalf("Tomb shouldn't have exited")
+	case <-time.After(1 * time.Second):
+		return
+	}
 }
 
 // Test that we can disable vault