From d7c554c25e614fd353436aa5db20a0fcb88bcb52 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Agn=C3=A8s=20Toulet?= <35176601+AgnesToulet@users.noreply.github.com> Date: Fri, 11 Apr 2025 15:34:10 +0200 Subject: [PATCH] Renderer: Fix regression on callback URL in plugin mode (#103787) * Renderer: Fix regression on callback URL in plugin mode * cleanup * refactor to not mutate cfg object * apply review feedback --- pkg/services/rendering/capabilities_test.go | 2 +- pkg/services/rendering/http_mode.go | 4 +- pkg/services/rendering/rendering.go | 62 +++++++----- pkg/services/rendering/rendering_test.go | 100 +++++++++++++++++--- pkg/setting/setting.go | 26 +---- pkg/setting/setting_test.go | 13 +-- 6 files changed, 133 insertions(+), 74 deletions(-) diff --git a/pkg/services/rendering/capabilities_test.go b/pkg/services/rendering/capabilities_test.go index e9b4989dbbf..956fae30e0f 100644 --- a/pkg/services/rendering/capabilities_test.go +++ b/pkg/services/rendering/capabilities_test.go @@ -121,7 +121,7 @@ func TestCapabilities(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - rs.Cfg.RendererUrl = tt.rendererUrl + rs.Cfg.RendererServerUrl = tt.rendererUrl rs.version = tt.rendererVersion res, err := rs.HasCapability(context.Background(), tt.capabilityName) diff --git a/pkg/services/rendering/http_mode.go b/pkg/services/rendering/http_mode.go index 0f877867465..705f9672240 100644 --- a/pkg/services/rendering/http_mode.go +++ b/pkg/services/rendering/http_mode.go @@ -71,7 +71,7 @@ func (rs *RenderingService) renderCSVViaHTTP(ctx context.Context, renderKey stri } func (rs *RenderingService) generateImageRendererURL(renderType RenderType, opts Opts, renderKey string) (*url.URL, error) { - rendererUrl := rs.Cfg.RendererUrl + rendererUrl := rs.Cfg.RendererServerUrl if renderType == RenderCSV { rendererUrl += "/csv" } @@ -242,7 +242,7 @@ func (rs *RenderingService) getRemotePluginVersionWithRetry(callback func(string } func (rs *RenderingService) getRemotePluginVersion() (string, error) { - rendererURL, err := url.Parse(rs.Cfg.RendererUrl + "/version") + rendererURL, err := url.Parse(rs.Cfg.RendererServerUrl + "/version") if err != nil { return "", err } diff --git a/pkg/services/rendering/rendering.go b/pkg/services/rendering/rendering.go index 8f25cb8da22..2585f9b783e 100644 --- a/pkg/services/rendering/rendering.go +++ b/pkg/services/rendering/rendering.go @@ -27,18 +27,19 @@ import ( var _ Service = (*RenderingService)(nil) type RenderingService struct { - log log.Logger - plugin Plugin - renderAction renderFunc - renderCSVAction renderCSVFunc - sanitizeSVGAction sanitizeFunc - sanitizeURL string - domain string - inProgressCount int32 - version string - versionMutex sync.RWMutex - capabilities []Capability - pluginAvailable bool + log log.Logger + plugin Plugin + renderAction renderFunc + renderCSVAction renderCSVFunc + sanitizeSVGAction sanitizeFunc + sanitizeURL string + domain string + inProgressCount int32 + version string + versionMutex sync.RWMutex + capabilities []Capability + pluginAvailable bool + rendererCallbackURL string perRequestRenderKeyProvider renderKeyProvider Cfg *setting.Cfg @@ -80,9 +81,24 @@ func ProvideService(cfg *setting.Cfg, features featuremgmt.FeatureToggles, remot // value used for domain attribute of renderKey cookie var domain string + // value used by the image renderer to make requests to Grafana + rendererCallbackURL := cfg.RendererCallbackUrl + if cfg.RendererServerUrl != "" { + sanitizeURL = getSanitizerURL(cfg.RendererServerUrl) + + // Default value for callback URL using a remote renderer should be AppURL + if rendererCallbackURL == "" { + rendererCallbackURL = cfg.AppURL + } + } + switch { - case cfg.RendererCallbackUrl != "": - u, err := url.Parse(cfg.RendererCallbackUrl) + case rendererCallbackURL != "": + if rendererCallbackURL[len(rendererCallbackURL)-1] != '/' { + rendererCallbackURL += "/" + } + + u, err := url.Parse(rendererCallbackURL) if err != nil { logger.Warn("Image renderer callback url is not valid. " + "Please provide a valid RendererCallbackUrl. " + @@ -96,10 +112,6 @@ func ProvideService(cfg *setting.Cfg, features featuremgmt.FeatureToggles, remot domain = "localhost" } - if cfg.RendererUrl != "" { - sanitizeURL = getSanitizerURL(cfg.RendererUrl) - } - var renderKeyProvider renderKeyProvider if features.IsEnabledGlobally(featuremgmt.FlagRenderAuthJWT) { renderKeyProvider = &jwtRenderKeyProvider{ @@ -145,6 +157,7 @@ func ProvideService(cfg *setting.Cfg, features featuremgmt.FeatureToggles, remot domain: domain, sanitizeURL: sanitizeURL, pluginAvailable: exists, + rendererCallbackURL: rendererCallbackURL, } gob.Register(&RenderUser{}) @@ -215,7 +228,7 @@ func (rs *RenderingService) Run(ctx context.Context) error { } func (rs *RenderingService) remoteAvailable() bool { - return rs.Cfg.RendererUrl != "" + return rs.Cfg.RendererServerUrl != "" } func (rs *RenderingService) IsAvailable(ctx context.Context) bool { @@ -424,13 +437,14 @@ func (rs *RenderingService) getNewFilePath(rt RenderType) (string, error) { // getGrafanaCallbackURL creates a URL to send to the image rendering as callback for rendering a Grafana resource func (rs *RenderingService) getGrafanaCallbackURL(path string) string { - if rs.Cfg.RendererUrl != "" || rs.Cfg.RendererCallbackUrl != "" { - // The backend rendering service can potentially be remote. - // So we need to use the root_url to ensure the rendering service - // can reach this Grafana instance. + if rs.rendererCallbackURL != "" { + // rendererCallbackURL should be set if: + // - the backend rendering service is remote (default value is cfg.AppURL + // and set when initializing the service) + // - the service is a plugin and Grafana is running behind a proxy changing its domain // &render=1 signals to the legacy redirect layer to - return fmt.Sprintf("%s%s&render=1", rs.Cfg.RendererCallbackUrl, path) + return fmt.Sprintf("%s%s&render=1", rs.rendererCallbackURL, path) } protocol := rs.Cfg.Protocol diff --git a/pkg/services/rendering/rendering_test.go b/pkg/services/rendering/rendering_test.go index 2bcd6463633..15f473b555c 100644 --- a/pkg/services/rendering/rendering_test.go +++ b/pkg/services/rendering/rendering_test.go @@ -14,6 +14,7 @@ import ( "github.com/grafana/grafana/pkg/infra/log" "github.com/grafana/grafana/pkg/models" + "github.com/grafana/grafana/pkg/services/featuremgmt" "github.com/grafana/grafana/pkg/setting" ) @@ -25,21 +26,21 @@ func TestGetUrl(t *testing.T) { } t.Run("When renderer and callback url configured should return callback url plus path", func(t *testing.T) { - rs.Cfg.RendererUrl = "http://localhost:8081/render" - rs.Cfg.RendererCallbackUrl = "http://public-grafana.com/" + rs.Cfg.RendererServerUrl = "http://localhost:8081/render" + rs.rendererCallbackURL = "http://public-grafana.com/" url := rs.getGrafanaCallbackURL(path) - require.Equal(t, rs.Cfg.RendererCallbackUrl+path+"&render=1", url) + require.Equal(t, rs.rendererCallbackURL+path+"&render=1", url) }) t.Run("When callback url is configured and https should return domain of callback url plus path", func(t *testing.T) { - rs.Cfg.RendererCallbackUrl = "https://public-grafana.com/" + rs.rendererCallbackURL = "https://public-grafana.com/" url := rs.getGrafanaCallbackURL(path) - require.Equal(t, rs.Cfg.RendererCallbackUrl+path+"&render=1", url) + require.Equal(t, rs.rendererCallbackURL+path+"&render=1", url) }) t.Run("When renderer url not configured", func(t *testing.T) { - rs.Cfg.RendererUrl = "" - rs.Cfg.RendererCallbackUrl = "" + rs.Cfg.RendererServerUrl = "" + rs.rendererCallbackURL = "" rs.domain = "localhost" rs.Cfg.HTTPPort = "3000" @@ -128,8 +129,8 @@ func TestRenderLimitImage(t *testing.T) { rs := RenderingService{ Cfg: &setting.Cfg{ - HomePath: path, - RendererUrl: "http://localhost:8081/render", + HomePath: path, + RendererServerUrl: "http://localhost:8081/render", }, inProgressCount: 2, log: log.New("test"), @@ -170,7 +171,7 @@ func TestRenderLimitImage(t *testing.T) { func TestRenderLimitImageError(t *testing.T) { rs := RenderingService{ Cfg: &setting.Cfg{ - RendererUrl: "http://localhost:8081/render", + RendererServerUrl: "http://localhost:8081/render", }, inProgressCount: 2, log: log.New("test"), @@ -201,7 +202,7 @@ func TestRenderingServiceGetRemotePluginVersion(t *testing.T) { })) defer server.Close() - rs.Cfg.RendererUrl = server.URL + "/render" + rs.Cfg.RendererServerUrl = server.URL + "/render" version, err := rs.getRemotePluginVersion() require.NoError(t, err) @@ -214,7 +215,7 @@ func TestRenderingServiceGetRemotePluginVersion(t *testing.T) { })) defer server.Close() - rs.Cfg.RendererUrl = server.URL + "/render" + rs.Cfg.RendererServerUrl = server.URL + "/render" version, err := rs.getRemotePluginVersion() require.NoError(t, err) @@ -239,7 +240,7 @@ func TestRenderingServiceGetRemotePluginVersion(t *testing.T) { })) defer server.Close() - rs.Cfg.RendererUrl = server.URL + "/render" + rs.Cfg.RendererServerUrl = server.URL + "/render" remoteVersionFetchInterval = time.Millisecond remoteVersionFetchRetries = 5 go func() { @@ -249,3 +250,76 @@ func TestRenderingServiceGetRemotePluginVersion(t *testing.T) { require.Eventually(t, func() bool { return rs.Version() == "3.1.4159" }, time.Second, time.Millisecond) }) } + +func TestProvideService(t *testing.T) { + cfg := setting.NewCfg() + cfg.AppURL = "http://app-url" + cfg.ImagesDir = filepath.Join(t.TempDir(), "images") + cfg.CSVsDir = filepath.Join(t.TempDir(), "csvs") + cfg.PDFsDir = filepath.Join(t.TempDir(), "pdfs") + + t.Run("Default configuration values", func(t *testing.T) { + rs, err := ProvideService(cfg, featuremgmt.WithFeatures(), nil, &dummyPluginManager{}) + require.NoError(t, err) + + require.Equal(t, "", rs.Cfg.RendererServerUrl) + require.Equal(t, "", rs.rendererCallbackURL) + require.Equal(t, "", rs.domain) + }) + + t.Run("RendererURL is set but not RendererCallbackUrl", func(t *testing.T) { + cfg.RendererServerUrl = "http://custom-renderer:8081" + cfg.RendererCallbackUrl = "" + + rs, err := ProvideService(cfg, featuremgmt.WithFeatures(), nil, &dummyPluginManager{}) + require.NoError(t, err) + + require.Equal(t, "http://custom-renderer:8081", rs.Cfg.RendererServerUrl) + require.Equal(t, "http://app-url/", rs.rendererCallbackURL) + require.Equal(t, "app-url", rs.domain) + }) + + t.Run("RendererURL and RendererCallbackUrl are set", func(t *testing.T) { + cfg.RendererServerUrl = "http://custom-renderer:8081" + cfg.RendererCallbackUrl = "http://public-grafana.com/" + + rs, err := ProvideService(cfg, featuremgmt.WithFeatures(), nil, &dummyPluginManager{}) + require.NoError(t, err) + + require.Equal(t, "http://custom-renderer:8081", rs.Cfg.RendererServerUrl) + require.Equal(t, "http://public-grafana.com/", rs.rendererCallbackURL) + require.Equal(t, "public-grafana.com", rs.domain) + }) + + t.Run("RendererURL is not set but RendererCallbackUrl is set", func(t *testing.T) { + cfg.RendererServerUrl = "" + cfg.RendererCallbackUrl = "https://public-grafana.com/" + + rs, err := ProvideService(cfg, featuremgmt.WithFeatures(), nil, &dummyPluginManager{}) + require.NoError(t, err) + + require.Equal(t, "", rs.Cfg.RendererServerUrl) + require.Equal(t, "https://public-grafana.com/", rs.rendererCallbackURL) + require.Equal(t, "public-grafana.com", rs.domain) + }) + + t.Run("RendererCallbackURL is missing trailing slash", func(t *testing.T) { + cfg.RendererServerUrl = "" + cfg.RendererCallbackUrl = "https://public-grafana.com" + + rs, err := ProvideService(cfg, featuremgmt.WithFeatures(), nil, &dummyPluginManager{}) + require.NoError(t, err) + + require.Equal(t, "", rs.Cfg.RendererServerUrl) + require.Equal(t, "https://public-grafana.com/", rs.rendererCallbackURL) + require.Equal(t, "public-grafana.com", rs.domain) + }) + + t.Run("RendererCallbackURL is invalid", func(t *testing.T) { + cfg.RendererServerUrl = "" + cfg.RendererCallbackUrl = "http://public{grafana" + + _, err := ProvideService(cfg, featuremgmt.WithFeatures(), nil, &dummyPluginManager{}) + require.Error(t, err) + }) +} diff --git a/pkg/setting/setting.go b/pkg/setting/setting.go index c20b0468699..4a77bfdbf04 100644 --- a/pkg/setting/setting.go +++ b/pkg/setting/setting.go @@ -144,7 +144,7 @@ type Cfg struct { ImagesDir string CSVsDir string PDFsDir string - RendererUrl string + RendererServerUrl string RendererCallbackUrl string RendererAuthToken string RendererConcurrentRequestLimit int @@ -1168,9 +1168,7 @@ func (cfg *Cfg) parseINIFile(iniFile *ini.File) error { cfg.readZanzanaSettings() - if err := cfg.readRenderingSettings(iniFile); err != nil { - return err - } + cfg.readRenderingSettings(iniFile) cfg.TempDataLifetime = iniFile.Section("paths").Key("temp_data_lifetime").MustDuration(time.Second * 3600 * 24) cfg.MetricsEndpointEnabled = iniFile.Section("metrics").Key("enabled").MustBool(true) @@ -1796,26 +1794,12 @@ func readServiceAccountSettings(iniFile *ini.File, cfg *Cfg) error { return nil } -func (cfg *Cfg) readRenderingSettings(iniFile *ini.File) error { +func (cfg *Cfg) readRenderingSettings(iniFile *ini.File) { renderSec := iniFile.Section("rendering") - cfg.RendererUrl = valueAsString(renderSec, "server_url", "") + cfg.RendererServerUrl = valueAsString(renderSec, "server_url", "") cfg.RendererCallbackUrl = valueAsString(renderSec, "callback_url", "") cfg.RendererAuthToken = valueAsString(renderSec, "renderer_token", "-") - if cfg.RendererCallbackUrl == "" { - cfg.RendererCallbackUrl = AppUrl - } else { - if cfg.RendererCallbackUrl[len(cfg.RendererCallbackUrl)-1] != '/' { - cfg.RendererCallbackUrl += "/" - } - _, err := url.Parse(cfg.RendererCallbackUrl) - if err != nil { - // XXX: Should return an error? - cfg.Logger.Error("Invalid callback_url.", "url", cfg.RendererCallbackUrl, "error", err) - os.Exit(1) - } - } - cfg.RendererConcurrentRequestLimit = renderSec.Key("concurrent_render_request_limit").MustInt(30) cfg.RendererRenderKeyLifeTime = renderSec.Key("render_key_lifetime").MustDuration(5 * time.Minute) cfg.RendererDefaultImageWidth = renderSec.Key("default_image_width").MustInt(1000) @@ -1824,8 +1808,6 @@ func (cfg *Cfg) readRenderingSettings(iniFile *ini.File) error { cfg.ImagesDir = filepath.Join(cfg.DataPath, "png") cfg.CSVsDir = filepath.Join(cfg.DataPath, "csv") cfg.PDFsDir = filepath.Join(cfg.DataPath, "pdf") - - return nil } func (cfg *Cfg) readAlertingSettings(iniFile *ini.File) error { diff --git a/pkg/setting/setting_test.go b/pkg/setting/setting_test.go index b112bd3130a..0077bf27ef4 100644 --- a/pkg/setting/setting_test.go +++ b/pkg/setting/setting_test.go @@ -33,7 +33,7 @@ func TestLoadingSettings(t *testing.T) { require.Nil(t, err) require.Equal(t, "admin", cfg.AdminUser) - require.Equal(t, "http://localhost:3000/", cfg.RendererCallbackUrl) + require.Equal(t, "", cfg.RendererCallbackUrl) require.Equal(t, "TLS1.2", cfg.MinTLSVersion) }) @@ -255,17 +255,6 @@ func TestLoadingSettings(t *testing.T) { require.Equal(t, hostname, cfg.InstanceName) }) - t.Run("Reading callback_url should add trailing slash", func(t *testing.T) { - cfg := NewCfg() - err := cfg.Load(CommandLineArgs{ - HomePath: "../../", - Args: []string{"cfg:rendering.callback_url=http://myserver/renderer"}, - }) - require.Nil(t, err) - - require.Equal(t, "http://myserver/renderer/", cfg.RendererCallbackUrl) - }) - t.Run("Only sync_ttl should return the value sync_ttl", func(t *testing.T) { cfg := NewCfg() err := cfg.Load(CommandLineArgs{