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.

Sign up for GitHub

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

prefix is_pool check for ip assignments #12937

Closed
wants to merge 5 commits into from
Closed

prefix is_pool check for ip assignments #12937

Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1ea0035
prefix is_pool check for ip assignments
ITJamie Jun 19, 2023
89a78a6
linting
ITJamie Jun 19, 2023
a9b2d0e
Update model_forms.py
ITJamie Jun 20, 2023
5cbe332
use cleaned address object
ITJamie Jun 20, 2023
d008b5c
requested changes
ITJamie Jun 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 18 additions & 8 deletions netbox/ipam/forms/model_forms.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -362,15 +362,25 @@ def clean(self):

# Do not allow assigning a network ID or broadcast address to an interface.
if interface and (address := self.cleaned_data.get('address')):
if address.ip == address.network:
msg = f"{address} is a network ID, which may not be assigned to an interface."
if address.version == 4 and address.prefixlen not in (31, 32):
prefix_str = f"{address.network}/{address.prefixlen}"
ITJamie marked this conversation as resolved.
Show resolved Hide resolved
allow_assignment_error = True
if self.instance.vrf is None:
prefix_obj = Prefix.objects.filter(prefix=prefix_str)
else:
prefix_obj = Prefix.objects.filter(prefix=prefix_str, vrf=self.vrf)
if prefix_obj.exists() and prefix_obj[0].is_pool:
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't use indexes on querysets. Use first() if you need the first one.

Copy link
Contributor

Choose a reason for hiding this comment

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

In general is this logic really sound? I'm not actually a networking guy, so I apologize if this doesn't make sense, but what if you have a /26 as a prefix, marked as "Is Pool". You then create a 10.100.0.x/26 in it, that works with your logic. If you create a 10.100.0.x/28. This will still be considered in the prefix by netbox, we do no validation here, but your logic will not find the prefix for it.

Does this really make sen?

Copy link
Contributor Author

@ITJamie ITJamie Jun 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

so from a logic POV.

lets say you have two prefixes.
192.168.0.0/24 - with has_pool = False
192.168.1.0/24 - with has_pool = True

in the following cases:

  • 192.168.0.0/24 - should error if created and attempt to assign to an interface
  • 192.168.0.255/24 - should error if created and attempt to assign to an interface
  • 192.168.1.0/24 - should allow creation and assignment to an interface
  • 192.168.1.255/24 - should allow creation and assignment to an interface

if you then decided to create an assign an IP with NO matching prefix in netbox. it should error if you attempt to assign it to an interface (standard logic as there's nothing to tell it its a pool of ips)

  • 192.168.2.0/24 - should error if created and attempt to assign to an interface
  • 192.168.2.255/24 - should error if created and attempt to assign to an interface

this is making the assumption that a netbox user has correctly made their prefix object with the same prefixlen.
but unless #7845 happens I see no better way, this way is explicit in that the supplied address.cidr has to match an existing prefix if you want the is_pool assignment override logic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't use indexes on querysets. Use first() if you need the first one.

previous version I had used .get() but rightly so it was pointed out that If there was no prefix it would throw an exception.
so i guess which do we prefer, .filter() with .first() or .get() but needing to add error catching (twice as there's two potential places)

allow_assignment_error = False

if allow_assignment_error:
if address.ip == address.network:
msg = f"{address} is a network ID, which may not be assigned to an interface unless the prefix is a pool."
if address.version == 4 and address.prefixlen not in (31, 32):
raise ValidationError(msg)
if address.version == 6 and address.prefixlen not in (127, 128):
raise ValidationError(msg)
if address.version == 4 and address.ip == address.broadcast and address.prefixlen not in (31, 32):
msg = f"{address} is a broadcast address, which may not be assigned to an interface unless the prefix is a pool."
raise ValidationError(msg)
if address.version == 6 and address.prefixlen not in (127, 128):
raise ValidationError(msg)
if address.version == 4 and address.ip == address.broadcast and address.prefixlen not in (31, 32):
msg = f"{address} is a broadcast address, which may not be assigned to an interface."
raise ValidationError(msg)

def save(self, *args, **kwargs):
ipaddress = super().save(*args, **kwargs)
Expand Down