-
Notifications
You must be signed in to change notification settings - Fork 905
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
dnsdist: Add expungeBySuffix function #5159
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for this PR, very much appreciated! A few minor things to fix and it should be all good!
pdns/dnsdist-cache.cc
Outdated
const CacheValue& value = it->second; | ||
uint16_t cqtype = 0; | ||
uint16_t cqclass = 0; | ||
DNSName cqname(value.value.c_str(), value.len, sizeof(dnsheader), false, &cqtype, &cqclass, nullptr); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The qname
, qtype
and qclass
are stored in the cache entry, we don't need to parse them again. I just noticed DNSDistPacketCache::expungeByName()
does the parsing too, which is awful, we should fix that!
Thinking about it a bit more, we should probably just add a boolean parameter to DNSDistPacketCache::expungeByName()
and do an equal comparison or an isPartOf()
one depending on the boolean value.
""" | ||
misses = 0 | ||
ttl = 600 | ||
name = 'expungebyname.cache.tests.powerdns.com.' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are reusing the exact same qname than a previous test in the same suite (expungebyname.cache.tests.powerdns.com
) so depending on the execution order it might already be cached and the expected miss below will fail. I'd advise using a different qname for each test to prevent this kind of issues.
""" | ||
misses = 0 | ||
ttl = 600 | ||
name = 'expungebynameandtype.cache.tests.powerdns.com.' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same issue here, this name is used by another test of the same suite.
I think the last commit should fix all your points. |
Looks good, thank you! Would you mind squashing a bit before I merge? |
8237526
to
490dc58
Compare
All squashed :) |
Short description
Adds an expungeBySuffix function that allows removing all names matching a suffix from cache. Closes #5109
Checklist
I have: