dns: preserve failures in dual-family resolution

changelogs/current/bug_fixes/dns_resolver__dual_resolution_failure.rst

@@ -0,0 +1,4 @@
+Fixed the c-ares resolver reporting a successful empty result when one address family returned no records
+and the other lookup failed in ``AUTO`` or ``V4_PREFERRED`` mode. Such failures no longer remove existing
+hosts from strict DNS clusters. This change can be reverted by setting the runtime guard
+``envoy.reloadable_features.cares_dual_resolution_preserve_failure`` to ``false``.

docs/root/intro/arch_overview/upstream/dns_resolution.rst

@@ -31,6 +31,12 @@ Envoy contains 4 built-in DNS resolver extensions:
 For an example of a built-in DNS typed configuration, see the
 :ref:`HTTP filter configuration documentation <config_http_filters_dynamic_forward_proxy>`.
 
+With the c-ares resolver, ``AUTO`` and ``V4_PREFERRED`` lookups try the other address family when
+the preferred family returns no addresses. If neither lookup provides addresses and a lookup
+fails, the resolution fails even if the other family reports no records. Strict DNS clusters
+retain their existing hosts on resolution failure. A successful fallback still returns its
+addresses, and lookups that both report no records return a successful empty result.
+
 The c-ares-based DNS resolver emits the following statistics rooted in the ``dns.cares`` stats tree:
 
   .. csv-table::

source/common/runtime/runtime_features.cc

@@ -35,6 +35,7 @@
 // ASAP by filing a bug on github. Overriding non-buggy code is strongly discouraged to avoid the
 // problem of the bugs being found after the old code path has been removed.
 RUNTIME_GUARD(envoy_reloadable_features_async_host_selection);
+RUNTIME_GUARD(envoy_reloadable_features_cares_dual_resolution_preserve_failure);
 RUNTIME_GUARD(envoy_reloadable_features_cel_message_serialize_text_format);
 RUNTIME_GUARD(envoy_reloadable_features_coalesce_formatter_accept_empty_values);
 RUNTIME_GUARD(envoy_reloadable_features_coalesce_lb_rebuilds_on_batch_update);

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

