Fixes #10950 - Add filtering and validation to primary_ip4/primary_ip6 VirtualDeviceContext model #10954
Conversation
netbox/dcim/forms/model_forms.py
Outdated
| @@ -1705,6 +1705,22 @@ class VirtualDeviceContextForm(TenancyForm, NetBoxModelForm): | |||
| 'rack_id': '$rack', | |||
| } | |||
| ) | |||
| primary_ip4 = DynamicModelChoiceField( | |||
There was a problem hiding this comment.
Need to set labels on the form fields to ensure their proper display.
|
|
||
| def clean(self): | ||
| super().clean() | ||
| vc_interfaces = self.device.vc_interfaces(if_master=False) |
There was a problem hiding this comment.
Shouldn't we limit this to interfaces assigned to the VDC?
There was a problem hiding this comment.
Yes, I have went ahead and made this change.
| @@ -866,7 +866,7 @@ def clean(self): | |||
|
|
|||
| # Check for primary IP assignment that doesn't match the assigned device/VM | |||
| if self.pk: | |||
| for cls, attr in ((Device, 'device'), (VirtualMachine, 'virtual_machine')): | |||
| for cls, attr in ((Device, 'device'), (VirtualMachine, 'virtual_machine'), (VirtualDeviceContext, 'device')): | |||
There was a problem hiding this comment.
| for cls, attr in ((Device, 'device'), (VirtualMachine, 'virtual_machine'), (VirtualDeviceContext, 'device')): | |
| for cls, attr in ((Device, 'device'), (VirtualMachine, 'virtual_machine'), (VirtualDeviceContext, 'vdc')): |
There was a problem hiding this comment.
The reverse attribute is vdcs and is a manytomany manager field. I had to do some thing different but perhaps you can take a look and maybe make a suggestion to clean this up better.
| @@ -866,9 +866,16 @@ def clean(self): | |||
|
|
|||
| # Check for primary IP assignment that doesn't match the assigned device/VM | |||
| if self.pk: | |||
| for cls, attr in ((Device, 'device'), (VirtualMachine, 'virtual_machine')): | |||
| for cls, attr in ((Device, 'device'), (VirtualMachine, 'virtual_machine'), (VirtualDeviceContext, 'vdcs')): | |||
There was a problem hiding this comment.
This might be a bit cleaner - I think line 872 comparison of .all() != parent might be wrong, should be first()? I'm assuming the m2m check only needs to be done if 'vdcs', device and virtual_machine attrs aren't m2m.
if self.pk:
for cls, attr in ((Device, 'device'), (VirtualMachine, 'virtual_machine'), (VirtualDeviceContext, 'vdcs')):
parent = cls.objects.filter(Q(primary_ip4=self) | Q(primary_ip6=self)).first()
if parent and hasattr(self.assigned_object, attr):
if attr == 'vdcs'and getattr(self.assigned_object, attr).first() != parent:
raise ValidationError({
'interface': f"IP address is primary for {cls._meta.model_name} {parent} but "
f"not assigned to it!"
})
elif getattr(self.assigned_object, attr, None) != parent:
# Check for a NAT relationship
if not self.nat_inside or getattr(self.nat_inside.assigned_object, attr, None) != parent:
raise ValidationError({
'interface': f"IP address is primary for {cls._meta.model_name} {parent} but "
f"not assigned to it!"
})
There was a problem hiding this comment.
also, move the parent = cls.objects outside the loop:
if self.pk:
parent = cls.objects.filter(Q(primary_ip4=self) | Q(primary_ip6=self)).first()
for cls, attr in ((Device, 'device'), (VirtualMachine, 'virtual_machine'), (VirtualDeviceContext, 'vdcs')):
Considering this further, we probably don't want to force this limitation. I can see a case where the primary IP address via which a VDC is managed does not belong to the VDC itself (e.g. an out-of-band or otherwise "special" IP). It does need to be validated against the parent device though. Additionally, we've moved away from statically rendering choice fields; selection options should always be populated via the REST API. And the primary IP validation under the IPAddress model has since been removed as part of unrelated work. I've committed b2f34ce which I believe fully resolves #10950 and supersedes this PR. Open to further discussion re: validation, but that should occur outside the context of this particular bug. |
Fixes: #10950