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

[pve-devel] [PATCH 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)

Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
---
 src/PVE/API2/LXC.pm | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
index 8ce462f..4168a7c 100644
--- a/src/PVE/API2/LXC.pm
+++ b/src/PVE/API2/LXC.pm
@@ -362,6 +362,10 @@ __PACKAGE__->register_method({
 			# '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};
+
+			# we also need to make sure the privileged/unprivileged bit is recovered
+			# from the old config if the parameter is not passed
+			$conf->{unprivileged} = $orig_conf->{unprivileged} if !defined $unprivileged && $orig_conf->{unprivileged};
 		    }
 		}
 		if ($storage_only_mode) {
-- 
2.20.1

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

[Fabian Ebner]

Am 22.02.21 um 16:03 schrieb 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)
> 
> Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
> ---
>   src/PVE/API2/LXC.pm | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
> index 8ce462f..4168a7c 100644
> --- a/src/PVE/API2/LXC.pm
> +++ b/src/PVE/API2/LXC.pm
> @@ -362,6 +362,10 @@ __PACKAGE__->register_method({
>   			# '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};
> +
> +			# we also need to make sure the privileged/unprivileged bit is recovered
> +			# from the old config if the parameter is not passed
> +			$conf->{unprivileged} = $orig_conf->{unprivileged} if !defined $unprivileged && $orig_conf->{unprivileged};

This is guarded by a
     if ($is_root && $archive ne '-') {
but the unprivileged flag should be recovered for all users or am I 
missing something?

The existing $was_template logic probably shouldn't be guarded by 
$is_root either...

>   		    }
>   		}
>   		if ($storage_only_mode) {
> 

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

[Thomas Lamprecht]

On 22.02.21 16:03, 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)
> 

some comments on top of Fabis review (thanks for that!)

> Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
> ---
>  src/PVE/API2/LXC.pm | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
> index 8ce462f..4168a7c 100644
> --- a/src/PVE/API2/LXC.pm
> +++ b/src/PVE/API2/LXC.pm
> @@ -362,6 +362,10 @@ __PACKAGE__->register_method({
>  			# '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};
> +
> +			# we also need to make sure the privileged/unprivileged bit is recovered
> +			# from the old config if the parameter is not passed


also shorten the comment when sending a v2, either omit it as its clear what happens
and this is not a manual edge case like "lxc" (which is not handled by our API).
If use a short "ensure to restore privileged level if not overwritten", but its just
redundant..


> +			$conf->{unprivileged} = $orig_conf->{unprivileged} if !defined $unprivileged && $orig_conf->{unprivileged};

1. seems like a pretty long line, is this under the 100 cc max?

2. should the check be: !defined($unprivileged) && defined($orig_conf->{unprivileged});

3. nit: we normally use parentheses for defined()


>  		    }
>  		}
>  		if ($storage_only_mode) {
>