From b551d1b562de943e71fc37a8abf68277bebc23e5 Mon Sep 17 00:00:00 2001 From: alyssawilk Date: Mon, 29 Nov 2021 08:29:02 -0500 Subject: [PATCH] tcp: fix overenthusiastic bounds on the new pool (#19036) Risk Level: Low Testing: new integration test Docs Changes: made API more clear when requests count as connections :-/ Release Notes: inline Fixes #19033 Co-authored-by: Alyssa Wilk Signed-off-by: Greg Greenway (cherry picked from commit 2bf847854610db8bc5a44ef3046fcc8f3a23518e) --- .../config/cluster/v3/circuit_breaker.proto | 2 ++ docs/root/version_history/current.rst | 1 + source/common/conn_pool/conn_pool_base.cc | 2 +- source/common/conn_pool/conn_pool_base.h | 4 ++++ source/common/tcp/conn_pool.h | 1 + test/integration/tcp_proxy_integration_test.cc | 16 ++++++++++++++++ 6 files changed, 25 insertions(+), 1 deletion(-) 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 1146d86ac3838..a814f5d48f144 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -19,6 +19,7 @@ Bug Fixes * listener: fix a crash when updating any listener that does not bind to port. * listener: listener add can reuse the listener socket of a draining filter chain listener and fix the request lost. * mac: fix crash on startup on macOS 12 by changing the default allocator. +* tcp: fixed a bug where upstream circuit breakers applied HTTP per-request bounds to TCP connections. 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 113836ab13648..a22f8d618c747 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 dafc845f88915..a6a4802a59f4b 100644 --- a/source/common/conn_pool/conn_pool_base.h +++ b/source/common/conn_pool/conn_pool_base.h @@ -306,6 +306,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) {