AlinsRan commented on code in PR #2872:
URL:
https://github.com/apache/apisix-ingress-controller/pull/2872#discussion_r4227067463
##########
internal/adc/translator/httproute.go:
##########
@@ -316,12 +309,40 @@ func (t *Translator)
fillPluginFromHTTPRequestRedirectFilter(plugins adctypes.Pl
}
if reqRedirect.Port != nil {
- uri = fmt.Sprintf("%s://%s:%d$request_uri", scheme, hostname,
int(*reqRedirect.Port))
+ uri = fmt.Sprintf("%s://%s:%d", scheme, hostname,
int(*reqRedirect.Port))
} else {
- uri = fmt.Sprintf("%s://%s$request_uri", scheme, hostname)
+ uri = fmt.Sprintf("%s://%s", scheme, hostname)
}
+
plugin.RetCode = code
- plugin.URI = uri
+ if reqRedirect.Path == nil {
+ plugin.URI = uri + "$request_uri"
+ return
+ }
+
+ switch reqRedirect.Path.Type {
+ case gatewayv1.FullPathHTTPPathModifier:
+ if reqRedirect.Path.ReplaceFullPath != nil {
+ plugin.URI = uri + *reqRedirect.Path.ReplaceFullPath
+ plugin.AppendQueryString = true
+ }
+ case gatewayv1.PrefixMatchHTTPPathModifier:
+ plugin.RegexURI = buildPrefixMatchRegex(
+ *matches[0].Path.Value,
Review Comment:
nit: `*matches[0].Path.Value` (here and in the URLRewrite branch) relies on
the Gateway API CEL rule that `ReplacePrefixMatch` comes with exactly one
`PathPrefix` match. The previous URLRewrite code skipped non-prefix matches
instead of dereferencing. Since the caller can return an error now, a
`len(matches) != 1 || matches[0].Path == nil || matches[0].Path.Value == nil`
check costs little and avoids a panic if the CEL validation is ever bypassed.
##########
internal/adc/translator/httproute.go:
##########
@@ -316,12 +309,40 @@ func (t *Translator)
fillPluginFromHTTPRequestRedirectFilter(plugins adctypes.Pl
}
if reqRedirect.Port != nil {
- uri = fmt.Sprintf("%s://%s:%d$request_uri", scheme, hostname,
int(*reqRedirect.Port))
+ uri = fmt.Sprintf("%s://%s:%d", scheme, hostname,
int(*reqRedirect.Port))
} else {
- uri = fmt.Sprintf("%s://%s$request_uri", scheme, hostname)
+ uri = fmt.Sprintf("%s://%s", scheme, hostname)
}
+
plugin.RetCode = code
- plugin.URI = uri
+ if reqRedirect.Path == nil {
+ plugin.URI = uri + "$request_uri"
+ return
+ }
+
+ switch reqRedirect.Path.Type {
+ case gatewayv1.FullPathHTTPPathModifier:
+ if reqRedirect.Path.ReplaceFullPath != nil {
+ plugin.URI = uri + *reqRedirect.Path.ReplaceFullPath
+ plugin.AppendQueryString = true
+ }
+ case gatewayv1.PrefixMatchHTTPPathModifier:
+ plugin.RegexURI = buildPrefixMatchRegex(
+ *matches[0].Path.Value,
+ *reqRedirect.Path.ReplacePrefixMatch,
+ )
+ if reqRedirect.Hostname != nil {
Review Comment:
When `hostname` is not set, `scheme` and `port` are silently dropped for
`ReplacePrefixMatch`. The redirect plugin expands `$host` / `$scheme` only in
`uri` (via `concat_new_uri`). `regex_uri` goes straight through
`ngx.re.sub(ctx.var.uri, ...)`, so the template can't carry them.
Output of `fillPluginFromHTTPRequestRedirectFilter` for a rule matching
`PathPrefix /old` (this branch merged onto master):
| redirect filter | generated `redirect` config |
|---|---|
| `scheme: https`, `ReplacePrefixMatch: /new` |
`"regex_uri":["^/old(/.*)?$","/new$1"]` (scheme lost) |
| `port: 8443`, `ReplacePrefixMatch: /new` |
`"regex_uri":["^/old(/.*)?$","/new$1"]` (port lost) |
| `scheme: https`, `ReplacePrefixMatch: /old` |
`"regex_uri":["^/old(/.*)?$","/old$1"]` (Location points back to the same URL,
so the client loops, as in #2857) |
| `scheme: https`, `ReplaceFullPath: /x` | `"uri":"https://$host/x"` (OK) |
`fillPluginsFromHTTPRouteFilters` already returns an error on master, so
this helper could return one as well. Reject `ReplacePrefixMatch` combined with
`scheme`/`port` but no `hostname` so that it surfaces on the route status,
instead of generating a redirect that doesn't do what the filter asks for. Full
support would need `regex_uri` templates to expand variables in APISIX, which
is better tracked as a separate issue.
--
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]
