From 25d334a55443585ca7773ae8d88a5bad8d9de7aa Mon Sep 17 00:00:00 2001 From: Arthur Date: Fri, 25 Apr 2025 11:08:53 -0700 Subject: [PATCH 1/3] 18706 Fix VLAN assignment checking --- netbox/ipam/models/vlans.py | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/netbox/ipam/models/vlans.py b/netbox/ipam/models/vlans.py index 91e39c6d3b6..8ebefe0838e 100644 --- a/netbox/ipam/models/vlans.py +++ b/netbox/ipam/models/vlans.py @@ -1,4 +1,5 @@ from django.contrib.contenttypes.fields import GenericForeignKey, GenericRelation +from django.contrib.contenttypes.models import ContentType from django.contrib.postgres.fields import ArrayField, IntegerRangeField from django.core.exceptions import ValidationError from django.core.validators import MaxValueValidator, MinValueValidator @@ -6,7 +7,7 @@ from django.db.backends.postgresql.psycopg_any import NumericRange from django.utils.translation import gettext_lazy as _ -from dcim.models import Interface +from dcim.models import Interface, Site, SiteGroup from ipam.choices import * from ipam.constants import * from ipam.querysets import VLANQuerySet, VLANGroupQuerySet @@ -279,12 +280,20 @@ def clean(self): super().clean() # Validate VLAN group (if assigned) - if self.group and self.site and self.group.scope != self.site: - raise ValidationError( - _( - "VLAN is assigned to group {group} (scope: {scope}); cannot also assign to site {site}." - ).format(group=self.group, scope=self.group.scope, site=self.site) - ) + if self.group and self.site and self.group.scope_type == ContentType.objects.get_for_model(Site): + if self.site != self.group.scope: + raise ValidationError( + _( + "VLAN is assigned to group {group} (scope: {scope}); cannot also assign to site {site}." + ).format(group=self.group, scope=self.group.scope, site=self.site) + ) + if self.group and self.site and self.group.scope_type == ContentType.objects.get_for_model(SiteGroup): + if self.site not in self.group.scope.sites.all(): + raise ValidationError( + _( + "VLAN is assigned to group {group} (scope: {scope}); cannot also assign to site {site}." + ).format(group=self.group, scope=self.group.scope, site=self.site) + ) # Check that the VLAN ID is permitted in the assigned group (if any) if self.group: From 90cf97ee29e29bdfb519620d58150a05bbec5440 Mon Sep 17 00:00:00 2001 From: Arthur Date: Fri, 25 Apr 2025 11:32:02 -0700 Subject: [PATCH 2/3] 18706 add tests --- netbox/ipam/tests/test_models.py | 53 ++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+) diff --git a/netbox/ipam/tests/test_models.py b/netbox/ipam/tests/test_models.py index 62eb741238b..7996636ad96 100644 --- a/netbox/ipam/tests/test_models.py +++ b/netbox/ipam/tests/test_models.py @@ -1,8 +1,10 @@ +from django.contrib.contenttypes.models import ContentType from django.core.exceptions import ValidationError from django.test import TestCase, override_settings from netaddr import IPNetwork, IPSet from utilities.data import string_to_ranges +from dcim.models import Site, SiteGroup from ipam.choices import * from ipam.models import * @@ -645,3 +647,54 @@ def test_qinq_role(self): ) with self.assertRaises(ValidationError): vlan.full_clean() + + def test_vlan_group_site_validation(self): + sitegroup = SiteGroup.objects.create( + name='Site Group 1', + slug='site-group-1', + ) + sites = Site.objects.bulk_create(( + Site( + name='Site 1', + slug='site-1', + ), + Site( + name='Site 2', + slug='site-2', + ), + )) + sitegroup.sites.add(sites[0]) + vlangroups = VLANGroup.objects.bulk_create(( + VLANGroup( + name='VLAN Group 1', + slug='vlan-group-1', + scope=sitegroup, + scope_type=ContentType.objects.get_for_model(SiteGroup), + ), + VLANGroup( + name='VLAN Group 2', + slug='vlan-group-2', + scope=sites[0], + scope_type=ContentType.objects.get_for_model(Site), + ), + VLANGroup( + name='VLAN Group 2', + slug='vlan-group-2', + scope=sites[1], + scope_type=ContentType.objects.get_for_model(Site), + ), + )) + vlan = VLAN( + name='VLAN 1', + vid=1, + group=vlangroups[0], + site=sites[0], + ) + + # VLAN Group 0 and 1 should be valid + vlan.full_clean() + vlan.group = vlangroups[1] + vlan.full_clean() + vlan.group = vlangroups[2] + with self.assertRaises(ValidationError): + vlan.full_clean() From cbadbd83ab37da47e630e77913ce250a9c5cd6b3 Mon Sep 17 00:00:00 2001 From: Arthur Date: Mon, 28 Apr 2025 07:49:46 -0700 Subject: [PATCH 3/3] 18706 review feedback --- netbox/ipam/models/vlans.py | 2 +- netbox/ipam/tests/test_models.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/netbox/ipam/models/vlans.py b/netbox/ipam/models/vlans.py index 8ebefe0838e..0288daf2e07 100644 --- a/netbox/ipam/models/vlans.py +++ b/netbox/ipam/models/vlans.py @@ -291,7 +291,7 @@ def clean(self): if self.site not in self.group.scope.sites.all(): raise ValidationError( _( - "VLAN is assigned to group {group} (scope: {scope}); cannot also assign to site {site}." + "The assigned site {site} is not a member of the assigned group {group} (scope: {scope})." ).format(group=self.group, scope=self.group.scope, site=self.site) ) diff --git a/netbox/ipam/tests/test_models.py b/netbox/ipam/tests/test_models.py index 7996636ad96..e8cd11a871b 100644 --- a/netbox/ipam/tests/test_models.py +++ b/netbox/ipam/tests/test_models.py @@ -691,7 +691,7 @@ def test_vlan_group_site_validation(self): site=sites[0], ) - # VLAN Group 0 and 1 should be valid + # VLAN Group 1 and 2 should be valid vlan.full_clean() vlan.group = vlangroups[1] vlan.full_clean()