Skip to content

Closes #18040: Add a MACPrefix model to track blocks of MAC addresses - #23674

Open
bctiemann wants to merge 1 commit into
featurefrom
18040-mac-prefix
Open

bctiemann wants to merge 1 commit into
featurefrom
18040-mac-prefix

Conversation

@bctiemann

@bctiemann bctiemann commented Nov 11, 2026 •

Copy link
Copy Markdown
Contributor

Closes: #18040

Adds a MACPrefix model to track blocks of MAC addresses, such as OUIs, IEEE MA-M/MA-S blocks, or blocks within an OUI.

  • A MAC prefix is a base address plus a prefix length (1–48, default 24). Host bits are cleared on save, and the last address in the block is cached in _last_address so range lookups are indexed. (prefix, prefix_length) is unique.
  • MAC addresses aren't linked to prefixes by a foreign key. A MAC address belongs to every prefix whose range contains it, like IP addresses and IPAM prefixes. MACAddress.mac_prefix returns the most specific one, which the MAC address view shows. MAC addresses can be filtered by mac_prefix_id.
  • Utilization is the share of the prefix's addresses that exist in NetBox, counting duplicates once. The list views and API annotate the count, so it isn't a query per row.
  • The prefix view lists its MAC addresses, with a button to add the next available one.
  • /api/dcim/mac-prefixes/<id>/available-mac-addresses/ lists available addresses (GET) and creates MAC addresses from them (POST), reusing IPAM's AvailableObjectsView with a new advisory lock. A GET without a limit returns one page, since a /24 has 16.7M addresses.
  • enforce_unique rejects a MAC address that duplicates another within the prefix (this also covers mac address - allow uniqueness enforcement per oui #21370). It can't be enabled on a prefix that already contains duplicates.
  • Also added: filtersets (including a contains filter), forms, bulk import/edit/delete, table, GraphQL type and filter, search index, navigation, and docs.

Database changes: one new table, dcim_macprefix. No existing tables are changed.

MAC prefixes can't be created with bulk_create(), since save() sets the cached _last_address.

The MAC prefix list: an OUI, two /28 blocks within it (one enforcing uniqueness), and an MA-S block. The MAC address counts link to the filtered MAC address list.

1-mac-prefix-list

An OUI's view, showing its range, size and utilization, and the MAC addresses within it.

2-mac-prefix-detail

"Add Next Available" opens the MAC address form with the first available address in the prefix (here, filling the gap at BC:24:11:00:00:03).

3-add-next-available-form

After saving, the new MAC address appears in the prefix.

4-next-available-created

A MAC address's view shows the most specific prefix that contains it (the /28, not the OUI).

5-mac-address-shows-prefix

A duplicate MAC address is rejected within a prefix that enforces uniqueness.

6-enforce-unique-error

The form for adding a MAC prefix.

7-add-mac-prefix-form

The REST API lists the available MAC addresses within a prefix.

8-api-available-mac-addresses

@bctiemann

Copy link
Copy Markdown
Contributor Author

@claude /review and report only on actionable issues. Be sure to check for all required items for an editable core model throughout the UI and API, including the bespoke "available MACs within prefix" endpoint.

@github-actions

github-actions Bot commented Nov 11, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @bctiemann's task in 12m 19s —— View job


Code Review

  • Read the full diff (origin/feature...HEAD)
  • Review model, migration, querysets
  • Review API (serializers, viewset, available-mac-addresses)
  • Review UI (forms, tables, views, panels, nav)
  • Review filtersets / GraphQL / search / docs
  • Check completeness checklist for an editable core model
  • Post findings

Overall this is a thorough, well-covered addition — the full surface area for an editable core model is present (model + migration, filterset w/ @register_filterset, model/bulk-edit/bulk-import/filter forms, table, serializer + viewset + router, all eight UI views, URLs, GraphQL type + filter + schema entry, search index, navigation, docs + mkdocs nav, and tests across api/filtersets/models/tables/views). Changelog and journal views come free via PrimaryModel, and _last_address is correctly excluded from the GraphQL type and the filterset test. I verified the available-mac-addresses endpoint against AvailableObjectsView's contract (limit handling on GET vs. POST, check_sufficient_available running before prep_object_data, the new advisory lock key) and found no correctness problem there — the batching in _iter_occupied terminates correctly for empty batches, short batches, and a final address equal to last_address, and islice keeps the generator lazy.

Three actionable items below.


1. Host bits are cleared after uniqueness validation → unhandled IntegrityError

netbox/dcim/models/devices.py:1707

def save(self, *args, **kwargs):
    # Clear any host bits from the prefix, and cache the last address in the block
    self.prefix = self.first_address
    self._last_address = self.last_address
    super().save(*args, **kwargs)

full_clean() runs validate_constraints() — and ModelForm._post_clean() / ValidatedModelSerializer.validate() likewise — against the raw prefix, before save() normalizes it.

Scenario: BC:24:11:00:00:00/24 already exists. Submit prefix=BC:24:11:AA:BB:CC, prefix_length=24 via the form, the REST API, or CSV import. The constraint check compares BC:24:11:AA:BB:CC against the stored BC:24:11:00:00:00, sees no conflict, and passes. save() then rewrites prefix to BC:24:11:00:00:00 and the INSERT violates dcim_macprefix_unique_prefix_prefix_length → IntegrityError / HTTP 500 instead of a field-level error. Nothing in ObjectEditView or the API exception handling catches IntegrityError, so this is a hard 500.

test_prefix_normalized and test_duplicate_prefix each cover one half of this but never the combination.

Suggested fix: normalize in clean() instead of (or in addition to) save(), so validation sees the normalized value, and add model_clean_fields = ('prefix',) to MACPrefixSerializer.Meta — that hook exists precisely for "DRF builds a fresh instance from the validated data on create, discarding what clean() wrote".

Fix this →

2. save() can write NULL into the non-nullable prefix / _last_address

netbox/dcim/models/devices.py:1708

first_address / last_address return None whenever _get_bounds() fails (invalid prefix, or a prefix_length outside 1–48 — the validators only run under full_clean()). save() assigns those Nones unconditionally.

Scenario: a script or plugin calls MACPrefix.objects.create(prefix='BC:24:11:00:00:00', prefix_length=0). Instead of a ValidationError naming prefix_length, you get null value in column "prefix" violates not-null constraint. Worth guarding in save() (raise, or leave the values untouched) so the failure is diagnosable.

3. The API doesn't expose last_address, size, or utilization

netbox/dcim/api/serializers_/devices.py:371

The detail panel and list table render last_address, size, and utilization, and docs/models/dcim/macprefix.md advertises utilization as a feature, but MACPrefixSerializer exposes only mac_address_count. An API/GraphQL consumer reporting on block utilization has to re-derive all three from prefix + prefix_length client-side, even though last_address is already cached in the _last_address column. Adding them as read-only serializer fields would close the UI/API gap.


Things I checked that are fine

  • prefix__icontains in MACPrefixFilterSet.search — safe on a macaddr column: PatternLookup.prepare_rhs = False means get_prep_value() (which would raise ValidationError on "Prefix 1") is never called, and Postgres renders UPPER(prefix::text) LIKE ….
  • mac_address_count annotation vs. the cached_property of the same name — cached_property is a non-data descriptor, so the annotation written into __dict__ wins. No conflict with annotate() either, since it isn't a model field.
  • Pagination COUNT on the annotated queryset — the subquery annotation isn't an aggregate and doesn't force a GROUP BY, so Django masks it out of a plain .count(). StripCountAnnotationsPaginator isn't needed here.
  • exclude(pk=self.pk) with self.pk is None in MACAddress.clean() — Django converts this to pk__isnull=True, so nothing is wrongly excluded on create.
  • url_params with a callable returning None (prefix fully allocated) — LinkAction.get_url() omits None values, so no ?mac_address=None.
  • The "Add Next Available" lambda runs only after PanelAction.render()'s add-permission check, so the scan isn't triggered for view-only users.
  • _last_address is editable=False but still serialized into changelog snapshots — consistent with IPAM's _depth/_children, and Field.value_to_string handles EUI fine.
  • owner / comments are absent from MACPrefixForm.fieldsets but htmx/form.html renders them automatically (same as MACAddressForm).
    • branch 18040-mac-prefix

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant