Re: [pve-devel] [PATCH qemu-server v3 2/3] Move importdisk from qm to API

[Dominik Csapak <d.csapak@proxmox.com>]

comments inline

On 8/25/20 11:24 AM, Dominic Jäger wrote:
> Additionally, add parameters that enable directly (avoiding the intermediate
> step as unused disk) attaching the disk to a bus/device with all known disk
> options.
> 
> Required to create a GUI for importdisk.
> 
> Signed-off-by: Dominic Jäger <d.jaeger@proxmox.com>
> ---
> v2->v3: unchanged
> 
>   PVE/API2/Qemu.pm             | 113 ++++++++++++++++++++++++++++++++++-
>   PVE/CLI/qm.pm                |  57 +-----------------
>   PVE/QemuServer/Drive.pm      |  14 +++++
>   PVE/QemuServer/ImportDisk.pm |   2 +-
>   4 files changed, 127 insertions(+), 59 deletions(-)
> 
> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
> index 8da616a..66e630d 100644
> --- a/PVE/API2/Qemu.pm
> +++ b/PVE/API2/Qemu.pm
> @@ -24,6 +24,7 @@ use PVE::QemuServer;
>   use PVE::QemuServer::Drive;
>   use PVE::QemuServer::CPUConfig;
>   use PVE::QemuServer::Monitor qw(mon_cmd);
> +use PVE::QemuServer::ImportDisk qw(do_import);
>   use PVE::QemuMigrate;
>   use PVE::RPCEnvironment;
>   use PVE::AccessControl;
> @@ -45,8 +46,6 @@ BEGIN {
>       }
>   }
>   
> -use Data::Dumper; # fixme: remove
> -
>   use base qw(PVE::RESTHandler);
>   
>   my $opt_force_description = "Force physical removal. Without this, we simple remove the disk from the config file and create an additional configuration entry called 'unused[n]', which contains the volume ID. Unlink of unused[n] always cause physical removal.";
> @@ -4265,4 +4264,114 @@ __PACKAGE__->register_method({
>   	return PVE::QemuServer::Cloudinit::dump_cloudinit_config($conf, $param->{vmid}, $param->{type});
>       }});
>   
> +__PACKAGE__->register_method ({
> +    name => 'importdisk',
> +    path => '{vmid}/importdisk',
> +    method => 'POST',
> +    protected => 1, # for worker upid file
> +    proxyto => 'node',
> +    description => "Import an external disk image into a VM. The image format ".
> +	"has to be supported by qemu-img.",
> +    permissions => {
> +	check => [ 'and',
> +	    [ 'perm', '/storage/{storage}', ['Datastore.AllocateTemplate']],
> +	    [ 'perm', '/storage/{storage}', ['Datastore.AllocateSpace']],
> +	    [ 'perm', '/vms/{vmid}', ['VM.Allocate']],
> +	],

this is a big no-no

now everyone that has permissions to create a vm can import any
file they want from any path?

(e.g. another vm image, /dev/sda (!!), etc.)

this is basically circumventing our whole permission system...

for 'qm' this was okay since that was root@pam only, but if
we are in the api, we have to be careful what to import

in general, we have to do either:
* restrict the source path to a very small subset of possible
   paths, that are guaranteed to not be dangerous
   (/tmp/pve-import-$userid/ ?)

* or leave it as 'root@pam' only

> +    },
> +    parameters => {
> +	additionalProperties => 0,
> +	properties => {
> +	    node => get_standard_option('pve-node'),
> +	    vmid => get_standard_option('pve-vmid',
> +		{completion => \&PVE::QemuServer::complete_vmid}),
> +	    source => {
> +		description => "Path to the disk image to import",
> +		type => 'string',
> +	    },
> +	    device => {
> +		type => 'string',
> +		description => "Bus/Device type of the new disk ".
> +		    "(e.g. 'ide0', 'scsi2'). Will add the image as unused disk ".
> +		    "if omitted.",
> +		enum => [PVE::QemuServer::Drive::valid_drive_names()],
> +		optional => 1,
> +	    },
> +	    device_options => {
> +		type => 'string',
> +		format => 'drive_options',
> +		description => "Options to set for the new disk ".
> +		    "(e.g. 'discard=on,backup=0')",
> +		optional => 1,
> +	    },
> +	    storage => get_standard_option('pve-storage-id', {
> +		description => "The storage to which the image will be imported to.",
> +		completion => \&PVE::QemuServer::complete_storage,
> +	    }),
> +	    format => {
> +		type => 'string',
> +		description => 'Target format.',
> +		enum => [ 'raw', 'qcow2', 'vmdk' ],
> +		optional => 1,
> +	    },
> +	    digest => get_standard_option('pve-config-digest'),
> +	},
> +    },
> +    returns => { type => 'string'},
> +    code => sub {
> +	my ($params) = @_;
> +	my $rpcenv = PVE::RPCEnvironment::get();
> +	my $authuser = $rpcenv->get_user();
> +
> +	my $vmid = extract_param($params, 'vmid');
> +	my $source = extract_param($params, 'source');
> +	my $digest = extract_param($params, 'digest');
> +	my $device_options = extract_param($params, 'device_options');
> +	my $device = extract_param($params, 'device');
> +	my $storage = extract_param($params, 'storage');
> +	my $vm_conf = PVE::QemuConfig->load_config($vmid);
> +
> +	die "Could not import because source parameter is an empty string\n"
> +	    if ($source eq "");
> +	die "Could not import because source '$source' is not an absolute path\n"
> +	    if $source !~ m!/!;
> +	die "Could not import because source '$source' does not exist"
> +	    if !-e $source;
> +	die "VM $vmid config checksum missmatch (file change by other user?)\n"
> +	    if $digest && $digest ne $vm_conf->{digest};
> +	die "Could not import because device $device is already in use in ".
> +	    "VM $vmid. Choose a different device!\n"
> +	    if $device && $vm_conf->{$device};
> +
> +	my $worker = sub {
> +	    my $message = $device ? "to $device on" : 'as unused disk to';
> +	    print "Importing disk '$source' $message VM $vmid ...\n";
> +	    my ($unused_disk, $volid) = eval {
> +		PVE::QemuServer::ImportDisk::do_import(
> +		    $source, $vmid, $storage,
> +		    {format => extract_param($params, 'format')},
> +		)
> +	    };
> +	    die "Importing disk failed: $@\n" if $@;
> +
> +	    if ($device) {
> +		eval {
> +		    $update_vm_api->({
> +			vmid => $vmid,
> +			$device => "$volid,$device_options",
> +		    }, 1);
> +		};
> +		# Importing large disks takes a lot of time
> +		# => don't remove disk automatically on option update error
> +		die "Copying disk to $unused_disk succeeded but attaching it ".
> +		    "as $device and setting its options to $device_options ".
> +		    "failed: $@.\n Adjust them manually.\n" if $@;
> +	    }
> +	    print "Successfully imported disk '$source' as ".
> +		($device ? $device : $unused_disk) . ": $volid'\n";

is that not somehow racy?

i start an import and give devices scsi2

then at the right time i update the vm with scsi2 and now i overwrite it 
again?

should we not hold a lock ? or can we ignore that ? (at least document it?)

also if i see it right, 'do_import' can do this already?
you even change the comment of the code but you don't use it...

> +	};
> +	my $upid = $rpcenv->fork_worker('importdisk', $vmid, $authuser, $worker);
> +	return $upid;
> +    }});
> +
>   1;
> diff --git a/PVE/CLI/qm.pm b/PVE/CLI/qm.pm
> index 282fa86..4a304ce 100755
> --- a/PVE/CLI/qm.pm
> +++ b/PVE/CLI/qm.pm
> @@ -445,61 +445,6 @@ __PACKAGE__->register_method ({
>   	return undef;
>       }});
>   
> -__PACKAGE__->register_method ({
> -    name => 'importdisk',
> -    path => 'importdisk',
> -    method => 'POST',
> -    description => "Import an external disk image as an unused disk in a VM. The
> - image format has to be supported by qemu-img(1).",
> -    parameters => {
> -	additionalProperties => 0,
> -	properties => {
> -	    vmid => get_standard_option('pve-vmid', {completion => \&PVE::QemuServer::complete_vmid}),
> -	    source => {
> -		description => 'Path to the disk image to import',
> -		type => 'string',
> -		optional => 0,
> -	    },
> -            storage => get_standard_option('pve-storage-id', {
> -		description => 'Target storage ID',
> -		completion => \&PVE::QemuServer::complete_storage,
> -		optional => 0,
> -            }),
> -	    format => {
> -		type => 'string',
> -		description => 'Target format',
> -		enum => [ 'raw', 'qcow2', 'vmdk' ],
> -		optional => 1,
> -	    },
> -	},
> -    },
> -    returns => { type => 'null'},
> -    code => sub {
> -	my ($param) = @_;
> -
> -	my $vmid = extract_param($param, 'vmid');
> -	my $source = extract_param($param, 'source');
> -	my $storeid = extract_param($param, 'storage');
> -	my $format = extract_param($param, 'format');
> -
> -	my $vm_conf = PVE::QemuConfig->load_config($vmid);
> -	PVE::QemuConfig->check_lock($vm_conf);
> -	die "$source: non-existent or non-regular file\n" if (! -f $source);
> -
> -	my $storecfg = PVE::Storage::config();
> -	PVE::Storage::storage_check_enabled($storecfg, $storeid);
> -
> -	my $target_storage_config =  PVE::Storage::storage_config($storecfg, $storeid);
> -	die "storage $storeid does not support vm images\n"
> -	    if !$target_storage_config->{content}->{images};
> -
> -	print "importing disk '$source' to VM $vmid ...\n";
> -	my ($drive_id, $volid) = PVE::QemuServer::ImportDisk::do_import($source, $vmid, $storeid, { format => $format });
> -	print "Successfully imported disk as '$drive_id:$volid'\n";
> -
> -	return undef;
> -    }});
> -
>   __PACKAGE__->register_method ({
>       name => 'terminal',
>       path => 'terminal',
> @@ -984,7 +929,7 @@ our $cmddef = {
>   
>       terminal => [ __PACKAGE__, 'terminal', ['vmid']],
>   
> -    importdisk => [ __PACKAGE__, 'importdisk', ['vmid', 'source', 'storage']],
> +    importdisk => [ "PVE::API2::Qemu", 'importdisk', ['vmid', 'source', 'storage'], { node => $nodename }],
>   
>       importovf => [ __PACKAGE__, 'importovf', ['vmid', 'manifest', 'storage']],
>   
> diff --git a/PVE/QemuServer/Drive.pm b/PVE/QemuServer/Drive.pm
> index 91c33f8..e7cb74c 100644
> --- a/PVE/QemuServer/Drive.pm
> +++ b/PVE/QemuServer/Drive.pm
> @@ -201,6 +201,7 @@ my %wwn_fmt = (
>       },
>   );
>   
> +
>   my $add_throttle_desc = sub {
>       my ($key, $type, $what, $unit, $longunit, $minimum) = @_;
>       my $d = {
> @@ -308,6 +309,19 @@ my $alldrive_fmt = {
>       %wwn_fmt,
>   };
>   
> +my %optional_file_drivedesc_base = %drivedesc_base;
> +$optional_file_drivedesc_base{file}{optional} = 1;
> +my $drive_options_fmt = {
> +    %optional_file_drivedesc_base,
> +    %iothread_fmt,
> +    %model_fmt,
> +    %queues_fmt,
> +    %scsiblock_fmt,
> +    %ssd_fmt,
> +    %wwn_fmt,
> +};
> +PVE::JSONSchema::register_format('drive_options', $drive_options_fmt);
> +
>   my $efidisk_fmt = {
>       volume => { alias => 'file' },
>       file => {
> diff --git a/PVE/QemuServer/ImportDisk.pm b/PVE/QemuServer/ImportDisk.pm
> index 51ad52e..d89cd4d 100755
> --- a/PVE/QemuServer/ImportDisk.pm
> +++ b/PVE/QemuServer/ImportDisk.pm
> @@ -38,7 +38,7 @@ sub do_import {
>   	}
>   
>   	if ($drive_name) {
> -	    # should never happen as setting $drive_name is not exposed to public interface
> +	    # Exposed in importdisk API only
>   	    die "cowardly refusing to overwrite existing entry: $drive_name\n" if $vm_conf->{$drive_name};
>   
>   	    my $modified = {}; # record what $option we modify
>