Re: [pve-devel] 3rd-party storage has "Unknown" type

Re: [pve-devel] 3rd-party storage has "Unknown" type

[Thomas Lamprecht]

Hi Josh,

On 31.03.22 22:39, Joshua Huber wrote:
...
> 
> 1) The simplest: if there's no entry in storageSchema for value, just
> return value as the "formatted" storage type. This seems like a great
> low-cost, low-risk change.
> 
> diff --git a/www/manager6/Utils.js b/www/manager6/Utils.js
> index aafe359a..9ebd3758 100644
> --- a/www/manager6/Utils.js
> +++ b/www/manager6/Utils.js
> @@ -1001,7 +1001,7 @@ Ext.define('PVE.Utils', {
>         if (schema) {
>             return schema.name;
>         }
> -       return Proxmox.Utils.unknownText;
> +       return value;
>      },
> 
>      format_ha: function(value) {

That's actually a good point, returning unknown here isn't really helping
anyone, especially as  it's a place where we known that there can be values
not under Proxmox VE's control, so the value is definitively the better
fallback.

I pushed out a commit with your proposed 1) behavior as its a good stop-gap
measurement in any way and doesn't harm any fancier way we may possibly add
in the future.

https://git.proxmox.com/?p=pve-manager.git;a=commitdiff;h=ea9b688a8234bbdb8cd9e0f6498bed29777a1e2c

> 
> 2) Slightly fancier: provide a common plugin-layer configuration
> option (e.g., "type_name"), which allows the storage to name itself,
> and even allows the end-user to give friendlier names to different
> storage pools they've defined. In pve-manager, all we need is a
> different final return statement:
> 
> + return record && record.get('type_name') || value;

would need to be encoded to avoid HTML-element or JS injection, e.g., via
the `Ext.htmlEncode()`, besides that it would seem OK from a technical POV.

> 
> And a new entry in the default storage plugin property list (in
> pve-storage, PVE/Storage/Plugin.pm):
> 
> + type_name => {

not all to hard feelings but for most new properties we use kebab case, i.e.,
here it'd be 'type-name' (needs the quotes then too, else perl gets confused
as you probably already know), but due to backward compat we got quite the mix
present, at least in Proxmox VE (the newer, rust based PBS is quite a bit more
consistent).

Possibly alternative: 'type-text' (as one could draw parallels to our
JSONSchema's internal 'format-text, which is used roughly similarly for
property format definitions).

> +     description => "User-friendly display name for storage",
> +     type => 'string',
> +     optional => 1,

adding a generous maxLength (say something between 64 to 128) and a pattern
to avoid other whitespace-types than, well, a plain space would IMO be good
to add.

> + },
> 
> This has the possible side-benefit of allowing the type name to be
> used in command line tools as well as the UI, which seems like it'd be
> nice for consistency.

Seems like an OK argument for me, especially considering the relatively
small impact.

> 
> 3) Something extremely fancy: 3rd-party storage plugins bundle/provide
> an associated front-end component. This is clearly superior, but is
> much more involved. It could could enable adding and editing 3rd-party
> via the pve-manager UI, provide a customized type name, and even a
> custom icon! 😄
> 

Without encoding 2) would already be quite close to that, but I'd really
like to avoid adding generic XSS injection as a feature ;-)

> 
> Any of these ideas sound interesting? At this point, I'd be pretty
> happy with #1 or #2 -- we don't have the time (nor really the
> expertise with ExtJS) to take on the fancy option at the moment. Any
> other ideas? I'm happy to submit a patch! (I'll get the CLA process
> started, as well.)

Hehe, sorry, seems like I jumped the gun on reading and already pushed out
1) as a low hanging fruit before I even got to this paragraph, maybe a good
sign to call this Friday a day and go home :-)

Anyhow, if you want to for 2) then please feel free to do so.

cheers,
Thomas