Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
alyssawilk merged 26 commits into from
Apr 28, 2020
Merged

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

Show file tree
Hide file tree
Changes from 24 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
0cb59af
router: adding CONNECT support
alyssawilk Apr 1, 2020
7f134d1
decapsulating *is* a word
alyssawilk Apr 2, 2020
f1fe955
sigh
alyssawilk Apr 2, 2020
6ddcb00
reviewer comments
alyssawilk Apr 6, 2020
21986a9
hidden docs
alyssawilk Apr 8, 2020
e8de80b
Merge branch 'master' into upstream_connect
alyssawilk Apr 8, 2020
0729b25
Merge branch 'master' into upstream_connect
alyssawilk Apr 9, 2020
500c4e5
Merge branch 'master' into upstream_connect
alyssawilk Apr 13, 2020
0286031
Matcher API?
alyssawilk Apr 13, 2020
771c1a6
WIP matcher API use
alyssawilk Apr 13, 2020
b83f9bd
API WIP
alyssawilk Apr 15, 2020
f56e2f0
cleanup
alyssawilk Apr 16, 2020
1f2e9e5
Merge branch 'master' into upstream_connect
alyssawilk Apr 16, 2020
9587888
Merge branch 'master' into upstream_connect
alyssawilk Apr 16, 2020
67a753d
cleanup and tests of new API
alyssawilk Apr 16, 2020
795231f
finally got the config impl test working
alyssawilk Apr 16, 2020
a847f91
default return
alyssawilk Apr 20, 2020
ac38de1
fixing build rule
alyssawilk Apr 20, 2020
9a82f12
tidier
alyssawilk Apr 20, 2020
c3193d2
Merge branch 'master' into upstream_connect
alyssawilk Apr 21, 2020
92d833d
integration tests!
alyssawilk Apr 21, 2020
4434593
Merge branch 'refs/heads/master' into upstream_connect
alyssawilk Apr 21, 2020
8363d5a
reviewer comments
alyssawilk Apr 22, 2020
394c387
Merge branch 'master' into upstream_connect
alyssawilk Apr 22, 2020
72b292a
docs rewording
alyssawilk Apr 22, 2020
e44bfbe
Merge branch 'master' into upstream_connect
alyssawilk Apr 27, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 28 additions & 1 deletion api/envoy/config/route/v3/route_components.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -370,7 +370,7 @@ message WeightedCluster {
string runtime_key_prefix = 2;
}

// [#next-free-field: 12]
// [#next-free-field: 13]
message RouteMatch {
option (udpa.annotations.versioning).previous_message_type = "envoy.api.v2.route.RouteMatch";

Expand All @@ -392,6 +392,11 @@ message RouteMatch {
google.protobuf.BoolValue validated = 2;
}

// [#not-implemented-hide:]
// An extensible message for matching CONNECT requests.
message ConnectMatcher {
}

reserved 5, 3;

reserved "regex";
Expand Down Expand Up @@ -420,6 +425,16 @@ message RouteMatch {
// on :path, etc. The issue with that is it is unclear how to generically deal with query string
// stripping. This needs more thought.]
type.matcher.v3.RegexMatcher safe_regex = 10 [(validate.rules).message = {required: true}];

// [#not-implemented-hide:]
// If this is used as the matcher, the matcher will only match CONNECT requests.
// Note that this will not match HTTP/2 upgrade-style CONNECT requests
// (WebSocket and the like) as they are normalized in Envoy as HTTP/1.1 style
// upgrades.
// This is the only way to match CONNECT requests for HTTP/1.1. For HTTP/2,
// where CONNECT requests may have a path, the path matchers will work if
// there is a path present.
ConnectMatcher connect_matcher = 12;
}

// Indicates that prefix/path matching should be case insensitive. The default
Expand Down Expand Up @@ -705,6 +720,13 @@ message RouteAction {
option (udpa.annotations.versioning).previous_message_type =
"envoy.api.v2.route.RouteAction.UpgradeConfig";

// [#not-implemented-hide:]
// Configuration for sending data upstream as a raw data payload. This is used for
// CONNECT requests, when forwarding CONNECT payload as raw TCP.
message ConnectConfig {
// TODO(alyssawilk) add proxy proto configuration here.
}

// The case-insensitive name of this upgrade, e.g. "websocket".
// For each upgrade type present in upgrade_configs, requests with
// Upgrade: [upgrade_type] will be proxied upstream.
Expand All @@ -713,6 +735,11 @@ message RouteAction {

// Determines if upgrades are available on this route. Defaults to true.
google.protobuf.BoolValue enabled = 2;

// [#not-implemented-hide:]
// Configuration for sending data upstream as a raw data payload. This is used for
// CONNECT requests, when forwarding CONNECT payload as raw TCP.
ConnectConfig connect_config = 3;
}

reserved 12, 18, 19, 16, 22, 21, 10;
Expand Down
34 changes: 33 additions & 1 deletion api/envoy/config/route/v4alpha/route_components.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -371,7 +371,7 @@ message WeightedCluster {
string runtime_key_prefix = 2;
}

// [#next-free-field: 12]
// [#next-free-field: 13]
message RouteMatch {
option (udpa.annotations.versioning).previous_message_type = "envoy.config.route.v3.RouteMatch";

Expand All @@ -393,6 +393,13 @@ message RouteMatch {
google.protobuf.BoolValue validated = 2;
}

// [#not-implemented-hide:]
// An extensible message for matching CONNECT requests.
message ConnectMatcher {
option (udpa.annotations.versioning).previous_message_type =
"envoy.config.route.v3.RouteMatch.ConnectMatcher";
}

reserved 5, 3;

reserved "regex";
Expand Down Expand Up @@ -421,6 +428,16 @@ message RouteMatch {
// on :path, etc. The issue with that is it is unclear how to generically deal with query string
// stripping. This needs more thought.]
type.matcher.v3.RegexMatcher safe_regex = 10 [(validate.rules).message = {required: true}];

// [#not-implemented-hide:]
// If this is used as the matcher, the matcher will only match CONNECT requests.
// Note that this will not match HTTP/2 upgrade-style CONNECT requests
// (WebSocket and the like) as they are normalized in Envoy as HTTP/1.1 style
// upgrades.
// This is the only way to match CONNECT requests for HTTP/1.1. For HTTP/2,
// where CONNECT requests may have a path, the path matchers will work if
// there is a path present.
ConnectMatcher connect_matcher = 12;
}

// Indicates that prefix/path matching should be case insensitive. The default
Expand Down Expand Up @@ -706,6 +723,16 @@ message RouteAction {
option (udpa.annotations.versioning).previous_message_type =
"envoy.config.route.v3.RouteAction.UpgradeConfig";

// [#not-implemented-hide:]
// Configuration for sending data upstream as a raw data payload. This is used for
// CONNECT requests, when forwarding CONNECT payload as raw TCP.
message ConnectConfig {
// TODO(alyssawilk) add proxy proto configuration here.

option (udpa.annotations.versioning).previous_message_type =
"envoy.config.route.v3.RouteAction.UpgradeConfig.ConnectConfig";
}

// The case-insensitive name of this upgrade, e.g. "websocket".
// For each upgrade type present in upgrade_configs, requests with
// Upgrade: [upgrade_type] will be proxied upstream.
Expand All @@ -714,6 +741,11 @@ message RouteAction {

// Determines if upgrades are available on this route. Defaults to true.
google.protobuf.BoolValue enabled = 2;

// [#not-implemented-hide:]
// Configuration for sending data upstream as a raw data payload. This is used for
// CONNECT requests, when forwarding CONNECT payload as raw TCP.
ConnectConfig connect_config = 3;

@mattklein123 mattklein123 Apr 15, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this came up in Slack, but would this make more sense as part of this oneof in Route?

envoy/api/envoy/config/route/v3/route_components.proto

Lines 209 to 224 in 2413b83

oneof action {
option (validate.required) = true;
// Route request to some upstream cluster.
RouteAction route = 2;
// Return a redirect.
RedirectAction redirect = 3;
// Return an arbitrary HTTP response directly, without proxying.
DirectResponseAction direct_response = 7;
// [#not-implemented-hide:]
// If true, a filter will define the action (e.g., it could dynamically generate the
// RouteAction).
FilterAction filter_action = 17;
.

I can see the logical benefit of it being part of RouteAction, but many of the fields don't apply (some do like hash policy I guess). I think this is fine but I just want to make sure we are all on the same page. cc @htuch

@mattklein123 mattklein123 Apr 15, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, if connect_config is not specified (wherever it ends up) the connect will be refused, right? I would clarify that.

@htuch htuch Apr 16, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with this being called ConnectAction and placed here. I thought that @alyssawilk mentioned that there is some overlapping state between this and RouteAction, it would be good to not duplicate too much of this is a significant overlap.

@alyssawilk alyssawilk Apr 16, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I had a note to talk to you about that then opted to take it to the PATH pr, since it's not relevant until we remove the synthetic path.
HTTP/2 allows connect-with-path. If there's connect with PATH should we allow the connect matchers to match it or still refuse?
It's the difference between "CONNECT requests will not be matched" on the other matchers and "requests without a PATH (such as HTTP/1 CONNECT requests) will not be matched.

@mattklein123 mattklein123 Apr 16, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CONNECT requests will not be matched" on the other matchers

This is how I assumed it would work. Meaning, if h2 supports connect with path we can still do a header match on path, but by having the connect matcher it is much more intentional what is being matched. I'm still willing to be swayed that this split does not make sense, but given how much of a special case CONNECT is and all of the inherent security implications IMO it's not a bad split to have. wdyt?

@alyssawilk alyssawilk Apr 16, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I was much of the same mind - CONNECT is so dangerous I'm super twitchy about enabling folks to screw themselves up. That said, CONNECT doesn't work by default, and you have to configure it 2 places for HTTP/2 so I think if we comment it would be Ok to allow patch matches. I'd prefer to only do it if folks need it, but given the 2 year deprecation dance I guess we have to guess. Allowing it does mean we couldn't validate that we only allow ConnectConfig with connect matchers, since then any matcher could work as a connect matcher, but still the most dangerous form of connect (terminating and forwarding payload) would only happen on routes which explicitly have ConnectConfig, so there's enough checks I think it'd be hard to do it by accident.

@alyssawilk alyssawilk Apr 16, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically I could go either way, but if there's a path and 3 guards I guess I'm inclined to let the patch matchers work.

@alyssawilk alyssawilk Apr 16, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(though while folks are thinking about the pros and cons, it's a bit weird that a .* regex matcher wouldn't match if there's no path header. I think as long as we comment explicitly that they only work with Path present it's OK.)

}

reserved 12, 18, 19, 16, 22, 21, 10;
Expand Down
2 changes: 1 addition & 1 deletion docs/root/intro/arch_overview/http/http.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,5 +7,5 @@ HTTP
http_connection_management
http_filters
http_routing
websocket
upgrades
http_proxy
43 changes: 35 additions & 8 deletions ...ot/intro/arch_overview/http/websocket.rst → ...oot/intro/arch_overview/http/upgrades.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,19 +1,19 @@
.. _arch_overview_websocket:

WebSocket and HTTP upgrades
HTTP upgrades
===========================

Envoy Upgrade support is intended mainly for WebSocket but may be used for non-WebSocket
upgrades as well. Upgrades pass both the HTTP headers and the upgrade payload
Envoy Upgrade support is intended mainly for WebSocket and CONNECT support, but may be used for
arbitrary upgrades as well. Upgrades pass both the HTTP headers and the upgrade payload
through an HTTP filter chain. One may configure the
:ref:`upgrade_configs <envoy_api_field_config.filter.network.http_connection_manager.v2.HttpConnectionManager.upgrade_configs>`
with or without custom filter chains. If only the
:ref:`upgrade_type <envoy_api_field_config.filter.network.http_connection_manager.v2.HttpConnectionManager.UpgradeConfig.upgrade_type>`
is specified, both the upgrade headers, any request and response body, and WebSocket payload will
is specified, both the upgrade headers, any request and response body, and HTTP data payload will
pass through the default HTTP filter chain. To avoid the use of HTTP-only filters for upgrade payload,
one can set up custom
:ref:`filters <envoy_api_field_config.filter.network.http_connection_manager.v2.HttpConnectionManager.UpgradeConfig.filters>`
for the given upgrade type, up to and including only using the router filter to send the WebSocket
for the given upgrade type, up to and including only using the router filter to send the HTTP
data upstream.

Upgrades can be enabled or disabled on a :ref:`per-route <envoy_api_field_route.RouteAction.upgrade_configs>` basis.
Expand All @@ -32,12 +32,12 @@ laid out below, but custom filter chains can only be configured on a per-HttpCon
| F | F | F |
+-----------------------+-------------------------+-------------------+

Note that the statistics for upgrades are all bundled together so WebSocket
Note that the statistics for upgrades are all bundled together so WebSocket and other upgrades
:ref:`statistics <config_http_conn_man_stats>` are tracked by stats such as
downstream_cx_upgrades_total and downstream_cx_upgrades_active

Handling HTTP/2 hops
^^^^^^^^^^^^^^^^^^^^
Websocket over HTTP/2 hops
^^^^^^^^^^^^^^^^^^^^^^^^^^

While HTTP/2 support for WebSockets is off by default, Envoy does support tunneling WebSockets over
HTTP/2 streams for deployments that prefer a uniform HTTP/2 mesh throughout; this enables, for example,
Expand All @@ -61,3 +61,30 @@ a GET method on the final Envoy-Upstream hop.

Note that the HTTP/2 upgrade path has very strict HTTP/1.1 compliance, so will not proxy WebSocket
upgrade requests or responses with bodies.

.. TODO(alyssawilk) unhide this when unhiding config
.. CONNECT support
.. ^^^^^^^^^^^^^^^

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

@snowp snowp Apr 22, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: internall -> internally

.. CONNECT requests). CONNECT support can be enabled via the upgrade options described above, setting
.. the upgrade value to the special keyword "CONNECT".

.. While for HTTP/2, CONNECT request may have a path, in general and for HTTP/1.1 CONNECT requests do
.. 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

@mattklein123 mattklein123 Apr 21, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@alyssawilk alyssawilk Apr 22, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@snowp snowp Apr 22, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@mattklein123 mattklein123 Apr 22, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

@alyssawilk alyssawilk Apr 22, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@mattklein123 mattklein123 Apr 27, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

.. were any other request, and letting the upstream handle the CONNECT request, or by terminating the
.. CONNECT request, and forwarding the payload as raw TCP data. When CONNECT upgrade configuration is
.. set up, the default behavior is to proxy the CONNECT request. If termination is desired, this can
.. be accomplished by setting
.. :ref:`connect_config <envoy_api_field_config.filter.network.http_connection_manager.v2.HttpConnectionManager.UpgradeConfig.connect_config>`
.. If it that message is present for CONNECT requests, the router filter will strip the request headers,
.. and forward the HTTP payload upstream. On receipt of initial TCP data from upstream, the router
.. will synthesize 200 response headers, and then forward the TCP data as the HTTP response body.

.. .. warning::
.. This mode of CONNECT support can create major security holes if configured correctly, as the upstream
.. will be forwarded *unsanitized* headers if they are in the body payload. Please use with caution

51 changes: 39 additions & 12 deletions generated_api_shadow/envoy/config/route/v3/route_components.proto
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

34 changes: 33 additions & 1 deletion generated_api_shadow/envoy/config/route/v4alpha/route_components.proto
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading