diff --git a/pkg/registry/apis/alerting/notifications/timeinterval/conversions.go b/pkg/registry/apis/alerting/notifications/timeinterval/conversions.go index ae8ed546c98..93845cb37c7 100644 --- a/pkg/registry/apis/alerting/notifications/timeinterval/conversions.go +++ b/pkg/registry/apis/alerting/notifications/timeinterval/conversions.go @@ -2,8 +2,6 @@ package timeinterval import ( "encoding/json" - "fmt" - "hash/fnv" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/fields" @@ -14,12 +12,6 @@ import ( "github.com/grafana/grafana/pkg/services/ngalert/api/tooling/definitions" ) -func getIntervalUID(t definitions.MuteTimeInterval) string { - sum := fnv.New64() - _, _ = sum.Write([]byte(t.Name)) - return fmt.Sprintf("%016x", sum.Sum64()) -} - func convertToK8sResources(orgID int64, intervals []definitions.MuteTimeInterval, namespacer request.NamespaceMapper, selector fields.Selector) (*model.TimeIntervalList, error) { data, err := json.Marshal(intervals) if err != nil { @@ -59,12 +51,11 @@ func convertToK8sResource(orgID int64, interval definitions.MuteTimeInterval, na } func buildTimeInterval(orgID int64, interval definitions.MuteTimeInterval, spec model.TimeIntervalSpec, namespacer request.NamespaceMapper) model.TimeInterval { - uid := getIntervalUID(interval) // TODO replace to stable UID when we switch to normal storage i := model.TimeInterval{ TypeMeta: resourceInfo.TypeMeta(), ObjectMeta: metav1.ObjectMeta{ - UID: types.UID(uid), // TODO This is needed to make PATCH work - Name: uid, // TODO replace to stable UID when we switch to normal storage + UID: types.UID(interval.UID), // TODO This is needed to make PATCH work + Name: interval.UID, // TODO replace to stable UID when we switch to normal storage Namespace: namespacer(orgID), ResourceVersion: interval.Version, }, @@ -85,6 +76,7 @@ func convertToDomainModel(interval *model.TimeInterval) (definitions.MuteTimeInt return definitions.MuteTimeInterval{}, err } result.Version = interval.ResourceVersion + result.UID = interval.ObjectMeta.Name err = result.Validate() if err != nil { return definitions.MuteTimeInterval{}, err diff --git a/pkg/registry/apis/alerting/notifications/timeinterval/legacy_storage.go b/pkg/registry/apis/alerting/notifications/timeinterval/legacy_storage.go index ac2137a2cb0..ebfeb1a8f5b 100644 --- a/pkg/registry/apis/alerting/notifications/timeinterval/legacy_storage.go +++ b/pkg/registry/apis/alerting/notifications/timeinterval/legacy_storage.go @@ -25,10 +25,9 @@ var resourceInfo = notifications.TimeIntervalResourceInfo type TimeIntervalService interface { GetMuteTimings(ctx context.Context, orgID int64) ([]definitions.MuteTimeInterval, error) - GetMuteTiming(ctx context.Context, name string, orgID int64) (definitions.MuteTimeInterval, error) CreateMuteTiming(ctx context.Context, mt definitions.MuteTimeInterval, orgID int64) (definitions.MuteTimeInterval, error) UpdateMuteTiming(ctx context.Context, mt definitions.MuteTimeInterval, orgID int64) (definitions.MuteTimeInterval, error) - DeleteMuteTiming(ctx context.Context, name string, orgID int64, provenance definitions.Provenance, version string) error + DeleteMuteTiming(ctx context.Context, nameOrUid string, orgID int64, provenance definitions.Provenance, version string) error } type legacyStorage struct { @@ -85,7 +84,7 @@ func (s *legacyStorage) Get(ctx context.Context, uid string, _ *metav1.GetOption } for _, mt := range timings { - if getIntervalUID(mt) == uid { + if mt.UID == uid { return convertToK8sResource(info.OrgID, mt, s.namespacer) } } @@ -159,7 +158,7 @@ func (s *legacyStorage) Update(ctx context.Context, return old, false, err } - if p.ObjectMeta.Name != getIntervalUID(interval) { + if p.ObjectMeta.Name != interval.UID { return nil, false, errors.NewBadRequest("title of cannot be changed. Consider creating a new resource.") } @@ -196,8 +195,8 @@ func (s *legacyStorage) Delete(ctx context.Context, uid string, deleteValidation return nil, false, fmt.Errorf("expected time-interval but got %s", old.GetObjectKind().GroupVersionKind()) } - err = s.service.DeleteMuteTiming(ctx, p.Spec.Name, info.OrgID, definitions.Provenance(models.ProvenanceNone), version) // TODO add support for dry-run option - return old, false, err // false - will be deleted async + err = s.service.DeleteMuteTiming(ctx, p.ObjectMeta.Name, info.OrgID, definitions.Provenance(models.ProvenanceNone), version) // TODO add support for dry-run option + return old, false, err // false - will be deleted async } func (s *legacyStorage) DeleteCollection(ctx context.Context, deleteValidation rest.ValidateObjectFunc, options *metav1.DeleteOptions, listOptions *internalversion.ListOptions) (runtime.Object, error) { diff --git a/pkg/services/ngalert/api/api_provisioning.go b/pkg/services/ngalert/api/api_provisioning.go index 2736e25fd37..dc6567100c2 100644 --- a/pkg/services/ngalert/api/api_provisioning.go +++ b/pkg/services/ngalert/api/api_provisioning.go @@ -298,7 +298,14 @@ func (srv *ProvisioningSrv) RoutePostMuteTiming(c *contextmodel.ReqContext, mt d } func (srv *ProvisioningSrv) RoutePutMuteTiming(c *contextmodel.ReqContext, mt definitions.MuteTimeInterval, name string) response.Response { - mt.Name = name + // if body does not specify name, assume that the path contains the name + if mt.Name == "" { + mt.Name = name + } + // if body contains a name, and it's different from the one in the path, assume the latter to be UID + if mt.Name != name { + mt.UID = name + } mt.Provenance = determineProvenance(c) updated, err := srv.muteTimings.UpdateMuteTiming(c.Req.Context(), mt, c.SignedInUser.GetOrgID()) if err != nil { diff --git a/pkg/services/ngalert/api/tooling/definitions/provisioning_mute_timings.go b/pkg/services/ngalert/api/tooling/definitions/provisioning_mute_timings.go index 75c2f353f86..3aa945af048 100644 --- a/pkg/services/ngalert/api/tooling/definitions/provisioning_mute_timings.go +++ b/pkg/services/ngalert/api/tooling/definitions/provisioning_mute_timings.go @@ -117,6 +117,7 @@ type MuteTimingHeaders struct { // swagger:model type MuteTimeInterval struct { + UID string `json:"-" yaml:"-"` config.MuteTimeInterval `json:",inline" yaml:",inline"` Version string `json:"version,omitempty"` Provenance Provenance `json:"provenance,omitempty"` diff --git a/pkg/services/ngalert/provisioning/mute_timings.go b/pkg/services/ngalert/provisioning/mute_timings.go index 5bc0b9692c3..72d4ca1a34c 100644 --- a/pkg/services/ngalert/provisioning/mute_timings.go +++ b/pkg/services/ngalert/provisioning/mute_timings.go @@ -2,7 +2,9 @@ package provisioning import ( "context" + "encoding/base64" "encoding/binary" + "errors" "fmt" "hash/fnv" "slices" @@ -57,7 +59,11 @@ func (svc *MuteTimingService) GetMuteTimings(ctx context.Context, orgID int64) ( result := make([]definitions.MuteTimeInterval, 0, len(rev.cfg.AlertmanagerConfig.MuteTimeIntervals)) for _, interval := range rev.cfg.AlertmanagerConfig.MuteTimeIntervals { version := calculateMuteTimeIntervalFingerprint(interval) - def := definitions.MuteTimeInterval{MuteTimeInterval: interval, Version: version} + def := definitions.MuteTimeInterval{ + UID: getIntervalUID(interval), + MuteTimeInterval: interval, + Version: version, + } if prov, ok := provenances[def.ResourceID()]; ok { def.Provenance = definitions.Provenance(prov) } @@ -67,18 +73,25 @@ func (svc *MuteTimingService) GetMuteTimings(ctx context.Context, orgID int64) ( } // GetMuteTiming returns a mute timing by name -func (svc *MuteTimingService) GetMuteTiming(ctx context.Context, name string, orgID int64) (definitions.MuteTimeInterval, error) { +func (svc *MuteTimingService) GetMuteTiming(ctx context.Context, nameOrUID string, orgID int64) (definitions.MuteTimeInterval, error) { rev, err := svc.configStore.Get(ctx, orgID) if err != nil { return definitions.MuteTimeInterval{}, err } - mt, idx := getMuteTiming(rev, name) + mt, idx := getMuteTimingByName(rev, nameOrUID) + if idx == -1 { + name, err := uidToName(nameOrUID) + if err == nil { + mt, idx = getMuteTimingByName(rev, name) + } + } if idx == -1 { return definitions.MuteTimeInterval{}, ErrTimeIntervalNotFound.Errorf("") } result := definitions.MuteTimeInterval{ + UID: getIntervalUID(mt), MuteTimeInterval: mt, Version: calculateMuteTimeIntervalFingerprint(mt), } @@ -102,7 +115,7 @@ func (svc *MuteTimingService) CreateMuteTiming(ctx context.Context, mt definitio return definitions.MuteTimeInterval{}, err } - _, idx := getMuteTiming(revision, mt.Name) + _, idx := getMuteTimingByName(revision, mt.Name) if idx != -1 { return definitions.MuteTimeInterval{}, ErrTimeIntervalExists.Errorf("") } @@ -118,6 +131,7 @@ func (svc *MuteTimingService) CreateMuteTiming(ctx context.Context, mt definitio return definitions.MuteTimeInterval{}, err } return definitions.MuteTimeInterval{ + UID: getIntervalUID(mt.MuteTimeInterval), MuteTimeInterval: mt.MuteTimeInterval, Version: calculateMuteTimeIntervalFingerprint(mt.MuteTimeInterval), Provenance: mt.Provenance, @@ -130,8 +144,33 @@ func (svc *MuteTimingService) UpdateMuteTiming(ctx context.Context, mt definitio return definitions.MuteTimeInterval{}, MakeErrTimeIntervalInvalid(err) } + revision, err := svc.configStore.Get(ctx, orgID) + if err != nil { + return definitions.MuteTimeInterval{}, err + } + + var old config.MuteTimeInterval + var idx = -1 + if mt.UID != "" { + name, err := uidToName(mt.UID) + if err == nil { + old, idx = getMuteTimingByName(revision, name) + } + } else { + old, idx = getMuteTimingByName(revision, mt.Name) + } + if idx == -1 { + return definitions.MuteTimeInterval{}, ErrTimeIntervalNotFound.Errorf("") + } + + // check optimistic concurrency + err = svc.checkOptimisticConcurrency(old, models.Provenance(mt.Provenance), mt.Version, "update") + if err != nil { + return definitions.MuteTimeInterval{}, err + } + // check that provenance is not changed in an invalid way - storedProvenance, err := svc.provenanceStore.GetProvenance(ctx, &mt, orgID) + storedProvenance, err := svc.provenanceStore.GetProvenance(ctx, &definitions.MuteTimeInterval{MuteTimeInterval: old}, orgID) if err != nil { return definitions.MuteTimeInterval{}, err } @@ -139,23 +178,10 @@ func (svc *MuteTimingService) UpdateMuteTiming(ctx context.Context, mt definitio return definitions.MuteTimeInterval{}, err } - revision, err := svc.configStore.Get(ctx, orgID) - if err != nil { - return definitions.MuteTimeInterval{}, err - } - - if revision.cfg.AlertmanagerConfig.MuteTimeIntervals == nil { - return definitions.MuteTimeInterval{}, nil - } - - old, idx := getMuteTiming(revision, mt.Name) - if idx == -1 { - return definitions.MuteTimeInterval{}, ErrTimeIntervalNotFound.Errorf("") - } - - err = svc.checkOptimisticConcurrency(old, models.Provenance(mt.Provenance), mt.Version, "update") - if err != nil { - return definitions.MuteTimeInterval{}, err + // if the name of the time interval changed + if old.Name != mt.Name { + // TODO implement support for renaming. + return definitions.MuteTimeInterval{}, MakeErrTimeIntervalInvalid(errors.New("name change is not allowed")) } revision.cfg.AlertmanagerConfig.MuteTimeIntervals[idx] = mt.MuteTimeInterval @@ -171,6 +197,7 @@ func (svc *MuteTimingService) UpdateMuteTiming(ctx context.Context, mt definitio return definitions.MuteTimeInterval{}, err } return definitions.MuteTimeInterval{ + UID: getIntervalUID(mt.MuteTimeInterval), MuteTimeInterval: mt.MuteTimeInterval, Version: calculateMuteTimeIntervalFingerprint(mt.MuteTimeInterval), Provenance: mt.Provenance, @@ -178,15 +205,21 @@ func (svc *MuteTimingService) UpdateMuteTiming(ctx context.Context, mt definitio } // DeleteMuteTiming deletes the mute timing with the given name in the given org. If the mute timing does not exist, no error is returned. -func (svc *MuteTimingService) DeleteMuteTiming(ctx context.Context, name string, orgID int64, provenance definitions.Provenance, version string) error { +func (svc *MuteTimingService) DeleteMuteTiming(ctx context.Context, nameOrUID string, orgID int64, provenance definitions.Provenance, version string) error { revision, err := svc.configStore.Get(ctx, orgID) if err != nil { return err } - existing, idx := getMuteTiming(revision, name) + existing, idx := getMuteTimingByName(revision, nameOrUID) if idx == -1 { - svc.log.FromContext(ctx).Debug("Time interval was not found. Skip deleting", "name", name) + name, err := uidToName(nameOrUID) + if err == nil { + existing, idx = getMuteTimingByName(revision, name) + } + } + if idx == -1 { + svc.log.FromContext(ctx).Debug("Time interval was not found. Skip deleting", "name", nameOrUID) return nil } @@ -200,7 +233,7 @@ func (svc *MuteTimingService) DeleteMuteTiming(ctx context.Context, name string, return err } - if isMuteTimeInUseInRoutes(name, revision.cfg.AlertmanagerConfig.Route) { + if isMuteTimeInUseInRoutes(existing.Name, revision.cfg.AlertmanagerConfig.Route) { ns, _ := svc.ruleNotificationsStore.ListNotificationSettings(ctx, models.ListNotificationSettingsQuery{OrgID: orgID, TimeIntervalName: existing.Name}) // ignore error here because it's not important return MakeErrTimeIntervalInUse(true, maps.Keys(ns)) @@ -243,7 +276,7 @@ func isMuteTimeInUseInRoutes(name string, route *definitions.Route) bool { return false } -func getMuteTiming(rev *cfgRevision, name string) (config.MuteTimeInterval, int) { +func getMuteTimingByName(rev *cfgRevision, name string) (config.MuteTimeInterval, int) { idx := slices.IndexFunc(rev.cfg.AlertmanagerConfig.MuteTimeIntervals, func(interval config.MuteTimeInterval) bool { return interval.Name == name }) @@ -322,3 +355,15 @@ func (svc *MuteTimingService) checkOptimisticConcurrency(current config.MuteTime } return nil } + +func getIntervalUID(t config.MuteTimeInterval) string { + return base64.RawURLEncoding.EncodeToString([]byte(t.Name)) +} + +func uidToName(uid string) (string, error) { + data, err := base64.RawURLEncoding.DecodeString(uid) + if err != nil { + return uid, err + } + return string(data), nil +} diff --git a/pkg/services/ngalert/provisioning/mute_timings_test.go b/pkg/services/ngalert/provisioning/mute_timings_test.go index 5168fcd3189..dd4154d9e42 100644 --- a/pkg/services/ngalert/provisioning/mute_timings_test.go +++ b/pkg/services/ngalert/provisioning/mute_timings_test.go @@ -63,12 +63,17 @@ func TestGetMuteTimings(t *testing.T) { require.Equal(t, "Test1", result[0].Name) require.EqualValues(t, provenances["Test1"], result[0].Provenance) require.NotEmpty(t, result[0].Version) + require.Equal(t, getIntervalUID(result[0].MuteTimeInterval), result[0].UID) + require.Equal(t, "Test2", result[1].Name) require.EqualValues(t, provenances["Test2"], result[1].Provenance) require.NotEmpty(t, result[1].Version) + require.Equal(t, getIntervalUID(result[1].MuteTimeInterval), result[1].UID) + require.Equal(t, "Test3", result[2].Name) require.EqualValues(t, "", result[2].Provenance) require.NotEmpty(t, result[2].Version) + require.Equal(t, getIntervalUID(result[2].MuteTimeInterval), result[2].UID) require.Len(t, store.Calls, 1) require.Equal(t, "Get", store.Calls[0].Method) @@ -147,6 +152,7 @@ func TestGetMuteTiming(t *testing.T) { require.Equal(t, "Test1", result.Name) require.EqualValues(t, models.ProvenanceAPI, result.Provenance) + require.Equal(t, getIntervalUID(result.MuteTimeInterval), result.UID) require.NotEmpty(t, result.Version) require.Len(t, store.Calls, 1) @@ -154,6 +160,14 @@ func TestGetMuteTiming(t *testing.T) { require.Equal(t, orgID, store.Calls[0].Args[1]) prov.AssertCalled(t, "GetProvenance", mock.Anything, &result, orgID) + + t.Run("and by UID", func(t *testing.T) { + result2, err := sut.GetMuteTiming(context.Background(), result.UID, orgID) + + require.NoError(t, err) + + require.Equal(t, result, result2) + }) }) t.Run("service returns ErrTimeIntervalNotFound if no mute timings", func(t *testing.T) { @@ -294,6 +308,7 @@ func TestCreateMuteTimings(t *testing.T) { require.EqualValues(t, expected, result.MuteTimeInterval) require.EqualValues(t, expectedProvenance, result.Provenance) + require.Equal(t, getIntervalUID(expected), result.UID) require.NotEmpty(t, result.Version) require.Len(t, store.Calls, 2) @@ -402,6 +417,7 @@ func TestUpdateMuteTimings(t *testing.T) { } expectedProvenance := models.ProvenanceAPI expectedVersion := calculateMuteTimeIntervalFingerprint(expected) + expectedUID := getIntervalUID(expected) timing := definitions.MuteTimeInterval{ MuteTimeInterval: expected, Version: originalVersion, @@ -423,7 +439,51 @@ func TestUpdateMuteTimings(t *testing.T) { }) t.Run("rejects mute timings if provenance is not right", func(t *testing.T) { - sut, _, prov := createMuteTimingSvcSut() + sut, store, prov := createMuteTimingSvcSut() + store.GetFn = func(ctx context.Context, orgID int64) (*cfgRevision, error) { + return &cfgRevision{cfg: initialConfig()}, nil + } + expectedErr := errors.New("test") + sut.validator = func(from, to models.Provenance) error { + return expectedErr + } + timing := definitions.MuteTimeInterval{ + MuteTimeInterval: expected, + Provenance: definitions.Provenance(models.ProvenanceFile), + } + + prov.EXPECT().GetProvenance(mock.Anything, mock.Anything, mock.Anything).Return(expectedProvenance, nil) + + _, err := sut.UpdateMuteTiming(context.Background(), timing, orgID) + + require.ErrorIs(t, err, expectedErr) + }) + + t.Run("rejects if mute timing is renamed", func(t *testing.T) { + sut, store, prov := createMuteTimingSvcSut() + store.GetFn = func(ctx context.Context, orgID int64) (*cfgRevision, error) { + return &cfgRevision{cfg: initialConfig()}, nil + } + prov.EXPECT().GetProvenance(mock.Anything, mock.Anything, mock.Anything).Return(expectedProvenance, nil) + + interval := expected + interval.Name = "another-time-interval" + timing := definitions.MuteTimeInterval{ + UID: expectedUID, + MuteTimeInterval: interval, + Version: originalVersion, + Provenance: definitions.Provenance(expectedProvenance), + } + + _, err := sut.UpdateMuteTiming(context.Background(), timing, orgID) + require.ErrorIs(t, err, ErrTimeIntervalInvalid) + }) + + t.Run("rejects mute timings if provenance is not right", func(t *testing.T) { + sut, store, prov := createMuteTimingSvcSut() + store.GetFn = func(ctx context.Context, orgID int64) (*cfgRevision, error) { + return &cfgRevision{cfg: initialConfig()}, nil + } expectedErr := errors.New("test") sut.validator = func(from, to models.Provenance) error { return expectedErr @@ -477,6 +537,39 @@ func TestUpdateMuteTimings(t *testing.T) { require.Truef(t, ErrTimeIntervalNotFound.Is(err), "expected ErrTimeIntervalNotFound but got %s", err) }) + t.Run("returns ErrMuteTimingsNotFound if mute timing does not exist", func(t *testing.T) { + sut, store, prov := createMuteTimingSvcSut() + store.GetFn = func(ctx context.Context, orgID int64) (*cfgRevision, error) { + return &cfgRevision{cfg: initialConfig()}, nil + } + prov.EXPECT().GetProvenance(mock.Anything, mock.Anything, mock.Anything).Return(expectedProvenance, nil) + + t.Run("when UID is specified", func(t *testing.T) { + timing := definitions.MuteTimeInterval{ + UID: "not-found", + MuteTimeInterval: expected, + Provenance: definitions.Provenance(expectedProvenance), + } + + _, err := sut.UpdateMuteTiming(context.Background(), timing, orgID) + + require.ErrorIs(t, err, ErrTimeIntervalNotFound) + }) + + t.Run("when only Name is specified", func(t *testing.T) { + timing := definitions.MuteTimeInterval{ + MuteTimeInterval: config.MuteTimeInterval{ + Name: "not-found", + }, + Provenance: definitions.Provenance(expectedProvenance), + } + + _, err := sut.UpdateMuteTiming(context.Background(), timing, orgID) + + require.ErrorIs(t, err, ErrTimeIntervalNotFound) + }) + }) + t.Run("saves mute timing and provenance in a transaction if optimistic concurrency passes", func(t *testing.T) { sut, store, prov := createMuteTimingSvcSut() store.GetFn = func(ctx context.Context, orgID int64) (*cfgRevision, error) { @@ -499,6 +592,7 @@ func TestUpdateMuteTimings(t *testing.T) { require.EqualValues(t, expected, result.MuteTimeInterval) require.EqualValues(t, expectedProvenance, result.Provenance) require.EqualValues(t, expectedVersion, result.Version) + require.Equal(t, getIntervalUID(result.MuteTimeInterval), result.UID) require.Len(t, store.Calls, 2) require.Equal(t, "Get", store.Calls[0].Method) @@ -762,6 +856,43 @@ func TestDeleteMuteTimings(t *testing.T) { }) }) + t.Run("deletes mute timing and provenance by UID", func(t *testing.T) { + sut, store, prov := createMuteTimingSvcSut() + store.GetFn = func(ctx context.Context, orgID int64) (*cfgRevision, error) { + return &cfgRevision{cfg: initialConfig()}, nil + } + store.SaveFn = func(ctx context.Context, revision *cfgRevision) error { + assertInTransaction(t, ctx) + return nil + } + prov.EXPECT().GetProvenance(mock.Anything, mock.Anything, mock.Anything).Return(models.ProvenanceNone, nil) + prov.EXPECT().DeleteProvenance(mock.Anything, mock.Anything, mock.Anything).RunAndReturn( + func(ctx context.Context, _ models.Provisionable, _ int64) error { + assertInTransaction(t, ctx) + return nil + }) + + uid := getIntervalUID(timingToDelete) + + err := sut.DeleteMuteTiming(context.Background(), uid, orgID, "", correctVersion) + require.NoError(t, err) + + require.Len(t, store.Calls, 2) + require.Equal(t, "Get", store.Calls[0].Method) + require.Equal(t, orgID, store.Calls[0].Args[1]) + + require.Equal(t, "Save", store.Calls[1].Method) + require.Equal(t, orgID, store.Calls[1].Args[2]) + revision := store.Calls[1].Args[1].(*cfgRevision) + + expectedMuteTimings := slices.DeleteFunc(initialConfig().AlertmanagerConfig.MuteTimeIntervals, func(interval config.MuteTimeInterval) bool { + return interval.Name == timingToDelete.Name + }) + require.EqualValues(t, expectedMuteTimings, revision.cfg.AlertmanagerConfig.MuteTimeIntervals) + + prov.AssertCalled(t, "DeleteProvenance", mock.Anything, &definitions.MuteTimeInterval{MuteTimeInterval: timingToDelete}, orgID) + }) + t.Run("propagates errors", func(t *testing.T) { t.Run("when unable to read config", func(t *testing.T) { sut, store, prov := createMuteTimingSvcSut()