From: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
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 [thread overview]
Message-ID: <20261022124708.429069-14-a.bied-charreton@proxmox.com> (raw)
In-Reply-To: <20261022124708.429069-1-a.bied-charreton@proxmox.com>
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 <a.bied-charreton@proxmox.com>
---
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
next prev parent reply other threads:[~2026-10-22 12:50 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-10-22 12:46 [PATCH container/docs/firewall/manager/network/qemu-server v4 00/27] handle dangling references when firewall objects go away Arthur Bied-Charreton
2026-10-22 12:46 ` [PATCH pve-firewall v4 01/27] firewall: move object reference helpers to their own module Arthur Bied-Charreton
2026-10-22 12:46 ` [PATCH pve-firewall v4 02/27] firewall: references: add helpers to update object references Arthur Bied-Charreton
2026-10-22 12:46 ` [PATCH pve-firewall v4 03/27] firewall: references: log the references an object update touched Arthur Bied-Charreton
2026-10-22 12:46 ` [PATCH pve-firewall v4 04/27] firewall: references: add wrappers for the SDN-generated IPSets Arthur Bied-Charreton
2026-10-22 12:46 ` [PATCH pve-firewall v4 05/27] firewall: parser: do not log errors for disabled rules Arthur Bied-Charreton
2026-10-22 12:46 ` [PATCH pve-firewall v4 06/27] firewall: ipset: add option to handle dangling references on rename Arthur Bied-Charreton
2026-10-22 12:46 ` [PATCH pve-firewall v4 07/27] firewall: ipset: add option to handle dangling references on delete Arthur Bied-Charreton
2026-10-22 12:46 ` [PATCH pve-firewall v4 08/27] firewall: aliases: add option to handle dangling references on rename Arthur Bied-Charreton
2026-10-22 12:46 ` [PATCH pve-firewall v4 09/27] firewall: aliases: add option to handle dangling references on delete Arthur Bied-Charreton
2026-10-22 12:46 ` [PATCH pve-firewall v4 10/27] firewall: api: run reference updates as tasks Arthur Bied-Charreton
2026-10-22 12:46 ` [PATCH pve-firewall v4 11/27] firewall: tests: add tests for object reference updates Arthur Bied-Charreton
2026-10-22 12:46 ` [PATCH pve-firewall v4 12/27] firewall: references: handle security groups Arthur Bied-Charreton
2026-10-22 12:46 ` Arthur Bied-Charreton [this message]
2026-10-22 12:46 ` [PATCH pve-firewall v4 14/27] fix #8176: firewall: groups: handle dangling references on delete Arthur Bied-Charreton
2026-10-22 12:46 ` [PATCH pve-network v4 15/27] apply: add option to handle dangling references on VNet deletion Arthur Bied-Charreton
2026-10-22 12:46 ` [PATCH qemu-server v4 16/27] api: destroy_vm: add option to handle dangling IPSet references Arthur Bied-Charreton
2026-10-22 12:46 ` [PATCH pve-container v4 17/27] " Arthur Bied-Charreton
2026-10-22 12:46 ` [PATCH pve-docs v4 18/27] firewall: remove trailing spaces Arthur Bied-Charreton
2026-10-22 12:47 ` [PATCH pve-docs v4 19/27] firewall: document renaming and removing objects Arthur Bied-Charreton
2026-10-22 12:47 ` [PATCH pve-manager v4 20/27] ui: firewall: add a common widget for handling dangling references Arthur Bied-Charreton
2026-10-22 12:47 ` [PATCH pve-manager v4 21/27] ui: firewall: ipset: add controls for " Arthur Bied-Charreton
2026-10-22 12:47 ` [PATCH pve-manager v4 22/27] ui: firewall: aliases: " Arthur Bied-Charreton
2026-10-22 12:47 ` [PATCH pve-manager v4 23/27] ui: sdn: apply: add control for dangling IPSet references Arthur Bied-Charreton
2026-10-22 12:47 ` [PATCH pve-manager v4 24/27] ui: guest destroy: use let for non-constant variable bindings Arthur Bied-Charreton
2026-10-22 12:47 ` [PATCH pve-manager v4 25/27] ui: guest destroy: add control for dangling IPSet references Arthur Bied-Charreton
2026-10-22 12:47 ` [PATCH pve-manager v4 26/27] ui: firewall: show the task for reference updates Arthur Bied-Charreton
2026-10-22 12:47 ` [PATCH pve-manager v4 27/27] ui: firewall: groups: add controls for dangling references Arthur Bied-Charreton
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20261022124708.429069-14-a.bied-charreton@proxmox.com \
--to=a.bied-charreton@proxmox.com \
--cc=pve-devel@lists.proxmox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox