Merge pull request #8 from ethpandaops/feat/disable-metrics-default

feat(metrics): metrics srv disabled by default
ethpandaops · Jan 14, 2025 · 091d137 · 091d137
2 parents df6dee4 + b3c8dc1
commit 091d137
Show file tree

Hide file tree

Showing 4 changed files with 190 additions and 12 deletions.
diff --git a/cmd/sentry/main.go b/cmd/sentry/main.go
@@ -3,6 +3,7 @@ package main
 import (
 	"context"
 	"fmt"
+	"net"
 	"net/http"
 	_ "net/http/pprof" //nolint:gosec // pprof only enabled if pprofAddr config is set.
 	"os"
@@ -211,24 +212,34 @@ func (s *contributoor) stop(ctx context.Context) error {
 }
 
 func (s *contributoor) startMetricsServer() error {
+	metricsHost, metricsPort := s.config.GetMetricsHostPort()
+	if metricsHost == "" {
+		return nil
+	}
+
+	var addr = fmt.Sprintf(":%s", metricsPort)
+
+	// Start listening before creating the server to catch invalid addresses.
+	listener, err := net.Listen("tcp", addr)
+	if err != nil {
+		return fmt.Errorf("failed to start metrics server: %w", err)
+	}
+
 	sm := http.NewServeMux()
 	sm.Handle("/metrics", promhttp.Handler())
 
-	var (
-		_, metricsPort = s.config.GetMetricsHostPort()
-		addr           = fmt.Sprintf(":%s", metricsPort)
-	)
-
-	s.metricsServer = &http.Server{
+	server := &http.Server{
 		Addr:              addr,
 		ReadHeaderTimeout: 15 * time.Second,
 		Handler:           sm,
 	}
 
+	s.metricsServer = server
+
 	s.log.Infof("Serving metrics at %s", addr)
 
 	go func() {
-		if err := s.metricsServer.ListenAndServe(); err != nil && err != http.ErrServerClosed {
+		if err := server.Serve(listener); err != nil && err != http.ErrServerClosed {
 			s.log.Fatal(err)
 		}
 	}()
@@ -244,15 +255,23 @@ func (s *contributoor) startPProfServer() error {
 
 	var addr = fmt.Sprintf(":%s", pprofPort)
 
-	s.pprofServer = &http.Server{
+	// Start listening before creating the server to catch invalid addresses.
+	listener, err := net.Listen("tcp", addr)
+	if err != nil {
+		return fmt.Errorf("failed to start pprof server: %w", err)
+	}
+
+	server := &http.Server{
 		Addr:              addr,
 		ReadHeaderTimeout: 120 * time.Second,
 	}
 
+	s.pprofServer = server
+
 	s.log.Infof("Serving pprof at %s", addr)
 
 	go func() {
-		if err := s.pprofServer.ListenAndServe(); err != nil && err != http.ErrServerClosed {
+		if err := server.Serve(listener); err != nil && err != http.ErrServerClosed {
 			s.log.Fatal(err)
 		}
 	}()

diff --git a/cmd/sentry/main_test.go b/cmd/sentry/main_test.go
@@ -0,0 +1,155 @@
+package main
+
+import (
+	"fmt"
+	"net"
+	"net/http"
+	"testing"
+	"time"
+
+	"github.com/ethpandaops/contributoor/pkg/config/v1"
+	"github.com/sirupsen/logrus"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestStartMetricsServer(t *testing.T) {
+	tests := []struct {
+		name         string
+		metricsAddr  string
+		expectServer bool
+		expectError  bool
+	}{
+		{
+			name:         "empty address skips server",
+			metricsAddr:  "",
+			expectServer: false,
+		},
+		{
+			name:         "valid address starts server",
+			metricsAddr:  "localhost:9090",
+			expectServer: true,
+		},
+		{
+			name:         "invalid address errors",
+			metricsAddr:  "256.256.256.256:99999",
+			expectServer: false,
+			expectError:  true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			s := &contributoor{
+				log: logrus.New(),
+				config: &config.Config{
+					MetricsAddress: tt.metricsAddr,
+				},
+			}
+
+			err := s.startMetricsServer()
+			if tt.expectError {
+				assert.Error(t, err)
+			} else {
+				assert.NoError(t, err)
+			}
+
+			if tt.expectServer {
+				require.NotNil(t, s.metricsServer)
+
+				waitForServer(t, s.metricsServer.Addr)
+
+				client := http.Client{Timeout: time.Second}
+
+				resp, err := client.Get(fmt.Sprintf("http://%s/metrics", s.metricsServer.Addr))
+				require.NoError(t, err)
+				assert.Equal(t, http.StatusOK, resp.StatusCode)
+
+				resp.Body.Close()
+				_ = s.metricsServer.Close()
+			} else {
+				assert.Nil(t, s.metricsServer)
+			}
+		})
+	}
+}
+
+func TestStartPProfServer(t *testing.T) {
+	tests := []struct {
+		name         string
+		pprofAddr    string
+		expectServer bool
+		expectError  bool
+	}{
+		{
+			name:         "empty address skips server",
+			pprofAddr:    "",
+			expectServer: false,
+		},
+		{
+			name:         "valid address starts server",
+			pprofAddr:    "localhost:6060",
+			expectServer: true,
+		},
+		{
+			name:         "invalid address errors",
+			pprofAddr:    "256.256.256.256:99999",
+			expectServer: false,
+			expectError:  true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			s := &contributoor{
+				log: logrus.New(),
+				config: &config.Config{
+					PprofAddress: tt.pprofAddr,
+				},
+			}
+
+			err := s.startPProfServer()
+			if tt.expectError {
+				assert.Error(t, err)
+			} else {
+				assert.NoError(t, err)
+			}
+
+			if tt.expectServer {
+				require.NotNil(t, s.pprofServer)
+
+				waitForServer(t, s.pprofServer.Addr)
+
+				client := http.Client{Timeout: time.Second}
+
+				resp, err := client.Get(fmt.Sprintf("http://%s/debug/pprof/", s.pprofServer.Addr))
+				require.NoError(t, err)
+				assert.Equal(t, http.StatusOK, resp.StatusCode)
+
+				resp.Body.Close()
+				_ = s.pprofServer.Close()
+			} else {
+				assert.Nil(t, s.pprofServer)
+			}
+		})
+	}
+}
+
+func waitForServer(t *testing.T, addr string) {
+	t.Helper()
+
+	deadline := time.Now().Add(2 * time.Second)
+
+	for time.Now().Before(deadline) {
+		conn, err := net.DialTimeout("tcp", addr, 100*time.Millisecond)
+		if err == nil {
+			conn.Close()
+
+			return
+		}
+
+		time.Sleep(100 * time.Millisecond)
+	}
+
+	t.Fatalf("Server at %s did not start within deadline", addr)
+}
diff --git a/pkg/config/v1/config.go b/pkg/config/v1/config.go
@@ -137,6 +137,10 @@ func ParseAddress(address, defaultHost, defaultPort string) (host, port string)
 // GetMetricsHostPort returns the metrics host and port.
 // If MetricsAddress is not set, returns default values.
 func (c *Config) GetMetricsHostPort() (host, port string) {
+	if c.MetricsAddress == "" {
+		return "", ""
+	}
+
 	return ParseAddress(c.MetricsAddress, defaultMetricsHost, defaultMetricsPort)
 }
 

diff --git a/pkg/config/v1/config_test.go b/pkg/config/v1/config_test.go
@@ -169,10 +169,10 @@ func TestConfig_GetMetricsHostPort(t *testing.T) {
 		expectedPort string
 	}{
 		{
-			name:         "empty address returns defaults",
+			name:         "empty address returns empty strings",
 			config:       &Config{MetricsAddress: ""},
-			expectedHost: defaultMetricsHost,
-			expectedPort: defaultMetricsPort,
+			expectedHost: "",
+			expectedPort: "",
 		},
 		{
 			name:         "port only",