Merge pull request moby#49305 from thaJeztah/cleanup_diagnosticserver

Assorted fixes,  improvements, and refactoring of  network diagnostic server

daemon/config/config.go

@@ -734,6 +734,9 @@ func Validate(config *Config) error {
 	if config.MaxDownloadAttempts < 0 {
 		return errors.Errorf("invalid max download attempts: %d", config.MaxDownloadAttempts)
 	}
+	if config.NetworkDiagnosticPort < 0 || config.NetworkDiagnosticPort > 65535 {
+		return errors.Errorf("invalid network-diagnostic-port (%d): value must be between 0 and 65535", config.NetworkDiagnosticPort)
+	}
 
 	if _, err := ParseGenericResources(config.NodeGenericResources); err != nil {
 		return err

daemon/config/config_test.go

@@ -318,6 +318,24 @@ func TestValidateConfigurationErrors(t *testing.T) {
 				expectedErr: "invalid max download attempts: 0",
 			},
 		*/
+		{
+			name: "negative network-diagnostic-port",
+			config: &Config{
+				CommonConfig: CommonConfig{
+					NetworkDiagnosticPort: -1,
+				},
+			},
+			expectedErr: "invalid network-diagnostic-port (-1): value must be between 0 and 65535",
+		},
+		{
+			name: "network-diagnostic-port out of range",
+			config: &Config{
+				CommonConfig: CommonConfig{
+					NetworkDiagnosticPort: 65536,
+				},
+			},
+			expectedErr: "invalid network-diagnostic-port (65536): value must be between 0 and 65535",
+		},
 		{
 			name: "generic resource without =",
 			config: &Config{

daemon/reload.go

@@ -266,16 +266,14 @@ func (daemon *Daemon) reloadLiveRestore(txn *reloadTxn, newCfg *configStore, con
 // reloadNetworkDiagnosticPort updates the network controller starting the diagnostic if the config is valid
 func (daemon *Daemon) reloadNetworkDiagnosticPort(txn *reloadTxn, newCfg *configStore, conf *config.Config, attributes map[string]string) error {
 	txn.OnCommit(func() error {
-		if conf == nil || daemon.netController == nil || !conf.IsValueSet("network-diagnostic-port") ||
-			conf.NetworkDiagnosticPort < 1 || conf.NetworkDiagnosticPort > 65535 {
+		if conf == nil || daemon.netController == nil || !conf.IsValueSet("network-diagnostic-port") || conf.NetworkDiagnosticPort == 0 {
 			// If there is no config make sure that the diagnostic is off
 			if daemon.netController != nil {
 				daemon.netController.StopDiagnostic()
 			}
 			return nil
 		}
 		// Enable the network diagnostic if the flag is set with a valid port within the range
-		log.G(context.TODO()).WithFields(log.Fields{"port": conf.NetworkDiagnosticPort, "ip": "127.0.0.1"}).Warn("Starting network diagnostic server")
 		daemon.netController.StartDiagnostic(conf.NetworkDiagnosticPort)
 		return nil
 	})

libnetwork/agent.go

@@ -332,7 +332,7 @@ func (c *Controller) agentInit(listenAddr, bindAddrOrInterface, advertiseAddr, d
 	}
 
 	// Register the diagnostic handlers
-	nDB.RegisterDiagnosticHandlers(c.DiagnosticServer)
+	nDB.RegisterDiagnosticHandlers(c.diagnosticServer)
 
 	var cancelList []func()
 	ch, cancel := nDB.Watch(libnetworkEPTable, "")

libnetwork/cmd/networkdb-test/dbserver/ndbServer.go

@@ -62,7 +62,7 @@ func Server(args []string) {
 	nDB.RegisterDiagnosticHandlers(server)
 	server.HandleFunc("/myip", ipaddress)
 	dummyclient.RegisterDiagnosticHandlers(server, nDB)
-	server.EnableDiagnostic("", port)
+	server.Enable("", port)
 	// block here
 	select {}
 }

libnetwork/controller.go

@@ -97,7 +97,7 @@ type Controller struct {
 	agentInitDone    chan struct{}
 	agentStopDone    chan struct{}
 	keys             []*types.EncryptionKey
-	DiagnosticServer *diagnostic.Server
+	diagnosticServer *diagnostic.Server
 	mu               sync.Mutex
 
 	// FIXME(thaJeztah): defOsSbox is always nil on non-Linux: move these fields to Linux-only files.
@@ -122,7 +122,7 @@ func New(cfgOptions ...config.Option) (*Controller, error) {
 		serviceBindings:  make(map[serviceKey]*service),
 		agentInitDone:    make(chan struct{}),
 		networkLocker:    locker.New(),
-		DiagnosticServer: diagnostic.New(),
+		diagnosticServer: diagnostic.New(),
 	}
 
 	c.drvRegistry.Notify = c
@@ -1093,25 +1093,15 @@ func (c *Controller) Stop() {
 
 // StartDiagnostic starts the network diagnostic server listening on port.
 func (c *Controller) StartDiagnostic(port int) {
-	c.mu.Lock()
-	if !c.DiagnosticServer.IsDiagnosticEnabled() {
-		c.DiagnosticServer.EnableDiagnostic("127.0.0.1", port)
-	}
-	c.mu.Unlock()
+	c.diagnosticServer.Enable("127.0.0.1", port)
 }
 
 // StopDiagnostic stops the network diagnostic server.
 func (c *Controller) StopDiagnostic() {
-	c.mu.Lock()
-	if c.DiagnosticServer.IsDiagnosticEnabled() {
-		c.DiagnosticServer.DisableDiagnostic()
-	}
-	c.mu.Unlock()
+	c.diagnosticServer.Shutdown()
 }
 
 // IsDiagnosticEnabled returns true if the diagnostic server is running.
 func (c *Controller) IsDiagnosticEnabled() bool {
-	c.mu.Lock()
-	defer c.mu.Unlock()
-	return c.DiagnosticServer.IsDiagnosticEnabled()
+	return c.diagnosticServer.Enabled()
 }

libnetwork/diagnostic/server.go

@@ -70,21 +70,24 @@ func (s *Server) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 	s.mux.ServeHTTP(w, r)
 }
 
-// EnableDiagnostic opens a TCP socket to debug the passed network DB
-func (s *Server) EnableDiagnostic(ip string, port int) {
+// Enable opens a TCP socket to debug the passed network DB
+func (s *Server) Enable(ip string, port int) {
 	s.mu.Lock()
 	defer s.mu.Unlock()
 
 	s.port = port
+	log.G(context.TODO()).WithFields(log.Fields{"port": s.port, "ip": ip}).Warn("Starting network diagnostic server")
 
+	// FIXME(thaJeztah): this check won't allow re-configuring the port on reload.
 	if s.enable {
-		log.G(context.TODO()).Info("The server is already up and running")
+		log.G(context.TODO()).WithFields(log.Fields{"port": s.port, "ip": ip}).Info("Network diagnostic server is already up and running")
 		return
 	}
 
-	log.G(context.TODO()).Infof("Starting the diagnostic server listening on %d for commands", port)
+	addr := net.JoinHostPort(ip, strconv.Itoa(s.port))
+	log.G(context.TODO()).WithFields(log.Fields{"port": s.port, "ip": ip}).Infof("Starting network diagnostic server listening on %s for commands", addr)
 	srv := &http.Server{
-		Addr:              net.JoinHostPort(ip, strconv.Itoa(port)),
+		Addr:              addr,
 		Handler:           s,
 		ReadHeaderTimeout: 5 * time.Minute, // "G112: Potential Slowloris Attack (gosec)"; not a real concern for our use, so setting a long timeout.
 	}
@@ -101,19 +104,25 @@ func (s *Server) EnableDiagnostic(ip string, port int) {
 	}(s)
 }
 
-// DisableDiagnostic stop the debug and closes the tcp socket
-func (s *Server) DisableDiagnostic() {
+// Shutdown stop the debug and closes the tcp socket
+func (s *Server) Shutdown() {
 	s.mu.Lock()
 	defer s.mu.Unlock()
-
-	s.srv.Shutdown(context.Background()) //nolint:errcheck
-	s.srv = nil
+	if !s.enable {
+		return
+	}
+	if s.srv != nil {
+		if err := s.srv.Shutdown(context.Background()); err != nil {
+			log.G(context.TODO()).WithError(err).Warn("Error during network diagnostic server shutdown")
+		}
+		s.srv = nil
+	}
 	s.enable = false
-	log.G(context.TODO()).Info("Disabling the diagnostic server")
+	log.G(context.TODO()).Info("Network diagnostic server shutdown complete")
 }
 
-// IsDiagnosticEnabled returns true when the debug is enabled
-func (s *Server) IsDiagnosticEnabled() bool {
+// Enabled returns true when the debug is enabled
+func (s *Server) Enabled() bool {
 	s.mu.Lock()
 	defer s.mu.Unlock()
 	return s.enable
@@ -176,15 +185,16 @@ func stackTrace(w http.ResponseWriter, r *http.Request) {
 
 	// audit logs
 	logger := log.G(context.TODO()).WithFields(log.Fields{"component": "diagnostic", "remoteIP": r.RemoteAddr, "method": caller.Name(0), "url": r.URL.String()})
-	logger.Info("stack trace")
+	logger.Info("collecting stack trace")
 
+	// FIXME(thaJeztah): make path configurable, or use same location as used by daemon.setupDumpStackTrap
 	path, err := stack.DumpToFile("/tmp/")
 	if err != nil {
-		logger.WithError(err).Error("failed to write goroutines dump")
+		logger.WithError(err).Error("failed to write stack trace to file")
 		_, _ = HTTPReply(w, FailCommand(err), jsonOutput)
 	} else {
-		logger.Info("stack trace done")
-		_, _ = HTTPReply(w, CommandSucceed(&StringCmd{Info: "goroutine stacks written to " + path}), jsonOutput)
+		logger.WithField("file", path).Info("wrote stack trace to file")
+		_, _ = HTTPReply(w, CommandSucceed(&StringCmd{Info: "goroutine stacks written to " + path + "\n"}), jsonOutput)
 	}
 }