config updates/copies should be on agent

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

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

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
 	}

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

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