From a3f486a664c765f070085623556889dfd4c4b1c1 Mon Sep 17 00:00:00 2001
From: Alyssa Wilk <alyssar@chromium.org>
Date: Wed, 3 Apr 2024 15:48:23 -0400
Subject: [PATCH] Reapply "getaddrinfo: retrying on eai_again (#33130)"
 (#33159)

This reverts commit d59eb55358992485e159da54143fa23f1c5639fd.

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
---
 changelogs/current.yaml                       |  4 ++
 source/common/runtime/runtime_features.cc     |  1 +
 .../dns_resolver/getaddrinfo/getaddrinfo.cc   | 10 +++++
 .../getaddrinfo/getaddrinfo_test.cc           | 42 ++++++++++++++++++-
 4 files changed, 56 insertions(+), 1 deletion(-)

diff --git a/changelogs/current.yaml b/changelogs/current.yaml
index 5d42e1c3e803..eaf79745d8fc 100644
--- a/changelogs/current.yaml
+++ b/changelogs/current.yaml
@@ -34,6 +34,10 @@ minor_behavior_changes:
 - area: dns
   change: |
     Allowing <envoy_v3_api_field_extensions.common.dynamic_forward_proxy.v3.DnsCacheConfig.dns_min_refresh_rate>` to go as low as 1s.
+- area: dns
+  change: |
+    Fixing a bug in the getaddrinfo resolver where it did not reresolve on EAI_AGAIN. This behavioral change can be temporarily
+    reverted by setting ``envoy.reloadable_features.dns_reresolve_on_eai_again`` to false.
 - area: upstream
   change: |
     Upstream now excludes hosts set to ``DRAINING`` state via EDS from load balancing and panic routing
diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc
index 2fceb79fe5da..13f7b55aac7b 100644
--- a/source/common/runtime/runtime_features.cc
+++ b/source/common/runtime/runtime_features.cc
@@ -40,6 +40,7 @@ RUNTIME_GUARD(envoy_reloadable_features_detect_and_raise_rst_tcp_connection);
 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_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);
 RUNTIME_GUARD(envoy_reloadable_features_enable_compression_bomb_protection);
diff --git a/source/extensions/network/dns_resolver/getaddrinfo/getaddrinfo.cc b/source/extensions/network/dns_resolver/getaddrinfo/getaddrinfo.cc
index 30d2646d87c2..ea6f44f8e479 100644
--- a/source/extensions/network/dns_resolver/getaddrinfo/getaddrinfo.cc
+++ b/source/extensions/network/dns_resolver/getaddrinfo/getaddrinfo.cc
@@ -145,6 +145,8 @@ class GetAddrInfoDnsResolver : public DnsResolver, public Logger::Loggable<Logge
 
     while (true) {
       PendingQuerySharedPtr next_query;
+      const bool reresolve =
+          Runtime::runtimeFeatureEnabled("envoy.reloadable_features.dns_reresolve_on_eai_again");
       {
         absl::MutexLock guard(&mutex_);
         auto condition = [this]() ABSL_EXCLUSIVE_LOCKS_REQUIRED(mutex_) {
@@ -157,6 +159,9 @@ class GetAddrInfoDnsResolver : public DnsResolver, public Logger::Loggable<Logge
 
         next_query = std::move(pending_queries_.front());
         pending_queries_.pop_front();
+        if (reresolve && next_query->cancelled_) {
+          continue;
+        }
       }
 
       ENVOY_LOG(debug, "popped pending query [{}]", next_query->dns_name_);
@@ -178,6 +183,11 @@ class GetAddrInfoDnsResolver : public DnsResolver, public Logger::Loggable<Logge
         auto addrinfo_wrapper = AddrInfoWrapper(addrinfo_result_do_not_use);
         if (rc.return_value_ == 0) {
           response = processResponse(*next_query, addrinfo_wrapper.get());
+        } else if (reresolve && rc.return_value_ == EAI_AGAIN) {
+          ENVOY_LOG(debug, "retrying query [{}]", next_query->dns_name_);
+          absl::MutexLock guard(&mutex_);
+          pending_queries_.push_back(next_query);
+          continue;
         } else {
           // TODO(mattklein123): Handle some errors differently such as `EAI_NODATA`.
           ENVOY_LOG(debug, "getaddrinfo failed with rc={} errno={}", gai_strerror(rc.return_value_),
diff --git a/test/extensions/network/dns_resolver/getaddrinfo/getaddrinfo_test.cc b/test/extensions/network/dns_resolver/getaddrinfo/getaddrinfo_test.cc
index 7c219f11433f..c6e6a2f6156a 100644
--- a/test/extensions/network/dns_resolver/getaddrinfo/getaddrinfo_test.cc
+++ b/test/extensions/network/dns_resolver/getaddrinfo/getaddrinfo_test.cc
@@ -140,7 +140,7 @@ TEST_F(GetAddrInfoDnsImplTest, Failure) {
   TestThreadsafeSingletonInjector<Api::OsSysCallsImpl> os_calls(&os_sys_calls_);
 
   EXPECT_CALL(os_sys_calls_, getaddrinfo(_, _, _, _))
-      .WillOnce(Return(Api::SysCallIntResult{EAI_AGAIN, 0}));
+      .WillOnce(Return(Api::SysCallIntResult{EAI_FAIL, 0}));
   resolver_->resolve(
       "localhost", DnsLookupFamily::All,
       [this](DnsResolver::ResolutionStatus status, std::list<DnsResponse>&& response) {
@@ -153,6 +153,46 @@ TEST_F(GetAddrInfoDnsImplTest, Failure) {
   dispatcher_->run(Event::Dispatcher::RunType::RunUntilExit);
 }
 
+TEST_F(GetAddrInfoDnsImplTest, TryAgainAndSuccess) {
+  TestThreadsafeSingletonInjector<Api::OsSysCallsImpl> os_calls(&os_sys_calls_);
+
+  // 2 calls - one EAGAIN, one success.
+  EXPECT_CALL(os_sys_calls_, getaddrinfo(_, _, _, _))
+      .Times(2)
+      .WillOnce(Return(Api::SysCallIntResult{EAI_AGAIN, 0}))
+      .WillOnce(Return(Api::SysCallIntResult{0, 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, TryAgainThenCancel) {
+  TestThreadsafeSingletonInjector<Api::OsSysCallsImpl> os_calls(&os_sys_calls_);
+
+  std::atomic<ActiveDnsQuery*> query = nullptr;
+
+  EXPECT_CALL(os_sys_calls_, getaddrinfo(_, _, _, _))
+      .Times(testing::AnyNumber())
+      .WillOnce(Invoke([&](const char*, const char*, const addrinfo*, addrinfo**) {
+        query.load()->cancel(ActiveDnsQuery::CancelReason::QueryAbandoned);
+        dispatcher_->exit();
+        return Api::SysCallIntResult{EAI_AGAIN, 0};
+      }));
+  query =
+      resolver_->resolve("localhost", DnsLookupFamily::All,
+                         [](DnsResolver::ResolutionStatus, std::list<DnsResponse>&&) { FAIL(); });
+
+  dispatcher_->run(Event::Dispatcher::RunType::RunUntilExit);
+  resolver_.reset();
+}
+
 TEST_F(GetAddrInfoDnsImplTest, All) {
   // See https://github.com/envoyproxy/envoy/issues/28504.
   DISABLE_UNDER_WINDOWS;