listener: fixed a bug where the addresses cannot be updated partially (#38601)

Author: wbpcode

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 if 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

@@ -271,8 +271,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 (listener.reusePort() && existing_listener->name() == listener.name()) {
+      // If reuse port is enabled, we can skip the check between different versions of the
+      // same listener as they can create their own sockets anyway.
+      // If reuse port is disabled, the duplicated addresses check is necessary to to avoid
+      // attempting to bind to the same address when creating new sockets for the listener.
+      continue;
+    }
+
     if (existing_listener->hasDuplicatedAddress(listener)) {
       return true;
     }
@@ -1148,7 +1151,9 @@ absl::Status ListenerManagerImpl::setNewOrDrainingSocketFactory(const std::strin
   if (hasListenerWithDuplicatedAddress(warming_listeners_, listener) ||
       hasListenerWithDuplicatedAddress(active_listeners_, listener)) {
     const std::string message =
-        fmt::format("error adding listener: '{}' has duplicate address '{}' as existing listener",
+        fmt::format("error adding listener: '{}' has duplicate address '{}' as existing listener, "
+                    "to check if the listener has duplicated addresses with other listeners or "
+                    "'enable_reuse_port' is set to 'false' for the listener",
                     name, absl::StrJoin(listener.addresses(), ",", Network::AddressStrFormatter()));
     ENVOY_LOG(warn, "{}", message);
     return absl::InvalidArgumentError(message);
@@ -1188,6 +1193,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

@@ -251,7 +251,9 @@ name: bar
   addOrUpdateListener(parseListenerFromV3Yaml(yaml1));
   EXPECT_THROW_WITH_MESSAGE(
       addOrUpdateListener(parseListenerFromV3Yaml(yaml2)), EnvoyException,
-      "error adding listener: 'bar' has duplicate address '127.0.0.1:1234' as existing listener");
+      "error adding listener: 'bar' has duplicate address '127.0.0.1:1234' as existing listener, "
+      "to check if the listener has duplicated addresses with other listeners or "
+      "'enable_reuse_port' is set to 'false' for the listener");
 }
 
 TEST_P(ListenerManagerImplWithRealFiltersTest, DuplicateNonIPAddressNotAllowed) {
@@ -278,7 +280,9 @@ name: bar
   addOrUpdateListener(parseListenerFromV3Yaml(yaml1));
   EXPECT_THROW_WITH_MESSAGE(
       addOrUpdateListener(parseListenerFromV3Yaml(yaml2)), EnvoyException,
-      "error adding listener: 'bar' has duplicate address '/path' as existing listener");
+      "error adding listener: 'bar' has duplicate address '/path' as existing listener, to check "
+      "if the listener has duplicated addresses with other listeners or 'enable_reuse_port' is set "
+      "to 'false' for the listener");
 }
 
 TEST_P(ListenerManagerImplWithRealFiltersTest, MultipleAddressesDuplicatePortNotAllowed) {
@@ -318,7 +322,9 @@ name: bar
   addOrUpdateListener(parseListenerFromV3Yaml(yaml1));
   EXPECT_THROW_WITH_MESSAGE(addOrUpdateListener(parseListenerFromV3Yaml(yaml2)), EnvoyException,
                             "error adding listener: 'bar' has duplicate address "
-                            "'127.0.0.1:1234,127.0.0.3:1234' as existing listener");
+                            "'127.0.0.1:1234,127.0.0.3:1234' as existing listener, to check if the "
+                            "listener has duplicated addresses with other listeners or "
+                            "'enable_reuse_port' is set to 'false' for the listener");
 }
 
 TEST_P(ListenerManagerImplWithRealFiltersTest,
@@ -358,9 +364,11 @@ bind_to_port: false
   )EOF";
 
   addOrUpdateListener(parseListenerFromV3Yaml(yaml1));
-  EXPECT_THROW_WITH_MESSAGE(addOrUpdateListener(parseListenerFromV3Yaml(yaml2)), EnvoyException,
-                            "error adding listener: 'bar' has duplicate address "
-                            "'127.0.0.1:0,127.0.0.3:0' as existing listener");
+  EXPECT_THROW_WITH_MESSAGE(
+      addOrUpdateListener(parseListenerFromV3Yaml(yaml2)), EnvoyException,
+      "error adding listener: 'bar' has duplicate address "
+      "'127.0.0.1:0,127.0.0.3:0' as existing listener, to check if the listener has duplicated "
+      "addresses with other listeners or 'enable_reuse_port' is set to 'false' for the listener");
 }
 
 TEST_P(ListenerManagerImplWithRealFiltersTest, AllowCreateListenerWithMutipleZeroPorts) {
@@ -402,6 +410,98 @@ name: bar
   addOrUpdateListener(parseListenerFromV3Yaml(yaml2));
 }
 
