listener: disallow duplicate port specification

This bug existed previously, but with the recent reuse port as the default change is now more obvious. Previously, we would allow multiple listeners to listen on the same port, which is obviously very wrong. This change blocks that at config load time. Fixes #19099 Signed-off-by: Matt Klein <[email protected]>
envoyproxy · Nov 30, 2021 · c51be67 · c51be67
1 parent 3baae86
commit c51be67
Show file tree

Hide file tree

Showing 2 changed files with 35 additions and 7 deletions.
diff --git a/source/server/listener_manager_impl.cc b/source/server/listener_manager_impl.cc
@@ -961,13 +961,13 @@ Network::DrainableFilterChainSharedPtr ListenerFilterChainFactoryBuilder::buildF
 void ListenerManagerImpl::setNewOrDrainingSocketFactory(
     const std::string& name, const envoy::config::core::v3::Address& proto_address,
     ListenerImpl& listener) {
-  // Typically we catch address issues when we try to bind to the same address multiple times.
-  // However, for listeners that do not bind we must check to make sure we are not duplicating. This
-  // is an edge case and nothing will explicitly break, but there is no possibility that two
-  // listeners that do not bind will ever be used. Only the first one will be used when searched for
-  // by address. Thus we block it.
-  if (!listener.bindToPort() && (hasListenerWithCompatibleAddress(warming_listeners_, listener) ||
-                                 hasListenerWithCompatibleAddress(active_listeners_, listener))) {
+  // For listeners that do not bind or listeners that do not bind to port 0 we must check to make
+  // sure we are not duplicating the address. This avoids ambiguity about about which non-binding
+  // listener is used or even worse for the binding to port != 0 and reuse port case multiple
+  // different listeners receiving connections.
+  if ((!listener.bindToPort() || listener.config().address().socket_address().port_value() != 0) &&
+      (hasListenerWithCompatibleAddress(warming_listeners_, listener) ||
+       hasListenerWithCompatibleAddress(active_listeners_, listener))) {
     const std::string message =
         fmt::format("error adding listener: '{}' has duplicate address '{}' as existing listener",
                     name, listener.address()->asString());

diff --git a/test/server/listener_manager_impl_test.cc b/test/server/listener_manager_impl_test.cc
@@ -206,6 +206,34 @@ TEST_F(ListenerManagerImplWithRealFiltersTest, DefaultListenerPerConnectionBuffe
   EXPECT_EQ(1024 * 1024U, manager_->listeners().back().get().perConnectionBufferLimitBytes());
 }
 
+TEST_F(ListenerManagerImplWithRealFiltersTest, DuplicatePortNotAllowed) {
+  const std::string yaml1 = R"EOF(
+name: foo
+address:
+  socket_address:
+    address: 127.0.0.1
+    port_value: 1234
+filter_chains:
+- filters: []
+  )EOF";
+
+  const std::string yaml2 = R"EOF(
+name: bar
+address:
+  socket_address:
+    address: 127.0.0.1
+    port_value: 1234
+filter_chains:
+- filters: []
+  )EOF";
+
+  EXPECT_CALL(listener_factory_, createListenSocket(_, _, _, default_bind_type, _, 0));
+  manager_->addOrUpdateListener(parseListenerFromV3Yaml(yaml1), "", true);
+  EXPECT_THROW_WITH_MESSAGE(
+      manager_->addOrUpdateListener(parseListenerFromV3Yaml(yaml2), "", true), EnvoyException,
+      "error adding listener: 'bar' has duplicate address '127.0.0.1:1234' as existing listener");
+}
+
 TEST_F(ListenerManagerImplWithRealFiltersTest, SetListenerPerConnectionBufferLimit) {
   const std::string yaml = R"EOF(
 address: