Add one more unit test and use the proper counter.

nretrieveAddressesForNS is only incremented on empty results, so the clearing did not work as expected.

diff --git a/pdns/recursordist/test-syncres_cc2.cc b/pdns/recursordist/test-syncres_cc2.cc
index 8f2da0e7b9d466167ae1bb2aba29aa8017021768..34921b0ecd2449eee7dfa01d7322840bd3bcfa2b 100644 (file)
--- a/pdns/recursordist/test-syncres_cc2.cc
+++ b/pdns/recursordist/test-syncres_cc2.cc
@@ -233,6 +233,102 @@ BOOST_AUTO_TEST_CASE(test_glueless_referral_loop_with_nonresolving)
   BOOST_CHECK_EQUAL(SyncRes::getNonResolvingNSSize(), 4U);
 }
 
+BOOST_AUTO_TEST_CASE(test_glueless_referral_with_non_resolving)
+{
+  std::unique_ptr<SyncRes> sr;
+  initSR(sr);
+
+  primeHints();
+
+  const DNSName target("powerdns.com.");
+
+  size_t queryCount = 0;
+
+  sr->setAsyncCallback([target, &queryCount](const ComboAddress& ip, const DNSName& domain, int type, bool doTCP, bool sendRDQuery, int EDNS0Level, struct timeval* now, boost::optional<Netmask>& srcmask, boost::optional<const ResolveContext&> context, LWResult* res, bool* chained) {
+    if (isRootServer(ip)) {
+      setLWResult(res, 0, false, false, true);
+
+      if (domain.isPartOf(DNSName("com."))) {
+        addRecordToLW(res, "com.", QType::NS, "a.gtld-servers.net.", DNSResourceRecord::AUTHORITY, 172800);
+      }
+      else if (domain.isPartOf(DNSName("org."))) {
+        addRecordToLW(res, "org.", QType::NS, "a.gtld-servers.net.", DNSResourceRecord::AUTHORITY, 172800);
+      }
+      else {
+        setLWResult(res, RCode::NXDomain, false, false, true);
+        return LWResult::Result::Success;
+      }
+
+      addRecordToLW(res, "a.gtld-servers.net.", QType::A, "192.0.2.1", DNSResourceRecord::ADDITIONAL, 3600);
+      addRecordToLW(res, "a.gtld-servers.net.", QType::AAAA, "2001:DB8::1", DNSResourceRecord::ADDITIONAL, 3600);
+      return LWResult::Result::Success;
+    }
+    else if (ip == ComboAddress("192.0.2.1:53") || ip == ComboAddress("[2001:DB8::1]:53")) {
+      if (domain == target) {
+        setLWResult(res, 0, false, false, true);
+        addRecordToLW(res, "powerdns.com.", QType::NS, "pdns-public-ns1.powerdns.org.", DNSResourceRecord::AUTHORITY, 172800);
+        addRecordToLW(res, "powerdns.com.", QType::NS, "pdns-public-ns2.powerdns.org.", DNSResourceRecord::AUTHORITY, 172800);
+        return LWResult::Result::Success;
+      }
+      else if (domain == DNSName("pdns-public-ns1.powerdns.org.")) {
+        queryCount++;
+        setLWResult(res, 0, true, false, true);
+        if (queryCount > 8) {
+          addRecordToLW(res, "pdns-public-ns1.powerdns.org.", QType::A, "192.0.2.2");
+          addRecordToLW(res, "pdns-public-ns1.powerdns.org.", QType::AAAA, "2001:DB8::2");
+        }
+        return LWResult::Result::Success;
+      }
+      else if (domain == DNSName("pdns-public-ns2.powerdns.org.")) {
+        queryCount++;
+        setLWResult(res, 0, true, false, true);
+        if (queryCount > 8) {
+          addRecordToLW(res, "pdns-public-ns2.powerdns.org.", QType::A, "192.0.2.3");
+          addRecordToLW(res, "pdns-public-ns2.powerdns.org.", QType::AAAA, "2001:DB8::3");
+        }
+        return LWResult::Result::Success;
+      }
+
+      setLWResult(res, RCode::NXDomain, false, false, true);
+      return LWResult::Result::Success;
+    }
+    else if (ip == ComboAddress("192.0.2.2:53") || ip == ComboAddress("192.0.2.3:53") || ip == ComboAddress("[2001:DB8::2]:53") || ip == ComboAddress("[2001:DB8::3]:53")) {
+      setLWResult(res, 0, true, false, true);
+      addRecordToLW(res, target, QType::A, "192.0.2.4");
+      return LWResult::Result::Success;
+    }
+    else
+      return LWResult::Result::Timeout;
+  });
+
+  SyncRes::s_nonresolvingnsmaxfails = 10;
+  SyncRes::s_nonresolvingnsthrottletime = 60;
+
+  vector<DNSRecord> ret;
+  int res = sr->beginResolve(target, QType(QType::A), QClass::IN, ret);
+  BOOST_CHECK_EQUAL(res, RCode::ServFail);
+  BOOST_REQUIRE_EQUAL(ret.size(), 0U);
+  BOOST_CHECK_EQUAL(SyncRes::getNonResolvingNSSize(), 2U);
+
+  // Again, should not change anything
+  res = sr->beginResolve(target, QType(QType::A), QClass::IN, ret);
+  BOOST_CHECK_EQUAL(res, RCode::ServFail);
+  BOOST_REQUIRE_EQUAL(ret.size(), 0U);
+  //BOOST_CHECK(ret[0].d_type == QType::A);
+  //BOOST_CHECK_EQUAL(ret[0].d_name, target);
+
+  BOOST_CHECK_EQUAL(SyncRes::getNonResolvingNSSize(), 2U);
+
+  // Again, but now things should start working because of the queryCounter getting high enough
+  // and one entry remains in the non-resolving cache
+  res = sr->beginResolve(target, QType(QType::A), QClass::IN, ret);
+  BOOST_CHECK_EQUAL(res, RCode::NoError);
+  BOOST_REQUIRE_EQUAL(ret.size(), 1U);
+  BOOST_CHECK(ret[0].d_type == QType::A);
+  BOOST_CHECK_EQUAL(ret[0].d_name, target);
+  BOOST_CHECK_EQUAL(SyncRes::getNonResolvingNSSize(), 1U);
+}
+
 BOOST_AUTO_TEST_CASE(test_cname_qperq)
 {
   std::unique_ptr<SyncRes> sr;

diff --git a/pdns/syncres.cc b/pdns/syncres.cc
index d0d72c6e0be07979c6442daff995fa9953fd2228..5e4e5db830e5ba475529bda51f2058398a97cf04 100644 (file)
--- a/pdns/syncres.cc
+++ b/pdns/syncres.cc
@@ -2299,13 +2299,13 @@ vector<ComboAddress> SyncRes::retrieveAddressesForNS(const std::string& prefix,
     }
 
     LOG(prefix<<qname<<": Trying to resolve NS '"<<tns->first<< "' ("<<1+tns-rnameservers.begin()<<"/"<<(unsigned int)rnameservers.size()<<")"<<endl);
-    const unsigned int oldnretrieveAddressesForNS = nretrieveAddressesForNS;
+    const unsigned int oldOutQueries = d_outqueries;
     try {
       result = getAddrs(tns->first, depth, beenthere, cacheOnly, nretrieveAddressesForNS);
     }
     // Other exceptions should likely not throttle...
     catch (const ImmediateServFailException& ex) {
-      if (s_nonresolvingnsmaxfails > 0 && nretrieveAddressesForNS > oldnretrieveAddressesForNS) {
+      if (s_nonresolvingnsmaxfails > 0 && d_outqueries > oldOutQueries) {
         auto dontThrottleNames = g_dontThrottleNames.getLocal();
         if (!dontThrottleNames->check(tns->first)) {
           t_sstorage.nonresolving.incr(tns->first, d_now);
@@ -2313,7 +2313,7 @@ vector<ComboAddress> SyncRes::retrieveAddressesForNS(const std::string& prefix,
       }
       throw ex;
     }
-    if (s_nonresolvingnsmaxfails > 0 && nretrieveAddressesForNS > oldnretrieveAddressesForNS) {
+    if (s_nonresolvingnsmaxfails > 0 && d_outqueries > oldOutQueries) {
       if (result.empty()) {
         auto dontThrottleNames = g_dontThrottleNames.getLocal();
         if (!dontThrottleNames->check(tns->first)) {