hcm: add match_upstream to SchemeHeaderTransformation #34099

wtzhang23 · 2024-05-11T17:14:10Z

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

repokitteh-read-only · 2024-05-11T17:14:15Z

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.

🐱

Caused by: #34099 was opened by wtzhang23.

see: more, trace.

repokitteh-read-only · 2024-05-11T17:14:16Z

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #34099 was opened by wtzhang23.

see: more, trace.

repokitteh-read-only · 2024-05-11T17:14:21Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @adisuissa
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #34099 was opened by wtzhang23.

see: more, trace.

wtzhang23 · 2024-05-11T17:18:43Z

Small Qs:

Not familiar with the async client implementation. Is it possible that especially the gRPC client may take advantage of these settings?
What setting should the Admin interface use? I have it set to true which from my understanding will be a no-op.
Pass the should_match_upstream as a constructor param or keep using a setter in StreamInfo?

Signed-off-by: William <wtzhang23@gmail.com>

wtzhang23 · 2024-05-11T22:30:50Z

/retest

adisuissa

Thanks!
High-level API comment, but otherwise API LGTM.

api/envoy/config/core/v3/protocol.proto

Signed-off-by: William <wtzhang23@gmail.com>

api/envoy/config/core/v3/protocol.proto

adisuissa

Thanks!
Please also add a release note.
/assign-from @envoyproxy/senior-maintainers

source/common/http/async_client_impl.cc

source/extensions/filters/network/http_connection_manager/config.cc

source/server/admin/admin.h

test/common/http/conn_manager_impl_test_2.cc

adisuissa · 2024-05-13T19:22:29Z

test/common/http/conn_manager_impl_test_2.cc

@@ -4210,5 +4210,63 @@ TEST_F(HttpConnectionManagerImplTest, DownstreamTimingsRecordWhenRequestHeaderPr
  filter_callbacks_.connection_.raiseEvent(Network::ConnectionEvent::RemoteClose);
 }

+TEST_F(HttpConnectionManagerImplTest, PassMatchUpstreamSchemeHintToStreamInfo) {


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?

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.

adisuissa · 2024-05-13T19:24:05Z

test/common/http/conn_manager_impl_test_base.h

@@ -235,6 +236,7 @@ class HttpConnectionManagerImplMixin : public ConnectionManagerConfig {
  HttpConnectionManagerProto::ServerHeaderTransformation server_transformation_{
      HttpConnectionManagerProto::OVERWRITE};
  absl::optional<std::string> scheme_;
+  bool scheme_match_upstream_{false};


nit: If this is not being overwritten, consider converting to const.

Need this to be non-const to override in the PassMatchUpstreamSchemeHintToStreamInfo test I wrote.

repokitteh-read-only · 2024-05-13T19:27:02Z

@envoyproxy/senior-maintainers assignee is @htuch

🐱

Caused by: a #34099 (review) was submitted by @adisuissa.

see: more, trace.

…compat Signed-off-by: William <wtzhang23@gmail.com>

Signed-off-by: William <wtzhang23@gmail.com>

wtzhang23 · 2024-05-14T00:42:26Z

/retest

htuch · 2024-05-14T03:39:39Z

Reassigning to @alyssawilk for data plane senior maintainer coverage.

alyssawilk

I think this version is cleaner, thanks for being willing to move over to it!
/wait on comments

alyssawilk · 2024-05-14T15:52:56Z

api/envoy/config/core/v3/protocol.proto

+  // request be sent to the upstream over TLS, the scheme header will be set to "https". Should the
+  // request be sent over plaintext, the scheme header will be set to "http".
+  // If scheme_to_overwrite is set, this field is not used.
+  bool match_upstream = 2;


If we have to have this separate can we instead reject config if both are set?

Not too familiar with how validation of configs is done but my understanding is it uses protoc-gen-validate and I'm not sure if that schema supports cross-field rules? ~~Could you guide me on where in the code base validation occurs to add this change?~~ Looked through the code base for other examples and it seems throwing an exception is how this is done.

FWIW I do have a warning log present already. But agree it might be worthwhile to not fail silently.

Previously Envoy would've rejected the config. However, the issue is supporting both old and new Envoy binaries that use the same server. The server may want to set the new field for the new Envoys, but have the old field set for the old ones. That is why precedence was preferred over rejection.

That said, in the current case the new field does not take precedence over the old one, so it is less of an issue.

Should we just use oneof then since this new field doesn't take precedence so if a user wants to use this field they have to not set the other field? Think I'd prefer this as a user rather than an exception, even if it violates the style guide. Especially since those that maintain/debug the data plane may not necessarily be those that own the control plane.

alyssawilk · 2024-05-14T15:55:16Z

source/common/http/async_client_impl.cc

@@ -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);


ooc isn't this just setting it to the default? Why not remove?

source/extensions/filters/network/http_connection_manager/config.cc

alyssawilk · 2024-05-14T15:59:12Z

test/common/router/router_2_test.cc

@@ -978,5 +978,33 @@ TEST_F(RouterTestSupressGRPCStatsDisabled, IncludeHttpTimeoutStats) {
                .value());
 }

