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:

core/src/opnsense/mvc/app/controllers/OPNsense/OpenVPN/forms/dialogInstance.xml

Line 130 in a8e329b

<style>selectpicker role role_server</style>

core/src/opnsense/mvc/app/views/OPNsense/OpenVPN/instances.volt

Lines 63 to 76 in a8e329b

	$("#instance\\.role, #instance\\.dev_type").change(function(){
	let show_advanced = $("#show_advanced_formDialogDialogInstance").hasClass("fa-toggle-on");
	let this_role = $("#instance\\.role").val();
	let this_dev_type = $("#instance\\.dev_type").val();
	$(".role").each(function(){
	let tr = $(this).closest("tr").hide();
	if ((tr.data('advanced') === true && show_advanced) || !tr.data('advanced')) {
	if ($(this).hasClass('role_' + this_role) || $(this).hasClass('role_' + this_role + '_' + this_dev_type)) {
	tr.show();
	}
	}
	});
	});

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.

[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.

[mimugmail]

@AdSchellevis I just tried "make mount" with a current system and I can set the values. Is this way to go to test?
Currently having a problem with a customer and also with hex in isc dhcp, would be a good fit to continue testing :)

[AdSchellevis]

@mimugmail this probably needs a rebase, the ticket has been stale for months I'm afraid. The branch I opened is https://github.com/opnsense/core/tree/kea_custom_opt_pr7361, I can try to rebase to master, but that has a lot of other breaking changes.

[mimugmail]

I did:

opnsense-code core
cd /usr/core
git checkout kea_custom_opt_pr7361
make mount 

Sure, Dashboard is messy but for testing it's fine. No need to rebase right now, let me test first :)

[fichtner]

why not "make upgrade" or if it complains "make upgrade CORE_NAME=opnsense" ;)

(ok I couldn't resist)

[mimugmail]

Shawn would say: Because reasons :)

[mimugmail]

Hi, I spent some time on this turning some rounds and it seems the PR of @reitermarkus is correct. Kea requirest the use of client-classes for code 43, as sub classes are intended with this one.

This is working when editing the file directly:

        "subnet4": [
            {
                "id": 1,
                "subnet": "10.1.1.0\/24",
                "client-class": "ubnt",
                "option-data": [
                    {
                        "name": "domain-name-servers",
                        "data": "192.168.1.1,192.168.1.2"
                    },
                    {
                        "name": "domain-search",
                        "data": "domain"
                    },
                    {
                        "name": "routers",
                        "data": "10.1.1.1"
                    },
                    {
                        "name": "domain-name",
                        "data": "domain"
                    },
                    {
                        "name": "ntp-servers",
                        "data": "192.168.1.1"
                    },
                    {
                        "name": "time-servers",
                        "data": "192.168.1.1"
                    }
                ],
                "pools": [
                    {
                        "pool": "10.101.1.100-10.101.1.200"
                    }
                ],
                "reservations": [
                    {
                        "hw-address": "ff:ff:ff:ff:ff:ff",
                        "ip-address": "10.1.1.200",
                        "hostname": "WOL-Broadcast"
                    }
                ]
            }
        ],
        "option-def": [
            {
                **"name": "unifi-address",
                "code": 1,
                "type": "ipv4-address",
                "space": "ubnt",**
            }
        ],
    "client-classes": [
        { "name": "ubnt",
          "test": "(option[vendor-class-identifier].text == 'ubnt')",
          "option-def": [
                { "name": "vendor-encapsulated-options",
                  "type": "empty",
                  "encapsulate": "ubnt",
                  "code": 43 }
          ],
          "option-data": [
              { "name": "unifi-address",
                "space": "ubnt",
                "data": "16.1.1.1" },
              { "name": "vendor-encapsulated-options" }
          ]
        }
    ]
    }
}

[mimugmail]

Btw. space name needs to be ubnt, vendor-encapsulated-options-space_43 will be ignored by the dhcp device.

[AdSchellevis]

@mimugmail it has been quite some time since I made the branch, not sure what it needs to offer this functionality without offering impossible to validate input. Can you ping me in a couple of weeks? With the example you offered I can have another look when I have some time