From 9759eeda17158e6784c075c5b19c2fade4558535 Mon Sep 17 00:00:00 2001 From: Joe Blubaugh Date: Thu, 2 Jun 2022 17:18:35 +0800 Subject: [PATCH] Alerting: Remove Image Upload code from Slack notifier. (#50062) The image file upload code as it is now simply doesn't work - it's missing several important steps in the file upload process. There is more information in the fixed issue as to the steps required. After this change, screenshots will still be attached to slack messages when external image storage is used with Grafana (an S3 bucket, for example). Fixes #50056 --- .../ngalert/notifier/channels/slack.go | 107 ++-------------- .../ngalert/notifier/channels/slack_test.go | 117 ------------------ 2 files changed, 8 insertions(+), 216 deletions(-) diff --git a/pkg/services/ngalert/notifier/channels/slack.go b/pkg/services/ngalert/notifier/channels/slack.go index e1132d3c819..d6b76f8dbfd 100644 --- a/pkg/services/ngalert/notifier/channels/slack.go +++ b/pkg/services/ngalert/notifier/channels/slack.go @@ -8,8 +8,6 @@ import ( "errors" "fmt" "io" - "math/rand" - "mime/multipart" "net" "net/http" "net/url" @@ -27,7 +25,6 @@ import ( ) var SlackAPIEndpoint = "https://slack.com/api/chat.postMessage" -var SlackImageAPIEndpoint = "https://slack.com/api/files.upload" // SlackNotifier is responsible for sending // alert notification to Slack. @@ -39,7 +36,6 @@ type SlackNotifier struct { webhookSender notifications.WebhookSender URL *url.URL - ImageUploadURL string Username string IconEmoji string IconURL string @@ -55,7 +51,6 @@ type SlackNotifier struct { type SlackConfig struct { *NotificationChannelConfig URL *url.URL - ImageUploadURL string Username string IconEmoji string IconURL string @@ -83,7 +78,6 @@ func NewSlackConfig(factoryConfig FactoryConfig) (*SlackConfig, error) { channelConfig := factoryConfig.Config decryptFunc := factoryConfig.DecryptFunc endpointURL := channelConfig.Settings.Get("endpointUrl").MustString(SlackAPIEndpoint) - imageUploadURL := channelConfig.Settings.Get("imageUploadUrl").MustString(SlackImageAPIEndpoint) slackURL := decryptFunc(context.Background(), channelConfig.SecureSettings, "url", channelConfig.Settings.Get("url").MustString()) if slackURL == "" { slackURL = endpointURL @@ -127,7 +121,6 @@ func NewSlackConfig(factoryConfig FactoryConfig) (*SlackConfig, error) { MentionUsers: mentionUsers, MentionGroups: mentionGroups, URL: apiURL, - ImageUploadURL: imageUploadURL, Username: channelConfig.Settings.Get("username").MustString("Grafana"), IconEmoji: channelConfig.Settings.Get("icon_emoji").MustString(), IconURL: channelConfig.Settings.Get("icon_url").MustString(), @@ -152,7 +145,6 @@ func NewSlackNotifier(config *SlackConfig, Settings: config.Settings, }), URL: config.URL, - ImageUploadURL: config.ImageUploadURL, Recipient: config.Recipient, MentionUsers: config.MentionUsers, MentionGroups: config.MentionGroups, @@ -196,6 +188,7 @@ type attachment struct { // Notify sends an alert notification to Slack. func (sn *SlackNotifier) Notify(ctx context.Context, alerts ...*types.Alert) (bool, error) { + sn.log.Debug("building slack message", "alerts", len(alerts)) msg, err := sn.buildSlackMessage(ctx, alerts) if err != nil { return false, fmt.Errorf("build slack message: %w", err) @@ -227,46 +220,18 @@ func (sn *SlackNotifier) Notify(ctx context.Context, alerts ...*types.Alert) (bo return false, err } - var imgData io.ReadCloser - - // Try to upload if we have an image path but no image URL. This uploads the file - // immediately after the message. A bit of a hack, but it doesn't require the - // user to have an image host set up. - // TODO: We need a refactoring so we don't do two database reads for the same data. - if len(msg.Attachments[0].ImageURL) == 0 { - _ = withStoredImage(ctx, sn.log, sn.images, - func(index int, image *ngmodels.Image) error { - if image == nil || len(image.Path) == 0 { - return nil - } - - imgData, err = openImage(image.Path) - if err != nil { - imgData = nil - } - - return nil - }, - 0, alerts...) - - if imgData != nil { - defer func() { - _ = imgData.Close() - }() - - err = sn.slackFileUpload(ctx, imgData, sn.Recipient, sn.Token) - if err != nil { - sn.log.Warn("Error reading screenshot data from ImageStore: %v", err) - } - } - } - return true, nil } // sendSlackRequest sends a request to the Slack API. // Stubbable by tests. -var sendSlackRequest = func(request *http.Request, logger log.Logger) error { +var sendSlackRequest = func(request *http.Request, logger log.Logger) (retErr error) { + defer func() { + if retErr != nil { + logger.Warn("failed to send slack request", "err", retErr) + } + }() + netTransport := &http.Transport{ TLSClientConfig: &tls.Config{ Renegotiation: tls.RenegotiateFreelyAsClient, @@ -407,59 +372,3 @@ func (sn *SlackNotifier) buildSlackMessage(ctx context.Context, alrts []*types.A func (sn *SlackNotifier) SendResolved() bool { return !sn.GetDisableResolveMessage() } - -func (sn *SlackNotifier) slackFileUpload(ctx context.Context, data io.Reader, recipient, token string) error { - sn.log.Info("Uploading to slack via file.upload API") - headers, uploadBody, err := sn.generateFileUploadBody(data, token, recipient) - if err != nil { - return err - } - cmd := &models.SendWebhookSync{ - Url: sn.ImageUploadURL, Body: uploadBody.String(), HttpHeader: headers, HttpMethod: "POST", - } - if err := sn.webhookSender.SendWebhookSync(ctx, cmd); err != nil { - sn.log.Error("Failed to upload slack image", "error", err, "webhook", "file.upload") - return err - } - return nil -} - -func (sn *SlackNotifier) generateFileUploadBody(data io.Reader, token string, recipient string) (map[string]string, bytes.Buffer, error) { - // Slack requires all POSTs to files.upload to present - // an "application/x-www-form-urlencoded" encoded querystring - // See https://api.slack.com/methods/files.upload - var b bytes.Buffer - w := multipart.NewWriter(&b) - defer func() { - if err := w.Close(); err != nil { - // Shouldn't matter since we already close w explicitly on the non-error path - sn.log.Warn("Failed to close multipart writer", "err", err) - } - }() - - // TODO: perhaps we should pass the filename through to here to use the local name. - // https://github.com/grafana/grafana/issues/49375 - fw, err := w.CreateFormFile("file", fmt.Sprintf("screenshot-%v", rand.Intn(2e6))) - if err != nil { - return nil, b, err - } - if _, err := io.Copy(fw, data); err != nil { - return nil, b, err - } - // Add the authorization token - if err := w.WriteField("token", token); err != nil { - return nil, b, err - } - // Add the channel(s) to POST to - if err := w.WriteField("channels", recipient); err != nil { - return nil, b, err - } - if err := w.Close(); err != nil { - return nil, b, fmt.Errorf("failed to close multipart writer: %w", err) - } - headers := map[string]string{ - "Content-Type": w.FormDataContentType(), - "Authorization": "auth_token=\"" + token + "\"", - } - return headers, b, nil -} diff --git a/pkg/services/ngalert/notifier/channels/slack_test.go b/pkg/services/ngalert/notifier/channels/slack_test.go index 82fc387a986..83899b44f59 100644 --- a/pkg/services/ngalert/notifier/channels/slack_test.go +++ b/pkg/services/ngalert/notifier/channels/slack_test.go @@ -39,14 +39,6 @@ func TestSlackNotifier(t *testing.T) { Token: "test-with-url", URL: "https://www.example.com/image.jpg", }, - { - Token: "test-with-path-not-found", - Path: "usr/home/nouser/noway.jpg", - }, - { - Token: "test-with-path-found", - Path: f.Name(), // Has the full path because of how CreateTemp works. - }, }, } @@ -172,78 +164,6 @@ func TestSlackNotifier(t *testing.T) { }, expMsgError: nil, }, - { - name: "Image URL with path but no file creates message with no error", - settings: `{ - "token": "1234", - "image_upload_url": "https://www.webhook.com", - "recipient": "#testchannel", - "icon_emoji": ":emoji:" - }`, - alerts: []*types.Alert{ - { - Alert: model.Alert{ - Labels: model.LabelSet{"alertname": "alert1", "lbl1": "val1"}, - Annotations: model.LabelSet{"ann1": "annv1", "__dashboardUid__": "abcd", "__panelId__": "efgh", "__alertScreenshotToken__": "test-with-path-not-found"}, - }, - }, - }, - expMsg: &slackMessage{ - Channel: "#testchannel", - Username: "Grafana", - IconEmoji: ":emoji:", - Attachments: []attachment{ - { - Title: "[FIRING:1] (val1)", - TitleLink: "http://localhost/alerting/list", - Text: "**Firing**\n\nValue: [no value]\nLabels:\n - alertname = alert1\n - lbl1 = val1\nAnnotations:\n - ann1 = annv1\nSilence: http://localhost/alerting/silence/new?alertmanager=grafana&matcher=alertname%3Dalert1&matcher=lbl1%3Dval1\nDashboard: http://localhost/d/abcd\nPanel: http://localhost/d/abcd?viewPanel=efgh\n", - Fallback: "[FIRING:1] (val1)", - Fields: nil, - Footer: "Grafana v" + setting.BuildVersion, - FooterIcon: "https://grafana.com/assets/img/fav32.png", - Color: "#D63232", - Ts: 0, - }, - }, - }, - expMsgError: nil, - }, - { - name: "Image URL with path and file creates message and uploads image", - settings: `{ - "token": "1234", - "recipient": "#testchannel", - "icon_emoji": ":emoji:" - }`, - alerts: []*types.Alert{ - { - Alert: model.Alert{ - Labels: model.LabelSet{"alertname": "alert1", "lbl1": "val1"}, - Annotations: model.LabelSet{"ann1": "annv1", "__dashboardUid__": "abcd", "__panelId__": "efgh", "__alertScreenshotToken__": "test-with-path-found"}, - }, - }, - }, - expMsg: &slackMessage{ - Channel: "#testchannel", - Username: "Grafana", - IconEmoji: ":emoji:", - Attachments: []attachment{ - { - Title: "[FIRING:1] (val1)", - TitleLink: "http://localhost/alerting/list", - Text: "**Firing**\n\nValue: [no value]\nLabels:\n - alertname = alert1\n - lbl1 = val1\nAnnotations:\n - ann1 = annv1\nSilence: http://localhost/alerting/silence/new?alertmanager=grafana&matcher=alertname%3Dalert1&matcher=lbl1%3Dval1\nDashboard: http://localhost/d/abcd\nPanel: http://localhost/d/abcd?viewPanel=efgh\n", - Fallback: "[FIRING:1] (val1)", - Fields: nil, - Footer: "Grafana v" + setting.BuildVersion, - FooterIcon: "https://grafana.com/assets/img/fav32.png", - Color: "#D63232", - Ts: 0, - }, - }, - }, - expMsgError: nil, - expWebhookURL: SlackImageAPIEndpoint, - }, { name: "Correct config with multiple alerts and template", settings: `{ @@ -334,43 +254,6 @@ func TestSlackNotifier(t *testing.T) { }, expMsgError: nil, }, - { - name: "Custom image upload URL", - settings: `{ - "token": "1234", - "recipient": "#testchannel", - "icon_emoji": ":emoji:", - "imageUploadUrl": "https://custom-domain.upload" - }`, - alerts: []*types.Alert{ - { - Alert: model.Alert{ - Labels: model.LabelSet{"alertname": "alert1", "lbl1": "val1"}, - Annotations: model.LabelSet{"ann1": "annv1", "__dashboardUid__": "abcd", "__panelId__": "efgh", "__alertScreenshotToken__": "test-with-path-found"}, - }, - }, - }, - expMsg: &slackMessage{ - Channel: "#testchannel", - Username: "Grafana", - IconEmoji: ":emoji:", - Attachments: []attachment{ - { - Title: "[FIRING:1] (val1)", - TitleLink: "http://localhost/alerting/list", - Text: "**Firing**\n\nValue: [no value]\nLabels:\n - alertname = alert1\n - lbl1 = val1\nAnnotations:\n - ann1 = annv1\nSilence: http://localhost/alerting/silence/new?alertmanager=grafana&matcher=alertname%3Dalert1&matcher=lbl1%3Dval1\nDashboard: http://localhost/d/abcd\nPanel: http://localhost/d/abcd?viewPanel=efgh\n", - Fallback: "[FIRING:1] (val1)", - Fields: nil, - Footer: "Grafana v" + setting.BuildVersion, - FooterIcon: "https://grafana.com/assets/img/fav32.png", - Color: "#D63232", - Ts: 0, - }, - }, - }, - expMsgError: nil, - expWebhookURL: "https://custom-domain.upload", - }, } for _, c := range cases {