diff --git a/api/envoy/config/listener/v3/listener.proto b/api/envoy/config/listener/v3/listener.proto index 9381d4eb7aca..4bcd3882cb57 100644 --- a/api/envoy/config/listener/v3/listener.proto +++ b/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 ` - // 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. diff --git a/changelogs/current.yaml b/changelogs/current.yaml index f0d3d9e75ab7..62ba5ef9086d 100644 --- a/changelogs/current.yaml +++ b/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 ` diff --git a/source/common/listener_manager/listener_impl.cc b/source/common/listener_manager/listener_impl.cc index 9db574220cce..6c5040d619c8 100644 --- a/source/common/listener_manager/listener_impl.cc +++ b/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; } diff --git a/source/common/listener_manager/listener_impl.h b/source/common/listener_manager/listener_impl.h index 2f37b82b1dbb..f288b90485d2 100644 --- a/source/common/listener_manager/listener_impl.h +++ b/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. diff --git a/source/common/listener_manager/listener_manager_impl.cc b/source/common/listener_manager/listener_manager_impl.cc index 8de7a7da3360..74ebfe38a623 100644 --- a/source/common/listener_manager/listener_manager_impl.cc +++ b/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 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; } @@ -1140,7 +1143,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); @@ -1180,6 +1185,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 { diff --git a/test/common/listener_manager/listener_manager_impl_test.cc b/test/common/listener_manager/listener_manager_impl_test.cc index 2ad053736efe..be19b62ba57f 100644 --- a/test/common/listener_manager/listener_manager_impl_test.cc +++ b/test/common/listener_manager/listener_manager_impl_test.cc @@ -249,7 +249,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) { @@ -276,7 +278,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) { @@ -316,7 +320,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, @@ -356,9 +362,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) { @@ -400,6 +408,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: @@ -2295,7 +2395,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); } @@ -3333,7 +3435,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(_, _, _, _, _)); @@ -3344,7 +3448,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()); } diff --git a/test/integration/listener_lds_integration_test.cc b/test/integration/listener_lds_integration_test.cc index 51a806d09ef2..cc34e9c08224 100644 --- a/test/integration/listener_lds_integration_test.cc +++ b/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);