From e9fd191065c699018da96f499b19a83c7972ecd0 Mon Sep 17 00:00:00 2001 From: Steve Simpson Date: Fri, 5 Jul 2024 19:06:37 +0200 Subject: [PATCH] Alerting: Fix some status codes returned from provisioning API. (#90117) The contact point deletion API was returning 500 when it should have been returning a 4xx error, when the contact point is in use: - When in use by a notificiation policy, we were missing the `.Errorf("")` to convert `errutil.Base` into `errutil.Error`. - When in use by an alert rule, an regular error was returned. --- .../ngalert/provisioning/contactpoints.go | 4 +- pkg/services/ngalert/provisioning/errors.go | 3 +- .../api/alerting/api_provisioning_test.go | 154 ++++++++++++++++++ 3 files changed, 158 insertions(+), 3 deletions(-) diff --git a/pkg/services/ngalert/provisioning/contactpoints.go b/pkg/services/ngalert/provisioning/contactpoints.go index 1a57046e526..c475f09ffcf 100644 --- a/pkg/services/ngalert/provisioning/contactpoints.go +++ b/pkg/services/ngalert/provisioning/contactpoints.go @@ -339,7 +339,7 @@ func (ecp *ContactPointService) DeleteContactPoint(ctx context.Context, orgID in } } if fullRemoval && isContactPointInUse(name, []*apimodels.Route{revision.cfg.AlertmanagerConfig.Route}) { - return ErrContactPointReferenced + return ErrContactPointReferenced.Errorf("") } return ecp.xact.InTransaction(ctx, func(ctx context.Context) error { @@ -354,7 +354,7 @@ func (ecp *ContactPointService) DeleteContactPoint(ctx context.Context, orgID in uids = append(uids, key.UID) } ecp.log.Error("Cannot delete contact point because it is used in rule's notification settings", "receiverName", name, "rulesUid", strings.Join(uids, ",")) - return fmt.Errorf("contact point '%s' is currently used in notification settings by one or many alert rules", name) + return ErrContactPointUsedInRule.Errorf("") } } diff --git a/pkg/services/ngalert/provisioning/errors.go b/pkg/services/ngalert/provisioning/errors.go index 45d8e3eb533..0f95920f880 100644 --- a/pkg/services/ngalert/provisioning/errors.go +++ b/pkg/services/ngalert/provisioning/errors.go @@ -28,7 +28,8 @@ var ( ErrTimeIntervalInvalid = errutil.BadRequest("alerting.notifications.time-intervals.invalidFormat").MustTemplate("Invalid format of the submitted time interval", errutil.WithPublic("Time interval is in invalid format. Correct the payload and try again.")) ErrTimeIntervalInUse = errutil.Conflict("alerting.notifications.time-intervals.used", errutil.WithPublicMessage("Time interval is used by one or many notification policies")) - ErrContactPointReferenced = errutil.BadRequest("alerting.notifications.contact-points.referenced", errutil.WithPublicMessage("Contact point is currently referenced by a notification policy.")) + ErrContactPointReferenced = errutil.Conflict("alerting.notifications.contact-points.referenced", errutil.WithPublicMessage("Contact point is currently referenced by a notification policy.")) + ErrContactPointUsedInRule = errutil.Conflict("alerting.notifications.contact-points.used-by-rule", errutil.WithPublicMessage("Contact point is currently used in the notification settings of one or many alert rules.")) ) func makeErrBadAlertmanagerConfiguration(err error) error { diff --git a/pkg/tests/api/alerting/api_provisioning_test.go b/pkg/tests/api/alerting/api_provisioning_test.go index f56b1df0cb6..c16eff9ce74 100644 --- a/pkg/tests/api/alerting/api_provisioning_test.go +++ b/pkg/tests/api/alerting/api_provisioning_test.go @@ -10,13 +10,16 @@ import ( "sort" "strings" "testing" + "time" "github.com/prometheus/alertmanager/config" "github.com/prometheus/alertmanager/timeinterval" + "github.com/prometheus/common/model" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/grafana/grafana/pkg/apimachinery/errutil" + "github.com/grafana/grafana/pkg/expr" "github.com/grafana/grafana/pkg/services/ngalert/api/tooling/definitions" "github.com/grafana/grafana/pkg/services/ngalert/models" "github.com/grafana/grafana/pkg/services/org" @@ -24,6 +27,50 @@ import ( "github.com/grafana/grafana/pkg/tests/testinfra" ) +func createRuleWithNotificationSettings(t *testing.T, client apiClient, folder string, nfSettings *definitions.AlertRuleNotificationSettings) (definitions.PostableRuleGroupConfig, string) { + t.Helper() + + interval, err := model.ParseDuration("1m") + require.NoError(t, err) + doubleInterval := 2 * interval + rules := definitions.PostableRuleGroupConfig{ + Name: "arulegroup", + Interval: interval, + Rules: []definitions.PostableExtendedRuleNode{ + { + ApiRuleNode: &definitions.ApiRuleNode{ + For: &doubleInterval, + Labels: map[string]string{"label1": "val1"}, + Annotations: map[string]string{"annotation1": "val1"}, + }, + GrafanaManagedAlert: &definitions.PostableGrafanaRule{ + Title: fmt.Sprintf("rule under folder %s", folder), + Condition: "A", + Data: []definitions.AlertQuery{ + { + RefID: "A", + RelativeTimeRange: definitions.RelativeTimeRange{ + From: definitions.Duration(time.Duration(5) * time.Hour), + To: definitions.Duration(time.Duration(3) * time.Hour), + }, + DatasourceUID: expr.DatasourceUID, + Model: json.RawMessage(`{ + "type": "math", + "expression": "2 + 3 > 1" + }`), + }, + }, + NotificationSettings: nfSettings, + }, + }, + }, + } + resp, status, _ := client.PostRulesGroupWithStatus(t, folder, &rules) + assert.Equal(t, http.StatusAccepted, status) + require.Len(t, resp.Created, 1) + return rules, resp.Created[0] +} + func TestIntegrationProvisioning(t *testing.T) { testinfra.SQLiteIntegrationTest(t) @@ -258,6 +305,113 @@ func TestIntegrationProvisioning(t *testing.T) { require.Equal(t, 202, resp.StatusCode) }) + + createContactPoint := func(t *testing.T, name string) definitions.EmbeddedContactPoint { + cpBody := fmt.Sprintf(` + { + "name": "%s", + "type": "slack", + "settings": { + "recipient": "value_recipient", + "token": "value_token" + } + }`, name) + + req := createTestRequest("POST", url, "admin", cpBody) + + resp, err := http.DefaultClient.Do(req) + require.NoError(t, err) + require.Equal(t, 202, resp.StatusCode) + + ecp := definitions.EmbeddedContactPoint{} + require.NoError(t, json.NewDecoder(resp.Body).Decode(&ecp)) + require.NoError(t, resp.Body.Close()) + + return ecp + } + + createPolicyForContactPoint := func(t *testing.T, receiver string) { + url := fmt.Sprintf("http://%s/api/v1/provisioning/policies", grafanaListedAddr) + body := fmt.Sprintf(` + { + "receiver": "%s", + "group_by": [ + "..." + ], + "routes": [] + }`, receiver) + + req := createTestRequest("PUT", url, "admin", body) + + resp, err := http.DefaultClient.Do(req) + require.NoError(t, err) + require.NoError(t, resp.Body.Close()) + require.Equal(t, 202, resp.StatusCode) + } + + t.Run("viewer DELETE should 403", func(t *testing.T) { + ecp := createContactPoint(t, "my-contact-point") + + deleteURL := fmt.Sprintf("http://%s/api/v1/provisioning/contact-points/%s", grafanaListedAddr, ecp.UID) + req := createTestRequest("DELETE", deleteURL, "viewer", body) + + resp, err := http.DefaultClient.Do(req) + require.NoError(t, err) + require.NoError(t, resp.Body.Close()) + require.Equal(t, 403, resp.StatusCode) + }) + + t.Run("admin DELETE should succeed", func(t *testing.T) { + ecp := createContactPoint(t, "my-contact-point") + + deleteURL := fmt.Sprintf("http://%s/api/v1/provisioning/contact-points/%s", grafanaListedAddr, ecp.UID) + req := createTestRequest("DELETE", deleteURL, "admin", "") + + resp, err := http.DefaultClient.Do(req) + require.NoError(t, err) + require.NoError(t, resp.Body.Close()) + require.Equal(t, 202, resp.StatusCode) + }) + + t.Run("admin DELETE should 409 when contact point used by notification policy", func(t *testing.T) { + ecp := createContactPoint(t, "my-cp-used-by-policy") + + createPolicyForContactPoint(t, "my-cp-used-by-policy") + + deleteURL := fmt.Sprintf("http://%s/api/v1/provisioning/contact-points/%s", grafanaListedAddr, ecp.UID) + deleteReq := createTestRequest("DELETE", deleteURL, "admin", "") + + resp, err := http.DefaultClient.Do(deleteReq) + require.NoError(t, err) + require.Equal(t, 409, resp.StatusCode) + var validationError errutil.PublicError + assert.NoError(t, json.NewDecoder(resp.Body).Decode(&validationError)) + require.NoError(t, resp.Body.Close()) + assert.NotEmpty(t, validationError, validationError.Message) + assert.Equal(t, "alerting.notifications.contact-points.referenced", validationError.MessageID) + }) + + t.Run("admin DELETE should 409 when contact point used by rule", func(t *testing.T) { + ecp := createContactPoint(t, "my-cp-used-by-rule") + + nfSettings := &definitions.AlertRuleNotificationSettings{ + Receiver: "my-cp-used-by-rule", + } + apiClient := newAlertingApiClient(grafanaListedAddr, "admin", "admin") + createRuleWithNotificationSettings(t, apiClient, namespaceUID, nfSettings) + + deleteURL := fmt.Sprintf("http://%s/api/v1/provisioning/contact-points/%s", grafanaListedAddr, ecp.UID) + deleteReq := createTestRequest("DELETE", deleteURL, "admin", "") + + resp, err := http.DefaultClient.Do(deleteReq) + require.NoError(t, err) + require.Equal(t, 409, resp.StatusCode) + var validationError errutil.PublicError + assert.NoError(t, json.NewDecoder(resp.Body).Decode(&validationError)) + require.NoError(t, resp.Body.Close()) + assert.NotEmpty(t, validationError, validationError.Message) + assert.Equal(t, "alerting.notifications.contact-points.used-by-rule", validationError.MessageID) + }) }) t.Run("when provisioning templates", func(t *testing.T) {