RBAC: Improve performance of dashboard filter query #56813

kalleep · 2022-10-12T15:33:07Z

What this PR does / why we need it:
The old dashboard filter query, used in search, when using RBAC computed all permission on grafana side and provided them to the sql store as parameters.

This does not scale at all, so we need to move the RBAC check to sql.

First we check if user has a wildcard scope for both folders and dashboards, if they do we do not apply any filter. If user has no wildcard we resolve all dashboard and folder permission for a user using a sub query.

I tested this on a mysql database with 20k dashboards.

Which issue(s) this PR fixes:
Fixes https://github.com/grafana/grafana-enterprise/issues/3960

Special notes for your reviewer:

grafanabot · 2022-10-12T15:48:39Z

Drone build failed: https://drone.grafana.net/grafana/grafana-enterprise/37375

grafanabot · 2022-10-12T19:26:19Z

Drone build failed: https://drone.grafana.net/grafana/grafana-enterprise/37406

grafanabot · 2022-10-13T07:52:46Z

Drone build failed: https://drone.grafana.net/grafana/grafana-enterprise/37438

grafanabot · 2022-10-13T08:32:33Z

Drone build failed: https://drone.grafana.net/grafana/grafana-enterprise/37448

grafanabot · 2022-10-13T10:50:39Z

Drone build failed: https://drone.grafana.net/grafana/grafana-enterprise/37486

grafanabot · 2022-10-13T11:21:05Z

Drone build failed: https://drone.grafana.net/grafana/grafana-enterprise/37489

IevaVasiljeva

LGTM, I've added a few nits and a question in comments.

pkg/services/sqlstore/permissions/dashboard.go

IevaVasiljeva · 2022-10-19T13:35:32Z

pkg/services/sqlstore/permissions/dashboard.go

