From dbf253afb4f96b6470d11d89aa0fae0c2f004c2b Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Thu, 9 Sep 2021 15:21:08 -0400 Subject: [PATCH 1/4] WIP Signed-off-by: Alyssa Wilk --- envoy/server/factory_context.h | 70 ++++++++++--------- envoy/upstream/cluster_factory.h | 51 ++------------ .../clusters/dynamic_forward_proxy/cluster.cc | 3 +- .../dns_cache_manager_impl.cc | 13 ++-- .../dns_cache_manager_impl.h | 34 ++------- .../http/dynamic_forward_proxy/config.cc | 3 +- .../sni_dynamic_forward_proxy/config.cc | 3 +- 7 files changed, 56 insertions(+), 121 deletions(-) diff --git a/envoy/server/factory_context.h b/envoy/server/factory_context.h index 03cbfe9c23ff4..d9f34354508c0 100644 --- a/envoy/server/factory_context.h +++ b/envoy/server/factory_context.h @@ -38,17 +38,15 @@ namespace Envoy { namespace Server { namespace Configuration { -/** - * Common interface for downstream and upstream network filters. - */ -class CommonFactoryContext { +// Shared factory context between server factories and cluster factories +class FactoryContextBase { public: - virtual ~CommonFactoryContext() = default; + virtual ~FactoryContextBase() = default; /** - * @return Upstream::ClusterManager& singleton for use by the entire server. + * @return Server::Options& the command-line options that Envoy was started with. */ - virtual Upstream::ClusterManager& clusterManager() PURE; + virtual const Options& options() PURE; /** * @return Event::Dispatcher& the main thread's dispatcher. This dispatcher should be used @@ -57,30 +55,29 @@ class CommonFactoryContext { virtual Event::Dispatcher& dispatcher() PURE; /** - * @return Server::Options& the command-line options that Envoy was started with. + * @return Api::Api& a reference to the api object. */ - virtual const Options& options() PURE; + virtual Api::Api& api() PURE; /** - * @return information about the local environment the server is running in. + * @return Upstream::ClusterManager& singleton for use by the entire server. */ - virtual const LocalInfo::LocalInfo& localInfo() const PURE; + virtual Upstream::ClusterManager& clusterManager() PURE; /** - * @return ProtobufMessage::ValidationContext& validation visitor for xDS and static configuration - * messages. + * @return information about the local environment the server is running in. */ - virtual ProtobufMessage::ValidationContext& messageValidationContext() PURE; + virtual const LocalInfo::LocalInfo& localInfo() PURE; /** - * @return Runtime::Loader& the singleton runtime loader for the server. + * @return Server::Admin& the server's global admin HTTP endpoint. */ - virtual Envoy::Runtime::Loader& runtime() PURE; + virtual Server::Admin& admin() PURE; /** - * @return Stats::Scope& the filter's stats scope. + * @return Runtime::Loader& the singleton runtime loader for the server. */ - virtual Stats::Scope& scope() PURE; + virtual Envoy::Runtime::Loader& runtime() PURE; /** * @return Singleton::Manager& the server-wide singleton manager. @@ -88,36 +85,45 @@ class CommonFactoryContext { virtual Singleton::Manager& singletonManager() PURE; /** - * @return ThreadLocal::SlotAllocator& the thread local storage engine for the server. This is - * used to allow runtime lockless updates to configuration, etc. across multiple threads. + * @return ProtobufMessage::ValidationVisitor& validation visitor for filter configuration + * messages. */ - virtual ThreadLocal::SlotAllocator& threadLocal() PURE; + virtual ProtobufMessage::ValidationVisitor& messageValidationVisitor() PURE; /** - * @return Server::Admin& the server's global admin HTTP endpoint. + * @return Stats::Scope& the context's stats scope. */ - virtual Server::Admin& admin() PURE; + virtual Stats::Scope& scope() PURE; +}; + +/** + * Common interface for downstream and upstream network filters. + */ +class CommonFactoryContext : public FactoryContextBase { +public: + virtual ~CommonFactoryContext() = default; /** - * @return TimeSource& a reference to the time source. + * @return ProtobufMessage::ValidationContext& validation visitor for xDS and static configuration + * messages. */ - virtual TimeSource& timeSource() PURE; + virtual ProtobufMessage::ValidationContext& messageValidationContext() PURE; /** - * @return Api::Api& a reference to the api object. + * @return ThreadLocal::SlotAllocator& the thread local storage engine for the server. This is + * used to allow runtime lockless updates to configuration, etc. across multiple threads. */ - virtual Api::Api& api() PURE; + virtual ThreadLocal::SlotAllocator& threadLocal() PURE; /** - * @return AccessLogManager for use by the entire server. + * @return TimeSource& a reference to the time source. */ - virtual AccessLog::AccessLogManager& accessLogManager() PURE; + virtual TimeSource& timeSource() PURE; /** - * @return ProtobufMessage::ValidationVisitor& validation visitor for filter configuration - * messages. + * @return AccessLogManager for use by the entire server. */ - virtual ProtobufMessage::ValidationVisitor& messageValidationVisitor() PURE; + virtual AccessLog::AccessLogManager& accessLogManager() PURE; /** * @return ServerLifecycleNotifier& the lifecycle notifier for the server. diff --git a/envoy/upstream/cluster_factory.h b/envoy/upstream/cluster_factory.h index 6196cef791576..bb232eecd9743 100644 --- a/envoy/upstream/cluster_factory.h +++ b/envoy/upstream/cluster_factory.h @@ -18,6 +18,7 @@ #include "envoy/network/dns.h" #include "envoy/runtime/runtime.h" #include "envoy/server/admin.h" +#include "envoy/server/factory_context.h" #include "envoy/server/options.h" #include "envoy/singleton/manager.h" #include "envoy/ssl/context.h" @@ -35,7 +36,7 @@ namespace Upstream { * Context passed to cluster factory to access envoy resources. Cluster factory should only access * the rest of the server through this context object. */ -class ClusterFactoryContext { +class ClusterFactoryContext : public Server::Configuration::FactoryContextBase { public: virtual ~ClusterFactoryContext() = default; @@ -44,57 +45,16 @@ class ClusterFactoryContext { */ virtual bool addedViaApi() PURE; - /** - * @return Server::Admin& the server's admin interface. - */ - virtual Server::Admin& admin() PURE; - - /** - * @return Api::Api& a reference to the api object. - */ - virtual Api::Api& api() PURE; - - /** - * @return Upstream::ClusterManager& singleton for use by the entire server. - */ - virtual ClusterManager& clusterManager() PURE; - - /** - * @return Event::Dispatcher& the main thread's dispatcher. This dispatcher should be used - * for all singleton processing. - */ - virtual Event::Dispatcher& dispatcher() PURE; - /** * @return Network::DnsResolverSharedPtr the dns resolver for the server. */ virtual Network::DnsResolverSharedPtr dnsResolver() PURE; - /** - * @return information about the local environment the server is running in. - */ - virtual const LocalInfo::LocalInfo& localInfo() PURE; - - /** - * @return Server::Options& the command-line options that Envoy was started with. - */ - virtual const Server::Options& options() PURE; - /** * @return AccessLogManager for use by the entire server. */ virtual AccessLog::AccessLogManager& logManager() PURE; - /** - * @return Runtime::Loader& the singleton runtime loader for the server. - */ - virtual Runtime::Loader& runtime() PURE; - - /** - * @return Singleton::Manager& the server-wide singleton manager. - */ - virtual Singleton::Manager& singletonManager() PURE; - /** * @return Ssl::ContextManager& the SSL context manager. */ @@ -115,11 +75,8 @@ class ClusterFactoryContext { */ virtual Outlier::EventLoggerSharedPtr outlierEventLogger() PURE; - /** - * @return ProtobufMessage::ValidationVisitor& validation visitor for filter configuration - * messages. - */ - virtual ProtobufMessage::ValidationVisitor& messageValidationVisitor() PURE; + // FactoryContextBase + virtual Stats::Scope& scope() override { return stats(); } }; /** diff --git a/source/extensions/clusters/dynamic_forward_proxy/cluster.cc b/source/extensions/clusters/dynamic_forward_proxy/cluster.cc index cc72fc392466a..2bef1a7f333b8 100644 --- a/source/extensions/clusters/dynamic_forward_proxy/cluster.cc +++ b/source/extensions/clusters/dynamic_forward_proxy/cluster.cc @@ -177,8 +177,7 @@ ClusterFactory::createClusterWithConfig( Server::Configuration::TransportSocketFactoryContextImpl& socket_factory_context, Stats::ScopePtr&& stats_scope) { Extensions::Common::DynamicForwardProxy::DnsCacheManagerFactoryImpl cache_manager_factory( - context.singletonManager(), context.dispatcher(), context.tls(), context.api(), - context.runtime(), context.stats(), context.messageValidationVisitor()); + context); envoy::config::cluster::v3::Cluster cluster_config = cluster; if (!cluster_config.has_upstream_http_protocol_options()) { // This sets defaults which will only apply if using old style http config. diff --git a/source/extensions/common/dynamic_forward_proxy/dns_cache_manager_impl.cc b/source/extensions/common/dynamic_forward_proxy/dns_cache_manager_impl.cc index 7cb28f80e68de..7239ab61fd20a 100644 --- a/source/extensions/common/dynamic_forward_proxy/dns_cache_manager_impl.cc +++ b/source/extensions/common/dynamic_forward_proxy/dns_cache_manager_impl.cc @@ -26,19 +26,18 @@ DnsCacheSharedPtr DnsCacheManagerImpl::getCache( return existing_cache->second.cache_; } - DnsCacheSharedPtr new_cache = - std::make_shared(main_thread_dispatcher_, tls_, random_, file_system_, loader_, - root_scope_, validation_visitor_, config); + DnsCacheSharedPtr new_cache = std::make_shared( + context_.dispatcher(), context_.tls(), context_.api().randomGenerator(), + context.api().fileSystem(), context_.runtime(), context.scope(), context_.validationVisitor(), + config); caches_.emplace(config.name(), ActiveCache{config, new_cache}); return new_cache; } DnsCacheManagerSharedPtr DnsCacheManagerFactoryImpl::get() { return singleton_manager_.getTyped( - SINGLETON_MANAGER_REGISTERED_NAME(dns_cache_manager), [this] { - return std::make_shared(dispatcher_, tls_, random_, file_system_, - loader_, root_scope_, validation_visitor_); - }); + SINGLETON_MANAGER_REGISTERED_NAME(dns_cache_manager), + [this] { return std::make_shared(context_); }); } } // namespace DynamicForwardProxy diff --git a/source/extensions/common/dynamic_forward_proxy/dns_cache_manager_impl.h b/source/extensions/common/dynamic_forward_proxy/dns_cache_manager_impl.h index 4b27404366a8c..adb1e92717eef 100644 --- a/source/extensions/common/dynamic_forward_proxy/dns_cache_manager_impl.h +++ b/source/extensions/common/dynamic_forward_proxy/dns_cache_manager_impl.h @@ -13,13 +13,7 @@ namespace DynamicForwardProxy { class DnsCacheManagerImpl : public DnsCacheManager, public Singleton::Instance { public: - DnsCacheManagerImpl(Event::Dispatcher& main_thread_dispatcher, ThreadLocal::SlotAllocator& tls, - Random::RandomGenerator& random, Filesystem::Instance& file_system, - Runtime::Loader& loader, Stats::Scope& root_scope, - ProtobufMessage::ValidationVisitor& validation_visitor) - : main_thread_dispatcher_(main_thread_dispatcher), tls_(tls), random_(random), - file_system_(file_system), loader_(loader), root_scope_(root_scope), - validation_visitor_(validation_visitor) {} + DnsCacheManagerImpl(Server::Configuration::FactoryContextBase& context) : context_(context) {} // DnsCacheManager DnsCacheSharedPtr getCache( @@ -35,38 +29,20 @@ class DnsCacheManagerImpl : public DnsCacheManager, public Singleton::Instance { DnsCacheSharedPtr cache_; }; - Event::Dispatcher& main_thread_dispatcher_; - ThreadLocal::SlotAllocator& tls_; - Random::RandomGenerator& random_; - Filesystem::Instance& file_system_; - Runtime::Loader& loader_; - Stats::Scope& root_scope_; - ProtobufMessage::ValidationVisitor& validation_visitor_; + Server::Configuration::FactoryContextBase& context_; absl::flat_hash_map caches_; }; class DnsCacheManagerFactoryImpl : public DnsCacheManagerFactory { public: - DnsCacheManagerFactoryImpl(Singleton::Manager& singleton_manager, Event::Dispatcher& dispatcher, - ThreadLocal::SlotAllocator& tls, Api::Api& api, - Runtime::Loader& loader, Stats::Scope& root_scope, - ProtobufMessage::ValidationVisitor& validation_visitor) - : singleton_manager_(singleton_manager), dispatcher_(dispatcher), tls_(tls), - random_(api.randomGenerator()), file_system_(api.fileSystem()), loader_(loader), - root_scope_(root_scope), validation_visitor_(validation_visitor) {} + DnsCacheManagerFactoryImpl(Server::Configuration::FactoryContextBase context) + : context_(context) {} DnsCacheManagerSharedPtr get() override; private: - Singleton::Manager& singleton_manager_; - Event::Dispatcher& dispatcher_; - ThreadLocal::SlotAllocator& tls_; - Random::RandomGenerator& random_; - Filesystem::Instance& file_system_; - Runtime::Loader& loader_; - Stats::Scope& root_scope_; - ProtobufMessage::ValidationVisitor& validation_visitor_; + Server::Configuration::FactoryContextBase& context_; }; } // namespace DynamicForwardProxy diff --git a/source/extensions/filters/http/dynamic_forward_proxy/config.cc b/source/extensions/filters/http/dynamic_forward_proxy/config.cc index 3f58fa19ca06a..75b69d19876d5 100644 --- a/source/extensions/filters/http/dynamic_forward_proxy/config.cc +++ b/source/extensions/filters/http/dynamic_forward_proxy/config.cc @@ -15,8 +15,7 @@ Http::FilterFactoryCb DynamicForwardProxyFilterFactory::createFilterFactoryFromP const envoy::extensions::filters::http::dynamic_forward_proxy::v3::FilterConfig& proto_config, const std::string&, Server::Configuration::FactoryContext& context) { Extensions::Common::DynamicForwardProxy::DnsCacheManagerFactoryImpl cache_manager_factory( - context.singletonManager(), context.dispatcher(), context.threadLocal(), context.api(), - context.runtime(), context.scope(), context.messageValidationVisitor()); + context); ProxyFilterConfigSharedPtr filter_config(std::make_shared( proto_config, cache_manager_factory, context.clusterManager())); return [filter_config](Http::FilterChainFactoryCallbacks& callbacks) -> void { diff --git a/source/extensions/filters/network/sni_dynamic_forward_proxy/config.cc b/source/extensions/filters/network/sni_dynamic_forward_proxy/config.cc index dedff7689748e..14c2396cdf459 100644 --- a/source/extensions/filters/network/sni_dynamic_forward_proxy/config.cc +++ b/source/extensions/filters/network/sni_dynamic_forward_proxy/config.cc @@ -19,8 +19,7 @@ SniDynamicForwardProxyNetworkFilterConfigFactory::createFilterFactoryFromProtoTy const FilterConfig& proto_config, Server::Configuration::FactoryContext& context) { Extensions::Common::DynamicForwardProxy::DnsCacheManagerFactoryImpl cache_manager_factory( - context.singletonManager(), context.dispatcher(), context.threadLocal(), context.api(), - context.runtime(), context.scope(), context.messageValidationVisitor()); + context); ProxyFilterConfigSharedPtr filter_config(std::make_shared( proto_config, cache_manager_factory, context.clusterManager())); From 18884a125b64988f6c30a60658bb70bfbc409ba2 Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Fri, 10 Sep 2021 10:37:14 -0400 Subject: [PATCH 2/4] factory: refactoring overlapping APIs to allow for passing fewer arguments Signed-off-by: Alyssa Wilk --- envoy/server/factory_context.h | 17 ++++++++--------- envoy/upstream/cluster_factory.h | 7 ------- source/common/upstream/cluster_factory_impl.cc | 2 +- source/common/upstream/cluster_factory_impl.h | 4 ++-- .../dns_cache_manager_impl.cc | 8 ++++---- .../dns_cache_manager_impl.h | 3 ++- 6 files changed, 17 insertions(+), 24 deletions(-) diff --git a/envoy/server/factory_context.h b/envoy/server/factory_context.h index d9f34354508c0..351f5904fef31 100644 --- a/envoy/server/factory_context.h +++ b/envoy/server/factory_context.h @@ -67,7 +67,7 @@ class FactoryContextBase { /** * @return information about the local environment the server is running in. */ - virtual const LocalInfo::LocalInfo& localInfo() PURE; + virtual const LocalInfo::LocalInfo& localInfo() const PURE; /** * @return Server::Admin& the server's global admin HTTP endpoint. @@ -85,8 +85,7 @@ class FactoryContextBase { virtual Singleton::Manager& singletonManager() PURE; /** - * @return ProtobufMessage::ValidationVisitor& validation visitor for filter configuration - * messages. + * @return ProtobufMessage::ValidationVisitor& validation visitor for configuration messages. */ virtual ProtobufMessage::ValidationVisitor& messageValidationVisitor() PURE; @@ -94,6 +93,12 @@ class FactoryContextBase { * @return Stats::Scope& the context's stats scope. */ virtual Stats::Scope& scope() PURE; + + /** + * @return ThreadLocal::SlotAllocator& the thread local storage engine for the server. This is + * used to allow runtime lockless updates to configuration, etc. across multiple threads. + */ + virtual ThreadLocal::SlotAllocator& threadLocal() PURE; }; /** @@ -109,12 +114,6 @@ class CommonFactoryContext : public FactoryContextBase { */ virtual ProtobufMessage::ValidationContext& messageValidationContext() PURE; - /** - * @return ThreadLocal::SlotAllocator& the thread local storage engine for the server. This is - * used to allow runtime lockless updates to configuration, etc. across multiple threads. - */ - virtual ThreadLocal::SlotAllocator& threadLocal() PURE; - /** * @return TimeSource& a reference to the time source. */ diff --git a/envoy/upstream/cluster_factory.h b/envoy/upstream/cluster_factory.h index bb232eecd9743..9d7dd9ff6bb63 100644 --- a/envoy/upstream/cluster_factory.h +++ b/envoy/upstream/cluster_factory.h @@ -65,17 +65,10 @@ class ClusterFactoryContext : public Server::Configuration::FactoryContextBase { */ virtual Stats::Store& stats() PURE; - /** - * @return the server's TLS slot allocator. - */ - virtual ThreadLocal::SlotAllocator& tls() PURE; - /** * @return Outlier::EventLoggerSharedPtr sink for outlier detection event logs. */ virtual Outlier::EventLoggerSharedPtr outlierEventLogger() PURE; - - // FactoryContextBase virtual Stats::Scope& scope() override { return stats(); } }; diff --git a/source/common/upstream/cluster_factory_impl.cc b/source/common/upstream/cluster_factory_impl.cc index 2e90917e28b56..7425d9e546fb6 100644 --- a/source/common/upstream/cluster_factory_impl.cc +++ b/source/common/upstream/cluster_factory_impl.cc @@ -128,7 +128,7 @@ ClusterFactoryImplBase::create(const envoy::config::cluster::v3::Cluster& cluste std::make_unique( context.admin(), context.sslContextManager(), *stats_scope, context.clusterManager(), context.localInfo(), context.dispatcher(), context.stats(), - context.singletonManager(), context.tls(), context.messageValidationVisitor(), + context.singletonManager(), context.threadLocal(), context.messageValidationVisitor(), context.api(), context.options()); std::pair new_cluster_pair = diff --git a/source/common/upstream/cluster_factory_impl.h b/source/common/upstream/cluster_factory_impl.h index a1a2020cc834e..1d7f3a6bc94cb 100644 --- a/source/common/upstream/cluster_factory_impl.h +++ b/source/common/upstream/cluster_factory_impl.h @@ -70,13 +70,13 @@ class ClusterFactoryContextImpl : public ClusterFactoryContext { ClusterManager& clusterManager() override { return cluster_manager_; } Stats::Store& stats() override { return stats_; } - ThreadLocal::SlotAllocator& tls() override { return tls_; } + ThreadLocal::SlotAllocator& threadLocal() override { return tls_; } Network::DnsResolverSharedPtr dnsResolver() override { return dns_resolver_; } Ssl::ContextManager& sslContextManager() override { return ssl_context_manager_; } Runtime::Loader& runtime() override { return runtime_; } Event::Dispatcher& dispatcher() override { return dispatcher_; } AccessLog::AccessLogManager& logManager() override { return log_manager_; } - const LocalInfo::LocalInfo& localInfo() override { return local_info_; } + const LocalInfo::LocalInfo& localInfo() const override { return local_info_; } const Server::Options& options() override { return options_; } Server::Admin& admin() override { return admin_; } Singleton::Manager& singletonManager() override { return singleton_manager_; } diff --git a/source/extensions/common/dynamic_forward_proxy/dns_cache_manager_impl.cc b/source/extensions/common/dynamic_forward_proxy/dns_cache_manager_impl.cc index 7239ab61fd20a..5eecc3d17cc32 100644 --- a/source/extensions/common/dynamic_forward_proxy/dns_cache_manager_impl.cc +++ b/source/extensions/common/dynamic_forward_proxy/dns_cache_manager_impl.cc @@ -27,15 +27,15 @@ DnsCacheSharedPtr DnsCacheManagerImpl::getCache( } DnsCacheSharedPtr new_cache = std::make_shared( - context_.dispatcher(), context_.tls(), context_.api().randomGenerator(), - context.api().fileSystem(), context_.runtime(), context.scope(), context_.validationVisitor(), - config); + context_.dispatcher(), context_.threadLocal(), context_.api().randomGenerator(), + context_.api().fileSystem(), context_.runtime(), context_.scope(), + context_.messageValidationVisitor(), config); caches_.emplace(config.name(), ActiveCache{config, new_cache}); return new_cache; } DnsCacheManagerSharedPtr DnsCacheManagerFactoryImpl::get() { - return singleton_manager_.getTyped( + return context_.singletonManager().getTyped( SINGLETON_MANAGER_REGISTERED_NAME(dns_cache_manager), [this] { return std::make_shared(context_); }); } diff --git a/source/extensions/common/dynamic_forward_proxy/dns_cache_manager_impl.h b/source/extensions/common/dynamic_forward_proxy/dns_cache_manager_impl.h index adb1e92717eef..657279c2323ce 100644 --- a/source/extensions/common/dynamic_forward_proxy/dns_cache_manager_impl.h +++ b/source/extensions/common/dynamic_forward_proxy/dns_cache_manager_impl.h @@ -1,6 +1,7 @@ #pragma once #include "envoy/extensions/common/dynamic_forward_proxy/v3/dns_cache.pb.h" +#include "envoy/server/factory_context.h" #include "source/extensions/common/dynamic_forward_proxy/dns_cache.h" @@ -36,7 +37,7 @@ class DnsCacheManagerImpl : public DnsCacheManager, public Singleton::Instance { class DnsCacheManagerFactoryImpl : public DnsCacheManagerFactory { public: - DnsCacheManagerFactoryImpl(Server::Configuration::FactoryContextBase context) + DnsCacheManagerFactoryImpl(Server::Configuration::FactoryContextBase& context) : context_(context) {} DnsCacheManagerSharedPtr get() override; From 730b57cd5302b287bb471c0ccffcf014cb3cfca7 Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Mon, 13 Sep 2021 10:28:54 -0400 Subject: [PATCH 3/4] more cleanup Signed-off-by: Alyssa Wilk --- .../dynamic_forward_proxy/dns_cache_impl.cc | 19 ++- .../dynamic_forward_proxy/dns_cache_impl.h | 6 +- .../dns_cache_manager_impl.cc | 5 +- .../dns_cache_impl_test.cc | 155 ++++++++---------- 4 files changed, 78 insertions(+), 107 deletions(-) diff --git a/source/extensions/common/dynamic_forward_proxy/dns_cache_impl.cc b/source/extensions/common/dynamic_forward_proxy/dns_cache_impl.cc index 7a208b9f1398d..3d2538dc495af 100644 --- a/source/extensions/common/dynamic_forward_proxy/dns_cache_impl.cc +++ b/source/extensions/common/dynamic_forward_proxy/dns_cache_impl.cc @@ -16,23 +16,24 @@ namespace Common { namespace DynamicForwardProxy { DnsCacheImpl::DnsCacheImpl( - Event::Dispatcher& main_thread_dispatcher, ThreadLocal::SlotAllocator& tls, - Random::RandomGenerator& random, Filesystem::Instance& file_system, Runtime::Loader& loader, - Stats::Scope& root_scope, ProtobufMessage::ValidationVisitor& validation_visitor, + Server::Configuration::FactoryContextBase& context, const envoy::extensions::common::dynamic_forward_proxy::v3::DnsCacheConfig& config) - : main_thread_dispatcher_(main_thread_dispatcher), + : main_thread_dispatcher_(context.dispatcher()), dns_lookup_family_(Upstream::getDnsLookupFamilyFromEnum(config.dns_lookup_family())), - resolver_(selectDnsResolver(config, main_thread_dispatcher)), tls_slot_(tls), - scope_(root_scope.createScope(fmt::format("dns_cache.{}.", config.name()))), + resolver_(selectDnsResolver(config, main_thread_dispatcher_)), + tls_slot_(context.threadLocal()), + scope_(context.scope().createScope(fmt::format("dns_cache.{}.", config.name()))), stats_(generateDnsCacheStats(*scope_)), - resource_manager_(*scope_, loader, config.name(), config.dns_cache_circuit_breaker()), + resource_manager_(*scope_, context.runtime(), config.name(), + config.dns_cache_circuit_breaker()), refresh_interval_(PROTOBUF_GET_MS_OR_DEFAULT(config, dns_refresh_rate, 60000)), timeout_interval_(PROTOBUF_GET_MS_OR_DEFAULT(config, dns_query_timeout, 5000)), failure_backoff_strategy_( Config::Utility::prepareDnsRefreshStrategy< envoy::extensions::common::dynamic_forward_proxy::v3::DnsCacheConfig>( - config, refresh_interval_.count(), random)), - file_system_(file_system), validation_visitor_(validation_visitor), + config, refresh_interval_.count(), context.api().randomGenerator())), + file_system_(context.api().fileSystem()), + validation_visitor_(context.messageValidationVisitor()), host_ttl_(PROTOBUF_GET_MS_OR_DEFAULT(config, host_ttl, 300000)), max_hosts_(PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, max_hosts, 1024)) { tls_slot_.set([&](Event::Dispatcher&) { return std::make_shared(*this); }); diff --git a/source/extensions/common/dynamic_forward_proxy/dns_cache_impl.h b/source/extensions/common/dynamic_forward_proxy/dns_cache_impl.h index 28614a0181736..38374419f1f8d 100644 --- a/source/extensions/common/dynamic_forward_proxy/dns_cache_impl.h +++ b/source/extensions/common/dynamic_forward_proxy/dns_cache_impl.h @@ -5,6 +5,7 @@ #include "envoy/extensions/common/dynamic_forward_proxy/v3/dns_cache.pb.h" #include "envoy/http/filter.h" #include "envoy/network/dns.h" +#include "envoy/server/factory_context.h" #include "envoy/thread_local/thread_local.h" #include "source/common/common/cleanup.h" @@ -45,10 +46,7 @@ class DnsCacheImplTest; class DnsCacheImpl : public DnsCache, Logger::Loggable { public: - DnsCacheImpl(Event::Dispatcher& main_thread_dispatcher, ThreadLocal::SlotAllocator& tls, - Random::RandomGenerator& random, Filesystem::Instance& file_system, - Runtime::Loader& loader, Stats::Scope& root_scope, - ProtobufMessage::ValidationVisitor& validation_visitor, + DnsCacheImpl(Server::Configuration::FactoryContextBase& context, const envoy::extensions::common::dynamic_forward_proxy::v3::DnsCacheConfig& config); ~DnsCacheImpl() override; static DnsCacheStats generateDnsCacheStats(Stats::Scope& scope); diff --git a/source/extensions/common/dynamic_forward_proxy/dns_cache_manager_impl.cc b/source/extensions/common/dynamic_forward_proxy/dns_cache_manager_impl.cc index 5eecc3d17cc32..7dee0887fb44c 100644 --- a/source/extensions/common/dynamic_forward_proxy/dns_cache_manager_impl.cc +++ b/source/extensions/common/dynamic_forward_proxy/dns_cache_manager_impl.cc @@ -26,10 +26,7 @@ DnsCacheSharedPtr DnsCacheManagerImpl::getCache( return existing_cache->second.cache_; } - DnsCacheSharedPtr new_cache = std::make_shared( - context_.dispatcher(), context_.threadLocal(), context_.api().randomGenerator(), - context_.api().fileSystem(), context_.runtime(), context_.scope(), - context_.messageValidationVisitor(), config); + DnsCacheSharedPtr new_cache = std::make_shared(context_, config); caches_.emplace(config.name(), ActiveCache{config, new_cache}); return new_cache; } diff --git a/test/extensions/common/dynamic_forward_proxy/dns_cache_impl_test.cc b/test/extensions/common/dynamic_forward_proxy/dns_cache_impl_test.cc index 5b823d55118f2..1322ba1e5f398 100644 --- a/test/extensions/common/dynamic_forward_proxy/dns_cache_impl_test.cc +++ b/test/extensions/common/dynamic_forward_proxy/dns_cache_impl_test.cc @@ -13,6 +13,7 @@ #include "test/mocks/network/mocks.h" #include "test/mocks/protobuf/mocks.h" #include "test/mocks/runtime/mocks.h" +#include "test/mocks/server/factory_context.h" #include "test/mocks/thread_local/mocks.h" #include "test/test_common/registry.h" #include "test/test_common/simulated_time_system.h" @@ -44,23 +45,22 @@ class DnsCacheImplTest : public testing::Test, public Event::TestUsingSimulatedT } } - EXPECT_CALL(dispatcher_, isThreadSafe).WillRepeatedly(Return(true)); + EXPECT_CALL(context_.dispatcher_, isThreadSafe).WillRepeatedly(Return(true)); - EXPECT_CALL(dispatcher_, createDnsResolver(_, _)).WillOnce(Return(resolver_)); - dns_cache_ = std::make_unique(dispatcher_, tls_, random_, filesystem_, loader_, - store_, validation_visitor_, config_); + EXPECT_CALL(context_.dispatcher_, createDnsResolver(_, _)).WillOnce(Return(resolver_)); + dns_cache_ = std::make_unique(context_, config_); update_callbacks_handle_ = dns_cache_->addUpdateCallbacks(update_callbacks_); } ~DnsCacheImplTest() override { dns_cache_.reset(); - EXPECT_EQ(0, TestUtility::findGauge(store_, "dns_cache.foo.num_hosts")->value()); + EXPECT_EQ(0, TestUtility::findGauge(context_.scope_, "dns_cache.foo.num_hosts")->value()); } void checkStats(uint64_t query_attempt, uint64_t query_success, uint64_t query_failure, uint64_t address_changed, uint64_t added, uint64_t removed, uint64_t num_hosts) { const auto counter_value = [this](const std::string& name) { - return TestUtility::findCounter(store_, "dns_cache.foo." + name)->value(); + return TestUtility::findCounter(context_.scope_, "dns_cache.foo." + name)->value(); }; EXPECT_EQ(query_attempt, counter_value("dns_query_attempt")); @@ -69,21 +69,16 @@ class DnsCacheImplTest : public testing::Test, public Event::TestUsingSimulatedT EXPECT_EQ(address_changed, counter_value("host_address_changed")); EXPECT_EQ(added, counter_value("host_added")); EXPECT_EQ(removed, counter_value("host_removed")); - EXPECT_EQ(num_hosts, TestUtility::findGauge(store_, "dns_cache.foo.num_hosts")->value()); + EXPECT_EQ(num_hosts, + TestUtility::findGauge(context_.scope_, "dns_cache.foo.num_hosts")->value()); } + NiceMock context_; envoy::extensions::common::dynamic_forward_proxy::v3::DnsCacheConfig config_; - NiceMock dispatcher_; std::shared_ptr resolver_{std::make_shared()}; - NiceMock tls_; - NiceMock random_; - NiceMock filesystem_; - NiceMock loader_; - Stats::IsolatedStoreImpl store_; std::unique_ptr dns_cache_; MockUpdateCallbacks update_callbacks_; DnsCache::AddUpdateCallbacksHandlePtr update_callbacks_handle_; - Envoy::ProtobufMessage::MockValidationVisitor validation_visitor_; }; MATCHER_P3(DnsHostInfoEquals, address, resolved_host, is_ip_address, "") { @@ -148,8 +143,8 @@ TEST_F(DnsCacheImplTest, ResolveSuccess) { MockLoadDnsCacheEntryCallbacks callbacks; Network::DnsResolver::ResolveCb resolve_cb; - Event::MockTimer* resolve_timer = new Event::MockTimer(&dispatcher_); - Event::MockTimer* timeout_timer = new Event::MockTimer(&dispatcher_); + Event::MockTimer* resolve_timer = new Event::MockTimer(&context_.dispatcher_); + Event::MockTimer* timeout_timer = new Event::MockTimer(&context_.dispatcher_); EXPECT_CALL(*timeout_timer, enableTimer(std::chrono::milliseconds(5000), nullptr)); EXPECT_CALL(*resolver_, resolve("foo.com", _, _)) .WillOnce(DoAll(SaveArg<2>(&resolve_cb), Return(&resolver_->active_query_))); @@ -219,8 +214,8 @@ TEST_F(DnsCacheImplTest, Ipv4Address) { MockLoadDnsCacheEntryCallbacks callbacks; Network::DnsResolver::ResolveCb resolve_cb; - Event::MockTimer* resolve_timer = new Event::MockTimer(&dispatcher_); - Event::MockTimer* timeout_timer = new Event::MockTimer(&dispatcher_); + Event::MockTimer* resolve_timer = new Event::MockTimer(&context_.dispatcher_); + Event::MockTimer* timeout_timer = new Event::MockTimer(&context_.dispatcher_); EXPECT_CALL(*timeout_timer, enableTimer(std::chrono::milliseconds(5000), nullptr)); EXPECT_CALL(*resolver_, resolve("127.0.0.1", _, _)) .WillOnce(DoAll(SaveArg<2>(&resolve_cb), Return(&resolver_->active_query_))); @@ -247,8 +242,8 @@ TEST_F(DnsCacheImplTest, Ipv4AddressWithPort) { MockLoadDnsCacheEntryCallbacks callbacks; Network::DnsResolver::ResolveCb resolve_cb; - Event::MockTimer* resolve_timer = new Event::MockTimer(&dispatcher_); - Event::MockTimer* timeout_timer = new Event::MockTimer(&dispatcher_); + Event::MockTimer* resolve_timer = new Event::MockTimer(&context_.dispatcher_); + Event::MockTimer* timeout_timer = new Event::MockTimer(&context_.dispatcher_); EXPECT_CALL(*timeout_timer, enableTimer(std::chrono::milliseconds(5000), nullptr)); EXPECT_CALL(*resolver_, resolve("127.0.0.1", _, _)) .WillOnce(DoAll(SaveArg<2>(&resolve_cb), Return(&resolver_->active_query_))); @@ -275,8 +270,8 @@ TEST_F(DnsCacheImplTest, Ipv6Address) { MockLoadDnsCacheEntryCallbacks callbacks; Network::DnsResolver::ResolveCb resolve_cb; - Event::MockTimer* resolve_timer = new Event::MockTimer(&dispatcher_); - Event::MockTimer* timeout_timer = new Event::MockTimer(&dispatcher_); + Event::MockTimer* resolve_timer = new Event::MockTimer(&context_.dispatcher_); + Event::MockTimer* timeout_timer = new Event::MockTimer(&context_.dispatcher_); EXPECT_CALL(*timeout_timer, enableTimer(std::chrono::milliseconds(5000), nullptr)); EXPECT_CALL(*resolver_, resolve("::1", _, _)) .WillOnce(DoAll(SaveArg<2>(&resolve_cb), Return(&resolver_->active_query_))); @@ -301,8 +296,8 @@ TEST_F(DnsCacheImplTest, Ipv6AddressWithPort) { MockLoadDnsCacheEntryCallbacks callbacks; Network::DnsResolver::ResolveCb resolve_cb; - Event::MockTimer* resolve_timer = new Event::MockTimer(&dispatcher_); - Event::MockTimer* timeout_timer = new Event::MockTimer(&dispatcher_); + Event::MockTimer* resolve_timer = new Event::MockTimer(&context_.dispatcher_); + Event::MockTimer* timeout_timer = new Event::MockTimer(&context_.dispatcher_); EXPECT_CALL(*timeout_timer, enableTimer(std::chrono::milliseconds(5000), nullptr)); EXPECT_CALL(*resolver_, resolve("::1", _, _)) .WillOnce(DoAll(SaveArg<2>(&resolve_cb), Return(&resolver_->active_query_))); @@ -327,8 +322,8 @@ TEST_F(DnsCacheImplTest, TTL) { MockLoadDnsCacheEntryCallbacks callbacks; Network::DnsResolver::ResolveCb resolve_cb; - Event::MockTimer* resolve_timer = new Event::MockTimer(&dispatcher_); - Event::MockTimer* timeout_timer = new Event::MockTimer(&dispatcher_); + Event::MockTimer* resolve_timer = new Event::MockTimer(&context_.dispatcher_); + Event::MockTimer* timeout_timer = new Event::MockTimer(&context_.dispatcher_); EXPECT_CALL(*timeout_timer, enableTimer(std::chrono::milliseconds(5000), nullptr)); EXPECT_CALL(*resolver_, resolve("foo.com", _, _)) .WillOnce(DoAll(SaveArg<2>(&resolve_cb), Return(&resolver_->active_query_))); @@ -377,8 +372,8 @@ TEST_F(DnsCacheImplTest, TTL) { 1 /* added */, 1 /* removed */, 0 /* num hosts */); // Make sure we don't get a cache hit the next time the host is requested. - resolve_timer = new Event::MockTimer(&dispatcher_); - timeout_timer = new Event::MockTimer(&dispatcher_); + resolve_timer = new Event::MockTimer(&context_.dispatcher_); + timeout_timer = new Event::MockTimer(&context_.dispatcher_); EXPECT_CALL(*timeout_timer, enableTimer(std::chrono::milliseconds(5000), nullptr)); EXPECT_CALL(*resolver_, resolve("foo.com", _, _)) .WillOnce(DoAll(SaveArg<2>(&resolve_cb), Return(&resolver_->active_query_))); @@ -400,8 +395,8 @@ TEST_F(DnsCacheImplTest, TTLWithCustomParameters) { MockLoadDnsCacheEntryCallbacks callbacks; Network::DnsResolver::ResolveCb resolve_cb; - Event::MockTimer* resolve_timer = new Event::MockTimer(&dispatcher_); - Event::MockTimer* timeout_timer = new Event::MockTimer(&dispatcher_); + Event::MockTimer* resolve_timer = new Event::MockTimer(&context_.dispatcher_); + Event::MockTimer* timeout_timer = new Event::MockTimer(&context_.dispatcher_); EXPECT_CALL(*timeout_timer, enableTimer(std::chrono::milliseconds(1000), nullptr)); EXPECT_CALL(*resolver_, resolve("foo.com", _, _)) .WillOnce(DoAll(SaveArg<2>(&resolve_cb), Return(&resolver_->active_query_))); @@ -443,14 +438,14 @@ TEST_F(DnsCacheImplTest, InlineResolve) { MockLoadDnsCacheEntryCallbacks callbacks; Event::PostCb post_cb; - EXPECT_CALL(dispatcher_, post(_)).WillOnce(SaveArg<0>(&post_cb)); + EXPECT_CALL(context_.dispatcher_, post(_)).WillOnce(SaveArg<0>(&post_cb)); auto result = dns_cache_->loadDnsCacheEntry("localhost", 80, callbacks); EXPECT_EQ(DnsCache::LoadDnsCacheEntryStatus::Loading, result.status_); EXPECT_NE(result.handle_, nullptr); EXPECT_EQ(absl::nullopt, result.host_info_); - Event::MockTimer* resolve_timer = new Event::MockTimer(&dispatcher_); - Event::MockTimer* timeout_timer = new Event::MockTimer(&dispatcher_); + Event::MockTimer* resolve_timer = new Event::MockTimer(&context_.dispatcher_); + Event::MockTimer* timeout_timer = new Event::MockTimer(&context_.dispatcher_); EXPECT_CALL(*timeout_timer, enableTimer(std::chrono::milliseconds(5000), nullptr)); EXPECT_CALL(*resolver_, resolve("localhost", _, _)) .WillOnce(Invoke([](const std::string&, Network::DnsLookupFamily, @@ -476,8 +471,8 @@ TEST_F(DnsCacheImplTest, ResolveTimeout) { MockLoadDnsCacheEntryCallbacks callbacks; Network::DnsResolver::ResolveCb resolve_cb; - Event::MockTimer* resolve_timer = new Event::MockTimer(&dispatcher_); - Event::MockTimer* timeout_timer = new Event::MockTimer(&dispatcher_); + Event::MockTimer* resolve_timer = new Event::MockTimer(&context_.dispatcher_); + Event::MockTimer* timeout_timer = new Event::MockTimer(&context_.dispatcher_); EXPECT_CALL(*timeout_timer, enableTimer(std::chrono::milliseconds(5000), nullptr)); EXPECT_CALL(*resolver_, resolve("foo.com", _, _)) .WillOnce(DoAll(SaveArg<2>(&resolve_cb), Return(&resolver_->active_query_))); @@ -496,7 +491,8 @@ TEST_F(DnsCacheImplTest, ResolveTimeout) { timeout_timer->invokeCallback(); checkStats(1 /* attempt */, 0 /* success */, 1 /* failure */, 0 /* address changed */, 1 /* added */, 0 /* removed */, 1 /* num hosts */); - EXPECT_EQ(1, TestUtility::findCounter(store_, "dns_cache.foo.dns_query_timeout")->value()); + EXPECT_EQ(1, + TestUtility::findCounter(context_.scope_, "dns_cache.foo.dns_query_timeout")->value()); } // Resolve failure that returns no addresses. @@ -506,8 +502,8 @@ TEST_F(DnsCacheImplTest, ResolveFailure) { MockLoadDnsCacheEntryCallbacks callbacks; Network::DnsResolver::ResolveCb resolve_cb; - Event::MockTimer* resolve_timer = new Event::MockTimer(&dispatcher_); - Event::MockTimer* timeout_timer = new Event::MockTimer(&dispatcher_); + Event::MockTimer* resolve_timer = new Event::MockTimer(&context_.dispatcher_); + Event::MockTimer* timeout_timer = new Event::MockTimer(&context_.dispatcher_); EXPECT_CALL(*timeout_timer, enableTimer(std::chrono::milliseconds(5000), nullptr)); EXPECT_CALL(*resolver_, resolve("foo.com", _, _)) .WillOnce(DoAll(SaveArg<2>(&resolve_cb), Return(&resolver_->active_query_))); @@ -555,8 +551,8 @@ TEST_F(DnsCacheImplTest, ResolveFailureWithFailureRefreshRate) { MockLoadDnsCacheEntryCallbacks callbacks; Network::DnsResolver::ResolveCb resolve_cb; - Event::MockTimer* resolve_timer = new Event::MockTimer(&dispatcher_); - Event::MockTimer* timeout_timer = new Event::MockTimer(&dispatcher_); + Event::MockTimer* resolve_timer = new Event::MockTimer(&context_.dispatcher_); + Event::MockTimer* timeout_timer = new Event::MockTimer(&context_.dispatcher_); EXPECT_CALL(*timeout_timer, enableTimer(std::chrono::milliseconds(5000), nullptr)); EXPECT_CALL(*resolver_, resolve("foo.com", _, _)) .WillOnce(DoAll(SaveArg<2>(&resolve_cb), Return(&resolver_->active_query_))); @@ -570,7 +566,7 @@ TEST_F(DnsCacheImplTest, ResolveFailureWithFailureRefreshRate) { EXPECT_CALL(*timeout_timer, disableTimer()); EXPECT_CALL(update_callbacks_, onDnsHostAddOrUpdate(_, _)).Times(0); EXPECT_CALL(callbacks, onLoadDnsCacheComplete(DnsHostInfoAddressIsNull())); - ON_CALL(random_, random()).WillByDefault(Return(8000)); + ON_CALL(context_.api_.random_, random()).WillByDefault(Return(8000)); EXPECT_CALL(*resolve_timer, enableTimer(std::chrono::milliseconds(1000), _)); resolve_cb(Network::DnsResolver::ResolutionStatus::Failure, TestUtility::makeDnsResponse({})); checkStats(1 /* attempt */, 0 /* success */, 1 /* failure */, 0 /* address changed */, @@ -601,8 +597,8 @@ TEST_F(DnsCacheImplTest, ResolveSuccessWithEmptyResult) { MockLoadDnsCacheEntryCallbacks callbacks; Network::DnsResolver::ResolveCb resolve_cb; - Event::MockTimer* resolve_timer = new Event::MockTimer(&dispatcher_); - Event::MockTimer* timeout_timer = new Event::MockTimer(&dispatcher_); + Event::MockTimer* resolve_timer = new Event::MockTimer(&context_.dispatcher_); + Event::MockTimer* timeout_timer = new Event::MockTimer(&context_.dispatcher_); EXPECT_CALL(*timeout_timer, enableTimer(std::chrono::milliseconds(5000), nullptr)); EXPECT_CALL(*resolver_, resolve("foo.com", _, _)) .WillOnce(DoAll(SaveArg<2>(&resolve_cb), Return(&resolver_->active_query_))); @@ -824,7 +820,7 @@ TEST_F(DnsCacheImplTest, MaxHostOverflow) { EXPECT_EQ(DnsCache::LoadDnsCacheEntryStatus::Overflow, result.status_); EXPECT_EQ(result.handle_, nullptr); EXPECT_EQ(absl::nullopt, result.host_info_); - EXPECT_EQ(1, TestUtility::findCounter(store_, "dns_cache.foo.host_overflow")->value()); + EXPECT_EQ(1, TestUtility::findCounter(context_.scope_, "dns_cache.foo.host_overflow")->value()); } TEST_F(DnsCacheImplTest, CircuitBreakersNotInvoked) { @@ -840,17 +836,18 @@ TEST_F(DnsCacheImplTest, DnsCacheCircuitBreakersOverflow) { auto raii_ptr = dns_cache_->canCreateDnsRequest(); EXPECT_EQ(raii_ptr.get(), nullptr); - EXPECT_EQ(1, TestUtility::findCounter(store_, "dns_cache.foo.dns_rq_pending_overflow")->value()); + EXPECT_EQ( + 1, + TestUtility::findCounter(context_.scope_, "dns_cache.foo.dns_rq_pending_overflow")->value()); } TEST_F(DnsCacheImplTest, UseTcpForDnsLookupsOptionSetDeprecatedField) { initialize(); config_.set_use_tcp_for_dns_lookups(true); envoy::config::core::v3::DnsResolverOptions dns_resolver_options; - EXPECT_CALL(dispatcher_, createDnsResolver(_, _)) + EXPECT_CALL(context_.dispatcher_, createDnsResolver(_, _)) .WillOnce(DoAll(SaveArg<1>(&dns_resolver_options), Return(resolver_))); - DnsCacheImpl dns_cache_(dispatcher_, tls_, random_, filesystem_, loader_, store_, - validation_visitor_, config_); + DnsCacheImpl dns_cache_(context_, config_); // `true` here means dns_resolver_options.use_tcp_for_dns_lookups is set to true. EXPECT_EQ(true, dns_resolver_options.use_tcp_for_dns_lookups()); } @@ -861,10 +858,9 @@ TEST_F(DnsCacheImplTest, UseTcpForDnsLookupsOptionSet) { ->mutable_dns_resolver_options() ->set_use_tcp_for_dns_lookups(true); envoy::config::core::v3::DnsResolverOptions dns_resolver_options; - EXPECT_CALL(dispatcher_, createDnsResolver(_, _)) + EXPECT_CALL(context_.dispatcher_, createDnsResolver(_, _)) .WillOnce(DoAll(SaveArg<1>(&dns_resolver_options), Return(resolver_))); - DnsCacheImpl dns_cache_(dispatcher_, tls_, random_, filesystem_, loader_, store_, - validation_visitor_, config_); + DnsCacheImpl dns_cache_(context_, config_); // `true` here means dns_resolver_options.use_tcp_for_dns_lookups is set to true. EXPECT_EQ(true, dns_resolver_options.use_tcp_for_dns_lookups()); } @@ -875,10 +871,9 @@ TEST_F(DnsCacheImplTest, NoDefaultSearchDomainOptionSet) { ->mutable_dns_resolver_options() ->set_no_default_search_domain(true); envoy::config::core::v3::DnsResolverOptions dns_resolver_options; - EXPECT_CALL(dispatcher_, createDnsResolver(_, _)) + EXPECT_CALL(context_.dispatcher_, createDnsResolver(_, _)) .WillOnce(DoAll(SaveArg<1>(&dns_resolver_options), Return(resolver_))); - DnsCacheImpl dns_cache_(dispatcher_, tls_, random_, filesystem_, loader_, store_, - validation_visitor_, config_); + DnsCacheImpl dns_cache_(context_, config_); // `true` here means dns_resolver_options.no_default_search_domain is set to true. EXPECT_EQ(true, dns_resolver_options.no_default_search_domain()); } @@ -886,10 +881,9 @@ TEST_F(DnsCacheImplTest, NoDefaultSearchDomainOptionSet) { TEST_F(DnsCacheImplTest, UseTcpForDnsLookupsOptionUnSet) { initialize(); envoy::config::core::v3::DnsResolverOptions dns_resolver_options; - EXPECT_CALL(dispatcher_, createDnsResolver(_, _)) + EXPECT_CALL(context_.dispatcher_, createDnsResolver(_, _)) .WillOnce(DoAll(SaveArg<1>(&dns_resolver_options), Return(resolver_))); - DnsCacheImpl dns_cache_(dispatcher_, tls_, random_, filesystem_, loader_, store_, - validation_visitor_, config_); + DnsCacheImpl dns_cache_(context_, config_); // `false` here means dns_resolver_options.use_tcp_for_dns_lookups is set to false. EXPECT_EQ(false, dns_resolver_options.use_tcp_for_dns_lookups()); } @@ -897,24 +891,17 @@ TEST_F(DnsCacheImplTest, UseTcpForDnsLookupsOptionUnSet) { TEST_F(DnsCacheImplTest, NoDefaultSearchDomainOptionUnSet) { initialize(); envoy::config::core::v3::DnsResolverOptions dns_resolver_options; - EXPECT_CALL(dispatcher_, createDnsResolver(_, _)) + EXPECT_CALL(context_.dispatcher_, createDnsResolver(_, _)) .WillOnce(DoAll(SaveArg<1>(&dns_resolver_options), Return(resolver_))); - DnsCacheImpl dns_cache_(dispatcher_, tls_, random_, filesystem_, loader_, store_, - validation_visitor_, config_); + DnsCacheImpl dns_cache_(context_, config_); // `false` here means dns_resolver_options.no_default_search_domain is set to false. EXPECT_EQ(false, dns_resolver_options.no_default_search_domain()); } // DNS cache manager config tests. TEST(DnsCacheManagerImplTest, LoadViaConfig) { - NiceMock dispatcher; - NiceMock tls; - NiceMock random; - NiceMock loader; - Stats::IsolatedStoreImpl store; - NiceMock filesystem; - Envoy::ProtobufMessage::MockValidationVisitor visitor; - DnsCacheManagerImpl cache_manager(dispatcher, tls, random, filesystem, loader, store, visitor); + NiceMock context; + DnsCacheManagerImpl cache_manager(context); envoy::extensions::common::dynamic_forward_proxy::v3::DnsCacheConfig config1; config1.set_name("foo"); @@ -940,30 +927,20 @@ TEST(DnsCacheManagerImplTest, LoadViaConfig) { } TEST(DnsCacheConfigOptionsTest, EmtpyDnsResolutionConfig) { - NiceMock dispatcher; + NiceMock context; + envoy::extensions::common::dynamic_forward_proxy::v3::DnsCacheConfig config; std::shared_ptr resolver{std::make_shared()}; - NiceMock tls; - NiceMock random; - NiceMock loader; - Stats::IsolatedStoreImpl store; - envoy::extensions::common::dynamic_forward_proxy::v3::DnsCacheConfig config; std::vector expected_empty_dns_resolvers; - EXPECT_CALL(dispatcher, createDnsResolver(expected_empty_dns_resolvers, _)) + EXPECT_CALL(context.dispatcher_, createDnsResolver(expected_empty_dns_resolvers, _)) .WillOnce(Return(resolver)); - NiceMock filesystem; - Envoy::ProtobufMessage::MockValidationVisitor visitor; - DnsCacheImpl dns_cache(dispatcher, tls, random, filesystem, loader, store, visitor, config); + DnsCacheImpl dns_cache_(context, config); } TEST(DnsCacheConfigOptionsTest, NonEmptyDnsResolutionConfig) { - NiceMock dispatcher; - std::shared_ptr resolver{std::make_shared()}; - NiceMock tls; - NiceMock random; - NiceMock loader; - Stats::IsolatedStoreImpl store; + NiceMock context; envoy::extensions::common::dynamic_forward_proxy::v3::DnsCacheConfig config; + std::shared_ptr resolver{std::make_shared()}; envoy::config::core::v3::Address* dns_resolvers = config.mutable_dns_resolution_config()->add_resolvers(); @@ -972,12 +949,10 @@ TEST(DnsCacheConfigOptionsTest, NonEmptyDnsResolutionConfig) { std::vector expected_dns_resolvers; expected_dns_resolvers.push_back(Network::Address::resolveProtoAddress(*dns_resolvers)); - EXPECT_CALL(dispatcher, + EXPECT_CALL(context.dispatcher_, createDnsResolver(CustomDnsResolversSizeEquals(expected_dns_resolvers), _)) .WillOnce(Return(resolver)); - NiceMock filesystem; - Envoy::ProtobufMessage::MockValidationVisitor visitor; - DnsCacheImpl dns_cache_(dispatcher, tls, random, filesystem, loader, store, visitor, config); + DnsCacheImpl dns_cache_(context, config); } // Note: this test is done here, rather than a TYPED_TEST_SUITE in @@ -1049,8 +1024,8 @@ TEST_F(DnsCacheImplTest, ResolveSuccessWithCaching) { MockLoadDnsCacheEntryCallbacks callbacks; Network::DnsResolver::ResolveCb resolve_cb; - Event::MockTimer* resolve_timer = new Event::MockTimer(&dispatcher_); - Event::MockTimer* timeout_timer = new Event::MockTimer(&dispatcher_); + Event::MockTimer* resolve_timer = new Event::MockTimer(&context_.dispatcher_); + Event::MockTimer* timeout_timer = new Event::MockTimer(&context_.dispatcher_); EXPECT_CALL(*timeout_timer, enableTimer(std::chrono::milliseconds(5000), nullptr)); EXPECT_CALL(*resolver_, resolve("foo.com", _, _)) .WillOnce(DoAll(SaveArg<2>(&resolve_cb), Return(&resolver_->active_query_))); From 13c5ae3d37ae78cfcf0ddcfd8e49c83a0bf068a9 Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Mon, 13 Sep 2021 14:47:03 -0400 Subject: [PATCH 4/4] tidy, clawback, coverage Signed-off-by: Alyssa Wilk --- envoy/server/factory_context.h | 10 ++++------ envoy/upstream/cluster_factory.h | 11 ++++++++--- .../network/sni_dynamic_forward_proxy/proxy_filter.cc | 9 +++------ .../dynamic_forward_proxy/dns_cache_impl_test.cc | 2 +- 4 files changed, 16 insertions(+), 16 deletions(-) diff --git a/envoy/server/factory_context.h b/envoy/server/factory_context.h index 351f5904fef31..3624f32fe95b6 100644 --- a/envoy/server/factory_context.h +++ b/envoy/server/factory_context.h @@ -59,11 +59,6 @@ class FactoryContextBase { */ virtual Api::Api& api() PURE; - /** - * @return Upstream::ClusterManager& singleton for use by the entire server. - */ - virtual Upstream::ClusterManager& clusterManager() PURE; - /** * @return information about the local environment the server is running in. */ @@ -106,7 +101,10 @@ class FactoryContextBase { */ class CommonFactoryContext : public FactoryContextBase { public: - virtual ~CommonFactoryContext() = default; + /** + * @return Upstream::ClusterManager& singleton for use by the entire server. + */ + virtual Upstream::ClusterManager& clusterManager() PURE; /** * @return ProtobufMessage::ValidationContext& validation visitor for xDS and static configuration diff --git a/envoy/upstream/cluster_factory.h b/envoy/upstream/cluster_factory.h index 9d7dd9ff6bb63..9440e374c0edb 100644 --- a/envoy/upstream/cluster_factory.h +++ b/envoy/upstream/cluster_factory.h @@ -38,13 +38,16 @@ namespace Upstream { */ class ClusterFactoryContext : public Server::Configuration::FactoryContextBase { public: - virtual ~ClusterFactoryContext() = default; - /** * @return bool flag indicating whether the cluster is added via api. */ virtual bool addedViaApi() PURE; + /** + * @return Upstream::ClusterManager& singleton for use by the entire server. + */ + virtual Upstream::ClusterManager& clusterManager() PURE; + /** * @return Network::DnsResolverSharedPtr the dns resolver for the server. */ @@ -69,7 +72,9 @@ class ClusterFactoryContext : public Server::Configuration::FactoryContextBase { * @return Outlier::EventLoggerSharedPtr sink for outlier detection event logs. */ virtual Outlier::EventLoggerSharedPtr outlierEventLogger() PURE; - virtual Stats::Scope& scope() override { return stats(); } + + // Server::Configuration::FactoryContextBase + Stats::Scope& scope() override { return stats(); } }; /** diff --git a/source/extensions/filters/network/sni_dynamic_forward_proxy/proxy_filter.cc b/source/extensions/filters/network/sni_dynamic_forward_proxy/proxy_filter.cc index 3c8f6e0798906..b04749aabef10 100644 --- a/source/extensions/filters/network/sni_dynamic_forward_proxy/proxy_filter.cc +++ b/source/extensions/filters/network/sni_dynamic_forward_proxy/proxy_filter.cc @@ -51,24 +51,21 @@ Network::FilterStatus ProxyFilter::onNewConnection() { } switch (result.status_) { - case LoadDnsCacheEntryStatus::InCache: { + case LoadDnsCacheEntryStatus::InCache: ASSERT(cache_load_handle_ == nullptr); ENVOY_CONN_LOG(debug, "DNS cache entry already loaded, continuing", read_callbacks_->connection()); return Network::FilterStatus::Continue; - } - case LoadDnsCacheEntryStatus::Loading: { + case LoadDnsCacheEntryStatus::Loading: ASSERT(cache_load_handle_ != nullptr); ENVOY_CONN_LOG(debug, "waiting to load DNS cache entry", read_callbacks_->connection()); return Network::FilterStatus::StopIteration; - } - case LoadDnsCacheEntryStatus::Overflow: { + case LoadDnsCacheEntryStatus::Overflow: ASSERT(cache_load_handle_ == nullptr); ENVOY_CONN_LOG(debug, "DNS cache overflow", read_callbacks_->connection()); read_callbacks_->connection().close(Network::ConnectionCloseType::NoFlush); return Network::FilterStatus::StopIteration; } - } NOT_REACHED_GCOVR_EXCL_LINE; } diff --git a/test/extensions/common/dynamic_forward_proxy/dns_cache_impl_test.cc b/test/extensions/common/dynamic_forward_proxy/dns_cache_impl_test.cc index 1322ba1e5f398..289b8326b4918 100644 --- a/test/extensions/common/dynamic_forward_proxy/dns_cache_impl_test.cc +++ b/test/extensions/common/dynamic_forward_proxy/dns_cache_impl_test.cc @@ -372,7 +372,7 @@ TEST_F(DnsCacheImplTest, TTL) { 1 /* added */, 1 /* removed */, 0 /* num hosts */); // Make sure we don't get a cache hit the next time the host is requested. - resolve_timer = new Event::MockTimer(&context_.dispatcher_); + new Event::MockTimer(&context_.dispatcher_); // resolve_timer timeout_timer = new Event::MockTimer(&context_.dispatcher_); EXPECT_CALL(*timeout_timer, enableTimer(std::chrono::milliseconds(5000), nullptr)); EXPECT_CALL(*resolver_, resolve("foo.com", _, _))