Plugins: Preserve trailing slash in plugin proxy (#86859)

* Plugins: Preserve trailing slash in plugin proxy

* enable toggle by default
This commit is contained in:
Marcus Efraimsson
2024-06-05 13:36:14 +02:00
committed by GitHub
parent 17f03882d4
commit fe3e5917f1
11 changed files with 72 additions and 1 deletions

View File

@@ -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 | | | `alertingQueryOptimization` | Optimizes eligible queries in order to reduce load on datasources | |
| `betterPageScrolling` | Removes CustomScrollbar from the UI, relying on native browser scrollbars | Yes | | `betterPageScrolling` | Removes CustomScrollbar from the UI, relying on native browser scrollbars | Yes |
| `cloudWatchNewLabelParsing` | Updates CloudWatch label parsing to be more accurate | Yes | | `cloudWatchNewLabelParsing` | Updates CloudWatch label parsing to be more accurate | Yes |
| `pluginProxyPreserveTrailingSlash` | Preserve plugin proxy trailing slash. | Yes |
## Public preview feature toggles ## Public preview feature toggles

View File

@@ -190,4 +190,5 @@ export interface FeatureToggles {
alertingDisableSendAlertsExternal?: boolean; alertingDisableSendAlertsExternal?: boolean;
preserveDashboardStateWhenNavigating?: boolean; preserveDashboardStateWhenNavigating?: boolean;
alertingCentralAlertHistory?: boolean; alertingCentralAlertHistory?: boolean;
pluginProxyPreserveTrailingSlash?: boolean;
} }

View File

