[PATCH firewall/manager 0/5] Allow updating references to firewall objects when editing them

[PATCH firewall/manager 0/5] Allow updating references to firewall objects when editing them

[Arthur Bied-Charreton]

Renaming/deleting ipsets or aliases that are referenced by other rules,
security groups, or ipsets currently leaves dangling references behind.
This creates broken firewall configurations where the rules with those
dangling references fail to parse, meaning they are ignored by the
firewall.

This is especially dangerous in the renaming case, where a user might
reasonably expect references to update automatically, meaning they may
go about their day after renaming an object without realizing a whole
set of firewall rules is now inactive.

This patch series addresses this by adding optional parameters to the
related firewall endpoints that allow updating/deleting all references
to an object when editing/removing it. The UI patches add checkboxes to
the edit/remove windows to optionally pass this parameter.

The casing in the firewall API is inconsistent: ipset names are stored
as-is in the SectionConfig (e.g. 'FOO'), but lowercased when the config
is loaded. This means a rule referencing '+dc/FOO' will fail to match
its own definition after a load. In practice this is self-correcting:
lowercasing is applied in-place, so definitions are written back in
lowercase on the next read-write cycle, and the API does not allow
creating rules with references that do not match anything. There is
however a window between object creation and that first cycle where
references can be broken. To avoid false negatives in that window,
this series uses case-insensitive matching when scanning for
references, and renamed references are always written back lowercased.

Stefan and I talked about this off-list and came to the conclusion that
enforcing either casing may break existing configs that have been
manually edited, so a more general fix would be better folded into a
major release.

The UI patches introduce some code duplication. I tried to generalize
the remove button with an optional extra URL parameter, but I did not
find a good name for it, nor did I find a nice common ground that did
not involve making the call sites even more verbose with configuration
options, which is why I ended up leaving it like this. I am of course
open to suggestions.

The original plan was to have an upfront check for would-be-dangling
references before allowing any edit to an object, however this can be a
very expensive operation, since an object defined at the cluster level
may be referenced by guest and host configurations in the whole cluster.
A check like this would trigger an unconditional scan of all those
configs when clicking the remove buttons, which is why it is kept
opt-in in the UI.


pve-firewall:

Arthur Bied-Charreton (3):
  Add helpers for updating alias and ipset references
  ipset: Add option to update references on rename/delete
  aliases: Add option to update references on rename/delete

 src/PVE/API2/Firewall/Aliases.pm | 115 +++++++++++++++++++++++++++++++
 src/PVE/API2/Firewall/Helpers.pm |  66 ++++++++++++++++++
 src/PVE/API2/Firewall/IPSet.pm   | 108 ++++++++++++++++++++++++++++-
 3 files changed, 288 insertions(+), 1 deletion(-)


pve-manager:

Arthur Bied-Charreton (2):
  ipset: Add option to update references on rename/delete
  aliases: Add option to update references on rename/delete

 www/manager6/grid/FirewallAliases.js | 101 +++++++++++++++++++++------
 www/manager6/panel/IPSet.js          |  60 +++++++++++++++-
 2 files changed, 136 insertions(+), 25 deletions(-)


Summary over all repositories:
  5 files changed, 424 insertions(+), 26 deletions(-)

-- 
Generated by murpp 0.11.0

[PATCH pve-firewall 1/5] Add helpers for updating alias and ipset references

[Arthur Bied-Charreton]

Both ipsets and aliases require similar logic, where we need to
be able to iterate over all firewall configs in the cluster (cluster,
nodes and guests) to find and update all references to a given object.

Add filter_map() and foreach_conf_in_env() as shared helpers that take
closures, allowing the ipset and alias handlers to reuse some of the
traversal logic.

Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
---
 src/PVE/API2/Firewall/Helpers.pm | 66 ++++++++++++++++++++++++++++++++
 1 file changed, 66 insertions(+)

diff --git a/src/PVE/API2/Firewall/Helpers.pm b/src/PVE/API2/Firewall/Helpers.pm
index 0fb71f7..8a8759a 100644
--- a/src/PVE/API2/Firewall/Helpers.pm
+++ b/src/PVE/API2/Firewall/Helpers.pm
@@ -4,8 +4,74 @@ use strict;
 use warnings;
 
 use PVE::Cluster;
+use PVE::Firewall;
 use PVE::Network::SDN::Vnets;
 use PVE::RPCEnvironment;
