quiche: handle connection close during Http::Http3::ActiveClient crea…

…tion (#18056) Commit Message: Quic connection might get closed due to write error during connect(). This will cause the client gets disconnected during creation while assuming it's connecting. This PR fixes it by explicitly checking connection state and fail client creation and checking for early detaching in various place during initialize(). Additional Message: Use getSystemErrorCode() which returns the actual errno in convertToQuicWriteResult() instead of getErrorCode() which returns the corresponding Envoy enum. Risk Level: low Testing: added new conn_pool_grid unit tests Signed-off-by: Dan Zhang <[email protected]>
envoyproxy · Sep 14, 2021 · efcf2e5 · efcf2e5
1 parent 9b106d2
commit efcf2e5
Show file tree

Hide file tree

Showing 10 changed files with 80 additions and 5 deletions.
diff --git a/envoy/api/io_error.h b/envoy/api/io_error.h
@@ -35,6 +35,8 @@ class IoError {
     BadFd,
     // An existing connection was forcibly closed by the remote host.
     ConnectionReset,
+    // Network is unreachable due to network settings.
+    NetworkUnreachable,
     // Other error codes cannot be mapped to any one above in getErrorCode().
     UnknownError
   };

diff --git a/envoy/common/platform.h b/envoy/common/platform.h
@@ -150,6 +150,7 @@ struct msghdr {
 #define SOCKET_ERROR_ADDR_IN_USE WSAEADDRINUSE
 #define SOCKET_ERROR_BADF WSAEBADF
 #define SOCKET_ERROR_CONNRESET WSAECONNRESET
+#define SOCKET_ERROR_NETUNREACH WSAENETUNREACH
 
 #define HANDLE_ERROR_PERM ERROR_ACCESS_DENIED
 #define HANDLE_ERROR_INVALID ERROR_INVALID_HANDLE
@@ -259,6 +260,7 @@ typedef int signal_t;           // NOLINT(modernize-use-using)
 #define SOCKET_ERROR_ADDR_IN_USE EADDRINUSE
 #define SOCKET_ERROR_BADF EBADF
 #define SOCKET_ERROR_CONNRESET ECONNRESET
+#define SOCKET_ERROR_NETUNREACH ENETUNREACH
 
 // Mapping POSIX file errors to common error names
 #define HANDLE_ERROR_PERM EACCES

diff --git a/source/common/http/codec_client.cc b/source/common/http/codec_client.cc
@@ -117,6 +117,9 @@ void CodecClient::onEvent(Network::ConnectionEvent event) {
               StreamInfo::ResponseFlag::UpstreamProtocolError);
         }
       }
