Re: [pve-devel] [PATCH storage 09/10] add FileRestore API for PBS

[Stefan Reiter <s.reiter@proxmox.com>]

On 21/04/2021 15:26, Fabian Grünbichler wrote:
> On April 21, 2021 1:15 pm, Stefan Reiter wrote:
>> Includes list and restore calls.
>>
>> Requires VM.Backup and Datastore.Audit permissions, for the accessed
>> VM/CT and containing datastore respectively.
> 
> we require Datastore.AllocateSpace + VM.Backup for the owning guest,
> or Datastore.Allocate for the storage altogether for accessing backup
> archives otherwise, maybe this should have the same logic?
> 

sounds reasonable

>>
>> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
>> ---
>>
>> Requires updated pve-common, pve-http-server.
>>
>>   PVE/API2/Storage/FileRestore.pm | 163 ++++++++++++++++++++++++++++++++
>>   PVE/API2/Storage/Makefile       |   2 +-
>>   PVE/API2/Storage/Status.pm      |   6 ++
>>   3 files changed, 170 insertions(+), 1 deletion(-)
>>   create mode 100644 PVE/API2/Storage/FileRestore.pm
>>
>> diff --git a/PVE/API2/Storage/FileRestore.pm b/PVE/API2/Storage/FileRestore.pm
>> new file mode 100644
>> index 0000000..a0b5e88
>> --- /dev/null
>> +++ b/PVE/API2/Storage/FileRestore.pm
>> @@ -0,0 +1,163 @@
>> +package PVE::API2::Storage::FileRestore;
>> +
>> +use strict;
>> +use warnings;
>> +
>> +use PVE::JSONSchema qw(get_standard_option);
>> +use PVE::PBSClient;
>> +use PVE::Storage;
>> +use PVE::Tools qw(extract_param);
>> +
>> +use PVE::RESTHandler;
>> +use base qw(PVE::RESTHandler);
>> +
>> +__PACKAGE__->register_method ({
>> +    name => 'list',
>> +    path => 'list',
>> +    method => 'GET',
>> +    proxyto => 'node',
>> +    permissions => {
>> +	description => "Requires 'VM.Backup' permission on the VM being accessed, and " .
>> +	    "'Datastore.Audit' on the datastore being restored from.",
>> +	user => 'all', # checked explicitly
>> +    },
>> +    description => "List files and directories for single file restore under the given path.",
>> +    protected => 1,
>> +    parameters => {
>> +	additionalProperties => 0,
>> +	properties => {
>> +	    node => get_standard_option('pve-node'),
>> +	    storage => get_standard_option('pve-storage-id'),
>> +	    snapshot => {
>> +		description => "Backup snapshot identifier.",
>> +		type => 'string',
>> +	    },
> 
> why not use a volume id here (instead of storage + snapshot ID), and
> then check inside whether it's a pbs backup? would allow easily
> extending this to VMA backups as well later on, completion by our usual
> volume ID helpers/selectors, ..
> 

I did it this way mostly because we get the 'storage' parameter here 
anyway - it's in the URL path, since this lives under 
'/nodes/{node}/storage/{storage}'. Thus the only thing missing was the 
snapshot.

Is there a format for the "latter part of a volume-id"? If there is, 
this would also just be a simple change later on, as it'd just replace 
the 'snapshot' param.

>> +	    filepath => {
>> +		description => 'base64-path to the directory or file being listed, or "/".',
>> +		type => 'string',
>> +	    },
>> +	},
>> +    },
>> +    returns => {
>> +	type => 'array',
>> +	items => {
>> +	    type => "object",
>> +	    properties => {
>> +		filepath => {
>> +		    description => "base64 path of the current entry",
>> +		    type => 'string',
>> +		},
>> +		type => {
>> +		    description => "Entry type.",
>> +		    type => 'string',
>> +		},
>> +		text => {
>> +		    description => "Entry display text.",
>> +		    type => 'string',
>> +		},
>> +		leaf => {
>> +		    description => "If this entry is a leaf in the directory graph.",
>> +		    type => 'any', # JSON::PP::Boolean gets passed through
>> +		},
>> +		size => {
>> +		    description => "Entry file size.",
>> +		    type => 'integer',
>> +		    optional => 1,
>> +		},
>> +		mtime => {
>> +		    description => "Entry last-modified time (unix timestamp).",
>> +		    type => 'integer',
>> +		    optional => 1,
>> +		},
>> +	    },
>> +	},
>> +    },
>> +    code => sub {
>> +	my ($param) = @_;
>> +
>> +	my $rpcenv = PVE::RPCEnvironment::get();
>> +	my $user = $rpcenv->get_user();
>> +
>> +	my $path = extract_param($param, 'filepath') || "/";
>> +	my $base64 = $path ne "/";
>> +	my $snap = extract_param($param, 'snapshot');
>> +	my $storeid = extract_param($param, 'storage');
>> +	my $cfg = PVE::Storage::config();
>> +	my $scfg = PVE::Storage::storage_config($cfg, $storeid);
>> +
>> +	my $volid = "$storeid:backup/$snap";
>> +	my (undef, undef, $ownervm) = PVE::Storage::parse_volname($cfg, $volid);
>> +	$rpcenv->check($user, "/storage/$storeid", ['Datastore.Audit']);
>> +	$rpcenv->check($user, "/vms/$ownervm", ['VM.Backup']);
> 
> see comment above, this could then become
> 'PVE::Storage::check_volume_access(..)
>> +
>> +	my $client = PVE::PBSClient->new($scfg, $storeid);
>> +	my $ret = $client->file_restore_list($snap, $path, $base64);
>> +
>> +	return $ret;
>> +    }});
>> +
>> +__PACKAGE__->register_method ({
>> +    name => 'download',
>> +    path => 'download',
>> +    method => 'GET',
>> +    proxyto => 'node',
>> +    permissions => {
>> +	description => "Requires 'VM.Backup' permission on the VM being accessed, and " .
>> +	    "'Datastore.Audit' on the datastore being restored from.",
>> +	user => 'all', # checked explicitly
>> +    },
>> +    description => "Extract a file or directory (as zip archive) from a PBS backup.",
>> +    parameters => {
>> +	additionalProperties => 0,
>> +	properties => {
>> +	    node => get_standard_option('pve-node'),
>> +	    storage => get_standard_option('pve-storage-id'),
>> +	    snapshot => {
>> +		description => "Backup snapshot identifier.",
>> +		type => 'string',
>> +	    },
> 
> same here as above
> 
>> +	    filepath => {
>> +		description => 'base64-path to the directory or file being listed.',
>> +		type => 'string',
>> +	    },
>> +	},
>> +    },
>> +    returns => {
>> +	type => 'any', # download
>> +    },
>> +    protected => 1,
>> +    code => sub {
>> +	my ($param) = @_;
>> +
>> +	my $rpcenv = PVE::RPCEnvironment::get();
>> +	my $user = $rpcenv->get_user();
>> +
>> +	my $path = extract_param($param, 'filepath');
>> +	my $snap = extract_param($param, 'snapshot');
>> +	my $storeid = extract_param($param, 'storage');
>> +	my $cfg = PVE::Storage::config();
>> +	my $scfg = PVE::Storage::storage_config($cfg, $storeid);
>> +
>> +	my $volid = "$storeid:backup/$snap";
>> +	my (undef, undef, $ownervm) = PVE::Storage::parse_volname($cfg, $volid);
>> +	$rpcenv->check($user, "/storage/$storeid", ['Datastore.Audit']);
>> +	$rpcenv->check($user, "/vms/$ownervm", ['VM.Backup']);
> 
> and here as well
> 
>> +
>> +	my $client = PVE::PBSClient->new($scfg, $storeid);
>> +	my $fifo = $client->file_restore_extract_prepare();
>> +
>> +	$rpcenv->fork_worker('pbs-download', undef, $user, sub {
>> +	    $client->file_restore_extract($fifo, $snap, $path, 1);
>> +	});
>> +
>> +	my $ret = {
>> +	    download => {
>> +		path => $fifo,
>> +		stream => 1,
>> +		'content-type' => 'application/octet-stream',
>> +	    },
>> +	};
>> +	return $ret;
>> +    }});
>> +
>> +1;
>> diff --git a/PVE/API2/Storage/Makefile b/PVE/API2/Storage/Makefile
>> index 690b437..1705080 100644
>> --- a/PVE/API2/Storage/Makefile
>> +++ b/PVE/API2/Storage/Makefile
>> @@ -1,5 +1,5 @@
>>   
>> -SOURCES= Content.pm Status.pm Config.pm PruneBackups.pm Scan.pm
>> +SOURCES= Content.pm Status.pm Config.pm PruneBackups.pm Scan.pm FileRestore.pm
>>   
>>   .PHONY: install
>>   install:
>> diff --git a/PVE/API2/Storage/Status.pm b/PVE/API2/Storage/Status.pm
>> index d12643f..897b4a7 100644
>> --- a/PVE/API2/Storage/Status.pm
>> +++ b/PVE/API2/Storage/Status.pm
>> @@ -12,6 +12,7 @@ use PVE::RRD;
>>   use PVE::Storage;
>>   use PVE::API2::Storage::Content;
>>   use PVE::API2::Storage::PruneBackups;
>> +use PVE::API2::Storage::FileRestore;
>>   use PVE::RESTHandler;
>>   use PVE::RPCEnvironment;
>>   use PVE::JSONSchema qw(get_standard_option);
>> @@ -32,6 +33,11 @@ __PACKAGE__->register_method ({
>>       path => '{storage}/content',
>>   });
>>   
>> +__PACKAGE__->register_method ({
>> +   subclass => "PVE::API2::Storage::FileRestore",
>> +   path => '{storage}/file-restore',
>> +});
>> +
>>   __PACKAGE__->register_method ({
>>       name => 'index',
>>       path => '',
>> -- 
>> 2.20.1
>>
>>
>>
>> _______________________________________________
>> pve-devel mailing list
>> pve-devel@lists.proxmox.com
>> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>>
>>
>>
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
>