listener: fixed a bug where the addresses cannot be updated partially

Signed-off-by: wangbaiping(wbpcode) <[email protected]>

api/envoy/config/listener/v3/listener.proto

@@ -247,10 +247,10 @@ message Listener {
   google.protobuf.BoolValue freebind = 11;
 
   // Additional socket options that may not be present in Envoy source code or
-  // precompiled binaries. The socket options can be updated for a listener when
+  // precompiled binaries.
+  // It is not allowed to update the socket options for any existing address if
   // :ref:`enable_reuse_port <envoy_v3_api_field_config.listener.v3.Listener.enable_reuse_port>`
-  // is ``true``. Otherwise, if socket options change during a listener update the update will be rejected
-  // to make it clear that the options were not updated.
+  // is ``false`` to avoid the conflict when creating new sockets for the listener.
   repeated core.v3.SocketOption socket_options = 13;
 
   // Whether the listener should accept TCP Fast Open (TFO) connections.

changelogs/current.yaml

@@ -80,6 +80,9 @@ bug_fixes:
     Before the :scheme header was always 'http'.
     This behavioral change can be temporarily reverted by setting runtime guard
     ``envoy.reloadable_features.jwt_fetcher_use_scheme_from_uri`` to false.
+- area: listener
+  change: |
+    Fixed a bug where the addresses cannot be updated partially even the reuse port is enabled.
 
 removed_config_or_runtime:
 # *Normally occurs at the end of the* :ref:`deprecation period <deprecated>`

source/common/listener_manager/listener_impl.cc

@@ -1128,15 +1128,13 @@ bool ListenerImpl::getReusePortOrDefault(Server::Instance& server,
   return initial_reuse_port_value;
 }
 
-bool ListenerImpl::socketOptionsEqual(const ListenerImpl& other) const {
-  return ListenerMessageUtil::socketOptionsEqual(config(), other.config());
-}
-
 bool ListenerImpl::hasCompatibleAddress(const ListenerImpl& other) const {
-  if ((socket_type_ != other.socket_type_) || (addresses_.size() != other.addresses().size())) {
+  // First, check if the listener has the same socket type and the same number of addresses.
+  if (socket_type_ != other.socket_type_ || addresses_.size() != other.addresses().size()) {
     return false;
   }
 
+  // Second, check if the listener has the same addresses.
   // The listener support listening on the zero port address for test. Multiple zero
   // port addresses are also supported. For comparing two listeners with multiple
   // zero port addresses, only need to ensure there are the same number of zero
@@ -1153,17 +1151,12 @@ bool ListenerImpl::hasCompatibleAddress(const ListenerImpl& other) const {
     }
     other_addresses.erase(iter);
   }
-  return true;
+
+  // Third, check if the listener has the same socket options.
+  return ListenerMessageUtil::socketOptionsEqual(config(), other.config());
 }
 
 bool ListenerImpl::hasDuplicatedAddress(const ListenerImpl& other) const {
-  // Skip the duplicate address check if this is the case of a listener update with new socket
-  // options.
-  if ((name_ == other.name_) &&
-      !ListenerMessageUtil::socketOptionsEqual(config(), other.config())) {
-    return false;
-  }
-
   if (socket_type_ != other.socket_type_) {
     return false;
   }

source/common/listener_manager/listener_impl.h

@@ -270,8 +270,6 @@ class ListenerImpl final : public Network::ListenerConfig,
                                     const envoy::config::listener::v3::Listener& config,
                                     Network::Socket::Type socket_type);
 
-  // Compare whether two listeners have different socket options.
-  bool socketOptionsEqual(const ListenerImpl& other) const;
   // Check whether a new listener can share sockets with this listener.
   bool hasCompatibleAddress(const ListenerImpl& other) const;
   // Check whether a new listener has duplicated listening address this listener.

source/common/listener_manager/listener_manager_impl.cc

@@ -512,21 +512,12 @@ ListenerManagerImpl::addOrUpdateListener(const envoy::config::listener::v3::List
 absl::Status
 ListenerManagerImpl::setupSocketFactoryForListener(ListenerImpl& new_listener,
                                                    const ListenerImpl& existing_listener) {
-  bool same_socket_options = true;
   if (new_listener.reusePort() != existing_listener.reusePort()) {
     return absl::InvalidArgumentError(fmt::format(
         "Listener {}: reuse port cannot be changed during an update", new_listener.name()));
   }
 
-  same_socket_options = existing_listener.socketOptionsEqual(new_listener);
-  if (!same_socket_options && new_listener.reusePort() == false) {
-    return absl::InvalidArgumentError(
-        fmt::format("Listener {}: doesn't support update any socket options "
-                    "when the reuse port isn't enabled",
-                    new_listener.name()));
-  }
-
-  if (!(existing_listener.hasCompatibleAddress(new_listener) && same_socket_options)) {
+  if (!existing_listener.hasCompatibleAddress(new_listener)) {
     RETURN_IF_NOT_OK(setNewOrDrainingSocketFactory(new_listener.name(), new_listener));
   } else {
     RETURN_IF_NOT_OK(new_listener.cloneSocketFactoryFrom(existing_listener));
@@ -639,9 +630,21 @@ absl::StatusOr<bool> ListenerManagerImpl::addOrUpdateListenerInternal(
   return true;
 }
 
-bool ListenerManagerImpl::hasListenerWithDuplicatedAddress(const ListenerList& list,
+bool ListenerManagerImpl::hasListenerWithDuplicatedAddress(const ListenerList& listener_list,
                                                            const ListenerImpl& listener) {
-  for (const auto& existing_listener : list) {
+  // This is new listener or new version of existing listener but with different addresses
+  // or different socket options.
+  // Check if the listener has duplicated address with existing listeners.
+
+  for (const auto& existing_listener : listener_list) {
+    // If reuse port is enabled, we can skip the check between different versions of the same
+    // listener.
+    // If reuse port is disabled, the duplicated addresses checking is necessary to ensure we
+    // can create new sockets for the new version of the listener as expected.
+    if (listener.reusePort() && existing_listener->name() == listener.name()) {
+      continue;
+    }
+
     if (existing_listener->hasDuplicatedAddress(listener)) {
       return true;
     }
@@ -1180,6 +1183,9 @@ absl::Status ListenerManagerImpl::setNewOrDrainingSocketFactory(const std::strin
     }
   }
 
+  // TODO(wbpcode): if we cannot clone the socket factory from the draining listener, we should
+  // check the duplicated addresses again the draining listeners to avoid the creation failure
+  // of the sockets.
   if (draining_listener_ptr != nullptr) {
     RETURN_IF_NOT_OK(listener.cloneSocketFactoryFrom(*draining_listener_ptr));
   } else {

test/common/listener_manager/listener_manager_impl_test.cc

@@ -400,6 +400,95 @@ name: bar
   addOrUpdateListener(parseListenerFromV3Yaml(yaml2));
 }
 
+TEST_P(ListenerManagerImplWithRealFiltersTest, AllowAddressesUpdateParitially) {
+  const std::string yaml1 = R"EOF(
+name: foo
+address:
+  socket_address:
+    address: 127.0.0.1
+    port_value: 1000
+additional_addresses:
+- address:
+    socket_address:
+      address: 127.0.0.2
+      port_value: 1000
+filter_chains:
+- filters: []
+  name: foo
+  )EOF";
+
+  const std::string yaml2 = R"EOF(
+name: foo
+address:
+  socket_address:
+    address: 127.0.0.1
+    port_value: 1000
+additional_addresses:
+- address:
+    socket_address:
+      address: 127.0.0.3
+      port_value: 2000
+filter_chains:
+- filters: []
+  name: foo
+  )EOF";
+
+  EXPECT_CALL(listener_factory_, createListenSocket(_, _, _, default_bind_type, _, 0)).Times(2);
+  addOrUpdateListener(parseListenerFromV3Yaml(yaml1));
+  EXPECT_CALL(listener_factory_, createListenSocket(_, _, _, default_bind_type, _, 0)).Times(2);
+  addOrUpdateListener(parseListenerFromV3Yaml(yaml2));
+}
+
+TEST_P(ListenerManagerImplWithRealFiltersTest,
+       AllowUpdateSocketOptionsIfNotDuplicatedEvenReusePortIsDisabled) {
+  const std::string yaml1 = R"EOF(
+name: foo
+address:
+  socket_address:
+    address: 127.0.0.1
+    port_value: 1000
+additional_addresses:
+- address:
+    socket_address:
+      address: 127.0.0.2
+      port_value: 1000
+enable_reuse_port: false
+filter_chains:
+- filters: []
+  name: foo
+  )EOF";
+
+  const std::string yaml2 = R"EOF(
+name: foo
+address:
+  socket_address:
+    address: 127.0.0.4
+    port_value: 1000
+additional_addresses:
+- address:
+    socket_address:
+      address: 127.0.0.3
+      port_value: 2000
+enable_reuse_port: false
+socket_options:
+    - level: 1
+      name: 9
+      int_value: 1
+filter_chains:
+- filters: []
+  name: foo
+  )EOF";
+
+  EXPECT_CALL(listener_factory_,
+              createListenSocket(_, _, _, ListenerComponentFactory::BindType::NoReusePort, _, 0))
+      .Times(2);
+  addOrUpdateListener(parseListenerFromV3Yaml(yaml1));
+  EXPECT_CALL(listener_factory_,
+              createListenSocket(_, _, _, ListenerComponentFactory::BindType::NoReusePort, _, 0))
+      .Times(2);
+  addOrUpdateListener(parseListenerFromV3Yaml(yaml2));
+}
+
 TEST_P(ListenerManagerImplWithRealFiltersTest, SetListenerPerConnectionBufferLimit) {
   const std::string yaml = R"EOF(
 address: