-
Notifications
You must be signed in to change notification settings - Fork 5.5k
quiche: implement http stream interfaces #8556
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 20 commits
edfbf4b
f574a58
5cc1622
469dc2a
68b6501
3a42546
c0ede9a
3e39fbe
d7c610b
013701b
95eb51d
e040f72
29128aa
31340f2
88ebd06
8a2740d
e43a1fe
438a8d7
4056e85
b6b10ac
f7eb6e3
75e36e0
92068a4
c6c2177
6175a34
35d4539
b06844f
6c53df8
20fe85c
484ffba
90a6953
bddb942
3cad65e
81e2fa9
9b72210
6c03488
86c8ec1
a23de7f
6f33c26
e1e9fa4
2d7f6ac
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 |
|---|---|---|
| @@ -0,0 +1,49 @@ | ||
| ### Overview | ||
|
|
||
| QUICHE is integrated in the way described below: | ||
|
|
||
| A combination of QUIC session and connection serves as a Network::Connection instance. More than that, QUIC session manages all the QUIC streams. QUIC codec is a very thin layer between QUIC session and HCM. It doesn't do de-multiplexing stream management, but only provides interfaces for HCM to communicate with QUIC session. | ||
|
|
||
| QUIC's Http::StreamEncoder and Http::StreamDecoder implementation is decoupled, encoder is implemented by EnvoyQuicStream which is a QUIC stream and owned by session. HCM owns the decoder which can be accessed by QUIC stream instances. And decoder also knows about stream encoder. | ||
|
|
||
| ### Request piepline | ||
|
|
||
| QUIC stream calls decodeHeaders() to deliver request headers. Request body needs | ||
| to be delivered after headers are delieverd as some QUICHE implementation allows | ||
| body to arrive earlier than headers. If not read disabled, always deliever available data via decodeData(). Stream | ||
| doesn't buffer any readable data in QUICHE stream buffers. | ||
|
|
||
| ### Response pipeline | ||
|
|
||
| HCM will call encoder's encodeHeaders() to write response headers, and then encodeData() and encodeTrailers(). encodeData() calls WriteBodySlices() to write response body. Quic stream in QUICHE can config it's send buffer threshold QuicStream::buffered_data_threshold_ to be high enough to take all the data passed in, so stream can have unlimited buffer. | ||
|
|
||
| ### Flow Control | ||
|
|
||
| ## Receive buffer | ||
|
|
||
| All the arrived out-of-order data is buffered in QUICH stream. And this buffered | ||
| in capped by max stream flow control window in QUICHE which is 64MB. Once they are put | ||
| in sequence and ready to be used, OnBodyDataAvailable() is called. Our stream | ||
| implementation overrides this call and call StreamDecoder::decodeData() in it. | ||
| Request and response body is buffered in each L7 filter if desired, and the | ||
| stream itself doesn't buffer any of them if not set to be blocked. | ||
|
|
||
| When upstream or any L7 filter reaches its buffer limit, it call | ||
| Http::Stream::readDisable() with false to set QUIC stream in block state. In | ||
| this state, even more request/reponse body is available to be delivered, | ||
| OnBodyDataAvailable() will not be called. As a result, downstream flow contol | ||
| will not shift as no data will be consumed. As both filters and upstream buffer | ||
| can call readDisable(), each stream has a counter of how many | ||
| times the HCM wants to block the stream. Until the counter is cleared, stream | ||
| will resume its state to be unblocked and thus delivering any new and existing | ||
| available data in QUICHE stream. | ||
|
|
||
| ## Send buffer | ||
|
|
||
| We use the unlimited stream send buffer in QUICHE along with a book keeping data structure `EnvoyQuicSimulatedWatermarkBffer` to server the purpose of WatermarkBuffer in Envoy to prevent buffering too much in QUICHE. | ||
|
|
||
| When the bytes buffered in a stream's send buffer exceeds its high watermark, its inherited method StreamCallbackHelper::runHighWatermarkCallbacks() is called. The buffered bytes will go below stream's low watermark as the stream writes out data gradually via QuicStream::OnCanWrite(). And in this case StreamCallbackHelper::runLowWatermarkCallbacks() will be called. QUICHE buffers all the data upon QuicSpdyStream::WriteBodySlices(), assuming `buffered_data_threshold_` is set high enough, and then writes all or part of them according to flow control window and congestion control window. To prevent transient changes in buffered bytes from triggering these two watermark callbacks one immediately after the other, encodeData() and OnCanWrite() only update the watermark book keeping once at the end if buffered bytes are changed. | ||
|
|
||
| QUICHE doesn't buffer data in connection. And all the data is buffered in stream.To prevent the case where all streams collectively buffers a lot of data, there is also a simulated watermark buffer for each Quic connection which changes upon each stream write. | ||
|
|
||
| When the aggregated buffered bytes goes above high watermark, its registered network callbacks will call Network::ConnectionCallbacks::onAboveWriteBufferHighWatermark(). HCM as a such callback will notify each stream via QUIC codec Http::Connection::onUnderlyingConnectionAboveWriteBufferHighWatermark() which will call each stream's StreamCallbackHelper::runHighWatermarkCallbacks(). There might be a way to simply the call stack as Quic connection already knows about all the stream, there is no need to call to HCM and notify each stream via codec. But here we just follow the same logic as HTTP2 codec does. Same way, any QuicStream::OnCanWrite() may change the aggregated buffered bytes in the connection level book keeping as well. If the buffered bytes goes down below low watermark, same calls will be triggered to propergate onBelowWriteBufferLowWatermark() to each stream callbacks. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -49,6 +49,7 @@ namespace Logger { | |
| FUNCTION(misc) \ | ||
| FUNCTION(mongo) \ | ||
| FUNCTION(quic) \ | ||
| FUNCTION(quic_stream) \ | ||
|
Contributor
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 do we need 'quic_stream'? Looking at the list, most dependent libraries only use one logger id.
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. EnvoyQuicSimulatedWatermarkBuffer is used in both session and stream, to make the log more clear, I pass in logger instance from its owner. And the logger needs a different id to distinguish each other.
Contributor
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 see. I'm inclined to replace EnvoyQuicSimulatedWatermarkBuffer.logger_ by EnvoyQuicSimulatedWatermarkBuffer.stream_id, use QuicUtils::GetInvalidStreamId() if it's for a session. That way you can log to the 'quic' logger with a stream id in it.
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. EnvoyQuicSimulatedWatermarkBuffer.stream_id is a good idea. That way I can use ENVOY_STREAM_LOG. But I'm not sure if moving all the quic related log under 'quic' logger is what we want. Quic session and connections are using 'connection' logger currently in parallel to Network::ConnectionImpl |
||
| FUNCTION(pool) \ | ||
| FUNCTION(rbac) \ | ||
| FUNCTION(redis) \ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,18 +1,34 @@ | ||
| #include "extensions/quic_listeners/quiche/codec_impl.h" | ||
|
|
||
| #include "extensions/quic_listeners/quiche/envoy_quic_server_stream.h" | ||
|
|
||
| namespace Envoy { | ||
| namespace Quic { | ||
|
|
||
| bool QuicHttpConnectionImplBase::wantsToWrite() { return quic_session_.HasDataToWrite(); } | ||
|
|
||
| // TODO(danzh): modify QUIC stack to react based on aggregated bytes across all | ||
| // the streams. And call StreamCallbackHelper::runHighWatermarkCallbacks() for each stream. | ||
| void QuicHttpConnectionImplBase::onUnderlyingConnectionAboveWriteBufferHighWatermark() { | ||
| NOT_IMPLEMENTED_GCOVR_EXCL_LINE; | ||
| QuicHttpServerConnectionImpl::QuicHttpServerConnectionImpl( | ||
| EnvoyQuicServerSession& quic_session, Http::ServerConnectionCallbacks& callbacks) | ||
| : QuicHttpConnectionImplBase(quic_session), quic_server_session_(quic_session) { | ||
| quic_session.setHttpConnectionCallbacks(callbacks); | ||
| } | ||
|
|
||
| void QuicHttpServerConnectionImpl::onUnderlyingConnectionAboveWriteBufferHighWatermark() { | ||
| for (auto& it : quic_server_session_.stream_map()) { | ||
| if (!it.second->is_static()) { | ||
|
Contributor
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 am completely failing to find what is_static checks. can you clue me in?
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. is_static is true for static streams, for IETF QUIC, it's crypto stream, and for GQuic, also headers stream.
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. Nowadays, all the streams are managed in one place: QuicSession::stream_map_
Contributor
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 we're willing to infinite buffer on headers stream? We had a recent attack on the H2 stack where when an incoming request could generate an immediate response (like a 400 or 404) the flow control "didn't work" because we had assumed downstream buffers were fed by upstream (backends) not downstream (new bad requests) and adversary could keep feeding bad requests, generating header-only responses and adding more and more to the downstream connection. I want to make sure we don't reintroduce that flaw for HTTP/3 One workaround would be #7619 to have an upper upper bound but that hasn't landed yet. The other was the series of PRs @yanavlasov did to cap outbound frames (search for "flood" in the h2 codec). I think we need one or the other here if headers are otherwise unbounded. TODO is fine. If we leave this as is for now I'd suggest a comment with the TODO
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. QuicHeaderStream always buffer data, its WriteHeaders() doesn't check this threshold. In IETF QUIC, there is no headers stream, and headers will be serialized into stream payload. That would solve this problem, right?
Contributor
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. Not having a headers stream helps, but it'd be good to have hard bounds on the crypto stream too.
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. If we need a hard bound on crypto stream, it needs to be in QUICHE because the handshake is hidden from envoy. But I still don't understand how can crypto stream subject to this sort of attack?
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. Checked QUICHE code, the IETF quic data stream should be robust against this sort of attack because the buffered headers in data stream's send buffer should have prevented stream from being closed still the header is ACK'ed. So the totally data buffered is capped by max allowed header size * max allowed stream number. For GQuic, because all the headers are sent on header stream and stream is closed once header is buffered in send buffer, the header stream can buffer infinite number of response headers. We need a hard bound for headers stream. But this probably not worth to be added in QUICHE now as QUICHE is moving to IETF quic. crypto stream also has infinite send buffer, if handshake implementation is wrong, it can also buffer infinitely. I added TODO in EnvoyQuicServerStream::encodeHeaders() about this vulnerability. |
||
| ENVOY_LOG(debug, "runHighWatermarkCallbacks on stream {}", it.first); | ||
| dynamic_cast<EnvoyQuicServerStream*>(it.second.get())->runHighWatermarkCallbacks(); | ||
|
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 do we need the dynamic cast here and below? Can the stored stream just derive from the right interface?
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. stream_map() is a QUICHE interface, for sure it doesn't know about Envoy stream interface.
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 see so this map is storing an opaque pointer of some type? Could it eventually just store a common interface that our classes derive from? Can this be a TODO here and below?
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. QuicSession::stream_map() store all QuicStream objects. Our envoy quic stream also derives from that class and from EnvoyQuicStream as well. And the latter has runHighWatermarkCallbacks() interface. So we need a cross cast here I believe.
Contributor
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'm happy with the TODO there. Can we have some comment the first time we reference is_static just explaining it's crypto and gQUIC headers?
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. done |
||
| } | ||
| } | ||
| } | ||
|
|
||
| void QuicHttpConnectionImplBase::onUnderlyingConnectionBelowWriteBufferLowWatermark() { | ||
| NOT_IMPLEMENTED_GCOVR_EXCL_LINE; | ||
| void QuicHttpServerConnectionImpl::onUnderlyingConnectionBelowWriteBufferLowWatermark() { | ||
| for (const auto& it : quic_server_session_.stream_map()) { | ||
| if (!it.second->is_static()) { | ||
| ENVOY_LOG(debug, "runLowWatermarkCallbacks on stream {}", it.first); | ||
| dynamic_cast<EnvoyQuicServerStream*>(it.second.get())->runLowWatermarkCallbacks(); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| void QuicHttpServerConnectionImpl::goAway() { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,6 +9,7 @@ | |
| #include "quiche/quic/core/quic_crypto_server_stream.h" | ||
| #pragma GCC diagnostic pop | ||
|
|
||
| #include "common/common/assert.h" | ||
| #include "extensions/quic_listeners/quiche/envoy_quic_server_stream.h" | ||
|
|
||
| namespace Envoy { | ||
|
|
@@ -18,10 +19,16 @@ EnvoyQuicServerSession::EnvoyQuicServerSession( | |
| const quic::QuicConfig& config, const quic::ParsedQuicVersionVector& supported_versions, | ||
| std::unique_ptr<EnvoyQuicConnection> connection, quic::QuicSession::Visitor* visitor, | ||
| quic::QuicCryptoServerStream::Helper* helper, const quic::QuicCryptoServerConfig* crypto_config, | ||
| quic::QuicCompressedCertsCache* compressed_certs_cache, Event::Dispatcher& dispatcher) | ||
| quic::QuicCompressedCertsCache* compressed_certs_cache, Event::Dispatcher& dispatcher, | ||
| uint32_t send_buffer_limit) | ||
| : quic::QuicServerSessionBase(config, supported_versions, connection.get(), visitor, helper, | ||
| crypto_config, compressed_certs_cache), | ||
| QuicFilterManagerConnectionImpl(std::move(connection), dispatcher) {} | ||
| QuicFilterManagerConnectionImpl(connection.get(), dispatcher, send_buffer_limit), | ||
|
Contributor
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. As we talked offline, let's try simplify the lifetimes by changing QuicFilterManagerConnectionImpl to a member of EnvoyQuicServerSession, such that the Impl doesn't need to check if quic_connection_ is nullptr. Feel free to TODO it in a future PR.
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. Defer to #8496 so the refactoring can be tested. TODO added.
Contributor
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. Thanks! |
||
| quic_connection_(std::move(connection)) {} | ||
|
|
||
| EnvoyQuicServerSession::~EnvoyQuicServerSession() { | ||
| QuicFilterManagerConnectionImpl::quic_connection_ = nullptr; | ||
| } | ||
|
|
||
| absl::string_view EnvoyQuicServerSession::requestedServerName() const { | ||
| return {GetCryptoStream()->crypto_negotiated_params().sni}; | ||
|
|
||
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.
Excellent doc!
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.
nit: can we move to https://github.com/envoyproxy/envoy/tree/master/source/docs? This is where we currently have our dev docs. The top level docs directory is for user docs.
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.
done