[PATCH pve-firewall v6 07/28] firewall: ipset: add option to handle dangling references on rename

[Arthur Bied-Charreton <a.bied-charreton@proxmox.com>]

Renaming an ipset still referenced by rules leaves dangling references.
The firewall then fails to parse those rules during compilation and
drops them. A rename can therefore effectively disable a whole set of
rules.

Add a 'dangling-references' option to the rename path offering to
'keep' the references around, 'disable' the referencing rules, 'drop'
them, or to 'rename' the references to the new name. For cluster
ipsets, this also covers references in downstream configs (host, guest
and vnet).

A rename that fails partway leaves both the old and the new ipset in
the cluster config. Retrying it chokes on the check against
overwriting an existing ipset. 'force-rename' bypasses that check, so
an interrupted rename can be finished. Avoiding the half-updated state
altogether would need either a transaction across every config in the
cluster or a rollback mechanism, neither of which is really an option
here.

Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
---
 src/PVE/API2/Firewall/IPSet.pm | 66 +++++++++++++++++++++++++++-------
 src/PVE/Firewall.pm            | 24 +++++++++++++
 2 files changed, 78 insertions(+), 12 deletions(-)

diff --git a/src/PVE/API2/Firewall/IPSet.pm b/src/PVE/API2/Firewall/IPSet.pm
index 82b9aaf..9d3c562 100644
--- a/src/PVE/API2/Firewall/IPSet.pm
+++ b/src/PVE/API2/Firewall/IPSet.pm
@@ -534,6 +534,7 @@ package PVE::API2::Firewall::BaseIPSetList;
 
 use strict;
 use warnings;
+use PVE::Firewall::References qw(update_refs);
 use PVE::JSONSchema qw(get_standard_option);
 use PVE::Exception qw(raise_param_exc);
 use PVE::Firewall;
@@ -661,6 +662,9 @@ sub register_create {
         },
     );
 
+    $properties->{'dangling-references'} = get_standard_option('pve-dangling-references-rename');
+    $properties->{'force-rename'} = get_standard_option('pve-dangling-references-force-rename');
+
     $class->register_method({
         name => 'create_ipset',
         path => '',
@@ -681,6 +685,11 @@ sub register_create {
                 sub {
                     my ($param) = @_;
 
+                    my $dangling_refs = $param->{'dangling-references'}
+                        // $properties->{'dangling-references'}->{default};
+                    my $force = $param->{'force-rename'}
+                        // $properties->{'force-rename'}->{default};
+
                     my ($cluster_conf, $fw_conf) = $class->load_config($param);
 
                     if ($param->{rename}) {
@@ -690,19 +699,52 @@ sub register_create {
                         raise_param_exc({ name => "IPSet '$param->{rename}' does not exist" })
                             if !$fw_conf->{ipset}->{ $param->{rename} };
 
-                        # prevent overwriting existing ipset
-                        raise_param_exc({ name => "IPSet '$param->{name}' does already exist" })
-                            if $fw_conf->{ipset}->{ $param->{name} }
-                            && $param->{name} ne $param->{rename};
-
-                        my $data = delete $fw_conf->{ipset}->{ $param->{rename} };
-                        $fw_conf->{ipset}->{ $param->{name} } = $data;
-                        if (
-                            my $comment =
-                            delete $fw_conf->{ipset_comments}->{ $param->{rename} }
-                        ) {
-                            $fw_conf->{ipset_comments}->{ $param->{name} } = $comment;
+                        if ($param->{name} ne $param->{rename}) {
+                            # prevent overwriting existing ipset
+                            raise_param_exc({ name => "IPSet '$param->{name}' already exists" })
+                                if $fw_conf->{ipset}->{ $param->{name} }
+                                && !$force;
+
+                            $fw_conf->{ipset}->{ $param->{name} } =
+                                $fw_conf->{ipset}->{ $param->{rename} };
+
+                            my $saved = 0;
+                            my $save = sub {
+                                $class->save_config($param, $fw_conf);
+                                $saved = 1;
+                            };
+
+                            eval {
+                                update_refs({
+                                    conf => $fw_conf,
+                                    kind => 'ipset',
+                                    old => $param->{rename},
+                                    new => $param->{name},
+                                    env => $class->rule_env(),
+                                    action => $dangling_refs eq 'keep' ? undef : $dangling_refs,
+                                    subject =>
+                                        "ipset '$param->{rename}' renamed to '$param->{name}'",
+                                    save => $save,
+                                });
+                            };
+                            if (my $err = $@) {
+                                die "rename interrupted, the IPSet exists under both names; "
+                                    . "retry with 'force-rename' to finish: $err"
+                                    if $saved;
+                                die "could not update references to IPSet "
+                                    . "'$param->{rename}': $err";
+                            }
+
+                            delete $fw_conf->{ipset}->{ $param->{rename} };
+
+                            if (
+                                my $comment =
+                                delete $fw_conf->{ipset_comments}->{ $param->{rename} }
+                            ) {
+                                $fw_conf->{ipset_comments}->{ $param->{name} } = $comment;
+                            }
                         }
+
                         $fw_conf->{ipset_comments}->{ $param->{name} } = $param->{comment}
                             if defined($param->{comment});
                     } else {
diff --git a/src/PVE/Firewall.pm b/src/PVE/Firewall.pm
index 6ce5306..d4e1ad9 100644
--- a/src/PVE/Firewall.pm
+++ b/src/PVE/Firewall.pm
@@ -210,6 +210,30 @@ PVE::JSONSchema::register_standard_option(
     },
 );
 
+PVE::JSONSchema::register_standard_option(
+    'pve-dangling-references-rename',
+    {
+        type => 'string',
+        enum => ['keep', 'disable', 'drop', 'rename'],
+        optional => 1,
+        description => "Handle references that renaming this object would leave dangling. Use "
+            . "'rename' to point them at the new name, 'disable' to disable the referencing rules, or "
+            . "'drop' to remove them entirely.",
+        default => 'keep',
+    },
+);
+
+PVE::JSONSchema::register_standard_option(
+    'pve-dangling-references-force-rename',
+    {
+        type => 'boolean',
+        optional => 1,
+        description => "Go on with the rename even if an object with the target name already "
+            . "exists, overwriting it. 'dangling-references' is still applied the same.",
+        default => 0,
+    },
+);
+
 my $feature_ipset_nomatch = 0;
 eval {
     my (undef, undef, $release) = POSIX::uname();
-- 
2.47.3