From 1f984409a2b878ddbca98ecdbd54efc1334436d4 Mon Sep 17 00:00:00 2001 From: George Robinson Date: Thu, 9 Feb 2023 20:23:01 +0000 Subject: [PATCH] Alerting: Fix a bug taking screenshots with Dashboard UID (#63220) This commit fixes a bug where Grafana would fail to take a screenshot if the same Dashboard UID was present across two or more different orgs. --- pkg/services/ngalert/image/service.go | 1 + pkg/services/ngalert/image/service_test.go | 15 +++++++++------ pkg/services/screenshot/option.go | 2 ++ pkg/services/screenshot/option_test.go | 22 +++++++++++----------- pkg/services/screenshot/screenshot.go | 10 +++++----- 5 files changed, 28 insertions(+), 22 deletions(-) diff --git a/pkg/services/ngalert/image/service.go b/pkg/services/ngalert/image/service.go index e95ef6ac8d9..89684c8bceb 100644 --- a/pkg/services/ngalert/image/service.go +++ b/pkg/services/ngalert/image/service.go @@ -135,6 +135,7 @@ func (s *ScreenshotImageService) NewImage(ctx context.Context, r *models.AlertRu logger = logger.New("dashboard", dashboardUID, "panel", panelID) opts := screenshot.ScreenshotOptions{ + OrgID: r.OrgID, DashboardUID: dashboardUID, PanelID: panelID, Timeout: s.screenshotTimeout, diff --git a/pkg/services/ngalert/image/service_test.go b/pkg/services/ngalert/image/service_test.go index 11cef0ea5a5..03a60f65215 100644 --- a/pkg/services/ngalert/image/service_test.go +++ b/pkg/services/ngalert/image/service_test.go @@ -38,10 +38,11 @@ func TestScreenshotImageService(t *testing.T) { t.Run("image is taken, uploaded, saved to database and cached", func(t *testing.T) { // assert that the cache is checked for an existing image - cache.EXPECT().Get(gomock.Any(), "M2DGZaRLXtg=").Return(models.Image{}, false) + cache.EXPECT().Get(gomock.Any(), "oyh1kYgaJwM=").Return(models.Image{}, false) // assert that a screenshot is taken screenshots.EXPECT().Take(gomock.Any(), screenshot.ScreenshotOptions{ + OrgID: 1, DashboardUID: "foo", PanelID: 1, Timeout: 5 * time.Second, @@ -62,7 +63,7 @@ func TestScreenshotImageService(t *testing.T) { } // assert that the image is saved into the cache - cache.EXPECT().Set(gomock.Any(), "M2DGZaRLXtg=", expected).Return(nil) + cache.EXPECT().Set(gomock.Any(), "oyh1kYgaJwM=", expected).Return(nil) image, err := s.NewImage(ctx, &models.AlertRule{ OrgID: 1, @@ -75,10 +76,11 @@ func TestScreenshotImageService(t *testing.T) { t.Run("image is taken, upload return error, saved to database without URL and cached", func(t *testing.T) { // assert that the cache is checked for an existing image - cache.EXPECT().Get(gomock.Any(), "rTOWVcbRidk=").Return(models.Image{}, false) + cache.EXPECT().Get(gomock.Any(), "yszV9tgmKAo=").Return(models.Image{}, false) // assert that a screenshot is taken screenshots.EXPECT().Take(gomock.Any(), screenshot.ScreenshotOptions{ + OrgID: 1, DashboardUID: "bar", PanelID: 1, Timeout: 5 * time.Second, @@ -98,7 +100,7 @@ func TestScreenshotImageService(t *testing.T) { } // assert that the image is saved into the cache, but without a URL - cache.EXPECT().Set(gomock.Any(), "rTOWVcbRidk=", expected).Return(nil) + cache.EXPECT().Set(gomock.Any(), "yszV9tgmKAo=", expected).Return(nil) image, err := s.NewImage(ctx, &models.AlertRule{ OrgID: 1, @@ -113,7 +115,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(), "8hJuVe20rVE=").Return(expected, true) + cache.EXPECT().Get(gomock.Any(), "he399rFDBPI=").Return(expected, true) image, err := s.NewImage(ctx, &models.AlertRule{ OrgID: 1, @@ -126,10 +128,11 @@ func TestScreenshotImageService(t *testing.T) { t.Run("error is returned when timeout is exceeded", func(t *testing.T) { // assert that the cache is checked for an existing image - cache.EXPECT().Get(gomock.Any(), "jtThkFaZLA4=").Return(models.Image{}, false) + cache.EXPECT().Get(gomock.Any(), "TTHub8HUe2U=").Return(models.Image{}, false) // assert that when the timeout is exceeded an error is returned screenshots.EXPECT().Take(gomock.Any(), screenshot.ScreenshotOptions{ + OrgID: 1, DashboardUID: "qux", PanelID: 1, Timeout: 5 * time.Second, diff --git a/pkg/services/screenshot/option.go b/pkg/services/screenshot/option.go index f19381abdae..13cc89f6c47 100644 --- a/pkg/services/screenshot/option.go +++ b/pkg/services/screenshot/option.go @@ -19,6 +19,7 @@ var ( // ScreenshotOptions are the options for taking a screenshot. type ScreenshotOptions struct { + OrgID int64 DashboardUID string PanelID int64 From string @@ -56,6 +57,7 @@ func (s ScreenshotOptions) SetDefaults() ScreenshotOptions { func (s ScreenshotOptions) Hash() []byte { h := fnv.New64() + _, _ = h.Write([]byte(strconv.FormatInt(s.OrgID, 10))) _, _ = h.Write([]byte(s.DashboardUID)) _, _ = h.Write([]byte(strconv.FormatInt(s.PanelID, 10))) _, _ = h.Write([]byte(s.From)) diff --git a/pkg/services/screenshot/option_test.go b/pkg/services/screenshot/option_test.go index 2187b032391..9d7316e6749 100644 --- a/pkg/services/screenshot/option_test.go +++ b/pkg/services/screenshot/option_test.go @@ -88,30 +88,30 @@ func TestScreenshotOptions(t *testing.T) { func TestScreenshotOptions_Hash(t *testing.T) { o := ScreenshotOptions{} - assert.Equal(t, []byte{0xd9, 0x83, 0x82, 0x18, 0x6c, 0x3d, 0x7d, 0x47}, o.Hash()) + assert.Equal(t, []byte{0xd7, 0xf3, 0x56, 0x7f, 0xec, 0x7b, 0xdf, 0x95}, o.Hash()) o = o.SetDefaults() - assert.Equal(t, []byte{0x3, 0x52, 0x33, 0x5f, 0xed, 0x96, 0x47, 0xb5}, o.Hash()) + assert.Equal(t, []byte{0x13, 0x59, 0xa3, 0x88, 0x6a, 0xdd, 0x26, 0x3}, o.Hash()) + + o.OrgID = 1 + assert.Equal(t, []byte{0xd8, 0xfb, 0xf2, 0x7, 0x87, 0x92, 0x57, 0x28}, o.Hash()) o.From = "now-6h" - assert.Equal(t, []byte{0x1b, 0x61, 0xc4, 0x40, 0x1f, 0xae, 0xa0, 0xe0}, o.Hash()) + assert.Equal(t, []byte{0x52, 0x3f, 0x86, 0xe3, 0x59, 0x1a, 0x7f, 0xfd}, o.Hash()) o.To = "now-2h" - assert.Equal(t, []byte{0x2, 0x4f, 0xfd, 0xd5, 0x68, 0xdd, 0xd1, 0x99}, o.Hash()) + assert.Equal(t, []byte{0x8b, 0x78, 0xf4, 0xe7, 0xaa, 0x6, 0xba, 0xde}, o.Hash()) o.Width = 100 - o = o.SetDefaults() - assert.Equal(t, []byte{0x16, 0x6c, 0xd, 0xad, 0x9f, 0x32, 0x9d, 0x1}, o.Hash()) + assert.Equal(t, []byte{0xa8, 0x75, 0x43, 0xc1, 0xb5, 0xd5, 0xd2, 0x3c}, o.Hash()) o.Height = 100 - o = o.SetDefaults() - assert.Equal(t, []byte{0x2, 0x82, 0x96, 0x68, 0x2b, 0x57, 0x7d, 0xdd}, o.Hash()) + assert.Equal(t, []byte{0x3d, 0xf3, 0x37, 0x3d, 0x9a, 0xfd, 0x71, 0x88}, o.Hash()) o.Theme = "Not a theme" - o = o.SetDefaults() - assert.Equal(t, []byte{0x2, 0x82, 0x96, 0x68, 0x2b, 0x57, 0x7d, 0xdd}, o.Hash()) + assert.Equal(t, []byte{0x3b, 0xd1, 0xfb, 0x3f, 0x3, 0x64, 0xba, 0xad}, o.Hash()) // the timeout should not change the sum o.Timeout = DefaultTimeout + 1 - assert.Equal(t, []byte{0x2, 0x82, 0x96, 0x68, 0x2b, 0x57, 0x7d, 0xdd}, o.Hash()) + assert.Equal(t, []byte{0x3b, 0xd1, 0xfb, 0x3f, 0x3, 0x64, 0xba, 0xad}, o.Hash()) } diff --git a/pkg/services/screenshot/screenshot.go b/pkg/services/screenshot/screenshot.go index 0e88b9c62f3..743089874a8 100644 --- a/pkg/services/screenshot/screenshot.go +++ b/pkg/services/screenshot/screenshot.go @@ -85,8 +85,8 @@ func (s *HeadlessScreenshotService) Take(ctx context.Context, opts ScreenshotOpt start := time.Now() defer func() { s.duration.Observe(time.Since(start).Seconds()) }() - q := dashboards.GetDashboardQuery{UID: opts.DashboardUID} - qResult, err := s.ds.GetDashboard(ctx, &q) + q := dashboards.GetDashboardQuery{OrgID: opts.OrgID, UID: opts.DashboardUID} + dashboard, err := s.ds.GetDashboard(ctx, &q) if err != nil { s.instrumentError(err) return nil, err @@ -95,9 +95,9 @@ func (s *HeadlessScreenshotService) Take(ctx context.Context, opts ScreenshotOpt opts = opts.SetDefaults() u := url.URL{} - u.Path = path.Join("d-solo", qResult.UID, qResult.Slug) + u.Path = path.Join("d-solo", dashboard.UID, dashboard.Slug) p := u.Query() - p.Add("orgId", strconv.FormatInt(qResult.OrgID, 10)) + p.Add("orgId", strconv.FormatInt(dashboard.OrgID, 10)) p.Add("panelId", strconv.FormatInt(opts.PanelID, 10)) p.Add("from", opts.From) p.Add("to", opts.To) @@ -105,7 +105,7 @@ func (s *HeadlessScreenshotService) Take(ctx context.Context, opts ScreenshotOpt renderOpts := rendering.Opts{ AuthOpts: rendering.AuthOpts{ - OrgID: qResult.OrgID, + OrgID: dashboard.OrgID, OrgRole: org.RoleAdmin, }, ErrorOpts: rendering.ErrorOpts{