Enable HTTPRouteRewritePath test (envoyproxy#2112)

* Enable HTTPRouteRewritePath test Fixes: envoyproxy#2004 Signed-off-by: Arko Dasgupta <arko@tetrate.io> * fix prefix match Signed-off-by: Arko Dasgupta <arko@tetrate.io> * make testdata Signed-off-by: Arko Dasgupta <arko@tetrate.io> * fix path match Signed-off-by: Arko Dasgupta <arko@tetrate.io> * rm trailing / Signed-off-by: Arko Dasgupta <arko@tetrate.io> * sort on path match type Signed-off-by: Arko Dasgupta <arko@tetrate.io> * make testdata Signed-off-by: Arko Dasgupta <arko@tetrate.io> * temp var Signed-off-by: Arko Dasgupta <arko@tetrate.io> * fix url rewrite Signed-off-by: Arko Dasgupta <arko@tetrate.io> --------- Signed-off-by: Arko Dasgupta <arko@tetrate.io> (cherry picked from commit 2b3bc9f) Signed-off-by: Arko Dasgupta <arko@tetrate.io>
arkodg · Nov 1, 2023 · ea8fe92 · ea8fe92
1 parent 87bb834
commit ea8fe92
Show file tree

Hide file tree

Showing 8 changed files with 80 additions and 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
@@ -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
@@ -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
@@ -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
@@ -137,26 +137,26 @@ 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
               port: 8080
             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,23 +177,23 @@ 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
               port: 8080
             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
@@ -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/...al/xds/translator/testdata/out/xds-ir/http-route-rewrite-root-path-url-prefix.routes.yaml b/...al/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
@@ -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
@@ -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,