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 BC05D1FF142 for ; Fri, 03 Jul 2026 11:42:55 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 827A121387; Fri, 03 Jul 2026 11:42:55 +0200 (CEST) Message-ID: <8b717041-cb10-4b48-a71e-e60b5398f2d7@proxmox.com> Date: Fri, 3 Jul 2026 11:42:17 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH pve-network v4 5/5] fix #7520: config: prune orphaned ACLs and relocate moved VNets To: Daniel Kral , pve-devel@lists.proxmox.com References: <20260626105258.56914-1-d.riley@proxmox.com> <20260626105258.56914-6-d.riley@proxmox.com> Content-Language: en-US From: David Riley In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1783071730494 X-SPAM-LEVEL: Spam detection results: 0 AWL -0.155 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) POISEN_SPAM_PILL 0.1 Meta: its spam POISEN_SPAM_PILL_1 0.1 random spam to be learned in bayes POISEN_SPAM_PILL_3 0.1 random spam to be learned in bayes 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: GUFABWCLLCUXIJ7QMBGA5E5SR233CLEX X-Message-ID-Hash: GUFABWCLLCUXIJ7QMBGA5E5SR233CLEX X-MailFrom: d.riley@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: Thanks for taking a look. Will address all the nits in v5. Some comments inline. On 7/2/26 3:26 PM, Daniel Kral wrote: > On Fri Jun 26, 2026 at 12:52 PM CEST, David Riley wrote: >> Compare the running configuration with the newly compiled state during >> config commit to identify and prune orphaned SDN ACL entries. This >> ensures state consistency for manual applies via the UI/API as well as >> during automatic configuration reloads on system boot. >> >> Track configuration changes across: >> * Zones >> * VNets >> * Fabrics >> * Controllers >> * Route maps >> * Prefix lists >> >> Trigger a validation check when a VNet is moved between zones to >> mitigate potential ACL path conflicts. Abort the apply operation if a >> conflict is detected to prevent configuration overwrites. If the path >> is clear, relocate the ACLs to the updated zone path. >> >> Suggested-by: Stefan Hanreich >> Signed-off-by: David Riley >> --- >> src/PVE/Network/SDN.pm | 141 +++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 141 insertions(+) >> >> diff --git a/src/PVE/Network/SDN.pm b/src/PVE/Network/SDN.pm >> index 33a3cf3..cd563d2 100644 >> --- a/src/PVE/Network/SDN.pm >> +++ b/src/PVE/Network/SDN.pm >> @@ -10,6 +10,7 @@ use LWP::UserAgent; >> use Net::SSLeay; >> use UUID; >> >> +use PVE::AccessControl; >> use PVE::Cluster qw(cfs_read_file cfs_write_file cfs_lock_file); >> use PVE::INotify; >> use PVE::RESTEnvironment qw(log_warn); >> @@ -238,11 +239,151 @@ sub compile_running_cfg { >> } >> >> sub commit_config { >> + my $old_cfg = cfs_read_file($RUNNING_CFG_FILENAME); >> my $cfg = compile_running_cfg(); >> >> + cleanup($old_cfg, $cfg); >> + >> cfs_write_file($RUNNING_CFG_FILENAME, $cfg); >> } >> >> +sub cleanup { > maybe a more telling name like cleanup_sdn_resource_acls(...) or > something similar? Otherwise, it might suggest that it cleans up > something in the SDN config. > Intentionally kept it generic, because I'm hooking into this cleanup routine in my Pool Patch series [0], which adds VNets as Pool Members. I use the Vnet diff to remove/move VNets in the pool configuration so that it stays in  sync with the state of the SDN configuration [1]. So if I name it cleanup_sdn_resource_acls it might not be obvious that it also touches the pool configuration, but one could argue that Pools are related to ACLs. I will most likely rename it to: cleanup_access_control [0] https://lore.proxmox.com/pve-devel/20260626131035.112374-1-d.riley@proxmox.com/ [1] https://lore.proxmox.com/pve-devel/20260626131035.112374-8-d.riley@proxmox.com/ >> + my ($old_cfg, $cfg) = @_; >> + if (!defined($old_cfg) || !defined($cfg)) { >> + return; >> + } > AFAICT $old_cfg and $cfg are always defined as the cfs_read_file() > parser for $RUNNING_CFG_FILENAME will always at least return an empty > hash ref and compile_running_cfg() will also always return a hash ref, > so this should not be needed? I think you are right. I was being very defensive in this patch series. Will drop this check. >> + >> + my $route_map_paths = diff_route_maps($old_cfg, $cfg); >> + my $generic_paths = diff_generic_resources($old_cfg, $cfg); >> + my ($vnet_delete_paths, $vnet_move_paths) = diff_vnets($old_cfg, $cfg); >> + >> + if (@$vnet_move_paths) { >> + PVE::AccessControl::migrate_sdn_resource_access($vnet_move_paths); >> + } >> + >> + my @paths_to_delete = (@$vnet_delete_paths, @$route_map_paths, @$generic_paths); >> + if (@paths_to_delete) { >> + PVE::AccessControl::remove_sdn_resource_access(\@paths_to_delete); >> + } >> + >> +} >> + >> +sub diff_generic_resources { >> + my ($old_cfg, $cfg) = @_; >> + >> + my @paths_to_delete; > nit: initialize with empty array () > >> + >> + my @types = qw(zones controllers fabrics prefix-lists); >> + for my $type (@types) { >> + if (!defined($old_cfg->{$type}) || !defined($old_cfg->{$type}->{ids})) { >> + next; >> + } >> + >> + my $old_ids = $old_cfg->{$type}->{ids}; >> + my $new_ids = {}; >> + >> + if (defined($cfg->{$type}) && defined($cfg->{$type}->{ids})) { >> + $new_ids = $cfg->{$type}->{ids}; >> + } > As compile_running_cfg() does always set the $cfg->{$type}->{ids} this > could probably be set unconditionally? That's true, will simplify that and drop the redundant definition checks. >> + >> + for my $id (keys %$old_ids) { >> + if (defined($new_ids->{$id})) { >> + next; >> + } > nit: use post-if statement > >> + >> + push(@paths_to_delete, [$type, $id]); >> + } >> + } >> + >> + return \@paths_to_delete; >> +} >> + >> +sub diff_route_maps { >> + my ($old_cfg, $cfg) = @_; >> + >> + my @paths_to_delete; > nit: initialize with empty array () > >> + >> + if (defined($old_cfg->{'route-maps'}) && defined($old_cfg->{'route-maps'}->{ids})) { >> + my $old_route_maps = $old_cfg->{'route-maps'}->{ids}; >> + my $route_map_suffix = qr/_[0-9]+$/; > nit: suffix with _re(gex) > >> + >> + my %active_route_maps; > nit: initialize with empty hash () > >> + if (defined($cfg->{'route-maps'}) && defined($cfg->{'route-maps'}->{ids})) { >> + for my $id (keys %{ $cfg->{'route-maps'}->{ids} }) { > nit: use postfix: keys $cfg->{'route-maps'}->{ids}->%* > >> + (my $base_name = $id) =~ s/$route_map_suffix//; >> + $active_route_maps{$base_name} = 1; >> + } >> + } >> + >> + my %queued_route_maps; > nit: initialize with empty hash () > >> + for my $id (keys %$old_route_maps) { >> + if (defined($cfg->{'route-maps'}->{ids}->{$id})) { >> + next; >> + } > nit: use post-if statement > >> + >> + (my $base_name = $id) =~ s/$route_map_suffix//; >> + >> + if ($active_route_maps{$base_name}) { >> + next; >> + } > nit: use post-if statement > >> + >> + if ($queued_route_maps{$base_name}) { >> + next; >> + } > nit: use post-if statement > >> + >> + push(@paths_to_delete, ['route-maps', $base_name]); >> + $queued_route_maps{$base_name} = 1; >> + } >> + } >> + >> + return \@paths_to_delete; >> +} >> + >> +sub diff_vnets { >> + my ($old_cfg, $cfg) = @_; >> + >> + my @paths_to_delete; >> + my @paths_to_move; > nit: initialize with empty arrays () > >> + >> + if (defined($old_cfg->{vnets}) && defined($old_cfg->{vnets}->{ids})) { >> + my $old_vnets = $old_cfg->{vnets}->{ids}; >> + my $new_vnets = {}; >> + >> + if (defined($cfg->{vnets}) && defined($cfg->{vnets}->{ids})) { >> + $new_vnets = $cfg->{vnets}->{ids}; >> + } > same as above, could be set unconditionally as compile_running_cfg() > always sets the $cfg->{vnets}->{ids} hash ref > >> + >> + for my $vnetid (keys %$old_vnets) { >> + my $old_zone = $old_vnets->{$vnetid}->{zone}; >> + >> + if (!defined($new_vnets->{$vnetid})) { >> + if (defined($old_zone)) { >> + push(@paths_to_delete, ['zones', $old_zone, $vnetid]); >> + } else { >> + log_warn( >> + "SDN Cleanup: Could not find zone for deleted VNet $vnetid, skipping ACL" >> + . " cleanup"); >> + } >> + next; >> + } >> + >> + my $new_zone = $new_vnets->{$vnetid}->{zone}; >> + >> + if (defined($old_zone) && defined($new_zone) && $old_zone ne $new_zone) { >> + push( >> + @paths_to_move, >> + { >> + src_path => ['zones', $old_zone, $vnetid], >> + dest_path => ['zones', $new_zone, $vnetid], >> + }, >> + ); >> + } > Hm, there's a lot of testing whether the zone exists... Can this > actually happen besides users directly editing the sdn/vnets.cfg file > and removing the vnet property? You are absolutely right, these checks don't add any real value since the API and parser enforce this. I refactor this in v5. > The API handler doesn't permit me to add a VNet without or with a > non-existing zone (only the zone 'abc' is existing; the error message is > a bit misleading, because the type is undefined here because the zone > does not exist at all): > > # pvesh create /cluster/sdn/vnets --vnet aeaewk --zone '' --type 'vnet' > create sdn vnet object failed: cannot lookup undefined type! at /usr/share/perl5/PVE/API2/Network/SDN/Vnets.pm line 326. > # pvesh create /cluster/sdn/vnets --vnet aeaewk --zone 'abc' --type 'vnet' > # pvesh create /cluster/sdn/vnets --vnet aeaewk2 --zone 'def' --type 'vnet' > create sdn vnet object failed: cannot lookup undefined type! at /usr/share/perl5/PVE/API2/Network/SDN/Vnets.pm line 326. > >> + } >> + } >> + >> + return (\@paths_to_delete, \@paths_to_move); >> +} >> + >> sub has_pending_changes { >> my $running_cfg = PVE::Network::SDN::running_config(); >> >