Re: [pve-devel] [PATCH pve-firewall 10/15] api: add vnet endpoints

[Stefan Hanreich <s.hanreich@proxmox.com>]

On 9/26/24 08:37, Thomas Lamprecht wrote:

> I'd prefer a _hook suffix for such a method for slightly added clarity.
> 
> And FWIW, if all you do now is check privileges, and there's nothing you already
> know that's gonna get added here soon, you could just name it after what it does
> and avoid being all to generic, i.e. something like
> 
> sub assert_privs_for_method

Thats's probably the better approach for naming this function, since I
don't have any future additions in mind.


>> +my $option_properties = $PVE::Firewall::vnet_option_properties;
> 
> might need a clone to avoid modifying the original reference I think

Yes, I think we never write to it - but accidents happen and better safe
than sorry. Might make sense to also do this in the other API modules as
well in a separate patch then.


> Hmm, I'm wondering might it make sense to add a SDN.Firewall privilege to separate
> allowing VNet allocation and allowing to configure a VNet's firewall?
> While adding privs is a bit tricky, this one might be dooable later one too though.
> 
> But whatever gets chosen should then be also addressed in a commit message with some
> background reasoning (if it's already then I might have overlooked, I did not checked
> every all too closely yet).

Initial reasoning was that there's not really a privilege like that for
DC / Host / Guests, where it is always tied to the networking permissions.

Maybe it would make sense to create a completely separate Firewall
permission that is then also used for DC / Host / Guests? Of course this
could only happen in the next major release.



Thanks for the review!


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel