Support static DNS mappings using Kea DHCP. #7362
Conversation
|
I've added 14cc9a1c2f and b53fe7c1d9 to avoid tainting private I'm not against this, but this needs careful crafting because the current PR is (in light of the impending breakage it would have created above) not complete. Here are a few thoughts:
Cheers, |
So the action is not needed for the plugin function to work? Where is the
This is now checked.
I have removed the useless loop. |
reitermarkus
force-pushed
the
kea-static-mapping
branch
from
a574b43
to
86cb989
Compare
|
@fichtner, can you have another look here? |
src/etc/inc/plugins.inc.d/kea.inc
Outdated
|
|
||
| $subnet_node = $keav4->getNodeByReference("subnets.subnet4.{$reservation->subnet}"); | ||
|
|
||
| $domain = $domain_fallback; |
There was a problem hiding this comment.
we should maybe considering removing domain_fallback or at least not implementing it for kea.. it was only a workaround for unbound when it wasn't capable of setting the right domain
There was a problem hiding this comment.
So it should not use the system domain as fallback?
https://github.com/opnsense/core/blob/24.7.a/src/etc/inc/plugins.inc.d/unbound.inc#L586
Or is this set automatically?
src/etc/inc/plugins.inc.d/kea.inc
Outdated
| $entry = [ | ||
| 'descr' => $description, | ||
| 'domain' => $domain, | ||
| 'hostname' => $hostname, | ||
| 'ipaddr' => $ip_address, | ||
| ]; |
There was a problem hiding this comment.
Where to get the interface from? Or just set it to null?
src/etc/inc/plugins.inc.d/kea.inc
Outdated
| $hostname = (string)$reservation->hostname; | ||
| if (empty($hostname)) { | ||
| $hostname = null; | ||
| } | ||
| $description = (string)$reservation->description; | ||
| if (empty($description)) { | ||
| $description = null; | ||
| } |
There was a problem hiding this comment.
this feels a bit overcomplicated, empty strings should be fine. you end up thowing a PHP warning now or later because empty() should be the first sanity check then which can mute the warning. Here it does nothing.
There was a problem hiding this comment.
you end up thowing a PHP warning now or later because empty() should be the first sanity check then which can mute the warning
I'm not quite following, should the empty check be removed completely or moved somewhere else?
|
I would love to see it in the next release :) |
AdSchellevis
force-pushed
the
master
branch
2 times, most recently
from
be6c1bb
to
030ed19
Compare
|
Merged, thanks! |
|
Just for myself: that means it will be part of the NEXT release, right? |
|
Next development version. Final release in 24.1.9 most likely. |
|
opnsense-patch 139a3ad Works well :). Thanks. Next stop: dynamic leases into unbound 👯♂️ |
|
It's not the full patching. I'd say it works but I'd rather have confirmation on latest development state because there it works without the obvious issues it introduces into the wider system. |
|
Yeah, all fine, for "me" it seems to work fine. And i can always revert if in case of any issues |
|
Well, the point was that if the actual development state is tested it helps us more to ship the feature than cherry-picking the PR commit because mistakes can happen. I know it works as a concept because it was reviewed and merged, but it doesn't reflect the full state of changes necessary. That's all. |
|
Ah, okay I understood. Might consider it, but it might affect the woman acceptance factor 😅 |
|
We all have our reasons, no problem. I just wanted to get that point across. In any case kudos for using opnsense-patch to subvert the WAF. ;) |
Closes #7307.