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

[aderumier@odiso.com]

Le mercredi 27 janvier 2021 à 17:56 +0100, Mira Limbeck a écrit :
> 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?
> 
Hi, sorry, I didn't see your response.
I'll rebase my patch.


> 
> 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?
> 
> 
> 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


The main problem currently is indeed that we change instance-id at each
rebuild of the cloud-init disk.
But I'm not sure that's it's possible to change ip address when keeping
same instance-id, because ip configuration is done
at the cloudinit-init-local service, at it's already done once. 
Maybe this was the historic reason why we change the the instance-id
each time, I don't remember exactly.
I'll check that tomorrow to be sure, but indeed, keeping the instance-
id should be the clean way.



> > 
> > 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'.

Ok no problem, I'll change that


Thanks for the review !