Alternate approach: remmeber the actual orig_ttl. A bit harded than you think, as

in somce cases TTDs are modified later.

diff --git a/pdns/recursordist/recursor_cache.cc b/pdns/recursordist/recursor_cache.cc
index c32364c40d75471411544a351065f1c524dd28d7..4001df41a870e67c7e3b65d80f649a257cd4856b 100644 (file)
--- a/pdns/recursordist/recursor_cache.cc
+++ b/pdns/recursordist/recursor_cache.cc
@@ -531,7 +531,7 @@ bool MemRecursorCache::CacheEntry::shouldReplace(time_t now, bool auth, vState s
   return true;
 }
 
-void MemRecursorCache::replace(time_t now, const DNSName& qname, const QType qt, const vector<DNSRecord>& content, const vector<shared_ptr<const RRSIGRecordContent>>& signatures, const std::vector<std::shared_ptr<DNSRecord>>& authorityRecs, bool auth, const DNSName& authZone, boost::optional<Netmask> ednsmask, const OptTag& routingTag, vState state, boost::optional<ComboAddress> from, bool refresh)
+void MemRecursorCache::replace(time_t now, const DNSName& qname, const QType qt, const vector<DNSRecord>& content, const vector<shared_ptr<const RRSIGRecordContent>>& signatures, const std::vector<std::shared_ptr<DNSRecord>>& authorityRecs, bool auth, const DNSName& authZone, boost::optional<Netmask> ednsmask, const OptTag& routingTag, vState state, boost::optional<ComboAddress> from, bool refresh, uint32_t orig_ttl)
 {
   auto& shard = getMap(qname);
   auto lockedShard = shard.lock();
@@ -608,8 +608,8 @@ void MemRecursorCache::replace(time_t now, const DNSName& qname, const QType qt,
        prior to calling this function, so the TTL actually holds a TTD. */
     ce.d_ttd = min(maxTTD, static_cast<time_t>(i.d_ttl)); // XXX this does weird things if TTLs differ in the set
 
-    ce.d_orig_ttl = ce.d_ttd - now;
-
+    ce.d_orig_ttl = orig_ttl;
+#if 0
     // d_orig_ttl should be between s_minimumTTL and s_maxcachettl, as d_ttd was sanitized wrt those
     // bounds. But our reference point (d_now aka now) might be "too new" at this point, if we went
     // outside to e.g. get DNSKEYS and that took a while. In that case, if the original TTL was
@@ -620,6 +620,7 @@ void MemRecursorCache::replace(time_t now, const DNSName& qname, const QType qt,
     if (ce.d_orig_ttl < SyncRes::s_minimumTTL || ce.d_orig_ttl > SyncRes::s_maxcachettl) {
       ce.d_orig_ttl = SyncRes::s_minimumTTL;
     }
+#endif
     ce.d_records.push_back(i.getContent());
   }
 

diff --git a/pdns/recursordist/recursor_cache.hh b/pdns/recursordist/recursor_cache.hh
index ce37145f003ef178fb2fbd64dd27de70deef1c92..638728a2700389f4ce8c968b8d278c4eebd94370 100644 (file)
--- a/pdns/recursordist/recursor_cache.hh
+++ b/pdns/recursordist/recursor_cache.hh
@@ -69,7 +69,7 @@ public:
 
   time_t get(time_t, const DNSName& qname, const QType qt, Flags flags, vector<DNSRecord>* res, const ComboAddress& who, const OptTag& routingTag = boost::none, vector<std::shared_ptr<const RRSIGRecordContent>>* signatures = nullptr, std::vector<std::shared_ptr<DNSRecord>>* authorityRecs = nullptr, bool* variable = nullptr, vState* state = nullptr, bool* wasAuth = nullptr, DNSName* fromAuthZone = nullptr, ComboAddress* fromAuthIP = nullptr);
 
-  void replace(time_t, const DNSName& qname, const QType qt, const vector<DNSRecord>& content, const vector<shared_ptr<const RRSIGRecordContent>>& signatures, const std::vector<std::shared_ptr<DNSRecord>>& authorityRecs, bool auth, const DNSName& authZone, boost::optional<Netmask> ednsmask = boost::none, const OptTag& routingTag = boost::none, vState state = vState::Indeterminate, boost::optional<ComboAddress> from = boost::none, bool refresh = false);
+  void replace(time_t, const DNSName& qname, const QType qt, const vector<DNSRecord>& content, const vector<shared_ptr<const RRSIGRecordContent>>& signatures, const std::vector<std::shared_ptr<DNSRecord>>& authorityRecs, bool auth, const DNSName& authZone, boost::optional<Netmask> ednsmask = boost::none, const OptTag& routingTag = boost::none, vState state = vState::Indeterminate, boost::optional<ComboAddress> from = boost::none, bool refresh = false, uint32_t orig_ttl = 0);
 
   void doPrune(size_t keep);
   uint64_t doDump(int fd, size_t maxCacheEntries);

diff --git a/pdns/recursordist/syncres.cc b/pdns/recursordist/syncres.cc
index 6119a325e991c8f5c744b884127a8a491fcbbf71..bb19aa9f21522060e60da6052b32a8452156340b 100644 (file)
--- a/pdns/recursordist/syncres.cc
+++ b/pdns/recursordist/syncres.cc
@@ -2627,6 +2627,7 @@ struct CacheEntry
   vector<DNSRecord> records;
   vector<shared_ptr<const RRSIGRecordContent>> signatures;
   uint32_t signaturesTTL{std::numeric_limits<uint32_t>::max()};
+  uint32_t d_orig_ttl{0};
 };
 struct CacheKey
 {
@@ -4410,6 +4411,7 @@ RCode::rcodes_ SyncRes::updateCacheFromRecords(unsigned int depth, const string&
         rec.d_ttl = min(s_maxcachettl, rec.d_ttl);
 
         DNSRecord dr(rec);
+        tcache[{rec.d_name, rec.d_type, rec.d_place}].d_orig_ttl = dr.d_ttl;
         dr.d_ttl += d_now.tv_sec;
         dr.d_place = DNSResourceRecord::ANSWER;
         tcache[{rec.d_name, rec.d_type, rec.d_place}].records.push_back(dr);
@@ -4424,9 +4426,17 @@ RCode::rcodes_ SyncRes::updateCacheFromRecords(unsigned int depth, const string&
     if ((entry.second.records.size() + entry.second.signatures.size() + authorityRecs.size()) > 1) { // need to group the ttl to be the minimum of the RRSET (RFC 2181, 5.2)
       uint32_t lowestTTD = computeLowestTTD(entry.second.records, entry.second.signatures, entry.second.signaturesTTL, authorityRecs);
 
+      uint32_t compensation = 0;
       for (auto& record : entry.second.records) {
+        compensation = record.d_ttl - lowestTTD;
         record.d_ttl = lowestTTD; // boom
       }
+      entry.second.d_orig_ttl -= compensation;
+#if 0
+      if (compensation != 0) {
+        cerr << entry.first.name << '/' << entry.first.type << " compensated " << compensation << endl;
+      }
+#endif
     }
   }
 
@@ -4533,10 +4543,19 @@ RCode::rcodes_ SyncRes::updateCacheFromRecords(unsigned int depth, const string&
     }
 
     if (vStateIsBogus(recordState)) {
+      uint32_t compensation = 0;
       /* this is a TTD by now, be careful */
       for (auto& record : i->second.records) {
-        record.d_ttl = std::min(record.d_ttl, static_cast<uint32_t>(s_maxbogusttl + d_now.tv_sec));
+        auto newval = std::min(record.d_ttl, static_cast<uint32_t>(s_maxbogusttl + d_now.tv_sec));
+        compensation = record.d_ttl - newval;
+        record.d_ttl = newval;
       }
+      i->second.d_orig_ttl -= compensation;
+#if 0
+      if (compensation != 0) {
+        cerr << i->first.name << '/' << i->first.type << " compensated by bogus case " << compensation << endl;
+      }
+#endif
     }
 
     /* We don't need to store NSEC3 records in the positive cache because:
@@ -4582,7 +4601,7 @@ RCode::rcodes_ SyncRes::updateCacheFromRecords(unsigned int depth, const string&
         if (isAA && i->first.type == QType::NS && s_save_parent_ns_set) {
           rememberParentSetIfNeeded(i->first.name, i->second.records, depth, prefix);
         }
-        g_recCache->replace(d_now.tv_sec, i->first.name, i->first.type, i->second.records, i->second.signatures, authorityRecs, i->first.type == QType::DS ? true : isAA, auth, i->first.place == DNSResourceRecord::ANSWER ? ednsmask : boost::none, d_routingTag, recordState, remoteIP, d_refresh);
+        g_recCache->replace(d_now.tv_sec, i->first.name, i->first.type, i->second.records, i->second.signatures, authorityRecs, i->first.type == QType::DS ? true : isAA, auth, i->first.place == DNSResourceRecord::ANSWER ? ednsmask : boost::none, d_routingTag, recordState, remoteIP, d_refresh, i->second.d_orig_ttl);
 
         // Delete potential negcache entry. When a record recovers with serve-stale the negcache entry can cause the wrong entry to
         // be served, as negcache entries are checked before record cache entries
@@ -4606,7 +4625,7 @@ RCode::rcodes_ SyncRes::updateCacheFromRecords(unsigned int depth, const string&
               content.push_back(std::move(nonExpandedRecord));
             }
 
-            g_recCache->replace(d_now.tv_sec, realOwner, QType(i->first.type), content, i->second.signatures, /* no additional records in that case */ {}, i->first.type == QType::DS ? true : isAA, auth, boost::none, boost::none, recordState, remoteIP, d_refresh);
+            g_recCache->replace(d_now.tv_sec, realOwner, QType(i->first.type), content, i->second.signatures, /* no additional records in that case */ {}, i->first.type == QType::DS ? true : isAA, auth, boost::none, boost::none, recordState, remoteIP, d_refresh, i->second.d_orig_ttl);
           }
         }
       }

