Move all query service HTTP handlers into one function (jaegertracing#6128)

## Which problem is this PR solving?
- Prequel for jaegertracing#6121

## Description of the changes
- Consolidate business logic handlers into a single function
`initRouter`
- Move tenancy handling to the same function, remove it from API and
APIv3 handlers

## How was this change tested?
- `go test ./cmd/query/app/...`

---------

Signed-off-by: Yuri Shkuro <[email protected]>

Author: yurishkuro

cmd/query/app/apiv3/gateway_test.go

@@ -22,7 +22,6 @@ import (
 	"github.com/jaegertracing/jaeger/cmd/query/app/internal/api_v3"
 	"github.com/jaegertracing/jaeger/model"
 	_ "github.com/jaegertracing/jaeger/pkg/gogocodec" // force gogo codec registration
-	"github.com/jaegertracing/jaeger/pkg/tenancy"
 	"github.com/jaegertracing/jaeger/storage/spanstore"
 	spanstoremocks "github.com/jaegertracing/jaeger/storage/spanstore/mocks"
 )
@@ -101,10 +100,9 @@ func makeTestTrace() (*model.Trace, model.TraceID) {
 func runGatewayTests(
 	t *testing.T,
 	basePath string,
-	tenancyOptions tenancy.Options,
 	setupRequest func(*http.Request),
 ) {
-	gw := setupHTTPGateway(t, basePath, tenancyOptions)
+	gw := setupHTTPGateway(t, basePath)
 	gw.setupRequest = setupRequest
 	t.Run("GetServices", gw.runGatewayGetServices)
 	t.Run("GetOperations", gw.runGatewayGetOperations)

cmd/query/app/apiv3/http_gateway.go

@@ -23,7 +23,6 @@ import (
 	"github.com/jaegertracing/jaeger/cmd/query/app/internal/api_v3"
 	"github.com/jaegertracing/jaeger/cmd/query/app/querysvc"
 	"github.com/jaegertracing/jaeger/model"
-	"github.com/jaegertracing/jaeger/pkg/tenancy"
 	"github.com/jaegertracing/jaeger/storage/spanstore"
 )
 
@@ -46,7 +45,6 @@ const (
 // HTTPGateway exposes APIv3 HTTP endpoints.
 type HTTPGateway struct {
 	QueryService *querysvc.QueryService
-	TenancyMgr   *tenancy.Manager
 	Logger       *zap.Logger
 	Tracer       trace.TracerProvider
 }
@@ -69,9 +67,6 @@ func (h *HTTPGateway) addRoute(
 	_ ...any, /* args */
 ) *mux.Route {
 	var handler http.Handler = http.HandlerFunc(f)
-	if h.TenancyMgr.Enabled {
-		handler = tenancy.ExtractTenantHTTPHandler(h.TenancyMgr, handler)
-	}
 	traceMiddleware := otelhttp.NewHandler(
 		otelhttp.WithRouteTag(route, handler),
 		route,

cmd/query/app/apiv3/http_gateway_test.go

@@ -6,7 +6,6 @@ package apiv3
 import (
 	"errors"
 	"fmt"
-	"io"
 	"net/http"
 	"net/http/httptest"
 	"net/url"
@@ -22,7 +21,6 @@ import (
 	"github.com/jaegertracing/jaeger/cmd/query/app/querysvc"
 	"github.com/jaegertracing/jaeger/model"
 	"github.com/jaegertracing/jaeger/pkg/jtracer"
-	"github.com/jaegertracing/jaeger/pkg/tenancy"
 	"github.com/jaegertracing/jaeger/pkg/testutils"
 	dependencyStoreMocks "github.com/jaegertracing/jaeger/storage/dependencystore/mocks"
 	"github.com/jaegertracing/jaeger/storage/spanstore"
@@ -32,7 +30,6 @@ import (
 func setupHTTPGatewayNoServer(
 	_ *testing.T,
 	basePath string,
-	tenancyOptions tenancy.Options,
 ) *testGateway {
 	gw := &testGateway{
 		reader: &spanstoremocks.Reader{},
@@ -45,7 +42,6 @@ func setupHTTPGatewayNoServer(
 
 	hgw := &HTTPGateway{
 		QueryService: q,
-		TenancyMgr:   tenancy.NewManager(&tenancyOptions),
 		Logger:       zap.NewNop(),
 		Tracer:       jtracer.NoOp().OTEL,
 	}
@@ -61,9 +57,8 @@ func setupHTTPGatewayNoServer(
 func setupHTTPGateway(
 	t *testing.T,
 	basePath string,
-	tenancyOptions tenancy.Options,
 ) *testGateway {
-	gw := setupHTTPGatewayNoServer(t, basePath, tenancyOptions)
+	gw := setupHTTPGatewayNoServer(t, basePath)
 
 	httpServer := httptest.NewServer(gw.router)
 	t.Cleanup(func() { httpServer.Close() })
@@ -76,22 +71,7 @@ func setupHTTPGateway(
 }
 
 func TestHTTPGateway(t *testing.T) {
-	for _, ten := range []bool{false, true} {
-		t.Run(fmt.Sprintf("tenancy=%v", ten), func(t *testing.T) {
-			tenancyOptions := tenancy.Options{
-				Enabled: ten,
-			}
-			tm := tenancy.NewManager(&tenancyOptions)
-			runGatewayTests(t, "/",
-				tenancyOptions,
-				func(req *http.Request) {
-					if ten {
-						// Add a tenancy header on outbound requests
-						req.Header.Add(tm.Header, "dummy")
-					}
-				})
-		})
-	}
+	runGatewayTests(t, "/", func(_ *http.Request) {})
 }
 
 func TestHTTPGatewayTryHandleError(t *testing.T) {
@@ -126,7 +106,7 @@ func TestHTTPGatewayOTLPError(t *testing.T) {
 }
 
 func TestHTTPGatewayGetTraceErrors(t *testing.T) {
-	gw := setupHTTPGatewayNoServer(t, "", tenancy.Options{})
+	gw := setupHTTPGatewayNoServer(t, "")
 
 	// malformed trace id
 	r, err := http.NewRequest(http.MethodGet, "/api/v3/traces/xyz", nil)
@@ -233,7 +213,7 @@ func TestHTTPGatewayFindTracesErrors(t *testing.T) {
 			require.NoError(t, err)
 			w := httptest.NewRecorder()
 
-			gw := setupHTTPGatewayNoServer(t, "", tenancy.Options{})
+			gw := setupHTTPGatewayNoServer(t, "")
 			gw.router.ServeHTTP(w, r)
 			assert.Contains(t, w.Body.String(), tc.expErr)
 		})
@@ -245,7 +225,7 @@ func TestHTTPGatewayFindTracesErrors(t *testing.T) {
 		require.NoError(t, err)
 		w := httptest.NewRecorder()
 
-		gw := setupHTTPGatewayNoServer(t, "", tenancy.Options{})
+		gw := setupHTTPGatewayNoServer(t, "")
 		gw.reader.
 			On("FindTraces", matchContext, qp).
 			Return(nil, errors.New(simErr)).Once()
@@ -256,7 +236,7 @@ func TestHTTPGatewayFindTracesErrors(t *testing.T) {
 }
 
 func TestHTTPGatewayGetServicesErrors(t *testing.T) {
-	gw := setupHTTPGatewayNoServer(t, "", tenancy.Options{})
+	gw := setupHTTPGatewayNoServer(t, "")
 
 	const simErr = "simulated error"
 	gw.reader.
@@ -271,7 +251,7 @@ func TestHTTPGatewayGetServicesErrors(t *testing.T) {
 }
 
 func TestHTTPGatewayGetOperationsErrors(t *testing.T) {
-	gw := setupHTTPGatewayNoServer(t, "", tenancy.Options{})
+	gw := setupHTTPGatewayNoServer(t, "")
 
 	qp := spanstore.OperationQueryParameters{ServiceName: "foo", SpanKind: "server"}
 	const simErr = "simulated error"
@@ -285,41 +265,3 @@ func TestHTTPGatewayGetOperationsErrors(t *testing.T) {
 	gw.router.ServeHTTP(w, r)
 	assert.Contains(t, w.Body.String(), simErr)
 }
-
-func TestHTTPGatewayTenancyRejection(t *testing.T) {
-	basePath := "/"
-	tenancyOptions := tenancy.Options{Enabled: true}
-	gw := setupHTTPGateway(t, basePath, tenancyOptions)
-
-	traceID := model.NewTraceID(150, 160)
-	gw.reader.On("GetTrace", matchContext, matchTraceID).Return(
-		&model.Trace{
-			Spans: []*model.Span{
-				{
-					TraceID:       traceID,
-					SpanID:        model.NewSpanID(180),
-					OperationName: "foobar",
-				},
-			},
-		}, nil).Once()
-
-	req, err := http.NewRequest(http.MethodGet, gw.url+"/api/v3/traces/123", nil)
-	require.NoError(t, err)
-	req.Header.Set("Content-Type", "application/json")
-	// We don't set tenant header
-	response, err := http.DefaultClient.Do(req)
-	require.NoError(t, err)
-	body, err := io.ReadAll(response.Body)
-	require.NoError(t, err)
-	require.NoError(t, response.Body.Close())
-	require.Equal(t, http.StatusUnauthorized, response.StatusCode, "response=%s", string(body))
-
-	// Try again with tenant header set
-	tm := tenancy.NewManager(&tenancyOptions)
-	req.Header.Set(tm.Header, "acme")
-	response, err = http.DefaultClient.Do(req)
-	require.NoError(t, err)
-	require.NoError(t, response.Body.Close())
-	require.Equal(t, http.StatusOK, response.StatusCode)
-	// Skip unmarshal of response; it is enough that it succeeded
-}

cmd/query/app/http_handler.go

@@ -27,7 +27,6 @@ import (
 	uiconv "github.com/jaegertracing/jaeger/model/converter/json"
 	ui "github.com/jaegertracing/jaeger/model/json"
 	"github.com/jaegertracing/jaeger/pkg/jtracer"
-	"github.com/jaegertracing/jaeger/pkg/tenancy"
 	"github.com/jaegertracing/jaeger/plugin/metrics/disabled"
 	"github.com/jaegertracing/jaeger/proto-gen/api_v2/metrics"
 	"github.com/jaegertracing/jaeger/storage/metricsstore"
@@ -76,22 +75,20 @@ type APIHandler struct {
 	queryService        *querysvc.QueryService
 	metricsQueryService querysvc.MetricsQueryService
 	queryParser         queryParser
-	tenancyMgr          *tenancy.Manager
 	basePath            string
 	apiPrefix           string
 	logger              *zap.Logger
 	tracer              trace.TracerProvider
 }
 
 // NewAPIHandler returns an APIHandler
-func NewAPIHandler(queryService *querysvc.QueryService, tm *tenancy.Manager, options ...HandlerOption) *APIHandler {
+func NewAPIHandler(queryService *querysvc.QueryService, options ...HandlerOption) *APIHandler {
 	aH := &APIHandler{
 		queryService: queryService,
 		queryParser: queryParser{
 			traceQueryLookbackDuration: defaultTraceQueryLookbackDuration,
 			timeNow:                    time.Now,
 		},
-		tenancyMgr: tm,
 	}
 
 	for _, option := range options {
@@ -135,9 +132,6 @@ func (aH *APIHandler) handleFunc(
 ) *mux.Route {
 	route := aH.formatRoute(routeFmt, args...)
 	var handler http.Handler = http.HandlerFunc(f)
-	if aH.tenancyMgr.Enabled {
-		handler = tenancy.ExtractTenantHTTPHandler(aH.tenancyMgr, handler)
-	}
 	traceMiddleware := otelhttp.NewHandler(
 		otelhttp.WithRouteTag(route, traceResponseHandler(handler)),
 		route,

cmd/query/app/http_handler_test.go

@@ -121,13 +121,13 @@ func initializeTestServerWithOptions(
 	dependencyStorage := &depsmocks.Reader{}
 	qs := querysvc.NewQueryService(readStorage, dependencyStorage, queryOptions)
 	r := NewRouter()
-	handler := NewAPIHandler(qs, tenancyMgr, options...)
-	handler.RegisterRoutes(r)
+	apiHandler := NewAPIHandler(qs, options...)
+	apiHandler.RegisterRoutes(r)
 	ts := &testServer{
 		server:           httptest.NewServer(tenancy.ExtractTenantHTTPHandler(tenancyMgr, r)),
 		spanReader:       readStorage,
 		dependencyReader: dependencyStorage,
-		handler:          handler,
+		handler:          apiHandler,
 	}
 	t.Cleanup(func() {
 		ts.server.Close()
@@ -168,7 +168,7 @@ func TestLogOnServerError(t *testing.T) {
 	readStorage := &spanstoremocks.Reader{}
 	dependencyStorage := &depsmocks.Reader{}
 	qs := querysvc.NewQueryService(readStorage, dependencyStorage, querysvc.QueryServiceOptions{})
-	h := NewAPIHandler(qs, &tenancy.Manager{}, HandlerOptions.Logger(logger))
+	h := NewAPIHandler(qs, HandlerOptions.Logger(logger))
 	e := errors.New("test error")
 	h.handleError(&httptest.ResponseRecorder{}, e, http.StatusInternalServerError)
 	require.Len(t, logs.All(), 1)

cmd/query/app/server.go

@@ -203,14 +203,13 @@ type httpServer struct {
 
 var _ io.Closer = (*httpServer)(nil)
 
-func createHTTPServer(
-	ctx context.Context,
+func initRouter(
 	querySvc *querysvc.QueryService,
 	metricsQuerySvc querysvc.MetricsQueryService,
 	queryOpts *QueryOptions,
-	tm *tenancy.Manager,
+	tenancyMgr *tenancy.Manager,
 	telset telemetery.Setting,
-) (*httpServer, error) {
+) (http.Handler, io.Closer) {
 	apiHandlerOptions := []HandlerOption{
 		HandlerOptions.Logger(telset.Logger),
 		HandlerOptions.Tracer(telset.TracerProvider),
@@ -219,7 +218,6 @@ func createHTTPServer(
 
 	apiHandler := NewAPIHandler(
 		querySvc,
-		tm,
 		apiHandlerOptions...)
 	r := NewRouter()
 	if queryOpts.BasePath != "/" {
@@ -228,17 +226,33 @@ func createHTTPServer(
 
 	(&apiv3.HTTPGateway{
 		QueryService: querySvc,
-		TenancyMgr:   tm,
 		Logger:       telset.Logger,
 		Tracer:       telset.TracerProvider,
 	}).RegisterRoutes(r)
 
 	apiHandler.RegisterRoutes(r)
+	staticHandlerCloser := RegisterStaticHandler(r, telset.Logger, queryOpts, querySvc.GetCapabilities())
+
 	var handler http.Handler = r
-	handler = responseHeadersHandler(handler, queryOpts.HTTP.ResponseHeaders)
 	if queryOpts.BearerTokenPropagation {
 		handler = bearertoken.PropagationHandler(telset.Logger, handler)
 	}
+	if tenancyMgr.Enabled {
+		handler = tenancy.ExtractTenantHTTPHandler(tenancyMgr, handler)
+	}
+	return handler, staticHandlerCloser
+}
+
+func createHTTPServer(
+	ctx context.Context,
+	querySvc *querysvc.QueryService,
+	metricsQuerySvc querysvc.MetricsQueryService,
+	queryOpts *QueryOptions,
+	tm *tenancy.Manager,
+	telset telemetery.Setting,
+) (*httpServer, error) {
+	handler, staticHandlerCloser := initRouter(querySvc, metricsQuerySvc, queryOpts, tm, telset)
+	handler = responseHeadersHandler(handler, queryOpts.HTTP.ResponseHeaders)
 	handler = handlers.CompressHandler(handler)
 	recoveryHandler := recoveryhandler.NewRecoveryHandler(telset.Logger, true)
 
@@ -249,18 +263,17 @@ func createHTTPServer(
 			ErrorLog:          errorLog,
 			ReadHeaderTimeout: 2 * time.Second,
 		},
+		staticHandlerCloser: staticHandlerCloser,
 	}
 
 	if queryOpts.HTTP.TLSSetting != nil {
 		tlsCfg, err := queryOpts.HTTP.TLSSetting.LoadTLSConfig(ctx) // This checks if the certificates are correctly provided
 		if err != nil {
-			return nil, err
+			return nil, errors.Join(err, staticHandlerCloser.Close())
 		}
 		server.TLSConfig = tlsCfg
 	}
 
-	server.staticHandlerCloser = RegisterStaticHandler(r, telset.Logger, queryOpts, querySvc.GetCapabilities())
-
 	return server, nil
 }