diff --git a/pkg/models/notifications.go b/pkg/models/notifications.go index 41dafa15614..79a4ed41bfd 100644 --- a/pkg/models/notifications.go +++ b/pkg/models/notifications.go @@ -37,6 +37,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 a363ea2b9e6..3c8ae5ef592 100644 --- a/pkg/services/ngalert/notifier/channels/email_test.go +++ b/pkg/services/ngalert/notifier/channels/email_test.go @@ -10,10 +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/services/notifications" - "github.com/grafana/grafana/pkg/setting" ) func TestEmailNotifier(t *testing.T) { @@ -106,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") @@ -250,27 +248,6 @@ func TestEmailNotifierIntegration(t *testing.T) { } } -func createCoreEmailService(t *testing.T) *notifications.NotificationService { - t.Helper() - - bus := bus.New() - 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, 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 f358084fd1e..4612b472e1a 100644 --- a/pkg/services/ngalert/notifier/channels/teams.go +++ b/pkg/services/ngalert/notifier/channels/teams.go @@ -125,6 +125,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 f91322c3905..56564057ebc 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 @@ -190,6 +196,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 { @@ -203,7 +227,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) @@ -225,12 +257,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 3fac1346bc6..a23c63da002 100644 --- a/pkg/services/ngalert/notifier/channels/testing.go +++ b/pkg/services/ngalert/notifier/channels/testing.go @@ -2,9 +2,15 @@ package channels import ( "context" + "testing" "time" + "github.com/stretchr/testify/require" + + "github.com/grafana/grafana/pkg/bus" "github.com/grafana/grafana/pkg/models" + "github.com/grafana/grafana/pkg/services/notifications" + "github.com/grafana/grafana/pkg/setting" ) // mockTimeNow replaces function timeNow to return constant time. @@ -48,3 +54,24 @@ func (ns *notificationServiceMock) SendEmailCommandHandler(ctx context.Context, } func mockNotificationService() *notificationServiceMock { return ¬ificationServiceMock{} } + +func CreateNotificationService(t *testing.T) *notifications.NotificationService { + t.Helper() + + bus := bus.New() + 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 7c86ebb8348..84a0d42cb6c 100644 --- a/pkg/services/notifications/notifications.go +++ b/pkg/services/notifications/notifications.go @@ -126,6 +126,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) }