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

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

On April 21, 2021 3:38 pm, Stefan Reiter wrote:
> 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.

hmm, yeah. there is precedent for using a volname/volid accepting 
parameter in PVE::API2::Storage::Content though (complete with helper to 
resolve the storage path parameter and the 'potentially contained in 
volid' storage parameter), so maybe that might be an option as well? 
might make the CLI easier, and potentially also API usage (no need to 
have the "split volume id into parts" logic client-side as well then).

> 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.

no, although it might be nice to make one ;) the volume id format is 
specified at least as far as 'STORAGE_ID:VOLUME_NAME', but the volname 
itself is rather "freeform" (as in, '(.+)' with the details left up to 
the plugin :-/ - see PVE::Storage::Plugin::parse_volume_id).

> 
>>> +	    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
>> 
>> 
>