From fe3e5917f1bc83ab29b6a57578316e054718aa4a Mon Sep 17 00:00:00 2001 From: Marcus Efraimsson Date: Wed, 5 Jun 2024 13:36:14 +0200 Subject: [PATCH] Plugins: Preserve trailing slash in plugin proxy (#86859) * Plugins: Preserve trailing slash in plugin proxy * enable toggle by default --- .../feature-toggles/index.md | 1 + .../src/types/featureToggles.gen.ts | 1 + pkg/api/plugin_proxy_test.go | 4 ++++ pkg/api/pluginproxy/ds_proxy_test.go | 14 ++++++++++++++ pkg/api/pluginproxy/pluginproxy.go | 6 ++++++ pkg/api/pluginproxy/pluginproxy_test.go | 19 ++++++++++++++++++- .../datasourceproxy/datasourceproxy_test.go | 4 ++++ pkg/services/featuremgmt/registry.go | 7 +++++++ pkg/services/featuremgmt/toggles_gen.csv | 1 + pkg/services/featuremgmt/toggles_gen.go | 4 ++++ pkg/services/featuremgmt/toggles_gen.json | 12 ++++++++++++ 11 files changed, 72 insertions(+), 1 deletion(-) diff --git a/docs/sources/setup-grafana/configure-grafana/feature-toggles/index.md b/docs/sources/setup-grafana/configure-grafana/feature-toggles/index.md index 43d769862d5..55e8d553b36 100644 --- a/docs/sources/setup-grafana/configure-grafana/feature-toggles/index.md +++ b/docs/sources/setup-grafana/configure-grafana/feature-toggles/index.md @@ -64,6 +64,7 @@ Most [generally available](https://grafana.com/docs/release-life-cycle/#general- | `alertingQueryOptimization` | Optimizes eligible queries in order to reduce load on datasources | | | `betterPageScrolling` | Removes CustomScrollbar from the UI, relying on native browser scrollbars | Yes | | `cloudWatchNewLabelParsing` | Updates CloudWatch label parsing to be more accurate | Yes | +| `pluginProxyPreserveTrailingSlash` | Preserve plugin proxy trailing slash. | Yes | ## Public preview feature toggles diff --git a/packages/grafana-data/src/types/featureToggles.gen.ts b/packages/grafana-data/src/types/featureToggles.gen.ts index 94c9266957e..272d45a9abf 100644 --- a/packages/grafana-data/src/types/featureToggles.gen.ts +++ b/packages/grafana-data/src/types/featureToggles.gen.ts @@ -190,4 +190,5 @@ export interface FeatureToggles { alertingDisableSendAlertsExternal?: boolean; preserveDashboardStateWhenNavigating?: boolean; alertingCentralAlertHistory?: boolean; + pluginProxyPreserveTrailingSlash?: boolean; } diff --git a/pkg/api/plugin_proxy_test.go b/pkg/api/plugin_proxy_test.go index 96cad24dd2b..76d58c1fa83 100644 --- a/pkg/api/plugin_proxy_test.go +++ b/pkg/api/plugin_proxy_test.go @@ -27,6 +27,10 @@ func TestExtractPluginProxyPath(t *testing.T) { "/api/plugin-proxy/cloudflare-app/with-token/api/v4/accounts", "with-token/api/v4/accounts", }, + { + "/api/plugin-proxy/cloudflare-app/with-token/api/v4/accounts/", + "with-token/api/v4/accounts/", + }, } for _, tc := range testCases { t.Run("Given raw path, should extract expected proxy path", func(t *testing.T) { diff --git a/pkg/api/pluginproxy/ds_proxy_test.go b/pkg/api/pluginproxy/ds_proxy_test.go index 61198115755..acd8e6cd8fe 100644 --- a/pkg/api/pluginproxy/ds_proxy_test.go +++ b/pkg/api/pluginproxy/ds_proxy_test.go @@ -104,6 +104,10 @@ func TestDataSourceProxy_routeRule(t *testing.T) { URL: "http://www.test.com", Body: []byte(`{ "url": "{{.JsonData.dynamicUrl}}", "secret": "{{.SecureJsonData.key}}" }`), }, + { + Path: "mypath", + URL: "https://example.com/api/v1/", + }, } ds := &datasources.DataSource{ @@ -209,6 +213,16 @@ func TestDataSourceProxy_routeRule(t *testing.T) { require.Equal(t, `{ "url": "https://dynamic.grafana.com", "secret": "123" }`, string(content)) }) + t.Run("When matching route path ending with a slash", func(t *testing.T) { + ctx, req := setUp() + proxy, err := setupDSProxyTest(t, ctx, ds, routes, "mypath/some-route/") + require.NoError(t, err) + proxy.matchedRoute = routes[6] + ApplyRoute(proxy.ctx.Req.Context(), req, proxy.proxyPath, proxy.matchedRoute, dsInfo, proxy.cfg) + + assert.Equal(t, "https://example.com/api/v1/some-route/", req.URL.String()) + }) + t.Run("Validating request", func(t *testing.T) { t.Run("plugin route with valid role", func(t *testing.T) { ctx, _ := setUp() diff --git a/pkg/api/pluginproxy/pluginproxy.go b/pkg/api/pluginproxy/pluginproxy.go index 5a959d97ab6..0430278e8af 100644 --- a/pkg/api/pluginproxy/pluginproxy.go +++ b/pkg/api/pluginproxy/pluginproxy.go @@ -6,6 +6,7 @@ import ( "io" "net/http" "net/url" + "strings" "go.opentelemetry.io/otel/attribute" @@ -76,7 +77,12 @@ func (proxy *PluginProxy) HandleRequest() { } if path, exists := params["*"]; exists { + hasSlash := strings.HasSuffix(proxy.proxyPath, "/") proxy.proxyPath = path + + if hasSlash && !strings.HasSuffix(path, "/") && proxy.features.IsEnabled(proxy.ctx.Req.Context(), featuremgmt.FlagPluginProxyPreserveTrailingSlash) { + proxy.proxyPath += "/" + } } else { proxy.proxyPath = "" } diff --git a/pkg/api/pluginproxy/pluginproxy_test.go b/pkg/api/pluginproxy/pluginproxy_test.go index 79bf30e1c1a..7bd4634de0d 100644 --- a/pkg/api/pluginproxy/pluginproxy_test.go +++ b/pkg/api/pluginproxy/pluginproxy_test.go @@ -316,10 +316,16 @@ func TestPluginProxyRoutes(t *testing.T) { Method: "GET", URL: "http://localhost/api/v2/instances", }, + { + Path: "/mypath/*", + Method: "GET", + URL: "https://example.com/api/v1/", + }, } tcs := []struct { proxyPath string + withFeatures []any expectedURLPath string expectedStatus int }{ @@ -362,6 +368,17 @@ func TestPluginProxyRoutes(t *testing.T) { expectedURLPath: "/api/v2/instances/instance-one", expectedStatus: http.StatusOK, }, + { + proxyPath: "/mypath/some-route/", + expectedURLPath: "/api/v1/some-route", + expectedStatus: http.StatusOK, + }, + { + proxyPath: "/mypath/some-route/", + withFeatures: []any{featuremgmt.FlagPluginProxyPreserveTrailingSlash}, + expectedURLPath: "/api/v1/some-route/", + expectedStatus: http.StatusOK, + }, } for _, tc := range tcs { @@ -404,7 +421,7 @@ func TestPluginProxyRoutes(t *testing.T) { SecureJSONData: map[string][]byte{}, } cfg := &setting.Cfg{} - proxy, err := NewPluginProxy(ps, testRoutes, ctx, tc.proxyPath, cfg, secretsService, tracing.InitializeTracerForTest(), &http.Transport{}, acimpl.ProvideAccessControl(featuremgmt.WithFeatures()), featuremgmt.WithFeatures()) + proxy, err := NewPluginProxy(ps, testRoutes, ctx, tc.proxyPath, cfg, secretsService, tracing.InitializeTracerForTest(), &http.Transport{}, acimpl.ProvideAccessControl(featuremgmt.WithFeatures()), featuremgmt.WithFeatures(tc.withFeatures...)) require.NoError(t, err) proxy.HandleRequest() diff --git a/pkg/services/datasourceproxy/datasourceproxy_test.go b/pkg/services/datasourceproxy/datasourceproxy_test.go index cf26cfab381..9a5e4f73401 100644 --- a/pkg/services/datasourceproxy/datasourceproxy_test.go +++ b/pkg/services/datasourceproxy/datasourceproxy_test.go @@ -57,6 +57,10 @@ func TestDataProxy(t *testing.T) { "/api/datasources/proxy/uid/26MI0wZ7k/api/services/afsd%2Fafsd/operations", "api/services/afsd%2Fafsd/operations", }, + { + "/api/datasources/proxy/uid/26MI0wZ7k/api/services/afsd%2Fafsd/operations/", + "api/services/afsd%2Fafsd/operations/", + }, } for _, tc := range testCases { t.Run("Given raw path, should extract expected proxy path", func(t *testing.T) { diff --git a/pkg/services/featuremgmt/registry.go b/pkg/services/featuremgmt/registry.go index 2307eda3d73..cbe98b57e7e 100644 --- a/pkg/services/featuremgmt/registry.go +++ b/pkg/services/featuremgmt/registry.go @@ -1285,6 +1285,13 @@ var ( Owner: grafanaAlertingSquad, FrontendOnly: true, }, + { + Name: "pluginProxyPreserveTrailingSlash", + Description: "Preserve plugin proxy trailing slash.", + Stage: FeatureStageGeneralAvailability, + Owner: grafanaPluginsPlatformSquad, + Expression: "true", // enabled by default + }, } ) diff --git a/pkg/services/featuremgmt/toggles_gen.csv b/pkg/services/featuremgmt/toggles_gen.csv index 34e38508a9f..37a96386636 100644 --- a/pkg/services/featuremgmt/toggles_gen.csv +++ b/pkg/services/featuremgmt/toggles_gen.csv @@ -171,3 +171,4 @@ datasourceProxyDisableRBAC,GA,@grafana/identity-access-team,false,false,false alertingDisableSendAlertsExternal,experimental,@grafana/alerting-squad,false,false,false preserveDashboardStateWhenNavigating,experimental,@grafana/dashboards-squad,false,false,false alertingCentralAlertHistory,experimental,@grafana/alerting-squad,false,false,true +pluginProxyPreserveTrailingSlash,GA,@grafana/plugins-platform-backend,false,false,false diff --git a/pkg/services/featuremgmt/toggles_gen.go b/pkg/services/featuremgmt/toggles_gen.go index 966666f7b3b..487fb6dc8b2 100644 --- a/pkg/services/featuremgmt/toggles_gen.go +++ b/pkg/services/featuremgmt/toggles_gen.go @@ -694,4 +694,8 @@ const ( // FlagAlertingCentralAlertHistory // Enables the new central alert history. FlagAlertingCentralAlertHistory = "alertingCentralAlertHistory" + + // FlagPluginProxyPreserveTrailingSlash + // Preserve plugin proxy trailing slash. + FlagPluginProxyPreserveTrailingSlash = "pluginProxyPreserveTrailingSlash" ) diff --git a/pkg/services/featuremgmt/toggles_gen.json b/pkg/services/featuremgmt/toggles_gen.json index e1c031d5127..7e47611e0ba 100644 --- a/pkg/services/featuremgmt/toggles_gen.json +++ b/pkg/services/featuremgmt/toggles_gen.json @@ -2212,6 +2212,18 @@ "stage": "GA", "codeowner": "@grafana/observability-logs" } + }, + { + "metadata": { + "name": "pluginProxyPreserveTrailingSlash", + "resourceVersion": "1717581171624", + "creationTimestamp": "2024-06-05T09:52:51Z" + }, + "spec": { + "description": "Preserve plugin proxy trailing slash.", + "stage": "GA", + "codeowner": "@grafana/plugins-platform-backend" + } } ] } \ No newline at end of file