[pve-devel] [PATCH close #545, #5203: Allow bridges to have any valid interface name 0/2]

[pve-devel] [PATCH close #545, #5203: Allow bridges to have any valid interface name 0/2]

[Jillian Morgan]

Allow bridges to have any valid interface name

The patches are 1-line each, but across two repositories:
  proxmox-widget-toolkit (Toolkit.js)
  pve-common (INotify.pm)

Jillian Morgan (2):
  Allow bridges to have any valid interface name
  Detect bridge interface by bridge_ports attribute

 src/Toolkit.js | 2 +-
 src/PVE/INotify.pm | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

[pve-devel] [PATCH close #545, #5203: Allow bridges to have any valid interface name 1/2]

[Jillian Morgan]

Allow bridges to have any valid interface name
Repo: proxmox-widget-toolkit

Allow the web UI to accept bridge interfaces with any valid interface name,
rather than being limited to the arbitrary "vmbr" prefix.

Signed-off-by: Jillian Morgan <jillian.morgan@primordial.ca>
---
 proxmox-widget-toolkit/src/Toolkit.js | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/Toolkit.js b/src/Toolkit.js
index 6fd73f5..3a03f55 100644
--- a/src/Toolkit.js
+++ b/src/Toolkit.js
@@ -76,7 +76,7 @@ Ext.apply(Ext.form.field.VTypes, {
     MacPrefixText: gettext('Example') + ': 02:8f - ' + gettext('only unicast addresses are allowed'),
 
     BridgeName: function(v) {
-	return (/^vmbr\d{1,4}$/).test(v);
+	return (/^[a-z][a-z0-9_]{1,20}$/).test(v);
     },
     VlanName: function(v) {
        if (Proxmox.Utils.VlanInterface_match.test(v)) {

Re: [pve-devel] [PATCH close #545, #5203: Allow bridges to have any valid interface name 1/2]

[Stefan Hanreich]

On 1/26/24 18:17, Jillian Morgan wrote:
> Allow bridges to have any valid interface name
> Repo: proxmox-widget-toolkit
> 
> Allow the web UI to accept bridge interfaces with any valid interface name,
> rather than being limited to the arbitrary "vmbr" prefix.
> 
> Signed-off-by: Jillian Morgan <jillian.morgan@primordial.ca>
> ---
>  proxmox-widget-toolkit/src/Toolkit.js | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/Toolkit.js b/src/Toolkit.js
> index 6fd73f5..3a03f55 100644
> --- a/src/Toolkit.js
> +++ b/src/Toolkit.js
> @@ -76,7 +76,7 @@ Ext.apply(Ext.form.field.VTypes, {
>      MacPrefixText: gettext('Example') + ': 02:8f - ' + gettext('only unicast addresses are allowed'),
>  
>      BridgeName: function(v) {
> -	return (/^vmbr\d{1,4}$/).test(v);
> +	return (/^[a-z][a-z0-9_]{1,20}$/).test(v);

character limit for interfaces is 15 characters (excluding null
terminator), so we should set the regex accordingly

>      },
>      VlanName: function(v) {
>         if (Proxmox.Utils.VlanInterface_match.test(v)) {
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 

[pve-devel] [PATCH close #545, #5203: Allow bridges to have any valid interface name 2/2]

[Jillian Morgan]

Detect bridge interface by bridge_ports attribute
Repo: pve-common

Similar to other interface types, we can detect a bridge by the presense of
it's bridge_ports attribute rather than solely relying on the "vmbr" ifname
prefix heuristic.

Signed-off-by: Jillian Morgan <jillian.morgan@primordial.ca>
---
 pve-common/src/PVE/INotify.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/PVE/INotify.pm b/src/PVE/INotify.pm
index 6432295..9b45346 100644
--- a/src/PVE/INotify.pm
+++ b/src/PVE/INotify.pm
@@ -1053,7 +1053,7 @@ sub __read_etc_network_interfaces {
 		my $tag = &$extract_ovs_option($d, 'tag');
 		$d->{ovs_tag} = $tag if defined($tag);
 	    }
-	} elsif ($iface =~ m/^vmbr\d+$/) {
+	} elsif ($iface =~ m/^vmbr\d+$/ || $d->{'bridge_ports'}) {
 	    if (!$d->{ovs_type}) {
 		$d->{type} = 'bridge';
 		if (!defined ($d->{bridge_stp})) {

Re: [pve-devel] [PATCH close #545, #5203: Allow bridges to have any valid interface name 2/2]

[Stefan Hanreich]

On 1/26/24 18:17, Jillian Morgan wrote:
> Detect bridge interface by bridge_ports attribute
> Repo: pve-common
> 
> Similar to other interface types, we can detect a bridge by the presense of
> it's bridge_ports attribute rather than solely relying on the "vmbr" ifname
> prefix heuristic.
> 
> Signed-off-by: Jillian Morgan <jillian.morgan@primordial.ca>
> ---
>  pve-common/src/PVE/INotify.pm | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/PVE/INotify.pm b/src/PVE/INotify.pm
> index 6432295..9b45346 100644
> --- a/src/PVE/INotify.pm
> +++ b/src/PVE/INotify.pm
> @@ -1053,7 +1053,7 @@ sub __read_etc_network_interfaces {
>  		my $tag = &$extract_ovs_option($d, 'tag');
>  		$d->{ovs_tag} = $tag if defined($tag);
>  	    }
> -	} elsif ($iface =~ m/^vmbr\d+$/) {
> +	} elsif ($iface =~ m/^vmbr\d+$/ || $d->{'bridge_ports'}) {

This only works for detecting Linux bridges, we would need to add an
additional check for OVS bridges as well

>  	    if (!$d->{ovs_type}) {
>  		$d->{type} = 'bridge';
>  		if (!defined ($d->{bridge_stp})) {
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 

Re: [pve-devel] [PATCH close #545, #5203: Allow bridges to have any valid interface name 2/2]

[Stefan Hanreich]

Hit send a bit too early, had another remark on this

On 1/26/24 18:17, Jillian Morgan wrote:
> Detect bridge interface by bridge_ports attribute
> Repo: pve-common
> 
> Similar to other interface types, we can detect a bridge by the presense of
> it's bridge_ports attribute rather than solely relying on the "vmbr" ifname
> prefix heuristic.
> 
> Signed-off-by: Jillian Morgan <jillian.morgan@primordial.ca>
> ---
>  pve-common/src/PVE/INotify.pm | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/PVE/INotify.pm b/src/PVE/INotify.pm
> index 6432295..9b45346 100644
> --- a/src/PVE/INotify.pm
> +++ b/src/PVE/INotify.pm
> @@ -1053,7 +1053,7 @@ sub __read_etc_network_interfaces {
>  		my $tag = &$extract_ovs_option($d, 'tag');
>  		$d->{ovs_tag} = $tag if defined($tag);
>  	    }
> -	} elsif ($iface =~ m/^vmbr\d+$/) {
> +	} elsif ($iface =~ m/^vmbr\d+$/ || $d->{'bridge_ports'}) {

This only works for bridges that actually have a bridge port assigned.
In cases of bridges with no default bridge_port

$d->{'bridge_ports'}) === ''

which evaluates to false. Something like exists/defined would fit better.

>  	    if (!$d->{ovs_type}) {
>  		$d->{type} = 'bridge';
>  		if (!defined ($d->{bridge_stp})) {
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 

Re: [pve-devel] [PATCH close #545, #5203: Allow bridges to have any valid interface name 0/2]

[Stefan Hanreich]

Hi!

Thanks for your patch! In general we like the idea of being able to
choose arbitrary names for bridges - but there's some additional things
to consider before we can apply those patches.

We are currently using prefixes for other types of interfaces as well.
With this patch series I could create bridges named 'bondX' which are
then picked up as a bond by the Web UI since the check for that in
INotify.pm is located before the check for bridges.
It would be a bit weird to name a bridge with the prefix bond
admittedly, but we should try to handle this more gracefully (iow. check
first whether an entry is a bridge and only then check for the other cases)

It might make sense to check for any possible conflicts with the SDN
config (running & staged).

I've added some additional comments inline - I can look into a v2
addressing those issues.

On 1/26/24 18:17, Jillian Morgan wrote:
> Allow bridges to have any valid interface name
> 
> The patches are 1-line each, but across two repositories:
>   proxmox-widget-toolkit (Toolkit.js)
>   pve-common (INotify.pm)
> 
> Jillian Morgan (2):
>   Allow bridges to have any valid interface name
>   Detect bridge interface by bridge_ports attribute
> 
>  src/Toolkit.js | 2 +-
>  src/PVE/INotify.pm | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 

Re: [pve-devel] [PATCH close #545, #5203: Allow bridges to have any valid interface name 0/2]

[Thomas Lamprecht]

Am 22/02/2024 um 09:00 schrieb DERUMIER, Alexandre:
>>> It might make sense to check for any possible conflicts with the SDN
>>> config (running & staged).
> 
> Technically, ifupdown2 will try to merge config options, if the
> interface is defined in both /etc/network/interfaces  &&
> /etc/network/interfaces.d/

So we basically it could be fine to only show a warning (or hint) in either:

- node-specific bridge create if a vnet with that name already exists

- vnet create, if such a bridge already exists

And similar for edit and deletions, as a user might wonder why the bridge is
still there if one deleted either vnet or the bridge entry.

As then the user is aware of it and can better decide if they indeed to
create such a duplication.

That might be a bit more work for one not used to our code base though, so
for starters adding a hint to the documentation could be enough.