From 27c44f4709ad203010f004134f58149b45c38cfb Mon Sep 17 00:00:00 2001 From: Yuri Tseretyan Date: Thu, 10 Oct 2024 16:26:30 -0400 Subject: [PATCH] Alerting: Update notification policy service to check provenance status (#94359) * update ResetPolicyTree to accept provenance status * update methods to check for provenance status use relaxed validation --- pkg/services/ngalert/api/api_provisioning.go | 11 +- .../ngalert/api/api_provisioning_test.go | 8 +- .../provisioning/notification_policies.go | 24 +++- .../notification_policies_test.go | 103 ++++++++++++++---- .../notification_policy_provisioner.go | 2 +- 5 files changed, 115 insertions(+), 33 deletions(-) diff --git a/pkg/services/ngalert/api/api_provisioning.go b/pkg/services/ngalert/api/api_provisioning.go index f22cf445ce5..ccdfde76c75 100644 --- a/pkg/services/ngalert/api/api_provisioning.go +++ b/pkg/services/ngalert/api/api_provisioning.go @@ -53,7 +53,7 @@ type TemplateService interface { type NotificationPolicyService interface { GetPolicyTree(ctx context.Context, orgID int64) (definitions.Route, string, error) UpdatePolicyTree(ctx context.Context, orgID int64, tree definitions.Route, p alerting_models.Provenance, version string) error - ResetPolicyTree(ctx context.Context, orgID int64) (definitions.Route, error) + ResetPolicyTree(ctx context.Context, orgID int64, provenance alerting_models.Provenance) (definitions.Route, error) } type MuteTimingService interface { @@ -84,7 +84,7 @@ func (srv *ProvisioningSrv) RouteGetPolicyTree(c *contextmodel.ReqContext) respo return ErrResp(http.StatusNotFound, err, "") } if err != nil { - return ErrResp(http.StatusInternalServerError, err, "") + return response.ErrOrFallback(http.StatusInternalServerError, "failed to get notification policy tree", err) } return response.JSON(http.StatusOK, policies) @@ -117,16 +117,17 @@ func (srv *ProvisioningSrv) RoutePutPolicyTree(c *contextmodel.ReqContext, tree return ErrResp(http.StatusBadRequest, err, "") } if err != nil { - return response.ErrOrFallback(http.StatusInternalServerError, "", err) + return response.ErrOrFallback(http.StatusInternalServerError, "failed to update notification policy tree", err) } return response.JSON(http.StatusAccepted, util.DynMap{"message": "policies updated"}) } func (srv *ProvisioningSrv) RouteResetPolicyTree(c *contextmodel.ReqContext) response.Response { - tree, err := srv.policies.ResetPolicyTree(c.Req.Context(), c.SignedInUser.GetOrgID()) + provenance := determineProvenance(c) + tree, err := srv.policies.ResetPolicyTree(c.Req.Context(), c.SignedInUser.GetOrgID(), alerting_models.Provenance(provenance)) if err != nil { - return ErrResp(http.StatusInternalServerError, err, "") + return response.ErrOrFallback(http.StatusInternalServerError, "failed to reset notification policy tree", err) } return response.JSON(http.StatusAccepted, tree) } diff --git a/pkg/services/ngalert/api/api_provisioning_test.go b/pkg/services/ngalert/api/api_provisioning_test.go index e6747915ae6..6b418e3b6e4 100644 --- a/pkg/services/ngalert/api/api_provisioning_test.go +++ b/pkg/services/ngalert/api/api_provisioning_test.go @@ -140,7 +140,6 @@ func TestProvisioningApi(t *testing.T) { require.Equal(t, 500, response.Status()) require.NotEmpty(t, response.Body()) - require.Contains(t, string(response.Body()), "something went wrong") }) t.Run("PUT returns 500", func(t *testing.T) { @@ -164,7 +163,6 @@ func TestProvisioningApi(t *testing.T) { require.Equal(t, 500, response.Status()) require.NotEmpty(t, response.Body()) - require.Contains(t, string(response.Body()), "something went wrong") }) }) }) @@ -2002,7 +2000,7 @@ func (f *fakeNotificationPolicyService) UpdatePolicyTree(ctx context.Context, or return nil } -func (f *fakeNotificationPolicyService) ResetPolicyTree(ctx context.Context, orgID int64) (definitions.Route, error) { +func (f *fakeNotificationPolicyService) ResetPolicyTree(ctx context.Context, orgID int64, provenance models.Provenance) (definitions.Route, error) { f.tree = definitions.Route{} // TODO return f.tree, nil } @@ -2017,7 +2015,7 @@ func (f *fakeFailingNotificationPolicyService) UpdatePolicyTree(ctx context.Cont return fmt.Errorf("something went wrong") } -func (f *fakeFailingNotificationPolicyService) ResetPolicyTree(ctx context.Context, orgID int64) (definitions.Route, error) { +func (f *fakeFailingNotificationPolicyService) ResetPolicyTree(ctx context.Context, orgID int64, provenance models.Provenance) (definitions.Route, error) { return definitions.Route{}, fmt.Errorf("something went wrong") } @@ -2031,7 +2029,7 @@ func (f *fakeRejectingNotificationPolicyService) UpdatePolicyTree(ctx context.Co return fmt.Errorf("%w: invalid policy tree", provisioning.ErrValidation) } -func (f *fakeRejectingNotificationPolicyService) ResetPolicyTree(ctx context.Context, orgID int64) (definitions.Route, error) { +func (f *fakeRejectingNotificationPolicyService) ResetPolicyTree(ctx context.Context, orgID int64, provenance models.Provenance) (definitions.Route, error) { return definitions.Route{}, nil } diff --git a/pkg/services/ngalert/provisioning/notification_policies.go b/pkg/services/ngalert/provisioning/notification_policies.go index 8eeeeff5211..afba5f0c0d9 100644 --- a/pkg/services/ngalert/provisioning/notification_policies.go +++ b/pkg/services/ngalert/provisioning/notification_policies.go @@ -16,6 +16,7 @@ import ( "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/legacy_storage" + "github.com/grafana/grafana/pkg/services/ngalert/provisioning/validation" "github.com/grafana/grafana/pkg/setting" ) @@ -25,6 +26,7 @@ type NotificationPolicyService struct { xact TransactionManager log log.Logger settings setting.UnifiedAlertingSettings + validator validation.ProvenanceStatusTransitionValidator } func NewNotificationPolicyService(am alertmanagerConfigStore, prov ProvisioningStore, @@ -35,6 +37,7 @@ func NewNotificationPolicyService(am alertmanagerConfigStore, prov ProvisioningS xact: xact, log: log, settings: settings, + validator: validation.ValidateProvenanceRelaxed, } } @@ -69,11 +72,20 @@ func (nps *NotificationPolicyService) UpdatePolicyTree(ctx context.Context, orgI return err } - err = nps.checkOptimisticConcurrency(*revision.Config.AlertmanagerConfig.Route, models.Provenance(tree.Provenance), version, "update") + err = nps.checkOptimisticConcurrency(*revision.Config.AlertmanagerConfig.Route, p, version, "update") if err != nil { return err } + // check that provenance is not changed in an invalid way + storedProvenance, err := nps.provenanceStore.GetProvenance(ctx, &tree, orgID) + if err != nil { + return err + } + if err := nps.validator(storedProvenance, p); err != nil { + return err + } + receivers, err := nps.receiversToMap(revision.Config.AlertmanagerConfig.Receivers) if err != nil { return err @@ -107,7 +119,15 @@ func (nps *NotificationPolicyService) UpdatePolicyTree(ctx context.Context, orgI }) } -func (nps *NotificationPolicyService) ResetPolicyTree(ctx context.Context, orgID int64) (definitions.Route, error) { +func (nps *NotificationPolicyService) ResetPolicyTree(ctx context.Context, orgID int64, provenance models.Provenance) (definitions.Route, error) { + storedProvenance, err := nps.provenanceStore.GetProvenance(ctx, &definitions.Route{}, orgID) + if err != nil { + return definitions.Route{}, err + } + if err := nps.validator(storedProvenance, provenance); err != nil { + return definitions.Route{}, err + } + defaultCfg, err := legacy_storage.DeserializeAlertmanagerConfig([]byte(nps.settings.DefaultConfiguration)) if err != nil { nps.log.Error("Failed to parse default alertmanager config: %w", err) diff --git a/pkg/services/ngalert/provisioning/notification_policies_test.go b/pkg/services/ngalert/provisioning/notification_policies_test.go index e67fb83be32..9fa49c8f5f1 100644 --- a/pkg/services/ngalert/provisioning/notification_policies_test.go +++ b/pkg/services/ngalert/provisioning/notification_policies_test.go @@ -2,6 +2,7 @@ package provisioning import ( "context" + "errors" "testing" "github.com/grafana/alerting/definition" @@ -102,7 +103,10 @@ func TestUpdatePolicyTree(t *testing.T) { t.Run("ErrValidation if referenced receiver does not exist", func(t *testing.T) { rev := getDefaultConfigRevision() - sut, store, _ := createNotificationPolicyServiceSut() + sut, store, prov := createNotificationPolicyServiceSut() + prov.GetProvenanceFunc = func(ctx context.Context, o models.Provisionable, org int64) (models.Provenance, error) { + return models.ProvenanceNone, nil + } store.GetFn = func(ctx context.Context, orgID int64) (*legacy_storage.ConfigRevision, error) { return &rev, nil } @@ -137,6 +141,35 @@ func TestUpdatePolicyTree(t *testing.T) { require.ErrorIs(t, err, ErrVersionConflict) }) + t.Run("Error if provenance validation fails", func(t *testing.T) { + sut, store, prov := createNotificationPolicyServiceSut() + prov.GetProvenanceFunc = func(ctx context.Context, o models.Provisionable, org int64) (models.Provenance, error) { + return models.ProvenanceAPI, nil + } + store.GetFn = func(ctx context.Context, orgID int64) (*legacy_storage.ConfigRevision, error) { + return &rev, nil + } + expectedRev := getDefaultConfigRevision() + route := newRoute + expectedRev.ConcurrencyToken = rev.ConcurrencyToken + expectedRev.Config.AlertmanagerConfig.Route = &route + + expectedErr := errors.New("test") + sut.validator = func(from, to models.Provenance) error { + assert.Equal(t, models.ProvenanceAPI, from) + assert.Equal(t, models.ProvenanceNone, to) + return expectedErr + } + + err := sut.UpdatePolicyTree(context.Background(), orgID, newRoute, models.ProvenanceNone, defaultVersion) + require.ErrorIs(t, err, expectedErr) + + assert.Len(t, prov.Calls, 1) + assert.Equal(t, "GetProvenance", prov.Calls[0].MethodName) + assert.IsType(t, &definitions.Route{}, prov.Calls[0].Arguments[1]) + assert.Equal(t, orgID, prov.Calls[0].Arguments[2].(int64)) + }) + t.Run("updates Route and sets provenance in transaction if route is valid and version matches", func(t *testing.T) { sut, store, prov := createNotificationPolicyServiceSut() store.GetFn = func(ctx context.Context, orgID int64) (*legacy_storage.ConfigRevision, error) { @@ -155,12 +188,16 @@ func TestUpdatePolicyTree(t *testing.T) { assertInTransaction(t, store.Calls[1].Args[0].(context.Context)) assert.Equal(t, &expectedRev, store.Calls[1].Args[1]) - assert.Len(t, prov.Calls, 1) - assert.Equal(t, "SetProvenance", prov.Calls[0].MethodName) - assertInTransaction(t, prov.Calls[0].Arguments[0].(context.Context)) - assert.IsType(t, &definitions.Route{}, prov.Calls[0].Arguments[1]) - assert.Equal(t, orgID, prov.Calls[0].Arguments[2].(int64)) - assert.Equal(t, models.ProvenanceAPI, prov.Calls[0].Arguments[3].(models.Provenance)) + c := prov.Calls[0] + assert.Equal(t, "GetProvenance", c.MethodName) + assert.IsType(t, &definitions.Route{}, c.Arguments[1]) + assert.Equal(t, orgID, c.Arguments[2].(int64)) + c = prov.Calls[1] + assert.Equal(t, "SetProvenance", c.MethodName) + assertInTransaction(t, c.Arguments[0].(context.Context)) + assert.IsType(t, &definitions.Route{}, c.Arguments[1]) + assert.Equal(t, orgID, c.Arguments[2].(int64)) + assert.Equal(t, models.ProvenanceAPI, c.Arguments[3].(models.Provenance)) }) t.Run("bypasses optimistic concurrency if provided version is empty", func(t *testing.T) { @@ -181,12 +218,13 @@ func TestUpdatePolicyTree(t *testing.T) { assertInTransaction(t, store.Calls[1].Args[0].(context.Context)) assert.Equal(t, &expectedRev, store.Calls[1].Args[1]) - assert.Len(t, prov.Calls, 1) - assert.Equal(t, "SetProvenance", prov.Calls[0].MethodName) - assertInTransaction(t, prov.Calls[0].Arguments[0].(context.Context)) - assert.IsType(t, &definitions.Route{}, prov.Calls[0].Arguments[1]) - assert.Equal(t, orgID, prov.Calls[0].Arguments[2].(int64)) - assert.Equal(t, models.ProvenanceAPI, prov.Calls[0].Arguments[3].(models.Provenance)) + assert.Len(t, prov.Calls, 2) + c := prov.Calls[1] + assert.Equal(t, "SetProvenance", c.MethodName) + assertInTransaction(t, c.Arguments[0].(context.Context)) + assert.IsType(t, &definitions.Route{}, c.Arguments[1]) + assert.Equal(t, orgID, c.Arguments[2].(int64)) + assert.Equal(t, models.ProvenanceAPI, c.Arguments[3].(models.Provenance)) }) } @@ -223,10 +261,27 @@ func TestResetPolicyTree(t *testing.T) { sut.settings = setting.UnifiedAlertingSettings{ DefaultConfiguration: "{", } - _, err := sut.ResetPolicyTree(context.Background(), orgID) + _, err := sut.ResetPolicyTree(context.Background(), orgID, models.ProvenanceNone) require.ErrorContains(t, err, "failed to parse default alertmanager config") }) + t.Run("Error if provenance validation fails", func(t *testing.T) { + sut, _, prov := createNotificationPolicyServiceSut() + prov.GetProvenanceFunc = func(ctx context.Context, o models.Provisionable, org int64) (models.Provenance, error) { + return models.ProvenanceAPI, nil + } + + expectedErr := errors.New("test") + sut.validator = func(from, to models.Provenance) error { + assert.Equal(t, models.ProvenanceAPI, from) + assert.Equal(t, models.ProvenanceNone, to) + return expectedErr + } + + _, err := sut.ResetPolicyTree(context.Background(), orgID, models.ProvenanceNone) + require.ErrorIs(t, err, expectedErr) + }) + t.Run("replaces route with one from the default config and copies receivers if do not exist", func(t *testing.T) { defaultConfig := getDefaultConfigRevision().Config data, err := legacy_storage.SerializeAlertmanagerConfig(*defaultConfig) @@ -252,7 +307,7 @@ func TestResetPolicyTree(t *testing.T) { expectedRev.Config.AlertmanagerConfig.Route = getDefaultConfigRevision().Config.AlertmanagerConfig.Route expectedRev.Config.AlertmanagerConfig.Receivers = append(expectedRev.Config.AlertmanagerConfig.Receivers, getDefaultConfigRevision().Config.AlertmanagerConfig.Receivers[0]) - tree, err := sut.ResetPolicyTree(context.Background(), orgID) + tree, err := sut.ResetPolicyTree(context.Background(), orgID, models.ProvenanceNone) require.NoError(t, err) assert.Equal(t, *defaultConfig.AlertmanagerConfig.Route, tree) @@ -262,11 +317,16 @@ func TestResetPolicyTree(t *testing.T) { resetRev := store.Calls[1].Args[1].(*legacy_storage.ConfigRevision) assert.Equal(t, expectedRev.Config.AlertmanagerConfig, resetRev.Config.AlertmanagerConfig) - assert.Len(t, prov.Calls, 1) - assert.Equal(t, "DeleteProvenance", prov.Calls[0].MethodName) - assertInTransaction(t, prov.Calls[0].Arguments[0].(context.Context)) - assert.IsType(t, &definitions.Route{}, prov.Calls[0].Arguments[1]) - assert.Equal(t, orgID, prov.Calls[0].Arguments[2]) + assert.Len(t, prov.Calls, 2) + c := prov.Calls[0] + assert.Equal(t, "GetProvenance", c.MethodName) + assert.IsType(t, &definitions.Route{}, c.Arguments[1]) + assert.Equal(t, orgID, c.Arguments[2].(int64)) + c = prov.Calls[1] + assert.Equal(t, "DeleteProvenance", c.MethodName) + assertInTransaction(t, c.Arguments[0].(context.Context)) + assert.IsType(t, &definitions.Route{}, c.Arguments[1]) + assert.Equal(t, orgID, c.Arguments[2]) }) } @@ -286,6 +346,9 @@ func createNotificationPolicyServiceSut() (*NotificationPolicyService, *legacy_s settings: setting.UnifiedAlertingSettings{ DefaultConfiguration: setting.GetAlertmanagerDefaultConfiguration(), }, + validator: func(from, to models.Provenance) error { + return nil + }, }, configStore, prov } diff --git a/pkg/services/provisioning/alerting/notification_policy_provisioner.go b/pkg/services/provisioning/alerting/notification_policy_provisioner.go index 55f3dd2ae9d..63f6cbe9ce3 100644 --- a/pkg/services/provisioning/alerting/notification_policy_provisioner.go +++ b/pkg/services/provisioning/alerting/notification_policy_provisioner.go @@ -45,7 +45,7 @@ func (c *defaultNotificationPolicyProvisioner) Unprovision(ctx context.Context, files []*AlertingFile) error { for _, file := range files { for _, orgID := range file.ResetPolicies { - _, err := c.notificationPolicyService.ResetPolicyTree(ctx, int64(orgID)) + _, err := c.notificationPolicyService.ResetPolicyTree(ctx, int64(orgID), models.ProvenanceFile) if err != nil { return fmt.Errorf("%s: %w", file.Filename, err) }