+    } else {
+      ENVOY_CONN_LOG(warn, "Connection is closed by {} during connecting.", *connection_,
+                     (event == Network::ConnectionEvent::RemoteClose ? "peer" : "self"));
     }
     while (!active_requests_.empty()) {
       // Fake resetting all active streams so that reset() callbacks get invoked.

diff --git a/source/common/http/http3/conn_pool.cc b/source/common/http/http3/conn_pool.cc
@@ -65,10 +65,15 @@ allocateConnPool(Event::Dispatcher& dispatcher, Random::RandomGenerator& random_
       host, priority, dispatcher, options, transport_socket_options, random_generator, state,
       [&quic_stat_names,
        &scope](HttpConnPoolImplBase* pool) -> ::Envoy::ConnectionPool::ActiveClientPtr {
+        ENVOY_LOG_TO_LOGGER(Envoy::Logger::Registry::getLog(Envoy::Logger::Id::pool), debug,
+                            "Creating Http/3 client");
         // If there's no ssl context, the secrets are not loaded. Fast-fail by returning null.
         auto factory = &pool->host()->transportSocketFactory();
         ASSERT(dynamic_cast<Quic::QuicClientTransportSocketFactory*>(factory) != nullptr);
         if (static_cast<Quic::QuicClientTransportSocketFactory*>(factory)->sslCtx() == nullptr) {
+          ENVOY_LOG_TO_LOGGER(Envoy::Logger::Registry::getLog(Envoy::Logger::Id::pool), warn,
+                              "Failed to create Http/3 client. Transport socket "
+                              "factory is not configured correctly.");
           return nullptr;
         }
         Http3ConnPoolImpl* h3_pool = reinterpret_cast<Http3ConnPoolImpl*>(pool);
@@ -82,7 +87,13 @@ allocateConnPool(Event::Dispatcher& dispatcher, Random::RandomGenerator& random_
         data.connection_ =
             Quic::createQuicNetworkConnection(h3_pool->quicInfo(), pool->dispatcher(), host_address,
                                               source_address, quic_stat_names, scope);
-        return std::make_unique<ActiveClient>(*pool, data);
+        // Store a handle to connection as it will be moved during client construction.
+        Network::Connection& connection = *data.connection_;
+        auto client = std::make_unique<ActiveClient>(*pool, data);
+        if (connection.state() == Network::Connection::State::Closed) {
+          return nullptr;
+        }
+        return client;
       },
       [](Upstream::Host::CreateConnectionData& data, HttpConnPoolImplBase* pool) {
         CodecClientPtr codec{new CodecClientProd(CodecType::HTTP3, std::move(data.connection_),

diff --git a/source/common/network/io_socket_error_impl.cc b/source/common/network/io_socket_error_impl.cc
@@ -58,6 +58,8 @@ Api::IoError::IoErrorCode IoSocketError::errorCodeFromErrno(int sys_errno) {
     return IoErrorCode::BadFd;
   case SOCKET_ERROR_CONNRESET:
     return IoErrorCode::ConnectionReset;
+  case SOCKET_ERROR_NETUNREACH:
+    return IoErrorCode::NetworkUnreachable;
   default:
     ENVOY_LOG_MISC(debug, "Unknown error code {} details {}", sys_errno, errorDetails(sys_errno));
     return IoErrorCode::UnknownError;

diff --git a/source/common/quic/envoy_quic_packet_writer.cc b/source/common/quic/envoy_quic_packet_writer.cc
@@ -16,7 +16,7 @@ quic::WriteResult convertToQuicWriteResult(Api::IoCallUint64Result& result) {
   quic::WriteStatus status = result.err_->getErrorCode() == Api::IoError::IoErrorCode::Again
                                  ? quic::WRITE_STATUS_BLOCKED
                                  : quic::WRITE_STATUS_ERROR;
-  return {status, static_cast<int>(result.err_->getErrorCode())};
+  return {status, static_cast<int>(result.err_->getSystemErrorCode())};
 }
 
 } // namespace

diff --git a/source/common/quic/quic_filter_manager_connection_impl.h b/source/common/quic/quic_filter_manager_connection_impl.h
@@ -82,6 +82,10 @@ class QuicFilterManagerConnectionImpl : public Network::ConnectionImplBase,
   void setConnectionStats(const Network::Connection::ConnectionStats& stats) override {
     // TODO(danzh): populate stats.
     Network::ConnectionImplBase::setConnectionStats(stats);
+    if (network_connection_ == nullptr) {
+      ENVOY_CONN_LOG(error, "Quic connection has been detached.", *this);
+      return;
+    }
     network_connection_->setConnectionStats(stats);
   }
   Ssl::ConnectionInfoConstSharedPtr ssl() const override;

diff --git a/test/common/http/BUILD b/test/common/http/BUILD
@@ -446,6 +446,7 @@ envoy_cc_test(
         "//test/mocks/runtime:runtime_mocks",
         "//test/mocks/stats:stats_mocks",
         "//test/mocks/server:transport_socket_factory_context_mocks",
+        "//test/test_common:threadsafe_singleton_injector_lib",
         "//source/common/quic:quic_factory_lib",
         "//source/common/quic:quic_transport_socket_factory_lib",
         "//source/common/quic:client_connection_factory_lib",

diff --git a/test/common/http/conn_pool_grid_test.cc b/test/common/http/conn_pool_grid_test.cc
@@ -14,6 +14,7 @@
 #include "test/mocks/ssl/mocks.h"
 #include "test/mocks/upstream/cluster_info.h"
 #include "test/test_common/simulated_time_system.h"
+#include "test/test_common/threadsafe_singleton_injector.h"
 #include "test/test_common/utility.h"
 
 #include "gmock/gmock.h"
@@ -680,6 +681,55 @@ TEST_F(ConnectivityGridTest, RealGrid) {
   auto optional_it3 = ConnectivityGridForTest::forceCreateNextPool(grid);
   ASSERT_FALSE(optional_it3.has_value());
 }
+
+TEST_F(ConnectivityGridTest, ConnectionCloseDuringCreation) {
+  EXPECT_CALL(*cluster_, connectTimeout()).WillRepeatedly(Return(std::chrono::seconds(10)));
+
+  testing::InSequence s;
+  dispatcher_.allow_null_callback_ = true;
+  // Set the cluster up to have a quic transport socket.
+  Envoy::Ssl::ClientContextConfigPtr config(new NiceMock<Ssl::MockClientContextConfig>());
+  NiceMock<Server::Configuration::MockTransportSocketFactoryContext> factory_context;
+  Ssl::ClientContextSharedPtr ssl_context(new Ssl::MockClientContext());
+  EXPECT_CALL(factory_context.context_manager_, createSslClientContext(_, _, _))
+      .WillOnce(Return(ssl_context));
+  auto factory =
+      std::make_unique<Quic::QuicClientTransportSocketFactory>(std::move(config), factory_context);
+  factory->initialize();
+  ASSERT_FALSE(factory->usesProxyProtocolOptions());
+  auto& matcher =
+      static_cast<Upstream::MockTransportSocketMatcher&>(*cluster_->transport_socket_matcher_);
+  EXPECT_CALL(matcher, resolve(_))
+      .WillRepeatedly(
+          Return(Upstream::TransportSocketMatcher::MatchData(*factory, matcher.stats_, "test")));
+
+  ConnectivityGrid grid(dispatcher_, random_,
+                        Upstream::makeTestHost(cluster_, "tcp://127.0.0.1:9000", simTime()),
+                        Upstream::ResourcePriority::Default, socket_options_,
+                        transport_socket_options_, state_, simTime(), alternate_protocols_,
+                        std::chrono::milliseconds(300), options_, quic_stat_names_, store_);
+
+  // Create the HTTP/3 pool.
+  auto optional_it1 = ConnectivityGridForTest::forceCreateNextPool(grid);
+  ASSERT_TRUE(optional_it1.has_value());
+  EXPECT_EQ("HTTP/3", (**optional_it1)->protocolDescription());
+
+  Api::MockOsSysCalls os_sys_calls;
+  TestThreadsafeSingletonInjector<Api::OsSysCallsImpl> os_calls(&os_sys_calls);
+  EXPECT_CALL(os_sys_calls, socket(_, _, _)).WillOnce(Return(Api::SysCallSocketResult{1, 0}));
+#if defined(__APPLE__) || defined(WIN32)
+  EXPECT_CALL(os_sys_calls, setsocketblocking(1, false))
+      .WillOnce(Return(Api::SysCallIntResult{1, 0}));
+#endif
+  EXPECT_CALL(os_sys_calls, bind(_, _, _)).WillOnce(Return(Api::SysCallIntResult{1, 0}));
+  EXPECT_CALL(os_sys_calls, setsockopt_(_, _, _, _, _)).WillRepeatedly(Return(0));
+  EXPECT_CALL(os_sys_calls, sendmsg(_, _, _)).WillOnce(Return(Api::SysCallSizeResult{-1, 101}));
+
+  EXPECT_CALL(os_sys_calls, close(1)).WillOnce(Return(Api::SysCallIntResult{0, 0}));
+  ConnectionPool::Cancellable* cancel = (**optional_it1)->newStream(decoder_, callbacks_);
+  EXPECT_EQ(nullptr, cancel);
+}
+
 #endif
 
 } // namespace

diff --git a/test/common/quic/envoy_quic_writer_test.cc b/test/common/quic/envoy_quic_writer_test.cc
@@ -90,7 +90,7 @@ TEST_F(EnvoyQuicWriterTest, SendBlocked) {
   quic::WriteResult result = envoy_quic_writer_.WritePacket(str.data(), str.length(), self_address_,
                                                             peer_address_, nullptr);
   EXPECT_EQ(quic::WRITE_STATUS_BLOCKED, result.status);
-  EXPECT_EQ(static_cast<int>(Api::IoError::IoErrorCode::Again), result.error_code);
+  EXPECT_EQ(SOCKET_ERROR_AGAIN, result.error_code);
   EXPECT_TRUE(envoy_quic_writer_.IsWriteBlocked());
   // Writing while blocked is not allowed.
 #ifdef NDEBUG
@@ -117,7 +117,7 @@ TEST_F(EnvoyQuicWriterTest, SendFailure) {
   quic::WriteResult result = envoy_quic_writer_.WritePacket(str.data(), str.length(), self_address_,
                                                             peer_address_, nullptr);
   EXPECT_EQ(quic::WRITE_STATUS_ERROR, result.status);
-  EXPECT_EQ(static_cast<int>(Api::IoError::IoErrorCode::NoSupport), result.error_code);
+  EXPECT_EQ(SOCKET_ERROR_NOT_SUP, result.error_code);
   EXPECT_FALSE(envoy_quic_writer_.IsWriteBlocked());
 }
 
@@ -133,7 +133,7 @@ TEST_F(EnvoyQuicWriterTest, SendFailureMessageTooBig) {
   // Currently MessageSize should be propagated through error_code. This test
   // would fail if QUICHE changes to propagate through status in the future.
   EXPECT_EQ(quic::WRITE_STATUS_ERROR, result.status);
-  EXPECT_EQ(static_cast<int>(Api::IoError::IoErrorCode::MessageTooBig), result.error_code);
+  EXPECT_EQ(SOCKET_ERROR_MSG_SIZE, result.error_code);
   EXPECT_FALSE(envoy_quic_writer_.IsWriteBlocked());
 }