-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Fix nsupdate when updating NS record #5112
Conversation
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>
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) The
and we call this:
if we use the ANSWER value and then fall back to AUTHORITY, this component would say that |
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.
Other than the small adjustment in the chglog fragment, LGTM.
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
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 |
Co-authored-by: Felix Fontein <felix@fontein.de>
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.
Looks good to me.
This comment was marked as outdated.
This comment was marked as outdated.
Co-authored-by: Felix Fontein <felix@fontein.de>
SUMMARY
Fixed a bug where one cannot update an existing NS record.
ISSUE TYPE
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
, orCNAME
records, one gets something like this in the ANSWER section:Here, we can see a TTL of 60 seconds.
However, an
NS
record and its TTL appears in the AUTHORITY section:The
nsupdate
component only searches for TTL in the ANSWER section. Thus, when updatingNS
records, thensupdate
component reads the wrong section for a TTL. When no ANSWER section records exist, thensupdate
component outright fails due to an index out of bounds error.This bug fix reads the AUTHORITY section when an NS request is made.