* [PATCH container/docs/firewall/manager/network/qemu-server v4 00/27] handle dangling references when firewall objects go away
@ 2026-10-22 12:46 Arthur Bied-Charreton
2026-10-22 12:46 ` [PATCH pve-firewall v4 01/27] firewall: move object reference helpers to their own module Arthur Bied-Charreton
` (26 more replies)
0 siblings, 27 replies; 28+ messages in thread
From: Arthur Bied-Charreton @ 2026-10-22 12:46 UTC (permalink / raw)
To: pve-devel
Renaming or deleting a firewall object (an IPSet, alias or security
group) that rules still reference leaves those references dangling.
The firewall fails to parse the affected rules and drops them from the
generated ruleset. For security groups, the reference reaches the
backend, which creates empty chains on iptables, and causes the ruleset
to fail to load on nftables.
This is especially bad in the rename case, where one might reasonably
expect the references to follow the new object name and walk away
thinking the firewall rules are active.
This series makes the affected operations offer to deal with the
references instead of leaving them behind.
pve-firewall gains a shared helper, update_refs(), whcih finds
references in rules, security groups and IPset members and applies one
of three actions: 'rename' rewrites the reference to point at the new
name, 'disable' disables the referencing rules, and 'drop' removes them.
An IPSet member has no disabled state, so 'disable' removes members as
well.
These options are added to the API as follows:
POST .../firewall/ipset dangling-references keep|disable|drop|rename|force-rename
PUT .../firewall/aliases/{name} dangling-references keep|disable|drop|rename|force-rename
POST /cluster/firewall/groups dangling-references keep|disable|drop|rename|force-rename
DELETE .../ipset/{name} dangling-references keep|disable|drop
DELETE .../aliases/{name} dangling-references keep|disable|drop
DELETE /cluster/firewall/groups/{name} dangling-references keep|disable|drop
PUT /cluster/sdn dangling-ipset-references keep|disable|drop
DELETE /nodes/{node}/qemu/{vmid} dangling-ipset-references keep|disable|drop
DELETE /nodes/{node}/lxc/{vmid} dangling-ipset-references keep|disable|drop
For a cluster object, the helper also walks every downstream
config in the cluster (guest, host and vnet), locking and saving each
one. On a rename, the new object is persisted before its references are
rewritten, so a concurrent compilation does not observe a reference to
an object that does not exist yet. If a cluster-wide rewrite is
interrupted part way, passing 'force-rename' resumes it (without this,
the next attempt would fail due to the target name already existing in
the config).
While these options allow to trigger changes to the firewall
configurations from other endpoints requiring different permissions,
like guest destroy and SDN apply, they do not require any additional
permissions, see full explanation here [0].
The API keeps backwards-compatible defaults: 'keep' everywhere, with the
exception of POST /cluster/firewall/groups, which was already renaming
references by default and keeps doing so. The UI however defaults
towards renaming/dropping dangling references, as this is expected to
be the normal case.
Pre-built packages on sani under:
abied-charreton/firewall-object-references/v4/
Build dependencies:
pve-container, qemu-server <- pve-firewall
pve-manager <- pve-docs
Runtime dependencies:
pve-network, pve-manager <- pve-firewall
Changes since [v3]
- Rename the option from 'update-references' (no|yes|force) to
'dangling-references' (keep|disable|drop|rename|force-rename) so the
parameter name stays the same everywhere.
- Add 'drop' and 'disable' actions to the rename paths
- Handle security groups as well (the rename path was missing vnet
configs and the delete path did nothing, fixes #8176 [1]).
- Move all helpers related to firewall object references out of
PVE::Firewall::Helpers into PVE::Firewall::References.
- update_refs() takes a callback for saving the cluster config so it
can decide when the object has to be persisted instead of relying on
the caller for correctness.
- Run every rename and delete as a task and log every change made to the
configs to the task log for better observability.
- Drop 'force-rename' from the GUI and document it as an escape hatch
for partially applied renames
- Trim the API option descriptions and move the reasoning into a
pve-docs patch.
- Merge the two ExtJS widgets into one ComboGrid taking the list of
actions from the caller and showing each descriptoin directly in
a column instead of using a tooltip.
- Preselect 'rename' and 'drop' in the UI for edits and removals,
respectively. API defaults keep doing the same as they were before.
- Move the pve-firewall tests to src/test
- Move selector to advanced section in ipset/alias edit/remove dialogs
[v3] https://lore.proxmox.com/pve-devel/20260925094230.844917-1-a.bied-charreton@proxmox.com/
[v2] https://lore.proxmox.com/pve-devel/20260818133413.450776-1-a.bied-charreton@proxmox.com/
[v1] https://lore.proxmox.com/pve-devel/20260407073658.90818-1-a.bied-charreton@proxmox.com/
[0] https://lore.proxmox.com/pve-devel/awaoshjzbr7adisjngsrcts4zs3hxgoxw2lgcoq66gtiap3al2@mpxrbmtyfcwj/
[1] https://bugzilla.proxmox.com/show_bug.cgi?id=8176
pve-firewall:
Arthur Bied-Charreton (14):
firewall: move object reference helpers to their own module
firewall: references: add helpers to update object references
firewall: references: log the references an object update touched
firewall: references: add wrappers for the SDN-generated IPSets
firewall: parser: do not log errors for disabled rules
firewall: ipset: add option to handle dangling references on rename
firewall: ipset: add option to handle dangling references on delete
firewall: aliases: add option to handle dangling references on rename
firewall: aliases: add option to handle dangling references on delete
firewall: api: run reference updates as tasks
firewall: tests: add tests for object reference updates
firewall: references: handle security groups
firewall: groups: add option to handle dangling references on rename
fix #8176: firewall: groups: handle dangling references on delete
src/PVE/API2/Firewall/Aliases.pm | 218 +++++++++---
src/PVE/API2/Firewall/Cluster.pm | 6 +-
src/PVE/API2/Firewall/Groups.pm | 199 ++++++-----
src/PVE/API2/Firewall/IPSet.pm | 215 +++++++++---
src/PVE/API2/Firewall/Rules.pm | 53 ++-
src/PVE/API2/Firewall/VM.pm | 7 +-
src/PVE/Firewall.pm | 2 +-
src/PVE/Firewall/Helpers.pm | 38 ---
src/PVE/Firewall/Makefile | 2 +-
src/PVE/Firewall/References.pm | 570 +++++++++++++++++++++++++++++++
src/test/Makefile | 2 +-
src/test/test_update_refs.pl | 413 ++++++++++++++++++++++
12 files changed, 1473 insertions(+), 252 deletions(-)
create mode 100644 src/PVE/Firewall/References.pm
create mode 100755 src/test/test_update_refs.pl
pve-network:
Arthur Bied-Charreton (1):
apply: add option to handle dangling references on VNet deletion
src/PVE/API2/Network/SDN.pm | 38 +++++++++++++++++++++++++++++++++++++
1 file changed, 38 insertions(+)
qemu-server:
Arthur Bied-Charreton (1):
api: destroy_vm: add option to handle dangling IPSet references
src/PVE/API2/Qemu.pm | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
pve-container:
Arthur Bied-Charreton (1):
api: destroy_vm: add option to handle dangling IPSet references
src/PVE/API2/LXC.pm | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
pve-docs:
Arthur Bied-Charreton (2):
firewall: remove trailing spaces
firewall: document renaming and removing objects
pve-firewall.adoc | 76 +++++++++++++++++++++++++++++++++++++++++++----
pvesdn.adoc | 9 ++++--
2 files changed, 76 insertions(+), 9 deletions(-)
pve-manager:
Arthur Bied-Charreton (8):
ui: firewall: add a common widget for handling dangling references
ui: firewall: ipset: add controls for dangling references
ui: firewall: aliases: add controls for dangling references
ui: sdn: apply: add control for dangling IPSet references
ui: guest destroy: use let for non-constant variable bindings
ui: guest destroy: add control for dangling IPSet references
ui: firewall: show the task for reference updates
ui: firewall: groups: add controls for dangling references
www/manager6/Makefile | 1 +
www/manager6/Utils.js | 6 +
www/manager6/dc/SecurityGroups.js | 81 ++++++++++++-
www/manager6/grid/FirewallAliases.js | 125 ++++++++++++++++----
www/manager6/grid/FirewallObjectCommon.js | 136 ++++++++++++++++++++++
www/manager6/panel/IPSet.js | 84 +++++++++++--
www/manager6/sdn/StatusView.js | 118 +++++++++++++++----
www/manager6/window/SafeDestroyGuest.js | 69 ++++++++++-
8 files changed, 563 insertions(+), 57 deletions(-)
create mode 100644 www/manager6/grid/FirewallObjectCommon.js
Summary over all repositories:
25 files changed, 2188 insertions(+), 318 deletions(-)
--
Generated by murpp 0.12.1
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH pve-firewall v4 01/27] firewall: move object reference helpers to their own module
2026-10-22 12:46 [PATCH container/docs/firewall/manager/network/qemu-server v4 00/27] handle dangling references when firewall objects go away Arthur Bied-Charreton
@ 2026-10-22 12:46 ` Arthur Bied-Charreton
2026-10-22 12:46 ` [PATCH pve-firewall v4 02/27] firewall: references: add helpers to update object references Arthur Bied-Charreton
` (25 subsequent siblings)
26 siblings, 0 replies; 28+ messages in thread
From: Arthur Bied-Charreton @ 2026-10-22 12:46 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] 28+ messages in thread
* [PATCH pve-firewall v4 02/27] firewall: references: add helpers to update object references
2026-10-22 12:46 [PATCH container/docs/firewall/manager/network/qemu-server v4 00/27] handle dangling references when firewall objects go away Arthur Bied-Charreton
2026-10-22 12:46 ` [PATCH pve-firewall v4 01/27] firewall: move object reference helpers to their own module Arthur Bied-Charreton
@ 2026-10-22 12:46 ` Arthur Bied-Charreton
2026-10-22 12:46 ` [PATCH pve-firewall v4 03/27] firewall: references: log the references an object update touched Arthur Bied-Charreton
` (24 subsequent siblings)
26 siblings, 0 replies; 28+ messages in thread
From: Arthur Bied-Charreton @ 2026-10-22 12:46 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.
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/References.pm | 332 +++++++++++++++++++++++++++++++++
1 file changed, 332 insertions(+)
diff --git a/src/PVE/Firewall/References.pm b/src/PVE/Firewall/References.pm
index a4ea967..8998380 100644
--- a/src/PVE/Firewall/References.pm
+++ b/src/PVE/Firewall/References.pm
@@ -3,9 +3,14 @@ package PVE::Firewall::References;
use strict;
use warnings;
+use PVE::Cluster;
+use PVE::Firewall;
+use PVE::Network::SDN::Vnets;
+
use base 'Exporter';
our @EXPORT_OK = qw(
collect_refs
+ update_refs
);
sub collect_refs {
@@ -45,4 +50,331 @@ 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);
+ }
+ },
+ );
+ }
+
+ my $vnets = (PVE::Network::SDN::Vnets::config(1) // {})->{ids} // {};
+ for my $vnet (keys $vnets->%*) {
+ 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> declare where the items come from and end up in those records.
+
+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 = {};
+
+ 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 name the references carry, or an arrayref of names to disable or drop
+ new # the name to rewrite them 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 is: 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] 28+ messages in thread
* [PATCH pve-firewall v4 03/27] firewall: references: log the references an object update touched
2026-10-22 12:46 [PATCH container/docs/firewall/manager/network/qemu-server v4 00/27] handle dangling references when firewall objects go away Arthur Bied-Charreton
2026-10-22 12:46 ` [PATCH pve-firewall v4 01/27] firewall: move object reference helpers to their own module Arthur Bied-Charreton
2026-10-22 12:46 ` [PATCH pve-firewall v4 02/27] firewall: references: add helpers to update object references Arthur Bied-Charreton
@ 2026-10-22 12:46 ` Arthur Bied-Charreton
2026-10-22 12:46 ` [PATCH pve-firewall v4 04/27] firewall: references: add wrappers for the SDN-generated IPSets Arthur Bied-Charreton
` (23 subsequent siblings)
26 siblings, 0 replies; 28+ messages in thread
From: Arthur Bied-Charreton @ 2026-10-22 12:46 UTC (permalink / raw)
To: pve-devel
Make update_refs() log each reference as it is rewritten, so a run that
dies halfway through the cluster still leaves an account of what it got
to:
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 tally only counts what landed, so it is logged whether or not the
walk ran to the end. Errors are raised unchanged once that is written,
as what they mean for the caller's API is the caller's to phrase.
The lines go to stdout in a worker, and to the syslog otherwise, since
PVE::Daemon sends a daemon's stdout to /dev/null. Callers that have to
rewrite before their task exists can pass a log sink and replay the
lines in the worker.
Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
---
src/PVE/Firewall/References.pm | 93 +++++++++++++++++++++++++++++++---
1 file changed, 87 insertions(+), 6 deletions(-)
diff --git a/src/PVE/Firewall/References.pm b/src/PVE/Firewall/References.pm
index 8998380..fb25bc2 100644
--- a/src/PVE/Firewall/References.pm
+++ b/src/PVE/Firewall/References.pm
@@ -6,6 +6,8 @@ use warnings;
use PVE::Cluster;
use PVE::Firewall;
use PVE::Network::SDN::Vnets;
+use PVE::RESTEnvironment;
+use PVE::SafeSyslog qw(syslog);
use base 'Exporter';
our @EXPORT_OK = qw(
@@ -71,7 +73,11 @@ sub rewrite_refs_in_env {
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->@*;
+ for my $change ($conf_changes->@*) {
+ my $scoped = { $change->%*, scope => $scope };
+ push $changes->@*, $scoped;
+ $op->{on_change}->($scoped) if $op->{on_change};
+ }
return scalar($conf_changes->@*);
};
@@ -317,9 +323,16 @@ 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 as it is rewritten.
References are matched in rules, security groups and IPSet members. Matching is case-insensitive
and renames are written back lowercased.
@@ -333,6 +346,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 # what the log should call the operation
+ log # where the logs go, task log if in worker or syslog by default
+
+The caller is responsible for locking and saving C<conf> itself.
IPSet members have no disabled state, so C<disable> removes them as well.
@@ -342,7 +359,8 @@ C<conf> itself have been rewritten and before any downstream config is: downstre
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.
@@ -351,9 +369,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);
@@ -365,6 +389,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,
@@ -372,9 +400,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] 28+ messages in thread
* [PATCH pve-firewall v4 04/27] firewall: references: add wrappers for the SDN-generated IPSets
2026-10-22 12:46 [PATCH container/docs/firewall/manager/network/qemu-server v4 00/27] handle dangling references when firewall objects go away Arthur Bied-Charreton
` (2 preceding siblings ...)
2026-10-22 12:46 ` [PATCH pve-firewall v4 03/27] firewall: references: log the references an object update touched Arthur Bied-Charreton
@ 2026-10-22 12:46 ` Arthur Bied-Charreton
2026-10-22 12:46 ` [PATCH pve-firewall v4 05/27] firewall: parser: do not log errors for disabled rules Arthur Bied-Charreton
` (22 subsequent siblings)
26 siblings, 0 replies; 28+ messages in thread
From: Arthur Bied-Charreton @ 2026-10-22 12:46 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 fb25bc2..37df434 100644
--- a/src/PVE/Firewall/References.pm
+++ b/src/PVE/Firewall/References.pm
@@ -458,4 +458,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> names what made them go away for the log, and C<$log> is where that account goes.
+
+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] 28+ messages in thread
* [PATCH pve-firewall v4 05/27] firewall: parser: do not log errors for disabled rules
2026-10-22 12:46 [PATCH container/docs/firewall/manager/network/qemu-server v4 00/27] handle dangling references when firewall objects go away Arthur Bied-Charreton
` (3 preceding siblings ...)
2026-10-22 12:46 ` [PATCH pve-firewall v4 04/27] firewall: references: add wrappers for the SDN-generated IPSets Arthur Bied-Charreton
@ 2026-10-22 12:46 ` Arthur Bied-Charreton
2026-10-22 12:46 ` [PATCH pve-firewall v4 06/27] firewall: ipset: add option to handle dangling references on rename Arthur Bied-Charreton
` (21 subsequent siblings)
26 siblings, 0 replies; 28+ messages in thread
From: Arthur Bied-Charreton @ 2026-10-22 12:46 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 da6976e..a01738c 100644
--- a/src/PVE/Firewall.pm
+++ b/src/PVE/Firewall.pm
@@ -3384,7 +3384,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] 28+ messages in thread
* [PATCH pve-firewall v4 06/27] firewall: ipset: add option to handle dangling references on rename
2026-10-22 12:46 [PATCH container/docs/firewall/manager/network/qemu-server v4 00/27] handle dangling references when firewall objects go away Arthur Bied-Charreton
` (4 preceding siblings ...)
2026-10-22 12:46 ` [PATCH pve-firewall v4 05/27] firewall: parser: do not log errors for disabled rules Arthur Bied-Charreton
@ 2026-10-22 12:46 ` Arthur Bied-Charreton
2026-10-22 12:46 ` [PATCH pve-firewall v4 07/27] firewall: ipset: add option to handle dangling references on delete Arthur Bied-Charreton
` (20 subsequent siblings)
26 siblings, 0 replies; 28+ messages in thread
From: Arthur Bied-Charreton @ 2026-10-22 12:46 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 | 83 +++++++++++++++++++++++++++++-----
1 file changed, 71 insertions(+), 12 deletions(-)
diff --git a/src/PVE/API2/Firewall/IPSet.pm b/src/PVE/API2/Firewall/IPSet.pm
index 82b9aaf..a6a44be 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,19 @@ sub register_create {
},
);
+ $properties->{'dangling-references'} = {
+ type => 'string',
+ enum => ['keep', 'disable', 'drop', 'rename', 'force-rename'],
+ optional => 1,
+ description =>
+ "Handle references that renaming the IPSet would leave dangling. Use 'rename' to "
+ . "point them at the new name, 'disable' to disable the referencing rules, or "
+ . "'drop' to remove them entirely. 'force-rename' also renames, but skips the "
+ . "check for an existing IPSet with the target name, to finish a rename "
+ . "interrupted partway.",
+ default => 'keep',
+ };
+
$class->register_method({
name => 'create_ipset',
path => '',
@@ -681,6 +695,17 @@ sub register_create {
sub {
my ($param) = @_;
+ my $dangling_refs = $param->{'dangling-references'} // 'keep';
+
+ # 'force-rename' only differs in skipping the check below, the references are
+ # renamed either way
+ my $ref_action;
+ if ($dangling_refs eq 'force-rename') {
+ $ref_action = 'rename';
+ } elsif ($dangling_refs ne 'keep') {
+ $ref_action = $dangling_refs;
+ }
+
my ($cluster_conf, $fw_conf) = $class->load_config($param);
if ($param->{rename}) {
@@ -690,19 +715,53 @@ 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}' does already exist" })
+ if $fw_conf->{ipset}->{ $param->{name} }
+ && $dangling_refs ne 'force-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 => $ref_action,
+ 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 {
--
2.47.3
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH pve-firewall v4 07/27] firewall: ipset: add option to handle dangling references on delete
2026-10-22 12:46 [PATCH container/docs/firewall/manager/network/qemu-server v4 00/27] handle dangling references when firewall objects go away Arthur Bied-Charreton
` (5 preceding siblings ...)
2026-10-22 12:46 ` [PATCH pve-firewall v4 06/27] firewall: ipset: add option to handle dangling references on rename Arthur Bied-Charreton
@ 2026-10-22 12:46 ` Arthur Bied-Charreton
2026-10-22 12:46 ` [PATCH pve-firewall v4 08/27] firewall: aliases: add option to handle dangling references on rename Arthur Bied-Charreton
` (19 subsequent siblings)
26 siblings, 0 replies; 28+ messages in thread
From: Arthur Bied-Charreton @ 2026-10-22 12:46 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 | 21 +++++++++++++++++++++
1 file changed, 21 insertions(+)
diff --git a/src/PVE/API2/Firewall/IPSet.pm b/src/PVE/API2/Firewall/IPSet.pm
index a6a44be..fe4f8c9 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);
@@ -140,6 +141,15 @@ sub register_delete_ipset {
optional => 1,
description => 'Delete all members of the IPSet, if there are any.',
};
+ $properties->{'dangling-references'} = {
+ type => 'string',
+ enum => ['keep', 'disable', 'drop'],
+ optional => 1,
+ description =>
+ "Handle references that the deletion would leave dangling. Use 'disable' to disable "
+ . "the referencing rules, or 'drop' to remove them entirely.",
+ default => 'keep',
+ };
$class->register_method({
name => 'delete_ipset',
@@ -166,6 +176,17 @@ sub register_delete_ipset {
die "IPSet '$param->{name}' is not empty\n"
if scalar(@$ipset) && !$param->{force};
+ my $action = $param->{'dangling-references'} // 'keep';
+
+ 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);
},
--
2.47.3
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH pve-firewall v4 08/27] firewall: aliases: add option to handle dangling references on rename
2026-10-22 12:46 [PATCH container/docs/firewall/manager/network/qemu-server v4 00/27] handle dangling references when firewall objects go away Arthur Bied-Charreton
` (6 preceding siblings ...)
2026-10-22 12:46 ` [PATCH pve-firewall v4 07/27] firewall: ipset: add option to handle dangling references on delete Arthur Bied-Charreton
@ 2026-10-22 12:46 ` Arthur Bied-Charreton
2026-10-22 12:46 ` [PATCH pve-firewall v4 09/27] firewall: aliases: add option to handle dangling references on delete Arthur Bied-Charreton
` (18 subsequent siblings)
26 siblings, 0 replies; 28+ messages in thread
From: Arthur Bied-Charreton @ 2026-10-22 12:46 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 | 60 ++++++++++++++++++++++++++++++--
1 file changed, 57 insertions(+), 3 deletions(-)
diff --git a/src/PVE/API2/Firewall/Aliases.pm b/src/PVE/API2/Firewall/Aliases.pm
index 4f6960d..9e03de0 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,19 @@ sub register_update_alias {
$properties->{comment} = $api_properties->{comment};
$properties->{digest} = get_standard_option('pve-config-digest');
+ $properties->{'dangling-references'} = {
+ type => 'string',
+ enum => ['keep', 'disable', 'drop', 'rename', 'force-rename'],
+ optional => 1,
+ description =>
+ "Handle references that renaming the alias would leave dangling. Use 'rename' to "
+ . "point them at the new name, 'disable' to disable the referencing rules, or "
+ . "'drop' to remove them entirely. IPSet members have no disabled state and are "
+ . "removed in both cases. 'force-rename' also renames, but skips the check for an "
+ . "existing alias with the target name, to finish a rename interrupted partway.",
+ default => 'keep',
+ };
+
$class->register_method({
name => 'update_alias',
path => '{name}',
@@ -239,6 +254,17 @@ sub register_update_alias {
sub {
my ($param) = @_;
+ my $dangling_refs = $param->{'dangling-references'} // 'keep';
+
+ # 'force-rename' only differs in skipping the check above, the references are
+ # renamed either way
+ my $ref_action;
+ if ($dangling_refs eq 'force-rename') {
+ $ref_action = 'rename';
+ } elsif ($dangling_refs ne 'keep') {
+ $ref_action = $dangling_refs;
+ }
+
my ($fw_conf, $aliases) = $class->load_config($param);
my $list = &$aliases_to_list($aliases);
@@ -261,9 +287,37 @@ 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})
+ && $dangling_refs ne 'force-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 => $ref_action,
+ subject => "alias '$name' renamed to '$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] 28+ messages in thread
* [PATCH pve-firewall v4 09/27] firewall: aliases: add option to handle dangling references on delete
2026-10-22 12:46 [PATCH container/docs/firewall/manager/network/qemu-server v4 00/27] handle dangling references when firewall objects go away Arthur Bied-Charreton
` (7 preceding siblings ...)
2026-10-22 12:46 ` [PATCH pve-firewall v4 08/27] firewall: aliases: add option to handle dangling references on rename Arthur Bied-Charreton
@ 2026-10-22 12:46 ` Arthur Bied-Charreton
2026-10-22 12:46 ` [PATCH pve-firewall v4 10/27] firewall: api: run reference updates as tasks Arthur Bied-Charreton
` (17 subsequent siblings)
26 siblings, 0 replies; 28+ messages in thread
From: Arthur Bied-Charreton @ 2026-10-22 12:46 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 | 22 ++++++++++++++++++++++
1 file changed, 22 insertions(+)
diff --git a/src/PVE/API2/Firewall/Aliases.pm b/src/PVE/API2/Firewall/Aliases.pm
index 9e03de0..cb9b448 100644
--- a/src/PVE/API2/Firewall/Aliases.pm
+++ b/src/PVE/API2/Firewall/Aliases.pm
@@ -337,6 +337,16 @@ sub register_delete_alias {
$properties->{name} = $api_properties->{name};
$properties->{digest} = get_standard_option('pve-config-digest');
+ $properties->{'dangling-references'} = {
+ type => 'string',
+ enum => ['keep', 'disable', 'drop'],
+ optional => 1,
+ description =>
+ "Handle references that the deletion would leave dangling. Use 'disable' to disable "
+ . "the referencing rules, or 'drop' to remove them entirely. IPSet members have no "
+ . "disabled state and are removed in both cases.",
+ default => 'keep',
+ };
$class->register_method({
name => 'remove_alias',
@@ -365,6 +375,18 @@ sub register_delete_alias {
PVE::Tools::assert_if_modified($digest, $param->{digest});
my $name = lc($param->{name});
+
+ my $action = $param->{'dangling-references'} // 'keep';
+
+ 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] 28+ messages in thread
* [PATCH pve-firewall v4 10/27] firewall: api: run reference updates as tasks
2026-10-22 12:46 [PATCH container/docs/firewall/manager/network/qemu-server v4 00/27] handle dangling references when firewall objects go away Arthur Bied-Charreton
` (8 preceding siblings ...)
2026-10-22 12:46 ` [PATCH pve-firewall v4 09/27] firewall: aliases: add option to handle dangling references on delete Arthur Bied-Charreton
@ 2026-10-22 12:46 ` Arthur Bied-Charreton
2026-10-22 12:46 ` [PATCH pve-firewall v4 11/27] firewall: tests: add tests for object reference updates Arthur Bied-Charreton
` (16 subsequent siblings)
26 siblings, 0 replies; 28+ messages in thread
From: Arthur Bied-Charreton @ 2026-10-22 12:46 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
workers for renames and removals in order for the logs to land in the
task log, which is visible cluster-wide, instead of the syslog, which
is only visible on the node that happened to handle the request. Plain
creates or updates that only change a comment still run synchronously.
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 | 238 +++++++++++++++++-------------
src/PVE/API2/Firewall/IPSet.pm | 241 ++++++++++++++++++-------------
src/PVE/Firewall/References.pm | 20 +++
3 files changed, 297 insertions(+), 202 deletions(-)
diff --git a/src/PVE/API2/Firewall/Aliases.pm b/src/PVE/API2/Firewall/Aliases.pm
index cb9b448..36d091e 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,36 @@ 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, $dangling_refs) = @_;
+
+ $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})
+ && $dangling_refs ne 'force-rename'
+ ) {
+ raise_param_exc({ name => "alias '$param->{rename}' already exists" });
+ }
+};
+
sub register_get_aliases {
my ($class) = @_;
@@ -245,87 +275,93 @@ 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'} // 'keep';
-
- # 'force-rename' only differs in skipping the check above, the references are
- # renamed either way
- my $ref_action;
- if ($dangling_refs eq 'force-rename') {
- $ref_action = 'rename';
- } elsif ($dangling_refs ne 'keep') {
- $ref_action = $dangling_refs;
- }
-
- 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 $data = { name => $param->{name}, cidr => $param->{cidr} };
- $data->{comment} = $param->{comment} if $param->{comment};
+ my $dangling_refs = $param->{'dangling-references'} // 'keep';
+
+ # 'force-rename' only differs in skipping the check above, the references are renamed
+ # either way
+ my $ref_action;
+ if ($dangling_refs eq 'force-rename') {
+ $ref_action = 'rename';
+ } elsif ($dangling_refs ne 'keep') {
+ $ref_action = $dangling_refs;
+ }
+
+ my $renaming = $param->{rename} && lc($param->{name}) ne lc($param->{rename});
+ my $subject =
+ $renaming ? "alias '$param->{name}' renamed to '$param->{rename}'" : undef;
+
+ my $code = sub {
+ $class->lock_config(
+ $param,
+ sub {
+ my ($param) = @_;
+
+ my ($fw_conf, $aliases) = $class->load_config($param);
+
+ $check_update->($aliases, $param, $dangling_refs);
+
+ my $name = lc($param->{name});
+
+ my $data = { name => $param->{name}, cidr => $param->{cidr} };
+ $data->{comment} = $param->{comment} if $param->{comment};
+
+ $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 => $ref_action,
+ subject => $subject,
+ 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};
+ }
- $aliases->{$name} = $data;
+ $class->save_aliases($param, $fw_conf, $aliases);
+ },
+ );
+ };
- my $rename = $param->{rename};
- $rename = lc($rename) if $rename;
-
- if ($rename && ($name ne $rename)) {
- raise_param_exc({ name => "alias '$param->{rename}' already exists" })
- if defined($aliases->{$rename})
- && $dangling_refs ne 'force-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 => $ref_action,
- subject => "alias '$name' renamed to '$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";
- }
+ if (!$renaming) {
+ $code->();
+ return undef;
+ }
- delete $aliases->{$name};
- }
+ # fail on the request instead of inside the task.
+ my (undef, $aliases) = $class->load_config($param);
- $class->save_aliases($param, $fw_conf, $aliases);
- },
- );
+ $check_update->($aliases, $param, $dangling_refs);
- return undef;
+ return run_ref_update('fw-alias-rename', $param->{vmid} // $param->{name}, $code);
},
});
}
@@ -359,41 +395,47 @@ 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'} // 'keep';
+ my $subject = "alias '$param->{name}' deleted";
- 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});
+ $check_digest->($aliases, $param);
- my $action = $param->{'dangling-references'} // 'keep';
+ 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 => $subject,
+ });
- delete $aliases->{$name};
+ delete $aliases->{$name};
- $class->save_aliases($param, $fw_conf, $aliases);
- },
- );
+ $class->save_aliases($param, $fw_conf, $aliases);
+ },
+ );
+ };
- return undef;
+ # fail on the request instead of inside the task
+ my (undef, $aliases) = $class->load_config($param);
+
+ $check_digest->($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 fe4f8c9..a4356e4 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);
@@ -162,37 +162,45 @@ 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'} // 'keep';
+ my $subject = "ipset '$param->{name}' deleted";
- 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'} // 'keep';
+ 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 => $subject,
+ });
- $class->save_ipset($param, $fw_conf, undef);
+ $class->save_ipset($param, $fw_conf, undef);
+ },
+ );
+ };
- },
- );
+ # fail on the request instead of inside the task
+ my (undef, undef, $ipset) = $class->load_config($param);
- return undef;
+ 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);
},
});
}
@@ -555,7 +563,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;
@@ -623,6 +631,25 @@ my $get_ipset_list = sub {
return wantarray ? ($list, $digest) : $list;
};
+my $check_rename = sub {
+ my ($conf, $param, $dangling_refs) = @_;
+
+ 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} }
+ && $dangling_refs ne 'force-rename'
+ ) {
+ raise_param_exc({ name => "IPSet '$param->{name}' already exists" });
+ }
+};
+
sub register_index {
my ($class) = @_;
@@ -707,100 +734,106 @@ 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'} // 'keep';
+ my $dangling_refs = $param->{'dangling-references'} // 'keep';
- # 'force-rename' only differs in skipping the check below, the references are
- # renamed either way
- my $ref_action;
- if ($dangling_refs eq 'force-rename') {
- $ref_action = 'rename';
- } elsif ($dangling_refs ne 'keep') {
- $ref_action = $dangling_refs;
- }
+ # 'force-rename' only differs in skipping the check above, the references are renamed
+ # either way
+ my $ref_action;
+ if ($dangling_refs eq 'force-rename') {
+ $ref_action = 'rename';
+ } elsif ($dangling_refs ne 'keep') {
+ $ref_action = $dangling_refs;
+ }
- 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}' does already exist" })
- if $fw_conf->{ipset}->{ $param->{name} }
- && $dangling_refs ne 'force-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 => $ref_action,
- 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 $renaming = $param->{rename} && $param->{name} ne $param->{rename};
+ my $subject =
+ $renaming ? "ipset '$param->{rename}' renamed to '$param->{name}'" : undef;
+
+ 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, $dangling_refs);
+
+ 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 => $ref_action,
+ subject => $subject,
+ 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) {
+ $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, $dangling_refs);
+
+ 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 37df434..f32c7db 100644
--- a/src/PVE/Firewall/References.pm
+++ b/src/PVE/Firewall/References.pm
@@ -13,6 +13,7 @@ use base 'Exporter';
our @EXPORT_OK = qw(
collect_refs
update_refs
+ run_ref_update
);
sub collect_refs {
@@ -422,6 +423,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] 28+ messages in thread
* [PATCH pve-firewall v4 11/27] firewall: tests: add tests for object reference updates
2026-10-22 12:46 [PATCH container/docs/firewall/manager/network/qemu-server v4 00/27] handle dangling references when firewall objects go away Arthur Bied-Charreton
` (9 preceding siblings ...)
2026-10-22 12:46 ` [PATCH pve-firewall v4 10/27] firewall: api: run reference updates as tasks Arthur Bied-Charreton
@ 2026-10-22 12:46 ` Arthur Bied-Charreton
2026-10-22 12:46 ` [PATCH pve-firewall v4 12/27] firewall: references: handle security groups Arthur Bied-Charreton
` (15 subsequent siblings)
26 siblings, 0 replies; 28+ messages in thread
From: Arthur Bied-Charreton @ 2026-10-22 12:46 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 | 375 +++++++++++++++++++++++++++++++++++
2 files changed, 376 insertions(+), 1 deletion(-)
create mode 100755 src/test/test_update_refs.pl
diff --git a/src/test/Makefile b/src/test/Makefile
index 272a19a..ea3e378 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_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_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..f148eed
--- /dev/null
+++ b/src/test/test_update_refs.pl
@@ -0,0 +1,375 @@
+#!/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 {
+ 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',
+ );
+ # it goes on to walk the cluster, which this has no business doing, but it gets past the
+ # argument checks without a save callback
+ 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] 28+ messages in thread
* [PATCH pve-firewall v4 12/27] firewall: references: handle security groups
2026-10-22 12:46 [PATCH container/docs/firewall/manager/network/qemu-server v4 00/27] handle dangling references when firewall objects go away Arthur Bied-Charreton
` (10 preceding siblings ...)
2026-10-22 12:46 ` [PATCH pve-firewall v4 11/27] firewall: tests: add tests for object reference updates Arthur Bied-Charreton
@ 2026-10-22 12:46 ` Arthur Bied-Charreton
2026-10-22 12:46 ` [PATCH pve-firewall v4 13/27] firewall: groups: add option to handle dangling references on rename Arthur Bied-Charreton
` (14 subsequent siblings)
26 siblings, 0 replies; 28+ messages in thread
From: Arthur Bied-Charreton @ 2026-10-22 12:46 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 f32c7db..67ba4d0 100644
--- a/src/PVE/Firewall/References.pm
+++ b/src/PVE/Firewall/References.pm
@@ -143,27 +143,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;
@@ -195,7 +205,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;
@@ -215,7 +225,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});
}
@@ -229,7 +239,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.
@@ -252,9 +262,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};
@@ -305,8 +313,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 => [],
};
@@ -341,7 +352,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 name the references carry, or an arrayref of names to disable or drop
new # the name to rewrite them to, only used when renaming
env # 'cluster', 'sdn', or 'vm'/'ct' for a guest-defined object
@@ -352,7 +363,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 f148eed..359c4c4 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 {
my $run = sub {
my (%args) = @_;
--
2.47.3
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH pve-firewall v4 13/27] firewall: groups: add option to handle dangling references on rename
2026-10-22 12:46 [PATCH container/docs/firewall/manager/network/qemu-server v4 00/27] handle dangling references when firewall objects go away Arthur Bied-Charreton
` (11 preceding siblings ...)
2026-10-22 12:46 ` [PATCH pve-firewall v4 12/27] firewall: references: handle security groups Arthur Bied-Charreton
@ 2026-10-22 12:46 ` Arthur Bied-Charreton
2026-10-22 12:46 ` [PATCH pve-firewall v4 14/27] fix #8176: firewall: groups: handle dangling references on delete Arthur Bied-Charreton
` (13 subsequent siblings)
26 siblings, 0 replies; 28+ messages in thread
From: Arthur Bied-Charreton @ 2026-10-22 12:46 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.
The walk runs as a task, so the logs can be found in the task log
afterwards.
Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
---
src/PVE/API2/Firewall/Groups.pm | 199 +++++++++++++++++---------------
1 file changed, 106 insertions(+), 93 deletions(-)
diff --git a/src/PVE/API2/Firewall/Groups.pm b/src/PVE/API2/Firewall/Groups.pm
index d80a650..6b55c97 100644
--- a/src/PVE/API2/Firewall/Groups.pm
+++ b/src/PVE/API2/Firewall/Groups.pm
@@ -4,6 +4,7 @@ use strict;
use warnings;
use PVE::JSONSchema qw(get_standard_option);
use PVE::Exception qw(raise raise_param_exc);
+use PVE::Firewall::References qw(update_refs run_ref_update);
use PVE::Firewall;
use PVE::API2::Firewall::Rules;
@@ -29,13 +30,20 @@ my $get_security_group_list = sub {
return wantarray ? ($list, $digest) : $list;
};
-my $rename_fw_rules = sub {
- my ($old, $new, $rules) = @_;
+my $check_rename = sub {
+ my ($cluster_conf, $param, $dangling_refs) = @_;
- for my $rule (@{$rules}) {
- next if ($rule->{type} ne "group" || $rule->{action} ne $old);
- $rule->{action} = $new;
- }
+ my (undef, $digest) = &$get_security_group_list($cluster_conf);
+ PVE::Tools::assert_if_modified($digest, $param->{digest});
+
+ raise_param_exc({ group => "Security group '$param->{rename}' does not exist" })
+ if !$cluster_conf->{groups}->{ $param->{rename} };
+
+ # prevent overwriting an existing group
+ raise_param_exc({ group => "Security group '$param->{group}' already exists" })
+ if $param->{group} ne $param->{rename}
+ && $cluster_conf->{groups}->{ $param->{group} }
+ && $dangling_refs ne 'force-rename';
};
__PACKAGE__->register_method({
@@ -100,113 +108,118 @@ __PACKAGE__->register_method({
},
),
digest => get_standard_option('pve-config-digest'),
+ 'dangling-references' => {
+ type => 'string',
+ enum => ['keep', 'disable', 'drop', 'rename', 'force-rename'],
+ optional => 1,
+ description =>
+ "Handle references that renaming the security group would leave dangling. "
+ . "Use 'rename' to point them at the new name, 'disable' to disable the "
+ . "referencing rules, or 'drop' to remove them entirely. 'force-rename' "
+ . "also renames, but skips the check for an existing security group with "
+ . "the target name, to finish a rename interrupted partway.",
+ default => 'rename',
+ },
},
},
- returns => { type => 'null' },
+ returns => { type => 'string', optional => 1 },
code => sub {
my ($param) = @_;
my $group = $param->{group};
my $rename = $param->{rename};
my $comment = $param->{comment};
+ my $dangling_refs = $param->{'dangling-references'} // 'rename';
+
+ # 'force-rename' only differs in skipping the check above, the references are renamed
+ # either way
+ my $ref_action;
+ if ($dangling_refs eq 'force-rename') {
+ $ref_action = 'rename';
+ } elsif ($dangling_refs ne 'keep') {
+ $ref_action = $dangling_refs;
+ }
- PVE::Firewall::lock_clusterfw_conf(
- 10,
- sub {
- my $cluster_conf = PVE::Firewall::load_clusterfw_conf();
+ my $renaming = $rename && $rename ne $group;
+ my $subject = $renaming ? "security group '$rename' renamed to '$group'" : undef;
- if ($rename) {
- my (undef, $digest) = &$get_security_group_list($cluster_conf);
- PVE::Tools::assert_if_modified($digest, $param->{digest});
+ my $code = sub {
+ PVE::Firewall::lock_clusterfw_conf(
+ 10,
+ sub {
+ my $cluster_conf = PVE::Firewall::load_clusterfw_conf();
- raise_param_exc({ group => "Security group '$rename' does not exist" })
- if !$cluster_conf->{groups}->{$rename};
+ if ($rename) {
+ $check_rename->($cluster_conf, $param, $dangling_refs);
- # prevent overwriting an existing group
- raise_param_exc({ group => "Security group '$group' does already exist" })
- if $cluster_conf->{groups}->{$group}
- && $group ne $rename;
+ if ($renaming) {
+ # create an exact copy of the old security group
+ $cluster_conf->{groups}->{$group} =
+ $cluster_conf->{groups}->{$rename};
+ $cluster_conf->{group_comments}->{$group} =
+ $cluster_conf->{group_comments}->{$rename};
+ }
- if ($rename eq $group) {
- $cluster_conf->{group_comments}->{$rename} = $comment
+ $cluster_conf->{group_comments}->{$group} = $comment
if defined($comment);
- PVE::Firewall::save_clusterfw_conf($cluster_conf);
- return;
- }
-
- # Create an exact copy of the old security group
- $cluster_conf->{groups}->{$group} = $cluster_conf->{groups}->{$rename};
- $cluster_conf->{group_comments}->{$group} =
- $cluster_conf->{group_comments}->{$rename};
-
- # Update comment if provided
- $cluster_conf->{group_comments}->{$group} = $comment if defined($comment);
-
- # Write the copy to the cluster config, so that if something fails inbetween, the new firewall
- # rules won't be broken when the new name is referenced
- PVE::Firewall::save_clusterfw_conf($cluster_conf);
-
- # Update all the host configs to the new copy
- my $hosts = PVE::Cluster::get_nodelist();
- foreach my $host (@$hosts) {
- PVE::Firewall::lock_hostfw_conf(
- $host,
- 10,
- sub {
- my $host_conf_path = "/etc/pve/nodes/$host/host.fw";
- my $host_conf =
- PVE::Firewall::load_hostfw_conf($cluster_conf, $host_conf_path);
-
- if (defined($host_conf)) {
- &$rename_fw_rules($rename, $group, $host_conf->{rules});
- PVE::Firewall::save_hostfw_conf($host_conf,
- $host_conf_path);
- }
- },
- );
- }
- # Update all the VM configs
- my $vms = PVE::Cluster::get_vmlist();
- foreach my $vm (keys %{ $vms->{ids} }) {
- PVE::Firewall::lock_vmfw_conf(
- $vm,
- 10,
- sub {
- my $vm_type = $vms->{ids}->{$vm}->{type} eq "lxc" ? "ct" : "vm";
- my $vm_conf = PVE::Firewall::load_vmfw_conf(
- $cluster_conf, $vm_type, $vm, "/etc/pve/firewall",
- );
-
- if (defined($vm_conf)) {
- &$rename_fw_rules($rename, $group, $vm_conf->{rules});
- PVE::Firewall::save_vmfw_conf($vm, $vm_conf);
- }
- },
- );
+ if ($renaming) {
+ my $saved = 0;
+ my $save = sub {
+ PVE::Firewall::save_clusterfw_conf($cluster_conf);
+ $saved = 1;
+ };
+
+ eval {
+ update_refs({
+ conf => $cluster_conf,
+ kind => 'groups',
+ old => $rename,
+ new => $group,
+ env => 'cluster',
+ action => $ref_action,
+ subject => $subject,
+ save => $save,
+ });
+ };
+ if (my $err = $@) {
+ die "rename interrupted, the security group exists under both "
+ . "names; retry with 'force-rename' to finish: $err"
+ if $saved;
+ die "could not update references to security group "
+ . "'$rename': $err";
+ }
+
+ # now that everything has been updated, the old group can go
+ delete $cluster_conf->{groups}->{$rename};
+ delete $cluster_conf->{group_comments}->{$rename};
+ }
+ } else {
+ foreach my $name (keys %{ $cluster_conf->{groups} }) {
+ raise_param_exc({
+ group => "Security group '$name' already exists" })
+ if $name eq $group;
+ }
+
+ $cluster_conf->{groups}->{$group} = [];
+ $cluster_conf->{group_comments}->{$group} = $comment
+ if defined($comment);
}
- # And also update the cluster itself
- &$rename_fw_rules($rename, $group, $cluster_conf->{rules});
-
- # Now that everything has been updated, the old rule can be deleted
- delete $cluster_conf->{groups}->{$rename};
- delete $cluster_conf->{group_comments}->{$rename};
- } else {
- foreach my $name (keys %{ $cluster_conf->{groups} }) {
- raise_param_exc({ group => "Security group '$name' already exists" })
- if $name eq $group;
- }
+ PVE::Firewall::save_clusterfw_conf($cluster_conf);
+ },
+ );
+ };
- $cluster_conf->{groups}->{$group} = [];
- $cluster_conf->{group_comments}->{$group} = $comment if defined($comment);
- }
+ if (!$renaming) {
+ $code->();
+ return undef;
+ }
- PVE::Firewall::save_clusterfw_conf($cluster_conf);
- },
- );
+ # fail on the request instead of inside the task.
+ $check_rename->(PVE::Firewall::load_clusterfw_conf(), $param, $dangling_refs);
- return undef;
+ return run_ref_update('fw-group-rename', $rename, $code);
},
});
--
2.47.3
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH pve-firewall v4 14/27] fix #8176: firewall: groups: handle dangling references on delete
2026-10-22 12:46 [PATCH container/docs/firewall/manager/network/qemu-server v4 00/27] handle dangling references when firewall objects go away Arthur Bied-Charreton
` (12 preceding siblings ...)
2026-10-22 12:46 ` [PATCH pve-firewall v4 13/27] firewall: groups: add option to handle dangling references on rename Arthur Bied-Charreton
@ 2026-10-22 12:46 ` Arthur Bied-Charreton
2026-10-22 12:46 ` [PATCH pve-network v4 15/27] apply: add option to handle dangling references on VNet deletion Arthur Bied-Charreton
` (12 subsequent siblings)
26 siblings, 0 replies; 28+ messages in thread
From: Arthur Bied-Charreton @ 2026-10-22 12:46 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.
The delete runs as a task for observability, all changes made to
configs are logged to the task log.
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 | 53 ++++++++++++++++++++++++++--------
1 file changed, 41 insertions(+), 12 deletions(-)
diff --git a/src/PVE/API2/Firewall/Rules.pm b/src/PVE/API2/Firewall/Rules.pm
index 785ad05..17f76df 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,55 @@ __PACKAGE__->register_method({
additionalProperties => 0,
properties => {
group => get_standard_option('pve-security-group-name'),
+ 'dangling-references' => {
+ type => 'string',
+ enum => ['keep', 'disable', 'drop'],
+ optional => 1,
+ description =>
+ "Handle references that the deletion would leave dangling. Use 'disable' "
+ . "to disable the referencing rules, or 'drop' to remove them entirely.",
+ default => 'keep',
+ },
},
},
- 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 $subject = "security group '$param->{group}' deleted";
- 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 => $subject,
+ });
+
+ __PACKAGE__->save_rules($param, $cluster_conf, 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] 28+ messages in thread
* [PATCH pve-network v4 15/27] apply: add option to handle dangling references on VNet deletion
2026-10-22 12:46 [PATCH container/docs/firewall/manager/network/qemu-server v4 00/27] handle dangling references when firewall objects go away Arthur Bied-Charreton
` (13 preceding siblings ...)
2026-10-22 12:46 ` [PATCH pve-firewall v4 14/27] fix #8176: firewall: groups: handle dangling references on delete Arthur Bied-Charreton
@ 2026-10-22 12:46 ` Arthur Bied-Charreton
2026-10-22 12:46 ` [PATCH qemu-server v4 16/27] api: destroy_vm: add option to handle dangling IPSet references Arthur Bied-Charreton
` (11 subsequent siblings)
26 siblings, 0 replies; 28+ messages in thread
From: Arthur Bied-Charreton @ 2026-10-22 12:46 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 | 38 +++++++++++++++++++++++++++++++++++++
1 file changed, 38 insertions(+)
diff --git a/src/PVE/API2/Network/SDN.pm b/src/PVE/API2/Network/SDN.pm
index e3c8d9d..88d9d78 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,16 @@ __PACKAGE__->register_method({
description =>
'When lock-token has been provided and configuration successfully committed, release the lock automatically afterwards',
},
+ 'dangling-ipset-references' => {
+ type => 'string',
+ enum => ['keep', 'disable', 'drop'],
+ optional => 1,
+ default => 'keep',
+ 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 +327,35 @@ __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 = [];
+
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] },
+ );
+ };
+ warn "could not update references to VNet IPSets: $@" if $@;
+ }
+ }
+
PVE::Network::SDN::commit_config();
$new_config_has_frr = PVE::Network::SDN::running_config_has_frr();
@@ -336,6 +370,10 @@ __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 the record is only logged here.
+ print("$_\n") for $ref_log->@*;
+
$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] 28+ messages in thread
* [PATCH qemu-server v4 16/27] api: destroy_vm: add option to handle dangling IPSet references
2026-10-22 12:46 [PATCH container/docs/firewall/manager/network/qemu-server v4 00/27] handle dangling references when firewall objects go away Arthur Bied-Charreton
` (14 preceding siblings ...)
2026-10-22 12:46 ` [PATCH pve-network v4 15/27] apply: add option to handle dangling references on VNet deletion Arthur Bied-Charreton
@ 2026-10-22 12:46 ` Arthur Bied-Charreton
2026-10-22 12:46 ` [PATCH pve-container v4 17/27] " Arthur Bied-Charreton
` (10 subsequent siblings)
26 siblings, 0 replies; 28+ messages in thread
From: Arthur Bied-Charreton @ 2026-10-22 12:46 UTC (permalink / raw)
To: pve-devel
For each guest with IPAM entries, a 'guest-ipam-<vmid>' IPSet is
auto-generated. 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.
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 cfbc1f51..bba6c270 100644
--- a/src/PVE/API2/Qemu.pm
+++ b/src/PVE/API2/Qemu.pm
@@ -56,6 +56,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;
@@ -2838,6 +2839,15 @@ __PACKAGE__->register_method({
optional => 1,
default => 0,
},
+ 'dangling-ipset-references' => {
+ type => 'string',
+ enum => ['keep', 'disable', 'drop'],
+ optional => 1,
+ default => 'keep',
+ 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 => {
@@ -2915,6 +2925,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] 28+ messages in thread
* [PATCH pve-container v4 17/27] api: destroy_vm: add option to handle dangling IPSet references
2026-10-22 12:46 [PATCH container/docs/firewall/manager/network/qemu-server v4 00/27] handle dangling references when firewall objects go away Arthur Bied-Charreton
` (15 preceding siblings ...)
2026-10-22 12:46 ` [PATCH qemu-server v4 16/27] api: destroy_vm: add option to handle dangling IPSet references Arthur Bied-Charreton
@ 2026-10-22 12:46 ` Arthur Bied-Charreton
2026-10-22 12:46 ` [PATCH pve-docs v4 18/27] firewall: remove trailing spaces Arthur Bied-Charreton
` (9 subsequent siblings)
26 siblings, 0 replies; 28+ messages in thread
From: Arthur Bied-Charreton @ 2026-10-22 12:46 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 5f94d5a..f0d7438 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);
@@ -845,6 +846,15 @@ __PACKAGE__->register_method({
. " enabled storages which are not referenced in the config.",
optional => 1,
},
+ 'dangling-ipset-references' => {
+ type => 'string',
+ enum => ['keep', 'disable', 'drop'],
+ optional => 1,
+ default => 'keep',
+ 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 => {
@@ -922,6 +932,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] 28+ messages in thread
* [PATCH pve-docs v4 18/27] firewall: remove trailing spaces
2026-10-22 12:46 [PATCH container/docs/firewall/manager/network/qemu-server v4 00/27] handle dangling references when firewall objects go away Arthur Bied-Charreton
` (16 preceding siblings ...)
2026-10-22 12:46 ` [PATCH pve-container v4 17/27] " Arthur Bied-Charreton
@ 2026-10-22 12:46 ` Arthur Bied-Charreton
2026-10-22 12:47 ` [PATCH pve-docs v4 19/27] firewall: document renaming and removing objects Arthur Bied-Charreton
` (8 subsequent siblings)
26 siblings, 0 replies; 28+ messages in thread
From: Arthur Bied-Charreton @ 2026-10-22 12:46 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] 28+ messages in thread
* [PATCH pve-docs v4 19/27] firewall: document renaming and removing objects
2026-10-22 12:46 [PATCH container/docs/firewall/manager/network/qemu-server v4 00/27] handle dangling references when firewall objects go away Arthur Bied-Charreton
` (17 preceding siblings ...)
2026-10-22 12:46 ` [PATCH pve-docs v4 18/27] firewall: remove trailing spaces Arthur Bied-Charreton
@ 2026-10-22 12:47 ` Arthur Bied-Charreton
2026-10-22 12:47 ` [PATCH pve-manager v4 20/27] ui: firewall: add a common widget for handling dangling references Arthur Bied-Charreton
` (7 subsequent siblings)
26 siblings, 0 replies; 28+ messages in thread
From: Arthur Bied-Charreton @ 2026-10-22 12:47 UTC (permalink / raw)
To: pve-devel
Renaming or removing a security group, IP alias or IP set leaves every
reference to it naming something that no longer exists, and those
references are not confined to the configuration the object is defined
in. Describe where they live, what each value of the
'dangling-references' option does with them, and why 'force-rename'
exists and is kept to the command line.
The option descriptions in the API and the GUI are deliberately short,
so this is where the reasoning behind each choice belongs.
Point the SDN warning about rules referencing a released IPAM IPSet at
the new section, since destroying a guest now offers to deal with them.
Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
---
pve-firewall.adoc | 64 +++++++++++++++++++++++++++++++++++++++++++++++
pvesdn.adoc | 9 ++++---
2 files changed, 70 insertions(+), 3 deletions(-)
diff --git a/pve-firewall.adoc b/pve-firewall.adoc
index cf04781..b4d7068 100644
--- a/pve-firewall.adoc
+++ b/pve-firewall.adoc
@@ -468,6 +468,70 @@ 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. 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.
+
+The `force-rename` option skips that check so a retry can finish the rename. It
+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 force-rename
+----
+
+[[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 3fd3533..69cfa93 100644
--- a/pvesdn.adoc
+++ b/pvesdn.adoc
@@ -1526,9 +1526,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] 28+ messages in thread
* [PATCH pve-manager v4 20/27] ui: firewall: add a common widget for handling dangling references
2026-10-22 12:46 [PATCH container/docs/firewall/manager/network/qemu-server v4 00/27] handle dangling references when firewall objects go away Arthur Bied-Charreton
` (18 preceding siblings ...)
2026-10-22 12:47 ` [PATCH pve-docs v4 19/27] firewall: document renaming and removing objects Arthur Bied-Charreton
@ 2026-10-22 12:47 ` Arthur Bied-Charreton
2026-10-22 12:47 ` [PATCH pve-manager v4 21/27] ui: firewall: ipset: add controls for " Arthur Bied-Charreton
` (6 subsequent siblings)
26 siblings, 0 replies; 28+ messages in thread
From: Arthur Bied-Charreton @ 2026-10-22 12:47 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] 28+ messages in thread
* [PATCH pve-manager v4 21/27] ui: firewall: ipset: add controls for dangling references
2026-10-22 12:46 [PATCH container/docs/firewall/manager/network/qemu-server v4 00/27] handle dangling references when firewall objects go away Arthur Bied-Charreton
` (19 preceding siblings ...)
2026-10-22 12:47 ` [PATCH pve-manager v4 20/27] ui: firewall: add a common widget for handling dangling references Arthur Bied-Charreton
@ 2026-10-22 12:47 ` Arthur Bied-Charreton
2026-10-22 12:47 ` [PATCH pve-manager v4 22/27] ui: firewall: aliases: " Arthur Bied-Charreton
` (5 subsequent siblings)
26 siblings, 0 replies; 28+ messages in thread
From: Arthur Bied-Charreton @ 2026-10-22 12:47 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] 28+ messages in thread
* [PATCH pve-manager v4 22/27] ui: firewall: aliases: add controls for dangling references
2026-10-22 12:46 [PATCH container/docs/firewall/manager/network/qemu-server v4 00/27] handle dangling references when firewall objects go away Arthur Bied-Charreton
` (20 preceding siblings ...)
2026-10-22 12:47 ` [PATCH pve-manager v4 21/27] ui: firewall: ipset: add controls for " Arthur Bied-Charreton
@ 2026-10-22 12:47 ` Arthur Bied-Charreton
2026-10-22 12:47 ` [PATCH pve-manager v4 23/27] ui: sdn: apply: add control for dangling IPSet references Arthur Bied-Charreton
` (4 subsequent siblings)
26 siblings, 0 replies; 28+ messages in thread
From: Arthur Bied-Charreton @ 2026-10-22 12:47 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] 28+ messages in thread
* [PATCH pve-manager v4 23/27] ui: sdn: apply: add control for dangling IPSet references
2026-10-22 12:46 [PATCH container/docs/firewall/manager/network/qemu-server v4 00/27] handle dangling references when firewall objects go away Arthur Bied-Charreton
` (21 preceding siblings ...)
2026-10-22 12:47 ` [PATCH pve-manager v4 22/27] ui: firewall: aliases: " Arthur Bied-Charreton
@ 2026-10-22 12:47 ` Arthur Bied-Charreton
2026-10-22 12:47 ` [PATCH pve-manager v4 24/27] ui: guest destroy: use let for non-constant variable bindings Arthur Bied-Charreton
` (3 subsequent siblings)
26 siblings, 0 replies; 28+ messages in thread
From: Arthur Bied-Charreton @ 2026-10-22 12:47 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 | 118 +++++++++++++++++++++++++++------
1 file changed, 98 insertions(+), 20 deletions(-)
diff --git a/www/manager6/sdn/StatusView.js b/www/manager6/sdn/StatusView.js
index fada5041..ce919639 100644
--- a/www/manager6/sdn/StatusView.js
+++ b/www/manager6/sdn/StatusView.js
@@ -1,3 +1,87 @@
+// 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 helpLabel = Ext.htmlEncode(gettext('their auto-generated IPSets'));
+ let refsHint = Ext.String.format(
+ gettext('Firewall rules referencing {0} will no longer resolve.'),
+ `<a target="_blank" href="${helpLink}">${helpLabel}</a>`,
+ );
+
+ 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 +129,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] 28+ messages in thread
* [PATCH pve-manager v4 24/27] ui: guest destroy: use let for non-constant variable bindings
2026-10-22 12:46 [PATCH container/docs/firewall/manager/network/qemu-server v4 00/27] handle dangling references when firewall objects go away Arthur Bied-Charreton
` (22 preceding siblings ...)
2026-10-22 12:47 ` [PATCH pve-manager v4 23/27] ui: sdn: apply: add control for dangling IPSet references Arthur Bied-Charreton
@ 2026-10-22 12:47 ` Arthur Bied-Charreton
2026-10-22 12:47 ` [PATCH pve-manager v4 25/27] ui: guest destroy: add control for dangling IPSet references Arthur Bied-Charreton
` (2 subsequent siblings)
26 siblings, 0 replies; 28+ messages in thread
From: Arthur Bied-Charreton @ 2026-10-22 12:47 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] 28+ messages in thread
* [PATCH pve-manager v4 25/27] ui: guest destroy: add control for dangling IPSet references
2026-10-22 12:46 [PATCH container/docs/firewall/manager/network/qemu-server v4 00/27] handle dangling references when firewall objects go away Arthur Bied-Charreton
` (23 preceding siblings ...)
2026-10-22 12:47 ` [PATCH pve-manager v4 24/27] ui: guest destroy: use let for non-constant variable bindings Arthur Bied-Charreton
@ 2026-10-22 12:47 ` Arthur Bied-Charreton
2026-10-22 12:47 ` [PATCH pve-manager v4 26/27] ui: firewall: show the task for reference updates Arthur Bied-Charreton
2026-10-22 12:47 ` [PATCH pve-manager v4 27/27] ui: firewall: groups: add controls for dangling references Arthur Bied-Charreton
26 siblings, 0 replies; 28+ messages in thread
From: Arthur Bied-Charreton @ 2026-10-22 12:47 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 | 65 ++++++++++++++++++++++++-
1 file changed, 64 insertions(+), 1 deletion(-)
diff --git a/www/manager6/window/SafeDestroyGuest.js b/www/manager6/window/SafeDestroyGuest.js
index 45b277e5..536bfc6a 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,62 @@ 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 helpLabel = Ext.htmlEncode(gettext("this guest's auto-generated IPSet"));
+ let refsHint = Ext.String.format(
+ gettext('Firewall rules referencing {0} will no longer resolve.'),
+ `<a target="_blank" href="${helpLink}">${helpLabel}</a>`,
+ );
+
+ 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 +103,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] 28+ messages in thread
* [PATCH pve-manager v4 26/27] ui: firewall: show the task for reference updates
2026-10-22 12:46 [PATCH container/docs/firewall/manager/network/qemu-server v4 00/27] handle dangling references when firewall objects go away Arthur Bied-Charreton
` (24 preceding siblings ...)
2026-10-22 12:47 ` [PATCH pve-manager v4 25/27] ui: guest destroy: add control for dangling IPSet references Arthur Bied-Charreton
@ 2026-10-22 12:47 ` Arthur Bied-Charreton
2026-10-22 12:47 ` [PATCH pve-manager v4 27/27] ui: firewall: groups: add controls for dangling references Arthur Bied-Charreton
26 siblings, 0 replies; 28+ messages in thread
From: Arthur Bied-Charreton @ 2026-10-22 12:47 UTC (permalink / raw)
To: pve-devel
Renaming or deleting an ipset or alias returns a UPID. 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 8b99371d..9ec3951a 100644
--- a/www/manager6/Utils.js
+++ b/www/manager6/Utils.js
@@ -2194,6 +2194,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] 28+ messages in thread
* [PATCH pve-manager v4 27/27] ui: firewall: groups: add controls for dangling references
2026-10-22 12:46 [PATCH container/docs/firewall/manager/network/qemu-server v4 00/27] handle dangling references when firewall objects go away Arthur Bied-Charreton
` (25 preceding siblings ...)
2026-10-22 12:47 ` [PATCH pve-manager v4 26/27] ui: firewall: show the task for reference updates Arthur Bied-Charreton
@ 2026-10-22 12:47 ` Arthur Bied-Charreton
26 siblings, 0 replies; 28+ messages in thread
From: Arthur Bied-Charreton @ 2026-10-22 12:47 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
'delete'. 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 9ec3951a..fcfb5f95 100644
--- a/www/manager6/Utils.js
+++ b/www/manager6/Utils.js
@@ -2196,6 +2196,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] 28+ messages in thread
end of thread, other threads:[~2026-10-22 12:51 UTC | newest]
Thread overview: 28+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-10-22 12:46 [PATCH container/docs/firewall/manager/network/qemu-server v4 00/27] handle dangling references when firewall objects go away Arthur Bied-Charreton
2026-10-22 12:46 ` [PATCH pve-firewall v4 01/27] firewall: move object reference helpers to their own module Arthur Bied-Charreton
2026-10-22 12:46 ` [PATCH pve-firewall v4 02/27] firewall: references: add helpers to update object references Arthur Bied-Charreton
2026-10-22 12:46 ` [PATCH pve-firewall v4 03/27] firewall: references: log the references an object update touched Arthur Bied-Charreton
2026-10-22 12:46 ` [PATCH pve-firewall v4 04/27] firewall: references: add wrappers for the SDN-generated IPSets Arthur Bied-Charreton
2026-10-22 12:46 ` [PATCH pve-firewall v4 05/27] firewall: parser: do not log errors for disabled rules Arthur Bied-Charreton
2026-10-22 12:46 ` [PATCH pve-firewall v4 06/27] firewall: ipset: add option to handle dangling references on rename Arthur Bied-Charreton
2026-10-22 12:46 ` [PATCH pve-firewall v4 07/27] firewall: ipset: add option to handle dangling references on delete Arthur Bied-Charreton
2026-10-22 12:46 ` [PATCH pve-firewall v4 08/27] firewall: aliases: add option to handle dangling references on rename Arthur Bied-Charreton
2026-10-22 12:46 ` [PATCH pve-firewall v4 09/27] firewall: aliases: add option to handle dangling references on delete Arthur Bied-Charreton
2026-10-22 12:46 ` [PATCH pve-firewall v4 10/27] firewall: api: run reference updates as tasks Arthur Bied-Charreton
2026-10-22 12:46 ` [PATCH pve-firewall v4 11/27] firewall: tests: add tests for object reference updates Arthur Bied-Charreton
2026-10-22 12:46 ` [PATCH pve-firewall v4 12/27] firewall: references: handle security groups Arthur Bied-Charreton
2026-10-22 12:46 ` [PATCH pve-firewall v4 13/27] firewall: groups: add option to handle dangling references on rename Arthur Bied-Charreton
2026-10-22 12:46 ` [PATCH pve-firewall v4 14/27] fix #8176: firewall: groups: handle dangling references on delete Arthur Bied-Charreton
2026-10-22 12:46 ` [PATCH pve-network v4 15/27] apply: add option to handle dangling references on VNet deletion Arthur Bied-Charreton
2026-10-22 12:46 ` [PATCH qemu-server v4 16/27] api: destroy_vm: add option to handle dangling IPSet references Arthur Bied-Charreton
2026-10-22 12:46 ` [PATCH pve-container v4 17/27] " Arthur Bied-Charreton
2026-10-22 12:46 ` [PATCH pve-docs v4 18/27] firewall: remove trailing spaces Arthur Bied-Charreton
2026-10-22 12:47 ` [PATCH pve-docs v4 19/27] firewall: document renaming and removing objects Arthur Bied-Charreton
2026-10-22 12:47 ` [PATCH pve-manager v4 20/27] ui: firewall: add a common widget for handling dangling references Arthur Bied-Charreton
2026-10-22 12:47 ` [PATCH pve-manager v4 21/27] ui: firewall: ipset: add controls for " Arthur Bied-Charreton
2026-10-22 12:47 ` [PATCH pve-manager v4 22/27] ui: firewall: aliases: " Arthur Bied-Charreton
2026-10-22 12:47 ` [PATCH pve-manager v4 23/27] ui: sdn: apply: add control for dangling IPSet references Arthur Bied-Charreton
2026-10-22 12:47 ` [PATCH pve-manager v4 24/27] ui: guest destroy: use let for non-constant variable bindings Arthur Bied-Charreton
2026-10-22 12:47 ` [PATCH pve-manager v4 25/27] ui: guest destroy: add control for dangling IPSet references Arthur Bied-Charreton
2026-10-22 12:47 ` [PATCH pve-manager v4 26/27] ui: firewall: show the task for reference updates Arthur Bied-Charreton
2026-10-22 12:47 ` [PATCH pve-manager v4 27/27] ui: firewall: groups: add controls for dangling references Arthur Bied-Charreton
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox