Re: [pve-devel] [PATCH network 2/4] fix #5900: add helper functions

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

I think in this case it is a lot easier to ask for forgiveness rather
than asking for permission. One could just try to allocate the IP and
check if that fails, avoiding the need to implement a complete check for
the IP allocation.

Checking beforehand also leads to some TOCTOU issues (admittedly small,
since in the window between the check and the actual IP allocation the
state of the IPAM could change).

I talked with Daniel off-list and the initial implementation actually
did only try to allocate and then error out, but the reasoning for
implementing it this way was: If you create a VM and then allocating an
IP fails the wizard closes, but the task fails. The user would then have
to create the VM configuration completely from scratch, which is bad UX.
So the tradeoff here is UX vs some technical reasons (TOCTOU,
implementation complexity).

I think Daniel also talked to you off-list @Aaron, so it'd be great if
you could chime in here.


Nevertheless I have added some notes on the Perl code, regardless of
which option we choose, although I think we could do completely without
implementing a manual check which would obsolete this commit. But the
general ideas apply for future patch series as well, so I thought they'd
help either way. Some general notes on the architecture:

If we do it this way (see top-level discussion), I think we should
abstract this into the IpamPlugins itself, since this implementation is
specific to the PVE Plugin, but that's just one type of IPAM plugin.
Something like:

Add a abstract method in the base Ipam plugin
(Network/SDN/Ipams/Plugin.pm), i.e.

  PVE::Network::SDN::Ipams::Plugin::vnet_has_free_ip($range, $ipversions)

Then implement it for every IPAM Plugin separately.

Add a helper method to the VNet that selects the correct plugin based on
the zone setting and then iterates over all its subnets to check for
free IPs - something like:

  PVE::Network::SDN::Vnets::has_free_ip($range, $ipversions)



The current implementation only works for the PVE plugin and would
actually break on zones using Netbox / Phpipam (if my brain compiler is
correct).


