From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [IPv6:2a0f:8001:1:32::40]) by lore.proxmox.com (Postfix) with ESMTPS id 1D7DD1FF142 for ; Fri, 03 Jul 2026 16:35:33 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 30DF221387; Fri, 03 Jul 2026 16:35:32 +0200 (CEST) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Fri, 03 Jul 2026 16:35:26 +0200 Message-Id: Subject: Re: [PATCH pve-access-control v2 05/10] fix #7294: acl: pool: add SDN VNets as pool members From: "Daniel Kral" To: "David Riley" , X-Mailer: aerc 0.21.0-147-g43ac7b685014-dirty References: <20260626131035.112374-1-d.riley@proxmox.com> <20260626131035.112374-6-d.riley@proxmox.com> In-Reply-To: <20260626131035.112374-6-d.riley@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1783089318404 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.025 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: NOG6GUBMI2NF5FSPZZU2Y7ECTZ7VLV52 X-Message-ID-Hash: NOG6GUBMI2NF5FSPZZU2Y7ECTZ7VLV52 X-MailFrom: d.kral@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: 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// > * vnet/// > > 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=3D7294 > Signed-off-by: David Riley > --- 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= "); > } > =20 > +sub remove_vnet_from_pool { > + my ($zone, $vnet) =3D @_; > + my $network_key =3D "vnet/$zone/$vnet"; > + > + my $del_vnet_from_pool_fn =3D sub { > + my $usercfg =3D 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 =3D=3D 0 > + > + my $modified =3D 0; > + for my $pool (keys $usercfg->{pools}->%*) { > + my $pool_cfg =3D $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 =3D~ m!^\Q$netw= ork_key\E/\d+$!) { Nice usage of the quotemeta ops! > + delete $pool_cfg->{network}->{$net_key}; > + $modified =3D 1; > + } > + } > + } > + > + if ($modified) { > + cfs_write_file("user.cfg", $usercfg); > + } > + }; > + > + lock_user_config($del_vnet_from_pool_fn, "pool cleanup for VNet $vne= t failed"); > +} > + > +sub migrate_vnet_zone_in_pool { > + my ($src_zone, $dest_zone, $vnet) =3D @_; > + > + my $src_key =3D "vnet/$src_zone/$vnet"; > + my $dest_key =3D "vnet/$dest_zone/$vnet"; > + > + my $update_vnet_zone_fn =3D sub { > + my $usercfg =3D cfs_read_file("user.cfg"); > + return if !$usercfg->{pools}; same here as in remove_vnet_from_pool > + > + my $modified =3D 0; > + for my $pool (keys $usercfg->{pools}->%*) { > + my $pool_cfg =3D $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 =3D~ m!^\Q$src_key\= E/(\d+)$!) { > + my $vlan =3D $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 =3D $dest_key; > + $target_key .=3D "/$vlan" if defined($vlan); > + $pool_cfg->{network}->{$target_key} =3D 1; > + > + $modified =3D 1; > + } > + } > + } > + > + if ($modified) { > + cfs_write_file("user.cfg", $usercfg); > + } > + }; > + > + lock_user_config($update_vnet_zone_fn, "pool update for VNet $vnet f= ailed"); > +} > + > sub remove_sdn_resource_access { > my ($paths) =3D @_; # [ ['zones', ''], ['zones', '', ''] ] > =20 > 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 =3D sub { > $data->{poolroles}->{"/storage/$storeid"}->{$role} = =3D 1; > } > } > + > + for my $network_key (keys $d->{network}->%*) { > + my ($type, @path) =3D split('/', $network_key); > + > + if ($type eq 'vnet') { > + my ($zoneid, $vnetid, $vlan) =3D @path; > + > + my $acl_path =3D "/sdn/zones/$zoneid/$vnetid"; > + $acl_path =3D "$acl_path/$vlan" if defined($vlan); nit: use the .=3D operator here > + > + for my $role (keys $pool_roles->%*) { > + $data->{poolroles}->{$acl_path}->{$role} =3D 1; > + } > + } > + } > } > } > =20 I'll review the rest from here on next week