Alerting: Handle custom dashboard permissions in migration service #74504

JacobsonMT · 2023-09-07T05:19:53Z

Followup to #72702 and #74503.

Summary

This PR replaces the obsolete and broken code that checks for custom dashboard ACLs for creating new folders during migration. The new code now uses the new RBAC permissions models / logic.

The main focus of this PR is the new permissions logic, however there are also other changes that were necessary or prudent to carry-out at the same time. I'll list them first before detailing the main logic.

Fixes: #71529, #67448

Other Changes

Included in this PR is a refactoring of the main migration Exec method, heavily simplifying the logic. Enough changes were needed to incorporate the new permissions logic that it made sense to simplify the rest at the same time. This was moved to Alerting: Move legacy alert migration from sqlstore migration to service #72702
~~uidSet has been renamed and slightly reworked into a more generic deduplicator helper struct that is used to deduplicate both the UIDs the previous version did as well as alert rule titles.~~ This was moved to Alerting: Move legacy alert migration from sqlstore migration to service #72702
Some general reorganising of code.
Adds extensive testing for permission migrations.

Permissions Migration

Background

In legacy alerting, dashboard alert permissions were determined by both their dashboard and folder scoped permissions. This was done by taking the greater permission defined between the two for each mapped OrgRole, User, or Team.

Now, alert rules only have folder scoped permissions. This can lead to a situation where simply migrating a legacy alert from panelA in dashboardA in folderA to a UA alert rule in folderA will cause the newly created alert rule to be viewable/editable/adminable by different users than in legacy.

To prevent this, we must make a determination of when this situation will occur, and instead migrate the legacy alert into a newly created folder that will ensure the same mapped permissions (or as close to as possible).

Logic

To determine when a migrated alert rule will have different mapped permissions, we calculate two sets of permissions:

The set of mapped permissions of the dashboard (including those inherited from the parent folder).
The set of mapped permissions of the parent folder.

These permissions include mappings of:

OrgRoles -> Viewer/Editor/Admin
Teams -> Viewer/Editor/Admin
Users -> Viewer/Editor/Admin

When the dashboard permissions differ from the parent folder permissions we need to create a new folder to represent the equivalent access-level of the legacy dashboard. To reduce folder clutter, we create one such new folder per set of unique permissions in a parent folder, instead of one per dashboard with differing permissions.

Permission Calculation

Permissions for a dashboard or folder are calculated using the accesscontrol.ResourcePermissions assigned to the resource. These ResourcePermissions are determined by roles with actions that are scoped to the dashboard or folder (or wildcard). For example, this can be managed:builtins:editor:permissions -> [folders:read, dashboards:write, ...] or managed:users:1:permissions -> [...] or fixed:dashboards:writer -> [...].

We iterate over each of these ResourcePermission and map the set of actions (dashboardPermissions.MapActions) to its equivalent dashboards.PermissionType of either PERMISSION_VIEW, PERMISSION_EDIT, or PERMISSION_ADMIN. When we have multiple sources of permission level for an OrgRole, User, or Team, we take the highest one.

Included in the calculation are:

managed roles (ex. managed:users:1:permissions, managed:builtins:editor:permissions, managed:teams:1:permissions)
basic roles (ex. basic:admin, basic:editor)
and for dashboards, inherited roles from the parent folder.

Currently, we do not attempt to include fixed or custom roles in the calculation but do attempt to warn the user during migration if one of these roles had the potential to override the folder permissions. Note that these overrides would always be to increase permissions not decrease them, so the risk of giving users access to alerts they didn't have access to before is mitigated.

Also, we don't attempt to reconcile any missing datasource permissions. Users without access to the necessary datasources to read/edit an alert rule will need to obtain said access separate from the migration.

JacobsonMT · 2023-09-11T17:15:47Z

Updated main and rebased

yuri-tceretian

LGTM

yuri-tceretian · 2023-10-11T13:20:48Z

pkg/services/ngalert/migration/alert_rule.go

 		PanelID:         &da.PanelID,
-		RuleGroup:       name,
+		RuleGroup:       fmt.Sprintf("%s - %d", info.DashboardName, da.PanelID), // Unique to this dash alert but still contains useful info.


I think this is not related to this PR scope. Probably makes sense to address it in a follow-up PR because RuleGroup length is limited and we need to check that the concatenated name does not exceed it, and if it exceeds - the trimmed version is still unique.

yuri-tceretian · 2023-10-11T13:24:23Z

pkg/services/ngalert/migration/permissions.go

+	if err != nil {
+		return nil, err
+	}
+	l := log.New("dashboardTitle", dash.Title, "dashboardUID", dash.UID)


Please make sure the context labels follow the guidelines. In this case it should be dashboardUid. It can be addressed it later.

