Skip to content

Commit ceb697f

Browse files
zhaohuabingzhaohuabing
authored
Fix: double slashes in redirect URL (#2998)
* fix: double trailing splashs in redirect URL Signed-off-by: huabing zhao <[email protected]> * add e2e tests Signed-off-by: huabing zhao <[email protected]> * fix lint Signed-off-by: huabing zhao <[email protected]> * fix test Signed-off-by: huabing zhao <[email protected]> * fix test Signed-off-by: huabing zhao <[email protected]> * fix test Signed-off-by: huabing zhao <[email protected]> * fix test Signed-off-by: huabing zhao <[email protected]> * add e2e tests Signed-off-by: huabing zhao <[email protected]> * fix test Signed-off-by: huabing zhao <[email protected]> * revert Signed-off-by: huabing zhao <[email protected]> * use regex rewrite to generate the redirect url Signed-off-by: huabing zhao <[email protected]> * use regex rewrite to generate the redirect url Signed-off-by: huabing zhao <[email protected]> * use regex rewrite to generate the redirect url Signed-off-by: huabing zhao <[email protected]> * remove comments Signed-off-by: huabing zhao <[email protected]> * extract method Signed-off-by: huabing zhao <[email protected]> * address comments Signed-off-by: huabing zhao <[email protected]> --------- Signed-off-by: huabing zhao <[email protected]>
1 parent deea895 commit ceb697f

File tree

5 files changed

+232
-15
lines changed

5 files changed

+232
-15
lines changed

‎internal/xds/translator/route.go‎

Lines changed: 34 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@ func buildXdsRoute(httpRoute *ir.HTTPRoute) (*routev3.Route, error) {
5252
case httpRoute.DirectResponse != nil:
5353
router.Action = &routev3.Route_DirectResponse{DirectResponse: buildXdsDirectResponseAction(httpRoute.DirectResponse)}
5454
case httpRoute.Redirect != nil:
55-
router.Action = &routev3.Route_Redirect{Redirect: buildXdsRedirectAction(httpRoute.Redirect)}
55+
router.Action = &routev3.Route_Redirect{Redirect: buildXdsRedirectAction(httpRoute)}
5656
case httpRoute.URLRewrite != nil:
5757
routeAction := buildXdsURLRewriteAction(httpRoute.Destination.Name, httpRoute.URLRewrite, httpRoute.PathMatch)
5858
if httpRoute.Mirrors != nil {
@@ -279,8 +279,11 @@ func idleTimeout(httpRoute *ir.HTTPRoute) *durationpb.Duration {
279279
return nil
280280
}
281281

282-
func buildXdsRedirectAction(redirection *ir.Redirect) *routev3.RedirectAction {
283-
routeAction := &routev3.RedirectAction{}
282+
func buildXdsRedirectAction(httpRoute *ir.HTTPRoute) *routev3.RedirectAction {
283+
var (
284+
redirection = httpRoute.Redirect
285+
routeAction = &routev3.RedirectAction{}
286+
)
284287

285288
if redirection.Scheme != nil {
286289
routeAction.SchemeRewriteSpecifier = &routev3.RedirectAction_SchemeRedirect{
@@ -293,8 +296,14 @@ func buildXdsRedirectAction(redirection *ir.Redirect) *routev3.RedirectAction {
293296
PathRedirect: *redirection.Path.FullReplace,
294297
}
295298
} else if redirection.Path.PrefixMatchReplace != nil {
296-
routeAction.PathRewriteSpecifier = &routev3.RedirectAction_PrefixRewrite{
297-
PrefixRewrite: *redirection.Path.PrefixMatchReplace,
299+
if useRegexRewriteForPrefixMatchReplace(httpRoute.PathMatch, *redirection.Path.PrefixMatchReplace) {
300+
routeAction.PathRewriteSpecifier = &routev3.RedirectAction_RegexRewrite{
301+
RegexRewrite: prefix2RegexRewrite(*httpRoute.PathMatch.Prefix),
302+
}
303+
} else {
304+
routeAction.PathRewriteSpecifier = &routev3.RedirectAction_PrefixRewrite{
305+
PrefixRewrite: *redirection.Path.PrefixMatchReplace,
306+
}
298307
}
299308
}
300309
}
@@ -313,6 +322,24 @@ func buildXdsRedirectAction(redirection *ir.Redirect) *routev3.RedirectAction {
313322
return routeAction
314323
}
315324

325+
// useRegexRewriteForPrefixMatchReplace checks if the regex rewrite should be used for prefix match replace
326+
// due to the issue with Envoy not handling the case of "//" when the replace string is "/".
327+
// See: https://github.com/envoyproxy/envoy/issues/26055
328+
func useRegexRewriteForPrefixMatchReplace(pathMatch *ir.StringMatch, prefixMatchReplace string) bool {
329+
return pathMatch != nil &&
330+
pathMatch.Prefix != nil &&
331+
(prefixMatchReplace == "" || prefixMatchReplace == "/")
332+
}
333+
334+
func prefix2RegexRewrite(prefix string) *matcherv3.RegexMatchAndSubstitute {
335+
return &matcherv3.RegexMatchAndSubstitute{
336+
Pattern: &matcherv3.RegexMatcher{
337+
Regex: "^" + prefix + `\/*`,
338+
},
339+
Substitution: "/",
340+
}
341+
}
342+
316343
func buildXdsURLRewriteAction(destName string, urlRewrite *ir.URLRewrite, pathMatch *ir.StringMatch) *routev3.RouteAction {
317344
routeAction := &routev3.RouteAction{
318345
ClusterSpecifier: &routev3.RouteAction_Cluster{
@@ -333,14 +360,8 @@ func buildXdsURLRewriteAction(destName string, urlRewrite *ir.URLRewrite, pathMa
333360
// An empty replace string does not seem to solve the issue so we are using
334361
// a regex match and replace instead
335362
// Remove this workaround once https://github.com/envoyproxy/envoy/issues/26055 is fixed
336-
if pathMatch != nil && pathMatch.Prefix != nil &&
337-
(*urlRewrite.Path.PrefixMatchReplace == "" || *urlRewrite.Path.PrefixMatchReplace == "/") {
338-
routeAction.RegexRewrite = &matcherv3.RegexMatchAndSubstitute{
339-
Pattern: &matcherv3.RegexMatcher{
340-
Regex: "^" + *pathMatch.Prefix + `\/*`,
341-
},
342-
Substitution: "/",
343-
}
363+
if useRegexRewriteForPrefixMatchReplace(pathMatch, *urlRewrite.Path.PrefixMatchReplace) {
364+
routeAction.RegexRewrite = prefix2RegexRewrite(*pathMatch.Prefix)
344365
} else {
345366
routeAction.PrefixRewrite = *urlRewrite.Path.PrefixMatchReplace
346367
}

‎internal/xds/translator/testdata/in/xds-ir/http-route-redirect.yaml‎

Lines changed: 27 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ http:
99
mergeSlashes: true
1010
escapedSlashesAction: UnescapeAndRedirect
1111
routes:
12-
- name: "redirect-route"
12+
- name: "redirect-route-1"
1313
hostname: "*"
1414
destination:
1515
name: "redirect-route-dest"
@@ -24,3 +24,29 @@ http:
2424
port: 8443
2525
path:
2626
prefixMatchReplace: /redirected
27+
- name: "redirect-route-2"
28+
hostname: "*"
29+
pathMatch:
30+
prefix: "/redirect"
31+
destination:
32+
name: "redirect-route-dest"
33+
settings:
34+
- endpoints:
35+
- host: "1.2.3.4"
36+
port: 50000
37+
redirect:
38+
path:
39+
prefixMatchReplace: /
40+
- name: "redirect-route-3"
41+
hostname: "*"
42+
pathMatch:
43+
prefix: "/redirect/"
44+
destination:
45+
name: "redirect-route-dest"
46+
settings:
47+
- endpoints:
48+
- host: "1.2.3.4"
49+
port: 50000
50+
redirect:
51+
path:
52+
prefixMatchReplace: /

‎internal/xds/translator/testdata/out/xds-ir/http-route-redirect.routes.yaml‎

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,10 +7,26 @@
77
routes:
88
- match:
99
prefix: /
10-
name: redirect-route
10+
name: redirect-route-1
1111
redirect:
1212
hostRedirect: redirected.com
1313
portRedirect: 8443
1414
prefixRewrite: /redirected
1515
responseCode: FOUND
1616
schemeRedirect: https
17+
- match:
18+
pathSeparatedPrefix: /redirect
19+
name: redirect-route-2
20+
redirect:
21+
regexRewrite:
22+
pattern:
23+
regex: ^/redirect\/*
24+
substitution: /
25+
- match:
26+
pathSeparatedPrefix: /redirect
27+
name: redirect-route-3
28+
redirect:
29+
regexRewrite:
30+
pattern:
31+
regex: ^/redirect/\/*
32+
substitution: /

‎test/e2e/testdata/redirect-replaceprefixmatch-slash.yaml‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
apiVersion: gateway.networking.k8s.io/v1
2+
kind: HTTPRoute
3+
metadata:
4+
name: redirect-replaceprefixmatch-slash
5+
namespace: gateway-conformance-infra
6+
spec:
7+
parentRefs:
8+
- name: same-namespace
9+
rules:
10+
- matches:
11+
- path:
12+
type: PathPrefix
13+
value: /api/foo/
14+
filters:
15+
- requestRedirect:
16+
path:
17+
replacePrefixMatch: /
18+
type: ReplacePrefixMatch
19+
type: RequestRedirect
20+
- matches:
21+
- path:
22+
type: PathPrefix
23+
value: /api/bar
24+
filters:
25+
- requestRedirect:
26+
path:
27+
replacePrefixMatch: /
28+
type: ReplacePrefixMatch
29+
type: RequestRedirect

‎test/e2e/tests/redirect_replaceprefixmatch_slash.go‎

Lines changed: 125 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,125 @@
1+
// Copyright Envoy Gateway Authors
2+
// SPDX-License-Identifier: Apache-2.0
3+
// The full text of the Apache license is available in the LICENSE file at
4+
// the root of the repo.
5+
6+
//go:build e2e
7+
// +build e2e
8+
9+
package tests
10+
11+
import (
12+
"testing"
13+
14+
"k8s.io/apimachinery/pkg/types"
15+
"sigs.k8s.io/gateway-api/conformance/utils/http"
16+
"sigs.k8s.io/gateway-api/conformance/utils/kubernetes"
17+
"sigs.k8s.io/gateway-api/conformance/utils/suite"
18+
)
19+
20+
func init() {
21+
ConformanceTests = append(ConformanceTests, RedirectTrailingSlashTest)
22+
23+
}
24+
25+
// RedirectTrailingSlashTest tests that only one slash in the redirect URL
26+
// See https://github.com/envoyproxy/gateway/issues/2976
27+
var RedirectTrailingSlashTest = suite.ConformanceTest{
28+
ShortName: "RedirectTrailingSlash",
29+
Description: "Test that only one slash in the redirect URL",
30+
Manifests: []string{"testdata/redirect-replaceprefixmatch-slash.yaml"},
31+
32+
Test: func(t *testing.T, suite *suite.ConformanceTestSuite) {
33+
testCases := []struct {
34+
name string
35+
path string
36+
statusCode int
37+
expectedLocation string
38+
}{
39+
// Test cases for the HTTPRoute match /api/foo/
40+
{
41+
name: "match: /api/foo/, request: /api/foo/redirect",
42+
path: "/api/foo/redirect",
43+
statusCode: 302,
44+
expectedLocation: "/redirect",
45+
},
46+
{
47+
name: "match: /api/foo/, request: /api/foo/",
48+
path: "/api/foo/",
49+
statusCode: 302,
50+
expectedLocation: "/",
51+
},
52+
{
53+
name: "match: /api/foo/, request: /api/foo",
54+
path: "/api/foo",
55+
statusCode: 302,
56+
expectedLocation: "/",
57+
},
58+
{
59+
name: "match: /api/foo/, request: /api/foo-bar",
60+
path: "/api/foo-bar",
61+
statusCode: 404,
62+
},
63+
64+
// Test cases for the HTTPRoute match /api/bar
65+
{
66+
name: "match: /api/bar, request: /api/bar/redirect",
67+
path: "/api/bar/redirect",
68+
statusCode: 302,
69+
expectedLocation: "/redirect",
70+
},
71+
{
72+
name: "match: /api/bar, request: /api/bar/",
73+
path: "/api/bar/",
74+
statusCode: 302,
75+
expectedLocation: "/",
76+
},
77+
{
78+
name: "match: /api/bar, request: /api/bar",
79+
path: "/api/bar",
80+
statusCode: 302,
81+
expectedLocation: "/",
82+
},
83+
{
84+
name: "match: /api/bar, request: /api/bar-foo",
85+
path: "/api/bar-foo",
86+
statusCode: 404,
87+
},
88+
}
89+
90+
for _, testCase := range testCases {
91+
t.Run(testCase.name, func(t *testing.T) {
92+
ns := "gateway-conformance-infra"
93+
routeNN := types.NamespacedName{Name: "redirect-replaceprefixmatch-slash", Namespace: ns}
94+
gwNN := types.NamespacedName{Name: "same-namespace", Namespace: ns}
95+
gwAddr := kubernetes.GatewayAndHTTPRoutesMustBeAccepted(t, suite.Client, suite.TimeoutConfig, suite.ControllerName, kubernetes.NewGatewayRef(gwNN), routeNN)
96+
97+
expectedResponse := http.ExpectedResponse{
98+
Request: http.Request{
99+
Path: testCase.path,
100+
UnfollowRedirect: true,
101+
},
102+
Response: http.Response{
103+
StatusCode: testCase.statusCode,
104+
},
105+
Namespace: ns,
106+
}
107+
if testCase.expectedLocation != "" {
108+
expectedResponse.Response.Headers = map[string]string{
109+
"Location": testCase.expectedLocation,
110+
}
111+
}
112+
113+
req := http.MakeRequest(t, &expectedResponse, gwAddr, "HTTP", "http")
114+
cReq, cResp, err := suite.RoundTripper.CaptureRoundTrip(req)
115+
if err != nil {
116+
t.Errorf("failed to get expected response: %v", err)
117+
}
118+
119+
if err := http.CompareRequest(t, &req, cReq, cResp, expectedResponse); err != nil {
120+
t.Errorf("failed to compare request and response: %v", err)
121+
}
122+
})
123+
}
124+
},
125+
}

0 commit comments

Comments
 (0)