From: Otto Moerbeek <otto.moerbeek@open-xchange.com>
Date: Mon, 16 May 2022 08:33:20 +0000 (+0200)
Subject: Only store edns status if it is not EDNSOK
X-Git-Tag: auth-4.8.0-alpha0~53^2~3
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=3a016e9234658139a7a896b482ff9d4241f835cf;p=thirdparty%2Fpdns.git

Only store edns status if it is not EDNSOK
---

diff --git a/pdns/recursordist/rec-main.cc b/pdns/recursordist/rec-main.cc
index a5c6bc1a61..649b423169 100644
--- a/pdns/recursordist/rec-main.cc
+++ b/pdns/recursordist/rec-main.cc
@@ -2117,7 +2117,7 @@ static void houseKeeping(void*)
 
       static PeriodicTask pruneEDNSTask{"pruneEDNSTask", 60};
       pruneEDNSTask.runIfDue(now, [now]() {
-        SyncRes::pruneEDNSStatuses(now.tv_sec - 2 * 3600);
+        SyncRes::pruneEDNSStatuses(now.tv_sec);
       });
 
       if (SyncRes::s_max_busy_dot_probes > 0) {
diff --git a/pdns/recursordist/test-syncres_cc1.cc b/pdns/recursordist/test-syncres_cc1.cc
index c58550d9f4..29f1a3fea6 100644
--- a/pdns/recursordist/test-syncres_cc1.cc
+++ b/pdns/recursordist/test-syncres_cc1.cc
@@ -348,7 +348,7 @@ BOOST_AUTO_TEST_CASE(test_edns_formerr_but_edns_enabled)
   BOOST_CHECK_EQUAL(ret.size(), 0U);
   BOOST_CHECK_EQUAL(queriesWithEDNS, 26U);
   BOOST_CHECK_EQUAL(queriesWithoutEDNS, 0U);
-  BOOST_CHECK_EQUAL(SyncRes::getEDNSStatusesSize(), 26U);
+  BOOST_CHECK_EQUAL(SyncRes::getEDNSStatusesSize(), 0U);
   BOOST_CHECK_EQUAL(usedServers.size(), 26U);
   for (const auto& server : usedServers) {
     BOOST_CHECK_EQUAL(SyncRes::getEDNSStatus(server), SyncRes::EDNSStatus::EDNSOK);
diff --git a/pdns/syncres.cc b/pdns/syncres.cc
index 8fdc2bd26f..2e115fe69a 100644
--- a/pdns/syncres.cc
+++ b/pdns/syncres.cc
@@ -1063,7 +1063,7 @@ static const char* timestamp(time_t t, char* buf, size_t sz)
 struct ednsstatus_t : public multi_index_container<SyncRes::EDNSStatus,
                                                    indexed_by<
                                                      ordered_unique<tag<ComboAddress>, member<SyncRes::EDNSStatus, ComboAddress, &SyncRes::EDNSStatus::address>>,
-                                                     ordered_non_unique<tag<time_t>, member<SyncRes::EDNSStatus, time_t, &SyncRes::EDNSStatus::modeSetAt>>
+                                                     ordered_non_unique<tag<time_t>, member<SyncRes::EDNSStatus, time_t, &SyncRes::EDNSStatus::ttd>>
                                                      >>
 {
   // Get a copy
@@ -1072,23 +1072,20 @@ struct ednsstatus_t : public multi_index_container<SyncRes::EDNSStatus,
     return *this;
   }
 
-  void reset(index<ComboAddress>::type &ind, iterator it)
-  {
-    ind.modify(it, [](SyncRes::EDNSStatus &s) { s.mode = SyncRes::EDNSStatus::EDNSMode::UNKNOWN; s.modeSetAt = 0; });
-  }
-
   void setMode(index<ComboAddress>::type &ind, iterator it, SyncRes::EDNSStatus::EDNSMode mode, time_t ts)
   {
-    if (it->mode != mode || it->modeSetAt == 0) {
-      ind.modify(it, [=](SyncRes::EDNSStatus &s) { s.mode = mode; s.modeSetAt = ts; });
+    if (it->mode != mode || it->ttd == 0) {
+      ind.modify(it, [=](SyncRes::EDNSStatus &s) { s.mode = mode; s.ttd = ts + Expire; });
     }
   }
 
-  void prune(time_t cutoff)
+  void prune(time_t now)
   {
     auto &ind = get<time_t>();
-    ind.erase(ind.begin(), ind.upper_bound(cutoff));
+    ind.erase(ind.begin(), ind.upper_bound(now));
   }
+
+  static const time_t Expire = 7200;
 };
 
 static LockGuarded<ednsstatus_t> s_ednsstatus;
@@ -1098,7 +1095,7 @@ SyncRes::EDNSStatus::EDNSMode SyncRes::getEDNSStatus(const ComboAddress& server)
   auto lock = s_ednsstatus.lock();
   const auto& it = lock->find(server);
   if (it == lock->end()) {
-    return EDNSStatus::UNKNOWN;
+    return EDNSStatus::EDNSOK;
   }
   return it->mode;
 }
@@ -1131,12 +1128,12 @@ uint64_t SyncRes::doEDNSDump(int fd)
   }
   uint64_t count = 0;
 
-  fprintf(fp.get(),"; edns dump follows\n;\n");
+  fprintf(fp.get(),"; edns dump follows\n; ip\tstatus\tttd\n");
   const auto copy = s_ednsstatus.lock()->getMap();
   for (const auto& eds : copy) {
     count++;
     char tmp[26];
-    fprintf(fp.get(), "%s\t%s\t%s\n", eds.address.toString().c_str(), eds.toString().c_str(), timestamp(eds.modeSetAt, tmp, sizeof(tmp)));
+    fprintf(fp.get(), "%s\t%s\t%s\n", eds.address.toString().c_str(), eds.toString().c_str(), timestamp(eds.ttd, tmp, sizeof(tmp)));
   }
   return count;
 }
