[pve-devel] [PATCH manager v7 02/19] api: jobs: vzdump: pass job 'job-id' parameter

[Fabian Grünbichler]

Quoting Lukas Wagner (2024-06-10 10:40:21)
> This allows us to access us the backup job id in the send_notification
> function, where we can set it as metadata for the notification.

should we have some sort of safeguard against passing in a bogus/fake job-id?
e.g., right now, I could call this API endpoint with arbitrary job-id values
and (potentially) trigger notifications to other users..

some possible avenues would be:
- limit the parameter to root (but that means only scheduled executions can set
  it, manual invocations can't)
- limit to existing job-ids (doesn't provide much benefit)
- ..

> 
> Signed-off-by: Lukas Wagner <l.wagner at proxmox.com>
> ---
>  PVE/API2/VZDump.pm | 8 ++++++++
>  PVE/Jobs/VZDump.pm | 4 +++-
>  PVE/VZDump.pm      | 6 +++---
>  3 files changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/PVE/API2/VZDump.pm b/PVE/API2/VZDump.pm
> index 7f92e7ec..84dbc100 100644
> --- a/PVE/API2/VZDump.pm
> +++ b/PVE/API2/VZDump.pm
> @@ -53,6 +53,14 @@ __PACKAGE__->register_method ({
>      parameters => {
>         additionalProperties => 0,
>         properties => PVE::VZDump::Common::json_config_properties({
> +           'job-id' => {
> +               description => "The ID of the backup job. If set, the 'backup-job' metadata field"
> +                   . " of the backup notification will be set to this value.",
> +               type => 'string',
> +               format => 'pve-configid',
> +               maxLength => 256,
> +               optional => 1,
> +           },
>             stdout => {
>                 type => 'boolean',
>                 description => "Write tar to stdout, not to a file.",
> diff --git a/PVE/Jobs/VZDump.pm b/PVE/Jobs/VZDump.pm
> index b8e57945..2dad3f55 100644
> --- a/PVE/Jobs/VZDump.pm
> +++ b/PVE/Jobs/VZDump.pm
> @@ -12,7 +12,7 @@ use PVE::API2::VZDump;
>  use base qw(PVE::VZDump::JobBase);
>  
>  sub run {
> -    my ($class, $conf) = @_;
> +    my ($class, $conf, $job_id) = @_;
>  
>      my $props = $class->properties();
>      # remove all non vzdump related options
> @@ -20,6 +20,8 @@ sub run {
>         delete $conf->{$opt} if !defined($props->{$opt});
>      }
>  
> +    $conf->{'job-id'} = $job_id;
> +
>      # Required as string parameters # FIXME why?! we could just check ref()
>      for my $key (keys $PVE::VZDump::Common::PROPERTY_STRINGS->%*) {
>         if ($conf->{$key} && ref($conf->{$key}) eq 'HASH') {
> diff --git a/PVE/VZDump.pm b/PVE/VZDump.pm
> index 5b7080f3..2167b289 100644
> --- a/PVE/VZDump.pm
> +++ b/PVE/VZDump.pm
> @@ -483,6 +483,7 @@ sub send_notification {
>      my ($self, $tasklist, $total_time, $err, $detail_pre, $detail_post) = @_;
>  
>      my $opts = $self->{opts};
> +    my $job_id = $opts->{'job-id'};
>      my $mailto = $opts->{mailto};
>      my $cmdline = $self->{cmdline};
>      my $policy = $opts->{mailnotification} // 'always';
> @@ -529,12 +530,11 @@ sub send_notification {
>      };
>  
>      my $fields = {
> -       # TODO: There is no straight-forward way yet to get the
> -       # backup job id here... (I think pvescheduler would need
> -       # to pass that to the vzdump call?)
>         type => "vzdump",
>         hostname => $hostname,
>      };
> +    # Add backup-job metadata field in case this is a backup job.
> +    $fields->{'job-id'} = $job_id if $job_id;
>  
>      my $severity = $failed ? "error" : "info";
>      my $email_configured = $mailto && scalar(@$mailto);
> -- 
> 2.39.2
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel at lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
>