diff --git a/changelogs/current/bug_fixes/dns_resolver__cares-shared-resolver-cross-thread.rst b/changelogs/current/bug_fixes/dns_resolver__cares-shared-resolver-cross-thread.rst new file mode 100644 index 0000000000000..aa34ee575542e --- /dev/null +++ b/changelogs/current/bug_fixes/dns_resolver__cares-shared-resolver-cross-thread.rst @@ -0,0 +1,9 @@ +Fixed a data race and cross-thread resolver sharing introduced with the +``envoy.restart_features.shared_cares_dns_resolver`` runtime guard. The shared resolver cache lived +on the process wide c-ares DNS resolver factory and was consulted by every caller, without +synchronization. Callers that create resolvers on worker thread could therefore race on the +cache, and could be handed a resolver bound to another thread's dispatcher. Moving the shared logic +into the upstream cluster similar to the default shared resolver eliminates any future issues and +avoid locking in the worker thread. Also switch default to for +``envoy.restart_features.shared_cares_dns_resolver`` to false. + diff --git a/changelogs/current/minor_behavior_changes/cares__disable_shared_resolver.rst b/changelogs/current/minor_behavior_changes/cares__disable_shared_resolver.rst new file mode 100644 index 0000000000000..4e7aed6ce6fa9 --- /dev/null +++ b/changelogs/current/minor_behavior_changes/cares__disable_shared_resolver.rst @@ -0,0 +1,2 @@ +Changes the default value of ``envoy.restart_features.shared_cares_dns_resolver`` to ``false``. +Turn this on if ``qcache_max_ttl`` is set in c-ares config and sharing the query cache across DNS clusters is desired. diff --git a/envoy/upstream/BUILD b/envoy/upstream/BUILD index 1b85059baf93f..cbc7eac06bf11 100644 --- a/envoy/upstream/BUILD +++ b/envoy/upstream/BUILD @@ -206,6 +206,7 @@ envoy_cc_library( "//envoy/ssl:context_interface", "//envoy/ssl:context_manager_interface", "@envoy_api//envoy/config/cluster/v3:pkg_cc_proto", + "@envoy_api//envoy/config/core/v3:pkg_cc_proto", ], ) diff --git a/envoy/upstream/cluster_factory.h b/envoy/upstream/cluster_factory.h index 55ec3acee5557..677c35a507c8a 100644 --- a/envoy/upstream/cluster_factory.h +++ b/envoy/upstream/cluster_factory.h @@ -12,6 +12,7 @@ #include "envoy/api/api.h" #include "envoy/common/random_generator.h" #include "envoy/config/cluster/v3/cluster.pb.h" +#include "envoy/config/core/v3/extension.pb.h" #include "envoy/config/typed_config.h" #include "envoy/event/dispatcher.h" #include "envoy/local_info/local_info.h" @@ -30,6 +31,10 @@ #include "envoy/upstream/outlier_detection.h" namespace Envoy { +namespace Network { +class DnsResolverFactory; +} // namespace Network + namespace Upstream { /** @@ -62,6 +67,18 @@ class ClusterFactoryContext { */ virtual Network::DnsResolverSharedPtr dnsResolver() PURE; + /** + * Returns the DNS resolver for a cluster that configures its own resolver. The returned resolver + * may be one that is already in use by another cluster configured identically, rather than a + * newly created one. + * + * @param dns_resolver_factory the factory resolved from typed_dns_resolver_config. + * @param typed_dns_resolver_config the cluster's resolver configuration. + */ + virtual absl::StatusOr sharedDnsResolver( + Network::DnsResolverFactory& dns_resolver_factory, + const envoy::config::core::v3::TypedExtensionConfig& typed_dns_resolver_config) PURE; + /** * @return Outlier::EventLoggerSharedPtr sink for outlier detection event logs. */ diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index 7a5c65a0c20ef..82a649f17b83a 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -155,6 +155,9 @@ RUNTIME_GUARD(envoy_reloadable_features_websocket_enable_timeout_on_upgrade_resp RUNTIME_GUARD(envoy_reloadable_features_xds_failover_to_primary_enabled); RUNTIME_GUARD(envoy_reloadable_features_xds_legacy_delta_skip_subsequent_node); RUNTIME_GUARD(envoy_restart_features_raise_file_limits); +// This is only needed if sharing c-ares query cache across dns clusters. +// Disabled by default to for now to prevent behavior changes. +FALSE_RUNTIME_GUARD(envoy_restart_features_shared_cares_dns_resolver); RUNTIME_GUARD(envoy_restart_features_validate_http3_pseudo_headers); RUNTIME_GUARD(envoy_restart_features_worker_threads_watchdog_fix); // Begin false flags. Most of them should come with a TODO to flip true. @@ -293,10 +296,6 @@ FALSE_RUNTIME_GUARD(envoy_reloadable_features_http2_record_histograms); // no certificate compression. FALSE_RUNTIME_GUARD(envoy_reloadable_features_tls_certificate_compression_brotli); -// DnsFilter created resolver on the worker thread which could lead to race when sharing resolvers -// Do not turn this on if DnsFilter is used or until the race is fixed -FALSE_RUNTIME_GUARD(envoy_restart_features_shared_cares_dns_resolver); - // Block of non-boolean flags. Use of int flags is deprecated. Do not add more. ABSL_FLAG(uint64_t, re2_max_program_size_error_level, 100, ""); // NOLINT ABSL_FLAG(uint64_t, re2_max_program_size_warn_level, // NOLINT diff --git a/source/common/upstream/BUILD b/source/common/upstream/BUILD index 43b723fccbd37..8e27efa4c347c 100644 --- a/source/common/upstream/BUILD +++ b/source/common/upstream/BUILD @@ -83,6 +83,7 @@ envoy_cc_library( deps = [ ":cds_api_lib", ":cluster_discovery_manager_lib", + ":cluster_dns_resolver_cache_lib", ":host_utility_lib", ":load_balancer_context_base_lib", ":load_stats_reporter_lib", @@ -515,6 +516,23 @@ envoy_cc_library( ], ) +envoy_cc_library( + name = "cluster_dns_resolver_cache_lib", + srcs = ["cluster_dns_resolver_cache.cc"], + hdrs = ["cluster_dns_resolver_cache.h"], + deps = [ + "//envoy/api:api_interface", + "//envoy/common:exception_lib", + "//envoy/event:dispatcher_interface", + "//envoy/network:dns_resolver_interface", + "//source/common/common:assert_lib", + "//source/common/common:minimal_logger_lib", + "//source/common/protobuf:utility_lib", + "//source/common/runtime:runtime_features_lib", + "@envoy_api//envoy/config/core/v3:pkg_cc_proto", + ], +) + envoy_cc_library( name = "cluster_factory_lib", srcs = ["cluster_factory_impl.cc"], @@ -546,6 +564,7 @@ envoy_cc_library( name = "cluster_factory_includes", hdrs = ["cluster_factory_impl.h"], deps = [ + ":cluster_dns_resolver_cache_lib", ":load_balancer_context_base_lib", ":outlier_detection_lib", ":resource_manager_lib", diff --git a/source/common/upstream/cluster_dns_resolver_cache.cc b/source/common/upstream/cluster_dns_resolver_cache.cc new file mode 100644 index 0000000000000..dbdd6aa375e5a --- /dev/null +++ b/source/common/upstream/cluster_dns_resolver_cache.cc @@ -0,0 +1,55 @@ +#include "source/common/upstream/cluster_dns_resolver_cache.h" + +#include "envoy/common/exception.h" + +#include "source/common/common/assert.h" +#include "source/common/common/logger.h" +#include "source/common/protobuf/utility.h" +#include "source/common/runtime/runtime_features.h" + +#include "absl/container/flat_hash_map.h" + +namespace Envoy { +namespace Upstream { + +absl::StatusOr ClusterDnsResolverCache::getOrCreate( + Network::DnsResolverFactory& dns_resolver_factory, Event::Dispatcher& dispatcher, Api::Api& api, + const envoy::config::core::v3::TypedExtensionConfig& typed_dns_resolver_config) { + ASSERT(dispatcher.isThreadSafe()); + + // Sharing is limited to the c-ares resolver: its channel and query cache are the state worth + // sharing, and limiting the scope keeps the other resolver types on their existing + // one-per-cluster behavior. + const bool shareable = + dns_resolver_factory.name() == Network::CaresDnsResolver && + Runtime::runtimeFeatureEnabled("envoy.restart_features.shared_cares_dns_resolver"); + if (!shareable) { + return dns_resolver_factory.createDnsResolver(dispatcher, api, typed_dns_resolver_config); + } + + // Hashing the typed config rather than the unpacked resolver config means two configurations + // that mean the same thing but do not serialize identically simply do not share. That is the + // safe direction to err in. + const std::size_t key = MessageUtil::hash(typed_dns_resolver_config); + const auto it = resolvers_.find(key); + if (it != resolvers_.end()) { + if (auto resolver = it->second.lock(); resolver != nullptr) { + ENVOY_LOG_MISC(trace, "reusing shared DNS resolver for config hash {}", key); + return resolver; + } + } + + auto resolver_or_error = + dns_resolver_factory.createDnsResolver(dispatcher, api, typed_dns_resolver_config); + RETURN_IF_NOT_OK_REF(resolver_or_error.status()); + + // Drop entries whose resolver is gone so the map does not grow as clusters come and go. + absl::erase_if(resolvers_, [](const auto& entry) { return entry.second.expired(); }); + resolvers_[key] = resolver_or_error.value(); + ENVOY_LOG_MISC(trace, "created shared DNS resolver for config hash {}, cache size {}", key, + resolvers_.size()); + return resolver_or_error; +} + +} // namespace Upstream +} // namespace Envoy diff --git a/source/common/upstream/cluster_dns_resolver_cache.h b/source/common/upstream/cluster_dns_resolver_cache.h new file mode 100644 index 0000000000000..92a03620aabe4 --- /dev/null +++ b/source/common/upstream/cluster_dns_resolver_cache.h @@ -0,0 +1,59 @@ +#pragma once + +#include +#include + +#include "envoy/api/api.h" +#include "envoy/config/core/v3/extension.pb.h" +#include "envoy/event/dispatcher.h" +#include "envoy/network/dns_resolver.h" + +#include "absl/container/flat_hash_map.h" +#include "absl/status/statusor.h" + +namespace Envoy { +namespace Upstream { + +/** + * Caches the DNS resolvers created for clusters, so that clusters configured with an identical + * resolver configuration share a single resolver and, with it, the resolver's query cache. + * + * This deliberately lives on the cluster manager factory rather than on the DNS resolver factory: + * + * - It is owned per server, so two servers running in the same process (see + * test/integration/multi_envoy_test.cc) cannot share resolvers with each other. + * - It is only ever used while creating a cluster, which happens on the main thread with the main + * thread dispatcher, so it needs no locking and every cached resolver is bound to the one + * dispatcher that all of its users run on. + * - Nothing outside cluster creation can reach it, so callers that must stay isolated - notably + * the UDP DNS filter, which is constructed once per worker thread - keep getting their own + * resolver from DnsResolverFactory::createDnsResolver(). + * + * Sharing is limited to the c-ares resolver, whose channel and query cache are what the sharing is + * for, and can be disabled entirely with the "envoy.restart_features.shared_cares_dns_resolver" + * runtime guard. + */ +class ClusterDnsResolverCache { +public: + /** + * @return a DNS resolver for the given configuration, either newly created or shared with + * another cluster that was created with an identical configuration. + * @param dns_resolver_factory the factory resolved from typed_dns_resolver_config. + * @param dispatcher the main thread dispatcher, which the resolver is bound to. + * @param api API interface to interact with system resources. + * @param typed_dns_resolver_config the resolver configuration. + */ + absl::StatusOr + getOrCreate(Network::DnsResolverFactory& dns_resolver_factory, Event::Dispatcher& dispatcher, + Api::Api& api, + const envoy::config::core::v3::TypedExtensionConfig& typed_dns_resolver_config); + +private: + // Keyed on the hash of the typed resolver config. Entries are weak so that a resolver is + // released once the last cluster using it goes away, and so the map does not grow without bound + // as clusters churn. + absl::flat_hash_map> resolvers_; +}; + +} // namespace Upstream +} // namespace Envoy diff --git a/source/common/upstream/cluster_factory_impl.cc b/source/common/upstream/cluster_factory_impl.cc index 556e1a19057c1..af60825720744 100644 --- a/source/common/upstream/cluster_factory_impl.cc +++ b/source/common/upstream/cluster_factory_impl.cc @@ -19,7 +19,8 @@ ClusterFactoryImplBase::create(const envoy::config::cluster::v3::Cluster& cluste Server::Configuration::ServerFactoryContext& server_context, LazyCreateDnsResolver dns_resolver_fn, Outlier::EventLoggerSharedPtr outlier_event_logger, - bool added_via_api) { + bool added_via_api, + OptRef dns_resolver_cache) { std::string cluster_name; std::string cluster_config_type_name; @@ -75,7 +76,8 @@ ClusterFactoryImplBase::create(const envoy::config::cluster::v3::Cluster& cluste } ClusterFactoryContextImpl context(server_context, dns_resolver_fn, - std::move(outlier_event_logger), added_via_api); + std::move(outlier_event_logger), added_via_api, + dns_resolver_cache); return factory->create(cluster, context); } @@ -85,9 +87,12 @@ ClusterFactoryImplBase::selectDnsResolver(const envoy::config::cluster::v3::Clus // We make this a shared pointer to deal with the distinct ownership // scenarios that can exist: in one case, we pass in the "default" // DNS resolver that is owned by the Server::Instance. In the case - // where 'dns_resolvers' is specified, we have per-cluster DNS - // resolvers that are created here but ownership resides with - // StrictDnsClusterImpl/LogicalDnsCluster. + // where the cluster configures its own resolver, the resolver is + // created here and kept alive by the clusters using it. Note that it + // is not necessarily one resolver per cluster: it may be shared with + // other clusters configured identically. The cache that hands it out + // holds only a weak reference, so the resolver is released once the + // last cluster using it goes away. if ((cluster.has_typed_dns_resolver_config() && !(cluster.typed_dns_resolver_config().typed_config().type_url().empty())) || (cluster.has_dns_resolution_config() && @@ -97,9 +102,7 @@ ClusterFactoryImplBase::selectDnsResolver(const envoy::config::cluster::v3::Clus envoy::config::core::v3::TypedExtensionConfig typed_dns_resolver_config; Network::DnsResolverFactory& dns_resolver_factory = Network::createDnsResolverFactoryFromProto(cluster, typed_dns_resolver_config); - auto& server_context = context.serverFactoryContext(); - return dns_resolver_factory.createDnsResolver(server_context.mainThreadDispatcher(), - server_context.api(), typed_dns_resolver_config); + return context.sharedDnsResolver(dns_resolver_factory, typed_dns_resolver_config); } return context.dnsResolver(); @@ -111,9 +114,7 @@ absl::StatusOr ClusterFactoryImplBase::selectDnsR if (typed_dns_resolver_config.has_typed_config()) { Network::DnsResolverFactory& dns_resolver_factory = Network::createDnsResolverFactoryFromTypedConfig(typed_dns_resolver_config); - auto& server_context = context.serverFactoryContext(); - return dns_resolver_factory.createDnsResolver(server_context.mainThreadDispatcher(), - server_context.api(), typed_dns_resolver_config); + return context.sharedDnsResolver(dns_resolver_factory, typed_dns_resolver_config); } return context.dnsResolver(); } diff --git a/source/common/upstream/cluster_factory_impl.h b/source/common/upstream/cluster_factory_impl.h index be21c1d08a2c6..6fcbc4e4d3de0 100644 --- a/source/common/upstream/cluster_factory_impl.h +++ b/source/common/upstream/cluster_factory_impl.h @@ -16,6 +16,7 @@ #include "envoy/event/timer.h" #include "envoy/local_info/local_info.h" #include "envoy/network/dns.h" +#include "envoy/network/dns_resolver.h" #include "envoy/runtime/runtime.h" #include "envoy/secret/secret_manager.h" #include "envoy/server/options.h" @@ -39,6 +40,7 @@ #include "source/common/network/utility.h" #include "source/common/protobuf/utility.h" #include "source/common/stats/isolated_store_impl.h" +#include "source/common/upstream/cluster_dns_resolver_cache.h" #include "source/common/upstream/load_balancer_context_base.h" #include "source/common/upstream/outlier_detection_impl.h" #include "source/common/upstream/resource_manager_impl.h" @@ -54,8 +56,10 @@ class ClusterFactoryContextImpl : public ClusterFactoryContext { ClusterFactoryContextImpl(Server::Configuration::ServerFactoryContext& server_context, LazyCreateDnsResolver dns_resolver_fn, - Outlier::EventLoggerSharedPtr outlier_event_logger, bool added_via_api) + Outlier::EventLoggerSharedPtr outlier_event_logger, bool added_via_api, + OptRef dns_resolver_cache = {}) : server_context_(server_context), dns_resolver_fn_(dns_resolver_fn), + dns_resolver_cache_(dns_resolver_cache), outlier_event_logger_(std::move(outlier_event_logger)), validation_visitor_( added_via_api ? server_context.messageValidationContext().dynamicValidationVisitor() @@ -75,6 +79,24 @@ class ClusterFactoryContextImpl : public ClusterFactoryContext { } return dns_resolver_; } + // Sharing is limited to the c-ares resolver, where the query cache is the reason for sharing + // among clusters; see ClusterDnsResolverCache. Clusters that end up on the same c-ares resolver + // share more than its query cache: anything scoped to the channel rather than to a single query + // is common to all of them, including the ``udp_max_queries`` budget and channel reinitialization + // triggered by a query timeout. + absl::StatusOr sharedDnsResolver( + Network::DnsResolverFactory& dns_resolver_factory, + const envoy::config::core::v3::TypedExtensionConfig& typed_dns_resolver_config) override { + // The cache is absent when a cluster is created outside the cluster manager, e.g. in tests. + // In that case each cluster simply gets its own resolver. + if (!dns_resolver_cache_.has_value()) { + return dns_resolver_factory.createDnsResolver( + server_context_.mainThreadDispatcher(), server_context_.api(), typed_dns_resolver_config); + } + return dns_resolver_cache_->getOrCreate(dns_resolver_factory, + server_context_.mainThreadDispatcher(), + server_context_.api(), typed_dns_resolver_config); + } Outlier::EventLoggerSharedPtr outlierEventLogger() override { return outlier_event_logger_; } bool addedViaApi() override { return added_via_api_; } @@ -82,6 +104,7 @@ class ClusterFactoryContextImpl : public ClusterFactoryContext { Server::Configuration::ServerFactoryContext& server_context_; Network::DnsResolverSharedPtr dns_resolver_; LazyCreateDnsResolver dns_resolver_fn_; + OptRef dns_resolver_cache_; Outlier::EventLoggerSharedPtr outlier_event_logger_; ProtobufMessage::ValidationVisitor& validation_visitor_; const bool added_via_api_; @@ -102,7 +125,7 @@ class ClusterFactoryImplBase : public ClusterFactory { create(const envoy::config::cluster::v3::Cluster& cluster, Server::Configuration::ServerFactoryContext& server_context, LazyCreateDnsResolver dns_resolver_fn, Outlier::EventLoggerSharedPtr outlier_event_logger, - bool added_via_api); + bool added_via_api, OptRef dns_resolver_cache = {}); /** * Create a dns resolver to be used by the cluster. diff --git a/source/common/upstream/cluster_manager_impl.cc b/source/common/upstream/cluster_manager_impl.cc index 16c01cc1bfdd5..8f546a4d39657 100644 --- a/source/common/upstream/cluster_manager_impl.cc +++ b/source/common/upstream/cluster_manager_impl.cc @@ -2489,7 +2489,7 @@ ProdClusterManagerFactory::clusterFromProto(const envoy::config::cluster::v3::Cl Outlier::EventLoggerSharedPtr outlier_event_logger, bool added_via_api) { return ClusterFactoryImplBase::create(cluster, context_, dns_resolver_fn_, outlier_event_logger, - added_via_api); + added_via_api, makeOptRef(dns_resolver_cache_)); } absl::StatusOr diff --git a/source/common/upstream/cluster_manager_impl.h b/source/common/upstream/cluster_manager_impl.h index 90944375ce6f6..b357c83e026f2 100644 --- a/source/common/upstream/cluster_manager_impl.h +++ b/source/common/upstream/cluster_manager_impl.h @@ -38,6 +38,7 @@ #include "source/common/quic/quic_stat_names.h" #include "source/common/tcp/async_tcp_client_impl.h" #include "source/common/upstream/cluster_discovery_manager.h" +#include "source/common/upstream/cluster_dns_resolver_cache.h" #include "source/common/upstream/host_utility.h" #include "source/common/upstream/priority_conn_pool_map.h" #include "source/common/upstream/upstream_impl.h" @@ -92,6 +93,9 @@ class ProdClusterManagerFactory : public ClusterManagerFactory { Server::Configuration::ServerFactoryContext& context_; Stats::Store& stats_; LazyCreateDnsResolver dns_resolver_fn_; + // Lets clusters configured with an identical DNS resolver configuration share a resolver. Owned + // here so that the sharing is scoped to this server's clusters and to the main thread. + ClusterDnsResolverCache dns_resolver_cache_; Quic::QuicStatNames& quic_stat_names_; Http::HttpServerPropertiesCacheManager& alternate_protocols_cache_manager_; }; diff --git a/source/extensions/network/dns_resolver/cares/dns_impl.cc b/source/extensions/network/dns_resolver/cares/dns_impl.cc index f5668650ca146..2926cf959c5c2 100644 --- a/source/extensions/network/dns_resolver/cares/dns_impl.cc +++ b/source/extensions/network/dns_resolver/cares/dns_impl.cc @@ -23,7 +23,6 @@ #include "source/common/protobuf/utility.h" #include "source/common/runtime/runtime_features.h" -#include "absl/container/flat_hash_map.h" #include "absl/strings/str_join.h" #include "ares.h" @@ -681,18 +680,6 @@ class CaresDnsResolverFactory : public DnsResolverFactory, // Only c-ares DNS factory will call into this function. // Directly unpack the typed config to a c-ares object. RETURN_IF_NOT_OK(Envoy::MessageUtil::unpackTo(typed_dns_resolver_config.typed_config(), cares)); - std::size_t key = 0; - if (Runtime::runtimeFeatureEnabled("envoy.restart_features.shared_cares_dns_resolver")) { - key = MessageUtil::hash(cares); - const auto it = resolver_map_.find(key); - if (it != resolver_map_.end()) { - auto resolver = it->second.lock(); - if (resolver) { - ENVOY_LOG(trace, "found existing resolvers: {}", key); - return resolver; - } - } - } if (!cares.resolvers().empty()) { const auto& resolver_addrs = cares.resolvers(); @@ -706,24 +693,8 @@ class CaresDnsResolverFactory : public DnsResolverFactory, auto csv_or_error = DnsResolverImpl::maybeBuildResolversCsv(resolvers); RETURN_IF_NOT_OK(csv_or_error.status()); - auto resolver = std::make_shared( - cares, dispatcher, csv_or_error.value(), api.rootScope()); - if (Runtime::runtimeFeatureEnabled("envoy.restart_features.shared_cares_dns_resolver")) { - // clean up any nil resolver in the map so it doesn't keep growing - auto original_size = resolver_map_.size(); - absl::erase_if( - resolver_map_, - [](const std::pair>& entry) { - return entry.second.lock() == nullptr; - }); - if (resolver_map_.size() < original_size) { - ENVOY_LOG(trace, "cleaned up {} entries in resolver_map_", - original_size - resolver_map_.size()); - } - resolver_map_[key] = resolver; - ENVOY_LOG(trace, "resolver_map_ size after adding: {}", resolver_map_.size()); - } - return resolver; + return std::make_shared(cares, dispatcher, csv_or_error.value(), + api.rootScope()); } void initialize() override { @@ -748,7 +719,6 @@ class CaresDnsResolverFactory : public DnsResolverFactory, private: bool ares_library_initialized_ ABSL_GUARDED_BY(mutex_){false}; absl::Mutex mutex_; - mutable absl::flat_hash_map> resolver_map_; }; // Register the CaresDnsResolverFactory diff --git a/test/common/upstream/BUILD b/test/common/upstream/BUILD index 7e0cd48f12ffa..ebb710e5b557b 100644 --- a/test/common/upstream/BUILD +++ b/test/common/upstream/BUILD @@ -768,6 +768,23 @@ envoy_cc_test_library( ], ) +envoy_cc_test( + name = "cluster_dns_resolver_cache_test", + srcs = ["cluster_dns_resolver_cache_test.cc"], + rbe_pool = "6gig", + deps = [ + "//source/common/network/dns_resolver:dns_factory_util_lib", + "//source/common/upstream:cluster_dns_resolver_cache_lib", + "//source/extensions/network/dns_resolver/cares:config", + "//source/extensions/network/dns_resolver/getaddrinfo:config", + "//test/test_common:test_runtime_lib", + "//test/test_common:utility_lib", + "@envoy_api//envoy/config/core/v3:pkg_cc_proto", + "@envoy_api//envoy/extensions/network/dns_resolver/cares/v3:pkg_cc_proto", + "@envoy_api//envoy/extensions/network/dns_resolver/getaddrinfo/v3:pkg_cc_proto", + ], +) + envoy_cc_test( name = "cluster_factory_impl_test", srcs = ["cluster_factory_impl_test.cc"], diff --git a/test/common/upstream/cluster_dns_resolver_cache_test.cc b/test/common/upstream/cluster_dns_resolver_cache_test.cc new file mode 100644 index 0000000000000..6283cac682c4c --- /dev/null +++ b/test/common/upstream/cluster_dns_resolver_cache_test.cc @@ -0,0 +1,128 @@ +#include "envoy/config/core/v3/extension.pb.h" +#include "envoy/extensions/network/dns_resolver/cares/v3/cares_dns_resolver.pb.h" +#include "envoy/extensions/network/dns_resolver/getaddrinfo/v3/getaddrinfo_dns_resolver.pb.h" + +#include "source/common/network/dns_resolver/dns_factory_util.h" +#include "source/common/upstream/cluster_dns_resolver_cache.h" + +#include "test/test_common/test_runtime.h" +#include "test/test_common/utility.h" + +#include "gtest/gtest.h" + +namespace Envoy { +namespace Upstream { +namespace { + +class ClusterDnsResolverCacheTest : public testing::Test { +protected: + ClusterDnsResolverCacheTest() + : api_(Api::createApiForTest()), dispatcher_(api_->allocateDispatcher("test_thread")) {} + + // `qcache_max_ttl` is only used here as a convenient way to vary the config hash. + envoy::config::core::v3::TypedExtensionConfig caresConfig(uint32_t qcache_max_ttl) { + envoy::extensions::network::dns_resolver::cares::v3::CaresDnsResolverConfig cares; + cares.mutable_qcache_max_ttl()->set_value(qcache_max_ttl); + + envoy::config::core::v3::TypedExtensionConfig typed_config; + std::ignore = typed_config.mutable_typed_config()->PackFrom(cares); + typed_config.set_name(std::string(Network::CaresDnsResolver)); + return typed_config; + } + + envoy::config::core::v3::TypedExtensionConfig getAddrInfoConfig() { + envoy::extensions::network::dns_resolver::getaddrinfo::v3::GetAddrInfoDnsResolverConfig config; + + envoy::config::core::v3::TypedExtensionConfig typed_config; + std::ignore = typed_config.mutable_typed_config()->PackFrom(config); + typed_config.set_name("envoy.network.dns_resolver.getaddrinfo"); + return typed_config; + } + + Network::DnsResolverSharedPtr + getOrCreate(const envoy::config::core::v3::TypedExtensionConfig& config) { + Network::DnsResolverFactory& factory = Network::createDnsResolverFactoryFromTypedConfig(config); + auto resolver_or_error = cache_.getOrCreate(factory, *dispatcher_, *api_, config); + EXPECT_TRUE(resolver_or_error.ok()); + return resolver_or_error.value(); + } + + Api::ApiPtr api_; + Event::DispatcherPtr dispatcher_; + ClusterDnsResolverCache cache_; +}; + +// The point of the cache: clusters configured identically share one resolver, and with it the +// resolver's query cache. +TEST_F(ClusterDnsResolverCacheTest, SharesResolverForIdenticalConfig) { + TestScopedRuntime scoped_runtime; + scoped_runtime.mergeValues({{"envoy.restart_features.shared_cares_dns_resolver", "true"}}); + + auto config = caresConfig(11); + + auto resolver1 = getOrCreate(config); + auto resolver2 = getOrCreate(config); + + EXPECT_NE(nullptr, resolver1); + EXPECT_EQ(resolver1.get(), resolver2.get()); +} + +TEST_F(ClusterDnsResolverCacheTest, DoesNotShareResolverForDifferentConfig) { + TestScopedRuntime scoped_runtime; + scoped_runtime.mergeValues({{"envoy.restart_features.shared_cares_dns_resolver", "true"}}); + + auto resolver1 = getOrCreate(caresConfig(22)); + auto resolver2 = getOrCreate(caresConfig(33)); + + EXPECT_NE(resolver1.get(), resolver2.get()); +} + +TEST_F(ClusterDnsResolverCacheTest, DoesNotShareWhenRuntimeGuardDisabled) { + TestScopedRuntime scoped_runtime; + scoped_runtime.mergeValues({{"envoy.restart_features.shared_cares_dns_resolver", "false"}}); + + auto config = caresConfig(44); + + auto resolver1 = getOrCreate(config); + auto resolver2 = getOrCreate(config); + + EXPECT_NE(resolver1.get(), resolver2.get()); +} + +// Sharing is deliberately limited to c-ares. Other resolver types keep one resolver per cluster. +TEST_F(ClusterDnsResolverCacheTest, DoesNotShareNonCaresResolver) { + TestScopedRuntime scoped_runtime; + scoped_runtime.mergeValues({{"envoy.restart_features.shared_cares_dns_resolver", "true"}}); + + auto config = getAddrInfoConfig(); + + auto resolver1 = getOrCreate(config); + auto resolver2 = getOrCreate(config); + + EXPECT_NE(nullptr, resolver1); + EXPECT_NE(resolver1.get(), resolver2.get()); +} + +// Entries are weak, so once every cluster using a resolver is gone the cache creates a new one +// rather than handing back a dangling pointer. +TEST_F(ClusterDnsResolverCacheTest, CreatesNewResolverAfterCachedOneIsReleased) { + TestScopedRuntime scoped_runtime; + scoped_runtime.mergeValues({{"envoy.restart_features.shared_cares_dns_resolver", "true"}}); + + auto config = caresConfig(55); + + { + auto released = getOrCreate(config); + } + + auto resolver = getOrCreate(config); + EXPECT_NE(nullptr, resolver); + + // And the fresh one is itself cached. + auto shared = getOrCreate(config); + EXPECT_EQ(resolver.get(), shared.get()); +} + +} // namespace +} // namespace Upstream +} // namespace Envoy diff --git a/test/extensions/network/dns_resolver/cares/BUILD b/test/extensions/network/dns_resolver/cares/BUILD index a82964825c1a8..8947b27f496fa 100644 --- a/test/extensions/network/dns_resolver/cares/BUILD +++ b/test/extensions/network/dns_resolver/cares/BUILD @@ -34,7 +34,6 @@ envoy_cc_test( "//test/mocks/network:network_mocks", "//test/test_common:environment_lib", "//test/test_common:network_utility_lib", - "//test/test_common:test_runtime_lib", "//test/test_common:threadsafe_singleton_injector_lib", "//test/test_common:utility_lib", "@envoy_api//envoy/config/core/v3:pkg_cc_proto", diff --git a/test/extensions/network/dns_resolver/cares/dns_impl_test.cc b/test/extensions/network/dns_resolver/cares/dns_impl_test.cc index 2cd1d996a54d5..563b46c175325 100644 --- a/test/extensions/network/dns_resolver/cares/dns_impl_test.cc +++ b/test/extensions/network/dns_resolver/cares/dns_impl_test.cc @@ -33,7 +33,6 @@ #include "test/test_common/environment.h" #include "test/test_common/network_utility.h" #include "test/test_common/printers.h" -#include "test/test_common/test_runtime.h" #include "test/test_common/threadsafe_singleton_injector.h" #include "test/test_common/utility.h" @@ -2295,104 +2294,6 @@ TEST_F(DnsImplConstructor, VerifyCustomQcacheMaxTtl) { ares_destroy_options(&opts); } -TEST_F(DnsImplConstructor, ReusesResolverForIdenticalConfig) { - TestScopedRuntime scoped_runtime; - scoped_runtime.mergeValues({{"envoy.restart_features.shared_cares_dns_resolver", "true"}}); - - auto typed_dns_resolver_config = getCaresDnsResolverConfig(0); - Network::DnsResolverFactory& dns_resolver_factory = - createDnsResolverFactoryFromTypedConfig(typed_dns_resolver_config); - - auto resolver1 = - dns_resolver_factory.createDnsResolver(*dispatcher_, *api_, typed_dns_resolver_config) - .value(); - auto resolver2 = - dns_resolver_factory.createDnsResolver(*dispatcher_, *api_, typed_dns_resolver_config) - .value(); - - EXPECT_EQ(resolver1.get(), resolver2.get()); -} - -TEST_F(DnsImplConstructor, DoesNotReuseResolverForIdenticalConfigWhenFeatureDisabled) { - TestScopedRuntime scoped_runtime; - scoped_runtime.mergeValues({{"envoy.restart_features.shared_cares_dns_resolver", "false"}}); - - auto typed_dns_resolver_config = getCaresDnsResolverConfig(0); - Network::DnsResolverFactory& dns_resolver_factory = - createDnsResolverFactoryFromTypedConfig(typed_dns_resolver_config); - - auto resolver1 = - dns_resolver_factory.createDnsResolver(*dispatcher_, *api_, typed_dns_resolver_config) - .value(); - auto resolver2 = - dns_resolver_factory.createDnsResolver(*dispatcher_, *api_, typed_dns_resolver_config) - .value(); - - EXPECT_NE(resolver1.get(), resolver2.get()); -} - -TEST_F(DnsImplConstructor, DoesNotReuseResolverForDifferentConfig) { - TestScopedRuntime scoped_runtime; - scoped_runtime.mergeValues({{"envoy.restart_features.shared_cares_dns_resolver", "true"}}); - - auto typed_dns_resolver_config1 = getCaresDnsResolverConfig(67); - auto typed_dns_resolver_config2 = getCaresDnsResolverConfig(123); - - Network::DnsResolverFactory& dns_resolver_factory = - createDnsResolverFactoryFromTypedConfig(typed_dns_resolver_config1); - - auto resolver1 = - dns_resolver_factory.createDnsResolver(*dispatcher_, *api_, typed_dns_resolver_config1) - .value(); - auto resolver2 = - dns_resolver_factory.createDnsResolver(*dispatcher_, *api_, typed_dns_resolver_config2) - .value(); - - EXPECT_NE(resolver1.get(), resolver2.get()); -} - -TEST_F(DnsImplConstructor, CleansExpiredResolverBeforeReinsertingIdenticalConfig) { - TestScopedRuntime scoped_runtime; - scoped_runtime.mergeValues({{"envoy.restart_features.shared_cares_dns_resolver", "true"}}); - - auto typed_dns_resolver_config = getCaresDnsResolverConfig(1234); - - Network::DnsResolverFactory& dns_resolver_factory = - createDnsResolverFactoryFromTypedConfig(typed_dns_resolver_config); - - DnsResolver* first_resolver = nullptr; - { - auto resolver1 = - dns_resolver_factory.createDnsResolver(*dispatcher_, *api_, typed_dns_resolver_config) - .value(); - // Save the pointer only for identity comparison after resolver1 is destroyed. - first_resolver = resolver1.get(); - } - - auto typed_dns_resolver_config2 = getCaresDnsResolverConfig(5678); - // Create another resolver with a different config to trigger eviction of the first resolver from - // the resolver map. - auto resolver2 = - dns_resolver_factory.createDnsResolver(*dispatcher_, *api_, typed_dns_resolver_config2) - .value(); - - // This is a dummy resolver so if memory is immediately reused, this will take the memory released - // by the first resolver. - auto typed_dns_resolver_config3 = getCaresDnsResolverConfig(890); - auto resolver3 = - dns_resolver_factory.createDnsResolver(*dispatcher_, *api_, typed_dns_resolver_config3) - .value(); - - // Create a forth resolver with the same config as the first resolver and verify the first - // resolver is not reused, which proves that the first resolver was evicted from the resolver map. - auto resolver4 = - dns_resolver_factory.createDnsResolver(*dispatcher_, *api_, typed_dns_resolver_config) - .value(); - - EXPECT_NE(resolver2.get(), resolver4.get()); - EXPECT_NE(first_resolver, resolver4.get()); -} - class DnsImplAresFlagsForMaxUdpQueriesinTest : public DnsImplTest { protected: bool tcpOnly() const override { return false; }