Fix: double slashes in redirect URL by zhaohuabing · Pull Request #2998 · envoyproxy/gateway

zhaohuabing · 2024-03-23T05:53:06Z

This PR fix double splashes in the redirect URL

When an HTTPRoute's match is a path prefix and has a redirect filter with a replacePrefixMatch "/",
A redirect routeaction with regex_rewrite is generated to match both the prefix with and without trailing "/" to avoid double "/" in the redirect URL.

For example, given the following HTTPRoute:

 - matches:
   - path:
       type: PathPrefix
       value: /api/foo/
   filters:
   - requestRedirect:
       path:
         replacePrefixMatch: /
         type: ReplacePrefixMatch
     type: RequestRedirect

The following route will be created:

              routes:
              - match:
                  path_separated_prefix: "/api/foo"
                redirect:
                  regex_rewrite:       
                    pattern:
                      regex: "^\/api\/foo\/*"
                    substitution: /

See #2976 for more details.

Fix: #2976

codecov · 2024-03-23T05:56:20Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 64.58%. Comparing base (31b4058) to head (b2998af).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2998      +/-   ##
==========================================
+ Coverage   64.56%   64.58%   +0.02%     
==========================================
  Files         121      121              
  Lines       21369    21382      +13     
==========================================
+ Hits        13796    13809      +13     
  Misses       6707     6707              
  Partials      866      866

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: huabing zhao <[email protected]>

arkodg · 2024-03-26T11:09:39Z

hey @zhaohuabing I found some code under URLRewrite,

gateway/internal/xds/translator/route.go

Line 332 in 485edbe

// Circumvent the case of "//" when the replace string is "/"

, can we reuse this ?

internal/xds/translator/route.go

Signed-off-by: huabing zhao <[email protected]>

arkodg · 2024-03-28T19:48:12Z

internal/xds/translator/route.go

+func useRegexRewriteForPrefixMatchReplace(pathMatch *ir.StringMatch, prefixMatchReplace string) bool {
+	return pathMatch != nil &&
+		pathMatch.Prefix != nil &&
+		*pathMatch.Prefix != "/" &&


this line is new ?

It's an edge case. We don't need to use prefixMatchReplace if the patchMatch prefix is "/". But regex still work anyway. I removed this line.

virtual_hosts: - name: service domains: - "*" routes: - match: prefix: "/" redirect: prefixRewrite: "/"

arkodg · 2024-03-28T19:49:47Z

looks good, thanks for reusing the existing code !
minor comment, else LGTM

Signed-off-by: huabing zhao <[email protected]>

arkodg

LGTM thanks for fixing this quirk !
and also adding e2es 🚀

* 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]> (cherry picked from commit ceb697f) Signed-off-by: Arko Dasgupta <[email protected]>

