From 521ab33dafcc567ffd4748ec1fb5769f68202c76 Mon Sep 17 00:00:00 2001 From: Mahendra Paipuri <44365948+mahendrapaipuri@users.noreply.github.com> Date: Wed, 15 Jan 2025 10:25:00 +0100 Subject: [PATCH] Handle cases when backend context does not have Grafana version (#236) * fix: Dont use headless chrome from grafana-image-renderer on Windows. Seems like it generates log files in the headless chrome folder which violates the MANIFEST file of plugin. So Grafana refuses to run plugin. * refactor: Fetch Grafana version from frontend when backend does not have one. Seems like Grafana backend might fail setting the correct version of the plugin. In that case we try to get version from frontend plugin settings. Fix comparing sem vers on panels JS. Handle pre and post releases in `semver.Compare` in backend. * Add a e2e test for Grafana 11.3.0+security-01 version that tests these fixes * test: Update expected e2e test outputs. Increase timeout for e2e tests --------- Signed-off-by: Mahendra Paipuri --- .ci/config/plain/app.yaml | 1 + .ci/config/tls/app.yaml | 2 +- .github/workflows/step_e2e-tests.yml | 8 ++++ pkg/plugin/app.go | 14 ++++-- pkg/plugin/chrome/local.go | 41 ++++++++++++---- pkg/plugin/config/settings.go | 6 +++ pkg/plugin/dashboard/js/panels.js | 23 +++++++-- pkg/plugin/helpers/helpers.go | 21 +++++++++ pkg/plugin/helpers/helpers_test.go | 65 ++++++++++++++++++++++++++ pkg/plugin/permissions.go | 4 +- pkg/plugin/resources.go | 6 +-- src/components/AppConfig/AppConfig.tsx | 7 +++ tests/appReport.spec.ts | 22 +++++---- tests/appReportNormalUser.spec.ts | 5 +- 14 files changed, 194 insertions(+), 31 deletions(-) create mode 100644 pkg/plugin/helpers/helpers_test.go diff --git a/.ci/config/plain/app.yaml b/.ci/config/plain/app.yaml index e0884dd..3335af2 100644 --- a/.ci/config/plain/app.yaml +++ b/.ci/config/plain/app.yaml @@ -9,3 +9,4 @@ apps: jsonData: maxBrowserWorkers: 10 maxRenderWorkers: 10 + timeout: 60 diff --git a/.ci/config/tls/app.yaml b/.ci/config/tls/app.yaml index bd852f2..5fc8be7 100644 --- a/.ci/config/tls/app.yaml +++ b/.ci/config/tls/app.yaml @@ -16,4 +16,4 @@ apps: logo: '' maxBrowserWorkers: 10 maxRenderWorkers: 10 - persistData: false + timeout: 60 diff --git a/.github/workflows/step_e2e-tests.yml b/.github/workflows/step_e2e-tests.yml index 353a2ed..b935391 100644 --- a/.github/workflows/step_e2e-tests.yml +++ b/.github/workflows/step_e2e-tests.yml @@ -47,6 +47,14 @@ jobs: # snapshots-folder: remote-chrome name: remote-chrome-11.1.0-with-features + # Grafana v11.3.0+security-01 with local chrome when backend does not have Grafana + # version + - grafana-version: 11.3.0-security-01 + feature-flags: 'accessControlOnCall,idForwarding,externalServiceAccounts' + native-rendering: false + # snapshots-folder: remote-chrome + name: remote-chrome-11.3.0-security-with-features + # Latest Grafana with local chrome and grafana-image-renderer - grafana-version: 11.4.0 remote-chrome-url: ws://localhost:9222 diff --git a/pkg/plugin/app.go b/pkg/plugin/app.go index 9fb668d..df04cda 100644 --- a/pkg/plugin/app.go +++ b/pkg/plugin/app.go @@ -74,6 +74,17 @@ func NewDashboardReporterApp(ctx context.Context, settings backend.AppInstanceSe app.ctxLogger.Info("starting plugin with initial config: " + app.conf.String()) + // Get current Grafana version + app.grafanaSemVer = "v" + backend.UserAgentFromContext(ctx).GrafanaVersion() + + if app.grafanaSemVer == "v0.0.0" && app.conf.AppVersion != "0.0.0" { + app.grafanaSemVer = "v" + app.conf.AppVersion + + app.ctxLogger.Debug("got grafana version from plugin settings", "version", app.grafanaSemVer) + } else { + app.ctxLogger.Debug("got grafana version from backend user agent", "version", app.grafanaSemVer) + } + // Make a new HTTP client if app.httpClient, err = httpclient.New(app.conf.HTTPClientOptions); err != nil { return nil, fmt.Errorf("error in httpclient new: %w", err) @@ -117,9 +128,6 @@ func NewDashboardReporterApp(ctx context.Context, settings backend.AppInstanceSe worker.Renderer: worker.New(context.Background(), app.conf.MaxRenderWorkers), } - // Get current Grafana version - app.grafanaSemVer = "v" + backend.UserAgentFromContext(ctx).GrafanaVersion() - return &app, nil } diff --git a/pkg/plugin/chrome/local.go b/pkg/plugin/chrome/local.go index f0e39b2..33a1a40 100644 --- a/pkg/plugin/chrome/local.go +++ b/pkg/plugin/chrome/local.go @@ -6,6 +6,7 @@ import ( "os" "os/exec" "path/filepath" + "runtime" "slices" "time" @@ -22,6 +23,13 @@ var ( ) func init() { + // Seems like using the same headless chrome distibution from grafana-image-renderer + // does not work on Windows. Until we ship our own distribution of headless chrome, + // skip this part. + if runtime.GOOS == "windows" { + return + } + // Get Grafana data path based on path of current executable pluginExe, err := os.Executable() if err != nil { @@ -47,7 +55,7 @@ func init() { } // In recent releases of grafana-image-renderer, the binary is called chrome-headless-shell - validChromeBins := []string{"chrome", "chrome-headless-shell"} + validChromeBins := []string{"chrome", "chrome.exe", "chrome-headless-shell", "chrome-headless-shell.exe"} if !info.IsDir() && slices.Contains(validChromeBins, info.Name()) { // If the chrome shipped is not "usable", plugin cannot be used // even a "usable" chrome (for instance chromium installed using snap on Ubuntu) @@ -59,7 +67,10 @@ func init() { defer cancel() // This command should print an empty DOM and exit - if _, err := exec.CommandContext(ctx, path, "--headless", "--no-sandbox", "--disable-gpu", "--dump-dom").Output(); err == nil { + if _, err := exec.CommandContext( + ctx, path, "--headless", "--no-sandbox", "--disable-gpu", + "--disable-logging ", "--dump-dom", + ).Output(); err == nil { chromeExec = path } @@ -67,7 +78,8 @@ func init() { } return nil - }) + }, + ) } // LocalInstance is a locally running browser instance. @@ -90,13 +102,24 @@ func NewLocalBrowserInstance(ctx context.Context, logger log.Logger, insecureSki // If we managed to create a home for chrome in a "writable" location, set it to chrome options if chromeHomeDir != "" { logger.Debug("created home directory for chromium process", "home", chromeHomeDir) - chromeOptions = append(chromeOptions, chromedp.Env("XDG_CONFIG_HOME="+chromeHomeDir, "XDG_CACHE_HOME="+chromeHomeDir)) - } - // If chromExec is not empty we found chrome binary shipped by grafana-image-renderer - if chromeExec != "" { - logger.Info("chrome executable provided by grafana-image-renderer will be used", "chrome", chromeExec) - chromeOptions = append(chromeOptions, chromedp.ExecPath(chromeExec)) + // Seems like on windows using headless chrome distributed by grafana-image-renderer + // produces a debug log of chrome in the same folder which violates the list of + // files distributed in the MANIFEST and hence, Grafana refuses to run grafana-image-renderer. + // So, override default chrome's --log-file location to the new home that we created for + // chrome. + chromeOptions = append( + chromeOptions, chromedp.Env( + "XDG_CONFIG_HOME="+chromeHomeDir, "XDG_CACHE_HOME="+chromeHomeDir, + "CHROME_LOG_FILE="+filepath.Join(chromeHomeDir, "debug.log"), + ), + ) + + // If we managed to make chrome home dir and find chrom exec from `grafana-image-renderer` use it. + if chromeExec != "" { + logger.Info("chrome executable provided by grafana-image-renderer will be used", "chrome", chromeExec) + chromeOptions = append(chromeOptions, chromedp.ExecPath(chromeExec)) + } } if insecureSkipVerify { diff --git a/pkg/plugin/config/settings.go b/pkg/plugin/config/settings.go index 3592a38..b7950ec 100644 --- a/pkg/plugin/config/settings.go +++ b/pkg/plugin/config/settings.go @@ -42,6 +42,7 @@ type Config struct { MaxRenderWorkers int `env:"GF_REPORTER_PLUGIN_MAX_RENDER_WORKERS, overwrite" json:"maxRenderWorkers"` RemoteChromeURL string `env:"GF_REPORTER_PLUGIN_REMOTE_CHROME_URL, overwrite" json:"remoteChromeUrl"` NativeRendering bool `env:"GF_REPORTER_PLUGIN_NATIVE_RENDERER, overwrite" json:"nativeRenderer"` + AppVersion string `json:"appVersion"` IncludePanelIDs []string ExcludePanelIDs []string IncludePanelDataIDs []string @@ -105,6 +106,11 @@ func (c *Config) Validate() error { } } + // If AppVersion is empty, set it to 0.0.0 + if c.AppVersion == "" { + c.AppVersion = "0.0.0" + } + return nil } diff --git a/pkg/plugin/dashboard/js/panels.js b/pkg/plugin/dashboard/js/panels.js index d233be3..fa87757 100644 --- a/pkg/plugin/dashboard/js/panels.js +++ b/pkg/plugin/dashboard/js/panels.js @@ -2,6 +2,9 @@ // and panels are fully loaded on the current Grafana // dashboard +// Fallback version string +const fallbackVersion = '11.3.0' + // Base backoff duration in ms const baseDelayMsecs = 10; @@ -16,17 +19,31 @@ const panelData = selector => [...document.querySelectorAll('[' + selector + ']' * #see https://semver.org/ * #see https://stackoverflow.com/a/65687141/456536 * #see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl/Collator/Collator#options + * + * Seems like Grafana uses "-" for pre-releases and "+" for post releases (bug fixes) */ function semverCompare(a, b) { + // Pre-releases if (a.startsWith(b + "-")) {return -1} if (b.startsWith(a + "-")) {return 1} + + // Post releases + if (a.startsWith(b + "+")) {return 1} + if (b.startsWith(a + "+")) {return -1} return a.localeCompare(b, undefined, { numeric: true, sensitivity: "case", caseFirst: "upper" }) } // Wait for queries to finish and panels to load data -const waitForQueriesAndVisualizations = async (version = '11.3.0', mode = 'default', timeout = 30000) => { +const waitForQueriesAndVisualizations = async (version = `v${fallbackVersion}`, mode = 'default', timeout = 30000) => { // Remove v prefix from version - const ver = version.split('v')[1]; + let ver = version.split('v')[1] || '0.0.0'; + + // Seems like Grafana is CAPABLE of sending zero version string + // on backend plugin. In that case attempt to get version from + // frontend boot data + if (semverCompare(ver, '0.0.0') === 0) { + ver = grafanaBootData?.settings?.buildInfo?.version || fallbackVersion + } // Set selector based on version let selector; @@ -128,7 +145,7 @@ const checkFormatDataToggle = async () => { }; // Waits for CSV data to be ready to download -const waitForCSVData = async (version = '11.3.0', timeout = 30000) => { +const waitForCSVData = async (version = `v${fallbackVersion}`, timeout = 30000) => { // First wait for panel to load data await waitForQueriesAndVisualizations(version, 'default', timeout); diff --git a/pkg/plugin/helpers/helpers.go b/pkg/plugin/helpers/helpers.go index d45a063..ed55a4c 100644 --- a/pkg/plugin/helpers/helpers.go +++ b/pkg/plugin/helpers/helpers.go @@ -1,9 +1,11 @@ package helpers import ( + "strings" "time" "github.com/grafana/grafana-plugin-sdk-go/backend/log" + "golang.org/x/mod/semver" ) // TimeTrack tracks execution time of each function. @@ -12,3 +14,22 @@ func TimeTrack(start time.Time, name string, logger log.Logger, args ...interfac args = append(args, "duration", elapsed.String()) logger.Debug(name, args...) } + +// SemverCompare compares the semantic version of Grafana versions. +// Grafana uses "+" as post release suffix and "-" as pre-release +// suffixes. We take that into account when calling upstream semver +// package. +func SemverCompare(a, b string) int { + switch { + case strings.HasPrefix(a, b+"+"): + return 1 + case strings.HasPrefix(b, a+"+"): + return -1 + case strings.HasPrefix(a, b+"-"): + return -1 + case strings.HasPrefix(b, a+"-"): + return 1 + } + + return semver.Compare(a, b) +} diff --git a/pkg/plugin/helpers/helpers_test.go b/pkg/plugin/helpers/helpers_test.go new file mode 100644 index 0000000..410bdc6 --- /dev/null +++ b/pkg/plugin/helpers/helpers_test.go @@ -0,0 +1,65 @@ +package helpers + +import ( + "testing" + + . "github.com/smartystreets/goconvey/convey" +) + +func TestSemverComapre(t *testing.T) { + Convey("When comparing semantic versions", t, func() { + tests := []struct { + name string + a, b string + expected int + }{ + { + name: "regular sem ver comparison", + a: "v1.2.3", + b: "v1.2.5", + expected: -1, + }, + { + name: "regular sem ver with pre-release comparison", + a: "v1.2.3", + b: "v1.2.3-rc0", + expected: 1, + }, + { + name: "regular sem ver with pre-release comparison with inverse order", + a: "v1.2.3-rc1", + b: "v1.2.3", + expected: -1, + }, + { + name: "regular sem ver with post-release comparison", + a: "v1.2.3", + b: "v1.2.3+security-01", + expected: -1, + }, + { + name: "regular sem ver with post-release comparison with inverse order", + a: "v1.2.3+security-01", + b: "v1.2.3", + expected: 1, + }, + { + name: "comparison with zero version", + a: "v0.0.0", + b: "v1.2.5", + expected: -1, + }, + { + name: "comparison with zero version with inverse order", + a: "v1.2.3", + b: "v0.0.0", + expected: 1, + }, + } + + for _, test := range tests { + got := SemverCompare(test.a, test.b) + So(got, ShouldEqual, test.expected) + } + }) +} diff --git a/pkg/plugin/permissions.go b/pkg/plugin/permissions.go index 06aa00b..811e226 100644 --- a/pkg/plugin/permissions.go +++ b/pkg/plugin/permissions.go @@ -10,7 +10,7 @@ import ( "github.com/mahendrapaipuri/authlib/authn" "github.com/mahendrapaipuri/authlib/authz" "github.com/mahendrapaipuri/authlib/cache" - "golang.org/x/mod/semver" + "github.com/mahendrapaipuri/grafana-dashboard-reporter-app/pkg/plugin/helpers" ) // HasAccess verifies if the current request context has access to certain action. @@ -77,7 +77,7 @@ func (app *App) GetAuthZClient(req *http.Request) (authz.EnforcementClient, erro // So this check will fail for Grafana < 11.1.0 // Set VerifierConfig{DisableTypHeaderCheck: true} for those cases disableTypHeaderCheck := false - if semver.Compare(app.grafanaSemVer, "v11.1.0") == -1 { + if helpers.SemverCompare(app.grafanaSemVer, "v11.1.0") == -1 { disableTypHeaderCheck = true } diff --git a/pkg/plugin/resources.go b/pkg/plugin/resources.go index c1c4641..0bd13d5 100644 --- a/pkg/plugin/resources.go +++ b/pkg/plugin/resources.go @@ -15,8 +15,8 @@ import ( "github.com/mahendrapaipuri/authlib/authz" "github.com/mahendrapaipuri/grafana-dashboard-reporter-app/pkg/plugin/config" "github.com/mahendrapaipuri/grafana-dashboard-reporter-app/pkg/plugin/dashboard" + "github.com/mahendrapaipuri/grafana-dashboard-reporter-app/pkg/plugin/helpers" "github.com/mahendrapaipuri/grafana-dashboard-reporter-app/pkg/plugin/report" - "golang.org/x/mod/semver" ) // GrafanaUserSignInTokenHeaderName the header name used for forwarding @@ -33,7 +33,7 @@ const ( // convertPanelIDs returns panel IDs based on Grafana version. func (app *App) convertPanelIDs(ids []string) []string { // For Grafana < 11.3.0, we can use the IDs as such - if semver.Compare(app.grafanaSemVer, "v11.3.0") == -1 { + if helpers.SemverCompare(app.grafanaSemVer, "v11.3.0") == -1 { return ids } @@ -107,7 +107,7 @@ func (app *App) updateConfig(req *http.Request, conf *config.Config) { func (app *App) featureTogglesEnabled(ctx context.Context) bool { // If Grafana <= 10.4.3, we use cookies to make request. Moreover feature toggles are // not available for these Grafana versions. - if semver.Compare(app.grafanaSemVer, "v10.4.3") <= -1 { + if helpers.SemverCompare(app.grafanaSemVer, "v10.4.3") <= -1 { return false } diff --git a/src/components/AppConfig/AppConfig.tsx b/src/components/AppConfig/AppConfig.tsx index bb1d841..4e8a051 100755 --- a/src/components/AppConfig/AppConfig.tsx +++ b/src/components/AppConfig/AppConfig.tsx @@ -24,6 +24,7 @@ import { testIds } from "../testIds"; export type JsonData = { appUrl?: string; + appVersion?: string; skipTlsCheck?: boolean; theme?: string; orientation?: string; @@ -46,6 +47,8 @@ type State = { appUrlChanged: boolean; // Skip TLS verification to grafana skipTlsCheck: boolean; + // Grafana version + appVersion: string; // If skipTlsCheck has changed skipTlsCheckChanged: boolean; // Theme of panels (light or dark) @@ -114,6 +117,7 @@ export const AppConfig = ({ plugin }: Props) => { const [state, setState] = useState({ appUrl: jsonData?.appUrl || "", appUrlChanged: false, + appVersion: window?.grafanaBootData?.settings?.buildInfo?.version || "0.0.0", skipTlsCheck: jsonData?.skipTlsCheck || false, skipTlsCheckChanged: false, theme: jsonData?.theme || "light", @@ -310,6 +314,7 @@ export const AppConfig = ({ plugin }: Props) => { pinned: true, jsonData: { appUrl: state.appUrl, + appVersion: state.appVersion, skipTlsCheck: state.skipTlsCheck, theme: state.theme, orientation: state.orientation, @@ -352,6 +357,7 @@ export const AppConfig = ({ plugin }: Props) => { pinned: false, jsonData: { appUrl: state.appUrl, + appVersion: state.appVersion, skipTlsCheck: state.skipTlsCheck, theme: state.theme, orientation: state.orientation, @@ -665,6 +671,7 @@ export const AppConfig = ({ plugin }: Props) => { pinned, jsonData: { appUrl: state.appUrl, + appVersion: state.appVersion, skipTlsCheck: state.skipTlsCheck, theme: state.theme, orientation: state.orientation, diff --git a/tests/appReport.spec.ts b/tests/appReport.spec.ts index 3746cc6..9c5ae5a 100644 --- a/tests/appReport.spec.ts +++ b/tests/appReport.spec.ts @@ -16,15 +16,21 @@ test("should be possible to generate report", async ({ request }, testInfo) => { ); // TLS case will attempt to create a report by a user without View permission - // on dashboard except for the case where Grafana 10.4.7 and not using appropriate - // feature toogles. In the exceptional case, the test should pass - if ( - testInfo.project.name === "tlsViewerUser" && - process.env.GRAFANA_VERSION !== "10.4.7" && - process.env.GF_FEATURE_TOGGLES_ENABLE !== "externalServiceAccounts" - ) { - expect(report.ok()).toBeFalsy(); + // on dashboard which should fail to create report. Exceptional cases are: + // - Grafana 10.4.7 and not using appropriate feature toogles. + // - Grafana 11.3.0+security-01 when Grafana version is not available. + // In these exceptional cases, the test should pass + if (testInfo.project.name === "tlsViewerUser") { + if ( + (process.env.GRAFANA_VERSION === "10.4.7" && process.env.GF_FEATURE_TOGGLES_ENABLE === "externalServiceAccounts") || + process.env.GRAFANA_VERSION === "11.3.0-security-01" + ) { + expect(report.ok()).toBeTruthy(); + } else { + expect(report.ok()).toBeFalsy(); + } } else { + // plain case should always pass expect(report.ok()).toBeTruthy(); } }); diff --git a/tests/appReportNormalUser.spec.ts b/tests/appReportNormalUser.spec.ts index d447565..6e9096f 100644 --- a/tests/appReportNormalUser.spec.ts +++ b/tests/appReportNormalUser.spec.ts @@ -18,8 +18,9 @@ test("should not be possible to generate report by a normal user without permiss // report generation should pass. In rest of the cases, it should fail // due to access control if ( - process.env.GRAFANA_VERSION === "10.4.7" && - process.env.GF_FEATURE_TOGGLES_ENABLE === "externalServiceAccounts" + (process.env.GRAFANA_VERSION === "10.4.7" && + process.env.GF_FEATURE_TOGGLES_ENABLE === "externalServiceAccounts") || + process.env.GRAFANA_VERSION === "11.3.0-security-01" ) { expect(report.ok()).toBeTruthy(); } else {