Treat error response payloads from Slack as errors

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

@@ -92,6 +92,12 @@ type attachment struct {
 	MrkdwnIn   []string             `json:"mrkdwn_in,omitempty"`
 }
 
+// response is for parsing out errors from the JSON response.
+type response struct {
+	OK    bool   `json:"ok"`
+	Error string `json:"error"`
+}
+
 // Notify implements the Notifier interface.
 func (n *Notifier) Notify(ctx context.Context, as ...*types.Alert) (bool, error) {
 	var err error
@@ -210,14 +216,39 @@ 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
 	retry, err := n.retrier.Check(resp.StatusCode, resp.Body)
 	err = errors.Wrap(err, fmt.Sprintf("channel %q", req.Channel))
-	reasonErr := notify.NewErrorWithReason(notify.GetFailureReasonFromStatusCode(resp.StatusCode), err)
+	if err != nil {
+		io.Copy(io.Discard, resp.Body)
+		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 = respError(resp)
+	if err != nil {
+		return retry, notify.NewErrorWithReason(notify.ClientErrorReason, err)
+	}
 
-	return retry, reasonErr
+	return retry, nil
+	// return retry, notify.NewErrorWithReason(notify.GetFailureReasonFromStatusCode(resp.StatusCode), err)
+}
+
+// respError parses out the error message from Slack API response.
+func respError(resp *http.Response) (bool, error) {
+	body, err := io.ReadAll(resp.Body)
+	if err != nil {
+		return true, errors.Wrap(err, "could not read response body")
+	}
+	var data response
+	err = json.Unmarshal(body, &data)
+	if 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

@@ -114,9 +114,11 @@ func TestTrimmingSlackURLFromFile(t *testing.T) {
 
 func TestNotifier_Notify_WithReason(t *testing.T) {
 	tests := []struct {
-		name           string
-		statusCode     int
-		expectedReason notify.Reason
+		name            string
+		statusCode      int
+		responseBody    string
+		expectedSuccess bool
+		expectedReason  notify.Reason
 	}{
 		{
 			name:           "with a 4xx status code",
@@ -129,10 +131,22 @@ func TestNotifier_Notify_WithReason(t *testing.T) {
 			expectedReason: notify.ServerErrorReason,
 		},
 		{
-			name:           "with any other status code",
+			name:           "with a 3xx status code",
 			statusCode:     http.StatusTemporaryRedirect,
 			expectedReason: notify.DefaultReason,
 		},
+		{
+			name:           "error with a 2xx status code",
+			statusCode:     http.StatusOK,
+			responseBody:   `{"ok":false,"error":"error_message"}`,
+			expectedReason: notify.ClientErrorReason,
+		},
+		{
+			name:            "successful 2xx response",
+			statusCode:      http.StatusOK,
+			responseBody:    `{"ok":true}`,
+			expectedSuccess: true,
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
@@ -151,6 +165,7 @@ 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()
 				resp.WriteHeader(tt.statusCode)
+				resp.Write([]byte(tt.responseBody))
 				return resp.Result(), nil
 			}
 			ctx := context.Background()
@@ -163,9 +178,53 @@ func TestNotifier_Notify_WithReason(t *testing.T) {
 				},
 			}
 			_, err = notifier.Notify(ctx, alert1)
+			if tt.expectedSuccess && err == nil {
+				return
+			}
 			reasonError, ok := err.(*notify.ErrorWithReason)
 			require.True(t, ok)
 			require.Equal(t, tt.expectedReason, reasonError.Reason)
 		})
 	}
 }
+
+func TestSlackErrorResponse(t *testing.T) {
+	var response *string
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.Write([]byte(*response))
+	}))
+	u, err := url.Parse(srv.URL)
+	require.NoError(t, err)
+
+	notifier, err := New(
+		&config.SlackConfig{
+			APIURL:     &config.SecretURL{URL: u},
+			HTTPConfig: &commoncfg.HTTPClientConfig{},
+		},
+		test.CreateTmpl(t),
+		log.NewNopLogger(),
+	)
+	require.NoError(t, err)
+
+	for _, tt := range []struct {
+		name      string
+		response  string
+		wantRetry bool
+		wantErr   string
+	}{
+		{"no error", `{"ok":true,"error":"none"}`, false, ""},
+		// Slack web api returns errors with a 200 response code.
+		// https://slack.dev/node-slack-sdk/web-api#handle-errors
+		{"error response", `{"ok":false,"error":"channel_not_found"}`, false, "channel_not_found"},
+		{"invalid json", "plaintext_response", true, "plaintext_response"},
+	} {
+		t.Run(tt.name, func(t *testing.T) {
+			response = &tt.response
+			retry, err := notifier.Notify(context.Background(), &types.Alert{Alert: model.Alert{Labels: model.LabelSet{"foo": "bar"}}})
+			require.Equal(t, tt.wantRetry, retry)
+			if tt.wantErr != "" {
+				require.ErrorContains(t, err, tt.wantErr)
+			}
+		})
+	}
+}