+TEST_P(ListenerManagerImplWithRealFiltersTest, AllowAddressesUpdatePartially) {
+  // Update one of the addresses from '127.0.0.2:1000' to '127.0.0.3:2000'.
+  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) {
+  // All addresses are different, so it should be allowed to update socket options even
+  // reuse port is disabled.
+  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:
@@ -2298,7 +2398,9 @@ name: bar
   )EOF";
 
   const std::string expected_error_message =
-      "error adding listener: 'bar' has duplicate address '127.0.0.1:1234' as existing listener";
+      "error adding listener: 'bar' has duplicate address '127.0.0.1:1234' as existing listener, "
+      "to check if the listener has duplicated addresses with other listeners or "
+      "'enable_reuse_port' is set to 'false' for the listener";
   testListenerUpdateWithSocketOptionsChangeRejected(listener_origin, listener_updated,
                                                     expected_error_message);
 }
@@ -3343,7 +3445,9 @@ bind_to_port: false
   EXPECT_CALL(*listener_bar, onDestroy());
   EXPECT_THROW_WITH_MESSAGE(
       addOrUpdateListener(parseListenerFromV3Yaml(listener_bar_yaml)), EnvoyException,
-      "error adding listener: 'bar' has duplicate address '0.0.0.0:1234' as existing listener");
+      "error adding listener: 'bar' has duplicate address '0.0.0.0:1234' as existing listener, to "
+      "check if the listener has duplicated addresses with other listeners or 'enable_reuse_port' "
+      "is set to 'false' for the listener");
 
   // Move foo to active and then try to add again. This should still fail.
   EXPECT_CALL(*worker_, addListener(_, _, _, _, _));
@@ -3354,7 +3458,9 @@ bind_to_port: false
   EXPECT_CALL(*listener_bar, onDestroy());
   EXPECT_THROW_WITH_MESSAGE(
       addOrUpdateListener(parseListenerFromV3Yaml(listener_bar_yaml)), EnvoyException,
-      "error adding listener: 'bar' has duplicate address '0.0.0.0:1234' as existing listener");
+      "error adding listener: 'bar' has duplicate address '0.0.0.0:1234' as existing listener, to "
+      "check if the listener has duplicated addresses with other listeners or 'enable_reuse_port' "
+      "is set to 'false' for the listener");
 
   EXPECT_CALL(*listener_foo, onDestroy());
 }

test/integration/listener_lds_integration_test.cc

@@ -8,6 +8,7 @@
 #include "envoy/network/connection.h"
 #include "envoy/service/discovery/v3/discovery.pb.h"
 
+#include "source/common/common/random_generator.h"
 #include "source/common/config/api_version.h"
 
 #include "test/common/grpc/grpc_client_integration.h"
@@ -1639,6 +1640,12 @@ TEST_P(ListenerFilterIntegrationTest,
     listener_config_.set_name("listener_foo");
     listener_config_.set_stat_prefix("listener_stat");
     listener_config_.mutable_enable_reuse_port()->set_value(false);
+
+    // Set random port manually for test. 0 port means the kernel will generate a random port
+    // and envoy will skip the port conflict check.
+    const uint32_t random_port = Random::RandomUtility::random() % 10000 + 10000;
+    listener_config_.mutable_address()->mutable_socket_address()->set_port_value(random_port);
+
     ENVOY_LOG_MISC(debug, "listener config: {}", listener_config_.DebugString());
     bootstrap.mutable_static_resources()->mutable_listeners()->Clear();
     auto* lds_config_source = bootstrap.mutable_dynamic_resources()->mutable_lds_config();
@@ -1682,8 +1689,8 @@ TEST_P(ListenerFilterIntegrationTest,
   EXPECT_LOG_CONTAINS(
       "warning",
       "gRPC config for type.googleapis.com/envoy.config.listener.v3.Listener rejected: Error "
-      "adding/updating listener(s) listener_foo: Listener listener_foo: doesn't support update any "
-      "socket options when the reuse port isn't enabled",
+      "adding/updating listener(s) listener_foo: error adding listener: 'listener_foo' has "
+      "duplicate address",
       {
         sendLdsResponse({MessageUtil::getYamlStringFromMessage(listener_config_)}, "2");
         test_server_->waitForCounterGe("listener_manager.lds.update_rejected", 1);