diff --git a/pkg/services/ngalert/models/testing.go b/pkg/services/ngalert/models/testing.go index 20c0f8704ff..0dcc4ecc546 100644 --- a/pkg/services/ngalert/models/testing.go +++ b/pkg/services/ngalert/models/testing.go @@ -16,6 +16,7 @@ import ( "github.com/prometheus/common/model" "github.com/stretchr/testify/require" + alertingModels "github.com/grafana/alerting/models" "github.com/grafana/grafana/pkg/expr" "github.com/grafana/grafana/pkg/services/datasources" "github.com/grafana/grafana/pkg/services/folder" @@ -1054,6 +1055,29 @@ func (n SilenceMutators) WithMatcher(name, value string, matchType labels.MatchT s.Silence.Matchers = append(s.Silence.Matchers, &m) } } +func (n SilenceMutators) WithRuleUID(value string) Mutator[Silence] { + return func(s *Silence) { + name := alertingModels.RuleUIDLabel + m := amv2.Matcher{ + Name: &name, + Value: &value, + IsRegex: util.Pointer(false), + IsEqual: util.Pointer(true), + } + for _, matcher := range s.Silence.Matchers { + if isRuleUIDMatcher(*matcher) { + *matcher = m + return + } + } + s.Silence.Matchers = append(s.Silence.Matchers, &m) + } +} +func (n SilenceMutators) Expired() Mutator[Silence] { + return func(s *Silence) { + s.EndsAt = util.Pointer(strfmt.DateTime(time.Now().Add(-time.Minute))) + } +} func (n SilenceMutators) WithEmptyId() Mutator[Silence] { return func(s *Silence) { diff --git a/pkg/services/ngalert/notifier/silence_svc.go b/pkg/services/ngalert/notifier/silence_svc.go index f41b3a9674c..fb04dfcb5e2 100644 --- a/pkg/services/ngalert/notifier/silence_svc.go +++ b/pkg/services/ngalert/notifier/silence_svc.go @@ -5,6 +5,7 @@ import ( "golang.org/x/exp/maps" + alertingModels "github.com/grafana/alerting/models" "github.com/grafana/grafana/pkg/infra/log" "github.com/grafana/grafana/pkg/services/auth/identity" "github.com/grafana/grafana/pkg/services/ngalert/accesscontrol" @@ -115,6 +116,15 @@ func (s *SilenceService) UpdateSilence(ctx context.Context, user identity.Reques return "", err } + existing, err := s.store.GetSilence(ctx, user.GetOrgID(), *ps.ID) + if err != nil { + return "", err + } + + if err := validateSilenceUpdate(existing, ps); err != nil { + return "", err + } + silenceId, err := s.store.UpdateSilence(ctx, user.GetOrgID(), ps) if err != nil { return "", err @@ -226,3 +236,21 @@ func (s *SilenceService) WithRuleMetadata(ctx context.Context, user identity.Req return nil } + +// validateSilenceUpdate validates the diff between an existing silence and a new silence. Currently, this is use to +// prevent changing the rule UID matcher. +// Alternatively, we could check WRITE permission on the old silence followed by CREATE permission on the new silence +// if the rule folder is different. +func validateSilenceUpdate(existing *models.Silence, new models.Silence) error { + existingRuleUID := existing.GetRuleUID() + newRuleUID := new.GetRuleUID() + if existingRuleUID == nil || newRuleUID == nil { + if existingRuleUID != newRuleUID { + return WithPublicError(ErrSilencesBadRequest.Errorf("Silence rule matcher '%s' cannot be updated, please create a new silence", alertingModels.RuleUIDLabel)) + } + } else if *existingRuleUID != *newRuleUID { + return WithPublicError(ErrSilencesBadRequest.Errorf("Silence rule matcher '%s' cannot be updated, please create a new silence", alertingModels.RuleUIDLabel)) + } + + return nil +} diff --git a/pkg/services/ngalert/notifier/silence_svc_test.go b/pkg/services/ngalert/notifier/silence_svc_test.go index 96af8bfe2d6..f5042bf5859 100644 --- a/pkg/services/ngalert/notifier/silence_svc_test.go +++ b/pkg/services/ngalert/notifier/silence_svc_test.go @@ -161,3 +161,74 @@ func TestWithRuleMetadata(t *testing.T) { assert.Equal(t, "folder1", ruleAuthz.Calls[0].Arguments[2].(accesscontrol.Namespaced).GetNamespaceUID()) }) } + +func TestUpdateSilence(t *testing.T) { + user := ac.BackgroundUser("test", 1, org.RoleNone, nil) + testCases := []struct { + name string + existing func() models.Silence + mutators []models.Mutator[models.Silence] + errContains string + }{ + { + name: "Updates to general silences allowed", + existing: models.SilenceGen(), + mutators: []models.Mutator[models.Silence]{ + models.SilenceMuts.Expired(), + }, + errContains: "", // No Error. + }, + { + name: "Updates to general silences that add rule_uid matcher error", + existing: models.SilenceGen(), + mutators: []models.Mutator[models.Silence]{ + models.SilenceMuts.WithRuleUID("rule1"), + }, + errContains: alertingmodels.RuleUIDLabel, // Mention matcher in error message. + }, + { + name: "Updates that change rule_uid matcher error", + existing: models.SilenceGen(models.SilenceMuts.WithRuleUID("rule1")), + mutators: []models.Mutator[models.Silence]{ + models.SilenceMuts.WithRuleUID("rule2"), + }, + errContains: alertingmodels.RuleUIDLabel, // Mention matcher in error message. + }, + { + name: "Updates that don't change rule_uid matcher are allowed", + existing: models.SilenceGen(models.SilenceMuts.WithRuleUID("rule1")), + mutators: []models.Mutator[models.Silence]{ + models.SilenceMuts.Expired(), + }, + errContains: "", // No Error. + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + authz := fakes.FakeSilenceService{} + authz.AuthorizeUpdateSilenceFunc = func(ctx context.Context, user identity.Requester, silence *models.Silence) error { + return nil + } + silence := tc.existing() + silenceStore := ngfakes.FakeSilenceStore{ + Silences: map[string]*models.Silence{ + *silence.ID: &silence, + }, + } + svc := SilenceService{ + authz: &authz, + store: &silenceStore, + } + + modified := models.CopySilenceWith(silence, tc.mutators...) + _, err := svc.UpdateSilence(context.Background(), user, modified) + if tc.errContains != "" { + assert.Error(t, err) + assert.ErrorContains(t, err, tc.errContains) + } else { + require.NoError(t, err) + } + }) + } +} diff --git a/pkg/services/ngalert/tests/fakes/silences.go b/pkg/services/ngalert/tests/fakes/silences.go new file mode 100644 index 00000000000..e43fd9f0fb7 --- /dev/null +++ b/pkg/services/ngalert/tests/fakes/silences.go @@ -0,0 +1,62 @@ +package fakes + +import ( + "context" + + "golang.org/x/exp/maps" + + alertingNotify "github.com/grafana/alerting/notify" + "github.com/grafana/grafana/pkg/services/ngalert/models" + "github.com/grafana/grafana/pkg/util" +) + +type Call struct { + MethodName string + Arguments []interface{} +} + +type FakeSilenceStore struct { + Silences map[string]*models.Silence + RuleUIDFolders map[string]string + + RecordedOps []GenericRecordedQuery +} + +func (s *FakeSilenceStore) ListSilences(ctx context.Context, orgID int64, filter []string) ([]*models.Silence, error) { + s.RecordedOps = append(s.RecordedOps, GenericRecordedQuery{"ListSilences", []interface{}{ctx, orgID, filter}}) + return maps.Values(s.Silences), nil +} + +func (s *FakeSilenceStore) GetSilence(ctx context.Context, orgID int64, id string) (*models.Silence, error) { + s.RecordedOps = append(s.RecordedOps, GenericRecordedQuery{"GetSilence", []interface{}{ctx, orgID, id}}) + if silence, ok := s.Silences[id]; ok { + return silence, nil + } + return nil, alertingNotify.ErrSilenceNotFound +} + +func (s *FakeSilenceStore) CreateSilence(ctx context.Context, orgID int64, ps models.Silence) (string, error) { + s.RecordedOps = append(s.RecordedOps, GenericRecordedQuery{"CreateSilence", []interface{}{ctx, orgID, ps}}) + uid := util.GenerateShortUID() + ps.ID = &uid + s.Silences[uid] = &ps + return uid, nil +} + +func (s *FakeSilenceStore) UpdateSilence(ctx context.Context, orgID int64, ps models.Silence) (string, error) { + s.RecordedOps = append(s.RecordedOps, GenericRecordedQuery{"UpdateSilence", []interface{}{ctx, orgID, ps}}) + if _, ok := s.Silences[*ps.ID]; !ok { + return "", alertingNotify.ErrSilenceNotFound + } + s.Silences[*ps.ID] = &ps + return *ps.ID, nil +} + +func (s *FakeSilenceStore) DeleteSilence(ctx context.Context, orgID int64, id string) error { + s.RecordedOps = append(s.RecordedOps, GenericRecordedQuery{"DeleteSilence", []interface{}{ctx, orgID, id}}) + if _, ok := s.Silences[id]; !ok { + return alertingNotify.ErrSilenceNotFound + } + delete(s.Silences, id) + return nil +}