Re: [pve-devel] [PATCH storage v4 4/4] pbs: fix #5008: Check if datastore and namespace is valid on add- and update hooks

[Fiona Ebner <f.ebner@proxmox.com>]

Am 27.11.23 um 12:40 schrieb Philipp Hufnagl:
> This adds a check if the datastore and the namespace is valid when a
> user attempts to add a new PBS datastore.
> 
> Since the namespace only can be checked after the datastore is
> validated, the datastore will be checked as well, regardless that it
> will be done later in the superclass anyway.
> 
> The functionallity to check namespaces is added with this commit. For
> checking the datastore, existing code that has previously been
> refactored will be reused.
> 
> Because the server address is needed to check the namespaces in the
> update hook, it has to be included in the Storage/Config.pm.

Rather than "in the Storage/Config.pm" I suppose you mean "in the

> 
> Signed-off-by: Philipp Hufnagl <p.hufnagl@proxmox.com>
> ---
>  src/PVE/API2/Storage/Config.pm |  4 ++--
>  src/PVE/Storage/PBSPlugin.pm   | 41 ++++++++++++++++++++++++++++++++++
>  2 files changed, 43 insertions(+), 2 deletions(-)
> 
> diff --git a/src/PVE/API2/Storage/Config.pm b/src/PVE/API2/Storage/Config.pm
> index e04b6ab..651d4bc 100755
> --- a/src/PVE/API2/Storage/Config.pm
> +++ b/src/PVE/API2/Storage/Config.pm
> @@ -362,12 +362,12 @@ __PACKAGE__->register_method ({
>  		}
>  	    }
>  
> -	    $returned_config = $plugin->on_update_hook($storeid, $opts, %$sensitive);
> -
>  	    for my $k (keys %$opts) {
>  		$scfg->{$k} = $opts->{$k};
>  	    }
>  
> +	    $returned_config = $plugin->on_update_hook($storeid, $scfg, %$sensitive);
> +

This hunk should be its own patch.

Hmm, could be fine in practice, but there is potential for breakage.
Existing (third-party) plugins might be relying on the fact that the
hook is only called with updated values somehow?

On the other hand, currently our CIFS plugin prints a warning that the
updated password will be ignored, because no user is set if the user is
not also updated at same time but does exist in the current config.
Luckily the actual setting of the credentials is not affected. This
change would correct this.

>  	    if (defined($scfg->{mkdir})) { # TODO: remove complete option in Proxmox VE 9
>  		warn "NOTE: The 'mkdir' option set for '${storeid}' is deprecated and will be removed"
>  		    ." in Proxmox VE 9. Use 'create-base-path' or 'create-subdirs' instead.\n"

---snip---

> @@ -833,6 +855,25 @@ sub check_datastore_exists {
>      die "$storeid: Cannot find datastore '$datastore', check permissions and existence!\n";
>  }
>  
> +sub check_namespace_exists {

Because it dies when the namespace doesn't exist, I'd prefer to call it
assert_ rather than check_

> +    my ($class, $storeid, $scfg, $password, $conn) = @_;
> +
> +    my $namespace = $scfg->{namespace};
> +    return 1 if !defined($namespace);
> +    my $datastore = $scfg->{datastore};

Style nit: rather than having this one-time-use variable, please use
$scfg->{datastore} directly below.

> +
> +    my $namespaces = eval { scan_namespaces($scfg, $datastore, $password, $conn) };
> +    die "$storeid: $@" if $@;
> +
> +    for my $ns (@$namespaces) {
> +	if ($ns->{ns} eq $namespace) {
> +	    return 1;
> +	}

Style nit: would save two lines using post-if

> +    }
> +
> +    die "$storeid: Cannot find namespace '$namespace', check permissions and existence!\n";
> +}
> +
>  sub activate_storage {
>      my ($class, $storeid, $scfg, $cache) = @_;
>