Make V6 Static DHCP Lease GUI More user friendly

The following is a conversation that happened on the same topic shortly before release.

@Bucking_Horn

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.

@DL6ER

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:

<hwaddr>   id:<client_id>   set:<tag>   tag:<tag>   <ipaddr>   <hostname>   <lease_time>   <ignore>

but this is probably nightmare in UI-design.

---

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?

Just generating a few idea here...

@Bucking_Horn

@DL6ER

The tricky part being not to wipe out possible "expert" setting when editing the line on the "basic" level.

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.

@rdwebdesign

@DL6ER

but this is probably nightmare in UI-design.

Maybe in smaller screens, but there is another possibility, similar to your table + text box idea.

We could create a simple table (4 columns = 3 values + action buttons) and an "Advanced" button to show the full table (with 8, 9 or 10 columns).

The "auto disable" feature can be used to disable the simple table as well.

@Bucking_Horn

I don't think that a fixed column layout is a good fit for the full set/table, as there can be no fixed number of columns.

set:, tag: and IPv6 addresses can occur multiple times.

@rdwebdesign

@Bucking_Horn

and IPv6 addresses can occur multiple times.

All IPv6 will be added in the same field/column. The the fixed number of columns is not the issue.

The issue would be the table width.

@rdwebdesign

@Bucking_Horn

How about keeping a copy of those 3 entries per line when populating the UI table, in addition to the entry field values?

Do you mean a table with 4 columns? Mac Address + IP + Hostname + "Advanced (containing the raw line text)"?

@Bucking_Horn

@rdwebdesign

All IPv6 will be added in the same field/column. The the fixed number of columns is not the issue.

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

@rdwebdesign

Do you mean a table with 4 columns? Mac Address + IP + Hostname + "Advanced (containing the raw line text)"?

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

@rdwebdesign

Yes. Similar to DL6ER's idea.

---

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

@DL6ER

Sorry, I missed the

@rdwebdesign

I was testing the page and (unintentionally) I entered a line containing spaces and tabs. I noticed FTL didn't remove these characters.

...

Do we need to clean up the entries to avoid dnsmasq issues?

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,

@rdwebdesign

Thanks for the detailed explanation.

@DL6ER

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.

@DL6ER

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.

@rdwebdesign

@DL6ER

Note that the DHCP server needs to be enabled to get the error

Hmmm... that was the issue.
I tested simply adding values to the field, but with the DHCP disabled.

We probably should not accept static leases if DHCP is disabled to avoid issues.

If an user enter many invalid static leases while the DHCP server is disabled (like I did) and later enable it, there will be issues.

@rdwebdesign

For completeness:

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:

A4:EE:57:30:0E:0F, 	192.168.1.2, 	printer
#                 ^^            ^ ^                 
#           space_||__tab space_| |__tab            

Note:
The spaces and tabs are preserved in the API return. Not sure if they will cause some issues with dnsmasq.

@DL6ER

@rdwebdesign

If an user enter many invalid static leases while the DHCP server is disabled (like I did) and later enable it, there will be issues.

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.

@rdwebdesign

but I also added a line using the format "MAC,IP,hostname" containing spaces and tabs in the IP and hostname.

@rdwebdesign

The spaces and tabs are preserved in the API return. Not sure if they will cause some issues with dnsmasq.

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.

@yubiuser

Regarding the basic/advanced input field. The first idea that came to my mind is exactly this

@DL6ER

howing 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?

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.

@rdwebdesign

@DL6ER

TL;DR: Your spaces/tab example works because dnsmasq is graceful.

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.

@DL6ER

But how realistic of a scenario is this?

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.

@rdwebdesign

I like this idea.

• table with the original 3 fields;
• textbox;
• "auto disable" (or auto hide) feature, to disable the table when there are advanced options;
• show note about the presence of advanced options