router: adding CONNECT support to upstreams (not prod-ready) #10623

alyssawilk · 2020-04-02T12:41:15Z

CONNECT won't be very useful without proxy protocol support, and needs some pausing for security reasons, both coming soon.

Risk Level: Medium (router refactor)
Testing: unit tests and integration tests
Docs Changes: some
Release Notes: n/a (still hidden)
Part of #1630 #1451

repokitteh-read-only · 2020-04-02T12:41:20Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #10623 was opened by alyssawilk.

see: more, trace.

PiotrSikora · 2020-04-02T17:23:08Z

cc @lambdai

Signed-off-by: Alyssa Wilk <[email protected]>

alyssawilk · 2020-04-02T20:05:35Z

@snowp or @ggreenway would one of you be up for doing first pass on this? Kind of crosses the line between TCP and upstream

snowp

Haven't looked at the tests yet, just some initial comments/qs on the impl

snowp · 2020-04-02T20:07:56Z

api/envoy/api/v2/route/route_components.proto

+  // [#not-implemented-hide:]
+  // Configuration for sending data upstream as a raw data payload. This is used for
+  // CONNECT requests, when translating HTTP body data to TCP payload.
+  message ProxyingConfig {


ProxyConfig maybe? or maybe HttpConnectConfig if this is specific to CONNECT?

api/envoy/api/v2/route/route_components.proto

snowp · 2020-04-02T20:16:44Z

include/envoy/router/router.h

+   * @return configuration for TCP proxying, if present.
+   */
+  using ProxyingConfig = envoy::config::route::v3::VirtualHost::ProxyingConfig;
+  virtual const absl::optional<ProxyingConfig> proxyingConfig() const PURE;


return by const ref?

snowp · 2020-04-02T20:17:21Z

include/envoy/router/router.h

+   * @return configuration for TCP proxying, if present.
+   */
+  using ProxyingConfig = envoy::config::route::v3::VirtualHost::ProxyingConfig;
+  virtual const absl::optional<ProxyingConfig> proxyingConfig() const PURE;


source/common/router/router.cc

source/common/router/upstream_request.cc

snowp · 2020-04-02T20:30:31Z

source/common/router/upstream_request.cc

+}
+
+void TcpUpstream::onUpstreamData(Buffer::Instance& data, bool end_stream) {
+  if (!upstream_request_) {


should this ever happen? do we end up just dropping data in this case?

Yeah seems odd that we would ever get into this state?

source/common/router/router.cc

alyssawilk · 2020-04-06T15:43:16Z

Also when you get around to tests, note I could only unit test this one - integration tests are over in master...alyssawilk:shinkansen_next

Enabling CONNECT has enough security impact and weird HTTP corner cases I wanted to split the upstream and downstream PRs out, and we can't integration test until both halves land

Signed-off-by: Alyssa Wilk <[email protected]>

mattklein123

Super awesome to see this getting close. This has been a very long requested feature. I have some API/config comments to get started with. Thank you!

/wait

api/envoy/api/v2/route/route_components.proto

source/common/router/router.cc

htuch · 2020-04-07T14:50:32Z

Please merge master to pick up #10672. We no longer accept changes to v2 (without explicit exception), so any API modifications should happen in v3. If this PR is adding a new proto, please follow the updated instructions in https://github.com/envoyproxy/envoy/blob/master/api/STYLE.md#adding-an-extension-configuration-to-the-api.

Signed-off-by: Alyssa Wilk <[email protected]>

alyssawilk · 2020-04-08T19:27:14Z

So naively, I'd assume I could pull master, revert my changes to api/envoy/api/v2/route/route_components.proto / generated_api_shadow/envoy/api/v2/route/route_components.proto
run ./tools/proto_format/proto_format.sh fix
and things would work.

In practice I get

multiprocessing.pool.RemoteTraceback:
"""
Traceback (most recent call last):
File "/usr/lib/python3.7/multiprocessing/pool.py", line 121, in worker
result = (True, func(*args, **kwds))
File "/usr/lib/python3.7/multiprocessing/pool.py", line 44, in mapstar
return list(map(*args))
File "./tools/proto_format/proto_sync.py", line 175, in SyncProtoFile
MergeActiveShadow(active_src, shadow_srcs[0], f.name)
File "./tools/proto_format/proto_sync.py", line 141, in MergeActiveShadow
dst,
File "/usr/lib/python3.7/subprocess.py", line 411, in check_output
**kwargs).stdout
File "/usr/lib/python3.7/subprocess.py", line 512, in run
output=stdout, stderr=stderr)
subprocess.CalledProcessError: Command '['bazel-bin/tools/protoxform/merge_active_shadow', 'bazel-bin/external/envoy_api_canonical/envoy/config/route/v3/pkg/envoy/config/route/v3/route_components.proto.active_or_frozen.proto', 'bazel-bin/external/envoy_api_canonical/envoy/api/v2/route/pkg/envoy/api/v2/route/route_components.proto.next_major_version_candidate.envoy_internal.proto', '/tmp/tmpycuwl_o4']' returned non-zero exit status 1.
"""

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
File "./tools/proto_format/proto_sync.py", line 402, in
Sync(args.api_shadow_root, args.mode, args.labels, True)
File "./tools/proto_format/proto_sync.py", line 344, in Sync
pkg_deps = p.map(SyncProtoFile, dst_src_paths.items())
File "/usr/lib/python3.7/multiprocessing/pool.py", line 268, in map
return self._map_async(func, iterable, mapstar, chunksize).get()
File "/usr/lib/python3.7/multiprocessing/pool.py", line 657, in get
raise self._value
subprocess.CalledProcessError: Command '['bazel-bin/tools/protoxform/merge_active_shadow', 'bazel-bin/external/envoy_api_canonical/envoy/config/route/v3/pkg/envoy/config/route/v3/route_components.proto.active_or_frozen.proto', 'bazel-bin/external/envoy_api_canonical/envoy/api/v2/route/pkg/envoy/api/v2/route/route_components.proto.next_major_version_candidate.envoy_internal.proto', '/tmp/tmpycuwl_o4']' returned non-zero exit status 1.

If I just leave things as is, it builds happily, with the changes to v2 intact.
Suggestions?

Signed-off-by: Alyssa Wilk <[email protected]>

alyssawilk · 2020-04-13T13:34:29Z

Sorry, having looked at this, do we want something more than the existing option of using a header matcher with
{
"name": ":method",
"exact_match": "CONNECT"
}
As you say CONNECT is a special case, but given we already have this functionality I'd be inclined to call it out in the docs, rather than add a one off. Thoughts?

Signed-off-by: Alyssa Wilk <[email protected]>

alyssawilk · 2020-04-13T14:15:50Z

@htuch if I apply the diffs to api/envoy/config/route/v3/route_components.proto and run fix_proto from clean master, and bazel test //test/common/router:all the build seg-faults.

Have the new tools for freezing been tested for field additions? Is there some state I should be removing? bazel clean --expunge doesn't do it and I'm not sure what else to do from a clean branch.

htuch · 2020-04-13T14:59:35Z

api/envoy/api/v2/route/route_components.proto

@@ -34,7 +34,7 @@ option (udpa.annotations.file_status).package_version_status = FROZEN;
 // host header. This allows a single listener to service multiple top level domain path trees. Once
 // a virtual host is selected based on the domain, the routes are processed in order to see which
 // upstream cluster to route to or whether to perform a redirect.
-// [#next-free-field: 21]
+// [#next-free-field: 23]


No more changes allowed to v2, it's frozen :)

I understand that.
I reverted them and my build seg-faulted. I can't get it to build without the changes.

pastebin of

copying v3 changes into a clean build

running fix_proto

showing the expected files are changed

building tests fails

https://pastebin.com/N8vSXHQM

htuch · 2020-04-13T15:00:43Z

@alyssawilk is your master at HEAD on GitHub? I landed some fixes late last week. If not, please push a branch somewhere and I can investigate.

Signed-off-by: Alyssa Wilk <[email protected]>

alyssawilk · 2020-04-20T16:15:42Z

sigh. clang_tidy caught a missing override but enough of the presubmits were fine I think this is ready for another look while CI churns

Signed-off-by: Alyssa Wilk <[email protected]>

alyssawilk · 2020-04-21T19:50:40Z

Oh yeah, and what with #10720 landing I turned up the integration tests (modernized to the new config, and host:port requirement)

Signed-off-by: Alyssa Wilk <[email protected]>

mattklein123

Thanks this is epic. Super excited to see all of this land. Just some high level questions but looks great. Very clean and easy to follow.

/wait

mattklein123 · 2020-04-21T21:12:23Z

source/common/router/config_impl.cc

+void ConnectRouteEntryImpl::rewritePathHeader(Http::RequestHeaderMap& headers,
+                                              bool insert_envoy_original_path) const {
+  const absl::string_view path =
+      Http::PathUtil::removeQueryAndFragment(headers.Path()->value().getStringView());


We need to land the other PR for this to be safe, right? As an aside, any thoughts on making it programatically impossible to mess this up? Now that we split the header map APIs, should we special case Path so it returns an optional or something like that? WDYT? cc @asraa

It might also be good to split the config_impl changes/tests out from the rest of this PR, but up to you.

We don't, because the HCM still adds the synthetic "/" path header.

Regarding impossible to mess up (which I could do with a follow-up after this and before removing the synthetic path header, I think if we wanted to do that we might want to do it with a bunch of the "expected but not guaranteed" headers. My preference would be to have a filter fuzzer / filter compliance suite where every filter is hit with pathless requests, but also other common unusual scenarios like hostless responses.

Yes this would be great, agreed. I think we are going to have a very common class of bugs where filters thing that certain headers will always be set which we are now breaking.

source/common/router/router.cc

mattklein123 · 2020-04-21T21:22:23Z

source/common/router/upstream_request.cc

+}
+
+void TcpUpstream::onUpstreamData(Buffer::Instance& data, bool end_stream) {
+  if (!upstream_request_) {


Yeah seems odd that we would ever get into this state?

mattklein123 · 2020-04-21T21:33:41Z

docs/root/intro/arch_overview/http/upgrades.rst

+.. not have a path, and can only be matched using a
+.. :ref:`connect_matcher <envoy_api_field_route.RouteMatch.connect_matcher>`
+..
+.. Envoy can handle CONNECT in one of two ways, either proxying the CONNECT headers through as if they


For my own understanding, if we proxy CONNECT through, this is really no different from WebSocket, is that right?

Yeah, in that case it's just a blind upgrade where we proxy payload. Want me to comment something to that effect?

From reading this it sounds to me like if we proxy CONNECT we don't do anything special and just treat it like any other request? I think it could be clearer about what happens after the CONNECT is proxied/established.

I think it might also be useful to explain whether we support terminating the CONNECT and dropping down to L4 proxying. I don't think we do (?) but I think it's a use case people might be interested in and it might be worth calling out what our support is there if we don't already.

Yeah I think some clarifications would be helpful to explain the differences between WS/CONNECT and termination vs. pass through. With this change terminating WS would be pretty trivial so that could be a future enhancement for someone (might be worth tracking in an issue).

I think I may have been steeped in the code too long but I'm not sure how I could make this more clear, but I think it's clear from Snow's comment that it's not as clear as I want it to be.

For connect we can basically treat it like normal L7 request, forwarding request headers and payload through, or we can terminate, strip connect headers, and forward the CONNECT payload on a raw TCP connection. I don't know how to explain that more clearly than I am doing so - I'll try for a little rewording in my next push but I'd totally appreciate any help making it clear what the options are.

I think this is fine and we can ship and iterate.

mattklein123 · 2020-04-21T21:35:22Z

source/common/router/upstream_request.h

@@ -259,5 +294,29 @@ class HttpUpstream : public GenericUpstream, public Http::StreamCallbacks {
  Http::RequestEncoder* request_encoder_{};
 };

+class TcpUpstream : public GenericUpstream, public Tcp::ConnectionPool::UpstreamCallbacks {


Which timeouts come into play with a CONNECT/WS session? I guess the downstream idle timeouts? Anything else? I think we will need to make sure that #10531 works with non-pass through CONNECT also? WDYT? cc @Shikugawa

I haven't reviewed the tests yet but it would be great to make sure we have good integration coverage over all of the various timeout scenarios.

All HTTP timeouts may apply - we're 100% reusing the classes which do timeout logic.
Presumably one would want larger (or no) overall timeouts on WS / CONNECT routes.
Given the code reuse I did one timeout test to make sure that disconnects worked. I can do the full suite if you'd prefer?

I don't think you have to do them all I just wanted to make sure we are covered. @Shikugawa can make sure to cover CONNECT when the linked PR is finished for idle timeouts.

mattklein123 · 2020-04-21T21:38:26Z

source/common/router/router.cc


-  if (http_pool) {
-    host = http_pool->host();
+  if (!should_tcp_proxy) {


Do we need any stats for the CONNECT cases, both termination and pass through? Any other missing stats? Fine to do this stuff in a follow up but might be worth a TODO for this type of stuff. I assume also that all of the circuit breakers are built into the TCP connection pool so we are good there?

We've got general upgrade stats, but I didn't split out CONNECT or differentiate connect upgrade from connect termination. Have thoughts on what folks would use and I can add them in a follow up?

Ultimately I think having some stats specific to CONNECT/WS might be useful, but I agree we can see what people want later.

Signed-off-by: Alyssa Wilk <[email protected]>

source/common/router/router.cc

snowp · 2020-04-22T18:04:34Z

docs/root/intro/arch_overview/http/upgrades.rst

+.. CONNECT support
+.. ^^^^^^^^^^^^^^^
+
+.. Envoy CONNECT support is off by default (Envoy will send an internall generated 403 in response to


typo: internall -> internally

snowp · 2020-04-22T18:12:28Z

docs/root/intro/arch_overview/http/upgrades.rst

+.. not have a path, and can only be matched using a
+.. :ref:`connect_matcher <envoy_api_field_route.RouteMatch.connect_matcher>`
+..
+.. Envoy can handle CONNECT in one of two ways, either proxying the CONNECT headers through as if they


From reading this it sounds to me like if we proxy CONNECT we don't do anything special and just treat it like any other request? I think it could be clearer about what happens after the CONNECT is proxied/established.

I think it might also be useful to explain whether we support terminating the CONNECT and dropping down to L4 proxying. I don't think we do (?) but I think it's a use case people might be interested in and it might be worth calling out what our support is there if we don't already.

Signed-off-by: Alyssa Wilk <[email protected]>

Split out from #10623 Signed-off-by: Alyssa Wilk <[email protected]>

Signed-off-by: Alyssa Wilk <[email protected]>

alyssawilk

OK, merged in the connect matcher, which makes this PR much simpler.
Still outstanding but TODO in follow-ups

figure out what to do with pathless-connect for safety (current plan is improved fuzzing with Asra is on) and removing the synthetic path
proxy proto (written and tested - will mail once this lands)
pause-before sending upgrade (written and testing - ditto)

I think this PR has addressed all outstanding comments modulo the docs maybe not being clear enough on the two options (proxying vs terminating/L4-downgrade). If folks are amenable I'd love to land this and iterate on docs since they're hidden but we can do them in place too.

mattklein123

Thanks, epic. LGTM to ship and iterate. Feel free to action on my comments in follow ups if you want.

mattklein123 · 2020-04-27T22:33:47Z

docs/root/intro/arch_overview/http/upgrades.rst

+.. not have a path, and can only be matched using a
+.. :ref:`connect_matcher <envoy_api_field_route.RouteMatch.connect_matcher>`
+..
+.. Envoy can handle CONNECT in one of two ways, either proxying the CONNECT headers through as if they


I think this is fine and we can ship and iterate.

mattklein123 · 2020-04-27T22:34:05Z

source/common/router/router.cc

@@ -655,6 +662,38 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::RequestHeaderMap& headers,
  return Http::FilterHeadersStatus::StopIteration;
 }

+Filter::HttpOrTcpPool Filter::createConnPool(Upstream::HostDescriptionConstSharedPtr& host) {
+  Filter::HttpOrTcpPool conn_pool;
+  bool should_tcp_proxy = route_entry_->connectConfig().has_value() &&


mattklein123 · 2020-04-27T22:36:20Z

source/common/router/upstream_request.h

+    }
+    return false;
+  }
+  absl::optional<Http::Protocol> protocol() const override { return absl::nullopt; }


Is this used for logging? Should we populate this? I guess there is the question of upstream access logging for terminated connect requests? Maybe a TODO for this? cc @kyessenov

yeah, it goes in the streaminfo. IMO if we're doing raw L4 proxying upstream the HTTP based protocol should not be set, since we're not doing HTTP/1 or HTTP/2. Thoughts?

I would recommend briefly chatting with @kyessenov about this. He has been trying to normalize all of this so we support other protocols including TCP, etc. so I think we should probably have some indication in the logs of what is happening here. Definitely fine to TODO/issue after you determine the best future course of action.

addressed I think

alyssawilk · 2020-04-28T13:39:34Z

Sweet, will address comments in the next one :-)

alyssawilk assigned mattklein123 Apr 2, 2020

repokitteh-read-only bot added the api label Apr 2, 2020

alyssawilk added 2 commits April 2, 2020 14:16

router: adding CONNECT support

0cb59af

Signed-off-by: Alyssa Wilk <[email protected]>

decapsulating *is* a word

7f134d1

Signed-off-by: Alyssa Wilk <[email protected]>

alyssawilk force-pushed the upstream_connect branch from d6d253f to 7f134d1 Compare April 2, 2020 18:17

sigh

f1fe955

Signed-off-by: Alyssa Wilk <[email protected]>

snowp suggested changes Apr 2, 2020

View reviewed changes

lizan requested a review from lambdai April 3, 2020 09:19

lambdai reviewed Apr 5, 2020

View reviewed changes

source/common/router/router.cc Outdated Show resolved Hide resolved

alyssawilk assigned snowp Apr 6, 2020

reviewer comments

6ddcb00

Signed-off-by: Alyssa Wilk <[email protected]>

mattklein123 requested changes Apr 6, 2020

View reviewed changes

api/envoy/api/v2/route/route_components.proto Outdated Show resolved Hide resolved

source/common/router/router.cc Outdated Show resolved Hide resolved

repokitteh-read-only bot added the waiting label Apr 6, 2020

repokitteh-read-only bot removed the waiting label Apr 8, 2020

hidden docs

21986a9

Signed-off-by: Alyssa Wilk <[email protected]>

alyssawilk force-pushed the upstream_connect branch from ff118b5 to 21986a9 Compare April 8, 2020 17:38

Merge branch 'master' into upstream_connect

e8de80b

Signed-off-by: Alyssa Wilk <[email protected]>

Merge branch 'master' into upstream_connect

0729b25

Signed-off-by: Alyssa Wilk <[email protected]>

alyssawilk mentioned this pull request Apr 9, 2020

http: downstream connect support #10720

Merged

Merge branch 'master' into upstream_connect

500c4e5

Signed-off-by: Alyssa Wilk <[email protected]>

htuch reviewed Apr 13, 2020

View reviewed changes

fixing build rule

ac38de1

Signed-off-by: Alyssa Wilk <[email protected]>

repokitteh-read-only bot removed waiting api labels Apr 20, 2020

alyssawilk added 3 commits April 20, 2020 12:19

tidier

9a82f12

Signed-off-by: Alyssa Wilk <[email protected]>

Merge branch 'master' into upstream_connect

c3193d2

Signed-off-by: Alyssa Wilk <[email protected]>

integration tests!

92d833d

Signed-off-by: Alyssa Wilk <[email protected]>

repokitteh-read-only bot added the api label Apr 21, 2020

alyssawilk changed the title ~~router: adding CONNECT support to upstreams (not yet usable)~~ router: adding CONNECT support to upstreams (not prod-ready) Apr 21, 2020

Merge branch 'refs/heads/master' into upstream_connect

4434593

Signed-off-by: Alyssa Wilk <[email protected]>

mattklein123 requested changes Apr 21, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Apr 21, 2020

alyssawilk mentioned this pull request Apr 22, 2020

config: adding connect matcher (unused) #10894

Merged

reviewer comments

8363d5a

Signed-off-by: Alyssa Wilk <[email protected]>

repokitteh-read-only bot removed the waiting label Apr 22, 2020

Merge branch 'master' into upstream_connect

394c387

Signed-off-by: Alyssa Wilk <[email protected]>

snowp previously requested changes Apr 22, 2020

View reviewed changes

docs rewording

72b292a

Signed-off-by: Alyssa Wilk <[email protected]>

mattklein123 pushed a commit that referenced this pull request Apr 23, 2020

config: adding connect matcher (unused) (#10894)

09bf9a1

Split out from #10623 Signed-off-by: Alyssa Wilk <[email protected]>

jsanford-toast mentioned this pull request Apr 23, 2020

config: adding connect matcher (unused) (#10894) jsanford-toast/envoy#1

Closed

alyssawilk mentioned this pull request Apr 27, 2020

Add CONNECT support #1451

Closed

Merge branch 'master' into upstream_connect

e44bfbe

Signed-off-by: Alyssa Wilk <[email protected]>

alyssawilk commented Apr 27, 2020

View reviewed changes

mattklein123 approved these changes Apr 27, 2020

View reviewed changes

alyssawilk merged commit c46c0a2 into envoyproxy:master Apr 28, 2020

irozzo-1A mentioned this pull request Nov 9, 2020

Initial support for upstream HTTP/1.1 tunneling #13293

Merged

thesuperzapper mentioned this pull request Nov 2, 2023

unable to upgrade websocket connection when HTTP 1.1 upgraded to HTTP2 istio/istio#27078

Closed

router: adding CONNECT support to upstreams (not prod-ready) #10623

router: adding CONNECT support to upstreams (not prod-ready) #10623

Conversation

alyssawilk commented Apr 2, 2020 • edited Loading

repokitteh-read-only bot commented Apr 2, 2020

PiotrSikora commented Apr 2, 2020

alyssawilk commented Apr 2, 2020

snowp 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

alyssawilk commented Apr 6, 2020

mattklein123 left a comment

Choose a reason for hiding this comment

htuch commented Apr 7, 2020

alyssawilk commented Apr 8, 2020

alyssawilk commented Apr 13, 2020

alyssawilk commented Apr 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

htuch commented Apr 13, 2020

alyssawilk commented Apr 20, 2020

alyssawilk commented Apr 21, 2020

mattklein123 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

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 Apr 22, 2020 • edited Loading

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 left a comment • edited Loading

Choose a reason for hiding this comment

mattklein123 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

alyssawilk commented Apr 28, 2020

alyssawilk commented Apr 2, 2020 •

edited

Loading

alyssawilk Apr 22, 2020 •

edited

Loading

alyssawilk left a comment •

edited

Loading