dns: rename Success to Completed (envoyproxy#36143)

Signed-off-by: chenyuliang5 <[email protected]>
fredyw · Sep 19, 2024 · ec92cc3 · ec92cc3
1 parent 322cda1
commit ec92cc3
Show file tree

Hide file tree

Showing 27 changed files with 347 additions and 339 deletions.
diff --git a/envoy/network/dns.h b/envoy/network/dns.h
@@ -86,11 +86,11 @@ 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
+   * DNS resolution can return result statuses like NODATA、SERVFAIL 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 };
+  enum class ResolutionStatus { Completed, Failure };
 
   /**
    * Called when a resolution attempt is complete.

diff --git a/mobile/test/common/network/connectivity_manager_test.cc b/mobile/test/common/network/connectivity_manager_test.cc
@@ -81,15 +81,15 @@ TEST_F(ConnectivityManagerTest, WhenDrainPostDnsRefreshEnabledDrainsPostDnsRefre
   connectivity_manager_->onDnsResolutionComplete(
       "cached.example.com",
       std::make_shared<Extensions::Common::DynamicForwardProxy::MockDnsHostInfo>(),
-      Network::DnsResolver::ResolutionStatus::Success);
+      Network::DnsResolver::ResolutionStatus::Completed);
   connectivity_manager_->onDnsResolutionComplete(
       "not-cached.example.com",
       std::make_shared<Extensions::Common::DynamicForwardProxy::MockDnsHostInfo>(),
-      Network::DnsResolver::ResolutionStatus::Success);
+      Network::DnsResolver::ResolutionStatus::Completed);
   connectivity_manager_->onDnsResolutionComplete(
       "not-cached2.example.com",
       std::make_shared<Extensions::Common::DynamicForwardProxy::MockDnsHostInfo>(),
-      Network::DnsResolver::ResolutionStatus::Success);
+      Network::DnsResolver::ResolutionStatus::Completed);
 }
 
 TEST_F(ConnectivityManagerTest, WhenDrainPostDnsNotEnabledDoesntDrainPostDnsRefresh) {
@@ -102,7 +102,7 @@ TEST_F(ConnectivityManagerTest, WhenDrainPostDnsNotEnabledDoesntDrainPostDnsRefr
   EXPECT_CALL(cm_, drainConnections(_)).Times(0);
   connectivity_manager_->onDnsResolutionComplete(
       "example.com", std::make_shared<Extensions::Common::DynamicForwardProxy::MockDnsHostInfo>(),
-      Network::DnsResolver::ResolutionStatus::Success);
+      Network::DnsResolver::ResolutionStatus::Completed);
 }
 
 TEST_F(ConnectivityManagerTest,

diff --git a/source/extensions/clusters/logical_dns/logical_dns_cluster.cc b/source/extensions/clusters/logical_dns/logical_dns_cluster.cc
@@ -107,7 +107,7 @@ void LogicalDnsCluster::startResolve() {
         // If the DNS resolver successfully resolved with an empty response list, the logical DNS
         // cluster does not update. This ensures that a potentially previously resolved address does
         // not stabilize back to 0 hosts.
-        if (status == Network::DnsResolver::ResolutionStatus::Success && !response.empty()) {
+        if (status == Network::DnsResolver::ResolutionStatus::Completed && !response.empty()) {
           info_->configUpdateStats().update_success_.inc();
           const auto addrinfo = response.front().addrInfo();
           // TODO(mattklein123): Move port handling into the DNS interface.

diff --git a/source/extensions/clusters/redis/redis_cluster.cc b/source/extensions/clusters/redis/redis_cluster.cc
@@ -365,7 +365,7 @@ void RedisCluster::RedisDiscoverySession::resolveClusterHostnames(
             updateDnsStats(status, response.empty());
             // If DNS resolution for a primary fails, we stop resolution for remaining, and reset
             // the timer.
-            if (status != Network::DnsResolver::ResolutionStatus::Success) {
+            if (status != Network::DnsResolver::ResolutionStatus::Completed) {
               ENVOY_LOG(error, "Unable to resolve cluster slot primary hostname {}",
                         slot.primary_hostname_);
               resolve_timer_->enableTimer(parent_.cluster_refresh_rate_);
@@ -419,7 +419,7 @@ void RedisCluster::RedisDiscoverySession::resolveReplicas(
           updateDnsStats(status, response.empty());
           // If DNS resolution fails here, we move on to resolve other replicas in the list.
           // We log a warn message.
-          if (status != Network::DnsResolver::ResolutionStatus::Success) {
+          if (status != Network::DnsResolver::ResolutionStatus::Completed) {
             ENVOY_LOG(warn, "Unable to resolve cluster replica address {}", replica.first);
           } else {
             // Replica resolved

diff --git a/source/extensions/clusters/strict_dns/strict_dns_cluster.cc b/source/extensions/clusters/strict_dns/strict_dns_cluster.cc
@@ -130,7 +130,7 @@ void StrictDnsClusterImpl::ResolveTarget::startResolve() {
 
         std::chrono::milliseconds final_refresh_rate = parent_.dns_refresh_rate_ms_;
 
-        if (status == Network::DnsResolver::ResolutionStatus::Success) {
+        if (status == Network::DnsResolver::ResolutionStatus::Completed) {
           parent_.info_->configUpdateStats().update_success_.inc();
 
           HostVector new_hosts;

diff --git a/source/extensions/common/dynamic_forward_proxy/dns_cache_impl.cc b/source/extensions/common/dynamic_forward_proxy/dns_cache_impl.cc
@@ -216,7 +216,7 @@ void DnsCacheImpl::startCacheLoad(const std::string& host, uint16_t default_port
   // If the DNS request was simply to create a host endpoint in a Dynamic Forward Proxy cluster,
   // fast fail the look-up as the address is not needed.
   if (is_proxy_lookup) {
-    finishResolve(host, Network::DnsResolver::ResolutionStatus::Success, "proxy_resolve", {}, {},
+    finishResolve(host, Network::DnsResolver::ResolutionStatus::Completed, "proxy_resolve", {}, {},
                   true);
   } else {
     startResolve(host, *primary_host);
@@ -503,7 +503,7 @@ void DnsCacheImpl::finishResolve(const std::string& host,
   runResolutionCompleteCallbacks(host, primary_host_info->host_info_, status);
 
   // Kick off the refresh timer.
-  if (status == Network::DnsResolver::ResolutionStatus::Success) {
+  if (status == Network::DnsResolver::ResolutionStatus::Completed) {
     primary_host_info->failure_backoff_strategy_->reset(
         std::chrono::duration_cast<std::chrono::milliseconds>(dns_ttl).count());
     primary_host_info->refresh_timer_->enableTimer(dns_ttl);
@@ -682,7 +682,7 @@ void DnsCacheImpl::loadCacheEntries(
         key, accumulateToString<Network::DnsResponse>(responses, [](const auto& dns_response) {
           return dns_response.addrInfo().address_->asString();
         }));
-    finishResolve(key, Network::DnsResolver::ResolutionStatus::Success, "from_cache",
+    finishResolve(key, Network::DnsResolver::ResolutionStatus::Completed, "from_cache",
                   std::move(responses), resolution_time);
     stats_.cache_load_.inc();
     return KeyValueStore::Iterate::Continue;

diff --git a/source/extensions/common/wasm/context.cc b/source/extensions/common/wasm/context.cc
@@ -202,7 +202,7 @@ void Context::onResolveDns(uint32_t token, Envoy::Network::DnsResolver::Resoluti
   if (wasm()->isFailed() || !wasm()->on_resolve_dns_) {
     return;
   }
-  if (status != Network::DnsResolver::ResolutionStatus::Success) {
+  if (status != Network::DnsResolver::ResolutionStatus::Completed) {
     buffer_.set("");
     wasm()->on_resolve_dns_(this, id_, token, 0);
     return;

diff --git a/source/extensions/filters/udp/dns_filter/dns_filter.cc b/source/extensions/filters/udp/dns_filter/dns_filter.cc
@@ -234,7 +234,7 @@ DnsFilter::DnsFilter(Network::UdpReadFilterCallbacks& callbacks,
     // We cannot retry the resolution if ares returns without a response. The ares context
     // is still dirty and will result in a segfault when it is freed during a subsequent resolve
     // call from here. We will retry resolutions for pending lookups only
-    if (context->resolution_status_ != Network::DnsResolver::ResolutionStatus::Success &&
+    if (context->resolution_status_ != Network::DnsResolver::ResolutionStatus::Completed &&
         !context->in_callback_ && context->retry_ > 0) {
       --context->retry_;
       ENVOY_LOG(debug, "resolving name [{}] via external resolvers [retry {}]", query->name_,

diff --git a/source/extensions/filters/udp/dns_filter/dns_filter_resolver.cc b/source/extensions/filters/udp/dns_filter/dns_filter_resolver.cc
@@ -30,7 +30,7 @@ void DnsFilterResolver::resolveExternalQuery(DnsQueryContextPtr context,
     // We don't support other lookups other than A and AAAA. Set success here so that we don't
     // retry for something that we are certain will fail.
     ENVOY_LOG(debug, "Unknown query type [{}] for upstream lookup", domain_query->type_);
-    ctx.query_context->resolution_status_ = Network::DnsResolver::ResolutionStatus::Success;
+    ctx.query_context->resolution_status_ = Network::DnsResolver::ResolutionStatus::Completed;
     ctx.resolver_status = DnsFilterResolverStatus::Complete;
     invokeCallback(ctx);
     return;
@@ -87,7 +87,7 @@ void DnsFilterResolver::resolveExternalQuery(DnsQueryContextPtr context,
                        ctx.resolver_status = DnsFilterResolverStatus::Complete;
 
                        // C-ares doesn't expose the TTL in the data available here.
-                       if (status == Network::DnsResolver::ResolutionStatus::Success) {
+                       if (status == Network::DnsResolver::ResolutionStatus::Completed) {
                          ctx.resolved_hosts.reserve(response.size());
                          for (const auto& resp : response) {
                            const auto& addrinfo = resp.addrInfo();

diff --git a/source/extensions/filters/udp/dns_filter/dns_parser.h b/source/extensions/filters/udp/dns_filter/dns_parser.h
@@ -200,7 +200,7 @@ class DnsQueryContext {
   uint64_t retry_;
   uint16_t id_;
   Network::DnsResolver::ResolutionStatus resolution_status_ =
-      Network::DnsResolver::ResolutionStatus::Success;
+      Network::DnsResolver::ResolutionStatus::Completed;
   DnsHeader header_;
   DnsHeader response_header_;
   DnsQueryPtrVec queries_;

diff --git a/source/extensions/network/dns_resolver/apple/apple_dns_impl.cc b/source/extensions/network/dns_resolver/apple/apple_dns_impl.cc
@@ -72,7 +72,7 @@ AppleDnsResolverImpl::startResolution(const std::string& dns_name,
     ENVOY_LOG_EVENT(debug, "apple_dns_immediate_resolution",
                     "DNS resolver resolved ({}) to ({}) without issuing call to Apple API",
                     dns_name, address->asString());
-    callback(DnsResolver::ResolutionStatus::Success, "apple_dns_success",
+    callback(DnsResolver::ResolutionStatus::Completed, "apple_dns_success",
              {DnsResponse(address, std::chrono::seconds(60))});
     return {nullptr, true};
   }

diff --git a/source/extensions/network/dns_resolver/apple/apple_dns_impl.h b/source/extensions/network/dns_resolver/apple/apple_dns_impl.h
@@ -122,7 +122,7 @@ class AppleDnsResolverImpl : public DnsResolver, protected Logger::Loggable<Logg
     // Small wrapping struct to accumulate addresses from firings of the
     // onDNSServiceGetAddrInfoReply callback.
     struct PendingResponse {
-      ResolutionStatus status_ = ResolutionStatus::Success;
+      ResolutionStatus status_ = ResolutionStatus::Completed;
       std::string details_ = "not_set";
       // `v4_response_received_` and `v6_response_received_` denote whether a callback from the
       // `DNSServiceGetAddrInfo` call has been received for the IPv4 address family and IPv6

diff --git a/source/extensions/network/dns_resolver/cares/dns_impl.cc b/source/extensions/network/dns_resolver/cares/dns_impl.cc
@@ -215,7 +215,7 @@ void DnsResolverImpl::AddrInfoPendingResolution::onAresGetAddrInfoCallback(
   }
 
   if (status == ARES_SUCCESS) {
-    pending_response_.status_ = ResolutionStatus::Success;
+    pending_response_.status_ = ResolutionStatus::Completed;
     pending_response_.details_ = absl::StrCat("cares_success:", ares_strerror(status));
 
     if (addrinfo != nullptr && addrinfo->nodes != nullptr) {
@@ -264,7 +264,7 @@ void DnsResolverImpl::AddrInfoPendingResolution::onAresGetAddrInfoCallback(
   } else if (isResponseWithNoRecords(status)) {
     // Treat `ARES_ENODATA` or `ARES_ENOTFOUND` here as success to populate back the
     // "empty records" response.
-    pending_response_.status_ = ResolutionStatus::Success;
+    pending_response_.status_ = ResolutionStatus::Completed;
     pending_response_.details_ = absl::StrCat("cares_norecords:", ares_strerror(status));
     ASSERT(addrinfo == nullptr);
   } else {

diff --git a/source/extensions/network/dns_resolver/cares/dns_impl.h b/source/extensions/network/dns_resolver/cares/dns_impl.h
@@ -110,10 +110,9 @@ class DnsResolverImpl : public DnsResolver, protected Logger::Loggable<Logger::I
     };
 
     // Note: pending_response_ is constructed with ResolutionStatus::Failure by default and
-    // __only__ changed to ResolutionStatus::Success 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::Success callback.
+    // __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.
     PendingResponse pending_response_{ResolutionStatus::Failure, {}};
   };
 

diff --git a/source/extensions/network/dns_resolver/getaddrinfo/getaddrinfo.cc b/source/extensions/network/dns_resolver/getaddrinfo/getaddrinfo.cc
@@ -109,7 +109,7 @@ GetAddrInfoDnsResolver::processResponse(const PendingQuery& query,
               return dns_response.addrInfo().address_->asString();
             }));
 
-  return std::make_pair(ResolutionStatus::Success, final_results);
+  return std::make_pair(ResolutionStatus::Completed, final_results);
 }
 
 // Background thread which wakes up and does resolutions.
@@ -190,7 +190,7 @@ void GetAddrInfoDnsResolver::resolveThreadRoutine() {
         // https://github.com/envoyproxy/envoy/blob/099d85925b32ce8bf06e241ee433375a0a3d751b/source/extensions/network/dns_resolver/cares/dns_impl.h#L109-L111.
         ENVOY_LOG(debug, "getaddrinfo for host={} has no results rc={}", next_query->dns_name_,
                   gai_strerror(rc.return_value_));
-        response = std::make_pair(ResolutionStatus::Success, std::list<DnsResponse>());
+        response = std::make_pair(ResolutionStatus::Completed, 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/common/upstream/cluster_manager_impl_test.cc b/test/common/upstream/cluster_manager_impl_test.cc
@@ -2824,7 +2824,7 @@ TEST_P(ClusterManagerLifecycleTest, DynamicHostRemove) {
   cluster_manager_->setInitializedCb([&]() -> void { initialized.ready(); });
   EXPECT_CALL(initialized, ready());
 
-  dns_callback(Network::DnsResolver::ResolutionStatus::Success, "",
+  dns_callback(Network::DnsResolver::ResolutionStatus::Completed, "",
                TestUtility::makeDnsResponse({"127.0.0.1", "127.0.0.2"}));
 
   // After we are initialized, we should immediately get called back if someone asks for an
@@ -2878,7 +2878,7 @@ TEST_P(ClusterManagerLifecycleTest, DynamicHostRemove) {
 
   // Remove the first host, this should lead to the first cp being drained.
   dns_timer_->invokeCallback();
-  dns_callback(Network::DnsResolver::ResolutionStatus::Success, "",
+  dns_callback(Network::DnsResolver::ResolutionStatus::Completed, "",
                TestUtility::makeDnsResponse({"127.0.0.2"}));
   cp1->idle_cb_();
   cp1->idle_cb_ = nullptr;
@@ -2912,7 +2912,7 @@ TEST_P(ClusterManagerLifecycleTest, DynamicHostRemove) {
   // Now add and remove a host that we never have a conn pool to. This should not lead to any
   // drain callbacks, etc.
   dns_timer_->invokeCallback();
-  dns_callback(Network::DnsResolver::ResolutionStatus::Success, "",
+  dns_callback(Network::DnsResolver::ResolutionStatus::Completed, "",
                // NOLINTNEXTLINE(clang-analyzer-cplusplus.NewDeleteLeaks)
                TestUtility::makeDnsResponse({"127.0.0.2", "127.0.0.3"}));
   factory_.tls_.shutdownThread();
@@ -2980,7 +2980,7 @@ TEST_P(ClusterManagerLifecycleTest, DynamicHostRemoveWithTls) {
   cluster_manager_->setInitializedCb([&]() -> void { initialized.ready(); });
   EXPECT_CALL(initialized, ready());
 
-  dns_callback(Network::DnsResolver::ResolutionStatus::Success, "",
+  dns_callback(Network::DnsResolver::ResolutionStatus::Completed, "",
                TestUtility::makeDnsResponse({"127.0.0.1", "127.0.0.2"}));
 
   // After we are initialized, we should immediately get called back if someone asks for an
@@ -3076,7 +3076,7 @@ TEST_P(ClusterManagerLifecycleTest, DynamicHostRemoveWithTls) {
 
   // Remove the first host, this should lead to the first cp being drained.
   dns_timer_->invokeCallback();
-  dns_callback(Network::DnsResolver::ResolutionStatus::Success, "",
+  dns_callback(Network::DnsResolver::ResolutionStatus::Completed, "",
                TestUtility::makeDnsResponse({"127.0.0.2"}));
   cp1->idle_cb_();
   cp1->idle_cb_ = nullptr;
@@ -3134,7 +3134,7 @@ TEST_P(ClusterManagerLifecycleTest, DynamicHostRemoveWithTls) {
   // Now add and remove a host that we never have a conn pool to. This should not lead to any
   // drain callbacks, etc.
   dns_timer_->invokeCallback();
-  dns_callback(Network::DnsResolver::ResolutionStatus::Success, "",
+  dns_callback(Network::DnsResolver::ResolutionStatus::Completed, "",
                // NOLINTNEXTLINE(clang-analyzer-cplusplus.NewDeleteLeaks)
                TestUtility::makeDnsResponse({"127.0.0.2", "127.0.0.3"}));
   factory_.tls_.shutdownThread();
@@ -3924,7 +3924,7 @@ TEST_P(ClusterManagerLifecycleTest, DynamicHostRemoveDefaultPriority) {
   EXPECT_FALSE(all_clusters.active_clusters_.at("cluster_1").get().info()->addedViaApi());
   EXPECT_EQ(nullptr, cluster_manager_->getThreadLocalCluster("cluster_1"));
 
-  dns_callback(Network::DnsResolver::ResolutionStatus::Success, "",
+  dns_callback(Network::DnsResolver::ResolutionStatus::Completed, "",
                TestUtility::makeDnsResponse({"127.0.0.2"}));
 
   EXPECT_CALL(factory_, allocateConnPool_(_, _, _, _, _, _))
@@ -3957,7 +3957,7 @@ TEST_P(ClusterManagerLifecycleTest, DynamicHostRemoveDefaultPriority) {
   // crash.
   dns_timer_->invokeCallback();
   // NOLINTNEXTLINE(clang-analyzer-cplusplus.NewDeleteLeaks)
-  dns_callback(Network::DnsResolver::ResolutionStatus::Success, "",
+  dns_callback(Network::DnsResolver::ResolutionStatus::Completed, "",
                TestUtility::makeDnsResponse({}));
 
   factory_.tls_.shutdownThread();
@@ -4018,7 +4018,7 @@ TEST_P(ClusterManagerLifecycleTest, ConnPoolDestroyWithDraining) {
   EXPECT_FALSE(all_clusters.active_clusters_.at("cluster_1").get().info()->addedViaApi());
   EXPECT_EQ(nullptr, cluster_manager_->getThreadLocalCluster("cluster_1"));
 
-  dns_callback(Network::DnsResolver::ResolutionStatus::Success, "",
+  dns_callback(Network::DnsResolver::ResolutionStatus::Completed, "",
                TestUtility::makeDnsResponse({"127.0.0.2"}));
 
   MockConnPoolWithDestroy* mock_cp = new MockConnPoolWithDestroy();
@@ -4043,7 +4043,7 @@ TEST_P(ClusterManagerLifecycleTest, ConnPoolDestroyWithDraining) {
   // Remove the first host, this should lead to the cp being drained.
   dns_timer_->invokeCallback();
   // NOLINTNEXTLINE(clang-analyzer-cplusplus.NewDeleteLeaks)
-  dns_callback(Network::DnsResolver::ResolutionStatus::Success, "",
+  dns_callback(Network::DnsResolver::ResolutionStatus::Completed, "",
                TestUtility::makeDnsResponse({}));
 
   // The drained callback might get called when the CP is being destroyed.