[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

[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

[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

[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