Re: [PATCH pve-network 5/9] fix #7294: sdn: register api formats for zones and vnets

[David Riley <d.riley@proxmox.com>]

Thanks for the feedback.

The intention behind adding this segment to the ACL path is to allow
for fine-grained, hierarchical permission scoping, not to couple the
ACL system to specific VNet properties.

I used vlan as a placeholder for 'tag', but in retrospect, the naming
is a bit confusing, and I'm happy to adapt this in a v2.

 From a permission perspective, including the tag in the path makes
sense, as it allows us to restrict pool users to a specific VNet and
tag combination.

So if you have a pool with a VM, storage and VNet + Tag and assign the
pool permissions: PVEVMAdmin, PVESDNUser

The user can fully manage this VM, including adding a new NIC, but
they can only add it using the exact VNet + Tag combination. Just
adding the VNet would not work.

Let me know if this makes sense.

More inline.

On 6/12/26 2:17 PM, Gabriel Goller wrote:
> Generally on this series:
> What was your rationale on adding the vlan tag? IMO having
> `vnet/<zone>/<vnet>/<vlan>` doesn't really make sense, as not all vnets have
> a tag property, and it's also not always a vlan e.g. EVPN vnets have a vni-tag
> property.
>
> One small comment inline as well.
>
>> [snip]
>> diff --git a/src/PVE/Network/SDN/VnetPlugin.pm b/src/PVE/Network/SDN/VnetPlugin.pm
>> index e04157573083..2299b46601c2 100644
>> --- a/src/PVE/Network/SDN/VnetPlugin.pm
>> +++ b/src/PVE/Network/SDN/VnetPlugin.pm
>> @@ -16,17 +16,34 @@ PVE::Cluster::cfs_register_file(
>>       sub { __PACKAGE__->write_config(@_); },
>>   );
>>   
>> +my $sdn_vnet_id_pattern = '[a-zA-Z][a-zA-Z0-9]*[a-zA-Z0-9]';
>> +my $vnet_min_length = 2;
>> +my $vnet_max_length = 8;
>> +
>>   PVE::JSONSchema::register_standard_option(
>>       'pve-sdn-vnet-id',
>>       {
>>           description => "The SDN vnet object identifier.",
>>           type => 'string',
>> -        pattern => '[a-zA-Z][a-zA-Z0-9]*[a-zA-Z0-9]',
>> -        minLength => 2,
>> -        maxLength => 8,
>> +        pattern => $sdn_vnet_id_pattern,
>> +        minLength => $vnet_min_length,
>> +        maxLength => $vnet_max_length,
>>       },
>>   );
>>   
>> +sub pve_verify_sdn_vnet_id {
>> +    my ($vnet, $noerr) = @_;
>> +
>> +    if ($vnet !~ m/^$sdn_vnet_id_pattern$/) {
>> +        return undef if $noerr;
>> +        die "invalid SDN VNet '$vnet' - must be $vnet_min_length-$vnet_max_length characters"
>> +            . " long, start with a letter, and contain only alphanumeric characters\n";
>> +    }
>> +    return $vnet;
>> +}
> I think this is missing a min/max lenght check?
>
You are right. Will fix this in a v2.
>> +
>> +PVE::JSONSchema::register_format('pve-sdn-vnet-id', \&pve_verify_sdn_vnet_id);
>> +
>>   my $defaultData = {
>>   
>>       propertyList => {
>> diff --git a/src/PVE/Network/SDN/Zones/Plugin.pm b/src/PVE/Network/SDN/Zones/Plugin.pm
>> index 74a3384cd7ae..cd761e0448c3 100644
>> --- a/src/PVE/Network/SDN/Zones/Plugin.pm
>> +++ b/src/PVE/Network/SDN/Zones/Plugin.pm
>> @@ -19,17 +19,34 @@ PVE::Cluster::cfs_register_file(
>>       sub { __PACKAGE__->write_config(@_); },
>>   );
>>   
>> +my $sdn_zone_id_pattern = '[a-zA-Z][a-zA-Z0-9]*[a-zA-Z0-9]';
>> +my $zone_min_length = 2;
>> +my $zone_max_length = 8;
>> +
>>   PVE::JSONSchema::register_standard_option(
>>       'pve-sdn-zone-id',
>>       {
>>           description => "The SDN zone object identifier.",
>>           type => 'string',
>> -        pattern => '[a-zA-Z][a-zA-Z0-9]*[a-zA-Z0-9]',
>> -        minLength => 2,
>> -        maxLength => 8,
>> +        pattern => $sdn_zone_id_pattern,
>> +        minLength => $zone_min_length,
>> +        maxLength => $zone_max_length,
>>       },
>>   );
>>   
>> +sub pve_verify_sdn_zone_id {
>> +    my ($zone, $noerr) = @_;
>> +
>> +    if ($zone !~ m/^$sdn_zone_id_pattern$/) {
>> +        return undef if $noerr;
>> +        die "invalid SDN zone '$zone' - must be $zone_min_length-$zone_max_length characters"
>> +            . " long, start with a letter, and contain only alphanumeric characters\n";
>> +    }
>> +    return $zone;
>> +}
> Here as well.
ack.