WARNING: THIS SITE IS A MIRROR OF GITHUB.COM / IT CANNOT LOGIN OR REGISTER ACCOUNTS / THE CONTENTS ARE PROVIDED AS-IS / THIS SITE ASSUMES NO RESPONSIBILITY FOR ANY DISPLAYED CONTENT OR LINKS / IF YOU FOUND SOMETHING MAY NOT GOOD FOR EVERYONE, CONTACT ADMIN AT ilovescratch@foxmail.com
Skip to content

Conversation

@wtoorop
Copy link
Member

@wtoorop wtoorop commented Oct 26, 2024

No description provided.

@huguei
Copy link

huguei commented Oct 27, 2024

Thanks Willem! Just two comments:

  • line 1820 of query.c: in the spirit of future maintainability, would it not be better to define a constant, for example “ZONEVERSION_SOA-SERIAL" with value 0 and referring to https://www.iana.org/assignments/dns-parameters/dns-parameters.xhtml#zoneversion-type-values , just in case new ones appear in the future?
  • in my patches on an older version of nsd, I added a “(RCODE(q->packet) == RCODE_OK)” condition to the option. At the moment I don't remember the reason for that test, but couldn't it be that we're returning zonerversion on some edge condition that shouldn't happen? Sorry, but I haven't compiled the code to do live compliance testing.

@wtoorop
Copy link
Member Author

wtoorop commented Oct 27, 2024

Thanks Willem! Just two comments:

Thanks for the prompt review @huguei

Good point! I'll add that shortly.

  • in my patches on an older version of nsd, I added a “(RCODE(q->packet) == RCODE_OK)” condition to the option. At the moment I don't remember the reason for that test, but couldn't it be that we're returning zonerversion on some edge condition that shouldn't happen?

But the last paragraph of Section 3.2 of RFC 9660, Responders states that ZONEVERSION should also be returned for NXDOMAIN and for SERVFAIL. So I reckoned that if nsd could find a zone for the question (i.e. the zone field in struct query is set) it can also return a ZONEVERSION for that zone regardless the kind of response. WDYT?

Sorry, but I haven't compiled the code to do live compliance testing.

I can do that for you and serve some zones on a droplet in digital ocean ;). Do you have a zone in mind (or several zones) for you to test?
I suppose we should capture those compliance tests in a test package, so you can review that script.

Currently only 1: The SOA-SERIAL type.
Copy link

@k0ekk0ek k0ekk0ek left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@wcawijngaards wcawijngaards left a comment

Choose a reason for hiding this comment

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

Looks like good code. Support for the edns option could be useful to disambiguate query results for what zone and serial number of that zone it is from.

@wtoorop wtoorop merged commit 77f7f12 into master Nov 7, 2024
wtoorop added a commit that referenced this pull request Nov 7, 2024
@wtoorop wtoorop deleted the features/zoneversion branch January 3, 2025 09:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants