Re: [PATCH pve-access-control v2 05/10] fix #7294: acl: pool: add SDN VNets as pool members

["Daniel Kral" <d.kral@proxmox.com>]

On Fri Jun 26, 2026 at 3:10 PM CEST, David Riley wrote:
> Extend the pool configuration to allow SDN VNets as pool members by
> introducing a new 'network' property.
>
> The property tracks entries using a type prefix for future expansion:
> * vnet/<zone>/<vnet>
> * vnet/<zone>/<vnet>/<vlan>
>
> Adapt the path resolution for bridges to ensure pool configurations
> are considered. This is necessary to allow users to assign a VNet to
> a VM when they only have access to a specific VLAN tag. Note that if
> a VLAN tag is present in the pool configuration, the user is
> restricted to that specific tag and cannot assign the base VNet
> untagged.
>
> Update the parser_writer tests to verify serialization and parsing
> of the updated configuration format.
>
> Link: https://bugzilla.proxmox.com/show_bug.cgi?id=7294
> Signed-off-by: David Riley <d.riley@proxmox.com>
> ---

might make sense to move out the remove_vnet_from_pool() and
migrate_vnet_zone_in_pool() functions out to a separate patch as
removing/migrating the pool entries in case of zone reassignments, but
not sure about this.

>  src/PVE/AccessControl.pm  | 93 ++++++++++++++++++++++++++++++++++++---
>  src/PVE/RPCEnvironment.pm | 68 ++++++++++++++++++++++++++--
>  src/test/parser_writer.pl | 53 ++++++++++++++++++----
>  3 files changed, 198 insertions(+), 16 deletions(-)
>
> diff --git a/src/PVE/AccessControl.pm b/src/PVE/AccessControl.pm
> index 34e56b6..6217f10 100644
> --- a/src/PVE/AccessControl.pm
> +++ b/src/PVE/AccessControl.pm

[ snip ]

> @@ -1974,6 +1991,72 @@ sub remove_vm_from_pool {
>      lock_user_config($delVMfromPoolFn, "pool cleanup for VM $vmid failed");
>  }
>  
> +sub remove_vnet_from_pool {
> +    my ($zone, $vnet) = @_;
> +    my $network_key = "vnet/$zone/$vnet";
> +
> +    my $del_vnet_from_pool_fn = sub {
> +        my $usercfg = cfs_read_file("user.cfg");
> +        return if !$usercfg->{pools};

nit: early return is a bit unnecessary as an empty $usercfg->{pools}
     will essentially be skipping the loop anyway and not end up writing
     the file as $modified == 0

> +
> +        my $modified = 0;
> +        for my $pool (keys $usercfg->{pools}->%*) {
> +            my $pool_cfg = $usercfg->{pools}->{$pool};
> +            next if !$pool_cfg->{network};

same here

> +
> +            for my $net_key (keys $pool_cfg->{network}->%*) {
> +                if ($net_key eq $network_key || $net_key =~ m!^\Q$network_key\E/\d+$!) {

Nice usage of the quotemeta ops!

> +                    delete $pool_cfg->{network}->{$net_key};
> +                    $modified = 1;
> +                }
> +            }
> +        }
> +
> +        if ($modified) {
> +            cfs_write_file("user.cfg", $usercfg);
> +        }
> +    };
> +
> +    lock_user_config($del_vnet_from_pool_fn, "pool cleanup for VNet $vnet failed");
> +}
> +
> +sub migrate_vnet_zone_in_pool {
> +    my ($src_zone, $dest_zone, $vnet) = @_;
> +
> +    my $src_key = "vnet/$src_zone/$vnet";
> +    my $dest_key = "vnet/$dest_zone/$vnet";
> +
> +    my $update_vnet_zone_fn = sub {
> +        my $usercfg = cfs_read_file("user.cfg");
> +        return if !$usercfg->{pools};

same here as in remove_vnet_from_pool

> +
> +        my $modified = 0;
> +        for my $pool (keys $usercfg->{pools}->%*) {
> +            my $pool_cfg = $usercfg->{pools}->{$pool};
> +            next if !$pool_cfg->{network};

same here

> +
> +            for my $net_key (keys $pool_cfg->{network}->%*) {
> +                if ($net_key eq $src_key || $net_key =~ m!^\Q$src_key\E/(\d+)$!) {
> +                    my $vlan = $1;
> +                    delete $pool_cfg->{network}->{$net_key};

the delete while iterating over the same hash is not very robust... I'd
prefer a double pass even if it's marginally less efficient just to make
sure that there's nothing funky going on. But no very hard feelings
about this.



Besides that, this code is a little buggy: The if condition will
evaluate the first term $net_key eq $src_key for VNets without vlan
tags, but will also evaluate the second term with the regex for VNets
with vlan tags. So the $1 will only be overwritten if there was a VNet
with a vlan tag and will still be defined if a VNet without a vlan tag
comes afterwards.

Iterating hashes in Perl is random every run, so if the iteration is
something like this:

    - vnet/somezone/vnet1    -> vnet/otherzon/vnet1
    - vnet/somezone/vnet1.3  -> vnet/otherzon/vnet1.3
    - vnet/somezone/vnet1.2  -> vnet/otherzon/vnet1.2

Then everything is fine, but if we only slightly change it, it will be:

    - vnet/somezone/vnet1.3  -> vnet/otherzon/vnet1.3
    - vnet/somezone/vnet1    -> vnet/otherzon/vnet1.3
    - vnet/somezone/vnet1.2  -> vnet/otherzon/vnet1.2

So it will remove vnet1 SOMETIMES, because of the randomness here.

There are multiple ways to fix this, including sorting the keys and
always executing the regex with making the tag optional, so the $1 will
be overwritten every time.

> +
> +                    my $target_key = $dest_key;
> +                    $target_key .= "/$vlan" if defined($vlan);
> +                    $pool_cfg->{network}->{$target_key} = 1;
> +
> +                    $modified = 1;
> +                }
> +            }
> +        }
> +
> +        if ($modified) {
> +            cfs_write_file("user.cfg", $usercfg);
> +        }
> +    };
> +
> +    lock_user_config($update_vnet_zone_fn, "pool update for VNet $vnet failed");
> +}
> +
>  sub remove_sdn_resource_access {
>      my ($paths) = @_; # [ ['zones', '<zone>'], ['zones', '<zone>', '<vnet>'] ]
>  
> diff --git a/src/PVE/RPCEnvironment.pm b/src/PVE/RPCEnvironment.pm
> index 7591aa9..d9372a0 100644
> --- a/src/PVE/RPCEnvironment.pm
> +++ b/src/PVE/RPCEnvironment.pm
> @@ -53,6 +53,21 @@ my $compile_acl_path = sub {
>                      $data->{poolroles}->{"/storage/$storeid"}->{$role} = 1;
>                  }
>              }
> +
> +            for my $network_key (keys $d->{network}->%*) {
> +                my ($type, @path) = split('/', $network_key);
> +
> +                if ($type eq 'vnet') {
> +                    my ($zoneid, $vnetid, $vlan) = @path;
> +
> +                    my $acl_path = "/sdn/zones/$zoneid/$vnetid";
> +                    $acl_path = "$acl_path/$vlan" if defined($vlan);

nit: use the .= operator here

> +
> +                    for my $role (keys $pool_roles->%*) {
> +                        $data->{poolroles}->{$acl_path}->{$role} = 1;
> +                    }
> +                }
> +            }
>          }
>      }
>  

I'll review the rest from here on next week