-
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 34 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,33 @@ | ||
| ### 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. The QUIC codec is a very thin layer between QUIC session and HCM. It doesn't do de-multiplexing or stream management, but only provides interfaces for the HCM to communicate with the QUIC session. | ||
|
|
||
| QUIC's Http::StreamEncoder and Http::StreamDecoder implementation is decoupled. The encoder is implemented by EnvoyQuicStream which is a QUIC stream and owned by session. The HCM owns the decoder which can be accessed by QUIC stream instances. And the decoder also knows about stream encoder. | ||
|
|
||
| ### Request pipeline | ||
|
|
||
| The QUIC stream calls decodeHeaders() to deliver request headers. The request body needs to be delivered after headers are delivered as some QUICHE implementations allow the body to arrive earlier than headers. If not read disabled, it always deliver available data via decodeData(). The stream doesn't buffer any readable data in QUICHE stream buffers. | ||
|
|
||
| ### Response pipeline | ||
|
|
||
| The HCM will call encoder's encodeHeaders() to write response headers, and then encodeData() and encodeTrailers(). encodeData() calls WriteBodySlices() to write out response body. The quic stream in QUICHE configures its send buffer threshold QuicStream::buffered_data_threshold_ to be high enough to take all the data passed in, so stream functionally has unlimited buffer. | ||
|
|
||
| ### Flow Control | ||
|
|
||
| #### Receive buffer | ||
|
|
||
| All arrived out-of-order data is buffered in QUICHE stream. This buffer is capped by max stream flow control window in QUICHE which is 16MB. Once bytes are put in sequence and ready to be used, OnBodyDataAvailable() is called. The stream implementation overrides this call and calls StreamDecoder::decodeData() in it. Request and response body are buffered in each L7 filter if desired, and the stream itself doesn't buffer any of them unless set as read blocked. | ||
|
|
||
| When upstream or any L7 filter reaches its buffer limit, it will call Http::Stream::readDisable() with false to set QUIC stream to be read blocked. In this state, even if more request/response body is available to be delivered, OnBodyDataAvailable() will not be called. As a result, downstream flow control will not shift as no data will be consumed. As both filters and upstream buffers can call readDisable(), each stream has a counter of how many times the HCM blocks the stream. When the counter is cleared, the stream will set its state to unblocked and thus deliver any new and existing available data buffered in the QUICHE stream. | ||
|
|
||
| #### Send buffer | ||
|
|
||
| We use the unlimited stream send buffer in QUICHE along with a book keeping data structure `EnvoyQuicSimulatedWatermarkBuffer` to serve the function 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(). 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 bookkeeping once at the end if buffered bytes are changed. | ||
|
|
||
| QUICHE doesn't buffer data at the local connection layer. All the data is buffered in the respective streams.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 is updated upon each stream write. | ||
|
|
||
| When the aggregated buffered bytes goes above high watermark, its registered network callbacks will call Network::ConnectionCallbacks::onAboveWriteBufferHighWatermark(). The HCM 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. In the same way, any QuicStream::OnCanWrite() may change the aggregated buffered bytes in the connection level bookkeeping as well. If the buffered bytes goes down below the low watermark, the same calls will be triggered to propagate onBelowWriteBufferLowWatermark() to each stream. | ||
| 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 |
|---|---|---|
|
|
@@ -25,6 +25,13 @@ EnvoyQuicDispatcher::EnvoyQuicDispatcher( | |
| // Network::UdpListenerCallbacks which should be called at the beginning | ||
| // of HandleReadEvent(). And this callback should call quic::Dispatcher::ProcessBufferedChlos(). | ||
| SetQuicFlag(FLAGS_quic_allow_chlo_buffering, false); | ||
| // Set send buffer twice of max flow control window to ensure that stream send | ||
| // buffer always takes all the data. | ||
| // The max amount of data buffered is the per-stream high watermark + the max | ||
| // flow control window of upstream. The per-stream high watermark should be | ||
| // smaller than max flow control window to make sure upper stream can be flow | ||
| // control blocked early enough not to send more than the threshold allows. | ||
| SetQuicFlag(FLAGS_quic_buffered_data_threshold, 2 * 16 * 1024 * 1024); // 32MB | ||
|
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 seems pretty huge compared to what we typically recommend for HTTP/2 at the edge. Will we eventually make this configurable?
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. This threshold needs to be large enough to hold all the data from upstream even after readDisable(). So it should be larger than HTTP2 per stream flow control window whose negotiated value is not accessible at this level. I will add a TODO to plumb through to get that value. But before that, for QUICHE to work correctly, we need this threshold to be large enough what each encodeData() can consume all the data passed in. This doesn't mean that we are actually buffering that much data because upstream's flow control window probably is not that large after negotiation. |
||
| } | ||
|
|
||
| void EnvoyQuicDispatcher::OnConnectionClosed(quic::QuicConnectionId connection_id, | ||
|
|
@@ -44,7 +51,8 @@ quic::QuicSession* EnvoyQuicDispatcher::CreateQuicSession( | |
| listener_stats_); | ||
| auto quic_session = new EnvoyQuicServerSession( | ||
| config(), quic::ParsedQuicVersionVector{version}, std::move(quic_connection), this, | ||
| session_helper(), crypto_config(), compressed_certs_cache(), dispatcher_); | ||
| session_helper(), crypto_config(), compressed_certs_cache(), dispatcher_, | ||
| listener_config_.perConnectionBufferLimitBytes()); | ||
| quic_session->Initialize(); | ||
| // Filter chain can't be retrieved here as self address is unknown at this | ||
| // point. | ||
|
|
||
| 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}; | ||
|
|
@@ -41,6 +48,9 @@ quic::QuicSpdyStream* EnvoyQuicServerSession::CreateIncomingStream(quic::QuicStr | |
| auto stream = new EnvoyQuicServerStream(id, this, quic::BIDIRECTIONAL); | ||
| ActivateStream(absl::WrapUnique(stream)); | ||
| setUpRequestDecoder(*stream); | ||
| if (aboveHighWatermark()) { | ||
| stream->runHighWatermarkCallbacks(); | ||
| } | ||
| return stream; | ||
| } | ||
|
|
||
|
|
||
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