Skip to content

http3: applied cluster buffer limits to http/3 #16466

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.

Sign up for GitHub

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

Merged
alyssawilk merged 16 commits into from
May 25, 2021
Merged

http3: applied cluster buffer limits to http/3 #16466

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
4f47a7f
http3: applied cluster buffer limits to http/3
alyssawilk May 12, 2021
4d67310
format only, no tests
alyssawilk May 12, 2021
eb4b8cc
tests
alyssawilk May 12, 2021
809b70e
Merge branch 'main' into config
alyssawilk May 17, 2021
060eced
whitespace
alyssawilk May 17, 2021
9564a9d
test cleanup
alyssawilk May 17, 2021
e618b0b
test
alyssawilk May 17, 2021
0ed2bc9
buildifier
alyssawilk May 17, 2021
d530af6
test fix
alyssawilk May 18, 2021
97ebb87
comment cleanup
alyssawilk May 18, 2021
56a31b2
Merge branch 'main' into config
alyssawilk May 19, 2021
7ca1e64
Merge branch 'main' into config
alyssawilk May 20, 2021
4032998
fix bad merge
alyssawilk May 20, 2021
bafb1a8
16569 follow-up
alyssawilk May 20, 2021
8ae8ffd
Merge branch 'main' into config
alyssawilk May 20, 2021
4a2a3f8
Merge branch 'main' into config
alyssawilk May 24, 2021
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
5 changes: 3 additions & 2 deletions source/common/http/http3/conn_pool.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,9 @@ Http3ConnPoolImpl::Http3ConnPoolImpl(
source_address = Network::Utility::getLocalAddress(host_address->ip()->version());
}
Network::TransportSocketFactory& transport_socket_factory = host->transportSocketFactory();
quic_info_ = std::make_unique<Quic::PersistentQuicInfoImpl>(dispatcher, transport_socket_factory,
time_source, source_address);
quic_info_ = std::make_unique<Quic::PersistentQuicInfoImpl>(
dispatcher, transport_socket_factory, time_source, source_address,
host->cluster().perConnectionBufferLimitBytes());
setQuicConfigFromClusterConfig(host_->cluster(), quic_info_->quic_config_);
}

Expand Down
10 changes: 5 additions & 5 deletions source/common/quic/client_connection_factory_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,11 +42,13 @@ std::shared_ptr<quic::QuicCryptoClientConfig> PersistentQuicInfoImpl::cryptoConf

PersistentQuicInfoImpl::PersistentQuicInfoImpl(
Event::Dispatcher& dispatcher, Network::TransportSocketFactory& transport_socket_factory,
TimeSource& time_source, Network::Address::InstanceConstSharedPtr server_addr)
TimeSource& time_source, Network::Address::InstanceConstSharedPtr server_addr,
uint32_t buffer_limit)
: conn_helper_(dispatcher), alarm_factory_(dispatcher, *conn_helper_.GetClock()),
server_id_{getConfig(transport_socket_factory).serverNameIndication(),
static_cast<uint16_t>(server_addr->ip()->port()), false},
transport_socket_factory_(transport_socket_factory), time_source_(time_source) {
transport_socket_factory_(transport_socket_factory), time_source_(time_source),
buffer_limit_(buffer_limit) {
quiche::FlagRegistry::getInstance();
}

Expand All @@ -69,12 +71,10 @@ createQuicNetworkConnection(Http::PersistentQuicInfo& info, Event::Dispatcher& d

ASSERT(!info_impl->supported_versions_.empty());
// QUICHE client session always use the 1st version to start handshake.
// TODO(alyssawilk) pass in ClusterInfo::perConnectionBufferLimitBytes() for
// send_buffer_limit instead of using 0.
auto ret = std::make_unique<EnvoyQuicClientSession>(
info_impl->quic_config_, info_impl->supported_versions_, std::move(connection),
info_impl->server_id_, std::move(config), &info_impl->push_promise_index_, dispatcher,
/*send_buffer_limit=*/0);
info_impl->buffer_limit_);
return ret;
}

Expand Down
5 changes: 4 additions & 1 deletion source/common/quic/client_connection_factory_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,8 @@ struct PersistentQuicInfoImpl : public Http::PersistentQuicInfo {
PersistentQuicInfoImpl(Event::Dispatcher& dispatcher,
Network::TransportSocketFactory& transport_socket_factory,
TimeSource& time_source,
Network::Address::InstanceConstSharedPtr server_addr);
Network::Address::InstanceConstSharedPtr server_addr,
uint32_t buffer_limit);

