From: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
To: pve-devel@lists.proxmox.com
Subject: [PATCH pve-firewall v4 02/27] firewall: references: add helpers to update object references
Date: Thu, 22 Oct 2026 14:46:43 +0200 [thread overview]
Message-ID: <20261022124708.429069-3-a.bied-charreton@proxmox.com> (raw)
In-Reply-To: <20261022124708.429069-1-a.bied-charreton@proxmox.com>
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
next prev parent reply other threads:[~2026-10-22 12:48 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-10-22 12:46 [PATCH container/docs/firewall/manager/network/qemu-server v4 00/27] handle dangling references when firewall objects go away Arthur Bied-Charreton
2026-10-22 12:46 ` [PATCH pve-firewall v4 01/27] firewall: move object reference helpers to their own module Arthur Bied-Charreton
2026-10-22 12:46 ` Arthur Bied-Charreton [this message]
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20261022124708.429069-3-a.bied-charreton@proxmox.com \
--to=a.bied-charreton@proxmox.com \
--cc=pve-devel@lists.proxmox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox