Re: [pve-devel] [PATCH qemu-server] cloudinit: add sshdeletehostkeys option

[Mira Limbeck <m.limbeck@proxmox.com>]

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 {