AlinsRan commented on code in PR #2872:
URL:
https://github.com/apache/apisix-ingress-controller/pull/2872#discussion_r4227067454
##########
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:
**Regression: `ReplacePrefixMatch` drops `scheme` and `port` when `hostname`
is not set.**
### Why
The APISIX `redirect` plugin expands variables such as `$host` and `$scheme`
only in `uri`. `regex_uri` is applied with a plain `ngx.re.sub(ctx.var.uri,
...)`, so its template cannot carry the scheme or host. This branch therefore
adds them only when `hostname` is set, and drops them otherwise.
### Master vs. this PR
Rule matching `PathPrefix /old`, translated with
`fillPluginFromHTTPRequestRedirectFilter`:
| redirect filter | master | this PR |
|---|---|---|
| `ReplacePrefixMatch: /new` | `$scheme://$host$request_uri` (redirect loop,
#2857) | `/new$1` ✅ |
| `scheme: https` + `ReplacePrefixMatch: /new` | `https://$host$request_uri`
(path ignored) | `/new$1` ❌ https lost |
| `port: 8443` + `ReplacePrefixMatch: /new` |
`$scheme://$host:8443$request_uri` (path ignored) | `/new$1` ❌ port lost |
| `scheme: https` + `ReplacePrefixMatch: /old` | `https://$host$request_uri`
✅ | `/old$1` ❌ redirect loop |
`ReplaceFullPath` uses `uri`, so it keeps scheme and port and is not
affected.
On master these configs at least upgrade the scheme or switch the port. With
this PR they lose that, and the last row goes from a working redirect to an
infinite loop.
### Suggestion
`fillPluginsFromHTTPRouteFilters` already returns an error on master, so
this helper can return one too:
- Reject `ReplacePrefixMatch` combined with `scheme` or `port` but no
`hostname`, so the problem shows up on the route status instead of producing a
wrong redirect.
- Full support would need `regex_uri` templates to expand variables in
APISIX. That 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]
