From 351541789ca28a73e39dd900a72ca6a01c6495a6 Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Thu, 1 Apr 2021 09:13:45 -0700 Subject: [PATCH 01/21] techdebt: clean up connection_handler_impl dependency issue Signed-off-by: Yuchen Dai --- source/common/quic/envoy_quic_dispatcher.h | 1 + source/common/quic/envoy_quic_proof_source.h | 1 + source/server/BUILD | 26 ++------ source/server/active_listener_base.h | 63 ++++++++++++++++++++ source/server/active_tcp_listener.cc | 52 ++++++++-------- source/server/active_tcp_listener.h | 8 ++- source/server/active_udp_listener.cc | 2 +- source/server/active_udp_listener.h | 5 +- source/server/connection_handler_impl.cc | 10 ---- source/server/connection_handler_impl.h | 45 -------------- 10 files changed, 102 insertions(+), 111 deletions(-) create mode 100644 source/server/active_listener_base.h diff --git a/source/common/quic/envoy_quic_dispatcher.h b/source/common/quic/envoy_quic_dispatcher.h index d59307f415ec..85a0a009d713 100644 --- a/source/common/quic/envoy_quic_dispatcher.h +++ b/source/common/quic/envoy_quic_dispatcher.h @@ -18,6 +18,7 @@ #include "envoy/network/listener.h" #include "server/connection_handler_impl.h" +#include "server/active_listener_base.h" namespace Envoy { namespace Quic { diff --git a/source/common/quic/envoy_quic_proof_source.h b/source/common/quic/envoy_quic_proof_source.h index 7642e26c2039..97c2120870b7 100644 --- a/source/common/quic/envoy_quic_proof_source.h +++ b/source/common/quic/envoy_quic_proof_source.h @@ -3,6 +3,7 @@ #include "common/quic/envoy_quic_proof_source_base.h" #include "common/quic/quic_transport_socket_factory.h" +#include "server/active_listener_base.h" #include "server/connection_handler_impl.h" namespace Envoy { diff --git a/source/server/BUILD b/source/server/BUILD index bd7727db6d21..dd46e4e446c0 100644 --- a/source/server/BUILD +++ b/source/server/BUILD @@ -77,6 +77,7 @@ envoy_cc_library( "connection_handler_impl.h", ], deps = [ + ":active_tcp_listener", "//include/envoy/common:time_interface", "//include/envoy/event:deferred_deletable", "//include/envoy/event:dispatcher_interface", @@ -92,38 +93,19 @@ envoy_cc_library( "//source/common/event:deferred_task", "//source/common/network:connection_lib", "//source/common/stream_info:stream_info_lib", - "//source/server:active_tcp_listener_header", ], ) envoy_cc_library( - # Currently both `active_tcp_listener` and `connection_handler_impl` need below headers - # while addressing https://github.com/envoyproxy/envoy/issues/15126 - # TODO(lambdai): Remove the definition of ActiveTcpListener from dependency of ConnectionHandlerImpl - # and delete this target. - name = "active_tcp_listener_header", + name = "active_listener_base", hdrs = [ - "active_tcp_listener.h", - "connection_handler_impl.h", + "active_listener_base.h", ], deps = [ - "//include/envoy/common:time_interface", - "//include/envoy/event:deferred_deletable", - "//include/envoy/event:dispatcher_interface", - "//include/envoy/event:timer_interface", "//include/envoy/network:connection_handler_interface", - "//include/envoy/network:connection_interface", - "//include/envoy/network:filter_interface", - "//include/envoy/network:listen_socket_interface", "//include/envoy/network:listener_interface", - "//include/envoy/server:listener_manager_interface", "//include/envoy/stats:timespan_interface", - "//source/common/common:linked_object", - "//source/common/common:non_copyable", - "//source/common/event:deferred_task", - "//source/common/network:connection_lib", "//source/common/stats:timespan_lib", - "//source/common/stream_info:stream_info_lib", ], ) @@ -153,7 +135,7 @@ envoy_cc_library( "//source/common/stats:timespan_lib", "//source/common/stream_info:stream_info_lib", "//source/extensions/transport_sockets:well_known_names", - "//source/server:active_tcp_listener_header", + "//source/server:active_listener_base", ], ) diff --git a/source/server/active_listener_base.h b/source/server/active_listener_base.h new file mode 100644 index 000000000000..5cfdbe3c5669 --- /dev/null +++ b/source/server/active_listener_base.h @@ -0,0 +1,63 @@ +#pragma once + +#include "envoy/network/connection_handler.h" +#include "envoy/network/listener.h" +#include "envoy/stats/scope.h" + +namespace Envoy { +namespace Server { + +#define ALL_LISTENER_STATS(COUNTER, GAUGE, HISTOGRAM) \ + 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) \ + GAUGE(downstream_cx_active, Accumulate) \ + GAUGE(downstream_pre_cx_active, Accumulate) \ + HISTOGRAM(downstream_cx_length_ms, Milliseconds) + +/** + * Wrapper struct for listener stats. @see stats_macros.h + */ +struct ListenerStats { + ALL_LISTENER_STATS(GENERATE_COUNTER_STRUCT, GENERATE_GAUGE_STRUCT, GENERATE_HISTOGRAM_STRUCT) +}; + +#define ALL_PER_HANDLER_LISTENER_STATS(COUNTER, GAUGE) \ + COUNTER(downstream_cx_total) \ + GAUGE(downstream_cx_active, Accumulate) + +/** + * Wrapper struct for per-handler listener stats. @see stats_macros.h + */ +struct PerHandlerListenerStats { + ALL_PER_HANDLER_LISTENER_STATS(GENERATE_COUNTER_STRUCT, GENERATE_GAUGE_STRUCT) +}; + +/** + * Wrapper for an active listener owned by this handler. + */ +class ActiveListenerImplBase : public virtual Network::ConnectionHandler::ActiveListener { +public: + ActiveListenerImplBase(Network::ConnectionHandler& parent, Network::ListenerConfig* config) + : stats_({ALL_LISTENER_STATS(POOL_COUNTER(config->listenerScope()), + POOL_GAUGE(config->listenerScope()), + POOL_HISTOGRAM(config->listenerScope()))}), + per_worker_stats_({ALL_PER_HANDLER_LISTENER_STATS( + POOL_COUNTER_PREFIX(config->listenerScope(), parent.statPrefix()), + POOL_GAUGE_PREFIX(config->listenerScope(), parent.statPrefix()))}), + config_(config) {} + + // Network::ConnectionHandler::ActiveListener. + uint64_t listenerTag() override { return config_->listenerTag(); } + + ListenerStats stats_; + PerHandlerListenerStats per_worker_stats_; + Network::ListenerConfig* config_{}; +}; + +} // namespace Server +} // namespace Envoy \ No newline at end of file diff --git a/source/server/active_tcp_listener.cc b/source/server/active_tcp_listener.cc index c522aa0e0110..7a5f0a64edcc 100644 --- a/source/server/active_tcp_listener.cc +++ b/source/server/active_tcp_listener.cc @@ -12,8 +12,6 @@ #include "common/network/utility.h" #include "common/stats/timespan_impl.h" -#include "server/connection_handler_impl.h" - #include "extensions/transport_sockets/well_known_names.h" namespace Envoy { @@ -28,25 +26,6 @@ void emitLogs(Network::ListenerConfig& config, StreamInfo::StreamInfo& stream_in } } // namespace -void ActiveTcpListener::removeConnection(ActiveTcpConnection& connection) { - ENVOY_CONN_LOG(debug, "adding to cleanup list", *connection.connection_); - ActiveConnections& active_connections = connection.active_connections_; - ActiveTcpConnectionPtr removed = connection.removeFromList(active_connections.connections_); - parent_.dispatcher().deferredDelete(std::move(removed)); - // Delete map entry only iff connections becomes empty. - if (active_connections.connections_.empty()) { - auto iter = connections_by_context_.find(&active_connections.filter_chain_); - ASSERT(iter != connections_by_context_.end()); - // To cover the lifetime of every single connection, Connections need to be deferred deleted - // because the previously contained connection is deferred deleted. - parent_.dispatcher().deferredDelete(std::move(iter->second)); - // The erase will break the iteration over the connections_by_context_ during the deletion. - if (!is_deleting_) { - connections_by_context_.erase(iter); - } - } -} - ActiveTcpListener::ActiveTcpListener(Network::TcpConnectionHandler& parent, Network::ListenerConfig& config) : ActiveTcpListener( @@ -64,12 +43,6 @@ ActiveTcpListener::ActiveTcpListener(Network::TcpConnectionHandler& parent, config.connectionBalancer().registerHandler(*this); } -void ActiveTcpListener::updateListenerConfig(Network::ListenerConfig& config) { - ENVOY_LOG(trace, "replacing listener ", config_->listenerTag(), " by ", config.listenerTag()); - ASSERT(&config_->connectionBalancer() == &config.connectionBalancer()); - config_ = &config; -} - ActiveTcpListener::~ActiveTcpListener() { is_deleting_ = true; config_->connectionBalancer().unregisterHandler(*this); @@ -99,6 +72,31 @@ ActiveTcpListener::~ActiveTcpListener() { ASSERT(num_listener_connections_ == 0); } +void ActiveTcpListener::removeConnection(ActiveTcpConnection& connection) { + ENVOY_CONN_LOG(debug, "adding to cleanup list", *connection.connection_); + ActiveConnections& active_connections = connection.active_connections_; + ActiveTcpConnectionPtr removed = connection.removeFromList(active_connections.connections_); + parent_.dispatcher().deferredDelete(std::move(removed)); + // Delete map entry only iff connections becomes empty. + if (active_connections.connections_.empty()) { + auto iter = connections_by_context_.find(&active_connections.filter_chain_); + ASSERT(iter != connections_by_context_.end()); + // To cover the lifetime of every single connection, Connections need to be deferred deleted + // because the previously contained connection is deferred deleted. + parent_.dispatcher().deferredDelete(std::move(iter->second)); + // The erase will break the iteration over the connections_by_context_ during the deletion. + if (!is_deleting_) { + connections_by_context_.erase(iter); + } + } +} + +void ActiveTcpListener::updateListenerConfig(Network::ListenerConfig& config) { + ENVOY_LOG(trace, "replacing listener ", config_->listenerTag(), " by ", config.listenerTag()); + ASSERT(&config_->connectionBalancer() == &config.connectionBalancer()); + config_ = &config; +} + void ActiveTcpSocket::onTimeout() { listener_.stats_.downstream_pre_cx_timeout_.inc(); ASSERT(inserted()); diff --git a/source/server/active_tcp_listener.h b/source/server/active_tcp_listener.h index fd4a17160aab..01cf11b72408 100644 --- a/source/server/active_tcp_listener.h +++ b/source/server/active_tcp_listener.h @@ -1,11 +1,13 @@ #pragma once +#include "envoy/event/dispatcher.h" +#include "envoy/event/timer.h" #include "envoy/stats/timespan.h" #include "common/common/linked_object.h" #include "common/stream_info/stream_info_impl.h" -#include "server/connection_handler_impl.h" +#include "server/active_listener_base.h" namespace Envoy { namespace Server { @@ -29,7 +31,7 @@ using RebalancedSocketSharedPtr = std::shared_ptr; * Wrapper for an active tcp listener owned by this handler. */ class ActiveTcpListener : public Network::TcpListenerCallbacks, - public ConnectionHandlerImpl::ActiveListenerImplBase, + public ActiveListenerImplBase, public Network::BalancedConnectionHandler, Logger::Loggable { public: @@ -104,7 +106,7 @@ class ActiveTcpListener : public Network::TcpListenerCallbacks, const std::chrono::milliseconds listener_filters_timeout_; const bool continue_on_listener_filters_timeout_; std::list sockets_; - absl::node_hash_map connections_by_context_; + absl::flat_hash_map connections_by_context_; // The number of connections currently active on this listener. This is typically used for // connection balancing across per-handler listeners. diff --git a/source/server/active_udp_listener.cc b/source/server/active_udp_listener.cc index 7b712069025c..ceb01bbcd14e 100644 --- a/source/server/active_udp_listener.cc +++ b/source/server/active_udp_listener.cc @@ -15,7 +15,7 @@ ActiveUdpListenerBase::ActiveUdpListenerBase(uint32_t worker_index, uint32_t con Network::Socket& listen_socket, Network::UdpListenerPtr&& listener, Network::ListenerConfig* config) - : ConnectionHandlerImpl::ActiveListenerImplBase(parent, config), worker_index_(worker_index), + : ActiveListenerImplBase(parent, config), worker_index_(worker_index), concurrency_(concurrency), parent_(parent), listen_socket_(listen_socket), udp_listener_(std::move(listener)), udp_stats_({ALL_UDP_LISTENER_STATS(POOL_COUNTER_PREFIX(config->listenerScope(), "udp"))}) { diff --git a/source/server/active_udp_listener.h b/source/server/active_udp_listener.h index 645d37e4c8f6..e122b65d2e42 100644 --- a/source/server/active_udp_listener.h +++ b/source/server/active_udp_listener.h @@ -8,8 +8,7 @@ #include "envoy/network/listen_socket.h" #include "envoy/network/listener.h" -// TODO(lambdai): remove connection_handler_impl after ActiveListenerImplBase is extracted from it. -#include "server/connection_handler_impl.h" +#include "server/active_listener_base.h" namespace Envoy { namespace Server { @@ -23,7 +22,7 @@ struct UdpListenerStats { ALL_UDP_LISTENER_STATS(GENERATE_COUNTER_STRUCT) }; -class ActiveUdpListenerBase : public ConnectionHandlerImpl::ActiveListenerImplBase, +class ActiveUdpListenerBase : public ActiveListenerImplBase, public Network::ConnectionHandler::ActiveUdpListener { public: ActiveUdpListenerBase(uint32_t worker_index, uint32_t concurrency, diff --git a/source/server/connection_handler_impl.cc b/source/server/connection_handler_impl.cc index 37983bc08331..882a9fd77612 100644 --- a/source/server/connection_handler_impl.cc +++ b/source/server/connection_handler_impl.cc @@ -212,15 +212,5 @@ ConnectionHandlerImpl::getBalancedHandlerByAddress(const Network::Address::Insta : absl::nullopt; } -ConnectionHandlerImpl::ActiveListenerImplBase::ActiveListenerImplBase( - Network::ConnectionHandler& parent, Network::ListenerConfig* config) - : stats_({ALL_LISTENER_STATS(POOL_COUNTER(config->listenerScope()), - POOL_GAUGE(config->listenerScope()), - POOL_HISTOGRAM(config->listenerScope()))}), - per_worker_stats_({ALL_PER_HANDLER_LISTENER_STATS( - POOL_COUNTER_PREFIX(config->listenerScope(), parent.statPrefix()), - POOL_GAUGE_PREFIX(config->listenerScope(), parent.statPrefix()))}), - config_(config) {} - } // namespace Server } // namespace Envoy diff --git a/source/server/connection_handler_impl.h b/source/server/connection_handler_impl.h index 0d2e8a52f65d..4d73ba79fe21 100644 --- a/source/server/connection_handler_impl.h +++ b/source/server/connection_handler_impl.h @@ -19,36 +19,6 @@ namespace Envoy { namespace Server { -#define ALL_LISTENER_STATS(COUNTER, GAUGE, HISTOGRAM) \ - 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) \ - GAUGE(downstream_cx_active, Accumulate) \ - GAUGE(downstream_pre_cx_active, Accumulate) \ - HISTOGRAM(downstream_cx_length_ms, Milliseconds) - -/** - * Wrapper struct for listener stats. @see stats_macros.h - */ -struct ListenerStats { - ALL_LISTENER_STATS(GENERATE_COUNTER_STRUCT, GENERATE_GAUGE_STRUCT, GENERATE_HISTOGRAM_STRUCT) -}; - -#define ALL_PER_HANDLER_LISTENER_STATS(COUNTER, GAUGE) \ - COUNTER(downstream_cx_total) \ - GAUGE(downstream_cx_active, Accumulate) - -/** - * Wrapper struct for per-handler listener stats. @see stats_macros.h - */ -struct PerHandlerListenerStats { - ALL_PER_HANDLER_LISTENER_STATS(GENERATE_COUNTER_STRUCT, GENERATE_GAUGE_STRUCT) -}; - class ActiveUdpListenerBase; class ActiveTcpListener; @@ -93,21 +63,6 @@ class ConnectionHandlerImpl : public Network::TcpConnectionHandler, // Network::UdpConnectionHandler Network::UdpListenerCallbacksOptRef getUdpListenerCallbacks(uint64_t listener_tag) override; - /** - * Wrapper for an active listener owned by this handler. - */ - class ActiveListenerImplBase : public virtual Network::ConnectionHandler::ActiveListener { - public: - ActiveListenerImplBase(Network::ConnectionHandler& parent, Network::ListenerConfig* config); - - // Network::ConnectionHandler::ActiveListener. - uint64_t listenerTag() override { return config_->listenerTag(); } - - ListenerStats stats_; - PerHandlerListenerStats per_worker_stats_; - Network::ListenerConfig* config_{}; - }; - private: struct ActiveListenerDetails { // Strong pointer to the listener, whether TCP, UDP, QUIC, etc. From 99bf7479e75e640eb38c80f0b5186c24b72c7d42 Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Fri, 2 Apr 2021 00:17:42 -0700 Subject: [PATCH 02/21] pass all existing tests Signed-off-by: Yuchen Dai --- source/server/active_tcp_listener.cc | 16 +++++++++------- source/server/active_tcp_listener.h | 1 + test/server/connection_handler_test.cc | 6 +++--- 3 files changed, 13 insertions(+), 10 deletions(-) diff --git a/source/server/active_tcp_listener.cc b/source/server/active_tcp_listener.cc index 7a5f0a64edcc..2a6aa539f7b1 100644 --- a/source/server/active_tcp_listener.cc +++ b/source/server/active_tcp_listener.cc @@ -69,7 +69,7 @@ ActiveTcpListener::~ActiveTcpListener() { // for now. If it becomes a problem (developers hitting this assert when using debug builds) we // can revisit. This case, if it happens, should be benign on production builds. This case is // covered in ConnectionHandlerTest::RemoveListenerDuringRebalance. - ASSERT(num_listener_connections_ == 0); + ASSERT(num_listener_connections_ == 0, fmt::format("{} {}", config_->name(), numConnections())); } void ActiveTcpListener::removeConnection(ActiveTcpConnection& connection) { @@ -186,16 +186,18 @@ void ActiveTcpSocket::newConnection() { listener_.parent_.getBalancedHandlerByAddress(*socket_->addressProvider().localAddress()); } if (new_listener.has_value()) { + FANCY_LOG(debug, "listener {} to listener {}", listener_.config_->name(), "unknown"); // Hands off connections redirected by iptables to the listener associated with the // original destination address. Pass 'hand_off_restored_destination_connections' as false to - // prevent further redirection as well as 'rebalanced' as true since the connection has - // already been balanced if applicable inside onAcceptWorker() when the connection was - // initially accepted. Note also that we must account for the number of connections properly - // across both listeners. + // prevent further redirection. + // Leave the new listener to decide whether to execute rebalance. + // Note also that we must account for the number of connections properly across both listeners. // TODO(mattklein123): See note in ~ActiveTcpSocket() related to making this accounting better. listener_.decNumConnections(); - new_listener.value().get().incNumConnections(); - new_listener.value().get().onAcceptWorker(std::move(socket_), false, true); + FANCY_LOG(debug, "listener {} has {} conns, new listener unknown has {}", + listener_.config_->name(), listener_.numConnections(), + new_listener.value().get().numConnections()); + new_listener.value().get().onAcceptWorker(std::move(socket_), false, false); } else { // Set default transport protocol if none of the listener filters did it. if (socket_->detectedTransportProtocol().empty()) { diff --git a/source/server/active_tcp_listener.h b/source/server/active_tcp_listener.h index 01cf11b72408..e788490ee385 100644 --- a/source/server/active_tcp_listener.h +++ b/source/server/active_tcp_listener.h @@ -249,6 +249,7 @@ struct ActiveTcpSocket : public Network::ListenerFilterManager, ActiveTcpListener& listener_; Network::ConnectionSocketPtr socket_; const bool hand_off_restored_destination_connections_; + std::list accept_filters_; std::list::iterator iter_; Event::TimerPtr timer_; diff --git a/test/server/connection_handler_test.cc b/test/server/connection_handler_test.cc index 1e9a174fee06..69858ae2fc08 100644 --- a/test/server/connection_handler_test.cc +++ b/test/server/connection_handler_test.cc @@ -415,7 +415,7 @@ TEST_F(ConnectionHandlerTest, ListenerConnectionLimitEnforced) { EXPECT_CALL(dispatcher_, createServerConnection_()).WillOnce(Return(conn1)); listener_callbacks1->onAccept( Network::ConnectionSocketPtr{new NiceMock()}); - EXPECT_EQ(1, handler_->numConnections()); + ASSERT_EQ(1, handler_->numConnections()); // Note that these stats are not the per-worker stats, but the per-listener stats. EXPECT_EQ(1, TestUtility::findCounter(stats_store_, "downstream_cx_total")->value()); EXPECT_EQ(1, TestUtility::findGauge(stats_store_, "downstream_cx_active")->value()); @@ -425,7 +425,7 @@ TEST_F(ConnectionHandlerTest, ListenerConnectionLimitEnforced) { // overflow stat. listener_callbacks1->onAccept( Network::ConnectionSocketPtr{new NiceMock()}); - EXPECT_EQ(1, handler_->numConnections()); + ASSERT_EQ(1, handler_->numConnections()); EXPECT_EQ(1, TestUtility::findCounter(stats_store_, "downstream_cx_total")->value()); EXPECT_EQ(1, TestUtility::findGauge(stats_store_, "downstream_cx_active")->value()); EXPECT_EQ(2, TestUtility::findCounter(stats_store_, "downstream_cx_overflow")->value()); @@ -433,7 +433,7 @@ TEST_F(ConnectionHandlerTest, ListenerConnectionLimitEnforced) { // Check behavior again for good measure. listener_callbacks1->onAccept( Network::ConnectionSocketPtr{new NiceMock()}); - EXPECT_EQ(1, handler_->numConnections()); + ASSERT_EQ(1, handler_->numConnections()); EXPECT_EQ(1, TestUtility::findCounter(stats_store_, "downstream_cx_total")->value()); EXPECT_EQ(1, TestUtility::findGauge(stats_store_, "downstream_cx_active")->value()); EXPECT_EQ(3, TestUtility::findCounter(stats_store_, "downstream_cx_overflow")->value()); From 3b9aec13b7226b447699271fcace683c6cd59225 Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Fri, 2 Apr 2021 01:30:18 -0700 Subject: [PATCH 03/21] add initial active_tcp_listener test Signed-off-by: Yuchen Dai --- test/mocks/network/mocks.h | 2 +- test/server/BUILD | 20 +++ test/server/active_tcp_listener_test.cc | 184 ++++++++++++++++++++++++ 3 files changed, 205 insertions(+), 1 deletion(-) create mode 100644 test/server/active_tcp_listener_test.cc diff --git a/test/mocks/network/mocks.h b/test/mocks/network/mocks.h index 63c4c5e7c2ff..49a055ff327e 100644 --- a/test/mocks/network/mocks.h +++ b/test/mocks/network/mocks.h @@ -421,7 +421,7 @@ class MockListener : public Listener { MOCK_METHOD(void, setRejectFraction, (UnitFloat)); }; -class MockConnectionHandler : public ConnectionHandler { +class MockConnectionHandler : public virtual ConnectionHandler { public: MockConnectionHandler(); ~MockConnectionHandler() override; diff --git a/test/server/BUILD b/test/server/BUILD index edcb9f3fd5ff..d615ee7fe886 100644 --- a/test/server/BUILD +++ b/test/server/BUILD @@ -92,6 +92,26 @@ envoy_cc_test( ], ) +envoy_cc_test( + name = "active_tcp_listener_test", + srcs = ["active_tcp_listener_test.cc"], + deps = [ + "//source/common/common:utility_lib", + "//source/common/config:utility_lib", + "//source/common/network:address_lib", + "//source/common/network:connection_balancer_lib", + "//source/common/stats:stats_lib", + "//source/server:active_raw_udp_listener_config", + "//source/server:connection_handler_lib", + "//test/mocks/access_log:access_log_mocks", + "//test/mocks/api:api_mocks", + "//test/mocks/network:network_mocks", + "//test/test_common:network_utility_lib", + "//test/test_common:threadsafe_singleton_injector_lib", + "@envoy_api//envoy/config/core/v3:pkg_cc_proto", + ], +) + envoy_cc_test( name = "drain_manager_impl_test", srcs = ["drain_manager_impl_test.cc"], diff --git a/test/server/active_tcp_listener_test.cc b/test/server/active_tcp_listener_test.cc new file mode 100644 index 000000000000..d715d2aa9bfb --- /dev/null +++ b/test/server/active_tcp_listener_test.cc @@ -0,0 +1,184 @@ +#include + +#include "envoy/config/core/v3/base.pb.h" +#include "envoy/network/exception.h" +#include "envoy/network/filter.h" +#include "envoy/network/listener.h" +#include "envoy/stats/scope.h" + +#include "common/common/utility.h" +#include "common/config/utility.h" +#include "common/network/address_impl.h" +#include "common/network/connection_balancer_impl.h" +#include "common/network/io_socket_handle_impl.h" +#include "common/network/raw_buffer_socket.h" +#include "common/network/utility.h" + +#include "server/active_tcp_listener.h" + +#include "test/mocks/access_log/mocks.h" +#include "test/mocks/api/mocks.h" +#include "test/mocks/common.h" +#include "test/mocks/network/mocks.h" +#include "test/test_common/network_utility.h" +#include "test/test_common/threadsafe_singleton_injector.h" + +#include "gmock/gmock.h" +#include "gtest/gtest.h" + +using testing::_; +using testing::AtLeast; +using testing::HasSubstr; +using testing::InSequence; +using testing::Invoke; +using testing::NiceMock; +using testing::Return; +using testing::ReturnPointee; +using testing::ReturnRef; +using testing::SaveArg; + +namespace Envoy { +namespace Server { +namespace { + +class MockTcpConnectionHandler : public Network::TcpConnectionHandler, + public Network::MockConnectionHandler { +public: + MOCK_METHOD(Event::Dispatcher&, dispatcher, ()); + MOCK_METHOD(Network::BalancedConnectionHandlerOptRef, getBalancedHandlerByTag, + (uint64_t listener_tag)); + MOCK_METHOD(Network::BalancedConnectionHandlerOptRef, getBalancedHandlerByAddress, + (const Network::Address::Instance& address)); +}; +class ActiveTcpListenerTest : public testing::Test, protected Logger::Loggable { +public: + ActiveTcpListenerTest() { + // EXPECT_CALL(listener_config_, listenerScope).Times(testing::AnyNumber()); + EXPECT_CALL(conn_handler_, statPrefix()).WillRepeatedly(ReturnRef(listener_stat_prefix_)); + listener_filter_matcher_ = std::make_shared>(); + } + // void addListener() { + // EXPECT_CALL(listener_config_, listenerFiltersTimeout()); + // EXPECT_CALL(listener_config_, continueOnListenerFiltersTimeout()); + // EXPECT_CALL(listener_config_, filterChainManager()).WillRepeatedly(ReturnRef(manager_)); + // EXPECT_CALL(listener_config_, + // openConnections()).WillRepeatedly(ReturnRef(resource_limit_)); auto + // mock_listener_will_be_moved = std::unique_ptr(); generic_listener_ = + // mock_listener_will_be_moved.get(); internal_listener_ = + // std::make_shared( + // conn_handler_, dispatcher_, std::move(mock_listener_will_be_moved), listener_config_); + // } + // void expectFilterChainFactory() { + // EXPECT_CALL(listener_config_, filterChainFactory()) + // .WillRepeatedly(ReturnRef(filter_chain_factory_)); + // } + std::string listener_stat_prefix_{"listener_stat_prefix"}; + std::shared_ptr socket_factory_{ + std::make_shared()}; + NiceMock dispatcher_{"test"}; + BasicResourceLimitImpl resource_limit_; + MockTcpConnectionHandler conn_handler_; + Network::MockListener* generic_listener_; + // MockInternalListenerCallback internal_listener_; + Network::MockListenerConfig listener_config_; + NiceMock manager_; + + NiceMock filter_chain_factory_; + std::shared_ptr filter_chain_; + + std::shared_ptr> listener_filter_matcher_; + + // std::shared_ptr active_listener_; +}; + +TEST_F(ActiveTcpListenerTest, RedirectedRebalancer) { + Network::MockListenerConfig listener_config1; + Network::MockConnectionBalancer balancer1; + EXPECT_CALL(listener_config1, connectionBalancer()).WillRepeatedly(ReturnRef(balancer1)); + Network::Address::InstanceConstSharedPtr normal_address( + new Network::Address::Ipv4Instance("127.0.0.1", 10001)); + EXPECT_CALL(*socket_factory_, localAddress()).WillRepeatedly(ReturnRef(normal_address)); + + EXPECT_CALL(listener_config1, listenerFiltersTimeout()); + EXPECT_CALL(listener_config1, continueOnListenerFiltersTimeout()); + EXPECT_CALL(listener_config1, filterChainManager()).WillRepeatedly(ReturnRef(manager_)); + EXPECT_CALL(listener_config1, openConnections()).WillRepeatedly(ReturnRef(resource_limit_)); + auto mock_listener_will_be_moved1 = std::unique_ptr(); + auto listener1 = mock_listener_will_be_moved1.get(); + auto active_listener1 = std::make_shared( + conn_handler_, std::move(mock_listener_will_be_moved1), listener_config1); + + Network::MockListenerConfig listener_config2; + Network::MockConnectionBalancer balancer2; + EXPECT_CALL(listener_config1, connectionBalancer()).WillRepeatedly(ReturnRef(balancer2)); + auto mock_listener_will_be_moved2 = std::unique_ptr(); + auto listener2 = mock_listener_will_be_moved1.get(); + auto active_listener2 = std::make_shared( + conn_handler_, std::move(mock_listener_will_be_moved2), listener_config2); + Network::Address::InstanceConstSharedPtr alt_address( + new Network::Address::Ipv4Instance("127.0.0.2", 20002)); + EXPECT_CALL(*socket_factory_, localAddress()).WillRepeatedly(ReturnRef(alt_address)); + + auto* test_filter = new NiceMock(); + EXPECT_CALL(*test_filter, destroy_()); + Network::MockConnectionSocket* accepted_socket = new NiceMock(); + bool redirected = false; + + // Listener1 rebalance. Set the balance target to the the active listener itself. + EXPECT_CALL(balancer1, pickTargetHandler(_)).WillOnce(ReturnRef(*active_listener1)); + + // Listener1 has one listener filter in the listener filter chain. + EXPECT_CALL(filter_chain_factory_, createListenerFilterChain(_)) + .WillRepeatedly(Invoke([&](Network::ListenerFilterManager& manager) -> bool { + // Insert the Mock filter. + if (!redirected) { + manager.addAcceptFilter(nullptr, Network::ListenerFilterPtr{test_filter}); + redirected = true; + } + return true; + })); + EXPECT_CALL(*test_filter, onAccept(_)) + .WillOnce(Invoke([&](Network::ListenerFilterCallbacks& cb) -> Network::FilterStatus { + cb.socket().addressProvider().restoreLocalAddress(alt_address); + return Network::FilterStatus::Continue; + })); + + // Redirect to Listener2. + EXPECT_CALL(conn_handler_, getBalancedHandlerByAddress(_)) + .WillOnce(Return(Network::BalancedConnectionHandlerOptRef(*active_listener2))); + + // Listener2 rebalance. Set the balance target to the the active listener itself. + EXPECT_CALL(balancer2, pickTargetHandler(_)).WillOnce(ReturnRef(*active_listener2)); + + EXPECT_CALL(manager_, findFilterChain(_)).WillOnce(Return(filter_chain_.get())); + auto* connection = new NiceMock(); + EXPECT_CALL(dispatcher_, createServerConnection_()).WillOnce(Return(connection)); + EXPECT_CALL(filter_chain_factory_, createNetworkFilterChain(_, _)).WillOnce(Return(true)); + active_listener1->onAccept(Network::ConnectionSocketPtr{accepted_socket}); + + // Verify per-listener connection stats. + EXPECT_EQ(1UL, conn_handler_.numConnections()); + // EXPECT_EQ(1UL, TestUtility::findCounter(stats_store_, "downstream_cx_total")->value()); + // EXPECT_EQ(1UL, TestUtility::findGauge(stats_store_, "downstream_cx_active")->value()); + // EXPECT_EQ(1UL, TestUtility::findCounter(stats_store_, "test.downstream_cx_total")->value()); + // EXPECT_EQ(1UL, TestUtility::findGauge(stats_store_, "test.downstream_cx_active")->value()); + + // EXPECT_CALL(*access_log_, log(_, _, _, _)) + // .WillOnce( + // Invoke([&](const Http::RequestHeaderMap*, const Http::ResponseHeaderMap*, + // const Http::ResponseTrailerMap*, const StreamInfo::StreamInfo& + // stream_info) { + // EXPECT_EQ(alt_address, stream_info.downstreamAddressProvider().localAddress()); + // })); + connection->close(Network::ConnectionCloseType::NoFlush); + // dispatcher_.clearDeferredDeleteList(); + // EXPECT_EQ(0UL, TestUtility::findGauge(stats_store_, "downstream_cx_active")->value()); + // EXPECT_EQ(0UL, TestUtility::findGauge(stats_store_, "test.downstream_cx_active")->value()); + + EXPECT_CALL(*listener2, onDestroy()); + EXPECT_CALL(*listener1, onDestroy()); +} + +} // namespace +} // namespace Server +} // namespace Envoy \ No newline at end of file From fd23487aeabed0e7581737b2038868fc60cc5ad6 Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Fri, 2 Apr 2021 11:52:00 -0700 Subject: [PATCH 04/21] fixing test Signed-off-by: Yuchen Dai --- source/server/active_listener_base.h | 3 +- test/mocks/network/mocks.cc | 16 ++- test/mocks/network/mocks.h | 2 + test/server/active_tcp_listener_test.cc | 137 ++++++++++++------------ 4 files changed, 87 insertions(+), 71 deletions(-) diff --git a/source/server/active_listener_base.h b/source/server/active_listener_base.h index 5cfdbe3c5669..fa64000c709d 100644 --- a/source/server/active_listener_base.h +++ b/source/server/active_listener_base.h @@ -58,6 +58,5 @@ class ActiveListenerImplBase : public virtual Network::ConnectionHandler::Active PerHandlerListenerStats per_worker_stats_; Network::ListenerConfig* config_{}; }; - } // namespace Server -} // namespace Envoy \ No newline at end of file +} // namespace Envoy diff --git a/test/mocks/network/mocks.cc b/test/mocks/network/mocks.cc index 7a2ff7458a5c..c91117c9e061 100644 --- a/test/mocks/network/mocks.cc +++ b/test/mocks/network/mocks.cc @@ -171,7 +171,21 @@ MockListener::MockListener() = default; MockListener::~MockListener() { onDestroy(); } -MockConnectionHandler::MockConnectionHandler() = default; +MockConnectionHandler::MockConnectionHandler() { + ON_CALL(*this, incNumConnections()).WillByDefault(Invoke([this]() { + ++num_handler_connections_; + FANCY_LOG(debug, "lambdai conn handler incNumConn(), new value is {}", + num_handler_connections_.load()); + })); + ON_CALL(*this, decNumConnections()).WillByDefault(Invoke([this]() { + --num_handler_connections_; + FANCY_LOG(debug, "lambdai conn handler decNumConn(), new value is {}", + num_handler_connections_.load()); + })); + ON_CALL(*this, numConnections()).WillByDefault(Invoke([this]() { + return num_handler_connections_.load(); + })); +} MockConnectionHandler::~MockConnectionHandler() = default; MockIp::MockIp() = default; diff --git a/test/mocks/network/mocks.h b/test/mocks/network/mocks.h index 49a055ff327e..756b2ed84b21 100644 --- a/test/mocks/network/mocks.h +++ b/test/mocks/network/mocks.h @@ -441,6 +441,8 @@ class MockConnectionHandler : public virtual ConnectionHandler { MOCK_METHOD(void, enableListeners, ()); MOCK_METHOD(void, setListenerRejectFraction, (UnitFloat), (override)); MOCK_METHOD(const std::string&, statPrefix, (), (const)); + + std::atomic num_handler_connections_{}; }; class MockIp : public Address::Ip { diff --git a/test/server/active_tcp_listener_test.cc b/test/server/active_tcp_listener_test.cc index d715d2aa9bfb..294b5fe1f677 100644 --- a/test/server/active_tcp_listener_test.cc +++ b/test/server/active_tcp_listener_test.cc @@ -1,27 +1,19 @@ #include -#include "envoy/config/core/v3/base.pb.h" -#include "envoy/network/exception.h" #include "envoy/network/filter.h" #include "envoy/network/listener.h" #include "envoy/stats/scope.h" -#include "common/common/utility.h" -#include "common/config/utility.h" #include "common/network/address_impl.h" #include "common/network/connection_balancer_impl.h" -#include "common/network/io_socket_handle_impl.h" #include "common/network/raw_buffer_socket.h" #include "common/network/utility.h" - #include "server/active_tcp_listener.h" -#include "test/mocks/access_log/mocks.h" #include "test/mocks/api/mocks.h" #include "test/mocks/common.h" #include "test/mocks/network/mocks.h" #include "test/test_common/network_utility.h" -#include "test/test_common/threadsafe_singleton_injector.h" #include "gmock/gmock.h" #include "gtest/gtest.h" @@ -53,81 +45,86 @@ class MockTcpConnectionHandler : public Network::TcpConnectionHandler, class ActiveTcpListenerTest : public testing::Test, protected Logger::Loggable { public: ActiveTcpListenerTest() { - // EXPECT_CALL(listener_config_, listenerScope).Times(testing::AnyNumber()); + EXPECT_CALL(conn_handler_, dispatcher()).WillRepeatedly(ReturnRef(dispatcher_)); + EXPECT_CALL(conn_handler_, numConnections()).Times(testing::AnyNumber()); EXPECT_CALL(conn_handler_, statPrefix()).WillRepeatedly(ReturnRef(listener_stat_prefix_)); listener_filter_matcher_ = std::make_shared>(); } - // void addListener() { - // EXPECT_CALL(listener_config_, listenerFiltersTimeout()); - // EXPECT_CALL(listener_config_, continueOnListenerFiltersTimeout()); - // EXPECT_CALL(listener_config_, filterChainManager()).WillRepeatedly(ReturnRef(manager_)); - // EXPECT_CALL(listener_config_, - // openConnections()).WillRepeatedly(ReturnRef(resource_limit_)); auto - // mock_listener_will_be_moved = std::unique_ptr(); generic_listener_ = - // mock_listener_will_be_moved.get(); internal_listener_ = - // std::make_shared( - // conn_handler_, dispatcher_, std::move(mock_listener_will_be_moved), listener_config_); - // } - // void expectFilterChainFactory() { - // EXPECT_CALL(listener_config_, filterChainFactory()) - // .WillRepeatedly(ReturnRef(filter_chain_factory_)); - // } + std::string listener_stat_prefix_{"listener_stat_prefix"}; std::shared_ptr socket_factory_{ std::make_shared()}; NiceMock dispatcher_{"test"}; BasicResourceLimitImpl resource_limit_; - MockTcpConnectionHandler conn_handler_; + NiceMock conn_handler_; Network::MockListener* generic_listener_; - // MockInternalListenerCallback internal_listener_; Network::MockListenerConfig listener_config_; NiceMock manager_; - NiceMock filter_chain_factory_; std::shared_ptr filter_chain_; - std::shared_ptr> listener_filter_matcher_; - - // std::shared_ptr active_listener_; }; +// Verify that the server connection with recovered address is rebalanced at redirected listener. TEST_F(ActiveTcpListenerTest, RedirectedRebalancer) { - Network::MockListenerConfig listener_config1; - Network::MockConnectionBalancer balancer1; - EXPECT_CALL(listener_config1, connectionBalancer()).WillRepeatedly(ReturnRef(balancer1)); + NiceMock listener_config1; + NiceMock balancer1; + EXPECT_CALL(balancer1, registerHandler(_)); + EXPECT_CALL(balancer1, unregisterHandler(_)); + Network::Address::InstanceConstSharedPtr normal_address( new Network::Address::Ipv4Instance("127.0.0.1", 10001)); EXPECT_CALL(*socket_factory_, localAddress()).WillRepeatedly(ReturnRef(normal_address)); - + EXPECT_CALL(listener_config1, connectionBalancer()).WillRepeatedly(ReturnRef(balancer1)); + EXPECT_CALL(listener_config1, listenerScope).Times(testing::AnyNumber()); EXPECT_CALL(listener_config1, listenerFiltersTimeout()); EXPECT_CALL(listener_config1, continueOnListenerFiltersTimeout()); EXPECT_CALL(listener_config1, filterChainManager()).WillRepeatedly(ReturnRef(manager_)); EXPECT_CALL(listener_config1, openConnections()).WillRepeatedly(ReturnRef(resource_limit_)); - auto mock_listener_will_be_moved1 = std::unique_ptr(); - auto listener1 = mock_listener_will_be_moved1.get(); - auto active_listener1 = std::make_shared( + EXPECT_CALL(listener_config1, handOffRestoredDestinationConnections()) + .WillRepeatedly(Return(true)); + + auto mock_listener_will_be_moved1 = std::make_unique(); + auto& listener1 = *mock_listener_will_be_moved1; + auto active_listener1 = std::make_unique( conn_handler_, std::move(mock_listener_will_be_moved1), listener_config1); - Network::MockListenerConfig listener_config2; + NiceMock listener_config2; Network::MockConnectionBalancer balancer2; - EXPECT_CALL(listener_config1, connectionBalancer()).WillRepeatedly(ReturnRef(balancer2)); - auto mock_listener_will_be_moved2 = std::unique_ptr(); - auto listener2 = mock_listener_will_be_moved1.get(); - auto active_listener2 = std::make_shared( - conn_handler_, std::move(mock_listener_will_be_moved2), listener_config2); + EXPECT_CALL(balancer2, registerHandler(_)); + EXPECT_CALL(balancer2, unregisterHandler(_)); + Network::Address::InstanceConstSharedPtr alt_address( new Network::Address::Ipv4Instance("127.0.0.2", 20002)); EXPECT_CALL(*socket_factory_, localAddress()).WillRepeatedly(ReturnRef(alt_address)); + EXPECT_CALL(listener_config2, listenerFiltersTimeout()); + EXPECT_CALL(listener_config2, connectionBalancer()).WillRepeatedly(ReturnRef(balancer2)); + EXPECT_CALL(listener_config2, listenerScope).Times(testing::AnyNumber()); + EXPECT_CALL(listener_config2, handOffRestoredDestinationConnections()) + .WillRepeatedly(Return(false)); + EXPECT_CALL(listener_config2, continueOnListenerFiltersTimeout()); + EXPECT_CALL(listener_config2, filterChainManager()).WillRepeatedly(ReturnRef(manager_)); + EXPECT_CALL(listener_config2, openConnections()).WillRepeatedly(ReturnRef(resource_limit_)); + auto mock_listener_will_be_moved2 = std::make_unique(); + auto& listener2 = *mock_listener_will_be_moved2.get(); + auto active_listener2 = std::make_shared( + conn_handler_, std::move(mock_listener_will_be_moved2), listener_config2); auto* test_filter = new NiceMock(); EXPECT_CALL(*test_filter, destroy_()); Network::MockConnectionSocket* accepted_socket = new NiceMock(); bool redirected = false; - // Listener1 rebalance. Set the balance target to the the active listener itself. - EXPECT_CALL(balancer1, pickTargetHandler(_)).WillOnce(ReturnRef(*active_listener1)); + // 1. Listener1 rebalance. Set the balance target to the the active listener itself. + EXPECT_CALL(balancer1, pickTargetHandler(_)) + .WillOnce(testing::DoAll( + testing::WithArg<0>(Invoke([](auto& target) { target.incNumConnections(); })), + ReturnRef(*active_listener1))); - // Listener1 has one listener filter in the listener filter chain. + EXPECT_CALL(listener_config1, filterChainFactory()) + .WillRepeatedly(ReturnRef(filter_chain_factory_)); + + // Listener1 has a listener filter in the listener filter chain. EXPECT_CALL(filter_chain_factory_, createListenerFilterChain(_)) .WillRepeatedly(Invoke([&](Network::ListenerFilterManager& manager) -> bool { // Insert the Mock filter. @@ -142,15 +139,31 @@ TEST_F(ActiveTcpListenerTest, RedirectedRebalancer) { cb.socket().addressProvider().restoreLocalAddress(alt_address); return Network::FilterStatus::Continue; })); + // Verify that listener1 hands off the connection by not creating network filter chain. + EXPECT_CALL(manager_, findFilterChain(_)).Times(0); - // Redirect to Listener2. + // 2. Redirect to Listener2. EXPECT_CALL(conn_handler_, getBalancedHandlerByAddress(_)) .WillOnce(Return(Network::BalancedConnectionHandlerOptRef(*active_listener2))); - // Listener2 rebalance. Set the balance target to the the active listener itself. - EXPECT_CALL(balancer2, pickTargetHandler(_)).WillOnce(ReturnRef(*active_listener2)); + // 3. Listener2 rebalance. Set the balance target to the the active listener itself. + EXPECT_CALL(balancer2, pickTargetHandler(_)) + .WillOnce(testing::DoAll( + testing::WithArg<0>(Invoke([](auto& target) { target.incNumConnections(); })), + ReturnRef(*active_listener2))); + + auto filter_factory_callback = std::make_shared>(); + auto transport_socket_factory = Network::Test::createRawBufferSocketFactory(); + filter_chain_ = std::make_shared>(); + EXPECT_CALL(conn_handler_, incNumConnections()); EXPECT_CALL(manager_, findFilterChain(_)).WillOnce(Return(filter_chain_.get())); + EXPECT_CALL(*filter_chain_, transportSocketFactory) + .WillOnce(testing::ReturnRef(*transport_socket_factory)); + EXPECT_CALL(*filter_chain_, networkFilterFactories).WillOnce(ReturnRef(*filter_factory_callback)); + EXPECT_CALL(listener_config2, filterChainFactory()) + .WillRepeatedly(ReturnRef(filter_chain_factory_)); + auto* connection = new NiceMock(); EXPECT_CALL(dispatcher_, createServerConnection_()).WillOnce(Return(connection)); EXPECT_CALL(filter_chain_factory_, createNetworkFilterChain(_, _)).WillOnce(Return(true)); @@ -158,27 +171,15 @@ TEST_F(ActiveTcpListenerTest, RedirectedRebalancer) { // Verify per-listener connection stats. EXPECT_EQ(1UL, conn_handler_.numConnections()); - // EXPECT_EQ(1UL, TestUtility::findCounter(stats_store_, "downstream_cx_total")->value()); - // EXPECT_EQ(1UL, TestUtility::findGauge(stats_store_, "downstream_cx_active")->value()); - // EXPECT_EQ(1UL, TestUtility::findCounter(stats_store_, "test.downstream_cx_total")->value()); - // EXPECT_EQ(1UL, TestUtility::findGauge(stats_store_, "test.downstream_cx_active")->value()); - - // EXPECT_CALL(*access_log_, log(_, _, _, _)) - // .WillOnce( - // Invoke([&](const Http::RequestHeaderMap*, const Http::ResponseHeaderMap*, - // const Http::ResponseTrailerMap*, const StreamInfo::StreamInfo& - // stream_info) { - // EXPECT_EQ(alt_address, stream_info.downstreamAddressProvider().localAddress()); - // })); + + EXPECT_CALL(conn_handler_, decNumConnections()); connection->close(Network::ConnectionCloseType::NoFlush); - // dispatcher_.clearDeferredDeleteList(); - // EXPECT_EQ(0UL, TestUtility::findGauge(stats_store_, "downstream_cx_active")->value()); - // EXPECT_EQ(0UL, TestUtility::findGauge(stats_store_, "test.downstream_cx_active")->value()); - EXPECT_CALL(*listener2, onDestroy()); - EXPECT_CALL(*listener1, onDestroy()); + EXPECT_CALL(listener1, onDestroy()); + active_listener1.reset(); + EXPECT_CALL(listener2, onDestroy()); + active_listener2.reset(); } - } // namespace } // namespace Server -} // namespace Envoy \ No newline at end of file +} // namespace Envoy From a5f26c9290e6997f3ab3cd86214e2ffb338819a0 Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Fri, 2 Apr 2021 15:32:29 -0700 Subject: [PATCH 05/21] llvm-11 formatter Signed-off-by: Yuchen Dai --- source/common/quic/platform/quic_logging_impl.h | 2 +- test/server/BUILD | 1 - test/server/active_tcp_listener_test.cc | 1 + 3 files changed, 2 insertions(+), 2 deletions(-) diff --git a/source/common/quic/platform/quic_logging_impl.h b/source/common/quic/platform/quic_logging_impl.h index 74966040510d..91504224d1eb 100644 --- a/source/common/quic/platform/quic_logging_impl.h +++ b/source/common/quic/platform/quic_logging_impl.h @@ -147,7 +147,7 @@ enum { // DFATAL is FATAL in debug mode, ERROR in release mode. #ifdef NDEBUG LogLevelDFATAL = LogLevelERROR, -#else // NDEBUG +#else // NDEBUG LogLevelDFATAL = LogLevelFATAL, #endif // NDEBUG }; diff --git a/test/server/BUILD b/test/server/BUILD index d615ee7fe886..429fe8131d6c 100644 --- a/test/server/BUILD +++ b/test/server/BUILD @@ -108,7 +108,6 @@ envoy_cc_test( "//test/mocks/network:network_mocks", "//test/test_common:network_utility_lib", "//test/test_common:threadsafe_singleton_injector_lib", - "@envoy_api//envoy/config/core/v3:pkg_cc_proto", ], ) diff --git a/test/server/active_tcp_listener_test.cc b/test/server/active_tcp_listener_test.cc index 294b5fe1f677..6396cfc392fa 100644 --- a/test/server/active_tcp_listener_test.cc +++ b/test/server/active_tcp_listener_test.cc @@ -8,6 +8,7 @@ #include "common/network/connection_balancer_impl.h" #include "common/network/raw_buffer_socket.h" #include "common/network/utility.h" + #include "server/active_tcp_listener.h" #include "test/mocks/api/mocks.h" From aede638c959d8a2988f4aa964df972be05db78da Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Mon, 5 Apr 2021 11:31:19 -0700 Subject: [PATCH 06/21] fix format Signed-off-by: Yuchen Dai --- source/common/quic/platform/quic_logging_impl.h | 2 +- source/server/active_tcp_listener.cc | 2 +- test/server/active_tcp_listener_test.cc | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/source/common/quic/platform/quic_logging_impl.h b/source/common/quic/platform/quic_logging_impl.h index 91504224d1eb..74966040510d 100644 --- a/source/common/quic/platform/quic_logging_impl.h +++ b/source/common/quic/platform/quic_logging_impl.h @@ -147,7 +147,7 @@ enum { // DFATAL is FATAL in debug mode, ERROR in release mode. #ifdef NDEBUG LogLevelDFATAL = LogLevelERROR, -#else // NDEBUG +#else // NDEBUG LogLevelDFATAL = LogLevelFATAL, #endif // NDEBUG }; diff --git a/source/server/active_tcp_listener.cc b/source/server/active_tcp_listener.cc index 2a6aa539f7b1..b68cf9952925 100644 --- a/source/server/active_tcp_listener.cc +++ b/source/server/active_tcp_listener.cc @@ -190,7 +190,7 @@ void ActiveTcpSocket::newConnection() { // Hands off connections redirected by iptables to the listener associated with the // original destination address. Pass 'hand_off_restored_destination_connections' as false to // prevent further redirection. - // Leave the new listener to decide whether to execute rebalance. + // Leave the new listener to decide whether to execute re-balance. // Note also that we must account for the number of connections properly across both listeners. // TODO(mattklein123): See note in ~ActiveTcpSocket() related to making this accounting better. listener_.decNumConnections(); diff --git a/test/server/active_tcp_listener_test.cc b/test/server/active_tcp_listener_test.cc index 6396cfc392fa..1ca46aa93434 100644 --- a/test/server/active_tcp_listener_test.cc +++ b/test/server/active_tcp_listener_test.cc @@ -116,7 +116,7 @@ TEST_F(ActiveTcpListenerTest, RedirectedRebalancer) { Network::MockConnectionSocket* accepted_socket = new NiceMock(); bool redirected = false; - // 1. Listener1 rebalance. Set the balance target to the the active listener itself. + // 1. Listener1 re-balance. Set the balance target to the the active listener itself. EXPECT_CALL(balancer1, pickTargetHandler(_)) .WillOnce(testing::DoAll( testing::WithArg<0>(Invoke([](auto& target) { target.incNumConnections(); })), @@ -147,7 +147,7 @@ TEST_F(ActiveTcpListenerTest, RedirectedRebalancer) { EXPECT_CALL(conn_handler_, getBalancedHandlerByAddress(_)) .WillOnce(Return(Network::BalancedConnectionHandlerOptRef(*active_listener2))); - // 3. Listener2 rebalance. Set the balance target to the the active listener itself. + // 3. Listener2 re-balance. Set the balance target to the the active listener itself. EXPECT_CALL(balancer2, pickTargetHandler(_)) .WillOnce(testing::DoAll( testing::WithArg<0>(Invoke([](auto& target) { target.incNumConnections(); })), From fddcb00d026e7a422385086e1dde6a9cd32518f6 Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Mon, 5 Apr 2021 16:00:16 -0700 Subject: [PATCH 07/21] fix quic Signed-off-by: Yuchen Dai --- test/common/quic/BUILD | 1 + test/common/quic/envoy_quic_server_stream_test.cc | 1 + 2 files changed, 2 insertions(+) diff --git a/test/common/quic/BUILD b/test/common/quic/BUILD index 567e93adce0e..fc2565773d1b 100644 --- a/test/common/quic/BUILD +++ b/test/common/quic/BUILD @@ -79,6 +79,7 @@ envoy_cc_test( "//source/common/quic:envoy_quic_connection_helper_lib", "//source/common/quic:envoy_quic_server_connection_lib", "//source/common/quic:envoy_quic_server_session_lib", + "//source/server:active_listener_base", "//test/mocks/http:http_mocks", "//test/mocks/http:stream_decoder_mock", "//test/mocks/network:network_mocks", diff --git a/test/common/quic/envoy_quic_server_stream_test.cc b/test/common/quic/envoy_quic_server_stream_test.cc index fc7bcfbf8521..a967251b2fbf 100644 --- a/test/common/quic/envoy_quic_server_stream_test.cc +++ b/test/common/quic/envoy_quic_server_stream_test.cc @@ -15,6 +15,7 @@ #include "common/event/libevent_scheduler.h" #include "common/http/headers.h" +#include "server/active_listener_base.h" #include "common/quic/envoy_quic_alarm_factory.h" #include "common/quic/envoy_quic_connection_helper.h" From 0532563a8579ce08e063cbe558b3b1a44c0e1bd0 Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Mon, 5 Apr 2021 23:51:27 -0700 Subject: [PATCH 08/21] ctidy Signed-off-by: Yuchen Dai --- source/server/active_tcp_listener.h | 8 ++++---- test/server/active_tcp_listener_test.cc | 7 +------ 2 files changed, 5 insertions(+), 10 deletions(-) diff --git a/source/server/active_tcp_listener.h b/source/server/active_tcp_listener.h index e788490ee385..b49037caf8b3 100644 --- a/source/server/active_tcp_listener.h +++ b/source/server/active_tcp_listener.h @@ -30,10 +30,10 @@ using RebalancedSocketSharedPtr = std::shared_ptr; /** * Wrapper for an active tcp listener owned by this handler. */ -class ActiveTcpListener : public Network::TcpListenerCallbacks, - public ActiveListenerImplBase, - public Network::BalancedConnectionHandler, - Logger::Loggable { +class ActiveTcpListener final : public Network::TcpListenerCallbacks, + public ActiveListenerImplBase, + public Network::BalancedConnectionHandler, + Logger::Loggable { public: ActiveTcpListener(Network::TcpConnectionHandler& parent, Network::ListenerConfig& config); ActiveTcpListener(Network::TcpConnectionHandler& parent, Network::ListenerPtr&& listener, diff --git a/test/server/active_tcp_listener_test.cc b/test/server/active_tcp_listener_test.cc index 1ca46aa93434..cc1e2710f9f2 100644 --- a/test/server/active_tcp_listener_test.cc +++ b/test/server/active_tcp_listener_test.cc @@ -20,15 +20,10 @@ #include "gtest/gtest.h" using testing::_; -using testing::AtLeast; -using testing::HasSubstr; -using testing::InSequence; using testing::Invoke; using testing::NiceMock; using testing::Return; -using testing::ReturnPointee; using testing::ReturnRef; -using testing::SaveArg; namespace Envoy { namespace Server { @@ -107,7 +102,7 @@ TEST_F(ActiveTcpListenerTest, RedirectedRebalancer) { EXPECT_CALL(listener_config2, filterChainManager()).WillRepeatedly(ReturnRef(manager_)); EXPECT_CALL(listener_config2, openConnections()).WillRepeatedly(ReturnRef(resource_limit_)); auto mock_listener_will_be_moved2 = std::make_unique(); - auto& listener2 = *mock_listener_will_be_moved2.get(); + auto& listener2 = *mock_listener_will_be_moved2; auto active_listener2 = std::make_shared( conn_handler_, std::move(mock_listener_will_be_moved2), listener_config2); From 9ae749006daee4128595e4b1e1dd47a2fc55ed57 Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Wed, 7 Apr 2021 13:03:34 -0700 Subject: [PATCH 09/21] fix merge Signed-off-by: Yuchen Dai --- source/server/active_tcp_listener.cc | 28 ++-------------------------- 1 file changed, 2 insertions(+), 26 deletions(-) diff --git a/source/server/active_tcp_listener.cc b/source/server/active_tcp_listener.cc index 612e6f734360..29501471cefd 100644 --- a/source/server/active_tcp_listener.cc +++ b/source/server/active_tcp_listener.cc @@ -69,32 +69,8 @@ ActiveTcpListener::~ActiveTcpListener() { // for now. If it becomes a problem (developers hitting this assert when using debug builds) we // can revisit. This case, if it happens, should be benign on production builds. This case is // covered in ConnectionHandlerTest::RemoveListenerDuringRebalance. - ASSERT(num_listener_connections_ == 0, fmt::format("{} {}", config_->name(), numConnections())); -} - -void ActiveTcpListener::removeConnection(ActiveTcpConnection& connection) { - ENVOY_CONN_LOG(debug, "adding to cleanup list", *connection.connection_); - ActiveConnections& active_connections = connection.active_connections_; - ActiveTcpConnectionPtr removed = connection.removeFromList(active_connections.connections_); - parent_.dispatcher().deferredDelete(std::move(removed)); - // Delete map entry only iff connections becomes empty. - if (active_connections.connections_.empty()) { - auto iter = connections_by_context_.find(&active_connections.filter_chain_); - ASSERT(iter != connections_by_context_.end()); - // To cover the lifetime of every single connection, Connections need to be deferred deleted - // because the previously contained connection is deferred deleted. - parent_.dispatcher().deferredDelete(std::move(iter->second)); - // The erase will break the iteration over the connections_by_context_ during the deletion. - if (!is_deleting_) { - connections_by_context_.erase(iter); - } - } -} - -void ActiveTcpListener::updateListenerConfig(Network::ListenerConfig& config) { - ENVOY_LOG(trace, "replacing listener ", config_->listenerTag(), " by ", config.listenerTag()); - ASSERT(&config_->connectionBalancer() == &config.connectionBalancer()); - config_ = &config; + ASSERT(num_listener_connections_ == 0, fmt::format("destroyed listener {} has {} connections", + config_->name(), numConnections())); } void ActiveTcpListener::removeConnection(ActiveTcpConnection& connection) { From ad29747224878193dd078cba24d6c3cfbdfaaebc Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Wed, 7 Apr 2021 13:42:20 -0700 Subject: [PATCH 10/21] clean up Signed-off-by: Yuchen Dai --- source/server/active_tcp_listener.cc | 4 ---- test/mocks/network/mocks.cc | 4 ---- test/server/connection_handler_test.cc | 6 +++--- 3 files changed, 3 insertions(+), 11 deletions(-) diff --git a/source/server/active_tcp_listener.cc b/source/server/active_tcp_listener.cc index 29501471cefd..5f9b48e98fac 100644 --- a/source/server/active_tcp_listener.cc +++ b/source/server/active_tcp_listener.cc @@ -187,7 +187,6 @@ void ActiveTcpSocket::newConnection() { listener_.parent_.getBalancedHandlerByAddress(*socket_->addressProvider().localAddress()); } if (new_listener.has_value()) { - FANCY_LOG(debug, "listener {} to listener {}", listener_.config_->name(), "unknown"); // Hands off connections redirected by iptables to the listener associated with the // original destination address. Pass 'hand_off_restored_destination_connections' as false to // prevent further redirection. @@ -195,9 +194,6 @@ void ActiveTcpSocket::newConnection() { // Note also that we must account for the number of connections properly across both listeners. // TODO(mattklein123): See note in ~ActiveTcpSocket() related to making this accounting better. listener_.decNumConnections(); - FANCY_LOG(debug, "listener {} has {} conns, new listener unknown has {}", - listener_.config_->name(), listener_.numConnections(), - new_listener.value().get().numConnections()); new_listener.value().get().onAcceptWorker(std::move(socket_), false, false); } else { // Set default transport protocol if none of the listener filters did it. diff --git a/test/mocks/network/mocks.cc b/test/mocks/network/mocks.cc index c91117c9e061..f08dffa10fe5 100644 --- a/test/mocks/network/mocks.cc +++ b/test/mocks/network/mocks.cc @@ -174,13 +174,9 @@ MockListener::~MockListener() { onDestroy(); } MockConnectionHandler::MockConnectionHandler() { ON_CALL(*this, incNumConnections()).WillByDefault(Invoke([this]() { ++num_handler_connections_; - FANCY_LOG(debug, "lambdai conn handler incNumConn(), new value is {}", - num_handler_connections_.load()); })); ON_CALL(*this, decNumConnections()).WillByDefault(Invoke([this]() { --num_handler_connections_; - FANCY_LOG(debug, "lambdai conn handler decNumConn(), new value is {}", - num_handler_connections_.load()); })); ON_CALL(*this, numConnections()).WillByDefault(Invoke([this]() { return num_handler_connections_.load(); diff --git a/test/server/connection_handler_test.cc b/test/server/connection_handler_test.cc index 69858ae2fc08..1e9a174fee06 100644 --- a/test/server/connection_handler_test.cc +++ b/test/server/connection_handler_test.cc @@ -415,7 +415,7 @@ TEST_F(ConnectionHandlerTest, ListenerConnectionLimitEnforced) { EXPECT_CALL(dispatcher_, createServerConnection_()).WillOnce(Return(conn1)); listener_callbacks1->onAccept( Network::ConnectionSocketPtr{new NiceMock()}); - ASSERT_EQ(1, handler_->numConnections()); + EXPECT_EQ(1, handler_->numConnections()); // Note that these stats are not the per-worker stats, but the per-listener stats. EXPECT_EQ(1, TestUtility::findCounter(stats_store_, "downstream_cx_total")->value()); EXPECT_EQ(1, TestUtility::findGauge(stats_store_, "downstream_cx_active")->value()); @@ -425,7 +425,7 @@ TEST_F(ConnectionHandlerTest, ListenerConnectionLimitEnforced) { // overflow stat. listener_callbacks1->onAccept( Network::ConnectionSocketPtr{new NiceMock()}); - ASSERT_EQ(1, handler_->numConnections()); + EXPECT_EQ(1, handler_->numConnections()); EXPECT_EQ(1, TestUtility::findCounter(stats_store_, "downstream_cx_total")->value()); EXPECT_EQ(1, TestUtility::findGauge(stats_store_, "downstream_cx_active")->value()); EXPECT_EQ(2, TestUtility::findCounter(stats_store_, "downstream_cx_overflow")->value()); @@ -433,7 +433,7 @@ TEST_F(ConnectionHandlerTest, ListenerConnectionLimitEnforced) { // Check behavior again for good measure. listener_callbacks1->onAccept( Network::ConnectionSocketPtr{new NiceMock()}); - ASSERT_EQ(1, handler_->numConnections()); + EXPECT_EQ(1, handler_->numConnections()); EXPECT_EQ(1, TestUtility::findCounter(stats_store_, "downstream_cx_total")->value()); EXPECT_EQ(1, TestUtility::findGauge(stats_store_, "downstream_cx_active")->value()); EXPECT_EQ(3, TestUtility::findCounter(stats_store_, "downstream_cx_overflow")->value()); From 4c28c5db890a1f713b640ef512b5aa16af6b0242 Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Thu, 15 Apr 2021 10:04:15 -0700 Subject: [PATCH 11/21] dont use atomic in mock Signed-off-by: Yuchen Dai --- test/mocks/network/mocks.cc | 2 +- test/mocks/network/mocks.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/test/mocks/network/mocks.cc b/test/mocks/network/mocks.cc index f08dffa10fe5..5575eeb722bb 100644 --- a/test/mocks/network/mocks.cc +++ b/test/mocks/network/mocks.cc @@ -179,7 +179,7 @@ MockConnectionHandler::MockConnectionHandler() { --num_handler_connections_; })); ON_CALL(*this, numConnections()).WillByDefault(Invoke([this]() { - return num_handler_connections_.load(); + return num_handler_connections_; })); } MockConnectionHandler::~MockConnectionHandler() = default; diff --git a/test/mocks/network/mocks.h b/test/mocks/network/mocks.h index 756b2ed84b21..c102194d4016 100644 --- a/test/mocks/network/mocks.h +++ b/test/mocks/network/mocks.h @@ -442,7 +442,7 @@ class MockConnectionHandler : public virtual ConnectionHandler { MOCK_METHOD(void, setListenerRejectFraction, (UnitFloat), (override)); MOCK_METHOD(const std::string&, statPrefix, (), (const)); - std::atomic num_handler_connections_{}; + uint64_t num_handler_connections_{}; }; class MockIp : public Address::Ip { From 01b8d5ac5f245af294bf3d015a0ca4c15d151ce6 Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Fri, 23 Apr 2021 14:07:03 -0700 Subject: [PATCH 12/21] add integration test Signed-off-by: Yuchen Dai --- test/integration/BUILD | 2 + test/integration/filters/BUILD | 19 ++++ .../address_restore_listener_filter.cc | 62 +++++++++++++ .../listener_lds_integration_test.cc | 92 +++++++++++++++++++ 4 files changed, 175 insertions(+) create mode 100644 test/integration/filters/address_restore_listener_filter.cc diff --git a/test/integration/BUILD b/test/integration/BUILD index 97e39f4d156c..459c21a30656 100644 --- a/test/integration/BUILD +++ b/test/integration/BUILD @@ -1544,6 +1544,8 @@ envoy_cc_test( "//source/common/network:utility_lib", "//source/extensions/filters/http/health_check:config", "//test/common/grpc:grpc_client_integration_lib", + "//test/integration/filters:address_restore_listener_filter_lib", + "//source/extensions/filters/listener/original_dst:config", "//test/test_common:resources_lib", "//test/test_common:utility_lib", "@envoy_api//envoy/config/bootstrap/v3:pkg_cc_proto", diff --git a/test/integration/filters/BUILD b/test/integration/filters/BUILD index 6ab4dbab5e54..334259bb1aff 100644 --- a/test/integration/filters/BUILD +++ b/test/integration/filters/BUILD @@ -447,3 +447,22 @@ envoy_cc_test_library( "//test/extensions/filters/http/common:empty_http_filter_config_lib", ], ) + +envoy_cc_test_library( + name = "address_restore_listener_filter_lib", + srcs = [ + "address_restore_listener_filter.cc", + ], + deps = [ + ":common_lib", + "//include/envoy/network:filter_interface", + "//include/envoy/network:listen_socket_interface", + "//include/envoy/registry", + "//include/envoy/server:filter_config_interface", + "//source/common/common:assert_lib", + "//source/common/common:minimal_logger_lib", + "//source/common/network:address_lib", + "//source/common/network:upstream_socket_options_filter_state_lib", + "//source/common/network:utility_lib", + ], +) diff --git a/test/integration/filters/address_restore_listener_filter.cc b/test/integration/filters/address_restore_listener_filter.cc new file mode 100644 index 000000000000..b9cc29b12af3 --- /dev/null +++ b/test/integration/filters/address_restore_listener_filter.cc @@ -0,0 +1,62 @@ + + +#include "envoy/network/filter.h" + +#include "common/network/address_impl.h" +#include "envoy/network/listen_socket.h" +#include "common/network/utility.h" +#include "envoy/server/filter_config.h" + +namespace Envoy { + +class FakeListenerFilter : public Network::ListenerFilter { +public: + // Network::ListenerFilter + Network::FilterStatus onAccept(Network::ListenerFilterCallbacks& cb) override { + FANCY_LOG(info, "calling FakeListenerFilter::onAccept"); + Network::ConnectionSocket& socket = cb.socket(); + FANCY_LOG(info, "lambdai: current local socket address is {} restored = {}", + socket.addressProvider().localAddress()->asString(), + socket.addressProvider().localAddressRestored()); + FANCY_LOG(info, "lambdai: current remote socket address is {}", + socket.addressProvider().remoteAddress()->asString()); + socket.addressProvider().restoreLocalAddress( + std::make_shared("127.0.0.2", 80)); + FANCY_LOG(info, "lambdai: current local socket address is {} restored = {}", + socket.addressProvider().localAddress()->asString(), + socket.addressProvider().localAddressRestored()); + FANCY_LOG(info, "lambdai: current remote socket address is {}", + socket.addressProvider().remoteAddress()->asString()); + return Network::FilterStatus::Continue; + } +}; + +class FakeListenerFilterConfigFactory + : public Server::Configuration::NamedListenerFilterConfigFactory { +public: + // NamedListenerFilterConfigFactory + Network::ListenerFilterFactoryCb createListenerFilterFactoryFromProto( + const Protobuf::Message&, + const Network::ListenerFilterMatcherSharedPtr& listener_filter_matcher, + Server::Configuration::ListenerFactoryContext&) override { + return [listener_filter_matcher](Network::ListenerFilterManager& filter_manager) -> void { + filter_manager.addAcceptFilter(listener_filter_matcher, + std::make_unique()); + }; + } + + ProtobufTypes::MessagePtr createEmptyConfigProto() override { + return ProtobufTypes::MessagePtr{new Envoy::ProtobufWkt::Struct()}; + } + + std::string name() const override { return "envoy.listener.test_address_restore"; } +}; + +static Registry::RegisterFactory + register_; + +// REGISTER_FACTORY(FakeListenerFilterConfigFactory, +// Server::Configuration::NamedListenerFilterConfigFactory){ +// "envoy.listener.test_address_restore"}; +} // namespace Envoy \ No newline at end of file diff --git a/test/integration/listener_lds_integration_test.cc b/test/integration/listener_lds_integration_test.cc index e256e8a09275..4495efccb408 100644 --- a/test/integration/listener_lds_integration_test.cc +++ b/test/integration/listener_lds_integration_test.cc @@ -17,6 +17,7 @@ #include "absl/strings/str_cat.h" #include "gmock/gmock.h" #include "gtest/gtest.h" +#include namespace Envoy { namespace { @@ -409,5 +410,96 @@ TEST_P(ListenerIntegrationTest, MultipleLdsUpdatesSharingListenSocketFactory) { } } +// Tests that a LDS adding listener works as expected. +TEST_P(ListenerIntegrationTest, RedirectConnectionIsBalancedOnDestinationListener) { + concurrency_ = 2; + + // filter->mutable_typed_config(); + on_server_init_function_ = [&]() { + createLdsStream(); + + auto src_listener_config = listener_config_; + src_listener_config.mutable_use_original_dst()->set_value(true); + src_listener_config.add_listener_filters()->set_name("envoy.listener.test_address_restore"); + //src_listener_config.clear_filter_chains(); + auto dst_listener_config = src_listener_config; + dst_listener_config.mutable_use_original_dst()->set_value(false); + dst_listener_config.clear_listener_filters(); + dst_listener_config.mutable_bind_to_port()->set_value(false); + dst_listener_config.set_name("balanced_target_listener"); + *dst_listener_config.mutable_address()->mutable_socket_address()->mutable_address() = + "127.0.0.2"; // defined in test_address_restore listener filter. + dst_listener_config.mutable_address()->mutable_socket_address()->set_port_value(80); + + sendLdsResponse({src_listener_config, dst_listener_config}, "1"); + createRdsStream(route_table_name_); + }; + initialize(); + FANCY_LOG(info, "lambdai: {} ", __LINE__); + // test_server_->waitForCounterGe("listener_manager.lds.update_success", 1); + FANCY_LOG(info, "lambdai: {} ", __LINE__); + // testing-listener-0 is not initialized as we haven't pushed any RDS yet. + EXPECT_EQ(test_server_->server().initManager().state(), Init::Manager::State::Initializing); + + FANCY_LOG(info, "lambdai: {} ", __LINE__); + + // Workers not started, the LDS added listener 0 is in active_listeners_ list. + EXPECT_EQ(test_server_->server().listenerManager().listeners().size(), 2); + registerTestServerPorts({listener_name_}); + + const std::string route_config_tmpl = R"EOF( + name: {} + virtual_hosts: + - name: integration + domains: ["*"] + routes: + - match: {{ prefix: "/" }} + route: {{ cluster: {} }} +)EOF"; + sendRdsResponse(fmt::format(route_config_tmpl, route_table_name_, "cluster_0"), "1"); + FANCY_LOG(info, "lambdai: {} ", __LINE__); + test_server_->waitForCounterGe( + fmt::format("http.config_test.rds.{}.update_success", route_table_name_), 1); + FANCY_LOG(info, "lambdai: {} ", __LINE__); + // Now testing-listener-0 finishes initialization, Server initManager will be ready. + EXPECT_EQ(test_server_->server().initManager().state(), Init::Manager::State::Initialized); + FANCY_LOG(info, "lambdai: {} ", __LINE__); + test_server_->waitUntilListenersReady(); + FANCY_LOG(info, "lambdai: {} ", __LINE__); + // NOTE: The line above doesn't tell you if listener is up and listening. + test_server_->waitForCounterGe("listener_manager.listener_create_success", 1); + // Request is sent to cluster_0. + + codec_client_ = makeHttpConnection(lookupPort(listener_name_)); + int response_size = 800; + int request_size = 10; + Http::TestResponseHeaderMapImpl response_headers{{":status", "200"}, + {"server_id", "cluster_0, backend_0"}}; + auto response = sendRequestAndWaitForResponse( + Http::TestResponseHeaderMapImpl{ + {":method", "GET"}, {":path", "/"}, {":authority", "host"}, {":scheme", "http"}}, + request_size, response_headers, response_size, /*cluster_0*/ 0); + verifyResponse(std::move(response), "200", response_headers, std::string(response_size, 'a')); + test_server_->waitForCounterGe("listener_manager.listener_create_success", 2); + std::this_thread::sleep_for(std::chrono::seconds(10)); + + FANCY_LOG(info, "lambdai cx {}", Network::Test::getLoopbackAddressString(ipVersion())); + FANCY_LOG( + info, "worker 0 cx {}", + TestUtility::findCounter(test_server_->statStore(), + fmt::format("listener.{}_0.worker_0.downstream_cx_total", + Network::Test::getLoopbackAddressString(ipVersion()))) + ->value()); + FANCY_LOG( + info, "worker 1 cx {}", + TestUtility::findCounter(test_server_->statStore(), + fmt::format("listener.{}_0.worker_1.downstream_cx_total", + Network::Test::getLoopbackAddressString(ipVersion()))) + ->value()); + + EXPECT_TRUE(upstream_request_->complete()); + EXPECT_EQ(request_size, upstream_request_->bodyLength()); +} + } // namespace } // namespace Envoy From cda38a998f34bfd54011726fc1a1398fbc13fe26 Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Fri, 23 Apr 2021 18:49:25 -0700 Subject: [PATCH 13/21] cleanup Signed-off-by: Yuchen Dai --- source/server/active_tcp_listener.cc | 2 +- source/server/active_tcp_listener.h | 1 - test/integration/BUILD | 3 +- .../address_restore_listener_filter.cc | 35 ++-- .../listener_lds_integration_test.cc | 174 +++++++++--------- 5 files changed, 108 insertions(+), 107 deletions(-) diff --git a/source/server/active_tcp_listener.cc b/source/server/active_tcp_listener.cc index 5f9b48e98fac..4146ed86b39a 100644 --- a/source/server/active_tcp_listener.cc +++ b/source/server/active_tcp_listener.cc @@ -249,7 +249,7 @@ void ActiveTcpListener::onAcceptWorker(Network::ConnectionSocketPtr&& socket, auto active_socket = std::make_unique(*this, std::move(socket), hand_off_restored_destination_connections); - // Create and run the filters + // Create and run the filters. config_->filterChainFactory().createListenerFilterChain(*active_socket); active_socket->continueFilterChain(true); diff --git a/source/server/active_tcp_listener.h b/source/server/active_tcp_listener.h index b49037caf8b3..c698faaa0605 100644 --- a/source/server/active_tcp_listener.h +++ b/source/server/active_tcp_listener.h @@ -249,7 +249,6 @@ struct ActiveTcpSocket : public Network::ListenerFilterManager, ActiveTcpListener& listener_; Network::ConnectionSocketPtr socket_; const bool hand_off_restored_destination_connections_; - std::list accept_filters_; std::list::iterator iter_; Event::TimerPtr timer_; diff --git a/test/integration/BUILD b/test/integration/BUILD index 9de18c73dee3..7d0ed082a021 100644 --- a/test/integration/BUILD +++ b/test/integration/BUILD @@ -1547,15 +1547,16 @@ envoy_cc_test( "//source/common/network:connection_lib", "//source/common/network:utility_lib", "//source/extensions/filters/http/health_check:config", + "//source/extensions/filters/network/tcp_proxy:config", "//test/common/grpc:grpc_client_integration_lib", "//test/integration/filters:address_restore_listener_filter_lib", - "//source/extensions/filters/listener/original_dst:config", "//test/test_common:resources_lib", "//test/test_common:utility_lib", "@envoy_api//envoy/config/bootstrap/v3:pkg_cc_proto", "@envoy_api//envoy/config/core/v3:pkg_cc_proto", "@envoy_api//envoy/config/route/v3:pkg_cc_proto", "@envoy_api//envoy/extensions/filters/network/http_connection_manager/v3:pkg_cc_proto", + "@envoy_api//envoy/extensions/filters/network/tcp_proxy/v3:pkg_cc_proto", "@envoy_api//envoy/service/discovery/v3:pkg_cc_proto", ], ) diff --git a/test/integration/filters/address_restore_listener_filter.cc b/test/integration/filters/address_restore_listener_filter.cc index b9cc29b12af3..c769994b9644 100644 --- a/test/integration/filters/address_restore_listener_filter.cc +++ b/test/integration/filters/address_restore_listener_filter.cc @@ -1,37 +1,31 @@ #include "envoy/network/filter.h" +#include "envoy/network/listen_socket.h" +#include "envoy/server/filter_config.h" #include "common/network/address_impl.h" -#include "envoy/network/listen_socket.h" #include "common/network/utility.h" -#include "envoy/server/filter_config.h" namespace Envoy { -class FakeListenerFilter : public Network::ListenerFilter { +// The FakeOriginalDstListenerFilter restore desired local address without the dependency of OS. +class FakeOriginalDstListenerFilter : public Network::ListenerFilter { public: // Network::ListenerFilter Network::FilterStatus onAccept(Network::ListenerFilterCallbacks& cb) override { - FANCY_LOG(info, "calling FakeListenerFilter::onAccept"); + FANCY_LOG(debug, "in FakeOriginalDstListenerFilter::onAccept"); Network::ConnectionSocket& socket = cb.socket(); - FANCY_LOG(info, "lambdai: current local socket address is {} restored = {}", - socket.addressProvider().localAddress()->asString(), - socket.addressProvider().localAddressRestored()); - FANCY_LOG(info, "lambdai: current remote socket address is {}", - socket.addressProvider().remoteAddress()->asString()); socket.addressProvider().restoreLocalAddress( std::make_shared("127.0.0.2", 80)); - FANCY_LOG(info, "lambdai: current local socket address is {} restored = {}", + FANCY_LOG(debug, "current local socket address is {} restored = {}", socket.addressProvider().localAddress()->asString(), socket.addressProvider().localAddressRestored()); - FANCY_LOG(info, "lambdai: current remote socket address is {}", - socket.addressProvider().remoteAddress()->asString()); return Network::FilterStatus::Continue; } }; -class FakeListenerFilterConfigFactory +class FakeOriginalDstListenerFilterConfigFactory : public Server::Configuration::NamedListenerFilterConfigFactory { public: // NamedListenerFilterConfigFactory @@ -41,7 +35,7 @@ class FakeListenerFilterConfigFactory Server::Configuration::ListenerFactoryContext&) override { return [listener_filter_matcher](Network::ListenerFilterManager& filter_manager) -> void { filter_manager.addAcceptFilter(listener_filter_matcher, - std::make_unique()); + std::make_unique()); }; } @@ -49,14 +43,13 @@ class FakeListenerFilterConfigFactory return ProtobufTypes::MessagePtr{new Envoy::ProtobufWkt::Struct()}; } - std::string name() const override { return "envoy.listener.test_address_restore"; } + std::string name() const override { + // This fake original_dest should be used only in integration test! + return "envoy.filters.listener.original_dst"; + } }; -static Registry::RegisterFactory register_; - -// REGISTER_FACTORY(FakeListenerFilterConfigFactory, -// Server::Configuration::NamedListenerFilterConfigFactory){ -// "envoy.listener.test_address_restore"}; -} // namespace Envoy \ No newline at end of file +} // namespace Envoy diff --git a/test/integration/listener_lds_integration_test.cc b/test/integration/listener_lds_integration_test.cc index 4495efccb408..f26a11f0ff81 100644 --- a/test/integration/listener_lds_integration_test.cc +++ b/test/integration/listener_lds_integration_test.cc @@ -4,6 +4,8 @@ #include "envoy/config/route/v3/route.pb.h" #include "envoy/config/route/v3/scoped_route.pb.h" #include "envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.pb.h" +#include "envoy/extensions/filters/network/tcp_proxy/v3/tcp_proxy.pb.h" +#include "envoy/network/connection.h" #include "envoy/service/discovery/v3/discovery.pb.h" #include "common/config/api_version.h" @@ -17,7 +19,6 @@ #include "absl/strings/str_cat.h" #include "gmock/gmock.h" #include "gtest/gtest.h" -#include namespace Envoy { namespace { @@ -410,96 +411,103 @@ TEST_P(ListenerIntegrationTest, MultipleLdsUpdatesSharingListenSocketFactory) { } } -// Tests that a LDS adding listener works as expected. -TEST_P(ListenerIntegrationTest, RedirectConnectionIsBalancedOnDestinationListener) { - concurrency_ = 2; +class RebalancerTest : public testing::TestWithParam, + public BaseIntegrationTest { +public: + RebalancerTest() + : BaseIntegrationTest(GetParam(), ConfigHelper::baseConfig() + R"EOF( + filter_chains: + - filters: + - name: envoy.filters.network.tcp_proxy + typed_config: + "@type": type.googleapis.com/envoy.extensions.filters.network.tcp_proxy.v3.TcpProxy + stat_prefix: tcp_stats + cluster: cluster_0 +)EOF") {} - // filter->mutable_typed_config(); - on_server_init_function_ = [&]() { - createLdsStream(); + void initialize() override { + config_helper_.renameListener("tcp"); + config_helper_.addConfigModifier( + [&](envoy::config::bootstrap::v3::Bootstrap& bootstrap) -> void { + auto& src_listener_config = *bootstrap.mutable_static_resources()->mutable_listeners(0); + src_listener_config.mutable_use_original_dst()->set_value(true); + // Note that the below original_dst is replaced by FakeOriginalDstListenerFilter at the + // link time. + src_listener_config.add_listener_filters()->set_name( + "envoy.filters.listener.original_dst"); + auto& dst_listener_config = *bootstrap.mutable_static_resources()->add_listeners(); + dst_listener_config = src_listener_config; + dst_listener_config.mutable_use_original_dst()->set_value(false); + dst_listener_config.clear_listener_filters(); + dst_listener_config.mutable_bind_to_port()->set_value(false); + dst_listener_config.set_name("balanced_target_listener"); + dst_listener_config.mutable_connection_balance_config()->mutable_exact_balance(); + + // 127.0.0.2 is defined in FakeOriginalDstListenerFilter. + *dst_listener_config.mutable_address()->mutable_socket_address()->mutable_address() = + "127.0.0.2"; + dst_listener_config.mutable_address()->mutable_socket_address()->set_port_value(80); + }); + BaseIntegrationTest::initialize(); + } - auto src_listener_config = listener_config_; - src_listener_config.mutable_use_original_dst()->set_value(true); - src_listener_config.add_listener_filters()->set_name("envoy.listener.test_address_restore"); - //src_listener_config.clear_filter_chains(); - auto dst_listener_config = src_listener_config; - dst_listener_config.mutable_use_original_dst()->set_value(false); - dst_listener_config.clear_listener_filters(); - dst_listener_config.mutable_bind_to_port()->set_value(false); - dst_listener_config.set_name("balanced_target_listener"); - *dst_listener_config.mutable_address()->mutable_socket_address()->mutable_address() = - "127.0.0.2"; // defined in test_address_restore listener filter. - dst_listener_config.mutable_address()->mutable_socket_address()->set_port_value(80); - - sendLdsResponse({src_listener_config, dst_listener_config}, "1"); - createRdsStream(route_table_name_); - }; - initialize(); - FANCY_LOG(info, "lambdai: {} ", __LINE__); - // test_server_->waitForCounterGe("listener_manager.lds.update_success", 1); - FANCY_LOG(info, "lambdai: {} ", __LINE__); - // testing-listener-0 is not initialized as we haven't pushed any RDS yet. - EXPECT_EQ(test_server_->server().initManager().state(), Init::Manager::State::Initializing); + std::unique_ptr createConnectionAndWrite(const std::string& request, + std::string& response) { + Buffer::OwnedImpl buffer(request); + return std::make_unique( + lookupPort("tcp"), buffer, + [&response](Network::ClientConnection&, const Buffer::Instance& data) -> void { + response.append(data.toString()); + }, + version_, *dispatcher_); + } +}; - FANCY_LOG(info, "lambdai: {} ", __LINE__); +struct PerConnection { + std::string response_; + std::unique_ptr client_conn_; + FakeRawConnectionPtr upstream_conn_; +}; - // Workers not started, the LDS added listener 0 is in active_listeners_ list. - EXPECT_EQ(test_server_->server().listenerManager().listeners().size(), 2); - registerTestServerPorts({listener_name_}); +// Verify the connections are distributed evenly on the 2 worker threads of the redirected +// listener. +TEST_P(RebalancerTest, RedirectConnectionIsBalancedOnDestinationListener) { + concurrency_ = 2; + int repeats = 10; + initialize(); - const std::string route_config_tmpl = R"EOF( - name: {} - virtual_hosts: - - name: integration - domains: ["*"] - routes: - - match: {{ prefix: "/" }} - route: {{ cluster: {} }} -)EOF"; - sendRdsResponse(fmt::format(route_config_tmpl, route_table_name_, "cluster_0"), "1"); - FANCY_LOG(info, "lambdai: {} ", __LINE__); - test_server_->waitForCounterGe( - fmt::format("http.config_test.rds.{}.update_success", route_table_name_), 1); - FANCY_LOG(info, "lambdai: {} ", __LINE__); - // Now testing-listener-0 finishes initialization, Server initManager will be ready. - EXPECT_EQ(test_server_->server().initManager().state(), Init::Manager::State::Initialized); - FANCY_LOG(info, "lambdai: {} ", __LINE__); - test_server_->waitUntilListenersReady(); - FANCY_LOG(info, "lambdai: {} ", __LINE__); - // NOTE: The line above doesn't tell you if listener is up and listening. - test_server_->waitForCounterGe("listener_manager.listener_create_success", 1); - // Request is sent to cluster_0. + // The balancer is balanced as per active connection instead of total connection. + // The below vector maintains all the connections alive. + std::vector connections; + for (uint32_t i = 0; i < repeats * concurrency_; ++i) { + connections.emplace_back(); + connections.back().client_conn_ = + createConnectionAndWrite("dummy", connections.back().response_); + connections.back().client_conn_->waitForConnection(); + ASSERT_TRUE(fake_upstreams_[0]->waitForRawConnection(connections.back().upstream_conn_)); + } + for (auto& conn : connections) { + conn.client_conn_->close(); + while (!conn.client_conn_->closed()) { + dispatcher_->run(Event::Dispatcher::RunType::NonBlock); + } + } - codec_client_ = makeHttpConnection(lookupPort(listener_name_)); - int response_size = 800; - int request_size = 10; - Http::TestResponseHeaderMapImpl response_headers{{":status", "200"}, - {"server_id", "cluster_0, backend_0"}}; - auto response = sendRequestAndWaitForResponse( - Http::TestResponseHeaderMapImpl{ - {":method", "GET"}, {":path", "/"}, {":authority", "host"}, {":scheme", "http"}}, - request_size, response_headers, response_size, /*cluster_0*/ 0); - verifyResponse(std::move(response), "200", response_headers, std::string(response_size, 'a')); - test_server_->waitForCounterGe("listener_manager.listener_create_success", 2); - std::this_thread::sleep_for(std::chrono::seconds(10)); - - FANCY_LOG(info, "lambdai cx {}", Network::Test::getLoopbackAddressString(ipVersion())); - FANCY_LOG( - info, "worker 0 cx {}", - TestUtility::findCounter(test_server_->statStore(), - fmt::format("listener.{}_0.worker_0.downstream_cx_total", - Network::Test::getLoopbackAddressString(ipVersion()))) - ->value()); - FANCY_LOG( - info, "worker 1 cx {}", - TestUtility::findCounter(test_server_->statStore(), - fmt::format("listener.{}_0.worker_1.downstream_cx_total", - Network::Test::getLoopbackAddressString(ipVersion()))) - ->value()); + ASSERT_EQ(TestUtility::findCounter(test_server_->statStore(), + "listener.127.0.0.2_80.worker_0.downstream_cx_total") - EXPECT_TRUE(upstream_request_->complete()); - EXPECT_EQ(request_size, upstream_request_->bodyLength()); + ->value(), + repeats); + ASSERT_EQ(TestUtility::findCounter(test_server_->statStore(), + "listener.127.0.0.2_80.worker_1.downstream_cx_total") + + ->value(), + repeats); } +INSTANTIATE_TEST_SUITE_P(IpVersions, RebalancerTest, + testing::ValuesIn(TestEnvironment::getIpVersionsForTest()), + TestUtility::ipTestParamsToString); + } // namespace } // namespace Envoy From 3b25b2bde99eabb202fe1d0b8048bc8348503ec2 Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Fri, 23 Apr 2021 20:59:55 -0700 Subject: [PATCH 14/21] add version history Signed-off-by: Yuchen Dai --- docs/root/version_history/current.rst | 3 +++ 1 file changed, 3 insertions(+) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 0fde709aaffe..60fbac4a6d95 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -14,6 +14,9 @@ Minor Behavior Changes (require upstream 1xx or 204 responses to not have Transfer-Encoding or non-zero Content-Length headers) and `envoy.reloadable_features.send_strict_1xx_and_204_response_headers` (do not send 1xx or 204 responses with these headers). Both are true by default. +* listener: Respect the :ref:`ConnectionBalanceConfig ` + defined within the listener where the sockets are redirected to. Clear that field to restore the previous behavior. + Bug Fixes --------- From cafb8c53a6965ac74c7852c3922d59866db4efd6 Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Sat, 24 Apr 2021 00:53:41 -0700 Subject: [PATCH 15/21] more doc Signed-off-by: Yuchen Dai --- api/envoy/config/listener/v3/listener.proto | 4 ++++ docs/root/version_history/current.rst | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/api/envoy/config/listener/v3/listener.proto b/api/envoy/config/listener/v3/listener.proto index 4e0a857ce256..7d8e7e62b0c3 100644 --- a/api/envoy/config/listener/v3/listener.proto +++ b/api/envoy/config/listener/v3/listener.proto @@ -247,6 +247,10 @@ message Listener { // The listener's connection balancer configuration, currently only applicable to TCP listeners. // If no configuration is specified, Envoy will not attempt to balance active connections between // worker threads. + // + // Consider the scenario that the listener X set :ref: `original_dst `, + // and the listeners Y1,Y2 set `bind_to_port ` to + // false. The balance config should be cleared in listener X and be enabled in Y1 and Y2. ConnectionBalanceConfig connection_balance_config = 20; // When this flag is set to true, listeners set the *SO_REUSEPORT* socket option and diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 60fbac4a6d95..7247a600a0b1 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -14,7 +14,7 @@ Minor Behavior Changes (require upstream 1xx or 204 responses to not have Transfer-Encoding or non-zero Content-Length headers) and `envoy.reloadable_features.send_strict_1xx_and_204_response_headers` (do not send 1xx or 204 responses with these headers). Both are true by default. -* listener: Respect the :ref:`ConnectionBalanceConfig ` +* listener: Respect the :ref:`connection balance config ` defined within the listener where the sockets are redirected to. Clear that field to restore the previous behavior. From 46e87321870730828d9db2d543221f60879e92ca Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Sat, 24 Apr 2021 01:14:02 -0700 Subject: [PATCH 16/21] doc Signed-off-by: Yuchen Dai --- 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 7247a600a0b1..bcab0a769dae 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -14,7 +14,7 @@ Minor Behavior Changes (require upstream 1xx or 204 responses to not have Transfer-Encoding or non-zero Content-Length headers) and `envoy.reloadable_features.send_strict_1xx_and_204_response_headers` (do not send 1xx or 204 responses with these headers). Both are true by default. -* listener: Respect the :ref:`connection balance config ` +* listener: respect the :ref:`connection balance config ` defined within the listener where the sockets are redirected to. Clear that field to restore the previous behavior. From 381423a2ca0813ead299e30f218ff2fe26f5da25 Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Sat, 24 Apr 2021 10:44:36 -0700 Subject: [PATCH 17/21] sync api v4 Signed-off-by: Yuchen Dai --- api/envoy/config/listener/v4alpha/listener.proto | 4 ++++ generated_api_shadow/envoy/config/listener/v3/listener.proto | 4 ++++ .../envoy/config/listener/v4alpha/listener.proto | 4 ++++ 3 files changed, 12 insertions(+) diff --git a/api/envoy/config/listener/v4alpha/listener.proto b/api/envoy/config/listener/v4alpha/listener.proto index e2eb3a1e6065..af09794c5beb 100644 --- a/api/envoy/config/listener/v4alpha/listener.proto +++ b/api/envoy/config/listener/v4alpha/listener.proto @@ -249,6 +249,10 @@ message Listener { // The listener's connection balancer configuration, currently only applicable to TCP listeners. // If no configuration is specified, Envoy will not attempt to balance active connections between // worker threads. + // + // Consider the scenario that the listener X set :ref: `original_dst `, + // and the listeners Y1,Y2 set `bind_to_port ` to + // false. The balance config should be cleared in listener X and be enabled in Y1 and Y2. ConnectionBalanceConfig connection_balance_config = 20; // When this flag is set to true, listeners set the *SO_REUSEPORT* socket option and diff --git a/generated_api_shadow/envoy/config/listener/v3/listener.proto b/generated_api_shadow/envoy/config/listener/v3/listener.proto index 4e0a857ce256..7d8e7e62b0c3 100644 --- a/generated_api_shadow/envoy/config/listener/v3/listener.proto +++ b/generated_api_shadow/envoy/config/listener/v3/listener.proto @@ -247,6 +247,10 @@ message Listener { // The listener's connection balancer configuration, currently only applicable to TCP listeners. // If no configuration is specified, Envoy will not attempt to balance active connections between // worker threads. + // + // Consider the scenario that the listener X set :ref: `original_dst `, + // and the listeners Y1,Y2 set `bind_to_port ` to + // false. The balance config should be cleared in listener X and be enabled in Y1 and Y2. ConnectionBalanceConfig connection_balance_config = 20; // When this flag is set to true, listeners set the *SO_REUSEPORT* socket option and diff --git a/generated_api_shadow/envoy/config/listener/v4alpha/listener.proto b/generated_api_shadow/envoy/config/listener/v4alpha/listener.proto index cad42c77a1ff..4f061e9a78b6 100644 --- a/generated_api_shadow/envoy/config/listener/v4alpha/listener.proto +++ b/generated_api_shadow/envoy/config/listener/v4alpha/listener.proto @@ -252,6 +252,10 @@ message Listener { // The listener's connection balancer configuration, currently only applicable to TCP listeners. // If no configuration is specified, Envoy will not attempt to balance active connections between // worker threads. + // + // Consider the scenario that the listener X set :ref: `original_dst `, + // and the listeners Y1,Y2 set `bind_to_port ` to + // false. The balance config should be cleared in listener X and be enabled in Y1 and Y2. ConnectionBalanceConfig connection_balance_config = 20; // When this flag is set to true, listeners set the *SO_REUSEPORT* socket option and From 799d2e18d4c1ae7c27204d8bfb251049ad2b1e1a Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Sun, 25 Apr 2021 18:48:19 -0700 Subject: [PATCH 18/21] cleared -> disabled Signed-off-by: Yuchen Dai --- api/envoy/config/listener/v3/listener.proto | 2 +- api/envoy/config/listener/v4alpha/listener.proto | 2 +- generated_api_shadow/envoy/config/listener/v3/listener.proto | 2 +- .../envoy/config/listener/v4alpha/listener.proto | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/api/envoy/config/listener/v3/listener.proto b/api/envoy/config/listener/v3/listener.proto index 7d8e7e62b0c3..f0a6992b6c0d 100644 --- a/api/envoy/config/listener/v3/listener.proto +++ b/api/envoy/config/listener/v3/listener.proto @@ -250,7 +250,7 @@ message Listener { // // Consider the scenario that the listener X set :ref: `original_dst `, // and the listeners Y1,Y2 set `bind_to_port ` to - // false. The balance config should be cleared in listener X and be enabled in Y1 and Y2. + // false. The balance config should be disabled in listener X and be enabled in Y1 and Y2. ConnectionBalanceConfig connection_balance_config = 20; // When this flag is set to true, listeners set the *SO_REUSEPORT* socket option and diff --git a/api/envoy/config/listener/v4alpha/listener.proto b/api/envoy/config/listener/v4alpha/listener.proto index af09794c5beb..fbb6ba9ec4ee 100644 --- a/api/envoy/config/listener/v4alpha/listener.proto +++ b/api/envoy/config/listener/v4alpha/listener.proto @@ -252,7 +252,7 @@ message Listener { // // Consider the scenario that the listener X set :ref: `original_dst `, // and the listeners Y1,Y2 set `bind_to_port ` to - // false. The balance config should be cleared in listener X and be enabled in Y1 and Y2. + // false. The balance config should be disabled in listener X and be enabled in Y1 and Y2. ConnectionBalanceConfig connection_balance_config = 20; // When this flag is set to true, listeners set the *SO_REUSEPORT* socket option and diff --git a/generated_api_shadow/envoy/config/listener/v3/listener.proto b/generated_api_shadow/envoy/config/listener/v3/listener.proto index 7d8e7e62b0c3..f0a6992b6c0d 100644 --- a/generated_api_shadow/envoy/config/listener/v3/listener.proto +++ b/generated_api_shadow/envoy/config/listener/v3/listener.proto @@ -250,7 +250,7 @@ message Listener { // // Consider the scenario that the listener X set :ref: `original_dst `, // and the listeners Y1,Y2 set `bind_to_port ` to - // false. The balance config should be cleared in listener X and be enabled in Y1 and Y2. + // false. The balance config should be disabled in listener X and be enabled in Y1 and Y2. ConnectionBalanceConfig connection_balance_config = 20; // When this flag is set to true, listeners set the *SO_REUSEPORT* socket option and diff --git a/generated_api_shadow/envoy/config/listener/v4alpha/listener.proto b/generated_api_shadow/envoy/config/listener/v4alpha/listener.proto index 4f061e9a78b6..16219b43144e 100644 --- a/generated_api_shadow/envoy/config/listener/v4alpha/listener.proto +++ b/generated_api_shadow/envoy/config/listener/v4alpha/listener.proto @@ -255,7 +255,7 @@ message Listener { // // Consider the scenario that the listener X set :ref: `original_dst `, // and the listeners Y1,Y2 set `bind_to_port ` to - // false. The balance config should be cleared in listener X and be enabled in Y1 and Y2. + // false. The balance config should be disabled in listener X and be enabled in Y1 and Y2. ConnectionBalanceConfig connection_balance_config = 20; // When this flag is set to true, listeners set the *SO_REUSEPORT* socket option and From be4033a96402a163628ed2a73b9b5f02094eb103 Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Tue, 27 Apr 2021 11:29:41 -0700 Subject: [PATCH 19/21] update proto inline comment Signed-off-by: Yuchen Dai --- api/envoy/config/listener/v3/listener.proto | 6 +++--- api/envoy/config/listener/v4alpha/listener.proto | 6 +++--- .../envoy/config/listener/v3/listener.proto | 6 +++--- .../envoy/config/listener/v4alpha/listener.proto | 6 +++--- 4 files changed, 12 insertions(+), 12 deletions(-) diff --git a/api/envoy/config/listener/v3/listener.proto b/api/envoy/config/listener/v3/listener.proto index f0a6992b6c0d..5f3629156b8b 100644 --- a/api/envoy/config/listener/v3/listener.proto +++ b/api/envoy/config/listener/v3/listener.proto @@ -248,9 +248,9 @@ message Listener { // If no configuration is specified, Envoy will not attempt to balance active connections between // worker threads. // - // Consider the scenario that the listener X set :ref: `original_dst `, - // and the listeners Y1,Y2 set `bind_to_port ` to - // false. The balance config should be disabled in listener X and be enabled in Y1 and Y2. + // In the scenario that the listener X redirects all the connections to the listeners Y1 and Y2, + // you may want to disable the balance config in listener X to avoid the cost of balancing, and + // enable the balance config in Y1 and Y2 to balance the connections among the workers. ConnectionBalanceConfig connection_balance_config = 20; // When this flag is set to true, listeners set the *SO_REUSEPORT* socket option and diff --git a/api/envoy/config/listener/v4alpha/listener.proto b/api/envoy/config/listener/v4alpha/listener.proto index fbb6ba9ec4ee..39aeb4e5902f 100644 --- a/api/envoy/config/listener/v4alpha/listener.proto +++ b/api/envoy/config/listener/v4alpha/listener.proto @@ -250,9 +250,9 @@ message Listener { // If no configuration is specified, Envoy will not attempt to balance active connections between // worker threads. // - // Consider the scenario that the listener X set :ref: `original_dst `, - // and the listeners Y1,Y2 set `bind_to_port ` to - // false. The balance config should be disabled in listener X and be enabled in Y1 and Y2. + // In the scenario that the listener X redirects all the connections to the listeners Y1 and Y2, + // you may want to disable the balance config in listener X to avoid the cost of balancing, and + // enable the balance config in Y1 and Y2 to balance the connections among the workers. ConnectionBalanceConfig connection_balance_config = 20; // When this flag is set to true, listeners set the *SO_REUSEPORT* socket option and diff --git a/generated_api_shadow/envoy/config/listener/v3/listener.proto b/generated_api_shadow/envoy/config/listener/v3/listener.proto index f0a6992b6c0d..5f3629156b8b 100644 --- a/generated_api_shadow/envoy/config/listener/v3/listener.proto +++ b/generated_api_shadow/envoy/config/listener/v3/listener.proto @@ -248,9 +248,9 @@ message Listener { // If no configuration is specified, Envoy will not attempt to balance active connections between // worker threads. // - // Consider the scenario that the listener X set :ref: `original_dst `, - // and the listeners Y1,Y2 set `bind_to_port ` to - // false. The balance config should be disabled in listener X and be enabled in Y1 and Y2. + // In the scenario that the listener X redirects all the connections to the listeners Y1 and Y2, + // you may want to disable the balance config in listener X to avoid the cost of balancing, and + // enable the balance config in Y1 and Y2 to balance the connections among the workers. ConnectionBalanceConfig connection_balance_config = 20; // When this flag is set to true, listeners set the *SO_REUSEPORT* socket option and diff --git a/generated_api_shadow/envoy/config/listener/v4alpha/listener.proto b/generated_api_shadow/envoy/config/listener/v4alpha/listener.proto index 16219b43144e..faf93a484331 100644 --- a/generated_api_shadow/envoy/config/listener/v4alpha/listener.proto +++ b/generated_api_shadow/envoy/config/listener/v4alpha/listener.proto @@ -253,9 +253,9 @@ message Listener { // If no configuration is specified, Envoy will not attempt to balance active connections between // worker threads. // - // Consider the scenario that the listener X set :ref: `original_dst `, - // and the listeners Y1,Y2 set `bind_to_port ` to - // false. The balance config should be disabled in listener X and be enabled in Y1 and Y2. + // In the scenario that the listener X redirects all the connections to the listeners Y1 and Y2, + // you may want to disable the balance config in listener X to avoid the cost of balancing, and + // enable the balance config in Y1 and Y2 to balance the connections among the workers. ConnectionBalanceConfig connection_balance_config = 20; // When this flag is set to true, listeners set the *SO_REUSEPORT* socket option and From 726ba101fec58c5c3d8aaec39351a52c8923e98f Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Tue, 27 Apr 2021 16:32:46 -0700 Subject: [PATCH 20/21] add ref Signed-off-by: Yuchen Dai --- api/envoy/config/listener/v3/listener.proto | 6 ++++-- api/envoy/config/listener/v4alpha/listener.proto | 6 ++++-- .../envoy/config/listener/v3/listener.proto | 6 ++++-- .../envoy/config/listener/v4alpha/listener.proto | 6 ++++-- 4 files changed, 16 insertions(+), 8 deletions(-) diff --git a/api/envoy/config/listener/v3/listener.proto b/api/envoy/config/listener/v3/listener.proto index 5f3629156b8b..5461318ada01 100644 --- a/api/envoy/config/listener/v3/listener.proto +++ b/api/envoy/config/listener/v3/listener.proto @@ -248,8 +248,10 @@ message Listener { // If no configuration is specified, Envoy will not attempt to balance active connections between // worker threads. // - // In the scenario that the listener X redirects all the connections to the listeners Y1 and Y2, - // you may want to disable the balance config in listener X to avoid the cost of balancing, and + // In the scenario that the listener X redirects all the connections to the listeners Y1 and Y2 + // by setting :ref:`use_original_dst ` in X + // and :ref:`bind_to_port ` to false in Y1 and Y2, + // it is recommended to disable the balance config in listener X to avoid the cost of balancing, and // enable the balance config in Y1 and Y2 to balance the connections among the workers. ConnectionBalanceConfig connection_balance_config = 20; diff --git a/api/envoy/config/listener/v4alpha/listener.proto b/api/envoy/config/listener/v4alpha/listener.proto index 39aeb4e5902f..e40dbf9058af 100644 --- a/api/envoy/config/listener/v4alpha/listener.proto +++ b/api/envoy/config/listener/v4alpha/listener.proto @@ -250,8 +250,10 @@ message Listener { // If no configuration is specified, Envoy will not attempt to balance active connections between // worker threads. // - // In the scenario that the listener X redirects all the connections to the listeners Y1 and Y2, - // you may want to disable the balance config in listener X to avoid the cost of balancing, and + // In the scenario that the listener X redirects all the connections to the listeners Y1 and Y2 + // by setting :ref:`use_original_dst ` in X + // and :ref:`bind_to_port ` to false in Y1 and Y2, + // it is recommended to disable the balance config in listener X to avoid the cost of balancing, and // enable the balance config in Y1 and Y2 to balance the connections among the workers. ConnectionBalanceConfig connection_balance_config = 20; diff --git a/generated_api_shadow/envoy/config/listener/v3/listener.proto b/generated_api_shadow/envoy/config/listener/v3/listener.proto index 5f3629156b8b..5461318ada01 100644 --- a/generated_api_shadow/envoy/config/listener/v3/listener.proto +++ b/generated_api_shadow/envoy/config/listener/v3/listener.proto @@ -248,8 +248,10 @@ message Listener { // If no configuration is specified, Envoy will not attempt to balance active connections between // worker threads. // - // In the scenario that the listener X redirects all the connections to the listeners Y1 and Y2, - // you may want to disable the balance config in listener X to avoid the cost of balancing, and + // In the scenario that the listener X redirects all the connections to the listeners Y1 and Y2 + // by setting :ref:`use_original_dst ` in X + // and :ref:`bind_to_port ` to false in Y1 and Y2, + // it is recommended to disable the balance config in listener X to avoid the cost of balancing, and // enable the balance config in Y1 and Y2 to balance the connections among the workers. ConnectionBalanceConfig connection_balance_config = 20; diff --git a/generated_api_shadow/envoy/config/listener/v4alpha/listener.proto b/generated_api_shadow/envoy/config/listener/v4alpha/listener.proto index faf93a484331..47611a615efb 100644 --- a/generated_api_shadow/envoy/config/listener/v4alpha/listener.proto +++ b/generated_api_shadow/envoy/config/listener/v4alpha/listener.proto @@ -253,8 +253,10 @@ message Listener { // If no configuration is specified, Envoy will not attempt to balance active connections between // worker threads. // - // In the scenario that the listener X redirects all the connections to the listeners Y1 and Y2, - // you may want to disable the balance config in listener X to avoid the cost of balancing, and + // In the scenario that the listener X redirects all the connections to the listeners Y1 and Y2 + // by setting :ref:`use_original_dst ` in X + // and :ref:`bind_to_port ` to false in Y1 and Y2, + // it is recommended to disable the balance config in listener X to avoid the cost of balancing, and // enable the balance config in Y1 and Y2 to balance the connections among the workers. ConnectionBalanceConfig connection_balance_config = 20; From eeb6413492b12a582363f1a3c4f92db5be9f0f6a Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Tue, 27 Apr 2021 20:41:47 -0700 Subject: [PATCH 21/21] add comment and rename to virtual_listener Signed-off-by: Yuchen Dai --- .../listener_lds_integration_test.cc | 23 ++++++++++--------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/test/integration/listener_lds_integration_test.cc b/test/integration/listener_lds_integration_test.cc index f26a11f0ff81..2179d9b3c3e5 100644 --- a/test/integration/listener_lds_integration_test.cc +++ b/test/integration/listener_lds_integration_test.cc @@ -435,18 +435,19 @@ class RebalancerTest : public testing::TestWithParamset_name( "envoy.filters.listener.original_dst"); - auto& dst_listener_config = *bootstrap.mutable_static_resources()->add_listeners(); - dst_listener_config = src_listener_config; - dst_listener_config.mutable_use_original_dst()->set_value(false); - dst_listener_config.clear_listener_filters(); - dst_listener_config.mutable_bind_to_port()->set_value(false); - dst_listener_config.set_name("balanced_target_listener"); - dst_listener_config.mutable_connection_balance_config()->mutable_exact_balance(); - - // 127.0.0.2 is defined in FakeOriginalDstListenerFilter. - *dst_listener_config.mutable_address()->mutable_socket_address()->mutable_address() = + auto& virtual_listener_config = *bootstrap.mutable_static_resources()->add_listeners(); + virtual_listener_config = src_listener_config; + virtual_listener_config.mutable_use_original_dst()->set_value(false); + virtual_listener_config.clear_listener_filters(); + virtual_listener_config.mutable_bind_to_port()->set_value(false); + virtual_listener_config.set_name("balanced_target_listener"); + virtual_listener_config.mutable_connection_balance_config()->mutable_exact_balance(); + + // 127.0.0.2 is defined in FakeOriginalDstListenerFilter. This virtual listener does not + // listen on a passive socket so it's safe to use any ip address. + *virtual_listener_config.mutable_address()->mutable_socket_address()->mutable_address() = "127.0.0.2"; - dst_listener_config.mutable_address()->mutable_socket_address()->set_port_value(80); + virtual_listener_config.mutable_address()->mutable_socket_address()->set_port_value(80); }); BaseIntegrationTest::initialize(); }