getaddrinfo: Change the dns_nodata_noname_is_success runtime guard …

…to false runtime (envoyproxy#38256) Treating `EAI_NONAME` and `EAI_NODATA` as success can cause an issue because once the host is in the cache as a success, it will only be refreshed for the duration of the TTL, which means any subsequent requests for that host will automatically fail. This fixes an issue introduced in envoyproxy#33934 and changes the `envoy.reloadable_features.dns_nodata_noname_is_success` to `FALSE_RUNTIME_GUARD`, which defaults to false. Risk Level: low Testing: unit tests Docs Changes: n/a Release Notes: n/a Platform Specific Features: getaddrinfo Optional Runtime guard: `envoy.reloadable_features.dns_nodata_noname_is_success` Signed-off-by: Fredy Wijaya <[email protected]>
fredyw · Jan 30, 2025 · ba7bf5f · ba7bf5f
1 parent 107b7e1
commit ba7bf5f
Show file tree

Hide file tree

Showing 4 changed files with 38 additions and 7 deletions.
diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc
@@ -36,7 +36,6 @@ RUNTIME_GUARD(envoy_reloadable_features_boolean_to_string_fix);
 RUNTIME_GUARD(envoy_reloadable_features_check_switch_protocol_websocket_handshake);
 RUNTIME_GUARD(envoy_reloadable_features_dfp_fail_on_empty_host_header);
 RUNTIME_GUARD(envoy_reloadable_features_disallow_quic_client_udp_mmsg);
-RUNTIME_GUARD(envoy_reloadable_features_dns_nodata_noname_is_success);
 RUNTIME_GUARD(envoy_reloadable_features_enable_compression_bomb_protection);
 RUNTIME_GUARD(envoy_reloadable_features_enable_include_histograms);
 RUNTIME_GUARD(envoy_reloadable_features_enable_new_query_param_present_match_behavior);
@@ -165,6 +164,8 @@ FALSE_RUNTIME_GUARD(envoy_reloadable_features_allow_multiplexed_upstream_half_cl
 
 // TODO(renjietang): Flip to true after prod testing.
 FALSE_RUNTIME_GUARD(envoy_reloadable_features_use_network_type_socket_option);
+// TODO(fredyw): Remove after prod testing.
+FALSE_RUNTIME_GUARD(envoy_reloadable_features_dns_nodata_noname_is_success);
 
 // Block of non-boolean flags. Use of int flags is deprecated. Do not add more.
 ABSL_FLAG(uint64_t, re2_max_program_size_error_level, 100, ""); // NOLINT

diff --git a/source/extensions/network/dns_resolver/getaddrinfo/getaddrinfo.cc b/source/extensions/network/dns_resolver/getaddrinfo/getaddrinfo.cc
@@ -194,8 +194,7 @@ void GetAddrInfoDnsResolver::resolveThreadRoutine() {
                   *num_retries);
         next_query->addTrace(static_cast<uint8_t>(GetAddrInfoTrace::DoneRetrying));
         response = std::make_pair(ResolutionStatus::Failure, std::list<DnsResponse>());
-      } else if (treat_nodata_noname_as_success &&
-                 (rc.return_value_ == EAI_NONAME || rc.return_value_ == EAI_NODATA)) {
+      } else if (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.
@@ -204,7 +203,9 @@ void GetAddrInfoDnsResolver::resolveThreadRoutine() {
         ENVOY_LOG(debug, "getaddrinfo for host={} has no results rc={}", next_query->dns_name_,
                   gai_strerror(rc.return_value_));
         next_query->addTrace(static_cast<uint8_t>(GetAddrInfoTrace::NoResult));
-        response = std::make_pair(ResolutionStatus::Completed, std::list<DnsResponse>());
+        response = std::make_pair(treat_nodata_noname_as_success ? ResolutionStatus::Completed
+                                                                 : ResolutionStatus::Failure,
+                                  std::list<DnsResponse>());
       } else {
         ENVOY_LOG(debug, "getaddrinfo failed for host={} with rc={} errno={}",
                   next_query->dns_name_, gai_strerror(rc.return_value_), errorDetails(rc.errno_));

diff --git a/test/extensions/network/dns_resolver/getaddrinfo/BUILD b/test/extensions/network/dns_resolver/getaddrinfo/BUILD
@@ -19,6 +19,7 @@ envoy_extension_cc_test(
     deps = [
         "//source/extensions/network/dns_resolver/getaddrinfo:config",
         "//test/mocks/api:api_mocks",
+        "//test/test_common:test_runtime_lib",
         "//test/test_common:threadsafe_singleton_injector_lib",
         "@envoy_api//envoy/extensions/network/dns_resolver/getaddrinfo/v3:pkg_cc_proto",
     ],

diff --git a/test/extensions/network/dns_resolver/getaddrinfo/getaddrinfo_test.cc b/test/extensions/network/dns_resolver/getaddrinfo/getaddrinfo_test.cc
@@ -5,6 +5,7 @@
 #include "source/extensions/network//dns_resolver/getaddrinfo/getaddrinfo.h"
 
 #include "test/mocks/api/mocks.h"
+#include "test/test_common/test_runtime.h"
 #include "test/test_common/threadsafe_singleton_injector.h"
 #include "test/test_common/utility.h"
 
@@ -107,6 +108,7 @@ class GetAddrInfoDnsImplTest : public testing::Test {
   NiceMock<Api::MockOsSysCalls> os_sys_calls_;
   envoy::extensions::network::dns_resolver::getaddrinfo::v3::GetAddrInfoDnsResolverConfig config_;
   ActiveDnsQuery* active_dns_query_;
+  TestScopedRuntime scoped_runtime_;
 };
 
 MATCHER_P(HasTrace, expected_trace, "") {
@@ -193,7 +195,8 @@ TEST_F(GetAddrInfoDnsImplTest, Failure) {
   dispatcher_->run(Event::Dispatcher::RunType::RunUntilExit);
 }
 
-TEST_F(GetAddrInfoDnsImplTest, NoData) {
+TEST_F(GetAddrInfoDnsImplTest, NoDataAsSuccess) {
+  scoped_runtime_.mergeValues({{"envoy.reloadable_features.dns_nodata_noname_is_success", "true"}});
   initialize();
 
   TestThreadsafeSingletonInjector<Api::OsSysCallsImpl> os_calls(&os_sys_calls_);
@@ -218,7 +221,32 @@ TEST_F(GetAddrInfoDnsImplTest, NoData) {
   dispatcher_->run(Event::Dispatcher::RunType::RunUntilExit);
 }
 
-TEST_F(GetAddrInfoDnsImplTest, NoName) {
+TEST_F(GetAddrInfoDnsImplTest, NoDataAsFailure) {
+  initialize();
+
+  TestThreadsafeSingletonInjector<Api::OsSysCallsImpl> os_calls(&os_sys_calls_);
+
+  EXPECT_CALL(os_sys_calls_, getaddrinfo(_, _, _, _))
+      .WillOnce(Return(Api::SysCallIntResult{EAI_NODATA, 0}));
+  active_dns_query_ =
+      resolver_->resolve("localhost", DnsLookupFamily::All,
+                         [this](DnsResolver::ResolutionStatus status, absl::string_view,
+                                std::list<DnsResponse>&& response) {
+                           EXPECT_EQ(status, DnsResolver::ResolutionStatus::Failure);
+                           EXPECT_TRUE(response.empty());
+                           std::vector<std::string> traces =
+                               absl::StrSplit(active_dns_query_->getTraces(), ',');
+                           EXPECT_THAT(traces, ElementsAre(HasTrace(GetAddrInfoTrace::NotStarted),
+                                                           HasTrace(GetAddrInfoTrace::Starting),
+                                                           HasTrace(GetAddrInfoTrace::NoResult),
+                                                           HasTrace(GetAddrInfoTrace::Callback)));
+                           dispatcher_->exit();
+                         });
+
+  dispatcher_->run(Event::Dispatcher::RunType::RunUntilExit);
+}
+
+TEST_F(GetAddrInfoDnsImplTest, NoNameAsFailure) {
   initialize();
 
   TestThreadsafeSingletonInjector<Api::OsSysCallsImpl> os_calls(&os_sys_calls_);
@@ -229,7 +257,7 @@ TEST_F(GetAddrInfoDnsImplTest, NoName) {
       resolver_->resolve("localhost", DnsLookupFamily::All,
                          [this](DnsResolver::ResolutionStatus status, absl::string_view,
                                 std::list<DnsResponse>&& response) {
-                           EXPECT_EQ(status, DnsResolver::ResolutionStatus::Completed);
+                           EXPECT_EQ(status, DnsResolver::ResolutionStatus::Failure);
                            EXPECT_TRUE(response.empty());
                            std::vector<std::string> traces =
                                absl::StrSplit(active_dns_query_->getTraces(), ',');