[pve-devel] [PATCH pve-container 1/2] pct: set worker user for pull_file/push_file calls

[pve-devel] [PATCH pve-container 1/2] pct: set worker user for pull_file/push_file calls

[Oguz Bektas]

was previously unset, causing a 'root@pve' to show up in the task logs
instead of the regular 'root@pam'.

Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
---
 src/PVE/CLI/pct.pm | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/src/PVE/CLI/pct.pm b/src/PVE/CLI/pct.pm
index 462917b..99c160c 100755
--- a/src/PVE/CLI/pct.pm
+++ b/src/PVE/CLI/pct.pm
@@ -544,6 +544,8 @@ __PACKAGE__->register_method({
 
 	my $rpcenv = PVE::RPCEnvironment::get();
 
+	my $authuser = $rpcenv->get_user();
+
 	my $vmid = extract_param($param, 'vmid');
 	my $path = extract_param($param, 'path');
 	my $dest = extract_param($param, 'destination');
@@ -578,7 +580,7 @@ __PACKAGE__->register_method({
 	    };
 
 	    # This avoids having to setns() back to our namespace.
-	    return $rpcenv->fork_worker('pull_file', $vmid, undef, $realcmd);
+	    return $rpcenv->fork_worker('pull_file', $vmid, $authuser, $realcmd);
 	};
 
 	return PVE::LXC::Config->lock_config($vmid, $code);
@@ -627,6 +629,8 @@ __PACKAGE__->register_method({
 
 	my $rpcenv = PVE::RPCEnvironment::get();
 
+	my $authuser = $rpcenv->get_user();
+
 	my $vmid = extract_param($param, 'vmid');
 	my $file = extract_param($param, 'file');
 	my $dest = extract_param($param, 'destination');
@@ -682,7 +686,7 @@ __PACKAGE__->register_method({
 	    };
 
 	    # This avoids having to setns() back to our namespace.
-	    return $rpcenv->fork_worker('push_file', $vmid, undef, $realcmd);
+	    return $rpcenv->fork_worker('push_file', $vmid, $authuser, $realcmd);
 	};
 
 	return PVE::LXC::Config->lock_config($vmid, $code);
-- 
2.30.2

[pve-devel] [PATCH common 2/2] REST environment: default to 'root@pam' for forked workers in case no user was specified

[Oguz Bektas]

previously we had a default of 'root@pve', which doesn't exist.
since the username is only relevant for the task logs, we can change it
to 'root@pam' without ill effects.

also add a warning in case there are other call sites that we missed
where fork_worker is called without a user variable (found call sites
only in pve-container where this was unset, namely in 'push_file' and
'pull_file').

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

diff --git a/src/PVE/RESTEnvironment.pm b/src/PVE/RESTEnvironment.pm
index 1b2af08..5352aad 100644
--- a/src/PVE/RESTEnvironment.pm
+++ b/src/PVE/RESTEnvironment.pm
@@ -492,7 +492,10 @@ sub fork_worker {
     $dtype = 'unknown' if !defined ($dtype);
     $id = '' if !defined ($id);
 
-    $user = 'root@pve' if !defined ($user);
+    if (!defined($user)) {
+	warn 'Worker user was not specified, defaulting to "root@pam"!';
+	$user = 'root@pam';
+    }
 
     my $sync = ($self->{type} eq 'cli' && !$background) ? 1 : 0;
 
-- 
2.30.2

Re: [pve-devel] [PATCH common 2/2] REST environment: default to 'root@pam' for forked workers in case no user was specified

[Fabian Ebner]

Am 07.03.22 um 10:20 schrieb Oguz Bektas:
> previously we had a default of 'root@pve', which doesn't exist.
> since the username is only relevant for the task logs, we can change it
> to 'root@pam' without ill effects.
> 
> also add a warning in case there are other call sites that we missed
> where fork_worker is called without a user variable (found call sites
> only in pve-container where this was unset, namely in 'push_file' and
> 'pull_file').
> 
> Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
> ---
>  src/PVE/RESTEnvironment.pm | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/src/PVE/RESTEnvironment.pm b/src/PVE/RESTEnvironment.pm
> index 1b2af08..5352aad 100644
> --- a/src/PVE/RESTEnvironment.pm
> +++ b/src/PVE/RESTEnvironment.pm
> @@ -492,7 +492,10 @@ sub fork_worker {
>      $dtype = 'unknown' if !defined ($dtype);
>      $id = '' if !defined ($id);
>  
> -    $user = 'root@pve' if !defined ($user);

It'd be a bit cleaner to default to $self->get_user(1) first and only if
that's not set to something else. Maybe the warning is only needed in
the latter case. Would also make the first patch unnecessary, although
it doesn't hurt of course.

> +    if (!defined($user)) {
> +	warn 'Worker user was not specified, defaulting to "root@pam"!';

Could mention that it's an internal error. And could use $self->warn().

> +	$user = 'root@pam';
> +    }
>  
>      my $sync = ($self->{type} eq 'cli' && !$background) ? 1 : 0;
>  

Re: [pve-devel] [PATCH common 2/2] REST environment: default to 'root@pam' for forked workers in case no user was specified

[Fabian Ebner]

Am 10.03.22 um 13:01 schrieb Fabian Ebner:
> Am 07.03.22 um 10:20 schrieb Oguz Bektas:
>> +    if (!defined($user)) {
>> +	warn 'Worker user was not specified, defaulting to "root@pam"!';
> 
> Could mention that it's an internal error. And could use $self->warn().

Shouldn't use $self->warn(), because the warning here won't show up in
the task log and it'd be very confusing to have a non-zero warning count
without warning.