quic: fix stream reset error stats (envoyproxy#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 <[email protected]>
Co-authored-by: Dan Zhang <[email protected]>

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",

source/common/quic/envoy_quic_client_session.cc

@@ -85,12 +85,12 @@ EnvoyQuicClientSession::EnvoyQuicClientSession(
           std::make_unique<StreamInfo::StreamInfoImpl>(
               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<QuicTransportSocketFactoryBase*>(transport_socket_factory.ptr()))) {
   streamInfo().setUpstreamInfo(std::make_shared<StreamInfo::UpstreamInfoImpl>());
@@ -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) {

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<quic::QuicEncrypter> encrypter) override;
@@ -118,9 +116,7 @@ class EnvoyQuicClientSession : public QuicFilterManagerConnectionImpl,
   Http::ConnectionCallbacks* http_connection_callbacks_{nullptr};
   std::shared_ptr<quic::QuicCryptoClientConfig> crypto_config_;
   EnvoyQuicCryptoClientStreamFactoryInterface& crypto_stream_factory_;
-  QuicStatNames& quic_stat_names_;
   OptRef<Http::HttpServerPropertiesCache> rtt_cache_;
-  Stats::Scope& scope_;
   bool disable_keepalive_{false};
   Network::TransportSocketOptionsConstSharedPtr transport_socket_options_;
   OptRef<QuicTransportSocketFactoryBase> transport_socket_factory_;

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

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<QuicSslConnectionInfo>(*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<QuicSslConnectionInfo>(*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(

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<ConnectionMapPosition> position_;
   QuicConnectionStats& connection_stats_;

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(

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<QuicSslConnectionInfo>&& info,
-    std::unique_ptr<StreamInfo::StreamInfo>&& stream_info)
+    std::unique_ptr<StreamInfo::StreamInfo>&& 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<Network::FilterManagerImpl>(*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

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<QuicSslConnectionInfo>&& info,
-                                  std::unique_ptr<StreamInfo::StreamInfo>&& stream_info);
+                                  std::unique_ptr<StreamInfo::StreamInfo>&& 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<const envoy::config::core::v3::Http3ProtocolOptions> http3_options_;
   bool initialized_{false};
   std::shared_ptr<QuicSslConnectionInfo> quic_ssl_info_;
+  QuicStatNames& quic_stat_names_;
+  Stats::Scope& stats_scope_;
 
 private:
   friend class Envoy::TestPauseFilterForQuic;

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<EnvoyQuicServerStream*>(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) {

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<MockEnvoyQuicServerConnection> quic_connection_;
+  Envoy::Quic::QuicStatNames quic_stat_names_;
   MockEnvoyQuicSession quic_session_;
   quic::QuicStreamId stream_id_{kStreamId};
   Http::Http3::CodecStats stats_;

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<QuicSslConnectionInfo>&& ssl_info)
+                                      std::shared_ptr<QuicSslConnectionInfo>&& ssl_info,
+                                      QuicStatNames& quic_stat_names, Stats::Scope& scope)
       : QuicFilterManagerConnectionImpl(
             connection, connection_id, dispatcher, send_buffer_limit, std::move(ssl_info),
             std::make_unique<StreamInfo::StreamInfoImpl>(
                 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<NiceMock<Network::MockConnectionSocket>>()),
-        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<QuicSslConnectionInfo>(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<NiceMock<Network::MockConnectionSocket>> 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<QuicSslConnectionInfo> ssl_info_;