[pve-devel] [PATCH common 1/1] Fix #545: Allow descriptive names for network bridges and bonds

[Fabian Grünbichler]

On Thu, Jun 06, 2019 at 07:54:21AM +0200, Alexandre DERUMIER wrote:
> Hi,
> 
> I think the main problem could be the 16 characters limit.

we currently allow up to 20 characters in our API/the interface standard
option, and even more in the actual interface name format ('pve-iface').

'ip link' starts failing with 16 character-long interface names already,
so effectively its 15 characters (maybe this is a bug in iproute2?).

so effectively that means we have
- 11 characters left for hypervisor interfaces (vmbr/vnet/bond all have
  4 characters)
- already hit the limit for container interfaces (veth/fwbr/fwpr/fwln +
  9 digits of VMID + i + 1 digit interface index)
- already over the limit for VM interfaces (fwbr/fpr/fwln/tap + 9 digits
  of VMID +i + 2 digits interface index)

so IMHO, we already have at least two bugs here:
- API/JSONSchema accept interface names that are too long
- our naming scheme generates interface names that are too long for VMs
  with firewall enabled, maximum-length VMID, maximum-length interface
  index (e.g., VM 100000000 and net10)

> ifupdown2 already support "alias" option on interfaces
> 
> auto eth0
> iface eth0
>     alias "my super long string of description"
> 
> 
> which is doing:
> 
> #ip link set dev eth0 alias "my super long string of description"
> 
> 
> and you can see description with 
> #ip link

that would be nice to support in addition to more readable names. IMHO,
allowing at least alphanumeric (and some other characters like '-_.+'?)
should be completely fine, and allows admins to set short, descriptive
names like

vmbr_internal
vmbr_external
vmbr_dmz
vmbr_uplink
vmbr_vpn
vmbr_private
vmbr_c_xyz (for client/customer xyz)
vmbr_test
vmbr_dev

the interface name is used/displayed/supported across the board, the
alias likely is not (yet). e.g. stuff like references in config files,
firewall rules, monitoring scripts, metrics/billing, ...

> I was planning to use that for vnet too. (keeping vnet(\d+) as interface id, and add a custom description).
> 
> We could add easily a ifup script for ifupdown1.

if we allow more characters for vmbr and bond, the same would probably
apply to vnet as well ;)

> What do you think about this ?

I'd like to see both - more descriptive names, and some way to specify a
long form description (whether as alias, comment, label, .. - whatever
has the best ease of implementation/range of support ratio ;))

> ----- Mail original -----
> De: "Fabian Grünbichler" <f.gruenbichler at proxmox.com>
> À: "pve-devel" <pve-devel at pve.proxmox.com>
> Envoyé: Mercredi 5 Juin 2019 11:25:47
> Objet: Re: [pve-devel] [PATCH common 1/1] Fix #545: Allow descriptive names for network bridges and bonds
> 
> On Wed, May 15, 2019 at 11:49:30AM +0200, Dominic Jäger wrote: 
> > Allowing more descriptive names for network bridges and bonds makes 
> > their identification easier. 
> 
> there are probably a few more places where this would need to be 
> adjusted: 
> 
> /usr/share/perl5/PVE/FirewallSimulator.pm:395: } elsif ($route_state =~ m/^vmbr\d+$/) { 
> /usr/share/perl5/PVE/FirewallSimulator.pm:529: } elsif ($from =~ m|^(vmbr\d+)/(\S+)$|) { 
> /usr/share/perl5/PVE/FirewallSimulator.pm:567: } elsif ($to =~ m|^(vmbr\d+)/(\S+)$|) { 
> /usr/share/perl5/PVE/QemuServer.pm:5914: bridge => "vmbr$ind", 
> /usr/share/perl5/PVE/Service/pve_firewall.pm:287: pattern => '(host|outside|vm\d+|ct\d+|vmbr\d+/\S+)', 
> /usr/share/perl5/PVE/Service/pve_firewall.pm:294: pattern => '(host|outside|vm\d+|ct\d+|vmbr\d+/\S+)', 
> /usr/share/perl5/PVE/Network.pm:498: PVE::Tools::dir_glob_foreach($dir, '(((eth|bond)\d+|en[^.]+)(\.\d+)?)', sub { 
> 
> as well as the GUI part. 
> 
> per our discussion, maybe this + labels/comments on interfaces could 
> get a v2? ;) 
> 
> > Signed-off-by: Dominic Jäger <d.jaeger at proxmox.com> 
> > --- 
> > src/PVE/INotify.pm | 4 ++-- 
> > 1 file changed, 2 insertions(+), 2 deletions(-) 
> > 
> > diff --git a/src/PVE/INotify.pm b/src/PVE/INotify.pm 
> > index b660041..75e5010 100644 
> > --- a/src/PVE/INotify.pm 
> > +++ b/src/PVE/INotify.pm 
> > @@ -1027,7 +1027,7 @@ sub __read_etc_network_interfaces { 
> > 
> > foreach my $iface (keys %$ifaces) { 
> > my $d = $ifaces->{$iface}; 
> > - if ($iface =~ m/^bond\d+$/) { 
> > + if ($iface =~ m/^bond\w+$/) { 
> > if (!$d->{ovs_type}) { 
> > $d->{type} = 'bond'; 
> > } elsif ($d->{ovs_type} eq 'OVSBond') { 
> > @@ -1049,7 +1049,7 @@ sub __read_etc_network_interfaces { 
> > } else { 
> > $d->{type} = 'unknown'; 
> > } 
> > - } elsif ($iface =~ m/^vmbr\d+$/) { 
> > + } elsif ($iface =~ m/^vmbr\w+$/) { 
> > if (!$d->{ovs_type}) { 
> > $d->{type} = 'bridge'; 
> > 
> > -- 
> > 2.11.0 
> > 
> > _______________________________________________ 
> > pve-devel mailing list 
> > pve-devel at pve.proxmox.com 
> > https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel 
> 
> _______________________________________________ 
> pve-devel mailing list 
> pve-devel at pve.proxmox.com 
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel at pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel