From: Otto Moerbeek Date: Tue, 17 Dec 2024 09:24:04 +0000 (+0100) Subject: Add test X-Git-Tag: dnsdist-2.0.0-alpha1~203^2 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=refs%2Fpull%2F14973%2Fhead;p=thirdparty%2Fpdns.git Add test The last step of the test shows that there is likely room for more improvement. --- diff --git a/pdns/recursordist/syncres.cc b/pdns/recursordist/syncres.cc index 20a7c3d4b4..b8a24c5c85 100644 --- a/pdns/recursordist/syncres.cc +++ b/pdns/recursordist/syncres.cc @@ -2014,7 +2014,7 @@ int SyncRes::doResolveNoQNameMinimization(const DNSName& qname, const QType qtyp } // This handles the case mentioned above: if the full CNAME chain leading to the answer was // constructed from the cache, indicate that. - if (fromCache != nullptr && *fromCache == false && haveFinalAnswer(qname, qtype, res, ret)) { + if (fromCache != nullptr && !*fromCache && haveFinalAnswer(qname, qtype, res, ret)) { *fromCache = true; } return res; @@ -2066,7 +2066,7 @@ int SyncRes::doResolveNoQNameMinimization(const DNSName& qname, const QType qtyp } } } - if (fromCache != nullptr && *fromCache == false && haveFinalAnswer(qname, qtype, res, ret)) { + if (fromCache != nullptr && !*fromCache && haveFinalAnswer(qname, qtype, res, ret)) { *fromCache = true; } return res; diff --git a/pdns/recursordist/test-syncres_cc1.cc b/pdns/recursordist/test-syncres_cc1.cc index d51953fd2f..af5fccaa28 100644 --- a/pdns/recursordist/test-syncres_cc1.cc +++ b/pdns/recursordist/test-syncres_cc1.cc @@ -1737,6 +1737,117 @@ BOOST_AUTO_TEST_CASE(test_cname_long_loop) } } +BOOST_AUTO_TEST_CASE(test_cname_long_step0_shortcut) +{ + // This tets the case fixed /optimizaed in #14973 + std::unique_ptr resolver; + initSR(resolver); + resolver->setQNameMinimization(); + primeHints(); + resolver->setLogMode(SyncRes::Store); + + size_t count = 0; + const DNSName target1("cname1.powerdns.com."); + const DNSName target2("cname2.powerdns.com."); + const DNSName target3("cname3.powerdns.com."); + const DNSName target4("cname4.powerdns.com."); + + resolver->setAsyncCallback([&](const ComboAddress& address, const DNSName& domain, int qtype, bool /* doTCP */, bool /* sendRDQuery */, int /* EDNS0Level */, struct timeval* /* now */, boost::optional& /* srcmask */, const ResolveContext& /* context */, LWResult* res, bool* /* chained */) { + count++; + + if (domain == DNSName("com.")) { + setLWResult(res, 0, false, false, true); + addRecordToLW(res, domain, QType::NS, "a.gtld-servers.net.", DNSResourceRecord::AUTHORITY, 172800); + addRecordToLW(res, "a.gtld-servers.net.", QType::A, "192.0.2.1", DNSResourceRecord::ADDITIONAL, 3600); + return LWResult::Result::Success; + } + if (domain == DNSName("powerdns.com.")) { + setLWResult(res, 0, false, false, true); + addRecordToLW(res, domain, QType::NS, "ns.powerdns.com.", DNSResourceRecord::AUTHORITY, 172800); + addRecordToLW(res, "ns.powerdns.com.", QType::A, "192.0.2.2", DNSResourceRecord::ADDITIONAL, 3600); + return LWResult::Result::Success; + } + if (address == ComboAddress("192.0.2.2:53")) { + + if (domain == target1) { + setLWResult(res, 0, true, false, false); + addRecordToLW(res, domain, QType::CNAME, target2.toString()); + return LWResult::Result::Success; + } + if (domain == target2) { + setLWResult(res, 0, true, false, false); + addRecordToLW(res, domain, QType::CNAME, target3.toString()); + return LWResult::Result::Success; + } + if (domain == target3) { + setLWResult(res, 0, true, false, false); + addRecordToLW(res, domain, QType::CNAME, target4.toString()); + return LWResult::Result::Success; + } + if (domain == target4) { + setLWResult(res, 0, true, false, false); + if (qtype == QType::A) { + addRecordToLW(res, domain, QType::A, "1.2.3.4"); + } + else if (qtype == QType::AAAA) { + addRecordToLW(res, domain, QType::AAAA, "::1234"); + } + return LWResult::Result::Success; + } + + return LWResult::Result::Success; + } + + return LWResult::Result::Timeout; + }); + + // We analyze the trace to see how many cases of a failed Step0 occurs. This is a bit dirty, but + // we have no other way of telling how the resolving took place, as the number of cache lookups is + // not recorded by the record cache. Also, we like to know if something in Syncres changed that + // influences the number of failing Step0 lookups. + auto counter = [](const string& str) { + const std::string key = "Step0 Not cached"; + size_t occurences = 0; + auto pos = str.find(key); + while (pos != std::string::npos) { + occurences++; + pos = str.find(key, pos + 1); + } + return occurences; + }; + vector ret; + int res = resolver->beginResolve(target1, QType(QType::AAAA), QClass::IN, ret); + BOOST_CHECK_EQUAL(res, RCode::NoError); + BOOST_CHECK_EQUAL(ret.size(), 4U); + BOOST_CHECK_EQUAL(count, 6U); + BOOST_CHECK_EQUAL(resolver->d_maxdepth, 3U); + auto trace = resolver->getTrace(); + trace = resolver->getTrace(); + BOOST_CHECK_EQUAL(counter(trace), 4U); + + // And again to check cache, all Step0 cases should succeed + ret.clear(); + res = resolver->beginResolve(target1, QType(QType::AAAA), QClass::IN, ret); + BOOST_CHECK_EQUAL(res, RCode::NoError); + BOOST_CHECK_EQUAL(ret.size(), 4U); + BOOST_CHECK_EQUAL(count, 6U); + BOOST_CHECK_EQUAL(resolver->d_maxdepth, 3U); + trace = resolver->getTrace(); + BOOST_CHECK_EQUAL(counter(trace), 4U); + + // And again to check a name that does not fully resolve, we expect Step0 failures to increase + g_recCache->doWipeCache(target4, false, QType::AAAA); + ret.clear(); + res = resolver->beginResolve(target1, QType(QType::AAAA), QClass::IN, ret); + BOOST_CHECK_EQUAL(res, RCode::NoError); + BOOST_CHECK_EQUAL(ret.size(), 4U); + BOOST_CHECK_EQUAL(count, 7U); + BOOST_CHECK_EQUAL(resolver->d_maxdepth, 3U); + trace = resolver->getTrace(); + // XXX This number feels pretty high (15 extra, same as before #14973, there seems to be room for more improvement). + BOOST_CHECK_EQUAL(counter(trace), 19U); +} + BOOST_AUTO_TEST_CASE(test_cname_length) { std::unique_ptr sr;