From: Otto Moerbeek Date: Fri, 29 Mar 2019 10:40:05 +0000 (+0100) Subject: New approach. I spelled out the logic to make it more clear. X-Git-Tag: dnsdist-1.4.0-alpha1~37^2~2 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=e7861cc408a4984d7c3e2a430825beb22ecf2a6d;p=thirdparty%2Fpdns.git New approach. I spelled out the logic to make it more clear. Points to keep in mind: > vs >= What do we do if s_ecscachelimitttl is not set? I chose to let the scope determine cacheability. --- diff --git a/pdns/recursor_cache.cc b/pdns/recursor_cache.cc index 51bfa3663b..7e0bf054ce 100644 --- a/pdns/recursor_cache.cc +++ b/pdns/recursor_cache.cc @@ -236,7 +236,7 @@ int32_t MemRecursorCache::get(time_t now, const DNSName &qname, const QType& qt, return -1; } -void MemRecursorCache::replace(time_t now, const DNSName &qname, const QType& qt, const vector& content, const vector>& signatures, const std::vector>& authorityRecs, bool auth, boost::optional ednsmask, time_t minTTD, vState state) +void MemRecursorCache::replace(time_t now, const DNSName &qname, const QType& qt, const vector& content, const vector>& signatures, const std::vector>& authorityRecs, bool auth, boost::optional ednsmask, vState state) { d_cachecachevalid = false; // cerr<<"Replacing "<toString() : "everyone") << endl; @@ -305,11 +305,9 @@ void MemRecursorCache::replace(time_t now, const DNSName &qname, const QType& qt for(const auto i : content) { /* Yes, we have altered the d_ttl value by adding time(nullptr) to it prior to calling this function, so the TTL actually holds a TTD. */ - if (minTTD == 0 || static_cast(i.d_ttl) >= minTTD) { - ce.d_ttd = min(maxTTD, static_cast(i.d_ttl)); // XXX this does weird things if TTLs differ in the set - // cerr<<"To store: "<getZoneRepresentation()<<" with ttl/ttd "<(i.d_ttl)); // XXX this does weird things if TTLs differ in the set + // cerr<<"To store: "<getZoneRepresentation()<<" with ttl/ttd "<* res, const ComboAddress& who, vector>* signatures=nullptr, std::vector>* authorityRecs=nullptr, bool* variable=nullptr, vState* state=nullptr, bool* wasAuth=nullptr); - void replace(time_t, const DNSName &qname, const QType& qt, const vector& content, const vector>& signatures, const std::vector>& authorityRecs, bool auth, boost::optional ednsmask=boost::none, time_t minTTD = 0, vState state=Indeterminate); + void replace(time_t, const DNSName &qname, const QType& qt, const vector& content, const vector>& signatures, const std::vector>& authorityRecs, bool auth, boost::optional ednsmask=boost::none, vState state=Indeterminate); void doPrune(unsigned int keep); uint64_t doDump(int fd); diff --git a/pdns/recursordist/test-syncres_cc.cc b/pdns/recursordist/test-syncres_cc.cc index a2ed49b8a2..bfeac25455 100644 --- a/pdns/recursordist/test-syncres_cc.cc +++ b/pdns/recursordist/test-syncres_cc.cc @@ -2221,6 +2221,46 @@ BOOST_AUTO_TEST_CASE(test_ecs_cache_ttllimit_allowed) { BOOST_REQUIRE_EQUAL(cached.size(), 1); } +BOOST_AUTO_TEST_CASE(test_ecs_cache_ttllimit_and_scope_allowed) { + std::unique_ptr sr; + initSR(sr); + + primeHints(); + + const DNSName target("www.powerdns.com."); + + SyncRes::addEDNSDomain(DNSName("powerdns.com.")); + + EDNSSubnetOpts incomingECS; + incomingECS.source = Netmask("192.0.2.128/32"); + sr->setQuerySource(ComboAddress(), boost::optional(incomingECS)); + SyncRes::s_ecscachelimitttl = 100; + SyncRes::s_ecsipv4cachelimit = 24; + + sr->setAsyncCallback([target](const ComboAddress& ip, const DNSName& domain, int type, bool doTCP, bool sendRDQuery, int EDNS0Level, struct timeval* now, boost::optional& srcmask, boost::optional context, LWResult* res, bool* chained) { + + BOOST_REQUIRE(srcmask); + BOOST_CHECK_EQUAL(srcmask->toString(), "192.0.2.0/24"); + + setLWResult(res, 0, true, false, true); + addRecordToLW(res, target, QType::A, "192.0.2.1"); + + return 1; + }); + + const time_t now = sr->getNow().tv_sec; + vector ret; + int res = sr->beginResolve(target, QType(QType::A), QClass::IN, ret); + BOOST_CHECK_EQUAL(res, RCode::NoError); + BOOST_CHECK_EQUAL(ret.size(), 1); + + /* should have been cached */ + const ComboAddress who("192.0.2.128"); + vector cached; + BOOST_REQUIRE_GT(t_RC->get(now, target, QType(QType::A), true, &cached, who), 0); + BOOST_REQUIRE_EQUAL(cached.size(), 1); +} + BOOST_AUTO_TEST_CASE(test_ecs_cache_ttllimit_notallowed) { std::unique_ptr sr; initSR(sr); @@ -2235,6 +2275,7 @@ BOOST_AUTO_TEST_CASE(test_ecs_cache_ttllimit_notallowed) { incomingECS.source = Netmask("192.0.2.128/32"); sr->setQuerySource(ComboAddress(), boost::optional(incomingECS)); SyncRes::s_ecscachelimitttl = 100; + SyncRes::s_ecsipv4cachelimit = 16; sr->setAsyncCallback([target](const ComboAddress& ip, const DNSName& domain, int type, bool doTCP, bool sendRDQuery, int EDNS0Level, struct timeval* now, boost::optional& srcmask, boost::optional context, LWResult* res, bool* chained) { @@ -2253,7 +2294,7 @@ BOOST_AUTO_TEST_CASE(test_ecs_cache_ttllimit_notallowed) { BOOST_CHECK_EQUAL(res, RCode::NoError); BOOST_CHECK_EQUAL(ret.size(), 1); - /* should have NOT been cached because TTL of 60 is too small */ + /* should have NOT been cached because TTL of 60 is too small and /24 is more specific than /16 */ const ComboAddress who("192.0.2.128"); vector cached; BOOST_REQUIRE_LT(t_RC->get(now, target, QType(QType::A), true, &cached, who), 0); diff --git a/pdns/syncres.cc b/pdns/syncres.cc index 52c75c8d27..cb7cbf4df2 100644 --- a/pdns/syncres.cc +++ b/pdns/syncres.cc @@ -60,6 +60,7 @@ unsigned int SyncRes::s_packetcachettl; unsigned int SyncRes::s_packetcacheservfailttl; unsigned int SyncRes::s_serverdownmaxfails; unsigned int SyncRes::s_serverdownthrottletime; +unsigned int SyncRes::s_ecscachelimitttl; std::atomic SyncRes::s_authzonequeries; std::atomic SyncRes::s_queries; std::atomic SyncRes::s_outgoingtimeouts; @@ -80,7 +81,6 @@ uint8_t SyncRes::s_ecsipv4limit; uint8_t SyncRes::s_ecsipv6limit; uint8_t SyncRes::s_ecsipv4cachelimit; uint8_t SyncRes::s_ecsipv6cachelimit; -unsigned int SyncRes::s_ecscachelimitttl; bool SyncRes::s_doIPv6; bool SyncRes::s_nopacketcache; @@ -2420,15 +2420,36 @@ RCode::rcodes_ SyncRes::updateCacheFromRecords(unsigned int depth, LWResult& lwr - NS, A and AAAA (used for infra queries) */ if (i->first.type != QType::NSEC3 && (i->first.type == QType::DS || i->first.type == QType::NS || i->first.type == QType::A || i->first.type == QType::AAAA || isAA || wasForwardRecurse)) { - if (i->first.place != DNSResourceRecord::ANSWER || - !ednsmask || - (ednsmask->isIpv4() && ednsmask->getBits() <= SyncRes::s_ecsipv4cachelimit) || - (ednsmask->isIpv6() && ednsmask->getBits() <= SyncRes::s_ecsipv6cachelimit)) { - time_t minTTD = 0; - if (ednsmask && SyncRes::s_ecscachelimitttl > 0) { - minTTD = SyncRes::s_ecscachelimitttl + d_now.tv_sec; + + bool doCache = i->first.place != DNSResourceRecord::ANSWER || !ednsmask; + // if ednsmask is relevant, we do not want to cache if the scope > ecslimit and TTL < limitttl + if (!doCache && ednsmask) { + bool manyMaskBits = (ednsmask->isIpv4() && ednsmask->getBits() > SyncRes::s_ecsipv4cachelimit) || + (ednsmask->isIpv6() && ednsmask->getBits() > SyncRes::s_ecsipv6cachelimit); + + if (SyncRes::s_ecscachelimitttl > 0) { + if (manyMaskBits) { + uint32_t minttl = UINT32_MAX; + for (const auto &it : i->second.records) { + if (it.d_ttl < minttl) + minttl = it.d_ttl; + } + bool ttlIsSmall = minttl < SyncRes::s_ecscachelimitttl + d_now.tv_sec; + if (ttlIsSmall) { + // Case: many bits and ttlIsSmall + doCache = false; + } + } else { + // Case: few mask bits + doCache = true; + } + } else { + // no applicable TTL limit, scope determines cacheability + doCache = !manyMaskBits; } - t_RC->replace(d_now.tv_sec, i->first.name, QType(i->first.type), i->second.records, i->second.signatures, authorityRecs, i->first.type == QType::DS ? true : isAA, i->first.place == DNSResourceRecord::ANSWER ? ednsmask : boost::none, minTTD, recordState); + } + if (doCache) { + t_RC->replace(d_now.tv_sec, i->first.name, QType(i->first.type), i->second.records, i->second.signatures, authorityRecs, i->first.type == QType::DS ? true : isAA, i->first.place == DNSResourceRecord::ANSWER ? ednsmask : boost::none, recordState); } } diff --git a/pdns/syncres.hh b/pdns/syncres.hh index 7b0734fd19..9570ab22a8 100644 --- a/pdns/syncres.hh +++ b/pdns/syncres.hh @@ -716,11 +716,11 @@ public: static unsigned int s_packetcacheservfailttl; static unsigned int s_serverdownmaxfails; static unsigned int s_serverdownthrottletime; + static unsigned int s_ecscachelimitttl; static uint8_t s_ecsipv4limit; static uint8_t s_ecsipv6limit; static uint8_t s_ecsipv4cachelimit; static uint8_t s_ecsipv6cachelimit; - static unsigned int s_ecscachelimitttl; static bool s_doIPv6; static bool s_noEDNSPing; static bool s_noEDNS;