Skip to content
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.

Sign up for GitHub

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

router redirect: use port from the request if redirect_port not specified #25573

Closed
wants to merge 3 commits into from
Closed

router redirect: use port from the request if redirect_port not specified #25573

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
eb7cfd6
redirect: preserve port if not specified in config
cpakulski Feb 14, 2023
c6b1ce4
Merge remote-tracking branch 'upstream/main' into issue/17318
cpakulski Feb 14, 2023
a5b7f5a
updated docs and release notes.
cpakulski Feb 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions api/envoy/config/route/v3/route_components.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is from headers, not L4.

uint32 port_redirect = 8;

oneof path_rewrite_specifier {
Expand Down
3 changes: 3 additions & 0 deletions changelogs/current.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With a uint32 I don't think you can distinguish unset and 0?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah.. you are right :-(

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

API LGTM revoked until we resolve this, thanks for noticing @howardjohn

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It really doesn't matter what Istio does or docs say, this is a breaking change in Envoy which will extend beyond Istio.


bug_fixes:
# *Changes expected to improve the state of the world and are unlikely to have negative effects*
Expand Down
51 changes: 27 additions & 24 deletions source/common/router/config_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1053,41 +1053,33 @@ absl::optional<std::string> RouteEntryImplBase::currentUrlPathAfterRewriteWithMa
return {};
}

bool RouteEntryImplBase::newSchemePortMismatch(absl::string_view request_protocol,
absl::string_view request_port,
absl::string_view new_scheme) const {
// In the rare case that X-Forwarded-Proto and scheme disagree (say http URL over an HTTPS
// connection), do port stripping based on X-Forwarded-Proto so http://foo.com:80 won't
// have the port stripped when served over TLS.
return ((new_scheme != request_protocol) &&
(((request_protocol == Http::Headers::get().SchemeValues.Https.c_str()) &&
request_port == ":443") ||
((request_protocol == Http::Headers::get().SchemeValues.Http.c_str()) &&
request_port == ":80")));
}

