From fbc81d2fd0037e1abe90e0eeeb4cb95df027121b Mon Sep 17 00:00:00 2001 From: Dave Henderson Date: Wed, 22 Oct 2025 18:25:10 -0400 Subject: [PATCH] fix(accesscontrol): Reduce memory usage in GroupScopesByActionContext (#112295) Signed-off-by: Dave Henderson --- pkg/services/accesscontrol/accesscontrol.go | 55 +++++++++++++++++- .../accesscontrol/accesscontrol_test.go | 58 +++++++++++++------ .../authn/authnimpl/sync/rbac_sync.go | 8 ++- 3 files changed, 97 insertions(+), 24 deletions(-) diff --git a/pkg/services/accesscontrol/accesscontrol.go b/pkg/services/accesscontrol/accesscontrol.go index faf2b962ab9..39bedde31fd 100644 --- a/pkg/services/accesscontrol/accesscontrol.go +++ b/pkg/services/accesscontrol/accesscontrol.go @@ -244,10 +244,59 @@ func GroupScopesByActionContext(ctx context.Context, permissions []Permission) m )) defer span.End() - m := make(map[string][]string) - for i := range permissions { - m[permissions[i].Action] = append(m[permissions[i].Action], permissions[i].Scope) + // Note: this has been optimized to improve memory usage in large instances + // where there are lots of permissions. This isn't quite as fast as it can + // be, but we should prioritize memory over speed in this case. + + if len(permissions) == 0 { + return make(map[string][]string) } + + // Use index-based approach with cached lookups for better performance + // First pass: assign and cache indices for each permission + actionIndex := make(map[string]int) + indices := make([]int, len(permissions)) + + for i := range permissions { + action := permissions[i].Action + if idx, ok := actionIndex[action]; ok { + indices[i] = idx + } else { + idx = len(actionIndex) + actionIndex[action] = idx + indices[i] = idx + } + } + + // Count scopes per action using cached indices + actionCounts := make([]int, len(actionIndex)) + for i := range indices { + actionCounts[indices[i]]++ + } + + // Preallocate slice array with exact capacities + scopes := make([][]string, len(actionCounts)) + for i, count := range actionCounts { + scopes[i] = make([]string, 0, count) + } + + // Second pass: append scopes using cached indices (no map lookups!) + for i := range permissions { + idx := indices[i] + scopes[idx] = append(scopes[idx], permissions[i].Scope) + } + + // Build result map + m := make(map[string][]string, len(actionIndex)) + for action, idx := range actionIndex { + m[action] = scopes[idx] + } + + span.SetAttributes( + attribute.Int("unique_actions", len(actionIndex)), + attribute.Float64("avg_scopes_per_action", float64(len(permissions))/float64(len(actionIndex))), + ) + return m } diff --git a/pkg/services/accesscontrol/accesscontrol_test.go b/pkg/services/accesscontrol/accesscontrol_test.go index f56dff456a0..d61bfb1293c 100644 --- a/pkg/services/accesscontrol/accesscontrol_test.go +++ b/pkg/services/accesscontrol/accesscontrol_test.go @@ -155,27 +155,47 @@ func TestGroupScopesByActionContext(t *testing.T) { } func BenchmarkGroupScopesByAction(b *testing.B) { - // create a big list of permissions with a bunch of duplicates - permissions := []Permission{} - for i := 0; i < 100; i++ { - for j := 0; j < 500+i; j++ { - permissions = append(permissions, Permission{ - Action: fmt.Sprintf("action:%d", i), - Scope: fmt.Sprintf("scope:%d_%d", i, j), - }) - } - // add duplicate scopes - for j := 0; j < 10; j++ { - permissions = append(permissions, Permission{ - Action: fmt.Sprintf("action:%d", i), - Scope: fmt.Sprintf("scope:%d_%d", i, 0), - }) - } + testCases := []struct { + name string + numActions int + totalPerms int + avgPerAction int + }{ + {"small", 10, 1000, 100}, + {"medium", 50, 10000, 200}, + {"large", 100, 70000, 700}, } - b.ResetTimer() + for _, tc := range testCases { + b.Run(tc.name, func(b *testing.B) { + permissions := make([]Permission, 0, tc.totalPerms) - for i := 0; i < b.N; i++ { - GroupScopesByActionContext(context.Background(), permissions) + // Create realistic distribution with variance + // Some actions have more scopes than others + for i := 0; i < tc.numActions; i++ { + // Add variance: some actions get more scopes + scopeCount := tc.avgPerAction + if i%3 == 0 { + scopeCount = scopeCount * 2 + } else if i%5 == 0 { + scopeCount = scopeCount / 2 + } + + for j := 0; j < scopeCount && len(permissions) < tc.totalPerms; j++ { + permissions = append(permissions, Permission{ + Action: fmt.Sprintf("action:%d", i), + Scope: fmt.Sprintf("scope:%d_%d", i, j), + }) + } + } + + b.ReportMetric(float64(len(permissions)), "permissions") + b.ReportMetric(float64(tc.numActions), "actions") + b.ResetTimer() + + for b.Loop() { + GroupScopesByActionContext(context.Background(), permissions) + } + }) } } diff --git a/pkg/services/authn/authnimpl/sync/rbac_sync.go b/pkg/services/authn/authnimpl/sync/rbac_sync.go index acc0dbf83a9..4c94e5d405e 100644 --- a/pkg/services/authn/authnimpl/sync/rbac_sync.go +++ b/pkg/services/authn/authnimpl/sync/rbac_sync.go @@ -5,9 +5,11 @@ import ( "errors" "strings" + claims "github.com/grafana/authlib/types" + "go.opentelemetry.io/otel/attribute" + "go.opentelemetry.io/otel/trace" "golang.org/x/exp/maps" - claims "github.com/grafana/authlib/types" "github.com/grafana/grafana/pkg/apimachinery/errutil" "github.com/grafana/grafana/pkg/infra/log" "github.com/grafana/grafana/pkg/infra/tracing" @@ -43,7 +45,9 @@ type RBACSync struct { } func (s *RBACSync) SyncPermissionsHook(ctx context.Context, ident *authn.Identity, _ *authn.Request) error { - ctx, span := s.tracer.Start(ctx, "rbac.sync.SyncPermissionsHook") + ctx, span := s.tracer.Start(ctx, "rbac.sync.SyncPermissionsHook", trace.WithAttributes( + attribute.String("ident_uid", ident.UID), + )) defer span.End() if !ident.ClientParams.SyncPermissions {