rec: If serving stale, wipe CNAME records from cache when we get a NODATA negative response for them #13353
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…negative response for them PR PowerDNS#12395 already did that for the NXDOMAIN case.
omoerbeek
force-pushed
the
rec-ss-cname-wipe
branch
from
3a5c6b0
to
f1bebdb
Compare
Pull Request Test Coverage Report for Build 6483780494
💛 - Coveralls |
omoerbeek
force-pushed
the
rec-ss-cname-wipe
branch
from
0dd7d3e
to
db263dd
Compare
fredmorcos
reviewed
Oct 16, 2023
| @@ -4747,7 +4747,7 @@ dState SyncRes::getDenialValidationState(const NegCache::NegCacheEntry& ne, cons | |||
| return getDenial(csp, ne.d_name, ne.d_qtype.getCode(), referralToUnsigned, expectedState == dState::NXQTYPE, LogObject(prefix)); | |||
| } | |||
|
|
|||
| bool SyncRes::processRecords(const std::string& prefix, const DNSName& qname, const QType qtype, const DNSName& auth, LWResult& lwr, const bool sendRDQuery, vector<DNSRecord>& ret, set<DNSName>& nsset, DNSName& newtarget, DNSName& newauth, bool& realreferral, bool& negindic, vState& state, const bool needWildcardProof, const bool gatherWildcardProof, const unsigned int wildcardLabelsCount, int& rcode, bool& negIndicHasSignatures, unsigned int depth) | |||
| bool SyncRes::processRecords(const std::string& prefix, const DNSName& qname, const QType qtype, const DNSName& auth, LWResult& lwr, const bool sendRDQuery, vector<DNSRecord>& ret, set<DNSName>& nsset, DNSName& newtarget, DNSName& newauth, bool& realreferral, bool& negindic, vState& state, const bool needWildcardProof, const bool gatherWildcardProof, const unsigned int wildcardLabelsCount, int& rcode, bool& negIndicHasSignatures, unsigned int depth) // NOLINT(readability-function-cognitive-complexity) | |||
There was a problem hiding this comment.
If it's unaffected by the CI clang-tidy I'd rather remove the // NOLINT here and (potentially at a later point) fix the function implementation.
There was a problem hiding this comment.
If it's unaffected by the CI
clang-tidyI'd rather remove the// NOLINThere and (potentially at a later point) fix the function implementation.
I'll remove it and see if it's filtered out.
rgacogne
reviewed
Oct 16, 2023
| // serve-stale is active. Avoid that by explicitly zapping that CNAME record. | ||
| if (qtype == QType::CNAME && MemRecursorCache::s_maxServedStaleExtensions > 0) { | ||
| g_recCache->doWipeCache(qname, false, qtype); | ||
| } |
There was a problem hiding this comment.
Do we need to also wipe the cache line 4999? I guess we do not because for DS we take care of doing a lookup for the exact type (positive and negative) first, before looking at a possible CNAME, but it feels a bit weird to have a different behaviour in that case.
There was a problem hiding this comment.
Yes, that was my reasoning. It would not hurt though, I believe.
rgacogne
approved these changes
Oct 16, 2023
omoerbeek
added a commit
to omoerbeek/pdns
that referenced
this pull request
Oct 17, 2023
….yml to .gitignore
8 tasks
omoerbeek
added a commit
that referenced
this pull request
Oct 17, 2023
Followup to #13353, missed the suggested change; add recursor.yml to .gitignore
8 tasks
8 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR #12395 already did that for the NXDOMAIN case.
Short description
Checklist
I have: