From f58b74b096559655ce205cdc4d12299d96a4dd97 Mon Sep 17 00:00:00 2001 From: Otto Moerbeek Date: Mon, 16 Dec 2024 11:19:17 +0100 Subject: [PATCH 1/3] rec: if the full CNAME chain leading to the answer is cached, indicate that Alternative approach to #14918 --- pdns/recursordist/syncres.cc | 35 ++++++++++++++++++++++++++++++++++- 1 file changed, 34 insertions(+), 1 deletion(-) diff --git a/pdns/recursordist/syncres.cc b/pdns/recursordist/syncres.cc index 9a318dfc46ba..a3591e931121 100644 --- a/pdns/recursordist/syncres.cc +++ b/pdns/recursordist/syncres.cc @@ -1885,6 +1885,32 @@ unsigned int SyncRes::getAdjustedRecursionBound() const return bound; } +static bool haveFinalAnswer(const DNSName& qname, QType qtype, int res, const vector& ret) +{ + if (res != RCode::NoError) { + return false; + } + + // This loop assumes the CNAME's records are in-order + DNSName target(qname); + for (const auto& record : ret) { + if (record.d_place == DNSResourceRecord::ANSWER && record.d_name == target) { + if (record.d_type == qtype) { + return true; + } + if (record.d_type == QType::CNAME) { + if (auto ptr = getRR(record)) { + target = ptr->getTarget(); + } + else { + return false; + } + } + } + } + return false; +} + /*! This function will check the cache and go out to the internet if the answer is not in cache * * \param qname The name we need an answer for @@ -1986,6 +2012,11 @@ 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 && haveFinalAnswer(qname, qtype, res, ret)) { + *fromCache = true; + } return res; } @@ -2035,7 +2066,9 @@ int SyncRes::doResolveNoQNameMinimization(const DNSName& qname, const QType qtyp } } } - + if (fromCache != nullptr && haveFinalAnswer(qname, qtype, res, ret)) { + *fromCache = true; + } return res; } } From 3455105d0de1b738156440b70f6711fd1e6128fa Mon Sep 17 00:00:00 2001 From: Otto Moerbeek Date: Mon, 16 Dec 2024 17:13:49 +0100 Subject: [PATCH 2/3] if *fromCache is already true, no need to check CNAME chain Co-authored-by: Remi Gacogne --- pdns/recursordist/syncres.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pdns/recursordist/syncres.cc b/pdns/recursordist/syncres.cc index a3591e931121..20a7c3d4b400 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 && haveFinalAnswer(qname, qtype, res, ret)) { + if (fromCache != nullptr && *fromCache == false && 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 && haveFinalAnswer(qname, qtype, res, ret)) { + if (fromCache != nullptr && *fromCache == false && haveFinalAnswer(qname, qtype, res, ret)) { *fromCache = true; } return res; From f533aca00994264373a01b58c27efe25104e0cfa Mon Sep 17 00:00:00 2001 From: Otto Moerbeek Date: Tue, 17 Dec 2024 10:24:04 +0100 Subject: [PATCH 3/3] Add test The last step of the test shows that there is likely room for more improvement. --- pdns/recursordist/syncres.cc | 4 +- pdns/recursordist/test-syncres_cc1.cc | 111 ++++++++++++++++++++++++++ 2 files changed, 113 insertions(+), 2 deletions(-) diff --git a/pdns/recursordist/syncres.cc b/pdns/recursordist/syncres.cc index 20a7c3d4b400..b8a24c5c8506 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 d51953fd2ff7..af5fccaa28c5 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;