router redirect: use port from the request if redirect_port not specified #25573
Conversation
Signed-off-by: Christoph Pakulski <christoph@tetrate.io>
Signed-off-by: Christoph Pakulski <christoph@tetrate.io>
Signed-off-by: Christoph Pakulski <christoph@tetrate.io>
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
There was a problem hiding this comment.
LGTM modulo comments, @yanavlasov to provide another set of eyes.
| @@ -1117,6 +1109,17 @@ std::string RouteEntryImplBase::newPath(const Http::RequestHeaderMap& headers) c | |||
| final_port = redirect_config_->port_redirect_.c_str(); | |||
| } else { | |||
| final_port = ""; | |||
| const absl::string_view current_path = headers.getHostValue(); | |||
There was a problem hiding this comment.
Out of curiosity, this is a direct response but not necessarily a redirect when you get here I think, what's to stop this applying even if it's not a redirect?
|
/lgtm api |
| @@ -1659,6 +1659,7 @@ message RedirectAction { | |||
| [(validate.rules).string = {well_known_regex: HTTP_HEADER_VALUE strict: false}]; | |||
|
|
|||
| // The port value of the URL will be swapped with this value. | |||
| // If not specified, the request's port will be used. | |||
There was a problem hiding this comment.
The "request port" is ambiguous as there can be multiple ports associated with a given request. Is it from :authority? from the actual L4 port?
There was a problem hiding this comment.
It is from headers, not L4.
| @@ -20,6 +20,9 @@ minor_behavior_changes: | |||
| - area: http2 | |||
| change: | | |||
| Request authorities are now validated with a library function from QUICHE rather than nghttp2. This behavior change can be reverted by setting ``envoy.reloadable_features.http2_validate_authority_with_quiche`` to ``false``. | |||
| - area: router redirect | |||
| change: | | |||
| when ``port_redirect`` is not specified, the request's port is used. | |||
There was a problem hiding this comment.
Is this a breaking change? Our current API, which exposes this option, has the following enum option:
FROM_PROTOCOL_DEFAULT: automatically set to 80 for HTTP and 443 for HTTPS.
AFAIK this was the previous behavior in envoy with port unset. With this new change, it will use another port
There was a problem hiding this comment.
AFAIK this was the previous behavior in envoy with port unset. With this new change, it will use another port
If port_redirect is set explicitly, there is no change in behavior. Does your API requires this param or can it be left unspecified?
There was a problem hiding this comment.
Sorry to clarify, when a user sets that in our API we do not set port_redirect and explicitly expect the "automatically set to 80 for HTTP and 443 for HTTPS.". This behavior is changed by this PR, which I think represents a breaking API change for envoy.
There was a problem hiding this comment.
Thanks for explaining. To be precise, Envoy did not add port 80 or 443, but simply stripped the incoming port. The result was that port was auto chosen based on the protocol: http or https.
Solution to that problem would be to set port_redirect to zero for reusing the port from the request. The logic would be:
- port_redirect not specified - as before (strip port)
- port_redirect = 0 (reuse from request)
- port_redirect = value (use the value specified)
There was a problem hiding this comment.
With a uint32 I don't think you can distinguish unset and 0?
There was a problem hiding this comment.
yeah.. you are right :-(
There was a problem hiding this comment.
API LGTM revoked until we resolve this, thanks for noticing @howardjohn
There was a problem hiding this comment.
@howardjohn if Istio's API says
FROM_PROTOCOL_DEFAULT: automatically set to 80 for HTTP and 443 for HTTPS
It could now explicitly set these published port values in
https://github.com/istio/istio/blob/96f6a4f16d5a5aa4905ff0c0f94d9dd607b5f5f3/pilot/pkg/networking/core/v1alpha3/route/route.go#L603
rather than implictly assuming envoy will
from the original API PR istio/api#2088 it looks like this is what istio would have wanted envoy to do
There was a problem hiding this comment.
It really doesn't matter what Istio does or docs say, this is a breaking change in Envoy which will extend beyond Istio.
|
/wait |
|
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
github-actions
bot
added
the
stale
|
I am planning to add a new config item with 3 options:
It will be backwards compatible, so if not specified, it will behave as before (STRIP). If |
github-actions
bot
removed
the
stale
|
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
github-actions
bot
added
the
stale
|
Closing for now. Will open a new PR with new API. |
Commit Message:
use port from the request if redirect_port not specified
Additional Description:
If redirect_port config item was empty, the redirect URL did not contain any port. Now the same port is used as in the original request.
Risk Level: Low
Testing: added unit tests and manual testing.
Docs Changes: yes
Release Notes: yes
Fixes #17318