From 885c17509cf3492a738458df5e717e895614b129 Mon Sep 17 00:00:00 2001 From: Chelsea Holland Komlo Date: Tue, 14 Nov 2017 15:59:56 -0500 Subject: [PATCH] config updates/copies should be on agent --- command/agent/agent.go | 30 ++++++++++-- command/agent/agent_test.go | 70 +++++++++++++++++++++++++++ command/agent/command.go | 5 +- command/agent/config.go | 72 +--------------------------- command/agent/config_test.go | 91 ------------------------------------ 5 files changed, 101 insertions(+), 167 deletions(-) diff --git a/command/agent/agent.go b/command/agent/agent.go index 380483d2813d..5ffab01464c7 100644 --- a/command/agent/agent.go +++ b/command/agent/agent.go @@ -43,7 +43,9 @@ const ( // scheduled to, and are responsible for interfacing with // servers to run allocations. type Agent struct { - config *Config + config *Config + configLock sync.Mutex + logger *log.Logger logOutput io.Writer @@ -724,23 +726,43 @@ func (a *Agent) Stats() map[string]map[string]string { return stats } +func updateTLSConfig(newConfig *config.TLSConfig) (*config.TLSConfig, error) { + keyLoader := newConfig.GetKeyLoader() + _, err := keyLoader.LoadKeyPair(newConfig.CertFile, newConfig.KeyFile) + if err != nil { + return nil, err + } + + return newConfig, nil +} + // Reload handles configuration changes for the agent. Provides a method that // is easier to unit test, as this action is invoked via SIGHUP. func (a *Agent) Reload(newConfig *Config) error { + a.configLock.Lock() + defer a.configLock.Unlock() + if newConfig.TLSConfig != nil { - // If the agent is already running with TLS enabled, we need to only reload - // its certificates. // TODO(chelseakomlo) In a later PR, we will introduce the ability to reload // TLS configuration if the agent is not running with TLS enabled. if a.config.TLSConfig != nil { - return a.config.UpdateTLSConfig(newConfig.TLSConfig) + new, err := updateTLSConfig(newConfig.TLSConfig) + if err != nil { + return err + } + a.config.TLSConfig = new } } return nil } +// GetConfigCopy creates a replica of the agent's config, excluding locks +func (a *Agent) GetConfig() *Config { + return a.config +} + // setupConsul creates the Consul client and starts its main Run loop. func (a *Agent) setupConsul(consulConfig *config.ConsulConfig) error { apiConf, err := consulConfig.ApiConfig() diff --git a/command/agent/agent_test.go b/command/agent/agent_test.go index d26c3227a6c0..073c26966686 100644 --- a/command/agent/agent_test.go +++ b/command/agent/agent_test.go @@ -605,3 +605,73 @@ func TestServer_Reload_TLS_Certificate(t *testing.T) { assert.Equal(agent.config.TLSConfig.CertFile, newConfig.TLSConfig.CertFile) assert.Equal(agent.config.TLSConfig.KeyFile, newConfig.TLSConfig.KeyFile) } + +func TestServer_Reload_TLS_Certificate_Invalid(t *testing.T) { + t.Parallel() + assert := assert.New(t) + + const ( + cafile = "../../helper/tlsutil/testdata/ca.pem" + foocert = "../../helper/tlsutil/testdata/nomad-bad.pem" + fookey = "../../helper/tlsutil/testdata/nomad-bad-key.pem" + foocert2 = "invalid_cert_path" + fookey2 = "invalid_key_path" + ) + dir := tmpDir(t) + defer os.RemoveAll(dir) + + agentConfig := &Config{ + TLSConfig: &sconfig.TLSConfig{ + EnableHTTP: true, + EnableRPC: true, + VerifyServerHostname: true, + CAFile: cafile, + CertFile: foocert, + KeyFile: fookey, + }, + } + + agent := &Agent{ + config: agentConfig, + } + + newConfig := &Config{ + TLSConfig: &sconfig.TLSConfig{ + EnableHTTP: true, + EnableRPC: true, + VerifyServerHostname: true, + CAFile: cafile, + CertFile: foocert2, + KeyFile: fookey2, + }, + } + + err := agent.Reload(newConfig) + assert.NotNil(err) + assert.NotEqual(agent.config.TLSConfig.CertFile, newConfig.TLSConfig.CertFile) + assert.NotEqual(agent.config.TLSConfig.KeyFile, newConfig.TLSConfig.KeyFile) +} + +func Test_GetConfig(t *testing.T) { + assert := assert.New(t) + + agentConfig := &Config{ + Telemetry: &Telemetry{}, + Client: &ClientConfig{}, + Server: &ServerConfig{}, + ACL: &ACLConfig{}, + Ports: &Ports{}, + Addresses: &Addresses{}, + AdvertiseAddrs: &AdvertiseAddrs{}, + Vault: &sconfig.VaultConfig{}, + Consul: &sconfig.ConsulConfig{}, + Sentinel: &sconfig.SentinelConfig{}, + } + + agent := &Agent{ + config: agentConfig, + } + + actualAgentConfig := agent.GetConfig() + assert.Equal(actualAgentConfig, agentConfig) +} diff --git a/command/agent/command.go b/command/agent/command.go index 69b3e9a6e1d5..97a16c83ef0a 100644 --- a/command/agent/command.go +++ b/command/agent/command.go @@ -557,8 +557,9 @@ WAIT: // Check if this is a SIGHUP if sig == syscall.SIGHUP { - if conf := c.handleReload(config); conf != nil { - config = conf.Copy() + c.handleReload(config) + if c.agent.GetConfig() != nil { + *config = *c.agent.GetConfig() } goto WAIT } diff --git a/command/agent/config.go b/command/agent/config.go index 1f093d936192..d4407acf0864 100644 --- a/command/agent/config.go +++ b/command/agent/config.go @@ -12,7 +12,6 @@ import ( "sort" "strconv" "strings" - "sync" "time" "github.com/hashicorp/go-sockaddr/template" @@ -125,8 +124,6 @@ type Config struct { // client TLSConfig *config.TLSConfig `mapstructure:"tls"` - tlsConfigLock sync.Mutex - // HTTPAPIResponseHeaders allows users to configure the Nomad http agent to // set arbritrary headers on API responses HTTPAPIResponseHeaders map[string]string `mapstructure:"http_api_response_headers"` @@ -135,47 +132,6 @@ type Config struct { Sentinel *config.SentinelConfig `mapstructure:"sentinel"` } -// Copy creates a replica of the original config, excluding locks -func (c *Config) Copy() *Config { - if c == nil { - return nil - } - - new := Config{} - new.Region = c.Region - new.Datacenter = c.Datacenter - new.NodeName = c.NodeName - new.DataDir = c.DataDir - new.LogLevel = c.LogLevel - new.BindAddr = c.BindAddr - new.EnableDebug = c.EnableDebug - new.Ports = c.Ports - new.Addresses = c.Addresses - new.normalizedAddrs = c.normalizedAddrs - new.AdvertiseAddrs = c.AdvertiseAddrs - new.Client = c.Client - new.Server = c.Server - new.ACL = c.ACL - new.Telemetry = c.Telemetry - new.LeaveOnInt = c.LeaveOnInt - new.LeaveOnTerm = c.LeaveOnTerm - new.EnableSyslog = c.EnableSyslog - new.SyslogFacility = c.SyslogFacility - new.DisableUpdateCheck = c.DisableUpdateCheck - new.DisableAnonymousSignature = c.DisableAnonymousSignature - new.Consul = c.Consul - new.Vault = c.Vault - new.NomadConfig = c.NomadConfig - new.ClientConfig = c.ClientConfig - new.DevMode = c.DevMode - new.Version = c.Version - new.Files = c.Files - new.TLSConfig = c.TLSConfig.Copy() - new.HTTPAPIResponseHeaders = c.HTTPAPIResponseHeaders - new.Sentinel = c.Sentinel - return &new -} - // ClientConfig is configuration specific to the client mode type ClientConfig struct { // Enabled controls if we are a client @@ -372,30 +328,6 @@ type ServerConfig struct { EncryptKey string `mapstructure:"encrypt" json:"-"` } -// UpdateTLSConfig will reload an agent's TLS configuration. If there is an error -// while loading key and certificate files, the agent will remain at its -// current configuration and return an error. -// This only allows reloading the certificate and keyfile- other TLSConfig -// fields are ignored. -func (c *Config) UpdateTLSConfig(newConfig *config.TLSConfig) error { - c.tlsConfigLock.Lock() - defer c.tlsConfigLock.Unlock() - - if c.TLSConfig == nil { - return fmt.Errorf("unable to update non-existing TLSConfig") - } - - keyLoader := c.TLSConfig.GetKeyLoader() - _, err := keyLoader.LoadKeyPair(newConfig.CertFile, newConfig.KeyFile) - if err != nil { - return err - } - - c.TLSConfig.CertFile = newConfig.CertFile - c.TLSConfig.KeyFile = newConfig.KeyFile - return nil -} - // EncryptBytes returns the encryption key configured. func (s *ServerConfig) EncryptBytes() ([]byte, error) { return base64.StdEncoding.DecodeString(s.EncryptKey) @@ -696,7 +628,7 @@ func (c *Config) Listener(proto, addr string, port int) (net.Listener, error) { // Merge merges two configurations. func (c *Config) Merge(b *Config) *Config { - result := c.Copy() + result := *c if b.Region != "" { result.Region = b.Region @@ -835,7 +767,7 @@ func (c *Config) Merge(b *Config) *Config { result.HTTPAPIResponseHeaders[k] = v } - return result + return &result } // normalizeAddrs normalizes Addresses and AdvertiseAddrs to always be diff --git a/command/agent/config_test.go b/command/agent/config_test.go index dec7b261e67c..0324be6fdaa1 100644 --- a/command/agent/config_test.go +++ b/command/agent/config_test.go @@ -11,7 +11,6 @@ import ( "github.com/hashicorp/nomad/nomad/structs" "github.com/hashicorp/nomad/nomad/structs/config" - "github.com/stretchr/testify/assert" ) var ( @@ -872,93 +871,3 @@ func TestIsMissingPort(t *testing.T) { t.Errorf("expected no error, but got %v", err) } } - -func TestConfigCopy(t *testing.T) { - assert := assert.New(t) - - c1 := &Config{ - Telemetry: &Telemetry{}, - Client: &ClientConfig{}, - Server: &ServerConfig{}, - ACL: &ACLConfig{}, - Ports: &Ports{}, - Addresses: &Addresses{}, - AdvertiseAddrs: &AdvertiseAddrs{}, - Vault: &config.VaultConfig{}, - Consul: &config.ConsulConfig{}, - Sentinel: &config.SentinelConfig{}, - } - - expected := c1.Copy() - assert.Equal(expected, c1) -} - -func Test_UpdateTLS_Config_NoTLS(t *testing.T) { - assert := assert.New(t) - - c1 := &Config{ - Telemetry: &Telemetry{}, - Client: &ClientConfig{}, - Server: &ServerConfig{}, - ACL: &ACLConfig{}, - Ports: &Ports{}, - Addresses: &Addresses{}, - AdvertiseAddrs: &AdvertiseAddrs{}, - Vault: &config.VaultConfig{}, - Consul: &config.ConsulConfig{}, - Sentinel: &config.SentinelConfig{}, - } - - newTLSConfig := &config.TLSConfig{} - - err := c1.UpdateTLSConfig(newTLSConfig) - assert.NotNil(err) - assert.Contains(err.Error(), "unable to update non-existing TLSConfig") -} - -func Test_UpdateTLS_Config_TLS(t *testing.T) { - assert := assert.New(t) - - const ( - cafile = "../../helper/tlsutil/testdata/ca.pem" - foocert = "../../helper/tlsutil/testdata/nomad-foo.pem" - fookey = "../../helper/tlsutil/testdata/nomad-foo-key.pem" - foocert2 = "../../helper/tlsutil/testdata/nomad-bad.pem" - fookey2 = "../../helper/tlsutil/testdata/nomad-bad-key.pem" - ) - - c1 := &Config{ - Telemetry: &Telemetry{}, - Client: &ClientConfig{}, - Server: &ServerConfig{}, - ACL: &ACLConfig{}, - Ports: &Ports{}, - Addresses: &Addresses{}, - AdvertiseAddrs: &AdvertiseAddrs{}, - Vault: &config.VaultConfig{}, - Consul: &config.ConsulConfig{}, - Sentinel: &config.SentinelConfig{}, - TLSConfig: &config.TLSConfig{ - EnableHTTP: true, - EnableRPC: true, - VerifyServerHostname: true, - CAFile: cafile, - CertFile: foocert, - KeyFile: fookey, - }, - } - - newTLSConfig := &config.TLSConfig{ - EnableHTTP: true, - EnableRPC: true, - VerifyServerHostname: true, - CAFile: cafile, - CertFile: foocert2, - KeyFile: fookey2, - } - - err := c1.UpdateTLSConfig(newTLSConfig) - assert.Nil(err) - assert.Equal(c1.TLSConfig.CertFile, newTLSConfig.CertFile) - assert.Equal(c1.TLSConfig.KeyFile, newTLSConfig.KeyFile) -}