+use base 'Exporter';
+our @EXPORT_OK = qw(filter_map foreach_conf_in_env);
+
+# Apply $action to each item in $items for which $matches->($item) is true. Remove item
+# if $matches->($item) is true and $action->($item) returns undef.
+#
+# Returns the updated items arrayref and a boolean indicating whether any item was matched.
+sub filter_map {
+    my ($items, $action, $matches) = @_;
+    my @result;
+    my $modified = 0;
+    for my $item (@{ $items // [] }) {
+        if ($matches->($item)) {
+            $modified = 1;
+            my $new = $action->($item);
+            push @result, $new if defined $new;
+        } else {
+            push @result, $item;
+        }
+    }
+    return (\@result, $modified);
+}
+
+# Apply $rewrite to the main firewall config and, if $rule_env is 'cluster', to all guest
+# and host firewall configs across the cluster. Configs where $rewrite returns true are saved.
+# The caller is responsible for locking and saving the cluster config. Guest and host
+# configs are locked by this function.
+sub foreach_conf_in_env {
+    my ($conf, $rule_env, $rewrite) = @_;
+
+    $rewrite->($conf, $rule_env, 0);
+
+    return if $rule_env ne 'cluster';
+
+    my $vmlist = PVE::Cluster::get_vmlist();
+    for my $vmid (keys %{ ($vmlist // {})->{ids} // {} }) {
+        PVE::Firewall::lock_vmfw_conf(
+            $vmid,
+            10,
+            sub {
+                my $type = $vmlist->{ids}->{$vmid}->{type};
+                my $env = $type eq 'lxc' ? 'ct' : 'vm';
+                my $guest_conf = PVE::Firewall::load_vmfw_conf($conf, $env, $vmid);
+                if ($rewrite->($guest_conf, 'cluster', 1)) {
+                    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);
+                return if !defined($host_conf);
+                if ($rewrite->($host_conf, 'cluster', 0)) {
+                    PVE::Firewall::save_hostfw_conf($host_conf, $host_conf_path);
+                }
+            },
+        );
+    }
+}
 
 sub get_allowed_vnets {
     my $rpcenv = eval { PVE::RPCEnvironment::get() };
-- 
2.47.3

Re: [PATCH pve-firewall 1/5] Add helpers for updating alias and ipset references

[Stefan Hanreich]

On 4/7/26 9:36 AM, Arthur Bied-Charreton wrote:
> Both ipsets and aliases require similar logic, where we need to
> be able to iterate over all firewall configs in the cluster (cluster,
> nodes and guests) to find and update all references to a given object.
> 
> Add filter_map() and foreach_conf_in_env() as shared helpers that take
> closures, allowing the ipset and alias handlers to reuse some of the
> traversal logic.
> 
> Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
> ---
>  src/PVE/API2/Firewall/Helpers.pm | 66 ++++++++++++++++++++++++++++++++
>  1 file changed, 66 insertions(+)
> 
> diff --git a/src/PVE/API2/Firewall/Helpers.pm b/src/PVE/API2/Firewall/Helpers.pm
> index 0fb71f7..8a8759a 100644
> --- a/src/PVE/API2/Firewall/Helpers.pm
> +++ b/src/PVE/API2/Firewall/Helpers.pm
> @@ -4,8 +4,74 @@ use strict;
>  use warnings;
>  
>  use PVE::Cluster;
> +use PVE::Firewall;
>  use PVE::Network::SDN::Vnets;
>  use PVE::RPCEnvironment;
> +use base 'Exporter';
> +our @EXPORT_OK = qw(filter_map foreach_conf_in_env);
> +
> +# Apply $action to each item in $items for which $matches->($item) is true. Remove item
> +# if $matches->($item) is true and $action->($item) returns undef.
> +#
> +# Returns the updated items arrayref and a boolean indicating whether any item was matched.

I've started to introduce the POD format for comments (e.g. in
src/PVE/Firewall/Helpers.pm) for new functions in the Firewall, so we
could utilize that here as well?

> +sub filter_map {

The name is imo a bit misleading, since the result still includes
everything that *doesn't* match, so there's not really any filtering
included. I, personally, would think from the name of this function that
it filters everything according to matches, and then maps it via
$action. I'd think of this more like a simple map operation rather, that
returns the item as-is if it doesn't fit a given criteria, otherwise
transforms it.

> +    my ($items, $action, $matches) = @_;
> +    my @result;
> +    my $modified = 0;
> +    for my $item (@{ $items // [] }) {
> +        if ($matches->($item)) {
> +            $modified = 1;
> +            my $new = $action->($item);
> +            push @result, $new if defined $new;
> +        } else {
> +            push @result, $item;
> +        }
> +    }
> +    return (\@result, $modified);
> +}
> +
> +# Apply $rewrite to the main firewall config and, if $rule_env is 'cluster', to all guest
> +# and host firewall configs across the cluster. Configs where $rewrite returns true are saved.
> +# The caller is responsible for locking and saving the cluster config. Guest and host
> +# configs are locked by this function.
> +sub foreach_conf_in_env {
> +    my ($conf, $rule_env, $rewrite) = @_;
> +
> +    $rewrite->($conf, $rule_env, 0);
> +
> +    return if $rule_env ne 'cluster';
> +
> +    my $vmlist = PVE::Cluster::get_vmlist();
> +    for my $vmid (keys %{ ($vmlist // {})->{ids} // {} }) {
> +        PVE::Firewall::lock_vmfw_conf(
> +            $vmid,
> +            10,
> +            sub {
> +                my $type = $vmlist->{ids}->{$vmid}->{type};
> +                my $env = $type eq 'lxc' ? 'ct' : 'vm';
> +                my $guest_conf = PVE::Firewall::load_vmfw_conf($conf, $env, $vmid);
> +                if ($rewrite->($guest_conf, 'cluster', 1)) {
> +                    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);
> +                return if !defined($host_conf);
> +                if ($rewrite->($host_conf, 'cluster', 0)) {
> +                    PVE::Firewall::save_hostfw_conf($host_conf, $host_conf_path);
> +                }
> +            },
> +        );
> +    }
> +}

This is missing the VNet firewall configurations entirely, they do not
get updated as a consequence.

[PATCH pve-firewall 2/5] ipset: Add option to update references on rename/delete

[Arthur Bied-Charreton]

Renaming or deleting an IPSet that is referenced by rules or security
groups would leave dangling references, creating broken configurations.

Add option to the POST and DELETE endpoints for /firewall/ipset to
update or delete those references from cluster, hosts, and guests
firewall configs.

If these endpoints are hit from the cluster environment (/cluster/),
all firewall config files in the cluster (cluster + all nodes + all
guests) have to be checked and possibly updated.

Renaming a cluster ipset referenced by 10.000 different config files in
a 3-node test cluster takes about 4.8 seconds on my machine. Doing the
same with an unreferenced ipset (i.e. config files checked but not
written back due to lack of changes) takes about 2 seconds.

Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
---
 src/PVE/API2/Firewall/IPSet.pm | 108 ++++++++++++++++++++++++++++++++-
 1 file changed, 107 insertions(+), 1 deletion(-)

diff --git a/src/PVE/API2/Firewall/IPSet.pm b/src/PVE/API2/Firewall/IPSet.pm
index 43a51a7..9896fdd 100644
--- a/src/PVE/API2/Firewall/IPSet.pm
+++ b/src/PVE/API2/Firewall/IPSet.pm
@@ -5,6 +5,7 @@ use warnings;
 use PVE::Exception qw(raise raise_param_exc);
 use PVE::JSONSchema qw(get_standard_option);
 
+use PVE::API2::Firewall::Helpers qw(filter_map foreach_conf_in_env);
 use PVE::Firewall;
 
 use base qw(PVE::RESTHandler);
@@ -128,6 +129,92 @@ sub register_get_ipset {
     });
 }
 
+# Apply $rewrite to each rule in $conf referencing $ipset, modifying $conf in place.
+#
+# $rewrite ->($rule) is expected to return the new rule, or undef if the goal is to remove matches.
+#
+# Returns a boolean indicating whether $conf was modified.
+#
+# If $is_guest is set to 1, references will be solved assuming $conf is a guest FW config.
+# This is important because 2 different IPSets with the same name may be defined at the
+# cluster level and at the guest level, in which case the guest's IPSet shadows the cluster's.
+sub rewrite_ipset_refs_in_conf {
+    my ($conf, $ipset, $rule_env, $is_guest, $rewrite) = @_;
+    my $modified = 0;
+
+    my @patterns;
+    if ($rule_env eq 'cluster') {
+        @patterns = ("+dc/$ipset");
+        # Guest config ipsets may shadow cluster config ones
+        push @patterns, "+$ipset" if !($is_guest && $conf->{ipset}->{$ipset});
+    } else {
+        @patterns = ("+$ipset", "+guest/$ipset");
+    }
+
+    my $rule_matches = sub {
+        my ($rule) = @_;
+        grep { ($rule->{source} // '') eq $_ || ($rule->{dest} // '') eq $_ } @patterns;
+    };
+
+    ($conf->{rules}, my $changed) = filter_map($conf->{rules}, $rewrite, $rule_matches);
+    $modified ||= $changed;
+
+    if ($rule_env eq 'cluster') {
+        my $groups = $conf->{groups} // {};
+        for my $group (keys %$groups) {
+            ($groups->{$group}, $changed) = filter_map($groups->{$group}, $rewrite, $rule_matches);
+            $modified ||= $changed;
+        }
+    }
+
+    return $modified;
+}
+
+# Apply $rewrite to each rule in the current environment ($rule_env) referencing $ipset, modifying
+# $conf in place. The caller is responsible for locking and saving $conf.
+#
+# If $rule_env is 'cluster', this function will also map over all guest and node firewall configs
+# in the cluster, locking and saving them in the process, since those may reference IPSets defined
+# in the cluster config.
+sub rewrite_ipset_refs_in_env {
+    my ($conf, $ipset, $rule_env, $rewrite) = @_;
+    return foreach_conf_in_env(
+        $conf,
+        $rule_env,
+        sub {
+            my ($fw_conf, $effective_env, $is_guest) = @_;
+            return rewrite_ipset_refs_in_conf(
+                $fw_conf, $ipset, $effective_env, $is_guest, $rewrite,
+            );
+        },
+    );
+}
+
+sub delete_ipset_refs {
+    my ($conf, $ipset, $rule_env) = @_;
+    return rewrite_ipset_refs_in_env($conf, lc($ipset), $rule_env, sub { undef });
+}
+
+sub rename_ipset_refs {
+    my ($conf, $ipset, $new_name, $rule_env) = @_;
+    my $lc_ipset = lc($ipset);
+    return rewrite_ipset_refs_in_env(
+        $conf,
+        $lc_ipset,
+        $rule_env,
+        sub {
+            my ($rule) = @_;
+            for my $field (qw(source dest)) {
+                my $val = lc($rule->{$field} // '');
+                if ($val eq "+dc/$lc_ipset") { $rule->{$field} = "+dc/$new_name" }
+                elsif ($val eq "+$lc_ipset") { $rule->{$field} = "+$new_name" }
+                elsif ($val eq "+guest/$lc_ipset") { $rule->{$field} = "+guest/$new_name" }
+            }
+            return $rule;
+        },
+    );
+}
+
 sub register_delete_ipset {
     my ($class) = @_;
 
@@ -139,6 +226,12 @@ sub register_delete_ipset {
         optional => 1,
         description => 'Delete all members of the IPSet, if there are any.',
     };
+    $properties->{'delete-references'} = {
+        type => 'boolean',
+        optional => 1,
+        description => 'Delete dangling references after deleting the IPSet',
+        default => 0,
+    };
 
     $class->register_method({
         name => 'delete_ipset',
@@ -165,8 +258,10 @@ sub register_delete_ipset {
                     die "IPSet '$param->{name}' is not empty\n"
                         if scalar(@$ipset) && !$param->{force};
 
-                    $class->save_ipset($param, $fw_conf, undef);
+                    delete_ipset_refs($fw_conf, $param->{name}, $class->rule_env())
+                        if $param->{'delete-references'};
 
+                    $class->save_ipset($param, $fw_conf, undef);
                 },
             );
 
@@ -654,6 +749,13 @@ sub register_create {
         },
     );
 
+    $properties->{'update-references'} = {
+        type => 'boolean',
+        optional => 1,
+        description => 'Update dangling references when renaming an IPSet.',
+        default => 0,
+    };
+
     $class->register_method({
         name => 'create_ipset',
         path => '',
@@ -688,6 +790,10 @@ sub register_create {
                             if $fw_conf->{ipset}->{ $param->{name} }
                             && $param->{name} ne $param->{rename};
 
+                        PVE::API2::Firewall::IPSetBase::rename_ipset_refs(
+                            $fw_conf, $param->{rename}, $param->{name}, $class->rule_env(),
+                        ) if $param->{'update-references'};
+
                         my $data = delete $fw_conf->{ipset}->{ $param->{rename} };
                         $fw_conf->{ipset}->{ $param->{name} } = $data;
                         if (
-- 
2.47.3

Re: [PATCH pve-firewall 2/5] ipset: Add option to update references on rename/delete

[Stefan Hanreich]

On 4/7/26 9:36 AM, Arthur Bied-Charreton wrote:
> Renaming or deleting an IPSet that is referenced by rules or security
> groups would leave dangling references, creating broken configurations.
> 
> Add option to the POST and DELETE endpoints for /firewall/ipset to
> update or delete those references from cluster, hosts, and guests
> firewall configs.
> 
> If these endpoints are hit from the cluster environment (/cluster/),
> all firewall config files in the cluster (cluster + all nodes + all
> guests) have to be checked and possibly updated.
> 
> Renaming a cluster ipset referenced by 10.000 different config files in
> a 3-node test cluster takes about 4.8 seconds on my machine. Doing the
> same with an unreferenced ipset (i.e. config files checked but not
> written back due to lack of changes) takes about 2 seconds.
> 
> Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
> ---
>  src/PVE/API2/Firewall/IPSet.pm | 108 ++++++++++++++++++++++++++++++++-
>  1 file changed, 107 insertions(+), 1 deletion(-)
> 
> diff --git a/src/PVE/API2/Firewall/IPSet.pm b/src/PVE/API2/Firewall/IPSet.pm
> index 43a51a7..9896fdd 100644
> --- a/src/PVE/API2/Firewall/IPSet.pm
> +++ b/src/PVE/API2/Firewall/IPSet.pm
> @@ -5,6 +5,7 @@ use warnings;
>  use PVE::Exception qw(raise raise_param_exc);
>  use PVE::JSONSchema qw(get_standard_option);
>  
> +use PVE::API2::Firewall::Helpers qw(filter_map foreach_conf_in_env);
>  use PVE::Firewall;
>  
>  use base qw(PVE::RESTHandler);
> @@ -128,6 +129,92 @@ sub register_get_ipset {
>      });
>  }
>  
> +# Apply $rewrite to each rule in $conf referencing $ipset, modifying $conf in place.
> +#
> +# $rewrite ->($rule) is expected to return the new rule, or undef if the goal is to remove matches.
> +#
> +# Returns a boolean indicating whether $conf was modified.
> +#
> +# If $is_guest is set to 1, references will be solved assuming $conf is a guest FW config.
> +# This is important because 2 different IPSets with the same name may be defined at the
> +# cluster level and at the guest level, in which case the guest's IPSet shadows the cluster's.
> +sub rewrite_ipset_refs_in_conf {
> +    my ($conf, $ipset, $rule_env, $is_guest, $rewrite) = @_;
> +    my $modified = 0;
> +
> +    my @patterns;
> +    if ($rule_env eq 'cluster') {
> +        @patterns = ("+dc/$ipset");
> +        # Guest config ipsets may shadow cluster config ones
> +        push @patterns, "+$ipset" if !($is_guest && $conf->{ipset}->{$ipset});
> +    } else {
> +        @patterns = ("+$ipset", "+guest/$ipset");
> +    }
> +
> +    my $rule_matches = sub {
> +        my ($rule) = @_;
> +        grep { ($rule->{source} // '') eq $_ || ($rule->{dest} // '') eq $_ } @patterns;
> +    };
> +
> +    ($conf->{rules}, my $changed) = filter_map($conf->{rules}, $rewrite, $rule_matches);
> +    $modified ||= $changed;
> +
> +    if ($rule_env eq 'cluster') {
> +        my $groups = $conf->{groups} // {};
> +        for my $group (keys %$groups) {
> +            ($groups->{$group}, $changed) = filter_map($groups->{$group}, $rewrite, $rule_matches);
> +            $modified ||= $changed;
> +        }
> +    }
> +
> +    return $modified;
> +}
> +
> +# Apply $rewrite to each rule in the current environment ($rule_env) referencing $ipset, modifying
> +# $conf in place. The caller is responsible for locking and saving $conf.
> +#
> +# If $rule_env is 'cluster', this function will also map over all guest and node firewall configs
> +# in the cluster, locking and saving them in the process, since those may reference IPSets defined
> +# in the cluster config.
> +sub rewrite_ipset_refs_in_env {
> +    my ($conf, $ipset, $rule_env, $rewrite) = @_;
> +    return foreach_conf_in_env(
> +        $conf,
> +        $rule_env,
> +        sub {
> +            my ($fw_conf, $effective_env, $is_guest) = @_;
> +            return rewrite_ipset_refs_in_conf(
> +                $fw_conf, $ipset, $effective_env, $is_guest, $rewrite,
> +            );
> +        },
> +    );
> +}
> +
> +sub delete_ipset_refs {
> +    my ($conf, $ipset, $rule_env) = @_;
> +    return rewrite_ipset_refs_in_env($conf, lc($ipset), $rule_env, sub { undef });
> +}
> +
> +sub rename_ipset_refs {
> +    my ($conf, $ipset, $new_name, $rule_env) = @_;
> +    my $lc_ipset = lc($ipset);
> +    return rewrite_ipset_refs_in_env(
> +        $conf,
> +        $lc_ipset,
> +        $rule_env,
> +        sub {
> +            my ($rule) = @_;
> +            for my $field (qw(source dest)) {
> +                my $val = lc($rule->{$field} // '');
> +                if ($val eq "+dc/$lc_ipset") { $rule->{$field} = "+dc/$new_name" }
> +                elsif ($val eq "+$lc_ipset") { $rule->{$field} = "+$new_name" }
> +                elsif ($val eq "+guest/$lc_ipset") { $rule->{$field} = "+guest/$new_name" }

In the Firewall parsing logic itself (verify_rule in Firewall.pm), we do
use regex matching:

  $value =~ m@^\+(guest/|dc/|sdn/)?(${ipset_name_pattern})$@

We could utilize that to rewrite this as:

if ($value =~ m@^\+(guest/|dc/)?(${ipset_name_pattern})$@) {
  $rule->{$field} = "$1/$new_name" if $2 eq $lc_ipset;
}

but that might be a bit overkill / overcomplicated.

I just figured I'd mention it since i've often written similar if /
elsif chains myself that might have been better expressed by utilizing
perl regexes.

> +            }
> +            return $rule;
> +        },
> +    );
> +}
> +

All the helpers contained here, might better fit in the general Helper
module, rather than the API module itself. Particularly if we implement
similar functionality when deleting SDN VNets, pve-network would be able
to use the Helper module instead of having to import API modules.

>  sub register_delete_ipset {
>      my ($class) = @_;
>  
> @@ -139,6 +226,12 @@ sub register_delete_ipset {
>          optional => 1,
>          description => 'Delete all members of the IPSet, if there are any.',
>      };
> +    $properties->{'delete-references'} = {
> +        type => 'boolean',
> +        optional => 1,
> +        description => 'Delete dangling references after deleting the IPSet',
> +        default => 0,
> +    };
>  
>      $class->register_method({
>          name => 'delete_ipset',
> @@ -165,8 +258,10 @@ sub register_delete_ipset {
>                      die "IPSet '$param->{name}' is not empty\n"
>                          if scalar(@$ipset) && !$param->{force};
>  
> -                    $class->save_ipset($param, $fw_conf, undef);
> +                    delete_ipset_refs($fw_conf, $param->{name}, $class->rule_env())
> +                        if $param->{'delete-references'};
>  
> +                    $class->save_ipset($param, $fw_conf, undef);
>                  },
>              );
>  
> @@ -654,6 +749,13 @@ sub register_create {
>          },
>      );
>  
> +    $properties->{'update-references'} = {
> +        type => 'boolean',
> +        optional => 1,
> +        description => 'Update dangling references when renaming an IPSet.',
> +        default => 0,
> +    };
> +
>      $class->register_method({
>          name => 'create_ipset',
>          path => '',
> @@ -688,6 +790,10 @@ sub register_create {
>                              if $fw_conf->{ipset}->{ $param->{name} }
>                              && $param->{name} ne $param->{rename};
>  
> +                        PVE::API2::Firewall::IPSetBase::rename_ipset_refs(
> +                            $fw_conf, $param->{rename}, $param->{name}, $class->rule_env(),
> +                        ) if $param->{'update-references'};

This suffers from timing issues, doesn't it? We update the references in
all configuration files, but only save the new IPSet *after* we have
updated all references. This leaves a window where rules get skipped by
the firewall since they reference a not-yet-existing IPSet.

In order to get atomicity we'd have to do it in two steps, wouldn't we?
* Save the new ipset and write it to the configuration, but still keep
the old one around.
* update the references one-by-one, now at every point in time a rule
references an IPSet that actually exists in the configuration
* Delete the old IPSet

Otherwise, introducing a sleep inbetween the rename call and the
save_config call seems to make this surface easily:

* Create an IPSet at cluster level
* Reference the IPSet in a host rule
* Update the name of the IPSet + update-references

Some firewall cycles fail due to the rules getting updated, but not the
IPSet name itself:

> Jul 16 14:48:28 fw-testi pve-firewall[1213]: /etc/pve/nodes/fw-testi/host.fw (line 7) - errors in rule parameters: IN ACCEPT -source +dc/owo -log nolog
> Jul 16 14:48:28 fw-testi pve-firewall[1213]:   source: no such ipset 'owo'

Delete seems fine, since all references are deleted and only then the
IPSet itself afterwards.

[PATCH pve-firewall 3/5] aliases: Add option to update references on rename/delete

[Arthur Bied-Charreton]

Renaming or deleting an alias that is referenced by rules or security
groups would leave dangling references, creating broken configurations.

Add option to the POST and DELETE endpoints for /firewall/aliases to
update or delete those references from cluster, hosts, and guests
firewall configs.

If these endpoints are hit from the cluster environment (/cluster/),
all firewall config files in the cluster (cluster + all nodes + all
guests) have to be checked and possibly updated.

Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
---
 src/PVE/API2/Firewall/Aliases.pm | 115 +++++++++++++++++++++++++++++++
 1 file changed, 115 insertions(+)

diff --git a/src/PVE/API2/Firewall/Aliases.pm b/src/PVE/API2/Firewall/Aliases.pm
index eaafe68..2a06822 100644
--- a/src/PVE/API2/Firewall/Aliases.pm
+++ b/src/PVE/API2/Firewall/Aliases.pm
@@ -5,6 +5,7 @@ use warnings;
 use PVE::Exception qw(raise raise_param_exc);
 use PVE::JSONSchema qw(get_standard_option);
 
+use PVE::API2::Firewall::Helpers qw(filter_map foreach_conf_in_env);
 use PVE::Firewall;
 
 use base qw(PVE::RESTHandler);
@@ -217,6 +218,12 @@ sub register_update_alias {
     $properties->{cidr} = $api_properties->{cidr};
     $properties->{comment} = $api_properties->{comment};
     $properties->{digest} = get_standard_option('pve-config-digest');
+    $properties->{'update-references'} = {
+        type => 'boolean',
+        optional => 1,
+        description => 'Update dangling references when renaming the alias.',
+        default => 0,
+    };
 
     $class->register_method({
         name => 'update_alias',
@@ -261,6 +268,9 @@ sub register_update_alias {
                     if ($rename && ($name ne $rename)) {
                         raise_param_exc({ name => "alias '$param->{rename}' already exists" })
                             if defined($aliases->{$rename});
+                        rename_alias_refs(
+                            $fw_conf, $name, $param->{rename}, $class->rule_env(),
+                        ) if $param->{'update-references'};
                         $aliases->{$name}->{name} = $param->{rename};
                         $aliases->{$rename} = $aliases->{$name};
                         delete $aliases->{$name};
@@ -282,6 +292,12 @@ sub register_delete_alias {
 
     $properties->{name} = $api_properties->{name};
     $properties->{digest} = get_standard_option('pve-config-digest');
+    $properties->{'delete-references'} = {
+        type => 'boolean',
+        optional => 1,
+        description => 'Delete dangling references after deleting the alias.',
+        default => 0,
+    };
 
     $class->register_method({
         name => 'remove_alias',
@@ -310,6 +326,10 @@ sub register_delete_alias {
                     PVE::Tools::assert_if_modified($digest, $param->{digest});
 
                     my $name = lc($param->{name});
+
+                    delete_alias_refs($fw_conf, $param->{name}, $class->rule_env())
+                        if $param->{'delete-references'};
+
                     delete $aliases->{$name};
 
                     $class->save_aliases($param, $fw_conf, $aliases);
@@ -321,6 +341,101 @@ sub register_delete_alias {
     });
 }
 
+# Apply $rewrite to each rule or IPSet entry in $conf referencing $alias, modifying $conf in place.
+#
+# $rewrite->($obj) is expected to return the new rule/entry, or undef to remove it.
+#
+# Returns a boolean indicating whether $conf was modified.
+#
+# If $is_guest is set to 1, references will be solved assuming $conf is a guest FW config.
+# This is important because 2 different aliases with the same name may be defined at the
+# cluster level and at the guest level, in which case the guest's alias shadows the cluster's.
+sub rewrite_alias_refs_in_conf {
+    my ($conf, $alias, $rule_env, $is_guest, $rewrite) = @_;
+    my $lc_alias = lc($alias);
+    my $modified = 0;
+
+    my @patterns;
+    if ($rule_env eq 'cluster') {
+        @patterns = ("dc/$lc_alias");
+        push @patterns, $lc_alias if !($is_guest && $conf->{aliases}->{$lc_alias});
+    } else {
+        @patterns = ($lc_alias, "guest/$lc_alias");
+    }
+
+    my $rule_matches = sub {
+        my ($rule) = @_;
+        grep { lc($rule->{source} // '') eq $_ || lc($rule->{dest} // '') eq $_ } @patterns;
+    };
+    my $entry_matches = sub {
+        grep { lc($_[0]->{cidr} // '') eq $_ } @patterns;
+    };
+
+    ($conf->{rules}, my $changed) = filter_map($conf->{rules}, $rewrite, $rule_matches);
+    $modified ||= $changed;
+
+    my $ipsets = $conf->{ipset} // {};
+    for my $name (keys %$ipsets) {
+        ($ipsets->{$name}, $changed) = filter_map($ipsets->{$name}, $rewrite, $entry_matches);
+        $modified ||= $changed;
+    }
+
+    if ($rule_env eq 'cluster') {
+        my $groups = $conf->{groups} // {};
+        for my $group (keys %$groups) {
+            ($groups->{$group}, $changed) = filter_map($groups->{$group}, $rewrite, $rule_matches);
+            $modified ||= $changed;
+        }
+    }
+
+    return $modified;
+}
+
+# Apply $rewrite to each rule or IPSet entry in the current environment ($rule_env) referencing
+# $alias, modifying $conf in place. The caller is responsible for locking and saving $conf.
+#
+# If $rule_env is 'cluster', this function will also map over all guest and node firewall configs
+# in the cluster, locking and saving them in the process, since those may reference aliases defined
+# in the cluster config.
+sub rewrite_alias_refs_in_env {
+    my ($conf, $alias, $rule_env, $rewrite) = @_;
+    return foreach_conf_in_env(
+        $conf,
+        $rule_env,
+        sub {
+            my ($fw_conf, $effective_env, $is_guest) = @_;
+            return rewrite_alias_refs_in_conf(
+                $fw_conf, $alias, $effective_env, $is_guest, $rewrite,
+            );
+        },
+    );
+}
+
+sub delete_alias_refs {
+    my ($conf, $alias, $rule_env) = @_;
+    return rewrite_alias_refs_in_env($conf, $alias, $rule_env, sub { undef });
+}
+
+sub rename_alias_refs {
+    my ($conf, $alias, $new_name, $rule_env) = @_;
+    my $lc_alias = lc($alias);
+    return rewrite_alias_refs_in_env(
+        $conf,
+        $lc_alias,
+        $rule_env,
+        sub {
+            my ($obj) = @_;
+            for my $field (qw(source dest cidr)) {
+                my $val = lc($obj->{$field} // '');
+                if ($val eq "dc/$lc_alias") { $obj->{$field} = "dc/$new_name" }
+                elsif ($val eq $lc_alias) { $obj->{$field} = $new_name }
+                elsif ($val eq "guest/$lc_alias") { $obj->{$field} = "guest/$new_name" }
+            }
+            return $obj;
+        },
+    );
+}
+
 sub register_handlers {
     my ($class) = @_;
 
-- 
2.47.3

Re: [PATCH pve-firewall 3/5] aliases: Add option to update references on rename/delete

[Stefan Hanreich]

On 4/7/26 9:36 AM, Arthur Bied-Charreton wrote:
> Renaming or deleting an alias that is referenced by rules or security
> groups would leave dangling references, creating broken configurations.
> 
> Add option to the POST and DELETE endpoints for /firewall/aliases to
> update or delete those references from cluster, hosts, and guests
> firewall configs.
> 
> If these endpoints are hit from the cluster environment (/cluster/),
> all firewall config files in the cluster (cluster + all nodes + all
> guests) have to be checked and possibly updated.
> 
> Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
> ---
>  src/PVE/API2/Firewall/Aliases.pm | 115 +++++++++++++++++++++++++++++++
>  1 file changed, 115 insertions(+)
> 
> diff --git a/src/PVE/API2/Firewall/Aliases.pm b/src/PVE/API2/Firewall/Aliases.pm
> index eaafe68..2a06822 100644
> --- a/src/PVE/API2/Firewall/Aliases.pm
> +++ b/src/PVE/API2/Firewall/Aliases.pm
> @@ -5,6 +5,7 @@ use warnings;
>  use PVE::Exception qw(raise raise_param_exc);
>  use PVE::JSONSchema qw(get_standard_option);
>  
> +use PVE::API2::Firewall::Helpers qw(filter_map foreach_conf_in_env);
>  use PVE::Firewall;
>  
>  use base qw(PVE::RESTHandler);
> @@ -217,6 +218,12 @@ sub register_update_alias {
>      $properties->{cidr} = $api_properties->{cidr};
>      $properties->{comment} = $api_properties->{comment};
>      $properties->{digest} = get_standard_option('pve-config-digest');
> +    $properties->{'update-references'} = {
> +        type => 'boolean',
> +        optional => 1,
> +        description => 'Update dangling references when renaming the alias.',
> +        default => 0,
> +    };
>  
>      $class->register_method({
>          name => 'update_alias',
> @@ -261,6 +268,9 @@ sub register_update_alias {
>                      if ($rename && ($name ne $rename)) {
>                          raise_param_exc({ name => "alias '$param->{rename}' already exists" })
>                              if defined($aliases->{$rename});
> +                        rename_alias_refs(
> +                            $fw_conf, $name, $param->{rename}, $class->rule_env(),
> +                        ) if $param->{'update-references'};

This suffers from the same timing issues as the IPSet renaming.

>                          $aliases->{$name}->{name} = $param->{rename};
>                          $aliases->{$rename} = $aliases->{$name};
>                          delete $aliases->{$name};
> @@ -282,6 +292,12 @@ sub register_delete_alias {
>  
>      $properties->{name} = $api_properties->{name};
>      $properties->{digest} = get_standard_option('pve-config-digest');
> +    $properties->{'delete-references'} = {
> +        type => 'boolean',
> +        optional => 1,
> +        description => 'Delete dangling references after deleting the alias.',
> +        default => 0,
> +    };
>  
>      $class->register_method({
>          name => 'remove_alias',
> @@ -310,6 +326,10 @@ sub register_delete_alias {
>                      PVE::Tools::assert_if_modified($digest, $param->{digest});
>  
>                      my $name = lc($param->{name});
> +
> +                    delete_alias_refs($fw_conf, $param->{name}, $class->rule_env())
> +                        if $param->{'delete-references'};
> +
>                      delete $aliases->{$name};
>  
>                      $class->save_aliases($param, $fw_conf, $aliases);
> @@ -321,6 +341,101 @@ sub register_delete_alias {
>      });
>  }
>  
> +# Apply $rewrite to each rule or IPSet entry in $conf referencing $alias, modifying $conf in place.
> +#
> +# $rewrite->($obj) is expected to return the new rule/entry, or undef to remove it.
> +#
> +# Returns a boolean indicating whether $conf was modified.
> +#
> +# If $is_guest is set to 1, references will be solved assuming $conf is a guest FW config.
> +# This is important because 2 different aliases with the same name may be defined at the
> +# cluster level and at the guest level, in which case the guest's alias shadows the cluster's.
> +sub rewrite_alias_refs_in_conf {
> +    my ($conf, $alias, $rule_env, $is_guest, $rewrite) = @_;
> +    my $lc_alias = lc($alias);
> +    my $modified = 0;
> +
> +    my @patterns;
> +    if ($rule_env eq 'cluster') {
> +        @patterns = ("dc/$lc_alias");
> +        push @patterns, $lc_alias if !($is_guest && $conf->{aliases}->{$lc_alias});
> +    } else {
> +        @patterns = ($lc_alias, "guest/$lc_alias");
> +    }
> +
> +    my $rule_matches = sub {
> +        my ($rule) = @_;
> +        grep { lc($rule->{source} // '') eq $_ || lc($rule->{dest} // '') eq $_ } @patterns;
> +    };
> +    my $entry_matches = sub {
> +        grep { lc($_[0]->{cidr} // '') eq $_ } @patterns;
> +    };
> +
> +    ($conf->{rules}, my $changed) = filter_map($conf->{rules}, $rewrite, $rule_matches);
> +    $modified ||= $changed;
> +
> +    my $ipsets = $conf->{ipset} // {};
> +    for my $name (keys %$ipsets) {
> +        ($ipsets->{$name}, $changed) = filter_map($ipsets->{$name}, $rewrite, $entry_matches);
> +        $modified ||= $changed;
> +    }
> +
> +    if ($rule_env eq 'cluster') {
> +        my $groups = $conf->{groups} // {};
> +        for my $group (keys %$groups) {
> +            ($groups->{$group}, $changed) = filter_map($groups->{$group}, $rewrite, $rule_matches);
> +            $modified ||= $changed;
> +        }
> +    }
> +
> +    return $modified;
> +}
> +
> +# Apply $rewrite to each rule or IPSet entry in the current environment ($rule_env) referencing
> +# $alias, modifying $conf in place. The caller is responsible for locking and saving $conf.
> +#
> +# If $rule_env is 'cluster', this function will also map over all guest and node firewall configs
> +# in the cluster, locking and saving them in the process, since those may reference aliases defined
> +# in the cluster config.
> +sub rewrite_alias_refs_in_env {
> +    my ($conf, $alias, $rule_env, $rewrite) = @_;
> +    return foreach_conf_in_env(
> +        $conf,
> +        $rule_env,
> +        sub {
> +            my ($fw_conf, $effective_env, $is_guest) = @_;
> +            return rewrite_alias_refs_in_conf(
> +                $fw_conf, $alias, $effective_env, $is_guest, $rewrite,
> +            );
> +        },
> +    );
> +}
> +
> +sub delete_alias_refs {
> +    my ($conf, $alias, $rule_env) = @_;
> +    return rewrite_alias_refs_in_env($conf, $alias, $rule_env, sub { undef });
> +}
> +
> +sub rename_alias_refs {
> +    my ($conf, $alias, $new_name, $rule_env) = @_;
> +    my $lc_alias = lc($alias);
> +    return rewrite_alias_refs_in_env(
> +        $conf,
> +        $lc_alias,
> +        $rule_env,
> +        sub {
> +            my ($obj) = @_;
> +            for my $field (qw(source dest cidr)) {
> +                my $val = lc($obj->{$field} // '');
> +                if ($val eq "dc/$lc_alias") { $obj->{$field} = "dc/$new_name" }
> +                elsif ($val eq $lc_alias) { $obj->{$field} = $new_name }
> +                elsif ($val eq "guest/$lc_alias") { $obj->{$field} = "guest/$new_name" }
> +            }
> +            return $obj;
> +        },
> +    );
> +}
> +

same point w.r.t moving them to the Helper module. Also, this seems
quite similar to the respective IPSet function (minus the name parsing).
Would it be possible to further simplify this and re-use the functions
across the ipset / alias functions?

[PATCH pve-manager 4/5] ipset: Add option to update references on rename/delete

[Arthur Bied-Charreton]

Add checkboxes to the ipset edit & remove dialogs which allow opting
into updating/deleting all references that would be left dangling
otherwise.

This adds a remove button with a custom IPSetConfirmRemoveDialog built
on top of ConfirmRemoveDialog to allow setting the extra URL parameter
in the DELETE request, which StdRemoveButton does not support out of the
box.

Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
---
 www/manager6/panel/IPSet.js | 60 +++++++++++++++++++++++++++++++++++--
 1 file changed, 57 insertions(+), 3 deletions(-)

diff --git a/www/manager6/panel/IPSet.js b/www/manager6/panel/IPSet.js
index 9e0203a0..3db86c58 100644
--- a/www/manager6/panel/IPSet.js
+++ b/www/manager6/panel/IPSet.js
@@ -1,3 +1,32 @@
+Ext.define('PVE.window.IPSetConfirmRemoveDialog', {
+    extend: 'Proxmox.window.ConfirmRemoveDialog',
+    alias: 'widget.pveIPSetConfirmRemoveDialog',
+    width: 350,
+
+    additionalItems: [
+        {
+            xtype: 'proxmoxcheckbox',
+            reference: 'deleteRefsCheckbox',
+            fieldLabel: gettext('Delete referencing rules'),
+            labelWidth: 260,
+            autoEl: {
+                tag: 'span',
+                'data-qtip': gettext(
+                    'Delete all rules referencing this IPSet. If left unchecked, some firewall configurations may end up in invalid states.',
+                ),
+            },
+        },
+    ],
+
+    getParams: function () {
+        let me = this;
+        if (me.lookupReference('deleteRefsCheckbox').getValue()) {
+            me.params['delete-references'] = 1;
+        }
+        return me.callParent();
+    },
+});
+
 Ext.define('pve-fw-ipsets', {
     extend: 'Ext.data.Model',
     fields: ['name', 'comment', 'digest'],
@@ -91,6 +120,19 @@ Ext.define('PVE.IPSetList', {
                         value: rec.data.comment,
                         fieldLabel: gettext('Comment'),
                     },
+                    {
+                        xtype: 'proxmoxcheckbox',
+                        name: 'update-references',
+                        fieldLabel: gettext('Update referencing rules'),
+                        labelWidth: 250,
+                        anchor: '100%',
+                        autoEl: {
+                            tag: 'span',
+                            'data-qtip': gettext(
+                                'Update all rules referencing this IPSet to use the new identifier. If left unchecked, some firewall configurations may end up in invalid states.',
+                            ),
+                        },
+                    },
                 ],
             });
             win.show();
@@ -134,11 +176,23 @@ 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.IPSetConfirmRemoveDialog', {
+                    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,
+                    ),
+                    apiCallDone: reload,
+                }).show();
+            },
         });
 
         Ext.apply(me, {
-- 
2.47.3

[PATCH pve-manager 5/5] aliases: Add option to update references on rename/delete

[Arthur Bied-Charreton]

Add checkboxes to the alias edit & remove dialogs which allow opting
into updating/deleting all references that would be left dangling
otherwise.

This adds a remove button with a custom AliasConfirmRemoveDialog built
on top of ConfirmRemoveDialog to allow setting the extra URL parameter
in the DELETE request, which StdRemoveButton does not support out of the
box.

Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
---
 www/manager6/grid/FirewallAliases.js | 101 +++++++++++++++++++++------
 1 file changed, 79 insertions(+), 22 deletions(-)

diff --git a/www/manager6/grid/FirewallAliases.js b/www/manager6/grid/FirewallAliases.js
index 06801d33..85cfea89 100644
--- a/www/manager6/grid/FirewallAliases.js
+++ b/www/manager6/grid/FirewallAliases.js
@@ -1,3 +1,32 @@
+Ext.define('PVE.window.AliasConfirmRemoveDialog', {
+    extend: 'Proxmox.window.ConfirmRemoveDialog',
+    alias: 'widget.pveAliasConfirmRemoveDialog',
+    width: 350,
+
+    additionalItems: [
+        {
+            xtype: 'proxmoxcheckbox',
+            reference: 'deleteRefsCheckbox',
+            fieldLabel: gettext('Delete referencing rules and IPSets'),
+            labelWidth: 260,
+            autoEl: {
+                tag: 'span',
+                'data-qtip': gettext(
+                    'Delete all rules and IPSets referencing this alias. If left unchecked, some firewall configurations may end up in invalid states.',
+                ),
+            },
+        },
+    ],
+
+    getParams: function () {
+        let me = this;
+        if (me.lookupReference('deleteRefsCheckbox').getValue()) {
+            me.params['delete-references'] = 1;
+        }
+        return me.callParent();
+    },
+});
+
 Ext.define('PVE.FirewallAliasEdit', {
     extend: 'Proxmox.window.Edit',
 
@@ -20,27 +49,44 @@ 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'),
+            },
+        ];
+        if (!me.isCreate) {
+            items.push({
+                xtype: 'proxmoxcheckbox',
+                name: 'update-references',
+                fieldLabel: gettext('Update referencing rules and IPSets'),
+                labelWidth: 350,
+                anchor: '100%',
+                autoEl: {
+                    tag: 'span',
+                    'data-qtip': gettext(
+                        'Update all rules and IPSets referencing this alias to use the new identifier. If left unchecked, some firewall configurations may end up in invalid states.',
+                    ),
+                },
+            });
+        }
+
         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,
         });
 
         Ext.apply(me, {
@@ -158,15 +204,26 @@ 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.AliasConfirmRemoveDialog', {
+                    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,
+                    ),
+                    apiCallDone: reload,
+                }).show();
+            },
         });
 
         Ext.apply(me, {
-- 
2.47.3

Re: [PATCH firewall/manager 0/5] Allow updating references to firewall objects when editing them

[Stefan Hanreich]

Thanks for tackling this! I've added some comments on the individual
patches and did some testing

An idea for an additional feature I noticed while testing: We
auto-generate IPSets for SDN objects. It might make sense to add similar
functionality when deleting SDN objects, so there are no dangling IPSet
references after deleting an SDN object. This could be done in a v2, or
even in a completely separate patch series.


On 4/7/26 9:36 AM, Arthur Bied-Charreton wrote:
> Renaming/deleting ipsets or aliases that are referenced by other rules,
> security groups, or ipsets currently leaves dangling references behind.
> This creates broken firewall configurations where the rules with those
> dangling references fail to parse, meaning they are ignored by the
> firewall.
> 
> This is especially dangerous in the renaming case, where a user might
> reasonably expect references to update automatically, meaning they may
> go about their day after renaming an object without realizing a whole
> set of firewall rules is now inactive.
> 
> This patch series addresses this by adding optional parameters to the
> related firewall endpoints that allow updating/deleting all references
> to an object when editing/removing it. The UI patches add checkboxes to
> the edit/remove windows to optionally pass this parameter.
> 
> The casing in the firewall API is inconsistent: ipset names are stored
> as-is in the SectionConfig (e.g. 'FOO'), but lowercased when the config
> is loaded. This means a rule referencing '+dc/FOO' will fail to match
> its own definition after a load. In practice this is self-correcting:
> lowercasing is applied in-place, so definitions are written back in
> lowercase on the next read-write cycle, and the API does not allow
> creating rules with references that do not match anything. There is
> however a window between object creation and that first cycle where
> references can be broken. To avoid false negatives in that window,
> this series uses case-insensitive matching when scanning for
> references, and renamed references are always written back lowercased.
> 
> Stefan and I talked about this off-list and came to the conclusion that
> enforcing either casing may break existing configs that have been
> manually edited, so a more general fix would be better folded into a
> major release.
> 
> The UI patches introduce some code duplication. I tried to generalize
> the remove button with an optional extra URL parameter, but I did not
> find a good name for it, nor did I find a nice common ground that did
> not involve making the call sites even more verbose with configuration
> options, which is why I ended up leaving it like this. I am of course
> open to suggestions.
> 
> The original plan was to have an upfront check for would-be-dangling
> references before allowing any edit to an object, however this can be a
> very expensive operation, since an object defined at the cluster level
> may be referenced by guest and host configurations in the whole cluster.
> A check like this would trigger an unconditional scan of all those
> configs when clicking the remove buttons, which is why it is kept
> opt-in in the UI.
> 
> 
> pve-firewall:
> 
> Arthur Bied-Charreton (3):
>   Add helpers for updating alias and ipset references
>   ipset: Add option to update references on rename/delete
>   aliases: Add option to update references on rename/delete
> 
>  src/PVE/API2/Firewall/Aliases.pm | 115 +++++++++++++++++++++++++++++++
>  src/PVE/API2/Firewall/Helpers.pm |  66 ++++++++++++++++++
>  src/PVE/API2/Firewall/IPSet.pm   | 108 ++++++++++++++++++++++++++++-
>  3 files changed, 288 insertions(+), 1 deletion(-)
> 
> 
> pve-manager:
> 
> Arthur Bied-Charreton (2):
>   ipset: Add option to update references on rename/delete
>   aliases: Add option to update references on rename/delete
> 
>  www/manager6/grid/FirewallAliases.js | 101 +++++++++++++++++++++------
>  www/manager6/panel/IPSet.js          |  60 +++++++++++++++-
>  2 files changed, 136 insertions(+), 25 deletions(-)
> 
> 
> Summary over all repositories:
>   5 files changed, 424 insertions(+), 26 deletions(-)
>