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

Fix nsupdate when updating NS record #5112

Merged
merged 6 commits into from Aug 20, 2022

Conversation

lungj
Copy link
Contributor

@lungj lungj commented Aug 12, 2022

SUMMARY

Fixed a bug where one cannot update an existing NS record.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

nsupdate

ADDITIONAL INFORMATION

In order to determine if any changes occurred, the nsupdate component queries the TTL of the resulting record.

When querying A, AAAA, SOA, or CNAME records, one gets something like this in the ANSWER section:

;; ANSWER SECTION:
github.com.		60	IN	A	140.82.113.3

Here, we can see a TTL of 60 seconds.

However, an NS record and its TTL appears in the AUTHORITY section:

;; AUTHORITY SECTION:
github.com.		900	IN	NS	dns1.p08.nsone.net.
github.com.		900	IN	NS	dns2.p08.nsone.net.
github.com.		900	IN	NS	dns3.p08.nsone.net.
github.com.		900	IN	NS	dns4.p08.nsone.net.

The nsupdate component only searches for TTL in the ANSWER section. Thus, when updating NS records, the nsupdate component reads the wrong section for a TTL. When no ANSWER section records exist, the nsupdate component outright fails due to an index out of bounds error.

This bug fix reads the AUTHORITY section when an NS request is made.

@ansibullbot
Copy link
Collaborator

@ansibullbot ansibullbot added WIP Work in progress bug This issue/PR relates to a bug module module net_tools plugins plugin (any type) labels Aug 12, 2022
@lungj lungj marked this pull request as ready for review August 12, 2022 14:17
@ansibullbot ansibullbot removed the WIP Work in progress label Aug 12, 2022
@felixfontein felixfontein added check-before-release PR will be looked at again shortly before release and merged if possible. backport-4 labels Aug 13, 2022
@felixfontein
Copy link
Collaborator

Hmm, depending on the name server and the domain queried, it also shows up in the answer section. How about first looking in the answer section, and if it's not there, then use the authority section?

Co-authored-by: Felix Fontein <felix@fontein.de>
@lungj
Copy link
Contributor Author

lungj commented Aug 13, 2022

Hmm, depending on the name server and the domain queried, it also shows up in the answer section. How about first looking in the answer section, and if it's not there, then use the authority section?

Thanks for the review again! The TTLs do not need to match between ANSWER and AUTHORITY. In fact, in many cases, A and NS records probably differ in TTL, as the actual (abridged) github.com records do. Using a fallback could lead to an incorrect return value from this function.

The nsupdate component compares the desired TTL with the TTL from the DNS query to see if they match; when trying to set an NS record, we should only be looking at the NS records' TTLs. Otherwise, whenever we call this module on an NS record, we are likely to have it report a change because the desired TTL in the AUTHORITY section will not match the TTL of anything in the ANSWER section. For example, if we have this:

;; ANSWER SECTION:
github.com.		60	IN	A	140.82.113.3
;; AUTHORITY SECTION:
github.com.		900	IN	NS	dns1.p08.nsone.net.

and we call this:

- community.general.nsupdate:
    key_name: "nsupdate"
    key_secret: "+bFQtBCta7j2vWkjPkAFtgA=="
    server: "127.0.0.1"
    zone: "github.com"
    record: "github.com."
    type: "NS"
    value:
      - "dns1.p08.nsone.net"
    ttl: 900

if we use the ANSWER value and then fall back to AUTHORITY, this component would say that 60 != 900 (line 431) and thus that there is a change -- when, in fact, no change is requested.

Copy link
Collaborator

@russoz russoz left a comment

Choose a reason for hiding this comment

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

Other than the small adjustment in the chglog fragment, LGTM.

@felixfontein
Copy link
Collaborator

Hmm, depending on the name server and the domain queried, it also shows up in the answer section. How about first looking in the answer section, and if it's not there, then use the authority section?

Thanks for the review again! The TTLs do not need to match between ANSWER and AUTHORITY. In fact, in many cases, A and NS records probably differ in TTL, as the actual (abridged) github.com records do. Using a fallback could lead to an incorrect return value from this function.

I was not talking about the TTL of A records in the answer, but of the NS records in the answer. If you query for a domain's NS records, you should get the correct TTL in the answer to that request.

For example dig -t NS github.com gives me

;; ANSWER SECTION:
github.com.		3600	IN	NS	dns3.p08.nsone.net.
github.com.		3600	IN	NS	ns-520.awsdns-01.net.
[... several more ...]

;; AUTHORITY SECTION:
github.com.		3600	IN	NS	ns-520.awsdns-01.net.
github.com.		3600	IN	NS	dns1.p08.nsone.net.
[... several more ...]

After all the code does query for records of the type it's interested in: https://github.com/ansible-collections/community.general/pull/5112/files#diff-b86a495028fd26a8d2fbf68b85320b9792958611523d19ad00b12b429371193fR413

@lungj
Copy link
Contributor Author

lungj commented Aug 14, 2022

Hmm, depending on the name server and the domain queried, it also shows up in the answer section. How about first looking in the answer section, and if it's not there, then use the authority section?

Thanks for the review again! The TTLs do not need to match between ANSWER and AUTHORITY. In fact, in many cases, A and NS records probably differ in TTL, as the actual (abridged) github.com records do. Using a fallback could lead to an incorrect return value from this function.

I was not talking about the TTL of A records in the answer, but of the NS records in the answer. If you query for a domain's NS records, you should get the correct TTL in the answer to that request.

For example dig -t NS github.com gives me

;; ANSWER SECTION:
github.com.		3600	IN	NS	dns3.p08.nsone.net.
github.com.		3600	IN	NS	ns-520.awsdns-01.net.
[... several more ...]

;; AUTHORITY SECTION:
github.com.		3600	IN	NS	ns-520.awsdns-01.net.
github.com.		3600	IN	NS	dns1.p08.nsone.net.
[... several more ...]

After all the code does query for records of the type it's interested in: https://github.com/ansible-collections/community.general/pull/5112/files#diff-b86a495028fd26a8d2fbf68b85320b9792958611523d19ad00b12b429371193fR413

Thanks for clarifying! I've made the patch sinceI'm not versed enough in all the differences in behaviour of different DNS servers. One of the other use cases for the nsupdate module is for updating a zone that contains subdomains so we're interested in results of dig -t NS github.com @l.gtld-servers.net. (assuming we're the administrators of the .com TLD), but the fallback to AUTHORITY you suggested works in this situation since there is no ANSWER section given.

Co-authored-by: Felix Fontein <felix@fontein.de>
Copy link
Collaborator

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@ansibullbot

This comment was marked as outdated.

@ansibullbot ansibullbot added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR label Aug 14, 2022
Co-authored-by: Felix Fontein <felix@fontein.de>
@ansibullbot ansibullbot removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR label Aug 15, 2022
This was referenced Nov 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue/PR relates to a bug module module net_tools plugins plugin (any type)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants