getaddrinfo: Treat EAI_NONAME and EAI_NODATA as non-failures (envoypr…

…oxy#33934)

When the DNS resolution status is Failure, the DNS cache treats it as a retryable error, using a retry policy to enable a timer for retries. With EAI_NONAME and EAI_NODATA, we do not expect these to be transient failures, so the getaddrinfo now treats it as a succesful query with empty results, so we don't trigger the failure refresh/retry timer.

NOTE: The c-ares resolver also treats NODATA and NONAME results as successful queries.
Signed-off-by: Ali Beyad <[email protected]>

* todo

Signed-off-by: Ali Beyad <[email protected]>

---------

Signed-off-by: Ali Beyad <[email protected]>

changelogs/current.yaml

@@ -30,6 +30,12 @@ minor_behavior_changes:
 - area: statistics
   change: |
     Hot restart statistics like hot_restart_epoch are only set when hot restart is enabled.
+- area: dns
+  change: |
+    Changes the behavior of the getaddrinfo DNS resolver so that it treats EAI_NODATA and EAI_NONAME
+    as successful queries with empty results, instead of as DNS failures. This change brings the
+    getaddrinfo behavior in-line with the c-ares resolver behavior. This behavior can be reverted by
+    setting the runtime guard ``envoy.reloadable_features.dns_nodata_noname_is_success`` to false.
 
 bug_fixes:
 # *Changes expected to improve the state of the world and are unlikely to have negative effects*

envoy/network/dns.h

@@ -86,6 +86,9 @@ class DnsResolver {
 
   /**
    * Final status for a DNS resolution.
+   * TODO(abeyad): Rename `Success` to `Completed` or something similar. DNS resolution can return
+   * result statuses like NODATA and NONAME, which indicate successful completion of the query but
+   * no results, and `Completed` is a more accurate way of reflecting that.
    */
   enum class ResolutionStatus { Success, Failure };
 

source/common/runtime/runtime_features.cc

@@ -37,6 +37,7 @@ RUNTIME_GUARD(envoy_reloadable_features_defer_processing_backedup_streams);
 RUNTIME_GUARD(envoy_reloadable_features_dfp_mixed_scheme);
 RUNTIME_GUARD(envoy_reloadable_features_disallow_quic_client_udp_mmsg);
 RUNTIME_GUARD(envoy_reloadable_features_dns_cache_set_first_resolve_complete);
+RUNTIME_GUARD(envoy_reloadable_features_dns_nodata_noname_is_success);
 RUNTIME_GUARD(envoy_reloadable_features_dns_reresolve_on_eai_again);
 RUNTIME_GUARD(envoy_reloadable_features_edf_lb_host_scheduler_init_fix);
 RUNTIME_GUARD(envoy_reloadable_features_edf_lb_locality_scheduler_init_fix);

source/extensions/network/dns_resolver/getaddrinfo/getaddrinfo.cc

@@ -114,6 +114,8 @@ void GetAddrInfoDnsResolver::resolveThreadRoutine() {
     PendingQuerySharedPtr next_query;
     const bool reresolve =
         Runtime::runtimeFeatureEnabled("envoy.reloadable_features.dns_reresolve_on_eai_again");
+    const bool treat_nodata_noname_as_success =
+        Runtime::runtimeFeatureEnabled("envoy.reloadable_features.dns_nodata_noname_is_success");
     {
       absl::MutexLock guard(&mutex_);
       auto condition = [this]() ABSL_EXCLUSIVE_LOCKS_REQUIRED(mutex_) {
@@ -155,8 +157,16 @@ void GetAddrInfoDnsResolver::resolveThreadRoutine() {
         absl::MutexLock guard(&mutex_);
         pending_queries_.push_back(next_query);
         continue;
+      } else if (treat_nodata_noname_as_success &&
+                 (rc.return_value_ == EAI_NONAME || rc.return_value_ == EAI_NODATA)) {
+        // Treat NONAME and NODATA as DNS records with no results.
+        // NODATA and NONAME are typically not transient failures, so we don't expect success if
+        // the DNS query is retried.
+        // NOTE: this is also how the c-ares resolver treats NONAME and NODATA:
+        // https://github.com/envoyproxy/envoy/blob/099d85925b32ce8bf06e241ee433375a0a3d751b/source/extensions/network/dns_resolver/cares/dns_impl.h#L109-L111.
+        ENVOY_LOG(debug, "getaddrinfo no results rc={}", gai_strerror(rc.return_value_));
+        response = std::make_pair(ResolutionStatus::Success, std::list<DnsResponse>());
       } else {
-        // TODO(mattklein123): Handle some errors differently such as `EAI_NODATA`.
         ENVOY_LOG(debug, "getaddrinfo failed with rc={} errno={}", gai_strerror(rc.return_value_),
                   errorDetails(rc.errno_));
         response = std::make_pair(ResolutionStatus::Failure, std::list<DnsResponse>());

test/extensions/network/dns_resolver/getaddrinfo/getaddrinfo_test.cc

@@ -153,6 +153,40 @@ TEST_F(GetAddrInfoDnsImplTest, Failure) {
   dispatcher_->run(Event::Dispatcher::RunType::RunUntilExit);
 }
 
+TEST_F(GetAddrInfoDnsImplTest, NoData) {
+  TestThreadsafeSingletonInjector<Api::OsSysCallsImpl> os_calls(&os_sys_calls_);
+
+  EXPECT_CALL(os_sys_calls_, getaddrinfo(_, _, _, _))
+      .WillOnce(Return(Api::SysCallIntResult{EAI_NODATA, 0}));
+  resolver_->resolve(
+      "localhost", DnsLookupFamily::All,
+      [this](DnsResolver::ResolutionStatus status, std::list<DnsResponse>&& response) {
+        EXPECT_EQ(status, DnsResolver::ResolutionStatus::Success);
+        EXPECT_TRUE(response.empty());
+
+        dispatcher_->exit();
+      });
+
+  dispatcher_->run(Event::Dispatcher::RunType::RunUntilExit);
+}
+
+TEST_F(GetAddrInfoDnsImplTest, NoName) {
+  TestThreadsafeSingletonInjector<Api::OsSysCallsImpl> os_calls(&os_sys_calls_);
+
+  EXPECT_CALL(os_sys_calls_, getaddrinfo(_, _, _, _))
+      .WillOnce(Return(Api::SysCallIntResult{EAI_NONAME, 0}));
+  resolver_->resolve(
+      "localhost", DnsLookupFamily::All,
+      [this](DnsResolver::ResolutionStatus status, std::list<DnsResponse>&& response) {
+        EXPECT_EQ(status, DnsResolver::ResolutionStatus::Success);
+        EXPECT_TRUE(response.empty());
+
+        dispatcher_->exit();
+      });
+
+  dispatcher_->run(Event::Dispatcher::RunType::RunUntilExit);
+}
+
 TEST_F(GetAddrInfoDnsImplTest, TryAgainAndSuccess) {
   TestThreadsafeSingletonInjector<Api::OsSysCallsImpl> os_calls(&os_sys_calls_);
 

tools/spelling/spelling_dictionary.txt

@@ -301,9 +301,11 @@ NGHTTP
 NLOHMANN
 NOAUTH
 NOCHECKRESP
+NODATA
 NODELAY
 NOLINT
 NOLINTNEXTLINE
+NONAME
 NONBLOCK
 NONCES
 NOSORT