From ea8fe92d7aa9517f6d02aff7c2fd2d4cfbcf2d94 Mon Sep 17 00:00:00 2001 From: Arko Dasgupta Date: Tue, 31 Oct 2023 17:59:15 -0700 Subject: [PATCH] Enable HTTPRouteRewritePath test (#2112) * Enable HTTPRouteRewritePath test Fixes: https://github.com/envoyproxy/gateway/issues/2004 Signed-off-by: Arko Dasgupta * fix prefix match Signed-off-by: Arko Dasgupta * make testdata Signed-off-by: Arko Dasgupta * fix path match Signed-off-by: Arko Dasgupta * rm trailing / Signed-off-by: Arko Dasgupta * sort on path match type Signed-off-by: Arko Dasgupta * make testdata Signed-off-by: Arko Dasgupta * temp var Signed-off-by: Arko Dasgupta * fix url rewrite Signed-off-by: Arko Dasgupta --------- Signed-off-by: Arko Dasgupta (cherry picked from commit 2b3bc9f37d289b6bf659d5a76677530371cc98e4) Signed-off-by: Arko Dasgupta --- .../translate/out/multiple-xds.route.json | 2 +- internal/gatewayapi/sort.go | 41 +++++++++++++++++-- .../grpcroute-with-service-match.out.yaml | 8 ++-- ...h-single-rule-with-multiple-rules.out.yaml | 20 ++++----- internal/xds/translator/route.go | 29 +++++++++---- ...e-rewrite-root-path-url-prefix.routes.yaml | 7 +++- test/conformance/conformance_test.go | 1 - .../experimental_conformance_test.go | 1 - 8 files changed, 80 insertions(+), 29 deletions(-) diff --git a/internal/cmd/egctl/testdata/translate/out/multiple-xds.route.json b/internal/cmd/egctl/testdata/translate/out/multiple-xds.route.json index 748a5d4f37e..c2d43139edd 100644 --- a/internal/cmd/egctl/testdata/translate/out/multiple-xds.route.json +++ b/internal/cmd/egctl/testdata/translate/out/multiple-xds.route.json @@ -48,7 +48,7 @@ "routes": [ { "match": { - "prefix": "/v2/" + "pathSeparatedPrefix": "/v2" }, "name": "httproute/default/backend/rule/0/match/0/www_example2_com", "route": { diff --git a/internal/gatewayapi/sort.go b/internal/gatewayapi/sort.go index 589d564cf3f..00a5fc6389d 100644 --- a/internal/gatewayapi/sort.go +++ b/internal/gatewayapi/sort.go @@ -16,7 +16,42 @@ type XdsIRRoutes []*ir.HTTPRoute func (x XdsIRRoutes) Len() int { return len(x) } func (x XdsIRRoutes) Swap(i, j int) { x[i], x[j] = x[j], x[i] } func (x XdsIRRoutes) Less(i, j int) bool { - // 1. Sort based on characters in a matching path. + + // 1. Sort based on path match type + // Exact > PathPrefix > RegularExpression + if x[i].PathMatch != nil && x[i].PathMatch.Exact != nil { + if x[j].PathMatch != nil { + if x[j].PathMatch.Prefix != nil { + return false + } + if x[j].PathMatch.SafeRegex != nil { + return false + } + } + } + if x[i].PathMatch != nil && x[i].PathMatch.Prefix != nil { + if x[j].PathMatch != nil { + if x[j].PathMatch.Exact != nil { + return true + } + if x[j].PathMatch.SafeRegex != nil { + return false + } + } + } + if x[i].PathMatch != nil && x[i].PathMatch.SafeRegex != nil { + if x[j].PathMatch != nil { + if x[j].PathMatch.Exact != nil { + return true + } + if x[j].PathMatch.Prefix != nil { + return true + } + } + } + // Equal case + + // 2. Sort based on characters in a matching path. pCountI := pathMatchCount(x[i].PathMatch) pCountJ := pathMatchCount(x[j].PathMatch) if pCountI < pCountJ { @@ -27,7 +62,7 @@ func (x XdsIRRoutes) Less(i, j int) bool { } // Equal case - // 2. Sort based on the number of Header matches. + // 3. Sort based on the number of Header matches. hCountI := len(x[i].HeaderMatches) hCountJ := len(x[j].HeaderMatches) if hCountI < hCountJ { @@ -38,7 +73,7 @@ func (x XdsIRRoutes) Less(i, j int) bool { } // Equal case - // 3. Sort based on the number of Query param matches. + // 4. Sort based on the number of Query param matches. qCountI := len(x[i].QueryParamMatches) qCountJ := len(x[j].QueryParamMatches) return qCountI < qCountJ diff --git a/internal/gatewayapi/testdata/grpcroute-with-service-match.out.yaml b/internal/gatewayapi/testdata/grpcroute-with-service-match.out.yaml index 4406f205c53..55e3230098b 100644 --- a/internal/gatewayapi/testdata/grpcroute-with-service-match.out.yaml +++ b/internal/gatewayapi/testdata/grpcroute-with-service-match.out.yaml @@ -119,11 +119,11 @@ xdsIR: port: 8080 weight: 1 hostname: '*' - name: grpcroute/default/grpcroute-1/rule/0/match/1/* + name: grpcroute/default/grpcroute-1/rule/0/match/0/* pathMatch: distinct: false name: "" - safeRegex: /com.[A-Z]+/[A-Za-z_][A-Za-z_0-9]* + prefix: /com.ExampleExact - backendWeights: invalid: 0 valid: 0 @@ -135,8 +135,8 @@ xdsIR: port: 8080 weight: 1 hostname: '*' - name: grpcroute/default/grpcroute-1/rule/0/match/0/* + name: grpcroute/default/grpcroute-1/rule/0/match/1/* pathMatch: distinct: false name: "" - prefix: /com.ExampleExact + safeRegex: /com.[A-Z]+/[A-Za-z_][A-Za-z_0-9]* diff --git a/internal/gatewayapi/testdata/httproute-with-single-rule-with-multiple-rules.out.yaml b/internal/gatewayapi/testdata/httproute-with-single-rule-with-multiple-rules.out.yaml index 33d4418d46a..c5e10904d8e 100644 --- a/internal/gatewayapi/testdata/httproute-with-single-rule-with-multiple-rules.out.yaml +++ b/internal/gatewayapi/testdata/httproute-with-single-rule-with-multiple-rules.out.yaml @@ -137,7 +137,7 @@ xdsIR: invalid: 0 valid: 0 destination: - name: httproute/default/httproute-1/rule/2 + name: httproute/default/httproute-1/rule/0 settings: - endpoints: - host: 7.7.7.7 @@ -145,18 +145,18 @@ xdsIR: weight: 1 headerMatches: - distinct: false + exact: exact name: Header-1 - safeRegex: '*regex*' hostname: '*' - name: httproute/default/httproute-1/rule/2/match/0/* + name: httproute/default/httproute-1/rule/0/match/0/* pathMatch: distinct: false + exact: /exact name: "" - safeRegex: '*regex*' queryParamMatches: - distinct: false + exact: exact name: QueryParam-1 - safeRegex: '*regex*' - backendWeights: invalid: 0 valid: 0 @@ -177,7 +177,7 @@ xdsIR: invalid: 0 valid: 0 destination: - name: httproute/default/httproute-1/rule/0 + name: httproute/default/httproute-1/rule/2 settings: - endpoints: - host: 7.7.7.7 @@ -185,15 +185,15 @@ xdsIR: weight: 1 headerMatches: - distinct: false - exact: exact name: Header-1 + safeRegex: '*regex*' hostname: '*' - name: httproute/default/httproute-1/rule/0/match/0/* + name: httproute/default/httproute-1/rule/2/match/0/* pathMatch: distinct: false - exact: /exact name: "" + safeRegex: '*regex*' queryParamMatches: - distinct: false - exact: exact name: QueryParam-1 + safeRegex: '*regex*' diff --git a/internal/xds/translator/route.go b/internal/xds/translator/route.go index e9546e22772..e69a953f6bc 100644 --- a/internal/xds/translator/route.go +++ b/internal/xds/translator/route.go @@ -43,7 +43,7 @@ func buildXdsRoute(httpRoute *ir.HTTPRoute) *routev3.Route { case httpRoute.Redirect != nil: router.Action = &routev3.Route_Redirect{Redirect: buildXdsRedirectAction(httpRoute.Redirect)} case httpRoute.URLRewrite != nil: - routeAction := buildXdsURLRewriteAction(httpRoute.Destination.Name, httpRoute.URLRewrite) + routeAction := buildXdsURLRewriteAction(httpRoute.Destination.Name, httpRoute.URLRewrite, httpRoute.PathMatch) if httpRoute.Mirrors != nil { routeAction.RequestMirrorPolicies = buildXdsRequestMirrorPolicies(httpRoute.Mirrors) } @@ -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 == "/" { outMatch.PathSpecifier = &routev3.RouteMatch_Prefix{ - Prefix: *pathMatch.Prefix, + Prefix: "/", } } else { + // Remove trailing / + trimmedPrefix := strings.TrimSuffix(*pathMatch.Prefix, "/") outMatch.PathSpecifier = &routev3.RouteMatch_PathSeparatedPrefix{ - PathSeparatedPrefix: *pathMatch.Prefix, + PathSeparatedPrefix: trimmedPrefix, } } } else if pathMatch.SafeRegex != nil { @@ -252,7 +253,7 @@ func buildXdsRedirectAction(redirection *ir.Redirect) *routev3.RedirectAction { return routeAction } -func buildXdsURLRewriteAction(destName string, urlRewrite *ir.URLRewrite) *routev3.RouteAction { +func buildXdsURLRewriteAction(destName string, urlRewrite *ir.URLRewrite, pathMatch *ir.StringMatch) *routev3.RouteAction { routeAction := &routev3.RouteAction{ ClusterSpecifier: &routev3.RouteAction_Cluster{ Cluster: destName, @@ -268,7 +269,21 @@ func buildXdsURLRewriteAction(destName string, urlRewrite *ir.URLRewrite) *route Substitution: *urlRewrite.Path.FullReplace, } } else if urlRewrite.Path.PrefixMatchReplace != nil { - routeAction.PrefixRewrite = *urlRewrite.Path.PrefixMatchReplace + // Circumvent the case of "//" when the replace string is "/" + // An empty replace string does not seem to solve the issue so we are using + // a regex match and replace instead + // Remove this workaround once https://github.com/envoyproxy/envoy/issues/26055 is fixed + if pathMatch != nil && pathMatch.Prefix != nil && + (*urlRewrite.Path.PrefixMatchReplace == "" || *urlRewrite.Path.PrefixMatchReplace == "/") { + routeAction.RegexRewrite = &matcherv3.RegexMatchAndSubstitute{ + Pattern: &matcherv3.RegexMatcher{ + Regex: "^" + *pathMatch.Prefix + `\/*`, + }, + Substitution: "/", + } + } else { + routeAction.PrefixRewrite = *urlRewrite.Path.PrefixMatchReplace + } } } diff --git a/internal/xds/translator/testdata/out/xds-ir/http-route-rewrite-root-path-url-prefix.routes.yaml b/internal/xds/translator/testdata/out/xds-ir/http-route-rewrite-root-path-url-prefix.routes.yaml index 1604805cc9b..2bf01099ad2 100644 --- a/internal/xds/translator/testdata/out/xds-ir/http-route-rewrite-root-path-url-prefix.routes.yaml +++ b/internal/xds/translator/testdata/out/xds-ir/http-route-rewrite-root-path-url-prefix.routes.yaml @@ -10,8 +10,11 @@ - name: :authority stringMatch: exact: gateway.envoyproxy.io - prefix: /origin/ + pathSeparatedPrefix: /origin name: rewrite-route route: cluster: rewrite-route-dest - prefixRewrite: / + regexRewrite: + pattern: + regex: ^/origin/\/* + substitution: / diff --git a/test/conformance/conformance_test.go b/test/conformance/conformance_test.go index eeb8898ffc7..0e08a5a1ad1 100644 --- a/test/conformance/conformance_test.go +++ b/test/conformance/conformance_test.go @@ -48,7 +48,6 @@ func TestGatewayAPIConformance(t *testing.T) { CleanupBaseResources: *flags.CleanupBaseResources, SupportedFeatures: suite.AllFeatures, SkipTests: []string{ - tests.HTTPRouteRewritePath.ShortName, tests.GatewayStaticAddresses.ShortName, tests.GatewayWithAttachedRoutes.ShortName, tests.HTTPRouteBackendProtocolH2C.ShortName, diff --git a/test/conformance/experimental_conformance_test.go b/test/conformance/experimental_conformance_test.go index fd4a5620973..a6a308cbbce 100644 --- a/test/conformance/experimental_conformance_test.go +++ b/test/conformance/experimental_conformance_test.go @@ -97,7 +97,6 @@ func experimentalConformance(t *testing.T) { CleanupBaseResources: *flags.CleanupBaseResources, SupportedFeatures: suite.AllFeatures, SkipTests: []string{ - tests.HTTPRouteRewritePath.ShortName, tests.GatewayStaticAddresses.ShortName, tests.GatewayWithAttachedRoutes.ShortName, tests.HTTPRouteBackendProtocolH2C.ShortName,