From fc8376bff9d279737fa14410862bdc7a03a2c8d0 Mon Sep 17 00:00:00 2001 From: Georges Chaudy Date: Tue, 6 Jan 2026 17:27:28 +0100 Subject: [PATCH] Refactor: Remove fallback logic from Check and Compile methods in authzLimitedClient --- pkg/storage/unified/resource/access.go | 41 +----------- pkg/storage/unified/resource/access_test.go | 64 ------------------- pkg/storage/unified/sql/service.go | 70 +-------------------- 3 files changed, 3 insertions(+), 172 deletions(-) diff --git a/pkg/storage/unified/resource/access.go b/pkg/storage/unified/resource/access.go index c0ddd9e6613..006444c5f45 100644 --- a/pkg/storage/unified/resource/access.go +++ b/pkg/storage/unified/resource/access.go @@ -110,24 +110,9 @@ func (c authzLimitedClient) Check(ctx context.Context, id claims.AuthInfo, req c attribute.String("name", req.Name), attribute.String("verb", req.Verb), attribute.String("folder", folder), - attribute.Bool("fallback_used", FallbackUsed(ctx)), )) defer span.End() - if FallbackUsed(ctx) { - if req.Namespace == "" { - // cross namespace queries are not allowed when fallback is used - span.SetAttributes(attribute.Bool("allowed", false)) - span.SetStatus(codes.Error, "Namespace empty") - err := fmt.Errorf("namespace empty") - span.RecordError(err) - return claims.CheckResponse{Allowed: false}, err - } - - span.SetAttributes(attribute.Bool("allowed", true)) - return claims.CheckResponse{Allowed: true}, nil - } - if !claims.NamespaceMatches(id.GetNamespace(), req.Namespace) { span.SetAttributes(attribute.Bool("allowed", false)) span.SetStatus(codes.Error, "Namespace mismatch") @@ -155,28 +140,14 @@ func (c authzLimitedClient) Check(ctx context.Context, id claims.AuthInfo, req c // Compile implements claims.AccessClient. func (c authzLimitedClient) Compile(ctx context.Context, id claims.AuthInfo, req claims.ListRequest) (claims.ItemChecker, claims.Zookie, error) { t := time.Now() - fallbackUsed := FallbackUsed(ctx) ctx, span := tracer.Start(ctx, "resource.authzLimitedClient.Compile", trace.WithAttributes( attribute.String("group", req.Group), attribute.String("resource", req.Resource), attribute.String("namespace", req.Namespace), attribute.String("verb", req.Verb), - attribute.Bool("fallback_used", fallbackUsed), )) defer span.End() - if fallbackUsed { - if req.Namespace == "" { - // cross namespace queries are not allowed when fallback is used - span.SetAttributes(attribute.Bool("allowed", false)) - span.SetStatus(codes.Error, "Namespace empty") - err := fmt.Errorf("namespace empty") - span.RecordError(err) - return nil, claims.NoopZookie{}, err - } - return func(name, folder string) bool { - return true - }, claims.NoopZookie{}, nil - } + if !claims.NamespaceMatches(id.GetNamespace(), req.Namespace) { span.SetAttributes(attribute.Bool("allowed", false)) span.SetStatus(codes.Error, "Namespace mismatch") @@ -211,13 +182,3 @@ func (c authzLimitedClient) IsCompatibleWithRBAC(group, resource string) bool { } var _ claims.AccessClient = &authzLimitedClient{} - -type contextFallbackKey struct{} - -func WithFallback(ctx context.Context) context.Context { - return context.WithValue(ctx, contextFallbackKey{}, true) -} - -func FallbackUsed(ctx context.Context) bool { - return ctx.Value(contextFallbackKey{}) != nil -} diff --git a/pkg/storage/unified/resource/access_test.go b/pkg/storage/unified/resource/access_test.go index 5d664aaba74..f0130bdd3d5 100644 --- a/pkg/storage/unified/resource/access_test.go +++ b/pkg/storage/unified/resource/access_test.go @@ -158,67 +158,3 @@ func TestNamespaceMatching(t *testing.T) { }) } } - -// TestNamespaceMatchingFallback tests namespace matching in Check and Compile methods when fallback is used -func TestNamespaceMatchingFallback(t *testing.T) { - // Create a mock client that always returns allowed=true - mockClient := authlib.FixedAccessClient(true) - client := NewAuthzLimitedClient(mockClient, AuthzOptions{}) - - // Create a context with fallback disabled - ctx := context.Background() - - tests := []struct { - name string - authNamespace string - reqNamespace string - expectError bool - }{ - { - name: "with namespace fallback", - reqNamespace: "ns1", - expectError: false, - }, - { - name: "empty request namespace with fallback", - reqNamespace: "", - expectError: true, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - // Test Check method with namespace matching - checkReq := authlib.CheckRequest{ - Group: "unknown.group", // Use unknown group to bypass RBAC check - Resource: "unknown.resource", - Verb: utils.VerbGet, - Namespace: tt.reqNamespace, - } - ctx = WithFallback(ctx) - // Create a mock auth info with the specified namespace - // Test Check method - user := &identity.StaticRequester{Namespace: tt.authNamespace} - _, checkErr := client.Check(ctx, user, checkReq, "") - - // Test Compile method - compileReq := authlib.ListRequest{ - Group: "unknown.group", // Use unknown group to bypass RBAC check - Resource: "unknown.resource", - Verb: utils.VerbGet, - Namespace: tt.reqNamespace, - } - _, _, compileErr := client.Compile(ctx, user, compileReq) - - if tt.expectError { - require.Error(t, checkErr, "Check should return error") - require.Error(t, compileErr, "Compile should return error") - assert.ErrorContains(t, checkErr, "namespace empty", "Check should return namespace mismatch error") - assert.ErrorContains(t, compileErr, "namespace empty", "Compile should return namespace mismatch error") - } else { - assert.NoError(t, checkErr, "Check should not return error when namespaces match") - assert.NoError(t, compileErr, "Compile should not return error when namespaces match") - } - }) - } -} diff --git a/pkg/storage/unified/sql/service.go b/pkg/storage/unified/sql/service.go index 06275c8754c..e47554536ea 100644 --- a/pkg/storage/unified/sql/service.go +++ b/pkg/storage/unified/sql/service.go @@ -13,9 +13,7 @@ import ( "github.com/gorilla/mux" "github.com/prometheus/client_golang/prometheus" - "github.com/prometheus/client_golang/prometheus/promauto" "go.opentelemetry.io/otel" - "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/trace" "google.golang.org/grpc/health/grpc_health_v1" @@ -34,7 +32,6 @@ import ( "github.com/grafana/grafana/pkg/services/grpcserver/interceptors" "github.com/grafana/grafana/pkg/setting" "github.com/grafana/grafana/pkg/storage/unified/resource" - "github.com/grafana/grafana/pkg/storage/unified/resource/grpc" "github.com/grafana/grafana/pkg/storage/unified/resourcepb" "github.com/grafana/grafana/pkg/storage/unified/search" "github.com/grafana/grafana/pkg/util/scheduler" @@ -103,12 +100,8 @@ func ProvideUnifiedStorageGrpcService( var err error tracer := otel.Tracer("unified-storage") - // FIXME: This is a temporary solution while we are migrating to the new authn interceptor - // grpcutils.NewGrpcAuthenticator should be used instead. - authn := NewAuthenticatorWithFallback(cfg, reg, tracer, func(ctx context.Context) (context.Context, error) { - auth := grpc.Authenticator{Tracer: tracer} - return auth.Authenticate(ctx) - }) + authCfg := ReadGrpcServerConfig(cfg) + authn := grpcutils.NewAuthenticator(authCfg, tracer) s := &service{ backend: backend, @@ -378,50 +371,6 @@ func (s *service) stopping(_ error) error { return nil } -type authenticatorWithFallback struct { - authenticator func(ctx context.Context) (context.Context, error) - fallback func(ctx context.Context) (context.Context, error) - metrics *metrics - tracer trace.Tracer -} - -type metrics struct { - requestsTotal *prometheus.CounterVec -} - -func (f *authenticatorWithFallback) Authenticate(ctx context.Context) (context.Context, error) { - ctx, span := f.tracer.Start(ctx, "grpcutils.AuthenticatorWithFallback.Authenticate") - defer span.End() - - // Try to authenticate with the new authenticator first - span.SetAttributes(attribute.Bool("fallback_used", false)) - newCtx, err := f.authenticator(ctx) - if err == nil { - // fallback not used, authentication successful - f.metrics.requestsTotal.WithLabelValues("false", "true").Inc() - return newCtx, nil - } - - // In case of error, fallback to the legacy authenticator - span.SetAttributes(attribute.Bool("fallback_used", true)) - newCtx, err = f.fallback(ctx) - if newCtx != nil { - newCtx = resource.WithFallback(newCtx) - } - f.metrics.requestsTotal.WithLabelValues("true", fmt.Sprintf("%t", err == nil)).Inc() - return newCtx, err -} - -func newMetrics(reg prometheus.Registerer) *metrics { - return &metrics{ - requestsTotal: promauto.With(reg).NewCounterVec( - prometheus.CounterOpts{ - Name: "grafana_grpc_authenticator_with_fallback_requests_total", - Help: "Number requests using the authenticator with fallback", - }, []string{"fallback_used", "result"}), - } -} - func ReadGrpcServerConfig(cfg *setting.Cfg) *grpcutils.AuthenticatorConfig { section := cfg.SectionWithEnvOverrides("grpc_server_authentication") @@ -432,21 +381,6 @@ func ReadGrpcServerConfig(cfg *setting.Cfg) *grpcutils.AuthenticatorConfig { } } -func NewAuthenticatorWithFallback(cfg *setting.Cfg, reg prometheus.Registerer, tracer trace.Tracer, fallback func(context.Context) (context.Context, error)) func(context.Context) (context.Context, error) { - authCfg := ReadGrpcServerConfig(cfg) - authenticator := grpcutils.NewAuthenticator(authCfg, tracer) - metrics := newMetrics(reg) - return func(ctx context.Context) (context.Context, error) { - a := &authenticatorWithFallback{ - authenticator: authenticator, - fallback: fallback, - tracer: tracer, - metrics: metrics, - } - return a.Authenticate(ctx) - } -} - func toLifecyclerConfig(cfg *setting.Cfg, logger log.Logger) (ring.BasicLifecyclerConfig, error) { instanceAddr, err := ring.GetInstanceAddr(cfg.MemberlistBindAddr, netutil.PrivateNetworkInterfacesWithFallback([]string{"eth0", "en0"}, logger), logger, true) if err != nil {