Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

apple_dns: Update the Apple DNS resolution details #36536

Merged
merged 5 commits into from
Oct 14, 2024
Merged

apple_dns: Update the Apple DNS resolution details #36536

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9b3fb82
Update Apple DNS resolution details
fredyw Oct 10, 2024
dc83a12
Merge branch 'main' into apple_dns_details
fredyw Oct 10, 2024
dcf4a53
Merge branch 'main' into apple_dns_details
fredyw Oct 14, 2024
4e24e17
Debug
fredyw Oct 14, 2024
a1a3f86
Fix tests
fredyw Oct 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions source/extensions/network/dns_resolver/apple/apple_dns_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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::Completed, "apple_dns_success",
callback(DnsResolver::ResolutionStatus::Completed, "apple_dns_immediate_success",
{DnsResponse(address, std::chrono::seconds(60))});
return {nullptr, true};
}
Expand Down Expand Up @@ -188,7 +188,7 @@ void AppleDnsResolverImpl::PendingResolution::onEventCallback(uint32_t events) {
// events indicates that the sd_ref state is broken.
// Therefore, finish resolving with an error.
pending_response_.status_ = ResolutionStatus::Failure;
pending_response_.details_ = absl::StrCat(error);
pending_response_.details_ = absl::StrCat("apple_dns_error_", error);
finishResolve();
}
}
Expand Down Expand Up @@ -378,6 +378,8 @@ void AppleDnsResolverImpl::PendingResolution::onDNSServiceGetAddrInfoReply(
if (!(flags & kDNSServiceFlagsMoreComing) && isAddressFamilyProcessed(kDNSServiceProtocol_IPv4) &&
isAddressFamilyProcessed(kDNSServiceProtocol_IPv6)) {
ENVOY_LOG(debug, "DNS Resolver flushing queries pending callback");
pending_response_.status_ = ResolutionStatus::Completed;
pending_response_.details_ = "apple_dns_success";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

was this broken before? Can we regression test?
/wait

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We discovered some instances of not_set in our testing, which gives the impression that the DNS query never gets executed. This is because by default, the status is set to Completed and details to not_set. This PR aims to fix the potential confusion.

I also updated the PR to add missing tests for the DNS resolution details that previously didn't exist.

finishResolve();
// Note: Nothing can follow this call to finishResolve due to deletion of this
// object upon resolution.
Expand Down
Loading