The older static lease for DHCP in V5 had a more user-friendly interface, while the new V6 expose a text field for the user write a config file-like text.
While one can argue that the new interface is more powerful, it lacks the ease of use that was a fundamental cornerstone of Pi-Hole.
I suggest reverting to the old GUI, leaving this new one as an "advanced" option for those who want.
I'd expect that v5's three columns (MAC|IP|hostname) would be enough for the vast majority of v6 users as well, especially since that would be what they did configure in v5.
If v5 users needed some of the more exotic options, they probably have created a custom dnmasq conf or edited the file directly, so they'd be already somewhat familiar with dnsmasq's detailed options.
Instead of trying to fit all possible options into a table, how about making the existing table a Basic level interface, with a new, extra column 'expert options present' that shows as ticked when more options are set, and replacing that with the current v6 style view in the Expert panel?
Parsing the Expert options for the Basic UI shouldn't be overly difficult:
Strip or ignore options starting with set: or tag: as well as ignore, and then parse by strict order, the first being MAC or id, second IP, third hostname.
Sorting Basic should affect how Expert lines are sorted accordingly.
The tricky part being not to wipe out possible "expert" setting when editing the line on the "basic" level. We could also consider making a much larger table having eight columns:
How about a middle ground? Showing both the table and the text field below it? One automatically updates the other (I did the same for the DNS server selection where you could add extra or special servers below in the (usually collapsed) text area?
When the script detects a non-basic configuration in any line, it could simply disable the table with a small note that advanced stuff has been detected and, hence, editing needs to happen below?
How about keeping a copy of those 3 entries per line when populating the UI table, in addition to the entry field values?
Updating a dnsmasq configuration line would then just be a search for the copied values to be replaced by the values entered via UI.
Allowing comma-separated lists of values in one column still wouldn't seem to be a good fit to me.
Also, showing those additional options as columns may prompt users to fill them in, even when there would be no obvious use (e.g. aforementioned IPv6 addresses will never get assigned as Pi-hole won't operate as DHCPv6 server anyway (hmm, maybe we could cut down on options with that approach)).
I'd rather have the v5 table for backwards compatibilty, and leave the flat file format for expert use only (which it probably should be).
No, I thought of it as a purely informational tick box.
But yours looks similar to DL6ER's suggestion, only that his idea is to show the full line under the three fields, if I understand him correctly.
But combining fields and full line editing in one UI could make UI updating more challenging than when you keep them separate (fields for basic, flat file editing for expert).
If we are going to use separate input fields for each option, there is no way to avoid comma separated entries (some options will need to accept more than one value).
before: Yes, they can cause issues. The IP address is fed through inet_pton which may trim spaces around addresses, I have not seen any details about this in man inet_ntop but it may be possible. The extra spaces and tabs in the hostname are probably an issue but dnsmasq validates the hostnames you define here for validity, i.e., only a-z A-Z 0-9 - _ are allowed here. It should fail hard with bad DHCP host name otherwise,
FTL and the web interface didn't generate any error messages when entering the values containing spaces and tabs. If later dnsmasq generates an error, this will cause user confusion.
Maybe we need to validate the values sent via web interface / API.
I did just try something useless and get an error as expected:
Note that the DHCP server needs to be enabled to get the error as - otherwise - the dhcp.hosts array is not added to dnsmasq's configuration and, accordingly, also is not tested.
I enabled DHCP and added an static lease (ÄÖÄ,192.168.1.4,fake).
It failed with an error message (as expected), but I also added a line using the format "MAC,IP,hostname" containing spaces and tabs in the IP and hostname.
FTL accepted them without errors:
But how realistic of a scenario is this? It would need quite some extra code as FTL is currently not aware of any kind of dependency between settings. Even without it, the issues should be small. Once you try to enable it, FTL will validate the entire config and detect that at least one dhcp-host is invalid in your example. It would then reject the config change (in this case the try to enable the DHCP server) and print the warning about the offending line (the first invalid dhcp-host line). The user can then try and fix it and repeat this as long as all invalid entries are gone. There should be no time without a running dnsmasq as it should never really be restarted if at least one invalid entry remains.
But we could change this to always adding dhcp-host to the config even when the server is disabled. dhcp-host lines without enabled DHCP server should be possible, they will just not do anything useful (but they'd be checked). However, there is an entire block of settings which are only applied when dhcp.active = true, changing this may mean a bit of a mess in the config file and seems not the best option at first sight.
They also end up in the used dnsmasq configuration so that's consistent.
I did just check that and can confirm inet_ntop is pretty robust when it comes to detecting and analyzing IP addresses plus dnsmasq feeds the specified host names through libidn which strips away the spaces/tabs on the hostname.
TL;DR: Your spaces/tab example works because dnsmasq is graceful. This does not have to apply to everything, though. The source code processing dhcp-host is rather difficult to read because it has some rather strange backwards-compatibility stuff, but, e.g., spaces even inside MAC addresses are tolerable as well.
Regarding the basic/advanced input field. The first idea that came to my mind is exactly this
90% of users will be happy with the current 3 column input fields. If users need more advanced configurations, they could use the text input field then.
OK.
That was my main concern, because it is allowed by the web interface and FTL, but I wasn't sure if dnsmasq would accept or complain.
The scenario I imagined was a case were an user import or create many static invalid leases while Pi-hole DHCP is disabled. Later (after all invalid addresses are already in Pi-hole options), the user decides to turn the DHCP on, and the user will receive many warnings during dnsmaq. Users will say "The web interface accepted the values, but now it is saying this is invalid".
I'm not sure how realistic is this. Maybe I'm just imagining a very crazy/unlikely scenario, but apparently it is possible.
I think we don't need to change anything for now.
At least we discussed this possibility and now we know where to look if some one starts to complain.
