Revert "config: removing deprecated bind_to_port (#5288)" (#5344)

This reverts commit 36db776.

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

api/envoy/api/v2/lds.proto

@@ -86,7 +86,23 @@ message Listener {
   // Listener metadata.
   core.Metadata metadata = 6;
 
-  reserved 7;
+  // [#not-implemented-hide:]
+  message DeprecatedV1 {
+    // Whether the listener should bind to the port. A listener that doesn’t
+    // bind can only receive connections redirected from other listeners that
+    // set use_original_dst parameter to true. Default is true.
+    //
+    // [V2-API-DIFF] This is deprecated in v2, all Listeners will bind to their
+    // port. An additional filter chain must be created for every original
+    // destination port this listener may redirect to in v2, with the original
+    // port specified in the FilterChainMatch destination_port field.
+    //
+    // [#comment:TODO(PiotrSikora): Remove this once verified that we no longer need it.]
+    google.protobuf.BoolValue bind_to_port = 1;
+  }
+
+  // [#not-implemented-hide:]
+  DeprecatedV1 deprecated_v1 = 7;
 
   enum DrainType {
     // Drain in response to calling /healthcheck/fail admin endpoint (along with the health check

docs/root/intro/version_history.rst

@@ -15,7 +15,6 @@ Version history
 * circuit-breaker: added cx_open, rq_pending_open, rq_open and rq_retry_open gauges to expose live
   state via :ref:`circuit breakers statistics <config_cluster_manager_cluster_stats_circuit_breakers>`.
 * cluster: set a default of 1s for :ref:`option <envoy_api_field_Cluster.CommonLbConfig.update_merge_window>`.
-* config: removed deprecated_v1 bind_to_port option.
 * config: removed support for the v1 API.
 * config: added support for :ref:`rate limiting<envoy_api_msg_core.RateLimitSettings>` discovery request calls.
 * cors: added :ref: `invalid/valid stats <cors-statistics>` to filter.

include/envoy/event/dispatcher.h

@@ -101,12 +101,13 @@ class Dispatcher {
    * Create a listener on a specific port.
    * @param socket supplies the socket to listen on.
    * @param cb supplies the callbacks to invoke for listener events.
+   * @param bind_to_port controls whether the listener binds to a transport port or not.
    * @param hand_off_restored_destination_connections controls whether the listener searches for
    *        another listener after restoring the destination address of a new connection.
    * @return Network::ListenerPtr a new listener that is owned by the caller.
    */
   virtual Network::ListenerPtr createListener(Network::Socket& socket,
-                                              Network::ListenerCallbacks& cb,
+                                              Network::ListenerCallbacks& cb, bool bind_to_port,
                                               bool hand_off_restored_destination_connections) PURE;
 
   /**

include/envoy/network/listener.h

@@ -38,6 +38,13 @@ class ListenerConfig {
   virtual Socket& socket() PURE;
   virtual const Socket& socket() const PURE;
 
+  /**
+   * @return bool specifies whether the listener should actually listen on the port.
+   *         A listener that doesn't listen on a port can only receive connections
+   *         redirected from other listeners.
+   */
+  virtual bool bindToPort() PURE;
+
   /**
    * @return bool if a connection should be handed off to another Listener after the original
    *         destination address has been restored. 'true' when 'use_original_dst' flag in listener

include/envoy/server/listener_manager.h

@@ -45,11 +45,12 @@ class ListenerComponentFactory {
    * Creates a socket.
    * @param address supplies the socket's address.
    * @param options to be set on the created socket just before calling 'bind()'.
+   * @param bind_to_port supplies whether to actually bind the socket.
    * @return Network::SocketSharedPtr an initialized and potentially bound socket.
    */
   virtual Network::SocketSharedPtr
   createListenSocket(Network::Address::InstanceConstSharedPtr address,
-                     const Network::Socket::OptionsSharedPtr& options) PURE;
+                     const Network::Socket::OptionsSharedPtr& options, bool bind_to_port) PURE;
 
   /**
    * Creates a list of filter factories.

source/common/config/lds_json.cc

@@ -58,6 +58,7 @@ void LdsJson::translateListener(const Json::Object& json_listener,
 
   JSON_UTIL_SET_BOOL(json_listener, *filter_chain, use_proxy_proto);
   JSON_UTIL_SET_BOOL(json_listener, listener, use_original_dst);
+  JSON_UTIL_SET_BOOL(json_listener, *listener.mutable_deprecated_v1(), bind_to_port);
   JSON_UTIL_SET_INTEGER(json_listener, listener, per_connection_buffer_limit_bytes);
   JSON_UTIL_SET_BOOL(json_listener, listener, transparent);
 }

source/common/event/dispatcher_impl.cc

@@ -110,10 +110,10 @@ Filesystem::WatcherPtr DispatcherImpl::createFilesystemWatcher() {
 
 Network::ListenerPtr
 DispatcherImpl::createListener(Network::Socket& socket, Network::ListenerCallbacks& cb,
-                               bool hand_off_restored_destination_connections) {
+                               bool bind_to_port, bool hand_off_restored_destination_connections) {
   ASSERT(isThreadSafe());
-  return Network::ListenerPtr{
-      new Network::ListenerImpl(*this, socket, cb, hand_off_restored_destination_connections)};
+  return Network::ListenerPtr{new Network::ListenerImpl(*this, socket, cb, bind_to_port,
+                                                        hand_off_restored_destination_connections)};
 }
 
 TimerPtr DispatcherImpl::createTimer(TimerCb cb) {

source/common/event/dispatcher_impl.h

@@ -48,6 +48,7 @@ class DispatcherImpl : Logger::Loggable<Logger::Id::main>, public Dispatcher {
                                uint32_t events) override;
   Filesystem::WatcherPtr createFilesystemWatcher() override;
   Network::ListenerPtr createListener(Network::Socket& socket, Network::ListenerCallbacks& cb,
+                                      bool bind_to_port,
                                       bool hand_off_restored_destination_connections) override;
   TimerPtr createTimer(TimerCb cb) override;
   void deferredDelete(DeferredDeletablePtr&& to_delete) override;

source/common/json/config_schemas.cc

@@ -184,6 +184,7 @@ const std::string Json::Schema::LISTENER_SCHEMA(R"EOF(
        "drain_type": {"type" : "string", "enum" : ["default", "modify_only"]},
        "ssl_context" : {"$ref" : "#/definitions/ssl_context"},
        "transparent" : {"type": "boolean"},
+       "bind_to_port" : {"type": "boolean"},
        "use_proxy_proto" : {"type" : "boolean"},
        "use_original_dst" : {"type" : "boolean"},
        "per_connection_buffer_limit_bytes" : {

source/common/network/listen_socket_impl.cc

@@ -38,10 +38,13 @@ void ListenSocketImpl::setListenSocketOptions(const Network::Socket::OptionsShar
   }
 }
 
-void ListenSocketImpl::setupSocket(const Network::Socket::OptionsSharedPtr& options) {
+void ListenSocketImpl::setupSocket(const Network::Socket::OptionsSharedPtr& options,
+                                   bool bind_to_port) {
   setListenSocketOptions(options);
 
-  doBind();
+  if (bind_to_port) {
+    doBind();
+  }
 }
 
 template <>

source/common/network/listen_socket_impl.h

@@ -56,7 +56,7 @@ class ListenSocketImpl : public SocketImpl {
   ListenSocketImpl(int fd, const Address::InstanceConstSharedPtr& local_address)
       : SocketImpl(fd, local_address) {}
 
-  void setupSocket(const Network::Socket::OptionsSharedPtr& options);
+  void setupSocket(const Network::Socket::OptionsSharedPtr& options, bool bind_to_port);
   void doBind();
   void setListenSocketOptions(const Network::Socket::OptionsSharedPtr& options);
 };
@@ -77,13 +77,13 @@ template <> struct NetworkSocketTrait<Address::SocketType::Datagram> {
 template <typename T> class NetworkListenSocket : public ListenSocketImpl {
 public:
   NetworkListenSocket(const Address::InstanceConstSharedPtr& address,
-                      const Network::Socket::OptionsSharedPtr& options)
+                      const Network::Socket::OptionsSharedPtr& options, bool bind_to_port)
       : ListenSocketImpl(address->socket(T::type), address) {
     RELEASE_ASSERT(fd_ != -1, "");
 
     setPrebindSocketOptions();
 
-    setupSocket(options);
+    setupSocket(options, bind_to_port);
   }
 
   NetworkListenSocket(int fd, const Address::InstanceConstSharedPtr& address,

source/common/network/listener_impl.cc

@@ -45,7 +45,7 @@ void ListenerImpl::listenCallback(evconnlistener*, evutil_socket_t fd, sockaddr*
 }
 
 ListenerImpl::ListenerImpl(Event::DispatcherImpl& dispatcher, Socket& socket, ListenerCallbacks& cb,
-                           bool hand_off_restored_destination_connections)
+                           bool bind_to_port, bool hand_off_restored_destination_connections)
     : local_address_(nullptr), cb_(cb),
       hand_off_restored_destination_connections_(hand_off_restored_destination_connections),
       listener_(nullptr) {
@@ -57,20 +57,23 @@ ListenerImpl::ListenerImpl(Event::DispatcherImpl& dispatcher, Socket& socket, Li
     local_address_ = socket.localAddress();
   }
 
-  listener_.reset(evconnlistener_new(&dispatcher.base(), listenCallback, this, 0, -1, socket.fd()));
+  if (bind_to_port) {
+    listener_.reset(
+        evconnlistener_new(&dispatcher.base(), listenCallback, this, 0, -1, socket.fd()));
 
-  if (!listener_) {
-    throw CreateListenerException(
-        fmt::format("cannot listen on socket: {}", socket.localAddress()->asString()));
-  }
+    if (!listener_) {
+      throw CreateListenerException(
+          fmt::format("cannot listen on socket: {}", socket.localAddress()->asString()));
+    }
 
-  if (!Network::Socket::applyOptions(socket.options(), socket,
-                                     envoy::api::v2::core::SocketOption::STATE_LISTENING)) {
-    throw CreateListenerException(fmt::format("cannot set post-listen socket option on socket: {}",
-                                              socket.localAddress()->asString()));
-  }
+    if (!Network::Socket::applyOptions(socket.options(), socket,
+                                       envoy::api::v2::core::SocketOption::STATE_LISTENING)) {
+      throw CreateListenerException(fmt::format(
+          "cannot set post-listen socket option on socket: {}", socket.localAddress()->asString()));
+    }
 
-  evconnlistener_set_error_cb(listener_.get(), errorCallback);
+    evconnlistener_set_error_cb(listener_.get(), errorCallback);
+  }
 }
 
 void ListenerImpl::errorCallback(evconnlistener*, void*) {

source/common/network/listener_impl.h

@@ -17,7 +17,7 @@ namespace Network {
 class ListenerImpl : public Listener {
 public:
   ListenerImpl(Event::DispatcherImpl& dispatcher, Socket& socket, ListenerCallbacks& cb,
-               bool hand_off_restored_destination_connections);
+               bool bind_to_port, bool hand_off_restored_destination_connections);
 
   void disable();
   void enable();

source/server/config_validation/dispatcher.cc

@@ -22,7 +22,7 @@ Network::DnsResolverSharedPtr ValidationDispatcher::createDnsResolver(
 }
 
 Network::ListenerPtr ValidationDispatcher::createListener(Network::Socket&,
-                                                          Network::ListenerCallbacks&, bool) {
+                                                          Network::ListenerCallbacks&, bool, bool) {
   NOT_IMPLEMENTED_GCOVR_EXCL_LINE;
 }
 

source/server/config_validation/dispatcher.h

@@ -25,6 +25,7 @@ class ValidationDispatcher : public DispatcherImpl {
   Network::DnsResolverSharedPtr createDnsResolver(
       const std::vector<Network::Address::InstanceConstSharedPtr>& resolvers) override;
   Network::ListenerPtr createListener(Network::Socket&, Network::ListenerCallbacks&,
+                                      bool bind_to_port,
                                       bool hand_off_restored_destination_connections) override;
 
 protected:

source/server/config_validation/server.h

@@ -114,7 +114,8 @@ class ValidationInstance : Logger::Loggable<Logger::Id::main>,
     return ProdListenerComponentFactory::createListenerFilterFactoryList_(filters, context);
   }
   Network::SocketSharedPtr createListenSocket(Network::Address::InstanceConstSharedPtr,
-                                              const Network::Socket::OptionsSharedPtr&) override {
+                                              const Network::Socket::OptionsSharedPtr&,
+                                              bool) override {
     // Returned sockets are not currently used so we can return nothing here safely vs. a
     // validation mock.
     return nullptr;

source/server/connection_handler_impl.cc

@@ -74,10 +74,11 @@ void ConnectionHandlerImpl::ActiveListener::removeConnection(ActiveConnection& c
 
 ConnectionHandlerImpl::ActiveListener::ActiveListener(ConnectionHandlerImpl& parent,
                                                       Network::ListenerConfig& config)
-    : ActiveListener(parent,
-                     parent.dispatcher_.createListener(
-                         config.socket(), *this, config.handOffRestoredDestinationConnections()),
-                     config) {}
+    : ActiveListener(
+          parent,
+          parent.dispatcher_.createListener(config.socket(), *this, config.bindToPort(),
+                                            config.handOffRestoredDestinationConnections()),
+          config) {}
 
 ConnectionHandlerImpl::ActiveListener::ActiveListener(ConnectionHandlerImpl& parent,
                                                       Network::ListenerPtr&& listener,

source/server/http/admin.cc

@@ -962,7 +962,7 @@ void AdminImpl::startHttpListener(const std::string& access_log_path,
   access_logs_.emplace_back(new Extensions::AccessLoggers::File::FileAccessLog(
       access_log_path, {}, AccessLog::AccessLogFormatUtils::defaultAccessLogFormatter(),
       server_.accessLogManager()));
-  socket_ = std::make_unique<Network::TcpListenSocket>(address, nullptr);
+  socket_ = std::make_unique<Network::TcpListenSocket>(address, nullptr, true);
   listener_ = std::make_unique<AdminListener>(*this, std::move(listener_scope));
   if (!address_out_path.empty()) {
     std::ofstream address_out_file(address_out_path);

source/server/http/admin.h

@@ -258,6 +258,7 @@ class AdminImpl : public Admin,
     Network::FilterChainFactory& filterChainFactory() override { return parent_; }
     Network::Socket& socket() override { return parent_.mutable_socket(); }
     const Network::Socket& socket() const override { return parent_.mutable_socket(); }
+    bool bindToPort() override { return true; }
     bool handOffRestoredDestinationConnections() const override { return false; }
     uint32_t perConnectionBufferLimitBytes() const override { return 0; }
     std::chrono::milliseconds listenerFiltersTimeout() const override {

source/server/listener_manager_impl.cc

@@ -84,7 +84,8 @@ ProdListenerComponentFactory::createListenerFilterFactoryList_(
 
 Network::SocketSharedPtr
 ProdListenerComponentFactory::createListenSocket(Network::Address::InstanceConstSharedPtr address,
-                                                 const Network::Socket::OptionsSharedPtr& options) {
+                                                 const Network::Socket::OptionsSharedPtr& options,
+                                                 bool bind_to_port) {
   ASSERT(address->type() == Network::Address::Type::Ip ||
          address->type() == Network::Address::Type::Pipe);
 
@@ -106,7 +107,7 @@ ProdListenerComponentFactory::createListenSocket(Network::Address::InstanceConst
     ENVOY_LOG(debug, "obtained socket for address {} from parent", addr);
     return std::make_shared<Network::TcpListenSocket>(fd, address, options);
   }
-  return std::make_shared<Network::TcpListenSocket>(address, options);
+  return std::make_shared<Network::TcpListenSocket>(address, options, bind_to_port);
 }
 
 DrainManagerPtr
@@ -121,6 +122,7 @@ ListenerImpl::ListenerImpl(const envoy::api::v2::Listener& config, const std::st
       global_scope_(parent_.server_.stats().createScope("")),
       listener_scope_(
           parent_.server_.stats().createScope(fmt::format("listener.{}.", address_->asString()))),
+      bind_to_port_(PROTOBUF_GET_WRAPPED_OR_DEFAULT(config.deprecated_v1(), bind_to_port, true)),
       hand_off_restored_destination_connections_(
           PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, use_original_dst, false)),
       per_connection_buffer_limit_bytes_(
@@ -739,6 +741,21 @@ bool ListenerManagerImpl::addOrUpdateListener(const envoy::api::v2::Listener& co
       *existing_active_listener = std::move(new_listener);
     }
   } else {
+    // 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 (!new_listener->bindToPort() &&
+        (hasListenerWithAddress(warming_listeners_, *new_listener->address()) ||
+         hasListenerWithAddress(active_listeners_, *new_listener->address()))) {
+      const std::string message =
+          fmt::format("error adding listener: '{}' has duplicate address '{}' as existing listener",
+                      name, new_listener->address()->asString());
+      ENVOY_LOG(warn, "{}", message);
+      throw EnvoyException(message);
+    }
+
     // We have no warming or active listener so we need to make a new one. What we do depends on
     // whether workers have been started or not. Additionally, search through draining listeners
     // to see if there is a listener that has a socket bound to the address we are configured for.
@@ -757,7 +774,8 @@ bool ListenerManagerImpl::addOrUpdateListener(const envoy::api::v2::Listener& co
     new_listener->setSocket(draining_listener_socket
                                 ? draining_listener_socket
                                 : factory_.createListenSocket(new_listener->address(),
-                                                              new_listener->listenSocketOptions()));
+                                                              new_listener->listenSocketOptions(),
+                                                              new_listener->bindToPort()));
     if (workers_started_) {
       new_listener->debugLog("add warming listener");
       warming_listeners_.emplace_back(std::move(new_listener));

source/server/listener_manager_impl.h

@@ -60,9 +60,9 @@ class ProdListenerComponentFactory : public ListenerComponentFactory,
       Configuration::ListenerFactoryContext& context) override {
     return createListenerFilterFactoryList_(filters, context);
   }
-  Network::SocketSharedPtr
-  createListenSocket(Network::Address::InstanceConstSharedPtr address,
-                     const Network::Socket::OptionsSharedPtr& options) override;
+  Network::SocketSharedPtr createListenSocket(Network::Address::InstanceConstSharedPtr address,
+                                              const Network::Socket::OptionsSharedPtr& options,
+                                              bool bind_to_port) override;
   DrainManagerPtr createDrainManager(envoy::api::v2::Listener::DrainType drain_type) override;
   uint64_t nextListenerTag() override { return next_listener_tag_++; }
 
@@ -242,6 +242,7 @@ class ListenerImpl : public Network::ListenerConfig,
   Network::FilterChainFactory& filterChainFactory() override { return *this; }
   Network::Socket& socket() override { return *socket_; }
   const Network::Socket& socket() const override { return *socket_; }
+  bool bindToPort() override { return bind_to_port_; }
   bool handOffRestoredDestinationConnections() const override {
     return hand_off_restored_destination_connections_;
   }
@@ -388,6 +389,7 @@ class ListenerImpl : public Network::ListenerConfig,
   Network::SocketSharedPtr socket_;
   Stats::ScopePtr global_scope_;   // Stats with global named scope, but needed for LDS cleanup.
   Stats::ScopePtr listener_scope_; // Stats with listener named scope.
+  const bool bind_to_port_;
   const bool hand_off_restored_destination_connections_;
   const uint32_t per_connection_buffer_limit_bytes_;
   const uint64_t listener_tag_;