diff --git a/pkg/models/notifications.go b/pkg/models/notifications.go index c4e4289a68b..159d6a04edb 100644 --- a/pkg/models/notifications.go +++ b/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 { diff --git a/pkg/services/ngalert/notifier/channels/email_test.go b/pkg/services/ngalert/notifier/channels/email_test.go index 8bfb69de391..da621f7e646 100644 --- a/pkg/services/ngalert/notifier/channels/email_test.go +++ b/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() diff --git a/pkg/services/ngalert/notifier/channels/teams.go b/pkg/services/ngalert/notifier/channels/teams.go index fd45764a5c5..634569dd9bc 100644 --- a/pkg/services/ngalert/notifier/channels/teams.go +++ b/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") } diff --git a/pkg/services/ngalert/notifier/channels/teams_test.go b/pkg/services/ngalert/notifier/channels/teams_test.go index 68834b881d2..c52f805dc93 100644 --- a/pkg/services/ngalert/notifier/channels/teams_test.go +++ b/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)), + } +} diff --git a/pkg/services/ngalert/notifier/channels/testing.go b/pkg/services/ngalert/notifier/channels/testing.go index af7467cc62e..2234d909b69 100644 --- a/pkg/services/ngalert/notifier/channels/testing.go +++ b/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 ¬ificationServiceMock{} } + +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 +} diff --git a/pkg/services/notifications/notifications.go b/pkg/services/notifications/notifications.go index 261fa13a5f0..a753f088bdf 100644 --- a/pkg/services/notifications/notifications.go +++ b/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, }) } diff --git a/pkg/services/notifications/testing.go b/pkg/services/notifications/testing.go index 325ea0cc878..ad06b2bef1a 100644 --- a/pkg/services/notifications/testing.go +++ b/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 +} diff --git a/pkg/services/notifications/webhook.go b/pkg/services/notifications/webhook.go index 9c790c3c549..15acd8ea2fd 100644 --- a/pkg/services/notifications/webhook.go +++ b/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) }