diff --git a/notify/slack/slack.go b/notify/slack/slack.go index 3c2ec056f2..06a7a076e9 100644 --- a/notify/slack/slack.go +++ b/notify/slack/slack.go @@ -210,16 +210,66 @@ func (n *Notifier) Notify(ctx context.Context, as ...*types.Alert) (bool, error) if err != nil { return true, notify.RedactURL(err) } - defer notify.Drain(resp) + defer resp.Body.Close() - // Only 5xx response codes are recoverable and 2xx codes are successful. - // https://api.slack.com/incoming-webhooks#handling_errors - // https://api.slack.com/changelog/2016-05-17-changes-to-errors-for-incoming-webhooks + // Use a retrier to generate an error message for non-200 responses and + // classify them as retriable or not. retry, err := n.retrier.Check(resp.StatusCode, resp.Body) if err != nil { + io.Copy(io.Discard, resp.Body) err = errors.Wrap(err, fmt.Sprintf("channel %q", req.Channel)) return retry, notify.NewErrorWithReason(notify.GetFailureReasonFromStatusCode(resp.StatusCode), err) } + // Slack web API might return errors with a 200 response code. + // https://slack.dev/node-slack-sdk/web-api#handle-errors + retry, err = checkResponseError(resp) + if err != nil { + err = errors.Wrap(err, fmt.Sprintf("channel %q", req.Channel)) + return retry, notify.NewErrorWithReason(notify.ClientErrorReason, err) + } + + return retry, nil +} + +// checkResponseError parses out the error message from Slack API response. +func checkResponseError(resp *http.Response) (bool, error) { + body, err := io.ReadAll(resp.Body) + if err != nil { + return true, errors.Wrap(err, "could not read response body") + } + + if strings.HasPrefix(resp.Header.Get("Content-Type"), "application/json") { + return checkJSONResponseError(body) + } + return checkTextResponseError(body) +} + +// checkTextResponseError classifies plaintext responses from Slack. +func checkTextResponseError(body []byte) (bool, error) { + // A plaintext (non-JSON) response is successful if it's a string "ok". + // This is typically a response for an Incoming Webhook + // (https://api.slack.com/messaging/webhooks#handling_errors) + if bytes.Equal(body, []byte("ok")) { + return false, nil + } + return false, fmt.Errorf("received an error response from Slack: %s", string(body)) +} + +// checkJSONResponseError classifies JSON responses from Slack. +func checkJSONResponseError(body []byte) (bool, error) { + // response is for parsing out errors from the JSON response. + type response struct { + OK bool `json:"ok"` + Error string `json:"error"` + } + + var data response + if err := json.Unmarshal(body, &data); err != nil { + return false, errors.Wrapf(err, "could not unmarshal JSON response %q", string(body)) + } + if !data.OK { + return false, fmt.Errorf("error response from Slack: %s", data.Error) + } return false, nil } diff --git a/notify/slack/slack_test.go b/notify/slack/slack_test.go index 627b97fb5d..ffe9450ff4 100644 --- a/notify/slack/slack_test.go +++ b/notify/slack/slack_test.go @@ -21,6 +21,7 @@ import ( "net/http/httptest" "net/url" "os" + "strings" "testing" "time" @@ -116,28 +117,75 @@ func TestNotifier_Notify_WithReason(t *testing.T) { tests := []struct { name string statusCode int + responseBody string expectedReason notify.Reason + expectedErr string + expectedRetry bool noError bool }{ { name: "with a 4xx status code", statusCode: http.StatusUnauthorized, expectedReason: notify.ClientErrorReason, + expectedRetry: false, + expectedErr: "unexpected status code 401", }, { name: "with a 5xx status code", statusCode: http.StatusInternalServerError, expectedReason: notify.ServerErrorReason, + expectedRetry: true, + expectedErr: "unexpected status code 500", }, { - name: "with any other status code", + name: "with a 3xx status code", statusCode: http.StatusTemporaryRedirect, expectedReason: notify.DefaultReason, + expectedRetry: false, + expectedErr: "unexpected status code 307", }, { - name: "with a 2xx status code", - statusCode: http.StatusOK, - noError: true, + name: "with a 1xx status code", + statusCode: http.StatusSwitchingProtocols, + expectedReason: notify.DefaultReason, + expectedRetry: false, + expectedErr: "unexpected status code 101", + }, + { + name: "2xx response with invalid JSON", + statusCode: http.StatusOK, + responseBody: `{"not valid json"}`, + expectedReason: notify.ClientErrorReason, + expectedRetry: false, + expectedErr: "could not unmarshal", + }, + { + name: "2xx response with a JSON error", + statusCode: http.StatusOK, + responseBody: `{"ok":false,"error":"error_message"}`, + expectedReason: notify.ClientErrorReason, + expectedRetry: false, + expectedErr: "error response from Slack: error_message", + }, + { + name: "2xx response with a plaintext error", + statusCode: http.StatusOK, + responseBody: "no_channel", + expectedReason: notify.ClientErrorReason, + expectedRetry: false, + expectedErr: "error response from Slack: no_channel", + }, + { + name: "successful JSON response", + statusCode: http.StatusOK, + responseBody: `{"ok":true}`, + noError: true, + }, + { + name: "successful plaintext response", + statusCode: http.StatusOK, + responseBody: "ok", + noError: true, }, } for _, tt := range tests { @@ -148,6 +196,7 @@ func TestNotifier_Notify_WithReason(t *testing.T) { NotifierConfig: config.NotifierConfig{}, HTTPConfig: &commoncfg.HTTPClientConfig{}, APIURL: &config.SecretURL{URL: apiurl}, + Channel: "channelname", }, test.CreateTmpl(t), log.NewNopLogger(), @@ -156,7 +205,11 @@ func TestNotifier_Notify_WithReason(t *testing.T) { notifier.postJSONFunc = func(ctx context.Context, client *http.Client, url string, body io.Reader) (*http.Response, error) { resp := httptest.NewRecorder() + if strings.HasPrefix(tt.responseBody, "{") { + resp.Header().Add("Content-Type", "application/json; charset=utf-8") + } resp.WriteHeader(tt.statusCode) + resp.WriteString(tt.responseBody) return resp.Result(), nil } ctx := context.Background() @@ -168,13 +221,16 @@ func TestNotifier_Notify_WithReason(t *testing.T) { EndsAt: time.Now().Add(time.Hour), }, } - _, err = notifier.Notify(ctx, alert1) + retry, err := notifier.Notify(ctx, alert1) + require.Equal(t, tt.expectedRetry, retry) if tt.noError { require.NoError(t, err) } else { reasonError, ok := err.(*notify.ErrorWithReason) require.True(t, ok) require.Equal(t, tt.expectedReason, reasonError.Reason) + require.Contains(t, err.Error(), tt.expectedErr) + require.Contains(t, err.Error(), "channelname") } }) }