Skip to content
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.

Sign up for GitHub

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

luci-mod-network: DHCP tabs redesign phase 2 #7178

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

luci-mod-network: DHCP tabs redesign phase 2 #7178

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
289a9ff
luci-mod-network: Add MAC tab.
systemcrash Jun 27, 2024
dfb2875
luci-mod-network: Add Match Tags (via matching Opt+Val) tab.
systemcrash Jun 27, 2024
8b075b5
luci-mod-network: Match Tags tab: validate tag names
systemcrash Jun 27, 2024
8acbb3d
luci-mod-network: Add VendorClass tab.
systemcrash Jul 16, 2024
8438cfc
luci-mod-network: Add UserClass tab.
systemcrash Jul 16, 2024
4dcadf0
luci-mod-network: Add Set Tags (via Match (Option+Value)) tab.
systemcrash Jul 16, 2024
c8acf94
luci-mod-network: merge tag related tabs under Tags tab
systemcrash Jun 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
197 changes: 196 additions & 1 deletion modules/luci-mod-network/htdocs/luci-static/resources/view/network/dhcp.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,18 @@ callDHCPLeases = rpc.declare({
expect: { '': {} }
});

var callNetworkDevices = rpc.declare({
object: 'luci-rpc',
method: 'getNetworkDevices',
expect: { '': {} }
});

var listServices = rpc.declare({
object: 'service',
method: 'list',
expect: { '': {} }
});

