From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [45.144.208.40]) by lore.proxmox.com (Postfix) with ESMTPS id CFE761FF0E2 for ; Thu, 30 Jul 2026 12:44:05 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 6D89B213D3; Thu, 30 Jul 2026 12:44:05 +0200 (CEST) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Thu, 30 Jul 2026 12:43:59 +0200 Message-Id: Subject: Re: [PATCH container v2] fix #7164: lxc: restore: apply password and ssh keys during container restore From: "Elias Huhsovitz" To: "Erik Fastermann" , X-Mailer: aerc 0.20.0 References: <20260729082545.17885-1-e.huhsovitz@proxmox.com> <2f38582b-6c68-427f-94b7-1eab09dec34e@proxmox.com> In-Reply-To: <2f38582b-6c68-427f-94b7-1eab09dec34e@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1785408231666 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.124 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) RCVD_IN_DNSWL_LOW -0.7 Sender listed at https://www.dnswl.org/, low trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: KFTFP4YS7BOMWOLOYAWIPPFY4KQ5AQXP X-Message-ID-Hash: KFTFP4YS7BOMWOLOYAWIPPFY4KQ5AQXP X-MailFrom: e.huhsovitz@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: Thanks for the quick reply, i have added my comments below. On Wed Jul 29, 2026 at 4:20 PM CEST, Erik Fastermann wrote: > The commit message is a little long. I would just shorten it to 'fix=20 > #7164: restore: apply password and ssh keys', as lxc is obvious since we= =20 > are in pve-container and 'during container restore' is clear from the=20 > 'restore:' tag. Fixed, thanks. >> 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. >>=20 >> Introduce a new `post_restore_hook` in the LXC setup plugin >> architecture. This hook applies the provided credentials if they are >> defined. >>=20 >> Add `check_systemd_nesting` to the `post_restore_hook`. > > Why is this added? A rational here would be helpful. If the reason is=20 > just symmetry with `post_create_hook`, we might need to be careful here,= =20 > as this could introduce new unwanted failure conditions. I have not=20 > checked in detail, but some of the code can die. I believed adding this check might make the restore process more resiliant since the user might get important warnings early, thus reducing the amount= =20 of troubleshooting needed. But as you pointed out, this could lead to a regression. Since a restore is technically a creating a new contaienrs, i felt as=20 though adding this check made sense. Perhaps the `die` is a good thing?=20 Alerting the user early regarding potential issues, might be beneficial. I am curious to know what your opinion is on this! After more consideration= =20 I am strongly considering omitting the check. >>=20 >> Move `template_fixup`from the API endpoint function to the > > Nit: Missing space (`template_fixup`from) Fixed, thanks. > >> `post_restore_hook`, matching the `post_create_hook` logic. >>=20 >> Signed-off-by: Elias Huhsovitz >> --- >>=20 >> v1: https://lore.proxmox.com/all/20260716153959.183045-1-e.huhsovitz@pro= xmox.com/ >>=20 >> Changes made according to the feedback (big thanks): >> https://lore.proxmox.com/all/95a38704-543e-4820-b36c-696cf192a684@proxmo= x.com/ >>=20 >> 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 >>=20 >> 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(-) >>=20 >> 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 =3D PVE::LXC::Setup->new($conf, = $rootdir); >> - $lxc_setup->template_fixup($conf); >> + $lxc_setup->post_restore_hook($password, $ssh_k= eys); >> } else { >> my $lxc_setup =3D 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(); >> } >> =20 >> +sub post_restore_hook { >> + my ($self, $root_password, $ssh_keys) =3D @_; >> + >> + $self->protected_call(sub { >> + $self->{plugin}->post_restore_hook($self->{conf}, $root_passwor= d, $ssh_keys); >> + }); >> + $self->check_systemd_nesting(); >> +} >> + >> sub unified_cgroupv2_support { >> my ($self) =3D @_; >> =20 >> 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 ? >> } >> =20 >> +sub post_restore_hook { >> + my ($self, $conf, $root_password, $ssh_keys) =3D @_; >> + >> + $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 de= fined($ssh_keys); > > The `post_create_hook` uses `if $ssh_keys`, so I think this should also= =20 > be used here instead of `if defined($ssh_keys)`? Yes, i agree. Originally i wanted to avoid truthy/falsy checks, but my implementation would return true if the user passed an empty string, leading to an unnecessary operation. I will change it to `if $ssh_keys` in v3. >> +} >> + >> # 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"; >> } >> =20 >> +sub post_restore_hook { >> + my ($self, $conf, $root_password, $ssh_keys) =3D @_; >> + croak "implement me in sub-class"; >> +} >> + >> 1; >> diff --git a/src/PVE/LXC/Setup/Unmanaged.pm b/src/PVE/LXC/Setup/Unmanage= d.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) =3D @_; >> } >> =20 >> +sub post_restore_hook { >> + my ($self, $conf, $root_password, $ssh_keys) =3D @_; >> +} >> + >> 1;