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@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@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@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.m...@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@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@pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
_______________________________________________
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
