Change the way RD=0 forwarded queries are handled.

Since forever, there has been special case code for forwarded queries
in the RD=0 case.  This special case code does a hardcoded RD=0
query to the specified forwarder.  This code has two consequences:

1. Even if the forwarder is marked recursive it gets a RD=0 query
2. The cache is not consulted at all

The corresponding unit tests actually test this behaviour, but after
historic digging with help from @rgacogne it turns out the the unit
test do not reflect the desired functionality, but the current state
of affairs to help with a refactoring PR.  That is good, since
refactoring should not change functionality.

But now the time has come to change the code to do the desired thing:

1. If an RD=0 query is received, do a cache only-lookup in all cases.
2. Never send a RD=0 query to a recursive forwarder

I already did a similar thing when I wrote the QName Minimization
code, introducing a conditional that only gets set for that case,
to avoid changing unrelated (to QM) functionality.

diff --git a/pdns/recursordist/syncres.cc b/pdns/recursordist/syncres.cc
index e4f2aa3ce24f02eedd63d5f06324576f17c37936..2b46e54af072dafb73cf3d8a260cacf83a26a258 100644 (file)
--- a/pdns/recursordist/syncres.cc
+++ b/pdns/recursordist/syncres.cc
@@ -1655,8 +1655,7 @@ int SyncRes::doResolve(const DNSName& qname, const QType qtype, vector<DNSRecord
   bool fromCache = false;
   // For cache peeking, we tell doResolveNoQNameMinimization not to consider the (non-recursive) forward case.
   // Otherwise all queries in a forward domain will be forwarded, while we want to consult the cache.
-  // The out-of-band cases for doResolveNoQNameMinimization() should be reconsidered and redone some day.
-  int res = doResolveNoQNameMinimization(qname, qtype, retq, depth, beenthere, context, &fromCache, nullptr, false);
+  int res = doResolveNoQNameMinimization(qname, qtype, retq, depth, beenthere, context, &fromCache, nullptr);
   setCacheOnly(old);
   if (fromCache) {
     QLOG("Step0 Found in cache");
@@ -1805,7 +1804,7 @@ int SyncRes::doResolve(const DNSName& qname, const QType qtype, vector<DNSRecord
  * \param stopAtDelegation if non-nullptr and pointed-to value is Stop requests the callee to stop at a delegation, if so pointed-to value is set to Stopped
  * \return DNS RCODE or -1 (Error)
  */
-int SyncRes::doResolveNoQNameMinimization(const DNSName& qname, const QType qtype, vector<DNSRecord>& ret, unsigned int depth, set<GetBestNSAnswer>& beenthere, Context& context, bool* fromCache, StopAtDelegation* stopAtDelegation, bool considerforwards)
+int SyncRes::doResolveNoQNameMinimization(const DNSName& qname, const QType qtype, vector<DNSRecord>& ret, unsigned int depth, set<GetBestNSAnswer>& beenthere, Context& context, bool* fromCache, StopAtDelegation* stopAtDelegation)
 {
   string prefix;
   if (doLog()) {
@@ -1815,7 +1814,7 @@ int SyncRes::doResolveNoQNameMinimization(const DNSName& qname, const QType qtyp
 
   LOG(prefix << qname << ": Wants " << (d_doDNSSEC ? "" : "NO ") << "DNSSEC processing, " << (d_requireAuthData ? "" : "NO ") << "auth data in query for " << qtype << endl);
 
-  if (s_maxdepth && depth > s_maxdepth) {
+  if (s_maxdepth > 0 && depth > s_maxdepth) {
     string msg = "More than " + std::to_string(s_maxdepth) + " (max-recursion-depth) levels of recursion needed while resolving " + qname.toLogString();
     LOG(prefix << qname << ": " << msg << endl);
     throw ImmediateServFailException(msg);
@@ -1835,63 +1834,34 @@ int SyncRes::doResolveNoQNameMinimization(const DNSName& qname, const QType qtyp
     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())) {
-        if (d_cacheonly) { // very limited OOB support
-          LWResult lwr;
-          LOG(prefix << qname << ": cache only lookup for '" << qname << "|" << qtype << "', first peeking at auth/forward zones" << endl);
-          DNSName authname(qname);
-          domainmap_t::const_iterator iter = getBestAuthZone(&authname);
+        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, res);
-              if (fromCache)
+              if (fromCache != nullptr) {
                 *fromCache = d_wasOutOfBand;
-              return res;
-            }
-            else if (considerforwards) {
-              const vector<ComboAddress>& servers = iter->second.d_servers;
-              const ComboAddress remoteIP = servers.front();
-              LOG(prefix << qname << ": forwarding query to hardcoded nameserver '" << remoteIP.toStringWithPort() << "' for zone '" << authname << "'" << endl);
-
-              boost::optional<Netmask> nm;
-              bool chained = false;
-              // forwardes are "anonymous", so plug in an empty name; some day we might have a fancier config language...
-              auto resolveRet = asyncresolveWrapper(remoteIP, d_doDNSSEC, qname, authname, qtype.getCode(), false, false, &d_now, nm, &lwr, &chained, DNSName());
-
-              d_totUsec += lwr.d_usec;
-              accountAuthLatency(lwr.d_usec, remoteIP.sin4.sin_family);
-              ++t_Counters.at(rec::RCode::auth).rcodeCounters.at(static_cast<uint8_t>(lwr.d_rcode));
-              if (fromCache)
-                *fromCache = true;
-
-              // filter out the good stuff from lwr.result()
-              if (resolveRet == LWResult::Result::Success) {
-                for (const auto& rec : lwr.d_records) {
-                  if (rec.d_place == DNSResourceRecord::ANSWER)
-                    ret.push_back(rec);
-                }
-                return 0;
-              }
-              else {
-                return RCode::ServFail;
               }
+              return res;
             }
           }
         }
 
-        DNSName authname(qname);
         bool wasForwardedOrAuthZone = false;
         bool wasAuthZone = false;
         bool wasForwardRecurse = false;
-        domainmap_t::const_iterator iter = getBestAuthZone(&authname);
+
         if (iter != t_sstorage.domainmap->end()) {
-          const auto& domain = iter->second;
           wasForwardedOrAuthZone = true;
 
-          if (domain.isAuth()) {
+          if (iter->second.isAuth()) {
             wasAuthZone = true;
           }
-          else if (domain.shouldRecurse()) {
+          else if (iter->second.shouldRecurse()) {
             wasForwardRecurse = true;
           }
         }

diff --git a/pdns/recursordist/syncres.hh b/pdns/recursordist/syncres.hh
index 151d279b99a26eb9a0aef1418eb75c5302fd86a8..1fa9ff63e04f71729712ef58e97d659ce171ac61 100644 (file)
--- a/pdns/recursordist/syncres.hh
+++ b/pdns/recursordist/syncres.hh
@@ -590,7 +590,7 @@ private:
   bool processAnswer(unsigned int depth, LWResult& lwr, const DNSName& qname, const 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);
 
   int doResolve(const DNSName& qname, QType qtype, vector<DNSRecord>& ret, unsigned int depth, set<GetBestNSAnswer>& beenthere, Context& context);
-  int doResolveNoQNameMinimization(const DNSName& qname, QType qtype, vector<DNSRecord>& ret, unsigned int depth, set<GetBestNSAnswer>& beenthere, Context& context, bool* fromCache = NULL, StopAtDelegation* stopAtDelegation = NULL, bool considerforwards = true);
+  int doResolveNoQNameMinimization(const DNSName& qname, QType qtype, vector<DNSRecord>& ret, unsigned int depth, set<GetBestNSAnswer>& beenthere, Context& context, bool* fromCache = NULL, StopAtDelegation* stopAtDelegation = NULL);
   bool doOOBResolve(const AuthDomain& domain, const DNSName& qname, QType qtype, vector<DNSRecord>& ret, int& res);
   bool doOOBResolve(const DNSName& qname, QType qtype, vector<DNSRecord>& ret, unsigned int depth, int& res);
   bool isRecursiveForwardOrAuth(const DNSName& qname) const;

diff --git a/pdns/recursordist/test-syncres_cc3.cc b/pdns/recursordist/test-syncres_cc3.cc
index a75dcf2267caa18ab3705a99744d5980c8491e79..1fe09133de6bdbb1117f8757f7c392c6533e9dc3 100644 (file)
--- a/pdns/recursordist/test-syncres_cc3.cc
+++ b/pdns/recursordist/test-syncres_cc3.cc
@@ -762,7 +762,10 @@ BOOST_AUTO_TEST_CASE(test_forward_zone_nord)
   ad.d_servers.push_back(forwardedNS);
   (*SyncRes::t_sstorage.domainmap)[target] = ad;
 
-  sr->setAsyncCallback([forwardedNS](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) {
+  size_t queriesCount = 0;
+
+  sr->setAsyncCallback([forwardedNS, &queriesCount](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) {
+    ++queriesCount;
     if (ip == forwardedNS) {
       BOOST_CHECK_EQUAL(sendRDQuery, false);
 
@@ -774,13 +777,33 @@ BOOST_AUTO_TEST_CASE(test_forward_zone_nord)
     return LWResult::Result::Timeout;
   });
 
+  BOOST_CHECK_EQUAL(queriesCount, 0U);
   /* simulate a no-RD query */
   sr->setCacheOnly();
 
   vector<DNSRecord> ret;
   int res = sr->beginResolve(target, QType(QType::A), QClass::IN, ret);
   BOOST_CHECK_EQUAL(res, RCode::NoError);
+  BOOST_CHECK_EQUAL(ret.size(), 0U);
+  BOOST_CHECK_EQUAL(queriesCount, 0U);
+
+  /* simulate a RD query */
+  sr->setCacheOnly(false);
+
+  ret.clear();
+  res = sr->beginResolve(target, QType(QType::A), QClass::IN, ret);
+  BOOST_CHECK_EQUAL(res, RCode::NoError);
   BOOST_CHECK_EQUAL(ret.size(), 1U);
+  BOOST_CHECK_EQUAL(queriesCount, 1U);
+
+  /* simulate a no-RD query */
+  sr->setCacheOnly();
+
+  ret.clear();
+  res = sr->beginResolve(target, QType(QType::A), QClass::IN, ret);
+  BOOST_CHECK_EQUAL(res, RCode::NoError);
+  BOOST_CHECK_EQUAL(ret.size(), 1U);
+  BOOST_CHECK_EQUAL(queriesCount, 1U);
 }
 
 BOOST_AUTO_TEST_CASE(test_forward_zone_rd)
@@ -849,9 +872,12 @@ BOOST_AUTO_TEST_CASE(test_forward_zone_recurse_nord)
   ad.d_servers.push_back(forwardedNS);
   (*SyncRes::t_sstorage.domainmap)[target] = ad;
 
-  sr->setAsyncCallback([forwardedNS](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) {
+  size_t queriesCount = 0;
+
+  sr->setAsyncCallback([forwardedNS, &queriesCount](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) {
+    ++queriesCount;
     if (ip == forwardedNS) {
-      BOOST_CHECK_EQUAL(sendRDQuery, false);
+      BOOST_CHECK_EQUAL(sendRDQuery, true);
 
       setLWResult(res, 0, true, false, true);
       addRecordToLW(res, domain, QType::A, "192.0.2.42");
@@ -861,13 +887,33 @@ BOOST_AUTO_TEST_CASE(test_forward_zone_recurse_nord)
     return LWResult::Result::Timeout;
   });
 
+  BOOST_CHECK_EQUAL(queriesCount, 0U);
   /* simulate a no-RD query */
   sr->setCacheOnly();
 
   vector<DNSRecord> ret;
   int res = sr->beginResolve(target, QType(QType::A), QClass::IN, ret);
   BOOST_CHECK_EQUAL(res, RCode::NoError);
+  BOOST_CHECK_EQUAL(ret.size(), 0U);
+  BOOST_CHECK_EQUAL(queriesCount, 0U);
+
+  /* simulate a RD query */
+  sr->setCacheOnly(false);
+
+  ret.clear();
+  res = sr->beginResolve(target, QType(QType::A), QClass::IN, ret);
+  BOOST_CHECK_EQUAL(res, RCode::NoError);
   BOOST_CHECK_EQUAL(ret.size(), 1U);
+  BOOST_CHECK_EQUAL(queriesCount, 1U);
+
+  /* simulate a no-RD query */
+  sr->setCacheOnly();
+
+  ret.clear();
+  res = sr->beginResolve(target, QType(QType::A), QClass::IN, ret);
+  BOOST_CHECK_EQUAL(res, RCode::NoError);
+  BOOST_CHECK_EQUAL(ret.size(), 1U);
+  BOOST_CHECK_EQUAL(queriesCount, 1U);
 }
 
 BOOST_AUTO_TEST_CASE(test_forward_zone_recurse_rd)