Skip to content

Allow setting UniFi Controller IP in Kea DHCP. #7361

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

Open
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

reitermarkus
Copy link
Contributor

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

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

@AdSchellevis AdSchellevis self-assigned this Apr 5, 2024
@AdSchellevis
Copy link
Member

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
Copy link
Member

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

@AdSchellevis
Copy link
Member

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

@reitermarkus
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

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

and options

and allowing to specify the options in subnets

Bildschirmfoto 2024-04-07 um 00 39 26

@AdSchellevis
Copy link
Member

@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
Copy link
Member

@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
Copy link
Contributor Author

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
Copy link
Member

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:

<style>selectpicker role role_server</style>

$("#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
Copy link
Contributor Author

reitermarkus commented Apr 7, 2024

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
Copy link
Member

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

@AdSchellevis AdSchellevis force-pushed the master branch 4 times, most recently from 6586a65 to 607e32a Compare December 3, 2024 15:53
@mimugmail
Copy link
Member

@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
Copy link
Member

@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
Copy link
Member

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
Copy link
Member

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

(ok I couldn't resist)

@mimugmail
Copy link
Member

Shawn would say: Because reasons :)

@mimugmail
Copy link
Member

mimugmail commented Jan 10, 2025

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
Copy link
Member

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

@AdSchellevis
Copy link
Member

@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

AdSchellevis added a commit that referenced this pull request Jan 23, 2025
Since our efforts to implement #7361 hasn't reached a functional state, lets move the code into a separate branch to keep master clean.
@AdSchellevis
Copy link
Member

I did rebase https://github.com/opnsense/core/tree/kea_custom_opt_pr7361 , but realistically this doesn't bring it much further at the moment. It doesn't look very feasible to offer an interface to client-classes which is easy to use and is possible to validate in a uniform way.

@AdSchellevis AdSchellevis force-pushed the master branch 2 times, most recently from bfdf0d3 to 968e5f9 Compare March 3, 2025 20:25
@pmhausen
Copy link
Contributor

pmhausen commented May 2, 2025

Came to this issue while checking the state of vendor option support.

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

Experience at least firmly says yes, although that might depend on how "intelligent" the client devices are.

DHCP provided unifi controller address confused the heck out of our SIP phones here which tried to parse it as their firmware bootstrap URL (also option 43). We ended up keeping the unifi option in place and configuring the phones statically through their UI.

Or maybe it's supposed to be a server side job to detect the type of client device and serve the matching option?

Any ETA/target release for at least a rudimentary implementation? I need that unifi thing before ISC DHCP is removed ;) Given how far this issue has gone I will not try to hack my own solution and submit it.

Kind regards,
Patrick

@AdSchellevis
Copy link
Member

Any ETA/target release for at least a rudimentary implementation? I need that unifi thing before ISC DHCP is removed ;) Given how far this issue has gone I will not try to hack my own solution and submit it.

no, not likely to appear either for reasons mentioned earlier. dnsmasq might be a better target to aim for or use a completely custom kea config (see also https://opnsense.org/roadmap/)

@pmhausen
Copy link
Contributor

pmhausen commented May 2, 2025

What do you think of "prepackaged" UI options for the more common vendor options? Given the ubiquity (grin) of Unifi gear we could start with that and leave it to users to request more - mutually exclusive and with easy fail safe setup and proper constraints like "valid IP address" for Unifi for example.

Precedence: you did take my "Observium support" option for SNMP, for example.

@mimugmail
Copy link
Member

Isnt there a config include folder for such setups with kea?

@AdSchellevis
Copy link
Member

Isnt there a config include folder for such setups with kea?

Unfortunately kea doesn't support includes, which either ends up in some hacky construct or not offering it at all, we choose the later for good reasons (see also #7822). Somewhere down the line we came to the conclusion kea might not be the best option for small and medium sized networks (which often has other requirements than provider grade ones), which is what triggered addition of dnsmasq for dhcp on our end.

I'm open for maintainable pull-requests, the time we have spend so-far on kea is a bit staggering to be honest, there are just limits in the amount of time I am willing to invest.

(maybe the best example so-far is about adding kea for v6, implementing the example as documented https://kea.readthedocs.io/en/stable/arm/dhcp6-srv.html#introduction, leading to the conclusion this can't work for "reasons" https://lists.isc.org/pipermail/kea-users/2018-January/001581.html)

@Poliisi5
Copy link

Poliisi5 commented May 5, 2025

Hi, i don't know if this is a stupid idea or not but could the inclusions be done trough "slicing" the the config files into multiple little files such as client-classes, custom-options-def, etc and including them when needed in the right sections.
https://kea.readthedocs.io/en/latest/arm/config.html#json-syntax

Example:
includes/client_classes/foo.conf
"client-classes": [ { "name": "Client_foo", "test": "substring(option[61].hex,0,3) == 'foo'", "option-data": [ { "name": "domain-name-servers", "code": 6, "space": "dhcp4", "csv-format": true, "data": "192.0.2.1, 192.0.2.2" } ] } ],

kea-dhcp4.conf
{ "Dhcp4": { "subnets4":[{ "id": 1, <?include "includes/client_classes/foo.conf"?> } ] }

(sorry if this a stupid idea and or improper but i thought this might be useful)

@AdSchellevis
Copy link
Member

It won't offer a consistent state, you can merge json files, but the outcome is unpredictable (increasing the chances kea won't start, which as a result triggers unsolvable reports).

kea in my experience is quite fragile, which means data validation is crucial. When people want custom configurations, just ship the full config, which is possible since #7822

@meyergru
Copy link
Contributor

meyergru commented May 29, 2025

What do you think of "prepackaged" UI options for the more common vendor options? Given the ubiquity (grin) of Unifi gear we could start with that and leave it to users to request more - mutually exclusive and with easy fail safe setup and proper constraints like "valid IP address" for Unifi for example.

Going forward, it seems like this is the far easier route to take than to try making a fully configurable template for generic options. This gets even more complex if such options are made dynamic / conditional, like only for a specfic vendor class. That mechanism was originally designed to keep DHCP/BOOTP packets small as to not overflow the available space.

If instead, such options are made available one-after-another on demand, afterwards, users can indivdually choose if they need them and also, validations are easy, like in this case, a single IPv4 address. Considering the recent discussions on how to correctly specify the Unifi opton for ISC DHCP and DNSmasq suggests, that this would be a good idea.

Also, implementing a specific variant for Unifi does not stand in the way of other vendor-specific option 43 use cases for other manufacturers, AFAIK.

@pmhausen
Copy link
Contributor

Also, implementing a specific variant for Unifi does not stand in the way of other vendor-specific option 43 use cases for other manufacturers, AFAIK.

Correct. As I found out with vendor classes you can have as many of them as you need. Will give unifi a try over the long weekend.

@Monviech
Copy link
Member

@AdSchellevis

https://help.ui.com/hc/en-us/articles/204909754-Remote-Adoption-Layer-3

https://forum.opnsense.org/index.php?topic=47370.msg238856#msg238856

You'll need to configure your DNS server to resolve unifi to your remote UniFi Network Application.
There are two methods of specifying the machine running the Network Application:
IP Address: [http://ip-address:8080/inform](http://xyz:8080/inform)
Fully Qualified Domain Name (FQDN): http://fqdn:8080/inform

Using this dhcp option 43 is optional for unifiy, not a strict requirement.

Just leaving this here for those who come after.

@mimugmail
Copy link
Member

Yep, but there are other business solutions around too, using option 43 to send configuration to clients like VoIP phones

@AdSchellevis
Copy link
Member

newer equipment is probably less likely to use option 43, but with dnsmasq it's likely a lot easier to match clients option 60 request to a proper 43 response anyway.

@meyergru
Copy link
Contributor

meyergru commented May 31, 2025

Yes, for DNSmasq, we already have the full IANA list and means to do this rule-based. Basically, the question is if you like to go the same route with Kea. For standard options, we have specific inputs already.

As I have said for DNSmasq already, I like a less error-prone, typed approach to have better checks for specific purposes, which create the needed output for daemon configurations. Of course, that comes with less flexibility.

Now, that I have learned how to modify the MVC model to generate non-generic output, I could try a pull request to add the Unifi option with an optional, standard IPv4 input field that does this. I do not think that a rule-based output would even be needed if the user can choose if he needs it, anyway. I would just plainly generate option 43 - and also, in the global section, since it is used for layer 3 traversal, anyway.

@AdSchellevis
Copy link
Member

I'm just not a fan of adding vendor specific items which aren't bound to any standard and can only be used for the vendor in question. There's just a high risk of creating a maintenance nightmare as we have no idea about the actual scope of the change.

Looking at other vendors, the most common method seems to be to just send raw data (optionally upon request), which is pretty similar to what dnsmasq on our end is currently offering.

A similar approach for KEA seems to be challenging and very time consuming to develop due to how the configuration file is modelled (and how poorly documentation seems to fit reality). It's just not something I plan to spend my time on at the moment, also given the limited amount of valuable use-cases.

In theory, for dnsmasq, we could eventually add validators for common types to protect agains common input mistakes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

9 participants