From: Otto Moerbeek Date: Wed, 22 Mar 2023 13:05:40 +0000 (+0100) Subject: Alternate approach: remmeber the actual orig_ttl. A bit harded than you think, as X-Git-Tag: rec-4.9.0-beta1~2^2~3 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=86c6bb8a16324dd51bf48ce4554e41f301f90e53;p=thirdparty%2Fpdns.git Alternate approach: remmeber the actual orig_ttl. A bit harded than you think, as in somce cases TTDs are modified later. --- diff --git a/pdns/recursordist/recursor_cache.cc b/pdns/recursordist/recursor_cache.cc index c32364c40d..4001df41a8 100644 --- a/pdns/recursordist/recursor_cache.cc +++ b/pdns/recursordist/recursor_cache.cc @@ -531,7 +531,7 @@ bool MemRecursorCache::CacheEntry::shouldReplace(time_t now, bool auth, vState s return true; } -void MemRecursorCache::replace(time_t now, const DNSName& qname, const QType qt, const vector& content, const vector>& signatures, const std::vector>& authorityRecs, bool auth, const DNSName& authZone, boost::optional ednsmask, const OptTag& routingTag, vState state, boost::optional from, bool refresh) +void MemRecursorCache::replace(time_t now, const DNSName& qname, const QType qt, const vector& content, const vector>& signatures, const std::vector>& authorityRecs, bool auth, const DNSName& authZone, boost::optional ednsmask, const OptTag& routingTag, vState state, boost::optional from, bool refresh, uint32_t orig_ttl) { auto& shard = getMap(qname); auto lockedShard = shard.lock(); @@ -608,8 +608,8 @@ void MemRecursorCache::replace(time_t now, const DNSName& qname, const QType qt, prior to calling this function, so the TTL actually holds a TTD. */ ce.d_ttd = min(maxTTD, static_cast(i.d_ttl)); // XXX this does weird things if TTLs differ in the set - ce.d_orig_ttl = ce.d_ttd - now; - + ce.d_orig_ttl = orig_ttl; +#if 0 // d_orig_ttl should be between s_minimumTTL and s_maxcachettl, as d_ttd was sanitized wrt those // bounds. But our reference point (d_now aka now) might be "too new" at this point, if we went // outside to e.g. get DNSKEYS and that took a while. In that case, if the original TTL was @@ -620,6 +620,7 @@ void MemRecursorCache::replace(time_t now, const DNSName& qname, const QType qt, if (ce.d_orig_ttl < SyncRes::s_minimumTTL || ce.d_orig_ttl > SyncRes::s_maxcachettl) { ce.d_orig_ttl = SyncRes::s_minimumTTL; } +#endif ce.d_records.push_back(i.getContent()); } diff --git a/pdns/recursordist/recursor_cache.hh b/pdns/recursordist/recursor_cache.hh index ce37145f00..638728a270 100644 --- a/pdns/recursordist/recursor_cache.hh +++ b/pdns/recursordist/recursor_cache.hh @@ -69,7 +69,7 @@ public: time_t get(time_t, const DNSName& qname, const QType qt, Flags flags, vector* res, const ComboAddress& who, const OptTag& routingTag = boost::none, vector>* signatures = nullptr, std::vector>* authorityRecs = nullptr, bool* variable = nullptr, vState* state = nullptr, bool* wasAuth = nullptr, DNSName* fromAuthZone = nullptr, ComboAddress* fromAuthIP = nullptr); - void replace(time_t, const DNSName& qname, const QType qt, const vector& content, const vector>& signatures, const std::vector>& authorityRecs, bool auth, const DNSName& authZone, boost::optional ednsmask = boost::none, const OptTag& routingTag = boost::none, vState state = vState::Indeterminate, boost::optional from = boost::none, bool refresh = false); + void replace(time_t, const DNSName& qname, const QType qt, const vector& content, const vector>& signatures, const std::vector>& authorityRecs, bool auth, const DNSName& authZone, boost::optional ednsmask = boost::none, const OptTag& routingTag = boost::none, vState state = vState::Indeterminate, boost::optional from = boost::none, bool refresh = false, uint32_t orig_ttl = 0); void doPrune(size_t keep); uint64_t doDump(int fd, size_t maxCacheEntries); diff --git a/pdns/recursordist/syncres.cc b/pdns/recursordist/syncres.cc index 6119a325e9..bb19aa9f21 100644 --- a/pdns/recursordist/syncres.cc +++ b/pdns/recursordist/syncres.cc @@ -2627,6 +2627,7 @@ struct CacheEntry vector records; vector> signatures; uint32_t signaturesTTL{std::numeric_limits::max()}; + uint32_t d_orig_ttl{0}; }; struct CacheKey { @@ -4410,6 +4411,7 @@ RCode::rcodes_ SyncRes::updateCacheFromRecords(unsigned int depth, const string& rec.d_ttl = min(s_maxcachettl, rec.d_ttl); DNSRecord dr(rec); + tcache[{rec.d_name, rec.d_type, rec.d_place}].d_orig_ttl = dr.d_ttl; dr.d_ttl += d_now.tv_sec; dr.d_place = DNSResourceRecord::ANSWER; tcache[{rec.d_name, rec.d_type, rec.d_place}].records.push_back(dr); @@ -4424,9 +4426,17 @@ RCode::rcodes_ SyncRes::updateCacheFromRecords(unsigned int depth, const string& if ((entry.second.records.size() + entry.second.signatures.size() + authorityRecs.size()) > 1) { // need to group the ttl to be the minimum of the RRSET (RFC 2181, 5.2) uint32_t lowestTTD = computeLowestTTD(entry.second.records, entry.second.signatures, entry.second.signaturesTTL, authorityRecs); + uint32_t compensation = 0; for (auto& record : entry.second.records) { + compensation = record.d_ttl - lowestTTD; record.d_ttl = lowestTTD; // boom } + entry.second.d_orig_ttl -= compensation; +#if 0 + if (compensation != 0) { + cerr << entry.first.name << '/' << entry.first.type << " compensated " << compensation << endl; + } +#endif } } @@ -4533,10 +4543,19 @@ RCode::rcodes_ SyncRes::updateCacheFromRecords(unsigned int depth, const string& } if (vStateIsBogus(recordState)) { + uint32_t compensation = 0; /* this is a TTD by now, be careful */ for (auto& record : i->second.records) { - record.d_ttl = std::min(record.d_ttl, static_cast(s_maxbogusttl + d_now.tv_sec)); + auto newval = std::min(record.d_ttl, static_cast(s_maxbogusttl + d_now.tv_sec)); + compensation = record.d_ttl - newval; + record.d_ttl = newval; } + i->second.d_orig_ttl -= compensation; +#if 0 + if (compensation != 0) { + cerr << i->first.name << '/' << i->first.type << " compensated by bogus case " << compensation << endl; + } +#endif } /* We don't need to store NSEC3 records in the positive cache because: @@ -4582,7 +4601,7 @@ RCode::rcodes_ SyncRes::updateCacheFromRecords(unsigned int depth, const string& if (isAA && i->first.type == QType::NS && s_save_parent_ns_set) { rememberParentSetIfNeeded(i->first.name, i->second.records, depth, prefix); } - g_recCache->replace(d_now.tv_sec, i->first.name, i->first.type, i->second.records, i->second.signatures, authorityRecs, i->first.type == QType::DS ? true : isAA, auth, i->first.place == DNSResourceRecord::ANSWER ? ednsmask : boost::none, d_routingTag, recordState, remoteIP, d_refresh); + g_recCache->replace(d_now.tv_sec, i->first.name, i->first.type, i->second.records, i->second.signatures, authorityRecs, i->first.type == QType::DS ? true : isAA, auth, i->first.place == DNSResourceRecord::ANSWER ? ednsmask : boost::none, d_routingTag, recordState, remoteIP, d_refresh, i->second.d_orig_ttl); // Delete potential negcache entry. When a record recovers with serve-stale the negcache entry can cause the wrong entry to // be served, as negcache entries are checked before record cache entries @@ -4606,7 +4625,7 @@ RCode::rcodes_ SyncRes::updateCacheFromRecords(unsigned int depth, const string& content.push_back(std::move(nonExpandedRecord)); } - g_recCache->replace(d_now.tv_sec, realOwner, QType(i->first.type), content, i->second.signatures, /* no additional records in that case */ {}, i->first.type == QType::DS ? true : isAA, auth, boost::none, boost::none, recordState, remoteIP, d_refresh); + g_recCache->replace(d_now.tv_sec, realOwner, QType(i->first.type), content, i->second.signatures, /* no additional records in that case */ {}, i->first.type == QType::DS ? true : isAA, auth, boost::none, boost::none, recordState, remoteIP, d_refresh, i->second.d_orig_ttl); } } } diff --git a/pdns/recursordist/test-syncres_cc2.cc b/pdns/recursordist/test-syncres_cc2.cc index 8d4bb74b50..7c51872db1 100644 --- a/pdns/recursordist/test-syncres_cc2.cc +++ b/pdns/recursordist/test-syncres_cc2.cc @@ -1806,12 +1806,12 @@ BOOST_AUTO_TEST_CASE(test_cache_almost_expired_ttl) std::vector> sigs; addRecordToList(records, target, QType::A, "192.0.2.2", DNSResourceRecord::ANSWER, now + 29); - g_recCache->replace(now - 30, target, QType(QType::A), records, sigs, vector>(), true, g_rootdnsname, boost::optional()); + g_recCache->replace(now - 30, target, QType(QType::A), records, sigs, {}, true, g_rootdnsname, boost::optional(), boost::none, vState::Indeterminate, boost::none, false, 60); /* Same for the NS record */ std::vector ns; addRecordToList(ns, target, QType::NS, "pdns-public-ns1.powerdns.com", DNSResourceRecord::ANSWER, now + 29); - g_recCache->replace(now - 30, target, QType::NS, ns, sigs, vector>(), false, target, boost::optional()); + g_recCache->replace(now - 30, target, QType::NS, ns, sigs, {}, false, target, boost::optional(), boost::none, vState::Indeterminate, boost::none, false, 60); vector ret; int res = sr->beginResolve(target, QType(QType::A), QClass::IN, ret); @@ -1823,7 +1823,7 @@ BOOST_AUTO_TEST_CASE(test_cache_almost_expired_ttl) BOOST_CHECK_EQUAL(ttl, 29U); // One task should be submitted - BOOST_CHECK_EQUAL(getTaskSize(), 1U); + BOOST_REQUIRE_EQUAL(getTaskSize(), 1U); auto task = taskQueuePop();