From ae2074ef55d20954f32b1662fe1778d833c6d332 Mon Sep 17 00:00:00 2001 From: Alexander Akhmetov Date: Fri, 28 Feb 2025 12:11:49 +0100 Subject: [PATCH] Alerting: Fix updating Prometheus definition in the metadata (#101440) Initially, Metadata had only the EditorSettings, and HasMetadata was used to understand if the incoming update request had metadata in the body because it could be omitted if it was empty. For example, when the rule is updated via the provisioning API or has only false values. If it was in the request, we used that; if not, we used the metadata from the existing rule from the database. If the rule was updated via the AlertRuleService, we didn't change Metadata at all if the rule already existed. But now, Metadata also has the Prometheus rule definition, and we always need to update it with the new version of the AlertRuleService when the rule exists in the DB and has the same UID. HasMetadata is renamed to HasEditorSettings to keep the old behaviour only for EditorSettings. Now, the provisioning API and the conversion API will overwrite everything except EditorSettings with the new data. --- .../ngalert/api/api_ruler_validation.go | 6 +- pkg/services/ngalert/models/alert_rule.go | 19 ++- .../ngalert/models/alert_rule_test.go | 2 +- .../ngalert/provisioning/alert_rules_test.go | 125 ++++++++++++++++++ pkg/services/ngalert/store/deltas_test.go | 4 +- .../alerting/api_convert_prometheus_test.go | 88 ++++++++++++ 6 files changed, 227 insertions(+), 17 deletions(-) diff --git a/pkg/services/ngalert/api/api_ruler_validation.go b/pkg/services/ngalert/api/api_ruler_validation.go index 1ed855378fd..672873fa040 100644 --- a/pkg/services/ngalert/api/api_ruler_validation.go +++ b/pkg/services/ngalert/api/api_ruler_validation.go @@ -314,7 +314,7 @@ func ValidateRuleGroup( uids[rule.UID] = idx } - var hasPause, isPaused, hasMetadata bool + var hasPause, isPaused, hasEditorSettings bool original := ruleGroupConfig.Rules[idx] if alert := original.GrafanaManagedAlert; alert != nil { if alert.IsPaused != nil { @@ -322,7 +322,7 @@ func ValidateRuleGroup( hasPause = true } if alert.Metadata != nil { - hasMetadata = true + hasEditorSettings = true } } @@ -331,7 +331,7 @@ func ValidateRuleGroup( rule.RuleGroupIndex = idx + 1 ruleWithOptionals.AlertRule = *rule ruleWithOptionals.HasPause = hasPause - ruleWithOptionals.HasMetadata = hasMetadata + ruleWithOptionals.HasEditorSettings = hasEditorSettings result = append(result, &ruleWithOptionals) } diff --git a/pkg/services/ngalert/models/alert_rule.go b/pkg/services/ngalert/models/alert_rule.go index bef50b1b4e7..cdbc2a35b20 100644 --- a/pkg/services/ngalert/models/alert_rule.go +++ b/pkg/services/ngalert/models/alert_rule.go @@ -327,8 +327,8 @@ type AlertRuleWithOptionals struct { AlertRule // This parameter is to know if an optional API field was sent and, therefore, patch it with the current field from // DB in case it was not sent. - HasPause bool - HasMetadata bool + HasPause bool + HasEditorSettings bool } // AlertsRulesBy is a function that defines the ordering of alert rules. @@ -902,9 +902,10 @@ func (c Condition) IsValid() bool { // PatchPartialAlertRule patches `ruleToPatch` by `existingRule` following the rule that if a field of `ruleToPatch` is empty or has the default value, it is populated by the value of the corresponding field from `existingRule`. // There are several exceptions: -// 1. Following fields are not patched and therefore will be ignored: AlertRule.ID, AlertRule.OrgID, AlertRule.Updated, AlertRule.Version, AlertRule.UID, AlertRule.DashboardUID, AlertRule.PanelID, AlertRule.Annotations and AlertRule.Labels -// 2. There are fields that are patched together: -// - AlertRule.Condition and AlertRule.Data +// 1. Following fields are not patched and therefore will be ignored: AlertRule.ID, AlertRule.OrgID, AlertRule.Updated, AlertRule.Version, +// AlertRule.UID, AlertRule.DashboardUID, AlertRule.PanelID, AlertRule.Annotations, AlertRule.Labels, AlertRule.Metadata (except for EditorSettings) +// 2. There are fields that are patched together: +// - AlertRule.Condition and AlertRule.Data // // If either of the pair is specified, neither is patched. func PatchPartialAlertRule(existingRule *AlertRule, ruleToPatch *AlertRuleWithOptionals) { @@ -936,12 +937,8 @@ func PatchPartialAlertRule(existingRule *AlertRule, ruleToPatch *AlertRuleWithOp if !ruleToPatch.HasPause { ruleToPatch.IsPaused = existingRule.IsPaused } - - // Currently metadata contains only editor settings, so we can just copy it. - // If we add more fields to metadata, we might need to handle them separately, - // and/or merge or update their values. - if !ruleToPatch.HasMetadata { - ruleToPatch.Metadata = existingRule.Metadata + if !ruleToPatch.HasEditorSettings { + ruleToPatch.Metadata.EditorSettings = existingRule.Metadata.EditorSettings } } diff --git a/pkg/services/ngalert/models/alert_rule_test.go b/pkg/services/ngalert/models/alert_rule_test.go index 9f6c8b52b96..84cbed48175 100644 --- a/pkg/services/ngalert/models/alert_rule_test.go +++ b/pkg/services/ngalert/models/alert_rule_test.go @@ -263,7 +263,7 @@ func TestPatchPartialAlertRule(t *testing.T) { name: "No metadata", mutator: func(r *AlertRuleWithOptionals) { r.Metadata = AlertRuleMetadata{} - r.HasMetadata = false + r.HasEditorSettings = false }, }, } diff --git a/pkg/services/ngalert/provisioning/alert_rules_test.go b/pkg/services/ngalert/provisioning/alert_rules_test.go index c269a92c664..491d084c096 100644 --- a/pkg/services/ngalert/provisioning/alert_rules_test.go +++ b/pkg/services/ngalert/provisioning/alert_rules_test.go @@ -229,6 +229,97 @@ func TestAlertRuleService(t *testing.T) { require.Equal(t, ruleMetadata, readGroup.Rules[0].Metadata) }) + t.Run("updating a group with editor settings should override its prometheus rule definition", func(t *testing.T) { + namespaceUID := "my-namespace" + groupTitle := "test-group-123" + + // create the rule group via the rule store, to persist the editor settings + rule := createTestRule(util.GenerateShortUID(), groupTitle, orgID, namespaceUID) + ruleMetadata := models.AlertRuleMetadata{ + EditorSettings: models.EditorSettings{ + SimplifiedQueryAndExpressionsSection: true, + }, + PrometheusStyleRule: &models.PrometheusStyleRule{ + OriginalRuleDefinition: "old", + }, + } + rule.Metadata = ruleMetadata + r, err := ruleService.ruleStore.InsertAlertRules(context.Background(), models.NewUserUID(u), []models.AlertRule{rule}) + require.NoError(t, err) + require.Len(t, r, 1) + + // Set the UID for the rule to update it + rule.UID = r[0].UID + // clear the editor settings in the metadata to check that the existing setting is not overridden + rule.Metadata = models.AlertRuleMetadata{ + PrometheusStyleRule: &models.PrometheusStyleRule{ + OriginalRuleDefinition: "new", + }, + } + + // Now update the rule group with the rule to update its metadata + group := models.AlertRuleGroup{ + Title: groupTitle, + Interval: 60, + FolderUID: namespaceUID, + Rules: []models.AlertRule{rule}, + } + + err = ruleService.ReplaceRuleGroup(context.Background(), u, group, models.ProvenanceAPI) + require.NoError(t, err) + + readGroup, err := ruleService.GetRuleGroup(context.Background(), u, namespaceUID, groupTitle) + require.NoError(t, err) + require.NotEmpty(t, readGroup.Rules) + require.Len(t, readGroup.Rules, 1) + + // check that the editor settings are still there + require.True(t, readGroup.Rules[0].Metadata.EditorSettings.SimplifiedQueryAndExpressionsSection) + // check the new prometheus rule definition + require.Equal(t, "new", readGroup.Rules[0].Metadata.PrometheusStyleRule.OriginalRuleDefinition) + }) + + t.Run("updating a group should override its prometheus rule definition", func(t *testing.T) { + namespaceUID := "my-namespace" + groupTitle := "test-group-123" + + // create the rule group via the rule store, to persist the editor settings + rule := createTestRule(util.GenerateShortUID(), groupTitle, orgID, namespaceUID) + ruleMetadata := models.AlertRuleMetadata{ + PrometheusStyleRule: &models.PrometheusStyleRule{ + OriginalRuleDefinition: "old", + }, + } + rule.Metadata = ruleMetadata + r, err := ruleService.ruleStore.InsertAlertRules(context.Background(), models.NewUserUID(u), []models.AlertRule{rule}) + require.NoError(t, err) + require.Len(t, r, 1) + + // Set the UID for the rule to update it + rule.UID = r[0].UID + // make the metadata empty + rule.Metadata = models.AlertRuleMetadata{} + + // Now update the rule group with the rule to update its metadata + group := models.AlertRuleGroup{ + Title: groupTitle, + Interval: 60, + FolderUID: namespaceUID, + Rules: []models.AlertRule{rule}, + } + + err = ruleService.ReplaceRuleGroup(context.Background(), u, group, models.ProvenanceAPI) + require.NoError(t, err) + + readGroup, err := ruleService.GetRuleGroup(context.Background(), u, namespaceUID, groupTitle) + require.NoError(t, err) + require.NotEmpty(t, readGroup.Rules) + require.Len(t, readGroup.Rules, 1) + + // check that the prometheus rule definition is empty + require.Nil(t, readGroup.Rules[0].Metadata.PrometheusStyleRule) + }) + t.Run("updating a rule should not override its editor settings", func(t *testing.T) { rule := createTestRule(util.GenerateShortUID(), "my-group", orgID, "my-folder") ruleMetadata := models.AlertRuleMetadata{ @@ -1508,6 +1599,40 @@ func TestReplaceGroup(t *testing.T) { require.Len(t, updates, 1) }) }) + + t.Run("alert rule metadata should be updated correctly", func(t *testing.T) { + service, _, _, _ := initServiceWithData(t) + + rule := dummyRule("test#3", orgID) + // the rule must have a UID to be updated, otherwise it will be created as new + // and the previous version will be deleted + rule.UID = util.GenerateShortUID() + rule.Metadata = models.AlertRuleMetadata{ + EditorSettings: models.EditorSettings{ + SimplifiedQueryAndExpressionsSection: true, + }, + PrometheusStyleRule: &models.PrometheusStyleRule{ + OriginalRuleDefinition: "old", + }, + } + group := models.AlertRuleGroup{ + Title: rule.RuleGroup, + Interval: rule.IntervalSeconds, + FolderUID: rule.NamespaceUID, + Rules: []models.AlertRule{rule}, + } + + err := service.ReplaceRuleGroup(context.Background(), u, group, models.ProvenanceNone) + require.NoError(t, err) + + rule.Metadata.PrometheusStyleRule.OriginalRuleDefinition = "new" + err = service.ReplaceRuleGroup(context.Background(), u, group, models.ProvenanceNone) + require.NoError(t, err) + + rule, _, err = service.GetAlertRule(context.Background(), u, rule.UID) + require.NoError(t, err) + require.Equal(t, "new", rule.Metadata.PrometheusStyleRule.OriginalRuleDefinition) + }) } func TestDeleteRuleGroup(t *testing.T) { diff --git a/pkg/services/ngalert/store/deltas_test.go b/pkg/services/ngalert/store/deltas_test.go index fc27fbe9af2..7381df4397a 100644 --- a/pkg/services/ngalert/store/deltas_test.go +++ b/pkg/services/ngalert/store/deltas_test.go @@ -81,7 +81,7 @@ func TestCalculateChanges(t *testing.T) { submittedMap := groupByUID(t, rules) submitted := make([]*models.AlertRuleWithOptionals, 0, len(rules)) for _, rule := range rules { - submitted = append(submitted, &models.AlertRuleWithOptionals{AlertRule: *rule, HasMetadata: true}) + submitted = append(submitted, &models.AlertRuleWithOptionals{AlertRule: *rule, HasEditorSettings: true}) } fakeStore := fakes.NewRuleStore(t) @@ -216,7 +216,7 @@ func TestCalculateChanges(t *testing.T) { submittedMap := groupByUID(t, rules) submitted := make([]*models.AlertRuleWithOptionals, 0, len(rules)) for _, rule := range rules { - submitted = append(submitted, &models.AlertRuleWithOptionals{AlertRule: *rule, HasMetadata: true}) + submitted = append(submitted, &models.AlertRuleWithOptionals{AlertRule: *rule, HasEditorSettings: true}) } changes, err := CalculateChanges(context.Background(), fakeStore, groupKey, submitted) diff --git a/pkg/tests/api/alerting/api_convert_prometheus_test.go b/pkg/tests/api/alerting/api_convert_prometheus_test.go index cb248fba7f3..780e11a1d15 100644 --- a/pkg/tests/api/alerting/api_convert_prometheus_test.go +++ b/pkg/tests/api/alerting/api_convert_prometheus_test.go @@ -213,6 +213,94 @@ func TestIntegrationConvertPrometheusEndpoints(t *testing.T) { }) } +func TestIntegrationConvertPrometheusEndpoints_UpdateRule(t *testing.T) { + runTest := func(t *testing.T, enableLokiPaths bool) { + testinfra.SQLiteIntegrationTest(t) + + // Setup Grafana and its Database + dir, gpath := testinfra.CreateGrafDir(t, testinfra.GrafanaOpts{ + DisableLegacyAlerting: true, + EnableUnifiedAlerting: true, + DisableAnonymous: true, + AppModeProduction: true, + EnableFeatureToggles: []string{"alertingConversionAPI"}, + }) + + grafanaListedAddr, env := testinfra.StartGrafanaEnv(t, dir, gpath) + + // Create a user to make authenticated requests + createUser(t, env.SQLStore, env.Cfg, user.CreateUserCommand{ + DefaultOrgRole: string(org.RoleAdmin), + Password: "password", + Login: "admin", + }) + apiClient := newAlertingApiClient(grafanaListedAddr, "admin", "password") + apiClient.prometheusConversionUseLokiPaths = enableLokiPaths + + createUser(t, env.SQLStore, env.Cfg, user.CreateUserCommand{ + DefaultOrgRole: string(org.RoleViewer), + Password: "password", + Login: "viewer", + }) + + namespace1 := "test-namespace-1" + + ds := apiClient.CreateDatasource(t, datasources.DS_PROMETHEUS) + + promGroup := apimodels.PrometheusRuleGroup{ + Name: "test-group-for-an-update", + Interval: prommodel.Duration(60 * time.Second), + Rules: []apimodels.PrometheusRule{ + { + Alert: "HighDiskUsage", + Expr: "disk_usage > 80", + For: util.Pointer(prommodel.Duration(1 * time.Minute)), + Labels: map[string]string{ + "severity": "low", + "team": "alerting", + }, + Annotations: map[string]string{ + "annotation-5": "value-5", + }, + }, + }, + } + + t.Run("update a rule", func(t *testing.T) { + // Create the rule group + _, status, body := apiClient.ConvertPrometheusPostRuleGroup(t, namespace1, ds.Body.Datasource.UID, promGroup, nil) + requireStatusCode(t, http.StatusAccepted, status, body) + + // Now get the group + group1 := apiClient.ConvertPrometheusGetRuleGroupRules(t, namespace1, promGroup.Name) + require.Equal(t, promGroup, group1) + + // Update the rule group interval + promGroup.Interval = prommodel.Duration(30 * time.Second) + // Update the query + promGroup.Rules[0].Expr = "disk_usage > 90" + // Labels, and annotations too + promGroup.Rules[0].Labels["another-label"] = "something" + promGroup.Rules[0].Annotations["another-annotation"] = "also-something" + // Update the group + _, status, body = apiClient.ConvertPrometheusPostRuleGroup(t, namespace1, ds.Body.Datasource.UID, promGroup, nil) + requireStatusCode(t, http.StatusAccepted, status, body) + + // Now get the group again and check that the rule group has been updated + group1 = apiClient.ConvertPrometheusGetRuleGroupRules(t, namespace1, promGroup.Name) + require.Equal(t, promGroup, group1) + }) + } + + t.Run("with the mimirtool paths", func(t *testing.T) { + runTest(t, false) + }) + + t.Run("with the cortextool Loki paths", func(t *testing.T) { + runTest(t, true) + }) +} + func TestIntegrationConvertPrometheusEndpoints_Conflict(t *testing.T) { runTest := func(t *testing.T, enableLokiPaths bool) { testinfra.SQLiteIntegrationTest(t)