From 0be92abafb39a12513ffbfc2d83a18c072dd4386 Mon Sep 17 00:00:00 2001 From: Martin Conte Mac Donell Date: Mon, 4 Oct 2021 14:05:03 -0700 Subject: [PATCH 1/7] dns ios: attempt to filter out unroutable addresses category Signed-off-by: Martin Conte Mac Donell --- source/common/network/apple_dns_impl.cc | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/source/common/network/apple_dns_impl.cc b/source/common/network/apple_dns_impl.cc index 9a423621acbf3..c828f4c0e66f2 100644 --- a/source/common/network/apple_dns_impl.cc +++ b/source/common/network/apple_dns_impl.cc @@ -233,7 +233,19 @@ DNSServiceErrorType AppleDnsResolverImpl::PendingResolution::dnsServiceGetAddrIn break; case DnsLookupFamily::Auto: case DnsLookupFamily::V4Preferred: - protocol = kDNSServiceProtocol_IPv4 | kDNSServiceProtocol_IPv6; + /* 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: + * + * If neither flag is set, the system will apply an intelligent heuristic, which + * is (currently) that it will attempt to look up both, except: + * If "hostname" is a wide-area unicast DNS hostname (i.e. not a ".local." name) but + * this host has no routable IPv6 address, then the call will not try to look up IPv6 + * addresses for "hostname", since any addresses it found would be unlikely to be of + * any use anyway. Similarly, if this host has no routable IPv4 address, the call will + * not try to look up IPv4 addresses for "hostname". + */ + protocol = 0 break; } From f98ed9a00536499468f42769458426a4315a0b0a Mon Sep 17 00:00:00 2001 From: Martin Conte Mac Donell Date: Tue, 5 Oct 2021 10:03:04 -0700 Subject: [PATCH 2/7] Add 'routable' to the spelling dictionary Signed-off-by: Martin Conte Mac Donell --- tools/spelling/spelling_dictionary.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/tools/spelling/spelling_dictionary.txt b/tools/spelling/spelling_dictionary.txt index f9d05beace12e..f5c2c0b8d08b8 100644 --- a/tools/spelling/spelling_dictionary.txt +++ b/tools/spelling/spelling_dictionary.txt @@ -1300,3 +1300,4 @@ crlf ep suri transid +routable From b694de6ba2635f1c99bb192380632915060bac34 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mart=C3=ADn=20Conte=20Mac=20Donell?= Date: Tue, 5 Oct 2021 11:06:43 -0700 Subject: [PATCH 3/7] Apply suggestions from code review Co-authored-by: Jose Ulises Nino Rivera Signed-off-by: Martin Conte Mac Donell --- source/common/network/apple_dns_impl.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/source/common/network/apple_dns_impl.cc b/source/common/network/apple_dns_impl.cc index c828f4c0e66f2..a5b39ce1d1d54 100644 --- a/source/common/network/apple_dns_impl.cc +++ b/source/common/network/apple_dns_impl.cc @@ -233,7 +233,7 @@ DNSServiceErrorType AppleDnsResolverImpl::PendingResolution::dnsServiceGetAddrIn break; case DnsLookupFamily::Auto: case DnsLookupFamily::V4Preferred: - /* We want to make sure we don't get any address that is not routable. Passing 0 + /* 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: * @@ -245,7 +245,7 @@ DNSServiceErrorType AppleDnsResolverImpl::PendingResolution::dnsServiceGetAddrIn * any use anyway. Similarly, if this host has no routable IPv4 address, the call will * not try to look up IPv4 addresses for "hostname". */ - protocol = 0 + protocol = 0; break; } From aa40bda637b70a77b4577fff97a17e34e36c6184 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Tue, 5 Oct 2021 17:20:40 -0700 Subject: [PATCH 4/7] fix tests Signed-off-by: Jose Nino --- test/common/network/apple_dns_impl_test.cc | 78 ++++++++-------------- 1 file changed, 26 insertions(+), 52 deletions(-) diff --git a/test/common/network/apple_dns_impl_test.cc b/test/common/network/apple_dns_impl_test.cc index b7e7904ad7e68..8ce3b131a00f0 100644 --- a/test/common/network/apple_dns_impl_test.cc +++ b/test/common/network/apple_dns_impl_test.cc @@ -305,10 +305,8 @@ class AppleDnsImplFakeApiTest : public testing::Test { 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()), _, _)) + EXPECT_CALL(dns_service_, dnsServiceGetAddrInfo(_, kDNSServiceFlagsTimeout, 0, 0, + StrEq(hostname.c_str()), _, _)) .WillOnce(DoAll( // Have the API call synchronously call the provided callback. WithArgs<5, 6>(Invoke([&](DNSServiceGetAddrInfoReply callback, void* context) -> void { @@ -349,10 +347,8 @@ class AppleDnsImplFakeApiTest : public testing::Test { DNSServiceGetAddrInfoReply reply_callback; absl::Notification dns_callback_executed; - EXPECT_CALL(dns_service_, - dnsServiceGetAddrInfo(_, kDNSServiceFlagsTimeout, 0, - kDNSServiceProtocol_IPv4 | kDNSServiceProtocol_IPv6, - StrEq(hostname.c_str()), _, _)) + EXPECT_CALL(dns_service_, dnsServiceGetAddrInfo(_, kDNSServiceFlagsTimeout, 0, 0, + StrEq(hostname.c_str()), _, _)) .WillOnce(DoAll(SaveArg<5>(&reply_callback), Return(kDNSServiceErr_NoError))); EXPECT_CALL(dns_service_, dnsServiceRefSockFD(_)).WillOnce(Return(0)); @@ -429,10 +425,8 @@ TEST_F(AppleDnsImplFakeApiTest, ErrorInSocketAccess) { DNSServiceGetAddrInfoReply reply_callback; absl::Notification dns_callback_executed; - EXPECT_CALL(dns_service_, - dnsServiceGetAddrInfo(_, kDNSServiceFlagsTimeout, 0, - kDNSServiceProtocol_IPv4 | kDNSServiceProtocol_IPv6, - StrEq(hostname.c_str()), _, _)) + EXPECT_CALL(dns_service_, dnsServiceGetAddrInfo(_, kDNSServiceFlagsTimeout, 0, 0, + StrEq(hostname.c_str()), _, _)) .WillOnce(DoAll(SaveArg<5>(&reply_callback), Return(kDNSServiceErr_NoError))); EXPECT_CALL(dns_service_, dnsServiceRefSockFD(_)).WillOnce(Return(-1)); @@ -466,10 +460,8 @@ TEST_F(AppleDnsImplFakeApiTest, InvalidFileEvent) { DNSServiceGetAddrInfoReply reply_callback; absl::Notification dns_callback_executed; - EXPECT_CALL(dns_service_, - dnsServiceGetAddrInfo(_, kDNSServiceFlagsTimeout, 0, - kDNSServiceProtocol_IPv4 | kDNSServiceProtocol_IPv6, - StrEq(hostname.c_str()), _, _)) + EXPECT_CALL(dns_service_, dnsServiceGetAddrInfo(_, kDNSServiceFlagsTimeout, 0, 0, + StrEq(hostname.c_str()), _, _)) .WillOnce(DoAll(SaveArg<5>(&reply_callback), Return(kDNSServiceErr_NoError))); EXPECT_CALL(dns_service_, dnsServiceRefSockFD(_)).WillOnce(Return(0)); @@ -503,10 +495,8 @@ TEST_F(AppleDnsImplFakeApiTest, ErrorInProcessResult) { DNSServiceGetAddrInfoReply reply_callback; absl::Notification dns_callback_executed; - EXPECT_CALL(dns_service_, - dnsServiceGetAddrInfo(_, kDNSServiceFlagsTimeout, 0, - kDNSServiceProtocol_IPv4 | kDNSServiceProtocol_IPv6, - StrEq(hostname.c_str()), _, _)) + EXPECT_CALL(dns_service_, dnsServiceGetAddrInfo(_, kDNSServiceFlagsTimeout, 0, 0, + StrEq(hostname.c_str()), _, _)) .WillOnce(DoAll(SaveArg<5>(&reply_callback), Return(kDNSServiceErr_NoError))); EXPECT_CALL(dns_service_, dnsServiceRefSockFD(_)).WillOnce(Return(0)); @@ -558,10 +548,8 @@ TEST_F(AppleDnsImplFakeApiTest, QuerySynchronousCompletion) { 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()), _, _)) + EXPECT_CALL(dns_service_, dnsServiceGetAddrInfo(_, kDNSServiceFlagsTimeout, 0, 0, + StrEq(hostname.c_str()), _, _)) .WillOnce(DoAll( // Have the API call synchronously call the provided callback. WithArgs<5, 6>(Invoke([&](DNSServiceGetAddrInfoReply callback, void* context) -> void { @@ -618,10 +606,8 @@ TEST_F(AppleDnsImplFakeApiTest, MultipleAddresses) { DNSServiceGetAddrInfoReply reply_callback; absl::Notification dns_callback_executed; - EXPECT_CALL(dns_service_, - dnsServiceGetAddrInfo(_, kDNSServiceFlagsTimeout, 0, - kDNSServiceProtocol_IPv4 | kDNSServiceProtocol_IPv6, - StrEq(hostname.c_str()), _, _)) + EXPECT_CALL(dns_service_, dnsServiceGetAddrInfo(_, kDNSServiceFlagsTimeout, 0, 0, + StrEq(hostname.c_str()), _, _)) .WillOnce(DoAll(SaveArg<5>(&reply_callback), Return(kDNSServiceErr_NoError))); EXPECT_CALL(dns_service_, dnsServiceRefSockFD(_)).WillOnce(Return(0)); @@ -684,10 +670,8 @@ TEST_F(AppleDnsImplFakeApiTest, MultipleAddressesSecondOneFails) { DNSServiceGetAddrInfoReply reply_callback; absl::Notification dns_callback_executed; - EXPECT_CALL(dns_service_, - dnsServiceGetAddrInfo(_, kDNSServiceFlagsTimeout, 0, - kDNSServiceProtocol_IPv4 | kDNSServiceProtocol_IPv6, - StrEq(hostname.c_str()), _, _)) + EXPECT_CALL(dns_service_, dnsServiceGetAddrInfo(_, kDNSServiceFlagsTimeout, 0, 0, + StrEq(hostname.c_str()), _, _)) .WillOnce(DoAll(SaveArg<5>(&reply_callback), Return(kDNSServiceErr_NoError))); EXPECT_CALL(dns_service_, dnsServiceRefSockFD(_)).WillOnce(Return(0)); @@ -734,10 +718,8 @@ TEST_F(AppleDnsImplFakeApiTest, MultipleQueries) { absl::Notification dns_callback_executed2; // Start first query. - EXPECT_CALL(dns_service_, - dnsServiceGetAddrInfo(_, kDNSServiceFlagsTimeout, 0, - kDNSServiceProtocol_IPv4 | kDNSServiceProtocol_IPv6, - StrEq(hostname.c_str()), _, _)) + EXPECT_CALL(dns_service_, dnsServiceGetAddrInfo(_, kDNSServiceFlagsTimeout, 0, 0, + StrEq(hostname.c_str()), _, _)) .WillOnce(DoAll(SaveArg<5>(&reply_callback), Return(kDNSServiceErr_NoError))); EXPECT_CALL(dns_service_, dnsServiceRefSockFD(_)).WillOnce(Return(0)); @@ -805,10 +787,8 @@ TEST_F(AppleDnsImplFakeApiTest, MultipleQueriesOneFails) { absl::Notification dns_callback_executed2; // Start first query. - EXPECT_CALL(dns_service_, - dnsServiceGetAddrInfo(_, kDNSServiceFlagsTimeout, 0, - kDNSServiceProtocol_IPv4 | kDNSServiceProtocol_IPv6, - StrEq(hostname.c_str()), _, _)) + EXPECT_CALL(dns_service_, dnsServiceGetAddrInfo(_, kDNSServiceFlagsTimeout, 0, 0, + StrEq(hostname.c_str()), _, _)) .WillOnce(DoAll(SaveArg<5>(&reply_callback), Return(kDNSServiceErr_NoError))); EXPECT_CALL(dns_service_, dnsServiceRefSockFD(_)).WillOnce(Return(0)); @@ -869,10 +849,8 @@ TEST_F(AppleDnsImplFakeApiTest, ResultWithOnlyNonAdditiveReplies) { DNSServiceGetAddrInfoReply reply_callback; absl::Notification dns_callback_executed; - EXPECT_CALL(dns_service_, - dnsServiceGetAddrInfo(_, kDNSServiceFlagsTimeout, 0, - kDNSServiceProtocol_IPv4 | kDNSServiceProtocol_IPv6, - StrEq(hostname.c_str()), _, _)) + EXPECT_CALL(dns_service_, dnsServiceGetAddrInfo(_, kDNSServiceFlagsTimeout, 0, 0, + StrEq(hostname.c_str()), _, _)) .WillOnce(DoAll(SaveArg<5>(&reply_callback), Return(kDNSServiceErr_NoError))); EXPECT_CALL(dns_service_, dnsServiceRefSockFD(_)).WillOnce(Return(0)); @@ -904,10 +882,8 @@ TEST_F(AppleDnsImplFakeApiTest, ResultWithNullAddress) { Network::Address::Ipv4Instance address(&addr4); DNSServiceGetAddrInfoReply reply_callback; - EXPECT_CALL(dns_service_, - dnsServiceGetAddrInfo(_, kDNSServiceFlagsTimeout, 0, - kDNSServiceProtocol_IPv4 | kDNSServiceProtocol_IPv6, - StrEq(hostname.c_str()), _, _)) + EXPECT_CALL(dns_service_, dnsServiceGetAddrInfo(_, kDNSServiceFlagsTimeout, 0, 0, + StrEq(hostname.c_str()), _, _)) .WillOnce(DoAll(SaveArg<5>(&reply_callback), Return(kDNSServiceErr_NoError))); EXPECT_CALL(dns_service_, dnsServiceRefSockFD(_)).WillOnce(Return(0)); @@ -941,10 +917,8 @@ TEST_F(AppleDnsImplFakeApiTest, DeallocateOnDestruction) { DNSServiceGetAddrInfoReply reply_callback; absl::Notification dns_callback_executed; - EXPECT_CALL(dns_service_, - dnsServiceGetAddrInfo(_, kDNSServiceFlagsTimeout, 0, - kDNSServiceProtocol_IPv4 | kDNSServiceProtocol_IPv6, - StrEq(hostname.c_str()), _, _)) + EXPECT_CALL(dns_service_, dnsServiceGetAddrInfo(_, kDNSServiceFlagsTimeout, 0, 0, + StrEq(hostname.c_str()), _, _)) .WillOnce(DoAll( SaveArg<5>(&reply_callback), WithArgs<0>(Invoke([](DNSServiceRef* ref) -> void { *ref = new _DNSServiceRef_t{}; })), From 5c219ecff123b0b8de855afc07fd55ce8dd90d0f Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Wed, 6 Oct 2021 09:53:34 -0700 Subject: [PATCH 5/7] segment test Signed-off-by: Jose Nino --- test/common/network/apple_dns_impl_test.cc | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/test/common/network/apple_dns_impl_test.cc b/test/common/network/apple_dns_impl_test.cc index 8ce3b131a00f0..4e4f739a91cff 100644 --- a/test/common/network/apple_dns_impl_test.cc +++ b/test/common/network/apple_dns_impl_test.cc @@ -149,19 +149,25 @@ TEST_F(AppleDnsImplTest, LocalLookup) { dispatcher_->run(Event::Dispatcher::RunType::Block); } -TEST_F(AppleDnsImplTest, DnsIpAddressVersion) { +TEST_F(AppleDnsImplTest, DnsIpAddressVersionAuto) { EXPECT_NE(nullptr, resolveWithExpectations("google.com", DnsLookupFamily::Auto, DnsResolver::ResolutionStatus::Success, true)); dispatcher_->run(Event::Dispatcher::RunType::Block); +} +TEST_F(AppleDnsImplTest, DnsIpAddressVersionV4Preferred) { EXPECT_NE(nullptr, resolveWithExpectations("google.com", DnsLookupFamily::V4Preferred, DnsResolver::ResolutionStatus::Success, true)); dispatcher_->run(Event::Dispatcher::RunType::Block); +} +TEST_F(AppleDnsImplTest, DnsIpAddressVersionV4Only) { EXPECT_NE(nullptr, resolveWithExpectations("google.com", DnsLookupFamily::V4Only, DnsResolver::ResolutionStatus::Success, true)); dispatcher_->run(Event::Dispatcher::RunType::Block); +} +TEST_F(AppleDnsImplTest, DnsIpAddressVersionV6Only) { EXPECT_NE(nullptr, resolveWithExpectations("google.com", DnsLookupFamily::V6Only, DnsResolver::ResolutionStatus::Success, true)); dispatcher_->run(Event::Dispatcher::RunType::Block); @@ -222,10 +228,7 @@ TEST_F(AppleDnsImplTest, CallbackExceptionLocalResolution) { // Validate working of cancellation provided by ActiveDnsQuery return. TEST_F(AppleDnsImplTest, Cancel) { ActiveDnsQuery* query = - resolveWithUnreferencedParameters("some.domain", DnsLookupFamily::Auto, false); - - EXPECT_NE(nullptr, resolveWithExpectations("google.com", DnsLookupFamily::Auto, - DnsResolver::ResolutionStatus::Success, true)); + resolveWithUnreferencedParameters("google.com", DnsLookupFamily::Auto, false); ASSERT_NE(nullptr, query); query->cancel(Network::ActiveDnsQuery::CancelReason::QueryAbandoned); From 61565c5d25e33c5546df470d9f5941099586f232 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Wed, 6 Oct 2021 14:26:23 -0700 Subject: [PATCH 6/7] update tests Signed-off-by: Jose Nino --- test/common/network/apple_dns_impl_test.cc | 37 +++++++++++++++++++--- 1 file changed, 32 insertions(+), 5 deletions(-) diff --git a/test/common/network/apple_dns_impl_test.cc b/test/common/network/apple_dns_impl_test.cc index 4e4f739a91cff..8b06c6bc526ba 100644 --- a/test/common/network/apple_dns_impl_test.cc +++ b/test/common/network/apple_dns_impl_test.cc @@ -75,13 +75,37 @@ class AppleDnsImplTest : public testing::Test { EXPECT_EQ(expected_status, status); if (expected_results) { EXPECT_FALSE(results.empty()); + absl::optional is_v4{}; for (const auto& result : results) { - if (lookup_family == DnsLookupFamily::V4Only || - lookup_family == DnsLookupFamily::V4Preferred) { + switch (lookup_family) + { + case DnsLookupFamily::V4Only: EXPECT_NE(nullptr, result.address_->ip()->ipv4()); - } else if (lookup_family == DnsLookupFamily::V6Only || - lookup_family == DnsLookupFamily::Auto) { + break; + case DnsLookupFamily::V6Only: EXPECT_NE(nullptr, result.address_->ip()->ipv6()); + break; + // In CI these modes could return either V4 or V6 with the non-mocked API calls. But + // regardless of the family all returned addresses need to be one _or_ the other. + case DnsLookupFamily::V4Preferred: + case DnsLookupFamily::Auto: + // Set the expectation for subsequent responses based on the first one. + if (!is_v4.has_value()) { + if (result.address_->ip()->ipv4()) { + is_v4 = true; + } else { + is_v4 = false; + } + } + + if (is_v4.value()) { + EXPECT_NE(nullptr, result.address_->ip()->ipv4()); + } else { + EXPECT_NE(nullptr, result.address_->ip()->ipv6()); + } + break; + default: + NOT_REACHED_GCOVR_EXCL_LINE; } } } @@ -228,7 +252,10 @@ TEST_F(AppleDnsImplTest, CallbackExceptionLocalResolution) { // Validate working of cancellation provided by ActiveDnsQuery return. TEST_F(AppleDnsImplTest, Cancel) { ActiveDnsQuery* query = - resolveWithUnreferencedParameters("google.com", DnsLookupFamily::Auto, false); + resolveWithUnreferencedParameters("some.domain", DnsLookupFamily::Auto, false); + + EXPECT_NE(nullptr, resolveWithExpectations("google.com", DnsLookupFamily::Auto, + DnsResolver::ResolutionStatus::Success, true)); ASSERT_NE(nullptr, query); query->cancel(Network::ActiveDnsQuery::CancelReason::QueryAbandoned); From 33037227ba46a31e70e9ce6c9d9ae82d6e958d78 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Wed, 6 Oct 2021 15:34:12 -0700 Subject: [PATCH 7/7] fmt Signed-off-by: Jose Nino --- test/common/network/apple_dns_impl_test.cc | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/test/common/network/apple_dns_impl_test.cc b/test/common/network/apple_dns_impl_test.cc index 8b06c6bc526ba..bf9cc87508a8f 100644 --- a/test/common/network/apple_dns_impl_test.cc +++ b/test/common/network/apple_dns_impl_test.cc @@ -77,8 +77,7 @@ class AppleDnsImplTest : public testing::Test { EXPECT_FALSE(results.empty()); absl::optional is_v4{}; for (const auto& result : results) { - switch (lookup_family) - { + switch (lookup_family) { case DnsLookupFamily::V4Only: EXPECT_NE(nullptr, result.address_->ip()->ipv4()); break;