Enable HTTPRouteRewritePath test #2112

arkodg · 2023-10-29T23:32:41Z

Addresses this conformance test https://github.com/kubernetes-sigs/gateway-api/blob/8f1b718543caae927bb4f57dfb538aff1efc8159/conformance/tests/httproute-rewrite-path.yaml#L32 by using a regex match and replace instead of a prefix replace, since a prefix replace adds a double slash
tries to implement https://gateway-api.sigs.k8s.io/reference/spec/#gateway.networking.k8s.io/v1.HTTPPathModifier
reverts fix PathSeparatedPrefix & optimize the prefix ends with / logic. #1125 which tried to solve the same problem
trim the / suffix when it comes to prefix matches because Gateway API recommends ignoring the trailing / and envoy errors out when the trailing char is / for path separated prefix
enhance IR sorting function so preference is given to exact match over path prefix match, this is needed when exact match of /match and path prefix match of /match/ exists which need to reordered in envoy to make sure path prefix is not selected

Fixes: #2004

codecov · 2023-10-29T23:38:09Z

Codecov Report

Merging #2112 (aaa93e4) into main (58146ff) will decrease coverage by 0.14%.
Report is 5 commits behind head on main.
The diff coverage is 77.55%.

@@            Coverage Diff             @@
##             main    #2112      +/-   ##
==========================================
- Coverage   64.32%   64.18%   -0.14%     
==========================================
  Files         107      107              
  Lines       14623    14663      +40     
==========================================
+ Hits         9406     9412       +6     
- Misses       4654     4681      +27     
- Partials      563      570       +7

Files	Coverage Δ
internal/xds/translator/route.go	`92.51% <100.00%> (+0.37%)`	⬆️
internal/gatewayapi/sort.go	`74.62% <59.25%> (-25.38%)`	⬇️

... and 2 files with indirect coverage changes

arkodg · 2023-10-30T00:26:05Z

this is a workaround for envoyproxy/envoy#26055, would be great to fix this upstream
cc @cpakulski @zhaohuabing

arkodg · 2023-10-30T00:34:19Z

also relates to #1125
cc @qicz

Fixes: envoyproxy#2004 Signed-off-by: Arko Dasgupta <[email protected]>

Signed-off-by: Arko Dasgupta <[email protected]>

zirain

LGTM, @qicz PTAL

qicz · 2023-10-31T03:16:19Z

internal/xds/translator/route.go

