Alerting: Prevent updating rule uid matcher for silences (#88519)

Prevents updating the `__alert_rule_uid__` equality matcher (used for rule-specific silences) on existing silences

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) {

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
+}

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)
+			}
+		})
+	}
+}

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
+}