RBAC: Split non-empty scopes into `kind`, `attribute` and `identifier` fields for better search performance #71933

IevaVasiljeva · 2023-07-19T11:20:06Z

Part 2 of RBAC search v1 permission filter improvements. First PR: #71913, ultimate PR: https://github.com/grafana/grafana-enterprise/pull/5460

What is this feature?

This PR lays the groundwork for the upcoming search permission filter improvements.

This PR:

adds a feature toggle splitScopes, the new permission filter logic will be behind this feature toggle;
adds fields for the scope parts - kind, attribute and identifier;
adds migrations that introduces new kind, attribute and identifier fields in the DB;
adds a migration that will be ran upon server startup and will set the new fields for any permissions with non-empty scopes that don't have the new fields set.

Why do we need this feature?

It breaks down work from #71844 into smaller chunks.

Co-authored-by: Kalle Persson <kalle.persson@grafana.com>

IevaVasiljeva · 2023-07-19T11:22:17Z

pkg/services/accesscontrol/migrator/migrator.go

+			permissions[i].Attribute = attribute
+			permissions[i].Identifier = identifier
+
+			_, err := sess.Exec("UPDATE permission SET kind = ?, attribute = ?, identifier = ? WHERE id = ?", permissions[i].Kind, permissions[i].Attribute, permissions[i].Identifier, permissions[i].ID)


Thought: instead of doing individual permission updates, we could delete the existing permissions and do a batch insert. It is slightly riskier (as it deletes the existing entries), but it should be faster, especially for large instances with lots of permissions. Let me know if you think it's worth doing.

Good idea :) ~~If you do the insert before the delete it's safer~~

Edit: disregard, we have a unique clause on roleID/action/scope

I wonder if we can do batching or concurrent batching in a transaction 🤔

gamab

I left a couple of comments, I guess the only important one is the missing unique constraint and maybe the feature toggle check not to run the migration until everything is ready.

pkg/services/store/testdata/public_testdata.golden.jsonc

pkg/services/sqlstore/migrations/accesscontrol/migrations.go

gamab · 2023-07-19T12:18:46Z

pkg/services/sqlstore/migrations/accesscontrol/migrations.go

