diff --git a/api/envoy/config/cluster/v3/circuit_breaker.proto b/api/envoy/config/cluster/v3/circuit_breaker.proto index 82cd329b91a72..34c907a5013ac 100644 --- a/api/envoy/config/cluster/v3/circuit_breaker.proto +++ b/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 diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index b9b9cbbf47171..34939bfbabb5d 100644 --- a/docs/root/version_history/current.rst +++ b/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 `. Removed Config or Runtime diff --git a/source/common/conn_pool/conn_pool_base.cc b/source/common/conn_pool/conn_pool_base.cc index 6a3f603148003..11b65b5555866 100644 --- a/source/common/conn_pool/conn_pool_base.cc +++ b/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); diff --git a/source/common/conn_pool/conn_pool_base.h b/source/common/conn_pool/conn_pool_base.h index 802cff1d639c2..4bdf01a5aec01 100644 --- a/source/common/conn_pool/conn_pool_base.h +++ b/source/common/conn_pool/conn_pool_base.h @@ -323,6 +323,10 @@ class ConnPoolImplBase : protected Logger::Loggable { 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 idle_callbacks_; // When calling purgePendingStreams, this list will be used to hold the streams we are about diff --git a/source/common/tcp/conn_pool.h b/source/common/tcp/conn_pool.h index d3a4a6d81d37e..760ed61e1dcef 100644 --- a/source/common/tcp/conn_pool.h +++ b/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() {} diff --git a/test/integration/tcp_proxy_integration_test.cc b/test/integration/tcp_proxy_integration_test.cc index 691bcd8df6118..435a7884b2191 100644 --- a/test/integration/tcp_proxy_integration_test.cc +++ b/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 clients(num_connections); for (int i = 0; i < num_connections; ++i) {