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

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

On 11/18/23 17:27, Thomas Lamprecht wrote:
> 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...

All very valid points. One thing I have also considered was /ipams/pve
as a prefix - maybe that one is better suited? What do you think? It
shouldn't be too much of a hassle to move that around.

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

will look into adding that!

> 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)

I was under the impression that DELETE should not include a request body
(at least they're not interoperable according to RFC 9110). I thought I
tried sending a DELETE with a request body to our API, but it didn't get
parsed. After looking through our API docs though I found DELETE
endpoints with optional parameters, so I don't know what went wrong there.

Anyway, that was the main reason why I designed the path the way it is,
since I needed to pass the parameters somehow. The other endpoints were
designed to have the same URL in order to be uniform.

I'll look again into this, maybe POST / PUT / DELETE
`/ipams/pve/mapping` or `/ipams/pve/ip` would be a good alternative
here? We need to move away from MAC addresses as a unique identifier
anyway (since with dual-stack there can be multiple IP addresses for the
same MAC address) so I would need to adjust those endpoints anyway to
work on IP/MAC pairings).

So the endpoints would then look like this:

GET /ipams/pve

POST /ipams/pve/ip
PUT /ipams/pve/ip
DELETE /ipams/pve/ip