absl::string_view RouteEntryImplBase::processRequestHost(const Http::RequestHeaderMap& headers,
absl::string_view new_scheme,
absl::string_view new_port) const {

absl::string_view request_host = headers.getHostValue();
size_t host_end;
if (request_host.empty()) {
return request_host;
}
// Detect if IPv6 URI
if (request_host[0] == '[') {
host_end = request_host.rfind("]:");
if (host_end != absl::string_view::npos) {
host_end += 1; // advance to :
}
} else {
host_end = request_host.rfind(':');
}
const size_t host_end = Http::HeaderUtility::getPortStart(request_host);

if (host_end != absl::string_view::npos) {
absl::string_view request_port = request_host.substr(host_end);
// In the rare case that X-Forwarded-Proto and scheme disagree (say http URL over an HTTPS
// connection), do port stripping based on X-Forwarded-Proto so http://foo.com:80 won't
// have the port stripped when served over TLS.
absl::string_view request_protocol = headers.getForwardedProtoValue();
bool remove_port = !new_port.empty();

if (new_scheme != request_protocol) {
remove_port |= (request_protocol == Http::Headers::get().SchemeValues.Https.c_str()) &&
request_port == ":443";
remove_port |= (request_protocol == Http::Headers::get().SchemeValues.Http.c_str()) &&
request_port == ":80";
}

if (remove_port) {
if (!new_port.empty() ||
newSchemePortMismatch(headers.getForwardedProtoValue(), request_port, new_scheme)) {
return request_host.substr(0, host_end);
}
}
Expand Down Expand Up @@ -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();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

const auto port_start = Http::HeaderUtility::getPortStart(current_path);
if (port_start != absl::string_view::npos) {
const auto deduced_port = current_path.substr(port_start);
// use deduced port only when scheme did not change to avoid using port 80 when scheme
// changed from http to https and to avoid using port 443 when scheme changed from
// https to http.
if (!newSchemePortMismatch(headers.getForwardedProtoValue(), deduced_port, final_scheme)) {
htuch marked this conversation as resolved.
Show resolved Hide resolved
final_port = deduced_port;
}
}
}

if (redirect_config_ != nullptr && !redirect_config_->host_redirect_.empty()) {
Expand Down
2 changes: 2 additions & 0 deletions source/common/router/config_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -801,6 +801,8 @@ class RouteEntryImplBase : public RouteEntryAndRoute,

// Router::DirectResponseEntry
std::string newPath(const Http::RequestHeaderMap& headers) const override;
bool newSchemePortMismatch(absl::string_view request_protocol, absl::string_view request_port,
absl::string_view new_scheme) const;
absl::string_view processRequestHost(const Http::RequestHeaderMap& headers,
absl::string_view new_scheme,
absl::string_view new_port) const;
Expand Down
24 changes: 24 additions & 0 deletions test/common/router/config_impl_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5491,6 +5491,8 @@ max_direct_response_body_size_bytes: 1024
routes:
- match: { path: /port }
redirect: { port_redirect: 8181 }
- match: { path: /deduce_port }
redirect: { host_redirect: new.lyft.com }
- name: redirect_ipv4
domains: [10.0.0.1]
routes:
Expand All @@ -5505,6 +5507,8 @@ max_direct_response_body_size_bytes: 1024
routes:
- match: { path: /port }
redirect: { port_redirect: 8181 }
- match: { path: /deduce_port }
redirect: { host_redirect: 20.0.0.2 }
- name: redirect_ipv4_port_80
domains: [10.0.0.1:80]
routes:
Expand Down Expand Up @@ -5537,6 +5541,8 @@ max_direct_response_body_size_bytes: 1024
routes:
- match: { path: /port }
redirect: { port_redirect: 8181 }
- match: { path: /deduce_port }
redirect: { host_redirect: "[fe80::2]" }
- name: redirect_ipv6_port_80
domains: ["[fe80::1]:80"]
routes:
Expand Down Expand Up @@ -5609,6 +5615,12 @@ max_direct_response_body_size_bytes: 1024
EXPECT_EQ("https://api.lyft.com/foo",
config.route(headers, 0)->directResponseEntry()->newPath(headers));
}
{
Http::TestRequestHeaderMapImpl headers =
genRedirectHeaders("redirect.lyft.com:8080", "/deduce_port", false, false);
EXPECT_EQ("http://new.lyft.com:8080/deduce_port",
config.route(headers, 0)->directResponseEntry()->newPath(headers));
}
{
Http::TestRequestHeaderMapImpl headers =
genRedirectHeaders("redirect.lyft.com", "/host", false, false);
Expand Down Expand Up @@ -5759,6 +5771,12 @@ max_direct_response_body_size_bytes: 1024
EXPECT_EQ("http://10.0.0.1:8181/port",
config.route(headers, 0)->directResponseEntry()->newPath(headers));
}
{
Http::TestRequestHeaderMapImpl headers =
genRedirectHeaders("10.0.0.1:8080", "/deduce_port", false, false);
EXPECT_EQ("http://20.0.0.2:8080/deduce_port",
config.route(headers, 0)->directResponseEntry()->newPath(headers));
}
{
Http::TestRequestHeaderMapImpl headers =
genRedirectHeaders("10.0.0.1", "/host_port", false, false);
Expand Down Expand Up @@ -5818,6 +5836,12 @@ max_direct_response_body_size_bytes: 1024
EXPECT_EQ("http://[fe80::1]:8181/port",
config.route(headers, 0)->directResponseEntry()->newPath(headers));
}
{
Http::TestRequestHeaderMapImpl headers =
genRedirectHeaders("[fe80::1]:8080", "/deduce_port", false, false);
EXPECT_EQ("http://[fe80::2]:8080/deduce_port",
config.route(headers, 0)->directResponseEntry()->newPath(headers));
}
{
Http::TestRequestHeaderMapImpl headers =
genRedirectHeaders("[fe80::1]", "/host_port", false, false);
Expand Down