add lock for config updates; improve copy of tls config

CHANGELOG.md

@@ -3,11 +3,13 @@
 __BACKWARDS INCOMPATIBILITIES:__
  * config: Nomad no longer parses Atlas configuration stanzas. Atlas has been
    deprecated since earlier this year. If you have an Atlas stanza in your
-   config file it will have to be removed. 
+   config file it will have to be removed.
 
 IMPROVEMENTS:
  * core: Allow agents to be run in `rpc_upgrade_mode` when migrating a cluster
    to TLS rather than changing `heartbeat_grace`
+ * core: Allow operators to reload TLS certificate and key files via SIGHUP
+   #3479
  * api: Allocations now track and return modify time in addition to create time
    [GH-3446]
  * api: Introduced new fields to track details and display message for task

command/agent/command.go

@@ -558,7 +558,7 @@ WAIT:
 	// Check if this is a SIGHUP
 	if sig == syscall.SIGHUP {
 		if conf := c.handleReload(config); conf != nil {
-			*config = *conf
+			config = conf.Copy()
 		}
 		goto WAIT
 	}

command/agent/config.go

@@ -12,6 +12,7 @@ import (
 	"sort"
 	"strconv"
 	"strings"
+	"sync"
 	"time"
 
 	"github.com/hashicorp/go-sockaddr/template"
@@ -124,6 +125,8 @@ 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"`
@@ -132,6 +135,47 @@ 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
@@ -334,6 +378,9 @@ type ServerConfig struct {
 // 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")
 	}
@@ -649,7 +696,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
+	result := c.Copy()
 
 	if b.Region != "" {
 		result.Region = b.Region
@@ -701,8 +748,7 @@ func (c *Config) Merge(b *Config) *Config {
 
 	// Apply the TLS Config
 	if result.TLSConfig == nil && b.TLSConfig != nil {
-		tlsConfig := *b.TLSConfig
-		result.TLSConfig = &tlsConfig
+		result.TLSConfig = b.TLSConfig.Copy()
 	} else if b.TLSConfig != nil {
 		result.TLSConfig = result.TLSConfig.Merge(b.TLSConfig)
 	}
@@ -789,7 +835,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,6 +11,7 @@ import (
 
 	"github.com/hashicorp/nomad/nomad/structs"
 	"github.com/hashicorp/nomad/nomad/structs/config"
+	"github.com/stretchr/testify/assert"
 )
 
 var (
@@ -871,3 +872,93 @@ 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)
+}

nomad/structs/config/tls.go

@@ -85,6 +85,16 @@ func (k *KeyLoader) GetOutgoingCertificate(*tls.ClientHelloInfo) (*tls.Certifica
 	return k.certificate, nil
 }
 
+func (k *KeyLoader) Copy() *KeyLoader {
+	if k == nil {
+		return nil
+	}
+
+	new := KeyLoader{}
+	new.certificate = k.certificate
+	return &new
+}
+
 // GetKeyLoader returns the keyloader for a TLSConfig object. If the keyloader
 // has not been initialized, it will first do so.
 func (t *TLSConfig) GetKeyLoader() *KeyLoader {
@@ -101,6 +111,9 @@ func (t *TLSConfig) GetKeyLoader() *KeyLoader {
 // Copy copies the fields of TLSConfig to another TLSConfig object. Required as
 // to not copy mutexes between objects.
 func (t *TLSConfig) Copy() *TLSConfig {
+	if t == nil {
+		return t
+	}
 
 	new := &TLSConfig{}
 	new.EnableHTTP = t.EnableHTTP
@@ -110,7 +123,7 @@ func (t *TLSConfig) Copy() *TLSConfig {
 	new.CertFile = t.CertFile
 
 	t.keyloaderLock.Lock()
-	new.KeyLoader = t.KeyLoader
+	new.KeyLoader = t.KeyLoader.Copy()
 	t.keyloaderLock.Unlock()
 
 	new.KeyFile = t.KeyFile