review feedback

netbox/dcim/forms/object_create.py

@@ -385,11 +385,19 @@ class VirtualChassisCreateForm(OwnerMixin, NetBoxModelForm):
     def clean(self):
         super().clean()
 
-        if self.cleaned_data['members'] and self.cleaned_data['initial_position'] is None:
+        members = self.cleaned_data.get('members')
+        if members and self.cleaned_data['initial_position'] is None:
             raise forms.ValidationError({
                 'initial_position': _("A position must be specified for the first VC member.")
             })
 
+        # Members are saved without full_clean(), so check cross-chassis LAGs here
+        for member in members or []:
+            try:
+                member.clean_vc_lag_membership(members=members)
+            except forms.ValidationError as e:
+                raise forms.ValidationError({'members': e.messages})
+
     def save(self, *args, **kwargs):
         instance = super().save(*args, **kwargs)
 

netbox/dcim/models/devices.py

@@ -1024,13 +1024,17 @@ class Device(
             if self.virtual_chassis_id != stored_vc_id:
                 self.clean_vc_lag_membership()
 
-    def clean_vc_lag_membership(self):
+    def clean_vc_lag_membership(self, members=None):
         """
         Raise ValidationError if any interface would form a LAG with a device outside this device's virtual chassis.
+        `members` overrides the set of fellow chassis members (used when the chassis has not been saved yet).
         """
         lag_members = Interface.objects.filter(device=self, lag__isnull=False).exclude(lag__device=self)
         lag_peers = Interface.objects.filter(lag__device=self).exclude(device=self)
-        if self.virtual_chassis_id:
+        if members is not None:
+            lag_members = lag_members.exclude(lag__device__in=members)
+            lag_peers = lag_peers.exclude(device__in=members)
+        elif self.virtual_chassis_id:
             lag_members = lag_members.exclude(lag__device__virtual_chassis=self.virtual_chassis_id)
             lag_peers = lag_peers.exclude(device__virtual_chassis=self.virtual_chassis_id)
         interfaces = [

netbox/dcim/tests/test_views.py

@@ -5681,6 +5681,55 @@ class VirtualChassisTestCase(ViewTestCases.PrimaryObjectViewTestCase):
         self.assertEqual(member.virtual_chassis.name, 'VC1')
         self.assertEqual(member.vc_position, 2)
 
+    @tag('regression')  # Ref: #23212
+    def test_add_member_blocked_by_cross_chassis_lag(self):
+        self.add_permissions('dcim.change_virtualchassis')
+        vc1_member = Device.objects.get(name='Device 1')
+        device = Device.objects.get(name='Device 10')
+        lag = Interface.objects.create(device=vc1_member, name='lag0', type=InterfaceTypeChoices.TYPE_LAG)
+        Interface.objects.create(device=device, name='eth0', type=InterfaceTypeChoices.TYPE_1GE_FIXED, lag=lag)
+
+        vc2 = VirtualChassis.objects.get(name='VC2')
+        url = reverse('dcim:virtualchassis_add_member', kwargs={'pk': vc2.pk})
+        response = self.client.post(url, {'device': device.pk, 'vc_position': 4, 'vc_priority': ''})
+        self.assertHttpStatus(response, 200)
+        self.assertIn('outside the virtual chassis', str(response.content))
+
+        device.refresh_from_db()
+        self.assertIsNone(device.virtual_chassis)
+        self.assertIsNone(device.vc_position)
+
+    @tag('regression')  # Ref: #23212
+    def test_create_with_members_blocked_by_cross_chassis_lag(self):
+        self.add_permissions('dcim.add_virtualchassis', 'dcim.view_device')
+        vc1_member = Device.objects.get(name='Device 1')
+        device = Device.objects.get(name='Device 10')
+        lag = Interface.objects.create(device=vc1_member, name='lag0', type=InterfaceTypeChoices.TYPE_LAG)
+        Interface.objects.create(device=device, name='eth0', type=InterfaceTypeChoices.TYPE_1GE_FIXED, lag=lag)
+
+        data = {'name': 'VC4', 'members': [device.pk], 'initial_position': 1}
+        response = self.client.post(reverse('dcim:virtualchassis_add'), data)
+        self.assertHttpStatus(response, 200)
+        self.assertIn('outside the virtual chassis', str(response.content))
+        self.assertFalse(VirtualChassis.objects.filter(name='VC4').exists())
+
+        device.refresh_from_db()
+        self.assertIsNone(device.virtual_chassis)
+
+    def test_create_with_members_allowed_with_lag_between_new_members(self):
+        self.add_permissions('dcim.add_virtualchassis', 'dcim.view_device')
+        device1 = Device.objects.get(name='Device 10')
+        device2 = Device.objects.get(name='Device 11')
+        lag = Interface.objects.create(device=device1, name='lag0', type=InterfaceTypeChoices.TYPE_LAG)
+        Interface.objects.create(device=device2, name='eth0', type=InterfaceTypeChoices.TYPE_1GE_FIXED, lag=lag)
+
+        data = {'name': 'VC4', 'members': [device1.pk, device2.pk], 'initial_position': 1}
+        response = self.client.post(reverse('dcim:virtualchassis_add'), data)
+        self.assertHttpStatus(response, 302)
+
+        vc = VirtualChassis.objects.get(name='VC4')
+        self.assertEqual(set(vc.members.values_list('name', flat=True)), {'Device 10', 'Device 11'})
+
     def test_remove_member(self):
         self.add_permissions('dcim.change_device')
         master = Device.objects.get(name='Device 1')

netbox/dcim/views.py

@@ -5303,6 +5303,13 @@ class VirtualChassisAddMemberView(ObjectPermissionRequiredMixin, GetReturnURLMix
             }
             membership_form = forms.DeviceVCMembershipForm(data=data, validate_vc_position=True, instance=device)
 
+            # DeviceVCMembershipForm has no virtual_chassis field, so this error cannot surface through is_valid()
+            try:
+                device.clean_vc_lag_membership()
+            except ValidationError as e:
+                membership_form = forms.DeviceVCMembershipForm(data=data)
+                membership_form.add_error(None, e.messages)
+
             if membership_form.is_valid():
                 membership_form.save()
                 messages.success(request, mark_safe(