From ea788975e054b32de436085e5ae24122696323c7 Mon Sep 17 00:00:00 2001 From: Charandas <542168+charandas@users.noreply.github.com> Date: Fri, 14 Feb 2025 18:08:00 -0800 Subject: [PATCH] K8s: refactor build handler chain func to allow easier injection from enterprise (#100777) --- pkg/api/common_test.go | 1 - pkg/middleware/auth_test.go | 3 +- pkg/middleware/middleware_test.go | 4 +- pkg/server/wireexts_oss.go | 7 ++-- .../apiserver/aggregator/aggregator.go | 9 +++-- pkg/services/apiserver/builder/helper.go | 15 +++---- .../apiserver/options/kube-aggregator.go | 8 +++- pkg/services/apiserver/service.go | 40 ++++++++++--------- pkg/services/contexthandler/contexthandler.go | 7 ++-- .../contexthandler/contexthandler_test.go | 7 ---- 10 files changed, 50 insertions(+), 51 deletions(-) diff --git a/pkg/api/common_test.go b/pkg/api/common_test.go index 0072b068de8..d324831b075 100644 --- a/pkg/api/common_test.go +++ b/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(), ) diff --git a/pkg/middleware/auth_test.go b/pkg/middleware/auth_test.go index c7f2eb1d655..b9b83ae9bae 100644 --- a/pkg/middleware/auth_test.go +++ b/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()) diff --git a/pkg/middleware/middleware_test.go b/pkg/middleware/middleware_test.go index bd447ab5779..1cf18c20824 100644 --- a/pkg/middleware/middleware_test.go +++ b/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()) } diff --git a/pkg/server/wireexts_oss.go b/pkg/server/wireexts_oss.go index 038a628f323..96927f3ddba 100644 --- a/pkg/server/wireexts_oss.go +++ b/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( diff --git a/pkg/services/apiserver/aggregator/aggregator.go b/pkg/services/apiserver/aggregator/aggregator.go index 83080c780fc..67611fc3a71 100644 --- a/pkg/services/apiserver/aggregator/aggregator.go +++ b/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 } diff --git a/pkg/services/apiserver/builder/helper.go b/pkg/services/apiserver/builder/helper.go index 46b8e33aa8b..1d27da2c435 100644 --- a/pkg/services/apiserver/builder/helper.go +++ b/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 diff --git a/pkg/services/apiserver/options/kube-aggregator.go b/pkg/services/apiserver/options/kube-aggregator.go index 1186747f379..ced1599729c 100644 --- a/pkg/services/apiserver/options/kube-aggregator.go +++ b/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 } diff --git a/pkg/services/apiserver/service.go b/pkg/services/apiserver/service.go index 65490a742d4..98532a8ba24 100644 --- a/pkg/services/apiserver/service.go +++ b/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 diff --git a/pkg/services/contexthandler/contexthandler.go b/pkg/services/contexthandler/contexthandler.go index b230dcdb147..df738065715 100644 --- a/pkg/services/contexthandler/contexthandler.go +++ b/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), diff --git a/pkg/services/contexthandler/contexthandler_test.go b/pkg/services/contexthandler/contexthandler_test.go index 290a33bb895..9aace92ed7f 100644 --- a/pkg/services/contexthandler/contexthandler_test.go +++ b/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(), )