From 09dd3268adcc67cae254f19e61ae284abcae2123 Mon Sep 17 00:00:00 2001 From: Enrico Schiattarella Date: Thu, 5 Jan 2017 14:48:56 -0800 Subject: [PATCH 01/22] Extend and customize listener behavior This patch allows listeners to: - be instantiated without binding to the specified port (config option bind_to_port) - have connections that have been redirected via iptables be handled by the listener associated with the original port rather than the listener that received the connection request. Use cases for these changes include: - Funnelling through the proxy connections for an entire range of destination port. If the proxy has a listener for the original destination port (before the redirect) it is used, otherwise the listener that received the connection handles it - Transparently proxy connections for an app listening on port X. The proxy can have an unbound listener for port X and a bound on for port Y. Connections originally directed to the app on port X are redirected with iptables to the proxy on port Y. The listener for port Y picks up the connection and hands it off to the listener for port X. Neither the app, nor the client, have to be changed or become aware of the presence of the proxy (except for the different src IP seen by the app). See https://docs.google.com/document/d/1v870Igrj5QS52G9O43fhxbV_S3mpvf_H6Hb8r85KZLY/ for the entire story --- include/envoy/event/dispatcher.h | 17 ++++-- include/envoy/server/configuration.h | 13 +++++ source/common/event/dispatcher_impl.cc | 16 +++--- source/common/event/dispatcher_impl.h | 6 ++- source/common/network/listen_socket_impl.cc | 13 +++-- source/common/network/listen_socket_impl.h | 4 +- source/common/network/listener_impl.cc | 54 ++++++++++++++++--- source/common/network/listener_impl.h | 32 +++++++++-- source/common/network/utility.cc | 10 ++++ source/common/network/utility.h | 19 +++++++ source/server/configuration_impl.cc | 10 ++++ source/server/configuration_impl.h | 4 ++ source/server/connection_handler.cc | 50 ++++++++++++----- source/server/connection_handler.h | 30 +++++++++-- source/server/http/admin.cc | 2 +- source/server/server.cc | 5 +- source/server/worker.cc | 7 ++- test/common/network/connection_impl_test.cc | 4 +- .../common/network/listen_socket_impl_test.cc | 4 +- test/common/network/listener_impl_test.cc | 4 +- test/common/network/proxy_protocol_test.cc | 3 +- test/common/ssl/connection_impl_test.cc | 12 ++--- test/integration/fake_upstream.cc | 10 ++-- test/mocks/event/mocks.h | 22 ++++---- test/server/connection_handler_test.cc | 6 +-- 25 files changed, 275 insertions(+), 82 deletions(-) diff --git a/include/envoy/event/dispatcher.h b/include/envoy/event/dispatcher.h index 4c24f25e59da..bb2f40025fcf 100644 --- a/include/envoy/event/dispatcher.h +++ b/include/envoy/event/dispatcher.h @@ -73,11 +73,17 @@ class Dispatcher { * @param stats_store supplies the Stats::Store to use. * @param use_proxy_proto whether to use the PROXY Protocol V1 * (http://www.haproxy.org/download/1.5/doc/proxy-protocol.txt) + * @param bind_to_port specifies if the listener should actually bind to the port. + * a listener that doesn't bind can only receive connections redirected from + * other listeners that use the use_orig_dst + * @param use_orig_dst if a connection was redirected to this port using iptables, + * allow the listener to hand it off to the listener associated to the original port * @return Network::ListenerPtr a new listener that is owned by the caller. */ virtual Network::ListenerPtr createListener(Network::ListenSocket& socket, Network::ListenerCallbacks& cb, - Stats::Store& stats_store, bool use_proxy_proto) PURE; + Stats::Store& stats_store, bool bind_to_port, + bool use_proxy_proto, bool use_orig_dst) PURE; /** * Create a listener on a specific port. @@ -85,13 +91,18 @@ class Dispatcher { * @param socket supplies the socket to listen on. * @param cb supplies the callbacks to invoke for listener events. * @param stats_store supplies the Stats::Store to use. + * @param bind_to_port specifies if the listener should actually bind to the port. + * a listener that doesn't bind can only receive connections redirected from + * other listeners that use the use_orig_dst + * @param use_orig_dst if a connection was redirected to this port using iptables, + * allow the listener to hand it off to the listener associated to the original port * @return Network::ListenerPtr a new listener that is owned by the caller. */ virtual Network::ListenerPtr createSslListener(Ssl::ServerContext& ssl_ctx, Network::ListenSocket& socket, Network::ListenerCallbacks& cb, - Stats::Store& stats_store, - bool use_proxy_proto) PURE; + Stats::Store& stats_store, bool bind_to_port, + bool use_proxy_proto, bool use_orig_dst) PURE; /** * Allocate a timer. @see Event::Timer for docs on how to use the timer. diff --git a/include/envoy/server/configuration.h b/include/envoy/server/configuration.h index 654969fc618e..24047dc8d6cd 100644 --- a/include/envoy/server/configuration.h +++ b/include/envoy/server/configuration.h @@ -37,6 +37,19 @@ class Listener { * (http://www.haproxy.org/download/1.5/doc/proxy-protocol.txt) */ virtual bool useProxyProto() 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 was redirected to this listener port using iptables, + * allow the listener to hand it off to the listener associated to the original port + */ + virtual bool useOriginalDst() PURE; }; typedef std::unique_ptr ListenerPtr; diff --git a/source/common/event/dispatcher_impl.cc b/source/common/event/dispatcher_impl.cc index b54897a4b131..83b8de86bb95 100644 --- a/source/common/event/dispatcher_impl.cc +++ b/source/common/event/dispatcher_impl.cc @@ -78,19 +78,19 @@ Filesystem::WatcherPtr DispatcherImpl::createFilesystemWatcher() { Network::ListenerPtr DispatcherImpl::createListener(Network::ListenSocket& socket, Network::ListenerCallbacks& cb, - Stats::Store& stats_store, - bool use_proxy_proto) { - return Network::ListenerPtr{ - new Network::ListenerImpl(*this, socket, cb, stats_store, use_proxy_proto)}; + Stats::Store& stats_store, bool bind_to_port, + bool use_proxy_proto, bool use_orig_dst) { + return Network::ListenerPtr{new Network::ListenerImpl( + *this, socket, cb, stats_store, bind_to_port, use_proxy_proto, use_orig_dst)}; } Network::ListenerPtr DispatcherImpl::createSslListener(Ssl::ServerContext& ssl_ctx, Network::ListenSocket& socket, Network::ListenerCallbacks& cb, - Stats::Store& stats_store, - bool use_proxy_proto) { - return Network::ListenerPtr{ - new Network::SslListenerImpl(*this, ssl_ctx, socket, cb, stats_store, use_proxy_proto)}; + Stats::Store& stats_store, bool bind_to_port, + bool use_proxy_proto, bool use_orig_dst) { + return Network::ListenerPtr{new Network::SslListenerImpl( + *this, ssl_ctx, socket, cb, stats_store, bind_to_port, use_proxy_proto, use_orig_dst)}; } TimerPtr DispatcherImpl::createTimer(TimerCb cb) { return TimerPtr{new TimerImpl(*this, cb)}; } diff --git a/source/common/event/dispatcher_impl.h b/source/common/event/dispatcher_impl.h index 396297275a24..51f82c86ad35 100644 --- a/source/common/event/dispatcher_impl.h +++ b/source/common/event/dispatcher_impl.h @@ -31,10 +31,12 @@ class DispatcherImpl : Logger::Loggable, public Dispatcher { FileEventPtr createFileEvent(int fd, FileReadyCb cb) override; Filesystem::WatcherPtr createFilesystemWatcher() override; Network::ListenerPtr createListener(Network::ListenSocket& socket, Network::ListenerCallbacks& cb, - Stats::Store& stats_store, bool use_proxy_proto) override; + Stats::Store& stats_store, bool bind_to_port, + bool use_proxy_proto, bool use_orig_dst) override; Network::ListenerPtr createSslListener(Ssl::ServerContext& ssl_ctx, Network::ListenSocket& socket, Network::ListenerCallbacks& cb, Stats::Store& stats_store, - bool use_proxy_proto) override; + bool bind_to_port, bool use_proxy_proto, + bool use_orig_dst) override; TimerPtr createTimer(TimerCb cb) override; void deferredDelete(DeferredDeletablePtr&& to_delete) override; void exit() override; diff --git a/source/common/network/listen_socket_impl.cc b/source/common/network/listen_socket_impl.cc index 861fe41208ff..36ce862fd5ef 100644 --- a/source/common/network/listen_socket_impl.cc +++ b/source/common/network/listen_socket_impl.cc @@ -7,7 +7,8 @@ namespace Network { -TcpListenSocket::TcpListenSocket(uint32_t port) : port_(port) { +TcpListenSocket::TcpListenSocket(uint32_t port, bool bindToPort) : port_(port) { + AddrInfoPtr address = Utility::resolveTCP("", port); fd_ = socket(address->ai_addr->sa_family, SOCK_STREAM | SOCK_NONBLOCK, 0); RELEASE_ASSERT(fd_ != -1); @@ -16,10 +17,12 @@ TcpListenSocket::TcpListenSocket(uint32_t port) : port_(port) { int rc = setsockopt(fd_, SOL_SOCKET, SO_REUSEADDR, &on, sizeof(on)); RELEASE_ASSERT(rc != -1); - rc = bind(fd_, address->ai_addr, address->ai_addrlen); - if (rc == -1) { - close(); - throw EnvoyException(fmt::format("cannot bind on port {}: {}", port, strerror(errno))); + if (bindToPort) { + rc = bind(fd_, address->ai_addr, address->ai_addrlen); + if (rc == -1) { + close(); + throw EnvoyException(fmt::format("cannot bind on port {}: {}", port, strerror(errno))); + } } } diff --git a/source/common/network/listen_socket_impl.h b/source/common/network/listen_socket_impl.h index 5058baf48d4d..933d208fd330 100644 --- a/source/common/network/listen_socket_impl.h +++ b/source/common/network/listen_socket_impl.h @@ -27,11 +27,11 @@ class ListenSocketImpl : public ListenSocket { }; /** - * Wraps a bound unix socket. + * Wraps a unix socket. */ class TcpListenSocket : public ListenSocketImpl { public: - TcpListenSocket(uint32_t port); + TcpListenSocket(uint32_t port, bool bindToPort); TcpListenSocket(int fd, uint32_t port) : ListenSocketImpl(fd), port_(port) {} uint32_t port() { return port_; } diff --git a/source/common/network/listener_impl.cc b/source/common/network/listener_impl.cc index 8f604cad36f8..3224e02f67c0 100644 --- a/source/common/network/listener_impl.cc +++ b/source/common/network/listener_impl.cc @@ -9,19 +9,53 @@ #include "common/network/connection_impl.h" #include "common/ssl/connection_impl.h" +#include "server/connection_handler.h" + #include "event2/listener.h" namespace Network { +void ListenerImpl::listenCallback(evconnlistener*, evutil_socket_t fd, sockaddr* addr, int, + void* arg) { + ListenerImpl* listener = static_cast(arg); + + if (listener->use_original_dst_ && listener->connection_handler_ != nullptr) { + struct sockaddr_storage orig_dst_addr; + memset(&orig_dst_addr, 0, sizeof(orig_dst_addr)); + + bool success = Utility::getOriginalDst(fd, &orig_dst_addr); + + if (success) { + std::string orig_sock_name = std::to_string( + listener->getAddressPort(reinterpret_cast(&orig_dst_addr))); + + if (listener->socket_.name() != orig_sock_name) { + ListenerImpl* new_listener = + static_cast(listener->connection_handler_->findListener(orig_sock_name)); + + if (new_listener != nullptr) { + listener = new_listener; + } + } + } + } + + listener->newConnection(fd, addr); +} + ListenerImpl::ListenerImpl(Event::DispatcherImpl& dispatcher, ListenSocket& socket, - ListenerCallbacks& cb, Stats::Store& stats_store, bool use_proxy_proto) - : dispatcher_(dispatcher), cb_(cb), use_proxy_proto_(use_proxy_proto), - proxy_protocol_(stats_store) { - listener_.reset( - evconnlistener_new(&dispatcher_.base(), - [](evconnlistener*, evutil_socket_t fd, sockaddr* addr, int, void* arg) - -> void { static_cast(arg)->newConnection(fd, addr); }, - this, 0, -1, socket.fd())); + ListenerCallbacks& cb, Stats::Store& stats_store, bool bind_to_port, + bool use_proxy_proto, bool use_orig_dst) + : dispatcher_(dispatcher), socket_(socket), cb_(cb), bind_to_port_(bind_to_port), + use_proxy_proto_(use_proxy_proto), proxy_protocol_(stats_store), + use_original_dst_(use_orig_dst), connection_handler_(nullptr) { + + if (bind_to_port_) { + listener_.reset( + evconnlistener_new(&dispatcher_.base(), listenCallback, this, 0, -1, socket.fd())); + } else { + listener_.reset(evconnlistener_new(&dispatcher_.base(), nullptr, this, 0, -1, socket.fd())); + } if (!listener_) { throw CreateListenerException(fmt::format("cannot listen on socket: {}", socket.name())); @@ -70,4 +104,8 @@ const std::string ListenerImpl::getAddressName(sockaddr* addr) { : EMPTY_STRING; } +uint16_t ListenerImpl::getAddressPort(sockaddr* addr) { + return (addr->sa_family == AF_INET) ? ntohs(reinterpret_cast(addr)->sin_port) : 0; +} + } // Network diff --git a/source/common/network/listener_impl.h b/source/common/network/listener_impl.h index a2508e0c6e71..4dd61ef70d00 100644 --- a/source/common/network/listener_impl.h +++ b/source/common/network/listener_impl.h @@ -8,6 +8,10 @@ #include "common/event/dispatcher_impl.h" #include "common/event/libevent.h" +#include "server/connection_handler.h" + +#include "event2/event.h" + namespace Network { /** @@ -16,7 +20,8 @@ namespace Network { class ListenerImpl : public Listener { public: ListenerImpl(Event::DispatcherImpl& dispatcher, ListenSocket& socket, ListenerCallbacks& cb, - Stats::Store& stats_store, bool use_proxy_proto); + Stats::Store& stats_store, bool bind_to_port, bool use_proxy_proto, + bool use_orig_dst); /** * Accept/process a new connection. @@ -32,16 +37,34 @@ class ListenerImpl : public Listener { */ virtual void newConnection(int fd, const std::string& remote_address); + /** + * @return the socket supplied to the listener at construction time + */ + ListenSocket& socket(void) { return socket_; } + + /** + * Set a pointer to the connection handler that handles connections for this listener + * Invoked when the listener becomes active. + * @param conn_handler the connection handler associated to this listener + */ + void connectionHandler(ConnectionHandler* conn_handler) { connection_handler_ = conn_handler; } + protected: const std::string getAddressName(sockaddr* addr); + uint16_t getAddressPort(sockaddr* addr); Event::DispatcherImpl& dispatcher_; + ListenSocket& socket_; ListenerCallbacks& cb_; + bool bind_to_port_; bool use_proxy_proto_; ProxyProtocol proxy_protocol_; + bool use_original_dst_; + ConnectionHandler* connection_handler_; private: static void errorCallback(evconnlistener* listener, void* context); + static void listenCallback(evconnlistener*, evutil_socket_t fd, sockaddr* addr, int, void* arg); Event::Libevent::ListenerPtr listener_; }; @@ -49,8 +72,11 @@ class ListenerImpl : public Listener { class SslListenerImpl : public ListenerImpl { public: SslListenerImpl(Event::DispatcherImpl& dispatcher, Ssl::Context& ssl_ctx, ListenSocket& socket, - ListenerCallbacks& cb, Stats::Store& stats_store, bool use_proxy_proto) - : ListenerImpl(dispatcher, socket, cb, stats_store, use_proxy_proto), ssl_ctx_(ssl_ctx) {} + ListenerCallbacks& cb, Stats::Store& stats_store, bool bind_to_port, + bool use_proxy_proto, bool use_orig_dst) + : ListenerImpl(dispatcher, socket, cb, stats_store, bind_to_port, use_proxy_proto, + use_orig_dst), + ssl_ctx_(ssl_ctx) {} // ListenerImpl void newConnection(int fd, sockaddr* addr) override; diff --git a/source/common/network/utility.cc b/source/common/network/utility.cc index 96b3f6d3181c..9d8e6f349b70 100644 --- a/source/common/network/utility.cc +++ b/source/common/network/utility.cc @@ -193,6 +193,8 @@ std::string Utility::getAddressName(sockaddr_in* addr) { return std::string(str); } +uint16_t Utility::getAddressPort(sockaddr_in* addr) { return ntohs(addr->sin_port); } + bool Utility::isInternalAddress(const char* address) { in_addr addr; int rc = inet_pton(AF_INET, address, &addr); @@ -221,4 +223,12 @@ bool Utility::isLoopbackAddress(const char* address) { return addr.s_addr == htonl(INADDR_LOOPBACK); } +bool Utility::getOriginalDst(int fd, sockaddr_storage* orig_addr) { + + socklen_t addr_len = sizeof(sockaddr_storage); + int status = getsockopt(fd, SOL_IP, SO_ORIGINAL_DST, orig_addr, &addr_len); + + return (status == 0) ? true : false; +} + } // Network diff --git a/source/common/network/utility.h b/source/common/network/utility.h index 4d243e719076..85f5044c729a 100644 --- a/source/common/network/utility.h +++ b/source/common/network/utility.h @@ -8,6 +8,8 @@ #include +#include + namespace Network { /** @@ -100,6 +102,13 @@ class Utility { */ static std::string getAddressName(sockaddr_in* addr); + /** + * Extract port information from a sockaddr_in. + * @param addr the address from which to extract the port number + * @return the port number + */ + static uint16_t getAddressPort(sockaddr_in* addr); + /** * Determine whether this is an internal (RFC1918) address. * @return bool the address is an RFC1918 address. @@ -111,6 +120,16 @@ class Utility { * @return true if so, otherwise false */ static bool isLoopbackAddress(const char* address); + + /** + * Retrieve the original destination address from an accepted fd. + * The address (IP and port) may be not local and the port may differ from + * the listener port if the packets were redirected using iptables + * @param fd is the descriptor returned by accept() + * @param orig_addr is the data structure that contains the original address + * @return true if the operation succeeded, false otherwise + */ + static bool getOriginalDst(int fd, sockaddr_storage* orig_addr); }; } // Network diff --git a/source/server/configuration_impl.cc b/source/server/configuration_impl.cc index 9588273fc684..6a27e53862ec 100644 --- a/source/server/configuration_impl.cc +++ b/source/server/configuration_impl.cc @@ -123,10 +123,20 @@ MainImpl::ListenerConfig::ListenerConfig(MainImpl& parent, Json::Object& json) fmt::format("listener.{}.", port_), parent_.server_.stats(), context_config); } + if (json.hasObject("bind_to_port")) { + bind_to_port_ = json.getBoolean("bind_to_port"); + } else { + bind_to_port_ = true; + } + if (json.hasObject("use_proxy_proto")) { use_proxy_proto_ = json.getBoolean("use_proxy_proto"); } + if (json.hasObject("use_original_dst")) { + use_original_dst_ = json.getBoolean("use_original_dst"); + } + std::vector filters = json.getObjectArray("filters"); for (size_t i = 0; i < filters.size(); i++) { std::string string_type = filters[i]->getString("type"); diff --git a/source/server/configuration_impl.h b/source/server/configuration_impl.h index 84747665a347..673bf63e5e15 100644 --- a/source/server/configuration_impl.h +++ b/source/server/configuration_impl.h @@ -92,7 +92,9 @@ class MainImpl : Logger::Loggable, public Main { Network::FilterChainFactory& filterChainFactory() override { return *this; } uint64_t port() override { return port_; } Ssl::ServerContext* sslContext() override { return ssl_context_; } + bool bindToPort() override { return bind_to_port_; } bool useProxyProto() override { return use_proxy_proto_; } + bool useOriginalDst() override { return use_original_dst_; } // Network::FilterChainFactory void createFilterChain(Network::Connection& connection) override; @@ -101,7 +103,9 @@ class MainImpl : Logger::Loggable, public Main { MainImpl& parent_; uint64_t port_; Ssl::ServerContext* ssl_context_{}; + bool bind_to_port_{}; bool use_proxy_proto_{}; + bool use_original_dst_{}; std::list filter_factories_; }; diff --git a/source/server/connection_handler.cc b/source/server/connection_handler.cc index a6f52a454de2..7a3af8daf127 100644 --- a/source/server/connection_handler.cc +++ b/source/server/connection_handler.cc @@ -4,6 +4,9 @@ #include "envoy/event/timer.h" #include "envoy/network/filter.h" +#include "common/network/listener_impl.h" +#include "common/event/dispatcher_impl.h" + ConnectionHandler::ConnectionHandler(Stats::Store& stats_store, spdlog::logger& logger, Api::ApiPtr&& api) : stats_store_(stats_store), logger_(logger), api_(std::move(api)), @@ -14,14 +17,19 @@ ConnectionHandler::ConnectionHandler(Stats::Store& stats_store, spdlog::logger& ConnectionHandler::~ConnectionHandler() { closeConnections(); } void ConnectionHandler::addListener(Network::FilterChainFactory& factory, - Network::ListenSocket& socket, bool use_proxy_proto) { - listeners_.emplace_back(new ActiveListener(*this, socket, factory, use_proxy_proto)); + Network::ListenSocket& socket, bool bind_to_port, + bool use_proxy_proto, bool use_orig_dst) { + ActiveListenerPtr l( + new ActiveListener(*this, socket, factory, bind_to_port, use_proxy_proto, use_orig_dst)); + listeners_.insert(std::make_pair(socket.name(), std::move(l))); } void ConnectionHandler::addSslListener(Network::FilterChainFactory& factory, Ssl::ServerContext& ssl_ctx, Network::ListenSocket& socket, - bool use_proxy_proto) { - listeners_.emplace_back(new SslActiveListener(*this, ssl_ctx, socket, factory, use_proxy_proto)); + bool bind_to_port, bool use_proxy_proto, bool use_orig_dst) { + ActiveListenerPtr l(new SslActiveListener(*this, ssl_ctx, socket, factory, bind_to_port, + use_proxy_proto, use_orig_dst)); + listeners_.insert(std::make_pair(socket.name(), std::move(l))); } void ConnectionHandler::closeConnections() { @@ -33,8 +41,8 @@ void ConnectionHandler::closeConnections() { } void ConnectionHandler::closeListeners() { - for (ActiveListenerPtr& l : listeners_) { - l->listener_.reset(); + for (auto& l : listeners_) { + l.second->listener_.reset(); } } @@ -48,10 +56,17 @@ void ConnectionHandler::removeConnection(ActiveConnection& connection) { ConnectionHandler::ActiveListener::ActiveListener(ConnectionHandler& parent, Network::ListenSocket& socket, Network::FilterChainFactory& factory, - bool use_proxy_proto) + bool bind_to_port, bool use_proxy_proto, + bool use_orig_dst) : ActiveListener(parent, parent.dispatcher_->createListener(socket, *this, parent.stats_store_, - use_proxy_proto), - factory, socket.name()) {} + bind_to_port, use_proxy_proto, + use_orig_dst), + factory, socket.name()) { + + if (use_orig_dst) { + static_cast(listener_.get())->connectionHandler(&parent); + } +} ConnectionHandler::ActiveListener::ActiveListener(ConnectionHandler& parent, Network::ListenerPtr&& listener, @@ -65,10 +80,21 @@ ConnectionHandler::SslActiveListener::SslActiveListener(ConnectionHandler& paren Ssl::ServerContext& ssl_ctx, Network::ListenSocket& socket, Network::FilterChainFactory& factory, - bool use_proxy_proto) + bool bind_to_port, bool use_proxy_proto, + bool use_orig_dst) : ActiveListener(parent, parent.dispatcher_->createSslListener( - ssl_ctx, socket, *this, parent.stats_store_, use_proxy_proto), - factory, socket.name()) {} + ssl_ctx, socket, *this, parent.stats_store_, bind_to_port, + use_proxy_proto, use_orig_dst), + factory, socket.name()) { + if (use_orig_dst) { + static_cast(listener_.get())->connectionHandler(&parent); + } +} + +Network::Listener* ConnectionHandler::findListener(const std::string& socket_name) { + auto l = listeners_.find(socket_name); + return (l != listeners_.end()) ? l->second->listener_.get() : nullptr; +} void ConnectionHandler::ActiveListener::onNewConnection(Network::ConnectionPtr&& new_connection) { conn_log(parent_.logger_, info, "new connection", *new_connection); diff --git a/source/server/connection_handler.h b/source/server/connection_handler.h index 2408af1679b9..6af7ce287fe7 100644 --- a/source/server/connection_handler.h +++ b/source/server/connection_handler.h @@ -44,21 +44,40 @@ class ConnectionHandler final : NonCopyable { * Adds listener to the handler. * @param factory supplies the configuration factory for new connections. * @param socket supplies the already bound socket to listen on. + * @param bind_to_port specifies if the listener should actually bind to the port. + * a listener that doesn't bind can only receive connections redirected from + * other listeners that use the use_orig_dst * @param use_proxy_proto whether to use the PROXY Protocol V1 * (http://www.haproxy.org/download/1.5/doc/proxy-protocol.txt) + * @param use_orig_dst if a connection was redirected to this port using iptables, + * allow the listener to hand it off to the listener associated to the original port */ void addListener(Network::FilterChainFactory& factory, Network::ListenSocket& socket, - bool use_proxy_proto); + bool bind_to_port, bool use_proxy_proto, bool use_orig_dst); /** * Adds listener to the handler. * @param factory supplies the configuration factory for new connections. * @param socket supplies the already bound socket to listen on. + * @param bind_to_port specifies if the listener should actually bind to the port. + * a listener that doesn't bind can only receive connections redirected from + * other listeners that use the use_orig_dst * @param use_proxy_proto whether to use the PROXY Protocol V1 * (http://www.haproxy.org/download/1.5/doc/proxy-protocol.txt) + * @param use_orig_dst if a connection was redirected to this port using iptables, + * allow the listener to hand it off to the listener associated to the original port */ void addSslListener(Network::FilterChainFactory& factory, Ssl::ServerContext& ssl_ctx, - Network::ListenSocket& socket, bool use_proxy_proto); + Network::ListenSocket& socket, bool bind_to_port, bool use_proxy_proto, + bool use_orig_dst); + + /** + * Find a listener based on the provided socket name + * @param name supplies the name of the socket + * @return a pointer to the listener or nullptr if not found. + * Ownership of the listener is NOT transferred + */ + Network::Listener* findListener(const std::string& socket_name); /** * Close and destroy all connections. This must be called from the same thread that is running @@ -84,7 +103,8 @@ class ConnectionHandler final : NonCopyable { */ struct ActiveListener : public Network::ListenerCallbacks { ActiveListener(ConnectionHandler& parent, Network::ListenSocket& socket, - Network::FilterChainFactory& factory, bool use_proxy_proto); + Network::FilterChainFactory& factory, bool use_proxy_proto, bool bind_to_port, + bool use_orig_dst); ActiveListener(ConnectionHandler& parent, Network::ListenerPtr&& listener, Network::FilterChainFactory& factory, const std::string& stats_prefix); @@ -104,7 +124,7 @@ class ConnectionHandler final : NonCopyable { struct SslActiveListener : public ActiveListener { SslActiveListener(ConnectionHandler& parent, Ssl::ServerContext& ssl_ctx, Network::ListenSocket& socket, Network::FilterChainFactory& factory, - bool use_proxy_proto); + bool use_proxy_proto, bool bind_to_port, bool use_orig_dst); }; typedef std::unique_ptr ActiveListenerPtr; @@ -148,7 +168,7 @@ class ConnectionHandler final : NonCopyable { spdlog::logger& logger_; Api::ApiPtr api_; Event::DispatcherPtr dispatcher_; - std::list listeners_; + std::map listeners_; std::list connections_; std::atomic num_connections_{}; Stats::Counter& watchdog_miss_counter_; diff --git a/source/server/http/admin.cc b/source/server/http/admin.cc index 98834aa6266b..bee5af6bf8c1 100644 --- a/source/server/http/admin.cc +++ b/source/server/http/admin.cc @@ -291,7 +291,7 @@ void AdminFilter::onComplete() { } AdminImpl::AdminImpl(const std::string& access_log_path, uint32_t port, Server::Instance& server) - : server_(server), socket_(new Network::TcpListenSocket(port)), + : server_(server), socket_(new Network::TcpListenSocket(port, true)), stats_(Http::ConnectionManagerImpl::generateStats("http.admin.", server_.stats())), route_config_(new Router::NullConfigImpl()), handlers_{ diff --git a/source/server/server.cc b/source/server/server.cc index 456d73432668..559d31d57315 100644 --- a/source/server/server.cc +++ b/source/server/server.cc @@ -141,7 +141,7 @@ void InstanceImpl::initialize(Options& options, TestHooks& hooks, original_start_time_ = info.original_start_time_; admin_.reset( new AdminImpl(initial_config.admin().accessLogPath(), initial_config.admin().port(), *this)); - handler_.addListener(*admin_, admin_->socket(), false); + handler_.addListener(*admin_, admin_->socket(), true, false, false); loadServerFlags(initial_config.flagsPath()); @@ -178,7 +178,8 @@ void InstanceImpl::initialize(Options& options, TestHooks& hooks, log().info("obtained socket for port {} from parent", listener->port()); socket_map_[listener.get()].reset(new Network::TcpListenSocket(fd, listener->port())); } else { - socket_map_[listener.get()].reset(new Network::TcpListenSocket(listener->port())); + socket_map_[listener.get()].reset( + new Network::TcpListenSocket(listener->port(), listener->bindToPort())); } } diff --git a/source/server/worker.cc b/source/server/worker.cc index b514d78af2cf..dc976d745a6f 100644 --- a/source/server/worker.cc +++ b/source/server/worker.cc @@ -20,13 +20,16 @@ Worker::~Worker() {} void Worker::initializeConfiguration(Server::Configuration::Main& config, const SocketMap& socket_map) { for (const Server::Configuration::ListenerPtr& listener : config.listeners()) { + bool bind_to_port = listener->bindToPort(); bool use_proxy_proto = listener->useProxyProto(); + bool use_orig_dst = listener->useOriginalDst(); if (listener->sslContext()) { handler_->addSslListener(listener->filterChainFactory(), *listener->sslContext(), - *socket_map.at(listener.get()), use_proxy_proto); + *socket_map.at(listener.get()), bind_to_port, use_proxy_proto, + use_orig_dst); } else { handler_->addListener(listener->filterChainFactory(), *socket_map.at(listener.get()), - use_proxy_proto); + bind_to_port, use_proxy_proto, use_orig_dst); } } diff --git a/test/common/network/connection_impl_test.cc b/test/common/network/connection_impl_test.cc index 12b746bc02e1..68f4b83e23fd 100644 --- a/test/common/network/connection_impl_test.cc +++ b/test/common/network/connection_impl_test.cc @@ -59,10 +59,10 @@ struct MockBufferStats { TEST(ConnectionImplTest, BufferStats) { Stats::IsolatedStoreImpl stats_store; Event::DispatcherImpl dispatcher; - Network::TcpListenSocket socket(10000); + Network::TcpListenSocket socket(uint32_t(10000), true); Network::MockListenerCallbacks listener_callbacks; Network::ListenerPtr listener = - dispatcher.createListener(socket, listener_callbacks, stats_store, false); + dispatcher.createListener(socket, listener_callbacks, stats_store, true, false, false); Network::ClientConnectionPtr client_connection = dispatcher.createClientConnection("tcp://127.0.0.1:10000"); diff --git a/test/common/network/listen_socket_impl_test.cc b/test/common/network/listen_socket_impl_test.cc index 70ad7bc3157b..6bbad1b5ebb5 100644 --- a/test/common/network/listen_socket_impl_test.cc +++ b/test/common/network/listen_socket_impl_test.cc @@ -5,11 +5,11 @@ namespace Network { TEST(ListenSocket, All) { - TcpListenSocket socket1(15000); + TcpListenSocket socket1(uint32_t(15000), true); listen(socket1.fd(), 0); EXPECT_EQ(15000U, socket1.port()); - EXPECT_THROW(Network::TcpListenSocket socket2(15000), EnvoyException); + EXPECT_THROW(Network::TcpListenSocket socket2(uint32_t(15000), true), EnvoyException); TcpListenSocket socket2(dup(socket1.fd()), 15000); EXPECT_EQ(15000U, socket2.port()); diff --git a/test/common/network/listener_impl_test.cc b/test/common/network/listener_impl_test.cc index 5a2938bd1aed..27eb9b9ce867 100644 --- a/test/common/network/listener_impl_test.cc +++ b/test/common/network/listener_impl_test.cc @@ -13,10 +13,10 @@ static void errorCallbackTest() { // test in the forked process to avoid confusion when the fork happens. Stats::IsolatedStoreImpl stats_store; Event::DispatcherImpl dispatcher; - Network::TcpListenSocket socket(10000); + Network::TcpListenSocket socket(uint32_t(10000), true); Network::MockListenerCallbacks listener_callbacks; Network::ListenerPtr listener = - dispatcher.createListener(socket, listener_callbacks, stats_store, false); + dispatcher.createListener(socket, listener_callbacks, stats_store, true, false, false); Network::ClientConnectionPtr client_connection = dispatcher.createClientConnection("tcp://127.0.0.1:10000"); diff --git a/test/common/network/proxy_protocol_test.cc b/test/common/network/proxy_protocol_test.cc index a5b4dc2f79ae..4a53b5c88fef 100644 --- a/test/common/network/proxy_protocol_test.cc +++ b/test/common/network/proxy_protocol_test.cc @@ -15,7 +15,8 @@ namespace Network { class ProxyProtocolTest : public testing::Test { public: ProxyProtocolTest() - : socket_(1234), listener_(dispatcher_, socket_, callbacks_, stats_store_, true) { + : socket_(uint32_t(1234), true), + listener_(dispatcher_, socket_, callbacks_, stats_store_, true, true, false) { conn_ = dispatcher_.createClientConnection("tcp://127.0.0.1:1234"); conn_->addConnectionCallbacks(connection_callbacks_); conn_->connect(); diff --git a/test/common/ssl/connection_impl_test.cc b/test/common/ssl/connection_impl_test.cc index cdac48668d84..3a8c03fc42b1 100644 --- a/test/common/ssl/connection_impl_test.cc +++ b/test/common/ssl/connection_impl_test.cc @@ -31,10 +31,10 @@ TEST(SslConnectionImplTest, ClientAuth) { ServerContextImpl server_ctx("server_ctx", stats_store, server_ctx_config, runtime); Event::DispatcherImpl dispatcher; - Network::TcpListenSocket socket(10000); + Network::TcpListenSocket socket(uint32_t(10000), true); Network::MockListenerCallbacks callbacks; Network::ListenerPtr listener = - dispatcher.createSslListener(server_ctx, socket, callbacks, stats_store, false); + dispatcher.createSslListener(server_ctx, socket, callbacks, stats_store, true, false, false); std::string client_ctx_json = R"EOF( { @@ -89,10 +89,10 @@ TEST(SslConnectionImplTest, ClientAuthBadVerification) { ServerContextImpl server_ctx("server_ctx", stats_store, server_ctx_config, runtime); Event::DispatcherImpl dispatcher; - Network::TcpListenSocket socket(10000); + Network::TcpListenSocket socket(uint32_t(10000), true); Network::MockListenerCallbacks callbacks; Network::ListenerPtr listener = - dispatcher.createSslListener(server_ctx, socket, callbacks, stats_store, false); + dispatcher.createSslListener(server_ctx, socket, callbacks, stats_store, true, false, false); std::string client_ctx_json = R"EOF( { @@ -143,10 +143,10 @@ TEST(SslConnectionImplTest, SslError) { ServerContextImpl server_ctx("server_ctx", stats_store, server_ctx_config, runtime); Event::DispatcherImpl dispatcher; - Network::TcpListenSocket socket(10000); + Network::TcpListenSocket socket(uint32_t(10000), true); Network::MockListenerCallbacks callbacks; Network::ListenerPtr listener = - dispatcher.createSslListener(server_ctx, socket, callbacks, stats_store, false); + dispatcher.createSslListener(server_ctx, socket, callbacks, stats_store, true, false, false); Network::ClientConnectionPtr client_connection = dispatcher.createClientConnection("tcp://127.0.0.1:10000"); diff --git a/test/integration/fake_upstream.cc b/test/integration/fake_upstream.cc index ccc49c7f57df..83b153a9c49c 100644 --- a/test/integration/fake_upstream.cc +++ b/test/integration/fake_upstream.cc @@ -186,13 +186,15 @@ FakeUpstream::FakeUpstream(const std::string& uds_path, FakeHttpConnection::Type } FakeUpstream::FakeUpstream(uint32_t port, FakeHttpConnection::Type type) - : FakeUpstream(nullptr, Network::ListenSocketPtr{new Network::TcpListenSocket(port)}, type) { + : FakeUpstream(nullptr, Network::ListenSocketPtr{new Network::TcpListenSocket(port, true)}, + type) { log().info("starting fake server on port {}", port); } FakeUpstream::FakeUpstream(Ssl::ServerContext* ssl_ctx, uint32_t port, FakeHttpConnection::Type type) - : FakeUpstream(ssl_ctx, Network::ListenSocketPtr{new Network::TcpListenSocket(port)}, type) { + : FakeUpstream(ssl_ctx, Network::ListenSocketPtr{new Network::TcpListenSocket(port, true)}, + type) { log().info("starting fake SSL server on port {}", port); } @@ -219,9 +221,9 @@ void FakeUpstream::createFilterChain(Network::Connection& connection) { void FakeUpstream::threadRoutine() { if (ssl_ctx_) { - handler_.addSslListener(*this, *ssl_ctx_, *socket_, false); + handler_.addSslListener(*this, *ssl_ctx_, *socket_, true, false, false); } else { - handler_.addListener(*this, *socket_, false); + handler_.addListener(*this, *socket_, true, false, false); } server_initialized_.setReady(); diff --git a/test/mocks/event/mocks.h b/test/mocks/event/mocks.h index cbcd345ffcbc..3a3b9a677d7c 100644 --- a/test/mocks/event/mocks.h +++ b/test/mocks/event/mocks.h @@ -39,15 +39,18 @@ class MockDispatcher : public Dispatcher { } Network::ListenerPtr createListener(Network::ListenSocket& socket, Network::ListenerCallbacks& cb, - Stats::Store& stats_store, bool use_proxy_proto) override { - return Network::ListenerPtr{createListener_(socket, cb, stats_store, use_proxy_proto)}; + Stats::Store& stats_store, bool bind_to_port, + bool use_proxy_proto, bool use_original_dst) override { + return Network::ListenerPtr{ + createListener_(socket, cb, stats_store, bind_to_port, use_proxy_proto, use_original_dst)}; } Network::ListenerPtr createSslListener(Ssl::ServerContext& ssl_ctx, Network::ListenSocket& socket, Network::ListenerCallbacks& cb, Stats::Store& stats_store, - bool use_proxy_proto) override { - return Network::ListenerPtr{ - createSslListener_(ssl_ctx, socket, cb, stats_store, use_proxy_proto)}; + bool bind_to_port, bool use_proxy_proto, + bool use_original_dst) override { + return Network::ListenerPtr{createSslListener_(ssl_ctx, socket, cb, stats_store, bind_to_port, + use_proxy_proto, use_original_dst)}; } TimerPtr createTimer(TimerCb cb) override { return TimerPtr{createTimer_(cb)}; } @@ -71,13 +74,14 @@ class MockDispatcher : public Dispatcher { MOCK_METHOD0(createDnsResolver_, Network::DnsResolver*()); MOCK_METHOD2(createFileEvent_, FileEvent*(int fd, FileReadyCb cb)); MOCK_METHOD0(createFilesystemWatcher_, Filesystem::Watcher*()); - MOCK_METHOD4(createListener_, + MOCK_METHOD6(createListener_, Network::Listener*(Network::ListenSocket& socket, Network::ListenerCallbacks& cb, - Stats::Store& stats_store, bool use_proxy_proto)); - MOCK_METHOD5(createSslListener_, + Stats::Store& stats_store, bool bind_to_port, + bool use_proxy_proto, bool use_original_dst)); + MOCK_METHOD7(createSslListener_, Network::Listener*(Ssl::ServerContext& ssl_ctx, Network::ListenSocket& socket, Network::ListenerCallbacks& cb, Stats::Store& stats_store, - bool use_proxy_proto)); + bool bind_to_port, bool use_proxy_proto, bool use_original_dst)); MOCK_METHOD1(createTimer_, Timer*(TimerCb cb)); MOCK_METHOD1(deferredDelete_, void(DeferredDeletablePtr& to_delete)); MOCK_METHOD0(exit, void()); diff --git a/test/server/connection_handler_test.cc b/test/server/connection_handler_test.cc index 923fcd112e24..698647c51501 100644 --- a/test/server/connection_handler_test.cc +++ b/test/server/connection_handler_test.cc @@ -25,14 +25,14 @@ TEST_F(ConnectionHandlerTest, CloseDuringFilterChainCreate) { Network::Listener* listener = new Network::MockListener(); Network::ListenerCallbacks* listener_callbacks; - EXPECT_CALL(*dispatcher, createListener_(_, _, _, _)) + EXPECT_CALL(*dispatcher, createListener_(_, _, _, _, _, _)) .WillOnce(Invoke([&](Network::ListenSocket&, Network::ListenerCallbacks& cb, Stats::Store&, - bool) -> Network::Listener* { + bool, bool, bool) -> Network::Listener* { listener_callbacks = &cb; return listener; })); - handler.addListener(factory, socket, false); + handler.addListener(factory, socket, true, false, false); Network::MockConnection* connection = new NiceMock(); EXPECT_CALL(factory, createFilterChain(_)); From ed44be253cc81682e99339e3bdab4af9acde98e5 Mon Sep 17 00:00:00 2001 From: Enrico Schiattarella Date: Mon, 9 Jan 2017 11:20:11 -0800 Subject: [PATCH 02/22] Incorporate review comments (except ConnectionHandler interface) --- docs/configuration/listeners/listeners.rst | 16 +++++++++++++++- include/envoy/event/dispatcher.h | 10 ++++++---- include/envoy/server/configuration.h | 4 ++-- source/common/network/listen_socket_impl.cc | 4 ++-- source/common/network/listen_socket_impl.h | 2 +- source/common/network/listener_impl.cc | 20 +++++++++++--------- source/common/network/listener_impl.h | 6 +++--- source/common/network/utility.cc | 2 +- source/server/configuration_impl.cc | 16 +++------------- source/server/connection_handler.h | 4 ++-- 10 files changed, 46 insertions(+), 38 deletions(-) diff --git a/docs/configuration/listeners/listeners.rst b/docs/configuration/listeners/listeners.rst index fa68bbfe29ec..406333d80588 100644 --- a/docs/configuration/listeners/listeners.rst +++ b/docs/configuration/listeners/listeners.rst @@ -12,7 +12,9 @@ Each individual listener configuration has the following format: "port": "...", "filters": [], "ssl_context": "{...}", - "use_proxy_proto": "..." + "bind_to_port": "...", + "use_proxy_proto": "...", + "use_original_dst": "..." } port @@ -28,6 +30,11 @@ port *(optional, object)* The :ref:`TLS ` context configuration for a TLS listener. If no TLS context block is defined, the listener is a plain text listener. +bind_to_port + *(optional, boolean)* Whether the listener should bind to the port. A listener that doesn't bind + can only receive connections redirected from other listeners that set the use_origin_dst to + true. Default is true. + use_proxy_proto *(optional, boolean)* Whether the listener should expect a `PROXY protocol V1 `_ header on new @@ -36,6 +43,13 @@ use_proxy_proto this option. If the option is absent or set to false, Envoy will use the physical peer address of the connection as the remote address. +use_original_dst + *(optional, boolean)* If a connection is redirected using *iptables*, the port on which the proxy + receives it might be different from the original destination port. When this flag is set to true, + the listener hands off redirected connections to the listener associated with the original + destination port. If there is no listener associated with the original destination port, the + connection is handled by the listener that receives it. Default is false. + .. toctree:: :hidden: diff --git a/include/envoy/event/dispatcher.h b/include/envoy/event/dispatcher.h index bb2f40025fcf..8673ddaef594 100644 --- a/include/envoy/event/dispatcher.h +++ b/include/envoy/event/dispatcher.h @@ -71,11 +71,11 @@ class Dispatcher { * @param socket supplies the socket to listen on. * @param cb supplies the callbacks to invoke for listener events. * @param stats_store supplies the Stats::Store to use. - * @param use_proxy_proto whether to use the PROXY Protocol V1 - * (http://www.haproxy.org/download/1.5/doc/proxy-protocol.txt) * @param bind_to_port specifies if the listener should actually bind to the port. * a listener that doesn't bind can only receive connections redirected from - * other listeners that use the use_orig_dst + * other listeners that that set use_origin_dst to true + * @param use_proxy_proto whether to use the PROXY Protocol V1 + * (http://www.haproxy.org/download/1.5/doc/proxy-protocol.txt) * @param use_orig_dst if a connection was redirected to this port using iptables, * allow the listener to hand it off to the listener associated to the original port * @return Network::ListenerPtr a new listener that is owned by the caller. @@ -93,7 +93,9 @@ class Dispatcher { * @param stats_store supplies the Stats::Store to use. * @param bind_to_port specifies if the listener should actually bind to the port. * a listener that doesn't bind can only receive connections redirected from - * other listeners that use the use_orig_dst + * other listeners that set use_origin_dst to true + * @param use_proxy_proto whether to use the PROXY Protocol V1 + * (http://www.haproxy.org/download/1.5/doc/proxy-protocol.txt) * @param use_orig_dst if a connection was redirected to this port using iptables, * allow the listener to hand it off to the listener associated to the original port * @return Network::ListenerPtr a new listener that is owned by the caller. diff --git a/include/envoy/server/configuration.h b/include/envoy/server/configuration.h index 24047dc8d6cd..82680760a6f3 100644 --- a/include/envoy/server/configuration.h +++ b/include/envoy/server/configuration.h @@ -40,8 +40,8 @@ class Listener { /** * @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 + * A listener that doesn't listen on a port can only receive connections + * redirected from other listeners. */ virtual bool bindToPort() PURE; diff --git a/source/common/network/listen_socket_impl.cc b/source/common/network/listen_socket_impl.cc index 36ce862fd5ef..848ed0d0ff24 100644 --- a/source/common/network/listen_socket_impl.cc +++ b/source/common/network/listen_socket_impl.cc @@ -7,7 +7,7 @@ namespace Network { -TcpListenSocket::TcpListenSocket(uint32_t port, bool bindToPort) : port_(port) { +TcpListenSocket::TcpListenSocket(uint32_t port, bool bind_to_port) : port_(port) { AddrInfoPtr address = Utility::resolveTCP("", port); fd_ = socket(address->ai_addr->sa_family, SOCK_STREAM | SOCK_NONBLOCK, 0); @@ -17,7 +17,7 @@ TcpListenSocket::TcpListenSocket(uint32_t port, bool bindToPort) : port_(port) { int rc = setsockopt(fd_, SOL_SOCKET, SO_REUSEADDR, &on, sizeof(on)); RELEASE_ASSERT(rc != -1); - if (bindToPort) { + if (bind_to_port) { rc = bind(fd_, address->ai_addr, address->ai_addrlen); if (rc == -1) { close(); diff --git a/source/common/network/listen_socket_impl.h b/source/common/network/listen_socket_impl.h index 933d208fd330..015f1d6d747d 100644 --- a/source/common/network/listen_socket_impl.h +++ b/source/common/network/listen_socket_impl.h @@ -31,7 +31,7 @@ class ListenSocketImpl : public ListenSocket { */ class TcpListenSocket : public ListenSocketImpl { public: - TcpListenSocket(uint32_t port, bool bindToPort); + TcpListenSocket(uint32_t port, bool bind_to_port); TcpListenSocket(int fd, uint32_t port) : ListenSocketImpl(fd), port_(port) {} uint32_t port() { return port_; } diff --git a/source/common/network/listener_impl.cc b/source/common/network/listener_impl.cc index 3224e02f67c0..4bfdec667d36 100644 --- a/source/common/network/listener_impl.cc +++ b/source/common/network/listener_impl.cc @@ -20,7 +20,7 @@ void ListenerImpl::listenCallback(evconnlistener*, evutil_socket_t fd, sockaddr* ListenerImpl* listener = static_cast(arg); if (listener->use_original_dst_ && listener->connection_handler_ != nullptr) { - struct sockaddr_storage orig_dst_addr; + sockaddr_storage orig_dst_addr; memset(&orig_dst_addr, 0, sizeof(orig_dst_addr)); bool success = Utility::getOriginalDst(fd, &orig_dst_addr); @@ -29,6 +29,10 @@ void ListenerImpl::listenCallback(evconnlistener*, evutil_socket_t fd, sockaddr* std::string orig_sock_name = std::to_string( listener->getAddressPort(reinterpret_cast(&orig_dst_addr))); + // A listener that has the use_original_dst flag set to true can still receive connections + // that are NOT redirected using iptables. If a connection was not redirected, + // the address and port returned by getOriginalDst() match the listener port. + // In this case the listener handles the connection directly and does not hand it off. if (listener->socket_.name() != orig_sock_name) { ListenerImpl* new_listener = static_cast(listener->connection_handler_->findListener(orig_sock_name)); @@ -48,20 +52,18 @@ ListenerImpl::ListenerImpl(Event::DispatcherImpl& dispatcher, ListenSocket& sock bool use_proxy_proto, bool use_orig_dst) : dispatcher_(dispatcher), socket_(socket), cb_(cb), bind_to_port_(bind_to_port), use_proxy_proto_(use_proxy_proto), proxy_protocol_(stats_store), - use_original_dst_(use_orig_dst), connection_handler_(nullptr) { + use_original_dst_(use_orig_dst), connection_handler_(nullptr), listener_(nullptr) { if (bind_to_port_) { listener_.reset( evconnlistener_new(&dispatcher_.base(), listenCallback, this, 0, -1, socket.fd())); - } else { - listener_.reset(evconnlistener_new(&dispatcher_.base(), nullptr, this, 0, -1, socket.fd())); - } - if (!listener_) { - throw CreateListenerException(fmt::format("cannot listen on socket: {}", socket.name())); - } + if (!listener_) { + throw CreateListenerException(fmt::format("cannot listen on socket: {}", socket.name())); + } - evconnlistener_set_error_cb(listener_.get(), errorCallback); + evconnlistener_set_error_cb(listener_.get(), errorCallback); + } } void ListenerImpl::errorCallback(evconnlistener*, void*) { diff --git a/source/common/network/listener_impl.h b/source/common/network/listener_impl.h index 4dd61ef70d00..68f3cdbff72f 100644 --- a/source/common/network/listener_impl.h +++ b/source/common/network/listener_impl.h @@ -8,10 +8,10 @@ #include "common/event/dispatcher_impl.h" #include "common/event/libevent.h" -#include "server/connection_handler.h" - #include "event2/event.h" +class ConnectionHandler; + namespace Network { /** @@ -40,7 +40,7 @@ class ListenerImpl : public Listener { /** * @return the socket supplied to the listener at construction time */ - ListenSocket& socket(void) { return socket_; } + ListenSocket& socket() { return socket_; } /** * Set a pointer to the connection handler that handles connections for this listener diff --git a/source/common/network/utility.cc b/source/common/network/utility.cc index 9d8e6f349b70..2ad331910853 100644 --- a/source/common/network/utility.cc +++ b/source/common/network/utility.cc @@ -228,7 +228,7 @@ bool Utility::getOriginalDst(int fd, sockaddr_storage* orig_addr) { socklen_t addr_len = sizeof(sockaddr_storage); int status = getsockopt(fd, SOL_IP, SO_ORIGINAL_DST, orig_addr, &addr_len); - return (status == 0) ? true : false; + return (status == 0); } } // Network diff --git a/source/server/configuration_impl.cc b/source/server/configuration_impl.cc index 6a27e53862ec..220151349394 100644 --- a/source/server/configuration_impl.cc +++ b/source/server/configuration_impl.cc @@ -123,19 +123,9 @@ MainImpl::ListenerConfig::ListenerConfig(MainImpl& parent, Json::Object& json) fmt::format("listener.{}.", port_), parent_.server_.stats(), context_config); } - if (json.hasObject("bind_to_port")) { - bind_to_port_ = json.getBoolean("bind_to_port"); - } else { - bind_to_port_ = true; - } - - if (json.hasObject("use_proxy_proto")) { - use_proxy_proto_ = json.getBoolean("use_proxy_proto"); - } - - if (json.hasObject("use_original_dst")) { - use_original_dst_ = json.getBoolean("use_original_dst"); - } + bind_to_port_ = json.getBoolean("bind_to_port", true); + use_proxy_proto_ = json.getBoolean("use_proxy_proto", false); + use_original_dst_ = json.getBoolean("use_original_dst", false); std::vector filters = json.getObjectArray("filters"); for (size_t i = 0; i < filters.size(); i++) { diff --git a/source/server/connection_handler.h b/source/server/connection_handler.h index 6af7ce287fe7..bd0810d26ff2 100644 --- a/source/server/connection_handler.h +++ b/source/server/connection_handler.h @@ -46,7 +46,7 @@ class ConnectionHandler final : NonCopyable { * @param socket supplies the already bound socket to listen on. * @param bind_to_port specifies if the listener should actually bind to the port. * a listener that doesn't bind can only receive connections redirected from - * other listeners that use the use_orig_dst + * other listeners that set use_origin_dst to true * @param use_proxy_proto whether to use the PROXY Protocol V1 * (http://www.haproxy.org/download/1.5/doc/proxy-protocol.txt) * @param use_orig_dst if a connection was redirected to this port using iptables, @@ -61,7 +61,7 @@ class ConnectionHandler final : NonCopyable { * @param socket supplies the already bound socket to listen on. * @param bind_to_port specifies if the listener should actually bind to the port. * a listener that doesn't bind can only receive connections redirected from - * other listeners that use the use_orig_dst + * other listeners that set use_origin_dst to true * @param use_proxy_proto whether to use the PROXY Protocol V1 * (http://www.haproxy.org/download/1.5/doc/proxy-protocol.txt) * @param use_orig_dst if a connection was redirected to this port using iptables, From 60e72a6a7f356b816bc592988658ba1c651464f8 Mon Sep 17 00:00:00 2001 From: Enrico Schiattarella Date: Mon, 9 Jan 2017 17:40:11 -0800 Subject: [PATCH 03/22] Create an interface for ConnectionHandler --- include/envoy/event/dispatcher.h | 9 +- include/envoy/server/connection_handler.h | 68 ++++++++++++ source/common/event/dispatcher_impl.cc | 13 ++- source/common/event/dispatcher_impl.h | 7 +- source/common/network/listener_impl.cc | 19 ++-- source/common/network/listener_impl.h | 29 ++--- source/server/CMakeLists.txt | 2 +- ..._handler.cc => connection_handler_impl.cc} | 101 +++++++++--------- ...on_handler.h => connection_handler_impl.h} | 72 +++++-------- source/server/server.h | 4 +- source/server/worker.cc | 2 +- source/server/worker.h | 6 +- test/common/network/connection_impl_test.cc | 6 +- test/common/network/listener_impl_test.cc | 6 +- test/common/network/proxy_protocol_test.cc | 6 +- test/common/ssl/connection_impl_test.cc | 16 +-- test/integration/fake_upstream.h | 4 +- test/mocks/event/mocks.h | 26 +++-- test/mocks/server/mocks.cc | 2 + test/mocks/server/mocks.h | 17 +++ test/server/connection_handler_test.cc | 21 ++-- 21 files changed, 260 insertions(+), 176 deletions(-) create mode 100644 include/envoy/server/connection_handler.h rename source/server/{connection_handler.cc => connection_handler_impl.cc} (53%) rename source/server/{connection_handler.h => connection_handler_impl.h} (63%) diff --git a/include/envoy/event/dispatcher.h b/include/envoy/event/dispatcher.h index 8673ddaef594..7227fbf9a57f 100644 --- a/include/envoy/event/dispatcher.h +++ b/include/envoy/event/dispatcher.h @@ -9,6 +9,7 @@ #include "envoy/network/listener.h" #include "envoy/network/listen_socket.h" #include "envoy/ssl/context.h" +#include "envoy/server/connection_handler.h" #include "envoy/stats/stats.h" namespace Event { @@ -68,6 +69,7 @@ class Dispatcher { /** * Create a listener on a specific port. + * @param conn_handler supplies the handler for connections received by the listener * @param socket supplies the socket to listen on. * @param cb supplies the callbacks to invoke for listener events. * @param stats_store supplies the Stats::Store to use. @@ -80,13 +82,15 @@ class Dispatcher { * allow the listener to hand it off to the listener associated to the original port * @return Network::ListenerPtr a new listener that is owned by the caller. */ - virtual Network::ListenerPtr createListener(Network::ListenSocket& socket, + virtual Network::ListenerPtr createListener(Server::ConnectionHandler& conn_handler, + Network::ListenSocket& socket, Network::ListenerCallbacks& cb, Stats::Store& stats_store, bool bind_to_port, bool use_proxy_proto, bool use_orig_dst) PURE; /** * Create a listener on a specific port. + * @param conn_handler supplies the handler for connections received by the listener * @param ssl_ctx supplies the SSL context to use. * @param socket supplies the socket to listen on. * @param cb supplies the callbacks to invoke for listener events. @@ -100,7 +104,8 @@ class Dispatcher { * allow the listener to hand it off to the listener associated to the original port * @return Network::ListenerPtr a new listener that is owned by the caller. */ - virtual Network::ListenerPtr createSslListener(Ssl::ServerContext& ssl_ctx, + virtual Network::ListenerPtr createSslListener(Server::ConnectionHandler& conn_handler, + Ssl::ServerContext& ssl_ctx, Network::ListenSocket& socket, Network::ListenerCallbacks& cb, Stats::Store& stats_store, bool bind_to_port, diff --git a/include/envoy/server/connection_handler.h b/include/envoy/server/connection_handler.h new file mode 100644 index 000000000000..ef293a85f0d3 --- /dev/null +++ b/include/envoy/server/connection_handler.h @@ -0,0 +1,68 @@ +#pragma once + +#include "envoy/network/connection.h" +#include "envoy/network/filter.h" +#include "envoy/network/listener.h" +#include "envoy/network/listen_socket.h" +#include "envoy/ssl/context.h" + +namespace Server { + +/** + * Abstract connection handler. + */ + +class ConnectionHandler { +public: + virtual ~ConnectionHandler(){}; + + virtual uint64_t numConnections() PURE; + + /** + * Adds listener to the handler. + * @param factory supplies the configuration factory for new connections. + * @param socket supplies the already bound socket to listen on. + * @param bind_to_port specifies if the listener should actually bind to the port. + * a listener that doesn't bind can only receive connections redirected from + * other listeners that set use_origin_dst to true + * @param use_proxy_proto whether to use the PROXY Protocol V1 + * (http://www.haproxy.org/download/1.5/doc/proxy-protocol.txt) + * @param use_orig_dst if a connection was redirected to this port using iptables, + * allow the listener to hand it off to the listener associated to the original port + */ + virtual void addListener(Network::FilterChainFactory& factory, Network::ListenSocket& socket, + bool bind_to_port, bool use_proxy_proto, bool use_orig_dst) PURE; + + /** + * Adds listener to the handler. + * @param factory supplies the configuration factory for new connections. + * @param socket supplies the already bound socket to listen on. + * @param bind_to_port specifies if the listener should actually bind to the port. + * a listener that doesn't bind can only receive connections redirected from + * other listeners that set use_origin_dst to true + * @param use_proxy_proto whether to use the PROXY Protocol V1 + * (http://www.haproxy.org/download/1.5/doc/proxy-protocol.txt) + * @param use_orig_dst if a connection was redirected to this port using iptables, + * allow the listener to hand it off to the listener associated to the original port + */ + virtual void addSslListener(Network::FilterChainFactory& factory, Ssl::ServerContext& ssl_ctx, + Network::ListenSocket& socket, bool bind_to_port, + bool use_proxy_proto, bool use_orig_dst) PURE; + + /** + * Find a listener based on the provided socket name + * @param name supplies the name of the socket + * @return a pointer to the listener or nullptr if not found. + * Ownership of the listener is NOT transferred + */ + virtual Network::Listener* findListener(const std::string& socket_name) PURE; + + /** + * Close and destroy all listeners. + */ + virtual void closeListeners() PURE; +}; + +typedef std::unique_ptr ConnectionHandlerPtr; + +} // Server diff --git a/source/common/event/dispatcher_impl.cc b/source/common/event/dispatcher_impl.cc index 83b8de86bb95..90bfde120aed 100644 --- a/source/common/event/dispatcher_impl.cc +++ b/source/common/event/dispatcher_impl.cc @@ -76,21 +76,24 @@ Filesystem::WatcherPtr DispatcherImpl::createFilesystemWatcher() { return Filesystem::WatcherPtr{new Filesystem::WatcherImpl(*this)}; } -Network::ListenerPtr DispatcherImpl::createListener(Network::ListenSocket& socket, +Network::ListenerPtr DispatcherImpl::createListener(Server::ConnectionHandler& conn_handler, + Network::ListenSocket& socket, Network::ListenerCallbacks& cb, Stats::Store& stats_store, bool bind_to_port, bool use_proxy_proto, bool use_orig_dst) { return Network::ListenerPtr{new Network::ListenerImpl( - *this, socket, cb, stats_store, bind_to_port, use_proxy_proto, use_orig_dst)}; + conn_handler, *this, socket, cb, stats_store, bind_to_port, use_proxy_proto, use_orig_dst)}; } -Network::ListenerPtr DispatcherImpl::createSslListener(Ssl::ServerContext& ssl_ctx, +Network::ListenerPtr DispatcherImpl::createSslListener(Server::ConnectionHandler& conn_handler, + Ssl::ServerContext& ssl_ctx, Network::ListenSocket& socket, Network::ListenerCallbacks& cb, Stats::Store& stats_store, bool bind_to_port, bool use_proxy_proto, bool use_orig_dst) { - return Network::ListenerPtr{new Network::SslListenerImpl( - *this, ssl_ctx, socket, cb, stats_store, bind_to_port, use_proxy_proto, use_orig_dst)}; + return Network::ListenerPtr{new Network::SslListenerImpl(conn_handler, *this, ssl_ctx, socket, cb, + stats_store, bind_to_port, + use_proxy_proto, use_orig_dst)}; } TimerPtr DispatcherImpl::createTimer(TimerCb cb) { return TimerPtr{new TimerImpl(*this, cb)}; } diff --git a/source/common/event/dispatcher_impl.h b/source/common/event/dispatcher_impl.h index 51f82c86ad35..0f75c4c3a57d 100644 --- a/source/common/event/dispatcher_impl.h +++ b/source/common/event/dispatcher_impl.h @@ -4,6 +4,7 @@ #include "envoy/event/deferred_deletable.h" #include "envoy/event/dispatcher.h" +#include "envoy/server/connection_handler.h" #include "common/common/logger.h" @@ -30,10 +31,12 @@ class DispatcherImpl : Logger::Loggable, public Dispatcher { Network::DnsResolverPtr createDnsResolver() override; FileEventPtr createFileEvent(int fd, FileReadyCb cb) override; Filesystem::WatcherPtr createFilesystemWatcher() override; - Network::ListenerPtr createListener(Network::ListenSocket& socket, Network::ListenerCallbacks& cb, + Network::ListenerPtr createListener(Server::ConnectionHandler& conn_handler, + Network::ListenSocket& socket, Network::ListenerCallbacks& cb, Stats::Store& stats_store, bool bind_to_port, bool use_proxy_proto, bool use_orig_dst) override; - Network::ListenerPtr createSslListener(Ssl::ServerContext& ssl_ctx, Network::ListenSocket& socket, + Network::ListenerPtr createSslListener(Server::ConnectionHandler& conn_handler, + Ssl::ServerContext& ssl_ctx, Network::ListenSocket& socket, Network::ListenerCallbacks& cb, Stats::Store& stats_store, bool bind_to_port, bool use_proxy_proto, bool use_orig_dst) override; diff --git a/source/common/network/listener_impl.cc b/source/common/network/listener_impl.cc index 4bfdec667d36..10b21bacbaa6 100644 --- a/source/common/network/listener_impl.cc +++ b/source/common/network/listener_impl.cc @@ -9,7 +9,7 @@ #include "common/network/connection_impl.h" #include "common/ssl/connection_impl.h" -#include "server/connection_handler.h" +#include "envoy/server/connection_handler.h" #include "event2/listener.h" @@ -19,7 +19,7 @@ void ListenerImpl::listenCallback(evconnlistener*, evutil_socket_t fd, sockaddr* void* arg) { ListenerImpl* listener = static_cast(arg); - if (listener->use_original_dst_ && listener->connection_handler_ != nullptr) { + if (listener->use_original_dst_) { sockaddr_storage orig_dst_addr; memset(&orig_dst_addr, 0, sizeof(orig_dst_addr)); @@ -34,11 +34,11 @@ void ListenerImpl::listenCallback(evconnlistener*, evutil_socket_t fd, sockaddr* // the address and port returned by getOriginalDst() match the listener port. // In this case the listener handles the connection directly and does not hand it off. if (listener->socket_.name() != orig_sock_name) { - ListenerImpl* new_listener = - static_cast(listener->connection_handler_->findListener(orig_sock_name)); + Listener* new_listener = listener->connection_handler_.findListener(orig_sock_name); if (new_listener != nullptr) { - listener = new_listener; + listener->newConnection(fd, addr); + return; } } } @@ -47,12 +47,13 @@ void ListenerImpl::listenCallback(evconnlistener*, evutil_socket_t fd, sockaddr* listener->newConnection(fd, addr); } -ListenerImpl::ListenerImpl(Event::DispatcherImpl& dispatcher, ListenSocket& socket, +ListenerImpl::ListenerImpl(Server::ConnectionHandler& conn_handler, + Event::DispatcherImpl& dispatcher, ListenSocket& socket, ListenerCallbacks& cb, Stats::Store& stats_store, bool bind_to_port, bool use_proxy_proto, bool use_orig_dst) - : dispatcher_(dispatcher), socket_(socket), cb_(cb), bind_to_port_(bind_to_port), - use_proxy_proto_(use_proxy_proto), proxy_protocol_(stats_store), - use_original_dst_(use_orig_dst), connection_handler_(nullptr), listener_(nullptr) { + : connection_handler_(conn_handler), dispatcher_(dispatcher), socket_(socket), cb_(cb), + bind_to_port_(bind_to_port), use_proxy_proto_(use_proxy_proto), proxy_protocol_(stats_store), + use_original_dst_(use_orig_dst), listener_(nullptr) { if (bind_to_port_) { listener_.reset( diff --git a/source/common/network/listener_impl.h b/source/common/network/listener_impl.h index 68f3cdbff72f..14acb63c09f8 100644 --- a/source/common/network/listener_impl.h +++ b/source/common/network/listener_impl.h @@ -4,14 +4,13 @@ #include "proxy_protocol.h" #include "envoy/network/listener.h" +#include "envoy/server/connection_handler.h" #include "common/event/dispatcher_impl.h" #include "common/event/libevent.h" #include "event2/event.h" -class ConnectionHandler; - namespace Network { /** @@ -19,9 +18,9 @@ namespace Network { */ class ListenerImpl : public Listener { public: - ListenerImpl(Event::DispatcherImpl& dispatcher, ListenSocket& socket, ListenerCallbacks& cb, - Stats::Store& stats_store, bool bind_to_port, bool use_proxy_proto, - bool use_orig_dst); + ListenerImpl(Server::ConnectionHandler& conn_handler, Event::DispatcherImpl& dispatcher, + ListenSocket& socket, ListenerCallbacks& cb, Stats::Store& stats_store, + bool bind_to_port, bool use_proxy_proto, bool use_orig_dst); /** * Accept/process a new connection. @@ -42,17 +41,11 @@ class ListenerImpl : public Listener { */ ListenSocket& socket() { return socket_; } - /** - * Set a pointer to the connection handler that handles connections for this listener - * Invoked when the listener becomes active. - * @param conn_handler the connection handler associated to this listener - */ - void connectionHandler(ConnectionHandler* conn_handler) { connection_handler_ = conn_handler; } - protected: const std::string getAddressName(sockaddr* addr); uint16_t getAddressPort(sockaddr* addr); + Server::ConnectionHandler& connection_handler_; Event::DispatcherImpl& dispatcher_; ListenSocket& socket_; ListenerCallbacks& cb_; @@ -60,7 +53,6 @@ class ListenerImpl : public Listener { bool use_proxy_proto_; ProxyProtocol proxy_protocol_; bool use_original_dst_; - ConnectionHandler* connection_handler_; private: static void errorCallback(evconnlistener* listener, void* context); @@ -71,11 +63,12 @@ class ListenerImpl : public Listener { class SslListenerImpl : public ListenerImpl { public: - SslListenerImpl(Event::DispatcherImpl& dispatcher, Ssl::Context& ssl_ctx, ListenSocket& socket, - ListenerCallbacks& cb, Stats::Store& stats_store, bool bind_to_port, - bool use_proxy_proto, bool use_orig_dst) - : ListenerImpl(dispatcher, socket, cb, stats_store, bind_to_port, use_proxy_proto, - use_orig_dst), + SslListenerImpl(Server::ConnectionHandler& conn_handler, Event::DispatcherImpl& dispatcher, + Ssl::Context& ssl_ctx, ListenSocket& socket, ListenerCallbacks& cb, + Stats::Store& stats_store, bool bind_to_port, bool use_proxy_proto, + bool use_orig_dst) + : ListenerImpl(conn_handler, dispatcher, socket, cb, stats_store, bind_to_port, + use_proxy_proto, use_orig_dst), ssl_ctx_(ssl_ctx) {} // ListenerImpl diff --git a/source/server/CMakeLists.txt b/source/server/CMakeLists.txt index 65277c1c67bf..14f76106437b 100644 --- a/source/server/CMakeLists.txt +++ b/source/server/CMakeLists.txt @@ -12,7 +12,7 @@ add_library(envoy-server OBJECT config/network/ratelimit.cc config/network/tcp_proxy.cc configuration_impl.cc - connection_handler.cc + connection_handler_impl.cc drain_manager_impl.cc http/admin.cc http/health_check.cc diff --git a/source/server/connection_handler.cc b/source/server/connection_handler_impl.cc similarity index 53% rename from source/server/connection_handler.cc rename to source/server/connection_handler_impl.cc index 7a3af8daf127..04abcc6a0bb6 100644 --- a/source/server/connection_handler.cc +++ b/source/server/connection_handler_impl.cc @@ -1,4 +1,4 @@ -#include "connection_handler.h" +#include "connection_handler_impl.h" #include "envoy/event/dispatcher.h" #include "envoy/event/timer.h" @@ -7,32 +7,35 @@ #include "common/network/listener_impl.h" #include "common/event/dispatcher_impl.h" -ConnectionHandler::ConnectionHandler(Stats::Store& stats_store, spdlog::logger& logger, - Api::ApiPtr&& api) +namespace Server { + +ConnectionHandlerImpl::ConnectionHandlerImpl(Stats::Store& stats_store, spdlog::logger& logger, + Api::ApiPtr&& api) : stats_store_(stats_store), logger_(logger), api_(std::move(api)), dispatcher_(api_->allocateDispatcher()), watchdog_miss_counter_(stats_store.counter("server.watchdog_miss")), watchdog_mega_miss_counter_(stats_store.counter("server.watchdog_mega_miss")) {} -ConnectionHandler::~ConnectionHandler() { closeConnections(); } +ConnectionHandlerImpl::~ConnectionHandlerImpl() { closeConnections(); } -void ConnectionHandler::addListener(Network::FilterChainFactory& factory, - Network::ListenSocket& socket, bool bind_to_port, - bool use_proxy_proto, bool use_orig_dst) { +void ConnectionHandlerImpl::addListener(Network::FilterChainFactory& factory, + Network::ListenSocket& socket, bool bind_to_port, + bool use_proxy_proto, bool use_orig_dst) { ActiveListenerPtr l( new ActiveListener(*this, socket, factory, bind_to_port, use_proxy_proto, use_orig_dst)); listeners_.insert(std::make_pair(socket.name(), std::move(l))); } -void ConnectionHandler::addSslListener(Network::FilterChainFactory& factory, - Ssl::ServerContext& ssl_ctx, Network::ListenSocket& socket, - bool bind_to_port, bool use_proxy_proto, bool use_orig_dst) { +void ConnectionHandlerImpl::addSslListener(Network::FilterChainFactory& factory, + Ssl::ServerContext& ssl_ctx, + Network::ListenSocket& socket, bool bind_to_port, + bool use_proxy_proto, bool use_orig_dst) { ActiveListenerPtr l(new SslActiveListener(*this, ssl_ctx, socket, factory, bind_to_port, use_proxy_proto, use_orig_dst)); listeners_.insert(std::make_pair(socket.name(), std::move(l))); } -void ConnectionHandler::closeConnections() { +void ConnectionHandlerImpl::closeConnections() { while (!connections_.empty()) { connections_.front()->connection_->close(Network::ConnectionCloseType::NoFlush); } @@ -40,63 +43,55 @@ void ConnectionHandler::closeConnections() { dispatcher_->clearDeferredDeleteList(); } -void ConnectionHandler::closeListeners() { +void ConnectionHandlerImpl::closeListeners() { for (auto& l : listeners_) { l.second->listener_.reset(); } } -void ConnectionHandler::removeConnection(ActiveConnection& connection) { +void ConnectionHandlerImpl::removeConnection(ActiveConnection& connection) { conn_log(logger_, info, "adding to cleanup list", *connection.connection_); ActiveConnectionPtr removed = connection.removeFromList(connections_); dispatcher_->deferredDelete(std::move(removed)); num_connections_--; } -ConnectionHandler::ActiveListener::ActiveListener(ConnectionHandler& parent, - Network::ListenSocket& socket, - Network::FilterChainFactory& factory, - bool bind_to_port, bool use_proxy_proto, - bool use_orig_dst) - : ActiveListener(parent, parent.dispatcher_->createListener(socket, *this, parent.stats_store_, - bind_to_port, use_proxy_proto, - use_orig_dst), - factory, socket.name()) { - - if (use_orig_dst) { - static_cast(listener_.get())->connectionHandler(&parent); - } -} - -ConnectionHandler::ActiveListener::ActiveListener(ConnectionHandler& parent, - Network::ListenerPtr&& listener, - Network::FilterChainFactory& factory, - const std::string& stats_prefix) +ConnectionHandlerImpl::ActiveListener::ActiveListener(ConnectionHandlerImpl& parent, + Network::ListenSocket& socket, + Network::FilterChainFactory& factory, + bool bind_to_port, bool use_proxy_proto, + bool use_orig_dst) + : ActiveListener(parent, parent.dispatcher_->createListener(parent, socket, *this, + parent.stats_store_, bind_to_port, + use_proxy_proto, use_orig_dst), + factory, socket.name()) {} + +ConnectionHandlerImpl::ActiveListener::ActiveListener(ConnectionHandlerImpl& parent, + Network::ListenerPtr&& listener, + Network::FilterChainFactory& factory, + const std::string& stats_prefix) : parent_(parent), factory_(factory), stats_(generateStats(stats_prefix, parent.stats_store_)) { listener_ = std::move(listener); } -ConnectionHandler::SslActiveListener::SslActiveListener(ConnectionHandler& parent, - Ssl::ServerContext& ssl_ctx, - Network::ListenSocket& socket, - Network::FilterChainFactory& factory, - bool bind_to_port, bool use_proxy_proto, - bool use_orig_dst) +ConnectionHandlerImpl::SslActiveListener::SslActiveListener(ConnectionHandlerImpl& parent, + Ssl::ServerContext& ssl_ctx, + Network::ListenSocket& socket, + Network::FilterChainFactory& factory, + bool bind_to_port, bool use_proxy_proto, + bool use_orig_dst) : ActiveListener(parent, parent.dispatcher_->createSslListener( - ssl_ctx, socket, *this, parent.stats_store_, bind_to_port, + parent, ssl_ctx, socket, *this, parent.stats_store_, bind_to_port, use_proxy_proto, use_orig_dst), - factory, socket.name()) { - if (use_orig_dst) { - static_cast(listener_.get())->connectionHandler(&parent); - } -} + factory, socket.name()) {} -Network::Listener* ConnectionHandler::findListener(const std::string& socket_name) { +Network::Listener* ConnectionHandlerImpl::findListener(const std::string& socket_name) { auto l = listeners_.find(socket_name); return (l != listeners_.end()) ? l->second->listener_.get() : nullptr; } -void ConnectionHandler::ActiveListener::onNewConnection(Network::ConnectionPtr&& new_connection) { +void ConnectionHandlerImpl::ActiveListener::onNewConnection( + Network::ConnectionPtr&& new_connection) { conn_log(parent_.logger_, info, "new connection", *new_connection); factory_.createFilterChain(*new_connection); @@ -109,9 +104,9 @@ void ConnectionHandler::ActiveListener::onNewConnection(Network::ConnectionPtr&& } } -ConnectionHandler::ActiveConnection::ActiveConnection(ConnectionHandler& parent, - Network::ConnectionPtr&& new_connection, - ListenerStats& stats) +ConnectionHandlerImpl::ActiveConnection::ActiveConnection(ConnectionHandlerImpl& parent, + Network::ConnectionPtr&& new_connection, + ListenerStats& stats) : parent_(parent), connection_(std::move(new_connection)), stats_(stats), conn_length_(stats_.downstream_cx_length_ms_.allocateSpan()) { // We just universally set no delay on connections. Theoretically we might at some point want @@ -122,20 +117,20 @@ ConnectionHandler::ActiveConnection::ActiveConnection(ConnectionHandler& parent, stats_.downstream_cx_active_.inc(); } -ConnectionHandler::ActiveConnection::~ActiveConnection() { +ConnectionHandlerImpl::ActiveConnection::~ActiveConnection() { stats_.downstream_cx_active_.dec(); stats_.downstream_cx_destroy_.inc(); conn_length_->complete(); } -ListenerStats ConnectionHandler::generateStats(const std::string& prefix, Stats::Store& store) { +ListenerStats ConnectionHandlerImpl::generateStats(const std::string& prefix, Stats::Store& store) { std::string final_prefix = fmt::format("listener.{}.", prefix); return {ALL_LISTENER_STATS(POOL_COUNTER_PREFIX(store, final_prefix), POOL_GAUGE_PREFIX(store, final_prefix), POOL_TIMER_PREFIX(store, final_prefix))}; } -void ConnectionHandler::startWatchdog() { +void ConnectionHandlerImpl::startWatchdog() { watchdog_timer_ = dispatcher_->createTimer([this]() -> void { auto delta = std::chrono::system_clock::now() - last_watchdog_time_; if (delta > std::chrono::milliseconds(200)) { @@ -152,3 +147,5 @@ void ConnectionHandler::startWatchdog() { last_watchdog_time_ = std::chrono::system_clock::now(); watchdog_timer_->enableTimer(std::chrono::milliseconds(100)); } + +} // Server diff --git a/source/server/connection_handler.h b/source/server/connection_handler_impl.h similarity index 63% rename from source/server/connection_handler.h rename to source/server/connection_handler_impl.h index bd0810d26ff2..019551997e81 100644 --- a/source/server/connection_handler.h +++ b/source/server/connection_handler_impl.h @@ -1,5 +1,6 @@ #pragma once +#include "envoy/server/connection_handler.h" #include "envoy/api/api.h" #include "envoy/common/time.h" #include "envoy/event/deferred_deletable.h" @@ -20,6 +21,8 @@ TIMER (downstream_cx_length_ms) // clang-format on +namespace Server { + /** * Wrapper struct for listener stats. @see stats_macros.h */ @@ -31,65 +34,33 @@ struct ListenerStats { * Server side connection handler. This is used both by workers as well as the * main thread for non-threaded listeners. */ -class ConnectionHandler final : NonCopyable { +class ConnectionHandlerImpl : public ConnectionHandler, NonCopyable { public: - ConnectionHandler(Stats::Store& stats_store, spdlog::logger& logger, Api::ApiPtr&& api); - ~ConnectionHandler(); + ConnectionHandlerImpl(Stats::Store& stats_store, spdlog::logger& logger, Api::ApiPtr&& api); + ~ConnectionHandlerImpl(); Api::Api& api() { return *api_; } Event::Dispatcher& dispatcher() { return *dispatcher_; } uint64_t numConnections() { return num_connections_; } - /** - * Adds listener to the handler. - * @param factory supplies the configuration factory for new connections. - * @param socket supplies the already bound socket to listen on. - * @param bind_to_port specifies if the listener should actually bind to the port. - * a listener that doesn't bind can only receive connections redirected from - * other listeners that set use_origin_dst to true - * @param use_proxy_proto whether to use the PROXY Protocol V1 - * (http://www.haproxy.org/download/1.5/doc/proxy-protocol.txt) - * @param use_orig_dst if a connection was redirected to this port using iptables, - * allow the listener to hand it off to the listener associated to the original port - */ void addListener(Network::FilterChainFactory& factory, Network::ListenSocket& socket, - bool bind_to_port, bool use_proxy_proto, bool use_orig_dst); + bool bind_to_port, bool use_proxy_proto, bool use_orig_dst) override; - /** - * Adds listener to the handler. - * @param factory supplies the configuration factory for new connections. - * @param socket supplies the already bound socket to listen on. - * @param bind_to_port specifies if the listener should actually bind to the port. - * a listener that doesn't bind can only receive connections redirected from - * other listeners that set use_origin_dst to true - * @param use_proxy_proto whether to use the PROXY Protocol V1 - * (http://www.haproxy.org/download/1.5/doc/proxy-protocol.txt) - * @param use_orig_dst if a connection was redirected to this port using iptables, - * allow the listener to hand it off to the listener associated to the original port - */ void addSslListener(Network::FilterChainFactory& factory, Ssl::ServerContext& ssl_ctx, Network::ListenSocket& socket, bool bind_to_port, bool use_proxy_proto, - bool use_orig_dst); + bool use_orig_dst) override; - /** - * Find a listener based on the provided socket name - * @param name supplies the name of the socket - * @return a pointer to the listener or nullptr if not found. - * Ownership of the listener is NOT transferred - */ - Network::Listener* findListener(const std::string& socket_name); + Network::Listener* findListener(const std::string& socket_name) override; /** - * Close and destroy all connections. This must be called from the same thread that is running - * the dispatch loop. + * Close and destroy all listeners. */ - void closeConnections(); + void closeListeners() override; /** - * Close and destroy all listeners. Existing connections will not be effected. This must be - * called from the same thread that is running the dispatch loop. + * Close and destroy all connections. */ - void closeListeners(); + void closeConnections(); /** * Start a watchdog that attempts to tick every 100ms and will increment a stat if a tick takes @@ -102,11 +73,11 @@ class ConnectionHandler final : NonCopyable { * Wrapper for an active listener owned by this worker. */ struct ActiveListener : public Network::ListenerCallbacks { - ActiveListener(ConnectionHandler& parent, Network::ListenSocket& socket, + ActiveListener(ConnectionHandlerImpl& parent, Network::ListenSocket& socket, Network::FilterChainFactory& factory, bool use_proxy_proto, bool bind_to_port, bool use_orig_dst); - ActiveListener(ConnectionHandler& parent, Network::ListenerPtr&& listener, + ActiveListener(ConnectionHandlerImpl& parent, Network::ListenerPtr&& listener, Network::FilterChainFactory& factory, const std::string& stats_prefix); /** @@ -115,14 +86,14 @@ class ConnectionHandler final : NonCopyable { */ void onNewConnection(Network::ConnectionPtr&& new_connection) override; - ConnectionHandler& parent_; + ConnectionHandlerImpl& parent_; Network::FilterChainFactory& factory_; Network::ListenerPtr listener_; ListenerStats stats_; }; struct SslActiveListener : public ActiveListener { - SslActiveListener(ConnectionHandler& parent, Ssl::ServerContext& ssl_ctx, + SslActiveListener(ConnectionHandlerImpl& parent, Ssl::ServerContext& ssl_ctx, Network::ListenSocket& socket, Network::FilterChainFactory& factory, bool use_proxy_proto, bool bind_to_port, bool use_orig_dst); }; @@ -135,7 +106,7 @@ class ConnectionHandler final : NonCopyable { struct ActiveConnection : LinkedObject, public Event::DeferredDeletable, public Network::ConnectionCallbacks { - ActiveConnection(ConnectionHandler& parent, Network::ConnectionPtr&& new_connection, + ActiveConnection(ConnectionHandlerImpl& parent, Network::ConnectionPtr&& new_connection, ListenerStats& stats); ~ActiveConnection(); @@ -148,7 +119,7 @@ class ConnectionHandler final : NonCopyable { } } - ConnectionHandler& parent_; + ConnectionHandlerImpl& parent_; Network::ConnectionPtr connection_; ListenerStats& stats_; Stats::TimespanPtr conn_length_; @@ -176,3 +147,8 @@ class ConnectionHandler final : NonCopyable { Event::TimerPtr watchdog_timer_; SystemTime last_watchdog_time_; }; + +typedef std::unique_ptr ConnectionHandlerImplPtr; + +} // Server + diff --git a/source/server/server.h b/source/server/server.h index bac3e9d6cd18..f4d386477eb8 100644 --- a/source/server/server.h +++ b/source/server/server.h @@ -1,7 +1,7 @@ #pragma once -#include "connection_handler.h" #include "worker.h" +#include "connection_handler_impl.h" #include "envoy/common/optional.h" #include "envoy/server/drain_manager.h" @@ -127,7 +127,7 @@ class InstanceImpl : Logger::Loggable, public Instance { ServerStats server_stats_; ThreadLocal::InstanceImpl thread_local_; SocketMap socket_map_; - ConnectionHandler handler_; + ConnectionHandlerImpl handler_; Runtime::RandomGeneratorImpl random_generator_; Runtime::LoaderPtr runtime_loader_; std::unique_ptr config_; diff --git a/source/server/worker.cc b/source/server/worker.cc index dc976d745a6f..0ce8d7bc4fe6 100644 --- a/source/server/worker.cc +++ b/source/server/worker.cc @@ -10,7 +10,7 @@ Worker::Worker(Stats::Store& stats_store, ThreadLocal::Instance& tls, std::chrono::milliseconds file_flush_interval_msec) - : tls_(tls), handler_(new ConnectionHandler( + : tls_(tls), handler_(new Server::ConnectionHandlerImpl( stats_store, log(), Api::ApiPtr{new Api::Impl(file_flush_interval_msec)})) { tls_.registerThread(handler_->dispatcher(), false); } diff --git a/source/server/worker.h b/source/server/worker.h index afa09fd4f3bc..ece92fd5d725 100644 --- a/source/server/worker.h +++ b/source/server/worker.h @@ -1,6 +1,6 @@ #pragma once -#include "connection_handler.h" +#include "connection_handler_impl.h" #include "envoy/server/configuration.h" #include "envoy/thread_local/thread_local.h" @@ -20,7 +20,7 @@ class Worker : Logger::Loggable { ~Worker(); Event::Dispatcher& dispatcher() { return handler_->dispatcher(); } - ConnectionHandler* handler() { return handler_.get(); } + Server::ConnectionHandler* handler() { return handler_.get(); } void initializeConfiguration(Server::Configuration::Main& config, const SocketMap& socket_map); /** @@ -33,7 +33,7 @@ class Worker : Logger::Loggable { void threadRoutine(); ThreadLocal::Instance& tls_; - std::unique_ptr handler_; + Server::ConnectionHandlerImplPtr handler_; Event::TimerPtr no_exit_timer_; Thread::ThreadPtr thread_; }; diff --git a/test/common/network/connection_impl_test.cc b/test/common/network/connection_impl_test.cc index 68f4b83e23fd..93e77f11faef 100644 --- a/test/common/network/connection_impl_test.cc +++ b/test/common/network/connection_impl_test.cc @@ -6,6 +6,7 @@ #include "test/mocks/network/mocks.h" #include "test/mocks/stats/mocks.h" +#include "test/mocks/server/mocks.h" using testing::_; using testing::Sequence; @@ -61,8 +62,9 @@ TEST(ConnectionImplTest, BufferStats) { Event::DispatcherImpl dispatcher; Network::TcpListenSocket socket(uint32_t(10000), true); Network::MockListenerCallbacks listener_callbacks; - Network::ListenerPtr listener = - dispatcher.createListener(socket, listener_callbacks, stats_store, true, false, false); + Server::MockConnectionHandler connection_handler; + Network::ListenerPtr listener = dispatcher.createListener( + connection_handler, socket, listener_callbacks, stats_store, true, false, false); Network::ClientConnectionPtr client_connection = dispatcher.createClientConnection("tcp://127.0.0.1:10000"); diff --git a/test/common/network/listener_impl_test.cc b/test/common/network/listener_impl_test.cc index 27eb9b9ce867..c619a9fc5c55 100644 --- a/test/common/network/listener_impl_test.cc +++ b/test/common/network/listener_impl_test.cc @@ -2,6 +2,7 @@ #include "common/stats/stats_impl.h" #include "test/mocks/network/mocks.h" +#include "test/mocks/server/mocks.h" using testing::_; using testing::Invoke; @@ -15,8 +16,9 @@ static void errorCallbackTest() { Event::DispatcherImpl dispatcher; Network::TcpListenSocket socket(uint32_t(10000), true); Network::MockListenerCallbacks listener_callbacks; - Network::ListenerPtr listener = - dispatcher.createListener(socket, listener_callbacks, stats_store, true, false, false); + Server::MockConnectionHandler connection_handler; + Network::ListenerPtr listener = dispatcher.createListener( + connection_handler, socket, listener_callbacks, stats_store, true, false, false); Network::ClientConnectionPtr client_connection = dispatcher.createClientConnection("tcp://127.0.0.1:10000"); diff --git a/test/common/network/proxy_protocol_test.cc b/test/common/network/proxy_protocol_test.cc index 4a53b5c88fef..a9e8f593d60d 100644 --- a/test/common/network/proxy_protocol_test.cc +++ b/test/common/network/proxy_protocol_test.cc @@ -5,6 +5,7 @@ #include "test/mocks/buffer/mocks.h" #include "test/mocks/network/mocks.h" +#include "test/mocks/server/mocks.h" using testing::_; using testing::Invoke; @@ -15,8 +16,8 @@ namespace Network { class ProxyProtocolTest : public testing::Test { public: ProxyProtocolTest() - : socket_(uint32_t(1234), true), - listener_(dispatcher_, socket_, callbacks_, stats_store_, true, true, false) { + : socket_(uint32_t(1234), true), listener_(connection_handler_, dispatcher_, socket_, + callbacks_, stats_store_, true, true, false) { conn_ = dispatcher_.createClientConnection("tcp://127.0.0.1:1234"); conn_->addConnectionCallbacks(connection_callbacks_); conn_->connect(); @@ -31,6 +32,7 @@ class ProxyProtocolTest : public testing::Test { TcpListenSocket socket_; Stats::IsolatedStoreImpl stats_store_; MockListenerCallbacks callbacks_; + Server::MockConnectionHandler connection_handler_; ListenerImpl listener_; ClientConnectionPtr conn_; NiceMock connection_callbacks_; diff --git a/test/common/ssl/connection_impl_test.cc b/test/common/ssl/connection_impl_test.cc index 3a8c03fc42b1..540675a1e711 100644 --- a/test/common/ssl/connection_impl_test.cc +++ b/test/common/ssl/connection_impl_test.cc @@ -8,6 +8,7 @@ #include "test/mocks/network/mocks.h" #include "test/mocks/runtime/mocks.h" +#include "test/mocks/server/mocks.h" using testing::_; using testing::Invoke; @@ -33,8 +34,9 @@ TEST(SslConnectionImplTest, ClientAuth) { Event::DispatcherImpl dispatcher; Network::TcpListenSocket socket(uint32_t(10000), true); Network::MockListenerCallbacks callbacks; - Network::ListenerPtr listener = - dispatcher.createSslListener(server_ctx, socket, callbacks, stats_store, true, false, false); + Server::MockConnectionHandler connection_handler; + Network::ListenerPtr listener = dispatcher.createSslListener( + connection_handler, server_ctx, socket, callbacks, stats_store, true, false, false); std::string client_ctx_json = R"EOF( { @@ -91,8 +93,9 @@ TEST(SslConnectionImplTest, ClientAuthBadVerification) { Event::DispatcherImpl dispatcher; Network::TcpListenSocket socket(uint32_t(10000), true); Network::MockListenerCallbacks callbacks; - Network::ListenerPtr listener = - dispatcher.createSslListener(server_ctx, socket, callbacks, stats_store, true, false, false); + Server::MockConnectionHandler connection_handler; + Network::ListenerPtr listener = dispatcher.createSslListener( + connection_handler, server_ctx, socket, callbacks, stats_store, true, false, false); std::string client_ctx_json = R"EOF( { @@ -145,8 +148,9 @@ TEST(SslConnectionImplTest, SslError) { Event::DispatcherImpl dispatcher; Network::TcpListenSocket socket(uint32_t(10000), true); Network::MockListenerCallbacks callbacks; - Network::ListenerPtr listener = - dispatcher.createSslListener(server_ctx, socket, callbacks, stats_store, true, false, false); + Server::MockConnectionHandler connection_handler; + Network::ListenerPtr listener = dispatcher.createSslListener( + connection_handler, server_ctx, socket, callbacks, stats_store, true, false, false); Network::ClientConnectionPtr client_connection = dispatcher.createClientConnection("tcp://127.0.0.1:10000"); diff --git a/test/integration/fake_upstream.h b/test/integration/fake_upstream.h index ae10fac4fda5..6ad6e06e937f 100644 --- a/test/integration/fake_upstream.h +++ b/test/integration/fake_upstream.h @@ -10,7 +10,7 @@ #include "common/network/filter_impl.h" #include "common/network/listen_socket_impl.h" #include "common/stats/stats_impl.h" -#include "server/connection_handler.h" +#include "server/connection_handler_impl.h" class FakeHttpConnection; @@ -171,7 +171,7 @@ class FakeUpstream : Logger::Loggable, public Network::Filt std::mutex lock_; std::condition_variable new_connection_event_; Stats::IsolatedStoreImpl stats_store_; - ConnectionHandler handler_; + Server::ConnectionHandlerImpl handler_; std::list new_connections_; FakeHttpConnection::Type http_type_; }; diff --git a/test/mocks/event/mocks.h b/test/mocks/event/mocks.h index 3a3b9a677d7c..847c5e2fcdbd 100644 --- a/test/mocks/event/mocks.h +++ b/test/mocks/event/mocks.h @@ -9,6 +9,7 @@ #include "envoy/network/dns.h" #include "envoy/network/listener.h" #include "envoy/ssl/context.h" +#include "envoy/server/connection_handler.h" namespace Event { @@ -38,19 +39,22 @@ class MockDispatcher : public Dispatcher { return Filesystem::WatcherPtr{createFilesystemWatcher_()}; } - Network::ListenerPtr createListener(Network::ListenSocket& socket, Network::ListenerCallbacks& cb, + Network::ListenerPtr createListener(Server::ConnectionHandler& conn_handler, + Network::ListenSocket& socket, Network::ListenerCallbacks& cb, Stats::Store& stats_store, bool bind_to_port, bool use_proxy_proto, bool use_original_dst) override { - return Network::ListenerPtr{ - createListener_(socket, cb, stats_store, bind_to_port, use_proxy_proto, use_original_dst)}; + return Network::ListenerPtr{createListener_(conn_handler, socket, cb, stats_store, bind_to_port, + use_proxy_proto, use_original_dst)}; } - Network::ListenerPtr createSslListener(Ssl::ServerContext& ssl_ctx, Network::ListenSocket& socket, + Network::ListenerPtr createSslListener(Server::ConnectionHandler& conn_handler, + Ssl::ServerContext& ssl_ctx, Network::ListenSocket& socket, Network::ListenerCallbacks& cb, Stats::Store& stats_store, bool bind_to_port, bool use_proxy_proto, bool use_original_dst) override { - return Network::ListenerPtr{createSslListener_(ssl_ctx, socket, cb, stats_store, bind_to_port, - use_proxy_proto, use_original_dst)}; + return Network::ListenerPtr{createSslListener_(conn_handler, ssl_ctx, socket, cb, stats_store, + bind_to_port, use_proxy_proto, + use_original_dst)}; } TimerPtr createTimer(TimerCb cb) override { return TimerPtr{createTimer_(cb)}; } @@ -74,12 +78,14 @@ class MockDispatcher : public Dispatcher { MOCK_METHOD0(createDnsResolver_, Network::DnsResolver*()); MOCK_METHOD2(createFileEvent_, FileEvent*(int fd, FileReadyCb cb)); MOCK_METHOD0(createFilesystemWatcher_, Filesystem::Watcher*()); - MOCK_METHOD6(createListener_, - Network::Listener*(Network::ListenSocket& socket, Network::ListenerCallbacks& cb, + MOCK_METHOD7(createListener_, + Network::Listener*(Server::ConnectionHandler& conn_handler, + Network::ListenSocket& socket, Network::ListenerCallbacks& cb, Stats::Store& stats_store, bool bind_to_port, bool use_proxy_proto, bool use_original_dst)); - MOCK_METHOD7(createSslListener_, - Network::Listener*(Ssl::ServerContext& ssl_ctx, Network::ListenSocket& socket, + MOCK_METHOD8(createSslListener_, + Network::Listener*(Server::ConnectionHandler& conn_handler, + Ssl::ServerContext& ssl_ctx, Network::ListenSocket& socket, Network::ListenerCallbacks& cb, Stats::Store& stats_store, bool bind_to_port, bool use_proxy_proto, bool use_original_dst)); MOCK_METHOD1(createTimer_, Timer*(TimerCb cb)); diff --git a/test/mocks/server/mocks.cc b/test/mocks/server/mocks.cc index 86441e4a2d59..4466a3dda7b6 100644 --- a/test/mocks/server/mocks.cc +++ b/test/mocks/server/mocks.cc @@ -44,4 +44,6 @@ MockInstance::MockInstance() : ssl_context_manager_(runtime_loader_) { MockInstance::~MockInstance() {} +MockConnectionHandler::MockConnectionHandler() {} +MockConnectionHandler::~MockConnectionHandler() {} } // Server diff --git a/test/mocks/server/mocks.h b/test/mocks/server/mocks.h index 3ea4b5c0d333..6c5d8de92727 100644 --- a/test/mocks/server/mocks.h +++ b/test/mocks/server/mocks.h @@ -4,6 +4,7 @@ #include "envoy/server/drain_manager.h" #include "envoy/server/instance.h" #include "envoy/server/options.h" +#include "envoy/server/connection_handler.h" #include "envoy/ssl/context_manager.h" #include "common/ssl/context_manager_impl.h" @@ -136,4 +137,20 @@ class MockInstance : public Instance { testing::NiceMock random_; }; +class MockConnectionHandler : public ConnectionHandler { +public: + MockConnectionHandler(); + ~MockConnectionHandler(); + + MOCK_METHOD0(numConnections, uint64_t()); + MOCK_METHOD5(addListener, + void(Network::FilterChainFactory& factory, Network::ListenSocket& socket, + bool bind_to_port, bool use_proxy_proto, bool use_orig_dst)); + MOCK_METHOD6(addSslListener, void(Network::FilterChainFactory& factory, + Ssl::ServerContext& ssl_ctx, Network::ListenSocket& socket, + bool bind_to_port, bool use_proxy_proto, bool use_orig_dst)); + MOCK_METHOD1(findListener, Network::Listener*(const std::string& socket_name)); + MOCK_METHOD0(closeListeners, void()); +}; + } // Server diff --git a/test/server/connection_handler_test.cc b/test/server/connection_handler_test.cc index 698647c51501..88d048da8dc0 100644 --- a/test/server/connection_handler_test.cc +++ b/test/server/connection_handler_test.cc @@ -1,8 +1,9 @@ #include "common/stats/stats_impl.h" -#include "server/connection_handler.h" +#include "server/connection_handler_impl.h" #include "test/mocks/api/mocks.h" #include "test/mocks/network/mocks.h" +#include "test/mocks/server/mocks.h" using testing::_; using testing::InSequence; @@ -19,19 +20,21 @@ TEST_F(ConnectionHandlerTest, CloseDuringFilterChainCreate) { Api::MockApi* api = new Api::MockApi(); Event::MockDispatcher* dispatcher = new NiceMock(); EXPECT_CALL(*api, allocateDispatcher_()).WillOnce(Return(dispatcher)); - ConnectionHandler handler(stats_store, log(), Api::ApiPtr{api}); + Server::ConnectionHandlerImpl handler(stats_store, log(), Api::ApiPtr{api}); Network::MockFilterChainFactory factory; + Server::MockConnectionHandler connection_handler; NiceMock socket; Network::Listener* listener = new Network::MockListener(); Network::ListenerCallbacks* listener_callbacks; - EXPECT_CALL(*dispatcher, createListener_(_, _, _, _, _, _)) - .WillOnce(Invoke([&](Network::ListenSocket&, Network::ListenerCallbacks& cb, Stats::Store&, - bool, bool, bool) -> Network::Listener* { - listener_callbacks = &cb; - return listener; - - })); + EXPECT_CALL(*dispatcher, createListener_(_, _, _, _, _, _, _)) + .WillOnce(Invoke([&](Server::ConnectionHandler&, Network::ListenSocket&, + Network::ListenerCallbacks& cb, Stats::Store&, bool, bool, bool) + -> Network::Listener* { + listener_callbacks = &cb; + return listener; + + })); handler.addListener(factory, socket, true, false, false); Network::MockConnection* connection = new NiceMock(); From 14eaed459f8d0a0e9d096a5e40f55861ad4587bf Mon Sep 17 00:00:00 2001 From: Enrico Schiattarella Date: Mon, 9 Jan 2017 21:50:13 -0800 Subject: [PATCH 04/22] Fix style and typo --- source/common/network/listener_impl.cc | 6 +++--- source/server/connection_handler_impl.h | 1 - 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/source/common/network/listener_impl.cc b/source/common/network/listener_impl.cc index 10b21bacbaa6..1c6f60e774fe 100644 --- a/source/common/network/listener_impl.cc +++ b/source/common/network/listener_impl.cc @@ -34,11 +34,11 @@ void ListenerImpl::listenCallback(evconnlistener*, evutil_socket_t fd, sockaddr* // the address and port returned by getOriginalDst() match the listener port. // In this case the listener handles the connection directly and does not hand it off. if (listener->socket_.name() != orig_sock_name) { - Listener* new_listener = listener->connection_handler_.findListener(orig_sock_name); + ListenerImpl* new_listener = + dynamic_cast(listener->connection_handler_.findListener(orig_sock_name)); if (new_listener != nullptr) { - listener->newConnection(fd, addr); - return; + new_listener = listener; } } } diff --git a/source/server/connection_handler_impl.h b/source/server/connection_handler_impl.h index 019551997e81..04fc529879b2 100644 --- a/source/server/connection_handler_impl.h +++ b/source/server/connection_handler_impl.h @@ -151,4 +151,3 @@ class ConnectionHandlerImpl : public ConnectionHandler, NonCopyable { typedef std::unique_ptr ConnectionHandlerImplPtr; } // Server - From f655994ee3d4abfb4c6cb82772cc950b039693e0 Mon Sep 17 00:00:00 2001 From: Shriram Rajagopalan Date: Tue, 10 Jan 2017 11:08:34 -0500 Subject: [PATCH 05/22] Add regular expression support in header match (#335) --- .../http_conn_man/route_config/route.rst | 7 ++++- .../http_filters/fault_filter.rst | 7 ++++- source/common/http/filter/fault_filter.cc | 3 +- source/common/router/config_impl.cc | 22 +++++++++----- source/common/router/config_impl.h | 7 +++-- test/common/router/config_impl_test.cc | 29 ++++++++++++++++++- 6 files changed, 61 insertions(+), 14 deletions(-) diff --git a/docs/configuration/http_conn_man/route_config/route.rst b/docs/configuration/http_conn_man/route_config/route.rst index e27eacaab513..f162237b464e 100644 --- a/docs/configuration/http_conn_man/route_config/route.rst +++ b/docs/configuration/http_conn_man/route_config/route.rst @@ -198,7 +198,7 @@ The router can match a request to a route based on headers specified in the rout .. code-block:: json [ - {"name": "...", "value": "..."} + {"name": "...", "value": "...", "regex": "..."} ] name @@ -208,6 +208,11 @@ value *(optional, string)* Specifies the value of the header. If the value is absent a request that has the *name* header will match, regardless of the header's value. +regex + *(optional, boolean)* Specifies whether the header value is a regular + expression or not. Defaults to false. The regex grammar used in the value field + is defined `here `_. + The router will check the request's headers against all the specified headers in the route config. A match will happen if all the headers in the route are present in the request with the same values (or based on presence if the ``value`` field is not in the config). diff --git a/docs/configuration/http_filters/fault_filter.rst b/docs/configuration/http_filters/fault_filter.rst index 9749d0f67525..ceb246378446 100644 --- a/docs/configuration/http_filters/fault_filter.rst +++ b/docs/configuration/http_filters/fault_filter.rst @@ -119,7 +119,7 @@ actual fault injection further depend on the values of *abort_percent* and .. code-block:: json [ - {"name": "...", "value": "..."} + {"name": "...", "value": "...", "regex": "..."} ] name @@ -130,6 +130,11 @@ value absent a request that has the *name* header will match, regardless of the header's value. +regex + *(optional, boolean)* Specifies whether the header value is a regular expression + or not. Defaults to false. The regex grammar used in the value field + is defined `here `_. + The filter will check the request's headers against all the specified headers in the filter config. A match will happen if all the headers in the config are present in the request with the same values (or based on diff --git a/source/common/http/filter/fault_filter.cc b/source/common/http/filter/fault_filter.cc index 36ae4ab7f9cc..3ddba4244c6f 100644 --- a/source/common/http/filter/fault_filter.cc +++ b/source/common/http/filter/fault_filter.cc @@ -60,7 +60,8 @@ FaultFilterConfig::FaultFilterConfig(const Json::Object& json_config, Runtime::L for (const Json::ObjectPtr& header_map : config_headers) { // allow header value to be empty, allows matching to be only based on header presence. fault_filter_headers_.emplace_back(Http::LowerCaseString(header_map->getString("name")), - header_map->getString("value", EMPTY_STRING)); + header_map->getString("value", EMPTY_STRING), + header_map->getBoolean("regex", false)); } } } diff --git a/source/common/router/config_impl.cc b/source/common/router/config_impl.cc index e34738d408d1..398bc8fe1f66 100644 --- a/source/common/router/config_impl.cc +++ b/source/common/router/config_impl.cc @@ -48,17 +48,20 @@ Upstream::ResourcePriority ConfigUtility::parsePriority(const Json::Object& conf } } -bool ConfigUtility::matchHeaders(const Http::HeaderMap& headers, - const std::vector request_headers) { +bool ConfigUtility::matchHeaders(const Http::HeaderMap& request_headers, + const std::vector config_headers) { bool matches = true; - if (!request_headers.empty()) { - for (const HeaderData& header_data : request_headers) { - const Http::HeaderEntry* header = headers.get(header_data.name_); - if (header_data.value_ == EMPTY_STRING) { + if (!config_headers.empty()) { + for (const HeaderData& cfg_header_data : config_headers) { + const Http::HeaderEntry* header = request_headers.get(cfg_header_data.name_); + if (cfg_header_data.value_.empty()) { matches &= (header != nullptr); + } else if (!cfg_header_data.is_regex_) { + matches &= (header != nullptr) && (header->value() == cfg_header_data.value_.c_str()); } else { - matches &= (header != nullptr) && (header->value() == header_data.value_.c_str()); + matches &= (header != nullptr) && + std::regex_match(header->value().c_str(), cfg_header_data.regex_pattern_); } if (!matches) { break; @@ -91,8 +94,11 @@ RouteEntryImplBase::RouteEntryImplBase(const VirtualHostImpl& vhost, const Json: std::vector config_headers = route.getObjectArray("headers"); for (const Json::ObjectPtr& header_map : config_headers) { // allow header value to be empty, allows matching to be only based on header presence. + // Regex is an opt-in. Unless explicitly mentioned, we will use header values for exact string + // matches. config_headers_.emplace_back(Http::LowerCaseString(header_map->getString("name")), - header_map->getString("value", EMPTY_STRING)); + header_map->getString("value", EMPTY_STRING), + header_map->getBoolean("regex", false)); } } } diff --git a/source/common/router/config_impl.h b/source/common/router/config_impl.h index baac21ff3998..33c14887e574 100644 --- a/source/common/router/config_impl.h +++ b/source/common/router/config_impl.h @@ -46,11 +46,14 @@ class SslRedirector : public RedirectEntry { class ConfigUtility { public: struct HeaderData { - HeaderData(const Http::LowerCaseString& name, const std::string& value) - : name_(name), value_(value) {} + HeaderData(const Http::LowerCaseString& name, const std::string& value, const bool is_regex) + : name_(name), value_(value), regex_pattern_(value_, std::regex::optimize), + is_regex_(is_regex) {} const Http::LowerCaseString name_; const std::string value_; + const std::regex regex_pattern_; + const bool is_regex_; }; /** diff --git a/test/common/router/config_impl_test.cc b/test/common/router/config_impl_test.cc index aca68929f94f..8d525f27498f 100644 --- a/test/common/router/config_impl_test.cc +++ b/test/common/router/config_impl_test.cc @@ -3,8 +3,8 @@ #include "common/json/json_loader.h" #include "common/router/config_impl.h" -#include "test/mocks/upstream/mocks.h" #include "test/mocks/runtime/mocks.h" +#include "test/mocks/upstream/mocks.h" #include "test/test_common/utility.h" using testing::_; @@ -436,6 +436,20 @@ TEST(RouteMatcherTest, HeaderMatchedRouting) { {"name": "test_header_presence"} ] }, + { + "prefix": "/", + "cluster": "local_service_with_header_pattern_set_regex", + "headers" : [ + {"name": "test_header_pattern", "value": "^user=test-\\d+$", "regex": true} + ] + }, + { + "prefix": "/", + "cluster": "local_service_with_header_pattern_unset_regex", + "headers" : [ + {"name": "test_header_pattern", "value": "^customer=test-\\d+$"} + ] + }, { "prefix": "/", "cluster": "local_service_without_headers" @@ -484,6 +498,19 @@ TEST(RouteMatcherTest, HeaderMatchedRouting) { EXPECT_EQ("local_service_with_empty_headers", config.routeForRequest(headers, 0)->clusterName()); } + + { + Http::TestHeaderMapImpl headers = genHeaders("www.lyft.com", "/", "GET"); + headers.addViaCopy("test_header_pattern", "user=test-1223"); + EXPECT_EQ("local_service_with_header_pattern_set_regex", + config.routeForRequest(headers, 0)->clusterName()); + } + + { + Http::TestHeaderMapImpl headers = genHeaders("www.lyft.com", "/", "GET"); + headers.addViaCopy("test_header_pattern", "customer=test-1223"); + EXPECT_EQ("local_service_without_headers", config.routeForRequest(headers, 0)->clusterName()); + } } TEST(RouteMatcherTest, ContentType) { From 8641d2a67599b7d8c036dd9363f62d872244ee57 Mon Sep 17 00:00:00 2001 From: Roman Dzhabarov Date: Tue, 10 Jan 2017 08:12:17 -0800 Subject: [PATCH 06/22] Replace openssl with boringssl as the official ssl provider (#339) --- ci/build_container/build_container.sh | 22 +++++++++++++--------- docs/install/requirements.rst | 3 ++- thirdparty.cmake | 4 ++-- 3 files changed, 17 insertions(+), 12 deletions(-) diff --git a/ci/build_container/build_container.sh b/ci/build_container/build_container.sh index 19c1b68f9809..1c6e941681fa 100755 --- a/ci/build_container/build_container.sh +++ b/ci/build_container/build_container.sh @@ -5,6 +5,7 @@ set -e # Setup basic requirements and install them. apt-get update apt-get install -y wget software-properties-common make cmake git python python-pip clang-format-3.6 bc +apt-get install -y golang add-apt-repository -y ppa:ubuntu-toolchain-r/test apt-get update apt-get install -y g++-4.9 @@ -25,15 +26,6 @@ cd thirdparty export CC=gcc-4.9 export CXX=g++-4.9 -# openssl -wget https://www.openssl.org/source/openssl-1.0.2i.tar.gz -tar xf openssl-1.0.2i.tar.gz -cd openssl-1.0.2i -./config --prefix=$THIRDPARTY_BUILD -DPURIFY no-shared -make install -cd .. -rm -fr openssl* - # libevent wget https://github.com/libevent/libevent/releases/download/release-2.0.22-stable/libevent-2.0.22-stable.tar.gz tar xf libevent-2.0.22-stable.tar.gz @@ -43,6 +35,18 @@ make install cd .. rm -fr libevent* +# BoringSSL +git clone https://boringssl.googlesource.com/boringssl +cd boringssl +git reset --hard 78684e5b222645828ca302e56b40b9daff2b2d27 +cmake . +make +cp -r include/* $THIRDPARTY_BUILD/include +cp ssl/libssl.a $THIRDPARTY_BUILD/lib +cp crypto/libcrypto.a $THIRDPARTY_BUILD/lib +cd .. +rm -rf boringssl + # gperftools wget https://github.com/gperftools/gperftools/releases/download/gperftools-2.5/gperftools-2.5.tar.gz tar xf gperftools-2.5.tar.gz diff --git a/docs/install/requirements.rst b/docs/install/requirements.rst index 7c7db923da78..37cd5c5a47c1 100644 --- a/docs/install/requirements.rst +++ b/docs/install/requirements.rst @@ -16,7 +16,8 @@ Envoy has the following requirements: * `libevent `_ (last tested with 2.0.22) * `tclap `_ (last tested with 1.2.1) * `gperftools `_ (last tested with 2.5.0) -* `openssl `_ (last tested with 1.0.2i) +* `boringSSL `_ (last tested with sha 78684e5b222645828ca302e56b40b9daff2b2d27). + Envoy is built against BoringSSL but `openssl `_ should still work. * `protobuf `_ (last tested with 3.0.0) * `lightstep-tracer-cpp `_ (last tested with 0.19) * `rapidjson `_ (last tested with 1.1.0) diff --git a/thirdparty.cmake b/thirdparty.cmake index bfafe037a7ba..7d1ffe74e83f 100644 --- a/thirdparty.cmake +++ b/thirdparty.cmake @@ -30,8 +30,8 @@ set(ENVOY_TCLAP_INCLUDE_DIR "" CACHE FILEPATH "location of tclap includes") # Last tested with 2.5.0 set(ENVOY_GPERFTOOLS_INCLUDE_DIR "" CACHE FILEPATH "location of gperftools includes") -# https://www.openssl.org/ -# Last tested with 1.0.2i +# https://boringssl.googlesource.com/boringssl/+/chromium-stable +# Last tested with sha 78684e5b222645828ca302e56b40b9daff2b2d27 set(ENVOY_OPENSSL_INCLUDE_DIR "" CACHE FILEPATH "location of openssl includes") # https://github.com/google/protobuf From aa20a22bca23e4137b1629563a21c76d8ecd077a Mon Sep 17 00:00:00 2001 From: Matt Klein Date: Tue, 10 Jan 2017 09:33:34 -0800 Subject: [PATCH 07/22] stats: fix scope timer prefix (#340) Regresion from #334. I'm in the process of rewriting all of the scope stuff currently so I'm not going to bother with tests. I will add tests in the real change. --- source/common/stats/stats_scope_impl.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/source/common/stats/stats_scope_impl.h b/source/common/stats/stats_scope_impl.h index 45688cd43d2f..ca8dc526e792 100644 --- a/source/common/stats/stats_scope_impl.h +++ b/source/common/stats/stats_scope_impl.h @@ -10,10 +10,10 @@ class ScopeImpl : public Scope { // Stats::Scope void deliverHistogramToSinks(const std::string& name, uint64_t value) override { - parent_.deliverHistogramToSinks(name, value); + parent_.deliverHistogramToSinks(prefix_ + name, value); } void deliverTimingToSinks(const std::string& name, std::chrono::milliseconds ms) { - parent_.deliverTimingToSinks(name, ms); + parent_.deliverTimingToSinks(prefix_ + name, ms); } Counter& counter(const std::string& name) override; From fa5bbd93e758cc8da1f041eda9766d2537838bf7 Mon Sep 17 00:00:00 2001 From: Enrico Schiattarella Date: Tue, 10 Jan 2017 11:12:38 -0800 Subject: [PATCH 08/22] Move ConnectionHandler interface to namespace Network and incorporate other review comments --- docs/configuration/listeners/listeners.rst | 2 +- include/envoy/event/dispatcher.h | 6 ++-- .../{server => network}/connection_handler.h | 5 ++- source/common/event/dispatcher_impl.cc | 4 +-- source/common/event/dispatcher_impl.h | 6 ++-- source/common/network/listen_socket_impl.cc | 1 - source/common/network/listener_impl.cc | 4 +-- source/common/network/listener_impl.h | 14 ++++---- source/common/network/utility.cc | 1 - source/server/connection_handler_impl.h | 33 +++++++++---------- source/server/server.h | 2 +- source/server/worker.h | 2 +- test/common/network/connection_impl_test.cc | 2 +- test/common/network/listener_impl_test.cc | 2 +- test/common/network/proxy_protocol_test.cc | 2 +- test/common/ssl/connection_impl_test.cc | 6 ++-- test/mocks/event/mocks.h | 10 +++--- test/mocks/network/mocks.cc | 3 ++ test/mocks/network/mocks.h | 16 +++++++++ test/mocks/server/mocks.cc | 2 -- test/mocks/server/mocks.h | 17 ---------- test/server/connection_handler_test.cc | 4 +-- 22 files changed, 70 insertions(+), 74 deletions(-) rename include/envoy/{server => network}/connection_handler.h (98%) diff --git a/docs/configuration/listeners/listeners.rst b/docs/configuration/listeners/listeners.rst index 406333d80588..fc85905a08c9 100644 --- a/docs/configuration/listeners/listeners.rst +++ b/docs/configuration/listeners/listeners.rst @@ -32,7 +32,7 @@ port bind_to_port *(optional, boolean)* Whether the listener should bind to the port. A listener that doesn't bind - can only receive connections redirected from other listeners that set the use_origin_dst to + can only receive connections redirected from other listeners that set use_origin_dst parameter to true. Default is true. use_proxy_proto diff --git a/include/envoy/event/dispatcher.h b/include/envoy/event/dispatcher.h index 7227fbf9a57f..77690c927cd3 100644 --- a/include/envoy/event/dispatcher.h +++ b/include/envoy/event/dispatcher.h @@ -8,8 +8,8 @@ #include "envoy/network/dns.h" #include "envoy/network/listener.h" #include "envoy/network/listen_socket.h" +#include "envoy/network/connection_handler.h" #include "envoy/ssl/context.h" -#include "envoy/server/connection_handler.h" #include "envoy/stats/stats.h" namespace Event { @@ -82,7 +82,7 @@ class Dispatcher { * allow the listener to hand it off to the listener associated to the original port * @return Network::ListenerPtr a new listener that is owned by the caller. */ - virtual Network::ListenerPtr createListener(Server::ConnectionHandler& conn_handler, + virtual Network::ListenerPtr createListener(Network::ConnectionHandler& conn_handler, Network::ListenSocket& socket, Network::ListenerCallbacks& cb, Stats::Store& stats_store, bool bind_to_port, @@ -104,7 +104,7 @@ class Dispatcher { * allow the listener to hand it off to the listener associated to the original port * @return Network::ListenerPtr a new listener that is owned by the caller. */ - virtual Network::ListenerPtr createSslListener(Server::ConnectionHandler& conn_handler, + virtual Network::ListenerPtr createSslListener(Network::ConnectionHandler& conn_handler, Ssl::ServerContext& ssl_ctx, Network::ListenSocket& socket, Network::ListenerCallbacks& cb, diff --git a/include/envoy/server/connection_handler.h b/include/envoy/network/connection_handler.h similarity index 98% rename from include/envoy/server/connection_handler.h rename to include/envoy/network/connection_handler.h index ef293a85f0d3..61711dbbb628 100644 --- a/include/envoy/server/connection_handler.h +++ b/include/envoy/network/connection_handler.h @@ -6,12 +6,11 @@ #include "envoy/network/listen_socket.h" #include "envoy/ssl/context.h" -namespace Server { +namespace Network { /** * Abstract connection handler. */ - class ConnectionHandler { public: virtual ~ConnectionHandler(){}; @@ -65,4 +64,4 @@ class ConnectionHandler { typedef std::unique_ptr ConnectionHandlerPtr; -} // Server +} // Network diff --git a/source/common/event/dispatcher_impl.cc b/source/common/event/dispatcher_impl.cc index 3714d9456ee3..cbd79ecaad47 100644 --- a/source/common/event/dispatcher_impl.cc +++ b/source/common/event/dispatcher_impl.cc @@ -75,7 +75,7 @@ Filesystem::WatcherPtr DispatcherImpl::createFilesystemWatcher() { return Filesystem::WatcherPtr{new Filesystem::WatcherImpl(*this)}; } -Network::ListenerPtr DispatcherImpl::createListener(Server::ConnectionHandler& conn_handler, +Network::ListenerPtr DispatcherImpl::createListener(Network::ConnectionHandler& conn_handler, Network::ListenSocket& socket, Network::ListenerCallbacks& cb, Stats::Store& stats_store, bool bind_to_port, @@ -84,7 +84,7 @@ Network::ListenerPtr DispatcherImpl::createListener(Server::ConnectionHandler& c conn_handler, *this, socket, cb, stats_store, bind_to_port, use_proxy_proto, use_orig_dst)}; } -Network::ListenerPtr DispatcherImpl::createSslListener(Server::ConnectionHandler& conn_handler, +Network::ListenerPtr DispatcherImpl::createSslListener(Network::ConnectionHandler& conn_handler, Ssl::ServerContext& ssl_ctx, Network::ListenSocket& socket, Network::ListenerCallbacks& cb, diff --git a/source/common/event/dispatcher_impl.h b/source/common/event/dispatcher_impl.h index 0f75c4c3a57d..0dd27c2997e7 100644 --- a/source/common/event/dispatcher_impl.h +++ b/source/common/event/dispatcher_impl.h @@ -4,7 +4,7 @@ #include "envoy/event/deferred_deletable.h" #include "envoy/event/dispatcher.h" -#include "envoy/server/connection_handler.h" +#include "envoy/network/connection_handler.h" #include "common/common/logger.h" @@ -31,11 +31,11 @@ class DispatcherImpl : Logger::Loggable, public Dispatcher { Network::DnsResolverPtr createDnsResolver() override; FileEventPtr createFileEvent(int fd, FileReadyCb cb) override; Filesystem::WatcherPtr createFilesystemWatcher() override; - Network::ListenerPtr createListener(Server::ConnectionHandler& conn_handler, + Network::ListenerPtr createListener(Network::ConnectionHandler& conn_handler, Network::ListenSocket& socket, Network::ListenerCallbacks& cb, Stats::Store& stats_store, bool bind_to_port, bool use_proxy_proto, bool use_orig_dst) override; - Network::ListenerPtr createSslListener(Server::ConnectionHandler& conn_handler, + Network::ListenerPtr createSslListener(Network::ConnectionHandler& conn_handler, Ssl::ServerContext& ssl_ctx, Network::ListenSocket& socket, Network::ListenerCallbacks& cb, Stats::Store& stats_store, bool bind_to_port, bool use_proxy_proto, diff --git a/source/common/network/listen_socket_impl.cc b/source/common/network/listen_socket_impl.cc index 848ed0d0ff24..5f95aec70e01 100644 --- a/source/common/network/listen_socket_impl.cc +++ b/source/common/network/listen_socket_impl.cc @@ -8,7 +8,6 @@ namespace Network { TcpListenSocket::TcpListenSocket(uint32_t port, bool bind_to_port) : port_(port) { - AddrInfoPtr address = Utility::resolveTCP("", port); fd_ = socket(address->ai_addr->sa_family, SOCK_STREAM | SOCK_NONBLOCK, 0); RELEASE_ASSERT(fd_ != -1); diff --git a/source/common/network/listener_impl.cc b/source/common/network/listener_impl.cc index 1c6f60e774fe..1a83df171c10 100644 --- a/source/common/network/listener_impl.cc +++ b/source/common/network/listener_impl.cc @@ -9,7 +9,7 @@ #include "common/network/connection_impl.h" #include "common/ssl/connection_impl.h" -#include "envoy/server/connection_handler.h" +#include "envoy/network/connection_handler.h" #include "event2/listener.h" @@ -47,7 +47,7 @@ void ListenerImpl::listenCallback(evconnlistener*, evutil_socket_t fd, sockaddr* listener->newConnection(fd, addr); } -ListenerImpl::ListenerImpl(Server::ConnectionHandler& conn_handler, +ListenerImpl::ListenerImpl(Network::ConnectionHandler& conn_handler, Event::DispatcherImpl& dispatcher, ListenSocket& socket, ListenerCallbacks& cb, Stats::Store& stats_store, bool bind_to_port, bool use_proxy_proto, bool use_orig_dst) diff --git a/source/common/network/listener_impl.h b/source/common/network/listener_impl.h index 14acb63c09f8..dc267ea1fa10 100644 --- a/source/common/network/listener_impl.h +++ b/source/common/network/listener_impl.h @@ -4,7 +4,7 @@ #include "proxy_protocol.h" #include "envoy/network/listener.h" -#include "envoy/server/connection_handler.h" +#include "envoy/network/connection_handler.h" #include "common/event/dispatcher_impl.h" #include "common/event/libevent.h" @@ -18,7 +18,7 @@ namespace Network { */ class ListenerImpl : public Listener { public: - ListenerImpl(Server::ConnectionHandler& conn_handler, Event::DispatcherImpl& dispatcher, + ListenerImpl(Network::ConnectionHandler& conn_handler, Event::DispatcherImpl& dispatcher, ListenSocket& socket, ListenerCallbacks& cb, Stats::Store& stats_store, bool bind_to_port, bool use_proxy_proto, bool use_orig_dst); @@ -45,14 +45,14 @@ class ListenerImpl : public Listener { const std::string getAddressName(sockaddr* addr); uint16_t getAddressPort(sockaddr* addr); - Server::ConnectionHandler& connection_handler_; + Network::ConnectionHandler& connection_handler_; Event::DispatcherImpl& dispatcher_; ListenSocket& socket_; ListenerCallbacks& cb_; - bool bind_to_port_; - bool use_proxy_proto_; + const bool bind_to_port_; + const bool use_proxy_proto_; ProxyProtocol proxy_protocol_; - bool use_original_dst_; + const bool use_original_dst_; private: static void errorCallback(evconnlistener* listener, void* context); @@ -63,7 +63,7 @@ class ListenerImpl : public Listener { class SslListenerImpl : public ListenerImpl { public: - SslListenerImpl(Server::ConnectionHandler& conn_handler, Event::DispatcherImpl& dispatcher, + SslListenerImpl(Network::ConnectionHandler& conn_handler, Event::DispatcherImpl& dispatcher, Ssl::Context& ssl_ctx, ListenSocket& socket, ListenerCallbacks& cb, Stats::Store& stats_store, bool bind_to_port, bool use_proxy_proto, bool use_orig_dst) diff --git a/source/common/network/utility.cc b/source/common/network/utility.cc index 2ad331910853..be6467750f6b 100644 --- a/source/common/network/utility.cc +++ b/source/common/network/utility.cc @@ -224,7 +224,6 @@ bool Utility::isLoopbackAddress(const char* address) { } bool Utility::getOriginalDst(int fd, sockaddr_storage* orig_addr) { - socklen_t addr_len = sizeof(sockaddr_storage); int status = getsockopt(fd, SOL_IP, SO_ORIGINAL_DST, orig_addr, &addr_len); diff --git a/source/server/connection_handler_impl.h b/source/server/connection_handler_impl.h index 04fc529879b2..2a5bed23fbb7 100644 --- a/source/server/connection_handler_impl.h +++ b/source/server/connection_handler_impl.h @@ -1,10 +1,10 @@ #pragma once -#include "envoy/server/connection_handler.h" #include "envoy/api/api.h" #include "envoy/common/time.h" #include "envoy/event/deferred_deletable.h" #include "envoy/network/connection.h" +#include "envoy/network/connection_handler.h" #include "envoy/network/filter.h" #include "envoy/network/listener.h" #include "envoy/network/listen_socket.h" @@ -34,28 +34,13 @@ struct ListenerStats { * Server side connection handler. This is used both by workers as well as the * main thread for non-threaded listeners. */ -class ConnectionHandlerImpl : public ConnectionHandler, NonCopyable { +class ConnectionHandlerImpl : public Network::ConnectionHandler, NonCopyable { public: ConnectionHandlerImpl(Stats::Store& stats_store, spdlog::logger& logger, Api::ApiPtr&& api); ~ConnectionHandlerImpl(); Api::Api& api() { return *api_; } Event::Dispatcher& dispatcher() { return *dispatcher_; } - uint64_t numConnections() { return num_connections_; } - - void addListener(Network::FilterChainFactory& factory, Network::ListenSocket& socket, - bool bind_to_port, bool use_proxy_proto, bool use_orig_dst) override; - - void addSslListener(Network::FilterChainFactory& factory, Ssl::ServerContext& ssl_ctx, - Network::ListenSocket& socket, bool bind_to_port, bool use_proxy_proto, - bool use_orig_dst) override; - - Network::Listener* findListener(const std::string& socket_name) override; - - /** - * Close and destroy all listeners. - */ - void closeListeners() override; /** * Close and destroy all connections. @@ -68,6 +53,20 @@ class ConnectionHandlerImpl : public ConnectionHandler, NonCopyable { */ void startWatchdog(); + // Network::ConnectionHandler + uint64_t numConnections() override { return num_connections_; } + + void addListener(Network::FilterChainFactory& factory, Network::ListenSocket& socket, + bool bind_to_port, bool use_proxy_proto, bool use_orig_dst) override; + + void addSslListener(Network::FilterChainFactory& factory, Ssl::ServerContext& ssl_ctx, + Network::ListenSocket& socket, bool bind_to_port, bool use_proxy_proto, + bool use_orig_dst) override; + + Network::Listener* findListener(const std::string& socket_name) override; + + void closeListeners() override; + private: /** * Wrapper for an active listener owned by this worker. diff --git a/source/server/server.h b/source/server/server.h index 2045987c564a..5465d8a2f518 100644 --- a/source/server/server.h +++ b/source/server/server.h @@ -1,7 +1,7 @@ #pragma once -#include "worker.h" #include "connection_handler_impl.h" +#include "worker.h" #include "envoy/common/optional.h" #include "envoy/server/drain_manager.h" diff --git a/source/server/worker.h b/source/server/worker.h index ece92fd5d725..133648a7f9bc 100644 --- a/source/server/worker.h +++ b/source/server/worker.h @@ -20,7 +20,7 @@ class Worker : Logger::Loggable { ~Worker(); Event::Dispatcher& dispatcher() { return handler_->dispatcher(); } - Server::ConnectionHandler* handler() { return handler_.get(); } + Network::ConnectionHandler* handler() { return handler_.get(); } void initializeConfiguration(Server::Configuration::Main& config, const SocketMap& socket_map); /** diff --git a/test/common/network/connection_impl_test.cc b/test/common/network/connection_impl_test.cc index 93e77f11faef..950214050751 100644 --- a/test/common/network/connection_impl_test.cc +++ b/test/common/network/connection_impl_test.cc @@ -62,7 +62,7 @@ TEST(ConnectionImplTest, BufferStats) { Event::DispatcherImpl dispatcher; Network::TcpListenSocket socket(uint32_t(10000), true); Network::MockListenerCallbacks listener_callbacks; - Server::MockConnectionHandler connection_handler; + Network::MockConnectionHandler connection_handler; Network::ListenerPtr listener = dispatcher.createListener( connection_handler, socket, listener_callbacks, stats_store, true, false, false); diff --git a/test/common/network/listener_impl_test.cc b/test/common/network/listener_impl_test.cc index c619a9fc5c55..01ba09e5d1a8 100644 --- a/test/common/network/listener_impl_test.cc +++ b/test/common/network/listener_impl_test.cc @@ -16,7 +16,7 @@ static void errorCallbackTest() { Event::DispatcherImpl dispatcher; Network::TcpListenSocket socket(uint32_t(10000), true); Network::MockListenerCallbacks listener_callbacks; - Server::MockConnectionHandler connection_handler; + Network::MockConnectionHandler connection_handler; Network::ListenerPtr listener = dispatcher.createListener( connection_handler, socket, listener_callbacks, stats_store, true, false, false); diff --git a/test/common/network/proxy_protocol_test.cc b/test/common/network/proxy_protocol_test.cc index a9e8f593d60d..72d56b523b20 100644 --- a/test/common/network/proxy_protocol_test.cc +++ b/test/common/network/proxy_protocol_test.cc @@ -32,7 +32,7 @@ class ProxyProtocolTest : public testing::Test { TcpListenSocket socket_; Stats::IsolatedStoreImpl stats_store_; MockListenerCallbacks callbacks_; - Server::MockConnectionHandler connection_handler_; + Network::MockConnectionHandler connection_handler_; ListenerImpl listener_; ClientConnectionPtr conn_; NiceMock connection_callbacks_; diff --git a/test/common/ssl/connection_impl_test.cc b/test/common/ssl/connection_impl_test.cc index 25eddbbbb83a..4d3c810a66fd 100644 --- a/test/common/ssl/connection_impl_test.cc +++ b/test/common/ssl/connection_impl_test.cc @@ -35,7 +35,7 @@ TEST(SslConnectionImplTest, ClientAuth) { Event::DispatcherImpl dispatcher; Network::TcpListenSocket socket(uint32_t(10000), true); Network::MockListenerCallbacks callbacks; - Server::MockConnectionHandler connection_handler; + Network::MockConnectionHandler connection_handler; Network::ListenerPtr listener = dispatcher.createSslListener( connection_handler, *server_ctx, socket, callbacks, stats_store, true, false, false); @@ -95,7 +95,7 @@ TEST(SslConnectionImplTest, ClientAuthBadVerification) { Event::DispatcherImpl dispatcher; Network::TcpListenSocket socket(uint32_t(10000), true); Network::MockListenerCallbacks callbacks; - Server::MockConnectionHandler connection_handler; + Network::MockConnectionHandler connection_handler; Network::ListenerPtr listener = dispatcher.createSslListener( connection_handler, *server_ctx, socket, callbacks, stats_store, true, false, false); @@ -151,7 +151,7 @@ TEST(SslConnectionImplTest, SslError) { Event::DispatcherImpl dispatcher; Network::TcpListenSocket socket(uint32_t(10000), true); Network::MockListenerCallbacks callbacks; - Server::MockConnectionHandler connection_handler; + Network::MockConnectionHandler connection_handler; Network::ListenerPtr listener = dispatcher.createSslListener( connection_handler, *server_ctx, socket, callbacks, stats_store, true, false, false); diff --git a/test/mocks/event/mocks.h b/test/mocks/event/mocks.h index 847c5e2fcdbd..e98dae8467b0 100644 --- a/test/mocks/event/mocks.h +++ b/test/mocks/event/mocks.h @@ -9,7 +9,7 @@ #include "envoy/network/dns.h" #include "envoy/network/listener.h" #include "envoy/ssl/context.h" -#include "envoy/server/connection_handler.h" +#include "envoy/network/connection_handler.h" namespace Event { @@ -39,7 +39,7 @@ class MockDispatcher : public Dispatcher { return Filesystem::WatcherPtr{createFilesystemWatcher_()}; } - Network::ListenerPtr createListener(Server::ConnectionHandler& conn_handler, + Network::ListenerPtr createListener(Network::ConnectionHandler& conn_handler, Network::ListenSocket& socket, Network::ListenerCallbacks& cb, Stats::Store& stats_store, bool bind_to_port, bool use_proxy_proto, bool use_original_dst) override { @@ -47,7 +47,7 @@ class MockDispatcher : public Dispatcher { use_proxy_proto, use_original_dst)}; } - Network::ListenerPtr createSslListener(Server::ConnectionHandler& conn_handler, + Network::ListenerPtr createSslListener(Network::ConnectionHandler& conn_handler, Ssl::ServerContext& ssl_ctx, Network::ListenSocket& socket, Network::ListenerCallbacks& cb, Stats::Store& stats_store, bool bind_to_port, bool use_proxy_proto, @@ -79,12 +79,12 @@ class MockDispatcher : public Dispatcher { MOCK_METHOD2(createFileEvent_, FileEvent*(int fd, FileReadyCb cb)); MOCK_METHOD0(createFilesystemWatcher_, Filesystem::Watcher*()); MOCK_METHOD7(createListener_, - Network::Listener*(Server::ConnectionHandler& conn_handler, + Network::Listener*(Network::ConnectionHandler& conn_handler, Network::ListenSocket& socket, Network::ListenerCallbacks& cb, Stats::Store& stats_store, bool bind_to_port, bool use_proxy_proto, bool use_original_dst)); MOCK_METHOD8(createSslListener_, - Network::Listener*(Server::ConnectionHandler& conn_handler, + Network::Listener*(Network::ConnectionHandler& conn_handler, Ssl::ServerContext& ssl_ctx, Network::ListenSocket& socket, Network::ListenerCallbacks& cb, Stats::Store& stats_store, bool bind_to_port, bool use_proxy_proto, bool use_original_dst)); diff --git a/test/mocks/network/mocks.cc b/test/mocks/network/mocks.cc index c9923e278eee..278f58e440e2 100644 --- a/test/mocks/network/mocks.cc +++ b/test/mocks/network/mocks.cc @@ -113,4 +113,7 @@ MockListenSocket::~MockListenSocket() {} MockListener::MockListener() {} MockListener::~MockListener() {} +MockConnectionHandler::MockConnectionHandler() {} +MockConnectionHandler::~MockConnectionHandler() {} + } // Network diff --git a/test/mocks/network/mocks.h b/test/mocks/network/mocks.h index e574e955437e..60d05ac73ba9 100644 --- a/test/mocks/network/mocks.h +++ b/test/mocks/network/mocks.h @@ -199,4 +199,20 @@ class MockListener : public Listener { ~MockListener(); }; +class MockConnectionHandler : public ConnectionHandler { +public: + MockConnectionHandler(); + ~MockConnectionHandler(); + + MOCK_METHOD0(numConnections, uint64_t()); + MOCK_METHOD5(addListener, + void(Network::FilterChainFactory& factory, Network::ListenSocket& socket, + bool bind_to_port, bool use_proxy_proto, bool use_orig_dst)); + MOCK_METHOD6(addSslListener, void(Network::FilterChainFactory& factory, + Ssl::ServerContext& ssl_ctx, Network::ListenSocket& socket, + bool bind_to_port, bool use_proxy_proto, bool use_orig_dst)); + MOCK_METHOD1(findListener, Network::Listener*(const std::string& socket_name)); + MOCK_METHOD0(closeListeners, void()); +}; + } // Network diff --git a/test/mocks/server/mocks.cc b/test/mocks/server/mocks.cc index 049fa820f41b..9ff8f8ba01a1 100644 --- a/test/mocks/server/mocks.cc +++ b/test/mocks/server/mocks.cc @@ -40,6 +40,4 @@ MockInstance::MockInstance() : ssl_context_manager_(runtime_loader_) { MockInstance::~MockInstance() {} -MockConnectionHandler::MockConnectionHandler() {} -MockConnectionHandler::~MockConnectionHandler() {} } // Server diff --git a/test/mocks/server/mocks.h b/test/mocks/server/mocks.h index 519e99c0a78c..ebaf26c39229 100644 --- a/test/mocks/server/mocks.h +++ b/test/mocks/server/mocks.h @@ -4,7 +4,6 @@ #include "envoy/server/drain_manager.h" #include "envoy/server/instance.h" #include "envoy/server/options.h" -#include "envoy/server/connection_handler.h" #include "envoy/ssl/context_manager.h" #include "common/ssl/context_manager_impl.h" @@ -134,20 +133,4 @@ class MockInstance : public Instance { testing::NiceMock local_info_; }; -class MockConnectionHandler : public ConnectionHandler { -public: - MockConnectionHandler(); - ~MockConnectionHandler(); - - MOCK_METHOD0(numConnections, uint64_t()); - MOCK_METHOD5(addListener, - void(Network::FilterChainFactory& factory, Network::ListenSocket& socket, - bool bind_to_port, bool use_proxy_proto, bool use_orig_dst)); - MOCK_METHOD6(addSslListener, void(Network::FilterChainFactory& factory, - Ssl::ServerContext& ssl_ctx, Network::ListenSocket& socket, - bool bind_to_port, bool use_proxy_proto, bool use_orig_dst)); - MOCK_METHOD1(findListener, Network::Listener*(const std::string& socket_name)); - MOCK_METHOD0(closeListeners, void()); -}; - } // Server diff --git a/test/server/connection_handler_test.cc b/test/server/connection_handler_test.cc index 88d048da8dc0..1e582d0705b2 100644 --- a/test/server/connection_handler_test.cc +++ b/test/server/connection_handler_test.cc @@ -22,13 +22,13 @@ TEST_F(ConnectionHandlerTest, CloseDuringFilterChainCreate) { EXPECT_CALL(*api, allocateDispatcher_()).WillOnce(Return(dispatcher)); Server::ConnectionHandlerImpl handler(stats_store, log(), Api::ApiPtr{api}); Network::MockFilterChainFactory factory; - Server::MockConnectionHandler connection_handler; + Network::MockConnectionHandler connection_handler; NiceMock socket; Network::Listener* listener = new Network::MockListener(); Network::ListenerCallbacks* listener_callbacks; EXPECT_CALL(*dispatcher, createListener_(_, _, _, _, _, _, _)) - .WillOnce(Invoke([&](Server::ConnectionHandler&, Network::ListenSocket&, + .WillOnce(Invoke([&](Network::ConnectionHandler&, Network::ListenSocket&, Network::ListenerCallbacks& cb, Stats::Store&, bool, bool, bool) -> Network::Listener* { listener_callbacks = &cb; From 987bc7a7ea25809e42f3ff1c39cf779fc0543e1e Mon Sep 17 00:00:00 2001 From: Enrico Schiattarella Date: Wed, 11 Jan 2017 15:31:02 -0800 Subject: [PATCH 09/22] Fix bad merge --- test/common/router/config_impl_test.cc | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/test/common/router/config_impl_test.cc b/test/common/router/config_impl_test.cc index 6268eb1bf3a1..2ffcf1812c53 100644 --- a/test/common/router/config_impl_test.cc +++ b/test/common/router/config_impl_test.cc @@ -519,19 +519,6 @@ TEST(RouteMatcherTest, HeaderMatchedRouting) { EXPECT_EQ("local_service_without_headers", config.route(headers, 0)->routeEntry()->clusterName()); } - - { - Http::TestHeaderMapImpl headers = genHeaders("www.lyft.com", "/", "GET"); - headers.addViaCopy("test_header_pattern", "user=test-1223"); - EXPECT_EQ("local_service_with_header_pattern_set_regex", - config.routeForRequest(headers, 0)->clusterName()); - } - - { - Http::TestHeaderMapImpl headers = genHeaders("www.lyft.com", "/", "GET"); - headers.addViaCopy("test_header_pattern", "customer=test-1223"); - EXPECT_EQ("local_service_without_headers", config.routeForRequest(headers, 0)->clusterName()); - } } TEST(RouteMatcherTest, ContentType) { From d1c75f68e68c2086356c1c013d6db7f9c6fc4071 Mon Sep 17 00:00:00 2001 From: Enrico Schiattarella Date: Wed, 11 Jan 2017 21:00:27 -0800 Subject: [PATCH 10/22] Incorporate review comment --- source/common/network/listener_impl.cc | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/source/common/network/listener_impl.cc b/source/common/network/listener_impl.cc index 1a83df171c10..edc8594613b6 100644 --- a/source/common/network/listener_impl.cc +++ b/source/common/network/listener_impl.cc @@ -2,6 +2,7 @@ #include "utility.h" #include "envoy/common/exception.h" +#include "envoy/network/connection_handler.h" #include "common/common/empty_string.h" #include "common/event/dispatcher_impl.h" @@ -9,8 +10,6 @@ #include "common/network/connection_impl.h" #include "common/ssl/connection_impl.h" -#include "envoy/network/connection_handler.h" - #include "event2/listener.h" namespace Network { From 5e2c2b17b7c9e6af17ce56e9ead3f5d98d4a4290 Mon Sep 17 00:00:00 2001 From: Enrico Schiattarella Date: Mon, 23 Jan 2017 10:20:52 -0800 Subject: [PATCH 11/22] Fix handling of subnet "0.0.0.0/0" in whitelist. "0.0.0.0/0" is a well-known subnet that contains all possible IPv4 addresses. Envoy whitelist code currently does not handle the "/0" case correctly. The code uses this formula to compute the subnet mask: white_list_entry.ipv4_mask_ = ~((1 << (32 - mask)) - 1); The LHS is an int32_t and the RHS is also computed as a 32-bit value. However, the (<< 32) operation is invalid for a 32-bit value (undefined behavior). The fix makes the RHS first compute as a 64-bits value that later gets truncated to 32-bits on assignment. --- source/common/network/utility.cc | 2 +- test/common/network/utility_test.cc | 22 ++++++++++++++++++++++ 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/source/common/network/utility.cc b/source/common/network/utility.cc index be6467750f6b..de16b7319b64 100644 --- a/source/common/network/utility.cc +++ b/source/common/network/utility.cc @@ -37,7 +37,7 @@ IpWhiteList::IpWhiteList(const Json::Object& config) { Ipv4Entry white_list_entry; white_list_entry.ipv4_address_ = ntohl(addr.s_addr); - white_list_entry.ipv4_mask_ = ~((1 << (32 - mask)) - 1); + white_list_entry.ipv4_mask_ = ~((1ULL << (32 - mask)) - 1); // Check to make sure applying the mask to the address equals the address. This can prevent // user error. diff --git a/test/common/network/utility_test.cc b/test/common/network/utility_test.cc index 830b2c96583f..9b4226a7d3c1 100644 --- a/test/common/network/utility_test.cc +++ b/test/common/network/utility_test.cc @@ -83,6 +83,28 @@ TEST(IpWhiteListTest, Normal) { EXPECT_FALSE(wl.contains("")); } +TEST(IpWhiteListTest, MatchAny) { + std::string json = R"EOF( + { + "ip_white_list": [ + "0.0.0.0/0" + ] + } + )EOF"; + + Json::ObjectPtr loader = Json::Factory::LoadFromString(json); + IpWhiteList wl(*loader); + + EXPECT_TRUE(wl.contains("192.168.3.3")); + EXPECT_TRUE(wl.contains("192.168.3.0")); + EXPECT_TRUE(wl.contains("192.168.3.255")); + EXPECT_TRUE(wl.contains("192.168.0.0")); + EXPECT_TRUE(wl.contains("192.0.0.0")); + EXPECT_TRUE(wl.contains("1.1.1.1")); + + EXPECT_FALSE(wl.contains("")); +} + TEST(NetworkUtility, NonNumericResolve) { EXPECT_THROW(Utility::resolveTCP("localhost", 80), EnvoyException); } From becb250654dc9e00c39ee5a7e68078b61e20b66a Mon Sep 17 00:00:00 2001 From: Enrico Schiattarella Date: Mon, 23 Jan 2017 12:23:32 -0800 Subject: [PATCH 12/22] Add a comment regarding the fix for "0.0.0.0/0" subnet --- source/common/network/utility.cc | 3 +++ 1 file changed, 3 insertions(+) diff --git a/source/common/network/utility.cc b/source/common/network/utility.cc index de16b7319b64..54a9569b387c 100644 --- a/source/common/network/utility.cc +++ b/source/common/network/utility.cc @@ -34,9 +34,12 @@ IpWhiteList::IpWhiteList(const Json::Object& config) { throw EnvoyException( fmt::format("invalid ipv4/mask combo '{}' (mask bits must be <= 32)", entry)); } + // "0.0.0.0/0" is a valid subnet that contains all possible IPv4 addresses, so mask can be equal to 0 Ipv4Entry white_list_entry; white_list_entry.ipv4_address_ = ntohl(addr.s_addr); + // The 1ULL below makes sure that the RHS is computed as a 64-bit value, so that we do not over-shift + // to the left when mask = 0. The assignment to ipv4_mask_ then truncates the value back to 32 bits. white_list_entry.ipv4_mask_ = ~((1ULL << (32 - mask)) - 1); // Check to make sure applying the mask to the address equals the address. This can prevent From 2729237ae9435347035ba114dfda6958a4aaf4e8 Mon Sep 17 00:00:00 2001 From: Enrico Schiattarella Date: Mon, 23 Jan 2017 12:23:32 -0800 Subject: [PATCH 13/22] Add a comment regarding the fix for "0.0.0.0/0" subnet --- source/common/network/utility.cc | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/source/common/network/utility.cc b/source/common/network/utility.cc index de16b7319b64..377f4159c9df 100644 --- a/source/common/network/utility.cc +++ b/source/common/network/utility.cc @@ -29,6 +29,8 @@ IpWhiteList::IpWhiteList(const Json::Object& config) { throw EnvoyException(fmt::format("invalid ipv4/mask combo '{}' (invalid IP address)", entry)); } + // "0.0.0.0/0" is a valid subnet that contains all possible IPv4 addresses, + // so mask can be equal to 0 uint64_t mask; if (!StringUtil::atoul(parts[1].c_str(), mask) || mask > 32) { throw EnvoyException( @@ -37,6 +39,9 @@ IpWhiteList::IpWhiteList(const Json::Object& config) { Ipv4Entry white_list_entry; white_list_entry.ipv4_address_ = ntohl(addr.s_addr); + // The 1ULL below makes sure that the RHS is computed as a 64-bit value, so that we do not + // over-shift to the left when mask = 0. The assignment to ipv4_mask_ then truncates + // the value back to 32 bits. white_list_entry.ipv4_mask_ = ~((1ULL << (32 - mask)) - 1); // Check to make sure applying the mask to the address equals the address. This can prevent From d833ec8de587d49aac0097dd2b793ef47177a3d1 Mon Sep 17 00:00:00 2001 From: Enrico Schiattarella Date: Tue, 24 Jan 2017 17:16:36 -0800 Subject: [PATCH 14/22] Add routing capabilities to tcp_proxy Allows the tcp_proxy filter to pick the destination cluster based on a combination of L4 connection parameters (source/destination IP address/port) See https://github.com/lyft/envoy/issues/345 --- .../envoy_service_to_service.template.json | 10 +- .../network_filters/tcp_proxy_filter.rst | 14 +- .../tcp_proxy_filter_route_config.rst | 21 ++ include/envoy/network/connection.h | 19 ++ source/common/filter/tcp_proxy.cc | 90 ++++++- source/common/filter/tcp_proxy.h | 16 +- source/common/network/connection_impl.cc | 42 +++- source/common/network/connection_impl.h | 10 +- source/common/network/listener_impl.cc | 14 +- source/common/network/listener_impl.h | 5 +- source/common/network/proxy_protocol.cc | 5 +- source/common/network/utility.cc | 58 +++-- source/common/network/utility.h | 51 +++- source/common/ssl/connection_impl.cc | 9 +- source/common/ssl/connection_impl.h | 2 +- test/common/filter/tcp_proxy_test.cc | 233 +++++++++++++++++- test/common/network/connection_impl_test.cc | 2 +- .../network/filter_manager_impl_test.cc | 10 +- test/common/network/listener_impl_test.cc | 4 +- test/common/network/utility_test.cc | 38 +++ test/config/integration/server.json | 11 +- test/config/integration/server_http2.json | 11 +- test/mocks/network/mocks.h | 6 + 23 files changed, 614 insertions(+), 67 deletions(-) create mode 100644 docs/configuration/network_filters/tcp_proxy_filter_route_config.rst diff --git a/configs/envoy_service_to_service.template.json b/configs/envoy_service_to_service.template.json index fcbc4ae0f993..674e77ad52a6 100644 --- a/configs/envoy_service_to_service.template.json +++ b/configs/envoy_service_to_service.template.json @@ -245,8 +245,14 @@ "type": "read", "name": "tcp_proxy", "config": { - "cluster": "mongo_{{ key }}", - "stat_prefix": "mongo_{{ key }}" + "stat_prefix": "mongo_{{ key }}", + "route_config": { + "routes": [ + { + "cluster": "mongo_{{ key }}" + } + ] + } } }] }{% if not loop.last %},{% endif -%} diff --git a/docs/configuration/network_filters/tcp_proxy_filter.rst b/docs/configuration/network_filters/tcp_proxy_filter.rst index 39eb2cdc8e71..58d4f146322f 100644 --- a/docs/configuration/network_filters/tcp_proxy_filter.rst +++ b/docs/configuration/network_filters/tcp_proxy_filter.rst @@ -11,14 +11,13 @@ TCP proxy :ref:`architecture overview `. "type": "read", "name": "tcp_proxy", "config": { - "cluster": "...", - "stat_prefix": "..." + "stat_prefix": "...", + "route_config": "{...}" } } -cluster - *(required, string)* The :ref:`cluster manager ` cluster to connect - to when a new downstream network connection is received. +:ref:`route_config ` + *(required, object)* The route table for the filter. All filter instances must have a route table, even if it is empty. stat_prefix *(required, string)* The prefix to use when emitting :ref:`statistics @@ -39,3 +38,8 @@ statistics are rooted at *tcp..* with the following statistics: downstream_cx_tx_bytes_total, Counter, Total bytes written to the downstream connection. downstream_cx_tx_bytes_buffered, Gauge, Total bytes currently buffered to the downstream connection. + +.. toctree:: + :hidden: + + tcp_proxy_filter_route_config diff --git a/docs/configuration/network_filters/tcp_proxy_filter_route_config.rst b/docs/configuration/network_filters/tcp_proxy_filter_route_config.rst new file mode 100644 index 000000000000..b5d3ce77709b --- /dev/null +++ b/docs/configuration/network_filters/tcp_proxy_filter_route_config.rst @@ -0,0 +1,21 @@ +.. _config_network_filters_tcp_proxy_route_config: + +Route Configuration +=================== + +* TCP proxy :ref:`architecture overview `. +* TCP proxy :ref:`filter `. + +.. code-block:: json + + { + "routes": [] + } + +:ref:`routes ` + *(required, array)* An array of route entries that make up the route table. + +.. toctree:: + :hidden: + + tcp_proxy_filter_route diff --git a/include/envoy/network/connection.h b/include/envoy/network/connection.h index 50bdc0b97f6a..78a938712417 100644 --- a/include/envoy/network/connection.h +++ b/include/envoy/network/connection.h @@ -115,6 +115,25 @@ class Connection : public Event::DeferredDeletable, public FilterManager { */ virtual const std::string& remoteAddress() PURE; + /** + * @return The port number used by the remote client. + */ + virtual uint32_t remotePort() PURE; + + /** + * @return The address the remote client is trying to connect to. + * It can be different from the proxy address if the downstream connection + * has been redirected or the proxy is operating in transparent mode. + */ + virtual const std::string destinationAddress() PURE; + + /** + * @return The port number the remote client is trying to connect to. + * It can be different from the port the listener is listening on if the connection + * has been redirected or the proxy is operating in transparent mode. + */ + virtual uint32_t destinationPort() PURE; + /** * Set the buffer stats to update when the connection's read/write buffers change. Note that * for performance reasons these stats are eventually consistent and may not always accurately diff --git a/source/common/filter/tcp_proxy.cc b/source/common/filter/tcp_proxy.cc index 96c7cd1656eb..f8156fecee9f 100644 --- a/source/common/filter/tcp_proxy.cc +++ b/source/common/filter/tcp_proxy.cc @@ -8,19 +8,84 @@ #include "envoy/upstream/upstream.h" #include "common/common/assert.h" +#include "common/common/empty_string.h" #include "common/json/json_loader.h" namespace Filter { +TcpProxyConfig::Route::Route(const Json::Object& config) { + if (config.hasObject("cluster")) { + cluster_name_ = config.getString("cluster"); + } else { + throw EnvoyException(fmt::format("tcp proxy: route without cluster")); + } + + if (config.hasObject("source_ip_list")) { + source_ips_ = Network::IpList(config.getStringArray("source_ip_list")); + } + + if (config.hasObject("source_ports")) { + Network::Utility::parsePortRangeList(config.getString("source_ports"), source_port_ranges_); + } + + if (config.hasObject("destination_ip_list")) { + destination_ips_ = Network::IpList(config.getStringArray("destination_ip_list")); + } + + if (config.hasObject("destination_ports")) { + Network::Utility::parsePortRangeList(config.getString("destination_ports"), + destination_port_ranges_); + } +} + TcpProxyConfig::TcpProxyConfig(const Json::Object& config, Upstream::ClusterManager& cluster_manager, Stats::Store& stats_store) - : cluster_name_(config.getString("cluster")), - stats_(generateStats(config.getString("stat_prefix"), stats_store)) { - if (!cluster_manager.get(cluster_name_)) { - throw EnvoyException(fmt::format("tcp proxy: unknown cluster '{}'", cluster_name_)); + : stats_(generateStats(config.getString("stat_prefix"), stats_store)) { + if (!config.hasObject("route_config")) { + throw EnvoyException(fmt::format("tcp proxy: missing route config")); + } + + for (const Json::ObjectPtr& route_desc : + config.getObject("route_config")->getObjectArray("routes")) { + routes_.emplace_back(Route(*route_desc)); + + if (!cluster_manager.get(route_desc->getString("cluster"))) { + throw EnvoyException(fmt::format("tcp proxy: unknown cluster '{}' in TCP route", + route_desc->getString("cluster"))); + } } } +const std::string& TcpProxyConfig::getClusterForConnection(Network::Connection& connection) { + for (const TcpProxyConfig::Route& route : routes_) { + if (!route.source_port_ranges_.empty() && + !Network::Utility::portInRangeList(connection.remotePort(), route.source_port_ranges_)) { + continue; // no match, try next route + } + + if (!route.source_ips_.empty() && !route.source_ips_.contains(connection.remoteAddress())) { + continue; // no match, try next route + } + + if (!route.destination_port_ranges_.empty() && + !Network::Utility::portInRangeList(connection.destinationPort(), + route.destination_port_ranges_)) { + continue; // no match, try next route + } + + if (!route.destination_ips_.empty() && + !route.destination_ips_.contains(connection.destinationAddress())) { + continue; // no match, try next route + } + + // if we made it past all checks, the route matches + return route.cluster_name_; + } + + // no match, no more routes to try + return EMPTY_STRING; +} + TcpProxy::TcpProxy(TcpProxyConfigPtr config, Upstream::ClusterManager& cluster_manager) : config_(config), cluster_manager_(cluster_manager), downstream_callbacks_(*this), upstream_callbacks_(new UpstreamCallbacks(*this)) {} @@ -56,14 +121,27 @@ void TcpProxy::initializeReadFilterCallbacks(Network::ReadFilterCallbacks& callb } Network::FilterStatus TcpProxy::initializeUpstreamConnection() { - Upstream::ClusterInfoPtr cluster = cluster_manager_.get(config_->clusterName()); + const std::string& destination_cluster = + config_->getClusterForConnection(read_callbacks_->connection()); + conn_log_debug("Connection from {}", read_callbacks_->connection(), destination_cluster); + + Upstream::ClusterInfoPtr cluster = cluster_manager_.get(destination_cluster); + if (cluster) { + conn_log_debug("Connection cluster with name {} found", read_callbacks_->connection(), + destination_cluster); + } else { + conn_log_debug("Connection cluster with name {} NOT FOUND", read_callbacks_->connection(), + destination_cluster); + return Network::FilterStatus::StopIteration; + } + if (!cluster->resourceManager(Upstream::ResourcePriority::Default).connections().canCreate()) { cluster->stats().upstream_cx_overflow_.inc(); read_callbacks_->connection().close(Network::ConnectionCloseType::NoFlush); return Network::FilterStatus::StopIteration; } Upstream::Host::CreateConnectionData conn_info = - cluster_manager_.tcpConnForCluster(config_->clusterName()); + cluster_manager_.tcpConnForCluster(destination_cluster); upstream_connection_ = std::move(conn_info.connection_); read_callbacks_->upstreamHost(conn_info.host_description_); diff --git a/source/common/filter/tcp_proxy.h b/source/common/filter/tcp_proxy.h index 898686076983..9855c6c35959 100644 --- a/source/common/filter/tcp_proxy.h +++ b/source/common/filter/tcp_proxy.h @@ -9,6 +9,7 @@ #include "common/common/logger.h" #include "common/json/json_loader.h" #include "common/network/filter_impl.h" +#include "common/network/utility.h" namespace Filter { @@ -38,13 +39,24 @@ class TcpProxyConfig { TcpProxyConfig(const Json::Object& config, Upstream::ClusterManager& cluster_manager, Stats::Store& stats_store); - const std::string& clusterName() { return cluster_name_; } + const std::string& getClusterForConnection(Network::Connection& connection); + const TcpProxyStats& stats() { return stats_; } private: + struct Route { + Route(const Json::Object& config); + + Network::IpList source_ips_; + Network::PortRangeList source_port_ranges_; + Network::IpList destination_ips_; + Network::PortRangeList destination_port_ranges_; + std::string cluster_name_; + }; + static TcpProxyStats generateStats(const std::string& name, Stats::Store& store); - std::string cluster_name_; + std::list routes_; const TcpProxyStats stats_; }; diff --git a/source/common/network/connection_impl.cc b/source/common/network/connection_impl.cc index 1e35f7fa98f0..70254b4d7ae4 100644 --- a/source/common/network/connection_impl.cc +++ b/source/common/network/connection_impl.cc @@ -1,4 +1,5 @@ #include "connection_impl.h" +#include "utility.h" #include "envoy/event/timer.h" #include "envoy/common/exception.h" @@ -33,9 +34,9 @@ void ConnectionImplUtility::updateBufferStats(uint64_t delta, uint64_t new_total std::atomic ConnectionImpl::next_global_id_; ConnectionImpl::ConnectionImpl(Event::DispatcherImpl& dispatcher, int fd, - const std::string& remote_address) - : filter_manager_(*this, *this), remote_address_(remote_address), dispatcher_(dispatcher), - fd_(fd), id_(++next_global_id_) { + const std::string& remote_address, uint32_t remote_port) + : filter_manager_(*this, *this), remote_address_(remote_address), remote_port_(remote_port), + dispatcher_(dispatcher), fd_(fd), id_(++next_global_id_) { // Treat the lack of a valid fd (which in practice only happens if we run out of FDs) as an OOM // condition and just crash. @@ -395,9 +396,35 @@ void ConnectionImpl::updateWriteBufferStats(uint64_t num_written, uint64_t new_s buffer_stats_->write_current_); } +const std::string Network::ConnectionImpl::destinationAddress() { + if (fd_ != -1) { + sockaddr_storage orig_dst_addr; + memset(&orig_dst_addr, 0, sizeof(orig_dst_addr)); + bool success = Utility::getOriginalDst(fd_, &orig_dst_addr); + if (success) { + return Utility::getAddressName(reinterpret_cast(&orig_dst_addr)); + } + } + + return EMPTY_STRING; +} + +uint32_t Network::ConnectionImpl::destinationPort() { + if (fd_ != -1) { + sockaddr_storage orig_dst_addr; + memset(&orig_dst_addr, 0, sizeof(orig_dst_addr)); + bool success = Utility::getOriginalDst(fd_, &orig_dst_addr); + if (success) { + return Utility::getAddressPort(reinterpret_cast(&orig_dst_addr)); + } + } + + return 0; +} + ClientConnectionImpl::ClientConnectionImpl(Event::DispatcherImpl& dispatcher, int fd, - const std::string& url) - : ConnectionImpl(dispatcher, fd, url) {} + const std::string& url, uint32_t port) + : ConnectionImpl(dispatcher, fd, url, port) {} Network::ClientConnectionPtr ClientConnectionImpl::create(Event::DispatcherImpl& dispatcher, const std::string& url) { @@ -412,7 +439,8 @@ Network::ClientConnectionPtr ClientConnectionImpl::create(Event::DispatcherImpl& TcpClientConnectionImpl::TcpClientConnectionImpl(Event::DispatcherImpl& dispatcher, const std::string& url) - : ClientConnectionImpl(dispatcher, socket(AF_INET, SOCK_STREAM | SOCK_NONBLOCK, 0), url) {} + : ClientConnectionImpl(dispatcher, socket(AF_INET, SOCK_STREAM | SOCK_NONBLOCK, 0), url, + Network::Utility::portFromUrl(url)) {} void TcpClientConnectionImpl::connect() { AddrInfoPtr addr_info = Utility::resolveTCP(Utility::hostFromUrl(remote_address_), @@ -422,7 +450,7 @@ void TcpClientConnectionImpl::connect() { UdsClientConnectionImpl::UdsClientConnectionImpl(Event::DispatcherImpl& dispatcher, const std::string& url) - : ClientConnectionImpl(dispatcher, socket(AF_UNIX, SOCK_STREAM | SOCK_NONBLOCK, 0), url) {} + : ClientConnectionImpl(dispatcher, socket(AF_UNIX, SOCK_STREAM | SOCK_NONBLOCK, 0), url, 0) {} void UdsClientConnectionImpl::connect() { sockaddr_un addr = Utility::resolveUnixDomainSocket(Utility::pathFromUrl(remote_address_)); diff --git a/source/common/network/connection_impl.h b/source/common/network/connection_impl.h index f0e3151728af..53951c9661cb 100644 --- a/source/common/network/connection_impl.h +++ b/source/common/network/connection_impl.h @@ -37,7 +37,8 @@ class ConnectionImpl : public virtual Connection, public BufferSource, protected Logger::Loggable { public: - ConnectionImpl(Event::DispatcherImpl& dispatcher, int fd, const std::string& remote_address); + ConnectionImpl(Event::DispatcherImpl& dispatcher, int fd, const std::string& remote_address, + uint32_t remote_port); ~ConnectionImpl(); // Network::FilterManager @@ -56,6 +57,9 @@ class ConnectionImpl : public virtual Connection, void readDisable(bool disable) override; bool readEnabled() override; const std::string& remoteAddress() override { return remote_address_; } + uint32_t remotePort() override { return remote_port_; }; + const std::string destinationAddress() override; + uint32_t destinationPort() override; void setBufferStats(const BufferStats& stats) override; Ssl::Connection* ssl() override { return nullptr; } State state() override; @@ -79,6 +83,7 @@ class ConnectionImpl : public virtual Connection, FilterManagerImpl filter_manager_; const std::string remote_address_; + uint32_t remote_port_; Buffer::OwnedImpl read_buffer_; Buffer::OwnedImpl write_buffer_; @@ -122,7 +127,8 @@ class ConnectionImpl : public virtual Connection, */ class ClientConnectionImpl : public ConnectionImpl, virtual public ClientConnection { public: - ClientConnectionImpl(Event::DispatcherImpl& dispatcher, int fd, const std::string& url); + ClientConnectionImpl(Event::DispatcherImpl& dispatcher, int fd, const std::string& url, + uint32_t port); static Network::ClientConnectionPtr create(Event::DispatcherImpl& dispatcher, const std::string& url); diff --git a/source/common/network/listener_impl.cc b/source/common/network/listener_impl.cc index cc95ca1baabf..1535b97f7fd5 100644 --- a/source/common/network/listener_impl.cc +++ b/source/common/network/listener_impl.cc @@ -77,12 +77,12 @@ void ListenerImpl::newConnection(int fd, sockaddr* addr) { if (use_proxy_proto_) { proxy_protocol_.newConnection(dispatcher_, fd, *this); } else { - newConnection(fd, getAddressName(addr)); + newConnection(fd, getAddressName(addr), getAddressPort(addr)); } } -void ListenerImpl::newConnection(int fd, const std::string& remote_address) { - ConnectionPtr new_connection(new ConnectionImpl(dispatcher_, fd, remote_address)); +void ListenerImpl::newConnection(int fd, const std::string& remote_address, uint32_t remote_port) { + ConnectionPtr new_connection(new ConnectionImpl(dispatcher_, fd, remote_address, remote_port)); cb_.onNewConnection(std::move(new_connection)); } @@ -90,12 +90,14 @@ void SslListenerImpl::newConnection(int fd, sockaddr* addr) { if (use_proxy_proto_) { proxy_protocol_.newConnection(dispatcher_, fd, *this); } else { - newConnection(fd, getAddressName(addr)); + newConnection(fd, getAddressName(addr), getAddressPort(addr)); } } -void SslListenerImpl::newConnection(int fd, const std::string& remote_address) { - ConnectionPtr new_connection(new Ssl::ConnectionImpl(dispatcher_, fd, remote_address, ssl_ctx_, +void SslListenerImpl::newConnection(int fd, const std::string& remote_address, + uint32_t remote_port) { + ConnectionPtr new_connection(new Ssl::ConnectionImpl(dispatcher_, fd, remote_address, remote_port, + ssl_ctx_, Ssl::ConnectionImpl::InitialState::Server)); cb_.onNewConnection(std::move(new_connection)); } diff --git a/source/common/network/listener_impl.h b/source/common/network/listener_impl.h index d82103970b24..1092166a4792 100644 --- a/source/common/network/listener_impl.h +++ b/source/common/network/listener_impl.h @@ -33,8 +33,9 @@ class ListenerImpl : public Listener { * Accept/process a new connection with the given remote address. * @param fd supplies the new connection's fd. * @param remote_address supplies the remote address for the new connection. + * @param remote_address supplies the remote port for the new connection. */ - virtual void newConnection(int fd, const std::string& remote_address); + virtual void newConnection(int fd, const std::string& remote_address, uint32_t remote_port); /** * @return the socket supplied to the listener at construction time @@ -73,7 +74,7 @@ class SslListenerImpl : public ListenerImpl { // ListenerImpl void newConnection(int fd, sockaddr* addr) override; - void newConnection(int fd, const std::string& remote_address) override; + void newConnection(int fd, const std::string& remote_address, uint32_t remote_port) override; private: Ssl::Context& ssl_ctx_; diff --git a/source/common/network/proxy_protocol.cc b/source/common/network/proxy_protocol.cc index 094317146c85..1a0f856f82e5 100644 --- a/source/common/network/proxy_protocol.cc +++ b/source/common/network/proxy_protocol.cc @@ -68,7 +68,10 @@ void ProxyProtocol::ActiveConnection::onReadWorker() { removeFromList(parent_.connections_); - listener.newConnection(fd, remote_address); + // Technically we could extract the remote port from the PROXY protocol header + // and pass it to the listener. However, the listener does not care about + // client-side ports for downstream connections, so we can just pass a 0 + listener.newConnection(fd, remote_address, 0); } void ProxyProtocol::ActiveConnection::close() { diff --git a/source/common/network/utility.cc b/source/common/network/utility.cc index 377f4159c9df..797c62a7e914 100644 --- a/source/common/network/utility.cc +++ b/source/common/network/utility.cc @@ -11,12 +11,8 @@ namespace Network { -IpWhiteList::IpWhiteList(const Json::Object& config) { - if (!config.hasObject("ip_white_list")) { - return; - } - - for (const std::string& entry : config.getStringArray("ip_white_list")) { +IpList::IpList(const std::vector& subnets) { + for (const std::string& entry : subnets) { std::vector parts = StringUtil::split(entry, '/'); if (parts.size() != 2) { throw EnvoyException( @@ -37,33 +33,32 @@ IpWhiteList::IpWhiteList(const Json::Object& config) { fmt::format("invalid ipv4/mask combo '{}' (mask bits must be <= 32)", entry)); } - Ipv4Entry white_list_entry; - white_list_entry.ipv4_address_ = ntohl(addr.s_addr); + Ipv4Entry list_entry; + list_entry.ipv4_address_ = ntohl(addr.s_addr); // The 1ULL below makes sure that the RHS is computed as a 64-bit value, so that we do not // over-shift to the left when mask = 0. The assignment to ipv4_mask_ then truncates // the value back to 32 bits. - white_list_entry.ipv4_mask_ = ~((1ULL << (32 - mask)) - 1); + list_entry.ipv4_mask_ = ~((1ULL << (32 - mask)) - 1); // Check to make sure applying the mask to the address equals the address. This can prevent // user error. - if ((white_list_entry.ipv4_address_ & white_list_entry.ipv4_mask_) != - white_list_entry.ipv4_address_) { + if ((list_entry.ipv4_address_ & list_entry.ipv4_mask_) != list_entry.ipv4_address_) { throw EnvoyException( fmt::format("invalid ipv4/mask combo '{}' ((address & mask) != address)", entry)); } - ipv4_white_list_.push_back(white_list_entry); + ipv4_list_.push_back(list_entry); } } -bool IpWhiteList::contains(const std::string& remote_address) const { +bool IpList::contains(const std::string& remote_address) const { in_addr addr; int rc = inet_pton(AF_INET, remote_address.c_str(), &addr); if (1 != rc) { return false; } - for (const Ipv4Entry& entry : ipv4_white_list_) { + for (const Ipv4Entry& entry : ipv4_list_) { if ((ntohl(addr.s_addr) & entry.ipv4_mask_) == entry.ipv4_address_) { return true; } @@ -72,6 +67,10 @@ bool IpWhiteList::contains(const std::string& remote_address) const { return false; } +IpWhiteList::IpWhiteList(const Json::Object& config) + : ip_list_(config.hasObject("ip_white_list") ? config.getStringArray("ip_white_list") + : std::vector()) {} + const std::string Utility::TCP_SCHEME = "tcp://"; const std::string Utility::UNIX_SCHEME = "unix://"; @@ -235,4 +234,35 @@ bool Utility::getOriginalDst(int fd, sockaddr_storage* orig_addr) { return (status == 0); } +void Utility::parsePortRangeList(const std::string& string, std::list& list) { + std::vector ranges = StringUtil::split(string.c_str(), ','); + for (const std::string& s : ranges) { + uint32_t min = 0; + uint32_t max = 0; + char dash = 0; + + std::stringstream ss(s); + + if (s.find('-') != std::string::npos) { + ss >> min; + ss >> dash; + ss >> max; + } else { + ss >> min; + max = min; + } + + list.emplace_back(PortRange(min, max)); + } +} + +bool Utility::portInRangeList(uint32_t port, const std::list& list) { + for (const Network::PortRange& p : list) { + if (p.contains(port)) { + return true; + } + } + return false; +} + } // Network diff --git a/source/common/network/utility.h b/source/common/network/utility.h index 85f5044c729a..2c1b3620fd4a 100644 --- a/source/common/network/utility.h +++ b/source/common/network/utility.h @@ -16,11 +16,13 @@ namespace Network { * Utility class for keeping a list of IPV4 addresses and masks, and then determining whether an * IP address is in the address/mask list. */ -class IpWhiteList { +class IpList { public: - IpWhiteList(const Json::Object& config); + IpList(const std::vector& subnets); + IpList(){}; - bool contains(const std::string& remote_address) const; + bool contains(const std::string& address) const; + bool empty() const { return ipv4_list_.empty(); } private: struct Ipv4Entry { @@ -28,9 +30,34 @@ class IpWhiteList { uint32_t ipv4_mask_; }; - std::vector ipv4_white_list_; + std::vector ipv4_list_; +}; + +class IpWhiteList { +public: + IpWhiteList(const Json::Object& config); + bool contains(const std::string& address) const { return ip_list_.contains(address); } + +private: + IpList ip_list_; +}; + +/** + * Utility class to represent TCP/UDP port range + */ +class PortRange { +public: + PortRange(uint32_t min, uint32_t max) : min_(min), max_(max) {} + + bool contains(uint32_t port) const { return (port >= min_ && port <= max_); } + +private: + uint32_t min_; + uint32_t max_; }; +typedef std::list PortRangeList; + /** * Common network utility routines. */ @@ -130,6 +157,22 @@ class Utility { * @return true if the operation succeeded, false otherwise */ static bool getOriginalDst(int fd, sockaddr_storage* orig_addr); + + /** + * Parses a string containing a comma-separated list of port numbers and/or + * port ranges and appends the values to a caller-provided list of PortRange structures. + * @param str is the string containing the port numbers and ranges + * @param list is the list to append the new data structures to + */ + static void parsePortRangeList(const std::string& string, std::list& list); + + /** + * Checks whether a given port number appears in at least one of the port ranges in a list + * @param port is the port number to search + * @param list the list of port ranges in which the port may appear + * @return whether the port appears in at least one of the ranges in the list + */ + static bool portInRangeList(uint32_t port, const std::list& list); }; } // Network diff --git a/source/common/ssl/connection_impl.cc b/source/common/ssl/connection_impl.cc index 901e49e4268a..77f402a3068d 100644 --- a/source/common/ssl/connection_impl.cc +++ b/source/common/ssl/connection_impl.cc @@ -10,8 +10,9 @@ namespace Ssl { ConnectionImpl::ConnectionImpl(Event::DispatcherImpl& dispatcher, int fd, - const std::string& remote_address, Context& ctx, InitialState state) - : Network::ConnectionImpl(dispatcher, fd, remote_address), + const std::string& remote_address, uint32_t remote_port, + Context& ctx, InitialState state) + : Network::ConnectionImpl(dispatcher, fd, remote_address, remote_port), ctx_(dynamic_cast(ctx)), ssl_(ctx_.newSsl()) { BIO* bio = BIO_new_socket(fd, 0); SSL_set_bio(ssl_.get(), bio, bio); @@ -187,8 +188,8 @@ std::string ConnectionImpl::sha256PeerCertificateDigest() { ClientConnectionImpl::ClientConnectionImpl(Event::DispatcherImpl& dispatcher, Context& ctx, const std::string& url) - : ConnectionImpl(dispatcher, socket(AF_INET, SOCK_STREAM | SOCK_NONBLOCK, 0), url, ctx, - InitialState::Client) {} + : ConnectionImpl(dispatcher, socket(AF_INET, SOCK_STREAM | SOCK_NONBLOCK, 0), url, + Network::Utility::portFromUrl(url), ctx, InitialState::Client) {} void ClientConnectionImpl::connect() { Network::AddrInfoPtr addr_info = diff --git a/source/common/ssl/connection_impl.h b/source/common/ssl/connection_impl.h index 0f38d29275b9..ea0118818786 100644 --- a/source/common/ssl/connection_impl.h +++ b/source/common/ssl/connection_impl.h @@ -11,7 +11,7 @@ class ConnectionImpl : public Network::ConnectionImpl, public Connection { enum class InitialState { Client, Server }; ConnectionImpl(Event::DispatcherImpl& dispatcher, int fd, const std::string& remote_address, - Context& ctx, InitialState state); + uint32_t remote_port, Context& ctx, InitialState state); ~ConnectionImpl(); // Network::Connection diff --git a/test/common/filter/tcp_proxy_test.cc b/test/common/filter/tcp_proxy_test.cc index 68e322081f39..53b4b4b23244 100644 --- a/test/common/filter/tcp_proxy_test.cc +++ b/test/common/filter/tcp_proxy_test.cc @@ -12,18 +12,39 @@ using testing::_; using testing::NiceMock; using testing::Return; +using testing::ReturnRefOfCopy; using testing::SaveArg; namespace Filter { -TEST(TcpProxyConfigTest, NoCluster) { +TEST(TcpProxyConfigTest, NoRouteConfig) { std::string json = R"EOF( { - "cluster": "fake_cluster", "stat_prefix": "name" } )EOF"; + Json::ObjectPtr config = Json::Factory::LoadFromString(json); + NiceMock cluster_manager; + EXPECT_THROW( + TcpProxyConfig(*config, cluster_manager, cluster_manager.cluster_.info_->stats_store_), + EnvoyException); +} + +TEST(TcpProxyConfigTest, NoCluster) { + std::string json = R"EOF( + { + "stat_prefix": "name", + "route_config": { + "routes": [ + { + "cluster": "fake_cluster" + } + ] + } + } + )EOF"; + Json::ObjectPtr config = Json::Factory::LoadFromString(json); NiceMock cluster_manager; EXPECT_CALL(cluster_manager, get("fake_cluster")).WillOnce(Return(nullptr)); @@ -32,13 +53,217 @@ TEST(TcpProxyConfigTest, NoCluster) { EnvoyException); } +TEST(TcpProxyConfigTest, Routes) { + std::string json = R"EOF( + { + "stat_prefix": "name", + "route_config": { + "routes": [ + { + "destination_ip_list": [ + "10.10.10.10/32", + "10.10.11.0/24", + "10.11.0.0/16", + "11.0.0.0/8", + "128.0.0.0/1" + ], + "cluster": "with_destination_ip_list" + }, + { + "destination_ports": "1-1024,2048-4096,12345", + "cluster": "with_destination_ports" + }, + { + "source_ports": "23457,23459", + "cluster": "with_source_ports" + }, + { + "destination_ip_list": [ + "10.0.0.0/24" + ], + "source_ip_list": [ + "20.0.0.0/24" + ], + "destination_ports" : "10000", + "source_ports": "20000", + "cluster": "with_everything" + }, + { + "cluster": "catch_all" + } + ] + } + } + )EOF"; + + Json::ObjectPtr json_config = Json::Factory::LoadFromString(json); + NiceMock cm_; + + // The TcpProxyConfig constructor checks if the clusters mentioned in the route_config are valid. + // We need to make sure to return a non-null pointer for each, otherwise the constructor will + // throw an exception and fail. + EXPECT_CALL(cm_, get("with_destination_ip_list")).WillRepeatedly(Return(cm_.cluster_.info_)); + EXPECT_CALL(cm_, get("with_destination_ports")).WillRepeatedly(Return(cm_.cluster_.info_)); + EXPECT_CALL(cm_, get("with_source_ports")).WillRepeatedly(Return(cm_.cluster_.info_)); + EXPECT_CALL(cm_, get("with_everything")).WillRepeatedly(Return(cm_.cluster_.info_)); + EXPECT_CALL(cm_, get("catch_all")).WillRepeatedly(Return(cm_.cluster_.info_)); + + TcpProxyConfig config_obj(*json_config, cm_, cm_.cluster_.info_->stats_store_); + + { + // hit route with destination_ip (10.10.10.10/32) + NiceMock connection; + EXPECT_CALL(connection, destinationAddress()).WillRepeatedly(Return("10.10.10.10")); + EXPECT_EQ(std::string("with_destination_ip_list"), config_obj.getClusterForConnection(connection)); + } + + { + // fall-through + NiceMock connection; + EXPECT_CALL(connection, destinationAddress()).WillRepeatedly(Return("10.10.10.11")); + EXPECT_EQ(std::string("catch_all"), config_obj.getClusterForConnection(connection)); + } + + { + // hit route with destination_ip (10.10.11.0/24) + NiceMock connection; + EXPECT_CALL(connection, destinationAddress()).WillRepeatedly(Return("10.10.11.11")); + EXPECT_EQ(std::string("with_destination_ip_list"), config_obj.getClusterForConnection(connection)); + } + + { + // fall-through + NiceMock connection; + EXPECT_CALL(connection, destinationAddress()).WillRepeatedly(Return("10.10.12.12")); + EXPECT_EQ(std::string("catch_all"), config_obj.getClusterForConnection(connection)); + } + + { + // hit route with destination_ip (10.11.0.0/16) + NiceMock connection; + EXPECT_CALL(connection, destinationAddress()).WillRepeatedly(Return("10.11.11.11")); + EXPECT_EQ(std::string("with_destination_ip_list"), config_obj.getClusterForConnection(connection)); + } + + { + // fall-through + NiceMock connection; + EXPECT_CALL(connection, destinationAddress()).WillRepeatedly(Return("10.12.12.12")); + EXPECT_EQ(std::string("catch_all"), config_obj.getClusterForConnection(connection)); + } + + { + // hit route with destination_ip (11.0.0.0/8) + NiceMock connection; + EXPECT_CALL(connection, destinationAddress()).WillRepeatedly(Return("11.11.11.11")); + EXPECT_EQ(std::string("with_destination_ip_list"), config_obj.getClusterForConnection(connection)); + } + + { + // fall-through + NiceMock connection; + EXPECT_CALL(connection, destinationAddress()).WillRepeatedly(Return("12.12.12.12")); + EXPECT_EQ(std::string("catch_all"), config_obj.getClusterForConnection(connection)); + } + + { + // hit route with destination_ip (128.0.0.0/8) + NiceMock connection; + EXPECT_CALL(connection, destinationAddress()).WillRepeatedly(Return("128.255.255.255")); + EXPECT_EQ(std::string("with_destination_ip_list"), config_obj.getClusterForConnection(connection)); + } + + { + // hit route with destination port range + NiceMock connection; + EXPECT_CALL(connection, destinationAddress()).WillRepeatedly(Return("1.2.3.4")); + EXPECT_CALL(connection, destinationPort()).WillRepeatedly(Return(12345)); + EXPECT_EQ(std::string("with_destination_ports"), + config_obj.getClusterForConnection(connection)); + } + + { + // fall through + NiceMock connection; + EXPECT_CALL(connection, destinationAddress()).WillRepeatedly(Return("1.2.3.4")); + EXPECT_CALL(connection, destinationPort()).WillRepeatedly(Return(23456)); + EXPECT_EQ(std::string("catch_all"), config_obj.getClusterForConnection(connection)); + } + + { + // hit route with source port range + NiceMock connection; + EXPECT_CALL(connection, destinationAddress()).WillRepeatedly(Return("1.2.3.4")); + EXPECT_CALL(connection, destinationPort()).WillRepeatedly(Return(23456)); + EXPECT_CALL(connection, remotePort()).WillRepeatedly(Return(23459)); + EXPECT_EQ(std::string("with_source_ports"), config_obj.getClusterForConnection(connection)); + } + + { + // fall through + NiceMock connection; + EXPECT_CALL(connection, destinationAddress()).WillRepeatedly(Return("1.2.3.4")); + EXPECT_CALL(connection, destinationPort()).WillRepeatedly(Return(23456)); + EXPECT_CALL(connection, remotePort()).WillRepeatedly(Return(23458)); + EXPECT_EQ(std::string("catch_all"), config_obj.getClusterForConnection(connection)); + } + + { + // hit the route with all criterias present + NiceMock connection; + EXPECT_CALL(connection, destinationAddress()).WillRepeatedly(Return("10.0.0.0")); + EXPECT_CALL(connection, destinationPort()).WillRepeatedly(Return(10000)); + EXPECT_CALL(connection, remoteAddress()) + .WillRepeatedly(ReturnRefOfCopy(std::string("20.0.0.0"))); + EXPECT_CALL(connection, remotePort()).WillRepeatedly(Return(20000)); + EXPECT_EQ(std::string("with_everything"), config_obj.getClusterForConnection(connection)); + } + + { + // fall through + NiceMock connection; + EXPECT_CALL(connection, destinationAddress()).WillRepeatedly(Return("10.0.0.0")); + EXPECT_CALL(connection, destinationPort()).WillRepeatedly(Return(10000)); + EXPECT_CALL(connection, remoteAddress()) + .WillRepeatedly(ReturnRefOfCopy(std::string("30.0.0.0"))); + EXPECT_CALL(connection, remotePort()).WillRepeatedly(Return(20000)); + EXPECT_EQ(std::string("catch_all"), config_obj.getClusterForConnection(connection)); + } +} + +TEST(TcpProxyConfigTest, EmptyRouteConfig) { + std::string json = R"EOF( + { + "stat_prefix": "name", + "route_config": { + "routes": [ + ] + } + } + )EOF"; + + Json::ObjectPtr json_config = Json::Factory::LoadFromString(json); + NiceMock cm_; + + TcpProxyConfig config_obj(*json_config, cm_, cm_.cluster_.info_->stats_store_); + + NiceMock connection; + EXPECT_EQ(std::string(""), config_obj.getClusterForConnection(connection)); +} + class TcpProxyTest : public testing::Test { public: TcpProxyTest() { std::string json = R"EOF( { - "cluster": "fake_cluster", - "stat_prefix": "name" + "stat_prefix": "name", + "route_config": { + "routes": [ + { + "cluster": "fake_cluster" + } + ] + } } )EOF"; diff --git a/test/common/network/connection_impl_test.cc b/test/common/network/connection_impl_test.cc index 950214050751..0fb75d9fc659 100644 --- a/test/common/network/connection_impl_test.cc +++ b/test/common/network/connection_impl_test.cc @@ -43,7 +43,7 @@ TEST(ConnectionImplUtility, updateBufferStats) { TEST(ConnectionImplDeathTest, BadFd) { Event::DispatcherImpl dispatcher; - EXPECT_DEATH(ConnectionImpl(dispatcher, -1, "127.0.0.1"), ".*assert failure: fd_ != -1.*"); + EXPECT_DEATH(ConnectionImpl(dispatcher, -1, "127.0.0.1", 0), ".*assert failure: fd_ != -1.*"); } struct MockBufferStats { diff --git a/test/common/network/filter_manager_impl_test.cc b/test/common/network/filter_manager_impl_test.cc index cbd2d6460e24..4bb767e687f4 100644 --- a/test/common/network/filter_manager_impl_test.cc +++ b/test/common/network/filter_manager_impl_test.cc @@ -122,8 +122,14 @@ TEST_F(NetworkFilterManagerTest, RateLimitAndTcpProxy) { std::string tcp_proxy_json = R"EOF( { - "cluster": "fake_cluster", - "stat_prefix": "name" + "stat_prefix": "name", + "route_config": { + "routes": [ + { + "cluster": "fake_cluster" + } + ] + } } )EOF"; diff --git a/test/common/network/listener_impl_test.cc b/test/common/network/listener_impl_test.cc index 72bd5020424c..d33770c781d5 100644 --- a/test/common/network/listener_impl_test.cc +++ b/test/common/network/listener_impl_test.cc @@ -52,8 +52,8 @@ class TestListenerImpl : public ListenerImpl { MOCK_METHOD2(newConnection_, void(int, sockaddr*)); void newConnection(int fd, sockaddr* addr) override { newConnection_(fd, addr); } - void newConnection(int fd, const std::string& addr) override { - ListenerImpl::newConnection(fd, addr); + void newConnection(int fd, const std::string& addr, uint32_t port) override { + ListenerImpl::newConnection(fd, addr, port); } protected: diff --git a/test/common/network/utility_test.cc b/test/common/network/utility_test.cc index 9b4226a7d3c1..bfcda90253d2 100644 --- a/test/common/network/utility_test.cc +++ b/test/common/network/utility_test.cc @@ -138,4 +138,42 @@ TEST(NetworkUtility, loopbackAddress) { } } +TEST(NetworkUtility, PortRangeList) { + { + std::string port_range_str = "1"; + std::list port_range_list; + + Utility::parsePortRangeList(port_range_str, port_range_list); + EXPECT_TRUE(Utility::portInRangeList(1, port_range_list)); + EXPECT_FALSE(Utility::portInRangeList(2, port_range_list)); + } + + { + std::string port_range_str = "1024-2048"; + std::list port_range_list; + + Utility::parsePortRangeList(port_range_str, port_range_list); + EXPECT_TRUE(Utility::portInRangeList(1024, port_range_list)); + EXPECT_TRUE(Utility::portInRangeList(2048, port_range_list)); + EXPECT_TRUE(Utility::portInRangeList(1536, port_range_list)); + EXPECT_FALSE(Utility::portInRangeList(1023, port_range_list)); + EXPECT_FALSE(Utility::portInRangeList(2049, port_range_list)); + EXPECT_FALSE(Utility::portInRangeList(0, port_range_list)); + } + + { + std::string port_range_str = "1,10-100,1000-10000,65535"; + std::list port_range_list; + + Utility::parsePortRangeList(port_range_str, port_range_list); + EXPECT_TRUE(Utility::portInRangeList(1, port_range_list)); + EXPECT_TRUE(Utility::portInRangeList(50, port_range_list)); + EXPECT_TRUE(Utility::portInRangeList(5000, port_range_list)); + EXPECT_TRUE(Utility::portInRangeList(65535, port_range_list)); + EXPECT_FALSE(Utility::portInRangeList(2, port_range_list)); + EXPECT_FALSE(Utility::portInRangeList(200, port_range_list)); + EXPECT_FALSE(Utility::portInRangeList(20000, port_range_list)); + } +} + } // Network diff --git a/test/config/integration/server.json b/test/config/integration/server.json index 531684730de1..1e639211ec8a 100644 --- a/test/config/integration/server.json +++ b/test/config/integration/server.json @@ -188,7 +188,16 @@ "filters": [ { "type": "read", "name": "tcp_proxy", - "config": { "cluster": "cluster_1", "stat_prefix": "test_tcp" } + "config": { + "stat_prefix": "test_tcp", + "route_config": { + "routes": [ + { + "cluster": "cluster_1" + } + ] + } + } } ] }], diff --git a/test/config/integration/server_http2.json b/test/config/integration/server_http2.json index 992fd6e456ab..f7e12a9ecc8c 100644 --- a/test/config/integration/server_http2.json +++ b/test/config/integration/server_http2.json @@ -157,7 +157,16 @@ "filters": [ { "type": "read", "name": "tcp_proxy", - "config": { "cluster": "cluster_1", "stat_prefix": "test_tcp" } + "config": { + "stat_prefix": "test_tcp", + "route_config": { + "routes": [ + { + "cluster": "cluster_1" + } + ] + } + } } ] }], diff --git a/test/mocks/network/mocks.h b/test/mocks/network/mocks.h index 60d05ac73ba9..1877c6fffc3d 100644 --- a/test/mocks/network/mocks.h +++ b/test/mocks/network/mocks.h @@ -52,6 +52,9 @@ class MockConnection : public Connection, public MockConnectionBase { MOCK_METHOD1(readDisable, void(bool disable)); MOCK_METHOD0(readEnabled, bool()); MOCK_METHOD0(remoteAddress, const std::string&()); + MOCK_METHOD0(remotePort, uint32_t()); + MOCK_METHOD0(destinationAddress, const std::string()); + MOCK_METHOD0(destinationPort, uint32_t()); MOCK_METHOD1(setBufferStats, void(const BufferStats& stats)); MOCK_METHOD0(ssl, Ssl::Connection*()); MOCK_METHOD0(state, State()); @@ -81,6 +84,9 @@ class MockClientConnection : public ClientConnection, public MockConnectionBase MOCK_METHOD1(readDisable, void(bool disable)); MOCK_METHOD0(readEnabled, bool()); MOCK_METHOD0(remoteAddress, const std::string&()); + MOCK_METHOD0(remotePort, uint32_t()); + MOCK_METHOD0(destinationAddress, const std::string()); + MOCK_METHOD0(destinationPort, uint32_t()); MOCK_METHOD1(setBufferStats, void(const BufferStats& stats)); MOCK_METHOD0(ssl, Ssl::Connection*()); MOCK_METHOD0(state, State()); From d877b8a336fb87d1f6b1866676553e574ef1d206 Mon Sep 17 00:00:00 2001 From: Enrico Schiattarella Date: Tue, 24 Jan 2017 18:25:14 -0800 Subject: [PATCH 15/22] Add some comments and cosmetic fixes. --- source/common/filter/tcp_proxy.cc | 16 ++++++---------- source/common/filter/tcp_proxy.h | 7 +++++++ test/common/filter/tcp_proxy_test.cc | 15 ++++++++++----- 3 files changed, 23 insertions(+), 15 deletions(-) diff --git a/source/common/filter/tcp_proxy.cc b/source/common/filter/tcp_proxy.cc index f8156fecee9f..f7144974f081 100644 --- a/source/common/filter/tcp_proxy.cc +++ b/source/common/filter/tcp_proxy.cc @@ -121,17 +121,14 @@ void TcpProxy::initializeReadFilterCallbacks(Network::ReadFilterCallbacks& callb } Network::FilterStatus TcpProxy::initializeUpstreamConnection() { - const std::string& destination_cluster = - config_->getClusterForConnection(read_callbacks_->connection()); - conn_log_debug("Connection from {}", read_callbacks_->connection(), destination_cluster); + const std::string& cluster_name = config_->getClusterForConnection(read_callbacks_->connection()); + + Upstream::ClusterInfoPtr cluster = cluster_manager_.get(cluster_name); - Upstream::ClusterInfoPtr cluster = cluster_manager_.get(destination_cluster); if (cluster) { - conn_log_debug("Connection cluster with name {} found", read_callbacks_->connection(), - destination_cluster); + conn_log_debug("Creating connection to cluster {}", read_callbacks_->connection(), + cluster_name); } else { - conn_log_debug("Connection cluster with name {} NOT FOUND", read_callbacks_->connection(), - destination_cluster); return Network::FilterStatus::StopIteration; } @@ -140,8 +137,7 @@ Network::FilterStatus TcpProxy::initializeUpstreamConnection() { read_callbacks_->connection().close(Network::ConnectionCloseType::NoFlush); return Network::FilterStatus::StopIteration; } - Upstream::Host::CreateConnectionData conn_info = - cluster_manager_.tcpConnForCluster(destination_cluster); + Upstream::Host::CreateConnectionData conn_info = cluster_manager_.tcpConnForCluster(cluster_name); upstream_connection_ = std::move(conn_info.connection_); read_callbacks_->upstreamHost(conn_info.host_description_); diff --git a/source/common/filter/tcp_proxy.h b/source/common/filter/tcp_proxy.h index 9855c6c35959..7d3683a94400 100644 --- a/source/common/filter/tcp_proxy.h +++ b/source/common/filter/tcp_proxy.h @@ -39,6 +39,13 @@ class TcpProxyConfig { TcpProxyConfig(const Json::Object& config, Upstream::ClusterManager& cluster_manager, Stats::Store& stats_store); + /** + * Find out which cluster an upstream connection should be opened to. + * @param connection supplies the parameters of the downstream connection for + * which the proxy needs to open the corresponding upstream + * @return the cluster name to be used for the upstream connection. + If no route applies, returns the empty string. + */ const std::string& getClusterForConnection(Network::Connection& connection); const TcpProxyStats& stats() { return stats_; } diff --git a/test/common/filter/tcp_proxy_test.cc b/test/common/filter/tcp_proxy_test.cc index 53b4b4b23244..26b612a25b02 100644 --- a/test/common/filter/tcp_proxy_test.cc +++ b/test/common/filter/tcp_proxy_test.cc @@ -114,7 +114,8 @@ TEST(TcpProxyConfigTest, Routes) { // hit route with destination_ip (10.10.10.10/32) NiceMock connection; EXPECT_CALL(connection, destinationAddress()).WillRepeatedly(Return("10.10.10.10")); - EXPECT_EQ(std::string("with_destination_ip_list"), config_obj.getClusterForConnection(connection)); + EXPECT_EQ(std::string("with_destination_ip_list"), + config_obj.getClusterForConnection(connection)); } { @@ -128,7 +129,8 @@ TEST(TcpProxyConfigTest, Routes) { // hit route with destination_ip (10.10.11.0/24) NiceMock connection; EXPECT_CALL(connection, destinationAddress()).WillRepeatedly(Return("10.10.11.11")); - EXPECT_EQ(std::string("with_destination_ip_list"), config_obj.getClusterForConnection(connection)); + EXPECT_EQ(std::string("with_destination_ip_list"), + config_obj.getClusterForConnection(connection)); } { @@ -142,7 +144,8 @@ TEST(TcpProxyConfigTest, Routes) { // hit route with destination_ip (10.11.0.0/16) NiceMock connection; EXPECT_CALL(connection, destinationAddress()).WillRepeatedly(Return("10.11.11.11")); - EXPECT_EQ(std::string("with_destination_ip_list"), config_obj.getClusterForConnection(connection)); + EXPECT_EQ(std::string("with_destination_ip_list"), + config_obj.getClusterForConnection(connection)); } { @@ -156,7 +159,8 @@ TEST(TcpProxyConfigTest, Routes) { // hit route with destination_ip (11.0.0.0/8) NiceMock connection; EXPECT_CALL(connection, destinationAddress()).WillRepeatedly(Return("11.11.11.11")); - EXPECT_EQ(std::string("with_destination_ip_list"), config_obj.getClusterForConnection(connection)); + EXPECT_EQ(std::string("with_destination_ip_list"), + config_obj.getClusterForConnection(connection)); } { @@ -170,7 +174,8 @@ TEST(TcpProxyConfigTest, Routes) { // hit route with destination_ip (128.0.0.0/8) NiceMock connection; EXPECT_CALL(connection, destinationAddress()).WillRepeatedly(Return("128.255.255.255")); - EXPECT_EQ(std::string("with_destination_ip_list"), config_obj.getClusterForConnection(connection)); + EXPECT_EQ(std::string("with_destination_ip_list"), + config_obj.getClusterForConnection(connection)); } { From 91bd2a68f64c94fbae9ca219218d2290fda6a4a2 Mon Sep 17 00:00:00 2001 From: Enrico Schiattarella Date: Tue, 24 Jan 2017 20:01:47 -0800 Subject: [PATCH 16/22] Add missing file --- .../tcp_proxy_filter_route.rst | 74 +++++++++++++++++++ 1 file changed, 74 insertions(+) create mode 100644 docs/configuration/network_filters/tcp_proxy_filter_route.rst diff --git a/docs/configuration/network_filters/tcp_proxy_filter_route.rst b/docs/configuration/network_filters/tcp_proxy_filter_route.rst new file mode 100644 index 000000000000..374f3ad4e2bf --- /dev/null +++ b/docs/configuration/network_filters/tcp_proxy_filter_route.rst @@ -0,0 +1,74 @@ +.. _config_network_filters_tcp_proxy_route: + +Route +===== + +A TCP proxy route consists of a set of optional L4 criteria and the name of a :ref:`cluster manager ` to use for a connection if it mateches all the specified criteria. Routes are tried in the order specified until a match is found. If no match is found, the connection is closed. A route with no criteria is valid and always produces a match. + +.. code-block:: json + + { + "cluster": "...", + "destination_ip_list": [], + "destination_ports": "...", + "source_ip_list": [], + "source_ports": "..." + } + +cluster + *(required, string)* The :ref:`cluster manager ` cluster to connect + to when a the downstream network connection matches the specified criteria. + +destination_ip_list + *(optional, array)* An optional list of IPv4 subnets in the form "a.b.c.d/xx". + The criteria is satisfied if the destination IP address of the downstream connection is contained in at least one of the specified subnets. + If the parameter is not specified or the list is empty, the destination IP address is ignored. + The destination IP address of the downstream connection might be different from the addresses on which the proxy is listening if the connection has been redirected using iptables. + Example: + + .. code-block:: json + + [ + "192.168.3.0/24", + "50.1.2.3/32", + "10.15.0.0/16" + ] + +destination_ports + *(optional, string)* An optional string containing a comma-separated list of port numbers or ranges. + The criteria is satisfied if the destination port of the downstream connection is contained in at least one of the specified ranges. + If the parameter is not specified or the list is empty, the destination port is ignored. + The destination port address of the downstream connection might be different from the port on which the proxy is listening if the connection has been redirected using iptables. + Example: + + .. code-block:: json + + { + "destination_ports": "1-1024,2048-4096,12345" + } + +source_ip_list + *(optional, array)* An optional list of IPv4 subnets in the form "a.b.c.d/xx". + The criteria is satisfied if the source IP address of the downstream connection is contained in at least one of the specified subnets. + If the parameter is not specified or the list is empty, the source IP address is ignored. + Example: + + .. code-block:: json + + [ + "192.168.3.0/24", + "50.1.2.3/32", + "10.15.0.0/16" + ] + +source_ports + *(optional, string)* An optional string containing a comma-separated list of port numbers or ranges. + The criteria is satisfied if the source port of the downstream connection is contained in at least one of the specified ranges. + If the parameter is not specified or the list is empty, the source port is ignored. + Example: + + .. code-block:: json + + { + "source_ports": "1-1024,2048-4096,12345" + } From 8b530168bfd4a272d02f68908cfec41f17fff4a2 Mon Sep 17 00:00:00 2001 From: Enrico Schiattarella Date: Thu, 26 Jan 2017 10:24:01 -0800 Subject: [PATCH 17/22] Incorporate review comments --- .../network_filters/tcp_proxy_filter.rst | 104 +++++++++++++++++- .../tcp_proxy_filter_route.rst | 74 ------------- .../tcp_proxy_filter_route_config.rst | 21 ---- include/envoy/network/connection.h | 12 -- source/common/filter/tcp_proxy.cc | 25 +++-- source/common/filter/tcp_proxy.h | 13 ++- source/common/network/connection_impl.cc | 32 ++---- source/common/network/connection_impl.h | 9 +- source/common/network/listener_impl.cc | 9 +- source/common/network/utility.cc | 5 +- source/common/network/utility.h | 2 + source/common/ssl/connection_impl.cc | 9 +- source/common/ssl/connection_impl.h | 2 +- test/common/filter/tcp_proxy_test.cc | 96 ++++++++-------- test/common/network/connection_impl_test.cc | 3 +- test/common/network/proxy_protocol_test.cc | 7 +- test/mocks/network/mocks.h | 4 - 17 files changed, 202 insertions(+), 225 deletions(-) delete mode 100644 docs/configuration/network_filters/tcp_proxy_filter_route.rst delete mode 100644 docs/configuration/network_filters/tcp_proxy_filter_route_config.rst diff --git a/docs/configuration/network_filters/tcp_proxy_filter.rst b/docs/configuration/network_filters/tcp_proxy_filter.rst index 58d4f146322f..2966ed9a4c2b 100644 --- a/docs/configuration/network_filters/tcp_proxy_filter.rst +++ b/docs/configuration/network_filters/tcp_proxy_filter.rst @@ -17,12 +17,108 @@ TCP proxy :ref:`architecture overview `. } :ref:`route_config ` - *(required, object)* The route table for the filter. All filter instances must have a route table, even if it is empty. + *(required, object)* The route table for the filter. + All filter instances must have a route table, even if it is empty. stat_prefix *(required, string)* The prefix to use when emitting :ref:`statistics `. +.. _config_network_filters_tcp_proxy_route_config: + +Route Configuration +------------------- + +.. code-block:: json + + { + "routes": [] + } + +:ref:`routes ` + *(required, array)* An array of route entries that make up the route table. + +.. _config_network_filters_tcp_proxy_route: + +Route +----- + +A TCP proxy route consists of a set of optional L4 criteria and the name of a +:ref:`cluster `. If a downstream connection matches +all the specified criteria, the cluster in the route is used for the corresponding upstream +connection. Routes are tried in the order specified until a match is found. If no match is +found, the connection is closed. A route with no criteria is valid and always produces a match. + +.. code-block:: json + + { + "cluster": "...", + "destination_ip_list": [], + "destination_ports": "...", + "source_ip_list": [], + "source_ports": "..." + } + +cluster + *(required, string)* The :ref:`cluster ` to connect + to when a the downstream network connection matches the specified criteria. + +destination_ip_list + *(optional, array)* An optional list of IPv4 subnets in the form "a.b.c.d/xx". + The criteria is satisfied if the destination IP address of the downstream connection is + contained in at least one of the specified subnets. + If the parameter is not specified or the list is empty, the destination IP address is ignored. + The destination IP address of the downstream connection might be different from the addresses + on which the proxy is listening if the connection has been redirected. Example: + + .. code-block:: json + + [ + "192.168.3.0/24", + "50.1.2.3/32", + "10.15.0.0/16" + ] + +destination_ports + *(optional, string)* An optional string containing a comma-separated list of port numbers or + ranges. The criteria is satisfied if the destination port of the downstream connection + is contained in at least one of the specified ranges. + If the parameter is not specified or the list is empty, the destination port is ignored. + The destination port address of the downstream connection might be different from the port + on which the proxy is listening if the connection has been redirected. Example: + + .. code-block:: json + + { + "destination_ports": "1-1024,2048-4096,12345" + } + +source_ip_list + *(optional, array)* An optional list of IPv4 subnets in the form "a.b.c.d/xx". + The criteria is satisfied if the source IP address of the downstream connection is contained + in at least one of the specified subnets. If the parameter is not specified or the list is empty, + the source IP address is ignored. Example: + + .. code-block:: json + + [ + "192.168.3.0/24", + "50.1.2.3/32", + "10.15.0.0/16" + ] + +source_ports + *(optional, string)* An optional string containing a comma-separated list of port numbers or + ranges. The criteria is satisfied if the source port of the downstream connection is contained + in at least one of the specified ranges. If the parameter is not specified or the list is empty, + the source port is ignored. Example: + + .. code-block:: json + + { + "source_ports": "1-1024,2048-4096,12345" + } + .. _config_network_filters_tcp_proxy_stats: Statistics @@ -36,10 +132,8 @@ statistics are rooted at *tcp..* with the following statistics: :header: Name, Type, Description :widths: 1, 1, 2 + downstream_cx_total, Counter, Total number of connections handled by the filter. + downstream_cx_no_route, Counter, Number of connections for which no matching route was found. downstream_cx_tx_bytes_total, Counter, Total bytes written to the downstream connection. downstream_cx_tx_bytes_buffered, Gauge, Total bytes currently buffered to the downstream connection. -.. toctree:: - :hidden: - - tcp_proxy_filter_route_config diff --git a/docs/configuration/network_filters/tcp_proxy_filter_route.rst b/docs/configuration/network_filters/tcp_proxy_filter_route.rst deleted file mode 100644 index 374f3ad4e2bf..000000000000 --- a/docs/configuration/network_filters/tcp_proxy_filter_route.rst +++ /dev/null @@ -1,74 +0,0 @@ -.. _config_network_filters_tcp_proxy_route: - -Route -===== - -A TCP proxy route consists of a set of optional L4 criteria and the name of a :ref:`cluster manager ` to use for a connection if it mateches all the specified criteria. Routes are tried in the order specified until a match is found. If no match is found, the connection is closed. A route with no criteria is valid and always produces a match. - -.. code-block:: json - - { - "cluster": "...", - "destination_ip_list": [], - "destination_ports": "...", - "source_ip_list": [], - "source_ports": "..." - } - -cluster - *(required, string)* The :ref:`cluster manager ` cluster to connect - to when a the downstream network connection matches the specified criteria. - -destination_ip_list - *(optional, array)* An optional list of IPv4 subnets in the form "a.b.c.d/xx". - The criteria is satisfied if the destination IP address of the downstream connection is contained in at least one of the specified subnets. - If the parameter is not specified or the list is empty, the destination IP address is ignored. - The destination IP address of the downstream connection might be different from the addresses on which the proxy is listening if the connection has been redirected using iptables. - Example: - - .. code-block:: json - - [ - "192.168.3.0/24", - "50.1.2.3/32", - "10.15.0.0/16" - ] - -destination_ports - *(optional, string)* An optional string containing a comma-separated list of port numbers or ranges. - The criteria is satisfied if the destination port of the downstream connection is contained in at least one of the specified ranges. - If the parameter is not specified or the list is empty, the destination port is ignored. - The destination port address of the downstream connection might be different from the port on which the proxy is listening if the connection has been redirected using iptables. - Example: - - .. code-block:: json - - { - "destination_ports": "1-1024,2048-4096,12345" - } - -source_ip_list - *(optional, array)* An optional list of IPv4 subnets in the form "a.b.c.d/xx". - The criteria is satisfied if the source IP address of the downstream connection is contained in at least one of the specified subnets. - If the parameter is not specified or the list is empty, the source IP address is ignored. - Example: - - .. code-block:: json - - [ - "192.168.3.0/24", - "50.1.2.3/32", - "10.15.0.0/16" - ] - -source_ports - *(optional, string)* An optional string containing a comma-separated list of port numbers or ranges. - The criteria is satisfied if the source port of the downstream connection is contained in at least one of the specified ranges. - If the parameter is not specified or the list is empty, the source port is ignored. - Example: - - .. code-block:: json - - { - "source_ports": "1-1024,2048-4096,12345" - } diff --git a/docs/configuration/network_filters/tcp_proxy_filter_route_config.rst b/docs/configuration/network_filters/tcp_proxy_filter_route_config.rst deleted file mode 100644 index b5d3ce77709b..000000000000 --- a/docs/configuration/network_filters/tcp_proxy_filter_route_config.rst +++ /dev/null @@ -1,21 +0,0 @@ -.. _config_network_filters_tcp_proxy_route_config: - -Route Configuration -=================== - -* TCP proxy :ref:`architecture overview `. -* TCP proxy :ref:`filter `. - -.. code-block:: json - - { - "routes": [] - } - -:ref:`routes ` - *(required, array)* An array of route entries that make up the route table. - -.. toctree:: - :hidden: - - tcp_proxy_filter_route diff --git a/include/envoy/network/connection.h b/include/envoy/network/connection.h index 78a938712417..d16081987ceb 100644 --- a/include/envoy/network/connection.h +++ b/include/envoy/network/connection.h @@ -115,11 +115,6 @@ class Connection : public Event::DeferredDeletable, public FilterManager { */ virtual const std::string& remoteAddress() PURE; - /** - * @return The port number used by the remote client. - */ - virtual uint32_t remotePort() PURE; - /** * @return The address the remote client is trying to connect to. * It can be different from the proxy address if the downstream connection @@ -127,13 +122,6 @@ class Connection : public Event::DeferredDeletable, public FilterManager { */ virtual const std::string destinationAddress() PURE; - /** - * @return The port number the remote client is trying to connect to. - * It can be different from the port the listener is listening on if the connection - * has been redirected or the proxy is operating in transparent mode. - */ - virtual uint32_t destinationPort() PURE; - /** * Set the buffer stats to update when the connection's read/write buffers change. Note that * for performance reasons these stats are eventually consistent and may not always accurately diff --git a/source/common/filter/tcp_proxy.cc b/source/common/filter/tcp_proxy.cc index f7144974f081..2fdb075cc23a 100644 --- a/source/common/filter/tcp_proxy.cc +++ b/source/common/filter/tcp_proxy.cc @@ -17,7 +17,7 @@ TcpProxyConfig::Route::Route(const Json::Object& config) { if (config.hasObject("cluster")) { cluster_name_ = config.getString("cluster"); } else { - throw EnvoyException(fmt::format("tcp proxy: route without cluster")); + throw EnvoyException("tcp proxy: route without cluster"); } if (config.hasObject("source_ip_list")) { @@ -42,7 +42,7 @@ TcpProxyConfig::TcpProxyConfig(const Json::Object& config, Upstream::ClusterManager& cluster_manager, Stats::Store& stats_store) : stats_(generateStats(config.getString("stat_prefix"), stats_store)) { if (!config.hasObject("route_config")) { - throw EnvoyException(fmt::format("tcp proxy: missing route config")); + throw EnvoyException("tcp proxy: missing route config"); } for (const Json::ObjectPtr& route_desc : @@ -56,25 +56,29 @@ TcpProxyConfig::TcpProxyConfig(const Json::Object& config, } } -const std::string& TcpProxyConfig::getClusterForConnection(Network::Connection& connection) { +const std::string& TcpProxyConfig::getRouteFromEntries(Network::Connection& connection) { for (const TcpProxyConfig::Route& route : routes_) { if (!route.source_port_ranges_.empty() && - !Network::Utility::portInRangeList(connection.remotePort(), route.source_port_ranges_)) { + !Network::Utility::portInRangeList( + Network::Utility::portFromUrl(connection.remoteAddress()), route.source_port_ranges_)) { continue; // no match, try next route } - if (!route.source_ips_.empty() && !route.source_ips_.contains(connection.remoteAddress())) { + if (!route.source_ips_.empty() && + !route.source_ips_.contains(Network::Utility::hostFromUrl(connection.remoteAddress()))) { continue; // no match, try next route } if (!route.destination_port_ranges_.empty() && - !Network::Utility::portInRangeList(connection.destinationPort(), - route.destination_port_ranges_)) { + !Network::Utility::portInRangeList( + Network::Utility::portFromUrl(connection.destinationAddress()), + route.destination_port_ranges_)) { continue; // no match, try next route } if (!route.destination_ips_.empty() && - !route.destination_ips_.contains(connection.destinationAddress())) { + !route.destination_ips_.contains( + Network::Utility::hostFromUrl(connection.destinationAddress()))) { continue; // no match, try next route } @@ -113,6 +117,7 @@ TcpProxyStats TcpProxyConfig::generateStats(const std::string& name, Stats::Stor void TcpProxy::initializeReadFilterCallbacks(Network::ReadFilterCallbacks& callbacks) { read_callbacks_ = &callbacks; conn_log_info("new tcp proxy session", read_callbacks_->connection()); + config_->stats().downstream_cx_total_.inc(); read_callbacks_->connection().addConnectionCallbacks(downstream_callbacks_); read_callbacks_->connection().setBufferStats({config_->stats().downstream_cx_rx_bytes_total_, config_->stats().downstream_cx_rx_bytes_buffered_, @@ -121,7 +126,7 @@ void TcpProxy::initializeReadFilterCallbacks(Network::ReadFilterCallbacks& callb } Network::FilterStatus TcpProxy::initializeUpstreamConnection() { - const std::string& cluster_name = config_->getClusterForConnection(read_callbacks_->connection()); + const std::string& cluster_name = config_->getRouteFromEntries(read_callbacks_->connection()); Upstream::ClusterInfoPtr cluster = cluster_manager_.get(cluster_name); @@ -129,6 +134,8 @@ Network::FilterStatus TcpProxy::initializeUpstreamConnection() { conn_log_debug("Creating connection to cluster {}", read_callbacks_->connection(), cluster_name); } else { + config_->stats().downstream_cx_no_route_.inc(); + read_callbacks_->connection().close(Network::ConnectionCloseType::NoFlush); return Network::FilterStatus::StopIteration; } diff --git a/source/common/filter/tcp_proxy.h b/source/common/filter/tcp_proxy.h index 7d3683a94400..38de94f9277f 100644 --- a/source/common/filter/tcp_proxy.h +++ b/source/common/filter/tcp_proxy.h @@ -21,7 +21,9 @@ namespace Filter { COUNTER(downstream_cx_rx_bytes_total) \ GAUGE (downstream_cx_rx_bytes_buffered) \ COUNTER(downstream_cx_tx_bytes_total) \ - GAUGE (downstream_cx_tx_bytes_buffered) + GAUGE (downstream_cx_tx_bytes_buffered) \ + COUNTER(downstream_cx_total) \ + COUNTER(downstream_cx_no_route) // clang-format on /** @@ -40,13 +42,14 @@ class TcpProxyConfig { Stats::Store& stats_store); /** - * Find out which cluster an upstream connection should be opened to. + * Find out which cluster an upstream connection should be opened to based on the + * parameters of a downstream connection. * @param connection supplies the parameters of the downstream connection for - * which the proxy needs to open the corresponding upstream + * which the proxy needs to open the corresponding upstream. * @return the cluster name to be used for the upstream connection. If no route applies, returns the empty string. */ - const std::string& getClusterForConnection(Network::Connection& connection); + const std::string& getRouteFromEntries(Network::Connection& connection); const TcpProxyStats& stats() { return stats_; } @@ -63,7 +66,7 @@ class TcpProxyConfig { static TcpProxyStats generateStats(const std::string& name, Stats::Store& store); - std::list routes_; + std::vector routes_; const TcpProxyStats stats_; }; diff --git a/source/common/network/connection_impl.cc b/source/common/network/connection_impl.cc index f61783f18117..93c4a0e00e5e 100644 --- a/source/common/network/connection_impl.cc +++ b/source/common/network/connection_impl.cc @@ -34,9 +34,9 @@ void ConnectionImplUtility::updateBufferStats(uint64_t delta, uint64_t new_total std::atomic ConnectionImpl::next_global_id_; ConnectionImpl::ConnectionImpl(Event::DispatcherImpl& dispatcher, int fd, - const std::string& remote_address, uint32_t remote_port) - : filter_manager_(*this, *this), remote_address_(remote_address), remote_port_(remote_port), - dispatcher_(dispatcher), fd_(fd), id_(++next_global_id_) { + const std::string& remote_address) + : filter_manager_(*this, *this), remote_address_(remote_address), dispatcher_(dispatcher), + fd_(fd), id_(++next_global_id_) { // Treat the lack of a valid fd (which in practice only happens if we run out of FDs) as an OOM // condition and just crash. @@ -402,29 +402,18 @@ const std::string Network::ConnectionImpl::destinationAddress() { memset(&orig_dst_addr, 0, sizeof(orig_dst_addr)); bool success = Utility::getOriginalDst(fd_, &orig_dst_addr); if (success) { - return Utility::getAddressName(reinterpret_cast(&orig_dst_addr)); + return Utility::urlForTcp( + Utility::getAddressName(reinterpret_cast(&orig_dst_addr)), + Utility::getAddressPort(reinterpret_cast(&orig_dst_addr))); } } return EMPTY_STRING; } -uint32_t Network::ConnectionImpl::destinationPort() { - if (fd_ != -1) { - sockaddr_storage orig_dst_addr; - memset(&orig_dst_addr, 0, sizeof(orig_dst_addr)); - bool success = Utility::getOriginalDst(fd_, &orig_dst_addr); - if (success) { - return Utility::getAddressPort(reinterpret_cast(&orig_dst_addr)); - } - } - - return 0; -} - ClientConnectionImpl::ClientConnectionImpl(Event::DispatcherImpl& dispatcher, int fd, - const std::string& url, uint32_t port) - : ConnectionImpl(dispatcher, fd, url, port) {} + const std::string& url) + : ConnectionImpl(dispatcher, fd, url) {} Network::ClientConnectionPtr ClientConnectionImpl::create(Event::DispatcherImpl& dispatcher, const std::string& url) { @@ -439,8 +428,7 @@ Network::ClientConnectionPtr ClientConnectionImpl::create(Event::DispatcherImpl& TcpClientConnectionImpl::TcpClientConnectionImpl(Event::DispatcherImpl& dispatcher, const std::string& url) - : ClientConnectionImpl(dispatcher, socket(AF_INET, SOCK_STREAM | SOCK_NONBLOCK, 0), url, - Network::Utility::portFromUrl(url)) {} + : ClientConnectionImpl(dispatcher, socket(AF_INET, SOCK_STREAM | SOCK_NONBLOCK, 0), url) {} void TcpClientConnectionImpl::connect() { AddrInfoPtr addr_info = Utility::resolveTCP(Utility::hostFromUrl(remote_address_), @@ -450,7 +438,7 @@ void TcpClientConnectionImpl::connect() { UdsClientConnectionImpl::UdsClientConnectionImpl(Event::DispatcherImpl& dispatcher, const std::string& url) - : ClientConnectionImpl(dispatcher, socket(AF_UNIX, SOCK_STREAM | SOCK_NONBLOCK, 0), url, 0) {} + : ClientConnectionImpl(dispatcher, socket(AF_UNIX, SOCK_STREAM | SOCK_NONBLOCK, 0), url) {} void UdsClientConnectionImpl::connect() { sockaddr_un addr = Utility::resolveUnixDomainSocket(Utility::pathFromUrl(remote_address_)); diff --git a/source/common/network/connection_impl.h b/source/common/network/connection_impl.h index 65a79824583c..5d2aa0e04dfd 100644 --- a/source/common/network/connection_impl.h +++ b/source/common/network/connection_impl.h @@ -37,8 +37,7 @@ class ConnectionImpl : public virtual Connection, public BufferSource, protected Logger::Loggable { public: - ConnectionImpl(Event::DispatcherImpl& dispatcher, int fd, const std::string& remote_address, - uint32_t remote_port); + ConnectionImpl(Event::DispatcherImpl& dispatcher, int fd, const std::string& remote_address); ~ConnectionImpl(); // Network::FilterManager @@ -57,9 +56,7 @@ class ConnectionImpl : public virtual Connection, void readDisable(bool disable) override; bool readEnabled() override; const std::string& remoteAddress() override { return remote_address_; } - uint32_t remotePort() override { return remote_port_; }; const std::string destinationAddress() override; - uint32_t destinationPort() override; void setBufferStats(const BufferStats& stats) override; Ssl::Connection* ssl() override { return nullptr; } State state() override; @@ -83,7 +80,6 @@ class ConnectionImpl : public virtual Connection, FilterManagerImpl filter_manager_; const std::string remote_address_; - uint32_t remote_port_; Buffer::OwnedImpl read_buffer_; Buffer::OwnedImpl write_buffer_; @@ -127,8 +123,7 @@ class ConnectionImpl : public virtual Connection, */ class ClientConnectionImpl : public ConnectionImpl, virtual public ClientConnection { public: - ClientConnectionImpl(Event::DispatcherImpl& dispatcher, int fd, const std::string& url, - uint32_t port); + ClientConnectionImpl(Event::DispatcherImpl& dispatcher, int fd, const std::string& url); static Network::ClientConnectionPtr create(Event::DispatcherImpl& dispatcher, const std::string& url); diff --git a/source/common/network/listener_impl.cc b/source/common/network/listener_impl.cc index 1535b97f7fd5..ae83f41b95f2 100644 --- a/source/common/network/listener_impl.cc +++ b/source/common/network/listener_impl.cc @@ -82,7 +82,8 @@ void ListenerImpl::newConnection(int fd, sockaddr* addr) { } void ListenerImpl::newConnection(int fd, const std::string& remote_address, uint32_t remote_port) { - ConnectionPtr new_connection(new ConnectionImpl(dispatcher_, fd, remote_address, remote_port)); + ConnectionPtr new_connection(new ConnectionImpl( + dispatcher_, fd, Network::Utility::urlForTcp(remote_address, remote_port))); cb_.onNewConnection(std::move(new_connection)); } @@ -96,9 +97,9 @@ void SslListenerImpl::newConnection(int fd, sockaddr* addr) { void SslListenerImpl::newConnection(int fd, const std::string& remote_address, uint32_t remote_port) { - ConnectionPtr new_connection(new Ssl::ConnectionImpl(dispatcher_, fd, remote_address, remote_port, - ssl_ctx_, - Ssl::ConnectionImpl::InitialState::Server)); + ConnectionPtr new_connection(new Ssl::ConnectionImpl( + dispatcher_, fd, Network::Utility::urlForTcp(remote_address, remote_port), ssl_ctx_, + Ssl::ConnectionImpl::InitialState::Server)); cb_.onNewConnection(std::move(new_connection)); } diff --git a/source/common/network/utility.cc b/source/common/network/utility.cc index 797c62a7e914..161f4b0c4aae 100644 --- a/source/common/network/utility.cc +++ b/source/common/network/utility.cc @@ -237,13 +237,12 @@ bool Utility::getOriginalDst(int fd, sockaddr_storage* orig_addr) { void Utility::parsePortRangeList(const std::string& string, std::list& list) { std::vector ranges = StringUtil::split(string.c_str(), ','); for (const std::string& s : ranges) { + std::stringstream ss(s); uint32_t min = 0; uint32_t max = 0; - char dash = 0; - - std::stringstream ss(s); if (s.find('-') != std::string::npos) { + char dash = 0; ss >> min; ss >> dash; ss >> max; diff --git a/source/common/network/utility.h b/source/common/network/utility.h index 2c1b3620fd4a..9dc2ce3e008d 100644 --- a/source/common/network/utility.h +++ b/source/common/network/utility.h @@ -161,6 +161,8 @@ class Utility { /** * Parses a string containing a comma-separated list of port numbers and/or * port ranges and appends the values to a caller-provided list of PortRange structures. + * For example, the string "1-1024,2048-4096,12345" causes 3 PortRange structures + * to be appended to the supplied list. * @param str is the string containing the port numbers and ranges * @param list is the list to append the new data structures to */ diff --git a/source/common/ssl/connection_impl.cc b/source/common/ssl/connection_impl.cc index 77f402a3068d..901e49e4268a 100644 --- a/source/common/ssl/connection_impl.cc +++ b/source/common/ssl/connection_impl.cc @@ -10,9 +10,8 @@ namespace Ssl { ConnectionImpl::ConnectionImpl(Event::DispatcherImpl& dispatcher, int fd, - const std::string& remote_address, uint32_t remote_port, - Context& ctx, InitialState state) - : Network::ConnectionImpl(dispatcher, fd, remote_address, remote_port), + const std::string& remote_address, Context& ctx, InitialState state) + : Network::ConnectionImpl(dispatcher, fd, remote_address), ctx_(dynamic_cast(ctx)), ssl_(ctx_.newSsl()) { BIO* bio = BIO_new_socket(fd, 0); SSL_set_bio(ssl_.get(), bio, bio); @@ -188,8 +187,8 @@ std::string ConnectionImpl::sha256PeerCertificateDigest() { ClientConnectionImpl::ClientConnectionImpl(Event::DispatcherImpl& dispatcher, Context& ctx, const std::string& url) - : ConnectionImpl(dispatcher, socket(AF_INET, SOCK_STREAM | SOCK_NONBLOCK, 0), url, - Network::Utility::portFromUrl(url), ctx, InitialState::Client) {} + : ConnectionImpl(dispatcher, socket(AF_INET, SOCK_STREAM | SOCK_NONBLOCK, 0), url, ctx, + InitialState::Client) {} void ClientConnectionImpl::connect() { Network::AddrInfoPtr addr_info = diff --git a/source/common/ssl/connection_impl.h b/source/common/ssl/connection_impl.h index ea0118818786..0f38d29275b9 100644 --- a/source/common/ssl/connection_impl.h +++ b/source/common/ssl/connection_impl.h @@ -11,7 +11,7 @@ class ConnectionImpl : public Network::ConnectionImpl, public Connection { enum class InitialState { Client, Server }; ConnectionImpl(Event::DispatcherImpl& dispatcher, int fd, const std::string& remote_address, - uint32_t remote_port, Context& ctx, InitialState state); + Context& ctx, InitialState state); ~ConnectionImpl(); // Network::Connection diff --git a/test/common/filter/tcp_proxy_test.cc b/test/common/filter/tcp_proxy_test.cc index 26b612a25b02..1a191b01fea9 100644 --- a/test/common/filter/tcp_proxy_test.cc +++ b/test/common/filter/tcp_proxy_test.cc @@ -113,126 +113,124 @@ TEST(TcpProxyConfigTest, Routes) { { // hit route with destination_ip (10.10.10.10/32) NiceMock connection; - EXPECT_CALL(connection, destinationAddress()).WillRepeatedly(Return("10.10.10.10")); - EXPECT_EQ(std::string("with_destination_ip_list"), - config_obj.getClusterForConnection(connection)); + EXPECT_CALL(connection, destinationAddress()).WillRepeatedly(Return("tcp://10.10.10.10:0")); + EXPECT_EQ(std::string("with_destination_ip_list"), config_obj.getRouteFromEntries(connection)); } { // fall-through NiceMock connection; - EXPECT_CALL(connection, destinationAddress()).WillRepeatedly(Return("10.10.10.11")); - EXPECT_EQ(std::string("catch_all"), config_obj.getClusterForConnection(connection)); + EXPECT_CALL(connection, destinationAddress()).WillRepeatedly(Return("tcp://10.10.10.11:0")); + EXPECT_CALL(connection, remoteAddress()) + .WillRepeatedly(ReturnRefOfCopy(std::string("tcp://0.0.0.0:0"))); + EXPECT_EQ(std::string("catch_all"), config_obj.getRouteFromEntries(connection)); } { // hit route with destination_ip (10.10.11.0/24) NiceMock connection; - EXPECT_CALL(connection, destinationAddress()).WillRepeatedly(Return("10.10.11.11")); - EXPECT_EQ(std::string("with_destination_ip_list"), - config_obj.getClusterForConnection(connection)); + EXPECT_CALL(connection, destinationAddress()).WillRepeatedly(Return("tcp://10.10.11.11:0")); + EXPECT_EQ(std::string("with_destination_ip_list"), config_obj.getRouteFromEntries(connection)); } { // fall-through NiceMock connection; - EXPECT_CALL(connection, destinationAddress()).WillRepeatedly(Return("10.10.12.12")); - EXPECT_EQ(std::string("catch_all"), config_obj.getClusterForConnection(connection)); + EXPECT_CALL(connection, destinationAddress()).WillRepeatedly(Return("tcp://10.10.12.12:0")); + EXPECT_CALL(connection, remoteAddress()) + .WillRepeatedly(ReturnRefOfCopy(std::string("tcp://0.0.0.0:0"))); + EXPECT_EQ(std::string("catch_all"), config_obj.getRouteFromEntries(connection)); } { // hit route with destination_ip (10.11.0.0/16) NiceMock connection; - EXPECT_CALL(connection, destinationAddress()).WillRepeatedly(Return("10.11.11.11")); - EXPECT_EQ(std::string("with_destination_ip_list"), - config_obj.getClusterForConnection(connection)); + EXPECT_CALL(connection, destinationAddress()).WillRepeatedly(Return("tcp://10.11.11.11:0")); + EXPECT_EQ(std::string("with_destination_ip_list"), config_obj.getRouteFromEntries(connection)); } { // fall-through NiceMock connection; - EXPECT_CALL(connection, destinationAddress()).WillRepeatedly(Return("10.12.12.12")); - EXPECT_EQ(std::string("catch_all"), config_obj.getClusterForConnection(connection)); + EXPECT_CALL(connection, destinationAddress()).WillRepeatedly(Return("tcp://10.12.12.12:0")); + EXPECT_CALL(connection, remoteAddress()) + .WillRepeatedly(ReturnRefOfCopy(std::string("tcp://0.0.0.0:0"))); + EXPECT_EQ(std::string("catch_all"), config_obj.getRouteFromEntries(connection)); } { // hit route with destination_ip (11.0.0.0/8) NiceMock connection; - EXPECT_CALL(connection, destinationAddress()).WillRepeatedly(Return("11.11.11.11")); - EXPECT_EQ(std::string("with_destination_ip_list"), - config_obj.getClusterForConnection(connection)); + EXPECT_CALL(connection, destinationAddress()).WillRepeatedly(Return("tcp://11.11.11.11:0")); + EXPECT_EQ(std::string("with_destination_ip_list"), config_obj.getRouteFromEntries(connection)); } { // fall-through NiceMock connection; - EXPECT_CALL(connection, destinationAddress()).WillRepeatedly(Return("12.12.12.12")); - EXPECT_EQ(std::string("catch_all"), config_obj.getClusterForConnection(connection)); + EXPECT_CALL(connection, destinationAddress()).WillRepeatedly(Return("tcp://12.12.12.12:0")); + EXPECT_CALL(connection, remoteAddress()) + .WillRepeatedly(ReturnRefOfCopy(std::string("tcp://0.0.0.0:0"))); + EXPECT_EQ(std::string("catch_all"), config_obj.getRouteFromEntries(connection)); } { // hit route with destination_ip (128.0.0.0/8) NiceMock connection; - EXPECT_CALL(connection, destinationAddress()).WillRepeatedly(Return("128.255.255.255")); - EXPECT_EQ(std::string("with_destination_ip_list"), - config_obj.getClusterForConnection(connection)); + EXPECT_CALL(connection, destinationAddress()).WillRepeatedly(Return("tcp://128.255.255.255:0")); + EXPECT_EQ(std::string("with_destination_ip_list"), config_obj.getRouteFromEntries(connection)); } { // hit route with destination port range NiceMock connection; - EXPECT_CALL(connection, destinationAddress()).WillRepeatedly(Return("1.2.3.4")); - EXPECT_CALL(connection, destinationPort()).WillRepeatedly(Return(12345)); - EXPECT_EQ(std::string("with_destination_ports"), - config_obj.getClusterForConnection(connection)); + EXPECT_CALL(connection, destinationAddress()).WillRepeatedly(Return("tcp://1.2.3.4:12345")); + EXPECT_EQ(std::string("with_destination_ports"), config_obj.getRouteFromEntries(connection)); } { // fall through NiceMock connection; - EXPECT_CALL(connection, destinationAddress()).WillRepeatedly(Return("1.2.3.4")); - EXPECT_CALL(connection, destinationPort()).WillRepeatedly(Return(23456)); - EXPECT_EQ(std::string("catch_all"), config_obj.getClusterForConnection(connection)); + EXPECT_CALL(connection, destinationAddress()).WillRepeatedly(Return("tcp://1.2.3.4:23456")); + EXPECT_CALL(connection, remoteAddress()) + .WillRepeatedly(ReturnRefOfCopy(std::string("tcp://0.0.0.0:0"))); + EXPECT_EQ(std::string("catch_all"), config_obj.getRouteFromEntries(connection)); } { // hit route with source port range NiceMock connection; - EXPECT_CALL(connection, destinationAddress()).WillRepeatedly(Return("1.2.3.4")); - EXPECT_CALL(connection, destinationPort()).WillRepeatedly(Return(23456)); - EXPECT_CALL(connection, remotePort()).WillRepeatedly(Return(23459)); - EXPECT_EQ(std::string("with_source_ports"), config_obj.getClusterForConnection(connection)); + EXPECT_CALL(connection, destinationAddress()).WillRepeatedly(Return("tcp://1.2.3.4:23456")); + EXPECT_CALL(connection, remoteAddress()) + .WillRepeatedly(ReturnRefOfCopy(std::string("tcp://0.0.0.0:23459"))); + EXPECT_EQ(std::string("with_source_ports"), config_obj.getRouteFromEntries(connection)); } { // fall through NiceMock connection; - EXPECT_CALL(connection, destinationAddress()).WillRepeatedly(Return("1.2.3.4")); - EXPECT_CALL(connection, destinationPort()).WillRepeatedly(Return(23456)); - EXPECT_CALL(connection, remotePort()).WillRepeatedly(Return(23458)); - EXPECT_EQ(std::string("catch_all"), config_obj.getClusterForConnection(connection)); + EXPECT_CALL(connection, destinationAddress()).WillRepeatedly(Return("tcp://1.2.3.4:23456")); + EXPECT_CALL(connection, remoteAddress()) + .WillRepeatedly(ReturnRefOfCopy(std::string("tcp://0.0.0.0:23458"))); + EXPECT_EQ(std::string("catch_all"), config_obj.getRouteFromEntries(connection)); } { // hit the route with all criterias present NiceMock connection; - EXPECT_CALL(connection, destinationAddress()).WillRepeatedly(Return("10.0.0.0")); - EXPECT_CALL(connection, destinationPort()).WillRepeatedly(Return(10000)); + EXPECT_CALL(connection, destinationAddress()).WillRepeatedly(Return("tcp://10.0.0.0:10000")); EXPECT_CALL(connection, remoteAddress()) - .WillRepeatedly(ReturnRefOfCopy(std::string("20.0.0.0"))); - EXPECT_CALL(connection, remotePort()).WillRepeatedly(Return(20000)); - EXPECT_EQ(std::string("with_everything"), config_obj.getClusterForConnection(connection)); + .WillRepeatedly(ReturnRefOfCopy(std::string("tcp://20.0.0.0:20000"))); + EXPECT_EQ(std::string("with_everything"), config_obj.getRouteFromEntries(connection)); } { // fall through NiceMock connection; - EXPECT_CALL(connection, destinationAddress()).WillRepeatedly(Return("10.0.0.0")); - EXPECT_CALL(connection, destinationPort()).WillRepeatedly(Return(10000)); + EXPECT_CALL(connection, destinationAddress()).WillRepeatedly(Return("tcp://10.0.0.0:10000")); EXPECT_CALL(connection, remoteAddress()) - .WillRepeatedly(ReturnRefOfCopy(std::string("30.0.0.0"))); - EXPECT_CALL(connection, remotePort()).WillRepeatedly(Return(20000)); - EXPECT_EQ(std::string("catch_all"), config_obj.getClusterForConnection(connection)); + .WillRepeatedly(ReturnRefOfCopy(std::string("tcp://30.0.0.0:20000"))); + EXPECT_EQ(std::string("catch_all"), config_obj.getRouteFromEntries(connection)); } } @@ -253,7 +251,7 @@ TEST(TcpProxyConfigTest, EmptyRouteConfig) { TcpProxyConfig config_obj(*json_config, cm_, cm_.cluster_.info_->stats_store_); NiceMock connection; - EXPECT_EQ(std::string(""), config_obj.getClusterForConnection(connection)); + EXPECT_EQ(std::string(""), config_obj.getRouteFromEntries(connection)); } class TcpProxyTest : public testing::Test { diff --git a/test/common/network/connection_impl_test.cc b/test/common/network/connection_impl_test.cc index 0fb75d9fc659..3c1feb6b10ef 100644 --- a/test/common/network/connection_impl_test.cc +++ b/test/common/network/connection_impl_test.cc @@ -43,7 +43,8 @@ TEST(ConnectionImplUtility, updateBufferStats) { TEST(ConnectionImplDeathTest, BadFd) { Event::DispatcherImpl dispatcher; - EXPECT_DEATH(ConnectionImpl(dispatcher, -1, "127.0.0.1", 0), ".*assert failure: fd_ != -1.*"); + EXPECT_DEATH(ConnectionImpl(dispatcher, -1, "tcp://127.0.0.1:0"), + ".*assert failure: fd_ != -1.*"); } struct MockBufferStats { diff --git a/test/common/network/proxy_protocol_test.cc b/test/common/network/proxy_protocol_test.cc index 72d56b523b20..eda21fe0e35a 100644 --- a/test/common/network/proxy_protocol_test.cc +++ b/test/common/network/proxy_protocol_test.cc @@ -1,6 +1,7 @@ #include "common/buffer/buffer_impl.h" #include "common/event/dispatcher_impl.h" #include "common/network/listener_impl.h" +#include "common/network/utility.h" #include "common/stats/stats_impl.h" #include "test/mocks/buffer/mocks.h" @@ -47,7 +48,7 @@ TEST_F(ProxyProtocolTest, Basic) { EXPECT_CALL(callbacks_, onNewConnection_(_)) .WillOnce(Invoke([&](ConnectionPtr& conn) -> void { - ASSERT_EQ("1.2.3.4", conn->remoteAddress()); + ASSERT_EQ("1.2.3.4", Network::Utility::hostFromUrl(conn->remoteAddress())); conn->addReadFilter(read_filter_); accepted_connection = std::move(conn); })); @@ -71,7 +72,7 @@ TEST_F(ProxyProtocolTest, Fragmented) { EXPECT_CALL(callbacks_, onNewConnection_(_)) .WillOnce(Invoke([&](ConnectionPtr& conn) -> void { - ASSERT_EQ("255.255.255.255", conn->remoteAddress()); + ASSERT_EQ("255.255.255.255", Network::Utility::hostFromUrl(conn->remoteAddress())); read_filter_.reset(new MockReadFilter()); conn->addReadFilter(read_filter_); conn->close(ConnectionCloseType::NoFlush); @@ -87,7 +88,7 @@ TEST_F(ProxyProtocolTest, PartialRead) { EXPECT_CALL(callbacks_, onNewConnection_(_)) .WillOnce(Invoke([&](ConnectionPtr& conn) -> void { - ASSERT_EQ("255.255.255.255", conn->remoteAddress()); + ASSERT_EQ("255.255.255.255", Network::Utility::hostFromUrl(conn->remoteAddress())); read_filter_.reset(new MockReadFilter()); conn->addReadFilter(read_filter_); conn->close(ConnectionCloseType::NoFlush); diff --git a/test/mocks/network/mocks.h b/test/mocks/network/mocks.h index c539e1ac594a..4bff1841eb82 100644 --- a/test/mocks/network/mocks.h +++ b/test/mocks/network/mocks.h @@ -52,9 +52,7 @@ class MockConnection : public Connection, public MockConnectionBase { MOCK_METHOD1(readDisable, void(bool disable)); MOCK_METHOD0(readEnabled, bool()); MOCK_METHOD0(remoteAddress, const std::string&()); - MOCK_METHOD0(remotePort, uint32_t()); MOCK_METHOD0(destinationAddress, const std::string()); - MOCK_METHOD0(destinationPort, uint32_t()); MOCK_METHOD1(setBufferStats, void(const BufferStats& stats)); MOCK_METHOD0(ssl, Ssl::Connection*()); MOCK_METHOD0(state, State()); @@ -84,9 +82,7 @@ class MockClientConnection : public ClientConnection, public MockConnectionBase MOCK_METHOD1(readDisable, void(bool disable)); MOCK_METHOD0(readEnabled, bool()); MOCK_METHOD0(remoteAddress, const std::string&()); - MOCK_METHOD0(remotePort, uint32_t()); MOCK_METHOD0(destinationAddress, const std::string()); - MOCK_METHOD0(destinationPort, uint32_t()); MOCK_METHOD1(setBufferStats, void(const BufferStats& stats)); MOCK_METHOD0(ssl, Ssl::Connection*()); MOCK_METHOD0(state, State()); From 8bc1d82ba2ed27ac62b2e41a0e83ae8197ff709f Mon Sep 17 00:00:00 2001 From: Enrico Schiattarella Date: Thu, 26 Jan 2017 19:38:21 -0800 Subject: [PATCH 18/22] Fix TCP_PROXY_NETWORK_FILTER_SCHEMA error. --- source/common/json/config_schemas.cc | 56 +++++++++++++++------------- 1 file changed, 30 insertions(+), 26 deletions(-) diff --git a/source/common/json/config_schemas.cc b/source/common/json/config_schemas.cc index aa7befef04d9..9d7984c33fb6 100644 --- a/source/common/json/config_schemas.cc +++ b/source/common/json/config_schemas.cc @@ -129,36 +129,40 @@ const std::string Json::Schema::TCP_PROXY_NETWORK_FILTER_SCHEMA(R"EOF( { "$schema": "http://json-schema.org/schema#", "properties": { - "stat_prefix" : {"type" : "string"}, - "route_config" : { - "type" : "object", + "stat_prefix": {"type" : "string"}, + "route_config": { + "type": "object", "properties": { - "routes" : { - "type" : "array", - "items" : { - "cluster": { - "type" : "string" - }, - "source_ip_list" : { - "type" : "array", - "items" : { - "type" : "string" - } - }, - "source__ports": { - "type" : "string" - }, - "destination_ip_list" : { - "type" : "array", - "items" : { - "type" : "string" + "routes": { + "type": "array", + "items": { + "type": "object", + "properties": { + "cluster": { + "type": "string" + }, + "source_ip_list" : { + "type": "array", + "items": { + "type": "string" + } + }, + "source_ports": { + "type": "string" + }, + "destination_ip_list" : { + "type": "array", + "items": { + "type": "string" + } + }, + "destination_ports": { + "type": "string" } }, - "destination_ports": { - "type" : "string" - } + "required": ["cluster"], + "additionalProperties": false }, - "required": ["cluster"], "additionalProperties": false } }, From 2dffce87179f03285c0edafedbb41f719824440f Mon Sep 17 00:00:00 2001 From: Enrico Schiattarella Date: Fri, 27 Jan 2017 09:19:57 -0800 Subject: [PATCH 19/22] Chande destinationAddress() to localAddress() and populate at construction time --- include/envoy/network/connection.h | 2 +- source/common/filter/tcp_proxy.cc | 7 ++-- source/common/network/connection_impl.cc | 11 +++--- source/common/network/connection_impl.h | 4 ++- test/common/filter/tcp_proxy_test.cc | 45 ++++++++++++++++-------- test/mocks/network/mocks.h | 4 +-- 6 files changed, 45 insertions(+), 28 deletions(-) diff --git a/include/envoy/network/connection.h b/include/envoy/network/connection.h index d16081987ceb..e48958aeff76 100644 --- a/include/envoy/network/connection.h +++ b/include/envoy/network/connection.h @@ -120,7 +120,7 @@ class Connection : public Event::DeferredDeletable, public FilterManager { * It can be different from the proxy address if the downstream connection * has been redirected or the proxy is operating in transparent mode. */ - virtual const std::string destinationAddress() PURE; + virtual const std::string& localAddress() PURE; /** * Set the buffer stats to update when the connection's read/write buffers change. Note that diff --git a/source/common/filter/tcp_proxy.cc b/source/common/filter/tcp_proxy.cc index 46851a2a232a..36114fd68e48 100644 --- a/source/common/filter/tcp_proxy.cc +++ b/source/common/filter/tcp_proxy.cc @@ -69,15 +69,14 @@ const std::string& TcpProxyConfig::getRouteFromEntries(Network::Connection& conn } if (!route.destination_port_ranges_.empty() && - !Network::Utility::portInRangeList( - Network::Utility::portFromUrl(connection.destinationAddress()), - route.destination_port_ranges_)) { + !Network::Utility::portInRangeList(Network::Utility::portFromUrl(connection.localAddress()), + route.destination_port_ranges_)) { continue; // no match, try next route } if (!route.destination_ips_.empty() && !route.destination_ips_.contains( - Network::Utility::hostFromUrl(connection.destinationAddress()))) { + Network::Utility::hostFromUrl(connection.localAddress()))) { continue; // no match, try next route } diff --git a/source/common/network/connection_impl.cc b/source/common/network/connection_impl.cc index 93c4a0e00e5e..05e95b17e7f0 100644 --- a/source/common/network/connection_impl.cc +++ b/source/common/network/connection_impl.cc @@ -35,8 +35,9 @@ std::atomic ConnectionImpl::next_global_id_; ConnectionImpl::ConnectionImpl(Event::DispatcherImpl& dispatcher, int fd, const std::string& remote_address) - : filter_manager_(*this, *this), remote_address_(remote_address), dispatcher_(dispatcher), - fd_(fd), id_(++next_global_id_) { + : filter_manager_(*this, *this), remote_address_(remote_address), + local_address_(getLocalAddress(fd)), dispatcher_(dispatcher), fd_(fd), + id_(++next_global_id_) { // Treat the lack of a valid fd (which in practice only happens if we run out of FDs) as an OOM // condition and just crash. @@ -396,11 +397,11 @@ void ConnectionImpl::updateWriteBufferStats(uint64_t num_written, uint64_t new_s buffer_stats_->write_current_); } -const std::string Network::ConnectionImpl::destinationAddress() { - if (fd_ != -1) { +const std::string Network::ConnectionImpl::getLocalAddress(int fd) { + if (fd != -1) { sockaddr_storage orig_dst_addr; memset(&orig_dst_addr, 0, sizeof(orig_dst_addr)); - bool success = Utility::getOriginalDst(fd_, &orig_dst_addr); + bool success = Utility::getOriginalDst(fd, &orig_dst_addr); if (success) { return Utility::urlForTcp( Utility::getAddressName(reinterpret_cast(&orig_dst_addr)), diff --git a/source/common/network/connection_impl.h b/source/common/network/connection_impl.h index 5d2aa0e04dfd..3544063e2b6f 100644 --- a/source/common/network/connection_impl.h +++ b/source/common/network/connection_impl.h @@ -56,7 +56,7 @@ class ConnectionImpl : public virtual Connection, void readDisable(bool disable) override; bool readEnabled() override; const std::string& remoteAddress() override { return remote_address_; } - const std::string destinationAddress() override; + const std::string& localAddress() override { return local_address_; } void setBufferStats(const BufferStats& stats) override; Ssl::Connection* ssl() override { return nullptr; } State state() override; @@ -80,6 +80,7 @@ class ConnectionImpl : public virtual Connection, FilterManagerImpl filter_manager_; const std::string remote_address_; + const std::string local_address_; Buffer::OwnedImpl read_buffer_; Buffer::OwnedImpl write_buffer_; @@ -103,6 +104,7 @@ class ConnectionImpl : public virtual Connection, void onWriteReady(); void updateReadBufferStats(uint64_t num_read, uint64_t new_size); void updateWriteBufferStats(uint64_t num_written, uint64_t new_size); + static const std::string getLocalAddress(int fd); static std::atomic next_global_id_; diff --git a/test/common/filter/tcp_proxy_test.cc b/test/common/filter/tcp_proxy_test.cc index c2fd4b21ad71..cd1fd5a8b76b 100644 --- a/test/common/filter/tcp_proxy_test.cc +++ b/test/common/filter/tcp_proxy_test.cc @@ -134,14 +134,16 @@ TEST(TcpProxyConfigTest, Routes) { { // hit route with destination_ip (10.10.10.10/32) NiceMock connection; - EXPECT_CALL(connection, destinationAddress()).WillRepeatedly(Return("tcp://10.10.10.10:0")); + EXPECT_CALL(connection, localAddress()) + .WillRepeatedly(ReturnRefOfCopy(std::string("tcp://10.10.10.10:0"))); EXPECT_EQ(std::string("with_destination_ip_list"), config_obj.getRouteFromEntries(connection)); } { // fall-through NiceMock connection; - EXPECT_CALL(connection, destinationAddress()).WillRepeatedly(Return("tcp://10.10.10.11:0")); + EXPECT_CALL(connection, localAddress()) + .WillRepeatedly(ReturnRefOfCopy(std::string("tcp://10.10.10.11:0"))); EXPECT_CALL(connection, remoteAddress()) .WillRepeatedly(ReturnRefOfCopy(std::string("tcp://0.0.0.0:0"))); EXPECT_EQ(std::string("catch_all"), config_obj.getRouteFromEntries(connection)); @@ -150,14 +152,16 @@ TEST(TcpProxyConfigTest, Routes) { { // hit route with destination_ip (10.10.11.0/24) NiceMock connection; - EXPECT_CALL(connection, destinationAddress()).WillRepeatedly(Return("tcp://10.10.11.11:0")); + EXPECT_CALL(connection, localAddress()) + .WillRepeatedly(ReturnRefOfCopy(std::string("tcp://10.10.11.11:0"))); EXPECT_EQ(std::string("with_destination_ip_list"), config_obj.getRouteFromEntries(connection)); } { // fall-through NiceMock connection; - EXPECT_CALL(connection, destinationAddress()).WillRepeatedly(Return("tcp://10.10.12.12:0")); + EXPECT_CALL(connection, localAddress()) + .WillRepeatedly(ReturnRefOfCopy(std::string("tcp://10.10.12.12:0"))); EXPECT_CALL(connection, remoteAddress()) .WillRepeatedly(ReturnRefOfCopy(std::string("tcp://0.0.0.0:0"))); EXPECT_EQ(std::string("catch_all"), config_obj.getRouteFromEntries(connection)); @@ -166,14 +170,16 @@ TEST(TcpProxyConfigTest, Routes) { { // hit route with destination_ip (10.11.0.0/16) NiceMock connection; - EXPECT_CALL(connection, destinationAddress()).WillRepeatedly(Return("tcp://10.11.11.11:0")); + EXPECT_CALL(connection, localAddress()) + .WillRepeatedly(ReturnRefOfCopy(std::string("tcp://10.11.11.11:0"))); EXPECT_EQ(std::string("with_destination_ip_list"), config_obj.getRouteFromEntries(connection)); } { // fall-through NiceMock connection; - EXPECT_CALL(connection, destinationAddress()).WillRepeatedly(Return("tcp://10.12.12.12:0")); + EXPECT_CALL(connection, localAddress()) + .WillRepeatedly(ReturnRefOfCopy(std::string("tcp://10.12.12.12:0"))); EXPECT_CALL(connection, remoteAddress()) .WillRepeatedly(ReturnRefOfCopy(std::string("tcp://0.0.0.0:0"))); EXPECT_EQ(std::string("catch_all"), config_obj.getRouteFromEntries(connection)); @@ -182,14 +188,16 @@ TEST(TcpProxyConfigTest, Routes) { { // hit route with destination_ip (11.0.0.0/8) NiceMock connection; - EXPECT_CALL(connection, destinationAddress()).WillRepeatedly(Return("tcp://11.11.11.11:0")); + EXPECT_CALL(connection, localAddress()) + .WillRepeatedly(ReturnRefOfCopy(std::string("tcp://11.11.11.11:0"))); EXPECT_EQ(std::string("with_destination_ip_list"), config_obj.getRouteFromEntries(connection)); } { // fall-through NiceMock connection; - EXPECT_CALL(connection, destinationAddress()).WillRepeatedly(Return("tcp://12.12.12.12:0")); + EXPECT_CALL(connection, localAddress()) + .WillRepeatedly(ReturnRefOfCopy(std::string("tcp://12.12.12.12:0"))); EXPECT_CALL(connection, remoteAddress()) .WillRepeatedly(ReturnRefOfCopy(std::string("tcp://0.0.0.0:0"))); EXPECT_EQ(std::string("catch_all"), config_obj.getRouteFromEntries(connection)); @@ -198,21 +206,24 @@ TEST(TcpProxyConfigTest, Routes) { { // hit route with destination_ip (128.0.0.0/8) NiceMock connection; - EXPECT_CALL(connection, destinationAddress()).WillRepeatedly(Return("tcp://128.255.255.255:0")); + EXPECT_CALL(connection, localAddress()) + .WillRepeatedly(ReturnRefOfCopy(std::string("tcp://128.255.255.255:0"))); EXPECT_EQ(std::string("with_destination_ip_list"), config_obj.getRouteFromEntries(connection)); } { // hit route with destination port range NiceMock connection; - EXPECT_CALL(connection, destinationAddress()).WillRepeatedly(Return("tcp://1.2.3.4:12345")); + EXPECT_CALL(connection, localAddress()) + .WillRepeatedly(ReturnRefOfCopy(std::string("tcp://1.2.3.4:12345"))); EXPECT_EQ(std::string("with_destination_ports"), config_obj.getRouteFromEntries(connection)); } { // fall through NiceMock connection; - EXPECT_CALL(connection, destinationAddress()).WillRepeatedly(Return("tcp://1.2.3.4:23456")); + EXPECT_CALL(connection, localAddress()) + .WillRepeatedly(ReturnRefOfCopy(std::string("tcp://1.2.3.4:23456"))); EXPECT_CALL(connection, remoteAddress()) .WillRepeatedly(ReturnRefOfCopy(std::string("tcp://0.0.0.0:0"))); EXPECT_EQ(std::string("catch_all"), config_obj.getRouteFromEntries(connection)); @@ -221,7 +232,8 @@ TEST(TcpProxyConfigTest, Routes) { { // hit route with source port range NiceMock connection; - EXPECT_CALL(connection, destinationAddress()).WillRepeatedly(Return("tcp://1.2.3.4:23456")); + EXPECT_CALL(connection, localAddress()) + .WillRepeatedly(ReturnRefOfCopy(std::string("tcp://1.2.3.4:23456"))); EXPECT_CALL(connection, remoteAddress()) .WillRepeatedly(ReturnRefOfCopy(std::string("tcp://0.0.0.0:23459"))); EXPECT_EQ(std::string("with_source_ports"), config_obj.getRouteFromEntries(connection)); @@ -230,7 +242,8 @@ TEST(TcpProxyConfigTest, Routes) { { // fall through NiceMock connection; - EXPECT_CALL(connection, destinationAddress()).WillRepeatedly(Return("tcp://1.2.3.4:23456")); + EXPECT_CALL(connection, localAddress()) + .WillRepeatedly(ReturnRefOfCopy(std::string("tcp://1.2.3.4:23456"))); EXPECT_CALL(connection, remoteAddress()) .WillRepeatedly(ReturnRefOfCopy(std::string("tcp://0.0.0.0:23458"))); EXPECT_EQ(std::string("catch_all"), config_obj.getRouteFromEntries(connection)); @@ -239,7 +252,8 @@ TEST(TcpProxyConfigTest, Routes) { { // hit the route with all criterias present NiceMock connection; - EXPECT_CALL(connection, destinationAddress()).WillRepeatedly(Return("tcp://10.0.0.0:10000")); + EXPECT_CALL(connection, localAddress()) + .WillRepeatedly(ReturnRefOfCopy(std::string("tcp://10.0.0.0:10000"))); EXPECT_CALL(connection, remoteAddress()) .WillRepeatedly(ReturnRefOfCopy(std::string("tcp://20.0.0.0:20000"))); EXPECT_EQ(std::string("with_everything"), config_obj.getRouteFromEntries(connection)); @@ -248,7 +262,8 @@ TEST(TcpProxyConfigTest, Routes) { { // fall through NiceMock connection; - EXPECT_CALL(connection, destinationAddress()).WillRepeatedly(Return("tcp://10.0.0.0:10000")); + EXPECT_CALL(connection, localAddress()) + .WillRepeatedly(ReturnRefOfCopy(std::string("tcp://10.0.0.0:10000"))); EXPECT_CALL(connection, remoteAddress()) .WillRepeatedly(ReturnRefOfCopy(std::string("tcp://30.0.0.0:20000"))); EXPECT_EQ(std::string("catch_all"), config_obj.getRouteFromEntries(connection)); diff --git a/test/mocks/network/mocks.h b/test/mocks/network/mocks.h index 4bff1841eb82..7dcc259bf2d6 100644 --- a/test/mocks/network/mocks.h +++ b/test/mocks/network/mocks.h @@ -52,7 +52,7 @@ class MockConnection : public Connection, public MockConnectionBase { MOCK_METHOD1(readDisable, void(bool disable)); MOCK_METHOD0(readEnabled, bool()); MOCK_METHOD0(remoteAddress, const std::string&()); - MOCK_METHOD0(destinationAddress, const std::string()); + MOCK_METHOD0(localAddress, const std::string&()); MOCK_METHOD1(setBufferStats, void(const BufferStats& stats)); MOCK_METHOD0(ssl, Ssl::Connection*()); MOCK_METHOD0(state, State()); @@ -82,7 +82,7 @@ class MockClientConnection : public ClientConnection, public MockConnectionBase MOCK_METHOD1(readDisable, void(bool disable)); MOCK_METHOD0(readEnabled, bool()); MOCK_METHOD0(remoteAddress, const std::string&()); - MOCK_METHOD0(destinationAddress, const std::string()); + MOCK_METHOD0(localAddress, const std::string&()); MOCK_METHOD1(setBufferStats, void(const BufferStats& stats)); MOCK_METHOD0(ssl, Ssl::Connection*()); MOCK_METHOD0(state, State()); From 6eecce285bfa7456f1f6252a7baad36b1da37b7f Mon Sep 17 00:00:00 2001 From: Enrico Schiattarella Date: Fri, 27 Jan 2017 16:22:40 -0800 Subject: [PATCH 20/22] Make local_addr part of connection and pass it down from listeners --- include/envoy/network/connection.h | 10 +-- source/common/filter/auth/client_ssl.cc | 4 +- source/common/filter/tcp_proxy.cc | 14 +++-- source/common/filter/tcp_proxy.h | 2 +- source/common/http/conn_manager_utility.cc | 9 ++- source/common/network/connection_impl.cc | 25 ++------ source/common/network/connection_impl.h | 5 +- source/common/network/listener_impl.cc | 62 ++++++++++--------- source/common/network/listener_impl.h | 13 ++-- source/common/network/proxy_protocol.cc | 9 +-- source/common/ssl/connection_impl.cc | 10 +-- source/common/ssl/connection_impl.h | 2 +- test/common/filter/auth/client_ssl_test.cc | 6 +- test/common/http/conn_manager_impl_test.cc | 3 + test/common/http/conn_manager_utility_test.cc | 31 ++++++---- test/common/network/connection_impl_test.cc | 3 +- test/common/network/listener_impl_test.cc | 17 ++--- 17 files changed, 121 insertions(+), 104 deletions(-) diff --git a/include/envoy/network/connection.h b/include/envoy/network/connection.h index e48958aeff76..e34b019ec547 100644 --- a/include/envoy/network/connection.h +++ b/include/envoy/network/connection.h @@ -111,14 +111,16 @@ class Connection : public Event::DeferredDeletable, public FilterManager { virtual bool readEnabled() PURE; /** - * @return The address of the remote client + * @return The address of the remote client. + * For TCP connections, it is in the form tcp://a.b.c.d:port */ virtual const std::string& remoteAddress() PURE; /** - * @return The address the remote client is trying to connect to. - * It can be different from the proxy address if the downstream connection - * has been redirected or the proxy is operating in transparent mode. + * @return the local address of the connection. For client connections, this is the origin + * address. For server connections, this is the local destination address. For server connections + * it can be different from the proxy address if the downstream connection has been redirected or + * the proxy is operating in transparent mode. */ virtual const std::string& localAddress() PURE; diff --git a/source/common/filter/auth/client_ssl.cc b/source/common/filter/auth/client_ssl.cc index fab197b72bfd..6e7314b94a20 100644 --- a/source/common/filter/auth/client_ssl.cc +++ b/source/common/filter/auth/client_ssl.cc @@ -8,6 +8,7 @@ #include "common/http/message_impl.h" #include "common/http/utility.h" #include "common/json/config_schemas.h" +#include "common/network/utility.h" namespace Filter { namespace Auth { @@ -97,7 +98,8 @@ void Instance::onEvent(uint32_t events) { } ASSERT(read_callbacks_->connection().ssl()); - if (config_->ipWhiteList().contains(read_callbacks_->connection().remoteAddress())) { + if (config_->ipWhiteList().contains( + Network::Utility::hostFromUrl(read_callbacks_->connection().remoteAddress()))) { config_->stats().auth_ip_white_list_.inc(); read_callbacks_->continueReading(); return; diff --git a/source/common/filter/tcp_proxy.cc b/source/common/filter/tcp_proxy.cc index 36114fd68e48..42bee5c3c255 100644 --- a/source/common/filter/tcp_proxy.cc +++ b/source/common/filter/tcp_proxy.cc @@ -15,11 +15,7 @@ namespace Filter { TcpProxyConfig::Route::Route(const Json::Object& config) { - if (config.hasObject("cluster")) { - cluster_name_ = config.getString("cluster"); - } else { - throw EnvoyException("tcp proxy: route without cluster"); - } + cluster_name_ = config.getString("cluster"); if (config.hasObject("source_ip_list")) { source_ips_ = Network::IpList(config.getStringArray("source_ip_list")); @@ -68,6 +64,14 @@ const std::string& TcpProxyConfig::getRouteFromEntries(Network::Connection& conn continue; // no match, try next route } + // If the route needs to match on destination address and port but they are not available + // (localAddress is empty), we skip it. The connection has a chance to match a different + // route that does not depend on destination address and port. + if ((!route.destination_port_ranges_.empty() || !route.destination_ips_.empty()) && + connection.localAddress().empty()) { + continue; + } + if (!route.destination_port_ranges_.empty() && !Network::Utility::portInRangeList(Network::Utility::portFromUrl(connection.localAddress()), route.destination_port_ranges_)) { diff --git a/source/common/filter/tcp_proxy.h b/source/common/filter/tcp_proxy.h index 38de94f9277f..6b87be460cb1 100644 --- a/source/common/filter/tcp_proxy.h +++ b/source/common/filter/tcp_proxy.h @@ -47,7 +47,7 @@ class TcpProxyConfig { * @param connection supplies the parameters of the downstream connection for * which the proxy needs to open the corresponding upstream. * @return the cluster name to be used for the upstream connection. - If no route applies, returns the empty string. + * If no route applies, returns the empty string. */ const std::string& getRouteFromEntries(Network::Connection& connection); diff --git a/source/common/http/conn_manager_utility.cc b/source/common/http/conn_manager_utility.cc index e95f483bc159..739ccf36e02d 100644 --- a/source/common/http/conn_manager_utility.cc +++ b/source/common/http/conn_manager_utility.cc @@ -1,5 +1,6 @@ #include "conn_manager_utility.h" +#include "common/common/empty_string.h" #include "common/http/access_log/access_log_formatter.h" #include "common/http/headers.h" #include "common/http/utility.h" @@ -38,10 +39,11 @@ void ConnectionManagerUtility::mutateRequestHeaders(Http::HeaderMap& request_hea // peer. Cases where we don't "use remote address" include trusted double proxy where we expect // our peer to have already properly set XFF, etc. if (config.useRemoteAddress()) { - if (Network::Utility::isLoopbackAddress(connection.remoteAddress().c_str())) { + const std::string& remote_address = Network::Utility::hostFromUrl(connection.remoteAddress()); + if (Network::Utility::isLoopbackAddress(remote_address.c_str())) { Utility::appendXff(request_headers, config.localAddress()); } else { - Utility::appendXff(request_headers, connection.remoteAddress()); + Utility::appendXff(request_headers, remote_address); } request_headers.insertForwardedProto().value( connection.ssl() ? Headers::get().SchemeValues.Https : Headers::get().SchemeValues.Http); @@ -94,7 +96,8 @@ void ConnectionManagerUtility::mutateRequestHeaders(Http::HeaderMap& request_hea // If we are an external request, AND we are "using remote address" (see above), we set // x-envoy-external-address since this is our first ingress point into the trusted network. if (edge_request) { - request_headers.insertEnvoyExternalAddress().value(connection.remoteAddress()); + request_headers.insertEnvoyExternalAddress().value( + Network::Utility::hostFromUrl(connection.remoteAddress())); } // Generate x-request-id for all edge requests, or if there is none. diff --git a/source/common/network/connection_impl.cc b/source/common/network/connection_impl.cc index 05e95b17e7f0..b76b0b5ba82a 100644 --- a/source/common/network/connection_impl.cc +++ b/source/common/network/connection_impl.cc @@ -34,10 +34,9 @@ void ConnectionImplUtility::updateBufferStats(uint64_t delta, uint64_t new_total std::atomic ConnectionImpl::next_global_id_; ConnectionImpl::ConnectionImpl(Event::DispatcherImpl& dispatcher, int fd, - const std::string& remote_address) - : filter_manager_(*this, *this), remote_address_(remote_address), - local_address_(getLocalAddress(fd)), dispatcher_(dispatcher), fd_(fd), - id_(++next_global_id_) { + const std::string& remote_address, const std::string& local_address) + : filter_manager_(*this, *this), remote_address_(remote_address), local_address_(local_address), + dispatcher_(dispatcher), fd_(fd), id_(++next_global_id_) { // Treat the lack of a valid fd (which in practice only happens if we run out of FDs) as an OOM // condition and just crash. @@ -397,24 +396,10 @@ void ConnectionImpl::updateWriteBufferStats(uint64_t num_written, uint64_t new_s buffer_stats_->write_current_); } -const std::string Network::ConnectionImpl::getLocalAddress(int fd) { - if (fd != -1) { - sockaddr_storage orig_dst_addr; - memset(&orig_dst_addr, 0, sizeof(orig_dst_addr)); - bool success = Utility::getOriginalDst(fd, &orig_dst_addr); - if (success) { - return Utility::urlForTcp( - Utility::getAddressName(reinterpret_cast(&orig_dst_addr)), - Utility::getAddressPort(reinterpret_cast(&orig_dst_addr))); - } - } - - return EMPTY_STRING; -} - +// TODO: see if we can pass something more meaningful than EMPTY_STRING as localAddress ClientConnectionImpl::ClientConnectionImpl(Event::DispatcherImpl& dispatcher, int fd, const std::string& url) - : ConnectionImpl(dispatcher, fd, url) {} + : ConnectionImpl(dispatcher, fd, url, EMPTY_STRING) {} Network::ClientConnectionPtr ClientConnectionImpl::create(Event::DispatcherImpl& dispatcher, const std::string& url) { diff --git a/source/common/network/connection_impl.h b/source/common/network/connection_impl.h index 3544063e2b6f..d96304a99329 100644 --- a/source/common/network/connection_impl.h +++ b/source/common/network/connection_impl.h @@ -37,7 +37,9 @@ class ConnectionImpl : public virtual Connection, public BufferSource, protected Logger::Loggable { public: - ConnectionImpl(Event::DispatcherImpl& dispatcher, int fd, const std::string& remote_address); + ConnectionImpl(Event::DispatcherImpl& dispatcher, int fd, const std::string& remote_address, + const std::string& local_address); + ~ConnectionImpl(); // Network::FilterManager @@ -104,7 +106,6 @@ class ConnectionImpl : public virtual Connection, void onWriteReady(); void updateReadBufferStats(uint64_t num_read, uint64_t new_size); void updateWriteBufferStats(uint64_t num_written, uint64_t new_size); - static const std::string getLocalAddress(int fd); static std::atomic next_global_id_; diff --git a/source/common/network/listener_impl.cc b/source/common/network/listener_impl.cc index ae83f41b95f2..87539f1df07f 100644 --- a/source/common/network/listener_impl.cc +++ b/source/common/network/listener_impl.cc @@ -14,36 +14,34 @@ namespace Network { -void ListenerImpl::listenCallback(evconnlistener*, evutil_socket_t fd, sockaddr* addr, int, +void ListenerImpl::listenCallback(evconnlistener*, evutil_socket_t fd, sockaddr* remote_addr, int, void* arg) { ListenerImpl* listener = static_cast(arg); - if (listener->use_original_dst_) { - sockaddr_storage orig_dst_addr; - memset(&orig_dst_addr, 0, sizeof(orig_dst_addr)); + sockaddr_storage local_addr; + memset(&local_addr, 0, sizeof(local_addr)); - bool success = Utility::getOriginalDst(fd, &orig_dst_addr); + bool success = Utility::getOriginalDst(fd, &local_addr); - if (success) { - std::string orig_sock_name = std::to_string( - listener->getAddressPort(reinterpret_cast(&orig_dst_addr))); + if (success && listener->use_original_dst_) { + std::string orig_sock_name = + std::to_string(listener->getAddressPort(reinterpret_cast(&local_addr))); - // A listener that has the use_original_dst flag set to true can still receive connections - // that are NOT redirected using iptables. If a connection was not redirected, - // the address and port returned by getOriginalDst() match the listener port. - // In this case the listener handles the connection directly and does not hand it off. - if (listener->socket_.name() != orig_sock_name) { - ListenerImpl* new_listener = - dynamic_cast(listener->connection_handler_.findListener(orig_sock_name)); + // A listener that has the use_original_dst flag set to true can still receive connections + // that are NOT redirected using iptables. If a connection was not redirected, + // the address and port returned by getOriginalDst() match the listener port. + // In this case the listener handles the connection directly and does not hand it off. + if (listener->socket_.name() != orig_sock_name) { + ListenerImpl* new_listener = + dynamic_cast(listener->connection_handler_.findListener(orig_sock_name)); - if (new_listener != nullptr) { - listener = new_listener; - } + if (new_listener != nullptr) { + listener = new_listener; } } } - listener->newConnection(fd, addr); + listener->newConnection(fd, remote_addr, reinterpret_cast(&local_addr)); } ListenerImpl::ListenerImpl(Network::ConnectionHandler& conn_handler, @@ -72,34 +70,38 @@ void ListenerImpl::errorCallback(evconnlistener*, void*) { PANIC(fmt::format("listener accept failure: {}", strerror(errno))); } -void ListenerImpl::newConnection(int fd, sockaddr* addr) { +void ListenerImpl::newConnection(int fd, sockaddr* remote_addr, sockaddr* local_addr) { evutil_make_socket_nonblocking(fd); if (use_proxy_proto_) { proxy_protocol_.newConnection(dispatcher_, fd, *this); } else { - newConnection(fd, getAddressName(addr), getAddressPort(addr)); + newConnection( + fd, Network::Utility::urlForTcp(getAddressName(remote_addr), getAddressPort(remote_addr)), + Network::Utility::urlForTcp(getAddressName(local_addr), getAddressPort(local_addr))); } } -void ListenerImpl::newConnection(int fd, const std::string& remote_address, uint32_t remote_port) { - ConnectionPtr new_connection(new ConnectionImpl( - dispatcher_, fd, Network::Utility::urlForTcp(remote_address, remote_port))); +void ListenerImpl::newConnection(int fd, const std::string& remote_address, + const std::string& local_address) { + ConnectionPtr new_connection(new ConnectionImpl(dispatcher_, fd, remote_address, local_address)); cb_.onNewConnection(std::move(new_connection)); } -void SslListenerImpl::newConnection(int fd, sockaddr* addr) { +void SslListenerImpl::newConnection(int fd, sockaddr* remote_addr, sockaddr* local_addr) { if (use_proxy_proto_) { proxy_protocol_.newConnection(dispatcher_, fd, *this); } else { - newConnection(fd, getAddressName(addr), getAddressPort(addr)); + newConnection( + fd, Network::Utility::urlForTcp(getAddressName(remote_addr), getAddressPort(remote_addr)), + Network::Utility::urlForTcp(getAddressName(local_addr), getAddressPort(local_addr))); } } void SslListenerImpl::newConnection(int fd, const std::string& remote_address, - uint32_t remote_port) { - ConnectionPtr new_connection(new Ssl::ConnectionImpl( - dispatcher_, fd, Network::Utility::urlForTcp(remote_address, remote_port), ssl_ctx_, - Ssl::ConnectionImpl::InitialState::Server)); + const std::string& local_address) { + ConnectionPtr new_connection(new Ssl::ConnectionImpl(dispatcher_, fd, remote_address, + local_address, ssl_ctx_, + Ssl::ConnectionImpl::InitialState::Server)); cb_.onNewConnection(std::move(new_connection)); } diff --git a/source/common/network/listener_impl.h b/source/common/network/listener_impl.h index 1092166a4792..c3320466ee43 100644 --- a/source/common/network/listener_impl.h +++ b/source/common/network/listener_impl.h @@ -26,16 +26,18 @@ class ListenerImpl : public Listener { * Accept/process a new connection. * @param fd supplies the new connection's fd. * @param remote_address supplies the remote address for the new connection. + * @param local_address supplies the local address for the new connection. */ - virtual void newConnection(int fd, sockaddr* addr); + virtual void newConnection(int fd, sockaddr* remote_address, sockaddr* local_address); /** * Accept/process a new connection with the given remote address. * @param fd supplies the new connection's fd. * @param remote_address supplies the remote address for the new connection. - * @param remote_address supplies the remote port for the new connection. + * @param remote_address supplies the local address for the new connection. */ - virtual void newConnection(int fd, const std::string& remote_address, uint32_t remote_port); + virtual void newConnection(int fd, const std::string& remote_address, + const std::string& local_address); /** * @return the socket supplied to the listener at construction time @@ -73,8 +75,9 @@ class SslListenerImpl : public ListenerImpl { ssl_ctx_(ssl_ctx) {} // ListenerImpl - void newConnection(int fd, sockaddr* addr) override; - void newConnection(int fd, const std::string& remote_address, uint32_t remote_port) override; + void newConnection(int fd, sockaddr* remote_addr, sockaddr* local_addr) override; + void newConnection(int fd, const std::string& remote_address, + const std::string& local_address) override; private: Ssl::Context& ssl_ctx_; diff --git a/source/common/network/proxy_protocol.cc b/source/common/network/proxy_protocol.cc index 1a0f856f82e5..58c087b997f0 100644 --- a/source/common/network/proxy_protocol.cc +++ b/source/common/network/proxy_protocol.cc @@ -6,6 +6,9 @@ #include "envoy/event/file_event.h" #include "envoy/stats/stats.h" +#include "common/common/empty_string.h" +#include "common/network/utility.h" + namespace Network { const std::string ProxyProtocol::ActiveConnection::PROXY_TCP4 = "PROXY TCP4 "; @@ -68,10 +71,8 @@ void ProxyProtocol::ActiveConnection::onReadWorker() { removeFromList(parent_.connections_); - // Technically we could extract the remote port from the PROXY protocol header - // and pass it to the listener. However, the listener does not care about - // client-side ports for downstream connections, so we can just pass a 0 - listener.newConnection(fd, remote_address, 0); + // TODO: pass in something more meaningful than 0 as remote port and EMPTY_STRING as local_address + listener.newConnection(fd, Network::Utility::urlForTcp(remote_address, 0), EMPTY_STRING); } void ProxyProtocol::ActiveConnection::close() { diff --git a/source/common/ssl/connection_impl.cc b/source/common/ssl/connection_impl.cc index 901e49e4268a..d36ce9f7a176 100644 --- a/source/common/ssl/connection_impl.cc +++ b/source/common/ssl/connection_impl.cc @@ -10,8 +10,9 @@ namespace Ssl { ConnectionImpl::ConnectionImpl(Event::DispatcherImpl& dispatcher, int fd, - const std::string& remote_address, Context& ctx, InitialState state) - : Network::ConnectionImpl(dispatcher, fd, remote_address), + const std::string& remote_address, const std::string& local_address, + Context& ctx, InitialState state) + : Network::ConnectionImpl(dispatcher, fd, remote_address, local_address), ctx_(dynamic_cast(ctx)), ssl_(ctx_.newSsl()) { BIO* bio = BIO_new_socket(fd, 0); SSL_set_bio(ssl_.get(), bio, bio); @@ -185,10 +186,11 @@ std::string ConnectionImpl::sha256PeerCertificateDigest() { return Hex::encode(computed_hash); } +// TODO: see if we can pass something more meaningful than EMPTY_STRING as localAddress ClientConnectionImpl::ClientConnectionImpl(Event::DispatcherImpl& dispatcher, Context& ctx, const std::string& url) - : ConnectionImpl(dispatcher, socket(AF_INET, SOCK_STREAM | SOCK_NONBLOCK, 0), url, ctx, - InitialState::Client) {} + : ConnectionImpl(dispatcher, socket(AF_INET, SOCK_STREAM | SOCK_NONBLOCK, 0), url, EMPTY_STRING, + ctx, InitialState::Client) {} void ClientConnectionImpl::connect() { Network::AddrInfoPtr addr_info = diff --git a/source/common/ssl/connection_impl.h b/source/common/ssl/connection_impl.h index 0f38d29275b9..7e87fd16b744 100644 --- a/source/common/ssl/connection_impl.h +++ b/source/common/ssl/connection_impl.h @@ -11,7 +11,7 @@ class ConnectionImpl : public Network::ConnectionImpl, public Connection { enum class InitialState { Client, Server }; ConnectionImpl(Event::DispatcherImpl& dispatcher, int fd, const std::string& remote_address, - Context& ctx, InitialState state); + const std::string& local_address, Context& ctx, InitialState state); ~ConnectionImpl(); // Network::Connection diff --git a/test/common/filter/auth/client_ssl_test.cc b/test/common/filter/auth/client_ssl_test.cc index be63b4b472dc..76852913f218 100644 --- a/test/common/filter/auth/client_ssl_test.cc +++ b/test/common/filter/auth/client_ssl_test.cc @@ -136,7 +136,7 @@ TEST_F(ClientSslAuthFilterTest, Ssl) { createAuthFilter(); ON_CALL(filter_callbacks_.connection_, ssl()).WillByDefault(Return(&ssl_)); EXPECT_CALL(filter_callbacks_.connection_, remoteAddress()) - .WillOnce(ReturnRefOfCopy(std::string("192.168.1.1"))); + .WillOnce(ReturnRefOfCopy(std::string("tcp://192.168.1.1:0"))); EXPECT_CALL(ssl_, sha256PeerCertificateDigest()).WillOnce(Return("digest")); EXPECT_CALL(filter_callbacks_.connection_, close(Network::ConnectionCloseType::NoFlush)); EXPECT_EQ(Network::FilterStatus::StopIteration, instance_->onNewConnection()); @@ -155,7 +155,7 @@ TEST_F(ClientSslAuthFilterTest, Ssl) { // Create a new filter for an SSL connection with an authorized cert. createAuthFilter(); EXPECT_CALL(filter_callbacks_.connection_, remoteAddress()) - .WillOnce(ReturnRefOfCopy(std::string("192.168.1.1"))); + .WillOnce(ReturnRefOfCopy(std::string("tcp://192.168.1.1:0"))); EXPECT_CALL(ssl_, sha256PeerCertificateDigest()) .WillOnce(Return("1b7d42ef0025ad89c1c911d6c10d7e86a4cb7c5863b2980abcbad1895f8b5314")); EXPECT_EQ(Network::FilterStatus::StopIteration, instance_->onNewConnection()); @@ -168,7 +168,7 @@ TEST_F(ClientSslAuthFilterTest, Ssl) { // White list case. createAuthFilter(); EXPECT_CALL(filter_callbacks_.connection_, remoteAddress()) - .WillOnce(ReturnRefOfCopy(std::string("1.2.3.4"))); + .WillOnce(ReturnRefOfCopy(std::string("tcp://1.2.3.4:0"))); EXPECT_EQ(Network::FilterStatus::StopIteration, instance_->onNewConnection()); EXPECT_CALL(filter_callbacks_, continueReading()); filter_callbacks_.connection_.raiseEvents(Network::ConnectionEvent::Connected); diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index 8fe39e781d20..0ead6024cf66 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -27,6 +27,7 @@ using testing::Invoke; using testing::NiceMock; using testing::Return; using testing::ReturnRef; +using testing::ReturnRefOfCopy; using testing::Sequence; using testing::Test; @@ -60,6 +61,8 @@ class HttpConnectionManagerImplTest : public Test, public ConnectionManagerConfi server_name_ = server_name; ON_CALL(filter_callbacks_.connection_, ssl()).WillByDefault(Return(ssl_connection_.get())); + ON_CALL(filter_callbacks_.connection_, remoteAddress()) + .WillByDefault(ReturnRefOfCopy(std::string("tcp://0.0.0.0:0"))); conn_manager_.reset(new ConnectionManagerImpl(*this, drain_close_, random_, tracer_, runtime_)); conn_manager_->initializeReadFilterCallbacks(filter_callbacks_); } diff --git a/test/common/http/conn_manager_utility_test.cc b/test/common/http/conn_manager_utility_test.cc index aa04e4acd3b8..29d40a96305b 100644 --- a/test/common/http/conn_manager_utility_test.cc +++ b/test/common/http/conn_manager_utility_test.cc @@ -1,5 +1,6 @@ #include "common/http/conn_manager_utility.h" #include "common/http/headers.h" +#include "common/network/utility.h" #include "common/runtime/runtime_impl.h" #include "common/runtime/uuid_util.h" @@ -46,7 +47,7 @@ TEST_F(ConnectionManagerUtilityTest, generateStreamId) { } TEST_F(ConnectionManagerUtilityTest, UseRemoteAddressWhenNotLocalHostRemoteAddress) { - const std::string not_local_host_remote_address = "12.12.12.12"; + const std::string not_local_host_remote_address = "tcp://12.12.12.12:0"; EXPECT_CALL(config_, useRemoteAddress()).WillRepeatedly(Return(true)); EXPECT_CALL(connection_, remoteAddress()) .WillRepeatedly(ReturnRef(not_local_host_remote_address)); @@ -55,11 +56,12 @@ TEST_F(ConnectionManagerUtilityTest, UseRemoteAddressWhenNotLocalHostRemoteAddre ConnectionManagerUtility::mutateRequestHeaders(headers, connection_, config_, random_, runtime_); EXPECT_TRUE(headers.has(Headers::get().ForwardedFor)); - EXPECT_EQ(not_local_host_remote_address, headers.get_(Headers::get().ForwardedFor)); + EXPECT_EQ(Network::Utility::hostFromUrl(not_local_host_remote_address), + headers.get_(Headers::get().ForwardedFor)); } TEST_F(ConnectionManagerUtilityTest, UseLocalAddressWhenLocalHostRemoteAddress) { - const std::string local_host_remote_address = "127.0.0.1"; + const std::string local_host_remote_address = "tcp://127.0.0.1:0"; const std::string local_address = "10.3.2.1"; EXPECT_CALL(connection_, remoteAddress()).WillRepeatedly(ReturnRef(local_host_remote_address)); @@ -74,7 +76,7 @@ TEST_F(ConnectionManagerUtilityTest, UseLocalAddressWhenLocalHostRemoteAddress) } TEST_F(ConnectionManagerUtilityTest, UserAgentDontSet) { - const std::string internal_remote_address = "10.0.0.1"; + const std::string internal_remote_address = "tcp://10.0.0.1:0"; EXPECT_CALL(config_, useRemoteAddress()).WillRepeatedly(Return(true)); EXPECT_CALL(connection_, remoteAddress()).WillRepeatedly(ReturnRef(internal_remote_address)); @@ -88,7 +90,7 @@ TEST_F(ConnectionManagerUtilityTest, UserAgentDontSet) { } TEST_F(ConnectionManagerUtilityTest, UserAgentSetWhenIncomingEmpty) { - const std::string internal_remote_address = "10.0.0.1"; + const std::string internal_remote_address = "tcp://10.0.0.1:0"; EXPECT_CALL(config_, useRemoteAddress()).WillRepeatedly(Return(true)); EXPECT_CALL(connection_, remoteAddress()).WillRepeatedly(ReturnRef(internal_remote_address)); @@ -137,7 +139,7 @@ TEST_F(ConnectionManagerUtilityTest, InternalServiceForceTrace) { } TEST_F(ConnectionManagerUtilityTest, EdgeRequestRegenerateRequestIdAndWipeDownstream) { - const std::string external_remote_address = "34.0.0.1"; + const std::string external_remote_address = "tcp://34.0.0.1:0"; const std::string generated_uuid = "f4dca0a9-12c7-4307-8002-969403baf480"; ON_CALL(config_, useRemoteAddress()).WillByDefault(Return(true)); @@ -208,7 +210,7 @@ TEST_F(ConnectionManagerUtilityTest, ExternalRequestPreserveRequestIdAndDownstre } TEST_F(ConnectionManagerUtilityTest, UserAgentSetIncomingUserAgent) { - const std::string internal_remote_address = "10.0.0.1"; + const std::string internal_remote_address = "tcp://10.0.0.1:0"; EXPECT_CALL(config_, useRemoteAddress()).WillRepeatedly(Return(true)); EXPECT_CALL(connection_, remoteAddress()).WillRepeatedly(ReturnRef(internal_remote_address)); @@ -223,7 +225,7 @@ TEST_F(ConnectionManagerUtilityTest, UserAgentSetIncomingUserAgent) { } TEST_F(ConnectionManagerUtilityTest, UserAgentSetNoIncomingUserAgent) { - const std::string internal_remote_address = "10.0.0.1"; + const std::string internal_remote_address = "tcp://10.0.0.1:0"; EXPECT_CALL(config_, useRemoteAddress()).WillRepeatedly(Return(true)); EXPECT_CALL(connection_, remoteAddress()).WillRepeatedly(ReturnRef(internal_remote_address)); @@ -263,7 +265,7 @@ TEST_F(ConnectionManagerUtilityTest, RequestIdGeneratedWhenItsNotPresent) { } TEST_F(ConnectionManagerUtilityTest, DoNotOverrideRequestIdIfPresentWhenInternalRequest) { - std::string local_remote_address = "10.0.0.1"; + std::string local_remote_address = "tcp://10.0.0.1:0"; EXPECT_CALL(config_, useRemoteAddress()).WillOnce(Return(true)); EXPECT_CALL(connection_, remoteAddress()).WillRepeatedly(ReturnRef(local_remote_address)); @@ -275,7 +277,7 @@ TEST_F(ConnectionManagerUtilityTest, DoNotOverrideRequestIdIfPresentWhenInternal } TEST_F(ConnectionManagerUtilityTest, OverrideRequestIdForExternalRequests) { - std::string external_ip = "134.2.2.11"; + std::string external_ip = "tcp://134.2.2.11:0"; EXPECT_CALL(connection_, remoteAddress()).WillRepeatedly(ReturnRef(external_ip)); TestHeaderMapImpl headers{{"x-request-id", "original"}}; @@ -287,7 +289,8 @@ TEST_F(ConnectionManagerUtilityTest, OverrideRequestIdForExternalRequests) { } TEST_F(ConnectionManagerUtilityTest, ExternalAddressExternalRequestUseRemote) { - ON_CALL(connection_, remoteAddress()).WillByDefault(ReturnRefOfCopy(std::string("50.0.0.1"))); + ON_CALL(connection_, remoteAddress()) + .WillByDefault(ReturnRefOfCopy(std::string("tcp://50.0.0.1:0"))); ON_CALL(config_, useRemoteAddress()).WillByDefault(Return(true)); config_.route_config_.internal_only_headers_.push_back(LowerCaseString("custom_header")); @@ -310,7 +313,8 @@ TEST_F(ConnectionManagerUtilityTest, ExternalAddressExternalRequestUseRemote) { } TEST_F(ConnectionManagerUtilityTest, ExternalAddressExternalRequestDontUseRemote) { - ON_CALL(connection_, remoteAddress()).WillByDefault(ReturnRefOfCopy(std::string("50.0.0.1"))); + ON_CALL(connection_, remoteAddress()) + .WillByDefault(ReturnRefOfCopy(std::string("tcp://50.0.0.1:0"))); ON_CALL(config_, useRemoteAddress()).WillByDefault(Return(false)); TestHeaderMapImpl headers{{"x-envoy-external-address", "60.0.0.1"}, @@ -322,7 +326,8 @@ TEST_F(ConnectionManagerUtilityTest, ExternalAddressExternalRequestDontUseRemote } TEST_F(ConnectionManagerUtilityTest, ExternalAddressInternalRequestUseRemote) { - ON_CALL(connection_, remoteAddress()).WillByDefault(ReturnRefOfCopy(std::string("10.0.0.1"))); + ON_CALL(connection_, remoteAddress()) + .WillByDefault(ReturnRefOfCopy(std::string("tcp://10.0.0.1:0"))); ON_CALL(config_, useRemoteAddress()).WillByDefault(Return(true)); TestHeaderMapImpl headers{{"x-envoy-external-address", "60.0.0.1"}, diff --git a/test/common/network/connection_impl_test.cc b/test/common/network/connection_impl_test.cc index 3c1feb6b10ef..d2a14961129e 100644 --- a/test/common/network/connection_impl_test.cc +++ b/test/common/network/connection_impl_test.cc @@ -1,4 +1,5 @@ #include "common/buffer/buffer_impl.h" +#include "common/common/empty_string.h" #include "common/event/dispatcher_impl.h" #include "common/network/connection_impl.h" #include "common/network/listen_socket_impl.h" @@ -43,7 +44,7 @@ TEST(ConnectionImplUtility, updateBufferStats) { TEST(ConnectionImplDeathTest, BadFd) { Event::DispatcherImpl dispatcher; - EXPECT_DEATH(ConnectionImpl(dispatcher, -1, "tcp://127.0.0.1:0"), + EXPECT_DEATH(ConnectionImpl(dispatcher, -1, "tcp://127.0.0.1:0", EMPTY_STRING), ".*assert failure: fd_ != -1.*"); } diff --git a/test/common/network/listener_impl_test.cc b/test/common/network/listener_impl_test.cc index d33770c781d5..2f4d3cf6fb1e 100644 --- a/test/common/network/listener_impl_test.cc +++ b/test/common/network/listener_impl_test.cc @@ -49,11 +49,14 @@ class TestListenerImpl : public ListenerImpl { ~TestListenerImpl() {} MOCK_METHOD1(getAddressPort_, uint16_t(sockaddr*)); - MOCK_METHOD2(newConnection_, void(int, sockaddr*)); + MOCK_METHOD3(newConnection_, void(int, sockaddr*, sockaddr*)); - void newConnection(int fd, sockaddr* addr) override { newConnection_(fd, addr); } - void newConnection(int fd, const std::string& addr, uint32_t port) override { - ListenerImpl::newConnection(fd, addr, port); + void newConnection(int fd, sockaddr* remote_addr, sockaddr* local_addr) override { + newConnection_(fd, remote_addr, local_addr); + } + void newConnection(int fd, const std::string& remote_addr, + const std::string& local_addr) override { + ListenerImpl::newConnection(fd, remote_addr, local_addr); } protected: @@ -79,10 +82,10 @@ TEST(ListenerImplTest, UseOriginalDst) { EXPECT_CALL(listener, getAddressPort_(_)).WillRepeatedly(Return(10001)); EXPECT_CALL(connection_handler, findListener("10001")).WillRepeatedly(Return(&listenerDst)); - EXPECT_CALL(listener, newConnection_(_, _)).Times(0); - EXPECT_CALL(listenerDst, newConnection_(_, _)) + EXPECT_CALL(listener, newConnection_(_, _, _)).Times(0); + EXPECT_CALL(listenerDst, newConnection_(_, _, _)) .Times(1) - .WillOnce(Invoke([&](int, sockaddr*) -> void { + .WillOnce(Invoke([&](int, sockaddr*, sockaddr*) -> void { client_connection->close(ConnectionCloseType::NoFlush); dispatcher.exit(); })); From 435ac47fb644deaace66b3d6c3108de99f245704 Mon Sep 17 00:00:00 2001 From: Enrico Schiattarella Date: Tue, 31 Jan 2017 00:21:07 -0800 Subject: [PATCH 21/22] TCP proxy routing: config validation logic, test for non-routable connections --- include/envoy/network/connection.h | 1 - source/common/filter/auth/client_ssl.cc | 2 +- source/common/filter/auth/client_ssl.h | 4 +- source/common/filter/tcp_proxy.cc | 13 +++- source/common/http/conn_manager_utility.cc | 2 +- source/common/network/listener_impl.h | 2 +- source/common/network/utility.cc | 10 ++- source/common/network/utility.h | 14 +--- test/common/filter/tcp_proxy_test.cc | 85 +++++++++++++++++++--- test/common/network/utility_test.cc | 52 ++++++++++--- 10 files changed, 143 insertions(+), 42 deletions(-) diff --git a/include/envoy/network/connection.h b/include/envoy/network/connection.h index e34b019ec547..b6c49e0e8ec5 100644 --- a/include/envoy/network/connection.h +++ b/include/envoy/network/connection.h @@ -112,7 +112,6 @@ class Connection : public Event::DeferredDeletable, public FilterManager { /** * @return The address of the remote client. - * For TCP connections, it is in the form tcp://a.b.c.d:port */ virtual const std::string& remoteAddress() PURE; diff --git a/source/common/filter/auth/client_ssl.cc b/source/common/filter/auth/client_ssl.cc index 6e7314b94a20..91c5fc47907a 100644 --- a/source/common/filter/auth/client_ssl.cc +++ b/source/common/filter/auth/client_ssl.cc @@ -19,7 +19,7 @@ Config::Config(const Json::Object& config, ThreadLocal::Instance& tls, Upstream: Runtime::RandomGenerator& random) : RestApiFetcher(cm, config.getString("auth_api_cluster"), dispatcher, random, std::chrono::milliseconds(config.getInteger("refresh_interval_ms", 60000))), - tls_(tls), tls_slot_(tls.allocateSlot()), ip_white_list_(config), + tls_(tls), tls_slot_(tls.allocateSlot()), ip_white_list_(config, "ip_white_list"), stats_(generateStats(stats_store, config.getString("stat_prefix"))) { config.validateSchema(Json::Schema::CLIENT_SSL_NETWORK_FILTER_SCHEMA); diff --git a/source/common/filter/auth/client_ssl.h b/source/common/filter/auth/client_ssl.h index 39f16c7b024a..d85e6f04e6cc 100644 --- a/source/common/filter/auth/client_ssl.h +++ b/source/common/filter/auth/client_ssl.h @@ -74,7 +74,7 @@ class Config : public Http::RestApiFetcher { Stats::Store& stats_store, Runtime::RandomGenerator& random); const AllowedPrincipals& allowedPrincipals(); - const Network::IpWhiteList& ipWhiteList() { return ip_white_list_; } + const Network::IpList& ipWhiteList() { return ip_white_list_; } GlobalStats& stats() { return stats_; } private: @@ -92,7 +92,7 @@ class Config : public Http::RestApiFetcher { ThreadLocal::Instance& tls_; uint32_t tls_slot_; - Network::IpWhiteList ip_white_list_; + Network::IpList ip_white_list_; GlobalStats stats_; }; diff --git a/source/common/filter/tcp_proxy.cc b/source/common/filter/tcp_proxy.cc index 42bee5c3c255..0c89814542ba 100644 --- a/source/common/filter/tcp_proxy.cc +++ b/source/common/filter/tcp_proxy.cc @@ -22,7 +22,11 @@ TcpProxyConfig::Route::Route(const Json::Object& config) { } if (config.hasObject("source_ports")) { - Network::Utility::parsePortRangeList(config.getString("source_ports"), source_port_ranges_); + const std::string source_ports = config.getString("source_ports"); + if (source_ports.empty()) { + throw EnvoyException("source_ports cannot be empty"); + } + Network::Utility::parsePortRangeList(source_ports, source_port_ranges_); } if (config.hasObject("destination_ip_list")) { @@ -30,8 +34,11 @@ TcpProxyConfig::Route::Route(const Json::Object& config) { } if (config.hasObject("destination_ports")) { - Network::Utility::parsePortRangeList(config.getString("destination_ports"), - destination_port_ranges_); + const std::string destination_ports = config.getString("destination_ports"); + if (destination_ports.empty()) { + throw EnvoyException("destination_ports cannot be empty"); + } + Network::Utility::parsePortRangeList(destination_ports, destination_port_ranges_); } } diff --git a/source/common/http/conn_manager_utility.cc b/source/common/http/conn_manager_utility.cc index 739ccf36e02d..b9e65769a636 100644 --- a/source/common/http/conn_manager_utility.cc +++ b/source/common/http/conn_manager_utility.cc @@ -39,7 +39,7 @@ void ConnectionManagerUtility::mutateRequestHeaders(Http::HeaderMap& request_hea // peer. Cases where we don't "use remote address" include trusted double proxy where we expect // our peer to have already properly set XFF, etc. if (config.useRemoteAddress()) { - const std::string& remote_address = Network::Utility::hostFromUrl(connection.remoteAddress()); + const std::string remote_address = Network::Utility::hostFromUrl(connection.remoteAddress()); if (Network::Utility::isLoopbackAddress(remote_address.c_str())) { Utility::appendXff(request_headers, config.localAddress()); } else { diff --git a/source/common/network/listener_impl.h b/source/common/network/listener_impl.h index c3320466ee43..5f809c9193f7 100644 --- a/source/common/network/listener_impl.h +++ b/source/common/network/listener_impl.h @@ -34,7 +34,7 @@ class ListenerImpl : public Listener { * Accept/process a new connection with the given remote address. * @param fd supplies the new connection's fd. * @param remote_address supplies the remote address for the new connection. - * @param remote_address supplies the local address for the new connection. + * @param local_address supplies the local address for the new connection. */ virtual void newConnection(int fd, const std::string& remote_address, const std::string& local_address); diff --git a/source/common/network/utility.cc b/source/common/network/utility.cc index 161f4b0c4aae..7dc920ee2100 100644 --- a/source/common/network/utility.cc +++ b/source/common/network/utility.cc @@ -67,9 +67,9 @@ bool IpList::contains(const std::string& remote_address) const { return false; } -IpWhiteList::IpWhiteList(const Json::Object& config) - : ip_list_(config.hasObject("ip_white_list") ? config.getStringArray("ip_white_list") - : std::vector()) {} +IpList::IpList(const Json::Object& config, const std::string& member_name) + : IpList(config.hasObject(member_name) ? config.getStringArray(member_name) + : std::vector()) {} const std::string Utility::TCP_SCHEME = "tcp://"; const std::string Utility::UNIX_SCHEME = "unix://"; @@ -251,6 +251,10 @@ void Utility::parsePortRangeList(const std::string& string, std::list max = min; } + if (s.empty() || (min > 65535) || (max > 65535) || ss.fail() || !ss.eof()) { + throw EnvoyException(fmt::format("invalid port number or range '{}'", s)); + } + list.emplace_back(PortRange(min, max)); } } diff --git a/source/common/network/utility.h b/source/common/network/utility.h index 9dc2ce3e008d..49e770c0762c 100644 --- a/source/common/network/utility.h +++ b/source/common/network/utility.h @@ -19,6 +19,7 @@ namespace Network { class IpList { public: IpList(const std::vector& subnets); + IpList(const Json::Object& config, const std::string& member_name); IpList(){}; bool contains(const std::string& address) const; @@ -33,15 +34,6 @@ class IpList { std::vector ipv4_list_; }; -class IpWhiteList { -public: - IpWhiteList(const Json::Object& config); - bool contains(const std::string& address) const { return ip_list_.contains(address); } - -private: - IpList ip_list_; -}; - /** * Utility class to represent TCP/UDP port range */ @@ -52,8 +44,8 @@ class PortRange { bool contains(uint32_t port) const { return (port >= min_ && port <= max_); } private: - uint32_t min_; - uint32_t max_; + const uint32_t min_; + const uint32_t max_; }; typedef std::list PortRangeList; diff --git a/test/common/filter/tcp_proxy_test.cc b/test/common/filter/tcp_proxy_test.cc index cd1fd5a8b76b..a842d6c6cc31 100644 --- a/test/common/filter/tcp_proxy_test.cc +++ b/test/common/filter/tcp_proxy_test.cc @@ -120,15 +120,6 @@ TEST(TcpProxyConfigTest, Routes) { Json::ObjectPtr json_config = Json::Factory::LoadFromString(json); NiceMock cm_; - // The TcpProxyConfig constructor checks if the clusters mentioned in the route_config are valid. - // We need to make sure to return a non-null pointer for each, otherwise the constructor will - // throw an exception and fail. - EXPECT_CALL(cm_, get("with_destination_ip_list")).WillRepeatedly(Return(cm_.cluster_.info_)); - EXPECT_CALL(cm_, get("with_destination_ports")).WillRepeatedly(Return(cm_.cluster_.info_)); - EXPECT_CALL(cm_, get("with_source_ports")).WillRepeatedly(Return(cm_.cluster_.info_)); - EXPECT_CALL(cm_, get("with_everything")).WillRepeatedly(Return(cm_.cluster_.info_)); - EXPECT_CALL(cm_, get("catch_all")).WillRepeatedly(Return(cm_.cluster_.info_)); - TcpProxyConfig config_obj(*json_config, cm_, cm_.cluster_.info_->stats_store_); { @@ -455,4 +446,80 @@ TEST_F(TcpProxyTest, UpstreamConnectionLimit) { cluster_manager_.cluster_.info_->stats_store_.counter("upstream_cx_overflow").value()); } +class TcpProxyRoutingTest : public testing::Test { +public: + TcpProxyRoutingTest() { + std::string json = R"EOF( + { + "stat_prefix": "name", + "route_config": { + "routes": [ + { + "destination_ports": "1-9999", + "cluster": "fake_cluster" + } + ] + } + } + )EOF"; + + Json::ObjectPtr config = Json::Factory::LoadFromString(json); + config_.reset(new TcpProxyConfig(*config, cluster_manager_, + cluster_manager_.cluster_.info_->stats_store_)); + } + + void setup() { + EXPECT_CALL(filter_callbacks_, connection()).WillRepeatedly(ReturnRef(connection_)); + + filter_.reset(new TcpProxy(config_, cluster_manager_)); + filter_->initializeReadFilterCallbacks(filter_callbacks_); + } + + TcpProxyConfigPtr config_; + NiceMock connection_; + NiceMock filter_callbacks_; + NiceMock cluster_manager_; + std::unique_ptr filter_; +}; + +TEST_F(TcpProxyRoutingTest, NonRoutableConnection) { + uint32_t total_cx = config_->stats().downstream_cx_total_.value(); + uint32_t non_routable_cx = config_->stats().downstream_cx_no_route_.value(); + + setup(); + + // port 10000 is outside the specified destination port range + EXPECT_CALL(connection_, localAddress()) + .WillRepeatedly(ReturnRefOfCopy(std::string("tcp://1.2.3.4:10000"))); + + // getRouteFromEntries() returns an empty string if no route matches + EXPECT_CALL(cluster_manager_, get("")).WillRepeatedly(Return(nullptr)); + + // Expect filter to stop iteration and close connection + EXPECT_CALL(connection_, close(Network::ConnectionCloseType::NoFlush)); + EXPECT_EQ(Network::FilterStatus::StopIteration, filter_->onNewConnection()); + + EXPECT_EQ(total_cx + 1, config_->stats().downstream_cx_total_.value()); + EXPECT_EQ(non_routable_cx + 1, config_->stats().downstream_cx_no_route_.value()); +} + +TEST_F(TcpProxyRoutingTest, RoutableConnection) { + uint32_t total_cx = config_->stats().downstream_cx_total_.value(); + uint32_t non_routable_cx = config_->stats().downstream_cx_no_route_.value(); + + setup(); + + // port 9999 is within the specified destination port range + EXPECT_CALL(connection_, localAddress()) + .WillRepeatedly(ReturnRefOfCopy(std::string("tcp://1.2.3.4:9999"))); + + // Expect filter to try to open a connection to specified cluster + EXPECT_CALL(cluster_manager_, tcpConnForCluster_("fake_cluster")); + + filter_->onNewConnection(); + + EXPECT_EQ(total_cx + 1, config_->stats().downstream_cx_total_.value()); + EXPECT_EQ(non_routable_cx, config_->stats().downstream_cx_no_route_.value()); +} + } // Filter diff --git a/test/common/network/utility_test.cc b/test/common/network/utility_test.cc index bfcda90253d2..e51bab21b681 100644 --- a/test/common/network/utility_test.cc +++ b/test/common/network/utility_test.cc @@ -4,7 +4,7 @@ namespace Network { -TEST(IpWhiteListTest, Errors) { +TEST(IpListTest, Errors) { { std::string json = R"EOF( { @@ -13,7 +13,7 @@ TEST(IpWhiteListTest, Errors) { )EOF"; Json::ObjectPtr loader = Json::Factory::LoadFromString(json); - EXPECT_THROW({ IpWhiteList wl(*loader); }, EnvoyException); + EXPECT_THROW({ IpList wl(*loader, "ip_white_list"); }, EnvoyException); } { @@ -24,7 +24,7 @@ TEST(IpWhiteListTest, Errors) { )EOF"; Json::ObjectPtr loader = Json::Factory::LoadFromString(json); - EXPECT_THROW({ IpWhiteList wl(*loader); }, EnvoyException); + EXPECT_THROW({ IpList wl(*loader, "ip_white_list"); }, EnvoyException); } { @@ -35,7 +35,7 @@ TEST(IpWhiteListTest, Errors) { )EOF"; Json::ObjectPtr loader = Json::Factory::LoadFromString(json); - EXPECT_THROW({ IpWhiteList wl(*loader); }, EnvoyException); + EXPECT_THROW({ IpList wl(*loader, "ip_white_list"); }, EnvoyException); } { @@ -46,11 +46,11 @@ TEST(IpWhiteListTest, Errors) { )EOF"; Json::ObjectPtr loader = Json::Factory::LoadFromString(json); - EXPECT_THROW({ IpWhiteList wl(*loader); }, EnvoyException); + EXPECT_THROW({ IpList wl(*loader, "ip_white_list"); }, EnvoyException); } } -TEST(IpWhiteListTest, Normal) { +TEST(IpListTest, Normal) { std::string json = R"EOF( { "ip_white_list": [ @@ -62,7 +62,7 @@ TEST(IpWhiteListTest, Normal) { )EOF"; Json::ObjectPtr loader = Json::Factory::LoadFromString(json); - IpWhiteList wl(*loader); + IpList wl(*loader, "ip_white_list"); EXPECT_TRUE(wl.contains("192.168.3.0")); EXPECT_TRUE(wl.contains("192.168.3.3")); @@ -83,7 +83,7 @@ TEST(IpWhiteListTest, Normal) { EXPECT_FALSE(wl.contains("")); } -TEST(IpWhiteListTest, MatchAny) { +TEST(IpListTest, MatchAny) { std::string json = R"EOF( { "ip_white_list": [ @@ -93,7 +93,7 @@ TEST(IpWhiteListTest, MatchAny) { )EOF"; Json::ObjectPtr loader = Json::Factory::LoadFromString(json); - IpWhiteList wl(*loader); + IpList wl(*loader, "ip_white_list"); EXPECT_TRUE(wl.contains("192.168.3.3")); EXPECT_TRUE(wl.contains("192.168.3.0")); @@ -138,7 +138,39 @@ TEST(NetworkUtility, loopbackAddress) { } } -TEST(NetworkUtility, PortRangeList) { +TEST(PortRangeListTest, Errors) { + { + std::string port_range_str = "a1"; + std::list port_range_list; + EXPECT_THROW(Utility::parsePortRangeList(port_range_str, port_range_list), EnvoyException); + } + + { + std::string port_range_str = "1A"; + std::list port_range_list; + EXPECT_THROW(Utility::parsePortRangeList(port_range_str, port_range_list), EnvoyException); + } + + { + std::string port_range_str = "1_1"; + std::list port_range_list; + EXPECT_THROW(Utility::parsePortRangeList(port_range_str, port_range_list), EnvoyException); + } + + { + std::string port_range_str = "1,1X1"; + std::list port_range_list; + EXPECT_THROW(Utility::parsePortRangeList(port_range_str, port_range_list), EnvoyException); + } + + { + std::string port_range_str = "1,1*1"; + std::list port_range_list; + EXPECT_THROW(Utility::parsePortRangeList(port_range_str, port_range_list), EnvoyException); + } +} + +TEST(PortRangeListTest, Normal) { { std::string port_range_str = "1"; std::list port_range_list; From 87d9a904557bd6aa2d65c0656ee2f1abdf03bc99 Mon Sep 17 00:00:00 2001 From: Enrico Schiattarella Date: Tue, 31 Jan 2017 17:08:05 -0800 Subject: [PATCH 22/22] Fix include order --- source/common/filter/tcp_proxy.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/common/filter/tcp_proxy.cc b/source/common/filter/tcp_proxy.cc index 0c89814542ba..90603dd0fa05 100644 --- a/source/common/filter/tcp_proxy.cc +++ b/source/common/filter/tcp_proxy.cc @@ -8,8 +8,8 @@ #include "envoy/upstream/upstream.h" #include "common/common/assert.h" -#include "common/json/config_schemas.h" #include "common/common/empty_string.h" +#include "common/json/config_schemas.h" #include "common/json/json_loader.h" namespace Filter {