Re: [pve-devel] [PATCH v4 pve-network 10/33] api: add endpoints for managing PVE IPAM

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

Am 17/11/2023 um 12:39 schrieb Stefan Hanreich:
> Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
> ---
>  src/PVE/API2/Network/SDN.pm       |   6 +
>  src/PVE/API2/Network/SDN/Ipam.pm  | 221 ++++++++++++++++++++++++++++++
>  src/PVE/API2/Network/SDN/Makefile |   2 +-
>  3 files changed, 228 insertions(+), 1 deletion(-)
>  create mode 100644 src/PVE/API2/Network/SDN/Ipam.pm
> 
> diff --git a/src/PVE/API2/Network/SDN.pm b/src/PVE/API2/Network/SDN.pm
> index d216e48..551afcf 100644
> --- a/src/PVE/API2/Network/SDN.pm
> +++ b/src/PVE/API2/Network/SDN.pm
> @@ -15,6 +15,7 @@ use PVE::Network::SDN;
>  use PVE::API2::Network::SDN::Controllers;
>  use PVE::API2::Network::SDN::Vnets;
>  use PVE::API2::Network::SDN::Zones;
> +use PVE::API2::Network::SDN::Ipam;
>  use PVE::API2::Network::SDN::Ipams;

what's the deal with Ipam vs. Ipams?

I did not looked to closely into it but it seems like the existing Ipams, plural,
is for managing ipam plugins and Ipam, singular, here is for getting the current
state? That should really be better encoded in poth perl module names and api
endpoint paths, and is possibly also a smell about the choosen API path 
hierarchy.

Now, I know we're on a tight schedule here if this should make the next release,
but I cannot just wave _everything_ through, albeit I trust Alexandre's testing big
time, so that helps.

I can do some re-factoring myself, but I'd like to not find out such details on
my own (where's the commit message...? If one adds a module besides the exact same
module/api-endpoint name just differing in singular/plural, this really needs to
be explained somehwere...


>  use PVE::API2::Network::SDN::Dns;
>  
> @@ -35,6 +36,11 @@ __PACKAGE__->register_method ({
>      path => 'controllers',
>  });
>  
> +__PACKAGE__->register_method ({
> +    subclass => "PVE::API2::Network::SDN::Ipam",
> +    path => 'ipam',
> +});
> +
>  __PACKAGE__->register_method ({
>      subclass => "PVE::API2::Network::SDN::Ipams",
>      path => 'ipams',
> diff --git a/src/PVE/API2/Network/SDN/Ipam.pm b/src/PVE/API2/Network/SDN/Ipam.pm
> new file mode 100644
> index 0000000..e71ca7d
> --- /dev/null
> +++ b/src/PVE/API2/Network/SDN/Ipam.pm
> @@ -0,0 +1,221 @@
> +package PVE::API2::Network::SDN::Ipam;
> +
> +use strict;
> +use warnings;
> +
> +use PVE::Tools qw(extract_param);
> +use PVE::Cluster qw(cfs_read_file cfs_write_file);
> +
> +use PVE::Network::SDN;
> +use PVE::Network::SDN::Dhcp;
> +use PVE::Network::SDN::Vnets;
> +use PVE::Network::SDN::Ipams::Plugin;
> +
> +use PVE::JSONSchema qw(get_standard_option);
> +use PVE::RPCEnvironment;
> +
> +use PVE::RESTHandler;
> +
> +use base qw(PVE::RESTHandler);
> +
> +__PACKAGE__->register_method ({
> +    name => 'ipamindex',
> +    path => '',
> +    method => 'GET',
> +    description => 'List PVE IPAM Entries',
> +    protected => 1,
> +    permissions => {
> +	description => "Only list entries where you have 'SDN.Audit' or 'SDN.Allocate' permissions on '/sdn/zones/<zone>/<vnet>'",
> +	user => 'all',
> +    },
> +    parameters => {
> +	additionalProperties => 0,
> +    },
> +    returns => {
> +	type => 'array',
> +    },

any index should have a "links" definition here, otherwise api docs and browser won't
be complete and it's just not nice.

Also wondering, as the other sub-paths you registeter here have three template
components in the API path, but here you only got one index, shouldn't that be
either split over three levels (a bit of a nuisances but mostly boiler plate
code) or be a single endpoint the the actual thing passed as parameter (i.e.,
not part of the URL)