Repository: netbox-community/netbox
Pull request: Fixes: #17358 - Ensure correct comparison of overlapping IPRanges
### Fixes: #17358

Adds new Lookup classes for `lt/gt/lte/gte` comparisons with INET objects, ignoring the mask. This enables correct comparison of adjacent IPRanges when looking for overlaps, and prevents acceptance of an invalid new IPRange where the entered start_address and end_address masks do not match the existing ranges.

Problem statement:
Overlapping IPRanges are accepted if prefix lengths are different
### Deployment Type

Self-hosted

### NetBox Version

v4.0.11

### Python Version

3.12

### Steps to Reproduce

1. Create an IPRange with start 1.2.3.100/24 and end 1.2.3.200/24
2. Create an IPRange with start 1.2.3.123/26 and end 1.2.3.124/26

### Expected Behavior

The second IPRange should be rejected, as it overlaps with the first. (There is code in `IPRange.clean()` which is clearly intended to do this)

### Observed Behavior

The overlapping range is accepted.

The problem occurs because the Django ORM lte/gte filter for overlapping ranges only works if the prefix lengths match:

```
>>> ip = "1.2.3.101/24"
>>> IPRange.objects.filter(start_address__lte=ip, end_address__gte=ip, vrf=None)
<RestrictedQuerySet [<IPRange: 1.2.3.100-200/24>]>
>>> ip = "1.2.3.101/26"
>>> IPRange.objects.filter(start_address__lte=ip, end_address__gte=ip, vrf=None)
<RestrictedQuerySet []>
```

I cannot see an obvious solution here, apart from brute force iterating over all possible prefix lengths:

```
>>> [IPRange.objects.filter(start_address__lte=f"1.2.3.101/{pl}", end_address__gte=f"1.2.3.101/{pl}", vrf=None).count() for pl in range(33)]
[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 0]
```

Alternatively, changing the data model so that a range uses IPAddress instead of IPNetwork for the start/end addresses would eliminate this problem.

Hints from issue discussion:

Commit message: Add new INET lookups for net_host_lt/gt/lte/gte comparisons irrespective of subnet inclusion

Patch to review:
diff --git a/netbox/ipam/fields.py b/netbox/ipam/fields.py
index 20341005d98..a8682e6004c 100644
--- a/netbox/ipam/fields.py
+++ b/netbox/ipam/fields.py
@@ -105,6 +105,10 @@ def db_type(self, connection):
 IPAddressField.register_lookup(lookups.NetHostContained)
 IPAddressField.register_lookup(lookups.NetFamily)
 IPAddressField.register_lookup(lookups.NetMaskLength)
+IPAddressField.register_lookup(lookups.NetHostLessThan)
+IPAddressField.register_lookup(lookups.NetHostLessThanOrEqual)
+IPAddressField.register_lookup(lookups.NetHostGreaterThan)
+IPAddressField.register_lookup(lookups.NetHostGreaterThanOrEqual)
 
 
 class ASNField(models.BigIntegerField):
diff --git a/netbox/ipam/lookups.py b/netbox/ipam/lookups.py
index c6abb5a26a3..95fd1ce0504 100644
--- a/netbox/ipam/lookups.py
+++ b/netbox/ipam/lookups.py
@@ -154,6 +154,46 @@ def as_sql(self, qn, connection):
         return 'CAST(HOST(%s) AS INET) <<= %s' % (lhs, rhs), params
 
 
+class NetHostGreaterThan(Lookup):
+    lookup_name = 'net_host_gt'
+
+    def as_sql(self, qn, connection):
+        lhs, lhs_params = self.process_lhs(qn, connection)
+        rhs, rhs_params = self.process_rhs(qn, connection)
+        params = lhs_params + rhs_params
+        return 'CAST(HOST(%s) AS INET) > INET %s' % (lhs, rhs), params
+
+
+class NetHostLessThan(Lookup):
+    lookup_name = 'net_host_lt'
+
+    def as_sql(self, qn, connection):
+        lhs, lhs_params = self.process_lhs(qn, connection)
+        rhs, rhs_params = self.process_rhs(qn, connection)
+        params = lhs_params + rhs_params
+        return 'CAST(HOST(%s) AS INET) < INET %s' % (lhs, rhs), params
+
+
+class NetHostGreaterThanOrEqual(Lookup):
+    lookup_name = 'net_host_gte'
+
+    def as_sql(self, qn, connection):
+        lhs, lhs_params = self.process_lhs(qn, connection)
+        rhs, rhs_params = self.process_rhs(qn, connection)
+        params = lhs_params + rhs_params
+        return 'CAST(HOST(%s) AS INET) >= INET %s' % (lhs, rhs), params
+
+
+class NetHostLessThanOrEqual(Lookup):
+    lookup_name = 'net_host_lte'
+
+    def as_sql(self, qn, connection):
+        lhs, lhs_params = self.process_lhs(qn, connection)
+        rhs, rhs_params = self.process_rhs(qn, connection)
+        params = lhs_params + rhs_params
+        return 'CAST(HOST(%s) AS INET) <= INET %s' % (lhs, rhs), params
+
+
 class NetFamily(Transform):
     lookup_name = 'family'
     function = 'FAMILY'
