-
Notifications
You must be signed in to change notification settings - Fork 696
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
Allow setting UniFi Controller IP in Kea DHCP. #7361
base: master
Are you sure you want to change the base?
Conversation
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. |
Its the same as my previous request, option 43 should be some kind of grid as there could be multiple 43 option entries |
@mimugmail but if I'm not mistaken, they are mutually exclusive (so only one applies for that specific configuration) |
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 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. |
@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 |
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? |
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/views/OPNsense/OpenVPN/instances.volt Lines 63 to 76 in a8e329b
But if we agree on the direction, I'm more than fine polishing the final bits when merging. |
I have now also added a field to specify a client class test condition. I'll leave the polishing up to you. Ideally |
@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. |
… plugin configure hook, keep empty templates to inform people. (ref; #7361)
…ndor-encapsulated-options-space" options to subnets, for #7361
@reitermarkus can you try the code currently in master? 3f184a6 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. |
…el, move file generation to a plugin configure hook, keep empty templates to inform people. PR: #7361 (cherry picked from commit 29e87aa) (cherry picked from commit ac1d9d7) (cherry picked from commit b551927) (cherry picked from commit d241cfd) (cherry picked from commit 80b65b0) (cherry picked from commit dc80b7a)
Since our efforts to implement #7361 hasn't reached a functional state, lets move the code into a separate branch to keep master clean.
Since our efforts to implement #7361 hasn't reached a functional state, lets move the code into a separate branch to keep master clean.
Since our efforts to implement #7361 hasn't reached a functional state, lets move the code into a separate branch to keep master clean.
Since our efforts to implement #7361 hasn't reached a functional state, lets move the code into a separate branch to keep master clean.
Since our efforts to implement #7361 hasn't reached a functional state, lets move the code into a separate branch to keep master clean.
@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 79f62cf (kea_custom_opt_pr7361) |
Since our efforts to implement #7361 hasn't reached a functional state, lets move the code into a separate branch to keep master clean.
Since our efforts to implement #7361 hasn't reached a functional state, lets move the code into a separate branch to keep master clean.
The decision to move the |
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. |
@MeneerThijs the basic approach here is to ask for inclusion of file-based includes if kea supports it... |
@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 |
@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:
changed to
Cheers, |
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 |
Maybe I don't understand the problem scope. If someone wants an override and the system delivers that should be working as intended. Neither |
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. |
Tested using WireShark for my controller IP, 10.0.1.77 (
0x0a00014d
):