hcm: add match_upstream to SchemeHeaderTransformation by wtzhang23 · Pull Request #34099 · envoyproxy/envoy

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

  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

  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

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

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

source/common/http/async_client_impl.cc

  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

                .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

 }

-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

  }
+
+  // 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>

alyssawilk · 2024-05-28T13:32:13Z

/wait on CI

wtzhang23 · 2024-05-28T14:07:22Z

/retest

alyssawilk

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.

alyssawilk · 2024-06-04T12:57:42Z

ah actually needs shephard LGTM

adisuissa

/lgtm api

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 alyssawilk, mattklein123 and zuercher 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

Closed

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

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

alyssawilk removed the waiting label Jun 3, 2024

alyssawilk approved these changes Jun 3, 2024

View reviewed changes

adisuissa reviewed Jun 4, 2024

View reviewed changes

repokitteh-read-only bot removed the api label Jun 4, 2024

adisuissa merged commit b7d61cb into envoyproxy:main Jun 4, 2024

wtzhang23 deleted the hcm-match-upstream branch June 4, 2024 18:43

Conversation

wtzhang23 commented May 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented May 11, 2024

Uh oh!

repokitteh-read-only bot commented May 11, 2024

Uh oh!

repokitteh-read-only bot commented May 11, 2024

Uh oh!

wtzhang23 commented May 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wtzhang23 commented May 11, 2024

Uh oh!

adisuissa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

adisuissa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

repokitteh-read-only bot commented May 13, 2024

Uh oh!

wtzhang23 commented May 14, 2024

Uh oh!

htuch commented May 14, 2024

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wtzhang23 May 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alyssawilk commented May 28, 2024

Uh oh!

wtzhang23 commented May 28, 2024

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

alyssawilk commented Jun 4, 2024

Uh oh!

adisuissa left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

wtzhang23 commented May 11, 2024 •

edited

Loading

wtzhang23 commented May 11, 2024 •

edited

Loading

wtzhang23 May 14, 2024 •

edited

Loading