From 88259da745a82adcc1776c29be810519b3abc0ec Mon Sep 17 00:00:00 2001 From: Alexander Zobnin Date: Tue, 3 Sep 2024 15:46:56 +0200 Subject: [PATCH] RBAC: Optimize permissions caching (#92673) * Access control: Use composite cache key for team permissions * use composite key for teams * use cache for hotpath (getCachedUserPermissions) * don't cache empty teams set * don't pass permissions as argument * early return if no teams found * reload cache correctly * optimize allocations * Clear user's teams cache * remove composite cache for teams * fix linter * don't clear teams permissions * pre-allocate memory for basic roles permissions --- pkg/services/accesscontrol/acimpl/service.go | 44 +++++++++++++------ pkg/services/accesscontrol/cacheutils.go | 2 +- pkg/services/accesscontrol/cacheutils_test.go | 3 +- 3 files changed, 33 insertions(+), 16 deletions(-) diff --git a/pkg/services/accesscontrol/acimpl/service.go b/pkg/services/accesscontrol/acimpl/service.go index 8d45bfb2677..3d5502afbf1 100644 --- a/pkg/services/accesscontrol/acimpl/service.go +++ b/pkg/services/accesscontrol/acimpl/service.go @@ -12,6 +12,7 @@ import ( "go.opentelemetry.io/otel/attribute" "github.com/grafana/authlib/claims" + "github.com/grafana/grafana/pkg/api/routing" "github.com/grafana/grafana/pkg/apimachinery/identity" "github.com/grafana/grafana/pkg/infra/db" @@ -241,32 +242,40 @@ func (s *Service) getCachedUserPermissions(ctx context.Context, user identity.Re ctx, span := tracer.Start(ctx, "accesscontrol.acimpl.getCachedUserPermissions") defer span.End() - permissions := []accesscontrol.Permission{} - permissions, err := s.getCachedBasicRolesPermissions(ctx, user, options, permissions) + cacheKey := accesscontrol.GetUserPermissionCacheKey(user) + if cachedPermissions, ok := s.cache.Get(cacheKey); ok { + return cachedPermissions.([]accesscontrol.Permission), nil + } + + permissions, err := s.getCachedBasicRolesPermissions(ctx, user, options) if err != nil { return nil, err } - permissions, err = s.getCachedTeamsPermissions(ctx, user, options, permissions) + teamsPermissions, err := s.getCachedTeamsPermissions(ctx, user, options) + if err != nil { + return nil, err + } + permissions = append(permissions, teamsPermissions...) + + userManagedPermissions, err := s.getCachedUserDirectPermissions(ctx, user, options) if err != nil { return nil, err } - userPermissions, err := s.getCachedUserDirectPermissions(ctx, user, options) - if err != nil { - return nil, err - } - - permissions = append(permissions, userPermissions...) + permissions = append(permissions, userManagedPermissions...) + s.cache.Set(cacheKey, permissions, cacheTTL) span.SetAttributes(attribute.Int("num_permissions", len(permissions))) return permissions, nil } -func (s *Service) getCachedBasicRolesPermissions(ctx context.Context, user identity.Requester, options accesscontrol.Options, permissions []accesscontrol.Permission) ([]accesscontrol.Permission, error) { +func (s *Service) getCachedBasicRolesPermissions(ctx context.Context, user identity.Requester, options accesscontrol.Options) ([]accesscontrol.Permission, error) { ctx, span := tracer.Start(ctx, "accesscontrol.acimpl.getCachedBasicRolesPermissions") defer span.End() + // Viewer role has ~30 permissions, so we can pre-allocate memory + permissions := make([]accesscontrol.Permission, 0, 50) basicRoles := accesscontrol.GetOrgRoles(user) span.SetAttributes(attribute.Int("roles", len(basicRoles))) for _, role := range basicRoles { @@ -330,16 +339,20 @@ func (s *Service) getCachedPermissions(ctx context.Context, key string, getPermi return permissions, nil } -func (s *Service) getCachedTeamsPermissions(ctx context.Context, user identity.Requester, options accesscontrol.Options, permissions []accesscontrol.Permission) ([]accesscontrol.Permission, error) { +func (s *Service) getCachedTeamsPermissions(ctx context.Context, user identity.Requester, options accesscontrol.Options) ([]accesscontrol.Permission, error) { ctx, span := tracer.Start(ctx, "accesscontrol.acimpl.getCachedTeamsPermissions") defer span.End() teams := user.GetTeams() orgID := user.GetOrgID() - miss := teams + if len(teams) == 0 { + return []accesscontrol.Permission{}, nil + } + + miss := make([]int64, 0) + permissions := make([]accesscontrol.Permission, 0) if !options.ReloadCache { - miss = make([]int64, 0) for _, teamID := range teams { key := accesscontrol.GetTeamPermissionCacheKey(teamID, orgID) teamPermissions, ok := s.cache.Get(key) @@ -351,6 +364,9 @@ func (s *Service) getCachedTeamsPermissions(ctx context.Context, user identity.R miss = append(miss, teamID) } } + } else { + // reload cache and fetch all teams permissions + miss = teams } if len(miss) > 0 { @@ -373,7 +389,7 @@ func (s *Service) getCachedTeamsPermissions(ctx context.Context, user identity.R } func (s *Service) ClearUserPermissionCache(user identity.Requester) { - s.cache.Delete(accesscontrol.GetPermissionCacheKey(user)) + s.cache.Delete(accesscontrol.GetUserPermissionCacheKey(user)) s.cache.Delete(accesscontrol.GetUserDirectPermissionCacheKey(user)) } diff --git a/pkg/services/accesscontrol/cacheutils.go b/pkg/services/accesscontrol/cacheutils.go index 9326d9d88c8..39d1d1a45f8 100644 --- a/pkg/services/accesscontrol/cacheutils.go +++ b/pkg/services/accesscontrol/cacheutils.go @@ -7,7 +7,7 @@ import ( "github.com/grafana/grafana/pkg/apimachinery/identity" ) -func GetPermissionCacheKey(user identity.Requester) string { +func GetUserPermissionCacheKey(user identity.Requester) string { return fmt.Sprintf("rbac-permissions-%s", user.GetCacheKey()) } diff --git a/pkg/services/accesscontrol/cacheutils_test.go b/pkg/services/accesscontrol/cacheutils_test.go index 7aff7941586..c5bf747b60b 100644 --- a/pkg/services/accesscontrol/cacheutils_test.go +++ b/pkg/services/accesscontrol/cacheutils_test.go @@ -6,6 +6,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/grafana/authlib/claims" + "github.com/grafana/grafana/pkg/services/org" "github.com/grafana/grafana/pkg/services/user" ) @@ -68,7 +69,7 @@ func TestPermissionCacheKey(t *testing.T) { for _, tc := range testcases { t.Run(tc.name, func(t *testing.T) { - assert.Equal(t, tc.expected, GetPermissionCacheKey(tc.signedInUser)) + assert.Equal(t, tc.expected, GetUserPermissionCacheKey(tc.signedInUser)) }) } }