From f8f21c26db2e2c0e7f9c7f1cb111d712591753dc Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Fri, 3 Aug 2018 03:10:27 +0700 Subject: [PATCH 01/11] Rename duplicated ads integration test case name (#4035) Signed-off-by: Dhi Aurrahman --- test/integration/ads_integration_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/integration/ads_integration_test.cc b/test/integration/ads_integration_test.cc index c0759d4958255..fb94edf25736c 100644 --- a/test/integration/ads_integration_test.cc +++ b/test/integration/ads_integration_test.cc @@ -477,7 +477,7 @@ TEST_P(AdsIntegrationTest, Failure) { } // Validate that the request with duplicate listeners is rejected. -TEST_P(AdsIntegrationTest, DuplicateWarmingClusters) { +TEST_P(AdsIntegrationTest, DuplicateWarmingListeners) { initialize(); // Send initial configuration, validate we can process a request. From 9bc0472263d123a484549cad0090a5e8b9fdd6bf Mon Sep 17 00:00:00 2001 From: Jimmy Chen <28548492+JimmyCYJ@users.noreply.github.com> Date: Thu, 2 Aug 2018 15:26:46 -0700 Subject: [PATCH 02/11] Refactor TransportSocketFactoryContext and Cluster interfaces. (#4026) Signed-off-by: JimmyCYJ --- include/envoy/server/BUILD | 5 + .../envoy/server/transport_socket_config.h | 30 ++ source/common/upstream/BUILD | 1 + source/common/upstream/eds.cc | 22 +- source/common/upstream/eds.h | 6 +- source/common/upstream/logical_dns_cluster.cc | 21 +- source/common/upstream/logical_dns_cluster.h | 5 +- .../common/upstream/original_dst_cluster.cc | 18 +- source/common/upstream/original_dst_cluster.h | 5 +- source/common/upstream/upstream_impl.cc | 148 +++++---- source/common/upstream/upstream_impl.h | 34 +- source/server/BUILD | 9 + source/server/listener_manager_impl.cc | 16 +- source/server/listener_manager_impl.h | 6 - source/server/transport_socket_config_impl.h | 52 ++++ test/common/upstream/BUILD | 6 + test/common/upstream/eds_test.cc | 12 +- .../upstream/logical_dns_cluster_test.cc | 27 +- .../upstream/original_dst_cluster_test.cc | 16 +- test/common/upstream/sds_test.cc | 13 +- test/common/upstream/upstream_impl_test.cc | 292 ++++++++++++++---- test/mocks/server/mocks.h | 6 + 22 files changed, 544 insertions(+), 206 deletions(-) create mode 100644 source/server/transport_socket_config_impl.h diff --git a/include/envoy/server/BUILD b/include/envoy/server/BUILD index 42acd80d8f161..3a3179362ce49 100644 --- a/include/envoy/server/BUILD +++ b/include/envoy/server/BUILD @@ -175,9 +175,14 @@ envoy_cc_library( name = "transport_socket_config_interface", hdrs = ["transport_socket_config.h"], deps = [ + "//include/envoy/event:dispatcher_interface", + "//include/envoy/local_info:local_info_interface", "//include/envoy/network:transport_socket_interface", + "//include/envoy/runtime:runtime_interface", "//include/envoy/secret:secret_manager_interface", "//include/envoy/ssl:context_manager_interface", + "//include/envoy/stats:stats_interface", + "//include/envoy/upstream:cluster_manager_interface", "//source/common/protobuf", ], ) diff --git a/include/envoy/server/transport_socket_config.h b/include/envoy/server/transport_socket_config.h index 4e85386cb16d8..de36c3d963393 100644 --- a/include/envoy/server/transport_socket_config.h +++ b/include/envoy/server/transport_socket_config.h @@ -2,9 +2,14 @@ #include +#include "envoy/event/dispatcher.h" +#include "envoy/local_info/local_info.h" #include "envoy/network/transport_socket.h" +#include "envoy/runtime/runtime.h" #include "envoy/secret/secret_manager.h" #include "envoy/ssl/context_manager.h" +#include "envoy/stats/stats.h" +#include "envoy/upstream/cluster_manager.h" #include "common/protobuf/protobuf.h" @@ -33,6 +38,31 @@ class TransportSocketFactoryContext { * Return the instance of secret manager. */ virtual Secret::SecretManager& secretManager() PURE; + + /** + * @return the instance of ClusterManager. + */ + virtual Upstream::ClusterManager& clusterManager() PURE; + + /** + * @return information about the local environment the server is running in. + */ + virtual const LocalInfo::LocalInfo& localInfo() PURE; + + /** + * @return Event::Dispatcher& the main thread's dispatcher. + */ + virtual Event::Dispatcher& dispatcher() PURE; + + /** + * @return RandomGenerator& the random generator for the server. + */ + virtual Envoy::Runtime::RandomGenerator& random() PURE; + + /** + * @return the server-wide stats store. + */ + virtual Stats::Store& stats() PURE; }; class TransportSocketConfigFactory { diff --git a/source/common/upstream/BUILD b/source/common/upstream/BUILD index 939f0f58b220d..545a660a4368c 100644 --- a/source/common/upstream/BUILD +++ b/source/common/upstream/BUILD @@ -362,6 +362,7 @@ envoy_cc_library( "//source/common/protobuf", "//source/common/protobuf:utility_lib", "//source/extensions/transport_sockets:well_known_names", + "//source/server:transport_socket_config_lib", "@envoy_api//envoy/api/v2/core:base_cc", ], ) diff --git a/source/common/upstream/eds.cc b/source/common/upstream/eds.cc index 46435a6de4d3c..8272738fe5d4f 100644 --- a/source/common/upstream/eds.cc +++ b/source/common/upstream/eds.cc @@ -17,23 +17,25 @@ namespace Envoy { namespace Upstream { -EdsClusterImpl::EdsClusterImpl(const envoy::api::v2::Cluster& cluster, Runtime::Loader& runtime, - Stats::Store& stats, Ssl::ContextManager& ssl_context_manager, - const LocalInfo::LocalInfo& local_info, ClusterManager& cm, - Event::Dispatcher& dispatcher, Runtime::RandomGenerator& random, - bool added_via_api) - : BaseDynamicClusterImpl(cluster, cm.bindConfig(), runtime, stats, ssl_context_manager, - cm.clusterManagerFactory().secretManager(), added_via_api), - cm_(cm), local_info_(local_info), +EdsClusterImpl::EdsClusterImpl( + const envoy::api::v2::Cluster& cluster, Runtime::Loader& runtime, + Server::Configuration::TransportSocketFactoryContext& factory_context, + Stats::ScopePtr&& stats_scope, bool added_via_api) + : BaseDynamicClusterImpl(cluster, runtime, factory_context, std::move(stats_scope), + added_via_api), + cm_(factory_context.clusterManager()), local_info_(factory_context.localInfo()), cluster_name_(cluster.eds_cluster_config().service_name().empty() ? cluster.name() : cluster.eds_cluster_config().service_name()) { - Config::Utility::checkLocalInfo("eds", local_info); + Config::Utility::checkLocalInfo("eds", local_info_); const auto& eds_config = cluster.eds_cluster_config().eds_config(); + Event::Dispatcher& dispatcher = factory_context.dispatcher(); + Runtime::RandomGenerator& random = factory_context.random(); + Upstream::ClusterManager& cm = factory_context.clusterManager(); subscription_ = Config::SubscriptionFactory::subscriptionFromConfigSource< envoy::api::v2::ClusterLoadAssignment>( - eds_config, local_info.node(), dispatcher, cm, random, info_->statsScope(), + eds_config, local_info_.node(), dispatcher, cm, random, info_->statsScope(), [this, &eds_config, &cm, &dispatcher, &random]() -> Config::Subscription* { return new SdsSubscription(info_->stats(), eds_config, cm, dispatcher, random); diff --git a/source/common/upstream/eds.h b/source/common/upstream/eds.h index d84f02091799e..b4c10794e9ccf 100644 --- a/source/common/upstream/eds.h +++ b/source/common/upstream/eds.h @@ -19,10 +19,8 @@ class EdsClusterImpl : public BaseDynamicClusterImpl, Config::SubscriptionCallbacks { public: EdsClusterImpl(const envoy::api::v2::Cluster& cluster, Runtime::Loader& runtime, - Stats::Store& stats, Ssl::ContextManager& ssl_context_manager, - const LocalInfo::LocalInfo& local_info, ClusterManager& cm, - Event::Dispatcher& dispatcher, Runtime::RandomGenerator& random, - bool added_via_api); + Server::Configuration::TransportSocketFactoryContext& factory_context, + Stats::ScopePtr&& stats_scope, bool added_via_api); // Upstream::Cluster InitializePhase initializePhase() const override { return InitializePhase::Secondary; } diff --git a/source/common/upstream/logical_dns_cluster.cc b/source/common/upstream/logical_dns_cluster.cc index 5b3709041b021..10ffac930685c 100644 --- a/source/common/upstream/logical_dns_cluster.cc +++ b/source/common/upstream/logical_dns_cluster.cc @@ -15,21 +15,18 @@ namespace Envoy { namespace Upstream { -LogicalDnsCluster::LogicalDnsCluster(const envoy::api::v2::Cluster& cluster, - Runtime::Loader& runtime, Stats::Store& stats, - Ssl::ContextManager& ssl_context_manager, - const LocalInfo::LocalInfo& local_info, - Network::DnsResolverSharedPtr dns_resolver, - ThreadLocal::SlotAllocator& tls, ClusterManager& cm, - Event::Dispatcher& dispatcher, bool added_via_api) - : ClusterImplBase(cluster, cm.bindConfig(), runtime, stats, ssl_context_manager, - cm.clusterManagerFactory().secretManager(), added_via_api), +LogicalDnsCluster::LogicalDnsCluster( + const envoy::api::v2::Cluster& cluster, Runtime::Loader& runtime, + Network::DnsResolverSharedPtr dns_resolver, ThreadLocal::SlotAllocator& tls, + Server::Configuration::TransportSocketFactoryContext& factory_context, + Stats::ScopePtr&& stats_scope, bool added_via_api) + : ClusterImplBase(cluster, runtime, factory_context, std::move(stats_scope), added_via_api), dns_resolver_(dns_resolver), dns_refresh_rate_ms_( std::chrono::milliseconds(PROTOBUF_GET_MS_OR_DEFAULT(cluster, dns_refresh_rate, 5000))), - tls_(tls.allocateSlot()), - resolve_timer_(dispatcher.createTimer([this]() -> void { startResolve(); })), - local_info_(local_info), + tls_(tls.allocateSlot()), resolve_timer_(factory_context.dispatcher().createTimer( + [this]() -> void { startResolve(); })), + local_info_(factory_context.localInfo()), load_assignment_(cluster.has_load_assignment() ? cluster.load_assignment() : Config::Utility::translateClusterHosts(cluster.hosts())) { diff --git a/source/common/upstream/logical_dns_cluster.h b/source/common/upstream/logical_dns_cluster.h index 2feec15579b78..cf3660073762c 100644 --- a/source/common/upstream/logical_dns_cluster.h +++ b/source/common/upstream/logical_dns_cluster.h @@ -29,10 +29,9 @@ namespace Upstream { class LogicalDnsCluster : public ClusterImplBase { public: LogicalDnsCluster(const envoy::api::v2::Cluster& cluster, Runtime::Loader& runtime, - Stats::Store& stats, Ssl::ContextManager& ssl_context_manager, - const LocalInfo::LocalInfo& local_info, Network::DnsResolverSharedPtr dns_resolver, ThreadLocal::SlotAllocator& tls, - ClusterManager& cm, Event::Dispatcher& dispatcher, bool added_via_api); + Server::Configuration::TransportSocketFactoryContext& factory_context, + Stats::ScopePtr&& stats_scope, bool added_via_api); ~LogicalDnsCluster(); diff --git a/source/common/upstream/original_dst_cluster.cc b/source/common/upstream/original_dst_cluster.cc index e00f39f9410d9..f519d4c543c41 100644 --- a/source/common/upstream/original_dst_cluster.cc +++ b/source/common/upstream/original_dst_cluster.cc @@ -122,15 +122,15 @@ OriginalDstCluster::LoadBalancer::requestOverrideHost(LoadBalancerContext* conte return request_host; } -OriginalDstCluster::OriginalDstCluster(const envoy::api::v2::Cluster& config, - Runtime::Loader& runtime, Stats::Store& stats, - Ssl::ContextManager& ssl_context_manager, ClusterManager& cm, - Event::Dispatcher& dispatcher, bool added_via_api) - : ClusterImplBase(config, cm.bindConfig(), runtime, stats, ssl_context_manager, - cm.clusterManagerFactory().secretManager(), added_via_api), - dispatcher_(dispatcher), cleanup_interval_ms_(std::chrono::milliseconds( - PROTOBUF_GET_MS_OR_DEFAULT(config, cleanup_interval, 5000))), - cleanup_timer_(dispatcher.createTimer([this]() -> void { cleanup(); })) { +OriginalDstCluster::OriginalDstCluster( + const envoy::api::v2::Cluster& config, Runtime::Loader& runtime, + Server::Configuration::TransportSocketFactoryContext& factory_context, + Stats::ScopePtr&& stats_scope, bool added_via_api) + : ClusterImplBase(config, runtime, factory_context, std::move(stats_scope), added_via_api), + dispatcher_(factory_context.dispatcher()), + cleanup_interval_ms_( + std::chrono::milliseconds(PROTOBUF_GET_MS_OR_DEFAULT(config, cleanup_interval, 5000))), + cleanup_timer_(dispatcher_.createTimer([this]() -> void { cleanup(); })) { cleanup_timer_->enableTimer(cleanup_interval_ms_); } diff --git a/source/common/upstream/original_dst_cluster.h b/source/common/upstream/original_dst_cluster.h index 5cb5107a3a4aa..74d02a8627700 100644 --- a/source/common/upstream/original_dst_cluster.h +++ b/source/common/upstream/original_dst_cluster.h @@ -6,6 +6,7 @@ #include #include "envoy/secret/secret_manager.h" +#include "envoy/server/transport_socket_config.h" #include "envoy/thread_local/thread_local.h" #include "common/common/empty_string.h" @@ -24,8 +25,8 @@ namespace Upstream { class OriginalDstCluster : public ClusterImplBase { public: OriginalDstCluster(const envoy::api::v2::Cluster& config, Runtime::Loader& runtime, - Stats::Store& stats, Ssl::ContextManager& ssl_context_manager, - ClusterManager& cm, Event::Dispatcher& dispatcher, bool added_via_api); + Server::Configuration::TransportSocketFactoryContext& factory_context, + Stats::ScopePtr&& stats_scope, bool added_via_api); // Upstream::Cluster InitializePhase initializePhase() const override { return InitializePhase::Primary; } diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index 0ac8dc532041b..05d1396405b89 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -33,6 +33,8 @@ #include "common/upstream/logical_dns_cluster.h" #include "common/upstream/original_dst_cluster.h" +#include "server/transport_socket_config_impl.h" + #include "extensions/transport_sockets/well_known_names.h" namespace Envoy { @@ -265,9 +267,9 @@ ClusterLoadReportStats ClusterInfoImpl::generateLoadReportStats(Stats::Scope& sc ClusterInfoImpl::ClusterInfoImpl(const envoy::api::v2::Cluster& config, const envoy::api::v2::core::BindConfig& bind_config, - Runtime::Loader& runtime, Stats::Store& stats, - Ssl::ContextManager& ssl_context_manager, - Secret::SecretManager& secret_manager, bool added_via_api) + Runtime::Loader& runtime, + Network::TransportSocketFactoryPtr&& socket_factory, + Stats::ScopePtr&& stats_scope, bool added_via_api) : runtime_(runtime), name_(config.name()), type_(config.type()), max_requests_per_connection_( PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, max_requests_per_connection, 0)), @@ -275,9 +277,7 @@ ClusterInfoImpl::ClusterInfoImpl(const envoy::api::v2::Cluster& config, std::chrono::milliseconds(PROTOBUF_GET_MS_REQUIRED(config, connect_timeout))), per_connection_buffer_limit_bytes_( PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, per_connection_buffer_limit_bytes, 1024 * 1024)), - stats_scope_(stats.createScope(fmt::format( - "cluster.{}.", - config.alt_stat_name().empty() ? name_ : std::string(config.alt_stat_name())))), + transport_socket_factory_(std::move(socket_factory)), stats_scope_(std::move(stats_scope)), stats_(generateStats(*stats_scope_)), load_report_stats_(generateLoadReportStats(load_report_stats_store_)), features_(parseFeatures(config)), @@ -286,32 +286,11 @@ ClusterInfoImpl::ClusterInfoImpl(const envoy::api::v2::Cluster& config, maintenance_mode_runtime_key_(fmt::format("upstream.maintenance_mode.{}", name_)), source_address_(getSourceAddress(config, bind_config)), lb_ring_hash_config_(envoy::api::v2::Cluster::RingHashLbConfig(config.ring_hash_lb_config())), - ssl_context_manager_(ssl_context_manager), added_via_api_(added_via_api), + added_via_api_(added_via_api), lb_subset_(LoadBalancerSubsetInfoImpl(config.lb_subset_config())), metadata_(config.metadata()), common_lb_config_(config.common_lb_config()), cluster_socket_options_(parseClusterSocketOptions(config, bind_config)), - drain_connections_on_host_removal_(config.drain_connections_on_host_removal()), - secret_manager_(secret_manager) { - - // If the cluster doesn't have a transport socket configured, override with the default transport - // socket implementation based on the tls_context. We copy by value first then override if - // necessary. - auto transport_socket = config.transport_socket(); - if (!config.has_transport_socket()) { - if (config.has_tls_context()) { - transport_socket.set_name(Extensions::TransportSockets::TransportSocketNames::get().Tls); - MessageUtil::jsonConvert(config.tls_context(), *transport_socket.mutable_config()); - } else { - transport_socket.set_name( - Extensions::TransportSockets::TransportSocketNames::get().RawBuffer); - } - } - - auto& config_factory = Config::Utility::getAndCheckFactory< - Server::Configuration::UpstreamTransportSocketConfigFactory>(transport_socket.name()); - ProtobufTypes::MessagePtr message = - Config::Utility::translateToFactoryConfig(transport_socket, config_factory); - transport_socket_factory_ = config_factory.createTransportSocketFactory(*message, *this); + drain_connections_on_host_removal_(config.drain_connections_on_host_removal()) { switch (config.lb_policy()) { case envoy::api::v2::Cluster::ROUND_ROBIN: @@ -354,6 +333,40 @@ ClusterInfoImpl::ClusterInfoImpl(const envoy::api::v2::Cluster& config, } } +namespace { + +Stats::ScopePtr generateStatsScope(const envoy::api::v2::Cluster& config, Stats::Store& stats) { + return stats.createScope(fmt::format("cluster.{}.", config.alt_stat_name().empty() + ? config.name() + : std::string(config.alt_stat_name()))); +} + +Network::TransportSocketFactoryPtr createTransportSocketFactory( + const envoy::api::v2::Cluster& config, + Server::Configuration::TransportSocketFactoryContext& factory_context) { + // If the cluster config doesn't have a transport socket configured, override with the default + // transport socket implementation based on the tls_context. We copy by value first then override + // if necessary. + auto transport_socket = config.transport_socket(); + if (!config.has_transport_socket()) { + if (config.has_tls_context()) { + transport_socket.set_name(Extensions::TransportSockets::TransportSocketNames::get().Tls); + MessageUtil::jsonConvert(config.tls_context(), *transport_socket.mutable_config()); + } else { + transport_socket.set_name( + Extensions::TransportSockets::TransportSocketNames::get().RawBuffer); + } + } + + auto& config_factory = Config::Utility::getAndCheckFactory< + Server::Configuration::UpstreamTransportSocketConfigFactory>(transport_socket.name()); + ProtobufTypes::MessagePtr message = + Config::Utility::translateToFactoryConfig(transport_socket, config_factory); + return config_factory.createTransportSocketFactory(*message, factory_context); +} + +} // namespace + ClusterSharedPtr ClusterImplBase::create( const envoy::api::v2::Cluster& cluster, ClusterManager& cm, Stats::Store& stats, ThreadLocal::Instance& tls, Network::DnsResolverSharedPtr dns_resolver, @@ -380,19 +393,23 @@ ClusterSharedPtr ClusterImplBase::create( selected_dns_resolver = dispatcher.createDnsResolver(resolvers); } + auto stats_scope = generateStatsScope(cluster, stats); + Server::Configuration::TransportSocketFactoryContextImpl factory_context( + ssl_context_manager, *stats_scope, cm, local_info, dispatcher, random, stats); + switch (cluster.type()) { case envoy::api::v2::Cluster::STATIC: - new_cluster.reset(new StaticClusterImpl(cluster, runtime, stats, ssl_context_manager, - local_info, cm, added_via_api)); + new_cluster.reset(new StaticClusterImpl(cluster, runtime, factory_context, + std::move(stats_scope), added_via_api)); break; case envoy::api::v2::Cluster::STRICT_DNS: - new_cluster.reset(new StrictDnsClusterImpl(cluster, runtime, stats, ssl_context_manager, - local_info, selected_dns_resolver, cm, dispatcher, + new_cluster.reset(new StrictDnsClusterImpl(cluster, runtime, selected_dns_resolver, + factory_context, std::move(stats_scope), added_via_api)); break; case envoy::api::v2::Cluster::LOGICAL_DNS: - new_cluster.reset(new LogicalDnsCluster(cluster, runtime, stats, ssl_context_manager, - local_info, selected_dns_resolver, tls, cm, dispatcher, + new_cluster.reset(new LogicalDnsCluster(cluster, runtime, selected_dns_resolver, tls, + factory_context, std::move(stats_scope), added_via_api)); break; case envoy::api::v2::Cluster::ORIGINAL_DST: @@ -404,8 +421,8 @@ ClusterSharedPtr ClusterImplBase::create( throw EnvoyException(fmt::format( "cluster: cluster type 'original_dst' may not be used with lb_subset_config")); } - new_cluster.reset(new OriginalDstCluster(cluster, runtime, stats, ssl_context_manager, cm, - dispatcher, added_via_api)); + new_cluster.reset(new OriginalDstCluster(cluster, runtime, factory_context, + std::move(stats_scope), added_via_api)); break; case envoy::api::v2::Cluster::EDS: if (!cluster.has_eds_cluster_config()) { @@ -413,8 +430,8 @@ ClusterSharedPtr ClusterImplBase::create( } // We map SDS to EDS, since EDS provides backwards compatibility with SDS. - new_cluster.reset(new EdsClusterImpl(cluster, runtime, stats, ssl_context_manager, local_info, - cm, dispatcher, random, added_via_api)); + new_cluster.reset(new EdsClusterImpl(cluster, runtime, factory_context, std::move(stats_scope), + added_via_api)); break; default: NOT_REACHED_GCOVR_EXCL_LINE; @@ -435,14 +452,15 @@ ClusterSharedPtr ClusterImplBase::create( return std::move(new_cluster); } -ClusterImplBase::ClusterImplBase(const envoy::api::v2::Cluster& cluster, - const envoy::api::v2::core::BindConfig& bind_config, - Runtime::Loader& runtime, Stats::Store& stats, - Ssl::ContextManager& ssl_context_manager, - Secret::SecretManager& secret_manager, bool added_via_api) - : runtime_(runtime), - info_(new ClusterInfoImpl(cluster, bind_config, runtime, stats, ssl_context_manager, - secret_manager, added_via_api)) { +ClusterImplBase::ClusterImplBase( + const envoy::api::v2::Cluster& cluster, Runtime::Loader& runtime, + Server::Configuration::TransportSocketFactoryContext& factory_context, + Stats::ScopePtr&& stats_scope, bool added_via_api) + : runtime_(runtime) { + auto socket_factory = createTransportSocketFactory(cluster, factory_context); + info_ = std::make_unique(cluster, factory_context.clusterManager().bindConfig(), + runtime, std::move(socket_factory), + std::move(stats_scope), added_via_api); // Create the default (empty) priority set before registering callbacks to // avoid getting an update the first time it is accessed. priority_set_.getOrCreateHostSet(0); @@ -768,14 +786,12 @@ void PriorityStateManager::updateClusterPrioritySet( hosts_removed.value_or({})); } -StaticClusterImpl::StaticClusterImpl(const envoy::api::v2::Cluster& cluster, - Runtime::Loader& runtime, Stats::Store& stats, - Ssl::ContextManager& ssl_context_manager, - const LocalInfo::LocalInfo& local_info, ClusterManager& cm, - bool added_via_api) - : ClusterImplBase(cluster, cm.bindConfig(), runtime, stats, ssl_context_manager, - cm.clusterManagerFactory().secretManager(), added_via_api), - priority_state_manager_(new PriorityStateManager(*this, local_info)) { +StaticClusterImpl::StaticClusterImpl( + const envoy::api::v2::Cluster& cluster, Runtime::Loader& runtime, + Server::Configuration::TransportSocketFactoryContext& factory_context, + Stats::ScopePtr&& stats_scope, bool added_via_api) + : ClusterImplBase(cluster, runtime, factory_context, std::move(stats_scope), added_via_api), + priority_state_manager_(new PriorityStateManager(*this, factory_context.localInfo())) { // TODO(dio): Use by-reference when cluster.hosts() is removed. const envoy::api::v2::ClusterLoadAssignment cluster_load_assignment( cluster.has_load_assignment() ? cluster.load_assignment() @@ -952,16 +968,14 @@ bool BaseDynamicClusterImpl::updateDynamicHostList(const HostVector& new_hosts, } } -StrictDnsClusterImpl::StrictDnsClusterImpl(const envoy::api::v2::Cluster& cluster, - Runtime::Loader& runtime, Stats::Store& stats, - Ssl::ContextManager& ssl_context_manager, - const LocalInfo::LocalInfo& local_info, - Network::DnsResolverSharedPtr dns_resolver, - ClusterManager& cm, Event::Dispatcher& dispatcher, - bool added_via_api) - : BaseDynamicClusterImpl(cluster, cm.bindConfig(), runtime, stats, ssl_context_manager, - cm.clusterManagerFactory().secretManager(), added_via_api), - local_info_(local_info), dns_resolver_(dns_resolver), +StrictDnsClusterImpl::StrictDnsClusterImpl( + const envoy::api::v2::Cluster& cluster, Runtime::Loader& runtime, + Network::DnsResolverSharedPtr dns_resolver, + Server::Configuration::TransportSocketFactoryContext& factory_context, + Stats::ScopePtr&& stats_scope, bool added_via_api) + : BaseDynamicClusterImpl(cluster, runtime, factory_context, std::move(stats_scope), + added_via_api), + local_info_(factory_context.localInfo()), dns_resolver_(dns_resolver), dns_refresh_rate_ms_( std::chrono::milliseconds(PROTOBUF_GET_MS_OR_DEFAULT(cluster, dns_refresh_rate, 5000))) { switch (cluster.dns_lookup_family()) { @@ -987,8 +1001,8 @@ StrictDnsClusterImpl::StrictDnsClusterImpl(const envoy::api::v2::Cluster& cluste const auto& host = lb_endpoint.endpoint().address(); const std::string& url = fmt::format("tcp://{}:{}", host.socket_address().address(), host.socket_address().port_value()); - resolve_targets_.emplace_back( - new ResolveTarget(*this, dispatcher, url, locality_lb_endpoint, lb_endpoint)); + resolve_targets_.emplace_back(new ResolveTarget(*this, factory_context.dispatcher(), url, + locality_lb_endpoint, lb_endpoint)); } } } diff --git a/source/common/upstream/upstream_impl.h b/source/common/upstream/upstream_impl.h index f9b4c4e57cfd4..4b0989df38611 100644 --- a/source/common/upstream/upstream_impl.h +++ b/source/common/upstream/upstream_impl.h @@ -328,13 +328,12 @@ class PrioritySetImpl : public PrioritySet { /** * Implementation of ClusterInfo that reads from JSON. */ -class ClusterInfoImpl : public ClusterInfo, - public Server::Configuration::TransportSocketFactoryContext { +class ClusterInfoImpl : public ClusterInfo { public: ClusterInfoImpl(const envoy::api::v2::Cluster& config, const envoy::api::v2::core::BindConfig& bind_config, Runtime::Loader& runtime, - Stats::Store& stats, Ssl::ContextManager& ssl_context_manager, - Secret::SecretManager& secret_manager, bool added_via_api); + Network::TransportSocketFactoryPtr&& socket_factory, + Stats::ScopePtr&& stats_scope, bool added_via_api); static ClusterStats generateStats(Stats::Scope& scope); static ClusterLoadReportStats generateLoadReportStats(Stats::Scope& scope); @@ -375,17 +374,12 @@ class ClusterInfoImpl : public ClusterInfo, const LoadBalancerSubsetInfo& lbSubsetInfo() const override { return lb_subset_; } const envoy::api::v2::core::Metadata& metadata() const override { return metadata_; } - // Server::Configuration::TransportSocketFactoryContext - Ssl::ContextManager& sslContextManager() override { return ssl_context_manager_; } - const Network::ConnectionSocket::OptionsSharedPtr& clusterSocketOptions() const override { return cluster_socket_options_; }; bool drainConnectionsOnHostRemoval() const override { return drain_connections_on_host_removal_; } - Secret::SecretManager& secretManager() override { return secret_manager_; } - private: struct ResourceManagers { ResourceManagers(const envoy::api::v2::Cluster& config, Runtime::Loader& runtime, @@ -406,11 +400,11 @@ class ClusterInfoImpl : public ClusterInfo, const std::chrono::milliseconds connect_timeout_; absl::optional idle_timeout_; const uint32_t per_connection_buffer_limit_bytes_; + Network::TransportSocketFactoryPtr transport_socket_factory_; Stats::ScopePtr stats_scope_; mutable ClusterStats stats_; Stats::IsolatedStoreImpl load_report_stats_store_; mutable ClusterLoadReportStats load_report_stats_; - Network::TransportSocketFactoryPtr transport_socket_factory_; const uint64_t features_; const Http::Http2Settings http2_settings_; mutable ResourceManagers resource_managers_; @@ -418,14 +412,12 @@ class ClusterInfoImpl : public ClusterInfo, const Network::Address::InstanceConstSharedPtr source_address_; LoadBalancerType lb_type_; absl::optional lb_ring_hash_config_; - Ssl::ContextManager& ssl_context_manager_; const bool added_via_api_; LoadBalancerSubsetInfoImpl lb_subset_; const envoy::api::v2::core::Metadata metadata_; const envoy::api::v2::Cluster::CommonLbConfig common_lb_config_; const Network::ConnectionSocket::OptionsSharedPtr cluster_socket_options_; const bool drain_connections_on_host_removal_; - Secret::SecretManager& secret_manager_; }; /** @@ -478,10 +470,9 @@ class ClusterImplBase : public Cluster, protected Logger::Loggable callback) override; protected: - ClusterImplBase(const envoy::api::v2::Cluster& cluster, - const envoy::api::v2::core::BindConfig& bind_config, Runtime::Loader& runtime, - Stats::Store& stats, Ssl::ContextManager& ssl_context_manager, - Secret::SecretManager& secret_manager, bool added_via_api); + ClusterImplBase(const envoy::api::v2::Cluster& cluster, Runtime::Loader& runtime, + Server::Configuration::TransportSocketFactoryContext& factory_context, + Stats::ScopePtr&& stats_scope, bool added_via_api); /** * Overridden by every concrete cluster. The cluster should do whatever pre-init is needed. E.g., @@ -576,8 +567,8 @@ typedef std::unique_ptr PriorityStateManagerPtr; class StaticClusterImpl : public ClusterImplBase { public: StaticClusterImpl(const envoy::api::v2::Cluster& cluster, Runtime::Loader& runtime, - Stats::Store& stats, Ssl::ContextManager& ssl_context_manager, - const LocalInfo::LocalInfo& local_info, ClusterManager& cm, bool added_via_api); + Server::Configuration::TransportSocketFactoryContext& factory_context, + Stats::ScopePtr&& stats_scope, bool added_via_api); // Upstream::Cluster InitializePhase initializePhase() const override { return InitializePhase::Primary; } @@ -607,10 +598,9 @@ class BaseDynamicClusterImpl : public ClusterImplBase { class StrictDnsClusterImpl : public BaseDynamicClusterImpl { public: StrictDnsClusterImpl(const envoy::api::v2::Cluster& cluster, Runtime::Loader& runtime, - Stats::Store& stats, Ssl::ContextManager& ssl_context_manager, - const LocalInfo::LocalInfo& local_info, - Network::DnsResolverSharedPtr dns_resolver, ClusterManager& cm, - Event::Dispatcher& dispatcher, bool added_via_api); + Network::DnsResolverSharedPtr dns_resolver, + Server::Configuration::TransportSocketFactoryContext& factory_context, + Stats::ScopePtr&& stats_scope, bool added_via_api); // Upstream::Cluster InitializePhase initializePhase() const override { return InitializePhase::Primary; } diff --git a/source/server/BUILD b/source/server/BUILD index 1f02f4424c4cf..130b048592a22 100644 --- a/source/server/BUILD +++ b/source/server/BUILD @@ -217,6 +217,7 @@ envoy_cc_library( ":drain_manager_lib", ":init_manager_lib", ":lds_api_lib", + ":transport_socket_config_lib", "//include/envoy/server:filter_config_interface", "//include/envoy/server:listener_manager_interface", "//include/envoy/server:transport_socket_config_interface", @@ -349,3 +350,11 @@ envoy_cc_library( "//source/common/common:thread_lib", ], ) + +envoy_cc_library( + name = "transport_socket_config_lib", + hdrs = ["transport_socket_config_impl.h"], + deps = [ + "//include/envoy/server:transport_socket_config_interface", + ], +) diff --git a/source/server/listener_manager_impl.cc b/source/server/listener_manager_impl.cc index 3e3d25bb4f185..5008eaafe462b 100644 --- a/source/server/listener_manager_impl.cc +++ b/source/server/listener_manager_impl.cc @@ -17,6 +17,7 @@ #include "server/configuration_impl.h" #include "server/drain_manager_impl.h" +#include "server/transport_socket_config_impl.h" #include "extensions/filters/listener/well_known_names.h" #include "extensions/filters/network/well_known_names.h" @@ -225,12 +226,15 @@ ListenerImpl::ListenerImpl(const envoy::api::v2::Listener& config, const std::st std::vector application_protocols( filter_chain_match.application_protocols().begin(), filter_chain_match.application_protocols().end()); - - addFilterChain(PROTOBUF_GET_WRAPPED_OR_DEFAULT(filter_chain_match, destination_port, 0), - destination_ips, server_names, filter_chain_match.transport_protocol(), - application_protocols, - config_factory.createTransportSocketFactory(*message, *this, server_names), - parent_.factory_.createNetworkFilterFactoryList(filter_chain.filters(), *this)); + Server::Configuration::TransportSocketFactoryContextImpl factory_context( + parent_.server_.sslContextManager(), *listener_scope_, parent_.server_.clusterManager(), + parent_.server_.localInfo(), parent_.server_.dispatcher(), parent_.server_.random(), + parent_.server_.stats()); + addFilterChain( + PROTOBUF_GET_WRAPPED_OR_DEFAULT(filter_chain_match, destination_port, 0), destination_ips, + server_names, filter_chain_match.transport_protocol(), application_protocols, + config_factory.createTransportSocketFactory(*message, factory_context, server_names), + parent_.factory_.createNetworkFilterFactoryList(filter_chain.filters(), *this)); need_tls_inspector |= filter_chain_match.transport_protocol() == "tls" || (filter_chain_match.transport_protocol().empty() && diff --git a/source/server/listener_manager_impl.h b/source/server/listener_manager_impl.h index 14dfadf30ab58..89193b7def039 100644 --- a/source/server/listener_manager_impl.h +++ b/source/server/listener_manager_impl.h @@ -190,7 +190,6 @@ class ListenerImpl : public Network::ListenerConfig, public Network::DrainDecision, public Network::FilterChainManager, public Network::FilterChainFactory, - public Configuration::TransportSocketFactoryContext, Logger::Loggable { public: /** @@ -301,11 +300,6 @@ class ListenerImpl : public Network::ListenerConfig, const std::vector& factories) override; bool createListenerFilterChain(Network::ListenerFilterManager& manager) override; - // Configuration::TransportSocketFactoryContext - Ssl::ContextManager& sslContextManager() override { return parent_.server_.sslContextManager(); } - Stats::Scope& statsScope() const override { return *listener_scope_; } - Secret::SecretManager& secretManager() override { return parent_.server_.secretManager(); } - SystemTime last_updated_; private: diff --git a/source/server/transport_socket_config_impl.h b/source/server/transport_socket_config_impl.h new file mode 100644 index 0000000000000..74e50d35e6b00 --- /dev/null +++ b/source/server/transport_socket_config_impl.h @@ -0,0 +1,52 @@ +#pragma once + +#include "envoy/server/transport_socket_config.h" + +namespace Envoy { +namespace Server { +namespace Configuration { + +/** + * Implementation of TransportSocketFactoryContext. + */ +class TransportSocketFactoryContextImpl : public TransportSocketFactoryContext { +public: + TransportSocketFactoryContextImpl(Ssl::ContextManager& context_manager, Stats::Scope& stats_scope, + Upstream::ClusterManager& cm, + const LocalInfo::LocalInfo& local_info, + Event::Dispatcher& dispatcher, + Envoy::Runtime::RandomGenerator& random, Stats::Store& stats) + : context_manager_(context_manager), stats_scope_(stats_scope), cluster_manager_(cm), + local_info_(local_info), dispatcher_(dispatcher), random_(random), stats_(stats) {} + + Ssl::ContextManager& sslContextManager() override { return context_manager_; } + + Stats::Scope& statsScope() const override { return stats_scope_; } + + Upstream::ClusterManager& clusterManager() override { return cluster_manager_; } + + Secret::SecretManager& secretManager() override { + return cluster_manager_.clusterManagerFactory().secretManager(); + } + + const LocalInfo::LocalInfo& localInfo() override { return local_info_; } + + Event::Dispatcher& dispatcher() override { return dispatcher_; } + + Envoy::Runtime::RandomGenerator& random() override { return random_; } + + Stats::Store& stats() override { return stats_; } + +private: + Ssl::ContextManager& context_manager_; + Stats::Scope& stats_scope_; + Upstream::ClusterManager& cluster_manager_; + const LocalInfo::LocalInfo& local_info_; + Event::Dispatcher& dispatcher_; + Envoy::Runtime::RandomGenerator& random_; + Stats::Store& stats_; +}; + +} // namespace Configuration +} // namespace Server +} // namespace Envoy diff --git a/test/common/upstream/BUILD b/test/common/upstream/BUILD index 441ce2437613f..789dd66c28a1a 100644 --- a/test/common/upstream/BUILD +++ b/test/common/upstream/BUILD @@ -71,6 +71,7 @@ envoy_cc_test( "//source/common/config:utility_lib", "//source/common/upstream:eds_lib", "//source/extensions/transport_sockets/raw_buffer:config", + "//source/server:transport_socket_config_lib", "//test/mocks/local_info:local_info_mocks", "//test/mocks/runtime:runtime_mocks", "//test/mocks/ssl:ssl_mocks", @@ -171,6 +172,7 @@ envoy_cc_test( "//source/common/upstream:logical_dns_cluster_lib", "//source/common/upstream:upstream_lib", "//source/extensions/transport_sockets/raw_buffer:config", + "//source/server:transport_socket_config_lib", "//test/mocks:common_lib", "//test/mocks/local_info:local_info_mocks", "//test/mocks/network:network_mocks", @@ -193,8 +195,10 @@ envoy_cc_test( "//source/common/upstream:upstream_lib", "//source/extensions/transport_sockets/raw_buffer:config", "//test/mocks:common_lib", + "//test/mocks/local_info:local_info_mocks", "//test/mocks/network:network_mocks", "//test/mocks/runtime:runtime_mocks", + "//test/mocks/server:server_mocks", "//test/mocks/ssl:ssl_mocks", "//test/mocks/upstream:upstream_mocks", "//test/test_common:utility_lib", @@ -282,6 +286,7 @@ envoy_cc_test( "//source/common/network:utility_lib", "//source/common/upstream:eds_lib", "//source/extensions/transport_sockets/raw_buffer:config", + "//source/server:transport_socket_config_lib", "//test/mocks/local_info:local_info_mocks", "//test/mocks/runtime:runtime_mocks", "//test/mocks/ssl:ssl_mocks", @@ -326,6 +331,7 @@ envoy_cc_test( "//source/common/upstream:upstream_includes", "//source/common/upstream:upstream_lib", "//source/extensions/transport_sockets/raw_buffer:config", + "//source/server:transport_socket_config_lib", "//test/mocks:common_lib", "//test/mocks/local_info:local_info_mocks", "//test/mocks/network:network_mocks", diff --git a/test/common/upstream/eds_test.cc b/test/common/upstream/eds_test.cc index 1d8555da76228..33dd33578df34 100644 --- a/test/common/upstream/eds_test.cc +++ b/test/common/upstream/eds_test.cc @@ -5,6 +5,8 @@ #include "common/config/utility.h" #include "common/upstream/eds.h" +#include "server/transport_socket_config_impl.h" + #include "test/common/upstream/utility.h" #include "test/mocks/local_info/mocks.h" #include "test/mocks/runtime/mocks.h" @@ -51,8 +53,14 @@ class EdsTest : public testing::Test { EXPECT_CALL(cm_, clusters()).WillOnce(Return(cluster_map)); EXPECT_CALL(cluster, info()).Times(2); EXPECT_CALL(*cluster.info_, addedViaApi()); - cluster_.reset(new EdsClusterImpl(eds_cluster_, runtime_, stats_, ssl_context_manager_, - local_info_, cm_, dispatcher_, random_, false)); + Envoy::Stats::ScopePtr scope = stats_.createScope( + fmt::format("cluster.{}.", eds_cluster_.alt_stat_name().empty() + ? eds_cluster_.name() + : std::string(eds_cluster_.alt_stat_name()))); + Envoy::Server::Configuration::TransportSocketFactoryContextImpl factory_context( + ssl_context_manager_, *scope, cm_, local_info_, dispatcher_, random_, stats_); + cluster_.reset( + new EdsClusterImpl(eds_cluster_, runtime_, factory_context, std::move(scope), false)); EXPECT_EQ(Cluster::InitializePhase::Secondary, cluster_->initializePhase()); } diff --git a/test/common/upstream/logical_dns_cluster_test.cc b/test/common/upstream/logical_dns_cluster_test.cc index 80a5c7b705fc5..ba927f178985f 100644 --- a/test/common/upstream/logical_dns_cluster_test.cc +++ b/test/common/upstream/logical_dns_cluster_test.cc @@ -7,6 +7,8 @@ #include "common/network/utility.h" #include "common/upstream/logical_dns_cluster.h" +#include "server/transport_socket_config_impl.h" + #include "test/common/upstream/utility.h" #include "test/mocks/common.h" #include "test/mocks/local_info/mocks.h" @@ -34,9 +36,15 @@ class LogicalDnsClusterTest : public testing::Test { void setupFromV1Json(const std::string& json) { resolve_timer_ = new Event::MockTimer(&dispatcher_); NiceMock cm; - cluster_.reset(new LogicalDnsCluster(parseClusterFromJson(json), runtime_, stats_store_, - ssl_context_manager_, local_info_, dns_resolver_, tls_, cm, - dispatcher_, false)); + envoy::api::v2::Cluster cluster_config = parseClusterFromJson(json); + Envoy::Stats::ScopePtr scope = stats_store_.createScope( + fmt::format("cluster.{}.", cluster_config.alt_stat_name().empty() + ? cluster_config.name() + : std::string(cluster_config.alt_stat_name()))); + Envoy::Server::Configuration::TransportSocketFactoryContextImpl factory_context( + ssl_context_manager_, *scope, cm, local_info_, dispatcher_, random_, stats_store_); + cluster_.reset(new LogicalDnsCluster(cluster_config, runtime_, dns_resolver_, tls_, + factory_context, std::move(scope), false)); cluster_->prioritySet().addMemberUpdateCb( [&](uint32_t, const HostVector&, const HostVector&) -> void { membership_updated_.ready(); @@ -47,9 +55,15 @@ class LogicalDnsClusterTest : public testing::Test { void setupFromV2Yaml(const std::string& yaml) { resolve_timer_ = new Event::MockTimer(&dispatcher_); NiceMock cm; - cluster_.reset(new LogicalDnsCluster(parseClusterFromV2Yaml(yaml), runtime_, stats_store_, - ssl_context_manager_, local_info_, dns_resolver_, tls_, cm, - dispatcher_, false)); + envoy::api::v2::Cluster cluster_config = parseClusterFromV2Yaml(yaml); + Envoy::Stats::ScopePtr scope = stats_store_.createScope( + fmt::format("cluster.{}.", cluster_config.alt_stat_name().empty() + ? cluster_config.name() + : std::string(cluster_config.alt_stat_name()))); + Envoy::Server::Configuration::TransportSocketFactoryContextImpl factory_context( + ssl_context_manager_, *scope, cm, local_info_, dispatcher_, random_, stats_store_); + cluster_.reset(new LogicalDnsCluster(cluster_config, runtime_, dns_resolver_, tls_, + factory_context, std::move(scope), false)); cluster_->prioritySet().addMemberUpdateCb( [&](uint32_t, const HostVector&, const HostVector&) -> void { membership_updated_.ready(); @@ -168,6 +182,7 @@ class LogicalDnsClusterTest : public testing::Test { std::shared_ptr> dns_resolver_{ new NiceMock}; Network::MockActiveDnsQuery active_dns_query_; + NiceMock random_; Network::DnsResolver::ResolveCb dns_callback_; NiceMock tls_; Event::MockTimer* resolve_timer_; diff --git a/test/common/upstream/original_dst_cluster_test.cc b/test/common/upstream/original_dst_cluster_test.cc index 098723c4d0ddc..a15109db7cf50 100644 --- a/test/common/upstream/original_dst_cluster_test.cc +++ b/test/common/upstream/original_dst_cluster_test.cc @@ -9,8 +9,11 @@ #include "common/upstream/original_dst_cluster.h" #include "common/upstream/upstream_impl.h" +#include "server/transport_socket_config_impl.h" + #include "test/common/upstream/utility.h" #include "test/mocks/common.h" +#include "test/mocks/local_info/mocks.h" #include "test/mocks/network/mocks.h" #include "test/mocks/runtime/mocks.h" #include "test/mocks/ssl/mocks.h" @@ -59,8 +62,15 @@ class OriginalDstClusterTest : public testing::Test { void setup(const std::string& json) { NiceMock cm; - cluster_.reset(new OriginalDstCluster(parseClusterFromJson(json), runtime_, stats_store_, - ssl_context_manager_, cm, dispatcher_, false)); + envoy::api::v2::Cluster cluster_config = parseClusterFromJson(json); + Envoy::Stats::ScopePtr scope = stats_store_.createScope( + fmt::format("cluster.{}.", cluster_config.alt_stat_name().empty() + ? cluster_config.name() + : std::string(cluster_config.alt_stat_name()))); + Envoy::Server::Configuration::TransportSocketFactoryContextImpl factory_context( + ssl_context_manager_, *scope, cm, local_info_, dispatcher_, random_, stats_store_); + cluster_.reset( + new OriginalDstCluster(cluster_config, runtime_, factory_context, std::move(scope), false)); cluster_->prioritySet().addMemberUpdateCb( [&](uint32_t, const HostVector&, const HostVector&) -> void { membership_updated_.ready(); @@ -76,6 +86,8 @@ class OriginalDstClusterTest : public testing::Test { NiceMock runtime_; NiceMock dispatcher_; Event::MockTimer* cleanup_timer_; + NiceMock random_; + NiceMock local_info_; }; TEST(OriginalDstClusterConfigTest, BadConfig) { diff --git a/test/common/upstream/sds_test.cc b/test/common/upstream/sds_test.cc index cd01ae7539ccb..85a7dfb180b54 100644 --- a/test/common/upstream/sds_test.cc +++ b/test/common/upstream/sds_test.cc @@ -13,6 +13,8 @@ #include "common/protobuf/protobuf.h" #include "common/upstream/eds.h" +#include "server/transport_socket_config_impl.h" + #include "test/common/upstream/utility.h" #include "test/mocks/local_info/mocks.h" #include "test/mocks/runtime/mocks.h" @@ -30,6 +32,7 @@ using testing::InSequence; using testing::Invoke; using testing::NiceMock; using testing::Return; +using testing::ReturnRef; using testing::SaveArg; using testing::WithArg; using testing::_; @@ -62,8 +65,14 @@ class SdsTest : public testing::Test { EXPECT_CALL(cm_, clusters()).WillOnce(Return(cluster_map)); EXPECT_CALL(cluster, info()).Times(2); EXPECT_CALL(*cluster.info_, addedViaApi()); - cluster_.reset(new EdsClusterImpl(sds_cluster_, runtime_, stats_, ssl_context_manager_, - local_info_, cm_, dispatcher_, random_, false)); + Envoy::Stats::ScopePtr scope = stats_.createScope( + fmt::format("cluster.{}.", sds_cluster_.alt_stat_name().empty() + ? sds_cluster_.name() + : std::string(sds_cluster_.alt_stat_name()))); + Envoy::Server::Configuration::TransportSocketFactoryContextImpl factory_context( + ssl_context_manager_, *scope, cm_, local_info_, dispatcher_, random_, stats_); + cluster_.reset( + new EdsClusterImpl(sds_cluster_, runtime_, factory_context, std::move(scope), false)); EXPECT_EQ(Cluster::InitializePhase::Secondary, cluster_->initializePhase()); } diff --git a/test/common/upstream/upstream_impl_test.cc b/test/common/upstream/upstream_impl_test.cc index f4b43c393d2d6..4334b46717143 100644 --- a/test/common/upstream/upstream_impl_test.cc +++ b/test/common/upstream/upstream_impl_test.cc @@ -15,6 +15,8 @@ #include "common/network/utility.h" #include "common/upstream/upstream_impl.h" +#include "server/transport_socket_config_impl.h" + #include "test/common/upstream/utility.h" #include "test/mocks/common.h" #include "test/mocks/local_info/mocks.h" @@ -30,6 +32,7 @@ using testing::ContainerEq; using testing::Invoke; using testing::NiceMock; +using testing::ReturnRef; using testing::_; namespace Envoy { @@ -122,7 +125,7 @@ TEST_P(StrictDnsParamTest, ImmediateResolve) { NiceMock runtime; NiceMock local_info; ReadyWatcher initialized; - + NiceMock random; const std::string json = R"EOF( { "name": "name", @@ -142,8 +145,16 @@ TEST_P(StrictDnsParamTest, ImmediateResolve) { return nullptr; })); NiceMock cm; - StrictDnsClusterImpl cluster(parseClusterFromJson(json), runtime, stats, ssl_context_manager, - local_info, dns_resolver, cm, dispatcher, false); + envoy::api::v2::Cluster cluster_config = parseClusterFromJson(json); + Envoy::Stats::ScopePtr scope = stats.createScope( + fmt::format("cluster.{}.", cluster_config.alt_stat_name().empty() + ? cluster_config.name() + : std::string(cluster_config.alt_stat_name()))); + Envoy::Server::Configuration::TransportSocketFactoryContextImpl factory_context( + ssl_context_manager, *scope, cm, local_info, dispatcher, random, stats); + + StrictDnsClusterImpl cluster(cluster_config, runtime, dns_resolver, factory_context, + std::move(scope), false); cluster.initialize([&]() -> void { initialized.ready(); }); EXPECT_EQ(2UL, cluster.prioritySet().hostSetsPerPriority()[0]->hosts().size()); EXPECT_EQ(2UL, cluster.prioritySet().hostSetsPerPriority()[0]->healthyHosts().size()); @@ -158,6 +169,7 @@ TEST(StrictDnsClusterImplTest, ZeroHostsHealthChecker) { NiceMock runtime; NiceMock cm; NiceMock local_info; + NiceMock random; ReadyWatcher initialized; const std::string yaml = R"EOF( @@ -169,8 +181,15 @@ TEST(StrictDnsClusterImplTest, ZeroHostsHealthChecker) { )EOF"; ResolverData resolver(*dns_resolver, dispatcher); - StrictDnsClusterImpl cluster(parseClusterFromV2Yaml(yaml), runtime, stats, ssl_context_manager, - local_info, dns_resolver, cm, dispatcher, false); + envoy::api::v2::Cluster cluster_config = parseClusterFromV2Yaml(yaml); + Envoy::Stats::ScopePtr scope = stats.createScope( + fmt::format("cluster.{}.", cluster_config.alt_stat_name().empty() + ? cluster_config.name() + : std::string(cluster_config.alt_stat_name()))); + Envoy::Server::Configuration::TransportSocketFactoryContextImpl factory_context( + ssl_context_manager, *scope, cm, local_info, dispatcher, random, stats); + StrictDnsClusterImpl cluster(cluster_config, runtime, dns_resolver, factory_context, + std::move(scope), false); std::shared_ptr health_checker(new MockHealthChecker()); EXPECT_CALL(*health_checker, start()); EXPECT_CALL(*health_checker, addHostCheckCompleteCb(_)); @@ -192,7 +211,7 @@ TEST(StrictDnsClusterImplTest, Basic) { NiceMock dispatcher; NiceMock runtime; NiceMock local_info; - + NiceMock random; // gmock matches in LIFO order which is why these are swapped. ResolverData resolver2(*dns_resolver, dispatcher); ResolverData resolver1(*dns_resolver, dispatcher); @@ -228,8 +247,15 @@ TEST(StrictDnsClusterImplTest, Basic) { )EOF"; NiceMock cm; - StrictDnsClusterImpl cluster(parseClusterFromJson(json), runtime, stats, ssl_context_manager, - local_info, dns_resolver, cm, dispatcher, false); + envoy::api::v2::Cluster cluster_config = parseClusterFromJson(json); + Envoy::Stats::ScopePtr scope = stats.createScope( + fmt::format("cluster.{}.", cluster_config.alt_stat_name().empty() + ? cluster_config.name() + : std::string(cluster_config.alt_stat_name()))); + Envoy::Server::Configuration::TransportSocketFactoryContextImpl factory_context( + ssl_context_manager, *scope, cm, local_info, dispatcher, random, stats); + StrictDnsClusterImpl cluster(cluster_config, runtime, dns_resolver, factory_context, + std::move(scope), false); EXPECT_CALL(runtime.snapshot_, getInteger("circuit_breakers.name.default.max_connections", 43)); EXPECT_EQ(43U, cluster.info()->resourceManager(ResourcePriority::Default).connections().max()); EXPECT_CALL(runtime.snapshot_, @@ -334,6 +360,7 @@ TEST(StrictDnsClusterImplTest, HostRemovalActiveHealthSkipped) { NiceMock runtime; NiceMock cm; NiceMock local_info; + NiceMock random; const std::string yaml = R"EOF( name: name @@ -345,8 +372,15 @@ TEST(StrictDnsClusterImplTest, HostRemovalActiveHealthSkipped) { )EOF"; ResolverData resolver(*dns_resolver, dispatcher); - StrictDnsClusterImpl cluster(parseClusterFromV2Yaml(yaml), runtime, stats, ssl_context_manager, - local_info, dns_resolver, cm, dispatcher, false); + envoy::api::v2::Cluster cluster_config = parseClusterFromV2Yaml(yaml); + Envoy::Stats::ScopePtr scope = stats.createScope( + fmt::format("cluster.{}.", cluster_config.alt_stat_name().empty() + ? cluster_config.name() + : std::string(cluster_config.alt_stat_name()))); + Envoy::Server::Configuration::TransportSocketFactoryContextImpl factory_context( + ssl_context_manager, *scope, cm, local_info, dispatcher, random, stats); + StrictDnsClusterImpl cluster(cluster_config, runtime, dns_resolver, factory_context, + std::move(scope), false); std::shared_ptr health_checker(new MockHealthChecker()); EXPECT_CALL(*health_checker, start()); EXPECT_CALL(*health_checker, addHostCheckCompleteCb(_)); @@ -384,7 +418,7 @@ TEST(StrictDnsClusterImplTest, LoadAssignmentBasic) { NiceMock dispatcher; NiceMock runtime; NiceMock local_info; - + NiceMock random; // gmock matches in LIFO order which is why these are swapped. ResolverData resolver2(*dns_resolver, dispatcher); ResolverData resolver1(*dns_resolver, dispatcher); @@ -437,8 +471,16 @@ TEST(StrictDnsClusterImplTest, LoadAssignmentBasic) { )EOF"; NiceMock cm; - StrictDnsClusterImpl cluster(parseClusterFromV2Yaml(yaml), runtime, stats, ssl_context_manager, - local_info, dns_resolver, cm, dispatcher, false); + envoy::api::v2::Cluster cluster_config = parseClusterFromV2Yaml(yaml); + Envoy::Stats::ScopePtr scope = stats.createScope( + fmt::format("cluster.{}.", cluster_config.alt_stat_name().empty() + ? cluster_config.name() + : std::string(cluster_config.alt_stat_name()))); + Envoy::Server::Configuration::TransportSocketFactoryContextImpl factory_context( + ssl_context_manager, *scope, cm, local_info, dispatcher, random, stats); + StrictDnsClusterImpl cluster(cluster_config, runtime, dns_resolver, factory_context, + std::move(scope), false); + EXPECT_CALL(runtime.snapshot_, getInteger("circuit_breakers.name.default.max_connections", 43)); EXPECT_EQ(43U, cluster.info()->resourceManager(ResourcePriority::Default).connections().max()); EXPECT_CALL(runtime.snapshot_, @@ -540,7 +582,7 @@ TEST(StrictDnsClusterImplTest, LoadAssignmentBasicMultiplePriorities) { NiceMock dispatcher; NiceMock runtime; NiceMock local_info; - + NiceMock random; // gmock matches in LIFO order which is why these are swapped. ResolverData resolver3(*dns_resolver, dispatcher); ResolverData resolver2(*dns_resolver, dispatcher); @@ -587,9 +629,15 @@ TEST(StrictDnsClusterImplTest, LoadAssignmentBasicMultiplePriorities) { )EOF"; NiceMock cm; - StrictDnsClusterImpl cluster(parseClusterFromV2Yaml(yaml), runtime, stats, ssl_context_manager, - local_info, dns_resolver, cm, dispatcher, false); - + envoy::api::v2::Cluster cluster_config = parseClusterFromV2Yaml(yaml); + Envoy::Stats::ScopePtr scope = stats.createScope( + fmt::format("cluster.{}.", cluster_config.alt_stat_name().empty() + ? cluster_config.name() + : std::string(cluster_config.alt_stat_name()))); + Envoy::Server::Configuration::TransportSocketFactoryContextImpl factory_context( + ssl_context_manager, *scope, cm, local_info, dispatcher, random, stats); + StrictDnsClusterImpl cluster(cluster_config, runtime, dns_resolver, factory_context, + std::move(scope), false); ReadyWatcher membership_updated; cluster.prioritySet().addMemberUpdateCb( [&](uint32_t, const HostVector&, const HostVector&) -> void { membership_updated.ready(); }); @@ -719,8 +767,11 @@ TEST(HostImplTest, HostnameCanaryAndLocality) { TEST(StaticClusterImplTest, InitialHosts) { Stats::IsolatedStoreImpl stats; Ssl::MockContextManager ssl_context_manager; + NiceMock dispatcher; NiceMock runtime; NiceMock local_info; + NiceMock random; + const std::string yaml = R"EOF( name: staticcluster connect_timeout: 0.25s @@ -733,8 +784,14 @@ TEST(StaticClusterImplTest, InitialHosts) { )EOF"; NiceMock cm; - StaticClusterImpl cluster(parseClusterFromV2Yaml(yaml), runtime, stats, ssl_context_manager, - local_info, cm, false); + envoy::api::v2::Cluster cluster_config = parseClusterFromV2Yaml(yaml); + Envoy::Stats::ScopePtr scope = stats.createScope( + fmt::format("cluster.{}.", cluster_config.alt_stat_name().empty() + ? cluster_config.name() + : std::string(cluster_config.alt_stat_name()))); + Envoy::Server::Configuration::TransportSocketFactoryContextImpl factory_context( + ssl_context_manager, *scope, cm, local_info, dispatcher, random, stats); + StaticClusterImpl cluster(cluster_config, runtime, factory_context, std::move(scope), false); cluster.initialize([] {}); EXPECT_EQ(1UL, cluster.prioritySet().hostSetsPerPriority()[0]->healthyHosts().size()); @@ -747,6 +804,8 @@ TEST(StaticClusterImplTest, EmptyHostname) { Ssl::MockContextManager ssl_context_manager; NiceMock runtime; NiceMock local_info; + NiceMock dispatcher; + NiceMock random; const std::string json = R"EOF( { @@ -759,8 +818,14 @@ TEST(StaticClusterImplTest, EmptyHostname) { )EOF"; NiceMock cm; - StaticClusterImpl cluster(parseClusterFromJson(json), runtime, stats, ssl_context_manager, - local_info, cm, false); + envoy::api::v2::Cluster cluster_config = parseClusterFromJson(json); + Envoy::Stats::ScopePtr scope = stats.createScope( + fmt::format("cluster.{}.", cluster_config.alt_stat_name().empty() + ? cluster_config.name() + : std::string(cluster_config.alt_stat_name()))); + Envoy::Server::Configuration::TransportSocketFactoryContextImpl factory_context( + ssl_context_manager, *scope, cm, local_info, dispatcher, random, stats); + StaticClusterImpl cluster(cluster_config, runtime, factory_context, std::move(scope), false); cluster.initialize([] {}); EXPECT_EQ(1UL, cluster.prioritySet().hostSetsPerPriority()[0]->healthyHosts().size()); @@ -771,9 +836,10 @@ TEST(StaticClusterImplTest, EmptyHostname) { TEST(StaticClusterImplTest, LoadAssignmentEmptyHostname) { Stats::IsolatedStoreImpl stats; Ssl::MockContextManager ssl_context_manager; + NiceMock dispatcher; NiceMock runtime; NiceMock local_info; - + NiceMock random; const std::string yaml = R"EOF( name: staticcluster connect_timeout: 0.25s @@ -792,8 +858,14 @@ TEST(StaticClusterImplTest, LoadAssignmentEmptyHostname) { )EOF"; NiceMock cm; - StaticClusterImpl cluster(parseClusterFromV2Yaml(yaml), runtime, stats, ssl_context_manager, - local_info, cm, false); + envoy::api::v2::Cluster cluster_config = parseClusterFromV2Yaml(yaml); + Envoy::Stats::ScopePtr scope = stats.createScope( + fmt::format("cluster.{}.", cluster_config.alt_stat_name().empty() + ? cluster_config.name() + : std::string(cluster_config.alt_stat_name()))); + Envoy::Server::Configuration::TransportSocketFactoryContextImpl factory_context( + ssl_context_manager, *scope, cm, local_info, dispatcher, random, stats); + StaticClusterImpl cluster(cluster_config, runtime, factory_context, std::move(scope), false); cluster.initialize([] {}); EXPECT_EQ(1UL, cluster.prioritySet().hostSetsPerPriority()[0]->healthyHosts().size()); @@ -806,6 +878,8 @@ TEST(StaticClusterImplTest, LoadAssignmentMultiplePriorities) { Ssl::MockContextManager ssl_context_manager; NiceMock runtime; NiceMock local_info; + NiceMock dispatcher; + NiceMock random; const std::string yaml = R"EOF( name: staticcluster @@ -843,8 +917,14 @@ TEST(StaticClusterImplTest, LoadAssignmentMultiplePriorities) { )EOF"; NiceMock cm; - StaticClusterImpl cluster(parseClusterFromV2Yaml(yaml), runtime, stats, ssl_context_manager, - local_info, cm, false); + envoy::api::v2::Cluster cluster_config = parseClusterFromV2Yaml(yaml); + Envoy::Stats::ScopePtr scope = stats.createScope( + fmt::format("cluster.{}.", cluster_config.alt_stat_name().empty() + ? cluster_config.name() + : std::string(cluster_config.alt_stat_name()))); + Envoy::Server::Configuration::TransportSocketFactoryContextImpl factory_context( + ssl_context_manager, *scope, cm, local_info, dispatcher, random, stats); + StaticClusterImpl cluster(cluster_config, runtime, factory_context, std::move(scope), false); cluster.initialize([] {}); EXPECT_EQ(2UL, cluster.prioritySet().hostSetsPerPriority()[0]->healthyHosts().size()); @@ -858,6 +938,8 @@ TEST(StaticClusterImplTest, LoadAssignmentLocality) { Ssl::MockContextManager ssl_context_manager; NiceMock runtime; NiceMock local_info; + NiceMock dispatcher; + NiceMock random; const std::string yaml = R"EOF( name: staticcluster @@ -888,8 +970,14 @@ TEST(StaticClusterImplTest, LoadAssignmentLocality) { )EOF"; NiceMock cm; - StaticClusterImpl cluster(parseClusterFromV2Yaml(yaml), runtime, stats, ssl_context_manager, - local_info, cm, false); + envoy::api::v2::Cluster cluster_config = parseClusterFromV2Yaml(yaml); + Envoy::Stats::ScopePtr scope = stats.createScope( + fmt::format("cluster.{}.", cluster_config.alt_stat_name().empty() + ? cluster_config.name() + : std::string(cluster_config.alt_stat_name()))); + Envoy::Server::Configuration::TransportSocketFactoryContextImpl factory_context( + ssl_context_manager, *scope, cm, local_info, dispatcher, random, stats); + StaticClusterImpl cluster(cluster_config, runtime, factory_context, std::move(scope), false); cluster.initialize([] {}); auto& hosts = cluster.prioritySet().hostSetsPerPriority()[0]->hosts(); @@ -909,6 +997,8 @@ TEST(StaticClusterImplTest, AltStatName) { Ssl::MockContextManager ssl_context_manager; NiceMock runtime; NiceMock local_info; + NiceMock random; + NiceMock dispatcher; const std::string yaml = R"EOF( name: staticcluster @@ -920,8 +1010,14 @@ TEST(StaticClusterImplTest, AltStatName) { )EOF"; NiceMock cm; - StaticClusterImpl cluster(parseClusterFromV2Yaml(yaml), runtime, stats, ssl_context_manager, - local_info, cm, false); + envoy::api::v2::Cluster cluster_config = parseClusterFromV2Yaml(yaml); + Envoy::Stats::ScopePtr scope = stats.createScope( + fmt::format("cluster.{}.", cluster_config.alt_stat_name().empty() + ? cluster_config.name() + : std::string(cluster_config.alt_stat_name()))); + Envoy::Server::Configuration::TransportSocketFactoryContextImpl factory_context( + ssl_context_manager, *scope, cm, local_info, dispatcher, random, stats); + StaticClusterImpl cluster(cluster_config, runtime, factory_context, std::move(scope), false); cluster.initialize([] {}); // Increment a stat and verify it is emitted with alt_stat_name cluster.info()->stats().upstream_rq_total_.inc(); @@ -933,6 +1029,8 @@ TEST(StaticClusterImplTest, RingHash) { Ssl::MockContextManager ssl_context_manager; NiceMock runtime; NiceMock local_info; + NiceMock dispatcher; + NiceMock random; const std::string json = R"EOF( { @@ -945,8 +1043,14 @@ TEST(StaticClusterImplTest, RingHash) { )EOF"; NiceMock cm; - StaticClusterImpl cluster(parseClusterFromJson(json), runtime, stats, ssl_context_manager, - local_info, cm, true); + envoy::api::v2::Cluster cluster_config = parseClusterFromJson(json); + Envoy::Stats::ScopePtr scope = stats.createScope( + fmt::format("cluster.{}.", cluster_config.alt_stat_name().empty() + ? cluster_config.name() + : std::string(cluster_config.alt_stat_name()))); + Envoy::Server::Configuration::TransportSocketFactoryContextImpl factory_context( + ssl_context_manager, *scope, cm, local_info, dispatcher, random, stats); + StaticClusterImpl cluster(cluster_config, runtime, factory_context, std::move(scope), true); cluster.initialize([] {}); EXPECT_EQ(1UL, cluster.prioritySet().hostSetsPerPriority()[0]->healthyHosts().size()); @@ -959,6 +1063,8 @@ TEST(StaticClusterImplTest, OutlierDetector) { Ssl::MockContextManager ssl_context_manager; NiceMock runtime; NiceMock local_info; + NiceMock random; + NiceMock dispatcher; const std::string json = R"EOF( { @@ -972,8 +1078,14 @@ TEST(StaticClusterImplTest, OutlierDetector) { )EOF"; NiceMock cm; - StaticClusterImpl cluster(parseClusterFromJson(json), runtime, stats, ssl_context_manager, - local_info, cm, false); + envoy::api::v2::Cluster cluster_config = parseClusterFromJson(json); + Envoy::Stats::ScopePtr scope = stats.createScope( + fmt::format("cluster.{}.", cluster_config.alt_stat_name().empty() + ? cluster_config.name() + : std::string(cluster_config.alt_stat_name()))); + Envoy::Server::Configuration::TransportSocketFactoryContextImpl factory_context( + ssl_context_manager, *scope, cm, local_info, dispatcher, random, stats); + StaticClusterImpl cluster(cluster_config, runtime, factory_context, std::move(scope), false); Outlier::MockDetector* detector = new Outlier::MockDetector(); EXPECT_CALL(*detector, addChangedStateCb(_)); @@ -1008,6 +1120,8 @@ TEST(StaticClusterImplTest, HealthyStat) { Ssl::MockContextManager ssl_context_manager; NiceMock runtime; NiceMock local_info; + NiceMock dispatcher; + NiceMock random; const std::string json = R"EOF( { @@ -1021,8 +1135,14 @@ TEST(StaticClusterImplTest, HealthyStat) { )EOF"; NiceMock cm; - StaticClusterImpl cluster(parseClusterFromJson(json), runtime, stats, ssl_context_manager, - local_info, cm, false); + envoy::api::v2::Cluster cluster_config = parseClusterFromJson(json); + Envoy::Stats::ScopePtr scope = stats.createScope( + fmt::format("cluster.{}.", cluster_config.alt_stat_name().empty() + ? cluster_config.name() + : std::string(cluster_config.alt_stat_name()))); + Envoy::Server::Configuration::TransportSocketFactoryContextImpl factory_context( + ssl_context_manager, *scope, cm, local_info, dispatcher, random, stats); + StaticClusterImpl cluster(cluster_config, runtime, factory_context, std::move(scope), false); Outlier::MockDetector* outlier_detector = new NiceMock(); cluster.setOutlierDetector(Outlier::DetectorSharedPtr{outlier_detector}); @@ -1092,6 +1212,8 @@ TEST(StaticClusterImplTest, UrlConfig) { Ssl::MockContextManager ssl_context_manager; NiceMock runtime; NiceMock local_info; + NiceMock dispatcher; + NiceMock random; const std::string json = R"EOF( { @@ -1105,8 +1227,14 @@ TEST(StaticClusterImplTest, UrlConfig) { )EOF"; NiceMock cm; - StaticClusterImpl cluster(parseClusterFromJson(json), runtime, stats, ssl_context_manager, - local_info, cm, false); + envoy::api::v2::Cluster cluster_config = parseClusterFromJson(json); + Envoy::Stats::ScopePtr scope = stats.createScope( + fmt::format("cluster.{}.", cluster_config.alt_stat_name().empty() + ? cluster_config.name() + : std::string(cluster_config.alt_stat_name()))); + Envoy::Server::Configuration::TransportSocketFactoryContextImpl factory_context( + ssl_context_manager, *scope, cm, local_info, dispatcher, random, stats); + StaticClusterImpl cluster(cluster_config, runtime, factory_context, std::move(scope), false); cluster.initialize([] {}); EXPECT_EQ(1024U, cluster.info()->resourceManager(ResourcePriority::Default).connections().max()); @@ -1138,6 +1266,8 @@ TEST(StaticClusterImplTest, UnsupportedLBType) { NiceMock runtime; NiceMock cm; NiceMock local_info; + NiceMock dispatcher; + NiceMock random; const std::string json = R"EOF( { @@ -1150,9 +1280,22 @@ TEST(StaticClusterImplTest, UnsupportedLBType) { } )EOF"; - EXPECT_THROW(StaticClusterImpl(parseClusterFromJson(json), runtime, stats, ssl_context_manager, - local_info, cm, false), - EnvoyException); + EXPECT_THROW_WITH_MESSAGE( + { + envoy::api::v2::Cluster cluster_config = parseClusterFromJson(json); + Envoy::Stats::ScopePtr scope = stats.createScope( + fmt::format("cluster.{}.", cluster_config.alt_stat_name().empty() + ? cluster_config.name() + : std::string(cluster_config.alt_stat_name()))); + Envoy::Server::Configuration::TransportSocketFactoryContextImpl factory_context( + ssl_context_manager, *scope, cm, local_info, dispatcher, random, stats); + StaticClusterImpl(cluster_config, runtime, factory_context, std::move(scope), false); + }, + EnvoyException, + "JSON at lines 2-9 does not conform to schema.\n" + " Invalid schema: #/properties/lb_type\n" + " Schema violation: enum\n" + " Offending document key: #/lb_type"); } TEST(StaticClusterImplTest, MalformedHostIP) { @@ -1160,6 +1303,8 @@ TEST(StaticClusterImplTest, MalformedHostIP) { Ssl::MockContextManager ssl_context_manager; NiceMock runtime; NiceMock local_info; + NiceMock dispatcher; + NiceMock random; const std::string yaml = R"EOF( name: name @@ -1170,11 +1315,18 @@ TEST(StaticClusterImplTest, MalformedHostIP) { )EOF"; NiceMock cm; - EXPECT_THROW_WITH_MESSAGE(StaticClusterImpl(parseClusterFromV2Yaml(yaml), runtime, stats, - ssl_context_manager, local_info, cm, false), - EnvoyException, - "malformed IP address: foo.bar.com. Consider setting resolver_name or " - "setting cluster type to 'STRICT_DNS' or 'LOGICAL_DNS'"); + envoy::api::v2::Cluster cluster_config = parseClusterFromV2Yaml(yaml); + Envoy::Stats::ScopePtr scope = stats.createScope( + fmt::format("cluster.{}.", cluster_config.alt_stat_name().empty() + ? cluster_config.name() + : std::string(cluster_config.alt_stat_name()))); + Envoy::Server::Configuration::TransportSocketFactoryContextImpl factory_context( + ssl_context_manager, *scope, cm, local_info, dispatcher, random, stats); + EXPECT_THROW_WITH_MESSAGE( + StaticClusterImpl(cluster_config, runtime, factory_context, std::move(scope), false), + EnvoyException, + "malformed IP address: foo.bar.com. Consider setting resolver_name or " + "setting cluster type to 'STRICT_DNS' or 'LOGICAL_DNS'"); } TEST(ClusterDefinitionTest, BadClusterConfig) { @@ -1214,6 +1366,8 @@ TEST(StaticClusterImplTest, SourceAddressPriority) { Ssl::MockContextManager ssl_context_manager; NiceMock runtime; NiceMock local_info; + NiceMock dispatcher; + NiceMock random; envoy::api::v2::Cluster config; config.set_name("staticcluster"); @@ -1223,7 +1377,12 @@ TEST(StaticClusterImplTest, SourceAddressPriority) { // If the cluster manager gets a source address from the bootstrap proto, use it. NiceMock cm; cm.bind_config_.mutable_source_address()->set_address("1.2.3.5"); - StaticClusterImpl cluster(config, runtime, stats, ssl_context_manager, local_info, cm, false); + Envoy::Stats::ScopePtr scope = stats.createScope(fmt::format( + "cluster.{}.", + config.alt_stat_name().empty() ? config.name() : std::string(config.alt_stat_name()))); + Envoy::Server::Configuration::TransportSocketFactoryContextImpl factory_context( + ssl_context_manager, *scope, cm, local_info, dispatcher, random, stats); + StaticClusterImpl cluster(config, runtime, factory_context, std::move(scope), false); EXPECT_EQ("1.2.3.5:0", cluster.info()->sourceAddress()->asString()); } @@ -1232,7 +1391,12 @@ TEST(StaticClusterImplTest, SourceAddressPriority) { { // Verify source address from cluster config is used when present. NiceMock cm; - StaticClusterImpl cluster(config, runtime, stats, ssl_context_manager, local_info, cm, false); + Envoy::Stats::ScopePtr scope = stats.createScope(fmt::format( + "cluster.{}.", + config.alt_stat_name().empty() ? config.name() : std::string(config.alt_stat_name()))); + Envoy::Server::Configuration::TransportSocketFactoryContextImpl factory_context( + ssl_context_manager, *scope, cm, local_info, dispatcher, random, stats); + StaticClusterImpl cluster(config, runtime, factory_context, std::move(scope), false); EXPECT_EQ(cluster_address, cluster.info()->sourceAddress()->ip()->addressAsString()); } @@ -1240,7 +1404,12 @@ TEST(StaticClusterImplTest, SourceAddressPriority) { // The source address from cluster config takes precedence over one from the bootstrap proto. NiceMock cm; cm.bind_config_.mutable_source_address()->set_address("1.2.3.5"); - StaticClusterImpl cluster(config, runtime, stats, ssl_context_manager, local_info, cm, false); + Envoy::Stats::ScopePtr scope = stats.createScope(fmt::format( + "cluster.{}.", + config.alt_stat_name().empty() ? config.name() : std::string(config.alt_stat_name()))); + Envoy::Server::Configuration::TransportSocketFactoryContextImpl factory_context( + ssl_context_manager, *scope, cm, local_info, dispatcher, random, stats); + StaticClusterImpl cluster(config, runtime, factory_context, std::move(scope), false); EXPECT_EQ(cluster_address, cluster.info()->sourceAddress()->ip()->addressAsString()); } } @@ -1255,6 +1424,7 @@ TEST(ClusterImplTest, CloseConnectionsOnHostHealthFailure) { NiceMock runtime; NiceMock cm; NiceMock local_info; + NiceMock random; ReadyWatcher initialized; const std::string yaml = R"EOF( @@ -1265,8 +1435,16 @@ TEST(ClusterImplTest, CloseConnectionsOnHostHealthFailure) { close_connections_on_host_health_failure: true hosts: [{ socket_address: { address: foo.bar.com, port_value: 443 }}] )EOF"; - StrictDnsClusterImpl cluster(parseClusterFromV2Yaml(yaml), runtime, stats, ssl_context_manager, - local_info, dns_resolver, cm, dispatcher, false); + envoy::api::v2::Cluster cluster_config = parseClusterFromV2Yaml(yaml); + Envoy::Stats::ScopePtr scope = stats.createScope( + fmt::format("cluster.{}.", cluster_config.alt_stat_name().empty() + ? cluster_config.name() + : std::string(cluster_config.alt_stat_name()))); + Envoy::Server::Configuration::TransportSocketFactoryContextImpl factory_context( + ssl_context_manager, *scope, cm, local_info, dispatcher, random, stats); + + StrictDnsClusterImpl cluster(cluster_config, runtime, dns_resolver, factory_context, + std::move(scope), false); EXPECT_TRUE(cluster.info()->features() & ClusterInfo::Features::CLOSE_CONNECTIONS_ON_HOST_HEALTH_FAILURE); } @@ -1329,6 +1507,7 @@ TEST(ClusterMetadataTest, Metadata) { NiceMock runtime; NiceMock cm; NiceMock local_info; + NiceMock random; ReadyWatcher initialized; const std::string yaml = R"EOF( @@ -1343,8 +1522,15 @@ TEST(ClusterMetadataTest, Metadata) { value: 0.3 )EOF"; - StrictDnsClusterImpl cluster(parseClusterFromV2Yaml(yaml), runtime, stats, ssl_context_manager, - local_info, dns_resolver, cm, dispatcher, false); + envoy::api::v2::Cluster cluster_config = parseClusterFromV2Yaml(yaml); + Envoy::Stats::ScopePtr scope = stats.createScope( + fmt::format("cluster.{}.", cluster_config.alt_stat_name().empty() + ? cluster_config.name() + : std::string(cluster_config.alt_stat_name()))); + Envoy::Server::Configuration::TransportSocketFactoryContextImpl factory_context( + ssl_context_manager, *scope, cm, local_info, dispatcher, random, stats); + StrictDnsClusterImpl cluster(cluster_config, runtime, dns_resolver, factory_context, + std::move(scope), false); EXPECT_EQ("test_value", Config::Metadata::metadataValue(cluster.info()->metadata(), "com.bar.foo", "baz") .string_value()); diff --git a/test/mocks/server/mocks.h b/test/mocks/server/mocks.h index 17029e659303f..f214f5d8efa6a 100644 --- a/test/mocks/server/mocks.h +++ b/test/mocks/server/mocks.h @@ -411,6 +411,12 @@ class MockTransportSocketFactoryContext : public TransportSocketFactoryContext { MOCK_METHOD0(sslContextManager, Ssl::ContextManager&()); MOCK_CONST_METHOD0(statsScope, Stats::Scope&()); + MOCK_METHOD0(clusterManager, Upstream::ClusterManager&()); + MOCK_METHOD0(secretManager, Secret::SecretManager&()); + MOCK_METHOD0(local_info, const LocalInfo::LocalInfo&()); + MOCK_METHOD0(dispatcher, Event::Dispatcher&()); + MOCK_METHOD0(random, Envoy::Runtime::RandomGenerator&()); + MOCK_METHOD0(stats, Stats::Store&()); }; class MockListenerFactoryContext : public virtual MockFactoryContext, From cb3356fc5ec26b5d1482f5356735222993253a02 Mon Sep 17 00:00:00 2001 From: Wayne Zhang Date: Thu, 2 Aug 2018 15:29:42 -0700 Subject: [PATCH 03/11] Sds: Ssl socket factory owns ContextConfig (#4028) Signed-off-by: Wayne Zhang --- include/envoy/ssl/context_config.h | 4 + source/common/ssl/ssl_socket.cc | 11 ++- source/common/ssl/ssl_socket.h | 11 ++- .../transport_sockets/ssl/config.cc | 18 ++-- .../grpc_client_integration_test_harness.h | 10 +-- test/common/ssl/ssl_socket_test.cc | 87 ++++++++++--------- test/integration/ads_integration_test.cc | 4 +- .../sds_static_integration_test.cc | 4 +- test/integration/ssl_utility.cc | 4 +- test/integration/xfcc_integration_test.cc | 8 +- 10 files changed, 88 insertions(+), 73 deletions(-) diff --git a/include/envoy/ssl/context_config.h b/include/envoy/ssl/context_config.h index a56a89e903e6b..6e2236dfcaaa7 100644 --- a/include/envoy/ssl/context_config.h +++ b/include/envoy/ssl/context_config.h @@ -127,6 +127,8 @@ class ClientContextConfig : public virtual ContextConfig { virtual bool allowRenegotiation() const PURE; }; +typedef std::unique_ptr ClientContextConfigPtr; + class ServerContextConfig : public virtual ContextConfig { public: struct SessionTicketKey { @@ -148,5 +150,7 @@ class ServerContextConfig : public virtual ContextConfig { virtual const std::vector& sessionTicketKeys() const PURE; }; +typedef std::unique_ptr ServerContextConfigPtr; + } // namespace Ssl } // namespace Envoy diff --git a/source/common/ssl/ssl_socket.cc b/source/common/ssl/ssl_socket.cc index ec6d9967c6aa8..c04644281597a 100644 --- a/source/common/ssl/ssl_socket.cc +++ b/source/common/ssl/ssl_socket.cc @@ -382,10 +382,11 @@ std::string SslSocket::subjectLocalCertificate() const { return getSubjectFromCertificate(cert); } -ClientSslSocketFactory::ClientSslSocketFactory(const ClientContextConfig& config, +ClientSslSocketFactory::ClientSslSocketFactory(ClientContextConfigPtr config, Ssl::ContextManager& manager, Stats::Scope& stats_scope) - : ssl_ctx_(manager.createSslClientContext(stats_scope, config)) {} + : manager_(manager), stats_scope_(stats_scope), config_(std::move(config)), + ssl_ctx_(manager_.createSslClientContext(stats_scope_, *config_)) {} Network::TransportSocketPtr ClientSslSocketFactory::createTransportSocket() const { return std::make_unique(ssl_ctx_, Ssl::InitialState::Client); @@ -393,11 +394,13 @@ Network::TransportSocketPtr ClientSslSocketFactory::createTransportSocket() cons bool ClientSslSocketFactory::implementsSecureTransport() const { return true; } -ServerSslSocketFactory::ServerSslSocketFactory(const ServerContextConfig& config, +ServerSslSocketFactory::ServerSslSocketFactory(ServerContextConfigPtr config, Ssl::ContextManager& manager, Stats::Scope& stats_scope, const std::vector& server_names) - : ssl_ctx_(manager.createSslServerContext(stats_scope, config, server_names)) {} + : manager_(manager), stats_scope_(stats_scope), config_(std::move(config)), + server_names_(server_names), + ssl_ctx_(manager_.createSslServerContext(stats_scope_, *config_, server_names_)) {} Network::TransportSocketPtr ServerSslSocketFactory::createTransportSocket() const { return std::make_unique(ssl_ctx_, Ssl::InitialState::Server); diff --git a/source/common/ssl/ssl_socket.h b/source/common/ssl/ssl_socket.h index 68fec106eb916..70696c360006e 100644 --- a/source/common/ssl/ssl_socket.h +++ b/source/common/ssl/ssl_socket.h @@ -69,25 +69,32 @@ class SslSocket : public Network::TransportSocket, class ClientSslSocketFactory : public Network::TransportSocketFactory { public: - ClientSslSocketFactory(const ClientContextConfig& config, Ssl::ContextManager& manager, + ClientSslSocketFactory(ClientContextConfigPtr config, Ssl::ContextManager& manager, Stats::Scope& stats_scope); Network::TransportSocketPtr createTransportSocket() const override; bool implementsSecureTransport() const override; private: + Ssl::ContextManager& manager_; + Stats::Scope& stats_scope_; + ClientContextConfigPtr config_; ClientContextSharedPtr ssl_ctx_; }; class ServerSslSocketFactory : public Network::TransportSocketFactory { public: - ServerSslSocketFactory(const ServerContextConfig& config, Ssl::ContextManager& manager, + ServerSslSocketFactory(ServerContextConfigPtr config, Ssl::ContextManager& manager, Stats::Scope& stats_scope, const std::vector& server_names); Network::TransportSocketPtr createTransportSocket() const override; bool implementsSecureTransport() const override; private: + Ssl::ContextManager& manager_; + Stats::Scope& stats_scope_; + ServerContextConfigPtr config_; + const std::vector server_names_; ServerContextSharedPtr ssl_ctx_; }; diff --git a/source/extensions/transport_sockets/ssl/config.cc b/source/extensions/transport_sockets/ssl/config.cc index c0c0feec1970a..c3f69f301983c 100644 --- a/source/extensions/transport_sockets/ssl/config.cc +++ b/source/extensions/transport_sockets/ssl/config.cc @@ -16,12 +16,11 @@ namespace SslTransport { Network::TransportSocketFactoryPtr UpstreamSslSocketFactory::createTransportSocketFactory( const Protobuf::Message& message, Server::Configuration::TransportSocketFactoryContext& context) { + auto client_config = std::make_unique( + MessageUtil::downcastAndValidate(message), + context.secretManager()); return std::make_unique( - Ssl::ClientContextConfigImpl( - MessageUtil::downcastAndValidate( - message), - context.secretManager()), - context.sslContextManager(), context.statsScope()); + std::move(client_config), context.sslContextManager(), context.statsScope()); } ProtobufTypes::MessagePtr UpstreamSslSocketFactory::createEmptyConfigProto() { @@ -35,12 +34,11 @@ static Registry::RegisterFactory& server_names) { + auto server_config = std::make_unique( + MessageUtil::downcastAndValidate(message), + context.secretManager()); return std::make_unique( - Ssl::ServerContextConfigImpl( - MessageUtil::downcastAndValidate( - message), - context.secretManager()), - context.sslContextManager(), context.statsScope(), server_names); + std::move(server_config), context.sslContextManager(), context.statsScope(), server_names); } ProtobufTypes::MessagePtr DownstreamSslSocketFactory::createEmptyConfigProto() { diff --git a/test/common/grpc/grpc_client_integration_test_harness.h b/test/common/grpc/grpc_client_integration_test_harness.h index b13b802b3fc4c..7bb6c7a8426df 100644 --- a/test/common/grpc/grpc_client_integration_test_harness.h +++ b/test/common/grpc/grpc_client_integration_test_harness.h @@ -459,10 +459,10 @@ class GrpcSslClientIntegrationTest : public GrpcClientIntegrationTest { tls_cert->mutable_private_key()->set_filename( TestEnvironment::runfilesPath("test/config/integration/certs/clientkey.pem")); } - Ssl::ClientContextConfigImpl cfg(tls_context, secret_manager_); + auto cfg = std::make_unique(tls_context, secret_manager_); - mock_cluster_info_->transport_socket_factory_ = - std::make_unique(cfg, context_manager_, *stats_store_); + mock_cluster_info_->transport_socket_factory_ = std::make_unique( + std::move(cfg), context_manager_, *stats_store_); ON_CALL(*mock_cluster_info_, transportSocketFactory()) .WillByDefault(ReturnRef(*mock_cluster_info_->transport_socket_factory_)); async_client_transport_socket_ = @@ -489,11 +489,11 @@ class GrpcSslClientIntegrationTest : public GrpcClientIntegrationTest { TestEnvironment::runfilesPath("test/config/integration/certs/cacert.pem")); } - Ssl::ServerContextConfigImpl cfg(tls_context, secret_manager_); + auto cfg = std::make_unique(tls_context, secret_manager_); static Stats::Scope* upstream_stats_store = new Stats::IsolatedStoreImpl(); return std::make_unique( - cfg, context_manager_, *upstream_stats_store, std::vector{}); + std::move(cfg), context_manager_, *upstream_stats_store, std::vector{}); } bool use_client_cert_{}; diff --git a/test/common/ssl/ssl_socket_test.cc b/test/common/ssl/ssl_socket_test.cc index 9371041b1088c..4c0a9000a1376 100644 --- a/test/common/ssl/ssl_socket_test.cc +++ b/test/common/ssl/ssl_socket_test.cc @@ -56,9 +56,9 @@ void testUtil(const std::string& client_ctx_json, const std::string& server_ctx_ Secret::MockSecretManager secret_manager; Json::ObjectSharedPtr server_ctx_loader = TestEnvironment::jsonLoadFromString(server_ctx_json); - ServerContextConfigImpl server_ctx_config(*server_ctx_loader, secret_manager); + auto server_cfg = std::make_unique(*server_ctx_loader, secret_manager); ContextManagerImpl manager(runtime); - Ssl::ServerSslSocketFactory server_ssl_socket_factory(server_ctx_config, manager, stats_store, + Ssl::ServerSslSocketFactory server_ssl_socket_factory(std::move(server_cfg), manager, stats_store, std::vector{}); Event::DispatcherImpl dispatcher; @@ -69,8 +69,9 @@ void testUtil(const std::string& client_ctx_json, const std::string& server_ctx_ Network::ListenerPtr listener = dispatcher.createListener(socket, callbacks, true, false); Json::ObjectSharedPtr client_ctx_loader = TestEnvironment::jsonLoadFromString(client_ctx_json); - ClientContextConfigImpl client_ctx_config(*client_ctx_loader, secret_manager); - Ssl::ClientSslSocketFactory client_ssl_socket_factory(client_ctx_config, manager, stats_store); + auto client_cfg = std::make_unique(*client_ctx_loader, secret_manager); + Ssl::ClientSslSocketFactory client_ssl_socket_factory(std::move(client_cfg), manager, + stats_store); Network::ClientConnectionPtr client_connection = dispatcher.createClientConnection( socket.localAddress(), Network::Address::InstanceConstSharedPtr(), client_ssl_socket_factory.createTransportSocket(), nullptr); @@ -158,8 +159,9 @@ const std::string testUtilV2(const envoy::api::v2::Listener& server_proto, const auto& filter_chain = server_proto.filter_chains(0); std::vector server_names(filter_chain.filter_chain_match().server_names().begin(), filter_chain.filter_chain_match().server_names().end()); - Ssl::ServerContextConfigImpl server_ctx_config(filter_chain.tls_context(), secret_manager); - Ssl::ServerSslSocketFactory server_ssl_socket_factory(server_ctx_config, manager, stats_store, + auto server_cfg = + std::make_unique(filter_chain.tls_context(), secret_manager); + Ssl::ServerSslSocketFactory server_ssl_socket_factory(std::move(server_cfg), manager, stats_store, server_names); Event::DispatcherImpl dispatcher; @@ -169,9 +171,9 @@ const std::string testUtilV2(const envoy::api::v2::Listener& server_proto, Network::MockConnectionHandler connection_handler; Network::ListenerPtr listener = dispatcher.createListener(socket, callbacks, true, false); - ClientContextConfigImpl client_ctx_config(client_ctx_proto, secret_manager); - ClientSslSocketFactory client_ssl_socket_factory(client_ctx_config, manager, stats_store); - ClientContextSharedPtr client_ctx(manager.createSslClientContext(stats_store, client_ctx_config)); + auto client_cfg = std::make_unique(client_ctx_proto, secret_manager); + ClientContextSharedPtr client_ctx(manager.createSslClientContext(stats_store, *client_cfg)); + ClientSslSocketFactory client_ssl_socket_factory(std::move(client_cfg), manager, stats_store); Network::ClientConnectionPtr client_connection = dispatcher.createClientConnection( socket.localAddress(), Network::Address::InstanceConstSharedPtr(), client_ssl_socket_factory.createTransportSocket(), nullptr); @@ -1536,9 +1538,9 @@ TEST_P(SslSocketTest, FlushCloseDuringHandshake) { )EOF"; Json::ObjectSharedPtr server_ctx_loader = TestEnvironment::jsonLoadFromString(server_ctx_json); - ServerContextConfigImpl server_ctx_config(*server_ctx_loader, secret_manager_); + auto server_cfg = std::make_unique(*server_ctx_loader, secret_manager_); ContextManagerImpl manager(runtime); - Ssl::ServerSslSocketFactory server_ssl_socket_factory(server_ctx_config, manager, stats_store, + Ssl::ServerSslSocketFactory server_ssl_socket_factory(std::move(server_cfg), manager, stats_store, std::vector{}); Event::DispatcherImpl dispatcher; @@ -1594,9 +1596,9 @@ TEST_P(SslSocketTest, HalfClose) { )EOF"; Json::ObjectSharedPtr server_ctx_loader = TestEnvironment::jsonLoadFromString(server_ctx_json); - ServerContextConfigImpl server_ctx_config(*server_ctx_loader, secret_manager_); + auto server_cfg = std::make_unique(*server_ctx_loader, secret_manager_); ContextManagerImpl manager(runtime); - Ssl::ServerSslSocketFactory server_ssl_socket_factory(server_ctx_config, manager, stats_store, + Ssl::ServerSslSocketFactory server_ssl_socket_factory(std::move(server_cfg), manager, stats_store, std::vector{}); Event::DispatcherImpl dispatcher; @@ -1615,8 +1617,8 @@ TEST_P(SslSocketTest, HalfClose) { )EOF"; Json::ObjectSharedPtr client_ctx_loader = TestEnvironment::jsonLoadFromString(client_ctx_json); - ClientContextConfigImpl client_ctx_config(*client_ctx_loader, secret_manager_); - ClientSslSocketFactory client_ssl_socket_factory(client_ctx_config, manager, stats_store); + auto client_cfg = std::make_unique(*client_ctx_loader, secret_manager_); + ClientSslSocketFactory client_ssl_socket_factory(std::move(client_cfg), manager, stats_store); Network::ClientConnectionPtr client_connection = dispatcher.createClientConnection( socket.localAddress(), Network::Address::InstanceConstSharedPtr(), client_ssl_socket_factory.createTransportSocket(), nullptr); @@ -1678,9 +1680,9 @@ TEST_P(SslSocketTest, ClientAuthMultipleCAs) { )EOF"; Json::ObjectSharedPtr server_ctx_loader = TestEnvironment::jsonLoadFromString(server_ctx_json); - ServerContextConfigImpl server_ctx_config(*server_ctx_loader, secret_manager); + auto server_cfg = std::make_unique(*server_ctx_loader, secret_manager); ContextManagerImpl manager(runtime); - Ssl::ServerSslSocketFactory server_ssl_socket_factory(server_ctx_config, manager, stats_store, + Ssl::ServerSslSocketFactory server_ssl_socket_factory(std::move(server_cfg), manager, stats_store, std::vector{}); Event::DispatcherImpl dispatcher; @@ -1698,8 +1700,8 @@ TEST_P(SslSocketTest, ClientAuthMultipleCAs) { )EOF"; Json::ObjectSharedPtr client_ctx_loader = TestEnvironment::jsonLoadFromString(client_ctx_json); - ClientContextConfigImpl client_ctx_config(*client_ctx_loader, secret_manager); - ClientSslSocketFactory ssl_socket_factory(client_ctx_config, manager, stats_store); + auto client_cfg = std::make_unique(*client_ctx_loader, secret_manager); + ClientSslSocketFactory ssl_socket_factory(std::move(client_cfg), manager, stats_store); Network::ClientConnectionPtr client_connection = dispatcher.createClientConnection( socket.localAddress(), Network::Address::InstanceConstSharedPtr(), ssl_socket_factory.createTransportSocket(), nullptr); @@ -1760,12 +1762,12 @@ void testTicketSessionResumption(const std::string& server_ctx_json1, Json::ObjectSharedPtr server_ctx_loader1 = TestEnvironment::jsonLoadFromString(server_ctx_json1); Json::ObjectSharedPtr server_ctx_loader2 = TestEnvironment::jsonLoadFromString(server_ctx_json2); - ServerContextConfigImpl server_ctx_config1(*server_ctx_loader1, secret_manager); - ServerContextConfigImpl server_ctx_config2(*server_ctx_loader2, secret_manager); - Ssl::ServerSslSocketFactory server_ssl_socket_factory1(server_ctx_config1, manager, stats_store, - server_names1); - Ssl::ServerSslSocketFactory server_ssl_socket_factory2(server_ctx_config2, manager, stats_store, - server_names2); + auto server_cfg1 = std::make_unique(*server_ctx_loader1, secret_manager); + auto server_cfg2 = std::make_unique(*server_ctx_loader2, secret_manager); + Ssl::ServerSslSocketFactory server_ssl_socket_factory1(std::move(server_cfg1), manager, + stats_store, server_names1); + Ssl::ServerSslSocketFactory server_ssl_socket_factory2(std::move(server_cfg2), manager, + stats_store, server_names2); Event::DispatcherImpl dispatcher; Network::TcpListenSocket socket1(Network::Test::getCanonicalLoopbackAddress(ip_version), nullptr, @@ -1778,8 +1780,8 @@ void testTicketSessionResumption(const std::string& server_ctx_json1, Network::ListenerPtr listener2 = dispatcher.createListener(socket2, callbacks, true, false); Json::ObjectSharedPtr client_ctx_loader = TestEnvironment::jsonLoadFromString(client_ctx_json); - ClientContextConfigImpl client_ctx_config(*client_ctx_loader, secret_manager); - ClientSslSocketFactory ssl_socket_factory(client_ctx_config, manager, stats_store); + auto client_cfg = std::make_unique(*client_ctx_loader, secret_manager); + ClientSslSocketFactory ssl_socket_factory(std::move(client_cfg), manager, stats_store); Network::ClientConnectionPtr client_connection = dispatcher.createClientConnection( socket1.localAddress(), Network::Address::InstanceConstSharedPtr(), ssl_socket_factory.createTransportSocket(), nullptr); @@ -2118,14 +2120,15 @@ TEST_P(SslSocketTest, ClientAuthCrossListenerSessionResumption) { )EOF"; Json::ObjectSharedPtr server_ctx_loader = TestEnvironment::jsonLoadFromString(server_ctx_json); - ServerContextConfigImpl server_ctx_config(*server_ctx_loader, secret_manager_); + auto server_cfg = std::make_unique(*server_ctx_loader, secret_manager_); Json::ObjectSharedPtr server2_ctx_loader = TestEnvironment::jsonLoadFromString(server2_ctx_json); - ServerContextConfigImpl server2_ctx_config(*server2_ctx_loader, secret_manager_); + auto server2_cfg = + std::make_unique(*server2_ctx_loader, secret_manager_); ContextManagerImpl manager(runtime); - Ssl::ServerSslSocketFactory server_ssl_socket_factory(server_ctx_config, manager, stats_store, + Ssl::ServerSslSocketFactory server_ssl_socket_factory(std::move(server_cfg), manager, stats_store, std::vector{}); - Ssl::ServerSslSocketFactory server2_ssl_socket_factory(server2_ctx_config, manager, stats_store, - std::vector{}); + Ssl::ServerSslSocketFactory server2_ssl_socket_factory(std::move(server2_cfg), manager, + stats_store, std::vector{}); Event::DispatcherImpl dispatcher; Network::TcpListenSocket socket(Network::Test::getCanonicalLoopbackAddress(GetParam()), nullptr, @@ -2145,8 +2148,8 @@ TEST_P(SslSocketTest, ClientAuthCrossListenerSessionResumption) { )EOF"; Json::ObjectSharedPtr client_ctx_loader = TestEnvironment::jsonLoadFromString(client_ctx_json); - ClientContextConfigImpl client_ctx_config(*client_ctx_loader, secret_manager_); - ClientSslSocketFactory ssl_socket_factory(client_ctx_config, manager, stats_store); + auto client_cfg = std::make_unique(*client_ctx_loader, secret_manager_); + ClientSslSocketFactory ssl_socket_factory(std::move(client_cfg), manager, stats_store); Network::ClientConnectionPtr client_connection = dispatcher.createClientConnection( socket.localAddress(), Network::Address::InstanceConstSharedPtr(), ssl_socket_factory.createTransportSocket(), nullptr); @@ -2231,9 +2234,9 @@ TEST_P(SslSocketTest, SslError) { )EOF"; Json::ObjectSharedPtr server_ctx_loader = TestEnvironment::jsonLoadFromString(server_ctx_json); - ServerContextConfigImpl server_ctx_config(*server_ctx_loader, secret_manager_); + auto server_cfg = std::make_unique(*server_ctx_loader, secret_manager_); ContextManagerImpl manager(runtime); - Ssl::ServerSslSocketFactory server_ssl_socket_factory(server_ctx_config, manager, stats_store, + Ssl::ServerSslSocketFactory server_ssl_socket_factory(std::move(server_cfg), manager, stats_store, std::vector{}); Event::DispatcherImpl dispatcher; @@ -2574,18 +2577,20 @@ class SslReadBufferLimitTest : public SslCertsTest, public: void initialize() { server_ctx_loader_ = TestEnvironment::jsonLoadFromString(server_ctx_json_); - server_ctx_config_.reset(new ServerContextConfigImpl(*server_ctx_loader_, secret_manager_)); + auto server_cfg = + std::make_unique(*server_ctx_loader_, secret_manager_); manager_.reset(new ContextManagerImpl(runtime_)); server_ssl_socket_factory_.reset(new ServerSslSocketFactory( - *server_ctx_config_, *manager_, stats_store_, std::vector{})); + std::move(server_cfg), *manager_, stats_store_, std::vector{})); listener_ = dispatcher_->createListener(socket_, listener_callbacks_, true, false); client_ctx_loader_ = TestEnvironment::jsonLoadFromString(client_ctx_json_); - client_ctx_config_.reset(new ClientContextConfigImpl(*client_ctx_loader_, secret_manager_)); + auto client_cfg = + std::make_unique(*client_ctx_loader_, secret_manager_); client_ssl_socket_factory_.reset( - new ClientSslSocketFactory(*client_ctx_config_, *manager_, stats_store_)); + new ClientSslSocketFactory(std::move(client_cfg), *manager_, stats_store_)); client_connection_ = dispatcher_->createClientConnection( socket_.localAddress(), source_address_, client_ssl_socket_factory_->createTransportSocket(), nullptr); @@ -2749,12 +2754,10 @@ class SslReadBufferLimitTest : public SslCertsTest, )EOF"; Runtime::MockLoader runtime_; Json::ObjectSharedPtr server_ctx_loader_; - std::unique_ptr server_ctx_config_; std::unique_ptr manager_; Network::TransportSocketFactoryPtr server_ssl_socket_factory_; Network::ListenerPtr listener_; Json::ObjectSharedPtr client_ctx_loader_; - std::unique_ptr client_ctx_config_; ClientContextSharedPtr client_ctx_; Network::TransportSocketFactoryPtr client_ssl_socket_factory_; Network::ClientConnectionPtr client_connection_; diff --git a/test/integration/ads_integration_test.cc b/test/integration/ads_integration_test.cc index fb94edf25736c..99ddaae71cbd9 100644 --- a/test/integration/ads_integration_test.cc +++ b/test/integration/ads_integration_test.cc @@ -116,11 +116,11 @@ class AdsIntegrationTest : public AdsIntegrationBaseTest, TestEnvironment::runfilesPath("test/config/integration/certs/upstreamcert.pem")); tls_cert->mutable_private_key()->set_filename( TestEnvironment::runfilesPath("test/config/integration/certs/upstreamkey.pem")); - Ssl::ServerContextConfigImpl cfg(tls_context, secret_manager_); + auto cfg = std::make_unique(tls_context, secret_manager_); static Stats::Scope* upstream_stats_store = new Stats::TestIsolatedStoreImpl(); return std::make_unique( - cfg, context_manager_, *upstream_stats_store, std::vector{}); + std::move(cfg), context_manager_, *upstream_stats_store, std::vector{}); } AssertionResult diff --git a/test/integration/sds_static_integration_test.cc b/test/integration/sds_static_integration_test.cc index 28d017d242649..e46402b0b4293 100644 --- a/test/integration/sds_static_integration_test.cc +++ b/test/integration/sds_static_integration_test.cc @@ -166,11 +166,11 @@ class SdsStaticUpstreamIntegrationTest tls_certificate->mutable_private_key()->set_filename( TestEnvironment::runfilesPath("/test/config/integration/certs/serverkey.pem")); - Ssl::ServerContextConfigImpl cfg(tls_context, secret_manager_); + auto cfg = std::make_unique(tls_context, secret_manager_); static Stats::Scope* upstream_stats_store = new Stats::TestIsolatedStoreImpl(); return std::make_unique( - cfg, context_manager_, *upstream_stats_store, std::vector{}); + std::move(cfg), context_manager_, *upstream_stats_store, std::vector{}); } private: diff --git a/test/integration/ssl_utility.cc b/test/integration/ssl_utility.cc index 9c3d1e773b06d..bb6ccb9f20b10 100644 --- a/test/integration/ssl_utility.cc +++ b/test/integration/ssl_utility.cc @@ -59,10 +59,10 @@ createClientSslTransportSocketFactory(bool alpn, bool san, ContextManager& conte target = san ? json_san : json_plain; } Json::ObjectSharedPtr loader = TestEnvironment::jsonLoadFromString(target); - ClientContextConfigImpl cfg(*loader, secret_manager); + auto cfg = std::make_unique(*loader, secret_manager); static auto* client_stats_store = new Stats::TestIsolatedStoreImpl(); return Network::TransportSocketFactoryPtr{ - new Ssl::ClientSslSocketFactory(cfg, context_manager, *client_stats_store)}; + new Ssl::ClientSslSocketFactory(std::move(cfg), context_manager, *client_stats_store)}; } Network::Address::InstanceConstSharedPtr getSslAddress(const Network::Address::IpVersion& version, diff --git a/test/integration/xfcc_integration_test.cc b/test/integration/xfcc_integration_test.cc index a2d55546a25e1..782673fe9d5e6 100644 --- a/test/integration/xfcc_integration_test.cc +++ b/test/integration/xfcc_integration_test.cc @@ -60,10 +60,10 @@ Network::TransportSocketFactoryPtr XfccIntegrationTest::createClientSslContext(b target = json_tls; } Json::ObjectSharedPtr loader = TestEnvironment::jsonLoadFromString(target); - Ssl::ClientContextConfigImpl cfg(*loader, secret_manager_); + auto cfg = std::make_unique(*loader, secret_manager_); static auto* client_stats_store = new Stats::TestIsolatedStoreImpl(); return Network::TransportSocketFactoryPtr{ - new Ssl::ClientSslSocketFactory(cfg, *context_manager_, *client_stats_store)}; + new Ssl::ClientSslSocketFactory(std::move(cfg), *context_manager_, *client_stats_store)}; } Network::TransportSocketFactoryPtr XfccIntegrationTest::createUpstreamSslContext() { @@ -75,10 +75,10 @@ Network::TransportSocketFactoryPtr XfccIntegrationTest::createUpstreamSslContext )EOF"; Json::ObjectSharedPtr loader = TestEnvironment::jsonLoadFromString(json); - Ssl::ServerContextConfigImpl cfg(*loader, secret_manager_); + auto cfg = std::make_unique(*loader, secret_manager_); static Stats::Scope* upstream_stats_store = new Stats::TestIsolatedStoreImpl(); return std::make_unique( - cfg, *context_manager_, *upstream_stats_store, std::vector{}); + std::move(cfg), *context_manager_, *upstream_stats_store, std::vector{}); } Network::ClientConnectionPtr XfccIntegrationTest::makeClientConnection() { From ddd661ac027cc8a9c1b995aaf0a425ef77bc1b43 Mon Sep 17 00:00:00 2001 From: Julia Evans Date: Thu, 2 Aug 2018 15:36:55 -0700 Subject: [PATCH 04/11] hot restarter: Log errno for 'panic: cannot open shared memory' error (#4032) The error message right now assumes that the errno is EACCESS which isn't necessarily true. Logging the errno makes this easier to debug. Signed-off-by: Julia Evans --- source/server/hot_restart_impl.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/source/server/hot_restart_impl.cc b/source/server/hot_restart_impl.cc index a082eba18a6fd..a4be237f272b2 100644 --- a/source/server/hot_restart_impl.cc +++ b/source/server/hot_restart_impl.cc @@ -57,7 +57,8 @@ SharedMemory& SharedMemory::initialize(uint64_t stats_set_size, Options& options int shmem_fd = os_sys_calls.shmOpen(shmem_name.c_str(), flags, S_IRUSR | S_IWUSR); if (shmem_fd == -1) { - PANIC(fmt::format("cannot open shared memory region {} check user permissions", shmem_name)); + PANIC(fmt::format("cannot open shared memory region {} check user permissions. Error: {}", + shmem_name, strerror(errno))); } if (options.restartEpoch() == 0) { From 4c3219c0cfb8ab169eca2bf39590c2a6bac5fbfb Mon Sep 17 00:00:00 2001 From: Matt Klein Date: Thu, 2 Aug 2018 18:21:33 -0700 Subject: [PATCH 05/11] owners: promote Stephan and Greg to senior maintainer! (#4039) More responsibility, yay! ;) Signed-off-by: Matt Klein --- OWNERS.md | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/OWNERS.md b/OWNERS.md index 2fba8fb3a2be5..cf7e947c23fe2 100644 --- a/OWNERS.md +++ b/OWNERS.md @@ -14,6 +14,11 @@ routing PRs, questions, etc. to the right place. * Alyssa Wilk ([alyssawilk](https://github.com/alyssawilk)) (alyssar@google.com) * HTTP, flow control, cluster manager, load balancing, and core networking (listeners, connections, etc.). +* Stephan Zuercher ([zuercher](https://github.com/zuercher)) (stephan@turbinelabs.io) + * Load balancing, upstream clusters and cluster manager, logging, complex HTTP routing + (metadata, etc.), and OSX build. +* Greg Greenway ([ggreenway](https://github.com/ggreenway)) (ggreenway@apple.com) + * TCP proxy, TLS, logging, and core networking (listeners, connections, etc.). # Maintainers @@ -25,11 +30,6 @@ routing PRs, questions, etc. to the right place. * Base server (watchdog, workers, startup, stack trace handling, etc.). * Daniel Hochman ([danielhochman](https://github.com/danielhochman)) (dhochman@lyft.com) * Redis, Python, configuration/operational questions. -* Stephan Zuercher ([zuercher](https://github.com/zuercher)) (stephan@turbinelabs.io) - * Load balancing, upstream clusters and cluster manager, logging, complex HTTP routing - (metadata, etc.), and OSX build. -* Greg Greenway ([ggreenway](https://github.com/ggreenway)) (ggreenway@apple.com) - * TCP proxy, TLS, logging, and core networking (listeners, connections, etc.). * Lizan Zhou ([lizan](https://github.com/lizan)) (zlizan@google.com) * gRPC, gRPC/JSON transcoding, and core networking (transport socket abstractions). @@ -50,4 +50,4 @@ matter expert reviews. Feel free to loop them in as needed. * John Millikin ([jmillikin-stripe](https://github.com/jmillikin-stripe)) (jmillikin@stripe.com) * Bazel/build. * Joshua Marantz ([jmarantz](https://github.com/jmarantz)) (jmarantz@google.com) - * abseil and performance work. \ No newline at end of file + * abseil and performance work. From 55606ec3f0810683e9cab31b2f9db2f2cdb43ac0 Mon Sep 17 00:00:00 2001 From: cnorman Date: Thu, 2 Aug 2018 20:21:49 -0500 Subject: [PATCH 06/11] bump abseil-cpp commit (#4034) To pick up a fix for compiling on ppc64le (Power) Signed-off-by: Christy Norman --- bazel/repository_locations.bzl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bazel/repository_locations.bzl b/bazel/repository_locations.bzl index 7a65f4c956f3a..06f193eddc194 100644 --- a/bazel/repository_locations.bzl +++ b/bazel/repository_locations.bzl @@ -5,7 +5,7 @@ REPOSITORY_LOCATIONS = dict( remote = "https://github.com/google/boringssl", ), com_google_absl = dict( - commit = "92020a042c0cd46979db9f6f0cb32783dc07765e", # 2018-06-08 + commit = "92e07e5590752d6b8e67f7f2f86c6286561e8cea", # 2018-08-01 remote = "https://github.com/abseil/abseil-cpp", ), com_github_apache_thrift = dict( From fa628c44e74bbc3e70c371cfc01e83a002aebcb8 Mon Sep 17 00:00:00 2001 From: alyssawilk Date: Fri, 3 Aug 2018 10:40:16 -0400 Subject: [PATCH 07/11] logging: optional details for ASSERT (#3934) Allowing ASSERT to optionally use the details added to RELEASE_ASSERT in #3842 Risk Level: Low Testing: bazel //test/..., unit tests of new and old behavior. Docs Changes: n/a Release Notes: n/a Signed-off-by: Alyssa Wilk --- source/common/common/assert.h | 12 ++++++++++-- source/common/network/connection_impl.cc | 4 +++- test/common/common/assert_test.cc | 14 +++++++++++++- 3 files changed, 26 insertions(+), 4 deletions(-) diff --git a/source/common/common/assert.h b/source/common/common/assert.h index 9d4f4c5275215..3654a160df2ce 100644 --- a/source/common/common/assert.h +++ b/source/common/common/assert.h @@ -29,11 +29,19 @@ namespace Envoy { } while (false) #ifndef NDEBUG -#define ASSERT(X) RELEASE_ASSERT(X, "") +#define _ASSERT_ORIGINAL(X) RELEASE_ASSERT(X, "") +#define _ASSERT_VERBOSE(X, Y) RELEASE_ASSERT(X, Y) +#define _ASSERT_SELECTOR(_1, _2, ASSERT_MACRO, ...) ASSERT_MACRO + +// If ASSERT is called with one argument, the ASSERT_SELECTOR will return +// _ASSERT_ORIGINAL and this will call _ASSERT_ORIGINAL(__VA_ARGS__). +// If ASSERT is called with two arguments, ASSERT_SELECTOR will return +// _ASSERT_VERBOSE, and this will call _ASSERT_VERBOSE,(__VA_ARGS__) +#define ASSERT(...) _ASSERT_SELECTOR(__VA_ARGS__, _ASSERT_VERBOSE, _ASSERT_ORIGINAL)(__VA_ARGS__) #else // This non-implementation ensures that its argument is a valid expression that can be statically // casted to a bool, but the expression is never evaluated and will be compiled away. -#define ASSERT(X) \ +#define ASSERT(X, ...) \ do { \ constexpr bool __assert_dummy_variable = false && static_cast(X); \ (void)__assert_dummy_variable; \ diff --git a/source/common/network/connection_impl.cc b/source/common/network/connection_impl.cc index 073cead4dd664..aa921fa4593f9 100644 --- a/source/common/network/connection_impl.cc +++ b/source/common/network/connection_impl.cc @@ -504,7 +504,9 @@ void ConnectionImpl::onWriteReady() { } void ConnectionImpl::setConnectionStats(const ConnectionStats& stats) { - ASSERT(!connection_stats_); + ASSERT(!connection_stats_, + "Two network filters are attempting to set connection stats. This indicates an issue " + "with the configured filter chain."); connection_stats_.reset(new ConnectionStats(stats)); } diff --git a/test/common/common/assert_test.cc b/test/common/common/assert_test.cc index 49f3c8d7d748b..9143b1a6391e7 100644 --- a/test/common/common/assert_test.cc +++ b/test/common/common/assert_test.cc @@ -4,7 +4,7 @@ namespace Envoy { -TEST(Assert, VariousLogs) { +TEST(ReleaseAssert, VariousLogs) { Logger::StderrSinkDelegate stderr_sink(Logger::Registry::getSink()); // For coverage build. EXPECT_DEATH({ RELEASE_ASSERT(0, ""); }, ".*assert failure: 0.*"); EXPECT_DEATH({ RELEASE_ASSERT(0, "With some logs"); }, @@ -13,4 +13,16 @@ TEST(Assert, VariousLogs) { ".*assert failure: 0 == EAGAIN. Details: using fmt.*"); } +TEST(Assert, VariousLogs) { +#ifndef NDEBUG + EXPECT_DEATH({ ASSERT(0); }, ".*assert failure: 0.*"); + EXPECT_DEATH({ ASSERT(0, ""); }, ".*assert failure: 0.*"); + EXPECT_DEATH({ ASSERT(0, "With some logs"); }, ".*assert failure: 0. Details: With some logs.*"); +#else + ASSERT(0); + ASSERT(0, ""); + ASSERT(0, "With some logs"); +#endif +} + } // namespace Envoy From a3364380ae8dbfac692977f3d5a846b7498eb4ba Mon Sep 17 00:00:00 2001 From: Venil Noronha Date: Fri, 3 Aug 2018 08:48:01 -0700 Subject: [PATCH 08/11] rest-api: make request timeout configurable (#4006) The existing RestApiFetcher implementation used a hard-coded request timeout of 1s which wouldn't suit all environments. This commit enables the configuration of request timeout in the RestApiFetcher. Signed-off-by: Venil Noronha --- api/envoy/api/v2/core/config_source.proto | 4 +++ source/common/config/http_subscription_impl.h | 4 ++- source/common/config/subscription_factory.h | 1 + source/common/config/utility.cc | 6 +++++ source/common/config/utility.h | 8 ++++++ source/common/http/BUILD | 1 + source/common/http/rest_api_fetcher.cc | 27 ++++++++++++++----- source/common/http/rest_api_fetcher.h | 7 ++++- source/common/router/rds_subscription.cc | 16 +++-------- source/common/upstream/cds_subscription.cc | 13 ++------- source/common/upstream/sds_subscription.cc | 12 +-------- .../client_ssl_auth/client_ssl_auth.cc | 3 ++- source/server/lds_subscription.cc | 14 +++------- .../config/http_subscription_test_harness.h | 6 ++--- .../config/subscription_factory_test.cc | 21 +++++++++++++++ test/common/config/utility_test.cc | 12 +++++++++ test/common/upstream/sds_test.cc | 22 ++++++++++++++- 17 files changed, 118 insertions(+), 59 deletions(-) diff --git a/api/envoy/api/v2/core/config_source.proto b/api/envoy/api/v2/core/config_source.proto index 1ebb265da2ae4..b044c43a5311a 100644 --- a/api/envoy/api/v2/core/config_source.proto +++ b/api/envoy/api/v2/core/config_source.proto @@ -44,6 +44,10 @@ message ApiConfigSource { // For REST APIs, the delay between successive polls. google.protobuf.Duration refresh_delay = 3 [(gogoproto.stdduration) = true]; + + // For REST APIs, the request timeout. If not set, a default value of 1s will be used. + google.protobuf.Duration request_timeout = 5 + [(validate.rules).duration.gt.seconds = 0, (gogoproto.stdduration) = true]; } // Aggregated Discovery Service (ADS) options. This is currently empty, but when diff --git a/source/common/config/http_subscription_impl.h b/source/common/config/http_subscription_impl.h index 5e79c14ea5bff..b0d0ac94f0597 100644 --- a/source/common/config/http_subscription_impl.h +++ b/source/common/config/http_subscription_impl.h @@ -33,8 +33,10 @@ class HttpSubscriptionImpl : public Http::RestApiFetcher, HttpSubscriptionImpl(const envoy::api::v2::core::Node& node, Upstream::ClusterManager& cm, const std::string& remote_cluster_name, Event::Dispatcher& dispatcher, Runtime::RandomGenerator& random, std::chrono::milliseconds refresh_interval, + std::chrono::milliseconds request_timeout, const Protobuf::MethodDescriptor& service_method, SubscriptionStats stats) - : Http::RestApiFetcher(cm, remote_cluster_name, dispatcher, random, refresh_interval), + : Http::RestApiFetcher(cm, remote_cluster_name, dispatcher, random, refresh_interval, + request_timeout), stats_(stats) { request_.mutable_node()->CopyFrom(node); ASSERT(service_method.options().HasExtension(google::api::http)); diff --git a/source/common/config/subscription_factory.h b/source/common/config/subscription_factory.h index 25cbe119860c1..04b1b18a0ab25 100644 --- a/source/common/config/subscription_factory.h +++ b/source/common/config/subscription_factory.h @@ -60,6 +60,7 @@ class SubscriptionFactory { result.reset(new HttpSubscriptionImpl( node, cm, api_config_source.cluster_names()[0], dispatcher, random, Utility::apiConfigSourceRefreshDelay(api_config_source), + Utility::apiConfigSourceRequestTimeout(api_config_source), *Protobuf::DescriptorPool::generated_pool()->FindMethodByName(rest_method), stats)); break; case envoy::api::v2::core::ApiConfigSource::GRPC: { diff --git a/source/common/config/utility.cc b/source/common/config/utility.cc index 60d574619d7e8..5cdf9aaf8c9e3 100644 --- a/source/common/config/utility.cc +++ b/source/common/config/utility.cc @@ -159,6 +159,12 @@ std::chrono::milliseconds Utility::apiConfigSourceRefreshDelay( DurationUtil::durationToMilliseconds(api_config_source.refresh_delay())); } +std::chrono::milliseconds Utility::apiConfigSourceRequestTimeout( + const envoy::api::v2::core::ApiConfigSource& api_config_source) { + return std::chrono::milliseconds( + PROTOBUF_GET_MS_OR_DEFAULT(api_config_source, request_timeout, 1000)); +} + void Utility::translateEdsConfig(const Json::Object& json_config, envoy::api::v2::core::ConfigSource& eds_config) { translateApiConfigSource(json_config.getObject("cluster")->getString("name"), diff --git a/source/common/config/utility.h b/source/common/config/utility.h index 79086504ad94b..992baea6bee8d 100644 --- a/source/common/config/utility.h +++ b/source/common/config/utility.h @@ -74,6 +74,14 @@ class Utility { static std::chrono::milliseconds apiConfigSourceRefreshDelay(const envoy::api::v2::core::ApiConfigSource& api_config_source); + /** + * Extract request_timeout as a std::chrono::milliseconds from + * envoy::api::v2::core::ApiConfigSource. If request_timeout isn't set in the config source, a + * default value of 1s will be returned. + */ + static std::chrono::milliseconds + apiConfigSourceRequestTimeout(const envoy::api::v2::core::ApiConfigSource& api_config_source); + /** * Populate an envoy::api::v2::core::ApiConfigSource. * @param cluster supplies the cluster name for the ApiConfigSource. diff --git a/source/common/http/BUILD b/source/common/http/BUILD index 002c311e35bd3..900442c49b979 100644 --- a/source/common/http/BUILD +++ b/source/common/http/BUILD @@ -215,6 +215,7 @@ envoy_cc_library( "//include/envoy/runtime:runtime_interface", "//include/envoy/upstream:cluster_manager_interface", "//source/common/common:enum_to_int", + "//source/common/config:utility_lib", ], ) diff --git a/source/common/http/rest_api_fetcher.cc b/source/common/http/rest_api_fetcher.cc index f62f96f3a23b3..d79d4a37d2c43 100644 --- a/source/common/http/rest_api_fetcher.cc +++ b/source/common/http/rest_api_fetcher.cc @@ -5,17 +5,33 @@ #include #include "common/common/enum_to_int.h" +#include "common/config/utility.h" #include "common/http/message_impl.h" #include "common/http/utility.h" namespace Envoy { namespace Http { +RestApiFetcher::RestApiFetcher(Upstream::ClusterManager& cm, + const envoy::api::v2::core::ApiConfigSource& api_config_source, + Event::Dispatcher& dispatcher, Runtime::RandomGenerator& random) + : RestApiFetcher(cm, api_config_source.cluster_names()[0], dispatcher, random, + Config::Utility::apiConfigSourceRefreshDelay(api_config_source), + Config::Utility::apiConfigSourceRequestTimeout(api_config_source)) { + UNREFERENCED_PARAMETER(api_config_source); + // The ApiType must be REST_LEGACY for xDS implementations that call this constructor. + ASSERT(api_config_source.api_type() == envoy::api::v2::core::ApiConfigSource::REST_LEGACY); + // TODO(htuch): Add support for multiple clusters, #1170. + ASSERT(api_config_source.cluster_names().size() == 1); + ASSERT(api_config_source.has_refresh_delay()); +} + RestApiFetcher::RestApiFetcher(Upstream::ClusterManager& cm, const std::string& remote_cluster_name, Event::Dispatcher& dispatcher, Runtime::RandomGenerator& random, - std::chrono::milliseconds refresh_interval) + std::chrono::milliseconds refresh_interval, + std::chrono::milliseconds request_timeout) : remote_cluster_name_(remote_cluster_name), cm_(cm), random_(random), - refresh_interval_(refresh_interval), + refresh_interval_(refresh_interval), request_timeout_(request_timeout), refresh_timer_(dispatcher.createTimer([this]() -> void { refresh(); })) {} RestApiFetcher::~RestApiFetcher() { @@ -51,10 +67,9 @@ void RestApiFetcher::refresh() { MessagePtr message(new RequestMessageImpl()); createRequest(*message); message->headers().insertHost().value(remote_cluster_name_); - active_request_ = - cm_.httpAsyncClientForCluster(remote_cluster_name_) - .send(std::move(message), *this, - absl::optional(std::chrono::milliseconds(1000))); + active_request_ = cm_.httpAsyncClientForCluster(remote_cluster_name_) + .send(std::move(message), *this, + absl::optional(request_timeout_)); } void RestApiFetcher::requestComplete() { diff --git a/source/common/http/rest_api_fetcher.h b/source/common/http/rest_api_fetcher.h index 8c62e6f5aeb0c..fb9c5d9b2f8ec 100644 --- a/source/common/http/rest_api_fetcher.h +++ b/source/common/http/rest_api_fetcher.h @@ -16,9 +16,13 @@ namespace Http { */ class RestApiFetcher : public Http::AsyncClient::Callbacks { protected: + RestApiFetcher(Upstream::ClusterManager& cm, + const envoy::api::v2::core::ApiConfigSource& api_config_source, + Event::Dispatcher& dispatcher, Runtime::RandomGenerator& random); RestApiFetcher(Upstream::ClusterManager& cm, const std::string& remote_cluster_name, Event::Dispatcher& dispatcher, Runtime::RandomGenerator& random, - std::chrono::milliseconds refresh_interval); + std::chrono::milliseconds refresh_interval, + std::chrono::milliseconds request_timeout); ~RestApiFetcher(); /** @@ -63,6 +67,7 @@ class RestApiFetcher : public Http::AsyncClient::Callbacks { Runtime::RandomGenerator& random_; const std::chrono::milliseconds refresh_interval_; + const std::chrono::milliseconds request_timeout_; Event::TimerPtr refresh_timer_; Http::AsyncClient::Request* active_request_{}; }; diff --git a/source/common/router/rds_subscription.cc b/source/common/router/rds_subscription.cc index 53d8b172a069b..47f237ed09e34 100644 --- a/source/common/router/rds_subscription.cc +++ b/source/common/router/rds_subscription.cc @@ -17,20 +17,10 @@ RdsSubscription::RdsSubscription( const envoy::config::filter::network::http_connection_manager::v2::Rds& rds, Upstream::ClusterManager& cm, Event::Dispatcher& dispatcher, Runtime::RandomGenerator& random, const LocalInfo::LocalInfo& local_info, const Stats::Scope& scope) - : RestApiFetcher(cm, rds.config_source().api_config_source().cluster_names()[0], dispatcher, - random, - Envoy::Config::Utility::apiConfigSourceRefreshDelay( - rds.config_source().api_config_source())), + : RestApiFetcher(cm, rds.config_source().api_config_source(), dispatcher, random), local_info_(local_info), stats_(stats), scope_(scope) { - const auto& api_config_source = rds.config_source().api_config_source(); - UNREFERENCED_PARAMETER(api_config_source); - // If we are building an RdsSubscription, the ConfigSource should be REST_LEGACY. - ASSERT(api_config_source.api_type() == envoy::api::v2::core::ApiConfigSource::REST_LEGACY); - // TODO(htuch): Add support for multiple clusters, #1170. - ASSERT(api_config_source.cluster_names().size() == 1); - ASSERT(api_config_source.has_refresh_delay()); - Envoy::Config::Utility::checkClusterAndLocalInfo("rds", api_config_source.cluster_names()[0], cm, - local_info); + Envoy::Config::Utility::checkClusterAndLocalInfo( + "rds", rds.config_source().api_config_source().cluster_names()[0], cm, local_info); } void RdsSubscription::createRequest(Http::Message& request) { diff --git a/source/common/upstream/cds_subscription.cc b/source/common/upstream/cds_subscription.cc index c571b855d313c..7e975b833ecd7 100644 --- a/source/common/upstream/cds_subscription.cc +++ b/source/common/upstream/cds_subscription.cc @@ -17,18 +17,9 @@ CdsSubscription::CdsSubscription( const absl::optional& eds_config, ClusterManager& cm, Event::Dispatcher& dispatcher, Runtime::RandomGenerator& random, const LocalInfo::LocalInfo& local_info, const Stats::StatsOptions& stats_options) - : RestApiFetcher(cm, cds_config.api_config_source().cluster_names()[0], dispatcher, random, - Config::Utility::apiConfigSourceRefreshDelay(cds_config.api_config_source())), + : RestApiFetcher(cm, cds_config.api_config_source(), dispatcher, random), local_info_(local_info), stats_(stats), eds_config_(eds_config), - stats_options_(stats_options) { - const auto& api_config_source = cds_config.api_config_source(); - UNREFERENCED_PARAMETER(api_config_source); - // If we are building an CdsSubscription, the ConfigSource should be REST_LEGACY. - ASSERT(api_config_source.api_type() == envoy::api::v2::core::ApiConfigSource::REST_LEGACY); - // TODO(htuch): Add support for multiple clusters, #1170. - ASSERT(api_config_source.cluster_names().size() == 1); - ASSERT(api_config_source.has_refresh_delay()); -} + stats_options_(stats_options) {} void CdsSubscription::createRequest(Http::Message& request) { ENVOY_LOG(debug, "cds: starting request"); diff --git a/source/common/upstream/sds_subscription.cc b/source/common/upstream/sds_subscription.cc index c9d4fef525385..788c431441465 100644 --- a/source/common/upstream/sds_subscription.cc +++ b/source/common/upstream/sds_subscription.cc @@ -22,17 +22,7 @@ SdsSubscription::SdsSubscription(ClusterStats& stats, const envoy::api::v2::core::ConfigSource& eds_config, ClusterManager& cm, Event::Dispatcher& dispatcher, Runtime::RandomGenerator& random) - : RestApiFetcher(cm, eds_config.api_config_source().cluster_names()[0], dispatcher, random, - Config::Utility::apiConfigSourceRefreshDelay(eds_config.api_config_source())), - stats_(stats) { - const auto& api_config_source = eds_config.api_config_source(); - UNREFERENCED_PARAMETER(api_config_source); - // If we are building an SdsSubscription, the ConfigSource should be REST_LEGACY. - ASSERT(api_config_source.api_type() == envoy::api::v2::core::ApiConfigSource::REST_LEGACY); - // TODO(htuch): Add support for multiple clusters, #1170. - ASSERT(api_config_source.cluster_names().size() == 1); - ASSERT(api_config_source.has_refresh_delay()); -} + : RestApiFetcher(cm, eds_config.api_config_source(), dispatcher, random), stats_(stats) {} void SdsSubscription::parseResponse(const Http::Message& response) { const std::string response_body = response.bodyAsString(); diff --git a/source/extensions/filters/network/client_ssl_auth/client_ssl_auth.cc b/source/extensions/filters/network/client_ssl_auth/client_ssl_auth.cc index 0615bc3f80eba..2aef3909f7411 100644 --- a/source/extensions/filters/network/client_ssl_auth/client_ssl_auth.cc +++ b/source/extensions/filters/network/client_ssl_auth/client_ssl_auth.cc @@ -25,7 +25,8 @@ ClientSslAuthConfig::ClientSslAuthConfig( Stats::Scope& scope, Runtime::RandomGenerator& random) : RestApiFetcher( cm, config.auth_api_cluster(), dispatcher, random, - std::chrono::milliseconds(PROTOBUF_GET_MS_OR_DEFAULT(config, refresh_delay, 60000))), + std::chrono::milliseconds(PROTOBUF_GET_MS_OR_DEFAULT(config, refresh_delay, 60000)), + std::chrono::milliseconds(1000)), tls_(tls.allocateSlot()), ip_white_list_(config.ip_white_list()), stats_(generateStats(scope, config.stat_prefix())) { diff --git a/source/server/lds_subscription.cc b/source/server/lds_subscription.cc index 757d4c8e43f6c..713e6b751a789 100644 --- a/source/server/lds_subscription.cc +++ b/source/server/lds_subscription.cc @@ -18,18 +18,10 @@ LdsSubscription::LdsSubscription(Config::SubscriptionStats stats, Runtime::RandomGenerator& random, const LocalInfo::LocalInfo& local_info, const Stats::StatsOptions& stats_options) - : RestApiFetcher(cm, lds_config.api_config_source().cluster_names()[0], dispatcher, random, - Config::Utility::apiConfigSourceRefreshDelay(lds_config.api_config_source())), + : RestApiFetcher(cm, lds_config.api_config_source(), dispatcher, random), local_info_(local_info), stats_(stats), stats_options_(stats_options) { - const auto& api_config_source = lds_config.api_config_source(); - UNREFERENCED_PARAMETER(lds_config); - // If we are building an LdsSubscription, the ConfigSource should be REST_LEGACY. - ASSERT(api_config_source.api_type() == envoy::api::v2::core::ApiConfigSource::REST_LEGACY); - // TODO(htuch): Add support for multiple clusters, #1170. - ASSERT(api_config_source.cluster_names().size() == 1); - ASSERT(api_config_source.has_refresh_delay()); - Envoy::Config::Utility::checkClusterAndLocalInfo("lds", api_config_source.cluster_names()[0], cm, - local_info); + Envoy::Config::Utility::checkClusterAndLocalInfo( + "lds", lds_config.api_config_source().cluster_names()[0], cm, local_info); } void LdsSubscription::createRequest(Http::Message& request) { diff --git a/test/common/config/http_subscription_test_harness.h b/test/common/config/http_subscription_test_harness.h index 65b3d0aaa6b81..0853b50c8c7db 100644 --- a/test/common/config/http_subscription_test_harness.h +++ b/test/common/config/http_subscription_test_harness.h @@ -37,9 +37,9 @@ class HttpSubscriptionTestHarness : public SubscriptionTestHarness { timer_cb_ = timer_cb; return timer_; })); - subscription_.reset(new HttpEdsSubscriptionImpl(node_, cm_, "eds_cluster", dispatcher_, - random_gen_, std::chrono::milliseconds(1), - *method_descriptor_, stats_)); + subscription_.reset(new HttpEdsSubscriptionImpl( + node_, cm_, "eds_cluster", dispatcher_, random_gen_, std::chrono::milliseconds(1), + std::chrono::milliseconds(1000), *method_descriptor_, stats_)); } ~HttpSubscriptionTestHarness() { diff --git a/test/common/config/subscription_factory_test.cc b/test/common/config/subscription_factory_test.cc index 0dc79fc2b32e7..ce85042ffd9d9 100644 --- a/test/common/config/subscription_factory_test.cc +++ b/test/common/config/subscription_factory_test.cc @@ -213,6 +213,27 @@ TEST_F(SubscriptionFactoryTest, LegacySubscription) { subscriptionFromConfigSource(config)->start({"static_cluster"}, callbacks_); } +TEST_F(SubscriptionFactoryTest, HttpSubscriptionCustomRequestTimeout) { + envoy::api::v2::core::ConfigSource config; + auto* api_config_source = config.mutable_api_config_source(); + api_config_source->set_api_type(envoy::api::v2::core::ApiConfigSource::REST); + api_config_source->add_cluster_names("static_cluster"); + api_config_source->mutable_refresh_delay()->set_seconds(1); + api_config_source->mutable_request_timeout()->set_seconds(5); + Upstream::ClusterManager::ClusterInfoMap cluster_map; + Upstream::MockCluster cluster; + cluster_map.emplace("static_cluster", cluster); + EXPECT_CALL(cm_, clusters()).WillOnce(Return(cluster_map)); + EXPECT_CALL(cluster, info()).Times(2); + EXPECT_CALL(*cluster.info_, addedViaApi()); + EXPECT_CALL(dispatcher_, createTimer_(_)); + EXPECT_CALL(cm_, httpAsyncClientForCluster("static_cluster")); + EXPECT_CALL( + cm_.async_client_, + send_(_, _, absl::optional(std::chrono::milliseconds(5000)))); + subscriptionFromConfigSource(config)->start({"static_cluster"}, callbacks_); +} + TEST_F(SubscriptionFactoryTest, HttpSubscription) { envoy::api::v2::core::ConfigSource config; auto* api_config_source = config.mutable_api_config_source(); diff --git a/test/common/config/utility_test.cc b/test/common/config/utility_test.cc index 417c793d6dd94..7a192cd96049f 100644 --- a/test/common/config/utility_test.cc +++ b/test/common/config/utility_test.cc @@ -59,6 +59,18 @@ TEST(UtilityTest, ApiConfigSourceRefreshDelay) { EXPECT_EQ(1234, Utility::apiConfigSourceRefreshDelay(api_config_source).count()); } +TEST(UtilityTest, ApiConfigSourceDefaultRequestTimeout) { + envoy::api::v2::core::ApiConfigSource api_config_source; + EXPECT_EQ(1000, Utility::apiConfigSourceRequestTimeout(api_config_source).count()); +} + +TEST(UtilityTest, ApiConfigSourceRequestTimeout) { + envoy::api::v2::core::ApiConfigSource api_config_source; + api_config_source.mutable_request_timeout()->CopyFrom( + Protobuf::util::TimeUtil::MillisecondsToDuration(1234)); + EXPECT_EQ(1234, Utility::apiConfigSourceRequestTimeout(api_config_source).count()); +} + TEST(UtilityTest, TranslateApiConfigSource) { envoy::api::v2::core::ApiConfigSource api_config_source_rest_legacy; Utility::translateApiConfigSource("test_rest_legacy_cluster", 10000, ApiType::get().RestLegacy, diff --git a/test/common/upstream/sds_test.cc b/test/common/upstream/sds_test.cc index 85a7dfb180b54..ff888750c67ab 100644 --- a/test/common/upstream/sds_test.cc +++ b/test/common/upstream/sds_test.cc @@ -42,7 +42,9 @@ namespace Upstream { class SdsTest : public testing::Test { protected: - SdsTest() : request_(&cm_.async_client_) { + SdsTest() : request_(&cm_.async_client_) { resetCluster(false); } + + void resetCluster(bool set_request_timeout) { std::string raw_config = R"EOF( { "name": "name", @@ -58,6 +60,10 @@ class SdsTest : public testing::Test { envoy::api::v2::core::ConfigSource eds_config; eds_config.mutable_api_config_source()->add_cluster_names("sds"); eds_config.mutable_api_config_source()->mutable_refresh_delay()->set_seconds(1); + if (set_request_timeout) { + eds_config.mutable_api_config_source()->mutable_request_timeout()->CopyFrom( + Protobuf::util::TimeUtil::MillisecondsToDuration(5000)); + } sds_cluster_ = parseSdsClusterFromJson(raw_config, eds_config); Upstream::ClusterManager::ClusterInfoMap cluster_map; Upstream::MockCluster cluster; @@ -147,6 +153,20 @@ TEST_F(SdsTest, PoolFailure) { cluster_->initialize([] {}); } +TEST_F(SdsTest, RequestTimeout) { + resetCluster(true); + + EXPECT_CALL(cm_, httpAsyncClientForCluster("sds")).WillOnce(ReturnRef(cm_.async_client_)); + EXPECT_CALL( + cm_.async_client_, + send_(_, _, absl::optional(std::chrono::milliseconds(5000)))) + .WillOnce(DoAll(WithArg<1>(SaveArgAddress(&callbacks_)), Return(&request_))); + + cluster_->initialize([] {}); + EXPECT_CALL(request_, cancel()); + cluster_.reset(); +} + TEST_F(SdsTest, NoHealthChecker) { InSequence s; setupRequest(); From cc3657797135eccfe985a7d7c1ddecce388898ed Mon Sep 17 00:00:00 2001 From: Venil Noronha Date: Fri, 3 Aug 2018 09:49:14 -0700 Subject: [PATCH 09/11] docs: document request_timeout in version_history (#4041) This commit documents the proto change in #4006. Signed-off-by: Venil Noronha --- docs/root/intro/version_history.rst | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index 62f92f2070040..bcc83e03a0f65 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -48,6 +48,7 @@ Version history :ref:`use_data_plane_proto` boolean flag in the ratelimit configuration. Support for the legacy proto :repo:`source/common/ratelimit/ratelimit.proto` is deprecated and will be removed at the start of the 1.9.0 release cycle. +* rest-api: added ability to set the :ref:`request timeout ` for REST API requests. * router: added ability to set request/response headers at the :ref:`envoy_api_msg_route.Route` level. * tracing: added support for configuration of :ref:`tracing sampling `. From 6a1868dffebfa7ebf9e565ad4db827f0b07f0798 Mon Sep 17 00:00:00 2001 From: Daniel Hochman Date: Fri, 3 Aug 2018 12:00:36 -0700 Subject: [PATCH 10/11] Revert "tcp_proxy: convert TCP proxy to use TCP connection pool (#3938)" (#4043) This reverts commit 028387a3b0746deaf011ea50104692dfbb8b8d2f. Signed-off-by: Daniel Hochman --- include/envoy/tcp/conn_pool.h | 22 +- .../common/http/websocket/ws_handler_impl.cc | 3 +- source/common/tcp/conn_pool.cc | 34 +-- source/common/tcp/conn_pool.h | 27 +- source/common/tcp_proxy/BUILD | 1 - source/common/tcp_proxy/tcp_proxy.cc | 214 ++++++++-------- source/common/tcp_proxy/tcp_proxy.h | 45 ++-- .../thrift_proxy/router/router_impl.cc | 11 +- .../network/thrift_proxy/router/router_impl.h | 4 +- test/common/http/conn_manager_impl_test.cc | 176 ++++++++----- .../network/filter_manager_impl_test.cc | 16 +- test/common/tcp/conn_pool_test.cc | 18 +- test/common/tcp_proxy/tcp_proxy_test.cc | 237 ++++++++++-------- .../network/thrift_proxy/router_test.cc | 48 ++-- .../tcp_conn_pool_integration_test.cc | 8 +- test/mocks/tcp/mocks.cc | 16 +- test/mocks/tcp/mocks.h | 13 +- 17 files changed, 475 insertions(+), 418 deletions(-) diff --git a/include/envoy/tcp/conn_pool.h b/include/envoy/tcp/conn_pool.h index 8237af37fea31..c89b25eb91432 100644 --- a/include/envoy/tcp/conn_pool.h +++ b/include/envoy/tcp/conn_pool.h @@ -58,8 +58,7 @@ class UpstreamCallbacks : public Network::ConnectionCallbacks { }; /* - * ConnectionData wraps a ClientConnection allocated to a caller. Open ClientConnections are - * released back to the pool for re-use when their containing ConnectionData is destroyed. + * ConnectionData wraps a ClientConnection allocated to a caller. */ class ConnectionData { public: @@ -77,9 +76,13 @@ class ConnectionData { * @param callback the UpstreamCallbacks to invoke for upstream data */ virtual void addUpstreamCallbacks(ConnectionPool::UpstreamCallbacks& callback) PURE; -}; -typedef std::unique_ptr ConnectionDataPtr; + /** + * Release the connection after use. The connection should be closed first only if it is + * not viable for future use. + */ + virtual void release() PURE; +}; /** * Pool callbacks invoked in the context of a newConnection() call, either synchronously or @@ -99,17 +102,14 @@ class Callbacks { Upstream::HostDescriptionConstSharedPtr host) PURE; /** - * Called when a connection is available to process a request/response. Connections may be - * released back to the pool for re-use by resetting the ConnectionDataPtr. If the connection is - * no longer viable for reuse (e.g. due to some kind of protocol error), the underlying - * ClientConnection should be closed to prevent its reuse. - * + * Called when a connection is available to process a request/response. Recipients of connections + * must release the connection after use. They should only close the underlying ClientConnection + * if it is no longer viable for future requests. * @param conn supplies the connection data to use. * @param host supplies the description of the host that will carry the request. For logical * connection pools the description may be different each time this is called. */ - virtual void onPoolReady(ConnectionDataPtr&& conn, - Upstream::HostDescriptionConstSharedPtr host) PURE; + virtual void onPoolReady(ConnectionData& conn, Upstream::HostDescriptionConstSharedPtr host) PURE; }; /** diff --git a/source/common/http/websocket/ws_handler_impl.cc b/source/common/http/websocket/ws_handler_impl.cc index 5906c54b5d92c..541f085155dfe 100644 --- a/source/common/http/websocket/ws_handler_impl.cc +++ b/source/common/http/websocket/ws_handler_impl.cc @@ -131,8 +131,7 @@ void WsHandlerImpl::onConnectionSuccess() { // the connection pool. The current approach is a stop gap solution, where // we put the onus on the user to tell us if a route (and corresponding upstream) // is supposed to allow websocket upgrades or not. - Http1::ClientConnectionImpl upstream_http(upstream_conn_data_->connection(), - http_conn_callbacks_); + Http1::ClientConnectionImpl upstream_http(*upstream_connection_, http_conn_callbacks_); Http1::RequestStreamEncoderImpl upstream_request = Http1::RequestStreamEncoderImpl(upstream_http); upstream_request.encodeHeaders(request_headers_, false); ASSERT(state_ == ConnectState::PreConnect); diff --git a/source/common/tcp/conn_pool.cc b/source/common/tcp/conn_pool.cc index 2ee50a6fcdf9f..d6c60c174c465 100644 --- a/source/common/tcp/conn_pool.cc +++ b/source/common/tcp/conn_pool.cc @@ -46,10 +46,8 @@ void ConnPoolImpl::addDrainedCallback(DrainedCb cb) { void ConnPoolImpl::assignConnection(ActiveConn& conn, ConnectionPool::Callbacks& callbacks) { ASSERT(conn.wrapper_ == nullptr); - conn.wrapper_ = std::make_shared(conn); - - callbacks.onPoolReady(std::make_unique(conn.wrapper_), - conn.real_host_description_); + conn.wrapper_ = std::make_unique(conn); + callbacks.onPoolReady(*conn.wrapper_, conn.real_host_description_); } void ConnPoolImpl::checkForDrained() { @@ -126,8 +124,6 @@ void ConnPoolImpl::onConnectionEvent(ActiveConn& conn, Network::ConnectionEvent host_->cluster().stats().upstream_cx_destroy_remote_with_active_rq_.inc(); } host_->cluster().stats().upstream_cx_destroy_with_active_rq_.inc(); - - conn.wrapper_->release(true); } removed = conn.removeFromList(busy_conns_); @@ -263,29 +259,23 @@ ConnPoolImpl::ConnectionWrapper::ConnectionWrapper(ActiveConn& parent) : parent_ parent_.parent_.host_->stats().rq_active_.inc(); } -Network::ClientConnection& ConnPoolImpl::ConnectionWrapper::connection() { - ASSERT(!released_); - return *parent_.conn_; +ConnPoolImpl::ConnectionWrapper::~ConnectionWrapper() { + parent_.parent_.host_->cluster().stats().upstream_rq_active_.dec(); + parent_.parent_.host_->stats().rq_active_.dec(); } +Network::ClientConnection& ConnPoolImpl::ConnectionWrapper::connection() { return *parent_.conn_; } + void ConnPoolImpl::ConnectionWrapper::addUpstreamCallbacks(ConnectionPool::UpstreamCallbacks& cb) { ASSERT(!released_); callbacks_ = &cb; } -void ConnPoolImpl::ConnectionWrapper::release(bool closed) { - // Allow multiple calls: connection close and destruction of ConnectionDataImplPtr will both - // result in this call. - if (!released_) { - released_ = true; - callbacks_ = nullptr; - if (!closed) { - parent_.parent_.onConnReleased(parent_); - } - - parent_.parent_.host_->cluster().stats().upstream_rq_active_.dec(); - parent_.parent_.host_->stats().rq_active_.dec(); - } +void ConnPoolImpl::ConnectionWrapper::release() { + ASSERT(!released_); + released_ = true; + callbacks_ = nullptr; + parent_.parent_.onConnReleased(parent_); } ConnPoolImpl::PendingRequest::PendingRequest(ConnPoolImpl& parent, diff --git a/source/common/tcp/conn_pool.h b/source/common/tcp/conn_pool.h index 6e1846fe43c31..aa06d8b2d8b4d 100644 --- a/source/common/tcp/conn_pool.h +++ b/source/common/tcp/conn_pool.h @@ -34,32 +34,21 @@ class ConnPoolImpl : Logger::Loggable, public ConnectionPool:: protected: struct ActiveConn; - struct ConnectionWrapper { + struct ConnectionWrapper : public ConnectionPool::ConnectionData { ConnectionWrapper(ActiveConn& parent); + ~ConnectionWrapper(); - Network::ClientConnection& connection(); - void addUpstreamCallbacks(ConnectionPool::UpstreamCallbacks& callbacks); - void release(bool closed); + // ConnectionPool::ConnectionData + Network::ClientConnection& connection() override; + void addUpstreamCallbacks(ConnectionPool::UpstreamCallbacks& callbacks) override; + void release() override; ActiveConn& parent_; ConnectionPool::UpstreamCallbacks* callbacks_{}; bool released_{false}; }; - typedef std::shared_ptr ConnectionWrapperSharedPtr; - - struct ConnectionDataImpl : public ConnectionPool::ConnectionData { - ConnectionDataImpl(ConnectionWrapperSharedPtr wrapper) : wrapper_(wrapper) {} - ~ConnectionDataImpl() { wrapper_->release(false); } - - // ConnectionPool::ConnectionData - Network::ClientConnection& connection() override { return wrapper_->connection(); } - void addUpstreamCallbacks(ConnectionPool::UpstreamCallbacks& callbacks) override { - wrapper_->addUpstreamCallbacks(callbacks); - }; - - ConnectionWrapperSharedPtr wrapper_; - }; + typedef std::unique_ptr ConnectionWrapperPtr; struct ConnReadFilter : public Network::ReadFilterBaseImpl { ConnReadFilter(ActiveConn& parent) : parent_(parent) {} @@ -89,7 +78,7 @@ class ConnPoolImpl : Logger::Loggable, public ConnectionPool:: ConnPoolImpl& parent_; Upstream::HostDescriptionConstSharedPtr real_host_description_; - ConnectionWrapperSharedPtr wrapper_; + ConnectionWrapperPtr wrapper_; Network::ClientConnectionPtr conn_; Event::TimerPtr connect_timer_; Stats::TimespanPtr conn_length_; diff --git a/source/common/tcp_proxy/BUILD b/source/common/tcp_proxy/BUILD index 61de5015db426..fcd3000620fb8 100644 --- a/source/common/tcp_proxy/BUILD +++ b/source/common/tcp_proxy/BUILD @@ -24,7 +24,6 @@ envoy_cc_library( "//include/envoy/stats:stats_interface", "//include/envoy/stats:stats_macros", "//include/envoy/stats:timespan", - "//include/envoy/tcp:conn_pool_interface", "//include/envoy/upstream:cluster_manager_interface", "//include/envoy/upstream:upstream_interface", "//source/common/access_log:access_log_lib", diff --git a/source/common/tcp_proxy/tcp_proxy.cc b/source/common/tcp_proxy/tcp_proxy.cc index 906bdeeb696ea..68d4880200be7 100644 --- a/source/common/tcp_proxy/tcp_proxy.cc +++ b/source/common/tcp_proxy/tcp_proxy.cc @@ -135,12 +135,8 @@ Filter::~Filter() { access_log->log(nullptr, nullptr, nullptr, getRequestInfo()); } - if (upstream_handle_) { - upstream_handle_->cancel(); - } - - if (upstream_conn_data_) { - upstream_conn_data_->connection().close(Network::ConnectionCloseType::NoFlush); + if (upstream_connection_) { + finalizeUpstreamConnectionStats(); } } @@ -148,6 +144,21 @@ TcpProxyStats Config::SharedConfig::generateStats(Stats::Scope& scope) { return {ALL_TCP_PROXY_STATS(POOL_COUNTER(scope), POOL_GAUGE(scope))}; } +namespace { +void finalizeConnectionStats(const Upstream::HostDescription& host, + Stats::Timespan connected_timespan) { + host.cluster().stats().upstream_cx_destroy_.inc(); + host.cluster().stats().upstream_cx_active_.dec(); + host.stats().cx_active_.dec(); + host.cluster().resourceManager(Upstream::ResourcePriority::Default).connections().dec(); + connected_timespan.complete(); +} +} // namespace + +void Filter::finalizeUpstreamConnectionStats() { + finalizeConnectionStats(*read_callbacks_->upstreamHost(), *connected_timespan_); +} + void Filter::initializeReadFilterCallbacks(Network::ReadFilterCallbacks& callbacks) { initialize(callbacks, true); } @@ -177,15 +188,15 @@ void Filter::initialize(Network::ReadFilterCallbacks& callbacks, bool set_connec } void Filter::readDisableUpstream(bool disable) { - if (upstream_conn_data_ == nullptr || - upstream_conn_data_->connection().state() != Network::Connection::State::Open) { + if (upstream_connection_ == nullptr || + upstream_connection_->state() != Network::Connection::State::Open) { // Because we flush write downstream, we can have a case where upstream has already disconnected // and we are waiting to flush. If we had a watermark event during this time we should no // longer touch the upstream connection. return; } - upstream_conn_data_->connection().readDisable(disable); + upstream_connection_->readDisable(disable); if (disable) { read_callbacks_->upstreamHost() ->cluster() @@ -251,12 +262,13 @@ void Filter::UpstreamCallbacks::onBelowWriteBufferLowWatermark() { } } -void Filter::UpstreamCallbacks::onUpstreamData(Buffer::Instance& data, bool end_stream) { +Network::FilterStatus Filter::UpstreamCallbacks::onData(Buffer::Instance& data, bool end_stream) { if (parent_) { parent_->onUpstreamData(data, end_stream); } else { drainer_->onData(data, end_stream); } + return Network::FilterStatus::StopIteration; } void Filter::UpstreamCallbacks::onBytesSent() { @@ -282,7 +294,7 @@ void Filter::UpstreamCallbacks::drain(Drainer& drainer) { } Network::FilterStatus Filter::initializeUpstreamConnection() { - ASSERT(upstream_conn_data_ == nullptr); + ASSERT(upstream_connection_ == nullptr); const std::string& cluster_name = getUpstreamCluster(); @@ -299,9 +311,6 @@ Network::FilterStatus Filter::initializeUpstreamConnection() { } Upstream::ClusterInfoConstSharedPtr cluster = thread_local_cluster->info(); - - // Check this here because the TCP conn pool will queue our request waiting for a connection that - // will never be released. if (!cluster->resourceManager(Upstream::ResourcePriority::Default).connections().canCreate()) { getRequestInfo().setResponseFlag(RequestInfo::ResponseFlag::UpstreamOverflow); cluster->stats().upstream_cx_overflow_.inc(); @@ -316,105 +325,86 @@ Network::FilterStatus Filter::initializeUpstreamConnection() { return Network::FilterStatus::StopIteration; } - Tcp::ConnectionPool::Instance* conn_pool = cluster_manager_.tcpConnPoolForCluster( - cluster_name, Upstream::ResourcePriority::Default, this); - if (!conn_pool) { - // Either cluster is unknown or there are no healthy hosts. tcpConnPoolForCluster() increments - // cluster->stats().upstream_cx_none_healthy in the latter case. + Upstream::Host::CreateConnectionData conn_info = + cluster_manager_.tcpConnForCluster(cluster_name, this); + + upstream_connection_ = std::move(conn_info.connection_); + read_callbacks_->upstreamHost(conn_info.host_description_); + if (!upstream_connection_) { + // tcpConnForCluster() increments cluster->stats().upstream_cx_none_healthy. getRequestInfo().setResponseFlag(RequestInfo::ResponseFlag::NoHealthyUpstream); onInitFailure(UpstreamFailureReason::NO_HEALTHY_UPSTREAM); return Network::FilterStatus::StopIteration; } - connecting_ = true; connect_attempts_++; - - // Because we never return open connections to the pool, this should either return a handle while - // a connection completes or it invokes onPoolFailure inline. Either way, stop iteration. - upstream_handle_ = conn_pool->newConnection(*this); - return Network::FilterStatus::StopIteration; -} - -void Filter::onPoolFailure(Tcp::ConnectionPool::PoolFailureReason reason, - Upstream::HostDescriptionConstSharedPtr host) { - upstream_handle_ = nullptr; - - read_callbacks_->upstreamHost(host); - getRequestInfo().onUpstreamHostSelected(host); - - switch (reason) { - case Tcp::ConnectionPool::PoolFailureReason::Overflow: - case Tcp::ConnectionPool::PoolFailureReason::LocalConnectionFailure: - upstream_callbacks_->onEvent(Network::ConnectionEvent::LocalClose); - break; - - case Tcp::ConnectionPool::PoolFailureReason::RemoteConnectionFailure: - upstream_callbacks_->onEvent(Network::ConnectionEvent::RemoteClose); - break; - - case Tcp::ConnectionPool::PoolFailureReason::Timeout: - onConnectTimeout(); - break; - - default: - NOT_REACHED_GCOVR_EXCL_LINE; - } -} - -void Filter::onPoolReady(Tcp::ConnectionPool::ConnectionDataPtr&& conn_data, - Upstream::HostDescriptionConstSharedPtr host) { - upstream_handle_ = nullptr; - upstream_conn_data_ = std::move(conn_data); - read_callbacks_->upstreamHost(host); - - upstream_conn_data_->addUpstreamCallbacks(*upstream_callbacks_); - - Network::ClientConnection& connection = upstream_conn_data_->connection(); - - connection.enableHalfClose(true); - - getRequestInfo().onUpstreamHostSelected(host); - getRequestInfo().setUpstreamLocalAddress(connection.localAddress()); - - // Simulate the event that onPoolReady represents. - upstream_callbacks_->onEvent(Network::ConnectionEvent::Connected); - - read_callbacks_->continueReading(); + cluster->resourceManager(Upstream::ResourcePriority::Default).connections().inc(); + upstream_connection_->addReadFilter(upstream_callbacks_); + upstream_connection_->addConnectionCallbacks(*upstream_callbacks_); + upstream_connection_->enableHalfClose(true); + upstream_connection_->setConnectionStats( + {read_callbacks_->upstreamHost()->cluster().stats().upstream_cx_rx_bytes_total_, + read_callbacks_->upstreamHost()->cluster().stats().upstream_cx_rx_bytes_buffered_, + read_callbacks_->upstreamHost()->cluster().stats().upstream_cx_tx_bytes_total_, + read_callbacks_->upstreamHost()->cluster().stats().upstream_cx_tx_bytes_buffered_, + &read_callbacks_->upstreamHost()->cluster().stats().bind_errors_}); + upstream_connection_->connect(); + upstream_connection_->noDelay(true); + getRequestInfo().onUpstreamHostSelected(conn_info.host_description_); + getRequestInfo().setUpstreamLocalAddress(upstream_connection_->localAddress()); + + ASSERT(connect_timeout_timer_ == nullptr); + connect_timeout_timer_ = read_callbacks_->connection().dispatcher().createTimer( + [this]() -> void { onConnectTimeout(); }); + connect_timeout_timer_->enableTimer(cluster->connectTimeout()); + + read_callbacks_->upstreamHost()->cluster().stats().upstream_cx_total_.inc(); + read_callbacks_->upstreamHost()->cluster().stats().upstream_cx_active_.inc(); + read_callbacks_->upstreamHost()->stats().cx_total_.inc(); + read_callbacks_->upstreamHost()->stats().cx_active_.inc(); + connect_timespan_.reset(new Stats::Timespan( + read_callbacks_->upstreamHost()->cluster().stats().upstream_cx_connect_ms_)); + connected_timespan_.reset(new Stats::Timespan( + read_callbacks_->upstreamHost()->cluster().stats().upstream_cx_length_ms_)); + + return Network::FilterStatus::Continue; } void Filter::onConnectTimeout() { ENVOY_CONN_LOG(debug, "connect timeout", read_callbacks_->connection()); read_callbacks_->upstreamHost()->outlierDetector().putResult(Upstream::Outlier::Result::TIMEOUT); + read_callbacks_->upstreamHost()->cluster().stats().upstream_cx_connect_timeout_.inc(); getRequestInfo().setResponseFlag(RequestInfo::ResponseFlag::UpstreamConnectionFailure); - // Raise LocalClose, which will trigger a reconnect if needed/configured. - upstream_callbacks_->onEvent(Network::ConnectionEvent::LocalClose); + // This will cause a LocalClose event to be raised, which will trigger a reconnect if + // needed/configured. + upstream_connection_->close(Network::ConnectionCloseType::NoFlush); } Network::FilterStatus Filter::onData(Buffer::Instance& data, bool end_stream) { ENVOY_CONN_LOG(trace, "downstream connection received {} bytes, end_stream={}", read_callbacks_->connection(), data.length(), end_stream); getRequestInfo().addBytesReceived(data.length()); - upstream_conn_data_->connection().write(data, end_stream); + upstream_connection_->write(data, end_stream); ASSERT(0 == data.length()); resetIdleTimer(); // TODO(ggreenway) PERF: do we need to reset timer on both send and receive? return Network::FilterStatus::StopIteration; } void Filter::onDownstreamEvent(Network::ConnectionEvent event) { - if (upstream_conn_data_) { + if (upstream_connection_) { if (event == Network::ConnectionEvent::RemoteClose) { - upstream_conn_data_->connection().close(Network::ConnectionCloseType::FlushWrite); + upstream_connection_->close(Network::ConnectionCloseType::FlushWrite); - if (upstream_conn_data_ != nullptr && - upstream_conn_data_->connection().state() != Network::Connection::State::Closed) { - config_->drainManager().add(config_->sharedConfig(), std::move(upstream_conn_data_), + if (upstream_connection_ != nullptr && + upstream_connection_->state() != Network::Connection::State::Closed) { + config_->drainManager().add(config_->sharedConfig(), std::move(upstream_connection_), std::move(upstream_callbacks_), std::move(idle_timer_), - read_callbacks_->upstreamHost()); + read_callbacks_->upstreamHost(), + std::move(connected_timespan_)); } } else if (event == Network::ConnectionEvent::LocalClose) { - upstream_conn_data_->connection().close(Network::ConnectionCloseType::NoFlush); - upstream_conn_data_.reset(); + upstream_connection_->close(Network::ConnectionCloseType::NoFlush); disableIdleTimer(); } } @@ -430,21 +420,36 @@ void Filter::onUpstreamData(Buffer::Instance& data, bool end_stream) { } void Filter::onUpstreamEvent(Network::ConnectionEvent event) { - // Update the connecting flag before processing the event because we may start a new connection - // attempt in initializeUpstreamConnection. - bool connecting = connecting_; - connecting_ = false; + bool connecting = false; + + // The timer must be cleared before, not after, processing the event because + // if initializeUpstreamConnection() is called it will reset the timer, so + // clearing after that call will leave the timer unset. + if (connect_timeout_timer_) { + connecting = true; + connect_timeout_timer_->disableTimer(); + connect_timeout_timer_.reset(); + } if (event == Network::ConnectionEvent::RemoteClose || event == Network::ConnectionEvent::LocalClose) { - upstream_conn_data_.reset(); + finalizeUpstreamConnectionStats(); + read_callbacks_->connection().dispatcher().deferredDelete(std::move(upstream_connection_)); disableIdleTimer(); + auto& destroy_ctx_stat = + (event == Network::ConnectionEvent::RemoteClose) + ? read_callbacks_->upstreamHost()->cluster().stats().upstream_cx_destroy_remote_ + : read_callbacks_->upstreamHost()->cluster().stats().upstream_cx_destroy_local_; + destroy_ctx_stat.inc(); + if (connecting) { if (event == Network::ConnectionEvent::RemoteClose) { getRequestInfo().setResponseFlag(RequestInfo::ResponseFlag::UpstreamConnectionFailure); read_callbacks_->upstreamHost()->outlierDetector().putResult( Upstream::Outlier::Result::CONNECT_FAILED); + read_callbacks_->upstreamHost()->cluster().stats().upstream_cx_connect_fail_.inc(); + read_callbacks_->upstreamHost()->stats().cx_connect_fail_.inc(); } initializeUpstreamConnection(); @@ -454,6 +459,8 @@ void Filter::onUpstreamEvent(Network::ConnectionEvent event) { } } } else if (event == Network::ConnectionEvent::Connected) { + connect_timespan_->complete(); + // Re-enable downstream reads now that the upstream connection is established // so we have a place to send downstream data to. read_callbacks_->connection().readDisable(false); @@ -473,10 +480,8 @@ void Filter::onUpstreamEvent(Network::ConnectionEvent event) { }); resetIdleTimer(); read_callbacks_->connection().addBytesSentCallback([this](uint64_t) { resetIdleTimer(); }); - upstream_conn_data_->connection().addBytesSentCallback([upstream_callbacks = - upstream_callbacks_](uint64_t) { - upstream_callbacks->onBytesSent(); - }); + upstream_connection_->addBytesSentCallback([upstream_callbacks = upstream_callbacks_]( + uint64_t) { upstream_callbacks->onBytesSent(); }); } } } @@ -518,12 +523,14 @@ UpstreamDrainManager::~UpstreamDrainManager() { } void UpstreamDrainManager::add(const Config::SharedConfigSharedPtr& config, - Tcp::ConnectionPool::ConnectionDataPtr&& upstream_conn_data, + Network::ClientConnectionPtr&& upstream_connection, const std::shared_ptr& callbacks, Event::TimerPtr&& idle_timer, - const Upstream::HostDescriptionConstSharedPtr& upstream_host) { - DrainerPtr drainer(new Drainer(*this, config, callbacks, std::move(upstream_conn_data), - std::move(idle_timer), upstream_host)); + const Upstream::HostDescriptionConstSharedPtr& upstream_host, + Stats::TimespanPtr&& connected_timespan) { + DrainerPtr drainer(new Drainer(*this, config, callbacks, std::move(upstream_connection), + std::move(idle_timer), upstream_host, + std::move(connected_timespan))); callbacks->drain(*drainer); // Use temporary to ensure we get the pointer before we move it out of drainer @@ -540,10 +547,12 @@ void UpstreamDrainManager::remove(Drainer& drainer, Event::Dispatcher& dispatche Drainer::Drainer(UpstreamDrainManager& parent, const Config::SharedConfigSharedPtr& config, const std::shared_ptr& callbacks, - Tcp::ConnectionPool::ConnectionDataPtr&& conn_data, Event::TimerPtr&& idle_timer, - const Upstream::HostDescriptionConstSharedPtr& upstream_host) - : parent_(parent), callbacks_(callbacks), upstream_conn_data_(std::move(conn_data)), - timer_(std::move(idle_timer)), upstream_host_(upstream_host), config_(config) { + Network::ClientConnectionPtr&& connection, Event::TimerPtr&& idle_timer, + const Upstream::HostDescriptionConstSharedPtr& upstream_host, + Stats::TimespanPtr&& connected_timespan) + : parent_(parent), callbacks_(callbacks), upstream_connection_(std::move(connection)), + timer_(std::move(idle_timer)), connected_timespan_(std::move(connected_timespan)), + upstream_host_(upstream_host), config_(config) { config_->stats().upstream_flush_total_.inc(); config_->stats().upstream_flush_active_.inc(); } @@ -555,7 +564,8 @@ void Drainer::onEvent(Network::ConnectionEvent event) { timer_->disableTimer(); } config_->stats().upstream_flush_active_.dec(); - parent_.remove(*this, upstream_conn_data_->connection().dispatcher()); + finalizeConnectionStats(*upstream_host_, *connected_timespan_); + parent_.remove(*this, upstream_connection_->dispatcher()); } } @@ -582,7 +592,7 @@ void Drainer::onBytesSent() { void Drainer::cancelDrain() { // This sends onEvent(LocalClose). - upstream_conn_data_->connection().close(Network::ConnectionCloseType::NoFlush); + upstream_connection_->close(Network::ConnectionCloseType::NoFlush); } } // namespace TcpProxy diff --git a/source/common/tcp_proxy/tcp_proxy.h b/source/common/tcp_proxy/tcp_proxy.h index 61a803db85cca..3c9173edc3328 100644 --- a/source/common/tcp_proxy/tcp_proxy.h +++ b/source/common/tcp_proxy/tcp_proxy.h @@ -14,7 +14,6 @@ #include "envoy/server/filter_config.h" #include "envoy/stats/stats_macros.h" #include "envoy/stats/timespan.h" -#include "envoy/tcp/conn_pool.h" #include "envoy/upstream/cluster_manager.h" #include "envoy/upstream/upstream.h" @@ -139,7 +138,6 @@ typedef std::shared_ptr ConfigSharedPtr; * be proxied back and forth between the two connections. */ class Filter : public Network::ReadFilter, - Tcp::ConnectionPool::Callbacks, Upstream::LoadBalancerContext, protected Logger::Loggable { public: @@ -151,12 +149,6 @@ class Filter : public Network::ReadFilter, Network::FilterStatus onNewConnection() override { return initializeUpstreamConnection(); } void initializeReadFilterCallbacks(Network::ReadFilterCallbacks& callbacks) override; - // Tcp::ConnectionPool::Callbacks - void onPoolFailure(Tcp::ConnectionPool::PoolFailureReason reason, - Upstream::HostDescriptionConstSharedPtr host) override; - void onPoolReady(Tcp::ConnectionPool::ConnectionDataPtr&& conn_data, - Upstream::HostDescriptionConstSharedPtr host) override; - // Upstream::LoadBalancerContext absl::optional computeHashKey() override { return {}; } const Router::MetadataMatchCriteria* metadataMatchCriteria() override { @@ -174,15 +166,18 @@ class Filter : public Network::ReadFilter, void readDisableUpstream(bool disable); void readDisableDownstream(bool disable); - struct UpstreamCallbacks : public Tcp::ConnectionPool::UpstreamCallbacks { + struct UpstreamCallbacks : public Network::ConnectionCallbacks, + public Network::ReadFilterBaseImpl { UpstreamCallbacks(Filter* parent) : parent_(parent) {} - // Tcp::ConnectionPool::UpstreamCallbacks - void onUpstreamData(Buffer::Instance& data, bool end_stream) override; + // Network::ConnectionCallbacks void onEvent(Network::ConnectionEvent event) override; void onAboveWriteBufferHighWatermark() override; void onBelowWriteBufferLowWatermark() override; + // Network::ReadFilter + Network::FilterStatus onData(Buffer::Instance& data, bool end_stream) override; + void onBytesSent(); void onIdleTimeout(); void drain(Drainer& drainer); @@ -239,6 +234,7 @@ class Filter : public Network::ReadFilter, void onDownstreamEvent(Network::ConnectionEvent event); void onUpstreamData(Buffer::Instance& data, bool end_stream); void onUpstreamEvent(Network::ConnectionEvent event); + void finalizeUpstreamConnectionStats(); void onIdleTimeout(); void resetIdleTimer(); void disableIdleTimer(); @@ -246,26 +242,29 @@ class Filter : public Network::ReadFilter, const ConfigSharedPtr config_; Upstream::ClusterManager& cluster_manager_; Network::ReadFilterCallbacks* read_callbacks_{}; - Tcp::ConnectionPool::Cancellable* upstream_handle_{}; - Tcp::ConnectionPool::ConnectionDataPtr upstream_conn_data_; + Network::ClientConnectionPtr upstream_connection_; DownstreamCallbacks downstream_callbacks_; + Event::TimerPtr connect_timeout_timer_; Event::TimerPtr idle_timer_; + Stats::TimespanPtr connect_timespan_; + Stats::TimespanPtr connected_timespan_; std::shared_ptr upstream_callbacks_; // shared_ptr required for passing as a // read filter. RequestInfo::RequestInfoImpl request_info_; uint32_t connect_attempts_{}; - bool connecting_{}; }; -// This class deals with an upstream connection that needs to finish flushing, when the downstream -// connection has been closed. The TcpProxy is destroyed when the downstream connection is closed, -// so handling the upstream connection here allows it to finish draining or timeout. +// This class holds ownership of an upstream connection that needs to finish +// flushing, when the downstream connection has been closed. The TcpProxy is +// destroyed when the downstream connection is closed, so moving the upstream +// connection here allows it to finish draining or timeout. class Drainer : public Event::DeferredDeletable { public: Drainer(UpstreamDrainManager& parent, const Config::SharedConfigSharedPtr& config, const std::shared_ptr& callbacks, - Tcp::ConnectionPool::ConnectionDataPtr&& conn_data, Event::TimerPtr&& idle_timer, - const Upstream::HostDescriptionConstSharedPtr& upstream_host); + Network::ClientConnectionPtr&& connection, Event::TimerPtr&& idle_timer, + const Upstream::HostDescriptionConstSharedPtr& upstream_host, + Stats::TimespanPtr&& connected_timespan); void onEvent(Network::ConnectionEvent event); void onData(Buffer::Instance& data, bool end_stream); @@ -276,8 +275,9 @@ class Drainer : public Event::DeferredDeletable { private: UpstreamDrainManager& parent_; std::shared_ptr callbacks_; - Tcp::ConnectionPool::ConnectionDataPtr upstream_conn_data_; + Network::ClientConnectionPtr upstream_connection_; Event::TimerPtr timer_; + Stats::TimespanPtr connected_timespan_; Upstream::HostDescriptionConstSharedPtr upstream_host_; Config::SharedConfigSharedPtr config_; }; @@ -288,10 +288,11 @@ class UpstreamDrainManager : public ThreadLocal::ThreadLocalObject { public: ~UpstreamDrainManager(); void add(const Config::SharedConfigSharedPtr& config, - Tcp::ConnectionPool::ConnectionDataPtr&& upstream_conn_data, + Network::ClientConnectionPtr&& upstream_connection, const std::shared_ptr& callbacks, Event::TimerPtr&& idle_timer, - const Upstream::HostDescriptionConstSharedPtr& upstream_host); + const Upstream::HostDescriptionConstSharedPtr& upstream_host, + Stats::TimespanPtr&& connected_timespan); void remove(Drainer& drainer, Event::Dispatcher& dispatcher); private: diff --git a/source/extensions/filters/network/thrift_proxy/router/router_impl.cc b/source/extensions/filters/network/thrift_proxy/router/router_impl.cc index 09d1ac142c2d5..2b4fb77946c35 100644 --- a/source/extensions/filters/network/thrift_proxy/router/router_impl.cc +++ b/source/extensions/filters/network/thrift_proxy/router/router_impl.cc @@ -224,7 +224,7 @@ void Router::UpstreamRequest::start() { void Router::UpstreamRequest::resetStream() { if (conn_data_ != nullptr) { conn_data_->connection().close(Network::ConnectionCloseType::NoFlush); - conn_data_.reset(); + conn_data_ = nullptr; } } @@ -235,10 +235,10 @@ void Router::UpstreamRequest::onPoolFailure(Tcp::ConnectionPool::PoolFailureReas onResetStream(reason); } -void Router::UpstreamRequest::onPoolReady(Tcp::ConnectionPool::ConnectionDataPtr&& conn_data, +void Router::UpstreamRequest::onPoolReady(Tcp::ConnectionPool::ConnectionData& conn_data, Upstream::HostDescriptionConstSharedPtr host) { onUpstreamHostSelected(host); - conn_data_ = std::move(conn_data); + conn_data_ = &conn_data; conn_data_->addUpstreamCallbacks(parent_); conn_pool_handle_ = nullptr; @@ -263,7 +263,10 @@ void Router::UpstreamRequest::onRequestComplete() { request_complete_ = true; } void Router::UpstreamRequest::onResponseComplete() { response_complete_ = true; - conn_data_.reset(); + if (conn_data_ != nullptr) { + conn_data_->release(); + } + conn_data_ = nullptr; } void Router::UpstreamRequest::onUpstreamHostSelected(Upstream::HostDescriptionConstSharedPtr host) { diff --git a/source/extensions/filters/network/thrift_proxy/router/router_impl.h b/source/extensions/filters/network/thrift_proxy/router/router_impl.h index d298d38b354cd..117c99ca2635b 100644 --- a/source/extensions/filters/network/thrift_proxy/router/router_impl.h +++ b/source/extensions/filters/network/thrift_proxy/router/router_impl.h @@ -112,7 +112,7 @@ class Router : public Tcp::ConnectionPool::UpstreamCallbacks, // Tcp::ConnectionPool::Callbacks void onPoolFailure(Tcp::ConnectionPool::PoolFailureReason reason, Upstream::HostDescriptionConstSharedPtr host) override; - void onPoolReady(Tcp::ConnectionPool::ConnectionDataPtr&& conn, + void onPoolReady(Tcp::ConnectionPool::ConnectionData& conn, Upstream::HostDescriptionConstSharedPtr host) override; void onRequestComplete(); @@ -127,7 +127,7 @@ class Router : public Tcp::ConnectionPool::UpstreamCallbacks, const int32_t seq_id_; Tcp::ConnectionPool::Cancellable* conn_pool_handle_{}; - Tcp::ConnectionPool::ConnectionDataPtr conn_data_; + Tcp::ConnectionPool::ConnectionData* conn_data_{}; Upstream::HostDescriptionConstSharedPtr upstream_host_; TransportPtr transport_; ProtocolType proto_type_{ProtocolType::Auto}; diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index 3e7c089b59d16..a036e9edcadda 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -312,8 +312,6 @@ class HttpConnectionManagerImplTest : public Test, public ConnectionManagerConfi ConnectionManagerListenerStats listener_stats_; bool proxy_100_continue_ = false; Http::Http1Settings http1_settings_; - NiceMock upstream_conn_; // for websocket tests - NiceMock conn_pool_; // for websocket tests // TODO(mattklein123): Not all tests have been converted over to better setup. Convert the rest. MockStreamEncoder response_encoder_; @@ -1506,7 +1504,8 @@ TEST_F(HttpConnectionManagerImplTest, WebSocketNoThreadLocalCluster) { TEST_F(HttpConnectionManagerImplTest, WebSocketNoConnInPool) { setup(false, ""); - EXPECT_CALL(cluster_manager_, tcpConnPoolForCluster(_, _, _)).WillOnce(Return(nullptr)); + Upstream::MockHost::MockCreateConnectionData conn_info; + EXPECT_CALL(cluster_manager_, tcpConnForCluster_(_, _)).WillOnce(Return(conn_info)); expectOnUpstreamInitFailure(); EXPECT_EQ(1U, stats_.named_.downstream_cx_websocket_active_.value()); @@ -1521,37 +1520,14 @@ TEST_F(HttpConnectionManagerImplTest, WebSocketNoConnInPool) { TEST_F(HttpConnectionManagerImplTest, WebSocketDataAfterConnectFail) { setup(false, ""); - EXPECT_CALL(cluster_manager_, tcpConnPoolForCluster(_, _, _)).WillOnce(Return(&conn_pool_)); - - StreamDecoder* decoder = nullptr; - NiceMock encoder; - - configureRouteForWebsocket(route_config_provider_.route_config_->route_->route_entry_); - - EXPECT_CALL(*codec_, dispatch(_)).WillOnce(Invoke([&](Buffer::Instance& data) -> void { - decoder = &conn_manager_->newStream(encoder); - HeaderMapPtr headers{new TestHeaderMapImpl{{":authority", "host"}, - {":method", "GET"}, - {":path", "/"}, - {"connection", "Upgrade"}, - {"upgrade", "websocket"}}}; - decoder->decodeHeaders(std::move(headers), true); - data.drain(4); - })); - - Buffer::OwnedImpl fake_input("1234"); - conn_manager_->onData(fake_input, false); + Upstream::MockHost::MockCreateConnectionData conn_info; + EXPECT_CALL(cluster_manager_, tcpConnForCluster_(_, _)).WillOnce(Return(conn_info)); + expectOnUpstreamInitFailure(); EXPECT_EQ(1U, stats_.named_.downstream_cx_websocket_active_.value()); EXPECT_EQ(1U, stats_.named_.downstream_cx_websocket_total_.value()); EXPECT_EQ(0U, stats_.named_.downstream_cx_http1_active_.value()); - EXPECT_CALL(encoder, encodeHeaders(_, true)) - .WillOnce(Invoke([](const HeaderMap& headers, bool) -> void { - EXPECT_STREQ("504", headers.Status()->value().c_str()); - })); - - conn_pool_.poolFailure(Tcp::ConnectionPool::PoolFailureReason::RemoteConnectionFailure); filter_callbacks_.connection_.dispatcher_.clearDeferredDeleteList(); // This should get dropped, with no ASSERT or crash. @@ -1571,14 +1547,13 @@ TEST_F(HttpConnectionManagerImplTest, WebSocketMetadataMatch) { .WillByDefault(Return( &route_config_provider_.route_config_->route_->route_entry_.metadata_matches_criteria_)); - EXPECT_CALL(cluster_manager_, tcpConnPoolForCluster(_, _, _)) - .WillOnce(Invoke([&](const std::string&, Upstream::ResourcePriority, - Upstream::LoadBalancerContext* context) - -> Tcp::ConnectionPool::MockInstance* { + EXPECT_CALL(cluster_manager_, tcpConnForCluster_(_, _)) + .WillOnce(Invoke([&](const std::string&, Upstream::LoadBalancerContext* context) + -> Upstream::MockHost::MockCreateConnectionData { EXPECT_EQ( context->metadataMatchCriteria(), &route_config_provider_.route_config_->route_->route_entry_.metadata_matches_criteria_); - return nullptr; + return {}; })); expectOnUpstreamInitFailure(); @@ -1589,8 +1564,21 @@ TEST_F(HttpConnectionManagerImplTest, WebSocketMetadataMatch) { TEST_F(HttpConnectionManagerImplTest, WebSocketConnectTimeoutError) { setup(false, ""); - EXPECT_CALL(cluster_manager_, tcpConnPoolForCluster("fake_cluster", _, _)) - .WillOnce(Return(&conn_pool_)); + Event::MockTimer* connect_timer = + new NiceMock(&filter_callbacks_.connection_.dispatcher_); + NiceMock* upstream_connection = + new NiceMock(); + Upstream::MockHost::MockCreateConnectionData conn_info; + + conn_info.connection_ = upstream_connection; + conn_info.host_description_.reset(new Upstream::HostImpl( + cluster_manager_.thread_local_cluster_.cluster_.info_, "newhost", + Network::Utility::resolveUrl("tcp://127.0.0.1:80"), + envoy::api::v2::core::Metadata::default_instance(), 1, + envoy::api::v2::core::Locality().default_instance(), + envoy::api::v2::endpoint::Endpoint::HealthCheckConfig().default_instance())); + EXPECT_CALL(*connect_timer, enableTimer(_)); + EXPECT_CALL(cluster_manager_, tcpConnForCluster_("fake_cluster", _)).WillOnce(Return(conn_info)); StreamDecoder* decoder = nullptr; NiceMock encoder; @@ -1608,15 +1596,15 @@ TEST_F(HttpConnectionManagerImplTest, WebSocketConnectTimeoutError) { data.drain(4); })); - Buffer::OwnedImpl fake_input("1234"); - conn_manager_->onData(fake_input, false); - EXPECT_CALL(encoder, encodeHeaders(_, true)) .WillOnce(Invoke([](const HeaderMap& headers, bool) -> void { EXPECT_STREQ("504", headers.Status()->value().c_str()); })); - conn_pool_.poolFailure(Tcp::ConnectionPool::PoolFailureReason::Timeout); + Buffer::OwnedImpl fake_input("1234"); + conn_manager_->onData(fake_input, false); + + connect_timer->callback_(); filter_callbacks_.connection_.dispatcher_.clearDeferredDeleteList(); conn_manager_.reset(); } @@ -1624,8 +1612,21 @@ TEST_F(HttpConnectionManagerImplTest, WebSocketConnectTimeoutError) { TEST_F(HttpConnectionManagerImplTest, WebSocketConnectionFailure) { setup(false, ""); - EXPECT_CALL(cluster_manager_, tcpConnPoolForCluster("fake_cluster", _, _)) - .WillOnce(Return(&conn_pool_)); + Event::MockTimer* connect_timer = + new NiceMock(&filter_callbacks_.connection_.dispatcher_); + NiceMock* upstream_connection = + new NiceMock(); + Upstream::MockHost::MockCreateConnectionData conn_info; + + conn_info.connection_ = upstream_connection; + conn_info.host_description_.reset(new Upstream::HostImpl( + cluster_manager_.thread_local_cluster_.cluster_.info_, "newhost", + Network::Utility::resolveUrl("tcp://127.0.0.1:80"), + envoy::api::v2::core::Metadata::default_instance(), 1, + envoy::api::v2::core::Locality().default_instance(), + envoy::api::v2::endpoint::Endpoint::HealthCheckConfig().default_instance())); + EXPECT_CALL(*connect_timer, enableTimer(_)); + EXPECT_CALL(cluster_manager_, tcpConnForCluster_("fake_cluster", _)).WillOnce(Return(conn_info)); StreamDecoder* decoder = nullptr; NiceMock encoder; @@ -1643,16 +1644,16 @@ TEST_F(HttpConnectionManagerImplTest, WebSocketConnectionFailure) { data.drain(4); })); - Buffer::OwnedImpl fake_input("1234"); - conn_manager_->onData(fake_input, false); - EXPECT_CALL(encoder, encodeHeaders(_, true)) .WillOnce(Invoke([](const HeaderMap& headers, bool) -> void { EXPECT_STREQ("504", headers.Status()->value().c_str()); })); - conn_pool_.poolFailure(Tcp::ConnectionPool::PoolFailureReason::RemoteConnectionFailure); + Buffer::OwnedImpl fake_input("1234"); + conn_manager_->onData(fake_input, false); + // expectOnUpstreamInitFailure("504"); + upstream_connection->raiseEvent(Network::ConnectionEvent::RemoteClose); filter_callbacks_.connection_.dispatcher_.clearDeferredDeleteList(); conn_manager_.reset(); } @@ -1662,6 +1663,9 @@ TEST_F(HttpConnectionManagerImplTest, WebSocketPrefixAndAutoHostRewrite) { StreamDecoder* decoder = nullptr; NiceMock encoder; + NiceMock* upstream_connection = + new NiceMock(); + Upstream::MockHost::MockCreateConnectionData conn_info; HeaderMapPtr headers{new TestHeaderMapImpl{{":authority", "host"}, {":method", "GET"}, {":path", "/"}, @@ -1669,8 +1673,14 @@ TEST_F(HttpConnectionManagerImplTest, WebSocketPrefixAndAutoHostRewrite) { {"upgrade", "websocket"}}}; auto raw_header_ptr = headers.get(); - EXPECT_CALL(cluster_manager_, tcpConnPoolForCluster("fake_cluster", _, _)) - .WillOnce(Return(&conn_pool_)); + conn_info.connection_ = upstream_connection; + conn_info.host_description_.reset(new Upstream::HostImpl( + cluster_manager_.thread_local_cluster_.cluster_.info_, "newhost", + Network::Utility::resolveUrl("tcp://127.0.0.1:80"), + envoy::api::v2::core::Metadata::default_instance(), 1, + envoy::api::v2::core::Locality().default_instance(), + envoy::api::v2::endpoint::Endpoint::HealthCheckConfig().default_instance())); + EXPECT_CALL(cluster_manager_, tcpConnForCluster_("fake_cluster", _)).WillOnce(Return(conn_info)); configureRouteForWebsocket(route_config_provider_.route_config_->route_->route_entry_); @@ -1687,9 +1697,7 @@ TEST_F(HttpConnectionManagerImplTest, WebSocketPrefixAndAutoHostRewrite) { Buffer::OwnedImpl fake_input("1234"); conn_manager_->onData(fake_input, false); - - conn_pool_.host_->hostname_ = "newhost"; - conn_pool_.poolReady(upstream_conn_); + upstream_connection->raiseEvent(Network::ConnectionEvent::Connected); // rewritten authority header when auto_host_rewrite is true EXPECT_STREQ("newhost", raw_header_ptr->Host()->value().c_str()); @@ -1705,8 +1713,21 @@ TEST_F(HttpConnectionManagerImplTest, WebSocketPrefixAndAutoHostRewrite) { TEST_F(HttpConnectionManagerImplTest, WebSocketEarlyData) { setup(false, ""); - EXPECT_CALL(cluster_manager_, tcpConnPoolForCluster("fake_cluster", _, _)) - .WillOnce(Return(&conn_pool_)); + Event::MockTimer* connect_timer = + new NiceMock(&filter_callbacks_.connection_.dispatcher_); + NiceMock* upstream_connection = + new NiceMock(); + Upstream::MockHost::MockCreateConnectionData conn_info; + + conn_info.connection_ = upstream_connection; + conn_info.host_description_.reset(new Upstream::HostImpl( + cluster_manager_.thread_local_cluster_.cluster_.info_, "newhost", + Network::Utility::resolveUrl("tcp://127.0.0.1:80"), + envoy::api::v2::core::Metadata::default_instance(), 1, + envoy::api::v2::core::Locality().default_instance(), + envoy::api::v2::endpoint::Endpoint::HealthCheckConfig().default_instance())); + EXPECT_CALL(*connect_timer, enableTimer(_)); + EXPECT_CALL(cluster_manager_, tcpConnForCluster_("fake_cluster", _)).WillOnce(Return(conn_info)); StreamDecoder* decoder = nullptr; NiceMock encoder; @@ -1733,11 +1754,10 @@ TEST_F(HttpConnectionManagerImplTest, WebSocketEarlyData) { conn_manager_->onData(fake_input, false); - EXPECT_CALL(upstream_conn_, write(_, false)); - EXPECT_CALL(upstream_conn_, write(BufferEqual(&early_data), false)); + EXPECT_CALL(*upstream_connection, write(_, false)); + EXPECT_CALL(*upstream_connection, write(BufferEqual(&early_data), false)); EXPECT_CALL(filter_callbacks_.connection_, readDisable(false)); - conn_pool_.poolReady(upstream_conn_); - + upstream_connection->raiseEvent(Network::ConnectionEvent::Connected); filter_callbacks_.connection_.dispatcher_.clearDeferredDeleteList(); conn_manager_.reset(); } @@ -1745,8 +1765,21 @@ TEST_F(HttpConnectionManagerImplTest, WebSocketEarlyData) { TEST_F(HttpConnectionManagerImplTest, WebSocketEarlyDataConnectionFail) { setup(false, ""); - EXPECT_CALL(cluster_manager_, tcpConnPoolForCluster("fake_cluster", _, _)) - .WillOnce(Return(&conn_pool_)); + Event::MockTimer* connect_timer = + new NiceMock(&filter_callbacks_.connection_.dispatcher_); + NiceMock* upstream_connection = + new NiceMock(); + Upstream::MockHost::MockCreateConnectionData conn_info; + + conn_info.connection_ = upstream_connection; + conn_info.host_description_.reset(new Upstream::HostImpl( + cluster_manager_.thread_local_cluster_.cluster_.info_, "newhost", + Network::Utility::resolveUrl("tcp://127.0.0.1:80"), + envoy::api::v2::core::Metadata::default_instance(), 1, + envoy::api::v2::core::Locality().default_instance(), + envoy::api::v2::endpoint::Endpoint::HealthCheckConfig().default_instance())); + EXPECT_CALL(*connect_timer, enableTimer(_)); + EXPECT_CALL(cluster_manager_, tcpConnForCluster_("fake_cluster", _)).WillOnce(Return(conn_info)); StreamDecoder* decoder = nullptr; NiceMock encoder; @@ -1773,7 +1806,7 @@ TEST_F(HttpConnectionManagerImplTest, WebSocketEarlyDataConnectionFail) { conn_manager_->onData(fake_input, false); - conn_pool_.poolFailure(Tcp::ConnectionPool::PoolFailureReason::RemoteConnectionFailure); + upstream_connection->raiseEvent(Network::ConnectionEvent::RemoteClose); filter_callbacks_.connection_.dispatcher_.clearDeferredDeleteList(); // This should get dropped, with no crash or ASSERT. @@ -1786,8 +1819,21 @@ TEST_F(HttpConnectionManagerImplTest, WebSocketEarlyDataConnectionFail) { TEST_F(HttpConnectionManagerImplTest, WebSocketEarlyEndStream) { setup(false, ""); - EXPECT_CALL(cluster_manager_, tcpConnPoolForCluster("fake_cluster", _, _)) - .WillOnce(Return(&conn_pool_)); + Event::MockTimer* connect_timer = + new NiceMock(&filter_callbacks_.connection_.dispatcher_); + NiceMock* upstream_connection = + new NiceMock(); + Upstream::MockHost::MockCreateConnectionData conn_info; + + conn_info.connection_ = upstream_connection; + conn_info.host_description_.reset(new Upstream::HostImpl( + cluster_manager_.thread_local_cluster_.cluster_.info_, "newhost", + Network::Utility::resolveUrl("tcp://127.0.0.1:80"), + envoy::api::v2::core::Metadata::default_instance(), 1, + envoy::api::v2::core::Locality().default_instance(), + envoy::api::v2::endpoint::Endpoint::HealthCheckConfig().default_instance())); + EXPECT_CALL(*connect_timer, enableTimer(_)); + EXPECT_CALL(cluster_manager_, tcpConnForCluster_("fake_cluster", _)).WillOnce(Return(conn_info)); StreamDecoder* decoder = nullptr; NiceMock encoder; @@ -1809,9 +1855,9 @@ TEST_F(HttpConnectionManagerImplTest, WebSocketEarlyEndStream) { Buffer::OwnedImpl fake_input("1234"); conn_manager_->onData(fake_input, true); - EXPECT_CALL(upstream_conn_, write(_, false)); - EXPECT_CALL(upstream_conn_, write(_, true)).Times(0); - conn_pool_.poolReady(upstream_conn_); + EXPECT_CALL(*upstream_connection, write(_, false)); + EXPECT_CALL(*upstream_connection, write(_, true)).Times(0); + upstream_connection->raiseEvent(Network::ConnectionEvent::Connected); filter_callbacks_.connection_.dispatcher_.clearDeferredDeleteList(); conn_manager_.reset(); } diff --git a/test/common/network/filter_manager_impl_test.cc b/test/common/network/filter_manager_impl_test.cc index 9bcb366c4c42f..c8469a99f9336 100644 --- a/test/common/network/filter_manager_impl_test.cc +++ b/test/common/network/filter_manager_impl_test.cc @@ -153,8 +153,6 @@ TEST_F(NetworkFilterManagerTest, RateLimitAndTcpProxy) { InSequence s; NiceMock factory_context; NiceMock connection; - NiceMock upstream_connection; - NiceMock conn_pool; FilterManagerImpl manager(connection, *this); std::string rl_json = R"EOF( @@ -203,15 +201,21 @@ TEST_F(NetworkFilterManagerTest, RateLimitAndTcpProxy) { EXPECT_EQ(manager.initializeReadFilters(), true); - EXPECT_CALL(factory_context.cluster_manager_, tcpConnPoolForCluster("fake_cluster", _, _)) - .WillOnce(Return(&conn_pool)); + NiceMock* upstream_connection = + new NiceMock(); + Upstream::MockHost::MockCreateConnectionData conn_info; + conn_info.connection_ = upstream_connection; + conn_info.host_description_ = Upstream::makeTestHost( + factory_context.cluster_manager_.thread_local_cluster_.cluster_.info_, "tcp://127.0.0.1:80"); + EXPECT_CALL(factory_context.cluster_manager_, tcpConnForCluster_("fake_cluster", _)) + .WillOnce(Return(conn_info)); request_callbacks->complete(RateLimit::LimitStatus::OK); - conn_pool.poolReady(upstream_connection); + upstream_connection->raiseEvent(Network::ConnectionEvent::Connected); Buffer::OwnedImpl buffer("hello"); - EXPECT_CALL(upstream_connection, write(BufferEqual(&buffer), _)); + EXPECT_CALL(*upstream_connection, write(BufferEqual(&buffer), _)); read_buffer_.add("hello"); manager.onRead(); } diff --git a/test/common/tcp/conn_pool_test.cc b/test/common/tcp/conn_pool_test.cc index a6a080f83910d..90961972724e1 100644 --- a/test/common/tcp/conn_pool_test.cc +++ b/test/common/tcp/conn_pool_test.cc @@ -34,9 +34,9 @@ namespace Tcp { * Mock callbacks used for conn pool testing. */ struct ConnPoolCallbacks : public Tcp::ConnectionPool::Callbacks { - void onPoolReady(ConnectionPool::ConnectionDataPtr&& conn, + void onPoolReady(ConnectionPool::ConnectionData& conn, Upstream::HostDescriptionConstSharedPtr host) override { - conn_data_ = std::move(conn); + conn_data_ = &conn; host_ = host; pool_ready_.ready(); } @@ -50,7 +50,7 @@ struct ConnPoolCallbacks : public Tcp::ConnectionPool::Callbacks { ReadyWatcher pool_failure_; ReadyWatcher pool_ready_; - ConnectionPool::ConnectionDataPtr conn_data_{}; + ConnectionPool::ConnectionData* conn_data_{}; absl::optional reason_; Upstream::HostDescriptionConstSharedPtr host_; }; @@ -232,7 +232,7 @@ struct ActiveTestConn { void expectNewConn() { EXPECT_CALL(callbacks_.pool_ready_, ready()); } - void releaseConn() { callbacks_.conn_data_.reset(); } + void releaseConn() { callbacks_.conn_data_->release(); } void verifyConn() { EXPECT_EQ(&callbacks_.conn_data_->connection(), @@ -589,7 +589,7 @@ TEST_F(TcpConnPoolImplTest, DisconnectWhilePending) { conn_pool_.test_conns_[0].connection_->raiseEvent(Network::ConnectionEvent::Connected); EXPECT_CALL(conn_pool_, onConnReleasedForTest()); - callbacks2.conn_data_.reset(); + callbacks2.conn_data_->release(); // Disconnect EXPECT_CALL(conn_pool_, onConnDestroyedForTest()); @@ -625,11 +625,11 @@ TEST_F(TcpConnPoolImplTest, MaxConnections) { EXPECT_CALL(conn_pool_, onConnReleasedForTest()); conn_pool_.expectEnableUpstreamReady(); EXPECT_CALL(callbacks2.pool_ready_, ready()); - callbacks.conn_data_.reset(); + callbacks.conn_data_->release(); conn_pool_.expectAndRunUpstreamReady(); EXPECT_CALL(conn_pool_, onConnReleasedForTest()); - callbacks2.conn_data_.reset(); + callbacks2.conn_data_->release(); // Cause the connection to go away. EXPECT_CALL(conn_pool_, onConnDestroyedForTest()); @@ -657,7 +657,7 @@ TEST_F(TcpConnPoolImplTest, MaxRequestsPerConnection) { EXPECT_CALL(conn_pool_, onConnReleasedForTest()); EXPECT_CALL(conn_pool_, onConnDestroyedForTest()); - callbacks.conn_data_.reset(); + callbacks.conn_data_->release(); dispatcher_.clearDeferredDeleteList(); EXPECT_EQ(0U, cluster_->stats_.upstream_cx_destroy_with_active_rq_.value()); @@ -743,7 +743,7 @@ TEST_F(TcpConnPoolImplDestructorTest, TestReadyConnectionsAreClosed) { prepareConn(); // Transition connection to ready list - callbacks_->conn_data_.reset(); + callbacks_->conn_data_->release(); EXPECT_CALL(*connection_, close(Network::ConnectionCloseType::NoFlush)); EXPECT_CALL(dispatcher_, clearDeferredDeleteList()); diff --git a/test/common/tcp_proxy/tcp_proxy_test.cc b/test/common/tcp_proxy/tcp_proxy_test.cc index 73568c61de74e..a8616176943ba 100644 --- a/test/common/tcp_proxy/tcp_proxy_test.cc +++ b/test/common/tcp_proxy/tcp_proxy_test.cc @@ -18,7 +18,6 @@ #include "test/mocks/network/mocks.h" #include "test/mocks/runtime/mocks.h" #include "test/mocks/server/mocks.h" -#include "test/mocks/tcp/mocks.h" #include "test/mocks/upstream/host.h" #include "test/mocks/upstream/mocks.h" #include "test/test_common/printers.h" @@ -26,7 +25,6 @@ #include "gmock/gmock.h" #include "gtest/gtest.h" -using testing::Invoke; using testing::MatchesRegex; using testing::NiceMock; using testing::Return; @@ -381,50 +379,53 @@ class TcpProxyTest : public testing::Test { upstream_local_address_ = Network::Utility::resolveUrl("tcp://2.2.2.2:50000"); upstream_remote_address_ = Network::Utility::resolveUrl("tcp://127.0.0.1:80"); if (connections >= 1) { + { + testing::InSequence sequence; + for (uint32_t i = 0; i < connections; i++) { + connect_timers_.push_back( + new NiceMock(&filter_callbacks_.connection_.dispatcher_)); + EXPECT_CALL(*connect_timers_.at(i), enableTimer(_)); + } + } + for (uint32_t i = 0; i < connections; i++) { - upstream_connections_.push_back( - std::make_unique>()); - upstream_connection_data_.push_back( - std::make_unique>()); - ON_CALL(*upstream_connection_data_.back(), connection()) - .WillByDefault(ReturnRef(*upstream_connections_.back())); + upstream_connections_.push_back(new NiceMock()); upstream_hosts_.push_back(std::make_shared>()); - conn_pool_handles_.push_back( - std::make_unique>()); + conn_infos_.push_back(Upstream::MockHost::MockCreateConnectionData()); + conn_infos_.at(i).connection_ = upstream_connections_.back(); + conn_infos_.at(i).host_description_ = upstream_hosts_.back(); ON_CALL(*upstream_hosts_.at(i), cluster()) .WillByDefault(ReturnPointee( factory_context_.cluster_manager_.thread_local_cluster_.cluster_.info_)); ON_CALL(*upstream_hosts_.at(i), address()).WillByDefault(Return(upstream_remote_address_)); upstream_connections_.at(i)->local_address_ = upstream_local_address_; + EXPECT_CALL(*upstream_connections_.at(i), addReadFilter(_)) + .WillOnce(SaveArg<0>(&upstream_read_filter_)); EXPECT_CALL(*upstream_connections_.at(i), dispatcher()) .WillRepeatedly(ReturnRef(filter_callbacks_.connection_.dispatcher_)); + EXPECT_CALL(*upstream_connections_.at(i), enableHalfClose(true)); } } { testing::InSequence sequence; for (uint32_t i = 0; i < connections; i++) { - EXPECT_CALL(factory_context_.cluster_manager_, tcpConnPoolForCluster("fake_cluster", _, _)) - .WillOnce(Return(&conn_pool_)) - .RetiresOnSaturation(); - EXPECT_CALL(conn_pool_, newConnection(_)) - .WillOnce(Invoke( - [=](Tcp::ConnectionPool::Callbacks& cb) -> Tcp::ConnectionPool::Cancellable* { - conn_pool_callbacks_.push_back(&cb); - return conn_pool_handles_.at(i).get(); - })) + EXPECT_CALL(factory_context_.cluster_manager_, tcpConnForCluster_("fake_cluster", _)) + .WillOnce(Return(conn_infos_.at(i))) .RetiresOnSaturation(); } - EXPECT_CALL(factory_context_.cluster_manager_, tcpConnPoolForCluster("fake_cluster", _, _)) - .WillRepeatedly(Return(nullptr)); + EXPECT_CALL(factory_context_.cluster_manager_, tcpConnForCluster_("fake_cluster", _)) + .WillRepeatedly(Return(Upstream::MockHost::MockCreateConnectionData())); } filter_.reset(new Filter(config_, factory_context_.cluster_manager_)); EXPECT_CALL(filter_callbacks_.connection_, readDisable(true)); EXPECT_CALL(filter_callbacks_.connection_, enableHalfClose(true)); filter_->initializeReadFilterCallbacks(filter_callbacks_); - EXPECT_EQ(Network::FilterStatus::StopIteration, filter_->onNewConnection()); + EXPECT_EQ(connections >= 1 ? Network::FilterStatus::Continue + : Network::FilterStatus::StopIteration, + filter_->onNewConnection()); EXPECT_EQ(absl::optional(), filter_->computeHashKey()); EXPECT_EQ(&filter_callbacks_.connection_, filter_->downstreamConnection()); @@ -434,37 +435,19 @@ class TcpProxyTest : public testing::Test { void setup(uint32_t connections) { setup(connections, defaultConfig()); } void raiseEventUpstreamConnected(uint32_t conn_index) { + EXPECT_CALL(*connect_timers_.at(conn_index), disableTimer()); EXPECT_CALL(filter_callbacks_.connection_, readDisable(false)); - EXPECT_CALL(*upstream_connection_data_.at(conn_index), addUpstreamCallbacks(_)) - .WillOnce(Invoke([=](Tcp::ConnectionPool::UpstreamCallbacks& cb) -> void { - upstream_callbacks_ = &cb; - - // Simulate TCP conn pool upstream callbacks. This is safe because the TCP proxy never - // releases a connection so all events go to the same UpstreamCallbacks instance. - upstream_connections_.at(conn_index)->addConnectionCallbacks(cb); - })); - EXPECT_CALL(*upstream_connections_.at(conn_index), enableHalfClose(true)); - conn_pool_callbacks_.at(conn_index) - ->onPoolReady(std::move(upstream_connection_data_.at(conn_index)), - upstream_hosts_.at(conn_index)); - } - - void raiseEventUpstreamConnectFailed(uint32_t conn_index, - Tcp::ConnectionPool::PoolFailureReason reason) { - conn_pool_callbacks_.at(conn_index)->onPoolFailure(reason, upstream_hosts_.at(conn_index)); + upstream_connections_.at(conn_index)->raiseEvent(Network::ConnectionEvent::Connected); } ConfigSharedPtr config_; NiceMock filter_callbacks_; NiceMock factory_context_; std::vector>> upstream_hosts_{}; - std::vector>> upstream_connections_{}; - std::vector>> - upstream_connection_data_{}; - std::vector conn_pool_callbacks_; - std::vector>> conn_pool_handles_; - NiceMock conn_pool_; - Tcp::ConnectionPool::UpstreamCallbacks* upstream_callbacks_; + std::vector*> upstream_connections_{}; + std::vector conn_infos_; + Network::ReadFilterSharedPtr upstream_read_filter_; + std::vector*> connect_timers_; std::unique_ptr filter_; StringViewSaver access_log_data_; Network::Address::InstanceConstSharedPtr upstream_local_address_; @@ -478,65 +461,67 @@ TEST_F(TcpProxyTest, HalfCloseProxy) { EXPECT_CALL(filter_callbacks_.connection_, close(_)).Times(0); EXPECT_CALL(*upstream_connections_.at(0), close(_)).Times(0); - raiseEventUpstreamConnected(0); - Buffer::OwnedImpl buffer("hello"); EXPECT_CALL(*upstream_connections_.at(0), write(BufferEqual(&buffer), true)); filter_->onData(buffer, true); + raiseEventUpstreamConnected(0); + Buffer::OwnedImpl response("world"); EXPECT_CALL(filter_callbacks_.connection_, write(BufferEqual(&response), true)); - upstream_callbacks_->onUpstreamData(response, true); + upstream_read_filter_->onData(response, true); EXPECT_CALL(filter_callbacks_.connection_, close(_)); - upstream_callbacks_->onEvent(Network::ConnectionEvent::RemoteClose); + EXPECT_CALL(filter_callbacks_.connection_.dispatcher_, + deferredDelete_(upstream_connections_.at(0))); + upstream_connections_.at(0)->raiseEvent(Network::ConnectionEvent::RemoteClose); } // Test that downstream is closed after an upstream LocalClose. TEST_F(TcpProxyTest, UpstreamLocalDisconnect) { setup(1); - raiseEventUpstreamConnected(0); - Buffer::OwnedImpl buffer("hello"); EXPECT_CALL(*upstream_connections_.at(0), write(BufferEqual(&buffer), false)); filter_->onData(buffer, false); + raiseEventUpstreamConnected(0); + Buffer::OwnedImpl response("world"); EXPECT_CALL(filter_callbacks_.connection_, write(BufferEqual(&response), _)); - upstream_callbacks_->onUpstreamData(response, false); + upstream_read_filter_->onData(response, false); EXPECT_CALL(filter_callbacks_.connection_, close(_)); - upstream_callbacks_->onEvent(Network::ConnectionEvent::LocalClose); + upstream_connections_.at(0)->raiseEvent(Network::ConnectionEvent::LocalClose); } // Test that downstream is closed after an upstream RemoteClose. TEST_F(TcpProxyTest, UpstreamRemoteDisconnect) { setup(1); - raiseEventUpstreamConnected(0); - Buffer::OwnedImpl buffer("hello"); EXPECT_CALL(*upstream_connections_.at(0), write(BufferEqual(&buffer), false)); filter_->onData(buffer, false); + raiseEventUpstreamConnected(0); + Buffer::OwnedImpl response("world"); EXPECT_CALL(filter_callbacks_.connection_, write(BufferEqual(&response), _)); - upstream_callbacks_->onUpstreamData(response, false); + upstream_read_filter_->onData(response, false); EXPECT_CALL(filter_callbacks_.connection_, close(Network::ConnectionCloseType::FlushWrite)); - upstream_callbacks_->onEvent(Network::ConnectionEvent::RemoteClose); + upstream_connections_.at(0)->raiseEvent(Network::ConnectionEvent::RemoteClose); } // Test that reconnect is attempted after a local connect failure TEST_F(TcpProxyTest, ConnectAttemptsUpstreamLocalFail) { envoy::config::filter::network::tcp_proxy::v2::TcpProxy config = defaultConfig(); config.mutable_max_connect_attempts()->set_value(2); - setup(2, config); - raiseEventUpstreamConnectFailed(0, - Tcp::ConnectionPool::PoolFailureReason::LocalConnectionFailure); + EXPECT_CALL(filter_callbacks_.connection_.dispatcher_, + deferredDelete_(upstream_connections_.at(0))); + upstream_connections_.at(0)->raiseEvent(Network::ConnectionEvent::LocalClose); raiseEventUpstreamConnected(1); EXPECT_EQ(0U, factory_context_.cluster_manager_.thread_local_cluster_.cluster_.info_->stats_store_ @@ -550,8 +535,9 @@ TEST_F(TcpProxyTest, ConnectAttemptsUpstreamRemoteFail) { config.mutable_max_connect_attempts()->set_value(2); setup(2, config); - raiseEventUpstreamConnectFailed(0, - Tcp::ConnectionPool::PoolFailureReason::RemoteConnectionFailure); + EXPECT_CALL(filter_callbacks_.connection_.dispatcher_, + deferredDelete_(upstream_connections_.at(0))); + upstream_connections_.at(0)->raiseEvent(Network::ConnectionEvent::RemoteClose); raiseEventUpstreamConnected(1); EXPECT_EQ(0U, factory_context_.cluster_manager_.thread_local_cluster_.cluster_.info_->stats_store_ @@ -565,7 +551,8 @@ TEST_F(TcpProxyTest, ConnectAttemptsUpstreamTimeout) { config.mutable_max_connect_attempts()->set_value(2); setup(2, config); - raiseEventUpstreamConnectFailed(0, Tcp::ConnectionPool::PoolFailureReason::Timeout); + EXPECT_CALL(*upstream_connections_.at(0), close(Network::ConnectionCloseType::NoFlush)); + connect_timers_.at(0)->callback_(); raiseEventUpstreamConnected(1); EXPECT_EQ(0U, factory_context_.cluster_manager_.thread_local_cluster_.cluster_.info_->stats_store_ @@ -579,21 +566,38 @@ TEST_F(TcpProxyTest, ConnectAttemptsLimit) { config.mutable_max_connect_attempts()->set_value(3); setup(3, config); - EXPECT_CALL(upstream_hosts_.at(0)->outlier_detector_, - putResult(Upstream::Outlier::Result::TIMEOUT)); - EXPECT_CALL(upstream_hosts_.at(1)->outlier_detector_, - putResult(Upstream::Outlier::Result::CONNECT_FAILED)); - EXPECT_CALL(upstream_hosts_.at(2)->outlier_detector_, - putResult(Upstream::Outlier::Result::CONNECT_FAILED)); - - EXPECT_CALL(filter_callbacks_.connection_, close(Network::ConnectionCloseType::NoFlush)); + { + testing::InSequence sequence; + EXPECT_CALL(*upstream_connections_.at(0), close(Network::ConnectionCloseType::NoFlush)); + EXPECT_CALL(filter_callbacks_.connection_.dispatcher_, + deferredDelete_(upstream_connections_.at(0))); + EXPECT_CALL(filter_callbacks_.connection_.dispatcher_, + deferredDelete_(upstream_connections_.at(1))); + EXPECT_CALL(filter_callbacks_.connection_.dispatcher_, + deferredDelete_(upstream_connections_.at(2))); + EXPECT_CALL(filter_callbacks_.connection_, close(Network::ConnectionCloseType::NoFlush)); + } // Try both failure modes - raiseEventUpstreamConnectFailed(0, Tcp::ConnectionPool::PoolFailureReason::Timeout); - raiseEventUpstreamConnectFailed(1, - Tcp::ConnectionPool::PoolFailureReason::RemoteConnectionFailure); - raiseEventUpstreamConnectFailed(2, - Tcp::ConnectionPool::PoolFailureReason::RemoteConnectionFailure); + connect_timers_.at(0)->callback_(); + upstream_connections_.at(1)->raiseEvent(Network::ConnectionEvent::RemoteClose); + upstream_connections_.at(2)->raiseEvent(Network::ConnectionEvent::RemoteClose); + + EXPECT_EQ(1U, factory_context_.cluster_manager_.thread_local_cluster_.cluster_.info_->stats_store_ + .counter("upstream_cx_connect_timeout") + .value()); + EXPECT_EQ(2U, factory_context_.cluster_manager_.thread_local_cluster_.cluster_.info_->stats_store_ + .counter("upstream_cx_connect_fail") + .value()); + EXPECT_EQ(1U, factory_context_.cluster_manager_.thread_local_cluster_.cluster_.info_->stats_store_ + .counter("upstream_cx_connect_attempts_exceeded") + .value()); + EXPECT_EQ(0U, factory_context_.cluster_manager_.thread_local_cluster_.cluster_.info_->stats_store_ + .counter("upstream_cx_overflow") + .value()); + EXPECT_EQ(0U, factory_context_.cluster_manager_.thread_local_cluster_.cluster_.info_->stats_store_ + .counter("upstream_cx_no_successful_host") + .value()); } // Test that the tcp proxy sends the correct notifications to the outlier detector @@ -604,12 +608,11 @@ TEST_F(TcpProxyTest, OutlierDetection) { EXPECT_CALL(upstream_hosts_.at(0)->outlier_detector_, putResult(Upstream::Outlier::Result::TIMEOUT)); - raiseEventUpstreamConnectFailed(0, Tcp::ConnectionPool::PoolFailureReason::Timeout); + connect_timers_.at(0)->callback_(); EXPECT_CALL(upstream_hosts_.at(1)->outlier_detector_, putResult(Upstream::Outlier::Result::CONNECT_FAILED)); - raiseEventUpstreamConnectFailed(1, - Tcp::ConnectionPool::PoolFailureReason::RemoteConnectionFailure); + upstream_connections_.at(1)->raiseEvent(Network::ConnectionEvent::RemoteClose); EXPECT_CALL(upstream_hosts_.at(2)->outlier_detector_, putResult(Upstream::Outlier::Result::SUCCESS)); @@ -619,21 +622,21 @@ TEST_F(TcpProxyTest, OutlierDetection) { TEST_F(TcpProxyTest, UpstreamDisconnectDownstreamFlowControl) { setup(1); - raiseEventUpstreamConnected(0); - Buffer::OwnedImpl buffer("hello"); EXPECT_CALL(*upstream_connections_.at(0), write(BufferEqual(&buffer), _)); filter_->onData(buffer, false); + raiseEventUpstreamConnected(0); + Buffer::OwnedImpl response("world"); EXPECT_CALL(filter_callbacks_.connection_, write(BufferEqual(&response), _)); - upstream_callbacks_->onUpstreamData(response, false); + upstream_read_filter_->onData(response, false); EXPECT_CALL(*upstream_connections_.at(0), readDisable(true)); filter_callbacks_.connection_.runHighWatermarkCallbacks(); EXPECT_CALL(filter_callbacks_.connection_, close(Network::ConnectionCloseType::FlushWrite)); - upstream_callbacks_->onEvent(Network::ConnectionEvent::RemoteClose); + upstream_connections_.at(0)->raiseEvent(Network::ConnectionEvent::RemoteClose); filter_callbacks_.connection_.runLowWatermarkCallbacks(); } @@ -641,15 +644,15 @@ TEST_F(TcpProxyTest, UpstreamDisconnectDownstreamFlowControl) { TEST_F(TcpProxyTest, DownstreamDisconnectRemote) { setup(1); - raiseEventUpstreamConnected(0); - Buffer::OwnedImpl buffer("hello"); EXPECT_CALL(*upstream_connections_.at(0), write(BufferEqual(&buffer), _)); filter_->onData(buffer, false); + raiseEventUpstreamConnected(0); + Buffer::OwnedImpl response("world"); EXPECT_CALL(filter_callbacks_.connection_, write(BufferEqual(&response), _)); - upstream_callbacks_->onUpstreamData(response, false); + upstream_read_filter_->onData(response, false); EXPECT_CALL(*upstream_connections_.at(0), close(Network::ConnectionCloseType::FlushWrite)); filter_callbacks_.connection_.raiseEvent(Network::ConnectionEvent::RemoteClose); @@ -658,15 +661,15 @@ TEST_F(TcpProxyTest, DownstreamDisconnectRemote) { TEST_F(TcpProxyTest, DownstreamDisconnectLocal) { setup(1); - raiseEventUpstreamConnected(0); - Buffer::OwnedImpl buffer("hello"); EXPECT_CALL(*upstream_connections_.at(0), write(BufferEqual(&buffer), _)); filter_->onData(buffer, false); + raiseEventUpstreamConnected(0); + Buffer::OwnedImpl response("world"); EXPECT_CALL(filter_callbacks_.connection_, write(BufferEqual(&response), _)); - upstream_callbacks_->onUpstreamData(response, false); + upstream_read_filter_->onData(response, false); EXPECT_CALL(*upstream_connections_.at(0), close(Network::ConnectionCloseType::NoFlush)); filter_callbacks_.connection_.raiseEvent(Network::ConnectionEvent::LocalClose); @@ -675,8 +678,16 @@ TEST_F(TcpProxyTest, DownstreamDisconnectLocal) { TEST_F(TcpProxyTest, UpstreamConnectTimeout) { setup(1, accessLogConfig("%RESPONSE_FLAGS%")); + Buffer::OwnedImpl buffer("hello"); + EXPECT_CALL(*upstream_connections_.at(0), write(BufferEqual(&buffer), _)); + filter_->onData(buffer, false); + EXPECT_CALL(filter_callbacks_.connection_, close(Network::ConnectionCloseType::NoFlush)); - raiseEventUpstreamConnectFailed(0, Tcp::ConnectionPool::PoolFailureReason::Timeout); + EXPECT_CALL(*upstream_connections_.at(0), close(Network::ConnectionCloseType::NoFlush)); + connect_timers_.at(0)->callback_(); + EXPECT_EQ(1U, factory_context_.cluster_manager_.thread_local_cluster_.cluster_.info_->stats_store_ + .counter("upstream_cx_connect_timeout") + .value()); filter_.reset(); EXPECT_EQ(access_log_data_, "UF"); @@ -733,9 +744,15 @@ TEST_F(TcpProxyTest, DisconnectBeforeData) { TEST_F(TcpProxyTest, UpstreamConnectFailure) { setup(1, accessLogConfig("%RESPONSE_FLAGS%")); + Buffer::OwnedImpl buffer("hello"); + filter_->onData(buffer, false); + EXPECT_CALL(filter_callbacks_.connection_, close(Network::ConnectionCloseType::NoFlush)); - raiseEventUpstreamConnectFailed(0, - Tcp::ConnectionPool::PoolFailureReason::RemoteConnectionFailure); + EXPECT_CALL(*connect_timers_.at(0), disableTimer()); + upstream_connections_.at(0)->raiseEvent(Network::ConnectionEvent::RemoteClose); + EXPECT_EQ(1U, factory_context_.cluster_manager_.thread_local_cluster_.cluster_.info_->stats_store_ + .counter("upstream_cx_connect_fail") + .value()); filter_.reset(); EXPECT_EQ(access_log_data_, "UF"); @@ -753,6 +770,10 @@ TEST_F(TcpProxyTest, UpstreamConnectionLimit) { filter_->initializeReadFilterCallbacks(filter_callbacks_); filter_->onNewConnection(); + EXPECT_EQ(1U, factory_context_.cluster_manager_.thread_local_cluster_.cluster_.info_->stats_store_ + .counter("upstream_cx_overflow") + .value()); + filter_.reset(); EXPECT_EQ(access_log_data_, "UO"); } @@ -774,7 +795,7 @@ TEST_F(TcpProxyTest, IdleTimeout) { buffer.add("hello2"); EXPECT_CALL(*idle_timer, enableTimer(std::chrono::milliseconds(1000))); - upstream_callbacks_->onUpstreamData(buffer, false); + upstream_read_filter_->onData(buffer, false); EXPECT_CALL(*idle_timer, enableTimer(std::chrono::milliseconds(1000))); filter_callbacks_.connection_.raiseBytesSentCallbacks(1); @@ -813,13 +834,12 @@ TEST_F(TcpProxyTest, IdleTimerDisabledUpstreamClose) { raiseEventUpstreamConnected(0); EXPECT_CALL(*idle_timer, disableTimer()); - upstream_callbacks_->onEvent(Network::ConnectionEvent::RemoteClose); + upstream_connections_.at(0)->raiseEvent(Network::ConnectionEvent::RemoteClose); } // Test that access log fields %UPSTREAM_HOST% and %UPSTREAM_CLUSTER% are correctly logged. TEST_F(TcpProxyTest, AccessLogUpstreamHost) { setup(1, accessLogConfig("%UPSTREAM_HOST% %UPSTREAM_CLUSTER%")); - raiseEventUpstreamConnected(0); filter_.reset(); EXPECT_EQ(access_log_data_, "127.0.0.1:80 fake_cluster"); } @@ -827,7 +847,6 @@ TEST_F(TcpProxyTest, AccessLogUpstreamHost) { // Test that access log field %UPSTREAM_LOCAL_ADDRESS% is correctly logged. TEST_F(TcpProxyTest, AccessLogUpstreamLocalAddress) { setup(1, accessLogConfig("%UPSTREAM_LOCAL_ADDRESS%")); - raiseEventUpstreamConnected(0); filter_.reset(); EXPECT_EQ(access_log_data_, "2.2.2.2:50000"); } @@ -854,10 +873,10 @@ TEST_F(TcpProxyTest, AccessLogBytesRxTxDuration) { Buffer::OwnedImpl buffer("a"); filter_->onData(buffer, false); Buffer::OwnedImpl response("bb"); - upstream_callbacks_->onUpstreamData(response, false); + upstream_read_filter_->onData(response, false); std::this_thread::sleep_for(std::chrono::milliseconds(1)); - upstream_callbacks_->onEvent(Network::ConnectionEvent::RemoteClose); + upstream_connections_.at(0)->raiseEvent(Network::ConnectionEvent::RemoteClose); filter_.reset(); EXPECT_THAT(access_log_data_, @@ -870,8 +889,7 @@ TEST_F(TcpProxyTest, UpstreamFlushNoTimeout) { setup(1); raiseEventUpstreamConnected(0); - EXPECT_CALL(*upstream_connections_.at(0), - close(Network::ConnectionCloseType::FlushWrite)) + EXPECT_CALL(*upstream_connections_.at(0), close(Network::ConnectionCloseType::FlushWrite)) .WillOnce(Return()); // Cancel default action of raising LocalClose EXPECT_CALL(*upstream_connections_.at(0), state()) .WillOnce(Return(Network::Connection::State::Closing)); @@ -884,7 +902,7 @@ TEST_F(TcpProxyTest, UpstreamFlushNoTimeout) { upstream_connections_.at(0)->raiseBytesSentCallbacks(1); // Simulate flush complete. - upstream_callbacks_->onEvent(Network::ConnectionEvent::LocalClose); + upstream_connections_.at(0)->raiseEvent(Network::ConnectionEvent::LocalClose); EXPECT_EQ(1U, config_->stats().upstream_flush_total_.value()); EXPECT_EQ(0U, config_->stats().upstream_flush_active_.value()); } @@ -901,8 +919,7 @@ TEST_F(TcpProxyTest, UpstreamFlushTimeoutConfigured) { EXPECT_CALL(*idle_timer, enableTimer(_)); raiseEventUpstreamConnected(0); - EXPECT_CALL(*upstream_connections_.at(0), - close(Network::ConnectionCloseType::FlushWrite)) + EXPECT_CALL(*upstream_connections_.at(0), close(Network::ConnectionCloseType::FlushWrite)) .WillOnce(Return()); // Cancel default action of raising LocalClose EXPECT_CALL(*upstream_connections_.at(0), state()) .WillOnce(Return(Network::Connection::State::Closing)); @@ -916,7 +933,7 @@ TEST_F(TcpProxyTest, UpstreamFlushTimeoutConfigured) { // Simulate flush complete. EXPECT_CALL(*idle_timer, disableTimer()); - upstream_callbacks_->onEvent(Network::ConnectionEvent::LocalClose); + upstream_connections_.at(0)->raiseEvent(Network::ConnectionEvent::LocalClose); EXPECT_EQ(1U, config_->stats().upstream_flush_total_.value()); EXPECT_EQ(0U, config_->stats().upstream_flush_active_.value()); EXPECT_EQ(0U, config_->stats().idle_timeout_.value()); @@ -933,8 +950,7 @@ TEST_F(TcpProxyTest, UpstreamFlushTimeoutExpired) { EXPECT_CALL(*idle_timer, enableTimer(_)); raiseEventUpstreamConnected(0); - EXPECT_CALL(*upstream_connections_.at(0), - close(Network::ConnectionCloseType::FlushWrite)) + EXPECT_CALL(*upstream_connections_.at(0), close(Network::ConnectionCloseType::FlushWrite)) .WillOnce(Return()); // Cancel default action of raising LocalClose EXPECT_CALL(*upstream_connections_.at(0), state()) .WillOnce(Return(Network::Connection::State::Closing)); @@ -956,8 +972,7 @@ TEST_F(TcpProxyTest, UpstreamFlushReceiveUpstreamData) { setup(1); raiseEventUpstreamConnected(0); - EXPECT_CALL(*upstream_connections_.at(0), - close(Network::ConnectionCloseType::FlushWrite)) + EXPECT_CALL(*upstream_connections_.at(0), close(Network::ConnectionCloseType::FlushWrite)) .WillOnce(Return()); // Cancel default action of raising LocalClose EXPECT_CALL(*upstream_connections_.at(0), state()) .WillOnce(Return(Network::Connection::State::Closing)); @@ -969,7 +984,7 @@ TEST_F(TcpProxyTest, UpstreamFlushReceiveUpstreamData) { // Send some bytes; no timeout configured so this should be a no-op (not a crash). Buffer::OwnedImpl buffer("a"); EXPECT_CALL(*upstream_connections_.at(0), close(Network::ConnectionCloseType::NoFlush)); - upstream_callbacks_->onUpstreamData(buffer, false); + upstream_read_filter_->onData(buffer, false); } class TcpProxyRoutingTest : public testing::Test { @@ -1034,7 +1049,7 @@ TEST_F(TcpProxyRoutingTest, RoutableConnection) { connection_.local_address_ = std::make_shared("1.2.3.4", 9999); // Expect filter to try to open a connection to specified cluster. - EXPECT_CALL(factory_context_.cluster_manager_, tcpConnPoolForCluster("fake_cluster", _, _)); + EXPECT_CALL(factory_context_.cluster_manager_, tcpConnForCluster_("fake_cluster", _)); filter_->onNewConnection(); diff --git a/test/extensions/filters/network/thrift_proxy/router_test.cc b/test/extensions/filters/network/thrift_proxy/router_test.cc index 0fe593ed2cddd..94afe87c750d1 100644 --- a/test/extensions/filters/network/thrift_proxy/router_test.cc +++ b/test/extensions/filters/network/thrift_proxy/router_test.cc @@ -79,6 +79,9 @@ class ThriftRouterTestBase { route_ = new NiceMock(); route_ptr_.reset(route_); + host_ = new NiceMock(); + host_ptr_.reset(host_); + router_.reset(new Router(context_.clusterManager())); EXPECT_EQ(nullptr, router_->downstreamConnection()); @@ -95,8 +98,16 @@ class ThriftRouterTestBase { EXPECT_CALL(*route_, routeEntry()).WillOnce(Return(&route_entry_)); EXPECT_CALL(route_entry_, clusterName()).WillRepeatedly(ReturnRef(cluster_name_)); + EXPECT_CALL(context_.cluster_manager_.tcp_conn_pool_, newConnection(_)) + .WillOnce( + Invoke([&](Tcp::ConnectionPool::Callbacks& cb) -> Tcp::ConnectionPool::Cancellable* { + conn_pool_callbacks_ = &cb; + return &handle_; + })); + EXPECT_EQ(ThriftFilters::FilterStatus::StopIteration, router_->messageBegin(method_name_, msg_type_, seq_id_)); + EXPECT_NE(nullptr, conn_pool_callbacks_); NiceMock connection; EXPECT_CALL(callbacks_, connection()).WillRepeatedly(Return(&connection)); @@ -109,7 +120,7 @@ class ThriftRouterTestBase { } void connectUpstream() { - EXPECT_CALL(*context_.cluster_manager_.tcp_conn_pool_.connection_data_, addUpstreamCallbacks(_)) + EXPECT_CALL(conn_data_, addUpstreamCallbacks(_)) .WillOnce(Invoke([&](Tcp::ConnectionPool::UpstreamCallbacks& cb) -> void { upstream_callbacks_ = &cb; })); @@ -124,8 +135,7 @@ class ThriftRouterTestBase { EXPECT_CALL(*protocol_, writeMessageBegin(_, method_name_, msg_type_, seq_id_)); EXPECT_CALL(callbacks_, continueDecoding()); - - context_.cluster_manager_.tcp_conn_pool_.poolReady(upstream_connection_); + conn_pool_callbacks_->onPoolReady(conn_data_, host_ptr_); EXPECT_NE(nullptr, upstream_callbacks_); } @@ -184,10 +194,10 @@ class ThriftRouterTestBase { void completeRequest() { EXPECT_CALL(*protocol_, writeMessageEnd(_)); EXPECT_CALL(*transport_, encodeFrame(_, _)); - EXPECT_CALL(upstream_connection_, write(_, false)); + EXPECT_CALL(conn_data_.connection_, write(_, false)); if (msg_type_ == MessageType::Oneway) { - EXPECT_CALL(context_.cluster_manager_.tcp_conn_pool_, released(Ref(upstream_connection_))); + EXPECT_CALL(conn_data_, release()); } EXPECT_EQ(ThriftFilters::FilterStatus::Continue, router_->messageEnd()); @@ -203,7 +213,7 @@ class ThriftRouterTestBase { upstream_callbacks_->onUpstreamData(buffer, false); EXPECT_CALL(callbacks_, upstreamData(Ref(buffer))).WillOnce(Return(true)); - EXPECT_CALL(context_.cluster_manager_.tcp_conn_pool_, released(Ref(upstream_connection_))); + EXPECT_CALL(conn_data_, release()); upstream_callbacks_->onUpstreamData(buffer, false); } @@ -226,6 +236,8 @@ class ThriftRouterTestBase { NiceMock* host_{}; RouteConstSharedPtr route_ptr_; + Upstream::HostDescriptionConstSharedPtr host_ptr_; + std::unique_ptr router_; std::string cluster_name_{"cluster"}; @@ -234,8 +246,10 @@ class ThriftRouterTestBase { MessageType msg_type_{MessageType::Call}; int32_t seq_id_{1}; + NiceMock handle_; + NiceMock conn_data_; + Tcp::ConnectionPool::Callbacks* conn_pool_callbacks_{}; Tcp::ConnectionPool::UpstreamCallbacks* upstream_callbacks_{}; - NiceMock upstream_connection_; }; class ThriftRouterTest : public ThriftRouterTestBase, public Test { @@ -276,8 +290,8 @@ TEST_F(ThriftRouterTest, PoolRemoteConnectionFailure) { EXPECT_EQ(AppExceptionType::InternalError, app_ex->type_); EXPECT_THAT(app_ex->error_message_, ContainsRegex(".*connection failure.*")); })); - context_.cluster_manager_.tcp_conn_pool_.poolFailure( - Tcp::ConnectionPool::PoolFailureReason::RemoteConnectionFailure); + conn_pool_callbacks_->onPoolFailure( + Tcp::ConnectionPool::PoolFailureReason::RemoteConnectionFailure, host_ptr_); } TEST_F(ThriftRouterTest, PoolLocalConnectionFailure) { @@ -294,8 +308,8 @@ TEST_F(ThriftRouterTest, PoolLocalConnectionFailure) { EXPECT_EQ(AppExceptionType::InternalError, app_ex->type_); EXPECT_THAT(app_ex->error_message_, ContainsRegex(".*connection failure.*")); })); - context_.cluster_manager_.tcp_conn_pool_.poolFailure( - Tcp::ConnectionPool::PoolFailureReason::LocalConnectionFailure); + conn_pool_callbacks_->onPoolFailure( + Tcp::ConnectionPool::PoolFailureReason::LocalConnectionFailure, host_ptr_); } TEST_F(ThriftRouterTest, PoolTimeout) { @@ -312,8 +326,7 @@ TEST_F(ThriftRouterTest, PoolTimeout) { EXPECT_EQ(AppExceptionType::InternalError, app_ex->type_); EXPECT_THAT(app_ex->error_message_, ContainsRegex(".*connection failure.*")); })); - context_.cluster_manager_.tcp_conn_pool_.poolFailure( - Tcp::ConnectionPool::PoolFailureReason::Timeout); + conn_pool_callbacks_->onPoolFailure(Tcp::ConnectionPool::PoolFailureReason::Timeout, host_ptr_); } TEST_F(ThriftRouterTest, PoolOverflowFailure) { @@ -330,8 +343,7 @@ TEST_F(ThriftRouterTest, PoolOverflowFailure) { EXPECT_EQ(AppExceptionType::InternalError, app_ex->type_); EXPECT_THAT(app_ex->error_message_, ContainsRegex(".*too many connections.*")); })); - context_.cluster_manager_.tcp_conn_pool_.poolFailure( - Tcp::ConnectionPool::PoolFailureReason::Overflow); + conn_pool_callbacks_->onPoolFailure(Tcp::ConnectionPool::PoolFailureReason::Overflow, host_ptr_); } TEST_F(ThriftRouterTest, NoRoute) { @@ -428,7 +440,7 @@ TEST_F(ThriftRouterTest, TruncatedResponse) { EXPECT_CALL(callbacks_, startUpstreamResponse(TransportType::Framed, ProtocolType::Binary)); EXPECT_CALL(callbacks_, upstreamData(Ref(buffer))).WillOnce(Return(false)); - EXPECT_CALL(context_.cluster_manager_.tcp_conn_pool_, released(Ref(upstream_connection_))); + EXPECT_CALL(conn_data_, release()); EXPECT_CALL(callbacks_, resetDownstreamConnection()); upstream_callbacks_->onUpstreamData(buffer, true); @@ -450,7 +462,7 @@ TEST_F(ThriftRouterTest, UpstreamDataTriggersReset) { router_->resetUpstreamConnection(); return true; })); - EXPECT_CALL(upstream_connection_, close(Network::ConnectionCloseType::NoFlush)); + EXPECT_CALL(conn_data_.connection_, close(Network::ConnectionCloseType::NoFlush)); upstream_callbacks_->onUpstreamData(buffer, true); destroyRouter(); @@ -466,7 +478,7 @@ TEST_F(ThriftRouterTest, UnexpectedRouterDestroy) { initializeRouter(); startRequest(MessageType::Call); connectUpstream(); - EXPECT_CALL(upstream_connection_, close(Network::ConnectionCloseType::NoFlush)); + EXPECT_CALL(conn_data_.connection_, close(Network::ConnectionCloseType::NoFlush)); destroyRouter(); } diff --git a/test/integration/tcp_conn_pool_integration_test.cc b/test/integration/tcp_conn_pool_integration_test.cc index fbe93d877c9a4..c13d836e8d2e6 100644 --- a/test/integration/tcp_conn_pool_integration_test.cc +++ b/test/integration/tcp_conn_pool_integration_test.cc @@ -52,9 +52,9 @@ class TestFilter : public Network::ReadFilter { ASSERT(false); } - void onPoolReady(Tcp::ConnectionPool::ConnectionDataPtr&& conn, + void onPoolReady(Tcp::ConnectionPool::ConnectionData& conn, Upstream::HostDescriptionConstSharedPtr) override { - upstream_ = std::move(conn); + upstream_ = &conn; upstream_->addUpstreamCallbacks(*this); upstream_->connection().write(data_, false); @@ -67,7 +67,7 @@ class TestFilter : public Network::ReadFilter { Network::Connection& downstream = parent_.read_callbacks_->connection(); downstream.write(data, false); - upstream_.reset(); + upstream_->release(); } void onEvent(Network::ConnectionEvent) override {} void onAboveWriteBufferHighWatermark() override {} @@ -75,7 +75,7 @@ class TestFilter : public Network::ReadFilter { TestFilter& parent_; Buffer::OwnedImpl data_; - Tcp::ConnectionPool::ConnectionDataPtr upstream_; + Tcp::ConnectionPool::ConnectionData* upstream_; }; Upstream::ClusterManager& cluster_manager_; diff --git a/test/mocks/tcp/mocks.cc b/test/mocks/tcp/mocks.cc index 1374d415dea7f..757f7556f32b1 100644 --- a/test/mocks/tcp/mocks.cc +++ b/test/mocks/tcp/mocks.cc @@ -18,12 +18,10 @@ MockCancellable::~MockCancellable() {} MockUpstreamCallbacks::MockUpstreamCallbacks() {} MockUpstreamCallbacks::~MockUpstreamCallbacks() {} -MockConnectionData::MockConnectionData() {} -MockConnectionData::~MockConnectionData() { - if (release_callback_) { - release_callback_(); - } +MockConnectionData::MockConnectionData() { + ON_CALL(*this, connection()).WillByDefault(ReturnRef(connection_)); } +MockConnectionData::~MockConnectionData() {} MockInstance::MockInstance() { ON_CALL(*this, newConnection(_)).WillByDefault(Invoke([&](Callbacks& cb) -> Cancellable* { @@ -46,16 +44,12 @@ void MockInstance::poolFailure(PoolFailureReason reason) { cb->onPoolFailure(reason, host_); } -void MockInstance::poolReady(Network::MockClientConnection& conn) { +void MockInstance::poolReady() { Callbacks* cb = callbacks_.front(); callbacks_.pop_front(); handles_.pop_front(); - ON_CALL(*connection_data_, connection()).WillByDefault(ReturnRef(conn)); - - connection_data_->release_callback_ = [&]() -> void { released(conn); }; - - cb->onPoolReady(std::move(connection_data_), host_); + cb->onPoolReady(connection_data_, host_); } } // namespace ConnectionPool diff --git a/test/mocks/tcp/mocks.h b/test/mocks/tcp/mocks.h index 3fb969f088b2d..a9cc52fecbbb9 100644 --- a/test/mocks/tcp/mocks.h +++ b/test/mocks/tcp/mocks.h @@ -44,10 +44,9 @@ class MockConnectionData : public ConnectionData { // Tcp::ConnectionPool::ConnectionData MOCK_METHOD0(connection, Network::ClientConnection&()); MOCK_METHOD1(addUpstreamCallbacks, void(ConnectionPool::UpstreamCallbacks&)); + MOCK_METHOD0(release, void()); - // If set, invoked in ~MockConnectionData, which indicates that the connection pool - // caller has relased a connection. - std::function release_callback_; + NiceMock connection_; }; class MockInstance : public Instance { @@ -62,18 +61,14 @@ class MockInstance : public Instance { MockCancellable* newConnectionImpl(Callbacks& cb); void poolFailure(PoolFailureReason reason); - void poolReady(Network::MockClientConnection& conn); - - // Invoked when connection_data_, having been assigned via poolReady is released. - MOCK_METHOD1(released, void(Network::MockClientConnection&)); + void poolReady(); std::list> handles_; std::list callbacks_; std::shared_ptr> host_{ new NiceMock()}; - std::unique_ptr> connection_data_{ - new NiceMock()}; + NiceMock connection_data_; }; } // namespace ConnectionPool From f2c9652a92b90fe69293478b04ae1c405417ab63 Mon Sep 17 00:00:00 2001 From: Matt Klein Date: Fri, 3 Aug 2018 12:00:49 -0700 Subject: [PATCH 11/11] owners: add Dhi is maintainer (#4042) Yay! :) Signed-off-by: Matt Klein --- OWNERS.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/OWNERS.md b/OWNERS.md index cf7e947c23fe2..2feaae1f742e1 100644 --- a/OWNERS.md +++ b/OWNERS.md @@ -32,6 +32,8 @@ routing PRs, questions, etc. to the right place. * Redis, Python, configuration/operational questions. * Lizan Zhou ([lizan](https://github.com/lizan)) (zlizan@google.com) * gRPC, gRPC/JSON transcoding, and core networking (transport socket abstractions). +* Dhi Aurrahman ([dio](https://github.com/dio)) (dio@tetrate.io) + * Lua, access logging, and general miscellany. # Emeritus maintainers