[pve-devel] [PATCH pve-network 1/4] subnet: dhcp: improve Net::IP usage

[pve-devel] [PATCH pve-network 1/4] subnet: dhcp: improve Net::IP usage

[Stefan Hanreich]

This simplifies the comparison of IPs by using the object-oriented
interface over the procedural one. Also instantiate the ips using the
new method rather than using new, which isn't a keyword in Perl. This
fixes the respective perlcritic warning.

Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
---
 src/PVE/Network/SDN/SubnetPlugin.pm | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/PVE/Network/SDN/SubnetPlugin.pm b/src/PVE/Network/SDN/SubnetPlugin.pm
index 049f7e1..4bff2dd 100644
--- a/src/PVE/Network/SDN/SubnetPlugin.pm
+++ b/src/PVE/Network/SDN/SubnetPlugin.pm
@@ -86,13 +86,13 @@ sub validate_dhcp_ranges {
 	my $dhcp_start = $dhcp_range->{'start-address'};
 	my $dhcp_end = $dhcp_range->{'end-address'};
 
-	my $start_ip = new Net::IP($dhcp_start);
+	my $start_ip = Net::IP->new($dhcp_start);
 	raise_param_exc({ 'dhcp-range' => "start-adress is not a valid IP $dhcp_start" }) if !$start_ip;
 
-	my $end_ip = new Net::IP($dhcp_end);
+	my $end_ip = Net::IP->new($dhcp_end);
 	raise_param_exc({ 'dhcp-range' => "end-adress is not a valid IP $dhcp_end" }) if !$end_ip;
 
-	if (Net::IP::ip_bincomp($end_ip->binip(), 'lt', $start_ip->binip()) == 1) {
+	if ($start_ip->bincomp('gt', $end_ip)) {
 	    raise_param_exc({ 'dhcp-range' => "start-address $dhcp_start must be smaller than end-address $dhcp_end" })
 	}
 
-- 
2.39.5


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

[pve-devel] [PATCH pve-network 2/4] subnet: dhcp: fix typo in error message

[Stefan Hanreich]

Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
---
 src/PVE/Network/SDN/SubnetPlugin.pm | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/PVE/Network/SDN/SubnetPlugin.pm b/src/PVE/Network/SDN/SubnetPlugin.pm
index 4bff2dd..8733018 100644
--- a/src/PVE/Network/SDN/SubnetPlugin.pm
+++ b/src/PVE/Network/SDN/SubnetPlugin.pm
@@ -87,10 +87,10 @@ sub validate_dhcp_ranges {
 	my $dhcp_end = $dhcp_range->{'end-address'};
 
 	my $start_ip = Net::IP->new($dhcp_start);
-	raise_param_exc({ 'dhcp-range' => "start-adress is not a valid IP $dhcp_start" }) if !$start_ip;
+	raise_param_exc({ 'dhcp-range' => "start-address is not a valid IP $dhcp_start" }) if !$start_ip;
 
 	my $end_ip = Net::IP->new($dhcp_end);
-	raise_param_exc({ 'dhcp-range' => "end-adress is not a valid IP $dhcp_end" }) if !$end_ip;
+	raise_param_exc({ 'dhcp-range' => "end-address is not a valid IP $dhcp_end" }) if !$end_ip;
 
 	if ($start_ip->bincomp('gt', $end_ip)) {
 	    raise_param_exc({ 'dhcp-range' => "start-address $dhcp_start must be smaller than end-address $dhcp_end" })
-- 
2.39.5


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

[pve-devel] [PATCH pve-network 3/4] subnet: dhcp: only accept single ips and normalize them

[Stefan Hanreich]

Net::IP accepts a myriad of different IP objects from ranges to
prefixes to singular IPs. We check if the object consists only of a
singular IP and normalize the IP if it has size 1 (since then it
could still be a /32 prefix or a range consisting of one IP).
Otherwise we would theoretically accept any valid Net::IP object here.

Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
---
 src/PVE/Network/SDN/SubnetPlugin.pm | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/src/PVE/Network/SDN/SubnetPlugin.pm b/src/PVE/Network/SDN/SubnetPlugin.pm
index 8733018..c842450 100644
--- a/src/PVE/Network/SDN/SubnetPlugin.pm
+++ b/src/PVE/Network/SDN/SubnetPlugin.pm
@@ -88,9 +88,13 @@ sub validate_dhcp_ranges {
 
 	my $start_ip = Net::IP->new($dhcp_start);
 	raise_param_exc({ 'dhcp-range' => "start-address is not a valid IP $dhcp_start" }) if !$start_ip;
+	raise_param_exc({ 'dhcp-range' => "start-address must be a singular IP" }) if $start_ip->size() != 1;
+	$dhcp_range->{'start-address'} = $start_ip->ip();
 
 	my $end_ip = Net::IP->new($dhcp_end);
 	raise_param_exc({ 'dhcp-range' => "end-address is not a valid IP $dhcp_end" }) if !$end_ip;
+	raise_param_exc({ 'dhcp-range' => "end-address must be a singular IP" }) if $end_ip->size() != 1;
+	$dhcp_range->{'end-address'} = $end_ip->ip();
 
 	if ($start_ip->bincomp('gt', $end_ip)) {
 	    raise_param_exc({ 'dhcp-range' => "start-address $dhcp_start must be smaller than end-address $dhcp_end" })
-- 
2.39.5


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

[pve-devel] [PATCH pve-network 4/4] subnet: dhcp: do not allow overlapping dhcp ranges

[Stefan Hanreich]

Check for overlapping DHCP ranges and reject them if there are any
overlaps. If we can be certain that there are no overlapping DHCP
ranges this saves us from running into errors later in IPAM modules
where overlapping DHCP ranges are not allowed.

Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
---
 src/PVE/Network/SDN/SubnetPlugin.pm | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/src/PVE/Network/SDN/SubnetPlugin.pm b/src/PVE/Network/SDN/SubnetPlugin.pm
index c842450..b911d69 100644
--- a/src/PVE/Network/SDN/SubnetPlugin.pm
+++ b/src/PVE/Network/SDN/SubnetPlugin.pm
@@ -3,7 +3,7 @@ package PVE::Network::SDN::SubnetPlugin;
 use strict;
 use warnings;
 
-use Net::IP;
+use Net::IP qw($IP_NO_OVERLAP);
 use Net::Subnet qw(subnet_matcher);
 
 use PVE::Cluster qw(cfs_read_file cfs_write_file cfs_lock_file);
@@ -82,6 +82,8 @@ sub validate_dhcp_ranges {
 
     my $dhcp_ranges = PVE::Network::SDN::Subnets::get_dhcp_ranges($subnet);
 
+    my $validated_ranges = [];
+
     foreach my $dhcp_range (@$dhcp_ranges) {
 	my $dhcp_start = $dhcp_range->{'start-address'};
 	my $dhcp_end = $dhcp_range->{'end-address'};
@@ -102,6 +104,15 @@ sub validate_dhcp_ranges {
 
 	raise_param_exc({ 'dhcp-range' => "start-address $dhcp_start is not in subnet $cidr" }) if !$subnet_matcher->($dhcp_start);
 	raise_param_exc({ 'dhcp-range' => "end-address $dhcp_end is not in subnet $cidr" }) if !$subnet_matcher->($dhcp_end);
+
+	my $ip_range = Net::IP->new("$dhcp_range->{'start-address'} - $dhcp_range->{'end-address'}");
+	for my $other_range (@$validated_ranges) {
+	    if ($ip_range->overlaps($other_range) != $Net::IP::IP_NO_OVERLAP) {
+		raise_param_exc({ 'dhcp-range' => "dhcp ranges must not overlap" });
+	    }
+	}
+
+	push @$validated_ranges, $ip_range;
     }
 }
 
-- 
2.39.5


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

Re: [pve-devel] [PATCH pve-network 1/4] subnet: dhcp: improve Net::IP usage

[Hannes Duerr]

Tested all 4 Patches and didn't run into any regressions.
Please consider this

Tested-by: Hannes Duerr <h.duerr@proxmox.com>

On 3/7/25 13:50, Stefan Hanreich wrote:
> This simplifies the comparison of IPs by using the object-oriented
> interface over the procedural one. Also instantiate the ips using the
> new method rather than using new, which isn't a keyword in Perl. This
> fixes the respective perlcritic warning.
>
> Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
> ---
>   src/PVE/Network/SDN/SubnetPlugin.pm | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/src/PVE/Network/SDN/SubnetPlugin.pm b/src/PVE/Network/SDN/SubnetPlugin.pm
> index 049f7e1..4bff2dd 100644
> --- a/src/PVE/Network/SDN/SubnetPlugin.pm
> +++ b/src/PVE/Network/SDN/SubnetPlugin.pm
> @@ -86,13 +86,13 @@ sub validate_dhcp_ranges {
>   	my $dhcp_start = $dhcp_range->{'start-address'};
>   	my $dhcp_end = $dhcp_range->{'end-address'};
>   
> -	my $start_ip = new Net::IP($dhcp_start);
> +	my $start_ip = Net::IP->new($dhcp_start);
>   	raise_param_exc({ 'dhcp-range' => "start-adress is not a valid IP $dhcp_start" }) if !$start_ip;
>   
> -	my $end_ip = new Net::IP($dhcp_end);
> +	my $end_ip = Net::IP->new($dhcp_end);
>   	raise_param_exc({ 'dhcp-range' => "end-adress is not a valid IP $dhcp_end" }) if !$end_ip;
>   
> -	if (Net::IP::ip_bincomp($end_ip->binip(), 'lt', $start_ip->binip()) == 1) {
> +	if ($start_ip->bincomp('gt', $end_ip)) {
>   	    raise_param_exc({ 'dhcp-range' => "start-address $dhcp_start must be smaller than end-address $dhcp_end" })
>   	}
>   


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

[pve-devel] applied: [PATCH pve-network 1/4] subnet: dhcp: improve Net::IP usage

[Thomas Lamprecht]

On Fri, 07 Mar 2025 13:50:53 +0100, Stefan Hanreich wrote:
> This simplifies the comparison of IPs by using the object-oriented
> interface over the procedural one. Also instantiate the ips using the
> new method rather than using new, which isn't a keyword in Perl. This
> fixes the respective perlcritic warning.
> 

Applied, thanks!

[1/4] subnet: dhcp: improve Net::IP usage
      commit: 1a698b27f4e959bb2b63b9393a2c223d32fea020
[2/4] subnet: dhcp: fix typo in error message
      commit: e62b892fdc5d6b94d61af5424a6957983fbeeb29
[3/4] subnet: dhcp: only accept single ips and normalize them
      commit: f5ad8efc4e5fadd24311c06afb2c93a0eb65a575
[4/4] subnet: dhcp: do not allow overlapping dhcp ranges
      commit: 3a904729fdc1e0b6b52e404a56221304cb62c011


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