[PATCH pve-firewall v6 11/28] firewall: api: run reference updates as tasks

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

A 'dangling-references' action other than 'keep' rewrites the rules that
referenced the object, which for a cluster-scoped object means going
through all guest, host and vnet firewall configs in the cluster,
potentially updating each of them.

The update_refs() helper logs each change made to each config. Fork a
worker when the request passes 'dangling-references' so the log lands
in the task log, which is visible cluster-wide, instead of the syslog.
A request without the parameter, i.e. what every existing client sends,
keeps running synchronously and returning null for backwards
compatibility.

Checks that can be performed without holding the lock are made before
forking so a rejected request can still fail right away instead of
requiring the caller to go read the task.

Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
---
 src/PVE/API2/Firewall/Aliases.pm | 233 ++++++++++++++++++------------
 src/PVE/API2/Firewall/IPSet.pm   | 238 ++++++++++++++++++-------------
 src/PVE/Firewall/References.pm   |  20 +++
 3 files changed, 299 insertions(+), 192 deletions(-)

diff --git a/src/PVE/API2/Firewall/Aliases.pm b/src/PVE/API2/Firewall/Aliases.pm
index cdbe68c..301ca88 100644
--- a/src/PVE/API2/Firewall/Aliases.pm
+++ b/src/PVE/API2/Firewall/Aliases.pm
@@ -3,7 +3,7 @@ package PVE::API2::Firewall::AliasesBase;
 use strict;
 use warnings;
 
-use PVE::Firewall::References qw(update_refs);
+use PVE::Firewall::References qw(update_refs run_ref_update);
 use PVE::Exception qw(raise raise_param_exc);
 use PVE::JSONSchema qw(get_standard_option);
 
@@ -84,6 +84,44 @@ my $aliases_to_list = sub {
     return $list;
 };
 
