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

Commit 679faeb9 authored by Mike Yu's avatar Mike Yu Committed by Automerger Merge Worker
Browse files

Revert "Set maximum attempt for opportunistic mode private DNS validation" am:... 

Revert "Set maximum attempt for opportunistic mode private DNS validation" am: 7135fbfc am: 5d3a5c39 am: 563bd668

Original change: https://android-review.googlesource.com/c/platform/packages/modules/DnsResolver/+/1733917

Change-Id: I6e9afded6463b32b3ff2d6e1484a9daea4d1fde2
parents 39ba3bab 563bd668

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