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 51bfa3663b58db4ecc2282c1f9ec8b7916851ddd..7e0bf054ce2b5c4193c5bdd03cdd289492588fce 100644 (file)
--- 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<DNSRecord>& content, const vector<shared_ptr<RRSIGRecordContent>>& signatures, const std::vector<std::shared_ptr<DNSRecord>>& authorityRecs, bool auth, boost::optional<Netmask> ednsmask, time_t minTTD, vState state)
+void MemRecursorCache::replace(time_t now, const DNSName &qname, const QType& qt, const vector<DNSRecord>& content, const vector<shared_ptr<RRSIGRecordContent>>& signatures, const std::vector<std::shared_ptr<DNSRecord>>& authorityRecs, bool auth, boost::optional<Netmask> ednsmask, vState state)
 {
   d_cachecachevalid = false;
   //  cerr<<"Replacing "<<qname<<" for "<< (ednsmask ? ednsmask->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<time_t>(i.d_ttl) >= minTTD) {
-      ce.d_ttd = min(maxTTD, static_cast<time_t>(i.d_ttl));   // XXX this does weird things if TTLs differ in the set
-      //    cerr<<"To store: "<<i.d_content->getZoneRepresentation()<<" with ttl/ttd "<<i.d_ttl<<", capped at: "<<maxTTD<<endl;
-      ce.d_records.push_back(i.d_content);
-    }
+    ce.d_ttd=min(maxTTD, static_cast<time_t>(i.d_ttl));   // XXX this does weird things if TTLs differ in the set
+    //    cerr<<"To store: "<<i.d_content->getZoneRepresentation()<<" with ttl/ttd "<<i.d_ttl<<", capped at: "<<maxTTD<<endl;
+    ce.d_records.push_back(i.d_content);
   }
 
   if (!isNew) {

diff --git a/pdns/recursor_cache.hh b/pdns/recursor_cache.hh
index 3e841c5e612272d0a812de495ee3858e54063227..e9a44c86c747c4edd173fa3d9402b4e47e343a91 100644 (file)
--- a/pdns/recursor_cache.hh
+++ b/pdns/recursor_cache.hh
@@ -57,7 +57,7 @@ public:
 
   int32_t get(time_t, const DNSName &qname, const QType& qt, bool requireAuth, vector<DNSRecord>* res, const ComboAddress& who, vector<std::shared_ptr<RRSIGRecordContent>>* signatures=nullptr, std::vector<std::shared_ptr<DNSRecord>>* authorityRecs=nullptr, bool* variable=nullptr, vState* state=nullptr, bool* wasAuth=nullptr);
 
-  void replace(time_t, const DNSName &qname, const QType& qt,  const vector<DNSRecord>& content, const vector<shared_ptr<RRSIGRecordContent>>& signatures, const std::vector<std::shared_ptr<DNSRecord>>& authorityRecs, bool auth, boost::optional<Netmask> ednsmask=boost::none, time_t minTTD = 0, vState state=Indeterminate);
+  void replace(time_t, const DNSName &qname, const QType& qt,  const vector<DNSRecord>& content, const vector<shared_ptr<RRSIGRecordContent>>& signatures, const std::vector<std::shared_ptr<DNSRecord>>& authorityRecs, bool auth, boost::optional<Netmask> 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 a2ed49b8a2ba696f77cb09c69838f199816bb106..bfeac254552be3c6c124f2d00e01454a3d2cf6cd 100644 (file)
--- 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<SyncRes> 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<const EDNSSubnetOpts&>(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<Netmask>& srcmask, boost::optional<const ResolveContext&> 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<DNSRecord> 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<DNSRecord> 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<SyncRes> 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<const EDNSSubnetOpts&>(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<Netmask>& srcmask, boost::optional<const ResolveContext&> 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<DNSRecord> 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 52c75c8d279d098143c47d8e8be99b32af58ccd7..cb7cbf4df2f43d31a1f0506cd92e09806cff9cb4 100644 (file)
--- 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<uint64_t> SyncRes::s_authzonequeries;
 std::atomic<uint64_t> SyncRes::s_queries;
 std::atomic<uint64_t> 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 7b0734fd19000bdc570658605098263abdcbfb8e..9570ab22a8fe1b88bb80a5b67b0b3de700fb13db 100644 (file)
--- 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;