diff --git a/pdns/recursordist/test-syncres_cc2.cc b/pdns/recursordist/test-syncres_cc2.cc
index 8d4bb74b50d17b68a5ee49aef6ffb4c8748a1aba..7c51872db1ec4eadd943033f16152b12242a01f7 100644 (file)
--- a/pdns/recursordist/test-syncres_cc2.cc
+++ b/pdns/recursordist/test-syncres_cc2.cc
@@ -1806,12 +1806,12 @@ BOOST_AUTO_TEST_CASE(test_cache_almost_expired_ttl)
   std::vector<shared_ptr<const RRSIGRecordContent>> sigs;
   addRecordToList(records, target, QType::A, "192.0.2.2", DNSResourceRecord::ANSWER, now + 29);
 
-  g_recCache->replace(now - 30, target, QType(QType::A), records, sigs, vector<std::shared_ptr<DNSRecord>>(), true, g_rootdnsname, boost::optional<Netmask>());
+  g_recCache->replace(now - 30, target, QType(QType::A), records, sigs, {}, true, g_rootdnsname, boost::optional<Netmask>(), boost::none, vState::Indeterminate, boost::none, false, 60);
 
   /* Same for the NS record */
   std::vector<DNSRecord> ns;
   addRecordToList(ns, target, QType::NS, "pdns-public-ns1.powerdns.com", DNSResourceRecord::ANSWER, now + 29);
-  g_recCache->replace(now - 30, target, QType::NS, ns, sigs, vector<std::shared_ptr<DNSRecord>>(), false, target, boost::optional<Netmask>());
+  g_recCache->replace(now - 30, target, QType::NS, ns, sigs, {}, false, target, boost::optional<Netmask>(), boost::none, vState::Indeterminate, boost::none, false, 60);
 
   vector<DNSRecord> ret;
   int res = sr->beginResolve(target, QType(QType::A), QClass::IN, ret);
@@ -1823,7 +1823,7 @@ BOOST_AUTO_TEST_CASE(test_cache_almost_expired_ttl)
   BOOST_CHECK_EQUAL(ttl, 29U);
 
   // One task should be submitted
-  BOOST_CHECK_EQUAL(getTaskSize(), 1U);
+  BOOST_REQUIRE_EQUAL(getTaskSize(), 1U);
 
   auto task = taskQueuePop();