fixups from code review

command/agent/agent.go

@@ -727,7 +727,7 @@ func (a *Agent) Stats() map[string]map[string]string {
 // 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 {
-	if a.config != nil && newConfig.TLSConfig != nil {
+	if newConfig.TLSConfig != nil {
 
 		// If the agent is already running with TLS enabled, we need to only reload
 		// its certificates.

command/agent/agent_test.go

@@ -560,55 +560,16 @@ func TestAgent_HTTPCheckPath(t *testing.T) {
 	}
 }
 
-func TestServer_Reload_TLS_UpgradeToTLS(t *testing.T) {
+func TestServer_Reload_TLS_Certificate(t *testing.T) {
 	t.Parallel()
 	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"
-	)
-	dir := tmpDir(t)
-	defer os.RemoveAll(dir)
-
-	agentConfig := &Config{
-		TLSConfig: &sconfig.TLSConfig{
-			EnableHTTP: false,
-		},
-	}
-
-	agent := &Agent{
-		config: agentConfig,
-	}
-
-	newConfig := &Config{
-		TLSConfig: &sconfig.TLSConfig{
-			EnableHTTP:           true,
-			EnableRPC:            true,
-			VerifyServerHostname: true,
-			CAFile:               cafile,
-			CertFile:             foocert,
-			KeyFile:              fookey,
-		},
-	}
-
-	assert.Nil(agentConfig.TLSConfig.GetKeyLoader().Certificate)
-
-	err := agent.Reload(newConfig)
-	assert.Nil(err)
-
-	assert.NotNil(agentConfig.TLSConfig.GetKeyLoader().Certificate)
-}
-
-func TestServer_Reload_TLS_DowngradeFromTLS(t *testing.T) {
-	t.Parallel()
-	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"
+		cafile   = "../../helper/tlsutil/testdata/ca.pem"
+		foocert  = "../../helper/tlsutil/testdata/nomad-bad.pem"
+		fookey   = "../../helper/tlsutil/testdata/nomad-bad-key.pem"
+		foocert2 = "../../helper/tlsutil/testdata/nomad-foo.pem"
+		fookey2  = "../../helper/tlsutil/testdata/nomad-foo-key.pem"
 	)
 	dir := tmpDir(t)
 	defer os.RemoveAll(dir)
@@ -630,12 +591,17 @@ func TestServer_Reload_TLS_DowngradeFromTLS(t *testing.T) {
 
 	newConfig := &Config{
 		TLSConfig: &sconfig.TLSConfig{
-			EnableHTTP: false,
+			EnableHTTP:           true,
+			EnableRPC:            true,
+			VerifyServerHostname: true,
+			CAFile:               cafile,
+			CertFile:             foocert2,
+			KeyFile:              fookey2,
 		},
 	}
 
 	err := agent.Reload(newConfig)
 	assert.Nil(err)
-
-	assert.Nil(agentConfig.TLSConfig.GetKeyLoader().Certificate)
+	assert.Equal(agent.config.TLSConfig.CertFile, newConfig.TLSConfig.CertFile)
+	assert.Equal(agent.config.TLSConfig.KeyFile, newConfig.TLSConfig.KeyFile)
 }

command/agent/config.go

@@ -340,7 +340,6 @@ func (c *Config) UpdateTLSConfig(newConfig *config.TLSConfig) error {
 
 	keyLoader := c.TLSConfig.GetKeyLoader()
 	_, err := keyLoader.LoadKeyPair(newConfig.CertFile, newConfig.KeyFile)
-
 	if err != nil {
 		return err
 	}

helper/tlsutil/config.go

@@ -63,7 +63,7 @@ type Config struct {
 	// Must be provided to serve TLS connections.
 	KeyFile string
 
-	// Utility to dynamically reload TLS configuration.
+	// KeyLoader dynamically reloads TLS configuration.
 	KeyLoader *config.KeyLoader
 }
 

nomad/structs/config/tls.go

@@ -8,7 +8,6 @@ import (
 
 // TLSConfig provides TLS related configuration
 type TLSConfig struct {
-	configLock sync.Mutex
 
 	// EnableHTTP enabled TLS for http traffic to the Nomad server and clients
 	EnableHTTP bool `mapstructure:"http"`
@@ -35,6 +34,8 @@ type TLSConfig struct {
 	// KeyLoader is a helper to dynamically reload TLS configuration
 	KeyLoader *KeyLoader
 
+	keyloaderLock sync.Mutex
+
 	// KeyFile is used to provide a TLS key that is used for serving TLS connections.
 	// Must be provided to serve TLS connections.
 	KeyFile string `mapstructure:"key_file"`
@@ -50,15 +51,18 @@ type TLSConfig struct {
 
 type KeyLoader struct {
 	cacheLock   sync.Mutex
-	Certificate *tls.Certificate
+	certificate *tls.Certificate
 }
 
 // LoadKeyPair reloads the TLS certificate based on the specified certificate
 // and key file. If successful, stores the certificate for further use.
 func (k *KeyLoader) LoadKeyPair(certFile, keyFile string) (*tls.Certificate, error) {
+	k.cacheLock.Lock()
+	defer k.cacheLock.Unlock()
+
 	// Allow downgrading
 	if certFile == "" && keyFile == "" {
-		k.Certificate = nil
+		k.certificate = nil
 		return nil, nil
 	}
 
@@ -67,20 +71,25 @@ func (k *KeyLoader) LoadKeyPair(certFile, keyFile string) (*tls.Certificate, err
 		return nil, fmt.Errorf("Failed to load cert/key pair: %v", err)
 	}
 
-	k.cacheLock.Lock()
-	defer k.cacheLock.Unlock()
-
-	k.Certificate = &cert
-	return k.Certificate, nil
+	k.certificate = &cert
+	return k.certificate, nil
 }
 
+// GetOutgoingCertificate fetches the currently-loaded certificate. This
+// currently does not consider information in the ClientHello and only returns
+// the certificate that was last loaded.
 func (k *KeyLoader) GetOutgoingCertificate(*tls.ClientHelloInfo) (*tls.Certificate, error) {
-	return k.Certificate, nil
+	k.cacheLock.Lock()
+	defer k.cacheLock.Unlock()
+
+	return k.certificate, nil
 }
 
+// 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 {
-	t.configLock.Lock()
-	defer t.configLock.Unlock()
+	t.keyloaderLock.Lock()
+	defer t.keyloaderLock.Unlock()
 
 	// If the keyloader has not yet been initialized, do it here
 	if t.KeyLoader == nil {
@@ -92,13 +101,18 @@ 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 {
+
 	new := &TLSConfig{}
 	new.EnableHTTP = t.EnableHTTP
 	new.EnableRPC = t.EnableRPC
 	new.VerifyServerHostname = t.VerifyServerHostname
 	new.CAFile = t.CAFile
 	new.CertFile = t.CertFile
+
+	t.keyloaderLock.Lock()
 	new.KeyLoader = t.KeyLoader
+	t.keyloaderLock.Unlock()
+
 	new.KeyFile = t.KeyFile
 	new.RPCUpgradeMode = t.RPCUpgradeMode
 	new.VerifyHTTPSClient = t.VerifyHTTPSClient