From 2352e5c648ec71e6467b24866cfcfb10e19cef57 Mon Sep 17 00:00:00 2001 From: Dan Zhang Date: Fri, 3 Sep 2021 15:41:10 -0400 Subject: [PATCH 01/13] inplace filter chain update Signed-off-by: Dan Zhang --- envoy/network/connection_handler.h | 11 + source/common/network/udp_listener_impl.cc | 4 +- source/common/quic/BUILD | 1 + source/common/quic/active_quic_listener.cc | 19 ++ source/common/quic/active_quic_listener.h | 5 + .../quic/client_connection_factory_impl.cc | 23 +- .../quic/client_connection_factory_impl.h | 4 +- .../common/quic/envoy_quic_client_session.cc | 22 +- .../common/quic/envoy_quic_client_session.h | 10 +- source/common/quic/envoy_quic_dispatcher.cc | 56 +++-- source/common/quic/envoy_quic_dispatcher.h | 7 +- source/common/quic/envoy_quic_proof_source.cc | 9 +- source/common/quic/envoy_quic_proof_source.h | 6 +- .../common/quic/envoy_quic_server_session.cc | 54 ++++- .../common/quic/envoy_quic_server_session.h | 27 ++- .../quic_filter_manager_connection_impl.cc | 15 +- .../quic_filter_manager_connection_impl.h | 2 +- .../quic/quic_transport_socket_factory.cc | 15 +- .../quic/quic_transport_socket_factory.h | 18 +- .../transport_sockets/tls/context_impl.cc | 6 + source/server/active_stream_listener_base.h | 4 +- source/server/active_tcp_listener.h | 4 +- source/server/active_udp_listener.h | 6 + source/server/connection_handler_impl.cc | 19 +- source/server/listener_impl.cc | 29 +-- source/server/listener_impl.h | 2 +- source/server/listener_manager_impl.cc | 2 + source/server/listener_manager_impl.h | 6 +- .../common/quic/envoy_quic_dispatcher_test.cc | 51 +++++ .../quic/envoy_quic_server_session_test.cc | 3 +- test/common/quic/test_utils.h | 5 +- test/config/utility.cc | 11 +- test/config/utility.h | 2 +- test/integration/http_integration.cc | 3 +- .../integration/quic_http_integration_test.cc | 207 +++++++++++++++++- test/server/listener_manager_impl_test.cc | 4 +- 36 files changed, 560 insertions(+), 112 deletions(-) diff --git a/envoy/network/connection_handler.h b/envoy/network/connection_handler.h index beabccddf8cf2..5cab5acdc26bc 100644 --- a/envoy/network/connection_handler.h +++ b/envoy/network/connection_handler.h @@ -132,6 +132,17 @@ class ConnectionHandler { * Stop listening according to implementation's own definition. */ virtual void shutdownListener() PURE; + + /** + * Update the listener config. + */ + virtual void updateListenerConfig(Network::ListenerConfig& config) PURE; + + /** + * Called when the given filter chains are about to be removed. + */ + virtual void onFilterChainDraining( + const std::list& draining_filter_chains) PURE; }; using ActiveListenerPtr = std::unique_ptr; diff --git a/source/common/network/udp_listener_impl.cc b/source/common/network/udp_listener_impl.cc index f6dcb614d2788..f000236733cc4 100644 --- a/source/common/network/udp_listener_impl.cc +++ b/source/common/network/udp_listener_impl.cc @@ -132,7 +132,6 @@ UdpListenerWorkerRouterImpl::UdpListenerWorkerRouterImpl(uint32_t concurrency) void UdpListenerWorkerRouterImpl::registerWorkerForListener(UdpListenerCallbacks& listener) { absl::WriterMutexLock lock(&mutex_); - ASSERT(listener.workerIndex() < workers_.size()); ASSERT(workers_.at(listener.workerIndex()) == nullptr); workers_.at(listener.workerIndex()) = &listener; @@ -140,7 +139,8 @@ void UdpListenerWorkerRouterImpl::registerWorkerForListener(UdpListenerCallbacks void UdpListenerWorkerRouterImpl::unregisterWorkerForListener(UdpListenerCallbacks& listener) { absl::WriterMutexLock lock(&mutex_); - + ASSERT(workers_.at(listener.workerIndex()) != nullptr, + fmt::format("no listener is registered at {}", listener.workerIndex())); ASSERT(workers_.at(listener.workerIndex()) == &listener); workers_.at(listener.workerIndex()) = nullptr; } diff --git a/source/common/quic/BUILD b/source/common/quic/BUILD index e9ba4bcdba7e2..c370c912e07d0 100644 --- a/source/common/quic/BUILD +++ b/source/common/quic/BUILD @@ -266,6 +266,7 @@ envoy_cc_library( ":envoy_quic_utils_lib", ":quic_filter_manager_connection_lib", ":quic_stat_names_lib", + ":quic_transport_socket_factory_lib", "//source/common/buffer:buffer_lib", "//source/common/common:assert_lib", "//source/common/http:codes_lib", diff --git a/source/common/quic/active_quic_listener.cc b/source/common/quic/active_quic_listener.cc index 5bd7651758ada..16051ac8afef9 100644 --- a/source/common/quic/active_quic_listener.cc +++ b/source/common/quic/active_quic_listener.cc @@ -221,6 +221,25 @@ size_t ActiveQuicListener::numPacketsExpectedPerEventLoop() const { return quic_dispatcher_->NumSessions() * packets_to_read_to_connection_count_ratio_; } +void ActiveQuicListener::updateListenerConfig(Network::ListenerConfig& config) { + config_ = &config; + dynamic_cast(crypto_config_->proof_source()) + ->updateFilterChainManager(config.filterChainManager()); + quic_dispatcher_->updateListenerConfig(config); +} + +void ActiveQuicListener::onFilterChainDraining( + const std::list& draining_filter_chains) { + std::for_each(draining_filter_chains.begin(), draining_filter_chains.end(), + [this](const Network::FilterChain* filter_chain) { + closeConnectionsWithFilterChain(filter_chain); + }); +} + +void ActiveQuicListener::closeConnectionsWithFilterChain(const Network::FilterChain* filter_chain) { + quic_dispatcher_->closeConnectionsWithFilterChain(filter_chain); +} + ActiveQuicListenerFactory::ActiveQuicListenerFactory( const envoy::config::listener::v3::QuicProtocolOptions& config, uint32_t concurrency, QuicStatNames& quic_stat_names) diff --git a/source/common/quic/active_quic_listener.h b/source/common/quic/active_quic_listener.h index be72d17334770..eca1f28c0caa8 100644 --- a/source/common/quic/active_quic_listener.h +++ b/source/common/quic/active_quic_listener.h @@ -70,10 +70,15 @@ class ActiveQuicListener : public Envoy::Server::ActiveUdpListenerBase, void pauseListening() override; void resumeListening() override; void shutdownListener() override; + void updateListenerConfig(Network::ListenerConfig& config) override; + void onFilterChainDraining( + const std::list& draining_filter_chains) override; private: friend class ActiveQuicListenerPeer; + void closeConnectionsWithFilterChain(const Network::FilterChain* filter_chain); + uint8_t random_seed_[16]; std::unique_ptr crypto_config_; Event::Dispatcher& dispatcher_; diff --git a/source/common/quic/client_connection_factory_impl.cc b/source/common/quic/client_connection_factory_impl.cc index 00d86543436ff..d2df3665c1727 100644 --- a/source/common/quic/client_connection_factory_impl.cc +++ b/source/common/quic/client_connection_factory_impl.cc @@ -33,8 +33,9 @@ std::shared_ptr PersistentQuicInfoImpl::cryptoConf if (context.get() != client_context_.get()) { client_context_ = context; client_config_ = std::make_shared( - std::make_unique(getContext(transport_socket_factory_)), + std::make_unique(std::move(context)), std::make_unique((time_source_))); + ASSERT(server_id_.host() == getConfig(transport_socket_factory_).serverNameIndication()); } // Return the latest client config. return client_config_; @@ -52,14 +53,15 @@ PersistentQuicInfoImpl::PersistentQuicInfoImpl( quiche::FlagRegistry::getInstance(); } -std::unique_ptr -createQuicNetworkConnection(Http::PersistentQuicInfo& info, Event::Dispatcher& dispatcher, - Network::Address::InstanceConstSharedPtr server_addr, - Network::Address::InstanceConstSharedPtr local_addr, - QuicStatNames& quic_stat_names, Stats::Scope& scope) { +std::unique_ptr createQuicNetworkConnection( + Http::PersistentQuicInfo& info, Event::Dispatcher& dispatcher, + Network::Address::InstanceConstSharedPtr server_addr, + Network::Address::InstanceConstSharedPtr local_addr, QuicStatNames& quic_stat_names, + Stats::Scope& scope, const std::string& host_name_override, const std::string& alpn_override) { // This flag fix a QUICHE issue which may crash Envoy during connection close. SetQuicReloadableFlag(quic_single_ack_in_packet2, true); PersistentQuicInfoImpl* info_impl = reinterpret_cast(&info); + ASSERT(info_impl); auto config = info_impl->cryptoConfig(); if (config == nullptr) { return nullptr; // no secrets available yet. @@ -72,9 +74,14 @@ createQuicNetworkConnection(Http::PersistentQuicInfo& info, Event::Dispatcher& d // QUICHE client session always use the 1st version to start handshake. auto ret = std::make_unique( - info_impl->quic_config_, quic_versions, std::move(connection), info_impl->server_id_, + info_impl->quic_config_, quic_versions, std::move(connection), + (host_name_override.empty() + ? info_impl->server_id_ + : quic::QuicServerId{host_name_override, + static_cast(server_addr->ip()->port()), false}), std::move(config), &info_impl->push_promise_index_, dispatcher, info_impl->buffer_limit_, - info_impl->crypto_stream_factory_, quic_stat_names, scope); + info_impl->crypto_stream_factory_, quic_stat_names, scope, + info_impl->transport_socket_factory_, alpn_override); return ret; } diff --git a/source/common/quic/client_connection_factory_impl.h b/source/common/quic/client_connection_factory_impl.h index acc1d0cb244c2..f310d4d1fa972 100644 --- a/source/common/quic/client_connection_factory_impl.h +++ b/source/common/quic/client_connection_factory_impl.h @@ -54,7 +54,9 @@ std::unique_ptr createQuicNetworkConnection(Http::PersistentQuicInfo& info, Event::Dispatcher& dispatcher, Network::Address::InstanceConstSharedPtr server_addr, Network::Address::InstanceConstSharedPtr local_addr, - QuicStatNames& quic_stat_names, Stats::Scope& scope); + QuicStatNames& quic_stat_names, Stats::Scope& scope, + const std::string& host_name_override = "", + const std::string& alpn_override = ""); } // namespace Quic } // namespace Envoy diff --git a/source/common/quic/envoy_quic_client_session.cc b/source/common/quic/envoy_quic_client_session.cc index df9fd5f7a6755..87b4b9b1585b4 100644 --- a/source/common/quic/envoy_quic_client_session.cc +++ b/source/common/quic/envoy_quic_client_session.cc @@ -1,6 +1,7 @@ #include "source/common/quic/envoy_quic_client_session.h" #include "source/common/quic/envoy_quic_utils.h" +#include "source/common/quic/quic_transport_socket_factory.h" namespace Envoy { namespace Quic { @@ -11,14 +12,17 @@ EnvoyQuicClientSession::EnvoyQuicClientSession( std::shared_ptr crypto_config, quic::QuicClientPushPromiseIndex* push_promise_index, Event::Dispatcher& dispatcher, uint32_t send_buffer_limit, EnvoyQuicCryptoClientStreamFactoryInterface& crypto_stream_factory, - QuicStatNames& quic_stat_names, Stats::Scope& scope) + QuicStatNames& quic_stat_names, Stats::Scope& scope, + const Network::TransportSocketFactory& transport_socket_factory, + const std::string& alpn_override) : QuicFilterManagerConnectionImpl(*connection, connection->connection_id(), dispatcher, send_buffer_limit), quic::QuicSpdyClientSession(config, supported_versions, connection.release(), server_id, crypto_config.get(), push_promise_index), host_name_(server_id.host()), crypto_config_(crypto_config), crypto_stream_factory_(crypto_stream_factory), quic_stat_names_(quic_stat_names), - scope_(scope) {} + scope_(scope), transport_socket_factory_(transport_socket_factory), + alpn_override_(alpn_override) {} EnvoyQuicClientSession::~EnvoyQuicClientSession() { ASSERT(!connection()->connected()); @@ -122,5 +126,19 @@ std::unique_ptr EnvoyQuicClientSession::Create this, /*has_application_state = */ version().UsesHttp3()); } +std::vector EnvoyQuicClientSession::GetAlpnsToOffer() const { + if (!alpn_override_.empty()) { + return {alpn_override_}; + } + const std::vector& alpns_configured = + dynamic_cast(transport_socket_factory_) + .alpnsConfigured(); + if (!alpns_configured.empty()) { + return alpns_configured; + } + // Offer the default ALPN which is "h3". + return quic::QuicSpdyClientSession::GetAlpnsToOffer(); +} + } // namespace Quic } // namespace Envoy diff --git a/source/common/quic/envoy_quic_client_session.h b/source/common/quic/envoy_quic_client_session.h index def847ea0eb23..c752c779bb8dc 100644 --- a/source/common/quic/envoy_quic_client_session.h +++ b/source/common/quic/envoy_quic_client_session.h @@ -40,7 +40,9 @@ class EnvoyQuicClientSession : public QuicFilterManagerConnectionImpl, quic::QuicClientPushPromiseIndex* push_promise_index, Event::Dispatcher& dispatcher, uint32_t send_buffer_limit, EnvoyQuicCryptoClientStreamFactoryInterface& crypto_stream_factory, - QuicStatNames& quic_stat_names, Stats::Scope& scope); + QuicStatNames& quic_stat_names, Stats::Scope& scope, + const Network::TransportSocketFactory& transport_socket_factory, + const std::string& alpn_override); ~EnvoyQuicClientSession() override; @@ -78,6 +80,7 @@ class EnvoyQuicClientSession : public QuicFilterManagerConnectionImpl, // active stream. return std::max(1, GetNumActiveStreams()) * Network::NUM_DATAGRAMS_PER_RECEIVE; } + std::vector GetAlpnsToOffer() const override; using quic::QuicSpdyClientSession::PerformActionOnActiveStreams; @@ -99,11 +102,14 @@ class EnvoyQuicClientSession : public QuicFilterManagerConnectionImpl, // These callbacks are owned by network filters and quic session should outlive // them. Http::ConnectionCallbacks* http_connection_callbacks_{nullptr}; - const absl::string_view host_name_; + // TODO(danzh) deprecate this field once server_id() is made const. + const std::string host_name_; std::shared_ptr crypto_config_; EnvoyQuicCryptoClientStreamFactoryInterface& crypto_stream_factory_; QuicStatNames& quic_stat_names_; Stats::Scope& scope_; + const Network::TransportSocketFactory& transport_socket_factory_; + std::string alpn_override_; }; } // namespace Quic diff --git a/source/common/quic/envoy_quic_dispatcher.cc b/source/common/quic/envoy_quic_dispatcher.cc index 217e561e2b65d..03dfd38bf4879 100644 --- a/source/common/quic/envoy_quic_dispatcher.cc +++ b/source/common/quic/envoy_quic_dispatcher.cc @@ -2,12 +2,14 @@ #include +#include +#include + #include "envoy/common/optref.h" #include "source/common/common/safe_memcpy.h" #include "source/common/http/utility.h" #include "source/common/quic/envoy_quic_server_connection.h" -#include "source/common/quic/envoy_quic_server_session.h" #include "source/common/quic/envoy_quic_utils.h" namespace Envoy { @@ -26,7 +28,7 @@ EnvoyQuicDispatcher::EnvoyQuicDispatcher( : quic::QuicDispatcher(&quic_config, crypto_config, version_manager, std::move(helper), std::make_unique(), std::move(alarm_factory), expected_server_connection_id_length), - connection_handler_(connection_handler), listener_config_(listener_config), + connection_handler_(connection_handler), listener_config_(&listener_config), listener_stats_(listener_stats), per_worker_stats_(per_worker_stats), dispatcher_(dispatcher), listen_socket_(listen_socket), quic_stat_names_(quic_stat_names), crypto_server_stream_factory_(crypto_server_stream_factory) { @@ -52,19 +54,21 @@ void EnvoyQuicDispatcher::OnConnectionClosed(quic::QuicConnectionId connection_i listener_stats_.downstream_cx_active_.dec(); per_worker_stats_.downstream_cx_active_.dec(); connection_handler_.decNumConnections(); - quic_stat_names_.chargeQuicConnectionCloseStats(listener_config_.listenerScope(), error, source, + quic_stat_names_.chargeQuicConnectionCloseStats(listener_config_->listenerScope(), error, source, /*is_upstream*/ false); } std::unique_ptr EnvoyQuicDispatcher::CreateQuicSession( quic::QuicConnectionId server_connection_id, const quic::QuicSocketAddress& self_address, - const quic::QuicSocketAddress& peer_address, absl::string_view alpn, + const quic::QuicSocketAddress& peer_address, absl::string_view /*alpn*/, const quic::ParsedQuicVersion& version, absl::string_view sni) { quic::QuicConfig quic_config = config(); + // TODO(danzh) use passed-in ALPN instead of hard-coded h3 after proof source interfaces takes in + // ALPN. Network::ConnectionSocketPtr connection_socket = createServerConnectionSocket( - listen_socket_.ioHandle(), self_address, peer_address, std::string(sni), alpn); + listen_socket_.ioHandle(), self_address, peer_address, std::string(sni), "h3"); const Network::FilterChain* filter_chain = - listener_config_.filterChainManager().findFilterChain(*connection_socket); + listener_config_->filterChainManager().findFilterChain(*connection_socket); auto quic_connection = std::make_unique( server_connection_id, self_address, peer_address, *helper(), *alarm_factory(), writer(), @@ -72,24 +76,21 @@ std::unique_ptr EnvoyQuicDispatcher::CreateQuicSession( auto quic_session = std::make_unique( quic_config, quic::ParsedQuicVersionVector{version}, std::move(quic_connection), this, session_helper(), crypto_config(), compressed_certs_cache(), dispatcher_, - listener_config_.perConnectionBufferLimitBytes(), quic_stat_names_, - listener_config_.listenerScope(), crypto_server_stream_factory_, - makeOptRefFromPtr(filter_chain == nullptr ? nullptr - : &filter_chain->transportSocketFactory())); + listener_config_->perConnectionBufferLimitBytes(), quic_stat_names_, + listener_config_->listenerScope(), crypto_server_stream_factory_); if (filter_chain != nullptr) { + // Setup filter chain before Initialize(). const bool has_filter_initialized = - listener_config_.filterChainFactory().createNetworkFilterChain( + listener_config_->filterChainFactory().createNetworkFilterChain( *quic_session, filter_chain->networkFilterFactories()); // QUIC listener must have HCM filter configured. Otherwise, stream creation later will fail. ASSERT(has_filter_initialized); + connections_by_filter_chain_[filter_chain].push_front( + std::reference_wrapper(*quic_session)); + quic_session->storeConnectionMapPosition(connections_by_filter_chain_, *filter_chain, + connections_by_filter_chain_[filter_chain].begin()); } quic_session->Initialize(); - // Filter chain can't be retrieved here as self address is unknown at this - // point. - // TODO(danzh): change QUIC interface to pass in self address as it is already - // known. In this way, filter chain can be retrieved at this point. But one - // thing to pay attention is that if the retrieval fails, connection needs to - // be closed, and it should be added to time wait list instead of session map. connection_handler_.incNumConnections(); listener_stats_.downstream_cx_active_.inc(); listener_stats_.downstream_cx_total_.inc(); @@ -107,5 +108,26 @@ quic::QuicConnectionId EnvoyQuicDispatcher::ReplaceLongServerConnectionId( return new_connection_id; } +void EnvoyQuicDispatcher::closeConnectionsWithFilterChain( + const Network::FilterChain* filter_chain) { + if (connections_by_filter_chain_.find(filter_chain) != connections_by_filter_chain_.end()) { + std::list>& connections = + connections_by_filter_chain_[filter_chain]; + // Retain the number of connections in the list early because closing the connection will change + // the size. + size_t num_connections = connections.size(); + for (size_t i = 0; i < num_connections; ++i) { + Network::Connection& connection = connections.front().get(); + // This will remove the connection from the list. + connection.close(Network::ConnectionCloseType::NoFlush); + } + ASSERT(connections_by_filter_chain_.find(filter_chain) == connections_by_filter_chain_.end()); + } +} + +void EnvoyQuicDispatcher::updateListenerConfig(Network::ListenerConfig& new_listener_config) { + listener_config_ = &new_listener_config; +} + } // namespace Quic } // namespace Envoy diff --git a/source/common/quic/envoy_quic_dispatcher.h b/source/common/quic/envoy_quic_dispatcher.h index b429e908d11be..77ed2ffcb361c 100644 --- a/source/common/quic/envoy_quic_dispatcher.h +++ b/source/common/quic/envoy_quic_dispatcher.h @@ -20,6 +20,7 @@ #include "source/server/connection_handler_impl.h" #include "source/server/active_listener_base.h" #include "source/common/quic/envoy_quic_crypto_stream_factory.h" +#include "source/common/quic/envoy_quic_server_session.h" #include "source/common/quic/quic_stat_names.h" namespace Envoy { @@ -54,6 +55,9 @@ class EnvoyQuicDispatcher : public quic::QuicDispatcher { void OnConnectionClosed(quic::QuicConnectionId connection_id, quic::QuicErrorCode error, const std::string& error_details, quic::ConnectionCloseSource source) override; + void closeConnectionsWithFilterChain(const Network::FilterChain* filter_chain); + + void updateListenerConfig(Network::ListenerConfig& new_listener_config); protected: // quic::QuicDispatcher @@ -72,13 +76,14 @@ class EnvoyQuicDispatcher : public quic::QuicDispatcher { private: Network::ConnectionHandler& connection_handler_; - Network::ListenerConfig& listener_config_; + Network::ListenerConfig* listener_config_{nullptr}; Server::ListenerStats& listener_stats_; Server::PerHandlerListenerStats& per_worker_stats_; Event::Dispatcher& dispatcher_; Network::Socket& listen_socket_; QuicStatNames& quic_stat_names_; EnvoyQuicCryptoServerStreamFactoryInterface& crypto_server_stream_factory_; + FilterChainToConnectionMap connections_by_filter_chain_; }; } // namespace Quic diff --git a/source/common/quic/envoy_quic_proof_source.cc b/source/common/quic/envoy_quic_proof_source.cc index 67d9e0ce3ce55..84019c951fb6b 100644 --- a/source/common/quic/envoy_quic_proof_source.cc +++ b/source/common/quic/envoy_quic_proof_source.cc @@ -100,13 +100,15 @@ EnvoyQuicProofSource::getTlsCertConfigAndFilterChain(const quic::QuicSocketAddre Network::ConnectionSocketPtr connection_socket = createServerConnectionSocket( listen_socket_.ioHandle(), server_address, client_address, hostname, "h3"); const Network::FilterChain* filter_chain = - filter_chain_manager_.findFilterChain(*connection_socket); + filter_chain_manager_->findFilterChain(*connection_socket); if (filter_chain == nullptr) { listener_stats_.no_filter_chain_match_.inc(); ENVOY_LOG(warn, "No matching filter chain found for handshake."); return {absl::nullopt, absl::nullopt}; } + ENVOY_LOG(trace, "Got a matching cert chain {}", filter_chain->name()); + auto& transport_socket_factory = dynamic_cast(filter_chain->transportSocketFactory()); @@ -122,5 +124,10 @@ EnvoyQuicProofSource::getTlsCertConfigAndFilterChain(const quic::QuicSocketAddre return {tls_cert_configs[0].get(), *filter_chain}; } +void EnvoyQuicProofSource::updateFilterChainManager( + Network::FilterChainManager& filter_chain_manager) { + filter_chain_manager_ = &filter_chain_manager; +} + } // namespace Quic } // namespace Envoy diff --git a/source/common/quic/envoy_quic_proof_source.h b/source/common/quic/envoy_quic_proof_source.h index fcf388c609140..8d32ad7176ee1 100644 --- a/source/common/quic/envoy_quic_proof_source.h +++ b/source/common/quic/envoy_quic_proof_source.h @@ -14,7 +14,7 @@ class EnvoyQuicProofSource : public EnvoyQuicProofSourceBase { EnvoyQuicProofSource(Network::Socket& listen_socket, Network::FilterChainManager& filter_chain_manager, Server::ListenerStats& listener_stats) - : listen_socket_(listen_socket), filter_chain_manager_(filter_chain_manager), + : listen_socket_(listen_socket), filter_chain_manager_(&filter_chain_manager), listener_stats_(listener_stats) {} ~EnvoyQuicProofSource() override = default; @@ -24,6 +24,8 @@ class EnvoyQuicProofSource : public EnvoyQuicProofSourceBase { GetCertChain(const quic::QuicSocketAddress& server_address, const quic::QuicSocketAddress& client_address, const std::string& hostname) override; + void updateFilterChainManager(Network::FilterChainManager& filter_chain_manager); + protected: // quic::ProofSource void signPayload(const quic::QuicSocketAddress& server_address, @@ -43,7 +45,7 @@ class EnvoyQuicProofSource : public EnvoyQuicProofSourceBase { const std::string& hostname); Network::Socket& listen_socket_; - Network::FilterChainManager& filter_chain_manager_; + Network::FilterChainManager* filter_chain_manager_{nullptr}; Server::ListenerStats& listener_stats_; }; diff --git a/source/common/quic/envoy_quic_server_session.cc b/source/common/quic/envoy_quic_server_session.cc index 15d1d28745dd3..b64db0c791630 100644 --- a/source/common/quic/envoy_quic_server_session.cc +++ b/source/common/quic/envoy_quic_server_session.cc @@ -1,11 +1,15 @@ #include "source/common/quic/envoy_quic_server_session.h" +#include #include #include "source/common/common/assert.h" #include "source/common/quic/envoy_quic_proof_source.h" #include "source/common/quic/envoy_quic_server_stream.h" +#include "envoy_quic_server_connection.h" +#include "quic_transport_socket_factory.h" + namespace Envoy { namespace Quic { @@ -15,15 +19,14 @@ EnvoyQuicServerSession::EnvoyQuicServerSession( quic::QuicCryptoServerStream::Helper* helper, const quic::QuicCryptoServerConfig* crypto_config, quic::QuicCompressedCertsCache* compressed_certs_cache, Event::Dispatcher& dispatcher, uint32_t send_buffer_limit, QuicStatNames& quic_stat_names, Stats::Scope& listener_scope, - EnvoyQuicCryptoServerStreamFactoryInterface& crypto_server_stream_factory, - OptRef transport_socket_factory) + EnvoyQuicCryptoServerStreamFactoryInterface& crypto_server_stream_factory) : quic::QuicServerSessionBase(config, supported_versions, connection.get(), visitor, helper, crypto_config, compressed_certs_cache), QuicFilterManagerConnectionImpl(*connection, connection->connection_id(), dispatcher, send_buffer_limit), quic_connection_(std::move(connection)), quic_stat_names_(quic_stat_names), - listener_scope_(listener_scope), crypto_server_stream_factory_(crypto_server_stream_factory), - transport_socket_factory_(transport_socket_factory) {} + listener_scope_(listener_scope), crypto_server_stream_factory_(crypto_server_stream_factory) { +} EnvoyQuicServerSession::~EnvoyQuicServerSession() { ASSERT(!quic_connection_->connected()); @@ -39,7 +42,9 @@ EnvoyQuicServerSession::CreateQuicCryptoServerStream( const quic::QuicCryptoServerConfig* crypto_config, quic::QuicCompressedCertsCache* compressed_certs_cache) { return crypto_server_stream_factory_.createEnvoyQuicCryptoServerStream( - crypto_config, compressed_certs_cache, this, stream_helper(), transport_socket_factory_, + crypto_config, compressed_certs_cache, this, stream_helper(), + makeOptRefFromPtr(position_.has_value() ? &position_->filter_chain_.transportSocketFactory() + : nullptr), dispatcher()); } @@ -89,6 +94,17 @@ void EnvoyQuicServerSession::OnConnectionClosed(const quic::QuicConnectionCloseF quic::ConnectionCloseSource source) { quic::QuicServerSessionBase::OnConnectionClosed(frame, source); onConnectionCloseEvent(frame, source, version()); + if (position_.has_value()) { + // Remove this connection from the map. + std::list>& connections = + position_->connection_map_[&position_->filter_chain_]; + connections.erase(position_->iterator_); + if (connections.empty()) { + // Remove the whole entry if this is the last connection using this filter chain. + position_->connection_map_.erase(&position_->filter_chain_); + } + position_.reset(); + } } void EnvoyQuicServerSession::Initialize() { @@ -133,5 +149,33 @@ void EnvoyQuicServerSession::OnRstStream(const quic::QuicRstStreamFrame& frame) /*from_self*/ false, /*is_upstream*/ false); } +void EnvoyQuicServerSession::storeConnectionMapPosition(FilterChainToConnectionMap& connection_map, + const Network::FilterChain& filter_chain, + ConnectionMapIter position) { + position_.emplace(connection_map, filter_chain, position); +} + +std::vector::const_iterator +EnvoyQuicServerSession::SelectAlpn(const std::vector& alpns) const { + ASSERT(position_.has_value()); + const std::vector& alpns_configured = + dynamic_cast( + position_->filter_chain_.transportSocketFactory()) + .alpnsConfigured(); + if (alpns_configured.empty()) { + // If the server transport socket doesn't specify supported ALPNs, select the first one provided + // by the peer. + return alpns.begin(); + } + // Otherwise select the first one mutually supported. + for (auto iter = alpns.begin(); iter != alpns.end(); ++iter) { + if (std::find(alpns_configured.begin(), alpns_configured.end(), *iter) != + alpns_configured.end()) { + return iter; + } + } + return alpns.end(); +} + } // namespace Quic } // namespace Envoy diff --git a/source/common/quic/envoy_quic_server_session.h b/source/common/quic/envoy_quic_server_session.h index a3a804023aef9..b2c8f7cd62e48 100644 --- a/source/common/quic/envoy_quic_server_session.h +++ b/source/common/quic/envoy_quic_server_session.h @@ -29,6 +29,21 @@ namespace Envoy { namespace Quic { +using FilterChainToConnectionMap = + absl::flat_hash_map>>; +using ConnectionMapIter = std::list>::iterator; + +struct ConnectionMapPosition { + ConnectionMapPosition(FilterChainToConnectionMap& connection_map, + const Network::FilterChain& filter_chain, ConnectionMapIter iterator) + : connection_map_(connection_map), filter_chain_(filter_chain), iterator_(iterator) {} + + FilterChainToConnectionMap& connection_map_; + const Network::FilterChain& filter_chain_; + ConnectionMapIter iterator_; +}; + // Act as a Network::Connection to HCM and a FilterManager to FilterFactoryCb. // TODO(danzh) Lifetime of quic connection and filter manager connection can be // simplified by changing the inheritance to a member variable instantiated @@ -45,8 +60,7 @@ class EnvoyQuicServerSession : public quic::QuicServerSessionBase, quic::QuicCompressedCertsCache* compressed_certs_cache, Event::Dispatcher& dispatcher, uint32_t send_buffer_limit, QuicStatNames& quic_stat_names, Stats::Scope& listener_scope, - EnvoyQuicCryptoServerStreamFactoryInterface& crypto_server_stream_factory, - OptRef transport_socket_factory); + EnvoyQuicCryptoServerStreamFactoryInterface& crypto_server_stream_factory); ~EnvoyQuicServerSession() override; @@ -77,6 +91,13 @@ class EnvoyQuicServerSession : public quic::QuicServerSessionBase, headers_with_underscores_action_ = headers_with_underscores_action; } + void storeConnectionMapPosition(FilterChainToConnectionMap& connection_map, + const Network::FilterChain& filter_chain, + ConnectionMapIter position); + + std::vector::const_iterator + SelectAlpn(const std::vector& alpns) const override; + using quic::QuicSession::PerformActionOnActiveStreams; protected: @@ -113,7 +134,7 @@ class EnvoyQuicServerSession : public quic::QuicServerSessionBase, Stats::Scope& listener_scope_; EnvoyQuicCryptoServerStreamFactoryInterface& crypto_server_stream_factory_; - OptRef transport_socket_factory_; + absl::optional position_; }; } // namespace Quic diff --git a/source/common/quic/quic_filter_manager_connection_impl.cc b/source/common/quic/quic_filter_manager_connection_impl.cc index a7adce01ea084..31fe437e1dbb1 100644 --- a/source/common/quic/quic_filter_manager_connection_impl.cc +++ b/source/common/quic/quic_filter_manager_connection_impl.cc @@ -12,7 +12,9 @@ QuicFilterManagerConnectionImpl::QuicFilterManagerConnectionImpl( // Using this for purpose other than logging is not safe. Because QUIC connection id can be // 18 bytes, so there might be collision when it's hashed to 8 bytes. : Network::ConnectionImplBase(dispatcher, /*id=*/connection_id.Hash()), - network_connection_(&connection), filter_manager_(*this, *connection.connectionSocket()), + network_connection_(&connection), + filter_manager_( + std::make_unique(*this, *connection.connectionSocket())), stream_info_(dispatcher.timeSource(), connection.connectionSocket()->addressProviderSharedPtr()), write_buffer_watermark_simulation_( @@ -22,23 +24,23 @@ QuicFilterManagerConnectionImpl::QuicFilterManagerConnectionImpl( } void QuicFilterManagerConnectionImpl::addWriteFilter(Network::WriteFilterSharedPtr filter) { - filter_manager_.addWriteFilter(filter); + filter_manager_->addWriteFilter(filter); } void QuicFilterManagerConnectionImpl::addFilter(Network::FilterSharedPtr filter) { - filter_manager_.addFilter(filter); + filter_manager_->addFilter(filter); } void QuicFilterManagerConnectionImpl::addReadFilter(Network::ReadFilterSharedPtr filter) { - filter_manager_.addReadFilter(filter); + filter_manager_->addReadFilter(filter); } void QuicFilterManagerConnectionImpl::removeReadFilter(Network::ReadFilterSharedPtr filter) { - filter_manager_.removeReadFilter(filter); + filter_manager_->removeReadFilter(filter); } bool QuicFilterManagerConnectionImpl::initializeReadFilters() { - return filter_manager_.initializeReadFilters(); + return filter_manager_->initializeReadFilters(); } void QuicFilterManagerConnectionImpl::enableHalfClose(bool enabled) { @@ -171,6 +173,7 @@ void QuicFilterManagerConnectionImpl::onConnectionCloseEvent( network_connection_ = nullptr; } + filter_manager_ = nullptr; if (!codec_stats_.has_value()) { // The connection was closed before it could be used. Stats are not recorded. return; diff --git a/source/common/quic/quic_filter_manager_connection_impl.h b/source/common/quic/quic_filter_manager_connection_impl.h index 614c0e7c82670..af428e4974ef3 100644 --- a/source/common/quic/quic_filter_manager_connection_impl.h +++ b/source/common/quic/quic_filter_manager_connection_impl.h @@ -179,7 +179,7 @@ class QuicFilterManagerConnectionImpl : public Network::ConnectionImplBase, // filters are added, ConnectionManagerImpl should always be the last one. // Its onRead() is only called once to trigger ReadFilter::onNewConnection() // and the rest incoming data bypasses these filters. - Network::FilterManagerImpl filter_manager_; + std::unique_ptr filter_manager_; StreamInfo::StreamInfoImpl stream_info_; std::string transport_failure_reason_; diff --git a/source/common/quic/quic_transport_socket_factory.cc b/source/common/quic/quic_transport_socket_factory.cc index 452b4f45d7c66..8c2a0b6eab719 100644 --- a/source/common/quic/quic_transport_socket_factory.cc +++ b/source/common/quic/quic_transport_socket_factory.cc @@ -16,8 +16,9 @@ QuicServerTransportSocketConfigFactory::createTransportSocketFactory( config, context.messageValidationVisitor()); auto server_config = std::make_unique( quic_transport.downstream_tls_context(), context); - auto factory = - std::make_unique(context.scope(), std::move(server_config)); + auto factory = std::make_unique( + context.scope(), std::move(server_config), + quic_transport.downstream_tls_context().common_tls_context().alpn_protocols()); factory->initialize(); return factory; } @@ -36,16 +37,18 @@ QuicClientTransportSocketConfigFactory::createTransportSocketFactory( config, context.messageValidationVisitor()); auto client_config = std::make_unique( quic_transport.upstream_tls_context(), context); - auto factory = - std::make_unique(std::move(client_config), context); + auto factory = std::make_unique( + std::move(client_config), context, + quic_transport.upstream_tls_context().common_tls_context().alpn_protocols()); factory->initialize(); return factory; } QuicClientTransportSocketFactory::QuicClientTransportSocketFactory( Ssl::ClientContextConfigPtr config, - Server::Configuration::TransportSocketFactoryContext& factory_context) - : QuicTransportSocketFactoryBase(factory_context.scope(), "client"), + Server::Configuration::TransportSocketFactoryContext& factory_context, + const Protobuf::RepeatedPtrField& alpns) + : QuicTransportSocketFactoryBase(factory_context.scope(), "client", alpns), fallback_factory_(std::make_unique( std::move(config), factory_context.sslContextManager(), factory_context.scope())) {} diff --git a/source/common/quic/quic_transport_socket_factory.h b/source/common/quic/quic_transport_socket_factory.h index 7c98e1ef29e69..4793dfc35e1f1 100644 --- a/source/common/quic/quic_transport_socket_factory.h +++ b/source/common/quic/quic_transport_socket_factory.h @@ -37,8 +37,12 @@ QuicTransportSocketFactoryStats generateStats(Stats::Scope& store, const std::st class QuicTransportSocketFactoryBase : public Network::TransportSocketFactory, protected Logger::Loggable { public: - QuicTransportSocketFactoryBase(Stats::Scope& store, const std::string& perspective) - : stats_(generateStats(store, perspective)) {} + QuicTransportSocketFactoryBase(Stats::Scope& store, const std::string& perspective, + const Protobuf::RepeatedPtrField& alpns) + : stats_(generateStats(store, perspective)) { + for_each(alpns.begin(), alpns.end(), + [this](const std::string& alpn) { alpns_.push_back(alpn); }); + } // To be called right after construction. virtual void initialize() PURE; @@ -51,18 +55,21 @@ class QuicTransportSocketFactoryBase : public Network::TransportSocketFactory, bool implementsSecureTransport() const override { return true; } bool usesProxyProtocolOptions() const override { return false; } bool supportsAlpn() const override { return true; } + const std::vector& alpnsConfigured() const { return alpns_; } protected: virtual void onSecretUpdated() PURE; QuicTransportSocketFactoryStats stats_; + std::vector alpns_; }; // TODO(danzh): when implement ProofSource, examine of it's necessary to // differentiate server and client side context config. class QuicServerTransportSocketFactory : public QuicTransportSocketFactoryBase { public: - QuicServerTransportSocketFactory(Stats::Scope& store, Ssl::ServerContextConfigPtr config) - : QuicTransportSocketFactoryBase(store, "server"), config_(std::move(config)) {} + QuicServerTransportSocketFactory(Stats::Scope& store, Ssl::ServerContextConfigPtr config, + const Protobuf::RepeatedPtrField& alpns) + : QuicTransportSocketFactoryBase(store, "server", alpns), config_(std::move(config)) {} void initialize() override { config_->setSecretUpdateCallback([this]() { @@ -93,7 +100,8 @@ class QuicClientTransportSocketFactory : public QuicTransportSocketFactoryBase { public: QuicClientTransportSocketFactory( Ssl::ClientContextConfigPtr config, - Server::Configuration::TransportSocketFactoryContext& factory_context); + Server::Configuration::TransportSocketFactoryContext& factory_context, + const Protobuf::RepeatedPtrField& alpns); void initialize() override {} diff --git a/source/extensions/transport_sockets/tls/context_impl.cc b/source/extensions/transport_sockets/tls/context_impl.cc index f8870d4dddc88..ec5b55a4b8e4a 100644 --- a/source/extensions/transport_sockets/tls/context_impl.cc +++ b/source/extensions/transport_sockets/tls/context_impl.cc @@ -333,6 +333,7 @@ ContextImpl::ContextImpl(Stats::Scope& scope, const Envoy::Ssl::ContextConfig& c for (auto& ctx : tls_contexts_) { SSL_CTX_set_options(ctx.ssl_ctx_.get(), SSL_OP_CIPHER_SERVER_PREFERENCE); } + std::cerr << "============= configured ALPNs " << config.alpnProtocols() << "\n"; parsed_alpn_protocols_ = parseAlpnProtocols(config.alpnProtocols()); @@ -395,6 +396,11 @@ ContextImpl::ContextImpl(Stats::Scope& scope, const Envoy::Ssl::ContextConfig& c int ServerContextImpl::alpnSelectCallback(const unsigned char** out, unsigned char* outlen, const unsigned char* in, unsigned int inlen) { + std::cerr << "========= alpnSelectCallback with peer selected ALPNs " + << std::string(reinterpret_cast(in), inlen) << " configured ALPNs " + << std::string(reinterpret_cast(parsed_alpn_protocols_.data()), + parsed_alpn_protocols_.size()) + << "\n"; // Currently this uses the standard selection algorithm in priority order. const uint8_t* alpn_data = parsed_alpn_protocols_.data(); size_t alpn_data_size = parsed_alpn_protocols_.size(); diff --git a/source/server/active_stream_listener_base.h b/source/server/active_stream_listener_base.h index 89ebd2877c659..38e50a1a4af58 100644 --- a/source/server/active_stream_listener_base.h +++ b/source/server/active_stream_listener_base.h @@ -38,8 +38,8 @@ class ActiveStreamListenerBase : public ActiveListenerImplBase, * Schedule to remove and destroy the active connections which are not tracked by listener * config. Caution: The connection are not destroyed yet when function returns. */ - void - deferredRemoveFilterChains(const std::list& draining_filter_chains) { + void onFilterChainDraining( + const std::list& draining_filter_chains) override { // Need to recover the original deleting state. const bool was_deleting = is_deleting_; is_deleting_ = true; diff --git a/source/server/active_tcp_listener.h b/source/server/active_tcp_listener.h index 00d93e744a8d7..293b996959735 100644 --- a/source/server/active_tcp_listener.h +++ b/source/server/active_tcp_listener.h @@ -71,10 +71,10 @@ class ActiveTcpListener final : public Network::TcpListenerCallbacks, std::unique_ptr stream_info) override; /** - * Update the listener config. The follow up connections will see the new config. The existing + * The follow up connections will see the new config. The existing * connections are not impacted. */ - void updateListenerConfig(Network::ListenerConfig& config); + void updateListenerConfig(Network::ListenerConfig& config) override; Network::TcpConnectionHandler& tcp_conn_handler_; // The number of connections currently active on this listener. This is typically used for diff --git a/source/server/active_udp_listener.h b/source/server/active_udp_listener.h index eef7ca228e738..c8ccffb450297 100644 --- a/source/server/active_udp_listener.h +++ b/source/server/active_udp_listener.h @@ -103,6 +103,12 @@ class ActiveRawUdpListener : public ActiveUdpListenerBase, read_filter_.reset(); udp_listener_.reset(); } + void updateListenerConfig(Network::ListenerConfig&) override { + ENVOY_BUG(false, "In-place filter chain update observed on raw UDP listener."); + } + void onFilterChainDraining(const std::list&) override { + ENVOY_BUG(false, "Filter chain draining observed on raw UDP listener."); + } // Network::UdpListenerFilterManager void addReadFilter(Network::UdpListenerReadFilterPtr&& filter) override; diff --git a/source/server/connection_handler_impl.cc b/source/server/connection_handler_impl.cc index 4f64afc227b06..b218ae962c774 100644 --- a/source/server/connection_handler_impl.cc +++ b/source/server/connection_handler_impl.cc @@ -27,17 +27,16 @@ void ConnectionHandlerImpl::decNumConnections() { void ConnectionHandlerImpl::addListener(absl::optional overridden_listener, Network::ListenerConfig& config) { + if (overridden_listener.has_value()) { + ActiveListenerDetailsOptRef listener_detail = + findActiveListenerByTag(overridden_listener.value()); + ASSERT(listener_detail.has_value()); + listener_detail->get().listener_->updateListenerConfig(config); + return; + } + ActiveListenerDetails details; if (config.listenSocketFactory().socketType() == Network::Socket::Type::Stream) { - if (overridden_listener.has_value()) { - for (auto& listener : listeners_) { - if (listener.second.listener_->listenerTag() == overridden_listener) { - listener.second.tcpListener()->get().updateListenerConfig(config); - return; - } - } - NOT_REACHED_GCOVR_EXCL_LINE; - } // worker_index_ doesn't have a value on the main thread for the admin server. auto tcp_listener = std::make_unique( *this, config, worker_index_.has_value() ? *worker_index_ : 0); @@ -89,7 +88,7 @@ void ConnectionHandlerImpl::removeFilterChains( std::function completion) { for (auto& listener : listeners_) { if (listener.second.listener_->listenerTag() == listener_tag) { - listener.second.tcpListener()->get().deferredRemoveFilterChains(filter_chains); + listener.second.listener_->onFilterChainDraining(filter_chains); break; } } diff --git a/source/server/listener_impl.cc b/source/server/listener_impl.cc index a7c4259ffb23f..fef03cdf2d217 100644 --- a/source/server/listener_impl.cc +++ b/source/server/listener_impl.cc @@ -371,7 +371,7 @@ ListenerImpl::ListenerImpl(ListenerImpl& origin, const envoy::config::listener::v3::Listener& config, const std::string& version_info, ListenerManagerImpl& parent, const std::string& name, bool added_via_api, bool workers_started, - uint64_t hash, uint32_t concurrency) + uint64_t hash, uint32_t /*concurrency*/) : parent_(parent), address_(origin.address_), bind_to_port_(shouldBindToPort(config)), hand_off_restored_destination_connections_( PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, use_original_dst, false)), @@ -392,6 +392,7 @@ ListenerImpl::ListenerImpl(ListenerImpl& origin, listener_filters_timeout_( PROTOBUF_GET_MS_OR_DEFAULT(config, listener_filters_timeout, 15000)), continue_on_listener_filters_timeout_(config.continue_on_listener_filters_timeout()), + udp_listener_config_(origin.udp_listener_config_), connection_balancer_(origin.connection_balancer_), listener_factory_context_(std::make_shared( origin.listener_factory_context_->listener_factory_context_base_, this, *this)), @@ -407,18 +408,18 @@ ListenerImpl::ListenerImpl(ListenerImpl& origin, quic_stat_names_(parent_.quicStatNames()) { buildAccessLog(); auto socket_type = Network::Utility::protobufAddressSocketType(config.address()); - // buildUdpListenerFactory() must come before buildListenSocketOptions() because the UDP - // listener factory can provide additional options. - buildUdpListenerFactory(socket_type, concurrency); buildListenSocketOptions(socket_type); createListenerFilterFactories(socket_type); validateFilterChains(socket_type); buildFilterChains(); - // In place update is tcp only so it's safe to apply below tcp only initialization. - buildSocketOptions(); - buildOriginalDstListenerFilter(); - buildProxyProtocolListenerFilter(); - open_connections_ = origin.open_connections_; + + if (socket_type == Network::Socket::Type::Stream) { + // Apply below tcp only initialization. + buildSocketOptions(); + buildOriginalDstListenerFilter(); + buildProxyProtocolListenerFilter(); + open_connections_ = origin.open_connections_; + } } void ListenerImpl::buildAccessLog() { @@ -441,7 +442,7 @@ void ListenerImpl::buildUdpListenerFactory(Network::Socket::Type socket_type, "set concurrency = 1."); } - udp_listener_config_ = std::make_unique(config_.udp_listener_config()); + udp_listener_config_ = std::make_shared(config_.udp_listener_config()); if (config_.udp_listener_config().has_quic_options()) { #ifdef ENVOY_ENABLE_QUIC if (config_.has_connection_balance_config()) { @@ -767,14 +768,6 @@ bool ListenerImpl::supportUpdateFilterChain(const envoy::config::listener::v3::L return false; } - // Currently we only support TCP filter chain update. - if (Network::Utility::protobufAddressSocketType(config_.address()) != - Network::Socket::Type::Stream || - Network::Utility::protobufAddressSocketType(config.address()) != - Network::Socket::Type::Stream) { - return false; - } - // Full listener update currently rejects tcp listener having 0 filter chain. // In place filter chain update could survive under zero filter chain but we should keep the same // behavior for now. This also guards the below filter chain access. diff --git a/source/server/listener_impl.h b/source/server/listener_impl.h index 0114ff9c9e33e..3222d198f14c0 100644 --- a/source/server/listener_impl.h +++ b/source/server/listener_impl.h @@ -413,7 +413,7 @@ class ListenerImpl final : public Network::ListenerConfig, Network::Socket::OptionsSharedPtr listen_socket_options_; const std::chrono::milliseconds listener_filters_timeout_; const bool continue_on_listener_filters_timeout_; - std::unique_ptr udp_listener_config_; + std::shared_ptr udp_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 afd6ebd142d6d..4478ff67f0eb1 100644 --- a/source/server/listener_manager_impl.cc +++ b/source/server/listener_manager_impl.cc @@ -709,6 +709,8 @@ void ListenerManagerImpl::drainFilterChains(ListenerImplPtr&& draining_listener, // listener while filters might still be using its context (stats, etc.). server_.dispatcher().post([this, draining_group]() -> void { if (draining_group->decWorkersPendingRemoval() == 0) { + std::cerr << "============ Complete draining filter chain from listener " + << draining_group->getDrainingListener().name() << "\n"; draining_group->getDrainingListener().debugLog( absl::StrCat("draining filter chains from listener ", draining_group->getDrainingListener().name(), " complete")); diff --git a/source/server/listener_manager_impl.h b/source/server/listener_manager_impl.h index ad85054671954..b94fcdaf2de6b 100644 --- a/source/server/listener_manager_impl.h +++ b/source/server/listener_manager_impl.h @@ -149,10 +149,14 @@ class DrainingFilterChainsManager { // Schedule listener destroy. void startDrainSequence(std::chrono::seconds drain_time, Event::Dispatcher& dispatcher, std::function completion) { + std::cerr << "=========== startDrainSequence with timeout " << drain_time.count() << "\n"; drain_sequence_completion_ = completion; ASSERT(!drain_timer_); - drain_timer_ = dispatcher.createTimer([this]() -> void { drain_sequence_completion_(); }); + drain_timer_ = dispatcher.createTimer([this]() -> void { + std::cerr << "=========== drain_sequence_completion_\n"; + drain_sequence_completion_(); + }); drain_timer_->enableTimer(drain_time); } diff --git a/test/common/quic/envoy_quic_dispatcher_test.cc b/test/common/quic/envoy_quic_dispatcher_test.cc index ffe9ea95ec22d..391898a446b18 100644 --- a/test/common/quic/envoy_quic_dispatcher_test.cc +++ b/test/common/quic/envoy_quic_dispatcher_test.cc @@ -302,5 +302,56 @@ TEST_P(EnvoyQuicDispatcherTest, CreateNewConnectionUponBufferedCHLO) { processValidChloPacketAndInitializeFilters(true); } +TEST_P(EnvoyQuicDispatcherTest, CloseWithGivenFilterChain) { + Network::MockFilterChainManager filter_chain_manager; + std::shared_ptr read_filter(new Network::MockReadFilter()); + Network::MockConnectionCallbacks network_connection_callbacks; + testing::StrictMock read_total; + testing::StrictMock read_current; + testing::StrictMock write_total; + testing::StrictMock write_current; + + std::vector filter_factory( + {[&](Network::FilterManager& filter_manager) { + filter_manager.addReadFilter(read_filter); + read_filter->callbacks_->connection().addConnectionCallbacks(network_connection_callbacks); + read_filter->callbacks_->connection().setConnectionStats( + {read_total, read_current, write_total, write_current, nullptr, nullptr}); + }}); + EXPECT_CALL(listener_config_, filterChainManager()).WillOnce(ReturnRef(filter_chain_manager)); + EXPECT_CALL(filter_chain_manager, findFilterChain(_)) + .WillOnce(Return(&proof_source_->filterChain())); + Network::MockTransportSocketFactory transport_socket_factory; + EXPECT_CALL(proof_source_->filterChain(), transportSocketFactory()) + .WillOnce(ReturnRef(transport_socket_factory)); + EXPECT_CALL(proof_source_->filterChain(), networkFilterFactories()) + .WillOnce(ReturnRef(filter_factory)); + EXPECT_CALL(listener_config_, filterChainFactory()); + EXPECT_CALL(listener_config_.filter_chain_factory_, createNetworkFilterChain(_, _)) + .WillOnce(Invoke([](Network::Connection& connection, + const std::vector& filter_factories) { + EXPECT_EQ(1u, filter_factories.size()); + Server::Configuration::FilterChainUtility::buildFilterChain(connection, filter_factories); + return true; + })); + EXPECT_CALL(*read_filter, onNewConnection()) + // Stop iteration to avoid calling getRead/WriteBuffer(). + .WillOnce(Return(Network::FilterStatus::StopIteration)); + + quic::QuicSocketAddress peer_addr(version_ == Network::Address::IpVersion::v4 + ? quic::QuicIpAddress::Loopback4() + : quic::QuicIpAddress::Loopback6(), + 54321); + // Set QuicDispatcher::new_sessions_allowed_per_event_loop_ to + // |kNumSessionsToCreatePerLoopForTests| so that received CHLOs can be + // processed immediately. + envoy_quic_dispatcher_.ProcessBufferedChlos(kNumSessionsToCreatePerLoopForTests); + + processValidChloPacket(peer_addr); + + EXPECT_CALL(network_connection_callbacks, onEvent(Network::ConnectionEvent::LocalClose)); + envoy_quic_dispatcher_.closeConnectionsWithFilterChain(&proof_source_->filterChain()); +} + } // namespace Quic } // namespace Envoy diff --git a/test/common/quic/envoy_quic_server_session_test.cc b/test/common/quic/envoy_quic_server_session_test.cc index f44b359cc2b78..6ffc8b2ff8038 100644 --- a/test/common/quic/envoy_quic_server_session_test.cc +++ b/test/common/quic/envoy_quic_server_session_test.cc @@ -166,8 +166,7 @@ class EnvoyQuicServerSessionTest : public testing::Test { &compressed_certs_cache_, *dispatcher_, /*send_buffer_limit*/ quic::kDefaultFlowControlSendWindow * 1.5, quic_stat_names_, listener_config_.listenerScope(), - crypto_stream_factory_, - makeOptRefFromPtr(nullptr)), + crypto_stream_factory_), stats_({ALL_HTTP3_CODEC_STATS( POOL_COUNTER_PREFIX(listener_config_.listenerScope(), "http3."), POOL_GAUGE_PREFIX(listener_config_.listenerScope(), "http3."))}) { diff --git a/test/common/quic/test_utils.h b/test/common/quic/test_utils.h index 3999d237c1ff6..104e36a5fb94d 100644 --- a/test/common/quic/test_utils.h +++ b/test/common/quic/test_utils.h @@ -164,13 +164,14 @@ class MockEnvoyQuicClientSession : public EnvoyQuicClientSession { const quic::ParsedQuicVersionVector& supported_versions, std::unique_ptr connection, Event::Dispatcher& dispatcher, uint32_t send_buffer_limit, - EnvoyQuicCryptoClientStreamFactoryInterface& crypto_stream_factory) + EnvoyQuicCryptoClientStreamFactoryInterface& crypto_stream_factory, + Network::TransportSocketFactory& transport_socket_factory) : EnvoyQuicClientSession(config, supported_versions, std::move(connection), quic::QuicServerId("example.com", 443, false), std::make_shared( quic::test::crypto_test_utils::ProofVerifierForTesting()), nullptr, dispatcher, send_buffer_limit, crypto_stream_factory, - quic_stat_names_, stats_store_) {} + quic_stat_names_, stats_store_, transport_socket_factory, "") {} void Initialize() override { EnvoyQuicClientSession::Initialize(); diff --git a/test/config/utility.cc b/test/config/utility.cc index 4c39561331fa1..9e8250ab51158 100644 --- a/test/config/utility.cc +++ b/test/config/utility.cc @@ -1103,11 +1103,18 @@ void ConfigHelper::addSslConfig(const ServerSslOptions& options) { filter_chain->mutable_transport_socket()->mutable_typed_config()->PackFrom(tls_context); } -void ConfigHelper::addQuicDownstreamTransportSocketConfig() { +void ConfigHelper::addQuicDownstreamTransportSocketConfig( + const std::vector& extra_alpns) { configDownstreamTransportSocketWithTls( bootstrap_, - [](envoy::extensions::transport_sockets::tls::v3::CommonTlsContext& common_tls_context) { + [&extra_alpns]( + envoy::extensions::transport_sockets::tls::v3::CommonTlsContext& common_tls_context) { initializeTls(ServerSslOptions().setRsaCert(true).setTlsV13(true), common_tls_context); + common_tls_context.add_alpn_protocols(Http::Utility::AlpnNames::get().Http3); + std::cerr << "============= add extra alpns " << extra_alpns.size(); + for (auto& alpn : extra_alpns) { + common_tls_context.add_alpn_protocols(alpn); + } }); } diff --git a/test/config/utility.h b/test/config/utility.h index 06865cc36518a..27e43aebac326 100644 --- a/test/config/utility.h +++ b/test/config/utility.h @@ -239,7 +239,7 @@ class ConfigHelper { void addSslConfig() { addSslConfig({}); } // Add the default SSL configuration for QUIC downstream. - void addQuicDownstreamTransportSocketConfig(); + void addQuicDownstreamTransportSocketConfig(const std::vector& extra_alpns = {}); // Set the HTTP access log for the first HCM (if present) to a given file. The default is // the platform's null device. diff --git a/test/integration/http_integration.cc b/test/integration/http_integration.cc index 0430602910755..634610e431130 100644 --- a/test/integration/http_integration.cc +++ b/test/integration/http_integration.cc @@ -232,7 +232,8 @@ Network::ClientConnectionPtr HttpIntegrationTest::makeClientConnectionWithOption Network::Address::InstanceConstSharedPtr local_addr = Network::Test::getCanonicalLoopbackAddress(version_); return Quic::createQuicNetworkConnection(*quic_connection_persistent_info_, *dispatcher_, - server_addr, local_addr, quic_stat_names_, stats_store_); + server_addr, local_addr, quic_stat_names_, stats_store_, + "", ""); #else ASSERT(false, "running a QUIC integration test without compiling QUIC"); return nullptr; diff --git a/test/integration/quic_http_integration_test.cc b/test/integration/quic_http_integration_test.cc index 5fe18fe47eadb..179e0581b3170 100644 --- a/test/integration/quic_http_integration_test.cc +++ b/test/integration/quic_http_integration_test.cc @@ -85,8 +85,13 @@ class QuicHttpIntegrationTest : public HttpIntegrationTest, Network::ClientConnectionPtr makeClientConnectionWithOptions( uint32_t port, const Network::ConnectionSocket::OptionsSharedPtr& options) override { - // Setting socket options is not supported. ASSERT(!options); + return makeClientConnectionWithHost(port, ""); + } + + Network::ClientConnectionPtr makeClientConnectionWithHost(uint32_t port, + const std::string& host) { + // Setting socket options is not supported. server_addr_ = Network::Utility::resolveUrl( fmt::format("udp://{}:{}", Network::Test::getLoopbackAddressUrlString(version_), port)); Network::Address::InstanceConstSharedPtr local_addr = @@ -101,14 +106,18 @@ class QuicHttpIntegrationTest : public HttpIntegrationTest, quic_connection_ = connection.get(); ASSERT(quic_connection_persistent_info_ != nullptr); auto& persistent_info = static_cast(*quic_connection_persistent_info_); + std::cerr << "======= persistent_info.host " << persistent_info.server_id_.host() + << "host is empty " << host.empty() << "\n"; auto session = std::make_unique( persistent_info.quic_config_, supported_versions_, std::move(connection), - persistent_info.server_id_, persistent_info.cryptoConfig(), &push_promise_index_, - *dispatcher_, + (host.empty() ? persistent_info.server_id_ + : quic::QuicServerId{host, static_cast(port), false}), + persistent_info.cryptoConfig(), &push_promise_index_, *dispatcher_, // Use smaller window than the default one to have test coverage of client codec buffer // exceeding high watermark. /*send_buffer_limit=*/2 * Http2::Utility::OptionsLimits::MIN_INITIAL_STREAM_WINDOW_SIZE, - persistent_info.crypto_stream_factory_, quic_stat_names_, stats_store_); + persistent_info.crypto_stream_factory_, quic_stat_names_, stats_store_, + persistent_info.transport_socket_factory_, ""); return session; } @@ -119,8 +128,6 @@ class QuicHttpIntegrationTest : public HttpIntegrationTest, HttpIntegrationTest::makeRawHttpConnection(std::move(conn), http2_options); if (!codec->disconnected()) { codec->setCodecClientCallbacks(client_codec_callback_); - EXPECT_EQ(transport_socket_factory_->clientContextConfig().serverNameIndication(), - codec->connection()->requestedServerName()); } return codec; } @@ -240,6 +247,8 @@ TEST_P(QuicHttpIntegrationTest, ZeroRtt) { initialize(); // Start the first connection. codec_client_ = makeHttpConnection(makeClientConnection((lookupPort("http")))); + EXPECT_EQ(transport_socket_factory_->clientContextConfig().serverNameIndication(), + codec_client_->connection()->requestedServerName()); // Send a complete request on the first connection. auto response1 = codec_client_->makeHeaderOnlyRequest(default_request_headers_); waitForNextUpstreamRequest(0); @@ -436,5 +445,191 @@ TEST_P(QuicHttpIntegrationTest, ResetRequestWithInvalidCharacter) { ASSERT_TRUE(response->waitForReset()); } +class QuicInplaceLdsIntegrationTest : public QuicHttpIntegrationTest { +public: + void inplaceInitialize(bool add_default_filter_chain = false) { + autonomous_upstream_ = true; + setUpstreamCount(2); + + config_helper_.addConfigModifier([add_default_filter_chain]( + envoy::config::bootstrap::v3::Bootstrap& bootstrap) { + auto* filter_chain_0 = + bootstrap.mutable_static_resources()->mutable_listeners(0)->mutable_filter_chains(0); + *filter_chain_0->mutable_filter_chain_match()->mutable_server_names()->Add() = "www.lyft.com"; + auto* filter_chain_1 = bootstrap.mutable_static_resources() + ->mutable_listeners(0) + ->mutable_filter_chains() + ->Add(); + filter_chain_1->MergeFrom(*filter_chain_0); + + // filter chain 1 route to cluster_1 + *filter_chain_1->mutable_filter_chain_match()->mutable_server_names(0) = "lyft.com"; + + filter_chain_0->set_name("filter_chain_0"); + filter_chain_1->set_name("filter_chain_1"); + + auto* config_blob = filter_chain_1->mutable_filters(0)->mutable_typed_config(); + + ASSERT_TRUE(config_blob->Is()); + auto hcm_config = MessageUtil::anyConvert< + envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager>( + *config_blob); + hcm_config.mutable_route_config() + ->mutable_virtual_hosts(0) + ->mutable_routes(0) + ->mutable_route() + ->set_cluster("cluster_1"); + config_blob->PackFrom(hcm_config); + bootstrap.mutable_static_resources()->mutable_clusters()->Add()->MergeFrom( + *bootstrap.mutable_static_resources()->mutable_clusters(0)); + bootstrap.mutable_static_resources()->mutable_clusters(1)->set_name("cluster_1"); + + if (add_default_filter_chain) { + auto default_filter_chain = bootstrap.mutable_static_resources() + ->mutable_listeners(0) + ->mutable_default_filter_chain(); + default_filter_chain->MergeFrom(*filter_chain_0); + default_filter_chain->set_name("filter_chain_default"); + } + }); + + QuicHttpIntegrationTest::initialize(); + } + + void makeRequestAndWaitForResponse(IntegrationCodecClient& codec_client) { + IntegrationStreamDecoderPtr response = + codec_client.makeHeaderOnlyRequest(default_request_headers_); + + ASSERT_TRUE(response->waitForEndStream()); + EXPECT_TRUE(response->complete()); + EXPECT_EQ("200", response->headers().getStatusValue()); + EXPECT_FALSE(codec_client.sawGoAway()); + } +}; + +INSTANTIATE_TEST_SUITE_P(QuicHttpIntegrationTests, QuicInplaceLdsIntegrationTest, + testing::ValuesIn(TestEnvironment::getIpVersionsForTest()), + TestUtility::ipTestParamsToString); + +TEST_P(QuicInplaceLdsIntegrationTest, ReloadConfigUpdateNonDefaultFilterChain) { + inplaceInitialize(/*add_default_filter_chain=*/false); + + auto codec_client_0 = + makeHttpConnection(makeClientConnectionWithHost(lookupPort("http"), "www.lyft.com")); + auto codec_client_1 = + makeHttpConnection(makeClientConnectionWithHost(lookupPort("http"), "lyft.com")); + + // Remove filter_chain_1. + 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_filter_chains()->RemoveLast(); + }); + + new_config_helper.setLds("1"); + test_server_->waitForCounterGe("listener_manager.listener_in_place_updated", 1); + test_server_->waitForGaugeGe("listener_manager.total_filter_chains_draining", 1); + test_server_->waitForGaugeEq("listener_manager.total_filter_chains_draining", 0); + makeRequestAndWaitForResponse(*codec_client_0); + EXPECT_TRUE(codec_client_1->sawGoAway()); + codec_client_1->close(); + + auto codec_client_2 = + makeHttpConnection(makeClientConnectionWithHost(lookupPort("http"), "www.lyft.com")); + makeRequestAndWaitForResponse(*codec_client_2); + codec_client_2->close(); + + // Update filter chain again to add back filter_chain_1. + config_helper_.setLds("1"); + test_server_->waitForCounterGe("listener_manager.listener_in_place_updated", 2); + test_server_->waitForCounterGe("listener_manager.listener_create_success", 3); + + auto codec_client_3 = + makeHttpConnection(makeClientConnectionWithHost(lookupPort("http"), "lyft.com")); + makeRequestAndWaitForResponse(*codec_client_3); + makeRequestAndWaitForResponse(*codec_client_0); + codec_client_0->close(); + codec_client_3->close(); +} + +// Verify that the connection received GO_AWAY after its filter chain gets deleted during the +// listener update. +TEST_P(QuicInplaceLdsIntegrationTest, ReloadConfigUpdateDefaultFilterChain) { + inplaceInitialize(/*add_default_filter_chain=*/true); + + auto codec_client_0 = + makeHttpConnection(makeClientConnectionWithHost(lookupPort("http"), "www.lyft.com")); + + // Remove filter_chain_1. + 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_filter_chains()->RemoveLast(); + }); + + new_config_helper.setLds("1"); + test_server_->waitForCounterGe("listener_manager.listener_in_place_updated", 1); + test_server_->waitForGaugeGe("listener_manager.total_filter_chains_draining", 1); + + test_server_->waitForGaugeEq("listener_manager.total_filter_chains_draining", 0); + // This connection should pick up the default filter chain. + auto codec_client_default = + makeHttpConnection(makeClientConnectionWithHost(lookupPort("http"), "lyft.com")); + makeRequestAndWaitForResponse(*codec_client_default); + makeRequestAndWaitForResponse(*codec_client_0); + + // Modify the default filter chain. + ConfigHelper new_config_helper1( + version_, *api_, MessageUtil::getJsonStringFromMessageOrDie(new_config_helper.bootstrap())); + new_config_helper1.addConfigModifier([&](envoy::config::bootstrap::v3::Bootstrap& bootstrap) + -> void { + auto default_filter_chain = + bootstrap.mutable_static_resources()->mutable_listeners(0)->mutable_default_filter_chain(); + default_filter_chain->set_name("default_filter_chain_v3"); + }); + + new_config_helper1.setLds("1"); + test_server_->waitForCounterGe("listener_manager.listener_in_place_updated", 2); + test_server_->waitForGaugeGe("listener_manager.total_filter_chains_draining", 1); + std::cerr << "=========== after rename default filter chain\n"; + test_server_->waitForGaugeEq("listener_manager.total_filter_chains_draining", 0); + + std::cerr << "=========== after default filter chain drained\n"; + makeRequestAndWaitForResponse(*codec_client_0); + EXPECT_TRUE(codec_client_default->sawGoAway()); + codec_client_default->close(); + + // This connection should pick up the new default filter chain. + auto codec_client_1 = + makeHttpConnection(makeClientConnectionWithHost(lookupPort("http"), "lyft.com")); + std::cerr << "=========== a new connection after renaming default filter chain\n"; + makeRequestAndWaitForResponse(*codec_client_1); + + // Remove the default filter chain. + ConfigHelper new_config_helper2( + version_, *api_, MessageUtil::getJsonStringFromMessageOrDie(new_config_helper1.bootstrap())); + new_config_helper2.addConfigModifier( + [&](envoy::config::bootstrap::v3::Bootstrap& bootstrap) -> void { + auto* listener = bootstrap.mutable_static_resources()->mutable_listeners(0); + listener->clear_default_filter_chain(); + }); + + new_config_helper2.setLds("1"); + test_server_->waitForCounterGe("listener_manager.listener_in_place_updated", 3); + test_server_->waitForGaugeGe("listener_manager.total_filter_chains_draining", 1); + std::cerr << "============= after removing default filter chain\n"; + test_server_->waitForGaugeEq("listener_manager.total_filter_chains_draining", 0); + + makeRequestAndWaitForResponse(*codec_client_0); + codec_client_0->close(); + EXPECT_TRUE(codec_client_1->sawGoAway()); + codec_client_1->close(); +} + } // namespace Quic } // namespace Envoy diff --git a/test/server/listener_manager_impl_test.cc b/test/server/listener_manager_impl_test.cc index 26fa91df69d59..89087a377fb20 100644 --- a/test/server/listener_manager_impl_test.cc +++ b/test/server/listener_manager_impl_test.cc @@ -5102,9 +5102,9 @@ TEST_F(ListenerManagerImplForInPlaceFilterChainUpdateTest, TraditionalUpdateIfWo EXPECT_EQ(0, server_.stats_store_.counter("listener_manager.listener_in_place_updated").value()); } -// This case also verifies that listeners that share port but do not share socket type (TCP vs. UDP) +// This case verifies that listeners that share port but do not share socket type (TCP vs. UDP) // do not share a listener. -TEST_F(ListenerManagerImplForInPlaceFilterChainUpdateTest, TraditionalUpdateIfAnyListenerIsNotTcp) { +TEST_F(ListenerManagerImplForInPlaceFilterChainUpdateTest, TraditionalUpdateIfDifferentSocketType) { EXPECT_CALL(*worker_, start(_, _)); manager_->startWorkers(guard_dog_, callback_.AsStdFunction()); From 6b469dc6a5b10fd5bb1429fca9b6b9e8894bed0e Mon Sep 17 00:00:00 2001 From: Dan Zhang Date: Fri, 3 Sep 2021 17:00:13 -0400 Subject: [PATCH 02/13] revert debug change Signed-off-by: Dan Zhang --- source/common/network/udp_listener_impl.cc | 3 +-- source/server/listener_manager_impl.cc | 2 -- test/integration/quic_http_integration_test.cc | 7 +------ 3 files changed, 2 insertions(+), 10 deletions(-) diff --git a/source/common/network/udp_listener_impl.cc b/source/common/network/udp_listener_impl.cc index f000236733cc4..d8fff8e1d4b1f 100644 --- a/source/common/network/udp_listener_impl.cc +++ b/source/common/network/udp_listener_impl.cc @@ -132,6 +132,7 @@ UdpListenerWorkerRouterImpl::UdpListenerWorkerRouterImpl(uint32_t concurrency) void UdpListenerWorkerRouterImpl::registerWorkerForListener(UdpListenerCallbacks& listener) { absl::WriterMutexLock lock(&mutex_); + ASSERT(listener.workerIndex() < workers_.size()); ASSERT(workers_.at(listener.workerIndex()) == nullptr); workers_.at(listener.workerIndex()) = &listener; @@ -139,8 +140,6 @@ void UdpListenerWorkerRouterImpl::registerWorkerForListener(UdpListenerCallbacks void UdpListenerWorkerRouterImpl::unregisterWorkerForListener(UdpListenerCallbacks& listener) { absl::WriterMutexLock lock(&mutex_); - ASSERT(workers_.at(listener.workerIndex()) != nullptr, - fmt::format("no listener is registered at {}", listener.workerIndex())); ASSERT(workers_.at(listener.workerIndex()) == &listener); workers_.at(listener.workerIndex()) = nullptr; } diff --git a/source/server/listener_manager_impl.cc b/source/server/listener_manager_impl.cc index 4478ff67f0eb1..afd6ebd142d6d 100644 --- a/source/server/listener_manager_impl.cc +++ b/source/server/listener_manager_impl.cc @@ -709,8 +709,6 @@ void ListenerManagerImpl::drainFilterChains(ListenerImplPtr&& draining_listener, // listener while filters might still be using its context (stats, etc.). server_.dispatcher().post([this, draining_group]() -> void { if (draining_group->decWorkersPendingRemoval() == 0) { - std::cerr << "============ Complete draining filter chain from listener " - << draining_group->getDrainingListener().name() << "\n"; draining_group->getDrainingListener().debugLog( absl::StrCat("draining filter chains from listener ", draining_group->getDrainingListener().name(), " complete")); diff --git a/test/integration/quic_http_integration_test.cc b/test/integration/quic_http_integration_test.cc index 179e0581b3170..fb35d77d02784 100644 --- a/test/integration/quic_http_integration_test.cc +++ b/test/integration/quic_http_integration_test.cc @@ -85,6 +85,7 @@ class QuicHttpIntegrationTest : public HttpIntegrationTest, Network::ClientConnectionPtr makeClientConnectionWithOptions( uint32_t port, const Network::ConnectionSocket::OptionsSharedPtr& options) override { + // Setting socket options is not supported. ASSERT(!options); return makeClientConnectionWithHost(port, ""); } @@ -106,8 +107,6 @@ class QuicHttpIntegrationTest : public HttpIntegrationTest, quic_connection_ = connection.get(); ASSERT(quic_connection_persistent_info_ != nullptr); auto& persistent_info = static_cast(*quic_connection_persistent_info_); - std::cerr << "======= persistent_info.host " << persistent_info.server_id_.host() - << "host is empty " << host.empty() << "\n"; auto session = std::make_unique( persistent_info.quic_config_, supported_versions_, std::move(connection), (host.empty() ? persistent_info.server_id_ @@ -596,10 +595,8 @@ TEST_P(QuicInplaceLdsIntegrationTest, ReloadConfigUpdateDefaultFilterChain) { new_config_helper1.setLds("1"); test_server_->waitForCounterGe("listener_manager.listener_in_place_updated", 2); test_server_->waitForGaugeGe("listener_manager.total_filter_chains_draining", 1); - std::cerr << "=========== after rename default filter chain\n"; test_server_->waitForGaugeEq("listener_manager.total_filter_chains_draining", 0); - std::cerr << "=========== after default filter chain drained\n"; makeRequestAndWaitForResponse(*codec_client_0); EXPECT_TRUE(codec_client_default->sawGoAway()); codec_client_default->close(); @@ -607,7 +604,6 @@ TEST_P(QuicInplaceLdsIntegrationTest, ReloadConfigUpdateDefaultFilterChain) { // This connection should pick up the new default filter chain. auto codec_client_1 = makeHttpConnection(makeClientConnectionWithHost(lookupPort("http"), "lyft.com")); - std::cerr << "=========== a new connection after renaming default filter chain\n"; makeRequestAndWaitForResponse(*codec_client_1); // Remove the default filter chain. @@ -622,7 +618,6 @@ TEST_P(QuicInplaceLdsIntegrationTest, ReloadConfigUpdateDefaultFilterChain) { new_config_helper2.setLds("1"); test_server_->waitForCounterGe("listener_manager.listener_in_place_updated", 3); test_server_->waitForGaugeGe("listener_manager.total_filter_chains_draining", 1); - std::cerr << "============= after removing default filter chain\n"; test_server_->waitForGaugeEq("listener_manager.total_filter_chains_draining", 0); makeRequestAndWaitForResponse(*codec_client_0); From 89d08705598c6cdcce3571c7e9acccfacae709ab Mon Sep 17 00:00:00 2001 From: Dan Zhang Date: Fri, 3 Sep 2021 17:36:26 -0400 Subject: [PATCH 03/13] remove udp_listener_impl change Signed-off-by: Dan Zhang --- source/common/network/udp_listener_impl.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/source/common/network/udp_listener_impl.cc b/source/common/network/udp_listener_impl.cc index 3770675f7f806..764da61e94025 100644 --- a/source/common/network/udp_listener_impl.cc +++ b/source/common/network/udp_listener_impl.cc @@ -140,6 +140,7 @@ void UdpListenerWorkerRouterImpl::registerWorkerForListener(UdpListenerCallbacks void UdpListenerWorkerRouterImpl::unregisterWorkerForListener(UdpListenerCallbacks& listener) { absl::WriterMutexLock lock(&mutex_); + ASSERT(workers_.at(listener.workerIndex()) == &listener); workers_.at(listener.workerIndex()) = nullptr; } From 6d9f9c22e8b782dbd002886ea3b24a54b374e6b0 Mon Sep 17 00:00:00 2001 From: Dan Zhang Date: Fri, 3 Sep 2021 17:41:53 -0400 Subject: [PATCH 04/13] remove unused includes Signed-off-by: Dan Zhang --- source/common/quic/envoy_quic_server_session.cc | 3 --- 1 file changed, 3 deletions(-) diff --git a/source/common/quic/envoy_quic_server_session.cc b/source/common/quic/envoy_quic_server_session.cc index b64db0c791630..f1565646c75d4 100644 --- a/source/common/quic/envoy_quic_server_session.cc +++ b/source/common/quic/envoy_quic_server_session.cc @@ -7,9 +7,6 @@ #include "source/common/quic/envoy_quic_proof_source.h" #include "source/common/quic/envoy_quic_server_stream.h" -#include "envoy_quic_server_connection.h" -#include "quic_transport_socket_factory.h" - namespace Envoy { namespace Quic { From 3bd1557e79d13ed1860b949cdc8e734c097489d1 Mon Sep 17 00:00:00 2001 From: Dan Zhang Date: Fri, 3 Sep 2021 17:48:31 -0400 Subject: [PATCH 05/13] remove debug log Signed-off-by: Dan Zhang --- source/extensions/transport_sockets/tls/context_impl.cc | 6 ------ source/server/listener_manager_impl.h | 6 +----- 2 files changed, 1 insertion(+), 11 deletions(-) diff --git a/source/extensions/transport_sockets/tls/context_impl.cc b/source/extensions/transport_sockets/tls/context_impl.cc index ec5b55a4b8e4a..f8870d4dddc88 100644 --- a/source/extensions/transport_sockets/tls/context_impl.cc +++ b/source/extensions/transport_sockets/tls/context_impl.cc @@ -333,7 +333,6 @@ ContextImpl::ContextImpl(Stats::Scope& scope, const Envoy::Ssl::ContextConfig& c for (auto& ctx : tls_contexts_) { SSL_CTX_set_options(ctx.ssl_ctx_.get(), SSL_OP_CIPHER_SERVER_PREFERENCE); } - std::cerr << "============= configured ALPNs " << config.alpnProtocols() << "\n"; parsed_alpn_protocols_ = parseAlpnProtocols(config.alpnProtocols()); @@ -396,11 +395,6 @@ ContextImpl::ContextImpl(Stats::Scope& scope, const Envoy::Ssl::ContextConfig& c int ServerContextImpl::alpnSelectCallback(const unsigned char** out, unsigned char* outlen, const unsigned char* in, unsigned int inlen) { - std::cerr << "========= alpnSelectCallback with peer selected ALPNs " - << std::string(reinterpret_cast(in), inlen) << " configured ALPNs " - << std::string(reinterpret_cast(parsed_alpn_protocols_.data()), - parsed_alpn_protocols_.size()) - << "\n"; // Currently this uses the standard selection algorithm in priority order. const uint8_t* alpn_data = parsed_alpn_protocols_.data(); size_t alpn_data_size = parsed_alpn_protocols_.size(); diff --git a/source/server/listener_manager_impl.h b/source/server/listener_manager_impl.h index b94fcdaf2de6b..ad85054671954 100644 --- a/source/server/listener_manager_impl.h +++ b/source/server/listener_manager_impl.h @@ -149,14 +149,10 @@ class DrainingFilterChainsManager { // Schedule listener destroy. void startDrainSequence(std::chrono::seconds drain_time, Event::Dispatcher& dispatcher, std::function completion) { - std::cerr << "=========== startDrainSequence with timeout " << drain_time.count() << "\n"; drain_sequence_completion_ = completion; ASSERT(!drain_timer_); - drain_timer_ = dispatcher.createTimer([this]() -> void { - std::cerr << "=========== drain_sequence_completion_\n"; - drain_sequence_completion_(); - }); + drain_timer_ = dispatcher.createTimer([this]() -> void { drain_sequence_completion_(); }); drain_timer_->enableTimer(drain_time); } From 8e5889bd7313995b633d4e9f8f896b49583725e4 Mon Sep 17 00:00:00 2001 From: Dan Zhang Date: Fri, 3 Sep 2021 18:16:39 -0400 Subject: [PATCH 06/13] remove apln plumbing Signed-off-by: Dan Zhang --- .../quic/client_connection_factory_impl.cc | 23 +++++++------------ .../quic/client_connection_factory_impl.h | 4 +--- .../common/quic/envoy_quic_client_session.cc | 22 ++---------------- .../common/quic/envoy_quic_client_session.h | 7 +----- .../common/quic/envoy_quic_server_session.cc | 22 ------------------ .../common/quic/envoy_quic_server_session.h | 3 --- .../quic/quic_transport_socket_factory.cc | 15 +++++------- .../quic/quic_transport_socket_factory.h | 18 ++++----------- test/common/quic/test_utils.h | 5 ++-- test/config/utility.cc | 11 ++------- test/config/utility.h | 2 +- test/integration/http_integration.cc | 3 +-- .../integration/quic_http_integration_test.cc | 3 +-- 13 files changed, 30 insertions(+), 108 deletions(-) diff --git a/source/common/quic/client_connection_factory_impl.cc b/source/common/quic/client_connection_factory_impl.cc index d2df3665c1727..00d86543436ff 100644 --- a/source/common/quic/client_connection_factory_impl.cc +++ b/source/common/quic/client_connection_factory_impl.cc @@ -33,9 +33,8 @@ std::shared_ptr PersistentQuicInfoImpl::cryptoConf if (context.get() != client_context_.get()) { client_context_ = context; client_config_ = std::make_shared( - std::make_unique(std::move(context)), + std::make_unique(getContext(transport_socket_factory_)), std::make_unique((time_source_))); - ASSERT(server_id_.host() == getConfig(transport_socket_factory_).serverNameIndication()); } // Return the latest client config. return client_config_; @@ -53,15 +52,14 @@ PersistentQuicInfoImpl::PersistentQuicInfoImpl( quiche::FlagRegistry::getInstance(); } -std::unique_ptr createQuicNetworkConnection( - Http::PersistentQuicInfo& info, Event::Dispatcher& dispatcher, - Network::Address::InstanceConstSharedPtr server_addr, - Network::Address::InstanceConstSharedPtr local_addr, QuicStatNames& quic_stat_names, - Stats::Scope& scope, const std::string& host_name_override, const std::string& alpn_override) { +std::unique_ptr +createQuicNetworkConnection(Http::PersistentQuicInfo& info, Event::Dispatcher& dispatcher, + Network::Address::InstanceConstSharedPtr server_addr, + Network::Address::InstanceConstSharedPtr local_addr, + QuicStatNames& quic_stat_names, Stats::Scope& scope) { // This flag fix a QUICHE issue which may crash Envoy during connection close. SetQuicReloadableFlag(quic_single_ack_in_packet2, true); PersistentQuicInfoImpl* info_impl = reinterpret_cast(&info); - ASSERT(info_impl); auto config = info_impl->cryptoConfig(); if (config == nullptr) { return nullptr; // no secrets available yet. @@ -74,14 +72,9 @@ std::unique_ptr createQuicNetworkConnection( // QUICHE client session always use the 1st version to start handshake. auto ret = std::make_unique( - info_impl->quic_config_, quic_versions, std::move(connection), - (host_name_override.empty() - ? info_impl->server_id_ - : quic::QuicServerId{host_name_override, - static_cast(server_addr->ip()->port()), false}), + info_impl->quic_config_, quic_versions, std::move(connection), info_impl->server_id_, std::move(config), &info_impl->push_promise_index_, dispatcher, info_impl->buffer_limit_, - info_impl->crypto_stream_factory_, quic_stat_names, scope, - info_impl->transport_socket_factory_, alpn_override); + info_impl->crypto_stream_factory_, quic_stat_names, scope); return ret; } diff --git a/source/common/quic/client_connection_factory_impl.h b/source/common/quic/client_connection_factory_impl.h index f310d4d1fa972..acc1d0cb244c2 100644 --- a/source/common/quic/client_connection_factory_impl.h +++ b/source/common/quic/client_connection_factory_impl.h @@ -54,9 +54,7 @@ std::unique_ptr createQuicNetworkConnection(Http::PersistentQuicInfo& info, Event::Dispatcher& dispatcher, Network::Address::InstanceConstSharedPtr server_addr, Network::Address::InstanceConstSharedPtr local_addr, - QuicStatNames& quic_stat_names, Stats::Scope& scope, - const std::string& host_name_override = "", - const std::string& alpn_override = ""); + QuicStatNames& quic_stat_names, Stats::Scope& scope); } // namespace Quic } // namespace Envoy diff --git a/source/common/quic/envoy_quic_client_session.cc b/source/common/quic/envoy_quic_client_session.cc index 87b4b9b1585b4..df9fd5f7a6755 100644 --- a/source/common/quic/envoy_quic_client_session.cc +++ b/source/common/quic/envoy_quic_client_session.cc @@ -1,7 +1,6 @@ #include "source/common/quic/envoy_quic_client_session.h" #include "source/common/quic/envoy_quic_utils.h" -#include "source/common/quic/quic_transport_socket_factory.h" namespace Envoy { namespace Quic { @@ -12,17 +11,14 @@ EnvoyQuicClientSession::EnvoyQuicClientSession( std::shared_ptr crypto_config, quic::QuicClientPushPromiseIndex* push_promise_index, Event::Dispatcher& dispatcher, uint32_t send_buffer_limit, EnvoyQuicCryptoClientStreamFactoryInterface& crypto_stream_factory, - QuicStatNames& quic_stat_names, Stats::Scope& scope, - const Network::TransportSocketFactory& transport_socket_factory, - const std::string& alpn_override) + QuicStatNames& quic_stat_names, Stats::Scope& scope) : QuicFilterManagerConnectionImpl(*connection, connection->connection_id(), dispatcher, send_buffer_limit), quic::QuicSpdyClientSession(config, supported_versions, connection.release(), server_id, crypto_config.get(), push_promise_index), host_name_(server_id.host()), crypto_config_(crypto_config), crypto_stream_factory_(crypto_stream_factory), quic_stat_names_(quic_stat_names), - scope_(scope), transport_socket_factory_(transport_socket_factory), - alpn_override_(alpn_override) {} + scope_(scope) {} EnvoyQuicClientSession::~EnvoyQuicClientSession() { ASSERT(!connection()->connected()); @@ -126,19 +122,5 @@ std::unique_ptr EnvoyQuicClientSession::Create this, /*has_application_state = */ version().UsesHttp3()); } -std::vector EnvoyQuicClientSession::GetAlpnsToOffer() const { - if (!alpn_override_.empty()) { - return {alpn_override_}; - } - const std::vector& alpns_configured = - dynamic_cast(transport_socket_factory_) - .alpnsConfigured(); - if (!alpns_configured.empty()) { - return alpns_configured; - } - // Offer the default ALPN which is "h3". - return quic::QuicSpdyClientSession::GetAlpnsToOffer(); -} - } // namespace Quic } // namespace Envoy diff --git a/source/common/quic/envoy_quic_client_session.h b/source/common/quic/envoy_quic_client_session.h index c752c779bb8dc..a4d650166393f 100644 --- a/source/common/quic/envoy_quic_client_session.h +++ b/source/common/quic/envoy_quic_client_session.h @@ -40,9 +40,7 @@ class EnvoyQuicClientSession : public QuicFilterManagerConnectionImpl, quic::QuicClientPushPromiseIndex* push_promise_index, Event::Dispatcher& dispatcher, uint32_t send_buffer_limit, EnvoyQuicCryptoClientStreamFactoryInterface& crypto_stream_factory, - QuicStatNames& quic_stat_names, Stats::Scope& scope, - const Network::TransportSocketFactory& transport_socket_factory, - const std::string& alpn_override); + QuicStatNames& quic_stat_names, Stats::Scope& scope); ~EnvoyQuicClientSession() override; @@ -80,7 +78,6 @@ class EnvoyQuicClientSession : public QuicFilterManagerConnectionImpl, // active stream. return std::max(1, GetNumActiveStreams()) * Network::NUM_DATAGRAMS_PER_RECEIVE; } - std::vector GetAlpnsToOffer() const override; using quic::QuicSpdyClientSession::PerformActionOnActiveStreams; @@ -108,8 +105,6 @@ class EnvoyQuicClientSession : public QuicFilterManagerConnectionImpl, EnvoyQuicCryptoClientStreamFactoryInterface& crypto_stream_factory_; QuicStatNames& quic_stat_names_; Stats::Scope& scope_; - const Network::TransportSocketFactory& transport_socket_factory_; - std::string alpn_override_; }; } // namespace Quic diff --git a/source/common/quic/envoy_quic_server_session.cc b/source/common/quic/envoy_quic_server_session.cc index f1565646c75d4..fc3fe9b2b938f 100644 --- a/source/common/quic/envoy_quic_server_session.cc +++ b/source/common/quic/envoy_quic_server_session.cc @@ -152,27 +152,5 @@ void EnvoyQuicServerSession::storeConnectionMapPosition(FilterChainToConnectionM position_.emplace(connection_map, filter_chain, position); } -std::vector::const_iterator -EnvoyQuicServerSession::SelectAlpn(const std::vector& alpns) const { - ASSERT(position_.has_value()); - const std::vector& alpns_configured = - dynamic_cast( - position_->filter_chain_.transportSocketFactory()) - .alpnsConfigured(); - if (alpns_configured.empty()) { - // If the server transport socket doesn't specify supported ALPNs, select the first one provided - // by the peer. - return alpns.begin(); - } - // Otherwise select the first one mutually supported. - for (auto iter = alpns.begin(); iter != alpns.end(); ++iter) { - if (std::find(alpns_configured.begin(), alpns_configured.end(), *iter) != - alpns_configured.end()) { - return iter; - } - } - return alpns.end(); -} - } // namespace Quic } // namespace Envoy diff --git a/source/common/quic/envoy_quic_server_session.h b/source/common/quic/envoy_quic_server_session.h index b2c8f7cd62e48..66182fc78e302 100644 --- a/source/common/quic/envoy_quic_server_session.h +++ b/source/common/quic/envoy_quic_server_session.h @@ -95,9 +95,6 @@ class EnvoyQuicServerSession : public quic::QuicServerSessionBase, const Network::FilterChain& filter_chain, ConnectionMapIter position); - std::vector::const_iterator - SelectAlpn(const std::vector& alpns) const override; - using quic::QuicSession::PerformActionOnActiveStreams; protected: diff --git a/source/common/quic/quic_transport_socket_factory.cc b/source/common/quic/quic_transport_socket_factory.cc index 8c2a0b6eab719..452b4f45d7c66 100644 --- a/source/common/quic/quic_transport_socket_factory.cc +++ b/source/common/quic/quic_transport_socket_factory.cc @@ -16,9 +16,8 @@ QuicServerTransportSocketConfigFactory::createTransportSocketFactory( config, context.messageValidationVisitor()); auto server_config = std::make_unique( quic_transport.downstream_tls_context(), context); - auto factory = std::make_unique( - context.scope(), std::move(server_config), - quic_transport.downstream_tls_context().common_tls_context().alpn_protocols()); + auto factory = + std::make_unique(context.scope(), std::move(server_config)); factory->initialize(); return factory; } @@ -37,18 +36,16 @@ QuicClientTransportSocketConfigFactory::createTransportSocketFactory( config, context.messageValidationVisitor()); auto client_config = std::make_unique( quic_transport.upstream_tls_context(), context); - auto factory = std::make_unique( - std::move(client_config), context, - quic_transport.upstream_tls_context().common_tls_context().alpn_protocols()); + auto factory = + std::make_unique(std::move(client_config), context); factory->initialize(); return factory; } QuicClientTransportSocketFactory::QuicClientTransportSocketFactory( Ssl::ClientContextConfigPtr config, - Server::Configuration::TransportSocketFactoryContext& factory_context, - const Protobuf::RepeatedPtrField& alpns) - : QuicTransportSocketFactoryBase(factory_context.scope(), "client", alpns), + Server::Configuration::TransportSocketFactoryContext& factory_context) + : QuicTransportSocketFactoryBase(factory_context.scope(), "client"), fallback_factory_(std::make_unique( std::move(config), factory_context.sslContextManager(), factory_context.scope())) {} diff --git a/source/common/quic/quic_transport_socket_factory.h b/source/common/quic/quic_transport_socket_factory.h index 4793dfc35e1f1..7c98e1ef29e69 100644 --- a/source/common/quic/quic_transport_socket_factory.h +++ b/source/common/quic/quic_transport_socket_factory.h @@ -37,12 +37,8 @@ QuicTransportSocketFactoryStats generateStats(Stats::Scope& store, const std::st class QuicTransportSocketFactoryBase : public Network::TransportSocketFactory, protected Logger::Loggable { public: - QuicTransportSocketFactoryBase(Stats::Scope& store, const std::string& perspective, - const Protobuf::RepeatedPtrField& alpns) - : stats_(generateStats(store, perspective)) { - for_each(alpns.begin(), alpns.end(), - [this](const std::string& alpn) { alpns_.push_back(alpn); }); - } + QuicTransportSocketFactoryBase(Stats::Scope& store, const std::string& perspective) + : stats_(generateStats(store, perspective)) {} // To be called right after construction. virtual void initialize() PURE; @@ -55,21 +51,18 @@ class QuicTransportSocketFactoryBase : public Network::TransportSocketFactory, bool implementsSecureTransport() const override { return true; } bool usesProxyProtocolOptions() const override { return false; } bool supportsAlpn() const override { return true; } - const std::vector& alpnsConfigured() const { return alpns_; } protected: virtual void onSecretUpdated() PURE; QuicTransportSocketFactoryStats stats_; - std::vector alpns_; }; // TODO(danzh): when implement ProofSource, examine of it's necessary to // differentiate server and client side context config. class QuicServerTransportSocketFactory : public QuicTransportSocketFactoryBase { public: - QuicServerTransportSocketFactory(Stats::Scope& store, Ssl::ServerContextConfigPtr config, - const Protobuf::RepeatedPtrField& alpns) - : QuicTransportSocketFactoryBase(store, "server", alpns), config_(std::move(config)) {} + QuicServerTransportSocketFactory(Stats::Scope& store, Ssl::ServerContextConfigPtr config) + : QuicTransportSocketFactoryBase(store, "server"), config_(std::move(config)) {} void initialize() override { config_->setSecretUpdateCallback([this]() { @@ -100,8 +93,7 @@ class QuicClientTransportSocketFactory : public QuicTransportSocketFactoryBase { public: QuicClientTransportSocketFactory( Ssl::ClientContextConfigPtr config, - Server::Configuration::TransportSocketFactoryContext& factory_context, - const Protobuf::RepeatedPtrField& alpns); + Server::Configuration::TransportSocketFactoryContext& factory_context); void initialize() override {} diff --git a/test/common/quic/test_utils.h b/test/common/quic/test_utils.h index 104e36a5fb94d..3999d237c1ff6 100644 --- a/test/common/quic/test_utils.h +++ b/test/common/quic/test_utils.h @@ -164,14 +164,13 @@ class MockEnvoyQuicClientSession : public EnvoyQuicClientSession { const quic::ParsedQuicVersionVector& supported_versions, std::unique_ptr connection, Event::Dispatcher& dispatcher, uint32_t send_buffer_limit, - EnvoyQuicCryptoClientStreamFactoryInterface& crypto_stream_factory, - Network::TransportSocketFactory& transport_socket_factory) + EnvoyQuicCryptoClientStreamFactoryInterface& crypto_stream_factory) : EnvoyQuicClientSession(config, supported_versions, std::move(connection), quic::QuicServerId("example.com", 443, false), std::make_shared( quic::test::crypto_test_utils::ProofVerifierForTesting()), nullptr, dispatcher, send_buffer_limit, crypto_stream_factory, - quic_stat_names_, stats_store_, transport_socket_factory, "") {} + quic_stat_names_, stats_store_) {} void Initialize() override { EnvoyQuicClientSession::Initialize(); diff --git a/test/config/utility.cc b/test/config/utility.cc index 9e8250ab51158..4c39561331fa1 100644 --- a/test/config/utility.cc +++ b/test/config/utility.cc @@ -1103,18 +1103,11 @@ void ConfigHelper::addSslConfig(const ServerSslOptions& options) { filter_chain->mutable_transport_socket()->mutable_typed_config()->PackFrom(tls_context); } -void ConfigHelper::addQuicDownstreamTransportSocketConfig( - const std::vector& extra_alpns) { +void ConfigHelper::addQuicDownstreamTransportSocketConfig() { configDownstreamTransportSocketWithTls( bootstrap_, - [&extra_alpns]( - envoy::extensions::transport_sockets::tls::v3::CommonTlsContext& common_tls_context) { + [](envoy::extensions::transport_sockets::tls::v3::CommonTlsContext& common_tls_context) { initializeTls(ServerSslOptions().setRsaCert(true).setTlsV13(true), common_tls_context); - common_tls_context.add_alpn_protocols(Http::Utility::AlpnNames::get().Http3); - std::cerr << "============= add extra alpns " << extra_alpns.size(); - for (auto& alpn : extra_alpns) { - common_tls_context.add_alpn_protocols(alpn); - } }); } diff --git a/test/config/utility.h b/test/config/utility.h index 27e43aebac326..06865cc36518a 100644 --- a/test/config/utility.h +++ b/test/config/utility.h @@ -239,7 +239,7 @@ class ConfigHelper { void addSslConfig() { addSslConfig({}); } // Add the default SSL configuration for QUIC downstream. - void addQuicDownstreamTransportSocketConfig(const std::vector& extra_alpns = {}); + void addQuicDownstreamTransportSocketConfig(); // Set the HTTP access log for the first HCM (if present) to a given file. The default is // the platform's null device. diff --git a/test/integration/http_integration.cc b/test/integration/http_integration.cc index 149932c20d5ca..3cb411a0c730a 100644 --- a/test/integration/http_integration.cc +++ b/test/integration/http_integration.cc @@ -232,8 +232,7 @@ Network::ClientConnectionPtr HttpIntegrationTest::makeClientConnectionWithOption Network::Address::InstanceConstSharedPtr local_addr = Network::Test::getCanonicalLoopbackAddress(version_); return Quic::createQuicNetworkConnection(*quic_connection_persistent_info_, *dispatcher_, - server_addr, local_addr, quic_stat_names_, stats_store_, - "", ""); + server_addr, local_addr, quic_stat_names_, stats_store_); #else ASSERT(false, "running a QUIC integration test without compiling QUIC"); return nullptr; diff --git a/test/integration/quic_http_integration_test.cc b/test/integration/quic_http_integration_test.cc index fb35d77d02784..13d2741bdb4f7 100644 --- a/test/integration/quic_http_integration_test.cc +++ b/test/integration/quic_http_integration_test.cc @@ -115,8 +115,7 @@ class QuicHttpIntegrationTest : public HttpIntegrationTest, // Use smaller window than the default one to have test coverage of client codec buffer // exceeding high watermark. /*send_buffer_limit=*/2 * Http2::Utility::OptionsLimits::MIN_INITIAL_STREAM_WINDOW_SIZE, - persistent_info.crypto_stream_factory_, quic_stat_names_, stats_store_, - persistent_info.transport_socket_factory_, ""); + persistent_info.crypto_stream_factory_, quic_stat_names_, stats_store_); return session; } From 164867f33244be654fb601423b66c57bd1c31364 Mon Sep 17 00:00:00 2001 From: Dan Zhang Date: Tue, 7 Sep 2021 13:18:27 -0400 Subject: [PATCH 07/13] fix tests Signed-off-by: Dan Zhang --- docs/root/version_history/current.rst | 1 + source/common/runtime/runtime_features.cc | 1 + source/server/connection_handler_impl.cc | 15 +++++++++++++- source/server/listener_impl.cc | 20 ++++++++++++++----- source/server/listener_impl.h | 3 +-- test/common/quic/active_quic_listener_test.cc | 6 +++--- test/server/connection_handler_test.cc | 2 +- 7 files changed, 36 insertions(+), 12 deletions(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index a13ea3b633676..f138d442192df 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -105,6 +105,7 @@ New Features * route config: added :ref:`dynamic_metadata ` for routing based on dynamic metadata. * sxg_filter: added filter to transform response to SXG package to :ref:`contrib images `. This can be enabled by setting :ref:`SXG ` configuration. * thrift_proxy: added support for :ref:`mirroring requests `. +* udp: allows updating filter chain in-place through LDS, which is supported by Quic listener. This will be No-op with error log in other UDP listener implementations. It can be reverted by ``envoy.reloadable_features.udp_listener_updates_filter_chain_in_place``. Deprecated ---------- diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index cf0aeb7ae4807..18fafb77f4781 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -90,6 +90,7 @@ constexpr const char* runtime_features[] = { "envoy.reloadable_features.strip_port_from_connect", "envoy.reloadable_features.treat_host_like_authority", "envoy.reloadable_features.treat_upstream_connect_timeout_as_connect_failure", + "envoy.reloadable_features.udp_listener_updates_filter_chain_in_place", "envoy.reloadable_features.udp_per_event_loop_read_limit", "envoy.reloadable_features.unquote_log_string_values", "envoy.reloadable_features.upstream_host_weight_change_causes_rebuild", diff --git a/source/server/connection_handler_impl.cc b/source/server/connection_handler_impl.cc index b218ae962c774..b7916008a1f6a 100644 --- a/source/server/connection_handler_impl.cc +++ b/source/server/connection_handler_impl.cc @@ -27,7 +27,9 @@ void ConnectionHandlerImpl::decNumConnections() { void ConnectionHandlerImpl::addListener(absl::optional overridden_listener, Network::ListenerConfig& config) { - if (overridden_listener.has_value()) { + if (Runtime::runtimeFeatureEnabled( + "envoy.reloadable_features.udp_listener_updates_filter_chain_in_place") && + overridden_listener.has_value()) { ActiveListenerDetailsOptRef listener_detail = findActiveListenerByTag(overridden_listener.value()); ASSERT(listener_detail.has_value()); @@ -37,6 +39,17 @@ void ConnectionHandlerImpl::addListener(absl::optional overridden_list ActiveListenerDetails details; if (config.listenSocketFactory().socketType() == Network::Socket::Type::Stream) { + if (!Runtime::runtimeFeatureEnabled( + "envoy.reloadable_features.udp_listener_updates_filter_chain_in_place") && + overridden_listener.has_value()) { + for (auto& listener : listeners_) { + if (listener.second.listener_->listenerTag() == overridden_listener) { + listener.second.tcpListener()->get().updateListenerConfig(config); + return; + } + } + NOT_REACHED_GCOVR_EXCL_LINE; + } // worker_index_ doesn't have a value on the main thread for the admin server. auto tcp_listener = std::make_unique( *this, config, worker_index_.has_value() ? *worker_index_ : 0); diff --git a/source/server/listener_impl.cc b/source/server/listener_impl.cc index c6889c2f8eb48..45f6e96db6a51 100644 --- a/source/server/listener_impl.cc +++ b/source/server/listener_impl.cc @@ -371,7 +371,7 @@ ListenerImpl::ListenerImpl(ListenerImpl& origin, const envoy::config::listener::v3::Listener& config, const std::string& version_info, ListenerManagerImpl& parent, const std::string& name, bool added_via_api, bool workers_started, - uint64_t hash, uint32_t /*concurrency*/) + uint64_t hash) : parent_(parent), address_(origin.address_), bind_to_port_(shouldBindToPort(config)), hand_off_restored_destination_connections_( PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, use_original_dst, false)), @@ -768,6 +768,16 @@ bool ListenerImpl::supportUpdateFilterChain(const envoy::config::listener::v3::L return false; } + ; + if (!Runtime::runtimeFeatureEnabled( + "envoy.reloadable_features.udp_listener_updates_filter_chain_in_place") && + (Network::Utility::protobufAddressSocketType(config_.address()) != + Network::Socket::Type::Stream || + Network::Utility::protobufAddressSocketType(config.address()) != + Network::Socket::Type::Stream)) { + return false; + } + // Full listener update currently rejects tcp listener having 0 filter chain. // In place filter chain update could survive under zero filter chain but we should keep the same // behavior for now. This also guards the below filter chain access. @@ -786,10 +796,10 @@ ListenerImplPtr ListenerImpl::newListenerWithFilterChain(const envoy::config::listener::v3::Listener& config, bool workers_started, uint64_t hash) { // Use WrapUnique since the constructor is private. - return absl::WrapUnique( - new ListenerImpl(*this, config, version_info_, parent_, name_, added_via_api_, - /* new new workers started state */ workers_started, - /* use new hash */ hash, parent_.server_.options().concurrency())); + return absl::WrapUnique(new ListenerImpl(*this, config, version_info_, parent_, name_, + added_via_api_, + /* new new workers started state */ workers_started, + /* use new hash */ hash)); } void ListenerImpl::diffFilterChain(const ListenerImpl& another_listener, diff --git a/source/server/listener_impl.h b/source/server/listener_impl.h index 3222d198f14c0..83ed5d562f2a6 100644 --- a/source/server/listener_impl.h +++ b/source/server/listener_impl.h @@ -365,8 +365,7 @@ class ListenerImpl final : public Network::ListenerConfig, */ ListenerImpl(ListenerImpl& origin, const envoy::config::listener::v3::Listener& config, const std::string& version_info, ListenerManagerImpl& parent, - const std::string& name, bool added_via_api, bool workers_started, uint64_t hash, - uint32_t concurrency); + const std::string& name, bool added_via_api, bool workers_started, uint64_t hash); // Helpers for constructor. void buildAccessLog(); void buildUdpListenerFactory(Network::Socket::Type socket_type, uint32_t concurrency); diff --git a/test/common/quic/active_quic_listener_test.cc b/test/common/quic/active_quic_listener_test.cc index 06b49bd5cac6a..9f92852df8769 100644 --- a/test/common/quic/active_quic_listener_test.cc +++ b/test/common/quic/active_quic_listener_test.cc @@ -190,12 +190,12 @@ class ActiveQuicListenerTest : public testing::TestWithParamsocket_factory_, socketType()).WillOnce(Return(socket_type)); if (listener == nullptr) { // Expecting listener config in place update. // If so, dispatcher would not create new network listener. return listeners_.back().get(); } + EXPECT_CALL(listeners_.back()->socket_factory_, socketType()).WillOnce(Return(socket_type)); EXPECT_CALL(listeners_.back()->socket_factory_, getListenSocket(_)) .WillOnce(Return(listeners_.back()->socket_)); if (socket_type == Network::Socket::Type::Stream) { From 6bb5415c9327f1df8a420615f28df8fcd95293fe Mon Sep 17 00:00:00 2001 From: Dan Zhang Date: Tue, 7 Sep 2021 19:22:05 -0400 Subject: [PATCH 08/13] fix proof source test Signed-off-by: Dan Zhang --- test/common/quic/envoy_quic_proof_source_test.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/test/common/quic/envoy_quic_proof_source_test.cc b/test/common/quic/envoy_quic_proof_source_test.cc index 75230db0f504f..db54a3f07deaa 100644 --- a/test/common/quic/envoy_quic_proof_source_test.cc +++ b/test/common/quic/envoy_quic_proof_source_test.cc @@ -144,6 +144,7 @@ class EnvoyQuicProofSourceTest : public ::testing::Test { listener_config_.listenerScope(), std::unique_ptr(mock_context_config_)); transport_socket_factory_->initialize(); + EXPECT_CALL(filter_chain_, name()).WillRepeatedly(Return("")); } void expectCertChainAndPrivateKey(const std::string& cert, bool expect_private_key) { From eb7c29c6061beca50e4cf5041f6c8c95d1de20d7 Mon Sep 17 00:00:00 2001 From: Dan Zhang Date: Thu, 9 Sep 2021 16:56:03 -0400 Subject: [PATCH 09/13] address comments Signed-off-by: Dan Zhang --- source/common/quic/active_quic_listener.cc | 7 +++---- source/common/quic/envoy_quic_dispatcher.cc | 11 ++++------- 2 files changed, 7 insertions(+), 11 deletions(-) diff --git a/source/common/quic/active_quic_listener.cc b/source/common/quic/active_quic_listener.cc index 16051ac8afef9..304ff8bc7d34f 100644 --- a/source/common/quic/active_quic_listener.cc +++ b/source/common/quic/active_quic_listener.cc @@ -230,10 +230,9 @@ void ActiveQuicListener::updateListenerConfig(Network::ListenerConfig& config) { void ActiveQuicListener::onFilterChainDraining( const std::list& draining_filter_chains) { - std::for_each(draining_filter_chains.begin(), draining_filter_chains.end(), - [this](const Network::FilterChain* filter_chain) { - closeConnectionsWithFilterChain(filter_chain); - }); + for (auto* filter_chain : draining_filter_chains) { + closeConnectionsWithFilterChain(filter_chain); + } } void ActiveQuicListener::closeConnectionsWithFilterChain(const Network::FilterChain* filter_chain) { diff --git a/source/common/quic/envoy_quic_dispatcher.cc b/source/common/quic/envoy_quic_dispatcher.cc index 03dfd38bf4879..2f6f78d1b5324 100644 --- a/source/common/quic/envoy_quic_dispatcher.cc +++ b/source/common/quic/envoy_quic_dispatcher.cc @@ -110,13 +110,10 @@ quic::QuicConnectionId EnvoyQuicDispatcher::ReplaceLongServerConnectionId( void EnvoyQuicDispatcher::closeConnectionsWithFilterChain( const Network::FilterChain* filter_chain) { - if (connections_by_filter_chain_.find(filter_chain) != connections_by_filter_chain_.end()) { - std::list>& connections = - connections_by_filter_chain_[filter_chain]; - // Retain the number of connections in the list early because closing the connection will change - // the size. - size_t num_connections = connections.size(); - for (size_t i = 0; i < num_connections; ++i) { + auto iter = connections_by_filter_chain_.find(filter_chain); + if (iter != connections_by_filter_chain_.end()) { + std::list>& connections = iter->second; + while (!connections.empty()) { Network::Connection& connection = connections.front().get(); // This will remove the connection from the list. connection.close(Network::ConnectionCloseType::NoFlush); From 75dd2c212fc39759ef56ab7052a0d19876cfcd37 Mon Sep 17 00:00:00 2001 From: Dan Zhang Date: Mon, 13 Sep 2021 13:08:40 -0400 Subject: [PATCH 10/13] fix asan Signed-off-by: Dan Zhang --- source/common/quic/envoy_quic_dispatcher.cc | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/source/common/quic/envoy_quic_dispatcher.cc b/source/common/quic/envoy_quic_dispatcher.cc index 2f6f78d1b5324..37fd7472c9a6a 100644 --- a/source/common/quic/envoy_quic_dispatcher.cc +++ b/source/common/quic/envoy_quic_dispatcher.cc @@ -113,9 +113,13 @@ void EnvoyQuicDispatcher::closeConnectionsWithFilterChain( auto iter = connections_by_filter_chain_.find(filter_chain); if (iter != connections_by_filter_chain_.end()) { std::list>& connections = iter->second; - while (!connections.empty()) { + // Retain the number of connections in the list early because closing the connection will change + // the size. + const size_t num_connections = connections.size(); + for (size_t i = 0; i < num_connections; ++i) { Network::Connection& connection = connections.front().get(); - // This will remove the connection from the list. + // This will remove the connection from the list. And the last removal will remove connections + // from the map as well. connection.close(Network::ConnectionCloseType::NoFlush); } ASSERT(connections_by_filter_chain_.find(filter_chain) == connections_by_filter_chain_.end()); From 897aab779c55e3d944f08367067178a25e53ff0b Mon Sep 17 00:00:00 2001 From: Dan Zhang Date: Thu, 16 Sep 2021 17:51:01 -0400 Subject: [PATCH 11/13] reject raw udp listener with filter chain Signed-off-by: Dan Zhang --- source/server/active_udp_listener.h | 6 ++--- source/server/connection_handler_impl.cc | 10 +++---- source/server/listener_impl.cc | 8 +++++- test/server/listener_manager_impl_test.cc | 32 +++++++++++++++++------ 4 files changed, 37 insertions(+), 19 deletions(-) diff --git a/source/server/active_udp_listener.h b/source/server/active_udp_listener.h index c8ccffb450297..3d73ef1eda076 100644 --- a/source/server/active_udp_listener.h +++ b/source/server/active_udp_listener.h @@ -103,11 +103,9 @@ class ActiveRawUdpListener : public ActiveUdpListenerBase, read_filter_.reset(); udp_listener_.reset(); } - void updateListenerConfig(Network::ListenerConfig&) override { - ENVOY_BUG(false, "In-place filter chain update observed on raw UDP listener."); - } + void updateListenerConfig(Network::ListenerConfig&) override { NOT_REACHED_GCOVR_EXCL_LINE; } void onFilterChainDraining(const std::list&) override { - ENVOY_BUG(false, "Filter chain draining observed on raw UDP listener."); + NOT_REACHED_GCOVR_EXCL_LINE; } // Network::UdpListenerFilterManager diff --git a/source/server/connection_handler_impl.cc b/source/server/connection_handler_impl.cc index b7916008a1f6a..37abdcc8fa1b0 100644 --- a/source/server/connection_handler_impl.cc +++ b/source/server/connection_handler_impl.cc @@ -27,9 +27,9 @@ void ConnectionHandlerImpl::decNumConnections() { void ConnectionHandlerImpl::addListener(absl::optional overridden_listener, Network::ListenerConfig& config) { - if (Runtime::runtimeFeatureEnabled( - "envoy.reloadable_features.udp_listener_updates_filter_chain_in_place") && - overridden_listener.has_value()) { + bool support_udp_in_place_filter_chain_update = Runtime::runtimeFeatureEnabled( + "envoy.reloadable_features.udp_listener_updates_filter_chain_in_place"); + if (support_udp_in_place_filter_chain_update && overridden_listener.has_value()) { ActiveListenerDetailsOptRef listener_detail = findActiveListenerByTag(overridden_listener.value()); ASSERT(listener_detail.has_value()); @@ -39,9 +39,7 @@ void ConnectionHandlerImpl::addListener(absl::optional overridden_list ActiveListenerDetails details; if (config.listenSocketFactory().socketType() == Network::Socket::Type::Stream) { - if (!Runtime::runtimeFeatureEnabled( - "envoy.reloadable_features.udp_listener_updates_filter_chain_in_place") && - overridden_listener.has_value()) { + if (!support_udp_in_place_filter_chain_update && overridden_listener.has_value()) { for (auto& listener : listeners_) { if (listener.second.listener_->listenerTag() == overridden_listener) { listener.second.tcpListener()->get().updateListenerConfig(config); diff --git a/source/server/listener_impl.cc b/source/server/listener_impl.cc index 45f6e96db6a51..3c6f074e78450 100644 --- a/source/server/listener_impl.cc +++ b/source/server/listener_impl.cc @@ -553,6 +553,13 @@ void ListenerImpl::validateFilterChains(Network::Socket::Type socket_type) { "specified for connection oriented UDP listener", address_->asString())); } + } else if ((!config_.filter_chains().empty() || config_.has_default_filter_chain()) && + udp_listener_config_ != nullptr && + udp_listener_config_->listener_factory_->isTransportConnectionless()) { + + throw EnvoyException(fmt::format("error adding listener '{}': {} filter chain(s) specified for " + "connection-less UDP listener.", + address_->asString(), config_.filter_chains_size())); } } @@ -768,7 +775,6 @@ bool ListenerImpl::supportUpdateFilterChain(const envoy::config::listener::v3::L return false; } - ; if (!Runtime::runtimeFeatureEnabled( "envoy.reloadable_features.udp_listener_updates_filter_chain_in_place") && (Network::Utility::protobufAddressSocketType(config_.address()) != diff --git a/test/server/listener_manager_impl_test.cc b/test/server/listener_manager_impl_test.cc index 9ef3f0aa91165..8bf8e13880543 100644 --- a/test/server/listener_manager_impl_test.cc +++ b/test/server/listener_manager_impl_test.cc @@ -297,7 +297,6 @@ TEST_F(ListenerManagerImplWithRealFiltersTest, UdpAddress) { port_value: 1234 } } - filter_chains: {} )EOF"; envoy::config::listener::v3::Listener listener_proto; EXPECT_TRUE(Protobuf::TextFormat::ParseFromString(proto_text, &listener_proto)); @@ -389,6 +388,22 @@ TEST_F(ListenerManagerImplWithRealFiltersTest, BadFilterConfig) { EXPECT_THROW_WITH_REGEX(manager_->addOrUpdateListener(parseListenerFromV3Yaml(yaml), "", true), EnvoyException, "foo: Cannot find field"); } + +TEST_F(ListenerManagerImplWithRealFiltersTest, BadConnectionLessUdpConfigWithFilterChain) { + const std::string yaml = R"EOF( +address: + socket_address: + protocol: UDP + address: 127.0.0.1 + port_value: 1234 +filter_chains: {} + )EOF"; + + EXPECT_THROW_WITH_REGEX(manager_->addOrUpdateListener(parseListenerFromV3Yaml(yaml), "", true), + EnvoyException, + "1 filter chain\\(s\\) specified for connection-less UDP listener"); +} + class NonTerminalFilterFactory : public Configuration::NamedNetworkFilterConfigFactory { public: // Configuration::NamedNetworkFilterConfigFactory @@ -5117,12 +5132,15 @@ TEST_F(ListenerManagerImplForInPlaceFilterChainUpdateTest, TraditionalUpdateIfDi auto new_listener_proto = listener_proto; new_listener_proto.mutable_address()->mutable_socket_address()->set_protocol( envoy::config::core::v3::SocketAddress_Protocol::SocketAddress_Protocol_UDP); + EXPECT_CALL(server_.validation_context_, staticValidationVisitor()).Times(0); + EXPECT_CALL(server_.validation_context_, dynamicValidationVisitor()); + EXPECT_CALL(listener_factory_, createDrainManager_(_)); + EXPECT_THROW_WITH_MESSAGE(manager_->addOrUpdateListener(new_listener_proto, "", true), + EnvoyException, + "error adding listener '127.0.0.1:1234': 1 filter chain(s) specified " + "for connection-less UDP listener."); - ListenerHandle* listener_foo_update1 = expectListenerCreate(false, true); - expectUpdateToThenDrain(new_listener_proto, listener_foo, OptRef(), - ListenerComponentFactory::BindType::ReusePort); - expectRemove(new_listener_proto, listener_foo_update1, *listener_factory_.socket_); - + expectRemove(new_listener_proto, listener_foo, *listener_factory_.socket_); EXPECT_EQ(0UL, manager_->listeners().size()); EXPECT_EQ(0, server_.stats_store_.counter("listener_manager.listener_in_place_updated").value()); } @@ -5204,8 +5222,6 @@ TEST_F(ListenerManagerImplTest, UdpDefaultWriterConfig) { address: 127.0.0.1 protocol: UDP port_value: 1234 -filter_chains: - filters: [] )EOF"); manager_->addOrUpdateListener(listener, "", true); EXPECT_EQ(1U, manager_->listeners().size()); From 3dcecd0fe5f614bfaddd233ba432b8729bf6777e Mon Sep 17 00:00:00 2001 From: Dan Zhang Date: Fri, 17 Sep 2021 15:14:09 -0400 Subject: [PATCH 12/13] guard filter chain verification Signed-off-by: Dan Zhang --- docs/root/version_history/current.rst | 3 ++- source/server/active_udp_listener.h | 2 ++ source/server/connection_handler_impl.cc | 2 +- source/server/listener_impl.cc | 4 +++- 4 files changed, 8 insertions(+), 3 deletions(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index edad0158123b4..7a480fdd35017 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -114,7 +114,8 @@ New Features * route config: added :ref:`dynamic_metadata ` for routing based on dynamic metadata. * sxg_filter: added filter to transform response to SXG package to :ref:`contrib images `. This can be enabled by setting :ref:`SXG ` configuration. * thrift_proxy: added support for :ref:`mirroring requests `. -* udp: allows updating filter chain in-place through LDS, which is supported by Quic listener. This will be No-op with error log in other UDP listener implementations. It can be reverted by ``envoy.reloadable_features.udp_listener_updates_filter_chain_in_place``. +* udp: allows updating filter chain in-place through LDS, which is supported by Quic listener. Such listener config will be rejected in other connection-less UDP listener implementations. It can be reverted by ``envoy.reloadable_features.udp_listener_updates_filter_chain_in_place``. +* udp: disallow L4 filter chain in config which configures connection-less UDP listener. It can be reverted by ``envoy.reloadable_features.udp_listener_updates_filter_chain_in_place``. Deprecated ---------- diff --git a/source/server/active_udp_listener.h b/source/server/active_udp_listener.h index 3d73ef1eda076..68918ffc39301 100644 --- a/source/server/active_udp_listener.h +++ b/source/server/active_udp_listener.h @@ -103,6 +103,8 @@ class ActiveRawUdpListener : public ActiveUdpListenerBase, read_filter_.reset(); udp_listener_.reset(); } + // These two are unreachable because a config will be rejected if it configures both this listener + // and any L4 filter chain. void updateListenerConfig(Network::ListenerConfig&) override { NOT_REACHED_GCOVR_EXCL_LINE; } void onFilterChainDraining(const std::list&) override { NOT_REACHED_GCOVR_EXCL_LINE; diff --git a/source/server/connection_handler_impl.cc b/source/server/connection_handler_impl.cc index 37abdcc8fa1b0..4bcdcd9cc16aa 100644 --- a/source/server/connection_handler_impl.cc +++ b/source/server/connection_handler_impl.cc @@ -27,7 +27,7 @@ void ConnectionHandlerImpl::decNumConnections() { void ConnectionHandlerImpl::addListener(absl::optional overridden_listener, Network::ListenerConfig& config) { - bool support_udp_in_place_filter_chain_update = Runtime::runtimeFeatureEnabled( + const bool support_udp_in_place_filter_chain_update = Runtime::runtimeFeatureEnabled( "envoy.reloadable_features.udp_listener_updates_filter_chain_in_place"); if (support_udp_in_place_filter_chain_update && overridden_listener.has_value()) { ActiveListenerDetailsOptRef listener_detail = diff --git a/source/server/listener_impl.cc b/source/server/listener_impl.cc index 3c6f074e78450..a4eed2f7e2b42 100644 --- a/source/server/listener_impl.cc +++ b/source/server/listener_impl.cc @@ -553,7 +553,9 @@ void ListenerImpl::validateFilterChains(Network::Socket::Type socket_type) { "specified for connection oriented UDP listener", address_->asString())); } - } else if ((!config_.filter_chains().empty() || config_.has_default_filter_chain()) && + } else if (Runtime::runtimeFeatureEnabled( + "envoy.reloadable_features.udp_listener_updates_filter_chain_in_place") && + (!config_.filter_chains().empty() || config_.has_default_filter_chain()) && udp_listener_config_ != nullptr && udp_listener_config_->listener_factory_->isTransportConnectionless()) { From 844c366e87af5417c3e54a6b5987e09138778cb9 Mon Sep 17 00:00:00 2001 From: Dan Zhang Date: Mon, 20 Sep 2021 21:07:43 -0400 Subject: [PATCH 13/13] comments Signed-off-by: Dan Zhang --- source/common/quic/envoy_quic_server_session.h | 4 ++++ source/server/active_tcp_listener.h | 2 +- source/server/listener_impl.cc | 2 +- 3 files changed, 6 insertions(+), 2 deletions(-) diff --git a/source/common/quic/envoy_quic_server_session.h b/source/common/quic/envoy_quic_server_session.h index 66182fc78e302..313c447101bb4 100644 --- a/source/common/quic/envoy_quic_server_session.h +++ b/source/common/quic/envoy_quic_server_session.h @@ -34,13 +34,17 @@ using FilterChainToConnectionMap = std::list>>; using ConnectionMapIter = std::list>::iterator; +// Used to track the matching filter chain and its position in the filter chain to connection map. struct ConnectionMapPosition { ConnectionMapPosition(FilterChainToConnectionMap& connection_map, const Network::FilterChain& filter_chain, ConnectionMapIter iterator) : connection_map_(connection_map), filter_chain_(filter_chain), iterator_(iterator) {} + // Stores the map from filter chain of connections. FilterChainToConnectionMap& connection_map_; + // The matching filter chain of a connection. const Network::FilterChain& filter_chain_; + // The position of the connection in the map. ConnectionMapIter iterator_; }; diff --git a/source/server/active_tcp_listener.h b/source/server/active_tcp_listener.h index 293b996959735..0d0299a44914f 100644 --- a/source/server/active_tcp_listener.h +++ b/source/server/active_tcp_listener.h @@ -71,7 +71,7 @@ class ActiveTcpListener final : public Network::TcpListenerCallbacks, std::unique_ptr stream_info) override; /** - * The follow up connections will see the new config. The existing + * Update the listener config. The follow up connections will see the new config. The existing * connections are not impacted. */ void updateListenerConfig(Network::ListenerConfig& config) override; diff --git a/source/server/listener_impl.cc b/source/server/listener_impl.cc index a4eed2f7e2b42..bb051e5c78438 100644 --- a/source/server/listener_impl.cc +++ b/source/server/listener_impl.cc @@ -414,7 +414,7 @@ ListenerImpl::ListenerImpl(ListenerImpl& origin, buildFilterChains(); if (socket_type == Network::Socket::Type::Stream) { - // Apply below tcp only initialization. + // Apply the options below only for TCP. buildSocketOptions(); buildOriginalDstListenerFilter(); buildProxyProtocolListenerFilter();