From: Otto Moerbeek <otto.moerbeek@open-xchange.com>
Date: Fri, 3 Mar 2023 08:44:06 +0000 (+0100)
Subject: Fix serve-stale loop logic.
X-Git-Tag: dnsdist-1.8.0-rc2~4^2
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=refs%2Fpull%2F12610%2Fhead;p=thirdparty%2Fpdns.git

Fix serve-stale loop logic.

There are several issues here: we do not want to retry on an exception
ever as an exception indicates a final failure. e.g. a resolution
timeout for the whole query. Recursing in that state will generate
an exception storm. Individual timeouts of contacting a nameserver
do not generate an exception.  Also, we do not want to recurse if
the cache lookup for stale records did not produce anything in the
second iteration, we know it's probably going to be fatal and it
requires (portentially) a lot of work to find out.

This solves CPU pegging seen.
---

diff --git a/pdns/recursordist/syncres.cc b/pdns/recursordist/syncres.cc
index 6431cfa40d..0c24565a34 100644
--- a/pdns/recursordist/syncres.cc
+++ b/pdns/recursordist/syncres.cc
@@ -1815,200 +1815,194 @@ int SyncRes::doResolveNoQNameMinimization(const DNSName& qname, const QType qtyp
   const int iterations = !d_refresh && MemRecursorCache::s_maxServedStaleExtensions > 0 ? 2 : 1;
   for (int loop = 0; loop < iterations; loop++) {
 
-    if (loop == 1) {
-      d_serveStale = true;
-    }
+    d_serveStale = loop == 1;
 
-    // When we're not on the last iteration, a timeout is not fatal
-    const bool exceptionOnTimeout = loop == iterations - 1;
+    // This is a difficult way of expressing "this is a normal query", i.e. not getRootNS.
+    if (!(d_updatingRootNS && qtype.getCode() == QType::NS && qname.isRoot())) {
+      DNSName authname(qname);
+      const auto iter = getBestAuthZone(&authname);
 
-    try {
-      // This is a difficult way of expressing "this is a normal query", i.e. not getRootNS.
-      if (!(d_updatingRootNS && qtype.getCode() == QType::NS && qname.isRoot())) {
-        DNSName authname(qname);
-        const auto iter = getBestAuthZone(&authname);
-
-        if (d_cacheonly) {
-          if (iter != t_sstorage.domainmap->end()) {
-            if (iter->second.isAuth()) {
-              LOG(prefix << qname << ": Cache only lookup for '" << qname << "|" << qtype << "', in auth zone" << endl);
-              ret.clear();
-              d_wasOutOfBand = doOOBResolve(qname, qtype, ret, depth, prefix, res);
-              if (fromCache != nullptr) {
-                *fromCache = d_wasOutOfBand;
-              }
-              return res;
+      if (d_cacheonly) {
+        if (iter != t_sstorage.domainmap->end()) {
+          if (iter->second.isAuth()) {
+            LOG(prefix << qname << ": Cache only lookup for '" << qname << "|" << qtype << "', in auth zone" << endl);
+            ret.clear();
+            d_wasOutOfBand = doOOBResolve(qname, qtype, ret, depth, prefix, res);
+            if (fromCache != nullptr) {
+              *fromCache = d_wasOutOfBand;
             }
+            return res;
           }
         }
+      }
 
-        bool wasForwardedOrAuthZone = false;
-        bool wasAuthZone = false;
-        bool wasForwardRecurse = false;
+      bool wasForwardedOrAuthZone = false;
+      bool wasAuthZone = false;
+      bool wasForwardRecurse = false;
 
-        if (iter != t_sstorage.domainmap->end()) {
-          wasForwardedOrAuthZone = true;
+      if (iter != t_sstorage.domainmap->end()) {
+        wasForwardedOrAuthZone = true;
 
-          if (iter->second.isAuth()) {
-            wasAuthZone = true;
-          }
-          else if (iter->second.shouldRecurse()) {
-            wasForwardRecurse = true;
-          }
+        if (iter->second.isAuth()) {
+          wasAuthZone = true;
         }
+        else if (iter->second.shouldRecurse()) {
+          wasForwardRecurse = true;
+        }
+      }
 
-        /* When we are looking for a DS, we want to the non-CNAME cache check first
-           because we can actually have a DS (from the parent zone) AND a CNAME (from
-           the child zone), and what we really want is the DS */
-        if (qtype != QType::DS && doCNAMECacheCheck(qname, qtype, ret, depth, prefix, res, context, wasAuthZone, wasForwardRecurse)) { // will reroute us if needed
-          d_wasOutOfBand = wasAuthZone;
-          // Here we have an issue. If we were prevented from going out to the network (cache-only was set, possibly because we
-          // are in QM Step0) we might have a CNAME but not the corresponding target.
-          // It means that we will sometimes go to the next steps when we are in fact done, but that's fine since
-          // we will get the records from the cache, resulting in a small overhead.
-          // This might be a real problem if we had a RPZ hit, though, because we do not want the processing to continue, since
-          // RPZ rules will not be evaluated anymore (we already matched).
-          const bool stoppedByPolicyHit = d_appliedPolicy.wasHit();
-
-          if (fromCache && (!d_cacheonly || stoppedByPolicyHit)) {
-            *fromCache = true;
-          }
-          /* Apply Post filtering policies */
-
-          if (d_wantsRPZ && !stoppedByPolicyHit) {
-            auto luaLocal = g_luaconfs.getLocal();
-            if (luaLocal->dfe.getPostPolicy(ret, d_discardedPolicies, d_appliedPolicy)) {
-              mergePolicyTags(d_policyTags, d_appliedPolicy.getTags());
-              bool done = false;
-              handlePolicyHit(prefix, qname, qtype, ret, done, res, depth);
-              if (done && fromCache) {
-                *fromCache = true;
-              }
+      /* When we are looking for a DS, we want to the non-CNAME cache check first
+         because we can actually have a DS (from the parent zone) AND a CNAME (from
+         the child zone), and what we really want is the DS */
+      if (qtype != QType::DS && doCNAMECacheCheck(qname, qtype, ret, depth, prefix, res, context, wasAuthZone, wasForwardRecurse)) { // will reroute us if needed
+        d_wasOutOfBand = wasAuthZone;
+        // Here we have an issue. If we were prevented from going out to the network (cache-only was set, possibly because we
+        // are in QM Step0) we might have a CNAME but not the corresponding target.
+        // It means that we will sometimes go to the next steps when we are in fact done, but that's fine since
+        // we will get the records from the cache, resulting in a small overhead.
+        // This might be a real problem if we had a RPZ hit, though, because we do not want the processing to continue, since
+        // RPZ rules will not be evaluated anymore (we already matched).
+        const bool stoppedByPolicyHit = d_appliedPolicy.wasHit();
+
+        if (fromCache && (!d_cacheonly || stoppedByPolicyHit)) {
+          *fromCache = true;
+        }
+        /* Apply Post filtering policies */
+
+        if (d_wantsRPZ && !stoppedByPolicyHit) {
+          auto luaLocal = g_luaconfs.getLocal();
+          if (luaLocal->dfe.getPostPolicy(ret, d_discardedPolicies, d_appliedPolicy)) {
+            mergePolicyTags(d_policyTags, d_appliedPolicy.getTags());
+            bool done = false;
+            handlePolicyHit(prefix, qname, qtype, ret, done, res, depth);
+            if (done && fromCache) {
+              *fromCache = true;
             }
           }
-          return res;
         }
+        return res;
+      }
 
-        if (doCacheCheck(qname, authname, wasForwardedOrAuthZone, wasAuthZone, wasForwardRecurse, qtype, ret, depth, prefix, res, context)) {
-          // we done
-          d_wasOutOfBand = wasAuthZone;
-          if (fromCache) {
-            *fromCache = true;
-          }
+      if (doCacheCheck(qname, authname, wasForwardedOrAuthZone, wasAuthZone, wasForwardRecurse, qtype, ret, depth, prefix, res, context)) {
+        // we done
+        d_wasOutOfBand = wasAuthZone;
+        if (fromCache) {
+          *fromCache = true;
+        }
 
-          if (d_wantsRPZ && !d_appliedPolicy.wasHit()) {
-            auto luaLocal = g_luaconfs.getLocal();
-            if (luaLocal->dfe.getPostPolicy(ret, d_discardedPolicies, d_appliedPolicy)) {
-              mergePolicyTags(d_policyTags, d_appliedPolicy.getTags());
-              bool done = false;
-              handlePolicyHit(prefix, qname, qtype, ret, done, res, depth);
-            }
+        if (d_wantsRPZ && !d_appliedPolicy.wasHit()) {
+          auto luaLocal = g_luaconfs.getLocal();
+          if (luaLocal->dfe.getPostPolicy(ret, d_discardedPolicies, d_appliedPolicy)) {
+            mergePolicyTags(d_policyTags, d_appliedPolicy.getTags());
+            bool done = false;
+            handlePolicyHit(prefix, qname, qtype, ret, done, res, depth);
           }
-
-          return res;
         }
 
-        /* if we have not found a cached DS (or denial of), now is the time to look for a CNAME */
-        if (qtype == QType::DS && doCNAMECacheCheck(qname, qtype, ret, depth, prefix, res, context, wasAuthZone, wasForwardRecurse)) { // will reroute us if needed
-          d_wasOutOfBand = wasAuthZone;
-          // Here we have an issue. If we were prevented from going out to the network (cache-only was set, possibly because we
-          // are in QM Step0) we might have a CNAME but not the corresponding target.
-          // It means that we will sometimes go to the next steps when we are in fact done, but that's fine since
-          // we will get the records from the cache, resulting in a small overhead.
-          // This might be a real problem if we had a RPZ hit, though, because we do not want the processing to continue, since
-          // RPZ rules will not be evaluated anymore (we already matched).
-          const bool stoppedByPolicyHit = d_appliedPolicy.wasHit();
-
-          if (fromCache && (!d_cacheonly || stoppedByPolicyHit)) {
-            *fromCache = true;
-          }
-          /* Apply Post filtering policies */
-
-          if (d_wantsRPZ && !stoppedByPolicyHit) {
-            auto luaLocal = g_luaconfs.getLocal();
-            if (luaLocal->dfe.getPostPolicy(ret, d_discardedPolicies, d_appliedPolicy)) {
-              mergePolicyTags(d_policyTags, d_appliedPolicy.getTags());
-              bool done = false;
-              handlePolicyHit(prefix, qname, qtype, ret, done, res, depth);
-              if (done && fromCache) {
-                *fromCache = true;
-              }
+        return res;
+      }
+
+      /* if we have not found a cached DS (or denial of), now is the time to look for a CNAME */
+      if (qtype == QType::DS && doCNAMECacheCheck(qname, qtype, ret, depth, prefix, res, context, wasAuthZone, wasForwardRecurse)) { // will reroute us if needed
+        d_wasOutOfBand = wasAuthZone;
+        // Here we have an issue. If we were prevented from going out to the network (cache-only was set, possibly because we
+        // are in QM Step0) we might have a CNAME but not the corresponding target.
+        // It means that we will sometimes go to the next steps when we are in fact done, but that's fine since
+        // we will get the records from the cache, resulting in a small overhead.
+        // This might be a real problem if we had a RPZ hit, though, because we do not want the processing to continue, since
+        // RPZ rules will not be evaluated anymore (we already matched).
+        const bool stoppedByPolicyHit = d_appliedPolicy.wasHit();
+
+        if (fromCache && (!d_cacheonly || stoppedByPolicyHit)) {
+          *fromCache = true;
+        }
+        /* Apply Post filtering policies */
+
+        if (d_wantsRPZ && !stoppedByPolicyHit) {
+          auto luaLocal = g_luaconfs.getLocal();
+          if (luaLocal->dfe.getPostPolicy(ret, d_discardedPolicies, d_appliedPolicy)) {
+            mergePolicyTags(d_policyTags, d_appliedPolicy.getTags());
+            bool done = false;
+            handlePolicyHit(prefix, qname, qtype, ret, done, res, depth);
+            if (done && fromCache) {
+              *fromCache = true;
             }
           }
-
-          return res;
         }
-      }
 
-      if (d_cacheonly) {
-        return 0;
+        return res;
       }
+    }
 
-      LOG(prefix << qname << ": No cache hit for '" << qname << "|" << qtype << "', trying to find an appropriate NS record" << endl);
+    if (d_cacheonly) {
+      return 0;
+    }
 
-      DNSName subdomain(qname);
-      if (qtype == QType::DS)
-        subdomain.chopOff();
+    // When trying to serve-stale, we also only look at the cache. Don't look at d_serveStale, it
+    // might be changed by recursive calls (this should be fixed in a better way!).
+    if (loop == 1) {
+      return res;
+    }
 
-      NsSet nsset;
-      bool flawedNSSet = false;
+    LOG(prefix << qname << ": No cache hit for '" << qname << "|" << qtype << "', trying to find an appropriate NS record" << endl);
 
-      // the two retries allow getBestNSNamesFromCache&co to reprime the root
-      // hints, in case they ever go missing
-      for (int tries = 0; tries < 2 && nsset.empty(); ++tries) {
-        subdomain = getBestNSNamesFromCache(subdomain, qtype, nsset, &flawedNSSet, depth, prefix, beenthere); //  pass beenthere to both occasions
-      }
-
-      res = doResolveAt(nsset, subdomain, flawedNSSet, qname, qtype, ret, depth, prefix, beenthere, context, stopAtDelegation, nullptr);
-
-      if (res == -1 && s_save_parent_ns_set) {
-        // It did not work out, lets check if we have a saved parent NS set
-        map<DNSName, vector<ComboAddress>> fallBack;
-        {
-          auto lock = s_savedParentNSSet.lock();
-          auto domainData = lock->find(subdomain);
-          if (domainData != lock->end() && domainData->d_nsAddresses.size() > 0) {
-            nsset.clear();
-            // Build the nsset arg and fallBack data for the fallback doResolveAt() attempt
-            // Take a copy to be able to release the lock, NsSet is actually a map, go figure
-            for (const auto& ns : domainData->d_nsAddresses) {
-              nsset.emplace(ns.first, pair(std::vector<ComboAddress>(), false));
-              fallBack.emplace(ns.first, ns.second);
-            }
-          }
-        }
-        if (fallBack.size() > 0) {
-          LOG(prefix << qname << ": Failure, but we have a saved parent NS set, trying that one" << endl);
-          res = doResolveAt(nsset, subdomain, flawedNSSet, qname, qtype, ret, depth, prefix, beenthere, context, stopAtDelegation, &fallBack);
-          if (res == 0) {
-            // It did work out
-            s_savedParentNSSet.lock()->inc(subdomain);
+    DNSName subdomain(qname);
+    if (qtype == QType::DS)
+      subdomain.chopOff();
+
+    NsSet nsset;
+    bool flawedNSSet = false;
+
+    // the two retries allow getBestNSNamesFromCache&co to reprime the root
+    // hints, in case they ever go missing
+    for (int tries = 0; tries < 2 && nsset.empty(); ++tries) {
+      subdomain = getBestNSNamesFromCache(subdomain, qtype, nsset, &flawedNSSet, depth, prefix, beenthere); //  pass beenthere to both occasions
+    }
+
+    res = doResolveAt(nsset, subdomain, flawedNSSet, qname, qtype, ret, depth, prefix, beenthere, context, stopAtDelegation, nullptr);
+
+    if (res == -1 && s_save_parent_ns_set) {
+      // It did not work out, lets check if we have a saved parent NS set
+      map<DNSName, vector<ComboAddress>> fallBack;
+      {
+        auto lock = s_savedParentNSSet.lock();
+        auto domainData = lock->find(subdomain);
+        if (domainData != lock->end() && domainData->d_nsAddresses.size() > 0) {
+          nsset.clear();
+          // Build the nsset arg and fallBack data for the fallback doResolveAt() attempt
+          // Take a copy to be able to release the lock, NsSet is actually a map, go figure
+          for (const auto& ns : domainData->d_nsAddresses) {
+            nsset.emplace(ns.first, pair(std::vector<ComboAddress>(), false));
+            fallBack.emplace(ns.first, ns.second);
           }
         }
       }
-      /* Apply Post filtering policies */
-      if (d_wantsRPZ && !d_appliedPolicy.wasHit()) {
-        auto luaLocal = g_luaconfs.getLocal();
-        if (luaLocal->dfe.getPostPolicy(ret, d_discardedPolicies, d_appliedPolicy)) {
-          mergePolicyTags(d_policyTags, d_appliedPolicy.getTags());
-          bool done = false;
-          handlePolicyHit(prefix, qname, qtype, ret, done, res, depth);
+      if (fallBack.size() > 0) {
+        LOG(prefix << qname << ": Failure, but we have a saved parent NS set, trying that one" << endl);
+        res = doResolveAt(nsset, subdomain, flawedNSSet, qname, qtype, ret, depth, prefix, beenthere, context, stopAtDelegation, &fallBack);
+        if (res == 0) {
+          // It did work out
+          s_savedParentNSSet.lock()->inc(subdomain);
         }
       }
-
-      if (!res) {
-        return 0;
+    }
+    /* Apply Post filtering policies */
+    if (d_wantsRPZ && !d_appliedPolicy.wasHit()) {
+      auto luaLocal = g_luaconfs.getLocal();
+      if (luaLocal->dfe.getPostPolicy(ret, d_discardedPolicies, d_appliedPolicy)) {
+        mergePolicyTags(d_policyTags, d_appliedPolicy.getTags());
+        bool done = false;
+        handlePolicyHit(prefix, qname, qtype, ret, done, res, depth);
       }
+    }
 
-      LOG(prefix << qname << ": Failed (res=" << res << ")" << endl);
-      if (res >= 0) {
-        break;
-      }
+    if (!res) {
+      return 0;
     }
-    catch (const ImmediateServFailException&) {
-      if (exceptionOnTimeout) {
-        throw;
-      }
+
+    LOG(prefix << qname << ": Failed (res=" << res << ")" << endl);
+    if (res >= 0) {
+      break;
     }
   }
   return res < 0 ? RCode::ServFail : res;
diff --git a/pdns/recursordist/test-syncres_cc10.cc b/pdns/recursordist/test-syncres_cc10.cc
index ac4f0c42fb..1f0ce57b8e 100644
--- a/pdns/recursordist/test-syncres_cc10.cc
+++ b/pdns/recursordist/test-syncres_cc10.cc
@@ -1563,7 +1563,7 @@ BOOST_AUTO_TEST_CASE(test_servestale_cname_to_nxdomain)
   BOOST_CHECK(ret[1].d_type == QType::A);
   BOOST_CHECK_EQUAL(ret[0].d_name, target);
   BOOST_CHECK_EQUAL(ret[1].d_name, DNSName("cname.powerdns.com"));
-  BOOST_CHECK_EQUAL(downCount, 4U);
+  BOOST_CHECK_EQUAL(downCount, 8U);
   BOOST_CHECK_EQUAL(lookupCount, 2U);
 
   // Again, no lookup as the record is marked stale
@@ -1575,7 +1575,7 @@ BOOST_AUTO_TEST_CASE(test_servestale_cname_to_nxdomain)
   BOOST_CHECK(ret[1].d_type == QType::A);
   BOOST_CHECK_EQUAL(ret[0].d_name, target);
   BOOST_CHECK_EQUAL(ret[1].d_name, DNSName("cname.powerdns.com"));
-  BOOST_CHECK_EQUAL(downCount, 4U);
+  BOOST_CHECK_EQUAL(downCount, 8U);
   BOOST_CHECK_EQUAL(lookupCount, 2U);
 
   // Again, no lookup as the record is marked stale but as the TTL has passed a task should have been pushed
@@ -1588,7 +1588,7 @@ BOOST_AUTO_TEST_CASE(test_servestale_cname_to_nxdomain)
   BOOST_CHECK(ret[1].d_type == QType::A);
   BOOST_CHECK_EQUAL(ret[0].d_name, target);
   BOOST_CHECK_EQUAL(ret[1].d_name, DNSName("cname.powerdns.com"));
-  BOOST_CHECK_EQUAL(downCount, 4U);
+  BOOST_CHECK_EQUAL(downCount, 8U);
   BOOST_CHECK_EQUAL(lookupCount, 2U);
 
   BOOST_REQUIRE_EQUAL(getTaskSize(), 2U);
@@ -1611,7 +1611,7 @@ BOOST_AUTO_TEST_CASE(test_servestale_cname_to_nxdomain)
   BOOST_REQUIRE_EQUAL(ret.size(), 1U);
   BOOST_CHECK(ret[0].d_type == QType::SOA);
   BOOST_CHECK_EQUAL(ret[0].d_name, auth);
-  BOOST_CHECK_EQUAL(downCount, 4U);
+  BOOST_CHECK_EQUAL(downCount, 8U);
   BOOST_CHECK_EQUAL(lookupCount, 3U);
 
   // And again, result should come from cache
@@ -1622,7 +1622,7 @@ BOOST_AUTO_TEST_CASE(test_servestale_cname_to_nxdomain)
   BOOST_REQUIRE_EQUAL(ret.size(), 1U);
   BOOST_CHECK(ret[0].d_type == QType::SOA);
   BOOST_CHECK_EQUAL(ret[0].d_name, auth);
-  BOOST_CHECK_EQUAL(downCount, 4U);
+  BOOST_CHECK_EQUAL(downCount, 8U);
   BOOST_CHECK_EQUAL(lookupCount, 3U);
 }