From 5e48804364609c9ed70a2002fc9100c658f0dd08 Mon Sep 17 00:00:00 2001 From: Gabriel MABILLE Date: Mon, 25 Mar 2024 19:11:17 +0100 Subject: [PATCH] RBAC: Fix slow user permission search query on MySQL (#85058) * Bench testing search user perm * Add BenchmarkSearchUsersPermissions_1K_1K * Clarify benchmark searches by action prefix * Make MySQL more efficient * Move all filter options * Expand after assignments union * update comments --- .../acimpl/service_bench_test.go | 71 ++++++++--- .../accesscontrol/database/database.go | 110 ++++++++++++------ 2 files changed, 129 insertions(+), 52 deletions(-) diff --git a/pkg/services/accesscontrol/acimpl/service_bench_test.go b/pkg/services/accesscontrol/acimpl/service_bench_test.go index 37e83cfb2e9..c3b1a103b84 100644 --- a/pkg/services/accesscontrol/acimpl/service_bench_test.go +++ b/pkg/services/accesscontrol/acimpl/service_bench_test.go @@ -9,6 +9,7 @@ import ( "github.com/stretchr/testify/require" "github.com/grafana/grafana/pkg/infra/db" + "github.com/grafana/grafana/pkg/infra/localcache" "github.com/grafana/grafana/pkg/infra/log" "github.com/grafana/grafana/pkg/services/accesscontrol" "github.com/grafana/grafana/pkg/services/accesscontrol/actest" @@ -32,6 +33,7 @@ func setupBenchEnv(b *testing.B, usersCount, resourceCount int) (accesscontrol.S registrations: accesscontrol.RegistrationList{}, store: store, roles: accesscontrol.BuildBasicRoleDefinitions(), + cache: localcache.New(1*time.Second, 1*time.Second), } // Prepare default permissions @@ -54,6 +56,7 @@ func setupBenchEnv(b *testing.B, usersCount, resourceCount int) (accesscontrol.S for u := start + 1; u < end+1; u++ { users = append(users, user.User{ ID: int64(u), + UID: fmt.Sprintf("user%v", u), Name: fmt.Sprintf("user%v", u), Login: fmt.Sprintf("user%v", u), Email: fmt.Sprintf("user%v@example.org", u), @@ -136,7 +139,7 @@ func setupBenchEnv(b *testing.B, usersCount, resourceCount int) (accesscontrol.S return acService, &user.SignedInUser{OrgID: 1, Permissions: map[int64]map[string][]string{1: userPermissions}} } -func benchSearchUsersPermissions(b *testing.B, usersCount, resourceCount int) { +func benchSearchUsersWithActionPrefix(b *testing.B, usersCount, resourceCount int) { acService, siu := setupBenchEnv(b, usersCount, resourceCount) b.ResetTimer() @@ -152,49 +155,62 @@ func benchSearchUsersPermissions(b *testing.B, usersCount, resourceCount int) { } // Lots of resources -func BenchmarkSearchUsersPermissions_10_1K(b *testing.B) { benchSearchUsersPermissions(b, 10, 1000) } // ~0.047s/op -func BenchmarkSearchUsersPermissions_10_10K(b *testing.B) { benchSearchUsersPermissions(b, 10, 10000) } // ~0.5s/op -func BenchmarkSearchUsersPermissions_10_100K(b *testing.B) { +func BenchmarkSearchUsersWithActionPrefix_10_1K(b *testing.B) { + benchSearchUsersWithActionPrefix(b, 10, 1000) +} // ~0.047s/op +func BenchmarkSearchUsersWithActionPrefix_10_10K(b *testing.B) { + benchSearchUsersWithActionPrefix(b, 10, 10000) +} // ~0.5s/op +func BenchmarkSearchUsersWithActionPrefix_10_100K(b *testing.B) { if testing.Short() { b.Skip("Skipping benchmark in short mode") } - benchSearchUsersPermissions(b, 10, 100000) + benchSearchUsersWithActionPrefix(b, 10, 100000) } // ~4.6s/op -func BenchmarkSearchUsersPermissions_10_1M(b *testing.B) { +func BenchmarkSearchUsersWithActionPrefix_10_1M(b *testing.B) { if testing.Short() { b.Skip("Skipping benchmark in short mode") } - benchSearchUsersPermissions(b, 10, 1000000) + benchSearchUsersWithActionPrefix(b, 10, 1000000) } // ~55.36s/op // Lots of users (most probable case) -func BenchmarkSearchUsersPermissions_1K_10(b *testing.B) { benchSearchUsersPermissions(b, 1000, 10) } // ~0.056s/op -func BenchmarkSearchUsersPermissions_10K_10(b *testing.B) { benchSearchUsersPermissions(b, 10000, 10) } // ~0.58s/op -func BenchmarkSearchUsersPermissions_100K_10(b *testing.B) { +func BenchmarkSearchUsersWithActionPrefix_1K_10(b *testing.B) { + benchSearchUsersWithActionPrefix(b, 1000, 10) +} // ~0.056s/op +func BenchmarkSearchUsersWithActionPrefix_10K_10(b *testing.B) { + benchSearchUsersWithActionPrefix(b, 10000, 10) +} // ~0.58s/op +func BenchmarkSearchUsersWithActionPrefix_100K_10(b *testing.B) { if testing.Short() { b.Skip("Skipping benchmark in short mode") } - benchSearchUsersPermissions(b, 100000, 10) + benchSearchUsersWithActionPrefix(b, 100000, 10) } // ~6.21s/op -func BenchmarkSearchUsersPermissions_1M_10(b *testing.B) { +func BenchmarkSearchUsersWithActionPrefix_1M_10(b *testing.B) { if testing.Short() { b.Skip("Skipping benchmark in short mode") } - benchSearchUsersPermissions(b, 1000000, 10) + benchSearchUsersWithActionPrefix(b, 1000000, 10) } // ~57s/op // Lots of both -func BenchmarkSearchUsersPermissions_10K_100(b *testing.B) { + +func BenchmarkSearchUsersWithActionPrefix_1K_1K(b *testing.B) { + benchSearchUsersWithActionPrefix(b, 1000, 1000) +} + +func BenchmarkSearchUsersWithActionPrefix_10K_100(b *testing.B) { if testing.Short() { b.Skip("Skipping benchmark in short mode") } - benchSearchUsersPermissions(b, 10000, 100) + benchSearchUsersWithActionPrefix(b, 10000, 100) } // ~1.45s/op -func BenchmarkSearchUsersPermissions_10K_1K(b *testing.B) { +func BenchmarkSearchUsersWithActionPrefix_10K_1K(b *testing.B) { if testing.Short() { b.Skip("Skipping benchmark in short mode") } - benchSearchUsersPermissions(b, 10000, 1000) + benchSearchUsersWithActionPrefix(b, 10000, 1000) } // ~50s/op // Benchmarking search when we specify Action and Scope @@ -234,3 +250,24 @@ func BenchmarkSearchUsersWithPerm_20K_10K(b *testing.B) { benchSearchUsersWithPe func BenchmarkSearchUsersWithPerm_100K_10(b *testing.B) { benchSearchUsersWithPerm(b, 100000, 10) } // ~0.88s/op func BenchmarkSearchUsersWithPerm_100K_100(b *testing.B) { benchSearchUsersWithPerm(b, 100000, 100) } // ~0.72s/op + +// Benchmarking search when we specify Action and Scope +func benchSearchUserWithAction(b *testing.B, usersCount, resourceCount int) { + if testing.Short() { + b.Skip("Skipping benchmark in short mode") + } + acService, siu := setupBenchEnv(b, usersCount, resourceCount) + b.ResetTimer() + + for n := 0; n < b.N; n++ { + usersPermissions, err := acService.SearchUsersPermissions(context.Background(), siu, + accesscontrol.SearchOptions{Action: "resources:action2", NamespacedID: "user:14"}) + require.NoError(b, err) + require.Len(b, usersPermissions, 1) + for _, permissions := range usersPermissions { + require.Len(b, permissions, resourceCount) + } + } +} + +func BenchmarkSearchUserWithAction_1K_1k(b *testing.B) { benchSearchUserWithAction(b, 1000, 1000) } // ~0.6s/op (mysql) diff --git a/pkg/services/accesscontrol/database/database.go b/pkg/services/accesscontrol/database/database.go index fa91e954908..9d6187cd670 100644 --- a/pkg/services/accesscontrol/database/database.go +++ b/pkg/services/accesscontrol/database/database.go @@ -2,6 +2,7 @@ package database import ( "context" + "fmt" "strconv" "strings" @@ -9,6 +10,32 @@ import ( "github.com/grafana/grafana/pkg/services/accesscontrol" ) +const ( + // userAssignsSQL is a query to select all users assignments. + userAssignsSQL = `SELECT ur.user_id, ur.org_id, ur.role_id + FROM user_role AS ur` + + // teamAssignsSQL is a query to select all users' team assignments. + teamAssignsSQL = `SELECT tm.user_id, tr.org_id, tr.role_id + FROM team_role AS tr + INNER JOIN team_member AS tm ON tm.team_id = tr.team_id` + + // basicRoleAssignsSQL is a query to select all users basic role (Admin, Editor, Viewer, None) assignments. + basicRoleAssignsSQL = `SELECT ou.user_id, ou.org_id, br.role_id + FROM builtin_role AS br + INNER JOIN org_user AS ou ON ou.role = br.role` + + // grafanaAdminAssignsSQL is a query to select all grafana admin users. + // it has to be formatted with the quoted user table. + grafanaAdminAssignsSQL = `SELECT sa.user_id, br.org_id, br.role_id + FROM builtin_role AS br + INNER JOIN ( + SELECT u.id AS user_id + FROM %s AS u WHERE u.is_admin + ) AS sa ON 1 = 1 + WHERE br.role = ?` +) + func ProvideService(sql db.DB) *AccessControlStore { return &AccessControlStore{sql} } @@ -62,71 +89,84 @@ func (s *AccessControlStore) SearchUsersPermissions(ctx context.Context, orgID i } dbPerms := make([]UserRBACPermission, 0) + var userID int64 + if options.NamespacedID != "" { + var err error + userID, err = options.ComputeUserID() + if err != nil { + return nil, err + } + } + if err := s.sql.WithDbSession(ctx, func(sess *db.Session) error { roleNameFilterJoin := "" if len(options.RolePrefixes) > 0 { - roleNameFilterJoin = "INNER JOIN role AS r on up.role_id = r.id" + roleNameFilterJoin = "INNER JOIN role AS r ON up.role_id = r.id" + } + + params := []any{} + + direct := userAssignsSQL + if options.NamespacedID != "" { + direct += " WHERE ur.user_id = ?" + params = append(params, userID) + } + + team := teamAssignsSQL + if options.NamespacedID != "" { + team += " WHERE tm.user_id = ?" + params = append(params, userID) + } + + basic := basicRoleAssignsSQL + if options.NamespacedID != "" { + basic += " WHERE ou.user_id = ?" + params = append(params, userID) + } + + grafanaAdmin := fmt.Sprintf(grafanaAdminAssignsSQL, s.sql.Quote("user")) + params = append(params, accesscontrol.RoleGrafanaAdmin) + if options.NamespacedID != "" { + grafanaAdmin += " AND sa.user_id = ?" + params = append(params, userID) } // Find permissions q := ` SELECT user_id, - action, - scope + p.action, + p.scope FROM ( - SELECT ur.user_id, ur.org_id, p.action, p.scope, ur.role_id - FROM permission AS p - INNER JOIN user_role AS ur on ur.role_id = p.role_id + ` + direct + ` UNION ALL - SELECT tm.user_id, tr.org_id, p.action, p.scope, tr.role_id - FROM permission AS p - INNER JOIN team_role AS tr ON tr.role_id = p.role_id - INNER JOIN team_member AS tm ON tm.team_id = tr.team_id + ` + team + ` UNION ALL - SELECT ou.user_id, ou.org_id, p.action, p.scope, br.role_id - FROM permission AS p - INNER JOIN builtin_role AS br ON br.role_id = p.role_id - INNER JOIN org_user AS ou ON ou.role = br.role + ` + basic + ` UNION ALL - SELECT sa.user_id, br.org_id, p.action, p.scope, br.role_id - FROM permission AS p - INNER JOIN builtin_role AS br ON br.role_id = p.role_id - INNER JOIN ( - SELECT u.id AS user_id - FROM ` + s.sql.GetDialect().Quote("user") + ` AS u WHERE u.is_admin - ) AS sa ON 1 = 1 - WHERE br.role = ? + ` + grafanaAdmin + ` ) AS up ` + roleNameFilterJoin + ` + INNER JOIN permission AS p ON up.role_id = p.role_id WHERE (up.org_id = ? OR up.org_id = ?) ` - - params := []any{accesscontrol.RoleGrafanaAdmin, accesscontrol.GlobalOrgID, orgID} + params = append(params, orgID, accesscontrol.GlobalOrgID) if options.ActionPrefix != "" { - q += ` AND action LIKE ?` + q += ` AND p.action LIKE ?` params = append(params, options.ActionPrefix+"%") } if options.Action != "" { - q += ` AND action = ?` + q += ` AND p.action = ?` params = append(params, options.Action) } if options.Scope != "" { // Search for scope and wildcard that include the scope scopes := append(options.Wildcards(), options.Scope) - q += ` AND scope IN ( ? ` + strings.Repeat(", ?", len(scopes)-1) + ")" + q += ` AND p.scope IN ( ? ` + strings.Repeat(", ?", len(scopes)-1) + ")" for i := range scopes { params = append(params, scopes[i]) } } - if options.NamespacedID != "" { - userID, err := options.ComputeUserID() - if err != nil { - return err - } - q += ` AND user_id = ?` - params = append(params, userID) - } if len(options.RolePrefixes) > 0 { q += " AND ( " + strings.Repeat("r.name LIKE ? OR ", len(options.RolePrefixes)-1) q += "r.name LIKE ? )"