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) server-digest SHA256) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 0152D60147 for ; Wed, 27 Jan 2021 17:56:39 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id E7E0E253FE for ; Wed, 27 Jan 2021 17:56:38 +0100 (CET) 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 E78ED253F1 for ; Wed, 27 Jan 2021 17:56:37 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id B8ACE4590E for ; Wed, 27 Jan 2021 17:56:37 +0100 (CET) To: pve-devel@lists.proxmox.com References: <20210114171108.756728-1-aderumier@odiso.com> From: Mira Limbeck Message-ID: Date: Wed, 27 Jan 2021 17:56:36 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.6.0 MIME-Version: 1.0 In-Reply-To: <20210114171108.756728-1-aderumier@odiso.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-US X-SPAM-LEVEL: Spam detection results: 0 AWL 0.442 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.001 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. [readthedocs.io, qemuserver.pm, cloudinit.pm] Subject: Re: [pve-devel] [PATCH qemu-server] cloudinit: add sshdeletehostkeys option 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: Wed, 27 Jan 2021 16:56:39 -0000 Thank you for the patch. It doesn't apply on the latest qemu-server master. Looks like your Cloudinit.pm file already contained changes which are not part of the patch. Was it just the previous patch you sent? Some additional comments inline. On 1/14/21 6:11 PM, Alexandre Derumier wrote: > This define behaviour of ssh server keys generation on cloudinit > config change. > > different value: > > - once : only once at vmstart (default value) > - no : never generate ssh key > - yes: always generate ssh key > > When value is defined to 'once', the value is rewriten to 'no' > in vmconfig after vm start This is exactly the use case of vendor data (run once at boot): https://cloudinit.readthedocs.io/en/latest/topics/vendordata.html Maybe this could be done in addition to the instance-id change suggested below? > > Signed-off-by: Alexandre Derumier > --- > PVE/QemuServer.pm | 9 ++++++++- > PVE/QemuServer/Cloudinit.pm | 11 +++++++++-- > 2 files changed, 17 insertions(+), 3 deletions(-) > > diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm > index 54278e5..cd6c26c 100644 > --- a/PVE/QemuServer.pm > +++ b/PVE/QemuServer.pm > @@ -760,6 +760,13 @@ my $confdesc_cloudinit = { > format => 'urlencoded', > description => "cloud-init: Setup public SSH keys (one key per line, OpenSSH format).", > }, > + sshdeletehostkeys => { > + optional => 1, > + type => 'string', > + enum => [qw(once yes no)], > + default_key => 1, > + description => "cloud-init: Regenerate host SSH keys on config change.", > + }, > }; Consensus was that we do not want additional cloud-init options in the global options namespace. So instead it would be better to add it to cicustom instead and open that up for other custom options (as was initially intended). Regarding the enum => [qw(once yes no)] line, we probably want to accept everything type 'Boolean' accepts, not just 'yes' and 'no'. > > # what about other qemu settings ? > @@ -4943,7 +4950,7 @@ sub vm_start_nolock { > $conf = PVE::QemuConfig->load_config($vmid); # update/reload > } > > - PVE::QemuServer::Cloudinit::generate_cloudinitconfig($conf, $vmid); > + PVE::QemuServer::Cloudinit::generate_cloudinitconfig($conf, $vmid, 1); > > my $defaults = load_defaults(); > > diff --git a/PVE/QemuServer/Cloudinit.pm b/PVE/QemuServer/Cloudinit.pm > index dd643c1..4dbc4d6 100644 > --- a/PVE/QemuServer/Cloudinit.pm > +++ b/PVE/QemuServer/Cloudinit.pm > @@ -135,7 +135,7 @@ sub cloudinit_userdata { > $content .= " - $k\n"; > } > } > - $content .= "ssh_deletekeys: false\n" if PVE::QemuServer::check_running($vmid); > + $content .= "ssh_deletekeys: false\n" if defined($conf->{sshdeletehostkeys}) && $conf->{sshdeletehostkeys} eq 'no'; > > $content .= "chpasswd:\n"; > $content .= " expire: False\n"; > @@ -464,9 +464,10 @@ my $cloudinit_methods = { > }; > > sub generate_cloudinitconfig { > - my ($conf, $vmid) = @_; > + my ($conf, $vmid, $vmstart) = @_; > > my $format = get_cloudinit_format($conf); > + my $generated = undef; > > PVE::QemuConfig->foreach_volume($conf, sub { > my ($ds, $drive) = @_; > @@ -479,7 +480,13 @@ sub generate_cloudinitconfig { > or die "missing cloudinit methods for format '$format'\n"; > > $generator->($conf, $vmid, $drive, $volname, $storeid); > + $generated = 1; > }); > + > + if ($vmstart && $generated && (!defined($conf->{sshdeletehostkeys}) || $conf->{sshdeletehostkeys} eq 'once')) { > + $conf->{sshdeletehostkeys} = 'no'; > + PVE::QemuConfig->write_config($vmid, $conf); > + } > } Maybe it would make sense to create an instance id once and only change it if requested afterwards, instead of basing it on the user and network configs? This would also remove the need for this option. Then we could simply regenerate the instance id on a clone, or if requested when restoring from a backup to a new VMID. What do you think? I'll probably extend the documentation with info on preparing a cloudimg, as sometimes they do not work out of the box and require cleaning of the cloud-init artifacts [0] as well as changing the pre-configured cloud.cfg file. [0] https://cloudinit.readthedocs.io/en/latest/topics/cli.html#clean > > sub dump_cloudinit_config {