public inbox for pve-devel@lists.proxmox.com
 help / color / mirror / Atom feed
From: Thomas Lamprecht <t.lamprecht@proxmox.com>
To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>,
	Aaron Lauterer <a.lauterer@proxmox.com>
Subject: Re: [pve-devel] [PATCH manager 2/7] api: ceph: add rbd namespace management endpoints
Date: Tue, 10 Dec 2024 19:52:28 +0100	[thread overview]
Message-ID: <52991a04-5ea4-4b96-aea2-9e6983a536e1@proxmox.com> (raw)
In-Reply-To: <20241206135514.170226-3-a.lauterer@proxmox.com>

Am 06.12.24 um 14:55 schrieb Aaron Lauterer:
> RBD supports namespaces. To make the management easier and possible via
> the web UI, we need to add API endpoints to:
> * list
> * create
> * delete
> namespaces.
> 
> We only allow creatng namespaces for pools that have the RBD application
> set.
> 
> Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
> ---
>  PVE/API2/Ceph/Pool.pm | 182 +++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 180 insertions(+), 2 deletions(-)
> 
> diff --git a/PVE/API2/Ceph/Pool.pm b/PVE/API2/Ceph/Pool.pm
> index 5ee982f4..47194245 100644
> --- a/PVE/API2/Ceph/Pool.pm
> +++ b/PVE/API2/Ceph/Pool.pm
> @@ -3,6 +3,8 @@ package PVE::API2::Ceph::Pool;
>  use strict;
>  use warnings;
>  
> +use JSON;

The JSON module exports quite a few methods by default, so I'd prefer importing
the few required manually, e.g. here:

use JSON qw(decode_json);

