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
2025-02-25 18:55:37 -06:00 · 2022-06-02 17:18:35 +08:00 · 2022-06-02 17:18:35 +08:00 · 9759eeda17
commit 9759eeda17
parent b0925ed4ee
2 changed files with 8 additions and 216 deletions
--- 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
-}
--- 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 {