+		Name: "identifier", Type: migrator.DB_NVarchar, Length: 40, Default: "''",
+	}))
+
+	mg.AddMigration("add permission identifier index", migrator.NewAddIndexMigration(permissionV1, &migrator.Index{


I would also include the kind (and attribute), wdyt ? We might have resources of different kind with the same identifier.
It would make the migration lookup for untranslated permissions faster (unless my other comment is accepted 😅)
However, given the action already accounts for the kind in a certain way (ex:dashboards:read), this is not mandatory. This would just be double proofing in the eventuality that we remove the kind from the action.
Another drawback is that the index would be heavier.

I was thinking about it too, but decided to leave it out for now, and then we can test and see if additional indexes make a change to performance and add them later on. Does that sound ok?

pkg/services/accesscontrol/models.go

gamab · 2023-07-19T12:29:14Z

pkg/services/accesscontrol/models_test.go

+
+	tests := []testCase{
+		{desc: "all fields should be empty for empty scope", scope: "", kind: "", attribute: "", identifier: ""},
+		{desc: "only identifier should not be empty for wildcard", scope: "*", kind: "", attribute: "", identifier: "*"},


I cannot wrap my head around: should scope=* -> kind=*, attr=*, id=* 🤔
On one hand, it's simpler the way you did it to merge the scope back together.
On the other hand, if I want to search for permissions targeting a specific kind the where clause would be a bit weird:

SELECT * FROM permission WHERE ( identifier="*" AND kind="" ) OR ( kind="dashboards" )

VS

SELECT * FROM permission WHERE kind="*" OR kind="dashboards"

This is how Kalle had done it, so I stuck with this. I think both approaches make sense. Your suggestion is probably easier to think about, so I'll go with that :)

pkg/services/accesscontrol/acimpl/service.go

pkg/services/sqlstore/migrations/accesscontrol/migrations.go

pkg/services/accesscontrol/migrator/migrator.go

Co-authored-by: Gabriel MABILLE <gamab@users.noreply.github.com>

…ndex, changed how wildcard scopes are split

…h featuremgmt

…his breaks permission queries from db

…` fields for better search performance (#71933) * add a feature toggle * add the fields for attribute, kind and identifier to permission Co-authored-by: Kalle Persson <kalle.persson@grafana.com> * set the new fields when new permissions are stored * add migrations Co-authored-by: Kalle Persson <kalle.persson@grafana.com> * remove comments * Update pkg/services/accesscontrol/migrator/migrator.go Co-authored-by: Gabriel MABILLE <gamab@users.noreply.github.com> * feedback: put column migrations behind the feature toggle, added an index, changed how wildcard scopes are split * PR feedback: add a comment and revert an accidentally changed file * PR feedback: handle the case with : in resource identifier * switch from checking feature toggle through cfg to checking it through featuremgmt * don't put the column migrations behind a feature toggle after all - this breaks permission queries from db --------- Co-authored-by: Kalle Persson <kalle.persson@grafana.com> Co-authored-by: Gabriel MABILLE <gamab@users.noreply.github.com>

IevaVasiljeva and others added 5 commits July 19, 2023 10:58

add a feature toggle

ae80db6

add the fields for attribute, kind and identifier to permission

4d484b2

Co-authored-by: Kalle Persson <kalle.persson@grafana.com>

set the new fields when new permissions are stored

7b1a88e

add migrations

1894abe

Co-authored-by: Kalle Persson <kalle.persson@grafana.com>

remove comments

14b905d

IevaVasiljeva added add to changelog no-backport Skip backport of PR labels Jul 19, 2023

IevaVasiljeva added this to the 10.1.x milestone Jul 19, 2023

IevaVasiljeva requested review from papagian, gamab and Jguer July 19, 2023 11:20

IevaVasiljeva requested review from grafanabot and a team as code owners July 19, 2023 11:20

IevaVasiljeva requested a review from a team July 19, 2023 11:20

IevaVasiljeva requested review from a team as code owners July 19, 2023 11:20

IevaVasiljeva requested review from PoorlyDefinedBehaviour, IbrahimCSAE, rwwiv, JacobsonMT, yuri-tceretian, grobinson-grafana, sakjur and yangkb09 and removed request for a team July 19, 2023 11:20

grafanabot added the area/backend label Jul 19, 2023

grafanabot added area/backend/db/migration area/frontend type/docs labels Jul 19, 2023

IevaVasiljeva commented Jul 19, 2023

View reviewed changes

gamab approved these changes Jul 19, 2023

View reviewed changes

IevaVasiljeva and others added 7 commits July 21, 2023 10:23

Update pkg/services/accesscontrol/migrator/migrator.go

94b0599

Co-authored-by: Gabriel MABILLE <gamab@users.noreply.github.com>

Merge branch 'main' into rbac/search-v1-permission-filter-pt2

8cc4d42

feedback: put column migrations behind the feature toggle, added an i…

e839a65

…ndex, changed how wildcard scopes are split

PR feedback: add a comment and revert an accidentally changed file

0ae4d2c

PR feedback: handle the case with : in resource identifier

4026114

switch from checking feature toggle through cfg to checking it throug…

b403c85

…h featuremgmt

don't put the column migrations behind a feature toggle after all - t…

38080cc

…his breaks permission queries from db

IevaVasiljeva merged commit cfa1a2c into main Jul 21, 2023
17 checks passed

IevaVasiljeva deleted the rbac/search-v1-permission-filter-pt2 branch July 21, 2023 14:23

ricky-undeadcoders modified the milestones: 10.1.x, 10.1.0 Aug 1, 2023

amurray2306 added the postmortem label Aug 23, 2023

chenrui333 mentioned this pull request Aug 24, 2023

grafana 10.1.0 Homebrew/homebrew-core#140321

Merged

zserge mentioned this pull request Nov 24, 2023

Try a different search query #78599

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RBAC: Split non-empty scopes into `kind`, `attribute` and `identifier` fields for better search performance #71933

RBAC: Split non-empty scopes into `kind`, `attribute` and `identifier` fields for better search performance #71933

IevaVasiljeva commented Jul 19, 2023

IevaVasiljeva Jul 19, 2023

gamab Jul 19, 2023 •

edited

Loading

gamab Jul 20, 2023

gamab left a comment •

edited

Loading

gamab Jul 19, 2023

IevaVasiljeva Jul 21, 2023

gamab Jul 19, 2023

IevaVasiljeva Jul 21, 2023

RBAC: Split non-empty scopes into kind, attribute and identifier fields for better search performance #71933

RBAC: Split non-empty scopes into kind, attribute and identifier fields for better search performance #71933

Conversation

IevaVasiljeva commented Jul 19, 2023

IevaVasiljeva Jul 19, 2023

Choose a reason for hiding this comment

gamab Jul 19, 2023 • edited Loading

Choose a reason for hiding this comment

gamab Jul 20, 2023

Choose a reason for hiding this comment

gamab left a comment • edited Loading

Choose a reason for hiding this comment

gamab Jul 19, 2023

Choose a reason for hiding this comment

IevaVasiljeva Jul 21, 2023

Choose a reason for hiding this comment

gamab Jul 19, 2023

Choose a reason for hiding this comment

IevaVasiljeva Jul 21, 2023

Choose a reason for hiding this comment

RBAC: Split non-empty scopes into `kind`, `attribute` and `identifier` fields for better search performance #71933

RBAC: Split non-empty scopes into `kind`, `attribute` and `identifier` fields for better search performance #71933

gamab Jul 19, 2023 •

edited

Loading

gamab left a comment •

edited

Loading