From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id A58EA60CB0 for ; Thu, 3 Sep 2020 15:53:18 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 8ACEE19A76 for ; Thu, 3 Sep 2020 15:52:48 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [212.186.127.180]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id 2F0FC19A6C for ; Thu, 3 Sep 2020 15:52:47 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id E4783449DA for ; Thu, 3 Sep 2020 15:52:46 +0200 (CEST) To: pve-devel@lists.proxmox.com References: <20200825092449.49410-1-d.jaeger@proxmox.com> <20200825092449.49410-3-d.jaeger@proxmox.com> From: Dominik Csapak Message-ID: <4f1eb138-fa5d-a8a6-7b48-fcad171c5c1e@proxmox.com> Date: Thu, 3 Sep 2020 15:52:45 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:81.0) Gecko/20100101 Thunderbird/81.0 MIME-Version: 1.0 In-Reply-To: <20200825092449.49410-3-d.jaeger@proxmox.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.766 Adjusted score from AWL reputation of From: address KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment NICE_REPLY_A -0.324 Looks like a legit reply (A) RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [drive.pm, qm.pm, qemu.pm, importdisk.pm] Subject: Re: [pve-devel] [PATCH qemu-server v3 2/3] Move importdisk from qm to API X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 03 Sep 2020 13:53:18 -0000 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 > --- > 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 >