Skip to content
Merged
Show file tree
Hide file tree
Changes from 28 commits
Commits
Show all changes
41 commits
Select commit Hold shift + click to select a range
edfbf4b
merged all commits
danzh1989 Oct 4, 2019
f574a58
fix mem slice warning
danzh1989 Oct 4, 2019
5cc1622
remove broken include
danzh1989 Oct 4, 2019
469dc2a
cryptoConnect
danzh1989 Oct 4, 2019
68b6501
fix xfcc test
danzh1989 Oct 4, 2019
3a42546
remove stream libs
danzh1989 Oct 7, 2019
c0ede9a
fix asan
danzh1989 Oct 7, 2019
3e39fbe
revert non-HCM change
danzh1989 Oct 7, 2019
d7c610b
Revert "revert non-HCM change"
danzh1989 Oct 7, 2019
013701b
Merge branch 'master' into quicintegration
danzh1989 Oct 8, 2019
95eb51d
revert HCM, client and integration test
danzh1989 Oct 8, 2019
e040f72
remove unrelated change
danzh1989 Oct 9, 2019
29128aa
fix asan
danzh1989 Oct 10, 2019
31340f2
typo
danzh1989 Oct 10, 2019
88ebd06
add test for readDisable
danzh1989 Oct 14, 2019
8a2740d
rename var
danzh1989 Oct 14, 2019
e43a1fe
typo
danzh1989 Oct 14, 2019
438a8d7
bufferLimit
danzh1989 Oct 14, 2019
4056e85
typo
danzh1989 Oct 14, 2019
b6b10ac
add more
danzh1989 Oct 15, 2019
f7eb6e3
adjust send buffer threshold, modify doc
danzh1989 Oct 16, 2019
75e36e0
above watermark upon stream creation
danzh1989 Oct 16, 2019
92068a4
fix clang_tidy
danzh1989 Oct 16, 2019
c6c2177
typo
danzh1989 Oct 17, 2019
6175a34
remove debug log
danzh1989 Oct 17, 2019
35d4539
add test for simulated buffer
danzh1989 Oct 17, 2019
b06844f
doc
danzh1989 Oct 18, 2019
6c53df8
Merge branch 'master' into quicencode
danzh1989 Oct 21, 2019
20fe85c
add test util
danzh1989 Oct 21, 2019
484ffba
fix typos in doc
danzh1989 Oct 21, 2019
90a6953
improve comment
danzh1989 Oct 23, 2019
bddb942
code cleanup in test
danzh1989 Oct 24, 2019
3cad65e
Merge branch 'master' into quicencode
danzh1989 Oct 24, 2019
81e2fa9
fix quiche_copts
danzh1989 Oct 24, 2019
9b72210
remove AtMost EXPECT_CALL
danzh1989 Oct 28, 2019
6c03488
comment in watermark buffer
danzh1989 Oct 28, 2019
86c8ec1
comment about buffer limit
danzh1989 Oct 29, 2019
a23de7f
fix namespace
danzh1989 Oct 30, 2019
6f33c26
format
danzh1989 Oct 30, 2019
e1e9fa4
add tracking issue
danzh1989 Oct 31, 2019
2d7f6ac
comments
danzh1989 Nov 4, 2019
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
14 changes: 14 additions & 0 deletions bazel/external/quiche.BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -3112,6 +3112,20 @@ envoy_cc_test_library(
],
)

envoy_cc_test_library(
name = "quic_test_tools_server_session_base_peer",
hdrs = [
"quiche/quic/test_tools/quic_server_session_base_peer.h",
],
copts = quiche_copt,
repository = "@envoy",
tags = ["nofips"],
deps = [
":quic_core_http_spdy_session_lib",
":quic_core_utils_lib",
],
)

