From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id 7B9381FF168 for ; Tue, 10 Dec 2024 19:53:01 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id D59BB35E28; Tue, 10 Dec 2024 19:53:03 +0100 (CET) Message-ID: <52991a04-5ea4-4b96-aea2-9e6983a536e1@proxmox.com> Date: Tue, 10 Dec 2024 19:52:28 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Beta To: Proxmox VE development discussion , Aaron Lauterer References: <20241206135514.170226-1-a.lauterer@proxmox.com> <20241206135514.170226-3-a.lauterer@proxmox.com> Content-Language: en-GB, de-AT From: Thomas Lamprecht Autocrypt: addr=t.lamprecht@proxmox.com; keydata= xsFNBFsLjcYBEACsaQP6uTtw/xHTUCKF4VD4/Wfg7gGn47+OfCKJQAD+Oyb3HSBkjclopC5J uXsB1vVOfqVYE6PO8FlD2L5nxgT3SWkc6Ka634G/yGDU3ZC3C/7NcDVKhSBI5E0ww4Qj8s9w OQRloemb5LOBkJNEUshkWRTHHOmk6QqFB/qBPW2COpAx6oyxVUvBCgm/1S0dAZ9gfkvpqFSD 90B5j3bL6i9FIv3YGUCgz6Ue3f7u+HsEAew6TMtlt90XV3vT4M2IOuECG/pXwTy7NtmHaBQ7 UJBcwSOpDEweNob50+9B4KbnVn1ydx+K6UnEcGDvUWBkREccvuExvupYYYQ5dIhRFf3fkS4+ wMlyAFh8PQUgauod+vqs45FJaSgTqIALSBsEHKEs6IoTXtnnpbhu3p6XBin4hunwoBFiyYt6 YHLAM1yLfCyX510DFzX/Ze2hLqatqzY5Wa7NIXqYYelz7tXiuCLHP84+sV6JtEkeSUCuOiUY virj6nT/nJK8m0BzdR6FgGtNxp7RVXFRz/+mwijJVLpFsyG1i0Hmv2zTn3h2nyGK/I6yhFNt dX69y5hbo6LAsRjLUvZeHXpTU4TrpN/WiCjJblbj5um5eEr4yhcwhVmG102puTtuCECsDucZ jpKpUqzXlpLbzG/dp9dXFH3MivvfuaHrg3MtjXY1i+/Oxyp5iwARAQABzTNUaG9tYXMgTGFt cHJlY2h0IChBdXRoLTQpIDx0LmxhbXByZWNodEBwcm94bW94LmNvbT7CwY4EEwEIADgWIQQO R4qbEl/pah9K6VrTZCM6gDZWBgUCWwuNxgIbAwULCQgHAgYVCAkKCwIEFgIDAQIeAQIXgAAK CRDTZCM6gDZWBm/jD/4+6JB2s67eaqoP6x9VGaXNGJPCscwzLuxDTCG90G9FYu29VcXtubH/ bPwsyBbNUQpqTm/s4XboU2qpS5ykCuTjqavrcP33tdkYfGcItj2xMipJ1i3TWvpikQVsX42R G64wovLs/dvpTYphRZkg5DwhgTmy3mRkmofFCTa+//MOcNOORltemp984tWjpR3bUJETNWpF sKGZHa3N4kCNxb7A+VMsJZ/1gN3jbQbQG7GkJtnHlWkw9rKCYqBtWrnrHa4UAvSa9M/XCIAB FThFGqZI1ojdVlv5gd6b/nWxfOPrLlSxbUo5FZ1i/ycj7/24nznW1V4ykG9iUld4uYUY86bB UGSjew1KYp9FmvKiwEoB+zxNnuEQfS7/Bj1X9nxizgweiHIyFsRqgogTvLh403QMSGNSoArk tqkorf1U+VhEncIn4H3KksJF0njZKfilrieOO7Vuot1xKr9QnYrZzJ7m7ZxJ/JfKGaRHXkE1 feMmrvZD1AtdUATZkoeQtTOpMu4r6IQRfSdwm/CkppZXfDe50DJxAMDWwfK2rr2bVkNg/yZI tKLBS0YgRTIynkvv0h8d9dIjiicw3RMeYXyqOnSWVva2r+tl+JBaenr8YTQw0zARrhC0mttu cIZGnVEvQuDwib57QLqMjQaC1gazKHvhA15H5MNxUhwm229UmdH3KM7BTQRbC43GARAAyTkR D6KRJ9Xa2fVMh+6f186q0M3ni+5tsaVhUiykxjsPgkuWXWW9MbLpYXkzX6h/RIEKlo2BGA95 QwG5+Ya2Bo3g7FGJHAkXY6loq7DgMp5/TVQ8phsSv3WxPTJLCBq6vNBamp5hda4cfXFUymsy HsJy4dtgkrPQ/bnsdFDCRUuhJHopnAzKHN8APXpKU6xV5e3GE4LwFsDhNHfH/m9+2yO/trcD txSFpyftbK2gaMERHgA8SKkzRhiwRTt9w5idOfpJVkYRsgvuSGZ0pcD4kLCOIFrer5xXudk6 NgJc36XkFRMnwqrL/bB4k6Pi2u5leyqcXSLyBgeHsZJxg6Lcr2LZ35+8RQGPOw9C0ItmRjtY ZpGKPlSxjxA1WHT2YlF9CEt3nx7c4C3thHHtqBra6BGPyW8rvtq4zRqZRLPmZ0kt/kiMPhTM 8wZAlObbATVrUMcZ/uNjRv2vU9O5aTAD9E5r1B0dlqKgxyoImUWB0JgpILADaT3VybDd3C8X s6Jt8MytUP+1cEWt9VKo4vY4Jh5vwrJUDLJvzpN+TsYCZPNVj18+jf9uGRaoK6W++DdMAr5l gQiwsNgf9372dbMI7pt2gnT5/YdG+ZHnIIlXC6OUonA1Ro/Itg90Q7iQySnKKkqqnWVc+qO9 GJbzcGykxD6EQtCSlurt3/5IXTA7t6sAEQEAAcLBdgQYAQgAIBYhBA5HipsSX+lqH0rpWtNk IzqANlYGBQJbC43GAhsMAAoJENNkIzqANlYGD1sP/ikKgHgcspEKqDED9gQrTBvipH85si0j /Jwu/tBtnYjLgKLh2cjv1JkgYYjb3DyZa1pLsIv6rGnPX9bH9IN03nqirC/Q1Y1lnbNTynPk IflgvsJjoTNZjgu1wUdQlBgL/JhUp1sIYID11jZphgzfDgp/E6ve/8xE2HMAnf4zAfJaKgD0 F+fL1DlcdYUditAiYEuN40Ns/abKs8I1MYx7Yglu3RzJfBzV4t86DAR+OvuF9v188WrFwXCS RSf4DmJ8tntyNej+DVGUnmKHupLQJO7uqCKB/1HLlMKc5G3GLoGqJliHjUHUAXNzinlpE2Vj C78pxpwxRNg2ilE3AhPoAXrY5qED5PLE9sLnmQ9AzRcMMJUXjTNEDxEYbF55SdGBHHOAcZtA kEQKub86e+GHA+Z8oXQSGeSGOkqHi7zfgW1UexddTvaRwE6AyZ6FxTApm8wq8NT2cryWPWTF BDSGB3ujWHMM8ERRYJPcBSjTvt0GcEqnd+OSGgxTkGOdufn51oz82zfpVo1t+J/FNz6MRMcg 8nEC+uKvgzH1nujxJ5pRCBOquFZaGn/p71Yr0oVitkttLKblFsqwa+10Lt6HBxm+2+VLp4Ja 0WZNncZciz3V3cuArpan/ZhhyiWYV5FD0pOXPCJIx7WS9PTtxiv0AOS4ScWEUmBxyhFeOpYa DrEx In-Reply-To: <20241206135514.170226-3-a.lauterer@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL -0.046 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [perl.org, pool.pm] Subject: Re: [pve-devel] [PATCH manager 2/7] api: ceph: add rbd namespace management endpoints X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox VE development discussion Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pve-devel-bounces@lists.proxmox.com Sender: "pve-devel" 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 > --- > 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