From: Otto Moerbeek Date: Mon, 15 May 2023 13:48:46 +0000 (+0200) Subject: Record time the ttd was computed instead of compensating X-Git-Tag: rec-4.9.0-beta1~2^2~2 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=46dcd67bbf8151089f514179840777f51f8ea6a7;p=thirdparty%2Fpdns.git Record time the ttd was computed instead of compensating --- diff --git a/pdns/recursordist/recursor_cache.cc b/pdns/recursordist/recursor_cache.cc index 4001df41a8..8b5b72f908 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, uint32_t orig_ttl) +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, time_t ttl_time) { auto& shard = getMap(qname); auto lockedShard = shard.lock(); @@ -608,19 +608,7 @@ 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 = 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 - // smaller than the delay, d_orig_ttl will wrap and become very large. Detect that case and - // make sure d_orig_ttl is fixed. Likewise if there was a delay but that was smaller than the - // original TTL, d_orig_ttl can become smaller than s_minimumTTL. Detect those cases and use a - // small but legal d_orig_ttl in those cases. - 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_orig_ttl = ce.d_ttd - ttl_time; ce.d_records.push_back(i.getContent()); } diff --git a/pdns/recursordist/recursor_cache.hh b/pdns/recursordist/recursor_cache.hh index 638728a270..379249b610 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, uint32_t orig_ttl = 0); + 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, time_t ttl_time = time(nullptr)); 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 bb19aa9f21..e797035f53 100644 --- a/pdns/recursordist/syncres.cc +++ b/pdns/recursordist/syncres.cc @@ -2626,8 +2626,8 @@ struct CacheEntry { vector records; vector> signatures; + time_t d_ttl_time{0}; uint32_t signaturesTTL{std::numeric_limits::max()}; - uint32_t d_orig_ttl{0}; }; struct CacheKey { @@ -4269,7 +4269,7 @@ void SyncRes::rememberParentSetIfNeeded(const DNSName& domain, const vector ednsmask, vState& state, bool& needWildcardProof, bool& gatherWildcardProof, unsigned int& wildcardLabelsCount, bool rdQuery, const ComboAddress& remoteIP) +RCode::rcodes_ SyncRes::updateCacheFromRecords(unsigned int depth, const string& prefix, LWResult& lwr, const DNSName& qname, const QType qtype, const DNSName& auth, bool wasForwarded, const boost::optional ednsmask, vState& state, bool& needWildcardProof, bool& gatherWildcardProof, unsigned int& wildcardLabelsCount, bool rdQuery, const ComboAddress& remoteIP) // NOLINT(readability-function-cognitive-complexity) { bool wasForwardRecurse = wasForwarded && rdQuery; tcache_t tcache; @@ -4411,7 +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; + tcache[{rec.d_name, rec.d_type, rec.d_place}].d_ttl_time = d_now.tv_sec; 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); @@ -4426,17 +4426,9 @@ 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 } } @@ -4543,19 +4535,12 @@ 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) { 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 + i->second.d_ttl_time = d_now.tv_sec; } /* We don't need to store NSEC3 records in the positive cache because: @@ -4601,7 +4586,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, i->second.d_orig_ttl); + 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_ttl_time); // 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 @@ -4625,7 +4610,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, i->second.d_orig_ttl); + 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_ttl_time); } } } diff --git a/pdns/recursordist/test-syncres_cc2.cc b/pdns/recursordist/test-syncres_cc2.cc index 7c51872db1..7f761238f0 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, {}, true, g_rootdnsname, boost::optional(), boost::none, vState::Indeterminate, boost::none, false, 60); + g_recCache->replace(now - 30, target, QType(QType::A), records, sigs, {}, true, g_rootdnsname, boost::optional(), boost::none, vState::Indeterminate, boost::none, false, now - 31); /* 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, {}, false, target, boost::optional(), boost::none, vState::Indeterminate, boost::none, false, 60); + g_recCache->replace(now - 30, target, QType::NS, ns, sigs, {}, false, target, boost::optional(), boost::none, vState::Indeterminate, boost::none, false, now - 31); vector ret; int res = sr->beginResolve(target, QType(QType::A), QClass::IN, ret);