envoy_cc_test_library(
name = "quic_test_tools_simple_quic_framer_lib",
srcs = ["quiche/quic/test_tools/simple_quic_framer.cc"],
Expand Down
33 changes: 33 additions & 0 deletions docs/quiche_integration.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
### Overview

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.

Excellent doc!

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.

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.

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.

done


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 buffered is capped by max stream flow control window in QUICHE which is 64MB. 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.

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.

This buffered is capped => This buffer is capped

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.

in it.Request and response => Add a space after '.'

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.

done

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.

(As we talked offline) The max flow control window is 16MB for single stream, 24MB for all streams in a QUIC session.

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.

done


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 buttered in the QUICHE stream.

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.

available data buttered in the QUICHE stream => available data buffered in the QUICHE stream

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.

done


#### Send buffer

We use the unlimited stream send buffer in QUICHE along with a book keeping data structure `EnvoyQuicSimulatedWatermarkBffer` to serve the function of WatermarkBuffer in Envoy to prevent buffering too much in QUICHE.

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.

EnvoyQuicSimulatedWatermarkBffer => EnvoyQuicSimulatedWatermarkBuffer

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.

done


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 HCMwill 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 propergate onBelowWriteBufferLowWatermark() to each stream.

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.

The HCMwill notify => The HCM will notify

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.

triggered to propergate => triggered to propagate

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.

done

1 change: 1 addition & 0 deletions source/common/common/logger.h
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ namespace Logger {
FUNCTION(misc) \
FUNCTION(mongo) \
FUNCTION(quic) \
FUNCTION(quic_stream) \

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.

Why do we need 'quic_stream'? Looking at the list, most dependent libraries only use one logger id.

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.

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.

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.

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.

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.

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) \
Expand Down
45 changes: 27 additions & 18 deletions source/extensions/quic_listeners/quiche/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -97,26 +97,14 @@ envoy_cc_library(
hdrs = ["envoy_quic_stream.h"],
tags = ["nofips"],
deps = [
":envoy_quic_simulated_watermark_buffer_lib",
":quic_filter_manager_connection_lib",
"//include/envoy/event:dispatcher_interface",
"//include/envoy/http:codec_interface",
"//source/common/http:codec_helper_lib",
],
)

envoy_cc_library(
name = "envoy_quic_server_stream_lib",
srcs = ["envoy_quic_server_stream.cc"],
hdrs = ["envoy_quic_server_stream.h"],
tags = ["nofips"],
deps = [
":envoy_quic_stream_lib",
":envoy_quic_utils_lib",
"//source/common/buffer:buffer_lib",
"//source/common/common:assert_lib",
"//source/common/http:header_map_lib",
"@com_googlesource_quiche//:quic_core_http_spdy_session_lib",
],
)

envoy_cc_library(
name = "codec_lib",
srcs = ["codec_impl.cc"],
Expand All @@ -136,22 +124,37 @@ envoy_cc_library(
tags = ["nofips"],
deps = [
":envoy_quic_connection_lib",
":envoy_quic_simulated_watermark_buffer_lib",
"//include/envoy/event:dispatcher_interface",
"//include/envoy/network:connection_interface",
"//source/common/buffer:buffer_lib",
"//source/common/common:assert_lib",
"//source/common/common:empty_string",
"//source/common/http:header_map_lib",
"//source/common/network:filter_manager_lib",
"//source/common/stream_info:stream_info_lib",
],
)

envoy_cc_library(
name = "envoy_quic_server_session_lib",
srcs = ["envoy_quic_server_session.cc"],
hdrs = ["envoy_quic_server_session.h"],
srcs = [
"envoy_quic_server_session.cc",
"envoy_quic_server_stream.cc",
],
hdrs = [
"envoy_quic_server_session.h",
"envoy_quic_server_stream.h",
],
tags = ["nofips"],
deps = [
":envoy_quic_server_stream_lib",
":envoy_quic_stream_lib",
":envoy_quic_utils_lib",
":quic_filter_manager_connection_lib",
"//source/common/buffer:buffer_lib",
"//source/common/common:assert_lib",
"//source/common/http:header_map_lib",
"//source/extensions/quic_listeners/quiche/platform:quic_platform_mem_slice_storage_impl_lib",
"@com_googlesource_quiche//:quic_core_http_spdy_session_lib",
],
)
Expand Down Expand Up @@ -207,6 +210,12 @@ envoy_cc_library(
],
)

envoy_cc_library(
name = "envoy_quic_simulated_watermark_buffer_lib",
hdrs = ["envoy_quic_simulated_watermark_buffer.h"],
deps = ["//source/common/common:assert_lib"],
)

envoy_cc_library(
name = "active_quic_listener_lib",
srcs = ["active_quic_listener.cc"],
Expand Down
28 changes: 22 additions & 6 deletions source/extensions/quic_listeners/quiche/codec_impl.cc
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()) {

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.

I am completely failing to find what is_static checks. can you clue me in?

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.

is_static is true for static streams, for IETF QUIC, it's crypto stream, and for GQuic, also headers stream.

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.

Nowadays, all the streams are managed in one place: QuicSession::stream_map_

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.

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
"we do not yet cap bytes for static streams, like the gQUIC headers stream or the crypto stream" to make it clear what is_static means

@danzh2010 danzh2010 Oct 16, 2019

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.

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?

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.

Not having a headers stream helps, but it'd be good to have hard bounds on the crypto stream too.
Again maybe we can have a TODO which admits it will be solved by #1719 but as that's closed and not actively being worked on, I think we need something tracking it as an active problem

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.

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?

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.

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();

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.

Why do we need the dynamic cast here and below? Can the stored stream just derive from the right interface?

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.

stream_map() is a QUICHE interface, for sure it doesn't know about Envoy stream interface.

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 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?

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.

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.

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.

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?

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.

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() {
Expand Down
10 changes: 4 additions & 6 deletions source/extensions/quic_listeners/quiche/codec_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,10 @@ class QuicHttpConnectionImplBase : public virtual Http::Connection,
// TODO(danzh) add Http3 enum value for QUIC.
return Http::Protocol::Http2;
}

// Returns true if the session has data to send but queued in connection or
// stream send buffer.
bool wantsToWrite() override;
void onUnderlyingConnectionAboveWriteBufferHighWatermark() override;
void onUnderlyingConnectionBelowWriteBufferLowWatermark() override;

protected:
quic::QuicSpdySession& quic_session_;
Expand All @@ -41,17 +40,16 @@ class QuicHttpServerConnectionImpl : public QuicHttpConnectionImplBase,
public Http::ServerConnection {
public:
QuicHttpServerConnectionImpl(EnvoyQuicServerSession& quic_session,
Http::ServerConnectionCallbacks& callbacks)
: QuicHttpConnectionImplBase(quic_session), quic_server_session_(quic_session) {
quic_session.setHttpConnectionCallbacks(callbacks);
}
Http::ServerConnectionCallbacks& callbacks);

// Http::Connection
void goAway() override;
void shutdownNotice() override {
// TODO(danzh): Add double-GOAWAY support in QUIC.
ENVOY_CONN_LOG(error, "Shutdown notice is not propagated to QUIC.", quic_server_session_);
}
void onUnderlyingConnectionAboveWriteBufferHighWatermark() override;
void onUnderlyingConnectionBelowWriteBufferLowWatermark() override;

private:
EnvoyQuicServerSession& quic_server_session_;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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

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.

This seems pretty huge compared to what we typically recommend for HTTP/2 at the edge. Will we eventually make this configurable?

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.

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,
Expand All @@ -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.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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),

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.

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.

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.

Defer to #8496 so the refactoring can be tested. TODO added.

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.

Thanks!

quic_connection_(std::move(connection)) {}

EnvoyQuicServerSession::~EnvoyQuicServerSession() {
QuicFilterManagerConnectionImpl::quic_connection_ = nullptr;
}

absl::string_view EnvoyQuicServerSession::requestedServerName() const {
return {GetCryptoStream()->crypto_negotiated_params().sni};
Expand All @@ -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;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,9 @@ class EnvoyQuicServerSession : public quic::QuicServerSessionBase,
quic::QuicCryptoServerStream::Helper* helper,
const quic::QuicCryptoServerConfig* crypto_config,
quic::QuicCompressedCertsCache* compressed_certs_cache,
Event::Dispatcher& dispatcher);
Event::Dispatcher& dispatcher, uint32_t send_buffer_limit);

~EnvoyQuicServerSession() override;

// Network::Connection
absl::string_view requestedServerName() const override;
Expand All @@ -48,6 +50,8 @@ class EnvoyQuicServerSession : public quic::QuicServerSessionBase,
// quic::QuicSpdySession
void OnCryptoHandshakeEvent(CryptoHandshakeEvent event) override;

using quic::QuicSession::stream_map;

protected:
// quic::QuicServerSessionBase
quic::QuicCryptoServerStreamBase*
Expand All @@ -64,6 +68,7 @@ class EnvoyQuicServerSession : public quic::QuicServerSessionBase,
private:
void setUpRequestDecoder(EnvoyQuicStream& stream);

std::unique_ptr<EnvoyQuicConnection> quic_connection_;
// These callbacks are owned by network filters and quic session should out live
// them.
Http::ServerConnectionCallbacks* http_connection_callbacks_{nullptr};
Expand Down
Loading