Allow setting UniFi Controller IP in Kea DHCP.

[reitermarkus]

Tested using WireShark for my controller IP, 10.0.1.77 (0x0a00014d):

Option: (43) Vendor-Specific Information
  Length: 6
  Value: 01040a00014d

[AdSchellevis]

We need something more generic and better maintainable for this, option 43 equals vendor-encapsulated-options (https://kea.readthedocs.io/en/latest/arm/dhcp4-srv.html) and can be used for multiple purposes, when 10 different vendors use the same option, we rather don't want to add all of them (and possibly cause incompatible variations of options).

Leaving this open as feature request and room for further discussion.

[mimugmail]

Its the same as my previous request, option 43 should be some kind of grid as there could be multiple 43 option entries

[AdSchellevis]

@mimugmail but if I'm not mistaken, they are mutually exclusive (so only one applies for that specific configuration)

[reitermarkus]

they are mutually exclusive

Based on 9.3 of https://www.isc.org/docs/2023kea_custom_options.pdf, there is a way to apply different options for different clients.

[AdSchellevis]

Based on 9.3 of https://www.isc.org/docs/2023kea_custom_options.pdf, there is a way to apply different options for different clients.

But I would like to keep it as simple as possible.... these type of constructs are almost impossible to generalize.

[reitermarkus]

I have made this more generic now, by adding option definitions

and options

and allowing to specify the options in subnets

[AdSchellevis]

@reitermarkus although it feels a bit like using a cannon to kill a mosquito, maybe it's something we can't prevent for these custom options (I was hoping to keep this simple). I'll take a look as soon as I can.

[AdSchellevis]

@reitermarkus what if we merge the definitions and the data? from a data perspective the split might be cleaner, but from a user input perspective the question is how often this relation is actually 1-N. We can polish the input a bit to prevent non-relevant options being shown as well (e.g. the record type inputs). thoughts?

[reitermarkus]

I have merged the definitions and data. Can you let me know how to hide e.g. the record types field based on the type field?

[AdSchellevis]

I don't mind changing that for you, but if you want to give it a shot, usually we add a style to the field and hook an event. For example in OpenVPN:

https://github.com/opnsense/core/blob/a8e329b90529bac6ac3682d92fd6d7dec89b3cae/src/opnsense/mvc/app/controllers/OPNsense/OpenVPN/forms/dialogInstance.xml#L130

https://github.com/opnsense/core/blob/a8e329b90529bac6ac3682d92fd6d7dec89b3cae/src/opnsense/mvc/app/views/OPNsense/OpenVPN/instances.volt#L63-L76

But if we agree on the direction, I'm more than fine polishing the final bits when merging.

[reitermarkus]

I have now also added a field to specify a client class test condition.

I'll leave the polishing up to you. Ideally record_types should work like select_multiple but with duplicates allowed instead of being a plain comma-separated string.

[AdSchellevis]

@reitermarkus I don't think we should add the client class now as the relation between both entries is not the same (usually a client class contains a set of options). If in the future we do want to add client-classes, we should be able to reuse the custom options.

[reitermarkus]

I have tried to figure this out, but it doesn't seem that simple after all. I think the definitions have to be split again, as there are currently a bunch of problems:

• It is currently not possible to specify the same option with different data for different subnets.
• Pre-defined options cannot be re-defined, so these need to be specified without a definition.
• If client classes are added, sub-options need to be defined globally with a custom space in case they conflict with other client classes, while the parent-option needs to be defined in the client class, see the example here: https://kea.readthedocs.io/en/latest/arm/dhcp4-srv.html#dhcpv4-private-options

[AdSchellevis]

Maybe a stupid question, but can't we use uuids to bind the definitions and payload together? Personally I have a hard time finding definitions for kea fields in different area's, but something like this doesn't seem to crash:

         "option-def": [
                {
                        "name": "11635360-0eaf-4c63-9d30-a192c8e59168",
                        "code": 1,
                        "type": "string",
                        "space": "11635360-0eaf-4c63-9d30-a192c8e59168"
                }
        ],
....
        "option-data": [
                    {
                        "name": "11635360-0eaf-4c63-9d30-a192c8e59168",
                        "space": "11635360-0eaf-4c63-9d30-a192c8e59168",
                        "data": "0.0.0.0"
                    }
        ]

While writing this, it still feels like we're sending a rocket to the moon, where in practice, most issues only circle around code 43 as this option isn't well defined.

Personally I think offering fine grained controls with client classes adds way more complexity than 99% of our users are looking for, when someone needs almost programatic control of the dhcp process, a manual configuration might be a better alternative.

[reitermarkus]

but can't we use uuids to bind the definitions and payload together?

This will only work for sub-options. But then you need to also re-define the parent option with "encapsulate": "11635360-0eaf-4c63-9d30-a192c8e59168", which can only be done in a client class, i.e. you cannot re-define a pre-defined option globally.

[AdSchellevis]

hmm, that's annoying. Partly this is difficult in terms of templating (overcomplicated jinja templates are hard to maintain) and to some degree the user input is also problematic. We could opt for moving the json file generation to the model, which does make it easier to de-duplicate certain information into a single container.

If we change the config generation handling, the next question is, what should the user input look like and how do we prevent invalid entries being generated. Do you have thoughts about that?

I do think it makes sense to first figure out what we want to build and generate code after that, there are some loose ends here, currently I do have some difficulties seeing all constraints that apply.

I don't mind spending some time on this to ease future additions, this was also a bit of the goal from my end to not start with ipv6 yet until ew have enough feedback for the ipv6 variant.

[AdSchellevis]

@reitermarkus can you try the code currently in master? https://github.com/opnsense/core/commit/3f184a695fdbcbe4071f61da54227a437da94bcd adds the "vendor-encapsulated-options-space" with a minimal set of options. I haven't tested this myself, but the generated configuration on my end is valid and the ui offers validations to prevent duplicate sub-codes being inserted with different definitions.

[AdSchellevis]

@mimugmail tried it out, didn't seem to work, but config looks as expected. I've moved the feature into its own branch for now implementing https://github.com/opnsense/core/commit/79f62cf681afd722ca2d797901554ffcb2e73460 (kea_custom_opt_pr7361)

[MeneerThijs]

The decision to move the json file generation to the model breaks the ability to use target overwrites to create custom kea-dhcp4.conf templates. This change limits the use of Kea for users who want to use features that are not (yet) available in the GUI. Please reconsider this change.

[AdSchellevis]

not reconsidering (due to the long term maintainability of the json file), but if you wish to disable kea from the gui and use a custom configuration instead just open a ticket. It's not a priority for us, but I'm open for suggestions.

[fichtner]

@MeneerThijs the basic approach here is to ask for inclusion of file-based includes if kea supports it...

[MeneerThijs]

@fichtner Thank you for your suggestion. Kea indeed support file-based inclusions; however, my use case requires inclusions at multiple locations. I opted to create a custom template for keactrl.conf which references a custom kea-dhcp4.conf file in an alternate directory. (This is necessary because the filename must remain kea-dhcp4.conf, as it is used in the pid filename and the plugin definition expects a pid file named /var/run/kea/kea-dhcp4.kea-dhcp4.pid.) I realize this is a bit of a hack but at least it does not require patching any code..

[fichtner]

@MeneerThijs @AdSchellevis I know it is redundant but if we chain the config generation in the right order this becomes possible with the kea template override subfolder:

kea_setup="/usr/local/sbin/pluginctl -c kea_sync"

changed to

kea_setup="/usr/local/sbin/pluginctl -c kea_sync; /usr/local/sbin/configctl template reload OPNsense/Kea"

Cheers, Franco

[AdSchellevis]

I don’t think we should as we’re not offering a suitable template it will just overwrite the one just created if I’m not mistaken. The standard overwrite always has an example (our template) which you can revert to

[fichtner]

Maybe I don't understand the problem scope. If someone wants an override and the system delivers that should be working as intended. Neither kea-dhcp4.conf nor kea-ctrl-agent.conf are in the +TARGETS now but if someone wanted to use them the template system can't deliver this at the moment because it's rewritten later.

[AdSchellevis]

Before writing my response. I do agree the problem scope is not clear enough.

My concerns are that (if I’m not mistaken) we render the templates twice for everybody else and people trying to make a modification to our template can usually do that by cloning ours and ship a new version.

If we don’t offer the base template, people will start opening tickets like these when future changes are incompatible. The previous template will break when we progress in time, which means building your own json with parts of the configuration will be a less viable option in the near future anyway. There are very good reasons why in this case we don’t use the template system (template complexity growing rapidly leading to hard to debug edge cases)

In my opinion the only viable extension here is if you don’t want to use our code at all and ship your own config, for which there are likely cleaner solutions. …. This also loops back to problem scope.

long story short, it all start with a ticket explaining goals for which we can agree or disagree if they match ours.