-
Notifications
You must be signed in to change notification settings - Fork 5.5k
websocket: tunneling websockets (and upgrades in general) over H2 #4188
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.
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
Changes from 1 commit
b49f7e1
cdd027d
d8befc3
de45c14
a515a1e
96ea33f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,6 +20,31 @@ one can set up custom | |
| for the given upgrade type, up to and including only using the router filter to send the WebSocket | ||
| data upstream. | ||
|
|
||
| Handling H2 hops (implementation in progress) | ||
| --------------------------------------------- | ||
|
|
||
| One oft requested feature for Envoy was to allow WebSocket to traverse HTTP/2 hops, where there | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. :). I would remove the origin of this feature and simply state it as a fact like "Envoy currently has an alpha implementation of tunneling websockets over H2 streams for deployments that prefer a uniform H2 mesh throughout." |
||
| was a set-up such as | ||
|
|
||
| Client ---- HTTP/1.1 ---- Frontline Envoy ---- HTTP/2 ---- Second tier Envoy ---- H1 ---- Upstream | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this example, IMO, would be a lot clearer (at the risk of losing generality) if it was written as |
||
|
|
||
| In this case, if a client is for example using WebSocket, we want the Websocket to arive at the | ||
| upstream server functionally intact, which means it needs to traverse the HTTP/2 hop. | ||
|
|
||
| TODO(alyssawilk) copy the warnings from the config here, or just land the docs when we unhide. | ||
|
|
||
| This is accomplished via | ||
| `extended CONNECT <https://tools.ietf.org/html/draft-mcmanus-httpbis-h2-websockets`_ support. The | ||
| WebSocket request will be transformed into an HTTP/2 CONNECT stream, with :protocol header | ||
| indicating the original upgrade, traverse the HTTP/2 hop, and be downgraded back into an HTTP/1 | ||
| WebSocket Upgrade. This same Upgrade-CONNECT-Upgrade transformation will be performed on any | ||
| HTTP/2 hop, with the documented flaw that the HTTP/1.1 method is always assumed to be GET. | ||
| Non-WebSocket upgrades are allowed to use any valid HTTP method (i.e. POST) and the current | ||
| upgrade/downgrade mechanism will drop the original method and transform the Upgrade request to | ||
| a GET method on the final Envoy-Upstream hop. | ||
|
|
||
| TODO(alyssawilk) link to the config changes required to enable upgrade upstream/downstream | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. implicit TODO - fix the broken docs build :-) (just wanted to get this out tonight, since I'm out tomorrow and I'd like a round of @mattklein123 thoughts) |
||
|
|
||
| Old style WebSocket support | ||
| =========================== | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -208,6 +208,7 @@ struct Http2Settings { | |
| uint32_t max_concurrent_streams_{DEFAULT_MAX_CONCURRENT_STREAMS}; | ||
| uint32_t initial_stream_window_size_{DEFAULT_INITIAL_STREAM_WINDOW_SIZE}; | ||
| uint32_t initial_connection_window_size_{DEFAULT_INITIAL_CONNECTION_WINDOW_SIZE}; | ||
| bool allow_connect_{DEFAULT_ALLOW_CONNECT}; | ||
|
|
||
| // disable HPACK compression | ||
| static const uint32_t MIN_HPACK_TABLE_SIZE = 0; | ||
|
|
@@ -241,6 +242,8 @@ struct Http2Settings { | |
| // our default connection-level window also equals to our stream-level | ||
| static const uint32_t DEFAULT_INITIAL_CONNECTION_WINDOW_SIZE = 256 * 1024 * 1024; | ||
| static const uint32_t MAX_INITIAL_CONNECTION_WINDOW_SIZE = (1U << 31) - 1; | ||
| // By default both nghttp2 and Envoy do now allow CONNECT over H2. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. s/now/not/ |
||
| static const bool DEFAULT_ALLOW_CONNECT = false; | ||
| }; | ||
|
|
||
| /** | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,13 +19,13 @@ namespace Envoy { | |
| namespace Http { | ||
|
|
||
| Network::Address::InstanceConstSharedPtr ConnectionManagerUtility::mutateRequestHeaders( | ||
| Http::HeaderMap& request_headers, Protocol protocol, Network::Connection& connection, | ||
| Http::HeaderMap& request_headers, Network::Connection& connection, | ||
| ConnectionManagerConfig& config, const Router::Config& route_config, | ||
| Runtime::RandomGenerator& random, Runtime::Loader& runtime, | ||
| const LocalInfo::LocalInfo& local_info) { | ||
| // If this is a Upgrade request, do not remove the Connection and Upgrade headers, | ||
| // as we forward them verbatim to the upstream hosts. | ||
| if (protocol == Protocol::Http11 && Utility::isUpgrade(request_headers)) { | ||
| if (Utility::isUpgrade(request_headers)) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why remove the check? the extended connect protocol does not use any upgrade headers from what I see.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because as docced up, for filter chain consistency, we consistently transform upgrades to H1 style headers at the codec layer. If there's
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Previously this check was excluding Http2 and Http10. If I'm understanding this correctly, you want to allow Http2 here. Do we need to still exclude Http10?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We definitely need to include HTTP/2 here given the way we're doing upgrades in Envoy. Upgrade is an HTTP/1.1 header so arguably we could protocol checks back in and explicitly disallow something we don't expect folks to do. Given that 1.0 support is off by default and I don't think anyone's trying to do 1.0 upgrades I lean towards not worrying about it but I'm happy to add it back if you'd prefer!
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, I'm fine with it as-is. |
||
| // The current WebSocket implementation re-uses the HTTP1 codec to send upgrade headers to | ||
| // the upstream host. This adds the "transfer-encoding: chunked" request header if the stream | ||
| // has not ended and content-length does not exist. In HTTP1.1, if transfer-encoding and | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -90,7 +90,19 @@ void ConnectionImpl::StreamImpl::encode100ContinueHeaders(const HeaderMap& heade | |
|
|
||
| void ConnectionImpl::StreamImpl::encodeHeaders(const HeaderMap& headers, bool end_stream) { | ||
| std::vector<nghttp2_nv> final_headers; | ||
| buildHeaders(final_headers, headers); | ||
|
|
||
| if (Http::Utility::isUpgrade(headers)) { | ||
| HeaderMapImpl modified_headers(headers); | ||
| upgrade_type_ = headers.Upgrade()->value().c_str(); | ||
| if (headers.Status()) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: it feels a bit odd that we are handling both request/response at the base stream layer and I wonder if we should have request/response stream overrides to make this code more clear? I don't feel very strongly about this but throwing it out there. Same comment in the decode path.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Moving the header munging out makes for a lot of duplicate code which I'd prefer to avoid. I could either move the complexity (status check) to the utility or have a Server/Client wrapper for transforms. I'll start with the latter and see what folks think of it. |
||
| Http::Utility::transformUpgradeResponseFromH1toH2(modified_headers); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My high level comment above the encode/decode changes in this file is that it seems to assume that above the codec level, all upgrades are treated as H1 style upgrades. Basically, this is an invariant that we assume is true for both upstream and downstream proxy. Is that correct? If so, I think it makes sense to do the conversions in the codec, but perhaps more comments on what is going on?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. shouldn't this be transformUpgradeResponseFromH2toH1?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ignore.. |
||
| } else { | ||
| Http::Utility::transformUpgradeRequestFromH1toH2(modified_headers); | ||
| } | ||
| buildHeaders(final_headers, modified_headers); | ||
| } else { | ||
| buildHeaders(final_headers, headers); | ||
| } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a way to add some indication on the direction? whether the response is being returned to downstream or request is being forwarded to upstream ? that should make this code a bit more clear. |
||
|
|
||
| nghttp2_data_provider provider; | ||
| if (!end_stream) { | ||
|
|
@@ -151,6 +163,15 @@ void ConnectionImpl::StreamImpl::pendingRecvBufferLowWatermark() { | |
| readDisable(false); | ||
| } | ||
|
|
||
| void ConnectionImpl::StreamImpl::decodeHeaders() { | ||
| if (Http::Utility::isH2UpgradeRequest(*headers_)) { | ||
| Http::Utility::transformUpgradeRequestFromH2toH1(*headers_); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same comment as above. I can't figure out if this is between downstream->Envoy or Envoy->downstream, or Envoy->upstream or upstream->Envoy.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Or some comments would surely help
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this logic going to preclude us from stuffing arbitrary TCP streams into H2?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So I think the refactor I'm doing for Matt (not yet uploaded) will help since the ClientStreamImpl will transformUpgradeRequestFromH1toH2 and transformUpgradeResponseFromH2toH1 and the Server will do the same in reverse. If this is an extended connect we'll munge the headers back to websocket form, else we won't touch them, and the CONNECT will remain a CONNECT. I don't think we currently have plans for doing arbitrary TCP for H2 (we need to but we don't yet have support) so I don't know what we would do in the else today :-) |
||
| } else if (!upgrade_type_.empty() && headers_->Status()) { | ||
| Http::Utility::transformUpgradeResponseFromH2toH1(*headers_, upgrade_type_); | ||
| } | ||
| decoder_->decodeHeaders(std::move(headers_), remote_end_stream_); | ||
| } | ||
|
|
||
| void ConnectionImpl::StreamImpl::pendingSendBufferHighWatermark() { | ||
| ENVOY_CONN_LOG(debug, "send buffer over limit ", parent_.connection_); | ||
| ASSERT(!pending_send_buffer_high_watermark_called_); | ||
|
|
@@ -366,13 +387,13 @@ int ConnectionImpl::onFrameReceived(const nghttp2_frame* frame) { | |
| ASSERT(!stream->remote_end_stream_); | ||
| stream->decoder_->decode100ContinueHeaders(std::move(stream->headers_)); | ||
| } else { | ||
| stream->decoder_->decodeHeaders(std::move(stream->headers_), stream->remote_end_stream_); | ||
| stream->decodeHeaders(); | ||
| } | ||
| break; | ||
| } | ||
|
|
||
| case NGHTTP2_HCAT_REQUEST: { | ||
| stream->decoder_->decodeHeaders(std::move(stream->headers_), stream->remote_end_stream_); | ||
| stream->decodeHeaders(); | ||
| break; | ||
| } | ||
|
|
||
|
|
@@ -401,7 +422,7 @@ int ConnectionImpl::onFrameReceived(const nghttp2_frame* frame) { | |
| // start out with. In this case, raise as headers. nghttp2 message checking guarantees | ||
| // proper flow here. | ||
| ASSERT(!stream->headers_->Status() || stream->headers_->Status()->value() != "100"); | ||
| stream->decoder_->decodeHeaders(std::move(stream->headers_), stream->remote_end_stream_); | ||
| stream->decodeHeaders(); | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -735,6 +756,10 @@ ConnectionImpl::Http2Options::Http2Options(const Http2Settings& http2_settings) | |
| if (http2_settings.hpack_table_size_ != NGHTTP2_DEFAULT_HEADER_TABLE_SIZE) { | ||
| nghttp2_option_set_max_deflate_dynamic_table_size(options_, http2_settings.hpack_table_size_); | ||
| } | ||
| if (http2_settings.allow_connect_) { | ||
| // TODO(alyssawilk) change to ENABLE_CONNECT_PROTOCOL when it's available. | ||
| nghttp2_option_set_no_http_messaging(options_, 1); | ||
| } | ||
| } | ||
|
|
||
| ConnectionImpl::Http2Options::~Http2Options() { nghttp2_option_del(options_); } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -187,11 +187,16 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable<Log | |
| // I don't fully understand. | ||
| static const uint64_t MAX_HEADER_SIZE = 63 * 1024; | ||
|
|
||
| // Does any necessary WebSocket/Upgrade conversion, then passes the headers | ||
| // to the decoder_. | ||
| void decodeHeaders(); | ||
|
|
||
| bool buffers_overrun() const { return read_disable_count_ > 0; } | ||
|
|
||
| ConnectionImpl& parent_; | ||
| HeaderMapImplPtr headers_; | ||
| StreamDecoder* decoder_{}; | ||
| std::string upgrade_type_; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What do you think of qualifying this with websocket alone? Or may be an enum for other protos in future? The reason is that Websocket is the only Http-ish proto in this picture. Others are going to be just opaque TCP.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't see the advantage to restricting it? I guess we'd save the memory of latching one header but I don't think that's worth removing the flexibility. |
||
| int32_t stream_id_{-1}; | ||
| uint32_t unconsumed_bytes_{0}; | ||
| uint32_t read_disable_count_{0}; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -209,6 +209,12 @@ bool Utility::isUpgrade(const HeaderMap& headers) { | |
| Http::Headers::get().ConnectionValues.Upgrade.c_str())); | ||
| } | ||
|
|
||
| bool Utility::isH2UpgradeRequest(const HeaderMap& headers) { | ||
| return headers.Method() && | ||
| headers.Method()->value().c_str() == Http::Headers::get().MethodValues.Connect && | ||
| headers.Protocol() && !headers.Protocol()->value().empty(); | ||
| } | ||
|
|
||
| bool Utility::isWebSocketUpgradeRequest(const HeaderMap& headers) { | ||
| return (isUpgrade(headers) && (0 == StringUtil::caseInsensitiveCompare( | ||
| headers.Upgrade()->value().c_str(), | ||
|
|
@@ -227,6 +233,8 @@ Utility::parseHttp2Settings(const envoy::api::v2::core::Http2ProtocolOptions& co | |
| ret.initial_connection_window_size_ = | ||
| PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, initial_connection_window_size, | ||
| Http::Http2Settings::DEFAULT_INITIAL_CONNECTION_WINDOW_SIZE); | ||
| ret.allow_connect_ = PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, allow_connect, | ||
| Http::Http2Settings::DEFAULT_ALLOW_CONNECT); | ||
| return ret; | ||
| } | ||
|
|
||
|
|
@@ -392,5 +400,50 @@ std::string Utility::queryParamsToString(const QueryParams& params) { | |
| return out; | ||
| } | ||
|
|
||
| void Utility::transformUpgradeRequestFromH1toH2(HeaderMap& headers) { | ||
| ASSERT(Utility::isUpgrade(headers)); | ||
|
|
||
| const HeaderString& upgrade = headers.Upgrade()->value(); | ||
| headers.insertMethod().value().setReference(Http::Headers::get().MethodValues.Connect); | ||
| headers.insertProtocol().value().setCopy(upgrade.c_str(), upgrade.size()); | ||
| headers.removeUpgrade(); | ||
| headers.removeConnection(); | ||
| if (headers.ContentLength() == nullptr) { | ||
| headers.insertTransferEncoding().value().setReference( | ||
| Http::Headers::get().TransferEncodingValues.Chunked); | ||
| } | ||
| } | ||
|
|
||
| void Utility::transformUpgradeResponseFromH1toH2(HeaderMap& headers) { | ||
| if (getResponseStatus(headers) == 101) { | ||
| headers.insertStatus().value().setCopy("200", 3); | ||
| } | ||
| headers.removeUpgrade(); | ||
| headers.removeConnection(); | ||
| if (headers.ContentLength() == nullptr) { | ||
| headers.insertTransferEncoding().value().setReference( | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I haven't looked at the spec but is this right? For an H2 conversion I think we don't want any transfer encoding? Same above? More comments if it's correct? |
||
| Http::Headers::get().TransferEncodingValues.Chunked); | ||
| } | ||
| } | ||
|
|
||
| void Utility::transformUpgradeRequestFromH2toH1(HeaderMap& headers) { | ||
| ASSERT(Utility::isH2UpgradeRequest(headers)); | ||
|
|
||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, this definitely needs to be qualified to websocket alone. Coz for other protos, we are likely to initiate outbound TCP proxy connections, where we simply strip the H2 framing
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When you say initiate outbound TCP proxy connections, what do you mean? you can't strip H2 framing over an H2 connection. I can definitely think of other non-websocket upgrades we're going to do over H2 extended connect, and I'd prefer a consistent mechanism for handling them, so I'd prefer to not limit this to websocket. Keep in mind, these are configurable on a per-upgrade basis. We can always add a different type of upgrade, or configure an upgrade for different behavior, to extend Envoy functionality later without changing this code at all. Basically since the mechanism for this type of support and any new type of support are all done via config, I don't think having flexibility in this implementation will come back to bite us.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. may be I am missing something here. I am envisioning being able to tunnel mySQL connections over H2 streams to the envoy on the other end, and having the server Envoy hand over a plain tcp connection to the mysql container in the same pod. The client-side envoy would receive the inbound connection over a TCP_PROXY, and and shove the bytes into a H2 stream and send to an upstream Envoy. Now, if there are multiple envoys between the client and server side envoys, then these intermediaries are going to be receiving a H2 stream with extended CONNECT header, and have to forward the same H2 stream to the next envoy. Reading this code, I got the impression that with websockets, an intermediary envoy would strip the H2 framing and convert the extended connect/upgrade into standard H1 upgrade for websocket. Then when handing over the same connection to the cluster manager, the codec will re-wrap the H1 upgrade headers back into a H2 stream and forward. And my concern here (and elsewhere) was this H2-->H1 unwrapping where there is an implicit assumption that these protocols are all somehow h1 friendly.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think if we're tunneling upgrades, the payload has to be HTTP2/HTTP1 friendly, today. In the long run, I think we're going to have code which both encaps TCP in an HTTP/H2 upgrade, and decaps HTTP/H2 upgrades back to TCP. We don't have that code yet. I think we're absolutely going to have it, but the point of this PR is to allow Envoy to pass along encaped data, not to do the decap Our main use case for this is multiplexing transparent proxying. If we have a frontline envoy without certs, it can do TCP termination, forward the payload over a pre-warmed H2 hop, demux it at the far end, and then treat each H2 stream as if it were a new incoming TCP connection (which will often have TLS/H2 payload, requiring another layer of demuxing). Some second line Envoys might be configured to "foward raw_tcp upgrades untouched" and some might be configured to "demux and handle raw_tcp upgrades". I do think there's an advantage to allowing multiple types of upgrade (assuming the eventual nghttp2 implementation allows) so the config can do one thing for mysql_upgrades and a different thing for raw_tcp upgrades. It would allow the frontline Envoy which is doing the encap in HTTP1/HTTP2 to specify the upgrade type, and then the next Envoy in the chain could be configured to pass it along or terminate. But all this PR does is allow the upgrades to survive the H2 hop. |
||
| const HeaderString& protocol = headers.Protocol()->value(); | ||
| headers.insertMethod().value().setReference(Http::Headers::get().MethodValues.Get); | ||
| headers.insertUpgrade().value().setCopy(protocol.c_str(), protocol.size()); | ||
| headers.insertConnection().value().setReference(Http::Headers::get().ConnectionValues.Upgrade); | ||
| headers.removeProtocol(); | ||
| } | ||
|
|
||
| void Utility::transformUpgradeResponseFromH2toH1(HeaderMap& headers, absl::string_view upgrade) { | ||
| if (getResponseStatus(headers) == 200) { | ||
| headers.insertStatus().value().setCopy("101", 3); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can just use |
||
| } | ||
| // TODO(alyssawilk) what should we do for websocket responses on the failure path? | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We are not handling websocket upgrade rejects at all. The assumption is that the server would terminate the connection allowing us to cleanup. We could look at the response code and clean up |
||
| headers.insertUpgrade().value().setCopy(upgrade.data(), upgrade.size()); | ||
| headers.insertConnection().value().setReference(Http::Headers::get().ConnectionValues.Upgrade); | ||
| } | ||
|
|
||
| } // namespace Http | ||
| } // namespace Envoy | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be bool instead of g.p.BoolValue?