// Returns the most recent crypto config from transport_socket_factory_;
std::shared_ptr<quic::QuicCryptoClientConfig> cryptoConfig();
Expand All @@ -42,6 +43,8 @@ struct PersistentQuicInfoImpl : public Http::PersistentQuicInfo {
const quic::ParsedQuicVersionVector supported_versions_{quic::CurrentSupportedVersions()};
// TODO(alyssawilk) actually set this up properly.
quic::QuicConfig quic_config_;
// The cluster buffer limits.
const uint32_t buffer_limit_;
// This arguably should not be shared across connections but as Envoy doesn't
// support push promise it's really moot point.
quic::QuicClientPushPromiseIndex push_promise_index_;
Expand Down
4 changes: 3 additions & 1 deletion source/common/quic/envoy_quic_proof_verifier.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,9 @@ namespace Quic {
class EnvoyQuicProofVerifier : public EnvoyQuicProofVerifierBase {
public:
EnvoyQuicProofVerifier(Envoy::Ssl::ClientContextSharedPtr&& context)
: context_(std::move(context)) {}
: context_(std::move(context)) {
ASSERT(context_.get());
}

Comment on lines 15 to 18
Copy link
Copy Markdown
Contributor

@snowp snowp May 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this change belong here? Not opposed to it, just seems unrelated to the main change in this PR

Copy link
Copy Markdown
Contributor Author

@alyssawilk alyssawilk May 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it was a follow-up to a change requested in another PR.
I can split it into its own thing, but didn't think it was worth the CI cost :-P

Copy link
Copy Markdown
Contributor

@snowp snowp May 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine with me as long as we're aware that we're doing it!

// EnvoyQuicProofVerifierBase
quic::QuicAsyncStatus
Expand Down
3 changes: 3 additions & 0 deletions source/common/quic/quic_filter_manager_connection_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@ class TestPauseFilterForQuic;

namespace Quic {

class QuicNetworkConnectionTest;

// Act as a Network::Connection to HCM and a FilterManager to FilterFactoryCb.
class QuicFilterManagerConnectionImpl : public Network::ConnectionImplBase,
public SendBufferMonitor {
Expand Down Expand Up @@ -171,6 +173,7 @@ class QuicFilterManagerConnectionImpl : public Network::ConnectionImplBase,

private:
friend class Envoy::TestPauseFilterForQuic;
friend class Envoy::Quic::QuicNetworkConnectionTest;

// Called when aggregated buffered bytes across all the streams exceeds high watermark.
void onSendBufferHighWatermark();
Expand Down
35 changes: 17 additions & 18 deletions test/common/http/http3/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,22 +13,21 @@ envoy_cc_test(
name = "conn_pool_test",
srcs = envoy_select_enable_http3(["conn_pool_test.cc"]),
tags = ["nofips"],
deps =
envoy_select_enable_http3([
"//source/common/event:dispatcher_lib",
"//source/common/http/http3:conn_pool_lib",
"//source/common/network:utility_lib",
"//source/common/upstream:upstream_includes",
"//source/common/upstream:upstream_lib",
"//test/common/http:common_lib",
"//test/common/upstream:utility_lib",
"//test/mocks/event:event_mocks",
"//test/mocks/http:http_mocks",
"//test/mocks/network:network_mocks",
"//test/mocks/runtime:runtime_mocks",
"//test/mocks/server:transport_socket_factory_context_mocks",
"//test/mocks/upstream:cluster_info_mocks",
"//test/mocks/upstream:transport_socket_match_mocks",
"//test/test_common:test_runtime_lib",
]),
deps = envoy_select_enable_http3([
"//source/common/event:dispatcher_lib",
"//source/common/http/http3:conn_pool_lib",
"//source/common/network:utility_lib",
"//source/common/upstream:upstream_includes",
"//source/common/upstream:upstream_lib",
"//test/common/http:common_lib",
"//test/common/upstream:utility_lib",
"//test/mocks/event:event_mocks",
"//test/mocks/http:http_mocks",
"//test/mocks/network:network_mocks",
"//test/mocks/runtime:runtime_mocks",
"//test/mocks/server:transport_socket_factory_context_mocks",
"//test/mocks/upstream:cluster_info_mocks",
"//test/mocks/upstream:transport_socket_match_mocks",
"//test/test_common:test_runtime_lib",
]),
)
6 changes: 6 additions & 0 deletions test/common/http/http3/conn_pool_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ class Http3ConnPoolImplTest : public Event::TestUsingSimulatedTime, public testi
Upstream::MockHost& mockHost() { return static_cast<Upstream::MockHost&>(*host_); }

NiceMock<Event::MockDispatcher> dispatcher_;
std::shared_ptr<Upstream::MockClusterInfo> cluster_{new NiceMock<Upstream::MockClusterInfo>()};
Upstream::HostSharedPtr host_{new NiceMock<Upstream::MockHost>};
NiceMock<Random::MockRandomGenerator> random_;
Upstream::ClusterConnectivityState state_;
Expand All @@ -64,6 +65,11 @@ class Http3ConnPoolImplTest : public Event::TestUsingSimulatedTime, public testi
ConnectionPool::InstancePtr pool_;
};

TEST_F(Http3ConnPoolImplTest, CreationWithBufferLimits) {
EXPECT_CALL(mockHost().cluster_, perConnectionBufferLimitBytes);
initialize();
}

TEST_F(Http3ConnPoolImplTest, CreationWithConfig) {
// Set a couple of options from setQuicConfigFromClusterConfig to make sure they are applied.
auto* options = mockHost().cluster_.http3_options_.mutable_quic_protocol_options();
Expand Down
7 changes: 6 additions & 1 deletion test/common/quic/client_connection_factory_impl_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,10 @@ class QuicNetworkConnectionTest : public Event::TestUsingSimulatedTime, public t
context_);
}

uint32_t highWatermark(EnvoyQuicClientSession* session) {
return session->write_buffer_watermark_simulation_.highWatermark();
}

NiceMock<Event::MockDispatcher> dispatcher_;
std::shared_ptr<Upstream::MockClusterInfo> cluster_{new NiceMock<Upstream::MockClusterInfo>()};
Upstream::HostSharedPtr host_{new NiceMock<Upstream::MockHost>};
Expand All @@ -41,7 +45,7 @@ class QuicNetworkConnectionTest : public Event::TestUsingSimulatedTime, public t
TEST_F(QuicNetworkConnectionTest, BufferLimits) {
Copy link
Copy Markdown
Contributor

@danzh2010 danzh2010 May 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

initialize();

PersistentQuicInfoImpl info{dispatcher_, *factory_, simTime(), test_address_};
PersistentQuicInfoImpl info{dispatcher_, *factory_, simTime(), test_address_, 45};

std::unique_ptr<Network::ClientConnection> client_connection =
createQuicNetworkConnection(info, dispatcher_, test_address_, test_address_);
Expand All @@ -50,6 +54,7 @@ TEST_F(QuicNetworkConnectionTest, BufferLimits) {
client_connection->connect();
EXPECT_TRUE(client_connection->connecting());
ASSERT(session != nullptr);
EXPECT_EQ(highWatermark(session), 45);
EXPECT_EQ(absl::nullopt, session->unixSocketPeerCredentials());
EXPECT_EQ(absl::nullopt, session->lastRoundTripTime());
client_connection->close(Network::ConnectionCloseType::NoFlush);
Expand Down
2 changes: 1 addition & 1 deletion test/integration/http_integration.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -341,7 +341,7 @@ void HttpIntegrationTest::initialize() {
"udp://{}:{}", Network::Test::getLoopbackAddressUrlString(version_), lookupPort("http")));
// Needs to outlive all QUIC connections.
auto quic_connection_persistent_info = std::make_unique<Quic::PersistentQuicInfoImpl>(
*dispatcher_, *quic_transport_socket_factory_, timeSystem(), server_addr);
*dispatcher_, *quic_transport_socket_factory_, timeSystem(), server_addr, 0);
// Config IETF QUIC flow control window.
quic_connection_persistent_info->quic_config_
.SetInitialMaxStreamDataBytesIncomingBidirectionalToSend(
Expand Down
2 changes: 1 addition & 1 deletion test/integration/utility.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,7 +212,7 @@ IntegrationUtil::makeSingleRequest(const Network::Address::InstanceConstSharedPt
"spiffe://lyft.com/backend-team");
std::unique_ptr<Http::PersistentQuicInfo> persistent_info;
persistent_info = std::make_unique<Quic::PersistentQuicInfoImpl>(
*dispatcher, *transport_socket_factory, time_system, addr);
*dispatcher, *transport_socket_factory, time_system, addr, 0);

Network::Address::InstanceConstSharedPtr local_address;
if (addr->ip()->version() == Network::Address::IpVersion::v4) {
Expand Down