dns/apple: Ensure the Apple DNS resolver gets all IPs for the host

If the DNS query protocol to the Apple DNS resolution API `DNSServiceGetAddrInfo` is
either IPv4 | IPv6, or if protocol is set to 0, the resolution callback will be
invoked at least twice: at least once for the IPv4 family and at least once for the
IPv6 family.

Previously, we were not waiting to ensure we received both the IPv4 and the IPv6
response (even if empty), which meant the resolver would finish a DNS resolution
without necessarily getting all the responses from the `DNSServiceGetAddrInfo` call.

If the resolver finished while getting an IPv6 address, but not yet having received
an IPv4 address for the host, and the device is on a network that doesn't support
IPv6, then connections made to the host would all result in a "no route to host"
error.

This PR fixes the problem by ensuring we don't finish the DNS query resolution until
all responses (for both v4 and v6, if requested) have been received in the
resolution response callback.

Signed-off-by: Ali Beyad <abeyad@google.com>

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

@@ -145,7 +145,7 @@ AppleDnsResolverImpl::PendingResolution::PendingResolution(AppleDnsResolverImpl&
                                                            const std::string& dns_name,
                                                            DnsLookupFamily dns_lookup_family)
     : parent_(parent), callback_(callback), dispatcher_(dispatcher), dns_name_(dns_name),
