Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -15,4 +15,12 @@ option (udpa.annotations.file_status).package_version_status = ACTIVE;

// Configuration for apple DNS resolver.
message AppleDnsResolverConfig {
// The resolver will avoid the system's heuristics to only return
// IPv4 or IPv6 addresses that it considers to be "routable", instead
// returning all possible IPv4 or IPv6 addresses. This setting is
// ignored if the DNS lookup family is set to v4-only or v6-only.
bool include_unroutable_families = 1;
Comment on lines +18 to +25

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This makes sense to me, but potentially mention that is useful with happy eyeballs and for parity with the c-ares implementation? I think it will make it more clear why someone would want to do this and answer @htuch concerns.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, definitely, this totally makes sense with the mention of Happy Eyeballs.

// TODO(jpsim): Add tests
// TODO(jpsim): Add docs
// TODO(jpsim): Add release notes
}
31 changes: 22 additions & 9 deletions source/extensions/network/dns_resolver/apple/apple_dns_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -46,9 +46,12 @@ DNSServiceErrorType DnsService::dnsServiceGetAddrInfo(DNSServiceRef* sdRef, DNSS
return DNSServiceGetAddrInfo(sdRef, flags, interfaceIndex, protocol, hostname, callBack, context);
}

AppleDnsResolverImpl::AppleDnsResolverImpl(Event::Dispatcher& dispatcher, Stats::Scope& root_scope)
AppleDnsResolverImpl::AppleDnsResolverImpl(
const envoy::extensions::network::dns_resolver::apple::v3::AppleDnsResolverConfig& proto_config,
Event::Dispatcher& dispatcher, Stats::Scope& root_scope)
: dispatcher_(dispatcher), scope_(root_scope.createScope("dns.apple.")),
stats_(generateAppleDnsResolverStats(*scope_)) {}
stats_(generateAppleDnsResolverStats(*scope_)),
include_unroutable_families_(proto_config.include_unroutable_families()) {}