> +
>  use PVE::Ceph::Tools;
>  use PVE::Ceph::Services;
>  use PVE::JSONSchema qw(get_standard_option parse_property_string);
> @@ -10,7 +12,7 @@ use PVE::RADOS;
>  use PVE::RESTHandler;
>  use PVE::RPCEnvironment;
>  use PVE::Storage;
> -use PVE::Tools qw(extract_param);
> +use PVE::Tools qw(extract_param run_command);
>  
>  use PVE::API2::Storage::Config;
>  
> @@ -302,7 +304,7 @@ my $ceph_pool_common_options = sub {
>  
>  
>  my $add_storage = sub {
> -    my ($pool, $storeid, $ec_data_pool) = @_;
> +    my ($pool, $storeid, $ec_data_pool, $namespace) = @_;
>  
>      my $storage_params = {
>  	type => 'rbd',
> @@ -312,6 +314,8 @@ my $add_storage = sub {
>  	content => 'rootdir,images',
>      };
>  
> +    $storage_params->{namespace} = $namespace if $namespace;
> +
>      $storage_params->{'data-pool'} = $ec_data_pool if $ec_data_pool;
>  
>      PVE::API2::Storage::Config->create($storage_params);
> @@ -798,4 +802,178 @@ __PACKAGE__->register_method ({
>      }});
>  
>  
> +my $get_rbd_namespaces = sub {
> +    my ($pool) = @_;
> +
> +    my $cmd = ['/usr/bin/rbd', 'namespace', 'list', $pool, '--format', 'json'];
> +    my $raw = '';
> +    my $parser = sub { $raw .= shift };
> +    run_command($cmd, errmsg => "rbd error", errfunc => sub {}, outfunc => $parser);

style nit: I'd avoid the intermediate variables, they do not gain much on
readability or the like here IMO, so maybe do something like:

run_command(
    ['/usr/bin/rbd', 'namespace', 'list', $pool, '--format', 'json'],
    outfunc => sub { $raw .= shift },
    errmsg => "rbd error",
    errfunc => sub {},
);


> +    return [] if $raw eq '[]';
> +
> +    my $decoded;
> +    if ($raw =~ m/^(\[\{.*\}\])$/s) { #untaint

tiny nit: missing whitespace in comment

Besides that, feels a bit rigid, as any whitespace separator between the braces would
cause a false positive here. Maybe do a very generous /(.+)/ untaint and instead wrap
the decode_json inside an eval and throw a telling error there? E.g.:

my ($untainted_raw) = $raw =~ /^(.+)$/;

my $namespaces = eval { decode_json($untainted_raw) };
die "failed to parse as JSON - $@\n" if $@;

> +	$decoded = JSON::decode_json($1);

It would be great if decode_json acts as untaint, but one can only wish.

> +    } else {
> +	die "got unexpected data from rbd namespace list: '${raw}'\n";
> +    }
> +
> +    my $result = [];
> +    for my $val (@$decoded) {
> +	    push @$result, { name => $val->{name} };
> +    }

FWIW, could be shortened using map [0]:

return [
    map { { name => $val->{name} } } $namespaces->@*
];

That is normally a bit faster too, but won't matter here, so no hard feelings from
my side.

[0]: https://perldoc.perl.org/functions/map

> +    return $result;
> +};
> +
> +__PACKAGE__->register_method ({
> +    name => 'listnamespaces',
> +    path => '{name}/namespace',
> +    method => 'GET',
> +    permissions => {
> +	check => ['perm', '/', [ 'Sys.Audit', 'Datastore.Audit' ], any => 1],

Hmm, do we already use Datastore.Audit for similar things? As feels a bit odd,
the pool just because a pool is often a PVE storage I'd still see this as system
information. If we already use this check for similar things I'm fine to keep this
consistent, otherwise I'd maybe use only Sys.Audit here, we can still open this up
in the future if it's really a problem.

> +    },
> +    description => "Get pool RBD namespace index.",

this isn't a index in the sense of that there are further API leaf nodes below it,
so maybe just "Get pool RBD namespaces".

> +    proxyto => 'node',
> +    protected => 1,
> +    parameters => {
> +	additionalProperties => 0,
> +	properties => {
> +	    node => get_standard_option('pve-node'),
> +	    name => {
> +		description => 'The name of the pool.',
> +		type => 'string',
> +		default => 'rbd',
> +	    },
> +	},
> +    },
> +    returns => {
> +	type => 'array',
> +	items => {
> +	    type => 'object',
> +	    properties => {
> +		name => { type => 'string', title => "Namespace" }
> +	    },
> +	},
> +    },
> +    code => sub {
> +	my ($param) = @_;
> +
> +	my $pool = extract_param($param, 'name') // 'rbd';
> +	return $get_rbd_namespaces->($pool);
> +    }});
> +
> +
> +__PACKAGE__->register_method ({
> +    name => 'createnamespace',
> +    path => '{name}/namespace',
> +    method => 'POST',
> +    permissions => {
> +	check => ['perm', '/', [ 'Sys.Modify' ]],
> +    },
> +    description => "Create new RBD namespace.",
> +    proxyto => 'node',
> +    protected => 1,
> +    parameters => {
> +	additionalProperties => 0,
> +	properties => {
> +	    node => get_standard_option('pve-node'),
> +	    name => {
> +		description => 'The name of the pool.',
> +		type => 'string',
> +		default => 'rbd',
> +	    },
> +	    namespace => {
> +		description => 'The name of the new namespace',
> +		type => 'string',
> +	    },
> +	    'add-storage' => {
> +		description => "Configure VM and CT storage using the new namespace.",
> +		type => 'boolean',
> +		optional => 1,
> +		default => "0",
> +	    },
> +	},
> +    },
> +    returns => { type => 'string' },
> +    code => sub {
> +	my ($param) = @_;
> +
> +	my $pool = extract_param($param, 'name') // 'rbd';
> +	my $namespace = extract_param($param, 'namespace');
> +	my $add_storages = extract_param($param, 'add-storage');

This now allows anyone with Sys.Modify to create a datastore, but we should check for
Datastore.Allocate, just like the create pool API endpoint does.

Similarly, we should ensure that the resulting storage name, i.e. something like:

if ($add_storages) {
    $rpcenv->check($user, '/storage', ['Datastore.Allocate']);
    die "namespace name contains characters which are illegal for storage naming\n"
        if !PVE::JSONSchema::parse_storage_id("${pool}-${namespace}");
}

> +
> +	die "specify namespace" if !$namespace;

Is this for an empty string being passed, tbh. I do not know from top of my head.

> +
> +	my $rados = PVE::RADOS->new();
> +	my $apps = $rados->mon_command({ prefix => "osd pool application get", pool => "$pool", });
> +	die "the pool does not have application 'rbd' enabled" if !defined($apps->{rbd});
> +
> +	my $current_namespaces = { map { $_->{name} => 1 } $get_rbd_namespaces->($pool)->@*};
> +	die "namespace already exists" if $current_namespaces->{$namespace};
> +
> +	my $cmd = ['/usr/bin/rbd', 'namespace', 'create', "${pool}/${namespace}"];
> +
> +	my $rpcenv = PVE::RPCEnvironment::get();
> +	my $user = $rpcenv->get_user();
> +	my $worker = sub {
> +	    my $raw = '';
> +	    my $parser = sub { $raw .= shift };

Above $raw and $parser are unused here.

> +	    run_command($cmd, errmsg => "rbd create namespace error", errfunc => sub {}, outfunc => $parser);

Might be nicer to read to have the $cmd inlined here in terms of code locality.

> +	    if ($add_storages) {
> +		eval { $add_storage->($pool, "${pool}-${namespace}", 0, $namespace) };
> +		die "adding PVE storage for ceph rbd namespace failed: pool: ${pool}, namespace: ${namespace}: $@\n" if $@;
> +	    }
> +	};
> +
> +	return $rpcenv->fork_worker('cephcreaterbdnamespace', $pool,  $user, $worker);
> +    }});
> +
> +
> +__PACKAGE__->register_method ({
> +    name => 'destroynamespace',
> +    path => '{name}/namespace',
> +    method => 'DELETE',
> +    permissions => {
> +	check => ['perm', '/', [ 'Sys.Modify' ]],
> +    },
> +    description => "Delete RBD namespace.",
> +    proxyto => 'node',
> +    protected => 1,
> +    parameters => {
> +	additionalProperties => 0,
> +	properties => {
> +	    node => get_standard_option('pve-node'),
> +	    name => {
> +		description => 'The name of the pool.',
> +		type => 'string',
> +		default => 'rbd',
> +	    },
> +	    namespace => {
> +		description => 'The name of the new namespace',
> +		type => 'string',
> +	    },
> +	},
> +    },
> +    returns => { type => 'string' },
> +    code => sub {
> +	my ($param) = @_;
> +
> +	my $pool = extract_param($param, 'name') // 'rbd';
> +	my $namespace = extract_param($param, 'namespace');
> +
> +	die "specify namespace" if !$namespace;

If required you could do 

my $namespace = extract_param($param, 'namespace') or die "...";

But if one cannot already pass an empty string that might not be required.

But w.r.t. error checking: might be good to test if the namespace is in use in any
PVE storage? As in that case, it would be probably better to require that storage
entry being deleted first.

> +
> +	my $cmd = ['/usr/bin/rbd', 'namespace', 'remove', "${pool}/${namespace}"];
> +
> +	my $rpcenv = PVE::RPCEnvironment::get();
> +	my $user = $rpcenv->get_user();
> +	my $worker = sub {
> +	    my $raw = '';
> +	    my $parser = sub { $raw .= shift };

Above $raw and $parser are unused here.

> +	    run_command($cmd, errmsg => "rbd create namespace error", errfunc => sub {}, outfunc => $parser);

Same w.r.t. inlining $cmd.

> +	};
> +
> +	return $rpcenv->fork_worker('cephdestroyrbdnamespace', $pool,  $user, $worker);
> +    }});
> +
>  1;



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel


  reply	other threads:[~2024-12-10 18:53 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-12-06 13:55 [pve-devel] [PATCH manager, docs 0/7] Ceph: add RBD Namespace management Aaron Lauterer
2024-12-06 13:55 ` [pve-devel] [PATCH manager 1/7] ui: ceph pool: add columns for application Aaron Lauterer
2024-12-10 18:56   ` [pve-devel] applied: " Thomas Lamprecht
2024-12-06 13:55 ` [pve-devel] [PATCH manager 2/7] api: ceph: add rbd namespace management endpoints Aaron Lauterer
2024-12-10 18:52   ` Thomas Lamprecht [this message]
2024-12-06 13:55 ` [pve-devel] [PATCH manager 3/7] pveceph: add pool namespace subcommands Aaron Lauterer
2024-12-06 13:55 ` [pve-devel] [PATCH manager 4/7] ui: ceph pool: add rbd namespace panel Aaron Lauterer
2024-12-06 13:55 ` [pve-devel] [PATCH manager 5/7] ui: utils: add ceph rbd namespace task names Aaron Lauterer
2024-12-06 13:55 ` [pve-devel] [PATCH manager 6/7] ui: storage rbd: remove hint for manual rbd namespace creation Aaron Lauterer
2024-12-06 13:55 ` [pve-devel] [PATCH docs 7/7] pveceph: add section for rbd namespaces Aaron Lauterer

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=52991a04-5ea4-4b96-aea2-9e6983a536e1@proxmox.com \
    --to=t.lamprecht@proxmox.com \
    --cc=a.lauterer@proxmox.com \
    --cc=pve-devel@lists.proxmox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal