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 A78F31FF0E6 for ; Fri, 24 Jul 2026 10:40:55 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 63F4F2148A; Fri, 24 Jul 2026 10:40:55 +0200 (CEST) Date: Fri, 24 Jul 2026 10:40:50 +0200 From: Arthur Bied-Charreton To: Stefan Hanreich Subject: Re: [PATCH pve-firewall 2/5] ipset: Add option to update references on rename/delete Message-ID: References: <20260407073658.90818-1-a.bied-charreton@proxmox.com> <20260407073658.90818-3-a.bied-charreton@proxmox.com> <055351eb-f575-46c3-ac37-01925306c449@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <055351eb-f575-46c3-ac37-01925306c449@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1784882420546 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.766 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: YGSAF6JPMRLN7YEDM3MWFPVSXFXRYDLI X-Message-ID-Hash: YGSAF6JPMRLN7YEDM3MWFPVSXFXRYDLI X-MailFrom: a.bied-charreton@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 CC: pve-devel@lists.proxmox.com 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 Thu, Jul 16, 2026 at 03:17:25PM +0200, Stefan Hanreich wrote: > 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 > > --- [...] > > + > > +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. > thanks for mentioning! although i have to say i find the regex approach a lot harder to reason about here, i think i would stick with the if/elsif chain if that's okay with you. > > + } > > + 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. > good point, will move those over > > 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: > that's a very good point, thanks for catching that! i will go with your proposed alternative (create new & keep old -> update references -> delete old) in v2. > > 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. > > > > >