From 87bb932882bd3e37ff75fc782d71cf0623ffd049 Mon Sep 17 00:00:00 2001 From: Bjoern Metzdorf Date: Fri, 2 Feb 2018 22:01:37 -0800 Subject: [PATCH 01/31] Add TCP_FASTOPEN listener option Signed-off-by: Bjoern Metzdorf --- bazel/repository_locations.bzl | 2 +- include/envoy/event/dispatcher.h | 1 + include/envoy/network/listener.h | 5 + source/common/event/dispatcher_impl.cc | 4 +- source/common/event/dispatcher_impl.h | 2 +- source/common/network/listener_impl.cc | 56 +++++- source/common/network/listener_impl.h | 3 +- source/server/config_validation/dispatcher.cc | 3 +- source/server/config_validation/dispatcher.h | 2 +- source/server/connection_handler_impl.cc | 10 +- source/server/http/admin.h | 1 + source/server/listener_manager_impl.cc | 1 + source/server/listener_manager_impl.h | 3 +- test/common/http/codec_client_test.cc | 3 +- test/common/network/connection_impl_test.cc | 6 +- test/common/network/dns_impl_test.cc | 2 +- test/common/network/listener_impl_test.cc | 16 +- test/common/network/proxy_protocol_test.cc | 2 + test/common/ssl/ssl_socket_test.cc | 25 +-- test/integration/fake_upstream.h | 1 + test/mocks/event/mocks.h | 10 +- test/mocks/network/mocks.h | 1 + test/server/connection_handler_test.cc | 176 +++++++++--------- 23 files changed, 208 insertions(+), 127 deletions(-) diff --git a/bazel/repository_locations.bzl b/bazel/repository_locations.bzl index e74714f832f73..ce3a0e58bc466 100644 --- a/bazel/repository_locations.bzl +++ b/bazel/repository_locations.bzl @@ -76,7 +76,7 @@ REPOSITORY_LOCATIONS = dict( urls = ["https://github.com/google/protobuf/archive/v3.5.0.tar.gz"], ), envoy_api = dict( - commit = "15dc537b6078988ac6f7de5ffec697e876a4652f", + commit = "b6925003b383f6f1213f5c62f96cd2cfd0c072be", remote = "https://github.com/envoyproxy/data-plane-api", ), grpc_httpjson_transcoding = dict( diff --git a/include/envoy/event/dispatcher.h b/include/envoy/event/dispatcher.h index 4edd5927e0b3b..5f84e46f7c4e0 100644 --- a/include/envoy/event/dispatcher.h +++ b/include/envoy/event/dispatcher.h @@ -102,6 +102,7 @@ class Dispatcher { */ virtual Network::ListenerPtr createListener(Network::Socket& socket, Network::ListenerCallbacks& cb, bool bind_to_port, + bool enable_tcp_fast_open, bool hand_off_restored_destination_connections) PURE; /** diff --git a/include/envoy/network/listener.h b/include/envoy/network/listener.h index 16cf4e5d481a3..a2d1e3e4173a9 100644 --- a/include/envoy/network/listener.h +++ b/include/envoy/network/listener.h @@ -44,6 +44,11 @@ class ListenerConfig { */ virtual bool bindToPort() PURE; + /** + * @return bool specifies whether the listener should support TCP Fast Open. + */ + virtual bool enableTcpFastOpen() PURE; + /** * @return bool if a connection should be handed off to another Listener after the original * destination address has been restored. 'true' when 'use_original_dst' flag in listener diff --git a/source/common/event/dispatcher_impl.cc b/source/common/event/dispatcher_impl.cc index b195d6ae29133..82df1a60ac504 100644 --- a/source/common/event/dispatcher_impl.cc +++ b/source/common/event/dispatcher_impl.cc @@ -109,9 +109,11 @@ Filesystem::WatcherPtr DispatcherImpl::createFilesystemWatcher() { Network::ListenerPtr DispatcherImpl::createListener(Network::Socket& socket, Network::ListenerCallbacks& cb, - bool bind_to_port, bool hand_off_restored_destination_connections) { + bool bind_to_port, bool enable_tcp_fast_open, + bool hand_off_restored_destination_connections) { ASSERT(isThreadSafe()); return Network::ListenerPtr{new Network::ListenerImpl(*this, socket, cb, bind_to_port, + enable_tcp_fast_open, hand_off_restored_destination_connections)}; } diff --git a/source/common/event/dispatcher_impl.h b/source/common/event/dispatcher_impl.h index 95d52938da442..3d10ad787e10b 100644 --- a/source/common/event/dispatcher_impl.h +++ b/source/common/event/dispatcher_impl.h @@ -47,7 +47,7 @@ class DispatcherImpl : Logger::Loggable, public Dispatcher { uint32_t events) override; Filesystem::WatcherPtr createFilesystemWatcher() override; Network::ListenerPtr createListener(Network::Socket& socket, Network::ListenerCallbacks& cb, - bool bind_to_port, + bool bind_to_port, bool enable_tcp_fast_open, bool hand_off_restored_destination_connections) override; TimerPtr createTimer(TimerCb cb) override; void deferredDelete(DeferredDeletablePtr&& to_delete) override; diff --git a/source/common/network/listener_impl.cc b/source/common/network/listener_impl.cc index da70a74fffce5..539e1f063eee9 100644 --- a/source/common/network/listener_impl.cc +++ b/source/common/network/listener_impl.cc @@ -1,5 +1,6 @@ #include "common/network/listener_impl.h" +#include #include #include "envoy/common/exception.h" @@ -13,6 +14,16 @@ #include "event2/listener.h" +// On macOS the socket MUST be listening already for TCP_FASTOPEN to be set and backlog MUST be 1 +// (the actual value is set via the net.inet.tcp.fastopen_backlog kernel parameter. +// For Linux we default to 128, which libevent is using in +// https://github.com/libevent/libevent/blob/release-2.1.8-stable/listener.c#L176 +#if defined(__APPLE__) +#define TFO_BACKLOG 1 +#else +#define TFO_BACKLOG 128 +#endif + namespace Envoy { namespace Network { @@ -45,7 +56,8 @@ void ListenerImpl::listenCallback(evconnlistener*, evutil_socket_t fd, sockaddr* } ListenerImpl::ListenerImpl(Event::DispatcherImpl& dispatcher, Socket& socket, ListenerCallbacks& cb, - bool bind_to_port, bool hand_off_restored_destination_connections) + bool bind_to_port, bool enable_tcp_fast_open, + bool hand_off_restored_destination_connections) : local_address_(nullptr), cb_(cb), hand_off_restored_destination_connections_(hand_off_restored_destination_connections), listener_(nullptr) { @@ -66,6 +78,48 @@ ListenerImpl::ListenerImpl(Event::DispatcherImpl& dispatcher, Socket& socket, Li fmt::format("cannot listen on socket: {}", socket.localAddress()->asString())); } + // TODO: cleanup redundant setsockopt logic + if (enable_tcp_fast_open) { + int backlog = TFO_BACKLOG; + int fd = socket.fd(); + int rc = setsockopt(fd, IPPROTO_TCP, TCP_FASTOPEN, &backlog, sizeof(backlog)); + if (rc == -1) { + if (errno == ENOPROTOOPT) { + throw EnvoyException(fmt::format( + "TCP Fast Open is unsupported on listening socket fd {} : {}", fd, strerror(errno))); + } else { + throw EnvoyException( + fmt::format("Failed to enable TCP Fast Open on listening socket fd {} : {}", fd, + strerror(errno))); + } + } else { + ENVOY_LOG_MISC(debug, "Enabled TFO on listening socket fd {}.", fd); + } + } else { + int curbacklog; + int fd = socket.fd(); + socklen_t optlen = sizeof(curbacklog); + int rc = getsockopt(fd, IPPROTO_TCP, TCP_FASTOPEN, &curbacklog, &optlen); + // curbacklog == 0 means TFO is supported and already disabled + if (rc != -1 && curbacklog != 0) { + int backlog = 0; + int rc = setsockopt(fd, IPPROTO_TCP, TCP_FASTOPEN, &backlog, sizeof(backlog)); + if (rc == -1) { + if (errno == ENOPROTOOPT) { + throw EnvoyException( + fmt::format("TCP Fast Open is unsupported on listening socket fd {} : {}", fd, + strerror(errno))); + } else { + throw EnvoyException( + fmt::format("Failed to disable TCP Fast Open on listening socket fd {} : {}", fd, + strerror(errno))); + } + } else { + ENVOY_LOG_MISC(debug, "Disabled TFO on listening socket fd {}.", fd); + } + } + } + evconnlistener_set_error_cb(listener_.get(), errorCallback); } } diff --git a/source/common/network/listener_impl.h b/source/common/network/listener_impl.h index d55ccfc623bd3..555b9a9f8b5a2 100644 --- a/source/common/network/listener_impl.h +++ b/source/common/network/listener_impl.h @@ -17,7 +17,8 @@ namespace Network { class ListenerImpl : public Listener { public: ListenerImpl(Event::DispatcherImpl& dispatcher, Socket& socket, ListenerCallbacks& cb, - bool bind_to_port, bool hand_off_restored_destination_connections); + bool bind_to_port, bool enable_tcp_fast_open, + bool hand_off_restored_destination_connections); protected: virtual Address::InstanceConstSharedPtr getLocalAddress(int fd); diff --git a/source/server/config_validation/dispatcher.cc b/source/server/config_validation/dispatcher.cc index 7966f746e1cb9..06855c4ea2e1e 100644 --- a/source/server/config_validation/dispatcher.cc +++ b/source/server/config_validation/dispatcher.cc @@ -17,7 +17,8 @@ Network::DnsResolverSharedPtr ValidationDispatcher::createDnsResolver( } Network::ListenerPtr ValidationDispatcher::createListener(Network::Socket&, - Network::ListenerCallbacks&, bool, bool) { + Network::ListenerCallbacks&, bool, bool, + bool) { NOT_IMPLEMENTED; } diff --git a/source/server/config_validation/dispatcher.h b/source/server/config_validation/dispatcher.h index 54fad3c12943f..aef51eb9af57c 100644 --- a/source/server/config_validation/dispatcher.h +++ b/source/server/config_validation/dispatcher.h @@ -21,7 +21,7 @@ class ValidationDispatcher : public DispatcherImpl { Network::DnsResolverSharedPtr createDnsResolver( const std::vector& resolvers) override; Network::ListenerPtr createListener(Network::Socket&, Network::ListenerCallbacks&, - bool bind_to_port, + bool bind_to_port, bool enable_tcp_fast_open, bool hand_off_restored_destination_connections) override; }; diff --git a/source/server/connection_handler_impl.cc b/source/server/connection_handler_impl.cc index 89d807e966418..70524554b97c4 100644 --- a/source/server/connection_handler_impl.cc +++ b/source/server/connection_handler_impl.cc @@ -60,11 +60,11 @@ void ConnectionHandlerImpl::ActiveListener::removeConnection(ActiveConnection& c ConnectionHandlerImpl::ActiveListener::ActiveListener(ConnectionHandlerImpl& parent, Network::ListenerConfig& config) - : ActiveListener( - parent, - parent.dispatcher_.createListener(config.socket(), *this, config.bindToPort(), - config.handOffRestoredDestinationConnections()), - config) {} + : ActiveListener(parent, + parent.dispatcher_.createListener( + config.socket(), *this, config.bindToPort(), config.enableTcpFastOpen(), + config.handOffRestoredDestinationConnections()), + config) {} ConnectionHandlerImpl::ActiveListener::ActiveListener(ConnectionHandlerImpl& parent, Network::ListenerPtr&& listener, diff --git a/source/server/http/admin.h b/source/server/http/admin.h index d3fd7c3d19fdc..2655924a71e18 100644 --- a/source/server/http/admin.h +++ b/source/server/http/admin.h @@ -181,6 +181,7 @@ class AdminImpl : public Admin, return parent_.transport_socket_factory_; } bool bindToPort() override { return true; } + bool enableTcpFastOpen() override { return false; } bool handOffRestoredDestinationConnections() const override { return false; } uint32_t perConnectionBufferLimitBytes() override { return 0; } Stats::Scope& listenerScope() override { return *scope_; } diff --git a/source/server/listener_manager_impl.cc b/source/server/listener_manager_impl.cc index 31f9e283d155b..1f7a2d726204a 100644 --- a/source/server/listener_manager_impl.cc +++ b/source/server/listener_manager_impl.cc @@ -112,6 +112,7 @@ ListenerImpl::ListenerImpl(const envoy::api::v2::Listener& config, ListenerManag listener_scope_( parent_.server_.stats().createScope(fmt::format("listener.{}.", address_->asString()))), bind_to_port_(PROTOBUF_GET_WRAPPED_OR_DEFAULT(config.deprecated_v1(), bind_to_port, true)), + enable_tcp_fast_open_(config.enable_tcp_fast_open()), hand_off_restored_destination_connections_( PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, use_original_dst, false)), per_connection_buffer_limit_bytes_( diff --git a/source/server/listener_manager_impl.h b/source/server/listener_manager_impl.h index 3b649edd6ea3a..c79428917a4e1 100644 --- a/source/server/listener_manager_impl.h +++ b/source/server/listener_manager_impl.h @@ -48,7 +48,6 @@ class ProdListenerComponentFactory : public ListenerComponentFactory, Configuration::ListenerFactoryContext& context) override { return createListenerFilterFactoryList_(filters, context); } - Network::SocketSharedPtr createListenSocket(Network::Address::InstanceConstSharedPtr address, bool bind_to_port) override; DrainManagerPtr createDrainManager(envoy::api::v2::Listener::DrainType drain_type) override; @@ -213,6 +212,7 @@ class ListenerImpl : public Network::ListenerConfig, Network::FilterChainFactory& filterChainFactory() override { return *this; } Network::Socket& socket() override { return *socket_; } bool bindToPort() override { return bind_to_port_; } + bool enableTcpFastOpen() override { return enable_tcp_fast_open_; } bool handOffRestoredDestinationConnections() const override { return hand_off_restored_destination_connections_; } @@ -270,6 +270,7 @@ class ListenerImpl : public Network::ListenerConfig, std::vector tls_contexts_; std::vector transport_socket_factories_; const bool bind_to_port_; + const bool enable_tcp_fast_open_; const bool hand_off_restored_destination_connections_; const uint32_t per_connection_buffer_limit_bytes_; const uint64_t listener_tag_; diff --git a/test/common/http/codec_client_test.cc b/test/common/http/codec_client_test.cc index 1aa0dc5895728..3a376880f3cd7 100644 --- a/test/common/http/codec_client_test.cc +++ b/test/common/http/codec_client_test.cc @@ -179,7 +179,8 @@ class CodecNetworkTest : public testing::TestWithParamcreateListener(socket_, listener_callbacks_, true, false); + upstream_listener_ = + dispatcher_->createListener(socket_, listener_callbacks_, true, false, false); Network::ClientConnectionPtr client_connection = dispatcher_->createClientConnection( socket_.localAddress(), source_address_, Network::Test::createRawBufferSocket(), nullptr); client_connection_ = client_connection.get(); diff --git a/test/common/network/connection_impl_test.cc b/test/common/network/connection_impl_test.cc index 1fdea7b15c795..ac1bedd1c27b8 100644 --- a/test/common/network/connection_impl_test.cc +++ b/test/common/network/connection_impl_test.cc @@ -82,7 +82,7 @@ class ConnectionImplTest : public testing::TestWithParam { if (dispatcher_.get() == nullptr) { dispatcher_.reset(new Event::DispatcherImpl); } - listener_ = dispatcher_->createListener(socket_, listener_callbacks_, true, false); + listener_ = dispatcher_->createListener(socket_, listener_callbacks_, true, false, false); client_connection_ = dispatcher_->createClientConnection( socket_.localAddress(), source_address_, Network::Test::createRawBufferSocket(), nullptr); @@ -765,7 +765,7 @@ TEST_P(ConnectionImplTest, BindFailureTest) { new Network::Address::Ipv6Instance(address_string, 0)}; } dispatcher_.reset(new Event::DispatcherImpl); - listener_ = dispatcher_->createListener(socket_, listener_callbacks_, true, false); + listener_ = dispatcher_->createListener(socket_, listener_callbacks_, true, false, false); client_connection_ = dispatcher_->createClientConnection( socket_.localAddress(), source_address_, Network::Test::createRawBufferSocket(), nullptr); @@ -1159,7 +1159,7 @@ class ReadBufferLimitTest : public ConnectionImplTest { void readBufferLimitTest(uint32_t read_buffer_limit, uint32_t expected_chunk_size) { const uint32_t buffer_size = 256 * 1024; dispatcher_.reset(new Event::DispatcherImpl); - listener_ = dispatcher_->createListener(socket_, listener_callbacks_, true, false); + listener_ = dispatcher_->createListener(socket_, listener_callbacks_, true, false, false); client_connection_ = dispatcher_->createClientConnection( socket_.localAddress(), Network::Address::InstanceConstSharedPtr(), diff --git a/test/common/network/dns_impl_test.cc b/test/common/network/dns_impl_test.cc index fe0cf9ad2cc36..d8a42f6b58337 100644 --- a/test/common/network/dns_impl_test.cc +++ b/test/common/network/dns_impl_test.cc @@ -350,7 +350,7 @@ class DnsImplTest : public testing::TestWithParam { server_.reset(new TestDnsServer(dispatcher_)); socket_.reset( new Network::TcpListenSocket(Network::Test::getCanonicalLoopbackAddress(GetParam()), true)); - listener_ = dispatcher_.createListener(*socket_, *server_, true, false); + listener_ = dispatcher_.createListener(*socket_, *server_, true, false, false); // Point c-ares at the listener with no search domains and TCP-only. peer_.reset(new DnsResolverImplPeer(dynamic_cast(resolver_.get()))); diff --git a/test/common/network/listener_impl_test.cc b/test/common/network/listener_impl_test.cc index 2b01c6186abfa..ad2cd5a052b9b 100644 --- a/test/common/network/listener_impl_test.cc +++ b/test/common/network/listener_impl_test.cc @@ -28,7 +28,7 @@ static void errorCallbackTest(Address::IpVersion version) { Network::MockListenerCallbacks listener_callbacks; Network::MockConnectionHandler connection_handler; Network::ListenerPtr listener = - dispatcher.createListener(socket, listener_callbacks, true, false); + dispatcher.createListener(socket, listener_callbacks, true, false, false); Network::ClientConnectionPtr client_connection = dispatcher.createClientConnection( socket.localAddress(), Network::Address::InstanceConstSharedPtr(), @@ -62,8 +62,9 @@ TEST_P(ListenerImplDeathTest, ErrorCallback) { class TestListenerImpl : public ListenerImpl { public: TestListenerImpl(Event::DispatcherImpl& dispatcher, Socket& socket, ListenerCallbacks& cb, - bool bind_to_port, bool hand_off_restored_destination_connections) - : ListenerImpl(dispatcher, socket, cb, bind_to_port, + bool bind_to_port, bool enable_tcp_fast_open, + bool hand_off_restored_destination_connections) + : ListenerImpl(dispatcher, socket, cb, bind_to_port, enable_tcp_fast_open, hand_off_restored_destination_connections) {} MOCK_METHOD1(getLocalAddress, Address::InstanceConstSharedPtr(int fd)); @@ -90,9 +91,10 @@ TEST_P(ListenerImplTest, UseActualDst) { Network::MockListenerCallbacks listener_callbacks1; Network::MockConnectionHandler connection_handler; // Do not redirect since use_original_dst is false. - Network::TestListenerImpl listener(dispatcher, socket, listener_callbacks1, true, true); + Network::TestListenerImpl listener(dispatcher, socket, listener_callbacks1, true, false, true); Network::MockListenerCallbacks listener_callbacks2; - Network::TestListenerImpl listenerDst(dispatcher, socketDst, listener_callbacks2, false, false); + Network::TestListenerImpl listenerDst(dispatcher, socketDst, listener_callbacks2, false, false, + false); Network::ClientConnectionPtr client_connection = dispatcher.createClientConnection( socket.localAddress(), Network::Address::InstanceConstSharedPtr(), @@ -126,7 +128,7 @@ TEST_P(ListenerImplTest, WildcardListenerUseActualDst) { Network::MockListenerCallbacks listener_callbacks; Network::MockConnectionHandler connection_handler; // Do not redirect since use_original_dst is false. - Network::TestListenerImpl listener(dispatcher, socket, listener_callbacks, true, true); + Network::TestListenerImpl listener(dispatcher, socket, listener_callbacks, true, false, true); auto local_dst_address = Network::Utility::getAddressWithPort( *Network::Test::getCanonicalLoopbackAddress(version_), socket.localAddress()->ip()->port()); @@ -168,7 +170,7 @@ TEST_P(ListenerImplTest, WildcardListenerIpv4Compat) { ASSERT_TRUE(socket.localAddress()->ip()->isAnyAddress()); // Do not redirect since use_original_dst is false. - Network::TestListenerImpl listener(dispatcher, socket, listener_callbacks, true, true); + Network::TestListenerImpl listener(dispatcher, socket, listener_callbacks, true, false, true); auto listener_address = Network::Utility::getAddressWithPort( *Network::Test::getCanonicalLoopbackAddress(version_), socket.localAddress()->ip()->port()); diff --git a/test/common/network/proxy_protocol_test.cc b/test/common/network/proxy_protocol_test.cc index d0b179a48c188..d235e0c106dae 100644 --- a/test/common/network/proxy_protocol_test.cc +++ b/test/common/network/proxy_protocol_test.cc @@ -56,6 +56,7 @@ class ProxyProtocolTest : public testing::TestWithParam, return transport_socket_factory_; } bool bindToPort() override { return true; } + bool enableTcpFastOpen() override { return false; } bool handOffRestoredDestinationConnections() const override { return false; } uint32_t perConnectionBufferLimitBytes() override { return 0; } Stats::Scope& listenerScope() override { return stats_store_; } @@ -348,6 +349,7 @@ class WildcardProxyProtocolTest : public testing::TestWithParam callbacks; Network::MockConnectionHandler connection_handler; - Network::ListenerPtr listener = dispatcher.createListener(socket, callbacks, true, false); + Network::ListenerPtr listener = dispatcher.createListener(socket, callbacks, true, false, false); ClientContextConfigImpl client_ctx_config(client_ctx_proto); ClientSslSocketFactory client_ssl_socket_factory(client_ctx_config, manager, stats_store); @@ -738,7 +738,7 @@ TEST_P(SslSocketTest, FlushCloseDuringHandshake) { Network::TcpListenSocket socket(Network::Test::getCanonicalLoopbackAddress(GetParam()), true); Network::MockListenerCallbacks callbacks; Network::MockConnectionHandler connection_handler; - Network::ListenerPtr listener = dispatcher.createListener(socket, callbacks, true, false); + Network::ListenerPtr listener = dispatcher.createListener(socket, callbacks, true, false, false); Network::ClientConnectionPtr client_connection = dispatcher.createClientConnection( socket.localAddress(), Network::Address::InstanceConstSharedPtr(), @@ -796,7 +796,7 @@ TEST_P(SslSocketTest, HalfClose) { Network::MockListenerCallbacks listener_callbacks; Network::MockConnectionHandler connection_handler; Network::ListenerPtr listener = - dispatcher.createListener(socket, listener_callbacks, true, false); + dispatcher.createListener(socket, listener_callbacks, true, false, false); std::shared_ptr server_read_filter(new Network::MockReadFilter()); std::shared_ptr client_read_filter(new Network::MockReadFilter()); @@ -877,7 +877,7 @@ TEST_P(SslSocketTest, ClientAuthMultipleCAs) { Network::TcpListenSocket socket(Network::Test::getCanonicalLoopbackAddress(GetParam()), true); Network::MockListenerCallbacks callbacks; Network::MockConnectionHandler connection_handler; - Network::ListenerPtr listener = dispatcher.createListener(socket, callbacks, true, false); + Network::ListenerPtr listener = dispatcher.createListener(socket, callbacks, true, false, false); std::string client_ctx_json = R"EOF( { @@ -958,8 +958,10 @@ void testTicketSessionResumption(const std::string& server_ctx_json1, Network::TcpListenSocket socket2(Network::Test::getCanonicalLoopbackAddress(ip_version), true); NiceMock callbacks; Network::MockConnectionHandler connection_handler; - Network::ListenerPtr listener1 = dispatcher.createListener(socket1, callbacks, true, false); - Network::ListenerPtr listener2 = dispatcher.createListener(socket2, callbacks, true, false); + Network::ListenerPtr listener1 = + dispatcher.createListener(socket1, callbacks, true, false, false); + Network::ListenerPtr listener2 = + dispatcher.createListener(socket2, callbacks, true, false, false); Json::ObjectSharedPtr client_ctx_loader = TestEnvironment::jsonLoadFromString(client_ctx_json); ClientContextConfigImpl client_ctx_config(*client_ctx_loader); @@ -1280,8 +1282,9 @@ TEST_P(SslSocketTest, ClientAuthCrossListenerSessionResumption) { Network::TcpListenSocket socket2(Network::Test::getCanonicalLoopbackAddress(GetParam()), true); Network::MockListenerCallbacks callbacks; Network::MockConnectionHandler connection_handler; - Network::ListenerPtr listener = dispatcher.createListener(socket, callbacks, true, false); - Network::ListenerPtr listener2 = dispatcher.createListener(socket2, callbacks, true, false); + Network::ListenerPtr listener = dispatcher.createListener(socket, callbacks, true, false, false); + Network::ListenerPtr listener2 = + dispatcher.createListener(socket2, callbacks, true, false, false); std::string client_ctx_json = R"EOF( { @@ -1386,7 +1389,7 @@ TEST_P(SslSocketTest, SslError) { Network::TcpListenSocket socket(Network::Test::getCanonicalLoopbackAddress(GetParam()), true); Network::MockListenerCallbacks callbacks; Network::MockConnectionHandler connection_handler; - Network::ListenerPtr listener = dispatcher.createListener(socket, callbacks, true, false); + Network::ListenerPtr listener = dispatcher.createListener(socket, callbacks, true, false, false); Network::ClientConnectionPtr client_connection = dispatcher.createClientConnection( socket.localAddress(), Network::Address::InstanceConstSharedPtr(), @@ -2021,7 +2024,7 @@ class SslReadBufferLimitTest : public SslCertsTest, server_ssl_socket_factory_.reset( new ServerSslSocketFactory(*server_ctx_config_, "", {}, true, *manager_, stats_store_)); - listener_ = dispatcher_->createListener(socket_, listener_callbacks_, true, false); + listener_ = dispatcher_->createListener(socket_, listener_callbacks_, true, false, false); client_ctx_loader_ = TestEnvironment::jsonLoadFromString(client_ctx_json_); client_ctx_config_.reset(new ClientContextConfigImpl(*client_ctx_loader_)); diff --git a/test/integration/fake_upstream.h b/test/integration/fake_upstream.h index bf9e59be4f992..4b260a53ed463 100644 --- a/test/integration/fake_upstream.h +++ b/test/integration/fake_upstream.h @@ -331,6 +331,7 @@ class FakeUpstream : Logger::Loggable, public Network::Filt return *parent_.transport_socket_factory_; } bool bindToPort() override { return true; } + bool enableTcpFastOpen() override { return false; } bool handOffRestoredDestinationConnections() const override { return false; } uint32_t perConnectionBufferLimitBytes() override { return 0; } Stats::Scope& listenerScope() override { return parent_.stats_store_; } diff --git a/test/mocks/event/mocks.h b/test/mocks/event/mocks.h index bf21591b80bab..809ff775e86a0 100644 --- a/test/mocks/event/mocks.h +++ b/test/mocks/event/mocks.h @@ -54,10 +54,10 @@ class MockDispatcher : public Dispatcher { } Network::ListenerPtr createListener(Network::Socket& socket, Network::ListenerCallbacks& cb, - bool bind_to_port, + bool bind_to_port, bool enable_tcp_fast_open, bool hand_off_restored_destination_connections) override { - return Network::ListenerPtr{ - createListener_(socket, cb, bind_to_port, hand_off_restored_destination_connections)}; + return Network::ListenerPtr{createListener_(socket, cb, bind_to_port, enable_tcp_fast_open, + hand_off_restored_destination_connections)}; } TimerPtr createTimer(TimerCb cb) override { return TimerPtr{createTimer_(cb)}; } @@ -90,9 +90,9 @@ class MockDispatcher : public Dispatcher { MOCK_METHOD4(createFileEvent_, FileEvent*(int fd, FileReadyCb cb, FileTriggerType trigger, uint32_t events)); MOCK_METHOD0(createFilesystemWatcher_, Filesystem::Watcher*()); - MOCK_METHOD4(createListener_, + MOCK_METHOD5(createListener_, Network::Listener*(Network::Socket& socket, Network::ListenerCallbacks& cb, - bool bind_to_port, + bool bind_to_port, bool enable_tcp_fast_open, bool hand_off_restored_destination_connections)); MOCK_METHOD1(createTimer_, Timer*(TimerCb cb)); MOCK_METHOD1(deferredDelete_, void(DeferredDeletablePtr& to_delete)); diff --git a/test/mocks/network/mocks.h b/test/mocks/network/mocks.h index a8a861112d891..446a0df994aad 100644 --- a/test/mocks/network/mocks.h +++ b/test/mocks/network/mocks.h @@ -304,6 +304,7 @@ class MockListenerConfig : public ListenerConfig { MOCK_METHOD0(socket, Socket&()); MOCK_METHOD0(transportSocketFactory, TransportSocketFactory&()); MOCK_METHOD0(bindToPort, bool()); + MOCK_METHOD0(enableTcpFastOpen, bool()); MOCK_CONST_METHOD0(handOffRestoredDestinationConnections, bool()); MOCK_METHOD0(perConnectionBufferLimitBytes, uint32_t()); MOCK_METHOD0(listenerScope, Stats::Scope&()); diff --git a/test/server/connection_handler_test.cc b/test/server/connection_handler_test.cc index e9b95e6c01e57..a5861df0e5928 100644 --- a/test/server/connection_handler_test.cc +++ b/test/server/connection_handler_test.cc @@ -30,8 +30,10 @@ class ConnectionHandlerTest : public testing::Test, protected Logger::Loggable { public: TestListener(ConnectionHandlerTest& parent, uint64_t tag, bool bind_to_port, - bool hand_off_restored_destination_connections, const std::string& name) + bool enable_tcp_fast_open, bool hand_off_restored_destination_connections, + const std::string& name) : parent_(parent), tag_(tag), bind_to_port_(bind_to_port), + enable_tcp_fast_open_(enable_tcp_fast_open), hand_off_restored_destination_connections_(hand_off_restored_destination_connections), name_(name) {} @@ -41,6 +43,7 @@ class ConnectionHandlerTest : public testing::Test, protected Logger::Loggable TestListenerPtr; - TestListener* addListener(uint64_t tag, bool bind_to_port, + TestListener* addListener(uint64_t tag, bool bind_to_port, bool enable_tcp_fast_open, bool hand_off_restored_destination_connections, const std::string& name) { - TestListener* listener = - new TestListener(*this, tag, bind_to_port, hand_off_restored_destination_connections, name); + TestListener* listener = new TestListener(*this, tag, bind_to_port, enable_tcp_fast_open, + hand_off_restored_destination_connections, name); listener->moveIntoListBack(TestListenerPtr{listener}, listeners_); return listener; } @@ -81,14 +85,14 @@ TEST_F(ConnectionHandlerTest, RemoveListener) { Network::MockListener* listener = new NiceMock(); Network::ListenerCallbacks* listener_callbacks; - EXPECT_CALL(dispatcher_, createListener_(_, _, _, false)) - .WillOnce(Invoke( - [&](Network::Socket&, Network::ListenerCallbacks& cb, bool, bool) -> Network::Listener* { - listener_callbacks = &cb; - return listener; - - })); - TestListener* test_listener = addListener(1, true, false, "test_listener"); + EXPECT_CALL(dispatcher_, createListener_(_, _, _, _, false)) + .WillOnce(Invoke([&](Network::Socket&, Network::ListenerCallbacks& cb, bool, bool, + bool) -> Network::Listener* { + listener_callbacks = &cb; + return listener; + + })); + TestListener* test_listener = addListener(1, true, false, false, "test_listener"); EXPECT_CALL(test_listener->socket_, localAddress()); handler_->addListener(*test_listener); @@ -119,14 +123,14 @@ TEST_F(ConnectionHandlerTest, DestroyCloseConnections) { Network::MockListener* listener = new NiceMock(); Network::ListenerCallbacks* listener_callbacks; - EXPECT_CALL(dispatcher_, createListener_(_, _, _, _)) - .WillOnce(Invoke( - [&](Network::Socket&, Network::ListenerCallbacks& cb, bool, bool) -> Network::Listener* { - listener_callbacks = &cb; - return listener; - - })); - TestListener* test_listener = addListener(1, true, false, "test_listener"); + EXPECT_CALL(dispatcher_, createListener_(_, _, _, _, _)) + .WillOnce(Invoke([&](Network::Socket&, Network::ListenerCallbacks& cb, bool, bool, + bool) -> Network::Listener* { + listener_callbacks = &cb; + return listener; + + })); + TestListener* test_listener = addListener(1, true, false, false, "test_listener"); EXPECT_CALL(test_listener->socket_, localAddress()); handler_->addListener(*test_listener); @@ -146,14 +150,14 @@ TEST_F(ConnectionHandlerTest, CloseDuringFilterChainCreate) { Network::MockListener* listener = new Network::MockListener(); Network::ListenerCallbacks* listener_callbacks; - EXPECT_CALL(dispatcher_, createListener_(_, _, _, _)) - .WillOnce(Invoke( - [&](Network::Socket&, Network::ListenerCallbacks& cb, bool, bool) -> Network::Listener* { - listener_callbacks = &cb; - return listener; - - })); - TestListener* test_listener = addListener(1, true, false, "test_listener"); + EXPECT_CALL(dispatcher_, createListener_(_, _, _, _, _)) + .WillOnce(Invoke([&](Network::Socket&, Network::ListenerCallbacks& cb, bool, bool, + bool) -> Network::Listener* { + listener_callbacks = &cb; + return listener; + + })); + TestListener* test_listener = addListener(1, true, false, false, "test_listener"); EXPECT_CALL(test_listener->socket_, localAddress()); handler_->addListener(*test_listener); @@ -172,14 +176,14 @@ TEST_F(ConnectionHandlerTest, CloseConnectionOnEmptyFilterChain) { Network::MockListener* listener = new Network::MockListener(); Network::ListenerCallbacks* listener_callbacks; - EXPECT_CALL(dispatcher_, createListener_(_, _, _, _)) - .WillOnce(Invoke( - [&](Network::Socket&, Network::ListenerCallbacks& cb, bool, bool) -> Network::Listener* { - listener_callbacks = &cb; - return listener; - - })); - TestListener* test_listener = addListener(1, true, false, "test_listener"); + EXPECT_CALL(dispatcher_, createListener_(_, _, _, _, _)) + .WillOnce(Invoke([&](Network::Socket&, Network::ListenerCallbacks& cb, bool, bool, + bool) -> Network::Listener* { + listener_callbacks = &cb; + return listener; + + })); + TestListener* test_listener = addListener(1, true, false, false, "test_listener"); EXPECT_CALL(test_listener->socket_, localAddress()); handler_->addListener(*test_listener); @@ -193,28 +197,28 @@ TEST_F(ConnectionHandlerTest, CloseConnectionOnEmptyFilterChain) { } TEST_F(ConnectionHandlerTest, FindListenerByAddress) { - TestListener* test_listener1 = addListener(1, true, true, "test_listener1"); + TestListener* test_listener1 = addListener(1, true, false, true, "test_listener1"); Network::Address::InstanceConstSharedPtr alt_address( new Network::Address::Ipv4Instance("127.0.0.1", 10001)); Network::MockListener* listener = new Network::MockListener(); - EXPECT_CALL(dispatcher_, createListener_(_, _, _, true)) - .WillOnce(Invoke([&](Network::Socket&, Network::ListenerCallbacks&, bool, + EXPECT_CALL(dispatcher_, createListener_(_, _, _, _, true)) + .WillOnce(Invoke([&](Network::Socket&, Network::ListenerCallbacks&, bool, bool, bool) -> Network::Listener* { return listener; })); EXPECT_CALL(test_listener1->socket_, localAddress()).WillRepeatedly(ReturnRef(alt_address)); handler_->addListener(*test_listener1); EXPECT_EQ(listener, handler_->findListenerByAddress(ByRef(*alt_address))); - TestListener* test_listener2 = addListener(2, true, false, "test_listener2"); + TestListener* test_listener2 = addListener(2, true, false, false, "test_listener2"); Network::Address::InstanceConstSharedPtr alt_address2( new Network::Address::Ipv4Instance("0.0.0.0", 10001)); Network::Address::InstanceConstSharedPtr alt_address3( new Network::Address::Ipv4Instance("127.0.0.2", 10001)); Network::MockListener* listener2 = new Network::MockListener(); - EXPECT_CALL(dispatcher_, createListener_(_, _, _, false)) - .WillOnce(Invoke([&](Network::Socket&, Network::ListenerCallbacks&, bool, + EXPECT_CALL(dispatcher_, createListener_(_, _, _, _, false)) + .WillOnce(Invoke([&](Network::Socket&, Network::ListenerCallbacks&, bool, bool, bool) -> Network::Listener* { return listener2; })); EXPECT_CALL(test_listener2->socket_, localAddress()).WillRepeatedly(ReturnRef(alt_address2)); handler_->addListener(*test_listener2); @@ -231,8 +235,8 @@ TEST_F(ConnectionHandlerTest, FindListenerByAddress) { handler_->stopListeners(2); Network::MockListener* listener3 = new Network::MockListener(); - EXPECT_CALL(dispatcher_, createListener_(_, _, _, _)) - .WillOnce(Invoke([&](Network::Socket&, Network::ListenerCallbacks&, bool, + EXPECT_CALL(dispatcher_, createListener_(_, _, _, _, _)) + .WillOnce(Invoke([&](Network::Socket&, Network::ListenerCallbacks&, bool, bool, bool) -> Network::Listener* { return listener3; })); handler_->addListener(*test_listener2); @@ -243,29 +247,29 @@ TEST_F(ConnectionHandlerTest, FindListenerByAddress) { } TEST_F(ConnectionHandlerTest, NormalRedirect) { - TestListener* test_listener1 = addListener(1, true, true, "test_listener1"); + TestListener* test_listener1 = addListener(1, true, false, true, "test_listener1"); Network::MockListener* listener1 = new Network::MockListener(); Network::ListenerCallbacks* listener_callbacks1; - EXPECT_CALL(dispatcher_, createListener_(_, _, _, true)) - .WillOnce(Invoke( - [&](Network::Socket&, Network::ListenerCallbacks& cb, bool, bool) -> Network::Listener* { - listener_callbacks1 = &cb; - return listener1; - })); + EXPECT_CALL(dispatcher_, createListener_(_, _, _, _, true)) + .WillOnce(Invoke([&](Network::Socket&, Network::ListenerCallbacks& cb, bool, bool, + bool) -> Network::Listener* { + listener_callbacks1 = &cb; + return listener1; + })); Network::Address::InstanceConstSharedPtr normal_address( new Network::Address::Ipv4Instance("127.0.0.1", 10001)); EXPECT_CALL(test_listener1->socket_, localAddress()).WillRepeatedly(ReturnRef(normal_address)); handler_->addListener(*test_listener1); - TestListener* test_listener2 = addListener(1, false, false, "test_listener2"); + TestListener* test_listener2 = addListener(1, false, false, false, "test_listener2"); Network::MockListener* listener2 = new Network::MockListener(); Network::ListenerCallbacks* listener_callbacks2; - EXPECT_CALL(dispatcher_, createListener_(_, _, _, false)) - .WillOnce(Invoke( - [&](Network::Socket&, Network::ListenerCallbacks& cb, bool, bool) -> Network::Listener* { - listener_callbacks2 = &cb; - return listener2; - })); + EXPECT_CALL(dispatcher_, createListener_(_, _, _, _, false)) + .WillOnce(Invoke([&](Network::Socket&, Network::ListenerCallbacks& cb, bool, bool, + bool) -> Network::Listener* { + listener_callbacks2 = &cb; + return listener2; + })); Network::Address::InstanceConstSharedPtr alt_address( new Network::Address::Ipv4Instance("127.0.0.2", 20002)); EXPECT_CALL(test_listener2->socket_, localAddress()).WillRepeatedly(ReturnRef(alt_address)); @@ -302,29 +306,29 @@ TEST_F(ConnectionHandlerTest, NormalRedirect) { } TEST_F(ConnectionHandlerTest, FallbackToWildcardListener) { - TestListener* test_listener1 = addListener(1, true, true, "test_listener1"); + TestListener* test_listener1 = addListener(1, true, false, true, "test_listener1"); Network::MockListener* listener1 = new Network::MockListener(); Network::ListenerCallbacks* listener_callbacks1; - EXPECT_CALL(dispatcher_, createListener_(_, _, _, true)) - .WillOnce(Invoke( - [&](Network::Socket&, Network::ListenerCallbacks& cb, bool, bool) -> Network::Listener* { - listener_callbacks1 = &cb; - return listener1; - })); + EXPECT_CALL(dispatcher_, createListener_(_, _, _, _, true)) + .WillOnce(Invoke([&](Network::Socket&, Network::ListenerCallbacks& cb, bool, bool, + bool) -> Network::Listener* { + listener_callbacks1 = &cb; + return listener1; + })); Network::Address::InstanceConstSharedPtr normal_address( new Network::Address::Ipv4Instance("127.0.0.1", 10001)); EXPECT_CALL(test_listener1->socket_, localAddress()).WillRepeatedly(ReturnRef(normal_address)); handler_->addListener(*test_listener1); - TestListener* test_listener2 = addListener(1, false, false, "test_listener2"); + TestListener* test_listener2 = addListener(1, false, false, false, "test_listener2"); Network::MockListener* listener2 = new Network::MockListener(); Network::ListenerCallbacks* listener_callbacks2; - EXPECT_CALL(dispatcher_, createListener_(_, _, _, false)) - .WillOnce(Invoke( - [&](Network::Socket&, Network::ListenerCallbacks& cb, bool, bool) -> Network::Listener* { - listener_callbacks2 = &cb; - return listener2; - })); + EXPECT_CALL(dispatcher_, createListener_(_, _, _, _, false)) + .WillOnce(Invoke([&](Network::Socket&, Network::ListenerCallbacks& cb, bool, bool, + bool) -> Network::Listener* { + listener_callbacks2 = &cb; + return listener2; + })); Network::Address::InstanceConstSharedPtr any_address = Network::Utility::getIpv4AnyAddress(); EXPECT_CALL(test_listener2->socket_, localAddress()).WillRepeatedly(ReturnRef(any_address)); handler_->addListener(*test_listener2); @@ -363,15 +367,15 @@ TEST_F(ConnectionHandlerTest, FallbackToWildcardListener) { } TEST_F(ConnectionHandlerTest, WildcardListenerWithOriginalDst) { - TestListener* test_listener1 = addListener(1, true, true, "test_listener1"); + TestListener* test_listener1 = addListener(1, true, false, true, "test_listener1"); Network::MockListener* listener1 = new Network::MockListener(); Network::ListenerCallbacks* listener_callbacks1; - EXPECT_CALL(dispatcher_, createListener_(_, _, _, true)) - .WillOnce(Invoke( - [&](Network::Socket&, Network::ListenerCallbacks& cb, bool, bool) -> Network::Listener* { - listener_callbacks1 = &cb; - return listener1; - })); + EXPECT_CALL(dispatcher_, createListener_(_, _, _, _, true)) + .WillOnce(Invoke([&](Network::Socket&, Network::ListenerCallbacks& cb, bool, bool, + bool) -> Network::Listener* { + listener_callbacks1 = &cb; + return listener1; + })); Network::Address::InstanceConstSharedPtr normal_address( new Network::Address::Ipv4Instance("127.0.0.1", 80)); // Original dst address nor port number match that of the listener's address. @@ -408,15 +412,15 @@ TEST_F(ConnectionHandlerTest, WildcardListenerWithOriginalDst) { } TEST_F(ConnectionHandlerTest, WildcardListenerWithNoOriginalDst) { - TestListener* test_listener1 = addListener(1, true, true, "test_listener1"); + TestListener* test_listener1 = addListener(1, true, false, true, "test_listener1"); Network::MockListener* listener1 = new Network::MockListener(); Network::ListenerCallbacks* listener_callbacks1; - EXPECT_CALL(dispatcher_, createListener_(_, _, _, true)) - .WillOnce(Invoke( - [&](Network::Socket&, Network::ListenerCallbacks& cb, bool, bool) -> Network::Listener* { - listener_callbacks1 = &cb; - return listener1; - })); + EXPECT_CALL(dispatcher_, createListener_(_, _, _, _, true)) + .WillOnce(Invoke([&](Network::Socket&, Network::ListenerCallbacks& cb, bool, bool, + bool) -> Network::Listener* { + listener_callbacks1 = &cb; + return listener1; + })); Network::Address::InstanceConstSharedPtr normal_address( new Network::Address::Ipv4Instance("127.0.0.1", 80)); Network::Address::InstanceConstSharedPtr any_address = Network::Utility::getAddressWithPort( From 982a27bfe687873c401c734aef2e34ea10323508 Mon Sep 17 00:00:00 2001 From: Greg Greenway Date: Mon, 2 Apr 2018 16:47:10 -0700 Subject: [PATCH 02/31] Update to use new socket setting facility Signed-off-by: Greg Greenway --- bazel/repository_locations.bzl | 2 +- include/envoy/event/dispatcher.h | 1 - include/envoy/network/listen_socket.h | 2 +- include/envoy/network/listener.h | 5 - source/common/event/dispatcher_impl.cc | 4 +- source/common/event/dispatcher_impl.h | 2 +- source/common/network/listener_impl.cc | 57 +----- source/common/network/listener_impl.h | 3 +- source/common/network/socket_option_impl.cc | 19 +- source/common/network/socket_option_impl.h | 23 ++- source/common/upstream/upstream_impl.cc | 8 +- source/server/config_validation/dispatcher.cc | 3 +- source/server/config_validation/dispatcher.h | 2 +- source/server/connection_handler_impl.cc | 10 +- source/server/http/admin.h | 1 - source/server/listener_manager_impl.cc | 5 +- source/server/listener_manager_impl.h | 3 +- test/common/http/codec_client_test.cc | 3 +- test/common/network/connection_impl_test.cc | 6 +- test/common/network/listener_impl_test.cc | 16 +- .../common/network/socket_option_impl_test.cc | 110 +++++------ test/common/ssl/ssl_socket_test.cc | 25 ++- .../proxy_protocol/proxy_protocol_test.cc | 2 - test/integration/fake_upstream.h | 1 - test/mocks/event/mocks.h | 10 +- test/mocks/network/mocks.h | 1 - test/server/connection_handler_test.cc | 176 +++++++++--------- 27 files changed, 222 insertions(+), 278 deletions(-) diff --git a/bazel/repository_locations.bzl b/bazel/repository_locations.bzl index 31a2972c14022..5b130b4908ea2 100644 --- a/bazel/repository_locations.bzl +++ b/bazel/repository_locations.bzl @@ -76,7 +76,7 @@ REPOSITORY_LOCATIONS = dict( urls = ["https://github.com/google/protobuf/archive/v3.5.0.tar.gz"], ), envoy_api = dict( - commit = "e79eea73224207d7aa547806541c09b83ded6a16", + commit = "28c23f754539369c57b28359067f100059ce0d0b", remote = "https://github.com/bmetzdorf/data-plane-api.git", # TODO FIXME ), grpc_httpjson_transcoding = dict( diff --git a/include/envoy/event/dispatcher.h b/include/envoy/event/dispatcher.h index 5f84e46f7c4e0..4edd5927e0b3b 100644 --- a/include/envoy/event/dispatcher.h +++ b/include/envoy/event/dispatcher.h @@ -102,7 +102,6 @@ class Dispatcher { */ virtual Network::ListenerPtr createListener(Network::Socket& socket, Network::ListenerCallbacks& cb, bool bind_to_port, - bool enable_tcp_fast_open, bool hand_off_restored_destination_connections) PURE; /** diff --git a/include/envoy/network/listen_socket.h b/include/envoy/network/listen_socket.h index 9a5a320b2d592..128d83b7d3bd8 100644 --- a/include/envoy/network/listen_socket.h +++ b/include/envoy/network/listen_socket.h @@ -31,7 +31,7 @@ class Socket { */ virtual void close() PURE; - enum class SocketState { PreBind, PostBind }; + enum class SocketState { PreBind, PostBind, Listening }; /** * Visitor class for setting socket options. diff --git a/include/envoy/network/listener.h b/include/envoy/network/listener.h index a2d1e3e4173a9..16cf4e5d481a3 100644 --- a/include/envoy/network/listener.h +++ b/include/envoy/network/listener.h @@ -44,11 +44,6 @@ class ListenerConfig { */ virtual bool bindToPort() PURE; - /** - * @return bool specifies whether the listener should support TCP Fast Open. - */ - virtual bool enableTcpFastOpen() PURE; - /** * @return bool if a connection should be handed off to another Listener after the original * destination address has been restored. 'true' when 'use_original_dst' flag in listener diff --git a/source/common/event/dispatcher_impl.cc b/source/common/event/dispatcher_impl.cc index 82df1a60ac504..b195d6ae29133 100644 --- a/source/common/event/dispatcher_impl.cc +++ b/source/common/event/dispatcher_impl.cc @@ -109,11 +109,9 @@ Filesystem::WatcherPtr DispatcherImpl::createFilesystemWatcher() { Network::ListenerPtr DispatcherImpl::createListener(Network::Socket& socket, Network::ListenerCallbacks& cb, - bool bind_to_port, bool enable_tcp_fast_open, - bool hand_off_restored_destination_connections) { + bool bind_to_port, bool hand_off_restored_destination_connections) { ASSERT(isThreadSafe()); return Network::ListenerPtr{new Network::ListenerImpl(*this, socket, cb, bind_to_port, - enable_tcp_fast_open, hand_off_restored_destination_connections)}; } diff --git a/source/common/event/dispatcher_impl.h b/source/common/event/dispatcher_impl.h index bf8167b891ebf..055e89fcb889c 100644 --- a/source/common/event/dispatcher_impl.h +++ b/source/common/event/dispatcher_impl.h @@ -47,7 +47,7 @@ class DispatcherImpl : Logger::Loggable, public Dispatcher { uint32_t events) override; Filesystem::WatcherPtr createFilesystemWatcher() override; Network::ListenerPtr createListener(Network::Socket& socket, Network::ListenerCallbacks& cb, - bool bind_to_port, bool enable_tcp_fast_open, + bool bind_to_port, bool hand_off_restored_destination_connections) override; TimerPtr createTimer(TimerCb cb) override; void deferredDelete(DeferredDeletablePtr&& to_delete) override; diff --git a/source/common/network/listener_impl.cc b/source/common/network/listener_impl.cc index 539e1f063eee9..ec5fadd892d39 100644 --- a/source/common/network/listener_impl.cc +++ b/source/common/network/listener_impl.cc @@ -14,16 +14,6 @@ #include "event2/listener.h" -// On macOS the socket MUST be listening already for TCP_FASTOPEN to be set and backlog MUST be 1 -// (the actual value is set via the net.inet.tcp.fastopen_backlog kernel parameter. -// For Linux we default to 128, which libevent is using in -// https://github.com/libevent/libevent/blob/release-2.1.8-stable/listener.c#L176 -#if defined(__APPLE__) -#define TFO_BACKLOG 1 -#else -#define TFO_BACKLOG 128 -#endif - namespace Envoy { namespace Network { @@ -56,8 +46,7 @@ void ListenerImpl::listenCallback(evconnlistener*, evutil_socket_t fd, sockaddr* } ListenerImpl::ListenerImpl(Event::DispatcherImpl& dispatcher, Socket& socket, ListenerCallbacks& cb, - bool bind_to_port, bool enable_tcp_fast_open, - bool hand_off_restored_destination_connections) + bool bind_to_port, bool hand_off_restored_destination_connections) : local_address_(nullptr), cb_(cb), hand_off_restored_destination_connections_(hand_off_restored_destination_connections), listener_(nullptr) { @@ -78,45 +67,11 @@ ListenerImpl::ListenerImpl(Event::DispatcherImpl& dispatcher, Socket& socket, Li fmt::format("cannot listen on socket: {}", socket.localAddress()->asString())); } - // TODO: cleanup redundant setsockopt logic - if (enable_tcp_fast_open) { - int backlog = TFO_BACKLOG; - int fd = socket.fd(); - int rc = setsockopt(fd, IPPROTO_TCP, TCP_FASTOPEN, &backlog, sizeof(backlog)); - if (rc == -1) { - if (errno == ENOPROTOOPT) { - throw EnvoyException(fmt::format( - "TCP Fast Open is unsupported on listening socket fd {} : {}", fd, strerror(errno))); - } else { - throw EnvoyException( - fmt::format("Failed to enable TCP Fast Open on listening socket fd {} : {}", fd, - strerror(errno))); - } - } else { - ENVOY_LOG_MISC(debug, "Enabled TFO on listening socket fd {}.", fd); - } - } else { - int curbacklog; - int fd = socket.fd(); - socklen_t optlen = sizeof(curbacklog); - int rc = getsockopt(fd, IPPROTO_TCP, TCP_FASTOPEN, &curbacklog, &optlen); - // curbacklog == 0 means TFO is supported and already disabled - if (rc != -1 && curbacklog != 0) { - int backlog = 0; - int rc = setsockopt(fd, IPPROTO_TCP, TCP_FASTOPEN, &backlog, sizeof(backlog)); - if (rc == -1) { - if (errno == ENOPROTOOPT) { - throw EnvoyException( - fmt::format("TCP Fast Open is unsupported on listening socket fd {} : {}", fd, - strerror(errno))); - } else { - throw EnvoyException( - fmt::format("Failed to disable TCP Fast Open on listening socket fd {} : {}", fd, - strerror(errno))); - } - } else { - ENVOY_LOG_MISC(debug, "Disabled TFO on listening socket fd {}.", fd); - } + for (auto& option : *socket.options()) { + if (!option->setOption(socket, Socket::SocketState::Listening)) { + throw CreateListenerException( + fmt::format("cannot set post-listen socket option on socket: {}", + socket.localAddress()->asString())); } } diff --git a/source/common/network/listener_impl.h b/source/common/network/listener_impl.h index 555b9a9f8b5a2..d55ccfc623bd3 100644 --- a/source/common/network/listener_impl.h +++ b/source/common/network/listener_impl.h @@ -17,8 +17,7 @@ namespace Network { class ListenerImpl : public Listener { public: ListenerImpl(Event::DispatcherImpl& dispatcher, Socket& socket, ListenerCallbacks& cb, - bool bind_to_port, bool enable_tcp_fast_open, - bool hand_off_restored_destination_connections); + bool bind_to_port, bool hand_off_restored_destination_connections); protected: virtual Address::InstanceConstSharedPtr getLocalAddress(int fd); diff --git a/source/common/network/socket_option_impl.cc b/source/common/network/socket_option_impl.cc index 056af399df927..1dac8d9d6f5e2 100644 --- a/source/common/network/socket_option_impl.cc +++ b/source/common/network/socket_option_impl.cc @@ -32,8 +32,25 @@ bool SocketOptionImpl::setOption(Socket& socket, Socket::SocketState state) cons return false; } } - } + } else if (state == Socket::SocketState::Listening) { + if (accept_tcp_fast_open_.has_value()) { + const int tfo_value = accept_tcp_fast_open_.value() ? ENVOY_TCP_FASTOPEN_BACKLOG : 0; + const SocketOptionName option_name = ENVOY_SOCKET_TCP_FASTOPEN; + int error = -1; + if (option_name) { + error = Api::OsSysCallsSingleton::get().setsockopt( + socket.fd(), IPPROTO_TCP, option_name.value(), + reinterpret_cast(&tfo_value), sizeof(tfo_value)); + } else { + error = ENOTSUP; + } + if (error != 0) { + ENVOY_LOG(warn, "Setting IP_TCP_FASTOPEN on listener socket failed: {}", strerror(error)); + return false; + } + } + } return true; } diff --git a/source/common/network/socket_option_impl.h b/source/common/network/socket_option_impl.h index 766e30ce9e2be..6694bbb6c294a 100644 --- a/source/common/network/socket_option_impl.h +++ b/source/common/network/socket_option_impl.h @@ -41,10 +41,28 @@ typedef absl::optional SocketOptionName; #define ENVOY_SOCKET_IPV6_FREEBIND Network::SocketOptionName() #endif +#ifdef TCP_FASTOPEN +#define ENVOY_SOCKET_TCP_FASTOPEN Network::SocketOptionName(TCP_FASTOPEN) +#else +#define ENVOY_SOCKET_TCP_FASTOPEN Network::SocketOptionName() +#endif + +// On macOS the socket MUST be listening already for TCP_FASTOPEN to be set and backlog MUST be 1 +// (the actual value is set via the net.inet.tcp.fastopen_backlog kernel parameter. +// For Linux we default to 128, which libevent is using in +// https://github.com/libevent/libevent/blob/release-2.1.8-stable/listener.c#L176 +#if defined(__APPLE__) +#define ENVOY_TCP_FASTOPEN_BACKLOG 1 +#else +#define ENVOY_TCP_FASTOPEN_BACKLOG 128 +#endif + class SocketOptionImpl : public Socket::Option, Logger::Loggable { public: - SocketOptionImpl(absl::optional transparent, absl::optional freebind) - : transparent_(transparent), freebind_(freebind) {} + SocketOptionImpl(absl::optional transparent, absl::optional freebind, + absl::optional accept_tcp_fast_open) + : transparent_(transparent), freebind_(freebind), + accept_tcp_fast_open_(accept_tcp_fast_open) {} // Socket::Option bool setOption(Socket& socket, Socket::SocketState state) const override; @@ -72,6 +90,7 @@ class SocketOptionImpl : public Socket::Option, Logger::Loggable transparent_; const absl::optional freebind_; + const absl::optional accept_tcp_fast_open_; }; } // namespace Network diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index 7d35dbe27fb71..4b3587f9b4e44 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -75,9 +75,11 @@ uint64_t parseFeatures(const envoy::api::v2::Cluster& config, class UpstreamSocketOption : public Network::SocketOptionImpl { public: UpstreamSocketOption(const ClusterInfo& cluster_info) - : Network::SocketOptionImpl({}, cluster_info.features() & ClusterInfo::Features::FREEBIND - ? true - : absl::optional{}) {} + : Network::SocketOptionImpl({}, + cluster_info.features() & ClusterInfo::Features::FREEBIND + ? true + : absl::optional{}, + {}) {} }; } // namespace diff --git a/source/server/config_validation/dispatcher.cc b/source/server/config_validation/dispatcher.cc index 06855c4ea2e1e..7966f746e1cb9 100644 --- a/source/server/config_validation/dispatcher.cc +++ b/source/server/config_validation/dispatcher.cc @@ -17,8 +17,7 @@ Network::DnsResolverSharedPtr ValidationDispatcher::createDnsResolver( } Network::ListenerPtr ValidationDispatcher::createListener(Network::Socket&, - Network::ListenerCallbacks&, bool, bool, - bool) { + Network::ListenerCallbacks&, bool, bool) { NOT_IMPLEMENTED; } diff --git a/source/server/config_validation/dispatcher.h b/source/server/config_validation/dispatcher.h index aef51eb9af57c..54fad3c12943f 100644 --- a/source/server/config_validation/dispatcher.h +++ b/source/server/config_validation/dispatcher.h @@ -21,7 +21,7 @@ class ValidationDispatcher : public DispatcherImpl { Network::DnsResolverSharedPtr createDnsResolver( const std::vector& resolvers) override; Network::ListenerPtr createListener(Network::Socket&, Network::ListenerCallbacks&, - bool bind_to_port, bool enable_tcp_fast_open, + bool bind_to_port, bool hand_off_restored_destination_connections) override; }; diff --git a/source/server/connection_handler_impl.cc b/source/server/connection_handler_impl.cc index de2aac31bb8d4..e32bfcb835e31 100644 --- a/source/server/connection_handler_impl.cc +++ b/source/server/connection_handler_impl.cc @@ -54,11 +54,11 @@ void ConnectionHandlerImpl::ActiveListener::removeConnection(ActiveConnection& c ConnectionHandlerImpl::ActiveListener::ActiveListener(ConnectionHandlerImpl& parent, Network::ListenerConfig& config) - : ActiveListener(parent, - parent.dispatcher_.createListener( - config.socket(), *this, config.bindToPort(), config.enableTcpFastOpen(), - config.handOffRestoredDestinationConnections()), - config) {} + : ActiveListener( + parent, + parent.dispatcher_.createListener(config.socket(), *this, config.bindToPort(), + config.handOffRestoredDestinationConnections()), + config) {} ConnectionHandlerImpl::ActiveListener::ActiveListener(ConnectionHandlerImpl& parent, Network::ListenerPtr&& listener, diff --git a/source/server/http/admin.h b/source/server/http/admin.h index e92f65b28b76a..4f5f3f13efe98 100644 --- a/source/server/http/admin.h +++ b/source/server/http/admin.h @@ -195,7 +195,6 @@ class AdminImpl : public Admin, return parent_.transport_socket_factory_; } bool bindToPort() override { return true; } - bool enableTcpFastOpen() override { return false; } bool handOffRestoredDestinationConnections() const override { return false; } uint32_t perConnectionBufferLimitBytes() override { return 0; } Stats::Scope& listenerScope() override { return *scope_; } diff --git a/source/server/listener_manager_impl.cc b/source/server/listener_manager_impl.cc index 9d75ee626d4dd..5faa39ded0d47 100644 --- a/source/server/listener_manager_impl.cc +++ b/source/server/listener_manager_impl.cc @@ -114,7 +114,9 @@ class ListenerSocketOption : public Network::SocketOptionImpl { ListenerSocketOption(const envoy::api::v2::Listener& config) : Network::SocketOptionImpl( PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, transparent, absl::optional{}), - PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, freebind, absl::optional{})) {} + PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, freebind, absl::optional{}), + PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, accept_tcp_fast_open, absl::optional{})) { + } }; ListenerImpl::ListenerImpl(const envoy::api::v2::Listener& config, ListenerManagerImpl& parent, @@ -125,7 +127,6 @@ ListenerImpl::ListenerImpl(const envoy::api::v2::Listener& config, ListenerManag listener_scope_( parent_.server_.stats().createScope(fmt::format("listener.{}.", address_->asString()))), bind_to_port_(PROTOBUF_GET_WRAPPED_OR_DEFAULT(config.deprecated_v1(), bind_to_port, true)), - enable_tcp_fast_open_(config.enable_tcp_fast_open()), hand_off_restored_destination_connections_( PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, use_original_dst, false)), per_connection_buffer_limit_bytes_( diff --git a/source/server/listener_manager_impl.h b/source/server/listener_manager_impl.h index 3010c62e1f090..856b7e3877b91 100644 --- a/source/server/listener_manager_impl.h +++ b/source/server/listener_manager_impl.h @@ -48,6 +48,7 @@ class ProdListenerComponentFactory : public ListenerComponentFactory, Configuration::ListenerFactoryContext& context) override { return createListenerFilterFactoryList_(filters, context); } + Network::SocketSharedPtr createListenSocket(Network::Address::InstanceConstSharedPtr address, const Network::Socket::OptionsSharedPtr& options, bool bind_to_port) override; @@ -215,7 +216,6 @@ class ListenerImpl : public Network::ListenerConfig, Network::FilterChainFactory& filterChainFactory() override { return *this; } Network::Socket& socket() override { return *socket_; } bool bindToPort() override { return bind_to_port_; } - bool enableTcpFastOpen() override { return enable_tcp_fast_open_; } bool handOffRestoredDestinationConnections() const override { return hand_off_restored_destination_connections_; } @@ -277,7 +277,6 @@ class ListenerImpl : public Network::ListenerConfig, std::vector tls_contexts_; std::vector transport_socket_factories_; const bool bind_to_port_; - const bool enable_tcp_fast_open_; const bool hand_off_restored_destination_connections_; const uint32_t per_connection_buffer_limit_bytes_; const uint64_t listener_tag_; diff --git a/test/common/http/codec_client_test.cc b/test/common/http/codec_client_test.cc index de5dc2a186775..9bf053a7bcebf 100644 --- a/test/common/http/codec_client_test.cc +++ b/test/common/http/codec_client_test.cc @@ -261,8 +261,7 @@ class CodecNetworkTest : public testing::TestWithParamcreateListener(socket_, listener_callbacks_, true, false, false); + upstream_listener_ = dispatcher_->createListener(socket_, listener_callbacks_, true, false); Network::ClientConnectionPtr client_connection = dispatcher_->createClientConnection( socket_.localAddress(), source_address_, Network::Test::createRawBufferSocket(), nullptr); client_connection_ = client_connection.get(); diff --git a/test/common/network/connection_impl_test.cc b/test/common/network/connection_impl_test.cc index 95700d2fcfe65..144efada8712b 100644 --- a/test/common/network/connection_impl_test.cc +++ b/test/common/network/connection_impl_test.cc @@ -83,7 +83,7 @@ class ConnectionImplTest : public testing::TestWithParam { if (dispatcher_.get() == nullptr) { dispatcher_.reset(new Event::DispatcherImpl); } - listener_ = dispatcher_->createListener(socket_, listener_callbacks_, true, false, false); + listener_ = dispatcher_->createListener(socket_, listener_callbacks_, true, false); client_connection_ = dispatcher_->createClientConnection( socket_.localAddress(), source_address_, Network::Test::createRawBufferSocket(), nullptr); @@ -767,7 +767,7 @@ TEST_P(ConnectionImplTest, BindFailureTest) { new Network::Address::Ipv6Instance(address_string, 0)}; } dispatcher_.reset(new Event::DispatcherImpl); - listener_ = dispatcher_->createListener(socket_, listener_callbacks_, true, false, false); + listener_ = dispatcher_->createListener(socket_, listener_callbacks_, true, false); client_connection_ = dispatcher_->createClientConnection( socket_.localAddress(), source_address_, Network::Test::createRawBufferSocket(), nullptr); @@ -1161,7 +1161,7 @@ class ReadBufferLimitTest : public ConnectionImplTest { void readBufferLimitTest(uint32_t read_buffer_limit, uint32_t expected_chunk_size) { const uint32_t buffer_size = 256 * 1024; dispatcher_.reset(new Event::DispatcherImpl); - listener_ = dispatcher_->createListener(socket_, listener_callbacks_, true, false, false); + listener_ = dispatcher_->createListener(socket_, listener_callbacks_, true, false); client_connection_ = dispatcher_->createClientConnection( socket_.localAddress(), Network::Address::InstanceConstSharedPtr(), diff --git a/test/common/network/listener_impl_test.cc b/test/common/network/listener_impl_test.cc index bffbe5d190aeb..5d18d968826d0 100644 --- a/test/common/network/listener_impl_test.cc +++ b/test/common/network/listener_impl_test.cc @@ -30,7 +30,7 @@ static void errorCallbackTest(Address::IpVersion version) { Network::MockListenerCallbacks listener_callbacks; Network::MockConnectionHandler connection_handler; Network::ListenerPtr listener = - dispatcher.createListener(socket, listener_callbacks, true, false, false); + dispatcher.createListener(socket, listener_callbacks, true, false); Network::ClientConnectionPtr client_connection = dispatcher.createClientConnection( socket.localAddress(), Network::Address::InstanceConstSharedPtr(), @@ -65,9 +65,8 @@ TEST_P(ListenerImplDeathTest, ErrorCallback) { class TestListenerImpl : public ListenerImpl { public: TestListenerImpl(Event::DispatcherImpl& dispatcher, Socket& socket, ListenerCallbacks& cb, - bool bind_to_port, bool enable_tcp_fast_open, - bool hand_off_restored_destination_connections) - : ListenerImpl(dispatcher, socket, cb, bind_to_port, enable_tcp_fast_open, + bool bind_to_port, bool hand_off_restored_destination_connections) + : ListenerImpl(dispatcher, socket, cb, bind_to_port, hand_off_restored_destination_connections) {} MOCK_METHOD1(getLocalAddress, Address::InstanceConstSharedPtr(int fd)); @@ -96,10 +95,9 @@ TEST_P(ListenerImplTest, UseActualDst) { Network::MockListenerCallbacks listener_callbacks1; Network::MockConnectionHandler connection_handler; // Do not redirect since use_original_dst is false. - Network::TestListenerImpl listener(dispatcher, socket, listener_callbacks1, true, false, true); + Network::TestListenerImpl listener(dispatcher, socket, listener_callbacks1, true, true); Network::MockListenerCallbacks listener_callbacks2; - Network::TestListenerImpl listenerDst(dispatcher, socketDst, listener_callbacks2, false, false, - false); + Network::TestListenerImpl listenerDst(dispatcher, socketDst, listener_callbacks2, false, false); Network::ClientConnectionPtr client_connection = dispatcher.createClientConnection( socket.localAddress(), Network::Address::InstanceConstSharedPtr(), @@ -133,7 +131,7 @@ TEST_P(ListenerImplTest, WildcardListenerUseActualDst) { Network::MockListenerCallbacks listener_callbacks; Network::MockConnectionHandler connection_handler; // Do not redirect since use_original_dst is false. - Network::TestListenerImpl listener(dispatcher, socket, listener_callbacks, true, false, true); + Network::TestListenerImpl listener(dispatcher, socket, listener_callbacks, true, true); auto local_dst_address = Network::Utility::getAddressWithPort( *Network::Test::getCanonicalLoopbackAddress(version_), socket.localAddress()->ip()->port()); @@ -180,7 +178,7 @@ TEST_P(ListenerImplTest, WildcardListenerIpv4Compat) { ASSERT_TRUE(socket.localAddress()->ip()->isAnyAddress()); // Do not redirect since use_original_dst is false. - Network::TestListenerImpl listener(dispatcher, socket, listener_callbacks, true, false, true); + Network::TestListenerImpl listener(dispatcher, socket, listener_callbacks, true, true); auto listener_address = Network::Utility::getAddressWithPort( *Network::Test::getCanonicalLoopbackAddress(version_), socket.localAddress()->ip()->port()); diff --git a/test/common/network/socket_option_impl_test.cc b/test/common/network/socket_option_impl_test.cc index fcdbbba7c5fc1..fa2cc4c057e80 100644 --- a/test/common/network/socket_option_impl_test.cc +++ b/test/common/network/socket_option_impl_test.cc @@ -23,6 +23,24 @@ class SocketOptionImplTest : public testing::Test { NiceMock socket_; Api::MockOsSysCalls os_sys_calls_; TestThreadsafeSingletonInjector os_calls{&os_sys_calls_}; + + void testSetSocketOptionSuccess(SocketOptionImpl& socket_option, int socket_level, + Network::SocketOptionName option_name, int option_val, + Socket::SocketState when) { + if (option_name.has_value()) { + Address::Ipv4Instance address("1.2.3.4", 5678); + const int fd = address.socket(Address::SocketType::Stream); + EXPECT_CALL(socket_, fd()).WillRepeatedly(Return(fd)); + EXPECT_CALL(os_sys_calls_, setsockopt_(_, socket_level, option_name.value(), _, sizeof(int))) + .WillOnce(Invoke([option_val](int, int, int, const void* optval, socklen_t) -> int { + EXPECT_EQ(option_val, *static_cast(optval)); + return 0; + })); + EXPECT_TRUE(socket_option.setOption(socket_, when)); + } else { + EXPECT_FALSE(socket_option.setOption(socket_, when)); + } + } }; // We fail to set the option if the socket FD is bad. @@ -33,105 +51,63 @@ TEST_F(SocketOptionImplTest, BadFd) { // Nop when there are no socket options set. TEST_F(SocketOptionImplTest, SetOptionEmptyNop) { - SocketOptionImpl socket_option{{}, {}}; + SocketOptionImpl socket_option{{}, {}, {}}; EXPECT_TRUE(socket_option.setOption(socket_, Socket::SocketState::PreBind)); EXPECT_TRUE(socket_option.setOption(socket_, Socket::SocketState::PostBind)); + EXPECT_TRUE(socket_option.setOption(socket_, Socket::SocketState::Listening)); } // We fail to set the option when the underlying setsockopt syscall fails. TEST_F(SocketOptionImplTest, SetOptionTransparentFailure) { - SocketOptionImpl socket_option{true, {}}; + SocketOptionImpl socket_option{true, {}, {}}; EXPECT_FALSE(socket_option.setOption(socket_, Socket::SocketState::PreBind)); } // We fail to set the option when the underlying setsockopt syscall fails. TEST_F(SocketOptionImplTest, SetOptionFreebindFailure) { - SocketOptionImpl socket_option{{}, true}; + SocketOptionImpl socket_option{{}, true, {}}; EXPECT_FALSE(socket_option.setOption(socket_, Socket::SocketState::PreBind)); } +// We fail to set the tcp-fastopen option when the underlying setsockopt syscall fails. +TEST_F(SocketOptionImplTest, SetOptionTcpFastopenFailure) { + SocketOptionImpl socket_option{{}, {}, true}; + EXPECT_FALSE(socket_option.setOption(socket_, Socket::SocketState::Listening)); +} + // The happy path for setOption(); IP_TRANSPARENT is set to true. TEST_F(SocketOptionImplTest, SetOptionTransparentSuccessTrue) { - SocketOptionImpl socket_option{true, {}}; - if (ENVOY_SOCKET_IP_TRANSPARENT.has_value()) { - Address::Ipv4Instance address("1.2.3.4", 5678); - const int fd = address.socket(Address::SocketType::Stream); - EXPECT_CALL(socket_, fd()).WillRepeatedly(Return(fd)); - EXPECT_CALL(os_sys_calls_, - setsockopt_(_, IPPROTO_IP, ENVOY_SOCKET_IP_TRANSPARENT.value(), _, sizeof(int))) - .WillOnce(Invoke([](int, int, int, const void* optval, socklen_t) -> int { - EXPECT_EQ(1, *static_cast(optval)); - return 0; - })); - EXPECT_TRUE(socket_option.setOption(socket_, Socket::SocketState::PreBind)); - } else { - EXPECT_FALSE(socket_option.setOption(socket_, Socket::SocketState::PreBind)); - } + SocketOptionImpl socket_option{true, {}, {}}; + testSetSocketOptionSuccess(socket_option, IPPROTO_IP, ENVOY_SOCKET_IP_TRANSPARENT, 1, + Socket::SocketState::PreBind); } // The happy path for setOption(); IP_FREEBIND is set to true. TEST_F(SocketOptionImplTest, SetOptionFreebindSuccessTrue) { - SocketOptionImpl socket_option{{}, true}; - if (ENVOY_SOCKET_IP_FREEBIND.has_value()) { - Address::Ipv4Instance address("1.2.3.4", 5678); - const int fd = address.socket(Address::SocketType::Stream); - EXPECT_CALL(socket_, fd()).WillRepeatedly(Return(fd)); - EXPECT_CALL(os_sys_calls_, - setsockopt_(_, IPPROTO_IP, ENVOY_SOCKET_IP_FREEBIND.value(), _, sizeof(int))) - .WillOnce(Invoke([](int, int, int, const void* optval, socklen_t) -> int { - EXPECT_EQ(1, *static_cast(optval)); - return 0; - })); - EXPECT_TRUE(socket_option.setOption(socket_, Socket::SocketState::PreBind)); - } else { - EXPECT_FALSE(socket_option.setOption(socket_, Socket::SocketState::PreBind)); - } + SocketOptionImpl socket_option{{}, true, {}}; + testSetSocketOptionSuccess(socket_option, IPPROTO_IP, ENVOY_SOCKET_IP_FREEBIND, 1, + Socket::SocketState::PreBind); } // The happy path for setOpion(); IP_TRANSPARENT is set to false. TEST_F(SocketOptionImplTest, SetOptionTransparentSuccessFalse) { - SocketOptionImpl socket_option{false, {}}; - if (ENVOY_SOCKET_IP_TRANSPARENT.has_value()) { - Address::Ipv4Instance address("1.2.3.4", 5678); - const int fd = address.socket(Address::SocketType::Stream); - EXPECT_CALL(socket_, fd()).WillRepeatedly(Return(fd)); - EXPECT_CALL(os_sys_calls_, - setsockopt_(_, IPPROTO_IP, ENVOY_SOCKET_IP_TRANSPARENT.value(), _, sizeof(int))) - .Times(2) - .WillRepeatedly(Invoke([](int, int, int, const void* optval, socklen_t) -> int { - EXPECT_EQ(0, *static_cast(optval)); - return 0; - })); - EXPECT_TRUE(socket_option.setOption(socket_, Socket::SocketState::PreBind)); - EXPECT_TRUE(socket_option.setOption(socket_, Socket::SocketState::PostBind)); - } else { - EXPECT_FALSE(socket_option.setOption(socket_, Socket::SocketState::PreBind)); - EXPECT_FALSE(socket_option.setOption(socket_, Socket::SocketState::PostBind)); - } + SocketOptionImpl socket_option{false, {}, {}}; + testSetSocketOptionSuccess(socket_option, IPPROTO_IP, ENVOY_SOCKET_IP_TRANSPARENT, 0, + Socket::SocketState::PreBind); + testSetSocketOptionSuccess(socket_option, IPPROTO_IP, ENVOY_SOCKET_IP_TRANSPARENT, 0, + Socket::SocketState::PostBind); } // The happy path for setOpion(); IP_FREEBIND is set to false. TEST_F(SocketOptionImplTest, SetOptionFreebindSuccessFalse) { - SocketOptionImpl socket_option{{}, false}; - if (ENVOY_SOCKET_IP_FREEBIND.has_value()) { - Address::Ipv4Instance address("1.2.3.4", 5678); - const int fd = address.socket(Address::SocketType::Stream); - EXPECT_CALL(socket_, fd()).WillRepeatedly(Return(fd)); - EXPECT_CALL(os_sys_calls_, - setsockopt_(_, IPPROTO_IP, ENVOY_SOCKET_IP_FREEBIND.value(), _, sizeof(int))) - .WillOnce(Invoke([](int, int, int, const void* optval, socklen_t) -> int { - EXPECT_EQ(0, *static_cast(optval)); - return 0; - })); - EXPECT_TRUE(socket_option.setOption(socket_, Socket::SocketState::PreBind)); - } else { - EXPECT_FALSE(socket_option.setOption(socket_, Socket::SocketState::PreBind)); - } + SocketOptionImpl socket_option{{}, false, {}}; + testSetSocketOptionSuccess(socket_option, IPPROTO_IP, ENVOY_SOCKET_IP_FREEBIND, 0, + Socket::SocketState::PreBind); } // Freebind settings have no effect on post-bind behavior. TEST_F(SocketOptionImplTest, SetOptionFreebindPostBind) { - SocketOptionImpl socket_option{{}, true}; + SocketOptionImpl socket_option{{}, true, {}}; EXPECT_TRUE(socket_option.setOption(socket_, Socket::SocketState::PostBind)); } diff --git a/test/common/ssl/ssl_socket_test.cc b/test/common/ssl/ssl_socket_test.cc index 9fd9ef6b965c7..073c46d00a33c 100644 --- a/test/common/ssl/ssl_socket_test.cc +++ b/test/common/ssl/ssl_socket_test.cc @@ -61,7 +61,7 @@ void testUtil(const std::string& client_ctx_json, const std::string& server_ctx_ true); Network::MockListenerCallbacks callbacks; Network::MockConnectionHandler connection_handler; - Network::ListenerPtr listener = dispatcher.createListener(socket, callbacks, true, false, false); + Network::ListenerPtr listener = dispatcher.createListener(socket, callbacks, true, false); Json::ObjectSharedPtr client_ctx_loader = TestEnvironment::jsonLoadFromString(client_ctx_json); ClientContextConfigImpl client_ctx_config(*client_ctx_loader); @@ -161,7 +161,7 @@ const std::string testUtilV2(const envoy::api::v2::Listener& server_proto, true); NiceMock callbacks; Network::MockConnectionHandler connection_handler; - Network::ListenerPtr listener = dispatcher.createListener(socket, callbacks, true, false, false); + Network::ListenerPtr listener = dispatcher.createListener(socket, callbacks, true, false); ClientContextConfigImpl client_ctx_config(client_ctx_proto); ClientSslSocketFactory client_ssl_socket_factory(client_ctx_config, manager, stats_store); @@ -741,7 +741,7 @@ TEST_P(SslSocketTest, FlushCloseDuringHandshake) { true); Network::MockListenerCallbacks callbacks; Network::MockConnectionHandler connection_handler; - Network::ListenerPtr listener = dispatcher.createListener(socket, callbacks, true, false, false); + Network::ListenerPtr listener = dispatcher.createListener(socket, callbacks, true, false); Network::ClientConnectionPtr client_connection = dispatcher.createClientConnection( socket.localAddress(), Network::Address::InstanceConstSharedPtr(), @@ -800,7 +800,7 @@ TEST_P(SslSocketTest, HalfClose) { Network::MockListenerCallbacks listener_callbacks; Network::MockConnectionHandler connection_handler; Network::ListenerPtr listener = - dispatcher.createListener(socket, listener_callbacks, true, false, false); + dispatcher.createListener(socket, listener_callbacks, true, false); std::shared_ptr server_read_filter(new Network::MockReadFilter()); std::shared_ptr client_read_filter(new Network::MockReadFilter()); @@ -882,7 +882,7 @@ TEST_P(SslSocketTest, ClientAuthMultipleCAs) { true); Network::MockListenerCallbacks callbacks; Network::MockConnectionHandler connection_handler; - Network::ListenerPtr listener = dispatcher.createListener(socket, callbacks, true, false, false); + Network::ListenerPtr listener = dispatcher.createListener(socket, callbacks, true, false); std::string client_ctx_json = R"EOF( { @@ -965,10 +965,8 @@ void testTicketSessionResumption(const std::string& server_ctx_json1, true); NiceMock callbacks; Network::MockConnectionHandler connection_handler; - Network::ListenerPtr listener1 = - dispatcher.createListener(socket1, callbacks, true, false, false); - Network::ListenerPtr listener2 = - dispatcher.createListener(socket2, callbacks, true, false, false); + Network::ListenerPtr listener1 = dispatcher.createListener(socket1, callbacks, true, false); + Network::ListenerPtr listener2 = dispatcher.createListener(socket2, callbacks, true, false); Json::ObjectSharedPtr client_ctx_loader = TestEnvironment::jsonLoadFromString(client_ctx_json); ClientContextConfigImpl client_ctx_config(*client_ctx_loader); @@ -1291,9 +1289,8 @@ TEST_P(SslSocketTest, ClientAuthCrossListenerSessionResumption) { true); Network::MockListenerCallbacks callbacks; Network::MockConnectionHandler connection_handler; - Network::ListenerPtr listener = dispatcher.createListener(socket, callbacks, true, false, false); - Network::ListenerPtr listener2 = - dispatcher.createListener(socket2, callbacks, true, false, false); + Network::ListenerPtr listener = dispatcher.createListener(socket, callbacks, true, false); + Network::ListenerPtr listener2 = dispatcher.createListener(socket2, callbacks, true, false); std::string client_ctx_json = R"EOF( { @@ -1399,7 +1396,7 @@ TEST_P(SslSocketTest, SslError) { true); Network::MockListenerCallbacks callbacks; Network::MockConnectionHandler connection_handler; - Network::ListenerPtr listener = dispatcher.createListener(socket, callbacks, true, false, false); + Network::ListenerPtr listener = dispatcher.createListener(socket, callbacks, true, false); Network::ClientConnectionPtr client_connection = dispatcher.createClientConnection( socket.localAddress(), Network::Address::InstanceConstSharedPtr(), @@ -2034,7 +2031,7 @@ class SslReadBufferLimitTest : public SslCertsTest, server_ssl_socket_factory_.reset( new ServerSslSocketFactory(*server_ctx_config_, "", {}, true, *manager_, stats_store_)); - listener_ = dispatcher_->createListener(socket_, listener_callbacks_, true, false, false); + listener_ = dispatcher_->createListener(socket_, listener_callbacks_, true, false); client_ctx_loader_ = TestEnvironment::jsonLoadFromString(client_ctx_json_); client_ctx_config_.reset(new ClientContextConfigImpl(*client_ctx_loader_)); diff --git a/test/extensions/filters/listener/proxy_protocol/proxy_protocol_test.cc b/test/extensions/filters/listener/proxy_protocol/proxy_protocol_test.cc index f6c78bf5db19e..d5624762c7a30 100644 --- a/test/extensions/filters/listener/proxy_protocol/proxy_protocol_test.cc +++ b/test/extensions/filters/listener/proxy_protocol/proxy_protocol_test.cc @@ -60,7 +60,6 @@ class ProxyProtocolTest : public testing::TestWithParam, public Network::Filt return *parent_.transport_socket_factory_; } bool bindToPort() override { return true; } - bool enableTcpFastOpen() override { return false; } bool handOffRestoredDestinationConnections() const override { return false; } uint32_t perConnectionBufferLimitBytes() override { return 0; } Stats::Scope& listenerScope() override { return parent_.stats_store_; } diff --git a/test/mocks/event/mocks.h b/test/mocks/event/mocks.h index d44f6ce2a14db..643bb7120faf1 100644 --- a/test/mocks/event/mocks.h +++ b/test/mocks/event/mocks.h @@ -54,10 +54,10 @@ class MockDispatcher : public Dispatcher { } Network::ListenerPtr createListener(Network::Socket& socket, Network::ListenerCallbacks& cb, - bool bind_to_port, bool enable_tcp_fast_open, + bool bind_to_port, bool hand_off_restored_destination_connections) override { - return Network::ListenerPtr{createListener_(socket, cb, bind_to_port, enable_tcp_fast_open, - hand_off_restored_destination_connections)}; + return Network::ListenerPtr{ + createListener_(socket, cb, bind_to_port, hand_off_restored_destination_connections)}; } TimerPtr createTimer(TimerCb cb) override { return TimerPtr{createTimer_(cb)}; } @@ -90,9 +90,9 @@ class MockDispatcher : public Dispatcher { MOCK_METHOD4(createFileEvent_, FileEvent*(int fd, FileReadyCb cb, FileTriggerType trigger, uint32_t events)); MOCK_METHOD0(createFilesystemWatcher_, Filesystem::Watcher*()); - MOCK_METHOD5(createListener_, + MOCK_METHOD4(createListener_, Network::Listener*(Network::Socket& socket, Network::ListenerCallbacks& cb, - bool bind_to_port, bool enable_tcp_fast_open, + bool bind_to_port, bool hand_off_restored_destination_connections)); MOCK_METHOD1(createTimer_, Timer*(TimerCb cb)); MOCK_METHOD1(deferredDelete_, void(DeferredDeletable* to_delete)); diff --git a/test/mocks/network/mocks.h b/test/mocks/network/mocks.h index 0cd7a169bcab9..9b0d70ed699e9 100644 --- a/test/mocks/network/mocks.h +++ b/test/mocks/network/mocks.h @@ -308,7 +308,6 @@ class MockListenerConfig : public ListenerConfig { MOCK_METHOD0(socket, Socket&()); MOCK_METHOD0(transportSocketFactory, TransportSocketFactory&()); MOCK_METHOD0(bindToPort, bool()); - MOCK_METHOD0(enableTcpFastOpen, bool()); MOCK_CONST_METHOD0(handOffRestoredDestinationConnections, bool()); MOCK_METHOD0(perConnectionBufferLimitBytes, uint32_t()); MOCK_METHOD0(listenerScope, Stats::Scope&()); diff --git a/test/server/connection_handler_test.cc b/test/server/connection_handler_test.cc index a5861df0e5928..e9b95e6c01e57 100644 --- a/test/server/connection_handler_test.cc +++ b/test/server/connection_handler_test.cc @@ -30,10 +30,8 @@ class ConnectionHandlerTest : public testing::Test, protected Logger::Loggable { public: TestListener(ConnectionHandlerTest& parent, uint64_t tag, bool bind_to_port, - bool enable_tcp_fast_open, bool hand_off_restored_destination_connections, - const std::string& name) + bool hand_off_restored_destination_connections, const std::string& name) : parent_(parent), tag_(tag), bind_to_port_(bind_to_port), - enable_tcp_fast_open_(enable_tcp_fast_open), hand_off_restored_destination_connections_(hand_off_restored_destination_connections), name_(name) {} @@ -43,7 +41,6 @@ class ConnectionHandlerTest : public testing::Test, protected Logger::Loggable TestListenerPtr; - TestListener* addListener(uint64_t tag, bool bind_to_port, bool enable_tcp_fast_open, + TestListener* addListener(uint64_t tag, bool bind_to_port, bool hand_off_restored_destination_connections, const std::string& name) { - TestListener* listener = new TestListener(*this, tag, bind_to_port, enable_tcp_fast_open, - hand_off_restored_destination_connections, name); + TestListener* listener = + new TestListener(*this, tag, bind_to_port, hand_off_restored_destination_connections, name); listener->moveIntoListBack(TestListenerPtr{listener}, listeners_); return listener; } @@ -85,14 +81,14 @@ TEST_F(ConnectionHandlerTest, RemoveListener) { Network::MockListener* listener = new NiceMock(); Network::ListenerCallbacks* listener_callbacks; - EXPECT_CALL(dispatcher_, createListener_(_, _, _, _, false)) - .WillOnce(Invoke([&](Network::Socket&, Network::ListenerCallbacks& cb, bool, bool, - bool) -> Network::Listener* { - listener_callbacks = &cb; - return listener; - - })); - TestListener* test_listener = addListener(1, true, false, false, "test_listener"); + EXPECT_CALL(dispatcher_, createListener_(_, _, _, false)) + .WillOnce(Invoke( + [&](Network::Socket&, Network::ListenerCallbacks& cb, bool, bool) -> Network::Listener* { + listener_callbacks = &cb; + return listener; + + })); + TestListener* test_listener = addListener(1, true, false, "test_listener"); EXPECT_CALL(test_listener->socket_, localAddress()); handler_->addListener(*test_listener); @@ -123,14 +119,14 @@ TEST_F(ConnectionHandlerTest, DestroyCloseConnections) { Network::MockListener* listener = new NiceMock(); Network::ListenerCallbacks* listener_callbacks; - EXPECT_CALL(dispatcher_, createListener_(_, _, _, _, _)) - .WillOnce(Invoke([&](Network::Socket&, Network::ListenerCallbacks& cb, bool, bool, - bool) -> Network::Listener* { - listener_callbacks = &cb; - return listener; - - })); - TestListener* test_listener = addListener(1, true, false, false, "test_listener"); + EXPECT_CALL(dispatcher_, createListener_(_, _, _, _)) + .WillOnce(Invoke( + [&](Network::Socket&, Network::ListenerCallbacks& cb, bool, bool) -> Network::Listener* { + listener_callbacks = &cb; + return listener; + + })); + TestListener* test_listener = addListener(1, true, false, "test_listener"); EXPECT_CALL(test_listener->socket_, localAddress()); handler_->addListener(*test_listener); @@ -150,14 +146,14 @@ TEST_F(ConnectionHandlerTest, CloseDuringFilterChainCreate) { Network::MockListener* listener = new Network::MockListener(); Network::ListenerCallbacks* listener_callbacks; - EXPECT_CALL(dispatcher_, createListener_(_, _, _, _, _)) - .WillOnce(Invoke([&](Network::Socket&, Network::ListenerCallbacks& cb, bool, bool, - bool) -> Network::Listener* { - listener_callbacks = &cb; - return listener; - - })); - TestListener* test_listener = addListener(1, true, false, false, "test_listener"); + EXPECT_CALL(dispatcher_, createListener_(_, _, _, _)) + .WillOnce(Invoke( + [&](Network::Socket&, Network::ListenerCallbacks& cb, bool, bool) -> Network::Listener* { + listener_callbacks = &cb; + return listener; + + })); + TestListener* test_listener = addListener(1, true, false, "test_listener"); EXPECT_CALL(test_listener->socket_, localAddress()); handler_->addListener(*test_listener); @@ -176,14 +172,14 @@ TEST_F(ConnectionHandlerTest, CloseConnectionOnEmptyFilterChain) { Network::MockListener* listener = new Network::MockListener(); Network::ListenerCallbacks* listener_callbacks; - EXPECT_CALL(dispatcher_, createListener_(_, _, _, _, _)) - .WillOnce(Invoke([&](Network::Socket&, Network::ListenerCallbacks& cb, bool, bool, - bool) -> Network::Listener* { - listener_callbacks = &cb; - return listener; - - })); - TestListener* test_listener = addListener(1, true, false, false, "test_listener"); + EXPECT_CALL(dispatcher_, createListener_(_, _, _, _)) + .WillOnce(Invoke( + [&](Network::Socket&, Network::ListenerCallbacks& cb, bool, bool) -> Network::Listener* { + listener_callbacks = &cb; + return listener; + + })); + TestListener* test_listener = addListener(1, true, false, "test_listener"); EXPECT_CALL(test_listener->socket_, localAddress()); handler_->addListener(*test_listener); @@ -197,28 +193,28 @@ TEST_F(ConnectionHandlerTest, CloseConnectionOnEmptyFilterChain) { } TEST_F(ConnectionHandlerTest, FindListenerByAddress) { - TestListener* test_listener1 = addListener(1, true, false, true, "test_listener1"); + TestListener* test_listener1 = addListener(1, true, true, "test_listener1"); Network::Address::InstanceConstSharedPtr alt_address( new Network::Address::Ipv4Instance("127.0.0.1", 10001)); Network::MockListener* listener = new Network::MockListener(); - EXPECT_CALL(dispatcher_, createListener_(_, _, _, _, true)) - .WillOnce(Invoke([&](Network::Socket&, Network::ListenerCallbacks&, bool, bool, + EXPECT_CALL(dispatcher_, createListener_(_, _, _, true)) + .WillOnce(Invoke([&](Network::Socket&, Network::ListenerCallbacks&, bool, bool) -> Network::Listener* { return listener; })); EXPECT_CALL(test_listener1->socket_, localAddress()).WillRepeatedly(ReturnRef(alt_address)); handler_->addListener(*test_listener1); EXPECT_EQ(listener, handler_->findListenerByAddress(ByRef(*alt_address))); - TestListener* test_listener2 = addListener(2, true, false, false, "test_listener2"); + TestListener* test_listener2 = addListener(2, true, false, "test_listener2"); Network::Address::InstanceConstSharedPtr alt_address2( new Network::Address::Ipv4Instance("0.0.0.0", 10001)); Network::Address::InstanceConstSharedPtr alt_address3( new Network::Address::Ipv4Instance("127.0.0.2", 10001)); Network::MockListener* listener2 = new Network::MockListener(); - EXPECT_CALL(dispatcher_, createListener_(_, _, _, _, false)) - .WillOnce(Invoke([&](Network::Socket&, Network::ListenerCallbacks&, bool, bool, + EXPECT_CALL(dispatcher_, createListener_(_, _, _, false)) + .WillOnce(Invoke([&](Network::Socket&, Network::ListenerCallbacks&, bool, bool) -> Network::Listener* { return listener2; })); EXPECT_CALL(test_listener2->socket_, localAddress()).WillRepeatedly(ReturnRef(alt_address2)); handler_->addListener(*test_listener2); @@ -235,8 +231,8 @@ TEST_F(ConnectionHandlerTest, FindListenerByAddress) { handler_->stopListeners(2); Network::MockListener* listener3 = new Network::MockListener(); - EXPECT_CALL(dispatcher_, createListener_(_, _, _, _, _)) - .WillOnce(Invoke([&](Network::Socket&, Network::ListenerCallbacks&, bool, bool, + EXPECT_CALL(dispatcher_, createListener_(_, _, _, _)) + .WillOnce(Invoke([&](Network::Socket&, Network::ListenerCallbacks&, bool, bool) -> Network::Listener* { return listener3; })); handler_->addListener(*test_listener2); @@ -247,29 +243,29 @@ TEST_F(ConnectionHandlerTest, FindListenerByAddress) { } TEST_F(ConnectionHandlerTest, NormalRedirect) { - TestListener* test_listener1 = addListener(1, true, false, true, "test_listener1"); + TestListener* test_listener1 = addListener(1, true, true, "test_listener1"); Network::MockListener* listener1 = new Network::MockListener(); Network::ListenerCallbacks* listener_callbacks1; - EXPECT_CALL(dispatcher_, createListener_(_, _, _, _, true)) - .WillOnce(Invoke([&](Network::Socket&, Network::ListenerCallbacks& cb, bool, bool, - bool) -> Network::Listener* { - listener_callbacks1 = &cb; - return listener1; - })); + EXPECT_CALL(dispatcher_, createListener_(_, _, _, true)) + .WillOnce(Invoke( + [&](Network::Socket&, Network::ListenerCallbacks& cb, bool, bool) -> Network::Listener* { + listener_callbacks1 = &cb; + return listener1; + })); Network::Address::InstanceConstSharedPtr normal_address( new Network::Address::Ipv4Instance("127.0.0.1", 10001)); EXPECT_CALL(test_listener1->socket_, localAddress()).WillRepeatedly(ReturnRef(normal_address)); handler_->addListener(*test_listener1); - TestListener* test_listener2 = addListener(1, false, false, false, "test_listener2"); + TestListener* test_listener2 = addListener(1, false, false, "test_listener2"); Network::MockListener* listener2 = new Network::MockListener(); Network::ListenerCallbacks* listener_callbacks2; - EXPECT_CALL(dispatcher_, createListener_(_, _, _, _, false)) - .WillOnce(Invoke([&](Network::Socket&, Network::ListenerCallbacks& cb, bool, bool, - bool) -> Network::Listener* { - listener_callbacks2 = &cb; - return listener2; - })); + EXPECT_CALL(dispatcher_, createListener_(_, _, _, false)) + .WillOnce(Invoke( + [&](Network::Socket&, Network::ListenerCallbacks& cb, bool, bool) -> Network::Listener* { + listener_callbacks2 = &cb; + return listener2; + })); Network::Address::InstanceConstSharedPtr alt_address( new Network::Address::Ipv4Instance("127.0.0.2", 20002)); EXPECT_CALL(test_listener2->socket_, localAddress()).WillRepeatedly(ReturnRef(alt_address)); @@ -306,29 +302,29 @@ TEST_F(ConnectionHandlerTest, NormalRedirect) { } TEST_F(ConnectionHandlerTest, FallbackToWildcardListener) { - TestListener* test_listener1 = addListener(1, true, false, true, "test_listener1"); + TestListener* test_listener1 = addListener(1, true, true, "test_listener1"); Network::MockListener* listener1 = new Network::MockListener(); Network::ListenerCallbacks* listener_callbacks1; - EXPECT_CALL(dispatcher_, createListener_(_, _, _, _, true)) - .WillOnce(Invoke([&](Network::Socket&, Network::ListenerCallbacks& cb, bool, bool, - bool) -> Network::Listener* { - listener_callbacks1 = &cb; - return listener1; - })); + EXPECT_CALL(dispatcher_, createListener_(_, _, _, true)) + .WillOnce(Invoke( + [&](Network::Socket&, Network::ListenerCallbacks& cb, bool, bool) -> Network::Listener* { + listener_callbacks1 = &cb; + return listener1; + })); Network::Address::InstanceConstSharedPtr normal_address( new Network::Address::Ipv4Instance("127.0.0.1", 10001)); EXPECT_CALL(test_listener1->socket_, localAddress()).WillRepeatedly(ReturnRef(normal_address)); handler_->addListener(*test_listener1); - TestListener* test_listener2 = addListener(1, false, false, false, "test_listener2"); + TestListener* test_listener2 = addListener(1, false, false, "test_listener2"); Network::MockListener* listener2 = new Network::MockListener(); Network::ListenerCallbacks* listener_callbacks2; - EXPECT_CALL(dispatcher_, createListener_(_, _, _, _, false)) - .WillOnce(Invoke([&](Network::Socket&, Network::ListenerCallbacks& cb, bool, bool, - bool) -> Network::Listener* { - listener_callbacks2 = &cb; - return listener2; - })); + EXPECT_CALL(dispatcher_, createListener_(_, _, _, false)) + .WillOnce(Invoke( + [&](Network::Socket&, Network::ListenerCallbacks& cb, bool, bool) -> Network::Listener* { + listener_callbacks2 = &cb; + return listener2; + })); Network::Address::InstanceConstSharedPtr any_address = Network::Utility::getIpv4AnyAddress(); EXPECT_CALL(test_listener2->socket_, localAddress()).WillRepeatedly(ReturnRef(any_address)); handler_->addListener(*test_listener2); @@ -367,15 +363,15 @@ TEST_F(ConnectionHandlerTest, FallbackToWildcardListener) { } TEST_F(ConnectionHandlerTest, WildcardListenerWithOriginalDst) { - TestListener* test_listener1 = addListener(1, true, false, true, "test_listener1"); + TestListener* test_listener1 = addListener(1, true, true, "test_listener1"); Network::MockListener* listener1 = new Network::MockListener(); Network::ListenerCallbacks* listener_callbacks1; - EXPECT_CALL(dispatcher_, createListener_(_, _, _, _, true)) - .WillOnce(Invoke([&](Network::Socket&, Network::ListenerCallbacks& cb, bool, bool, - bool) -> Network::Listener* { - listener_callbacks1 = &cb; - return listener1; - })); + EXPECT_CALL(dispatcher_, createListener_(_, _, _, true)) + .WillOnce(Invoke( + [&](Network::Socket&, Network::ListenerCallbacks& cb, bool, bool) -> Network::Listener* { + listener_callbacks1 = &cb; + return listener1; + })); Network::Address::InstanceConstSharedPtr normal_address( new Network::Address::Ipv4Instance("127.0.0.1", 80)); // Original dst address nor port number match that of the listener's address. @@ -412,15 +408,15 @@ TEST_F(ConnectionHandlerTest, WildcardListenerWithOriginalDst) { } TEST_F(ConnectionHandlerTest, WildcardListenerWithNoOriginalDst) { - TestListener* test_listener1 = addListener(1, true, false, true, "test_listener1"); + TestListener* test_listener1 = addListener(1, true, true, "test_listener1"); Network::MockListener* listener1 = new Network::MockListener(); Network::ListenerCallbacks* listener_callbacks1; - EXPECT_CALL(dispatcher_, createListener_(_, _, _, _, true)) - .WillOnce(Invoke([&](Network::Socket&, Network::ListenerCallbacks& cb, bool, bool, - bool) -> Network::Listener* { - listener_callbacks1 = &cb; - return listener1; - })); + EXPECT_CALL(dispatcher_, createListener_(_, _, _, true)) + .WillOnce(Invoke( + [&](Network::Socket&, Network::ListenerCallbacks& cb, bool, bool) -> Network::Listener* { + listener_callbacks1 = &cb; + return listener1; + })); Network::Address::InstanceConstSharedPtr normal_address( new Network::Address::Ipv4Instance("127.0.0.1", 80)); Network::Address::InstanceConstSharedPtr any_address = Network::Utility::getAddressWithPort( From 7cc376821763a4037a8ab4f93446adf5215250f2 Mon Sep 17 00:00:00 2001 From: Greg Greenway Date: Tue, 3 Apr 2018 10:06:22 -0700 Subject: [PATCH 03/31] Fix crash when no options present Signed-off-by: Greg Greenway --- source/common/network/listener_impl.cc | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/source/common/network/listener_impl.cc b/source/common/network/listener_impl.cc index ec5fadd892d39..e6b0ba505f7fe 100644 --- a/source/common/network/listener_impl.cc +++ b/source/common/network/listener_impl.cc @@ -67,11 +67,14 @@ ListenerImpl::ListenerImpl(Event::DispatcherImpl& dispatcher, Socket& socket, Li fmt::format("cannot listen on socket: {}", socket.localAddress()->asString())); } - for (auto& option : *socket.options()) { - if (!option->setOption(socket, Socket::SocketState::Listening)) { - throw CreateListenerException( - fmt::format("cannot set post-listen socket option on socket: {}", - socket.localAddress()->asString())); + Socket::OptionsSharedPtr options = socket.options(); + if (options != nullptr) { + for (auto& option : *options) { + if (!option->setOption(socket, Socket::SocketState::Listening)) { + throw CreateListenerException( + fmt::format("cannot set post-listen socket option on socket: {}", + socket.localAddress()->asString())); + } } } From 8dcf11d2a1c31c95f545e786ba1edad150f8e34d Mon Sep 17 00:00:00 2001 From: Greg Greenway Date: Tue, 3 Apr 2018 10:06:48 -0700 Subject: [PATCH 04/31] Don't set TRANSPARENT option in state Listening Signed-off-by: Greg Greenway --- source/common/network/socket_option_impl.cc | 23 +++++++++++++-------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/source/common/network/socket_option_impl.cc b/source/common/network/socket_option_impl.cc index 1dac8d9d6f5e2..7dd4436fdbbe7 100644 --- a/source/common/network/socket_option_impl.cc +++ b/source/common/network/socket_option_impl.cc @@ -10,14 +10,16 @@ namespace Envoy { namespace Network { bool SocketOptionImpl::setOption(Socket& socket, Socket::SocketState state) const { - if (transparent_.has_value()) { - const int should_transparent = transparent_.value() ? 1 : 0; - const int error = - setIpSocketOption(socket, ENVOY_SOCKET_IP_TRANSPARENT, ENVOY_SOCKET_IPV6_TRANSPARENT, - &should_transparent, sizeof(should_transparent)); - if (error != 0) { - ENVOY_LOG(warn, "Setting IP_TRANSPARENT on listener socket failed: {}", strerror(error)); - return false; + if (state == Socket::SocketState::PreBind || state == Socket::SocketState::PostBind) { + if (transparent_.has_value()) { + const int should_transparent = transparent_.value() ? 1 : 0; + const int error = + setIpSocketOption(socket, ENVOY_SOCKET_IP_TRANSPARENT, ENVOY_SOCKET_IPV6_TRANSPARENT, + &should_transparent, sizeof(should_transparent)); + if (error != 0) { + ENVOY_LOG(warn, "Setting IP_TRANSPARENT on listener socket failed: {}", strerror(error)); + return false; + } } } @@ -32,7 +34,9 @@ bool SocketOptionImpl::setOption(Socket& socket, Socket::SocketState state) cons return false; } } - } else if (state == Socket::SocketState::Listening) { + } + + if (state == Socket::SocketState::Listening) { if (accept_tcp_fast_open_.has_value()) { const int tfo_value = accept_tcp_fast_open_.value() ? ENVOY_TCP_FASTOPEN_BACKLOG : 0; const SocketOptionName option_name = ENVOY_SOCKET_TCP_FASTOPEN; @@ -51,6 +55,7 @@ bool SocketOptionImpl::setOption(Socket& socket, Socket::SocketState state) cons } } } + return true; } From 6f55c8430103843bdaa33e87e9a04409ff0f80a4 Mon Sep 17 00:00:00 2001 From: Greg Greenway Date: Tue, 3 Apr 2018 10:07:18 -0700 Subject: [PATCH 05/31] Add more test coverage Signed-off-by: Greg Greenway --- .../common/network/socket_option_impl_test.cc | 70 +++++++++++++------ 1 file changed, 47 insertions(+), 23 deletions(-) diff --git a/test/common/network/socket_option_impl_test.cc b/test/common/network/socket_option_impl_test.cc index fa2cc4c057e80..c7100ece906c5 100644 --- a/test/common/network/socket_option_impl_test.cc +++ b/test/common/network/socket_option_impl_test.cc @@ -26,19 +26,37 @@ class SocketOptionImplTest : public testing::Test { void testSetSocketOptionSuccess(SocketOptionImpl& socket_option, int socket_level, Network::SocketOptionName option_name, int option_val, - Socket::SocketState when) { - if (option_name.has_value()) { - Address::Ipv4Instance address("1.2.3.4", 5678); - const int fd = address.socket(Address::SocketType::Stream); - EXPECT_CALL(socket_, fd()).WillRepeatedly(Return(fd)); - EXPECT_CALL(os_sys_calls_, setsockopt_(_, socket_level, option_name.value(), _, sizeof(int))) - .WillOnce(Invoke([option_val](int, int, int, const void* optval, socklen_t) -> int { - EXPECT_EQ(option_val, *static_cast(optval)); - return 0; - })); - EXPECT_TRUE(socket_option.setOption(socket_, when)); - } else { - EXPECT_FALSE(socket_option.setOption(socket_, when)); + const std::set& when) { + Address::Ipv4Instance address("1.2.3.4", 5678); + const int fd = address.socket(Address::SocketType::Stream); + EXPECT_CALL(socket_, fd()).WillRepeatedly(Return(fd)); + + for (Socket::SocketState state : when) { + if (option_name.has_value()) { + EXPECT_CALL(os_sys_calls_, + setsockopt_(_, socket_level, option_name.value(), _, sizeof(int))) + .WillOnce(Invoke([option_val](int, int, int, const void* optval, socklen_t) -> int { + EXPECT_EQ(option_val, *static_cast(optval)); + return 0; + })); + EXPECT_TRUE(socket_option.setOption(socket_, state)); + } else { + EXPECT_FALSE(socket_option.setOption(socket_, state)); + } + } + + // The set of SocketState for which this option should not be set. Initialize to all + // the states, and remove states that are passed in. + std::list unset_socketstates{ + Socket::SocketState::PreBind, + Socket::SocketState::PostBind, + Socket::SocketState::Listening, + }; + unset_socketstates.remove_if( + [&](Socket::SocketState state) -> bool { return when.find(state) != when.end(); }); + for (Socket::SocketState state : unset_socketstates) { + EXPECT_CALL(os_sys_calls_, setsockopt_(_, _, _, _, _)).Times(0); + EXPECT_TRUE(socket_option.setOption(socket_, state)); } } }; @@ -79,36 +97,42 @@ TEST_F(SocketOptionImplTest, SetOptionTcpFastopenFailure) { TEST_F(SocketOptionImplTest, SetOptionTransparentSuccessTrue) { SocketOptionImpl socket_option{true, {}, {}}; testSetSocketOptionSuccess(socket_option, IPPROTO_IP, ENVOY_SOCKET_IP_TRANSPARENT, 1, - Socket::SocketState::PreBind); + {Socket::SocketState::PreBind, Socket::SocketState::PostBind}); } // The happy path for setOption(); IP_FREEBIND is set to true. TEST_F(SocketOptionImplTest, SetOptionFreebindSuccessTrue) { SocketOptionImpl socket_option{{}, true, {}}; testSetSocketOptionSuccess(socket_option, IPPROTO_IP, ENVOY_SOCKET_IP_FREEBIND, 1, - Socket::SocketState::PreBind); + {Socket::SocketState::PreBind}); +} + +// The happy path for setOption(); TCP_FASTOPEN is set to true. +TEST_F(SocketOptionImplTest, SetOptionTcpFastopenSuccessTrue) { + SocketOptionImpl socket_option{{}, {}, true}; + testSetSocketOptionSuccess(socket_option, IPPROTO_TCP, ENVOY_SOCKET_TCP_FASTOPEN, 1, + {Socket::SocketState::Listening}); } // The happy path for setOpion(); IP_TRANSPARENT is set to false. TEST_F(SocketOptionImplTest, SetOptionTransparentSuccessFalse) { SocketOptionImpl socket_option{false, {}, {}}; testSetSocketOptionSuccess(socket_option, IPPROTO_IP, ENVOY_SOCKET_IP_TRANSPARENT, 0, - Socket::SocketState::PreBind); - testSetSocketOptionSuccess(socket_option, IPPROTO_IP, ENVOY_SOCKET_IP_TRANSPARENT, 0, - Socket::SocketState::PostBind); + {Socket::SocketState::PreBind, Socket::SocketState::PostBind}); } // The happy path for setOpion(); IP_FREEBIND is set to false. TEST_F(SocketOptionImplTest, SetOptionFreebindSuccessFalse) { SocketOptionImpl socket_option{{}, false, {}}; testSetSocketOptionSuccess(socket_option, IPPROTO_IP, ENVOY_SOCKET_IP_FREEBIND, 0, - Socket::SocketState::PreBind); + {Socket::SocketState::PreBind}); } -// Freebind settings have no effect on post-bind behavior. -TEST_F(SocketOptionImplTest, SetOptionFreebindPostBind) { - SocketOptionImpl socket_option{{}, true, {}}; - EXPECT_TRUE(socket_option.setOption(socket_, Socket::SocketState::PostBind)); +// The happy path for setOpion(); TCP_FASTOPEN is set to false. +TEST_F(SocketOptionImplTest, SetOptionTcpFastopenSuccessFalse) { + SocketOptionImpl socket_option{{}, {}, false}; + testSetSocketOptionSuccess(socket_option, IPPROTO_IP, ENVOY_SOCKET_TCP_FASTOPEN, 0, + {Socket::SocketState::Listening}); } // If a platform doesn't suppport IPv4 socket option variant for an IPv4 address, we fail From dea8e2c6c2db8af0325606ee69367407459f0ba9 Mon Sep 17 00:00:00 2001 From: Greg Greenway Date: Tue, 3 Apr 2018 10:46:38 -0700 Subject: [PATCH 06/31] Make the option tunable, not just boolean Signed-off-by: Greg Greenway --- bazel/repository_locations.bzl | 2 +- source/common/network/socket_option_impl.cc | 4 ++-- source/common/network/socket_option_impl.h | 16 +++------------- source/server/listener_manager_impl.cc | 4 ++-- test/common/network/socket_option_impl_test.cc | 8 ++++---- 5 files changed, 12 insertions(+), 22 deletions(-) diff --git a/bazel/repository_locations.bzl b/bazel/repository_locations.bzl index 5b130b4908ea2..4872bfa4da86a 100644 --- a/bazel/repository_locations.bzl +++ b/bazel/repository_locations.bzl @@ -76,7 +76,7 @@ REPOSITORY_LOCATIONS = dict( urls = ["https://github.com/google/protobuf/archive/v3.5.0.tar.gz"], ), envoy_api = dict( - commit = "28c23f754539369c57b28359067f100059ce0d0b", + commit = "55f78650483140f8dbce89d6b215e85d52aaefba", remote = "https://github.com/bmetzdorf/data-plane-api.git", # TODO FIXME ), grpc_httpjson_transcoding = dict( diff --git a/source/common/network/socket_option_impl.cc b/source/common/network/socket_option_impl.cc index 7dd4436fdbbe7..eb98ceb61c360 100644 --- a/source/common/network/socket_option_impl.cc +++ b/source/common/network/socket_option_impl.cc @@ -37,8 +37,8 @@ bool SocketOptionImpl::setOption(Socket& socket, Socket::SocketState state) cons } if (state == Socket::SocketState::Listening) { - if (accept_tcp_fast_open_.has_value()) { - const int tfo_value = accept_tcp_fast_open_.value() ? ENVOY_TCP_FASTOPEN_BACKLOG : 0; + if (tcp_fast_open_queue_length_.has_value()) { + const int tfo_value = tcp_fast_open_queue_length_.value(); const SocketOptionName option_name = ENVOY_SOCKET_TCP_FASTOPEN; int error = -1; if (option_name) { diff --git a/source/common/network/socket_option_impl.h b/source/common/network/socket_option_impl.h index 6694bbb6c294a..16adc6d2ddca7 100644 --- a/source/common/network/socket_option_impl.h +++ b/source/common/network/socket_option_impl.h @@ -47,22 +47,12 @@ typedef absl::optional SocketOptionName; #define ENVOY_SOCKET_TCP_FASTOPEN Network::SocketOptionName() #endif -// On macOS the socket MUST be listening already for TCP_FASTOPEN to be set and backlog MUST be 1 -// (the actual value is set via the net.inet.tcp.fastopen_backlog kernel parameter. -// For Linux we default to 128, which libevent is using in -// https://github.com/libevent/libevent/blob/release-2.1.8-stable/listener.c#L176 -#if defined(__APPLE__) -#define ENVOY_TCP_FASTOPEN_BACKLOG 1 -#else -#define ENVOY_TCP_FASTOPEN_BACKLOG 128 -#endif - class SocketOptionImpl : public Socket::Option, Logger::Loggable { public: SocketOptionImpl(absl::optional transparent, absl::optional freebind, - absl::optional accept_tcp_fast_open) + absl::optional tcp_fast_open_queue_length) : transparent_(transparent), freebind_(freebind), - accept_tcp_fast_open_(accept_tcp_fast_open) {} + tcp_fast_open_queue_length_(tcp_fast_open_queue_length) {} // Socket::Option bool setOption(Socket& socket, Socket::SocketState state) const override; @@ -90,7 +80,7 @@ class SocketOptionImpl : public Socket::Option, Logger::Loggable transparent_; const absl::optional freebind_; - const absl::optional accept_tcp_fast_open_; + const absl::optional tcp_fast_open_queue_length_; }; } // namespace Network diff --git a/source/server/listener_manager_impl.cc b/source/server/listener_manager_impl.cc index 5faa39ded0d47..a23ea88839751 100644 --- a/source/server/listener_manager_impl.cc +++ b/source/server/listener_manager_impl.cc @@ -115,8 +115,8 @@ class ListenerSocketOption : public Network::SocketOptionImpl { : Network::SocketOptionImpl( PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, transparent, absl::optional{}), PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, freebind, absl::optional{}), - PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, accept_tcp_fast_open, absl::optional{})) { - } + PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, tcp_fast_open_queue_length, + absl::optional{})) {} }; ListenerImpl::ListenerImpl(const envoy::api::v2::Listener& config, ListenerManagerImpl& parent, diff --git a/test/common/network/socket_option_impl_test.cc b/test/common/network/socket_option_impl_test.cc index c7100ece906c5..69671f1a8d5f9 100644 --- a/test/common/network/socket_option_impl_test.cc +++ b/test/common/network/socket_option_impl_test.cc @@ -89,7 +89,7 @@ TEST_F(SocketOptionImplTest, SetOptionFreebindFailure) { // We fail to set the tcp-fastopen option when the underlying setsockopt syscall fails. TEST_F(SocketOptionImplTest, SetOptionTcpFastopenFailure) { - SocketOptionImpl socket_option{{}, {}, true}; + SocketOptionImpl socket_option{{}, {}, 1}; EXPECT_FALSE(socket_option.setOption(socket_, Socket::SocketState::Listening)); } @@ -109,8 +109,8 @@ TEST_F(SocketOptionImplTest, SetOptionFreebindSuccessTrue) { // The happy path for setOption(); TCP_FASTOPEN is set to true. TEST_F(SocketOptionImplTest, SetOptionTcpFastopenSuccessTrue) { - SocketOptionImpl socket_option{{}, {}, true}; - testSetSocketOptionSuccess(socket_option, IPPROTO_TCP, ENVOY_SOCKET_TCP_FASTOPEN, 1, + SocketOptionImpl socket_option{{}, {}, 42}; + testSetSocketOptionSuccess(socket_option, IPPROTO_TCP, ENVOY_SOCKET_TCP_FASTOPEN, 42, {Socket::SocketState::Listening}); } @@ -130,7 +130,7 @@ TEST_F(SocketOptionImplTest, SetOptionFreebindSuccessFalse) { // The happy path for setOpion(); TCP_FASTOPEN is set to false. TEST_F(SocketOptionImplTest, SetOptionTcpFastopenSuccessFalse) { - SocketOptionImpl socket_option{{}, {}, false}; + SocketOptionImpl socket_option{{}, {}, 0}; testSetSocketOptionSuccess(socket_option, IPPROTO_IP, ENVOY_SOCKET_TCP_FASTOPEN, 0, {Socket::SocketState::Listening}); } From cc211dd24b057fa243655961ddc524c6b330296a Mon Sep 17 00:00:00 2001 From: Greg Greenway Date: Tue, 3 Apr 2018 11:18:50 -0700 Subject: [PATCH 07/31] Add test for setting Listening socket options Signed-off-by: Greg Greenway --- test/common/network/listener_impl_test.cc | 30 +++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/test/common/network/listener_impl_test.cc b/test/common/network/listener_impl_test.cc index 5d18d968826d0..48d5c777c879f 100644 --- a/test/common/network/listener_impl_test.cc +++ b/test/common/network/listener_impl_test.cc @@ -12,6 +12,7 @@ #include "gmock/gmock.h" #include "gtest/gtest.h" +using testing::AtLeast; using testing::Invoke; using testing::Return; using testing::_; @@ -86,6 +87,35 @@ INSTANTIATE_TEST_CASE_P(IpVersions, ListenerImplTest, testing::ValuesIn(TestEnvironment::getIpVersionsForTest()), TestUtility::ipTestParamsToString); +// Test that socket options are set after the listener is setup. +TEST_P(ListenerImplTest, SetListeningSocketOptionsSuccess) { + Event::DispatcherImpl dispatcher; + Network::MockListenerCallbacks listener_callbacks; + Network::MockConnectionHandler connection_handler; + + Network::TcpListenSocket socket(Network::Test::getCanonicalLoopbackAddress(version_), nullptr, + true); + std::shared_ptr option = std::make_shared(); + socket.addOption(option); + EXPECT_CALL(*option, setOption(_, Socket::SocketState::Listening)).WillOnce(Return(true)); + TestListenerImpl listener(dispatcher, socket, listener_callbacks, true, false); +} + +// Test that an exception is thrown if there is an error setting socket options. +TEST_P(ListenerImplTest, SetListeningSocketOptionsError) { + Event::DispatcherImpl dispatcher; + Network::MockListenerCallbacks listener_callbacks; + Network::MockConnectionHandler connection_handler; + + Network::TcpListenSocket socket(Network::Test::getCanonicalLoopbackAddress(version_), nullptr, + true); + std::shared_ptr option = std::make_shared(); + socket.addOption(option); + EXPECT_CALL(*option, setOption(_, Socket::SocketState::Listening)).WillOnce(Return(false)); + EXPECT_THROW(TestListenerImpl(dispatcher, socket, listener_callbacks, true, false), + CreateListenerException); +} + TEST_P(ListenerImplTest, UseActualDst) { Stats::IsolatedStoreImpl stats_store; Event::DispatcherImpl dispatcher; From d9ba97a2400c8e5ebff128e4a143732f61039741 Mon Sep 17 00:00:00 2001 From: Greg Greenway Date: Tue, 3 Apr 2018 15:45:56 -0700 Subject: [PATCH 08/31] Fixes Signed-off-by: Greg Greenway --- source/common/network/listener_impl.cc | 1 - source/common/network/socket_option_impl.cc | 16 ++++++++-------- source/common/network/socket_option_impl.h | 1 + source/server/listener_manager_impl.cc | 4 ++++ test/common/network/socket_option_impl_test.cc | 8 ++++++-- 5 files changed, 19 insertions(+), 11 deletions(-) diff --git a/source/common/network/listener_impl.cc b/source/common/network/listener_impl.cc index e6b0ba505f7fe..e3f67296cd7b7 100644 --- a/source/common/network/listener_impl.cc +++ b/source/common/network/listener_impl.cc @@ -1,6 +1,5 @@ #include "common/network/listener_impl.h" -#include #include #include "envoy/common/exception.h" diff --git a/source/common/network/socket_option_impl.cc b/source/common/network/socket_option_impl.cc index eb98ceb61c360..f3c20ec0fed18 100644 --- a/source/common/network/socket_option_impl.cc +++ b/source/common/network/socket_option_impl.cc @@ -40,18 +40,18 @@ bool SocketOptionImpl::setOption(Socket& socket, Socket::SocketState state) cons if (tcp_fast_open_queue_length_.has_value()) { const int tfo_value = tcp_fast_open_queue_length_.value(); const SocketOptionName option_name = ENVOY_SOCKET_TCP_FASTOPEN; - int error = -1; if (option_name) { - error = Api::OsSysCallsSingleton::get().setsockopt( + const int error = Api::OsSysCallsSingleton::get().setsockopt( socket.fd(), IPPROTO_TCP, option_name.value(), reinterpret_cast(&tfo_value), sizeof(tfo_value)); + if (error != 0) { + ENVOY_LOG(warn, "Setting TCP_FASTOPEN on listener socket failed: {}", strerror(errno)); + return false; + } else { + ENVOY_LOG(debug, "Successfully set socket option TCP_FASTOPEN to {}", tfo_value); + } } else { - error = ENOTSUP; - } - - if (error != 0) { - ENVOY_LOG(warn, "Setting IP_TCP_FASTOPEN on listener socket failed: {}", strerror(error)); - return false; + ENVOY_LOG(warn, "Unsupported socket option TCP_FASTOPEN"); } } } diff --git a/source/common/network/socket_option_impl.h b/source/common/network/socket_option_impl.h index 16adc6d2ddca7..84d47875124e5 100644 --- a/source/common/network/socket_option_impl.h +++ b/source/common/network/socket_option_impl.h @@ -2,6 +2,7 @@ #include #include +#include #include #include "envoy/network/listen_socket.h" diff --git a/source/server/listener_manager_impl.cc b/source/server/listener_manager_impl.cc index a23ea88839751..aa2b6c8b6421f 100644 --- a/source/server/listener_manager_impl.cc +++ b/source/server/listener_manager_impl.cc @@ -304,6 +304,10 @@ void ListenerImpl::setSocket(const Network::SocketSharedPtr& socket) { } else { ENVOY_LOG(debug, "{}", message); } + + // Add the options to the socket_ so that SocketState::Listening options can be + // set in the worker after listen()/evconnlistener_new() is called. + socket_->addOption(option); } } } diff --git a/test/common/network/socket_option_impl_test.cc b/test/common/network/socket_option_impl_test.cc index 69671f1a8d5f9..e51b9472eb5ac 100644 --- a/test/common/network/socket_option_impl_test.cc +++ b/test/common/network/socket_option_impl_test.cc @@ -89,8 +89,12 @@ TEST_F(SocketOptionImplTest, SetOptionFreebindFailure) { // We fail to set the tcp-fastopen option when the underlying setsockopt syscall fails. TEST_F(SocketOptionImplTest, SetOptionTcpFastopenFailure) { - SocketOptionImpl socket_option{{}, {}, 1}; - EXPECT_FALSE(socket_option.setOption(socket_, Socket::SocketState::Listening)); + if (ENVOY_SOCKET_TCP_FASTOPEN.has_value()) { + SocketOptionImpl socket_option{{}, {}, 1}; + EXPECT_CALL(os_sys_calls_, setsockopt_(_, IPPROTO_TCP, ENVOY_SOCKET_TCP_FASTOPEN.value(), _, _)) + .WillOnce(Return(-1)); + EXPECT_FALSE(socket_option.setOption(socket_, Socket::SocketState::Listening)); + } } // The happy path for setOption(); IP_TRANSPARENT is set to true. From e56a8d775b83ff746678397d904ba1540e7d7389 Mon Sep 17 00:00:00 2001 From: Greg Greenway Date: Tue, 3 Apr 2018 15:47:45 -0700 Subject: [PATCH 09/31] test typo Signed-off-by: Greg Greenway --- test/common/network/socket_option_impl_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/common/network/socket_option_impl_test.cc b/test/common/network/socket_option_impl_test.cc index e51b9472eb5ac..870a7ba31c78c 100644 --- a/test/common/network/socket_option_impl_test.cc +++ b/test/common/network/socket_option_impl_test.cc @@ -135,7 +135,7 @@ TEST_F(SocketOptionImplTest, SetOptionFreebindSuccessFalse) { // The happy path for setOpion(); TCP_FASTOPEN is set to false. TEST_F(SocketOptionImplTest, SetOptionTcpFastopenSuccessFalse) { SocketOptionImpl socket_option{{}, {}, 0}; - testSetSocketOptionSuccess(socket_option, IPPROTO_IP, ENVOY_SOCKET_TCP_FASTOPEN, 0, + testSetSocketOptionSuccess(socket_option, IPPROTO_TCP, ENVOY_SOCKET_TCP_FASTOPEN, 0, {Socket::SocketState::Listening}); } From f2db8640b2e576a4c978f461f936cd6dfeaf43cf Mon Sep 17 00:00:00 2001 From: Greg Greenway Date: Tue, 3 Apr 2018 15:50:41 -0700 Subject: [PATCH 10/31] Fix misspelled comments Signed-off-by: Greg Greenway --- test/common/network/socket_option_impl_test.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/common/network/socket_option_impl_test.cc b/test/common/network/socket_option_impl_test.cc index 870a7ba31c78c..d83513ba3cb9a 100644 --- a/test/common/network/socket_option_impl_test.cc +++ b/test/common/network/socket_option_impl_test.cc @@ -118,14 +118,14 @@ TEST_F(SocketOptionImplTest, SetOptionTcpFastopenSuccessTrue) { {Socket::SocketState::Listening}); } -// The happy path for setOpion(); IP_TRANSPARENT is set to false. +// The happy path for setOption(); IP_TRANSPARENT is set to false. TEST_F(SocketOptionImplTest, SetOptionTransparentSuccessFalse) { SocketOptionImpl socket_option{false, {}, {}}; testSetSocketOptionSuccess(socket_option, IPPROTO_IP, ENVOY_SOCKET_IP_TRANSPARENT, 0, {Socket::SocketState::PreBind, Socket::SocketState::PostBind}); } -// The happy path for setOpion(); IP_FREEBIND is set to false. +// The happy path for setOption(); IP_FREEBIND is set to false. TEST_F(SocketOptionImplTest, SetOptionFreebindSuccessFalse) { SocketOptionImpl socket_option{{}, false, {}}; testSetSocketOptionSuccess(socket_option, IPPROTO_IP, ENVOY_SOCKET_IP_FREEBIND, 0, From 9f3c97ea24e521c432487fef895b6c8be79ae4d5 Mon Sep 17 00:00:00 2001 From: Greg Greenway Date: Wed, 4 Apr 2018 08:36:43 -0700 Subject: [PATCH 11/31] one more misspelled comment Signed-off-by: Greg Greenway --- test/common/network/socket_option_impl_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/common/network/socket_option_impl_test.cc b/test/common/network/socket_option_impl_test.cc index d83513ba3cb9a..466c6bfaab15c 100644 --- a/test/common/network/socket_option_impl_test.cc +++ b/test/common/network/socket_option_impl_test.cc @@ -132,7 +132,7 @@ TEST_F(SocketOptionImplTest, SetOptionFreebindSuccessFalse) { {Socket::SocketState::PreBind}); } -// The happy path for setOpion(); TCP_FASTOPEN is set to false. +// The happy path for setOption(); TCP_FASTOPEN is set to false. TEST_F(SocketOptionImplTest, SetOptionTcpFastopenSuccessFalse) { SocketOptionImpl socket_option{{}, {}, 0}; testSetSocketOptionSuccess(socket_option, IPPROTO_TCP, ENVOY_SOCKET_TCP_FASTOPEN, 0, From d2d4a2706d92ceebd1239898bcd7d7331eeba8a8 Mon Sep 17 00:00:00 2001 From: Bjoern Metzdorf Date: Thu, 5 Apr 2018 10:51:32 -0700 Subject: [PATCH 12/31] Updated data-plane-api commit Signed-off-by: Bjoern Metzdorf --- bazel/repository_locations.bzl | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/bazel/repository_locations.bzl b/bazel/repository_locations.bzl index 4872bfa4da86a..5a00c913ef004 100644 --- a/bazel/repository_locations.bzl +++ b/bazel/repository_locations.bzl @@ -76,8 +76,8 @@ REPOSITORY_LOCATIONS = dict( urls = ["https://github.com/google/protobuf/archive/v3.5.0.tar.gz"], ), envoy_api = dict( - commit = "55f78650483140f8dbce89d6b215e85d52aaefba", - remote = "https://github.com/bmetzdorf/data-plane-api.git", # TODO FIXME + commit = "f9f5c41a02c3003d76c4a6f14fe77cecb6fc7b56", + remote = "https://github.com/envoyproxy/data-plane-api.git", ), grpc_httpjson_transcoding = dict( commit = "e4f58aa07b9002befa493a0a82e10f2e98b51fc6", From a03533ab750ebf4ffc7f3a23d531cd072c405305 Mon Sep 17 00:00:00 2001 From: Bjoern Metzdorf Date: Thu, 5 Apr 2018 10:53:18 -0700 Subject: [PATCH 13/31] fix typo Signed-off-by: Bjoern Metzdorf --- bazel/repository_locations.bzl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bazel/repository_locations.bzl b/bazel/repository_locations.bzl index 5a00c913ef004..245d95c4f2da8 100644 --- a/bazel/repository_locations.bzl +++ b/bazel/repository_locations.bzl @@ -77,7 +77,7 @@ REPOSITORY_LOCATIONS = dict( ), envoy_api = dict( commit = "f9f5c41a02c3003d76c4a6f14fe77cecb6fc7b56", - remote = "https://github.com/envoyproxy/data-plane-api.git", + remote = "https://github.com/envoyproxy/data-plane-api", ), grpc_httpjson_transcoding = dict( commit = "e4f58aa07b9002befa493a0a82e10f2e98b51fc6", From 8289daaa5a8e675a68d75e524206f000d2690032 Mon Sep 17 00:00:00 2001 From: Bjoern Metzdorf Date: Thu, 5 Apr 2018 14:42:45 -0700 Subject: [PATCH 14/31] set listening socket options in ListenerSocketOption Signed-off-by: Bjoern Metzdorf --- source/common/network/socket_option_impl.cc | 20 ------------ source/common/network/socket_option_impl.h | 9 ++---- source/common/upstream/upstream_impl.cc | 8 ++--- source/server/listener_manager_impl.cc | 34 +++++++++++++++++++-- 4 files changed, 37 insertions(+), 34 deletions(-) diff --git a/source/common/network/socket_option_impl.cc b/source/common/network/socket_option_impl.cc index f3c20ec0fed18..e969725cefe1b 100644 --- a/source/common/network/socket_option_impl.cc +++ b/source/common/network/socket_option_impl.cc @@ -36,26 +36,6 @@ bool SocketOptionImpl::setOption(Socket& socket, Socket::SocketState state) cons } } - if (state == Socket::SocketState::Listening) { - if (tcp_fast_open_queue_length_.has_value()) { - const int tfo_value = tcp_fast_open_queue_length_.value(); - const SocketOptionName option_name = ENVOY_SOCKET_TCP_FASTOPEN; - if (option_name) { - const int error = Api::OsSysCallsSingleton::get().setsockopt( - socket.fd(), IPPROTO_TCP, option_name.value(), - reinterpret_cast(&tfo_value), sizeof(tfo_value)); - if (error != 0) { - ENVOY_LOG(warn, "Setting TCP_FASTOPEN on listener socket failed: {}", strerror(errno)); - return false; - } else { - ENVOY_LOG(debug, "Successfully set socket option TCP_FASTOPEN to {}", tfo_value); - } - } else { - ENVOY_LOG(warn, "Unsupported socket option TCP_FASTOPEN"); - } - } - } - return true; } diff --git a/source/common/network/socket_option_impl.h b/source/common/network/socket_option_impl.h index 84d47875124e5..945d84809a1d6 100644 --- a/source/common/network/socket_option_impl.h +++ b/source/common/network/socket_option_impl.h @@ -48,12 +48,10 @@ typedef absl::optional SocketOptionName; #define ENVOY_SOCKET_TCP_FASTOPEN Network::SocketOptionName() #endif -class SocketOptionImpl : public Socket::Option, Logger::Loggable { +class SocketOptionImpl : public Socket::Option, protected Logger::Loggable { public: - SocketOptionImpl(absl::optional transparent, absl::optional freebind, - absl::optional tcp_fast_open_queue_length) - : transparent_(transparent), freebind_(freebind), - tcp_fast_open_queue_length_(tcp_fast_open_queue_length) {} + SocketOptionImpl(absl::optional transparent, absl::optional freebind) + : transparent_(transparent), freebind_(freebind) {} // Socket::Option bool setOption(Socket& socket, Socket::SocketState state) const override; @@ -81,7 +79,6 @@ class SocketOptionImpl : public Socket::Option, Logger::Loggable transparent_; const absl::optional freebind_; - const absl::optional tcp_fast_open_queue_length_; }; } // namespace Network diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index 4b3587f9b4e44..7d35dbe27fb71 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -75,11 +75,9 @@ uint64_t parseFeatures(const envoy::api::v2::Cluster& config, class UpstreamSocketOption : public Network::SocketOptionImpl { public: UpstreamSocketOption(const ClusterInfo& cluster_info) - : Network::SocketOptionImpl({}, - cluster_info.features() & ClusterInfo::Features::FREEBIND - ? true - : absl::optional{}, - {}) {} + : Network::SocketOptionImpl({}, cluster_info.features() & ClusterInfo::Features::FREEBIND + ? true + : absl::optional{}) {} }; } // namespace diff --git a/source/server/listener_manager_impl.cc b/source/server/listener_manager_impl.cc index f015c31a679c8..59d900623cc27 100644 --- a/source/server/listener_manager_impl.cc +++ b/source/server/listener_manager_impl.cc @@ -114,9 +114,37 @@ class ListenerSocketOption : public Network::SocketOptionImpl { ListenerSocketOption(const envoy::api::v2::Listener& config) : Network::SocketOptionImpl( PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, transparent, absl::optional{}), - PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, freebind, absl::optional{}), - PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, tcp_fast_open_queue_length, - absl::optional{})) {} + PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, freebind, absl::optional{})), + tcp_fast_open_queue_length_(PROTOBUF_GET_WRAPPED_OR_DEFAULT( + config, tcp_fast_open_queue_length, absl::optional{})) {} + + // Socket::Option + bool setOption(Network::Socket& socket, Network::Socket::SocketState state) const override { + if (state == Network::Socket::SocketState::Listening) { + if (tcp_fast_open_queue_length_.has_value()) { + const int tfo_value = tcp_fast_open_queue_length_.value(); + const Network::SocketOptionName option_name = ENVOY_SOCKET_TCP_FASTOPEN; + if (option_name) { + const int error = Api::OsSysCallsSingleton::get().setsockopt( + socket.fd(), IPPROTO_TCP, option_name.value(), + reinterpret_cast(&tfo_value), sizeof(tfo_value)); + if (error != 0) { + ENVOY_LOG(warn, "Setting TCP_FASTOPEN on listener socket failed: {}", strerror(errno)); + return false; + } else { + ENVOY_LOG(debug, "Successfully set socket option TCP_FASTOPEN to {}", tfo_value); + } + } else { + ENVOY_LOG(warn, "Unsupported socket option TCP_FASTOPEN"); + } + } + } + + return true; + } + +private: + const absl::optional tcp_fast_open_queue_length_; }; ListenerImpl::ListenerImpl(const envoy::api::v2::Listener& config, ListenerManagerImpl& parent, From e7ff081da0797a41858cac6be9b3ae72726e7291 Mon Sep 17 00:00:00 2001 From: Bjoern Metzdorf Date: Thu, 5 Apr 2018 16:06:42 -0700 Subject: [PATCH 15/31] move ListenerSocketOption into its own file Signed-off-by: Bjoern Metzdorf --- source/server/BUILD | 13 ++++++ source/server/listener_manager_impl.cc | 43 +------------------- source/server/listener_socket_option_impl.cc | 33 +++++++++++++++ source/server/listener_socket_option_impl.h | 35 ++++++++++++++++ 4 files changed, 83 insertions(+), 41 deletions(-) create mode 100644 source/server/listener_socket_option_impl.cc create mode 100644 source/server/listener_socket_option_impl.h diff --git a/source/server/BUILD b/source/server/BUILD index 293f396f4166e..268e96c14f499 100644 --- a/source/server/BUILD +++ b/source/server/BUILD @@ -197,6 +197,7 @@ envoy_cc_library( ":configuration_lib", ":drain_manager_lib", ":init_manager_lib", + ":listener_socket_option_lib", "//include/envoy/server:filter_config_interface", "//include/envoy/server:listener_manager_interface", "//include/envoy/server:transport_socket_config_interface", @@ -213,6 +214,18 @@ envoy_cc_library( ], ) +envoy_cc_library( + name = "listener_socket_option_lib", + srcs = ["listener_socket_option_impl.cc"], + hdrs = ["listener_socket_option_impl.h"], + deps = [ + ":configuration_lib", + "//source/common/network:listen_socket_lib", + "//source/common/network:socket_option_lib", + "//source/common/protobuf:utility_lib", + ], +) + envoy_cc_library( name = "proto_descriptors_lib", srcs = ["proto_descriptors.cc"], diff --git a/source/server/listener_manager_impl.cc b/source/server/listener_manager_impl.cc index 59d900623cc27..4d04d5799b9f4 100644 --- a/source/server/listener_manager_impl.cc +++ b/source/server/listener_manager_impl.cc @@ -1,4 +1,5 @@ #include "server/listener_manager_impl.h" +#include "server/listener_socket_option_impl.h" #include "envoy/registry/registry.h" #include "envoy/server/transport_socket_config.h" @@ -107,46 +108,6 @@ ProdListenerComponentFactory::createDrainManager(envoy::api::v2::Listener::Drain return DrainManagerPtr{new DrainManagerImpl(server_, drain_type)}; } -// Socket::Option implementation for API-defined listener socket options. -// This same object can be extended to handle additional listener socket options. -class ListenerSocketOption : public Network::SocketOptionImpl { -public: - ListenerSocketOption(const envoy::api::v2::Listener& config) - : Network::SocketOptionImpl( - PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, transparent, absl::optional{}), - PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, freebind, absl::optional{})), - tcp_fast_open_queue_length_(PROTOBUF_GET_WRAPPED_OR_DEFAULT( - config, tcp_fast_open_queue_length, absl::optional{})) {} - - // Socket::Option - bool setOption(Network::Socket& socket, Network::Socket::SocketState state) const override { - if (state == Network::Socket::SocketState::Listening) { - if (tcp_fast_open_queue_length_.has_value()) { - const int tfo_value = tcp_fast_open_queue_length_.value(); - const Network::SocketOptionName option_name = ENVOY_SOCKET_TCP_FASTOPEN; - if (option_name) { - const int error = Api::OsSysCallsSingleton::get().setsockopt( - socket.fd(), IPPROTO_TCP, option_name.value(), - reinterpret_cast(&tfo_value), sizeof(tfo_value)); - if (error != 0) { - ENVOY_LOG(warn, "Setting TCP_FASTOPEN on listener socket failed: {}", strerror(errno)); - return false; - } else { - ENVOY_LOG(debug, "Successfully set socket option TCP_FASTOPEN to {}", tfo_value); - } - } else { - ENVOY_LOG(warn, "Unsupported socket option TCP_FASTOPEN"); - } - } - } - - return true; - } - -private: - const absl::optional tcp_fast_open_queue_length_; -}; - ListenerImpl::ListenerImpl(const envoy::api::v2::Listener& config, ListenerManagerImpl& parent, const std::string& name, bool modifiable, bool workers_started, uint64_t hash) @@ -169,7 +130,7 @@ ListenerImpl::ListenerImpl(const envoy::api::v2::Listener& config, ListenerManag ASSERT(config.filter_chains().size() >= 1); // Add listen socket options from the config. - addListenSocketOption(std::make_shared(config)); + addListenSocketOption(std::make_shared(config)); if (!config.listener_filters().empty()) { listener_filter_factories_ = diff --git a/source/server/listener_socket_option_impl.cc b/source/server/listener_socket_option_impl.cc new file mode 100644 index 0000000000000..173a295879aeb --- /dev/null +++ b/source/server/listener_socket_option_impl.cc @@ -0,0 +1,33 @@ +#include "common/api/os_sys_calls_impl.h" + +#include "server/listener_socket_option_impl.h" + +namespace Envoy { +namespace Server { + +bool ListenerSocketOptionImpl::setOption(Network::Socket& socket, Network::Socket::SocketState state) const { + if (state == Network::Socket::SocketState::Listening) { + if (tcp_fast_open_queue_length_.has_value()) { + const int tfo_value = tcp_fast_open_queue_length_.value(); + const Network::SocketOptionName option_name = ENVOY_SOCKET_TCP_FASTOPEN; + if (option_name) { + const int error = Api::OsSysCallsSingleton::get().setsockopt( + socket.fd(), IPPROTO_TCP, option_name.value(), + reinterpret_cast(&tfo_value), sizeof(tfo_value)); + if (error != 0) { + ENVOY_LOG(warn, "Setting TCP_FASTOPEN on listener socket failed: {}", strerror(errno)); + return false; + } else { + ENVOY_LOG(debug, "Successfully set socket option TCP_FASTOPEN to {}", tfo_value); + } + } else { + ENVOY_LOG(warn, "Unsupported socket option TCP_FASTOPEN"); + } + } + } + + return true; +} + +} // namespace Server +} // namespace Envoy diff --git a/source/server/listener_socket_option_impl.h b/source/server/listener_socket_option_impl.h new file mode 100644 index 0000000000000..5818a5afa36ec --- /dev/null +++ b/source/server/listener_socket_option_impl.h @@ -0,0 +1,35 @@ +#pragma once + +#include "envoy/api/v2/listener/listener.pb.h" + +#include "common/network/listen_socket_impl.h" +#include "common/network/socket_option_impl.h" + +#include "server/configuration_impl.h" + +namespace Envoy { +namespace Server { + +// Socket::Option implementation for API-defined listener socket options. +// This same object can be extended to handle additional listener socket options. +class ListenerSocketOptionImpl : public Network::SocketOptionImpl { +public: + ListenerSocketOptionImpl(const envoy::api::v2::Listener& config) + : Network::SocketOptionImpl( + PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, transparent, absl::optional{}), + PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, freebind, absl::optional{})), + tcp_fast_open_queue_length_(PROTOBUF_GET_WRAPPED_OR_DEFAULT( + config, tcp_fast_open_queue_length, absl::optional{})) {} + + ListenerSocketOptionImpl(absl::optional transparent, absl::optional freebind, absl::optional tcp_fast_open_queue_length) + : Network::SocketOptionImpl(transparent, freebind), tcp_fast_open_queue_length_(tcp_fast_open_queue_length) {} + + // Socket::Option + bool setOption(Network::Socket& socket, Network::Socket::SocketState state) const override; + +private: + const absl::optional tcp_fast_open_queue_length_; +}; + +} // namespace Server +} // namespace Envoy From 8f40b58a76f652d3a781c39106569a672e880c7d Mon Sep 17 00:00:00 2001 From: Bjoern Metzdorf Date: Thu, 5 Apr 2018 16:08:54 -0700 Subject: [PATCH 16/31] add listener_socket_option_impl_test Signed-off-by: Bjoern Metzdorf --- test/common/network/BUILD | 1 + .../common/network/socket_option_impl_test.cc | 38 ++------- test/server/BUILD | 10 +++ .../listener_socket_option_impl_test.cc | 85 +++++++++++++++++++ 4 files changed, 103 insertions(+), 31 deletions(-) create mode 100644 test/server/listener_socket_option_impl_test.cc diff --git a/test/common/network/BUILD b/test/common/network/BUILD index 15aeb664709ec..4c9f71926e625 100644 --- a/test/common/network/BUILD +++ b/test/common/network/BUILD @@ -157,6 +157,7 @@ envoy_cc_test( deps = [ "//source/common/network:address_lib", "//source/common/network:socket_option_lib", + "//source/server:listener_socket_option_lib", "//test/mocks/api:api_mocks", "//test/mocks/network:network_mocks", "//test/test_common:threadsafe_singleton_injector_lib", diff --git a/test/common/network/socket_option_impl_test.cc b/test/common/network/socket_option_impl_test.cc index 466c6bfaab15c..c96d997af645a 100644 --- a/test/common/network/socket_option_impl_test.cc +++ b/test/common/network/socket_option_impl_test.cc @@ -69,7 +69,7 @@ TEST_F(SocketOptionImplTest, BadFd) { // Nop when there are no socket options set. TEST_F(SocketOptionImplTest, SetOptionEmptyNop) { - SocketOptionImpl socket_option{{}, {}, {}}; + SocketOptionImpl socket_option{{}, {}}; EXPECT_TRUE(socket_option.setOption(socket_, Socket::SocketState::PreBind)); EXPECT_TRUE(socket_option.setOption(socket_, Socket::SocketState::PostBind)); EXPECT_TRUE(socket_option.setOption(socket_, Socket::SocketState::Listening)); @@ -77,68 +77,44 @@ TEST_F(SocketOptionImplTest, SetOptionEmptyNop) { // We fail to set the option when the underlying setsockopt syscall fails. TEST_F(SocketOptionImplTest, SetOptionTransparentFailure) { - SocketOptionImpl socket_option{true, {}, {}}; + SocketOptionImpl socket_option{true, {}}; EXPECT_FALSE(socket_option.setOption(socket_, Socket::SocketState::PreBind)); } // We fail to set the option when the underlying setsockopt syscall fails. TEST_F(SocketOptionImplTest, SetOptionFreebindFailure) { - SocketOptionImpl socket_option{{}, true, {}}; + SocketOptionImpl socket_option{{}, true}; EXPECT_FALSE(socket_option.setOption(socket_, Socket::SocketState::PreBind)); } -// We fail to set the tcp-fastopen option when the underlying setsockopt syscall fails. -TEST_F(SocketOptionImplTest, SetOptionTcpFastopenFailure) { - if (ENVOY_SOCKET_TCP_FASTOPEN.has_value()) { - SocketOptionImpl socket_option{{}, {}, 1}; - EXPECT_CALL(os_sys_calls_, setsockopt_(_, IPPROTO_TCP, ENVOY_SOCKET_TCP_FASTOPEN.value(), _, _)) - .WillOnce(Return(-1)); - EXPECT_FALSE(socket_option.setOption(socket_, Socket::SocketState::Listening)); - } -} - // The happy path for setOption(); IP_TRANSPARENT is set to true. TEST_F(SocketOptionImplTest, SetOptionTransparentSuccessTrue) { - SocketOptionImpl socket_option{true, {}, {}}; + SocketOptionImpl socket_option{true, {}}; testSetSocketOptionSuccess(socket_option, IPPROTO_IP, ENVOY_SOCKET_IP_TRANSPARENT, 1, {Socket::SocketState::PreBind, Socket::SocketState::PostBind}); } // The happy path for setOption(); IP_FREEBIND is set to true. TEST_F(SocketOptionImplTest, SetOptionFreebindSuccessTrue) { - SocketOptionImpl socket_option{{}, true, {}}; + SocketOptionImpl socket_option{{}, true}; testSetSocketOptionSuccess(socket_option, IPPROTO_IP, ENVOY_SOCKET_IP_FREEBIND, 1, {Socket::SocketState::PreBind}); } -// The happy path for setOption(); TCP_FASTOPEN is set to true. -TEST_F(SocketOptionImplTest, SetOptionTcpFastopenSuccessTrue) { - SocketOptionImpl socket_option{{}, {}, 42}; - testSetSocketOptionSuccess(socket_option, IPPROTO_TCP, ENVOY_SOCKET_TCP_FASTOPEN, 42, - {Socket::SocketState::Listening}); -} - // The happy path for setOption(); IP_TRANSPARENT is set to false. TEST_F(SocketOptionImplTest, SetOptionTransparentSuccessFalse) { - SocketOptionImpl socket_option{false, {}, {}}; + SocketOptionImpl socket_option{false, {}}; testSetSocketOptionSuccess(socket_option, IPPROTO_IP, ENVOY_SOCKET_IP_TRANSPARENT, 0, {Socket::SocketState::PreBind, Socket::SocketState::PostBind}); } // The happy path for setOption(); IP_FREEBIND is set to false. TEST_F(SocketOptionImplTest, SetOptionFreebindSuccessFalse) { - SocketOptionImpl socket_option{{}, false, {}}; + SocketOptionImpl socket_option{{}, false}; testSetSocketOptionSuccess(socket_option, IPPROTO_IP, ENVOY_SOCKET_IP_FREEBIND, 0, {Socket::SocketState::PreBind}); } -// The happy path for setOption(); TCP_FASTOPEN is set to false. -TEST_F(SocketOptionImplTest, SetOptionTcpFastopenSuccessFalse) { - SocketOptionImpl socket_option{{}, {}, 0}; - testSetSocketOptionSuccess(socket_option, IPPROTO_TCP, ENVOY_SOCKET_TCP_FASTOPEN, 0, - {Socket::SocketState::Listening}); -} - // If a platform doesn't suppport IPv4 socket option variant for an IPv4 address, we fail // SocketOptionImpl::setIpSocketOption(). TEST_F(SocketOptionImplTest, V4EmptyOptionNames) { diff --git a/test/server/BUILD b/test/server/BUILD index 60ebba4273023..342f99529bc39 100644 --- a/test/server/BUILD +++ b/test/server/BUILD @@ -143,6 +143,16 @@ envoy_cc_test( ], ) +envoy_cc_test( + name = "listener_socket_option_impl_test", + srcs = ["listener_socket_option_impl_test.cc"], + deps = [ + "//source/server:listener_socket_option_lib", + "//test/mocks/server:server_mocks", + "//test/test_common:threadsafe_singleton_injector_lib", + ], +) + envoy_cc_test( name = "server_test", srcs = ["server_test.cc"], diff --git a/test/server/listener_socket_option_impl_test.cc b/test/server/listener_socket_option_impl_test.cc new file mode 100644 index 0000000000000..48190c56b8dd3 --- /dev/null +++ b/test/server/listener_socket_option_impl_test.cc @@ -0,0 +1,85 @@ +#include "server/listener_socket_option_impl.h" + +#include "test/mocks/server/mocks.h" +#include "test/test_common/threadsafe_singleton_injector.h" + +#include "gtest/gtest.h" + +using testing::Invoke; +using testing::Return; +using testing::_; + +namespace Envoy { +namespace Server { + +class ListenerSocketOptionImplTest : public testing::Test { +public: + ListenerSocketOptionImplTest() { socket_.local_address_.reset(); } + + NiceMock socket_; + Api::MockOsSysCalls os_sys_calls_; + TestThreadsafeSingletonInjector os_calls{&os_sys_calls_}; + + void testSetSocketOptionSuccess(ListenerSocketOptionImpl& socket_option, int socket_level, + Network::SocketOptionName option_name, int option_val, + const std::set& when) { + Network::Address::Ipv4Instance address("1.2.3.4", 5678); + const int fd = address.socket(Network::Address::SocketType::Stream); + EXPECT_CALL(socket_, fd()).WillRepeatedly(Return(fd)); + + for (Network::Socket::SocketState state : when) { + if (option_name.has_value()) { + EXPECT_CALL(os_sys_calls_, + setsockopt_(_, socket_level, option_name.value(), _, sizeof(int))) + .WillOnce(Invoke([option_val](int, int, int, const void* optval, socklen_t) -> int { + EXPECT_EQ(option_val, *static_cast(optval)); + return 0; + })); + EXPECT_TRUE(socket_option.setOption(socket_, state)); + } else { + EXPECT_FALSE(socket_option.setOption(socket_, state)); + } + } + + // The set of SocketState for which this option should not be set. Initialize to all + // the states, and remove states that are passed in. + std::list unset_socketstates{ + Network::Socket::SocketState::PreBind, + Network::Socket::SocketState::PostBind, + Network::Socket::SocketState::Listening, + }; + unset_socketstates.remove_if( + [&](Network::Socket::SocketState state) -> bool { return when.find(state) != when.end(); }); + for (Network::Socket::SocketState state : unset_socketstates) { + EXPECT_CALL(os_sys_calls_, setsockopt_(_, _, _, _, _)).Times(0); + EXPECT_TRUE(socket_option.setOption(socket_, state)); + } + } +}; + +// We fail to set the tcp-fastopen option when the underlying setsockopt syscall fails. +TEST_F(ListenerSocketOptionImplTest, SetOptionTcpFastopenFailure) { + if (ENVOY_SOCKET_TCP_FASTOPEN.has_value()) { + ListenerSocketOptionImpl socket_option{{}, {}, 1}; + EXPECT_CALL(os_sys_calls_, setsockopt_(_, IPPROTO_TCP, ENVOY_SOCKET_TCP_FASTOPEN.value(), _, _)) + .WillOnce(Return(-1)); + EXPECT_FALSE(socket_option.setOption(socket_, Network::Socket::SocketState::Listening)); + } +} + +// The happy path for setOption(); TCP_FASTOPEN is set to true. +TEST_F(ListenerSocketOptionImplTest, SetOptionTcpFastopenSuccessTrue) { + ListenerSocketOptionImpl socket_option{{}, {}, 42}; + testSetSocketOptionSuccess(socket_option, IPPROTO_TCP, ENVOY_SOCKET_TCP_FASTOPEN, 42, + {Network::Socket::SocketState::Listening}); +} + +// The happy path for setOption(); TCP_FASTOPEN is set to false. +TEST_F(ListenerSocketOptionImplTest, SetOptionTcpFastopenSuccessFalse) { + ListenerSocketOptionImpl socket_option{{}, {}, 0}; + testSetSocketOptionSuccess(socket_option, IPPROTO_TCP, ENVOY_SOCKET_TCP_FASTOPEN, 0, + {Network::Socket::SocketState::Listening}); +} + +} // namespace Server +} // namespace Envoy From 42ad7e408457f759ad4bfe9ff8489916588d8ae1 Mon Sep 17 00:00:00 2001 From: Bjoern Metzdorf Date: Thu, 5 Apr 2018 16:10:36 -0700 Subject: [PATCH 17/31] move ENVOY_SOCKET_TCP_FASTOPEN to listener_socket_option_impl.h Signed-off-by: Bjoern Metzdorf --- source/common/network/socket_option_impl.h | 6 ------ source/server/listener_socket_option_impl.h | 6 ++++++ 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/source/common/network/socket_option_impl.h b/source/common/network/socket_option_impl.h index 945d84809a1d6..4861e43155a28 100644 --- a/source/common/network/socket_option_impl.h +++ b/source/common/network/socket_option_impl.h @@ -42,12 +42,6 @@ typedef absl::optional SocketOptionName; #define ENVOY_SOCKET_IPV6_FREEBIND Network::SocketOptionName() #endif -#ifdef TCP_FASTOPEN -#define ENVOY_SOCKET_TCP_FASTOPEN Network::SocketOptionName(TCP_FASTOPEN) -#else -#define ENVOY_SOCKET_TCP_FASTOPEN Network::SocketOptionName() -#endif - class SocketOptionImpl : public Socket::Option, protected Logger::Loggable { public: SocketOptionImpl(absl::optional transparent, absl::optional freebind) diff --git a/source/server/listener_socket_option_impl.h b/source/server/listener_socket_option_impl.h index 5818a5afa36ec..d2f8e563deee2 100644 --- a/source/server/listener_socket_option_impl.h +++ b/source/server/listener_socket_option_impl.h @@ -7,6 +7,12 @@ #include "server/configuration_impl.h" +#ifdef TCP_FASTOPEN +#define ENVOY_SOCKET_TCP_FASTOPEN Network::SocketOptionName(TCP_FASTOPEN) +#else +#define ENVOY_SOCKET_TCP_FASTOPEN Network::SocketOptionName() +#endif + namespace Envoy { namespace Server { From f90b61f168f89eea3acbc2bee735e108442430f2 Mon Sep 17 00:00:00 2001 From: Bjoern Metzdorf Date: Thu, 5 Apr 2018 17:36:02 -0700 Subject: [PATCH 18/31] document Socket::SocketState Signed-off-by: Bjoern Metzdorf --- include/envoy/network/listen_socket.h | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/include/envoy/network/listen_socket.h b/include/envoy/network/listen_socket.h index 128d83b7d3bd8..f43940588b70d 100644 --- a/include/envoy/network/listen_socket.h +++ b/include/envoy/network/listen_socket.h @@ -31,6 +31,11 @@ class Socket { */ virtual void close() PURE; + /** + * PreBind: after socket creation but before binding the socket to a port + * PostBind: after binding the socket to a port but before calling listen() + * Listening: after calling listen() + */ enum class SocketState { PreBind, PostBind, Listening }; /** From 1d5734adb5b18f6d449a1d34f49609eee09bd9fa Mon Sep 17 00:00:00 2001 From: Bjoern Metzdorf Date: Thu, 5 Apr 2018 17:37:48 -0700 Subject: [PATCH 19/31] removing testing::AtLeast Signed-off-by: Bjoern Metzdorf --- test/common/network/listener_impl_test.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/test/common/network/listener_impl_test.cc b/test/common/network/listener_impl_test.cc index 48d5c777c879f..2b81fe9781485 100644 --- a/test/common/network/listener_impl_test.cc +++ b/test/common/network/listener_impl_test.cc @@ -12,7 +12,6 @@ #include "gmock/gmock.h" #include "gtest/gtest.h" -using testing::AtLeast; using testing::Invoke; using testing::Return; using testing::_; From 9e57d17c7be730afaccb5d015f85bbd51201d1ff Mon Sep 17 00:00:00 2001 From: Bjoern Metzdorf Date: Thu, 5 Apr 2018 17:38:01 -0700 Subject: [PATCH 20/31] fix formatting Signed-off-by: Bjoern Metzdorf --- source/server/listener_manager_impl.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/server/listener_manager_impl.cc b/source/server/listener_manager_impl.cc index 4d04d5799b9f4..f27f662000536 100644 --- a/source/server/listener_manager_impl.cc +++ b/source/server/listener_manager_impl.cc @@ -1,5 +1,4 @@ #include "server/listener_manager_impl.h" -#include "server/listener_socket_option_impl.h" #include "envoy/registry/registry.h" #include "envoy/server/transport_socket_config.h" @@ -16,6 +15,7 @@ #include "server/configuration_impl.h" #include "server/drain_manager_impl.h" +#include "server/listener_socket_option_impl.h" namespace Envoy { namespace Server { From 5abe337b30c421cbb48143268db6a7920f962096 Mon Sep 17 00:00:00 2001 From: Bjoern Metzdorf Date: Thu, 5 Apr 2018 17:41:04 -0700 Subject: [PATCH 21/31] just keep constructor prototypes in listener_socket_option_impl.h Signed-off-by: Bjoern Metzdorf --- source/server/listener_socket_option_impl.cc | 20 +++++++++++++++++--- source/server/listener_socket_option_impl.h | 13 ++++--------- 2 files changed, 21 insertions(+), 12 deletions(-) diff --git a/source/server/listener_socket_option_impl.cc b/source/server/listener_socket_option_impl.cc index 173a295879aeb..80e252a8361f3 100644 --- a/source/server/listener_socket_option_impl.cc +++ b/source/server/listener_socket_option_impl.cc @@ -1,11 +1,25 @@ -#include "common/api/os_sys_calls_impl.h" - #include "server/listener_socket_option_impl.h" +#include "common/api/os_sys_calls_impl.h" + namespace Envoy { namespace Server { -bool ListenerSocketOptionImpl::setOption(Network::Socket& socket, Network::Socket::SocketState state) const { +ListenerSocketOptionImpl::ListenerSocketOptionImpl(const envoy::api::v2::Listener& config) + : Network::SocketOptionImpl( + PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, transparent, absl::optional{}), + PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, freebind, absl::optional{})), + tcp_fast_open_queue_length_(PROTOBUF_GET_WRAPPED_OR_DEFAULT( + config, tcp_fast_open_queue_length, absl::optional{})) {} + +ListenerSocketOptionImpl::ListenerSocketOptionImpl( + absl::optional transparent, absl::optional freebind, + absl::optional tcp_fast_open_queue_length) + : Network::SocketOptionImpl(transparent, freebind), + tcp_fast_open_queue_length_(tcp_fast_open_queue_length) {} + +bool ListenerSocketOptionImpl::setOption(Network::Socket& socket, + Network::Socket::SocketState state) const { if (state == Network::Socket::SocketState::Listening) { if (tcp_fast_open_queue_length_.has_value()) { const int tfo_value = tcp_fast_open_queue_length_.value(); diff --git a/source/server/listener_socket_option_impl.h b/source/server/listener_socket_option_impl.h index d2f8e563deee2..1ed381f316b7b 100644 --- a/source/server/listener_socket_option_impl.h +++ b/source/server/listener_socket_option_impl.h @@ -20,15 +20,10 @@ namespace Server { // This same object can be extended to handle additional listener socket options. class ListenerSocketOptionImpl : public Network::SocketOptionImpl { public: - ListenerSocketOptionImpl(const envoy::api::v2::Listener& config) - : Network::SocketOptionImpl( - PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, transparent, absl::optional{}), - PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, freebind, absl::optional{})), - tcp_fast_open_queue_length_(PROTOBUF_GET_WRAPPED_OR_DEFAULT( - config, tcp_fast_open_queue_length, absl::optional{})) {} - - ListenerSocketOptionImpl(absl::optional transparent, absl::optional freebind, absl::optional tcp_fast_open_queue_length) - : Network::SocketOptionImpl(transparent, freebind), tcp_fast_open_queue_length_(tcp_fast_open_queue_length) {} + ListenerSocketOptionImpl(const envoy::api::v2::Listener& config); + + ListenerSocketOptionImpl(absl::optional transparent, absl::optional freebind, + absl::optional tcp_fast_open_queue_length); // Socket::Option bool setOption(Network::Socket& socket, Network::Socket::SocketState state) const override; From 2b21dd5c90cd0f73032c4b71d5558f5b22eb2ab0 Mon Sep 17 00:00:00 2001 From: Bjoern Metzdorf Date: Thu, 5 Apr 2018 17:45:24 -0700 Subject: [PATCH 22/31] move "#include " Signed-off-by: Bjoern Metzdorf --- source/common/network/socket_option_impl.h | 1 - source/server/listener_socket_option_impl.h | 2 ++ 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/source/common/network/socket_option_impl.h b/source/common/network/socket_option_impl.h index 4861e43155a28..cb2cb12928a9a 100644 --- a/source/common/network/socket_option_impl.h +++ b/source/common/network/socket_option_impl.h @@ -2,7 +2,6 @@ #include #include -#include #include #include "envoy/network/listen_socket.h" diff --git a/source/server/listener_socket_option_impl.h b/source/server/listener_socket_option_impl.h index 1ed381f316b7b..ef026d8ae717b 100644 --- a/source/server/listener_socket_option_impl.h +++ b/source/server/listener_socket_option_impl.h @@ -1,5 +1,7 @@ #pragma once +#include + #include "envoy/api/v2/listener/listener.pb.h" #include "common/network/listen_socket_impl.h" From 86268954d518645abe84c04fca7655a158d95251 Mon Sep 17 00:00:00 2001 From: Bjoern Metzdorf Date: Thu, 5 Apr 2018 17:48:12 -0700 Subject: [PATCH 23/31] document SocketState prettier Signed-off-by: Bjoern Metzdorf --- include/envoy/network/listen_socket.h | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/include/envoy/network/listen_socket.h b/include/envoy/network/listen_socket.h index f43940588b70d..9f9c86067f57f 100644 --- a/include/envoy/network/listen_socket.h +++ b/include/envoy/network/listen_socket.h @@ -31,12 +31,11 @@ class Socket { */ virtual void close() PURE; - /** - * PreBind: after socket creation but before binding the socket to a port - * PostBind: after binding the socket to a port but before calling listen() - * Listening: after calling listen() - */ - enum class SocketState { PreBind, PostBind, Listening }; + enum class SocketState { + PreBind, // after socket creation but before binding the socket to a port + PostBind, // after binding the socket to a port but before calling listen() + Listening, // after calling listen() + }; /** * Visitor class for setting socket options. From 4219d2e77016a678136c8f3c481520a7a05dcd33 Mon Sep 17 00:00:00 2001 From: Bjoern Metzdorf Date: Thu, 12 Apr 2018 14:44:24 -0700 Subject: [PATCH 24/31] document SocketState even prettier Signed-off-by: Bjoern Metzdorf --- include/envoy/network/listen_socket.h | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/include/envoy/network/listen_socket.h b/include/envoy/network/listen_socket.h index 9f9c86067f57f..47b38a2b8d084 100644 --- a/include/envoy/network/listen_socket.h +++ b/include/envoy/network/listen_socket.h @@ -32,9 +32,12 @@ class Socket { virtual void close() PURE; enum class SocketState { - PreBind, // after socket creation but before binding the socket to a port - PostBind, // after binding the socket to a port but before calling listen() - Listening, // after calling listen() + // Socket options are applied after socket creation but before binding the socket to a port + PreBind, + // Socket options are applied after binding the socket to a port but before calling listen() + PostBind, + // Socket options are applied after calling listen() + Listening, }; /** From b9312fd0370022cc94d07846227a3165ad894adf Mon Sep 17 00:00:00 2001 From: Greg Greenway Date: Thu, 12 Apr 2018 14:46:05 -0700 Subject: [PATCH 25/31] Call parent class setOption() Signed-off-by: Greg Greenway --- source/server/listener_socket_option_impl.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/source/server/listener_socket_option_impl.cc b/source/server/listener_socket_option_impl.cc index 80e252a8361f3..6d1a5ccf75bb8 100644 --- a/source/server/listener_socket_option_impl.cc +++ b/source/server/listener_socket_option_impl.cc @@ -20,6 +20,7 @@ ListenerSocketOptionImpl::ListenerSocketOptionImpl( bool ListenerSocketOptionImpl::setOption(Network::Socket& socket, Network::Socket::SocketState state) const { + Network::SocketOptionImpl::setOption(socket, state); if (state == Network::Socket::SocketState::Listening) { if (tcp_fast_open_queue_length_.has_value()) { const int tfo_value = tcp_fast_open_queue_length_.value(); From f9f608af86eed0bc64128e1cf042b99e9b2d9475 Mon Sep 17 00:00:00 2001 From: Bjoern Metzdorf Date: Thu, 12 Apr 2018 14:57:42 -0700 Subject: [PATCH 26/31] add os_sys_calls bazel dependency Signed-off-by: Bjoern Metzdorf --- source/server/BUILD | 1 + 1 file changed, 1 insertion(+) diff --git a/source/server/BUILD b/source/server/BUILD index b7bf5e5c3e003..a49e5b94b6aff 100644 --- a/source/server/BUILD +++ b/source/server/BUILD @@ -221,6 +221,7 @@ envoy_cc_library( hdrs = ["listener_socket_option_impl.h"], deps = [ ":configuration_lib", + "//source/common/api:os_sys_calls_lib", "//source/common/network:listen_socket_lib", "//source/common/network:socket_option_lib", "//source/common/protobuf:utility_lib", From 2ca16c3c532b4fe5dacdcb2080befa681f013be1 Mon Sep 17 00:00:00 2001 From: Bjoern Metzdorf Date: Thu, 12 Apr 2018 15:33:12 -0700 Subject: [PATCH 27/31] return false if TFO is requested but not supported Signed-off-by: Bjoern Metzdorf --- source/server/listener_socket_option_impl.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/source/server/listener_socket_option_impl.cc b/source/server/listener_socket_option_impl.cc index 6d1a5ccf75bb8..568d986482e49 100644 --- a/source/server/listener_socket_option_impl.cc +++ b/source/server/listener_socket_option_impl.cc @@ -37,6 +37,7 @@ bool ListenerSocketOptionImpl::setOption(Network::Socket& socket, } } else { ENVOY_LOG(warn, "Unsupported socket option TCP_FASTOPEN"); + return false; } } } From e0dbd9ff41d0473bac35af8f88e8123545607b8e Mon Sep 17 00:00:00 2001 From: Bjoern Metzdorf Date: Mon, 16 Apr 2018 13:01:02 -0700 Subject: [PATCH 28/31] Add TCP_FASTOPEN listener option release notes Signed-off-by: Bjoern Metzdorf --- docs/root/intro/version_history.rst | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index dae201a645e95..334ce0faa4e0f 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -24,6 +24,7 @@ Version history `, :ref:`unhealthy to healthy ` and for subsequent checks on :ref:`unhealthy hosts `. +* listeners: added :ref:`tcp_fast_open_queue_length ` option. * load balancing: added :ref:`weighted round robin ` support. The round robin scheduler now respects endpoint weights and also has improved fidelity across From 3dd937274e8b4d2a8fdfcecf6465ed3a1fd5f552 Mon Sep 17 00:00:00 2001 From: Bjoern Metzdorf Date: Mon, 16 Apr 2018 17:13:04 -0700 Subject: [PATCH 29/31] move TCP_FASTOPEN into socket_option_impl.h Signed-off-by: Bjoern Metzdorf --- source/common/network/socket_option_impl.h | 6 ++++++ source/server/listener_socket_option_impl.h | 6 ------ 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/source/common/network/socket_option_impl.h b/source/common/network/socket_option_impl.h index cb2cb12928a9a..0e7277aa1eda8 100644 --- a/source/common/network/socket_option_impl.h +++ b/source/common/network/socket_option_impl.h @@ -41,6 +41,12 @@ typedef absl::optional SocketOptionName; #define ENVOY_SOCKET_IPV6_FREEBIND Network::SocketOptionName() #endif +#ifdef TCP_FASTOPEN +#define ENVOY_SOCKET_TCP_FASTOPEN Network::SocketOptionName(TCP_FASTOPEN) +#else +#define ENVOY_SOCKET_TCP_FASTOPEN Network::SocketOptionName() +#endif + class SocketOptionImpl : public Socket::Option, protected Logger::Loggable { public: SocketOptionImpl(absl::optional transparent, absl::optional freebind) diff --git a/source/server/listener_socket_option_impl.h b/source/server/listener_socket_option_impl.h index ef026d8ae717b..0ffaa806a1c3b 100644 --- a/source/server/listener_socket_option_impl.h +++ b/source/server/listener_socket_option_impl.h @@ -9,12 +9,6 @@ #include "server/configuration_impl.h" -#ifdef TCP_FASTOPEN -#define ENVOY_SOCKET_TCP_FASTOPEN Network::SocketOptionName(TCP_FASTOPEN) -#else -#define ENVOY_SOCKET_TCP_FASTOPEN Network::SocketOptionName() -#endif - namespace Envoy { namespace Server { From 3f11ee6a2ee9c1bf4e61022d656837524f1afb14 Mon Sep 17 00:00:00 2001 From: Greg Greenway Date: Tue, 17 Apr 2018 09:31:20 -0700 Subject: [PATCH 30/31] Don't ignore return value from super-call Signed-off-by: Greg Greenway --- source/server/listener_socket_option_impl.cc | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/source/server/listener_socket_option_impl.cc b/source/server/listener_socket_option_impl.cc index 568d986482e49..a69fbe7fb8ef7 100644 --- a/source/server/listener_socket_option_impl.cc +++ b/source/server/listener_socket_option_impl.cc @@ -20,7 +20,11 @@ ListenerSocketOptionImpl::ListenerSocketOptionImpl( bool ListenerSocketOptionImpl::setOption(Network::Socket& socket, Network::Socket::SocketState state) const { - Network::SocketOptionImpl::setOption(socket, state); + bool result = Network::SocketOptionImpl::setOption(socket, state); + if (!result) { + return result; + } + if (state == Network::Socket::SocketState::Listening) { if (tcp_fast_open_queue_length_.has_value()) { const int tfo_value = tcp_fast_open_queue_length_.value(); From 4494d2cad97d060aac215ff5e37ce939c0cae35e Mon Sep 17 00:00:00 2001 From: Bjoern Metzdorf Date: Tue, 17 Apr 2018 20:24:33 -0700 Subject: [PATCH 31/31] ListenerSocketOptionImplTest should inherit from SocketOptionImplTest Signed-off-by: Bjoern Metzdorf --- test/common/network/BUILD | 15 ++++- .../common/network/socket_option_impl_test.cc | 60 +---------------- .../network/socket_option_test_harness.h | 67 +++++++++++++++++++ test/server/BUILD | 3 +- .../listener_socket_option_impl_test.cc | 50 ++------------ 5 files changed, 86 insertions(+), 109 deletions(-) create mode 100644 test/common/network/socket_option_test_harness.h diff --git a/test/common/network/BUILD b/test/common/network/BUILD index 84c73bd1c031d..322239689e3b4 100644 --- a/test/common/network/BUILD +++ b/test/common/network/BUILD @@ -3,6 +3,7 @@ licenses(["notice"]) # Apache 2 load( "//bazel:envoy_build_system.bzl", "envoy_cc_test", + "envoy_cc_test_library", "envoy_package", ) @@ -152,9 +153,9 @@ envoy_cc_test( ], ) -envoy_cc_test( - name = "socket_option_impl_test", - srcs = ["socket_option_impl_test.cc"], +envoy_cc_test_library( + name = "socket_option_test_harness", + srcs = ["socket_option_test_harness.h"], deps = [ "//source/common/network:address_lib", "//source/common/network:socket_option_lib", @@ -166,6 +167,14 @@ envoy_cc_test( ], ) +envoy_cc_test( + name = "socket_option_impl_test", + srcs = ["socket_option_impl_test.cc"], + deps = [ + ":socket_option_test_harness", + ], +) + envoy_cc_test( name = "utility_test", srcs = ["utility_test.cc"], diff --git a/test/common/network/socket_option_impl_test.cc b/test/common/network/socket_option_impl_test.cc index c10113fedb218..0abaf177ce188 100644 --- a/test/common/network/socket_option_impl_test.cc +++ b/test/common/network/socket_option_impl_test.cc @@ -1,66 +1,10 @@ -#include "common/network/address_impl.h" -#include "common/network/socket_option_impl.h" - -#include "test/mocks/api/mocks.h" -#include "test/mocks/network/mocks.h" -#include "test/test_common/logging.h" -#include "test/test_common/threadsafe_singleton_injector.h" - -#include "gtest/gtest.h" - -using testing::Invoke; -using testing::NiceMock; -using testing::Return; -using testing::_; +#include "test/common/network/socket_option_test_harness.h" namespace Envoy { namespace Network { namespace { -class SocketOptionImplTest : public testing::Test { -public: - SocketOptionImplTest() { socket_.local_address_.reset(); } - - NiceMock socket_; - Api::MockOsSysCalls os_sys_calls_; - TestThreadsafeSingletonInjector os_calls{&os_sys_calls_}; - - void testSetSocketOptionSuccess(SocketOptionImpl& socket_option, int socket_level, - Network::SocketOptionName option_name, int option_val, - const std::set& when) { - Address::Ipv4Instance address("1.2.3.4", 5678); - const int fd = address.socket(Address::SocketType::Stream); - EXPECT_CALL(socket_, fd()).WillRepeatedly(Return(fd)); - - for (Socket::SocketState state : when) { - if (option_name.has_value()) { - EXPECT_CALL(os_sys_calls_, - setsockopt_(_, socket_level, option_name.value(), _, sizeof(int))) - .WillOnce(Invoke([option_val](int, int, int, const void* optval, socklen_t) -> int { - EXPECT_EQ(option_val, *static_cast(optval)); - return 0; - })); - EXPECT_TRUE(socket_option.setOption(socket_, state)); - } else { - EXPECT_FALSE(socket_option.setOption(socket_, state)); - } - } - - // The set of SocketState for which this option should not be set. Initialize to all - // the states, and remove states that are passed in. - std::list unset_socketstates{ - Socket::SocketState::PreBind, - Socket::SocketState::PostBind, - Socket::SocketState::Listening, - }; - unset_socketstates.remove_if( - [&](Socket::SocketState state) -> bool { return when.find(state) != when.end(); }); - for (Socket::SocketState state : unset_socketstates) { - EXPECT_CALL(os_sys_calls_, setsockopt_(_, _, _, _, _)).Times(0); - EXPECT_TRUE(socket_option.setOption(socket_, state)); - } - } -}; +class SocketOptionImplTest : public SocketOptionTestHarness {}; // We fail to set the option if the socket FD is bad. TEST_F(SocketOptionImplTest, BadFd) { diff --git a/test/common/network/socket_option_test_harness.h b/test/common/network/socket_option_test_harness.h new file mode 100644 index 0000000000000..d1a724cb09960 --- /dev/null +++ b/test/common/network/socket_option_test_harness.h @@ -0,0 +1,67 @@ +#include "common/network/address_impl.h" +#include "common/network/socket_option_impl.h" + +#include "test/mocks/api/mocks.h" +#include "test/mocks/network/mocks.h" +#include "test/test_common/logging.h" +#include "test/test_common/threadsafe_singleton_injector.h" + +#include "gtest/gtest.h" + +using testing::Invoke; +using testing::NiceMock; +using testing::Return; +using testing::_; + +namespace Envoy { +namespace Network { +namespace { + +class SocketOptionTestHarness : public testing::Test { +public: + SocketOptionTestHarness() { socket_.local_address_.reset(); } + + NiceMock socket_; + Api::MockOsSysCalls os_sys_calls_; + TestThreadsafeSingletonInjector os_calls{&os_sys_calls_}; + + void testSetSocketOptionSuccess(SocketOptionImpl& socket_option, int socket_level, + Network::SocketOptionName option_name, int option_val, + const std::set& when) { + Address::Ipv4Instance address("1.2.3.4", 5678); + const int fd = address.socket(Address::SocketType::Stream); + EXPECT_CALL(socket_, fd()).WillRepeatedly(Return(fd)); + + for (Socket::SocketState state : when) { + if (option_name.has_value()) { + EXPECT_CALL(os_sys_calls_, + setsockopt_(_, socket_level, option_name.value(), _, sizeof(int))) + .WillOnce(Invoke([option_val](int, int, int, const void* optval, socklen_t) -> int { + EXPECT_EQ(option_val, *static_cast(optval)); + return 0; + })); + EXPECT_TRUE(socket_option.setOption(socket_, state)); + } else { + EXPECT_FALSE(socket_option.setOption(socket_, state)); + } + } + + // The set of SocketState for which this option should not be set. Initialize to all + // the states, and remove states that are passed in. + std::list unset_socketstates{ + Socket::SocketState::PreBind, + Socket::SocketState::PostBind, + Socket::SocketState::Listening, + }; + unset_socketstates.remove_if( + [&](Socket::SocketState state) -> bool { return when.find(state) != when.end(); }); + for (Socket::SocketState state : unset_socketstates) { + EXPECT_CALL(os_sys_calls_, setsockopt_(_, _, _, _, _)).Times(0); + EXPECT_TRUE(socket_option.setOption(socket_, state)); + } + } +}; + +} // namespace +} // namespace Network +} // namespace Envoy diff --git a/test/server/BUILD b/test/server/BUILD index d77ae290de6bd..312aa86453c24 100644 --- a/test/server/BUILD +++ b/test/server/BUILD @@ -147,8 +147,7 @@ envoy_cc_test( srcs = ["listener_socket_option_impl_test.cc"], deps = [ "//source/server:listener_socket_option_lib", - "//test/mocks/server:server_mocks", - "//test/test_common:threadsafe_singleton_injector_lib", + "//test/common/network:socket_option_test_harness", ], ) diff --git a/test/server/listener_socket_option_impl_test.cc b/test/server/listener_socket_option_impl_test.cc index 48190c56b8dd3..df55838274d0b 100644 --- a/test/server/listener_socket_option_impl_test.cc +++ b/test/server/listener_socket_option_impl_test.cc @@ -1,59 +1,17 @@ #include "server/listener_socket_option_impl.h" -#include "test/mocks/server/mocks.h" -#include "test/test_common/threadsafe_singleton_injector.h" - -#include "gtest/gtest.h" - -using testing::Invoke; -using testing::Return; -using testing::_; +#include "test/common/network/socket_option_test_harness.h" namespace Envoy { namespace Server { -class ListenerSocketOptionImplTest : public testing::Test { +class ListenerSocketOptionImplTest : public Network::SocketOptionTestHarness { public: - ListenerSocketOptionImplTest() { socket_.local_address_.reset(); } - - NiceMock socket_; - Api::MockOsSysCalls os_sys_calls_; - TestThreadsafeSingletonInjector os_calls{&os_sys_calls_}; - void testSetSocketOptionSuccess(ListenerSocketOptionImpl& socket_option, int socket_level, Network::SocketOptionName option_name, int option_val, const std::set& when) { - Network::Address::Ipv4Instance address("1.2.3.4", 5678); - const int fd = address.socket(Network::Address::SocketType::Stream); - EXPECT_CALL(socket_, fd()).WillRepeatedly(Return(fd)); - - for (Network::Socket::SocketState state : when) { - if (option_name.has_value()) { - EXPECT_CALL(os_sys_calls_, - setsockopt_(_, socket_level, option_name.value(), _, sizeof(int))) - .WillOnce(Invoke([option_val](int, int, int, const void* optval, socklen_t) -> int { - EXPECT_EQ(option_val, *static_cast(optval)); - return 0; - })); - EXPECT_TRUE(socket_option.setOption(socket_, state)); - } else { - EXPECT_FALSE(socket_option.setOption(socket_, state)); - } - } - - // The set of SocketState for which this option should not be set. Initialize to all - // the states, and remove states that are passed in. - std::list unset_socketstates{ - Network::Socket::SocketState::PreBind, - Network::Socket::SocketState::PostBind, - Network::Socket::SocketState::Listening, - }; - unset_socketstates.remove_if( - [&](Network::Socket::SocketState state) -> bool { return when.find(state) != when.end(); }); - for (Network::Socket::SocketState state : unset_socketstates) { - EXPECT_CALL(os_sys_calls_, setsockopt_(_, _, _, _, _)).Times(0); - EXPECT_TRUE(socket_option.setOption(socket_, state)); - } + Network::SocketOptionTestHarness::testSetSocketOptionSuccess(socket_option, socket_level, + option_name, option_val, when); } };