[pve-devel] [PATCH v3 manager 1/3] add pveceph install to shell api

[Fabian Grünbichler]

meta:

it might still make sense to have all those identical options only
defined once (e.g., in a local variable in this module) to prevent
unnecessary churn when adding new commands.

ideally, we would also pull out the mapping from 'cmd' value to actual
command array, so that we only need to update one place when adding new
commands to the whitelist.

currently, we have three almost identical chunks of code:

vncshell (794ff):
my $shcmd;

if ($user eq 'root at pam') {
    if ($param->{upgrade}) {
        my $upgradecmd = "pveupgrade --shell";
        $upgradecmd = PVE::Tools::shellquote($upgradecmd) if $remip;
        $shcmd = [ '/bin/bash', '-c', $upgradecmd ];
    } elsif ($param->{cmd}) {
        if ($param->{cmd} eq 'ceph_install') {
            $shcmd = [ '/usr/bin/pveceph', 'install' ];
        }
    } else {
        $shcmd = [ '/bin/login', '-f', 'root' ];
    }
} else {
    $shcmd = [ '/bin/login' ];
}

termproxy (934ff):
my $concmd;

if ($user eq 'root at pam') {
    if ($param->{upgrade}) {
        $concmd = [ '/usr/bin/pveupgrade', '--shell' ];
    } elsif ($param->{cmd}) {
        if ($param->{cmd} eq 'ceph_install') {
            $concmd = [ '/usr/bin/pveceph', 'install' ];
        }
    } else {
        $concmd = [ '/bin/login', '-f', 'root' ];
    }
} else {
    $concmd = [ '/bin/login' ];
}

spiceshell (1073ff):
my $shcmd;

if ($user eq 'root at pam') {
    if ($param->{upgrade}) {
        my $upgradecmd = "pveupgrade --shell";
        $shcmd = [ '/bin/bash', '-c', $upgradecmd ];
    } elsif ($param->{cmd}) {
        if ($param->{cmd} eq 'ceph_install') {
            $shcmd = [ '/usr/bin/pveceph', 'install' ];
        }
    } else {
        $shcmd = [ '/bin/login', '-f', 'root' ];
    }
} else {
    $shcmd = [ '/bin/login' ];
}

so we'd need to check whether / how we can unify the upgrade case if we
want a simple, single mapping from enum value / default case to command
array to reduce this duplication. permission checks for individual
whitelist entries could also be moved to such a mapping method (or to a
new, additional one that gets called before mapping if we want to have
the mapping as simple perl hash).

of course, this mapping can be easily done as a follow-up, together with
moving from the 'upgrade' parameter to 'cmd' => 'upgrade' and
deprecating the former.

On Tue, Jan 22, 2019 at 12:21:56PM +0100, Tim Marx wrote:
> Signed-off-by: Tim Marx <t.marx at proxmox.com>
> ---
> changed since v2:
> * removed get_standard_option
> 
>  PVE/API2/Nodes.pm | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
> 
> diff --git a/PVE/API2/Nodes.pm b/PVE/API2/Nodes.pm
> index 7f829b29..04fc4f08 100644
> --- a/PVE/API2/Nodes.pm
> +++ b/PVE/API2/Nodes.pm
> @@ -705,6 +705,12 @@ __PACKAGE__->register_method ({
>  		optional => 1,
>  		default => 0,
>  	    },
> +	    cmd => {
> +		type => 'string',
> +		description => "Run command instead of normal shell.",
> +		enum => ['ceph_install'],
> +		optional => 1,
> +	    },
>  	    websocket => {
>  		optional => 1,
>  		type => 'boolean',
> @@ -778,6 +784,10 @@ __PACKAGE__->register_method ({
>  		my $upgradecmd = "pveupgrade --shell";
>  		$upgradecmd = PVE::Tools::shellquote($upgradecmd) if $remip;
>  		$shcmd = [ '/bin/bash', '-c', $upgradecmd ];
> +	    } elsif ($param->{cmd}) {
> +		if ($param->{cmd} eq 'ceph_install') {
> +		    $shcmd = [ '/usr/bin/pveceph', 'install' ];
> +		}
>  	    } else {
>  		$shcmd = [ '/bin/login', '-f', 'root' ];
>  	    }
> @@ -864,6 +874,12 @@ __PACKAGE__->register_method ({
>  		optional => 1,
>  		default => 0,
>  	    },
> +	    cmd => {
> +		type => 'string',
> +		description => "Run command instead of normal shell.",
> +		enum => ['ceph_install'],
> +		optional => 1,
> +	    },
>  	},
>      },
>      returns => {
> @@ -908,6 +924,10 @@ __PACKAGE__->register_method ({
>  	if ($user eq 'root at pam') {
>  	    if ($param->{upgrade}) {
>  		$concmd = [ '/usr/bin/pveupgrade', '--shell' ];
> +	    } elsif ($param->{cmd}) {
> +		if ($param->{cmd} eq 'ceph_install') {
> +		    $concmd = [ '/usr/bin/pveceph', 'install' ];
> +		}
>  	    } else {
>  		$concmd = [ '/bin/login', '-f', 'root' ];
>  	    }
> @@ -1011,6 +1031,12 @@ __PACKAGE__->register_method ({
>  		optional => 1,
>  		default => 0,
>  	    },
> +	    cmd => {
> +		type => 'string',
> +		description => "Run command instead of normal shell.",
> +		enum => ['ceph_install'],
> +		optional => 1,
> +	    },
>  	},
>      },
>      returns => get_standard_option('remote-viewer-config'),
> @@ -1038,6 +1064,10 @@ __PACKAGE__->register_method ({
>  	    if ($param->{upgrade}) {
>  		my $upgradecmd = "pveupgrade --shell";
>  		$shcmd = [ '/bin/bash', '-c', $upgradecmd ];
> +	    } elsif ($param->{cmd}) {
> +		if ($param->{cmd} eq 'ceph_install') {
> +		    $shcmd = [ '/usr/bin/pveceph', 'install' ];
> +		}
>  	    } else {
>  		$shcmd = [ '/bin/login', '-f', 'root' ];
>  	    }
> -- 
> 2.11.0
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel at pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel