[signal] Stop with an error when a server fails (#8105)

Signal can report a successful start and then stop with exit code 0, or
continue without a listener, when a listener fails. Supervisors and
operators do not see the failure.

Stop at startup when the Let's Encrypt challenge listener cannot bind.
Keep a failure that occurs before the run loop waits, and return a
non-zero exit code after a server failure. A listener that is closed
during shutdown is not a failure.

signal/cmd/root.go

@@ -38,7 +38,8 @@ func Execute() error {
 }
 
 func init() {
-	stopCh = make(chan int)
+	// Buffered so a server failure before the run loop waits is not dropped.
+	stopCh = make(chan int, 1)
 	defaultLogFile = "/var/log/netbird/signal.log"
 
 	if runtime.GOOS == "windows" {

signal/cmd/run.go

@@ -140,7 +140,7 @@ var (
 			default:
 				certListener, err = startServerWithCertManager(certManager, grpcRootHandler)
 				if err != nil {
-					log.Errorf("LetsEncrypt challenge server not started: %v", err)
+					return err
 				}
 			}
 
@@ -184,7 +184,7 @@ var (
 
 			SetupCloseHandler()
 
-			<-stopCh
+			stopCode := <-stopCh
 			if certListener != nil {
 				_ = certListener.Close()
 				log.Infof("stopped LetsEncrypt challenge server")
@@ -211,6 +211,9 @@ var (
 
 			log.Infof("stopped Signal Service")
 
+			if stopCode != 0 {
+				return errors.New("signal service stopped after a server failure")
+			}
 			return nil
 		},
 	}
@@ -315,7 +318,8 @@ func serveHTTP(httpListener net.Listener, handler http.Handler) {
 			Handler: h2c.NewHandler(handler, &http2.Server{}),
 		}
 		err := h1s.Serve(httpListener)
-		if err != nil {
+		// Closing the listener on shutdown is not a failure.
+		if err != nil && !errors.Is(err, net.ErrClosed) {
 			notifyStop(fmt.Sprintf("failed running HTTP server %v", err))
 		}
 	}()
@@ -328,7 +332,7 @@ func serveGRPC(grpcServer *grpc.Server, port int) (net.Listener, error) {
 	}
 	go func() {
 		err := grpcServer.Serve(listener)
-		if err != nil {
+		if err != nil && !errors.Is(err, net.ErrClosed) {
 			notifyStop(fmt.Sprintf("failed running gRPC server on port %d: %v", port, err))
 		}
 	}()

signal/cmd/run_test.go

@@ -1,6 +1,7 @@
 package cmd
 
 import (
+	"crypto/tls"
 	"net"
 	"net/http"
 	"testing"
@@ -20,7 +21,18 @@ func setLetsencryptListen(t *testing.T, port int, address string) {
 	})
 }
 
+func drainStopCh(t *testing.T) {
+	t.Helper()
+	t.Cleanup(func() {
+		select {
+		case <-stopCh:
+		default:
+		}
+	})
+}
+
 func TestStartServerWithCertManager_CustomAddress(t *testing.T) {
+	drainStopCh(t)
 	setLetsencryptListen(t, 10000, "127.0.0.1:0")
 
 	listener, err := startServerWithCertManager(&autocert.Manager{}, http.NotFoundHandler())
@@ -50,3 +62,31 @@ func TestCertManagerTLSConfigAnswersTLSALPN01(t *testing.T) {
 	cfg := (&autocert.Manager{}).TLSConfig()
 	require.Contains(t, cfg.NextProtos, acme.ALPNProto, "cert manager TLS config should offer the ACME TLS-ALPN protocol")
 }
+
+func TestServeHTTP_ClosedListenerIsNotAFailure(t *testing.T) {
+	drainStopCh(t)
+	listener, err := tls.Listen("tcp", "127.0.0.1:0", (&autocert.Manager{}).TLSConfig())
+	require.NoError(t, err)
+
+	serveHTTP(listener, http.NotFoundHandler())
+	require.NoError(t, listener.Close())
+
+	select {
+	case code := <-stopCh:
+		t.Fatalf("closing the listener reported a failure with code %d", code)
+	case <-time.After(200 * time.Millisecond):
+	}
+}
+
+func TestNotifyStop_KeepsEarlyFailure(t *testing.T) {
+	drainStopCh(t)
+
+	notifyStop("failed before the run loop waits")
+
+	select {
+	case code := <-stopCh:
+		require.Equal(t, 1, code, "an early failure should be reported with stop code 1")
+	default:
+		t.Fatal("a failure reported before the run loop waits was dropped")
+	}
+}