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

different TTLs for a RRSet prevent zone loading #396

Open
and0x000 opened this issue Oct 25, 2024 · 6 comments · May be fixed by #403
Open

different TTLs for a RRSet prevent zone loading #396

and0x000 opened this issue Oct 25, 2024 · 6 comments · May be fixed by #403

Comments

@and0x000
Copy link

different TTLs in the same RRset prevent a zone from being loaded in NSD 4.10.1

minimal reproducible example:

$ORIGIN example.com.
$TTL 86400

@      IN SOA   ns1.example.com. dns.example.com. 2024102468 86400 10800 3600000 3600
@      IN NS    ns1.example.com.
@      IN NS    ns1.example.com.

@  1   IN TXT   foo
@  2   IN TXT   bar

result:

~ # dig example.com @localhost

; <<>> DiG 9.18.28-1~deb12u2-Debian <<>> example.com @localhost
;; global options: +cmd
;; Got answer:
;; ->>HEADER<<- opcode: QUERY, status: SERVFAIL, id: 59043
;; flags: qr rd; QUERY: 1, ANSWER: 0, AUTHORITY: 0, ADDITIONAL: 1
;; WARNING: recursion requested but not available

;; OPT PSEUDOSECTION:
; EDNS: version: 0, flags:; udp: 4096
; EDE: 14 (Not Ready): (Zone is configured but not loaded)
;; QUESTION SECTION:
;example.com.			IN	A

;; Query time: 0 msec
;; SERVER: 127.0.0.1#53(localhost) (UDP)
;; WHEN: Fri Oct 25 12:10:31 CEST 2024
;; MSG SIZE  rcvd: 79

Note the ; EDE: 14 (Not Ready): (Zone is configured but not loaded).

Log message:

2024-10-25T12:10:23.614609+02:00 test-dns nsd[2253841]: control cmd:  addzone example.com primary
2024-10-25T12:10:23.616598+02:00 test-dns nsd[2253843]: primary_zonefiles/e/example.com:9: example.com. TTL 2 does not match TTL 1 of TXT RRset
2024-10-25T12:10:23.616714+02:00 test-dns nsd[2253843]: zone example.com file primary_zonefiles/e/example.com read with 1 errors
2024-10-25T12:12:05.048216+02:00 test-dns nsd[2253841]: control cmd:  zonestatus example.com

Yet, zonestatus doesn't hint an issue.

# nsd-control zonestatus example.com
zone:	example.com
	pattern: primary
	state: primary

Personally I'd see this as a regression over the behavior from 4.9.1.
In 4.9.1 the warning messages were present as well. However, NSD did load the zone file and didn't resort to SERVFAIL responses.
@he32
Copy link

he32 commented Oct 25, 2024 via email

@and0x000
Copy link
Author

and0x000 commented Oct 25, 2024 via email

@he32
Copy link

he32 commented Oct 25, 2024 via email

@and0x000
Copy link
Author

I must admit that I do not quite understand what change in behaviour you would like to have.

To be honest, I'm not sure neither. But I'm rather unhappy with the way this change was introduced.

Prior to 4.10 NSD served all the records with their respective individual TTLs. Personally I'd prefer this behavior to remain (albeit violating the RFC) until their is a new major release.

I presume the change came with the introduction of simdzone. I don't know to what degree semantic versioning is considered in this project. In my opinion, the change from serving with the individual TTLs to configuring but not loading a zone is a change, that would mandate a major version increase.

E.g. should the zone load, but with the offending RRSet automatically converted so that all records in the set get the lower of all the different TTLs?

If there are problems with implementing the pre-4.10-behavior in the new simdzone parsing, this sounds like a reasonable middle ground to me while sticking with the 4.x major release.

@k0ekk0ek
Copy link
Contributor

Hi @and0x000. Thanks for reporting! This is a semantic error and previously always reported as a warning rather than an error. Most semantic errors in process_rr where/are either reported as an error or warning based on whether the server is a secondary or a primary. In itself, it has nothing to do with simdzone. However, the function processing each RR previously contained an if {} else {} with separate log statements for primary vs. secondary. When I included simdzone I simply used the same pattern for this one too. So, honest mistake on my part. As I believe @wcawijngaards prefers keeping this one a semantic error, I'll update the code to always log this as a warning.

k0ekk0ek added a commit to k0ekk0ek/nsd that referenced this issue Oct 28, 2024
@k0ekk0ek
Treat a mismatch in RRset TTLs as a warning
5758e2e 
Fixes NLnetLabs#396.
k0ekk0ek added a commit to k0ekk0ek/nsd that referenced this issue Oct 28, 2024
@k0ekk0ek
Treat a mismatch in RRset TTLs as a warning
a5ddeca 
Fixes NLnetLabs#396.
@k0ekk0ek k0ekk0ek linked a pull request Oct 28, 2024 that will close this issue
Open
@and0x000
Copy link
Author

Thank you for considering this report.

If I read this correctly, this adds a warning log message but doesn't change the behavior? i.e. the zone still won't be loaded if there is a TTL mismatch in the RRset?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Treat a mismatch in RRset TTLs as a warning k0ekk0ek/nsd
3 participants
@k0ekk0ek @he32 @and0x000