reload http server if tls configuration changes

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
+	// wait some amount of time, 500 milliseconds
+	for _, conn := range oldPool {
+		conn.Close()
+	}
 	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:

nomad/server.go

@@ -1,6 +1,7 @@
 package nomad
 
 import (
+	"context"
 	"crypto/tls"
 	"errors"
 	"fmt"
@@ -26,6 +27,7 @@ import (
 	"github.com/hashicorp/nomad/nomad/deploymentwatcher"
 	"github.com/hashicorp/nomad/nomad/state"
 	"github.com/hashicorp/nomad/nomad/structs"
+	"github.com/hashicorp/nomad/nomad/structs/config"
 	"github.com/hashicorp/raft"
 	raftboltdb "github.com/hashicorp/raft-boltdb"
 	"github.com/hashicorp/serf/serf"
@@ -87,6 +89,7 @@ type Server struct {
 
 	// Connection pool to other Nomad servers
 	connPool *ConnPool
+	//connPoolContext context.Context
 
 	// Endpoints holds our RPC endpoints
 	endpoints endpoints
@@ -109,7 +112,8 @@ type Server struct {
 	rpcAdvertise net.Addr
 
 	// rpcTLS is the TLS config for incoming TLS requests
-	rpcTLS *tls.Config
+	rpcTLS    *tls.Config
+	rpcCancel context.CancelFunc
 
 	// peers is used to track the known Nomad servers. This is
 	// used for region forwarding and clustering.
@@ -329,7 +333,9 @@ func NewServer(config *Config, consulCatalog consul.CatalogAPI, logger *log.Logg
 	go s.serfEventHandler()
 
 	// Start the RPC listeners
-	go s.listen()
+	ctx, cancel := context.WithCancel(context.Background())
+	s.rpcCancel = cancel
+	go s.listen(ctx)
 
 	// Emit metrics for the eval broker
 	go evalBroker.EmitStats(time.Second, s.shutdownCh)
@@ -355,10 +361,10 @@ func NewServer(config *Config, consulCatalog consul.CatalogAPI, logger *log.Logg
 
 // ReloadTLSConnections will completely reload the server's RPC connections if
 // the server is moving from a non-TLS to TLS connection, or vice versa.
-func (s *Server) ReloadTLSConnections() error {
-	s.logger.Printf("[INFO] nomad: reloading server network connections due to server configuration changes")
+func (s *Server) ReloadTLSConnections(newTLSConfig *config.TLSConfig) error {
+	s.logger.Printf("[INFO] nomad: reloading server connections due to configuration changes")
+	s.config.TLSConfig = newTLSConfig
 
-	// Configure TLS wrapper
 	var tlsWrap tlsutil.RegionWrapper
 	var incomingTLS *tls.Config
 	if s.config.TLSConfig.EnableRPC {
@@ -376,15 +382,32 @@ func (s *Server) ReloadTLSConnections() error {
 		incomingTLS = itls
 	}
 
-	// Reset the server's rpcTLS configuration
-	s.rpcTLS = incomingTLS
+	if s.rpcCancel == nil {
+		return fmt.Errorf("unable to reset tls context")
+	}
 
-	// Reload TLS configuration in the server's conn pool
+	s.rpcCancel()
 	s.connPool.ReloadTLS(tlsWrap)
+	time.Sleep(500 * time.Millisecond)
+
+	s.rpcTLS = incomingTLS
+
+	s.rpcListener.Close()
+	time.Sleep(500 * time.Millisecond)
+	list, err := net.ListenTCP("tcp", s.config.RPCAddr)
+	if err != nil || list == nil {
+		return err
+	}
+	time.Sleep(500 * time.Millisecond)
+	s.rpcListener = list
+
+	ctx, cancel := context.WithCancel(context.Background())
+	s.rpcCancel = cancel
+	go s.listen(ctx)
 
-	// Reload TLS configuration for the server's raft layer
 	wrapper := tlsutil.RegionSpecificWrapper(s.config.Region, tlsWrap)
 	s.raftLayer.ReloadTLS(wrapper)
+	time.Sleep(500 * time.Millisecond)
 
 	return nil
 }