From: Otto Moerbeek Date: Wed, 16 Feb 2022 09:31:31 +0000 (+0100) Subject: General rate limit on taskqueue tasks X-Git-Tag: rec-4.7.0-alpha1~10^2~4 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=33805cfc0a17a40466f196f2d943a063bb8ddedb;p=thirdparty%2Fpdns.git General rate limit on taskqueue tasks --- diff --git a/pdns/recursordist/rec-taskqueue.cc b/pdns/recursordist/rec-taskqueue.cc index 4e2f5f1f23..f59a350515 100644 --- a/pdns/recursordist/rec-taskqueue.cc +++ b/pdns/recursordist/rec-taskqueue.cc @@ -26,12 +26,66 @@ #include "stat_t.hh" #include "syncres.hh" -struct Queues +// For rate lmiting, we maintain a set of tasks recently submitted. +class TimedSet +{ +public: + TimedSet(time_t t) : + d_expiry_seconds(t) + { + } + bool insert(time_t now, const pdns::ResolveTask& task) + { + time_t ttd = now + d_expiry_seconds; + bool inserted = d_set.emplace(task, ttd).second; + if (!inserted) { + // Instead of a periodic clean, we always do it on a hit + // the operation should be cheap as we just walk the ordered time_t index + // There is a slim chance if we never hit a rate limiting case we'll never clean... oh well + auto& ind = d_set.template get(); + auto it = ind.begin(); + bool erased = false; + while (it != ind.end()) { + if (it->d_ttd < now) { + erased = true; + it = ind.erase(it); + } + else { + break; + } + } + // Try again if the loop deleted at least one entry + if (erased) { + inserted = d_set.emplace(task, ttd).second; + } + } + return inserted; + } + +private: + struct Entry + { + Entry(const pdns::ResolveTask& task, time_t ttd) : + d_task(task), d_ttd(ttd) {} + pdns::ResolveTask d_task; + time_t d_ttd; + }; + + typedef multi_index_container, member>, + ordered_non_unique, member>>> + timed_set_t; + timed_set_t d_set; + time_t d_expiry_seconds; +}; + +struct Queue { pdns::TaskQueue queue; - std::set running; + TimedSet rateLimitSet{60}; }; -static LockGuarded s_taskQueue; +static LockGuarded s_taskQueue; struct taskstats { @@ -55,7 +109,7 @@ static void resolve(const struct timeval& now, bool logErrors, const pdns::Resol log->info(Logr::Debug, "resolving"); int res = sr.beginResolve(task.d_qname, QType(task.d_qtype), QClass::IN, ret); ex = false; - log->info(Logr::Debug, "done", "rcode", Logging::Loggable(res), "records", Logging::Loggable(ret.size())); + log->info(Logr::Debug, "done", "rcode", Logging::Loggable(res), "records", Logging::Loggable(ret.size())); } catch (const std::exception& e) { log->error(Logr::Error, msg, e.what()); @@ -103,10 +157,8 @@ void runTaskOnce(bool logErrors) return; } task = lock->queue.pop(); - lock->running.insert(task); } bool expired = task.run(logErrors); - s_taskQueue.lock()->running.erase(task); if (expired) { s_taskQueue.lock()->queue.incExpired(); } @@ -116,7 +168,7 @@ void pushAlmostExpiredTask(const DNSName& qname, uint16_t qtype, time_t deadline { pdns::ResolveTask task{qname, qtype, deadline, true, resolve}; auto lock = s_taskQueue.lock(); - bool running = lock->running.count(task) > 0; + bool running = !lock->rateLimitSet.insert(time(nullptr), task); if (!running) { ++s_almost_expired_tasks.pushed; lock->queue.push(std::move(task)); @@ -127,7 +179,7 @@ void pushResolveTask(const DNSName& qname, uint16_t qtype, time_t deadline) { pdns::ResolveTask task{qname, qtype, deadline, false, resolve}; auto lock = s_taskQueue.lock(); - bool running = lock->running.count(task) > 0; + bool running = !lock->rateLimitSet.insert(time(nullptr), task); if (!running) { ++s_resolve_tasks.pushed; lock->queue.push(std::move(task)); diff --git a/pdns/recursordist/rec-taskqueue.hh b/pdns/recursordist/rec-taskqueue.hh index fe1fea6bfc..b33e2abc51 100644 --- a/pdns/recursordist/rec-taskqueue.hh +++ b/pdns/recursordist/rec-taskqueue.hh @@ -21,7 +21,10 @@ */ #pragma once -#include "dnsname.hh" +#include +#include + +class DNSName; void runTaskOnce(bool logErrors); void pushAlmostExpiredTask(const DNSName& qname, uint16_t qtype, time_t deadline); diff --git a/pdns/recursordist/taskqueue.cc b/pdns/recursordist/taskqueue.cc index 7e810015b2..600bec3b9c 100644 --- a/pdns/recursordist/taskqueue.cc +++ b/pdns/recursordist/taskqueue.cc @@ -46,7 +46,7 @@ ResolveTask TaskQueue::pop() bool ResolveTask::run(bool logErrors) { - if (!d_func) { + if (d_func == nullptr) { auto log = g_slog->withName("taskq")->withValues("name", Logging::Loggable(d_qname), "qtype", Logging::Loggable(QType(d_qtype).toString())); log->error(Logr::Debug, "null task"); return false; diff --git a/pdns/recursordist/taskqueue.hh b/pdns/recursordist/taskqueue.hh index a457823138..abdd768439 100644 --- a/pdns/recursordist/taskqueue.hh +++ b/pdns/recursordist/taskqueue.hh @@ -45,11 +45,12 @@ struct ResolveTask uint16_t d_qtype; time_t d_deadline; bool d_refreshMode; // Whether to run this task in regular mode (false) or in the mode that refreshes almost expired tasks - std::function d_func; + // Use a function ponter as comparing std::functions is a nuisance + void (*d_func)(const struct timeval& now, bool logErrors, const ResolveTask& task); bool operator<(const ResolveTask& a) const { - return std::tie(d_qname, d_qtype, d_refreshMode) < std::tie(d_qname, d_qtype, d_refreshMode); + return std::tie(d_qname, d_qtype, d_refreshMode, d_func) < std::tie(a.d_qname, a.d_qtype, a.d_refreshMode, a.d_func); } bool run(bool logErrors); }; diff --git a/pdns/recursordist/test-syncres_cc2.cc b/pdns/recursordist/test-syncres_cc2.cc index 15d53bf7fe..8e46deb05f 100644 --- a/pdns/recursordist/test-syncres_cc2.cc +++ b/pdns/recursordist/test-syncres_cc2.cc @@ -311,10 +311,6 @@ BOOST_AUTO_TEST_CASE(test_glueless_referral_with_non_resolving) BOOST_REQUIRE_EQUAL(ret.size(), 0U); BOOST_CHECK_EQUAL(SyncRes::getNonResolvingNSSize(), 2U); - // Originally empty NoData results where not cached, now they are - BOOST_CHECK_EQUAL(g_negCache->size(), 4U); - g_negCache->clear(); - // Again, should not change anything res = sr->beginResolve(target, QType(QType::A), QClass::IN, ret); BOOST_CHECK_EQUAL(res, RCode::ServFail); @@ -324,10 +320,6 @@ BOOST_AUTO_TEST_CASE(test_glueless_referral_with_non_resolving) BOOST_CHECK_EQUAL(SyncRes::getNonResolvingNSSize(), 2U); - // Originally empty NoData results where not cached, now they are - BOOST_CHECK_EQUAL(g_negCache->size(), 4U); - g_negCache->clear(); - // Again, but now things should start working because of the queryCounter getting high enough // and one entry remains in the non-resolving cache res = sr->beginResolve(target, QType(QType::A), QClass::IN, ret); diff --git a/pdns/recursordist/test-syncres_cc3.cc b/pdns/recursordist/test-syncres_cc3.cc index 29f881d809..bcd6ddbb23 100644 --- a/pdns/recursordist/test-syncres_cc3.cc +++ b/pdns/recursordist/test-syncres_cc3.cc @@ -1210,8 +1210,8 @@ BOOST_AUTO_TEST_CASE(test_forward_zone_recurse_rd_dnssec_nodata_bogus) BOOST_CHECK_EQUAL(res, RCode::NoError); BOOST_CHECK_EQUAL(sr->getValidationState(), vState::BogusMissingNegativeIndication); BOOST_REQUIRE_EQUAL(ret.size(), 0U); - /* we don store empty results */ - BOOST_CHECK_EQUAL(queriesCount, 3U); + /* we don't store empty results */ + BOOST_CHECK_EQUAL(queriesCount, 4U); } BOOST_AUTO_TEST_CASE(test_auth_zone_oob) diff --git a/pdns/recursordist/test-syncres_cc7.cc b/pdns/recursordist/test-syncres_cc7.cc index 16cb4e32ad..7d4484175f 100644 --- a/pdns/recursordist/test-syncres_cc7.cc +++ b/pdns/recursordist/test-syncres_cc7.cc @@ -1416,8 +1416,8 @@ BOOST_AUTO_TEST_CASE(test_dnssec_bogus_nodata) BOOST_CHECK_EQUAL(res, RCode::NoError); BOOST_CHECK_EQUAL(sr->getValidationState(), vState::BogusMissingNegativeIndication); BOOST_REQUIRE_EQUAL(ret.size(), 0U); - /* we do store empty results */ - BOOST_CHECK_EQUAL(queriesCount, 3U); + /* we don't store empty results */ + BOOST_CHECK_EQUAL(queriesCount, 4U); } BOOST_AUTO_TEST_CASE(test_dnssec_bogus_nxdomain) diff --git a/pdns/syncres.cc b/pdns/syncres.cc index bb39a50822..11973c56e4 100644 --- a/pdns/syncres.cc +++ b/pdns/syncres.cc @@ -3642,18 +3642,6 @@ bool SyncRes::processRecords(const std::string& prefix, const DNSName& qname, co uint32_t dnameTTL = 0; bool referralOnDS = false; - if (lwr.d_rcode == 0 && qtype.getCode() != 0 && lwr.d_records.size() == 0) { - // NODATA and no SOA, put that into the negcache for a while - NegCache::NegCacheEntry ne; - ne.d_auth = auth; - ne.d_name = qname; - ne.d_qtype = qtype; - ne.d_ttd = d_now.tv_sec + std::max(s_minimumTTL, 60U); - ne.d_validationState = vState::BogusMissingNegativeIndication; - LOG(prefix<add(ne); - } - for (auto& rec : lwr.d_records) { if (rec.d_type != QType::OPT && rec.d_class != QClass::IN) { continue;