listener: fix inplace update with conn balancer

docs/root/version_history/current.rst

@@ -21,8 +21,9 @@ Minor Behavior Changes
 * http: fixed the 100-continue response path to properly handle upstream failure by sending 5xx responses. This behavior can be temporarily reverted by setting `envoy.reloadable_features.allow_500_after_100` to false.
 * http: the per-stream FilterState maintained by the HTTP connection manager will now provide read/write access to the downstream connection FilterState. As such, code that relies on interacting with this might
   see a change in behavior.
-* logging: add fine-grain logging for file level log control with logger management at administration interface. It can be enabled by option `--enable-fine-grain-logging`.
-* logging: change default log format to `"[%Y-%m-%d %T.%e][%t][%l][%n] [%g:%#] %v"` and default value of :option:`--log-format-prefix-with-location` to `0`.
+* listener: fixed crash at listener inplace update when connetion load balancer is set.
+* logging: added fine-grain logging for file level log control with logger management at administration interface. It can be enabled by option `--enable-fine-grain-logging`.
+* logging: changed default log format to `"[%Y-%m-%d %T.%e][%t][%l][%n] [%g:%#] %v"` and default value of :option:`--log-format-prefix-with-location` to `0`.
 * logging: nghttp2 log messages no longer appear at trace level unless `ENVOY_NGHTTP2_TRACE` is set
   in the environment.
 * router: added transport failure reason to response body when upstream reset happens. After this change, the response body will be of the form `upstream connect error or disconnect/reset before headers. reset reason:{}, transport failure reason:{}`.This behavior may be reverted by setting runtime feature `envoy.reloadable_features.http_transport_failure_reason_in_body` to false.

include/envoy/network/connection_balancer.h

@@ -65,7 +65,7 @@ class ConnectionBalancer {
   pickTargetHandler(BalancedConnectionHandler& current_handler) PURE;
 };
 
-using ConnectionBalancerPtr = std::unique_ptr<ConnectionBalancer>;
+using ConnectionBalancerSharedPtr = std::shared_ptr<ConnectionBalancer>;
 
 } // namespace Network
 } // namespace Envoy

source/server/connection_handler_impl.cc

