[pve-devel] [PATCH pve-manager 1/2] fix #5198: ceph: mon: fix mon existence check in mon removal assertion

[pve-devel] [PATCH pve-manager 1/2] fix #5198: ceph: mon: fix mon existence check in mon removal assertion

[Max Carrara]

The Ceph monitor removal assertion contains a condition that checks
whether the given mon ID actually exists and thus may be removed.

The first part of the condition checks whether the hash returned by
`get_services_info` [0] contains the key "mon.$monid". However, the
hash's keys are never prefixed with "mon.", which makes this check
incorrect.

This is fixed by just using "$monid" directly.

The second part checks whether the mon hashes returned by
Ceph contain the "name" key before comparing the key with the given
mon ID. This key existence check is also incorrect; in particular:
  * If the lookup `$_->{name}` evaluates to e.g. "foo", the check
    passes, because "foo" is truthy. [1]
  * If the lookup `$_->{name}` evaluates to "0", the check fails,
    because "0" is falsy (due to it being equivalent to the number 0,
    according to Perl [1]).

This is solved by using the inbuilt `exists()` instead of relying on
Perl's definition of truthiness.

[0]: https://git.proxmox.com/?p=pve-manager.git;a=blob;f=PVE/Ceph/Services.pm;h=e0f31e8eb6bc9b3777b3d0d548497276efaa5c41;hb=HEAD#l112
[1]: https://perldoc.perl.org/perldata#Scalar-values

Fixes: https://bugzilla.proxmox.com/show_bug.cgi?id=5198
Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
 PVE/API2/Ceph/MON.pm | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/PVE/API2/Ceph/MON.pm b/PVE/API2/Ceph/MON.pm
index 1e959ef3..1737c294 100644
--- a/PVE/API2/Ceph/MON.pm
+++ b/PVE/API2/Ceph/MON.pm
@@ -147,8 +147,8 @@ my $assert_mon_prerequisites = sub {
 my $assert_mon_can_remove = sub {
     my ($monhash, $monlist, $monid, $mondir) = @_;
 
-    if (!(defined($monhash->{"mon.$monid"}) ||
-	  grep { $_->{name} && $_->{name} eq $monid } @$monlist))
+    if (!(defined($monhash->{$monid}) ||
+	  grep { exists($_->{name}) && $_->{name} eq $monid } @$monlist))
     {
 	die "no such monitor id '$monid'\n"
     }
-- 
2.39.2

[pve-devel] [PATCH pve-manager 2/2] ceph: mon: adapt code style in mon removal assertion

[Max Carrara]

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
 PVE/API2/Ceph/MON.pm | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/PVE/API2/Ceph/MON.pm b/PVE/API2/Ceph/MON.pm
index 1737c294..f13115a0 100644
--- a/PVE/API2/Ceph/MON.pm
+++ b/PVE/API2/Ceph/MON.pm
@@ -148,13 +148,13 @@ my $assert_mon_can_remove = sub {
     my ($monhash, $monlist, $monid, $mondir) = @_;
 
     if (!(defined($monhash->{$monid}) ||
-	  grep { exists($_->{name}) && $_->{name} eq $monid } @$monlist))
+	  grep { exists($_->{name}) && $_->{name} eq $monid } $monlist->@*))
     {
 	die "no such monitor id '$monid'\n"
     }
 
     die "monitor filesystem '$mondir' does not exist on this node\n" if ! -d $mondir;
-    die "can't remove last monitor\n" if scalar(@$monlist) <= 1;
+    die "can't remove last monitor\n" if scalar($monlist->@*) <= 1;
 };
 
 my $remove_addr_from_mon_host = sub {
-- 
2.39.2

Re: [pve-devel] [PATCH pve-manager 1/2] fix #5198: ceph: mon: fix mon existence check in mon removal assertion

[Stefan Sterz]

On Wed Mar 13, 2024 at 3:53 PM CET, Max Carrara wrote:
> The Ceph monitor removal assertion contains a condition that checks
> whether the given mon ID actually exists and thus may be removed.
>
> The first part of the condition checks whether the hash returned by
> `get_services_info` [0] contains the key "mon.$monid". However, the
> hash's keys are never prefixed with "mon.", which makes this check
> incorrect.
>
> This is fixed by just using "$monid" directly.
>
> The second part checks whether the mon hashes returned by
> Ceph contain the "name" key before comparing the key with the given
> mon ID. This key existence check is also incorrect; in particular:
>   * If the lookup `$_->{name}` evaluates to e.g. "foo", the check
>     passes, because "foo" is truthy. [1]
>   * If the lookup `$_->{name}` evaluates to "0", the check fails,
>     because "0" is falsy (due to it being equivalent to the number 0,
>     according to Perl [1]).
>
> This is solved by using the inbuilt `exists()` instead of relying on
> Perl's definition of truthiness.
>
> [0]: https://git.proxmox.com/?p=pve-manager.git;a=blob;f=PVE/Ceph/Services.pm;h=e0f31e8eb6bc9b3777b3d0d548497276efaa5c41;hb=HEAD#l112
> [1]: https://perldoc.perl.org/perldata#Scalar-values
>
> Fixes: https://bugzilla.proxmox.com/show_bug.cgi?id=5198
> Signed-off-by: Max Carrara <m.carrara@proxmox.com>
> ---
>  PVE/API2/Ceph/MON.pm | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/PVE/API2/Ceph/MON.pm b/PVE/API2/Ceph/MON.pm
> index 1e959ef3..1737c294 100644
> --- a/PVE/API2/Ceph/MON.pm
> +++ b/PVE/API2/Ceph/MON.pm
> @@ -147,8 +147,8 @@ my $assert_mon_prerequisites = sub {
>  my $assert_mon_can_remove = sub {
>      my ($monhash, $monlist, $monid, $mondir) = @_;
>
> -    if (!(defined($monhash->{"mon.$monid"}) ||
> -	  grep { $_->{name} && $_->{name} eq $monid } @$monlist))

not sure if splitting the fix and the code style clean up makes sense
here but otherwise this works as advertised. So:

Tested-by: Stefan Sterz <s.sterz@proxmox.com>

> +    if (!(defined($monhash->{$monid}) ||
> +	  grep { exists($_->{name}) && $_->{name} eq $monid } @$monlist))
>      {
>  	die "no such monitor id '$monid'\n"
>      }

Re: [pve-devel] [PATCH pve-manager 1/2] fix #5198: ceph: mon: fix mon existence check in mon removal assertion

[Fiona Ebner]

Am 13.03.24 um 16:42 schrieb Stefan Sterz:
> On Wed Mar 13, 2024 at 3:53 PM CET, Max Carrara wrote:
>> The Ceph monitor removal assertion contains a condition that checks
>> whether the given mon ID actually exists and thus may be removed.
>>
>> The first part of the condition checks whether the hash returned by
>> `get_services_info` [0] contains the key "mon.$monid". However, the
>> hash's keys are never prefixed with "mon.", which makes this check
>> incorrect.
>>
>> This is fixed by just using "$monid" directly.
>>
>> The second part checks whether the mon hashes returned by
>> Ceph contain the "name" key before comparing the key with the given
>> mon ID. This key existence check is also incorrect; in particular:
>>   * If the lookup `$_->{name}` evaluates to e.g. "foo", the check
>>     passes, because "foo" is truthy. [1]
>>   * If the lookup `$_->{name}` evaluates to "0", the check fails,
>>     because "0" is falsy (due to it being equivalent to the number 0,
>>     according to Perl [1]).
>>
>> This is solved by using the inbuilt `exists()` instead of relying on
>> Perl's definition of truthiness.
>>

Technically, it's two changes, so could be two patches, but can be fine
like this too, since both touch the same check.

>> [0]: https://git.proxmox.com/?p=pve-manager.git;a=blob;f=PVE/Ceph/Services.pm;h=e0f31e8eb6bc9b3777b3d0d548497276efaa5c41;hb=HEAD#l112
>> [1]: https://perldoc.perl.org/perldata#Scalar-values
>>
>> Fixes: https://bugzilla.proxmox.com/show_bug.cgi?id=5198
>> Signed-off-by: Max Carrara <m.carrara@proxmox.com>
>> ---
>>  PVE/API2/Ceph/MON.pm | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/PVE/API2/Ceph/MON.pm b/PVE/API2/Ceph/MON.pm
>> index 1e959ef3..1737c294 100644
>> --- a/PVE/API2/Ceph/MON.pm
>> +++ b/PVE/API2/Ceph/MON.pm
>> @@ -147,8 +147,8 @@ my $assert_mon_prerequisites = sub {
>>  my $assert_mon_can_remove = sub {
>>      my ($monhash, $monlist, $monid, $mondir) = @_;
>>
>> -    if (!(defined($monhash->{"mon.$monid"}) ||
>> -	  grep { $_->{name} && $_->{name} eq $monid } @$monlist))
> 
> not sure if splitting the fix and the code style clean up makes sense
> here but otherwise this works as advertised. So:
> 

If you already touch the same line, you can also adapt style in one go.
But since the style change is also done in a second place, which is not
touched by this patch, it's fine like Max did it too.

> Tested-by: Stefan Sterz <s.sterz@proxmox.com>
> 
>> +    if (!(defined($monhash->{$monid}) ||
>> +	  grep { exists($_->{name}) && $_->{name} eq $monid } @$monlist))

While I suppose we do not expect an entry with undef value here, you do
not want to compare against undef (just leads to an ugly Perl warning),
so this should be a definedness check, not an existence check.

>>      {
>>  	die "no such monitor id '$monid'\n"
>>      }
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 

Re: [pve-devel] [PATCH pve-manager 1/2] fix #5198: ceph: mon: fix mon existence check in mon removal assertion

[Max Carrara]

On Thu Mar 14, 2024 at 8:56 AM CET, Fiona Ebner wrote:
> Am 13.03.24 um 16:42 schrieb Stefan Sterz:
> > On Wed Mar 13, 2024 at 3:53 PM CET, Max Carrara wrote:
> >> The Ceph monitor removal assertion contains a condition that checks
> >> whether the given mon ID actually exists and thus may be removed.
> >>
> >> The first part of the condition checks whether the hash returned by
> >> `get_services_info` [0] contains the key "mon.$monid". However, the
> >> hash's keys are never prefixed with "mon.", which makes this check
> >> incorrect.
> >>
> >> This is fixed by just using "$monid" directly.
> >>
> >> The second part checks whether the mon hashes returned by
> >> Ceph contain the "name" key before comparing the key with the given
> >> mon ID. This key existence check is also incorrect; in particular:
> >>   * If the lookup `$_->{name}` evaluates to e.g. "foo", the check
> >>     passes, because "foo" is truthy. [1]
> >>   * If the lookup `$_->{name}` evaluates to "0", the check fails,
> >>     because "0" is falsy (due to it being equivalent to the number 0,
> >>     according to Perl [1]).
> >>
> >> This is solved by using the inbuilt `exists()` instead of relying on
> >> Perl's definition of truthiness.
> >>
>
> Technically, it's two changes, so could be two patches, but can be fine
> like this too, since both touch the same check.
>
> >> [0]: https://git.proxmox.com/?p=pve-manager.git;a=blob;f=PVE/Ceph/Services.pm;h=e0f31e8eb6bc9b3777b3d0d548497276efaa5c41;hb=HEAD#l112
> >> [1]: https://perldoc.perl.org/perldata#Scalar-values
> >>
> >> Fixes: https://bugzilla.proxmox.com/show_bug.cgi?id=5198
> >> Signed-off-by: Max Carrara <m.carrara@proxmox.com>
> >> ---
> >>  PVE/API2/Ceph/MON.pm | 4 ++--
> >>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/PVE/API2/Ceph/MON.pm b/PVE/API2/Ceph/MON.pm
> >> index 1e959ef3..1737c294 100644
> >> --- a/PVE/API2/Ceph/MON.pm
> >> +++ b/PVE/API2/Ceph/MON.pm
> >> @@ -147,8 +147,8 @@ my $assert_mon_prerequisites = sub {
> >>  my $assert_mon_can_remove = sub {
> >>      my ($monhash, $monlist, $monid, $mondir) = @_;
> >>
> >> -    if (!(defined($monhash->{"mon.$monid"}) ||
> >> -	  grep { $_->{name} && $_->{name} eq $monid } @$monlist))
> > 
> > not sure if splitting the fix and the code style clean up makes sense
> > here but otherwise this works as advertised. So:
> > 
>
> If you already touch the same line, you can also adapt style in one go.
> But since the style change is also done in a second place, which is not
> touched by this patch, it's fine like Max did it too.
>
> > Tested-by: Stefan Sterz <s.sterz@proxmox.com>
> > 
> >> +    if (!(defined($monhash->{$monid}) ||
> >> +	  grep { exists($_->{name}) && $_->{name} eq $monid } @$monlist))
>
> While I suppose we do not expect an entry with undef value here, you do
> not want to compare against undef (just leads to an ugly Perl warning),
> so this should be a definedness check, not an existence check.

Good catch, thanks! Will send in a v2.

>
> >>      {
> >>  	die "no such monitor id '$monid'\n"
> >>      }
> > 
> > 
> > 
> > _______________________________________________
> > pve-devel mailing list
> > pve-devel@lists.proxmox.com
> > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> > 
> >