From d359591daccafc39930249cf16a32aa0fcd84e71 Mon Sep 17 00:00:00 2001 From: William Wernert Date: Thu, 6 Jun 2024 14:05:02 -0400 Subject: [PATCH] Alerting: Support recording rule struct in provisioning API (#87849) * Support record struct in provisioning API * Update api spec * Use record field * Restrict API endpoints following toggle * Fix swagger spec * Add recording rule validation to store validator --- pkg/services/ngalert/api/api.go | 2 + pkg/services/ngalert/api/api_provisioning.go | 22 +++ .../ngalert/api/api_provisioning_test.go | 124 +++++++++++++++++ pkg/services/ngalert/api/compat.go | 20 ++- pkg/services/ngalert/api/tooling/api.json | 32 ++++- .../api/tooling/definitions/cortex-ruler.go | 18 ++- .../definitions/provisioning_alert_rules.go | 9 ++ pkg/services/ngalert/api/tooling/post.json | 123 ++++++----------- pkg/services/ngalert/api/tooling/spec.json | 127 ++++++------------ pkg/services/ngalert/models/alert_rule.go | 40 ++++-- pkg/services/ngalert/models/testing.go | 1 + pkg/services/ngalert/store/alert_rule_test.go | 22 ++- .../provisioning/alerting/rules_types.go | 20 +++ public/api-merged.json | 36 ++++- public/openapi3.json | 32 ++++- 15 files changed, 426 insertions(+), 202 deletions(-) diff --git a/pkg/services/ngalert/api/api.go b/pkg/services/ngalert/api/api.go index db3a8df6b61..01e37e2acb9 100644 --- a/pkg/services/ngalert/api/api.go +++ b/pkg/services/ngalert/api/api.go @@ -164,6 +164,8 @@ func (api *API) RegisterAPIEndpoints(m *metrics.API) { templates: api.Templates, muteTimings: api.MuteTimings, alertRules: api.AlertRules, + // XXX: Used to flag recording rules, remove when FT is removed + featureManager: api.FeatureManager, }), m) api.RegisterHistoryApiEndpoints(NewStateHistoryApi(&HistorySrv{ diff --git a/pkg/services/ngalert/api/api_provisioning.go b/pkg/services/ngalert/api/api_provisioning.go index d938b917930..f6dc9aebf3e 100644 --- a/pkg/services/ngalert/api/api_provisioning.go +++ b/pkg/services/ngalert/api/api_provisioning.go @@ -11,6 +11,7 @@ import ( "github.com/grafana/grafana/pkg/infra/log" "github.com/grafana/grafana/pkg/services/auth/identity" contextmodel "github.com/grafana/grafana/pkg/services/contexthandler/model" + "github.com/grafana/grafana/pkg/services/featuremgmt" "github.com/grafana/grafana/pkg/services/folder" "github.com/grafana/grafana/pkg/services/ngalert/api/hcl" "github.com/grafana/grafana/pkg/services/ngalert/api/tooling/definitions" @@ -30,6 +31,9 @@ type ProvisioningSrv struct { muteTimings MuteTimingService alertRules AlertRuleService folderSvc folder.Service + + // XXX: Used to flag recording rules, remove when FT is removed + featureManager featuremgmt.FeatureToggles } type ContactPointService interface { @@ -335,6 +339,15 @@ func (srv *ProvisioningSrv) RoutePostAlertRule(c *contextmodel.ReqContext, ar de if err != nil { return ErrResp(http.StatusBadRequest, err, "") } + + if upstreamModel.IsRecordingRule() && !srv.featureManager.IsEnabledGlobally(featuremgmt.FlagGrafanaManagedRecordingRules) { + return ErrResp( + http.StatusBadRequest, + fmt.Errorf("%w: recording rules cannot be created on this instance", alerting_models.ErrAlertRuleFailedValidation), + "", + ) + } + provenance := determineProvenance(c) createdAlertRule, err := srv.alertRules.CreateAlertRule(c.Req.Context(), c.SignedInUser, upstreamModel, alerting_models.Provenance(provenance)) if errors.Is(err, alerting_models.ErrAlertRuleFailedValidation) { @@ -362,6 +375,15 @@ func (srv *ProvisioningSrv) RoutePutAlertRule(c *contextmodel.ReqContext, ar def if err != nil { ErrResp(http.StatusBadRequest, err, "") } + + if updated.IsRecordingRule() && !srv.featureManager.IsEnabledGlobally(featuremgmt.FlagGrafanaManagedRecordingRules) { + return ErrResp( + http.StatusBadRequest, + fmt.Errorf("%w: recording rules cannot be created on this instance", alerting_models.ErrAlertRuleFailedValidation), + "", + ) + } + updated.OrgID = c.SignedInUser.GetOrgID() updated.UID = UID provenance := determineProvenance(c) diff --git a/pkg/services/ngalert/api/api_provisioning_test.go b/pkg/services/ngalert/api/api_provisioning_test.go index 6bdcc7cc05b..015166c13f3 100644 --- a/pkg/services/ngalert/api/api_provisioning_test.go +++ b/pkg/services/ngalert/api/api_provisioning_test.go @@ -406,6 +406,99 @@ func TestProvisioningApi(t *testing.T) { }) }) + t.Run("recording rules", func(t *testing.T) { + t.Run("are enabled", func(t *testing.T) { + env := createTestEnv(t, testConfig) + env.features = featuremgmt.WithFeatures(featuremgmt.FlagGrafanaManagedRecordingRules) + + t.Run("POST returns 201", func(t *testing.T) { + sut := createProvisioningSrvSutFromEnv(t, &env) + rc := createTestRequestCtx() + rule := createTestRecordingRule("rule", 1) + + response := sut.RoutePostAlertRule(&rc, rule) + + require.Equal(t, 201, response.Status()) + }) + + t.Run("PUT returns 200", func(t *testing.T) { + sut := createProvisioningSrvSutFromEnv(t, &env) + uid := util.GenerateShortUID() + rule := createTestAlertRule("rule", 3) + rule.UID = uid + + _, err := sut.folderSvc.Create(context.Background(), &folder.CreateFolderCommand{ + UID: rule.FolderUID, + Title: "Folder Title", + OrgID: rule.OrgID, + SignedInUser: &user.SignedInUser{OrgID: rule.OrgID}, + }) + require.NoError(t, err) + + insertRuleInOrg(t, sut, rule, 3) + + // make rule a recording rule + rule.Record = &definitions.Record{ + Metric: "test_metric", + From: "A", + } + + rc := createTestRequestCtx() + rc.SignedInUser.OrgID = 3 + + response := sut.RoutePutAlertRule(&rc, rule, rule.UID) + + require.Equal(t, 200, response.Status()) + }) + }) + + t.Run("are not enabled", func(t *testing.T) { + t.Run("POST returns 400", func(t *testing.T) { + sut := createProvisioningSrvSut(t) + rc := createTestRequestCtx() + rule := createTestRecordingRule("rule", 1) + + response := sut.RoutePostAlertRule(&rc, rule) + + require.Equal(t, 400, response.Status()) + require.NotEmpty(t, response.Body()) + require.Contains(t, string(response.Body()), "recording rules cannot be created on this instance") + }) + + t.Run("PUT returns 400", func(t *testing.T) { + sut := createProvisioningSrvSut(t) + uid := util.GenerateShortUID() + rule := createTestAlertRule("rule", 3) + rule.UID = uid + + _, err := sut.folderSvc.Create(context.Background(), &folder.CreateFolderCommand{ + UID: rule.FolderUID, + Title: "Folder Title", + OrgID: rule.OrgID, + SignedInUser: &user.SignedInUser{OrgID: rule.OrgID}, + }) + require.NoError(t, err) + + insertRuleInOrg(t, sut, rule, 3) + + // make rule a recording rule + rule.Record = &definitions.Record{ + Metric: "test_metric", + From: "A", + } + + rc := createTestRequestCtx() + rc.SignedInUser.OrgID = 3 + + response := sut.RoutePutAlertRule(&rc, rule, rule.UID) + + require.Equal(t, 400, response.Status()) + require.NotEmpty(t, response.Body()) + require.Contains(t, string(response.Body()), "recording rules cannot be created on this instance") + }) + }) + }) + t.Run("alert rule groups", func(t *testing.T) { t.Run("are present", func(t *testing.T) { sut := createProvisioningSrvSut(t) @@ -1644,6 +1737,7 @@ type testEnvironment struct { ac *recordingAccessControlFake user *user.SignedInUser rulesAuthz *fakes.FakeRuleService + features featuremgmt.FeatureToggles } func createTestEnv(t *testing.T, testConfig string) testEnvironment { @@ -1738,6 +1832,8 @@ func createTestEnv(t *testing.T, testConfig string) testEnvironment { ruleAuthz := &fakes.FakeRuleService{} + features := featuremgmt.WithFeatures() + return testEnvironment{ secrets: secretsService, log: log, @@ -1751,6 +1847,7 @@ func createTestEnv(t *testing.T, testConfig string) testEnvironment { ac: ac, user: user, rulesAuthz: ruleAuthz, + features: features, } } @@ -1773,6 +1870,7 @@ func createProvisioningSrvSutFromEnv(t *testing.T, env *testEnvironment) Provisi muteTimings: provisioning.NewMuteTimingService(env.configs, env.prov, env.xact, env.log), alertRules: provisioning.NewAlertRuleService(env.store, env.prov, env.folderService, env.quotas, env.xact, 60, 10, 100, env.log, &provisioning.NotificationSettingsValidatorProviderFake{}, env.rulesAuthz), folderSvc: env.folderService, + featureManager: env.features, } } @@ -1973,6 +2071,32 @@ func createTestAlertRule(title string, orgID int64) definitions.ProvisionedAlert } } +func createTestRecordingRule(title string, orgID int64) definitions.ProvisionedAlertRule { + return definitions.ProvisionedAlertRule{ + UID: title, + OrgID: orgID, + Title: title, + Condition: "A", + Data: []definitions.AlertQuery{ + { + RefID: "A", + Model: json.RawMessage(testModel), + RelativeTimeRange: definitions.RelativeTimeRange{ + From: definitions.Duration(60), + To: definitions.Duration(0), + }, + }, + }, + RuleGroup: "my-cool-group", + FolderUID: "folder-uid", + For: model.Duration(60), + Record: &definitions.Record{ + Metric: "test_record", + From: "A", + }, + } +} + func insertRule(t *testing.T, srv ProvisioningSrv, rule definitions.ProvisionedAlertRule) { insertRuleInOrg(t, srv, rule, 1) } diff --git a/pkg/services/ngalert/api/compat.go b/pkg/services/ngalert/api/compat.go index bca960a5b09..20a3fb3fcfe 100644 --- a/pkg/services/ngalert/api/compat.go +++ b/pkg/services/ngalert/api/compat.go @@ -32,9 +32,7 @@ func AlertRuleFromProvisionedAlertRule(a definitions.ProvisionedAlertRule) (mode Labels: a.Labels, IsPaused: a.IsPaused, NotificationSettings: NotificationSettingsFromAlertRuleNotificationSettings(a.NotificationSettings), - // Recording Rule fields will be implemented in the future. - // For now, no rules can be recording rules. So, we force these to be empty. - Record: nil, + Record: ModelRecordFromApiRecord(a.Record), }, nil } @@ -58,6 +56,7 @@ func ProvisionedAlertRuleFromAlertRule(rule models.AlertRule, provenance models. Provenance: definitions.Provenance(provenance), // TODO validate enum conversion? IsPaused: rule.IsPaused, NotificationSettings: AlertRuleNotificationSettingsFromNotificationSettings(rule.NotificationSettings), + Record: ApiRecordFromModelRecord(rule.Record), } } @@ -192,6 +191,7 @@ func AlertRuleExportFromAlertRule(rule models.AlertRule) (definitions.AlertRuleE ExecErrState: definitions.ExecutionErrorState(rule.ExecErrState), IsPaused: rule.IsPaused, NotificationSettings: AlertRuleNotificationSettingsExportFromNotificationSettings(rule.NotificationSettings), + Record: AlertRuleRecordExportFromRecord(rule.Record), } if rule.For.Seconds() > 0 { result.ForString = util.Pointer(model.Duration(rule.For).String()) @@ -456,11 +456,11 @@ func NotificationSettingsFromAlertRuleNotificationSettings(ns *definitions.Alert } } -func ApiRecordFromModelRecord(r *models.Record) *definitions.Record { +func AlertRuleRecordExportFromRecord(r *models.Record) *definitions.AlertRuleRecordExport { if r == nil { return nil } - return &definitions.Record{ + return &definitions.AlertRuleRecordExport{ Metric: r.Metric, From: r.From, } @@ -475,3 +475,13 @@ func ModelRecordFromApiRecord(r *definitions.Record) *models.Record { From: r.From, } } + +func ApiRecordFromModelRecord(r *models.Record) *definitions.Record { + if r == nil { + return nil + } + return &definitions.Record{ + Metric: r.Metric, + From: r.From, + } +} diff --git a/pkg/services/ngalert/api/tooling/api.json b/pkg/services/ngalert/api/tooling/api.json index db28b0c5d5a..22b6bc43335 100644 --- a/pkg/services/ngalert/api/tooling/api.json +++ b/pkg/services/ngalert/api/tooling/api.json @@ -218,6 +218,9 @@ "format": "int64", "type": "integer" }, + "record": { + "$ref": "#/definitions/AlertRuleRecordExport" + }, "title": { "type": "string" }, @@ -367,6 +370,18 @@ "title": "AlertRuleNotificationSettingsExport is the provisioned export of models.NotificationSettings.", "type": "object" }, + "AlertRuleRecordExport": { + "properties": { + "from": { + "type": "string" + }, + "metric": { + "type": "string" + } + }, + "title": "Record is the provisioned export of models.Record.", + "type": "object" + }, "AlertingFileExport": { "properties": { "apiVersion": { @@ -2922,6 +2937,9 @@ "provenance": { "$ref": "#/definitions/Provenance" }, + "record": { + "$ref": "#/definitions/Record" + }, "ruleGroup": { "example": "eval_group_1", "maxLength": 190, @@ -3256,13 +3274,20 @@ "Record": { "properties": { "from": { + "description": "Which expression node should be used as the input for the recorded metric.", + "example": "A", "type": "string" }, "metric": { + "description": "Name of the recorded metric.", + "example": "grafana_alerts_ratio", "type": "string" } }, - "title": "Record defines how data produced by a recording rule is written.", + "required": [ + "metric", + "from" + ], "type": "object" }, "RelativeTimeRange": { @@ -4435,7 +4460,6 @@ "type": "object" }, "alertGroup": { - "description": "AlertGroup alert group", "properties": { "alerts": { "description": "alerts", @@ -4459,7 +4483,6 @@ "type": "object" }, "alertGroups": { - "description": "AlertGroups alert groups", "items": { "$ref": "#/definitions/alertGroup" }, @@ -4626,6 +4649,7 @@ "type": "array" }, "gettableSilence": { + "description": "GettableSilence gettable silence", "properties": { "comment": { "description": "comment", @@ -4681,6 +4705,7 @@ "type": "array" }, "integration": { + "description": "Integration integration", "properties": { "lastNotifyAttempt": { "description": "A timestamp indicating the last attempt to deliver a notification regardless of the outcome.\nFormat: date-time", @@ -4861,7 +4886,6 @@ "type": "object" }, "receiver": { - "description": "Receiver receiver", "properties": { "active": { "description": "active", diff --git a/pkg/services/ngalert/api/tooling/definitions/cortex-ruler.go b/pkg/services/ngalert/api/tooling/definitions/cortex-ruler.go index b16523cbc7d..433b86f8ee8 100644 --- a/pkg/services/ngalert/api/tooling/definitions/cortex-ruler.go +++ b/pkg/services/ngalert/api/tooling/definitions/cortex-ruler.go @@ -477,6 +477,18 @@ type AlertRuleNotificationSettings struct { MuteTimeIntervals []string `json:"mute_time_intervals,omitempty"` } +// swagger:model +type Record struct { + // Name of the recorded metric. + // required: true + // example: grafana_alerts_ratio + Metric string `json:"metric" yaml:"metric"` + // Which expression node should be used as the input for the recorded metric. + // required: true + // example: A + From string `json:"from" yaml:"from"` +} + // swagger:model type PostableGrafanaRule struct { Title string `json:"title" yaml:"title"` @@ -578,12 +590,6 @@ func (d *Duration) UnmarshalYAML(unmarshal func(any) error) error { } } -// Record defines how data produced by a recording rule is written. -type Record struct { - Metric string `json:"metric" yaml:"metric"` - From string `json:"from" yaml:"from"` -} - // swagger:model type UpdateRuleGroupResponse struct { Message string `json:"message"` diff --git a/pkg/services/ngalert/api/tooling/definitions/provisioning_alert_rules.go b/pkg/services/ngalert/api/tooling/definitions/provisioning_alert_rules.go index c3d8d9bcf2c..5681a376cef 100644 --- a/pkg/services/ngalert/api/tooling/definitions/provisioning_alert_rules.go +++ b/pkg/services/ngalert/api/tooling/definitions/provisioning_alert_rules.go @@ -167,6 +167,8 @@ type ProvisionedAlertRule struct { IsPaused bool `json:"isPaused"` // example: {"receiver":"email","group_by":["alertname","grafana_folder","cluster"],"group_wait":"30s","group_interval":"1m","repeat_interval":"4d","mute_time_intervals":["Weekends","Holidays"]} NotificationSettings *AlertRuleNotificationSettings `json:"notification_settings"` + //example: {"metric":"grafana_alerts_ratio", "from":"A"} + Record *Record `json:"record"` } // swagger:route GET /v1/provisioning/folder/{FolderUID}/rule-groups/{Group} provisioning stable RouteGetAlertRuleGroup @@ -273,6 +275,7 @@ type AlertRuleExport struct { Labels *map[string]string `json:"labels,omitempty" yaml:"labels,omitempty" hcl:"labels"` IsPaused bool `json:"isPaused" yaml:"isPaused" hcl:"is_paused"` NotificationSettings *AlertRuleNotificationSettingsExport `json:"notification_settings,omitempty" yaml:"notification_settings,omitempty" hcl:"notification_settings,block"` + Record *AlertRuleRecordExport `json:"record,omitempty" yaml:"record,omitempty" hcl:"record"` } // AlertQueryExport is the provisioned export of models.AlertQuery. @@ -301,3 +304,9 @@ type AlertRuleNotificationSettingsExport struct { RepeatInterval *string `yaml:"repeat_interval,omitempty" json:"repeat_interval,omitempty" hcl:"repeat_interval,optional"` MuteTimeIntervals []string `yaml:"mute_time_intervals,omitempty" json:"mute_time_intervals,omitempty" hcl:"mute_timings"` // TF -> `mute_timings` } + +// Record is the provisioned export of models.Record. +type AlertRuleRecordExport struct { + Metric string `json:"metric" yaml:"metric" hcl:"metric"` + From string `json:"from" yaml:"from" hcl:"from"` +} diff --git a/pkg/services/ngalert/api/tooling/post.json b/pkg/services/ngalert/api/tooling/post.json index 8fcf6f3c609..65ad6b1860d 100644 --- a/pkg/services/ngalert/api/tooling/post.json +++ b/pkg/services/ngalert/api/tooling/post.json @@ -218,6 +218,9 @@ "format": "int64", "type": "integer" }, + "record": { + "$ref": "#/definitions/AlertRuleRecordExport" + }, "title": { "type": "string" }, @@ -367,6 +370,18 @@ "title": "AlertRuleNotificationSettingsExport is the provisioned export of models.NotificationSettings.", "type": "object" }, + "AlertRuleRecordExport": { + "properties": { + "from": { + "type": "string" + }, + "metric": { + "type": "string" + } + }, + "title": "Record is the provisioned export of models.Record.", + "type": "object" + }, "AlertingFileExport": { "properties": { "apiVersion": { @@ -967,7 +982,9 @@ }, "type": "object" }, - "EvalQueriesResponse": {}, + "EvalQueriesResponse": { + "type": "object" + }, "ExplorePanelsState": { "description": "This is an object constructed with the keys as the values of the enum VisType and the value being a bag of properties" }, @@ -2922,6 +2939,9 @@ "provenance": { "$ref": "#/definitions/Provenance" }, + "record": { + "$ref": "#/definitions/Record" + }, "ruleGroup": { "example": "eval_group_1", "maxLength": 190, @@ -3256,13 +3276,20 @@ "Record": { "properties": { "from": { + "description": "Which expression node should be used as the input for the recorded metric.", + "example": "A", "type": "string" }, "metric": { + "description": "Name of the recorded metric.", + "example": "grafana_alerts_ratio", "type": "string" } }, - "title": "Record defines how data produced by a recording rule is written.", + "required": [ + "metric", + "from" + ], "type": "object" }, "RelativeTimeRange": { @@ -4461,7 +4488,8 @@ }, "alertGroups": { "items": { - "$ref": "#/definitions/alertGroup" + "$ref": "#/definitions/alertGroup", + "type": "object" }, "type": "array" }, @@ -4564,49 +4592,6 @@ "type": "object" }, "gettableAlert": { - "description": "GettableAlert gettable alert", - "properties": { - "annotations": { - "$ref": "#/definitions/labelSet" - }, - "endsAt": { - "description": "ends at", - "format": "date-time", - "type": "string" - }, - "fingerprint": { - "description": "fingerprint", - "type": "string" - }, - "generatorURL": { - "description": "generator URL\nFormat: uri", - "format": "uri", - "type": "string" - }, - "labels": { - "$ref": "#/definitions/labelSet" - }, - "receivers": { - "description": "receivers", - "items": { - "$ref": "#/definitions/receiver" - }, - "type": "array" - }, - "startsAt": { - "description": "starts at", - "format": "date-time", - "type": "string" - }, - "status": { - "$ref": "#/definitions/alertStatus" - }, - "updatedAt": { - "description": "updated at", - "format": "date-time", - "type": "string" - } - }, "required": [ "labels", "annotations", @@ -4620,49 +4605,13 @@ "type": "object" }, "gettableAlerts": { - "description": "GettableAlerts gettable alerts", "items": { - "$ref": "#/definitions/gettableAlert" + "$ref": "#/definitions/gettableAlert", + "type": "object" }, "type": "array" }, "gettableSilence": { - "description": "GettableSilence gettable silence", - "properties": { - "comment": { - "description": "comment", - "type": "string" - }, - "createdBy": { - "description": "created by", - "type": "string" - }, - "endsAt": { - "description": "ends at", - "format": "date-time", - "type": "string" - }, - "id": { - "description": "id", - "type": "string" - }, - "matchers": { - "$ref": "#/definitions/matchers" - }, - "startsAt": { - "description": "starts at", - "format": "date-time", - "type": "string" - }, - "status": { - "$ref": "#/definitions/silenceStatus" - }, - "updatedAt": { - "description": "updated at", - "format": "date-time", - "type": "string" - } - }, "required": [ "comment", "createdBy", @@ -4677,11 +4626,13 @@ }, "gettableSilences": { "items": { - "$ref": "#/definitions/gettableSilence" + "$ref": "#/definitions/gettableSilence", + "type": "object" }, "type": "array" }, "integration": { + "description": "Integration integration", "properties": { "lastNotifyAttempt": { "description": "A timestamp indicating the last attempt to deliver a notification regardless of the outcome.\nFormat: date-time", @@ -4825,6 +4776,7 @@ "type": "array" }, "postableSilence": { + "description": "PostableSilence postable silence", "properties": { "comment": { "description": "comment", @@ -4862,6 +4814,7 @@ "type": "object" }, "receiver": { + "description": "Receiver receiver", "properties": { "active": { "description": "active", diff --git a/pkg/services/ngalert/api/tooling/spec.json b/pkg/services/ngalert/api/tooling/spec.json index 380f88e6fc5..e7df1af1f50 100644 --- a/pkg/services/ngalert/api/tooling/spec.json +++ b/pkg/services/ngalert/api/tooling/spec.json @@ -3731,6 +3731,9 @@ "type": "integer", "format": "int64" }, + "record": { + "$ref": "#/definitions/AlertRuleRecordExport" + }, "title": { "type": "string" }, @@ -3878,6 +3881,18 @@ } } }, + "AlertRuleRecordExport": { + "type": "object", + "title": "Record is the provisioned export of models.Record.", + "properties": { + "from": { + "type": "string" + }, + "metric": { + "type": "string" + } + } + }, "AlertingFileExport": { "type": "object", "title": "AlertingFileExport is the full provisioned file export.", @@ -4480,7 +4495,7 @@ } }, "EvalQueriesResponse": { - "$ref": "#/definitions/EvalQueriesResponse" + "type": "object" }, "ExplorePanelsState": { "description": "This is an object constructed with the keys as the values of the enum VisType and the value being a bag of properties" @@ -6449,6 +6464,9 @@ "provenance": { "$ref": "#/definitions/Provenance" }, + "record": { + "$ref": "#/definitions/Record" + }, "ruleGroup": { "type": "string", "maxLength": 190, @@ -6770,13 +6788,20 @@ }, "Record": { "type": "object", - "title": "Record defines how data produced by a recording rule is written.", + "required": [ + "metric", + "from" + ], "properties": { "from": { - "type": "string" + "description": "Which expression node should be used as the input for the recorded metric.", + "type": "string", + "example": "A" }, "metric": { - "type": "string" + "description": "Name of the recorded metric.", + "type": "string", + "example": "grafana_alerts_ratio" } } }, @@ -7972,15 +7997,15 @@ "receiver": { "$ref": "#/definitions/receiver" } - }, - "$ref": "#/definitions/alertGroup" + } }, "alertGroups": { + "description": "AlertGroups alert groups", "type": "array", "items": { + "type": "object", "$ref": "#/definitions/alertGroup" - }, - "$ref": "#/definitions/alertGroups" + } }, "alertStatus": { "description": "AlertStatus alert status", @@ -8081,7 +8106,6 @@ } }, "gettableAlert": { - "description": "GettableAlert gettable alert", "type": "object", "required": [ "labels", @@ -8092,58 +8116,14 @@ "startsAt", "status", "updatedAt" - ], - "properties": { - "annotations": { - "$ref": "#/definitions/labelSet" - }, - "endsAt": { - "description": "ends at", - "type": "string", - "format": "date-time" - }, - "fingerprint": { - "description": "fingerprint", - "type": "string" - }, - "generatorURL": { - "description": "generator URL\nFormat: uri", - "type": "string", - "format": "uri" - }, - "labels": { - "$ref": "#/definitions/labelSet" - }, - "receivers": { - "description": "receivers", - "type": "array", - "items": { - "$ref": "#/definitions/receiver" - } - }, - "startsAt": { - "description": "starts at", - "type": "string", - "format": "date-time" - }, - "status": { - "$ref": "#/definitions/alertStatus" - }, - "updatedAt": { - "description": "updated at", - "type": "string", - "format": "date-time" - } - }, - "$ref": "#/definitions/gettableAlert" + ] }, "gettableAlerts": { - "description": "GettableAlerts gettable alerts", "type": "array", "items": { + "type": "object", "$ref": "#/definitions/gettableAlert" - }, - "$ref": "#/definitions/gettableAlerts" + } }, "gettableSilence": { "description": "GettableSilence gettable silence", @@ -8192,17 +8172,17 @@ "type": "string", "format": "date-time" } - }, - "$ref": "#/definitions/gettableSilence" + } }, "gettableSilences": { "type": "array", "items": { + "type": "object", "$ref": "#/definitions/gettableSilence" - }, - "$ref": "#/definitions/gettableSilences" + } }, "integration": { + "description": "Integration integration", "type": "object", "required": [ "name", @@ -8230,8 +8210,7 @@ "description": "send resolved", "type": "boolean" } - }, - "$ref": "#/definitions/integration" + } }, "labelSet": { "description": "LabelSet label set", @@ -8347,6 +8326,7 @@ } }, "postableSilence": { + "description": "PostableSilence postable silence", "type": "object", "required": [ "comment", @@ -8381,8 +8361,7 @@ "type": "string", "format": "date-time" } - }, - "$ref": "#/definitions/postableSilence" + } }, "receiver": { "type": "object", @@ -8390,25 +8369,7 @@ "active", "integrations", "name" - ], - "properties": { - "active": { - "description": "active", - "type": "boolean" - }, - "integrations": { - "description": "integrations", - "type": "array", - "items": { - "$ref": "#/definitions/integration" - } - }, - "name": { - "description": "name", - "type": "string" - } - }, - "$ref": "#/definitions/receiver" + ] }, "silence": { "description": "Silence silence", diff --git a/pkg/services/ngalert/models/alert_rule.go b/pkg/services/ngalert/models/alert_rule.go index 1a3742a47a2..a2a1680c041 100644 --- a/pkg/services/ngalert/models/alert_rule.go +++ b/pkg/services/ngalert/models/alert_rule.go @@ -16,6 +16,7 @@ import ( "github.com/google/go-cmp/cmp/cmpopts" "github.com/grafana/grafana-plugin-sdk-go/data" + prommodels "github.com/prometheus/common/model" alertingModels "github.com/grafana/alerting/models" @@ -510,14 +511,14 @@ func (alertRule *AlertRule) ValidateAlertRule(cfg setting.UnifiedAlertingSetting return fmt.Errorf("%w: cannot have Panel ID without a Dashboard UID", ErrAlertRuleFailedValidation) } - if !alertRule.IsRecordingRule() { - if _, err := ErrStateFromString(string(alertRule.ExecErrState)); err != nil { - return err - } - - if _, err := NoDataStateFromString(string(alertRule.NoDataState)); err != nil { - return err - } + var err error + if alertRule.IsRecordingRule() { + err = validateRecordingRuleFields(alertRule) + } else { + err = validateAlertRuleFields(alertRule) + } + if err != nil { + return err } if alertRule.For < 0 { @@ -543,6 +544,29 @@ func (alertRule *AlertRule) ValidateAlertRule(cfg setting.UnifiedAlertingSetting return nil } +func validateAlertRuleFields(rule *AlertRule) error { + if _, err := ErrStateFromString(string(rule.ExecErrState)); err != nil { + return err + } + + if _, err := NoDataStateFromString(string(rule.NoDataState)); err != nil { + return err + } + + return nil +} + +func validateRecordingRuleFields(rule *AlertRule) error { + metricName := prommodels.LabelValue(rule.Record.Metric) + if !metricName.IsValid() { + return fmt.Errorf("%w: %s", ErrAlertRuleFailedValidation, "metric name for recording rule must be a valid utf8 string") + } + if !prommodels.IsValidMetricName(metricName) { + return fmt.Errorf("%w: %s", ErrAlertRuleFailedValidation, "metric name for recording rule must be a valid Prometheus metric name") + } + return nil +} + func (alertRule *AlertRule) ResourceType() string { return "alertRule" } diff --git a/pkg/services/ngalert/models/testing.go b/pkg/services/ngalert/models/testing.go index 0dcc4ecc546..8dfaa17807b 100644 --- a/pkg/services/ngalert/models/testing.go +++ b/pkg/services/ngalert/models/testing.go @@ -603,6 +603,7 @@ func CopyRule(r *AlertRule, mutators ...AlertRuleMutator) *AlertRule { NoDataState: r.NoDataState, ExecErrState: r.ExecErrState, For: r.For, + Record: r.Record, } if r.DashboardUID != nil { diff --git a/pkg/services/ngalert/store/alert_rule_test.go b/pkg/services/ngalert/store/alert_rule_test.go index 3342c39821a..a28b2aeef65 100644 --- a/pkg/services/ngalert/store/alert_rule_test.go +++ b/pkg/services/ngalert/store/alert_rule_test.go @@ -77,7 +77,7 @@ func TestIntegrationUpdateAlertRules(t *testing.T) { t.Run("updating record field should increase version", func(t *testing.T) { rule := createRule(t, store, recordingRuleGen) newRule := models.CopyRule(rule) - newRule.Record.Metric = "new-metric" + newRule.Record.Metric = "new_metric" err := store.UpdateAlertRules(context.Background(), []models.UpdateRule{{ Existing: rule, @@ -721,6 +721,26 @@ func TestIntegrationInsertAlertRules(t *testing.T) { } }) + t.Run("inserted recording rules fail validation if metric name is invalid", func(t *testing.T) { + t.Run("invalid UTF-8", func(t *testing.T) { + invalidMetric := "my_metric\x80" + invalidRule := recordingRulesGen.Generate() + invalidRule.Record.Metric = invalidMetric + _, err := store.InsertAlertRules(context.Background(), []models.AlertRule{invalidRule}) + require.ErrorIs(t, err, models.ErrAlertRuleFailedValidation) + require.ErrorContains(t, err, "metric name for recording rule must be a valid utf8 string") + }) + + t.Run("invalid metric name", func(t *testing.T) { + invalidMetric := "with-dashes" + invalidRule := recordingRulesGen.Generate() + invalidRule.Record.Metric = invalidMetric + _, err := store.InsertAlertRules(context.Background(), []models.AlertRule{invalidRule}) + require.ErrorIs(t, err, models.ErrAlertRuleFailedValidation) + require.ErrorContains(t, err, "metric name for recording rule must be a valid Prometheus metric name") + }) + }) + t.Run("fail to insert rules with same ID", func(t *testing.T) { _, err = store.InsertAlertRules(context.Background(), []models.AlertRule{rules[0]}) require.ErrorIs(t, err, models.ErrAlertRuleConflictBase) diff --git a/pkg/services/provisioning/alerting/rules_types.go b/pkg/services/provisioning/alerting/rules_types.go index 95f1218d17b..cce86eedba4 100644 --- a/pkg/services/provisioning/alerting/rules_types.go +++ b/pkg/services/provisioning/alerting/rules_types.go @@ -75,6 +75,7 @@ type AlertRuleV1 struct { Labels values.StringMapValue `json:"labels" yaml:"labels"` IsPaused values.BoolValue `json:"isPaused" yaml:"isPaused"` NotificationSettings *NotificationSettingsV1 `json:"notification_settings" yaml:"notification_settings"` + Record *RecordV1 `json:"record" yaml:"record"` } func (rule *AlertRuleV1) mapToModel(orgID int64) (models.AlertRule, error) { @@ -139,6 +140,13 @@ func (rule *AlertRuleV1) mapToModel(orgID int64) (models.AlertRule, error) { } alertRule.NotificationSettings = append(alertRule.NotificationSettings, ns) } + if rule.Record != nil { + record, err := rule.Record.mapToModel() + if err != nil { + return models.AlertRule{}, fmt.Errorf("rule '%s' failed to parse: %w", alertRule.Title, err) + } + alertRule.Record = &record + } return alertRule, nil } @@ -246,3 +254,15 @@ func (nsV1 *NotificationSettingsV1) mapToModel() (models.NotificationSettings, e MuteTimeIntervals: mute, }, nil } + +type RecordV1 struct { + Metric values.StringValue `json:"metric" yaml:"metric"` + From values.StringValue `json:"from" yaml:"from"` +} + +func (record *RecordV1) mapToModel() (models.Record, error) { + return models.Record{ + Metric: record.Metric.Value(), + From: record.From.Value(), + }, nil +} diff --git a/public/api-merged.json b/public/api-merged.json index c0d34d48a37..898236625ed 100644 --- a/public/api-merged.json +++ b/public/api-merged.json @@ -12191,6 +12191,9 @@ "type": "integer", "format": "int64" }, + "record": { + "$ref": "#/definitions/AlertRuleRecordExport" + }, "title": { "type": "string" }, @@ -12338,6 +12341,18 @@ } } }, + "AlertRuleRecordExport": { + "type": "object", + "title": "Record is the provisioned export of models.Record.", + "properties": { + "from": { + "type": "string" + }, + "metric": { + "type": "string" + } + } + }, "AlertingFileExport": { "type": "object", "title": "AlertingFileExport is the full provisioned file export.", @@ -17944,6 +17959,9 @@ "provenance": { "$ref": "#/definitions/Provenance" }, + "record": { + "$ref": "#/definitions/Record" + }, "ruleGroup": { "type": "string", "maxLength": 190, @@ -18511,13 +18529,20 @@ }, "Record": { "type": "object", - "title": "Record defines how data produced by a recording rule is written.", + "required": [ + "metric", + "from" + ], "properties": { "from": { - "type": "string" + "description": "Which expression node should be used as the input for the recorded metric.", + "type": "string", + "example": "A" }, "metric": { - "type": "string" + "description": "Name of the recorded metric.", + "type": "string", + "example": "grafana_alerts_ratio" } } }, @@ -21472,7 +21497,6 @@ } }, "alertGroup": { - "description": "AlertGroup alert group", "type": "object", "required": [ "alerts", @@ -21496,7 +21520,6 @@ } }, "alertGroups": { - "description": "AlertGroups alert groups", "type": "array", "items": { "$ref": "#/definitions/alertGroup" @@ -21691,6 +21714,7 @@ } }, "gettableSilence": { + "description": "GettableSilence gettable silence", "type": "object", "required": [ "comment", @@ -21746,6 +21770,7 @@ } }, "integration": { + "description": "Integration integration", "type": "object", "required": [ "name", @@ -21954,7 +21979,6 @@ } }, "receiver": { - "description": "Receiver receiver", "type": "object", "required": [ "active", diff --git a/public/openapi3.json b/public/openapi3.json index 049e953ef3e..c846cd1b7fc 100644 --- a/public/openapi3.json +++ b/public/openapi3.json @@ -2585,6 +2585,9 @@ "format": "int64", "type": "integer" }, + "record": { + "$ref": "#/components/schemas/AlertRuleRecordExport" + }, "title": { "type": "string" }, @@ -2734,6 +2737,18 @@ "title": "AlertRuleNotificationSettingsExport is the provisioned export of models.NotificationSettings.", "type": "object" }, + "AlertRuleRecordExport": { + "properties": { + "from": { + "type": "string" + }, + "metric": { + "type": "string" + } + }, + "title": "Record is the provisioned export of models.Record.", + "type": "object" + }, "AlertingFileExport": { "properties": { "apiVersion": { @@ -8328,6 +8343,9 @@ "provenance": { "$ref": "#/components/schemas/Provenance" }, + "record": { + "$ref": "#/components/schemas/Record" + }, "ruleGroup": { "example": "eval_group_1", "maxLength": 190, @@ -8908,13 +8926,20 @@ "Record": { "properties": { "from": { + "description": "Which expression node should be used as the input for the recorded metric.", + "example": "A", "type": "string" }, "metric": { + "description": "Name of the recorded metric.", + "example": "grafana_alerts_ratio", "type": "string" } }, - "title": "Record defines how data produced by a recording rule is written.", + "required": [ + "metric", + "from" + ], "type": "object" }, "RecordingRuleJSON": { @@ -11867,7 +11892,6 @@ "type": "object" }, "alertGroup": { - "description": "AlertGroup alert group", "properties": { "alerts": { "description": "alerts", @@ -11891,7 +11915,6 @@ "type": "object" }, "alertGroups": { - "description": "AlertGroups alert groups", "items": { "$ref": "#/components/schemas/alertGroup" }, @@ -12086,6 +12109,7 @@ "type": "array" }, "gettableSilence": { + "description": "GettableSilence gettable silence", "properties": { "comment": { "description": "comment", @@ -12141,6 +12165,7 @@ "type": "array" }, "integration": { + "description": "Integration integration", "properties": { "lastNotifyAttempt": { "description": "A timestamp indicating the last attempt to deliver a notification regardless of the outcome.\nFormat: date-time", @@ -12349,7 +12374,6 @@ "type": "object" }, "receiver": { - "description": "Receiver receiver", "properties": { "active": { "description": "active",