* [PATCH pve-firewall v6 01/28] firewall: run make tidy
2026-11-11 13:18 [PATCH container/docs/firewall/manager/network/qemu-server v6 00/28] handle dangling references when firewall objects go away Arthur Bied-Charreton
@ 2026-11-11 13:18 ` Arthur Bied-Charreton
2026-11-11 13:18 ` [PATCH pve-firewall v6 02/28] firewall: move object reference helpers to their own module Arthur Bied-Charreton
` (26 subsequent siblings)
27 siblings, 0 replies; 29+ messages in thread
From: Arthur Bied-Charreton @ 2026-11-11 13:18 UTC (permalink / raw)
To: pve-devel
No functional changes intended.
Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
---
src/PVE/API2/Firewall/Helpers.pm | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/PVE/API2/Firewall/Helpers.pm b/src/PVE/API2/Firewall/Helpers.pm
index d0decae..4c78924 100644
--- a/src/PVE/API2/Firewall/Helpers.pm
+++ b/src/PVE/API2/Firewall/Helpers.pm
@@ -52,7 +52,8 @@ sub check_vnet_access {
my ($vnetid, $privileges) = @_;
my $running_config = PVE::Network::SDN::Vnets::config(1);
- my $_vnet = PVE::Network::SDN::Vnets::api_get_vnet_checked($vnetid, $privileges, $running_config);
+ my $_vnet =
+ PVE::Network::SDN::Vnets::api_get_vnet_checked($vnetid, $privileges, $running_config);
}
1;
--
2.47.3
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH pve-firewall v6 02/28] firewall: move object reference helpers to their own module
2026-11-11 13:18 [PATCH container/docs/firewall/manager/network/qemu-server v6 00/28] handle dangling references when firewall objects go away Arthur Bied-Charreton
2026-11-11 13:18 ` [PATCH pve-firewall v6 01/28] firewall: run make tidy Arthur Bied-Charreton
@ 2026-11-11 13:18 ` Arthur Bied-Charreton
2026-11-11 13:18 ` [PATCH pve-firewall v6 03/28] firewall: references: add helpers to update object references Arthur Bied-Charreton
` (25 subsequent siblings)
27 siblings, 0 replies; 29+ messages in thread
From: Arthur Bied-Charreton @ 2026-11-11 13:18 UTC (permalink / raw)
To: pve-devel
The following commits add a lot of functionality related to handling
references to firewall objects in the different configuration files.
Create a new PVE::Firewall::References module grouping this
functionality instead of accumulating it in the generic Helpers module,
and move collect_refs() there.
No functional changes intended.
Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
---
src/PVE/API2/Firewall/Cluster.pm | 6 ++--
src/PVE/API2/Firewall/VM.pm | 7 +++--
src/PVE/Firewall/Helpers.pm | 38 -------------------------
src/PVE/Firewall/Makefile | 2 +-
src/PVE/Firewall/References.pm | 48 ++++++++++++++++++++++++++++++++
5 files changed, 57 insertions(+), 44 deletions(-)
create mode 100644 src/PVE/Firewall/References.pm
diff --git a/src/PVE/API2/Firewall/Cluster.pm b/src/PVE/API2/Firewall/Cluster.pm
index ebd955d..62f596d 100644
--- a/src/PVE/API2/Firewall/Cluster.pm
+++ b/src/PVE/API2/Firewall/Cluster.pm
@@ -6,6 +6,7 @@ use PVE::Exception qw(raise raise_param_exc raise_perm_exc);
use PVE::JSONSchema qw(get_standard_option);
use PVE::Firewall;
+use PVE::Firewall::References;
use PVE::API2::Firewall::Aliases;
use PVE::API2::Firewall::Rules;
use PVE::API2::Firewall::Groups;
@@ -273,8 +274,9 @@ __PACKAGE__->register_method({
my $allowed_vnets = PVE::API2::Firewall::Helpers::get_allowed_vnets();
my $sdn_conf = PVE::Firewall::load_sdn_conf($allowed_vms, $allowed_vnets);
- my $cluster_refs = PVE::Firewall::Helpers::collect_refs($conf, $param->{type}, "dc");
- my $sdn_refs = PVE::Firewall::Helpers::collect_refs($sdn_conf, $param->{type}, "sdn");
+ my $cluster_refs = PVE::Firewall::References::collect_refs($conf, $param->{type}, "dc");
+ my $sdn_refs =
+ PVE::Firewall::References::collect_refs($sdn_conf, $param->{type}, "sdn");
return [@$sdn_refs, @$cluster_refs];
},
diff --git a/src/PVE/API2/Firewall/VM.pm b/src/PVE/API2/Firewall/VM.pm
index c3a5acc..0c556b9 100644
--- a/src/PVE/API2/Firewall/VM.pm
+++ b/src/PVE/API2/Firewall/VM.pm
@@ -7,6 +7,7 @@ use PVE::Exception qw(raise_param_exc);
use PVE::JSONSchema qw(get_standard_option);
use PVE::Cluster;
use PVE::Firewall;
+use PVE::Firewall::References;
use PVE::API2::Firewall::Rules;
use PVE::API2::Firewall::Helpers;
use PVE::API2::Firewall::Aliases;
@@ -300,11 +301,11 @@ sub register_handlers {
my $sdn_conf = PVE::Firewall::load_sdn_conf($allowed_vms, $allowed_vnets);
my $dc_refs =
- PVE::Firewall::Helpers::collect_refs($cluster_conf, $param->{type}, 'dc');
+ PVE::Firewall::References::collect_refs($cluster_conf, $param->{type}, 'dc');
my $sdn_refs =
- PVE::Firewall::Helpers::collect_refs($sdn_conf, $param->{type}, "sdn");
+ PVE::Firewall::References::collect_refs($sdn_conf, $param->{type}, "sdn");
my $vm_refs =
- PVE::Firewall::Helpers::collect_refs($fw_conf, $param->{type}, 'guest');
+ PVE::Firewall::References::collect_refs($fw_conf, $param->{type}, 'guest');
return [@$dc_refs, @$sdn_refs, @$vm_refs];
},
diff --git a/src/PVE/Firewall/Helpers.pm b/src/PVE/Firewall/Helpers.pm
index fa3646c..eb8b58a 100644
--- a/src/PVE/Firewall/Helpers.pm
+++ b/src/PVE/Firewall/Helpers.pm
@@ -16,7 +16,6 @@ our @EXPORT_OK = qw(
lock_vmfw_conf
remove_vmfw_conf
clone_vmfw_conf
- collect_refs
flush_fw_ct_entries_by_mark
);
@@ -153,43 +152,6 @@ sub dump_fw_logfile {
return ($state{'count'}, $state{'lines'});
}
-sub collect_refs {
- my ($conf, $type, $scope) = @_;
-
- my $res = [];
-
- if (!$type || $type eq 'ipset') {
- foreach my $name (keys %{ $conf->{ipset} }) {
- my $data = {
- type => 'ipset',
- name => $name,
- ref => "+$name",
- scope => $scope,
- };
- if (my $comment = $conf->{ipset_comments}->{$name}) {
- $data->{comment} = $comment;
- }
- push @$res, $data;
- }
- }
-
- if (!$type || $type eq 'alias') {
- foreach my $name (keys %{ $conf->{aliases} }) {
- my $e = $conf->{aliases}->{$name};
- my $data = {
- type => 'alias',
- name => $name,
- ref => $name,
- scope => $scope,
- };
- $data->{comment} = $e->{comment} if $e->{comment};
- push @$res, $data;
- }
- }
-
- return $res;
-}
-
# This is checked in proxmox-firewall to avoid log-spam due to failing to parse the config
our $FORCE_NFT_DISABLE_FLAG_FILE = "/run/proxmox-nftables-firewall-force-disable";
diff --git a/src/PVE/Firewall/Makefile b/src/PVE/Firewall/Makefile
index 3274bc6..6c1dae4 100644
--- a/src/PVE/Firewall/Makefile
+++ b/src/PVE/Firewall/Makefile
@@ -2,7 +2,7 @@ DESTDIR=
PREFIX=/usr
PERLDIR=$(DESTDIR)/$(PREFIX)/share/perl5
-SOURCES=Helpers.pm
+SOURCES=Helpers.pm References.pm
.PHONY: install
install: $(SOURCES)
diff --git a/src/PVE/Firewall/References.pm b/src/PVE/Firewall/References.pm
new file mode 100644
index 0000000..a4ea967
--- /dev/null
+++ b/src/PVE/Firewall/References.pm
@@ -0,0 +1,48 @@
+package PVE::Firewall::References;
+
+use strict;
+use warnings;
+
+use base 'Exporter';
+our @EXPORT_OK = qw(
+ collect_refs
+);
+
+sub collect_refs {
+ my ($conf, $type, $scope) = @_;
+
+ my $res = [];
+
+ if (!$type || $type eq 'ipset') {
+ foreach my $name (keys %{ $conf->{ipset} }) {
+ my $data = {
+ type => 'ipset',
+ name => $name,
+ ref => "+$name",
+ scope => $scope,
+ };
+ if (my $comment = $conf->{ipset_comments}->{$name}) {
+ $data->{comment} = $comment;
+ }
+ push @$res, $data;
+ }
+ }
+
+ if (!$type || $type eq 'alias') {
+ foreach my $name (keys %{ $conf->{aliases} }) {
+ my $e = $conf->{aliases}->{$name};
+ my $data = {
+ type => 'alias',
+ name => $name,
+ ref => $name,
+ scope => $scope,
+ };
+ $data->{comment} = $e->{comment} if $e->{comment};
+ push @$res, $data;
+ }
+ }
+
+ return $res;
+}
+
+1;
--
2.47.3
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH pve-firewall v6 03/28] firewall: references: add helpers to update object references
2026-11-11 13:18 [PATCH container/docs/firewall/manager/network/qemu-server v6 00/28] handle dangling references when firewall objects go away Arthur Bied-Charreton
2026-11-11 13:18 ` [PATCH pve-firewall v6 01/28] firewall: run make tidy Arthur Bied-Charreton
2026-11-11 13:18 ` [PATCH pve-firewall v6 02/28] firewall: move object reference helpers to their own module Arthur Bied-Charreton
@ 2026-11-11 13:18 ` Arthur Bied-Charreton
2026-11-11 13:18 ` [PATCH pve-firewall v6 04/28] firewall: references: log the references an object update touched Arthur Bied-Charreton
` (24 subsequent siblings)
27 siblings, 0 replies; 29+ messages in thread
From: Arthur Bied-Charreton @ 2026-11-11 13:18 UTC (permalink / raw)
To: pve-devel
Renaming or deleting a firewall object (ipset or alias) that is still
referenced by rules, security groups or ipset members leaves dangling
references. The firewall fails to parse the affected rules and drops
them, so an edit can effectively disable a whole set of rules.
Introduce the update_refs() helper, which finds such references and
applies one of three actions to them: 'rename' points them at the new
name, 'disable' turns off the referencing rules, and 'drop' removes
them. An ipset member has no disabled state, so 'disable' removes
members as well. Matching is case-insensitive and rewritten references
are normalized to lowercase.
When operating on an object defined in the cluster config, update_refs()
also covers every downstream config (guest, host and vnet) across the
cluster. Downstream configs are locked and saved individually as they
are visited, so on a cluster object rename the config must be saved
with the new object present before updating the downstream configs;
otherwise a concurrent firewall compilation would choke on a reference
to an object that does not exist yet and drop the rule. For this
purpose, update_refs() takes a $save callback, which allows it to
decide when such a config save is required instead of relying on the
caller for correctness.
Object references are not guaranteed to be scoped (dc/, guest/, sdn/)
if the rules have been added by manually editing the configs (or before
scoped references were introduced). This is not an issue for cluster,
host and vnet configs, who cannot define their own objects and
therefore can only reference cluster objects. Guest configs may however
define their own objects. Unscoped references in guest rules are
therefore resolved by first checking for a definition in the relevant
guest config, and only then in the cluster config, to prevent
overwriting the wrong reference.
SDN IPSets can additionally be shadowed by IPSets defined in the cluster
config. This is handled by also checking for same-named IPSets in the
cluster config for SDN IPSets.
Note that renaming an unscoped reference that previously resolved to a
cluster object may cause it to be shadowed by a guest object with the
same name as the rename target's name. This is accepted, as properly
fixing this would require rewriting the references scoped based on how
the old name resolved, which could break existing automation. We plan to
address this with a clean break in PVE 10.
The ipset and alias endpoints in the following commits build on this.
Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
---
src/PVE/Firewall.pm | 6 +
src/PVE/Firewall/References.pm | 334 +++++++++++++++++++++++++++++++++
2 files changed, 340 insertions(+)
diff --git a/src/PVE/Firewall.pm b/src/PVE/Firewall.pm
index dd5d92c..4631b5a 100644
--- a/src/PVE/Firewall.pm
+++ b/src/PVE/Firewall.pm
@@ -4392,6 +4392,12 @@ sub save_vnetfw_conf {
}
}
+sub list_vnetfw_confs {
+ my $vnets = [];
+ dir_glob_foreach($vnetfw_conf_dir, qr/(.+)\.fw/, sub { push $vnets->@*, $_[1] });
+ return [sort $vnets->@*];
+}
+
sub compile {
my ($cluster_conf, $hostfw_conf, $vmdata, $corosync_conf) = @_;
diff --git a/src/PVE/Firewall/References.pm b/src/PVE/Firewall/References.pm
index a4ea967..9eb5bc8 100644
--- a/src/PVE/Firewall/References.pm
+++ b/src/PVE/Firewall/References.pm
@@ -3,9 +3,13 @@ package PVE::Firewall::References;
use strict;
use warnings;
+use PVE::Cluster;
+use PVE::Firewall;
+
use base 'Exporter';
our @EXPORT_OK = qw(
collect_refs
+ update_refs
);
sub collect_refs {
@@ -45,4 +49,334 @@ sub collect_refs {
return $res;
}
+=head3 rewrite_refs_in_env($conf, $rule_env, $op)
+
+Apply the rewrite described by C<$op> to C<$conf> and, if C<$rule_env> is 'cluster' or 'sdn', to
+all guest, host and vnet firewall configs across the cluster. Downstream configs are locked and the
+ones that changed are saved.
+
+The caller is responsible for locking and saving C<$conf> itself.
+
+Return a trace describing all changes made to the config files, each scoped by config.
+
+=cut
+
+sub rewrite_refs_in_env {
+ my ($conf, $rule_env, $op) = @_;
+
+ my $changes = [];
+
+ my $visit = sub {
+ my ($fw_conf, $env, $is_guest, $scope) = @_;
+
+ my $conf_changes = rewrite_refs_in_conf($fw_conf, $op, $env, $is_guest);
+ push $changes->@*, map { { $_->%*, scope => $scope } } $conf_changes->@*;
+
+ return scalar($conf_changes->@*);
+ };
+
+ my $self_scope = 'cluster';
+ if ($rule_env ne 'cluster' && $rule_env ne 'sdn') {
+ $self_scope = defined($conf->{vmid}) ? "$rule_env/$conf->{vmid}" : $rule_env;
+ }
+ $visit->($conf, $rule_env, 0, $self_scope);
+
+ return $changes if $rule_env ne 'cluster' && $rule_env ne 'sdn';
+
+ # on renames, the configs below are rewritten to reference the object under its new name. during
+ # this process, the object has to be resolvable under both names, otherwise concurrent firewall
+ # compilations will choke on unresolvable objects.
+ $op->{save}->($conf) if $op->{save};
+
+ my $vmlist = PVE::Cluster::get_vmlist();
+ my $vmids = ($vmlist // {})->{ids} // {};
+ for my $vmid (keys $vmids->%*) {
+ PVE::Firewall::lock_vmfw_conf(
+ $vmid,
+ 10,
+ sub {
+ my $guest_env = $vmlist->{ids}->{$vmid}->{type} eq 'lxc' ? 'ct' : 'vm';
+ my $guest_conf = PVE::Firewall::load_vmfw_conf($conf, $guest_env, $vmid);
+ if ($visit->($guest_conf, $rule_env, 1, "$guest_env/$vmid")) {
+ PVE::Firewall::save_vmfw_conf($vmid, $guest_conf);
+ }
+ },
+ );
+ }
+
+ for my $node (PVE::Cluster::get_nodelist()->@*) {
+ my $host_conf_path = "/etc/pve/nodes/$node/host.fw";
+ PVE::Firewall::lock_hostfw_conf(
+ $node,
+ 10,
+ sub {
+ my $host_conf = PVE::Firewall::load_hostfw_conf($conf, $host_conf_path);
+ if ($visit->($host_conf, $rule_env, 0, "host/$node")) {
+ PVE::Firewall::save_hostfw_conf($host_conf, $host_conf_path);
+ }
+ },
+ );
+ }
+
+ for my $vnet (PVE::Firewall::list_vnetfw_confs()->@*) {
+ PVE::Firewall::lock_vnetfw_conf(
+ $vnet,
+ 10,
+ sub {
+ my $vnet_conf = PVE::Firewall::load_vnetfw_conf($conf, 'vnet', $vnet);
+ if ($visit->($vnet_conf, $rule_env, 0, "vnet/$vnet")) {
+ PVE::Firewall::save_vnetfw_conf($vnet, $vnet_conf);
+ }
+ },
+ );
+ }
+
+ return $changes;
+}
+
+# field name + value prefix for firewall object references
+my $object_ref_specs = {
+ ipset => { prefix => '+', self => 'ipset' },
+ aliases => { prefix => '', self => 'aliases' },
+};
+
+# fields to check for object references
+my $ref_fields = ['source', 'dest', 'cidr'];
+
+=head3 matched_ref($item, $repl)
+
+Return the reference C<$item> holds that C<$repl> has an entry for, or C<undef> if there is none.
+
+=cut
+
+sub matched_ref {
+ my ($item, $repl) = @_;
+
+ for my $field ($ref_fields->@*) {
+ my $ref = lc($item->{$field} // '');
+ return $ref if exists($repl->{$ref});
+ }
+
+ return undef;
+}
+
+=head3 rewrite_items($items, $ctx, $section, $name)
+
+For each item in C<$items> that references one of the objects in its replacement map, apply the
+action defined in C<$ctx>, recording one change per item in C<$ctx->{changes}>.
+
+C<$section> and C<$name> describe where the items come from and are added to each change record.
+
+Return the updated items arrayref.
+
+=cut
+
+sub rewrite_items {
+ my ($items, $ctx, $section, $name) = @_;
+
+ my $repl = $ctx->{repl};
+ my $action = $ctx->{action};
+
+ # an IPSet member has no disabled state, so disabling it means removing it
+ my $remove = $action eq 'drop' || ($action eq 'disable' && $section eq 'ipset');
+
+ $items //= [];
+ my @result;
+
+ for my $pos (0 .. $items->$#*) {
+ my $item = $items->[$pos];
+
+ my $ref = matched_ref($item, $repl);
+ if (!defined($ref)) {
+ # no match, keep item
+ push @result, $item;
+ next;
+ }
+
+ push $ctx->{changes}->@*,
+ {
+ section => $section,
+ name => $name,
+ pos => $pos,
+ ref => $ref,
+ new => $repl->{$ref},
+ action => $action,
+ };
+
+ next if $remove;
+
+ if ($action eq 'rename') {
+ for my $field ($ref_fields->@*) {
+ my $value = lc($item->{$field} // '');
+ $item->{$field} = $repl->{$value} if exists($repl->{$value});
+ }
+ } else {
+ $item->{enable} = 0;
+ }
+
+ push @result, $item;
+ }
+
+ return \@result;
+}
+
+=head3 build_ref_map($op, $conf, $env, $is_guest)
+
+Map every allowed reference to the objects in C<$op> that resolves in C<$conf> to what it should
+become, i.e. either the renamed reference or C<undef> if C<$op> has no new name.
+
+A reference may be scoped, and which scopes a given object answers to depends on where it is defined
+and on whether C<$conf> is a cluster or guest config:
+
+=over
+
+=item a cluster object answers to C<dc/>, and to a bare reference unless C<$conf> is a guest config
+defining an object of the same name, which would shadow the cluster object instead
+
+=item an SDN object answers to C<sdn/>, and to a bare reference unless the same name is shadowed by
+a guest-local or a cluster object
+
+=item a guest object answers to C<guest/> and to a bare reference
+
+=back
+
+=cut
+
+sub build_ref_map {
+ my ($op, $conf, $env, $is_guest) = @_;
+
+ my $spec = $object_ref_specs->{ $op->{kind} } // die "unknown object kind '$op->{kind}'\n";
+
+ my $prefix = $spec->{prefix};
+ my $self = $spec->{self};
+ my $new = $op->{new};
+
+ my $map = {};
+
+ # FIXME: renaming an unscoped reference that previously resolved to a cluster object may cause
+ # it to be shadowed by a guest object with the same name as the rename's target name. The proper
+ # fix would be rewriting unscoped references scoped based on how the old name resolves,
+ # however this would possibly break some existing automation.
+ for my $name ($op->{old}->@*) {
+ my $shadowed = $is_guest && $conf->{$self}->{$name};
+
+ my $scopes = [];
+ if ($env eq 'cluster') {
+ push $scopes->@*, 'dc/';
+ push $scopes->@*, '' if !$shadowed;
+ } elsif ($env eq 'sdn') {
+ push $scopes->@*, 'sdn/';
+ push $scopes->@*, '' if !$shadowed && !$op->{cluster_conf}->{$self}->{$name};
+ } else {
+ push $scopes->@*, '', 'guest/';
+ }
+
+ $map->{"$prefix$_$name"} = defined($new) ? "$prefix$_$new" : undef for $scopes->@*;
+ }
+
+ return $map;
+}
+
+=head3 rewrite_refs_in_conf($conf, $op, $env, $is_guest)
+
+Apply the action of C<$op> to all references in C<$conf> to the objects it names. C<$env> is the
+environment the objects come from and C<$is_guest> says whether C<$conf> is a guest config. Together
+they decide how references are resolved, see C<build_ref_map>'s POD.
+
+C<$op> (built by C<update_refs>) describes the rewrite:
+
+ kind # the object kind, see update_refs()
+ old # arrayref of lowercased names whose references are to be rewritten
+ new # the new name, lowercased, when renaming, undef otherwise
+ action # 'rename' | 'disable' | 'drop'
+ cluster_conf # the cluster config
+
+Return the changes made.
+
+Only exposed for testing, see C<update_refs>'s POD for details.
+
+=cut
+
+sub rewrite_refs_in_conf {
+ my ($conf, $op, $env, $is_guest) = @_;
+
+ my $ctx = {
+ repl => build_ref_map($op, $conf, $env, $is_guest),
+ action => $op->{action},
+ changes => [],
+ };
+
+ $conf->{rules} = rewrite_items($conf->{rules}, $ctx, 'rules');
+
+ for my $part ([groups => 'group'], [ipset => 'ipset']) {
+ my ($key, $section) = $part->@*;
+ my $map = $conf->{$key} // {};
+ for my $name (keys $map->%*) {
+ $map->{$name} = rewrite_items($map->{$name}, $ctx, $section, $name);
+ }
+ }
+
+ return $ctx->{changes};
+}
+
+=head3 update_refs($opts)
+
+Rename, disable or drop all references to a firewall object across the environment.
+
+References are matched in rules, security groups and IPSet members. Matching is case-insensitive
+and renames are written back lowercased.
+
+C<$opts> holds:
+
+ conf # the firewall config the object is defined in
+ kind # 'ipset' or 'aliases'
+ old # the old name, or an arrayref of names to disable or drop
+ new # the name to rewrite them references to 'old' to, only used when renaming
+ env # 'cluster', 'sdn', or 'vm'/'ct' for a guest-defined object
+ action # 'rename', 'disable' or 'drop', or undef to leave the references alone
+ save # persists C<conf>, required for a cluster-wide rename
+
+IPSet members have no disabled state, so C<disable> removes them as well.
+
+If C<conf> is the cluster config, i.e. if C<env> is C<cluster> or C<sdn>, guest, host and vnet
+configs are sequentially locked, updated and saved. C<save> is called once the references in
+C<conf> itself have been rewritten and before any downstream config : downstream configs are
+rewritten to reference the object under its new name, and a concurrent compilation parsing the
+cluster config would fail to resolve that name until it has been written.
+
+The caller is responsible for locking and saving C<conf> itself.
+
+Return the number of references changed.
+
+=cut
+
+sub update_refs {
+ my ($opts) = @_;
+
+ my ($conf, $kind, $old, $new, $env, $action) = $opts->@{qw(conf kind old new env action)};
+
+ return 0 if !defined($action);
+
+ die "invalid action '$action'" if $action !~ m/^(rename|disable|drop)$/;
+ die "\$new must be set if \$action is 'rename'" if $action eq 'rename' && !defined($new);
+ die "\$save is required to rename across the cluster"
+ if $action eq 'rename' && ($env eq 'cluster' || $env eq 'sdn') && !$opts->{save};
+
+ my $lc_old = [map { lc($_) } (ref($old) eq 'ARRAY' ? $old->@* : $old)];
+
+ die "cannot rename more than one object at once\n"
+ if $action eq 'rename' && scalar($lc_old->@*) != 1;
+
+ my $op = {
+ kind => $kind,
+ old => $lc_old,
+ new => $action eq 'rename' ? lc($new) : undef,
+ action => $action,
+ cluster_conf => $conf,
+ save => $opts->{save},
+ };
+
+ return scalar(rewrite_refs_in_env($conf, $env, $op)->@*);
+}
+
1;
--
2.47.3
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH pve-firewall v6 04/28] firewall: references: log the references an object update touched
2026-11-11 13:18 [PATCH container/docs/firewall/manager/network/qemu-server v6 00/28] handle dangling references when firewall objects go away Arthur Bied-Charreton
` (2 preceding siblings ...)
2026-11-11 13:18 ` [PATCH pve-firewall v6 03/28] firewall: references: add helpers to update object references Arthur Bied-Charreton
@ 2026-11-11 13:18 ` Arthur Bied-Charreton
2026-11-11 13:18 ` [PATCH pve-firewall v6 05/28] firewall: references: add wrappers for the SDN-generated IPSets Arthur Bied-Charreton
` (23 subsequent siblings)
27 siblings, 0 replies; 29+ messages in thread
From: Arthur Bied-Charreton @ 2026-11-11 13:18 UTC (permalink / raw)
To: pve-devel
Make update_refs() log each reference once the config holding it was
saved, so a run that dies halfway through the cluster still shows what
it managed to update and does not count failed attempts:
firewall: ipset 'set' renamed to 'set2':
cluster: rules: rule 6: '+set' renamed to '+set2'
cluster: group 'fixgrp': rule 1: '+set' renamed to '+set2'
firewall: 2 references updated
The footer only counts what was actually updated, so it is logged even
if the walk did not run to the end. Errors are still raised.
The lines go to stdout in a worker, and to the syslog otherwise. Also
take an optional log sink coderef to allow a caller to collect logs
instead.
Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
---
src/PVE/Firewall/References.pm | 124 +++++++++++++++++++++++++++++----
1 file changed, 111 insertions(+), 13 deletions(-)
diff --git a/src/PVE/Firewall/References.pm b/src/PVE/Firewall/References.pm
index 9eb5bc8..6ea39cb 100644
--- a/src/PVE/Firewall/References.pm
+++ b/src/PVE/Firewall/References.pm
@@ -5,6 +5,8 @@ use warnings;
use PVE::Cluster;
use PVE::Firewall;
+use PVE::RESTEnvironment;
+use PVE::SafeSyslog qw(syslog);
use base 'Exporter';
our @EXPORT_OK = qw(
@@ -66,27 +68,38 @@ sub rewrite_refs_in_env {
my $changes = [];
+ # a change is only reported after successful save
+ my $emit = sub {
+ my ($conf_changes) = @_;
+ for my $change ($conf_changes->@*) {
+ push $changes->@*, $change;
+ $op->{on_change}->($change) if $op->{on_change};
+ }
+ };
+
my $visit = sub {
my ($fw_conf, $env, $is_guest, $scope) = @_;
my $conf_changes = rewrite_refs_in_conf($fw_conf, $op, $env, $is_guest);
- push $changes->@*, map { { $_->%*, scope => $scope } } $conf_changes->@*;
-
- return scalar($conf_changes->@*);
+ return [map { { $_->%*, scope => $scope } } $conf_changes->@*];
};
my $self_scope = 'cluster';
if ($rule_env ne 'cluster' && $rule_env ne 'sdn') {
$self_scope = defined($conf->{vmid}) ? "$rule_env/$conf->{vmid}" : $rule_env;
}
- $visit->($conf, $rule_env, 0, $self_scope);
+ my $self_changes = $visit->($conf, $rule_env, 0, $self_scope);
- return $changes if $rule_env ne 'cluster' && $rule_env ne 'sdn';
+ if ($rule_env ne 'cluster' && $rule_env ne 'sdn') {
+ $emit->($self_changes);
+ return $changes;
+ }
# on renames, the configs below are rewritten to reference the object under its new name. during
# this process, the object has to be resolvable under both names, otherwise concurrent firewall
# compilations will choke on unresolvable objects.
$op->{save}->($conf) if $op->{save};
+ $emit->($self_changes) if $op->{save};
my $vmlist = PVE::Cluster::get_vmlist();
my $vmids = ($vmlist // {})->{ids} // {};
@@ -97,8 +110,10 @@ sub rewrite_refs_in_env {
sub {
my $guest_env = $vmlist->{ids}->{$vmid}->{type} eq 'lxc' ? 'ct' : 'vm';
my $guest_conf = PVE::Firewall::load_vmfw_conf($conf, $guest_env, $vmid);
- if ($visit->($guest_conf, $rule_env, 1, "$guest_env/$vmid")) {
+ my $guest_changes = $visit->($guest_conf, $rule_env, 1, "$guest_env/$vmid");
+ if ($guest_changes->@*) {
PVE::Firewall::save_vmfw_conf($vmid, $guest_conf);
+ $emit->($guest_changes);
}
},
);
@@ -111,8 +126,10 @@ sub rewrite_refs_in_env {
10,
sub {
my $host_conf = PVE::Firewall::load_hostfw_conf($conf, $host_conf_path);
- if ($visit->($host_conf, $rule_env, 0, "host/$node")) {
+ my $host_changes = $visit->($host_conf, $rule_env, 0, "host/$node");
+ if ($host_changes->@*) {
PVE::Firewall::save_hostfw_conf($host_conf, $host_conf_path);
+ $emit->($host_changes);
}
},
);
@@ -124,13 +141,18 @@ sub rewrite_refs_in_env {
10,
sub {
my $vnet_conf = PVE::Firewall::load_vnetfw_conf($conf, 'vnet', $vnet);
- if ($visit->($vnet_conf, $rule_env, 0, "vnet/$vnet")) {
+ my $vnet_changes = $visit->($vnet_conf, $rule_env, 0, "vnet/$vnet");
+ if ($vnet_changes->@*) {
PVE::Firewall::save_vnetfw_conf($vnet, $vnet_conf);
+ $emit->($vnet_changes);
}
},
);
}
+ # without a save callback, the caller saves $conf once we return
+ $emit->($self_changes) if !$op->{save};
+
return $changes;
}
@@ -319,9 +341,17 @@ sub rewrite_refs_in_conf {
return $ctx->{changes};
}
+my $ref_change_verbs = {
+ rename => 'updated',
+ disable => 'disabled',
+ drop => 'dropped',
+};
+
=head3 update_refs($opts)
-Rename, disable or drop all references to a firewall object across the environment.
+Rename, disable or drop all references to a firewall object across the environment, logging each
+one once the config holding it was saved. Without C<save>, the changes to C<conf> are logged last,
+as the caller saves it after this returns.
References are matched in rules, security groups and IPSet members. Matching is case-insensitive
and renames are written back lowercased.
@@ -335,6 +365,10 @@ C<$opts> holds:
env # 'cluster', 'sdn', or 'vm'/'ct' for a guest-defined object
action # 'rename', 'disable' or 'drop', or undef to leave the references alone
save # persists C<conf>, required for a cluster-wide rename
+ subject # header for the task log
+ log # optional log sink coderef, by default logs go to task log if in worker, otherwise syslog
+
+The caller is responsible for locking and saving C<conf> itself.
IPSet members have no disabled state, so C<disable> removes them as well.
@@ -344,7 +378,8 @@ C<conf> itself have been rewritten and before any downstream config : downstream
rewritten to reference the object under its new name, and a concurrent compilation parsing the
cluster config would fail to resolve that name until it has been written.
-The caller is responsible for locking and saving C<conf> itself.
+If the logs cannot be written as we go (SDN apply is an example, since the conf update is done
+before forking a worker and logs would go to syslog), pass C<log> with custom collection logic.
Return the number of references changed.
@@ -353,9 +388,15 @@ Return the number of references changed.
sub update_refs {
my ($opts) = @_;
- my ($conf, $kind, $old, $new, $env, $action) = $opts->@{qw(conf kind old new env action)};
+ my ($conf, $kind, $old, $new, $env, $action, $subject) =
+ $opts->@{qw(conf kind old new env action subject)};
- return 0 if !defined($action);
+ my $log = $opts->{log} // \&log_ref_line;
+
+ if (!defined($action)) {
+ $log->("firewall: $subject: references left as they are");
+ return 0;
+ }
die "invalid action '$action'" if $action !~ m/^(rename|disable|drop)$/;
die "\$new must be set if \$action is 'rename'" if $action eq 'rename' && !defined($new);
@@ -367,6 +408,10 @@ sub update_refs {
die "cannot rename more than one object at once\n"
if $action eq 'rename' && scalar($lc_old->@*) != 1;
+ $log->("firewall: $subject:");
+
+ my $count = 0;
+
my $op = {
kind => $kind,
old => $lc_old,
@@ -374,9 +419,62 @@ sub update_refs {
action => $action,
cluster_conf => $conf,
save => $opts->{save},
+ on_change => sub {
+ $count++;
+ $log->(' ' . format_ref_change($_[0]));
+ },
};
- return scalar(rewrite_refs_in_env($conf, $env, $op)->@*);
+ eval { rewrite_refs_in_env($conf, $env, $op) };
+ my $err = $@;
+
+ # if the walk died before incrementing $count there is nothing to log
+ if ($count) {
+ my $noun = $count == 1 ? 'reference' : 'references';
+ $log->("firewall: $count $noun $ref_change_verbs->{$action}");
+ } elsif (!$err) {
+ $log->("firewall: no references found");
+ }
+
+ die $err if $err;
+
+ return $count;
+}
+
+=head3 format_ref_change($change)
+
+Format one recorded config change as a single line, e.g.
+
+ "cluster: rules: rule 6: '+ipset' renamed to '+ipset1'"
+
+=cut
+
+sub format_ref_change {
+ my ($change) = @_;
+
+ my $section = $change->{section};
+ my $where = $section eq 'rules' ? 'rules' : "$section '$change->{name}'";
+ my $item = $section eq 'ipset' ? 'member' : 'rule';
+
+ my $what = "$change->{scope}: $where: $item $change->{pos}: '$change->{ref}'";
+
+ return "$what renamed to '$change->{new}'" if $change->{action} eq 'rename';
+ return "$what dropped" if $change->{action} eq 'drop';
+
+ # an IPSet member has no disabled state, so disabling it removed it
+ return "$what " . ($section eq 'ipset' ? 'removed' : 'disabled');
+}
+
+sub log_ref_line {
+ my ($line) = @_;
+
+ if (PVE::RESTEnvironment::is_worker()) {
+ print("$line\n");
+ } else {
+ syslog('info', '%s', $line);
+ }
+
+ return;
}
1;
--
2.47.3
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH pve-firewall v6 05/28] firewall: references: add wrappers for the SDN-generated IPSets
2026-11-11 13:18 [PATCH container/docs/firewall/manager/network/qemu-server v6 00/28] handle dangling references when firewall objects go away Arthur Bied-Charreton
` (3 preceding siblings ...)
2026-11-11 13:18 ` [PATCH pve-firewall v6 04/28] firewall: references: log the references an object update touched Arthur Bied-Charreton
@ 2026-11-11 13:18 ` Arthur Bied-Charreton
2026-11-11 13:18 ` [PATCH pve-firewall v6 06/28] firewall: parser: do not log errors for disabled rules Arthur Bied-Charreton
` (22 subsequent siblings)
27 siblings, 0 replies; 29+ messages in thread
From: Arthur Bied-Charreton @ 2026-11-11 13:18 UTC (permalink / raw)
To: pve-devel
SDN generates IPSets that cannot be deleted directly, but rather
disappear once the config stops generating them: the 4 IPSets of a VNet
go away with the VNet, and a guest's IPAM IPSet goes away once the guest
is destroyed. Rules referencing those are left dangling as well.
Add update_sdn_ipset_refs() for a list of such IPSets,
update_vnet_ipset_refs() for the four IPSets of a VNet, and
update_guest_ipam_ipset_refs() for a guest's IPAM IPSet. They lock the
cluster config around update_refs(), save it, and log what they
changed.
The callers are PUT /cluster/sdn in pve-network, and the guest destroy
endpoints in qemu-server and pve-container.
Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
---
src/PVE/Firewall/References.pm | 77 ++++++++++++++++++++++++++++++++++
1 file changed, 77 insertions(+)
diff --git a/src/PVE/Firewall/References.pm b/src/PVE/Firewall/References.pm
index 6ea39cb..b106651 100644
--- a/src/PVE/Firewall/References.pm
+++ b/src/PVE/Firewall/References.pm
@@ -477,4 +477,81 @@ sub log_ref_line {
return;
}
+=head3 update_sdn_ipset_refs($ipsets, $action, $subject, $log)
+
+Apply C<$action> to all references to the SDN-generated IPSets in C<$ipsets>.
+
+C<$subject> is the header for the log block and C<$log> is an optional log sink callback.
+
+Return the number of references changed.
+
+=cut
+
+sub update_sdn_ipset_refs {
+ my ($ipsets, $action, $subject, $log) = @_;
+
+ die "SDN IPSets cannot be renamed\n" if $action && $action eq 'rename';
+
+ my $count = 0;
+
+ PVE::Firewall::lock_clusterfw_conf(
+ 10,
+ sub {
+ my $conf = PVE::Firewall::load_clusterfw_conf();
+
+ $count = update_refs({
+ conf => $conf,
+ kind => 'ipset',
+ old => $ipsets,
+ env => 'sdn',
+ action => $action,
+ subject => $subject,
+ log => $log,
+ });
+
+ PVE::Firewall::save_clusterfw_conf($conf);
+ },
+ );
+
+ return $count;
+}
+
+=head3 update_vnet_ipset_refs($vnets, $action, $log)
+
+Apply C<$action> to all references to the auto-generated IPSets for C<$vnets>.
+
+Return the number of references changed.
+
+=cut
+
+sub update_vnet_ipset_refs {
+ my ($vnets, $action, $log) = @_;
+
+ my $ipsets = [
+ map {
+ my $vnet = $_;
+ map { "$vnet-$_" } qw(all gateway no-gateway dhcp)
+ } $vnets->@*
+ ];
+
+ my $names = join(', ', map { "'$_'" } $vnets->@*);
+ my $subject = scalar($vnets->@*) > 1 ? "vnets $names removed" : "vnet $names removed";
+
+ return update_sdn_ipset_refs($ipsets, $action, $subject, $log);
+}
+
+=head3 update_guest_ipam_ipset_refs($vmid, $action)
+
+Apply C<$action> to all references to the IPAM IPSet of the guest C<$vmid>.
+
+Return the number of references changed.
+
+=cut
+
+sub update_guest_ipam_ipset_refs {
+ my ($vmid, $action) = @_;
+
+ return update_sdn_ipset_refs("guest-ipam-$vmid", $action, "guest $vmid destroyed");
+}
+
1;
--
2.47.3
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH pve-firewall v6 06/28] firewall: parser: do not log errors for disabled rules
2026-11-11 13:18 [PATCH container/docs/firewall/manager/network/qemu-server v6 00/28] handle dangling references when firewall objects go away Arthur Bied-Charreton
` (4 preceding siblings ...)
2026-11-11 13:18 ` [PATCH pve-firewall v6 05/28] firewall: references: add wrappers for the SDN-generated IPSets Arthur Bied-Charreton
@ 2026-11-11 13:18 ` Arthur Bied-Charreton
2026-11-11 13:18 ` [PATCH pve-firewall v6 07/28] firewall: ipset: add option to handle dangling references on rename Arthur Bied-Charreton
` (21 subsequent siblings)
27 siblings, 0 replies; 29+ messages in thread
From: Arthur Bied-Charreton @ 2026-11-11 13:18 UTC (permalink / raw)
To: pve-devel
The rule parser always logged errors when encountering them, even if the
offending rule was disabled.
The following commits add functionality to automatically disable rules
with dangling references to ipsets or aliases. Avoid logging errors for
disabled rules since that will then become an expected state.
Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
---
src/PVE/Firewall.pm | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/PVE/Firewall.pm b/src/PVE/Firewall.pm
index 4631b5a..6ce5306 100644
--- a/src/PVE/Firewall.pm
+++ b/src/PVE/Firewall.pm
@@ -3417,7 +3417,7 @@ sub parse_fw_rule {
die "unable to parse rule parameters: $line\n" if length($line);
$rule = verify_rule($rule, $cluster_conf, $fw_conf, $rule_env, 1);
- if ($rule->{errors}) {
+ if ($rule->{enable} && $rule->{errors}) {
# The verbose flag really means we're running from the CLI and want
# output on the console - in the other case we really want such errors
# to go into the syslog instead.
--
2.47.3
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH pve-firewall v6 07/28] firewall: ipset: add option to handle dangling references on rename
2026-11-11 13:18 [PATCH container/docs/firewall/manager/network/qemu-server v6 00/28] handle dangling references when firewall objects go away Arthur Bied-Charreton
` (5 preceding siblings ...)
2026-11-11 13:18 ` [PATCH pve-firewall v6 06/28] firewall: parser: do not log errors for disabled rules Arthur Bied-Charreton
@ 2026-11-11 13:18 ` Arthur Bied-Charreton
2026-11-11 13:18 ` [PATCH pve-firewall v6 08/28] firewall: ipset: add option to handle dangling references on delete Arthur Bied-Charreton
` (20 subsequent siblings)
27 siblings, 0 replies; 29+ messages in thread
From: Arthur Bied-Charreton @ 2026-11-11 13:18 UTC (permalink / raw)
To: pve-devel
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 <a.bied-charreton@proxmox.com>
---
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
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH pve-firewall v6 08/28] firewall: ipset: add option to handle dangling references on delete
2026-11-11 13:18 [PATCH container/docs/firewall/manager/network/qemu-server v6 00/28] handle dangling references when firewall objects go away Arthur Bied-Charreton
` (6 preceding siblings ...)
2026-11-11 13:18 ` [PATCH pve-firewall v6 07/28] firewall: ipset: add option to handle dangling references on rename Arthur Bied-Charreton
@ 2026-11-11 13:18 ` Arthur Bied-Charreton
2026-11-11 13:18 ` [PATCH pve-firewall v6 09/28] firewall: aliases: add option to handle dangling references on rename Arthur Bied-Charreton
` (19 subsequent siblings)
27 siblings, 0 replies; 29+ messages in thread
From: Arthur Bied-Charreton @ 2026-11-11 13:18 UTC (permalink / raw)
To: pve-devel
Deleting an ipset referenced by rules or security groups leaves dangling
references, which the firewall fails to parse and drops.
Add a 'dangling-references' option to the delete endpoint, to either
'disable' the referencing rules or 'drop' them along with the ipset. The
default, 'keep', leaves them as they are. For cluster ipsets, this also
covers all downstream configs (host, guest and vnet).
Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
---
src/PVE/API2/Firewall/IPSet.pm | 14 ++++++++++++++
src/PVE/Firewall.pm | 12 ++++++++++++
2 files changed, 26 insertions(+)
diff --git a/src/PVE/API2/Firewall/IPSet.pm b/src/PVE/API2/Firewall/IPSet.pm
index 9d3c562..144baf6 100644
--- a/src/PVE/API2/Firewall/IPSet.pm
+++ b/src/PVE/API2/Firewall/IPSet.pm
@@ -2,6 +2,7 @@ package PVE::API2::Firewall::IPSetBase;
use strict;
use warnings;
+use PVE::Firewall::References qw(update_refs);
use PVE::Exception qw(raise raise_param_exc);
use PVE::JSONSchema qw(get_standard_option);
@@ -135,6 +136,7 @@ sub register_delete_ipset {
my $properties = $class->additional_parameters();
$properties->{name} = get_standard_option('ipset-name');
+ $properties->{'dangling-references'} = get_standard_option('pve-dangling-references-delete');
$properties->{force} = {
type => 'boolean',
optional => 1,
@@ -166,6 +168,18 @@ sub register_delete_ipset {
die "IPSet '$param->{name}' is not empty\n"
if scalar(@$ipset) && !$param->{force};
+ my $action = $param->{'dangling-references'}
+ // $properties->{'dangling-references'}->{default};
+
+ update_refs({
+ conf => $fw_conf,
+ kind => 'ipset',
+ old => $param->{name},
+ env => $class->rule_env(),
+ action => $action eq 'keep' ? undef : $action,
+ subject => "ipset '$param->{name}' deleted",
+ });
+
$class->save_ipset($param, $fw_conf, undef);
},
diff --git a/src/PVE/Firewall.pm b/src/PVE/Firewall.pm
index d4e1ad9..85ad23e 100644
--- a/src/PVE/Firewall.pm
+++ b/src/PVE/Firewall.pm
@@ -223,6 +223,18 @@ PVE::JSONSchema::register_standard_option(
},
);
+PVE::JSONSchema::register_standard_option(
+ 'pve-dangling-references-delete',
+ {
+ type => 'string',
+ enum => ['keep', 'disable', 'drop'],
+ optional => 1,
+ description => "Handle references that deleting this object would leave dangling. Use "
+ . "'disable' to disable the referencing rules, or 'drop' to remove them entirely.",
+ default => 'keep',
+ },
+);
+
PVE::JSONSchema::register_standard_option(
'pve-dangling-references-force-rename',
{
--
2.47.3
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH pve-firewall v6 09/28] firewall: aliases: add option to handle dangling references on rename
2026-11-11 13:18 [PATCH container/docs/firewall/manager/network/qemu-server v6 00/28] handle dangling references when firewall objects go away Arthur Bied-Charreton
` (7 preceding siblings ...)
2026-11-11 13:18 ` [PATCH pve-firewall v6 08/28] firewall: ipset: add option to handle dangling references on delete Arthur Bied-Charreton
@ 2026-11-11 13:18 ` Arthur Bied-Charreton
2026-11-11 13:18 ` [PATCH pve-firewall v6 10/28] firewall: aliases: add option to handle dangling references on delete Arthur Bied-Charreton
` (18 subsequent siblings)
27 siblings, 0 replies; 29+ messages in thread
From: Arthur Bied-Charreton @ 2026-11-11 13:18 UTC (permalink / raw)
To: pve-devel
Renaming an alias still referenced by rules or ipset members 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
aliases, this also covers references in downstream configs (host, guest
and vnet).
A rename that fails partway leaves both the old and the new alias in
the cluster config. Retrying it chokes on the check against
overwriting an existing alias. '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 <a.bied-charreton@proxmox.com>
---
src/PVE/API2/Firewall/Aliases.pm | 45 +++++++++++++++++++++++++++++---
1 file changed, 42 insertions(+), 3 deletions(-)
diff --git a/src/PVE/API2/Firewall/Aliases.pm b/src/PVE/API2/Firewall/Aliases.pm
index 02835b3..90fe95f 100644
--- a/src/PVE/API2/Firewall/Aliases.pm
+++ b/src/PVE/API2/Firewall/Aliases.pm
@@ -2,6 +2,8 @@ package PVE::API2::Firewall::AliasesBase;
use strict;
use warnings;
+
+use PVE::Firewall::References qw(update_refs);
use PVE::Exception qw(raise raise_param_exc);
use PVE::JSONSchema qw(get_standard_option);
@@ -219,6 +221,9 @@ sub register_update_alias {
$properties->{comment} = $api_properties->{comment};
$properties->{digest} = get_standard_option('pve-config-digest');
+ $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 => 'update_alias',
path => '{name}',
@@ -239,6 +244,11 @@ sub register_update_alias {
sub {
my ($param) = @_;
+ my $dangling_refs = $param->{'dangling-references'}
+ // $properties->{'dangling-references'}->{default};
+ my $force = $param->{'force-rename'}
+ // $properties->{'force-rename'}->{default};
+
my ($fw_conf, $aliases) = $class->load_config($param);
my $list = &$aliases_to_list($aliases);
@@ -264,9 +274,38 @@ sub register_update_alias {
if ($rename && ($name ne $rename)) {
raise_param_exc({ name => "alias '$param->{rename}' already exists" })
- if defined($aliases->{$rename});
- $aliases->{$name}->{name} = $param->{rename};
- $aliases->{$rename} = $aliases->{$name};
+ if defined($aliases->{$rename})
+ && !$force;
+
+ $aliases->{$rename} =
+ { $aliases->{$name}->%*, name => $param->{rename} };
+
+ my $saved = 0;
+ my $save = sub {
+ $class->save_aliases($param, $fw_conf, $aliases);
+ $saved = 1;
+ };
+
+ eval {
+ update_refs({
+ conf => $fw_conf,
+ kind => 'aliases',
+ old => $name,
+ new => $param->{rename},
+ env => $class->rule_env(),
+ action => $dangling_refs eq 'keep' ? undef : $dangling_refs,
+ subject =>
+ "alias '$param->{name}' renamed to '$param->{rename}'",
+ save => $save,
+ });
+ };
+ if (my $err = $@) {
+ die "rename interrupted, the alias exists under both names; "
+ . "retry with 'force-rename' to finish: $err"
+ if $saved;
+ die "could not update references to alias '$name': $err";
+ }
+
delete $aliases->{$name};
}
--
2.47.3
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH pve-firewall v6 10/28] firewall: aliases: add option to handle dangling references on delete
2026-11-11 13:18 [PATCH container/docs/firewall/manager/network/qemu-server v6 00/28] handle dangling references when firewall objects go away Arthur Bied-Charreton
` (8 preceding siblings ...)
2026-11-11 13:18 ` [PATCH pve-firewall v6 09/28] firewall: aliases: add option to handle dangling references on rename Arthur Bied-Charreton
@ 2026-11-11 13:18 ` Arthur Bied-Charreton
2026-11-11 13:18 ` [PATCH pve-firewall v6 11/28] firewall: api: run reference updates as tasks Arthur Bied-Charreton
` (17 subsequent siblings)
27 siblings, 0 replies; 29+ messages in thread
From: Arthur Bied-Charreton @ 2026-11-11 13:18 UTC (permalink / raw)
To: pve-devel
Deleting an alias referenced by rules, security groups or ipset members
leaves dangling references, which the firewall fails to parse and drops.
Add a 'dangling-references' option to the delete endpoint, to either
'disable' the referencing rules or 'drop' them along with the alias. The
default, 'keep', leaves them as they are. Ipset members have no disabled
state, so they are removed in both cases. For cluster aliases, this also
covers all downstream configs (host, guest and vnet).
Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
---
src/PVE/API2/Firewall/Aliases.pm | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/src/PVE/API2/Firewall/Aliases.pm b/src/PVE/API2/Firewall/Aliases.pm
index 90fe95f..cdbe68c 100644
--- a/src/PVE/API2/Firewall/Aliases.pm
+++ b/src/PVE/API2/Firewall/Aliases.pm
@@ -325,6 +325,7 @@ sub register_delete_alias {
$properties->{name} = $api_properties->{name};
$properties->{digest} = get_standard_option('pve-config-digest');
+ $properties->{'dangling-references'} = get_standard_option('pve-dangling-references-delete');
$class->register_method({
name => 'remove_alias',
@@ -353,6 +354,20 @@ sub register_delete_alias {
PVE::Tools::assert_if_modified($digest, $param->{digest});
my $name = lc($param->{name});
+ raise_param_exc({ name => "no such alias" }) if !$aliases->{$name};
+
+ my $action = $param->{'dangling-references'}
+ // $properties->{'dangling-references'}->{default};
+
+ update_refs({
+ conf => $fw_conf,
+ kind => 'aliases',
+ old => $param->{name},
+ env => $class->rule_env(),
+ action => $action eq 'keep' ? undef : $action,
+ subject => "alias '$param->{name}' deleted",
+ });
+
delete $aliases->{$name};
$class->save_aliases($param, $fw_conf, $aliases);
--
2.47.3
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH pve-firewall v6 11/28] firewall: api: run reference updates as tasks
2026-11-11 13:18 [PATCH container/docs/firewall/manager/network/qemu-server v6 00/28] handle dangling references when firewall objects go away Arthur Bied-Charreton
` (9 preceding siblings ...)
2026-11-11 13:18 ` [PATCH pve-firewall v6 10/28] firewall: aliases: add option to handle dangling references on delete Arthur Bied-Charreton
@ 2026-11-11 13:18 ` Arthur Bied-Charreton
2026-11-11 13:18 ` [PATCH pve-firewall v6 12/28] firewall: tests: add tests for object reference updates Arthur Bied-Charreton
` (16 subsequent siblings)
27 siblings, 0 replies; 29+ messages in thread
From: Arthur Bied-Charreton @ 2026-11-11 13:18 UTC (permalink / raw)
To: pve-devel
A 'dangling-references' action other than 'keep' rewrites the rules that
referenced the object, which for a cluster-scoped object means going
through all guest, host and vnet firewall configs in the cluster,
potentially updating each of them.
The update_refs() helper logs each change made to each config. Fork a
worker when the request passes 'dangling-references' so the log lands
in the task log, which is visible cluster-wide, instead of the syslog.
A request without the parameter, i.e. what every existing client sends,
keeps running synchronously and returning null for backwards
compatibility.
Checks that can be performed without holding the lock are made before
forking so a rejected request can still fail right away instead of
requiring the caller to go read the task.
Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
---
src/PVE/API2/Firewall/Aliases.pm | 233 ++++++++++++++++++------------
src/PVE/API2/Firewall/IPSet.pm | 238 ++++++++++++++++++-------------
src/PVE/Firewall/References.pm | 20 +++
3 files changed, 299 insertions(+), 192 deletions(-)
diff --git a/src/PVE/API2/Firewall/Aliases.pm b/src/PVE/API2/Firewall/Aliases.pm
index cdbe68c..301ca88 100644
--- a/src/PVE/API2/Firewall/Aliases.pm
+++ b/src/PVE/API2/Firewall/Aliases.pm
@@ -3,7 +3,7 @@ package PVE::API2::Firewall::AliasesBase;
use strict;
use warnings;
-use PVE::Firewall::References qw(update_refs);
+use PVE::Firewall::References qw(update_refs run_ref_update);
use PVE::Exception qw(raise raise_param_exc);
use PVE::JSONSchema qw(get_standard_option);
@@ -84,6 +84,44 @@ my $aliases_to_list = sub {
return $list;
};
+my $check_digest = sub {
+ my ($aliases, $param) = @_;
+
+ my $list = &$aliases_to_list($aliases);
+ my (undef, $digest) = PVE::Firewall::copy_list_with_digest($list);
+
+ PVE::Tools::assert_if_modified($digest, $param->{digest});
+};
+
+my $check_update = sub {
+ my ($aliases, $param, $force) = @_;
+
+ $check_digest->($aliases, $param);
+
+ my $name = lc($param->{name});
+ raise_param_exc({ name => "no such alias" }) if !$aliases->{$name};
+
+ my $rename = $param->{rename};
+ $rename = lc($rename) if $rename;
+
+ if (
+ $rename
+ && $name ne $rename
+ && defined($aliases->{$rename})
+ && !$force
+ ) {
+ raise_param_exc({ name => "alias '$param->{rename}' already exists" });
+ }
+};
+
+my $check_delete = sub {
+ my ($aliases, $param) = @_;
+
+ $check_digest->($aliases, $param);
+
+ raise_param_exc({ name => "no such alias" }) if !$aliases->{ lc($param->{name}) };
+};
+
sub register_get_aliases {
my ($class) = @_;
@@ -220,7 +258,6 @@ sub register_update_alias {
$properties->{cidr} = { $api_properties->{cidr}->%*, optional => 1 };
$properties->{comment} = $api_properties->{comment};
$properties->{digest} = get_standard_option('pve-config-digest');
-
$properties->{'dangling-references'} = get_standard_option('pve-dangling-references-rename');
$properties->{'force-rename'} = get_standard_option('pve-dangling-references-force-rename');
@@ -235,85 +272,88 @@ sub register_update_alias {
additionalProperties => 0,
properties => $properties,
},
- returns => { type => "null" },
+ returns => { type => 'string', optional => 1 },
code => sub {
my ($param) = @_;
- $class->lock_config(
- $param,
- sub {
- my ($param) = @_;
-
- my $dangling_refs = $param->{'dangling-references'}
- // $properties->{'dangling-references'}->{default};
- my $force = $param->{'force-rename'}
- // $properties->{'force-rename'}->{default};
-
- my ($fw_conf, $aliases) = $class->load_config($param);
-
- my $list = &$aliases_to_list($aliases);
-
- my (undef, $digest) = PVE::Firewall::copy_list_with_digest($list);
-
- PVE::Tools::assert_if_modified($digest, $param->{digest});
-
- my $name = lc($param->{name});
-
- raise_param_exc({ name => "no such alias" }) if !$aliases->{$name};
+ my $dangling_refs = $param->{'dangling-references'}
+ // $properties->{'dangling-references'}->{default};
+ my $force = $param->{'force-rename'} // $properties->{'force-rename'}->{default};
- my $data = {
- name => $param->{name},
- cidr => $param->{cidr} // $aliases->{$name}->{cidr},
- comment => $param->{comment} // $aliases->{$name}->{comment},
- };
+ my $renaming = $param->{rename} && lc($param->{name}) ne lc($param->{rename});
- $aliases->{$name} = $data;
+ my $code = sub {
+ $class->lock_config(
+ $param,
+ sub {
+ my ($param) = @_;
- my $rename = $param->{rename};
- $rename = lc($rename) if $rename;
+ my ($fw_conf, $aliases) = $class->load_config($param);
- if ($rename && ($name ne $rename)) {
- raise_param_exc({ name => "alias '$param->{rename}' already exists" })
- if defined($aliases->{$rename})
- && !$force;
+ $check_update->($aliases, $param, $force);
- $aliases->{$rename} =
- { $aliases->{$name}->%*, name => $param->{rename} };
+ my $name = lc($param->{name});
- my $saved = 0;
- my $save = sub {
- $class->save_aliases($param, $fw_conf, $aliases);
- $saved = 1;
+ my $data = {
+ name => $param->{name},
+ cidr => $param->{cidr} // $aliases->{$name}->{cidr},
+ comment => $param->{comment} // $aliases->{$name}->{comment},
};
- eval {
- update_refs({
- conf => $fw_conf,
- kind => 'aliases',
- old => $name,
- new => $param->{rename},
- env => $class->rule_env(),
- action => $dangling_refs eq 'keep' ? undef : $dangling_refs,
- subject =>
- "alias '$param->{name}' renamed to '$param->{rename}'",
- save => $save,
- });
- };
- if (my $err = $@) {
- die "rename interrupted, the alias exists under both names; "
- . "retry with 'force-rename' to finish: $err"
- if $saved;
- die "could not update references to alias '$name': $err";
+ $aliases->{$name} = $data;
+
+ my $rename = $param->{rename};
+ $rename = lc($rename) if $rename;
+
+ if ($rename && ($name ne $rename)) {
+ $aliases->{$rename} =
+ { $aliases->{$name}->%*, name => $param->{rename} };
+
+ my $saved = 0;
+ my $save = sub {
+ $class->save_aliases($param, $fw_conf, $aliases);
+ $saved = 1;
+ };
+
+ eval {
+ update_refs({
+ conf => $fw_conf,
+ kind => 'aliases',
+ old => $name,
+ new => $param->{rename},
+ env => $class->rule_env(),
+ action => $dangling_refs eq 'keep' ? undef : $dangling_refs,
+ subject =>
+ "alias '$param->{name}' renamed to '$param->{rename}'",
+ save => $save,
+ });
+ };
+ if (my $err = $@) {
+ die "rename interrupted, the alias exists under both names; "
+ . "retry with 'force-rename' to finish: $err"
+ if $saved;
+ die "could not update references to alias '$name': $err";
+ }
+
+ delete $aliases->{$name};
}
- delete $aliases->{$name};
- }
+ $class->save_aliases($param, $fw_conf, $aliases);
+ },
+ );
+ };
- $class->save_aliases($param, $fw_conf, $aliases);
- },
- );
+ if (!$renaming || !defined($param->{'dangling-references'})) {
+ $code->();
+ return undef;
+ }
- return undef;
+ # fail on the request instead of inside the task.
+ my (undef, $aliases) = $class->load_config($param);
+
+ $check_update->($aliases, $param, $force);
+
+ return run_ref_update('fw-alias-rename', $param->{vmid} // $param->{name}, $code);
},
});
}
@@ -338,43 +378,52 @@ sub register_delete_alias {
additionalProperties => 0,
properties => $properties,
},
- returns => { type => "null" },
+ returns => { type => 'string', optional => 1 },
code => sub {
my ($param) = @_;
- $class->lock_config(
- $param,
- sub {
- my ($param) = @_;
+ my $action = $param->{'dangling-references'}
+ // $properties->{'dangling-references'}->{default};
- my ($fw_conf, $aliases) = $class->load_config($param);
+ my $code = sub {
+ $class->lock_config(
+ $param,
+ sub {
+ my ($param) = @_;
- my $list = &$aliases_to_list($aliases);
- my (undef, $digest) = PVE::Firewall::copy_list_with_digest($list);
- PVE::Tools::assert_if_modified($digest, $param->{digest});
+ my ($fw_conf, $aliases) = $class->load_config($param);
- my $name = lc($param->{name});
- raise_param_exc({ name => "no such alias" }) if !$aliases->{$name};
+ $check_delete->($aliases, $param);
- my $action = $param->{'dangling-references'}
- // $properties->{'dangling-references'}->{default};
+ my $name = lc($param->{name});
- update_refs({
- conf => $fw_conf,
- kind => 'aliases',
- old => $param->{name},
- env => $class->rule_env(),
- action => $action eq 'keep' ? undef : $action,
- subject => "alias '$param->{name}' deleted",
- });
+ update_refs({
+ conf => $fw_conf,
+ kind => 'aliases',
+ old => $param->{name},
+ env => $class->rule_env(),
+ action => $action eq 'keep' ? undef : $action,
+ subject => "alias '$param->{name}' deleted",
+ });
- delete $aliases->{$name};
+ delete $aliases->{$name};
- $class->save_aliases($param, $fw_conf, $aliases);
- },
- );
+ $class->save_aliases($param, $fw_conf, $aliases);
+ },
+ );
+ };
- return undef;
+ if (!defined($param->{'dangling-references'})) {
+ $code->();
+ return undef;
+ }
+
+ # fail on the request instead of inside the task
+ my (undef, $aliases) = $class->load_config($param);
+
+ $check_delete->($aliases, $param);
+
+ return run_ref_update('fw-alias-delete', $param->{vmid} // $param->{name}, $code);
},
});
}
diff --git a/src/PVE/API2/Firewall/IPSet.pm b/src/PVE/API2/Firewall/IPSet.pm
index 144baf6..05be0fe 100644
--- a/src/PVE/API2/Firewall/IPSet.pm
+++ b/src/PVE/API2/Firewall/IPSet.pm
@@ -2,7 +2,7 @@ package PVE::API2::Firewall::IPSetBase;
use strict;
use warnings;
-use PVE::Firewall::References qw(update_refs);
+use PVE::Firewall::References qw(update_refs run_ref_update);
use PVE::Exception qw(raise raise_param_exc);
use PVE::JSONSchema qw(get_standard_option);
@@ -154,38 +154,50 @@ sub register_delete_ipset {
additionalProperties => 0,
properties => $properties,
},
- returns => { type => 'null' },
+ returns => { type => 'string', optional => 1 },
code => sub {
my ($param) = @_;
- $class->lock_config(
- $param,
- sub {
- my ($param) = @_;
+ my $action = $param->{'dangling-references'}
+ // $properties->{'dangling-references'}->{default};
- my ($cluster_conf, $fw_conf, $ipset) = $class->load_config($param);
+ my $code = sub {
+ $class->lock_config(
+ $param,
+ sub {
+ my ($param) = @_;
- die "IPSet '$param->{name}' is not empty\n"
- if scalar(@$ipset) && !$param->{force};
+ my ($cluster_conf, $fw_conf, $ipset) = $class->load_config($param);
- my $action = $param->{'dangling-references'}
- // $properties->{'dangling-references'}->{default};
+ die "IPSet '$param->{name}' is not empty\n"
+ if scalar(@$ipset) && !$param->{force};
- update_refs({
- conf => $fw_conf,
- kind => 'ipset',
- old => $param->{name},
- env => $class->rule_env(),
- action => $action eq 'keep' ? undef : $action,
- subject => "ipset '$param->{name}' deleted",
- });
+ update_refs({
+ conf => $fw_conf,
+ kind => 'ipset',
+ old => $param->{name},
+ env => $class->rule_env(),
+ action => $action eq 'keep' ? undef : $action,
+ subject => "ipset '$param->{name}' deleted",
+ });
- $class->save_ipset($param, $fw_conf, undef);
+ $class->save_ipset($param, $fw_conf, undef);
+ },
+ );
+ };
- },
- );
+ if (!defined($param->{'dangling-references'})) {
+ $code->();
+ return undef;
+ }
- return undef;
+ # fail on the request instead of inside the task
+ my (undef, undef, $ipset) = $class->load_config($param);
+
+ die "IPSet '$param->{name}' is not empty\n"
+ if scalar($ipset->@*) && !$param->{force};
+
+ return run_ref_update('fw-ipset-delete', $param->{vmid} // $param->{name}, $code);
},
});
}
@@ -548,7 +560,7 @@ package PVE::API2::Firewall::BaseIPSetList;
use strict;
use warnings;
-use PVE::Firewall::References qw(update_refs);
+use PVE::Firewall::References qw(update_refs run_ref_update);
use PVE::JSONSchema qw(get_standard_option);
use PVE::Exception qw(raise_param_exc);
use PVE::Firewall;
@@ -616,6 +628,25 @@ my $get_ipset_list = sub {
return wantarray ? ($list, $digest) : $list;
};
+my $check_rename = sub {
+ my ($conf, $param, $force) = @_;
+
+ my (undef, $digest) = &$get_ipset_list($conf);
+ PVE::Tools::assert_if_modified($digest, $param->{digest});
+
+ raise_param_exc({ name => "IPSet '$param->{rename}' does not exist" })
+ if !$conf->{ipset}->{ $param->{rename} };
+
+ # prevent overwriting existing ipset
+ if (
+ $param->{name} ne $param->{rename}
+ && $conf->{ipset}->{ $param->{name} }
+ && !$force
+ ) {
+ raise_param_exc({ name => "IPSet '$param->{name}' already exists" });
+ }
+};
+
sub register_index {
my ($class) = @_;
@@ -690,93 +721,100 @@ sub register_create {
additionalProperties => 0,
properties => $properties,
},
- returns => { type => 'null' },
+ returns => { type => 'string', optional => 1 },
code => sub {
my ($param) = @_;
- $class->lock_config(
- $param,
- 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}) {
- my (undef, $digest) = &$get_ipset_list($fw_conf);
- PVE::Tools::assert_if_modified($digest, $param->{digest});
-
- raise_param_exc({ name => "IPSet '$param->{rename}' does not exist" })
- if !$fw_conf->{ipset}->{ $param->{rename} };
-
- 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";
+ my $dangling_refs = $param->{'dangling-references'}
+ // $properties->{'dangling-references'}->{default};
+ my $force = $param->{'force-rename'} // $properties->{'force-rename'}->{default};
+
+ my $renaming = $param->{rename} && $param->{name} ne $param->{rename};
+
+ my $code = sub {
+ $class->lock_config(
+ $param,
+ sub {
+ my ($param) = @_;
+
+ my ($cluster_conf, $fw_conf) = $class->load_config($param);
+
+ if ($param->{rename}) {
+ $check_rename->($fw_conf, $param, $force);
+
+ if ($param->{name} ne $param->{rename}) {
+ $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;
+ }
}
- 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 {
+ foreach my $name (keys %{ $fw_conf->{ipset} }) {
+ raise_param_exc({ name => "IPSet '$name' already exists" })
+ if $name eq $param->{name};
}
- }
- $fw_conf->{ipset_comments}->{ $param->{name} } = $param->{comment}
- if defined($param->{comment});
- } else {
- foreach my $name (keys %{ $fw_conf->{ipset} }) {
- raise_param_exc({ name => "IPSet '$name' already exists" })
- if $name eq $param->{name};
+ $fw_conf->{ipset}->{ $param->{name} } = [];
+ $fw_conf->{ipset_comments}->{ $param->{name} } = $param->{comment}
+ if defined($param->{comment});
}
- $fw_conf->{ipset}->{ $param->{name} } = [];
- $fw_conf->{ipset_comments}->{ $param->{name} } = $param->{comment}
- if defined($param->{comment});
- }
+ $class->save_config($param, $fw_conf);
+ },
+ );
+ };
- $class->save_config($param, $fw_conf);
- },
- );
+ if (!$renaming || !defined($param->{'dangling-references'})) {
+ $code->();
+ return undef;
+ }
- return undef;
+ # fail on the request instead of inside the task
+ my (undef, $fw_conf) = $class->load_config($param);
+
+ $check_rename->($fw_conf, $param, $force);
+
+ return run_ref_update('fw-ipset-rename', $param->{vmid} // $param->{rename}, $code);
},
});
}
diff --git a/src/PVE/Firewall/References.pm b/src/PVE/Firewall/References.pm
index b106651..2fb8767 100644
--- a/src/PVE/Firewall/References.pm
+++ b/src/PVE/Firewall/References.pm
@@ -12,6 +12,7 @@ use base 'Exporter';
our @EXPORT_OK = qw(
collect_refs
update_refs
+ run_ref_update
);
sub collect_refs {
@@ -441,6 +442,25 @@ sub update_refs {
return $count;
}
+=head3 run_ref_update($type, $id, $code)
+
+Run C<$code> as a worker task of type C<$type> for C<$id>.
+
+C<$id> is the guest a guest-scoped object belongs to so the task is logged with that guest, and the
+name of the object otherwise.
+
+Return the UPID of the task.
+
+=cut
+
+sub run_ref_update {
+ my ($type, $id, $code) = @_;
+
+ my $rpcenv = PVE::RESTEnvironment::get();
+
+ return $rpcenv->fork_worker($type, $id, $rpcenv->get_user(), $code);
+}
+
=head3 format_ref_change($change)
Format one recorded config change as a single line, e.g.
--
2.47.3
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH pve-firewall v6 12/28] firewall: tests: add tests for object reference updates
2026-11-11 13:18 [PATCH container/docs/firewall/manager/network/qemu-server v6 00/28] handle dangling references when firewall objects go away Arthur Bied-Charreton
` (10 preceding siblings ...)
2026-11-11 13:18 ` [PATCH pve-firewall v6 11/28] firewall: api: run reference updates as tasks Arthur Bied-Charreton
@ 2026-11-11 13:18 ` Arthur Bied-Charreton
2026-11-11 13:18 ` [PATCH pve-firewall v6 13/28] firewall: references: handle security groups Arthur Bied-Charreton
` (15 subsequent siblings)
27 siblings, 0 replies; 29+ messages in thread
From: Arthur Bied-Charreton @ 2026-11-11 13:18 UTC (permalink / raw)
To: pve-devel
The reference updating logic has quite a few edge cases, especially
regarding resolving unscoped references.
Add a tests to consolidate the intended functionality. Note that these
tests do not cover the actual iteration over the different configs in
the cluster, rather they are focused on the core reference rewriting
logic.
Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
---
src/test/Makefile | 2 +-
src/test/test_update_refs.pl | 379 +++++++++++++++++++++++++++++++++++
2 files changed, 380 insertions(+), 1 deletion(-)
create mode 100755 src/test/test_update_refs.pl
diff --git a/src/test/Makefile b/src/test/Makefile
index 74f9e35..506aa76 100644
--- a/src/test/Makefile
+++ b/src/test/Makefile
@@ -1,4 +1,4 @@
-TESTS = test_parse_address_list test_ipt_rule_to_cmds test_parameter_newlines test_compile_ipsets test_ebtables_ipfilter test_simulator_protocols test_interface_names test_restore_errors
+TESTS = test_parse_address_list test_ipt_rule_to_cmds test_parameter_newlines test_compile_ipsets test_ebtables_ipfilter test_simulator_protocols test_interface_names test_restore_errors test_update_refs
.PHONY: all test $(TESTS) clean
all: test
diff --git a/src/test/test_update_refs.pl b/src/test/test_update_refs.pl
new file mode 100755
index 0000000..36afff7
--- /dev/null
+++ b/src/test/test_update_refs.pl
@@ -0,0 +1,379 @@
+#!/usr/bin/env perl
+
+# tests for the alias/ipset reference updating logic
+
+use lib qw(..);
+
+use v5.36;
+
+use Test::More;
+
+use PVE::Firewall::References;
+
+sub rewrite($conf, $kind, $old, $new, $env, $is_guest, $action = undef, $cluster_conf = {}) {
+ my $op = {
+ kind => $kind,
+ old => ref($old) eq 'ARRAY' ? $old : [$old],
+ new => $new,
+ action => $action // (defined($new) ? 'rename' : 'disable'),
+ cluster_conf => $cluster_conf,
+ };
+ return PVE::Firewall::References::rewrite_refs_in_conf($conf, $op, $env, $is_guest);
+}
+
+subtest 'ipset: cluster rename rewrites dc/ and bare refs' => sub {
+ my $conf = {
+ rules => [
+ { source => '+dc/foo' },
+ { dest => '+foo' },
+ { source => '+bar' },
+ { source => '+dc/bar' },
+ ],
+ groups => {
+ grp => [{ source => '+dc/foo' }, { dest => '+foo' }],
+ },
+ ipset => {
+ set => [{ cidr => '10.0.0.0/8' }],
+ },
+ };
+
+ my $changes = rewrite($conf, 'ipset', 'foo', 'baz', 'cluster', 0);
+
+ ok(scalar($changes->@*), 'reports changes');
+ is($conf->{rules}->[0]->{source}, '+dc/baz', 'dc/ ref rewritten');
+ is($conf->{rules}->[1]->{dest}, '+baz', 'bare ref rewritten');
+ is($conf->{rules}->[2]->{source}, '+bar', 'unrelated bare ref kept');
+ is($conf->{rules}->[3]->{source}, '+dc/bar', 'unrelated dc/ ref kept');
+ is($conf->{groups}->{grp}->[0]->{source}, '+dc/baz', 'group dc/ ref rewritten');
+ is($conf->{groups}->{grp}->[1]->{dest}, '+baz', 'group bare ref rewritten');
+ is($conf->{ipset}->{set}->[0]->{cidr}, '10.0.0.0/8', 'ipset member not touched');
+};
+
+subtest 'ipset: cluster delete disables matching rules' => sub {
+ my $conf = {
+ rules => [
+ { source => '+dc/foo', enable => 1 },
+ { source => '+foo', enable => 1 },
+ { source => '+other', enable => 1 },
+ ],
+ groups =>
+ { grp => [{ dest => '+dc/foo', enable => 1 }, { dest => '+keep', enable => 1 }] },
+ };
+
+ my $changes = rewrite($conf, 'ipset', 'foo', undef, 'cluster', 0);
+
+ ok(scalar($changes->@*), 'reports changes');
+ is(scalar($conf->{rules}->@*), 3, 'no rule dropped');
+ is($conf->{rules}->[0]->{enable}, 0, 'dc/ ref rule disabled');
+ is($conf->{rules}->[1]->{enable}, 0, 'bare ref rule disabled');
+ is($conf->{rules}->[2]->{enable}, 1, 'unrelated rule untouched');
+ is($conf->{groups}->{grp}->[0]->{enable}, 0, 'group rule disabled');
+ is($conf->{groups}->{grp}->[1]->{enable}, 1, 'unrelated group rule untouched');
+};
+
+subtest 'ipset: cluster drop removes matching rules' => sub {
+ my $conf = {
+ rules => [
+ { source => '+dc/foo', enable => 1 },
+ { source => '+foo', enable => 1 },
+ { source => '+other', enable => 1 },
+ ],
+ groups =>
+ { grp => [{ dest => '+dc/foo', enable => 1 }, { dest => '+keep', enable => 1 }] },
+ };
+
+ my $changes = rewrite($conf, 'ipset', 'foo', undef, 'cluster', 0, 'drop');
+
+ ok(scalar($changes->@*), 'reports changes');
+ is(scalar($conf->{rules}->@*), 1, 'matching rules dropped');
+ is($conf->{rules}->[0]->{source}, '+other', 'unrelated rule kept');
+ is($conf->{rules}->[0]->{enable}, 1, 'kept rule not disabled');
+ is(scalar($conf->{groups}->{grp}->@*), 1, 'matching group rule dropped');
+ is($conf->{groups}->{grp}->[0]->{dest}, '+keep', 'unrelated group rule kept');
+};
+
+subtest 'ipset: guest shadows cluster object' => sub {
+ my $conf = {
+ rules => [
+ { source => '+foo' }, { dest => '+dc/foo' },
+ ],
+ ipset => { foo => [] },
+ };
+
+ my $changes = rewrite($conf, 'ipset', 'foo', 'baz', 'cluster', 1);
+
+ ok(scalar($changes->@*), 'reports changes');
+ is($conf->{rules}->[0]->{source}, '+foo', 'shadowed bare ref not touched');
+ is($conf->{rules}->[1]->{dest}, '+dc/baz', 'explicit cluster ref rewritten');
+};
+
+subtest 'ipset: guest without own object' => sub {
+ my $conf = {
+ rules => [{ source => '+foo' }, { dest => '+dc/foo' }],
+ ipset => {},
+ };
+
+ rewrite($conf, 'ipset', 'foo', 'baz', 'cluster', 1);
+
+ is($conf->{rules}->[0]->{source}, '+baz', 'implicit cluster ref rewritten');
+ is($conf->{rules}->[1]->{dest}, '+dc/baz', 'explicit cluster ref rewritten');
+};
+
+subtest 'ipset: guest-level rename' => sub {
+ my $conf = {
+ rules => [
+ { source => '+foo' }, { dest => '+guest/foo' }, { source => '+dc/foo' },
+ ],
+ };
+
+ rewrite($conf, 'ipset', 'foo', 'baz', 'vm', 0);
+
+ is($conf->{rules}->[0]->{source}, '+baz', 'implicit guest ref rewritten');
+ is($conf->{rules}->[1]->{dest}, '+guest/baz', 'guest/ ref rewritten');
+ is($conf->{rules}->[2]->{source}, '+dc/foo', 'dc/ ref not touched in guest env');
+};
+
+subtest 'ipset: sdn scope, shadowing and multiple objects' => sub {
+ my $conf = {
+ rules => [
+ { source => '+sdn/vnet0-all', enable => 1 },
+ { dest => '+vnet0-all', enable => 1 },
+ { source => '+sdn/vnet0-dhcp', enable => 1 },
+ { dest => '+vnet0-dhcp', enable => 1 },
+ { source => '+dc/vnet0-all', enable => 1 },
+ ],
+ # shadows the SDN-generated IPSet of the same name
+ ipset => { 'vnet0-dhcp' => [] },
+ };
+
+ my $vnet_ipsets = ['vnet0-all', 'vnet0-dhcp'];
+ my $changes = rewrite($conf, 'ipset', $vnet_ipsets, undef, 'sdn', 0, 'disable', $conf);
+
+ ok(scalar($changes->@*), 'reports changes');
+ is($conf->{rules}->[0]->{enable}, 0, 'sdn/ ref disabled');
+ is($conf->{rules}->[1]->{enable}, 0, 'bare ref disabled');
+ is($conf->{rules}->[2]->{enable}, 0, 'sdn/ ref disabled despite cluster IPSet');
+ is($conf->{rules}->[3]->{enable}, 1, 'bare ref kept, resolves to the cluster IPSet');
+ is($conf->{rules}->[4]->{enable}, 1, 'dc/ ref not touched in sdn env');
+
+ my $guest_conf = {
+ rules => [
+ { source => '+sdn/vnet0-all', enable => 1 }, { dest => '+vnet0-all', enable => 1 },
+ ],
+ ipset => { 'vnet0-all' => [] },
+ };
+
+ rewrite($guest_conf, 'ipset', $vnet_ipsets, undef, 'sdn', 1, 'disable', {});
+
+ is($guest_conf->{rules}->[0]->{enable}, 0, 'sdn/ ref disabled in guest conf');
+ is($guest_conf->{rules}->[1]->{enable}, 1, 'bare ref kept, resolves to the guest IPSet');
+};
+
+subtest 'ipset: not confused with alias' => sub {
+ my $conf = { rules => [{ source => 'foo' }, { dest => '+foo' }] };
+
+ my $changes = rewrite($conf, 'ipset', 'foo', 'baz', 'cluster', 0);
+
+ ok(scalar($changes->@*), 'reports changes');
+ is($conf->{rules}->[0]->{source}, 'foo', 'bare alias ref not touched');
+ is($conf->{rules}->[1]->{dest}, '+baz', 'ipset ref rewritten');
+};
+
+subtest 'alias: cluster rename rewrites rules and ipset members' => sub {
+ my $conf = {
+ rules => [{ source => 'al' }, { dest => 'dc/al' }, { source => 'other' }],
+ groups => { grp => [{ source => 'dc/al' }] },
+ ipset => {
+ set => [{ cidr => 'al' }, { cidr => 'dc/al' }, { cidr => '10.0.0.1' }],
+ },
+ };
+
+ my $changes = rewrite($conf, 'aliases', 'al', 'new', 'cluster', 0);
+
+ ok(scalar($changes->@*), 'reports changes');
+ is($conf->{rules}->[0]->{source}, 'new', 'bare alias in rule rewritten');
+ is($conf->{rules}->[1]->{dest}, 'dc/new', 'dc/ alias in rule rewritten');
+ is($conf->{rules}->[2]->{source}, 'other', 'unrelated rule kept');
+ is($conf->{groups}->{grp}->[0]->{source}, 'dc/new', 'alias in group rewritten');
+ is($conf->{ipset}->{set}->[0]->{cidr}, 'new', 'bare alias in ipset member rewritten');
+ is($conf->{ipset}->{set}->[1]->{cidr}, 'dc/new', 'dc/ alias in ipset member rewritten');
+ is($conf->{ipset}->{set}->[2]->{cidr}, '10.0.0.1', 'literal cidr member kept');
+};
+
+subtest 'alias: cluster delete disables rules, drops ipset members' => sub {
+ my $conf = {
+ rules => [
+ { source => 'al', dest => '10.0.0.1', action => 'ACCEPT', enable => 1 },
+ { source => 'keep', enable => 1 },
+ ],
+ groups =>
+ { grp => [{ source => 'dc/al', enable => 1 }, { source => 'keep', enable => 1 }] },
+ ipset => {
+ set =>
+ [{ cidr => 'al' }, { cidr => 'dc/al', nomatch => 1 }, { cidr => '10.0.0.1' }],
+ },
+ };
+
+ my $changes = rewrite($conf, 'aliases', 'al', undef, 'cluster', 0);
+
+ ok(scalar($changes->@*), 'reports changes');
+
+ is(scalar($conf->{rules}->@*), 2, 'no rule dropped');
+ is($conf->{rules}->[0]->{enable}, 0, 'matching rule disabled');
+ is($conf->{rules}->[0]->{source}, 'al', 'disabled rule keeps its reference');
+ is($conf->{rules}->[0]->{dest}, '10.0.0.1', 'disabled rule keeps its other properties');
+ is($conf->{rules}->[1]->{enable}, 1, 'unrelated rule untouched');
+
+ is(scalar($conf->{groups}->{grp}->@*), 2, 'no group rule dropped');
+ is($conf->{groups}->{grp}->[0]->{enable}, 0, 'matching group rule disabled');
+ is($conf->{groups}->{grp}->[1]->{enable}, 1, 'unrelated group rule untouched');
+
+ is(scalar($conf->{ipset}->{set}->@*), 1, 'matching members dropped, not disabled');
+ is($conf->{ipset}->{set}->[0]->{cidr}, '10.0.0.1', 'literal member kept');
+ ok(
+ !(grep { exists($_->{enable}) } $conf->{ipset}->{set}->@*),
+ 'no ipset member carries an enable flag',
+ );
+};
+
+subtest 'alias: cluster drop removes rules and ipset members' => sub {
+ my $conf = {
+ rules => [{ source => 'al', enable => 1 }, { source => 'keep', enable => 1 }],
+ ipset => { set => [{ cidr => 'al' }, { cidr => 'dc/al' }, { cidr => '10.0.0.1' }] },
+ };
+
+ rewrite($conf, 'aliases', 'al', undef, 'cluster', 0, 'drop');
+
+ is(scalar($conf->{rules}->@*), 1, 'matching rule dropped');
+ is($conf->{rules}->[0]->{source}, 'keep', 'unrelated rule kept');
+ is($conf->{rules}->[0]->{enable}, 1, 'kept rule not disabled');
+ is(scalar($conf->{ipset}->{set}->@*), 1, 'matching members dropped');
+ is($conf->{ipset}->{set}->[0]->{cidr}, '10.0.0.1', 'literal member kept');
+};
+
+subtest 'alias: not confused with ipset' => sub {
+ my $conf = { rules => [{ source => '+al' }, { dest => 'al' }] };
+
+ rewrite($conf, 'aliases', 'al', 'new', 'cluster', 0);
+
+ is($conf->{rules}->[0]->{source}, '+al', 'ipset ref (+) not touched');
+ is($conf->{rules}->[1]->{dest}, 'new', 'alias ref rewritten');
+};
+
+subtest 'case-insensitive match, written back lowercase' => sub {
+ my $conf = { rules => [{ source => '+DC/FOO' }, { dest => '+Foo' }] };
+
+ rewrite($conf, 'ipset', 'foo', 'baz', 'cluster', 0);
+
+ is($conf->{rules}->[0]->{source}, '+dc/baz', 'dc/ ref matched and lowercased');
+ is($conf->{rules}->[1]->{dest}, '+baz', 'bare ref matched and lowercased');
+};
+
+subtest 'no match reports no changes' => sub {
+ my $conf = { rules => [{ source => '+other' }, { dest => 'somealias' }] };
+
+ my $changes = rewrite($conf, 'ipset', 'foo', 'baz', 'cluster', 0);
+
+ is(scalar($changes->@*), 0, 'nothing matched -> no changes');
+ is($conf->{rules}->[0]->{source}, '+other', 'unrelated refs not touched');
+};
+
+subtest 'changes describe every reference that was touched' => sub {
+ my $conf = {
+ rules => [{ source => 'al', enable => 1 }, { source => 'keep', enable => 1 }],
+ groups => { grp => [{ dest => 'dc/al', enable => 1 }] },
+ ipset => { set => [{ cidr => '10.0.0.1' }, { cidr => 'al' }] },
+ };
+
+ my $changes = rewrite($conf, 'aliases', 'al', undef, 'cluster', 0, 'drop');
+
+ is(scalar($changes->@*), 3, 'one change per reference');
+
+ my $by_section = { map { ($_->{section} => $_) } $changes->@* };
+
+ is($by_section->{rules}->{pos}, 0, 'rule position recorded');
+ is($by_section->{rules}->{ref}, 'al', 'matched reference recorded');
+ is($by_section->{rules}->{action}, 'drop', 'action recorded');
+ is($by_section->{group}->{name}, 'grp', 'group name recorded');
+ is($by_section->{group}->{ref}, 'dc/al', 'scoped reference recorded');
+ is($by_section->{ipset}->{name}, 'set', 'ipset name recorded');
+ is($by_section->{ipset}->{pos}, 1, 'member position recorded, literal member skipped');
+
+ # update_refs tags each change with the config it was made in
+ $_->{scope} = 'cluster' for $changes->@*;
+
+ is(
+ PVE::Firewall::References::format_ref_change($by_section->{ipset}),
+ "cluster: ipset 'set': member 1: 'al' dropped",
+ 'drop formatted for the task log',
+ );
+};
+
+subtest 'rename changes record the new reference' => sub {
+ my $conf = { rules => [{ source => '+dc/foo' }] };
+
+ my $changes = rewrite($conf, 'ipset', 'foo', 'baz', 'cluster', 0);
+
+ is($changes->[0]->{new}, '+dc/baz', 'new reference recorded');
+
+ $changes->[0]->{scope} = 'vm/100';
+
+ is(
+ PVE::Firewall::References::format_ref_change($changes->[0]),
+ "vm/100: rules: rule 0: '+dc/foo' renamed to '+dc/baz'",
+ 'rename formatted for the task log',
+ );
+};
+
+subtest 'update_refs: rejects invalid argument combinations' => sub {
+ # monkey patches to prevent tests from actually walking the cluster
+ no warnings qw(redefine once);
+ local *PVE::Cluster::get_vmlist = sub { {} };
+ local *PVE::Cluster::get_nodelist = sub { [] };
+ local *PVE::Firewall::list_vnetfw_confs = sub { [] };
+
+ my $run = sub {
+ my (%args) = @_;
+
+ eval {
+ PVE::Firewall::References::update_refs({
+ conf => { rules => [] },
+ kind => 'ipset',
+ env => 'vm',
+ subject => 'test',
+ log => sub { },
+ %args,
+ });
+ };
+
+ return $@;
+ };
+
+ like(
+ $run->(old => 'foo', new => 'baz', action => 'bogus'),
+ qr/invalid action/,
+ 'unknown action rejected',
+ );
+ like(
+ $run->(kind => 'bogus', old => 'foo', new => 'baz', action => 'rename'),
+ qr/unknown object kind/,
+ 'unknown object kind rejected',
+ );
+ like(
+ $run->(old => ['foo', 'bar'], new => 'baz', action => 'rename'),
+ qr/cannot rename more than one object/,
+ 'renaming several objects rejected',
+ );
+ like(
+ $run->(old => 'foo', new => 'baz', env => 'cluster', action => 'rename'),
+ qr/\$save is required/,
+ 'cluster rename without a save callback rejected',
+ );
+ unlike(
+ $run->(old => 'foo', env => 'cluster', action => 'drop'),
+ qr/\$save is required/,
+ 'a drop needs no save callback',
+ );
+};
+
+done_testing();
--
2.47.3
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH pve-firewall v6 13/28] firewall: references: handle security groups
2026-11-11 13:18 [PATCH container/docs/firewall/manager/network/qemu-server v6 00/28] handle dangling references when firewall objects go away Arthur Bied-Charreton
` (11 preceding siblings ...)
2026-11-11 13:18 ` [PATCH pve-firewall v6 12/28] firewall: tests: add tests for object reference updates Arthur Bied-Charreton
@ 2026-11-11 13:18 ` Arthur Bied-Charreton
2026-11-11 13:18 ` [PATCH pve-firewall v6 14/28] firewall: groups: add option to handle dangling references on rename Arthur Bied-Charreton
` (14 subsequent siblings)
27 siblings, 0 replies; 29+ messages in thread
From: Arthur Bied-Charreton @ 2026-11-11 13:18 UTC (permalink / raw)
To: pve-devel
Security groups may be referenced by the action of a rule, rather than
the source, dest and cidr fields that ipsets and aliases are referenced
in.
Move field lists into the object spec and let a spec limit its
references to a single rule type so a group named 'drop' is not matched
by the action of a normal DROP rule.
Groups may only be defined in the cluster config so they cannot be
shadowed and references to them are always unscoped.
Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
---
src/PVE/Firewall/References.pm | 52 +++++++++++++++++++++-------------
src/test/test_update_refs.pl | 38 +++++++++++++++++++++++++
2 files changed, 70 insertions(+), 20 deletions(-)
diff --git a/src/PVE/Firewall/References.pm b/src/PVE/Firewall/References.pm
index 2fb8767..baefccc 100644
--- a/src/PVE/Firewall/References.pm
+++ b/src/PVE/Firewall/References.pm
@@ -157,27 +157,37 @@ sub rewrite_refs_in_env {
return $changes;
}
-# field name + value prefix for firewall object references
+# how a reference to a firewall object looks and where it is found:
+#
+# prefix: the prefix a reference carries
+# self: the section a config defines the object in, used to resolve shadowing
+# fields: the fields that can hold a reference
+# type: the rule type references are limited to
my $object_ref_specs = {
- ipset => { prefix => '+', self => 'ipset' },
- aliases => { prefix => '', self => 'aliases' },
+ ipset => { prefix => '+', self => 'ipset', fields => ['source', 'dest', 'cidr'] },
+ aliases => { prefix => '', self => 'aliases', fields => ['source', 'dest', 'cidr'] },
+ # a security group may only be referenced by the action of the rule invoking it. matching
+ # any other rule could rewrite a DROP action when renaming a group named 'drop', for example.
+ groups => { prefix => '', self => 'groups', fields => ['action'], type => 'group' },
};
-# fields to check for object references
-my $ref_fields = ['source', 'dest', 'cidr'];
+=head3 matched_ref($item, $ctx)
-=head3 matched_ref($item, $repl)
-
-Return the reference C<$item> holds that C<$repl> has an entry for, or C<undef> if there is none.
+Return the reference C<$item> holds that the replacement map of C<$ctx> has an entry for, or
+C<undef> if there is none.
=cut
sub matched_ref {
- my ($item, $repl) = @_;
+ my ($item, $ctx) = @_;
+
+ my $spec = $ctx->{spec};
- for my $field ($ref_fields->@*) {
+ return undef if $spec->{type} && ($item->{type} // '') ne $spec->{type};
+
+ for my $field ($spec->{fields}->@*) {
my $ref = lc($item->{$field} // '');
- return $ref if exists($repl->{$ref});
+ return $ref if exists($ctx->{repl}->{$ref});
}
return undef;
@@ -209,7 +219,7 @@ sub rewrite_items {
for my $pos (0 .. $items->$#*) {
my $item = $items->[$pos];
- my $ref = matched_ref($item, $repl);
+ my $ref = matched_ref($item, $ctx);
if (!defined($ref)) {
# no match, keep item
push @result, $item;
@@ -229,7 +239,7 @@ sub rewrite_items {
next if $remove;
if ($action eq 'rename') {
- for my $field ($ref_fields->@*) {
+ for my $field ($ctx->{spec}->{fields}->@*) {
my $value = lc($item->{$field} // '');
$item->{$field} = $repl->{$value} if exists($repl->{$value});
}
@@ -243,7 +253,7 @@ sub rewrite_items {
return \@result;
}
-=head3 build_ref_map($op, $conf, $env, $is_guest)
+=head3 build_ref_map($op, $spec, $conf, $env, $is_guest)
Map every allowed reference to the objects in C<$op> that resolves in C<$conf> to what it should
become, i.e. either the renamed reference or C<undef> if C<$op> has no new name.
@@ -266,9 +276,7 @@ a guest-local or a cluster object
=cut
sub build_ref_map {
- my ($op, $conf, $env, $is_guest) = @_;
-
- my $spec = $object_ref_specs->{ $op->{kind} } // die "unknown object kind '$op->{kind}'\n";
+ my ($op, $spec, $conf, $env, $is_guest) = @_;
my $prefix = $spec->{prefix};
my $self = $spec->{self};
@@ -323,8 +331,11 @@ Only exposed for testing, see C<update_refs>'s POD for details.
sub rewrite_refs_in_conf {
my ($conf, $op, $env, $is_guest) = @_;
+ my $spec = $object_ref_specs->{ $op->{kind} } // die "unknown object kind '$op->{kind}'\n";
+
my $ctx = {
- repl => build_ref_map($op, $conf, $env, $is_guest),
+ spec => $spec,
+ repl => build_ref_map($op, $spec, $conf, $env, $is_guest),
action => $op->{action},
changes => [],
};
@@ -360,7 +371,7 @@ and renames are written back lowercased.
C<$opts> holds:
conf # the firewall config the object is defined in
- kind # 'ipset' or 'aliases'
+ kind # 'ipset', 'aliases' or 'groups'
old # the old name, or an arrayref of names to disable or drop
new # the name to rewrite them references to 'old' to, only used when renaming
env # 'cluster', 'sdn', or 'vm'/'ct' for a guest-defined object
@@ -371,7 +382,8 @@ C<$opts> holds:
The caller is responsible for locking and saving C<conf> itself.
-IPSet members have no disabled state, so C<disable> removes them as well.
+A security group may only be referenced by the action of a rule so those references are confined to
+rules. IPSet members have no disabled state, so C<disable> removes them as well.
If C<conf> is the cluster config, i.e. if C<env> is C<cluster> or C<sdn>, guest, host and vnet
configs are sequentially locked, updated and saved. C<save> is called once the references in
diff --git a/src/test/test_update_refs.pl b/src/test/test_update_refs.pl
index 36afff7..b4262b3 100755
--- a/src/test/test_update_refs.pl
+++ b/src/test/test_update_refs.pl
@@ -325,6 +325,44 @@ subtest 'rename changes record the new reference' => sub {
);
};
+subtest 'groups: rename rewrites the action of group rules' => sub {
+ my $conf = {
+ rules => [
+ { type => 'group', action => 'web' },
+ { type => 'in', action => 'ACCEPT', source => 'web' },
+ ],
+ groups => {
+ web => [{ type => 'in', action => 'ACCEPT' }],
+ },
+ ipset => {
+ set => [{ cidr => 'web' }],
+ },
+ };
+
+ my $changes = rewrite($conf, 'groups', 'web', 'www', 'cluster', 0);
+
+ is($conf->{rules}->[0]->{action}, 'www', 'group rule points at the new name');
+ is($conf->{rules}->[1]->{action}, 'ACCEPT', 'a plain rule keeps its action');
+ is($conf->{rules}->[1]->{source}, 'web', 'a source matching the group name is left alone');
+ is($conf->{ipset}->{set}->[0]->{cidr}, 'web', 'an IPSet member is left alone');
+ is(scalar($changes->@*), 1, 'only the group rule is reported');
+};
+
+subtest 'groups: a group named after an action does not match plain rules' => sub {
+ my $conf = {
+ rules => [
+ { type => 'out', action => 'DROP' }, { type => 'group', action => 'drop' },
+ ],
+ };
+
+ my $changes = rewrite($conf, 'groups', 'drop', undef, 'cluster', 0);
+
+ is($conf->{rules}->[0]->{action}, 'DROP', 'the DROP rule is untouched');
+ ok(!$conf->{rules}->[0]->{enable}, 'the DROP rule is not disabled');
+ is($conf->{rules}->[1]->{enable}, 0, 'the group rule is disabled');
+ is(scalar($changes->@*), 1, 'only the group rule is reported');
+};
+
subtest 'update_refs: rejects invalid argument combinations' => sub {
# monkey patches to prevent tests from actually walking the cluster
no warnings qw(redefine once);
--
2.47.3
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH pve-firewall v6 14/28] firewall: groups: add option to handle dangling references on rename
2026-11-11 13:18 [PATCH container/docs/firewall/manager/network/qemu-server v6 00/28] handle dangling references when firewall objects go away Arthur Bied-Charreton
` (12 preceding siblings ...)
2026-11-11 13:18 ` [PATCH pve-firewall v6 13/28] firewall: references: handle security groups Arthur Bied-Charreton
@ 2026-11-11 13:18 ` Arthur Bied-Charreton
2026-11-11 13:18 ` [PATCH pve-firewall v6 15/28] fix #8176: firewall: groups: handle dangling references on delete Arthur Bied-Charreton
` (13 subsequent siblings)
27 siblings, 0 replies; 29+ messages in thread
From: Arthur Bied-Charreton @ 2026-11-11 13:18 UTC (permalink / raw)
To: pve-devel
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 <a.bied-charreton@proxmox.com>
---
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
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH pve-firewall v6 15/28] fix #8176: firewall: groups: handle dangling references on delete
2026-11-11 13:18 [PATCH container/docs/firewall/manager/network/qemu-server v6 00/28] handle dangling references when firewall objects go away Arthur Bied-Charreton
` (13 preceding siblings ...)
2026-11-11 13:18 ` [PATCH pve-firewall v6 14/28] firewall: groups: add option to handle dangling references on rename Arthur Bied-Charreton
@ 2026-11-11 13:18 ` Arthur Bied-Charreton
2026-11-11 13:18 ` [PATCH pve-network v6 16/28] apply: add option to handle dangling references on VNet deletion Arthur Bied-Charreton
` (12 subsequent siblings)
27 siblings, 0 replies; 29+ messages in thread
From: Arthur Bied-Charreton @ 2026-11-11 13:18 UTC (permalink / raw)
To: pve-devel
Deleting a security group referenced by rules leaves dangling
references. Nothing checks that a group rule names a group that exists.
The dangling reference is carried into the generated ruleset and what
happens then depends on the firewall implementation.
Add a 'dangling-references' option to the delete endpoint, to either
'disable' the referencing rules or 'drop' them along with the security
group. The default, 'keep', leaves them as they are.
If that option is passed, the delete runs as a task so the changes made
to the configs can be found in the task log afterwards.
Link: https://bugzilla.proxmox.com/show_bug.cgi?id=8176
Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
---
src/PVE/API2/Firewall/Rules.pm | 49 +++++++++++++++++++++++++---------
1 file changed, 37 insertions(+), 12 deletions(-)
diff --git a/src/PVE/API2/Firewall/Rules.pm b/src/PVE/API2/Firewall/Rules.pm
index 785ad05..9931d0c 100644
--- a/src/PVE/API2/Firewall/Rules.pm
+++ b/src/PVE/API2/Firewall/Rules.pm
@@ -487,6 +487,7 @@ package PVE::API2::Firewall::GroupRules;
use strict;
use warnings;
use PVE::JSONSchema qw(get_standard_option);
+use PVE::Firewall::References qw(update_refs run_ref_update);
use base qw(PVE::API2::Firewall::RulesBase);
@@ -539,27 +540,51 @@ __PACKAGE__->register_method({
additionalProperties => 0,
properties => {
group => get_standard_option('pve-security-group-name'),
+ 'dangling-references' => get_standard_option('pve-dangling-references-delete'),
},
},
- returns => { type => 'null' },
+ returns => { type => 'string', optional => 1 },
code => sub {
my ($param) = @_;
- __PACKAGE__->lock_config(
- $param,
- sub {
- my ($param) = @_;
+ my $action = $param->{'dangling-references'} // 'keep';
- my (undef, $cluster_conf, $rules) = __PACKAGE__->load_config($param);
+ my $code = sub {
+ __PACKAGE__->lock_config(
+ $param,
+ sub {
+ my ($param) = @_;
- die "Security group '$param->{group}' is not empty\n"
- if scalar(@$rules);
+ my (undef, $cluster_conf, $rules) = __PACKAGE__->load_config($param);
- __PACKAGE__->save_rules($param, $cluster_conf, undef);
- },
- );
+ die "Security group '$param->{group}' is not empty\n"
+ if scalar(@$rules);
+
+ update_refs({
+ conf => $cluster_conf,
+ kind => 'groups',
+ old => $param->{group},
+ env => 'cluster',
+ action => $action eq 'keep' ? undef : $action,
+ subject => "security group '$param->{group}' deleted",
+ });
+
+ __PACKAGE__->save_rules($param, $cluster_conf, undef);
+ },
+ );
+ };
+
+ if (!defined($param->{'dangling-references'})) {
+ $code->();
+ return undef;
+ }
+
+ # fail on the request instead of inside the task.
+ my (undef, undef, $rules) = __PACKAGE__->load_config($param);
+
+ die "Security group '$param->{group}' is not empty\n" if scalar($rules->@*);
- return undef;
+ return run_ref_update('fw-group-delete', $param->{group}, $code);
},
});
--
2.47.3
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH pve-network v6 16/28] apply: add option to handle dangling references on VNet deletion
2026-11-11 13:18 [PATCH container/docs/firewall/manager/network/qemu-server v6 00/28] handle dangling references when firewall objects go away Arthur Bied-Charreton
` (14 preceding siblings ...)
2026-11-11 13:18 ` [PATCH pve-firewall v6 15/28] fix #8176: firewall: groups: handle dangling references on delete Arthur Bied-Charreton
@ 2026-11-11 13:18 ` Arthur Bied-Charreton
2026-11-11 13:18 ` [PATCH qemu-server v6 17/28] api: destroy_vm: add option to handle dangling IPSet references Arthur Bied-Charreton
` (11 subsequent siblings)
27 siblings, 0 replies; 29+ messages in thread
From: Arthur Bied-Charreton @ 2026-11-11 13:18 UTC (permalink / raw)
To: pve-devel
For each VNet, 4 IPSets are auto-generated (<vnet>-all, -gateway,
-no-gateway and -dhcp) [0]. Therefore, deleting a VNet can lead to
rules referencing non-existing IPSets, which the firewall fails to
parse and ignores.
Add a 'dangling-ipset-references' option for PUT /cluster/sdn allowing
to 'drop' rules referencing now-deleted IPSets, 'disable' them, or
'keep' them as they are.
[0] https://pve.proxmox.com/wiki/Software-Defined_Network#pvesdn_firewall_integration
Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
---
src/PVE/API2/Network/SDN.pm | 39 +++++++++++++++++++++++++++++++++++++
1 file changed, 39 insertions(+)
diff --git a/src/PVE/API2/Network/SDN.pm b/src/PVE/API2/Network/SDN.pm
index e3c8d9d..cc290e9 100644
--- a/src/PVE/API2/Network/SDN.pm
+++ b/src/PVE/API2/Network/SDN.pm
@@ -8,6 +8,8 @@ use JSON qw(from_json);
use PVE::Cluster qw(cfs_lock_file cfs_read_file cfs_write_file);
use PVE::Exception qw(raise_param_exc);
+use PVE::Firewall;
+use PVE::Firewall::References;
use PVE::JSONSchema qw(get_standard_option);
use PVE::RESTHandler;
use PVE::RPCEnvironment;
@@ -302,6 +304,15 @@ __PACKAGE__->register_method({
description =>
'When lock-token has been provided and configuration successfully committed, release the lock automatically afterwards',
},
+ 'dangling-ipset-references' => get_standard_option(
+ 'pve-dangling-references-delete',
+ {
+ description =>
+ 'If the applied SDN changes delete one or more VNets, handle the firewall '
+ . 'rules referencing their auto-generated IPSets. Use \'disable\' to '
+ . 'disable those rules, or \'drop\' to remove them entirely.',
+ },
+ ),
},
},
returns => {
@@ -315,13 +326,36 @@ __PACKAGE__->register_method({
my $lock_token = extract_param($param, 'lock-token');
my $release_lock = extract_param($param, 'release-lock');
+ my $dangling_refs = extract_param($param, 'dangling-ipset-references') // 'keep';
my $previous_config_has_frr;
my $new_config_has_frr;
+ my $ref_log = [];
+ my $ref_err;
+
PVE::Network::SDN::lock_sdn_config(
sub {
$previous_config_has_frr = PVE::Network::SDN::running_config_has_frr();
+
+ if ($dangling_refs ne 'keep') {
+ my $running = PVE::Network::SDN::running_config() // {};
+ my $vnets = PVE::Network::SDN::Vnets::config() // {};
+ my $removed =
+ [grep { !$vnets->{ids}->{$_} } keys $running->{vnets}->{ids}->%*];
+
+ if ($removed->@*) {
+ eval {
+ PVE::Firewall::References::update_vnet_ipset_refs(
+ $removed,
+ $dangling_refs,
+ sub { push $ref_log->@*, $_[0] },
+ );
+ };
+ $ref_err = $@ if $@;
+ }
+ }
+
PVE::Network::SDN::commit_config();
$new_config_has_frr = PVE::Network::SDN::running_config_has_frr();
@@ -336,6 +370,11 @@ __PACKAGE__->register_method({
my $regenerate_frr = ($previous_config_has_frr || $new_config_has_frr) ? 1 : 0;
my $code = sub {
+ # the rewrite has to happen under the SDN lock and before the config is committed,
+ # which is before there is a task, so only log here.
+ print("$_\n") for $ref_log->@*;
+ warn "could not update references to VNet IPSets: $ref_err" if $ref_err;
+
$rpcenv->{type} = 'priv'; # to start tasks in background
PVE::Cluster::check_cfs_quorum();
my $nodelist = PVE::Cluster::get_nodelist();
--
2.47.3
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH qemu-server v6 17/28] api: destroy_vm: add option to handle dangling IPSet references
2026-11-11 13:18 [PATCH container/docs/firewall/manager/network/qemu-server v6 00/28] handle dangling references when firewall objects go away Arthur Bied-Charreton
` (15 preceding siblings ...)
2026-11-11 13:18 ` [PATCH pve-network v6 16/28] apply: add option to handle dangling references on VNet deletion Arthur Bied-Charreton
@ 2026-11-11 13:18 ` Arthur Bied-Charreton
2026-11-11 13:18 ` [PATCH pve-container v6 18/28] " Arthur Bied-Charreton
` (10 subsequent siblings)
27 siblings, 0 replies; 29+ messages in thread
From: Arthur Bied-Charreton @ 2026-11-11 13:18 UTC (permalink / raw)
To: pve-devel
For each guest with IPAM entries, a 'guest-ipam-<vmid>' IPSet is
auto-generated [0]. Therefore, deleting a guest can lead to firewall
rules referencing non-existing IPSets, which the firewall fails to
parse and ignores.
Add a 'dangling-ipset-references' option for DELETE
/nodes/{node}/qemu/{vmid} allowing to 'drop' rules referencing
now-deleted IPSets, to 'disable' them, or to 'keep' them as they are.
[0] https://pve.proxmox.com/wiki/Software-Defined_Network#pvesdn_firewall_integration
Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
---
src/PVE/API2/Qemu.pm | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/src/PVE/API2/Qemu.pm b/src/PVE/API2/Qemu.pm
index 7830d09b..a0cc0ca5 100644
--- a/src/PVE/API2/Qemu.pm
+++ b/src/PVE/API2/Qemu.pm
@@ -61,6 +61,7 @@ use PVE::AccessControl;
use PVE::INotify;
use PVE::Network;
use PVE::Firewall;
+use PVE::Firewall::References;
use PVE::API2::Firewall::VM;
use PVE::API2::Qemu::Agent;
use PVE::API2::Qemu::HMPPerms;
@@ -2860,6 +2861,15 @@ __PACKAGE__->register_method({
optional => 1,
default => 0,
},
+ 'dangling-ipset-references' => get_standard_option(
+ 'pve-dangling-references-delete',
+ {
+ description =>
+ 'Handle firewall rules referencing the IPSet auto-generated for '
+ . 'this guest (\'guest-ipam-<vmid>\'). Use \'disable\' to disable them, or '
+ . '\'drop\' to remove them entirely.',
+ },
+ ),
},
},
returns => {
@@ -2937,6 +2947,16 @@ __PACKAGE__->register_method({
}
}
+ my $action = $param->{'dangling-ipset-references'} // 'keep';
+ if ($action ne 'keep') {
+ eval {
+ PVE::Firewall::References::update_guest_ipam_ipset_refs(
+ $vmid, $action,
+ );
+ };
+ warn "could not $action dangling IPSet references: $@" if $@;
+ }
+
# only now remove the zombie config, else we can have reuse race
PVE::QemuConfig->destroy_config($vmid);
},
--
2.47.3
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH pve-container v6 18/28] api: destroy_vm: add option to handle dangling IPSet references
2026-11-11 13:18 [PATCH container/docs/firewall/manager/network/qemu-server v6 00/28] handle dangling references when firewall objects go away Arthur Bied-Charreton
` (16 preceding siblings ...)
2026-11-11 13:18 ` [PATCH qemu-server v6 17/28] api: destroy_vm: add option to handle dangling IPSet references Arthur Bied-Charreton
@ 2026-11-11 13:18 ` Arthur Bied-Charreton
2026-11-11 13:18 ` [PATCH pve-manager v6 19/28] ui: firewall: add a common widget for handling dangling references Arthur Bied-Charreton
` (9 subsequent siblings)
27 siblings, 0 replies; 29+ messages in thread
From: Arthur Bied-Charreton @ 2026-11-11 13:18 UTC (permalink / raw)
To: pve-devel
For each guest with IPAM entries, a 'guest-ipam-<vmid>' IPSet is
auto-generated [0]. Therefore, deleting a guest can lead to firewall
rules referencing non-existing IPSets, which the firewall fails to
parse and ignores.
Add a 'dangling-ipset-references' option for DELETE
/nodes/{node}/lxc/{vmid} allowing to 'drop' rules referencing
now-deleted IPSets, to 'disable' them, or to 'keep' them as they are.
[0] https://pve.proxmox.com/wiki/Software-Defined_Network#pvesdn_firewall_integration
Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
---
src/PVE/API2/LXC.pm | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
index d736dac..6d5d49e 100644
--- a/src/PVE/API2/LXC.pm
+++ b/src/PVE/API2/LXC.pm
@@ -13,6 +13,7 @@ use PVE::Cluster qw(cfs_read_file);
use PVE::DataCenterConfig;
use PVE::Exception qw(raise raise_param_exc raise_perm_exc);
use PVE::Firewall;
+use PVE::Firewall::References;
use PVE::GuestHelpers;
use PVE::INotify;
use PVE::JSONSchema qw(get_standard_option);
@@ -849,6 +850,15 @@ __PACKAGE__->register_method({
. " enabled storages which are not referenced in the config.",
optional => 1,
},
+ 'dangling-ipset-references' => get_standard_option(
+ 'pve-dangling-references-delete',
+ {
+ description =>
+ 'Handle firewall rules referencing the IPSet auto-generated for '
+ . 'this guest (\'guest-ipam-<vmid>\'). Use \'disable\' to disable them, or '
+ . '\'drop\' to remove them entirely.',
+ },
+ ),
},
},
returns => {
@@ -926,6 +936,14 @@ __PACKAGE__->register_method({
}
}
+ my $action = $param->{'dangling-ipset-references'} // 'keep';
+ if ($action ne 'keep') {
+ eval {
+ PVE::Firewall::References::update_guest_ipam_ipset_refs($vmid, $action);
+ };
+ warn "could not $action dangling IPSet references: $@" if $@;
+ }
+
# only now remove the zombie config, else we can have reuse race
PVE::LXC::Config->destroy_config($vmid);
};
--
2.47.3
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH pve-manager v6 19/28] ui: firewall: add a common widget for handling dangling references
2026-11-11 13:18 [PATCH container/docs/firewall/manager/network/qemu-server v6 00/28] handle dangling references when firewall objects go away Arthur Bied-Charreton
` (17 preceding siblings ...)
2026-11-11 13:18 ` [PATCH pve-container v6 18/28] " Arthur Bied-Charreton
@ 2026-11-11 13:18 ` Arthur Bied-Charreton
2026-11-11 13:18 ` [PATCH pve-manager v6 20/28] ui: firewall: ipset: add controls for " Arthur Bied-Charreton
` (8 subsequent siblings)
27 siblings, 0 replies; 29+ messages in thread
From: Arthur Bied-Charreton @ 2026-11-11 13:18 UTC (permalink / raw)
To: pve-devel
The ipset and alias grids both need to let users decide what happens to
the rules and IPSet members referencing an object they are about to
rename or delete. Add the pieces as reusable components.
FirewallReferenceAction is a ComboGrid over a list of actions,
submitting the chosen one as 'dangling-references'. The picker shows
each action's description in its own column. The actions differ per
endpoint so they are passed by the caller.
FirewallObjectRemove extends ConfirmRemoveDialog with such a selector,
for the delete paths that StdRemoveButton cannot handle because it
submits no extra parameters. The selector sits in an 'Advanced' section
there.
Callers will be added in subsequent commits.
Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
---
www/manager6/Makefile | 1 +
www/manager6/grid/FirewallObjectCommon.js | 134 ++++++++++++++++++++++
2 files changed, 135 insertions(+)
create mode 100644 www/manager6/grid/FirewallObjectCommon.js
diff --git a/www/manager6/Makefile b/www/manager6/Makefile
index d2ea786b..e211ad40 100644
--- a/www/manager6/Makefile
+++ b/www/manager6/Makefile
@@ -98,6 +98,7 @@ JSSRC= \
form/TagFieldSet.js \
form/IsoSelector.js \
grid/BackupView.js \
+ grid/FirewallObjectCommon.js \
grid/FirewallAliases.js \
grid/FirewallOptions.js \
grid/FirewallRules.js \
diff --git a/www/manager6/grid/FirewallObjectCommon.js b/www/manager6/grid/FirewallObjectCommon.js
new file mode 100644
index 00000000..04dddf48
--- /dev/null
+++ b/www/manager6/grid/FirewallObjectCommon.js
@@ -0,0 +1,134 @@
+Ext.define('PVE.window.FirewallObjectRemove', {
+ extend: 'Proxmox.window.ConfirmRemoveDialog',
+ alias: 'widget.pveFirewallObjectRemove',
+
+ width: 500,
+
+ // [{ value, text, description }, ...]
+ referenceActions: undefined,
+
+ initComponent: function () {
+ let me = this;
+
+ let stateProvider = Ext.state.Manager.getProvider();
+ let advanced = stateProvider.get('proxmox-advanced-cb');
+
+ me.additionalItems = [
+ {
+ xtype: 'pveFirewallReferenceAction',
+ // API default is 'keep' for backwards compatibility
+ preferredValue: 'drop',
+ referenceActions: me.referenceActions,
+ hidden: !advanced,
+ },
+ ];
+
+ me.callParent();
+
+ let field = me.down('pveFirewallReferenceAction');
+ let footer = me.down('[dock=bottom]');
+
+ footer.insert(0, {
+ xtype: 'container',
+ flex: 1,
+ layout: { type: 'hbox', align: 'middle', pack: 'end' },
+ items: [
+ {
+ xtype: 'proxmoxcheckbox',
+ boxLabel: gettext('Advanced'),
+ boxLabelAlign: 'before',
+ margin: '0 10 0 0',
+ stateId: 'proxmox-advanced-cb',
+ value: advanced,
+ listeners: {
+ change: function (checkbox, value) {
+ field.setVisible(value);
+ stateProvider.set('proxmox-advanced-cb', value);
+ },
+ },
+ },
+ ],
+ });
+
+ footer.add({
+ xtype: 'container',
+ flex: 1,
+ layout: { type: 'hbox', align: 'middle', pack: 'start' },
+ items: [
+ {
+ xtype: 'proxmoxHelpButton',
+ onlineHelp: 'pve_firewall_dangling_references',
+ listenToGlobalEvent: false,
+ hidden: false,
+ margin: '0 0 0 10',
+ },
+ ],
+ });
+ },
+
+ getParams: function () {
+ let me = this;
+
+ let field = me.down('pveFirewallReferenceAction');
+
+ me.params = Ext.applyIf({ [field.getName()]: field.getValue() }, me.params);
+
+ return me.callParent();
+ },
+});
+
+Ext.define('PVE.form.FirewallReferenceAction', {
+ extend: 'Proxmox.form.ComboGrid',
+ alias: 'widget.pveFirewallReferenceAction',
+
+ fieldLabel: gettext('Dangling references'),
+
+ // submitted parameter
+ name: 'dangling-references',
+
+ preferredValue: 'keep',
+
+ editable: false,
+ allowBlank: false,
+ valueField: 'value',
+ displayField: 'text',
+
+ // [{ value, text, description }, ...]
+ referenceActions: undefined,
+
+ initComponent: function () {
+ let me = this;
+
+ if (!me.referenceActions?.length) {
+ throw new Error('no reference actions given, cannot initialize component');
+ }
+
+ me.store = Ext.create('Ext.data.Store', {
+ fields: ['value', 'text', 'description'],
+ data: me.referenceActions,
+ });
+
+ me.listConfig = {
+ width: 560,
+ resizable: true,
+ columns: [
+ {
+ header: gettext('Action'),
+ dataIndex: 'text',
+ flex: 1,
+ },
+ {
+ header: gettext('Description'),
+ dataIndex: 'description',
+ cellWrap: true,
+ flex: 4,
+ },
+ ],
+ };
+
+ me.callParent();
+
+ // ComboGrid applies preferredValue on store load
+ me.setValue(me.preferredValue);
+ },
+});
--
2.47.3
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH pve-manager v6 20/28] ui: firewall: ipset: add controls for dangling references
2026-11-11 13:18 [PATCH container/docs/firewall/manager/network/qemu-server v6 00/28] handle dangling references when firewall objects go away Arthur Bied-Charreton
` (18 preceding siblings ...)
2026-11-11 13:18 ` [PATCH pve-manager v6 19/28] ui: firewall: add a common widget for handling dangling references Arthur Bied-Charreton
@ 2026-11-11 13:18 ` Arthur Bied-Charreton
2026-11-11 13:18 ` [PATCH pve-manager v6 21/28] ui: firewall: aliases: " Arthur Bied-Charreton
` (7 subsequent siblings)
27 siblings, 0 replies; 29+ messages in thread
From: Arthur Bied-Charreton @ 2026-11-11 13:18 UTC (permalink / raw)
To: pve-devel
Add the shared dangling-references action selector to the update dialog
and replace the plain remove button with a confirmation dialog carrying
the same selector to let users choose how to handle dangling references
on config changes.
'force-rename' is not offered. It is meant as an escape hatch to recover
from a partially applied rename, putting it next to the normal actions
makes it too easy to select.
StdRemoveButton cannot pass the extra 'dangling-references' parameter,
so the remove button is switched to a plain button that opens the
shared FirewallObjectRemove dialog.
Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
---
www/manager6/panel/IPSet.js | 80 +++++++++++++++++++++++++++++++++----
1 file changed, 72 insertions(+), 8 deletions(-)
diff --git a/www/manager6/panel/IPSet.js b/www/manager6/panel/IPSet.js
index d510ae6d..40c1ef4b 100644
--- a/www/manager6/panel/IPSet.js
+++ b/www/manager6/panel/IPSet.js
@@ -67,11 +67,7 @@ Ext.define('PVE.IPSetList', {
if (!rec || !canEdit) {
return;
}
- var win = Ext.create('Proxmox.window.Edit', {
- subject: "IPSet '" + rec.data.name + "'",
- url: me.base_url,
- method: 'POST',
- digest: rec.data.digest,
+ var ipanel = Ext.create('Proxmox.panel.InputPanel', {
items: [
{
xtype: 'hiddenfield',
@@ -92,6 +88,45 @@ Ext.define('PVE.IPSetList', {
fieldLabel: gettext('Comment'),
},
],
+ advancedItems: [
+ {
+ xtype: 'pveFirewallReferenceAction',
+ preferredValue: 'rename',
+ referenceActions: [
+ {
+ text: gettext('Keep'),
+ value: 'keep',
+ description: gettext('Do not touch firewall rules referencing this IPSet.'),
+ },
+ {
+ text: gettext('Disable'),
+ value: 'disable',
+ description: gettext('Disable firewall rules referencing this IPSet.'),
+ },
+ {
+ text: gettext('Delete'),
+ value: 'drop',
+ description: gettext('Delete firewall rules referencing this IPSet.'),
+ },
+ {
+ text: gettext('Rename'),
+ value: 'rename',
+ description: gettext(
+ 'Rewrite references to this IPSet to point to the new name.',
+ ),
+ },
+ ],
+ },
+ ],
+ });
+
+ var win = Ext.create('Proxmox.window.Edit', {
+ subject: "IPSet '" + rec.data.name + "'",
+ url: me.base_url,
+ method: 'POST',
+ digest: rec.data.digest,
+ onlineHelp: 'pve_firewall_dangling_references',
+ items: [ipanel],
});
win.show();
win.on('destroy', reload);
@@ -134,11 +169,40 @@ Ext.define('PVE.IPSetList', {
},
});
- me.removeBtn = Ext.create('Proxmox.button.StdRemoveButton', {
+ me.removeBtn = Ext.create('Proxmox.button.Button', {
+ text: gettext('Remove'),
+ disabled: true,
+ dangerous: true,
enableFn: (rec) => canEdit,
selModel: sm,
- baseurl: me.base_url + '/',
- callback: reload,
+ handler: function (btn, event, rec) {
+ Ext.create('PVE.window.FirewallObjectRemove', {
+ item: { id: rec.data.name },
+ url: me.base_url + '/' + rec.data.name,
+ text: Ext.String.format(
+ gettext("Are you sure you want to remove IPSet '{0}'"),
+ rec.data.name,
+ ),
+ referenceActions: [
+ {
+ text: gettext('Keep'),
+ value: 'keep',
+ description: gettext('Do not touch firewall rules referencing this IPSet.'),
+ },
+ {
+ text: gettext('Disable'),
+ value: 'disable',
+ description: gettext('Disable firewall rules referencing this IPSet.'),
+ },
+ {
+ text: gettext('Delete'),
+ value: 'drop',
+ description: gettext('Delete firewall rules referencing this IPSet.'),
+ },
+ ],
+ apiCallDone: reload,
+ }).show();
+ },
});
Ext.apply(me, {
--
2.47.3
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH pve-manager v6 21/28] ui: firewall: aliases: add controls for dangling references
2026-11-11 13:18 [PATCH container/docs/firewall/manager/network/qemu-server v6 00/28] handle dangling references when firewall objects go away Arthur Bied-Charreton
` (19 preceding siblings ...)
2026-11-11 13:18 ` [PATCH pve-manager v6 20/28] ui: firewall: ipset: add controls for " Arthur Bied-Charreton
@ 2026-11-11 13:18 ` Arthur Bied-Charreton
2026-11-11 13:18 ` [PATCH pve-manager v6 22/28] ui: sdn: apply: add control for dangling IPSet references Arthur Bied-Charreton
` (6 subsequent siblings)
27 siblings, 0 replies; 29+ messages in thread
From: Arthur Bied-Charreton @ 2026-11-11 13:18 UTC (permalink / raw)
To: pve-devel
Add the shared dangling-references action selector to the update dialog
and replace the plain remove button with a confirmation dialog carrying
the same selector to let users choose how to handle dangling references
on config changes.
'force-rename' is not offered. It is meant as an escape hatch to recover
from a partially applied rename, putting it next to the normal actions
makes it too easy to select.
StdRemoveButton cannot pass the extra 'dangling-references' parameter,
so the remove button is switched to a plain button that opens the
shared FirewallObjectRemove dialog.
Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
---
www/manager6/grid/FirewallAliases.js | 121 ++++++++++++++++++++++-----
1 file changed, 99 insertions(+), 22 deletions(-)
diff --git a/www/manager6/grid/FirewallAliases.js b/www/manager6/grid/FirewallAliases.js
index 06801d33..06dc3b9c 100644
--- a/www/manager6/grid/FirewallAliases.js
+++ b/www/manager6/grid/FirewallAliases.js
@@ -7,6 +7,8 @@ Ext.define('PVE.FirewallAliasEdit', {
width: 400,
+ onlineHelp: 'pve_firewall_dangling_references',
+
initComponent: function () {
let me = this;
@@ -20,27 +22,68 @@ Ext.define('PVE.FirewallAliasEdit', {
me.method = 'PUT';
}
+ let items = [
+ {
+ xtype: 'textfield',
+ name: me.isCreate ? 'name' : 'rename',
+ fieldLabel: gettext('Name'),
+ allowBlank: false,
+ },
+ {
+ xtype: 'textfield',
+ name: 'cidr',
+ fieldLabel: gettext('IP/CIDR'),
+ allowBlank: false,
+ },
+ {
+ xtype: 'textfield',
+ name: 'comment',
+ fieldLabel: gettext('Comment'),
+ },
+ ];
+ let advancedItems = [];
+ if (!me.isCreate) {
+ advancedItems.push({
+ xtype: 'pveFirewallReferenceAction',
+ preferredValue: 'rename',
+ referenceActions: [
+ {
+ text: gettext('Keep'),
+ value: 'keep',
+ description: gettext(
+ 'Do not touch firewall rules and IPSet members referencing this alias.',
+ ),
+ },
+ {
+ text: gettext('Disable'),
+ value: 'disable',
+ description: gettext(
+ 'Disable firewall rules and delete IPSet members referencing this alias.',
+ ),
+ },
+ {
+ text: gettext('Delete'),
+ value: 'drop',
+ description: gettext(
+ 'Delete firewall rules and IPSet members referencing this alias.',
+ ),
+ },
+ {
+ text: gettext('Rename'),
+ value: 'rename',
+ description: gettext(
+ 'Rewrite referencing firewall rules and IPSet members to point to the new alias name.',
+ ),
+ },
+ ],
+ });
+ }
+
let ipanel = Ext.create('Proxmox.panel.InputPanel', {
isCreate: me.isCreate,
- items: [
- {
- xtype: 'textfield',
- name: me.isCreate ? 'name' : 'rename',
- fieldLabel: gettext('Name'),
- allowBlank: false,
- },
- {
- xtype: 'textfield',
- name: 'cidr',
- fieldLabel: gettext('IP/CIDR'),
- allowBlank: false,
- },
- {
- xtype: 'textfield',
- name: 'comment',
- fieldLabel: gettext('Comment'),
- },
- ],
+ items,
+ // an empty array would still turn on the 'Advanced' checkbox
+ advancedItems: advancedItems.length ? advancedItems : undefined,
});
Ext.apply(me, {
@@ -158,15 +201,49 @@ Ext.define('PVE.FirewallAliases', {
},
});
- me.removeBtn = Ext.create('Proxmox.button.StdRemoveButton', {
+ me.removeBtn = Ext.create('Proxmox.button.Button', {
+ text: gettext('Remove'),
disabled: true,
+ dangerous: true,
selModel: sm,
enableFn: (rec) =>
!!caps.vms['VM.Config.Network'] ||
!!caps.dc['Sys.Modify'] ||
!!caps.nodes['Sys.Modify'],
- baseurl: me.base_url + '/',
- callback: reload,
+ handler: function (btn, event, rec) {
+ Ext.create('PVE.window.FirewallObjectRemove', {
+ item: { id: rec.data.name },
+ url: me.base_url + '/' + rec.data.name,
+ text: Ext.String.format(
+ gettext("Are you sure you want to remove alias '{0}'"),
+ rec.data.name,
+ ),
+ referenceActions: [
+ {
+ text: gettext('Keep'),
+ value: 'keep',
+ description: gettext(
+ 'Do not touch firewall rules and IPSet members referencing this alias.',
+ ),
+ },
+ {
+ text: gettext('Disable'),
+ value: 'disable',
+ description: gettext(
+ 'Disable firewall rules and delete IPSet members referencing this alias.',
+ ),
+ },
+ {
+ text: gettext('Delete'),
+ value: 'drop',
+ description: gettext(
+ 'Delete firewall rules and IPSet members referencing this alias.',
+ ),
+ },
+ ],
+ apiCallDone: reload,
+ }).show();
+ },
});
Ext.apply(me, {
--
2.47.3
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH pve-manager v6 22/28] ui: sdn: apply: add control for dangling IPSet references
2026-11-11 13:18 [PATCH container/docs/firewall/manager/network/qemu-server v6 00/28] handle dangling references when firewall objects go away Arthur Bied-Charreton
` (20 preceding siblings ...)
2026-11-11 13:18 ` [PATCH pve-manager v6 21/28] ui: firewall: aliases: " Arthur Bied-Charreton
@ 2026-11-11 13:18 ` Arthur Bied-Charreton
2026-11-11 13:18 ` [PATCH pve-manager v6 23/28] ui: guest destroy: use let for non-constant variable bindings Arthur Bied-Charreton
` (5 subsequent siblings)
27 siblings, 0 replies; 29+ messages in thread
From: Arthur Bied-Charreton @ 2026-11-11 13:18 UTC (permalink / raw)
To: pve-devel
Applying an SDN configuration that removes a VNet also removes the
four IPSets auto-generated for it [0], so rules referencing those no
longer resolve and are dropped from the generated ruleset.
Replace the plain confirmation message box with a window carrying the
shared selector, so those rules can be disabled or deleted in the same
step. The Apply button first queries the pending VNets, and the window
only shows the selector if the apply actually removes one, since there
is nothing to decide otherwise.
[0] https://pve.proxmox.com/wiki/Software-Defined_Network#pvesdn_firewall_integration
Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
---
www/manager6/sdn/StatusView.js | 119 +++++++++++++++++++++++++++------
1 file changed, 99 insertions(+), 20 deletions(-)
diff --git a/www/manager6/sdn/StatusView.js b/www/manager6/sdn/StatusView.js
index fada5041..2715df61 100644
--- a/www/manager6/sdn/StatusView.js
+++ b/www/manager6/sdn/StatusView.js
@@ -1,3 +1,88 @@
+// scanned by asciidoc-pve, so the anchor lands in the documentation index
+const VNET_IPSETS_HELP = {
+ onlineHelp: 'pve_firewall_automatically_generated_ipsets',
+};
+
+Ext.define('PVE.sdn.ApplyWindow', {
+ extend: 'Proxmox.window.Edit',
+ alias: 'widget.pveSdnApplyWindow',
+
+ title: gettext('Apply SDN Configuration'),
+
+ url: '/cluster/sdn',
+ method: 'PUT',
+ isCreate: true,
+ submitText: gettext('Apply'),
+ width: 600,
+
+ removedVnets: [],
+
+ initComponent: function () {
+ let me = this;
+
+ me.items = [
+ {
+ xtype: 'displayfield',
+ value: gettext(
+ 'Applying pending SDN changes will also apply any pending local node network changes.',
+ ),
+ },
+ ];
+
+ if (me.removedVnets.length !== 0) {
+ let removedHint = Ext.htmlEncode(
+ Ext.String.format(
+ gettext('This removes the following VNet(s): {0}.'),
+ me.removedVnets.join(', '),
+ ),
+ );
+ let helpLink = Ext.htmlEncode(Proxmox.Utils.get_help_link(VNET_IPSETS_HELP.onlineHelp));
+ let refsHint = Ext.String.format(
+ gettext(
+ 'Firewall rules referencing <a target="_blank" href="{0}">their auto-generated IPSets</a> will no longer resolve.',
+ ),
+ helpLink,
+ );
+
+ me.items.push(
+ {
+ xtype: 'displayfield',
+ userCls: 'pmx-hint',
+ value: `${removedHint}<br>${refsHint}`,
+ },
+ {
+ xtype: 'pveFirewallReferenceAction',
+ name: 'dangling-ipset-references',
+ preferredValue: 'drop',
+ fieldLabel: gettext('Referencing rules'),
+ labelWidth: 200,
+ referenceActions: [
+ {
+ text: gettext('Keep'),
+ value: 'keep',
+ description: gettext(
+ "Do not touch firewall rules referencing this VNet's IPSets.",
+ ),
+ },
+ {
+ text: gettext('Disable'),
+ value: 'disable',
+ description: gettext("Disable firewall rules referencing this VNet's IPSets."),
+ },
+ {
+ text: gettext('Delete'),
+ value: 'drop',
+ description: gettext("Delete firewall rules referencing this VNet's IPSets."),
+ },
+ ],
+ },
+ );
+ }
+
+ me.callParent();
+ },
+});
+
Ext.define(
'PVE.sdn.StatusView',
{
@@ -45,26 +130,20 @@ Ext.define(
{
text: gettext('Apply'),
handler: function () {
- Ext.Msg.show({
- title: gettext('Confirm'),
- icon: Ext.Msg.QUESTION,
- msg: gettext(
- 'Applying pending SDN changes will also apply any pending local node network changes. Proceed?',
- ),
- buttons: Ext.Msg.YESNO,
- callback: function (btn) {
- if (btn === 'yes') {
- Proxmox.Utils.API2Request({
- url: '/cluster/sdn/',
- method: 'PUT',
- waitMsgTarget: me,
- failure: (response) =>
- Ext.Msg.alert(
- gettext('Error'),
- response.htmlStatus,
- ),
- });
- }
+ Proxmox.Utils.API2Request({
+ url: '/cluster/sdn/vnets',
+ method: 'GET',
+ params: { pending: 1 },
+ waitMsgTarget: me,
+ failure: (response) =>
+ Ext.Msg.alert(gettext('Error'), response.htmlStatus),
+ success: function (response) {
+ Ext.create('PVE.sdn.ApplyWindow', {
+ removedVnets: response.result.data
+ .filter((vnet) => vnet.state === 'deleted')
+ .map((vnet) => vnet.vnet),
+ autoShow: true,
+ });
},
});
},
--
2.47.3
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH pve-manager v6 23/28] ui: guest destroy: use let for non-constant variable bindings
2026-11-11 13:18 [PATCH container/docs/firewall/manager/network/qemu-server v6 00/28] handle dangling references when firewall objects go away Arthur Bied-Charreton
` (21 preceding siblings ...)
2026-11-11 13:18 ` [PATCH pve-manager v6 22/28] ui: sdn: apply: add control for dangling IPSet references Arthur Bied-Charreton
@ 2026-11-11 13:18 ` Arthur Bied-Charreton
2026-11-11 13:18 ` [PATCH pve-manager v6 24/28] ui: guest destroy: add control for dangling IPSet references Arthur Bied-Charreton
` (4 subsequent siblings)
27 siblings, 0 replies; 29+ messages in thread
From: Arthur Bied-Charreton @ 2026-11-11 13:18 UTC (permalink / raw)
To: pve-devel
See JS Style Guide [0].
[0] https://pve.proxmox.com/wiki/Javascript_Style_Guide#Variables
Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
---
www/manager6/window/SafeDestroyGuest.js | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/www/manager6/window/SafeDestroyGuest.js b/www/manager6/window/SafeDestroyGuest.js
index ee0c649c..45b277e5 100644
--- a/www/manager6/window/SafeDestroyGuest.js
+++ b/www/manager6/window/SafeDestroyGuest.js
@@ -37,10 +37,10 @@ Ext.define('PVE.window.SafeDestroyGuest', {
getParams: function () {
let me = this;
- const purgeCheckbox = me.lookupReference('purgeCheckbox');
+ let purgeCheckbox = me.lookupReference('purgeCheckbox');
me.params.purge = purgeCheckbox.checked ? 1 : 0;
- const destroyUnreferencedCheckbox = me.lookupReference('destroyUnreferencedCheckbox');
+ let destroyUnreferencedCheckbox = me.lookupReference('destroyUnreferencedCheckbox');
me.params['destroy-unreferenced-disks'] = destroyUnreferencedCheckbox.checked ? 1 : 0;
return me.callParent();
--
2.47.3
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH pve-manager v6 24/28] ui: guest destroy: add control for dangling IPSet references
2026-11-11 13:18 [PATCH container/docs/firewall/manager/network/qemu-server v6 00/28] handle dangling references when firewall objects go away Arthur Bied-Charreton
` (22 preceding siblings ...)
2026-11-11 13:18 ` [PATCH pve-manager v6 23/28] ui: guest destroy: use let for non-constant variable bindings Arthur Bied-Charreton
@ 2026-11-11 13:18 ` Arthur Bied-Charreton
2026-11-11 13:18 ` [PATCH pve-manager v6 25/28] ui: firewall: show the task for reference updates Arthur Bied-Charreton
` (3 subsequent siblings)
27 siblings, 0 replies; 29+ messages in thread
From: Arthur Bied-Charreton @ 2026-11-11 13:18 UTC (permalink / raw)
To: pve-devel
Destroying a guest releases its IPAM entries, so the auto-generated
'guest-ipam-<vmid>' IPSet disappears as well. Rules referencing that
IPSet do not resolve anymore and are dropped from the generated ruleset
as a result.
Add the shared selector to the destroy dialog, so those rules can be
disabled or deleted along with the guest, and a hint linking to the
SDN firewall integration documentation.
The 'note' config only renders a single hint, so it is replaced by
two display fields among the additional items.
Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
---
www/manager6/window/SafeDestroyGuest.js | 66 ++++++++++++++++++++++++-
1 file changed, 65 insertions(+), 1 deletion(-)
diff --git a/www/manager6/window/SafeDestroyGuest.js b/www/manager6/window/SafeDestroyGuest.js
index 45b277e5..b75402a3 100644
--- a/www/manager6/window/SafeDestroyGuest.js
+++ b/www/manager6/window/SafeDestroyGuest.js
@@ -1,6 +1,11 @@
/*
* SafeDestroy window with additional checkboxes for removing guests
*/
+// scanned by asciidoc-pve, so the anchor lands in the documentation index
+const GUEST_IPAM_IPSET_HELP = {
+ onlineHelp: 'pve_firewall_automatically_generated_ipsets',
+};
+
Ext.define('PVE.window.SafeDestroyGuest', {
extend: 'Proxmox.window.SafeDestroy',
alias: 'widget.pveSafeDestroyGuest',
@@ -32,7 +37,63 @@ Ext.define('PVE.window.SafeDestroyGuest', {
},
],
- note: gettext('Referenced disks will always be destroyed.'),
+ initComponent: function () {
+ let me = this;
+
+ let helpLink = Ext.htmlEncode(
+ Proxmox.Utils.get_help_link(GUEST_IPAM_IPSET_HELP.onlineHelp),
+ );
+ let refsHint = Ext.String.format(
+ gettext(
+ 'Firewall rules referencing <a target="_blank" href="{0}">this guest\'s auto-generated IPSet</a> will no longer resolve.',
+ ),
+ helpLink,
+ );
+
+ me.additionalItems = [
+ // pushing would append on every open
+ ...me.additionalItems,
+ {
+ xtype: 'displayfield',
+ userCls: 'pmx-hint',
+ value: gettext('Referenced disks will always be destroyed.'),
+ },
+ {
+ xtype: 'pveFirewallReferenceAction',
+ reference: 'firewallDanglingReferencesButton',
+ name: 'dangling-ipset-references',
+ preferredValue: 'drop',
+ fieldLabel: gettext('Referencing rules'),
+ labelWidth: 200,
+ referenceActions: [
+ {
+ text: gettext('Keep'),
+ value: 'keep',
+ description: gettext(
+ "Do not touch firewall rules referencing this guest's IPAM IPSet.",
+ ),
+ },
+ {
+ text: gettext('Disable'),
+ value: 'disable',
+ description: gettext("Disable firewall rules referencing this guest's IPAM IPSet."),
+ },
+ {
+ text: gettext('Delete'),
+ value: 'drop',
+ description: gettext("Delete firewall rules referencing this guest's IPAM IPSet."),
+ },
+ ],
+ },
+ {
+ xtype: 'displayfield',
+ userCls: 'pmx-hint',
+ value: refsHint,
+ },
+ ];
+
+ me.callParent();
+ },
getParams: function () {
let me = this;
@@ -43,6 +104,9 @@ Ext.define('PVE.window.SafeDestroyGuest', {
let destroyUnreferencedCheckbox = me.lookupReference('destroyUnreferencedCheckbox');
me.params['destroy-unreferenced-disks'] = destroyUnreferencedCheckbox.checked ? 1 : 0;
+ let danglingRefs = me.lookupReference('firewallDanglingReferencesButton');
+ me.params['dangling-ipset-references'] = danglingRefs.getValue();
+
return me.callParent();
},
});
--
2.47.3
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH pve-manager v6 25/28] ui: firewall: show the task for reference updates
2026-11-11 13:18 [PATCH container/docs/firewall/manager/network/qemu-server v6 00/28] handle dangling references when firewall objects go away Arthur Bied-Charreton
` (23 preceding siblings ...)
2026-11-11 13:18 ` [PATCH pve-manager v6 24/28] ui: guest destroy: add control for dangling IPSet references Arthur Bied-Charreton
@ 2026-11-11 13:18 ` Arthur Bied-Charreton
2026-11-11 13:18 ` [PATCH pve-manager v6 26/28] ui: firewall: groups: add controls for dangling references Arthur Bied-Charreton
` (2 subsequent siblings)
27 siblings, 0 replies; 29+ messages in thread
From: Arthur Bied-Charreton @ 2026-11-11 13:18 UTC (permalink / raw)
To: pve-devel
Renaming or deleting an ipset or alias returns a UPID if the
'dangling-references' parameter is passed. Open the task progress
window and reload the grid once the task has finished.
Both dialogs only open that window if a UPID is returned, so the
requests that are still answered synchronously (a plain create or an
edit that only changes a comment) are not affected.
Also add display names for the four task types.
Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
---
www/manager6/Utils.js | 4 ++++
www/manager6/grid/FirewallAliases.js | 4 ++++
www/manager6/grid/FirewallObjectCommon.js | 2 ++
www/manager6/panel/IPSet.js | 4 ++++
4 files changed, 14 insertions(+)
diff --git a/www/manager6/Utils.js b/www/manager6/Utils.js
index 61b8def2..168e7352 100644
--- a/www/manager6/Utils.js
+++ b/www/manager6/Utils.js
@@ -2219,6 +2219,10 @@ Ext.define('PVE.Utils', {
dircreate: [gettext('Directory Storage'), gettext('Create')],
dirremove: [gettext('Directory'), gettext('Remove')],
download: [gettext('File'), gettext('Download')],
+ 'fw-alias-delete': [gettext('Firewall Alias'), gettext('Delete')],
+ 'fw-alias-rename': [gettext('Firewall Alias'), gettext('Rename')],
+ 'fw-ipset-delete': [gettext('Firewall IPSet'), gettext('Delete')],
+ 'fw-ipset-rename': [gettext('Firewall IPSet'), gettext('Rename')],
hamigrate: ['HA', gettext('Migrate')],
hashutdown: ['HA', gettext('Shutdown')],
hastart: ['HA', gettext('Start')],
diff --git a/www/manager6/grid/FirewallAliases.js b/www/manager6/grid/FirewallAliases.js
index 06dc3b9c..de8a92eb 100644
--- a/www/manager6/grid/FirewallAliases.js
+++ b/www/manager6/grid/FirewallAliases.js
@@ -7,6 +7,9 @@ Ext.define('PVE.FirewallAliasEdit', {
width: 400,
+ // a rename that updates references comes back as a task
+ showProgress: true,
+
onlineHelp: 'pve_firewall_dangling_references',
initComponent: function () {
@@ -242,6 +245,7 @@ Ext.define('PVE.FirewallAliases', {
},
],
apiCallDone: reload,
+ taskDone: reload,
}).show();
},
});
diff --git a/www/manager6/grid/FirewallObjectCommon.js b/www/manager6/grid/FirewallObjectCommon.js
index 04dddf48..0b912650 100644
--- a/www/manager6/grid/FirewallObjectCommon.js
+++ b/www/manager6/grid/FirewallObjectCommon.js
@@ -4,6 +4,8 @@ Ext.define('PVE.window.FirewallObjectRemove', {
width: 500,
+ showProgress: true,
+
// [{ value, text, description }, ...]
referenceActions: undefined,
diff --git a/www/manager6/panel/IPSet.js b/www/manager6/panel/IPSet.js
index 40c1ef4b..b2438ce1 100644
--- a/www/manager6/panel/IPSet.js
+++ b/www/manager6/panel/IPSet.js
@@ -125,7 +125,10 @@ Ext.define('PVE.IPSetList', {
url: me.base_url,
method: 'POST',
digest: rec.data.digest,
+ // a rename that updates references comes back as a task
+ showProgress: true,
onlineHelp: 'pve_firewall_dangling_references',
+
items: [ipanel],
});
win.show();
@@ -201,6 +204,7 @@ Ext.define('PVE.IPSetList', {
},
],
apiCallDone: reload,
+ taskDone: reload,
}).show();
},
});
--
2.47.3
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH pve-manager v6 26/28] ui: firewall: groups: add controls for dangling references
2026-11-11 13:18 [PATCH container/docs/firewall/manager/network/qemu-server v6 00/28] handle dangling references when firewall objects go away Arthur Bied-Charreton
` (24 preceding siblings ...)
2026-11-11 13:18 ` [PATCH pve-manager v6 25/28] ui: firewall: show the task for reference updates Arthur Bied-Charreton
@ 2026-11-11 13:18 ` Arthur Bied-Charreton
2026-11-11 13:19 ` [PATCH pve-docs v6 27/28] firewall: remove trailing spaces Arthur Bied-Charreton
2026-11-11 13:19 ` [PATCH pve-docs v6 28/28] firewall: document renaming and removing objects Arthur Bied-Charreton
27 siblings, 0 replies; 29+ messages in thread
From: Arthur Bied-Charreton @ 2026-11-11 13:18 UTC (permalink / raw)
To: pve-devel
Add the reference action selector the ipset and alias dialogs have for
security groups as well.
The edit dialog has it in its advanced section, preselected to 'rename'.
The remove button becomes a FirewallObjectRemove, preselected to
'drop'. Both dialogs wait on the task the API returns.
Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
---
www/manager6/Utils.js | 2 +
www/manager6/dc/SecurityGroups.js | 81 +++++++++++++++++++++++++++++--
2 files changed, 79 insertions(+), 4 deletions(-)
diff --git a/www/manager6/Utils.js b/www/manager6/Utils.js
index 168e7352..67617ef0 100644
--- a/www/manager6/Utils.js
+++ b/www/manager6/Utils.js
@@ -2221,6 +2221,8 @@ Ext.define('PVE.Utils', {
download: [gettext('File'), gettext('Download')],
'fw-alias-delete': [gettext('Firewall Alias'), gettext('Delete')],
'fw-alias-rename': [gettext('Firewall Alias'), gettext('Rename')],
+ 'fw-group-delete': [gettext('Firewall Security Group'), gettext('Delete')],
+ 'fw-group-rename': [gettext('Firewall Security Group'), gettext('Rename')],
'fw-ipset-delete': [gettext('Firewall IPSet'), gettext('Delete')],
'fw-ipset-rename': [gettext('Firewall IPSet'), gettext('Rename')],
hamigrate: ['HA', gettext('Migrate')],
diff --git a/www/manager6/dc/SecurityGroups.js b/www/manager6/dc/SecurityGroups.js
index a1454003..43a154ff 100644
--- a/www/manager6/dc/SecurityGroups.js
+++ b/www/manager6/dc/SecurityGroups.js
@@ -12,6 +12,10 @@ Ext.define('PVE.SecurityGroupEdit', {
allow_iface: false,
+ showProgress: true,
+
+ onlineHelp: 'pve_firewall_dangling_references',
+
initComponent: function () {
var me = this;
@@ -49,10 +53,43 @@ Ext.define('PVE.SecurityGroupEdit', {
});
}
+ let advancedItems = [];
+ if (!me.isCreate) {
+ advancedItems.push({
+ xtype: 'pveFirewallReferenceAction',
+ preferredValue: 'rename',
+ referenceActions: [
+ {
+ text: gettext('Keep'),
+ value: 'keep',
+ description: gettext('Do not touch firewall rules referencing this security group.'),
+ },
+ {
+ text: gettext('Disable'),
+ value: 'disable',
+ description: gettext('Disable firewall rules referencing this security group.'),
+ },
+ {
+ text: gettext('Delete'),
+ value: 'drop',
+ description: gettext('Delete firewall rules referencing this security group.'),
+ },
+ {
+ text: gettext('Rename'),
+ value: 'rename',
+ description: gettext(
+ 'Rewrite referencing firewall rules to point to the new security group name.',
+ ),
+ },
+ ],
+ });
+ }
+
var ipanel = Ext.create('Proxmox.panel.InputPanel', {
// InputPanel does not have a 'create' property, does it need a 'isCreate'
isCreate: me.isCreate,
items: items,
+ advancedItems: advancedItems.length ? advancedItems : undefined,
});
Ext.apply(me, {
@@ -148,11 +185,47 @@ Ext.define('PVE.SecurityGroupList', {
},
});
- me.removeBtn = Ext.create('Proxmox.button.StdRemoveButton', {
+ me.removeBtn = new Proxmox.button.Button({
+ text: gettext('Remove'),
+ disabled: true,
+ dangerous: true,
selModel: sm,
- baseurl: me.base_url + '/',
- enableFn: (rec) => canEdit && rec && me.base_url,
- callback: () => reload(),
+ enableFn: (rec) => canEdit && !!rec,
+ handler: function (btn, event, rec) {
+ Ext.create('PVE.window.FirewallObjectRemove', {
+ item: { id: rec.data.group },
+ url: me.base_url + '/' + rec.data.group,
+ text: Ext.String.format(
+ gettext("Are you sure you want to remove security group '{0}'"),
+ rec.data.group,
+ ),
+ referenceActions: [
+ {
+ text: gettext('Keep'),
+ value: 'keep',
+ description: gettext(
+ 'Do not touch firewall rules referencing this security group.',
+ ),
+ },
+ {
+ text: gettext('Disable'),
+ value: 'disable',
+ description: gettext(
+ 'Disable firewall rules referencing this security group.',
+ ),
+ },
+ {
+ text: gettext('Delete'),
+ value: 'drop',
+ description: gettext(
+ 'Delete firewall rules referencing this security group.',
+ ),
+ },
+ ],
+ apiCallDone: reload,
+ taskDone: reload,
+ }).show();
+ },
});
Ext.apply(me, {
--
2.47.3
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH pve-docs v6 27/28] firewall: remove trailing spaces
2026-11-11 13:18 [PATCH container/docs/firewall/manager/network/qemu-server v6 00/28] handle dangling references when firewall objects go away Arthur Bied-Charreton
` (25 preceding siblings ...)
2026-11-11 13:18 ` [PATCH pve-manager v6 26/28] ui: firewall: groups: add controls for dangling references Arthur Bied-Charreton
@ 2026-11-11 13:19 ` Arthur Bied-Charreton
2026-11-11 13:19 ` [PATCH pve-docs v6 28/28] firewall: document renaming and removing objects Arthur Bied-Charreton
27 siblings, 0 replies; 29+ messages in thread
From: Arthur Bied-Charreton @ 2026-11-11 13:19 UTC (permalink / raw)
To: pve-devel
Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
---
pve-firewall.adoc | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/pve-firewall.adoc b/pve-firewall.adoc
index 21608c9..cf04781 100644
--- a/pve-firewall.adoc
+++ b/pve-firewall.adoc
@@ -132,7 +132,7 @@ Cluster Wide Setup
~~~~~~~~~~~~~~~~~~
The cluster-wide firewall configuration is stored at:
-
+
/etc/pve/firewall/cluster.fw
The configuration can contain the following sections:
@@ -309,7 +309,7 @@ DIRECTION ACTION [OPTIONS]
DIRECTION MACRO(ACTION) [OPTIONS] # use predefined macro
----
-The following options can be used to refine rule matches.
+The following options can be used to refine rule matches.
include::generated/pve-firewall-rules-opts.adoc[]
@@ -423,7 +423,7 @@ communication. (multicast,ssh,...)
----
# /etc/pve/firewall/cluster.fw
-[IPSET management]
+[IPSET management]
192.168.2.10
192.168.2.10/24
----
@@ -437,7 +437,7 @@ Traffic from these IPs is dropped by every host's and VM's firewall.
----
# /etc/pve/firewall/cluster.fw
-[IPSET blacklist]
+[IPSET blacklist]
77.240.159.182
213.87.123.0/24
----
@@ -644,7 +644,7 @@ How to allow FTP
FTP is an old style protocol which uses port 21 and several other dynamic ports. So you
need a rule to accept port 21. In addition, you need to load the `ip_conntrack_ftp` module.
-So please run:
+So please run:
modprobe ip_conntrack_ftp
@@ -666,7 +666,7 @@ Install suricata on proxmox host:
----
# apt-get install suricata
-# modprobe nfnetlink_queue
+# modprobe nfnetlink_queue
----
Don't forget to add `nfnetlink_queue` to `/etc/modules` for next reboot.
--
2.47.3
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH pve-docs v6 28/28] firewall: document renaming and removing objects
2026-11-11 13:18 [PATCH container/docs/firewall/manager/network/qemu-server v6 00/28] handle dangling references when firewall objects go away Arthur Bied-Charreton
` (26 preceding siblings ...)
2026-11-11 13:19 ` [PATCH pve-docs v6 27/28] firewall: remove trailing spaces Arthur Bied-Charreton
@ 2026-11-11 13:19 ` Arthur Bied-Charreton
27 siblings, 0 replies; 29+ messages in thread
From: Arthur Bied-Charreton @ 2026-11-11 13:19 UTC (permalink / raw)
To: pve-devel
Renaming or removing a security group, IP alias or IP set leaves every
reference to it dangling, and those references may also be found in
other configs than the one the object is defined in.
Describe the 'dangling-references' option and its different values.
Also point the SDN warning about rules referencing a released IPAM
IPSet at the new section.
Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
---
pve-firewall.adoc | 66 +++++++++++++++++++++++++++++++++++++++++++++++
pvesdn.adoc | 9 ++++---
2 files changed, 72 insertions(+), 3 deletions(-)
diff --git a/pve-firewall.adoc b/pve-firewall.adoc
index cf04781..7d49d04 100644
--- a/pve-firewall.adoc
+++ b/pve-firewall.adoc
@@ -468,6 +468,72 @@ discovery protocol to work.
----
+[[pve_firewall_dangling_references]]
+Renaming and Removing Firewall Objects
+--------------------------------------
+
+Firewall objects (aliases, IPSets and security groups) are referenced by name
+from the rest of the firewall configuration:
+
+* a security group is referenced by the action of a `GROUP` rule
+* an IPSet or alias is referenced by the source and destination properties of a
+ rule, and an alias additionally by the members of an IPSet
+
+A reference to a firewall object that does not exist because it was renamed or
+removed cannot be resolved. A rule referencing a renamed or removed object
+leads to errors during ruleset generation and should be avoided.
+
+The rename and delete operations take a `dangling-references` option that
+defines what to do with the references a change would break. The GUI exposes
+this option in the advanced section of the dialogs that rename or remove the
+object. Renaming an object preselects `rename` and removing one preselects
+`drop`. The different values are as follows:
+
+`keep`::
+ Leave references as they are.
+`disable`::
+ Disable all rules referencing the object. This keeps the references around
+ in the firewall configuration so they can be reviewed and re-enabled later.
+ An IPSet member has no disabled state, so members referencing a removed or
+ renamed alias are removed instead.
+`drop`::
+ Remove all rules and IPSet members referencing the object from the firewall
+ configuration.
+`rename`::
+ Rewrite all references to the object so they point to the new name. This is
+ only available when renaming.
+
+Changes made to the firewall configuration are recorded in the task log.
+
+Recovering From an Interrupted Rename
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+A rename of a cluster-level object needs to update references in all downstream
+configurations, i.e. the ones for hosts, guests and VNets, so it cannot be
+atomic. If the rename is interrupted in between, both the old and the new name
+are in the cluster config, and retrying it fails because an object with the
+target name already exists.
+
+Setting `force-rename` skips that check so a retry can finish the rename; the
+`dangling-references` action is applied as usual. The option is not exposed in
+the UI and can be used with `pvesh` as follows:
+
+----
+pvesh create /cluster/firewall/ipset --name foo --rename bar --dangling-references rename --force-rename 1
+----
+
+[[pve_firewall_automatically_generated_ipsets]]
+Automatically Generated IPSets
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+The same applies to the IPSets the SDN stack generates, which disappear when
+applying an SDN configuration that removes a VNet, and destroying a guest with
+IPAM entries. See xref:pvesdn_firewall_integration[Firewall Integration] for
+what those IP sets contain.
+
+Both operations take a `dangling-ipset-references` option, which accepts
+`keep`, `disable` and `drop` with the same meaning as above.
+
[[pve_firewall_services_commands]]
Services and Commands
---------------------
diff --git a/pvesdn.adoc b/pvesdn.adoc
index b701636..85cc344 100644
--- a/pvesdn.adoc
+++ b/pvesdn.adoc
@@ -1527,9 +1527,12 @@ then the IPset would contain the IPs from *all* VNets.
When entries get added / updated / deleted, then the respective IPSets will be
updated accordingly.
-WARNING: When removing all entries for a guest and there are firewall rules
-still referencing the auto-generated IPSet then the firewall will fail to update
-the ruleset, since it references a non-existing IPSet.
+WARNING: When removing all entries for a guest or a VNet and there are firewall
+rules still referencing the auto-generated IPSet then the firewall will fail to
+update the ruleset, since it references a non-existing IPSet. Destroying a guest
+and applying an SDN configuration that removes a VNet both offer to disable or
+remove the rules that would be left behind, see
+xref:pve_firewall_dangling_references[Renaming and Removing Firewall Objects].
[[pvesdn_setup_examples]]
Examples
--
2.47.3
^ permalink raw reply related [flat|nested] 29+ messages in thread