From d712a589f6fd383cfac028b025c3c0a0d9c84c50 Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Fri, 13 Aug 2021 21:42:28 +0000 Subject: [PATCH 01/31] socket: add get sockopt for connect Signed-off-by: Yuchen Dai --- source/extensions/io_socket/user_space/BUILD | 1 + .../io_socket/user_space/io_handle_impl.cc | 31 +++++++++++++--- .../io_socket/user_space/io_handle_impl.h | 2 + .../user_space/io_handle_impl_test.cc | 37 +++++++++++++++++++ 4 files changed, 66 insertions(+), 5 deletions(-) diff --git a/source/extensions/io_socket/user_space/BUILD b/source/extensions/io_socket/user_space/BUILD index 334fe51a916e4..01a5948f4c9e4 100644 --- a/source/extensions/io_socket/user_space/BUILD +++ b/source/extensions/io_socket/user_space/BUILD @@ -13,6 +13,7 @@ envoy_cc_extension( name = "config", srcs = ["config.h"], deps = [ + ":io_handle_impl_lib", ], ) diff --git a/source/extensions/io_socket/user_space/io_handle_impl.cc b/source/extensions/io_socket/user_space/io_handle_impl.cc index bbdb37e3b09ef..e079e985b0a1a 100644 --- a/source/extensions/io_socket/user_space/io_handle_impl.cc +++ b/source/extensions/io_socket/user_space/io_handle_impl.cc @@ -274,17 +274,38 @@ Network::IoHandlePtr IoHandleImpl::accept(struct sockaddr*, socklen_t*) { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; } -Api::SysCallIntResult IoHandleImpl::connect(Network::Address::InstanceConstSharedPtr) { - // Buffered Io handle should always be considered as connected. - // Use write or read to determine if peer is closed. - return {0, 0}; +Api::SysCallIntResult IoHandleImpl::connect(Network::Address::InstanceConstSharedPtr address) { + if (peer_handle_ != nullptr) { + // Buffered Io handle should always be considered as connected unless the server peer cannot be + // found. Use write or read to determine if peer is closed. + return {0, 0}; + } else { + ENVOY_LOG(debug, "user namespace handle {} connect to {} the closed peer.", + static_cast(this), address->asStringView()); + return Api::SysCallIntResult{-1, SOCKET_ERROR_INVAL}; + } } Api::SysCallIntResult IoHandleImpl::setOption(int, int, const void*, socklen_t) { return makeInvalidSyscallResult(); } -Api::SysCallIntResult IoHandleImpl::getOption(int, int, void*, socklen_t*) { +Api::SysCallIntResult IoHandleImpl::getOption(int level, int optname, void* optval, + socklen_t* optlen) { + // Check result of connect(). It is either connected or closed. + if (level == SOL_SOCKET && optname == SO_ERROR) { + if (peer_handle_ != nullptr) { + // The peer is valid at this comment. Consider it as connected. + *optlen = sizeof(int); + *static_cast(optval) = 0; + return Api::SysCallIntResult{0, 0}; + } else { + // The peer is closed. Reset the option value to non-zero. + *optlen = sizeof(int); + *static_cast(optval) = SOCKET_ERROR_INVAL; + return Api::SysCallIntResult{0, 0}; + } + } return makeInvalidSyscallResult(); } diff --git a/source/extensions/io_socket/user_space/io_handle_impl.h b/source/extensions/io_socket/user_space/io_handle_impl.h index 71d3248d47223..71d2161be17f3 100644 --- a/source/extensions/io_socket/user_space/io_handle_impl.h +++ b/source/extensions/io_socket/user_space/io_handle_impl.h @@ -143,6 +143,8 @@ class IoHandleImpl final : public Network::IoHandle, ASSERT(!peer_handle_); ASSERT(!write_shutdown_); peer_handle_ = writable_peer; + ENVOY_LOG(trace, "io handle {} set peer handle to {}.", static_cast(this), + static_cast(writable_peer)); } private: diff --git a/test/extensions/io_socket/user_space/io_handle_impl_test.cc b/test/extensions/io_socket/user_space/io_handle_impl_test.cc index fab734bb395d6..d75246348a8f3 100644 --- a/test/extensions/io_socket/user_space/io_handle_impl_test.cc +++ b/test/extensions/io_socket/user_space/io_handle_impl_test.cc @@ -20,6 +20,8 @@ namespace IoSocket { namespace UserSpace { namespace { +constexpr int CONNECTED = 0; + MATCHER(IsInvalidAddress, "") { return arg.err_->getErrorCode() == Api::IoError::IoErrorCode::NoSupport; } @@ -1007,6 +1009,41 @@ TEST_F(IoHandleImplTest, Connect) { auto address_is_ignored = std::make_shared("listener_id"); EXPECT_EQ(0, io_handle_->connect(address_is_ignored).return_value_); + + // Below is emulation of the connect(). + int immediate_error_value = -1; + socklen_t error_value_len = 0; + EXPECT_EQ(0, io_handle_->getOption(SOL_SOCKET, SO_ERROR, &immediate_error_value, &error_value_len) + .return_value_); + EXPECT_EQ(sizeof(int), error_value_len); + EXPECT_EQ(CONNECTED, immediate_error_value); + + // If the peer shutdown write but not yet closes, this io_handle should consider it + // as connected because the socket may be readable. + immediate_error_value = -1; + error_value_len = 0; + EXPECT_EQ(io_handle_peer_->shutdown(ENVOY_SHUT_WR).return_value_, 0); + EXPECT_EQ(0, io_handle_->getOption(SOL_SOCKET, SO_ERROR, &immediate_error_value, &error_value_len) + .return_value_); + EXPECT_EQ(sizeof(int), error_value_len); + EXPECT_EQ(CONNECTED, immediate_error_value); +} + +TEST_F(IoHandleImplTest, ConnectToClosedIoHandle) { + auto address_is_ignored = + std::make_shared("listener_id"); + io_handle_peer_->close(); + auto result = io_handle_->connect(address_is_ignored); + EXPECT_EQ(-1, result.return_value_); + EXPECT_EQ(SOCKET_ERROR_INVAL, result.errno_); + + // Below is emulation of the connect(). + int immediate_error_value = -1; + socklen_t error_value_len = 0; + EXPECT_EQ(0, io_handle_->getOption(SOL_SOCKET, SO_ERROR, &immediate_error_value, &error_value_len) + .return_value_); + EXPECT_EQ(sizeof(int), error_value_len); + EXPECT_NE(CONNECTED, immediate_error_value); } TEST_F(IoHandleImplTest, ActivateEvent) { From 1217008a5a33d36d851a282b6e95830b7a9da567 Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Wed, 18 Aug 2021 20:35:46 +0000 Subject: [PATCH 02/31] add inteneral listener Signed-off-by: Yuchen Dai --- source/server/active_internal_listener.cc | 194 +++++++++++++++++++ source/server/active_internal_listener.h | 89 +++++++++ test/server/active_internal_listener_test.cc | 187 ++++++++++++++++++ 3 files changed, 470 insertions(+) create mode 100644 source/server/active_internal_listener.cc create mode 100644 source/server/active_internal_listener.h create mode 100644 test/server/active_internal_listener_test.cc diff --git a/source/server/active_internal_listener.cc b/source/server/active_internal_listener.cc new file mode 100644 index 0000000000000..ed28746ab8e1e --- /dev/null +++ b/source/server/active_internal_listener.cc @@ -0,0 +1,194 @@ +#include "source/server/active_internal_listener.h" + +#include "envoy/network/filter.h" +#include "envoy/stats/scope.h" + +#include "source/common/network/address_impl.h" +#include "source/common/stats/timespan_impl.h" + +#include "active_stream_listener_base.h" + +namespace Envoy { +namespace Server { + +ActiveInternalListener::ActiveInternalListener(Network::ConnectionHandler& conn_handler, + Event::Dispatcher& dispatcher, + Network::ListenerConfig& config) + : OwnedActiveStreamListenerBase( + conn_handler, dispatcher, + std::make_unique(), config) {} +ActiveInternalListener::ActiveInternalListener(Network::ConnectionHandler& conn_handler, + Event::Dispatcher& dispatcher, + Network::ListenerPtr listener, + Network::ListenerConfig& config) + : OwnedActiveStreamListenerBase(conn_handler, dispatcher, std::move(listener), config) {} + +ActiveInternalListener::~ActiveInternalListener() { + is_deleting_ = true; + // Purge sockets that have not progressed to connections. This should only happen when + // a listener filter stops iteration and never resumes. + while (!sockets_.empty()) { + auto removed = sockets_.front()->removeFromList(sockets_); + dispatcher().deferredDelete(std::move(removed)); + } + + for (auto& [chain, active_connections] : connections_by_context_) { + ASSERT(active_connections != nullptr); + auto& connections = active_connections->connections_; + while (!connections.empty()) { + connections.front()->connection_->close(Network::ConnectionCloseType::NoFlush); + } + } + dispatcher().clearDeferredDeleteList(); +} + +// void ActiveInternalListener::removeConnection(ActiveInternalConnection& connection) { +// ENVOY_CONN_LOG(debug, "adding to cleanup list", *connection.connection_); +// ActiveConnections& active_connections = connection.active_connections_; +// ActiveInternalConnectionPtr removed = +// connection.removeFromList(active_connections.connections_); +// dispatcher().deferredDelete(std::move(removed)); +// // Delete map entry only iff connections becomes empty. +// if (active_connections.connections_.empty()) { +// auto iter = connections_by_context_.find(&active_connections.filter_chain_); +// ASSERT(iter != connections_by_context_.end()); +// // To cover the lifetime of every single connection, Connections need to be deferred deleted +// // because the previously contained connection is deferred deleted. +// dispatcher().deferredDelete(std::move(iter->second)); +// // The erase will break the iteration over the connections_by_context_ during the deletion. +// if (!is_deleting_) { +// connections_by_context_.erase(iter); +// } +// } +// } + +// void ActiveInternalListener::newConnection(Network::ConnectionSocketPtr&& socket, +// std::unique_ptr stream_info) { + +// // Find matching filter chain. +// const auto filter_chain = config_->filterChainManager().findFilterChain(*socket); +// if (filter_chain == nullptr) { +// ENVOY_LOG(debug, "closing connection: no matching filter chain found"); +// stats_.no_filter_chain_match_.inc(); +// stream_info->setResponseFlag(StreamInfo::ResponseFlag::NoRouteFound); +// stream_info->setResponseCodeDetails(StreamInfo::ResponseCodeDetails::get().FilterChainNotFound); +// emitLogs(*config_, *stream_info); +// socket->close(); +// return; +// } + +// stream_info->setFilterChainName(filter_chain->name()); +// auto transport_socket = filter_chain->transportSocketFactory().createTransportSocket(nullptr); +// stream_info->setDownstreamSslConnection(transport_socket->ssl()); +// auto& active_connections = getOrCreateActiveConnections(*filter_chain); +// auto server_conn_ptr = 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); +// } +// ActiveInternalConnectionPtr active_connection( +// new ActiveInternalConnection(active_connections, std::move(server_conn_ptr), +// dispatcher().timeSource(), std::move(stream_info))); +// active_connection->connection_->setBufferLimits(config_->perConnectionBufferLimitBytes()); + +// const bool empty_filter_chain = !config_->filterChainFactory().createNetworkFilterChain( +// *active_connection->connection_, filter_chain->networkFilterFactories()); +// if (empty_filter_chain) { +// ENVOY_CONN_LOG(debug, "closing connection: no filters", *active_connection->connection_); +// active_connection->connection_->close(Network::ConnectionCloseType::NoFlush); +// } + +// // If the connection is already closed, we can just let this connection immediately die. +// if (active_connection->connection_->state() != Network::Connection::State::Closed) { +// ENVOY_CONN_LOG(debug, "new connection", *active_connection->connection_); +// active_connection->connection_->addConnectionCallbacks(*active_connection); +// LinkedList::moveIntoList(std::move(active_connection), active_connections.connections_); +// } +// } + +void ActiveInternalListener::updateListenerConfig(Network::ListenerConfig& config) { + ENVOY_LOG(trace, "replacing listener ", config_->listenerTag(), " by ", config.listenerTag()); + config_ = &config; +} + +// ActiveInternalConnections::ActiveInternalConnections(ActiveInternalListener& listener, +// const Network::FilterChain& filter_chain) +// : listener_(listener), filter_chain_(filter_chain) {} + +// ActiveInternalConnections::~ActiveInternalConnections() { +// // connections should be defer deleted already. +// ASSERT(connections_.empty()); +// } + +// ActiveInternalConnection::ActiveInternalConnection( +// ActiveInternalConnections& active_connections, Network::ConnectionPtr&& new_connection, +// TimeSource& time_source, std::unique_ptr&& stream_info) +// : stream_info_(std::move(stream_info)), active_connections_(active_connections), +// connection_(std::move(new_connection)), +// conn_length_(new Stats::HistogramCompletableTimespanImpl( +// active_connections_.listener_.stats_.downstream_cx_length_ms_, time_source)) { +// // We just universally set no delay on connections. Theoretically we might at some point want +// // to make this configurable. +// connection_->noDelay(true); +// auto& listener = active_connections_.listener_; +// listener.stats_.downstream_cx_total_.inc(); +// listener.stats_.downstream_cx_active_.inc(); +// listener.per_worker_stats_.downstream_cx_total_.inc(); +// listener.per_worker_stats_.downstream_cx_active_.inc(); +// stream_info_->setConnectionID(connection_->id()); + +// // Active connections on the handler (not listener). The per listener connections have already +// // been incremented at this point either via the connection balancer or in the socket accept +// // path if there is no configured balancer. +// listener.parent_.incNumConnections(); +// } + +// ActiveInternalConnection::~ActiveInternalConnection() { +// ActiveInternalListener::emitLogs(*active_connections_.listener_.config_, *stream_info_); +// auto& listener = active_connections_.listener_; +// listener.stats_.downstream_cx_active_.dec(); +// listener.stats_.downstream_cx_destroy_.inc(); +// listener.per_worker_stats_.downstream_cx_active_.dec(); +// conn_length_->complete(); + +// // Active listener connections (not handler). +// listener.decNumConnections(); + +// // Active handler connections (not listener). +// listener.parent_.decNumConnections(); +// } + +void ActiveInternalListener::onAccept(Network::ConnectionSocketPtr&& socket) { + // Unlike tcp listener, no rebalancer is applied and won't call pickTargetHandler to account + // connections. + incNumConnections(); + + auto active_socket = std::make_unique( + *this, std::move(socket), false /* do not handle off at internal listener */); + // TODO(lambdai): restore address from either socket options, or from listener config. + active_socket->socket_->addressProvider().restoreLocalAddress( + std::make_shared("255.255.255.255", 0)); + active_socket->socket_->addressProvider().setRemoteAddress( + std::make_shared("255.255.255.254", 0)); + + onSocketAccepted(std::move(active_socket)); +} + +void ActiveInternalListener::newActiveConnection( + const Network::FilterChain& filter_chain, Network::ServerConnectionPtr server_conn_ptr, + std::unique_ptr stream_info) { + auto& active_connections = getOrCreateActiveConnections(filter_chain); + auto active_connection = + std::make_unique(active_connections, std::move(server_conn_ptr), + dispatcher().timeSource(), std::move(stream_info)); + // If the connection is already closed, we can just let this connection immediately die. + if (active_connection->connection_->state() != Network::Connection::State::Closed) { + ENVOY_CONN_LOG(debug, "new connection from {}", *active_connection->connection_, + active_connection->connection_->addressProvider().remoteAddress()->asString()); + active_connection->connection_->addConnectionCallbacks(*active_connection); + LinkedList::moveIntoList(std::move(active_connection), active_connections.connections_); + } +} +} // namespace Server +} // namespace Envoy diff --git a/source/server/active_internal_listener.h b/source/server/active_internal_listener.h new file mode 100644 index 0000000000000..a8d27388d6816 --- /dev/null +++ b/source/server/active_internal_listener.h @@ -0,0 +1,89 @@ +#pragma once +#include +#include +#include +#include + +#include "envoy/common/time.h" +#include "envoy/event/deferred_deletable.h" +#include "envoy/event/dispatcher.h" +#include "envoy/network/connection.h" +#include "envoy/network/connection_handler.h" +#include "envoy/network/filter.h" +#include "envoy/network/listen_socket.h" +#include "envoy/network/listener.h" +#include "envoy/server/listener_manager.h" +#include "envoy/stats/scope.h" +#include "envoy/stats/timespan.h" + +#include "source/common/common/linked_object.h" +#include "source/common/common/non_copyable.h" +#include "source/common/stream_info/stream_info_impl.h" +#include "source/server/active_stream_listener_base.h" + +#include "spdlog/spdlog.h" + +namespace Envoy { +namespace Server { + +class ActiveInternalListener : public OwnedActiveStreamListenerBase, + public Network::InternalListenerCallbacks { +public: + ActiveInternalListener(Network::ConnectionHandler& conn_handler, Event::Dispatcher& dispatcher, + Network::ListenerConfig& config); + ActiveInternalListener(Network::ConnectionHandler& conn_handler, Event::Dispatcher& dispatcher, + Network::ListenerPtr listener, Network::ListenerConfig& config); + ~ActiveInternalListener() override; + + class NetworkInternalListener : public Network::Listener { + + void disable() override { + // TODO(lambdai): think about how to elegantly disable internal listener. (Queue socket or + // close socket immediately?) + ENVOY_LOG(debug, "Warning: the internal listener cannot be disabled."); + } + + void enable() override { + ENVOY_LOG(debug, "Warning: the internal listener is always enabled."); + } + + void setRejectFraction(UnitFloat) override {} + }; + + Network::Listener* listener() override { return listener_.get(); } + + Network::BalancedConnectionHandlerOptRef + getBalancedHandlerByAddress(const Network::Address::Instance&) override { + NOT_IMPLEMENTED_GCOVR_EXCL_LINE; + } + + void pauseListening() override { + if (listener_ != nullptr) { + listener_->disable(); + } + } + void resumeListening() override { + if (listener_ != nullptr) { + listener_->enable(); + } + } + void shutdownListener() override { listener_.reset(); } + + // Network::InternalListenerCallbacks + void onAccept(Network::ConnectionSocketPtr&& socket) override; + + void incNumConnections() override { config_->openConnections().inc(); } + void decNumConnections() override { config_->openConnections().dec(); } + + void newActiveConnection(const Network::FilterChain& filter_chain, + Network::ServerConnectionPtr server_conn_ptr, + std::unique_ptr stream_info) override; + /** + * Update the listener config. The follow up connections will see the new config. The existing + * connections are not impacted. + */ + void updateListenerConfig(Network::ListenerConfig& config); +}; + +} // namespace Server +} // namespace Envoy diff --git a/test/server/active_internal_listener_test.cc b/test/server/active_internal_listener_test.cc new file mode 100644 index 0000000000000..71d170dba3d91 --- /dev/null +++ b/test/server/active_internal_listener_test.cc @@ -0,0 +1,187 @@ +#include + +#include "envoy/config/core/v3/base.pb.h" +#include "envoy/network/exception.h" +#include "envoy/network/filter.h" +#include "envoy/network/listener.h" +#include "envoy/stats/scope.h" + +#include "source/common/common/utility.h" +#include "source/common/config/utility.h" +#include "source/common/network/address_impl.h" +#include "source/common/network/connection_balancer_impl.h" +#include "source/common/network/io_socket_handle_impl.h" +#include "source/common/network/raw_buffer_socket.h" +#include "source/common/network/utility.h" +#include "source/server/active_internal_listener.h" +#include "source/server/connection_handler_impl.h" + +#include "test/mocks/access_log/mocks.h" +#include "test/mocks/api/mocks.h" +#include "test/mocks/common.h" +#include "test/mocks/network/mocks.h" +#include "test/test_common/network_utility.h" +#include "test/test_common/threadsafe_singleton_injector.h" + +#include "gmock/gmock.h" +#include "gtest/gtest.h" + +using testing::_; +using testing::Invoke; +using testing::NiceMock; +using testing::Return; +using testing::ReturnRef; + +namespace Envoy { +namespace Server { +namespace { + +class MockInternalListenerCallback : public Network::InternalListenerCallbacks { +public: + MOCK_METHOD(void, onAccept, (Network::ConnectionSocketPtr && socket), ()); + MOCK_METHOD(Event::Dispatcher&, dispatcher, ()); +}; +class ActiveInternalListenerTest : public testing::Test, + protected Logger::Loggable { +public: + ActiveInternalListenerTest() { + EXPECT_CALL(listener_config_, listenerScope).Times(testing::AnyNumber()); + EXPECT_CALL(conn_handler_, statPrefix()).WillRepeatedly(ReturnRef(listener_stat_prefix_)); + listener_filter_matcher_ = std::make_shared>(); + } + void addListener() { + EXPECT_CALL(listener_config_, listenerFiltersTimeout()); + EXPECT_CALL(listener_config_, continueOnListenerFiltersTimeout()); + EXPECT_CALL(listener_config_, filterChainManager()).WillRepeatedly(ReturnRef(manager_)); + EXPECT_CALL(listener_config_, openConnections()).WillRepeatedly(ReturnRef(resource_limit_)); + auto mock_listener_will_be_moved = std::make_unique(); + generic_listener_ = mock_listener_will_be_moved.get(); + internal_listener_ = std::make_shared( + conn_handler_, dispatcher_, std::move(mock_listener_will_be_moved), listener_config_); + } + Network::Listener* addListenerWithRealNetworkListener() { + EXPECT_CALL(listener_config_, listenerFiltersTimeout()); + EXPECT_CALL(listener_config_, continueOnListenerFiltersTimeout()); + EXPECT_CALL(listener_config_, filterChainManager()).WillRepeatedly(ReturnRef(manager_)); + EXPECT_CALL(listener_config_, openConnections()).WillRepeatedly(ReturnRef(resource_limit_)); + + internal_listener_ = + std::make_shared(conn_handler_, dispatcher_, listener_config_); + return internal_listener_->listener(); + } + void expectFilterChainFactory() { + EXPECT_CALL(listener_config_, filterChainFactory()) + .WillRepeatedly(ReturnRef(filter_chain_factory_)); + } + std::string listener_stat_prefix_{"listener_stat_prefix"}; + NiceMock dispatcher_{"test"}; + BasicResourceLimitImpl resource_limit_; + Network::MockConnectionHandler conn_handler_; + Network::MockListener* generic_listener_; + Network::MockListenerConfig listener_config_; + NiceMock manager_; + NiceMock filter_chain_factory_; + std::shared_ptr filter_chain_; + std::shared_ptr> listener_filter_matcher_; + std::shared_ptr internal_listener_; +}; + +TEST_F(ActiveInternalListenerTest, BasicInternalListener) { + addListener(); + EXPECT_CALL(*generic_listener_, onDestroy()); +} + +TEST_F(ActiveInternalListenerTest, AcceptSocketAndCreateListenerFilter) { + addListener(); + expectFilterChainFactory(); + Network::MockListenerFilter* test_listener_filter = new Network::MockListenerFilter(); + // FIX-ME: replace by mock socket + Network::Address::InstanceConstSharedPtr original_dst_address( + new Network::Address::Ipv4Instance("127.0.0.2", 8080)); + + Network::MockConnectionSocket* accepted_socket = new NiceMock(); + + EXPECT_CALL(filter_chain_factory_, createListenerFilterChain(_)) + .WillRepeatedly(Invoke([&](Network::ListenerFilterManager& manager) -> bool { + // Insert the Mock filter. + manager.addAcceptFilter(listener_filter_matcher_, + Network::ListenerFilterPtr{test_listener_filter}); + return true; + })); + EXPECT_CALL(*test_listener_filter, onAccept(_)) + .WillOnce(Invoke([&](Network::ListenerFilterCallbacks& cb) -> Network::FilterStatus { + cb.socket().addressProvider().restoreLocalAddress(original_dst_address); + return Network::FilterStatus::Continue; + })); + EXPECT_CALL(*test_listener_filter, destroy_()); + EXPECT_CALL(manager_, findFilterChain(_)).WillOnce(Return(nullptr)); + internal_listener_->onAccept(Network::ConnectionSocketPtr{accepted_socket}); + EXPECT_CALL(*generic_listener_, onDestroy()); +} + +TEST_F(ActiveInternalListenerTest, AcceptSocketAndCreateNetworkFilter) { + + addListener(); + expectFilterChainFactory(); + + Network::MockListenerFilter* test_listener_filter = new Network::MockListenerFilter(); + // FIX-ME: replace by mock socket + Network::Address::InstanceConstSharedPtr original_dst_address( + new Network::Address::Ipv4Instance("127.0.0.2", 8080)); + + Network::MockConnectionSocket* accepted_socket = new NiceMock(); + + EXPECT_CALL(filter_chain_factory_, createListenerFilterChain(_)) + .WillRepeatedly(Invoke([&](Network::ListenerFilterManager& manager) -> bool { + // Insert the Mock filter. + manager.addAcceptFilter(listener_filter_matcher_, + Network::ListenerFilterPtr{test_listener_filter}); + return true; + })); + EXPECT_CALL(*test_listener_filter, onAccept(_)) + .WillOnce(Invoke([&](Network::ListenerFilterCallbacks& cb) -> Network::FilterStatus { + cb.socket().addressProvider().restoreLocalAddress(original_dst_address); + return Network::FilterStatus::Continue; + })); + EXPECT_CALL(*test_listener_filter, destroy_()); + auto filter_factory_callback = std::make_shared>(); + filter_chain_ = std::make_shared>(); + auto transport_socket_factory = Network::Test::createRawBufferSocketFactory(); + + EXPECT_CALL(manager_, findFilterChain(_)).WillOnce(Return(filter_chain_.get())); + EXPECT_CALL(*filter_chain_, transportSocketFactory) + .WillOnce(testing::ReturnRef(*transport_socket_factory)); + EXPECT_CALL(*filter_chain_, networkFilterFactories).WillOnce(ReturnRef(*filter_factory_callback)); + auto* connection = new NiceMock(); + EXPECT_CALL(dispatcher_, createServerConnection_()).WillOnce(Return(connection)); + EXPECT_CALL(conn_handler_, incNumConnections()); + EXPECT_CALL(filter_chain_factory_, createNetworkFilterChain(_, _)).WillOnce(Return(true)); + EXPECT_CALL(listener_config_, perConnectionBufferLimitBytes()); + internal_listener_->onAccept(Network::ConnectionSocketPtr{accepted_socket}); + EXPECT_CALL(conn_handler_, decNumConnections()); + connection->close(Network::ConnectionCloseType::NoFlush); + dispatcher_.clearDeferredDeleteList(); + EXPECT_CALL(*generic_listener_, onDestroy()); +} + +TEST_F(ActiveInternalListenerTest, StopListener) { + addListener(); + EXPECT_CALL(*generic_listener_, onDestroy()); + internal_listener_->shutdownListener(); +} + +TEST_F(ActiveInternalListenerTest, PausedListenerAcceptNewSocket) { + addListenerWithRealNetworkListener(); + internal_listener_->pauseListening(); + + expectFilterChainFactory(); + Network::MockConnectionSocket* accepted_socket = new NiceMock(); + + EXPECT_CALL(filter_chain_factory_, createListenerFilterChain(_)) + .WillRepeatedly(Invoke([&](Network::ListenerFilterManager&) -> bool { return true; })); + EXPECT_CALL(manager_, findFilterChain(_)).WillOnce(Return(nullptr)); + internal_listener_->onAccept(Network::ConnectionSocketPtr{accepted_socket}); +} +} // namespace +} // namespace Server +} // namespace Envoy From b6737473ea3e40471a2d4b2b9e9ce9a65be7f3b4 Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Wed, 18 Aug 2021 20:36:13 +0000 Subject: [PATCH 03/31] add inteneral listener Signed-off-by: Yuchen Dai --- envoy/network/listener.h | 45 +++ source/common/network/BUILD | 1 + source/common/network/connection_impl.cc | 4 +- source/common/network/connection_impl.h | 1 - source/common/network/socket_impl.cc | 8 + source/common/network/utility.cc | 3 + source/common/runtime/runtime_features.cc | 1 + source/common/tcp_proxy/tcp_proxy.cc | 3 + source/common/tcp_proxy/tcp_proxy.h | 2 +- source/common/upstream/upstream_impl.cc | 4 +- source/server/BUILD | 32 ++ source/server/admin/admin.h | 3 + source/server/connection_handler_impl.cc | 42 ++- source/server/connection_handler_impl.h | 12 +- source/server/listener_impl.cc | 27 +- source/server/listener_impl.h | 13 + source/server/listener_manager_impl.cc | 12 +- test/common/network/BUILD | 1 + test/common/network/connection_impl_test.cc | 38 +++ test/config/utility.cc | 13 +- .../proxy_protocol_regression_test.cc | 3 + .../proxy_protocol/proxy_protocol_test.cc | 6 + test/integration/BUILD | 1 + test/integration/base_integration_test.h | 7 + test/integration/fake_upstream.h | 4 + test/integration/integration_tcp_client.cc | 1 - test/integration/server.cc | 10 + test/integration/server.h | 10 + .../socket_interface_integration_test.cc | 5 +- test/mocks/network/mocks.h | 1 + test/server/BUILD | 79 +++++ test/server/connection_handler_test.cc | 137 +++++++- test/server/listener_manager_impl_test.cc | 308 ++++++++++++++++++ 33 files changed, 814 insertions(+), 23 deletions(-) diff --git a/envoy/network/listener.h b/envoy/network/listener.h index ad2b69a6c7502..9aab1a8c0bdcb 100644 --- a/envoy/network/listener.h +++ b/envoy/network/listener.h @@ -106,6 +106,16 @@ class UdpListenerConfig { using UdpListenerConfigOptRef = OptRef; +/** + * Configuration for an internal listener. + */ +class InternalListenerConfig { +public: + virtual ~InternalListenerConfig() = default; +}; + +using InternalListenerConfigOptRef = OptRef; + /** * A configuration for an individual listener. */ @@ -184,6 +194,11 @@ class ListenerConfig { */ virtual UdpListenerConfigOptRef udpListenerConfig() PURE; + /** + * @return the internal configuration for the listener IFF it is an internal listener. + */ + virtual InternalListenerConfigOptRef internalListenerConfig() PURE; + /** * @return traffic direction of the listener. */ @@ -427,6 +442,36 @@ class UdpListener : public virtual Listener { using UdpListenerPtr = std::unique_ptr; +class InternalListenerCallbacks { +public: + virtual ~InternalListenerCallbacks() = default; + + /** + * Called when a new connection is accepted. + * @param socket supplies the socket that is moved into the callee. + */ + virtual void onAccept(ConnectionSocketPtr&& socket) PURE; + + // virtual Event::Dispatcher& dispatcher() PURE; +}; +using InternalListenerCallbacksOptRef = + absl::optional>; + +class InternalListener {}; + +using InternalListenerPtr = std::unique_ptr; +using InternalListenerOptRef = absl::optional>; + +class InternalListenerManager { +public: + virtual ~InternalListenerManager() = default; + virtual InternalListenerCallbacksOptRef + findByAddress(const Address::InstanceConstSharedPtr& listen_address) PURE; +}; + +using InternalListenerManagerOptRef = + absl::optional>; + /** * Handles delivering datagrams to the correct worker. */ diff --git a/source/common/network/BUILD b/source/common/network/BUILD index 529e25ad6c05d..d9df7a7e30a7c 100644 --- a/source/common/network/BUILD +++ b/source/common/network/BUILD @@ -64,6 +64,7 @@ envoy_cc_library( hdrs = ["connection_impl_base.h"], deps = [ ":filter_manager_lib", + ":listen_socket_lib", "//envoy/common:scope_tracker_interface", "//envoy/event:dispatcher_interface", "//source/common/common:assert_lib", diff --git a/source/common/network/connection_impl.cc b/source/common/network/connection_impl.cc index 6b6787ae9492a..ca565640d89a7 100644 --- a/source/common/network/connection_impl.cc +++ b/source/common/network/connection_impl.cc @@ -21,6 +21,7 @@ #include "source/common/network/listen_socket_impl.h" #include "source/common/network/raw_buffer_socket.h" #include "source/common/network/utility.h" +#include "source/common/runtime/runtime_features.h" namespace Envoy { namespace Network { @@ -280,7 +281,8 @@ void ConnectionImpl::noDelay(bool enable) { } // Don't set NODELAY for unix domain sockets - if (socket_->addressType() == Address::Type::Pipe) { + if (socket_->addressType() == Address::Type::Pipe || + socket_->addressType() == Address::Type::EnvoyInternal) { return; } diff --git a/source/common/network/connection_impl.h b/source/common/network/connection_impl.h index 242d7c926e9dc..b652596c09ada 100644 --- a/source/common/network/connection_impl.h +++ b/source/common/network/connection_impl.h @@ -241,7 +241,6 @@ class ClientConnectionImpl : public ConnectionImpl, virtual public ClientConnect const Address::InstanceConstSharedPtr& source_address, Network::TransportSocketPtr&& transport_socket, const Network::ConnectionSocket::OptionsSharedPtr& options); - // Network::ClientConnection void connect() override; diff --git a/source/common/network/socket_impl.cc b/source/common/network/socket_impl.cc index 76d1bd47c37cf..9f8381962c07e 100644 --- a/source/common/network/socket_impl.cc +++ b/source/common/network/socket_impl.cc @@ -23,10 +23,18 @@ SocketImpl::SocketImpl(IoHandlePtr&& io_handle, address_provider_(std::make_shared(local_address, remote_address)) { if (address_provider_->localAddress() != nullptr) { + // The remote address should have the exact local address type. + ASSERT(address_provider_->remoteAddress() == nullptr || + address_provider_->remoteAddress()->type() == address_provider_->localAddress()->type()); addr_type_ = address_provider_->localAddress()->type(); return; } + if (address_provider_->remoteAddress() != nullptr) { + addr_type_ = address_provider_->remoteAddress()->type(); + return; + } + // Should not happen but some tests inject -1 fds if (!io_handle_->isOpen()) { return; diff --git a/source/common/network/utility.cc b/source/common/network/utility.cc index 4145f3f4d5d68..2db017bc1257f 100644 --- a/source/common/network/utility.cc +++ b/source/common/network/utility.cc @@ -534,6 +534,9 @@ Utility::protobufAddressSocketType(const envoy::config::core::v3::Address& proto } case envoy::config::core::v3::Address::AddressCase::kPipe: return Socket::Type::Stream; + case envoy::config::core::v3::Address::AddressCase::kEnvoyInternalAddress: + // Currently internal address supports stream operation only. + return Socket::Type::Stream; default: NOT_REACHED_GCOVR_EXCL_LINE; } diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index 67cc2d136ae3b..7cf991f865217 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -74,6 +74,7 @@ constexpr const char* runtime_features[] = { "envoy.reloadable_features.http2_skip_encoding_empty_trailers", "envoy.reloadable_features.http_transport_failure_reason_in_body", "envoy.reloadable_features.improved_stream_limit_handling", + "envoy.reloadable_features.internal_address", "envoy.reloadable_features.internal_redirects_with_body", "envoy.reloadable_features.listener_reuse_port_default_enabled", "envoy.reloadable_features.listener_wildcard_match_ip_family", diff --git a/source/common/tcp_proxy/tcp_proxy.cc b/source/common/tcp_proxy/tcp_proxy.cc index a369af7358d16..76cfddea91d09 100644 --- a/source/common/tcp_proxy/tcp_proxy.cc +++ b/source/common/tcp_proxy/tcp_proxy.cc @@ -589,6 +589,8 @@ Network::FilterStatus Filter::onNewConnection() { } void Filter::onDownstreamEvent(Network::ConnectionEvent event) { + ENVOY_CONN_LOG(trace, "on downstream event {}, has upstream = {}", read_callbacks_->connection(), + static_cast(event), upstream_ == nullptr); if (upstream_) { Tcp::ConnectionPool::ConnectionDataPtr conn_data(upstream_->onDownstreamEvent(event)); if (conn_data != nullptr && @@ -757,6 +759,7 @@ Drainer::Drainer(UpstreamDrainManager& parent, const Config::SharedConfigSharedP const Upstream::HostDescriptionConstSharedPtr& upstream_host) : parent_(parent), callbacks_(callbacks), upstream_conn_data_(std::move(conn_data)), timer_(std::move(idle_timer)), upstream_host_(upstream_host), config_(config) { + ENVOY_CONN_LOG(debug, "draining the upstream connection", upstream_conn_data_->connection()); config_->stats().upstream_flush_total_.inc(); config_->stats().upstream_flush_active_.inc(); } diff --git a/source/common/tcp_proxy/tcp_proxy.h b/source/common/tcp_proxy/tcp_proxy.h index 7e22c3273bc62..7662802d9b2ee 100644 --- a/source/common/tcp_proxy/tcp_proxy.h +++ b/source/common/tcp_proxy/tcp_proxy.h @@ -389,7 +389,7 @@ class Filter : public Network::ReadFilter, // This class deals with an upstream connection that needs to finish flushing, when the downstream // connection has been closed. The TcpProxy is destroyed when the downstream connection is closed, // so handling the upstream connection here allows it to finish draining or timeout. -class Drainer : public Event::DeferredDeletable { +class Drainer : public Event::DeferredDeletable, protected Logger::Loggable { public: Drainer(UpstreamDrainManager& parent, const Config::SharedConfigSharedPtr& config, const std::shared_ptr& callbacks, diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index e0ed1d8058d4f..3751f410cbcc2 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -317,7 +317,9 @@ HostImpl::createConnection(Event::Dispatcher& dispatcher, const ClusterInfo& clu } else { connection_options = options; } - ASSERT(!address->envoyInternalAddress()); + if (!Runtime::runtimeFeatureEnabled("envoy.reloadable_features.internal_address")) { + ASSERT(!address->envoyInternalAddress()); + } Network::ClientConnectionPtr connection = dispatcher.createClientConnection( address, cluster.sourceAddress(), socket_factory.createTransportSocket(std::move(transport_socket_options)), diff --git a/source/server/BUILD b/source/server/BUILD index 630e61710f15a..cdcfcf3c1d5f7 100644 --- a/source/server/BUILD +++ b/source/server/BUILD @@ -64,6 +64,7 @@ envoy_cc_library( envoy_cc_library( name = "connection_handler_lib", deps = [ + ":active_internal_listener", ":active_tcp_listener", ":active_udp_listener", ":connection_handler_impl", @@ -77,6 +78,7 @@ envoy_cc_library( "connection_handler_impl.h", ], deps = [ + ":active_internal_listener", ":active_tcp_listener", "//envoy/common:time_interface", "//envoy/event:deferred_deletable", @@ -179,6 +181,36 @@ envoy_cc_library( ], ) +envoy_cc_library( + name = "active_internal_listener", + srcs = ["active_internal_listener.cc"], + hdrs = [ + "active_internal_listener.h", + ], + deps = [ + ":active_listener_base", + ":active_tcp_listener_headers", + "//envoy/common:time_interface", + "//envoy/event:deferred_deletable", + "//envoy/event:dispatcher_interface", + "//envoy/event:timer_interface", + "//envoy/network:connection_handler_interface", + "//envoy/network:connection_interface", + "//envoy/network:exception_interface", + "//envoy/network:filter_interface", + "//envoy/network:listen_socket_interface", + "//envoy/network:listener_interface", + "//envoy/server:listener_manager_interface", + "//envoy/stats:timespan_interface", + "//source/common/common:linked_object", + "//source/common/common:non_copyable", + "//source/common/event:deferred_task", + "//source/common/network:connection_lib", + "//source/common/stats:timespan_lib", + "//source/common/stream_info:stream_info_lib", + ], +) + envoy_cc_library( name = "active_tcp_socket", srcs = ["active_tcp_socket.cc"], diff --git a/source/server/admin/admin.h b/source/server/admin/admin.h index de0cf7a55e2ab..48fdbc0cf669a 100644 --- a/source/server/admin/admin.h +++ b/source/server/admin/admin.h @@ -360,6 +360,9 @@ class AdminImpl : public Admin, Network::UdpListenerConfigOptRef udpListenerConfig() override { return Network::UdpListenerConfigOptRef(); } + Network::InternalListenerConfigOptRef internalListenerConfig() override { + return Network::InternalListenerConfigOptRef(); + } envoy::config::core::v3::TrafficDirection direction() const override { return envoy::config::core::v3::UNSPECIFIED; } diff --git a/source/server/connection_handler_impl.cc b/source/server/connection_handler_impl.cc index 4f64afc227b06..af2bc999a5ef2 100644 --- a/source/server/connection_handler_impl.cc +++ b/source/server/connection_handler_impl.cc @@ -5,9 +5,11 @@ #include "envoy/event/dispatcher.h" #include "envoy/network/filter.h" +#include "source/common/common/logger.h" #include "source/common/event/deferred_task.h" #include "source/common/network/utility.h" #include "source/common/runtime/runtime_features.h" +#include "source/server/active_internal_listener.h" #include "source/server/active_tcp_listener.h" namespace Envoy { @@ -28,7 +30,20 @@ void ConnectionHandlerImpl::decNumConnections() { void ConnectionHandlerImpl::addListener(absl::optional overridden_listener, Network::ListenerConfig& config) { ActiveListenerDetails details; - if (config.listenSocketFactory().socketType() == Network::Socket::Type::Stream) { + if (config.internalListenerConfig().has_value()) { + if (overridden_listener.has_value()) { + for (auto& listener : listeners_) { + if (listener.second.listener_->listenerTag() == overridden_listener) { + listener.second.internalListener()->get().updateListenerConfig(config); + return; + } + } + NOT_REACHED_GCOVR_EXCL_LINE; + } + auto internal_listener = std::make_unique(*this, dispatcher(), config); + details.typed_listener_ = *internal_listener; + details.listener_ = std::move(internal_listener); + } else if (config.listenSocketFactory().socketType() == Network::Socket::Type::Stream) { if (overridden_listener.has_value()) { for (auto& listener : listeners_) { if (listener.second.listener_->listenerTag() == overridden_listener) { @@ -134,6 +149,25 @@ void ConnectionHandlerImpl::setListenerRejectFraction(UnitFloat reject_fraction) } } +Network::InternalListenerCallbacksOptRef +ConnectionHandlerImpl::findByAddress(const Network::Address::InstanceConstSharedPtr& address) { + auto listener_it = + std::find_if(listeners_.begin(), listeners_.end(), + [&address](std::pair& p) { + return p.second.internalListener().has_value() && + p.second.listener_->listener() != nullptr && + p.first->type() == Network::Address::Type::EnvoyInternal && + *(p.first) == *address; + }); + + if (listener_it != listeners_.end()) { + return Network::InternalListenerCallbacksOptRef( + listener_it->second.internalListener().value().get()); + } + return absl::nullopt; +} + ConnectionHandlerImpl::ActiveTcpListenerOptRef ConnectionHandlerImpl::ActiveListenerDetails::tcpListener() { auto* val = absl::get_if>(&typed_listener_); @@ -146,6 +180,12 @@ ConnectionHandlerImpl::ActiveListenerDetails::udpListener() { return (val != nullptr) ? absl::make_optional(*val) : absl::nullopt; } +ConnectionHandlerImpl::ActiveInternalListenerOptRef +ConnectionHandlerImpl::ActiveListenerDetails::internalListener() { + auto* val = absl::get_if>(&typed_listener_); + return (val != nullptr) ? absl::make_optional(*val) : absl::nullopt; +} + ConnectionHandlerImpl::ActiveListenerDetailsOptRef ConnectionHandlerImpl::findActiveListenerByTag(uint64_t listener_tag) { // TODO(mattklein123): We should probably use a hash table here to lookup the tag diff --git a/source/server/connection_handler_impl.h b/source/server/connection_handler_impl.h index f10424b7c1a2d..c58543a1d8fc4 100644 --- a/source/server/connection_handler_impl.h +++ b/source/server/connection_handler_impl.h @@ -21,6 +21,7 @@ namespace Server { class ActiveUdpListenerBase; class ActiveTcpListener; +class ActiveInternalListener; /** * Server side connection handler. This is used both by workers as well as the @@ -28,12 +29,15 @@ class ActiveTcpListener; */ class ConnectionHandlerImpl : public Network::TcpConnectionHandler, public Network::UdpConnectionHandler, + public Network::InternalListenerManager, NonCopyable, Logger::Loggable { public: using UdpListenerCallbacksOptRef = absl::optional>; using ActiveTcpListenerOptRef = absl::optional>; + using ActiveInternalListenerOptRef = + absl::optional>; ConnectionHandlerImpl(Event::Dispatcher& dispatcher, absl::optional worker_index); @@ -63,18 +67,24 @@ class ConnectionHandlerImpl : public Network::TcpConnectionHandler, // Network::UdpConnectionHandler Network::UdpListenerCallbacksOptRef getUdpListenerCallbacks(uint64_t listener_tag) override; + // Network::InternalListenerManager + Network::InternalListenerCallbacksOptRef + findByAddress(const Network::Address::InstanceConstSharedPtr& listen_address) override; + private: struct ActiveListenerDetails { // Strong pointer to the listener, whether TCP, UDP, QUIC, etc. Network::ConnectionHandler::ActiveListenerPtr listener_; absl::variant, - std::reference_wrapper> + std::reference_wrapper, + std::reference_wrapper> typed_listener_; // Helpers for accessing the data in the variant for cleaner code. ActiveTcpListenerOptRef tcpListener(); UdpListenerCallbacksOptRef udpListener(); + ActiveInternalListenerOptRef internalListener(); }; using ActiveListenerDetailsOptRef = absl::optional>; ActiveListenerDetailsOptRef findActiveListenerByTag(uint64_t listener_tag); diff --git a/source/server/listener_impl.cc b/source/server/listener_impl.cc index cd36b26627376..3f57233bdde1b 100644 --- a/source/server/listener_impl.cc +++ b/source/server/listener_impl.cc @@ -74,12 +74,16 @@ ListenSocketFactoryImpl::ListenSocketFactoryImpl( ASSERT(bind_type_ == ListenerComponentFactory::BindType::ReusePort); } } else { - ASSERT(local_address_->type() == Network::Address::Type::Pipe); - // Listeners with Unix domain socket always use shared socket. - // TODO(mattklein123): This should be blocked at the config parsing layer instead of getting - // here and disabling reuse_port. - if (bind_type_ == ListenerComponentFactory::BindType::ReusePort) { - bind_type_ = ListenerComponentFactory::BindType::NoReusePort; + if (local_address_->type() == Network::Address::Type::Pipe) { + // Listeners with Unix domain socket always use shared socket. + // TODO(mattklein123): This should be blocked at the config parsing layer instead of getting + // here and disabling reuse_port. + if (bind_type_ == ListenerComponentFactory::BindType::ReusePort) { + bind_type_ = ListenerComponentFactory::BindType::NoReusePort; + } + } else { + ASSERT(local_address_->type() == Network::Address::Type::EnvoyInternal); + bind_type_ = ListenerComponentFactory::BindType::NoBind; } } @@ -335,6 +339,7 @@ ListenerImpl::ListenerImpl(const envoy::config::listener::v3::Listener& config, buildSocketOptions(); buildOriginalDstListenerFilter(); buildProxyProtocolListenerFilter(); + buildInternalListener(); } if (!workers_started_) { // Initialize dynamic_init_manager_ from Server's init manager if it's not initialized. @@ -362,7 +367,8 @@ ListenerImpl::ListenerImpl(ListenerImpl& origin, validation_visitor_( added_via_api_ ? parent_.server_.messageValidationContext().dynamicValidationVisitor() : parent_.server_.messageValidationContext().staticValidationVisitor()), - // listener_init_target_ is not used during in place update because we expect server started. + // listener_init_target_ is not used during in place update because we expect server + // started. listener_init_target_("", nullptr), dynamic_init_manager_(std::make_unique( fmt::format("Listener-local-init-manager {} {}", name, hash))), @@ -395,6 +401,7 @@ ListenerImpl::ListenerImpl(ListenerImpl& origin, buildSocketOptions(); buildOriginalDstListenerFilter(); buildProxyProtocolListenerFilter(); + buildInternalListener(); open_connections_ = origin.open_connections_; } @@ -406,6 +413,12 @@ void ListenerImpl::buildAccessLog() { } } +void ListenerImpl::buildInternalListener() { + if (config_.has_internal_listener()) { + internal_listener_config_ = std::make_unique(); + } +} + void ListenerImpl::buildUdpListenerFactory(Network::Socket::Type socket_type, uint32_t concurrency) { if (socket_type != Network::Socket::Type::Datagram) { diff --git a/source/server/listener_impl.h b/source/server/listener_impl.h index effa7368f4681..333cd2e6b3507 100644 --- a/source/server/listener_impl.h +++ b/source/server/listener_impl.h @@ -312,6 +312,10 @@ class ListenerImpl final : public Network::ListenerConfig, return udp_listener_config_ != nullptr ? *udp_listener_config_ : Network::UdpListenerConfigOptRef(); } + Network::InternalListenerConfigOptRef internalListenerConfig() override { + return internal_listener_config_ != nullptr ? *internal_listener_config_ + : Network::InternalListenerConfigOptRef(); + } Network::ConnectionBalancer& connectionBalancer() override { return *connection_balancer_; } ResourceLimit& openConnections() override { return *open_connections_; } const std::vector& accessLogs() const override { @@ -358,6 +362,13 @@ class ListenerImpl final : public Network::ListenerConfig, Network::UdpListenerWorkerRouterPtr listener_worker_router_; }; + struct InternalListenerConfigImpl : public Network::InternalListenerConfig { + InternalListenerConfigImpl( + const envoy::config::listener::v3::Listener_InternalListenerConfig config) + : config_(config) {} + const envoy::config::listener::v3::Listener_InternalListenerConfig config_; + }; + /** * Create a new listener from an existing listener and the new config message if the in place * filter chain update is decided. Should be called only by newListenerWithFilterChain(). @@ -368,6 +379,7 @@ class ListenerImpl final : public Network::ListenerConfig, uint32_t concurrency); // Helpers for constructor. void buildAccessLog(); + void buildInternalListener(); void buildUdpListenerFactory(Network::Socket::Type socket_type, uint32_t concurrency); void buildListenSocketOptions(Network::Socket::Type socket_type); void createListenerFilterFactories(Network::Socket::Type socket_type); @@ -413,6 +425,7 @@ class ListenerImpl final : public Network::ListenerConfig, const std::chrono::milliseconds listener_filters_timeout_; const bool continue_on_listener_filters_timeout_; std::unique_ptr udp_listener_config_; + std::unique_ptr internal_listener_config_; Network::ConnectionBalancerSharedPtr connection_balancer_; std::shared_ptr listener_factory_context_; FilterChainManagerImpl filter_chain_manager_; diff --git a/source/server/listener_manager_impl.cc b/source/server/listener_manager_impl.cc index dedebf1684a0a..a5a5ecec8e89f 100644 --- a/source/server/listener_manager_impl.cc +++ b/source/server/listener_manager_impl.cc @@ -338,11 +338,13 @@ ListenerManagerStats ListenerManagerImpl::generateStats(Stats::Scope& scope) { bool ListenerManagerImpl::addOrUpdateListener(const envoy::config::listener::v3::Listener& config, const std::string& version_info, bool added_via_api) { - RELEASE_ASSERT( - !config.address().has_envoy_internal_address(), - fmt::format("listener {} has envoy internal address {}. Internal address cannot be used by " - "listener yet", - config.name(), config.address().envoy_internal_address().DebugString())); + if (!Runtime::runtimeFeatureEnabled("envoy.reloadable_features.internal_address")) { + RELEASE_ASSERT( + !config.address().has_envoy_internal_address(), + fmt::format("listener {} has envoy internal address {}. Internal address cannot be used by " + "listener yet", + config.name(), config.address().envoy_internal_address().DebugString())); + } // TODO(junr03): currently only one ApiListener can be installed via bootstrap to avoid having to // build a collection of listeners, and to have to be able to warm and drain the listeners. In the // future allow multiple ApiListeners, and allow them to be created via LDS as well as bootstrap. diff --git a/test/common/network/BUILD b/test/common/network/BUILD index ce5f097cd5f5e..c0ed193b61f12 100644 --- a/test/common/network/BUILD +++ b/test/common/network/BUILD @@ -91,6 +91,7 @@ envoy_cc_test( "//test/test_common:environment_lib", "//test/test_common:network_utility_lib", "//test/test_common:simulated_time_system_lib", + "//test/test_common:test_runtime_lib", "//test/test_common:test_time_lib", "//test/test_common:threadsafe_singleton_injector_lib", "@envoy_api//envoy/config/core/v3:pkg_cc_proto", diff --git a/test/common/network/connection_impl_test.cc b/test/common/network/connection_impl_test.cc index 279cefbb0b3bd..982c849972ec1 100644 --- a/test/common/network/connection_impl_test.cc +++ b/test/common/network/connection_impl_test.cc @@ -6,6 +6,7 @@ #include "envoy/config/core/v3/base.pb.h" #include "envoy/event/scaled_range_timer_manager.h" #include "envoy/network/address.h" +#include "envoy/network/listener.h" #include "source/common/api/os_sys_calls_impl.h" #include "source/common/buffer/buffer_impl.h" @@ -29,6 +30,7 @@ #include "test/test_common/network_utility.h" #include "test/test_common/printers.h" #include "test/test_common/simulated_time_system.h" +#include "test/test_common/test_runtime.h" #include "test/test_common/threadsafe_singleton_injector.h" #include "test/test_common/utility.h" @@ -53,6 +55,12 @@ namespace Envoy { namespace Network { namespace { +class MockInternalListenerManager : public InternalListenerManager { +public: + MOCK_METHOD(InternalListenerCallbacksOptRef, findByAddress, + (const Address::InstanceConstSharedPtr&), ()); +}; + TEST(RawBufferSocket, TestBasics) { TransportSocketPtr raw_buffer_socket(Network::Test::createRawBufferSocket()); EXPECT_FALSE(raw_buffer_socket->ssl()); @@ -2953,6 +2961,36 @@ TEST_F(PipeClientConnectionImplTest, SkipSourceAddress) { connection->close(ConnectionCloseType::NoFlush); } +class InternalClientConnectionImplTest : public testing::Test { +protected: + InternalClientConnectionImplTest() + : api_(Api::createApiForTest()), dispatcher_(api_->allocateDispatcher("test_thread")) {} + + Api::ApiPtr api_; + Event::DispatcherPtr dispatcher_; + StrictMock client_callbacks_; +}; + +TEST_F(InternalClientConnectionImplTest, + CannotCreateConnectionToInternalAddressWithInternalAddressEnabled) { + auto scoped_runtime_guard = std::make_unique(); + Runtime::LoaderSingleton::getExisting()->mergeValues( + {{"envoy.reloadable_features.internal_address", "true"}}); + + const Network::SocketInterface* sock_interface = Network::socketInterface( + "envoy.extensions.network.socket_interface.default_socket_interface"); + Network::Address::InstanceConstSharedPtr address = + std::make_shared("listener_0", sock_interface); + // Not implemented yet. + ASSERT_DEATH( + { + ClientConnectionPtr connection = + dispatcher_->createClientConnection(address, Network::Address::InstanceConstSharedPtr(), + Network::Test::createRawBufferSocket(), nullptr); + }, + "panic: not implemented"); +} + } // namespace } // namespace Network } // namespace Envoy diff --git a/test/config/utility.cc b/test/config/utility.cc index ac21e0fc31644..09c880aba3325 100644 --- a/test/config/utility.cc +++ b/test/config/utility.cc @@ -600,6 +600,18 @@ ConfigHelper::ConfigHelper(const Network::Address::IpVersion version, Api::Api& auto* static_resources = bootstrap_.mutable_static_resources(); for (int i = 0; i < static_resources->listeners_size(); ++i) { auto* listener = static_resources->mutable_listeners(i); + if (listener->mutable_address()->has_envoy_internal_address()) { + ENVOY_LOG_MISC( + debug, "Listener {} has internal address {}. Will not reset to loop back socket address.", + i, listener->mutable_address()->envoy_internal_address().server_listener_name()); + continue; + } + if (listener->mutable_address()->has_pipe()) { + ENVOY_LOG_MISC(debug, + "Listener {} has pipe address {}. Will not reset to loop back socket address.", + i, listener->mutable_address()->pipe().path()); + continue; + } auto* listener_socket_addr = listener->mutable_address()->mutable_socket_address(); if (listener_socket_addr->address() == "0.0.0.0" || listener_socket_addr->address() == "::") { listener_socket_addr->set_address(Network::Test::getAnyAddressString(version)); @@ -926,7 +938,6 @@ void ConfigHelper::setDefaultHostAndRoute(const std::string& domains, const std: void ConfigHelper::setBufferLimits(uint32_t upstream_buffer_limit, uint32_t downstream_buffer_limit) { RELEASE_ASSERT(!finalized_, ""); - RELEASE_ASSERT(bootstrap_.mutable_static_resources()->listeners_size() == 1, ""); auto* listener = bootstrap_.mutable_static_resources()->mutable_listeners(0); listener->mutable_per_connection_buffer_limit_bytes()->set_value(downstream_buffer_limit); const uint32_t stream_buffer_size = std::max( diff --git a/test/extensions/common/proxy_protocol/proxy_protocol_regression_test.cc b/test/extensions/common/proxy_protocol/proxy_protocol_regression_test.cc index aea16f0433be4..62d6d8b245495 100644 --- a/test/extensions/common/proxy_protocol/proxy_protocol_regression_test.cc +++ b/test/extensions/common/proxy_protocol/proxy_protocol_regression_test.cc @@ -72,6 +72,9 @@ class ProxyProtocolRegressionTest : public testing::TestWithParam { upstream_config_.http2_options_.MergeFrom(options); } + Event::Dispatcher* getWorkerDispatcher(uint32_t index) { + if (test_server_ == nullptr) { + return nullptr; + } + return &test_server_->getWorkerDispatcher(index).value().get(); + } + std::unique_ptr upstream_stats_store_; // Make sure the test server will be torn down after any fake client. diff --git a/test/integration/fake_upstream.h b/test/integration/fake_upstream.h index 2758cbad861c2..c002f100b4c94 100644 --- a/test/integration/fake_upstream.h +++ b/test/integration/fake_upstream.h @@ -13,6 +13,7 @@ #include "envoy/network/connection.h" #include "envoy/network/connection_handler.h" #include "envoy/network/filter.h" +#include "envoy/network/listener.h" #include "envoy/stats/scope.h" #include "source/common/buffer/buffer_impl.h" @@ -780,6 +781,9 @@ class FakeUpstream : Logger::Loggable, uint64_t listenerTag() const override { return 0; } const std::string& name() const override { return name_; } Network::UdpListenerConfigOptRef udpListenerConfig() override { return udp_listener_config_; } + Network::InternalListenerConfigOptRef internalListenerConfig() override { + return Network::InternalListenerConfigOptRef(); + } Network::ConnectionBalancer& connectionBalancer() override { return connection_balancer_; } envoy::config::core::v3::TrafficDirection direction() const override { return envoy::config::core::v3::UNSPECIFIED; diff --git a/test/integration/integration_tcp_client.cc b/test/integration/integration_tcp_client.cc index 2396d74c06da4..0e145ae12c838 100644 --- a/test/integration/integration_tcp_client.cc +++ b/test/integration/integration_tcp_client.cc @@ -53,7 +53,6 @@ IntegrationTcpClient::IntegrationTcpClient( std::function above_overflow) -> Buffer::Instance* { return new Buffer::WatermarkBuffer(below_low, above_high, above_overflow); })); - ; connection_ = dispatcher.createClientConnection( Network::Utility::resolveUrl( diff --git a/test/integration/server.cc b/test/integration/server.cc index 2325ccc592b6f..827cc2d622347 100644 --- a/test/integration/server.cc +++ b/test/integration/server.cc @@ -241,6 +241,16 @@ void IntegrationTestServerImpl::createAndRunEnvoyServer( admin_address_ = server.admin().socket().addressProvider().localAddress(); server_ = &server; stat_store_ = &stat_store; + // Use the tls slots to save dispatchers. The slot must be destroyed before server shutdown. + ThreadLocal::SlotPtr dispatcher_tls = tls.allocateSlot(); + + dispatcher_tls->set( + [this](Event::Dispatcher& dispatcher) -> ThreadLocal::ThreadLocalObjectSharedPtr { + Thread::LockGuard guard(dispatcher_mutex_); + registered_dispatchers_.push_back(dispatcher); + return nullptr; + }); + serverReady(); server.run(); } diff --git a/test/integration/server.h b/test/integration/server.h index 7e5ddd9fa93dc..c919602e1b7ea 100644 --- a/test/integration/server.h +++ b/test/integration/server.h @@ -515,6 +515,12 @@ class IntegrationTestServer : public Logger::Loggable, void useAdminInterfaceToQuit(bool use) { use_admin_interface_to_quit_ = use; } bool useAdminInterfaceToQuit() { return use_admin_interface_to_quit_; } + absl::optional> getWorkerDispatcher(uint32_t index) { + Thread::LockGuard guard(dispatcher_mutex_); + ASSERT(index < registered_dispatchers_.size()); + return registered_dispatchers_[index]; + } + protected: IntegrationTestServer(Event::TestTimeSystem& time_system, Api::Api& api, const std::string& config_path) @@ -537,6 +543,10 @@ class IntegrationTestServer : public Logger::Loggable, // adminAddress()) will be available. void serverReady(); + Thread::MutexBasicLockable dispatcher_mutex_; + // Assume 128 is greater than concurrency. + std::vector>> registered_dispatchers_; + private: /** * Runs the real server on a thread. diff --git a/test/integration/socket_interface_integration_test.cc b/test/integration/socket_interface_integration_test.cc index 63a531a415753..d4f78da877888 100644 --- a/test/integration/socket_interface_integration_test.cc +++ b/test/integration/socket_interface_integration_test.cc @@ -90,7 +90,6 @@ TEST_P(SocketInterfaceIntegrationTest, AddressWithSocketInterface) { } // Test that connecting to internal address will crash. -// TODO(lambdai): Add internal connection implementation to enable the connection creation. TEST_P(SocketInterfaceIntegrationTest, InternalAddressWithSocketInterface) { BaseIntegrationTest::initialize(); @@ -101,6 +100,8 @@ TEST_P(SocketInterfaceIntegrationTest, InternalAddressWithSocketInterface) { Network::Address::InstanceConstSharedPtr address = std::make_shared("listener_0", sock_interface); + Runtime::LoaderSingleton::getExisting()->mergeValues( + {{"envoy.reloadable_features.internal_address", "false"}}); ASSERT_DEATH(client_ = dispatcher_->createClientConnection( address, Network::Address::InstanceConstSharedPtr(), Network::Test::createRawBufferSocket(), nullptr), @@ -108,7 +109,7 @@ TEST_P(SocketInterfaceIntegrationTest, InternalAddressWithSocketInterface) { } // Test that recv from internal address will crash. -// TODO(lambdai): Add internal socket implementation to enable the io path. +// TODO(lambdai): Add UDP internal listener implementation to enable the io path. TEST_P(SocketInterfaceIntegrationTest, UdpRecvFromInternalAddressWithSocketInterface) { BaseIntegrationTest::initialize(); diff --git a/test/mocks/network/mocks.h b/test/mocks/network/mocks.h index ce971d0b92cd3..c1b8c624d3f16 100644 --- a/test/mocks/network/mocks.h +++ b/test/mocks/network/mocks.h @@ -406,6 +406,7 @@ class MockListenerConfig : public ListenerConfig { MOCK_METHOD(uint64_t, listenerTag, (), (const)); MOCK_METHOD(const std::string&, name, (), (const)); MOCK_METHOD(Network::UdpListenerConfigOptRef, udpListenerConfig, ()); + MOCK_METHOD(InternalListenerConfigOptRef, internalListenerConfig, ()); MOCK_METHOD(ConnectionBalancer&, connectionBalancer, ()); MOCK_METHOD(ResourceLimit&, openConnections, ()); MOCK_METHOD(uint32_t, tcpBacklogSize, (), (const)); diff --git a/test/server/BUILD b/test/server/BUILD index 5e50e3cbacb4d..7156ea5ede82a 100644 --- a/test/server/BUILD +++ b/test/server/BUILD @@ -74,20 +74,48 @@ envoy_cc_test( name = "connection_handler_test", srcs = ["connection_handler_test.cc"], deps = [ + ":utility_lib", + "//source/common/api:os_sys_calls_lib", "//source/common/common:utility_lib", + "//source/common/config:metadata_lib", "//source/common/config:utility_lib", + "//source/common/init:manager_lib", + "//source/common/network:addr_family_aware_socket_option_lib", "//source/common/network:address_lib", "//source/common/network:connection_balancer_lib", + "//source/common/network:listen_socket_lib", + "//source/common/network:socket_option_lib", "//source/common/network:udp_packet_writer_handler_lib", + "//source/common/network:utility_lib", + "//source/common/protobuf", "//source/common/stats:stats_lib", + "//source/extensions/filters/listener/original_dst:config", + "//source/extensions/filters/listener/proxy_protocol:config", + "//source/extensions/filters/listener/tls_inspector:config", + "//source/extensions/filters/network/http_connection_manager:config", + "//source/extensions/filters/network/tcp_proxy:config", + "//source/extensions/request_id/uuid:config", + "//source/extensions/transport_sockets/raw_buffer:config", + "//source/extensions/transport_sockets/tls:config", + "//source/extensions/transport_sockets/tls:ssl_socket_lib", "//source/server:active_raw_udp_listener_config", "//source/server:connection_handler_lib", + "//source/server:listener_manager_lib", "//test/mocks/access_log:access_log_mocks", "//test/mocks/api:api_mocks", + "//test/mocks/init:init_mocks", "//test/mocks/network:network_mocks", + "//test/mocks/server:drain_manager_mocks", + "//test/mocks/server:guard_dog_mocks", + "//test/mocks/server:instance_mocks", + "//test/mocks/server:listener_component_factory_mocks", + "//test/mocks/server:worker_factory_mocks", + "//test/mocks/server:worker_mocks", + "//test/test_common:environment_lib", "//test/test_common:network_utility_lib", "//test/test_common:test_runtime_lib", "//test/test_common:threadsafe_singleton_injector_lib", + "@envoy_api//envoy/admin/v3:pkg_cc_proto", "@envoy_api//envoy/config/core/v3:pkg_cc_proto", "@envoy_api//envoy/config/listener/v3:pkg_cc_proto", ], @@ -113,6 +141,57 @@ envoy_cc_test( ], ) +envoy_cc_test( + name = "active_internal_listener_test", + srcs = ["active_internal_listener_test.cc"], + deps = [ + ":utility_lib", + "//source/common/api:os_sys_calls_lib", + "//source/common/common:utility_lib", + "//source/common/config:metadata_lib", + "//source/common/config:utility_lib", + "//source/common/init:manager_lib", + "//source/common/network:addr_family_aware_socket_option_lib", + "//source/common/network:address_lib", + "//source/common/network:connection_balancer_lib", + "//source/common/network:listen_socket_lib", + "//source/common/network:socket_option_lib", + "//source/common/network:utility_lib", + "//source/common/protobuf", + "//source/common/stats:stats_lib", + "//source/extensions/filters/listener/original_dst:config", + "//source/extensions/filters/listener/proxy_protocol:config", + "//source/extensions/filters/listener/tls_inspector:config", + "//source/extensions/filters/network/http_connection_manager:config", + "//source/extensions/filters/network/tcp_proxy:config", + "//source/extensions/request_id/uuid:config", + "//source/extensions/transport_sockets/raw_buffer:config", + "//source/extensions/transport_sockets/tls:config", + "//source/extensions/transport_sockets/tls:ssl_socket_lib", + "//source/server:active_raw_udp_listener_config", + "//source/server:connection_handler_lib", + "//source/server:listener_manager_lib", + "//test/mocks/access_log:access_log_mocks", + "//test/mocks/api:api_mocks", + "//test/mocks/init:init_mocks", + "//test/mocks/network:network_mocks", + "//test/mocks/server:drain_manager_mocks", + "//test/mocks/server:guard_dog_mocks", + "//test/mocks/server:instance_mocks", + "//test/mocks/server:listener_component_factory_mocks", + "//test/mocks/server:worker_factory_mocks", + "//test/mocks/server:worker_mocks", + "//test/test_common:environment_lib", + "//test/test_common:network_utility_lib", + "//test/test_common:registry_lib", + "//test/test_common:simulated_time_system_lib", + "//test/test_common:test_runtime_lib", + "//test/test_common:test_time_lib", + "//test/test_common:threadsafe_singleton_injector_lib", + "@envoy_api//envoy/config/core/v3:pkg_cc_proto", + ], +) + envoy_cc_test( name = "drain_manager_impl_test", srcs = ["drain_manager_impl_test.cc"], diff --git a/test/server/connection_handler_test.cc b/test/server/connection_handler_test.cc index f4573005924e9..1d5cb3fa373a2 100644 --- a/test/server/connection_handler_test.cc +++ b/test/server/connection_handler_test.cc @@ -1,3 +1,50 @@ +#include +#include +#include +#include +#include + +#include "envoy/admin/v3/config_dump.pb.h" +#include "envoy/config/core/v3/address.pb.h" +#include "envoy/config/core/v3/base.pb.h" +#include "envoy/config/core/v3/config_source.pb.h" +#include "envoy/config/listener/v3/listener.pb.h" +#include "envoy/network/listener.h" +#include "envoy/server/filter_config.h" +#include "envoy/server/listener_manager.h" +#include "envoy/stream_info/filter_state.h" + +#include "source/common/api/os_sys_calls_impl.h" +#include "source/common/common/macros.h" +#include "source/common/config/metadata.h" +#include "source/common/init/manager_impl.h" +#include "source/common/network/address_impl.h" +#include "source/common/network/io_socket_handle_impl.h" +#include "source/common/network/utility.h" +#include "source/common/protobuf/protobuf.h" +#include "source/server/configuration_impl.h" +#include "source/server/listener_manager_impl.h" + +#include "test/mocks/init/mocks.h" +#include "test/mocks/network/mocks.h" +#include "test/mocks/server/drain_manager.h" +#include "test/mocks/server/guard_dog.h" +#include "test/mocks/server/instance.h" +#include "test/mocks/server/listener_component_factory.h" +#include "test/mocks/server/worker.h" +#include "test/mocks/server/worker_factory.h" +#include "test/server/utility.h" +#include "test/test_common/environment.h" +#include "test/test_common/network_utility.h" +#include "test/test_common/test_runtime.h" +#include "test/test_common/threadsafe_singleton_injector.h" +#include "test/test_common/utility.h" + +#include "absl/strings/escaping.h" +#include "absl/strings/match.h" + +// Above from listener_manager_impl_test.cc + #include "envoy/config/core/v3/base.pb.h" #include "envoy/config/listener/v3/udp_listener_config.pb.h" #include "envoy/network/exception.h" @@ -104,6 +151,8 @@ class ConnectionHandlerTest : public testing::Test, protected Logger::Loggable udp_listener_config_; + std::unique_ptr internal_listener_config_; Network::ConnectionBalancerSharedPtr connection_balancer_; BasicResourceLimitImpl open_connections_; const std::vector access_logs_; @@ -256,6 +313,22 @@ class ConnectionHandlerTest : public testing::Test, protected Logger::Loggable> overridden_filter_chain_manager = + nullptr) { + listeners_.emplace_back(std::make_unique( + *this, tag, /*bind_to_port*/ false, /*hand_off_restored_destination_connections*/ false, + name, Network::Socket::Type::Stream, listener_filters_timeout, + continue_on_listener_filters_timeout, access_log_, overridden_filter_chain_manager, + ENVOY_TCP_BACKLOG_SIZE, nullptr)); + listeners_.back()->internal_listener_config_ = + std::make_unique(); + return listeners_.back().get(); + } + void validateOriginalDst(Network::TcpListenerCallbacks** listener_callbacks, TestListener* test_listener, Network::MockListener* listener) { Network::Address::InstanceConstSharedPtr normal_address( @@ -300,7 +373,7 @@ class ConnectionHandlerTest : public testing::Test, protected Logger::Loggable dispatcher_{"test"}; std::list listeners_; - Network::ConnectionHandlerPtr handler_; + std::unique_ptr handler_; NiceMock manager_; NiceMock factory_; const std::shared_ptr filter_chain_; @@ -1546,6 +1619,68 @@ TEST_F(ConnectionHandlerTest, ShutdownUdpListener) { << "The read_filter_ should be deleted before the udp_listener_ is deleted."; } +TEST_F(ConnectionHandlerTest, DisableInternalListener) { + InSequence s; + Network::Address::InstanceConstSharedPtr local_address{ + new Network::Address::EnvoyInternalInstance("server_internal_address")}; + + TestListener* internal_listener = + addInternalListener(1, "test_internal_listener", std::chrono::milliseconds(), false, nullptr); + EXPECT_CALL(internal_listener->socket_factory_, localAddress()) + .WillOnce(ReturnRef(local_address)); + handler_->addListener(absl::nullopt, *internal_listener); + auto internal_listener_cb = handler_->findByAddress(local_address); + ASSERT(internal_listener_cb.has_value()); + + handler_->disableListeners(); + auto internal_listener_cb_disabled = handler_->findByAddress(local_address); + ASSERT(internal_listener_cb_disabled.has_value()); + ASSERT_EQ(&internal_listener_cb_disabled.value().get(), &internal_listener_cb.value().get()); + + handler_->enableListeners(); + auto internal_listener_cb_enabled = handler_->findByAddress(local_address); + ASSERT(internal_listener_cb_enabled.has_value()); + ASSERT_EQ(&internal_listener_cb_enabled.value().get(), &internal_listener_cb.value().get()); +} + +TEST_F(ConnectionHandlerTest, InternalListenerInplaceUpdate) { + InSequence s; + uint64_t old_listener_tag = 1; + uint64_t new_listener_tag = 2; + Network::Address::InstanceConstSharedPtr local_address{ + new Network::Address::EnvoyInternalInstance("server_internal_address")}; + + TestListener* internal_listener = addInternalListener( + old_listener_tag, "test_internal_listener", std::chrono::milliseconds(), false, nullptr); + EXPECT_CALL(internal_listener->socket_factory_, localAddress()) + .WillOnce(ReturnRef(local_address)); + handler_->addListener(absl::nullopt, *internal_listener); + + ASSERT_NE(internal_listener, nullptr); + + auto overridden_filter_chain_manager = + std::make_shared>(); + TestListener* new_test_listener = + addInternalListener(new_listener_tag, "test_internal_listener", std::chrono::milliseconds(), + false, overridden_filter_chain_manager); + + handler_->addListener(old_listener_tag, *new_test_listener); + + Network::MockConnectionSocket* connection = new NiceMock(); + + auto internal_listener_cb = handler_->findByAddress(local_address); + + EXPECT_CALL(manager_, findFilterChain(_)).Times(0); + EXPECT_CALL(*overridden_filter_chain_manager, findFilterChain(_)).WillOnce(Return(nullptr)); + EXPECT_CALL(*access_log_, log(_, _, _, _)); + internal_listener_cb.value().get().onAccept(Network::ConnectionSocketPtr{connection}); + EXPECT_EQ(0UL, handler_->numConnections()); + + testing::MockFunction completion; + handler_->removeFilterChains(old_listener_tag, {}, completion.AsStdFunction()); + EXPECT_CALL(completion, Call()); + dispatcher_.clearDeferredDeleteList(); +} } // namespace } // namespace Server } // namespace Envoy diff --git a/test/server/listener_manager_impl_test.cc b/test/server/listener_manager_impl_test.cc index c2c61c695c03c..5a7b3732d5787 100644 --- a/test/server/listener_manager_impl_test.cc +++ b/test/server/listener_manager_impl_test.cc @@ -557,6 +557,10 @@ bind_to_port: false } TEST_F(ListenerManagerImplTest, UnsupportedInternalListener) { + auto scoped_runtime_guard = std::make_unique(); + + Runtime::LoaderSingleton::getExisting()->mergeValues( + {{"envoy.reloadable_features.internal_address", "false"}}); const std::string yaml = R"EOF( address: envoy_internal_address: @@ -4711,6 +4715,310 @@ name: test_api_listener_2 EXPECT_EQ("test_api_listener", manager_->apiListener()->get().name()); } +TEST_F(ListenerManagerImplWithRealFiltersTest, AddOrUpdateInternalListener) { + auto scoped_runtime_guard = std::make_unique(); + Runtime::LoaderSingleton::getExisting()->mergeValues( + {{"envoy.reloadable_features.internal_address", "true"}}); + time_system_.setSystemTime(std::chrono::milliseconds(1001001001001)); + + InSequence s; + + auto* lds_api = new MockLdsApi(); + EXPECT_CALL(listener_factory_, createLdsApi_(_, _)).WillOnce(Return(lds_api)); + envoy::config::core::v3::ConfigSource lds_config; + manager_->createLdsApi(lds_config, nullptr); + + EXPECT_CALL(*lds_api, versionInfo()).WillOnce(Return("")); + checkConfigDump(R"EOF( +static_listeners: +)EOF"); + + // Add foo listener. + const std::string listener_foo_yaml = R"EOF( +name: test_internal_listener +address: + envoy_internal_address: + server_listener_name: test_internal_listener_name +internal_listener: {} +filter_chains: {} + )EOF"; + + ListenerHandle* listener_foo = expectListenerCreate(false, true); + EXPECT_TRUE( + manager_->addOrUpdateListener(parseListenerFromV3Yaml(listener_foo_yaml), "version1", true)); + checkStats(__LINE__, 1, 0, 0, 0, 1, 0, 0); + EXPECT_CALL(*lds_api, versionInfo()).WillOnce(Return("version1")); + checkConfigDump(R"EOF( +version_info: version1 +static_listeners: +dynamic_listeners: + - name: test_internal_listener + warming_state: + version_info: version1 + listener: + "@type": type.googleapis.com/envoy.config.listener.v3.Listener + name: test_internal_listener + address: + envoy_internal_address: + server_listener_name: test_internal_listener_name + filter_chains: {} + internal_listener: {} + last_updated: + seconds: 1001001001 + nanos: 1000000 +)EOF"); + + // Update duplicate should be a NOP. + EXPECT_FALSE(manager_->addOrUpdateListener(parseListenerFromV3Yaml(listener_foo_yaml), "", true)); + checkStats(__LINE__, 1, 0, 0, 0, 1, 0, 0); + + // Update foo listener. Should share socket. + const std::string listener_foo_update1_yaml = R"EOF( +name: test_internal_listener +address: + envoy_internal_address: + server_listener_name: test_internal_listener_name +filter_chains: {} +internal_listener: {} +per_connection_buffer_limit_bytes: 10 + )EOF"; + + time_system_.setSystemTime(std::chrono::milliseconds(2002002002002)); + + ListenerHandle* listener_foo_update1 = expectListenerCreate(false, true); + EXPECT_CALL(*listener_foo, onDestroy()); + EXPECT_TRUE(manager_->addOrUpdateListener(parseListenerFromV3Yaml(listener_foo_update1_yaml), + "version2", true)); + checkStats(__LINE__, 1, 1, 0, 0, 1, 0, 0); + EXPECT_CALL(*lds_api, versionInfo()).WillOnce(Return("version2")); + checkConfigDump(R"EOF( + version_info: version2 + static_listeners: + dynamic_listeners: + - name: test_internal_listener + warming_state: + version_info: version2 + listener: + "@type": type.googleapis.com/envoy.config.listener.v3.Listener + name: test_internal_listener + address: + envoy_internal_address: + server_listener_name: test_internal_listener_name + filter_chains: {} + internal_listener: {} + per_connection_buffer_limit_bytes: 10 + last_updated: + seconds: 2002002002 + nanos: 2000000 + )EOF"); + + // Validate that workers_started stat is zero before calling startWorkers. + EXPECT_EQ(0, server_.stats_store_ + .gauge("listener_manager.workers_started", Stats::Gauge::ImportMode::NeverImport) + .value()); + + // Start workers. + EXPECT_CALL(*worker_, addListener(_, _, _)); + EXPECT_CALL(*worker_, start(_, _)); + manager_->startWorkers(guard_dog_, callback_.AsStdFunction()); + // Validate that workers_started stat is still zero before workers set the status via + // completion callback. + EXPECT_EQ(0, server_.stats_store_ + .gauge("listener_manager.workers_started", Stats::Gauge::ImportMode::NeverImport) + .value()); + worker_->callAddCompletion(); + + // Validate that workers_started stat is set to 1 after workers have responded with initialization + // status. + EXPECT_EQ(1, server_.stats_store_ + .gauge("listener_manager.workers_started", Stats::Gauge::ImportMode::NeverImport) + .value()); + + // Update duplicate should be a NOP. + EXPECT_FALSE( + manager_->addOrUpdateListener(parseListenerFromV3Yaml(listener_foo_update1_yaml), "", true)); + checkStats(__LINE__, 1, 1, 0, 0, 1, 0, 0); + + time_system_.setSystemTime(std::chrono::milliseconds(3003003003003)); + + // Update foo. Should go into warming, have an immediate warming callback, and start immediate + // removal. + ListenerHandle* listener_foo_update2 = expectListenerCreate(false, true); + EXPECT_CALL(*worker_, addListener(_, _, _)); + EXPECT_CALL(*worker_, stopListener(_, _)); + EXPECT_CALL(*listener_foo_update1->drain_manager_, startDrainSequence(_)); + EXPECT_TRUE( + manager_->addOrUpdateListener(parseListenerFromV3Yaml(listener_foo_yaml), "version3", true)); + worker_->callAddCompletion(); + checkStats(__LINE__, 1, 2, 0, 0, 1, 1, 0); + EXPECT_CALL(*lds_api, versionInfo()).WillOnce(Return("version3")); + checkConfigDump(R"EOF( + version_info: version3 + static_listeners: + dynamic_listeners: + - name: test_internal_listener + active_state: + version_info: version3 + listener: + "@type": type.googleapis.com/envoy.config.listener.v3.Listener + name: test_internal_listener + address: + envoy_internal_address: + server_listener_name: test_internal_listener_name + filter_chains: {} + internal_listener: {} + last_updated: + seconds: 3003003003 + nanos: 3000000 + draining_state: + version_info: version2 + listener: + "@type": type.googleapis.com/envoy.config.listener.v3.Listener + name: test_internal_listener + address: + envoy_internal_address: + server_listener_name: test_internal_listener_name + filter_chains: {} + internal_listener: {} + per_connection_buffer_limit_bytes: 10 + last_updated: + seconds: 2002002002 + nanos: 2000000 + )EOF"); + + EXPECT_CALL(*worker_, removeListener(_, _)); + listener_foo_update1->drain_manager_->drain_sequence_completion_(); + checkStats(__LINE__, 1, 2, 0, 0, 1, 1, 0); + EXPECT_CALL(*listener_foo_update1, onDestroy()); + worker_->callRemovalCompletion(); + checkStats(__LINE__, 1, 2, 0, 0, 1, 0, 0); + + time_system_.setSystemTime(std::chrono::milliseconds(4004004004004)); + + // Add bar listener. + const std::string listener_bar_yaml = R"EOF( + name: test_internal_listener_bar + address: + envoy_internal_address: + server_listener_name: test_internal_listener_bar + filter_chains: {} + internal_listener: {} + )EOF"; + + ListenerHandle* listener_bar = expectListenerCreate(false, true); + EXPECT_CALL(*worker_, addListener(_, _, _)); + EXPECT_TRUE( + manager_->addOrUpdateListener(parseListenerFromV3Yaml(listener_bar_yaml), "version4", true)); + EXPECT_EQ(2UL, manager_->listeners().size()); + worker_->callAddCompletion(); + checkStats(__LINE__, 2, 2, 0, 0, 2, 0, 0); + + time_system_.setSystemTime(std::chrono::milliseconds(5005005005005)); + + // Add baz listener, this time requiring initializing. + const std::string listener_baz_yaml = R"EOF( + name: test_internal_listener_baz + address: + envoy_internal_address: + server_listener_name: test_internal_listener_baz + filter_chains: {} + internal_listener: {} + )EOF"; + + ListenerHandle* listener_baz = expectListenerCreate(true, true); + EXPECT_CALL(listener_baz->target_, initialize()); + EXPECT_TRUE( + manager_->addOrUpdateListener(parseListenerFromV3Yaml(listener_baz_yaml), "version5", true)); + EXPECT_EQ(2UL, manager_->listeners().size()); + checkStats(__LINE__, 3, 2, 0, 1, 2, 0, 0); + EXPECT_CALL(*lds_api, versionInfo()).WillOnce(Return("version5")); + checkConfigDump(R"EOF( + version_info: version5 + dynamic_listeners: + - name: test_internal_listener + active_state: + version_info: version3 + listener: + "@type": type.googleapis.com/envoy.config.listener.v3.Listener + name: test_internal_listener + address: + envoy_internal_address: + server_listener_name: test_internal_listener_name + filter_chains: {} + internal_listener: {} + last_updated: + seconds: 3003003003 + nanos: 3000000 + - name: test_internal_listener_bar + active_state: + version_info: version4 + listener: + "@type": type.googleapis.com/envoy.config.listener.v3.Listener + name: test_internal_listener_bar + address: + envoy_internal_address: + server_listener_name: test_internal_listener_bar + filter_chains: {} + internal_listener: {} + last_updated: + seconds: 4004004004 + nanos: 4000000 + - name: test_internal_listener_baz + warming_state: + version_info: version5 + listener: + "@type": type.googleapis.com/envoy.config.listener.v3.Listener + name: test_internal_listener_baz + address: + envoy_internal_address: + server_listener_name: test_internal_listener_baz + filter_chains: {} + internal_listener: {} + last_updated: + seconds: 5005005005 + nanos: 5000000 + )EOF"); + + // Update a duplicate baz that is currently warming. + EXPECT_FALSE(manager_->addOrUpdateListener(parseListenerFromV3Yaml(listener_baz_yaml), "", true)); + checkStats(__LINE__, 3, 2, 0, 1, 2, 0, 0); + + // Update baz while it is warming. + const std::string listener_baz_update1_yaml = R"EOF( + name: test_internal_listener_baz + address: + envoy_internal_address: + server_listener_name: test_internal_listener_baz + internal_listener: {} + filter_chains: + - filters: + - name: fake + typed_config: {} + )EOF"; + + ListenerHandle* listener_baz_update1 = expectListenerCreate(true, true); + EXPECT_CALL(*listener_baz, onDestroy()).WillOnce(Invoke([listener_baz]() -> void { + // Call the initialize callback during destruction like RDS will. + listener_baz->target_.ready(); + })); + EXPECT_CALL(listener_baz_update1->target_, initialize()); + EXPECT_TRUE( + manager_->addOrUpdateListener(parseListenerFromV3Yaml(listener_baz_update1_yaml), "", true)); + EXPECT_EQ(2UL, manager_->listeners().size()); + checkStats(__LINE__, 3, 3, 0, 1, 2, 0, 0); + + // Finish initialization for baz which should make it active. + EXPECT_CALL(*worker_, addListener(_, _, _)); + listener_baz_update1->target_.ready(); + EXPECT_EQ(3UL, manager_->listeners().size()); + worker_->callAddCompletion(); + checkStats(__LINE__, 3, 3, 0, 0, 3, 0, 0); + + EXPECT_CALL(*listener_foo_update2, onDestroy()); + EXPECT_CALL(*listener_bar, onDestroy()); + EXPECT_CALL(*listener_baz_update1, onDestroy()); +} + TEST_F(ListenerManagerImplTest, StopInplaceWarmingListener) { InSequence s; From 0361f4a7a111cbd2f5c21d6cc97e9fea3eee5962 Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Thu, 2 Sep 2021 06:41:40 +0000 Subject: [PATCH 04/31] merge master Signed-off-by: Yuchen Dai --- source/server/active_internal_listener.cc | 6 +++--- test/server/active_internal_listener_test.cc | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/source/server/active_internal_listener.cc b/source/server/active_internal_listener.cc index ed28746ab8e1e..0272c93404de1 100644 --- a/source/server/active_internal_listener.cc +++ b/source/server/active_internal_listener.cc @@ -167,9 +167,9 @@ void ActiveInternalListener::onAccept(Network::ConnectionSocketPtr&& socket) { auto active_socket = std::make_unique( *this, std::move(socket), false /* do not handle off at internal listener */); // TODO(lambdai): restore address from either socket options, or from listener config. - active_socket->socket_->addressProvider().restoreLocalAddress( + active_socket->socket_->connectionInfoProvider().restoreLocalAddress( std::make_shared("255.255.255.255", 0)); - active_socket->socket_->addressProvider().setRemoteAddress( + active_socket->socket_->connectionInfoProvider().setRemoteAddress( std::make_shared("255.255.255.254", 0)); onSocketAccepted(std::move(active_socket)); @@ -185,7 +185,7 @@ void ActiveInternalListener::newActiveConnection( // If the connection is already closed, we can just let this connection immediately die. if (active_connection->connection_->state() != Network::Connection::State::Closed) { ENVOY_CONN_LOG(debug, "new connection from {}", *active_connection->connection_, - active_connection->connection_->addressProvider().remoteAddress()->asString()); + active_connection->connection_->connectionInfoProvider().remoteAddress()->asString()); active_connection->connection_->addConnectionCallbacks(*active_connection); LinkedList::moveIntoList(std::move(active_connection), active_connections.connections_); } diff --git a/test/server/active_internal_listener_test.cc b/test/server/active_internal_listener_test.cc index 71d170dba3d91..7af142e31596c 100644 --- a/test/server/active_internal_listener_test.cc +++ b/test/server/active_internal_listener_test.cc @@ -110,7 +110,7 @@ TEST_F(ActiveInternalListenerTest, AcceptSocketAndCreateListenerFilter) { })); EXPECT_CALL(*test_listener_filter, onAccept(_)) .WillOnce(Invoke([&](Network::ListenerFilterCallbacks& cb) -> Network::FilterStatus { - cb.socket().addressProvider().restoreLocalAddress(original_dst_address); + cb.socket().connectionInfoProvider().restoreLocalAddress(original_dst_address); return Network::FilterStatus::Continue; })); EXPECT_CALL(*test_listener_filter, destroy_()); @@ -140,7 +140,7 @@ TEST_F(ActiveInternalListenerTest, AcceptSocketAndCreateNetworkFilter) { })); EXPECT_CALL(*test_listener_filter, onAccept(_)) .WillOnce(Invoke([&](Network::ListenerFilterCallbacks& cb) -> Network::FilterStatus { - cb.socket().addressProvider().restoreLocalAddress(original_dst_address); + cb.socket().connectionInfoProvider().restoreLocalAddress(original_dst_address); return Network::FilterStatus::Continue; })); EXPECT_CALL(*test_listener_filter, destroy_()); From c5e11314557719d85efe344649788be8db74efcf Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Mon, 13 Sep 2021 21:31:33 +0000 Subject: [PATCH 05/31] clean up Signed-off-by: Yuchen Dai --- source/server/active_internal_listener.cc | 118 +--------------------- source/server/active_internal_listener.h | 6 +- 2 files changed, 8 insertions(+), 116 deletions(-) diff --git a/source/server/active_internal_listener.cc b/source/server/active_internal_listener.cc index 0272c93404de1..310696816880a 100644 --- a/source/server/active_internal_listener.cc +++ b/source/server/active_internal_listener.cc @@ -17,6 +17,7 @@ ActiveInternalListener::ActiveInternalListener(Network::ConnectionHandler& conn_ : OwnedActiveStreamListenerBase( conn_handler, dispatcher, std::make_unique(), config) {} + ActiveInternalListener::ActiveInternalListener(Network::ConnectionHandler& conn_handler, Event::Dispatcher& dispatcher, Network::ListenerPtr listener, @@ -42,123 +43,11 @@ ActiveInternalListener::~ActiveInternalListener() { dispatcher().clearDeferredDeleteList(); } -// void ActiveInternalListener::removeConnection(ActiveInternalConnection& connection) { -// ENVOY_CONN_LOG(debug, "adding to cleanup list", *connection.connection_); -// ActiveConnections& active_connections = connection.active_connections_; -// ActiveInternalConnectionPtr removed = -// connection.removeFromList(active_connections.connections_); -// dispatcher().deferredDelete(std::move(removed)); -// // Delete map entry only iff connections becomes empty. -// if (active_connections.connections_.empty()) { -// auto iter = connections_by_context_.find(&active_connections.filter_chain_); -// ASSERT(iter != connections_by_context_.end()); -// // To cover the lifetime of every single connection, Connections need to be deferred deleted -// // because the previously contained connection is deferred deleted. -// dispatcher().deferredDelete(std::move(iter->second)); -// // The erase will break the iteration over the connections_by_context_ during the deletion. -// if (!is_deleting_) { -// connections_by_context_.erase(iter); -// } -// } -// } - -// void ActiveInternalListener::newConnection(Network::ConnectionSocketPtr&& socket, -// std::unique_ptr stream_info) { - -// // Find matching filter chain. -// const auto filter_chain = config_->filterChainManager().findFilterChain(*socket); -// if (filter_chain == nullptr) { -// ENVOY_LOG(debug, "closing connection: no matching filter chain found"); -// stats_.no_filter_chain_match_.inc(); -// stream_info->setResponseFlag(StreamInfo::ResponseFlag::NoRouteFound); -// stream_info->setResponseCodeDetails(StreamInfo::ResponseCodeDetails::get().FilterChainNotFound); -// emitLogs(*config_, *stream_info); -// socket->close(); -// return; -// } - -// stream_info->setFilterChainName(filter_chain->name()); -// auto transport_socket = filter_chain->transportSocketFactory().createTransportSocket(nullptr); -// stream_info->setDownstreamSslConnection(transport_socket->ssl()); -// auto& active_connections = getOrCreateActiveConnections(*filter_chain); -// auto server_conn_ptr = 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); -// } -// ActiveInternalConnectionPtr active_connection( -// new ActiveInternalConnection(active_connections, std::move(server_conn_ptr), -// dispatcher().timeSource(), std::move(stream_info))); -// active_connection->connection_->setBufferLimits(config_->perConnectionBufferLimitBytes()); - -// const bool empty_filter_chain = !config_->filterChainFactory().createNetworkFilterChain( -// *active_connection->connection_, filter_chain->networkFilterFactories()); -// if (empty_filter_chain) { -// ENVOY_CONN_LOG(debug, "closing connection: no filters", *active_connection->connection_); -// active_connection->connection_->close(Network::ConnectionCloseType::NoFlush); -// } - -// // If the connection is already closed, we can just let this connection immediately die. -// if (active_connection->connection_->state() != Network::Connection::State::Closed) { -// ENVOY_CONN_LOG(debug, "new connection", *active_connection->connection_); -// active_connection->connection_->addConnectionCallbacks(*active_connection); -// LinkedList::moveIntoList(std::move(active_connection), active_connections.connections_); -// } -// } - void ActiveInternalListener::updateListenerConfig(Network::ListenerConfig& config) { ENVOY_LOG(trace, "replacing listener ", config_->listenerTag(), " by ", config.listenerTag()); config_ = &config; } -// ActiveInternalConnections::ActiveInternalConnections(ActiveInternalListener& listener, -// const Network::FilterChain& filter_chain) -// : listener_(listener), filter_chain_(filter_chain) {} - -// ActiveInternalConnections::~ActiveInternalConnections() { -// // connections should be defer deleted already. -// ASSERT(connections_.empty()); -// } - -// ActiveInternalConnection::ActiveInternalConnection( -// ActiveInternalConnections& active_connections, Network::ConnectionPtr&& new_connection, -// TimeSource& time_source, std::unique_ptr&& stream_info) -// : stream_info_(std::move(stream_info)), active_connections_(active_connections), -// connection_(std::move(new_connection)), -// conn_length_(new Stats::HistogramCompletableTimespanImpl( -// active_connections_.listener_.stats_.downstream_cx_length_ms_, time_source)) { -// // We just universally set no delay on connections. Theoretically we might at some point want -// // to make this configurable. -// connection_->noDelay(true); -// auto& listener = active_connections_.listener_; -// listener.stats_.downstream_cx_total_.inc(); -// listener.stats_.downstream_cx_active_.inc(); -// listener.per_worker_stats_.downstream_cx_total_.inc(); -// listener.per_worker_stats_.downstream_cx_active_.inc(); -// stream_info_->setConnectionID(connection_->id()); - -// // Active connections on the handler (not listener). The per listener connections have already -// // been incremented at this point either via the connection balancer or in the socket accept -// // path if there is no configured balancer. -// listener.parent_.incNumConnections(); -// } - -// ActiveInternalConnection::~ActiveInternalConnection() { -// ActiveInternalListener::emitLogs(*active_connections_.listener_.config_, *stream_info_); -// auto& listener = active_connections_.listener_; -// listener.stats_.downstream_cx_active_.dec(); -// listener.stats_.downstream_cx_destroy_.inc(); -// listener.per_worker_stats_.downstream_cx_active_.dec(); -// conn_length_->complete(); - -// // Active listener connections (not handler). -// listener.decNumConnections(); - -// // Active handler connections (not listener). -// listener.parent_.decNumConnections(); -// } - void ActiveInternalListener::onAccept(Network::ConnectionSocketPtr&& socket) { // Unlike tcp listener, no rebalancer is applied and won't call pickTargetHandler to account // connections. @@ -184,8 +73,9 @@ void ActiveInternalListener::newActiveConnection( dispatcher().timeSource(), std::move(stream_info)); // If the connection is already closed, we can just let this connection immediately die. if (active_connection->connection_->state() != Network::Connection::State::Closed) { - ENVOY_CONN_LOG(debug, "new connection from {}", *active_connection->connection_, - active_connection->connection_->connectionInfoProvider().remoteAddress()->asString()); + ENVOY_CONN_LOG( + debug, "new connection from {}", *active_connection->connection_, + active_connection->connection_->connectionInfoProvider().remoteAddress()->asString()); active_connection->connection_->addConnectionCallbacks(*active_connection); LinkedList::moveIntoList(std::move(active_connection), active_connections.connections_); } diff --git a/source/server/active_internal_listener.h b/source/server/active_internal_listener.h index a8d27388d6816..5f8d699a226a8 100644 --- a/source/server/active_internal_listener.h +++ b/source/server/active_internal_listener.h @@ -38,8 +38,10 @@ class ActiveInternalListener : public OwnedActiveStreamListenerBase, class NetworkInternalListener : public Network::Listener { void disable() override { - // TODO(lambdai): think about how to elegantly disable internal listener. (Queue socket or - // close socket immediately?) + // Similar to the listeners that does not bind to port. Accept is not driven by OS io event so + // the disable is not working. + // TODO(lambdai): Explore the approach to elegantly disable internal listener. Maybe an user + // space accept queue should be put here. ENVOY_LOG(debug, "Warning: the internal listener cannot be disabled."); } From da47f122b4d5f847e52ef528fb95d27a150d2495 Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Mon, 13 Sep 2021 21:42:52 +0000 Subject: [PATCH 06/31] revert stub for advanced integration test Signed-off-by: Yuchen Dai --- test/integration/base_integration_test.h | 7 ------- test/integration/server.cc | 10 ---------- test/integration/server.h | 10 ---------- 3 files changed, 27 deletions(-) diff --git a/test/integration/base_integration_test.h b/test/integration/base_integration_test.h index 512c485a9522a..d629b0572ff35 100644 --- a/test/integration/base_integration_test.h +++ b/test/integration/base_integration_test.h @@ -346,13 +346,6 @@ class BaseIntegrationTest : protected Logger::Loggable { upstream_config_.http2_options_.MergeFrom(options); } - Event::Dispatcher* getWorkerDispatcher(uint32_t index) { - if (test_server_ == nullptr) { - return nullptr; - } - return &test_server_->getWorkerDispatcher(index).value().get(); - } - std::unique_ptr upstream_stats_store_; // Make sure the test server will be torn down after any fake client. diff --git a/test/integration/server.cc b/test/integration/server.cc index 21c711667f48a..451c951d20e3a 100644 --- a/test/integration/server.cc +++ b/test/integration/server.cc @@ -238,16 +238,6 @@ void IntegrationTestServerImpl::createAndRunEnvoyServer( admin_address_ = server.admin().socket().connectionInfoProvider().localAddress(); server_ = &server; stat_store_ = &stat_store; - // Use the tls slots to save dispatchers. The slot must be destroyed before server shutdown. - ThreadLocal::SlotPtr dispatcher_tls = tls.allocateSlot(); - - dispatcher_tls->set( - [this](Event::Dispatcher& dispatcher) -> ThreadLocal::ThreadLocalObjectSharedPtr { - Thread::LockGuard guard(dispatcher_mutex_); - registered_dispatchers_.push_back(dispatcher); - return nullptr; - }); - serverReady(); server.run(); } diff --git a/test/integration/server.h b/test/integration/server.h index d37c19f9b304b..8ad4c633d6892 100644 --- a/test/integration/server.h +++ b/test/integration/server.h @@ -527,12 +527,6 @@ class IntegrationTestServer : public Logger::Loggable, void useAdminInterfaceToQuit(bool use) { use_admin_interface_to_quit_ = use; } bool useAdminInterfaceToQuit() { return use_admin_interface_to_quit_; } - absl::optional> getWorkerDispatcher(uint32_t index) { - Thread::LockGuard guard(dispatcher_mutex_); - ASSERT(index < registered_dispatchers_.size()); - return registered_dispatchers_[index]; - } - protected: IntegrationTestServer(Event::TestTimeSystem& time_system, Api::Api& api, const std::string& config_path) @@ -555,10 +549,6 @@ class IntegrationTestServer : public Logger::Loggable, // adminAddress()) will be available. void serverReady(); - Thread::MutexBasicLockable dispatcher_mutex_; - // Assume 128 is greater than concurrency. - std::vector>> registered_dispatchers_; - private: /** * Runs the real server on a thread. From 427efc1151ab63d69be0355218457d99ee13ff07 Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Wed, 29 Sep 2021 04:47:13 +0000 Subject: [PATCH 07/31] cleanup Signed-off-by: Yuchen Dai --- source/common/network/connection_impl.cc | 1 - source/common/upstream/upstream_impl.cc | 7 ++-- test/server/BUILD | 40 -------------------- test/server/active_internal_listener_test.cc | 10 ----- test/server/active_tcp_listener_test.cc | 1 - 5 files changed, 4 insertions(+), 55 deletions(-) diff --git a/source/common/network/connection_impl.cc b/source/common/network/connection_impl.cc index 4ebda5472772f..b5d3472ad8f57 100644 --- a/source/common/network/connection_impl.cc +++ b/source/common/network/connection_impl.cc @@ -21,7 +21,6 @@ #include "source/common/network/listen_socket_impl.h" #include "source/common/network/raw_buffer_socket.h" #include "source/common/network/utility.h" -#include "source/common/runtime/runtime_features.h" namespace Envoy { namespace Network { diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index 2036152f6657f..56ebe7ef99f00 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -317,9 +317,10 @@ Network::ClientConnectionPtr HostImpl::createConnection( } else { connection_options = options; } - if (!Runtime::runtimeFeatureEnabled("envoy.reloadable_features.internal_address")) { - ASSERT(!address->envoyInternalAddress()); - } + + ASSERT(!address->envoyInternalAddress() || + Runtime::runtimeFeatureEnabled("envoy.reloadable_features.internal_address")); + Network::ClientConnectionPtr connection = address_list.size() > 1 ? std::make_unique( diff --git a/test/server/BUILD b/test/server/BUILD index bc37b6bdf22bc..219d6a74bd591 100644 --- a/test/server/BUILD +++ b/test/server/BUILD @@ -130,14 +130,10 @@ envoy_cc_test( "//source/common/network:address_lib", "//source/common/network:connection_balancer_lib", "//source/common/stats:stats_lib", - "//source/server:active_raw_udp_listener_config", "//source/server:connection_handler_lib", - "//test/mocks/access_log:access_log_mocks", - "//test/mocks/api:api_mocks", "//test/mocks/network:io_handle_mocks", "//test/mocks/network:network_mocks", "//test/test_common:network_utility_lib", - "//test/test_common:threadsafe_singleton_injector_lib", ], ) @@ -146,49 +142,13 @@ envoy_cc_test( srcs = ["active_internal_listener_test.cc"], deps = [ ":utility_lib", - "//source/common/api:os_sys_calls_lib", - "//source/common/common:utility_lib", - "//source/common/config:metadata_lib", - "//source/common/config:utility_lib", - "//source/common/init:manager_lib", - "//source/common/network:addr_family_aware_socket_option_lib", "//source/common/network:address_lib", - "//source/common/network:connection_balancer_lib", "//source/common/network:listen_socket_lib", - "//source/common/network:socket_option_lib", "//source/common/network:utility_lib", - "//source/common/protobuf", "//source/common/stats:stats_lib", - "//source/extensions/filters/listener/original_dst:config", - "//source/extensions/filters/listener/proxy_protocol:config", - "//source/extensions/filters/listener/tls_inspector:config", - "//source/extensions/filters/network/http_connection_manager:config", - "//source/extensions/filters/network/tcp_proxy:config", - "//source/extensions/request_id/uuid:config", "//source/extensions/transport_sockets/raw_buffer:config", - "//source/extensions/transport_sockets/tls:config", - "//source/extensions/transport_sockets/tls:ssl_socket_lib", - "//source/server:active_raw_udp_listener_config", "//source/server:connection_handler_lib", - "//source/server:listener_manager_lib", - "//test/mocks/access_log:access_log_mocks", - "//test/mocks/api:api_mocks", - "//test/mocks/init:init_mocks", "//test/mocks/network:network_mocks", - "//test/mocks/server:drain_manager_mocks", - "//test/mocks/server:guard_dog_mocks", - "//test/mocks/server:instance_mocks", - "//test/mocks/server:listener_component_factory_mocks", - "//test/mocks/server:worker_factory_mocks", - "//test/mocks/server:worker_mocks", - "//test/test_common:environment_lib", - "//test/test_common:network_utility_lib", - "//test/test_common:registry_lib", - "//test/test_common:simulated_time_system_lib", - "//test/test_common:test_runtime_lib", - "//test/test_common:test_time_lib", - "//test/test_common:threadsafe_singleton_injector_lib", - "@envoy_api//envoy/config/core/v3:pkg_cc_proto", ], ) diff --git a/test/server/active_internal_listener_test.cc b/test/server/active_internal_listener_test.cc index 7af142e31596c..2fcb689057ae9 100644 --- a/test/server/active_internal_listener_test.cc +++ b/test/server/active_internal_listener_test.cc @@ -1,27 +1,17 @@ #include -#include "envoy/config/core/v3/base.pb.h" -#include "envoy/network/exception.h" #include "envoy/network/filter.h" #include "envoy/network/listener.h" #include "envoy/stats/scope.h" -#include "source/common/common/utility.h" -#include "source/common/config/utility.h" #include "source/common/network/address_impl.h" -#include "source/common/network/connection_balancer_impl.h" -#include "source/common/network/io_socket_handle_impl.h" #include "source/common/network/raw_buffer_socket.h" -#include "source/common/network/utility.h" #include "source/server/active_internal_listener.h" #include "source/server/connection_handler_impl.h" -#include "test/mocks/access_log/mocks.h" -#include "test/mocks/api/mocks.h" #include "test/mocks/common.h" #include "test/mocks/network/mocks.h" #include "test/test_common/network_utility.h" -#include "test/test_common/threadsafe_singleton_injector.h" #include "gmock/gmock.h" #include "gtest/gtest.h" diff --git a/test/server/active_tcp_listener_test.cc b/test/server/active_tcp_listener_test.cc index 2e6e2b807b03f..2217c3f64020d 100644 --- a/test/server/active_tcp_listener_test.cc +++ b/test/server/active_tcp_listener_test.cc @@ -10,7 +10,6 @@ #include "source/common/network/utility.h" #include "source/server/active_tcp_listener.h" -#include "test/mocks/api/mocks.h" #include "test/mocks/common.h" #include "test/mocks/network/io_handle.h" #include "test/mocks/network/mocks.h" From a6aca895b8b8c0a42f8cf4403a88307594cc7f36 Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Wed, 29 Sep 2021 05:56:35 +0000 Subject: [PATCH 08/31] more Signed-off-by: Yuchen Dai --- envoy/network/listener.h | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/envoy/network/listener.h b/envoy/network/listener.h index 9aab1a8c0bdcb..f2e0175178c5e 100644 --- a/envoy/network/listener.h +++ b/envoy/network/listener.h @@ -442,6 +442,9 @@ class UdpListener : public virtual Listener { using UdpListenerPtr = std::unique_ptr; +/** + * Internal listener callbacks. + */ class InternalListenerCallbacks { public: virtual ~InternalListenerCallbacks() = default; @@ -451,8 +454,6 @@ class InternalListenerCallbacks { * @param socket supplies the socket that is moved into the callee. */ virtual void onAccept(ConnectionSocketPtr&& socket) PURE; - - // virtual Event::Dispatcher& dispatcher() PURE; }; using InternalListenerCallbacksOptRef = absl::optional>; @@ -462,9 +463,18 @@ class InternalListener {}; using InternalListenerPtr = std::unique_ptr; using InternalListenerOptRef = absl::optional>; +/** + * The query interface of the registered internal listener callbacks. + */ class InternalListenerManager { public: virtual ~InternalListenerManager() = default; + + /** + * Return the internal listener binding the listener address. + * + * @param listen_address the internal address of the expected internal listener. + */ virtual InternalListenerCallbacksOptRef findByAddress(const Address::InstanceConstSharedPtr& listen_address) PURE; }; From 6361da01d24eb453ddad3f2b3d79a20a47e63f42 Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Tue, 5 Oct 2021 05:55:38 +0000 Subject: [PATCH 09/31] add new listen socket type for internal listener Signed-off-by: Yuchen Dai --- source/common/network/listen_socket_impl.h | 14 +++++++++++++- source/server/connection_handler_impl.cc | 1 - source/server/listener_manager_impl.cc | 8 ++++++-- 3 files changed, 19 insertions(+), 4 deletions(-) diff --git a/source/common/network/listen_socket_impl.h b/source/common/network/listen_socket_impl.h index 4d4c84ac24a2a..a4b7f95c37c59 100644 --- a/source/common/network/listen_socket_impl.h +++ b/source/common/network/listen_socket_impl.h @@ -129,10 +129,22 @@ using UdpListenSocketPtr = std::unique_ptr; class UdsListenSocket : public ListenSocketImpl { public: UdsListenSocket(const Address::InstanceConstSharedPtr& address); - UdsListenSocket(IoHandlePtr&& io_handle, const Address::InstanceConstSharedPtr& address); Socket::Type socketType() const override { return Socket::Type::Stream; } }; +// This socket type adapts the ListenerComponentFactory. +class InternalListenSocket : public ListenSocketImpl { +public: + InternalListenSocket(const Address::InstanceConstSharedPtr& address) + : ListenSocketImpl(/* io_handle= */ nullptr, address) {} + Socket::Type socketType() const override { return Socket::Type::Stream; } + + // InternalListenSocket cannot be duplicated. + SocketPtr duplicate() override { + return std::make_unique(/* io_handle */ nullptr, + connectionInfoProvider().localAddress()); + } +}; class ConnectionSocketImpl : public SocketImpl, public ConnectionSocket { public: ConnectionSocketImpl(IoHandlePtr&& io_handle, diff --git a/source/server/connection_handler_impl.cc b/source/server/connection_handler_impl.cc index 2988ee0b76c7c..faf349d117136 100644 --- a/source/server/connection_handler_impl.cc +++ b/source/server/connection_handler_impl.cc @@ -83,7 +83,6 @@ void ConnectionHandlerImpl::addListener(absl::optional overridden_list if (auto* listener = details.listener_->listener(); listener != nullptr) { listener->setRejectFraction(listener_reject_fraction_); } - // TODO: make listenSocketFactory support envoy internal listener. listeners_.emplace_back(config.listenSocketFactory().localAddress(), std::move(details)); } diff --git a/source/server/listener_manager_impl.cc b/source/server/listener_manager_impl.cc index 6aa274a1784d5..c21e1642a9a3f 100644 --- a/source/server/listener_manager_impl.cc +++ b/source/server/listener_manager_impl.cc @@ -169,8 +169,6 @@ Network::ListenerFilterMatcherSharedPtr ProdListenerComponentFactory::createList Network::SocketSharedPtr ProdListenerComponentFactory::createListenSocket( Network::Address::InstanceConstSharedPtr address, Network::Socket::Type socket_type, const Network::Socket::OptionsSharedPtr& options, BindType bind_type, uint32_t worker_index) { - ASSERT(address->type() == Network::Address::Type::Ip || - address->type() == Network::Address::Type::Pipe); ASSERT(socket_type == Network::Socket::Type::Stream || socket_type == Network::Socket::Type::Datagram); @@ -191,6 +189,12 @@ Network::SocketSharedPtr ProdListenerComponentFactory::createListenSocket( return std::make_shared(std::move(io_handle), address); } return std::make_shared(address); + } else if (address->type() == Network::Address::Type::EnvoyInternal) { + if (socket_type != Network::Socket::Type::Stream) { + throw EnvoyException( + fmt::format("socket type {} not supported for EnvoyInternalAddress", toString(socket_type))); + } + return std::make_shared(address); } const std::string scheme = (socket_type == Network::Socket::Type::Stream) From 6d50f2acda7fe28e4db95de17448efc3f9b55465 Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Tue, 5 Oct 2021 16:19:41 +0000 Subject: [PATCH 10/31] WIP: listener address mutated to ip Signed-off-by: Yuchen Dai --- source/common/network/listen_socket_impl.h | 4 +-- test/config/utility.cc | 3 ++- test/integration/BUILD | 29 ++++++++++++++++++++++ test/integration/base_integration_test.cc | 3 +++ 4 files changed, 36 insertions(+), 3 deletions(-) diff --git a/source/common/network/listen_socket_impl.h b/source/common/network/listen_socket_impl.h index a4b7f95c37c59..3b4a29f221c17 100644 --- a/source/common/network/listen_socket_impl.h +++ b/source/common/network/listen_socket_impl.h @@ -129,6 +129,7 @@ using UdpListenSocketPtr = std::unique_ptr; class UdsListenSocket : public ListenSocketImpl { public: UdsListenSocket(const Address::InstanceConstSharedPtr& address); + UdsListenSocket(IoHandlePtr&& io_handle, const Address::InstanceConstSharedPtr& address); Socket::Type socketType() const override { return Socket::Type::Stream; } }; @@ -141,8 +142,7 @@ class InternalListenSocket : public ListenSocketImpl { // InternalListenSocket cannot be duplicated. SocketPtr duplicate() override { - return std::make_unique(/* io_handle */ nullptr, - connectionInfoProvider().localAddress()); + return std::make_unique(connectionInfoProvider().localAddress()); } }; class ConnectionSocketImpl : public SocketImpl, public ConnectionSocket { diff --git a/test/config/utility.cc b/test/config/utility.cc index ff54402af372f..f9bd88f6d5f30 100644 --- a/test/config/utility.cc +++ b/test/config/utility.cc @@ -691,6 +691,7 @@ void ConfigHelper::setConnectConfig( void ConfigHelper::applyConfigModifiers() { for (const auto& config_modifier : config_modifiers_) { config_modifier(bootstrap_); + ENVOY_LOG_MISC(debug, "lambdai: bootstrap each: {}", bootstrap_.DebugString()); } config_modifiers_.clear(); } @@ -790,7 +791,7 @@ void ConfigHelper::setHttp2(envoy::config::cluster::v3::Cluster& cluster) { void ConfigHelper::finalize(const std::vector& ports) { RELEASE_ASSERT(!finalized_, ""); - + applyConfigModifiers(); uint32_t port_idx = 0; diff --git a/test/integration/BUILD b/test/integration/BUILD index c0b28e3dd42c4..e2bf9b557abdf 100644 --- a/test/integration/BUILD +++ b/test/integration/BUILD @@ -1649,6 +1649,35 @@ envoy_cc_test( ], ) +envoy_cc_test( + name = "internal_listener_integration_test", + srcs = [ + "internal_listener_integration_test.cc", + ], + deps = [ + ":http_integration_lib", + "//source/common/config:api_version_lib", + "//source/common/event:dispatcher_includes", + "//source/common/event:dispatcher_lib", + "//source/common/network:connection_lib", + "//source/common/network:utility_lib", + "//source/extensions/filters/http/health_check:config", + "//source/extensions/filters/network/tcp_proxy:config", + "//test/common/grpc:grpc_client_integration_lib", + "//test/config:v2_link_hacks", + "//test/integration/filters:address_restore_listener_filter_lib", + "//test/test_common:network_utility_lib", + "//test/test_common:resources_lib", + "//test/test_common:utility_lib", + "@envoy_api//envoy/config/bootstrap/v3:pkg_cc_proto", + "@envoy_api//envoy/config/core/v3:pkg_cc_proto", + "@envoy_api//envoy/config/route/v3:pkg_cc_proto", + "@envoy_api//envoy/extensions/filters/network/http_connection_manager/v3:pkg_cc_proto", + "@envoy_api//envoy/extensions/filters/network/tcp_proxy/v3:pkg_cc_proto", + "@envoy_api//envoy/service/discovery/v3:pkg_cc_proto", + ], +) + envoy_cc_test( name = "listener_filter_integration_test", srcs = [ diff --git a/test/integration/base_integration_test.cc b/test/integration/base_integration_test.cc index 3caf9f43b3ec9..a12d5ecb10162 100644 --- a/test/integration/base_integration_test.cc +++ b/test/integration/base_integration_test.cc @@ -166,6 +166,7 @@ void BaseIntegrationTest::createEnvoy() { ports.push_back(upstream->localAddress()->ip()->port()); } } + ENVOY_LOG_MISC(debug, "lambdai: bootstrap before finalize: {}", config_helper_.bootstrap().DebugString()); if (use_lds_) { ENVOY_LOG_MISC(debug, "Setting up file-based LDS"); @@ -185,6 +186,8 @@ void BaseIntegrationTest::createEnvoy() { config_helper_.finalize(ports); envoy::config::bootstrap::v3::Bootstrap bootstrap = config_helper_.bootstrap(); + + // lambdai: the listener address is mutated here if (use_lds_) { // After the config has been finalized, write the final listener config to the lds file. const std::string lds_path = config_helper_.bootstrap().dynamic_resources().lds_config().path(); From 254a12f81122ba0ed72a2bdcb3ec0acaae5eadf6 Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Tue, 5 Oct 2021 23:06:27 +0000 Subject: [PATCH 11/31] add integration test Signed-off-by: Yuchen Dai --- source/common/network/listen_socket_impl.h | 7 ++ source/server/active_internal_listener.cc | 5 - source/server/listener_impl.cc | 3 +- source/server/listener_manager_impl.cc | 6 +- test/config/utility.cc | 3 +- test/integration/base_integration_test.cc | 3 - .../internal_listener_integration_test.cc | 118 ++++++++++++++++++ test/server/BUILD | 30 ----- test/server/connection_handler_test.cc | 44 ------- 9 files changed, 130 insertions(+), 89 deletions(-) create mode 100644 test/integration/internal_listener_integration_test.cc diff --git a/source/common/network/listen_socket_impl.h b/source/common/network/listen_socket_impl.h index 3b4a29f221c17..9f10034323150 100644 --- a/source/common/network/listen_socket_impl.h +++ b/source/common/network/listen_socket_impl.h @@ -144,7 +144,14 @@ class InternalListenSocket : public ListenSocketImpl { SocketPtr duplicate() override { return std::make_unique(connectionInfoProvider().localAddress()); } + + void close() override { ASSERT(io_handle_ == nullptr); } + bool isOpen() const override { + ASSERT(io_handle_ == nullptr); + return false; + } }; + class ConnectionSocketImpl : public SocketImpl, public ConnectionSocket { public: ConnectionSocketImpl(IoHandlePtr&& io_handle, diff --git a/source/server/active_internal_listener.cc b/source/server/active_internal_listener.cc index 310696816880a..45d051de19b74 100644 --- a/source/server/active_internal_listener.cc +++ b/source/server/active_internal_listener.cc @@ -55,11 +55,6 @@ void ActiveInternalListener::onAccept(Network::ConnectionSocketPtr&& socket) { auto active_socket = std::make_unique( *this, std::move(socket), false /* do not handle off at internal listener */); - // TODO(lambdai): restore address from either socket options, or from listener config. - active_socket->socket_->connectionInfoProvider().restoreLocalAddress( - std::make_shared("255.255.255.255", 0)); - active_socket->socket_->connectionInfoProvider().setRemoteAddress( - std::make_shared("255.255.255.254", 0)); onSocketAccepted(std::move(active_socket)); } diff --git a/source/server/listener_impl.cc b/source/server/listener_impl.cc index 62901f9e4a61d..059858316940e 100644 --- a/source/server/listener_impl.cc +++ b/source/server/listener_impl.cc @@ -391,8 +391,7 @@ ListenerImpl::ListenerImpl(ListenerImpl& origin, validation_visitor_( added_via_api_ ? parent_.server_.messageValidationContext().dynamicValidationVisitor() : parent_.server_.messageValidationContext().staticValidationVisitor()), - // listener_init_target_ is not used during in place update because we expect server - // started. + // listener_init_target_ is not used during in place update because we expect server started. listener_init_target_("", nullptr), dynamic_init_manager_(std::make_unique( fmt::format("Listener-local-init-manager {} {}", name, hash))), diff --git a/source/server/listener_manager_impl.cc b/source/server/listener_manager_impl.cc index c21e1642a9a3f..2c5524bae825d 100644 --- a/source/server/listener_manager_impl.cc +++ b/source/server/listener_manager_impl.cc @@ -190,9 +190,9 @@ Network::SocketSharedPtr ProdListenerComponentFactory::createListenSocket( } return std::make_shared(address); } else if (address->type() == Network::Address::Type::EnvoyInternal) { - if (socket_type != Network::Socket::Type::Stream) { - throw EnvoyException( - fmt::format("socket type {} not supported for EnvoyInternalAddress", toString(socket_type))); + if (socket_type != Network::Socket::Type::Stream) { + throw EnvoyException(fmt::format("socket type {} not supported for EnvoyInternalAddress", + toString(socket_type))); } return std::make_shared(address); } diff --git a/test/config/utility.cc b/test/config/utility.cc index f9bd88f6d5f30..ff54402af372f 100644 --- a/test/config/utility.cc +++ b/test/config/utility.cc @@ -691,7 +691,6 @@ void ConfigHelper::setConnectConfig( void ConfigHelper::applyConfigModifiers() { for (const auto& config_modifier : config_modifiers_) { config_modifier(bootstrap_); - ENVOY_LOG_MISC(debug, "lambdai: bootstrap each: {}", bootstrap_.DebugString()); } config_modifiers_.clear(); } @@ -791,7 +790,7 @@ void ConfigHelper::setHttp2(envoy::config::cluster::v3::Cluster& cluster) { void ConfigHelper::finalize(const std::vector& ports) { RELEASE_ASSERT(!finalized_, ""); - + applyConfigModifiers(); uint32_t port_idx = 0; diff --git a/test/integration/base_integration_test.cc b/test/integration/base_integration_test.cc index a12d5ecb10162..3caf9f43b3ec9 100644 --- a/test/integration/base_integration_test.cc +++ b/test/integration/base_integration_test.cc @@ -166,7 +166,6 @@ void BaseIntegrationTest::createEnvoy() { ports.push_back(upstream->localAddress()->ip()->port()); } } - ENVOY_LOG_MISC(debug, "lambdai: bootstrap before finalize: {}", config_helper_.bootstrap().DebugString()); if (use_lds_) { ENVOY_LOG_MISC(debug, "Setting up file-based LDS"); @@ -186,8 +185,6 @@ void BaseIntegrationTest::createEnvoy() { config_helper_.finalize(ports); envoy::config::bootstrap::v3::Bootstrap bootstrap = config_helper_.bootstrap(); - - // lambdai: the listener address is mutated here if (use_lds_) { // After the config has been finalized, write the final listener config to the lds file. const std::string lds_path = config_helper_.bootstrap().dynamic_resources().lds_config().path(); diff --git a/test/integration/internal_listener_integration_test.cc b/test/integration/internal_listener_integration_test.cc new file mode 100644 index 0000000000000..20105992f6ddb --- /dev/null +++ b/test/integration/internal_listener_integration_test.cc @@ -0,0 +1,118 @@ +#include "envoy/config/bootstrap/v3/bootstrap.pb.h" +#include "envoy/config/core/v3/config_source.pb.h" +#include "envoy/config/core/v3/grpc_service.pb.h" +#include "envoy/config/route/v3/route.pb.h" +#include "envoy/config/route/v3/scoped_route.pb.h" +#include "envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.pb.h" +#include "envoy/extensions/filters/network/tcp_proxy/v3/tcp_proxy.pb.h" +#include "envoy/network/connection.h" +#include "envoy/service/discovery/v3/discovery.pb.h" + +#include "source/common/config/api_version.h" + +#include "test/common/grpc/grpc_client_integration.h" +#include "test/config/v2_link_hacks.h" +#include "test/integration/http_integration.h" +#include "test/test_common/network_utility.h" +#include "test/test_common/printers.h" +#include "test/test_common/resources.h" + +#include "absl/strings/str_cat.h" +#include "gmock/gmock.h" +#include "gtest/gtest.h" + +namespace Envoy { +namespace { + +class InternalListenerIntegrationTest : public testing::TestWithParam, + public BaseIntegrationTest { +public: + InternalListenerIntegrationTest() + : BaseIntegrationTest(GetParam(), ConfigHelper::baseConfig() + +R"EOF( + filter_chains: + - filters: + - name: envoy.filters.network.tcp_proxy + typed_config: + "@type": type.googleapis.com/envoy.extensions.filters.network.tcp_proxy.v3.TcpProxy + stat_prefix: tcp_stats + cluster: cluster_0 +)EOF") {} + + void initialize() override { + config_helper_.renameListener("tcp"); + config_helper_.addConfigModifier([](envoy::config::bootstrap::v3::Bootstrap& bootstrap) { + auto& listener = *bootstrap.mutable_static_resources()->mutable_listeners(0); + listener.mutable_address()->mutable_envoy_internal_address()->set_server_listener_name( + "internal_listener"); + listener.mutable_internal_listener(); + }); + BaseIntegrationTest::initialize(); + } +}; + +TEST_P(InternalListenerIntegrationTest, Basic) { + initialize(); + EXPECT_EQ(1, test_server_->counter("listener_manager.lds.update_success")->value()); + + ConfigHelper new_config_helper( + version_, *api_, MessageUtil::getJsonStringFromMessageOrDie(config_helper_.bootstrap())); + new_config_helper.addConfigModifier( + [&](envoy::config::bootstrap::v3::Bootstrap& bootstrap) -> void { + auto* listener = bootstrap.mutable_static_resources()->mutable_listeners(0); + (*(*listener->mutable_metadata()->mutable_filter_metadata())["random_filter_name"] + .mutable_fields())["random_key"] + .set_number_value(1); + }); + + new_config_helper.setLds("1"); + + test_server_->waitForCounterEq("listener_manager.listener_modified", 1); + test_server_->waitForGaugeEq("listener_manager.total_listeners_draining", 0); +} + +TEST_P(InternalListenerIntegrationTest, InplaceUpdate) { + initialize(); + EXPECT_EQ(1, test_server_->counter("listener_manager.lds.update_success")->value()); + + ConfigHelper new_config_helper( + version_, *api_, MessageUtil::getJsonStringFromMessageOrDie(config_helper_.bootstrap())); + new_config_helper.addConfigModifier( + [&](envoy::config::bootstrap::v3::Bootstrap& bootstrap) -> void { + auto* listener = bootstrap.mutable_static_resources()->mutable_listeners(0); + auto new_filter_chain = *listener->mutable_filter_chains(0); + listener->mutable_filter_chains()->Add()->MergeFrom(new_filter_chain); + *(listener->mutable_filter_chains(1) + ->mutable_filter_chain_match() + ->mutable_application_protocols() + ->Add()) = "alpn"; + }); + + new_config_helper.setLds("1"); + + test_server_->waitForCounterEq("listener_manager.listener_modified", 1); + test_server_->waitForGaugeEq("listener_manager.total_listeners_draining", 0); +} + +TEST_P(InternalListenerIntegrationTest, DeleteListener) { + initialize(); + EXPECT_EQ(1, test_server_->counter("listener_manager.lds.update_success")->value()); + + ConfigHelper new_config_helper( + version_, *api_, MessageUtil::getJsonStringFromMessageOrDie(config_helper_.bootstrap())); + new_config_helper.addConfigModifier( + [&](envoy::config::bootstrap::v3::Bootstrap& bootstrap) -> void { + bootstrap.mutable_static_resources()->mutable_listeners()->RemoveLast(); + }); + + new_config_helper.setLds("1"); + + test_server_->waitForCounterEq("listener_manager.listener_removed", 1); + test_server_->waitForGaugeEq("listener_manager.total_listeners_draining", 0); +} + +INSTANTIATE_TEST_SUITE_P(IpVersions, InternalListenerIntegrationTest, + testing::ValuesIn(TestEnvironment::getIpVersionsForTest()), + TestUtility::ipTestParamsToString); + +} // namespace +} // namespace Envoy diff --git a/test/server/BUILD b/test/server/BUILD index 219d6a74bd591..ffb27b45f4723 100644 --- a/test/server/BUILD +++ b/test/server/BUILD @@ -74,50 +74,20 @@ envoy_cc_test( name = "connection_handler_test", srcs = ["connection_handler_test.cc"], deps = [ - ":utility_lib", - "//source/common/api:os_sys_calls_lib", "//source/common/common:utility_lib", - "//source/common/config:metadata_lib", "//source/common/config:utility_lib", - "//source/common/init:manager_lib", - "//source/common/network:addr_family_aware_socket_option_lib", "//source/common/network:address_lib", "//source/common/network:connection_balancer_lib", - "//source/common/network:listen_socket_lib", - "//source/common/network:socket_option_lib", "//source/common/network:udp_packet_writer_handler_lib", - "//source/common/network:utility_lib", - "//source/common/protobuf", "//source/common/stats:stats_lib", - "//source/extensions/filters/listener/original_dst:config", - "//source/extensions/filters/listener/proxy_protocol:config", - "//source/extensions/filters/listener/tls_inspector:config", - "//source/extensions/filters/network/http_connection_manager:config", - "//source/extensions/filters/network/tcp_proxy:config", - "//source/extensions/request_id/uuid:config", - "//source/extensions/transport_sockets/raw_buffer:config", - "//source/extensions/transport_sockets/tls:config", - "//source/extensions/transport_sockets/tls:ssl_socket_lib", "//source/server:active_raw_udp_listener_config", "//source/server:connection_handler_lib", - "//source/server:listener_manager_lib", "//test/mocks/access_log:access_log_mocks", "//test/mocks/api:api_mocks", - "//test/mocks/init:init_mocks", "//test/mocks/network:network_mocks", - "//test/mocks/server:drain_manager_mocks", - "//test/mocks/server:guard_dog_mocks", - "//test/mocks/server:instance_mocks", - "//test/mocks/server:listener_component_factory_mocks", - "//test/mocks/server:worker_factory_mocks", - "//test/mocks/server:worker_mocks", - "//test/test_common:environment_lib", "//test/test_common:network_utility_lib", "//test/test_common:test_runtime_lib", "//test/test_common:threadsafe_singleton_injector_lib", - "@envoy_api//envoy/admin/v3:pkg_cc_proto", - "@envoy_api//envoy/config/core/v3:pkg_cc_proto", - "@envoy_api//envoy/config/listener/v3:pkg_cc_proto", ], ) diff --git a/test/server/connection_handler_test.cc b/test/server/connection_handler_test.cc index 6a609b0554fd6..b57e542e593ef 100644 --- a/test/server/connection_handler_test.cc +++ b/test/server/connection_handler_test.cc @@ -4,50 +4,6 @@ #include #include -#include "envoy/admin/v3/config_dump.pb.h" -#include "envoy/config/core/v3/address.pb.h" -#include "envoy/config/core/v3/base.pb.h" -#include "envoy/config/core/v3/config_source.pb.h" -#include "envoy/config/listener/v3/listener.pb.h" -#include "envoy/network/listener.h" -#include "envoy/server/filter_config.h" -#include "envoy/server/listener_manager.h" -#include "envoy/stream_info/filter_state.h" - -#include "source/common/api/os_sys_calls_impl.h" -#include "source/common/common/macros.h" -#include "source/common/config/metadata.h" -#include "source/common/init/manager_impl.h" -#include "source/common/network/address_impl.h" -#include "source/common/network/io_socket_handle_impl.h" -#include "source/common/network/utility.h" -#include "source/common/protobuf/protobuf.h" -#include "source/server/configuration_impl.h" -#include "source/server/listener_manager_impl.h" - -#include "test/mocks/init/mocks.h" -#include "test/mocks/network/mocks.h" -#include "test/mocks/server/drain_manager.h" -#include "test/mocks/server/guard_dog.h" -#include "test/mocks/server/instance.h" -#include "test/mocks/server/listener_component_factory.h" -#include "test/mocks/server/worker.h" -#include "test/mocks/server/worker_factory.h" -#include "test/server/utility.h" -#include "test/test_common/environment.h" -#include "test/test_common/network_utility.h" -#include "test/test_common/test_runtime.h" -#include "test/test_common/threadsafe_singleton_injector.h" -#include "test/test_common/utility.h" - -#include "absl/strings/escaping.h" -#include "absl/strings/match.h" - -// Above from listener_manager_impl_test.cc - -#include "envoy/config/core/v3/base.pb.h" -#include "envoy/config/listener/v3/udp_listener_config.pb.h" -#include "envoy/network/exception.h" #include "envoy/network/filter.h" #include "envoy/stats/scope.h" From 131eaa9f679f7d47b041db72ee3f8d261d73ac59 Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Mon, 4 Oct 2021 22:52:24 +0000 Subject: [PATCH 12/31] listener: fix duplicate and isOpen of the listener socket Signed-off-by: Yuchen Dai --- docs/root/version_history/current.rst | 1 + source/common/network/listen_socket_impl.h | 41 ++++++- .../common/network/listen_socket_impl_test.cc | 28 +++-- .../address_restore_listener_filter.cc | 12 ++- .../listener_lds_integration_test.cc | 102 +++++++++++------- test/per_file_coverage.sh | 2 +- 6 files changed, 137 insertions(+), 49 deletions(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 48df3345b5b68..f8a37b9a8499b 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -90,6 +90,7 @@ Bug Fixes * listener: fixed an issue on Windows where connections are not handled by all worker threads. * lua: fix ``BodyBuffer`` setting a Lua string and printing Lua string containing hex characters. Previously, ``BodyBuffer`` setting a Lua string or printing strings with hex characters will be truncated. * xray: fix the AWS X-Ray tracer bug where span's error, fault and throttle information was not reported properly as per the `AWS X-Ray documentation `_. Before this fix, server error was reported under the 'annotations' section of the segment data. +* listener: fixed the crash when updating listeners that do not bind to port. Removed Config or Runtime ------------------------- diff --git a/source/common/network/listen_socket_impl.h b/source/common/network/listen_socket_impl.h index 9f10034323150..1ada8b9f4be39 100644 --- a/source/common/network/listen_socket_impl.h +++ b/source/common/network/listen_socket_impl.h @@ -17,6 +17,13 @@ namespace Envoy { namespace Network { +namespace { +std::string getConnectionInfoString(const ConnectionInfoProvider& conn_info) { + std::stringstream out; + conn_info.dumpState(out, 0); + return out.str(); +} +} // namespace class ListenSocketImpl : public SocketImpl { protected: @@ -33,6 +40,23 @@ class ListenSocketImpl : public SocketImpl { void setupSocket(const Network::Socket::OptionsSharedPtr& options); void setListenSocketOptions(const Network::Socket::OptionsSharedPtr& options); Api::SysCallIntResult bind(Network::Address::InstanceConstSharedPtr address) override; + + void close() override { + RELEASE_ASSERT(io_handle_ != nullptr, + absl::StrCat(__FUNCTION__, + " is called from the ListenSocket with no io handle. Socket info: ", + getConnectionInfoString(*connection_info_provider_))); + if (io_handle_->isOpen()) { + io_handle_->close(); + } + } + bool isOpen() const override { + RELEASE_ASSERT(io_handle_ != nullptr, + absl::StrCat(__FUNCTION__, + " is called from the ListenSocket with no io handle. Socket info: ", + getConnectionInfoString(*connection_info_provider_))); + return io_handle_->isOpen(); + } }; /** @@ -79,6 +103,18 @@ template class NetworkListenSocket : public ListenSocketImpl { Socket::Type socketType() const override { return T::type; } + SocketPtr duplicate() override { + if (io_handle_ == nullptr) { + // This is a listen socket that does not bind to port. Pass nullptr socket options. + return std::make_unique>(connection_info_provider_->localAddress(), + /*options=*/nullptr, /*bind_to_port*/ false); + } else { + // TODO(lambdai): verify if duplicate is all the need to set up a TCP/UDP socket. Should + // socket options be applied along with duplicate? + return ListenSocketImpl::duplicate(); + } + } + // These four overrides are introduced to perform check. A null io handle is possible only if the // the owner socket is a listen socket that does not bind to port. IoHandle& ioHandle() override { @@ -97,8 +133,9 @@ template class NetworkListenSocket : public ListenSocketImpl { } } bool isOpen() const override { - ASSERT(io_handle_ != nullptr); - return io_handle_->isOpen(); + return io_handle_ == nullptr ? false // Consider listen socket as closed if it does not bind to + // port. No fd will leak. + : io_handle_->isOpen(); } protected: diff --git a/test/common/network/listen_socket_impl_test.cc b/test/common/network/listen_socket_impl_test.cc index 8f514e993084c..8ff7193bee225 100644 --- a/test/common/network/listen_socket_impl_test.cc +++ b/test/common/network/listen_socket_impl_test.cc @@ -35,16 +35,15 @@ TEST(ConnectionSocketImplTest, LowerCaseRequestedServerName) { template class ListenSocketImplTest : public testing::TestWithParam { + using ListenSocketType = NetworkListenSocket>; + protected: ListenSocketImplTest() : version_(GetParam()) {} const Address::IpVersion version_; template - std::unique_ptr createListenSocketPtr(Args&&... args) { - using NetworkSocketTraitType = NetworkSocketTrait; - - return std::make_unique>( - std::forward(args)...); + std::unique_ptr createListenSocketPtr(Args&&... args) { + return std::make_unique(std::forward(args)...); } void testBindSpecificPort() { @@ -76,7 +75,7 @@ class ListenSocketImplTest : public testing::TestWithParam { EXPECT_CALL(*option, setOption(_, envoy::config::core::v3::SocketOption::STATE_PREBIND)) .WillOnce(Return(true)); options->emplace_back(std::move(option)); - std::unique_ptr socket1; + std::unique_ptr socket1; try { socket1 = createListenSocketPtr(addr, options, true); } catch (SocketBindException& e) { @@ -139,6 +138,19 @@ class ListenSocketImplTest : public testing::TestWithParam { EXPECT_GT(socket->connectionInfoProvider().localAddress()->ip()->port(), 0U); EXPECT_EQ(Type, socket->socketType()); } + + // Verify that a listen sockets that do not bind to port can be duplicated and closed. + void testNotBindToPort() { + auto local_address = version_ == Address::IpVersion::v4 ? Utility::getIpv6AnyAddress() + : Utility::getIpv4AnyAddress(); + auto socket = NetworkListenSocket>(local_address, nullptr, + /*bind_to_port=*/false); + auto dup_socket = socket.duplicate(); + EXPECT_FALSE(socket.isOpen()); + EXPECT_FALSE(dup_socket->isOpen()); + socket.close(); + dup_socket->close(); + } }; using ListenSocketImplTestTcp = ListenSocketImplTest; @@ -228,6 +240,10 @@ TEST_P(ListenSocketImplTestTcp, BindPortZero) { testBindPortZero(); } TEST_P(ListenSocketImplTestUdp, BindPortZero) { testBindPortZero(); } +TEST_P(ListenSocketImplTestTcp, NotBindToPortAccess) { testNotBindToPort(); } + +TEST_P(ListenSocketImplTestUdp, NotBindToPortAccess) { testNotBindToPort(); } + } // namespace } // namespace Network } // namespace Envoy diff --git a/test/integration/filters/address_restore_listener_filter.cc b/test/integration/filters/address_restore_listener_filter.cc index 84cd33d2c14e7..ed1605924d8dc 100644 --- a/test/integration/filters/address_restore_listener_filter.cc +++ b/test/integration/filters/address_restore_listener_filter.cc @@ -10,14 +10,22 @@ namespace Envoy { // The FakeOriginalDstListenerFilter restore desired local address without the dependency of OS. +// Ipv6 and Ipv4 addresses are restored to the corresponding loopback ip address and port 80. class FakeOriginalDstListenerFilter : public Network::ListenerFilter { public: // Network::ListenerFilter Network::FilterStatus onAccept(Network::ListenerFilterCallbacks& cb) override { FANCY_LOG(debug, "in FakeOriginalDstListenerFilter::onAccept"); Network::ConnectionSocket& socket = cb.socket(); - socket.connectionInfoProvider().restoreLocalAddress( - std::make_shared("127.0.0.2", 80)); + auto local_address = socket.connectionInfoProvider().localAddress(); + if (local_address != nullptr && + local_address->ip()->version() == Network::Address::IpVersion::v6) { + socket.connectionInfoProvider().restoreLocalAddress( + std::make_shared("::1", 80)); + } else { + socket.connectionInfoProvider().restoreLocalAddress( + std::make_shared("127.0.0.1", 80)); + } FANCY_LOG(debug, "current local socket address is {} restored = {}", socket.connectionInfoProvider().localAddress()->asString(), socket.connectionInfoProvider().localAddressRestored()); diff --git a/test/integration/listener_lds_integration_test.cc b/test/integration/listener_lds_integration_test.cc index 47d7859bf5f03..0cb583a4ed5e0 100644 --- a/test/integration/listener_lds_integration_test.cc +++ b/test/integration/listener_lds_integration_test.cc @@ -554,6 +554,11 @@ TEST_P(ListenerIntegrationTest, ChangeListenerAddress) { EXPECT_EQ(request_size, upstream_request_->bodyLength()); } +struct PerConnection { + std::string response_; + std::unique_ptr client_conn_; + FakeRawConnectionPtr upstream_conn_; +}; class RebalancerTest : public testing::TestWithParam, public BaseIntegrationTest { public: @@ -585,10 +590,7 @@ class RebalancerTest : public testing::TestWithParamset_value(false); virtual_listener_config.set_name("balanced_target_listener"); virtual_listener_config.mutable_connection_balance_config()->mutable_exact_balance(); - - // TODO(lambdai): Replace by getLoopbackAddressUrlString to emulate the real world. - *virtual_listener_config.mutable_address()->mutable_socket_address()->mutable_address() = - "127.0.0.2"; + *virtual_listener_config.mutable_stat_prefix() = target_listener_prefix_; virtual_listener_config.mutable_address()->mutable_socket_address()->set_port_value(80); }); BaseIntegrationTest::initialize(); @@ -604,14 +606,66 @@ class RebalancerTest : public testing::TestWithParam client_conn_; - FakeRawConnectionPtr upstream_conn_; + void verifyBalance(uint32_t repeats = 10) { + // The balancer is balanced as per active connection instead of total connection. + // The below vector maintains all the connections alive. + std::vector connections; + for (uint32_t i = 0; i < repeats * concurrency_; ++i) { + connections.emplace_back(); + connections.back().client_conn_ = + createConnectionAndWrite("dummy", connections.back().response_); + connections.back().client_conn_->waitForConnection(); + ASSERT_TRUE(fake_upstreams_[0]->waitForRawConnection(connections.back().upstream_conn_)); + } + for (auto& conn : connections) { + conn.client_conn_->close(); + while (!conn.client_conn_->closed()) { + dispatcher_->run(Event::Dispatcher::RunType::NonBlock); + } + } + ASSERT_EQ(TestUtility::findCounter(test_server_->statStore(), + absl::StrCat("listener.", target_listener_prefix_, + ".worker_0.downstream_cx_total")) + ->value(), + repeats); + ASSERT_EQ(TestUtility::findCounter(test_server_->statStore(), + absl::StrCat("listener.", target_listener_prefix_, + ".worker_1.downstream_cx_total")) + ->value(), + repeats); + } + + // The stats prefix that shared by ipv6 and ipv4 listener. + std::string target_listener_prefix_{"balanced_listener"}; }; +TEST_P(RebalancerTest, BindToPortUpdate) { + concurrency_ = 2; + initialize(); + + ConfigHelper new_config_helper( + version_, *api_, MessageUtil::getJsonStringFromMessageOrDie(config_helper_.bootstrap())); + + new_config_helper.addConfigModifier([&](envoy::config::bootstrap::v3::Bootstrap& bootstrap) + -> void { + // This virtual listener need updating. + auto& virtual_listener_config = *bootstrap.mutable_static_resources()->mutable_listeners(1); + *virtual_listener_config.mutable_address()->mutable_socket_address()->mutable_address() = + bootstrap.static_resources().listeners(0).address().socket_address().address(); + (*(*virtual_listener_config.mutable_metadata()->mutable_filter_metadata())["random_filter_name"] + .mutable_fields())["random_key"] + .set_number_value(2); + }); + // Create an LDS response with the new config, and reload config. + new_config_helper.setLds("1"); + + test_server_->waitForCounterEq("listener_manager.listener_modified", 1); + test_server_->waitForGaugeEq("listener_manager.total_listeners_draining", 0); + + verifyBalance(); +} + // Verify the connections are distributed evenly on the 2 worker threads of the redirected // listener. // Currently flaky because the virtual listener create listen socket anyway despite the socket is @@ -620,36 +674,8 @@ TEST_P(RebalancerTest, DISABLED_RedirectConnectionIsBalancedOnDestinationListene auto ip_address_str = Network::Test::getLoopbackAddressUrlString(TestEnvironment::getIpVersionsForTest().front()); concurrency_ = 2; - int repeats = 10; initialize(); - - // The balancer is balanced as per active connection instead of total connection. - // The below vector maintains all the connections alive. - std::vector connections; - for (uint32_t i = 0; i < repeats * concurrency_; ++i) { - connections.emplace_back(); - connections.back().client_conn_ = - createConnectionAndWrite("dummy", connections.back().response_); - connections.back().client_conn_->waitForConnection(); - ASSERT_TRUE(fake_upstreams_[0]->waitForRawConnection(connections.back().upstream_conn_)); - } - for (auto& conn : connections) { - conn.client_conn_->close(); - while (!conn.client_conn_->closed()) { - dispatcher_->run(Event::Dispatcher::RunType::NonBlock); - } - } - - ASSERT_EQ(TestUtility::findCounter( - test_server_->statStore(), - absl::StrCat("listener.", ip_address_str, "_80.worker_0.downstream_cx_total")) - ->value(), - repeats); - ASSERT_EQ(TestUtility::findCounter( - test_server_->statStore(), - absl::StrCat("listener.", ip_address_str, "_80.worker_1.downstream_cx_total")) - ->value(), - repeats); + verifyBalance(); } INSTANTIATE_TEST_SUITE_P(IpVersions, RebalancerTest, diff --git a/test/per_file_coverage.sh b/test/per_file_coverage.sh index 701be12081137..fb445b5305652 100755 --- a/test/per_file_coverage.sh +++ b/test/per_file_coverage.sh @@ -14,7 +14,7 @@ declare -a KNOWN_LOW_COVERAGE=( "source/common/http:96.5" "source/common/json:90.1" "source/common/matcher:94.2" -"source/common/network:94.8" # Flaky, `activateFileEvents`, `startSecureTransport` and `ioctl` do not always report LCOV +"source/common/network:94.4" # Flaky, `activateFileEvents`, `startSecureTransport` and `ioctl`, listener_socket do not always report LCOV "source/common/protobuf:95.3" "source/common/quic:91.8" "source/common/secret:96.3" From cb8f53d22b6ef372aac73d1e9c47db29c511a541 Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Tue, 5 Oct 2021 16:19:41 +0000 Subject: [PATCH 13/31] add integration test Signed-off-by: Yuchen Dai --- source/common/network/listen_socket_impl.h | 11 +- source/server/active_internal_listener.cc | 5 - source/server/listener_impl.cc | 3 +- source/server/listener_manager_impl.cc | 6 +- test/integration/BUILD | 29 +++++ .../internal_listener_integration_test.cc | 118 ++++++++++++++++++ test/server/BUILD | 30 ----- test/server/connection_handler_test.cc | 44 ------- 8 files changed, 160 insertions(+), 86 deletions(-) create mode 100644 test/integration/internal_listener_integration_test.cc diff --git a/source/common/network/listen_socket_impl.h b/source/common/network/listen_socket_impl.h index a4b7f95c37c59..9f10034323150 100644 --- a/source/common/network/listen_socket_impl.h +++ b/source/common/network/listen_socket_impl.h @@ -129,6 +129,7 @@ using UdpListenSocketPtr = std::unique_ptr; class UdsListenSocket : public ListenSocketImpl { public: UdsListenSocket(const Address::InstanceConstSharedPtr& address); + UdsListenSocket(IoHandlePtr&& io_handle, const Address::InstanceConstSharedPtr& address); Socket::Type socketType() const override { return Socket::Type::Stream; } }; @@ -141,10 +142,16 @@ class InternalListenSocket : public ListenSocketImpl { // InternalListenSocket cannot be duplicated. SocketPtr duplicate() override { - return std::make_unique(/* io_handle */ nullptr, - connectionInfoProvider().localAddress()); + return std::make_unique(connectionInfoProvider().localAddress()); + } + + void close() override { ASSERT(io_handle_ == nullptr); } + bool isOpen() const override { + ASSERT(io_handle_ == nullptr); + return false; } }; + class ConnectionSocketImpl : public SocketImpl, public ConnectionSocket { public: ConnectionSocketImpl(IoHandlePtr&& io_handle, diff --git a/source/server/active_internal_listener.cc b/source/server/active_internal_listener.cc index 310696816880a..45d051de19b74 100644 --- a/source/server/active_internal_listener.cc +++ b/source/server/active_internal_listener.cc @@ -55,11 +55,6 @@ void ActiveInternalListener::onAccept(Network::ConnectionSocketPtr&& socket) { auto active_socket = std::make_unique( *this, std::move(socket), false /* do not handle off at internal listener */); - // TODO(lambdai): restore address from either socket options, or from listener config. - active_socket->socket_->connectionInfoProvider().restoreLocalAddress( - std::make_shared("255.255.255.255", 0)); - active_socket->socket_->connectionInfoProvider().setRemoteAddress( - std::make_shared("255.255.255.254", 0)); onSocketAccepted(std::move(active_socket)); } diff --git a/source/server/listener_impl.cc b/source/server/listener_impl.cc index 62901f9e4a61d..059858316940e 100644 --- a/source/server/listener_impl.cc +++ b/source/server/listener_impl.cc @@ -391,8 +391,7 @@ ListenerImpl::ListenerImpl(ListenerImpl& origin, validation_visitor_( added_via_api_ ? parent_.server_.messageValidationContext().dynamicValidationVisitor() : parent_.server_.messageValidationContext().staticValidationVisitor()), - // listener_init_target_ is not used during in place update because we expect server - // started. + // listener_init_target_ is not used during in place update because we expect server started. listener_init_target_("", nullptr), dynamic_init_manager_(std::make_unique( fmt::format("Listener-local-init-manager {} {}", name, hash))), diff --git a/source/server/listener_manager_impl.cc b/source/server/listener_manager_impl.cc index c21e1642a9a3f..2c5524bae825d 100644 --- a/source/server/listener_manager_impl.cc +++ b/source/server/listener_manager_impl.cc @@ -190,9 +190,9 @@ Network::SocketSharedPtr ProdListenerComponentFactory::createListenSocket( } return std::make_shared(address); } else if (address->type() == Network::Address::Type::EnvoyInternal) { - if (socket_type != Network::Socket::Type::Stream) { - throw EnvoyException( - fmt::format("socket type {} not supported for EnvoyInternalAddress", toString(socket_type))); + if (socket_type != Network::Socket::Type::Stream) { + throw EnvoyException(fmt::format("socket type {} not supported for EnvoyInternalAddress", + toString(socket_type))); } return std::make_shared(address); } diff --git a/test/integration/BUILD b/test/integration/BUILD index c0b28e3dd42c4..e2bf9b557abdf 100644 --- a/test/integration/BUILD +++ b/test/integration/BUILD @@ -1649,6 +1649,35 @@ envoy_cc_test( ], ) +envoy_cc_test( + name = "internal_listener_integration_test", + srcs = [ + "internal_listener_integration_test.cc", + ], + deps = [ + ":http_integration_lib", + "//source/common/config:api_version_lib", + "//source/common/event:dispatcher_includes", + "//source/common/event:dispatcher_lib", + "//source/common/network:connection_lib", + "//source/common/network:utility_lib", + "//source/extensions/filters/http/health_check:config", + "//source/extensions/filters/network/tcp_proxy:config", + "//test/common/grpc:grpc_client_integration_lib", + "//test/config:v2_link_hacks", + "//test/integration/filters:address_restore_listener_filter_lib", + "//test/test_common:network_utility_lib", + "//test/test_common:resources_lib", + "//test/test_common:utility_lib", + "@envoy_api//envoy/config/bootstrap/v3:pkg_cc_proto", + "@envoy_api//envoy/config/core/v3:pkg_cc_proto", + "@envoy_api//envoy/config/route/v3:pkg_cc_proto", + "@envoy_api//envoy/extensions/filters/network/http_connection_manager/v3:pkg_cc_proto", + "@envoy_api//envoy/extensions/filters/network/tcp_proxy/v3:pkg_cc_proto", + "@envoy_api//envoy/service/discovery/v3:pkg_cc_proto", + ], +) + envoy_cc_test( name = "listener_filter_integration_test", srcs = [ diff --git a/test/integration/internal_listener_integration_test.cc b/test/integration/internal_listener_integration_test.cc new file mode 100644 index 0000000000000..20105992f6ddb --- /dev/null +++ b/test/integration/internal_listener_integration_test.cc @@ -0,0 +1,118 @@ +#include "envoy/config/bootstrap/v3/bootstrap.pb.h" +#include "envoy/config/core/v3/config_source.pb.h" +#include "envoy/config/core/v3/grpc_service.pb.h" +#include "envoy/config/route/v3/route.pb.h" +#include "envoy/config/route/v3/scoped_route.pb.h" +#include "envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.pb.h" +#include "envoy/extensions/filters/network/tcp_proxy/v3/tcp_proxy.pb.h" +#include "envoy/network/connection.h" +#include "envoy/service/discovery/v3/discovery.pb.h" + +#include "source/common/config/api_version.h" + +#include "test/common/grpc/grpc_client_integration.h" +#include "test/config/v2_link_hacks.h" +#include "test/integration/http_integration.h" +#include "test/test_common/network_utility.h" +#include "test/test_common/printers.h" +#include "test/test_common/resources.h" + +#include "absl/strings/str_cat.h" +#include "gmock/gmock.h" +#include "gtest/gtest.h" + +namespace Envoy { +namespace { + +class InternalListenerIntegrationTest : public testing::TestWithParam, + public BaseIntegrationTest { +public: + InternalListenerIntegrationTest() + : BaseIntegrationTest(GetParam(), ConfigHelper::baseConfig() + +R"EOF( + filter_chains: + - filters: + - name: envoy.filters.network.tcp_proxy + typed_config: + "@type": type.googleapis.com/envoy.extensions.filters.network.tcp_proxy.v3.TcpProxy + stat_prefix: tcp_stats + cluster: cluster_0 +)EOF") {} + + void initialize() override { + config_helper_.renameListener("tcp"); + config_helper_.addConfigModifier([](envoy::config::bootstrap::v3::Bootstrap& bootstrap) { + auto& listener = *bootstrap.mutable_static_resources()->mutable_listeners(0); + listener.mutable_address()->mutable_envoy_internal_address()->set_server_listener_name( + "internal_listener"); + listener.mutable_internal_listener(); + }); + BaseIntegrationTest::initialize(); + } +}; + +TEST_P(InternalListenerIntegrationTest, Basic) { + initialize(); + EXPECT_EQ(1, test_server_->counter("listener_manager.lds.update_success")->value()); + + ConfigHelper new_config_helper( + version_, *api_, MessageUtil::getJsonStringFromMessageOrDie(config_helper_.bootstrap())); + new_config_helper.addConfigModifier( + [&](envoy::config::bootstrap::v3::Bootstrap& bootstrap) -> void { + auto* listener = bootstrap.mutable_static_resources()->mutable_listeners(0); + (*(*listener->mutable_metadata()->mutable_filter_metadata())["random_filter_name"] + .mutable_fields())["random_key"] + .set_number_value(1); + }); + + new_config_helper.setLds("1"); + + test_server_->waitForCounterEq("listener_manager.listener_modified", 1); + test_server_->waitForGaugeEq("listener_manager.total_listeners_draining", 0); +} + +TEST_P(InternalListenerIntegrationTest, InplaceUpdate) { + initialize(); + EXPECT_EQ(1, test_server_->counter("listener_manager.lds.update_success")->value()); + + ConfigHelper new_config_helper( + version_, *api_, MessageUtil::getJsonStringFromMessageOrDie(config_helper_.bootstrap())); + new_config_helper.addConfigModifier( + [&](envoy::config::bootstrap::v3::Bootstrap& bootstrap) -> void { + auto* listener = bootstrap.mutable_static_resources()->mutable_listeners(0); + auto new_filter_chain = *listener->mutable_filter_chains(0); + listener->mutable_filter_chains()->Add()->MergeFrom(new_filter_chain); + *(listener->mutable_filter_chains(1) + ->mutable_filter_chain_match() + ->mutable_application_protocols() + ->Add()) = "alpn"; + }); + + new_config_helper.setLds("1"); + + test_server_->waitForCounterEq("listener_manager.listener_modified", 1); + test_server_->waitForGaugeEq("listener_manager.total_listeners_draining", 0); +} + +TEST_P(InternalListenerIntegrationTest, DeleteListener) { + initialize(); + EXPECT_EQ(1, test_server_->counter("listener_manager.lds.update_success")->value()); + + ConfigHelper new_config_helper( + version_, *api_, MessageUtil::getJsonStringFromMessageOrDie(config_helper_.bootstrap())); + new_config_helper.addConfigModifier( + [&](envoy::config::bootstrap::v3::Bootstrap& bootstrap) -> void { + bootstrap.mutable_static_resources()->mutable_listeners()->RemoveLast(); + }); + + new_config_helper.setLds("1"); + + test_server_->waitForCounterEq("listener_manager.listener_removed", 1); + test_server_->waitForGaugeEq("listener_manager.total_listeners_draining", 0); +} + +INSTANTIATE_TEST_SUITE_P(IpVersions, InternalListenerIntegrationTest, + testing::ValuesIn(TestEnvironment::getIpVersionsForTest()), + TestUtility::ipTestParamsToString); + +} // namespace +} // namespace Envoy diff --git a/test/server/BUILD b/test/server/BUILD index 219d6a74bd591..ffb27b45f4723 100644 --- a/test/server/BUILD +++ b/test/server/BUILD @@ -74,50 +74,20 @@ envoy_cc_test( name = "connection_handler_test", srcs = ["connection_handler_test.cc"], deps = [ - ":utility_lib", - "//source/common/api:os_sys_calls_lib", "//source/common/common:utility_lib", - "//source/common/config:metadata_lib", "//source/common/config:utility_lib", - "//source/common/init:manager_lib", - "//source/common/network:addr_family_aware_socket_option_lib", "//source/common/network:address_lib", "//source/common/network:connection_balancer_lib", - "//source/common/network:listen_socket_lib", - "//source/common/network:socket_option_lib", "//source/common/network:udp_packet_writer_handler_lib", - "//source/common/network:utility_lib", - "//source/common/protobuf", "//source/common/stats:stats_lib", - "//source/extensions/filters/listener/original_dst:config", - "//source/extensions/filters/listener/proxy_protocol:config", - "//source/extensions/filters/listener/tls_inspector:config", - "//source/extensions/filters/network/http_connection_manager:config", - "//source/extensions/filters/network/tcp_proxy:config", - "//source/extensions/request_id/uuid:config", - "//source/extensions/transport_sockets/raw_buffer:config", - "//source/extensions/transport_sockets/tls:config", - "//source/extensions/transport_sockets/tls:ssl_socket_lib", "//source/server:active_raw_udp_listener_config", "//source/server:connection_handler_lib", - "//source/server:listener_manager_lib", "//test/mocks/access_log:access_log_mocks", "//test/mocks/api:api_mocks", - "//test/mocks/init:init_mocks", "//test/mocks/network:network_mocks", - "//test/mocks/server:drain_manager_mocks", - "//test/mocks/server:guard_dog_mocks", - "//test/mocks/server:instance_mocks", - "//test/mocks/server:listener_component_factory_mocks", - "//test/mocks/server:worker_factory_mocks", - "//test/mocks/server:worker_mocks", - "//test/test_common:environment_lib", "//test/test_common:network_utility_lib", "//test/test_common:test_runtime_lib", "//test/test_common:threadsafe_singleton_injector_lib", - "@envoy_api//envoy/admin/v3:pkg_cc_proto", - "@envoy_api//envoy/config/core/v3:pkg_cc_proto", - "@envoy_api//envoy/config/listener/v3:pkg_cc_proto", ], ) diff --git a/test/server/connection_handler_test.cc b/test/server/connection_handler_test.cc index 6a609b0554fd6..b57e542e593ef 100644 --- a/test/server/connection_handler_test.cc +++ b/test/server/connection_handler_test.cc @@ -4,50 +4,6 @@ #include #include -#include "envoy/admin/v3/config_dump.pb.h" -#include "envoy/config/core/v3/address.pb.h" -#include "envoy/config/core/v3/base.pb.h" -#include "envoy/config/core/v3/config_source.pb.h" -#include "envoy/config/listener/v3/listener.pb.h" -#include "envoy/network/listener.h" -#include "envoy/server/filter_config.h" -#include "envoy/server/listener_manager.h" -#include "envoy/stream_info/filter_state.h" - -#include "source/common/api/os_sys_calls_impl.h" -#include "source/common/common/macros.h" -#include "source/common/config/metadata.h" -#include "source/common/init/manager_impl.h" -#include "source/common/network/address_impl.h" -#include "source/common/network/io_socket_handle_impl.h" -#include "source/common/network/utility.h" -#include "source/common/protobuf/protobuf.h" -#include "source/server/configuration_impl.h" -#include "source/server/listener_manager_impl.h" - -#include "test/mocks/init/mocks.h" -#include "test/mocks/network/mocks.h" -#include "test/mocks/server/drain_manager.h" -#include "test/mocks/server/guard_dog.h" -#include "test/mocks/server/instance.h" -#include "test/mocks/server/listener_component_factory.h" -#include "test/mocks/server/worker.h" -#include "test/mocks/server/worker_factory.h" -#include "test/server/utility.h" -#include "test/test_common/environment.h" -#include "test/test_common/network_utility.h" -#include "test/test_common/test_runtime.h" -#include "test/test_common/threadsafe_singleton_injector.h" -#include "test/test_common/utility.h" - -#include "absl/strings/escaping.h" -#include "absl/strings/match.h" - -// Above from listener_manager_impl_test.cc - -#include "envoy/config/core/v3/base.pb.h" -#include "envoy/config/listener/v3/udp_listener_config.pb.h" -#include "envoy/network/exception.h" #include "envoy/network/filter.h" #include "envoy/stats/scope.h" From dc4116ee01293f643c7bf63be3eef383f3555e04 Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Wed, 6 Oct 2021 17:22:27 +0000 Subject: [PATCH 14/31] stash Signed-off-by: Yuchen Dai --- test/server/active_internal_listener_test.cc | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/test/server/active_internal_listener_test.cc b/test/server/active_internal_listener_test.cc index 2fcb689057ae9..51d30952ed97e 100644 --- a/test/server/active_internal_listener_test.cc +++ b/test/server/active_internal_listener_test.cc @@ -110,7 +110,6 @@ TEST_F(ActiveInternalListenerTest, AcceptSocketAndCreateListenerFilter) { } TEST_F(ActiveInternalListenerTest, AcceptSocketAndCreateNetworkFilter) { - addListener(); expectFilterChainFactory(); @@ -172,6 +171,25 @@ TEST_F(ActiveInternalListenerTest, PausedListenerAcceptNewSocket) { EXPECT_CALL(manager_, findFilterChain(_)).WillOnce(Return(nullptr)); internal_listener_->onAccept(Network::ConnectionSocketPtr{accepted_socket}); } + +TEST_F(ActiveInternalListenerTest, DestroyListenerCloseAllConnections) { + addListenerWithRealNetworkListener(); + internal_listener_->pauseListening(); + + expectFilterChainFactory(); + Network::MockConnectionSocket* accepted_socket = new NiceMock(); + + auto filter_factory_callback = std::make_shared>(); + filter_chain_ = std::make_shared>(); + auto transport_socket_factory = Network::Test::createRawBufferSocketFactory(); + + EXPECT_CALL(manager_, findFilterChain(_)).WillOnce(Return(filter_chain_.get())); + + EXPECT_CALL(filter_chain_factory_, createListenerFilterChain(_)) + .WillRepeatedly(Invoke([&](Network::ListenerFilterManager&) -> bool { return true; })); + EXPECT_CALL(manager_, findFilterChain(_)).WillOnce(Return(nullptr)); + internal_listener_->onAccept(Network::ConnectionSocketPtr{accepted_socket}); +} } // namespace } // namespace Server } // namespace Envoy From f5692cf613fd8370e064f92cef5e32c52bc8248f Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Wed, 6 Oct 2021 19:06:08 +0000 Subject: [PATCH 15/31] add new test that destroy listener when connections are alive Signed-off-by: Yuchen Dai --- test/server/active_internal_listener_test.cc | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/test/server/active_internal_listener_test.cc b/test/server/active_internal_listener_test.cc index 51d30952ed97e..5ed3369c8d09c 100644 --- a/test/server/active_internal_listener_test.cc +++ b/test/server/active_internal_listener_test.cc @@ -183,12 +183,21 @@ TEST_F(ActiveInternalListenerTest, DestroyListenerCloseAllConnections) { filter_chain_ = std::make_shared>(); auto transport_socket_factory = Network::Test::createRawBufferSocketFactory(); - EXPECT_CALL(manager_, findFilterChain(_)).WillOnce(Return(filter_chain_.get())); - EXPECT_CALL(filter_chain_factory_, createListenerFilterChain(_)) .WillRepeatedly(Invoke([&](Network::ListenerFilterManager&) -> bool { return true; })); - EXPECT_CALL(manager_, findFilterChain(_)).WillOnce(Return(nullptr)); + EXPECT_CALL(manager_, findFilterChain(_)).WillOnce(Return(filter_chain_.get())); + EXPECT_CALL(*filter_chain_, transportSocketFactory) + .WillOnce(testing::ReturnRef(*transport_socket_factory)); + EXPECT_CALL(*filter_chain_, networkFilterFactories).WillOnce(ReturnRef(*filter_factory_callback)); + auto* connection = new NiceMock(); + EXPECT_CALL(dispatcher_, createServerConnection_()).WillOnce(Return(connection)); + EXPECT_CALL(conn_handler_, incNumConnections()); + EXPECT_CALL(filter_chain_factory_, createNetworkFilterChain(_, _)).WillOnce(Return(true)); + EXPECT_CALL(listener_config_, perConnectionBufferLimitBytes()); internal_listener_->onAccept(Network::ConnectionSocketPtr{accepted_socket}); + + EXPECT_CALL(conn_handler_, decNumConnections()); + internal_listener_.reset(); } } // namespace } // namespace Server From 09d023c40b053b01c7e81e9ea74977daa8964e52 Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Wed, 6 Oct 2021 21:01:22 +0000 Subject: [PATCH 16/31] add test coverage Signed-off-by: Yuchen Dai --- test/mocks/network/mocks.h | 1 - test/server/active_internal_listener_test.cc | 28 +++++++++++++++++++- 2 files changed, 27 insertions(+), 2 deletions(-) diff --git a/test/mocks/network/mocks.h b/test/mocks/network/mocks.h index 28bf70f0b79ec..9e5d92466bf82 100644 --- a/test/mocks/network/mocks.h +++ b/test/mocks/network/mocks.h @@ -339,7 +339,6 @@ class MockConnectionSocket : public ConnectionSocket { IoHandlePtr io_handle_; std::shared_ptr connection_info_provider_; - bool is_closed_; }; class MockListenerFilterCallbacks : public ListenerFilterCallbacks { diff --git a/test/server/active_internal_listener_test.cc b/test/server/active_internal_listener_test.cc index 5ed3369c8d09c..d065a4879c6d1 100644 --- a/test/server/active_internal_listener_test.cc +++ b/test/server/active_internal_listener_test.cc @@ -88,7 +88,6 @@ TEST_F(ActiveInternalListenerTest, AcceptSocketAndCreateListenerFilter) { // FIX-ME: replace by mock socket Network::Address::InstanceConstSharedPtr original_dst_address( new Network::Address::Ipv4Instance("127.0.0.2", 8080)); - Network::MockConnectionSocket* accepted_socket = new NiceMock(); EXPECT_CALL(filter_chain_factory_, createListenerFilterChain(_)) @@ -109,6 +108,33 @@ TEST_F(ActiveInternalListenerTest, AcceptSocketAndCreateListenerFilter) { EXPECT_CALL(*generic_listener_, onDestroy()); } +TEST_F(ActiveInternalListenerTest, DestroyListenerClosesActiveSocket) { + addListener(); + expectFilterChainFactory(); + Network::MockListenerFilter* test_listener_filter = new Network::MockListenerFilter(); + Network::MockConnectionSocket* accepted_socket = new NiceMock(); + NiceMock io_handle; + EXPECT_CALL(*accepted_socket, ioHandle()).WillOnce(ReturnRef(io_handle)); + EXPECT_CALL(io_handle, isOpen()).WillOnce(Return(true)); + + EXPECT_CALL(filter_chain_factory_, createListenerFilterChain(_)) + .WillRepeatedly(Invoke([&](Network::ListenerFilterManager& manager) -> bool { + manager.addAcceptFilter(listener_filter_matcher_, + Network::ListenerFilterPtr{test_listener_filter}); + return true; + })); + EXPECT_CALL(*test_listener_filter, onAccept(_)) + .WillOnce(Invoke([&](Network::ListenerFilterCallbacks&) -> Network::FilterStatus { + return Network::FilterStatus::StopIteration; + })); + + internal_listener_->onAccept(Network::ConnectionSocketPtr{accepted_socket}); + + EXPECT_CALL(*test_listener_filter, destroy_()); + EXPECT_CALL(*generic_listener_, onDestroy()); + internal_listener_.reset(); +} + TEST_F(ActiveInternalListenerTest, AcceptSocketAndCreateNetworkFilter) { addListener(); expectFilterChainFactory(); From 84300f33d66b20a6e7314d78972bec142234a0cf Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Wed, 6 Oct 2021 21:43:36 +0000 Subject: [PATCH 17/31] clear duplicated runtime value Signed-off-by: Yuchen Dai --- test/integration/socket_interface_integration_test.cc | 2 -- test/server/listener_manager_impl_test.cc | 3 --- 2 files changed, 5 deletions(-) diff --git a/test/integration/socket_interface_integration_test.cc b/test/integration/socket_interface_integration_test.cc index d4f78da877888..ea08b15cf91fc 100644 --- a/test/integration/socket_interface_integration_test.cc +++ b/test/integration/socket_interface_integration_test.cc @@ -100,8 +100,6 @@ TEST_P(SocketInterfaceIntegrationTest, InternalAddressWithSocketInterface) { Network::Address::InstanceConstSharedPtr address = std::make_shared("listener_0", sock_interface); - Runtime::LoaderSingleton::getExisting()->mergeValues( - {{"envoy.reloadable_features.internal_address", "false"}}); ASSERT_DEATH(client_ = dispatcher_->createClientConnection( address, Network::Address::InstanceConstSharedPtr(), Network::Test::createRawBufferSocket(), nullptr), diff --git a/test/server/listener_manager_impl_test.cc b/test/server/listener_manager_impl_test.cc index b5d6701ceeb43..fb058cdd29d51 100644 --- a/test/server/listener_manager_impl_test.cc +++ b/test/server/listener_manager_impl_test.cc @@ -535,9 +535,6 @@ bind_to_port: false TEST_F(ListenerManagerImplTest, UnsupportedInternalListener) { auto scoped_runtime_guard = std::make_unique(); - - Runtime::LoaderSingleton::getExisting()->mergeValues( - {{"envoy.reloadable_features.internal_address", "false"}}); const std::string yaml = R"EOF( address: envoy_internal_address: From 1b7d26ba4bebb3d7de0092706a90784b20e42646 Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Wed, 6 Oct 2021 22:57:13 +0000 Subject: [PATCH 18/31] sanitize dep Signed-off-by: Yuchen Dai --- test/integration/BUILD | 2 -- 1 file changed, 2 deletions(-) diff --git a/test/integration/BUILD b/test/integration/BUILD index e2bf9b557abdf..834d6771174b1 100644 --- a/test/integration/BUILD +++ b/test/integration/BUILD @@ -1661,10 +1661,8 @@ envoy_cc_test( "//source/common/event:dispatcher_lib", "//source/common/network:connection_lib", "//source/common/network:utility_lib", - "//source/extensions/filters/http/health_check:config", "//source/extensions/filters/network/tcp_proxy:config", "//test/common/grpc:grpc_client_integration_lib", - "//test/config:v2_link_hacks", "//test/integration/filters:address_restore_listener_filter_lib", "//test/test_common:network_utility_lib", "//test/test_common:resources_lib", From c58e7d159a6723027197ea232f495a0c156b15a7 Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Thu, 7 Oct 2021 06:08:08 +0000 Subject: [PATCH 19/31] few dep Signed-off-by: Yuchen Dai --- test/integration/BUILD | 6 +++--- test/integration/internal_listener_integration_test.cc | 7 +------ 2 files changed, 4 insertions(+), 9 deletions(-) diff --git a/test/integration/BUILD b/test/integration/BUILD index 834d6771174b1..8785afc353cbf 100644 --- a/test/integration/BUILD +++ b/test/integration/BUILD @@ -1655,13 +1655,15 @@ envoy_cc_test( "internal_listener_integration_test.cc", ], deps = [ - ":http_integration_lib", + ":base_integration_test_lib", "//source/common/config:api_version_lib", "//source/common/event:dispatcher_includes", "//source/common/event:dispatcher_lib", "//source/common/network:connection_lib", "//source/common/network:utility_lib", + "//source/extensions/access_loggers/file:config", "//source/extensions/filters/network/tcp_proxy:config", + "//source/extensions/transport_sockets/raw_buffer:config", "//test/common/grpc:grpc_client_integration_lib", "//test/integration/filters:address_restore_listener_filter_lib", "//test/test_common:network_utility_lib", @@ -1669,8 +1671,6 @@ envoy_cc_test( "//test/test_common:utility_lib", "@envoy_api//envoy/config/bootstrap/v3:pkg_cc_proto", "@envoy_api//envoy/config/core/v3:pkg_cc_proto", - "@envoy_api//envoy/config/route/v3:pkg_cc_proto", - "@envoy_api//envoy/extensions/filters/network/http_connection_manager/v3:pkg_cc_proto", "@envoy_api//envoy/extensions/filters/network/tcp_proxy/v3:pkg_cc_proto", "@envoy_api//envoy/service/discovery/v3:pkg_cc_proto", ], diff --git a/test/integration/internal_listener_integration_test.cc b/test/integration/internal_listener_integration_test.cc index 20105992f6ddb..9247067d4f07f 100644 --- a/test/integration/internal_listener_integration_test.cc +++ b/test/integration/internal_listener_integration_test.cc @@ -1,9 +1,6 @@ #include "envoy/config/bootstrap/v3/bootstrap.pb.h" #include "envoy/config/core/v3/config_source.pb.h" #include "envoy/config/core/v3/grpc_service.pb.h" -#include "envoy/config/route/v3/route.pb.h" -#include "envoy/config/route/v3/scoped_route.pb.h" -#include "envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.pb.h" #include "envoy/extensions/filters/network/tcp_proxy/v3/tcp_proxy.pb.h" #include "envoy/network/connection.h" #include "envoy/service/discovery/v3/discovery.pb.h" @@ -11,10 +8,8 @@ #include "source/common/config/api_version.h" #include "test/common/grpc/grpc_client_integration.h" -#include "test/config/v2_link_hacks.h" -#include "test/integration/http_integration.h" +#include "test/integration/base_integration_test.h" #include "test/test_common/network_utility.h" -#include "test/test_common/printers.h" #include "test/test_common/resources.h" #include "absl/strings/str_cat.h" From f8379dedb31de2a454b4c6ce996bbd186128a5a6 Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Wed, 27 Oct 2021 06:27:28 +0000 Subject: [PATCH 20/31] improve coverage Signed-off-by: Yuchen Dai --- source/server/listener_manager_impl.cc | 7 +++---- test/server/listener_manager_impl_test.cc | 2 +- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/source/server/listener_manager_impl.cc b/source/server/listener_manager_impl.cc index 2c5524bae825d..835781d164fa0 100644 --- a/source/server/listener_manager_impl.cc +++ b/source/server/listener_manager_impl.cc @@ -190,10 +190,9 @@ Network::SocketSharedPtr ProdListenerComponentFactory::createListenSocket( } return std::make_shared(address); } else if (address->type() == Network::Address::Type::EnvoyInternal) { - if (socket_type != Network::Socket::Type::Stream) { - throw EnvoyException(fmt::format("socket type {} not supported for EnvoyInternalAddress", - toString(socket_type))); - } + // Listener manager should have validated that envoy internal address doesn't work with udp + // listener yet. + ASSERT(socket_type != Network::Socket::Type::Stream); return std::make_shared(address); } diff --git a/test/server/listener_manager_impl_test.cc b/test/server/listener_manager_impl_test.cc index b5d6701ceeb43..49cd1a78067ca 100644 --- a/test/server/listener_manager_impl_test.cc +++ b/test/server/listener_manager_impl_test.cc @@ -546,7 +546,7 @@ TEST_F(ListenerManagerImplTest, UnsupportedInternalListener) { - filters: [] )EOF"; - ASSERT_DEATH(manager_->addOrUpdateListener(parseListenerFromV3Yaml(yaml), "", true), ""); + EXPECT_DEATH(manager_->addOrUpdateListener(parseListenerFromV3Yaml(yaml), "", true), ""); } TEST_F(ListenerManagerImplTest, NotDefaultListenerFiltersTimeout) { From f6c8fd8dbc2fb2725637b15051a4607112a1ed6e Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Wed, 27 Oct 2021 06:36:30 +0000 Subject: [PATCH 21/31] merge fix Signed-off-by: Yuchen Dai --- docs/root/version_history/current.rst | 16 ---------------- source/common/network/listen_socket_impl.h | 7 ------- source/server/listener_manager_impl.cc | 2 +- test/integration/BUILD | 1 - 4 files changed, 1 insertion(+), 25 deletions(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index ee8c1f9e26586..6b6d65be13c75 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -20,22 +20,6 @@ Bug Fixes --------- *Changes expected to improve the state of the world and are unlikely to have negative effects* -* access log: fix ``%UPSTREAM_CLUSTER%`` when used in http upstream access logs. Previously, it was always logging as an unset value. -* aws request signer: fix the AWS Request Signer extension to correctly normalize the path and query string to be signed according to AWS' guidelines, so that the hash on the server side matches. See `AWS SigV4 documentation `_. -* cluster: delete pools when they're idle to fix unbounded memory use when using PROXY protocol upstream with tcp_proxy. This behavior can be temporarily reverted by setting the ``envoy.reloadable_features.conn_pool_delete_when_idle`` runtime guard to false. -* cluster: finish cluster warming even if hosts are removed before health check initialization. This only affected clusters with :ref:`ignore_health_on_host_removal `. -* compressor: fix a bug where if trailers were added and a subsequent filter paused the filter chain, the request could be stalled. This behavior can be reverted by setting ``envoy.reloadable_features.fix_added_trailers`` to false. -* dynamic forward proxy: fixing a validation bug where san and sni checks were not applied setting :ref:`http_protocol_options ` via :ref:`typed_extension_protocol_options `. -* ext_authz: fix the ext_authz filter to correctly merge multiple same headers using the ',' as separator in the check request to the external authorization service. -* ext_authz: fix the use of ``append`` field of :ref:`response_headers_to_add ` to set or append encoded response headers from a gRPC auth server. -* ext_authz: fix the HTTP ext_authz filter to respond with ``403 Forbidden`` when a gRPC auth server sends a denied check response with an empty HTTP status code. -* ext_authz: the network ext_authz filter now correctly sets dynamic metadata returned by the authorization service for non-OK responses. This behavior now matches the http ext_authz filter. -* hcm: remove deprecation for :ref:`xff_num_trusted_hops ` and forbid mixing ip detection extensions with old related knobs. -* http: limit use of deferred resets in the http2 codec to server-side connections. Use of deferred reset for client connections can result in incorrect behavior and performance problems. -* listener: fixed an issue on Windows where connections are not handled by all worker threads. -* lua: fix ``BodyBuffer`` setting a Lua string and printing Lua string containing hex characters. Previously, ``BodyBuffer`` setting a Lua string or printing strings with hex characters will be truncated. -* xray: fix the AWS X-Ray tracer bug where span's error, fault and throttle information was not reported properly as per the `AWS X-Ray documentation `_. Before this fix, server error was reported under the 'annotations' section of the segment data. -* listener: fixed the crash when updating listeners that do not bind to port. * listener: fixed the crash when updating listeners that do not bind to port. * thrift_proxy: fix the thrift_proxy connection manager to correctly report success/error response metrics when performing :ref:`payload passthrough `. diff --git a/source/common/network/listen_socket_impl.h b/source/common/network/listen_socket_impl.h index 8f37759532476..cf6b4ecd06b92 100644 --- a/source/common/network/listen_socket_impl.h +++ b/source/common/network/listen_socket_impl.h @@ -17,13 +17,6 @@ namespace Envoy { namespace Network { -namespace { -std::string getConnectionInfoString(const ConnectionInfoProvider& conn_info) { - std::stringstream out; - conn_info.dumpState(out, 0); - return out.str(); -} -} // namespace class ListenSocketImpl : public SocketImpl { protected: diff --git a/source/server/listener_manager_impl.cc b/source/server/listener_manager_impl.cc index 3d5df42ccb10c..3dbd8a6d476fe 100644 --- a/source/server/listener_manager_impl.cc +++ b/source/server/listener_manager_impl.cc @@ -192,7 +192,7 @@ Network::SocketSharedPtr ProdListenerComponentFactory::createListenSocket( } else if (address->type() == Network::Address::Type::EnvoyInternal) { // Listener manager should have validated that envoy internal address doesn't work with udp // listener yet. - ASSERT(socket_type != Network::Socket::Type::Stream); + ASSERT(socket_type == Network::Socket::Type::Stream); return std::make_shared(address); } diff --git a/test/integration/BUILD b/test/integration/BUILD index 8f48a1c6ab9d7..011616e173ea1 100644 --- a/test/integration/BUILD +++ b/test/integration/BUILD @@ -1675,7 +1675,6 @@ envoy_cc_test( "//source/common/event:dispatcher_lib", "//source/common/network:connection_lib", "//source/common/network:utility_lib", - "//source/extensions/filters/http/health_check:config", "//source/extensions/filters/network/tcp_proxy:config", "//test/common/grpc:grpc_client_integration_lib", "//test/config:v2_link_hacks", From a9d5891cb69f245c451fd749c982ad52300a8f5e Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Wed, 27 Oct 2021 07:25:09 +0000 Subject: [PATCH 22/31] fix format Signed-off-by: Yuchen Dai --- test/integration/internal_listener_integration_test.cc | 2 -- 1 file changed, 2 deletions(-) diff --git a/test/integration/internal_listener_integration_test.cc b/test/integration/internal_listener_integration_test.cc index 60a84edc47233..9247067d4f07f 100644 --- a/test/integration/internal_listener_integration_test.cc +++ b/test/integration/internal_listener_integration_test.cc @@ -1,7 +1,6 @@ #include "envoy/config/bootstrap/v3/bootstrap.pb.h" #include "envoy/config/core/v3/config_source.pb.h" #include "envoy/config/core/v3/grpc_service.pb.h" - #include "envoy/extensions/filters/network/tcp_proxy/v3/tcp_proxy.pb.h" #include "envoy/network/connection.h" #include "envoy/service/discovery/v3/discovery.pb.h" @@ -9,7 +8,6 @@ #include "source/common/config/api_version.h" #include "test/common/grpc/grpc_client_integration.h" - #include "test/integration/base_integration_test.h" #include "test/test_common/network_utility.h" #include "test/test_common/resources.h" From d5654056cf74ffaed5eefc8e2ed92f2dfc95d9c2 Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Wed, 27 Oct 2021 19:03:04 +0000 Subject: [PATCH 23/31] another try windows release assert Signed-off-by: Yuchen Dai --- test/server/listener_manager_impl_test.cc | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/test/server/listener_manager_impl_test.cc b/test/server/listener_manager_impl_test.cc index 0065655fba69d..32580689fcb92 100644 --- a/test/server/listener_manager_impl_test.cc +++ b/test/server/listener_manager_impl_test.cc @@ -518,16 +518,16 @@ bind_to_port: false } TEST_F(ListenerManagerImplTest, UnsupportedInternalListener) { - auto scoped_runtime_guard = std::make_unique(); const std::string yaml = R"EOF( -address: - envoy_internal_address: - server_listener_name: a_listener_name -filter_chains: -- filters: [] + name: "foo" + address: + envoy_internal_address: + server_listener_name: a_listener_name + filter_chains: + - filters: [] )EOF"; - EXPECT_DEATH(manager_->addOrUpdateListener(parseListenerFromV3Yaml(yaml), "", true), ""); + EXPECT_DEATH(manager_->addOrUpdateListener(parseListenerFromV3Yaml(yaml), "", true), ".*"); } TEST_F(ListenerManagerImplTest, NotDefaultListenerFiltersTimeout) { From 1605a0df1f673a429366cdab0ec7741e8b32ed9e Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Thu, 28 Oct 2021 06:39:03 +0000 Subject: [PATCH 24/31] testcov Signed-off-by: Yuchen Dai --- test/common/network/listen_socket_impl_test.cc | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/test/common/network/listen_socket_impl_test.cc b/test/common/network/listen_socket_impl_test.cc index 4ca83f9604494..8ee72900ca8b0 100644 --- a/test/common/network/listen_socket_impl_test.cc +++ b/test/common/network/listen_socket_impl_test.cc @@ -24,15 +24,25 @@ namespace Envoy { namespace Network { namespace { -TEST(ConnectionSocketImplTest, LowerCaseRequestedServerName) { +class ConnectionSocketImplTest : public testing::TestWithParam {}; + +INSTANTIATE_TEST_SUITE_P(IpVersions, ConnectionSocketImplTest, + testing::ValuesIn(TestEnvironment::getIpVersionsForTest())); + +TEST_P(ConnectionSocketImplTest, LowerCaseRequestedServerName) { absl::string_view serverName("www.EXAMPLE.com"); absl::string_view expectedServerName("www.example.com"); - auto loopback_addr = Network::Test::getCanonicalLoopbackAddress(Address::IpVersion::v4); + auto loopback_addr = Network::Test::getCanonicalLoopbackAddress(GetParam()); auto conn_socket_ = ConnectionSocketImpl(Socket::Type::Stream, loopback_addr, loopback_addr); conn_socket_.setRequestedServerName(serverName); EXPECT_EQ(expectedServerName, conn_socket_.requestedServerName()); } +TEST_P(ConnectionSocketImplTest, IpVersion) { + ClientSocketImpl socket(Network::Test::getCanonicalLoopbackAddress(GetParam()), nullptr); + EXPECT_EQ(socket.ipVersion(), GetParam()); +} + template class ListenSocketImplTest : public testing::TestWithParam { using ListenSocketType = NetworkListenSocket>; From e7f0e50bfb3df2e94d84b6f1d911e7617b0fa6d9 Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Thu, 28 Oct 2021 06:42:28 +0000 Subject: [PATCH 25/31] another death at windows try Signed-off-by: Yuchen Dai --- test/server/listener_manager_impl_test.cc | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/test/server/listener_manager_impl_test.cc b/test/server/listener_manager_impl_test.cc index 32580689fcb92..7fcb497f765bc 100644 --- a/test/server/listener_manager_impl_test.cc +++ b/test/server/listener_manager_impl_test.cc @@ -518,6 +518,11 @@ bind_to_port: false } TEST_F(ListenerManagerImplTest, UnsupportedInternalListener) { + auto scoped_runtime_guard = std::make_unique(); + // Workaround of triggering death at windows platform. + Runtime::LoaderSingleton::getExisting()->mergeValues( + {{"envoy.reloadable_features.internal_address", "false"}}); + const std::string yaml = R"EOF( name: "foo" address: From c4b74e8c9e3f53889f4dd187b054614f4acad96b Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Mon, 1 Nov 2021 22:52:15 +0000 Subject: [PATCH 26/31] ASSERT to ASSERT_TRUE in test Signed-off-by: Yuchen Dai --- envoy/network/listener.h | 2 +- source/server/active_internal_listener.h | 5 +++++ test/server/connection_handler_test.cc | 6 +++--- 3 files changed, 9 insertions(+), 4 deletions(-) diff --git a/envoy/network/listener.h b/envoy/network/listener.h index 8250b5bfcffec..1e2fd37f45a9d 100644 --- a/envoy/network/listener.h +++ b/envoy/network/listener.h @@ -472,7 +472,7 @@ class InternalListenerManager { virtual ~InternalListenerManager() = default; /** - * Return the internal listener binding the listener address. + * Return the internal listener callbacks binding the listener address. * * @param listen_address the internal address of the expected internal listener. */ diff --git a/source/server/active_internal_listener.h b/source/server/active_internal_listener.h index 59836b8cfed79..d87c3592f2ea1 100644 --- a/source/server/active_internal_listener.h +++ b/source/server/active_internal_listener.h @@ -52,10 +52,14 @@ class ActiveInternalListener : public OwnedActiveStreamListenerBase, void setRejectFraction(UnitFloat) override {} }; + // ActiveListenerImplBase Network::Listener* listener() override { return listener_.get(); } + // Network::TcpConnectionHandler Network::BalancedConnectionHandlerOptRef getBalancedHandlerByAddress(const Network::Address::Instance&) override { + // Internal listener doesn't support migrate connection to another worker. + // TODO(lambdai): implement the function of handling off to another listener of the same worker. NOT_IMPLEMENTED_GCOVR_EXCL_LINE; } @@ -74,6 +78,7 @@ class ActiveInternalListener : public OwnedActiveStreamListenerBase, // Network::InternalListenerCallbacks void onAccept(Network::ConnectionSocketPtr&& socket) override; + // Network::BalancedConnectionHandler void incNumConnections() override { config_->openConnections().inc(); } void decNumConnections() override { config_->openConnections().dec(); } diff --git a/test/server/connection_handler_test.cc b/test/server/connection_handler_test.cc index 5d0f0715170cf..785e90790aae3 100644 --- a/test/server/connection_handler_test.cc +++ b/test/server/connection_handler_test.cc @@ -1586,16 +1586,16 @@ TEST_F(ConnectionHandlerTest, DisableInternalListener) { .WillOnce(ReturnRef(local_address)); handler_->addListener(absl::nullopt, *internal_listener); auto internal_listener_cb = handler_->findByAddress(local_address); - ASSERT(internal_listener_cb.has_value()); + ASSERT_TRUE(internal_listener_cb.has_value()); handler_->disableListeners(); auto internal_listener_cb_disabled = handler_->findByAddress(local_address); - ASSERT(internal_listener_cb_disabled.has_value()); + ASSERT_TURE(internal_listener_cb_disabled.has_value()); ASSERT_EQ(&internal_listener_cb_disabled.value().get(), &internal_listener_cb.value().get()); handler_->enableListeners(); auto internal_listener_cb_enabled = handler_->findByAddress(local_address); - ASSERT(internal_listener_cb_enabled.has_value()); + ASSERT_TRUE(internal_listener_cb_enabled.has_value()); ASSERT_EQ(&internal_listener_cb_enabled.value().get(), &internal_listener_cb.value().get()); } From 41401f4b4709d0c35f45c78ce98bd1e248373447 Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Tue, 2 Nov 2021 06:35:44 +0000 Subject: [PATCH 27/31] typo Signed-off-by: Yuchen Dai --- examples/tls-inspector/envoy.yaml | 56 ++++++++++++-------------- test/server/connection_handler_test.cc | 2 +- 2 files changed, 26 insertions(+), 32 deletions(-) diff --git a/examples/tls-inspector/envoy.yaml b/examples/tls-inspector/envoy.yaml index 75c5c52572ab4..84e71e981cb8c 100644 --- a/examples/tls-inspector/envoy.yaml +++ b/examples/tls-inspector/envoy.yaml @@ -5,40 +5,34 @@ admin: port_value: 12345 static_resources: listeners: - - address: + - name: listener_inbound + address: socket_address: address: 0.0.0.0 - port_value: 10000 - listener_filters: - - name: "envoy.filters.listener.tls_inspector" - typed_config: - "@type": type.googleapis.com/envoy.extensions.filters.listener.tls_inspector.v3.TlsInspector + port_value: 16000 + traffic_direction: INBOUND filter_chains: - - filter_chain_match: - transport_protocol: tls - application_protocols: [h2] - filters: - - name: envoy.filters.network.tcp_proxy - typed_config: - "@type": type.googleapis.com/envoy.extensions.filters.network.tcp_proxy.v3.TcpProxy - cluster: service-https-http2 - stat_prefix: https_passthrough - - filter_chain_match: - transport_protocol: tls - application_protocols: [http/1.1] - filters: - - name: envoy.filters.network.tcp_proxy - typed_config: - "@type": type.googleapis.com/envoy.extensions.filters.network.tcp_proxy.v3.TcpProxy - cluster: service-https-http1.1 - stat_prefix: https_passthrough - - filter_chain_match: - filters: - - name: envoy.filters.network.tcp_proxy - typed_config: - "@type": type.googleapis.com/envoy.extensions.filters.network.tcp_proxy.v3.TcpProxy - cluster: service-http - stat_prefix: ingress_http + - filters: + - name: envoy.filters.network.http_connection_manager + typed_config: + "@type": type.googleapis.com/envoy.extensions.filters.network.http_connection_manager.v3.HttpConnectionManager + codec_type: AUTO + stat_prefix: ingress_http + route_config: + name: ingress + virtual_hosts: + - name: ingress + domains: + - "*" + routes: + - match: + prefix: "/" + direct_response: + status: 503 + body: + inline_string: "{'message': 'FAILURE INJECTOIN!'}" + http_filters: + - name: envoy.filters.http.router clusters: - name: service-https-http2 diff --git a/test/server/connection_handler_test.cc b/test/server/connection_handler_test.cc index 785e90790aae3..74a5ca332db0b 100644 --- a/test/server/connection_handler_test.cc +++ b/test/server/connection_handler_test.cc @@ -1590,7 +1590,7 @@ TEST_F(ConnectionHandlerTest, DisableInternalListener) { handler_->disableListeners(); auto internal_listener_cb_disabled = handler_->findByAddress(local_address); - ASSERT_TURE(internal_listener_cb_disabled.has_value()); + ASSERT_TRUE(internal_listener_cb_disabled.has_value()); ASSERT_EQ(&internal_listener_cb_disabled.value().get(), &internal_listener_cb.value().get()); handler_->enableListeners(); From 178b70e2e129b0e5205fc92c0b285d8e1d8fc58f Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Tue, 2 Nov 2021 08:18:57 +0000 Subject: [PATCH 28/31] revert a test config Signed-off-by: Yuchen Dai --- examples/tls-inspector/envoy.yaml | 56 +++++++++++++++++-------------- 1 file changed, 31 insertions(+), 25 deletions(-) diff --git a/examples/tls-inspector/envoy.yaml b/examples/tls-inspector/envoy.yaml index 84e71e981cb8c..75c5c52572ab4 100644 --- a/examples/tls-inspector/envoy.yaml +++ b/examples/tls-inspector/envoy.yaml @@ -5,34 +5,40 @@ admin: port_value: 12345 static_resources: listeners: - - name: listener_inbound - address: + - address: socket_address: address: 0.0.0.0 - port_value: 16000 - traffic_direction: INBOUND + port_value: 10000 + listener_filters: + - name: "envoy.filters.listener.tls_inspector" + typed_config: + "@type": type.googleapis.com/envoy.extensions.filters.listener.tls_inspector.v3.TlsInspector filter_chains: - - filters: - - name: envoy.filters.network.http_connection_manager - typed_config: - "@type": type.googleapis.com/envoy.extensions.filters.network.http_connection_manager.v3.HttpConnectionManager - codec_type: AUTO - stat_prefix: ingress_http - route_config: - name: ingress - virtual_hosts: - - name: ingress - domains: - - "*" - routes: - - match: - prefix: "/" - direct_response: - status: 503 - body: - inline_string: "{'message': 'FAILURE INJECTOIN!'}" - http_filters: - - name: envoy.filters.http.router + - filter_chain_match: + transport_protocol: tls + application_protocols: [h2] + filters: + - name: envoy.filters.network.tcp_proxy + typed_config: + "@type": type.googleapis.com/envoy.extensions.filters.network.tcp_proxy.v3.TcpProxy + cluster: service-https-http2 + stat_prefix: https_passthrough + - filter_chain_match: + transport_protocol: tls + application_protocols: [http/1.1] + filters: + - name: envoy.filters.network.tcp_proxy + typed_config: + "@type": type.googleapis.com/envoy.extensions.filters.network.tcp_proxy.v3.TcpProxy + cluster: service-https-http1.1 + stat_prefix: https_passthrough + - filter_chain_match: + filters: + - name: envoy.filters.network.tcp_proxy + typed_config: + "@type": type.googleapis.com/envoy.extensions.filters.network.tcp_proxy.v3.TcpProxy + cluster: service-http + stat_prefix: ingress_http clusters: - name: service-https-http2 From 3f6407b5bdd5eed9d9ab94653668329da701b20d Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Tue, 9 Nov 2021 05:26:09 +0000 Subject: [PATCH 29/31] address comment Signed-off-by: Yuchen Dai --- envoy/network/listener.h | 5 ++- source/common/tcp_proxy/tcp_proxy.cc | 2 +- source/server/active_internal_listener.cc | 2 +- source/server/connection_handler_impl.cc | 2 +- source/server/listener_impl.cc | 8 ++++- .../internal_listener_integration_test.cc | 13 ++------ test/server/listener_manager_impl_test.cc | 31 ++++++++++++++----- 7 files changed, 37 insertions(+), 26 deletions(-) diff --git a/envoy/network/listener.h b/envoy/network/listener.h index 1e2fd37f45a9d..d468a278979e0 100644 --- a/envoy/network/listener.h +++ b/envoy/network/listener.h @@ -456,13 +456,12 @@ class InternalListenerCallbacks { */ virtual void onAccept(ConnectionSocketPtr&& socket) PURE; }; -using InternalListenerCallbacksOptRef = - absl::optional>; +using InternalListenerCallbacksOptRef = OptRef; class InternalListener {}; using InternalListenerPtr = std::unique_ptr; -using InternalListenerOptRef = absl::optional>; +using InternalListenerOptRef = OptRef; /** * The query interface of the registered internal listener callbacks. diff --git a/source/common/tcp_proxy/tcp_proxy.cc b/source/common/tcp_proxy/tcp_proxy.cc index 67759aeaa03fe..4dd925c34cdc0 100644 --- a/source/common/tcp_proxy/tcp_proxy.cc +++ b/source/common/tcp_proxy/tcp_proxy.cc @@ -701,7 +701,7 @@ Drainer::Drainer(UpstreamDrainManager& parent, const Config::SharedConfigSharedP const Upstream::HostDescriptionConstSharedPtr& upstream_host) : parent_(parent), callbacks_(callbacks), upstream_conn_data_(std::move(conn_data)), timer_(std::move(idle_timer)), upstream_host_(upstream_host), config_(config) { - ENVOY_CONN_LOG(debug, "draining the upstream connection", upstream_conn_data_->connection()); + ENVOY_CONN_LOG(trace, "draining the upstream connection", upstream_conn_data_->connection()); config_->stats().upstream_flush_total_.inc(); config_->stats().upstream_flush_active_.inc(); } diff --git a/source/server/active_internal_listener.cc b/source/server/active_internal_listener.cc index 45d051de19b74..701d9376175e7 100644 --- a/source/server/active_internal_listener.cc +++ b/source/server/active_internal_listener.cc @@ -54,7 +54,7 @@ void ActiveInternalListener::onAccept(Network::ConnectionSocketPtr&& socket) { incNumConnections(); auto active_socket = std::make_unique( - *this, std::move(socket), false /* do not handle off at internal listener */); + *this, std::move(socket), false /* do not hand off at internal listener */); onSocketAccepted(std::move(active_socket)); } diff --git a/source/server/connection_handler_impl.cc b/source/server/connection_handler_impl.cc index faf349d117136..768b24cdb105c 100644 --- a/source/server/connection_handler_impl.cc +++ b/source/server/connection_handler_impl.cc @@ -175,7 +175,7 @@ ConnectionHandlerImpl::findByAddress(const Network::Address::InstanceConstShared return Network::InternalListenerCallbacksOptRef( listener_it->second.internalListener().value().get()); } - return absl::nullopt; + return OptRef(); } ConnectionHandlerImpl::ActiveTcpListenerOptRef diff --git a/source/server/listener_impl.cc b/source/server/listener_impl.cc index 9ecce2dbac709..64295eed0e2d9 100644 --- a/source/server/listener_impl.cc +++ b/source/server/listener_impl.cc @@ -465,8 +465,14 @@ void ListenerImpl::buildAccessLog() { } void ListenerImpl::buildInternalListener() { - if (config_.has_internal_listener()) { + if (config_.address().has_envoy_internal_address()) { internal_listener_config_ = std::make_unique(); + } else { + if (config_.has_internal_listener()) { + throw EnvoyException(fmt::format("error adding listener '{}': address is not an internal " + "address but an internal listener config is provided", + address_->asString())); + } } } diff --git a/test/integration/internal_listener_integration_test.cc b/test/integration/internal_listener_integration_test.cc index 9247067d4f07f..11614b84874a9 100644 --- a/test/integration/internal_listener_integration_test.cc +++ b/test/integration/internal_listener_integration_test.cc @@ -23,15 +23,7 @@ class InternalListenerIntegrationTest : public testing::TestWithParammutable_listeners(0); listener.mutable_address()->mutable_envoy_internal_address()->set_server_listener_name( "internal_listener"); - listener.mutable_internal_listener(); }); BaseIntegrationTest::initialize(); } }; -TEST_P(InternalListenerIntegrationTest, Basic) { +TEST_P(InternalListenerIntegrationTest, BasicConfigUpdate) { initialize(); EXPECT_EQ(1, test_server_->counter("listener_manager.lds.update_success")->value()); diff --git a/test/server/listener_manager_impl_test.cc b/test/server/listener_manager_impl_test.cc index 87d65a789e3ee..6c61ac9b49fe5 100644 --- a/test/server/listener_manager_impl_test.cc +++ b/test/server/listener_manager_impl_test.cc @@ -536,6 +536,28 @@ TEST_F(ListenerManagerImplTest, UnsupportedInternalListener) { EXPECT_DEATH(manager_->addOrUpdateListener(parseListenerFromV3Yaml(yaml), "", true), ".*"); } +TEST_F(ListenerManagerImplTest, RejectListenerWithSocketAddressWithInternalListenerConfig) { + auto scoped_runtime_guard = std::make_unique(); + Runtime::LoaderSingleton::getExisting()->mergeValues( + {{"envoy.reloadable_features.internal_address", "true"}}); + + const std::string yaml = R"EOF( + name: "foo" + address: + socket_address: + address: 127.0.0.1 + port_value: 1234 + internal_listener: {} + filter_chains: + - filters: [] + )EOF"; + + EXPECT_THROW_WITH_MESSAGE(manager_->addOrUpdateListener(parseListenerFromV3Yaml(yaml), "", true), + EnvoyException, + "error adding listener '127.0.0.1:1234': address is not an internal " + "address but an internal listener config is provided"); +} + TEST_F(ListenerManagerImplTest, NotDefaultListenerFiltersTimeout) { const std::string yaml = R"EOF( name: "foo" @@ -4792,13 +4814,12 @@ TEST_F(ListenerManagerImplWithRealFiltersTest, AddOrUpdateInternalListener) { static_listeners: )EOF"); - // Add foo listener. + // Add foo listener. The internal listener does not need explicit internal listener field. const std::string listener_foo_yaml = R"EOF( name: test_internal_listener address: envoy_internal_address: server_listener_name: test_internal_listener_name -internal_listener: {} filter_chains: {} )EOF"; @@ -4821,7 +4842,6 @@ version_info: version1 envoy_internal_address: server_listener_name: test_internal_listener_name filter_chains: {} - internal_listener: {} last_updated: seconds: 1001001001 nanos: 1000000 @@ -4838,7 +4858,6 @@ name: test_internal_listener envoy_internal_address: server_listener_name: test_internal_listener_name filter_chains: {} -internal_listener: {} per_connection_buffer_limit_bytes: 10 )EOF"; @@ -4864,7 +4883,6 @@ per_connection_buffer_limit_bytes: 10 envoy_internal_address: server_listener_name: test_internal_listener_name filter_chains: {} - internal_listener: {} per_connection_buffer_limit_bytes: 10 last_updated: seconds: 2002002002 @@ -4925,7 +4943,6 @@ per_connection_buffer_limit_bytes: 10 envoy_internal_address: server_listener_name: test_internal_listener_name filter_chains: {} - internal_listener: {} last_updated: seconds: 3003003003 nanos: 3000000 @@ -4938,7 +4955,6 @@ per_connection_buffer_limit_bytes: 10 envoy_internal_address: server_listener_name: test_internal_listener_name filter_chains: {} - internal_listener: {} per_connection_buffer_limit_bytes: 10 last_updated: seconds: 2002002002 @@ -5004,7 +5020,6 @@ per_connection_buffer_limit_bytes: 10 envoy_internal_address: server_listener_name: test_internal_listener_name filter_chains: {} - internal_listener: {} last_updated: seconds: 3003003003 nanos: 3000000 From 18a4d07ef917546d7566aa4b4e4a4a1cf9df4410 Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Fri, 12 Nov 2021 21:06:13 +0000 Subject: [PATCH 30/31] add test case and address naming Signed-off-by: Yuchen Dai --- envoy/network/listener.h | 11 ++--- source/common/network/listen_socket_impl.h | 5 ++ source/server/active_internal_listener.h | 4 +- source/server/connection_handler_impl.cc | 8 ++-- source/server/connection_handler_impl.h | 2 +- source/server/listener_impl.cc | 12 +++++ test/common/network/connection_impl_test.cc | 3 +- test/server/active_internal_listener_test.cc | 2 +- test/server/listener_manager_impl_test.cc | 49 ++++++++++++++++++++ 9 files changed, 78 insertions(+), 18 deletions(-) diff --git a/envoy/network/listener.h b/envoy/network/listener.h index d468a278979e0..9969c4198fa93 100644 --- a/envoy/network/listener.h +++ b/envoy/network/listener.h @@ -446,9 +446,9 @@ using UdpListenerPtr = std::unique_ptr; /** * Internal listener callbacks. */ -class InternalListenerCallbacks { +class InternalListener { public: - virtual ~InternalListenerCallbacks() = default; + virtual ~InternalListener() = default; /** * Called when a new connection is accepted. @@ -456,11 +456,6 @@ class InternalListenerCallbacks { */ virtual void onAccept(ConnectionSocketPtr&& socket) PURE; }; -using InternalListenerCallbacksOptRef = OptRef; - -class InternalListener {}; - -using InternalListenerPtr = std::unique_ptr; using InternalListenerOptRef = OptRef; /** @@ -475,7 +470,7 @@ class InternalListenerManager { * * @param listen_address the internal address of the expected internal listener. */ - virtual InternalListenerCallbacksOptRef + virtual InternalListenerOptRef findByAddress(const Address::InstanceConstSharedPtr& listen_address) PURE; }; diff --git a/source/common/network/listen_socket_impl.h b/source/common/network/listen_socket_impl.h index 0e6852f5c4140..f48c04959b246 100644 --- a/source/common/network/listen_socket_impl.h +++ b/source/common/network/listen_socket_impl.h @@ -165,6 +165,11 @@ class InternalListenSocket : public ListenSocketImpl { return std::make_unique(connectionInfoProvider().localAddress()); } + Api::SysCallIntResult bind(Network::Address::InstanceConstSharedPtr) override { + // internal listener socket does not support bind semantic. + NOT_IMPLEMENTED_GCOVR_EXCL_LINE; + } + void close() override { ASSERT(io_handle_ == nullptr); } bool isOpen() const override { ASSERT(io_handle_ == nullptr); diff --git a/source/server/active_internal_listener.h b/source/server/active_internal_listener.h index d87c3592f2ea1..f455162a6c9c3 100644 --- a/source/server/active_internal_listener.h +++ b/source/server/active_internal_listener.h @@ -27,7 +27,7 @@ namespace Envoy { namespace Server { class ActiveInternalListener : public OwnedActiveStreamListenerBase, - public Network::InternalListenerCallbacks { + public Network::InternalListener { public: ActiveInternalListener(Network::ConnectionHandler& conn_handler, Event::Dispatcher& dispatcher, Network::ListenerConfig& config); @@ -75,7 +75,7 @@ class ActiveInternalListener : public OwnedActiveStreamListenerBase, } void shutdownListener() override { listener_.reset(); } - // Network::InternalListenerCallbacks + // Network::InternalListener void onAccept(Network::ConnectionSocketPtr&& socket) override; // Network::BalancedConnectionHandler diff --git a/source/server/connection_handler_impl.cc b/source/server/connection_handler_impl.cc index 768b24cdb105c..24789e5feb5aa 100644 --- a/source/server/connection_handler_impl.cc +++ b/source/server/connection_handler_impl.cc @@ -159,8 +159,9 @@ void ConnectionHandlerImpl::setListenerRejectFraction(UnitFloat reject_fraction) } } -Network::InternalListenerCallbacksOptRef +Network::InternalListenerOptRef ConnectionHandlerImpl::findByAddress(const Network::Address::InstanceConstSharedPtr& address) { + ASSERT(address->type() == Network::Address::Type::EnvoyInternal); auto listener_it = std::find_if(listeners_.begin(), listeners_.end(), [&address](std::pairsecond.internalListener().value().get()); + return Network::InternalListenerOptRef(listener_it->second.internalListener().value().get()); } - return OptRef(); + return OptRef(); } ConnectionHandlerImpl::ActiveTcpListenerOptRef diff --git a/source/server/connection_handler_impl.h b/source/server/connection_handler_impl.h index c58543a1d8fc4..23a39659c55d1 100644 --- a/source/server/connection_handler_impl.h +++ b/source/server/connection_handler_impl.h @@ -68,7 +68,7 @@ class ConnectionHandlerImpl : public Network::TcpConnectionHandler, Network::UdpListenerCallbacksOptRef getUdpListenerCallbacks(uint64_t listener_tag) override; // Network::InternalListenerManager - Network::InternalListenerCallbacksOptRef + Network::InternalListenerOptRef findByAddress(const Network::Address::InstanceConstSharedPtr& listen_address) override; private: diff --git a/source/server/listener_impl.cc b/source/server/listener_impl.cc index 64295eed0e2d9..13efe061d0ae9 100644 --- a/source/server/listener_impl.cc +++ b/source/server/listener_impl.cc @@ -467,6 +467,18 @@ void ListenerImpl::buildAccessLog() { void ListenerImpl::buildInternalListener() { if (config_.address().has_envoy_internal_address()) { internal_listener_config_ = std::make_unique(); + if (config_.has_connection_balance_config() || config_.enable_mptcp() || + config_.has_enable_reuse_port() || config_.has_freebind() || + config_.has_tcp_backlog_size() || config_.has_tcp_fast_open_queue_length() || + config_.has_transparent()) { + throw EnvoyException( + fmt::format("error adding listener '{}': has unsupported tcp listener feature", + address_->asString())); + } + if (!config_.socket_options().empty()) { + throw EnvoyException(fmt::format("error adding listener '{}': does not support socket option", + address_->asString())); + } } else { if (config_.has_internal_listener()) { throw EnvoyException(fmt::format("error adding listener '{}': address is not an internal " diff --git a/test/common/network/connection_impl_test.cc b/test/common/network/connection_impl_test.cc index 6b88828943a4c..af7d2786eddcb 100644 --- a/test/common/network/connection_impl_test.cc +++ b/test/common/network/connection_impl_test.cc @@ -58,8 +58,7 @@ namespace { class MockInternalListenerManager : public InternalListenerManager { public: - MOCK_METHOD(InternalListenerCallbacksOptRef, findByAddress, - (const Address::InstanceConstSharedPtr&), ()); + MOCK_METHOD(InternalListenerOptRef, findByAddress, (const Address::InstanceConstSharedPtr&), ()); }; TEST(RawBufferSocket, TestBasics) { diff --git a/test/server/active_internal_listener_test.cc b/test/server/active_internal_listener_test.cc index d065a4879c6d1..a2e60f3bc8c89 100644 --- a/test/server/active_internal_listener_test.cc +++ b/test/server/active_internal_listener_test.cc @@ -26,7 +26,7 @@ namespace Envoy { namespace Server { namespace { -class MockInternalListenerCallback : public Network::InternalListenerCallbacks { +class MockInternalListenerCallback : public Network::InternalListener { public: MOCK_METHOD(void, onAccept, (Network::ConnectionSocketPtr && socket), ()); MOCK_METHOD(Event::Dispatcher&, dispatcher, ()); diff --git a/test/server/listener_manager_impl_test.cc b/test/server/listener_manager_impl_test.cc index 6c61ac9b49fe5..078826a4ecf15 100644 --- a/test/server/listener_manager_impl_test.cc +++ b/test/server/listener_manager_impl_test.cc @@ -558,6 +558,55 @@ TEST_F(ListenerManagerImplTest, RejectListenerWithSocketAddressWithInternalListe "address but an internal listener config is provided"); } +TEST_F(ListenerManagerImplTest, RejectTcpOptionsWithInternalListenerConfig) { + auto scoped_runtime_guard = std::make_unique(); + Runtime::LoaderSingleton::getExisting()->mergeValues( + {{"envoy.reloadable_features.internal_address", "true"}}); + + const std::string yaml = R"EOF( + name: "foo" + address: + envoy_internal_address: + server_listener_name: test_internal_listener_name + filter_chains: + - filters: [] + )EOF"; + + auto listener = parseListenerFromV3Yaml(yaml); + auto listener_mutators = std::vector>{ + [](envoy::config::listener::v3::Listener& l) { + l.mutable_connection_balance_config()->mutable_exact_balance(); + }, + [](envoy::config::listener::v3::Listener& l) { l.mutable_enable_reuse_port(); }, + [](envoy::config::listener::v3::Listener& l) { l.mutable_freebind(); }, + [](envoy::config::listener::v3::Listener& l) { l.mutable_tcp_backlog_size(); }, + [](envoy::config::listener::v3::Listener& l) { l.mutable_tcp_fast_open_queue_length(); }, + [](envoy::config::listener::v3::Listener& l) { l.mutable_transparent(); }, + }; + for (const auto& f : listener_mutators) { + auto new_listener = listener; + f(new_listener); + EXPECT_THROW_WITH_MESSAGE(manager_->addOrUpdateListener(new_listener, "", true), EnvoyException, + "error adding listener 'envoy://test_internal_listener_name': has " + "unsupported tcp listener feature"); + } + + { + auto new_listener = listener; + new_listener.mutable_socket_options()->Add(); + EXPECT_THROW_WITH_MESSAGE(manager_->addOrUpdateListener(new_listener, "", true), EnvoyException, + "error adding listener 'envoy://test_internal_listener_name': does " + "not support socket option") + } + + { + auto new_listener = listener; + new_listener.set_enable_mptcp(true); + EXPECT_THROW_WITH_MESSAGE(manager_->addOrUpdateListener(new_listener, "", true), EnvoyException, + "listener foo: enable_mptcp can only be used with IP addresses") + } +} + TEST_F(ListenerManagerImplTest, NotDefaultListenerFiltersTimeout) { const std::string yaml = R"EOF( name: "foo" From f5efb17c7b1940898e5afba3fe737c06e1378bfc Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Tue, 16 Nov 2021 01:10:04 +0000 Subject: [PATCH 31/31] more validation Signed-off-by: Yuchen Dai --- source/server/listener_impl.cc | 14 +++++++++++--- test/server/listener_manager_impl_test.cc | 11 ++++++----- 2 files changed, 17 insertions(+), 8 deletions(-) diff --git a/source/server/listener_impl.cc b/source/server/listener_impl.cc index 13efe061d0ae9..1ed954ba5a9c3 100644 --- a/source/server/listener_impl.cc +++ b/source/server/listener_impl.cc @@ -467,10 +467,18 @@ void ListenerImpl::buildAccessLog() { void ListenerImpl::buildInternalListener() { if (config_.address().has_envoy_internal_address()) { internal_listener_config_ = std::make_unique(); - if (config_.has_connection_balance_config() || config_.enable_mptcp() || - config_.has_enable_reuse_port() || config_.has_freebind() || + if (config_.has_api_listener()) { + throw EnvoyException( + fmt::format("error adding listener '{}': internal address cannot be used in api listener", + address_->asString())); + } + if ((config_.has_connection_balance_config() && + config_.connection_balance_config().has_exact_balance()) || + config_.enable_mptcp() || + config_.has_enable_reuse_port() // internal listener doesn't use physical l4 port. + || (config_.has_freebind() && config_.freebind().value()) || config_.has_tcp_backlog_size() || config_.has_tcp_fast_open_queue_length() || - config_.has_transparent()) { + (config_.has_transparent() && config_.transparent().value())) { throw EnvoyException( fmt::format("error adding listener '{}': has unsupported tcp listener feature", address_->asString())); diff --git a/test/server/listener_manager_impl_test.cc b/test/server/listener_manager_impl_test.cc index 078826a4ecf15..8002094c46467 100644 --- a/test/server/listener_manager_impl_test.cc +++ b/test/server/listener_manager_impl_test.cc @@ -578,19 +578,21 @@ TEST_F(ListenerManagerImplTest, RejectTcpOptionsWithInternalListenerConfig) { l.mutable_connection_balance_config()->mutable_exact_balance(); }, [](envoy::config::listener::v3::Listener& l) { l.mutable_enable_reuse_port(); }, - [](envoy::config::listener::v3::Listener& l) { l.mutable_freebind(); }, + [](envoy::config::listener::v3::Listener& l) { l.mutable_freebind()->set_value(true); }, [](envoy::config::listener::v3::Listener& l) { l.mutable_tcp_backlog_size(); }, [](envoy::config::listener::v3::Listener& l) { l.mutable_tcp_fast_open_queue_length(); }, - [](envoy::config::listener::v3::Listener& l) { l.mutable_transparent(); }, + [](envoy::config::listener::v3::Listener& l) { l.mutable_transparent()->set_value(true); }, + }; for (const auto& f : listener_mutators) { auto new_listener = listener; f(new_listener); - EXPECT_THROW_WITH_MESSAGE(manager_->addOrUpdateListener(new_listener, "", true), EnvoyException, + EXPECT_THROW_WITH_MESSAGE(new ListenerImpl(new_listener, "version", *manager_, "foo", true, + false, /*hash=*/static_cast(0), 1), + EnvoyException, "error adding listener 'envoy://test_internal_listener_name': has " "unsupported tcp listener feature"); } - { auto new_listener = listener; new_listener.mutable_socket_options()->Add(); @@ -598,7 +600,6 @@ TEST_F(ListenerManagerImplTest, RejectTcpOptionsWithInternalListenerConfig) { "error adding listener 'envoy://test_internal_listener_name': does " "not support socket option") } - { auto new_listener = listener; new_listener.set_enable_mptcp(true);