From: Remi Gacogne <remi.gacogne@powerdns.com>
Date: Thu, 5 Apr 2018 13:48:08 +0000 (+0200)
Subject: rec: Clean up the insertion code in the recursor's cache
X-Git-Tag: dnsdist-1.3.1~163^2~1
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=48f19abe7d686398f9c07512598b606f8c7da6e2;p=thirdparty%2Fpdns.git

rec: Clean up the insertion code in the recursor's cache
---

diff --git a/pdns/recursor_cache.cc b/pdns/recursor_cache.cc
index 30286064f1..acaad7e140 100644
--- a/pdns/recursor_cache.cc
+++ b/pdns/recursor_cache.cc
@@ -232,34 +232,6 @@ int32_t MemRecursorCache::get(time_t now, const DNSName &qname, const QType& qt,
   return -1;
 }
 
-bool MemRecursorCache::attemptToRefreshNSTTL(const QType& qt, const vector<DNSRecord>& content, const CacheEntry& stored)
-{
-  if(!stored.d_auth) {
-    //~ cerr<<"feel free to scribble non-auth data!"<<endl;
-    return false;
-  }
-
-  if(qt.getCode()!=QType::NS) {
-    //~ cerr<<"Not NS record"<<endl;
-    return false;
-  }
-  if(content.size()!=stored.d_records.size()) {
-    //~ cerr<<"Not equal number of records"<<endl;
-    return false;
-  }
-  if(stored.d_records.empty())
-    return false;
-
-  if(stored.d_ttd > content.begin()->d_ttl) {
-    //~ cerr<<"attempt to LOWER TTL - fine by us"<<endl;
-    return false;
-  }
-
-
-//  cerr<<"Returning true - update attempt!\n";
-  return true;
-}
-
 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;
@@ -268,7 +240,7 @@ void MemRecursorCache::replace(time_t now, const DNSName &qname, const QType& qt
   bool isNew = false;
   cache_t::iterator stored = d_cache.find(key);
   if (stored == d_cache.end()) {
-    stored = d_cache.insert(CacheEntry(key, CacheEntry::records_t(), auth)).first;
+    stored = d_cache.insert(CacheEntry(key, auth)).first;
     isNew = true;
   }
 
@@ -309,22 +281,22 @@ void MemRecursorCache::replace(time_t now, const DNSName &qname, const QType& qt
       ce.d_auth = false;  // new data won't be auth
     }
   }
-  ce.d_records.clear();
 
-  // limit TTL of auth->auth NSset update if needed, except for root 
+  // refuse any attempt to *raise* the TTL of auth NS records, as it would make it possible
+  // for an auth to keep a "ghost" zone alive forever, even after the delegation is gone from
+  // the parent
+  // BUT make sure that we CAN refresh the root
   if(ce.d_auth && auth && qt.getCode()==QType::NS && !isNew && !qname.isRoot()) {
     //    cerr<<"\tLimiting TTL of auth->auth NS set replace to "<<ce.d_ttd<<endl;
     maxTTD = ce.d_ttd;
   }
 
-  // make sure that we CAN refresh the root
-  if(auth && (qname.isRoot() || !attemptToRefreshNSTTL(qt, content, ce) ) ) {
-    // cerr<<"\tGot auth data, and it was not refresh attempt of an unchanged NS set, nuking storage"<<endl;
-    ce.d_records.clear(); // clear non-auth data
+  if(auth) {
     ce.d_auth = true;
   }
-  //else cerr<<"\tNot nuking"<<endl;
 
+  ce.d_records.clear();
+  ce.d_records.reserve(content.size());
 
   for(const auto i : content) {
     /* Yes, we have altered the d_ttl value by adding time(nullptr) to it
@@ -332,7 +304,6 @@ void MemRecursorCache::replace(time_t now, const DNSName &qname, const QType& qt
     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);
-    // there was code here that did things with TTL and auth. Unsure if it was good. XXX
   }
 
   if (!isNew) {
diff --git a/pdns/recursor_cache.hh b/pdns/recursor_cache.hh
index afbee5e572..bf35c9fc1c 100644
--- a/pdns/recursor_cache.hh
+++ b/pdns/recursor_cache.hh
@@ -72,9 +72,10 @@ private:
 
   struct CacheEntry
   {
-    CacheEntry(const boost::tuple<DNSName, uint16_t, Netmask>& key, const vector<shared_ptr<DNSRecordContent>>& records, bool auth) : 
-      d_records(records), d_qname(key.get<0>()), d_netmask(key.get<2>()), d_state(Indeterminate), d_ttd(0), d_qtype(key.get<1>()), d_auth(auth)
-    {}
+    CacheEntry(const boost::tuple<DNSName, uint16_t, Netmask>& key, bool auth):
+      d_qname(key.get<0>()), d_netmask(key.get<2>()), d_state(Indeterminate), d_ttd(0), d_qtype(key.get<1>()), d_auth(auth)
+    {
+    }
 
     typedef vector<std::shared_ptr<DNSRecordContent>> records_t;
     time_t getTTD() const
@@ -176,7 +177,6 @@ private:
   DNSName d_cachedqname;
   bool d_cachecachevalid;
 
-  bool attemptToRefreshNSTTL(const QType& qt, const vector<DNSRecord>& content, const CacheEntry& stored);
   bool entryMatches(cache_t::const_iterator& entry, uint16_t qt, bool requireAuth, const ComboAddress& who);
   std::pair<cache_t::const_iterator, cache_t::const_iterator> getEntries(const DNSName &qname, const QType& qt);
   cache_t::const_iterator getEntryUsingECSIndex(time_t now, const DNSName &qname, uint16_t qtype, bool requireAuth, const ComboAddress& who);