-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
websocket: tunneling websockets (and upgrades in general) over H2 #4188
Changes from all commits
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 |
---|---|---|
|
@@ -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,7 @@ 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_ = config.allow_connect(); | ||
return ret; | ||
} | ||
|
||
|
@@ -392,5 +399,49 @@ 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(); | ||
} | ||
|
||
void Utility::transformUpgradeResponseFromH1toH2(HeaderMap& headers) { | ||
if (getResponseStatus(headers) == 101) { | ||
headers.insertStatus().value().setInteger(200); | ||
} | ||
headers.removeUpgrade(); | ||
headers.removeConnection(); | ||
} | ||
|
||
void Utility::transformUpgradeRequestFromH2toH1(HeaderMap& headers) { | ||
ASSERT(Utility::isH2UpgradeRequest(headers)); | ||
|
||
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 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. 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. 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(); | ||
if (headers.ContentLength() == nullptr) { | ||
headers.insertTransferEncoding().value().setReference( | ||
Http::Headers::get().TransferEncodingValues.Chunked); | ||
} | ||
} | ||
|
||
void Utility::transformUpgradeResponseFromH2toH1(HeaderMap& headers, absl::string_view upgrade) { | ||
if (getResponseStatus(headers) == 200) { | ||
headers.insertUpgrade().value().setCopy(upgrade.data(), upgrade.size()); | ||
headers.insertConnection().value().setReference(Http::Headers::get().ConnectionValues.Upgrade); | ||
if (headers.ContentLength() == nullptr) { | ||
headers.insertTransferEncoding().value().setReference( | ||
Http::Headers::get().TransferEncodingValues.Chunked); | ||
} | ||
headers.insertStatus().value().setInteger(101); | ||
} | ||
} | ||
|
||
} // 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.
why remove the check? the extended connect protocol does not use any upgrade headers from what I see.
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.
Because as docced up, for filter chain consistency, we consistently transform upgrades to H1 style headers at the codec layer. If there's
client - H1 upgrade - Envoy - H2 hop - another Envoy - H2 hop - something else
the Envoy sending and receiving the upgrade in H2 form will still see HTTP/1.1 style upgrade headers at the http connection manager and in the HTTP filters.
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.
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?
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.
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!
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.
Ok, I'm fine with it as-is.