From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id 5F76A1FF13A for ; Wed, 24 Jun 2026 09:33:14 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id DAF621067E; Wed, 24 Jun 2026 09:33:11 +0200 (CEST) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Wed, 24 Jun 2026 09:32:36 +0200 Message-Id: Subject: Re: [PATCH pve-manager 2/9] fix #7294: api: pool: add SDN VNets as pool members From: "Daniel Kral" To: "David Riley" , X-Mailer: aerc 0.21.0-145-gf21bb67f8cab-dirty References: <20260611145935.147788-1-d.riley@proxmox.com> <20260611145935.147788-3-d.riley@proxmox.com> In-Reply-To: X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1782286344388 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.074 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 Message-ID-Hash: K3YIVOPHLYLX2UZWO44V6XYVZNE44U3M X-Message-ID-Hash: K3YIVOPHLYLX2UZWO44V6XYVZNE44U3M X-MailFrom: d.kral@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On Mon Jun 22, 2026 at 3:37 PM CEST, David Riley wrote: > On 6/22/26 1:20 PM, Daniel Kral wrote: >> On Thu Jun 11, 2026 at 4:59 PM CEST, David Riley wrote: [ snip ] >>> @@ -243,6 +272,25 @@ __PACKAGE__->register_method({ >>> format =3D> 'pve-storage-id-list', >>> optional =3D> 1, >>> }, >>> + zone =3D> { >>> + description =3D> 'SDN Zone', >>> + type =3D> 'string', >>> + format =3D> 'pve-sdn-zone-id', >>> + optional =3D> 1, >>> + }, >>> + vnet =3D> { >>> + description =3D> 'VNet to add or remove from this pool= .', >>> + type =3D> 'string', >>> + format =3D> 'pve-sdn-vnet-id', >>> + optional =3D> 1, >>> + }, >> The other parameters like 'vms' and 'storage' allow specifying a list of >> them to be added or removed at a single time, would that also make sense >> with VNets? > > I actually initially allowed for a list of VNets, but it increased the > complexity significantly, so I opted to keep it simple for now. > Good to know, yeah if there's currently no strong demand for it then it's better left out for now and it can be added later anyway as it can be easily extended. >> and I'm not sure if there is a high demand for adding multiple VNets in = one go. >> Either way, is there some way to make zone, vnetid and tag a single >> property so it's clear that zone and vnet are required to add a VNet and >> tag is optional? >> > I can definitely update this if there is a strong demand for it. > I'll think about combining them or at least giving a clear error message > if only the VNet is supplied. > Right, I only thought about it as 'vms' and 'storage' does specify one entity and there seems to be 2 (3?) for VNet entries, and it would make the extra error path for one of them missing obsolete. Maybe there's some way how these can be specified in one string with the tag being optional at the end? Though I don't know enough about the SDN stack here. >>> + tag =3D> { >>> + description =3D> "Specify a VLAN tag", >>> + optional =3D> 1, >>> + type =3D> 'integer', >>> + minimum =3D> 1, >>> + maximum =3D> 4094, >>> + }, >>> 'allow-move' =3D> { >>> description =3D> 'Allow adding a guest even if alread= y in another pool.' >>> . ' The guest will be removed from its current po= ol and added to this one.', [ snip ] >>> + >>> + my $zones_cfg =3D PVE::Network::SDN::Zones::config= (); >>> + my $zone =3D $param->{zone}; >>> + >>> + if (!$zones_cfg->{ids}->{$zone}) { >>> + die "SDN Zone '$zone' does not exist\n"; >>> + } >>> + >>> + my $vnets_cfg =3D PVE::Network::SDN::Vnets::config= (); >>> + my $tag =3D $param->{tag}; >>> + >>> + my $vnetid =3D $param->{vnet}; >>> + my $vnet_data =3D $vnets_cfg->{ids}->{$vnetid} >>> + or die "VNet '$vnetid' does not exist\n"; >>> + >>> + my $vnet_zone =3D $vnet_data->{zone}; >>> + if ($zone ne $vnet_zone) { >>> + die "VNet '$vnetid' does not belong to zone '$= zone' (it belongs to" >>> + . " '$vnet_zone')\n"; >>> + } >> nit: might be nice to make this a single >> >> die "VNet ...\n" if $zone ne $vnet_data->{zone}; >> >> to not have two variables with essentially the same content after this? >> > I'm using the $vnet_zone further down as well that's why I pulled it out. Yes, but if $zone eq $vnet_data->{zone} is required to continue on here, the additional variable is only a duplicate, no? >>> + >>> + my $has_tag =3D defined($tag) && $tag ne ''; >> When is $tag eq '' true? Shouldn't the API verification already catch >> that $tag is a string even though it should be an integer? > You are right. Not sure what i was thinking here. All good :) >>> + if ($has_tag) { >>> + my $native_tag =3D $vnet_data->{tag}; >>> + if (!defined($native_tag) || $tag !=3D $native= _tag) { >>> + die >>> + "VNet '$vnetid' is not VLAN-aware, can= not assign a specific tag\n" >>> + if !$vnet_data->{vlanaware}; >>> + } >>> + } >>> + >>> + my $network_key =3D "vnet/$vnet_zone/$vnetid"; >>> + $network_key .=3D "/$tag" if $has_tag; >> nit: not sure about this, but could also be put in the previous if >> block, but I see the point that this should probably be tightly pu= t >> together so it's easy to see the possible contents of $network_key > Yes, my intention was to make it clear how the final network_key will loo= k like. > I will move it into the if in v2, my initial thought was that the if migh= t error out, but > it makes more sense to prioritize the happy path. I see, no strong feelings about this, so choose as you like. I only thought that it also makes clear that the $network_key changes if the optional $tag is given by the user. >>> + >>> + $rpcenv->check_perm_modify( >>> + $authuser, >>> + "/sdn/zones/$vnet_zone/$vnetid", >>> + ['SDN.Allocate'], >>> + ); >>> + >>> + if ($param->{delete}) { >>> + delete $pool_config->{network}->{$network_key}= ; >>> + } else { >>> + $pool_config->{network}->{$network_key} =3D 1; >>> + } >>> + } >>> + >>> cfs_write_file("user.cfg", $usercfg); >>> }, >>> "update pools failed", [ snip ]