@@ -156,6 +156,7 @@ ConnectionHandlerImpl::ActiveTcpListener::ActiveTcpListener(ConnectionHandlerImp
 void ConnectionHandlerImpl::ActiveTcpListener::updateListenerConfig(
     Network::ListenerConfig& config) {
   ENVOY_LOG(trace, "replacing listener ", config_->listenerTag(), " by ", config.listenerTag());
+  ASSERT(&config_->connectionBalancer() == &config.connectionBalancer());
   config_ = &config;
 }
 

source/server/listener_impl.cc

@@ -325,6 +325,7 @@ ListenerImpl::ListenerImpl(ListenerImpl& origin,
       listener_filters_timeout_(
           PROTOBUF_GET_MS_OR_DEFAULT(config, listener_filters_timeout, 15000)),
       continue_on_listener_filters_timeout_(config.continue_on_listener_filters_timeout()),
+      connection_balancer_(origin.connection_balancer_),
       listener_factory_context_(std::make_shared<PerListenerFactoryContextImpl>(
           origin.listener_factory_context_->listener_factory_context_base_, this, *this)),
       filter_chain_manager_(address_, origin.listener_factory_context_->parentFactoryContext(),
@@ -488,12 +489,15 @@ void ListenerImpl::buildFilterChains() {
 
 void ListenerImpl::buildSocketOptions() {
   // TCP specific setup.
-  if (config_.has_connection_balance_config()) {
-    // Currently exact balance is the only supported type and there are no options.
-    ASSERT(config_.connection_balance_config().has_exact_balance());
-    connection_balancer_ = std::make_unique<Network::ExactConnectionBalancerImpl>();
-  } else {
-    connection_balancer_ = std::make_unique<Network::NopConnectionBalancerImpl>();
+  if (connection_balancer_ == nullptr) {
+    // Not in place listener update.
+    if (config_.has_connection_balance_config()) {
+      // Currently exact balance is the only supported type and there are no options.
+      ASSERT(config_.connection_balance_config().has_exact_balance());
+      connection_balancer_ = std::make_shared<Network::ExactConnectionBalancerImpl>();
+    } else {
+      connection_balancer_ = std::make_shared<Network::NopConnectionBalancerImpl>();
+    }
   }
 
   if (config_.has_tcp_fast_open_queue_length()) {

source/server/listener_impl.h

@@ -393,7 +393,7 @@ class ListenerImpl final : public Network::ListenerConfig,
   const bool continue_on_listener_filters_timeout_;
   Network::ActiveUdpListenerFactoryPtr udp_listener_factory_;
   Network::UdpPacketWriterFactoryPtr udp_writer_factory_;
-  Network::ConnectionBalancerPtr connection_balancer_;
+  Network::ConnectionBalancerSharedPtr connection_balancer_;
   std::shared_ptr<PerListenerFactoryContextImpl> listener_factory_context_;
   FilterChainManagerImpl filter_chain_manager_;
 

test/integration/xds_integration_test.cc

@@ -266,6 +266,10 @@ class LdsInplaceUpdateHttpIntegrationTest
     config_helper_.addListenerFilter(tls_inspector_config);
     config_helper_.addSslConfig();
     config_helper_.addConfigModifier([](envoy::config::bootstrap::v3::Bootstrap& bootstrap) {
+      bootstrap.mutable_static_resources()
+          ->mutable_listeners(0)
+          ->mutable_connection_balance_config()
+          ->mutable_exact_balance();
       auto* filter_chain_0 =
           bootstrap.mutable_static_resources()->mutable_listeners(0)->mutable_filter_chains(0);
       *filter_chain_0->mutable_filter_chain_match()->mutable_application_protocols()->Add() =

test/server/connection_handler_test.cc

@@ -57,14 +57,17 @@ class ConnectionHandlerTest : public testing::Test, protected Logger::Loggable<L
         bool continue_on_listener_filters_timeout,
         Network::ListenSocketFactorySharedPtr socket_factory,
         std::shared_ptr<NiceMock<Network::MockFilterChainManager>> filter_chain_manager = nullptr,
-        uint32_t tcp_backlog_size = ENVOY_TCP_BACKLOG_SIZE)
+        uint32_t tcp_backlog_size = ENVOY_TCP_BACKLOG_SIZE,
+        Network::ConnectionBalancerSharedPtr connection_balancer = nullptr)
         : parent_(parent), socket_(std::make_shared<NiceMock<Network::MockListenSocket>>()),
           socket_factory_(std::move(socket_factory)), tag_(tag), bind_to_port_(bind_to_port),
           tcp_backlog_size_(tcp_backlog_size),
           hand_off_restored_destination_connections_(hand_off_restored_destination_connections),
           name_(name), listener_filters_timeout_(listener_filters_timeout),
           continue_on_listener_filters_timeout_(continue_on_listener_filters_timeout),
-          connection_balancer_(std::make_unique<Network::NopConnectionBalancerImpl>()),
+          connection_balancer_(connection_balancer == nullptr
+                                   ? std::make_shared<Network::NopConnectionBalancerImpl>()
+                                   : connection_balancer),
           inline_filter_chain_manager_(filter_chain_manager) {
       envoy::config::listener::v3::UdpListenerConfig dummy;
       std::string listener_name("raw_udp_listener");
@@ -130,7 +133,7 @@ class ConnectionHandlerTest : public testing::Test, protected Logger::Loggable<L
     const bool continue_on_listener_filters_timeout_;
     std::unique_ptr<Network::ActiveUdpListenerFactory> udp_listener_factory_;
     std::unique_ptr<Network::UdpPacketWriterFactory> udp_writer_factory_;
-    Network::ConnectionBalancerPtr connection_balancer_;
+    Network::ConnectionBalancerSharedPtr connection_balancer_;
     BasicResourceLimitImpl open_connections_;
     const std::vector<AccessLog::InstanceSharedPtr> empty_access_logs_;
     std::shared_ptr<NiceMock<Network::MockFilterChainManager>> inline_filter_chain_manager_;
@@ -176,7 +179,7 @@ class ConnectionHandlerTest : public testing::Test, protected Logger::Loggable<L
       uint64_t tag, bool bind_to_port, bool hand_off_restored_destination_connections,
       const std::string& name, Network::Listener* listener,
       Network::ListenerCallbacks** listener_callbacks = nullptr,
-      Network::MockConnectionBalancer* connection_balancer = nullptr,
+      std::shared_ptr<Network::MockConnectionBalancer> connection_balancer = nullptr,
       Network::BalancedConnectionHandler** balanced_connection_handler = nullptr,
       Network::Socket::Type socket_type = Network::Socket::Type::Stream,
       std::chrono::milliseconds listener_filters_timeout = std::chrono::milliseconds(15000),
@@ -187,8 +190,9 @@ class ConnectionHandlerTest : public testing::Test, protected Logger::Loggable<L
     listeners_.emplace_back(std::make_unique<TestListener>(
         *this, tag, bind_to_port, hand_off_restored_destination_connections, name, socket_type,
         listener_filters_timeout, continue_on_listener_filters_timeout, socket_factory_,
-        overridden_filter_chain_manager, tcp_backlog_size));
+        overridden_filter_chain_manager, tcp_backlog_size, connection_balancer));
     EXPECT_CALL(*socket_factory_, socketType()).WillOnce(Return(socket_type));
+
     if (listener == nullptr) {
       // Expecting listener config in place update.
       // If so, dispatcher would not create new network listener.
@@ -213,12 +217,11 @@ class ConnectionHandlerTest : public testing::Test, protected Logger::Loggable<L
           }));
     }
 
-    if (connection_balancer != nullptr) {
-      listeners_.back()->connection_balancer_.reset(connection_balancer);
-      ASSERT(balanced_connection_handler != nullptr);
+    if (balanced_connection_handler != nullptr) {
       EXPECT_CALL(*connection_balancer, registerHandler(_))
           .WillOnce(SaveArgAddress(balanced_connection_handler));
     }
+
     return listeners_.back().get();
   }
 
@@ -250,7 +253,7 @@ TEST_F(ConnectionHandlerTest, RemoveListenerDuringRebalance) {
 
   Network::ListenerCallbacks* listener_callbacks;
   auto listener = new NiceMock<Network::MockListener>();
-  Network::MockConnectionBalancer* connection_balancer = new Network::MockConnectionBalancer();
+  auto connection_balancer = std::make_shared<Network::MockConnectionBalancer>();
   Network::BalancedConnectionHandler* current_handler;
   TestListener* test_listener =
       addListener(1, true, false, "test_listener", listener, &listener_callbacks,
@@ -962,9 +965,14 @@ TEST_F(ConnectionHandlerTest, TcpListenerInplaceUpdate) {
   uint64_t old_listener_tag = 1;
   uint64_t new_listener_tag = 2;
   Network::ListenerCallbacks* old_listener_callbacks;
+  Network::BalancedConnectionHandler* current_handler;
+
   auto old_listener = new NiceMock<Network::MockListener>();
-  TestListener* old_test_listener = addListener(old_listener_tag, true, false, "test_listener",
-                                                old_listener, &old_listener_callbacks);
+  auto mock_connection_balancer = std::make_shared<Network::MockConnectionBalancer>();
+
+  TestListener* old_test_listener =
+      addListener(old_listener_tag, true, false, "test_listener", old_listener,
+                  &old_listener_callbacks, mock_connection_balancer, &current_handler);
   EXPECT_CALL(*socket_factory_, localAddress()).WillOnce(ReturnRef(local_address_));
   handler_->addListener(absl::nullopt, *old_test_listener);
   ASSERT_NE(old_test_listener, nullptr);
@@ -973,18 +981,24 @@ TEST_F(ConnectionHandlerTest, TcpListenerInplaceUpdate) {
 
   auto overridden_filter_chain_manager =
       std::make_shared<NiceMock<Network::MockFilterChainManager>>();
-  TestListener* new_test_listener =
-      addListener(new_listener_tag, true, false, "test_listener", /* Network::Listener */ nullptr,
-                  &new_listener_callbacks, nullptr, nullptr, Network::Socket::Type::Stream,
-                  std::chrono::milliseconds(15000), false, overridden_filter_chain_manager);
+  TestListener* new_test_listener = addListener(
+      new_listener_tag, true, false, "test_listener", /* Network::Listener */ nullptr,
+      &new_listener_callbacks, mock_connection_balancer, nullptr, Network::Socket::Type::Stream,
+      std::chrono::milliseconds(15000), false, overridden_filter_chain_manager);
   handler_->addListener(old_listener_tag, *new_test_listener);
   ASSERT_EQ(new_listener_callbacks, nullptr)
       << "new listener should be inplace added and callback should not change";
 
   Network::MockConnectionSocket* connection = new NiceMock<Network::MockConnectionSocket>();
+  current_handler->incNumConnections();
+
+  EXPECT_CALL(*mock_connection_balancer, pickTargetHandler(_))
+      .WillOnce(ReturnRef(*current_handler));
   EXPECT_CALL(manager_, findFilterChain(_)).Times(0);
   EXPECT_CALL(*overridden_filter_chain_manager, findFilterChain(_)).WillOnce(Return(nullptr));
+  EXPECT_CALL(*mock_connection_balancer, unregisterHandler(_));
   old_listener_callbacks->onAccept(Network::ConnectionSocketPtr{connection});
+  EXPECT_EQ(0UL, handler_->numConnections());
   EXPECT_CALL(*old_listener, onDestroy());
 }