From f68f942dd44d32f00ba39cdeecf28a1ee1dc150c Mon Sep 17 00:00:00 2001 From: danzh Date: Thu, 29 Apr 2021 09:21:05 -0400 Subject: [PATCH] quiche: use max header size configured in HCM (#15912) Commit Message: use max header size in HCM config to initialize quic session. Additional Description: change QuicSession::Initialize() and filter chain creation order to set max header list size for each session before Initialize(). Move Initialize() of Http3 connection from CodecClient to CodecClientProd. Added CodecClient::connect() interface. Change EnvoyQuicConnection not to directly inherit from QuicConnection. And renamed it. Fix a use-after-free bug in FakeUpstream which causes ASAN failure. Risk Level: low Testing: more protocol H3 tests Part of #2557 #12930 #14829 Signed-off-by: Dan Zhang Signed-off-by: Gokul Nair --- source/common/http/codec_client.cc | 2 +- source/common/quic/BUILD | 1 - .../quic/client_connection_factory_impl.cc | 9 +++---- source/common/quic/codec_impl.cc | 6 ++--- source/common/quic/envoy_quic_dispatcher.cc | 6 +++++ source/common/quic/envoy_quic_utils.cc | 26 ------------------- source/common/quic/envoy_quic_utils.h | 4 --- test/integration/protocol_integration_test.cc | 19 +++++--------- 8 files changed, 20 insertions(+), 53 deletions(-) diff --git a/source/common/http/codec_client.cc b/source/common/http/codec_client.cc index 3353ff81d090..7ea1916839b6 100644 --- a/source/common/http/codec_client.cc +++ b/source/common/http/codec_client.cc @@ -193,7 +193,7 @@ CodecClientProd::CodecClientProd(Type type, Network::ClientConnectionPtr&& conne auto& quic_session = dynamic_cast(*connection_); codec_ = std::make_unique( quic_session, *this, host->cluster().http3CodecStats(), host->cluster().http3Options(), - Http::DEFAULT_MAX_REQUEST_HEADERS_KB, host->cluster().maxResponseHeadersCount()); + Http::DEFAULT_MAX_REQUEST_HEADERS_KB); // Initialize the session after max request header size is changed in above http client // connection creation. quic_session.Initialize(); diff --git a/source/common/quic/BUILD b/source/common/quic/BUILD index be256eccbf6b..c4f1de6f8bb9 100644 --- a/source/common/quic/BUILD +++ b/source/common/quic/BUILD @@ -371,7 +371,6 @@ envoy_cc_library( "//source/common/network:socket_option_factory_lib", "//source/common/quic:quic_io_handle_wrapper_lib", "//source/extensions/transport_sockets:well_known_names", - "@com_googlesource_quiche//:quic_core_config_lib", "@com_googlesource_quiche//:quic_core_http_header_list_lib", "@envoy_api//envoy/config/core/v3:pkg_cc_proto", "@envoy_api//envoy/config/listener/v3:pkg_cc_proto", diff --git a/source/common/quic/client_connection_factory_impl.cc b/source/common/quic/client_connection_factory_impl.cc index c65b56614704..3bfab560b7e7 100644 --- a/source/common/quic/client_connection_factory_impl.cc +++ b/source/common/quic/client_connection_factory_impl.cc @@ -21,10 +21,9 @@ PersistentQuicInfoImpl::PersistentQuicInfoImpl( : conn_helper_(dispatcher), alarm_factory_(dispatcher, *conn_helper_.GetClock()), server_id_{getConfig(transport_socket_factory).serverNameIndication(), static_cast(server_addr->ip()->port()), false}, - crypto_config_(std::make_unique( - std::make_unique(stats_scope, getConfig(transport_socket_factory), - time_source), - std::make_unique(time_source))) { + crypto_config_( + std::make_unique(std::make_unique( + stats_scope, getConfig(transport_socket_factory), time_source))) { quiche::FlagRegistry::getInstance(); } @@ -60,7 +59,7 @@ createQuicNetworkConnection(Http::PersistentQuicInfo& info, Event::Dispatcher& d auto ret = std::make_unique( info_impl->quic_config_, info_impl->supported_versions_, std::move(connection), info_impl->server_id_, info_impl->crypto_config_.get(), &static_info.push_promise_index_, - dispatcher, /*send_buffer_limit=*/0); + dispatcher, 0); return ret; } diff --git a/source/common/quic/codec_impl.cc b/source/common/quic/codec_impl.cc index 6ec3c355e023..5cdae4dd8e96 100644 --- a/source/common/quic/codec_impl.cc +++ b/source/common/quic/codec_impl.cc @@ -22,7 +22,7 @@ QuicHttpServerConnectionImpl::QuicHttpServerConnectionImpl( EnvoyQuicServerSession& quic_session, Http::ServerConnectionCallbacks& callbacks, Http::Http3::CodecStats& stats, const envoy::config::core::v3::Http3ProtocolOptions& http3_options, - const uint32_t max_request_headers_kb, const uint32_t max_request_headers_count, + const uint32_t max_request_headers_kb, envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction headers_with_underscores_action) : QuicHttpConnectionImplBase(quic_session, stats), quic_server_session_(quic_session) { @@ -30,7 +30,6 @@ QuicHttpServerConnectionImpl::QuicHttpServerConnectionImpl( quic_session.setHttp3Options(http3_options); quic_session.setHeadersWithUnderscoreAction(headers_with_underscores_action); quic_session.setHttpConnectionCallbacks(callbacks); - quic_session.setMaxIncomingHeadersCount(max_request_headers_count); quic_session.set_max_inbound_header_list_size(max_request_headers_kb * 1024u); } @@ -70,12 +69,11 @@ QuicHttpClientConnectionImpl::QuicHttpClientConnectionImpl( EnvoyQuicClientSession& session, Http::ConnectionCallbacks& callbacks, Http::Http3::CodecStats& stats, const envoy::config::core::v3::Http3ProtocolOptions& http3_options, - const uint32_t max_request_headers_kb, const uint32_t max_response_headers_count) + const uint32_t max_request_headers_kb) : QuicHttpConnectionImplBase(session, stats), quic_client_session_(session) { session.setCodecStats(stats); session.setHttp3Options(http3_options); session.setHttpConnectionCallbacks(callbacks); - session.setMaxIncomingHeadersCount(max_response_headers_count); session.set_max_inbound_header_list_size(max_request_headers_kb * 1024); } diff --git a/source/common/quic/envoy_quic_dispatcher.cc b/source/common/quic/envoy_quic_dispatcher.cc index f1577a40ce52..862683019404 100644 --- a/source/common/quic/envoy_quic_dispatcher.cc +++ b/source/common/quic/envoy_quic_dispatcher.cc @@ -52,6 +52,12 @@ std::unique_ptr EnvoyQuicDispatcher::CreateQuicSession( const quic::QuicSocketAddress& peer_address, absl::string_view alpn, const quic::ParsedQuicVersion& version, absl::string_view sni) { quic::QuicConfig quic_config = config(); + // TODO(danzh) setup flow control window via config. + quic_config.SetInitialStreamFlowControlWindowToSend( + Http2::Utility::OptionsLimits::MIN_INITIAL_STREAM_WINDOW_SIZE); + quic_config.SetInitialSessionFlowControlWindowToSend( + 1.5 * Http2::Utility::OptionsLimits::MIN_INITIAL_STREAM_WINDOW_SIZE); + Network::ConnectionSocketPtr connection_socket = createServerConnectionSocket( listen_socket_.ioHandle(), self_address, peer_address, std::string(sni), alpn); const Network::FilterChain* filter_chain = diff --git a/source/common/quic/envoy_quic_utils.cc b/source/common/quic/envoy_quic_utils.cc index 969782322f09..a32c44671edb 100644 --- a/source/common/quic/envoy_quic_utils.cc +++ b/source/common/quic/envoy_quic_utils.cc @@ -244,31 +244,5 @@ createServerConnectionSocket(Network::IoHandle& io_handle, return connection_socket; } -void configQuicInitialFlowControlWindow(const envoy::config::core::v3::QuicProtocolOptions& config, - quic::QuicConfig& quic_config) { - size_t stream_flow_control_window_to_send = PROTOBUF_GET_WRAPPED_OR_DEFAULT( - config, initial_stream_window_size, - Http3::Utility::OptionsLimits::DEFAULT_INITIAL_STREAM_WINDOW_SIZE); - if (stream_flow_control_window_to_send < quic::kMinimumFlowControlSendWindow) { - // If the configured value is smaller than 16kB, only use it for IETF QUIC, because Google QUIC - // requires minimum 16kB stream flow control window. The QUICHE default 16kB will be used for - // Google QUIC connections. - quic_config.SetInitialMaxStreamDataBytesIncomingBidirectionalToSend( - stream_flow_control_window_to_send); - } else { - // Both Google QUIC and IETF Quic can be configured from this. - quic_config.SetInitialStreamFlowControlWindowToSend(stream_flow_control_window_to_send); - } - - uint32_t session_flow_control_window_to_send = PROTOBUF_GET_WRAPPED_OR_DEFAULT( - config, initial_connection_window_size, - Http3::Utility::OptionsLimits::DEFAULT_INITIAL_CONNECTION_WINDOW_SIZE); - // Config connection level flow control window shouldn't be smaller than the minimum flow control - // window supported in QUICHE which is 16kB. - quic_config.SetInitialSessionFlowControlWindowToSend( - std::max(quic::kMinimumFlowControlSendWindow, - static_cast(session_flow_control_window_to_send))); -} - } // namespace Quic } // namespace Envoy diff --git a/source/common/quic/envoy_quic_utils.h b/source/common/quic/envoy_quic_utils.h index 2625e7acc0e0..6305830e28c3 100644 --- a/source/common/quic/envoy_quic_utils.h +++ b/source/common/quic/envoy_quic_utils.h @@ -164,9 +164,5 @@ createServerConnectionSocket(Network::IoHandle& io_handle, const quic::QuicSocketAddress& peer_address, const std::string& hostname, absl::string_view alpn); -// Set initial flow control windows in quic_config according to the given Envoy config. -void configQuicInitialFlowControlWindow(const envoy::config::core::v3::QuicProtocolOptions& config, - quic::QuicConfig& quic_config); - } // namespace Quic } // namespace Envoy diff --git a/test/integration/protocol_integration_test.cc b/test/integration/protocol_integration_test.cc index da18aa66e82b..7b093a89eee8 100644 --- a/test/integration/protocol_integration_test.cc +++ b/test/integration/protocol_integration_test.cc @@ -765,6 +765,7 @@ TEST_P(DownstreamProtocolIntegrationTest, RetryAttemptCountHeader) { // The retry priority will always target P1, which would otherwise never be hit due to P0 being // healthy. TEST_P(DownstreamProtocolIntegrationTest, RetryPriority) { + EXCLUDE_UPSTREAM_HTTP3; // Timed out waiting for new stream. const Upstream::HealthyLoad healthy_priority_load({0u, 100u}); const Upstream::DegradedLoad degraded_priority_load({0u, 100u}); NiceMock retry_priority(healthy_priority_load, @@ -1321,14 +1322,10 @@ TEST_P(DownstreamProtocolIntegrationTest, LargeCookieParsingConcatenated) { // header size to avoid QUIC_HEADERS_TOO_LARGE stream error. config_helper_.addConfigModifier( [&](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager& - hcm) -> void { - hcm.mutable_max_request_headers_kb()->set_value(96); - hcm.mutable_common_http_protocol_options()->mutable_max_headers_count()->set_value(8000); - }); + hcm) -> void { hcm.mutable_max_request_headers_kb()->set_value(96); }); } if (upstreamProtocol() == FakeHttpConnection::Type::HTTP3) { setMaxRequestHeadersKb(96); - setMaxRequestHeadersCount(8000); } initialize(); @@ -1570,12 +1567,8 @@ TEST_P(DownstreamProtocolIntegrationTest, LargeRequestHeadersRejected) { } TEST_P(DownstreamProtocolIntegrationTest, VeryLargeRequestHeadersRejected) { -#ifdef WIN32 - // TODO(alyssawilk, wrowe) debug. - if (upstreamProtocol() == FakeHttpConnection::Type::HTTP3) { - return; - } -#endif + EXCLUDE_DOWNSTREAM_HTTP3; + EXCLUDE_UPSTREAM_HTTP3; // Send one very large 2048 kB (2 MB) header with limit 1024 kB (1 MB) and 100 headers. testLargeRequestHeaders(2048, 1, 1024, 100); } @@ -1605,7 +1598,9 @@ TEST_P(DownstreamProtocolIntegrationTest, ManyRequestHeadersAccepted) { } TEST_P(DownstreamProtocolIntegrationTest, ManyRequestTrailersRejected) { - // Default header (and trailer) count limit is 100. + // QUICHE doesn't limit number of headers. + EXCLUDE_DOWNSTREAM_HTTP3 + // The default configured header (and trailer) count limit is 100. config_helper_.addConfigModifier(setEnableDownstreamTrailersHttp1()); config_helper_.addConfigModifier(setEnableUpstreamTrailersHttp1()); Http::TestRequestTrailerMapImpl request_trailers;