@@ -101,14 +101,15 @@ func buildXdsRouteMatch(pathMatch *ir.StringMatch, headerMatches []*ir.StringMat
 				Path: *pathMatch.Exact,
 			}
 		} else if pathMatch.Prefix != nil {
-			// when the prefix ends with "/", use RouteMatch_Prefix
-			if strings.HasSuffix(*pathMatch.Prefix, "/") {
+			if *pathMatch.Prefix == "/" {


IMO, HasSuffix can cover Prefix = '/' and suffix with /. if we remove the trailing / can not cover the use case that suffix with / like /example/abc/. /example/abc and /example/abc/ are different case.

according to the gateway API a trailing slash in prefix can be ignored, ptal at
https://gateway-api.sigs.k8s.io/reference/spec/#gateway.networking.k8s.io%2fv1.PathMatchType

Matches based on a URL path prefix split by /. Matching is case sensitive and done on a path element by element basis. A path element refers to the list of labels in the path split by the / separator. When specified, a trailing / is ignored. For example, the paths /abc, /abc/, and /abc/def would all match the prefix /abc, but the path /abcd would not.

qicz · 2023-10-31T03:19:05Z

internal/xds/translator/route.go

 				outMatch.PathSpecifier = &routev3.RouteMatch_PathSeparatedPrefix{
-					PathSeparatedPrefix: *pathMatch.Prefix,


if the path prefix is not a suffix with /, the PathSpecifier use the PathSeparatedPrefix.

is this related to your above comment, if so ptal at #2112 (comment)

* Enable HTTPRouteRewritePath test Fixes: envoyproxy#2004 Signed-off-by: Arko Dasgupta <[email protected]> * fix prefix match Signed-off-by: Arko Dasgupta <[email protected]> * make testdata Signed-off-by: Arko Dasgupta <[email protected]> * fix path match Signed-off-by: Arko Dasgupta <[email protected]> * rm trailing / Signed-off-by: Arko Dasgupta <[email protected]> * sort on path match type Signed-off-by: Arko Dasgupta <[email protected]> * make testdata Signed-off-by: Arko Dasgupta <[email protected]> * temp var Signed-off-by: Arko Dasgupta <[email protected]> * fix url rewrite Signed-off-by: Arko Dasgupta <[email protected]> --------- Signed-off-by: Arko Dasgupta <[email protected]> (cherry picked from commit 2b3bc9f) Signed-off-by: Arko Dasgupta <[email protected]>

* Fix TestE2E/RateLimitBasedJwtClaims test (#2097) * Revert "Skip RateLimitBasedJwtClaimsTest test (#2096)" This reverts commit ef7a2a4. fix e2e test Signed-off-by: huabing zhao <[email protected]> * fix XValidation Signed-off-by: huabing zhao <[email protected]> --------- Signed-off-by: huabing zhao <[email protected]> (cherry picked from commit f301527) Signed-off-by: Arko Dasgupta <[email protected]> * add a newer PR reference in rc release docs (#2101) Signed-off-by: Arko Dasgupta <[email protected]> (cherry picked from commit fae8cd8) Signed-off-by: Arko Dasgupta <[email protected]> * fix: panic when using nil xdsRouteAction (#2104) Signed-off-by: bitliu <[email protected]> (cherry picked from commit 77445de) Signed-off-by: Arko Dasgupta <[email protected]> * remove cors, jwt, rl assignment from route translator (#2105) All the translations and assignments now happen in the policy translators Signed-off-by: Arko Dasgupta <[email protected]> (cherry picked from commit d65ab59) Signed-off-by: Arko Dasgupta <[email protected]> * fix: use lowercases of eg admin config fields (#2107) (cherry picked from commit ef5e450) Signed-off-by: Arko Dasgupta <[email protected]> * fix: testGatewayClassWithParamRef e2e test flaky (#2114) (cherry picked from commit 4047268) Signed-off-by: Arko Dasgupta <[email protected]> * build(deps): bump sigs.k8s.io/yaml from 1.3.0 to 1.4.0 (#2118) Bumps [sigs.k8s.io/yaml](https://github.com/kubernetes-sigs/yaml) from 1.3.0 to 1.4.0. - [Release notes](https://github.com/kubernetes-sigs/yaml/releases) - [Changelog](https://github.com/kubernetes-sigs/yaml/blob/master/RELEASE.md) - [Commits](kubernetes-sigs/yaml@v1.3.0...v1.4.0) --- updated-dependencies: - dependency-name: sigs.k8s.io/yaml dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> (cherry picked from commit bf93a4e) Signed-off-by: Arko Dasgupta <[email protected]> * build(deps): bump github.com/bufbuild/buf from 1.27.1 to 1.27.2 in /tools/src/buf (#2121) build(deps): bump github.com/bufbuild/buf in /tools/src/buf Bumps [github.com/bufbuild/buf](https://github.com/bufbuild/buf) from 1.27.1 to 1.27.2. - [Release notes](https://github.com/bufbuild/buf/releases) - [Changelog](https://github.com/bufbuild/buf/blob/main/CHANGELOG.md) - [Commits](bufbuild/buf@v1.27.1...v1.27.2) --- updated-dependencies: - dependency-name: github.com/bufbuild/buf dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> (cherry picked from commit 58146ff) Signed-off-by: Arko Dasgupta <[email protected]> * build(deps): bump github.com/go-logr/logr from 1.2.4 to 1.3.0 (#2117) Bumps [github.com/go-logr/logr](https://github.com/go-logr/logr) from 1.2.4 to 1.3.0. - [Release notes](https://github.com/go-logr/logr/releases) - [Changelog](https://github.com/go-logr/logr/blob/master/CHANGELOG.md) - [Commits](go-logr/logr@v1.2.4...v1.3.0) --- updated-dependencies: - dependency-name: github.com/go-logr/logr dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> (cherry picked from commit d1cbf5e) Signed-off-by: Arko Dasgupta <[email protected]> * build(deps): bump envoyproxy/toolshed from actions-v0.0.25 to 0.1.2 (#2116) Bumps [envoyproxy/toolshed](https://github.com/envoyproxy/toolshed) from actions-v0.0.25 to 0.1.2. This release includes the previously tagged commit. - [Release notes](https://github.com/envoyproxy/toolshed/releases) - [Commits](envoyproxy/toolshed@actions-v0.0.25...actions-v0.1.2) --- updated-dependencies: - dependency-name: envoyproxy/toolshed dependency-type: direct:production ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> (cherry picked from commit 7b6344d) Signed-off-by: Arko Dasgupta <[email protected]> * build(deps): bump actions/setup-node from 3 to 4 (#2115) Bumps [actions/setup-node](https://github.com/actions/setup-node) from 3 to 4. - [Release notes](https://github.com/actions/setup-node/releases) - [Commits](actions/setup-node@v3...v4) --- updated-dependencies: - dependency-name: actions/setup-node dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> (cherry picked from commit 5523099) Signed-off-by: Arko Dasgupta <[email protected]> * chore: add more EnvoyProxy cases (#2120) Signed-off-by: zirain <[email protected]> (cherry picked from commit 268a6ef) Signed-off-by: Arko Dasgupta <[email protected]> * build(deps): bump github.com/golangci/golangci-lint from 1.55.0 to 1.55.1 in /tools/src/golangci-lint (#2119) build(deps): bump github.com/golangci/golangci-lint Bumps [github.com/golangci/golangci-lint](https://github.com/golangci/golangci-lint) from 1.55.0 to 1.55.1. - [Release notes](https://github.com/golangci/golangci-lint/releases) - [Changelog](https://github.com/golangci/golangci-lint/blob/master/CHANGELOG.md) - [Commits](golangci/golangci-lint@v1.55.0...v1.55.1) --- updated-dependencies: - dependency-name: github.com/golangci/golangci-lint dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> (cherry picked from commit be86295) Signed-off-by: Arko Dasgupta <[email protected]> * feat: CEL Validation in BackendTrafficPolicy (#2110) * feat: CEL Validation in BackendTrafficPolicy Signed-off-by: slayer321 <[email protected]> * leastRequest with consistentHash nil test Signed-off-by: slayer321 <[email protected]> --------- Signed-off-by: slayer321 <[email protected]> (cherry picked from commit b45ae24) Signed-off-by: Arko Dasgupta <[email protected]> * e2e: eg controlplane metrics (#2106) * e2e: eg controlplane metrics Signed-off-by: zirain <[email protected]> * update Signed-off-by: zirain <[email protected]> * update Signed-off-by: zirain <[email protected]> * update name Signed-off-by: zirain <[email protected]> * rename Signed-off-by: zirain <[email protected]> * wait more Signed-off-by: zirain <[email protected]> * add comment Signed-off-by: zirain <[email protected]> * update Signed-off-by: zirain <[email protected]> --------- Signed-off-by: zirain <[email protected]> (cherry picked from commit 046a593) Signed-off-by: Arko Dasgupta <[email protected]> * chore: fix http2_protocol_options warning message (#2048) chore:fix http2_protocol_options warning message Signed-off-by: zhaonan <[email protected]> (cherry picked from commit 179d265) Signed-off-by: Arko Dasgupta <[email protected]> * fix Failed to update SecurityPolicy status (#2128) fix #2127 Signed-off-by: Huabing Zhao <[email protected]> (cherry picked from commit 57e1aec) Signed-off-by: Arko Dasgupta <[email protected]> * fix: add missing status equal for SecurityPolicy (#2134) (cherry picked from commit 71c09f2) Signed-off-by: Arko Dasgupta <[email protected]> * fix jwt doc (#2135) Signed-off-by: huabing zhao <[email protected]> (cherry picked from commit df1e209) Signed-off-by: Arko Dasgupta <[email protected]> * Bump Gateway API to v1.0.0 (#2142) (cherry picked from commit 3dd0ee7) Signed-off-by: Arko Dasgupta <[email protected]> * Enable HTTPRouteRewritePath test (#2112) * Enable HTTPRouteRewritePath test Fixes: #2004 Signed-off-by: Arko Dasgupta <[email protected]> * fix prefix match Signed-off-by: Arko Dasgupta <[email protected]> * make testdata Signed-off-by: Arko Dasgupta <[email protected]> * fix path match Signed-off-by: Arko Dasgupta <[email protected]> * rm trailing / Signed-off-by: Arko Dasgupta <[email protected]> * sort on path match type Signed-off-by: Arko Dasgupta <[email protected]> * make testdata Signed-off-by: Arko Dasgupta <[email protected]> * temp var Signed-off-by: Arko Dasgupta <[email protected]> * fix url rewrite Signed-off-by: Arko Dasgupta <[email protected]> --------- Signed-off-by: Arko Dasgupta <[email protected]> (cherry picked from commit 2b3bc9f) Signed-off-by: Arko Dasgupta <[email protected]> * helm: remove kube-rbac-proxy and update metrics service (#2108) (cherry picked from commit 0c5b054) Signed-off-by: Arko Dasgupta <[email protected]> * doc: user doc for CORS (#2137) * cors docs 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 d0dc987) Signed-off-by: Arko Dasgupta <[email protected]> * fix(ci): bump go version to 1.21 (#2144) * fix(ci): bump go version to 1.21 Signed-off-by: bitliu <[email protected]> * update Signed-off-by: bitliu <[email protected]> --------- Signed-off-by: bitliu <[email protected]> (cherry picked from commit 6e81fb3) Signed-off-by: Arko Dasgupta <[email protected]> * conformance: Enable HTTPRouteBackendProtocolH2C conformance test (#2136) * nit Signed-off-by: zirain <[email protected]> * build Cluster depends on route type and service appProtocol Signed-off-by: zirain <[email protected]> * enable HTTPRouteBackendProtocolH2C conformance test Signed-off-by: zirain <[email protected]> * lint Signed-off-by: zirain <[email protected]> * fix Signed-off-by: zirain <[email protected]> * address comment Signed-off-by: zirain <[email protected]> * move to package internal/ir Signed-off-by: zirain <[email protected]> --------- Signed-off-by: zirain <[email protected]> (cherry picked from commit 6d532c1) Signed-off-by: Arko Dasgupta <[email protected]> * Fix attachedRoutes computation (#2085) * Fix attachedRoutes computation * Fixes: #2077 * Fixes: #1916 Signed-off-by: Arko Dasgupta <[email protected]> * make testdata Signed-off-by: Arko Dasgupta <[email protected]> --------- Signed-off-by: Arko Dasgupta <[email protected]> (cherry picked from commit 93a12e7) Signed-off-by: Arko Dasgupta <[email protected]> * fix comments in loadbalancer api (#2145) Signed-off-by: zhaonan <[email protected]> (cherry picked from commit 77bcb3c) Signed-off-by: Arko Dasgupta <[email protected]> * fix(docs): incorrect quickstart links (#2146) Signed-off-by: bitliu <[email protected]> (cherry picked from commit defed57) Signed-off-by: Arko Dasgupta <[email protected]> * fix: null pointer when CORS maxAge is not specified (#2133) fix nullpointer Signed-off-by: huabing zhao <[email protected]> Signed-off-by: Huabing Zhao <[email protected]> (cherry picked from commit 35c4fea) Signed-off-by: Arko Dasgupta <[email protected]> --------- Signed-off-by: huabing zhao <[email protected]> Signed-off-by: Arko Dasgupta <[email protected]> Signed-off-by: bitliu <[email protected]> Signed-off-by: dependabot[bot] <[email protected]> Signed-off-by: zirain <[email protected]> Signed-off-by: slayer321 <[email protected]> Signed-off-by: zhaonan <[email protected]> Signed-off-by: Huabing Zhao <[email protected]> Co-authored-by: Huabing Zhao <[email protected]> Co-authored-by: Xunzhuo <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: zirain <[email protected]> Co-authored-by: Sachin Maurya <[email protected]> Co-authored-by: tmsnan <[email protected]>

arkodg requested a review from a team as a code owner October 29, 2023 23:32

arkodg marked this pull request as draft October 29, 2023 23:32

arkodg force-pushed the enable-rewrite-test branch from 4836348 to 96475fe Compare October 30, 2023 00:16

arkodg marked this pull request as ready for review October 30, 2023 19:41

arkodg added 9 commits October 30, 2023 13:49

Enable HTTPRouteRewritePath test

a0c286d

Fixes: envoyproxy#2004 Signed-off-by: Arko Dasgupta <[email protected]>

fix prefix match

2e01a87

Signed-off-by: Arko Dasgupta <[email protected]>

make testdata

e6e7a9e

Signed-off-by: Arko Dasgupta <[email protected]>

fix path match

713120a

Signed-off-by: Arko Dasgupta <[email protected]>

rm trailing /

71c21a3

Signed-off-by: Arko Dasgupta <[email protected]>

sort on path match type

9acd678

Signed-off-by: Arko Dasgupta <[email protected]>

make testdata

177bdc6

Signed-off-by: Arko Dasgupta <[email protected]>

temp var

1c26b15

Signed-off-by: Arko Dasgupta <[email protected]>

fix url rewrite

aaa93e4

Signed-off-by: Arko Dasgupta <[email protected]>

arkodg force-pushed the enable-rewrite-test branch from 5680ab4 to aaa93e4 Compare October 30, 2023 20:49

arkodg requested review from a team, kflynn, chauhanshubham and Xunzhuo and removed request for a team October 30, 2023 21:09

arkodg added this to the 0.6.0 milestone Oct 30, 2023

zirain reviewed Oct 31, 2023

View reviewed changes

Xunzhuo approved these changes Oct 31, 2023

View reviewed changes

qicz reviewed Oct 31, 2023

View reviewed changes

arkodg requested review from qicz and zirain October 31, 2023 05:10

qicz approved these changes Nov 1, 2023

View reviewed changes

zirain approved these changes Nov 1, 2023

View reviewed changes

zirain merged commit 2b3bc9f into envoyproxy:main Nov 1, 2023
21 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable HTTPRouteRewritePath test #2112

Enable HTTPRouteRewritePath test #2112

arkodg commented Oct 29, 2023 •

edited

Loading

codecov bot commented Oct 29, 2023 •

edited

Loading

arkodg commented Oct 30, 2023

arkodg commented Oct 30, 2023 •

edited

Loading

zirain left a comment

qicz Oct 31, 2023 •

edited

Loading

arkodg Oct 31, 2023

qicz Oct 31, 2023 •

edited

Loading

arkodg Oct 31, 2023

		outMatch.PathSpecifier = &routev3.RouteMatch_PathSeparatedPrefix{
		PathSeparatedPrefix: *pathMatch.Prefix,

Enable HTTPRouteRewritePath test #2112

Enable HTTPRouteRewritePath test #2112

Conversation

arkodg commented Oct 29, 2023 • edited Loading

codecov bot commented Oct 29, 2023 • edited Loading

Codecov Report

arkodg commented Oct 30, 2023

arkodg commented Oct 30, 2023 • edited Loading

zirain left a comment

Choose a reason for hiding this comment

qicz Oct 31, 2023 • edited Loading

Choose a reason for hiding this comment

arkodg Oct 31, 2023

Choose a reason for hiding this comment

qicz Oct 31, 2023 • edited Loading

Choose a reason for hiding this comment

arkodg Oct 31, 2023

Choose a reason for hiding this comment

arkodg commented Oct 29, 2023 •

edited

Loading

codecov bot commented Oct 29, 2023 •

edited

Loading

arkodg commented Oct 30, 2023 •

edited

Loading

qicz Oct 31, 2023 •

edited

Loading

qicz Oct 31, 2023 •

edited

Loading