Skip to content

Commit

Permalink
Alerting: Remove Image Upload code from Slack notifier. (#50062) (#50066
Browse files Browse the repository at this point in the history 
)

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

(cherry picked from commit 9759eed)

Co-authored-by: Joe Blubaugh <joe.blubaugh@grafana.com>
  • Loading branch information
@grafanabot @joeblubaugh
grafanabot and joeblubaugh committed Jun 2, 2022
1 parent cac5a19 commit 2fc2a15
Show file tree
Hide file tree
Showing 2 changed files with 8 additions and 216 deletions.
107 changes: 8 additions & 99 deletions pkg/services/ngalert/notifier/channels/slack.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,6 @@ import (
"errors"
"fmt"
"io"
"math/rand"
"mime/multipart"
"net"
"net/http"
"net/url"
Expand All @@ -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.
Expand All @@ -39,7 +36,6 @@ type SlackNotifier struct {
webhookSender notifications.WebhookSender

URL *url.URL
ImageUploadURL string
Username string
IconEmoji string
IconURL string
Expand All @@ -55,7 +51,6 @@ type SlackNotifier struct {
type SlackConfig struct {
*NotificationChannelConfig
URL *url.URL
ImageUploadURL string
Username string
IconEmoji string
IconURL string
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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(),
Expand All @@ -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,
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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
}
117 changes: 0 additions & 117 deletions pkg/services/ngalert/notifier/channels/slack_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
},
},
}

Expand Down Expand Up @@ -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: `{
Expand Down Expand Up @@ -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 {
Expand Down

0 comments on commit 2fc2a15

Please sign in to comment.