yuri-tceretian · 2023-10-11T13:27:42Z

pkg/services/ngalert/migration/permissions.go

+// Any dashboard that has greater read/write permissions for an orgRole/team/user compared to its folder will necessitate creating a new folder with the same permissions as the dashboard.
+func (om *OrgMigration) getOrCreateMigratedFolder(ctx context.Context, l log.Logger, dash *dashboards.Dashboard, parentFolder *folder.Folder) (*folder.Folder, error) {
+	// If parentFolder does not exist then the dashboard is an orphan. We migrate the alert to the general alerting folder.
+	if parentFolder == nil {


let's mention here in the comment that the "General Alerting" folder is available to only admins.

yuri-tceretian · 2023-10-11T13:33:21Z

pkg/services/ngalert/migration/permissions.go

+
+	// Check if the dashboard has custom permissions. If it does, we need to create a new folder for it.
+	// This folder will be cached for re-use for each dashboard in the folder with the same permissions.
+	customFolders, ok := om.permissionsMap[parentFolder.ID]


I think name "customFolders" is misleading here. It reflects the permissions of the parent folder and dashboards in this folder.

rwwiv

LGTM

Dashboard alert permissions were determined by both its dashboard and folder scoped permissions, while UA alert rules only have folder scoped permissions. This means, when migrating an alert, we'll need to decide if the parent folder is a correct location for the newly created alert rule so that users, teams, and org roles have the same access to it as they did in legacy. To do this, we translate both the folder and dashboard resource permissions to two sets of SetResourcePermissionCommands. Each of these encapsulates a mapping of all: OrgRoles -> Viewer/Editor/Admin Teams -> Viewer/Editor/Admin Users -> Viewer/Editor/Admin When the dashboard permissions (including those inherited from the parent folder) differ from the parent folder permissions alone, we need to create a new folder to represent the access-level of the legacy dashboard. Compromises: When determining the SetResourcePermissionCommands we only take into account managed and basic roles. Fixed and custom roles introduce significant complexity and synchronicity hurdles. Instead, we log a warning they had the potential to override the newly created folder permissions. Also, we don't attempt to reconcile datasource permissions that were not necessary in legacy alerting. Users without access to the necessary datasources to edit an alert rule will need to obtain said access separate from the migration.

JacobsonMT · 2023-10-12T14:13:27Z

/deploy-to-hg

ephemeral-instances-bot · 2023-10-12T14:14:10Z

Preparing your instance. A comment containing your instance's url will be added to this PR when the instance is ready.
Your instance will be ready in ~10 minutes.
Check the GitHub actions tab to follow the workflow progress
Slack channel: #proj-ephemeral-hg-instances
Building instance with jacobsonmt/migration_fix_permissions oss branch and main enterprise branch. How to choose a branch

ephemeral-instances-bot · 2023-10-12T14:25:18Z

Your instance can be accessed at: https://ephemeral1511182174504jacobso.grafana-dev.net
The instance is not using the CDN assets.
How to access / How to update instance config / How to build a specific branch

…74504) * Fix migration of custom dashboard permissions Dashboard alert permissions were determined by both its dashboard and folder scoped permissions, while UA alert rules only have folder scoped permissions. This means, when migrating an alert, we'll need to decide if the parent folder is a correct location for the newly created alert rule so that users, teams, and org roles have the same access to it as they did in legacy. To do this, we translate both the folder and dashboard resource permissions to two sets of SetResourcePermissionCommands. Each of these encapsulates a mapping of all: OrgRoles -> Viewer/Editor/Admin Teams -> Viewer/Editor/Admin Users -> Viewer/Editor/Admin When the dashboard permissions (including those inherited from the parent folder) differ from the parent folder permissions alone, we need to create a new folder to represent the access-level of the legacy dashboard. Compromises: When determining the SetResourcePermissionCommands we only take into account managed and basic roles. Fixed and custom roles introduce significant complexity and synchronicity hurdles. Instead, we log a warning they had the potential to override the newly created folder permissions. Also, we don't attempt to reconcile datasource permissions that were not necessary in legacy alerting. Users without access to the necessary datasources to edit an alert rule will need to obtain said access separate from the migration.

JacobsonMT added area/alerting Grafana Alerting area/backend add to changelog no-backport Skip backport of PR labels Sep 7, 2023

JacobsonMT requested a review from yuri-tceretian September 7, 2023 05:19

JacobsonMT requested a review from a team as a code owner September 7, 2023 05:19

JacobsonMT requested review from rwwiv and grobinson-grafana and removed request for a team September 7, 2023 05:19

grafana-delivery-bot bot added this to the 10.2.x milestone Sep 7, 2023

JacobsonMT force-pushed the jacobsonmt/migration_fix_permissions branch from 3f90c83 to 72d25b2 Compare September 8, 2023 14:04

JacobsonMT force-pushed the jacobsonmt/migration_remove_vendored branch from 84509f2 to ba3d80f Compare September 11, 2023 17:14

JacobsonMT requested review from a team as code owners September 11, 2023 17:14

JacobsonMT requested review from mildwonkey, suntala and yangkb09 and removed request for a team September 11, 2023 17:14

JacobsonMT force-pushed the jacobsonmt/migration_fix_permissions branch from 72d25b2 to 8a91edd Compare September 11, 2023 17:15

JacobsonMT force-pushed the jacobsonmt/migration_fix_permissions branch from 8a91edd to 4a4eb61 Compare September 11, 2023 22:55

grafana-pr-automation bot added the area/backend/db/migration label Sep 11, 2023

JacobsonMT mentioned this pull request Oct 5, 2023

Alerting: Guided legacy alerting upgrade dry-run #76074

Closed

JacobsonMT force-pushed the jacobsonmt/migration_remove_vendored branch from 93473b8 to 3b23991 Compare October 6, 2023 13:45

JacobsonMT force-pushed the jacobsonmt/migration_fix_permissions branch 2 times, most recently from 057bf9e to dbc6381 Compare October 8, 2023 19:17

yuri-tceretian approved these changes Oct 11, 2023

View reviewed changes

rwwiv approved these changes Oct 11, 2023

View reviewed changes

Base automatically changed from jacobsonmt/migration_remove_vendored to main October 11, 2023 16:22

JacobsonMT force-pushed the jacobsonmt/migration_fix_permissions branch 2 times, most recently from 3994d4e to e56e6e7 Compare October 11, 2023 16:29

JacobsonMT mentioned this pull request Oct 12, 2023

Alerting: Prevent cleanup of non-empty folders on migration revert #76439

Merged

3 tasks

JacobsonMT added 5 commits October 12, 2023 14:05

Improve naming of some permission map variable

a006824

Truncate and deduplicate migrated rule group

d7143de

Fix UID->Uid in log context and add comments

ff5fb02

Fix test from rule group dedup

dc6b396

JacobsonMT force-pushed the jacobsonmt/migration_fix_permissions branch from e56e6e7 to dc6b396 Compare October 12, 2023 13:05

Fix linting

203f667

JacobsonMT added the type/bug label Oct 12, 2023

JacobsonMT merged commit 5f48619 into main Oct 12, 2023
19 checks passed

JacobsonMT deleted the jacobsonmt/migration_fix_permissions branch October 12, 2023 22:12

JacobsonMT mentioned this pull request Oct 12, 2023

Alerting: Migration to Unified Alerting with RBAC enabled does not assign correct permissions to alerts #67448

Closed

zerok modified the milestones: 10.2.x, 10.2.0 Oct 23, 2023

dnhn mentioned this pull request Oct 24, 2023

grafana 10.2.0 Homebrew/homebrew-core#152264

Closed

BrewTestBot mentioned this pull request Oct 25, 2023

grafana 10.2.0 Homebrew/homebrew-core#152321

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Alerting: Handle custom dashboard permissions in migration service #74504

Alerting: Handle custom dashboard permissions in migration service #74504

JacobsonMT commented Sep 7, 2023 •

edited

JacobsonMT commented Sep 11, 2023

yuri-tceretian left a comment

yuri-tceretian Oct 11, 2023

yuri-tceretian Oct 11, 2023

yuri-tceretian Oct 11, 2023

yuri-tceretian Oct 11, 2023

rwwiv left a comment

JacobsonMT commented Oct 12, 2023

ephemeral-instances-bot bot commented Oct 12, 2023

ephemeral-instances-bot bot commented Oct 12, 2023

Alerting: Handle custom dashboard permissions in migration service #74504

Alerting: Handle custom dashboard permissions in migration service #74504

Conversation

JacobsonMT commented Sep 7, 2023 • edited

Summary

Other Changes

Permissions Migration

Background

Logic

Permission Calculation

JacobsonMT commented Sep 11, 2023

yuri-tceretian left a comment

Choose a reason for hiding this comment

yuri-tceretian Oct 11, 2023

Choose a reason for hiding this comment

yuri-tceretian Oct 11, 2023

Choose a reason for hiding this comment

yuri-tceretian Oct 11, 2023

Choose a reason for hiding this comment

yuri-tceretian Oct 11, 2023

Choose a reason for hiding this comment

rwwiv left a comment

Choose a reason for hiding this comment

JacobsonMT commented Oct 12, 2023

ephemeral-instances-bot bot commented Oct 12, 2023

ephemeral-instances-bot bot commented Oct 12, 2023

JacobsonMT commented Sep 7, 2023 •

edited