* Run certgen when upgrading (#2934) run certgen when upgrading Signed-off-by: huabing zhao <[email protected]> (cherry picked from commit 62ecf15) Signed-off-by: Arko Dasgupta <[email protected]> * Fix: nil secret in resourceversiontable (#2982) * fix nil secret in resourceversiontable Signed-off-by: huabing zhao <[email protected]> * check secrets in the xds result Signed-off-by: huabing zhao <[email protected]> --------- Signed-off-by: huabing zhao <[email protected]> (cherry picked from commit e880439) Signed-off-by: Arko Dasgupta <[email protected]> * fix: add missing http filters to the http filter chain (#2970) * fix: add missing http filters to the http filter chain Signed-off-by: huabing zhao <[email protected]> * refactor Signed-off-by: huabing zhao <[email protected]> * fix lint Signed-off-by: huabing zhao <[email protected]> * add comments Signed-off-by: huabing zhao <[email protected]> * remove refactor Signed-off-by: huabing zhao <[email protected]> * remove refactor Signed-off-by: huabing zhao <[email protected]> * fix gen Signed-off-by: huabing zhao <[email protected]> * fix lint Signed-off-by: Huabing Zhao <[email protected]> --------- Signed-off-by: huabing zhao <[email protected]> Signed-off-by: Huabing Zhao <[email protected]> (cherry picked from commit f699edf) Signed-off-by: Arko Dasgupta <[email protected]> * fix: allow websockets in url rewrite (#3022) allow websockets in url rewrite Signed-off-by: Jesse Haka <[email protected]> Co-authored-by: zirain <[email protected]> (cherry picked from commit 3d51933) Signed-off-by: Arko Dasgupta <[email protected]> * Set host for http health checker explicitly to avoid using the cluster name as host header for http health checking request. (#3057) * Set host for http health checker explictly to avoid using the cluster name as host header for http health checking request Signed-off-by: lemonlinger <[email protected]> * fix broken tests Signed-off-by: lemonlinger <[email protected]> * fix health-check test case in xds translation Signed-off-by: lemonlinger <[email protected]> * Simplify code and concise comments Signed-off-by: lemonlinger <[email protected]> --------- Signed-off-by: lemonlinger <[email protected]> (cherry picked from commit 8f450a9) Signed-off-by: Arko Dasgupta <[email protected]> * fix: do not create infra resources when missing translated listeners (#3043) * fix: do not create infra resources when missing translated listeners Signed-off-by: Karol Szwaj <[email protected]> * remove empty line Signed-off-by: Karol Szwaj <[email protected]> * skip infra creation on empty listeners and log it Signed-off-by: Karol Szwaj <[email protected]> --------- Signed-off-by: Karol Szwaj <[email protected]> (cherry picked from commit 36d7141) Signed-off-by: Arko Dasgupta <[email protected]> * 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]> (cherry picked from commit ceb697f) Signed-off-by: Arko Dasgupta <[email protected]> * fix: Allow Policy to attach to multiple http listeners (#2967) * Fixing the clienttrafficpolicy validation. Signed-off-by: Lior Okman <[email protected]> * Make SecurityPolicy validate correctly. Signed-off-by: Lior Okman <[email protected]> * Reverted the SecurityPolicy validation - handled differently via another feature. Signed-off-by: Lior Okman <[email protected]> * Updated the tests to reflect that this validation isn't required for SecurityPolicy Signed-off-by: Lior Okman <[email protected]> * Added some comments to explain the validation being performed. Signed-off-by: Lior Okman <[email protected]> * Updated the error message as requested in the review. Signed-off-by: Lior Okman <[email protected]> --------- Signed-off-by: Lior Okman <[email protected]> (cherry picked from commit f9409e4) Signed-off-by: Arko Dasgupta <[email protected]> * fix: set path prefix for http ext auth service (#3018) Signed-off-by: huabing zhao <[email protected]> (cherry picked from commit 2882b7c) Signed-off-by: Arko Dasgupta <[email protected]> * Change route sorting order to Exact > RegularExpression > PathPrefix (#2579) * Change route sorting order to Exact > RegularExpression > PathPrefix kubernetes-sigs/gateway-api#1770 kubernetes-sigs/gateway-api#1855 Signed-off-by: Stéphane Cottin <[email protected]> (cherry picked from commit 11f56fd) Signed-off-by: Arko Dasgupta <[email protected]> * fix: infraIR duplicate port translation for merged gateways (#3061) * fix: duplicate port translation for merged gateways Signed-off-by: Karol Szwaj <[email protected]> * refactor to map Signed-off-by: Karol Szwaj <[email protected]> * rename map Signed-off-by: Karol Szwaj <[email protected]> * add seperate testcase Signed-off-by: Karol Szwaj <[email protected]> --------- Signed-off-by: Karol Szwaj <[email protected]> (cherry picked from commit 29946b0) Signed-off-by: Arko Dasgupta <[email protected]> * translator: set SpawnUpstreamSpan to true (#3102) * translator: set SpawnUpstreamSpan to true Signed-off-by: zirain <[email protected]> * update Signed-off-by: zirain <[email protected]> --------- Signed-off-by: zirain <[email protected]> (cherry picked from commit 635ebfc) Signed-off-by: Arko Dasgupta <[email protected]> * fix: rate limit doesn't work with two(and more) listeners (#3085) * fix: rate limit doesn't work with two listeners Signed-off-by: huabing zhao <[email protected]> * add e2e test for rate limit on multiple listeners Signed-off-by: huabing zhao <[email protected]> * address comments Signed-off-by: huabing zhao <[email protected]> --------- Signed-off-by: huabing zhao <[email protected]> Co-authored-by: Xunzhuo <[email protected]> (cherry picked from commit a5bedbc) Signed-off-by: Arko Dasgupta <[email protected]> * rerun make testdata Signed-off-by: Arko Dasgupta <[email protected]> --------- Signed-off-by: huabing zhao <[email protected]> Signed-off-by: Arko Dasgupta <[email protected]> Signed-off-by: Huabing Zhao <[email protected]> Signed-off-by: Jesse Haka <[email protected]> Signed-off-by: lemonlinger <[email protected]> Signed-off-by: Karol Szwaj <[email protected]> Signed-off-by: Lior Okman <[email protected]> Signed-off-by: Stéphane Cottin <[email protected]> Signed-off-by: zirain <[email protected]> Co-authored-by: Huabing Zhao <[email protected]> Co-authored-by: Jesse Haka <[email protected]> Co-authored-by: zirain <[email protected]> Co-authored-by: Meng <[email protected]> Co-authored-by: Karol Szwaj <[email protected]> Co-authored-by: Lior Okman <[email protected]> Co-authored-by: vixns <[email protected]> Co-authored-by: Xunzhuo <[email protected]>

zhaohuabing requested a review from a team as a code owner March 23, 2024 05:53

zhaohuabing force-pushed the fix-redirect-trailling-slash branch from 7799df0 to a11cd5e Compare March 23, 2024 06:02

fix: double trailing splashs in redirect URL

c0b619c

Signed-off-by: huabing zhao <[email protected]>

zhaohuabing force-pushed the fix-redirect-trailling-slash branch from a11cd5e to c0b619c Compare March 23, 2024 06:04

zhaohuabing changed the title ~~Fix: double trailing splashs in redirect URL~~ Fix: double splashes in redirect URL Mar 23, 2024

zhaohuabing added 5 commits March 23, 2024 14:17

add e2e tests

9329419

Signed-off-by: huabing zhao <[email protected]>

fix lint

3e89d1d

Signed-off-by: huabing zhao <[email protected]>

fix test

be5ad08

Signed-off-by: huabing zhao <[email protected]>

fix test

2c370fc

Signed-off-by: huabing zhao <[email protected]>

fix test

76cc1c9

Signed-off-by: huabing zhao <[email protected]>

zhaohuabing force-pushed the fix-redirect-trailling-slash branch from 982eecb to 76cc1c9 Compare March 23, 2024 12:24

zhaohuabing added 2 commits March 24, 2024 09:32

fix test

3cb5340

Signed-off-by: huabing zhao <[email protected]>

add e2e tests

008c43d

Signed-off-by: huabing zhao <[email protected]>

zhaohuabing requested a review from arkodg March 24, 2024 01:45

zhaohuabing added 4 commits March 24, 2024 10:47

fix test

498d231

Signed-off-by: huabing zhao <[email protected]>

Merge branch 'main' into fix-redirect-trailling-slash

ccd717d

Merge branch 'main' into fix-redirect-trailling-slash

ac7a633

Merge branch 'main' into fix-redirect-trailling-slash

3994460

zirain reviewed Mar 26, 2024

View reviewed changes

internal/xds/translator/route.go Outdated Show resolved Hide resolved

zhaohuabing marked this pull request as draft March 27, 2024 07:00

zhaohuabing force-pushed the fix-redirect-trailling-slash branch from 24f7643 to 3994460 Compare March 27, 2024 07:06

zhaohuabing added 5 commits March 27, 2024 15:07

Merge branch 'main' into fix-redirect-trailling-slash

04fef45

revert

5f957bb

Signed-off-by: huabing zhao <[email protected]>

use regex rewrite to generate the redirect url

cb7a9d3

Signed-off-by: huabing zhao <[email protected]>

use regex rewrite to generate the redirect url

e0172b1

Signed-off-by: huabing zhao <[email protected]>

use regex rewrite to generate the redirect url

30d1386

Signed-off-by: huabing zhao <[email protected]>

zhaohuabing marked this pull request as ready for review March 27, 2024 10:14

zhaohuabing marked this pull request as draft March 27, 2024 10:27

zhaohuabing added 2 commits March 27, 2024 21:35

remove comments

a9958c0

Signed-off-by: huabing zhao <[email protected]>

Merge branch 'main' into fix-redirect-trailling-slash

1296419

zhaohuabing marked this pull request as ready for review March 27, 2024 13:37

zhaohuabing requested a review from zirain March 27, 2024 13:37

zhaohuabing added 2 commits March 27, 2024 21:49

extract method

2c7c5d1

Signed-off-by: huabing zhao <[email protected]>

Merge branch 'main' into fix-redirect-trailling-slash

76acf34

arkodg reviewed Mar 28, 2024

View reviewed changes

zhaohuabing added 2 commits March 29, 2024 09:12

address comments

4dcb883

Signed-off-by: huabing zhao <[email protected]>

Merge branch 'main' into fix-redirect-trailling-slash

b2998af

zhaohuabing requested a review from arkodg March 29, 2024 09:52

arkodg approved these changes Mar 29, 2024

View reviewed changes

arkodg changed the title ~~Fix: double splashes in redirect URL~~ Fix: double slashes in redirect URL Mar 29, 2024

arkodg requested review from a team March 29, 2024 09:58

zirain approved these changes Mar 29, 2024

View reviewed changes

zirain merged commit ceb697f into envoyproxy:main Mar 29, 2024

arkodg added the cherrypick/release-v1.0 label Apr 4, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: double slashes in redirect URL#2998

Fix: double slashes in redirect URL#2998
zirain merged 23 commits intoenvoyproxy:mainfrom
zhaohuabing:fix-redirect-trailling-slash

zhaohuabing commented Mar 23, 2024 •

edited

Loading

Uh oh!

codecov bot commented Mar 23, 2024 •

edited

Loading

Uh oh!

arkodg commented Mar 26, 2024

Uh oh!

Uh oh!

arkodg Mar 28, 2024

Uh oh!

zhaohuabing Mar 29, 2024 •

edited

Loading

Uh oh!

arkodg commented Mar 28, 2024

Uh oh!

arkodg left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

zhaohuabing commented Mar 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Mar 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

arkodg commented Mar 26, 2024

Uh oh!

Uh oh!

arkodg Mar 28, 2024

Choose a reason for hiding this comment

Uh oh!

zhaohuabing Mar 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arkodg commented Mar 28, 2024

Uh oh!

arkodg left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

zhaohuabing commented Mar 23, 2024 •

edited

Loading

codecov bot commented Mar 23, 2024 •

edited

Loading

zhaohuabing Mar 29, 2024 •

edited

Loading