prefix is_pool check for ip assignments #12937
Conversation
|
Please open a new issue to track these changes, as #12687 has already been closed. |
|
created #12962 |
There was a problem hiding this comment.
Personally I think this whole thing should be reverted and users encouraged to use custom validators to enforce it instead. The logic is getting more and more convoluted, there's been a bunch of edgecases and in general we don't really do strict validation of stuff like this for most of netbox.
The validation is also still only present in the form, so bulk editing and the API have no restrictions.
If it must be merged, maybe it would be the time to add some utility methods to the IPAddress model to allow finding the parent prefix(s) more easily. Would be useful for configuration generation and custom scripts as well.
netbox/ipam/forms/model_forms.py
Outdated
| 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: |
There was a problem hiding this comment.
Don't use indexes on querysets. Use first() if you need the first one.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
Im in agreement actually. That or add a configuration setting to disable this validation chunk completely
I would agree but at that point were starting to encroach on #7845 when it comes to relationship finding between prefixes and ips |
|
Closing out per #12962 |
Fixes: #12962
some users pointed out in #12687 that I missed checking for prefixes where is_pool is true with my previous patch.
I added some logic to find a prefix matching the IP and checks to see if is_pool is set.
It is also safe for ip's that have no matching prefix in netbox.