Adding direct connect support. by alyssawilk · Pull Request #21942 · envoyproxy/envoy

alyssawilk · 2022-06-29T14:31:37Z

Creates a transport socket for HTP/1.1 proxy support.

With the combination of the transport socket, and a filter putting the proxy stream info in place this will

redirect TCP connections to the proxy IP address
prefix TLS connections with cleartext CONECT headers to the destination host, and strip CONNECT response
change cleartext HTTP/1.1 to send fully qualified URLs

Risk Level: medium (intended as a no op but it does have data plane refactory)
Testing: new unit, integration tests
Docs Changes: n/a

Part of envoyproxy/envoy-mobile#1622

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

repokitteh-read-only · 2022-06-29T14:31:41Z

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

see: more, trace.

repokitteh-read-only · 2022-06-29T14:31:47Z

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

🐱

Caused by: #21942 was opened by alyssawilk.

see: more, trace.

RyanTheOptimist

This looks amazing! A couple of legit comments but mostly just nits.

api/envoy/extensions/transport_sockets/http_11_proxy/v3/upstream_http_11_connect.proto

envoy/network/transport_socket.h

source/common/network/filter_state_proxy_info.h

test/extensions/transport_sockets/http_11_proxy/connect_integration_test.cc

test/extensions/transport_sockets/http_11_proxy/connect_test.cc

kyessenov · 2022-06-30T02:25:38Z

envoy/network/transport_socket.h

+  /**
+   * @return any proxy information if sending to an intermediate proxy over HTTP/1.1.
+   */
+  virtual const std::unique_ptr<const ProxyInfo>& proxyInfo() const PURE;


Why not keep this private to the extension? E.g. per #21700, would be another private hashable object.

I don't think it can be because Envoy needs to use it for hashing and connection creation.

These objects are also hashed in transport options and passed along during connection creation to the upstream connection stream info. Maybe I'm missing something?

I see how I could handle hashing but I don't see how I could handle the createConnection in
source/common/upstream/upstream_impl.cc properly without explicit visibility of the addresses?

I see. I suppose we could split the two since address() override seems more generic than CONNECT (you could do passthrough and all sorts of things with it), but it's up to you.

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

RyanTheOptimist

LGTM, though it looks like this needs a main merge. I'm also curious if the proxy info should be added to the TransportSocketOption's hashKey() computation?

source/common/upstream/upstream_impl.cc

RyanTheOptimist · 2022-06-30T14:00:19Z

source/extensions/transport_sockets/http_11_proxy/connect.cc

