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 79FFB1FF0E9 for ; Thu, 16 Jul 2026 15:18:01 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id D12D421471; Thu, 16 Jul 2026 15:17:58 +0200 (CEST) Message-ID: <055351eb-f575-46c3-ac37-01925306c449@proxmox.com> Date: Thu, 16 Jul 2026 15:17:25 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH pve-firewall 2/5] ipset: Add option to update references on rename/delete To: pve-devel@lists.proxmox.com References: <20260407073658.90818-1-a.bied-charreton@proxmox.com> <20260407073658.90818-3-a.bied-charreton@proxmox.com> Content-Language: en-US From: Stefan Hanreich In-Reply-To: <20260407073658.90818-3-a.bied-charreton@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.111 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 RCVD_IN_DNSWL_LOW -0.7 Sender listed at https://www.dnswl.org/, low trust 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: FE7ZXL3DRCRU3JY7MGAP5KU3NKNUQLKP X-Message-ID-Hash: FE7ZXL3DRCRU3JY7MGAP5KU3NKNUQLKP X-MailFrom: s.hanreich@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 4/7/26 9:36 AM, Arthur Bied-Charreton wrote: > Renaming or deleting an IPSet that is referenced by rules or security > groups would leave dangling references, creating broken configurations. > > Add option to the POST and DELETE endpoints for /firewall/ipset to > update or delete those references from cluster, hosts, and guests > firewall configs. > > If these endpoints are hit from the cluster environment (/cluster/), > all firewall config files in the cluster (cluster + all nodes + all > guests) have to be checked and possibly updated. > > Renaming a cluster ipset referenced by 10.000 different config files in > a 3-node test cluster takes about 4.8 seconds on my machine. Doing the > same with an unreferenced ipset (i.e. config files checked but not > written back due to lack of changes) takes about 2 seconds. > > Signed-off-by: Arthur Bied-Charreton > --- > src/PVE/API2/Firewall/IPSet.pm | 108 ++++++++++++++++++++++++++++++++- > 1 file changed, 107 insertions(+), 1 deletion(-) > > diff --git a/src/PVE/API2/Firewall/IPSet.pm b/src/PVE/API2/Firewall/IPSet.pm > index 43a51a7..9896fdd 100644 > --- a/src/PVE/API2/Firewall/IPSet.pm > +++ b/src/PVE/API2/Firewall/IPSet.pm > @@ -5,6 +5,7 @@ use warnings; > use PVE::Exception qw(raise raise_param_exc); > use PVE::JSONSchema qw(get_standard_option); > > +use PVE::API2::Firewall::Helpers qw(filter_map foreach_conf_in_env); > use PVE::Firewall; > > use base qw(PVE::RESTHandler); > @@ -128,6 +129,92 @@ sub register_get_ipset { > }); > } > > +# Apply $rewrite to each rule in $conf referencing $ipset, modifying $conf in place. > +# > +# $rewrite ->($rule) is expected to return the new rule, or undef if the goal is to remove matches. > +# > +# Returns a boolean indicating whether $conf was modified. > +# > +# If $is_guest is set to 1, references will be solved assuming $conf is a guest FW config. > +# This is important because 2 different IPSets with the same name may be defined at the > +# cluster level and at the guest level, in which case the guest's IPSet shadows the cluster's. > +sub rewrite_ipset_refs_in_conf { > + my ($conf, $ipset, $rule_env, $is_guest, $rewrite) = @_; > + my $modified = 0; > + > + my @patterns; > + if ($rule_env eq 'cluster') { > + @patterns = ("+dc/$ipset"); > + # Guest config ipsets may shadow cluster config ones > + push @patterns, "+$ipset" if !($is_guest && $conf->{ipset}->{$ipset}); > + } else { > + @patterns = ("+$ipset", "+guest/$ipset"); > + } > + > + my $rule_matches = sub { > + my ($rule) = @_; > + grep { ($rule->{source} // '') eq $_ || ($rule->{dest} // '') eq $_ } @patterns; > + }; > + > + ($conf->{rules}, my $changed) = filter_map($conf->{rules}, $rewrite, $rule_matches); > + $modified ||= $changed; > + > + if ($rule_env eq 'cluster') { > + my $groups = $conf->{groups} // {}; > + for my $group (keys %$groups) { > + ($groups->{$group}, $changed) = filter_map($groups->{$group}, $rewrite, $rule_matches); > + $modified ||= $changed; > + } > + } > + > + return $modified; > +} > + > +# Apply $rewrite to each rule in the current environment ($rule_env) referencing $ipset, modifying > +# $conf in place. The caller is responsible for locking and saving $conf. > +# > +# If $rule_env is 'cluster', this function will also map over all guest and node firewall configs > +# in the cluster, locking and saving them in the process, since those may reference IPSets defined > +# in the cluster config. > +sub rewrite_ipset_refs_in_env { > + my ($conf, $ipset, $rule_env, $rewrite) = @_; > + return foreach_conf_in_env( > + $conf, > + $rule_env, > + sub { > + my ($fw_conf, $effective_env, $is_guest) = @_; > + return rewrite_ipset_refs_in_conf( > + $fw_conf, $ipset, $effective_env, $is_guest, $rewrite, > + ); > + }, > + ); > +} > + > +sub delete_ipset_refs { > + my ($conf, $ipset, $rule_env) = @_; > + return rewrite_ipset_refs_in_env($conf, lc($ipset), $rule_env, sub { undef }); > +} > + > +sub rename_ipset_refs { > + my ($conf, $ipset, $new_name, $rule_env) = @_; > + my $lc_ipset = lc($ipset); > + return rewrite_ipset_refs_in_env( > + $conf, > + $lc_ipset, > + $rule_env, > + sub { > + my ($rule) = @_; > + for my $field (qw(source dest)) { > + my $val = lc($rule->{$field} // ''); > + if ($val eq "+dc/$lc_ipset") { $rule->{$field} = "+dc/$new_name" } > + elsif ($val eq "+$lc_ipset") { $rule->{$field} = "+$new_name" } > + elsif ($val eq "+guest/$lc_ipset") { $rule->{$field} = "+guest/$new_name" } In the Firewall parsing logic itself (verify_rule in Firewall.pm), we do use regex matching: $value =~ m@^\+(guest/|dc/|sdn/)?(${ipset_name_pattern})$@ We could utilize that to rewrite this as: if ($value =~ m@^\+(guest/|dc/)?(${ipset_name_pattern})$@) { $rule->{$field} = "$1/$new_name" if $2 eq $lc_ipset; } but that might be a bit overkill / overcomplicated. I just figured I'd mention it since i've often written similar if / elsif chains myself that might have been better expressed by utilizing perl regexes. > + } > + return $rule; > + }, > + ); > +} > + All the helpers contained here, might better fit in the general Helper module, rather than the API module itself. Particularly if we implement similar functionality when deleting SDN VNets, pve-network would be able to use the Helper module instead of having to import API modules. > sub register_delete_ipset { > my ($class) = @_; > > @@ -139,6 +226,12 @@ sub register_delete_ipset { > optional => 1, > description => 'Delete all members of the IPSet, if there are any.', > }; > + $properties->{'delete-references'} = { > + type => 'boolean', > + optional => 1, > + description => 'Delete dangling references after deleting the IPSet', > + default => 0, > + }; > > $class->register_method({ > name => 'delete_ipset', > @@ -165,8 +258,10 @@ sub register_delete_ipset { > die "IPSet '$param->{name}' is not empty\n" > if scalar(@$ipset) && !$param->{force}; > > - $class->save_ipset($param, $fw_conf, undef); > + delete_ipset_refs($fw_conf, $param->{name}, $class->rule_env()) > + if $param->{'delete-references'}; > > + $class->save_ipset($param, $fw_conf, undef); > }, > ); > > @@ -654,6 +749,13 @@ sub register_create { > }, > ); > > + $properties->{'update-references'} = { > + type => 'boolean', > + optional => 1, > + description => 'Update dangling references when renaming an IPSet.', > + default => 0, > + }; > + > $class->register_method({ > name => 'create_ipset', > path => '', > @@ -688,6 +790,10 @@ sub register_create { > if $fw_conf->{ipset}->{ $param->{name} } > && $param->{name} ne $param->{rename}; > > + PVE::API2::Firewall::IPSetBase::rename_ipset_refs( > + $fw_conf, $param->{rename}, $param->{name}, $class->rule_env(), > + ) if $param->{'update-references'}; This suffers from timing issues, doesn't it? We update the references in all configuration files, but only save the new IPSet *after* we have updated all references. This leaves a window where rules get skipped by the firewall since they reference a not-yet-existing IPSet. In order to get atomicity we'd have to do it in two steps, wouldn't we? * Save the new ipset and write it to the configuration, but still keep the old one around. * update the references one-by-one, now at every point in time a rule references an IPSet that actually exists in the configuration * Delete the old IPSet Otherwise, introducing a sleep inbetween the rename call and the save_config call seems to make this surface easily: * Create an IPSet at cluster level * Reference the IPSet in a host rule * Update the name of the IPSet + update-references Some firewall cycles fail due to the rules getting updated, but not the IPSet name itself: > Jul 16 14:48:28 fw-testi pve-firewall[1213]: /etc/pve/nodes/fw-testi/host.fw (line 7) - errors in rule parameters: IN ACCEPT -source +dc/owo -log nolog > Jul 16 14:48:28 fw-testi pve-firewall[1213]: source: no such ipset 'owo' Delete seems fine, since all references are deleted and only then the IPSet itself afterwards.