Refactor: get rid of an unneccesary loop

diff --git a/pdns/recursordist/pdns_recursor.cc b/pdns/recursordist/pdns_recursor.cc
index 0be8a735e1d04e87296b8e0e3e4228290f827965..35bd62144b73182b8c04413a3a05cb7c9b3661af 100644 (file)
--- a/pdns/recursordist/pdns_recursor.cc
+++ b/pdns/recursordist/pdns_recursor.cc
@@ -372,6 +372,8 @@ LWResult::Result arecvfrom(PacketBuffer& packet, int /* flags */, const ComboAdd
 
     len = packet.size();
 
+    // In ecs hardening mode, we consider a missing ECS in the reply as a case for retrying without ECS
+    // The actual logic to do that is in Syncres::doResolveAtThisIP()
     if (g_ECSHardening && pident->ecsSubnet && !*pident->ecsReceived) {
       t_Counters.at(rec::Counter::ecsMissingCount)++;
       return LWResult::Result::ECSMissing;
@@ -3055,9 +3057,7 @@ static void handleUDPServerResponse(int fileDesc, FDMultiplexer::funcparam_t& va
   if (!pident->domain.empty()) {
     auto iter = g_multiTasker->getWaiters().find(pident);
     if (iter != g_multiTasker->getWaiters().end()) {
-      if (g_ECSHardening) {
-        iter->key->ecsReceived = iter->key->ecsSubnet && checkIncomingECSSource(packet, *iter->key->ecsSubnet);
-      }
+      iter->key->ecsReceived = iter->key->ecsSubnet && checkIncomingECSSource(packet, *iter->key->ecsSubnet);
       doResends(iter, pident, packet, iter->key->ecsReceived);
     }
   }

diff --git a/pdns/recursordist/syncres.cc b/pdns/recursordist/syncres.cc
index c7cc489591769927791b85aeda182f28a6272932..f10ba5b283a4eccc871b32c02a4e6e5a0e88d737 100644 (file)
--- a/pdns/recursordist/syncres.cc
+++ b/pdns/recursordist/syncres.cc
@@ -5424,45 +5424,47 @@ void SyncRes::incTimeoutStats(const ComboAddress& remoteIP)
   }
 }
 
-bool SyncRes::doResolveAtThisIP(const std::string& prefix, const DNSName& qname, const QType qtype, LWResult& lwr, boost::optional<Netmask>& ednsmask, const DNSName& auth, bool const sendRDQuery, const bool wasForwarded, const DNSName& nsName, const ComboAddress& remoteIP, bool doTCP, bool doDoT, bool& truncated, bool& spoofed, boost::optional<EDNSExtendedError>& extendedError, bool dontThrottle)
+void SyncRes::checkTotalTime(const DNSName& qname, QType qtype, boost::optional<EDNSExtendedError>& extendedError) const
 {
-  bool chained = false;
-  LWResult::Result resolveret = LWResult::Result::Success;
-
   if (s_maxtotusec != 0 && d_totUsec > s_maxtotusec) {
     if (s_addExtendedResolutionDNSErrors) {
       extendedError = EDNSExtendedError{static_cast<uint16_t>(EDNSExtendedError::code::NoReachableAuthority), "Timeout waiting for answer(s)"};
     }
     throw ImmediateServFailException("Too much time waiting for " + qname.toLogString() + "|" + qtype.toString() + ", timeouts: " + std::to_string(d_timeouts) + ", throttles: " + std::to_string(d_throttledqueries) + ", queries: " + std::to_string(d_outqueries) + ", " + std::to_string(d_totUsec / 1000) + " ms");
   }
+}
+
+bool SyncRes::doResolveAtThisIP(const std::string& prefix, const DNSName& qname, const QType qtype, LWResult& lwr, boost::optional<Netmask>& ednsmask, const DNSName& auth, bool const sendRDQuery, const bool wasForwarded, const DNSName& nsName, const ComboAddress& remoteIP, bool doTCP, bool doDoT, bool& truncated, bool& spoofed, boost::optional<EDNSExtendedError>& extendedError, bool dontThrottle)
+{
+  checkTotalTime(qname, qtype, extendedError);
 
+  bool chained = false;
+  LWResult::Result resolveret = LWResult::Result::Success;
   int preOutQueryRet = RCode::NoError;
+
   if (d_pdl && d_pdl->preoutquery(remoteIP, d_requestor, qname, qtype, doTCP, lwr.d_records, preOutQueryRet, d_eventTrace, timeval{0, 0})) {
     LOG(prefix << qname << ": Query handled by Lua" << endl);
   }
   else {
-    bool sendECSIfRelevant = true;
-    for (int count = 0; count < 2; ++count) {
-      ednsmask = sendECSIfRelevant ? getEDNSSubnetMask(qname, remoteIP) : boost::none;
-      if (ednsmask) {
-        LOG(prefix << qname << ": Adding EDNS Client Subnet Mask " << ednsmask->toString() << " to query" << endl);
-        s_ecsqueries++;
-      }
+    ednsmask = getEDNSSubnetMask(qname, remoteIP);
+    if (ednsmask) {
+      LOG(prefix << qname << ": Adding EDNS Client Subnet Mask " << ednsmask->toString() << " to query" << endl);
+      s_ecsqueries++;
+    }
+    auto match = d_eventTrace.add(RecEventTrace::AuthRequest, qname.toLogString() + '/' + qtype.toString(), true, 0);
+    updateQueryCounts(prefix, qname, remoteIP, doTCP, doDoT);
+    resolveret = asyncresolveWrapper(remoteIP, d_doDNSSEC, qname, auth, qtype.getCode(),
+                                     doTCP, sendRDQuery, &d_now, ednsmask, &lwr, &chained, nsName); // <- we go out on the wire!
+    d_eventTrace.add(RecEventTrace::AuthRequest, static_cast<int64_t>(lwr.d_rcode), false, match);
+    ednsStats(ednsmask, qname, prefix);
+    if (resolveret == LWResult::Result::ECSMissing) {
+      ednsmask = boost::none;
+      LOG(prefix << qname << ": Answer has no ECS, trying again without EDNS Client Subnet Mask" << endl);
       updateQueryCounts(prefix, qname, remoteIP, doTCP, doDoT);
-      auto match = d_eventTrace.add(RecEventTrace::AuthRequest, qname.toLogString() + '/' + qtype.toString(), true, 0);
+      match = d_eventTrace.add(RecEventTrace::AuthRequest, qname.toLogString() + '/' + qtype.toString(), true, 0);
       resolveret = asyncresolveWrapper(remoteIP, d_doDNSSEC, qname, auth, qtype.getCode(),
                                        doTCP, sendRDQuery, &d_now, ednsmask, &lwr, &chained, nsName); // <- we go out on the wire!
       d_eventTrace.add(RecEventTrace::AuthRequest, static_cast<int64_t>(lwr.d_rcode), false, match);
-      ednsStats(ednsmask, qname, prefix);
-      if (resolveret == LWResult::Result::ECSMissing) {
-        if (sendECSIfRelevant) {
-          sendECSIfRelevant = false;
-          LOG(prefix << qname << ": Answer has no ECS, trying again without EDNS Client Subnet Mask" << endl);
-          continue;
-        }
-        assert(0); // should not happen
-      }
-      break; // when no ECS shenanigans happened, only one pass through the loop
     }
   }
 

diff --git a/pdns/recursordist/syncres.hh b/pdns/recursordist/syncres.hh
index bff0f855382fe540959ae9a77b4979c8cea01cf4..bba70b522e3402efae49dfc6feb79e5f493c6984 100644 (file)
--- a/pdns/recursordist/syncres.hh
+++ b/pdns/recursordist/syncres.hh
@@ -645,6 +645,7 @@ private:
                   std::map<DNSName, std::vector<ComboAddress>>* fallback);
   void ednsStats(boost::optional<Netmask>& ednsmask, const DNSName& qname, const string& prefix);
   void incTimeoutStats(const ComboAddress& remoteIP);
+  void checkTotalTime(const DNSName& qname, QType qtype, boost::optional<EDNSExtendedError>& extendedError) const;
   bool doResolveAtThisIP(const std::string& prefix, const DNSName& qname, QType qtype, LWResult& lwr, boost::optional<Netmask>& ednsmask, const DNSName& auth, bool sendRDQuery, bool wasForwarded, const DNSName& nsName, const ComboAddress& remoteIP, bool doTCP, bool doDoT, bool& truncated, bool& spoofed, boost::optional<EDNSExtendedError>& extendedError, bool dontThrottle = false);
   bool processAnswer(unsigned int depth, const string& prefix, LWResult& lwr, const DNSName& qname, QType qtype, DNSName& auth, bool wasForwarded, const boost::optional<Netmask>& ednsmask, bool sendRDQuery, NsSet& nameservers, std::vector<DNSRecord>& ret, const DNSFilterEngine& dfe, bool* gotNewServers, int* rcode, vState& state, const ComboAddress& remoteIP);