T6773: RFC-2136 support for Kea DHCP4 server #4153
Conversation
abukharov
requested review from
dmbaturin,
sarthurdev,
zdc,
jestabro,
sever-sever and
fett0
|
👍 |
|
✅ No issues found in unused-imports check.. Please refer the workflow run |
|
Did a few cosmetic things. Please let me know what you think about generating that config, DNS servers list and the port 53001 for d2 and I'll get it fixed within a few days. |
Was able to figure it out. Should be fine now. |
abukharov
force-pushed
the
feature/dhcpv4-ddns
branch
from
899b0a4 to
8b6d4ae
Compare
|
Everything seems to be fixed now, all tests are green |
|
So I started productionising this setup, which meant setting up more sophisticated configurations for DHCP. And I quickly realised that this config language wasn't doing what was actually needed. The main problem is that in Kea it makes a difference if an attribute is set to false or not set at all. It's to do with how the DDNS settings cascade to nested scopes. Let's say you set I've re-implemented the behavioral settings in a way that they can be explicitly set to enabled or disabled, or left out/deleted from config, the latter will result in a removal of the attribute from the output config file. This way I got the whole thing to function properly, even though the language has become a bit more heavyweight. If you have any ideas about how to make it nicer and easier to understand for the end user, I'm open to suggestions. |
abukharov
force-pushed
the
feature/dhcpv4-ddns
branch
from
a307562 to
74800b3
Compare
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
|
@abukharov Can you resolve conflicts? |
|
Hey mate, sure, will try and get it done by the end of tomorrow. |
abukharov
force-pushed
the
feature/dhcpv4-ddns
branch
from
74800b3 to
c1bd755
Compare
sever-sever
requested review from
Copilot
and removed request for
fett0 and
nicolas-fort
There was a problem hiding this comment.
Copilot reviewed 4 out of 12 changed files in this pull request and generated 2 comments.
Files not reviewed (8)
- data/templates/dhcp-server/kea-dhcp-ddns.conf.j2: Language not supported
- data/templates/dhcp-server/kea-dhcp4.conf.j2: Language not supported
- interface-definitions/include/dhcp/ddns-dns-server.xml.i: Language not supported
- interface-definitions/include/dhcp/ddns-settings.xml.i: Language not supported
- interface-definitions/service_dhcp-server.xml.in: Language not supported
- smoketest/config-tests/basic-vyos: Language not supported
- smoketest/configs/basic-vyos: Language not supported
- src/etc/systemd/system/kea-dhcp-ddns-server.service.d/override.conf: Language not supported
Hey hey. Yes, unfortunately I can. This is what is happening on one of the devices. The situation is that globally we send ddns-updates except for the IOT scope, which has a_lot of devices, but not the entire one, there are some subnets, for which folks actually care. I dropped some static mappings and obfuscated a few things: |
Resolve conflicts with current
Resolve merge conflicts
…uage due to how Kea treats set/unset options in scopes
abukharov
force-pushed
the
feature/dhcpv4-ddns
branch
2 times, most recently
from
3bda0d6 to
502d89e
Compare
I don't like it either, but I'm at a loss as to how we can implement it in a better way without losing too much functionality/flexibility. If you have a suggestion I'm happy to reimplement this part. |
abukharov
force-pushed
the
feature/dhcpv4-ddns
branch
from
502d89e to
a49b9d0
Compare
There was a problem hiding this comment.
I left some comments about the CLI.
Regarding boolean <enable|disable> nodes — it's a guideline, not a dogma, but the guideline is that we do not use them because the value doesn't add new information to the config, and with valueless nodes, it's easier to understand what the default is (e.g., if it's "send-updates" it's clear that by default it doesn't send them; if it's disable-send-updates or similar, then it's clear that it sends them unless they are disabled).
Since nodes that are boolean here are boolean in Kea and clearly have defaults in it, I'm not convinced that <enable|disable> really improves flexibility. I may be wrong, of course.
| <properties> | ||
| <help>TSIG key algorithm</help> | ||
| <completionHelp> | ||
| <list>hmac-md5 hmac-sha1 hmac-sha224 hmac-sha256 hmac-sha384 hmac-sha512</list> |
There was a problem hiding this comment.
I understand that it mirrors option names from Kea, but I don't think an exact mirror is always a good thing. The hmac- part adds no new information for the user (of course they are all hashed message authentication code algorithms) and makes completion harder.
I think it should be just md5 sha1 sha224 ... — we can add the hmac- bit in the template.
| <!-- include start from dhcp/ddns-settings.xml.i --> | ||
| <leafNode name="send-updates"> | ||
| <properties> | ||
| <help>Enable or disable updates for this scope</help> |
There was a problem hiding this comment.
As far as I can see, these options are all boolean. Is there really a reason not to make it a valueless node send-updates?
| <constraintErrorMessage>Set it to either enable or disable</constraintErrorMessage> | ||
| </properties> | ||
| </leafNode> | ||
| <leafNode name="force-update-both"> |
There was a problem hiding this comment.
I think the name from Kea, override-client-update is clearer than force-update-both. I also cannot see a reason not to make it valueless here. All those things are boolean in the parser grammar, do they have a potential to become tri-state or more?
| <constraintErrorMessage>Set it to either enable or disable</constraintErrorMessage> | ||
| </properties> | ||
| </leafNode> | ||
| <leafNode name="force-update"> |
There was a problem hiding this comment.
I find it kinda odd that Kea has separate override-client-update and override-no-update options. I can't infer it from the docs if override-client-update implies override-no-update?
https://kb.isc.org/docs/why-doesnt-my-dns-get-updated-by-kea doesn't explain it well.
I think we may also name this override-no-update, since it's about a behavior that depends on what the client says.
| <constraintErrorMessage>Invalid qualifying suffix</constraintErrorMessage> | ||
| </properties> | ||
| </leafNode> | ||
| <leafNode name="update-on-renew"> |
| <constraintErrorMessage>Set it to either enable or disable</constraintErrorMessage> | ||
| </properties> | ||
| </leafNode> | ||
| <leafNode name="conflict-resolution"> |
When I was talking about flexibility, I meant it in a broader sense, as in introduce enough flexibility into the config language to not impair the functionality of Kea. The problem is, how I described it to @sarthurdev here: #4153 (comment), that the configuration logic in Kea is not binary. so it can't be described with only presence or absence of something. It also has a state where the attribute value is inherited from the parent scope, so 'enable' means that it's overridden into an enabled state, 'disable' means that it's overridden into a disabled state and means that it's propagated from the parent scope. I also brought up a real use-case in the config snippet above. In the conversation with @sarthurdev I did an experiment with an alternative language, that was technically fit for purpose, but going this way would significantly bloat the syntax, in a much worse way than <enable|disable> |
|
CI integration 👍 passed! Details
|
Change Summary
This PR introduces support for RFC-2136 DDNS updates in Kea DHCP4 server.
Types of changes
Related Task(s)
https://vyos.dev/T6773
Related PR(s)
vyos/vyos-documentation#1561
Component(s) name
dhcp-server
Proposed changes
Introduces new configuration parameters under service dhcp-server and the logic to translate it into Kea 2.4.1 configuration language.
How to test
Set up a Technitium DNS in a container inside VyOS. Set up DDNS updates as follows:
Get the DHCP server to issue some leases and check if the DNS server gets updated.
Smoketest result
Checklist: