From 4804af46b1a7e423dce8e4bd65fd6774d0a0a42c Mon Sep 17 00:00:00 2001 From: Fredy Wijaya Date: Wed, 16 Oct 2024 08:10:11 -0500 Subject: [PATCH] apple_dns: Add error code in the DNS resolution details on completion (#36617) Commit Message: Renames the resolution details from `apple_dns_success` to `apple_dns_completed` with error code since `completed` is more correct that `success` given that the callback will still be called when the error code is `kDNSServiceErr_NoSuchRecord`. Additional Description: This behavior is also consistent with the implementations of other resolvers. - [getaddrinfo](https://github.com/envoyproxy/envoy/blob/7847268600ed62e2485fc0c4ab853e7fd79ddf63/source/extensions/network/dns_resolver/getaddrinfo/getaddrinfo.cc#L216) - [c-ares](https://github.com/envoyproxy/envoy/blob/7847268600ed62e2485fc0c4ab853e7fd79ddf63/source/extensions/network/dns_resolver/cares/dns_impl.cc#L219) Risk Level: low Testing: unit tests Docs Changes: n/a Release Notes: n/a Platform Specific Features: apple_dns --------- Signed-off-by: Fredy Wijaya --- .../dns_resolver/apple/apple_dns_impl.cc | 2 +- .../dns_resolver/apple/apple_dns_impl_test.cc | 28 +++++++++---------- 2 files changed, 15 insertions(+), 15 deletions(-) diff --git a/source/extensions/network/dns_resolver/apple/apple_dns_impl.cc b/source/extensions/network/dns_resolver/apple/apple_dns_impl.cc index 112bcbb0ae22..23f8affa0c27 100644 --- a/source/extensions/network/dns_resolver/apple/apple_dns_impl.cc +++ b/source/extensions/network/dns_resolver/apple/apple_dns_impl.cc @@ -379,7 +379,7 @@ void AppleDnsResolverImpl::PendingResolution::onDNSServiceGetAddrInfoReply( isAddressFamilyProcessed(kDNSServiceProtocol_IPv6)) { ENVOY_LOG(debug, "DNS Resolver flushing queries pending callback"); pending_response_.status_ = ResolutionStatus::Completed; - pending_response_.details_ = "apple_dns_success"; + pending_response_.details_ = absl::StrCat("apple_dns_completed_", error_code); finishResolve(); // Note: Nothing can follow this call to finishResolve due to deletion of this // object upon resolution. diff --git a/test/extensions/network/dns_resolver/apple/apple_dns_impl_test.cc b/test/extensions/network/dns_resolver/apple/apple_dns_impl_test.cc index e1f10f409f93..23700a2f8add 100644 --- a/test/extensions/network/dns_resolver/apple/apple_dns_impl_test.cc +++ b/test/extensions/network/dns_resolver/apple/apple_dns_impl_test.cc @@ -337,7 +337,7 @@ TEST_F(AppleDnsImplTest, DnsIpAddressVersionAllSupportsV6Only) { [=](DnsResolver::ResolutionStatus status, absl::string_view details, std::list&& results) -> void { EXPECT_EQ(DnsResolver::ResolutionStatus::Completed, status); - EXPECT_EQ(details, "apple_dns_success"); + EXPECT_THAT(details, StartsWith("apple_dns_completed")); // On v4 only networks, there will be no results. for (const auto& result : results) { const auto& addrinfo = result.addrInfo(); @@ -368,7 +368,7 @@ TEST_F(AppleDnsImplTest, DnsIpAddressVersionV6OnlySupportsV4Only) { [=](DnsResolver::ResolutionStatus status, absl::string_view details, std::list&& results) -> void { EXPECT_EQ(DnsResolver::ResolutionStatus::Completed, status); - EXPECT_EQ(details, "apple_dns_success"); + EXPECT_THAT(details, StartsWith("apple_dns_completed")); for (const auto& result : results) { const auto& addrinfo = result.addrInfo(); EXPECT_THAT(addrinfo.address_->ip()->ipv6(), NotNull()); @@ -386,7 +386,7 @@ TEST_F(AppleDnsImplTest, DnsIpAddressVersionV6OnlySupportsV6Only) { [=](DnsResolver::ResolutionStatus status, absl::string_view details, std::list&& results) -> void { EXPECT_EQ(DnsResolver::ResolutionStatus::Completed, status); - EXPECT_EQ(details, "apple_dns_success"); + EXPECT_THAT(details, StartsWith("apple_dns_completed")); EXPECT_FALSE(results.empty()); for (const auto& result : results) { const auto& addrinfo = result.addrInfo(); @@ -411,7 +411,7 @@ TEST_F(AppleDnsImplTest, DnsIpAddressVersionAutoSupportsV6Only) { [=](DnsResolver::ResolutionStatus status, absl::string_view details, std::list&& results) -> void { EXPECT_EQ(DnsResolver::ResolutionStatus::Completed, status); - EXPECT_EQ(details, "apple_dns_success"); + EXPECT_THAT(details, StartsWith("apple_dns_completed")); // On v4 only networks, there will be no results. for (const auto& result : results) { const auto& addrinfo = result.addrInfo(); @@ -436,7 +436,7 @@ TEST_F(AppleDnsImplTest, DnsIpAddressVersionV4PreferredSupportsV6Only) { [=](DnsResolver::ResolutionStatus status, absl::string_view details, std::list&& results) -> void { EXPECT_EQ(DnsResolver::ResolutionStatus::Completed, status); - EXPECT_EQ(details, "apple_dns_success"); + EXPECT_THAT(details, StartsWith("apple_dns_completed")); // On v4 only networks, there will be no results. for (const auto& result : results) { const auto& addrinfo = result.addrInfo(); @@ -660,7 +660,7 @@ class AppleDnsImplFakeApiTest : public testing::Test { expected_address_size](DnsResolver::ResolutionStatus status, absl::string_view details, std::list&& response) -> void { EXPECT_EQ(DnsResolver::ResolutionStatus::Completed, status); - EXPECT_EQ(details, "apple_dns_success"); + EXPECT_THAT(details, StartsWith("apple_dns_completed")); EXPECT_EQ(expected_address_size, response.size()); if (dns_lookup_family == DnsLookupFamily::Auto) { @@ -918,7 +918,7 @@ TEST_F(AppleDnsImplFakeApiTest, QuerySynchronousCompletionUnroutableFamilies) { absl::string_view details, std::list&& response) -> void { EXPECT_EQ(DnsResolver::ResolutionStatus::Completed, status); - EXPECT_EQ(details, "apple_dns_success"); + EXPECT_THAT(details, StartsWith("apple_dns_completed")); EXPECT_EQ(1, response.size()); EXPECT_EQ("1.2.3.4:0", response.front().addrInfo().address_->asString()); EXPECT_EQ(std::chrono::seconds(30), response.front().addrInfo().ttl_); @@ -958,7 +958,7 @@ TEST_F(AppleDnsImplFakeApiTest, QuerySynchronousCompletion) { absl::string_view details, std::list&& response) -> void { EXPECT_EQ(DnsResolver::ResolutionStatus::Completed, status); - EXPECT_EQ(details, "apple_dns_success"); + EXPECT_THAT(details, StartsWith("apple_dns_completed")); EXPECT_EQ(1, response.size()); EXPECT_EQ("1.2.3.4:0", response.front().addrInfo().address_->asString()); EXPECT_EQ(std::chrono::seconds(30), response.front().addrInfo().ttl_); @@ -1015,7 +1015,7 @@ TEST_F(AppleDnsImplFakeApiTest, MultipleAddresses) { [&dns_callback_executed](DnsResolver::ResolutionStatus status, absl::string_view details, std::list&& response) -> void { EXPECT_EQ(DnsResolver::ResolutionStatus::Completed, status); - EXPECT_EQ(details, "apple_dns_success"); + EXPECT_THAT(details, StartsWith("apple_dns_completed")); EXPECT_EQ(2, response.size()); dns_callback_executed.Notify(); }); @@ -1148,7 +1148,7 @@ TEST_F(AppleDnsImplFakeApiTest, MultipleQueries) { [&dns_callback_executed](DnsResolver::ResolutionStatus status, absl::string_view details, std::list&& response) -> void { EXPECT_EQ(DnsResolver::ResolutionStatus::Completed, status); - EXPECT_EQ(details, "apple_dns_success"); + EXPECT_THAT(details, StartsWith("apple_dns_completed")); EXPECT_EQ(1, response.size()); EXPECT_EQ("1.2.3.4:0", response.front().addrInfo().address_->asString()); EXPECT_EQ(std::chrono::seconds(30), response.front().addrInfo().ttl_); @@ -1170,7 +1170,7 @@ TEST_F(AppleDnsImplFakeApiTest, MultipleQueries) { [&dns_callback_executed2](DnsResolver::ResolutionStatus status, absl::string_view details, std::list&& response) -> void { EXPECT_EQ(DnsResolver::ResolutionStatus::Completed, status); - EXPECT_EQ(details, "apple_dns_success"); + EXPECT_THAT(details, StartsWith("apple_dns_completed")); EXPECT_EQ(1, response.size()); EXPECT_EQ("5.6.7.8:0", response.front().addrInfo().address_->asString()); EXPECT_EQ(std::chrono::seconds(30), response.front().addrInfo().ttl_); @@ -1225,7 +1225,7 @@ TEST_F(AppleDnsImplFakeApiTest, MultipleQueriesOneFails) { // Even though the second query will fail, this one will flush with the // state it had. EXPECT_EQ(DnsResolver::ResolutionStatus::Completed, status); - EXPECT_EQ(details, "apple_dns_success"); + EXPECT_THAT(details, StartsWith("apple_dns_completed")); EXPECT_EQ(1, response.size()); EXPECT_EQ("1.2.3.4:0", response.front().addrInfo().address_->asString()); EXPECT_EQ(std::chrono::seconds(30), response.front().addrInfo().ttl_); @@ -1289,7 +1289,7 @@ TEST_F(AppleDnsImplFakeApiTest, ResultWithOnlyNonAdditiveReplies) { [&dns_callback_executed](DnsResolver::ResolutionStatus status, absl::string_view details, std::list&& response) -> void { EXPECT_EQ(DnsResolver::ResolutionStatus::Completed, status); - EXPECT_EQ(details, "apple_dns_success"); + EXPECT_THAT(details, StartsWith("apple_dns_completed")); EXPECT_TRUE(response.empty()); dns_callback_executed.Notify(); }); @@ -1367,7 +1367,7 @@ TEST_F(AppleDnsImplFakeApiTest, DeallocateOnDestruction) { [&dns_callback_executed](DnsResolver::ResolutionStatus status, absl::string_view details, std::list&& response) -> void { EXPECT_EQ(DnsResolver::ResolutionStatus::Completed, status); - EXPECT_EQ(details, "apple_dns_success"); + EXPECT_THAT(details, StartsWith("apple_dns_completed")); EXPECT_EQ(1, response.size()); dns_callback_executed.Notify(); });