Revert "Set maximum attempt for opportunistic mode private DNS validation" am: 7135fbfcbd (5d3a5c39) · Commits · e / os / android_packages_modules_DnsResolver

DnsTlsDispatcher.cpp

+6 −6

Original line number	Diff line number	Diff line
		@@ -348,15 +348,15 @@ bool DnsTlsDispatcher::Transport::checkRevalidationNecessary(DnsTlsTransport::Re

		if (code == DnsTlsTransport::Response::network_error) {
		continuousfailureCount++;
		LOG(DEBUG) << "continuousfailureCount incremented: network_error, count = "
		LOG(WARNING) << "continuousfailureCount incremented: network_error, count = "
		<< continuousfailureCount;
		} else if (latencyTooHigh) {
		continuousfailureCount++;
		LOG(DEBUG) << "continuousfailureCount incremented: latency too High, count = "
		LOG(WARNING) << "continuousfailureCount incremented: latency too High, count = "
		<< continuousfailureCount;
		} else if (continuousfailureCount > 0) {
		} else {
		continuousfailureCount = 0;
		LOG(DEBUG) << "continuousfailureCount reset";
		LOG(WARNING) << "continuousfailureCount reset";
		}

		// triggerThreshold must be greater than 0 because the value of revalidationEnabled is true.

PrivateDnsConfiguration.cpp

+21 −43

Original line number	Diff line number	Diff line
		@@ -61,11 +61,6 @@ bool parseServer(const char* server, sockaddr_storage* parsed) {
		return true;
		}

		// Returns true if the IPrivateDnsServer was created when the mode was opportunistic.
		bool isForOpportunisticMode(const PrivateDnsConfiguration::ServerIdentity& identity) {
		return identity.provider.empty();
		}

		int PrivateDnsConfiguration::set(int32_t netId, uint32_t mark,
		const std::vector<std::string>& servers, const std::string& name,
		const std::string& caCert) {
		@@ -207,11 +202,20 @@ void PrivateDnsConfiguration::startValidation(const ServerIdentity& identity, un

		const bool avoidBadPrivateDns =
		Experiments::getInstance()->getFlag("avoid_bad_private_dns", 0);
		std::optional<int64_t> latencyThreshold;
		if (avoidBadPrivateDns) {
		const int maxLatency = Experiments::getInstance()->getFlag(
		"max_private_dns_latency_threshold_ms", kMaxPrivateDnsLatencyThresholdMs);
		const int minLatency = Experiments::getInstance()->getFlag(
		"min_private_dns_latency_threshold_ms", kMinPrivateDnsLatencyThresholdMs);
		std::optional<int64_t> latencyThreshold;
		const auto do53Latency = resolv_stats_get_average_response_time(netId, PROTO_UDP);
		const int target =
		do53Latency.has_value() ? (3 * do53Latency.value().count() / 1000) : 0;

		// The time is limited to the range [minLatency, maxLatency].
		latencyThreshold = std::clamp(target, minLatency, maxLatency);
		}
		const bool isOpportunisticMode = server.name.empty();

		// cat /proc/sys/net/ipv4/tcp_syn_retries yields "6".
		//
		@@ -228,20 +232,7 @@ void PrivateDnsConfiguration::startValidation(const ServerIdentity& identity, un
		// (6 SYNs per ip, 4 ips per validation pass, 24 passes per day).
		auto backoff = mBackoffBuilder.build();

		for (int attempt = 1; /**/; ++attempt) {
		// Because the time between two probes is at least one minute, there might already be
		// some traffic sent to Do53 servers during the time. Update latencyThreshold every
		// time before the probe.
		if (avoidBadPrivateDns && isForOpportunisticMode(identity)) {
		const auto do53Latency = resolv_stats_get_average_response_time(netId, PROTO_UDP);
		const int target = do53Latency.has_value()
		? (3 * do53Latency.value().count() / 1000)
		: minLatency;

		// The threshold is limited to the range [minLatency, maxLatency].
		latencyThreshold = std::clamp(target, minLatency, maxLatency);
		}

		while (true) {
		// ::validate() is a blocking call that performs network operations.
		// It can take milliseconds to minutes, up to the SYN retry limit.
		LOG(WARNING) << "Validating DnsTlsServer " << server.toIpString() << " with mark 0x"
		@@ -250,28 +241,20 @@ void PrivateDnsConfiguration::startValidation(const ServerIdentity& identity, un
		Stopwatch stopwatch;
		const bool gotAnswer = DnsTlsTransport::validate(server, server.validationMark());
		const int32_t timeTaken = saturate_cast<int32_t>(stopwatch.timeTakenUs() / 1000);
		LOG(WARNING) << fmt::format(
		"validateDnsTlsServer returned {} for {}, took {}ms, attempt {}", gotAnswer,
		server.toIpString(), timeTaken, attempt);

		// Prevent from endlessly sending traffic on the network in opportunistic mode.
		bool maxAttemptsReached = false;
		if (avoidBadPrivateDns && attempt >= kOpportunisticModeMaxAttempts &&
		isForOpportunisticMode(identity)) {
		maxAttemptsReached = true;
		LOG(WARNING) << "Max attempts reached: " << kOpportunisticModeMaxAttempts;
		}
		LOG(WARNING) << fmt::format("validateDnsTlsServer returned {} for {}, took {}ms",
		gotAnswer, server.toIpString(), timeTaken);

		const int64_t targetTime = latencyThreshold.value_or(INT64_MAX);
		const bool latencyTooHigh = timeTaken > targetTime;
		if (latencyTooHigh) {
		bool latencyTooHigh = false;
		if (isOpportunisticMode && timeTaken > targetTime) {
		latencyTooHigh = true;
		LOG(WARNING) << "validateDnsTlsServer took too long: threshold is " << targetTime
		<< "ms";
		}

		// TODO: combine these boolean variables into a bitwise variable.
		const bool needs_reeval = this->recordPrivateDnsValidation(
		identity, netId, gotAnswer, isRevalidation, latencyTooHigh, maxAttemptsReached);
		identity, netId, gotAnswer, isRevalidation, latencyTooHigh);

		if (!needs_reeval) {
		break;
		@@ -322,8 +305,7 @@ void PrivateDnsConfiguration::sendPrivateDnsValidationEvent(const ServerIdentity

		bool PrivateDnsConfiguration::recordPrivateDnsValidation(const ServerIdentity& identity,
		unsigned netId, bool gotAnswer,
		bool isRevalidation, bool latencyTooHigh,
		bool maxAttemptsReached) {
		bool isRevalidation, bool latencyTooHigh) {
		constexpr bool NEEDS_REEVALUATION = true;
		constexpr bool DONT_REEVALUATE = false;

		@@ -354,10 +336,6 @@ bool PrivateDnsConfiguration::recordPrivateDnsValidation(const ServerIdentity& i
		reevaluationStatus = DONT_REEVALUATE;
		}

		if (maxAttemptsReached) {
		reevaluationStatus = DONT_REEVALUATE;
		}

		bool success = gotAnswer;
		auto& tracker = netPair->second;
		auto serverPair = tracker.find(identity);

PrivateDnsConfiguration.h

+2 −5

Original line number	Diff line number	Diff line
		@@ -98,9 +98,6 @@ class PrivateDnsConfiguration {
		static constexpr int kMaxPrivateDnsLatencyThresholdMs = 2000;
		static constexpr int kMinPrivateDnsLatencyThresholdMs = 500;

		// (Opportunistic mode only) The maximum attempts to send a probe to a private DNS server.
		static constexpr int kOpportunisticModeMaxAttempts = 3;

		PrivateDnsConfiguration() = default;

		// Launchs a thread to run the validation for \|server\| on the network \|netId\|.
		@@ -109,8 +106,8 @@ class PrivateDnsConfiguration {
		REQUIRES(mPrivateDnsLock);

		bool recordPrivateDnsValidation(const ServerIdentity& identity, unsigned netId, bool success,
		bool isRevalidation, bool latencyTooHigh,
		bool maxAttemptsReached) EXCLUDES(mPrivateDnsLock);
		bool isRevalidation, bool latencyTooHigh)
		EXCLUDES(mPrivateDnsLock);

		void sendPrivateDnsValidationEvent(const ServerIdentity& identity, unsigned netId, bool success)
		REQUIRES(mPrivateDnsLock);

PrivateDnsConfigurationTest.cpp

+0 −52

Original line number	Diff line number	Diff line
		@@ -156,8 +156,6 @@ class PrivateDnsConfigurationTest : public ::testing::Test {
		static constexpr char kBackend[] = "127.0.2.1";
		static constexpr char kServer1[] = "127.0.2.2";
		static constexpr char kServer2[] = "127.0.2.3";
		static constexpr int kOpportunisticModeMaxAttempts =
		PrivateDnsConfiguration::kOpportunisticModeMaxAttempts;

		MockObserver mObserver;
		PrivateDnsConfiguration mPdc;
		@@ -286,56 +284,6 @@ TEST_F(PrivateDnsConfigurationTest, ValidationProbingTime) {
		backend.setResponseDelayMs(0);
		}

		// Tests that Private DNS validation won't be endless if the server works and it's slow.
		TEST_F(PrivateDnsConfigurationTest, ValidationMaxProbes) {
		ScopedSystemProperty sp1(kMinPrivateDnsLatencyThresholdMsFlag, "100");
		ScopedSystemProperty sp2(kMaxPrivateDnsLatencyThresholdMsFlag, "200");
		const int serverLatencyMs = 300;

		// TODO: Complete STRICT test after the dependency of DnsTlsFrontend is removed.
		static const struct TestConfig {
		std::string dnsMode;
		bool avoidBadPrivateDns;
		Validation expectedValidationResult;
		int expectedProbes;
		} testConfigs[] = {
		// clang-format off
		{"OPPORTUNISTIC", false, Validation::success, 1},
		{"OPPORTUNISTIC", true, Validation::fail, kOpportunisticModeMaxAttempts},
		// clang-format on
		};

		for (const auto& config : testConfigs) {
		SCOPED_TRACE(
		fmt::format("testConfig: [{}, {}]", config.dnsMode, config.avoidBadPrivateDns));

		ScopedSystemProperty sp3(kAvoidBadPrivateDnsFlag, (config.avoidBadPrivateDns ? "1" : "0"));
		forceExperimentsInstanceUpdate();
		backend.setResponseDelayMs(serverLatencyMs);

		testing::InSequence seq;
		EXPECT_CALL(mObserver, onValidationStateUpdate(kServer1, Validation::in_process, kNetId))
		.Times(config.expectedProbes);
		EXPECT_CALL(mObserver,
		onValidationStateUpdate(kServer1, config.expectedValidationResult, kNetId));

		EXPECT_EQ(mPdc.set(kNetId, kMark, {kServer1}, {}, {}), 0);
		expectPrivateDnsStatus(PrivateDnsMode::OPPORTUNISTIC);

		// probing time + backoff delay
		const int expectedValidationTimeMs =
		config.expectedProbes * serverLatencyMs + (config.expectedProbes - 1) * 1000;
		ASSERT_TRUE(PollForCondition([&]() { return mObserver.runningThreads == 0; },
		std::chrono::milliseconds(expectedValidationTimeMs + 1000)));

		// Reset the state for the next round.
		mPdc.clear(kNetId);
		testing::Mock::VerifyAndClearExpectations(&mObserver);
		}

		backend.setResponseDelayMs(0);
		}

		TEST_F(PrivateDnsConfigurationTest, ValidationBlock) {
		backend.setDeferredResp(true);