From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <pve-devel-bounces@lists.proxmox.com>
Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68])
by lore.proxmox.com (Postfix) with ESMTPS id 244121FF17C
for <inbox@lore.proxmox.com>; Wed, 2 Apr 2025 12:38:43 +0200 (CEST)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
by firstgate.proxmox.com (Proxmox) with ESMTP id C822DF4EB;
Wed, 2 Apr 2025 12:38:32 +0200 (CEST)
Date: Wed, 02 Apr 2025 12:37:55 +0200
From: Fabian =?iso-8859-1?q?Gr=FCnbichler?= <f.gruenbichler@proxmox.com>
To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>
References: <20250328171340.885413-1-g.goller@proxmox.com>
<20250328171340.885413-41-g.goller@proxmox.com>
In-Reply-To: <20250328171340.885413-41-g.goller@proxmox.com>
MIME-Version: 1.0
User-Agent: astroid/0.16.0 (https://github.com/astroidmail/astroid)
Message-Id: <1743588820.z8iu9l0pdb.astroid@yuna.none>
X-SPAM-LEVEL: Spam detection results: 0
AWL 0.045 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
SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record
SPF_PASS -0.001 SPF: sender matches SPF record
Subject: Re: [pve-devel] [PATCH pve-network 13/17] fabric: openfabric: add
api endpoints
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>
Reply-To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Errors-To: pve-devel-bounces@lists.proxmox.com
Sender: "pve-devel" <pve-devel-bounces@lists.proxmox.com>
some high level comments, see below for details though I haven't
repeated them for all instances:
- lots of schema duplication
- lots of string types without formats
- no indices, but this is a two-level deep nested router with path
parameters/child links..
- ACL paths referenced are not yet valid/accepted by ACL API
- requires SDN.Allocate across the board, even for reading -> might
allow seeing (at least parts) of the config with Audit?
On March 28, 2025 6:13 pm, Gabriel Goller wrote:
> From: Stefan Hanreich <s.hanreich@proxmox.com>
>
> Add CRUD endpoints for the openfabric fabric and node section types.
>
> Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
> Co-authored-by: Gabriel Goller <g.goller@proxmox.com>
> Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
> ---
> src/PVE/API2/Network/SDN/Fabrics/Makefile | 2 +-
> .../API2/Network/SDN/Fabrics/OpenFabric.pm | 348 ++++++++++++++++++
> src/PVE/API2/Network/SDN/Makefile | 1 +
> 3 files changed, 350 insertions(+), 1 deletion(-)
> create mode 100644 src/PVE/API2/Network/SDN/Fabrics/OpenFabric.pm
>
> diff --git a/src/PVE/API2/Network/SDN/Fabrics/Makefile b/src/PVE/API2/Network/SDN/Fabrics/Makefile
> index e433f2e7d0a6..8f7c630ef3ab 100644
> --- a/src/PVE/API2/Network/SDN/Fabrics/Makefile
> +++ b/src/PVE/API2/Network/SDN/Fabrics/Makefile
> @@ -1,4 +1,4 @@
> -SOURCES=OpenFabric.pm Ospf.pm Common.pm
> +SOURCES=OpenFabric.pm Common.pm
this was wrong then in the previous patch ;)
>
>
> PERL5DIR=${DESTDIR}/usr/share/perl5
> diff --git a/src/PVE/API2/Network/SDN/Fabrics/OpenFabric.pm b/src/PVE/API2/Network/SDN/Fabrics/OpenFabric.pm
> new file mode 100644
> index 000000000000..fa5802f97ddf
> --- /dev/null
> +++ b/src/PVE/API2/Network/SDN/Fabrics/OpenFabric.pm
> @@ -0,0 +1,348 @@
> +package PVE::API2::Network::SDN::Fabrics::OpenFabric;
> +
> +use strict;
> +use warnings;
> +
> +use Storable qw(dclone);
> +
> +use PVE::RPCEnvironment;
> +use PVE::Tools qw(extract_param);
> +
> +use PVE::Network::SDN;
> +use PVE::Network::SDN::Fabrics;
> +use PVE::API2::Network::SDN::Fabrics::Common;
> +
> +use PVE::RESTHandler;
> +use base qw(PVE::RESTHandler);
> +
> +__PACKAGE__->register_method({
> + name => 'delete_fabric',
> + path => '{fabric}',
> + method => 'DELETE',
> + description => 'Delete SDN Fabric',
> + protected => 1,
> + permissions => {
> + check => ['perm', '/sdn/fabrics/openfabric/{fabric}', [ 'SDN.Allocate' ]],
pve-access-control patch missing (for all endpoints below)
> + },
> + parameters => {
> + properties => {
> + fabric => {
> + type => 'string',
string without format
> + description => 'The fabric id of the fabric to be deleted',
> + },
> + },
> + },
> + returns => { type => 'null' },
> + code => sub {
> + my ($param) = @_;
> +
> + PVE::Network::SDN::lock_sdn_config(
> + sub {
> + PVE::API2::Network::SDN::Fabrics::Common::delete_fabric("openfabric", $param);
> + }, "delete sdn fabric failed");
lock_sdn_config will return undef anyway unless I am missing something?
> + return undef;
> + },
> +});
> +
> +__PACKAGE__->register_method({
> + name => 'delete_node',
> + path => '{fabric}/node/{node}',
should this maybe live under its own router together with the other
node/ endpoints?
> + method => 'DELETE',
> + description => 'Delete SDN Fabric Node',
> + protected => 1,
> + permissions => {
> + check => ['perm', '/sdn/fabrics/openfabric/{fabric}/node/{node}', [ 'SDN.Allocate' ]],
> + },
> + parameters => {
> + properties => {
> + fabric => {
> + type => 'string',
string without format
> + description => 'The fabric id',
> + },
> + node => {
> + type => 'string',
> + description => 'The hostname of the node to be deleted',
string without format
> + },
> + },
> + },
> + returns => {
> + type => 'null',
> + },
> + code => sub {
> + my ($param) = @_;
> +
> + PVE::Network::SDN::lock_sdn_config(
> + sub {
> + PVE::API2::Network::SDN::Fabrics::Common::delete_node("openfabric", $param);
> + }, "delete sdn fabric node failed");
> + return undef;
same as above
> + },
> +});
> +
> +__PACKAGE__->register_method({
> + name => 'update_fabric',
> + path => '{fabric}',
> + method => 'PUT',
> + description => 'Update SDN Fabric configuration',
> + protected => 1,
> + permissions => {
> + check => ['perm', '/sdn/fabrics/openfabric/{fabric}', [ 'SDN.Allocate' ]],
> + },
> + parameters => {
> + properties => {
> + fabric => {
> + type => 'string',
> + description => 'The fabric id',
string without format
> + },
> + hello_interval => {
> + optional => 1,
> + type => 'integer',
> + description => 'The hello_interval in seconds (1-600)',
this repeats a lot of the schema, would it be possible to get it
generated somehow?
> + },
> + },
> + },
> + returns => { type => 'null' },
> + code => sub {
> + my ($param) = @_;
> +
> + PVE::Network::SDN::lock_sdn_config(
> + sub {
> + PVE::API2::Network::SDN::Fabrics::Common::edit_fabric("openfabric", $param);
> + }, "edit sdn fabric failed");
> + return undef;
same as above
> + },
> +});
> +
> +__PACKAGE__->register_method({
same comments apply here
> + name => 'update_node',
> + path => '{fabric}/node/{node}',
> + method => 'PUT',
> + description => 'Update SDN Fabric Node configuration',
> + protected => 1,
> + permissions => {
> + check => ['perm', '/sdn/fabrics/openfabric/{fabric}/node/{node}', [ 'SDN.Allocate' ]],
> + },
> + parameters => {
> + properties => {
> + fabric => {
> + type => 'string',
> + description => 'The fabric id',
> + },
> + node => {
> + type => 'string',
> + description => 'The hostname of the node',
> + },
> + router_id => {
> + type => 'string',
> + description => 'The Router-ID of this node (will be converted to a real NET later',
> + },
> + interfaces => {
> + type => 'array',
> + description => 'Array of openfabric interfaces as propertystrings',
> + items => {
> + type => 'string',
> + description => 'Propertystring of openfabric interfaces',
> + format => 'pve-sdn-openfabric-interface',
> + },
> + },
> + },
> + },
> + returns => { type => 'null' },
> + code => sub {
> + my ($param) = @_;
> +
> + PVE::Network::SDN::lock_sdn_config(
> + sub {
> + PVE::API2::Network::SDN::Fabrics::Common::edit_node("openfabric", $param);
> + }, "edit sdn fabric node failed");
> + return undef;
> + },
> +});
> +
> +__PACKAGE__->register_method({
> + name => 'get_fabric',
> + path => '{fabric}',
should there be an index listing these, e.g. for pvesh?
> + method => 'GET',
> + description => 'Get SDN Fabric configuration',
> + permissions => {
> + check => ['perm', '/sdn/fabrics/openfabric/{fabric}', [ 'SDN.Allocate' ]],
> + },
> + parameters => {
> + properties => {
> + fabric => {
> + type => 'string',
> + description => 'The fabric id',
> + },
> + },
> + },
> + returns => {
> + type => 'object',
> + properties => {
> + fabric => {
> + type => 'object',
> + description => 'The fabric object',
> + properties => {
> + name => {
> + type => 'string',
> + description => 'The id of the fabric',
> + },
> + loopback_prefix => {
> + type => 'string',
> + description => 'The IP prefix for Loopback IPs',
> + },
> + hello_interval => {
> + optional => 1,
> + type => 'integer',
> + description => 'The global hello_interval option of the fabric, this will be set of on all interfaces automatically',
> + },
> + },
> + },
> + },
> + },
> + code => sub {
> + my ($param) = @_;
> +
> + return PVE::API2::Network::SDN::Fabrics::Common::get_fabric("openfabric", $param);
> + },
> +});
> +
> +__PACKAGE__->register_method({
> + name => 'get_node',
> + path => '{fabric}/node/{node}',
same question here?
> + method => 'GET',
> + description => 'Get SDN Fabric Node configuration',
> + permissions => {
> + check => ['perm', '/sdn/fabrics/openfabric/{fabric}', [ 'SDN.Allocate' ]],
> + },
> + parameters => {
> + properties => {
> + fabric => {
> + type => 'string',
> + description => 'The id of the fabric',
> + },
> + node => {
> + type => 'string',
> + description => 'The hostname of the node',
> + },
> + },
> + },
> + returns => {
> + type => 'object',
> + properties => {
> + node => {
> + type => 'object',
> + description => 'The node object',
> + properties => {
> + router_id => {
> + type => 'string',
> + description => 'The Router-ID of this node (will be converted to a real NET later',
> + },
> + node => {
> + type => 'string',
> + description => 'The hostname of this node',
> + },
> + interface => {
> + type => 'array',
> + description => 'Array of interfaces in this fabric and node',
> + items => {
> + type => 'string',
> + description => 'Propertystring of the interface',
> + format => 'pve-sdn-openfabric-interface',
> + }
> + },
> + }
> + }
> + }
> + },
> + code => sub {
> + my ($param) = @_;
> +
> + return PVE::API2::Network::SDN::Fabrics::Common::get_node("openfabric", $param);
> + },
> +});
> +
> +__PACKAGE__->register_method({
> + name => 'add_fabric',
> + path => '/',
> + method => 'POST',
> + description => 'Create SDN Fabric configuration',
> + protected => 1,
> + permissions => {
> + check => ['perm', '/sdn/fabrics/openfabric', [ 'SDN.Allocate' ]],
> + },
> + parameters => {
> + properties => {
> + fabric_id => {
> + type => 'string',
> + description => 'The id of the fabric',
> + },
> + loopback_prefix => {
> + type => 'string',
> + description => 'The IP prefix for Loopback IPs',
> + },
> + hello_interval => {
> + type => 'number',
> + optional => 1,
> + description => 'The global hello_interval property in seconds, this will be set on all interfaces automatically',
> + }
> + },
> + },
> + returns => { type => 'null' },
> + code => sub {
> + my ($param) = @_;
> +
> + PVE::Network::SDN::lock_sdn_config(
> + sub {
> + PVE::API2::Network::SDN::Fabrics::Common::add_fabric("openfabric", $param);
> + }, "add sdn fabric failed");
> + return undef;
> + },
> +});
> +
> +__PACKAGE__->register_method({
> + name => 'add_node',
so does this add a node
> + path => '{fabric}/node/{node}',
> + method => 'POST',
> + description => 'Create SDN Fabric Node configuration',
or just create/generate a config?
> + protected => 1,
> + permissions => {
> + check => ['perm', '/sdn/fabrics/openfabric/{fabric}/node/{node}', [ 'SDN.Allocate' ]],
> + },
> + parameters => {
> + properties => {
> + fabric => {
> + type => 'string',
> + description => 'The fabric id',
> + },
> + node => {
> + type => 'string',
> + description => 'The node hostname',
> + },
> + router_id => {
> + type => 'string',
> + description => 'The Router-ID of this node (will be converted to a real NET later',
> + },
> + interfaces => {
> + type => 'array',
> + description => 'Array of the interfaces in this openfabric node',
> + items => {
> + type => 'string',
> + description => 'Propertystring of the interface',
> + format => 'pve-sdn-openfabric-interface',
> + },
> + },
> + },
> + },
> + returns => { type => 'null' },
> + code => sub {
> + my ($param) = @_;
> +
> + PVE::Network::SDN::lock_sdn_config(
> + sub {
> + PVE::API2::Network::SDN::Fabrics::Common::add_node("openfabric", $param);
> + }, "add sdn fabric node failed");
> + return undef;
> + },
> +});
> +
> +1;
> diff --git a/src/PVE/API2/Network/SDN/Makefile b/src/PVE/API2/Network/SDN/Makefile
> index 4dbb6c92fd82..08bec7535530 100644
> --- a/src/PVE/API2/Network/SDN/Makefile
> +++ b/src/PVE/API2/Network/SDN/Makefile
> @@ -7,4 +7,5 @@ PERL5DIR=${DESTDIR}/usr/share/perl5
> install:
> for i in ${SOURCES}; do install -D -m 0644 $$i ${PERL5DIR}/PVE/API2/Network/SDN/$$i; done
> make -C Zones install
> + make -C Fabrics install
this also seems like it would belong in a different patch?
>
> --
> 2.39.5
>
>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
>
>
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel