Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 473e881e authored by Mike Yu's avatar Mike Yu
Browse files

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
parent 1aede813

DnsTlsDispatcher.cpp

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


    if (code == DnsTlsTransport::Response::network_error) {

        continuousfailureCount++;

        LOG(WARNING) << "continuousfailureCount incremented: network_error, count = "

        LOG(DEBUG) << "continuousfailureCount incremented: network_error, count = "

                   << continuousfailureCount;

    } else if (latencyTooHigh) {

        continuousfailureCount++;

        LOG(WARNING) << "continuousfailureCount incremented: latency too High, count = "

        LOG(DEBUG) << "continuousfailureCount incremented: latency too High, count = "

                   << continuousfailureCount;

    } else {

    } else if (continuousfailureCount > 0) {

        continuousfailureCount = 0;

        LOG(WARNING) << "continuousfailureCount reset";

        LOG(DEBUG) << "continuousfailureCount reset";

    }



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

PrivateDnsConfiguration.cpp

+43 −21
Original line number Diff line number Diff line
@@ -61,6 +61,11 @@ 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) {
@@ -202,20 +207,11 @@ 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);

            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();

        std::optional<int64_t> latencyThreshold;



        // cat /proc/sys/net/ipv4/tcp_syn_retries yields "6".

        //
@@ -232,7 +228,20 @@ void PrivateDnsConfiguration::startValidation(const ServerIdentity& identity, un 
        // (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 @@ 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",

                                        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 @@ void PrivateDnsConfiguration::sendPrivateDnsValidationEvent(const ServerIdentity 


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 @@ 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

+5 −2
Original line number Diff line number Diff line
@@ -98,6 +98,9 @@ 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|.
@@ -106,8 +109,8 @@ class PrivateDnsConfiguration { 
            REQUIRES(mPrivateDnsLock);



    bool recordPrivateDnsValidation(const ServerIdentity& identity, unsigned netId, bool success,

                                    bool isRevalidation, bool latencyTooHigh)

            EXCLUDES(mPrivateDnsLock);

                                    bool isRevalidation, bool latencyTooHigh,

                                    bool maxAttemptsReached) EXCLUDES(mPrivateDnsLock);



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

            REQUIRES(mPrivateDnsLock);

PrivateDnsConfigurationTest.cpp

+52 −0
Original line number Diff line number Diff line
@@ -156,6 +156,8 @@ 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;
@@ -284,6 +286,56 @@ 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);