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 A48141FF0C2 for ; Thu, 22 Oct 2026 14:50:17 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id EC04221A08; Thu, 22 Oct 2026 14:48:34 +0200 (CEST) From: Arthur Bied-Charreton To: pve-devel@lists.proxmox.com Subject: [PATCH pve-firewall v4 13/27] firewall: groups: add option to handle dangling references on rename Date: Thu, 22 Oct 2026 14:46:54 +0200 Message-ID: <20261022124708.429069-14-a.bied-charreton@proxmox.com> X-Mailer: git-send-email 2.47.3 In-Reply-To: <20261022124708.429069-1-a.bied-charreton@proxmox.com> References: <20261022124708.429069-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: 1792673237383 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.833 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: YXQSOX34BUU6SBY5TEJLO7SMHKOSMLAA X-Message-ID-Hash: YXQSOX34BUU6SBY5TEJLO7SMHKOSMLAA 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 a security group rewrote the references to it in the cluster, host and guest configs, but not in the VNet firewall configs. The firewall then failed to parse the rules referencing the group in those configs and dropped them. A rename could therefore disable a whole set of rules. Replace the hand-rolled walk with update_refs(), which covers vnet configs as well as all others, and take the 'dangling-references' option like the ipset and alias paths do. It defaults to 'rename' to stay consistent with previous behavior. The walk runs as a task, so the logs can be found in the task log afterwards. Signed-off-by: Arthur Bied-Charreton --- src/PVE/API2/Firewall/Groups.pm | 199 +++++++++++++++++--------------- 1 file changed, 106 insertions(+), 93 deletions(-) diff --git a/src/PVE/API2/Firewall/Groups.pm b/src/PVE/API2/Firewall/Groups.pm index d80a650..6b55c97 100644 --- a/src/PVE/API2/Firewall/Groups.pm +++ b/src/PVE/API2/Firewall/Groups.pm @@ -4,6 +4,7 @@ use strict; use warnings; use PVE::JSONSchema qw(get_standard_option); use PVE::Exception qw(raise raise_param_exc); +use PVE::Firewall::References qw(update_refs run_ref_update); use PVE::Firewall; use PVE::API2::Firewall::Rules; @@ -29,13 +30,20 @@ my $get_security_group_list = sub { return wantarray ? ($list, $digest) : $list; }; -my $rename_fw_rules = sub { - my ($old, $new, $rules) = @_; +my $check_rename = sub { + my ($cluster_conf, $param, $dangling_refs) = @_; - for my $rule (@{$rules}) { - next if ($rule->{type} ne "group" || $rule->{action} ne $old); - $rule->{action} = $new; - } + my (undef, $digest) = &$get_security_group_list($cluster_conf); + PVE::Tools::assert_if_modified($digest, $param->{digest}); + + raise_param_exc({ group => "Security group '$param->{rename}' does not exist" }) + if !$cluster_conf->{groups}->{ $param->{rename} }; + + # prevent overwriting an existing group + raise_param_exc({ group => "Security group '$param->{group}' already exists" }) + if $param->{group} ne $param->{rename} + && $cluster_conf->{groups}->{ $param->{group} } + && $dangling_refs ne 'force-rename'; }; __PACKAGE__->register_method({ @@ -100,113 +108,118 @@ __PACKAGE__->register_method({ }, ), digest => get_standard_option('pve-config-digest'), + 'dangling-references' => { + type => 'string', + enum => ['keep', 'disable', 'drop', 'rename', 'force-rename'], + optional => 1, + description => + "Handle references that renaming the security group would leave dangling. " + . "Use 'rename' to point them at the new name, 'disable' to disable the " + . "referencing rules, or 'drop' to remove them entirely. 'force-rename' " + . "also renames, but skips the check for an existing security group with " + . "the target name, to finish a rename interrupted partway.", + default => 'rename', + }, }, }, - returns => { type => 'null' }, + returns => { type => 'string', optional => 1 }, code => sub { my ($param) = @_; my $group = $param->{group}; my $rename = $param->{rename}; my $comment = $param->{comment}; + my $dangling_refs = $param->{'dangling-references'} // 'rename'; + + # 'force-rename' only differs in skipping the check above, the references are renamed + # either way + my $ref_action; + if ($dangling_refs eq 'force-rename') { + $ref_action = 'rename'; + } elsif ($dangling_refs ne 'keep') { + $ref_action = $dangling_refs; + } - PVE::Firewall::lock_clusterfw_conf( - 10, - sub { - my $cluster_conf = PVE::Firewall::load_clusterfw_conf(); + my $renaming = $rename && $rename ne $group; + my $subject = $renaming ? "security group '$rename' renamed to '$group'" : undef; - if ($rename) { - my (undef, $digest) = &$get_security_group_list($cluster_conf); - PVE::Tools::assert_if_modified($digest, $param->{digest}); + my $code = sub { + PVE::Firewall::lock_clusterfw_conf( + 10, + sub { + my $cluster_conf = PVE::Firewall::load_clusterfw_conf(); - raise_param_exc({ group => "Security group '$rename' does not exist" }) - if !$cluster_conf->{groups}->{$rename}; + if ($rename) { + $check_rename->($cluster_conf, $param, $dangling_refs); - # prevent overwriting an existing group - raise_param_exc({ group => "Security group '$group' does already exist" }) - if $cluster_conf->{groups}->{$group} - && $group ne $rename; + if ($renaming) { + # create an exact copy of the old security group + $cluster_conf->{groups}->{$group} = + $cluster_conf->{groups}->{$rename}; + $cluster_conf->{group_comments}->{$group} = + $cluster_conf->{group_comments}->{$rename}; + } - if ($rename eq $group) { - $cluster_conf->{group_comments}->{$rename} = $comment + $cluster_conf->{group_comments}->{$group} = $comment if defined($comment); - PVE::Firewall::save_clusterfw_conf($cluster_conf); - return; - } - - # Create an exact copy of the old security group - $cluster_conf->{groups}->{$group} = $cluster_conf->{groups}->{$rename}; - $cluster_conf->{group_comments}->{$group} = - $cluster_conf->{group_comments}->{$rename}; - - # Update comment if provided - $cluster_conf->{group_comments}->{$group} = $comment if defined($comment); - - # Write the copy to the cluster config, so that if something fails inbetween, the new firewall - # rules won't be broken when the new name is referenced - PVE::Firewall::save_clusterfw_conf($cluster_conf); - - # Update all the host configs to the new copy - my $hosts = PVE::Cluster::get_nodelist(); - foreach my $host (@$hosts) { - PVE::Firewall::lock_hostfw_conf( - $host, - 10, - sub { - my $host_conf_path = "/etc/pve/nodes/$host/host.fw"; - my $host_conf = - PVE::Firewall::load_hostfw_conf($cluster_conf, $host_conf_path); - - if (defined($host_conf)) { - &$rename_fw_rules($rename, $group, $host_conf->{rules}); - PVE::Firewall::save_hostfw_conf($host_conf, - $host_conf_path); - } - }, - ); - } - # Update all the VM configs - my $vms = PVE::Cluster::get_vmlist(); - foreach my $vm (keys %{ $vms->{ids} }) { - PVE::Firewall::lock_vmfw_conf( - $vm, - 10, - sub { - my $vm_type = $vms->{ids}->{$vm}->{type} eq "lxc" ? "ct" : "vm"; - my $vm_conf = PVE::Firewall::load_vmfw_conf( - $cluster_conf, $vm_type, $vm, "/etc/pve/firewall", - ); - - if (defined($vm_conf)) { - &$rename_fw_rules($rename, $group, $vm_conf->{rules}); - PVE::Firewall::save_vmfw_conf($vm, $vm_conf); - } - }, - ); + if ($renaming) { + my $saved = 0; + my $save = sub { + PVE::Firewall::save_clusterfw_conf($cluster_conf); + $saved = 1; + }; + + eval { + update_refs({ + conf => $cluster_conf, + kind => 'groups', + old => $rename, + new => $group, + env => 'cluster', + action => $ref_action, + subject => $subject, + save => $save, + }); + }; + if (my $err = $@) { + die "rename interrupted, the security group exists under both " + . "names; retry with 'force-rename' to finish: $err" + if $saved; + die "could not update references to security group " + . "'$rename': $err"; + } + + # now that everything has been updated, the old group can go + delete $cluster_conf->{groups}->{$rename}; + delete $cluster_conf->{group_comments}->{$rename}; + } + } else { + foreach my $name (keys %{ $cluster_conf->{groups} }) { + raise_param_exc({ + group => "Security group '$name' already exists" }) + if $name eq $group; + } + + $cluster_conf->{groups}->{$group} = []; + $cluster_conf->{group_comments}->{$group} = $comment + if defined($comment); } - # And also update the cluster itself - &$rename_fw_rules($rename, $group, $cluster_conf->{rules}); - - # Now that everything has been updated, the old rule can be deleted - delete $cluster_conf->{groups}->{$rename}; - delete $cluster_conf->{group_comments}->{$rename}; - } else { - foreach my $name (keys %{ $cluster_conf->{groups} }) { - raise_param_exc({ group => "Security group '$name' already exists" }) - if $name eq $group; - } + PVE::Firewall::save_clusterfw_conf($cluster_conf); + }, + ); + }; - $cluster_conf->{groups}->{$group} = []; - $cluster_conf->{group_comments}->{$group} = $comment if defined($comment); - } + if (!$renaming) { + $code->(); + return undef; + } - PVE::Firewall::save_clusterfw_conf($cluster_conf); - }, - ); + # fail on the request instead of inside the task. + $check_rename->(PVE::Firewall::load_clusterfw_conf(), $param, $dangling_refs); - return undef; + return run_ref_update('fw-group-rename', $rename, $code); }, }); -- 2.47.3