From e64cde8727b819b8011a9242bc0eb3caa1ec3b1a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jean-Philippe=20Qu=C3=A9m=C3=A9ner?= Date: Tue, 5 Jul 2022 18:09:57 +0200 Subject: [PATCH] Alerting: validate that the receiver exist when updating routing tree (#51561) * Alerting: validate that the receiver exist when updating routing tree * rename interface * add missing file * change constructor * fix e2e tests * only import package once * add unit test for bug * wording * close response body * Update pkg/services/ngalert/api/tooling/definitions/alertmanager_validation.go * refactor to remove database roundtrip --- .../definitions/alertmanager_validation.go | 13 +++++ .../provisioning/notification_policies.go | 18 ++++++- .../notification_policies_test.go | 53 +++++++++++++++++++ .../api/alerting/api_provisioning_test.go | 22 +++++++- 4 files changed, 103 insertions(+), 3 deletions(-) diff --git a/pkg/services/ngalert/api/tooling/definitions/alertmanager_validation.go b/pkg/services/ngalert/api/tooling/definitions/alertmanager_validation.go index 5b6365ea412..c6759f245f5 100644 --- a/pkg/services/ngalert/api/tooling/definitions/alertmanager_validation.go +++ b/pkg/services/ngalert/api/tooling/definitions/alertmanager_validation.go @@ -101,6 +101,19 @@ func (r *Route) Validate() error { return r.validateChild() } +func (r *Route) ValidateReceivers(receivers map[string]struct{}) error { + if _, exists := receivers[r.Receiver]; !exists { + return fmt.Errorf("receiver '%s' does not exist", r.Receiver) + } + for _, children := range r.Routes { + err := children.ValidateReceivers(receivers) + if err != nil { + return err + } + } + return nil +} + func (mt *MuteTimeInterval) Validate() error { s, err := yaml.Marshal(mt.MuteTimeInterval) if err != nil { diff --git a/pkg/services/ngalert/provisioning/notification_policies.go b/pkg/services/ngalert/provisioning/notification_policies.go index f03611fc86d..b16a6377dd4 100644 --- a/pkg/services/ngalert/provisioning/notification_policies.go +++ b/pkg/services/ngalert/provisioning/notification_policies.go @@ -16,7 +16,8 @@ type NotificationPolicyService struct { log log.Logger } -func NewNotificationPolicyService(am AMConfigStore, prov ProvisioningStore, xact TransactionManager, log log.Logger) *NotificationPolicyService { +func NewNotificationPolicyService(am AMConfigStore, prov ProvisioningStore, + xact TransactionManager, log log.Logger) *NotificationPolicyService { return &NotificationPolicyService{ amStore: am, provenanceStore: prov, @@ -63,11 +64,18 @@ func (nps *NotificationPolicyService) UpdatePolicyTree(ctx context.Context, orgI if err != nil { return fmt.Errorf("%w: %s", ErrValidation, err.Error()) } + revision, err := getLastConfiguration(ctx, orgID, nps.amStore) if err != nil { return err } + receivers, err := nps.receiversToMap(revision.cfg.AlertmanagerConfig.Receivers) + err = tree.ValidateReceivers(receivers) + if err != nil { + return fmt.Errorf("%w: %s", ErrValidation, err.Error()) + } + revision.cfg.AlertmanagerConfig.Config.Route = &tree serialized, err := serializeAlertmanagerConfig(*revision.cfg) @@ -98,3 +106,11 @@ func (nps *NotificationPolicyService) UpdatePolicyTree(ctx context.Context, orgI return nil } + +func (nps *NotificationPolicyService) receiversToMap(records []*definitions.PostableApiReceiver) (map[string]struct{}, error) { + receivers := map[string]struct{}{} + for _, receiver := range records { + receivers[receiver.Name] = struct{}{} + } + return receivers, nil +} diff --git a/pkg/services/ngalert/provisioning/notification_policies_test.go b/pkg/services/ngalert/provisioning/notification_policies_test.go index df98eb4f7c6..6b0c2bb5a00 100644 --- a/pkg/services/ngalert/provisioning/notification_policies_test.go +++ b/pkg/services/ngalert/provisioning/notification_policies_test.go @@ -7,7 +7,9 @@ import ( "github.com/grafana/grafana/pkg/infra/log" "github.com/grafana/grafana/pkg/services/ngalert/api/tooling/definitions" "github.com/grafana/grafana/pkg/services/ngalert/models" + "github.com/prometheus/alertmanager/config" "github.com/prometheus/common/model" + mock "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" ) @@ -23,6 +25,7 @@ func TestNotificationPolicyService(t *testing.T) { t.Run("service stitches policy tree into org's AM config", func(t *testing.T) { sut := createNotificationPolicyServiceSut() + newRoute := createTestRoutingTree() err := sut.UpdatePolicyTree(context.Background(), 1, newRoute, models.ProvenanceNone) @@ -33,6 +36,56 @@ func TestNotificationPolicyService(t *testing.T) { require.Equal(t, "a new receiver", updated.Receiver) }) + t.Run("not existing receiver reference will error", func(t *testing.T) { + sut := createNotificationPolicyServiceSut() + + newRoute := createTestRoutingTree() + newRoute.Routes = append(newRoute.Routes, &definitions.Route{ + Receiver: "not-existing", + }) + + err := sut.UpdatePolicyTree(context.Background(), 1, newRoute, models.ProvenanceNone) + require.Error(t, err) + }) + + t.Run("existing receiver reference will pass", func(t *testing.T) { + sut := createNotificationPolicyServiceSut() + sut.amStore = &MockAMConfigStore{} + sut.amStore.(*MockAMConfigStore).On("GetLatestAlertmanagerConfiguration", mock.Anything, mock.Anything). + Return( + func(ctx context.Context, query *models.GetLatestAlertmanagerConfigurationQuery) error { + cfg, _ := deserializeAlertmanagerConfig([]byte(defaultConfig)) + cfg.AlertmanagerConfig.Receivers = append(cfg.AlertmanagerConfig.Receivers, + &definitions.PostableApiReceiver{ + Receiver: config.Receiver{ + // default one from createTestRoutingTree() + Name: "a new receiver", + }, + }) + cfg.AlertmanagerConfig.Receivers = append(cfg.AlertmanagerConfig.Receivers, + &definitions.PostableApiReceiver{ + Receiver: config.Receiver{ + Name: "existing", + }, + }) + data, _ := serializeAlertmanagerConfig(*cfg) + query.Result = &models.AlertConfiguration{ + AlertmanagerConfiguration: string(data), + } + return nil + }) + sut.amStore.(*MockAMConfigStore).EXPECT(). + UpdateAlertmanagerConfiguration(mock.Anything, mock.Anything). + Return(nil) + newRoute := createTestRoutingTree() + newRoute.Routes = append(newRoute.Routes, &definitions.Route{ + Receiver: "existing", + }) + + err := sut.UpdatePolicyTree(context.Background(), 1, newRoute, models.ProvenanceNone) + require.NoError(t, err) + }) + t.Run("default provenance of records is none", func(t *testing.T) { sut := createNotificationPolicyServiceSut() diff --git a/pkg/tests/api/alerting/api_provisioning_test.go b/pkg/tests/api/alerting/api_provisioning_test.go index fb1a02df474..f4d78dde729 100644 --- a/pkg/tests/api/alerting/api_provisioning_test.go +++ b/pkg/tests/api/alerting/api_provisioning_test.go @@ -44,13 +44,32 @@ func TestProvisioning(t *testing.T) { url := fmt.Sprintf("http://%s/api/v1/provisioning/policies", grafanaListedAddr) body := ` { - "receiver": "grafana-default-email", + "receiver": "test-receiver", "group_by": [ "..." ], "routes": [] }` + // As we check if the receiver exists that is referenced in the policy, + // we first need to create it, so the tests passes correctly. + urlReceiver := fmt.Sprintf("http://%s/api/v1/provisioning/contact-points", grafanaListedAddr) + bodyReceiver := ` + { + "name": "test-receiver", + "type": "slack", + "settings": { + "recipient": "value_recipient", + "token": "value_token" + } + }` + + req := createTestRequest("POST", urlReceiver, "admin", bodyReceiver) + resp, err := http.DefaultClient.Do(req) + require.NoError(t, err) + require.NoError(t, resp.Body.Close()) + require.Equal(t, 202, resp.StatusCode) + t.Run("un-authenticated GET should 401", func(t *testing.T) { req := createTestRequest("GET", url, "", "") @@ -127,7 +146,6 @@ func TestProvisioning(t *testing.T) { resp, err := http.DefaultClient.Do(req) require.NoError(t, err) require.NoError(t, resp.Body.Close()) - require.Equal(t, 202, resp.StatusCode) }) })