Alerting: Add stored screenshot utilities to the channels package. (#49470)

Adds three functions:
`withStoredImages` iterates over a list of models.Alerts, extracting a stored image's data from storage, if available, and executing a user-provided function.
`withStoredImage` does this for an image attached to a specific alert.
`openImage` finds and opens an image file on disk.

Moves `store.Image` to `models.Image`
Simplifies `channels.ImageStore` interface and updates notifiers that use it to use the simpler methods.
Updates all pkg/alert/notifier/channels to use withStoredImage routines.

pkg/services/ngalert/image/mock.go

@@ -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)
-}

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
 }

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"
+}

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 {

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)
+	_ = 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
 			}
-		}
 
-		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)
-				}
+			if image == nil {
+				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 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[index].Status(),
+					alertName: as[index].Name(),
+				})
+				embedQuota--
 			}
+			return nil
+		},
+		as...,
+	)
 
-			attachments = append(attachments, discordAttachment{
-				url:       url,
-				name:      base,
-				reader:    reader,
-				state:     as[i].Status(),
-				alertName: as[i].Name(),
-			})
-		}
-		embedQuota++
-	}
 	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

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())

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,

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
 	}

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)

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.

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)
-		}
-		return true, nil
-	}
+	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...)
 
-	defer func() {
-		// Nothing for us to do.
-		_ = imgData.Close()
-	}()
+		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)
+			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())
 	}

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)

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
 }
 

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",

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 {

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)
 

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",
 				},
 			},

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
 }
 

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)
 		}

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("", ""),
 	}