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.

Sign up for GitHub

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

Jump to bottom

pdnsutil {add-record,delete-rrset}: Don't append ZONE if NAME ends with . #14984

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

pdnsutil {add-record,delete-rrset}: Don't append ZONE if NAME ends with . #14984

wants to merge 3 commits into from
+16 −11

Conversation

ukleinek
Copy link
Contributor

If a NAME ends with a . it is to be understood as an absolute name and appending the zone is not intuitive then.

Closes: #8595

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)

@Habbie Habbie added this to the auth-5 milestone Dec 17, 2024
zeha
zeha reviewed Dec 17, 2024
View reviewed changes
pdns/pdnsutil.cc Outdated Show resolved Hide resolved
@coveralls
Copy link

coveralls commented Dec 17, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 12399833537

Details

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • 72 unchanged lines in 16 files lost coverage.
  • Overall coverage decreased (-0.02%) to 64.802%
Files with Coverage Reduction New Missed Lines %
pdns/packethandler.cc 1 72.35%
pdns/backends/gsql/gsqlbackend.hh 1 97.71%
pdns/dnsdistdist/dnsdist.hh 1 84.24%
modules/godbcbackend/sodbc.cc 2 70.8%
pdns/recursordist/aggressive_nsec.cc 2 66.32%
pdns/dnsdistdist/dnscrypt.cc 2 66.71%
pdns/iputils.cc 3 55.91%
pdns/rcpgenerator.cc 3 90.37%
modules/gpgsqlbackend/spgsql.cc 3 67.94%
pdns/ssqlite3.cc 5 66.77%
Totals Coverage Status
Change from base Build 12373746857: -0.02%
Covered Lines: 126089
Relevant Lines: 163760
💛 - Coveralls

@ukleinek ukleinek force-pushed the pdnsutil-absolute-names branch from 0429faf to a878c70 Compare December 18, 2024 09:07
@ukleinek
Copy link
Contributor Author

Pushed with using isCanonical(). While working on this change I found that this function open-codes boost::ends_with() and "fixed" that accordingly. Not sure this is benefical, so feel free to only take the first commit.

@ukleinek
pdnsutil add-record: Specify how to add a record to the apex
2a1e55f 
Fix the respective calls in a test and the docs according to the now
explicit recommendation.

Using NAME=@ is the condition that is explicitly treated for that
purpose in the code. Currently NAME='' and NAME=. have the same effect.
@ukleinek ukleinek force-pushed the pdnsutil-absolute-names branch from a878c70 to 5b56440 Compare December 18, 2024 11:32
@ukleinek ukleinek mentioned this pull request Dec 18, 2024
Open
@mind04
Copy link
Contributor

mind04 commented Dec 18, 2024

Add and delete record in pdnsutil are already somewhat special for pdns with the relative names. I'm not sure we should make them even more special by adding canonical logic.

If we touch this, I prefer switching to full names here like the rest of pdns and drop all the fiddling with apex canonical and/or relative names.

@ukleinek
Copy link
Contributor Author

The failing check is clang-tidy which wants me to use { } around the if bodies:
statement should be inside braces (readability-braces-around-statements - Level=Warning)
The style I picked matches the surrounding code. Looking again there is a big mixure, some if statements use braces for one line bodies others don't. There are even mixed ones and inconsistencies if the else starts on a new line or after the closing brace. shrug

@ukleinek
pdnsutil {add-record,delete-rrset}: Don't append ZONE if NAME ends wi…
ff14c7f 
…th .

If a NAME ends with a . it is to be understood as an absolute name and
appending the zone is not intuitive then.

Note this changes behaviour for calls like:

	pdnsutil --config-dir=configs/auth add-record example.net . NS 1.2.3.4

which added the NS record to the zone's apex before and is likely an
error now.

Closes: PowerDNS#8595
@ukleinek
misc: Use boost::ends_with() in isCanonical instead of open-coding
2042465 
boost:ends_with(qname, ".") behaves exactly as isCanonical(qname)
should. So use the first to implement the latter.
@ukleinek ukleinek force-pushed the pdnsutil-absolute-names branch from 5b56440 to 2042465 Compare December 18, 2024 19:18
miodvallat
miodvallat approved these changes Jan 10, 2025
View reviewed changes
Copy link
Contributor

@miodvallat miodvallat left a comment

Choose a reason for hiding this comment

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

LGTM

@Habbie
Copy link
Member

Habbie commented Jan 10, 2025 •
edited
Loading

If we touch this, I prefer switching to full names here like the rest of pdns and drop all the fiddling with apex canonical and/or relative names.

I have to say I like this, except for breaking compatibility. But I agree the old form confuses people (including myself) all the time. And it in fact simplifies the code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zeha zeha zeha left review comments

@miodvallat miodvallat miodvallat approved these changes

Assignees
No one assigned
Labels
auth enhancement
Projects
None yet
Milestone
auth-5
Development

Successfully merging this pull request may close these issues.

pdnsutil add-record (A) always adds zone to specified value, even when trailing dot is specified in value
6 participants
@ukleinek @coveralls @mind04 @Habbie @zeha @miodvallat