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

[Fabian Ebner <f.ebner@proxmox.com>]

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;
>