+my $check_digest = sub {
+    my ($aliases, $param) = @_;
+
+    my $list = &$aliases_to_list($aliases);
+    my (undef, $digest) = PVE::Firewall::copy_list_with_digest($list);
+
+    PVE::Tools::assert_if_modified($digest, $param->{digest});
+};
+
+my $check_update = sub {
+    my ($aliases, $param, $force) = @_;
+
+    $check_digest->($aliases, $param);
+
+    my $name = lc($param->{name});
+    raise_param_exc({ name => "no such alias" }) if !$aliases->{$name};
+
+    my $rename = $param->{rename};
+    $rename = lc($rename) if $rename;
+
+    if (
+        $rename
+        && $name ne $rename
+        && defined($aliases->{$rename})
+        && !$force
+    ) {
+        raise_param_exc({ name => "alias '$param->{rename}' already exists" });
+    }
+};
+
+my $check_delete = sub {
+    my ($aliases, $param) = @_;
+
+    $check_digest->($aliases, $param);
+
+    raise_param_exc({ name => "no such alias" }) if !$aliases->{ lc($param->{name}) };
+};
+
 sub register_get_aliases {
     my ($class) = @_;
 
@@ -220,7 +258,6 @@ sub register_update_alias {
     $properties->{cidr} = { $api_properties->{cidr}->%*, optional => 1 };
     $properties->{comment} = $api_properties->{comment};
     $properties->{digest} = get_standard_option('pve-config-digest');
-
     $properties->{'dangling-references'} = get_standard_option('pve-dangling-references-rename');
     $properties->{'force-rename'} = get_standard_option('pve-dangling-references-force-rename');
 
@@ -235,85 +272,88 @@ sub register_update_alias {
             additionalProperties => 0,
             properties => $properties,
         },
-        returns => { type => "null" },
+        returns => { type => 'string', optional => 1 },
         code => sub {
             my ($param) = @_;
 
-            $class->lock_config(
-                $param,
-                sub {
-                    my ($param) = @_;
-
-                    my $dangling_refs = $param->{'dangling-references'}
-                        // $properties->{'dangling-references'}->{default};
-                    my $force = $param->{'force-rename'}
-                        // $properties->{'force-rename'}->{default};
-
-                    my ($fw_conf, $aliases) = $class->load_config($param);
-
-                    my $list = &$aliases_to_list($aliases);
-
-                    my (undef, $digest) = PVE::Firewall::copy_list_with_digest($list);
-
-                    PVE::Tools::assert_if_modified($digest, $param->{digest});
-
-                    my $name = lc($param->{name});
-
-                    raise_param_exc({ name => "no such alias" }) if !$aliases->{$name};
+            my $dangling_refs = $param->{'dangling-references'}
+                // $properties->{'dangling-references'}->{default};
+            my $force = $param->{'force-rename'} // $properties->{'force-rename'}->{default};
 
-                    my $data = {
-                        name => $param->{name},
-                        cidr => $param->{cidr} // $aliases->{$name}->{cidr},
-                        comment => $param->{comment} // $aliases->{$name}->{comment},
-                    };
+            my $renaming = $param->{rename} && lc($param->{name}) ne lc($param->{rename});
 
-                    $aliases->{$name} = $data;
+            my $code = sub {
+                $class->lock_config(
+                    $param,
+                    sub {
+                        my ($param) = @_;
 
-                    my $rename = $param->{rename};
-                    $rename = lc($rename) if $rename;
+                        my ($fw_conf, $aliases) = $class->load_config($param);
 
-                    if ($rename && ($name ne $rename)) {
-                        raise_param_exc({ name => "alias '$param->{rename}' already exists" })
-                            if defined($aliases->{$rename})
-                            && !$force;
+                        $check_update->($aliases, $param, $force);
 
-                        $aliases->{$rename} =
-                            { $aliases->{$name}->%*, name => $param->{rename} };
+                        my $name = lc($param->{name});
 
-                        my $saved = 0;
-                        my $save = sub {
-                            $class->save_aliases($param, $fw_conf, $aliases);
-                            $saved = 1;
+                        my $data = {
+                            name => $param->{name},
+                            cidr => $param->{cidr} // $aliases->{$name}->{cidr},
+                            comment => $param->{comment} // $aliases->{$name}->{comment},
                         };
 
-                        eval {
-                            update_refs({
-                                conf => $fw_conf,
-                                kind => 'aliases',
-                                old => $name,
-                                new => $param->{rename},
-                                env => $class->rule_env(),
-                                action => $dangling_refs eq 'keep' ? undef : $dangling_refs,
-                                subject =>
-                                    "alias '$param->{name}' renamed to '$param->{rename}'",
-                                save => $save,
-                            });
-                        };
-                        if (my $err = $@) {
-                            die "rename interrupted, the alias exists under both names; "
-                                . "retry with 'force-rename' to finish: $err"
-                                if $saved;
-                            die "could not update references to alias '$name': $err";
+                        $aliases->{$name} = $data;
+
+                        my $rename = $param->{rename};
+                        $rename = lc($rename) if $rename;
+
+                        if ($rename && ($name ne $rename)) {
+                            $aliases->{$rename} =
+                                { $aliases->{$name}->%*, name => $param->{rename} };
+
+                            my $saved = 0;
+                            my $save = sub {
+                                $class->save_aliases($param, $fw_conf, $aliases);
+                                $saved = 1;
+                            };
+
+                            eval {
+                                update_refs({
+                                    conf => $fw_conf,
+                                    kind => 'aliases',
+                                    old => $name,
+                                    new => $param->{rename},
+                                    env => $class->rule_env(),
+                                    action => $dangling_refs eq 'keep' ? undef : $dangling_refs,
+                                    subject =>
+                                        "alias '$param->{name}' renamed to '$param->{rename}'",
+                                    save => $save,
+                                });
+                            };
+                            if (my $err = $@) {
+                                die "rename interrupted, the alias exists under both names; "
+                                    . "retry with 'force-rename' to finish: $err"
+                                    if $saved;
+                                die "could not update references to alias '$name': $err";
+                            }
+
+                            delete $aliases->{$name};
                         }
 
-                        delete $aliases->{$name};
-                    }
+                        $class->save_aliases($param, $fw_conf, $aliases);
+                    },
+                );
+            };
 
-                    $class->save_aliases($param, $fw_conf, $aliases);
-                },
-            );
+            if (!$renaming || !defined($param->{'dangling-references'})) {
+                $code->();
+                return undef;
+            }
 
-            return undef;
+            # fail on the request instead of inside the task.
+            my (undef, $aliases) = $class->load_config($param);
+
+            $check_update->($aliases, $param, $force);
+
+            return run_ref_update('fw-alias-rename', $param->{vmid} // $param->{name}, $code);
         },
     });
 }
@@ -338,43 +378,52 @@ sub register_delete_alias {
             additionalProperties => 0,
             properties => $properties,
         },
-        returns => { type => "null" },
+        returns => { type => 'string', optional => 1 },
         code => sub {
             my ($param) = @_;
 
-            $class->lock_config(
-                $param,
-                sub {
-                    my ($param) = @_;
+            my $action = $param->{'dangling-references'}
+                // $properties->{'dangling-references'}->{default};
 
-                    my ($fw_conf, $aliases) = $class->load_config($param);
+            my $code = sub {
+                $class->lock_config(
+                    $param,
+                    sub {
+                        my ($param) = @_;
 
-                    my $list = &$aliases_to_list($aliases);
-                    my (undef, $digest) = PVE::Firewall::copy_list_with_digest($list);
-                    PVE::Tools::assert_if_modified($digest, $param->{digest});
+                        my ($fw_conf, $aliases) = $class->load_config($param);
 
-                    my $name = lc($param->{name});
-                    raise_param_exc({ name => "no such alias" }) if !$aliases->{$name};
+                        $check_delete->($aliases, $param);
 
-                    my $action = $param->{'dangling-references'}
-                        // $properties->{'dangling-references'}->{default};
+                        my $name = lc($param->{name});
 
-                    update_refs({
-                        conf => $fw_conf,
-                        kind => 'aliases',
-                        old => $param->{name},
-                        env => $class->rule_env(),
-                        action => $action eq 'keep' ? undef : $action,
-                        subject => "alias '$param->{name}' deleted",
-                    });
+                        update_refs({
+                            conf => $fw_conf,
+                            kind => 'aliases',
+                            old => $param->{name},
+                            env => $class->rule_env(),
+                            action => $action eq 'keep' ? undef : $action,
+                            subject => "alias '$param->{name}' deleted",
+                        });
 
-                    delete $aliases->{$name};
+                        delete $aliases->{$name};
 
-                    $class->save_aliases($param, $fw_conf, $aliases);
-                },
-            );
+                        $class->save_aliases($param, $fw_conf, $aliases);
+                    },
+                );
+            };
 
-            return undef;
+            if (!defined($param->{'dangling-references'})) {
+                $code->();
+                return undef;
+            }
+
+            # fail on the request instead of inside the task
+            my (undef, $aliases) = $class->load_config($param);
+
+            $check_delete->($aliases, $param);
+
+            return run_ref_update('fw-alias-delete', $param->{vmid} // $param->{name}, $code);
         },
     });
 }