diff --git a/netbox/ipam/models/ip.py b/netbox/ipam/models/ip.py
index 0b8e3a8dfc8..09d0ba1e4e9 100644
--- a/netbox/ipam/models/ip.py
+++ b/netbox/ipam/models/ip.py
@@ -580,15 +580,15 @@ def clean(self):
                 })
 
             # Check for overlapping ranges
-            overlapping_range = IPRange.objects.exclude(pk=self.pk).filter(vrf=self.vrf).filter(
-                Q(start_address__gte=self.start_address, start_address__lte=self.end_address) |  # Starts inside
-                Q(end_address__gte=self.start_address, end_address__lte=self.end_address) |  # Ends inside
-                Q(start_address__lte=self.start_address, end_address__gte=self.end_address)  # Starts & ends outside
-            ).first()
-            if overlapping_range:
+            overlapping_ranges = IPRange.objects.exclude(pk=self.pk).filter(vrf=self.vrf).filter(
+                Q(start_address__net_host_gte=self.start_address.ip, start_address__net_host_lte=self.end_address.ip) |  # Starts inside
+                Q(end_address__net_host_gte=self.start_address.ip, end_address__net_host_lte=self.end_address.ip) |  # Ends inside
+                Q(start_address__net_host_lte=self.start_address.ip, end_address__net_host_gte=self.end_address.ip)  # Starts & ends outside
+            )
+            if overlapping_ranges.exists():
                 raise ValidationError(
                     _("Defined addresses overlap with range {overlapping_range} in VRF {vrf}").format(
-                        overlapping_range=overlapping_range,
+                        overlapping_range=overlapping_ranges.first(),
                         vrf=self.vrf
                     ))
 
diff --git a/netbox/ipam/tests/test_models.py b/netbox/ipam/tests/test_models.py
index 39eb33a4fcb..5c2c7c79a64 100644
--- a/netbox/ipam/tests/test_models.py
+++ b/netbox/ipam/tests/test_models.py
@@ -36,6 +36,35 @@ def test_get_utilization(self):
         self.assertEqual(aggregate.get_utilization(), 100)
 
 
+class TestIPRange(TestCase):
+
+    def test_overlapping_range(self):
+        iprange_192_168 = IPRange.objects.create(start_address=IPNetwork('192.168.0.1/22'), end_address=IPNetwork('192.168.0.49/22'))
+        iprange_192_168.clean()
+        iprange_3_1_99 = IPRange.objects.create(start_address=IPNetwork('1.2.3.1/24'), end_address=IPNetwork('1.2.3.99/24'))
+        iprange_3_1_99.clean()
+        iprange_3_100_199 = IPRange.objects.create(start_address=IPNetwork('1.2.3.100/24'), end_address=IPNetwork('1.2.3.199/24'))
+        iprange_3_100_199.clean()
+        iprange_3_200_255 = IPRange.objects.create(start_address=IPNetwork('1.2.3.200/24'), end_address=IPNetwork('1.2.3.255/24'))
+        iprange_3_200_255.clean()
+        iprange_4_1_99 = IPRange.objects.create(start_address=IPNetwork('1.2.4.1/24'), end_address=IPNetwork('1.2.4.99/24'))
+        iprange_4_1_99.clean()
+        iprange_4_200 = IPRange.objects.create(start_address=IPNetwork('1.2.4.200/24'), end_address=IPNetwork('1.2.4.255/24'))
+        iprange_4_200.clean()
+        # Overlapping range entirely within existing
+        with self.assertRaises(ValidationError):
+            iprange_3_123_124 = IPRange.objects.create(start_address=IPNetwork('1.2.3.123/26'), end_address=IPNetwork('1.2.3.124/26'))
+            iprange_3_123_124.clean()
+        # Overlapping range starting within existing
+        with self.assertRaises(ValidationError):
+            iprange_4_98_101 = IPRange.objects.create(start_address=IPNetwork('1.2.4.98/24'), end_address=IPNetwork('1.2.4.101/24'))
+            iprange_4_98_101.clean()
+        # Overlapping range ending within existing
+        with self.assertRaises(ValidationError):
+            iprange_4_198_201 = IPRange.objects.create(start_address=IPNetwork('1.2.4.198/24'), end_address=IPNetwork('1.2.4.201/24'))
+            iprange_4_198_201.clean()
+
+
 class TestPrefix(TestCase):
 
     def test_get_duplicates(self):


Code review comments:
diff_hunk: @@ -154,6 +154,46 @@ def as_sql(self, qn, connection):
         return 'CAST(HOST(%s) AS INET) <<= %s' % (lhs, rhs), params
 
 
+class NetHostGreaterThan(Lookup):
path: netbox/ipam/lookups.py
text: @user1:
Would it make sense to DRY the code here as the CAST looks like the only difference, so move:
```
         lhs, lhs_params = self.process_lhs(qn, connection)
         rhs, rhs_params = self.process_rhs(qn, connection)
         params = lhs_params + rhs_params
```
to a get_params call?

@author:
Yeah, it's a tricky question because a lot of the existing `Lookup` subclasses in this file all use the same `process_lhs` and `process_rhs` library calls, but they do different things with them where these four new ones all just assign them to `params` and then use `lhs` and `rhs` with it in the query builder. I agree it's repetitive but I would be wanting to generalize this logic in some way (i.e. in a mixin) that could be applied to the existing lookups as well but I'm not sure I see an obvious way to do that.

Maybe a more sensible way would be to refactor these into four subclasses of a base class where the part that differs is just the actual final line of `as_sql`. Let me try that...