From 2cade96ae2d9fef657cf64f1fbd8f0e7a24b8526 Mon Sep 17 00:00:00 2001 From: Chelsea Holland Komlo Date: Mon, 13 Nov 2017 18:15:52 -0500 Subject: [PATCH] add lock for config updates; improve copy of tls config --- CHANGELOG.md | 4 +- command/agent/command.go | 2 +- command/agent/config.go | 54 +++++++++++++++++++-- command/agent/config_test.go | 91 ++++++++++++++++++++++++++++++++++++ nomad/structs/config/tls.go | 15 +++++- 5 files changed, 159 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c4755426751c..382fb8dbf9fc 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/command/agent/command.go b/command/agent/command.go index 7c703e722765..69b3e9a6e1d5 100644 --- a/command/agent/command.go +++ b/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 } diff --git a/command/agent/config.go b/command/agent/config.go index f5915585030f..1f093d936192 100644 --- a/command/agent/config.go +++ b/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 diff --git a/command/agent/config_test.go b/command/agent/config_test.go index 0324be6fdaa1..dec7b261e67c 100644 --- a/command/agent/config_test.go +++ b/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) +} diff --git a/nomad/structs/config/tls.go b/nomad/structs/config/tls.go index 48b5f7e597db..e1793021f492 100644 --- a/nomad/structs/config/tls.go +++ b/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