From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: <s.hanreich@proxmox.com> Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 155829B236 for <pve-devel@lists.proxmox.com>; Mon, 20 Nov 2023 11:55:24 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id F250817634 for <pve-devel@lists.proxmox.com>; Mon, 20 Nov 2023 11:55:23 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for <pve-devel@lists.proxmox.com>; Mon, 20 Nov 2023 11:55:22 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id ABBA3433F9; Mon, 20 Nov 2023 11:55:22 +0100 (CET) Message-ID: <bd53c06c-ee8d-4a79-a9ca-8c6218b5c215@proxmox.com> Date: Mon, 20 Nov 2023 11:55:21 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird To: Thomas Lamprecht <t.lamprecht@proxmox.com>, Proxmox VE development discussion <pve-devel@lists.proxmox.com> References: <20231117114011.834002-1-s.hanreich@proxmox.com> <20231117114011.834002-11-s.hanreich@proxmox.com> <c7bb0c66-d789-4bce-86a3-b840a276be9b@proxmox.com> Content-Language: en-US From: Stefan Hanreich <s.hanreich@proxmox.com> In-Reply-To: <c7bb0c66-d789-4bce-86a3-b840a276be9b@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.276 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment POISEN_SPAM_PILL 0.1 Meta: its spam POISEN_SPAM_PILL_1 0.1 random spam to be learned in bayes POISEN_SPAM_PILL_3 0.1 random spam to be learned in bayes SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record T_SCC_BODY_TEXT_LINE -0.01 - Subject: Re: [pve-devel] [PATCH v4 pve-network 10/33] api: add endpoints for managing PVE IPAM X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion <pve-devel.lists.proxmox.com> List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pve-devel>, <mailto:pve-devel-request@lists.proxmox.com?subject=unsubscribe> List-Archive: <http://lists.proxmox.com/pipermail/pve-devel/> List-Post: <mailto:pve-devel@lists.proxmox.com> List-Help: <mailto:pve-devel-request@lists.proxmox.com?subject=help> List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel>, <mailto:pve-devel-request@lists.proxmox.com?subject=subscribe> X-List-Received-Date: Mon, 20 Nov 2023 10:55:24 -0000 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