-      pending_response_({ResolutionStatus::Success, {}, {}, {}}),
+      pending_response_({ResolutionStatus::Success, false, false, {}, {}, {}}),
       dns_lookup_family_(dns_lookup_family) {}
 
 AppleDnsResolverImpl::PendingResolution::~PendingResolution() {
@@ -242,19 +242,18 @@ void AppleDnsResolverImpl::PendingResolution::finishResolve() {
 
 DNSServiceErrorType
 AppleDnsResolverImpl::PendingResolution::dnsServiceGetAddrInfo(bool include_unroutable_families) {
-  DNSServiceProtocol protocol;
   switch (dns_lookup_family_) {
   case DnsLookupFamily::V4Only:
-    protocol = kDNSServiceProtocol_IPv4;
+    query_protocol_ = kDNSServiceProtocol_IPv4;
     break;
   case DnsLookupFamily::V6Only:
-    protocol = kDNSServiceProtocol_IPv6;
+    query_protocol_ = kDNSServiceProtocol_IPv6;
     break;
   case DnsLookupFamily::Auto:
   case DnsLookupFamily::V4Preferred:
   case DnsLookupFamily::All:
     if (include_unroutable_families) {
-      protocol = kDNSServiceProtocol_IPv4 | kDNSServiceProtocol_IPv6;
+      query_protocol_ = kDNSServiceProtocol_IPv4 | kDNSServiceProtocol_IPv6;
       break;
     }
 
@@ -270,15 +269,16 @@ AppleDnsResolverImpl::PendingResolution::dnsServiceGetAddrInfo(bool include_unro
      * any use anyway. Similarly, if this host has no routable IPv4 address, the call will
      * not try to look up IPv4 addresses for "hostname".
      */
-    protocol = 0;
+    query_protocol_ = 0;
     break;
   }
 
   // TODO: explore caching: there are caching flags in the dns_sd.h flags, allow expired answers
   // from the cache?
   // TODO: explore validation via `DNSSEC`?
   return DnsServiceSingleton::get().dnsServiceGetAddrInfo(
-      &sd_ref_, kDNSServiceFlagsTimeout, 0, protocol, dns_name_.c_str(),
+      &sd_ref_, kDNSServiceFlagsTimeout | kDNSServiceFlagsReturnIntermediates, 0, query_protocol_,
+      dns_name_.c_str(),
       /*
        * About Thread Safety (taken from inline documentation there):
        * The dns_sd.h API does not presuppose any particular threading model, and consequently
@@ -304,14 +304,27 @@ AppleDnsResolverImpl::PendingResolution::dnsServiceGetAddrInfo(bool include_unro
 void AppleDnsResolverImpl::PendingResolution::onDNSServiceGetAddrInfoReply(
     DNSServiceFlags flags, uint32_t interface_index, DNSServiceErrorType error_code,
     const char* hostname, const struct sockaddr* address, uint32_t ttl) {
+  // If the DNS query protocol is (kDNSServiceProtocol_IPv4 | kDNSServiceProtocol_IPv6) or if it is
+  // 0, then this callback is expected to be called at least two times: at least once for IPv4 and
+  // at least once for IPv6. This is true even if there are no DNS records for the given address
+  // family and/or the network that the code is running on doesn't support the given address family.
+  //
+  // That means if the network doesn't support an address family or the hostname doesn't have any
+  // DNS records for the address family, there will still be at least one callback to
+  // onDNSServiceGetAddrInfoReply() for requested address family. In such a case, the `address` will
+  // still be non-null and its `sa_family` will be the address family of the query (even if the
+  // address itself isn't a meaningful IP address).
+
   ENVOY_LOG(debug,
             "DNS for {} resolved with: flags={}[MoreComing={}, Add={}], interface_index={}, "
             "error_code={}, hostname={}",
             dns_name_, flags, flags & kDNSServiceFlagsMoreComing ? "yes" : "no",
             flags & kDNSServiceFlagsAdd ? "yes" : "no", interface_index, error_code, hostname);
 
   // Make sure that we trigger the failure callback if we get an error back.
-  if (error_code != kDNSServiceErr_NoError) {
+  // NoSuchRecord is *not* considered an error; it indicates that a query was successfully
+  // completed, but there were no DNS records for that address family.
+  if (error_code != kDNSServiceErr_NoError && error_code != kDNSServiceErr_NoSuchRecord) {
     parent_.chargeGetAddrInfoErrorStats(error_code);
 
     pending_response_.status_ = ResolutionStatus::Failure;
@@ -324,11 +337,17 @@ void AppleDnsResolverImpl::PendingResolution::onDNSServiceGetAddrInfoReply(
     return;
   }
 
+  ASSERT(address, "address cannot be null");
+  if (address->sa_family == AF_INET) {
+    pending_response_.v4_response_received_ = true;
+  } else if (address->sa_family == AF_INET6) {
+    pending_response_.v6_response_received_ = true;
+  }
+
   // dns_sd.h does not call out behavior where callbacks to DNSServiceGetAddrInfoReply
   // would respond without the flag. However, Envoy's API is solely additive.
   // Therefore, only add this address to the list if kDNSServiceFlagsAdd is set.
-  if (flags & kDNSServiceFlagsAdd) {
-    ASSERT(address, "invalid to add null address");
+  if (error_code == kDNSServiceErr_NoError && (flags & kDNSServiceFlagsAdd)) {
     auto dns_response = buildDnsResponse(address, ttl);
     ENVOY_LOG(debug, "Address to add address={}, ttl={}",
               dns_response.addrInfo().address_->ip()->addressAsString(), ttl);
@@ -340,7 +359,17 @@ void AppleDnsResolverImpl::PendingResolution::onDNSServiceGetAddrInfoReply(
     }
   }
 
-  if (!(flags & kDNSServiceFlagsMoreComing)) {
+  // If not expecting a v4 query, or the v4 response has been received, consider the v4 response
+  // complete.
+  const bool v4_response_complete =
+      !((query_protocol_ & kDNSServiceProtocol_IPv4) || query_protocol_ == 0) ||
+      pending_response_.v4_response_received_;
+  // If not expecting a v6 query, or the v4 response has been received, consider the v6 response
+  // complete.
+  const bool v6_response_complete =
+      !((query_protocol_ & kDNSServiceProtocol_IPv6) || query_protocol_ == 0) ||
+      pending_response_.v6_response_received_;
+  if (!(flags & kDNSServiceFlagsMoreComing) && v4_response_complete && v6_response_complete) {
     ENVOY_LOG(debug, "DNS Resolver flushing queries pending callback");
     finishResolve();
     // Note: Nothing can follow this call to finishResolve due to deletion of this

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

@@ -119,6 +119,8 @@ class AppleDnsResolverImpl : public DnsResolver, protected Logger::Loggable<Logg
     // onDNSServiceGetAddrInfoReply callback.
     struct PendingResponse {
       ResolutionStatus status_;
+      bool v4_response_received_;
+      bool v6_response_received_;
       std::list<DnsResponse> v4_responses_;
       std::list<DnsResponse> v6_responses_;
       std::list<DnsResponse> all_responses_;
@@ -131,6 +133,7 @@ class AppleDnsResolverImpl : public DnsResolver, protected Logger::Loggable<Logg
     Event::Dispatcher& dispatcher_;
     Event::FileEventPtr sd_ref_event_;
     DNSServiceRef sd_ref_{};
+    DNSServiceProtocol query_protocol_;
     const std::string dns_name_;
     bool synchronously_completed_{};
     bool owned_{};