[pve-devel] [PATCH manager 1/2] Fix #2053: OSD destroy only on specified node

[pve-devel] [PATCH manager 1/2] Fix #2053: OSD destroy only on specified node

[Dominic Jäger]

Allow destroying only OSDs that belong to the node that has been specified in
the API path.

So if
 - OSD 1 belongs to node A and
 - OSD 2 belongs to node B
then
 - pvesh delete nodes/A/ceph/osd/1 is allowed but
 - pvesh delete nodes/A/ceph/osd/2 is not

Destroying an OSD via GUI automatically inserts the correct node
into the API path.

pveceph automatically insert the local node into the API call, too.
Consequently, it can now only destroy local OSDs (fix #2053).
 - pveceph osd destroy 1 is allowed on node A but
 - pveceph osd destroy 2 is not

Signed-off-by: Dominic Jäger <d.jaeger@proxmox.com>
---
 PVE/API2/Ceph/OSD.pm | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/PVE/API2/Ceph/OSD.pm b/PVE/API2/Ceph/OSD.pm
index b81a8054..6763e95b 100644
--- a/PVE/API2/Ceph/OSD.pm
+++ b/PVE/API2/Ceph/OSD.pm
@@ -478,6 +478,22 @@ __PACKAGE__->register_method ({
 	return $rpcenv->fork_worker('cephcreateosd', $devname,  $authuser, $worker);
     }});
 
+# Check if $osdid belongs to $nodename
+# $tree ... rados osd tree (passing the tree makes it easy to test)
+sub osd_belongs_to_node {
+    my ($tree, $nodename, $osdid) = @_;
+
+    die "No tree nodes found\n" if !($tree && $tree->{nodes});
+    my $allNodes = $tree->{nodes};
+
+    my @match = grep($_->{name} eq $nodename, @$allNodes);
+    my $node = shift @match; # contains rados information about $nodename
+    die "There must not be more than one such node in the list" if @match;
+
+    my $osds = $node->{children};
+    return grep($_ == $osdid, @$osds);
+}
+
 __PACKAGE__->register_method ({
     name => 'destroyosd',
     path => '{osdid}',
@@ -515,6 +531,15 @@ __PACKAGE__->register_method ({
 	my $cleanup = $param->{cleanup};
 
 	my $rados = PVE::RADOS->new();
+
+	my $osd_belongs_to_node = osd_belongs_to_node(
+	    $rados->mon_command({ prefix => 'osd tree' }),
+	    $param->{node},
+	    $osdid,
+	);
+	die "OSD osd.$osdid does not belong to node $param->{node}!"
+	    if !$osd_belongs_to_node;
+
 	# dies if osdid is unknown
 	my $osdstat = $get_osd_status->($rados, $osdid);
 
-- 
2.20.1

[pve-devel] [PATCH manager 2/2] Add tests for OSD-belongs-to-node helper

[Dominic Jäger]

Signed-off-by: Dominic Jäger <d.jaeger@proxmox.com>
---
 test/Makefile    |  5 +++-
 test/OSD_test.pl | 72 ++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 76 insertions(+), 1 deletion(-)
 create mode 100755 test/OSD_test.pl

diff --git a/test/Makefile b/test/Makefile
index d383e89f..3c4d2f2a 100644
--- a/test/Makefile
+++ b/test/Makefile
@@ -5,7 +5,7 @@ all:
 export PERLLIB=..
 
 .PHONY: check balloon-test replication-test mail-test vzdump-test
-check: test-replication test-balloon test-mail test-vzdump
+check: test-replication test-balloon test-mail test-vzdump test-osd
 
 test-balloon:
 	./balloontest.pl
@@ -27,6 +27,9 @@ test-vzdump-guest-included:
 test-vzdump-retention:
 	./vzdump_new_retention_test.pl
 
+test-osd:
+	./OSD_test.pl
+
 .PHONY: install
 install:
 
diff --git a/test/OSD_test.pl b/test/OSD_test.pl
new file mode 100755
index 00000000..df8c7881
--- /dev/null
+++ b/test/OSD_test.pl
@@ -0,0 +1,72 @@
+#!/usr/bin/perl
+
+use strict;
+use warnings;
+
+use lib ('.', '..');
+
+use JSON;
+use Test::More;
+use PVE::API2::Ceph::OSD;
+
+use Data::Dumper;
+
+my $tree = {
+    nodes => [
+	{
+	    id => -3,
+	    name => 'pveA',
+	    children => [ 0,1,2,3 ],
+	}, {
+	    id => -5,
+	    name => 'pveB',
+	    children => [ 4,5,6,7 ],
+	}, {
+	    id => -7,
+	    name => 'pveC',
+	    children => [ 8,9,10,11 ],
+	},
+    ],
+};
+
+
+# Check if all the grep and casts are correct
+my @belong_to_B = ( 4,5 );
+my @not_belong_to_B = ( -1,1,10,15 );
+foreach (@belong_to_B) {
+    is (
+	PVE::API2::Ceph::OSD::osd_belongs_to_node($tree, 'pveB', $_),
+	1,
+	"OSD $_ belongs to node pveB",
+    );
+}
+foreach (@not_belong_to_B) {
+    is (
+	PVE::API2::Ceph::OSD::osd_belongs_to_node($tree, 'pveB', $_),
+	0,
+	"OSD $_ does not belong to node pveB",
+    );
+}
+
+
+my $double_nodes_tree = {
+    nodes => [
+	{
+	    name => 'pveA',
+	},
+	{
+	    name => 'pveA',
+	}
+    ]
+};
+eval { PVE::API2::Ceph::OSD::osd_belongs_to_node($double_nodes_tree, 'pveA') };
+like($@, qr/not be more than one/, "Die if node occurs too often");
+
+my $tree_without_nodes = {
+    dummy => 'dummy',
+};
+eval { PVE::API2::Ceph::OSD::osd_belongs_to_node(undef) };
+like($@, qr/No tree nodes/, "Die if tree has no nodes");
+
+
+done_testing(@belong_to_B + @not_belong_to_B + 2);
\ No newline at end of file
-- 
2.20.1

[pve-devel] applied-series: [PATCH manager 1/2] Fix #2053: OSD destroy only on specified node

[Thomas Lamprecht]

On 11.01.21 12:42, Dominic Jäger wrote:
> Allow destroying only OSDs that belong to the node that has been specified in
> the API path.
> 
> So if
>  - OSD 1 belongs to node A and
>  - OSD 2 belongs to node B
> then
>  - pvesh delete nodes/A/ceph/osd/1 is allowed but
>  - pvesh delete nodes/A/ceph/osd/2 is not
> 
> Destroying an OSD via GUI automatically inserts the correct node
> into the API path.
> 
> pveceph automatically insert the local node into the API call, too.
> Consequently, it can now only destroy local OSDs (fix #2053).
>  - pveceph osd destroy 1 is allowed on node A but
>  - pveceph osd destroy 2 is not
> 
> Signed-off-by: Dominic Jäger <d.jaeger@proxmox.com>
> ---
>  PVE/API2/Ceph/OSD.pm | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
>

applied, thanks!

I reworked the the filtering/parsing the $tree result a bit, maybe you can re-check
if all seem OK to you? The test are def. good and helped here - thanks for them!