K8s: refactor build handler chain func to allow easier injection from enterprise (#100777)

pkg/api/common_test.go

@@ -189,7 +189,6 @@ func getContextHandler(t *testing.T, cfg *setting.Cfg) *contexthandler.ContextHa
 
 	return contexthandler.ProvideService(
 		cfg,
-		tracing.InitializeTracerForTest(),
 		&authntest.FakeService{ExpectedIdentity: &authn.Identity{ID: "0", Type: claims.TypeAnonymous, SessionToken: &usertoken.UserToken{}}},
 		featuremgmt.WithFeatures(),
 	)

pkg/middleware/auth_test.go

@@ -13,7 +13,6 @@ import (
 	authlib "github.com/grafana/authlib/types"
 	"github.com/grafana/grafana/pkg/infra/log"
 	"github.com/grafana/grafana/pkg/infra/log/logtest"
-	"github.com/grafana/grafana/pkg/infra/tracing"
 	"github.com/grafana/grafana/pkg/plugins"
 	"github.com/grafana/grafana/pkg/services/accesscontrol/actest"
 	"github.com/grafana/grafana/pkg/services/authn"
@@ -30,7 +29,7 @@ import (
 )
 
 func setupAuthMiddlewareTest(t *testing.T, identity *authn.Identity, authErr error) *contexthandler.ContextHandler {
-	return contexthandler.ProvideService(setting.NewCfg(), tracing.InitializeTracerForTest(), &authntest.FakeService{
+	return contexthandler.ProvideService(setting.NewCfg(), &authntest.FakeService{
 		ExpectedErr:      authErr,
 		ExpectedIdentity: identity,
 	}, featuremgmt.WithFeatures())

pkg/middleware/middleware_test.go

@@ -14,7 +14,6 @@ import (
 	"github.com/grafana/grafana/pkg/api/dtos"
 	"github.com/grafana/grafana/pkg/infra/fs"
 	"github.com/grafana/grafana/pkg/infra/log"
-	"github.com/grafana/grafana/pkg/infra/tracing"
 	"github.com/grafana/grafana/pkg/services/authn"
 	"github.com/grafana/grafana/pkg/services/authn/authntest"
 	"github.com/grafana/grafana/pkg/services/contexthandler"
@@ -290,6 +289,5 @@ func middlewareScenario(t *testing.T, desc string, fn scenarioFunc, cbs ...func(
 func getContextHandler(t *testing.T, cfg *setting.Cfg, authnService authn.Service) *contexthandler.ContextHandler {
 	t.Helper()
 
-	tracer := tracing.InitializeTracerForTest()
-	return contexthandler.ProvideService(cfg, tracer, authnService, featuremgmt.WithFeatures())
+	return contexthandler.ProvideService(cfg, authnService, featuremgmt.WithFeatures())
 }

pkg/server/wireexts_oss.go

@@ -7,9 +7,6 @@ package server
 import (
 	"github.com/google/wire"
 
-	"github.com/grafana/grafana/pkg/storage/unified"
-	search2 "github.com/grafana/grafana/pkg/storage/unified/search"
-
 	"github.com/grafana/grafana/pkg/infra/metrics"
 	"github.com/grafana/grafana/pkg/plugins"
 	"github.com/grafana/grafana/pkg/plugins/manager"
@@ -22,6 +19,7 @@ import (
 	"github.com/grafana/grafana/pkg/services/anonymous"
 	"github.com/grafana/grafana/pkg/services/anonymous/anonimpl"
 	"github.com/grafana/grafana/pkg/services/anonymous/validator"
+	builder "github.com/grafana/grafana/pkg/services/apiserver/builder"
 	"github.com/grafana/grafana/pkg/services/apiserver/standalone"
 	"github.com/grafana/grafana/pkg/services/auth"
 	"github.com/grafana/grafana/pkg/services/auth/authimpl"
@@ -53,6 +51,8 @@ import (
 	"github.com/grafana/grafana/pkg/services/user"
 	"github.com/grafana/grafana/pkg/services/validations"
 	"github.com/grafana/grafana/pkg/setting"
+	"github.com/grafana/grafana/pkg/storage/unified"
+	search2 "github.com/grafana/grafana/pkg/storage/unified/search"
 )
 
 var wireExtsBasicSet = wire.NewSet(
@@ -119,6 +119,7 @@ var wireExtsBasicSet = wire.NewSet(
 	wire.Bind(new(sandbox.Sandbox), new(*sandbox.Service)),
 	wire.Struct(new(unified.Options), "*"),
 	unified.ProvideUnifiedStorageClient,
+	builder.ProvideDefaultBuildHandlerChainFuncFromBuilders,
 )
 
 var wireExtsSet = wire.NewSet(

pkg/services/apiserver/aggregator/aggregator.go

@@ -78,7 +78,7 @@ func readCABundlePEM(path string, devMode bool) ([]byte, error) {
 	return io.ReadAll(f)
 }
 
-func readRemoteServices(path string) ([]RemoteService, error) {
+func ReadRemoteServices(path string) ([]RemoteService, error) {
 	// We can ignore the gosec G304 warning on this one because `path` comes
 	// from Grafana configuration (commandOptions.AggregatorOptions.RemoteServicesFile)
 	//nolint:gosec
@@ -127,8 +127,9 @@ func CreateAggregatorConfig(commandOptions *options.Options, sharedConfig generi
 			ClientConfig:          sharedConfig.LoopbackClientConfig,
 		},
 		ExtraConfig: aggregatorapiserver.ExtraConfig{
-			ProxyClientCertFile: commandOptions.KubeAggregatorOptions.ProxyClientCertFile,
-			ProxyClientKeyFile:  commandOptions.KubeAggregatorOptions.ProxyClientKeyFile,
+			DisableRemoteAvailableConditionController: true,
+			ProxyClientCertFile:                       commandOptions.KubeAggregatorOptions.ProxyClientCertFile,
+			ProxyClientKeyFile:                        commandOptions.KubeAggregatorOptions.ProxyClientKeyFile,
 			// NOTE: while ProxyTransport can be skipped in the configuration, it allows honoring
 			// DISABLE_HTTP2, HTTPS_PROXY and NO_PROXY env vars as needed
 			ProxyTransport:  createProxyTransport(),
@@ -155,7 +156,7 @@ func CreateAggregatorConfig(commandOptions *options.Options, sharedConfig generi
 	if err != nil {
 		return nil, err
 	}
-	remoteServices, err := readRemoteServices(commandOptions.KubeAggregatorOptions.RemoteServicesFile)
+	remoteServices, err := ReadRemoteServices(commandOptions.KubeAggregatorOptions.RemoteServicesFile)
 	if err != nil {
 		return nil, err
 	}

pkg/services/apiserver/builder/helper.go

@@ -36,8 +36,13 @@ import (
 	"github.com/grafana/grafana/pkg/storage/unified/apistore"
 )
 
+type BuildHandlerChainFuncFromBuilders = func([]APIGroupBuilder) BuildHandlerChainFunc
 type BuildHandlerChainFunc = func(delegateHandler http.Handler, c *genericapiserver.Config) http.Handler
 
+func ProvideDefaultBuildHandlerChainFuncFromBuilders() BuildHandlerChainFuncFromBuilders {
+	return GetDefaultBuildHandlerChainFunc
+}
+
 // PathRewriters is a temporary hack to make rest.Connecter work with resource level routes (TODO)
 var PathRewriters = []filters.PathRewriter{
 	{
@@ -60,7 +65,7 @@ var PathRewriters = []filters.PathRewriter{
 	},
 }
 
-func getDefaultBuildHandlerChainFunc(builders []APIGroupBuilder) BuildHandlerChainFunc {
+func GetDefaultBuildHandlerChainFunc(builders []APIGroupBuilder) BuildHandlerChainFunc {
 	return func(delegateHandler http.Handler, c *genericapiserver.Config) http.Handler {
 		requestHandler, err := GetCustomRoutesHandler(
 			delegateHandler,
@@ -100,7 +105,7 @@ func SetupConfig(
 	buildVersion string,
 	buildCommit string,
 	buildBranch string,
-	buildHandlerChainFunc func(delegateHandler http.Handler, c *genericapiserver.Config) http.Handler,
+	buildHandlerChainFuncFromBuilders BuildHandlerChainFuncFromBuilders,
 ) error {
 	serverConfig.AdmissionControl = NewAdmissionFromBuilders(builders)
 	defsGetter := GetOpenAPIDefinitions(builders)
@@ -220,11 +225,7 @@ func SetupConfig(
 	serverConfig.OpenAPIV3Config.Info.Version = buildVersion
 
 	serverConfig.SkipOpenAPIInstallation = false
-	serverConfig.BuildHandlerChainFunc = getDefaultBuildHandlerChainFunc(builders)
-
-	if buildHandlerChainFunc != nil {
-		serverConfig.BuildHandlerChainFunc = buildHandlerChainFunc
-	}
+	serverConfig.BuildHandlerChainFunc = buildHandlerChainFuncFromBuilders(builders)
 
 	v := utilversion.DefaultKubeEffectiveVersion()
 	patchver := 0 // required for semver

pkg/services/apiserver/options/kube-aggregator.go

@@ -36,6 +36,11 @@ func (o *KubeAggregatorOptions) AddFlags(fs *pflag.FlagSet) {
 		return
 	}
 
+	// the following two config variables are slated to be faded out in cloud deployments after which
+	// their scope is restricted to local development and non Grafana Cloud use-cases only
+	// leaving them unspecified leads to graceful behavior in grafana-aggregator
+	// and would work for configurations where the aggregated servers and aggregator are auth-less and trusting
+	// of each other
 	fs.StringVar(&o.ProxyClientCertFile, "proxy-client-cert-file", o.ProxyClientCertFile,
 		"path to proxy client cert file")
 
@@ -101,9 +106,8 @@ func (o *KubeAggregatorOptions) ApplyTo(aggregatorConfig *aggregatorapiserver.Co
 	genericConfig.PostStartHooks = map[string]genericapiserver.PostStartHookConfigEntry{}
 
 	// These hooks use v1 informers, which are not available in the grafana aggregator.
-	genericConfig.DisabledPostStartHooks = genericConfig.DisabledPostStartHooks.Insert("apiservice-status-local-available-controller")
-	genericConfig.DisabledPostStartHooks = genericConfig.DisabledPostStartHooks.Insert("apiservice-status-remote-available-controller")
 	genericConfig.DisabledPostStartHooks = genericConfig.DisabledPostStartHooks.Insert("start-kube-aggregator-informers")
+	genericConfig.DisabledPostStartHooks = genericConfig.DisabledPostStartHooks.Insert("apiservice-status-local-available-controller")
 
 	return nil
 }

pkg/services/apiserver/service.go

@@ -144,6 +144,8 @@ type service struct {
 	contextProvider datasource.PluginContextWrapper
 	pluginStore     pluginstore.Store
 	unified         resource.ResourceClient
+
+	buildHandlerChainFuncFromBuilders builder.BuildHandlerChainFuncFromBuilders
 }
 
 func ProvideService(
@@ -160,25 +162,27 @@ func ProvideService(
 	contextProvider datasource.PluginContextWrapper,
 	pluginStore pluginstore.Store,
 	unified resource.ResourceClient,
+	buildHandlerChainFuncFromBuilders builder.BuildHandlerChainFuncFromBuilders,
 ) (*service, error) {
 	s := &service{
-		log:               log.New(modules.GrafanaAPIServer),
-		cfg:               cfg,
-		features:          features,
-		rr:                rr,
-		stopCh:            make(chan struct{}),
-		builders:          []builder.APIGroupBuilder{},
-		authorizer:        authorizer.NewGrafanaAuthorizer(cfg, orgService),
-		tracing:           tracing,
-		db:                db, // For Unified storage
-		metrics:           metrics.ProvideRegisterer(),
-		kvStore:           kvStore,
-		pluginClient:      pluginClient,
-		datasources:       datasources,
-		contextProvider:   contextProvider,
-		pluginStore:       pluginStore,
-		serverLockService: serverLockService,
-		unified:           unified,
+		log:                               log.New(modules.GrafanaAPIServer),
+		cfg:                               cfg,
+		features:                          features,
+		rr:                                rr,
+		stopCh:                            make(chan struct{}),
+		builders:                          []builder.APIGroupBuilder{},
+		authorizer:                        authorizer.NewGrafanaAuthorizer(cfg, orgService),
+		tracing:                           tracing,
+		db:                                db, // For Unified storage
+		metrics:                           metrics.ProvideRegisterer(),
+		kvStore:                           kvStore,
+		pluginClient:                      pluginClient,
+		datasources:                       datasources,
+		contextProvider:                   contextProvider,
+		pluginStore:                       pluginStore,
+		serverLockService:                 serverLockService,
+		unified:                           unified,
+		buildHandlerChainFuncFromBuilders: buildHandlerChainFuncFromBuilders,
 	}
 	// This will be used when running as a dskit service
 	service := services.NewBasicService(s.start, s.running, nil).WithName(modules.GrafanaAPIServer)
@@ -349,7 +353,7 @@ func (s *service) start(ctx context.Context) error {
 		s.cfg.BuildVersion,
 		s.cfg.BuildCommit,
 		s.cfg.BuildBranch,
-		nil,
+		s.buildHandlerChainFuncFromBuilders,
 	)
 	if err != nil {
 		return err

pkg/services/contexthandler/contexthandler.go

@@ -26,11 +26,10 @@ import (
 	"github.com/grafana/grafana/pkg/web"
 )
 
-func ProvideService(cfg *setting.Cfg, tracer tracing.Tracer, authenticator authn.Authenticator, features featuremgmt.FeatureToggles,
+func ProvideService(cfg *setting.Cfg, authenticator authn.Authenticator, features featuremgmt.FeatureToggles,
 ) *ContextHandler {
 	return &ContextHandler{
 		Cfg:           cfg,
-		tracer:        tracer,
 		authenticator: authenticator,
 		features:      features,
 	}
@@ -39,7 +38,6 @@ func ProvideService(cfg *setting.Cfg, tracer tracing.Tracer, authenticator authn
 // ContextHandler is a middleware.
 type ContextHandler struct {
 	Cfg           *setting.Cfg
-	tracer        tracing.Tracer
 	authenticator authn.Authenticator
 	features      featuremgmt.FeatureToggles
 }
@@ -88,7 +86,8 @@ func CopyWithReqContext(ctx context.Context) context.Context {
 func (h *ContextHandler) Middleware(next http.Handler) http.Handler {
 	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		ctx := r.Context()
-		_, span := h.tracer.Start(ctx, "Auth - Middleware")
+		// Don't modify context so that the auth middleware span doesn't get propagated as a parent elsewhere
+		_, span := tracing.Start(ctx, "Auth - Middleware")
 
 		reqContext := &contextmodel.ReqContext{
 			Context: web.FromContext(ctx),

pkg/services/contexthandler/contexthandler_test.go

@@ -11,7 +11,6 @@ import (
 	claims "github.com/grafana/authlib/types"
 	"github.com/grafana/grafana/pkg/api/routing"
 	"github.com/grafana/grafana/pkg/apimachinery/identity"
-	"github.com/grafana/grafana/pkg/infra/tracing"
 	"github.com/grafana/grafana/pkg/services/authn"
 	"github.com/grafana/grafana/pkg/services/authn/authntest"
 	"github.com/grafana/grafana/pkg/services/contexthandler"
@@ -27,7 +26,6 @@ func TestContextHandler(t *testing.T) {
 	t.Run("should set auth error if authentication was unsuccessful", func(t *testing.T) {
 		handler := contexthandler.ProvideService(
 			setting.NewCfg(),
-			tracing.InitializeTracerForTest(),
 			&authntest.FakeService{ExpectedErr: errors.New("some error")},
 			featuremgmt.WithFeatures(),
 		)
@@ -49,7 +47,6 @@ func TestContextHandler(t *testing.T) {
 		id := &authn.Identity{ID: "1", Type: claims.TypeUser, OrgID: 1}
 		handler := contexthandler.ProvideService(
 			setting.NewCfg(),
-			tracing.InitializeTracerForTest(),
 			&authntest.FakeService{ExpectedIdentity: id},
 			featuremgmt.WithFeatures(),
 		)
@@ -75,7 +72,6 @@ func TestContextHandler(t *testing.T) {
 		identity := &authn.Identity{ID: "0", Type: claims.TypeAnonymous, OrgID: 1}
 		handler := contexthandler.ProvideService(
 			setting.NewCfg(),
-			tracing.InitializeTracerForTest(),
 			&authntest.FakeService{ExpectedIdentity: identity},
 			featuremgmt.WithFeatures(),
 		)
@@ -97,7 +93,6 @@ func TestContextHandler(t *testing.T) {
 		identity := &authn.Identity{OrgID: 1, AuthenticatedBy: login.RenderModule}
 		handler := contexthandler.ProvideService(
 			setting.NewCfg(),
-			tracing.InitializeTracerForTest(),
 			&authntest.FakeService{ExpectedIdentity: identity},
 			featuremgmt.WithFeatures(),
 		)
@@ -128,7 +123,6 @@ func TestContextHandler(t *testing.T) {
 
 		handler := contexthandler.ProvideService(
 			cfg,
-			tracing.InitializeTracerForTest(),
 			&authntest.FakeService{ExpectedIdentity: &authn.Identity{}},
 			featuremgmt.WithFeatures(),
 		)
@@ -157,7 +151,6 @@ func TestContextHandler(t *testing.T) {
 
 			handler := contexthandler.ProvideService(
 				cfg,
-				tracing.InitializeTracerForTest(),
 				&authntest.FakeService{ExpectedIdentity: &authn.Identity{ID: i, Type: typ}},
 				featuremgmt.WithFeatures(),
 			)