@@ -228,6 +228,10 @@ void DnsResolverImpl::AddrInfoPendingResolution::onAresGetAddrInfoCallback(
     parent_.chargeGetAddrInfoErrorStats(status, timeouts);
 
     if (!isResponseWithNoRecords(status)) {
+      // A family deliberately skipped by filter_unroutable_families is not a DNS failure.
+      if (status != ARES_EBADFAMILY) {
+        had_resolution_failure_ = true;
+      }
       ENVOY_LOG_EVENT(debug, "cares_resolution_failure",
                       "dns resolution for {} failed with c-ares status {:#06x}: \"{}\"", dns_name_,
                       status, ares_strerror(status));
@@ -361,6 +365,12 @@ void DnsResolverImpl::AddrInfoPendingResolution::onAresGetAddrInfoCallback(
 }
 
 void DnsResolverImpl::PendingResolution::finishResolve() {
+  if (had_resolution_failure_ && pending_response_.address_list_.empty() &&
+      Runtime::runtimeFeatureEnabled(
+          "envoy.reloadable_features.cares_dual_resolution_preserve_failure")) {
+    pending_response_.status_ = ResolutionStatus::Failure;
+  }
+
   ENVOY_LOG_EVENT(trace, "cares_dns_resolution_complete",
                   "dns resolution for {} completed with status {:#06x}: \"{}\"", dns_name_,
                   static_cast<int>(pending_response_.status_), pending_response_.details_);

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

@@ -111,11 +111,10 @@ class DnsResolverImpl : public DnsResolver, protected Logger::Loggable<Logger::I
       std::string details_{"not_set"};
     };
 
-    // Note: pending_response_ is constructed with ResolutionStatus::Failure by default and
-    // __only__ changed to ResolutionStatus::Completed if there is an `ARES_SUCCESS`
-    // or `ARES_ENODATA` or `ARES_ENOTFOUND`reply. In the dual_resolution case __any__ ARES_SUCCESS
-    // reply will result in a ResolutionStatus::Completed callback.
+    // A successful lookup can supply addresses even if another lookup failed. Without addresses,
+    // however, an empty response must not hide a failure from the other address family.
     PendingResponse pending_response_{ResolutionStatus::Failure, {}};
+    bool had_resolution_failure_{false};
   };
 
   class AddrInfoPendingResolution final : public PendingResolution {

test/extensions/network/dns_resolver/cares/dns_impl_test.cc

@@ -1734,7 +1734,7 @@ TEST_P(DnsImplTest, WithBothAAndAAAARecord) {
              0 /*get_addr_failure*/, 0 /*timeouts*/, 0 /*reinitializations*/);
 }
 
-TEST_P(DnsImplTest, FallbackToNodataWithErrorOnA) {
+TEST_P(DnsImplTest, NoRecordsDoNotMaskErrorOnA) {
   server_->setErrorOnQtypeA(true);
   EXPECT_NE(nullptr,
             resolveWithExpectations("some.good.domain", DnsLookupFamily::V4Only,
@@ -1748,13 +1748,16 @@ TEST_P(DnsImplTest, FallbackToNodataWithErrorOnA) {
   checkStats(2 /*resolve_total*/, 0 /*pending_resolutions*/, 1 /*not_found*/,
              1 /*get_addr_failure*/, 0 /*timeouts*/, 0 /*reinitializations*/);
 
-  EXPECT_NE(nullptr, resolveWithNoRecordsExpectation("some.good.domain", DnsLookupFamily::Auto));
+  EXPECT_NE(nullptr,
+            resolveWithExpectations("some.good.domain", DnsLookupFamily::Auto,
+                                    DnsResolver::ResolutionStatus::Failure, {}, {}, std::nullopt));
   dispatcher_->run(Event::Dispatcher::RunType::Block);
   checkStats(4 /*resolve_total*/, 0 /*pending_resolutions*/, 2 /*not_found*/,
              2 /*get_addr_failure*/, 0 /*timeouts*/, 0 /*reinitializations*/);
 
   EXPECT_NE(nullptr,
-            resolveWithNoRecordsExpectation("some.good.domain", DnsLookupFamily::V4Preferred));
+            resolveWithExpectations("some.good.domain", DnsLookupFamily::V4Preferred,
+                                    DnsResolver::ResolutionStatus::Failure, {}, {}, std::nullopt));
   dispatcher_->run(Event::Dispatcher::RunType::Block);
   checkStats(6 /*resolve_total*/, 0 /*pending_resolutions*/, 3 /*not_found*/,
              3 /*get_addr_failure*/, 0 /*timeouts*/, 0 /*reinitializations*/);
@@ -1765,7 +1768,7 @@ TEST_P(DnsImplTest, FallbackToNodataWithErrorOnA) {
              3 /*get_addr_failure*/, 0 /*timeouts*/, 0 /*reinitializations*/);
 }
 
-TEST_P(DnsImplTest, FallbackToNodataWithErrorOnAAAA) {
+TEST_P(DnsImplTest, NoRecordsDoNotMaskErrorOnAAAA) {
   server_->setErrorOnQtypeAAAA(true);
   EXPECT_NE(nullptr,
             resolveWithExpectations("some.good.domain", DnsLookupFamily::V6Only,
@@ -1779,13 +1782,16 @@ TEST_P(DnsImplTest, FallbackToNodataWithErrorOnAAAA) {
   checkStats(2 /*resolve_total*/, 0 /*pending_resolutions*/, 1 /*not_found*/,
              1 /*get_addr_failure*/, 0 /*timeouts*/, 0 /*reinitializations*/);
 
-  EXPECT_NE(nullptr, resolveWithNoRecordsExpectation("some.good.domain", DnsLookupFamily::Auto));
+  EXPECT_NE(nullptr,
+            resolveWithExpectations("some.good.domain", DnsLookupFamily::Auto,
+                                    DnsResolver::ResolutionStatus::Failure, {}, {}, std::nullopt));
   dispatcher_->run(Event::Dispatcher::RunType::Block);
   checkStats(4 /*resolve_total*/, 0 /*pending_resolutions*/, 2 /*not_found*/,
              2 /*get_addr_failure*/, 0 /*timeouts*/, 0 /*reinitializations*/);
 
   EXPECT_NE(nullptr,
-            resolveWithNoRecordsExpectation("some.good.domain", DnsLookupFamily::V4Preferred));
+            resolveWithExpectations("some.good.domain", DnsLookupFamily::V4Preferred,
+                                    DnsResolver::ResolutionStatus::Failure, {}, {}, std::nullopt));
   dispatcher_->run(Event::Dispatcher::RunType::Block);
   checkStats(6 /*resolve_total*/, 0 /*pending_resolutions*/, 3 /*not_found*/,
              3 /*get_addr_failure*/, 0 /*timeouts*/, 0 /*reinitializations*/);
@@ -1801,6 +1807,39 @@ TEST_P(DnsImplTest, FallbackToNodataWithErrorOnAAAA) {
              4 /*get_addr_failure*/, 0 /*timeouts*/, 0 /*reinitializations*/);
 }
 
+TEST_P(DnsImplTest, SuccessfulFallbackAfterError) {
+  server_->addHosts("some.good.domain", {"201.134.56.7"}, RecordType::A);
+  server_->addHosts("some.good.domain", {"1::2"}, RecordType::AAAA);
+
+  for (const auto family : {DnsLookupFamily::Auto, DnsLookupFamily::V4Preferred}) {
+    SCOPED_TRACE(static_cast<int>(family));
+    server_->setErrorOnQtypeA(family == DnsLookupFamily::V4Preferred);
+    server_->setErrorOnQtypeAAAA(family == DnsLookupFamily::Auto);
+    EXPECT_NE(nullptr,
+              resolveWithExpectations(
+                  "some.good.domain", family, DnsResolver::ResolutionStatus::Completed,
+                  family == DnsLookupFamily::Auto ? IpList{"201.134.56.7"} : IpList{"1::2"}, {},
+                  std::nullopt));
+    dispatcher_->run(Event::Dispatcher::RunType::Block);
+  }
+}
+
+// With the runtime guard disabled, a no-records response from one family masks a failure from
+// the other family and the resolution completes with an empty result.
+TEST_P(DnsImplTest, NoRecordsMaskErrorWithRuntimeGuardDisabled) {
+  TestScopedRuntime scoped_runtime;
+  scoped_runtime.mergeValues(
+      {{"envoy.reloadable_features.cares_dual_resolution_preserve_failure", "false"}});
+  server_->setErrorOnQtypeA(true);
+
+  // Auto sees no records on AAAA before the A error, while V4Preferred sees the A error first.
+  for (const auto family : {DnsLookupFamily::Auto, DnsLookupFamily::V4Preferred}) {
+    SCOPED_TRACE(static_cast<int>(family));
+    EXPECT_NE(nullptr, resolveWithNoRecordsExpectation("some.good.domain", family));
+    dispatcher_->run(Event::Dispatcher::RunType::Block);
+  }
+}
+
 TEST_P(DnsImplTest, ErrorWithAcceptNodataEnabled) {
   server_->setErrorOnQtypeA(true);
   server_->setErrorOnQtypeAAAA(true);