Add tests to cover repeated setResolverConfiguration
Two tests are added to protect the resolver when it continually
receives the same setup requests.
When the resolver receives repeated and same setup:
[1] Do not clear the cache.
[2] Do not clear the stats (the stats for cleartext DNS servers
got from GetResolverInfo()).
[3] Do not start a new re-evaluation process for the private DNS
servers if they have been marked as in_progress.
[4] Need not to re-validate the private DNS servers if they have
been validated.
Another test is added to protect the implementation of aosp/1108695.
Fix: 150678049
Test: resolv_integration_test passed
Change-Id: I7a866e7e305c0fb703ccb9546d1c70ce77e2d3c7
diff --git a/tests/resolv_integration_test.cpp b/tests/resolv_integration_test.cpp
index 1aeb4c2..0286310 100644
--- a/tests/resolv_integration_test.cpp
+++ b/tests/resolv_integration_test.cpp
@@ -81,6 +81,8 @@
const addrinfo* hints, unsigned netid, unsigned mark,
struct addrinfo** result);
+using namespace std::chrono_literals;
+
using aidl::android::net::IDnsResolver;
using aidl::android::net::INetd;
using aidl::android::net::ResolverParamsParcel;
@@ -217,6 +219,10 @@
return sDnsMetricsListener->waitForPrivateDnsValidation(serverAddr, validated);
}
+ bool hasUncaughtPrivateDnsValidation(const std::string& serverAddr) {
+ return sDnsMetricsListener->findValidationRecord(serverAddr);
+ }
+
bool expectStatsFromGetResolverInfo(const std::vector<NameserverStats>& nameserversStats) {
std::vector<std::string> res_servers;
std::vector<std::string> res_domains;
@@ -265,6 +271,17 @@
return true;
}
+ // Since there's no way to terminate private DNS validation threads at any time. Tests that
+ // focus on the results of private DNS validation can interfere with each other if they use the
+ // same IP address for test servers. getUniqueIPv4Address() is a workaround to reduce the
+ // possibility of tests being flaky. A feasible solution is to forbid the validation threads,
+ // which are considered as outdated (e.g. switch the resolver to private DNS OFF mode), updating
+ // the result to the PrivateDnsConfiguration instance.
+ static std::string getUniqueIPv4Address() {
+ static int counter = 0;
+ return fmt::format("127.0.100.{}", (++counter & 0xff));
+ }
+
DnsResponderClient mDnsClient;
// Use a shared static DNS listener for all tests to avoid registering lots of listeners
@@ -4143,6 +4160,290 @@
}
}
+TEST_F(ResolverTest, RepeatedSetup_ResolverStatusRemains) {
+ constexpr char unusable_listen_addr[] = "127.0.0.3";
+ constexpr char listen_addr[] = "127.0.0.4";
+ constexpr char hostname[] = "a.hello.query.";
+ const auto repeatedSetResolversFromParcel = [&](const ResolverParamsParcel& parcel) {
+ ASSERT_TRUE(mDnsClient.SetResolversFromParcel(parcel));
+ ASSERT_TRUE(mDnsClient.SetResolversFromParcel(parcel));
+ ASSERT_TRUE(mDnsClient.SetResolversFromParcel(parcel));
+ };
+
+ test::DNSResponder dns(listen_addr);
+ StartDns(dns, {{hostname, ns_type::ns_t_a, "1.2.3.3"}});
+ test::DnsTlsFrontend tls1(listen_addr, "853", listen_addr, "53");
+ ASSERT_TRUE(tls1.startServer());
+
+ // Private DNS off mode.
+ ResolverParamsParcel parcel = DnsResponderClient::GetDefaultResolverParamsParcel();
+ parcel.servers = {unusable_listen_addr, listen_addr};
+ parcel.tlsServers.clear();
+ ASSERT_TRUE(mDnsClient.SetResolversFromParcel(parcel));
+
+ // Send a query.
+ const addrinfo hints = {.ai_family = AF_INET, .ai_socktype = SOCK_DGRAM};
+ EXPECT_NE(safe_getaddrinfo(hostname, nullptr, &hints), nullptr);
+
+ // Check the stats as expected.
+ const std::vector<NameserverStats> expectedCleartextDnsStats = {
+ NameserverStats(unusable_listen_addr).setInternalErrors(1),
+ NameserverStats(listen_addr).setSuccesses(1),
+ };
+ EXPECT_TRUE(expectStatsFromGetResolverInfo(expectedCleartextDnsStats));
+ EXPECT_EQ(GetNumQueries(dns, hostname), 1U);
+
+ // The stats is supposed to remain as long as the list of cleartext DNS servers is unchanged.
+ static const struct TestConfig {
+ std::vector<std::string> servers;
+ std::vector<std::string> tlsServers;
+ std::string tlsName;
+ } testConfigs[] = {
+ // Private DNS opportunistic mode.
+ {{listen_addr, unusable_listen_addr}, {listen_addr, unusable_listen_addr}, ""},
+ {{unusable_listen_addr, listen_addr}, {unusable_listen_addr, listen_addr}, ""},
+
+ // Private DNS strict mode.
+ {{listen_addr, unusable_listen_addr}, {"127.0.0.100"}, kDefaultPrivateDnsHostName},
+ {{unusable_listen_addr, listen_addr}, {"127.0.0.100"}, kDefaultPrivateDnsHostName},
+
+ // Private DNS off mode.
+ {{unusable_listen_addr, listen_addr}, {}, ""},
+ {{listen_addr, unusable_listen_addr}, {}, ""},
+ };
+
+ for (const auto& config : testConfigs) {
+ SCOPED_TRACE(fmt::format("testConfig: [{}] [{}] [{}]", fmt::join(config.servers, ","),
+ fmt::join(config.tlsServers, ","), config.tlsName));
+ parcel = DnsResponderClient::GetDefaultResolverParamsParcel();
+ parcel.servers = config.servers;
+ parcel.tlsServers = config.tlsServers;
+ parcel.tlsName = config.tlsName;
+ repeatedSetResolversFromParcel(parcel);
+ EXPECT_TRUE(expectStatsFromGetResolverInfo(expectedCleartextDnsStats));
+
+ // The stats remains when the list of search domains changes.
+ parcel.domains.push_back("tmp.domains");
+ repeatedSetResolversFromParcel(parcel);
+ EXPECT_TRUE(expectStatsFromGetResolverInfo(expectedCleartextDnsStats));
+
+ // The stats remains when the parameters change (except maxSamples).
+ parcel.sampleValiditySeconds++;
+ parcel.successThreshold++;
+ parcel.minSamples++;
+ parcel.baseTimeoutMsec++;
+ parcel.retryCount++;
+ repeatedSetResolversFromParcel(parcel);
+ EXPECT_TRUE(expectStatsFromGetResolverInfo(expectedCleartextDnsStats));
+ }
+
+ // The cache remains.
+ EXPECT_NE(safe_getaddrinfo(hostname, nullptr, &hints), nullptr);
+ EXPECT_EQ(GetNumQueries(dns, hostname), 1U);
+}
+
+TEST_F(ResolverTest, RepeatedSetup_NoRedundantPrivateDnsValidation) {
+ const std::string addr1 = getUniqueIPv4Address(); // For a workable DNS server.
+ const std::string addr2 = getUniqueIPv4Address(); // For an unresponsive DNS server.
+ const std::string unusable_addr = getUniqueIPv4Address();
+
+ test::DNSResponder dns1(addr1);
+ test::DNSResponder dns2(addr2);
+ StartDns(dns1, {});
+ StartDns(dns2, {});
+ test::DnsTlsFrontend workableTls(addr1, "853", addr1, "53");
+ test::DnsTlsFrontend unresponsiveTls(addr2, "853", addr2, "53");
+ unresponsiveTls.setHangOnHandshakeForTesting(true);
+ ASSERT_TRUE(workableTls.startServer());
+ ASSERT_TRUE(unresponsiveTls.startServer());
+
+ // First setup.
+ ResolverParamsParcel parcel = DnsResponderClient::GetDefaultResolverParamsParcel();
+ parcel.servers = {addr1, addr2, unusable_addr};
+ parcel.tlsServers = {addr1, addr2, unusable_addr};
+ ASSERT_TRUE(mDnsClient.SetResolversFromParcel(parcel));
+
+ // Check the validation results.
+ EXPECT_TRUE(WaitForPrivateDnsValidation(workableTls.listen_address(), true));
+ EXPECT_TRUE(WaitForPrivateDnsValidation(unusable_addr, false));
+ EXPECT_EQ(unresponsiveTls.acceptConnectionsCount(), 1); // The validation is still in progress.
+
+ static const struct TestConfig {
+ std::vector<std::string> tlsServers;
+ std::string tlsName;
+ } testConfigs[] = {
+ {{addr1, addr2, unusable_addr}, ""},
+ {{unusable_addr, addr1, addr2}, ""},
+ {{unusable_addr, addr1, addr2}, kDefaultPrivateDnsHostName},
+ {{addr1, addr2, unusable_addr}, kDefaultPrivateDnsHostName},
+ };
+
+ std::string TlsNameLastTime;
+ for (const auto& config : testConfigs) {
+ SCOPED_TRACE(fmt::format("testConfig: [{}] [{}]", fmt::join(config.tlsServers, ","),
+ config.tlsName));
+ parcel.servers = config.tlsServers;
+ parcel.tlsServers = config.tlsServers;
+ parcel.tlsName = config.tlsName;
+ parcel.caCertificate = config.tlsName.empty() ? "" : kCaCert;
+
+ const bool dnsModeChanged = (TlsNameLastTime != config.tlsName);
+ ASSERT_TRUE(mDnsClient.SetResolversFromParcel(parcel));
+
+ for (const auto& serverAddr : parcel.tlsServers) {
+ SCOPED_TRACE(serverAddr);
+ if (serverAddr == workableTls.listen_address()) {
+ if (dnsModeChanged) {
+ // In despite of the identical IP address, the server is regarded as a different
+ // server when DnsTlsServer.name is different. The resolver treats it as a
+ // different object and begins the validation process.
+ EXPECT_TRUE(WaitForPrivateDnsValidation(serverAddr, true));
+ }
+ } else if (serverAddr == unresponsiveTls.listen_address()) {
+ // No revalidation needed for the server which have been marked as in_progesss.
+ } else {
+ // Must be unusable_addr.
+ // In opportunistic mode, when a validation for a private DNS server fails, the
+ // resolver just marks the server as failed and doesn't re-evaluate it, but the
+ // server can be re-evaluated when setResolverConfiguration() is called.
+ // However, in strict mode, the resolver automatically re-evaluates the server and
+ // marks the server as in_progress until the validation succeeds, so repeated setup
+ // makes no effect.
+ if (dnsModeChanged || config.tlsName.empty() /* not in strict mode */) {
+ EXPECT_TRUE(WaitForPrivateDnsValidation(serverAddr, false));
+ }
+ }
+ }
+
+ // Repeated setups make no effect in strict mode.
+ ASSERT_TRUE(mDnsClient.SetResolversFromParcel(parcel));
+ if (config.tlsName.empty()) {
+ EXPECT_TRUE(WaitForPrivateDnsValidation(unusable_addr, false));
+ }
+ ASSERT_TRUE(mDnsClient.SetResolversFromParcel(parcel));
+ if (config.tlsName.empty()) {
+ EXPECT_TRUE(WaitForPrivateDnsValidation(unusable_addr, false));
+ }
+
+ EXPECT_EQ(unresponsiveTls.acceptConnectionsCount(), 1);
+
+ TlsNameLastTime = config.tlsName;
+ }
+
+ // Check that all the validation results are caught.
+ // Note: it doesn't mean no validation being in progress.
+ EXPECT_FALSE(hasUncaughtPrivateDnsValidation(addr1));
+ EXPECT_FALSE(hasUncaughtPrivateDnsValidation(addr2));
+ EXPECT_FALSE(hasUncaughtPrivateDnsValidation(unusable_addr));
+}
+
+TEST_F(ResolverTest, RepeatedSetup_KeepChangingPrivateDnsServers) {
+ enum TlsServerState { WORKING, UNSUPPORTED, UNRESPONSIVE };
+ const std::string addr1 = getUniqueIPv4Address();
+ const std::string addr2 = getUniqueIPv4Address();
+
+ test::DNSResponder dns1(addr1);
+ test::DNSResponder dns2(addr2);
+ StartDns(dns1, {});
+ StartDns(dns2, {});
+ test::DnsTlsFrontend tls1(addr1, "853", addr1, "53");
+ test::DnsTlsFrontend tls2(addr2, "853", addr2, "53");
+ ASSERT_TRUE(tls1.startServer());
+ ASSERT_TRUE(tls2.startServer());
+
+ static const struct TestConfig {
+ std::string tlsServer;
+ std::string tlsName;
+ bool expectNothingHappenWhenServerUnsupported;
+ bool expectNothingHappenWhenServerUnresponsive;
+ std::string asTestName() const {
+ return fmt::format("{}, {}, {}, {}", tlsServer, tlsName,
+ expectNothingHappenWhenServerUnsupported,
+ expectNothingHappenWhenServerUnresponsive);
+ }
+ } testConfigs[] = {
+ {{addr1}, "", false, false},
+ {{addr2}, "", false, false},
+ {{addr1}, "", false, true},
+ {{addr2}, "", false, true},
+ {{addr1}, kDefaultPrivateDnsHostName, false, true},
+ {{addr2}, kDefaultPrivateDnsHostName, false, true},
+ {{addr1}, kDefaultPrivateDnsHostName, true, true},
+ {{addr2}, kDefaultPrivateDnsHostName, true, true},
+
+ // There's no new validation to start because there are already two validation threads
+ // running (one is for addr1, the other is for addr2). This is because the comparator
+ // doesn't compare DnsTlsServer.name. Keep the design as-is until it's known to be
+ // harmful.
+ {{addr1}, "", true, true},
+ {{addr2}, "", true, true},
+ {{addr1}, "", true, true},
+ {{addr2}, "", true, true},
+ };
+
+ for (const auto& serverState : {WORKING, UNSUPPORTED, UNRESPONSIVE}) {
+ int testIndex = 0;
+ for (const auto& config : testConfigs) {
+ SCOPED_TRACE(fmt::format("serverState:{} testIndex:{} testConfig:[{}]", serverState,
+ testIndex++, config.asTestName()));
+ auto& tls = (config.tlsServer == addr1) ? tls1 : tls2;
+
+ if (serverState == UNSUPPORTED && tls.running()) ASSERT_TRUE(tls.stopServer());
+ if (serverState != UNSUPPORTED && !tls.running()) ASSERT_TRUE(tls.startServer());
+
+ tls.setHangOnHandshakeForTesting(serverState == UNRESPONSIVE);
+ const int connectCountsBefore = tls.acceptConnectionsCount();
+
+ ResolverParamsParcel parcel = DnsResponderClient::GetDefaultResolverParamsParcel();
+ parcel.servers = {config.tlsServer};
+ parcel.tlsServers = {config.tlsServer};
+ parcel.tlsName = config.tlsName;
+ parcel.caCertificate = config.tlsName.empty() ? "" : kCaCert;
+ ASSERT_TRUE(mDnsClient.SetResolversFromParcel(parcel));
+
+ if (serverState == WORKING) {
+ EXPECT_TRUE(WaitForPrivateDnsValidation(config.tlsServer, true));
+ } else if (serverState == UNSUPPORTED) {
+ if (config.expectNothingHappenWhenServerUnsupported) {
+ // It's possible that the resolver hasn't yet started to
+ // connect. Wait a while.
+ // TODO: See if we can get rid of the hard waiting time, such as comparing
+ // the CountDiff across two tests.
+ std::this_thread::sleep_for(100ms);
+ EXPECT_EQ(tls.acceptConnectionsCount(), connectCountsBefore);
+ } else {
+ EXPECT_TRUE(WaitForPrivateDnsValidation(config.tlsServer, false));
+ }
+ } else {
+ // Must be UNRESPONSIVE.
+ // DnsTlsFrontend is the only signal for checking whether or not the resolver starts
+ // another validation when the server is unresponsive.
+ const int expectCountDiff =
+ config.expectNothingHappenWhenServerUnresponsive ? 0 : 1;
+ if (expectCountDiff == 0) {
+ // It's possible that the resolver hasn't yet started to
+ // connect. Wait a while.
+ std::this_thread::sleep_for(100ms);
+ }
+ const auto condition = [&]() {
+ return tls.acceptConnectionsCount() == connectCountsBefore + expectCountDiff;
+ };
+ EXPECT_TRUE(PollForCondition(condition));
+ }
+ }
+
+ // Set to off mode to reset the PrivateDnsConfiguration state.
+ ResolverParamsParcel setupOffmode = DnsResponderClient::GetDefaultResolverParamsParcel();
+ setupOffmode.tlsServers.clear();
+ ASSERT_TRUE(mDnsClient.SetResolversFromParcel(setupOffmode));
+ }
+
+ // Check that all the validation results are caught.
+ // Note: it doesn't mean no validation being in progress.
+ EXPECT_FALSE(hasUncaughtPrivateDnsValidation(addr1));
+ EXPECT_FALSE(hasUncaughtPrivateDnsValidation(addr2));
+}
+
// Parameterized tests.
// TODO: Merge the existing tests as parameterized test if possible.
// TODO: Perhaps move parameterized tests to an independent file.