upstream: gracefully handle upstream connection creation failure

changelogs/current/bug_fixes/sockets__upstream_bind_network_namespace.rst

@@ -0,0 +1,5 @@
+Fixed a crash that occurred when an upstream connection was bound to an unavailable Linux network
+namespace via :ref:`network_namespace_filepath
+<envoy_v3_api_field_config.core.v3.SocketAddress.network_namespace_filepath>` in a cluster's
+upstream bind config. The failure to create the connection is now surfaced by the connection pools
+as a connection failure instead of crashing.

source/common/http/http1/conn_pool.cc

@@ -115,8 +115,17 @@ allocateConnPool(Event::Dispatcher& dispatcher, Random::RandomGenerator& random_
   return std::make_unique<FixedHttpConnPoolImpl>(
       std::move(host), std::move(priority), dispatcher, options, transport_socket_options,
       random_generator, state,
-      [](HttpConnPoolImplBase* pool) {
-        return std::make_unique<ActiveClient>(*pool, std::nullopt);
+      [](HttpConnPoolImplBase* pool) -> ::Envoy::ConnectionPool::ActiveClientPtr {
+        // Create the connection up front so a failure (e.g. network namespace binding failure)
+        // surfaces as a graceful connection failure rather than a crash when initializing the
+        // client with a null connection.
+        Upstream::Host::CreateConnectionData data =
+            static_cast<Envoy::ConnectionPool::ConnPoolImplBase*>(pool)->host()->createConnection(
+                pool->dispatcher(), pool->socketOptions(), pool->transportSocketOptions());
+        if (data.connection_ == nullptr) {
+          return nullptr;
+        }
+        return std::make_unique<ActiveClient>(*pool, makeOptRef(data));
       },
       [](Upstream::Host::CreateConnectionData& data, HttpConnPoolImplBase* pool) {
         CodecClientPtr codec{new CodecClientProd(

source/common/http/http2/conn_pool.cc

@@ -61,8 +61,17 @@ allocateConnPool(Event::Dispatcher& dispatcher, Random::RandomGenerator& random_
                  Http::HttpServerPropertiesCacheSharedPtr cache) {
   return std::make_unique<FixedHttpConnPoolImpl>(
       host, priority, dispatcher, options, transport_socket_options, random_generator, state,
-      [](HttpConnPoolImplBase* pool) {
-        return std::make_unique<ActiveClient>(*pool, std::nullopt);
+      [](HttpConnPoolImplBase* pool) -> ::Envoy::ConnectionPool::ActiveClientPtr {
+        // Create the connection up front so a failure (e.g. network namespace binding failure)
+        // surfaces as a graceful connection failure rather than a crash when initializing the
+        // client with a null connection.
+        Upstream::Host::CreateConnectionData data =
+            static_cast<Envoy::ConnectionPool::ConnPoolImplBase*>(pool)->host()->createConnection(
+                pool->dispatcher(), pool->socketOptions(), pool->transportSocketOptions());
+        if (data.connection_ == nullptr) {
+          return nullptr;
+        }
+        return std::make_unique<ActiveClient>(*pool, makeOptRef(data));
       },
       [](Upstream::Host::CreateConnectionData& data, HttpConnPoolImplBase* pool) {
         CodecClientPtr codec{new CodecClientProd(

source/common/http/mixed_conn_pool.cc

@@ -13,8 +13,14 @@ namespace Http {
 Envoy::ConnectionPool::ActiveClientPtr HttpConnPoolImplMixed::instantiateActiveClient() {
   uint32_t initial_streams = Http2::ActiveClient::calculateInitialStreamsLimit(
       http_server_properties_cache_, origin_, host());
-  return std::make_unique<Tcp::ActiveTcpClient>(
+  auto client = std::make_unique<Tcp::ActiveTcpClient>(
       *this, Envoy::ConnectionPool::ConnPoolImplBase::host(), initial_streams, std::nullopt);
+  // The underlying connection could not be created (e.g. network namespace binding failure).
+  // Return null so the pool surfaces this as a connection failure instead of using a broken client.
+  if (client->connection_ == nullptr) {
+    return nullptr;
+  }
+  return client;
 }
 
 CodecClientPtr

source/common/tcp/conn_pool.cc

@@ -22,6 +22,12 @@ ActiveTcpClient::ActiveTcpClient(Envoy::ConnectionPool::ConnPoolImplBase& parent
       parent_.dispatcher(), parent_.socketOptions(), parent_.transportSocketOptions());
   real_host_description_ = data.host_description_;
   connection_ = std::move(data.connection_);
+  // The connection can be null if it could not be created, e.g. when binding to a configured
+  // network namespace fails. Leave this client in a half-constructed state; instantiateActiveClient
+  // checks for a null connection and discards it, surfacing a graceful connection failure.
+  if (connection_ == nullptr) {
+    return;
+  }
   connection_->addConnectionCallbacks(*this);
   read_filter_handle_ = std::make_shared<ConnReadFilter>(*this);
   connection_->addReadFilter(read_filter_handle_);
@@ -176,8 +182,14 @@ ConnPoolImpl::newPendingStream(Envoy::ConnectionPool::AttachContext& context,
 }
 
 Envoy::ConnectionPool::ActiveClientPtr ConnPoolImpl::instantiateActiveClient() {
-  return std::make_unique<ActiveTcpClient>(*this, Envoy::ConnectionPool::ConnPoolImplBase::host(),
-                                           1, idle_timeout_);
+  auto client = std::make_unique<ActiveTcpClient>(
+      *this, Envoy::ConnectionPool::ConnPoolImplBase::host(), 1, idle_timeout_);
+  // The underlying connection could not be created (e.g. network namespace binding failure).
+  // Return null so the pool surfaces this as a connection failure instead of using a broken client.
+  if (client->connection_ == nullptr) {
+    return nullptr;
+  }
+  return client;
 }
 
 void ConnPoolImpl::onPoolReady(Envoy::ConnectionPool::ActiveClient& client,

source/common/upstream/upstream_impl.cc

@@ -751,6 +751,15 @@ Host::CreateConnectionData HostImplBase::createConnection(
         upstream_local_address.socket_options_, transport_socket_options);
   }
 
+  // `createClientConnection` can return nullptr, for example when binding the upstream connection
+  // to a configured network namespace fails (e.g. the namespace was removed at runtime). Returning
+  // a null connection lets the caller (connection pools) treat this as a connection failure rather
+  // than crashing on a null dereference below.
+  if (connection == nullptr) {
+    ENVOY_LOG(debug, "Failed to create upstream connection to host {}", address->asString());
+    return {nullptr, host};
+  }
+
   connection->connectionInfoSetter().enableSettingInterfaceName(
       cluster.setLocalInterfaceNameOnUpstreamConnections());
   connection->setBufferLimits(cluster.perConnectionBufferLimitBytes());

test/common/tcp/conn_pool_test.cc

@@ -1203,6 +1203,21 @@ TEST_F(TcpConnPoolImplTest, TestPreconnect) {
   conn_pool_->test_conns_[0].connection_->raiseEvent(Network::ConnectionEvent::RemoteClose);
 }
 
+/**
+ * Test that a failure to create the underlying connection (e.g. a network namespace binding
+ * failure) surfaces as a pool failure instead of crashing.
+ */
+TEST_F(TcpConnPoolImplDestructorTest, ConnectionCreationFailure) {
+  connect_timer_ = new NiceMock<Event::MockTimer>(&dispatcher_);
+  EXPECT_CALL(dispatcher_, createClientConnection_(_, _, _, _)).WillOnce(Return(nullptr));
+
+  callbacks_ = std::make_unique<ConnPoolCallbacks>();
+  EXPECT_CALL(callbacks_->mock_pool_failure_cb_, Call);
+  ConnectionPool::Cancellable* handle = conn_pool_->newConnection(*callbacks_);
+  EXPECT_EQ(nullptr, handle);
+  EXPECT_EQ(ConnectionPool::PoolFailureReason::LocalConnectionFailure, callbacks_->reason_);
+}
+
 /**
  * Test that pending connections are closed when the connection pool is destroyed.
  */

test/common/upstream/upstream_impl_test.cc

@@ -2033,6 +2033,30 @@ TEST_F(HostImplTest, CreateConnection) {
   EXPECT_EQ(host, connection->stream_info_.upstreamInfo()->upstreamHost());
 }
 
+// The dispatcher can fail to create a client connection (e.g. when binding the upstream socket to
+// a configured network namespace fails at runtime). createConnection must surface the null
+// connection to the caller rather than crash on a null dereference.
+TEST_F(HostImplTest, CreateConnectionFailure) {
+  MockClusterMockPrioritySet cluster;
+  Network::Address::InstanceConstSharedPtr address =
+      *Network::Utility::resolveUrl("tcp://10.0.0.1:1234");
+  auto host = std::shared_ptr<Upstream::HostImpl>(*HostImpl::create(
+      cluster.info_, "lyft.com", address, nullptr, nullptr, 1,
+      std::make_shared<const envoy::config::core::v3::Locality>(),
+      envoy::config::endpoint::v3::Endpoint::HealthCheckConfig::default_instance(), 1,
+      envoy::config::core::v3::UNKNOWN));
+
+  testing::StrictMock<Event::MockDispatcher> dispatcher;
+  Network::TransportSocketOptionsConstSharedPtr transport_socket_options;
+  Network::ConnectionSocket::OptionsSharedPtr options;
+
+  EXPECT_CALL(dispatcher, createClientConnection_(_, _, _, _)).WillOnce(Return(nullptr));
+  Envoy::Upstream::Host::CreateConnectionData connection_data =
+      host->createConnection(dispatcher, options, transport_socket_options);
+  EXPECT_EQ(nullptr, connection_data.connection_);
+  EXPECT_EQ(host.get(), connection_data.host_description_.get());
+}
+
 TEST_F(HostImplTest, OrcaReportingAddressDefaultsToDataAddress) {
   MockClusterMockPrioritySet cluster;
   Network::Address::InstanceConstSharedPtr address =