auroraresearchlab / netbox-dns

Netbox Dns is a netbox plugin for managing zone, nameserver and record inventory.
MIT License
207 stars 19 forks source link

Having a record with a TTL value of 'None' can cause downstream problems #264

Closed wolfspyre closed 1 year ago

wolfspyre commented 1 year ago

in #242 there were hardcoded TTLs...

243 "fixed" that by setting the value to None...

The problem I am running into with this is that when I've created zonefiles from the records, I'll get records with a value of None for the TTL... coredns doesn't like that.

Granted, this could certainly be addressed in the script I'm using to generate zonefiles.

A potentially less problematic way ( not saying a BETTER way ) would be to allow per (install?|view?|nameserver?|zone?) 'defaults' which populate the hierarchically inferior records... Such that the value is not None. then perhaps have an enum field describing where that record's value was last divined from. (LOCKED/explicit/zone/nameserver/view/install/random/magic 8 ball... etc.)

This would allow for inheritance logic to apply where appropriate, while still having the record be accurate regardless of where the value was set....

I can see a legitimate argument for either way, Having a null value in the record saves database space, and provides a simple logic path to assert inheritance should take precedent.... at the cost of an incomplete record.

having an extra field declaring each record's TTL's origin consumes more space, and likely more CPU overhead in maintenance, but the records are accurate, which has an upside from the standpoint of integrations as the record is accurate from the perspective of an external inquiry without needing to know how to interpret a null value for a field that should have a numerical value....

¯_(ツ)_/¯

peteeckel commented 1 year ago

Hi @wolfspyre,

this is interesting as we enter the 'implementation specifics' area again.

First, we must differentiate between the records stored in zone data and the records actually transmitted on the wire between DNS servers, resolvers and clients.

The latter always contain a TTL value, but the former do not necessarily need one, as specified in RFC #235 was about being able to define RRs without a TTL in zone files. For NetBox DNS, as it does not implement a DNS server itself, we need to look at the former as it is meant to provide input for authoritative DNS servers.

The relevant definition for this can be found in in RFC 1035, Section 5.1:

<rr> contents take one of the following forms:
    [<TTL>] [<class>] <type> <RDATA>
    [<class>] [<TTL>] <type> <RDATA>

As you can see, the TTL field is optional. So #235 is correct, and having a TTL with null value is perfectly legal in zone data.

Now for CoreDNS. I had a look at the documentation, and the very first example I found for a zone file was this. Look at the two RRs for www - no TTL. So either CoreDNS' documentation is wrong (in which case CoreDNS should barf on the RRs in not containing a TTL, which would in turn make it non-RFC-compliant), or CoreDNS does not have a problem with RRs without a TTL, at least not with the file backend.

As for the Redis backend provided as a plugin, the documentation of the configuration syntax documents a ttloption:

ttl default ttl for dns records, 300 if not provided

So the Redis backend has the option of providing a default TTL for RRs (and even a default for the default), which in turn means they are not required, otherwise a default would not make sense.

All in all, RRs with an unspecified TTL are correct as per RFC 1035, and TTLs are not required by CoreDNS either (which was to be expected given that CoreDNS should adhere to the RFC 1035/STD 13), so having the option to return null for a record TTL is really required. It was my fault to always set a TTL before #235, and yours to rely on that mistake :-)

If it is really necessary, TTL for RRs can be determined in various ways. There is the 'Default TTL' field on zone level which can be used to provide RRs with a TTL when exporting to a DNS server that actually requires a TTL. TTLs could also be set based on the ´MINIMUM´ SOA field (the correct algorithm is to use the maximum of the ´MINIMUM´ SOA field and the TTL specified explicitly or implicitly for a record.

tl;dr

  1. It's not a bug, it's a feature
  2. There already is a (simple) defaulting mechanism if you require one

When you have a good use case for a more elaborate defaulting mechanism please add it to your list of feature ideas so we have a central place to discuss it.

peteeckel commented 1 year ago

@wolfspyre: On second thought ... is your problem that you get a textual None, none, null or whatever as the TTL? That would indicate a problem with your script - I know because I had the same problem after implementing #235.

For Jinja 2/Ansible, the solution is to use something like

{{ (_record.ttl|string if _record.ttl is not none else '').ljust(8) }} 

instead of

{{ (_record.ttl|string).ljust(8) }} 

in your template so the TTL is replaced by an empty string if the value is absent.

The actual value in the database is not None. There just isn't a value, and depending on how you access the data via the API absent values is rendered in different ways, e.g. None in Python.

wolfspyre commented 1 year ago

@peteeckel

Yeah yer right... it's not required and netbox-dns isn't expected to maintain it...

I was thinking about this from the perspective of other things (like ansible) slurping a record from netbox as the source of truth and barfing because missing expectations....

Wholly agree with you that it's more integration-implementation-specific nuance than pedantic correctness. (feature not bug)

And thanks for pushing back on this... I don't believe my ideas are fantastic... I don't USUALLY think they're either terrible, but I do value the discussion around implementation points that may or may not be worth the squeeze. Admittedly, git issues aren't REALLY the best medium for these kinds of conversations... but I was thinking that raising this issue might have the positive upside of

oh hey someone else ran into this too and....fixed it by....

and yes, I'd already fixed in my^H^Hthe script you so generously shared as a starting point.... pretty much the same as you did there except I used the numerical 3600... Yer way's prolly better ... but.... ¯\_(ツ)_/¯

regardless.... hope yer day's going awesomely! thanks again!

❤️🐺W

peteeckel commented 1 year ago

Hi @wolfspyre, thanks!

While GitHub issues are not the right place to discuss this kind of abstract and less-concise implementation ideas, I'd gladly point to discussions - if there were more participation (ha, ha) they'd be a good place to incubate ideas.

Glad my afterthought worked for you.