From 7e852720e3fd6394e62749383d1e502fd9fca83d Mon Sep 17 00:00:00 2001
From: George Robinson <george.robinson@grafana.com>
Date: Wed, 9 Nov 2022 17:01:48 +0000
Subject: [PATCH] Alerting: Fix images cached on rule instead of dashboard
 panel signature (#58510)

---
 pkg/services/ngalert/image/service.go      | 27 +++++++++++-----------
 pkg/services/ngalert/image/service_test.go | 10 ++++----
 2 files changed, 19 insertions(+), 18 deletions(-)

diff --git a/pkg/services/ngalert/image/service.go b/pkg/services/ngalert/image/service.go
index 130d917b614..f3382d338c4 100644
--- a/pkg/services/ngalert/image/service.go
+++ b/pkg/services/ngalert/image/service.go
@@ -132,12 +132,22 @@ func (s *ScreenshotImageService) NewImage(ctx context.Context, r *models.AlertRu
 		return nil, ErrNoPanel
 	}
 
+	opts := screenshot.ScreenshotOptions{
+		DashboardUID: *r.DashboardUID,
+		PanelID:      *r.PanelID,
+		Timeout:      screenshotTimeout,
+	}
+
+	// To prevent concurrent screenshots of the same dashboard panel we use singleflight,
+	// deduplicated on a base64 hash of the screenshot options.
+	optsHash := base64.StdEncoding.EncodeToString(opts.Hash())
+
 	logger := s.logger.FromContext(ctx).New(
-		"dashboard", *r.DashboardUID,
-		"panel", *r.PanelID)
+		"dashboard", opts.DashboardUID,
+		"panel", opts.PanelID)
 
 	// If there is an image is in the cache return it instead of taking another screenshot
-	if image, ok := s.cache.Get(ctx, r.GetKey().String()); ok {
+	if image, ok := s.cache.Get(ctx, optsHash); ok {
 		logger.Debug("Found cached image", "token", image.Token)
 		return &image, nil
 	}
@@ -150,15 +160,6 @@ func (s *ScreenshotImageService) NewImage(ctx context.Context, r *models.AlertRu
 	ctx, cancelFunc := context.WithTimeout(ctx, screenshotTimeout)
 	defer cancelFunc()
 
-	opts := screenshot.ScreenshotOptions{
-		DashboardUID: *r.DashboardUID,
-		PanelID:      *r.PanelID,
-		Timeout:      screenshotTimeout,
-	}
-
-	// To prevent concurrent screenshots of the same dashboard panel we use singleflight,
-	// deduplicated on a base64 hash of the screenshot options.
-	optsHash := base64.StdEncoding.EncodeToString(opts.Hash())
 	logger.Debug("Requesting screenshot")
 
 	result, err, _ := s.singleflight.Do(optsHash, func() (interface{}, error) {
@@ -195,7 +196,7 @@ func (s *ScreenshotImageService) NewImage(ctx context.Context, r *models.AlertRu
 	}
 
 	image := result.(models.Image)
-	if err = s.cache.Set(ctx, r.GetKey().String(), image); err != nil {
+	if err = s.cache.Set(ctx, optsHash, image); err != nil {
 		s.logger.Warn("Failed to cache image",
 			"token", image.Token,
 			"error", err)
diff --git a/pkg/services/ngalert/image/service_test.go b/pkg/services/ngalert/image/service_test.go
index 57c0d1aeb78..eb5832e482d 100644
--- a/pkg/services/ngalert/image/service_test.go
+++ b/pkg/services/ngalert/image/service_test.go
@@ -36,7 +36,7 @@ func TestScreenshotImageService(t *testing.T) {
 	ctx := context.Background()
 
 	// assert that the cache is checked for an existing image
-	cache.EXPECT().Get(gomock.Any(), "{orgID: 1, UID: foo}").Return(models.Image{}, false)
+	cache.EXPECT().Get(gomock.Any(), "M2DGZaRLXtg=").Return(models.Image{}, false)
 
 	// assert that a screenshot is taken
 	screenshots.EXPECT().Take(gomock.Any(), screenshot.ScreenshotOptions{
@@ -60,7 +60,7 @@ func TestScreenshotImageService(t *testing.T) {
 	}
 
 	// assert that the image is saved into the cache
-	cache.EXPECT().Set(gomock.Any(), "{orgID: 1, UID: foo}", expected).Return(nil)
+	cache.EXPECT().Set(gomock.Any(), "M2DGZaRLXtg=", expected).Return(nil)
 
 	image, err := s.NewImage(ctx, &models.AlertRule{
 		OrgID:        1,
@@ -71,7 +71,7 @@ func TestScreenshotImageService(t *testing.T) {
 	assert.Equal(t, expected, *image)
 
 	// assert that the cache is checked for an existing image
-	cache.EXPECT().Get(gomock.Any(), "{orgID: 1, UID: bar}").Return(models.Image{}, false)
+	cache.EXPECT().Get(gomock.Any(), "rTOWVcbRidk=").Return(models.Image{}, false)
 
 	// assert that a screenshot is taken
 	screenshots.EXPECT().Take(gomock.Any(), screenshot.ScreenshotOptions{
@@ -94,7 +94,7 @@ func TestScreenshotImageService(t *testing.T) {
 	}
 
 	// assert that the image is saved into the cache, but without a URL
-	cache.EXPECT().Set(gomock.Any(), "{orgID: 1, UID: bar}", expected).Return(nil)
+	cache.EXPECT().Set(gomock.Any(), "rTOWVcbRidk=", expected).Return(nil)
 
 	image, err = s.NewImage(ctx, &models.AlertRule{
 		OrgID:        1,
@@ -107,7 +107,7 @@ func TestScreenshotImageService(t *testing.T) {
 	expected = models.Image{Path: "baz.png", URL: "https://example.com/baz.png"}
 
 	// assert that the cache is checked for an existing image and it is returned
-	cache.EXPECT().Get(gomock.Any(), "{orgID: 1, UID: baz}").Return(expected, true)
+	cache.EXPECT().Get(gomock.Any(), "8hJuVe20rVE=").Return(expected, true)
 
 	image, err = s.NewImage(ctx, &models.AlertRule{
 		OrgID:        1,