diff --git a/pkg/services/alerting/notifiers/slack.go b/pkg/services/alerting/notifiers/slack.go index f119800dbed..c4e73eb334c 100644 --- a/pkg/services/alerting/notifiers/slack.go +++ b/pkg/services/alerting/notifiers/slack.go @@ -382,15 +382,17 @@ func (sn *SlackNotifier) sendRequest(ctx context.Context, data []byte) error { return fmt.Errorf("failed to read response body: %w", err) } - if resp.StatusCode/100 == 2 { - var rslt map[string]interface{} + if resp.StatusCode >= http.StatusOK && resp.StatusCode < http.StatusMultipleChoices { // Slack responds to some requests with a JSON document, that might contain an error + rslt := struct { + Ok bool `json:"ok"` + Err string `json:"error"` + }{} if err := json.Unmarshal(body, &rslt); err == nil { - if !rslt["ok"].(bool) { - errMsg := rslt["error"].(string) + if !rslt.Ok && rslt.Err != "" { sn.log.Warn("Sending Slack API request failed", "url", sn.url.String(), "statusCode", resp.Status, - "err", errMsg) - return fmt.Errorf("failed to make Slack API request: %s", errMsg) + "err", rslt.Err) + return fmt.Errorf("failed to make Slack API request: %s", rslt.Err) } } diff --git a/pkg/services/alerting/notifiers/slack_test.go b/pkg/services/alerting/notifiers/slack_test.go index a232aac9c2b..55bf5e4cf71 100644 --- a/pkg/services/alerting/notifiers/slack_test.go +++ b/pkg/services/alerting/notifiers/slack_test.go @@ -1,6 +1,10 @@ package notifiers import ( + "context" + "fmt" + "net/http" + "net/http/httptest" "testing" "github.com/grafana/grafana/pkg/components/securejsondata" @@ -214,3 +218,102 @@ func TestSlackNotifier(t *testing.T) { assert.Equal(t, "1ABCDE", slackNotifier.recipient) }) } + +func TestSendSlackRequest(t *testing.T) { + tests := []struct { + name string + slackResponse string + statusCode int + expectError bool + }{ + { + name: "Example error", + slackResponse: `{ + "ok": false, + "error": "too_many_attachments" + }`, + statusCode: http.StatusBadRequest, + expectError: true, + }, + { + name: "Non 200 status code, no response body", + statusCode: http.StatusMovedPermanently, + expectError: true, + }, + { + name: "Success case, normal response body", + slackResponse: `{ + "ok": true, + "channel": "C1H9RESGL", + "ts": "1503435956.000247", + "message": { + "text": "Here's a message for you", + "username": "ecto1", + "bot_id": "B19LU7CSY", + "attachments": [ + { + "text": "This is an attachment", + "id": 1, + "fallback": "This is an attachment's fallback" + } + ], + "type": "message", + "subtype": "bot_message", + "ts": "1503435956.000247" + } + }`, + statusCode: http.StatusOK, + expectError: false, + }, + { + name: "Success case, no response body", + statusCode: http.StatusOK, + expectError: false, + }, + { + name: "Success case, unexpected response body", + statusCode: http.StatusOK, + slackResponse: "{}", + expectError: false, + }, + { + name: "Success case, ok: true", + statusCode: http.StatusOK, + slackResponse: "{\"ok\": true}", + expectError: false, + }, + { + name: "200 status code, error in body", + statusCode: http.StatusOK, + slackResponse: `{"ok": false, "error": "test error"}`, + expectError: true, + }, + } + + for _, test := range tests { + t.Run(test.name, func(tt *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(test.statusCode) + _, err := w.Write([]byte(test.slackResponse)) + require.NoError(tt, err) + })) + + settingsJSON, err := simplejson.NewJson([]byte(fmt.Sprintf(`{"url": %q}`, server.URL))) + require.NoError(t, err) + model := &models.AlertNotification{ + Settings: settingsJSON, + } + + not, err := NewSlackNotifier(model) + require.NoError(t, err) + slackNotifier := not.(*SlackNotifier) + + err = slackNotifier.sendRequest(context.TODO(), []byte("test")) + if !test.expectError { + require.NoError(tt, err) + } else { + require.Error(tt, err) + } + }) + } +} diff --git a/pkg/services/ngalert/notifier/channels/slack.go b/pkg/services/ngalert/notifier/channels/slack.go index 6ddda2509d4..5a9975bedce 100644 --- a/pkg/services/ngalert/notifier/channels/slack.go +++ b/pkg/services/ngalert/notifier/channels/slack.go @@ -220,19 +220,21 @@ var sendSlackRequest = func(request *http.Request, logger log.Logger) error { return fmt.Errorf("failed to read response body: %w", err) } - if resp.StatusCode/100 != 2 { + if resp.StatusCode < http.StatusOK || resp.StatusCode >= http.StatusMultipleChoices { logger.Warn("Slack API request failed", "url", request.URL.String(), "statusCode", resp.Status, "body", string(body)) return fmt.Errorf("request to Slack API failed with status code %d", resp.StatusCode) } - var rslt map[string]interface{} // Slack responds to some requests with a JSON document, that might contain an error + rslt := struct { + Ok bool `json:"ok"` + Err string `json:"error"` + }{} if err := json.Unmarshal(body, &rslt); err == nil { - if !rslt["ok"].(bool) { - errMsg := rslt["error"].(string) + if !rslt.Ok && rslt.Err != "" { logger.Warn("Sending Slack API request failed", "url", request.URL.String(), "statusCode", resp.Status, - "err", errMsg) - return fmt.Errorf("failed to make Slack API request: %s", errMsg) + "err", rslt.Err) + return fmt.Errorf("failed to make Slack API request: %s", rslt.Err) } } diff --git a/pkg/services/ngalert/notifier/channels/slack_test.go b/pkg/services/ngalert/notifier/channels/slack_test.go index ae89326adc2..4636a5350d9 100644 --- a/pkg/services/ngalert/notifier/channels/slack_test.go +++ b/pkg/services/ngalert/notifier/channels/slack_test.go @@ -5,6 +5,7 @@ import ( "encoding/json" "io" "net/http" + "net/http/httptest" "net/url" "testing" @@ -218,3 +219,94 @@ func TestSlackNotifier(t *testing.T) { }) } } + +func TestSendSlackRequest(t *testing.T) { + tests := []struct { + name string + slackResponse string + statusCode int + expectError bool + }{ + { + name: "Example error", + slackResponse: `{ + "ok": false, + "error": "too_many_attachments" + }`, + statusCode: http.StatusBadRequest, + expectError: true, + }, + { + name: "Non 200 status code, no response body", + statusCode: http.StatusMovedPermanently, + expectError: true, + }, + { + name: "Success case, normal response body", + slackResponse: `{ + "ok": true, + "channel": "C1H9RESGL", + "ts": "1503435956.000247", + "message": { + "text": "Here's a message for you", + "username": "ecto1", + "bot_id": "B19LU7CSY", + "attachments": [ + { + "text": "This is an attachment", + "id": 1, + "fallback": "This is an attachment's fallback" + } + ], + "type": "message", + "subtype": "bot_message", + "ts": "1503435956.000247" + } + }`, + statusCode: http.StatusOK, + expectError: false, + }, + { + name: "Success case, no response body", + statusCode: http.StatusOK, + expectError: false, + }, + { + name: "Success case, unexpected response body", + statusCode: http.StatusOK, + slackResponse: "{}", + expectError: false, + }, + { + name: "Success case, ok: true", + statusCode: http.StatusOK, + slackResponse: "{\"ok\": true}", + expectError: false, + }, + { + name: "200 status code, error in body", + statusCode: http.StatusOK, + slackResponse: `{"ok": false, "error": "test error"}`, + expectError: true, + }, + } + + for _, test := range tests { + t.Run(test.name, func(tt *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(test.statusCode) + _, err := w.Write([]byte(test.slackResponse)) + require.NoError(tt, err) + })) + req, err := http.NewRequest(http.MethodGet, server.URL, nil) + require.NoError(tt, err) + + err = sendSlackRequest(req, log.New("test")) + if !test.expectError { + require.NoError(tt, err) + } else { + require.Error(tt, err) + } + }) + } +}