diff --git a/src/PVE/API2/Firewall/IPSet.pm b/src/PVE/API2/Firewall/IPSet.pm
index 144baf6..05be0fe 100644
--- a/src/PVE/API2/Firewall/IPSet.pm
+++ b/src/PVE/API2/Firewall/IPSet.pm
@@ -2,7 +2,7 @@ package PVE::API2::Firewall::IPSetBase;
 
 use strict;
 use warnings;
-use PVE::Firewall::References qw(update_refs);
+use PVE::Firewall::References qw(update_refs run_ref_update);
 use PVE::Exception qw(raise raise_param_exc);
 use PVE::JSONSchema qw(get_standard_option);
 
@@ -154,38 +154,50 @@ sub register_delete_ipset {
             additionalProperties => 0,
             properties => $properties,
         },
-        returns => { type => 'null' },
+        returns => { type => 'string', optional => 1 },
         code => sub {
             my ($param) = @_;
 
-            $class->lock_config(
-                $param,
-                sub {
-                    my ($param) = @_;
+            my $action = $param->{'dangling-references'}
+                // $properties->{'dangling-references'}->{default};
 
-                    my ($cluster_conf, $fw_conf, $ipset) = $class->load_config($param);
+            my $code = sub {
+                $class->lock_config(
+                    $param,
+                    sub {
+                        my ($param) = @_;
 
-                    die "IPSet '$param->{name}' is not empty\n"
-                        if scalar(@$ipset) && !$param->{force};
+                        my ($cluster_conf, $fw_conf, $ipset) = $class->load_config($param);
 
-                    my $action = $param->{'dangling-references'}
-                        // $properties->{'dangling-references'}->{default};
+                        die "IPSet '$param->{name}' is not empty\n"
+                            if scalar(@$ipset) && !$param->{force};
 
-                    update_refs({
-                        conf => $fw_conf,
-                        kind => 'ipset',
-                        old => $param->{name},
-                        env => $class->rule_env(),
-                        action => $action eq 'keep' ? undef : $action,
-                        subject => "ipset '$param->{name}' deleted",
-                    });
+                        update_refs({
+                            conf => $fw_conf,
+                            kind => 'ipset',
+                            old => $param->{name},
+                            env => $class->rule_env(),
+                            action => $action eq 'keep' ? undef : $action,
+                            subject => "ipset '$param->{name}' deleted",
+                        });
 
-                    $class->save_ipset($param, $fw_conf, undef);
+                        $class->save_ipset($param, $fw_conf, undef);
+                    },
+                );
+            };
 
-                },
-            );
+            if (!defined($param->{'dangling-references'})) {
+                $code->();
+                return undef;
+            }
 
-            return undef;
+            # fail on the request instead of inside the task
+            my (undef, undef, $ipset) = $class->load_config($param);
+
+            die "IPSet '$param->{name}' is not empty\n"
+                if scalar($ipset->@*) && !$param->{force};
+
+            return run_ref_update('fw-ipset-delete', $param->{vmid} // $param->{name}, $code);
         },
     });
 }
