[pve-devel] [RFC v2 manager 1/5] api: backup: add endpoint to list included guests and volumes

[Aaron Lauterer]

On 4/7/20 5:55 PM, Thomas Lamprecht wrote:
 > On 4/6/20 4:24 PM, Aaron Lauterer wrote:
 >> [...]
 >> +    description => 'Root node of the tree object. Children 
represent guests, grandchildren represent volumes of that guest.',
 >> +    properties => {
 >> +        not_all_permissions => {
 >> +        type => 'boolean',
 >> +        optional => 1,
 >> +        description => 'Wheter the user is missing permissions to 
view all guests.',
 >
 > s/Wheter/Whether/

thanks for catching that typo
 >
 >> +        },
 >> +        children => {
 >> +        type => 'array',
 >> +        items => {
 >> +            type => 'object',
 >> +            properties => {
 >> +            id => {
 >> +                type => 'integer',
 >> +                description => 'VMID of the guest.',
 >> +            },
 >> +            name => {
 >> +                type => 'string',
 >> +                description => 'Name of the guest',
 >> +                optional => 1,
 >> +            },
 >> +            type => {
 >> +                type => 'string',
 >> +                description => 'Type of the guest, VM or CT.',
 >> +                enum => ['qemu', 'lxc', 'unknown'],
 >
 > We die in the unknown case, so that cannot be returned
right
 >
 >> +            },
 >> +            children => {
 >> +                type => 'array',
 >> +                optional => 1,
 >> +                description => 'The volumes of the guest with the 
information if they will be included in backups.',
 >> +                items => {
 >> +                type => 'object',
 >> +                properties => {
 >> +                    id => {
 >> +                    type => 'string',
 >> +                    description => 'Configuration key of the volume.',
 >> +                    },
 >> +                    name => {
 >> +                    type => 'string',
 >> +                    description => 'Name of the volume.',
 >> +                    },
 >> +                    included => {
 >> +                    type => 'boolean',
 >> +                    description => 'Wheter the volume is included 
in the backup or not.',
 >> +                    },
 >> +                    reason => {
 >> +                    type => 'string',
 >> +                    description => 'The reason why the volume is 
included (or excluded).',
 >> +                    },
 >> +                },
 >> +                },
 >> +            },
 >> +            },
 >> +        },
 >> +        },
 >> +    },
 >> +    },
 >> +    code => sub {
 >> +    my ($param) = @_;
 >> +
 >> +    my $rpcenv = PVE::RPCEnvironment::get();
 >> +
 >> +    my $user = $rpcenv->get_user();
 >> +
 >> +    my $vzconf = cfs_read_file('vzdump.cron');
 >> +    my $all_jobs = $vzconf->{jobs} || [];
 >> +    my $job;
 >> +    my $rrd = PVE::Cluster::rrd_dump();
 >> +
 >> +    foreach my $j (@$all_jobs) {
 >> +        $job = $j if $j->{id} eq $param->{id};
 >
 > This could let the reader think that this is a bug, as it looks like 
it gets
 > overwritten, plus on huge amount of jobs you would iterate a lot of 
those for
 > nothing, if the $job with the requested id was found early. Rather do:
 >
 > if ($j->{id} eq $param->{id}) {
 >      $job = $j;
 >      last;
 > }
 >
 > This makes it much more explicit and easier to grasp when just 
quickly reading over
 > the code.
will do

 >
 >> +    }
 >> +    raise_param_exc({ id => "No such job '$param->{id}'" }) if !$job;
 >> +
 >> +    my $vmlist = PVE::Cluster::get_vmlist();
 >> +
 >> +    my @vmids = @{PVE::VZDump->get_included_guests($job)};
 >
 > s/vmid/job_vmids/
 >
 > and the comment on the other series regarding passing this as reference,
 > and even as hash.
Yeah, this will be adapted to the changes of the other series, one of 
the reasons why this is labeled as RFC

 >
 >> +
 >> +    # remove VMIDs to which the user has no permission to not leak 
infos
 >> +    # like the guest name
 >> +    my $not_all_permissions = 0;
 >> +    @vmids = grep {
 >> +        my $allowed = $rpcenv->check($user, "/vms/$_", [ 
'VM.Backup' ], 1);
 >
 > hmm, is VM.Backup really the info we want to hide, why not VM.Audit?
 > (or those which the user has either permission?)
 >
 > As VM.Backup is the permission for being allowed to make a backup or 
restore one,
 > not for being allowed to view a VM in the cluster.

I haven't fully groked the permission system yet. What I want to achieve 
is to not show any VMs the user does not have permission to see.
I think VM.Audit should be enough then.

 >
 >> +        $not_all_permissions = 1 if !$allowed && !$job->{all};
 >
 > This could be found out afterwards, avoids a O(n) expression. 
$job->{all} is
 > available anyway and if you remember how many elements @vmids had 
before you
 > can just check if it has less after the permission checking. That has the
 > additional advantage that you can omit the $allowed intermediate 
variable. For
 > example (not fully thought out - so don't just copy 1:1)
 >
 > my $job_guest_count = scalar(@vmids);
 > my @allowed_vmids = grep {
 >     $rpcenv->check_any($user, "/vms/$_", [ 'VM.Audit', 'VM.Backup' ], 1)
 > } @vmids;
 >
 > my $permissions_for_all = $job->{all} || $job_guest_count == 
scalar(@allowed_vmids);

thx for the hint. I'll look into it.

 >
 >> +        $allowed;
 >> +    } @vmids;
 >> +
 >> +    my $result = {
 >> +        children => [],
 >> +        leaf => 0,
 >
 > can't that be determined indirectly? I.e., if children is empty? (or 
maybe made explicitly undef/null)

I checked the ExtJS docs again and IIUC the leaf flag only needs to be 
set if it is true. Thus we can remove it here altogether. But just out 
of curiosity; would it be a bad pattern to set it here to the most 
likely value (guests usually do have volumes) and check at the end after 
all possible volumes are added if there are any children, and if not, 
set leaf to 1?

 >
 >> +        not_all_permissions => $not_all_permissions,
 >> +    };
 >> +
 >> +    foreach (@vmids) {
 >> +        my $id = $_;
 >
 > This is a style we normally do not use, do:
 >
 > for my $vmid (@vmids) {

okay
 >
 >> +
 >> +        # it's possible that a job has VMIDs configured that are not in
 >> +        # vmlist. This could be because a guest was removed but not 
purged.
 >> +        # Since there is no more data available we can only deliver 
the VMID
 >> +        # and no volumes.
 >> +        if (!defined $vmlist->{ids}->{$id}) {
 >> +        my $missing_guest = {
 >> +            id => $id,
 >> +            type => 'unknown',
 >> +            leaf => 1,
 >> +        };
 >> +        push @{$result->{children}}, $missing_guest;
 >> +        next;
 >> +        }
 >> +
 >> +        my $guest = {
 >> +        id => $id + 0,
 >
 > here you do + 0 (which i rather have as int($x)) but a few lines 
above you don't?
 > I'd guess that it isn't required here?

It's possible that I forgot it above but I will check again if it is 
stored as a number or string and use int() if needed.