Alerting: Fix Teams notifier not failing on 200 response with error (#52254)

Team's webhook API does not always use the status code to communicate errors.
There are cases where it returns 200 and an error message in the body.
For example, 429 - Too Many Requests or when the message is too large.
Instead, what we should be looking for is a response body = "1".

https://docs.microsoft.com/en-us/microsoftteams/platform/webhooks-and-connectors/how-to/connectors-using?tabs=cURL#send-messages-using-curl-and-powershell

pkg/models/notifications.go

@@ -41,6 +41,7 @@ type SendWebhookSync struct {
 	HttpMethod  string
 	HttpHeader  map[string]string
 	ContentType string
+	Validation  func(body []byte, statusCode int) error
 }
 
 type SendResetPasswordEmailCommand struct {

pkg/services/ngalert/notifier/channels/email_test.go

@@ -10,11 +10,8 @@ import (
 	"github.com/prometheus/common/model"
 	"github.com/stretchr/testify/require"
 
-	"github.com/grafana/grafana/pkg/bus"
 	"github.com/grafana/grafana/pkg/components/simplejson"
-	"github.com/grafana/grafana/pkg/infra/tracing"
 	"github.com/grafana/grafana/pkg/services/notifications"
-	"github.com/grafana/grafana/pkg/setting"
 )
 
 func TestEmailNotifier(t *testing.T) {
@@ -107,7 +104,7 @@ func TestEmailNotifier(t *testing.T) {
 }
 
 func TestEmailNotifierIntegration(t *testing.T) {
-	ns := createCoreEmailService(t)
+	ns := CreateNotificationService(t)
 
 	emailTmpl := templateForTests(t)
 	externalURL, err := url.Parse("http://localhost/base")
@@ -266,29 +263,6 @@ func TestEmailNotifierIntegration(t *testing.T) {
 	}
 }
 
-func createCoreEmailService(t *testing.T) *notifications.NotificationService {
-	t.Helper()
-
-	tracer := tracing.InitializeTracerForTest()
-	bus := bus.ProvideBus(tracer)
-
-	cfg := setting.NewCfg()
-	cfg.StaticRootPath = "../../../../../public/"
-	cfg.BuildVersion = "4.0.0"
-	cfg.Smtp.Enabled = true
-	cfg.Smtp.TemplatesPatterns = []string{"emails/*.html", "emails/*.txt"}
-	cfg.Smtp.FromAddress = "from@address.com"
-	cfg.Smtp.FromName = "Grafana Admin"
-	cfg.Smtp.ContentTypes = []string{"text/html", "text/plain"}
-	cfg.Smtp.Host = "localhost:1234"
-	mailer := notifications.NewFakeMailer()
-
-	ns, err := notifications.ProvideService(bus, cfg, mailer, nil)
-	require.NoError(t, err)
-
-	return ns
-}
-
 func createSut(t *testing.T, messageTmpl string, subjectTmpl string, emailTmpl *template.Template, ns notifications.EmailSender) *EmailNotifier {
 	t.Helper()
 

pkg/services/ngalert/notifier/channels/teams.go

@@ -93,7 +93,7 @@ func (tn *TeamsNotifier) Notify(ctx context.Context, as ...*types.Alert) (bool,
 
 	ruleURL := joinUrlPath(tn.tmpl.ExternalURL.String(), "/alerting/list", tn.log)
 
-	images := []teamsImage{}
+	var images []teamsImage
 	_ = withStoredImages(ctx, tn.log, tn.images,
 		func(_ int, image ngmodels.Image) error {
 			if len(image.URL) != 0 {
@@ -157,6 +157,20 @@ func (tn *TeamsNotifier) Notify(ctx context.Context, as ...*types.Alert) (bool,
 	}
 	cmd := &models.SendWebhookSync{Url: u, Body: string(b)}
 
+	// Teams does not always return non-2xx response when the request fails. Instead, the response body can contain an error message regardless of status code.
+	// Ex. 429 - Too Many Requests: https://docs.microsoft.com/en-us/microsoftteams/platform/webhooks-and-connectors/how-to/connectors-using?tabs=cURL#rate-limiting-for-connectors
+	cmd.Validation = func(b []byte, statusCode int) error {
+		body := string(b)
+
+		// https://docs.microsoft.com/en-us/microsoftteams/platform/webhooks-and-connectors/how-to/connectors-using?tabs=cURL#send-messages-using-curl-and-powershell
+		// Above states that if the POST succeeds, you must see a simple "1" output.
+		if body != "1" {
+			return errors.New(body)
+		}
+
+		return nil
+	}
+
 	if err := tn.ns.SendWebhookSync(ctx, cmd); err != nil {
 		return false, errors.Wrap(err, "send notification to Teams")
 	}

pkg/services/ngalert/notifier/channels/teams_test.go

@@ -3,7 +3,11 @@ package channels
 import (
 	"context"
 	"encoding/json"
+	"errors"
+	"io/ioutil"
+	"net/http"
 	"net/url"
+	"strings"
 	"testing"
 
 	"github.com/prometheus/alertmanager/notify"
@@ -12,6 +16,7 @@ import (
 	"github.com/stretchr/testify/require"
 
 	"github.com/grafana/grafana/pkg/components/simplejson"
+	"github.com/grafana/grafana/pkg/services/notifications"
 )
 
 func TestTeamsNotifier(t *testing.T) {
@@ -25,6 +30,7 @@ func TestTeamsNotifier(t *testing.T) {
 		name         string
 		settings     string
 		alerts       []*types.Alert
+		response     *mockResponse
 		expMsg       map[string]interface{}
 		expInitError string
 		expMsgError  error
@@ -196,6 +202,24 @@ func TestTeamsNotifier(t *testing.T) {
 			settings:     `{}`,
 			expInitError: `could not find url property in settings`,
 		},
+		{
+			name:     "webhook returns error message in body with 200",
+			settings: `{"url": "http://localhost"}`,
+			alerts: []*types.Alert{
+				{
+					Alert: model.Alert{
+						Labels:      model.LabelSet{"alertname": "alert1", "lbl1": "val1"},
+						Annotations: model.LabelSet{"ann1": "annv1", "__dashboardUid__": "abcd", "__panelId__": "efgh"},
+					},
+				},
+			},
+			response: &mockResponse{
+				status: 200,
+				body:   "some error message",
+				error:  nil,
+			},
+			expMsgError: errors.New("send notification to Teams: webhook failed validation: some error message"),
+		},
 	}
 
 	for _, c := range cases {
@@ -209,7 +233,15 @@ func TestTeamsNotifier(t *testing.T) {
 				Settings: settingsJSON,
 			}
 
-			webhookSender := mockNotificationService()
+			webhookSender := CreateNotificationService(t)
+
+			originalClient := notifications.NetClient
+			defer func() {
+				notifications.SetWebhookClient(*originalClient)
+			}()
+			clientStub := newMockClient(c.response)
+			notifications.SetWebhookClient(clientStub)
+
 			cfg, err := NewTeamsConfig(m)
 			if c.expInitError != "" {
 				require.Error(t, err)
@@ -231,12 +263,54 @@ func TestTeamsNotifier(t *testing.T) {
 			require.True(t, ok)
 			require.NoError(t, err)
 
-			require.NotEmpty(t, webhookSender.Webhook.Url)
+			require.NotEmpty(t, clientStub.lastRequest.URL.String())
 
 			expBody, err := json.Marshal(c.expMsg)
 			require.NoError(t, err)
 
-			require.JSONEq(t, string(expBody), webhookSender.Webhook.Body)
+			body, err := ioutil.ReadAll(clientStub.lastRequest.Body)
+			require.NoError(t, err)
+			require.JSONEq(t, string(expBody), string(body))
 		})
 	}
 }
+
+type mockClient struct {
+	response    mockResponse
+	lastRequest *http.Request
+}
+
+type mockResponse struct {
+	status int
+	body   string
+	error  error
+}
+
+func (c *mockClient) Do(req *http.Request) (*http.Response, error) {
+	// Do Nothing
+	c.lastRequest = req
+	return makeResponse(c.response.status, c.response.body), c.response.error
+}
+
+func newMockClient(resp *mockResponse) *mockClient {
+	client := &mockClient{}
+
+	if resp != nil {
+		client.response = *resp
+	} else {
+		client.response = mockResponse{
+			status: 200,
+			body:   "1",
+			error:  nil,
+		}
+	}
+
+	return client
+}
+
+func makeResponse(status int, body string) *http.Response {
+	return &http.Response{
+		StatusCode: status,
+		Body:       ioutil.NopCloser(strings.NewReader(body)),
+	}
+}

pkg/services/ngalert/notifier/channels/testing.go

@@ -8,8 +8,14 @@ import (
 	"testing"
 	"time"
 
+	"github.com/stretchr/testify/require"
+
+	"github.com/grafana/grafana/pkg/bus"
+	"github.com/grafana/grafana/pkg/infra/tracing"
 	"github.com/grafana/grafana/pkg/models"
 	ngmodels "github.com/grafana/grafana/pkg/services/ngalert/models"
+	"github.com/grafana/grafana/pkg/services/notifications"
+	"github.com/grafana/grafana/pkg/setting"
 )
 
 type fakeImageStore struct {
@@ -142,3 +148,26 @@ func (ns *notificationServiceMock) SendEmailCommandHandler(ctx context.Context,
 }
 
 func mockNotificationService() *notificationServiceMock { return &notificationServiceMock{} }
+
+func CreateNotificationService(t *testing.T) *notifications.NotificationService {
+	t.Helper()
+
+	tracer := tracing.InitializeTracerForTest()
+	bus := bus.ProvideBus(tracer)
+
+	cfg := setting.NewCfg()
+	cfg.StaticRootPath = "../../../../../public/"
+	cfg.BuildVersion = "4.0.0"
+	cfg.Smtp.Enabled = true
+	cfg.Smtp.TemplatesPatterns = []string{"emails/*.html", "emails/*.txt"}
+	cfg.Smtp.FromAddress = "from@address.com"
+	cfg.Smtp.FromName = "Grafana Admin"
+	cfg.Smtp.ContentTypes = []string{"text/html", "text/plain"}
+	cfg.Smtp.Host = "localhost:1234"
+	mailer := notifications.NewFakeMailer()
+
+	ns, err := notifications.ProvideService(bus, cfg, mailer, nil)
+	require.NoError(t, err)
+
+	return ns
+}

pkg/services/notifications/notifications.go

@@ -127,6 +127,7 @@ func (ns *NotificationService) SendWebhookSync(ctx context.Context, cmd *models.
 		HttpMethod:  cmd.HttpMethod,
 		HttpHeader:  cmd.HttpHeader,
 		ContentType: cmd.ContentType,
+		Validation:  cmd.Validation,
 	})
 }
 

pkg/services/notifications/testing.go

@@ -30,3 +30,11 @@ func NewFakeDisconnectedMailer() *FakeDisconnectedMailer {
 func (fdm *FakeDisconnectedMailer) Send(messages ...*Message) (int, error) {
 	return 0, fmt.Errorf("connect: connection refused")
 }
+
+// NetClient is used to export original in test.
+var NetClient = &netClient
+
+// SetWebhookClient is used to mock in test.
+func SetWebhookClient(client WebhookClient) {
+	netClient = client
+}

pkg/services/notifications/webhook.go

@@ -5,7 +5,6 @@ import (
 	"context"
 	"crypto/tls"
 	"fmt"
-	"io"
 	"io/ioutil"
 	"net"
 	"net/http"
@@ -22,6 +21,15 @@ type Webhook struct {
 	HttpMethod  string
 	HttpHeader  map[string]string
 	ContentType string
+
+	// Validation is a function that will validate the response body and statusCode of the webhook. Any returned error will cause the webhook request to be considered failed.
+	// This can be useful when a webhook service communicates failures in creative ways, such as using the response body instead of the status code.
+	Validation func(body []byte, statusCode int) error
+}
+
+// WebhookClient exists to mock the client in tests.
+type WebhookClient interface {
+	Do(req *http.Request) (*http.Response, error)
 }
 
 var netTransport = &http.Transport{
@@ -34,7 +42,7 @@ var netTransport = &http.Transport{
 	}).Dial,
 	TLSHandshakeTimeout: 5 * time.Second,
 }
-var netClient = &http.Client{
+var netClient WebhookClient = &http.Client{
 	Timeout:   time.Second * 30,
 	Transport: netTransport,
 }
@@ -80,20 +88,24 @@ func (ns *NotificationService) sendWebRequestSync(ctx context.Context, webhook *
 		}
 	}()
 
-	if resp.StatusCode/100 == 2 {
-		ns.log.Debug("Webhook succeeded", "url", webhook.Url, "statuscode", resp.Status)
-		// flushing the body enables the transport to reuse the same connection
-		if _, err := io.Copy(ioutil.Discard, resp.Body); err != nil {
-			ns.log.Error("Failed to copy resp.Body to ioutil.Discard", "err", err)
-		}
-		return nil
-	}
-
 	body, err := ioutil.ReadAll(resp.Body)
 	if err != nil {
 		return err
 	}
 
+	if webhook.Validation != nil {
+		err := webhook.Validation(body, resp.StatusCode)
+		if err != nil {
+			ns.log.Debug("Webhook failed validation", "url", webhook.Url, "statuscode", resp.Status, "body", string(body))
+			return fmt.Errorf("webhook failed validation: %w", err)
+		}
+	}
+
+	if resp.StatusCode/100 == 2 {
+		ns.log.Debug("Webhook succeeded", "url", webhook.Url, "statuscode", resp.Status)
+		return nil
+	}
+
 	ns.log.Debug("Webhook failed", "url", webhook.Url, "statuscode", resp.Status, "body", string(body))
-	return fmt.Errorf("Webhook response status %v", resp.Status)
+	return fmt.Errorf("webhook response status %v", resp.Status)
 }