From 44bacc34944386ab88d85065054c31be3186fa3c Mon Sep 17 00:00:00 2001 From: Chelsea Holland Komlo Date: Fri, 8 Jun 2018 13:14:40 -0400 Subject: [PATCH 1/5] remove logic to reload RPC connections from agent --- command/agent/agent.go | 9 ++++----- command/agent/agent_test.go | 24 ++++++++---------------- command/agent/command.go | 25 +++++++++++-------------- 3 files changed, 23 insertions(+), 35 deletions(-) diff --git a/command/agent/agent.go b/command/agent/agent.go index b9bd554b9..a18fdf694 100644 --- a/command/agent/agent.go +++ b/command/agent/agent.go @@ -866,16 +866,16 @@ func (a *Agent) Stats() map[string]map[string]string { // ShouldReload determines if we should reload the configuration and agent // connections. If the TLS Configuration has not changed, we shouldn't reload. -func (a *Agent) ShouldReload(newConfig *Config) (agent, http, rpc bool) { +func (a *Agent) ShouldReload(newConfig *Config) (agent, http bool) { a.configLock.Lock() defer a.configLock.Unlock() isEqual, err := a.config.TLSConfig.CertificateInfoIsEqual(newConfig.TLSConfig) if err != nil { a.logger.Printf("[INFO] agent: error when parsing TLS certificate %v", err) - return false, false, false + return false, false } else if !isEqual { - return true, true, true + return true, true } // Allow the ability to only reload HTTP connections @@ -886,11 +886,10 @@ func (a *Agent) ShouldReload(newConfig *Config) (agent, http, rpc bool) { // Allow the ability to only reload HTTP connections if a.config.TLSConfig.EnableRPC != newConfig.TLSConfig.EnableRPC { - rpc = true agent = true } - return agent, http, rpc + return agent, http } // Reload handles configuration changes for the agent. Provides a method that diff --git a/command/agent/agent_test.go b/command/agent/agent_test.go index 89c556808..cf9ab9fd0 100644 --- a/command/agent/agent_test.go +++ b/command/agent/agent_test.go @@ -769,10 +769,9 @@ func TestServer_ShouldReload_ReturnFalseForNoChanges(t *testing.T) { }) defer agent.Shutdown() - shouldReloadAgent, shouldReloadHTTP, shouldReloadRPC := agent.ShouldReload(sameAgentConfig) + shouldReloadAgent, shouldReloadHTTP := agent.ShouldReload(sameAgentConfig) assert.False(shouldReloadAgent) assert.False(shouldReloadHTTP) - assert.False(shouldReloadRPC) } func TestServer_ShouldReload_ReturnTrueForOnlyHTTPChanges(t *testing.T) { @@ -810,10 +809,9 @@ func TestServer_ShouldReload_ReturnTrueForOnlyHTTPChanges(t *testing.T) { }) defer agent.Shutdown() - shouldReloadAgent, shouldReloadHTTP, shouldReloadRPC := agent.ShouldReload(sameAgentConfig) + shouldReloadAgent, shouldReloadHTTP := agent.ShouldReload(sameAgentConfig) require.True(shouldReloadAgent) require.True(shouldReloadHTTP) - require.False(shouldReloadRPC) } func TestServer_ShouldReload_ReturnTrueForOnlyRPCChanges(t *testing.T) { @@ -851,10 +849,9 @@ func TestServer_ShouldReload_ReturnTrueForOnlyRPCChanges(t *testing.T) { }) defer agent.Shutdown() - shouldReloadAgent, shouldReloadHTTP, shouldReloadRPC := agent.ShouldReload(sameAgentConfig) + shouldReloadAgent, shouldReloadHTTP := agent.ShouldReload(sameAgentConfig) assert.True(shouldReloadAgent) assert.False(shouldReloadHTTP) - assert.True(shouldReloadRPC) } func TestServer_ShouldReload_ReturnTrueForConfigChanges(t *testing.T) { @@ -894,10 +891,9 @@ func TestServer_ShouldReload_ReturnTrueForConfigChanges(t *testing.T) { }, } - shouldReloadAgent, shouldReloadHTTP, shouldReloadRPC := agent.ShouldReload(newConfig) + shouldReloadAgent, shouldReloadHTTP := agent.ShouldReload(newConfig) assert.True(shouldReloadAgent) assert.True(shouldReloadHTTP) - assert.True(shouldReloadRPC) } func TestServer_ShouldReload_ReturnTrueForFileChanges(t *testing.T) { @@ -959,10 +955,9 @@ func TestServer_ShouldReload_ReturnTrueForFileChanges(t *testing.T) { } agent.config.TLSConfig.SetChecksum() - shouldReloadAgent, shouldReloadHTTP, shouldReloadRPC := agent.ShouldReload(agentConfig) + shouldReloadAgent, shouldReloadHTTP := agent.ShouldReload(agentConfig) require.False(shouldReloadAgent) require.False(shouldReloadHTTP) - require.False(shouldReloadRPC) newCertificate := ` -----BEGIN CERTIFICATE----- @@ -999,10 +994,9 @@ func TestServer_ShouldReload_ReturnTrueForFileChanges(t *testing.T) { }, } - shouldReloadAgent, shouldReloadHTTP, shouldReloadRPC = agent.ShouldReload(newAgentConfig) + shouldReloadAgent, shouldReloadHTTP = agent.ShouldReload(newAgentConfig) require.True(shouldReloadAgent) require.True(shouldReloadHTTP) - require.True(shouldReloadRPC) } func TestServer_ShouldReload_ShouldHandleMultipleChanges(t *testing.T) { @@ -1043,20 +1037,18 @@ func TestServer_ShouldReload_ShouldHandleMultipleChanges(t *testing.T) { defer agent.Shutdown() { - shouldReloadAgent, shouldReloadHTTP, shouldReloadRPC := agent.ShouldReload(sameAgentConfig) + shouldReloadAgent, shouldReloadHTTP := agent.ShouldReload(sameAgentConfig) require.True(shouldReloadAgent) require.True(shouldReloadHTTP) - require.True(shouldReloadRPC) } err := agent.Reload(sameAgentConfig) require.Nil(err) { - shouldReloadAgent, shouldReloadHTTP, shouldReloadRPC := agent.ShouldReload(sameAgentConfig) + shouldReloadAgent, shouldReloadHTTP := agent.ShouldReload(sameAgentConfig) require.False(shouldReloadAgent) require.False(shouldReloadHTTP) - require.False(shouldReloadRPC) } } diff --git a/command/agent/command.go b/command/agent/command.go index 0a2d7a658..3a3e041d4 100644 --- a/command/agent/command.go +++ b/command/agent/command.go @@ -730,7 +730,7 @@ func (c *Command) handleReload() { newConf.LogLevel = c.agent.GetConfig().LogLevel } - shouldReloadAgent, shouldReloadHTTP, shouldReloadRPC := c.agent.ShouldReload(newConf) + shouldReloadAgent, shouldReloadHTTP := c.agent.ShouldReload(newConf) if shouldReloadAgent { c.agent.logger.Printf("[DEBUG] agent: starting reload of agent config") err := c.agent.Reload(newConf) @@ -754,19 +754,16 @@ func (c *Command) handleReload() { } } - if shouldReloadRPC { - - if s := c.agent.Client(); s != nil { - clientConfig, err := c.agent.clientConfig() - c.agent.logger.Printf("[DEBUG] agent: starting reload of client config") - if err != nil { - c.agent.logger.Printf("[ERR] agent: reloading client config failed: %v", err) - return - } - if err := c.agent.Client().Reload(clientConfig); err != nil { - c.agent.logger.Printf("[ERR] agent: reloading client config failed: %v", err) - return - } + if s := c.agent.Client(); s != nil { + clientConfig, err := c.agent.clientConfig() + c.agent.logger.Printf("[DEBUG] agent: starting reload of client config") + if err != nil { + c.agent.logger.Printf("[ERR] agent: reloading client config failed: %v", err) + return + } + if err := c.agent.Client().Reload(clientConfig); err != nil { + c.agent.logger.Printf("[ERR] agent: reloading client config failed: %v", err) + return } } From ce9e93514c64fc86b2d128179f91c7005a21679d Mon Sep 17 00:00:00 2001 From: Chelsea Holland Komlo Date: Fri, 8 Jun 2018 14:33:58 -0400 Subject: [PATCH 2/5] move logic to determine whether to reload tls configuration to tlsutil helper --- helper/tlsutil/config.go | 22 ++++++++++++++++++++++ nomad/server.go | 7 +++---- 2 files changed, 25 insertions(+), 4 deletions(-) diff --git a/helper/tlsutil/config.go b/helper/tlsutil/config.go index 8202b08db..7bc3ac2e0 100644 --- a/helper/tlsutil/config.go +++ b/helper/tlsutil/config.go @@ -421,3 +421,25 @@ func ParseMinVersion(version string) (uint16, error) { return vers, nil } + +func ShouldReloadRPCConnections(old, new *config.TLSConfig) (bool, error) { + var tlsInfoEqual bool + + // If already configured with TLS, compare with the new TLS configuration + if new != nil { + var err error + tlsInfoEqual, err = new.CertificateInfoIsEqual(old) + if err != nil { + return false, err + } + } else { + // If not configured with TLS, compare with the new TLS configuration + tlsInfoEqual = new == nil && old == nil + } + + if new != nil && old != nil { + tlsInfoEqual = new.EnableRPC == old.EnableRPC + } + + return tlsInfoEqual, nil +} diff --git a/nomad/server.go b/nomad/server.go index 777ff24af..ff1733dfe 100644 --- a/nomad/server.go +++ b/nomad/server.go @@ -678,13 +678,12 @@ func (s *Server) Reload(newConfig *Config) error { } } - tlsInfoEqual, err := newConfig.TLSConfig.CertificateInfoIsEqual(s.config.TLSConfig) + shouldReloadTLS, err := tlsutil.ShouldReloadRPCConnections(s.config.TLSConfig, newConfig.TLSConfig) if err != nil { - s.logger.Printf("[ERR] nomad: error parsing server TLS configuration: %s", err) - return err + s.logger.Printf("[ERR] nomad: error checking whether to reload TLS configuration: %s", err) } - if !tlsInfoEqual || newConfig.TLSConfig.EnableRPC != s.config.TLSConfig.EnableRPC { + if shouldReloadTLS { if err := s.reloadTLSConnections(newConfig.TLSConfig); err != nil { s.logger.Printf("[ERR] nomad: error reloading server TLS configuration: %s", err) multierror.Append(&mErr, err) From 45ff58e40e1d176e3bd19e70d0aca27b92c46775 Mon Sep 17 00:00:00 2001 From: Chelsea Holland Komlo Date: Fri, 8 Jun 2018 14:38:58 -0400 Subject: [PATCH 3/5] add client logic to determine whether TLS RPC connections should reload --- client/client.go | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/client/client.go b/client/client.go index 0cc2d8111..e8ce99e7c 100644 --- a/client/client.go +++ b/client/client.go @@ -432,7 +432,17 @@ func (c *Client) reloadTLSConnections(newConfig *nconfig.TLSConfig) error { // Reload allows a client to reload its configuration on the fly func (c *Client) Reload(newConfig *config.Config) error { - return c.reloadTLSConnections(newConfig.TLSConfig) + shouldReloadTLS, err := tlsutil.ShouldReloadRPCConnections(c.config.TLSConfig, newConfig.TLSConfig) + if err != nil { + c.logger.Printf("[ERR] nomad: error parsing server TLS configuration: %s", err) + return err + } + + if shouldReloadTLS { + return c.reloadTLSConnections(newConfig.TLSConfig) + } + + return nil } // Leave is used to prepare the client to leave the cluster From cd8de515cc44a8f43d283119e9c32b4a27e918d5 Mon Sep 17 00:00:00 2001 From: Chelsea Holland Komlo Date: Fri, 8 Jun 2018 15:10:10 -0400 Subject: [PATCH 4/5] add tests and improve should reload logic --- helper/tlsutil/config.go | 18 ++++---- helper/tlsutil/config_test.go | 86 +++++++++++++++++++++++++++++++++++ 2 files changed, 96 insertions(+), 8 deletions(-) diff --git a/helper/tlsutil/config.go b/helper/tlsutil/config.go index 7bc3ac2e0..016ecbb1c 100644 --- a/helper/tlsutil/config.go +++ b/helper/tlsutil/config.go @@ -422,24 +422,26 @@ func ParseMinVersion(version string) (uint16, error) { return vers, nil } +// ShouldReloadRPCConnections compares two TLS Configurations and determines +// whether they differ such that RPC connections should be reloaded func ShouldReloadRPCConnections(old, new *config.TLSConfig) (bool, error) { - var tlsInfoEqual bool + var certificateInfoEqual bool + var rpcInfoEqual bool // If already configured with TLS, compare with the new TLS configuration if new != nil { var err error - tlsInfoEqual, err = new.CertificateInfoIsEqual(old) + certificateInfoEqual, err = new.CertificateInfoIsEqual(old) if err != nil { return false, err } - } else { - // If not configured with TLS, compare with the new TLS configuration - tlsInfoEqual = new == nil && old == nil + } else if new == nil && old == nil { + certificateInfoEqual = true } - if new != nil && old != nil { - tlsInfoEqual = new.EnableRPC == old.EnableRPC + if new != nil && old != nil && new.EnableRPC == old.EnableRPC { + rpcInfoEqual = true } - return tlsInfoEqual, nil + return (!rpcInfoEqual || !certificateInfoEqual), nil } diff --git a/helper/tlsutil/config_test.go b/helper/tlsutil/config_test.go index f02520510..52366e245 100644 --- a/helper/tlsutil/config_test.go +++ b/helper/tlsutil/config_test.go @@ -783,3 +783,89 @@ func TestConfig_NewTLSConfiguration(t *testing.T) { } require.Equal(tlsConf.CipherSuites, expectedCiphers) } + +func TestConfig_ShouldReloadRPCConnections(t *testing.T) { + require := require.New(t) + + type shouldReloadTestInput struct { + old *config.TLSConfig + new *config.TLSConfig + shouldReload bool + errorStr string + } + + testInput := []*shouldReloadTestInput{ + { + old: &config.TLSConfig{ + CAFile: cacert, + CertFile: badcert, + KeyFile: badkey, + }, + new: &config.TLSConfig{ + CAFile: cacert, + CertFile: badcert, + KeyFile: badkey, + }, + shouldReload: false, + errorStr: "Same TLS Configuration should not reload", + }, + { + old: &config.TLSConfig{ + CAFile: cacert, + CertFile: badcert, + KeyFile: badkey, + }, + new: &config.TLSConfig{ + CAFile: cacert, + CertFile: foocert, + KeyFile: fookey, + }, + shouldReload: true, + errorStr: "Different TLS Configuration should reload", + }, + { + old: &config.TLSConfig{ + CAFile: cacert, + CertFile: badcert, + KeyFile: badkey, + EnableRPC: true, + }, + new: &config.TLSConfig{ + CAFile: cacert, + CertFile: badcert, + KeyFile: badkey, + EnableRPC: false, + }, + shouldReload: true, + errorStr: "Downgrading RPC connections should force reload", + }, + { + old: nil, + new: &config.TLSConfig{ + CAFile: cacert, + CertFile: badcert, + KeyFile: badkey, + EnableRPC: true, + }, + shouldReload: true, + errorStr: "Upgrading RPC connections should force reload", + }, + { + old: &config.TLSConfig{ + CAFile: cacert, + CertFile: badcert, + KeyFile: badkey, + EnableRPC: true, + }, + new: nil, + shouldReload: true, + errorStr: "Downgrading RPC connections should force reload", + }, + } + + for _, testCase := range testInput { + shouldReload, err := ShouldReloadRPCConnections(testCase.old, testCase.new) + require.Nil(err) + require.Equal(shouldReload, testCase.shouldReload, testCase.errorStr) + } +} From 2cc252baa7c141abbc8bb3398e55060de9591713 Mon Sep 17 00:00:00 2001 From: Chelsea Holland Komlo Date: Wed, 13 Jun 2018 09:58:40 -0400 Subject: [PATCH 5/5] fixup! more specific test assertion --- helper/tlsutil/config_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/helper/tlsutil/config_test.go b/helper/tlsutil/config_test.go index 52366e245..8b408d763 100644 --- a/helper/tlsutil/config_test.go +++ b/helper/tlsutil/config_test.go @@ -865,7 +865,7 @@ func TestConfig_ShouldReloadRPCConnections(t *testing.T) { for _, testCase := range testInput { shouldReload, err := ShouldReloadRPCConnections(testCase.old, testCase.new) - require.Nil(err) + require.NoError(err) require.Equal(shouldReload, testCase.shouldReload, testCase.errorStr) } }