-		builder.WriteString(" OR dashboard.folder_id IN(SELECT id FROM dashboard WHERE ")
-		dashFolderFilter, _ := accesscontrol.Filter(f.User, "dashboard.uid", dashboards.ScopeFoldersPrefix, f.dashboardActions...)
+		if len(actionsToCheck) > 0 {
+			builder.WriteString("(dashboard.uid IN (SELECT substr(scope, 16) FROM permission WHERE action IN (?" + strings.Repeat(", ?", len(actionsToCheck)-1) + ") AND scope LIKE 'dashboards:uid:%' " + rolesFilter + " GROUP BY role_id, scope HAVING COUNT(scope) = ?) AND NOT dashboard.is_folder)")


thought: it might be worth having variables for len("dashboards:uid:") and len("folders:uid:") and use that instead of just using 16 and 13 directly 🤔

grafanabot · 2022-10-19T13:50:10Z

Drone build failed: https://drone.grafana.net/grafana/grafana-enterprise/38313

Co-authored-by: Ieva <ieva.vasiljeva@grafana.com>

grafanabot · 2022-10-19T14:21:45Z

Drone build failed: https://drone.grafana.net/grafana/grafana-enterprise/38316

grafanabot · 2022-10-19T15:10:39Z

Drone build failed: https://drone.grafana.net/grafana/grafana-enterprise/38331

Jguer · 2022-10-19T15:17:45Z

pkg/services/sqlstore/permissions/dashboards_bench_test.go

+	benchmarkDashboardPermissionFilter(b, 100, 1000)
+}
+
+func BenchmarkDashboardPermissionFilter_300_10000(b *testing.B) {


could you post the results from the benchmark on the PR so we have a basis we can come back to?

gamab

Looks good to me!

The only edge case I could think of is, if the actions have been granted through different roles, but that seems highly unlikely given how inconvenient it would be to create roles targeting a specific uid with a specific action.

gamab · 2022-10-21T07:58:54Z

pkg/services/sqlstore/permissions/dashboard.go

-		builder.WriteString(" OR dashboard.folder_id IN(SELECT id FROM dashboard WHERE ")
-		dashFolderFilter, _ := accesscontrol.Filter(f.User, "dashboard.uid", dashboards.ScopeFoldersPrefix, f.dashboardActions...)
+		if len(actionsToCheck) > 0 {
+			builder.WriteString("(dashboard.uid IN (SELECT substr(scope, 16) FROM permission WHERE action IN (?" + strings.Repeat(", ?", len(actionsToCheck)-1) + ") AND scope LIKE 'dashboards:uid:%' " + rolesFilter + " GROUP BY role_id, scope HAVING COUNT(action) = ?) AND NOT dashboard.is_folder)")


Doesn't the GROUP BY role_id, scope HAVING COUNT(action) = ? assumes that the actions have been granted with the same role?
If we assume users will go through managed permissions, I guess that's ok given this is the case.

Edit: How likely is the edge case of actions been granted through different roles? Not that likely.

gamab · 2022-10-21T08:07:48Z

pkg/services/sqlstore/permissions/dashboard.go

+		actionsToCheck := make([]interface{}, 0, len(f.dashboardActions))
+		for _, action := range f.dashboardActions {
+			var hasWildcard bool
+			for _, scope := range f.user.Permissions[f.user.OrgID][action] {
+				if dashWildcards.Contains(scope) || folderWildcards.Contains(scope) {
+					hasWildcard = true
+					break
+				}
+			}
+			if !hasWildcard {
+				actionsToCheck = append(actionsToCheck, action)
+			}
+		}


I would place this in a function to dedup some code and ease reading.

gamab

Tested on postgres, created a few folders (Admin only, Editors only) a few dashboards (Admin only in General, Editors only in general, and in each folders), I granted Admin to a user, Edit to a team, everything seemed to be working as expected.

IevaVasiljeva · 2022-10-21T13:43:19Z

I've tested this with 100 folders, 10 000 dashboards, 100 users and 50 folder permissions and 1000 dashboard permissions per user. Everything seemed to work correctly as far as I could tell. I didn't play around too much with basic role assignments, but I think that should have a minimal impact performance wise.

Note that running the main branch at this scale also didn't lead to any noticeable performance degradation, so I'm relying on the bench marks for showing the performance improvement of these changes.

kalleep · 2022-10-21T14:33:29Z

Note that running the main branch at this scale also didn't lead to any noticeable performance degradation, so I'm relying on the bench marks for showing the performance improvement of these changes.

@IevaVasiljeva When using sqlite the current implementation is fine. The performance issues is when using either mysql or postgres

kalleep added add to changelog area/auth/rbac Grafana role-based access control backport v9.1.x Bot will automatically open backport PR backport v9.2.x Mark PR for automatic backport to v9.2.x labels Oct 12, 2022

kalleep added this to the 9.1.9 milestone Oct 12, 2022

kalleep self-assigned this Oct 12, 2022

kalleep requested review from a team as code owners October 12, 2022 15:33

kalleep requested review from vtorosyan, IevaVasiljeva, sh0rez, yangkb09, ying-jeanne, Jguer and gamab and removed request for a team October 12, 2022 15:33

grafanabot added the area/backend label Oct 12, 2022

grafanabot added the enterprise-failed label Oct 12, 2022

kalleep force-pushed the kalleep/rbac/dashboard-permission-search-query branch from 1c34734 to 40f90e2 Compare October 13, 2022 07:58

grafanabot added enterprise-ok enterprise-failed and removed enterprise-failed enterprise-ok labels Oct 13, 2022

kalleep added 3 commits October 19, 2022 15:43

Remove bench file

7f328ad

RBAC: Add benchmarks

cd425e9

RBAC: Add benchmarks

d04c4e8

kalleep force-pushed the kalleep/rbac/dashboard-permission-search-query branch from 42707d2 to d04c4e8 Compare October 19, 2022 13:43

IevaVasiljeva approved these changes Oct 19, 2022

View reviewed changes

grafanabot added enterprise-failed and removed enterprise-ok labels Oct 19, 2022

kalleep and others added 2 commits October 19, 2022 16:03

Update pkg/services/sqlstore/permissions/dashboard.go

9b8e446

Co-authored-by: Ieva <ieva.vasiljeva@grafana.com>

Fix import

dbad0a7

RBAC: Change to counting actions

290b335

Jguer reviewed Oct 19, 2022

View reviewed changes

Jguer approved these changes Oct 20, 2022

View reviewed changes

gamab approved these changes Oct 21, 2022

View reviewed changes

gamab reviewed Oct 21, 2022

View reviewed changes

kalleep modified the milestones: 9.2.2, 9.3.0 Oct 24, 2022

grafanabot added enterprise-ok and removed enterprise-failed labels Oct 24, 2022

kalleep added no-backport Skip backport of PR and removed backport v9.2.x Mark PR for automatic backport to v9.2.x labels Oct 25, 2022

kalleep merged commit 7386f86 into main Oct 25, 2022

kalleep deleted the kalleep/rbac/dashboard-permission-search-query branch October 25, 2022 09:14

leandro-deveikis modified the milestones: 9.3.0, 9.3.0-beta1 Nov 14, 2022

frankyi-gh mentioned this pull request Nov 23, 2022

Dashboard search performance regressed after upgraded to v9 #55332

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RBAC: Improve performance of dashboard filter query #56813

RBAC: Improve performance of dashboard filter query #56813

kalleep commented Oct 12, 2022 •

edited

grafanabot commented Oct 12, 2022

grafanabot commented Oct 12, 2022

grafanabot commented Oct 13, 2022

grafanabot commented Oct 13, 2022

grafanabot commented Oct 13, 2022

grafanabot commented Oct 13, 2022

IevaVasiljeva left a comment

IevaVasiljeva Oct 19, 2022

grafanabot commented Oct 19, 2022

grafanabot commented Oct 19, 2022

grafanabot commented Oct 19, 2022

Jguer Oct 19, 2022

gamab left a comment

gamab Oct 21, 2022

gamab Oct 21, 2022

gamab left a comment

IevaVasiljeva commented Oct 21, 2022

kalleep commented Oct 21, 2022

RBAC: Improve performance of dashboard filter query #56813

RBAC: Improve performance of dashboard filter query #56813

Conversation

kalleep commented Oct 12, 2022 • edited

grafanabot commented Oct 12, 2022

grafanabot commented Oct 12, 2022

grafanabot commented Oct 13, 2022

grafanabot commented Oct 13, 2022

grafanabot commented Oct 13, 2022

grafanabot commented Oct 13, 2022

IevaVasiljeva left a comment

Choose a reason for hiding this comment

IevaVasiljeva Oct 19, 2022

Choose a reason for hiding this comment

grafanabot commented Oct 19, 2022

grafanabot commented Oct 19, 2022

grafanabot commented Oct 19, 2022

Jguer Oct 19, 2022

Choose a reason for hiding this comment

gamab left a comment

Choose a reason for hiding this comment

gamab Oct 21, 2022

Choose a reason for hiding this comment

gamab Oct 21, 2022

Choose a reason for hiding this comment

gamab left a comment

Choose a reason for hiding this comment

IevaVasiljeva commented Oct 21, 2022

kalleep commented Oct 21, 2022

kalleep commented Oct 12, 2022 •

edited