tcp: fix overenthusiastic bounds on the new pool

api/envoy/config/cluster/v3/circuit_breaker.proto

@@ -59,10 +59,12 @@ message CircuitBreakers {
 
     // The maximum number of pending requests that Envoy will allow to the
     // upstream cluster. If not specified, the default is 1024.
+    // This limit is applied as a connection limit for non-HTTP traffic.
     google.protobuf.UInt32Value max_pending_requests = 3;
 
     // The maximum number of parallel requests that Envoy will make to the
     // upstream cluster. If not specified, the default is 1024.
+    // This limit does not apply to non-HTTP traffic.
     google.protobuf.UInt32Value max_requests = 4;
 
     // The maximum number of parallel retries that Envoy will allow to the

docs/root/version_history/current.rst

@@ -27,6 +27,7 @@ Bug Fixes
 
 * ext_authz: fix the ext_authz network filter to correctly set response flag and code details to ``UAEX`` when a connection is denied.
 * listener: fixed the crash when updating listeners that do not bind to port.
+* tcp: fixed a bug where upstream circuit breakers applied HTTP per-request bounds to TCP connections.
 * thrift_proxy: fix the thrift_proxy connection manager to correctly report success/error response metrics when performing :ref:`payload passthrough <envoy_v3_api_field_extensions.filters.network.thrift_proxy.v3.ThriftProxy.payload_passthrough>`.
 
 Removed Config or Runtime

source/common/conn_pool/conn_pool_base.cc

@@ -166,7 +166,7 @@ void ConnPoolImplBase::attachStreamToClient(Envoy::ConnectionPool::ActiveClient&
                                             AttachContext& context) {
   ASSERT(client.state() == Envoy::ConnectionPool::ActiveClient::State::READY);
 
-  if (!host_->cluster().resourceManager(priority_).requests().canCreate()) {
+  if (enforceMaxRequests() && !host_->cluster().resourceManager(priority_).requests().canCreate()) {
     ENVOY_LOG(debug, "max streams overflow");
     onPoolFailure(client.real_host_description_, absl::string_view(),
                   ConnectionPool::PoolFailureReason::Overflow, context);

source/common/conn_pool/conn_pool_base.h

@@ -323,6 +323,10 @@ class ConnPoolImplBase : protected Logger::Loggable<Logger::Id::pool> {
   const Network::ConnectionSocket::OptionsSharedPtr socket_options_;
   const Network::TransportSocketOptionsConstSharedPtr transport_socket_options_;
 
+  // True if the max requests circuit breakers apply.
+  // This will be false for the TCP pool, true otherwise.
+  virtual bool enforceMaxRequests() const { return true; }
+
   std::list<Instance::IdleCb> idle_callbacks_;
 
   // When calling purgePendingStreams, this list will be used to hold the streams we are about

source/common/tcp/conn_pool.h

@@ -217,6 +217,7 @@ class ConnPoolImpl : public Envoy::ConnectionPool::ConnPoolImplBase,
     callbacks->onPoolFailure(reason, failure_reason, host_description);
   }
 
+  bool enforceMaxRequests() const override { return false; }
   // These two functions exist for testing parity between old and new Tcp Connection Pools.
   virtual void onConnReleased(Envoy::ConnectionPool::ActiveClient&) {}
   virtual void onConnDestroyed() {}

test/integration/tcp_proxy_integration_test.cc

@@ -172,8 +172,24 @@ TEST_P(TcpProxyIntegrationTest, TcpProxyDownstreamDisconnect) {
 
 TEST_P(TcpProxyIntegrationTest, TcpProxyManyConnections) {
   autonomous_upstream_ = true;
+  config_helper_.addConfigModifier([&](envoy::config::bootstrap::v3::Bootstrap& bootstrap) -> void {
+    auto* static_resources = bootstrap.mutable_static_resources();
+    for (int i = 0; i < static_resources->clusters_size(); ++i) {
+      auto* cluster = static_resources->mutable_clusters(i);
+      auto* thresholds = cluster->mutable_circuit_breakers()->add_thresholds();
+      thresholds->mutable_max_connections()->set_value(1027);
+      thresholds->mutable_max_pending_requests()->set_value(1027);
+    }
+  });
   initialize();
+// The large number of connection is meant to regression test
+// https://github.com/envoyproxy/envoy/issues/19033 but fails on apple CI
+// TODO(alyssawilk) debug.
+#if defined(__APPLE__)
   const int num_connections = 50;
+#else
+  const int num_connections = 1026;
+#endif
   std::vector<IntegrationTcpClientPtr> clients(num_connections);
 
   for (int i = 0; i < num_connections; ++i) {