Skip to content
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

Provide additional answers in NAPTR queries #15083

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

miodvallat
Copy link
Contributor

Short description

This adds additional answers with replacement names found in NAPTR queries when known, and NAPTR records have the S or A flags. This is similar to what the recursor already does, and is based upon its logic.

Takes over #12966 (with @Habbie's approval)

Checklist

I have:

  • read the CONTRIBUTING.md document
  • compiled this code
  • tested this code
  • included documentation (including possible behaviour changes)
  • documented the code
  • added or modified regression test(s)
  • added or modified unit test(s)
  • ticked at least 4 boxes above

@coveralls
Copy link

coveralls commented Jan 24, 2025

Pull Request Test Coverage Report for Build 13116764149

Details

  • 27 of 27 (100.0%) changed or added relevant lines in 1 file are covered.
  • 33 unchanged lines in 12 files lost coverage.
  • Overall coverage increased (+0.01%) to 64.711%

Files with Coverage Reduction New Missed Lines %
modules/gpgsqlbackend/gpgsqlbackend.cc 1 88.62%
pdns/validate.cc 1 68.24%
pdns/dnsdistdist/dnsdist.hh 1 84.18%
pdns/misc.cc 2 62.93%
modules/lmdbbackend/lmdbbackend.cc 2 72.71%
pdns/rcpgenerator.cc 2 90.1%
pdns/tcpreceiver.cc 2 66.82%
pdns/opensslsigners.cc 3 61.34%
pdns/shuffle.cc 4 53.93%
pdns/recursordist/pdns_recursor.cc 4 72.59%
Totals Coverage Status
Change from base Build 13115942971: 0.01%
Covered Lines: 127906
Relevant Lines: 166554

💛 - Coveralls

@Habbie
Copy link
Member

Habbie commented Feb 3, 2025

if I recall correctly, the "S" case should look for SRV, and then follow those through to get to A/AAAA

@miodvallat
Copy link
Contributor Author

if I recall correctly, the "S" case should look for SRV, and then follow those through to get to A/AAAA

That makes perfect sense.

How about this new, uglier, logic?

pdns/packethandler.cc Show resolved Hide resolved
regression-tests/tests/naptr/expected_result Show resolved Hide resolved
@Habbie
Copy link
Member

Habbie commented Feb 3, 2025

How about this new, uglier, logic?

(besides review comments), not bad! If the process was recursive, we could just push <name,SRV> there, but it's not.

Copy link
Member

@Habbie Habbie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one open question for somebody to answer. If the answer to the cache question is good, this is approved.

else if (flags.find('s') != string::npos) {
content = naptrContent->getReplacement();
DLOG(g_log<<Logger::Debug<<"adding NAPTR replacement 's'="<<content<<endl);
B.lookup(QType(QType::SRV), content, d_sd.domain_id, &p);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

somebody (you? me?) needs to check if this is a cache hit if the previous query for the name was QType::ANY

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding a quick accessor to the d_cached überfield shows that, apart from the first time this code path is hit, the lookup is found in cache. That's probably not precise enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If a perform an ANY query to the target of SRV (_double._tcp.dc.test.com) before the NAPTR query of ensm, all SRV lookups are found in cache. Does this answer your question?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants