From 3de3b47829d941d824c87f8021afa3b9e12f3eac Mon Sep 17 00:00:00 2001 From: Chelsea Holland Komlo Date: Thu, 7 Jun 2018 15:34:18 -0400 Subject: [PATCH 1/5] fix for dynamically reloading vault --- command/agent/command.go | 15 ++++++- nomad/structs/config/vault.go | 52 ++++++++++++++++++++++ nomad/structs/config/vault_test.go | 70 ++++++++++++++++++++++++++++++ 3 files changed, 136 insertions(+), 1 deletion(-) diff --git a/command/agent/command.go b/command/agent/command.go index 9b9fb9ad4364..3ed6e96243c2 100644 --- a/command/agent/command.go +++ b/command/agent/command.go @@ -740,7 +740,17 @@ func (c *Command) handleReload() { } } - if shouldReloadRPC { + var shouldReloadVault bool + switch { + case c.agent.config.Vault == nil && newConf.Vault != nil: + fallthrough + case c.agent.config.Vault != nil && newConf.Vault == nil: + fallthrough + case c.agent.config.Vault != nil && !c.agent.config.Vault.IsEqual(newConf.Vault): + shouldReloadVault = true + } + + if shouldReloadRPC || shouldReloadVault { if s := c.agent.Server(); s != nil { sconf, err := convertServerConfig(newConf, c.logOutput) c.agent.logger.Printf("[DEBUG] agent: starting reload of server config") @@ -754,6 +764,9 @@ func (c *Command) handleReload() { } } } + } + + if shouldReloadRPC { if s := c.agent.Client(); s != nil { clientConfig, err := c.agent.clientConfig() diff --git a/nomad/structs/config/vault.go b/nomad/structs/config/vault.go index 42115c1a9132..bfb515623f54 100644 --- a/nomad/structs/config/vault.go +++ b/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 +} diff --git a/nomad/structs/config/vault_test.go b/nomad/structs/config/vault_test.go index f92f1dde75cc..e48b6ef02782 100644 --- a/nomad/structs/config/vault_test.go +++ b/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)) +} From f8f89d74903c07a2d0b12e1a8e645ac25154ffb7 Mon Sep 17 00:00:00 2001 From: Chelsea Holland Komlo Date: Thu, 7 Jun 2018 16:22:31 -0400 Subject: [PATCH 2/5] move logic for testing equality for vault config --- command/agent/command.go | 30 +++++++++--------------------- nomad/vault.go | 5 +++++ nomad/vault_test.go | 25 +++++++++++++++++++++++++ 3 files changed, 39 insertions(+), 21 deletions(-) diff --git a/command/agent/command.go b/command/agent/command.go index 3ed6e96243c2..256914e21b39 100644 --- a/command/agent/command.go +++ b/command/agent/command.go @@ -740,28 +740,16 @@ func (c *Command) handleReload() { } } - var shouldReloadVault bool - switch { - case c.agent.config.Vault == nil && newConf.Vault != nil: - fallthrough - case c.agent.config.Vault != nil && newConf.Vault == nil: - fallthrough - case c.agent.config.Vault != nil && !c.agent.config.Vault.IsEqual(newConf.Vault): - shouldReloadVault = true - } - - if shouldReloadRPC || shouldReloadVault { - 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) + 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) + 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 - } } } } diff --git a/nomad/vault.go b/nomad/vault.go index ca1486ebf93e..fc60075fb097 100644 --- a/nomad/vault.go +++ b/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 diff --git a/nomad/vault_test.go b/nomad/vault_test.go index 687395a0dc20..2c165fb6a17e 100644 --- a/nomad/vault_test.go +++ b/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 From a4df2260f79179f6a0490d1093f2e1992f9b7374 Mon Sep 17 00:00:00 2001 From: Chelsea Holland Komlo Date: Thu, 7 Jun 2018 17:07:06 -0400 Subject: [PATCH 3/5] fix test that now requires different config for test assertions --- nomad/server_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/nomad/server_test.go b/nomad/server_test.go index ef1439047599..30633b9e87b4 100644 --- a/nomad/server_test.go +++ b/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() From b92982c34624d9b11ed3695103a0894867dce5d9 Mon Sep 17 00:00:00 2001 From: Chelsea Holland Komlo Date: Thu, 7 Jun 2018 17:09:59 -0400 Subject: [PATCH 4/5] changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 38e27f80ea14..40f17ff6cb50 100644 --- a/CHANGELOG.md +++ b/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 From db4115a68f00bda37d434dba1f5a42ca0e37b5a9 Mon Sep 17 00:00:00 2001 From: Chelsea Holland Komlo Date: Thu, 7 Jun 2018 17:12:36 -0400 Subject: [PATCH 5/5] fixup! comment and move to always log server reload operation --- command/agent/command.go | 2 +- nomad/vault_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/command/agent/command.go b/command/agent/command.go index 256914e21b39..1fa22734cf65 100644 --- a/command/agent/command.go +++ b/command/agent/command.go @@ -740,9 +740,9 @@ func (c *Command) handleReload() { } } + c.agent.logger.Printf("[DEBUG] agent: starting reload of server config") 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) return diff --git a/nomad/vault_test.go b/nomad/vault_test.go index 2c165fb6a17e..5ba75f4da8d0 100644 --- a/nomad/vault_test.go +++ b/nomad/vault_test.go @@ -438,7 +438,7 @@ func TestVaultClient_SetConfig(t *testing.T) { t.Fatalf("unexpected token: %v", client.tokenData) } - // test that when SetConfig is called with the same configuration, it is a + // Test that when SetConfig is called with the same configuration, it is a // no-op failCh := make(chan struct{}, 1) go func() {