AppleDnsResolverStats AppleDnsResolverImpl::generateAppleDnsResolverStats(Stats::Scope& scope) {
return {ALL_APPLE_DNS_RESOLVER_STATS(POOL_COUNTER(scope))};
Expand Down Expand Up @@ -78,7 +81,8 @@ AppleDnsResolverImpl::startResolution(const std::string& dns_name,
auto pending_resolution = std::make_unique<PendingResolution>(*this, callback, dispatcher_,
dns_name, dns_lookup_family);

DNSServiceErrorType error = pending_resolution->dnsServiceGetAddrInfo();
DNSServiceErrorType error =
pending_resolution->dnsServiceGetAddrInfo(include_unroutable_families_);
if (error != kDNSServiceErr_NoError) {
ENVOY_LOG(warn, "DNS resolver error ({}) in dnsServiceGetAddrInfo for {}", error, dns_name);
chargeGetAddrInfoErrorStats(error);
Expand Down Expand Up @@ -238,7 +242,8 @@ void AppleDnsResolverImpl::PendingResolution::finishResolve() {
}
}

DNSServiceErrorType AppleDnsResolverImpl::PendingResolution::dnsServiceGetAddrInfo() {
DNSServiceErrorType
AppleDnsResolverImpl::PendingResolution::dnsServiceGetAddrInfo(bool include_unroutable_families) {
DNSServiceProtocol protocol;
switch (dns_lookup_family_) {
case DnsLookupFamily::V4Only:
Expand All @@ -250,6 +255,11 @@ DNSServiceErrorType AppleDnsResolverImpl::PendingResolution::dnsServiceGetAddrIn
case DnsLookupFamily::Auto:
case DnsLookupFamily::V4Preferred:
case DnsLookupFamily::All:
if (include_unroutable_families) {
protocol = kDNSServiceProtocol_IPv4 | kDNSServiceProtocol_IPv6;
break;
}

/* We want to make sure we don't get any address that is not routable. Passing 0
* to apple's `DNSServiceGetAddrInfo` will make a best attempt to filter out IPv6
* or IPv4 addresses depending on what's routable, per Apple's documentation:
Expand Down Expand Up @@ -280,7 +290,7 @@ DNSServiceErrorType AppleDnsResolverImpl::PendingResolution::dnsServiceGetAddrIn
* that DNSServiceRef.
*/

// Therefore, much like the c-ares implementation All calls and callbacks to the API need to
// Therefore, much like the c-ares implementation, all calls and callbacks to the API need to
// happen on the thread that owns the creating dispatcher. This is the case as callbacks are
// driven by processing bytes in onEventCallback which run on the passed in dispatcher's event
// loop.
Expand Down Expand Up @@ -393,11 +403,14 @@ class AppleDnsResolverFactory : public DnsResolverFactory {
return ProtobufTypes::MessagePtr{
new envoy::extensions::network::dns_resolver::apple::v3::AppleDnsResolverConfig()};
}
DnsResolverSharedPtr
createDnsResolver(Event::Dispatcher& dispatcher, Api::Api& api,
const envoy::config::core::v3::TypedExtensionConfig&) const override {

DnsResolverSharedPtr createDnsResolver(Event::Dispatcher& dispatcher, Api::Api& api,
const envoy::config::core::v3::TypedExtensionConfig&
typed_dns_resolver_config) const override {
ASSERT(dispatcher.isThreadSafe());
return std::make_shared<Network::AppleDnsResolverImpl>(dispatcher, api.rootScope());
envoy::extensions::network::dns_resolver::apple::v3::AppleDnsResolverConfig apple;
Envoy::MessageUtil::unpackTo(typed_dns_resolver_config.typed_config(), apple);
return std::make_shared<Network::AppleDnsResolverImpl>(apple, dispatcher, api.rootScope());
}
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
#include "source/common/common/linked_object.h"
#include "source/common/common/logger.h"
#include "source/common/common/utility.h"
#include "source/common/network/dns_resolver/dns_factory_util.h"
#include "source/common/singleton/threadsafe_singleton.h"

#include "absl/container/node_hash_map.h"
Expand Down Expand Up @@ -64,7 +65,10 @@ struct AppleDnsResolverStats {
*/
class AppleDnsResolverImpl : public DnsResolver, protected Logger::Loggable<Logger::Id::dns> {
public:
AppleDnsResolverImpl(Event::Dispatcher& dispatcher, Stats::Scope& root_scope);
AppleDnsResolverImpl(
const envoy::extensions::network::dns_resolver::apple::v3::AppleDnsResolverConfig&
proto_config,
Event::Dispatcher& dispatcher, Stats::Scope& root_scope);

static AppleDnsResolverStats generateAppleDnsResolverStats(Stats::Scope& scope);

Expand Down Expand Up @@ -99,7 +103,7 @@ class AppleDnsResolverImpl : public DnsResolver, protected Logger::Loggable<Logg
void finishResolve();

// Wrappers for the API calls.
DNSServiceErrorType dnsServiceGetAddrInfo();
DNSServiceErrorType dnsServiceGetAddrInfo(bool include_unroutable_families);
void onDNSServiceGetAddrInfoReply(DNSServiceFlags flags, uint32_t interface_index,
DNSServiceErrorType error_code, const char* hostname,
const struct sockaddr* address, uint32_t ttl);
Expand Down Expand Up @@ -138,6 +142,7 @@ class AppleDnsResolverImpl : public DnsResolver, protected Logger::Loggable<Logg
BackOffStrategyPtr backoff_strategy_;
Stats::ScopePtr scope_;
AppleDnsResolverStats stats_;
bool include_unroutable_families_;
};

DECLARE_FACTORY(AppleDnsResolverFactory);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -385,7 +385,8 @@ TEST_F(AppleDnsImplTest, LocalResolution) {
class AppleDnsImplFakeApiTest : public testing::Test {
public:
void SetUp() override {
resolver_ = std::make_unique<Network::AppleDnsResolverImpl>(dispatcher_, stats_store_);
const envoy::extensions::network::dns_resolver::apple::v3::AppleDnsResolverConfig config;
resolver_ = std::make_unique<Network::AppleDnsResolverImpl>(config, dispatcher_, stats_store_);
}

void checkErrorStat(DNSServiceErrorType error_code) {
Expand Down