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 E268F1FF0B3 for ; Wed, 11 Nov 2026 14:20:26 +0100 (CET) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id E908B21697; Wed, 11 Nov 2026 14:19:34 +0100 (CET) From: Arthur Bied-Charreton To: pve-devel@lists.proxmox.com Subject: [PATCH pve-firewall v6 07/28] firewall: ipset: add option to handle dangling references on rename Date: Wed, 11 Nov 2026 14:18:40 +0100 Message-ID: <20261111131901.1791124-8-a.bied-charreton@proxmox.com> X-Mailer: git-send-email 2.47.3 In-Reply-To: <20261111131901.1791124-1-a.bied-charreton@proxmox.com> References: <20261111131901.1791124-1-a.bied-charreton@proxmox.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1794403148174 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.636 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) RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium 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: 4UIH53BN4NH24F42HZAXAHOGAIFALXEP X-Message-ID-Hash: 4UIH53BN4NH24F42HZAXAHOGAIFALXEP 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 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 an ipset still referenced by rules leaves dangling references. The firewall then fails to parse those rules during compilation and drops them. A rename can therefore effectively disable a whole set of rules. Add a 'dangling-references' option to the rename path offering to 'keep' the references around, 'disable' the referencing rules, 'drop' them, or to 'rename' the references to the new name. For cluster ipsets, this also covers references in downstream configs (host, guest and vnet). A rename that fails partway leaves both the old and the new ipset in the cluster config. Retrying it chokes on the check against overwriting an existing ipset. 'force-rename' bypasses that check, so an interrupted rename can be finished. Avoiding the half-updated state altogether would need either a transaction across every config in the cluster or a rollback mechanism, neither of which is really an option here. Signed-off-by: Arthur Bied-Charreton --- src/PVE/API2/Firewall/IPSet.pm | 66 +++++++++++++++++++++++++++------- src/PVE/Firewall.pm | 24 +++++++++++++ 2 files changed, 78 insertions(+), 12 deletions(-) diff --git a/src/PVE/API2/Firewall/IPSet.pm b/src/PVE/API2/Firewall/IPSet.pm index 82b9aaf..9d3c562 100644 --- a/src/PVE/API2/Firewall/IPSet.pm +++ b/src/PVE/API2/Firewall/IPSet.pm @@ -534,6 +534,7 @@ package PVE::API2::Firewall::BaseIPSetList; use strict; use warnings; +use PVE::Firewall::References qw(update_refs); use PVE::JSONSchema qw(get_standard_option); use PVE::Exception qw(raise_param_exc); use PVE::Firewall; @@ -661,6 +662,9 @@ sub register_create { }, ); + $properties->{'dangling-references'} = get_standard_option('pve-dangling-references-rename'); + $properties->{'force-rename'} = get_standard_option('pve-dangling-references-force-rename'); + $class->register_method({ name => 'create_ipset', path => '', @@ -681,6 +685,11 @@ sub register_create { sub { my ($param) = @_; + my $dangling_refs = $param->{'dangling-references'} + // $properties->{'dangling-references'}->{default}; + my $force = $param->{'force-rename'} + // $properties->{'force-rename'}->{default}; + my ($cluster_conf, $fw_conf) = $class->load_config($param); if ($param->{rename}) { @@ -690,19 +699,52 @@ sub register_create { raise_param_exc({ name => "IPSet '$param->{rename}' does not exist" }) if !$fw_conf->{ipset}->{ $param->{rename} }; - # prevent overwriting existing ipset - raise_param_exc({ name => "IPSet '$param->{name}' does already exist" }) - if $fw_conf->{ipset}->{ $param->{name} } - && $param->{name} ne $param->{rename}; - - my $data = delete $fw_conf->{ipset}->{ $param->{rename} }; - $fw_conf->{ipset}->{ $param->{name} } = $data; - if ( - my $comment = - delete $fw_conf->{ipset_comments}->{ $param->{rename} } - ) { - $fw_conf->{ipset_comments}->{ $param->{name} } = $comment; + if ($param->{name} ne $param->{rename}) { + # prevent overwriting existing ipset + raise_param_exc({ name => "IPSet '$param->{name}' already exists" }) + if $fw_conf->{ipset}->{ $param->{name} } + && !$force; + + $fw_conf->{ipset}->{ $param->{name} } = + $fw_conf->{ipset}->{ $param->{rename} }; + + my $saved = 0; + my $save = sub { + $class->save_config($param, $fw_conf); + $saved = 1; + }; + + eval { + update_refs({ + conf => $fw_conf, + kind => 'ipset', + old => $param->{rename}, + new => $param->{name}, + env => $class->rule_env(), + action => $dangling_refs eq 'keep' ? undef : $dangling_refs, + subject => + "ipset '$param->{rename}' renamed to '$param->{name}'", + save => $save, + }); + }; + if (my $err = $@) { + die "rename interrupted, the IPSet exists under both names; " + . "retry with 'force-rename' to finish: $err" + if $saved; + die "could not update references to IPSet " + . "'$param->{rename}': $err"; + } + + delete $fw_conf->{ipset}->{ $param->{rename} }; + + if ( + my $comment = + delete $fw_conf->{ipset_comments}->{ $param->{rename} } + ) { + $fw_conf->{ipset_comments}->{ $param->{name} } = $comment; + } } + $fw_conf->{ipset_comments}->{ $param->{name} } = $param->{comment} if defined($param->{comment}); } else { diff --git a/src/PVE/Firewall.pm b/src/PVE/Firewall.pm index 6ce5306..d4e1ad9 100644 --- a/src/PVE/Firewall.pm +++ b/src/PVE/Firewall.pm @@ -210,6 +210,30 @@ PVE::JSONSchema::register_standard_option( }, ); +PVE::JSONSchema::register_standard_option( + 'pve-dangling-references-rename', + { + type => 'string', + enum => ['keep', 'disable', 'drop', 'rename'], + optional => 1, + description => "Handle references that renaming this object would leave dangling. Use " + . "'rename' to point them at the new name, 'disable' to disable the referencing rules, or " + . "'drop' to remove them entirely.", + default => 'keep', + }, +); + +PVE::JSONSchema::register_standard_option( + 'pve-dangling-references-force-rename', + { + type => 'boolean', + optional => 1, + description => "Go on with the rename even if an object with the target name already " + . "exists, overwriting it. 'dangling-references' is still applied the same.", + default => 0, + }, +); + my $feature_ipset_nomatch = 0; eval { my (undef, undef, $release) = POSIX::uname(); -- 2.47.3