+}
+
+Network::IoResult UpstreamHttp11ConnectSocket::doWrite(Buffer::Instance& buffer, bool end_stream) {
+  if (header_buffer_.length() > 0) {


Oh, whoops. I missed that. Follow up is fine, or leave alone.

RyanTheOptimist · 2022-06-30T14:07:27Z

source/extensions/transport_sockets/http_11_proxy/connect.cc

+Network::IoResult UpstreamHttp11ConnectSocket::doRead(Buffer::Instance& buffer) {
+  if (!stripped_connect_) {
+    char peek_buf[200];
+    auto result = callbacks_->ioHandle().recv(peek_buf, 200, MSG_PEEK);


Ah! Looks like it's ABSL_ARRAYSIZE() now. but const is great.

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

alyssawilk · 2022-06-30T16:03:04Z

/wait on docs

kyessenov · 2022-06-30T16:19:41Z

test/integration/filters/header_to_proxy_filter.cc

+    if (!request_headers.get(connect_proxy).empty()) {
+      std::string address_string(request_headers.get(connect_proxy)[0]->value().getStringView());
+      auto address = Network::Utility::parseInternetAddressAndPort(address_string);
+      decoder_callbacks_->streamInfo().filterState()->setData(


I wonder if we could use LbEndpoint itself somehow to determine the proxy address. In our case, each endpoint has an assigned proxy, and the proxy address is inside the endpoint metadata. I'm not sure how we'd make it work with a downstream filter.

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

alyssawilk · 2022-07-07T17:40:42Z

yeah I was trying to stay quiet until we'd had a chance to test downstream for E-M and work out the kinks, but alpha is enough of a warning I suppose.

mattklein123 · 2022-07-07T17:45:15Z

api/envoy/extensions/transport_sockets/http_11_proxy/v3/upstream_http_11_connect.proto

+// If this is configured and an intermediate filter adds the data necessary for
+// proxying to the stream info then


What filter does this? How do I configure it? I guess from an end user perspective it's not clear to me what I need to do?

yeah this is why I didn't want to do genreal Envoy docs for it for it as it's written for a very narrow use case. I suspect it will get extended to be general purpose over time but right now, the goal is to provide the infrastructure for E-M sending data through the local android proxy (envoyproxy/envoy-mobile#1622) The E-M filter will query Android for the address and port of the proxy, then add that information to the stream info filter state so it can be used to redirect the connection and alter the payload going to the proxy.
I'm super happy to put these details in the proto but I've been repeatedly chided by @markdroth for putting too much Envoy information in what is supposed to be generic protos. Given this one is Envoy-only maybe it's OK to add more detail there? But this really isn't intended for generic Envoy use so much as Envoy Mobile. I could also land the proto and extension in E-M but then I can't test any of the changes to core Envoy required to make it all hang together. And realistically at some point someone's going to add config to make it usable since folks hate the loopback proxy solution. Happy to take your advice on how to proceed here or on slack.

OK I see. I don't know maybe then just make it a bit more clear in the docs that this is EM specific right now and can be extended later?

I get the concern about Envoy specific info in the protos, but IMO 1st priority is documentation. We need to sort out the per client docs separately which is a known issue.

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

mattklein123

Cool, thanks!

alyssawilk · 2022-07-07T21:01:23Z

/retest

repokitteh-read-only · 2022-07-07T21:01:28Z

Retrying Azure Pipelines:
Check envoy-presubmit isn't fully completed, but will still attempt retrying.
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #21942 (comment) was created by @alyssawilk.

see: more, trace.

moderation · 2022-07-12T01:51:49Z

Does this PR mean you can implement something like #11569 (comment) but without the need to use a loopback?

alyssawilk · 2022-07-12T13:11:25Z

Not yet, but it's possible to get there pretty easily. Right now the transport socket will CONNECT encapsulate for https only (HTTP proxies just need fully qualified URLs) and only if some intermediate filter tells it where to proxy to. We could add an optional configuration option so in the absence of said intermediate filter, it would proxy to some hard-coded location pretty easily. If you want it, lmk and I can add it.
For android we don't know the location up front so we want it added by a filter ,and I didn't want to add other use cases without users.

alyssawilk · 2022-07-12T13:11:52Z

Also only works for HTTP/1.1 if not blatantly obvious from naming. H2 is too complicatd to do inline so requires the loopback/internal-listener pass

kyessenov · 2022-07-12T16:40:43Z

@moderation I think adding some well-known metadata in endpoints would be an alternate way to find a tunnel address. That's what we've been doing for H2 tunneling with the internal listener. I think this is probably the right long term evolution away from internal listeners, but the problem is that transport sockets need to support multiplexing (1 to many for downstream, many to 1 for upstream), and right now they are basically one-to-one with TCP streams.

moderation · 2022-07-12T21:11:17Z

Thanks @alyssawilk and @kyessenov

- Update the ENVOY_COMMIT and ENVOY_SHA in [bazel/repositories.bzl](https://github.com/envoyproxy/nighthawk/blob/main/bazel/repositories.bzl) to the latest Envoy's commit. - Update the `CodecClientProd` instantiation to accommodate the recent changes introduced by envoyproxy/envoy#21942. - Temporarily disable `test_gcc` which fails the CI. See #872 for more details. Disclaimer: The CI pipeline was blocked due to a machine level breakage. All local tests passed. Signed-off-by: jiajunye [jiajunye@google.com](mailto:jiajunye@google.com)

Description: Integrate existing upstream Envoy support for proxies (envoyproxy/envoy#21942) with Android APIs that are responsible for getting the information about the configured proxy settings from user's device. There are a few things that we will need to address before we fully productionize this - they are listed in #2533. Risk Level: Medium, it's supposed to be a no-op if not enabled explicitly. Testing: Manual and integration Docs Changes: Done Release Notes: Done Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>

Description: Integrate existing upstream Envoy support for proxies (envoyproxy/envoy#21942) with Android APIs that are responsible for getting the information about the configured proxy settings from user's device. There are a few things that we will need to address before we fully productionize this - they are listed in envoyproxy#2533. Risk Level: Medium, it's supposed to be a no-op if not enabled explicitly. Testing: Manual and integration Docs Changes: Done Release Notes: Done Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com> Signed-off-by: Chidera Olibie <colibie@google.com>

Description: Integrate existing upstream Envoy support for proxies (#21942) with Android APIs that are responsible for getting the information about the configured proxy settings from user's device. There are a few things that we will need to address before we fully productionize this - they are listed in envoyproxy/envoy-mobile#2533. Risk Level: Medium, it's supposed to be a no-op if not enabled explicitly. Testing: Manual and integration Docs Changes: Done Release Notes: Done Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com> Signed-off-by: JP Simard <jp@jpsim.com>

Adding direct connect support.

e6d74a6

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

alyssawilk marked this pull request as ready for review June 29, 2022 14:31

repokitteh-read-only bot added the api label Jun 29, 2022

repokitteh-read-only bot assigned mattklein123 Jun 29, 2022

alyssawilk assigned RyanTheOptimist Jun 29, 2022

RyanTheOptimist reviewed Jun 29, 2022

View reviewed changes

kyessenov reviewed Jun 30, 2022

View reviewed changes

comments

ae206f0

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

RyanTheOptimist reviewed Jun 30, 2022

View reviewed changes

Merge branch 'main' into dfp

5298b63

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

repokitteh-read-only bot added the waiting label Jun 30, 2022

kyessenov reviewed Jun 30, 2022

View reviewed changes

theoretically fixing docs build

db9ff26

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

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

alyssawilk force-pushed the dfp branch from 8c17c33 to db9ff26 Compare June 30, 2022 17:47

format?

57b8b6a

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

mattklein123 added the waiting label Jul 1, 2022

fixes

ceec0fd

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

repokitteh-read-only bot removed the waiting label Jul 5, 2022

alyssawilk added 6 commits July 5, 2022 15:52

format

f0f6a0b

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

clang+coverage

d4f0844

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

tidy

2ec1b9a

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

Merge branch 'main' into dfp

82afcd2

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

coverage fix

cff9d17

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

coverage fix

9b078cd

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

RyanTheOptimist previously approved these changes Jul 6, 2022

View reviewed changes

alyssawilk mentioned this pull request Jul 7, 2022

Initiate HTTPConnect using Envoy #21363

Closed

alyssawilk added 2 commits July 7, 2022 13:35

comments

903c300

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

and relnote

c0bb05a

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

alyssawilk dismissed RyanTheOptimist’s stale review via c0bb05a July 7, 2022 17:40

repokitteh-read-only bot removed the waiting label Jul 7, 2022

mattklein123 reviewed Jul 7, 2022

View reviewed changes

comments

1d491f7

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

mattklein123 approved these changes Jul 7, 2022

View reviewed changes

repokitteh-read-only bot removed the api label Jul 7, 2022

alyssawilk enabled auto-merge (squash) July 7, 2022 21:00

alyssawilk merged commit 095f4ca into envoyproxy:main Jul 11, 2022

jiajunye mentioned this pull request Jul 12, 2022

Update Envoy to 0f0f675 (Jul 11th 2022) envoyproxy/nighthawk#869

Merged

kyessenov mentioned this pull request Jul 13, 2022

original_dst_cluster: add override by filter state #22155

Merged

Augustyniak mentioned this pull request Aug 5, 2022

proxy: add android proxy monitor envoyproxy/envoy-mobile#2416

Merged

Augustyniak mentioned this pull request Sep 8, 2022

android: implement support for proxies envoyproxy/envoy-mobile#2533

Open

		// If this is configured and an intermediate filter adds the data necessary for
		// proxying to the stream info then

Conversation

alyssawilk commented Jun 29, 2022

Uh oh!

repokitteh-read-only bot commented Jun 29, 2022

Uh oh!

repokitteh-read-only bot commented Jun 29, 2022

Uh oh!

RyanTheOptimist left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

RyanTheOptimist left a comment

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!

alyssawilk commented Jun 30, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alyssawilk commented Jul 7, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alyssawilk Jul 7, 2022 • 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!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

alyssawilk commented Jul 7, 2022

Uh oh!

repokitteh-read-only bot commented Jul 7, 2022

Uh oh!

moderation commented Jul 12, 2022

Uh oh!

alyssawilk commented Jul 12, 2022

Uh oh!

alyssawilk commented Jul 12, 2022

Uh oh!

kyessenov commented Jul 12, 2022

Uh oh!

moderation commented Jul 12, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

alyssawilk Jul 7, 2022 •

edited

Loading