Treat error response payloads from Slack as errors (#3121)

As described in the "More error types" section below, Slack API can return
errors with a 200 response code:
https://slack.dev/node-slack-sdk/web-api#handle-errors

This change adds parsing of API response to extract error messages.

Signed-off-by: Anton Tolchanov <anton@tailscale.com>

notify/slack/slack.go

@@ -212,14 +212,63 @@ func (n *Notifier) Notify(ctx context.Context, as ...*types.Alert) (bool, error)
 	}
 	defer notify.Drain(resp)
 
-	// 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 {
 		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.
+// 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)
+func checkTextResponseError(body []byte) (bool, error) {
+	if !bytes.Equal(body, []byte("ok")) {
+		return false, fmt.Errorf("received an error response from Slack: %s", string(body))
+	}
+	return false, nil
+}
+
+// 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 true, 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
 }

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:  true,
+			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")
 			}
 		})
 	}