@@ -27,6 +27,10 @@ func TestExtractPluginProxyPath(t *testing.T) {
"/api/plugin-proxy/cloudflare-app/with-token/api/v4/accounts", "/api/plugin-proxy/cloudflare-app/with-token/api/v4/accounts",
"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 { for _, tc := range testCases {
t.Run("Given raw path, should extract expected proxy path", func(t *testing.T) { t.Run("Given raw path, should extract expected proxy path", func(t *testing.T) {

View File

@@ -104,6 +104,10 @@ func TestDataSourceProxy_routeRule(t *testing.T) {
URL: "http://www.test.com", URL: "http://www.test.com",
Body: []byte(`{ "url": "{{.JsonData.dynamicUrl}}", "secret": "{{.SecureJsonData.key}}" }`), Body: []byte(`{ "url": "{{.JsonData.dynamicUrl}}", "secret": "{{.SecureJsonData.key}}" }`),
}, },
{
Path: "mypath",
URL: "https://example.com/api/v1/",
},
} }
ds := &datasources.DataSource{ 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)) 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("Validating request", func(t *testing.T) {
t.Run("plugin route with valid role", func(t *testing.T) { t.Run("plugin route with valid role", func(t *testing.T) {
ctx, _ := setUp() ctx, _ := setUp()

View File

@@ -6,6 +6,7 @@ import (
"io" "io"
"net/http" "net/http"
"net/url" "net/url"
"strings"
"go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/attribute"
@@ -76,7 +77,12 @@ func (proxy *PluginProxy) HandleRequest() {
} }
if path, exists := params["*"]; exists { if path, exists := params["*"]; exists {
hasSlash := strings.HasSuffix(proxy.proxyPath, "/")
proxy.proxyPath = path proxy.proxyPath = path
if hasSlash && !strings.HasSuffix(path, "/") && proxy.features.IsEnabled(proxy.ctx.Req.Context(), featuremgmt.FlagPluginProxyPreserveTrailingSlash) {
proxy.proxyPath += "/"
}
} else { } else {
proxy.proxyPath = "" proxy.proxyPath = ""
} }

View File

@@ -316,10 +316,16 @@ func TestPluginProxyRoutes(t *testing.T) {
Method: "GET", Method: "GET",
URL: "http://localhost/api/v2/instances", URL: "http://localhost/api/v2/instances",
}, },
{
Path: "/mypath/*",
Method: "GET",
URL: "https://example.com/api/v1/",
},
} }
tcs := []struct { tcs := []struct {
proxyPath string proxyPath string
withFeatures []any
expectedURLPath string expectedURLPath string
expectedStatus int expectedStatus int
}{ }{
@@ -362,6 +368,17 @@ func TestPluginProxyRoutes(t *testing.T) {
expectedURLPath: "/api/v2/instances/instance-one", expectedURLPath: "/api/v2/instances/instance-one",
expectedStatus: http.StatusOK, 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 { for _, tc := range tcs {
@@ -404,7 +421,7 @@ func TestPluginProxyRoutes(t *testing.T) {
SecureJSONData: map[string][]byte{}, SecureJSONData: map[string][]byte{},
} }
cfg := &setting.Cfg{} 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) require.NoError(t, err)
proxy.HandleRequest() proxy.HandleRequest()

View File

@@ -57,6 +57,10 @@ func TestDataProxy(t *testing.T) {
"/api/datasources/proxy/uid/26MI0wZ7k/api/services/afsd%2Fafsd/operations", "/api/datasources/proxy/uid/26MI0wZ7k/api/services/afsd%2Fafsd/operations",
"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 { for _, tc := range testCases {
t.Run("Given raw path, should extract expected proxy path", func(t *testing.T) { t.Run("Given raw path, should extract expected proxy path", func(t *testing.T) {

View File

@@ -1285,6 +1285,13 @@ var (
Owner: grafanaAlertingSquad, Owner: grafanaAlertingSquad,
FrontendOnly: true, FrontendOnly: true,
}, },
{
Name: "pluginProxyPreserveTrailingSlash",
Description: "Preserve plugin proxy trailing slash.",
Stage: FeatureStageGeneralAvailability,
Owner: grafanaPluginsPlatformSquad,
Expression: "true", // enabled by default
},
} }
) )

View File

@@ -171,3 +171,4 @@ datasourceProxyDisableRBAC,GA,@grafana/identity-access-team,false,false,false
alertingDisableSendAlertsExternal,experimental,@grafana/alerting-squad,false,false,false alertingDisableSendAlertsExternal,experimental,@grafana/alerting-squad,false,false,false
preserveDashboardStateWhenNavigating,experimental,@grafana/dashboards-squad,false,false,false preserveDashboardStateWhenNavigating,experimental,@grafana/dashboards-squad,false,false,false
alertingCentralAlertHistory,experimental,@grafana/alerting-squad,false,false,true alertingCentralAlertHistory,experimental,@grafana/alerting-squad,false,false,true
pluginProxyPreserveTrailingSlash,GA,@grafana/plugins-platform-backend,false,false,false
1 Name Stage Owner requiresDevMode RequiresRestart FrontendOnly
171 alertingDisableSendAlertsExternal experimental @grafana/alerting-squad false false false
172 preserveDashboardStateWhenNavigating experimental @grafana/dashboards-squad false false false
173 alertingCentralAlertHistory experimental @grafana/alerting-squad false false true
174 pluginProxyPreserveTrailingSlash GA @grafana/plugins-platform-backend false false false

View File

@@ -694,4 +694,8 @@ const (
// FlagAlertingCentralAlertHistory // FlagAlertingCentralAlertHistory
// Enables the new central alert history. // Enables the new central alert history.
FlagAlertingCentralAlertHistory = "alertingCentralAlertHistory" FlagAlertingCentralAlertHistory = "alertingCentralAlertHistory"
// FlagPluginProxyPreserveTrailingSlash
// Preserve plugin proxy trailing slash.
FlagPluginProxyPreserveTrailingSlash = "pluginProxyPreserveTrailingSlash"
) )

View File

@@ -2212,6 +2212,18 @@
"stage": "GA", "stage": "GA",
"codeowner": "@grafana/observability-logs" "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"
}
} }
] ]
} }