CBILeaseStatus = form.DummyValue.extend({
renderWidget: function(section_id, option_id, cfgvalue) {
return E([
Expand Down Expand Up @@ -278,6 +290,8 @@ return view.extend({
callDUIDHints(),
getDHCPPools(),
network.getNetworks(),
callNetworkDevices(),
listServices(),
uci.load('firewall')
]);
},
Expand All @@ -288,11 +302,15 @@ return view.extend({
duids = hosts_duids_pools[1],
pools = hosts_duids_pools[2],
networks = hosts_duids_pools[3],
m, s, o, ss, so, dnss;
m, s, o, ss, so, dnss, tagstab;

var devices = Object.keys(L.toArray(hosts_duids_pools[4])[0]);
var services = Object.keys(L.toArray(hosts_duids_pools[5])[0]);

let noi18nstrings = {
etc_hosts: '<code>/etc/hosts</code>',
etc_ethers: '<code>/etc/ethers</code>',
exclamationmark_invert: '<code>!</code>',
localhost_v6: '<code>::1</code>',
loopback_slash_8_v4: '<code>127.0.0.0/8</code>',
not_found: '<code>Not found</code>',
Expand All @@ -304,6 +322,18 @@ return view.extend({
reverse_arpa: '<code>*.IN-ADDR.ARPA,*.IP6.ARPA</code>',
servers_file_entry01: '<code>server=1.2.3.4</code>',
servers_file_entry02: '<code>server=/domain/1.2.3.4</code>',
tagcodestring:'<code>tag</code>',
tag_named_ov_string: '<code>option(6):&lt;opt-name&gt;,[&lt;value&gt;[,&lt;value&gt;]]</code>',

//match tags
dhcp_option_code: '<code>option(6)</code>',
dhcp_optioncolon_code: '<code>option(6):</code>',
dhcp_option_client_arch: '<code>option:client-arch,6</code>',
dhcp_value_code: '<code>,value</code>',
tag_match_code_name: '<code>match</code>',
tag_match_option_syntax: '<code>&lt;option number&gt;|option:&lt;option name&gt;[,&lt;value&gt;]</code>',
tag_name_efi_ia32: '<code>efi-ia32</code>',
wildcard_code: '<code>*</code>',

};

Expand Down Expand Up @@ -367,6 +397,7 @@ return view.extend({
s.tab('ipsets', _('IP Sets'));
s.tab('relay', _('Relay'));
s.tab('pxe_tftp', _('PXE/TFTP'));
s.tab('tagsparent', _('Tags'));

s.taboption('filteropts', form.Flag, 'domainneeded',
_('Domain required'),
Expand Down Expand Up @@ -1138,6 +1169,170 @@ return view.extend({
_('Forward/reverse DNS'),
_('Add static forward and reverse DNS entries for this host.'));



o = s.taboption('tagsparent', form.SectionValue, '__tagsparent__', form.TypedSection, '__tagsparent__');

tagstab = o.subsection;

tagstab.anonymous = true;
tagstab.cfgsections = function() { return [ '__tagsparent__' ] };

tagstab.tab('mac', _('MAC'));
tagstab.tab('matchtags', _('Match Tags'));
tagstab.tab('vc', _('VC'));
tagstab.tab('uc', _('UC'));
tagstab.tab('settags', _('Set Tags'));


o = tagstab.taboption('mac', form.SectionValue, '__mac__', form.TableSection, 'mac', null,
_('MAC hardware addresses uniquely identify clients to set tags on them.') + '<br /><br />' +
_('Use the <em>Add</em> Button to add a new MAC.'));
ss = o.subsection;
ss.addremove = true;
ss.anonymous = true;
ss.sortable = true;
ss.nodescriptions = true;
ss.modaltitle = _('Edit MAC');
ss.rowcolors = true;

so = ss.option(form.Value, 'mac', _('MAC match'));
so.validate = isValidMAC;
so.rmempty = false;
so.optional = false;

so = ss.option(form.Value, 'networkid', _('Set this Tag'));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we not replace this string Set this Tag with string ... To this matching Tag, then it would remain consistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In short: no. That would change the meaning of how this tag tab works.

so.rmempty = false;
so.optional = false;

o = tagstab.taboption('matchtags', form.SectionValue, '__tags__', form.TableSection, 'tag', null,
customi18n( _('A {tagcodestring} is an alphanumeric label. dnsmasq applies chosen DHCP options when a specific {tagcodestring} is encountered.')) + '<br />' +
customi18n( _('In other words: "This {tagcodestring} gets these {tag_named_ov_string}".')) + '<br />' +
customi18n( _('Note: invalid {tag_named_ov_string} combinations may cause dnsmasq to silently crash.')) + '<br /><br />' +
customi18n( _('Prepend a {tagcodestring} with {exclamationmark_invert} to invert their domain of application, e.g. to send options to a host lacking a {tagcodestring}.') ) + '<br /><br />' +
customi18n( _('Use the %s Button to add a new {tagcodestring}.').format( _('<em>Add</em>') ) ) );
ss = o.subsection;
ss.addremove = true;
ss.anonymous = true;
ss.sortable = true;
ss.nodescriptions = true;
ss.modaltitle = _('Edit tag');
ss.rowcolors = true;

so = ss.option(form.DynamicList, 'dhcp_option',
_('Apply these DHCP Options...'),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since it is a table I would prefer that we change the following strings Apply these DHCP Options... and ... To this matching Tag. This text should not be seen as a sentence. Maybe this can`t be translated like this in other languages.

Apply these DHCP Options... -> DHCP options
... To this matching Tag -> Matching tag

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other languages are very capable of expressing this. I want these settings to be abundantly clear, because tags are evidently tough to grasp.

_('Options to be added for this tag.'));
so.rmempty = true;
so.optional = true;
so.placeholder = '3,192.168.10.1,10.10.10.1';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be nice to add a validation there so that a combination of wrong option doesn't crach the dnsmasq?

Or even better can we not abstract this further without having to look up in the documentation of dnsmasq what option for example '3' exactly means?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe code golf one day.

But the user is nevertheless expected to know what settings they need. (There are lots of them).


so = ss.option(form.Value, 'tag', _('...To this matching Tag'));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change text ...To this matching Tag?
See above.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would confuse its meaning (implying how tags are used here)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is not 'tag' option, 'tag' is actually the section name: https://github.com/openwrt/openwrt/blob/a9ff3ba24b9466d9c06b9a9e1c2e9672bdd9f6f9/package/network/services/dnsmasq/files/dnsmasq.init#L461

so.rmempty = false;
so.optional = false;
so.placeholder = 'specialgateways';
so.validate = function(section, value) {
for (var i = 0, l = devices.length; i < l; i++) {
if (devices[i] == value)
return _('Tag name %s must not match any active device name').format(value);
}
for (var i = 0, l = services.length; i < l; i++) {
if (services[i] == value)
return _('Tag name %s must not match any service name').format(value);
}
return true;
};

so = ss.option(form.Flag, 'force',
_('Force'),
_('Send options to clients that did not request them.'));
so.rmempty = false;
so.optional = true;

o = tagstab.taboption('vc', form.SectionValue, '__vc__', form.TableSection, 'vendorclass', null,
_('Match Vendor Class (VC) strings sent by DHCP clients as a trigger to set tags on them.') + '<br /><br />' +
_('Use the <em>Add</em> Button to add a new VC.'));
ss = o.subsection;
ss.addremove = true;
ss.anonymous = true;
ss.sortable = true;
ss.nodescriptions = true;
ss.modaltitle = _('Edit VC');
ss.rowcolors = true;

so = ss.option(form.Value, 'vendorclass', _('Match this Vendor Class'));
so.rmempty = false;
so.optional = false;

so = ss.option(form.Value, 'networkid', _('In order to set this Tag'));
so.rmempty = false;
so.optional = false;

so = ss.option(form.Flag, 'force',
_('Force'),
_('Send options to clients that did not request them.'));
so.rmempty = false;
so.optional = true;

o = tagstab.taboption('uc', form.SectionValue, '__uc__', form.TableSection, 'userclass', null,
_('Match User Class (UC) strings sent by DHCP clients as a trigger to set tags on them.') + '<br /><br />' +
_('Use the <em>Add</em> Button to add a new UC.'));
ss = o.subsection;
ss.addremove = true;
ss.anonymous = true;
ss.sortable = true;
ss.nodescriptions = true;
ss.modaltitle = _('Edit UC');
ss.rowcolors = true;

so = ss.option(form.Value, 'userclass', _('Match this User Class'));
so.rmempty = false;
so.optional = false;

so = ss.option(form.Value, 'networkid', _('In order to set this Tag'));
so.rmempty = false;
so.optional = false;

so = ss.option(form.Flag, 'force',
_('Force'),
_('Send options to clients that did not request them.'));
so.rmempty = false;
so.optional = true;

o = tagstab.taboption('settags', form.SectionValue, '__settags__', form.TableSection, 'match', null,
customi18n( _('Encountering chosen DHCP {dhcp_option_code}s (or also its {dhcp_value_code}) from clients triggers dnsmasq to set alphanumeric {tagcodestring}s.')) + '<br />' +
customi18n( _('In other words: "{tag_match_code_name} these {dhcp_option_code}s to set this {tagcodestring}" or "These {dhcp_option_code}s set this {tagcodestring}".')) + '<br />' +
customi18n( _('Internally, these configuration entries are called {tag_match_code_name}.')) + '<br />' +
customi18n( _('Matching option syntax: {tag_match_option_syntax}.')) + ' ' +
customi18n( _('Prefix named (IPv6) options with {dhcp_optioncolon_code}.')) + ' ' +
customi18n( _('Wildcards ({wildcard_code}) allowed.')) + '<br /><br />' +
customi18n( _('Match {dhcp_option_client_arch}, Tag {tag_name_efi_ia32}, sets tag {tag_name_efi_ia32}')) + ' ' +
_('when number %s appears in the list of architectures sent by the client in option %s.').format('<code>6</code>', '<code>93</code>') + '<br />' +
customi18n( _('Use the %s Button to add a new {tag_match_code_name}.').format(_('<em>Add</em>'))) );
ss = o.subsection;
ss.addremove = true;
ss.anonymous = true;
ss.sortable = true;
ss.nodescriptions = true;
ss.modaltitle = _('Edit Match');
ss.rowcolors = true;

so = ss.option(form.Value, 'match', _('Match this client option(+value)'));
so.rmempty = false;
so.optional = false;
so.placeholder = '61,8c:80:90:01:02:03';

so = ss.option(form.Value, 'networkid', _('In order to Set this Tag'));
so.rmempty = false;
so.optional = false;
so.placeholder = 'myuniqueclientid'

so = ss.option(form.Flag, 'force',
_('Force'),
_('Send options to clients that did not request them.'));
so.rmempty = false;
so.optional = true;


o = s.taboption('leases', CBILeaseStatus, '__status__');

if (has_dhcpv6)
Expand Down
3 changes: 2 additions & 1 deletion modules/luci-mod-network/root/usr/share/rpcd/acl.d/luci-mod-network.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,8 @@
"description": "Grant access to DHCP configuration",
"read": {
"ubus": {
"luci-rpc": [ "getDHCPLeases", "getDUIDHints", "getHostHints" ]
"luci-rpc": [ "getDHCPLeases", "getDUIDHints", "getHostHints", "getNetworkDevices" ],
"service": [ "list" ],
},
"uci": [ "dhcp" ]
},
Expand Down