* [pve-devel] [PATCH pve-common] inotify: interfaces: also set type 'bridge' for empty bridges
@ 2025-08-07 14:21 Hannes Laimer
2025-08-08 8:40 ` Fabian Grünbichler
2025-08-08 9:33 ` [pve-devel] superseded: " Hannes Laimer
0 siblings, 2 replies; 5+ messages in thread
From: Hannes Laimer @ 2025-08-07 14:21 UTC (permalink / raw)
To: pve-devel
If a bridge has `bridge_ports` set to `none` we just skip the field.
Later we use the existance of the field to determine whether the type
should be `bridge`. This led to bridges without `bridge_ports` not
being recognized as bridges.
In the `/nodes/{}/network` we do permissions checks but only for ifaces
with type `bridge`(or `OVSBridge`). So interfaces were returned by the
endpoint even if the user did not have permissions the correct
permissions because the interface did not have type `bridge`.
This fixes this by also setting the type to `bridge` for empty bridges.
Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
src/PVE/INotify.pm | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/src/PVE/INotify.pm b/src/PVE/INotify.pm
index bbcb9f8..5a42ddd 100644
--- a/src/PVE/INotify.pm
+++ b/src/PVE/INotify.pm
@@ -994,7 +994,10 @@ SECTION: while (defined($line = <$fh>)) {
} elsif ($id eq 'slaves' || $id eq 'bridge_ports') {
my $devs = {};
foreach my $p (split(/\s+/, $value)) {
- next if $p eq 'none';
+ if ($p eq 'none') {
+ $d->{'is_empty_bridge'} = 1;
+ next;
+ }
$devs->{$p} = 1;
}
my $str = join(' ', sort keys %{$devs});
@@ -1077,7 +1080,8 @@ OUTER:
my $ip_link = $ip_links->{$altnames->{$iface} // $iface};
- if (defined $d->{'bridge_ports'}) {
+ if (defined $d->{'bridge_ports'} || $d->{'is_empty_bridge'}) {
+ delete $d->{'is_empty_bridge'} if defined $d->{'is_empty_bridge'};
$d->{type} = 'bridge';
if (!defined($d->{bridge_stp})) {
$d->{bridge_stp} = 'off';
--
2.47.2
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [pve-devel] [PATCH pve-common] inotify: interfaces: also set type 'bridge' for empty bridges
2025-08-07 14:21 [pve-devel] [PATCH pve-common] inotify: interfaces: also set type 'bridge' for empty bridges Hannes Laimer
@ 2025-08-08 8:40 ` Fabian Grünbichler
2025-08-08 8:58 ` Hannes Laimer
2025-08-08 9:33 ` [pve-devel] superseded: " Hannes Laimer
1 sibling, 1 reply; 5+ messages in thread
From: Fabian Grünbichler @ 2025-08-08 8:40 UTC (permalink / raw)
To: Proxmox VE development discussion
On August 7, 2025 4:21 pm, Hannes Laimer wrote:
> If a bridge has `bridge_ports` set to `none` we just skip the field.
> Later we use the existance of the field to determine whether the type
> should be `bridge`. This led to bridges without `bridge_ports` not
> being recognized as bridges.
>
> In the `/nodes/{}/network` we do permissions checks but only for ifaces
> with type `bridge`(or `OVSBridge`). So interfaces were returned by the
> endpoint even if the user did not have permissions the correct
> permissions because the interface did not have type `bridge`.
>
> This fixes this by also setting the type to `bridge` for empty bridges.
>
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> ---
Note: this patch is likely a candidate for stable-bookworm as well?
> src/PVE/INotify.pm | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/src/PVE/INotify.pm b/src/PVE/INotify.pm
> index bbcb9f8..5a42ddd 100644
> --- a/src/PVE/INotify.pm
> +++ b/src/PVE/INotify.pm
> @@ -994,7 +994,10 @@ SECTION: while (defined($line = <$fh>)) {
> } elsif ($id eq 'slaves' || $id eq 'bridge_ports') {
this elsif here is taken for both bonds and bridges
> my $devs = {};
> foreach my $p (split(/\s+/, $value)) {
> - next if $p eq 'none';
> + if ($p eq 'none') {
> + $d->{'is_empty_bridge'} = 1;
so this here should be guarded with `if $id eq 'bridge_ports'`..
do we also need special handling of "empty" bonds (which make less sense
in practice, but might still exist?) - in particular if we ever plan on
dropping the naming restrictions there?
> + next;
> + }
> $devs->{$p} = 1;
> }
> my $str = join(' ', sort keys %{$devs});
> @@ -1077,7 +1080,8 @@ OUTER:
>
> my $ip_link = $ip_links->{$altnames->{$iface} // $iface};
>
> - if (defined $d->{'bridge_ports'}) {
> + if (defined $d->{'bridge_ports'} || $d->{'is_empty_bridge'}) {
> + delete $d->{'is_empty_bridge'} if defined $d->{'is_empty_bridge'};
> $d->{type} = 'bridge';
> if (!defined($d->{bridge_stp})) {
> $d->{bridge_stp} = 'off';
> --
> 2.47.2
>
>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
>
>
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [pve-devel] [PATCH pve-common] inotify: interfaces: also set type 'bridge' for empty bridges
2025-08-08 8:40 ` Fabian Grünbichler
@ 2025-08-08 8:58 ` Hannes Laimer
2025-08-08 9:21 ` Fabian Grünbichler
0 siblings, 1 reply; 5+ messages in thread
From: Hannes Laimer @ 2025-08-08 8:58 UTC (permalink / raw)
To: Proxmox VE development discussion, Fabian Grünbichler
On 08.08.25 10:40, Fabian Grünbichler wrote:
> On August 7, 2025 4:21 pm, Hannes Laimer wrote:
>> If a bridge has `bridge_ports` set to `none` we just skip the field.
>> Later we use the existance of the field to determine whether the type
>> should be `bridge`. This led to bridges without `bridge_ports` not
>> being recognized as bridges.
>>
>> In the `/nodes/{}/network` we do permissions checks but only for ifaces
>> with type `bridge`(or `OVSBridge`). So interfaces were returned by the
>> endpoint even if the user did not have permissions the correct
>> permissions because the interface did not have type `bridge`.
>>
>> This fixes this by also setting the type to `bridge` for empty bridges.
>>
>> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
>> ---
>
> Note: this patch is likely a candidate for stable-bookworm as well?
>
yup, this is not new
>> src/PVE/INotify.pm | 8 ++++++--
>> 1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/PVE/INotify.pm b/src/PVE/INotify.pm
>> index bbcb9f8..5a42ddd 100644
>> --- a/src/PVE/INotify.pm
>> +++ b/src/PVE/INotify.pm
>> @@ -994,7 +994,10 @@ SECTION: while (defined($line = <$fh>)) {
>> } elsif ($id eq 'slaves' || $id eq 'bridge_ports') {
>
> this elsif here is taken for both bonds and bridges
>
true, thanks! missed that.
>> my $devs = {};
>> foreach my $p (split(/\s+/, $value)) {
>> - next if $p eq 'none';
>> + if ($p eq 'none') {
>> + $d->{'is_empty_bridge'} = 1;
>
> so this here should be guarded with `if $id eq 'bridge_ports'`..
>
will fix that
> do we also need special handling of "empty" bonds (which make less sense
> in practice, but might still exist?) - in particular if we ever plan on
> dropping the naming restrictions there?
>
we allow something like this
```
auto bond0
iface bond0 inet manual
bond-slaves none
bond-miimon 100
bond-mode balance-rr
```
to be set (even through the UI), but it does trip up `ifreload`. I think
we should at least not allow setting `bond_slaves none` through the UI.
>> + next;
>> + }
>> $devs->{$p} = 1;
>> }
>> my $str = join(' ', sort keys %{$devs});
>> @@ -1077,7 +1080,8 @@ OUTER:
>>
>> my $ip_link = $ip_links->{$altnames->{$iface} // $iface};
>>
>> - if (defined $d->{'bridge_ports'}) {
>> + if (defined $d->{'bridge_ports'} || $d->{'is_empty_bridge'}) {
>> + delete $d->{'is_empty_bridge'} if defined $d->{'is_empty_bridge'};
>> $d->{type} = 'bridge';
>> if (!defined($d->{bridge_stp})) {
>> $d->{bridge_stp} = 'off';
>> --
>> 2.47.2
>>
>>
>>
>> _______________________________________________
>> pve-devel mailing list
>> pve-devel@lists.proxmox.com
>> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>>
>>
>>
>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
>
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [pve-devel] [PATCH pve-common] inotify: interfaces: also set type 'bridge' for empty bridges
2025-08-08 8:58 ` Hannes Laimer
@ 2025-08-08 9:21 ` Fabian Grünbichler
0 siblings, 0 replies; 5+ messages in thread
From: Fabian Grünbichler @ 2025-08-08 9:21 UTC (permalink / raw)
To: Hannes Laimer, Proxmox VE development discussion
On August 8, 2025 10:58 am, Hannes Laimer wrote:
> On 08.08.25 10:40, Fabian Grünbichler wrote:
>> On August 7, 2025 4:21 pm, Hannes Laimer wrote:
>>> If a bridge has `bridge_ports` set to `none` we just skip the field.
>>> Later we use the existance of the field to determine whether the type
>>> should be `bridge`. This led to bridges without `bridge_ports` not
>>> being recognized as bridges.
>>>
>>> In the `/nodes/{}/network` we do permissions checks but only for ifaces
>>> with type `bridge`(or `OVSBridge`). So interfaces were returned by the
>>> endpoint even if the user did not have permissions the correct
>>> permissions because the interface did not have type `bridge`.
>>>
>>> This fixes this by also setting the type to `bridge` for empty bridges.
>>>
>>> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
>>> ---
>>
[..]
>> do we also need special handling of "empty" bonds (which make less sense
>> in practice, but might still exist?) - in particular if we ever plan on
>> dropping the naming restrictions there?
>>
>
> we allow something like this
> ```
> auto bond0
> iface bond0 inet manual
> bond-slaves none
> bond-miimon 100
> bond-mode balance-rr
> ```
> to be set (even through the UI), but it does trip up `ifreload`. I think
> we should at least not allow setting `bond_slaves none` through the UI.
not sure if somebody might use this as an intermediate state (without
applying)?
as in, remove bond port A (bond has no ports), add port A to other bond,
remove port B from other bond, add port B to this bond, apply?
IIRC we don't allow ports to be shared in multiple devices..
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
* [pve-devel] superseded: Re: [PATCH pve-common] inotify: interfaces: also set type 'bridge' for empty bridges
2025-08-07 14:21 [pve-devel] [PATCH pve-common] inotify: interfaces: also set type 'bridge' for empty bridges Hannes Laimer
2025-08-08 8:40 ` Fabian Grünbichler
@ 2025-08-08 9:33 ` Hannes Laimer
1 sibling, 0 replies; 5+ messages in thread
From: Hannes Laimer @ 2025-08-08 9:33 UTC (permalink / raw)
To: pve-devel
superseded-by:
https://lore.proxmox.com/pve-devel/20250808093203.17668-1-h.laimer@proxmox.com/T/#u
On 07.08.25 16:21, Hannes Laimer wrote:
> If a bridge has `bridge_ports` set to `none` we just skip the field.
> Later we use the existance of the field to determine whether the type
> should be `bridge`. This led to bridges without `bridge_ports` not
> being recognized as bridges.
>
> In the `/nodes/{}/network` we do permissions checks but only for ifaces
> with type `bridge`(or `OVSBridge`). So interfaces were returned by the
> endpoint even if the user did not have permissions the correct
> permissions because the interface did not have type `bridge`.
>
> This fixes this by also setting the type to `bridge` for empty bridges.
>
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> ---
> src/PVE/INotify.pm | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/src/PVE/INotify.pm b/src/PVE/INotify.pm
> index bbcb9f8..5a42ddd 100644
> --- a/src/PVE/INotify.pm
> +++ b/src/PVE/INotify.pm
> @@ -994,7 +994,10 @@ SECTION: while (defined($line = <$fh>)) {
> } elsif ($id eq 'slaves' || $id eq 'bridge_ports') {
> my $devs = {};
> foreach my $p (split(/\s+/, $value)) {
> - next if $p eq 'none';
> + if ($p eq 'none') {
> + $d->{'is_empty_bridge'} = 1;
> + next;
> + }
> $devs->{$p} = 1;
> }
> my $str = join(' ', sort keys %{$devs});
> @@ -1077,7 +1080,8 @@ OUTER:
>
> my $ip_link = $ip_links->{$altnames->{$iface} // $iface};
>
> - if (defined $d->{'bridge_ports'}) {
> + if (defined $d->{'bridge_ports'} || $d->{'is_empty_bridge'}) {
> + delete $d->{'is_empty_bridge'} if defined $d->{'is_empty_bridge'};
> $d->{type} = 'bridge';
> if (!defined($d->{bridge_stp})) {
> $d->{bridge_stp} = 'off';
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2025-08-08 9:32 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2025-08-07 14:21 [pve-devel] [PATCH pve-common] inotify: interfaces: also set type 'bridge' for empty bridges Hannes Laimer
2025-08-08 8:40 ` Fabian Grünbichler
2025-08-08 8:58 ` Hannes Laimer
2025-08-08 9:21 ` Fabian Grünbichler
2025-08-08 9:33 ` [pve-devel] superseded: " Hannes Laimer
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox