diff --git a/pkg/services/ngalert/image/mock.go b/pkg/services/ngalert/image/mock.go deleted file mode 100644 index 1529f966ac4..00000000000 --- a/pkg/services/ngalert/image/mock.go +++ /dev/null @@ -1,53 +0,0 @@ -// Code generated by MockGen. DO NOT EDIT. -// Source: github.com/grafana/grafana/pkg/services/ngalert/image (interfaces: ImageService) - -// Package image is a generated GoMock package. -package image - -import ( - context "context" - reflect "reflect" - - gomock "github.com/golang/mock/gomock" - data "github.com/grafana/grafana-plugin-sdk-go/data" - models "github.com/grafana/grafana/pkg/services/ngalert/models" - store "github.com/grafana/grafana/pkg/services/ngalert/store" -) - -// MockImageService is a mock of ImageService interface. -type MockImageService struct { - ctrl *gomock.Controller - recorder *MockImageServiceMockRecorder -} - -// MockImageServiceMockRecorder is the mock recorder for MockImageService. -type MockImageServiceMockRecorder struct { - mock *MockImageService -} - -// NewMockImageService creates a new mock instance. -func NewMockImageService(ctrl *gomock.Controller) *MockImageService { - mock := &MockImageService{ctrl: ctrl} - mock.recorder = &MockImageServiceMockRecorder{mock} - return mock -} - -// EXPECT returns an object that allows the caller to indicate expected use. -func (m *MockImageService) EXPECT() *MockImageServiceMockRecorder { - return m.recorder -} - -// NewImage mocks base method. -func (m *MockImageService) NewImage(arg0 context.Context, arg1 *models.AlertRule, arg2 data.Labels) (*store.Image, error) { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "NewImage", arg0, arg1, arg2) - ret0, _ := ret[0].(*store.Image) - ret1, _ := ret[1].(error) - return ret0, ret1 -} - -// NewImage indicates an expected call of NewImage. -func (mr *MockImageServiceMockRecorder) NewImage(arg0, arg1, arg2 interface{}) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "NewImage", reflect.TypeOf((*MockImageService)(nil).NewImage), arg0, arg1, arg2) -} diff --git a/pkg/services/ngalert/image/service.go b/pkg/services/ngalert/image/service.go index 4af73af9a2f..f19501475fc 100644 --- a/pkg/services/ngalert/image/service.go +++ b/pkg/services/ngalert/image/service.go @@ -10,7 +10,7 @@ import ( "github.com/grafana/grafana/pkg/components/imguploader" "github.com/grafana/grafana/pkg/services/dashboards" - ngmodels "github.com/grafana/grafana/pkg/services/ngalert/models" + "github.com/grafana/grafana/pkg/services/ngalert/models" "github.com/grafana/grafana/pkg/services/ngalert/store" "github.com/grafana/grafana/pkg/services/rendering" "github.com/grafana/grafana/pkg/services/screenshot" @@ -20,7 +20,7 @@ import ( //go:generate mockgen -destination=mock.go -package=image github.com/grafana/grafana/pkg/services/ngalert/image ImageService type ImageService interface { // NewImage returns a new image for the alert instance. - NewImage(ctx context.Context, r *ngmodels.AlertRule) (*store.Image, error) + NewImage(ctx context.Context, r *models.AlertRule) (*models.Image, error) } var ( @@ -83,7 +83,7 @@ func NewScreenshotImageServiceFromCfg(cfg *setting.Cfg, metrics prometheus.Regis // NewImage returns a screenshot of the panel for the alert rule. It returns // ErrNoDashboard if the alert rule does not have a dashboard and ErrNoPanel // when the alert rule does not have a panel in a dashboard. -func (s *ScreenshotImageService) NewImage(ctx context.Context, r *ngmodels.AlertRule) (*store.Image, error) { +func (s *ScreenshotImageService) NewImage(ctx context.Context, r *models.AlertRule) (*models.Image, error) { if r.DashboardUID == nil { return nil, ErrNoDashboard } @@ -102,7 +102,7 @@ func (s *ScreenshotImageService) NewImage(ctx context.Context, r *ngmodels.Alert return nil, fmt.Errorf("failed to take screenshot: %w", err) } - v := store.Image{ + v := models.Image{ Path: screenshot.Path, URL: screenshot.URL, } @@ -115,12 +115,12 @@ func (s *ScreenshotImageService) NewImage(ctx context.Context, r *ngmodels.Alert type NotAvailableImageService struct{} -func (s *NotAvailableImageService) NewImage(ctx context.Context, r *ngmodels.AlertRule) (*store.Image, error) { +func (s *NotAvailableImageService) NewImage(ctx context.Context, r *models.AlertRule) (*models.Image, error) { return nil, screenshot.ErrScreenshotsUnavailable } type NoopImageService struct{} -func (s *NoopImageService) NewImage(ctx context.Context, r *ngmodels.AlertRule) (*store.Image, error) { - return &store.Image{}, nil +func (s *NoopImageService) NewImage(ctx context.Context, r *models.AlertRule) (*models.Image, error) { + return &models.Image{}, nil } diff --git a/pkg/services/ngalert/models/image.go b/pkg/services/ngalert/models/image.go new file mode 100644 index 00000000000..0610f6ca1c5 --- /dev/null +++ b/pkg/services/ngalert/models/image.go @@ -0,0 +1,23 @@ +package models + +import ( + "errors" + "time" +) + +// ErrImageNotFound is returned when the image does not exist. +var ErrImageNotFound = errors.New("image not found") + +type Image struct { + ID int64 `xorm:"pk autoincr 'id'"` + Token string `xorm:"token"` + Path string `xorm:"path"` + URL string `xorm:"url"` + CreatedAt time.Time `xorm:"created_at"` + ExpiresAt time.Time `xorm:"expires_at"` +} + +// A XORM interface that defines the used table for this struct. +func (i *Image) TableName() string { + return "alert_image" +} diff --git a/pkg/services/ngalert/notifier/alertmanager.go b/pkg/services/ngalert/notifier/alertmanager.go index 7d1ff55c89b..7243a9ac606 100644 --- a/pkg/services/ngalert/notifier/alertmanager.go +++ b/pkg/services/ngalert/notifier/alertmanager.go @@ -88,7 +88,7 @@ type ClusterPeer interface { type AlertingStore interface { store.AlertingStore - channels.ImageStore + store.ImageStore } type Alertmanager struct { diff --git a/pkg/services/ngalert/notifier/channels/discord.go b/pkg/services/ngalert/notifier/channels/discord.go index c113dfd0a29..874b9354c40 100644 --- a/pkg/services/ngalert/notifier/channels/discord.go +++ b/pkg/services/ngalert/notifier/channels/discord.go @@ -19,6 +19,7 @@ import ( "github.com/grafana/grafana/pkg/components/simplejson" "github.com/grafana/grafana/pkg/infra/log" "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" ) @@ -196,65 +197,52 @@ func (d DiscordNotifier) SendResolved() bool { func (d DiscordNotifier) constructAttachments(ctx context.Context, as []*types.Alert, embedQuota int) []discordAttachment { attachments := make([]discordAttachment, 0) - for i := range as { - if embedQuota == 0 { - break - } - imgToken := getTokenFromAnnotations(as[i].Annotations) - if len(imgToken) == 0 { - continue - } - timeoutCtx, cancel := context.WithTimeout(ctx, ImageStoreTimeout) - imgURL, err := d.images.GetURL(timeoutCtx, imgToken) - cancel() - if err != nil { - if !errors.Is(err, ErrImagesUnavailable) { - // Ignore errors. Don't log "ImageUnavailable", which means the storage doesn't exist. - d.log.Warn("failed to retrieve image url from store", "error", err) - } - } - - if len(imgURL) > 0 { - attachments = append(attachments, discordAttachment{ - url: imgURL, - state: as[i].Status(), - alertName: as[i].Name(), - }) - } else { - // Need to upload the file. Tell Discord that we're embedding an attachment. - timeoutCtx, cancel := context.WithTimeout(ctx, ImageStoreTimeout) - fp, err := d.images.GetFilepath(timeoutCtx, imgToken) - cancel() - if err != nil { - if !errors.Is(err, ErrImagesUnavailable) { - // Ignore errors. Don't log "ImageUnavailable", which means the storage doesn't exist. - d.log.Warn("failed to retrieve image filepath from store", "error", err) - } + _ = withStoredImages(ctx, d.log, d.images, + func(index int, image *ngmodels.Image) error { + if embedQuota < 1 { + // TODO: Could be a sentinel error to stop execution. + return nil } - base := filepath.Base(fp) - url := fmt.Sprintf("attachment://%s", base) - timeoutCtx, cancel = context.WithTimeout(ctx, ImageStoreTimeout) - reader, err := d.images.GetData(timeoutCtx, imgToken) - cancel() - if err != nil { - if !errors.Is(err, ErrImagesUnavailable) { - // Ignore errors. Don't log "ImageUnavailable", which means the storage doesn't exist. + if image == nil { + return nil + } + + if len(image.URL) > 0 { + attachments = append(attachments, discordAttachment{ + url: image.URL, + state: as[index].Status(), + alertName: as[index].Name(), + }) + embedQuota-- + return nil + } + + // If we have a local file, but no public URL, upload the image as an attachment. + if len(image.Path) > 0 { + base := filepath.Base(image.Path) + url := fmt.Sprintf("attachment://%s", base) + reader, err := openImage(image.Path) + if err != nil && !errors.Is(err, ngmodels.ErrImageNotFound) { d.log.Warn("failed to retrieve image data from store", "error", err) + return nil } - } - attachments = append(attachments, discordAttachment{ - url: url, - name: base, - reader: reader, - state: as[i].Status(), - alertName: as[i].Name(), - }) - } - embedQuota++ - } + attachments = append(attachments, discordAttachment{ + url: url, + name: base, + reader: reader, + state: as[index].Status(), + alertName: as[index].Name(), + }) + embedQuota-- + } + return nil + }, + as..., + ) + return attachments } @@ -282,21 +270,32 @@ func (d DiscordNotifier) buildRequest(ctx context.Context, url string, body []by if err != nil { return nil, err } + if _, err := payload.Write(body); err != nil { return nil, err } + for _, a := range attachments { - part, err := w.CreateFormFile("", a.name) - if err != nil { - return nil, err - } - if _, err := io.Copy(part, a.reader); err != nil { - return nil, err + if a.reader != nil { // We have an image to upload. + err = func() error { + defer func() { _ = a.reader.Close() }() + part, err := w.CreateFormFile("", a.name) + if err != nil { + return err + } + _, err = io.Copy(part, a.reader) + return err + }() + if err != nil { + return nil, err + } } } + if err := w.Close(); err != nil { return nil, fmt.Errorf("failed to close multipart writer: %w", err) } + cmd.ContentType = w.FormDataContentType() cmd.Body = b.String() return cmd, nil diff --git a/pkg/services/ngalert/notifier/channels/email.go b/pkg/services/ngalert/notifier/channels/email.go index 3120c267516..bbbc6de13e0 100644 --- a/pkg/services/ngalert/notifier/channels/email.go +++ b/pkg/services/ngalert/notifier/channels/email.go @@ -12,6 +12,7 @@ import ( "github.com/grafana/grafana/pkg/infra/log" "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/util" ) @@ -126,38 +127,25 @@ func (en *EmailNotifier) Notify(ctx context.Context, as ...*types.Alert) (bool, // TODO: modify the email sender code to support multiple file or image URL // fields. We cannot use images from every alert yet. - imgToken := getTokenFromAnnotations(as[0].Annotations) - if len(imgToken) != 0 { - timeoutCtx, cancel := context.WithTimeout(ctx, ImageStoreTimeout) - imgURL, err := en.images.GetURL(timeoutCtx, imgToken) - cancel() - if err != nil { - if !errors.Is(err, ErrImagesUnavailable) { - // Ignore errors. Don't log "ImageUnavailable", which means the storage doesn't exist. - en.log.Warn("failed to retrieve image url from store", "error", err) + _ = withStoredImage(ctx, en.log, en.images, + func(index int, image *ngmodels.Image) error { + if image == nil { + return nil } - } else if len(imgURL) > 0 { - cmd.Data["ImageLink"] = imgURL - } else { // Try to upload - timeoutCtx, cancel := context.WithTimeout(ctx, ImageStoreTimeout) - imgPath, err := en.images.GetFilepath(timeoutCtx, imgToken) - cancel() - if err != nil { - if !errors.Is(err, ErrImagesUnavailable) { - // Ignore errors. Don't log "ImageUnavailable", which means the storage doesn't exist. - en.log.Warn("failed to retrieve image url from store", "error", err) - } - } else if len(imgPath) != 0 { - file, err := os.Stat(imgPath) + + if len(image.URL) != 0 { + cmd.Data["ImageLink"] = image.URL + } else if len(image.Path) != 0 { + file, err := os.Stat(image.Path) if err == nil { - cmd.EmbeddedFiles = []string{imgPath} + cmd.EmbeddedFiles = []string{image.Path} cmd.Data["EmbeddedImage"] = file.Name() } else { en.log.Warn("failed to access email notification image attachment data", "error", err) } } - } - } + return nil + }, 0, as...) if tmplErr != nil { en.log.Warn("failed to template email message", "err", tmplErr.Error()) diff --git a/pkg/services/ngalert/notifier/channels/factory.go b/pkg/services/ngalert/notifier/channels/factory.go index 35334786d87..0a8d4c6a18b 100644 --- a/pkg/services/ngalert/notifier/channels/factory.go +++ b/pkg/services/ngalert/notifier/channels/factory.go @@ -3,9 +3,9 @@ package channels import ( "context" "errors" - "io" "strings" + "github.com/grafana/grafana/pkg/services/ngalert/models" "github.com/grafana/grafana/pkg/services/notifications" "github.com/prometheus/alertmanager/template" ) @@ -19,11 +19,8 @@ type FactoryConfig struct { Template *template.Template } -// A specialization of store.ImageStore, to avoid an import loop. type ImageStore interface { - GetURL(ctx context.Context, token string) (string, error) - GetFilepath(ctx context.Context, token string) (string, error) - GetData(ctx context.Context, token string) (io.ReadCloser, error) + GetImage(ctx context.Context, token string) (*models.Image, error) } func NewFactoryConfig(config *NotificationChannelConfig, notificationService notifications.Service, diff --git a/pkg/services/ngalert/notifier/channels/googlechat.go b/pkg/services/ngalert/notifier/channels/googlechat.go index 2179221ffcc..5c7ed8ad68d 100644 --- a/pkg/services/ngalert/notifier/channels/googlechat.go +++ b/pkg/services/ngalert/notifier/channels/googlechat.go @@ -12,6 +12,7 @@ import ( "github.com/grafana/grafana/pkg/infra/log" "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" ) @@ -188,40 +189,32 @@ func (gcn *GoogleChatNotifier) buildScreenshotCard(ctx context.Context, alerts [ }, Sections: []section{}, } - for _, alert := range alerts { - imgToken := getTokenFromAnnotations(alert.Annotations) - if len(imgToken) == 0 { - continue - } - timeoutCtx, cancel := context.WithTimeout(ctx, ImageStoreTimeout) - imgURL, err := gcn.images.GetURL(timeoutCtx, imgToken) - cancel() - if err != nil { - if !errors.Is(err, ErrImagesUnavailable) { - // Ignore errors. Don't log "ImageUnavailable", which means the storage doesn't exist. - gcn.log.Warn("failed to retrieve image url from store", "error", err) + _ = withStoredImages(ctx, gcn.log, gcn.images, + func(index int, image *ngmodels.Image) error { + if image == nil || len(image.URL) == 0 { + return nil } - } - if len(imgURL) > 0 { section := section{ Widgets: []widget{ textParagraphWidget{ Text: text{ - Text: fmt.Sprintf("%s: %s", alert.Status(), alert.Name()), + Text: fmt.Sprintf("%s: %s", alerts[index].Status(), alerts[index].Name()), }, }, imageWidget{ Image: imageData{ - ImageURL: imgURL, + ImageURL: image.URL, }, }, }, } card.Sections = append(card.Sections, section) - } - } + + return nil + }, alerts...) + if len(card.Sections) == 0 { return nil } diff --git a/pkg/services/ngalert/notifier/channels/opsgenie.go b/pkg/services/ngalert/notifier/channels/opsgenie.go index 2afcedac7d3..0a06501eb9c 100644 --- a/pkg/services/ngalert/notifier/channels/opsgenie.go +++ b/pkg/services/ngalert/notifier/channels/opsgenie.go @@ -11,6 +11,7 @@ import ( "github.com/grafana/grafana/pkg/components/simplejson" "github.com/grafana/grafana/pkg/infra/log" "github.com/grafana/grafana/pkg/models" + ngmodels "github.com/grafana/grafana/pkg/services/ngalert/models" "github.com/grafana/grafana/pkg/services/notifications" "github.com/prometheus/alertmanager/notify" "github.com/prometheus/alertmanager/template" @@ -212,25 +213,15 @@ func (on *OpsgenieNotifier) buildOpsgenieMessage(ctx context.Context, alerts mod } images := []string{} - for i := range as { - imgToken := getTokenFromAnnotations(as[i].Annotations) - if len(imgToken) == 0 { - continue - } - - dbContext, cancel := context.WithTimeout(ctx, ImageStoreTimeout) - imgURL, err := on.images.GetURL(dbContext, imgToken) - cancel() - - if err != nil { - if !errors.Is(err, ErrImagesUnavailable) { - // Ignore errors. Don't log "ImageUnavailable", which means the storage doesn't exist. - on.log.Warn("Error reading screenshot data from ImageStore: %v", err) + _ = withStoredImages(ctx, on.log, on.images, + func(index int, image *ngmodels.Image) error { + if image == nil || len(image.URL) == 0 { + return nil } - } else if len(imgURL) != 0 { - images = append(images, imgURL) - } - } + images = append(images, image.URL) + return nil + }, + as...) if len(images) != 0 { details.Set("image_urls", images) diff --git a/pkg/services/ngalert/notifier/channels/pagerduty.go b/pkg/services/ngalert/notifier/channels/pagerduty.go index 944ce15bca1..65cc96ebc6c 100644 --- a/pkg/services/ngalert/notifier/channels/pagerduty.go +++ b/pkg/services/ngalert/notifier/channels/pagerduty.go @@ -9,6 +9,7 @@ import ( "github.com/grafana/grafana/pkg/infra/log" "github.com/grafana/grafana/pkg/models" + ngmodels "github.com/grafana/grafana/pkg/services/ngalert/models" "github.com/grafana/grafana/pkg/services/notifications" "github.com/prometheus/alertmanager/notify" "github.com/prometheus/alertmanager/template" @@ -186,23 +187,15 @@ func (pn *PagerdutyNotifier) buildPagerdutyMessage(ctx context.Context, alerts m }, } - for i := range as { - imgToken := getTokenFromAnnotations(as[i].Annotations) - if len(imgToken) == 0 { - continue - } - timeoutCtx, cancel := context.WithTimeout(ctx, ImageStoreTimeout) - imgURL, err := pn.images.GetURL(timeoutCtx, imgToken) - cancel() - if err != nil { - if !errors.Is(err, ErrImagesUnavailable) { - // Ignore errors. Don't log "ImageUnavailable", which means the storage doesn't exist. - pn.log.Warn("failed to retrieve image url from store", "error", err) + _ = withStoredImages(ctx, pn.log, pn.images, + func(index int, image *ngmodels.Image) error { + if image != nil && len(image.URL) != 0 { + msg.Images = append(msg.Images, pagerDutyImage{Src: image.URL}) } - } else { - msg.Images = append(msg.Images, pagerDutyImage{Src: imgURL}) - } - } + + return nil + }, + as...) if len(msg.Payload.Summary) > 1024 { // This is the Pagerduty limit. diff --git a/pkg/services/ngalert/notifier/channels/slack.go b/pkg/services/ngalert/notifier/channels/slack.go index 9a5e9e67e68..e1132d3c819 100644 --- a/pkg/services/ngalert/notifier/channels/slack.go +++ b/pkg/services/ngalert/notifier/channels/slack.go @@ -18,6 +18,7 @@ import ( "github.com/grafana/grafana/pkg/infra/log" "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" "github.com/prometheus/alertmanager/config" @@ -226,34 +227,38 @@ 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: how many image files should we upload? In what order? Should we - // assume the alerts array is already sorted? // TODO: We need a refactoring so we don't do two database reads for the same data. - // TODO: Should we process all alerts' annotations? We can only have on image. - // TODO: Should we guard out-of-bounds errors here? Callers should prevent that from happening, imo - imgToken := getTokenFromAnnotations(alerts[0].Annotations) - dbContext, cancel := context.WithTimeout(ctx, ImageStoreTimeout) - imgData, err := sn.images.GetData(dbContext, imgToken) - cancel() - if err != nil { - if !errors.Is(err, ErrImagesUnavailable) { - // Ignore errors. Don't log "ImageUnavailable", which means the storage doesn't exist. - sn.log.Warn("Error reading screenshot data from ImageStore: %v", err) + 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 - } - - defer func() { - // Nothing for us to do. - _ = 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 @@ -319,26 +324,13 @@ var sendSlackRequest = func(request *http.Request, logger log.Logger) error { return nil } -func (sn *SlackNotifier) buildSlackMessage(ctx context.Context, as []*types.Alert) (*slackMessage, error) { - alerts := types.Alerts(as...) +func (sn *SlackNotifier) buildSlackMessage(ctx context.Context, alrts []*types.Alert) (*slackMessage, error) { + alerts := types.Alerts(alrts...) var tmplErr error - tmpl, _ := TmplText(ctx, sn.tmpl, as, sn.log, &tmplErr) + tmpl, _ := TmplText(ctx, sn.tmpl, alrts, sn.log, &tmplErr) ruleURL := joinUrlPath(sn.tmpl.ExternalURL.String(), "/alerting/list", sn.log) - // TODO: Should we process all alerts' annotations? We can only have on image. - // TODO: Should we guard out-of-bounds errors here? Callers should prevent that from happening, imo - imgToken := getTokenFromAnnotations(as[0].Annotations) - timeoutCtx, cancel := context.WithTimeout(ctx, ImageStoreTimeout) - imgURL, err := sn.images.GetURL(timeoutCtx, imgToken) - cancel() - if err != nil { - if !errors.Is(err, ErrImagesUnavailable) { - // Ignore errors. Don't log "ImageUnavailable", which means the storage doesn't exist. - sn.log.Warn("failed to retrieve image url from store", "error", err) - } - } - req := &slackMessage{ Channel: tmpl(sn.Recipient), Username: tmpl(sn.Username), @@ -353,7 +345,6 @@ func (sn *SlackNotifier) buildSlackMessage(ctx context.Context, as []*types.Aler Fallback: tmpl(sn.Title), Footer: "Grafana v" + setting.BuildVersion, FooterIcon: FooterIconURL, - ImageURL: imgURL, Ts: time.Now().Unix(), TitleLink: ruleURL, Text: tmpl(sn.Text), @@ -361,6 +352,16 @@ func (sn *SlackNotifier) buildSlackMessage(ctx context.Context, as []*types.Aler }, }, } + + _ = withStoredImage(ctx, sn.log, sn.images, + func(index int, image *ngmodels.Image) error { + if image != nil { + req.Attachments[0].ImageURL = image.URL + } + return nil + }, + 0, alrts...) + if tmplErr != nil { sn.log.Warn("failed to template Slack message", "err", tmplErr.Error()) } diff --git a/pkg/services/ngalert/notifier/channels/teams.go b/pkg/services/ngalert/notifier/channels/teams.go index 0b9799f4d91..f08f68c15b4 100644 --- a/pkg/services/ngalert/notifier/channels/teams.go +++ b/pkg/services/ngalert/notifier/channels/teams.go @@ -10,6 +10,7 @@ import ( "github.com/grafana/grafana/pkg/infra/log" "github.com/grafana/grafana/pkg/models" + ngmodels "github.com/grafana/grafana/pkg/services/ngalert/models" "github.com/grafana/grafana/pkg/services/notifications" ) @@ -93,21 +94,14 @@ func (tn *TeamsNotifier) Notify(ctx context.Context, as ...*types.Alert) (bool, ruleURL := joinUrlPath(tn.tmpl.ExternalURL.String(), "/alerting/list", tn.log) images := []teamsImage{} - for i := range as { - imgToken := getTokenFromAnnotations(as[i].Annotations) - timeoutCtx, cancel := context.WithTimeout(ctx, ImageStoreTimeout) - imgURL, err := tn.images.GetURL(timeoutCtx, imgToken) - cancel() - if err != nil { - if !errors.Is(err, ErrImagesUnavailable) { - // Ignore errors. Don't log "ImageUnavailable", which means the storage doesn't exist. - tn.log.Warn("failed to retrieve image url from store", "error", err) + _ = withStoredImages(ctx, tn.log, tn.images, + func(index int, image *ngmodels.Image) error { + if image != nil && len(image.URL) != 0 { + images = append(images, teamsImage{Image: image.URL}) } - } - if len(imgURL) > 0 { - images = append(images, teamsImage{Image: imgURL}) - } - } + return nil + }, + as...) // Note: these template calls must remain in this order title := tmpl(tn.Title) diff --git a/pkg/services/ngalert/notifier/channels/utils.go b/pkg/services/ngalert/notifier/channels/utils.go index 688d13f6a3c..01c9cae7382 100644 --- a/pkg/services/ngalert/notifier/channels/utils.go +++ b/pkg/services/ngalert/notifier/channels/utils.go @@ -10,10 +10,13 @@ import ( "net" "net/http" "net/url" + "os" "path" + "path/filepath" "time" "github.com/prometheus/alertmanager/notify" + "github.com/prometheus/alertmanager/types" "github.com/prometheus/common/model" "github.com/grafana/grafana/pkg/infra/log" @@ -38,6 +41,73 @@ var ( ErrImagesUnavailable = errors.New("alert screenshots are unavailable") ) +// For each alert, attempts to load the models.Image for an image token +// associated with the alert, then calls forEachFunc with the index of the +// alert and the retrieved image struct. If there is no image token, or the +// image does not exist, forEachFunc will be called with a nil value for the +// image. If forEachFunc returns an error, withStoredImages will return +// immediately. If there is a runtime error retrieving images from the image +// store, withStoredImages will attempt to continue executing, after logging +// a warning. +func withStoredImages(ctx context.Context, l log.Logger, imageStore ImageStore, forEachFunc func(index int, image *models.Image) error, alerts ...*types.Alert) error { + for i := range alerts { + err := withStoredImage(ctx, l, imageStore, forEachFunc, i, alerts...) + if err != nil { + return err + } + } + return nil +} + +func withStoredImage(ctx context.Context, l log.Logger, imageStore ImageStore, imageFunc func(index int, image *models.Image) error, index int, alerts ...*types.Alert) error { + imgToken := getTokenFromAnnotations(alerts[index].Annotations) + if len(imgToken) == 0 { + err := imageFunc(index, nil) + if err != nil { + return err + } + } + + timeoutCtx, cancel := context.WithTimeout(ctx, ImageStoreTimeout) + img, err := imageStore.GetImage(timeoutCtx, imgToken) + cancel() + + if errors.Is(err, models.ErrImageNotFound) || errors.Is(err, ErrImagesUnavailable) { + err := imageFunc(index, nil) + if err != nil { + return err + } + } else if err != nil { + // Ignore errors. Don't log "ImageUnavailable", which means the storage doesn't exist. + l.Warn("failed to retrieve image url from store", "error", err) + } + + err = imageFunc(index, img) + if err != nil { + return err + } + + return nil +} + +// The path argument here comes from reading internal image storage, not user +// input, so we ignore the security check here. +//nolint:gosec +func openImage(path string) (io.ReadCloser, error) { + fp := filepath.Clean(path) + _, err := os.Stat(fp) + if os.IsNotExist(err) || os.IsPermission(err) { + return nil, models.ErrImageNotFound + } + + f, err := os.Open(fp) + if err != nil { + return nil, err + } + + return f, nil +} + func getTokenFromAnnotations(annotations model.LabelSet) string { if value, ok := annotations[models.ScreenshotTokenAnnotation]; ok { return string(value) @@ -47,15 +117,8 @@ func getTokenFromAnnotations(annotations model.LabelSet) string { type UnavailableImageStore struct{} -func (n *UnavailableImageStore) GetURL(ctx context.Context, token string) (string, error) { - return "", ErrImagesUnavailable -} - -func (n *UnavailableImageStore) GetFilepath(ctx context.Context, token string) (string, error) { - return "", ErrImagesUnavailable -} - -func (n *UnavailableImageStore) GetData(ctx context.Context, token string) (io.ReadCloser, error) { +// Get returns the image with the corresponding token, or ErrImageNotFound. +func (u *UnavailableImageStore) GetImage(ctx context.Context, token string) (*models.Image, error) { return nil, ErrImagesUnavailable } diff --git a/pkg/services/ngalert/notifier/channels/webhook.go b/pkg/services/ngalert/notifier/channels/webhook.go index 864623383a0..b88723277dd 100644 --- a/pkg/services/ngalert/notifier/channels/webhook.go +++ b/pkg/services/ngalert/notifier/channels/webhook.go @@ -7,6 +7,7 @@ import ( "github.com/grafana/grafana/pkg/infra/log" "github.com/grafana/grafana/pkg/models" + ngmodels "github.com/grafana/grafana/pkg/services/ngalert/models" "github.com/grafana/grafana/pkg/services/notifications" "github.com/prometheus/alertmanager/notify" "github.com/prometheus/alertmanager/template" @@ -113,33 +114,19 @@ func (wn *WebhookNotifier) Notify(ctx context.Context, as ...*types.Alert) (bool return false, err } - // Get screenshot reference tokens out of data before private annotations are cleared. - imgTokens := make([]string, 0, len(as)) - for i := range as { - imgTokens = append(imgTokens, getTokenFromAnnotations(as[i].Annotations)) - } - as, numTruncated := truncateAlerts(wn.MaxAlerts, as) var tmplErr error tmpl, data := TmplText(ctx, wn.tmpl, as, wn.log, &tmplErr) // Augment our Alert data with ImageURLs if available. - for i := range data.Alerts { - imgURL := "" - if len(imgTokens[i]) != 0 { - timeoutCtx, cancel := context.WithTimeout(ctx, ImageStoreTimeout) - imgURL, err = wn.images.GetURL(timeoutCtx, imgTokens[i]) - cancel() - if err != nil { - if !errors.Is(err, ErrImagesUnavailable) { - // Ignore errors. Don't log "ImageUnavailable", which means the storage doesn't exist. - wn.log.Warn("failed to retrieve image url from store", "error", err) - } - } else if len(imgURL) != 0 { - data.Alerts[i].ImageURL = imgURL + _ = withStoredImages(ctx, wn.log, wn.images, + func(index int, image *ngmodels.Image) error { + if image != nil && len(image.URL) != 0 { + data.Alerts[index].ImageURL = image.URL } - } - } + return nil + }, + as...) msg := &webhookMessage{ Version: "1", diff --git a/pkg/services/ngalert/notifier/testing.go b/pkg/services/ngalert/notifier/testing.go index 1cfc41d1aee..a1fc49a03e5 100644 --- a/pkg/services/ngalert/notifier/testing.go +++ b/pkg/services/ngalert/notifier/testing.go @@ -5,7 +5,6 @@ import ( "crypto/md5" "errors" "fmt" - "io" "strings" "sync" "testing" @@ -19,18 +18,13 @@ type FakeConfigStore struct { configs map[int64]*models.AlertConfiguration } -func (f *FakeConfigStore) GetURL(ctx context.Context, token string) (string, error) { - return "", store.ErrImageNotFound +// Saves the image or returns an error. +func (f *FakeConfigStore) SaveImage(ctx context.Context, img *models.Image) error { + return models.ErrImageNotFound } -func (f *FakeConfigStore) GetFilepath(ctx context.Context, token string) (string, error) { - return "", store.ErrImageNotFound -} - -// Returns an io.ReadCloser that reads out the image data for the provided -// token, if available. May return ErrImageNotFound. -func (f *FakeConfigStore) GetData(ctx context.Context, token string) (io.ReadCloser, error) { - return nil, store.ErrImageNotFound +func (f *FakeConfigStore) GetImage(ctx context.Context, token string) (*models.Image, error) { + return nil, models.ErrImageNotFound } func NewFakeConfigStore(t *testing.T, configs map[int64]*models.AlertConfiguration) FakeConfigStore { diff --git a/pkg/services/ngalert/schedule/compat_test.go b/pkg/services/ngalert/schedule/compat_test.go index 7af2ee8a171..b94d6e07e56 100644 --- a/pkg/services/ngalert/schedule/compat_test.go +++ b/pkg/services/ngalert/schedule/compat_test.go @@ -16,7 +16,6 @@ import ( "github.com/grafana/grafana/pkg/services/ngalert/eval" ngModels "github.com/grafana/grafana/pkg/services/ngalert/models" "github.com/grafana/grafana/pkg/services/ngalert/state" - "github.com/grafana/grafana/pkg/services/ngalert/store" "github.com/grafana/grafana/pkg/util" ) @@ -122,7 +121,7 @@ func Test_stateToPostableAlert(t *testing.T) { t.Run("add __alertScreenshotToken__ if there is an image token", func(t *testing.T) { alertState := randomState(tc.state) alertState.Annotations = randomMapOfStrings() - alertState.Image = &store.Image{Token: "test_token"} + alertState.Image = &ngModels.Image{Token: "test_token"} result := stateToPostableAlert(alertState, appURL) diff --git a/pkg/services/ngalert/state/manager_private_test.go b/pkg/services/ngalert/state/manager_private_test.go index 00ad1160ac6..45933c2eef4 100644 --- a/pkg/services/ngalert/state/manager_private_test.go +++ b/pkg/services/ngalert/state/manager_private_test.go @@ -21,9 +21,9 @@ type CountingImageService struct { Called int } -func (c *CountingImageService) NewImage(_ context.Context, _ *ngmodels.AlertRule) (*store.Image, error) { +func (c *CountingImageService) NewImage(_ context.Context, _ *ngmodels.AlertRule) (*ngmodels.Image, error) { c.Called += 1 - return &store.Image{ + return &ngmodels.Image{ Token: fmt.Sprint(rand.Int()), }, nil } @@ -40,7 +40,7 @@ func Test_maybeNewImage(t *testing.T) { true, &State{ State: eval.Alerting, - Image: &store.Image{ + Image: &ngmodels.Image{ Token: "erase me", }, }, @@ -60,7 +60,7 @@ func Test_maybeNewImage(t *testing.T) { &State{ Resolved: true, State: eval.Normal, - Image: &store.Image{ + Image: &ngmodels.Image{ Token: "abcd", }, }, @@ -71,7 +71,7 @@ func Test_maybeNewImage(t *testing.T) { false, &State{ State: eval.Alerting, - Image: &store.Image{ + Image: &ngmodels.Image{ Token: "already set", }, }, diff --git a/pkg/services/ngalert/state/state.go b/pkg/services/ngalert/state/state.go index 44a1bcb1cbb..7aa8c20a66a 100644 --- a/pkg/services/ngalert/state/state.go +++ b/pkg/services/ngalert/state/state.go @@ -12,7 +12,6 @@ import ( "github.com/grafana/grafana/pkg/expr" "github.com/grafana/grafana/pkg/services/ngalert/eval" "github.com/grafana/grafana/pkg/services/ngalert/models" - "github.com/grafana/grafana/pkg/services/ngalert/store" ) type State struct { @@ -33,7 +32,7 @@ type State struct { Resolved bool Annotations map[string]string Labels data.Labels - Image *store.Image + Image *models.Image Error error } diff --git a/pkg/services/ngalert/store/image.go b/pkg/services/ngalert/store/image.go index 49a3781ee8c..4d0f94b0ee0 100644 --- a/pkg/services/ngalert/store/image.go +++ b/pkg/services/ngalert/store/image.go @@ -2,61 +2,32 @@ package store import ( "context" - "errors" "fmt" - "io" - "os" "time" "github.com/gofrs/uuid" + "github.com/grafana/grafana/pkg/services/ngalert/models" "github.com/grafana/grafana/pkg/services/sqlstore" ) -var ( - // ErrImageNotFound is returned when the image does not exist. - ErrImageNotFound = errors.New("image not found") -) - -type Image struct { - ID int64 `xorm:"pk autoincr 'id'"` - Token string `xorm:"token"` - Path string `xorm:"path"` - URL string `xorm:"url"` - CreatedAt time.Time `xorm:"created_at"` - ExpiresAt time.Time `xorm:"expires_at"` -} - -// A XORM interface that lets us clean up our SQL session definition. -func (i *Image) TableName() string { - return "alert_image" -} - type ImageStore interface { // Get returns the image with the token or ErrImageNotFound. - GetImage(ctx context.Context, token string) (*Image, error) + GetImage(ctx context.Context, token string) (*models.Image, error) // Saves the image or returns an error. - SaveImage(ctx context.Context, img *Image) error - - GetURL(ctx context.Context, token string) (string, error) - - GetFilepath(ctx context.Context, token string) (string, error) - - // Returns an io.ReadCloser that reads out the image data for the provided - // token, if available. May return ErrImageNotFound. - GetData(ctx context.Context, token string) (io.ReadCloser, error) + SaveImage(ctx context.Context, img *models.Image) error } -func (st DBstore) GetImage(ctx context.Context, token string) (*Image, error) { - var img Image +func (st DBstore) GetImage(ctx context.Context, token string) (*models.Image, error) { + var img models.Image if err := st.SQLStore.WithDbSession(ctx, func(sess *sqlstore.DBSession) error { exists, err := sess.Where("token = ?", token).Get(&img) if err != nil { return fmt.Errorf("failed to get image: %w", err) } if !exists { - return ErrImageNotFound + return models.ErrImageNotFound } return nil }); err != nil { @@ -65,7 +36,7 @@ func (st DBstore) GetImage(ctx context.Context, token string) (*Image, error) { return &img, nil } -func (st DBstore) SaveImage(ctx context.Context, img *Image) error { +func (st DBstore) SaveImage(ctx context.Context, img *models.Image) error { return st.SQLStore.WithTransactionalDbSession(ctx, func(sess *sqlstore.DBSession) error { // TODO: Is this a good idea? Do we actually want to automatically expire // rows? See issue https://github.com/grafana/grafana/issues/49366 @@ -93,47 +64,10 @@ func (st DBstore) SaveImage(ctx context.Context, img *Image) error { }) } -func (st *DBstore) GetURL(ctx context.Context, token string) (string, error) { - img, err := st.GetImage(ctx, token) - if err != nil { - return "", err - } - return img.URL, nil -} - -func (st *DBstore) GetFilepath(ctx context.Context, token string) (string, error) { - img, err := st.GetImage(ctx, token) - if err != nil { - return "", err - } - return img.Path, nil -} - -func (st *DBstore) GetData(ctx context.Context, token string) (io.ReadCloser, error) { - // TODO: Should we support getting data from image.URL? One could configure - // the system to upload to S3 while still reading data for notifiers like - // Slack that take multipart uploads. - img, err := st.GetImage(ctx, token) - if err != nil { - return nil, err - } - - if len(img.Path) == 0 { - return nil, ErrImageNotFound - } - - f, err := os.Open(img.Path) - if err != nil { - return nil, err - } - - return f, nil -} - //nolint:unused func (st DBstore) DeleteExpiredImages(ctx context.Context) error { return st.SQLStore.WithTransactionalDbSession(ctx, func(sess *sqlstore.DBSession) error { - n, err := sess.Where("expires_at < ?", TimeNow()).Delete(&Image{}) + n, err := sess.Where("expires_at < ?", TimeNow()).Delete(&models.Image{}) if err != nil { return fmt.Errorf("failed to delete expired images: %w", err) } diff --git a/pkg/services/ngalert/store/image_test.go b/pkg/services/ngalert/store/image_test.go index 1669adc2c4e..bd607a9137f 100644 --- a/pkg/services/ngalert/store/image_test.go +++ b/pkg/services/ngalert/store/image_test.go @@ -12,12 +12,13 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/grafana/grafana/pkg/services/ngalert/models" "github.com/grafana/grafana/pkg/services/ngalert/store" "github.com/grafana/grafana/pkg/services/ngalert/tests" ) -func createTestImg(fakeUrl string, fakePath string) *store.Image { - return &store.Image{ +func createTestImg(fakeUrl string, fakePath string) *models.Image { + return &models.Image{ ID: 0, Token: "", Path: fakeUrl + "local", @@ -25,12 +26,12 @@ func createTestImg(fakeUrl string, fakePath string) *store.Image { } } -func addID(img *store.Image, id int64) *store.Image { +func addID(img *models.Image, id int64) *models.Image { img.ID = id return img } -func addToken(img *store.Image) *store.Image { +func addToken(img *models.Image) *models.Image { token, err := uuid.NewV4() if err != nil { panic("wat") @@ -47,7 +48,7 @@ func TestIntegrationSaveAndGetImage(t *testing.T) { // Here are some images to save. imgs := []struct { name string - img *store.Image + img *models.Image errors bool }{ { @@ -99,7 +100,7 @@ func TestIntegrationDeleteExpiredImages(t *testing.T) { _, dbstore := tests.SetupTestEnv(t, baseIntervalSeconds) // Save two images. - imgs := []*store.Image{ + imgs := []*models.Image{ createTestImg("", ""), createTestImg("", ""), }