diff --git a/netbox/ipam/graphql/types.py b/netbox/ipam/graphql/types.py index 31b93b419..f17c9c72e 100644 --- a/netbox/ipam/graphql/types.py +++ b/netbox/ipam/graphql/types.py @@ -244,7 +244,7 @@ class RouteTargetType(PrimaryObjectType): @strawberry_django.type( models.Service, - exclude=('parent_object_type', 'parent_object_id'), + exclude=('_ports_lowest', 'parent_object_type', 'parent_object_id'), filters=ServiceFilter, pagination=True ) @@ -264,7 +264,7 @@ class ServiceType(ContactsMixin, PrimaryObjectType): @strawberry_django.type( models.ServiceTemplate, - fields='__all__', + exclude=('_ports_lowest',), filters=ServiceTemplateFilter, pagination=True ) diff --git a/netbox/ipam/migrations/0089_default_ordering_indexes.py b/netbox/ipam/migrations/0089_default_ordering_indexes.py index a718aa3fc..64ec1b020 100644 --- a/netbox/ipam/migrations/0089_default_ordering_indexes.py +++ b/netbox/ipam/migrations/0089_default_ordering_indexes.py @@ -32,9 +32,12 @@ class Migration(migrations.Migration): model_name='role', index=models.Index(fields=['weight', 'name'], name='ipam_role_weight_01396b_idx'), ), + # Adding a dummy index, to allow a safe migration in case updating users already have services + # with a large number of ports configured (see issue #22273) + # Will get removed in 0091_alter_service_index_and_ordering migrations.AddIndex( model_name='service', - index=models.Index(fields=['protocol', 'ports', 'id'], name='ipam_servic_protoco_687d13_idx'), + index=models.Index(fields=['id'], name='ipam_servic_protoco_687d13_idx'), ), migrations.AddIndex( model_name='vlangroup', diff --git a/netbox/ipam/migrations/0091_alter_service_index_and_ordering.py b/netbox/ipam/migrations/0091_alter_service_index_and_ordering.py new file mode 100644 index 000000000..aa91e863c --- /dev/null +++ b/netbox/ipam/migrations/0091_alter_service_index_and_ordering.py @@ -0,0 +1,58 @@ +from django.db import migrations, models + + +def populate__ports_lowest(apps, schema_editor): + Service = apps.get_model('ipam', 'Service') + ServiceTemplate = apps.get_model('ipam', 'ServiceTemplate') + CHUNK_SIZE = 500 + + for model in (Service, ServiceTemplate): + chunk = [] + qs = model.objects.filter(_ports_lowest__isnull=True).only('id', 'ports', '_ports_lowest') + for obj in qs.iterator(chunk_size=CHUNK_SIZE): + if obj.ports: + obj._ports_lowest = min(obj.ports) + chunk.append(obj) + if len(chunk) >= CHUNK_SIZE: + model.objects.bulk_update(chunk, ['_ports_lowest']) + chunk = [] + if chunk: + model.objects.bulk_update(chunk, ['_ports_lowest']) + + +class Migration(migrations.Migration): + + dependencies = [ + ('ipam', '0090_vlangroup_recompute_total_vlan_ids'), + ] + + operations = [ + migrations.RemoveIndex( + model_name='service', + name='ipam_servic_protoco_687d13_idx', + ), + migrations.AddField( + model_name='service', + name='_ports_lowest', + field=models.PositiveIntegerField(blank=True, null=True), + ), + migrations.AddField( + model_name='servicetemplate', + name='_ports_lowest', + field=models.PositiveIntegerField(blank=True, null=True), + ), + migrations.RunPython(populate__ports_lowest, migrations.RunPython.noop), + migrations.AddIndex( + model_name='service', + index=models.Index( + fields=['protocol', '_ports_lowest', 'id'], + name='ipam_servic_protoco_e2901d_idx' + ), + ), + migrations.AlterModelOptions( + name='service', + options={ + 'ordering': ('protocol', '_ports_lowest', 'id') + }, + ), + ] diff --git a/netbox/ipam/models/services.py b/netbox/ipam/models/services.py index d934465c3..ee12e3b04 100644 --- a/netbox/ipam/models/services.py +++ b/netbox/ipam/models/services.py @@ -31,10 +31,22 @@ class ServiceBase(models.Model): ), verbose_name=_('port numbers') ) + _ports_lowest = models.PositiveIntegerField( + null=True, + blank=True, + ) class Meta: abstract = True + def save(self, *args, **kwargs): + # On saving find the smallest port and save for default ordering + self._ports_lowest = min(self.ports) if self.ports else None + update_fields = kwargs.get('update_fields') + if update_fields is not None and '_ports_lowest' not in update_fields: + kwargs['update_fields'] = list(update_fields) + ['_ports_lowest'] + super().save(*args, **kwargs) + def __str__(self): return f'{self.name} ({self.get_protocol_display()}/{self.port_list})' @@ -74,7 +86,6 @@ class Service(ContactsMixin, ServiceBase, PrimaryModel): ct_field='parent_object_type', fk_field='parent_object_id' ) - name = models.CharField( max_length=100, verbose_name=_('name') @@ -93,9 +104,9 @@ class Service(ContactsMixin, ServiceBase, PrimaryModel): class Meta: indexes = ( - models.Index(fields=('protocol', 'ports', 'id')), # Default ordering + models.Index(fields=('protocol', '_ports_lowest', 'id')), # Default ordering models.Index(fields=('parent_object_type', 'parent_object_id')), ) - ordering = ('protocol', 'ports', 'pk') # (protocol, port) may be non-unique + ordering = ('protocol', '_ports_lowest', 'id') verbose_name = _('application service') verbose_name_plural = _('application services') diff --git a/netbox/ipam/tests/test_models.py b/netbox/ipam/tests/test_models.py index e5637433a..becbf9413 100644 --- a/netbox/ipam/tests/test_models.py +++ b/netbox/ipam/tests/test_models.py @@ -6,8 +6,10 @@ from netaddr import IPNetwork, IPSet from dcim.models import Site, SiteGroup from ipam.choices import * +from ipam.constants import SERVICE_PORT_MAX, SERVICE_PORT_MIN from ipam.models import * from utilities.data import string_to_ranges +from virtualization.models import VirtualMachine class AggregateTestCase(TestCase): @@ -926,3 +928,73 @@ class VLANTestCase(TestCase): vlan.group = vlangroups[2] with self.assertRaises(ValidationError): vlan.full_clean() + + +class ServiceTemplateTestCase(TestCase): + + def test_servicetemplate_lowest_port(self): + """ + Test lowest port setting for servicetemplate + """ + template = ServiceTemplate( + name='Template 1', + protocol=ServiceProtocolChoices.PROTOCOL_TCP, + ports=[80, 443, 22, 8080], # small test list + ) + template.full_clean() + template.save() + self.assertEqual(template._ports_lowest, 22) + + def test_servicetemplate_single_port(self): + """ + Test with a single port + """ + template = ServiceTemplate( + name='Template 2', + protocol=ServiceProtocolChoices.PROTOCOL_UDP, + ports=[53], + ) + template.full_clean() + template.save() + self.assertEqual(template._ports_lowest, 53) + + def test_servicetemplate_empty_ports(self): + """ + Test with empty ports list + """ + template = ServiceTemplate( + name='Template 3', + protocol=ServiceProtocolChoices.PROTOCOL_TCP, + ports=[], + ) + self.assertRaises(ValidationError, template.full_clean) + + +class ServiceTestCase(TestCase): + + @classmethod + def setUpTestData(cls): + site = Site.objects.create( + name='Site 1', + slug='site-1', + ) + VirtualMachine.objects.create( + name='virtual machine 1', + site=site, + ) + + def test_large_service(self): + """ + Test creation of service with large number of ports. + Related to issue #22273 + """ + service = Service( + name='Service 1', + protocol=ServiceProtocolChoices.PROTOCOL_TCP, + ports=list(range(SERVICE_PORT_MIN, SERVICE_PORT_MAX)), + parent=VirtualMachine.objects.first(), + ) + service.full_clean() + # Testing .save() is the important part, to check for database problems + service.save() + self.assertEqual(service._ports_lowest, SERVICE_PORT_MIN)