From b984e0c76c9eeced7f386f0adf735db4125be4f6 Mon Sep 17 00:00:00 2001 From: Alex Konradi Date: Mon, 28 Sep 2020 14:19:22 -0400 Subject: [PATCH 1/8] Add TcpListenerImpl::setRejectFraction method This will allow creating an overload action that rejects (accepts the connection and then closes it) some fraction of connections in response to overload conditions. Signed-off-by: Alex Konradi --- include/envoy/network/listener.h | 6 + include/envoy/server/BUILD | 1 + include/envoy/server/overload_manager.h | 18 +++ source/common/event/dispatcher_impl.cc | 4 +- source/common/http/conn_manager_impl.cc | 6 +- source/common/network/tcp_listener_impl.cc | 14 +- source/common/network/tcp_listener_impl.h | 10 +- source/common/network/udp_listener_impl.h | 1 + test/common/network/listener_impl_test.cc | 151 +++++++++++++++++++-- test/mocks/network/mocks.h | 2 + test/server/connection_handler_test.cc | 1 + test/server/overload_manager_impl_test.cc | 14 ++ test/test_common/utility.cc | 1 + 13 files changed, 206 insertions(+), 23 deletions(-) diff --git a/include/envoy/network/listener.h b/include/envoy/network/listener.h index f74d6416103a..298e3aeac72a 100644 --- a/include/envoy/network/listener.h +++ b/include/envoy/network/listener.h @@ -324,6 +324,12 @@ class Listener { * Enable accepting new connections. */ virtual void enable() PURE; + + /** + * Set the fraction of incoming connections that will be closed immediately + * after being opened. + */ + virtual void setRejectFraction(float reject_fraction) PURE; }; using ListenerPtr = std::unique_ptr; diff --git a/include/envoy/server/BUILD b/include/envoy/server/BUILD index 34f42371d6ba..0de41208a70d 100644 --- a/include/envoy/server/BUILD +++ b/include/envoy/server/BUILD @@ -307,6 +307,7 @@ envoy_cc_library( name = "overload_manager_interface", hdrs = ["overload_manager.h"], deps = [ + "//include/envoy/common:random_generator_interface", "//include/envoy/thread_local:thread_local_interface", "//source/common/singleton:const_singleton", ], diff --git a/include/envoy/server/overload_manager.h b/include/envoy/server/overload_manager.h index 77c6c21c097d..ea0ac2fc0ba9 100644 --- a/include/envoy/server/overload_manager.h +++ b/include/envoy/server/overload_manager.h @@ -3,6 +3,7 @@ #include #include "envoy/common/pure.h" +#include "envoy/common/random_generator.h" #include "envoy/thread_local/thread_local.h" #include "common/common/macros.h" @@ -29,6 +30,23 @@ class OverloadActionState { float value() const { return action_value_; } bool isSaturated() const { return action_value_ == 1; } + bool isInactive() const { return action_value_ == 0; } + + /** + * Converts the overload action state to a boolean. The return value will be true with probability + * of the level of saturation. + */ + bool isRandomizedActive(Random::RandomGenerator& random_generator) const { + if (isInactive()) { + return false; + } else if (isSaturated()) { + return true; + } + + return random_generator.random() < + static_cast( + action_value_ * static_cast(Random::RandomGenerator::max())); + } private: float action_value_; diff --git a/source/common/event/dispatcher_impl.cc b/source/common/event/dispatcher_impl.cc index 3ce0a54fdfb0..a83ec407ca91 100644 --- a/source/common/event/dispatcher_impl.cc +++ b/source/common/event/dispatcher_impl.cc @@ -140,8 +140,8 @@ Network::ListenerPtr DispatcherImpl::createListener(Network::SocketSharedPtr&& s Network::TcpListenerCallbacks& cb, bool bind_to_port, uint32_t backlog_size) { ASSERT(isThreadSafe()); - return std::make_unique(*this, std::move(socket), cb, bind_to_port, - backlog_size); + return std::make_unique(*this, api_.randomGenerator(), std::move(socket), cb, + bind_to_port, backlog_size); } Network::UdpListenerPtr DispatcherImpl::createUdpListener(Network::SocketSharedPtr socket, diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index b456a4ca295d..1a731c7d5dae 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -837,7 +837,8 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(RequestHeaderMapPtr&& he filter_manager_.maybeEndDecode(end_stream); // Drop new requests when overloaded as soon as we have decoded the headers. - if (connection_manager_.overload_stop_accepting_requests_ref_.isSaturated()) { + if (connection_manager_.overload_stop_accepting_requests_ref_.isRandomizedActive( + connection_manager_.random_generator_)) { // In this one special case, do not create the filter chain. If there is a risk of memory // overload it is more important to avoid unnecessary allocation than to create the filters. filter_manager_.skipFilterChainCreation(); @@ -1255,7 +1256,8 @@ void ConnectionManagerImpl::ActiveStream::encodeHeaders(ResponseHeaderMap& heade bool drain_connection_due_to_overload = false; if (connection_manager_.drain_state_ == DrainState::NotDraining && - connection_manager_.overload_disable_keepalive_ref_.isSaturated()) { + connection_manager_.overload_disable_keepalive_ref_.isRandomizedActive( + connection_manager_.random_generator_)) { ENVOY_STREAM_LOG(debug, "disabling keepalive due to envoy overload", *this); if (connection_manager_.codec_->protocol() < Protocol::Http2 || Runtime::runtimeFeatureEnabled( diff --git a/source/common/network/tcp_listener_impl.cc b/source/common/network/tcp_listener_impl.cc index 5c39ec692f89..d3ff1dd7fe4f 100644 --- a/source/common/network/tcp_listener_impl.cc +++ b/source/common/network/tcp_listener_impl.cc @@ -59,7 +59,7 @@ void TcpListenerImpl::onSocketEvent(short flags) { break; } - if (rejectCxOverGlobalLimit()) { + if (rejectCxOverGlobalLimit() || reject_fraction_.isRandomizedActive(random_)) { // The global connection limit has been reached. io_handle->close(); cb_.onReject(); @@ -106,9 +106,11 @@ void TcpListenerImpl::setupServerSocket(Event::DispatcherImpl& dispatcher, Socke } } -TcpListenerImpl::TcpListenerImpl(Event::DispatcherImpl& dispatcher, SocketSharedPtr socket, - TcpListenerCallbacks& cb, bool bind_to_port, uint32_t backlog_size) - : BaseListenerImpl(dispatcher, std::move(socket)), cb_(cb), backlog_size_(backlog_size) { +TcpListenerImpl::TcpListenerImpl(Event::DispatcherImpl& dispatcher, Random::RandomGenerator& random, + SocketSharedPtr socket, TcpListenerCallbacks& cb, + bool bind_to_port, uint32_t backlog_size) + : BaseListenerImpl(dispatcher, std::move(socket)), cb_(cb), backlog_size_(backlog_size), + random_(random), reject_fraction_(0.0) { if (bind_to_port) { setupServerSocket(dispatcher, *socket_); } @@ -118,5 +120,9 @@ void TcpListenerImpl::enable() { file_event_->setEnabled(Event::FileReadyType::R void TcpListenerImpl::disable() { file_event_->setEnabled(0); } +void TcpListenerImpl::setRejectFraction(const float reject_fraction) { + reject_fraction_ = Server::OverloadActionState(reject_fraction); +} + } // namespace Network } // namespace Envoy diff --git a/source/common/network/tcp_listener_impl.h b/source/common/network/tcp_listener_impl.h index 56b6725b36c9..ff9926755e42 100644 --- a/source/common/network/tcp_listener_impl.h +++ b/source/common/network/tcp_listener_impl.h @@ -1,6 +1,8 @@ #pragma once #include "envoy/runtime/runtime.h" +#include "envoy/common/random_generator.h" +#include "envoy/server/overload_manager.h" #include "absl/strings/string_view.h" #include "base_listener_impl.h" @@ -13,10 +15,12 @@ namespace Network { */ class TcpListenerImpl : public BaseListenerImpl { public: - TcpListenerImpl(Event::DispatcherImpl& dispatcher, SocketSharedPtr socket, - TcpListenerCallbacks& cb, bool bind_to_port, uint32_t backlog_size); + TcpListenerImpl(Event::DispatcherImpl& dispatcher, Random::RandomGenerator& random, + SocketSharedPtr socket, TcpListenerCallbacks& cb, bool bind_to_port, + uint32_t backlog_size); void disable() override; void enable() override; + void setRejectFraction(float reject_fraction) override; static const absl::string_view GlobalMaxCxRuntimeKey; @@ -33,7 +37,9 @@ class TcpListenerImpl : public BaseListenerImpl { // rejected/closed. If the accepted socket is to be admitted, false is returned. static bool rejectCxOverGlobalLimit(); + Random::RandomGenerator& random_; Event::FileEventPtr file_event_; + Server::OverloadActionState reject_fraction_; }; } // namespace Network diff --git a/source/common/network/udp_listener_impl.h b/source/common/network/udp_listener_impl.h index e857a0150f25..d555649833bc 100644 --- a/source/common/network/udp_listener_impl.h +++ b/source/common/network/udp_listener_impl.h @@ -30,6 +30,7 @@ class UdpListenerImpl : public BaseListenerImpl, // Network::Listener Interface void disable() override; void enable() override; + void setRejectFraction(float) override {} // Network::UdpListener Interface Event::Dispatcher& dispatcher() override; diff --git a/test/common/network/listener_impl_test.cc b/test/common/network/listener_impl_test.cc index 8056826de91a..2281ba324b09 100644 --- a/test/common/network/listener_impl_test.cc +++ b/test/common/network/listener_impl_test.cc @@ -15,6 +15,7 @@ #include "gmock/gmock.h" #include "gtest/gtest.h" +#include using testing::_; using testing::Invoke; @@ -65,10 +66,11 @@ TEST_P(ListenerImplDeathTest, ErrorCallback) { class TestTcpListenerImpl : public TcpListenerImpl { public: - TestTcpListenerImpl(Event::DispatcherImpl& dispatcher, SocketSharedPtr socket, - TcpListenerCallbacks& cb, bool bind_to_port, + TestTcpListenerImpl(Event::DispatcherImpl& dispatcher, Random::RandomGenerator& random_generator, + SocketSharedPtr socket, TcpListenerCallbacks& cb, bool bind_to_port, uint32_t tcp_backlog = ENVOY_TCP_BACKLOG_SIZE) - : TcpListenerImpl(dispatcher, std::move(socket), cb, bind_to_port, tcp_backlog) {} + : TcpListenerImpl(dispatcher, random_generator, std::move(socket), cb, bind_to_port, + tcp_backlog) {} MOCK_METHOD(Address::InstanceConstSharedPtr, getLocalAddress, (os_fd_t fd)); }; @@ -82,6 +84,7 @@ INSTANTIATE_TEST_SUITE_P(IpVersions, TcpListenerImplTest, TEST_P(TcpListenerImplTest, SetListeningSocketOptionsSuccess) { Network::MockTcpListenerCallbacks listener_callbacks; Network::MockConnectionHandler connection_handler; + Random::MockRandomGenerator random_generator; auto socket = std::make_shared( Network::Test::getCanonicalLoopbackAddress(version_), nullptr, true); @@ -89,13 +92,15 @@ TEST_P(TcpListenerImplTest, SetListeningSocketOptionsSuccess) { socket->addOption(option); EXPECT_CALL(*option, setOption(_, envoy::config::core::v3::SocketOption::STATE_LISTENING)) .WillOnce(Return(true)); - TestTcpListenerImpl listener(dispatcherImpl(), socket, listener_callbacks, true); + TestTcpListenerImpl listener(dispatcherImpl(), random_generator, socket, listener_callbacks, + true); } // Test that an exception is thrown if there is an error setting socket options. TEST_P(TcpListenerImplTest, SetListeningSocketOptionsError) { Network::MockTcpListenerCallbacks listener_callbacks; Network::MockConnectionHandler connection_handler; + Random::MockRandomGenerator random_generator; auto socket = std::make_shared( Network::Test::getCanonicalLoopbackAddress(version_), nullptr, true); @@ -103,10 +108,11 @@ TEST_P(TcpListenerImplTest, SetListeningSocketOptionsError) { socket->addOption(option); EXPECT_CALL(*option, setOption(_, envoy::config::core::v3::SocketOption::STATE_LISTENING)) .WillOnce(Return(false)); - EXPECT_THROW_WITH_MESSAGE(TestTcpListenerImpl(dispatcherImpl(), socket, listener_callbacks, true), - CreateListenerException, - fmt::format("cannot set post-listen socket option on socket: {}", - socket->localAddress()->asString())); + EXPECT_THROW_WITH_MESSAGE( + TestTcpListenerImpl(dispatcherImpl(), random_generator, socket, listener_callbacks, true), + CreateListenerException, + fmt::format("cannot set post-listen socket option on socket: {}", + socket->localAddress()->asString())); } TEST_P(TcpListenerImplTest, UseActualDst) { @@ -115,10 +121,13 @@ TEST_P(TcpListenerImplTest, UseActualDst) { auto socketDst = std::make_shared(alt_address_, nullptr, false); Network::MockTcpListenerCallbacks listener_callbacks1; Network::MockConnectionHandler connection_handler; + Random::MockRandomGenerator random_generator; // Do not redirect since use_original_dst is false. - Network::TestTcpListenerImpl listener(dispatcherImpl(), socket, listener_callbacks1, true); + Network::TestTcpListenerImpl listener(dispatcherImpl(), random_generator, socket, + listener_callbacks1, true); Network::MockTcpListenerCallbacks listener_callbacks2; - Network::TestTcpListenerImpl listenerDst(dispatcherImpl(), socketDst, listener_callbacks2, false); + Network::TestTcpListenerImpl listenerDst(dispatcherImpl(), random_generator, socketDst, + listener_callbacks2, false); Network::ClientConnectionPtr client_connection = dispatcher_->createClientConnection( socket->localAddress(), Network::Address::InstanceConstSharedPtr(), @@ -214,8 +223,10 @@ TEST_P(TcpListenerImplTest, WildcardListenerUseActualDst) { Network::Test::getCanonicalLoopbackAddress(version_), nullptr, true); Network::MockTcpListenerCallbacks listener_callbacks; Network::MockConnectionHandler connection_handler; + Random::MockRandomGenerator random_generator; // Do not redirect since use_original_dst is false. - Network::TestTcpListenerImpl listener(dispatcherImpl(), socket, listener_callbacks, true); + Network::TestTcpListenerImpl listener(dispatcherImpl(), random_generator, socket, + listener_callbacks, true); auto local_dst_address = Network::Utility::getAddressWithPort( *Network::Test::getCanonicalLoopbackAddress(version_), socket->localAddress()->ip()->port()); @@ -253,11 +264,13 @@ TEST_P(TcpListenerImplTest, WildcardListenerIpv4Compat) { options, true); Network::MockTcpListenerCallbacks listener_callbacks; Network::MockConnectionHandler connection_handler; + Random::MockRandomGenerator random_generator; ASSERT_TRUE(socket->localAddress()->ip()->isAnyAddress()); // Do not redirect since use_original_dst is false. - Network::TestTcpListenerImpl listener(dispatcherImpl(), socket, listener_callbacks, true); + Network::TestTcpListenerImpl listener(dispatcherImpl(), random_generator, socket, + listener_callbacks, true); auto listener_address = Network::Utility::getAddressWithPort( *Network::Test::getCanonicalLoopbackAddress(version_), socket->localAddress()->ip()->port()); @@ -291,7 +304,9 @@ TEST_P(TcpListenerImplTest, DisableAndEnableListener) { Network::Test::getCanonicalLoopbackAddress(version_), nullptr, true); MockTcpListenerCallbacks listener_callbacks; MockConnectionCallbacks connection_callbacks; - TestTcpListenerImpl listener(dispatcherImpl(), socket, listener_callbacks, true); + Random::MockRandomGenerator random_generator; + TestTcpListenerImpl listener(dispatcherImpl(), random_generator, socket, listener_callbacks, + true); // When listener is disabled, the timer should fire before any connection is accepted. listener.disable(); @@ -325,6 +340,116 @@ TEST_P(TcpListenerImplTest, DisableAndEnableListener) { dispatcher_->run(Event::Dispatcher::RunType::Block); } +TEST_P(TcpListenerImplTest, SetListenerRejectFractionZero) { + testing::InSequence s1; + + auto socket = std::make_shared( + Network::Test::getCanonicalLoopbackAddress(version_), nullptr, true); + MockTcpListenerCallbacks listener_callbacks; + MockConnectionCallbacks connection_callbacks; + Random::MockRandomGenerator random_generator; + TestTcpListenerImpl listener(dispatcherImpl(), random_generator, socket, listener_callbacks, + true); + + listener.setRejectFraction(0); + + // This connection will be accepted and not rejected. + EXPECT_CALL(connection_callbacks, onEvent(ConnectionEvent::Connected)); + EXPECT_CALL(listener_callbacks, onAccept_(_)).WillOnce([&] { dispatcher_->exit(); }); + + ClientConnectionPtr client_connection = + dispatcher_->createClientConnection(socket->localAddress(), Address::InstanceConstSharedPtr(), + Network::Test::createRawBufferSocket(), nullptr); + client_connection->addConnectionCallbacks(connection_callbacks); + client_connection->connect(); + dispatcher_->run(Event::Dispatcher::RunType::Block); + + EXPECT_CALL(connection_callbacks, onEvent(ConnectionEvent::LocalClose)); + // Now that we've seen that the connection hasn't been closed by the listener, make sure to close + // it. + client_connection->close(ConnectionCloseType::NoFlush); +} + +TEST_P(TcpListenerImplTest, SetListenerRejectFractionIntermediate) { + testing::InSequence s1; + + auto socket = std::make_shared( + Network::Test::getCanonicalLoopbackAddress(version_), nullptr, true); + MockTcpListenerCallbacks listener_callbacks; + MockConnectionCallbacks connection_callbacks; + Random::MockRandomGenerator random_generator; + TestTcpListenerImpl listener(dispatcherImpl(), random_generator, socket, listener_callbacks, + true); + + listener.setRejectFraction(0.5f); + + // The first connection will be rejected because the random value is too small. + EXPECT_CALL(connection_callbacks, onEvent(ConnectionEvent::Connected)); + EXPECT_CALL(random_generator, random()).WillOnce(Return(0)); + EXPECT_CALL(listener_callbacks, onReject()); + EXPECT_CALL(connection_callbacks, onEvent(ConnectionEvent::RemoteClose)).WillOnce([&] { + dispatcher_->exit(); + }); + + { + ClientConnectionPtr client_connection = dispatcher_->createClientConnection( + socket->localAddress(), Address::InstanceConstSharedPtr(), + Network::Test::createRawBufferSocket(), nullptr); + client_connection->addConnectionCallbacks(connection_callbacks); + client_connection->connect(); + dispatcher_->run(Event::Dispatcher::RunType::Block); + } + + // The second connection rolls better on initiative and is accepted. + EXPECT_CALL(connection_callbacks, onEvent(ConnectionEvent::Connected)).WillOnce([&] { + dispatcher_->exit(); + }); + EXPECT_CALL(random_generator, random()).WillOnce(Return(std::numeric_limits::max())); + EXPECT_CALL(listener_callbacks, onAccept_(_)); + EXPECT_CALL(connection_callbacks, onEvent(ConnectionEvent::RemoteClose)).Times(0); + + { + ClientConnectionPtr client_connection = dispatcher_->createClientConnection( + socket->localAddress(), Address::InstanceConstSharedPtr(), + Network::Test::createRawBufferSocket(), nullptr); + client_connection->addConnectionCallbacks(connection_callbacks); + client_connection->connect(); + dispatcher_->run(Event::Dispatcher::RunType::Block); + + EXPECT_CALL(connection_callbacks, onEvent(ConnectionEvent::LocalClose)); + // Now that we've seen that the connection hasn't been closed by the listener, make sure to + // close it. + client_connection->close(ConnectionCloseType::NoFlush); + } +} + +TEST_P(TcpListenerImplTest, SetListenerRejectFractionAll) { + testing::InSequence s1; + + auto socket = std::make_shared( + Network::Test::getCanonicalLoopbackAddress(version_), nullptr, true); + MockTcpListenerCallbacks listener_callbacks; + MockConnectionCallbacks connection_callbacks; + Random::MockRandomGenerator random_generator; + TestTcpListenerImpl listener(dispatcherImpl(), random_generator, socket, listener_callbacks, + true); + + listener.setRejectFraction(1); + + EXPECT_CALL(connection_callbacks, onEvent(ConnectionEvent::Connected)); + EXPECT_CALL(listener_callbacks, onReject()); + EXPECT_CALL(connection_callbacks, onEvent(ConnectionEvent::RemoteClose)).WillOnce([&] { + dispatcher_->exit(); + }); + + ClientConnectionPtr client_connection = + dispatcher_->createClientConnection(socket->localAddress(), Address::InstanceConstSharedPtr(), + Network::Test::createRawBufferSocket(), nullptr); + client_connection->addConnectionCallbacks(connection_callbacks); + client_connection->connect(); + dispatcher_->run(Event::Dispatcher::RunType::Block); +} + } // namespace } // namespace Network } // namespace Envoy diff --git a/test/mocks/network/mocks.h b/test/mocks/network/mocks.h index 304de415e7d3..62278c00adf2 100644 --- a/test/mocks/network/mocks.h +++ b/test/mocks/network/mocks.h @@ -391,6 +391,7 @@ class MockListener : public Listener { MOCK_METHOD(void, onDestroy, ()); MOCK_METHOD(void, enable, ()); MOCK_METHOD(void, disable, ()); + MOCK_METHOD(void, setRejectFraction, (float)); }; class MockConnectionHandler : public ConnectionHandler { @@ -501,6 +502,7 @@ class MockUdpListener : public UdpListener { MOCK_METHOD(void, onDestroy, ()); MOCK_METHOD(void, enable, ()); MOCK_METHOD(void, disable, ()); + MOCK_METHOD(void, setRejectFraction, (float), (override)); MOCK_METHOD(Event::Dispatcher&, dispatcher, ()); MOCK_METHOD(Address::InstanceConstSharedPtr&, localAddress, (), (const)); MOCK_METHOD(Api::IoCallUint64Result, send, (const UdpSendData&)); diff --git a/test/server/connection_handler_test.cc b/test/server/connection_handler_test.cc index 47d58f94e256..2bb26d0f1a41 100644 --- a/test/server/connection_handler_test.cc +++ b/test/server/connection_handler_test.cc @@ -176,6 +176,7 @@ class ConnectionHandlerTest : public testing::Test, protected Logger::Loggablestop(); } +TEST(OverloadActionState, RandomizedActive) { + Random::MockRandomGenerator random_gen; + EXPECT_CALL(random_gen, random()) + .Times(2) + .WillRepeatedly(Return(Random::RandomGenerator::max() / 2)); + + EXPECT_FALSE(OverloadActionState::inactive().isRandomizedActive(random_gen)); + EXPECT_TRUE(OverloadActionState::saturated().isRandomizedActive(random_gen)); + EXPECT_FALSE(OverloadActionState(0.49).isRandomizedActive(random_gen)); + EXPECT_TRUE(OverloadActionState(0.51).isRandomizedActive(random_gen)); +} + } // namespace } // namespace Server } // namespace Envoy diff --git a/test/test_common/utility.cc b/test/test_common/utility.cc index 9c6d468a17fd..51b26c1cfb9f 100644 --- a/test/test_common/utility.cc +++ b/test/test_common/utility.cc @@ -34,6 +34,7 @@ #include "test/mocks/common.h" #include "test/mocks/stats/mocks.h" +#include "test/mocks/common.h" #include "test/test_common/printers.h" #include "test/test_common/resources.h" #include "test/test_common/test_time.h" From 8eda768a389d171e1144288e9aa5c1b1cfcb7b59 Mon Sep 17 00:00:00 2001 From: Alex Konradi Date: Tue, 29 Sep 2020 11:03:33 -0400 Subject: [PATCH 2/8] Add overload action for rejecting TCP connections Signed-off-by: Alex Konradi --- include/envoy/network/connection_handler.h | 6 ++++++ include/envoy/server/overload_manager.h | 4 ++++ source/server/connection_handler_impl.cc | 8 ++++++++ source/server/connection_handler_impl.h | 1 + source/server/worker_impl.cc | 7 +++++++ source/server/worker_impl.h | 1 + test/mocks/network/mocks.h | 1 + 7 files changed, 28 insertions(+) diff --git a/include/envoy/network/connection_handler.h b/include/envoy/network/connection_handler.h index ea804eba5789..c42cc290cd61 100644 --- a/include/envoy/network/connection_handler.h +++ b/include/envoy/network/connection_handler.h @@ -94,6 +94,12 @@ class ConnectionHandler { */ virtual void enableListeners() PURE; + /** + * Set the fraction of connections the listeners should reject. + * @param reject_fraction a value between 0 (reject none) and 1 (reject all). + */ + virtual void setListenerRejectFraction(float reject_fraction) PURE; + /** * @return the stat prefix used for per-handler stats. */ diff --git a/include/envoy/server/overload_manager.h b/include/envoy/server/overload_manager.h index ea0ac2fc0ba9..8e1cc7d37413 100644 --- a/include/envoy/server/overload_manager.h +++ b/include/envoy/server/overload_manager.h @@ -80,6 +80,10 @@ class OverloadActionNameValues { // Overload action to stop accepting new connections. const std::string StopAcceptingConnections = "envoy.overload_actions.stop_accepting_connections"; + // Overload action to reject (accept and then close) new connections. + const std::string RejectIncomingConnections = + "envoy.overload_actions.reject_incoming_connections"; + // Overload action to try to shrink the heap by releasing free memory. const std::string ShrinkHeap = "envoy.overload_actions.shrink_heap"; }; diff --git a/source/server/connection_handler_impl.cc b/source/server/connection_handler_impl.cc index 4c3c431c4291..46f0eaee0613 100644 --- a/source/server/connection_handler_impl.cc +++ b/source/server/connection_handler_impl.cc @@ -148,6 +148,14 @@ void ConnectionHandlerImpl::enableListeners() { } } +void ConnectionHandlerImpl::setListenerRejectFraction(float reject_fraction) { + disable_listeners_ = false; + for (auto& listener : listeners_) { + listener.second.listener_->listener()->setRejectFraction(reject_fraction); + } +} + + void ConnectionHandlerImpl::ActiveTcpListener::removeConnection(ActiveTcpConnection& connection) { ENVOY_CONN_LOG(debug, "adding to cleanup list", *connection.connection_); ActiveConnections& active_connections = connection.active_connections_; diff --git a/source/server/connection_handler_impl.h b/source/server/connection_handler_impl.h index 3eb3e8e58c35..bc0cc9118587 100644 --- a/source/server/connection_handler_impl.h +++ b/source/server/connection_handler_impl.h @@ -82,6 +82,7 @@ class ConnectionHandlerImpl : public Network::ConnectionHandler, void stopListeners() override; void disableListeners() override; void enableListeners() override; + void setListenerRejectFraction(float reject_fraction) override; const std::string& statPrefix() const override { return per_handler_stat_prefix_; } /** diff --git a/source/server/worker_impl.cc b/source/server/worker_impl.cc index 1fd18f106618..b659ffec6e06 100644 --- a/source/server/worker_impl.cc +++ b/source/server/worker_impl.cc @@ -31,6 +31,9 @@ WorkerImpl::WorkerImpl(ThreadLocal::Instance& tls, ListenerHooks& hooks, overload_manager.registerForAction( OverloadActionNames::get().StopAcceptingConnections, *dispatcher_, [this](OverloadActionState state) { stopAcceptingConnectionsCb(state); }); + overload_manager.registerForAction( + OverloadActionNames::get().RejectIncomingConnections, *dispatcher_, + [this](OverloadActionState state) { rejectIncomingConnectionsCb(state); }); } void WorkerImpl::addListener(absl::optional overridden_listener, @@ -149,5 +152,9 @@ void WorkerImpl::stopAcceptingConnectionsCb(OverloadActionState state) { } } +void WorkerImpl::rejectIncomingConnectionsCb(OverloadActionState state) { + handler_->setListenerRejectFraction(static_cast(state.value())); +} + } // namespace Server } // namespace Envoy diff --git a/source/server/worker_impl.h b/source/server/worker_impl.h index c4cb4a58c2b5..22513b594e5d 100644 --- a/source/server/worker_impl.h +++ b/source/server/worker_impl.h @@ -58,6 +58,7 @@ class WorkerImpl : public Worker, Logger::Loggable { private: void threadRoutine(GuardDog& guard_dog); void stopAcceptingConnectionsCb(OverloadActionState state); + void rejectIncomingConnectionsCb(OverloadActionState state); ThreadLocal::Instance& tls_; ListenerHooks& hooks_; diff --git a/test/mocks/network/mocks.h b/test/mocks/network/mocks.h index 62278c00adf2..74a9a435946e 100644 --- a/test/mocks/network/mocks.h +++ b/test/mocks/network/mocks.h @@ -413,6 +413,7 @@ class MockConnectionHandler : public ConnectionHandler { MOCK_METHOD(void, stopListeners, ()); MOCK_METHOD(void, disableListeners, ()); MOCK_METHOD(void, enableListeners, ()); + MOCK_METHOD(void, setListenerRejectFraction, (float), (override)); MOCK_METHOD(const std::string&, statPrefix, (), (const)); }; From 792fd4166b051d58f31c0faa9df05c57a3d244e7 Mon Sep 17 00:00:00 2001 From: Alex Konradi Date: Wed, 30 Sep 2020 17:50:48 -0400 Subject: [PATCH 3/8] Record reject fraction in connection handler Address feedback around adding new listeners when the reject fraction is already set, and add tests. Signed-off-by: Alex Konradi --- source/server/connection_handler_impl.cc | 5 +++- source/server/connection_handler_impl.h | 1 + test/server/connection_handler_test.cc | 32 ++++++++++++++++++++++++ 3 files changed, 37 insertions(+), 1 deletion(-) diff --git a/source/server/connection_handler_impl.cc b/source/server/connection_handler_impl.cc index 46f0eaee0613..dd5e75c0d587 100644 --- a/source/server/connection_handler_impl.cc +++ b/source/server/connection_handler_impl.cc @@ -66,6 +66,9 @@ void ConnectionHandlerImpl::addListener(absl::optional overridden_list if (disable_listeners_) { details.listener_->pauseListening(); } + if (auto* listener = details.listener_->listener(); listener != nullptr) { + listener->setRejectFraction(listener_reject_fraction_); + } listeners_.emplace_back(config.listenSocketFactory().localAddress(), std::move(details)); } @@ -149,7 +152,7 @@ void ConnectionHandlerImpl::enableListeners() { } void ConnectionHandlerImpl::setListenerRejectFraction(float reject_fraction) { - disable_listeners_ = false; + listener_reject_fraction_ = reject_fraction; for (auto& listener : listeners_) { listener.second.listener_->listener()->setRejectFraction(reject_fraction); } diff --git a/source/server/connection_handler_impl.h b/source/server/connection_handler_impl.h index bc0cc9118587..f4b10db16090 100644 --- a/source/server/connection_handler_impl.h +++ b/source/server/connection_handler_impl.h @@ -362,6 +362,7 @@ class ConnectionHandlerImpl : public Network::ConnectionHandler, std::list> listeners_; std::atomic num_handler_connections_{}; bool disable_listeners_; + float listener_reject_fraction_{0}; }; class ActiveUdpListenerBase : public ConnectionHandlerImpl::ActiveListenerImplBase, diff --git a/test/server/connection_handler_test.cc b/test/server/connection_handler_test.cc index 2bb26d0f1a41..870e2ed6178b 100644 --- a/test/server/connection_handler_test.cc +++ b/test/server/connection_handler_test.cc @@ -452,6 +452,38 @@ TEST_F(ConnectionHandlerTest, AddDisabledListener) { handler_->addListener(absl::nullopt, *test_listener); } +TEST_F(ConnectionHandlerTest, SetListenerRejectFraction) { + InSequence s; + + Network::TcpListenerCallbacks* listener_callbacks; + auto listener = new NiceMock(); + TestListener* test_listener = + addListener(1, false, false, "test_listener", listener, &listener_callbacks); + EXPECT_CALL(*socket_factory_, localAddress()).WillOnce(ReturnRef(local_address_)); + handler_->addListener(absl::nullopt, *test_listener); + + EXPECT_CALL(*listener, setRejectFraction(0.1234f)); + EXPECT_CALL(*listener, onDestroy()); + + handler_->setListenerRejectFraction(0.1234f); +} + +TEST_F(ConnectionHandlerTest, AddListenerSetRejectFraction) { + InSequence s; + + Network::TcpListenerCallbacks* listener_callbacks; + auto listener = new NiceMock(); + TestListener* test_listener = + addListener(1, false, false, "test_listener", listener, &listener_callbacks); + EXPECT_CALL(*listener, setRejectFraction(0.12345f)); + EXPECT_CALL(*socket_factory_, localAddress()).WillOnce(ReturnRef(local_address_)); + EXPECT_CALL(*listener, onDestroy()); + + handler_->setListenerRejectFraction(0.12345f); + handler_->addListener(absl::nullopt, *test_listener); +} + + TEST_F(ConnectionHandlerTest, DestroyCloseConnections) { InSequence s; From dbba9e98ebfaa19c8f0027dfc62abbf00b5df058 Mon Sep 17 00:00:00 2001 From: Alex Konradi Date: Fri, 9 Oct 2020 16:36:35 -0400 Subject: [PATCH 4/8] Use separate stats for connection rejection Signed-off-by: Alex Konradi --- include/envoy/network/listener.h | 6 +++++- source/common/event/dispatcher_impl.cc | 4 ++-- source/common/network/tcp_listener_impl.cc | 8 ++++++-- source/common/network/tcp_listener_impl.h | 3 +-- source/server/connection_handler_impl.cc | 12 +++++++++++- source/server/connection_handler_impl.h | 3 ++- test/common/network/dns_impl_test.cc | 2 +- test/common/network/listener_impl_test.cc | 13 ++++++++----- test/mocks/network/mocks.h | 2 +- test/server/connection_handler_test.cc | 1 - test/test_common/utility.cc | 1 - 11 files changed, 37 insertions(+), 18 deletions(-) diff --git a/include/envoy/network/listener.h b/include/envoy/network/listener.h index 298e3aeac72a..4401df6cc20c 100644 --- a/include/envoy/network/listener.h +++ b/include/envoy/network/listener.h @@ -197,10 +197,14 @@ class TcpListenerCallbacks { */ virtual void onAccept(ConnectionSocketPtr&& socket) PURE; + enum class RejectCause { + GlobalCxLimit, + OverloadAction, + }; /** * Called when a new connection is rejected. */ - virtual void onReject() PURE; + virtual void onReject(RejectCause cause) PURE; }; /** diff --git a/source/common/event/dispatcher_impl.cc b/source/common/event/dispatcher_impl.cc index 0ef5dcc456f1..6cdbb623b720 100644 --- a/source/common/event/dispatcher_impl.cc +++ b/source/common/event/dispatcher_impl.cc @@ -162,8 +162,8 @@ Network::ListenerPtr DispatcherImpl::createListener(Network::SocketSharedPtr&& s Network::TcpListenerCallbacks& cb, bool bind_to_port, uint32_t backlog_size) { ASSERT(isThreadSafe()); - return std::make_unique(*this, api_.randomGenerator(), std::move(socket), cb, - bind_to_port, backlog_size); + return std::make_unique( + *this, api_.randomGenerator(), std::move(socket), cb, bind_to_port, backlog_size); } Network::UdpListenerPtr DispatcherImpl::createUdpListener(Network::SocketSharedPtr socket, diff --git a/source/common/network/tcp_listener_impl.cc b/source/common/network/tcp_listener_impl.cc index b404635f390e..32ddf272bfad 100644 --- a/source/common/network/tcp_listener_impl.cc +++ b/source/common/network/tcp_listener_impl.cc @@ -59,10 +59,14 @@ void TcpListenerImpl::onSocketEvent(short flags) { break; } - if (rejectCxOverGlobalLimit() || random_.bernoulli(reject_fraction_)) { + if (rejectCxOverGlobalLimit()) { // The global connection limit has been reached. io_handle->close(); - cb_.onReject(); + cb_.onReject(TcpListenerCallbacks::RejectCause::GlobalCxLimit); + continue; + } else if (random_.bernoulli(reject_fraction_)) { + io_handle->close(); + cb_.onReject(TcpListenerCallbacks::RejectCause::OverloadAction); continue; } diff --git a/source/common/network/tcp_listener_impl.h b/source/common/network/tcp_listener_impl.h index 2544519cd4ed..5ecec192abf2 100644 --- a/source/common/network/tcp_listener_impl.h +++ b/source/common/network/tcp_listener_impl.h @@ -1,8 +1,7 @@ #pragma once -#include "envoy/runtime/runtime.h" #include "envoy/common/random_generator.h" -#include "envoy/server/overload_manager.h" +#include "envoy/runtime/runtime.h" #include "absl/strings/string_view.h" #include "base_listener_impl.h" diff --git a/source/server/connection_handler_impl.cc b/source/server/connection_handler_impl.cc index fc844d58fefc..5f0c854dea44 100644 --- a/source/server/connection_handler_impl.cc +++ b/source/server/connection_handler_impl.cc @@ -158,7 +158,6 @@ void ConnectionHandlerImpl::setListenerRejectFraction(float reject_fraction) { } } - void ConnectionHandlerImpl::ActiveTcpListener::removeConnection(ActiveTcpConnection& connection) { ENVOY_CONN_LOG(debug, "adding to cleanup list", *connection.connection_); ActiveConnections& active_connections = connection.active_connections_; @@ -402,6 +401,17 @@ void ConnectionHandlerImpl::ActiveTcpListener::onAccept(Network::ConnectionSocke onAcceptWorker(std::move(socket), config_->handOffRestoredDestinationConnections(), false); } +void ConnectionHandlerImpl::ActiveTcpListener::onReject(RejectCause cause) { + switch (cause) { + case RejectCause::GlobalCxLimit: + stats_.downstream_global_cx_overflow_.inc(); + break; + case RejectCause::OverloadAction: + stats_.downstream_cx_overload_reject_.inc(); + break; + } +} + void ConnectionHandlerImpl::ActiveTcpListener::onAcceptWorker( Network::ConnectionSocketPtr&& socket, bool hand_off_restored_destination_connections, bool rebalanced) { diff --git a/source/server/connection_handler_impl.h b/source/server/connection_handler_impl.h index f4b10db16090..be49e5a3b314 100644 --- a/source/server/connection_handler_impl.h +++ b/source/server/connection_handler_impl.h @@ -30,6 +30,7 @@ namespace Server { COUNTER(downstream_cx_destroy) \ COUNTER(downstream_cx_overflow) \ COUNTER(downstream_cx_total) \ + COUNTER(downstream_cx_overload_reject) \ COUNTER(downstream_global_cx_overflow) \ COUNTER(downstream_pre_cx_timeout) \ COUNTER(no_filter_chain_match) \ @@ -134,7 +135,7 @@ class ConnectionHandlerImpl : public Network::ConnectionHandler, // Network::TcpListenerCallbacks void onAccept(Network::ConnectionSocketPtr&& socket) override; - void onReject() override { stats_.downstream_global_cx_overflow_.inc(); } + void onReject(RejectCause) override; // ActiveListenerImplBase Network::Listener* listener() override { return listener_.get(); } diff --git a/test/common/network/dns_impl_test.cc b/test/common/network/dns_impl_test.cc index ea83c16c21e7..339cfb4abc89 100644 --- a/test/common/network/dns_impl_test.cc +++ b/test/common/network/dns_impl_test.cc @@ -281,7 +281,7 @@ class TestDnsServer : public TcpListenerCallbacks { queries_.emplace_back(query); } - void onReject() override { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; } + void onReject(RejectCause) override { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; } void addHosts(const std::string& hostname, const IpList& ip, const RecordType& type) { if (type == RecordType::A) { diff --git a/test/common/network/listener_impl_test.cc b/test/common/network/listener_impl_test.cc index 2281ba324b09..e3cd4a1b268e 100644 --- a/test/common/network/listener_impl_test.cc +++ b/test/common/network/listener_impl_test.cc @@ -1,3 +1,5 @@ +#include + #include "envoy/config/core/v3/base.pb.h" #include "envoy/network/exception.h" @@ -15,7 +17,6 @@ #include "gmock/gmock.h" #include "gtest/gtest.h" -#include using testing::_; using testing::Invoke; @@ -184,7 +185,8 @@ TEST_P(TcpListenerImplTest, GlobalConnectionLimitEnforcement) { }; initiate_connections(5); - EXPECT_CALL(listener_callbacks, onReject()).Times(3); + EXPECT_CALL(listener_callbacks, onReject(TcpListenerCallbacks::RejectCause::GlobalCxLimit)) + .Times(3); dispatcher_->run(Event::Dispatcher::RunType::Block); // We expect any server-side connections that get created to populate 'server_connections'. @@ -194,7 +196,8 @@ TEST_P(TcpListenerImplTest, GlobalConnectionLimitEnforcement) { Runtime::LoaderSingleton::getExisting()->mergeValues( {{"overload.global_downstream_max_connections", "3"}}); initiate_connections(5); - EXPECT_CALL(listener_callbacks, onReject()).Times(4); + EXPECT_CALL(listener_callbacks, onReject(TcpListenerCallbacks::RejectCause::GlobalCxLimit)) + .Times(4); dispatcher_->run(Event::Dispatcher::RunType::Block); EXPECT_EQ(3, server_connections.size()); @@ -386,7 +389,7 @@ TEST_P(TcpListenerImplTest, SetListenerRejectFractionIntermediate) { // The first connection will be rejected because the random value is too small. EXPECT_CALL(connection_callbacks, onEvent(ConnectionEvent::Connected)); EXPECT_CALL(random_generator, random()).WillOnce(Return(0)); - EXPECT_CALL(listener_callbacks, onReject()); + EXPECT_CALL(listener_callbacks, onReject(TcpListenerCallbacks::RejectCause::OverloadAction)); EXPECT_CALL(connection_callbacks, onEvent(ConnectionEvent::RemoteClose)).WillOnce([&] { dispatcher_->exit(); }); @@ -437,7 +440,7 @@ TEST_P(TcpListenerImplTest, SetListenerRejectFractionAll) { listener.setRejectFraction(1); EXPECT_CALL(connection_callbacks, onEvent(ConnectionEvent::Connected)); - EXPECT_CALL(listener_callbacks, onReject()); + EXPECT_CALL(listener_callbacks, onReject(TcpListenerCallbacks::RejectCause::OverloadAction)); EXPECT_CALL(connection_callbacks, onEvent(ConnectionEvent::RemoteClose)).WillOnce([&] { dispatcher_->exit(); }); diff --git a/test/mocks/network/mocks.h b/test/mocks/network/mocks.h index 74a9a435946e..de7b843a72d0 100644 --- a/test/mocks/network/mocks.h +++ b/test/mocks/network/mocks.h @@ -131,7 +131,7 @@ class MockTcpListenerCallbacks : public TcpListenerCallbacks { void onAccept(ConnectionSocketPtr&& socket) override { onAccept_(socket); } MOCK_METHOD(void, onAccept_, (ConnectionSocketPtr & socket)); - MOCK_METHOD(void, onReject, ()); + MOCK_METHOD(void, onReject, (RejectCause), (override)); }; class MockUdpListenerCallbacks : public UdpListenerCallbacks { diff --git a/test/server/connection_handler_test.cc b/test/server/connection_handler_test.cc index 870e2ed6178b..fcdfa8cf84e0 100644 --- a/test/server/connection_handler_test.cc +++ b/test/server/connection_handler_test.cc @@ -483,7 +483,6 @@ TEST_F(ConnectionHandlerTest, AddListenerSetRejectFraction) { handler_->addListener(absl::nullopt, *test_listener); } - TEST_F(ConnectionHandlerTest, DestroyCloseConnections) { InSequence s; diff --git a/test/test_common/utility.cc b/test/test_common/utility.cc index e707e7117861..a0a4814f37c8 100644 --- a/test/test_common/utility.cc +++ b/test/test_common/utility.cc @@ -34,7 +34,6 @@ #include "test/mocks/common.h" #include "test/mocks/stats/mocks.h" -#include "test/mocks/common.h" #include "test/test_common/printers.h" #include "test/test_common/resources.h" #include "test/test_common/test_time.h" From 6e4525b032f5b8d2a5a1fa08063b1cd277a1d729 Mon Sep 17 00:00:00 2001 From: Alex Konradi Date: Fri, 9 Oct 2020 17:11:32 -0400 Subject: [PATCH 5/8] Add release notes and documentation Signed-off-by: Alex Konradi --- docs/root/configuration/listeners/stats.rst | 1 + .../overload_manager/overload_manager.rst | 25 ++++++++++++++----- docs/root/version_history/current.rst | 1 + 3 files changed, 21 insertions(+), 6 deletions(-) diff --git a/docs/root/configuration/listeners/stats.rst b/docs/root/configuration/listeners/stats.rst index 09ad6858fbe2..eb82810f6972 100644 --- a/docs/root/configuration/listeners/stats.rst +++ b/docs/root/configuration/listeners/stats.rst @@ -17,6 +17,7 @@ Every listener has a statistics tree rooted at *listener.
.* with the fo downstream_cx_active, Gauge, Total active connections downstream_cx_length_ms, Histogram, Connection length milliseconds downstream_cx_overflow, Counter, Total connections rejected due to enforcement of listener connection limit + downstream_cx_overload_reject, Counter, Total connections rejected due to configured overload actions downstream_pre_cx_timeout, Counter, Sockets that timed out during listener filter processing downstream_pre_cx_active, Gauge, Sockets currently undergoing listener filter processing global_cx_overflow, Counter, Total connections rejected due to enforecement of the global connection limit diff --git a/docs/root/configuration/operations/overload_manager/overload_manager.rst b/docs/root/configuration/operations/overload_manager/overload_manager.rst index ade5201f5c4c..22e7e70b33b5 100644 --- a/docs/root/configuration/operations/overload_manager/overload_manager.rst +++ b/docs/root/configuration/operations/overload_manager/overload_manager.rst @@ -68,14 +68,27 @@ Overload actions The following overload actions are supported: -.. csv-table:: - :header: Name, Description +.. list-table:: + :header-rows: 1 :widths: 1, 2 - envoy.overload_actions.stop_accepting_requests, Envoy will immediately respond with a 503 response code to new requests - envoy.overload_actions.disable_http_keepalive, Envoy will stop accepting streams on incoming HTTP connections - envoy.overload_actions.stop_accepting_connections, Envoy will stop accepting new network connections on its configured listeners - envoy.overload_actions.shrink_heap, Envoy will periodically try to shrink the heap by releasing free memory to the system + * - Name + - Description + + * - envoy.overload_actions.stop_accepting_requests + - Envoy will immediately respond with a 503 response code to new requests + + * - envoy.overload_actions.disable_http_keepalive + - Envoy will stop accepting streams on incoming HTTP connections + + * - envoy.overload_actions.stop_accepting_connections + - Envoy will stop accepting new network connections on its configured listeners + + * - envoy.overload_actions.reject_incoming_connections + - Envoy will reject incoming connections on its configured listeners without processing any data + + * - envoy.overload_actions.shrink_heap + - Envoy will periodically try to shrink the heap by releasing free memory to the system Limiting Active Connections --------------------------- diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index ce3938e22a6d..5e10f83eb1af 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -20,6 +20,7 @@ Removed Config or Runtime New Features ------------ * grpc: implemented header value syntax support when defining :ref:`initial metadata ` for gRPC-based `ext_authz` :ref:`HTTP ` and :ref:`network ` filters, and :ref:`ratelimit ` filters. +* tcp: added a new :ref:`envoy.overload_actions.reject_incoming_connections ` action to reject incoming TCP connections Deprecated ---------- From fdcb8cefa9f86580c6d1a6bb958e720fd1fc4744 Mon Sep 17 00:00:00 2001 From: Alex Konradi Date: Fri, 9 Oct 2020 17:30:21 -0400 Subject: [PATCH 6/8] Add missing period Signed-off-by: Alex Konradi --- docs/root/version_history/current.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 5e10f83eb1af..9b0fa19b97d5 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -20,7 +20,7 @@ Removed Config or Runtime New Features ------------ * grpc: implemented header value syntax support when defining :ref:`initial metadata ` for gRPC-based `ext_authz` :ref:`HTTP ` and :ref:`network ` filters, and :ref:`ratelimit ` filters. -* tcp: added a new :ref:`envoy.overload_actions.reject_incoming_connections ` action to reject incoming TCP connections +* tcp: added a new :ref:`envoy.overload_actions.reject_incoming_connections ` action to reject incoming TCP connections. Deprecated ---------- From 5b2f33328272d4a70dd46e979658fa485c8e14c3 Mon Sep 17 00:00:00 2001 From: Alex Konradi Date: Mon, 12 Oct 2020 15:16:43 -0400 Subject: [PATCH 7/8] Relax ordering constraints for events Relax the ordering constraints for events that occur asynchronously on different ends of the connection while maintaining requirements for relative ordering on each end. Signed-off-by: Alex Konradi --- test/common/network/listener_impl_test.cc | 66 ++++++++++++++--------- 1 file changed, 41 insertions(+), 25 deletions(-) diff --git a/test/common/network/listener_impl_test.cc b/test/common/network/listener_impl_test.cc index e3cd4a1b268e..21e4673ce643 100644 --- a/test/common/network/listener_impl_test.cc +++ b/test/common/network/listener_impl_test.cc @@ -344,8 +344,6 @@ TEST_P(TcpListenerImplTest, DisableAndEnableListener) { } TEST_P(TcpListenerImplTest, SetListenerRejectFractionZero) { - testing::InSequence s1; - auto socket = std::make_shared( Network::Test::getCanonicalLoopbackAddress(version_), nullptr, true); MockTcpListenerCallbacks listener_callbacks; @@ -357,7 +355,11 @@ TEST_P(TcpListenerImplTest, SetListenerRejectFractionZero) { listener.setRejectFraction(0); // This connection will be accepted and not rejected. - EXPECT_CALL(connection_callbacks, onEvent(ConnectionEvent::Connected)); + { + testing::InSequence s1; + EXPECT_CALL(connection_callbacks, onEvent(ConnectionEvent::Connected)); + EXPECT_CALL(connection_callbacks, onEvent(ConnectionEvent::LocalClose)); + } EXPECT_CALL(listener_callbacks, onAccept_(_)).WillOnce([&] { dispatcher_->exit(); }); ClientConnectionPtr client_connection = @@ -367,15 +369,12 @@ TEST_P(TcpListenerImplTest, SetListenerRejectFractionZero) { client_connection->connect(); dispatcher_->run(Event::Dispatcher::RunType::Block); - EXPECT_CALL(connection_callbacks, onEvent(ConnectionEvent::LocalClose)); // Now that we've seen that the connection hasn't been closed by the listener, make sure to close // it. client_connection->close(ConnectionCloseType::NoFlush); } TEST_P(TcpListenerImplTest, SetListenerRejectFractionIntermediate) { - testing::InSequence s1; - auto socket = std::make_shared( Network::Test::getCanonicalLoopbackAddress(version_), nullptr, true); MockTcpListenerCallbacks listener_callbacks; @@ -387,12 +386,18 @@ TEST_P(TcpListenerImplTest, SetListenerRejectFractionIntermediate) { listener.setRejectFraction(0.5f); // The first connection will be rejected because the random value is too small. - EXPECT_CALL(connection_callbacks, onEvent(ConnectionEvent::Connected)); - EXPECT_CALL(random_generator, random()).WillOnce(Return(0)); - EXPECT_CALL(listener_callbacks, onReject(TcpListenerCallbacks::RejectCause::OverloadAction)); - EXPECT_CALL(connection_callbacks, onEvent(ConnectionEvent::RemoteClose)).WillOnce([&] { - dispatcher_->exit(); - }); + { + testing::InSequence s1; + EXPECT_CALL(random_generator, random()).WillOnce(Return(0)); + EXPECT_CALL(listener_callbacks, onReject(TcpListenerCallbacks::RejectCause::OverloadAction)); + } + { + testing::InSequence s2; + EXPECT_CALL(connection_callbacks, onEvent(ConnectionEvent::Connected)); + EXPECT_CALL(connection_callbacks, onEvent(ConnectionEvent::RemoteClose)).WillOnce([&] { + dispatcher_->exit(); + }); + } { ClientConnectionPtr client_connection = dispatcher_->createClientConnection( @@ -404,12 +409,18 @@ TEST_P(TcpListenerImplTest, SetListenerRejectFractionIntermediate) { } // The second connection rolls better on initiative and is accepted. - EXPECT_CALL(connection_callbacks, onEvent(ConnectionEvent::Connected)).WillOnce([&] { - dispatcher_->exit(); - }); - EXPECT_CALL(random_generator, random()).WillOnce(Return(std::numeric_limits::max())); - EXPECT_CALL(listener_callbacks, onAccept_(_)); - EXPECT_CALL(connection_callbacks, onEvent(ConnectionEvent::RemoteClose)).Times(0); + { + testing::InSequence s1; + EXPECT_CALL(random_generator, random()).WillOnce(Return(std::numeric_limits::max())); + EXPECT_CALL(listener_callbacks, onAccept_(_)); + } + { + testing::InSequence s2; + EXPECT_CALL(connection_callbacks, onEvent(ConnectionEvent::Connected)).WillOnce([&] { + dispatcher_->exit(); + }); + EXPECT_CALL(connection_callbacks, onEvent(ConnectionEvent::RemoteClose)).Times(0); + } { ClientConnectionPtr client_connection = dispatcher_->createClientConnection( @@ -427,8 +438,6 @@ TEST_P(TcpListenerImplTest, SetListenerRejectFractionIntermediate) { } TEST_P(TcpListenerImplTest, SetListenerRejectFractionAll) { - testing::InSequence s1; - auto socket = std::make_shared( Network::Test::getCanonicalLoopbackAddress(version_), nullptr, true); MockTcpListenerCallbacks listener_callbacks; @@ -439,11 +448,18 @@ TEST_P(TcpListenerImplTest, SetListenerRejectFractionAll) { listener.setRejectFraction(1); - EXPECT_CALL(connection_callbacks, onEvent(ConnectionEvent::Connected)); - EXPECT_CALL(listener_callbacks, onReject(TcpListenerCallbacks::RejectCause::OverloadAction)); - EXPECT_CALL(connection_callbacks, onEvent(ConnectionEvent::RemoteClose)).WillOnce([&] { - dispatcher_->exit(); - }); + { + testing::InSequence s1; + EXPECT_CALL(listener_callbacks, onReject(TcpListenerCallbacks::RejectCause::OverloadAction)); + } + + { + testing::InSequence s2; + EXPECT_CALL(connection_callbacks, onEvent(ConnectionEvent::Connected)); + EXPECT_CALL(connection_callbacks, onEvent(ConnectionEvent::RemoteClose)).WillOnce([&] { + dispatcher_->exit(); + }); + } ClientConnectionPtr client_connection = dispatcher_->createClientConnection(socket->localAddress(), Address::InstanceConstSharedPtr(), From 9cc57ce2e74ea54644ed7e9a4dcb66153db47a7a Mon Sep 17 00:00:00 2001 From: Alex Konradi Date: Tue, 13 Oct 2020 10:33:00 -0400 Subject: [PATCH 8/8] Add ASSERT and stat tests Signed-off-by: Alex Konradi --- source/common/network/tcp_listener_impl.cc | 1 + test/server/connection_handler_test.cc | 30 ++++++++++++++++++++++ 2 files changed, 31 insertions(+) diff --git a/source/common/network/tcp_listener_impl.cc b/source/common/network/tcp_listener_impl.cc index 32ddf272bfad..91975d6ca5d1 100644 --- a/source/common/network/tcp_listener_impl.cc +++ b/source/common/network/tcp_listener_impl.cc @@ -125,6 +125,7 @@ void TcpListenerImpl::enable() { file_event_->setEnabled(Event::FileReadyType::R void TcpListenerImpl::disable() { file_event_->setEnabled(0); } void TcpListenerImpl::setRejectFraction(const float reject_fraction) { + ASSERT(0 <= reject_fraction && reject_fraction <= 1); reject_fraction_ = reject_fraction; } diff --git a/test/server/connection_handler_test.cc b/test/server/connection_handler_test.cc index fcdfa8cf84e0..b286dc588dd2 100644 --- a/test/server/connection_handler_test.cc +++ b/test/server/connection_handler_test.cc @@ -1112,6 +1112,36 @@ TEST_F(ConnectionHandlerTest, TcpListenerRemoveFilterChain) { handler_.reset(); } +TEST_F(ConnectionHandlerTest, TcpListenerGlobalCxLimitReject) { + Network::TcpListenerCallbacks* listener_callbacks; + auto listener = new NiceMock(); + TestListener* test_listener = + addListener(1, true, false, "test_listener", listener, &listener_callbacks); + EXPECT_CALL(*socket_factory_, localAddress()).WillOnce(ReturnRef(local_address_)); + handler_->addListener(absl::nullopt, *test_listener); + + listener_callbacks->onReject(Network::TcpListenerCallbacks::RejectCause::GlobalCxLimit); + + EXPECT_EQ(1UL, TestUtility::findCounter(stats_store_, "downstream_global_cx_overflow")->value()); + EXPECT_EQ(0UL, TestUtility::findCounter(stats_store_, "downstream_cx_overload_reject")->value()); + EXPECT_CALL(*listener, onDestroy()); +} + +TEST_F(ConnectionHandlerTest, TcpListenerOverloadActionReject) { + Network::TcpListenerCallbacks* listener_callbacks; + auto listener = new NiceMock(); + TestListener* test_listener = + addListener(1, true, false, "test_listener", listener, &listener_callbacks); + EXPECT_CALL(*socket_factory_, localAddress()).WillOnce(ReturnRef(local_address_)); + handler_->addListener(absl::nullopt, *test_listener); + + listener_callbacks->onReject(Network::TcpListenerCallbacks::RejectCause::OverloadAction); + + EXPECT_EQ(1UL, TestUtility::findCounter(stats_store_, "downstream_cx_overload_reject")->value()); + EXPECT_EQ(0UL, TestUtility::findCounter(stats_store_, "downstream_global_cx_overflow")->value()); + EXPECT_CALL(*listener, onDestroy()); +} + // Listener Filter matchers works. TEST_F(ConnectionHandlerTest, ListenerFilterWorks) { Network::TcpListenerCallbacks* listener_callbacks;