From 886270a3262c114724a45139b38e1df745841b90 Mon Sep 17 00:00:00 2001 From: JP Simard Date: Thu, 24 Mar 2022 22:06:09 -0400 Subject: [PATCH 1/7] dns: Add `AppleDnsResolverConfig.include_unroutable_families` config If set, 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. This may be a useful setting to specify if the addresses considered unroutable by the system's heuristics may in practice be routable. Signed-off-by: JP Simard --- .../apple/v3/apple_dns_resolver.proto | 8 +++++ .../dns_resolver/apple/apple_dns_impl.cc | 31 +++++++++++++------ .../dns_resolver/apple/apple_dns_impl.h | 9 ++++-- 3 files changed, 37 insertions(+), 11 deletions(-) diff --git a/api/envoy/extensions/network/dns_resolver/apple/v3/apple_dns_resolver.proto b/api/envoy/extensions/network/dns_resolver/apple/v3/apple_dns_resolver.proto index c27d1c78276a3..b6e737dd55832 100644 --- a/api/envoy/extensions/network/dns_resolver/apple/v3/apple_dns_resolver.proto +++ b/api/envoy/extensions/network/dns_resolver/apple/v3/apple_dns_resolver.proto @@ -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; + // TODO(jpsim): Add tests + // TODO(jpsim): Add docs + // TODO(jpsim): Add release notes } 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 60476112f0eb9..35e84c186a014 100644 --- a/source/extensions/network/dns_resolver/apple/apple_dns_impl.cc +++ b/source/extensions/network/dns_resolver/apple/apple_dns_impl.cc @@ -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))}; @@ -78,7 +81,8 @@ AppleDnsResolverImpl::startResolution(const std::string& dns_name, auto pending_resolution = std::make_unique(*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); @@ -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: @@ -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: @@ -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. @@ -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(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(apple, dispatcher, api.rootScope()); } }; diff --git a/source/extensions/network/dns_resolver/apple/apple_dns_impl.h b/source/extensions/network/dns_resolver/apple/apple_dns_impl.h index 421d14fe29e73..40883bd951071 100644 --- a/source/extensions/network/dns_resolver/apple/apple_dns_impl.h +++ b/source/extensions/network/dns_resolver/apple/apple_dns_impl.h @@ -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" @@ -64,7 +65,10 @@ struct AppleDnsResolverStats { */ class AppleDnsResolverImpl : public DnsResolver, protected Logger::Loggable { 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); @@ -99,7 +103,7 @@ class AppleDnsResolverImpl : public DnsResolver, protected Logger::Loggable Date: Fri, 25 Mar 2022 13:53:05 -0400 Subject: [PATCH 2/7] Fix tests Will add more to test the new functionality soon. Signed-off-by: JP Simard --- .../network/dns_resolver/apple/apple_dns_impl_test.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) 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 0476228605d00..548c241a0b942 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 @@ -385,7 +385,8 @@ TEST_F(AppleDnsImplTest, LocalResolution) { class AppleDnsImplFakeApiTest : public testing::Test { public: void SetUp() override { - resolver_ = std::make_unique(dispatcher_, stats_store_); + const envoy::extensions::network::dns_resolver::apple::v3::AppleDnsResolverConfig config; + resolver_ = std::make_unique(config, dispatcher_, stats_store_); } void checkErrorStat(DNSServiceErrorType error_code) { From 76a968cae4680d79b70fb02e2787ab7d391a571e Mon Sep 17 00:00:00 2001 From: JP Simard Date: Tue, 19 Apr 2022 14:40:13 -0400 Subject: [PATCH 3/7] Mention Happy Eyeballs in the proto comment. Signed-off-by: JP Simard --- .../network/dns_resolver/apple/v3/apple_dns_resolver.proto | 3 +++ 1 file changed, 3 insertions(+) diff --git a/api/envoy/extensions/network/dns_resolver/apple/v3/apple_dns_resolver.proto b/api/envoy/extensions/network/dns_resolver/apple/v3/apple_dns_resolver.proto index b6e737dd55832..5efe819af43fa 100644 --- a/api/envoy/extensions/network/dns_resolver/apple/v3/apple_dns_resolver.proto +++ b/api/envoy/extensions/network/dns_resolver/apple/v3/apple_dns_resolver.proto @@ -19,6 +19,9 @@ message AppleDnsResolverConfig { // 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. + // This should remain false in the vast majority of cases, but may be + // useful when performing custom filtering of addresses, such as with + // Happy Eyeballs. bool include_unroutable_families = 1; // TODO(jpsim): Add tests // TODO(jpsim): Add docs From ac8594bc8b5fd87ec81b225c25ebc68f57e200ba Mon Sep 17 00:00:00 2001 From: JP Simard Date: Tue, 19 Apr 2022 14:44:19 -0400 Subject: [PATCH 4/7] Add release notes Signed-off-by: JP Simard --- .../network/dns_resolver/apple/v3/apple_dns_resolver.proto | 1 - docs/root/version_history/current.rst | 1 + 2 files changed, 1 insertion(+), 1 deletion(-) diff --git a/api/envoy/extensions/network/dns_resolver/apple/v3/apple_dns_resolver.proto b/api/envoy/extensions/network/dns_resolver/apple/v3/apple_dns_resolver.proto index 5efe819af43fa..d152385bf3b54 100644 --- a/api/envoy/extensions/network/dns_resolver/apple/v3/apple_dns_resolver.proto +++ b/api/envoy/extensions/network/dns_resolver/apple/v3/apple_dns_resolver.proto @@ -25,5 +25,4 @@ message AppleDnsResolverConfig { bool include_unroutable_families = 1; // TODO(jpsim): Add tests // TODO(jpsim): Add docs - // TODO(jpsim): Add release notes } diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index c809a807f674f..d6016eb490b48 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -24,6 +24,7 @@ Removed Config or Runtime New Features ------------ * ext_proc: added support for per-route :ref:`grpc_service `. +* dns_resolver: added :ref:`include_unroutable_families` to the Apple DNS resolver. Deprecated ---------- From 73fb54ded48fd1372960129e6e34d91021a02031 Mon Sep 17 00:00:00 2001 From: JP Simard Date: Tue, 19 Apr 2022 15:16:38 -0400 Subject: [PATCH 5/7] fixup! Add release notes Signed-off-by: JP Simard --- docs/root/version_history/current.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index d6016eb490b48..fd3362ad78339 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -24,7 +24,7 @@ Removed Config or Runtime New Features ------------ * ext_proc: added support for per-route :ref:`grpc_service `. -* dns_resolver: added :ref:`include_unroutable_families` to the Apple DNS resolver. +* dns_resolver: added :ref:`include_unroutable_families` to the Apple DNS resolver. Deprecated ---------- From 1604240a3576691f85a95a63cd4d67cc8f4f8c15 Mon Sep 17 00:00:00 2001 From: JP Simard Date: Tue, 19 Apr 2022 15:25:16 -0400 Subject: [PATCH 6/7] fixup! fixup! Add release notes Signed-off-by: JP Simard --- docs/root/version_history/current.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index fd3362ad78339..fff25b62f318e 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -23,8 +23,8 @@ Removed Config or Runtime New Features ------------ -* ext_proc: added support for per-route :ref:`grpc_service `. * dns_resolver: added :ref:`include_unroutable_families` to the Apple DNS resolver. +* ext_proc: added support for per-route :ref:`grpc_service `. Deprecated ---------- From a4589fe322c61c828ef4f37e5e3009c876c66bf1 Mon Sep 17 00:00:00 2001 From: JP Simard Date: Tue, 19 Apr 2022 15:59:57 -0400 Subject: [PATCH 7/7] Add tests Signed-off-by: JP Simard --- .../apple/v3/apple_dns_resolver.proto | 2 - .../dns_resolver/apple/apple_dns_impl_test.cc | 56 ++++++++++++++++++- 2 files changed, 54 insertions(+), 4 deletions(-) diff --git a/api/envoy/extensions/network/dns_resolver/apple/v3/apple_dns_resolver.proto b/api/envoy/extensions/network/dns_resolver/apple/v3/apple_dns_resolver.proto index d152385bf3b54..60588ca848a21 100644 --- a/api/envoy/extensions/network/dns_resolver/apple/v3/apple_dns_resolver.proto +++ b/api/envoy/extensions/network/dns_resolver/apple/v3/apple_dns_resolver.proto @@ -23,6 +23,4 @@ message AppleDnsResolverConfig { // useful when performing custom filtering of addresses, such as with // Happy Eyeballs. bool include_unroutable_families = 1; - // TODO(jpsim): Add tests - // TODO(jpsim): Add docs } 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 9f3c88eeecbaf..a96e346ccf2cf 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 @@ -386,8 +386,8 @@ TEST_F(AppleDnsImplTest, LocalResolution) { class AppleDnsImplFakeApiTest : public testing::Test { public: void SetUp() override { - const envoy::extensions::network::dns_resolver::apple::v3::AppleDnsResolverConfig config; - resolver_ = std::make_unique(config, dispatcher_, stats_store_); + config_.set_include_unroutable_families(false); + resolver_ = std::make_unique(config_, dispatcher_, stats_store_); } void checkErrorStat(DNSServiceErrorType error_code) { @@ -560,11 +560,22 @@ class AppleDnsImplFakeApiTest : public testing::Test { TestThreadsafeSingletonInjector dns_service_injector_{&dns_service_}; Stats::IsolatedStoreImpl stats_store_; std::unique_ptr resolver_{}; + envoy::extensions::network::dns_resolver::apple::v3::AppleDnsResolverConfig config_; NiceMock dispatcher_; NiceMock* file_event_; Event::FileReadyCb file_ready_cb_; }; +TEST_F(AppleDnsImplFakeApiTest, IncludeUnroutableFamiliesConfigFalse) { + config_.set_include_unroutable_families(false); + EXPECT_EQ(false, config_.include_unroutable_families()); +} + +TEST_F(AppleDnsImplFakeApiTest, IncludeUnroutableFamiliesConfigTrue) { + config_.set_include_unroutable_families(true); + EXPECT_EQ(true, config_.include_unroutable_families()); +} + TEST_F(AppleDnsImplFakeApiTest, ErrorInSocketAccess) { const std::string hostname = "foo.com"; sockaddr_in addr4; @@ -692,6 +703,47 @@ TEST_F(AppleDnsImplFakeApiTest, SynchronousTimeoutInGetAddrInfo) { synchronousWithError(kDNSServiceErr_Timeout); } +TEST_F(AppleDnsImplFakeApiTest, QuerySynchronousCompletionUnroutableFamilies) { + config_.set_include_unroutable_families(true); + resolver_ = std::make_unique(config_, dispatcher_, stats_store_); + + const std::string hostname = "foo.com"; + sockaddr_in addr4; + memset(&addr4, 0, sizeof(addr4)); + addr4.sin_family = AF_INET; + EXPECT_EQ(1, inet_pton(AF_INET, "1.2.3.4", &addr4.sin_addr)); + addr4.sin_port = htons(6502); + + Network::Address::Ipv4Instance address(&addr4); + absl::Notification dns_callback_executed; + + EXPECT_CALL(dns_service_, + dnsServiceGetAddrInfo(_, kDNSServiceFlagsTimeout, 0, + kDNSServiceProtocol_IPv4 | kDNSServiceProtocol_IPv6, + StrEq(hostname.c_str()), _, _)) + .WillOnce(DoAll( + // Have the API call synchronously call the provided callback. + WithArgs<5, 6>(Invoke([&](DNSServiceGetAddrInfoReply callback, void* context) -> void { + callback(nullptr, kDNSServiceFlagsAdd, 0, kDNSServiceErr_NoError, hostname.c_str(), + address.sockAddr(), 30, context); + })), + Return(kDNSServiceErr_NoError))); + + // The returned value is nullptr because the query has already been fulfilled. Verify that the + // callback ran via notification. + EXPECT_EQ(nullptr, resolver_->resolve( + hostname, Network::DnsLookupFamily::Auto, + [&dns_callback_executed](DnsResolver::ResolutionStatus status, + std::list&& response) -> void { + EXPECT_EQ(DnsResolver::ResolutionStatus::Success, status); + 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_); + dns_callback_executed.Notify(); + })); + dns_callback_executed.WaitForNotification(); +} + TEST_F(AppleDnsImplFakeApiTest, QuerySynchronousCompletion) { const std::string hostname = "foo.com"; sockaddr_in addr4;