Revert "Alerting: Refactor the ImageStore/Provider to provide image URL/bytes" (#69265)

Revert "Alerting: Refactor the ImageStore/Provider to provide image URL/bytes (#67693)"

This reverts commit 72a187b0be.

go.mod

@@ -58,7 +58,7 @@ require (
 	github.com/google/uuid v1.3.0
 	github.com/google/wire v0.5.0
 	github.com/gorilla/websocket v1.5.0
-	github.com/grafana/alerting v0.0.0-20230505134339-793c67215ba1
+	github.com/grafana/alerting v0.0.0-20230428095912-33c5aa68a5ba
 	github.com/grafana/cuetsy v0.1.9
 	github.com/grafana/grafana-aws-sdk v0.15.0
 	github.com/grafana/grafana-azure-sdk-go v1.7.0

go.sum

@@ -1358,8 +1358,6 @@ github.com/gotestyourself/gotestyourself v1.3.0/go.mod h1:zZKM6oeNM8k+FRljX1mnzV
 github.com/gotestyourself/gotestyourself v2.2.0+incompatible/go.mod h1:zZKM6oeNM8k+FRljX1mnzVYeS8wiGgQyvST1/GafPbY=
 github.com/grafana/alerting v0.0.0-20230428095912-33c5aa68a5ba h1:aNTu22ojw4XY24DYNAuvw8v/5iFUNk2bkdTeeWQ5+0o=
 github.com/grafana/alerting v0.0.0-20230428095912-33c5aa68a5ba/go.mod h1:5edgy6tQY4+W2wuJdi8g2GjbVmpJufguy7QGIRl2q4o=
-github.com/grafana/alerting v0.0.0-20230505134339-793c67215ba1 h1:ed1jueGwj1SWHboT4JpmNf5wWEHaGchfksRjIxFSox0=
-github.com/grafana/alerting v0.0.0-20230505134339-793c67215ba1/go.mod h1:5edgy6tQY4+W2wuJdi8g2GjbVmpJufguy7QGIRl2q4o=
 github.com/grafana/codejen v0.0.3 h1:tAWxoTUuhgmEqxJPOLtJoxlPBbMULFwKFOcRsPRPXDw=
 github.com/grafana/codejen v0.0.3/go.mod h1:zmwwM/DRyQB7pfuBjTWII3CWtxcXh8LTwAYGfDfpR6s=
 github.com/grafana/cuetsy v0.1.9 h1:EwT8BqHoC0B3B4Y6Lg/D1aeYUKnQC1+2jjOHNsOuUfU=

pkg/services/ngalert/notifier/alertmanager.go

@@ -316,7 +316,7 @@ func (am *Alertmanager) buildReceiverIntegrations(receiver *alertingNotify.APIRe
 		return nil, err
 	}
 	s := &sender{am.NotificationService}
-	img := newImageProvider(am.Store, log.New("ngalert.notifier.image-provider"))
+	img := newImageStore(am.Store)
 	integrations, err := alertingNotify.BuildReceiverIntegrations(
 		receiverCfg,
 		tmpl,

pkg/services/ngalert/notifier/email_test.go

@@ -6,7 +6,7 @@ import (
 	"os"
 	"testing"
 
-	alertingImages "github.com/grafana/alerting/images"
+	"github.com/grafana/alerting/images"
 	alertingLogging "github.com/grafana/alerting/logging"
 	"github.com/grafana/alerting/receivers"
 	alertingEmail "github.com/grafana/alerting/receivers/email"
@@ -200,7 +200,7 @@ func createSut(t *testing.T, messageTmpl string, subjectTmpl string, emailTmpl *
 		},
 		Message: messageTmpl,
 		Subject: subjectTmpl,
-	}, receivers.Metadata{}, emailTmpl, ns, &alertingImages.UnavailableProvider{}, &alertingLogging.FakeLogger{})
+	}, receivers.Metadata{}, emailTmpl, ns, &images.UnavailableImageStore{}, &alertingLogging.FakeLogger{})
 }
 
 func getSingleSentMessage(t *testing.T, ns *emailSender) *notifications.Message {

pkg/services/ngalert/notifier/images.go

@@ -2,156 +2,45 @@ package notifier
 
 import (
 	"context"
-	"errors"
-	"io"
-	"os"
-	"path/filepath"
 	"strings"
 
-	alertingImages "github.com/grafana/alerting/images"
+	"github.com/grafana/alerting/images"
-	alertingModels "github.com/grafana/alerting/models"
-	alertingNotify "github.com/grafana/alerting/notify"
 
-	"github.com/grafana/grafana/pkg/infra/log"
 	"github.com/grafana/grafana/pkg/services/ngalert/models"
 	"github.com/grafana/grafana/pkg/services/ngalert/store"
 )
 
-type imageProvider struct {
+type imageStore struct {
-	store  store.ImageStore
+	store store.ImageStore
-	logger log.Logger
 }
 
-func newImageProvider(store store.ImageStore, logger log.Logger) alertingImages.Provider {
+func newImageStore(store store.ImageStore) images.ImageStore {
-	return &imageProvider{
+	return &imageStore{
-		store:  store,
+		store: store,
-		logger: logger,
 	}
 }
 
-func (i imageProvider) GetImage(ctx context.Context, uri string) (*alertingImages.Image, error) {
+func (i imageStore) GetImage(ctx context.Context, uri string) (*images.Image, error) {
-	image, err := i.getImageFromURI(ctx, uri)
+	var (
+		image *models.Image
+		err   error
+	)
+
+	// Check whether the uri is a URL or a token to know how to query the DB.
+	if strings.HasPrefix(uri, "http") {
+		image, err = i.store.GetImageByURL(ctx, uri)
+	} else {
+		token := strings.TrimPrefix(uri, "token://")
+		image, err = i.store.GetImage(ctx, token)
+	}
 	if err != nil {
-		if errors.Is(err, models.ErrImageNotFound) {
-			i.logger.Info("Image not found in database")
-			return nil, alertingImages.ErrImageNotFound
-		}
 		return nil, err
 	}
 
-	return &alertingImages.Image{
+	return &images.Image{
 		Token:     image.Token,
 		Path:      image.Path,
 		URL:       image.URL,
 		CreatedAt: image.CreatedAt,
 	}, nil
 }
-
-func (i imageProvider) GetImageURL(ctx context.Context, alert *alertingNotify.Alert) (string, error) {
-	uri, err := getImageURI(alert)
-	if err != nil {
-		return "", err
-	}
-
-	// If the identifier is a URL, validate that it corresponds to a stored, non-expired image.
-	if strings.HasPrefix(uri, "http") {
-		i.logger.Debug("Received an image URL in annotations", "alert", alert)
-		exists, err := i.store.URLExists(ctx, uri)
-		if err != nil {
-			return "", err
-		}
-		if !exists {
-			i.logger.Info("Image URL not found in database", "alert", alert)
-			return "", alertingImages.ErrImageNotFound
-		}
-		return uri, nil
-	}
-
-	// If the identifier is a token, remove the prefix, get the image and return the URL.
-	token := strings.TrimPrefix(uri, "token://")
-	i.logger.Debug("Received an image token in annotations", "alert", alert, "token", token)
-	return i.getImageURLFromToken(ctx, token)
-}
-
-// getImageURLFromToken takes a token and returns the URL of the image that token belongs to.
-func (i imageProvider) getImageURLFromToken(ctx context.Context, token string) (string, error) {
-	image, err := i.store.GetImage(ctx, token)
-	if err != nil {
-		if errors.Is(err, models.ErrImageNotFound) {
-			i.logger.Info("Image not found in database", "token", token)
-			return "", alertingImages.ErrImageNotFound
-		}
-		return "", err
-	}
-
-	if !image.HasURL() {
-		return "", alertingImages.ErrImagesNoURL
-	}
-	return image.URL, nil
-}
-
-func (i imageProvider) GetRawImage(ctx context.Context, alert *alertingNotify.Alert) (io.ReadCloser, string, error) {
-	uri, err := getImageURI(alert)
-	if err != nil {
-		return nil, "", err
-	}
-
-	image, err := i.getImageFromURI(ctx, uri)
-	if err != nil {
-		if errors.Is(err, models.ErrImageNotFound) {
-			i.logger.Info("Image not found in database", "alert", alert)
-			return nil, "", alertingImages.ErrImageNotFound
-		}
-		return nil, "", err
-	}
-
-	if !image.HasPath() {
-		return nil, "", alertingImages.ErrImagesNoPath
-	}
-
-	// Return image bytes and filename.
-	readCloser, err := openImage(image.Path)
-	if err != nil {
-		i.logger.Error("Error looking for image on disk", "alert", alert, "path", image.Path, "error", err)
-		return nil, "", err
-	}
-	filename := filepath.Base(image.Path)
-	return readCloser, filename, nil
-}
-
-func (i imageProvider) getImageFromURI(ctx context.Context, uri string) (*models.Image, error) {
-	// Check whether the uri is a URL or a token to know how to query the DB.
-	if strings.HasPrefix(uri, "http") {
-		i.logger.Debug("Received an image URL in annotations")
-		return i.store.GetImageByURL(ctx, uri)
-	}
-
-	token := strings.TrimPrefix(uri, "token://")
-	i.logger.Debug("Received an image token in annotations", "token", token)
-	return i.store.GetImage(ctx, token)
-}
-
-// getImageURI is a helper function to retrieve the image URI from the alert annotations as a string.
-func getImageURI(alert *alertingNotify.Alert) (string, error) {
-	uri, ok := alert.Annotations[alertingModels.ImageTokenAnnotation]
-	if !ok {
-		return "", alertingImages.ErrNoImageForAlert
-	}
-	return string(uri), nil
-}
-
-// openImage returns an the io representation of an image from the given path.
-func openImage(path string) (io.ReadCloser, error) {
-	fp := filepath.Clean(path)
-	_, err := os.Stat(fp)
-	if os.IsNotExist(err) || os.IsPermission(err) {
-		return nil, alertingImages.ErrImageNotFound
-	}
-
-	f, err := os.Open(fp)
-	if err != nil {
-		return nil, err
-	}
-
-	return f, nil
-}

pkg/services/ngalert/notifier/images_test.go

@@ -2,25 +2,16 @@ package notifier
 
 import (
 	"context"
-	"io"
-	"os"
-	"path/filepath"
 	"testing"
-	"time"
 
-	alertingImages "github.com/grafana/alerting/images"
-	alertingModels "github.com/grafana/alerting/models"
-	alertingNotify "github.com/grafana/alerting/notify"
-	"github.com/grafana/grafana/pkg/infra/log"
 	"github.com/grafana/grafana/pkg/services/ngalert/models"
 	"github.com/grafana/grafana/pkg/services/ngalert/store"
-	"github.com/prometheus/common/model"
 	"github.com/stretchr/testify/require"
 )
 
 func TestGetImage(t *testing.T) {
 	fakeImageStore := store.NewFakeImageStore(t)
-	store := newImageProvider(fakeImageStore, log.NewNopLogger())
+	store := newImageStore(fakeImageStore)
 
 	t.Run("queries by token when it gets a token", func(tt *testing.T) {
 		img := models.Image{
@@ -31,7 +22,6 @@ func TestGetImage(t *testing.T) {
 		err := fakeImageStore.SaveImage(context.Background(), &img)
 		require.NoError(tt, err)
 
-		// nolint:staticcheck
 		savedImg, err := store.GetImage(context.Background(), "token://"+img.Token)
 		require.NoError(tt, err)
 		require.Equal(tt, savedImg.Token, img.Token)
@@ -48,7 +38,6 @@ func TestGetImage(t *testing.T) {
 		err := fakeImageStore.SaveImage(context.Background(), &img)
 		require.NoError(tt, err)
 
-		// nolint:staticcheck
 		savedImg, err := store.GetImage(context.Background(), img.URL)
 		require.NoError(tt, err)
 		require.Equal(tt, savedImg.Token, img.Token)
@@ -56,170 +45,3 @@ func TestGetImage(t *testing.T) {
 		require.Equal(tt, savedImg.Path, img.Path)
 	})
 }
-
-func TestGetImageURL(t *testing.T) {
-	var (
-		imageWithoutURL = models.Image{
-			Token:     "test-no-url",
-			CreatedAt: time.Now().UTC(),
-			ExpiresAt: time.Now().UTC().Add(24 * time.Hour),
-		}
-		testImage = models.Image{
-			Token:     "test",
-			URL:       "https://test.com",
-			CreatedAt: time.Now().UTC(),
-			ExpiresAt: time.Now().UTC().Add(24 * time.Hour),
-		}
-	)
-
-	fakeImageStore := store.NewFakeImageStore(t, &imageWithoutURL, &testImage)
-	store := newImageProvider(fakeImageStore, log.NewNopLogger())
-
-	tests := []struct {
-		name   string
-		uri    string
-		expURL string
-		expErr error
-	}{
-		{
-			"URL does not exist",
-			"https://invalid.com/test",
-			"",
-			alertingImages.ErrImageNotFound,
-		}, {
-			"existing URL",
-			testImage.URL,
-			testImage.URL,
-			nil,
-		}, {
-			"token does not exist",
-			"token://invalid",
-			"",
-			alertingImages.ErrImageNotFound,
-		}, {
-			"existing token",
-			"token://" + testImage.Token,
-			testImage.URL,
-			nil,
-		}, {
-			"image has no URL",
-			"token://" + imageWithoutURL.Token,
-			"",
-			alertingImages.ErrImagesNoURL,
-		},
-	}
-
-	for _, test := range tests {
-		t.Run(test.name, func(tt *testing.T) {
-			alert := alertingNotify.Alert{
-				Alert: model.Alert{
-					Annotations: model.LabelSet{alertingModels.ImageTokenAnnotation: model.LabelValue(test.uri)},
-				},
-			}
-			url, err := store.GetImageURL(context.Background(), &alert)
-			require.ErrorIs(tt, err, test.expErr)
-			require.Equal(tt, test.expURL, url)
-		})
-	}
-}
-
-func TestGetRawImage(t *testing.T) {
-	var (
-		testBytes        = []byte("some test bytes")
-		testPath         = generateTestFile(t, testBytes)
-		imageWithoutPath = models.Image{
-			Token:     "test-no-path",
-			URL:       "https://test-no-path.com",
-			CreatedAt: time.Now().UTC(),
-			ExpiresAt: time.Now().UTC().Add(24 * time.Hour),
-		}
-		testImage = models.Image{
-			Token:     "test",
-			URL:       "https://test.com",
-			Path:      testPath,
-			CreatedAt: time.Now().UTC(),
-			ExpiresAt: time.Now().UTC().Add(24 * time.Hour),
-		}
-	)
-
-	fakeImageStore := store.NewFakeImageStore(t, &imageWithoutPath, &testImage)
-	store := newImageProvider(fakeImageStore, log.NewNopLogger())
-
-	tests := []struct {
-		name        string
-		uri         string
-		expFilename string
-		expBytes    []byte
-		expErr      error
-	}{
-		{
-			"URL does not exist",
-			"https://invalid.com/test",
-			"",
-			nil,
-			alertingImages.ErrImageNotFound,
-		}, {
-			"existing URL",
-			testImage.URL,
-			filepath.Base(testPath),
-			testBytes,
-			nil,
-		}, {
-			"token does not exist",
-			"token://invalid",
-			"",
-			nil,
-			alertingImages.ErrImageNotFound,
-		}, {
-			"existing token",
-			"token://" + testImage.Token,
-			filepath.Base(testPath),
-			testBytes,
-			nil,
-		}, {
-			"image has no path",
-			"token://" + imageWithoutPath.Token,
-			"",
-			nil,
-			alertingImages.ErrImagesNoPath,
-		},
-	}
-
-	for _, test := range tests {
-		t.Run(test.name, func(tt *testing.T) {
-			alert := alertingNotify.Alert{
-				Alert: model.Alert{
-					Annotations: model.LabelSet{alertingModels.ImageTokenAnnotation: model.LabelValue(test.uri)},
-				},
-			}
-			readCloser, filename, err := store.GetRawImage(context.Background(), &alert)
-			require.ErrorIs(tt, err, test.expErr)
-			require.Equal(tt, test.expFilename, filename)
-
-			if test.expBytes != nil {
-				b, err := io.ReadAll(readCloser)
-				require.NoError(tt, err)
-				require.Equal(tt, test.expBytes, b)
-				require.NoError(t, readCloser.Close())
-			}
-		})
-	}
-}
-
-func generateTestFile(t *testing.T, b []byte) string {
-	t.Helper()
-	f, err := os.CreateTemp("/tmp", "image")
-	require.NoError(t, err)
-	defer func(f *os.File) {
-		_ = f.Close()
-	}(f)
-
-	t.Cleanup(func() {
-		require.NoError(t, os.RemoveAll(f.Name()))
-	})
-
-	_, err = f.Write(b)
-	require.NoError(t, err)
-
-	return f.Name()
-}

pkg/services/ngalert/notifier/testing.go

@@ -10,7 +10,6 @@ import (
 	"testing"
 	"time"
 
-	alertingImages "github.com/grafana/alerting/images"
 	"github.com/grafana/grafana/pkg/infra/kvstore"
 	"github.com/grafana/grafana/pkg/services/ngalert/models"
 	"github.com/grafana/grafana/pkg/services/ngalert/store"
@@ -25,23 +24,19 @@ type fakeConfigStore struct {
 
 // Saves the image or returns an error.
 func (f *fakeConfigStore) SaveImage(ctx context.Context, img *models.Image) error {
-	return alertingImages.ErrImageNotFound
+	return models.ErrImageNotFound
 }
 
 func (f *fakeConfigStore) GetImage(ctx context.Context, token string) (*models.Image, error) {
-	return nil, alertingImages.ErrImageNotFound
+	return nil, models.ErrImageNotFound
 }
 
 func (f *fakeConfigStore) GetImageByURL(ctx context.Context, url string) (*models.Image, error) {
-	return nil, alertingImages.ErrImageNotFound
+	return nil, models.ErrImageNotFound
-}
-
-func (f *fakeConfigStore) URLExists(ctx context.Context, url string) (bool, error) {
-	return false, alertingImages.ErrImageNotFound
 }
 
 func (f *fakeConfigStore) GetImages(ctx context.Context, tokens []string) ([]models.Image, []string, error) {
-	return nil, nil, alertingImages.ErrImageNotFound
+	return nil, nil, models.ErrImageNotFound
 }
 
 func NewFakeConfigStore(t *testing.T, configs map[int64]*models.AlertConfiguration) *fakeConfigStore {

pkg/services/ngalert/store/image.go

@@ -31,10 +31,6 @@ type ImageStore interface {
 
 	// SaveImage saves the image or returns an error.
 	SaveImage(ctx context.Context, img *models.Image) error
-
-	// URLExists takes a URL and returns a boolean indicating whether or not
-	// we have an image for that URL.
-	URLExists(ctx context.Context, url string) (bool, error)
 }
 
 type ImageAdminStore interface {
@@ -79,19 +75,6 @@ func (st DBstore) GetImageByURL(ctx context.Context, url string) (*models.Image,
 	return &image, nil
 }
 
-func (st DBstore) URLExists(ctx context.Context, url string) (bool, error) {
-	var exists bool
-	err := st.SQLStore.WithDbSession(ctx, func(sess *db.Session) error {
-		ok, err := sess.Table("alert_image").Where("url = ? AND expires_at > ?", url, TimeNow().UTC()).Exist()
-		if err != nil {
-			return err
-		}
-		exists = ok
-		return nil
-	})
-	return exists, err
-}
-
 func (st DBstore) GetImages(ctx context.Context, tokens []string) ([]models.Image, []string, error) {
 	var images []models.Image
 	if err := st.SQLStore.WithDbSession(ctx, func(sess *db.Session) error {

pkg/services/ngalert/store/testing.go

@@ -27,15 +27,10 @@ import (
 	"github.com/grafana/grafana/pkg/setting"
 )
 
-func NewFakeImageStore(t *testing.T, images ...*models.Image) *FakeImageStore {
+func NewFakeImageStore(t *testing.T) *FakeImageStore {
-	imageMap := make(map[string]*models.Image)
-	for _, image := range images {
-		imageMap[image.Token] = image
-	}
-
 	return &FakeImageStore{
 		t:      t,
-		images: imageMap,
+		images: make(map[string]*models.Image),
 	}
 }
 
@@ -66,17 +61,6 @@ func (s *FakeImageStore) GetImageByURL(_ context.Context, url string) (*models.I
 	return nil, models.ErrImageNotFound
 }
 
-func (s *FakeImageStore) URLExists(_ context.Context, url string) (bool, error) {
-	s.mtx.Lock()
-	defer s.mtx.Unlock()
-	for _, image := range s.images {
-		if image.URL == url {
-			return true, nil
-		}
-	}
-	return false, nil
-}
-
 func (s *FakeImageStore) GetImages(_ context.Context, tokens []string) ([]models.Image, []string, error) {
 	s.mtx.Lock()
 	defer s.mtx.Unlock()

pkg/services/sqlstore/migrations/ualert/tables.go

@@ -24,11 +24,6 @@ func AddTablesMigrations(mg *migrator.Migrator) {
 	mg.AddMigration("add last_applied column to alert_configuration_history", migrator.NewAddColumnMigration(migrator.Table{Name: "alert_configuration_history"}, &migrator.Column{
 		Name: "last_applied", Type: migrator.DB_Int, Nullable: false, Default: "0",
 	}))
-
-	mg.AddMigration("add index in alert_image on url", migrator.NewAddIndexMigration(migrator.Table{Name: "alert_image"}, &migrator.Index{
-		Cols: []string{"url"}, Type: migrator.IndexType,
-	}))
-	// End of migration log, add new migrations above this line.
 }
 
 // historicalTableMigrations contains those migrations that existed prior to creating the improved messaging around migration immutability.