dns resolvers: add All lookup mode #18464
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -139,7 +139,8 @@ AppleDnsResolverImpl::PendingResolution::PendingResolution(AppleDnsResolverImpl& | |
| const std::string& dns_name, | ||
| DnsLookupFamily dns_lookup_family) | ||
| : parent_(parent), callback_(callback), dispatcher_(dispatcher), dns_name_(dns_name), | ||
| pending_response_({ResolutionStatus::Success, {}, {}, {}}), | ||
| dns_lookup_family_(dns_lookup_family) {} | ||
|
|
||
| AppleDnsResolverImpl::PendingResolution::~PendingResolution() { | ||
| ENVOY_LOG(debug, "Destroying PendingResolution for {}", dns_name_); | ||
|
|
@@ -181,37 +182,47 @@ void AppleDnsResolverImpl::PendingResolution::onEventCallback(uint32_t events) { | |
| // Similar to receiving an error in onDNSServiceGetAddrInfoReply, an error while processing fd | ||
| // events indicates that the sd_ref state is broken. | ||
| // Therefore, finish resolving with an error. | ||
| pending_response_.status_ = ResolutionStatus::Failure; | ||
| finishResolve(); | ||
| } | ||
| } | ||
|
|
||
| std::list<DnsResponse>& AppleDnsResolverImpl::PendingResolution::finalAddressList() { | ||
| switch (dns_lookup_family_) { | ||
| case DnsLookupFamily::V4Only: | ||
| return pending_response_.v4_responses_; | ||
| case DnsLookupFamily::V6Only: | ||
| return pending_response_.v6_responses_; | ||
| case DnsLookupFamily::Auto: | ||
| // Per API docs only give v4 if v6 is not available. | ||
| if (pending_response_.v6_responses_.empty()) { | ||
| return pending_response_.v4_responses_; | ||
| } | ||
| return pending_response_.v6_responses_; | ||
| case DnsLookupFamily::V4Preferred: | ||
| // Per API docs only give v6 if v4 is not available. | ||
| if (pending_response_.v4_responses_.empty()) { | ||
| return pending_response_.v6_responses_; | ||
| } | ||
| return pending_response_.v4_responses_; | ||
| case DnsLookupFamily::All: | ||
| ASSERT(pending_response_.all_responses_.empty()); | ||
| pending_response_.all_responses_.insert(pending_response_.all_responses_.end(), | ||
|
Contributor
There was a problem hiding this comment. If I'm understanding this correctly, it's adding v4 addresses before v6 addresses. In general I'd suggest v6 first but you can get fancier as per the happy eyeballs spec
Member
Author
There was a problem hiding this comment. yep, that's the case. But the callback receiver can do what they want with the list, and is where I would expect the ordering complexity to happen; so I am not too opinionated with order here.
Contributor
There was a problem hiding this comment. I think we should follow the RFC 8305 (the happy eyeballs spec) as David suggest, and I agree with Jose that we should probably do that in HappyEyeballsConnectionImpl. I'd be happy to take a whack at that. |
||
| pending_response_.v4_responses_.begin(), | ||
| pending_response_.v4_responses_.end()); | ||
| pending_response_.all_responses_.insert(pending_response_.all_responses_.end(), | ||
| pending_response_.v6_responses_.begin(), | ||
| pending_response_.v6_responses_.end()); | ||
| return pending_response_.all_responses_; | ||
| } | ||
| NOT_REACHED_GCOVR_EXCL_LINE; | ||
| } | ||
|
|
||
| void AppleDnsResolverImpl::PendingResolution::finishResolve() { | ||
| ENVOY_LOG_EVENT(debug, "apple_dns_resolution_complete", | ||
| "dns resolution for {} completed with status {}", dns_name_, | ||
| pending_response_.status_); | ||
| callback_(pending_response_.status_, std::move(finalAddressList())); | ||
|
|
||
| if (owned_) { | ||
| ENVOY_LOG(debug, "Resolution for {} completed (async)", dns_name_); | ||
|
|
@@ -233,6 +244,7 @@ DNSServiceErrorType AppleDnsResolverImpl::PendingResolution::dnsServiceGetAddrIn | |
| break; | ||
| case DnsLookupFamily::Auto: | ||
| case DnsLookupFamily::V4Preferred: | ||
| case DnsLookupFamily::All: | ||
| protocol = kDNSServiceProtocol_IPv4 | kDNSServiceProtocol_IPv6; | ||
| break; | ||
| } | ||
|
|
@@ -277,9 +289,9 @@ void AppleDnsResolverImpl::PendingResolution::onDNSServiceGetAddrInfoReply( | |
| if (error_code != kDNSServiceErr_NoError) { | ||
| parent_.chargeGetAddrInfoErrorStats(error_code); | ||
|
|
||
| pending_response_.status_ = ResolutionStatus::Failure; | ||
| pending_response_.v4_responses_.clear(); | ||
| pending_response_.v6_responses_.clear(); | ||
|
|
||
| finishResolve(); | ||
| // Note: Nothing can follow this call to flushPendingQueries due to deletion of this | ||
|
|
@@ -296,10 +308,10 @@ void AppleDnsResolverImpl::PendingResolution::onDNSServiceGetAddrInfoReply( | |
| ENVOY_LOG(debug, "Address to add address={}, ttl={}", | ||
| dns_response.address_->ip()->addressAsString(), ttl); | ||
| if (dns_response.address_->ip()->ipv4()) { | ||
| pending_response_.v4_responses_.push_back(dns_response); | ||
| } else { | ||
| ASSERT(dns_response.address_->ip()->ipv6()); | ||
| pending_response_.v6_responses_.push_back(dns_response); | ||
| } | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -104,16 +104,24 @@ void DnsResolverImpl::PendingResolution::onAresGetAddrInfoCallback(int status, i | |
| // ARES_ECONNREFUSED. If the PendingResolution has not been cancelled that means that the | ||
|
Member
Author
There was a problem hiding this comment. The c-ares based resolver changed a bit more because it was not originally written to accumulate responses from multiple c-ares callbacks.
Member
Author
There was a problem hiding this comment. I refactored for ease of understanding but open to comments to simplify further! |
||
| // callback_ target _should_ still be around. In that case, raise the callback_ so the target | ||
| // can be done with this query and initiate a new one. | ||
| ENVOY_LOG_EVENT(debug, "cares_dns_resolution_destroyed", "dns resolution for {} destroyed", | ||
| dns_name_); | ||
|
|
||
| if (!pending_response_.address_list_.empty()) { | ||
| ASSERT(dns_lookup_family_ == DnsLookupFamily::All && !dual_resolution_); | ||
|
Member
Author
There was a problem hiding this comment. Here and below I considered a failure in the second resolution an overall success if the first resolution was successful, and hence fired the callback with the existing addresses and a success state. Up to discussing if this should be the behavior.
Contributor
There was a problem hiding this comment. yeah IMO a resolve success of either v4 or v6 should be considered success.
Contributor
There was a problem hiding this comment. +1, success of v4 OR v6 should mean overall success since we have usable addresses |
||
| // The only way the resolver would have addresses here is if it is configured to resolve All | ||
| // families, and the first resolution was successful. In that case, we might as well return | ||
| // the addresses that resolved as a success. | ||
| } else { | ||
| pending_response_.status_ = ResolutionStatus::Failure; | ||
| } | ||
| // Nothing can follow a call to finishResolve due to the deletion of this object upon | ||
| // finishResolve(). | ||
| finishResolve(); | ||
| return; | ||
| } | ||
|
|
||
| if (!dual_resolution_) { | ||
|
Contributor
There was a problem hiding this comment. hm, are we holding all forward progress on the dns resolver returning v4 and v6 addreses? That might work for a first pass but I believe that the chrome folks have told me it's a bad idea in general for latency / Quality of Experience reasons. If they confirm, I think we need a TODO to handle ipv4 and ipv6 asynchronously.
Member
Author
There was a problem hiding this comment. Yeah I was thinking about this as well from two different angles:
(1) is easy and I could do that in this PR. (2) is a bit harder, and I wouldn't want to do it in this PR.
Member
There was a problem hiding this comment. Personally I would do (1) and not (2) for now.
Member
Author
There was a problem hiding this comment. @alyssawilk @mattklein123 re:(1) do we have a preference for all three modes (
Contributor
There was a problem hiding this comment. Hm, less sure on that - I'll defer to Matt and David.
Member
Author
There was a problem hiding this comment. @alyssawilk @mattklein123 I settled on doing both lookups concurrrently for I am already part of the way on the TODO, so will put that up as a separate PR.
Contributor
There was a problem hiding this comment. Agree that waiting for both means users are waiting unnecessarily. But tackling in a separate PR with TODO here sounds good
Contributor
There was a problem hiding this comment. SG. Do we have a TODO on not waiting for both?
Contributor
There was a problem hiding this comment. This definitely seems like a good way to handle this for now. While suboptimal because it block progres until both return, Chrome does exactly this today. Solving #18572 (later) is a great chance to make envoy mobile better than Chrome! |
||
| completed_ = true; | ||
|
|
||
| // If c-ares returns ARES_ECONNREFUSED and there is no fallback we assume that the channel_ is | ||
|
|
@@ -130,10 +138,8 @@ void DnsResolverImpl::PendingResolution::onAresGetAddrInfoCallback(int status, i | |
| } | ||
| } | ||
|
|
||
| if (status == ARES_SUCCESS) { | ||
| pending_response_.status_ = ResolutionStatus::Success; | ||
| if (addrinfo != nullptr && addrinfo->nodes != nullptr) { | ||
| if (addrinfo->nodes->ai_family == AF_INET) { | ||
| for (const ares_addrinfo_node* ai = addrinfo->nodes; ai != nullptr; ai = ai->ai_next) { | ||
|
|
@@ -143,7 +149,7 @@ void DnsResolverImpl::PendingResolution::onAresGetAddrInfoCallback(int status, i | |
| address.sin_port = 0; | ||
| address.sin_addr = reinterpret_cast<sockaddr_in*>(ai->ai_addr)->sin_addr; | ||
|
|
||
| pending_response_.address_list_.emplace_back( | ||
| DnsResponse(std::make_shared<const Address::Ipv4Instance>(&address), | ||
| std::chrono::seconds(ai->ai_ttl))); | ||
| } | ||
|
|
@@ -154,63 +160,47 @@ void DnsResolverImpl::PendingResolution::onAresGetAddrInfoCallback(int status, i | |
| address.sin6_family = AF_INET6; | ||
| address.sin6_port = 0; | ||
| address.sin6_addr = reinterpret_cast<sockaddr_in6*>(ai->ai_addr)->sin6_addr; | ||
| pending_response_.address_list_.emplace_back( | ||
| DnsResponse(std::make_shared<const Address::Ipv6Instance>(address), | ||
| std::chrono::seconds(ai->ai_ttl))); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (!pending_response_.address_list_.empty() && dns_lookup_family_ != DnsLookupFamily::All) { | ||
| completed_ = true; | ||
| } | ||
|
|
||
| ASSERT(addrinfo != nullptr); | ||
| ares_freeaddrinfo(addrinfo); | ||
| } else { | ||
| // FIXME: if the failure happens on the second resolution of DnsLookupFamily::All, should this | ||
| // return the v6 addresses that it has from the first resolution? | ||
| if (!pending_response_.address_list_.empty()) { | ||
| ASSERT(dns_lookup_family_ == DnsLookupFamily::All && !dual_resolution_); | ||
| // The only way the resolver would have addresses here is if it is configured to resolve All | ||
| // families, and the first resolution was successful. In that case, we might as well return | ||
| // the addresses that resolved. | ||
| } else { | ||
| pending_response_.status_ = ResolutionStatus::Failure; | ||
| } | ||
| } | ||
|
|
||
| if (timeouts > 0) { | ||
| ENVOY_LOG(debug, "DNS request timed out {} times", timeouts); | ||
| } | ||
|
|
||
| if (completed_) { | ||
| finishResolve(); | ||
| // Nothing can follow a call to finishResolve due to the deletion of this object upon | ||
| // finishResolve(). | ||
| return; | ||
| } | ||
|
|
||
| if (dual_resolution_) { | ||
| dual_resolution_ = false; | ||
|
|
||
| if (dns_lookup_family_ == DnsLookupFamily::Auto || dns_lookup_family_ == DnsLookupFamily::All) { | ||
| getAddrInfo(AF_INET); | ||
| } else { | ||
| ASSERT(dns_lookup_family_ == DnsLookupFamily::V4Preferred); | ||
|
|
@@ -223,6 +213,40 @@ void DnsResolverImpl::PendingResolution::onAresGetAddrInfoCallback(int status, i | |
| } | ||
| } | ||
|
|
||
| void DnsResolverImpl::PendingResolution::finishResolve() { | ||
| if (!cancelled_) { | ||
| // Use a raw try here because it is used in both main thread and filter. | ||
| // Can not convert to use status code as there may be unexpected exceptions in server fuzz | ||
| // tests, which must be handled. Potential exception may come from getAddressWithPort() or | ||
| // portFromTcpUrl(). | ||
| // TODO(chaoqin-li1123): remove try catch pattern here once we figure how to handle unexpected | ||
| // exception in fuzz tests. | ||
| ENVOY_LOG_EVENT(debug, "cares_dns_resolution_complete", | ||
| "dns resolution for {} completed with status {}", dns_name_, | ||
| pending_response_.status_); | ||
|
|
||
| TRY_NEEDS_AUDIT { | ||
|
Contributor
There was a problem hiding this comment. Does TRY_NEEDS_AUDIT imply that we're trying to avoids exceptions on the data plane but we're required to use them here? (Is there an actual audit happening?)
Member
Author
There was a problem hiding this comment. yeah I think there is a push to avoid exceptions on the data plane, except some times on the main thread. But this code does not always run on the main thread so it was left as one of the remaining uses of try that needs to be addressed. Looks like @chaoqin-li1123 has a TODO to clean up. |
||
| callback_(pending_response_.status_, std::move(pending_response_.address_list_)); | ||
| } | ||
| catch (const EnvoyException& e) { | ||
| ENVOY_LOG(critical, "EnvoyException in c-ares callback: {}", e.what()); | ||
| dispatcher_.post([s = std::string(e.what())] { throw EnvoyException(s); }); | ||
| } | ||
| catch (const std::exception& e) { | ||
| ENVOY_LOG(critical, "std::exception in c-ares callback: {}", e.what()); | ||
| dispatcher_.post([s = std::string(e.what())] { throw EnvoyException(s); }); | ||
| } | ||
| catch (...) { | ||
| ENVOY_LOG(critical, "Unknown exception in c-ares callback"); | ||
| dispatcher_.post([] { throw EnvoyException("unknown"); }); | ||
| } | ||
| } | ||
| if (owned_) { | ||
| delete this; | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| void DnsResolverImpl::updateAresTimer() { | ||
| // Update the timeout for events. | ||
| timeval timeout; | ||
|
|
@@ -283,8 +307,9 @@ ActiveDnsQuery* DnsResolverImpl::resolve(const std::string& dns_name, | |
| auto pending_resolution = std::make_unique<PendingResolution>( | ||
| *this, callback, dispatcher_, channel_, dns_name, dns_lookup_family); | ||
| if (dns_lookup_family == DnsLookupFamily::Auto || | ||
| dns_lookup_family == DnsLookupFamily::V4Preferred || | ||
| dns_lookup_family == DnsLookupFamily::All) { | ||
| pending_resolution->dual_resolution_ = true; | ||
| } | ||
|
|
||
| if (dns_lookup_family == DnsLookupFamily::V4Only || | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.