+class RouterTestSchemeMatchUpstream : public RouterTestBase {


do you need to subclass? Can't you just put the expect_call in your test body?

Removed new class and moved code to router.cc where all the tests that use RouterTest live.

alyssawilk · 2024-05-14T16:04:14Z

source/common/router/router.cc

@@ -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,


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

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.

Decided to keep it simple and just check if it has an SSL context (TLS) to set the scheme to https, otherwise use http.

alyssawilk · 2024-05-14T16:05:47Z

api/envoy/config/core/v3/protocol.proto

    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


comment doesn't match code as commented below. Do we want to set ssl over secure transports or only over TLS?

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>

…tocol test Signed-off-by: William <wtzhang23@gmail.com>

Signed-off-by: William <wtzhang23@gmail.com>

repokitteh-read-only bot added the api label May 11, 2024

repokitteh-read-only bot assigned adisuissa May 11, 2024

wtzhang23 force-pushed the hcm-match-upstream branch from bd0e1b2 to 90b1609 Compare May 11, 2024 17:14

wtzhang23 marked this pull request as ready for review May 11, 2024 17:18

wtzhang23 requested review from zuercher, alyssawilk and mattklein123 as code owners May 11, 2024 17:18

wtzhang23 mentioned this pull request May 11, 2024

router: add use_upstream_scheme to router filter #33839

Open

add match_upstream to SchemeHeaderTransformation

d419b59

Signed-off-by: William <wtzhang23@gmail.com>

wtzhang23 force-pushed the hcm-match-upstream branch from 90b1609 to d419b59 Compare May 11, 2024 17:58

adisuissa reviewed May 13, 2024

View reviewed changes

api/envoy/config/core/v3/protocol.proto Outdated Show resolved Hide resolved

move match_upstream out of oneof block

0a542fb

Signed-off-by: William <wtzhang23@gmail.com>

alyssawilk reviewed May 13, 2024

View reviewed changes

api/envoy/config/core/v3/protocol.proto Show resolved Hide resolved

adisuissa reviewed May 13, 2024

View reviewed changes

repokitteh-read-only bot assigned htuch May 13, 2024

wtzhang23 added 4 commits May 13, 2024 12:40

set match upstream to false for admin and async client for backwards …

07ccff2

…compat Signed-off-by: William <wtzhang23@gmail.com>

refactor PassMatchUpstreamSchemeHintToStreamInfo

92dd21e

Signed-off-by: William <wtzhang23@gmail.com>

log warning when match_upstream and scheme_to_overwrite are both set

16d82f8

Signed-off-by: William <wtzhang23@gmail.com>

add release note

5ea3820

Signed-off-by: William <wtzhang23@gmail.com>

htuch assigned alyssawilk and unassigned htuch May 14, 2024

alyssawilk reviewed May 14, 2024

View reviewed changes

repokitteh-read-only bot added the waiting label May 14, 2024

wtzhang23 added 3 commits May 25, 2024 13:46

don't explicitly set match upstream if using default

9325d99

Signed-off-by: William <wtzhang23@gmail.com>

set scheme to https only if upstream transport is ssl

695663e

Signed-off-by: William <wtzhang23@gmail.com>

don't use unique subclass for OverwriteSchemeWithUpstreamTransportPro…

0e5e248

…tocol test Signed-off-by: William <wtzhang23@gmail.com>

repokitteh-read-only bot removed the waiting label May 25, 2024

rename downstream_secure to downstream_ssl

0a80882

Signed-off-by: William <wtzhang23@gmail.com>

wtzhang23 force-pushed the hcm-match-upstream branch from 1ea29f6 to 0a80882 Compare May 25, 2024 21:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

hcm: add match_upstream to SchemeHeaderTransformation #34099

hcm: add match_upstream to SchemeHeaderTransformation #34099

wtzhang23 commented May 11, 2024 •

edited

repokitteh-read-only bot commented May 11, 2024

repokitteh-read-only bot commented May 11, 2024

repokitteh-read-only bot commented May 11, 2024

wtzhang23 commented May 11, 2024 •

edited

wtzhang23 commented May 11, 2024

adisuissa left a comment

adisuissa left a comment

adisuissa May 13, 2024

wtzhang23 May 13, 2024

adisuissa May 13, 2024

wtzhang23 May 13, 2024

repokitteh-read-only bot commented May 13, 2024

wtzhang23 commented May 14, 2024

htuch commented May 14, 2024

alyssawilk left a comment

alyssawilk May 14, 2024

wtzhang23 May 14, 2024 •

edited

adisuissa May 15, 2024

wtzhang23 May 25, 2024 •

edited

alyssawilk May 14, 2024

alyssawilk May 14, 2024

wtzhang23 May 25, 2024

alyssawilk May 14, 2024

wtzhang23 May 14, 2024 •

edited

wtzhang23 May 25, 2024

alyssawilk May 14, 2024

wtzhang23 May 25, 2024

hcm: add match_upstream to SchemeHeaderTransformation #34099

Are you sure you want to change the base?

hcm: add match_upstream to SchemeHeaderTransformation #34099

Conversation

wtzhang23 commented May 11, 2024 • edited

repokitteh-read-only bot commented May 11, 2024

repokitteh-read-only bot commented May 11, 2024

repokitteh-read-only bot commented May 11, 2024

wtzhang23 commented May 11, 2024 • edited

wtzhang23 commented May 11, 2024

adisuissa left a comment

Choose a reason for hiding this comment

adisuissa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

repokitteh-read-only bot commented May 13, 2024

wtzhang23 commented May 14, 2024

htuch commented May 14, 2024

alyssawilk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wtzhang23 May 14, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wtzhang23 May 25, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wtzhang23 May 14, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wtzhang23 commented May 11, 2024 •

edited

wtzhang23 commented May 11, 2024 •

edited

wtzhang23 May 14, 2024 •

edited

wtzhang23 May 25, 2024 •

edited

wtzhang23 May 14, 2024 •

edited