Alerting: Extend PUT rule-group route to write the entire rule group rather than top-level fields only (#53078)

* Wire up to full alert rule struct

* Extract group change detection logic to dedicated file

* GroupDiff -> GroupDelta for consistency

* Calculate deltas and handle backwards compatible requests

* Separate changes and insert/update/delete as needed

* Regenerate files

* Don't touch the DB if there are no changes

* Quota checking, delete unused file

* Mark modified records as provisioned

* Validation + a couple API layer tests

* Address linter errors

* Fix issue with UID assignment and rule creation

* Propagate top level group fields to all rules

* Tests for repeated updates and versioning

* Tests for quota and provenance checks

* Fix linter errors

* Regenerate

* Factor out some shared logic

* Drop unnecessary multiple nilchecks

* Use alternative strategy for rolling UIDs on inserted rules

* Fix tests, add back nilcheck, refresh UIDs during test

* Address feedback

* Add missing nil-check
This commit is contained in:
Alexander Weaver
2022-08-10 12:33:41 -05:00
committed by GitHub
parent dc23643bee
commit b198559225
12 changed files with 374 additions and 41 deletions

View File

@ -56,7 +56,7 @@ type AlertRuleService interface {
UpdateAlertRule(ctx context.Context, rule alerting_models.AlertRule, provenance alerting_models.Provenance) (alerting_models.AlertRule, error) UpdateAlertRule(ctx context.Context, rule alerting_models.AlertRule, provenance alerting_models.Provenance) (alerting_models.AlertRule, error)
DeleteAlertRule(ctx context.Context, orgID int64, ruleUID string, provenance alerting_models.Provenance) error DeleteAlertRule(ctx context.Context, orgID int64, ruleUID string, provenance alerting_models.Provenance) error
GetRuleGroup(ctx context.Context, orgID int64, folder, group string) (definitions.AlertRuleGroup, error) GetRuleGroup(ctx context.Context, orgID int64, folder, group string) (definitions.AlertRuleGroup, error)
UpdateRuleGroup(ctx context.Context, orgID int64, folderUID, rulegroup string, interval int64) error ReplaceRuleGroup(ctx context.Context, orgID int64, group definitions.AlertRuleGroup, userID int64, provenance alerting_models.Provenance) error
} }
func (srv *ProvisioningSrv) RouteGetPolicyTree(c *models.ReqContext) response.Response { func (srv *ProvisioningSrv) RouteGetPolicyTree(c *models.ReqContext) response.Response {
@ -312,8 +312,13 @@ func (srv *ProvisioningSrv) RouteGetAlertRuleGroup(c *models.ReqContext, folder
return response.JSON(http.StatusOK, g) return response.JSON(http.StatusOK, g)
} }
func (srv *ProvisioningSrv) RoutePutAlertRuleGroup(c *models.ReqContext, ag definitions.AlertRuleGroupMetadata, folderUID string, group string) response.Response { func (srv *ProvisioningSrv) RoutePutAlertRuleGroup(c *models.ReqContext, ag definitions.AlertRuleGroup, folderUID string, group string) response.Response {
err := srv.alertRules.UpdateRuleGroup(c.Req.Context(), c.OrgId, folderUID, group, ag.Interval) ag.FolderUID = folderUID
ag.Title = group
err := srv.alertRules.ReplaceRuleGroup(c.Req.Context(), c.OrgId, ag, c.UserId, alerting_models.ProvenanceAPI)
if errors.Is(err, alerting_models.ErrAlertRuleFailedValidation) {
return ErrResp(http.StatusBadRequest, err, "")
}
if err != nil { if err != nil {
if errors.Is(err, store.ErrOptimisticLock) { if errors.Is(err, store.ErrOptimisticLock) {
return ErrResp(http.StatusConflict, err, "") return ErrResp(http.StatusConflict, err, "")

View File

@ -299,6 +299,37 @@ func TestProvisioningApi(t *testing.T) {
require.Equal(t, 404, response.Status()) require.Equal(t, 404, response.Status())
}) })
t.Run("are invalid at group level", func(t *testing.T) {
t.Run("PUT returns 400", func(t *testing.T) {
sut := createProvisioningSrvSut(t)
rc := createTestRequestCtx()
insertRule(t, sut, createTestAlertRule("rule", 1))
group := createInvalidAlertRuleGroup()
group.Interval = 0
response := sut.RoutePutAlertRuleGroup(&rc, group, "folder-uid", group.Title)
require.Equal(t, 400, response.Status())
require.NotEmpty(t, response.Body())
require.Contains(t, string(response.Body()), "invalid alert rule")
})
})
t.Run("are invalid at rule level", func(t *testing.T) {
t.Run("PUT returns 400", func(t *testing.T) {
sut := createProvisioningSrvSut(t)
rc := createTestRequestCtx()
insertRule(t, sut, createTestAlertRule("rule", 1))
group := createInvalidAlertRuleGroup()
response := sut.RoutePutAlertRuleGroup(&rc, group, "folder-uid", group.Title)
require.Equal(t, 400, response.Status())
require.NotEmpty(t, response.Body())
require.Contains(t, string(response.Body()), "invalid alert rule")
})
})
}) })
} }
@ -477,6 +508,14 @@ func createInvalidAlertRule() definitions.ProvisionedAlertRule {
return definitions.ProvisionedAlertRule{} return definitions.ProvisionedAlertRule{}
} }
func createInvalidAlertRuleGroup() definitions.AlertRuleGroup {
return definitions.AlertRuleGroup{
Title: "invalid",
Interval: 10,
Rules: []models.AlertRule{{}},
}
}
func createTestAlertRule(title string, orgID int64) definitions.ProvisionedAlertRule { func createTestAlertRule(title string, orgID int64) definitions.ProvisionedAlertRule {
return definitions.ProvisionedAlertRule{ return definitions.ProvisionedAlertRule{
OrgID: orgID, OrgID: orgID,

View File

@ -135,7 +135,7 @@ func (f *ProvisioningApiHandler) RoutePutAlertRuleGroup(ctx *models.ReqContext)
folderUIDParam := web.Params(ctx.Req)[":FolderUID"] folderUIDParam := web.Params(ctx.Req)[":FolderUID"]
groupParam := web.Params(ctx.Req)[":Group"] groupParam := web.Params(ctx.Req)[":Group"]
// Parse Request Body // Parse Request Body
conf := apimodels.AlertRuleGroupMetadata{} conf := apimodels.AlertRuleGroup{}
if err := web.Bind(ctx.Req, &conf); err != nil { if err := web.Bind(ctx.Req, &conf); err != nil {
return response.Error(http.StatusBadRequest, "bad request data", err) return response.Error(http.StatusBadRequest, "bad request data", err)
} }

View File

@ -100,6 +100,6 @@ func (f *ProvisioningApiHandler) handleRouteGetAlertRuleGroup(ctx *models.ReqCon
return f.svc.RouteGetAlertRuleGroup(ctx, folder, group) return f.svc.RouteGetAlertRuleGroup(ctx, folder, group)
} }
func (f *ProvisioningApiHandler) handleRoutePutAlertRuleGroup(ctx *models.ReqContext, ag apimodels.AlertRuleGroupMetadata, folder, group string) response.Response { func (f *ProvisioningApiHandler) handleRoutePutAlertRuleGroup(ctx *models.ReqContext, ag apimodels.AlertRuleGroup, folder, group string) response.Response {
return f.svc.RoutePutAlertRuleGroup(ctx, ag, folder, group) return f.svc.RoutePutAlertRuleGroup(ctx, ag, folder, group)
} }

View File

@ -3513,7 +3513,6 @@
"$ref": "#/definitions/Duration" "$ref": "#/definitions/Duration"
}, },
"gettableAlert": { "gettableAlert": {
"description": "GettableAlert gettable alert",
"properties": { "properties": {
"annotations": { "annotations": {
"$ref": "#/definitions/labelSet" "$ref": "#/definitions/labelSet"
@ -3569,7 +3568,6 @@
"type": "object" "type": "object"
}, },
"gettableAlerts": { "gettableAlerts": {
"description": "GettableAlerts gettable alerts",
"items": { "items": {
"$ref": "#/definitions/gettableAlert" "$ref": "#/definitions/gettableAlert"
}, },
@ -3735,7 +3733,6 @@
"type": "array" "type": "array"
}, },
"postableSilence": { "postableSilence": {
"description": "PostableSilence postable silence",
"properties": { "properties": {
"comment": { "comment": {
"description": "comment", "description": "comment",
@ -4183,15 +4180,15 @@
"in": "body", "in": "body",
"name": "Body", "name": "Body",
"schema": { "schema": {
"$ref": "#/definitions/AlertRuleGroupMetadata" "$ref": "#/definitions/AlertRuleGroup"
} }
} }
], ],
"responses": { "responses": {
"200": { "200": {
"description": "AlertRuleGroupMetadata", "description": "AlertRuleGroup",
"schema": { "schema": {
"$ref": "#/definitions/AlertRuleGroupMetadata" "$ref": "#/definitions/AlertRuleGroup"
} }
}, },
"400": { "400": {

View File

@ -151,7 +151,7 @@ func NewAlertRule(rule models.AlertRule, provenance models.Provenance) Provision
// - application/json // - application/json
// //
// Responses: // Responses:
// 200: AlertRuleGroupMetadata // 200: AlertRuleGroup
// 400: ValidationError // 400: ValidationError
// swagger:parameters RouteGetAlertRuleGroup RoutePutAlertRuleGroup // swagger:parameters RouteGetAlertRuleGroup RoutePutAlertRuleGroup
@ -169,7 +169,7 @@ type RuleGroupPathParam struct {
// swagger:parameters RoutePutAlertRuleGroup // swagger:parameters RoutePutAlertRuleGroup
type AlertRuleGroupPayload struct { type AlertRuleGroupPayload struct {
// in:body // in:body
Body AlertRuleGroupMetadata Body AlertRuleGroup
} }
// swagger:model // swagger:model

View File

@ -3381,6 +3381,7 @@
"type": "object" "type": "object"
}, },
"alertGroup": { "alertGroup": {
"description": "AlertGroup alert group",
"properties": { "properties": {
"alerts": { "alerts": {
"description": "alerts", "description": "alerts",
@ -3568,7 +3569,6 @@
"type": "object" "type": "object"
}, },
"gettableAlerts": { "gettableAlerts": {
"description": "GettableAlerts gettable alerts",
"items": { "items": {
"$ref": "#/definitions/gettableAlert" "$ref": "#/definitions/gettableAlert"
}, },
@ -3624,7 +3624,6 @@
"type": "object" "type": "object"
}, },
"gettableSilences": { "gettableSilences": {
"description": "GettableSilences gettable silences",
"items": { "items": {
"$ref": "#/definitions/gettableSilence" "$ref": "#/definitions/gettableSilence"
}, },
@ -3735,7 +3734,6 @@
"type": "array" "type": "array"
}, },
"postableSilence": { "postableSilence": {
"description": "PostableSilence postable silence",
"properties": { "properties": {
"comment": { "comment": {
"description": "comment", "description": "comment",
@ -5923,15 +5921,15 @@
"in": "body", "in": "body",
"name": "Body", "name": "Body",
"schema": { "schema": {
"$ref": "#/definitions/AlertRuleGroupMetadata" "$ref": "#/definitions/AlertRuleGroup"
} }
} }
], ],
"responses": { "responses": {
"200": { "200": {
"description": "AlertRuleGroupMetadata", "description": "AlertRuleGroup",
"schema": { "schema": {
"$ref": "#/definitions/AlertRuleGroupMetadata" "$ref": "#/definitions/AlertRuleGroup"
} }
}, },
"400": { "400": {

View File

@ -2075,15 +2075,15 @@
"name": "Body", "name": "Body",
"in": "body", "in": "body",
"schema": { "schema": {
"$ref": "#/definitions/AlertRuleGroupMetadata" "$ref": "#/definitions/AlertRuleGroup"
} }
} }
], ],
"responses": { "responses": {
"200": { "200": {
"description": "AlertRuleGroupMetadata", "description": "AlertRuleGroup",
"schema": { "schema": {
"$ref": "#/definitions/AlertRuleGroupMetadata" "$ref": "#/definitions/AlertRuleGroup"
} }
}, },
"400": { "400": {
@ -5886,6 +5886,7 @@
} }
}, },
"alertGroup": { "alertGroup": {
"description": "AlertGroup alert group",
"type": "object", "type": "object",
"required": [ "required": [
"alerts", "alerts",
@ -6076,7 +6077,6 @@
"$ref": "#/definitions/gettableAlert" "$ref": "#/definitions/gettableAlert"
}, },
"gettableAlerts": { "gettableAlerts": {
"description": "GettableAlerts gettable alerts",
"type": "array", "type": "array",
"items": { "items": {
"$ref": "#/definitions/gettableAlert" "$ref": "#/definitions/gettableAlert"
@ -6134,7 +6134,6 @@
"$ref": "#/definitions/gettableSilence" "$ref": "#/definitions/gettableSilence"
}, },
"gettableSilences": { "gettableSilences": {
"description": "GettableSilences gettable silences",
"type": "array", "type": "array",
"items": { "items": {
"$ref": "#/definitions/gettableSilence" "$ref": "#/definitions/gettableSilence"
@ -6246,7 +6245,6 @@
} }
}, },
"postableSilence": { "postableSilence": {
"description": "PostableSilence postable silence",
"type": "object", "type": "object",
"required": [ "required": [
"comment", "comment",

View File

@ -91,15 +91,8 @@ func (service *AlertRuleService) CreateAlertRule(ctx context.Context, rule model
return errors.New("couldn't find newly created id") return errors.New("couldn't find newly created id")
} }
limitReached, err := service.quotas.CheckQuotaReached(ctx, "alert_rule", &quota.ScopeParameters{ if err = service.checkLimitsTransactionCtx(ctx, rule.OrgID, userID); err != nil {
OrgID: rule.OrgID, return err
UserID: userID,
})
if err != nil {
return fmt.Errorf("failed to check alert rule quota: %w", err)
}
if limitReached {
return models.ErrQuotaReached
} }
return service.provenanceStore.SetProvenance(ctx, &rule, rule.OrgID, provenance) return service.provenanceStore.SetProvenance(ctx, &rule, rule.OrgID, provenance)
@ -167,6 +160,109 @@ func (service *AlertRuleService) UpdateRuleGroup(ctx context.Context, orgID int6
}) })
} }
func (service *AlertRuleService) ReplaceRuleGroup(ctx context.Context, orgID int64, group definitions.AlertRuleGroup, userID int64, provenance models.Provenance) error {
if err := models.ValidateRuleGroupInterval(group.Interval, service.baseIntervalSeconds); err != nil {
return err
}
// If the provided request did not provide the rules list at all, treat it as though it does not wish to change rules.
// This is done for backwards compatibility. Requests which specify only the interval must update only the interval.
if group.Rules == nil {
listRulesQuery := models.ListAlertRulesQuery{
OrgID: orgID,
NamespaceUIDs: []string{group.FolderUID},
RuleGroup: group.Title,
}
if err := service.ruleStore.ListAlertRules(ctx, &listRulesQuery); err != nil {
return fmt.Errorf("failed to list alert rules: %w", err)
}
group.Rules = make([]models.AlertRule, 0, len(listRulesQuery.Result))
for _, r := range listRulesQuery.Result {
if r != nil {
group.Rules = append(group.Rules, *r)
}
}
}
key := models.AlertRuleGroupKey{
OrgID: orgID,
NamespaceUID: group.FolderUID,
RuleGroup: group.Title,
}
rules := make([]*models.AlertRule, len(group.Rules))
group = *syncGroupRuleFields(&group, orgID)
for i := range group.Rules {
rules = append(rules, &group.Rules[i])
}
delta, err := store.CalculateChanges(ctx, service.ruleStore, key, rules)
if err != nil {
return fmt.Errorf("failed to calculate diff for alert rules: %w", err)
}
// Refresh all calculated fields across all rules.
delta = store.UpdateCalculatedRuleFields(delta)
if len(delta.New) == 0 && len(delta.Update) == 0 && len(delta.Delete) == 0 {
return nil
}
return service.xact.InTransaction(ctx, func(ctx context.Context) error {
uids, err := service.ruleStore.InsertAlertRules(ctx, withoutNilAlertRules(delta.New))
if err != nil {
return fmt.Errorf("failed to insert alert rules: %w", err)
}
for uid := range uids {
if err := service.provenanceStore.SetProvenance(ctx, &models.AlertRule{UID: uid}, orgID, provenance); err != nil {
return err
}
}
updates := make([]store.UpdateRule, 0, len(delta.Update))
for _, update := range delta.Update {
// check that provenance is not changed in a invalid way
storedProvenance, err := service.provenanceStore.GetProvenance(ctx, update.New, orgID)
if err != nil {
return err
}
if storedProvenance != provenance && storedProvenance != models.ProvenanceNone {
return fmt.Errorf("cannot update with provided provenance '%s', needs '%s'", provenance, storedProvenance)
}
updates = append(updates, store.UpdateRule{
Existing: update.Existing,
New: *update.New,
})
}
if err = service.ruleStore.UpdateAlertRules(ctx, updates); err != nil {
return fmt.Errorf("failed to update alert rules: %w", err)
}
for _, update := range delta.Update {
if err := service.provenanceStore.SetProvenance(ctx, update.New, orgID, provenance); err != nil {
return err
}
}
for _, delete := range delta.Delete {
// check that provenance is not changed in a invalid way
storedProvenance, err := service.provenanceStore.GetProvenance(ctx, delete, orgID)
if err != nil {
return err
}
if storedProvenance != provenance && storedProvenance != models.ProvenanceNone {
return fmt.Errorf("cannot update with provided provenance '%s', needs '%s'", provenance, storedProvenance)
}
}
if err := service.deleteRules(ctx, orgID, delta.Delete...); err != nil {
return err
}
if err = service.checkLimitsTransactionCtx(ctx, orgID, userID); err != nil {
return err
}
return nil
})
}
// CreateAlertRule creates a new alert rule. This function will ignore any // CreateAlertRule creates a new alert rule. This function will ignore any
// interval that is set in the rule struct and fetch the current group interval // interval that is set in the rule struct and fetch the current group interval
// from database. // from database.
@ -220,10 +316,62 @@ func (service *AlertRuleService) DeleteAlertRule(ctx context.Context, orgID int6
return fmt.Errorf("cannot delete with provided provenance '%s', needs '%s'", provenance, storedProvenance) return fmt.Errorf("cannot delete with provided provenance '%s', needs '%s'", provenance, storedProvenance)
} }
return service.xact.InTransaction(ctx, func(ctx context.Context) error { return service.xact.InTransaction(ctx, func(ctx context.Context) error {
err := service.ruleStore.DeleteAlertRulesByUID(ctx, orgID, ruleUID) return service.deleteRules(ctx, orgID, rule)
if err != nil {
return err
}
return service.provenanceStore.DeleteProvenance(ctx, rule, rule.OrgID)
}) })
} }
// checkLimitsTransactionCtx checks whether the current transaction (as identified by the ctx) breaches configured alert rule limits.
func (service *AlertRuleService) checkLimitsTransactionCtx(ctx context.Context, orgID, userID int64) error {
limitReached, err := service.quotas.CheckQuotaReached(ctx, "alert_rule", &quota.ScopeParameters{
OrgID: orgID,
UserID: userID,
})
if err != nil {
return fmt.Errorf("failed to check alert rule quota: %w", err)
}
if limitReached {
return models.ErrQuotaReached
}
return nil
}
// deleteRules deletes a set of target rules and associated data, while checking for database consistency.
func (service *AlertRuleService) deleteRules(ctx context.Context, orgID int64, targets ...*models.AlertRule) error {
uids := make([]string, 0, len(targets))
for _, tgt := range targets {
if tgt != nil {
uids = append(uids, tgt.UID)
}
}
if err := service.ruleStore.DeleteAlertRulesByUID(ctx, orgID, uids...); err != nil {
return err
}
for _, uid := range uids {
if err := service.provenanceStore.DeleteProvenance(ctx, &models.AlertRule{UID: uid}, orgID); err != nil {
// We failed to clean up the record, but this doesn't break things. Log it and move on.
service.log.Warn("failed to delete provenance record for rule: %w", err)
}
}
return nil
}
// syncRuleGroupFields synchronizes calculated fields across multiple rules in a group.
func syncGroupRuleFields(group *definitions.AlertRuleGroup, orgID int64) *definitions.AlertRuleGroup {
for i := range group.Rules {
group.Rules[i].IntervalSeconds = group.Interval
group.Rules[i].RuleGroup = group.Title
group.Rules[i].NamespaceUID = group.FolderUID
group.Rules[i].OrgID = orgID
}
return group
}
func withoutNilAlertRules(ptrs []*models.AlertRule) []models.AlertRule {
result := make([]models.AlertRule, 0, len(ptrs))
for _, ptr := range ptrs {
if ptr != nil {
result = append(result, *ptr)
}
}
return result
}

View File

@ -7,6 +7,7 @@ import (
"time" "time"
"github.com/grafana/grafana/pkg/infra/log" "github.com/grafana/grafana/pkg/infra/log"
"github.com/grafana/grafana/pkg/services/ngalert/api/tooling/definitions"
"github.com/grafana/grafana/pkg/services/ngalert/models" "github.com/grafana/grafana/pkg/services/ngalert/models"
"github.com/grafana/grafana/pkg/services/ngalert/store" "github.com/grafana/grafana/pkg/services/ngalert/store"
"github.com/grafana/grafana/pkg/services/sqlstore" "github.com/grafana/grafana/pkg/services/sqlstore"
@ -35,6 +36,22 @@ func TestAlertRuleService(t *testing.T) {
require.Equal(t, models.ProvenanceAPI, provenance) require.Equal(t, models.ProvenanceAPI, provenance)
}) })
t.Run("group creation should set the right provenance", func(t *testing.T) {
var orgID int64 = 1
group := createDummyGroup("group-test-1", orgID)
err := ruleService.ReplaceRuleGroup(context.Background(), orgID, group, 0, models.ProvenanceAPI)
require.NoError(t, err)
readGroup, err := ruleService.GetRuleGroup(context.Background(), orgID, "my-namespace", "group-test-1")
require.NoError(t, err)
require.NotEmpty(t, readGroup.Rules)
for _, rule := range readGroup.Rules {
_, provenance, err := ruleService.GetAlertRule(context.Background(), orgID, rule.UID)
require.NoError(t, err)
require.Equal(t, models.ProvenanceAPI, provenance)
}
})
t.Run("alert rule group should be updated correctly", func(t *testing.T) { t.Run("alert rule group should be updated correctly", func(t *testing.T) {
var orgID int64 = 1 var orgID int64 = 1
rule := dummyRule("test#3", orgID) rule := dummyRule("test#3", orgID)
@ -52,6 +69,22 @@ func TestAlertRuleService(t *testing.T) {
require.Equal(t, interval, rule.IntervalSeconds) require.Equal(t, interval, rule.IntervalSeconds)
}) })
t.Run("group creation should propagate group title correctly", func(t *testing.T) {
var orgID int64 = 1
group := createDummyGroup("group-test-3", orgID)
group.Rules[0].RuleGroup = "something different"
err := ruleService.ReplaceRuleGroup(context.Background(), orgID, group, 0, models.ProvenanceAPI)
require.NoError(t, err)
readGroup, err := ruleService.GetRuleGroup(context.Background(), orgID, "my-namespace", "group-test-3")
require.NoError(t, err)
require.NotEmpty(t, readGroup.Rules)
for _, rule := range readGroup.Rules {
require.Equal(t, "group-test-3", rule.RuleGroup)
}
})
t.Run("alert rule should get interval from existing rule group", func(t *testing.T) { t.Run("alert rule should get interval from existing rule group", func(t *testing.T) {
var orgID int64 = 1 var orgID int64 = 1
rule := dummyRule("test#4", orgID) rule := dummyRule("test#4", orgID)
@ -70,7 +103,7 @@ func TestAlertRuleService(t *testing.T) {
require.Equal(t, interval, rule.IntervalSeconds) require.Equal(t, interval, rule.IntervalSeconds)
}) })
t.Run("updating a rule group should bump the version number", func(t *testing.T) { t.Run("updating a rule group's top level fields should bump the version number", func(t *testing.T) {
const ( const (
orgID = 123 orgID = 123
namespaceUID = "abc" namespaceUID = "abc"
@ -99,6 +132,26 @@ func TestAlertRuleService(t *testing.T) {
require.Equal(t, newInterval, rule.IntervalSeconds) require.Equal(t, newInterval, rule.IntervalSeconds)
}) })
t.Run("updating a group by updating a rule should bump that rule's data and version number", func(t *testing.T) {
var orgID int64 = 1
group := createDummyGroup("group-test-5", orgID)
err := ruleService.ReplaceRuleGroup(context.Background(), orgID, group, 0, models.ProvenanceAPI)
require.NoError(t, err)
updatedGroup, err := ruleService.GetRuleGroup(context.Background(), orgID, "my-namespace", "group-test-5")
require.NoError(t, err)
updatedGroup.Rules[0].Title = "some-other-title-asdf"
err = ruleService.ReplaceRuleGroup(context.Background(), orgID, updatedGroup, 0, models.ProvenanceAPI)
require.NoError(t, err)
readGroup, err := ruleService.GetRuleGroup(context.Background(), orgID, "my-namespace", "group-test-5")
require.NoError(t, err)
require.NotEmpty(t, readGroup.Rules)
require.Len(t, readGroup.Rules, 1)
require.Equal(t, "some-other-title-asdf", readGroup.Rules[0].Title)
require.Equal(t, int64(2), readGroup.Rules[0].Version)
})
t.Run("alert rule provenace should be correctly checked", func(t *testing.T) { t.Run("alert rule provenace should be correctly checked", func(t *testing.T) {
tests := []struct { tests := []struct {
name string name string
@ -160,6 +213,68 @@ func TestAlertRuleService(t *testing.T) {
} }
}) })
t.Run("alert rule provenace should be correctly checked when writing groups", func(t *testing.T) {
tests := []struct {
name string
from models.Provenance
to models.Provenance
errNil bool
}{
{
name: "should be able to update from provenance none to api",
from: models.ProvenanceNone,
to: models.ProvenanceAPI,
errNil: true,
},
{
name: "should be able to update from provenance none to file",
from: models.ProvenanceNone,
to: models.ProvenanceFile,
errNil: true,
},
{
name: "should not be able to update from provenance api to file",
from: models.ProvenanceAPI,
to: models.ProvenanceFile,
errNil: false,
},
{
name: "should not be able to update from provenance api to none",
from: models.ProvenanceAPI,
to: models.ProvenanceNone,
errNil: false,
},
{
name: "should not be able to update from provenance file to api",
from: models.ProvenanceFile,
to: models.ProvenanceAPI,
errNil: false,
},
{
name: "should not be able to update from provenance file to none",
from: models.ProvenanceFile,
to: models.ProvenanceNone,
errNil: false,
},
}
for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
var orgID int64 = 1
group := createDummyGroup(t.Name(), orgID)
err := ruleService.ReplaceRuleGroup(context.Background(), 1, group, 0, test.from)
require.NoError(t, err)
group.Rules[0].Title = t.Name()
err = ruleService.ReplaceRuleGroup(context.Background(), 1, group, 0, test.to)
if test.errNil {
require.NoError(t, err)
} else {
require.Error(t, err)
}
})
}
})
t.Run("quota met causes create to be rejected", func(t *testing.T) { t.Run("quota met causes create to be rejected", func(t *testing.T) {
ruleService := createAlertRuleService(t) ruleService := createAlertRuleService(t)
checker := &MockQuotaChecker{} checker := &MockQuotaChecker{}
@ -170,6 +285,18 @@ func TestAlertRuleService(t *testing.T) {
require.ErrorIs(t, err, models.ErrQuotaReached) require.ErrorIs(t, err, models.ErrQuotaReached)
}) })
t.Run("quota met causes group write to be rejected", func(t *testing.T) {
ruleService := createAlertRuleService(t)
checker := &MockQuotaChecker{}
checker.EXPECT().LimitExceeded()
ruleService.quotas = checker
group := createDummyGroup("quota-reached", 1)
err := ruleService.ReplaceRuleGroup(context.Background(), 1, group, 0, models.ProvenanceAPI)
require.ErrorIs(t, err, models.ErrQuotaReached)
})
} }
func createAlertRuleService(t *testing.T) AlertRuleService { func createAlertRuleService(t *testing.T) AlertRuleService {
@ -180,6 +307,7 @@ func createAlertRuleService(t *testing.T) AlertRuleService {
Cfg: setting.UnifiedAlertingSettings{ Cfg: setting.UnifiedAlertingSettings{
BaseInterval: time.Second * 10, BaseInterval: time.Second * 10,
}, },
Logger: log.NewNopLogger(),
} }
quotas := MockQuotaChecker{} quotas := MockQuotaChecker{}
quotas.EXPECT().LimitOK() quotas.EXPECT().LimitOK()
@ -195,6 +323,10 @@ func createAlertRuleService(t *testing.T) AlertRuleService {
} }
func dummyRule(title string, orgID int64) models.AlertRule { func dummyRule(title string, orgID int64) models.AlertRule {
return createTestRule(title, "my-cool-group", orgID)
}
func createTestRule(title string, groupTitle string, orgID int64) models.AlertRule {
return models.AlertRule{ return models.AlertRule{
OrgID: orgID, OrgID: orgID,
Title: title, Title: title,
@ -212,9 +344,21 @@ func dummyRule(title string, orgID int64) models.AlertRule {
}, },
}, },
}, },
RuleGroup: "my-cool-group", NamespaceUID: "my-namespace",
RuleGroup: groupTitle,
For: time.Second * 60, For: time.Second * 60,
NoDataState: models.OK, NoDataState: models.OK,
ExecErrState: models.OkErrState, ExecErrState: models.OkErrState,
} }
} }
func createDummyGroup(title string, orgID int64) definitions.AlertRuleGroup {
return definitions.AlertRuleGroup{
Title: title,
Interval: 60,
FolderUID: "my-namespace",
Rules: []models.AlertRule{
dummyRule(title+"-"+"rule-1", orgID),
},
}
}

View File

@ -37,6 +37,7 @@ type RuleStore interface {
InsertAlertRules(ctx context.Context, rule []models.AlertRule) (map[string]int64, error) InsertAlertRules(ctx context.Context, rule []models.AlertRule) (map[string]int64, error)
UpdateAlertRules(ctx context.Context, rule []store.UpdateRule) error UpdateAlertRules(ctx context.Context, rule []store.UpdateRule) error
DeleteAlertRulesByUID(ctx context.Context, orgID int64, ruleUID ...string) error DeleteAlertRulesByUID(ctx context.Context, orgID int64, ruleUID ...string) error
GetAlertRulesGroupByRuleUID(ctx context.Context, query *models.GetAlertRulesGroupByRuleUIDQuery) error
} }
// QuotaChecker represents the ability to evaluate whether quotas are met. // QuotaChecker represents the ability to evaluate whether quotas are met.

View File

@ -62,6 +62,9 @@ func CalculateChanges(ctx context.Context, ruleReader RuleReader, groupKey model
var toUpdate []RuleDelta var toUpdate []RuleDelta
loadedRulesByUID := map[string]*models.AlertRule{} // auxiliary cache to avoid unnecessary queries if there are multiple moves from the same group loadedRulesByUID := map[string]*models.AlertRule{} // auxiliary cache to avoid unnecessary queries if there are multiple moves from the same group
for _, r := range submittedRules { for _, r := range submittedRules {
if r == nil {
continue
}
var existing *models.AlertRule = nil var existing *models.AlertRule = nil
if r.UID != "" { if r.UID != "" {
if existingGroupRule, ok := existingGroupRulesUIDs[r.UID]; ok { if existingGroupRule, ok := existingGroupRulesUIDs[r.UID]; ok {