From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [45.144.208.40]) by lore.proxmox.com (Postfix) with ESMTPS id D90991FF0B3 for ; Wed, 11 Nov 2026 14:22:54 +0100 (CET) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 10C0521995; Wed, 11 Nov 2026 14:20:13 +0100 (CET) From: Arthur Bied-Charreton To: pve-devel@lists.proxmox.com Subject: [PATCH pve-firewall v6 14/28] firewall: groups: add option to handle dangling references on rename Date: Wed, 11 Nov 2026 14:18:47 +0100 Message-ID: <20261111131901.1791124-15-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: 1794403148576 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.608 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: 7VIDMAU65MVUFYBWZXAZLRUSELIRCXWH X-Message-ID-Hash: 7VIDMAU65MVUFYBWZXAZLRUSELIRCXWH 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. If that option is passed, the rename runs as a task so changes to the configs can be found in the task log afterwards. Signed-off-by: Arthur Bied-Charreton --- src/PVE/API2/Firewall/Groups.pm | 182 ++++++++++++++++---------------- 1 file changed, 89 insertions(+), 93 deletions(-) diff --git a/src/PVE/API2/Firewall/Groups.pm b/src/PVE/API2/Firewall/Groups.pm index d80a650..7df75eb 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, $force) = @_; - 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} } + && !$force; }; __PACKAGE__->register_method({ @@ -100,113 +108,101 @@ __PACKAGE__->register_method({ }, ), digest => get_standard_option('pve-config-digest'), + 'dangling-references' => get_standard_option( + 'pve-dangling-references-rename', { default => 'rename' }, + ), + 'force-rename' => get_standard_option('pve-dangling-references-force-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'; + my $force = $param->{'force-rename'} // 0; - PVE::Firewall::lock_clusterfw_conf( - 10, - sub { - my $cluster_conf = PVE::Firewall::load_clusterfw_conf(); + my $renaming = $rename && $rename ne $group; - 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, $force); - # 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 => $dangling_refs eq 'keep' ? undef : $dangling_refs, + subject => "security group '$rename' renamed to '$group'", + 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 || !defined($param->{'dangling-references'})) { + $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, $force); - return undef; + return run_ref_update('fw-group-rename', $rename, $code); }, }); -- 2.47.3