Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes: #17358 - Ensure correct comparison of overlapping IPRanges #17391

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

bctiemann
Copy link
Contributor

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.

Copy link
Collaborator

@arthanson arthanson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems good, just wondering if it makes sense to make the code a bit more DRY.

netbox/ipam/lookups.py Outdated Show resolved Hide resolved
Copy link
Member

@jeremystretch jeremystretch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than introduce new lookups for these specific comparisons, we should be able to leverage Postgres' HOST() function. Unfortunately, the NetHost() lookup won't support this, but we can instead register the Host() transform on IPAddressField:

>>> from ipam.fields import IPAddressField
>>> from ipam import lookups
>>> IPAddressField.register_lookup(lookups.Host)
<class 'ipam.lookups.Host'>
>>> IPRange.objects.filter(start_address__host__gte="1.2.3.100", start_address__host__lte="1.2.3.200")
<RestrictedQuerySet [<IPRange: 1.2.3.123-124/26>]>

(Ultimately, it probably makes sense to retire some of these lookups in favor of the transforms, but that's a separate discussion.)

@bctiemann
Copy link
Contributor Author

I don't think that works actually; the Host transform only casts the input value to a HOST (i.e. HOST(%s)), but in order to compare to a bare IP address on the rhs you have to cast it to an INET e.g. CAST(HOST(%s) AS INET). I don't think Transform supports that kind of structure unless I'm missing something.

At any rate the comparisons in my unit test fail and it throws an overlapping exception when trying to add 1.2.3.100/24-1.2.3.199/24 (it says it overlaps with 1.2.3.1-99/24).

That's why I created the four specific comparison lookups in the first place, because using HOST on its own leads to incorrect comparisons.

... (30 minutes later) ...

After some more digging into the Transform class, I've added a HostAsInet Transform that does what we would need for this lookup using the start_address__host_as_inet__gte lookup syntax. If you like this we can use it instead of the four specific lookups I did earlier (and remove those).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Overlapping IPRanges are accepted if prefix lengths are different
3 participants