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

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

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

Merge "Set maximum attempt for opportunistic mode private DNS validation" am: b977d1ac am: 23b798f2 am: 80975daf

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

Change-Id: Ib631c96f57c95cc060dc6f0c3e4f9f39704d37cf
parents 6317e225 80975daf

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