-
Notifications
You must be signed in to change notification settings - Fork 347
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enable HTTPRouteRewritePath test #2112
Changes from all commits
a0c286d
2e01a87
e6e7a9e
713120a
71c21a3
9acd678
177bdc6
1c26b15
aaa93e4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if the path prefix is not a suffix with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is this related to your above comment, if so ptal at #2112 (comment) |
||
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 | ||
} | ||
} | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO,
HasSuffix
can coverPrefix = '/'
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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