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

[David Riley <d.riley@proxmox.com>]

thanks for the review.
I'll address all the nits mentioned in v3.
comments inline.

On 7/3/26 4:35 PM, Daniel Kral wrote:
> 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.

I think splitting it up and having concise commit messages for each part would
make the git history much cleaner and easier to follow.
Will most likely split this up.


>>   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!


Thanks, but I just noticed I should not match '\d+' here.
Will switch to using '[0-9]' [0].

[0] https://pve.proxmox.com/wiki/Perl_Style_Guide#Regular_Expressions


>> +                    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.


That's a great catch, thank you for the detailed breakdown.
I'll update this in v3.


>> +
>> +                    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
>