From ed13959e336ebee1c582ada14b72f4b815e4eb8b Mon Sep 17 00:00:00 2001 From: Jeff Levin Date: Wed, 26 Jun 2024 12:03:13 -0800 Subject: [PATCH] Optimize memory allocations in permissions cache (#89645) This PR reduces the number of allocations made while caching permissions from the database, fixes the hierarchy of spans and adds new spans for tracing. --------- Signed-off-by: Dave Henderson Co-authored-by: Dave Henderson --- pkg/api/common_test.go | 2 + pkg/api/dashboard.go | 21 ++++---- pkg/api/dashboard_test.go | 28 +++++++---- pkg/services/accesscontrol/acimpl/service.go | 52 ++++++++++---------- pkg/services/authn/clients/basic.go | 4 +- 5 files changed, 60 insertions(+), 47 deletions(-) diff --git a/pkg/api/common_test.go b/pkg/api/common_test.go index 4d8b651f568..9e7c2f6756c 100644 --- a/pkg/api/common_test.go +++ b/pkg/api/common_test.go @@ -274,6 +274,7 @@ func setupSimpleHTTPServer(features featuremgmt.FeatureToggles) *HTTPServer { authInfoService: &authinfotest.FakeService{ ExpectedLabels: map[int64]string{int64(1): login.GetAuthProviderLabel(login.LDAPAuthModule)}, }, + tracer: tracing.InitializeTracerForTest(), } } @@ -299,6 +300,7 @@ func SetupAPITestServer(t *testing.T, opts ...APITestServerOption) *webtest.Serv Features: featuremgmt.WithFeatures(), QuotaService: quotatest.New(false, nil), searchUsersService: &searchusers.OSSService{}, + tracer: tracing.InitializeTracerForTest(), } for _, opt := range opts { diff --git a/pkg/api/dashboard.go b/pkg/api/dashboard.go index 631b74a5bf6..f9f231db381 100644 --- a/pkg/api/dashboard.go +++ b/pkg/api/dashboard.go @@ -50,7 +50,6 @@ func (hs *HTTPServer) isDashboardStarredByUser(c *contextmodel.ReqContext, dashI } userID, err := identity.IntIdentifier(namespaceID, userIDstr) - if err != nil { return false, err } @@ -83,8 +82,11 @@ func dashboardGuardianResponse(err error) response.Response { // 404: notFoundError // 500: internalServerError func (hs *HTTPServer) GetDashboard(c *contextmodel.ReqContext) response.Response { + ctx, span := hs.tracer.Start(c.Req.Context(), "httpserver.GetDashboard") + defer span.End() + uid := web.Params(c.Req)[":uid"] - dash, rsp := hs.getDashboardHelper(c.Req.Context(), c.SignedInUser.GetOrgID(), 0, uid) + dash, rsp := hs.getDashboardHelper(ctx, c.SignedInUser.GetOrgID(), 0, uid) if rsp != nil { return rsp } @@ -96,7 +98,7 @@ func (hs *HTTPServer) GetDashboard(c *contextmodel.ReqContext) response.Response // If public dashboards is enabled and we have a public dashboard, update meta values if hs.Features.IsEnabledGlobally(featuremgmt.FlagPublicDashboards) && hs.Cfg.PublicDashboardsEnabled { - publicDashboard, err := hs.PublicDashboardsApi.PublicDashboardService.FindByDashboardUid(c.Req.Context(), c.SignedInUser.GetOrgID(), dash.UID) + publicDashboard, err := hs.PublicDashboardsApi.PublicDashboardService.FindByDashboardUid(ctx, c.SignedInUser.GetOrgID(), dash.UID) if err != nil && !errors.Is(err, publicdashboardModels.ErrPublicDashboardNotFound) { return response.Error(http.StatusInternalServerError, "Error while retrieving public dashboards", err) } @@ -119,7 +121,7 @@ func (hs *HTTPServer) GetDashboard(c *contextmodel.ReqContext) response.Response return response.Error(http.StatusInternalServerError, "Error while loading dashboard, dashboard data is invalid", nil) } } - guardian, err := guardian.NewByDashboard(c.Req.Context(), dash, c.SignedInUser.GetOrgID(), c.SignedInUser) + guardian, err := guardian.NewByDashboard(ctx, dash, c.SignedInUser.GetOrgID(), c.SignedInUser) if err != nil { return response.Err(err) } @@ -139,14 +141,14 @@ func (hs *HTTPServer) GetDashboard(c *contextmodel.ReqContext) response.Response // Finding creator and last updater of the dashboard updater, creator := anonString, anonString if dash.UpdatedBy > 0 { - updater = hs.getUserLogin(c.Req.Context(), dash.UpdatedBy) + updater = hs.getUserLogin(ctx, dash.UpdatedBy) } if dash.CreatedBy > 0 { - creator = hs.getUserLogin(c.Req.Context(), dash.CreatedBy) + creator = hs.getUserLogin(ctx, dash.CreatedBy) } annotationPermissions := &dashboardsV0.AnnotationPermission{} - if hs.Features.IsEnabled(c.Req.Context(), featuremgmt.FlagAnnotationPermissionUpdate) { + if hs.Features.IsEnabled(ctx, featuremgmt.FlagAnnotationPermissionUpdate) { hs.getAnnotationPermissionsByScope(c, &annotationPermissions.Dashboard, dashboards.ScopeDashboardsProvider.GetResourceScopeUID(dash.UID)) } else { hs.getAnnotationPermissionsByScope(c, &annotationPermissions.Dashboard, accesscontrol.ScopeAnnotationsTypeDashboard) @@ -182,7 +184,7 @@ func (hs *HTTPServer) GetDashboard(c *contextmodel.ReqContext) response.Response // nolint:staticcheck query := dashboards.GetDashboardQuery{ID: dash.FolderID, OrgID: c.SignedInUser.GetOrgID()} metrics.MFolderIDsAPICount.WithLabelValues(metrics.GetDashboard).Inc() - queryResult, err := hs.DashboardService.GetDashboard(c.Req.Context(), &query) + queryResult, err := hs.DashboardService.GetDashboard(ctx, &query) if err != nil { if errors.Is(err, dashboards.ErrFolderNotFound) { return response.Error(http.StatusNotFound, "Folder not found", err) @@ -194,7 +196,7 @@ func (hs *HTTPServer) GetDashboard(c *contextmodel.ReqContext) response.Response meta.FolderUrl = queryResult.GetURL() } - provisioningData, err := hs.dashboardProvisioningService.GetProvisionedDashboardDataByDashboardID(c.Req.Context(), dash.ID) + provisioningData, err := hs.dashboardProvisioningService.GetProvisionedDashboardDataByDashboardID(ctx, dash.ID) if err != nil { return response.Error(http.StatusInternalServerError, "Error while checking if dashboard is provisioned", err) } @@ -1004,7 +1006,6 @@ func (hs *HTTPServer) CalculateDashboardDiff(c *contextmodel.ReqContext) respons newData := newVersionRes.Data result, err := dashdiffs.CalculateDiff(c.Req.Context(), &options, baseData, newData) - if err != nil { if errors.Is(err, dashver.ErrDashboardVersionNotFound) { return response.Error(http.StatusNotFound, "Dashboard version not found", err) diff --git a/pkg/api/dashboard_test.go b/pkg/api/dashboard_test.go index 7b724e6209c..e249765952b 100644 --- a/pkg/api/dashboard_test.go +++ b/pkg/api/dashboard_test.go @@ -81,6 +81,7 @@ func TestGetHomeDashboard(t *testing.T) { preferenceService: prefService, dashboardVersionService: dashboardVersionService, log: log.New("test-logger"), + tracer: tracing.InitializeTracerForTest(), } tests := []struct { @@ -585,7 +586,8 @@ func TestDashboardAPIEndpoint(t *testing.T) { DashboardID: 2, Version: 1, Data: fakeDash.Data, - }} + }, + } mockSQLStore := dbtest.NewFakeDB() origNewGuardian := guardian.New guardian.MockDashboardGuardian(&guardian.FakeDashboardGuardian{CanSaveValue: true}) @@ -622,7 +624,8 @@ func TestDashboardAPIEndpoint(t *testing.T) { DashboardID: 2, Version: 1, Data: fakeDash.Data, - }} + }, + } cmd := dtos.RestoreDashboardVersionCommand{ Version: 1, @@ -680,6 +683,7 @@ func TestDashboardAPIEndpoint(t *testing.T) { DashboardService: dashboardService, Features: featuremgmt.WithFeatures(), starService: startest.NewStarServiceFake(), + tracer: tracing.InitializeTracerForTest(), } hs.callGetDashboard(sc) @@ -718,6 +722,7 @@ func TestDashboardVersionsAPIEndpoint(t *testing.T) { userService: userSvc, CacheService: localcache.New(5*time.Minute, 10*time.Minute), log: log.New(), + tracer: tracing.InitializeTracerForTest(), } } @@ -855,6 +860,7 @@ func getDashboardShouldReturn200WithConfig(t *testing.T, sc *scenarioContext, pr DashboardService: dashboardService, Features: featuremgmt.WithFeatures(), starService: startest.NewStarServiceFake(), + tracer: tracing.InitializeTracerForTest(), } hs.callGetDashboard(sc) @@ -908,6 +914,7 @@ func postDashboardScenario(t *testing.T, desc string, url string, routePattern s Features: featuremgmt.WithFeatures(), accesscontrolService: actest.FakeService{}, log: log.New("test-logger"), + tracer: tracing.InitializeTracerForTest(), } sc := setupScenarioContext(t, url) @@ -927,7 +934,8 @@ func postDashboardScenario(t *testing.T, desc string, url string, routePattern s } func postDiffScenario(t *testing.T, desc string, url string, routePattern string, cmd dtos.CalculateDiffOptions, - role org.RoleType, fn scenarioFunc, sqlmock db.DB, fakeDashboardVersionService *dashvertest.FakeDashboardVersionService) { + role org.RoleType, fn scenarioFunc, sqlmock db.DB, fakeDashboardVersionService *dashvertest.FakeDashboardVersionService, +) { t.Run(fmt.Sprintf("%s %s", desc, url), func(t *testing.T) { cfg := setting.NewCfg() @@ -943,6 +951,7 @@ func postDiffScenario(t *testing.T, desc string, url string, routePattern string dashboardVersionService: fakeDashboardVersionService, Features: featuremgmt.WithFeatures(), DashboardService: dashSvc, + tracer: tracing.InitializeTracerForTest(), } sc := setupScenarioContext(t, url) @@ -967,7 +976,8 @@ func postDiffScenario(t *testing.T, desc string, url string, routePattern string func restoreDashboardVersionScenario(t *testing.T, desc string, url string, routePattern string, mock *dashboards.FakeDashboardService, fakeDashboardVersionService *dashvertest.FakeDashboardVersionService, - cmd dtos.RestoreDashboardVersionCommand, fn scenarioFunc, sqlStore db.DB) { + cmd dtos.RestoreDashboardVersionCommand, fn scenarioFunc, sqlStore db.DB, +) { t.Run(fmt.Sprintf("%s %s", desc, url), func(t *testing.T) { cfg := setting.NewCfg() folderSvc := foldertest.NewFakeService() @@ -986,6 +996,7 @@ func restoreDashboardVersionScenario(t *testing.T, desc string, url string, rout dashboardVersionService: fakeDashboardVersionService, accesscontrolService: actest.FakeService{}, folderService: folderSvc, + tracer: tracing.InitializeTracerForTest(), } sc := setupScenarioContext(t, url) @@ -1022,12 +1033,12 @@ type mockDashboardProvisioningService struct { } func (s mockDashboardProvisioningService) GetProvisionedDashboardDataByDashboardID(ctx context.Context, dashboardID int64) ( - *dashboards.DashboardProvisioning, error) { + *dashboards.DashboardProvisioning, error, +) { return nil, nil } -type mockLibraryPanelService struct { -} +type mockLibraryPanelService struct{} var _ librarypanels.Service = (*mockLibraryPanelService)(nil) @@ -1039,8 +1050,7 @@ func (m *mockLibraryPanelService) ImportLibraryPanelsForDashboard(c context.Cont return nil } -type mockLibraryElementService struct { -} +type mockLibraryElementService struct{} func (l *mockLibraryElementService) CreateElement(c context.Context, signedInUser identity.Requester, cmd model.CreateLibraryElementCommand) (model.LibraryElementDTO, error) { return model.LibraryElementDTO{}, nil diff --git a/pkg/services/accesscontrol/acimpl/service.go b/pkg/services/accesscontrol/acimpl/service.go index eb9010513aa..04c5c790aa3 100644 --- a/pkg/services/accesscontrol/acimpl/service.go +++ b/pkg/services/accesscontrol/acimpl/service.go @@ -9,6 +9,7 @@ import ( "time" "github.com/prometheus/client_golang/prometheus" + "go.opentelemetry.io/otel/attribute" "github.com/grafana/grafana/pkg/api/routing" "github.com/grafana/grafana/pkg/apimachinery/identity" @@ -45,8 +46,7 @@ var SharedWithMeFolderPermission = accesscontrol.Permission{ var OSSRolesPrefixes = []string{accesscontrol.ManagedRolePrefix, accesscontrol.ExternalServiceRolePrefix} -func ProvideService(cfg *setting.Cfg, db db.DB, routeRegister routing.RouteRegister, cache *localcache.CacheService, - accessControl accesscontrol.AccessControl, actionResolver accesscontrol.ActionResolver, features featuremgmt.FeatureToggles, tracer tracing.Tracer) (*Service, error) { +func ProvideService(cfg *setting.Cfg, db db.DB, routeRegister routing.RouteRegister, cache *localcache.CacheService, accessControl accesscontrol.AccessControl, actionResolver accesscontrol.ActionResolver, features featuremgmt.FeatureToggles, tracer tracing.Tracer) (*Service, error) { service := ProvideOSSService(cfg, database.ProvideService(db), actionResolver, cache, features, tracer) api.NewAccessControlAPI(routeRegister, accessControl, service, features).RegisterAPIEndpoints() @@ -151,9 +151,9 @@ func (s *Service) getBasicRolePermissions(ctx context.Context, role string, orgI ctx, span := s.tracer.Start(ctx, "authz.getBasicRolePermissions") defer span.End() - permissions := make([]accesscontrol.Permission, 0) + var permissions []accesscontrol.Permission if basicRole, ok := s.roles[role]; ok { - permissions = append(permissions, basicRole.Permissions...) + permissions = basicRole.Permissions } // Fetch managed role permissions assigned to basic roles @@ -209,7 +209,6 @@ func (s *Service) getUserDirectPermissions(ctx context.Context, user identity.Re UserID: userID, RolePrefixes: OSSRolesPrefixes, }) - if err != nil { return nil, err } @@ -225,12 +224,16 @@ func (s *Service) getUserDirectPermissions(ctx context.Context, user identity.Re } func (s *Service) getCachedUserPermissions(ctx context.Context, user identity.Requester, options accesscontrol.Options) ([]accesscontrol.Permission, error) { - basicRolesPermissions, err := s.getCachedBasicRolesPermissions(ctx, user, options) + ctx, span := s.tracer.Start(ctx, "authz.getCachedUserPermissions") + defer span.End() + + permissions := []accesscontrol.Permission{} + permissions, err := s.getCachedBasicRolesPermissions(ctx, user, options, permissions) if err != nil { return nil, err } - teamsPermissions, err := s.getCachedTeamsPermissions(ctx, user, options) + permissions, err = s.getCachedTeamsPermissions(ctx, user, options, permissions) if err != nil { return nil, err } @@ -240,32 +243,33 @@ func (s *Service) getCachedUserPermissions(ctx context.Context, user identity.Re return nil, err } - permissions := make([]accesscontrol.Permission, 0, len(basicRolesPermissions)+len(teamsPermissions)+len(userPermissions)) - permissions = append(permissions, basicRolesPermissions...) - permissions = append(permissions, teamsPermissions...) permissions = append(permissions, userPermissions...) + span.SetAttributes(attribute.Int("num_permissions", len(permissions))) + return permissions, nil } -func (s *Service) getCachedBasicRolesPermissions(ctx context.Context, user identity.Requester, options accesscontrol.Options) ([]accesscontrol.Permission, error) { +func (s *Service) getCachedBasicRolesPermissions(ctx context.Context, user identity.Requester, options accesscontrol.Options, permissions []accesscontrol.Permission) ([]accesscontrol.Permission, error) { ctx, span := s.tracer.Start(ctx, "authz.getCachedBasicRolesPermissions") defer span.End() basicRoles := accesscontrol.GetOrgRoles(user) - basicRolesPermissions := make([]accesscontrol.Permission, 0) for _, role := range basicRoles { - permissions, err := s.getCachedBasicRolePermissions(ctx, role, user.GetOrgID(), options) + perms, err := s.getCachedBasicRolePermissions(ctx, role, user.GetOrgID(), options) if err != nil { return nil, err } - basicRolesPermissions = append(basicRolesPermissions, permissions...) + permissions = append(permissions, perms...) } - return basicRolesPermissions, nil + return permissions, nil } func (s *Service) getCachedBasicRolePermissions(ctx context.Context, role string, orgID int64, options accesscontrol.Options) ([]accesscontrol.Permission, error) { + ctx, span := s.tracer.Start(ctx, "authz.getCachedBasicRolePermissions") + defer span.End() + key := accesscontrol.GetBasicRolePermissionCacheKey(role, orgID) - getPermissionsFn := func() ([]accesscontrol.Permission, error) { + getPermissionsFn := func(ctx context.Context) ([]accesscontrol.Permission, error) { return s.getBasicRolePermissions(ctx, role, orgID) } return s.getCachedPermissions(ctx, key, getPermissionsFn, options) @@ -276,17 +280,17 @@ func (s *Service) getCachedUserDirectPermissions(ctx context.Context, user ident defer span.End() key := accesscontrol.GetUserDirectPermissionCacheKey(user) - getUserPermissionsFn := func() ([]accesscontrol.Permission, error) { + getUserPermissionsFn := func(ctx context.Context) ([]accesscontrol.Permission, error) { return s.getUserDirectPermissions(ctx, user) } return s.getCachedPermissions(ctx, key, getUserPermissionsFn, options) } -type GetPermissionsFn = func() ([]accesscontrol.Permission, error) +type getPermissionsFunc = func(ctx context.Context) ([]accesscontrol.Permission, error) // Generic method for getting various permissions from cache -func (s *Service) getCachedPermissions(ctx context.Context, key string, getPermissionsFn GetPermissionsFn, options accesscontrol.Options) ([]accesscontrol.Permission, error) { - _, span := s.tracer.Start(ctx, "authz.getCachedTeamsPermissions") +func (s *Service) getCachedPermissions(ctx context.Context, key string, getPermissionsFn getPermissionsFunc, options accesscontrol.Options) ([]accesscontrol.Permission, error) { + _, span := s.tracer.Start(ctx, "authz.getCachedPermissions") defer span.End() if !options.ReloadCache { @@ -299,7 +303,7 @@ func (s *Service) getCachedPermissions(ctx context.Context, key string, getPermi span.AddEvent("cache miss") metrics.MAccessPermissionsCacheUsage.WithLabelValues(accesscontrol.CacheMiss).Inc() - permissions, err := getPermissionsFn() + permissions, err := getPermissionsFn(ctx) if err != nil { return nil, err } @@ -308,13 +312,12 @@ 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) ([]accesscontrol.Permission, error) { +func (s *Service) getCachedTeamsPermissions(ctx context.Context, user identity.Requester, options accesscontrol.Options, permissions []accesscontrol.Permission) ([]accesscontrol.Permission, error) { ctx, span := s.tracer.Start(ctx, "authz.getCachedTeamsPermissions") defer span.End() teams := user.GetTeams() orgID := user.GetOrgID() - permissions := make([]accesscontrol.Permission, 0) miss := teams if !options.ReloadCache { @@ -433,8 +436,7 @@ func GetActionFilter(options accesscontrol.SearchOptions) func(action string) bo } // SearchUsersPermissions returns all users' permissions filtered by action prefixes -func (s *Service) SearchUsersPermissions(ctx context.Context, usr identity.Requester, - options accesscontrol.SearchOptions) (map[int64][]accesscontrol.Permission, error) { +func (s *Service) SearchUsersPermissions(ctx context.Context, usr identity.Requester, options accesscontrol.SearchOptions) (map[int64][]accesscontrol.Permission, error) { // Limit roles to available in OSS options.RolePrefixes = OSSRolesPrefixes if options.NamespacedID != "" { diff --git a/pkg/services/authn/clients/basic.go b/pkg/services/authn/clients/basic.go index f27d15672e8..1aa4db44f4f 100644 --- a/pkg/services/authn/clients/basic.go +++ b/pkg/services/authn/clients/basic.go @@ -7,9 +7,7 @@ import ( "github.com/grafana/grafana/pkg/services/authn" ) -var ( - errDecodingBasicAuthHeader = errutil.BadRequest("basic-auth.invalid-header", errutil.WithPublicMessage("Invalid Basic Auth Header")) -) +var errDecodingBasicAuthHeader = errutil.BadRequest("basic-auth.invalid-header", errutil.WithPublicMessage("Invalid Basic Auth Header")) var _ authn.ContextAwareClient = new(Basic)