Set maximum attempt for opportunistic mode private DNS validation
If a private DNS works but it's always too slow to fit the latency
threshold, private DNS validation can be endless. To address this
problem, limit the number of attempts to three. If a private DNS
validation has attempted on three probes, it will finish.
Besides, latencyThreshold calculated at the beginning of validation
can be too outdated to be useful because of the backoff time which
starts with one minute delay. Change the code to update
latencyThreshold every time when probe is about to begin.
Bug: 188153519
Test: run atest with all the flags off/on
avoid_bad_private_dns: 0 / 1
sort_nameservers: 0 / 1
dot_xport_unusable_threshold: -1 / 20
dot_query_timeout_ms: -1 / 10000
min_private_dns_latency_threshold_ms: -1 / 500
keep_listening_udp: 0 / 1
parallel_lookup_sleep_time: 2 / 2
dot_revalidation_threshold: -1 / 10
max_private_dns_latency_threshold_ms: -1 / 2000
dot_async_handshake: 0 / 1
dot_maxtries: 3 / 1
dot_connect_timeout_ms: 127000 / 10000
parallel_lookup_release: UNSET / UNSET
Change-Id: I8251681152e4ed72bf5232ab688267e6d92878c4
diff --git a/PrivateDnsConfiguration.cpp b/PrivateDnsConfiguration.cpp
index 6939227..4637990 100644
--- a/PrivateDnsConfiguration.cpp
+++ b/PrivateDnsConfiguration.cpp
@@ -61,6 +61,11 @@
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) {
@@ -202,20 +207,11 @@
const bool avoidBadPrivateDns =
Experiments::getInstance()->getFlag("avoid_bad_private_dns", 0);
+ 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;
- 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);
- 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".
//
@@ -232,7 +228,20 @@
// (6 SYNs per ip, 4 ips per validation pass, 24 passes per day).
auto backoff = mBackoffBuilder.build();
- while (true) {
+ 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);
+ }
+
// ::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"
@@ -241,20 +250,28 @@
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",
- gotAnswer, server.toIpString(), timeTaken);
+ 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;
+ }
const int64_t targetTime = latencyThreshold.value_or(INT64_MAX);
- bool latencyTooHigh = false;
- if (isOpportunisticMode && timeTaken > targetTime) {
- latencyTooHigh = true;
+ const bool latencyTooHigh = timeTaken > targetTime;
+ if (latencyTooHigh) {
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);
+ identity, netId, gotAnswer, isRevalidation, latencyTooHigh, maxAttemptsReached);
if (!needs_reeval) {
break;
@@ -305,7 +322,8 @@
bool PrivateDnsConfiguration::recordPrivateDnsValidation(const ServerIdentity& identity,
unsigned netId, bool gotAnswer,
- bool isRevalidation, bool latencyTooHigh) {
+ bool isRevalidation, bool latencyTooHigh,
+ bool maxAttemptsReached) {
constexpr bool NEEDS_REEVALUATION = true;
constexpr bool DONT_REEVALUATE = false;
@@ -336,6 +354,10 @@
reevaluationStatus = DONT_REEVALUATE;
}
+ if (maxAttemptsReached) {
+ reevaluationStatus = DONT_REEVALUATE;
+ }
+
bool success = gotAnswer;
auto& tracker = netPair->second;
auto serverPair = tracker.find(identity);