From 0f788d8d830c5182ac13071adf009b15fb1d6af1 Mon Sep 17 00:00:00 2001 From: Yuri Tseretyan Date: Tue, 17 Sep 2024 12:07:31 -0400 Subject: [PATCH] Alerting: Support for renaming receivers (#93349) * update RenameReceiverInNotificationSettings in DbStore to check for provisioning * implement renaming in receiver service and provisioning * do not patch route when stitching * fix bug in stitching because it returned new name but the old one was expected * update receiver service to always return result converted from storage model this makes sure that UID and version are consistent with GET\LIST operations * use provided metadata.name for UID of domain model because rename changes UID and request fails * remove rename guard * update UI to not disable receiver name when k8s api enabled * create should calculate uid from name because new receiver does not have UID yet. --- .../notifications/receiver/conversions.go | 3 +- .../notifications/receiver/legacy_storage.go | 4 - .../notifier/legacy_storage/receivers.go | 43 ++--- pkg/services/ngalert/notifier/receiver_svc.go | 80 +++++++-- .../ngalert/notifier/receiver_svc_test.go | 129 ++++++++----- pkg/services/ngalert/notifier/testing.go | 64 ++++--- .../ngalert/provisioning/contactpoints.go | 17 +- .../provisioning/contactpoints_test.go | 51 +++++- pkg/services/ngalert/provisioning/testing.go | 33 +++- pkg/services/ngalert/store/alert_rule.go | 38 +++- pkg/services/ngalert/store/alert_rule_test.go | 100 ++++++++--- .../notifications/receivers/receiver_test.go | 170 +++++++++--------- .../receivers/form/GrafanaReceiverForm.tsx | 3 - .../receivers/form/ReceiverForm.tsx | 7 - 14 files changed, 488 insertions(+), 254 deletions(-) diff --git a/pkg/registry/apis/alerting/notifications/receiver/conversions.go b/pkg/registry/apis/alerting/notifications/receiver/conversions.go index 6402db56562..d6c564a0f03 100644 --- a/pkg/registry/apis/alerting/notifications/receiver/conversions.go +++ b/pkg/registry/apis/alerting/notifications/receiver/conversions.go @@ -12,7 +12,6 @@ import ( model "github.com/grafana/grafana/pkg/apis/alerting_notifications/v0alpha1" "github.com/grafana/grafana/pkg/services/apiserver/endpoints/request" ngmodels "github.com/grafana/grafana/pkg/services/ngalert/models" - "github.com/grafana/grafana/pkg/services/ngalert/notifier/legacy_storage" ) func convertToK8sResources( @@ -115,7 +114,7 @@ var permissionMapper = map[ngmodels.ReceiverPermission]string{ func convertToDomainModel(receiver *model.Receiver) (*ngmodels.Receiver, map[string][]string, error) { domain := &ngmodels.Receiver{ - UID: legacy_storage.NameToUid(receiver.Spec.Title), + UID: receiver.Name, Name: receiver.Spec.Title, Integrations: make([]*ngmodels.Integration, 0, len(receiver.Spec.Integrations)), Version: receiver.ResourceVersion, diff --git a/pkg/registry/apis/alerting/notifications/receiver/legacy_storage.go b/pkg/registry/apis/alerting/notifications/receiver/legacy_storage.go index 091cf5848f4..6f05581820c 100644 --- a/pkg/registry/apis/alerting/notifications/receiver/legacy_storage.go +++ b/pkg/registry/apis/alerting/notifications/receiver/legacy_storage.go @@ -231,10 +231,6 @@ func (s *legacyStorage) Update(ctx context.Context, return old, false, err } - if p.ObjectMeta.Name != model.GetUID() { - return nil, false, errors.NewBadRequest("title cannot be changed. Consider creating a new resource.") - } - updated, err := s.service.UpdateReceiver(ctx, model, storedSecureFields, info.OrgID, user) if err != nil { return nil, false, err diff --git a/pkg/services/ngalert/notifier/legacy_storage/receivers.go b/pkg/services/ngalert/notifier/legacy_storage/receivers.go index 81353609368..158575401ba 100644 --- a/pkg/services/ngalert/notifier/legacy_storage/receivers.go +++ b/pkg/services/ngalert/notifier/legacy_storage/receivers.go @@ -17,57 +17,57 @@ func (rev *ConfigRevision) DeleteReceiver(uid string) { }) } -func (rev *ConfigRevision) CreateReceiver(receiver *models.Receiver) error { +func (rev *ConfigRevision) CreateReceiver(receiver *models.Receiver) (*definitions.PostableApiReceiver, error) { // Check if the receiver already exists. - _, err := rev.GetReceiver(receiver.GetUID()) + _, err := rev.GetReceiver(NameToUid(receiver.Name)) // get UID from name because the new receiver does not have UID yet. if err == nil { - return ErrReceiverExists.Errorf("") + return nil, ErrReceiverExists.Errorf("") } if !errors.Is(err, ErrReceiverNotFound) { - return err + return nil, err } if err := validateAndSetIntegrationUIDs(receiver); err != nil { - return err + return nil, err } postable, err := ReceiverToPostableApiReceiver(receiver) if err != nil { - return err + return nil, err } rev.Config.AlertmanagerConfig.Receivers = append(rev.Config.AlertmanagerConfig.Receivers, postable) if err := rev.ValidateReceiver(postable); err != nil { - return err + return nil, err } - return nil + return postable, nil } -func (rev *ConfigRevision) UpdateReceiver(receiver *models.Receiver) error { +func (rev *ConfigRevision) UpdateReceiver(receiver *models.Receiver) (*definitions.PostableApiReceiver, error) { existing, err := rev.GetReceiver(receiver.GetUID()) if err != nil { - return err + return nil, err } if err := validateAndSetIntegrationUIDs(receiver); err != nil { - return err + return nil, err } postable, err := ReceiverToPostableApiReceiver(receiver) if err != nil { - return err + return nil, err } // Update receiver in the configuration. *existing = *postable if err := rev.ValidateReceiver(existing); err != nil { - return err + return nil, err } - return nil + return postable, nil } // ReceiverNameUsedByRoutes checks if a receiver name is used in any routes. @@ -101,9 +101,9 @@ func (rev *ConfigRevision) GetReceivers(uids []string) []*definitions.PostableAp return receivers } -// RenameReceiverInRoutes renames all references to a receiver in routes. -func (rev *ConfigRevision) RenameReceiverInRoutes(oldName, newName string) { - RenameReceiverInRoute(oldName, newName, rev.Config.AlertmanagerConfig.Route) +// RenameReceiverInRoutes renames all references to a receiver in routes. Returns number of routes that were updated +func (rev *ConfigRevision) RenameReceiverInRoutes(oldName, newName string) int { + return RenameReceiverInRoute(oldName, newName, rev.Config.AlertmanagerConfig.Route) } // ValidateReceiver checks if the given receiver conflicts in name or integration UID with existing receivers. @@ -135,16 +135,19 @@ func (rev *ConfigRevision) ValidateReceiver(p *definitions.PostableApiReceiver) return nil } -func RenameReceiverInRoute(oldName, newName string, routes ...*definitions.Route) { +func RenameReceiverInRoute(oldName, newName string, routes ...*definitions.Route) int { if len(routes) == 0 { - return + return 0 } + updated := 0 for _, route := range routes { if route.Receiver == oldName { route.Receiver = newName + updated++ } - RenameReceiverInRoute(oldName, newName, route.Routes...) + updated += RenameReceiverInRoute(oldName, newName, route.Routes...) } + return updated } // isReceiverInUse checks if a receiver is used in a route or any of its sub-routes. diff --git a/pkg/services/ngalert/notifier/receiver_svc.go b/pkg/services/ngalert/notifier/receiver_svc.go index 328ee24fdf0..e3b83d71e42 100644 --- a/pkg/services/ngalert/notifier/receiver_svc.go +++ b/pkg/services/ngalert/notifier/receiver_svc.go @@ -28,6 +28,11 @@ var ( "Provided version '{{ .Public.Version }}' of receiver '{{ .Public.Name }}' does not match current version '{{ .Public.CurrentVersion }}'", errutil.WithPublic("Provided version '{{ .Public.Version }}' of receiver '{{ .Public.Name }}' does not match current version '{{ .Public.CurrentVersion }}'"), ) + + ErrReceiverDependentResourcesProvenance = errutil.Conflict("alerting.notifications.receivers.usedProvisioned").MustTemplate( + "Receiver cannot be renamed because it is used by provisioned {{ if .Public.UsedByRules }}alert rules{{ end }}{{ if .Public.UsedByRoutes }}{{ if .Public.UsedByRules }} and {{ end }}notification policies{{ end }}", + errutil.WithPublic(`Receiver cannot be renamed because it is used by provisioned {{ if .Public.UsedByRules }}alert rules{{ end }}{{ if .Public.UsedByRoutes }}{{ if .Public.UsedByRules }} and {{ end }}notification policies{{ end }}. You must update those resources first using the original provision method.`), + ) ) // ReceiverService is the service for managing alertmanager receivers. @@ -43,7 +48,7 @@ type ReceiverService struct { } type alertRuleNotificationSettingsStore interface { - RenameReceiverInNotificationSettings(ctx context.Context, orgID int64, oldReceiver, newReceiver string) (int, error) + RenameReceiverInNotificationSettings(ctx context.Context, orgID int64, oldReceiver, newReceiver string, validateProvenance func(models.Provenance) bool, dryRun bool) ([]models.AlertRuleKey, []models.AlertRuleKey, error) ListNotificationSettings(ctx context.Context, q models.ListNotificationSettingsQuery) (map[models.AlertRuleKey][]models.NotificationSettings, error) } @@ -73,6 +78,7 @@ type alertmanagerConfigStore interface { } type provisoningStore interface { + GetProvenance(ctx context.Context, o models.Provisionable, org int64) (models.Provenance, error) GetProvenances(ctx context.Context, org int64, resourceType string) (map[string]models.Provenance, error) SetProvenance(ctx context.Context, o models.Provisionable, org int64, p models.Provenance) error DeleteProvenance(ctx context.Context, o models.Provisionable, org int64) error @@ -330,11 +336,10 @@ func (rs *ReceiverService) CreateReceiver(ctx context.Context, r *models.Receive return nil, legacy_storage.MakeErrReceiverInvalid(err) } - err = revision.CreateReceiver(&createdReceiver) + created, err := revision.CreateReceiver(&createdReceiver) if err != nil { return nil, err } - createdReceiver.Version = createdReceiver.Fingerprint() err = rs.xact.InTransaction(ctx, func(ctx context.Context) error { err = rs.cfgStore.Save(ctx, revision, orgID) @@ -346,7 +351,12 @@ func (rs *ReceiverService) CreateReceiver(ctx context.Context, r *models.Receive if err != nil { return nil, err } - return &createdReceiver, nil + + result, err := PostableApiReceiverToReceiver(created, createdReceiver.Provenance) + if err != nil { + return nil, err + } + return result, nil } func (rs *ReceiverService) UpdateReceiver(ctx context.Context, r *models.Receiver, storedSecureFields map[string][]string, orgID int64, user identity.Requester) (*models.Receiver, error) { @@ -400,26 +410,19 @@ func (rs *ReceiverService) UpdateReceiver(ctx context.Context, r *models.Receive return nil, legacy_storage.MakeErrReceiverInvalid(err) } - err = revision.UpdateReceiver(&updatedReceiver) + updated, err := revision.UpdateReceiver(&updatedReceiver) if err != nil { return nil, err } - updatedReceiver.Version = updatedReceiver.Fingerprint() err = rs.xact.InTransaction(ctx, func(ctx context.Context) error { // If the name of the receiver changed, we must update references to it in both routes and notification settings. - // TODO: Needs to check provenance status compatibility: For example, if we rename a receiver via UI but rules are provisioned, this call should be rejected. if existing.Name != r.Name { - affected, err := rs.ruleNotificationsStore.RenameReceiverInNotificationSettings(ctx, orgID, existing.Name, r.Name) + err := rs.RenameReceiverInDependentResources(ctx, orgID, revision.Config.AlertmanagerConfig.Route, existing.Name, r.Name, r.Provenance) if err != nil { return err } - if affected > 0 { - rs.log.Info("Renamed receiver in notification settings", "oldName", existing.Name, "newName", r.Name, "affectedSettings", affected) - } - revision.RenameReceiverInRoutes(existing.Name, r.Name) } - err = rs.cfgStore.Save(ctx, revision, orgID) if err != nil { return err @@ -434,7 +437,12 @@ func (rs *ReceiverService) UpdateReceiver(ctx context.Context, r *models.Receive if err != nil { return nil, err } - return &updatedReceiver, nil + + result, err := PostableApiReceiverToReceiver(updated, updatedReceiver.Provenance) + if err != nil { + return nil, err + } + return result, nil } func (rs *ReceiverService) UsedByRules(ctx context.Context, orgID int64, name string) ([]models.AlertRuleKey, error) { @@ -611,3 +619,47 @@ func makeErrReceiverVersionConflict(current *models.Receiver, desiredVersion str } return ErrReceiverVersionConflict.Build(data) } + +func makeErrReceiverDependentResourcesProvenance(usedByRoutes bool, rules []models.AlertRuleKey) error { + uids := make([]string, 0, len(rules)) + for _, key := range rules { + uids = append(uids, key.UID) + } + data := make(map[string]any, 2) + if len(uids) > 0 { + data["UsedByRules"] = uids + } + if usedByRoutes { + data["UsedByRoutes"] = true + } + + return ErrReceiverDependentResourcesProvenance.Build(errutil.TemplateData{ + Public: data, + }) +} + +func (rs *ReceiverService) RenameReceiverInDependentResources(ctx context.Context, orgID int64, route *definitions.Route, oldName, newName string, receiverProvenance models.Provenance) error { + validate := validation.ValidateProvenanceOfDependentResources(receiverProvenance) + // if there are no references to the old time interval, exit + updatedRoutes := legacy_storage.RenameReceiverInRoute(oldName, newName, route) + canUpdate := true + if updatedRoutes > 0 { + routeProvenance, err := rs.provisioningStore.GetProvenance(ctx, route, orgID) + if err != nil { + return err + } + canUpdate = validate(routeProvenance) + } + dryRun := !canUpdate + affected, invalidProvenance, err := rs.ruleNotificationsStore.RenameReceiverInNotificationSettings(ctx, orgID, oldName, newName, validate, dryRun) + if err != nil { + return err + } + if !canUpdate || len(invalidProvenance) > 0 { + return makeErrReceiverDependentResourcesProvenance(updatedRoutes > 0, invalidProvenance) + } + if len(affected) > 0 || updatedRoutes > 0 { + rs.log.FromContext(ctx).Info("Updated rules and routes that use renamed receiver", "oldName", oldName, "newName", newName, "rules", len(affected), "routes", updatedRoutes) + } + return nil +} diff --git a/pkg/services/ngalert/notifier/receiver_svc_test.go b/pkg/services/ngalert/notifier/receiver_svc_test.go index 75621d1c095..e8b168a36ee 100644 --- a/pkg/services/ngalert/notifier/receiver_svc_test.go +++ b/pkg/services/ngalert/notifier/receiver_svc_test.go @@ -287,16 +287,12 @@ func TestReceiverService_Delete(t *testing.T) { }, } { t.Run(tc.name, func(t *testing.T) { + store := &fakeAlertRuleNotificationStore{} + store.ListNotificationSettingsFn = func(ctx context.Context, q models.ListNotificationSettingsQuery) (map[models.AlertRuleKey][]models.NotificationSettings, error) { + return tc.storeSettings, nil + } sut := createReceiverServiceSut(t, &secretsService) - - store := sut.ruleNotificationsStore.(*fakeConfigStore) - store.notificationSettings = map[int64]map[models.AlertRuleKey][]models.NotificationSettings{ - 1: make(map[models.AlertRuleKey][]models.NotificationSettings), - } - - for key, settings := range tc.storeSettings { - store.notificationSettings[tc.user.GetOrgID()][key] = settings - } + sut.ruleNotificationsStore = store if tc.existing != nil { created, err := sut.CreateReceiver(context.Background(), tc.existing, tc.user.GetOrgID(), tc.user) @@ -758,8 +754,6 @@ func TestReceiverService_UpdateReceiverName(t *testing.T) { }} secretsService := fake_secrets.NewFakeSecretsService() - sut := createReceiverServiceSut(t, &secretsService) - receiverName := "grafana-default-email" newReceiverName := "new-name" slackIntegration := models.IntegrationGen(models.IntegrationMuts.WithName(receiverName), models.IntegrationMuts.WithValidConfig("slack"))() @@ -767,38 +761,78 @@ func TestReceiverService_UpdateReceiverName(t *testing.T) { baseReceiver.Version = "cd95627c75892a39" // Correct version for grafana-default-email. baseReceiver.Name = newReceiverName // Done here instead of in a mutator so we keep the same uid. - store := sut.ruleNotificationsStore.(*fakeConfigStore) - ns := models.NotificationSettingsGen(models.NSMuts.WithReceiver(receiverName))() - store.notificationSettings = map[int64]map[models.AlertRuleKey][]models.NotificationSettings{ - 1: { - {OrgID: 1, UID: "rule1"}: {ns}, - }, - } + t.Run("renames receiver and all its dependencies", func(t *testing.T) { + ruleStore := &fakeAlertRuleNotificationStore{} + sut := createReceiverServiceSut(t, &secretsService) + sut.ruleNotificationsStore = ruleStore - _, err := sut.UpdateReceiver(context.Background(), &baseReceiver, nil, writer.GetOrgID(), writer) - require.NoError(t, err) + _, err := sut.UpdateReceiver(context.Background(), &baseReceiver, nil, writer.GetOrgID(), writer) + require.NoError(t, err) - // Ensure receiver name is updated in notification settings. - oldSettings, err := sut.ruleNotificationsStore.ListNotificationSettings(context.Background(), models.ListNotificationSettingsQuery{ - OrgID: writer.GetOrgID(), - ReceiverName: receiverName, + assert.Equal(t, "RenameReceiverInNotificationSettings", ruleStore.Calls[0].Method) + assert.Equal(t, writer.OrgID, ruleStore.Calls[0].Args[1]) + assert.Equal(t, receiverName, ruleStore.Calls[0].Args[2]) + assert.Equal(t, newReceiverName, ruleStore.Calls[0].Args[3]) + assert.NotNil(t, ruleStore.Calls[0].Args[4]) + assert.Falsef(t, ruleStore.Calls[0].Args[5].(bool), "dryrun expected to be false") + + // Ensure receiver name is updated in routes. + revision, err := sut.cfgStore.Get(context.Background(), writer.GetOrgID()) + require.NoError(t, err) + + assert.Falsef(t, revision.ReceiverNameUsedByRoutes(receiverName), "old receiver name '%s' should not be used by routes", receiverName) + assert.Truef(t, revision.ReceiverNameUsedByRoutes(newReceiverName), "new receiver name '%s' should be used by routes", newReceiverName) }) - require.NoError(t, err) - assert.Equal(t, 0, len(oldSettings)) - newSettings, err := sut.ruleNotificationsStore.ListNotificationSettings(context.Background(), models.ListNotificationSettingsQuery{ - OrgID: writer.GetOrgID(), - ReceiverName: baseReceiver.Name, + + t.Run("returns ErrReceiverDependentResourcesProvenance if route has different provenance status", func(t *testing.T) { + sut := createReceiverServiceSut(t, &secretsService) + provenanceStore := sut.provisioningStore.(*fakes.FakeProvisioningStore) + provenanceStore.Records[1] = map[string]models.Provenance{ + (&definitions.Route{}).ResourceType(): models.ProvenanceFile, + } + + ruleStore := &fakeAlertRuleNotificationStore{ + RenameReceiverInNotificationSettingsFn: func(ctx context.Context, orgID int64, old, new string, validate func(models.Provenance) bool, dryRun bool) ([]models.AlertRuleKey, []models.AlertRuleKey, error) { + assertInTransaction(t, ctx) + return nil, nil, nil + }, + } + sut.ruleNotificationsStore = ruleStore + + _, err := sut.UpdateReceiver(context.Background(), &baseReceiver, nil, writer.GetOrgID(), writer) + require.ErrorIs(t, err, ErrReceiverDependentResourcesProvenance) + + require.Len(t, ruleStore.Calls, 1) + assert.Equal(t, "RenameReceiverInNotificationSettings", ruleStore.Calls[0].Method) + assert.Equal(t, writer.OrgID, ruleStore.Calls[0].Args[1]) + assert.Equal(t, receiverName, ruleStore.Calls[0].Args[2]) + assert.Equal(t, newReceiverName, ruleStore.Calls[0].Args[3]) + assert.NotNil(t, ruleStore.Calls[0].Args[4]) + assert.True(t, ruleStore.Calls[0].Args[5].(bool)) // still check if there are rules that have incompatible provenance }) - require.NoError(t, err) - assert.Equal(t, 1, len(newSettings)) - assert.Equal(t, newReceiverName, newSettings[models.AlertRuleKey{OrgID: 1, UID: "rule1"}][0].Receiver) - // Ensure receiver name is updated in routes. - revision, err := sut.cfgStore.Get(context.Background(), writer.GetOrgID()) - require.NoError(t, err) + t.Run("returns ErrReceiverDependentResourcesProvenance if rules have different provenance status", func(t *testing.T) { + sut := createReceiverServiceSut(t, &secretsService) - assert.Falsef(t, revision.ReceiverNameUsedByRoutes(receiverName), "old receiver name '%s' should not be used by routes", receiverName) - assert.Truef(t, revision.ReceiverNameUsedByRoutes(newReceiverName), "new receiver name '%s' should be used by routes", newReceiverName) + ruleStore := &fakeAlertRuleNotificationStore{ + RenameReceiverInNotificationSettingsFn: func(ctx context.Context, orgID int64, old, new string, validate func(models.Provenance) bool, dryRun bool) ([]models.AlertRuleKey, []models.AlertRuleKey, error) { + assertInTransaction(t, ctx) + return nil, []models.AlertRuleKey{models.GenerateRuleKey(orgID)}, nil + }, + } + sut.ruleNotificationsStore = ruleStore + + _, err := sut.UpdateReceiver(context.Background(), &baseReceiver, nil, writer.GetOrgID(), writer) + require.ErrorIs(t, err, ErrReceiverDependentResourcesProvenance) + + require.Len(t, ruleStore.Calls, 1) + assert.Equal(t, "RenameReceiverInNotificationSettings", ruleStore.Calls[0].Method) + assert.Equal(t, writer.OrgID, ruleStore.Calls[0].Args[1]) + assert.Equal(t, receiverName, ruleStore.Calls[0].Args[2]) + assert.Equal(t, newReceiverName, ruleStore.Calls[0].Args[3]) + assert.NotNil(t, ruleStore.Calls[0].Args[4]) + assert.Falsef(t, ruleStore.Calls[0].Args[5].(bool), "dryrun expected to be false") + }) } func TestReceiverServiceAC_Read(t *testing.T) { @@ -1401,16 +1435,13 @@ func TestReceiverService_InUseMetadata(t *testing.T) { }, } { t.Run(tc.name, func(t *testing.T) { + store := &fakeAlertRuleNotificationStore{} + store.ListNotificationSettingsFn = func(ctx context.Context, q models.ListNotificationSettingsQuery) (map[models.AlertRuleKey][]models.NotificationSettings, error) { + return tc.storeSettings, nil + } + sut := createReceiverServiceSut(t, &secretsService) - - store := sut.ruleNotificationsStore.(*fakeConfigStore) - store.notificationSettings = map[int64]map[models.AlertRuleKey][]models.NotificationSettings{ - 1: make(map[models.AlertRuleKey][]models.NotificationSettings), - } - - for key, settings := range tc.storeSettings { - store.notificationSettings[tc.user.GetOrgID()][key] = settings - } + sut.ruleNotificationsStore = store for _, recv := range tc.existing { _, err := sut.CreateReceiver(context.Background(), recv, tc.user.GetOrgID(), tc.user) @@ -1448,7 +1479,7 @@ func createReceiverServiceSut(t *testing.T, encryptSvc secretService) *ReceiverS ac.NewReceiverAccess[*models.Receiver](acimpl.ProvideAccessControl(featuremgmt.WithFeatures(), zanzana.NewNoopClient()), false), legacy_storage.NewAlertmanagerConfigStore(store), provisioningStore, - NewFakeConfigStore(t, nil), + &fakeAlertRuleNotificationStore{}, encryptSvc, xact, log.NewNopLogger(), @@ -1533,3 +1564,7 @@ func newNopTransactionManager() *NopTransactionManager { func (n *NopTransactionManager) InTransaction(ctx context.Context, work func(ctx context.Context) error) error { return work(context.WithValue(ctx, NopTransactionManager{}, struct{}{})) } + +func assertInTransaction(t *testing.T, ctx context.Context) { + assert.Truef(t, ctx.Value(NopTransactionManager{}) != nil, "Expected to be executed in transaction but there is none") +} diff --git a/pkg/services/ngalert/notifier/testing.go b/pkg/services/ngalert/notifier/testing.go index aac14ef988b..8c268c3de93 100644 --- a/pkg/services/ngalert/notifier/testing.go +++ b/pkg/services/ngalert/notifier/testing.go @@ -57,28 +57,6 @@ func (f *fakeConfigStore) ListNotificationSettings(ctx context.Context, q models return settings, nil } -func (f *fakeConfigStore) RenameReceiverInNotificationSettings(ctx context.Context, orgID int64, oldReceiver, newReceiver string) (int, error) { - if oldReceiver == newReceiver { - return 0, nil - } - settings, ok := f.notificationSettings[orgID] - if !ok { - return 0, nil - } - - var updated int - for _, notificationSettings := range settings { - for i, setting := range notificationSettings { - if setting.Receiver == oldReceiver { - updated++ - notificationSettings[i].Receiver = newReceiver - } - } - } - - return updated, nil -} - // Saves the image or returns an error. func (f *fakeConfigStore) SaveImage(ctx context.Context, img *models.Image) error { return alertingImages.ErrImageNotFound @@ -379,3 +357,45 @@ func createNotificationLog(groupKey string, receiverName string, sentAt, expires ExpiresAt: expiresAt, } } + +type call struct { + Method string + Args []interface{} +} + +type fakeAlertRuleNotificationStore struct { + Calls []call + + RenameReceiverInNotificationSettingsFn func(ctx context.Context, orgID int64, oldReceiver, newReceiver string, validateProvenance func(models.Provenance) bool, dryRun bool) ([]models.AlertRuleKey, []models.AlertRuleKey, error) + ListNotificationSettingsFn func(ctx context.Context, q models.ListNotificationSettingsQuery) (map[models.AlertRuleKey][]models.NotificationSettings, error) +} + +func (f *fakeAlertRuleNotificationStore) RenameReceiverInNotificationSettings(ctx context.Context, orgID int64, oldReceiver, newReceiver string, validateProvenance func(models.Provenance) bool, dryRun bool) ([]models.AlertRuleKey, []models.AlertRuleKey, error) { + call := call{ + Method: "RenameReceiverInNotificationSettings", + Args: []interface{}{ctx, orgID, oldReceiver, newReceiver, validateProvenance, dryRun}, + } + f.Calls = append(f.Calls, call) + + if f.RenameReceiverInNotificationSettingsFn != nil { + return f.RenameReceiverInNotificationSettingsFn(ctx, orgID, oldReceiver, newReceiver, validateProvenance, dryRun) + } + + // Default values when no function hook is provided + return nil, nil, nil +} + +func (f *fakeAlertRuleNotificationStore) ListNotificationSettings(ctx context.Context, q models.ListNotificationSettingsQuery) (map[models.AlertRuleKey][]models.NotificationSettings, error) { + call := call{ + Method: "ListNotificationSettings", + Args: []interface{}{ctx, q}, + } + f.Calls = append(f.Calls, call) + + if f.ListNotificationSettingsFn != nil { + return f.ListNotificationSettingsFn(ctx, q) + } + + // Default values when no function hook is provided + return nil, nil +} diff --git a/pkg/services/ngalert/provisioning/contactpoints.go b/pkg/services/ngalert/provisioning/contactpoints.go index 97a6fb10ab1..2fad1402fb9 100644 --- a/pkg/services/ngalert/provisioning/contactpoints.go +++ b/pkg/services/ngalert/provisioning/contactpoints.go @@ -24,7 +24,7 @@ import ( ) type AlertRuleNotificationSettingsStore interface { - RenameReceiverInNotificationSettings(ctx context.Context, orgID int64, oldReceiver, newReceiver string) (int, error) + RenameReceiverInNotificationSettings(ctx context.Context, orgID int64, oldReceiver, newReceiver string, validateProvenance func(models.Provenance) bool, dryRun bool) ([]models.AlertRuleKey, []models.AlertRuleKey, error) RenameTimeIntervalInNotificationSettings(ctx context.Context, orgID int64, oldTimeInterval, newTimeInterval string, validateProvenance func(models.Provenance) bool, dryRun bool) ([]models.AlertRuleKey, []models.AlertRuleKey, error) ListNotificationSettings(ctx context.Context, q models.ListNotificationSettingsQuery) (map[models.AlertRuleKey][]models.NotificationSettings, error) } @@ -41,6 +41,7 @@ type ContactPointService struct { type receiverService interface { GetReceivers(ctx context.Context, query models.GetReceiversQuery, user identity.Requester) ([]*models.Receiver, error) + RenameReceiverInDependentResources(ctx context.Context, orgID int64, route *apimodels.Route, oldName, newName string, receiverProvenance models.Provenance) error } func NewContactPointService(store alertmanagerConfigStore, encryptionService secrets.Service, @@ -290,17 +291,14 @@ func (ecp *ContactPointService) UpdateContactPoint(ctx context.Context, orgID in } err = ecp.xact.InTransaction(ctx, func(ctx context.Context) error { - if err := ecp.configStore.Save(ctx, revision, orgID); err != nil { - return err - } if renamedReceiver != "" && renamedReceiver != mergedReceiver.Name { - affected, err := ecp.notificationSettingsStore.RenameReceiverInNotificationSettings(ctx, orgID, renamedReceiver, mergedReceiver.Name) + err := ecp.receiverService.RenameReceiverInDependentResources(ctx, orgID, revision.Config.AlertmanagerConfig.Route, renamedReceiver, mergedReceiver.Name, provenance) if err != nil { return err } - if affected > 0 { - ecp.log.Info("Renamed receiver in notification settings", "oldName", renamedReceiver, "newName", mergedReceiver.Name, "affectedSettings", affected) - } + } + if err := ecp.configStore.Save(ctx, revision, orgID); err != nil { + return err } return ecp.provenanceStore.SetProvenance(ctx, &contactPoint, orgID, provenance) }) @@ -426,10 +424,9 @@ groupLoop: // If we're renaming, we'll need to fix up the macro receiver group for consistency. // Firstly, if we're the only receiver in the group, simply rename the group to match. Done! if len(receiverGroup.GrafanaManagedReceivers) == 1 { - legacy_storage.RenameReceiverInRoute(receiverGroup.Name, target.Name, cfg.AlertmanagerConfig.Route) + renamedReceiver = receiverGroup.Name // remember the old name of the receiver. receiverGroup.Name = target.Name receiverGroup.GrafanaManagedReceivers[i] = target - renamedReceiver = receiverGroup.Name } // Otherwise, we only want to rename the receiver we are touching... NOT all of them. diff --git a/pkg/services/ngalert/provisioning/contactpoints_test.go b/pkg/services/ngalert/provisioning/contactpoints_test.go index 34d057f8eab..8473bece839 100644 --- a/pkg/services/ngalert/provisioning/contactpoints_test.go +++ b/pkg/services/ngalert/provisioning/contactpoints_test.go @@ -178,6 +178,44 @@ func TestContactPointService(t *testing.T) { require.ErrorIs(t, err, ErrValidation) }) + t.Run("update renames references when group is renamed", func(t *testing.T) { + cfg := createEncryptedConfig(t, secretsService) + store := fakes.NewFakeAlertmanagerConfigStore(cfg) + sut := createContactPointServiceSutWithConfigStore(t, secretsService, store) + + svc := &fakeReceiverService{} + sut.receiverService = svc + + newCp := createTestContactPoint() + oldName := newCp.Name + newName := "new-name" + + newCp, err := sut.CreateContactPoint(context.Background(), 1, newCp, models.ProvenanceAPI) + require.NoError(t, err) + + newCp.Name = newName + + svc.RenameReceiverInDependentResourcesFunc = func(ctx context.Context, orgID int64, route *definitions.Route, oldName, newName string, receiverProvenance models.Provenance) error { + legacy_storage.RenameReceiverInRoute(oldName, newName, route) + return nil + } + + err = sut.UpdateContactPoint(context.Background(), 1, newCp, models.ProvenanceAPI) + require.NoError(t, err) + + parsed, err := legacy_storage.DeserializeAlertmanagerConfig([]byte(store.LastSaveCommand.AlertmanagerConfiguration)) + require.NoError(t, err) + + require.Lenf(t, svc.Calls, 1, "service was supposed to be called once") + assert.Equal(t, "RenameReceiverInDependentResources", svc.Calls[0].Method) + assertInTransaction(t, svc.Calls[0].Args[0].(context.Context)) + assert.Equal(t, int64(1), svc.Calls[0].Args[1]) + assert.EqualValues(t, parsed.AlertmanagerConfig.Route, svc.Calls[0].Args[2]) + assert.Equal(t, oldName, svc.Calls[0].Args[3]) + assert.Equal(t, newName, svc.Calls[0].Args[4]) + assert.Equal(t, models.ProvenanceAPI, svc.Calls[0].Args[5]) + }) + t.Run("default provenance of contact points is none", func(t *testing.T) { sut := createContactPointServiceSut(t, secretsService) @@ -442,6 +480,7 @@ func createContactPointServiceSut(t *testing.T, secretService secrets.Service) * } func createContactPointServiceSutWithConfigStore(t *testing.T, secretService secrets.Service, configStore legacy_storage.AMConfigStore) *ContactPointService { + t.Helper() // Encrypt secure settings. xact := newNopTransactionManager() provisioningStore := fakes.NewFakeProvisioningStore() @@ -450,7 +489,7 @@ func createContactPointServiceSutWithConfigStore(t *testing.T, secretService sec ac.NewReceiverAccess[*models.Receiver](acimpl.ProvideAccessControl(featuremgmt.WithFeatures(), zanzana.NewNoopClient()), true), legacy_storage.NewAlertmanagerConfigStore(configStore), provisioningStore, - notifier.NewFakeConfigStore(t, nil), + &fakeAlertRuleNotificationStore{}, secretService, xact, log.NewNopLogger(), @@ -589,14 +628,14 @@ func TestStitchReceivers(t *testing.T) { Type: "slack", }, expModified: true, - expRenamedReceiver: "new-receiver", + expRenamedReceiver: "receiver-1", expCfg: definitions.PostableApiAlertingConfig{ Config: definitions.Config{ Route: &definitions.Route{ - Receiver: "new-receiver", + Receiver: "receiver-1", Routes: []*definitions.Route{ { - Receiver: "new-receiver", + Receiver: "receiver-1", }, }, }, @@ -1191,7 +1230,7 @@ func TestStitchReceivers(t *testing.T) { Type: "slack", }, expModified: true, - expRenamedReceiver: "receiver-1", + expRenamedReceiver: "receiver-2", expCfg: definitions.PostableApiAlertingConfig{ Config: definitions.Config{ Route: &definitions.Route{ @@ -1201,7 +1240,7 @@ func TestStitchReceivers(t *testing.T) { Receiver: "receiver-1", }, { - Receiver: "receiver-1", + Receiver: "receiver-2", }, }, }, diff --git a/pkg/services/ngalert/provisioning/testing.go b/pkg/services/ngalert/provisioning/testing.go index 82fef4ed982..beefd0287fc 100644 --- a/pkg/services/ngalert/provisioning/testing.go +++ b/pkg/services/ngalert/provisioning/testing.go @@ -9,6 +9,7 @@ import ( mock "github.com/stretchr/testify/mock" "github.com/grafana/grafana/pkg/apimachinery/identity" + apimodels "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/notifier" "github.com/grafana/grafana/pkg/services/ngalert/store" @@ -169,24 +170,24 @@ func (s *fakeRuleAccessControlService) CanWriteAllRules(ctx context.Context, use type fakeAlertRuleNotificationStore struct { Calls []call - RenameReceiverInNotificationSettingsFn func(ctx context.Context, orgID int64, oldReceiver, newReceiver string) (int, error) + RenameReceiverInNotificationSettingsFn func(ctx context.Context, orgID int64, oldReceiver, newReceiver string, validateProvenance func(models.Provenance) bool, dryRun bool) ([]models.AlertRuleKey, []models.AlertRuleKey, error) RenameTimeIntervalInNotificationSettingsFn func(ctx context.Context, orgID int64, old, new string, validate func(models.Provenance) bool, dryRun bool) ([]models.AlertRuleKey, []models.AlertRuleKey, error) ListNotificationSettingsFn func(ctx context.Context, q models.ListNotificationSettingsQuery) (map[models.AlertRuleKey][]models.NotificationSettings, error) } -func (f *fakeAlertRuleNotificationStore) RenameReceiverInNotificationSettings(ctx context.Context, orgID int64, oldReceiver, newReceiver string) (int, error) { +func (f *fakeAlertRuleNotificationStore) RenameReceiverInNotificationSettings(ctx context.Context, orgID int64, oldReceiver, newReceiver string, validateProvenance func(models.Provenance) bool, dryRun bool) ([]models.AlertRuleKey, []models.AlertRuleKey, error) { call := call{ Method: "RenameReceiverInNotificationSettings", - Args: []interface{}{ctx, orgID, oldReceiver, newReceiver}, + Args: []interface{}{ctx, orgID, oldReceiver, newReceiver, validateProvenance, dryRun}, } f.Calls = append(f.Calls, call) if f.RenameReceiverInNotificationSettingsFn != nil { - return f.RenameReceiverInNotificationSettingsFn(ctx, orgID, oldReceiver, newReceiver) + return f.RenameReceiverInNotificationSettingsFn(ctx, orgID, oldReceiver, newReceiver, validateProvenance, dryRun) } // Default values when no function hook is provided - return 0, nil + return nil, nil, nil } func (f *fakeAlertRuleNotificationStore) RenameTimeIntervalInNotificationSettings(ctx context.Context, orgID int64, oldTimeInterval, newTimeInterval string, validate func(models.Provenance) bool, dryRun bool) ([]models.AlertRuleKey, []models.AlertRuleKey, error) { @@ -218,3 +219,25 @@ func (f *fakeAlertRuleNotificationStore) ListNotificationSettings(ctx context.Co // Default values when no function hook is provided return nil, nil } + +type fakeReceiverService struct { + Calls []call + GetReceiversFunc func(ctx context.Context, query models.GetReceiversQuery, user identity.Requester) ([]*models.Receiver, error) + RenameReceiverInDependentResourcesFunc func(ctx context.Context, orgID int64, route *apimodels.Route, oldName, newName string, receiverProvenance models.Provenance) error +} + +func (f *fakeReceiverService) GetReceivers(ctx context.Context, query models.GetReceiversQuery, user identity.Requester) ([]*models.Receiver, error) { + f.Calls = append(f.Calls, call{Method: "GetReceivers", Args: []interface{}{ctx, query, user}}) + if f.GetReceiversFunc != nil { + return f.GetReceiversFunc(ctx, query, user) + } + return nil, nil +} + +func (f *fakeReceiverService) RenameReceiverInDependentResources(ctx context.Context, orgID int64, route *apimodels.Route, oldName, newName string, receiverProvenance models.Provenance) error { + f.Calls = append(f.Calls, call{Method: "RenameReceiverInDependentResources", Args: []interface{}{ctx, orgID, route, oldName, newName, receiverProvenance}}) + if f.RenameReceiverInDependentResourcesFunc != nil { + return f.RenameReceiverInDependentResourcesFunc(ctx, orgID, route, oldName, newName, receiverProvenance) + } + return nil +} diff --git a/pkg/services/ngalert/store/alert_rule.go b/pkg/services/ngalert/store/alert_rule.go index a7892297b90..93c19037b15 100644 --- a/pkg/services/ngalert/store/alert_rule.go +++ b/pkg/services/ngalert/store/alert_rule.go @@ -824,21 +824,45 @@ func (st DBstore) filterByContentInNotificationSettings(value string, sess *xorm return sess.And(fmt.Sprintf("notification_settings %s ?", st.SQLStore.GetDialect().LikeStr()), "%"+search+"%"), nil } -func (st DBstore) RenameReceiverInNotificationSettings(ctx context.Context, orgID int64, oldReceiver, newReceiver string) (int, error) { +func (st DBstore) RenameReceiverInNotificationSettings(ctx context.Context, orgID int64, oldReceiver, newReceiver string, validateProvenance func(ngmodels.Provenance) bool, dryRun bool) ([]ngmodels.AlertRuleKey, []ngmodels.AlertRuleKey, error) { // fetch entire rules because Update method requires it because it copies rules to version table rules, err := st.ListAlertRules(ctx, &ngmodels.ListAlertRulesQuery{ OrgID: orgID, ReceiverName: oldReceiver, }) if err != nil { - return 0, err + return nil, nil, err } if len(rules) == 0 { - return 0, nil + return nil, nil, nil } + provenances, err := st.GetProvenances(ctx, orgID, (&ngmodels.AlertRule{}).ResourceType()) + if err != nil { + return nil, nil, err + } + + var invalidProvenance []ngmodels.AlertRuleKey + result := make([]ngmodels.AlertRuleKey, 0, len(rules)) updates := make([]ngmodels.UpdateRule, 0, len(rules)) for _, rule := range rules { + provenance, ok := provenances[rule.UID] + if !ok { + provenance = ngmodels.ProvenanceNone + } + if !validateProvenance(provenance) { + invalidProvenance = append(invalidProvenance, rule.GetKey()) + } + if len(invalidProvenance) > 0 { // do not do any fixes if there is at least one rule with not allowed provenance + continue + } + + result = append(result, rule.GetKey()) + + if dryRun { + continue + } + r := ngmodels.CopyRule(rule) for idx := range r.NotificationSettings { if r.NotificationSettings[idx].Receiver == oldReceiver { @@ -851,7 +875,13 @@ func (st DBstore) RenameReceiverInNotificationSettings(ctx context.Context, orgI New: *r, }) } - return len(updates), st.UpdateAlertRules(ctx, updates) + if len(invalidProvenance) > 0 { + return nil, invalidProvenance, nil + } + if dryRun { + return result, nil, nil + } + return result, nil, st.UpdateAlertRules(ctx, updates) } // RenameTimeIntervalInNotificationSettings renames all rules that use old time interval name to the new name. diff --git a/pkg/services/ngalert/store/alert_rule_test.go b/pkg/services/ngalert/store/alert_rule_test.go index 5512b870171..da24a442772 100644 --- a/pkg/services/ngalert/store/alert_rule_test.go +++ b/pkg/services/ngalert/store/alert_rule_test.go @@ -880,35 +880,91 @@ func TestIntegrationAlertRulesNotificationSettings(t *testing.T) { } }) - t.Run("RenameReceiverInNotificationSettings should update all rules that refer to the old receiver", func(t *testing.T) { + t.Run("RenameReceiverInNotificationSettings", func(t *testing.T) { newName := "new-receiver" - affected, err := store.RenameReceiverInNotificationSettings(context.Background(), 1, receiverName, newName) - require.NoError(t, err) - require.Equal(t, len(receiveRules), affected) - expected := getKeyMap(receiveRules) - - actual, err := store.ListAlertRules(context.Background(), &models.ListAlertRulesQuery{ - OrgID: 1, - ReceiverName: newName, - }) - require.NoError(t, err) - assert.Len(t, actual, len(expected)) - for _, rule := range actual { - assert.Contains(t, expected, rule.GetKey()) + alwaysTrue := func(p models.Provenance) bool { + return true } - actual, err = store.ListAlertRules(context.Background(), &models.ListAlertRulesQuery{ - OrgID: 1, - ReceiverName: receiverName, - }) - require.NoError(t, err) - require.Empty(t, actual) - t.Run("should do nothing if no rules that match the filter", func(t *testing.T) { - affected, err := store.RenameReceiverInNotificationSettings(context.Background(), 1, receiverName, util.GenerateShortUID()) + affected, invalidProvenance, err := store.RenameReceiverInNotificationSettings(context.Background(), 1, "not-found", timeIntervalName, alwaysTrue, false) require.NoError(t, err) require.Empty(t, affected) + require.Empty(t, invalidProvenance) + }) + + t.Run("should do nothing if at least one rule has provenance that is not allowed", func(t *testing.T) { + calledTimes := 0 + alwaysFalse := func(p models.Provenance) bool { + calledTimes++ + return false + } + + affected, invalidProvenance, err := store.RenameReceiverInNotificationSettings(context.Background(), 1, receiverName, newName, alwaysFalse, false) + + var expected []models.AlertRuleKey + for _, rule := range receiveRules { + expected = append(expected, rule.GetKey()) + } + + require.NoError(t, err) + require.Empty(t, affected) + require.ElementsMatch(t, expected, invalidProvenance) + assert.Equal(t, len(expected), calledTimes) + + actual, err := store.ListAlertRules(context.Background(), &models.ListAlertRulesQuery{ + OrgID: 1, + ReceiverName: receiverName, + }) + require.NoError(t, err) + assert.Len(t, actual, len(receiveRules)) + }) + + t.Run("should do nothing if dry run is set to true", func(t *testing.T) { + affected, invalidProvenance, err := store.RenameReceiverInNotificationSettings(context.Background(), 1, receiverName, newName, alwaysTrue, true) + require.NoError(t, err) + require.Empty(t, invalidProvenance) + assert.Len(t, affected, len(receiveRules)) + expected := getKeyMap(receiveRules) + for _, key := range affected { + assert.Contains(t, expected, key) + } + + actual, err := store.ListAlertRules(context.Background(), &models.ListAlertRulesQuery{ + OrgID: 1, + ReceiverName: receiverName, + }) + require.NoError(t, err) + assert.Len(t, actual, len(receiveRules)) + }) + + t.Run("should update all rules that refer to the old receiver", func(t *testing.T) { + affected, invalidProvenance, err := store.RenameReceiverInNotificationSettings(context.Background(), 1, receiverName, newName, alwaysTrue, false) + require.NoError(t, err) + require.Empty(t, invalidProvenance) + assert.Len(t, affected, len(receiveRules)) + expected := getKeyMap(receiveRules) + for _, key := range affected { + assert.Contains(t, expected, key) + } + + actual, err := store.ListAlertRules(context.Background(), &models.ListAlertRulesQuery{ + OrgID: 1, + ReceiverName: newName, + }) + require.NoError(t, err) + assert.Len(t, actual, len(expected)) + for _, rule := range actual { + assert.Contains(t, expected, rule.GetKey()) + } + + actual, err = store.ListAlertRules(context.Background(), &models.ListAlertRulesQuery{ + OrgID: 1, + ReceiverName: receiverName, + }) + require.NoError(t, err) + require.Empty(t, actual) }) }) diff --git a/pkg/tests/apis/alerting/notifications/receivers/receiver_test.go b/pkg/tests/apis/alerting/notifications/receivers/receiver_test.go index 4a2bd218020..175e83c5d6f 100644 --- a/pkg/tests/apis/alerting/notifications/receivers/receiver_test.go +++ b/pkg/tests/apis/alerting/notifications/receivers/receiver_test.go @@ -34,6 +34,7 @@ import ( "github.com/grafana/grafana/pkg/services/folder/foldertest" "github.com/grafana/grafana/pkg/services/ngalert/api" "github.com/grafana/grafana/pkg/services/ngalert/api/tooling/definitions" + ngmodels "github.com/grafana/grafana/pkg/services/ngalert/models" "github.com/grafana/grafana/pkg/services/ngalert/notifier/channels_config" "github.com/grafana/grafana/pkg/services/ngalert/store" "github.com/grafana/grafana/pkg/services/org" @@ -103,24 +104,25 @@ func TestIntegrationResourceIdentifier(t *testing.T) { require.Equal(t, newResource.Spec, actual.Spec) }) - // TODO uncomment when renaming is supported - // t.Run("update should rename receiver if name in the specification changes", func(t *testing.T) { - // existing, err := client.Get(ctx, resourceID, v1.GetOptions{}) - // require.NoError(t, err) - // - // updated := existing.DeepCopy() - // updated.Spec.Title = "another-newReceiver" - // - // actual, err := client.Update(ctx, updated, v1.UpdateOptions{}) - // require.NoError(t, err) - // require.Equal(t, updated.Spec, actual.Spec) - // require.NotEqualf(t, updated.Name, actual.Name, "Update should change the resource name but it didn't") - // require.NotEqualf(t, updated.ResourceVersion, actual.ResourceVersion, "Update should change the resource version but it didn't") - // - // resource, err := client.Get(ctx, actual.Name, v1.GetOptions{}) - // require.NoError(t, err) - // require.Equal(t, actual, resource) - // }) + t.Run("update should rename receiver if name in the specification changes", func(t *testing.T) { + existing, err := client.Get(ctx, resourceID, v1.GetOptions{}) + require.NoError(t, err) + + updated := existing.DeepCopy() + updated.Spec.Title = "another-newReceiver" + + actual, err := client.Update(ctx, updated, v1.UpdateOptions{}) + require.NoError(t, err) + require.Equal(t, updated.Spec, actual.Spec) + require.NotEqualf(t, updated.Name, actual.Name, "Update should change the resource name but it didn't") + require.NotEqualf(t, updated.ResourceVersion, actual.ResourceVersion, "Update should change the resource version but it didn't") + + resource, err := client.Get(ctx, actual.Name, v1.GetOptions{}) + require.NoError(t, err) + require.Equal(t, actual.Spec, resource.Spec) + require.Equal(t, actual.Name, resource.Name) + require.Equal(t, actual.ResourceVersion, resource.ResourceVersion) + }) } func TestIntegrationAccessControl(t *testing.T) { @@ -813,11 +815,11 @@ func TestIntegrationReferentialIntegrity(t *testing.T) { ctx := context.Background() helper := getTestHelper(t) - // env := helper.GetEnv() - // ac := acimpl.ProvideAccessControl(env.FeatureToggles, zanzana.NewNoopClient()) - // db, err := store.ProvideDBStore(env.Cfg, env.FeatureToggles, env.SQLStore, &foldertest.FakeService{}, &dashboards.FakeDashboardService{}, ac) - // require.NoError(t, err) - // orgID := helper.Org1.Admin.Identity.GetOrgID() + env := helper.GetEnv() + ac := acimpl.ProvideAccessControl(env.FeatureToggles, zanzana.NewNoopClient()) + db, err := store.ProvideDBStore(env.Cfg, env.FeatureToggles, env.SQLStore, &foldertest.FakeService{}, &dashboards.FakeDashboardService{}, ac) + require.NoError(t, err) + orgID := helper.Org1.Admin.Identity.GetOrgID() cliCfg := helper.Org1.Admin.NewRestConfig() legacyCli := alerting.NewAlertingLegacyAPIClient(helper.GetEnv().Server.HTTPServer.Listener.Addr().String(), cliCfg.Username, cliCfg.Password) @@ -852,71 +854,63 @@ func TestIntegrationReferentialIntegrity(t *testing.T) { }) receiver := receivers.Items[idx] - // TODO uncomment when renaming is enabled - // currentRoute := legacyCli.GetRoute(t) - // currentRuleGroup := legacyCli.GetRulesGroup(t, folderUID, ruleGroup.Name) - // replace := func(input []string, oldName, newName string) []string { - // result := make([]string, 0, len(input)) - // for _, s := range input { - // if s == oldName { - // result = append(result, newName) - // continue - // } - // result = append(result, s) - // } - // return result - // } - // - // t.Run("Update", func(t *testing.T) { - // t.Run("should rename all references if name changes", func(t *testing.T) { - // renamed := receiver.DeepCopy() - // expectedTitle := renamed.Spec.Title + "-new" - // renamed.Spec.Title += expectedTitle - // - // actual, err := adminClient.Update(ctx, renamed, v1.UpdateOptions{}) - // require.NoError(t, err) - // - // updatedRuleGroup := legacyCli.GetRulesGroup(t, folderUID, ruleGroup.Name) - // for idx, rule := range updatedRuleGroup.Rules { - // assert.Equalf(t, expectedTitle, rule.GrafanaManagedAlert.NotificationSettings.Receiver, "receiver in rule %d should have been renamed but it did not", idx) - // } - // - // updatedRoute := legacyCli.GetRoute(t) - // for _, route := range updatedRoute.Routes { - // assert.Equalf(t, expectedTitle, route.Receiver, "time receiver in routes should have been renamed but it did not") - // } - // receiver = *actual - // }) - // - // t.Run("should fail if at least one resource is provisioned", func(t *testing.T) { - // require.NoError(t, err) - // renamed := receiver.DeepCopy() - // renamed.Spec.Title += util.GenerateShortUID() - // - // t.Run("provisioned route", func(t *testing.T) { - // require.NoError(t, db.SetProvenance(ctx, ¤tRoute, orgID, "API")) - // t.Cleanup(func() { - // require.NoError(t, db.DeleteProvenance(ctx, ¤tRoute, orgID)) - // }) - // actual, err := adminClient.Update(ctx, renamed, v1.UpdateOptions{}) - // require.Errorf(t, err, "Expected error but got successful result: %v", actual) - // require.Truef(t, errors.IsConflict(err), "Expected Conflict, got: %s", err) - // }) - // - // t.Run("provisioned rules", func(t *testing.T) { - // ruleUid := currentRuleGroup.Rules[0].GrafanaManagedAlert.UID - // resource := &ngmodels.AlertRule{UID: ruleUid} - // require.NoError(t, db.SetProvenance(ctx, resource, orgID, "API")) - // t.Cleanup(func() { - // require.NoError(t, db.DeleteProvenance(ctx, resource, orgID)) - // }) - // - // actual, err := adminClient.Update(ctx, renamed, v1.UpdateOptions{}) - // require.Errorf(t, err, "Expected error but got successful result: %v", actual) - // require.Truef(t, errors.IsConflict(err), "Expected Conflict, got: %s", err) - // }) - // }) - // }) + currentRoute := legacyCli.GetRoute(t) + currentRuleGroup := legacyCli.GetRulesGroup(t, folderUID, ruleGroup.Name) + + t.Run("Update", func(t *testing.T) { + t.Run("should rename all references if name changes", func(t *testing.T) { + renamed := receiver.DeepCopy() + expectedTitle := renamed.Spec.Title + "-new" + renamed.Spec.Title = expectedTitle + + actual, err := adminClient.Update(ctx, renamed, v1.UpdateOptions{}) + require.NoError(t, err) + + updatedRuleGroup := legacyCli.GetRulesGroup(t, folderUID, ruleGroup.Name) + for idx, rule := range updatedRuleGroup.Rules { + assert.Equalf(t, expectedTitle, rule.GrafanaManagedAlert.NotificationSettings.Receiver, "receiver in rule %d should have been renamed but it did not", idx) + } + + updatedRoute := legacyCli.GetRoute(t) + for _, route := range updatedRoute.Routes { + assert.Equalf(t, expectedTitle, route.Receiver, "time receiver in routes should have been renamed but it did not") + } + + actual, err = adminClient.Get(ctx, actual.Name, v1.GetOptions{}) + require.NoError(t, err) + + receiver = *actual + }) + + t.Run("should fail if at least one resource is provisioned", func(t *testing.T) { + require.NoError(t, err) + renamed := receiver.DeepCopy() + renamed.Spec.Title += util.GenerateShortUID() + + t.Run("provisioned route", func(t *testing.T) { + require.NoError(t, db.SetProvenance(ctx, ¤tRoute, orgID, "API")) + t.Cleanup(func() { + require.NoError(t, db.DeleteProvenance(ctx, ¤tRoute, orgID)) + }) + actual, err := adminClient.Update(ctx, renamed, v1.UpdateOptions{}) + require.Errorf(t, err, "Expected error but got successful result: %v", actual) + require.Truef(t, errors.IsConflict(err), "Expected Conflict, got: %s", err) + }) + + t.Run("provisioned rules", func(t *testing.T) { + ruleUid := currentRuleGroup.Rules[0].GrafanaManagedAlert.UID + resource := &ngmodels.AlertRule{UID: ruleUid} + require.NoError(t, db.SetProvenance(ctx, resource, orgID, "API")) + t.Cleanup(func() { + require.NoError(t, db.DeleteProvenance(ctx, resource, orgID)) + }) + + actual, err := adminClient.Update(ctx, renamed, v1.UpdateOptions{}) + require.Errorf(t, err, "Expected error but got successful result: %v", actual) + require.Truef(t, errors.IsConflict(err), "Expected Conflict, got: %s", err) + }) + }) + }) t.Run("Delete", func(t *testing.T) { t.Run("should fail to delete if receiver is used in rule and routes", func(t *testing.T) { diff --git a/public/app/features/alerting/unified/components/receivers/form/GrafanaReceiverForm.tsx b/public/app/features/alerting/unified/components/receivers/form/GrafanaReceiverForm.tsx index ade39241b34..a7d26ed25cd 100644 --- a/public/app/features/alerting/unified/components/receivers/form/GrafanaReceiverForm.tsx +++ b/public/app/features/alerting/unified/components/receivers/form/GrafanaReceiverForm.tsx @@ -7,7 +7,6 @@ import { useUpdateContactPoint, } from 'app/features/alerting/unified/components/contact-points/useContactPoints'; import { GRAFANA_RULES_SOURCE_NAME } from 'app/features/alerting/unified/utils/datasource'; -import { shouldUseK8sApi } from 'app/features/alerting/unified/utils/k8s/utils'; import { GrafanaManagedContactPoint, GrafanaManagedReceiverConfig, @@ -139,7 +138,6 @@ export const GrafanaReceiverForm = ({ contactPoint, readOnly = false, editMode } return { dto: n }; }); - const disableEditTitle = editMode && shouldUseK8sApi(GRAFANA_RULES_SOURCE_NAME); return ( <> {hasOnCallError && ( @@ -152,7 +150,6 @@ export const GrafanaReceiverForm = ({ contactPoint, readOnly = false, editMode } {contactPoint?.provisioned && } - disableEditTitle={disableEditTitle} isEditable={isEditable} isTestable={isTestable} onSubmit={onSubmit} diff --git a/public/app/features/alerting/unified/components/receivers/form/ReceiverForm.tsx b/public/app/features/alerting/unified/components/receivers/form/ReceiverForm.tsx index a856376bddb..b1d5d9005a0 100644 --- a/public/app/features/alerting/unified/components/receivers/form/ReceiverForm.tsx +++ b/public/app/features/alerting/unified/components/receivers/form/ReceiverForm.tsx @@ -38,11 +38,6 @@ interface Props { * and that contact point being created will be set as the default? */ showDefaultRouteWarning?: boolean; - /** - * Should we disable the title editing? Required when we're using the API server, - * as at the time of writing this is not possible - */ - disableEditTitle?: boolean; } export function ReceiverForm({ @@ -57,7 +52,6 @@ export function ReceiverForm({ isTestable, customValidators, showDefaultRouteWarning, - disableEditTitle, }: Props) { const notifyApp = useAppNotification(); const styles = useStyles2(getStyles); @@ -138,7 +132,6 @@ export function ReceiverForm({