On 12/5/24 17:33, Daniel Herzig wrote:
> This patch adds helper functions to evaluate if a vnet (bridge) associated
> with a zone under SDN's auto-dhcp control via dnsmasq can retrieve a
> dhcp lease.
> 
> Signed-off-by: Daniel Herzig <d.herzig@proxmox.com>
> ---
>  src/PVE/Network/SDN/Dhcp.pm | 83 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 83 insertions(+)
> 
> diff --git a/src/PVE/Network/SDN/Dhcp.pm b/src/PVE/Network/SDN/Dhcp.pm
> index d48de34..4ddd128 100644
> --- a/src/PVE/Network/SDN/Dhcp.pm
> +++ b/src/PVE/Network/SDN/Dhcp.pm
> @@ -128,4 +128,87 @@ sub regenerate_config {
>      }
>  }
>  
> +sub defined_dhcp_ip_count_in_zone {
> +    my $zone_id = shift;

even with 1 argument I think we prefer `my ($arg) = @_;`, but I haven't
actually found a definitive answer in our style guide.

> +    my $vnets_in_zone = PVE::Network::SDN::Zones::get_vnets($zone_id);
> +    my $range_count_array;
> +    my $res;
> +    for my $vnet_id (keys %$vnets_in_zone) {
> +	my $subnets_in_vnet = PVE::Network::SDN::Vnets::get_subnets($vnet_id);
> +	for my $subnet (keys %$subnets_in_vnet) {
> +	    my $dhcp_ranges = PVE::Network::SDN::Subnets::get_dhcp_ranges(${subnets_in_vnet}->{$subnet});
> +	    if (scalar @$dhcp_ranges) {
> +		for my $dhcp_range (@$dhcp_ranges) {

You can just iterate over @$dhcp_ranges, get_dhcp_ranges() always
returns an array reference. If it is empty, then there are just 0
iterations of the loop, no need to check for existence.

> +		    my $start_ip = ${dhcp_range}->{'start-address'};

$dhcp_range is fine instead of ${dhcp_range}

> +		    my $end_ip = ${dhcp_range}->{'end-address'};
> +		    my $subnet_ip_count = new Net::IP("$start_ip - $end_ip")->size();
> +		    push (@$range_count_array, $subnet_ip_count);
> +		}
> +	    }
> +	}
> +    }
> +    if ($range_count_array) {
> +	$res = eval join '+', @$range_count_array;

I think it would be easier to just define the variable at the top to be 0:

my $res = 0;

and then add the size to it during the loop:

$count += new Net::IP("$start_ip - $end_ip")->size();

> +    }
> +    return $res;
> +}
> +> +sub used_dhcp_ips_in_zone {
> +    my $zone_id = shift;
> +    my $pve_ipam_db = PVE::Network::SDN::Ipams::PVEPlugin::read_db();
> +    my $subnets_in_zone = $pve_ipam_db->{'zones'}->{$zone_id}->{'subnets'};
> +    my $res;

similarly to the above method we could define res as 0 here

> +    for my $subnet_in_zone (keys %$subnets_in_zone) {
> +	my $ips_in_subnet = ${subnets_in_zone}->{$subnet_in_zone}->{'ips'};

After moving this to PVEPlugin, this could maybe also be split into some
helper functions like

PVEPlugin::get_ips_for_subnet()

or something.

> +	if (scalar (keys %$ips_in_subnet)) {
> +	    for my $leased_ip (keys %$ips_in_subnet) {
> +		$res++ if (!exists ${ips_in_subnet}->{$leased_ip}->{'gateway'});
> +	    }
> +	}
> +    }
> +    return $res;
> +}
> +
> +sub available_dhcp_ips_in_zone {
> +    my $zone_id = shift;
> +    my $available_ip_count = defined_dhcp_ip_count_in_zone($zone_id);
> +    my $used_ip_count = used_dhcp_ips_in_zone($zone_id);
> +    if (!defined($available_ip_count)) {
> +	$available_ip_count = 0;
> +    }
> +    if (!defined($used_ip_count)) {
> +	$used_ip_count = 0;
> +    }

If you define $res to be 0 as suggested above, then those checks become
unnecessary, since 0 becomes the default value.

> +    my $res = $available_ip_count - $used_ip_count;
> +    return $res;
> +}
> +
> +sub test_bridge_for_sdn_dnsmasq {

this function would basically be the helper inside the VNet - it just
needs to consider the IPAM plugin as well.

> +    my $bridge = shift;
> +    my $vnets_cfg = PVE::Network::SDN::Vnets::config();

config() takes $running as parameter. With SDN we have a running config
and a 'normal' config (that stores all changes you make before applying
them). In this case we need to use the running config rather than the
normal config.

> +    my $vnet_ids = [ PVE::Network::SDN::Vnets::sdn_vnets_ids($vnets_cfg) ];

no need for selecting the ids, you can directly iterate over the VNets:

    my $vnets = PVE::Network::SDN::Vnets::config(1);
    foreach my $vnet_id (sort keys %{$vnets->{ids}}) {
        $vnet = PVE::Network::SDN::Vnets::sdn_vnets_config($vnets,$vnet_id);
    }

With Vnets there is no difference, but with Subnets there is a subtle
difference, since it generates some additional keys from the section_id
(see Subnets.pm). So it is preferable to go via this function

-----

An even easier solution - using the fact that VNet names are always
equivalent to the bridge names:

  my $vnet = PVE::Network::SDN::Vnets::get_vnet($bridge)

This would give you the respective VNet (or undef if it isn't a VNet)
that corresponds to the bridge.

Then you just need to get the zone for the VNet:

  my $zone = PVE::Network::SDN::Zones::get_zone($vnet->{zone});

and then you could just easily check if that zone has DHCP enabled,
saving you both loops.

> +    my $zones_cfg = PVE::Network::SDN::Zones::config();
> +    my $zone_ids = [ PVE::Network::SDN::Zones::sdn_zones_ids($zones_cfg) ];

same as above - maybe even just defining a helper would be even better?

> +    my $dhcp_dnsmasq_zones;
> +    my $vnets_in_dhcp_dnsmasq_zones;
> +    for my $zone (@$zone_ids) {
> +	push(@$dhcp_dnsmasq_zones, $zone)
> +	    if (defined(${zones_cfg}->{'ids'}->{$zone}->{'dhcp'}) &&
> +		(${zones_cfg}->{'ids'}->{$zone}->{'dhcp'} eq 'dnsmasq'))
> +    }
> +    for my $vnet (@$vnet_ids) {
> +	my $vnet_zone = ${vnets_cfg}->{'ids'}->{$vnet}->{'zone'};
> +	push(@$vnets_in_dhcp_dnsmasq_zones, $vnet)
> +	    if ("@$dhcp_dnsmasq_zones" =~ /$vnet_zone/)

If you want to check for containment in an array it is preferable to use
grep:

my @array = (1, 2, 3);
if (grep {$_  eq 1} @array)

> +    }
> +    if (("@$vnets_in_dhcp_dnsmasq_zones" =~ /$bridge/)) {

same

> +	my $zone_id = ${vnets_cfg}->{'ids'}->{$bridge}->{'zone'};
> +	if ((PVE::Network::SDN::Dhcp::available_dhcp_ips_in_zone($zone_id)) lt 1) {
> +	    die "No DHCP leases left in zone '$zone_id' for bridge '$bridge', please check your SDN config.\n";
> +	}
> +    }
> +}
> +
> +
> +
>  1;



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel