[pve-devel] [PATCH v3 container] fix #3313: recover unprivileged bit from old config during pct restore

[pve-devel] [PATCH v3 container] fix #3313: recover unprivileged bit from old config during pct restore

[Oguz Bektas]

since pct defaults to privileged containers, it restores the container
as privileged when `--unprivileged 1` is not passed.

instead we should check the old configuration and retrieve it
from there.

this way, when one creates an unprivileged container on GUI, it will be
still restored as unprivileged via pct (without having to pass
`--unprivileged 1` parameter)

note: $orig_mp_param assignment is not guarded by $is_root anymore, but
this should still be okay since we do a second recover_config() call on
the archive file if $orig_mp_param is used

Reviewed-by: Fabian Ebner <f.ebner@proxmox.com>
Tested-by: Fabian Ebner <f.ebner@proxmox.com>
Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
---

thanks fabi for testing and reviewing!

v2->v3:
* remove comment
* wrap post-if according to style guide


 src/PVE/API2/LXC.pm | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
index 8ce462f..df0cc88 100644
--- a/src/PVE/API2/LXC.pm
+++ b/src/PVE/API2/LXC.pm
@@ -352,7 +352,7 @@ __PACKAGE__->register_method({
 		my $orig_mp_param; # only used if $restore
 		if ($restore) {
 		    die "can't overwrite running container\n" if PVE::LXC::check_running($vmid);
-		    if ($is_root && $archive ne '-') {
+		    if ($archive ne '-') {
 			my $orig_conf;
 			print "recovering backed-up configuration from '$archive'\n";
 			($orig_conf, $orig_mp_param) = PVE::LXC::Create::recover_config($storage_cfg, $archive, $vmid);
@@ -361,7 +361,10 @@ __PACKAGE__->register_method({
 			# causing it to restore the raw lxc entries, among which there may be
 			# 'lxc.idmap' entries. We need to make sure that the extracted contents
 			# of the container match up with the restored configuration afterwards:
-			$conf->{lxc} = $orig_conf->{lxc};
+			$conf->{lxc} = $orig_conf->{lxc} if $is_root;
+
+			$conf->{unprivileged} = $orig_conf->{unprivileged}
+			    if !defined($unprivileged) && defined($orig_conf->{unprivileged});
 		    }
 		}
 		if ($storage_only_mode) {
-- 
2.20.1

Re: [pve-devel] [PATCH v3 container] fix #3313: recover unprivileged bit from old config during pct restore

[Thomas Lamprecht]

On 23.02.21 15:54, Oguz Bektas wrote:
> since pct defaults to privileged containers, it restores the container
> as privileged when `--unprivileged 1` is not passed.
> 
> instead we should check the old configuration and retrieve it
> from there.
> 
> this way, when one creates an unprivileged container on GUI, it will be
> still restored as unprivileged via pct (without having to pass
> `--unprivileged 1` parameter)
> 
> note: $orig_mp_param assignment is not guarded by $is_root anymore, but
> this should still be okay since we do a second recover_config() call on
> the archive file if $orig_mp_param is used
> 

misses noting the effect of the $is_root change, namely that templates are
now restored as such (i.e., actually another bug)

Late in the review, I know and if the commit message would had mentioned
this I would ignore it and push it out nonetheless, but as that was not
mentioned, lets actually split it up into two patches:
Fix restoring templates in the first and then just adding the conditional
unprivileged setting in the second.

thanks!

> Reviewed-by: Fabian Ebner <f.ebner@proxmox.com>
> Tested-by: Fabian Ebner <f.ebner@proxmox.com>
> Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
> ---
> 
> thanks fabi for testing and reviewing!
> 
> v2->v3:
> * remove comment
> * wrap post-if according to style guide
> 
> 
>  src/PVE/API2/LXC.pm | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
> index 8ce462f..df0cc88 100644
> --- a/src/PVE/API2/LXC.pm
> +++ b/src/PVE/API2/LXC.pm
> @@ -352,7 +352,7 @@ __PACKAGE__->register_method({
>  		my $orig_mp_param; # only used if $restore
>  		if ($restore) {
>  		    die "can't overwrite running container\n" if PVE::LXC::check_running($vmid);
> -		    if ($is_root && $archive ne '-') {
> +		    if ($archive ne '-') {
>  			my $orig_conf;
>  			print "recovering backed-up configuration from '$archive'\n";
>  			($orig_conf, $orig_mp_param) = PVE::LXC::Create::recover_config($storage_cfg, $archive, $vmid);
> @@ -361,7 +361,10 @@ __PACKAGE__->register_method({
>  			# causing it to restore the raw lxc entries, among which there may be
>  			# 'lxc.idmap' entries. We need to make sure that the extracted contents
>  			# of the container match up with the restored configuration afterwards:
> -			$conf->{lxc} = $orig_conf->{lxc};
> +			$conf->{lxc} = $orig_conf->{lxc} if $is_root;
> +
> +			$conf->{unprivileged} = $orig_conf->{unprivileged}
> +			    if !defined($unprivileged) && defined($orig_conf->{unprivileged});
>  		    }
>  		}
>  		if ($storage_only_mode) {
>