opnsense
Kea: Add DHCP-DDNS Support
This PR adds full support for enabling and configuring the Kea DHCP-DDNS server.
This will enable Kea to perform Dynamic DNS updates over RFC2136 for both IPv4 and IPv6.
Are there any docs changes that should be done for this too?
Closes #9359
I made a few comments to move this along. In general this looks promising although I have to say test coverage will be tricky with all the options already present here.
Thanks, Franco
I made a few comments to move this along. In general this looks promising although I have to say test coverage will be tricky with all the options already present here.
Thanks, Franco
Anything I can do to help on that?
(this is just a comment, do not act upon it (yet))
Reading the scope of the code it feels like theres a lot of feature bloat in there. Couldn't this be stripped down to a more minimal viable thingy with only whats essential for the forward and reverse zones? Eg. these regex and replacement fields are most likely not needed by 99% of the users.
I also feel like theres too much "is this service enabled" validations going on, that could create circular dependencies, I would also cut this down to essentials (aka only validate what would hard crash KEA with an error).
I know that this is rather complete, but adding too much initially will add all the bloat that is unknown if anybody actually needs it, but then its around for good. Adding is easy, removing is very hard, so adding less and only adding more upon request is the better route to go imo.
I have the feeling if we would trim this down to the essentials it would fit at least 90% of usecases and safe quite some potentially dead code.
I suppose the hostname regex/replacement fields aren't likely to be needed by most users yeah. I'm not actually sure under what scenario would you want to uh, regex characters out of a client hostname, given that I think hostnames should already be valid as a DNS record anyway (?).
Although I don't think the same can be said of any other fields right now though. Except maybe the override no-update / override client update options, I'm not sure what options most DHCP clients can/will set with regards to those preference flags in their DHCP requests.
And I suppose if the criteria for "should this be validated?" is "will this crash kea?" then yeah, the service enabled validations are not needed at all. The worst that will happen is kea dhcp4/dhcp6 just logging warnings every time it tries to update if the ddns service isn't running.
Made several changes in-line with the latest round of suggestions, and also went over the DDNS model php file to change it to use the methods from BaseField as suggested.
There really is a lot of inconsistency in the kea dhcp4/dhcp6 code though, there's lots of field casting, and some use of isEmpty().
It seems like a git blame on BaseField says that at least the isEmpty() and getValue() methods were added somewhat recently, and parts of the kea code predates their existence?
Hello, yes some things have been added recently, that means if we use them in new code additions right away we dont have to clean up. Thanks I'll look at this tomorrow :)
Without the intent to totally derail this PR, here is the whole of what ISC had:
DHCPv4
DHCPv6
It was configured per range.
Why do I need more than that now in KEA, if the above was all that was ever needed without known complains about lacking in functionality?
I still have the feeling the current PR is a bit overdesigned, even though it seems to be quite generic. Most could be implicitly generated just from a few new fields in the ranges.
Challenge my assumption if you want to, this does not necessarily mean that ISC was designed correctly around this feature, but it has been awfully quiet around it since years (and I read a lot of forum, reddit, github issues).
Without the intent to totally derail this PR, here is the whole of what ISC had: DHCPv4
DHCPv6
It was configured per range.
Why do I need more than that now in KEA, if the above was all that was ever needed without known complains about lacking in functionality?
I still have the feeling the current PR is a bit overdesigned, even though it seems to be quite generic. Most could be implicitly generated just from a few new fields in the ranges.
Challenge my assumption if you want to, this does not necessarily mean that ISC was designed correctly around this feature, but it has been awfully quiet around it since years (and I read a lot of forum, reddit, github issues).
-
Not needing to adjust every single subnet (x2 for both dhcp4 and 6) if you change your DNS servers / rotate TSIG keys is great. This is exactly what I had to do for ISC DHCP. Not something you do often, but its definitely less effort to configure when you don't need to adjust the DDNS server options on every subnet.
-
Also, DDNS zone and subnet domain is not necessarily always the same, I believe the official RFC2136 specification has DDNS updates having a zone and one or more RRSets, implying that you can have a.b.x.y.z FQDN updated into x.y.z zone. Some DNS servers (Like earlier versions of Technitium DNS) would very strictly follow the zone specified in the request and fail to process the update if you sent an update with b.x.y.z as the zone to update.
-
Other then the split to separate out the DDNS service configs, some of which I've already addressed the use cases for:
-
~~hostname generation/replacement ( to replace badly behaved IoT clients that all send the -same- or no hostname, like a bunch of smart plugs I have all send p110 as the hostname)~~
- Actually taking a second look at the kea docs, these look quite useless since it generates a client name using (prefix)-(address text). This is silly and serves no real purpose beyond just giving it a rDNS PTR record. It's not even a stable generated name using the DUID or MAC address since it's tied to the lease, and if you set reservations, you can override the hostname there.
- I'm not even sure why would you want to clutter your DNS Zone with prefix-ip-address.domain records because if you know the domain you already know the IP address.
- It seems like the only way to have somewhat useful hostname generation for this use case is to use their hook-ddns-tuning which provides a hostname-expr parameter to do this. But this is most likely beyond the scope of this PR.
-
conflict resolution - this controls how kea uses the DHCID records, and is basically mandatory to tweak if you want to point dhcpv4 and dhcpv6 at the same zone. Because many clients do not send their DUID as a client identifier in dhcp4 requests. So they get different DHCIDs between dhcp4 and dhcp6 resulting in either dhcp4 or dhcp6 not updating DDNS because of the detected conflict. This -never- worked for me in ISC dhcp6, likely because of the conflict detection.
-
the qualifying name is just that, a qualifying name. Kea (and ISC DHCP) does actually respect the FQDN for clients that sends that. Naming the fields like this makes it more obvious what the behaviour is.
-
Its also handy to have kea able to send DDNS updates on lease renew instead of on just lease creation. Mostly when tweaking DDNS settings.
- I know there's the manual config option, but it's really suboptimal because now you can't rely on config backup and XMLRPC HA synchronization for Kea.
The added convenience inflates this feature though. I think some inconvenience is okay if it could cut down most of the code.
Essentially this is a set and forget feature for most so I do not see the need for so many new grids that relate to another.
Though lets keep it like this for now and see what others have to say, overall this feature would have it far easier in the review process if the LOC footprint would be less. That would also reduce future maintainance load/regression risk if it has less turning knobs.
I've further removed the generated prefix and client name replacement options as these result in generating a hostname with the assigned IP address, which is, of questionable usefulness for most cases where you want the DHCP server to register clients into DNS.
What's the point of having a DNS entry where you need to know the IP to get it's DNS entry?
I still have a feeling that if we would put the "tsig key" and the "dns server" into the existing "Dhcpv4" and "Dhcpv6" pools model, we could cut half of the LOC footpring of this code. And we could generate the config without having these reference maps in php.
The config could be built from information of the v4 and v6 models from flat relations.
Sure it would be more inconvenient for the user (if they have to change the key or server entry in a few pools), but the tradeoff would be maintainability and a clear scope of the offered feature (its very easy to configure, and very hard to do a mistake that way).
Other than the inconvenience, the scope of the ISC feature has seemed to be just right for /most/ people (not all of them).
Are you also suggesting to move the forward and reverse zone definitions into there? Because if not you'd need to add even more code just for that, to match the subnets to the forward and reverse zones (which IMO, adds fragility and additional complexity, for some LOC reduction)
Also, you cannot strictly make any assumptions about what the forward and reverse zones in use are, I'm pretty sure I've already seen some other issues talking about classless rdns zones for ipv4.
Neither, again, is the assumption that the forward zone(s) are exactly one level below the hostname. (I.e. depending on dns server imementation, this may be required to be specified as an exact zone that exists.)
That being said, if there are even more objections, the server and tsig keys could be moved to the zone definitions. (Which actually more closely aligns with the kea config format)
But for now I'll leave it as it is.