From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <m.limbeck@proxmox.com>
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 <pve-devel@lists.proxmox.com>; 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 <pve-devel@lists.proxmox.com>; 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 <pve-devel@lists.proxmox.com>; 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 <pve-devel@lists.proxmox.com>; 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 <m.limbeck@proxmox.com>
Message-ID: <d7e7747f-56a5-dd4a-609e-b6ae4eb83da5@proxmox.com>
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 <pve-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pve-devel>,
<mailto:pve-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pve-devel/>
List-Post: <mailto:pve-devel@lists.proxmox.com>
List-Help: <mailto:pve-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel>,
<mailto:pve-devel-request@lists.proxmox.com?subject=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 <aderumier@odiso.com>
> ---
> 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 {