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)