c-ares: move dns shared resolver logic to upstream cluster

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.
+

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.

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",
     ],
 )
 

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<Network::DnsResolverSharedPtr> 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.
    */

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

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",

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<Network::DnsResolverSharedPtr> 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

source/common/upstream/cluster_dns_resolver_cache.h

@@ -0,0 +1,59 @@
+#pragma once
+
+#include <cstddef>
+#include <memory>
+
+#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<Network::DnsResolverSharedPtr>
+  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<std::size_t, std::weak_ptr<Network::DnsResolver>> resolvers_;
+};
+
+} // namespace Upstream
+} // namespace Envoy

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<ClusterDnsResolverCache> 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<Network::DnsResolverSharedPtr> 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();
 }

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<ClusterDnsResolverCache> 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,13 +79,32 @@ 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<Network::DnsResolverSharedPtr> 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_; }
 
 private:
   Server::Configuration::ServerFactoryContext& server_context_;
   Network::DnsResolverSharedPtr dns_resolver_;
   LazyCreateDnsResolver dns_resolver_fn_;
+  OptRef<ClusterDnsResolverCache> 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<ClusterDnsResolverCache> dns_resolver_cache = {});
 
   /**
    * Create a dns resolver to be used by the cluster.

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<CdsApiPtr>

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_;
 };