reload http server if tls configuration changes

client/client.go

@@ -30,6 +30,7 @@ import (
 	"github.com/hashicorp/nomad/helper/uuid"
 	"github.com/hashicorp/nomad/nomad"
 	"github.com/hashicorp/nomad/nomad/structs"
+	nconfig "github.com/hashicorp/nomad/nomad/structs/config"
 	vaultapi "github.com/hashicorp/vault/api"
 	"github.com/mitchellh/hashstructure"
 	"github.com/shirou/gopsutil/host"
@@ -304,10 +305,12 @@ func NewClient(cfg *config.Config, consulCatalog consul.CatalogAPI, consulServic
 
 // ReloadTLSConnectoins allows a client to reload RPC connections if the
 // client's TLS configuration changes from plaintext to TLS
-func (c *Client) ReloadTLSConnections() error {
+func (c *Client) ReloadTLSConnections(newConfig *nconfig.TLSConfig) error {
 	c.configLock.Lock()
 	defer c.configLock.Unlock()
 
+	c.config.TLSConfig = newConfig
+
 	if c.config.TLSConfig.EnableRPC {
 		tw, err := c.config.TLSConfiguration().OutgoingTLSWrapper()
 		if err != nil {

client/client_test.go

@@ -2,7 +2,6 @@ package client
 
 import (
 	"fmt"
-	"io"
 	"io/ioutil"
 	"log"
 	"math/rand"
@@ -463,7 +462,6 @@ func TestClient_MixedTLS(t *testing.T) {
 
 func TestClient_ReloadTLS(t *testing.T) {
 	t.Parallel()
-	t.Skip("depends on closing non-tls channels on ReloadTLSConnections")
 	assert := assert.New(t)
 
 	s1, addr := testServer(t, func(c *nomad.Config) {
@@ -480,18 +478,19 @@ func TestClient_ReloadTLS(t *testing.T) {
 
 	c1 := testClient(t, func(c *config.Config) {
 		c.Servers = []string{addr}
-		c.TLSConfig = &nconfig.TLSConfig{
-			EnableHTTP:           true,
-			EnableRPC:            true,
-			VerifyServerHostname: true,
-			CAFile:               cafile,
-			CertFile:             foocert,
-			KeyFile:              fookey,
-		}
 	})
 	defer c1.Shutdown()
 
-	err := c1.ReloadTLSConnections()
+	newConfig := &nconfig.TLSConfig{
+		EnableHTTP:           true,
+		EnableRPC:            true,
+		VerifyServerHostname: true,
+		CAFile:               cafile,
+		CertFile:             foocert,
+		KeyFile:              fookey,
+	}
+
+	err := c1.ReloadTLSConnections(newConfig)
 	assert.Nil(err)
 
 	req := structs.NodeSpecificRequest{
@@ -502,8 +501,8 @@ func TestClient_ReloadTLS(t *testing.T) {
 	testutil.AssertUntil(100*time.Millisecond,
 		func() (bool, error) {
 			err := c1.RPC("Node.GetNode", &req, &out)
-			if err != io.EOF {
-				return false, fmt.Errorf("client RPC succeeded when it should have failed:\n%+v", out)
+			if err == nil {
+				return false, fmt.Errorf("client RPC succeeded when it should have failed:\n%+v", err)
 			}
 			return true, nil
 		},

command/agent/agent.go

@@ -726,48 +726,51 @@ 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 the agent is already running with TLS enabled and the new
-	// configuration specifies a TLS configuration, we need to only reload
-	// its certificates.
+func (a *Agent) Reload(newConfig *Config) (error, bool) {
+	if newConfig == nil || newConfig.TLSConfig == nil {
+		return fmt.Errorf("cannot reload agent with nil configuration"), false
+	}
+
+	// Don't reload if there is no change in the TLS configuration
+	if !a.config.TLSConfig.Equals(newConfig.TLSConfig) {
 
-	if newConfig.TLSConfig != nil {
+		// This is just a TLS configuration reload, we don't need to refresh
+		// existing network connections
 		if !a.config.TLSConfig.IsEmpty() && !newConfig.TLSConfig.IsEmpty() {
 			a.logger.Println("[INFO] Updating agent's existing TLS configuration")
-			// Handle errors in loading the new certificate files.
-			// This is just a TLS configuration reload, we don't need to refresh
-			// existing network connections
-			return a.config.UpdateTLSConfig(newConfig.TLSConfig)
+			return a.config.UpdateTLSConfig(newConfig.TLSConfig), true
 		}
 
-		// Completely reload the agent's TLS configuration.
+		// Completely reload the agent's TLS configuration (moving from non-TLS to
+		// TLS, or vice versa)
 		// This does not handle errors in loading the new TLS configuration
-		a.config.TLSConfig = newConfig.TLSConfig
+		a.config.TLSConfig = newConfig.TLSConfig.Copy()
 
-		if a.config.TLSConfig.IsEmpty() && !newConfig.TLSConfig.IsEmpty() {
-			a.logger.Println("[INFO] Upgrading from plaintext configuration to TLS")
-		} else if !a.config.TLSConfig.IsEmpty() && newConfig.TLSConfig.IsEmpty() {
+		if newConfig.TLSConfig.IsEmpty() {
 			a.logger.Println("[WARN] Downgrading agent's existing TLS configuration to plaintext")
+		} else {
+			a.logger.Println("[INFO] Upgrading from plaintext configuration to TLS")
 		}
 
 		// Reload the TLS configuration for the client or server, depending on how
 		// the agent is configured to run.
 		if s := a.Server(); s != nil {
-			err := s.ReloadTLSConnections()
+			err := s.ReloadTLSConnections(a.config.TLSConfig)
 			if err != nil {
 				a.logger.Printf("[WARN] agent: Issue reloading the server's TLS Configuration, consider a full system restart: %v", err.Error())
-				return err
+				return err, false
 			}
 		} else if c := a.Client(); c != nil {
 			err := c.ReloadTLSConnections()
 			if err != nil {
 				a.logger.Printf("[ERR] agent: Issue reloading the client's TLS Configuration, consider a full system restart: %v", err.Error())
-				return err
+				return err, false
 			}
 		}
+		return nil, true
 	}
 
-	return nil
+	return nil, false
 }
 
 // setupConsul creates the Consul client and starts its main Run loop.

command/agent/agent_test.go

@@ -560,6 +560,23 @@ func TestAgent_HTTPCheckPath(t *testing.T) {
 	}
 }
 
+func TestServer_Reload_TLS_WithNilConfiguration(t *testing.T) {
+	t.Parallel()
+	assert := assert.New(t)
+
+	logger := log.New(ioutil.Discard, "", 0)
+
+	agent := &Agent{
+		logger: logger,
+		config: &Config{},
+	}
+
+	err, reloadHTTP := agent.Reload(nil)
+	assert.NotNil(err)
+	assert.Equal(false, reloadHTTP)
+	assert.Equal(err.Error(), "cannot reload agent with nil configuration")
+}
+
 func TestServer_Reload_TLS_UpgradeToTLS(t *testing.T) {
 	t.Parallel()
 	assert := assert.New(t)
@@ -596,8 +613,9 @@ func TestServer_Reload_TLS_UpgradeToTLS(t *testing.T) {
 
 	assert.Nil(agentConfig.TLSConfig.GetKeyLoader().Certificate)
 
-	err := agent.Reload(newConfig)
+	err, reloadHTTP := agent.Reload(newConfig)
 	assert.Nil(err)
+	assert.True(reloadHTTP)
 
 	assert.Equal(agent.config.TLSConfig.CAFile, newConfig.TLSConfig.CAFile)
 	assert.Equal(agent.config.TLSConfig.CertFile, newConfig.TLSConfig.CertFile)
@@ -640,8 +658,9 @@ func TestServer_Reload_TLS_DowngradeFromTLS(t *testing.T) {
 
 	assert.False(agentConfig.TLSConfig.IsEmpty())
 
-	err := agent.Reload(newConfig)
+	err, reloadHTTP := agent.Reload(newConfig)
 	assert.Nil(err)
+	assert.True(reloadHTTP)
 
 	assert.True(agentConfig.TLSConfig.IsEmpty())
 }

command/agent/command.go

@@ -621,12 +621,31 @@ func (c *Command) handleReload(config *Config) *Config {
 		newConf.LogLevel = config.LogLevel
 	}
 
+	err := c.httpServer.Shutdown()
+	if err != nil {
+		c.agent.logger.Printf("[ERR] agent: failed to stop HTTP server: %v", err)
+		return nil
+	}
+	time.Sleep(5 * time.Second)
+
 	// Reloads configuration for an agent running in both client and server mode
-	err := c.agent.Reload(newConf)
+	// TODO remove bool to reload HTTP Serv
+	err, _ = c.agent.Reload(newConf)
 	if err != nil {
-		c.agent.logger.Printf("[ERR] agent: failed to reload the config: %v", err)
+		c.agent.logger.Printf("[ERR] agent: failed to reload agent configuration : %v", err)
+		return nil
 	}
 
+	// Wait some time to ensure a clean shutdown
+	time.Sleep(5 * time.Second)
+	http, err := NewHTTPServer(c.agent, c.agent.config)
+	if err != nil {
+		c.agent.logger.Printf("[ERR] agent: failed to reload http server: %v", err)
+		return nil
+	}
+	c.agent.logger.Println("[INFO] agent: successfully restarted the HTTP server")
+	c.httpServer = http
+
 	// If the configuration change is specific only to the server, handle it here
 	if s := c.agent.Server(); s != nil {
 		sconf, err := convertServerConfig(newConf, c.logOutput)
@@ -642,6 +661,24 @@ func (c *Command) handleReload(config *Config) *Config {
 	return newConf
 }
 
+func (c *Command) reloadHTTPServerOnConfigChange(newConfig *Config) error {
+	c.agent.logger.Println("[INFO] agent: Reloading HTTP server with new TLS configuration")
+	err := c.httpServer.Shutdown()
+	if err != nil {
+		return err
+	}
+
+	// Wait some time to ensure a clean shutdown
+	time.Sleep(5 * time.Second)
+	http, err := NewHTTPServer(c.agent, c.agent.config)
+	if err != nil {
+		return err
+	}
+	c.httpServer = http
+
+	return nil
+}
+
 // setupTelemetry is used ot setup the telemetry sub-systems
 func (c *Command) setupTelemetry(config *Config) (*metrics.InmemSink, error) {
 	/* Setup telemetry

command/agent/config.go

@@ -702,8 +702,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)
 	}

command/agent/http.go

@@ -126,11 +126,12 @@ func (ln tcpKeepAliveListener) Accept() (c net.Conn, err error) {
 }
 
 // Shutdown is used to shutdown the HTTP server
-func (s *HTTPServer) Shutdown() {
+func (s *HTTPServer) Shutdown() error {
 	if s != nil {
 		s.logger.Printf("[DEBUG] http: Shutting down http server")
-		s.listener.Close()
+		return s.listener.Close()
 	}
+	return nil
 }
 
 // registerHandlers is used to attach our handlers to the mux

command/agent/http_test.go

@@ -564,7 +564,7 @@ func TestHTTP_VerifyHTTPSClient_AfterConfigReload(t *testing.T) {
 	assert.Nil(err)
 
 	// Next, reload the TLS configuration
-	err = s.Agent.Reload(newConfig)
+	err, _ = s.Agent.Reload(newConfig)
 	assert.Nil(err)
 
 	// PASS: Requests that specify a valid hostname, CA cert, and client

nomad/pool.go

@@ -161,6 +161,12 @@ func NewPool(logOutput io.Writer, maxTime time.Duration, maxStreams int, tlsWrap
 func (p *ConnPool) ReloadTLS(tlsWrap tlsutil.RegionWrapper) {
 	p.Lock()
 	defer p.Unlock()
+
+	oldPool := p.pool
+	for _, conn := range oldPool {
+		conn.Close()
+	}
+	p.pool = make(map[string]*Conn)
 	p.tlsWrap = tlsWrap
 }
 

nomad/raft_rpc.go

@@ -45,7 +45,14 @@ func NewRaftLayer(addr net.Addr, tlsWrap tlsutil.Wrapper) *RaftLayer {
 func (l *RaftLayer) ReloadTLS(tlsWrap tlsutil.Wrapper) {
 	l.closeLock.Lock()
 	defer l.closeLock.Unlock()
+
+	if !l.closed {
+		l.closed = true
+		close(l.closeCh)
+	}
+
 	l.tlsWrap = tlsWrap
+	l.closeCh = make(chan struct{})
 }
 
 // Handoff is used to hand off a connection to the

nomad/rpc.go

@@ -68,8 +68,15 @@ func NewServerCodec(conn io.ReadWriteCloser) rpc.ServerCodec {
 }
 
 // listen is used to listen for incoming RPC connections
-func (s *Server) listen() {
+func (s *Server) listen(ctx context.Context) {
 	for {
+		select {
+		case <-ctx.Done():
+			s.logger.Println("[INFO] nomad.rpc: Closing server RPC connection")
+			return
+		default:
+		}
+
 		// Accept a connection
 		conn, err := s.rpcListener.Accept()
 		if err != nil {
@@ -150,7 +157,9 @@ func (s *Server) handleMultiplex(conn net.Conn) {
 			}
 			return
 		}
+		// this should also take a context?
 		go s.handleNomadConn(sub)
+		//go s.handleNomadConn(ctx, sub)
 	}
 }
 
@@ -160,6 +169,7 @@ func (s *Server) handleNomadConn(conn net.Conn) {
 	rpcCodec := NewServerCodec(conn)
 	for {
 		select {
+		// here we should handle the case of a connection closing
 		case <-s.shutdownCh:
 			return
 		default: