From 472429fbc2e32013e5131e13efdc8b487d5dacb7 Mon Sep 17 00:00:00 2001
From: Robin Geuze <robing@transip.nl>
Date: Fri, 19 Mar 2021 15:28:35 +0100
Subject: [PATCH] Switch priority over to an enum, and implement it in various
 places + one small nitpick fix

---
 pdns/communicator.hh      |  7 ++++---
 pdns/dnsbackend.hh        |  4 +++-
 pdns/dynhandler.cc        |  2 +-
 pdns/packethandler.cc     |  1 +
 pdns/slavecommunicator.cc | 30 +++++++++++++++++-------------
 pdns/ws-auth.cc           |  2 +-
 6 files changed, 27 insertions(+), 19 deletions(-)

diff --git a/pdns/communicator.hh b/pdns/communicator.hh
index 384ae2c669..a8a6a77b01 100644
--- a/pdns/communicator.hh
+++ b/pdns/communicator.hh
@@ -46,7 +46,8 @@ struct SuckRequest
   DNSName domain;
   ComboAddress master;
   bool force;
-  std::pair<uint8_t, uint64_t> priorityAndOrder;
+  enum RequestPriority : uint8_t { PdnsControl, Api, Notify, SerialRefresh, SignaturesRefresh };
+  std::pair<RequestPriority, uint64_t> priorityAndOrder;
   bool operator<(const SuckRequest& b) const
   {
     return tie(domain, master) < tie(b.domain, b.master);
@@ -59,7 +60,7 @@ struct QueueTag{};
 typedef multi_index_container<
   SuckRequest,
   indexed_by<
-    ordered_unique<tag<QueueTag>, member<SuckRequest,std::pair<uint8_t,uint64_t>,&SuckRequest::priorityAndOrder>>,
+    ordered_unique<tag<QueueTag>, member<SuckRequest,std::pair<SuckRequest::RequestPriority,uint64_t>,&SuckRequest::priorityAndOrder>>,
     ordered_unique<tag<IDTag>, identity<SuckRequest> >
   >
 > UniQueue;
@@ -166,7 +167,7 @@ public:
   
   void drillHole(const DNSName &domain, const string &ip);
   bool justNotified(const DNSName &domain, const string &ip);
-  void addSuckRequest(const DNSName &domain, const ComboAddress& master, bool force=false, uint8_t priority = 0);
+  void addSuckRequest(const DNSName &domain, const ComboAddress& master, SuckRequest::RequestPriority, bool force=false);
   void addSlaveCheckRequest(const DomainInfo& di, const ComboAddress& remote);
   void addTrySuperMasterRequest(const DNSPacket& p);
   void notify(const DNSName &domain, const string &ip);
diff --git a/pdns/dnsbackend.hh b/pdns/dnsbackend.hh
index b339b229e0..f200a0526a 100644
--- a/pdns/dnsbackend.hh
+++ b/pdns/dnsbackend.hh
@@ -46,7 +46,7 @@ class DNSPacket;
 class DNSBackend;  
 struct DomainInfo
 {
-  DomainInfo() : last_check(0), backend(nullptr), id(0), notified_serial(0), serial(0), kind(DomainInfo::Native) {}
+  DomainInfo() : last_check(0), backend(nullptr), id(0), notified_serial(0), receivedNotify(false), serial(0), kind(DomainInfo::Native) {}
 
   DNSName zone;
   time_t last_check;
@@ -57,6 +57,8 @@ struct DomainInfo
   uint32_t id;
   uint32_t notified_serial;
 
+  bool receivedNotify;
+
   uint32_t serial;
   enum DomainKind : uint8_t { Master, Slave, Native } kind;
   
diff --git a/pdns/dynhandler.cc b/pdns/dynhandler.cc
index 8926738f56..3c05f13072 100644
--- a/pdns/dynhandler.cc
+++ b/pdns/dynhandler.cc
@@ -278,7 +278,7 @@ string DLNotifyRetrieveHandler(const vector<string>&parts, Utility::pid_t ppid)
 
   shuffle(di.masters.begin(), di.masters.end(), pdns::dns_random_engine());
   const auto& master = di.masters.front();
-  Communicator.addSuckRequest(domain, master, override_master);
+  Communicator.addSuckRequest(domain, master, SuckRequest::PdnsControl, override_master);
   g_log<<Logger::Warning<<"Retrieval request for domain '"<<domain<<"' from master '"<<master<<"' received from operator"<<endl;
   return "Added retrieval request for '"+domain.toLogString()+"' from master "+master.toLogString();
 }
diff --git a/pdns/packethandler.cc b/pdns/packethandler.cc
index fb3ed7a883..939a28b133 100644
--- a/pdns/packethandler.cc
+++ b/pdns/packethandler.cc
@@ -1030,6 +1030,7 @@ int PacketHandler::processNotify(const DNSPacket& p)
 
   if(::arg().mustDo("slave")) {
     g_log<<Logger::Notice<<"Received NOTIFY for "<<p.qdomain<<" from "<<p.getRemote()<<" - queueing check"<<endl;
+    di.receivedNotify = true;
     Communicator.addSlaveCheckRequest(di, p.d_remote);
   }
   return 0;
diff --git a/pdns/slavecommunicator.cc b/pdns/slavecommunicator.cc
index 9197c649cd..635695e5b6 100644
--- a/pdns/slavecommunicator.cc
+++ b/pdns/slavecommunicator.cc
@@ -48,7 +48,7 @@
 
 #include "ixfr.hh"
 
-void CommunicatorClass::addSuckRequest(const DNSName &domain, const ComboAddress& master, bool force, uint8_t priority)
+void CommunicatorClass::addSuckRequest(const DNSName &domain, const ComboAddress& master, SuckRequest::RequestPriority priority, bool force)
 {
   std::lock_guard<std::mutex> l(d_lock);
   SuckRequest sr;
@@ -63,14 +63,7 @@ void CommunicatorClass::addSuckRequest(const DNSName &domain, const ComboAddress
   if(res.second) {
     d_suck_sem.post();
   } else {
-    // Domain is already in there, we should check the priority and whether its forced
-    domains_by_name_t& nameindex=boost::multi_index::get<IDTag>(d_suckdomains);
-    auto iter = nameindex.find(sr);
-    if (iter == nameindex.end()) {
-      // bit weird, but ok
-      return;
-    }
-    nameindex.modify(iter, [priorityAndOrder = sr.priorityAndOrder] (SuckRequest& so) {
+    d_suckdomains.modify(res.first, [priorityAndOrder = sr.priorityAndOrder] (SuckRequest& so) {
       if (priorityAndOrder.first < so.priorityAndOrder.first) {
         so.priorityAndOrder = priorityAndOrder;
       }
@@ -1003,6 +996,12 @@ void CommunicatorClass::slaveRefresh(PacketHandler *P)
           }
         }
       }
+
+      SuckRequest::RequestPriority prio = SuckRequest::SignaturesRefresh;
+      if (di.receivedNotify) {
+        prio = SuckRequest::Notify;
+      }
+
       if(! maxInception && ! ssr.d_freshness[di.id].theirInception) {
         g_log<<Logger::Info<<"Domain '"<< di.zone << "' is fresh (no DNSSEC), serial is " << ourserial << " (checked master " << remote.toStringWithPortExcept(53) << ")" << endl;
         di.backend->setFresh(di.id);
@@ -1017,25 +1016,30 @@ void CommunicatorClass::slaveRefresh(PacketHandler *P)
       }
       else if(maxInception && ! ssr.d_freshness[di.id].theirInception ) {
         g_log<<Logger::Notice<<"Domain '"<< di.zone << "' is stale, master " << remote.toStringWithPortExcept(53) << " is no longer signed and all signatures have expired, serial is " << ourserial << endl;
-        addSuckRequest(di.zone, remote);
+        addSuckRequest(di.zone, remote, prio);
       }
       else if(dk.doesDNSSEC() && ! maxInception && ssr.d_freshness[di.id].theirInception) {
         g_log<<Logger::Notice<<"Domain '"<< di.zone << "' is stale, master " << remote.toStringWithPortExcept(53) << " has signed, serial is " << ourserial << endl;
-        addSuckRequest(di.zone, remote);
+        addSuckRequest(di.zone, remote, prio);
       }
       else {
         g_log<<Logger::Notice<<"Domain '"<< di.zone << "' is fresh, but RRSIGs differ on master" << remote.toStringWithPortExcept(53)<<", so DNSSEC is stale, serial is " << ourserial << endl;
-        addSuckRequest(di.zone, remote);
+        addSuckRequest(di.zone, remote, prio);
       }
     }
     else {
+      SuckRequest::RequestPriority prio = SuckRequest::SerialRefresh;
+      if (di.receivedNotify) {
+        prio = SuckRequest::Notify;
+      }
+
       if (hasSOA) {
         g_log<<Logger::Notice<<"Domain '"<< di.zone << "' is stale, master " << remote.toStringWithPortExcept(53) << " serial " << theirserial << ", our serial " << ourserial << endl;
       }
       else {
         g_log<<Logger::Notice<<"Domain '"<< di.zone << "' is empty, master " << remote.toStringWithPortExcept(53) << " serial " << theirserial << endl;
       }
-      addSuckRequest(di.zone, remote);
+      addSuckRequest(di.zone, remote, prio);
     }
   }
 }
diff --git a/pdns/ws-auth.cc b/pdns/ws-auth.cc
index a17e05814f..b1f8140ef7 100644
--- a/pdns/ws-auth.cc
+++ b/pdns/ws-auth.cc
@@ -1876,7 +1876,7 @@ static void apiServerZoneAxfrRetrieve(HttpRequest* req, HttpResponse* resp) {
     throw ApiException("Domain '"+zonename.toString()+"' is not a slave domain (or has no master defined)");
 
   shuffle(di.masters.begin(), di.masters.end(), pdns::dns_random_engine());
-  Communicator.addSuckRequest(zonename, di.masters.front());
+  Communicator.addSuckRequest(zonename, di.masters.front(), SuckRequest::Api);
   resp->setSuccessResult("Added retrieval request for '"+zonename.toString()+"' from master "+di.masters.front().toLogString());
 }
 
-- 
2.47.2