RBAC: Split non-empty scopes into kind, attribute and identifier fields for better search performance
#71933
Conversation
Co-authored-by: Kalle Persson <kalle.persson@grafana.com>
Co-authored-by: Kalle Persson <kalle.persson@grafana.com>
IevaVasiljeva
requested review from
PoorlyDefinedBehaviour,
IbrahimCSAE,
rwwiv,
JacobsonMT,
yuri-tceretian,
grobinson-grafana,
sakjur and
yangkb09
and removed request for
a team
grafanabot
added
area/backend/db/migration
area/frontend
type/docs
| 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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Good idea :) If you do the insert before the delete it's safer
Edit: disregard, we have a unique clause on roleID/action/scope
There was a problem hiding this comment.
I wonder if we can do batching or concurrent batching in a transaction 🤔
| Name: "identifier", Type: migrator.DB_NVarchar, Length: 40, Default: "''", | ||
| })) | ||
|
|
||
| mg.AddMigration("add permission identifier index", migrator.NewAddIndexMigration(permissionV1, &migrator.Index{ |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
|
|
||
| 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: "*"}, |
There was a problem hiding this comment.
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" There was a problem hiding this comment.
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 :)
Co-authored-by: Gabriel MABILLE <gamab@users.noreply.github.com>
…ndex, changed how wildcard scopes are split
…his breaks permission queries from db
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:
splitScopes, the new permission filter logic will be behind this feature toggle;kind,attributeandidentifier;kind,attributeandidentifierfields in the DB;Why do we need this feature?
It breaks down work from #71844 into smaller chunks.