httptransport: rework constructor

This makes the `New` function return a `http.Handler` instead of a
custom Server type.

Signed-off-by: Hank Donnay <[email protected]>

Author: hdonnay

cmd/clair/main.go

@@ -126,7 +126,10 @@ func main() {
 		if err != nil {
 			return fmt.Errorf("service initialization failed: %w", err)
 		}
-		h, err := httptransport.New(srvctx, conf, srvs.Indexer, srvs.Matcher, srvs.Notifier)
+		srv := http.Server{
+			BaseContext: func(_ net.Listener) context.Context { return srvctx },
+		}
+		srv.Handler, err = httptransport.New(srvctx, &conf, srvs.Indexer, srvs.Matcher, srvs.Notifier)
 		if err != nil {
 			return fmt.Errorf("http transport configuration failed: %w", err)
 		}
@@ -140,11 +143,12 @@ func main() {
 				return fmt.Errorf("tls configuration failed: %w", err)
 			}
 			cfg.NextProtos = []string{"h2"}
+			srv.TLSConfig = cfg
 			l = tls.NewListener(l, cfg)
 		}
-		down.Add(h.Server)
+		down.Add(&srv)
 		health.Ready()
-		if err := h.Serve(l); err != http.ErrServerClosed {
+		if err := srv.Serve(l); err != http.ErrServerClosed {
 			return fmt.Errorf("http transport failed to launch: %w", err)
 		}
 		return nil

httptransport/server.go

@@ -3,17 +3,15 @@ package httptransport
 import (
 	"context"
 	"fmt"
-	"net"
 	"net/http"
 	"time"
 
 	"github.com/quay/clair/config"
 	"github.com/quay/zlog"
-	othttp "go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp"
+	"go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp"
 	"go.opentelemetry.io/otel"
 	"golang.org/x/sync/semaphore"
 
-	clairerror "github.com/quay/clair/v4/clair-error"
 	"github.com/quay/clair/v4/indexer"
 	"github.com/quay/clair/v4/matcher"
 	"github.com/quay/clair/v4/notifier"
@@ -39,193 +37,85 @@ const (
 	OpenAPIV1Path                = "/openapi/v1"
 )
 
-// Server is the primary http server Clair exposes its functionality on.
-type Server struct {
-	// Server embeds a http.Server and http.ServeMux.
-	// The http.Server will be configured with the ServeMux on successful
-	// initialization.
-	conf config.Config
-	*http.Server
-	*http.ServeMux
-	indexer  indexer.Service
-	matcher  matcher.Service
-	notifier notifier.Service
-	traceOpt othttp.Option
-}
-
-func New(ctx context.Context, conf config.Config, indexer indexer.Service, matcher matcher.Service, notifier notifier.Service) (*Server, error) {
-	serv := &http.Server{
-		Addr: conf.HTTPListenAddr,
-		// use the passed in global context as the base context for all http
-		// requests handled by this server
-		BaseContext: func(net.Listener) context.Context { return ctx },
-	}
+func New(ctx context.Context, conf *config.Config, indexer indexer.Service, matcher matcher.Service, notifier notifier.Service) (http.Handler, error) {
 	mux := http.NewServeMux()
-	t := &Server{
-		conf:     conf,
-		Server:   serv,
-		ServeMux: mux,
-		indexer:  indexer,
-		matcher:  matcher,
-		notifier: notifier,
-		traceOpt: othttp.WithTracerProvider(otel.GetTracerProvider()),
-	}
+	traceOpt := otelhttp.WithTracerProvider(otel.GetTracerProvider())
 	ctx = zlog.ContextWithValues(ctx, "component", "httptransport/New")
 
-	mux.Handle(OpenAPIV1Path, DiscoveryHandler(ctx, OpenAPIV1Path, t.traceOpt))
+	mux.Handle(OpenAPIV1Path, DiscoveryHandler(ctx, OpenAPIV1Path, traceOpt))
 	zlog.Info(ctx).Str("path", OpenAPIV1Path).Msg("openapi discovery configured")
 
-	var e error
-	switch conf.Mode {
-	case config.ComboMode:
-		e = t.configureComboMode(ctx)
-		if e != nil {
-			return nil, e
+	// NOTE(hank) My brain always wants to rewrite constructions like the
+	// following as a switch, but this is actually cleaner as an "if" sequence.
+
+	if conf.Mode == config.IndexerMode || conf.Mode == config.ComboMode {
+		if indexer == nil {
+			return nil, fmt.Errorf("mode %q requires an indexer service", conf.Mode)
+		}
+		prefix := indexerRoot + apiRoot
+
+		v1, err := NewIndexerV1(ctx, prefix, indexer, traceOpt)
+		if err != nil {
+			return nil, fmt.Errorf("indexer configuration: %w", err)
 		}
-	case config.IndexerMode:
-		e = t.configureIndexerMode(ctx)
-		if e != nil {
-			return nil, e
+		var sem *semaphore.Weighted
+		if ct := conf.Indexer.IndexReportRequestConcurrency; ct > 0 {
+			sem = semaphore.NewWeighted(int64(ct))
 		}
-	case config.MatcherMode:
-		e = t.configureMatcherMode(ctx)
-		if e != nil {
-			return nil, e
+		rl := &limitHandler{
+			Check: func(r *http.Request) (*semaphore.Weighted, string) {
+				if r.Method != http.MethodPost && r.URL.Path != IndexAPIPath {
+					return nil, ""
+				}
+				// Nil if the relevant config option isn't set.
+				return sem, IndexAPIPath
+			},
+			Next: v1,
 		}
-	case config.NotifierMode:
-		e = t.configureNotifierMode(ctx)
-		if e != nil {
-			return nil, e
+		mux.Handle(prefix, rl)
+	}
+	if conf.Mode == config.MatcherMode || conf.Mode == config.ComboMode {
+		if indexer == nil || matcher == nil {
+			return nil, fmt.Errorf("mode %q requires both an indexer service and a matcher service", conf.Mode)
 		}
+		prefix := matcherRoot + apiRoot
+		v1 := NewMatcherV1(ctx, prefix, matcher, indexer, time.Duration(conf.Matcher.CacheAge), traceOpt)
+		mux.Handle(prefix, v1)
 	}
-
-	// attach HttpTransport to server, this works because we embed http.ServeMux
-	t.Server.Handler = t
-
+	if conf.Mode == config.NotifierMode || (conf.Mode == config.ComboMode && notifier != nil) {
+		if notifier == nil {
+			return nil, fmt.Errorf("mode %q requires a notifier service", conf.Mode)
+		}
+		prefix := notifierRoot + apiRoot
+		v1, err := NewNotificationV1(ctx, prefix, notifier, traceOpt)
+		if err != nil {
+			return nil, fmt.Errorf("notifier configuration: %w", err)
+		}
+		mux.Handle(prefix, v1)
+	}
+	if conf.Mode == config.ComboMode && notifier == nil {
+		zlog.Debug(ctx).Msg("skipping unconfigured notifier")
+	}
+	var ret http.Handler = mux
 	// Add endpoint authentication if configured to add auth. Must happen after
 	// mux was configured for given mode.
 	if conf.Auth.Any() {
-		err := t.configureWithAuth(ctx)
+		h, err := authHandler(conf, mux)
 		if err != nil {
 			zlog.Warn(ctx).
 				Err(err).
 				Msg("received error configuring auth middleware")
+			return nil, err
 		}
+		ret = h
 	}
-
-	return t, nil
-}
-
-// ConfigureComboMode configures the HttpTransport for ComboMode.
-//
-// This mode runs both Indexer and Matcher in a single process.
-func (t *Server) configureComboMode(ctx context.Context) error {
-	// requires both indexer and matcher services
-	if t.indexer == nil || t.matcher == nil {
-		return clairerror.ErrNotInitialized{Msg: "Combo mode requires both indexer and macher services"}
-	}
-
-	err := t.configureIndexerMode(ctx)
-	if err != nil {
-		return clairerror.ErrNotInitialized{Msg: "could not configure indexer: " + err.Error()}
-	}
-
-	err = t.configureMatcherMode(ctx)
-	if err != nil {
-		return clairerror.ErrNotInitialized{Msg: "could not configure matcher: " + err.Error()}
-	}
-
-	if t.notifier != nil {
-		if err := t.configureNotifierMode(ctx); err != nil {
-			return clairerror.ErrNotInitialized{Msg: "could not configure notifier: " + err.Error()}
-		}
-	}
-
-	return nil
-}
-
-// ConfigureIndexerMode configures the HttpTransport for IndexerMode.
-//
-// This mode runs only an Indexer in a single process.
-func (t *Server) configureIndexerMode(ctx context.Context) error {
-	// requires only indexer service
-	if t.indexer == nil {
-		return clairerror.ErrNotInitialized{Msg: "IndexerMode requires an indexer service"}
-	}
-	prefix := indexerRoot + apiRoot
-
-	v1, err := NewIndexerV1(ctx, prefix, t.indexer, t.traceOpt)
-	if err != nil {
-		return fmt.Errorf("indexer configuration: %w", err)
-	}
-	var sem *semaphore.Weighted
-	if ct := t.conf.Indexer.IndexReportRequestConcurrency; ct > 0 {
-		sem = semaphore.NewWeighted(int64(ct))
-	}
-	rl := &limitHandler{
-		Check: func(r *http.Request) (*semaphore.Weighted, string) {
-			if r.Method != http.MethodPost && r.URL.Path != IndexAPIPath {
-				return nil, ""
-			}
-			// Nil if the relevant config option isn't set.
-			return sem, IndexAPIPath
-		},
-		Next: v1,
-	}
-	t.Handle(prefix, rl)
-	return nil
-}
-
-// ConfigureMatcherMode configures HttpTransport for MatcherMode.
-//
-// This mode runs only a Matcher in a single process.
-func (t *Server) configureMatcherMode(ctx context.Context) error {
-	// requires both an indexer and matcher service. indexer service
-	// is assumed to be a remote call over the network
-	if t.indexer == nil || t.matcher == nil {
-		return clairerror.ErrNotInitialized{Msg: "MatcherMode requires both indexer and matcher services"}
-	}
-	prefix := matcherRoot + apiRoot
-	v1 := NewMatcherV1(ctx, prefix, t.matcher, t.indexer, time.Duration(t.conf.Matcher.CacheAge), t.traceOpt)
-
-	t.Handle(prefix, v1)
-	return nil
-}
-
-// ConfigureNotifierMode configures HttpTransport for NotifierMode.
-//
-// This mode runs only a Notifier in a single process.
-func (t *Server) configureNotifierMode(ctx context.Context) error {
-	if t.notifier == nil {
-		return clairerror.ErrNotInitialized{Msg: "NotifierMode requires a notifier service"}
-	}
-	prefix := notifierRoot + apiRoot
-	v1, err := NewNotificationV1(ctx, prefix, t.notifier, t.traceOpt)
-	if err != nil {
-		return fmt.Errorf("notifier configuration: %w", err)
-	}
-
-	t.Handle(prefix, v1)
-	return nil
+	return ret, nil
 }
 
 // IntraserviceIssuer is the issuer that will be used if Clair is configured to
 // mint its own JWTs.
 const IntraserviceIssuer = `clair-intraservice`
 
-// ConfigureWithAuth will take the current serve mux and wrap it in an Auth
-// middleware handler.
-//
-// Must be ran after the config*Mode method of choice.
-func (t *Server) configureWithAuth(_ context.Context) error {
-	h, err := authHandler(&t.conf, t.Server.Handler)
-	if err != nil {
-		return err
-	}
-	t.Server.Handler = h
-	return nil
-}
-
 // Unmodified determines whether to return a conditional response.
 func unmodified(r *http.Request, v string) bool {
 	if vs, ok := r.Header["If-None-Match"]; ok {

httptransport/server_test.go

@@ -10,11 +10,10 @@ import (
 	"testing"
 
 	"github.com/google/uuid"
+	"github.com/quay/clair/config"
 	"github.com/quay/claircore"
 	"github.com/quay/claircore/libvuln/driver"
 	"github.com/quay/zlog"
-	othttp "go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp"
-	"go.opentelemetry.io/otel"
 
 	"github.com/quay/clair/v4/indexer"
 	"github.com/quay/clair/v4/matcher"
@@ -45,17 +44,12 @@ func TestUpdateEndpoints(t *testing.T) {
 			return nil, nil
 		},
 	}
-	s := &Server{
-		matcher:  m,
-		indexer:  i,
-		ServeMux: http.NewServeMux(),
-		traceOpt: othttp.WithTracerProvider(otel.GetTracerProvider()),
-	}
 	ctx := zlog.Test(context.Background(), t)
-	if err := s.configureMatcherMode(ctx); err != nil {
+	h, err := New(ctx, &config.Config{Mode: config.MatcherMode}, i, m, nil)
+	if err != nil {
 		t.Error(err)
 	}
-	srv := httptest.NewUnstartedServer(s)
+	srv := httptest.NewUnstartedServer(h)
 	srv.Config.BaseContext = func(_ net.Listener) context.Context {
 		return ctx
 	}