Alerting: Update API to use folders' full paths #81214

yuri-tceretian · 2024-01-24T23:46:13Z

What is this feature?
This PR changes Alerting API and Scheduler to refer to a folder using its full path.
This will have the following effect:

Alert list UI will display full paths of folders that contain alert rules. This corresponds with how it displays data source managed rules.
Scheduler will include the full path to label grafana_folder.

Why do we need this feature?
This is needed to properly support nested folders and relaxed constraints that allow folder title to be unique only among folders that share the same parent.

Who is this feature for?
Users of nested folders

Which issue(s) does this PR fix?:

Fixes #80324

Special notes for your reviewer:

Please check that:

It works as expected from a user's perspective.
If this is a pre-GA feature, it is behind a feature toggle.
The docs are updated, and if this is a notable improvement, it's added to our What's New doc.

Release notice breaking change

The following breaking change occurs only when feature flag nestedFolders is enabled.
If the folder title contains the symbol / (forward-slash) the notifications created from the rules that are placed in that folder will contain an escape sequence for that symbol in the label grafana_folder.
For example, the folder title is Grafana / Folder. Currently the label grafana_folder will contain the title as it is. If PR is merged - the label value will be Grafana \/ Folder.
This can break notifications if notification policies have matches that match that label and folder.

pkg/services/ngalert/store/alert_rule.go

use fake user with enough permissions to fetch all folders

ephemeral-instances-bot · 2024-01-31T23:13:46Z

Error: Contact #proj-ephemeral-hg-instances if it is not a compile error

handling pull request comment event: calling gcom to upsert instance: creating instance: unexpected response status: status=409 responseBody={
"code": "Conflict",
"message": "Region is missing required fields: llm_gateway_url",
"requestId": "43c13578-df95-4531-b5f2-e9b4984a7139"
}

pkg/services/ngalert/store/alert_rule.go

yuri-tceretian · 2024-02-01T17:04:59Z

/deploy-to-hg --enterprise-ref yuri-tceretian/fullpaths-folder

ephemeral-instances-bot · 2024-02-01T17:06:52Z

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 yuri-tceretian/fullpaths-folder oss branch and yuri-tceretian/fullpaths-folder enterprise branch. How to choose a branch

ephemeral-instances-bot · 2024-02-01T17:18:55Z

Your instance can be accessed at: https://ephemeral1511182181214yuritce.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

yuri-tceretian · 2024-02-01T20:21:38Z

pkg/services/ngalert/store/alert_rule.go

-	return folder, nil
+	if len(f) == 0 {
+		return nil, dashboards.ErrFolderAccessDenied
+	}


This is to replicate FolderService.Get behavior. @papagian, I wonder if you can fix Get to provide full path too.

sure, I'll do but I would suggest not to block the current work (merge this one and address it in a follow up)

@yuri-tceretian the FolderService.Get() change: #81972

alexweav

Looking very good, a couple question comments that are quite minor. I will defer the folderimpl changes to @papagian

alexweav · 2024-02-01T23:26:18Z

pkg/services/ngalert/models/alert_rule.go

@@ -705,29 +704,3 @@ func GroupByAlertRuleGroupKey(rules []*AlertRule) map[AlertRuleGroupKey]RulesGro
 	}
 	return result
 }