@@ -548,7 +560,7 @@ package PVE::API2::Firewall::BaseIPSetList;
 
 use strict;
 use warnings;
-use PVE::Firewall::References qw(update_refs);
+use PVE::Firewall::References qw(update_refs run_ref_update);
 use PVE::JSONSchema qw(get_standard_option);
 use PVE::Exception qw(raise_param_exc);
 use PVE::Firewall;
@@ -616,6 +628,25 @@ my $get_ipset_list = sub {
     return wantarray ? ($list, $digest) : $list;
 };
 
+my $check_rename = sub {
+    my ($conf, $param, $force) = @_;
+
+    my (undef, $digest) = &$get_ipset_list($conf);
+    PVE::Tools::assert_if_modified($digest, $param->{digest});
+
+    raise_param_exc({ name => "IPSet '$param->{rename}' does not exist" })
+        if !$conf->{ipset}->{ $param->{rename} };
+
+    # prevent overwriting existing ipset
+    if (
+        $param->{name} ne $param->{rename}
+        && $conf->{ipset}->{ $param->{name} }
+        && !$force
+    ) {
+        raise_param_exc({ name => "IPSet '$param->{name}' already exists" });
+    }
+};
+
 sub register_index {
     my ($class) = @_;
 
@@ -690,93 +721,100 @@ sub register_create {
             additionalProperties => 0,
             properties => $properties,
         },
-        returns => { type => 'null' },
+        returns => { type => 'string', optional => 1 },
         code => sub {
             my ($param) = @_;
 
-            $class->lock_config(
-                $param,
-                sub {
-                    my ($param) = @_;
-
-                    my $dangling_refs = $param->{'dangling-references'}
-                        // $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}) {
-                        my (undef, $digest) = &$get_ipset_list($fw_conf);
-                        PVE::Tools::assert_if_modified($digest, $param->{digest});
-
-                        raise_param_exc({ name => "IPSet '$param->{rename}' does not exist" })
-                            if !$fw_conf->{ipset}->{ $param->{rename} };
-
-                        if ($param->{name} ne $param->{rename}) {
-                            # prevent overwriting existing ipset
-                            raise_param_exc({ name => "IPSet '$param->{name}' 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";
+            my $dangling_refs = $param->{'dangling-references'}
+                // $properties->{'dangling-references'}->{default};
+            my $force = $param->{'force-rename'} // $properties->{'force-rename'}->{default};
+
+            my $renaming = $param->{rename} && $param->{name} ne $param->{rename};
+
+            my $code = sub {
+                $class->lock_config(
+                    $param,
+                    sub {
+                        my ($param) = @_;
+
+                        my ($cluster_conf, $fw_conf) = $class->load_config($param);
+
+                        if ($param->{rename}) {
+                            $check_rename->($fw_conf, $param, $force);
+
+                            if ($param->{name} ne $param->{rename}) {
+                                $fw_conf->{ipset}->{ $param->{name} } =
+                                    $fw_conf->{ipset}->{ $param->{rename} };
+
+                                my $saved = 0;
+                                my $save = sub {
+                                    $class->save_config($param, $fw_conf);
+                                    $saved = 1;
+                                };
+
+                                eval {
+                                    update_refs({
+                                        conf => $fw_conf,
+                                        kind => 'ipset',
+                                        old => $param->{rename},
+                                        new => $param->{name},
+                                        env => $class->rule_env(),
+                                        action => $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;
+                                }
                             }
 
-                            delete $fw_conf->{ipset}->{ $param->{rename} };
-
-                            if (
-                                my $comment =
-                                delete $fw_conf->{ipset_comments}->{ $param->{rename} }
-                            ) {
-                                $fw_conf->{ipset_comments}->{ $param->{name} } = $comment;
+                            $fw_conf->{ipset_comments}->{ $param->{name} } = $param->{comment}
+                                if defined($param->{comment});
+                        } else {
+                            foreach my $name (keys %{ $fw_conf->{ipset} }) {
+                                raise_param_exc({ name => "IPSet '$name' already exists" })
+                                    if $name eq $param->{name};
                             }
-                        }
 
-                        $fw_conf->{ipset_comments}->{ $param->{name} } = $param->{comment}
-                            if defined($param->{comment});
-                    } else {
-                        foreach my $name (keys %{ $fw_conf->{ipset} }) {
-                            raise_param_exc({ name => "IPSet '$name' already exists" })
-                                if $name eq $param->{name};
+                            $fw_conf->{ipset}->{ $param->{name} } = [];
+                            $fw_conf->{ipset_comments}->{ $param->{name} } = $param->{comment}
+                                if defined($param->{comment});
                         }
 
-                        $fw_conf->{ipset}->{ $param->{name} } = [];
-                        $fw_conf->{ipset_comments}->{ $param->{name} } = $param->{comment}
-                            if defined($param->{comment});
-                    }
+                        $class->save_config($param, $fw_conf);
+                    },
+                );
+            };
 
-                    $class->save_config($param, $fw_conf);
-                },
-            );
+            if (!$renaming || !defined($param->{'dangling-references'})) {
+                $code->();
+                return undef;
+            }
 
-            return undef;
+            # fail on the request instead of inside the task
+            my (undef, $fw_conf) = $class->load_config($param);
+
+            $check_rename->($fw_conf, $param, $force);
+
+            return run_ref_update('fw-ipset-rename', $param->{vmid} // $param->{rename}, $code);
         },
     });
 }
diff --git a/src/PVE/Firewall/References.pm b/src/PVE/Firewall/References.pm
index b106651..2fb8767 100644
--- a/src/PVE/Firewall/References.pm
+++ b/src/PVE/Firewall/References.pm
@@ -12,6 +12,7 @@ use base 'Exporter';
 our @EXPORT_OK = qw(
     collect_refs
     update_refs
+    run_ref_update
 );
 
 sub collect_refs {
@@ -441,6 +442,25 @@ sub update_refs {
     return $count;
 }
 
+=head3 run_ref_update($type, $id, $code)
+
+Run C<$code> as a worker task of type C<$type> for C<$id>.
+
+C<$id> is the guest a guest-scoped object belongs to so the task is logged with that guest, and the
+name of the object otherwise.
+
+Return the UPID of the task.
+
+=cut
+
+sub run_ref_update {
+    my ($type, $id, $code) = @_;
+
+    my $rpcenv = PVE::RESTEnvironment::get();
+
+    return $rpcenv->fork_worker($type, $id, $rpcenv->get_user(), $code);
+}
+
 =head3 format_ref_change($change)
 
 Format one recorded config change as a single line, e.g.
-- 
2.47.3