Martin4R commented on code in PR #2897:
URL:
https://github.com/apache/apisix-ingress-controller/pull/2897#discussion_r4183980409
##########
internal/adc/translator/httproute.go:
##########
@@ -798,46 +851,50 @@ func (t *Translator) TranslateHTTPRoute(tctx
*provider.TranslateContext, httpRou
}
func (t *Translator) translateGatewayHTTPRouteMatch(match
*gatewayv1.HTTPRouteMatch) (*adctypes.Route, error) {
- route := &adctypes.Route{}
+ // APISIX Admin API requires uris to be a non-null array. Use "/*" as a
+ // catch-all so APISIX accepts the route; the vars entry below performs
the
+ // actual path filtering.
+ route := &adctypes.Route{
+ Uris: []string{"/*"},
+ }
if match.Path != nil {
switch *match.Path.Type {
case gatewayv1.PathMatchExact:
- route.Uris = []string{*match.Path.Value}
+ route.Vars = append(route.Vars,
[]adctypes.StringOrSlice{
+ {StrVal: "uri"},
+ {StrVal: "=="},
+ {StrVal: *match.Path.Value},
+ })
case gatewayv1.PathMatchPathPrefix:
- pathValue := *match.Path.Value
- route.Uris = []string{pathValue}
-
- if strings.HasSuffix(pathValue, "/") {
- route.Uris = append(route.Uris, pathValue+"*")
- } else {
- route.Uris = append(route.Uris, pathValue+"/*")
+ pathValue :=
normalizeHTTPRoutePathPrefix(*match.Path.Value)
Review Comment:
This code change actually fixes a small bug in the previous implementation
of PathPrefix and therefore is a breaking change in behavior which should be
mentioned in the docs.
E.g.
Use-case 1: match.Path.Value = "mypath"
old code matches -> mypath, mypath/*
new code matches -> ^mypath(/|$) -> mypath, mypath/*
Use-case 2: match.Path.Value = "mypath/"
old code matches-> mypath/, mypath/*
new code matches -> normalizeHTTPRoutePathPrefix("mypath/") == "mypath" ->
^mypath(/|$) -> mypath, mypath/*
While use-case 1 stays the same in behavior, for users who have configured
paths like use-case 2 the behavior changes (before, "mypath" was no match and
"mypath/" was a match, now both are a match).
The new behavior is actually the one which is specified by the Gateway API
spec and is therefore correct. Still, the change could break existing
configurations of some users and should therefore be mentioned in
changelog/release-notes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