@@ -1468,31 +1465,30 @@ LWResult::Result SyncRes::asyncresolveWrapper(const ComboAddress& ip, bool ednsM
      the goal is to get as many remotes as possible on the highest level of EDNS support
      The levels are:
 
-     0) UNKNOWN Unknown state
-     1) EDNS: Honors EDNS0
+     1) EDNSOK: Honors EDNS0
      2) EDNSIGNORANT: Ignores EDNS0, gives replies without EDNS0
      3) NOEDNS: Generates FORMERR on EDNS queries
 
-     Everybody starts out assumed to be UNKNOWN.
-     If UNKNOWN, send out EDNS0
+     Everybody starts out assumed to be EDNS.
+     If EDNS, send out EDNS0
         If you FORMERR us, go to NOEDNS, 
         If no EDNS in response, go to EDNSIGNORANT
-     If EDNSOK, send out EDNS0
-        If FORMERR, downgrade to NOEDNS
      If EDNSIGNORANT, keep on including EDNS0, see what happens
-        Same behaviour as UNKNOWN
+        Same behaviour as EDNS
      If NOEDNS, send bare queries
   */
 
-  SyncRes::EDNSStatus::EDNSMode mode;
+  SyncRes::EDNSStatus::EDNSMode mode = EDNSStatus::EDNSOK;
   {
     auto lock = s_ednsstatus.lock();
-    auto ednsstatus = lock->insert(ip).first; // does this include port? YES
-    if (ednsstatus->modeSetAt && ednsstatus->modeSetAt + 3600 < d_now.tv_sec) {
-      auto &ind = lock->get<ComboAddress>();
-      lock->reset(ind, ednsstatus);
+    auto ednsstatus = lock->find(ip); // does this include port? YES
+    if (ednsstatus != lock->end()) {
+      if (ednsstatus->ttd && ednsstatus->ttd < d_now.tv_sec) {
+        lock->erase(ednsstatus);
+      } else {
+        mode = ednsstatus->mode;
+      }
     }
-    mode = ednsstatus->mode;
   }
 
   int EDNSLevel = 0;
@@ -1541,19 +1537,38 @@ LWResult::Result SyncRes::asyncresolveWrapper(const ComboAddress& ip, bool ednsM
       // ret is LWResult::Result::Success
       // ednsstatus in table might be pruned or changed by another request/thread, so do a new lookup/insert
       auto lock = s_ednsstatus.lock();
-      auto ednsstatus = lock->insert(ip).first;
-      auto &ind = lock->get<ComboAddress>();
+      auto ednsstatus = lock->find(ip); // does this include port? YES
+
+      // Read current mode, defaulting to OK
+      mode = EDNSStatus::EDNSOK;
+      if (ednsstatus != lock->end()) {
+        if (ednsstatus->ttd && ednsstatus->ttd  < d_now.tv_sec) {
+          lock->erase(ednsstatus);
+          ednsstatus = lock->end();
+        } else {
+          mode = ednsstatus->mode;
+        }
+      }
 
+      // Determine new mode
       if (res->d_validpacket && !res->d_haveEDNS && res->d_rcode == RCode::FormErr)  {
         mode = EDNSStatus::NOEDNS;
+        if (ednsstatus == lock->end()) {
+          ednsstatus = lock->insert(ip).first;
+        }
+        auto &ind = lock->get<ComboAddress>();
         lock->setMode(ind, ednsstatus, mode, d_now.tv_sec);
         continue;
       }
       else if (!res->d_haveEDNS) {
+        if (ednsstatus == lock->end()) {
+          ednsstatus = lock->insert(ip).first;
+        }
+        auto &ind = lock->get<ComboAddress>();
         lock->setMode(ind, ednsstatus, EDNSStatus::EDNSIGNORANT, d_now.tv_sec);
       }
       else {
-        lock->setMode(ind, ednsstatus, EDNSStatus::EDNSOK, d_now.tv_sec);
+        lock->erase(ip);
       }
     }
 
diff --git a/pdns/syncres.hh b/pdns/syncres.hh
index 161ed219a1..0817107a3f 100644
--- a/pdns/syncres.hh
+++ b/pdns/syncres.hh
@@ -152,6 +152,7 @@ public:
   {
     s_lm = lm;
   }
+
   static uint64_t doEDNSDump(int fd);
   static uint64_t doDumpNSSpeeds(int fd);
   static uint64_t doDumpThrottleMap(int fd);
@@ -215,12 +216,12 @@ public:
   struct EDNSStatus {
     EDNSStatus(const ComboAddress &arg) : address(arg) {}
     ComboAddress address;
-    time_t modeSetAt{0};
-    enum EDNSMode : uint8_t { UNKNOWN = 0, EDNSOK = 1, EDNSIGNORANT = 2, NOEDNS = 3 } mode{UNKNOWN};
+    time_t ttd{0};
+    enum EDNSMode : uint8_t { EDNSOK = 0, EDNSIGNORANT = 1, NOEDNS = 2 } mode{EDNSOK};
 
     std::string toString() const
     {
-      const std::array<std::string,4> modes = { "Unknown", "OK", "Ignorant", "No" };
+      const std::array<std::string,3> modes = { "OK", "Ignorant", "No" };
       unsigned int m = static_cast<unsigned int>(mode);
       if (m >= modes.size()) {
         return "?";