dns resolvers: add All lookup mode

api/envoy/config/cluster/v3/cluster.proto

@@ -129,6 +129,8 @@ message Cluster {
   // If V4_PREFERRED is specified, the DNS resolver will first perform a lookup for addresses in the
   // IPv4 family and fallback to a lookup for addresses in the IPv6 family. i.e., the callback
   // target will only get v6 addresses if there were NO v4 addresses to return.
+  // If ALL is specified, the DNS resolver will perform a lookup for both IPv4 and IPv6 families,
+  // and return all resolved addresses.
   // For cluster types other than
   // :ref:`STRICT_DNS<envoy_v3_api_enum_value_config.cluster.v3.Cluster.DiscoveryType.STRICT_DNS>` and
   // :ref:`LOGICAL_DNS<envoy_v3_api_enum_value_config.cluster.v3.Cluster.DiscoveryType.LOGICAL_DNS>`,
@@ -140,6 +142,7 @@ message Cluster {
     V4_ONLY = 1;
     V6_ONLY = 2;
     V4_PREFERRED = 3;
+    ALL = 4;
   }
 
   enum ClusterProtocolSelection {

docs/root/version_history/current.rst

@@ -43,6 +43,7 @@ New Features
 * bootstrap: added :ref:`typed_dns_resolver_config <envoy_v3_api_field_config.bootstrap.v3.Bootstrap.typed_dns_resolver_config>` in the bootstrap to support DNS resolver as an extension.
 * cluster: added :ref:`typed_dns_resolver_config <envoy_v3_api_field_config.cluster.v3.Cluster.typed_dns_resolver_config>` in the cluster to support DNS resolver as an extension.
 * config: added :ref:`environment_variable <envoy_v3_api_field_config.core.v3.datasource.environment_variable>` to the :ref:`DataSource <envoy_v3_api_msg_config.core.v3.datasource>`.
+* dns: added :ref:`ALL <envoy_v3_api_enum_value_config.cluster.v3.Cluster.DnsLookupFamily.ALL>` option to return both IPv4 and IPv6 addresses.
 * dns_cache: added :ref:`typed_dns_resolver_config <envoy_v3_api_field_extensions.common.dynamic_forward_proxy.v3.DnsCacheConfig.typed_dns_resolver_config>` in the dns_cache to support DNS resolver as an extension.
 * dns_filter: added :ref:`typed_dns_resolver_config <envoy_v3_api_field_extensions.filters.udp.dns_filter.v3.DnsFilterConfig.ClientContextConfig.typed_dns_resolver_config>` in the dns_filter to support DNS resolver as an extension.
 * dns_resolver: added :ref:`CaresDnsResolverConfig<envoy_v3_api_msg_extensions.network.dns_resolver.cares.v3.CaresDnsResolverConfig>` to support c-ares DNS resolver as an extension.

envoy/network/dns.h

@@ -46,7 +46,7 @@ struct DnsResponse {
   const std::chrono::seconds ttl_;
 };
 
-enum class DnsLookupFamily { V4Only, V6Only, Auto, V4Preferred };
+enum class DnsLookupFamily { V4Only, V6Only, Auto, V4Preferred, All };
 
 /**
  * An asynchronous DNS resolver.

source/common/upstream/upstream_impl.cc

@@ -1777,6 +1777,8 @@ getDnsLookupFamilyFromEnum(envoy::config::cluster::v3::Cluster::DnsLookupFamily
     return Network::DnsLookupFamily::Auto;
   case envoy::config::cluster::v3::Cluster::V4_PREFERRED:
     return Network::DnsLookupFamily::V4Preferred;
+  case envoy::config::cluster::v3::Cluster::ALL:
+    return Network::DnsLookupFamily::All;
   default:
     NOT_REACHED_GCOVR_EXCL_LINE;
   }

source/extensions/network/dns_resolver/apple/apple_dns_impl.cc

@@ -141,7 +141,8 @@ AppleDnsResolverImpl::PendingResolution::PendingResolution(AppleDnsResolverImpl&
                                                            const std::string& dns_name,
                                                            DnsLookupFamily dns_lookup_family)
     : parent_(parent), callback_(callback), dispatcher_(dispatcher), dns_name_(dns_name),
-      pending_cb_({ResolutionStatus::Success, {}, {}}), dns_lookup_family_(dns_lookup_family) {}
+      pending_response_({ResolutionStatus::Success, {}, {}, {}}),
+      dns_lookup_family_(dns_lookup_family) {}
 
 AppleDnsResolverImpl::PendingResolution::~PendingResolution() {
   ENVOY_LOG(debug, "Destroying PendingResolution for {}", dns_name_);
@@ -183,37 +184,47 @@ void AppleDnsResolverImpl::PendingResolution::onEventCallback(uint32_t events) {
     // Similar to receiving an error in onDNSServiceGetAddrInfoReply, an error while processing fd
     // events indicates that the sd_ref state is broken.
     // Therefore, finish resolving with an error.
-    pending_cb_.status_ = ResolutionStatus::Failure;
+    pending_response_.status_ = ResolutionStatus::Failure;
     finishResolve();
   }
 }
 
 std::list<DnsResponse>& AppleDnsResolverImpl::PendingResolution::finalAddressList() {
   switch (dns_lookup_family_) {
   case DnsLookupFamily::V4Only:
-    return pending_cb_.v4_responses_;
+    return pending_response_.v4_responses_;
   case DnsLookupFamily::V6Only:
-    return pending_cb_.v6_responses_;
+    return pending_response_.v6_responses_;
   case DnsLookupFamily::Auto:
     // Per API docs only give v4 if v6 is not available.
-    if (pending_cb_.v6_responses_.empty()) {
-      return pending_cb_.v4_responses_;
+    if (pending_response_.v6_responses_.empty()) {
+      return pending_response_.v4_responses_;
     }
-    return pending_cb_.v6_responses_;
+    return pending_response_.v6_responses_;
   case DnsLookupFamily::V4Preferred:
     // Per API docs only give v6 if v4 is not available.
-    if (pending_cb_.v4_responses_.empty()) {
-      return pending_cb_.v6_responses_;
+    if (pending_response_.v4_responses_.empty()) {
+      return pending_response_.v6_responses_;
     }
-    return pending_cb_.v4_responses_;
+    return pending_response_.v4_responses_;
+  case DnsLookupFamily::All:
+    ASSERT(pending_response_.all_responses_.empty());
+    pending_response_.all_responses_.insert(pending_response_.all_responses_.end(),
+                                            pending_response_.v4_responses_.begin(),
+                                            pending_response_.v4_responses_.end());
+    pending_response_.all_responses_.insert(pending_response_.all_responses_.end(),
+                                            pending_response_.v6_responses_.begin(),
+                                            pending_response_.v6_responses_.end());
+    return pending_response_.all_responses_;
   }
   NOT_REACHED_GCOVR_EXCL_LINE;
 }
 
 void AppleDnsResolverImpl::PendingResolution::finishResolve() {
   ENVOY_LOG_EVENT(debug, "apple_dns_resolution_complete",
-                  "dns resolution for {} completed with status {}", dns_name_, pending_cb_.status_);
-  callback_(pending_cb_.status_, std::move(finalAddressList()));
+                  "dns resolution for {} completed with status {}", dns_name_,
+                  pending_response_.status_);
+  callback_(pending_response_.status_, std::move(finalAddressList()));
 
   if (owned_) {
     ENVOY_LOG(debug, "Resolution for {} completed (async)", dns_name_);
@@ -235,6 +246,7 @@ DNSServiceErrorType AppleDnsResolverImpl::PendingResolution::dnsServiceGetAddrIn
     break;
   case DnsLookupFamily::Auto:
   case DnsLookupFamily::V4Preferred:
+  case DnsLookupFamily::All:
     /* We want to make sure we don't get any address that is not routable. Passing 0
      * to apple's `DNSServiceGetAddrInfo` will make a best attempt to filter out IPv6
      * or IPv4 addresses depending on what's routable, per Apple's documentation:
@@ -291,9 +303,9 @@ void AppleDnsResolverImpl::PendingResolution::onDNSServiceGetAddrInfoReply(
   if (error_code != kDNSServiceErr_NoError) {
     parent_.chargeGetAddrInfoErrorStats(error_code);
 
-    pending_cb_.status_ = ResolutionStatus::Failure;
-    pending_cb_.v4_responses_.clear();
-    pending_cb_.v6_responses_.clear();
+    pending_response_.status_ = ResolutionStatus::Failure;
+    pending_response_.v4_responses_.clear();
+    pending_response_.v6_responses_.clear();
 
     finishResolve();
     // Note: Nothing can follow this call to flushPendingQueries due to deletion of this
@@ -310,10 +322,10 @@ void AppleDnsResolverImpl::PendingResolution::onDNSServiceGetAddrInfoReply(
     ENVOY_LOG(debug, "Address to add address={}, ttl={}",
               dns_response.address_->ip()->addressAsString(), ttl);
     if (dns_response.address_->ip()->ipv4()) {
-      pending_cb_.v4_responses_.push_back(dns_response);
+      pending_response_.v4_responses_.push_back(dns_response);
     } else {
       ASSERT(dns_response.address_->ip()->ipv6());
-      pending_cb_.v6_responses_.push_back(dns_response);
+      pending_response_.v6_responses_.push_back(dns_response);
     }
   }
 

source/extensions/network/dns_resolver/apple/apple_dns_impl.h

@@ -109,10 +109,11 @@ class AppleDnsResolverImpl : public DnsResolver, protected Logger::Loggable<Logg
 
     // Small wrapping struct to accumulate addresses from firings of the
     // onDNSServiceGetAddrInfoReply callback.
-    struct FinalResponse {
+    struct PendingResponse {
       ResolutionStatus status_;
       std::list<DnsResponse> v4_responses_;
       std::list<DnsResponse> v6_responses_;
+      std::list<DnsResponse> all_responses_;
     };
 
     AppleDnsResolverImpl& parent_;
@@ -128,7 +129,7 @@ class AppleDnsResolverImpl : public DnsResolver, protected Logger::Loggable<Logg
     // DNSServiceGetAddrInfo fires one callback DNSServiceGetAddrInfoReply callback per IP address,
     // and informs via flags if more IP addresses are incoming. Therefore, these addresses need to
     // be accumulated before firing callback_.
-    FinalResponse pending_cb_;
+    PendingResponse pending_response_;
     DnsLookupFamily dns_lookup_family_;
   };
 

source/extensions/network/dns_resolver/cares/dns_impl.cc

@@ -107,16 +107,16 @@ void DnsResolverImpl::PendingResolution::onAresGetAddrInfoCallback(int status, i
     // ARES_ECONNREFUSED. If the PendingResolution has not been cancelled that means that the
     // callback_ target _should_ still be around. In that case, raise the callback_ so the target
     // can be done with this query and initiate a new one.
-    if (!cancelled_) {
-      ENVOY_LOG_EVENT(debug, "cares_dns_resolution_destroyed", "dns resolution for {} destroyed",
-                      dns_name_);
+    ENVOY_LOG_EVENT(debug, "cares_dns_resolution_destroyed", "dns resolution for {} destroyed",
+                    dns_name_);
 
-      callback_(ResolutionStatus::Failure, {});
-    }
-    delete this;
+    // Nothing can follow a call to finishResolve due to the deletion of this object upon
+    // finishResolve().
+    finishResolve();
     return;
   }
-  if (!fallback_if_failed_) {
+
+  if (!dual_resolution_) {
     completed_ = true;
 
     // If c-ares returns ARES_ECONNREFUSED and there is no fallback we assume that the channel_ is
@@ -133,10 +133,9 @@ void DnsResolverImpl::PendingResolution::onAresGetAddrInfoCallback(int status, i
     }
   }
 
-  std::list<DnsResponse> address_list;
-  ResolutionStatus resolution_status;
   if (status == ARES_SUCCESS) {
-    resolution_status = ResolutionStatus::Success;
+    pending_response_.status_ = ResolutionStatus::Success;
+
     if (addrinfo != nullptr && addrinfo->nodes != nullptr) {
       if (addrinfo->nodes->ai_family == AF_INET) {
         for (const ares_addrinfo_node* ai = addrinfo->nodes; ai != nullptr; ai = ai->ai_next) {
@@ -146,7 +145,7 @@ void DnsResolverImpl::PendingResolution::onAresGetAddrInfoCallback(int status, i
           address.sin_port = 0;
           address.sin_addr = reinterpret_cast<sockaddr_in*>(ai->ai_addr)->sin_addr;
 
-          address_list.emplace_back(
+          pending_response_.address_list_.emplace_back(
               DnsResponse(std::make_shared<const Address::Ipv4Instance>(&address),
                           std::chrono::seconds(ai->ai_ttl)));
         }
@@ -157,66 +156,41 @@ void DnsResolverImpl::PendingResolution::onAresGetAddrInfoCallback(int status, i
           address.sin6_family = AF_INET6;
           address.sin6_port = 0;
           address.sin6_addr = reinterpret_cast<sockaddr_in6*>(ai->ai_addr)->sin6_addr;
-          address_list.emplace_back(
+          pending_response_.address_list_.emplace_back(
               DnsResponse(std::make_shared<const Address::Ipv6Instance>(address),
                           std::chrono::seconds(ai->ai_ttl)));
         }
       }
     }
 
-    if (!address_list.empty()) {
+    if (!pending_response_.address_list_.empty() && dns_lookup_family_ != DnsLookupFamily::All) {
       completed_ = true;
     }
 
     ASSERT(addrinfo != nullptr);
     ares_freeaddrinfo(addrinfo);
-  } else {
-    resolution_status = ResolutionStatus::Failure;
   }
 
   if (timeouts > 0) {
     ENVOY_LOG(debug, "DNS request timed out {} times", timeouts);
   }
 
   if (completed_) {
-    if (!cancelled_) {
-      // Use a raw try here because it is used in both main thread and filter.
-      // Can not convert to use status code as there may be unexpected exceptions in server fuzz
-      // tests, which must be handled. Potential exception may come from getAddressWithPort() or
-      // portFromTcpUrl().
-      // TODO(chaoqin-li1123): remove try catch pattern here once we figure how to handle unexpected
-      // exception in fuzz tests.
-      ENVOY_LOG_EVENT(debug, "cares_dns_resolution_complete",
-                      "dns resolution for {} completed with status {}", dns_name_,
-                      resolution_status);
-
-      TRY_NEEDS_AUDIT { callback_(resolution_status, std::move(address_list)); }
-      catch (const EnvoyException& e) {
-        ENVOY_LOG(critical, "EnvoyException in c-ares callback: {}", e.what());
-        dispatcher_.post([s = std::string(e.what())] { throw EnvoyException(s); });
-      }
-      catch (const std::exception& e) {
-        ENVOY_LOG(critical, "std::exception in c-ares callback: {}", e.what());
-        dispatcher_.post([s = std::string(e.what())] { throw EnvoyException(s); });
-      }
-      catch (...) {
-        ENVOY_LOG(critical, "Unknown exception in c-ares callback");
-        dispatcher_.post([] { throw EnvoyException("unknown"); });
-      }
-    }
-    if (owned_) {
-      delete this;
-      return;
-    }
+    finishResolve();
+    // Nothing can follow a call to finishResolve due to the deletion of this object upon
+    // finishResolve().
+    return;
   }
 
-  if (!completed_ && fallback_if_failed_) {
-    fallback_if_failed_ = false;
+  if (dual_resolution_) {
+    dual_resolution_ = false;
 
+    // Perform a second lookup for DnsLookupFamily::Auto and DnsLookupFamily::V4Preferred, given
+    // that the first lookup failed to return any addresses. Note that DnsLookupFamily::All issues
+    // both lookups concurrently so there is no need to fire a second lookup here.
     if (dns_lookup_family_ == DnsLookupFamily::Auto) {
       getAddrInfo(AF_INET);
-    } else {
-      ASSERT(dns_lookup_family_ == DnsLookupFamily::V4Preferred);
+    } else if (dns_lookup_family_ == DnsLookupFamily::V4Preferred) {
       getAddrInfo(AF_INET6);
     }
 
@@ -226,6 +200,40 @@ void DnsResolverImpl::PendingResolution::onAresGetAddrInfoCallback(int status, i
   }
 }
 
+void DnsResolverImpl::PendingResolution::finishResolve() {
+  if (!cancelled_) {
+    // Use a raw try here because it is used in both main thread and filter.
+    // Can not convert to use status code as there may be unexpected exceptions in server fuzz
+    // tests, which must be handled. Potential exception may come from getAddressWithPort() or
+    // portFromTcpUrl().
+    // TODO(chaoqin-li1123): remove try catch pattern here once we figure how to handle unexpected
+    // exception in fuzz tests.
+    ENVOY_LOG_EVENT(debug, "cares_dns_resolution_complete",
+                    "dns resolution for {} completed with status {}", dns_name_,
+                    pending_response_.status_);
+
+    TRY_NEEDS_AUDIT {
+      callback_(pending_response_.status_, std::move(pending_response_.address_list_));
+    }
+    catch (const EnvoyException& e) {
+      ENVOY_LOG(critical, "EnvoyException in c-ares callback: {}", e.what());
+      dispatcher_.post([s = std::string(e.what())] { throw EnvoyException(s); });
+    }
+    catch (const std::exception& e) {
+      ENVOY_LOG(critical, "std::exception in c-ares callback: {}", e.what());
+      dispatcher_.post([s = std::string(e.what())] { throw EnvoyException(s); });
+    }
+    catch (...) {
+      ENVOY_LOG(critical, "Unknown exception in c-ares callback");
+      dispatcher_.post([] { throw EnvoyException("unknown"); });
+    }
+  }
+  if (owned_) {
+    delete this;
+    return;
+  }
+}
+
 void DnsResolverImpl::updateAresTimer() {
   // Update the timeout for events.
   timeval timeout;
@@ -286,15 +294,28 @@ ActiveDnsQuery* DnsResolverImpl::resolve(const std::string& dns_name,
   auto pending_resolution = std::make_unique<PendingResolution>(
       *this, callback, dispatcher_, channel_, dns_name, dns_lookup_family);
   if (dns_lookup_family == DnsLookupFamily::Auto ||
-      dns_lookup_family == DnsLookupFamily::V4Preferred) {
-    pending_resolution->fallback_if_failed_ = true;
+      dns_lookup_family == DnsLookupFamily::V4Preferred ||
+      dns_lookup_family == DnsLookupFamily::All) {
+    pending_resolution->dual_resolution_ = true;
   }
 
-  if (dns_lookup_family == DnsLookupFamily::V4Only ||
-      dns_lookup_family == DnsLookupFamily::V4Preferred) {
+  switch (dns_lookup_family) {
+  case DnsLookupFamily::V4Only:
+  case DnsLookupFamily::V4Preferred:
+    pending_resolution->getAddrInfo(AF_INET);
+    break;
+  case DnsLookupFamily::V6Only:
+  case DnsLookupFamily::Auto:
+    pending_resolution->getAddrInfo(AF_INET6);
+    break;
+  // NOTE: DnsLookupFamily::All performs both lookups concurrently as addresses from both families
+  // are being requested.
+  case DnsLookupFamily::All:
     pending_resolution->getAddrInfo(AF_INET);
-  } else {
     pending_resolution->getAddrInfo(AF_INET6);
+    break;
+  default:
+    NOT_REACHED_GCOVR_EXCL_LINE;
   }
 
   if (pending_resolution->completed_) {