-
-// GetNamespaceKey concatenates two strings with / as separator. If the latter string contains '/' it gets escaped with \/
-func GetNamespaceKey(parentUID, title string) string {


Small reminder to merge the Enterprise change (consuming this function) before or at the same time as this PR 😄

alexweav · 2024-02-01T23:32:24Z

pkg/services/ngalert/api/api.go

@@ -128,6 +128,7 @@ func (api *API) RegisterAPIEndpoints(m *metrics.API) {
 			featureManager:  api.FeatureManager,
 			appUrl:          api.AppUrl,
 			tracer:          api.Tracer,
+			rulesStore:      api.RuleStore,


Nit: RuleStore is a pretty large interface, and we seem to only use it to actually query for folders, not even rules directly. Rather than adding fairly big dependency here, would it be worth declaring a minimal interface containing just GetNamespaceByUID and whatever else is needed?

alexweav · 2024-02-01T23:36:49Z

pkg/services/ngalert/api/api_testing.go

 }

 // RouteTestGrafanaRuleConfig returns a list of potential alerts for a given rule configuration. This is intended to be
 // as true as possible to what would be generated by the ruler except that the resulting alerts are not filtered to
 // only Resolved / Firing and ready to send.
 func (srv TestingApiSrv) RouteTestGrafanaRuleConfig(c *contextmodel.ReqContext, body apimodels.PostableExtendedRuleNodeExtended) response.Response {
+	folder, err := srv.rulesStore.GetNamespaceByUID(c.Req.Context(), body.NamespaceUID, c.OrgID, c.SignedInUser)
+	if err != nil {
+		return toNamespaceErrorResponse(dashboards.ErrFolderAccessDenied)


In the previous version, we purely passed along the UID without validating it.

I think there are contexts where this endpoint may be hit without a valid folder supplied (i.e. you try to test your query and have not yet selected a folder in the UI, the folder selector being below the query).

Does this break such cases, by attempting to look up the folder and failing the test if it's not yet selected? Or, is there a spot later in the flow that does this lookup anyway? We could consider filling the folder with something arbitrary and allowing the test to proceed.

Unfortunately, this becomes mandatory in this API. Otherwise, we risk providing incorrect routing information. Previously we took the folder title from the request and it was enough but UI cannot provide full path in the folder title anymore. So, I had to update it. Anyway, as long as user has access to the rule it has access to the folder it contains.

alexweav · 2024-02-01T23:41:47Z

pkg/services/ngalert/store/alert_rule.go

-					OrgID: f.OrgId,
-					UID:   f.Uid,
+			for orgID, uids := range uids {
+				schedulerUser := accesscontrol.BackgroundUser("grafana_scheduler", orgID, org.RoleAdmin,


Could we extract schedulerUserFor in schedule.go to some shared location and call it from here? And add the folder permissions to it?

That way we don't have an inconsistency if the fake user ever changes, and we have a list of all combined permissions for the rule evaluation in one place.

The problem here is that this is supposed to be a multi-org user (it requires per -org permissions) we can extract it to a function but it won't make any difference because we won't be able to pass it around in scheduler. Another problem is where to put it... perhaps in store or in models package but neither of them seem to be good.

I see. I wonder if we could change UX so user is not guided into the path? Put folder selector next to name selector, above the query or something? @gillesdemey

Clicked the wrong comment box...that was meant for the one about testing API
#81214 (comment)

papagian

LGTM

however I observe some no rule groups found errors but I assume that these are attempts to fetch rule from the different data sources

papagian · 2024-02-02T10:00:14Z

pkg/services/ngalert/store/alert_rule.go

-	return folder, nil
+	if len(f) == 0 {
+		return nil, dashboards.ErrFolderAccessDenied
+	}


sure, I'll do but I would suggest not to block the current work (merge this one and address it in a follow up)

yuri-tceretian · 2024-02-02T18:48:28Z

however I observe some no rule groups found errors but I assume that these are attempts to fetch rule from the different data sources

those are for Lotex APIs. I did not touch them in this PR

yuri-tceretian · 2024-02-06T20:16:56Z

/deploy-to-hg --enterprise-ref yuri-tceretian/fullpaths-folder

yuri-tceretian · 2024-02-06T21:44:11Z

/deploy-to-hg --enterprise-ref yuri-tceretian/fullpaths-folder

ephemeral-instances-bot · 2024-02-06T21:45:05Z

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 yuri-tceretian/fullpaths-folder oss branch and yuri-tceretian/fullpaths-folder enterprise branch. How to choose a branch

ephemeral-instances-bot · 2024-02-06T21:56:56Z

Your instance can be accessed at: https://ephemeral1511182181214yuritce.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

* update GetUserVisibleNamespaces to use FolderSeriver * update GetNamespaceByUID to use FolderService.GetFolders * update GetAlertRulesForScheduling to use FolderService.GetFolders * Update API and GetAlertRulesForScheduling to use the folder's full path * get full path of folder in RouteTestGrafanaRuleConfig * fix escaping of titles for MySQL

grafana-delivery-bot bot added this to the 10.4.x milestone Jan 24, 2024

grafana-pr-automation bot added the area/backend label Jan 24, 2024

yuri-tceretian force-pushed the yuri-tceretian/fullpaths-folder branch from c566960 to da85196 Compare January 25, 2024 19:25

yuri-tceretian changed the title ~~Alerting: Update how API fetches rules available to user~~ Alerting: Update API to use folders' full paths Jan 25, 2024

yuri-tceretian force-pushed the yuri-tceretian/fullpaths-folder branch 2 times, most recently from 6d9b358 to c222d70 Compare January 31, 2024 14:30

yuri-tceretian marked this pull request as ready for review January 31, 2024 15:19

yuri-tceretian requested review from a team, rwwiv, JacobsonMT and grobinson-grafana and removed request for a team January 31, 2024 15:19

yuri-tceretian self-assigned this Jan 31, 2024

yuri-tceretian added the area/alerting Grafana Alerting label Jan 31, 2024

yuri-tceretian added 2 commits January 31, 2024 10:24

update GetUserVisibleNamespaces

b931e3f

update GetNamespaceByUID to use FolderService.GetFolders

9d0466b

papagian reviewed Jan 31, 2024

View reviewed changes

pkg/services/ngalert/store/alert_rule.go Outdated Show resolved Hide resolved

yuri-tceretian force-pushed the yuri-tceretian/fullpaths-folder branch from c222d70 to a148072 Compare January 31, 2024 15:25

yuri-tceretian added add to changelog no-backport Skip backport of PR labels Jan 31, 2024

yuri-tceretian added 4 commits January 31, 2024 11:44

update GetAlertRulesForScheduling to use FolderService.GetFolders

4c0cb10

use fake user with enough permissions to fetch all folders

update api and GetAlertRulesForScheduling to return full path

dbd33cd

get full path of folder in RouteTestGrafanaRuleConfig

3e6d1aa

update tests

5bf3482

yuri-tceretian force-pushed the yuri-tceretian/fullpaths-folder branch from 3040f9b to 5bf3482 Compare January 31, 2024 16:45

fix escaping for mysql

5562f31

yuri-tceretian requested a review from a team as a code owner January 31, 2024 18:10

yuri-tceretian requested review from mildwonkey and undef1nd and removed request for a team January 31, 2024 18:10

yuri-tceretian added the breaking change Relevant for changelog generation label Jan 31, 2024

papagian reviewed Feb 1, 2024

View reviewed changes

pkg/services/ngalert/store/alert_rule.go Outdated Show resolved Hide resolved

use background user

0681013

yuri-tceretian commented Feb 1, 2024

View reviewed changes

alexweav reviewed Feb 1, 2024

View reviewed changes

papagian approved these changes Feb 2, 2024

View reviewed changes

use smaller interface

12d3bd8

alexweav approved these changes Feb 2, 2024

View reviewed changes

yuri-tceretian added 3 commits February 6, 2024 10:07

Merge branch 'up/main' into yuri-tceretian/fullpaths-folder

a40faeb

update SetupFolderService to accept feature manager

80879a3

update integration tests to use feature flag

50c4fe4

grafana deleted a comment from ephemeral-instances-bot bot Feb 6, 2024

Merge branch 'up/main' into yuri-tceretian/fullpaths-folder

41797d5

yuri-tceretian merged commit 47546a4 into main Feb 6, 2024
12 checks passed

yuri-tceretian deleted the yuri-tceretian/fullpaths-folder branch February 6, 2024 22:12

aangelisc modified the milestones: 10.4.x, 10.4.0 Mar 6, 2024

BrewTestBot mentioned this pull request Mar 6, 2024

grafana 10.4.0 Homebrew/homebrew-core#165251

Closed

BrewTestBot mentioned this pull request Mar 14, 2024

grafana 10.4.0 Homebrew/homebrew-core#166070

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Alerting: Update API to use folders' full paths #81214

Alerting: Update API to use folders' full paths #81214

yuri-tceretian commented Jan 24, 2024 •

edited

Loading

ephemeral-instances-bot bot commented Jan 31, 2024

yuri-tceretian commented Feb 1, 2024

ephemeral-instances-bot bot commented Feb 1, 2024

ephemeral-instances-bot bot commented Feb 1, 2024

yuri-tceretian Feb 1, 2024 •

edited

Loading

papagian Feb 2, 2024

papagian Feb 6, 2024

alexweav left a comment •

edited

Loading

alexweav Feb 1, 2024

alexweav Feb 1, 2024

alexweav Feb 1, 2024

yuri-tceretian Feb 2, 2024

alexweav Feb 1, 2024

yuri-tceretian Feb 2, 2024 •

edited

Loading

alexweav Feb 2, 2024

alexweav Feb 2, 2024

papagian left a comment •

edited

Loading

papagian Feb 2, 2024

yuri-tceretian commented Feb 2, 2024 •

edited

Loading

yuri-tceretian commented Feb 6, 2024

yuri-tceretian commented Feb 6, 2024

ephemeral-instances-bot bot commented Feb 6, 2024

ephemeral-instances-bot bot commented Feb 6, 2024

Alerting: Update API to use folders' full paths #81214

Alerting: Update API to use folders' full paths #81214

Conversation

yuri-tceretian commented Jan 24, 2024 • edited Loading

Release notice breaking change

ephemeral-instances-bot bot commented Jan 31, 2024

yuri-tceretian commented Feb 1, 2024

ephemeral-instances-bot bot commented Feb 1, 2024

ephemeral-instances-bot bot commented Feb 1, 2024

yuri-tceretian Feb 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexweav left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuri-tceretian Feb 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

papagian left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuri-tceretian commented Feb 2, 2024 • edited Loading

yuri-tceretian commented Feb 6, 2024

yuri-tceretian commented Feb 6, 2024

ephemeral-instances-bot bot commented Feb 6, 2024

ephemeral-instances-bot bot commented Feb 6, 2024

yuri-tceretian commented Jan 24, 2024 •

edited

Loading

yuri-tceretian Feb 1, 2024 •

edited

Loading

alexweav left a comment •

edited

Loading

yuri-tceretian Feb 2, 2024 •

edited

Loading

papagian left a comment •

edited

Loading

yuri-tceretian commented Feb 2, 2024 •

edited

Loading