[pve-devel] [PATCH storage] fix #5008: prevent adding pbs storage with invalid namespace

[pve-devel] [PATCH storage] fix #5008: prevent adding pbs storage with invalid namespace

[Philipp Hufnagl]

Currently, when adding a PBS storage with a namespace that does not
exist, the storage gets added normally, but browsing/using it only
returns a cryptic error message.

This change checks if the namespace entered when adding is valid and
prompts an error if it is not. If no namespace is provided, the storage
will be added without error.

Signed-off-by: Philipp Hufnagl <p.hufnagl@proxmox.com>
---
 src/PVE/Storage/PBSPlugin.pm | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/src/PVE/Storage/PBSPlugin.pm b/src/PVE/Storage/PBSPlugin.pm
index 4320974..aceb2c4 100644
--- a/src/PVE/Storage/PBSPlugin.pm
+++ b/src/PVE/Storage/PBSPlugin.pm
@@ -817,6 +817,17 @@ sub scan_datastores {
     return $response;
 }
 
+sub scan_namespaces {
+    my ($scfg, $datastore, $password) = @_;
+
+    my $conn = pbs_api_connect($scfg, $password);
+
+    my $namespaces = eval { $conn->get("/api2/json/admin/datastore/$datastore/namespace", {}); };
+    die "error fetching namespaces - $@" if $@;
+
+    return $namespaces;
+}
+
 sub activate_storage {
     my ($class, $storeid, $scfg, $cache) = @_;
 
@@ -826,10 +837,18 @@ sub activate_storage {
     die "$storeid: $@" if $@;
 
     my $datastore = $scfg->{datastore};
+    my $namespace = $scfg->{namespace};
 
     for my $ds (@$datastores) {
 	if ($ds->{store} eq $datastore) {
-	    return 1;
+	    return 1 if !defined($namespace);
+	    my $namespaces = eval { scan_namespaces($scfg, $datastore, $password) };
+	    for my $ns (@$namespaces) {
+		if ($ns->{ns} eq $namespace) {
+		    return 1;
+		}
+	    }
+	    die "$storeid: Cannot find namespace '$namespace', check permissions and existence!\n";
 	}
     }
 
-- 
2.39.2

Re: [pve-devel] [PATCH storage] fix #5008: prevent adding pbs storage with invalid namespace

[Fiona Ebner]

Am 14.11.23 um 15:27 schrieb Philipp Hufnagl:
> Currently, when adding a PBS storage with a namespace that does not
> exist, the storage gets added normally, but browsing/using it only
> returns a cryptic error message.
> 
> This change checks if the namespace entered when adding is valid and
> prompts an error if it is not. If no namespace is provided, the storage
> will be added without error.

Does not fully describe the change: It checks if the namespace is valid
each time the storage is activated, not just when adding.

> 
> Signed-off-by: Philipp Hufnagl <p.hufnagl@proxmox.com>
> ---
>  src/PVE/Storage/PBSPlugin.pm | 21 ++++++++++++++++++++-
>  1 file changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/src/PVE/Storage/PBSPlugin.pm b/src/PVE/Storage/PBSPlugin.pm
> index 4320974..aceb2c4 100644
> --- a/src/PVE/Storage/PBSPlugin.pm
> +++ b/src/PVE/Storage/PBSPlugin.pm
> @@ -817,6 +817,17 @@ sub scan_datastores {
>      return $response;
>  }
>  
> +sub scan_namespaces {
> +    my ($scfg, $datastore, $password) = @_;
> +
> +    my $conn = pbs_api_connect($scfg, $password);

Not super important, but would be nice to have a way to re-use the same
connection in scan_datastores() and here, since activate_storage() will
call both of them.

> +
> +    my $namespaces = eval { $conn->get("/api2/json/admin/datastore/$datastore/namespace", {}); };
> +    die "error fetching namespaces - $@" if $@;
> +
> +    return $namespaces;
> +}
> +
>  sub activate_storage {
>      my ($class, $storeid, $scfg, $cache) = @_;
>  
> @@ -826,10 +837,18 @@ sub activate_storage {
>      die "$storeid: $@" if $@;
>  
>      my $datastore = $scfg->{datastore};
> +    my $namespace = $scfg->{namespace};
>  
>      for my $ds (@$datastores) {
>  	if ($ds->{store} eq $datastore) {
> -	    return 1;
> +	    return 1 if !defined($namespace);
> +	    my $namespaces = eval { scan_namespaces($scfg, $datastore, $password) };

Why use eval and ignore the error here? Like that users (and we) won't
know if the api request or connection failed and just get the error
message from below about permissions/existence then.

> +	    for my $ns (@$namespaces) {
> +		if ($ns->{ns} eq $namespace) {
> +		    return 1;
> +		}
> +	    }
> +	    die "$storeid: Cannot find namespace '$namespace', check permissions and existence!\n";
>  	}
>      }
>  

Re: [pve-devel] [PATCH storage] fix #5008: prevent adding pbs storage with invalid namespace

[Philipp Hufnagl]

On 11/15/23 09:31, Fiona Ebner wrote:
> Am 14.11.23 um 15:27 schrieb Philipp Hufnagl:
>> Currently, when adding a PBS storage with a namespace that does not
>> exist, the storage gets added normally, but browsing/using it only
>> returns a cryptic error message.
>>
>> This change checks if the namespace entered when adding is valid and
>> prompts an error if it is not. If no namespace is provided, the storage
>> will be added without error.
> 
> Does not fully describe the change: It checks if the namespace is valid
> each time the storage is activated, not just when adding.
> 

Sorry. Ill try to elaborate more.

>>
>> Signed-off-by: Philipp Hufnagl <p.hufnagl@proxmox.com>
>> ---
>>  src/PVE/Storage/PBSPlugin.pm | 21 ++++++++++++++++++++-
>>  1 file changed, 20 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/PVE/Storage/PBSPlugin.pm b/src/PVE/Storage/PBSPlugin.pm
>> index 4320974..aceb2c4 100644
>> --- a/src/PVE/Storage/PBSPlugin.pm
>> +++ b/src/PVE/Storage/PBSPlugin.pm
>> @@ -817,6 +817,17 @@ sub scan_datastores {
>>      return $response;
>>  }
>>  
>> +sub scan_namespaces {
>> +    my ($scfg, $datastore, $password) = @_;
>> +
>> +    my $conn = pbs_api_connect($scfg, $password);
> 
> Not super important, but would be nice to have a way to re-use the same
> connection in scan_datastores() and here, since activate_storage() will
> call both of them.

scan_datastores() seem to be called somewhere else as well. I see if I
can find a way to reuse the connection but not break the code there.
> 
>> +
>> +    my $namespaces = eval { $conn->get("/api2/json/admin/datastore/$datastore/namespace", {}); };
>> +    die "error fetching namespaces - $@" if $@;
>> +
>> +    return $namespaces;
>> +}
>> +
>>  sub activate_storage {
>>      my ($class, $storeid, $scfg, $cache) = @_;
>>  
>> @@ -826,10 +837,18 @@ sub activate_storage {
>>      die "$storeid: $@" if $@;
>>  
>>      my $datastore = $scfg->{datastore};
>> +    my $namespace = $scfg->{namespace};
>>  
>>      for my $ds (@$datastores) {
>>  	if ($ds->{store} eq $datastore) {
>> -	    return 1;
>> +	    return 1 if !defined($namespace);
>> +	    my $namespaces = eval { scan_namespaces($scfg, $datastore, $password) };
> 
> Why use eval and ignore the error here? Like that users (and we) won't
> know if the api request or connection failed and just get the error
> message from below about permissions/existence then.

I tried to mimic the behavior from scan_datastores(). Did I make a
mistake there? Is the way of scan_datastores() deprecated or bad practice?
> 
>> +	    for my $ns (@$namespaces) {
>> +		if ($ns->{ns} eq $namespace) {
>> +		    return 1;
>> +		}
>> +	    }
>> +	    die "$storeid: Cannot find namespace '$namespace', check permissions and existence!\n";
>>  	}
>>      }
>>  

Re: [pve-devel] [PATCH storage] fix #5008: prevent adding pbs storage with invalid namespace

[Thomas Lamprecht]

Am 15/11/2023 um 10:37 schrieb Philipp Hufnagl:
> On 11/15/23 09:31, Fiona Ebner wrote:
>> Am 14.11.23 um 15:27 schrieb Philipp Hufnagl:
>>> diff --git a/src/PVE/Storage/PBSPlugin.pm b/src/PVE/Storage/PBSPlugin.pm
>>> index 4320974..aceb2c4 100644
>>> --- a/src/PVE/Storage/PBSPlugin.pm
>>> +++ b/src/PVE/Storage/PBSPlugin.pm
>>> @@ -817,6 +817,17 @@ sub scan_datastores {
>>>      return $response;
>>>  }
>>>  
>>> +sub scan_namespaces {
>>> +    my ($scfg, $datastore, $password) = @_;
>>> +
>>> +    my $conn = pbs_api_connect($scfg, $password);
>>
>> Not super important, but would be nice to have a way to re-use the same
>> connection in scan_datastores() and here, since activate_storage() will
>> call both of them.
> 
> scan_datastores() seem to be called somewhere else as well. I see if I
> can find a way to reuse the connection but not break the code there.


In the long run it maybe could be better to have an explicit check_availability
hook, but IMO it's a bit late in the release cycle for that as this needs a bit
extra care, especially w.r.t. external plugins and our ABI compat.

Anyhow, as workaround we might be able to do this check in the on_add_hook and
on_update_hook methods for now.

Re: [pve-devel] [PATCH storage] fix #5008: prevent adding pbs storage with invalid namespace

[Fiona Ebner]

Am 15.11.23 um 10:37 schrieb Philipp Hufnagl:
> 
> 
> On 11/15/23 09:31, Fiona Ebner wrote:
>> Am 14.11.23 um 15:27 schrieb Philipp Hufnagl:
>>> @@ -826,10 +837,18 @@ sub activate_storage {
>>>      die "$storeid: $@" if $@;
>>>  
>>>      my $datastore = $scfg->{datastore};
>>> +    my $namespace = $scfg->{namespace};
>>>  
>>>      for my $ds (@$datastores) {
>>>  	if ($ds->{store} eq $datastore) {
>>> -	    return 1;
>>> +	    return 1 if !defined($namespace);
>>> +	    my $namespaces = eval { scan_namespaces($scfg, $datastore, $password) };
>>
>> Why use eval and ignore the error here? Like that users (and we) won't
>> know if the api request or connection failed and just get the error
>> message from below about permissions/existence then.
> 
> I tried to mimic the behavior from scan_datastores(). Did I make a
> mistake there? Is the way of scan_datastores() deprecated or bad practice?
>>

There, the error is not ignored, but propagated:

>     my $datastores = eval { scan_datastores($scfg, $password) };
>     die "$storeid: $@" if $@;

Re: [pve-devel] [PATCH storage] fix #5008: prevent adding pbs storage with invalid namespace

[Philipp Hufnagl]

On 11/15/23 10:52, Thomas Lamprecht wrote:
> Am 15/11/2023 um 10:37 schrieb Philipp Hufnagl:
>> On 11/15/23 09:31, Fiona Ebner wrote:
>>> Am 14.11.23 um 15:27 schrieb Philipp Hufnagl:
>>>> diff --git a/src/PVE/Storage/PBSPlugin.pm b/src/PVE/Storage/PBSPlugin.pm
>>>> index 4320974..aceb2c4 100644
>>>> --- a/src/PVE/Storage/PBSPlugin.pm
>>>> +++ b/src/PVE/Storage/PBSPlugin.pm
>>>> @@ -817,6 +817,17 @@ sub scan_datastores {
>>>>      return $response;
>>>>  }
>>>>  
>>>> +sub scan_namespaces {
>>>> +    my ($scfg, $datastore, $password) = @_;
>>>> +
>>>> +    my $conn = pbs_api_connect($scfg, $password);
>>>
>>> Not super important, but would be nice to have a way to re-use the same
>>> connection in scan_datastores() and here, since activate_storage() will
>>> call both of them.
>>
>> scan_datastores() seem to be called somewhere else as well. I see if I
>> can find a way to reuse the connection but not break the code there.
> 
> 
> In the long run it maybe could be better to have an explicit check_availability
> hook, but IMO it's a bit late in the release cycle for that as this needs a bit
> extra care, especially w.r.t. external plugins and our ABI compat.
> 
> Anyhow, as workaround we might be able to do this check in the on_add_hook and
> on_update_hook methods for now.

I am planning on introducing a function called "connect_if_none" that
checks if it gets passed a connection and if so, return it. If it gets
passed undef, it will establish a connection and return that one.

That way a user can simply write something like

  my $conn = connect_if_none($scfg, $password, $conn);

and not worry about it.

Re: [pve-devel] [PATCH storage] fix #5008: prevent adding pbs storage with invalid namespace

[Thomas Lamprecht]

Am 15/11/2023 um 11:05 schrieb Philipp Hufnagl:
> 
> 
> On 11/15/23 10:52, Thomas Lamprecht wrote:
>> Am 15/11/2023 um 10:37 schrieb Philipp Hufnagl:
>>> On 11/15/23 09:31, Fiona Ebner wrote:
>>>> Am 14.11.23 um 15:27 schrieb Philipp Hufnagl:
>>>>> diff --git a/src/PVE/Storage/PBSPlugin.pm b/src/PVE/Storage/PBSPlugin.pm
>>>>> index 4320974..aceb2c4 100644
>>>>> --- a/src/PVE/Storage/PBSPlugin.pm
>>>>> +++ b/src/PVE/Storage/PBSPlugin.pm
>>>>> @@ -817,6 +817,17 @@ sub scan_datastores {
>>>>>      return $response;
>>>>>  }
>>>>>  
>>>>> +sub scan_namespaces {
>>>>> +    my ($scfg, $datastore, $password) = @_;
>>>>> +
>>>>> +    my $conn = pbs_api_connect($scfg, $password);
>>>>
>>>> Not super important, but would be nice to have a way to re-use the same
>>>> connection in scan_datastores() and here, since activate_storage() will
>>>> call both of them.
>>>
>>> scan_datastores() seem to be called somewhere else as well. I see if I
>>> can find a way to reuse the connection but not break the code there.
>>
>>
>> In the long run it maybe could be better to have an explicit check_availability
>> hook, but IMO it's a bit late in the release cycle for that as this needs a bit
>> extra care, especially w.r.t. external plugins and our ABI compat.
>>
>> Anyhow, as workaround we might be able to do this check in the on_add_hook and
>> on_update_hook methods for now.
> 
> I am planning on introducing a function called "connect_if_none" that
> checks if it gets passed a connection and if so, return it. If it gets
> passed undef, it will establish a connection and return that one.
> 
> That way a user can simply write something like
> 
>   my $conn = connect_if_none($scfg, $password, $conn);
> 
> and not worry about it.


not sure how above fits to my comment at all...
Connection re-use is not the real issue here, doing it senslessy on every
activate is..

And connect_if_none is a rather generic/undescriptive name and still would
not solve re-use if done in different method calls, so a rather for only
a very limited use case where one can just pass $conn around directly.

Re: [pve-devel] [PATCH storage] fix #5008: prevent adding pbs storage with invalid namespace

[Philipp Hufnagl]

On 11/15/23 11:09, Thomas Lamprecht wrote:
> Am 15/11/2023 um 11:05 schrieb Philipp Hufnagl:
>>
>>
>> On 11/15/23 10:52, Thomas Lamprecht wrote:
>>> Am 15/11/2023 um 10:37 schrieb Philipp Hufnagl:
>>>> On 11/15/23 09:31, Fiona Ebner wrote:
>>>>> Am 14.11.23 um 15:27 schrieb Philipp Hufnagl:
>>>>>> diff --git a/src/PVE/Storage/PBSPlugin.pm b/src/PVE/Storage/PBSPlugin.pm
>>>>>> index 4320974..aceb2c4 100644
>>>>>> --- a/src/PVE/Storage/PBSPlugin.pm
>>>>>> +++ b/src/PVE/Storage/PBSPlugin.pm
>>>>>> @@ -817,6 +817,17 @@ sub scan_datastores {
>>>>>>      return $response;
>>>>>>  }
>>>>>>  
>>>>>> +sub scan_namespaces {
>>>>>> +    my ($scfg, $datastore, $password) = @_;
>>>>>> +
>>>>>> +    my $conn = pbs_api_connect($scfg, $password);
>>>>>
>>>>> Not super important, but would be nice to have a way to re-use the same
>>>>> connection in scan_datastores() and here, since activate_storage() will
>>>>> call both of them.
>>>>
>>>> scan_datastores() seem to be called somewhere else as well. I see if I
>>>> can find a way to reuse the connection but not break the code there.
>>>
>>>
>>> In the long run it maybe could be better to have an explicit check_availability
>>> hook, but IMO it's a bit late in the release cycle for that as this needs a bit
>>> extra care, especially w.r.t. external plugins and our ABI compat.
>>>
>>> Anyhow, as workaround we might be able to do this check in the on_add_hook and
>>> on_update_hook methods for now.
>>
>> I am planning on introducing a function called "connect_if_none" that
>> checks if it gets passed a connection and if so, return it. If it gets
>> passed undef, it will establish a connection and return that one.
>>
>> That way a user can simply write something like
>>
>>   my $conn = connect_if_none($scfg, $password, $conn);
>>
>> and not worry about it.
> 
> 
> not sure how above fits to my comment at all...
> Connection re-use is not the real issue here, doing it senslessy on every
> activate is..
> 
> And connect_if_none is a rather generic/undescriptive name and still would
> not solve re-use if done in different method calls, so a rather for only
> a very limited use case where one can just pass $conn around directly.

I can rename it.

I am not certain if I understand correctly. Aren't those hooks
trigered after adding a storage? The issue I am resolving here is
before the storage is actually added.