-
Notifications
You must be signed in to change notification settings - Fork 890
Kea: Add DHCP-DDNS Support #9401
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
base: master
Are you sure you want to change the base?
Conversation
src/opnsense/mvc/app/controllers/OPNsense/Kea/Api/DhcpddnsController.php
Outdated
Show resolved
Hide resolved
| <?php | ||
|
|
||
| /* | ||
| * Copyright (C) 2025 Deciso B.V. |
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.
I bet someone else wrote this code actually :)
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.
Eeeh, I just copied the copyright headers, not quite sure what do to here...?
EDIT: nevermind, looked at the COPYRIGHT file and some other code files, I'll update it.
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.
Do I need to update the COPYRIGHT file too?
|
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, |
src/opnsense/mvc/app/controllers/OPNsense/Kea/Api/DhcpddnsController.php
Outdated
Show resolved
Hide resolved
a28350d to
a956592
Compare
|
Rebased on latest state of master |
d08c1bb to
6dac93e
Compare
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. |
src/opnsense/mvc/app/controllers/OPNsense/Kea/forms/generalSettings6.xml
Outdated
Show resolved
Hide resolved
src/opnsense/mvc/app/controllers/OPNsense/Kea/forms/dialogDdnsDnsServer.xml
Outdated
Show resolved
Hide resolved
Monviech
left a comment
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.
I would suggest reducing the scope of the first version of the ddns feature to make it essier to review and test with less turning knobs.
There seem too many relations that do not make immediate sense (to me) in this design.
src/opnsense/mvc/app/controllers/OPNsense/Kea/forms/dialogSubnet4.xml
Outdated
Show resolved
Hide resolved
src/opnsense/mvc/app/controllers/OPNsense/Kea/forms/dialogSubnet4.xml
Outdated
Show resolved
Hide resolved
src/opnsense/mvc/app/controllers/OPNsense/Kea/forms/dialogSubnet4.xml
Outdated
Show resolved
Hide resolved
src/opnsense/mvc/app/controllers/OPNsense/Kea/forms/dialogSubnet4.xml
Outdated
Show resolved
Hide resolved
src/opnsense/mvc/app/controllers/OPNsense/Kea/forms/dialogSubnet4.xml
Outdated
Show resolved
Hide resolved
src/opnsense/mvc/app/controllers/OPNsense/Kea/forms/dialogSubnet4.xml
Outdated
Show resolved
Hide resolved
|
I've made several changes addressing further feedback |
Monviech
left a comment
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.
Now for the next round, just some general coding things that can be streamlined and or changed.
I don't know if everything I suggested is correct, but extrapolate from this.
| if (!$validateFullModel && !$subnet->isFieldChanged()) { | ||
| continue; | ||
| } | ||
| $send_updates = !empty((string)$subnet->ddns_options->send_updates); |
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.
Why is this important? The input should always be correct regardless of if this is enabled or not?
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.
This pattern was used for the reservations validation above. So this was, more or less, copied from there.
I think the intention is to ensure that should invalid data be in the model, the validation doesn't fail when the user is saving another dialog with no way to correct the existing values...?
| if (!$validateFullModel && !$domain->isFieldChanged()) { | ||
| continue; | ||
| } | ||
| $name = trim((string)$domain->name); |
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.
same as earlier
|
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 :) |
src/opnsense/mvc/app/controllers/OPNsense/Kea/forms/dialogDdnsForwardDomain.xml
Outdated
Show resolved
Hide resolved
src/opnsense/mvc/app/controllers/OPNsense/Kea/forms/dialogDdnsForwardDomain.xml
Outdated
Show resolved
Hide resolved
|
|
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? |
…s in manual_config mode.
… services for enable ddns related fields.
…it when any subnet enables ddns updates
…_char_set, hostname_char_replacement options because they are very unlikely to be needed by almost everyone
…consistent with the tab name.
…nt_name options These options cause kea to generate a DDNS name of prefix-(ip address text) as the hostname. This is of very questionable usefulness, because if you're using that dns record, you already know what the IP address is.
…wer accessor methods.
63bbd38 to
c442793
Compare
|
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. |
|
I would like to leave a small note of support from a user perspective. In my setup, OPNsense handles routing and DHCP while DNS is provided externally (Technitium DNS). ISC DHCP's DDNS support integrates well with that arrangement, and it has been reliable for a long time. With the announced EOL of ISC DHCP, I've tried to switch to Kea, but once I discoverd it doesn't support DDNS I had to go back to ISC. DNS registration for DHCP lease in an important feature in my environment, and this PR seems to address that gap. It would make the migration path much easier for setups like mine. I completely understand the need to keep things maintainable and not overload the UI. Having this functionality available, even in an advanced or optional section, would already be very helpful for users who rely on DDNS. Thank you to everyone involved for the work so far - I appreciate the discussion and hope this can move forward at whatever pace feels right for the project. |


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