http: fixing up CONNECT to be RFC compliant #21440

alyssawilk · 2022-05-25T14:20:35Z

Risk Level: Medium (data plane changes)
Testing: updated tests
Docs Changes: n/a
Release Notes: inline
Runtime guard: envoy.reloadable_features.use_rfc_connect
Fixes #20378

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

repokitteh-read-only · 2022-05-25T14:20:39Z

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: #21440 was opened by alyssawilk.

see: more, trace.

alyssawilk · 2022-05-25T14:22:59Z

cc @howardjohn

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

howardjohn · 2022-05-25T15:18:18Z

test/common/tcp_proxy/upstream_test.cc

+  std::unique_ptr<Http::RequestHeaderMapImpl> expected_headers;
+  expected_headers = Http::createHeaderMap<Http::RequestHeaderMapImpl>({
+      {Http::Headers::get().Method, "CONNECT"},
+      {Http::Headers::get().Host, this->config_->hostname()},


This is great, thanks for the fix!

One possibly concern is that in the current code we are using a header to replace Host, which allows dynamic metadata (

TunnelingConfig: &tcp.TcpProxy_TunnelingConfig{ Hostname: "ignored", HeadersToAdd: []*core.HeaderValueOption{ {Header: &core.HeaderValue{Key: "x-destination", Value: "%DYNAMIC_METADATA([\"tunnel\", \"address\"])%"}}, }, },

).

Now we would want to use Hostname to follow the spec, but that is only a static string.

Seems like something that could be expanded in a followup though @lambdai

@jewertow This is an instance when we need metadata for hostname. Not sure if you still want to complete #21067, but we'll probably add metadata support.

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

RyanTheOptimist

Looks great. A couple of minor comments.

RyanTheOptimist · 2022-05-26T19:37:33Z

source/common/http/http2/codec_impl.cc

-    modified_headers->setProtocol(Headers::get().ProtocolValues.Bytestream);
-    if (!headers.Path()) {
-      modified_headers->setPath("/");
+    if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.use_rfc_connect")) {


Should we consider moving the comment from line 296 down into the if blocks and editing it accordingly?

RyanTheOptimist · 2022-05-26T19:39:08Z

source/common/quic/envoy_quic_client_stream.cc

@@ -43,7 +43,8 @@ Http::Status EnvoyQuicClientStream::encodeHeaders(const Http::RequestHeaderMap&
  SendBufferMonitor::ScopedWatermarkBufferUpdater updater(this, this);
  auto spdy_headers = envoyHeadersToSpdyHeaderBlock(headers);
  if (headers.Method()) {
-    if (headers.Method()->value() == "CONNECT") {
+    if (headers.Method()->value() == "CONNECT" &&
+        !Runtime::runtimeFeatureEnabled("envoy.reloadable_features.use_rfc_connect")) {


I'm slightly surprised not to see an else for the case where the runtime flag is enabled to be parallel with the logic in the h2 code. Is that not required?

yeah it turns out we essentially had no testing of the HTTP/2 and HTTP/3 paths. Fixed.

RyanTheOptimist · 2022-05-26T19:41:18Z

test/common/http/http2/codec_impl_test.cc

@@ -3639,6 +3640,32 @@ TEST_P(Http2CodecImplTest, ConnectTest) {
  driveToCompletion();
 }

+// CONNECT without upgrade type gets tagged with "bytestream"


Given that the codec strips the :protocol = bytestream header, I'm not sure I understand this comment.

it was a bad copy paste :-P

RyanTheOptimist · 2022-05-26T19:43:22Z

test/extensions/filters/http/jwt_authn/filter_integration_test.cc

-    ASSERT_TRUE(codec_client_->waitForDisconnect());
-  }
+  // Because CONNECT requests do not include a path, they will fail
+  // to find a route match and return a 404.


It looks like this is a change in behavior that is visible to this filter. Presumably that's ok (and expected). Is there any need to call this out in release notes, or anything? Probably not, since they already mention the flag?

yeah that's my thought

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk · 2022-06-07T17:49:04Z

also @moderation if you have thoughts please feel encouraged to do a pass as well

alyssawilk · 2022-06-08T12:56:31Z

@snowp ping?

snowp

One small nit but otherwise this LGTM

snowp · 2022-06-08T13:12:44Z

source/common/tcp_proxy/upstream.cc

-  if (!config_.usePost()) {
-    headers->addReference(Http::Headers::get().Protocol,
-                          Http::Headers::get().ProtocolValues.Bytestream);
+  if (!Runtime::runtimeFeatureEnabled("envoy.reloadable_features.use_rfc_connect")) {


super nit but it might be nice to have all these checks have the feature enabled path first, just to make it easier to follow along the two branches when reading

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

snowp

Thanks!

Risk Level: Medium (data plane changes) Testing: updated tests Docs Changes: n/a Release Notes: inline Runtime guard: envoy.reloadable_features.use_rfc_connect Fixes envoyproxy#20378 Signed-off-by: Alyssa Wilk <alyssar@chromium.org> Signed-off-by: Tianyu Xia <tyxia@google.com>

Risk Level: Medium (data plane changes) Testing: updated tests Docs Changes: n/a Release Notes: inline Runtime guard: envoy.reloadable_features.use_rfc_connect Fixes envoyproxy#20378 Signed-off-by: Alyssa Wilk <alyssar@chromium.org> Signed-off-by: Amila Senadheera <amila.15@cse.mrt.ac.lk>

http: fixing up CONNECT to be RFC compliant

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Learn about vigilant mode

f327436

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

docs fix

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Learn about vigilant mode

f140470

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

howardjohn reviewed May 25, 2022

View reviewed changes

test fixup

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Learn about vigilant mode

1022eea

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk force-pushed the extended branch from f3465b3 to 1022eea Compare May 26, 2022 17:27

alyssawilk assigned RyanTheOptimist May 26, 2022

alyssawilk marked this pull request as ready for review May 26, 2022 17:27

alyssawilk requested review from KBaichoo, mattklein123 and lizan as code owners May 26, 2022 17:27

RyanTheOptimist reviewed May 26, 2022

View reviewed changes

PiotrSikora mentioned this pull request May 27, 2022

TcpProxy improperly sets :protocol=bytestream for HTTP2 #20378

Closed

KBaichoo added the waiting label May 31, 2022

alyssawilk added 2 commits June 6, 2022 09:57

Merge branch 'main' into extended

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Learn about vigilant mode

a87a0f9

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

comments

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Learn about vigilant mode

bb73c07

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

repokitteh-read-only bot removed the waiting label Jun 6, 2022

RyanTheOptimist previously approved these changes Jun 6, 2022

View reviewed changes

alyssawilk assigned snowp Jun 6, 2022

snowp suggested changes Jun 8, 2022

View reviewed changes

comment

0709f9e

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk dismissed RyanTheOptimist’s stale review via 0709f9e June 8, 2022 13:58

snowp approved these changes Jun 8, 2022

View reviewed changes

alyssawilk merged commit ac02715 into envoyproxy:main Jun 8, 2022

howardjohn mentioned this pull request Jun 21, 2022

tunneling_config: support dynamic Hostname #21804

Closed

KBaichoo mentioned this pull request Jan 18, 2023

envoy_reloadable_features_use_rfc_connect deprecation #25018

Closed

alyssawilk deleted the extended branch April 5, 2023 16:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http: fixing up CONNECT to be RFC compliant #21440

http: fixing up CONNECT to be RFC compliant #21440

alyssawilk commented May 25, 2022 •

edited

Loading

repokitteh-read-only bot commented May 25, 2022

alyssawilk commented May 25, 2022

howardjohn May 25, 2022

kyessenov May 25, 2022 •

edited

Loading

RyanTheOptimist left a comment

RyanTheOptimist May 26, 2022

alyssawilk Jun 6, 2022

RyanTheOptimist May 26, 2022

alyssawilk Jun 6, 2022

RyanTheOptimist May 26, 2022

alyssawilk Jun 6, 2022

RyanTheOptimist May 26, 2022

alyssawilk Jun 6, 2022

alyssawilk commented Jun 7, 2022

alyssawilk commented Jun 8, 2022

snowp left a comment

snowp Jun 8, 2022

snowp left a comment

http: fixing up CONNECT to be RFC compliant #21440

http: fixing up CONNECT to be RFC compliant #21440

Conversation

alyssawilk commented May 25, 2022 • edited Loading

repokitteh-read-only bot commented May 25, 2022

alyssawilk commented May 25, 2022

Choose a reason for hiding this comment

kyessenov May 25, 2022 • edited Loading

Choose a reason for hiding this comment

RyanTheOptimist 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

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

alyssawilk commented Jun 7, 2022

alyssawilk commented Jun 8, 2022

snowp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snowp left a comment

Choose a reason for hiding this comment

alyssawilk commented May 25, 2022 •

edited

Loading

kyessenov May 25, 2022 •

edited

Loading