From: Otto Moerbeek Date: Tue, 17 Jan 2023 09:00:30 +0000 (+0100) Subject: Change the way RD=0 forwarded queries are handled. X-Git-Tag: dnsdist-1.8.0-rc1~107^2 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=refs%2Fpull%2F12425%2Fhead;p=thirdparty%2Fpdns.git 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 e4f2aa3ce2..2b46e54af0 100644 --- a/pdns/recursordist/syncres.cc +++ b/pdns/recursordist/syncres.cc @@ -1655,8 +1655,7 @@ int SyncRes::doResolve(const DNSName& qname, const QType qtype, vector& ret, unsigned int depth, set& beenthere, Context& context, bool* fromCache, StopAtDelegation* stopAtDelegation, bool considerforwards) +int SyncRes::doResolveNoQNameMinimization(const DNSName& qname, const QType qtype, vector& ret, unsigned int depth, set& 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& 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 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(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 151d279b99..1fa9ff63e0 100644 --- 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 ednsmask, bool sendRDQuery, NsSet& nameservers, std::vector& ret, const DNSFilterEngine& dfe, bool* gotNewServers, int* rcode, vState& state, const ComboAddress& remoteIP); int doResolve(const DNSName& qname, QType qtype, vector& ret, unsigned int depth, set& beenthere, Context& context); - int doResolveNoQNameMinimization(const DNSName& qname, QType qtype, vector& ret, unsigned int depth, set& beenthere, Context& context, bool* fromCache = NULL, StopAtDelegation* stopAtDelegation = NULL, bool considerforwards = true); + int doResolveNoQNameMinimization(const DNSName& qname, QType qtype, vector& ret, unsigned int depth, set& beenthere, Context& context, bool* fromCache = NULL, StopAtDelegation* stopAtDelegation = NULL); bool doOOBResolve(const AuthDomain& domain, const DNSName& qname, QType qtype, vector& ret, int& res); bool doOOBResolve(const DNSName& qname, QType qtype, vector& 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 a75dcf2267..1fe09133de 100644 --- 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& srcmask, boost::optional 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& srcmask, boost::optional 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 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& srcmask, boost::optional 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& srcmask, boost::optional 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 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)