Re: [PATCH container v2] fix #7164: lxc: restore: apply password and ssh keys during container restore

[Erik Fastermann <e.fastermann@proxmox.com>]

Thank you for working on this. See my comments below.


The commit message is a little long. I would just shorten it to 'fix 
#7164: restore: apply password and ssh keys', as lxc is obvious since we 
are in pve-container and 'during container restore' is clear from the 
'restore:' tag.

> When restoring a container with `pct restore`, the `--password` and
> `--ssh-public-keys` options are silently ignored. Previously, the API
> endpoint only invoked setup hooks during initial container creation or
> cloning, skipping them entirely for restores.
> 
> Introduce a new `post_restore_hook` in the LXC setup plugin
> architecture. This hook applies the provided credentials if they are
> defined.
> 
> Add `check_systemd_nesting` to the `post_restore_hook`.

Why is this added? A rational here would be helpful. If the reason is 
just symmetry with `post_create_hook`, we might need to be careful here, 
as this could introduce new unwanted failure conditions. I have not 
checked in detail, but some of the code can die.

> 
> Move `template_fixup`from the API endpoint function to the

Nit: Missing space (`template_fixup`from)

> `post_restore_hook`, matching the `post_create_hook` logic.
> 
> Signed-off-by: Elias Huhsovitz <e.huhsovitz@proxmox.com>
> ---
> 
> v1: https://lore.proxmox.com/all/20260716153959.183045-1-e.huhsovitz@proxmox.com/
> 
> Changes made according to the feedback (big thanks):
> https://lore.proxmox.com/all/95a38704-543e-4820-b36c-696cf192a684@proxmox.com/
> 
> changes v1->v2:
> * move logic into new function `post_restore_hook`
> * do not call `post_create_hook` unconditionally
> * conditionally call functions to:
>      - set password
>      - set ssh keys
> * call `check_systemd_nesting()` for pct restore
> 
>   src/PVE/API2/LXC.pm            | 2 +-
>   src/PVE/LXC/Setup.pm           | 9 +++++++++
>   src/PVE/LXC/Setup/Base.pm      | 9 +++++++++
>   src/PVE/LXC/Setup/Plugin.pm    | 5 +++++
>   src/PVE/LXC/Setup/Unmanaged.pm | 4 ++++
>   5 files changed, 28 insertions(+), 1 deletion(-)
> 
> diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
> index 22320ba..ad3b635 100644
> --- a/src/PVE/API2/LXC.pm
> +++ b/src/PVE/API2/LXC.pm
> @@ -590,7 +590,7 @@ __PACKAGE__->register_method({
>                           );
>                           PVE::LXC::create_ifaces_ipams_ips($conf, $vmid) if $unique;
>                           my $lxc_setup = PVE::LXC::Setup->new($conf, $rootdir);
> -                        $lxc_setup->template_fixup($conf);
> +                        $lxc_setup->post_restore_hook($password, $ssh_keys);
>                       } else {
>                           my $lxc_setup = PVE::LXC::Setup->new($conf, $rootdir); # detect OS
>                           PVE::LXC::Config->write_config($vmid, $conf); # safe config (after OS detection)
> diff --git a/src/PVE/LXC/Setup.pm b/src/PVE/LXC/Setup.pm
> index d936af2..58cadba 100644
> --- a/src/PVE/LXC/Setup.pm
> +++ b/src/PVE/LXC/Setup.pm
> @@ -329,6 +329,15 @@ sub post_create_hook {
>       $self->check_systemd_nesting();
>   }
>   
> +sub post_restore_hook {
> +    my ($self, $root_password, $ssh_keys) = @_;
> +
> +    $self->protected_call(sub {
> +        $self->{plugin}->post_restore_hook($self->{conf}, $root_password, $ssh_keys);
> +    });
> +    $self->check_systemd_nesting();
> +}
> +
>   sub unified_cgroupv2_support {
>       my ($self) = @_;
>   
> diff --git a/src/PVE/LXC/Setup/Base.pm b/src/PVE/LXC/Setup/Base.pm
> index 567f9d6..f0e1f01 100644
> --- a/src/PVE/LXC/Setup/Base.pm
> +++ b/src/PVE/LXC/Setup/Base.pm
> @@ -735,6 +735,15 @@ sub post_create_hook {
>       # fixme: what else ?
>   }
>   
> +sub post_restore_hook {
> +    my ($self, $conf, $root_password, $ssh_keys) = @_;
> +
> +    $self->template_fixup($conf);
> +
> +    $self->set_user_password($conf, 'root', $root_password) if defined($root_password);
> +    $self->set_user_authorized_ssh_keys($conf, 'root', $ssh_keys) if defined($ssh_keys);

The `post_create_hook` uses `if $ssh_keys`, so I think this should also 
be used here instead of `if defined($ssh_keys)`?

> +}
> +
>   # File access wrappers for container setup code.
>   # NOTE: those are not direct part of the Plugin API (yet), avoid using them outside the child plugins
>   # For user-namespace support these might need to take uid and gid maps into account.
> diff --git a/src/PVE/LXC/Setup/Plugin.pm b/src/PVE/LXC/Setup/Plugin.pm
> index 95a633f..6ba656b 100644
> --- a/src/PVE/LXC/Setup/Plugin.pm
> +++ b/src/PVE/LXC/Setup/Plugin.pm
> @@ -99,4 +99,9 @@ sub post_create_hook {
>       croak "implement me in sub-class";
>   }
>   
> +sub post_restore_hook {
> +    my ($self, $conf, $root_password, $ssh_keys) = @_;
> +    croak "implement me in sub-class";
> +}
> +
>   1;
> diff --git a/src/PVE/LXC/Setup/Unmanaged.pm b/src/PVE/LXC/Setup/Unmanaged.pm
> index b51be55..0b440cf 100644
> --- a/src/PVE/LXC/Setup/Unmanaged.pm
> +++ b/src/PVE/LXC/Setup/Unmanaged.pm
> @@ -84,4 +84,8 @@ sub post_create_hook {
>       my ($self, $conf, $root_password, $ssh_keys) = @_;
>   }
>   
> +sub post_restore_hook {
> +    my ($self, $conf, $root_password, $ssh_keys) = @_;
> +}
> +
>   1;