[admin] Use OTEL helper instead of tlscfg (#6319)

## Which problem is this PR solving? - Part of #4316 ## Description of the changes - ## How was this change tested? - ## Checklist - [ ] I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md - [ ] I have signed all commits - [ ] I have added unit tests for the new functionality - [ ] I have run lint and test steps successfully - for `jaeger`: `make lint test` - for `jaeger-ui`: `yarn lint` and `yarn test` --------- Signed-off-by: chahatsagarmain <[email protected]> Signed-off-by: Yuri Shkuro <[email protected]> Signed-off-by: Yuri Shkuro <[email protected]> Co-authored-by: Yuri Shkuro <[email protected]> Co-authored-by: Yuri Shkuro <[email protected]>
jaegertracing · Dec 12, 2024 · 8696541 · 8696541
1 parent 4162f57
commit 8696541
Show file tree

Hide file tree

Showing 3 changed files with 53 additions and 58 deletions.
diff --git a/cmd/internal/flags/admin.go b/cmd/internal/flags/admin.go
@@ -5,23 +5,23 @@ package flags
 
 import (
 	"context"
-	"crypto/tls"
 	"errors"
 	"flag"
 	"fmt"
-	"io"
 	"net"
 	"net/http"
 	"net/http/pprof"
-	"time"
+	"sync"
 
 	"github.com/spf13/viper"
+	"go.opentelemetry.io/collector/config/confighttp"
 	"go.uber.org/zap"
 	"go.uber.org/zap/zapcore"
 
 	"github.com/jaegertracing/jaeger/pkg/config/tlscfg"
 	"github.com/jaegertracing/jaeger/pkg/healthcheck"
 	"github.com/jaegertracing/jaeger/pkg/recoveryhandler"
+	"github.com/jaegertracing/jaeger/pkg/telemetry"
 	"github.com/jaegertracing/jaeger/pkg/version"
 )
 
@@ -35,22 +35,23 @@ var tlsAdminHTTPFlagsConfig = tlscfg.ServerFlagsConfig{
 
 // AdminServer runs an HTTP server with admin endpoints, such as healthcheck at /, /metrics, etc.
 type AdminServer struct {
-	logger               *zap.Logger
-	adminHostPort        string
-	hc                   *healthcheck.HealthCheck
-	mux                  *http.ServeMux
-	server               *http.Server
-	tlsCfg               *tls.Config
-	tlsCertWatcherCloser io.Closer
+	logger    *zap.Logger
+	hc        *healthcheck.HealthCheck
+	mux       *http.ServeMux
+	server    *http.Server
+	serverCfg confighttp.ServerConfig
+	stopped   sync.WaitGroup
 }
 
 // NewAdminServer creates a new admin server.
 func NewAdminServer(hostPort string) *AdminServer {
 	return &AdminServer{
-		adminHostPort: hostPort,
-		logger:        zap.NewNop(),
-		hc:            healthcheck.New(),
-		mux:           http.NewServeMux(),
+		logger: zap.NewNop(),
+		hc:     healthcheck.New(),
+		mux:    http.NewServeMux(),
+		serverCfg: confighttp.ServerConfig{
+			Endpoint: hostPort,
+		},
 	}
 }
 
@@ -67,30 +68,21 @@ func (s *AdminServer) setLogger(logger *zap.Logger) {
 
 // AddFlags registers CLI flags.
 func (s *AdminServer) AddFlags(flagSet *flag.FlagSet) {
-	flagSet.String(adminHTTPHostPort, s.adminHostPort, fmt.Sprintf("The host:port (e.g. 127.0.0.1%s or %s) for the admin server, including health check, /metrics, etc.", s.adminHostPort, s.adminHostPort))
+	flagSet.String(adminHTTPHostPort, s.serverCfg.Endpoint, fmt.Sprintf("The host:port (e.g. 127.0.0.1%s or %s) for the admin server, including health check, /metrics, etc.", s.serverCfg.Endpoint, s.serverCfg.Endpoint))
 	tlsAdminHTTPFlagsConfig.AddFlags(flagSet)
 }
 
 // InitFromViper initializes the server with properties retrieved from Viper.
 func (s *AdminServer) initFromViper(v *viper.Viper, logger *zap.Logger) error {
 	s.setLogger(logger)
 
-	s.adminHostPort = v.GetString(adminHTTPHostPort)
-	var tlsAdminHTTP tlscfg.Options
 	tlsAdminHTTP, err := tlsAdminHTTPFlagsConfig.InitFromViper(v)
 	if err != nil {
 		return fmt.Errorf("failed to parse admin server TLS options: %w", err)
 	}
-	if tlsAdminHTTP.Enabled {
-		tlsCfg, err := tlsAdminHTTP.Config(s.logger) // This checks if the certificates are correctly provided
-		if err != nil {
-			return err
-		}
-		s.tlsCfg = tlsCfg
-		s.tlsCertWatcherCloser = &tlsAdminHTTP
-	} else {
-		s.tlsCertWatcherCloser = io.NopCloser(nil)
-	}
+
+	s.serverCfg.Endpoint = v.GetString(adminHTTPHostPort)
+	s.serverCfg.TLSSetting = tlsAdminHTTP.ToOtelServerConfig()
 	return nil
 }
 
@@ -101,48 +93,52 @@ func (s *AdminServer) Handle(path string, handler http.Handler) {
 
 // Serve starts HTTP server.
 func (s *AdminServer) Serve() error {
-	l, err := net.Listen("tcp", s.adminHostPort)
+	l, err := s.serverCfg.ToListener(context.Background())
 	if err != nil {
 		s.logger.Error("Admin server failed to listen", zap.Error(err))
 		return err
 	}
-	s.serveWithListener(l)
 
-	s.logger.Info(
-		"Admin server started",
-		zap.String("http.host-port", l.Addr().String()),
-		zap.Stringer("health-status", s.hc.Get()))
-	return nil
+	return s.serveWithListener(l)
 }
 
-func (s *AdminServer) serveWithListener(l net.Listener) {
+func (s *AdminServer) serveWithListener(l net.Listener) (err error) {
 	s.logger.Info("Mounting health check on admin server", zap.String("route", "/"))
 	s.mux.Handle("/", s.hc.Handler())
 	version.RegisterHandler(s.mux, s.logger)
 	s.registerPprofHandlers()
 	recoveryHandler := recoveryhandler.NewRecoveryHandler(s.logger, true)
-	errorLog, _ := zap.NewStdLogAt(s.logger, zapcore.ErrorLevel)
-	s.server = &http.Server{
-		Handler:           recoveryHandler(s.mux),
-		ErrorLog:          errorLog,
-		ReadHeaderTimeout: 2 * time.Second,
-	}
-	if s.tlsCfg != nil {
-		s.server.TLSConfig = s.tlsCfg
+	s.server, err = s.serverCfg.ToServer(
+		context.Background(),
+		nil, // host
+		telemetry.NoopSettings().ToOtelComponent(),
+		recoveryHandler(s.mux),
+	)
+	if err != nil {
+		return fmt.Errorf("failed to create admin server: %w", err)
 	}
-	s.logger.Info("Starting admin HTTP server", zap.String("http-addr", s.adminHostPort))
+	errorLog, _ := zap.NewStdLogAt(s.logger, zapcore.ErrorLevel)
+	s.server.ErrorLog = errorLog
+
+	s.logger.Info("Starting admin HTTP server")
+	var wg sync.WaitGroup
+	wg.Add(1)
+	s.stopped.Add(1)
 	go func() {
-		var err error
-		if s.tlsCfg != nil {
-			err = s.server.ServeTLS(l, "", "")
-		} else {
-			err = s.server.Serve(l)
-		}
+		wg.Done()
+		defer s.stopped.Done()
+		err := s.server.Serve(l)
 		if err != nil && !errors.Is(err, http.ErrServerClosed) {
 			s.logger.Error("failed to serve", zap.Error(err))
 			s.hc.Set(healthcheck.Broken)
 		}
 	}()
+	wg.Wait() // wait for the server to start listening
+	s.logger.Info(
+		"Admin server started",
+		zap.String("http.host-port", l.Addr().String()),
+		zap.Stringer("health-status", s.hc.Get()))
+	return nil
 }
 
 func (s *AdminServer) registerPprofHandlers() {
@@ -159,8 +155,7 @@ func (s *AdminServer) registerPprofHandlers() {
 
 // Close stops the HTTP server
 func (s *AdminServer) Close() error {
-	return errors.Join(
-		s.tlsCertWatcherCloser.Close(),
-		s.server.Shutdown(context.Background()),
-	)
+	err := s.server.Shutdown(context.Background())
+	s.stopped.Wait()
+	return err
 }
diff --git a/cmd/internal/flags/admin_test.go b/cmd/internal/flags/admin_test.go
@@ -4,6 +4,7 @@
 package flags
 
 import (
+	"context"
 	"crypto/tls"
 	"fmt"
 	"net"
@@ -69,7 +70,7 @@ func TestAdminFailToServe(t *testing.T) {
 	require.NoError(t, adminServer.initFromViper(v, logger))
 
 	adminServer.serveWithListener(l)
-	defer adminServer.Close()
+	t.Cleanup(func() { assert.NoError(t, adminServer.Close()) })
 
 	waitForEqual(t, healthcheck.Broken, func() any { return adminServer.HC().Get() })
 
@@ -133,9 +134,8 @@ func TestAdminServerTLS(t *testing.T) {
 			adminServer.Serve()
 			defer adminServer.Close()
 
-			clientTLSCfg, err0 := test.clientTLS.Config(zap.NewNop())
+			clientTLSCfg, err0 := test.clientTLS.ToOtelClientConfig().LoadTLSConfig(context.Background())
 			require.NoError(t, err0)
-			defer test.clientTLS.Close()
 			dialer := &net.Dialer{Timeout: 2 * time.Second}
 			conn, clientError := tls.DialWithDialer(dialer, "tcp", fmt.Sprintf("localhost:%d", ports.CollectorAdminHTTP), clientTLSCfg)
 			require.NoError(t, clientError)

diff --git a/cmd/internal/flags/service_test.go b/cmd/internal/flags/service_test.go
@@ -50,7 +50,7 @@ func TestStartErrors(t *testing.T) {
 		{
 			name:   "bad admin TLS",
 			flags:  []string{"--admin.http.tls.enabled=true", "--admin.http.tls.cert=invalid-cert"},
-			expErr: "cannot initialize admin server",
+			expErr: "cannot start the admin server: failed to load TLS config",
 		},
 		{
 			name:   "bad host:port",