From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id B48881FF142 for ; Tue, 07 Apr 2026 09:36:31 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 8A668E950; Tue, 7 Apr 2026 09:37:04 +0200 (CEST) From: Arthur Bied-Charreton To: pve-devel@lists.proxmox.com Subject: [PATCH pve-firewall 2/5] ipset: Add option to update references on rename/delete Date: Tue, 7 Apr 2026 09:36:55 +0200 Message-ID: <20260407073658.90818-3-a.bied-charreton@proxmox.com> X-Mailer: git-send-email 2.47.3 In-Reply-To: <20260407073658.90818-1-a.bied-charreton@proxmox.com> References: <20260407073658.90818-1-a.bied-charreton@proxmox.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-SPAM-LEVEL: Spam detection results: 0 AWL -0.245 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment KAM_LAZY_DOMAIN_SECURITY 1 Sending domain does not have any anti-forgery methods 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_VALIDITY_CERTIFIED_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RDNS_NONE 0.793 Delivered to internal network by a host with no rDNS SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_NONE 0.001 SPF: sender does not publish an SPF Record Message-ID-Hash: G3CWXSZIXGGSZVUYV3E64Z3ULYH5TSS5 X-Message-ID-Hash: G3CWXSZIXGGSZVUYV3E64Z3ULYH5TSS5 X-MailFrom: abied-charreton@jett.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: 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" } + } + return $rule; + }, + ); +} + 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'}; + my $data = delete $fw_conf->{ipset}->{ $param->{rename} }; $fw_conf->{ipset}->{ $param->{name} } = $data; if ( -- 2.47.3