From 32dab3692a07c88355461923e7fb7bd122665e43 Mon Sep 17 00:00:00 2001 From: Alex Konradi Date: Wed, 14 Oct 2020 15:47:26 -0400 Subject: [PATCH 01/11] Add Envoy::Network::ServerConnection class Add a new interface and impl class for incoming connections to the server. This class currently adds no methods on top of the existing Connection class, but will be used to add new functionality in the future. Signed-off-by: Alex Konradi --- include/envoy/event/dispatcher.h | 2 +- include/envoy/network/connection.h | 7 ++ source/common/event/dispatcher_impl.cc | 6 +- source/common/event/dispatcher_impl.h | 7 +- source/common/network/connection_impl.h | 5 + test/common/network/connection_impl_test.cc | 2 +- test/mocks/event/mocks.h | 11 +- test/mocks/event/wrapped_dispatcher.h | 7 +- test/mocks/network/connection.cc | 8 ++ test/mocks/network/connection.h | 122 ++++++++------------ test/server/connection_handler_test.cc | 16 +-- 11 files changed, 95 insertions(+), 98 deletions(-) diff --git a/include/envoy/event/dispatcher.h b/include/envoy/event/dispatcher.h index 0e7865b656d8..96b1d12310fb 100644 --- a/include/envoy/event/dispatcher.h +++ b/include/envoy/event/dispatcher.h @@ -92,7 +92,7 @@ class Dispatcher { * @param stream_info info object for the server connection * @return Network::ConnectionPtr a server connection that is owned by the caller. */ - virtual Network::ConnectionPtr + virtual Network::ServerConnectionPtr createServerConnection(Network::ConnectionSocketPtr&& socket, Network::TransportSocketPtr&& transport_socket, StreamInfo::StreamInfo& stream_info) PURE; diff --git a/include/envoy/network/connection.h b/include/envoy/network/connection.h index b486f614ed96..aecb24f3df3e 100644 --- a/include/envoy/network/connection.h +++ b/include/envoy/network/connection.h @@ -319,6 +319,13 @@ class Connection : public Event::DeferredDeletable, public FilterManager { using ConnectionPtr = std::unique_ptr; +/** + * Connections servicing inbound connects. + */ +class ServerConnection : public virtual Connection {}; + +using ServerConnectionPtr = std::unique_ptr; + /** * Connections capable of outbound connects. */ diff --git a/source/common/event/dispatcher_impl.cc b/source/common/event/dispatcher_impl.cc index 6cdbb623b720..c0beef89afa6 100644 --- a/source/common/event/dispatcher_impl.cc +++ b/source/common/event/dispatcher_impl.cc @@ -103,13 +103,13 @@ void DispatcherImpl::clearDeferredDeleteList() { deferred_deleting_ = false; } -Network::ConnectionPtr +Network::ServerConnectionPtr DispatcherImpl::createServerConnection(Network::ConnectionSocketPtr&& socket, Network::TransportSocketPtr&& transport_socket, StreamInfo::StreamInfo& stream_info) { ASSERT(isThreadSafe()); - return std::make_unique(*this, std::move(socket), - std::move(transport_socket), stream_info, true); + return std::make_unique( + *this, std::move(socket), std::move(transport_socket), stream_info, true); } Network::ClientConnectionPtr diff --git a/source/common/event/dispatcher_impl.h b/source/common/event/dispatcher_impl.h index 4b05b355410c..c01767f40730 100644 --- a/source/common/event/dispatcher_impl.h +++ b/source/common/event/dispatcher_impl.h @@ -45,9 +45,10 @@ class DispatcherImpl : Logger::Loggable, TimeSource& timeSource() override { return api_.timeSource(); } void initializeStats(Stats::Scope& scope, const absl::optional& prefix) override; void clearDeferredDeleteList() override; - Network::ConnectionPtr createServerConnection(Network::ConnectionSocketPtr&& socket, - Network::TransportSocketPtr&& transport_socket, - StreamInfo::StreamInfo& stream_info) override; + Network::ServerConnectionPtr + createServerConnection(Network::ConnectionSocketPtr&& socket, + Network::TransportSocketPtr&& transport_socket, + StreamInfo::StreamInfo& stream_info) override; Network::ClientConnectionPtr createClientConnection(Network::Address::InstanceConstSharedPtr address, Network::Address::InstanceConstSharedPtr source_address, diff --git a/source/common/network/connection_impl.h b/source/common/network/connection_impl.h index e28e05e9d182..761be98cd613 100644 --- a/source/common/network/connection_impl.h +++ b/source/common/network/connection_impl.h @@ -198,6 +198,11 @@ class ConnectionImpl : public ConnectionImplBase, public TransportSocketCallback bool dispatch_buffered_data_ : 1; }; +class ServerConnectionImpl : public ConnectionImpl, virtual public ServerConnection { +public: + using ConnectionImpl::ConnectionImpl; +}; + /** * libevent implementation of Network::ClientConnection. */ diff --git a/test/common/network/connection_impl_test.cc b/test/common/network/connection_impl_test.cc index 086551ade592..fa7a178a79e2 100644 --- a/test/common/network/connection_impl_test.cc +++ b/test/common/network/connection_impl_test.cc @@ -278,7 +278,7 @@ class ConnectionImplTest : public testing::TestWithParam { Network::ListenerPtr listener_; Network::ClientConnectionPtr client_connection_; StrictMock client_callbacks_; - Network::ConnectionPtr server_connection_; + Network::ServerConnectionPtr server_connection_; StrictMock server_callbacks_; std::shared_ptr read_filter_; MockWatermarkBuffer* client_write_buffer_ = nullptr; diff --git a/test/mocks/event/mocks.h b/test/mocks/event/mocks.h index 35475b694b97..47018e1bbbd7 100644 --- a/test/mocks/event/mocks.h +++ b/test/mocks/event/mocks.h @@ -36,13 +36,14 @@ class MockDispatcher : public Dispatcher { // Dispatcher const std::string& name() override { return name_; } TimeSource& timeSource() override { return time_system_; } - Network::ConnectionPtr createServerConnection(Network::ConnectionSocketPtr&& socket, - Network::TransportSocketPtr&& transport_socket, - StreamInfo::StreamInfo&) override { + Network::ServerConnectionPtr + createServerConnection(Network::ConnectionSocketPtr&& socket, + Network::TransportSocketPtr&& transport_socket, + StreamInfo::StreamInfo&) override { // The caller expects both the socket and the transport socket to be moved. socket.reset(); transport_socket.reset(); - return Network::ConnectionPtr{createServerConnection_()}; + return Network::ServerConnectionPtr{createServerConnection_()}; } Network::ClientConnectionPtr @@ -102,7 +103,7 @@ class MockDispatcher : public Dispatcher { // Event::Dispatcher MOCK_METHOD(void, initializeStats, (Stats::Scope&, const absl::optional&)); MOCK_METHOD(void, clearDeferredDeleteList, ()); - MOCK_METHOD(Network::Connection*, createServerConnection_, ()); + MOCK_METHOD(Network::ServerConnection*, createServerConnection_, ()); MOCK_METHOD(Network::ClientConnection*, createClientConnection_, (Network::Address::InstanceConstSharedPtr address, Network::Address::InstanceConstSharedPtr source_address, diff --git a/test/mocks/event/wrapped_dispatcher.h b/test/mocks/event/wrapped_dispatcher.h index 172066506bf0..db9fd1212c14 100644 --- a/test/mocks/event/wrapped_dispatcher.h +++ b/test/mocks/event/wrapped_dispatcher.h @@ -28,9 +28,10 @@ class WrappedDispatcher : public Dispatcher { void clearDeferredDeleteList() override { impl_.clearDeferredDeleteList(); } - Network::ConnectionPtr createServerConnection(Network::ConnectionSocketPtr&& socket, - Network::TransportSocketPtr&& transport_socket, - StreamInfo::StreamInfo& stream_info) override { + Network::ServerConnectionPtr + createServerConnection(Network::ConnectionSocketPtr&& socket, + Network::TransportSocketPtr&& transport_socket, + StreamInfo::StreamInfo& stream_info) override { return impl_.createServerConnection(std::move(socket), std::move(transport_socket), stream_info); } diff --git a/test/mocks/network/connection.cc b/test/mocks/network/connection.cc index 4c46f6af97de..6cff107c6143 100644 --- a/test/mocks/network/connection.cc +++ b/test/mocks/network/connection.cc @@ -82,6 +82,14 @@ MockConnection::MockConnection() { } MockConnection::~MockConnection() = default; +MockServerConnection::MockServerConnection() { + remote_address_ = Utility::resolveUrl("tcp://10.0.0.1:443"); + local_address_ = Utility::resolveUrl("tcp://10.0.0.2:40000"); + initializeMockConnection(*this); +} + +MockServerConnection::~MockServerConnection() = default; + MockClientConnection::MockClientConnection() { remote_address_ = Utility::resolveUrl("tcp://10.0.0.1:443"); local_address_ = Utility::resolveUrl("tcp://10.0.0.2:40000"); diff --git a/test/mocks/network/connection.h b/test/mocks/network/connection.h index 6a7856887fe0..900f4bd50544 100644 --- a/test/mocks/network/connection.h +++ b/test/mocks/network/connection.h @@ -45,48 +45,58 @@ class MockConnectionBase { Connection::State state_{Connection::State::Open}; }; +#define DEFINE_MOCK_CONNECTION_MOCK_METHODS \ + /* Network::Connection */ \ + MOCK_METHOD(void, addConnectionCallbacks, (ConnectionCallbacks & cb)); \ + MOCK_METHOD(void, addBytesSentCallback, (BytesSentCb cb)); \ + MOCK_METHOD(void, addWriteFilter, (WriteFilterSharedPtr filter)); \ + MOCK_METHOD(void, addFilter, (FilterSharedPtr filter)); \ + MOCK_METHOD(void, addReadFilter, (ReadFilterSharedPtr filter)); \ + MOCK_METHOD(void, enableHalfClose, (bool enabled)); \ + MOCK_METHOD(void, close, (ConnectionCloseType type)); \ + MOCK_METHOD(Event::Dispatcher&, dispatcher, ()); \ + MOCK_METHOD(uint64_t, id, (), (const)); \ + MOCK_METHOD(void, hashKey, (std::vector&), (const)); \ + MOCK_METHOD(bool, initializeReadFilters, ()); \ + MOCK_METHOD(std::string, nextProtocol, (), (const)); \ + MOCK_METHOD(void, noDelay, (bool enable)); \ + MOCK_METHOD(void, readDisable, (bool disable)); \ + MOCK_METHOD(void, detectEarlyCloseWhenReadDisabled, (bool)); \ + MOCK_METHOD(bool, readEnabled, (), (const)); \ + MOCK_METHOD(const Address::InstanceConstSharedPtr&, remoteAddress, (), (const)); \ + MOCK_METHOD(const Address::InstanceConstSharedPtr&, directRemoteAddress, (), (const)); \ + MOCK_METHOD(absl::optional, \ + unixSocketPeerCredentials, (), (const)); \ + MOCK_METHOD(const Address::InstanceConstSharedPtr&, localAddress, (), (const)); \ + MOCK_METHOD(void, setConnectionStats, (const ConnectionStats& stats)); \ + MOCK_METHOD(Ssl::ConnectionInfoConstSharedPtr, ssl, (), (const)); \ + MOCK_METHOD(absl::string_view, requestedServerName, (), (const)); \ + MOCK_METHOD(State, state, (), (const)); \ + MOCK_METHOD(void, write, (Buffer::Instance & data, bool end_stream)); \ + MOCK_METHOD(void, setBufferLimits, (uint32_t limit)); \ + MOCK_METHOD(uint32_t, bufferLimit, (), (const)); \ + MOCK_METHOD(bool, localAddressRestored, (), (const)); \ + MOCK_METHOD(bool, aboveHighWatermark, (), (const)); \ + MOCK_METHOD(const Network::ConnectionSocket::OptionsSharedPtr&, socketOptions, (), (const)); \ + MOCK_METHOD(StreamInfo::StreamInfo&, streamInfo, ()); \ + MOCK_METHOD(const StreamInfo::StreamInfo&, streamInfo, (), (const)); \ + MOCK_METHOD(void, setDelayedCloseTimeout, (std::chrono::milliseconds)); \ + MOCK_METHOD(absl::string_view, transportFailureReason, (), (const)); \ + MOCK_METHOD(absl::optional, lastRoundTripTime, (), (const)) + class MockConnection : public Connection, public MockConnectionBase { public: MockConnection(); ~MockConnection() override; + DEFINE_MOCK_CONNECTION_MOCK_METHODS; +}; - // Network::Connection - MOCK_METHOD(void, addConnectionCallbacks, (ConnectionCallbacks & cb)); - MOCK_METHOD(void, addBytesSentCallback, (BytesSentCb cb)); - MOCK_METHOD(void, addWriteFilter, (WriteFilterSharedPtr filter)); - MOCK_METHOD(void, addFilter, (FilterSharedPtr filter)); - MOCK_METHOD(void, addReadFilter, (ReadFilterSharedPtr filter)); - MOCK_METHOD(void, enableHalfClose, (bool enabled)); - MOCK_METHOD(void, close, (ConnectionCloseType type)); - MOCK_METHOD(Event::Dispatcher&, dispatcher, ()); - MOCK_METHOD(uint64_t, id, (), (const)); - MOCK_METHOD(void, hashKey, (std::vector&), (const)); - MOCK_METHOD(bool, initializeReadFilters, ()); - MOCK_METHOD(std::string, nextProtocol, (), (const)); - MOCK_METHOD(void, noDelay, (bool enable)); - MOCK_METHOD(void, readDisable, (bool disable)); - MOCK_METHOD(void, detectEarlyCloseWhenReadDisabled, (bool)); - MOCK_METHOD(bool, readEnabled, (), (const)); - MOCK_METHOD(const Address::InstanceConstSharedPtr&, remoteAddress, (), (const)); - MOCK_METHOD(const Address::InstanceConstSharedPtr&, directRemoteAddress, (), (const)); - MOCK_METHOD(absl::optional, - unixSocketPeerCredentials, (), (const)); - MOCK_METHOD(const Address::InstanceConstSharedPtr&, localAddress, (), (const)); - MOCK_METHOD(void, setConnectionStats, (const ConnectionStats& stats)); - MOCK_METHOD(Ssl::ConnectionInfoConstSharedPtr, ssl, (), (const)); - MOCK_METHOD(absl::string_view, requestedServerName, (), (const)); - MOCK_METHOD(State, state, (), (const)); - MOCK_METHOD(void, write, (Buffer::Instance & data, bool end_stream)); - MOCK_METHOD(void, setBufferLimits, (uint32_t limit)); - MOCK_METHOD(uint32_t, bufferLimit, (), (const)); - MOCK_METHOD(bool, localAddressRestored, (), (const)); - MOCK_METHOD(bool, aboveHighWatermark, (), (const)); - MOCK_METHOD(const Network::ConnectionSocket::OptionsSharedPtr&, socketOptions, (), (const)); - MOCK_METHOD(StreamInfo::StreamInfo&, streamInfo, ()); - MOCK_METHOD(const StreamInfo::StreamInfo&, streamInfo, (), (const)); - MOCK_METHOD(void, setDelayedCloseTimeout, (std::chrono::milliseconds)); - MOCK_METHOD(absl::string_view, transportFailureReason, (), (const)); - MOCK_METHOD(absl::optional, lastRoundTripTime, (), (const)); +class MockServerConnection : public ServerConnection, public MockConnectionBase { +public: + MockServerConnection(); + ~MockServerConnection() override; + + DEFINE_MOCK_CONNECTION_MOCK_METHODS; }; /** @@ -98,43 +108,7 @@ class MockClientConnection : public ClientConnection, public MockConnectionBase MockClientConnection(); ~MockClientConnection() override; - // Network::Connection - MOCK_METHOD(void, addConnectionCallbacks, (ConnectionCallbacks & cb)); - MOCK_METHOD(void, addBytesSentCallback, (BytesSentCb cb)); - MOCK_METHOD(void, addWriteFilter, (WriteFilterSharedPtr filter)); - MOCK_METHOD(void, addFilter, (FilterSharedPtr filter)); - MOCK_METHOD(void, addReadFilter, (ReadFilterSharedPtr filter)); - MOCK_METHOD(void, enableHalfClose, (bool enabled)); - MOCK_METHOD(void, close, (ConnectionCloseType type)); - MOCK_METHOD(Event::Dispatcher&, dispatcher, ()); - MOCK_METHOD(uint64_t, id, (), (const)); - MOCK_METHOD(void, hashKey, (std::vector&), (const)); - MOCK_METHOD(bool, initializeReadFilters, ()); - MOCK_METHOD(std::string, nextProtocol, (), (const)); - MOCK_METHOD(void, noDelay, (bool enable)); - MOCK_METHOD(void, readDisable, (bool disable)); - MOCK_METHOD(void, detectEarlyCloseWhenReadDisabled, (bool)); - MOCK_METHOD(bool, readEnabled, (), (const)); - MOCK_METHOD(const Address::InstanceConstSharedPtr&, remoteAddress, (), (const)); - MOCK_METHOD(const Address::InstanceConstSharedPtr&, directRemoteAddress, (), (const)); - MOCK_METHOD(absl::optional, - unixSocketPeerCredentials, (), (const)); - MOCK_METHOD(const Address::InstanceConstSharedPtr&, localAddress, (), (const)); - MOCK_METHOD(void, setConnectionStats, (const ConnectionStats& stats)); - MOCK_METHOD(Ssl::ConnectionInfoConstSharedPtr, ssl, (), (const)); - MOCK_METHOD(absl::string_view, requestedServerName, (), (const)); - MOCK_METHOD(State, state, (), (const)); - MOCK_METHOD(void, write, (Buffer::Instance & data, bool end_stream)); - MOCK_METHOD(void, setBufferLimits, (uint32_t limit)); - MOCK_METHOD(uint32_t, bufferLimit, (), (const)); - MOCK_METHOD(bool, localAddressRestored, (), (const)); - MOCK_METHOD(bool, aboveHighWatermark, (), (const)); - MOCK_METHOD(const Network::ConnectionSocket::OptionsSharedPtr&, socketOptions, (), (const)); - MOCK_METHOD(StreamInfo::StreamInfo&, streamInfo, ()); - MOCK_METHOD(const StreamInfo::StreamInfo&, streamInfo, (), (const)); - MOCK_METHOD(void, setDelayedCloseTimeout, (std::chrono::milliseconds)); - MOCK_METHOD(absl::string_view, transportFailureReason, (), (const)); - MOCK_METHOD(absl::optional, lastRoundTripTime, (), (const)); + DEFINE_MOCK_CONNECTION_MOCK_METHODS; // Network::ClientConnection MOCK_METHOD(void, connect, ()); diff --git a/test/server/connection_handler_test.cc b/test/server/connection_handler_test.cc index b286dc588dd2..ec36880a3665 100644 --- a/test/server/connection_handler_test.cc +++ b/test/server/connection_handler_test.cc @@ -359,7 +359,7 @@ TEST_F(ConnectionHandlerTest, ListenerConnectionLimitEnforced) { // For listener 1, verify connections are limited after one goes active. // First connection attempt should result in an active connection being created. - auto conn1 = new NiceMock(); + auto conn1 = new NiceMock(); EXPECT_CALL(dispatcher_, createServerConnection_()).WillOnce(Return(conn1)); listener_callbacks1->onAccept( Network::ConnectionSocketPtr{new NiceMock()}); @@ -514,7 +514,7 @@ TEST_F(ConnectionHandlerTest, CloseDuringFilterChainCreate) { handler_->addListener(absl::nullopt, *test_listener); EXPECT_CALL(manager_, findFilterChain(_)).WillOnce(Return(filter_chain_.get())); - Network::MockConnection* connection = new NiceMock(); + auto* connection = new NiceMock(); EXPECT_CALL(dispatcher_, createServerConnection_()).WillOnce(Return(connection)); EXPECT_CALL(factory_, createNetworkFilterChain(_, _)).WillOnce(Return(true)); EXPECT_CALL(*connection, state()).WillOnce(Return(Network::Connection::State::Closed)); @@ -538,7 +538,7 @@ TEST_F(ConnectionHandlerTest, CloseConnectionOnEmptyFilterChain) { handler_->addListener(absl::nullopt, *test_listener); EXPECT_CALL(manager_, findFilterChain(_)).WillOnce(Return(filter_chain_.get())); - Network::MockConnection* connection = new NiceMock(); + auto* connection = new NiceMock(); EXPECT_CALL(dispatcher_, createServerConnection_()).WillOnce(Return(connection)); EXPECT_CALL(factory_, createNetworkFilterChain(_, _)).WillOnce(Return(false)); EXPECT_CALL(*connection, close(Network::ConnectionCloseType::NoFlush)); @@ -592,7 +592,7 @@ TEST_F(ConnectionHandlerTest, NormalRedirect) { EXPECT_CALL(*accepted_socket, localAddressRestored()).WillOnce(Return(true)); EXPECT_CALL(*accepted_socket, localAddress()).WillRepeatedly(ReturnRef(alt_address)); EXPECT_CALL(manager_, findFilterChain(_)).WillOnce(Return(filter_chain_.get())); - Network::MockConnection* connection = new NiceMock(); + auto* connection = new NiceMock(); EXPECT_CALL(dispatcher_, createServerConnection_()).WillOnce(Return(connection)); EXPECT_CALL(factory_, createNetworkFilterChain(_, _)).WillOnce(Return(true)); listener_callbacks1->onAccept(Network::ConnectionSocketPtr{accepted_socket}); @@ -658,7 +658,7 @@ TEST_F(ConnectionHandlerTest, FallbackToWildcardListener) { EXPECT_CALL(*accepted_socket, localAddressRestored()).WillOnce(Return(true)); EXPECT_CALL(*accepted_socket, localAddress()).WillRepeatedly(ReturnRef(alt_address)); EXPECT_CALL(manager_, findFilterChain(_)).WillOnce(Return(filter_chain_.get())); - Network::MockConnection* connection = new NiceMock(); + auto* connection = new NiceMock(); EXPECT_CALL(dispatcher_, createServerConnection_()).WillOnce(Return(connection)); EXPECT_CALL(factory_, createNetworkFilterChain(_, _)).WillOnce(Return(true)); listener_callbacks1->onAccept(Network::ConnectionSocketPtr{accepted_socket}); @@ -702,7 +702,7 @@ TEST_F(ConnectionHandlerTest, WildcardListenerWithOriginalDst) { EXPECT_CALL(*accepted_socket, localAddressRestored()).WillOnce(Return(true)); EXPECT_CALL(*accepted_socket, localAddress()).WillRepeatedly(ReturnRef(original_dst_address)); EXPECT_CALL(manager_, findFilterChain(_)).WillOnce(Return(filter_chain_.get())); - Network::MockConnection* connection = new NiceMock(); + auto* connection = new NiceMock(); EXPECT_CALL(dispatcher_, createServerConnection_()).WillOnce(Return(connection)); EXPECT_CALL(factory_, createNetworkFilterChain(_, _)).WillOnce(Return(true)); listener_callbacks1->onAccept(Network::ConnectionSocketPtr{accepted_socket}); @@ -738,7 +738,7 @@ TEST_F(ConnectionHandlerTest, WildcardListenerWithNoOriginalDst) { EXPECT_CALL(*accepted_socket, localAddressRestored()).WillOnce(Return(false)); EXPECT_CALL(*accepted_socket, localAddress()).WillRepeatedly(ReturnRef(normal_address)); EXPECT_CALL(manager_, findFilterChain(_)).WillOnce(Return(filter_chain_.get())); - Network::MockConnection* connection = new NiceMock(); + auto* connection = new NiceMock(); EXPECT_CALL(dispatcher_, createServerConnection_()).WillOnce(Return(connection)); EXPECT_CALL(factory_, createNetworkFilterChain(_, _)).WillOnce(Return(true)); listener_callbacks1->onAccept(Network::ConnectionSocketPtr{accepted_socket}); @@ -1081,7 +1081,7 @@ TEST_F(ConnectionHandlerTest, TcpListenerRemoveFilterChain) { Network::MockConnectionSocket* connection = new NiceMock(); EXPECT_CALL(manager_, findFilterChain(_)).WillOnce(Return(filter_chain_.get())); - Network::MockConnection* server_connection = new NiceMock(); + auto* server_connection = new NiceMock(); EXPECT_CALL(dispatcher_, createServerConnection_()).WillOnce(Return(server_connection)); EXPECT_CALL(factory_, createNetworkFilterChain(_, _)).WillOnce(Return(true)); EXPECT_CALL(*access_log_, log(_, _, _, _)).Times(1); From f2cc49bd769ffe69aae050d5841e63665acd7d5d Mon Sep 17 00:00:00 2001 From: Alex Konradi Date: Wed, 14 Oct 2020 15:49:13 -0400 Subject: [PATCH 02/11] Support transport socket connect timeout Add code to the new ServerConnection class to support a timeout for the transport socket connect event. This will be used to require TLS connections to complete their handshake within a specified period of time. Signed-off-by: Alex Konradi --- include/envoy/network/connection.h | 10 +++- source/common/network/connection_impl.cc | 30 ++++++++++ source/common/network/connection_impl.h | 18 +++++- test/common/network/connection_impl_test.cc | 65 ++++++++++++++++++++- test/mocks/network/connection.h | 3 + 5 files changed, 122 insertions(+), 4 deletions(-) diff --git a/include/envoy/network/connection.h b/include/envoy/network/connection.h index aecb24f3df3e..2bb558afea64 100644 --- a/include/envoy/network/connection.h +++ b/include/envoy/network/connection.h @@ -322,7 +322,15 @@ using ConnectionPtr = std::unique_ptr; /** * Connections servicing inbound connects. */ -class ServerConnection : public virtual Connection {}; +class ServerConnection : public virtual Connection { +public: + /** + * Set the amount of time allowed for the transport socket to report that a connection is + * established. The provided timeout is relative to the current time. If the connection is already + * established, this has no effect. + */ + virtual void setTransportSocketConnectTimeout(std::chrono::milliseconds timeout) PURE; +}; using ServerConnectionPtr = std::unique_ptr; diff --git a/source/common/network/connection_impl.cc b/source/common/network/connection_impl.cc index 2804bad755ba..8af375f80d51 100644 --- a/source/common/network/connection_impl.cc +++ b/source/common/network/connection_impl.cc @@ -706,6 +706,36 @@ void ConnectionImpl::flushWriteBuffer() { } } +ServerConnectionImpl::ServerConnectionImpl(Event::Dispatcher& dispatcher, + ConnectionSocketPtr&& socket, + TransportSocketPtr&& transport_socket, + StreamInfo::StreamInfo& stream_info, bool connected) + : ConnectionImpl(dispatcher, std::move(socket), std::move(transport_socket), stream_info, + connected), + transport_socket_connect_timer_{ + dispatcher.createTimer([this] { onTransportSocketConnectTimeout(); })} {} + +void ServerConnectionImpl::setTransportSocketConnectTimeout(std::chrono::milliseconds timeout) { + if (transport_socket_connect_timer_ != nullptr) { + transport_socket_connect_timer_->enableTimer(timeout); + } +} + +void ServerConnectionImpl::raiseEvent(ConnectionEvent event) { + ConnectionImpl::raiseEvent(event); + switch (event) { + case ConnectionEvent::Connected: + case ConnectionEvent::RemoteClose: + case ConnectionEvent::LocalClose: + transport_socket_connect_timer_.reset(); + } +} + +void ServerConnectionImpl::onTransportSocketConnectTimeout() { + closeConnectionImmediately(); + stream_info_.setConnectionTerminationDetails("transport socket timeout was reached"); +} + ClientConnectionImpl::ClientConnectionImpl( Event::Dispatcher& dispatcher, const Address::InstanceConstSharedPtr& remote_address, const Network::Address::InstanceConstSharedPtr& source_address, diff --git a/source/common/network/connection_impl.h b/source/common/network/connection_impl.h index 761be98cd613..4f3d75124448 100644 --- a/source/common/network/connection_impl.h +++ b/source/common/network/connection_impl.h @@ -106,7 +106,7 @@ class ConnectionImpl : public ConnectionImplBase, public TransportSocketCallback IoHandle& ioHandle() final { return socket_->ioHandle(); } const IoHandle& ioHandle() const override { return socket_->ioHandle(); } Connection& connection() override { return *this; } - void raiseEvent(ConnectionEvent event) final; + void raiseEvent(ConnectionEvent event) override; // Should the read buffer be drained? bool shouldDrainReadBuffer() override { return read_buffer_limit_ > 0 && read_buffer_.length() >= read_buffer_limit_; @@ -200,7 +200,21 @@ class ConnectionImpl : public ConnectionImplBase, public TransportSocketCallback class ServerConnectionImpl : public ConnectionImpl, virtual public ServerConnection { public: - using ConnectionImpl::ConnectionImpl; + ServerConnectionImpl(Event::Dispatcher& dispatcher, ConnectionSocketPtr&& socket, + TransportSocketPtr&& transport_socket, StreamInfo::StreamInfo& stream_info, + bool connected); + + // ServerConnection impl + void setTransportSocketConnectTimeout(std::chrono::milliseconds timeout) override; + + void raiseEvent(ConnectionEvent event) override; + +private: + void onTransportSocketConnectTimeout(); + + // Implements a timeout for the transport socket signalling connection. The timer is enabled by a + // call to setTransportSocketConnectTimeout and is reset when the connection is established. + Event::TimerPtr transport_socket_connect_timer_; }; /** diff --git a/test/common/network/connection_impl_test.cc b/test/common/network/connection_impl_test.cc index fa7a178a79e2..29a2d14d2c12 100644 --- a/test/common/network/connection_impl_test.cc +++ b/test/common/network/connection_impl_test.cc @@ -36,9 +36,11 @@ using testing::_; using testing::AnyNumber; using testing::DoAll; using testing::Eq; +using testing::HasSubstr; using testing::InSequence; using testing::Invoke; using testing::InvokeWithoutArgs; +using testing::Optional; using testing::Return; using testing::SaveArg; using testing::Sequence; @@ -125,7 +127,7 @@ class ConnectionImplTest : public testing::TestWithParam { ConnectionImplTest() : api_(Api::createApiForTest(time_system_)), stream_info_(time_system_) {} void setUpBasicConnection() { - if (dispatcher_.get() == nullptr) { + if (dispatcher_ == nullptr) { dispatcher_ = api_->allocateDispatcher("test_thread"); } socket_ = std::make_shared( @@ -355,6 +357,67 @@ TEST_P(ConnectionImplTest, ImmediateConnectError) { dispatcher_->run(Event::Dispatcher::RunType::Block); } +TEST_P(ConnectionImplTest, SetServerTransportSocketTimeout) { + ConnectionMocks mocks = createConnectionMocks(false); + MockTransportSocket* transport_socket = mocks.transport_socket_.get(); + IoHandlePtr io_handle = std::make_unique(0); + + auto* mock_timer = new NiceMock(mocks.dispatcher_.get()); + auto server_connection = std::make_unique( + *mocks.dispatcher_, + std::make_unique(std::move(io_handle), nullptr, nullptr), + std::move(mocks.transport_socket_), stream_info_, true); + + EXPECT_CALL(*mock_timer, enableTimer(std::chrono::milliseconds(3 * 1000), _)); + + server_connection->setTransportSocketConnectTimeout(std::chrono::seconds(3)); + EXPECT_CALL(*transport_socket, closeSocket(ConnectionEvent::LocalClose)); + mock_timer->invokeCallback(); + EXPECT_THAT(stream_info_.connectionTerminationDetails(), + Optional(HasSubstr("transport socket timeout"))); +} + +TEST_P(ConnectionImplTest, SetServerTransportSocketTimeoutAfterConnect) { + ConnectionMocks mocks = createConnectionMocks(false); + MockTransportSocket* transport_socket = mocks.transport_socket_.get(); + IoHandlePtr io_handle = std::make_unique(0); + + auto* mock_timer = new NiceMock(mocks.dispatcher_.get()); + EXPECT_CALL(*mock_timer, enableTimer).Times(0); + auto server_connection = std::make_unique( + *mocks.dispatcher_, + std::make_unique(std::move(io_handle), nullptr, nullptr), + std::move(mocks.transport_socket_), stream_info_, true); + + transport_socket->callbacks_->raiseEvent(ConnectionEvent::Connected); + server_connection->setTransportSocketConnectTimeout(std::chrono::seconds(3)); + + server_connection->close(ConnectionCloseType::NoFlush); +} + +TEST_P(ConnectionImplTest, ServerTransportSocketTimeoutDisabledOnConnect) { + ConnectionMocks mocks = createConnectionMocks(false); + MockTransportSocket* transport_socket = mocks.transport_socket_.get(); + IoHandlePtr io_handle = std::make_unique(0); + + auto* mock_timer = new NiceMock(mocks.dispatcher_.get()); + auto server_connection = std::make_unique( + *mocks.dispatcher_, + std::make_unique(std::move(io_handle), nullptr, nullptr), + std::move(mocks.transport_socket_), stream_info_, true); + + bool timer_destroyed = false; + mock_timer->timer_destroyed_ = &timer_destroyed; + EXPECT_CALL(*mock_timer, enableTimer(std::chrono::milliseconds(3 * 1000), _)); + + server_connection->setTransportSocketConnectTimeout(std::chrono::seconds(3)); + + transport_socket->callbacks_->raiseEvent(ConnectionEvent::Connected); + EXPECT_TRUE(timer_destroyed); + + server_connection->close(ConnectionCloseType::NoFlush); +} + TEST_P(ConnectionImplTest, SocketOptions) { Network::ClientConnectionPtr upstream_connection_; diff --git a/test/mocks/network/connection.h b/test/mocks/network/connection.h index 900f4bd50544..f1afd5b87eca 100644 --- a/test/mocks/network/connection.h +++ b/test/mocks/network/connection.h @@ -97,6 +97,9 @@ class MockServerConnection : public ServerConnection, public MockConnectionBase ~MockServerConnection() override; DEFINE_MOCK_CONNECTION_MOCK_METHODS; + + // Network::ServerConnection + MOCK_METHOD(void, setTransportSocketConnectTimeout, (std::chrono::milliseconds)); }; /** From de4d2f9db123f1b83abfe2b85de771e12253323e Mon Sep 17 00:00:00 2001 From: Alex Konradi Date: Wed, 14 Oct 2020 17:33:06 -0400 Subject: [PATCH 03/11] Configure filter chain transport socket timeout Add a config field to the filter chain proto to specify the transport socket timeout and use that to set the value on the server socket. Signed-off-by: Alex Konradi --- .../listener/v3/listener_components.proto | 7 +++- .../v4alpha/listener_components.proto | 7 +++- .../listener/v3/listener_components.proto | 7 +++- .../v4alpha/listener_components.proto | 7 +++- include/envoy/network/filter.h | 7 ++++ source/server/admin/admin.h | 4 +++ source/server/connection_handler_impl.cc | 6 ++++ source/server/filter_chain_manager_impl.h | 10 ++++-- source/server/listener_manager_impl.cc | 13 ++++--- test/mocks/network/mocks.h | 1 + test/server/connection_handler_test.cc | 36 +++++++++++++++++-- test/test_common/network_utility.h | 4 +++ 12 files changed, 96 insertions(+), 13 deletions(-) diff --git a/api/envoy/config/listener/v3/listener_components.proto b/api/envoy/config/listener/v3/listener_components.proto index 3ecfc7932b56..c389c841e0ba 100644 --- a/api/envoy/config/listener/v3/listener_components.proto +++ b/api/envoy/config/listener/v3/listener_components.proto @@ -180,7 +180,7 @@ message FilterChainMatch { // A filter chain wraps a set of match criteria, an option TLS context, a set of filters, and // various other parameters. -// [#next-free-field: 9] +// [#next-free-field: 10] message FilterChain { option (udpa.annotations.versioning).previous_message_type = "envoy.api.v2.listener.FilterChain"; @@ -230,6 +230,11 @@ message FilterChain { // will be set up with plaintext. core.v3.TransportSocket transport_socket = 6; + // If present and nonzero, the amount of time to allow incoming connections to complete any + // transport socket negotiations. If this expires before the transport reports connection + // establishment, the connection is summarily closed. + google.protobuf.Duration transport_socket_connect_timeout = 9; + // [#not-implemented-hide:] The unique name (or empty) by which this filter chain is known. If no // name is provided, Envoy will allocate an internal UUID for the filter chain. If the filter // chain is to be dynamically updated or removed via FCDS a unique name must be provided. diff --git a/api/envoy/config/listener/v4alpha/listener_components.proto b/api/envoy/config/listener/v4alpha/listener_components.proto index 0c75f92b4027..e7fe84482475 100644 --- a/api/envoy/config/listener/v4alpha/listener_components.proto +++ b/api/envoy/config/listener/v4alpha/listener_components.proto @@ -180,7 +180,7 @@ message FilterChainMatch { // A filter chain wraps a set of match criteria, an option TLS context, a set of filters, and // various other parameters. -// [#next-free-field: 9] +// [#next-free-field: 10] message FilterChain { option (udpa.annotations.versioning).previous_message_type = "envoy.config.listener.v3.FilterChain"; @@ -234,6 +234,11 @@ message FilterChain { // will be set up with plaintext. core.v4alpha.TransportSocket transport_socket = 6; + // If present and nonzero, the amount of time to allow incoming connections to complete any + // transport socket negotiations. If this expires before the transport reports connection + // establishment, the connection is summarily closed. + google.protobuf.Duration transport_socket_connect_timeout = 9; + // [#not-implemented-hide:] The unique name (or empty) by which this filter chain is known. If no // name is provided, Envoy will allocate an internal UUID for the filter chain. If the filter // chain is to be dynamically updated or removed via FCDS a unique name must be provided. diff --git a/generated_api_shadow/envoy/config/listener/v3/listener_components.proto b/generated_api_shadow/envoy/config/listener/v3/listener_components.proto index 0d073197cabd..cb44a81459d2 100644 --- a/generated_api_shadow/envoy/config/listener/v3/listener_components.proto +++ b/generated_api_shadow/envoy/config/listener/v3/listener_components.proto @@ -181,7 +181,7 @@ message FilterChainMatch { // A filter chain wraps a set of match criteria, an option TLS context, a set of filters, and // various other parameters. -// [#next-free-field: 9] +// [#next-free-field: 10] message FilterChain { option (udpa.annotations.versioning).previous_message_type = "envoy.api.v2.listener.FilterChain"; @@ -227,6 +227,11 @@ message FilterChain { // will be set up with plaintext. core.v3.TransportSocket transport_socket = 6; + // If present and nonzero, the amount of time to allow incoming connections to complete any + // transport socket negotiations. If this expires before the transport reports connection + // establishment, the connection is summarily closed. + google.protobuf.Duration transport_socket_connect_timeout = 9; + // [#not-implemented-hide:] The unique name (or empty) by which this filter chain is known. If no // name is provided, Envoy will allocate an internal UUID for the filter chain. If the filter // chain is to be dynamically updated or removed via FCDS a unique name must be provided. diff --git a/generated_api_shadow/envoy/config/listener/v4alpha/listener_components.proto b/generated_api_shadow/envoy/config/listener/v4alpha/listener_components.proto index 0c75f92b4027..e7fe84482475 100644 --- a/generated_api_shadow/envoy/config/listener/v4alpha/listener_components.proto +++ b/generated_api_shadow/envoy/config/listener/v4alpha/listener_components.proto @@ -180,7 +180,7 @@ message FilterChainMatch { // A filter chain wraps a set of match criteria, an option TLS context, a set of filters, and // various other parameters. -// [#next-free-field: 9] +// [#next-free-field: 10] message FilterChain { option (udpa.annotations.versioning).previous_message_type = "envoy.config.listener.v3.FilterChain"; @@ -234,6 +234,11 @@ message FilterChain { // will be set up with plaintext. core.v4alpha.TransportSocket transport_socket = 6; + // If present and nonzero, the amount of time to allow incoming connections to complete any + // transport socket negotiations. If this expires before the transport reports connection + // establishment, the connection is summarily closed. + google.protobuf.Duration transport_socket_connect_timeout = 9; + // [#not-implemented-hide:] The unique name (or empty) by which this filter chain is known. If no // name is provided, Envoy will allocate an internal UUID for the filter chain. If the filter // chain is to be dynamically updated or removed via FCDS a unique name must be provided. diff --git a/include/envoy/network/filter.h b/include/envoy/network/filter.h index a111b1a22ed4..18f7bda54ad4 100644 --- a/include/envoy/network/filter.h +++ b/include/envoy/network/filter.h @@ -357,6 +357,13 @@ class FilterChain { */ virtual const TransportSocketFactory& transportSocketFactory() const PURE; + /** + * @return std::chrono::milliseconds the amount of time to wait for the transport socket to report + * that a connection has been established. If the timeout is reached, the connection is closed. 0 + * specifies a disabled timeout. + */ + virtual std::chrono::milliseconds transportSocketConnectTimeout() const PURE; + /** * const std::vector& a list of filters to be used by the new connection. */ diff --git a/source/server/admin/admin.h b/source/server/admin/admin.h index 945885ac3909..8174db71795a 100644 --- a/source/server/admin/admin.h +++ b/source/server/admin/admin.h @@ -383,6 +383,10 @@ class AdminImpl : public Admin, return transport_socket_factory_; } + std::chrono::milliseconds transportSocketConnectTimeout() const override { + return std::chrono::milliseconds::zero(); + } + const std::vector& networkFilterFactories() const override { return empty_network_filter_factory_; } diff --git a/source/server/connection_handler_impl.cc b/source/server/connection_handler_impl.cc index f819f5843c7b..a091dcc92f51 100644 --- a/source/server/connection_handler_impl.cc +++ b/source/server/connection_handler_impl.cc @@ -1,5 +1,7 @@ #include "server/connection_handler_impl.h" +#include + #include "envoy/event/dispatcher.h" #include "envoy/event/timer.h" #include "envoy/network/exception.h" @@ -475,6 +477,10 @@ void ConnectionHandlerImpl::ActiveTcpListener::newConnection( auto& active_connections = getOrCreateActiveConnections(*filter_chain); auto server_conn_ptr = parent_.dispatcher_.createServerConnection( std::move(socket), std::move(transport_socket), *stream_info); + if (const auto timeout = filter_chain->transportSocketConnectTimeout(); + timeout != std::chrono::milliseconds::zero()) { + server_conn_ptr->setTransportSocketConnectTimeout(timeout); + } ActiveTcpConnectionPtr active_connection( new ActiveTcpConnection(active_connections, std::move(server_conn_ptr), parent_.dispatcher_.timeSource(), std::move(stream_info))); diff --git a/source/server/filter_chain_manager_impl.h b/source/server/filter_chain_manager_impl.h index 3bcf01d2ec2e..5649c60eaa13 100644 --- a/source/server/filter_chain_manager_impl.h +++ b/source/server/filter_chain_manager_impl.h @@ -87,14 +87,19 @@ class PerFilterChainFactoryContextImpl : public Configuration::FilterChainFactor class FilterChainImpl : public Network::DrainableFilterChain { public: FilterChainImpl(Network::TransportSocketFactoryPtr&& transport_socket_factory, - std::vector&& filters_factory) + std::vector&& filters_factory, + std::chrono::milliseconds transport_socket_connect_timeout) : transport_socket_factory_(std::move(transport_socket_factory)), - filters_factory_(std::move(filters_factory)) {} + filters_factory_(std::move(filters_factory)), + transport_socket_connect_timeout_(transport_socket_connect_timeout) {} // Network::FilterChain const Network::TransportSocketFactory& transportSocketFactory() const override { return *transport_socket_factory_; } + std::chrono::milliseconds transportSocketConnectTimeout() const override { + return transport_socket_connect_timeout_; + } const std::vector& networkFilterFactories() const override { return filters_factory_; } @@ -110,6 +115,7 @@ class FilterChainImpl : public Network::DrainableFilterChain { Configuration::FilterChainFactoryContextPtr factory_context_; const Network::TransportSocketFactoryPtr transport_socket_factory_; const std::vector filters_factory_; + const std::chrono::milliseconds transport_socket_connect_timeout_; }; /** diff --git a/source/server/listener_manager_impl.cc b/source/server/listener_manager_impl.cc index d2d81a57a914..fb17e6810ed2 100644 --- a/source/server/listener_manager_impl.cc +++ b/source/server/listener_manager_impl.cc @@ -1021,11 +1021,14 @@ Network::DrainableFilterChainSharedPtr ListenerFilterChainFactoryBuilder::buildF std::vector server_names(filter_chain.filter_chain_match().server_names().begin(), filter_chain.filter_chain_match().server_names().end()); - auto filter_chain_res = - std::make_unique(config_factory.createTransportSocketFactory( - *message, factory_context_, std::move(server_names)), - listener_component_factory_.createNetworkFilterFactoryList( - filter_chain.filters(), *filter_chain_factory_context)); + auto filter_chain_res = std::make_unique( + config_factory.createTransportSocketFactory(*message, factory_context_, + std::move(server_names)), + listener_component_factory_.createNetworkFilterFactoryList(filter_chain.filters(), + *filter_chain_factory_context), + std::chrono::milliseconds( + PROTOBUF_GET_MS_OR_DEFAULT(filter_chain, transport_socket_connect_timeout, 0))); + filter_chain_res->setFilterChainFactoryContext(std::move(filter_chain_factory_context)); return filter_chain_res; } diff --git a/test/mocks/network/mocks.h b/test/mocks/network/mocks.h index de7b843a72d0..78a3863e5468 100644 --- a/test/mocks/network/mocks.h +++ b/test/mocks/network/mocks.h @@ -187,6 +187,7 @@ class MockFilterChain : public DrainableFilterChain { // Network::DrainableFilterChain MOCK_METHOD(const TransportSocketFactory&, transportSocketFactory, (), (const)); + MOCK_METHOD(std::chrono::milliseconds, transportSocketConnectTimeout, (), (const)); MOCK_METHOD(const std::vector&, networkFilterFactories, (), (const)); MOCK_METHOD(void, startDraining, ()); }; diff --git a/test/server/connection_handler_test.cc b/test/server/connection_handler_test.cc index ec36880a3665..9a04887ec63e 100644 --- a/test/server/connection_handler_test.cc +++ b/test/server/connection_handler_test.cc @@ -33,6 +33,7 @@ using testing::InSequence; using testing::Invoke; using testing::NiceMock; using testing::Return; +using testing::ReturnPointee; using testing::ReturnRef; using testing::SaveArg; @@ -45,9 +46,14 @@ class ConnectionHandlerTest : public testing::Test, protected Logger::Loggable()), handler_(new ConnectionHandlerImpl(dispatcher_, 0)), - filter_chain_(Network::Test::createEmptyFilterChainWithRawBufferSockets()), + filter_chain_(std::make_shared>()), listener_filter_matcher_(std::make_shared>()), access_log_(std::make_shared()) { + ON_CALL(*filter_chain_, transportSocketFactory) + .WillByDefault(ReturnPointee(std::shared_ptr{ + Network::Test::createRawBufferSocketFactory()})); + ON_CALL(*filter_chain_, networkFilterFactories) + .WillByDefault(ReturnPointee(std::make_shared>())); ON_CALL(*listener_filter_matcher_, matches(_)).WillByDefault(Return(false)); } @@ -249,7 +255,7 @@ class ConnectionHandlerTest : public testing::Test, protected Logger::Loggable manager_; NiceMock factory_; - const Network::FilterChainSharedPtr filter_chain_; + const std::shared_ptr filter_chain_; NiceMock os_sys_calls_; TestThreadsafeSingletonInjector os_calls_{&os_sys_calls_}; std::shared_ptr> listener_filter_matcher_; @@ -483,6 +489,32 @@ TEST_F(ConnectionHandlerTest, AddListenerSetRejectFraction) { handler_->addListener(absl::nullopt, *test_listener); } +TEST_F(ConnectionHandlerTest, SetsTransportSocketConnectTimeout) { + InSequence s; + + Network::TcpListenerCallbacks* listener_callbacks; + auto listener = new NiceMock(); + TestListener* test_listener = + addListener(1, false, false, "test_listener", listener, &listener_callbacks); + + EXPECT_CALL(*socket_factory_, localAddress()).WillOnce(ReturnRef(local_address_)); + handler_->addListener(absl::nullopt, *test_listener); + + auto server_connection = new NiceMock(); + + EXPECT_CALL(manager_, findFilterChain(_)).WillOnce(Return(filter_chain_.get())); + EXPECT_CALL(dispatcher_, createServerConnection_()).WillOnce(Return(server_connection)); + EXPECT_CALL(*filter_chain_, transportSocketConnectTimeout) + .WillOnce(Return(std::chrono::seconds(5))); + EXPECT_CALL(*server_connection, + setTransportSocketConnectTimeout(std::chrono::milliseconds(5 * 1000))); + EXPECT_CALL(*access_log_, log(_, _, _, _)).Times(1); + + listener_callbacks->onAccept(std::make_unique>()); + + EXPECT_CALL(*listener, onDestroy()); +} + TEST_F(ConnectionHandlerTest, DestroyCloseConnections) { InSequence s; diff --git a/test/test_common/network_utility.h b/test/test_common/network_utility.h index d1d8d4cf32fa..02f745a7e665 100644 --- a/test/test_common/network_utility.h +++ b/test/test_common/network_utility.h @@ -144,6 +144,10 @@ class EmptyFilterChain : public FilterChain { return *transport_socket_factory_; } + std::chrono::milliseconds transportSocketConnectTimeout() const override { + return std::chrono::milliseconds::zero(); + } + const std::vector& networkFilterFactories() const override { return empty_network_filter_factory_; } From 5de8f0e4257ca0493dda101fdce4cbb40bcbc5f4 Mon Sep 17 00:00:00 2001 From: Alex Konradi Date: Fri, 16 Oct 2020 11:51:37 -0400 Subject: [PATCH 04/11] Add release notes Signed-off-by: Alex Konradi --- docs/root/version_history/current.rst | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 71567c6cda82..6b05231fadbf 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -32,6 +32,7 @@ New Features * health_check: added option to use :ref:`no_traffic_healthy_interval ` which allows a different no traffic interval when the host is healthy. * listener: added an optional :ref:`default filter chain `. If this field is supplied, and none of the :ref:`filter_chains ` matches, this default filter chain is used to serve the connection. * mongo_proxy: the list of commands to produce metrics for is now :ref:`configurable `. +* network: added a :ref:`timeout ` for transport socket reporting connection establishment. * ratelimit: added support for use of various :ref:`metadata ` as a ratelimit action. * ratelimit: added :ref:`disable_x_envoy_ratelimited_header ` option to disable `X-Envoy-RateLimited` header. * tcp: added a new :ref:`envoy.overload_actions.reject_incoming_connections ` action to reject incoming TCP connections. From c5c64c825f709408c2e97cd72d91a3003c8ddf32 Mon Sep 17 00:00:00 2001 From: Alex Konradi Date: Fri, 16 Oct 2020 14:48:21 -0400 Subject: [PATCH 05/11] Fix spelling Signed-off-by: Alex Konradi --- source/common/network/connection_impl.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/common/network/connection_impl.h b/source/common/network/connection_impl.h index 4f3d75124448..f341347f35e9 100644 --- a/source/common/network/connection_impl.h +++ b/source/common/network/connection_impl.h @@ -212,7 +212,7 @@ class ServerConnectionImpl : public ConnectionImpl, virtual public ServerConnect private: void onTransportSocketConnectTimeout(); - // Implements a timeout for the transport socket signalling connection. The timer is enabled by a + // Implements a timeout for the transport socket signaling connection. The timer is enabled by a // call to setTransportSocketConnectTimeout and is reset when the connection is established. Event::TimerPtr transport_socket_connect_timer_; }; From aad8beea7da691f533fc044a2dc28bfc8acf4870 Mon Sep 17 00:00:00 2001 From: Alex Konradi Date: Mon, 19 Oct 2020 11:30:13 -0400 Subject: [PATCH 06/11] Address review feedback Signed-off-by: Alex Konradi --- docs/root/faq/configuration/timeouts.rst | 7 +++++++ docs/root/version_history/current.rst | 2 +- include/envoy/network/connection.h | 4 ++-- source/common/network/connection_impl.cc | 14 +++++++++----- source/common/network/connection_impl.h | 2 +- test/common/network/connection_impl_test.cc | 4 +--- 6 files changed, 21 insertions(+), 12 deletions(-) diff --git a/docs/root/faq/configuration/timeouts.rst b/docs/root/faq/configuration/timeouts.rst index ee5d501acb54..ac17acf2ff90 100644 --- a/docs/root/faq/configuration/timeouts.rst +++ b/docs/root/faq/configuration/timeouts.rst @@ -107,3 +107,10 @@ TCP ` is the amount of time that the TCP proxy will allow a connection to exist with no upstream or downstream activity. The default idle timeout if not otherwise specified is *1 hour*. + +TLS / Transport Socket + +* The :ref:`transport_socket_connect_timeout ` + specifies the amount of time Envoy will wait for a downstream client to complete transport-level + negotiations. This can be used to limit the amount of time allowed to finish a TLS handshake + after establishing a TCP connection. \ No newline at end of file diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 6b05231fadbf..85d9cef707f0 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -32,7 +32,7 @@ New Features * health_check: added option to use :ref:`no_traffic_healthy_interval ` which allows a different no traffic interval when the host is healthy. * listener: added an optional :ref:`default filter chain `. If this field is supplied, and none of the :ref:`filter_chains ` matches, this default filter chain is used to serve the connection. * mongo_proxy: the list of commands to produce metrics for is now :ref:`configurable `. -* network: added a :ref:`timeout ` for transport socket reporting connection establishment. +* network: added a :ref:`timeout ` for incoming connections completing transport-level hanshaking. * ratelimit: added support for use of various :ref:`metadata ` as a ratelimit action. * ratelimit: added :ref:`disable_x_envoy_ratelimited_header ` option to disable `X-Envoy-RateLimited` header. * tcp: added a new :ref:`envoy.overload_actions.reject_incoming_connections ` action to reject incoming TCP connections. diff --git a/include/envoy/network/connection.h b/include/envoy/network/connection.h index 2bb558afea64..01d8313bf2d7 100644 --- a/include/envoy/network/connection.h +++ b/include/envoy/network/connection.h @@ -326,8 +326,8 @@ class ServerConnection : public virtual Connection { public: /** * Set the amount of time allowed for the transport socket to report that a connection is - * established. The provided timeout is relative to the current time. If the connection is already - * established, this has no effect. + * established. The provided timeout is relative to the current time. If this method is called + * after a connection has already been established, it is a no-op. */ virtual void setTransportSocketConnectTimeout(std::chrono::milliseconds timeout) PURE; }; diff --git a/source/common/network/connection_impl.cc b/source/common/network/connection_impl.cc index 8af375f80d51..4f008e159c31 100644 --- a/source/common/network/connection_impl.cc +++ b/source/common/network/connection_impl.cc @@ -711,14 +711,17 @@ ServerConnectionImpl::ServerConnectionImpl(Event::Dispatcher& dispatcher, TransportSocketPtr&& transport_socket, StreamInfo::StreamInfo& stream_info, bool connected) : ConnectionImpl(dispatcher, std::move(socket), std::move(transport_socket), stream_info, - connected), - transport_socket_connect_timer_{ - dispatcher.createTimer([this] { onTransportSocketConnectTimeout(); })} {} + connected) {} void ServerConnectionImpl::setTransportSocketConnectTimeout(std::chrono::milliseconds timeout) { - if (transport_socket_connect_timer_ != nullptr) { - transport_socket_connect_timer_->enableTimer(timeout); + if (!transport_connect_pending_) { + return; + } + if (transport_socket_connect_timer_ == nullptr) { + transport_socket_connect_timer_ = + dispatcher_.createTimer([this] { onTransportSocketConnectTimeout(); }); } + transport_socket_connect_timer_->enableTimer(timeout); } void ServerConnectionImpl::raiseEvent(ConnectionEvent event) { @@ -727,6 +730,7 @@ void ServerConnectionImpl::raiseEvent(ConnectionEvent event) { case ConnectionEvent::Connected: case ConnectionEvent::RemoteClose: case ConnectionEvent::LocalClose: + transport_connect_pending_ = false; transport_socket_connect_timer_.reset(); } } diff --git a/source/common/network/connection_impl.h b/source/common/network/connection_impl.h index f341347f35e9..1fe9e117e02b 100644 --- a/source/common/network/connection_impl.h +++ b/source/common/network/connection_impl.h @@ -206,12 +206,12 @@ class ServerConnectionImpl : public ConnectionImpl, virtual public ServerConnect // ServerConnection impl void setTransportSocketConnectTimeout(std::chrono::milliseconds timeout) override; - void raiseEvent(ConnectionEvent event) override; private: void onTransportSocketConnectTimeout(); + bool transport_connect_pending_{true}; // Implements a timeout for the transport socket signaling connection. The timer is enabled by a // call to setTransportSocketConnectTimeout and is reset when the connection is established. Event::TimerPtr transport_socket_connect_timer_; diff --git a/test/common/network/connection_impl_test.cc b/test/common/network/connection_impl_test.cc index 29a2d14d2c12..064f6896bca9 100644 --- a/test/common/network/connection_impl_test.cc +++ b/test/common/network/connection_impl_test.cc @@ -353,7 +353,6 @@ TEST_P(ConnectionImplTest, ImmediateConnectError) { // Verify that also the immediate connect errors generate a remote close event. EXPECT_CALL(client_callbacks_, onEvent(ConnectionEvent::RemoteClose)) .WillOnce(Invoke([&](Network::ConnectionEvent) -> void { dispatcher_->exit(); })); - dispatcher_->run(Event::Dispatcher::RunType::Block); } @@ -382,14 +381,13 @@ TEST_P(ConnectionImplTest, SetServerTransportSocketTimeoutAfterConnect) { MockTransportSocket* transport_socket = mocks.transport_socket_.get(); IoHandlePtr io_handle = std::make_unique(0); - auto* mock_timer = new NiceMock(mocks.dispatcher_.get()); - EXPECT_CALL(*mock_timer, enableTimer).Times(0); auto server_connection = std::make_unique( *mocks.dispatcher_, std::make_unique(std::move(io_handle), nullptr, nullptr), std::move(mocks.transport_socket_), stream_info_, true); transport_socket->callbacks_->raiseEvent(ConnectionEvent::Connected); + // This should be a no-op. server_connection->setTransportSocketConnectTimeout(std::chrono::seconds(3)); server_connection->close(ConnectionCloseType::NoFlush); From 7e7a2fcec297f173e08e6428b4555fb7cb316a97 Mon Sep 17 00:00:00 2001 From: Alex Konradi Date: Mon, 19 Oct 2020 15:27:26 -0400 Subject: [PATCH 07/11] Address feedback Signed-off-by: Alex Konradi --- docs/root/faq/configuration/timeouts.rst | 7 ++++--- source/common/network/connection_impl.cc | 2 +- test/common/network/connection_impl_test.cc | 4 ++-- 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/docs/root/faq/configuration/timeouts.rst b/docs/root/faq/configuration/timeouts.rst index ac17acf2ff90..1209221b1e32 100644 --- a/docs/root/faq/configuration/timeouts.rst +++ b/docs/root/faq/configuration/timeouts.rst @@ -108,9 +108,10 @@ TCP is the amount of time that the TCP proxy will allow a connection to exist with no upstream or downstream activity. The default idle timeout if not otherwise specified is *1 hour*. -TLS / Transport Socket +Transport Socket +---------------------- * The :ref:`transport_socket_connect_timeout ` specifies the amount of time Envoy will wait for a downstream client to complete transport-level - negotiations. This can be used to limit the amount of time allowed to finish a TLS handshake - after establishing a TCP connection. \ No newline at end of file + negotiations. When configured on a filter chain with a TLS or ALTS transport socket, this limits + the amount of time allowed to finish the encrypted handshake after establishing a TCP connection. \ No newline at end of file diff --git a/source/common/network/connection_impl.cc b/source/common/network/connection_impl.cc index 4f008e159c31..0f3b81175bc7 100644 --- a/source/common/network/connection_impl.cc +++ b/source/common/network/connection_impl.cc @@ -725,7 +725,6 @@ void ServerConnectionImpl::setTransportSocketConnectTimeout(std::chrono::millise } void ServerConnectionImpl::raiseEvent(ConnectionEvent event) { - ConnectionImpl::raiseEvent(event); switch (event) { case ConnectionEvent::Connected: case ConnectionEvent::RemoteClose: @@ -733,6 +732,7 @@ void ServerConnectionImpl::raiseEvent(ConnectionEvent event) { transport_connect_pending_ = false; transport_socket_connect_timer_.reset(); } + ConnectionImpl::raiseEvent(event); } void ServerConnectionImpl::onTransportSocketConnectTimeout() { diff --git a/test/common/network/connection_impl_test.cc b/test/common/network/connection_impl_test.cc index 064f6896bca9..ebc985313d11 100644 --- a/test/common/network/connection_impl_test.cc +++ b/test/common/network/connection_impl_test.cc @@ -368,7 +368,6 @@ TEST_P(ConnectionImplTest, SetServerTransportSocketTimeout) { std::move(mocks.transport_socket_), stream_info_, true); EXPECT_CALL(*mock_timer, enableTimer(std::chrono::milliseconds(3 * 1000), _)); - server_connection->setTransportSocketConnectTimeout(std::chrono::seconds(3)); EXPECT_CALL(*transport_socket, closeSocket(ConnectionEvent::LocalClose)); mock_timer->invokeCallback(); @@ -387,7 +386,8 @@ TEST_P(ConnectionImplTest, SetServerTransportSocketTimeoutAfterConnect) { std::move(mocks.transport_socket_), stream_info_, true); transport_socket->callbacks_->raiseEvent(ConnectionEvent::Connected); - // This should be a no-op. + // This should be a no-op. No timer should be created. + EXPECT_CALL(dispatcher, createTimer_(_)).Times(0); server_connection->setTransportSocketConnectTimeout(std::chrono::seconds(3)); server_connection->close(ConnectionCloseType::NoFlush); From 2498d535b4cbc8520d160304535b7c974ce01471 Mon Sep 17 00:00:00 2001 From: Alex Konradi Date: Tue, 20 Oct 2020 11:52:44 -0400 Subject: [PATCH 08/11] Fix build Signed-off-by: Alex Konradi --- test/common/network/connection_impl_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/common/network/connection_impl_test.cc b/test/common/network/connection_impl_test.cc index ebc985313d11..063ec4ee9af1 100644 --- a/test/common/network/connection_impl_test.cc +++ b/test/common/network/connection_impl_test.cc @@ -387,7 +387,7 @@ TEST_P(ConnectionImplTest, SetServerTransportSocketTimeoutAfterConnect) { transport_socket->callbacks_->raiseEvent(ConnectionEvent::Connected); // This should be a no-op. No timer should be created. - EXPECT_CALL(dispatcher, createTimer_(_)).Times(0); + EXPECT_CALL(*mocks.dispatcher_, createTimer_(_)).Times(0); server_connection->setTransportSocketConnectTimeout(std::chrono::seconds(3)); server_connection->close(ConnectionCloseType::NoFlush); From 78fcb302a49f650a06faed4f03facc5c1f69fb3b Mon Sep 17 00:00:00 2001 From: Alex Konradi Date: Wed, 21 Oct 2020 11:16:57 -0400 Subject: [PATCH 09/11] Address feedback Signed-off-by: Alex Konradi --- docs/root/faq/configuration/timeouts.rst | 18 +++++++++--------- docs/root/version_history/current.rst | 2 +- test/server/listener_manager_impl_test.cc | 18 ++++++++++++++++++ 3 files changed, 28 insertions(+), 10 deletions(-) diff --git a/docs/root/faq/configuration/timeouts.rst b/docs/root/faq/configuration/timeouts.rst index 1209221b1e32..5e33dac600c0 100644 --- a/docs/root/faq/configuration/timeouts.rst +++ b/docs/root/faq/configuration/timeouts.rst @@ -93,6 +93,14 @@ stream timeouts already introduced above. :ref:`max_stream_duration ` for individual routes as well as setting both limits and a fixed time offset on grpc-timeout headers. +Transport Socket +---------------- + +* The :ref:`transport_socket_connect_timeout ` + specifies the amount of time Envoy will wait for a downstream client to complete transport-level + negotiations. When configured on a filter chain with a TLS or ALTS transport socket, this limits + the amount of time allowed to finish the encrypted handshake after establishing a TCP connection. + TCP --- @@ -106,12 +114,4 @@ TCP * The TCP proxy :ref:`idle_timeout ` is the amount of time that the TCP proxy will allow a connection to exist with no upstream - or downstream activity. The default idle timeout if not otherwise specified is *1 hour*. - -Transport Socket ----------------------- - -* The :ref:`transport_socket_connect_timeout ` - specifies the amount of time Envoy will wait for a downstream client to complete transport-level - negotiations. When configured on a filter chain with a TLS or ALTS transport socket, this limits - the amount of time allowed to finish the encrypted handshake after establishing a TCP connection. \ No newline at end of file + or downstream activity. The default idle timeout if not otherwise specified is *1 hour*. \ No newline at end of file diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 85d9cef707f0..bb0f7840f104 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -32,7 +32,7 @@ New Features * health_check: added option to use :ref:`no_traffic_healthy_interval ` which allows a different no traffic interval when the host is healthy. * listener: added an optional :ref:`default filter chain `. If this field is supplied, and none of the :ref:`filter_chains ` matches, this default filter chain is used to serve the connection. * mongo_proxy: the list of commands to produce metrics for is now :ref:`configurable `. -* network: added a :ref:`timeout ` for incoming connections completing transport-level hanshaking. +* network: added a :ref:`timeout ` for incoming connections completing transport-level negotiation, including TLS and ALTS hanshakes. * ratelimit: added support for use of various :ref:`metadata ` as a ratelimit action. * ratelimit: added :ref:`disable_x_envoy_ratelimited_header ` option to disable `X-Envoy-RateLimited` header. * tcp: added a new :ref:`envoy.overload_actions.reject_incoming_connections ` action to reject incoming TCP connections. diff --git a/test/server/listener_manager_impl_test.cc b/test/server/listener_manager_impl_test.cc index bbf8d6b03a1f..38934ce66feb 100644 --- a/test/server/listener_manager_impl_test.cc +++ b/test/server/listener_manager_impl_test.cc @@ -311,6 +311,24 @@ TEST_F(ListenerManagerImplWithRealFiltersTest, DEPRECATED_FEATURE_TEST(TlsContex EXPECT_TRUE(filter_chain->transportSocketFactory().implementsSecureTransport()); } +TEST_F(ListenerManagerImplWithRealFiltersTest, TransportSocketConnectTimeout) { + const std::string yaml = R"EOF( +address: + socket_address: + address: 127.0.0.1 + port_value: 1234 +filter_chains: +- filters: [] + transport_socket_connect_timeout: 3s + )EOF"; + + EXPECT_CALL(listener_factory_, createListenSocket(_, _, _, {true})); + manager_->addOrUpdateListener(parseListenerFromV3Yaml(yaml), "", true); + auto filter_chain = findFilterChain(1234, "127.0.0.1", "", "", {}, "8.8.8.8", 111); + ASSERT_NE(filter_chain, nullptr); + EXPECT_EQ(filter_chain->transportSocketConnectTimeout(), std::chrono::seconds(3)); +} + TEST_F(ListenerManagerImplWithRealFiltersTest, UdpAddress) { EXPECT_CALL(*worker_, start(_)); manager_->startWorkers(guard_dog_); From a8daa82a40c3a64acb37dc7b2a4251bce52e2946 Mon Sep 17 00:00:00 2001 From: Alex Konradi Date: Wed, 21 Oct 2020 14:42:06 -0400 Subject: [PATCH 10/11] Add references to Transport Socket section Signed-off-by: Alex Konradi --- docs/root/faq/configuration/timeouts.rst | 26 +++++++++++++++--------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/docs/root/faq/configuration/timeouts.rst b/docs/root/faq/configuration/timeouts.rst index 5e33dac600c0..bdf42877c33b 100644 --- a/docs/root/faq/configuration/timeouts.rst +++ b/docs/root/faq/configuration/timeouts.rst @@ -31,6 +31,8 @@ Connection timeouts apply to the entire HTTP connection and all streams the conn :ref:`common_http_protocol_options ` field in the cluster configuration. +See :ref:`below ` for other connection timeouts. + Stream timeouts ^^^^^^^^^^^^^^^ @@ -93,14 +95,6 @@ stream timeouts already introduced above. :ref:`max_stream_duration ` for individual routes as well as setting both limits and a fixed time offset on grpc-timeout headers. -Transport Socket ----------------- - -* The :ref:`transport_socket_connect_timeout ` - specifies the amount of time Envoy will wait for a downstream client to complete transport-level - negotiations. When configured on a filter chain with a TLS or ALTS transport socket, this limits - the amount of time allowed to finish the encrypted handshake after establishing a TCP connection. - TCP --- @@ -110,8 +104,20 @@ TCP .. attention:: - For TLS connections, the connect timeout includes the TLS handshake. + For upstream TLS connections, the connect timeout includes the TLS handshake. For downstream + connections, see :ref:`below ` for configuration options. + * The TCP proxy :ref:`idle_timeout ` is the amount of time that the TCP proxy will allow a connection to exist with no upstream - or downstream activity. The default idle timeout if not otherwise specified is *1 hour*. \ No newline at end of file + or downstream activity. The default idle timeout if not otherwise specified is *1 hour*. + +.. _faq_configuration_timeouts_transport_socket: + +Transport Socket +---------------- + +* The :ref:`transport_socket_connect_timeout ` + specifies the amount of time Envoy will wait for a downstream client to complete transport-level + negotiations. When configured on a filter chain with a TLS or ALTS transport socket, this limits + the amount of time allowed to finish the encrypted handshake after establishing a TCP connection. \ No newline at end of file From 902d92e201d9978b0f7e01b48812616dcc93ce98 Mon Sep 17 00:00:00 2001 From: Alex Konradi Date: Tue, 27 Oct 2020 15:34:54 -0400 Subject: [PATCH 11/11] Address feedback Signed-off-by: Alex Konradi --- source/common/network/connection_impl.cc | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/source/common/network/connection_impl.cc b/source/common/network/connection_impl.cc index 0f3b81175bc7..890ec9756ffa 100644 --- a/source/common/network/connection_impl.cc +++ b/source/common/network/connection_impl.cc @@ -21,6 +21,12 @@ namespace Envoy { namespace Network { +namespace { + +constexpr absl::string_view kTransportSocketConnectTimeoutTerminationDetails = + "transport socket timeout was reached"; + +} void ConnectionImplUtility::updateBufferStats(uint64_t delta, uint64_t new_total, uint64_t& previous_total, Stats::Counter& stat_total, @@ -736,8 +742,8 @@ void ServerConnectionImpl::raiseEvent(ConnectionEvent event) { } void ServerConnectionImpl::onTransportSocketConnectTimeout() { + stream_info_.setConnectionTerminationDetails(kTransportSocketConnectTimeoutTerminationDetails); closeConnectionImmediately(); - stream_info_.setConnectionTerminationDetails("transport socket timeout was reached"); } ClientConnectionImpl::ClientConnectionImpl(