From 65bcd93dbdd1517961c9148fecc8ad16a906755e Mon Sep 17 00:00:00 2001 From: danzh Date: Thu, 8 Aug 2024 17:05:16 -0400 Subject: [PATCH] quic: fix stream reset error stats (#35548) Commit Message: fix a stats error where outgoing QUIC reset error stats is mistakenly incremented both when sending RESET_STREAM frame and receiving RESET_STREAM frame in MaybeSendRstStreamFrame(). As a result, `.tx.quic_reset_stream_error_code_XXX` is counting `.rx.quic_reset_stream_error_code_XXX` into it. Additional Description: This PR also refactor the stats increment into QuicFilterManagerConnectionImpl shared between client and server sessions. Risk Level: low, stats only Testing: new unit tests Docs Changes: N/A Release Notes: N/A Platform Specific Features: N/A Signed-off-by: Dan Zhang Co-authored-by: Dan Zhang --- source/common/quic/BUILD | 1 + .../common/quic/envoy_quic_client_session.cc | 21 +++++-------- .../common/quic/envoy_quic_client_session.h | 4 --- .../common/quic/envoy_quic_client_stream.cc | 2 ++ .../common/quic/envoy_quic_server_session.cc | 23 +++++--------- .../common/quic/envoy_quic_server_session.h | 5 --- .../common/quic/envoy_quic_server_stream.cc | 2 ++ .../quic_filter_manager_connection_impl.cc | 9 +++++- .../quic_filter_manager_connection_impl.h | 9 +++++- .../quic/envoy_quic_server_session_test.cc | 31 +++++++++++++++++++ .../quic/envoy_quic_server_stream_test.cc | 5 ++- ...uic_filter_manager_connection_impl_test.cc | 13 +++++--- test/common/quic/test_utils.h | 6 ++-- 13 files changed, 84 insertions(+), 47 deletions(-) diff --git a/source/common/quic/BUILD b/source/common/quic/BUILD index 8c4cded47bff..ecb713acc45d 100644 --- a/source/common/quic/BUILD +++ b/source/common/quic/BUILD @@ -258,6 +258,7 @@ envoy_cc_library( ":envoy_quic_simulated_watermark_buffer_lib", ":quic_network_connection_lib", ":quic_ssl_connection_info_lib", + ":quic_stat_names_lib", ":send_buffer_monitor_lib", "//envoy/event:dispatcher_interface", "//envoy/network:connection_interface", diff --git a/source/common/quic/envoy_quic_client_session.cc b/source/common/quic/envoy_quic_client_session.cc index b6ac762f7b43..aed2ac6115d8 100644 --- a/source/common/quic/envoy_quic_client_session.cc +++ b/source/common/quic/envoy_quic_client_session.cc @@ -85,12 +85,12 @@ EnvoyQuicClientSession::EnvoyQuicClientSession( std::make_unique( dispatcher.timeSource(), connection->connectionSocket()->connectionInfoProviderSharedPtr(), - StreamInfo::FilterState::LifeSpan::Connection)), + StreamInfo::FilterState::LifeSpan::Connection), + quic_stat_names, scope), quic::QuicSpdyClientSession(config, supported_versions, connection.release(), server_id, crypto_config.get()), crypto_config_(crypto_config), crypto_stream_factory_(crypto_stream_factory), - quic_stat_names_(quic_stat_names), rtt_cache_(rtt_cache), scope_(scope), - transport_socket_options_(transport_socket_options), + rtt_cache_(rtt_cache), transport_socket_options_(transport_socket_options), transport_socket_factory_(makeOptRefFromPtr( dynamic_cast(transport_socket_factory.ptr()))) { streamInfo().setUpstreamInfo(std::make_shared()); @@ -137,7 +137,8 @@ void EnvoyQuicClientSession::OnConnectionClosed(const quic::QuicConnectionCloseF } } quic::QuicSpdyClientSession::OnConnectionClosed(frame, source); - quic_stat_names_.chargeQuicConnectionCloseStats(scope_, frame.quic_error_code, source, true); + quic_stat_names_.chargeQuicConnectionCloseStats(stats_scope_, frame.quic_error_code, source, + true); onConnectionCloseEvent(frame, source, version()); } @@ -163,18 +164,10 @@ void EnvoyQuicClientSession::OnHttp3GoAway(uint64_t stream_id) { } } -void EnvoyQuicClientSession::MaybeSendRstStreamFrame(quic::QuicStreamId id, - quic::QuicResetStreamError error, - quic::QuicStreamOffset bytes_written) { - QuicSpdyClientSession::MaybeSendRstStreamFrame(id, error, bytes_written); - quic_stat_names_.chargeQuicResetStreamErrorStats(scope_, error, /*from_self*/ true, - /*is_upstream*/ true); -} - void EnvoyQuicClientSession::OnRstStream(const quic::QuicRstStreamFrame& frame) { QuicSpdyClientSession::OnRstStream(frame); - quic_stat_names_.chargeQuicResetStreamErrorStats(scope_, frame.error(), - /*from_self*/ false, /*is_upstream*/ true); + incrementSentQuicResetStreamErrorStats(frame.error(), + /*from_self*/ false, /*is_upstream*/ true); } void EnvoyQuicClientSession::OnCanCreateNewOutgoingStream(bool unidirectional) { diff --git a/source/common/quic/envoy_quic_client_session.h b/source/common/quic/envoy_quic_client_session.h index 5e88f53e60d8..5ef804e3e8ea 100644 --- a/source/common/quic/envoy_quic_client_session.h +++ b/source/common/quic/envoy_quic_client_session.h @@ -59,8 +59,6 @@ class EnvoyQuicClientSession : public QuicFilterManagerConnectionImpl, void OnCanWrite() override; void OnHttp3GoAway(uint64_t stream_id) override; void OnTlsHandshakeComplete() override; - void MaybeSendRstStreamFrame(quic::QuicStreamId id, quic::QuicResetStreamError error, - quic::QuicStreamOffset bytes_written) override; void OnRstStream(const quic::QuicRstStreamFrame& frame) override; void OnNewEncryptionKeyAvailable(quic::EncryptionLevel level, std::unique_ptr encrypter) override; @@ -118,9 +116,7 @@ class EnvoyQuicClientSession : public QuicFilterManagerConnectionImpl, Http::ConnectionCallbacks* http_connection_callbacks_{nullptr}; std::shared_ptr crypto_config_; EnvoyQuicCryptoClientStreamFactoryInterface& crypto_stream_factory_; - QuicStatNames& quic_stat_names_; OptRef rtt_cache_; - Stats::Scope& scope_; bool disable_keepalive_{false}; Network::TransportSocketOptionsConstSharedPtr transport_socket_options_; OptRef transport_socket_factory_; diff --git a/source/common/quic/envoy_quic_client_stream.cc b/source/common/quic/envoy_quic_client_stream.cc index 3e329da66541..848817777945 100644 --- a/source/common/quic/envoy_quic_client_stream.cc +++ b/source/common/quic/envoy_quic_client_stream.cc @@ -369,6 +369,8 @@ void EnvoyQuicClientStream::OnStreamReset(const quic::QuicRstStreamFrame& frame) void EnvoyQuicClientStream::ResetWithError(quic::QuicResetStreamError error) { ENVOY_STREAM_LOG(debug, "sending reset code={}", *this, error.internal_code()); stats_.tx_reset_.inc(); + filterManagerConnection()->incrementSentQuicResetStreamErrorStats(error, /*from_self*/ true, + /*is_upstream*/ true); // Upper layers expect calling resetStream() to immediately raise reset callbacks. runResetCallbacks( quicRstErrorToEnvoyLocalResetReason(error.internal_code()), diff --git a/source/common/quic/envoy_quic_server_session.cc b/source/common/quic/envoy_quic_server_session.cc index fa418b4013db..6ff59b7c4825 100644 --- a/source/common/quic/envoy_quic_server_session.cc +++ b/source/common/quic/envoy_quic_server_session.cc @@ -43,11 +43,12 @@ EnvoyQuicServerSession::EnvoyQuicServerSession( EnvoyQuicConnectionDebugVisitorFactoryInterfaceOptRef debug_visitor_factory) : quic::QuicServerSessionBase(config, supported_versions, connection.get(), visitor, helper, crypto_config, compressed_certs_cache), - QuicFilterManagerConnectionImpl( - *connection, connection->connection_id(), dispatcher, send_buffer_limit, - std::make_shared(*this), std::move(stream_info)), - quic_connection_(std::move(connection)), quic_stat_names_(quic_stat_names), - listener_scope_(listener_scope), crypto_server_stream_factory_(crypto_server_stream_factory), + QuicFilterManagerConnectionImpl(*connection, connection->connection_id(), dispatcher, + send_buffer_limit, + std::make_shared(*this), + std::move(stream_info), quic_stat_names, listener_scope), + quic_connection_(std::move(connection)), + crypto_server_stream_factory_(crypto_server_stream_factory), connection_stats_(connection_stats) { #ifdef ENVOY_ENABLE_HTTP_DATAGRAMS http_datagram_support_ = quic::HttpDatagramSupport::kRfc; @@ -175,18 +176,10 @@ void EnvoyQuicServerSession::OnTlsHandshakeComplete() { raiseConnectionEvent(Network::ConnectionEvent::Connected); } -void EnvoyQuicServerSession::MaybeSendRstStreamFrame(quic::QuicStreamId id, - quic::QuicResetStreamError error, - quic::QuicStreamOffset bytes_written) { - QuicServerSessionBase::MaybeSendRstStreamFrame(id, error, bytes_written); - quic_stat_names_.chargeQuicResetStreamErrorStats(listener_scope_, error, /*from_self*/ true, - /*is_upstream*/ false); -} - void EnvoyQuicServerSession::OnRstStream(const quic::QuicRstStreamFrame& frame) { QuicServerSessionBase::OnRstStream(frame); - quic_stat_names_.chargeQuicResetStreamErrorStats(listener_scope_, frame.error(), - /*from_self*/ false, /*is_upstream*/ false); + incrementSentQuicResetStreamErrorStats(frame.error(), + /*from_self*/ false, /*is_upstream*/ false); } void EnvoyQuicServerSession::setHttp3Options( diff --git a/source/common/quic/envoy_quic_server_session.h b/source/common/quic/envoy_quic_server_session.h index 05ee14081129..62014e9d8854 100644 --- a/source/common/quic/envoy_quic_server_session.h +++ b/source/common/quic/envoy_quic_server_session.h @@ -82,8 +82,6 @@ class EnvoyQuicServerSession : public quic::QuicServerSessionBase, void Initialize() override; void OnCanWrite() override; void OnTlsHandshakeComplete() override; - void MaybeSendRstStreamFrame(quic::QuicStreamId id, quic::QuicResetStreamError error, - quic::QuicStreamOffset bytes_written) override; void OnRstStream(const quic::QuicRstStreamFrame& frame) override; void ProcessUdpPacket(const quic::QuicSocketAddress& self_address, const quic::QuicSocketAddress& peer_address, @@ -137,9 +135,6 @@ class EnvoyQuicServerSession : public quic::QuicServerSessionBase, envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction headers_with_underscores_action_; - QuicStatNames& quic_stat_names_; - Stats::Scope& listener_scope_; - EnvoyQuicCryptoServerStreamFactoryInterface& crypto_server_stream_factory_; absl::optional position_; QuicConnectionStats& connection_stats_; diff --git a/source/common/quic/envoy_quic_server_stream.cc b/source/common/quic/envoy_quic_server_stream.cc index 812025913c59..f3ca7a38a3e3 100644 --- a/source/common/quic/envoy_quic_server_stream.cc +++ b/source/common/quic/envoy_quic_server_stream.cc @@ -359,6 +359,8 @@ void EnvoyQuicServerStream::OnStreamReset(const quic::QuicRstStreamFrame& frame) void EnvoyQuicServerStream::ResetWithError(quic::QuicResetStreamError error) { ENVOY_STREAM_LOG(debug, "sending reset code={}", *this, error.internal_code()); stats_.tx_reset_.inc(); + filterManagerConnection()->incrementSentQuicResetStreamErrorStats(error, /*from_self*/ true, + /*is_upstream*/ false); if (!local_end_stream_) { // Upper layers expect calling resetStream() to immediately raise reset callbacks. runResetCallbacks( diff --git a/source/common/quic/quic_filter_manager_connection_impl.cc b/source/common/quic/quic_filter_manager_connection_impl.cc index 24d837c6116c..520cb822a51a 100644 --- a/source/common/quic/quic_filter_manager_connection_impl.cc +++ b/source/common/quic/quic_filter_manager_connection_impl.cc @@ -13,11 +13,13 @@ QuicFilterManagerConnectionImpl::QuicFilterManagerConnectionImpl( QuicNetworkConnection& connection, const quic::QuicConnectionId& connection_id, Event::Dispatcher& dispatcher, uint32_t send_buffer_limit, std::shared_ptr&& info, - std::unique_ptr&& stream_info) + std::unique_ptr&& stream_info, QuicStatNames& quic_stat_names, + Stats::Scope& stats_scope) // Using this for purpose other than logging is not safe. Because QUIC connection id can be // 18 bytes, so there might be collision when it's hashed to 8 bytes. : Network::ConnectionImplBase(dispatcher, /*id=*/connection_id.Hash()), network_connection_(&connection), quic_ssl_info_(std::move(info)), + quic_stat_names_(quic_stat_names), stats_scope_(stats_scope), filter_manager_( std::make_unique(*this, *connection.connectionSocket())), stream_info_(std::move(stream_info)), @@ -271,5 +273,10 @@ void QuicFilterManagerConnectionImpl::maybeHandleCloseDuringInitialize() { } } +void QuicFilterManagerConnectionImpl::incrementSentQuicResetStreamErrorStats( + quic::QuicResetStreamError error, bool from_self, bool is_upstream) { + quic_stat_names_.chargeQuicResetStreamErrorStats(stats_scope_, error, from_self, is_upstream); +} + } // namespace Quic } // namespace Envoy diff --git a/source/common/quic/quic_filter_manager_connection_impl.h b/source/common/quic/quic_filter_manager_connection_impl.h index 7e366650a935..da591a31b86b 100644 --- a/source/common/quic/quic_filter_manager_connection_impl.h +++ b/source/common/quic/quic_filter_manager_connection_impl.h @@ -13,6 +13,7 @@ #include "source/common/quic/envoy_quic_simulated_watermark_buffer.h" #include "source/common/quic/quic_network_connection.h" #include "source/common/quic/quic_ssl_connection_info.h" +#include "source/common/quic/quic_stat_names.h" #include "source/common/quic/send_buffer_monitor.h" #include "source/common/stream_info/stream_info_impl.h" @@ -35,7 +36,8 @@ class QuicFilterManagerConnectionImpl : public Network::ConnectionImplBase, const quic::QuicConnectionId& connection_id, Event::Dispatcher& dispatcher, uint32_t send_buffer_limit, std::shared_ptr&& info, - std::unique_ptr&& stream_info); + std::unique_ptr&& stream_info, + QuicStatNames& quic_stat_names, Stats::Scope& stats_scope); // Network::FilterManager // Overridden to delegate calls to filter_manager_. void addWriteFilter(Network::WriteFilterSharedPtr filter) override; @@ -171,6 +173,9 @@ class QuicFilterManagerConnectionImpl : public Network::ConnectionImplBase, max_headers_count_ = max_headers_count; } + void incrementSentQuicResetStreamErrorStats(quic::QuicResetStreamError error, bool from_self, + bool is_upstream); + protected: // Propagate connection close to network_connection_callbacks_. void onConnectionCloseEvent(const quic::QuicConnectionCloseFrame& frame, @@ -197,6 +202,8 @@ class QuicFilterManagerConnectionImpl : public Network::ConnectionImplBase, OptRef http3_options_; bool initialized_{false}; std::shared_ptr quic_ssl_info_; + QuicStatNames& quic_stat_names_; + Stats::Scope& stats_scope_; private: friend class Envoy::TestPauseFilterForQuic; diff --git a/test/common/quic/envoy_quic_server_session_test.cc b/test/common/quic/envoy_quic_server_session_test.cc index 4fba3ac27b03..b670a20d8702 100644 --- a/test/common/quic/envoy_quic_server_session_test.cc +++ b/test/common/quic/envoy_quic_server_session_test.cc @@ -376,6 +376,10 @@ TEST_F(EnvoyQuicServerSessionTest, OnResetFrameIetfQuic) { listener_config_.listenerScope().store(), "http3.downstream.rx.quic_reset_stream_error_code_QUIC_ERROR_PROCESSING_STREAM") ->value()); + EXPECT_EQ(nullptr, + TestUtility::findCounter( + listener_config_.listenerScope().store(), + "http3.downstream.tx.quic_reset_stream_error_code_QUIC_ERROR_PROCESSING_STREAM")); EXPECT_CALL(http_connection_callbacks_, newStream(_, false)) .WillOnce(Invoke([&request_decoder, &stream_callbacks](Http::ResponseEncoder& encoder, @@ -403,6 +407,9 @@ TEST_F(EnvoyQuicServerSessionTest, OnResetFrameIetfQuic) { listener_config_.listenerScope().store(), "http3.downstream.rx.quic_reset_stream_error_code_QUIC_REFUSED_STREAM") ->value()); + EXPECT_EQ(nullptr, TestUtility::findCounter( + listener_config_.listenerScope().store(), + "http3.downstream.tx.quic_reset_stream_error_code_QUIC_REFUSED_STREAM")); EXPECT_CALL(http_connection_callbacks_, newStream(_, false)) .WillOnce(Invoke([&request_decoder, &stream_callbacks](Http::ResponseEncoder& encoder, @@ -424,6 +431,30 @@ TEST_F(EnvoyQuicServerSessionTest, OnResetFrameIetfQuic) { listener_config_.listenerScope().store(), "http3.downstream.rx.quic_reset_stream_error_code_QUIC_REFUSED_STREAM") ->value()); + EXPECT_EQ(nullptr, TestUtility::findCounter( + listener_config_.listenerScope().store(), + "http3.downstream.tx.quic_reset_stream_error_code_QUIC_REFUSED_STREAM")); +} + +TEST_F(EnvoyQuicServerSessionTest, ResetStream) { + installReadFilter(); + + Http::MockRequestDecoder request_decoder; + Http::MockStreamCallbacks stream_callbacks; + EXPECT_CALL(request_decoder, accessLogHandlers()); + auto stream1 = + dynamic_cast(createNewStream(request_decoder, stream_callbacks)); + EXPECT_CALL(stream_callbacks, onResetStream(Http::StreamResetReason::LocalReset, _)); + EXPECT_CALL(*quic_connection_, SendControlFrame(_)); + stream1->resetStream(Http::StreamResetReason::LocalReset); + EXPECT_EQ(1U, TestUtility::findCounter( + listener_config_.listenerScope().store(), + "http3.downstream.tx.quic_reset_stream_error_code_QUIC_STREAM_REQUEST_REJECTED") + ->value()); + EXPECT_EQ(nullptr, + TestUtility::findCounter( + listener_config_.listenerScope().store(), + "http3.downstream.rx.quic_reset_stream_error_code_QUIC_STREAM_REQUEST_REJECTED")); } TEST_F(EnvoyQuicServerSessionTest, ConnectionClose) { diff --git a/test/common/quic/envoy_quic_server_stream_test.cc b/test/common/quic/envoy_quic_server_stream_test.cc index f8f02758f95d..a1958f3afa2d 100644 --- a/test/common/quic/envoy_quic_server_stream_test.cc +++ b/test/common/quic/envoy_quic_server_stream_test.cc @@ -50,8 +50,10 @@ class EnvoyQuicServerStreamTest : public testing::Test { quic_connection_(connection_helper_, alarm_factory_, writer_, quic::ParsedQuicVersionVector{quic_version_}, *listener_config_.socket_, connection_id_generator_), + quic_stat_names_(listener_config_.listenerScope().symbolTable()), quic_session_(quic_config_, {quic_version_}, &quic_connection_, *dispatcher_, - quic_config_.GetInitialStreamFlowControlWindowToSend() * 2), + quic_config_.GetInitialStreamFlowControlWindowToSend() * 2, quic_stat_names_, + listener_config_.listenerScope()), stats_( {ALL_HTTP3_CODEC_STATS(POOL_COUNTER_PREFIX(listener_config_.listenerScope(), "http3."), POOL_GAUGE_PREFIX(listener_config_.listenerScope(), "http3."))}), @@ -222,6 +224,7 @@ class EnvoyQuicServerStreamTest : public testing::Test { quic::DeterministicConnectionIdGenerator connection_id_generator_{ quic::kQuicDefaultConnectionIdLength}; testing::NiceMock quic_connection_; + Envoy::Quic::QuicStatNames quic_stat_names_; MockEnvoyQuicSession quic_session_; quic::QuicStreamId stream_id_{kStreamId}; Http::Http3::CodecStats stats_; diff --git a/test/common/quic/quic_filter_manager_connection_impl_test.cc b/test/common/quic/quic_filter_manager_connection_impl_test.cc index 1217c6d69d46..a4ddd7600825 100644 --- a/test/common/quic/quic_filter_manager_connection_impl_test.cc +++ b/test/common/quic/quic_filter_manager_connection_impl_test.cc @@ -16,13 +16,15 @@ class TestQuicFilterManagerConnectionImpl : public QuicFilterManagerConnectionIm TestQuicFilterManagerConnectionImpl(QuicNetworkConnection& connection, const quic::QuicConnectionId& connection_id, Event::Dispatcher& dispatcher, uint32_t send_buffer_limit, - std::shared_ptr&& ssl_info) + std::shared_ptr&& ssl_info, + QuicStatNames& quic_stat_names, Stats::Scope& scope) : QuicFilterManagerConnectionImpl( connection, connection_id, dispatcher, send_buffer_limit, std::move(ssl_info), std::make_unique( dispatcher.timeSource(), connection.connectionSocket()->connectionInfoProviderSharedPtr(), - StreamInfo::FilterState::LifeSpan::Connection)) {} + StreamInfo::FilterState::LifeSpan::Connection), + quic_stat_names, scope) {} void dumpState(std::ostream& /*os*/, int /*indent_level = 0*/) const override {} absl::string_view requestedServerName() const override { return {}; } @@ -42,11 +44,12 @@ class QuicFilterManagerConnectionImplTest : public ::testing::Test { public: QuicFilterManagerConnectionImplTest() : socket_(std::make_unique>()), - connection_(std::move(socket_)), + quic_stat_names_(store_.symbolTable()), connection_(std::move(socket_)), quic_session_(new quic::test::MockQuicConnection(&helper_, &alarm_factory_, quic::Perspective::IS_SERVER)), ssl_info_(std::make_shared(quic_session_)), - impl_(connection_, connection_id_, dispatcher_, send_buffer_limit_, std::move(ssl_info_)) {} + impl_(connection_, connection_id_, dispatcher_, send_buffer_limit_, std::move(ssl_info_), + quic_stat_names_, *store_.rootScope()) {} protected: std::unique_ptr> socket_; @@ -55,6 +58,8 @@ class QuicFilterManagerConnectionImplTest : public ::testing::Test { uint32_t send_buffer_limit_ = 0; quic::test::MockQuicConnectionHelper helper_; quic::test::MockAlarmFactory alarm_factory_; + Stats::IsolatedStoreImpl store_; + QuicStatNames quic_stat_names_; QuicNetworkConnection connection_; quic::test::MockQuicSession quic_session_; std::shared_ptr ssl_info_; diff --git a/test/common/quic/test_utils.h b/test/common/quic/test_utils.h index 38f73504af8c..c89c33e70ea0 100644 --- a/test/common/quic/test_utils.h +++ b/test/common/quic/test_utils.h @@ -97,14 +97,16 @@ class MockEnvoyQuicSession : public quic::QuicSpdySession, public QuicFilterMana MockEnvoyQuicSession(const quic::QuicConfig& config, const quic::ParsedQuicVersionVector& supported_versions, EnvoyQuicServerConnection* connection, Event::Dispatcher& dispatcher, - uint32_t send_buffer_limit) + uint32_t send_buffer_limit, QuicStatNames& quic_stat_names, + Stats::Scope& scope) : quic::QuicSpdySession(connection, /*visitor=*/nullptr, config, supported_versions), QuicFilterManagerConnectionImpl( *connection, connection->connection_id(), dispatcher, send_buffer_limit, {nullptr}, std::make_unique( dispatcher.timeSource(), connection->connectionSocket()->connectionInfoProviderSharedPtr(), - StreamInfo::FilterState::LifeSpan::Connection)), + StreamInfo::FilterState::LifeSpan::Connection), + quic_stat_names, scope), crypto_stream_(std::make_unique(this)) {} void Initialize() override {