fix(ipam): Correct VLAN ID range bound handling in VLANGroup
Fix miscounting of total_vlan_ids when VLANGroup vid_ranges use non-canonical bounds (e.g. '[]'). Normalize ranges to '[)' on save and add migration to recompute existing totals. Prevent division-by-zero in utilization queries for legacy rows with miscounted totals. Fixes #22228
This commit is contained in:
Martin Hauser
parent
64d3b114bc
commit
573b1b0634
|
|
@ -18,6 +18,16 @@ A unique URL-friendly identifier. (This value can be used for filtering.)
|
|||
|
||||
The set of VLAN IDs which are encompassed by the group. By default, this will be the entire range of valid IEEE 802.1Q VLAN IDs (1 to 4094, inclusive). VLANs created within a group must have a VID that falls within one of these ranges. Ranges may not overlap.
|
||||
|
||||
Internally, each range is stored in PostgreSQL as a canonical half-open `[start, end)` interval. The `start` value is the first VID included in the range; the `end` value sits one above the last VID included.
|
||||
|
||||
The REST API and UI both present ranges using inclusive bounds, so most users never see the half-open form. Users working with the stored value directly, such as through psql, raw SQL, Django ORM access in Custom Scripts or plugins, or third-party tools, should expect this canonical representation.
|
||||
|
||||
For the range covering VLANs 100 through 200:
|
||||
|
||||
* UI input: `100-200`
|
||||
* REST API range item: `[100, 200]`
|
||||
* Database row: `[100, 201)`
|
||||
|
||||
### Total VLAN IDs
|
||||
|
||||
A read-only integer indicating the total count of VLAN IDs available within the group, calculated from the configured VLAN ID Ranges. For example, a group with ranges `100-199` and `300-399` would have a total of 200 VLAN IDs. This value is automatically computed and updated whenever the VLAN ID ranges are modified.
|
||||
|
|
|
|||
|
|
@ -0,0 +1,32 @@
|
|||
from django.db import migrations
|
||||
|
||||
|
||||
def recompute_vlangroup_total_vlan_ids(apps, schema_editor):
|
||||
# PostgreSQL canonicalizes int4range values to '[)' on insert, so vid_ranges
|
||||
# at rest is already canonical. Only total_vlan_ids needs fixing: the
|
||||
# pre-fix VLANGroup.save() miscounted it for ranges supplied with
|
||||
# non-canonical bounds (e.g. NumericRange(lo, hi, bounds='[]')).
|
||||
VLANGroup = apps.get_model('ipam', 'VLANGroup')
|
||||
db_alias = schema_editor.connection.alias
|
||||
|
||||
stale_groups = []
|
||||
for group in VLANGroup.objects.using(db_alias).only('id', 'vid_ranges', 'total_vlan_ids').iterator(chunk_size=500):
|
||||
total_vlan_ids = sum(
|
||||
r.upper - r.lower for r in (group.vid_ranges or []) if r.lower is not None and r.upper is not None
|
||||
)
|
||||
if group.total_vlan_ids != total_vlan_ids:
|
||||
group.total_vlan_ids = total_vlan_ids
|
||||
stale_groups.append(group)
|
||||
|
||||
if stale_groups:
|
||||
VLANGroup.objects.using(db_alias).bulk_update(stale_groups, ['total_vlan_ids'], batch_size=100)
|
||||
|
||||
|
||||
class Migration(migrations.Migration):
|
||||
dependencies = [
|
||||
('ipam', '0089_default_ordering_indexes'),
|
||||
]
|
||||
|
||||
operations = [
|
||||
migrations.RunPython(recompute_vlangroup_total_vlan_ids, migrations.RunPython.noop),
|
||||
]
|
||||
|
|
@ -12,7 +12,13 @@ from ipam.choices import *
|
|||
from ipam.constants import *
|
||||
from ipam.querysets import VLANGroupQuerySet, VLANQuerySet
|
||||
from netbox.models import NetBoxModel, OrganizationalModel, PrimaryModel
|
||||
from utilities.data import check_ranges_overlap, ranges_to_string, ranges_to_string_list
|
||||
from utilities.data import (
|
||||
check_ranges_overlap,
|
||||
get_inclusive_integer_range_bounds,
|
||||
normalize_integer_range,
|
||||
ranges_to_string,
|
||||
ranges_to_string_list,
|
||||
)
|
||||
from virtualization.models import VMInterface
|
||||
|
||||
__all__ = (
|
||||
|
|
@ -55,6 +61,7 @@ class VLANGroup(OrganizationalModel):
|
|||
ct_field='scope_type',
|
||||
fk_field='scope_id'
|
||||
)
|
||||
# Normalized to canonical '[)' bounds on save() to match PostgreSQL storage.
|
||||
vid_ranges = ArrayField(
|
||||
IntegerRangeField(),
|
||||
verbose_name=_('VLAN ID ranges'),
|
||||
|
|
@ -103,8 +110,7 @@ class VLANGroup(OrganizationalModel):
|
|||
|
||||
# Validate VID ranges
|
||||
for vid_range in self.vid_ranges:
|
||||
lower_vid = vid_range.lower if vid_range.lower_inc else vid_range.lower + 1
|
||||
upper_vid = vid_range.upper if vid_range.upper_inc else vid_range.upper - 1
|
||||
lower_vid, upper_vid = get_inclusive_integer_range_bounds(vid_range)
|
||||
if lower_vid < VLAN_VID_MIN:
|
||||
raise ValidationError({
|
||||
'vid_ranges': _("Starting VLAN ID in range ({value}) cannot be less than {minimum}").format(
|
||||
|
|
@ -129,10 +135,20 @@ class VLANGroup(OrganizationalModel):
|
|||
raise ValidationError({'vid_ranges': _("Ranges cannot overlap.")})
|
||||
|
||||
def save(self, *args, **kwargs):
|
||||
# Canonicalize vid_ranges on the instance so callers (e.g. Custom Scripts)
|
||||
# see the same value in memory that PostgreSQL stores.
|
||||
self.total_vlan_ids = 0
|
||||
vid_ranges = []
|
||||
for vid_range in self.vid_ranges:
|
||||
# VID range is inclusive on lower-bound, exclusive on upper-bound
|
||||
vid_range = normalize_integer_range(vid_range)
|
||||
vid_ranges.append(vid_range)
|
||||
self.total_vlan_ids += vid_range.upper - vid_range.lower
|
||||
self.vid_ranges = vid_ranges
|
||||
|
||||
update_fields = kwargs.get('update_fields')
|
||||
if update_fields is not None and 'vid_ranges' in update_fields:
|
||||
# total_vlan_ids is a denormalized cache of vid_ranges; persist them together.
|
||||
kwargs['update_fields'] = list(set(update_fields) | {'total_vlan_ids'})
|
||||
|
||||
super().save(*args, **kwargs)
|
||||
|
||||
|
|
@ -142,9 +158,8 @@ class VLANGroup(OrganizationalModel):
|
|||
"""
|
||||
available_vlans = set()
|
||||
for vlan_range in self.vid_ranges:
|
||||
available_vlans = available_vlans.union({
|
||||
vid for vid in range(vlan_range.lower, vlan_range.upper)
|
||||
})
|
||||
lower_vid, upper_vid = get_inclusive_integer_range_bounds(vlan_range)
|
||||
available_vlans.update(range(lower_vid, upper_vid + 1))
|
||||
available_vlans -= set(VLAN.objects.filter(group=self).values_list('vid', flat=True))
|
||||
|
||||
return sorted(available_vlans)
|
||||
|
|
|
|||
|
|
@ -1,7 +1,7 @@
|
|||
from django.contrib.contenttypes.models import ContentType
|
||||
from django.db.models import Count, F, OuterRef, Q, Subquery, Value
|
||||
from django.db.models.expressions import RawSQL
|
||||
from django.db.models.functions import Round
|
||||
from django.db.models.functions import NullIf, Round
|
||||
|
||||
from utilities.query import count_related
|
||||
from utilities.querysets import RestrictedQuerySet
|
||||
|
|
@ -62,9 +62,11 @@ class VLANGroupQuerySet(RestrictedQuerySet):
|
|||
def annotate_utilization(self):
|
||||
from .models import VLAN
|
||||
|
||||
# NullIf guards against legacy rows where total_vlan_ids was miscounted to
|
||||
# 0 by the pre-fix VLANGroup.save(); without it, the annotation 500s.
|
||||
return self.annotate(
|
||||
vlan_count=count_related(VLAN, 'group'),
|
||||
utilization=Round(F('vlan_count') * 100.0 / F('total_vlan_ids'), 2),
|
||||
utilization=Round(F('vlan_count') * 100.0 / NullIf(F('total_vlan_ids'), Value(0)), 2),
|
||||
)
|
||||
|
||||
|
||||
|
|
|
|||
|
|
@ -1,5 +1,6 @@
|
|||
from django.contrib.contenttypes.models import ContentType
|
||||
from django.core.exceptions import ValidationError
|
||||
from django.db.backends.postgresql.psycopg_any import NumericRange
|
||||
from django.test import TestCase, override_settings
|
||||
from netaddr import IPNetwork, IPSet
|
||||
|
||||
|
|
@ -741,6 +742,18 @@ class VLANGroupTestCase(TestCase):
|
|||
available_vids = vlangroup.get_available_vids()
|
||||
self.assertListEqual(available_vids, list(range(104, 200)))
|
||||
|
||||
def test_get_available_vids_with_inclusive_ranges(self):
|
||||
vlangroup = VLANGroup.objects.create(
|
||||
name='VLAN Group 2',
|
||||
slug='vlan-group-2',
|
||||
vid_ranges=[NumericRange(200, 204, bounds='[]')],
|
||||
)
|
||||
VLAN.objects.create(name='VLAN 200', vid=200, group=vlangroup)
|
||||
|
||||
# Re-assign with non-canonical bounds to exercise the unsaved in-memory path.
|
||||
vlangroup.vid_ranges = [NumericRange(200, 204, bounds='[]')]
|
||||
self.assertListEqual(vlangroup.get_available_vids(), [201, 202, 203, 204])
|
||||
|
||||
def test_get_next_available_vid(self):
|
||||
vlangroup = VLANGroup.objects.first()
|
||||
self.assertEqual(vlangroup.get_next_available_vid(), 104)
|
||||
|
|
@ -776,6 +789,72 @@ class VLANGroupTestCase(TestCase):
|
|||
vlangroup = VLANGroup.objects.first()
|
||||
self.assertEqual(vlangroup.total_vlan_ids, 100)
|
||||
|
||||
def test_total_vlan_ids_with_inclusive_ranges(self):
|
||||
test_cases = (
|
||||
(
|
||||
NumericRange(100, 199, bounds='[]'),
|
||||
100,
|
||||
NumericRange(100, 200, bounds='[)'),
|
||||
),
|
||||
(
|
||||
NumericRange(100, 100, bounds='[]'),
|
||||
1,
|
||||
NumericRange(100, 101, bounds='[)'),
|
||||
),
|
||||
(
|
||||
NumericRange(99, 199, bounds='(]'),
|
||||
100,
|
||||
NumericRange(100, 200, bounds='[)'),
|
||||
),
|
||||
)
|
||||
|
||||
for i, (vid_range, total_vlan_ids, normalized_range) in enumerate(test_cases, start=1):
|
||||
vlangroup = VLANGroup(
|
||||
name=f'VLAN Group Inclusive {i}',
|
||||
slug=f'vlan-group-inclusive-{i}',
|
||||
vid_ranges=[vid_range],
|
||||
)
|
||||
|
||||
vlangroup.full_clean()
|
||||
vlangroup.save()
|
||||
self.assertEqual(vlangroup.vid_ranges, [normalized_range])
|
||||
self.assertEqual(vlangroup.total_vlan_ids, total_vlan_ids)
|
||||
|
||||
def test_total_vlan_ids_with_inclusive_ranges_without_full_clean(self):
|
||||
vlangroup = VLANGroup.objects.create(
|
||||
name='VLAN Group Inclusive Save',
|
||||
slug='vlan-group-inclusive-save',
|
||||
vid_ranges=[NumericRange(100, 100, bounds='[]')],
|
||||
)
|
||||
|
||||
self.assertEqual(vlangroup.vid_ranges, [NumericRange(100, 101, bounds='[)')])
|
||||
self.assertEqual(vlangroup.total_vlan_ids, 1)
|
||||
|
||||
def test_total_vlan_ids_with_update_fields(self):
|
||||
vlangroup = VLANGroup.objects.create(
|
||||
name='VLAN Group Update Fields',
|
||||
slug='vlan-group-update-fields',
|
||||
vid_ranges=[NumericRange(100, 200, bounds='[)')],
|
||||
)
|
||||
|
||||
vlangroup.vid_ranges = [NumericRange(100, 100, bounds='[]')]
|
||||
vlangroup.save(update_fields=['vid_ranges'])
|
||||
vlangroup.refresh_from_db()
|
||||
|
||||
self.assertEqual(vlangroup.vid_ranges, [NumericRange(100, 101, bounds='[)')])
|
||||
self.assertEqual(vlangroup.total_vlan_ids, 1)
|
||||
|
||||
def test_annotate_utilization_with_zero_total_vlan_ids(self):
|
||||
vlangroup = VLANGroup.objects.create(
|
||||
name='VLAN Group Zero Total',
|
||||
slug='vlan-group-zero-total',
|
||||
vid_ranges=[NumericRange(100, 101)],
|
||||
)
|
||||
VLANGroup.objects.filter(pk=vlangroup.pk).update(total_vlan_ids=0)
|
||||
|
||||
vlangroup = VLANGroup.objects.annotate_utilization().get(pk=vlangroup.pk)
|
||||
self.assertIsNone(vlangroup.utilization)
|
||||
|
||||
|
||||
class VLANTestCase(TestCase):
|
||||
|
||||
|
|
|
|||
|
|
@ -9,6 +9,8 @@ from rest_framework import serializers
|
|||
from rest_framework.exceptions import ValidationError
|
||||
from rest_framework.relations import PrimaryKeyRelatedField, RelatedField
|
||||
|
||||
from utilities.data import get_inclusive_integer_range_bounds
|
||||
|
||||
__all__ = (
|
||||
'AttributesField',
|
||||
'ChoiceField',
|
||||
|
|
@ -173,7 +175,7 @@ class IntegerRangeSerializer(serializers.Serializer):
|
|||
return NumericRange(data[0], data[1] + 1, bounds='[)')
|
||||
|
||||
def to_representation(self, instance):
|
||||
return instance.lower, instance.upper - 1
|
||||
return get_inclusive_integer_range_bounds(instance)
|
||||
|
||||
|
||||
class AttributesField(serializers.JSONField):
|
||||
|
|
|
|||
|
|
@ -1,11 +1,13 @@
|
|||
import uuid
|
||||
|
||||
from django.db.backends.postgresql.psycopg_any import NumericRange
|
||||
from django.test import RequestFactory, TestCase
|
||||
from django.urls import reverse
|
||||
from rest_framework.exceptions import ValidationError
|
||||
from rest_framework.request import Request
|
||||
|
||||
from netbox.api.exceptions import QuerySetNotOrdered
|
||||
from netbox.api.fields import IntegerRangeSerializer
|
||||
from netbox.api.pagination import NetBoxPagination
|
||||
from users.models import Token
|
||||
from utilities.testing import APITestCase
|
||||
|
|
@ -111,3 +113,44 @@ class OptionalLimitOffsetPaginationTestCase(TestCase):
|
|||
request = self._make_drf_request(query_params={'start': '1', 'ordering': 'created'})
|
||||
with self.assertRaises(ValidationError):
|
||||
self.paginator.paginate_queryset(queryset, request)
|
||||
|
||||
|
||||
class IntegerRangeSerializerTestCase(TestCase):
|
||||
|
||||
def test_to_representation_emits_inclusive_bounds_for_non_canonical_range(self):
|
||||
"""A NumericRange with bounds='[]' must serialize to its inclusive (lower, upper) pair."""
|
||||
serializer = IntegerRangeSerializer()
|
||||
self.assertEqual(
|
||||
serializer.to_representation(NumericRange(100, 199, bounds='[]')),
|
||||
(100, 199)
|
||||
)
|
||||
self.assertEqual(
|
||||
serializer.to_representation(NumericRange(100, 200, bounds='[)')),
|
||||
(100, 199)
|
||||
)
|
||||
|
||||
def test_to_internal_value_produces_canonical_half_open_range(self):
|
||||
"""An inclusive [lo, hi] pair is normalized to NumericRange(lo, hi+1, '[)')."""
|
||||
serializer = IntegerRangeSerializer()
|
||||
self.assertEqual(
|
||||
serializer.to_internal_value([100, 199]),
|
||||
NumericRange(100, 200, bounds='[)')
|
||||
)
|
||||
|
||||
def test_to_internal_value_rejects_malformed_input(self):
|
||||
"""Input must be a two-element list or tuple of ints."""
|
||||
serializer = IntegerRangeSerializer()
|
||||
with self.assertRaises(ValidationError):
|
||||
serializer.to_internal_value('100-200')
|
||||
with self.assertRaises(ValidationError):
|
||||
serializer.to_internal_value([100])
|
||||
with self.assertRaises(ValidationError):
|
||||
serializer.to_internal_value([100, 200, 300])
|
||||
|
||||
def test_to_internal_value_rejects_non_integer_bounds(self):
|
||||
"""Range boundaries must be integers, not strings or floats."""
|
||||
serializer = IntegerRangeSerializer()
|
||||
with self.assertRaises(ValidationError):
|
||||
serializer.to_internal_value(['100', '200'])
|
||||
with self.assertRaises(ValidationError):
|
||||
serializer.to_internal_value([100.5, 200.5])
|
||||
|
|
|
|||
|
|
@ -12,6 +12,8 @@ __all__ = (
|
|||
'drange',
|
||||
'flatten_dict',
|
||||
'get_config_value_ci',
|
||||
'get_inclusive_integer_range_bounds',
|
||||
'normalize_integer_range',
|
||||
'ranges_to_string',
|
||||
'ranges_to_string_list',
|
||||
'resolve_attr_path',
|
||||
|
|
@ -169,16 +171,43 @@ def drange(start, end, step=decimal.Decimal(1)):
|
|||
start += step
|
||||
|
||||
|
||||
def get_inclusive_integer_range_bounds(value_range):
|
||||
"""
|
||||
Return the lower and upper bounds of a bounded, non-empty discrete
|
||||
integer range as inclusive values.
|
||||
|
||||
For example, ``[10, 20)`` is returned as ``(10, 19)``, while
|
||||
``[10, 20]`` is returned as ``(10, 20)``.
|
||||
|
||||
Both bounds must be non-``None``; unbounded ranges are not supported.
|
||||
"""
|
||||
lower = value_range.lower if value_range.lower_inc else value_range.lower + 1
|
||||
upper = value_range.upper if value_range.upper_inc else value_range.upper - 1
|
||||
|
||||
return lower, upper
|
||||
|
||||
|
||||
def normalize_integer_range(value_range):
|
||||
"""
|
||||
Return an equivalent canonical half-open ``[)`` range for a bounded,
|
||||
non-empty discrete integer range, regardless of the input range's
|
||||
bounds metadata.
|
||||
"""
|
||||
lower, upper = get_inclusive_integer_range_bounds(value_range)
|
||||
|
||||
return NumericRange(lower, upper + 1, bounds='[)')
|
||||
|
||||
|
||||
def check_ranges_overlap(ranges):
|
||||
"""
|
||||
Check for overlap in an iterable of NumericRanges.
|
||||
Check for overlap in an iterable of NumericRanges. Does not mutate the input.
|
||||
"""
|
||||
ranges.sort(key=lambda x: x.lower)
|
||||
ranges = sorted(ranges, key=lambda value_range: get_inclusive_integer_range_bounds(value_range)[0])
|
||||
|
||||
for i in range(1, len(ranges)):
|
||||
prev_range = ranges[i - 1]
|
||||
prev_upper = prev_range.upper if prev_range.upper_inc else prev_range.upper - 1
|
||||
lower = ranges[i].lower if ranges[i].lower_inc else ranges[i].lower + 1
|
||||
prev_upper = get_inclusive_integer_range_bounds(ranges[i - 1])[1]
|
||||
lower = get_inclusive_integer_range_bounds(ranges[i])[0]
|
||||
|
||||
if prev_upper >= lower:
|
||||
return True
|
||||
|
||||
|
|
@ -201,9 +230,7 @@ def ranges_to_string_list(ranges):
|
|||
|
||||
output: list[str] = []
|
||||
for r in ranges:
|
||||
# Compute inclusive bounds regardless of how the DB range is stored.
|
||||
lower = r.lower if r.lower_inc else r.lower + 1
|
||||
upper = r.upper if r.upper_inc else r.upper - 1
|
||||
lower, upper = get_inclusive_integer_range_bounds(r)
|
||||
output.append(f"{lower}-{upper}" if lower != upper else str(lower))
|
||||
return output
|
||||
|
||||
|
|
|
|||
|
|
@ -5,6 +5,8 @@ from utilities.data import (
|
|||
check_ranges_overlap,
|
||||
deep_compare_dict,
|
||||
get_config_value_ci,
|
||||
get_inclusive_integer_range_bounds,
|
||||
normalize_integer_range,
|
||||
ranges_to_string,
|
||||
ranges_to_string_list,
|
||||
string_to_ranges,
|
||||
|
|
@ -54,6 +56,54 @@ class RangeFunctionsTestCase(TestCase):
|
|||
])
|
||||
)
|
||||
|
||||
def test_check_ranges_overlap_does_not_mutate_input(self):
|
||||
"""Verify check_ranges_overlap does not reorder the caller's list."""
|
||||
ranges = [
|
||||
NumericRange(10, 20, bounds='[)'),
|
||||
NumericRange(1, 5, bounds='[)'),
|
||||
]
|
||||
expected = [
|
||||
NumericRange(10, 20, bounds='[)'),
|
||||
NumericRange(1, 5, bounds='[)'),
|
||||
]
|
||||
check_ranges_overlap(ranges)
|
||||
self.assertEqual(ranges, expected)
|
||||
|
||||
def test_check_ranges_overlap_with_non_canonical_bounds(self):
|
||||
# Both ranges share raw .lower=1 but different inclusive lowers (2 vs. 1).
|
||||
# The pre-fix sort key (raw .lower) would order them as written and report
|
||||
# a false overlap; sorting by inclusive lower catches this.
|
||||
self.assertFalse(check_ranges_overlap([
|
||||
NumericRange(1, 10, bounds='(]'), # 2-10
|
||||
NumericRange(1, 1, bounds='[]'), # 1
|
||||
]))
|
||||
|
||||
self.assertTrue(check_ranges_overlap([
|
||||
NumericRange(1, 10, bounds='(]'), # 2-10
|
||||
NumericRange(2, 2, bounds='[]'), # 2
|
||||
]))
|
||||
|
||||
def test_get_inclusive_integer_range_bounds(self):
|
||||
self.assertEqual(get_inclusive_integer_range_bounds(NumericRange(10, 20, bounds='[)')), (10, 19))
|
||||
self.assertEqual(get_inclusive_integer_range_bounds(NumericRange(10, 20, bounds='[]')), (10, 20))
|
||||
self.assertEqual(get_inclusive_integer_range_bounds(NumericRange(10, 20, bounds='(]')), (11, 20))
|
||||
self.assertEqual(get_inclusive_integer_range_bounds(NumericRange(10, 20, bounds='()')), (11, 19))
|
||||
self.assertEqual(get_inclusive_integer_range_bounds(NumericRange(10, 10, bounds='[]')), (10, 10))
|
||||
|
||||
def test_normalize_integer_range(self):
|
||||
self.assertEqual(
|
||||
normalize_integer_range(NumericRange(10, 20, bounds='[)')),
|
||||
NumericRange(10, 20, bounds='[)')
|
||||
)
|
||||
self.assertEqual(
|
||||
normalize_integer_range(NumericRange(10, 20, bounds='[]')),
|
||||
NumericRange(10, 21, bounds='[)')
|
||||
)
|
||||
self.assertEqual(
|
||||
normalize_integer_range(NumericRange(10, 10, bounds='[]')),
|
||||
NumericRange(10, 11, bounds='[)')
|
||||
)
|
||||
|
||||
def test_ranges_to_string_list(self):
|
||||
self.assertEqual(
|
||||
ranges_to_string_list([
|
||||
|
|
|
|||
Loading…
Reference in New Issue