* [pve-devel] [PATCH storage] fix #5008: prevent adding pbs storage with invalid namespace
@ 2023-11-14 14:27 Philipp Hufnagl
2023-11-15 8:31 ` Fiona Ebner
0 siblings, 1 reply; 8+ messages in thread
From: Philipp Hufnagl @ 2023-11-14 14:27 UTC (permalink / raw)
To: pve-devel
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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [pve-devel] [PATCH storage] fix #5008: prevent adding pbs storage with invalid namespace
2023-11-14 14:27 [pve-devel] [PATCH storage] fix #5008: prevent adding pbs storage with invalid namespace Philipp Hufnagl
@ 2023-11-15 8:31 ` Fiona Ebner
2023-11-15 9:37 ` Philipp Hufnagl
0 siblings, 1 reply; 8+ messages in thread
From: Fiona Ebner @ 2023-11-15 8:31 UTC (permalink / raw)
To: Proxmox VE development discussion, Philipp Hufnagl
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";
> }
> }
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [pve-devel] [PATCH storage] fix #5008: prevent adding pbs storage with invalid namespace
2023-11-15 8:31 ` Fiona Ebner
@ 2023-11-15 9:37 ` Philipp Hufnagl
2023-11-15 9:52 ` Thomas Lamprecht
2023-11-15 10:01 ` Fiona Ebner
0 siblings, 2 replies; 8+ messages in thread
From: Philipp Hufnagl @ 2023-11-15 9:37 UTC (permalink / raw)
To: Fiona Ebner, Proxmox VE development discussion
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";
>> }
>> }
>>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [pve-devel] [PATCH storage] fix #5008: prevent adding pbs storage with invalid namespace
2023-11-15 9:37 ` Philipp Hufnagl
@ 2023-11-15 9:52 ` Thomas Lamprecht
2023-11-15 10:05 ` Philipp Hufnagl
2023-11-15 10:01 ` Fiona Ebner
1 sibling, 1 reply; 8+ messages in thread
From: Thomas Lamprecht @ 2023-11-15 9:52 UTC (permalink / raw)
To: Proxmox VE development discussion, Philipp Hufnagl, Fiona Ebner
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.
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [pve-devel] [PATCH storage] fix #5008: prevent adding pbs storage with invalid namespace
2023-11-15 9:52 ` Thomas Lamprecht
@ 2023-11-15 10:05 ` Philipp Hufnagl
2023-11-15 10:09 ` Thomas Lamprecht
0 siblings, 1 reply; 8+ messages in thread
From: Philipp Hufnagl @ 2023-11-15 10:05 UTC (permalink / raw)
To: Thomas Lamprecht, Proxmox VE development discussion, Fiona Ebner
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.
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [pve-devel] [PATCH storage] fix #5008: prevent adding pbs storage with invalid namespace
2023-11-15 10:05 ` Philipp Hufnagl
@ 2023-11-15 10:09 ` Thomas Lamprecht
2023-11-15 10:40 ` Philipp Hufnagl
0 siblings, 1 reply; 8+ messages in thread
From: Thomas Lamprecht @ 2023-11-15 10:09 UTC (permalink / raw)
To: Philipp Hufnagl, Proxmox VE development discussion, Fiona Ebner
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.
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [pve-devel] [PATCH storage] fix #5008: prevent adding pbs storage with invalid namespace
2023-11-15 10:09 ` Thomas Lamprecht
@ 2023-11-15 10:40 ` Philipp Hufnagl
0 siblings, 0 replies; 8+ messages in thread
From: Philipp Hufnagl @ 2023-11-15 10:40 UTC (permalink / raw)
To: Thomas Lamprecht, Proxmox VE development discussion, Fiona Ebner
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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [pve-devel] [PATCH storage] fix #5008: prevent adding pbs storage with invalid namespace
2023-11-15 9:37 ` Philipp Hufnagl
2023-11-15 9:52 ` Thomas Lamprecht
@ 2023-11-15 10:01 ` Fiona Ebner
1 sibling, 0 replies; 8+ messages in thread
From: Fiona Ebner @ 2023-11-15 10:01 UTC (permalink / raw)
To: Philipp Hufnagl, Proxmox VE development discussion
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 $@;
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2023-11-15 10:40 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-11-14 14:27 [pve-devel] [PATCH storage] fix #5008: prevent adding pbs storage with invalid namespace Philipp Hufnagl
2023-11-15 8:31 ` Fiona Ebner
2023-11-15 9:37 ` Philipp Hufnagl
2023-11-15 9:52 ` Thomas Lamprecht
2023-11-15 10:05 ` Philipp Hufnagl
2023-11-15 10:09 ` Thomas Lamprecht
2023-11-15 10:40 ` Philipp Hufnagl
2023-11-15 10:01 ` Fiona Ebner
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox