-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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.
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
hcm: add match_upstream to SchemeHeaderTransformation #34099
Conversation
Hi @wtzhang23, welcome and thank you for your contribution. We will try to review your Pull Request as quickly as possible. In the meantime, please take a look at the contribution guidelines if you have not done so already. |
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
bd0e1b2
to
90b1609
Compare
Small Qs:
|
Signed-off-by: William <wtzhang23@gmail.com>
90b1609
to
d419b59
Compare
/retest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
High-level API comment, but otherwise API LGTM.
Signed-off-by: William <wtzhang23@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Please also add a release note.
/assign-from @envoyproxy/senior-maintainers
@@ -4210,5 +4210,63 @@ TEST_F(HttpConnectionManagerImplTest, DownstreamTimingsRecordWhenRequestHeaderPr | |||
filter_callbacks_.connection_.raiseEvent(Network::ConnectionEvent::RemoteClose); | |||
} | |||
|
|||
TEST_F(HttpConnectionManagerImplTest, PassMatchUpstreamSchemeHintToStreamInfo) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
High-level comment:
it seems that the relevant part of the test is essentially:
EXPECT_TRUE(filter->callbacks_->streamInfo().shouldSchemeMatchUpstream());
Can this test be refactored to minimize the non-relevant parts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cut as much of the test as I'm familiar with. Will need to dive deeper to see if any other cuts can be made.
@@ -235,6 +236,7 @@ class HttpConnectionManagerImplMixin : public ConnectionManagerConfig { | |||
HttpConnectionManagerProto::ServerHeaderTransformation server_transformation_{ | |||
HttpConnectionManagerProto::OVERWRITE}; | |||
absl::optional<std::string> scheme_; | |||
bool scheme_match_upstream_{false}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: If this is not being overwritten, consider converting to const.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need this to be non-const to override in the PassMatchUpstreamSchemeHintToStreamInfo
test I wrote.
@envoyproxy/senior-maintainers assignee is @htuch |
…compat Signed-off-by: William <wtzhang23@gmail.com>
Signed-off-by: William <wtzhang23@gmail.com>
Signed-off-by: William <wtzhang23@gmail.com>
Signed-off-by: William <wtzhang23@gmail.com>
/retest |
Reassigning to @alyssawilk for data plane senior maintainer coverage. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this version is cleaner, thanks for being willing to move over to it!
/wait on comments
@@ -116,6 +116,7 @@ AsyncStreamImpl::AsyncStreamImpl(AsyncClientImpl& parent, AsyncClient::StreamCal | |||
stream_info_.dynamicMetadata().MergeFrom(options.metadata); | |||
stream_info_.setIsShadow(options.is_shadow); | |||
stream_info_.setUpstreamClusterInfo(parent_.cluster_); | |||
stream_info_.setShouldSchemeMatchUpstream(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ooc isn't this just setting it to the default? Why not remove?
test/common/router/router_2_test.cc
Outdated
@@ -978,5 +978,33 @@ TEST_F(RouterTestSupressGRPCStatsDisabled, IncludeHttpTimeoutStats) { | |||
.value()); | |||
} | |||
|
|||
class RouterTestSchemeMatchUpstream : public RouterTestBase { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you need to subclass? Can't you just put the expect_call in your test body?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed new class and moved code to router.cc where all the tests that use RouterTest
live.
source/common/router/router.cc
Outdated
@@ -124,7 +124,17 @@ uint64_t FilterUtility::percentageOfTimeout(const std::chrono::milliseconds resp | |||
return static_cast<uint64_t>(response_time.count() * TimeoutPrecisionFactor / timeout.count()); | |||
} | |||
|
|||
void FilterUtility::setUpstreamScheme(Http::RequestHeaderMap& headers, bool downstream_secure) { | |||
void FilterUtility::setUpstreamScheme(Http::RequestHeaderMap& headers, bool downstream_secure, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so there's a difference in what we pass in here
I think we need to decide if we want to set scheme based on "is upstream ssl" or "is upstream secure"
e.g. ALTS is secure but not TLS
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the golang grpc and alts source code, it seems like this should only be set when the upstream is using TLS and not ATLS. The RFC also seems to couple it with TLS.
Given the number of transport sockets, I'm thinking of adding a new virtual method to the transport socket to allow each socket to select which scheme it would like to set. Let me know if you have any opinions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Decided to keep it simple and just check if it has an SSL context (TLS) to set the scheme to https, otherwise use http.
string scheme_to_overwrite = 1 [(validate.rules).string = {in: "http" in: "https"}]; | ||
} | ||
|
||
// Set the Scheme header to match the upstream transport protocol. For example, should a | ||
// request be sent to the upstream over TLS, the scheme header will be set to "https". Should the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comment doesn't match code as commented below. Do we want to set ssl over secure transports or only over TLS?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Decided to set it only over TLS to be consistent with the downstream fallback, which also only sets it if ssl_connection
is not null.
Signed-off-by: William <wtzhang23@gmail.com>
Signed-off-by: William <wtzhang23@gmail.com>
…tocol test Signed-off-by: William <wtzhang23@gmail.com>
Signed-off-by: William <wtzhang23@gmail.com>
1ea29f6
to
0a80882
Compare
/wait on CI |
/retest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
apologies - this got tagged as waiting due to CI but fixing CI didn't untag.
LGTM. @adisuissa any final thoughts? if not no I'll merge tomorrow.
ah actually needs shephard LGTM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm api
Commit Message: add match_upstream to SchemeHeaderTransformation
Additional Description: Define a new variant that configures the hcm to override the scheme with the upstream transport protocol. The value is accessible by all downstream filters and is specifically used by the Router filter. Renamed variant from
use_upstream_scheme
to be clear that the scheme matches the transport protocol.Risk Level: low
Testing: unit testing
Docs Changes: n/a
Release Notes: n/a
Platform Specific Features: n/a
Fixes #33020