Closes: #202 - IS-IS Support#203
Open
marcinpsk wants to merge 17 commits into
Open
Conversation
Adds IS-IS as a routing protocol in netbox-routing: models, migration, REST and GraphQL APIs, filtersets, forms (incl. bulk edit/import), tables, detail/list UI panels, navigation, and search indexes. Covers instances, interfaces, levels, interface-levels, segment-routing, flex-algos, and an EAV-style ISISSetting for the long-tail per-object knobs that don't warrant their own model column. Notable behaviour: - Plaintext routing-auth keys (area/domain/hello/level) are kept off the GraphQL query surface; the non-secret auth type stays queryable. - ISISSetting requires an assigned object (model clean + DB CheckConstraint) and its list endpoint GenericPrefetches the assigned_object target. - Child list endpoints select_related the FK chains their display strings walk, so the API query count stays flat as rows grow. - (device, process_tag) is unique per instance; filterset search trims input.
The aggregate tests.test_models __all__ enumerated 13 of the 14 model TestCases its submodules export but omitted ISISSettingModelTestCase, so a wildcard import `from netbox_routing.tests.test_models import *` (used for test discovery) could silently drop it. Add it, plus a guard test asserting the aggregate re-exports every name in each wildcard-imported submodule's __all__.
…all__ membership The AggregateModelTestExportsTestCase guard only checked that each submodule TestCase name appears in this module's __all__ tuple. That passes even if a 'from ... import *' line is dropped while the name is left in __all__ — exactly the regression the guard exists to catch, since 'from netbox_routing.tests.test_models import *' (used for test discovery) then raises AttributeError on the unbound name. Also assert each name is bound on the aggregate module via hasattr, so the guard fails when re-export actually breaks. Addresses CodeRabbit review on PR #1.
Addresses the IS-IS review findings on this branch; touches IS-IS files only (plus the shared test aggregates, for IS-IS entries). No BGP/other-protocol source changed. - api: ISISInstanceSerializer.vrf was required though the model field is optional (blank/null) — add required=False, allow_null=True so an API create/update may omit vrf (matches the OSPF sibling). - model/migration: dedup Segment-Routing state — drop the duplicate ISISInstance.sr_enabled / sr_node_msd columns; SR state lives only on the 1:1 ISISSegmentRouting child (removed from model, migration 0033, serializer, forms, panels, import, clone_fields). - migration: record bases=(DeleteMixin, models.Model) on the ISISInstance and ISISInterface CreateModel ops (every sibling does; their historical state had silently dropped DeleteMixin). - model: validate the NET format in ISISInstance.clean() (form/API/import all run full_clean) instead of letting a malformed NET fail at device push. - forms: ISISInterfaceImportForm scopes the by-name interface lookup to the row's device (an interface name is only unique within a device). - views: add select_related/prefetch_related to every IS-IS list view and ObjectChildrenView.get_children (matching the API viewsets) — fixes the per-row N+1 on instance/device/vrf/interface. - model: extract the shared _auth_pair_errors() helper (was hand-rolled 4x); move the instance/interface same-device check fully to ISISInterface.clean() (flagging both fields) and drop the redundant form-level clean(). - constants/model: derive the ISISSetting assignment allow-list from a single ISISSETTING_ASSIGNMENT_MODEL_NAMES tuple; the Q deconstructs identically so migration 0033 stays in sync. - navigation: build the IS-IS menu via a build_menus() loop (matches bgp/community) and gate the Import button on the real add_* permission. - ui: add the Device row to ISISInterfacePanel (mirrors OSPF). - forms: drop the dead 'ipaddr' settings-field branch (no such FIELD_TYPE). - model: drop the inert SearchAttributeMixin from ISISSetting (its search_display_name is used only by BGP indexes; no IS-IS index references it) and from the recorded migration bases. - tests: red->green coverage for each fix; complete the test_api/test_forms/ test_models aggregate __all__ exports and extend the re-export guard. Out of scope (intentionally not changed): the ISISSettingMixin/BGPSettingMixin duplication (would require touching BGP).
Owner
|
Is there anything in IS-IS that is missing from this PR? If not, the model itself looks pretty solid |
Contributor
Author
|
@DanSheps that's a good question, what is there now worked for me, but just going over devices I see missing - |
Owner
|
If you could add those that you think are best that would be sweet. |
- HMAC-SHA auth-type choices + TE router-ID setting keys - Fast-reroute / TI-LFA fields: ISISInstance.fast_reroute + microloop_avoidance, ISISInterface.frr_enabled (tri-state) + frr_protection - SR-MPLS / SRv6 model surface: per-prefix ISISPrefixSID, SRLB + srv6_enabled + clean() on ISISSegmentRouting, ISISSRv6Locator, attached-bit on ISISInstance - SR test-case re-exports, SR URL registration
…ializers Drop the parent `instance` FK from ISISFlexAlgoSerializer.brief_fields so it matches the other five ISIS child serializers (Level, InterfaceLevel, SegmentRouting, PrefixSID, SRv6Locator), which all expose only url/id/display plus natural identifiers. The owning object is already conveyed by `display`. Update the API test's expected brief_fields to match.
Remove hello_auth_key (and its paired hello_auth_type) from ISISInterface.clone_fields. Cloning serializes clone_fields into the add-form URL query string, which would expose the plaintext key in browser history, server logs and referrer headers. This mirrors ISISInstance/ISISLevel, which already exclude their auth secrets from clone_fields; dropping the type too avoids a type-without-key prefill tripping the paired-auth validation on every clone. Adds a model test asserting clone() omits both fields.
…gFilterSet filter_prefix() called value.strip() directly, so a None value (an empty prefix filter) raised AttributeError; normalize `value = (value or '').strip()` first, matching the sibling search() helpers. Adds a filter_prefix(None) regression test, plus the previously-missing q / blank-search coverage for ISISSettingFilterSetTestCase (its search() matches key/value).
…length ISISSRv6Locator.algorithm documents the domain 0 (SPF) or 128-255 (Flex-Algo) but had no enforcement, so 1-127/>255 could persist. Add a clean() check and a DB CheckConstraint (mirrored in migration 0033), matching ISISPrefixSID. Also fix the SID-structure length check: when block and node lengths are both unpinned the locator length is the prefix length, so derive `loc` from prefix.prefixlen instead of falling back to 0 — otherwise an oversized derived locator (e.g. /120 + 16-bit function = 136 > 128) slipped through. Tests: clean() rejects 1/127/256 and the oversized derived locator; accepts 0/128/255/None; the DB constraint rejects out-of-range on direct save().
ISISPrefixSID/ISISSRv6Locator BulkDelete/BulkImport views are defined and exported in views/isis.py but were missing from the views package __all__, unlike every other IS-IS view group. Add them, with a regression test.
isisinterface_prefix-sids and isisinstance_srv6-locators were grouped under the wrong model blocks. Move each into its owning model's section so the hardcoded (pre-sorted) tab-route list stays readable; sorted() already normalized runtime.
Contributor
Author
|
added those. But CI is going to be red because of what came in recently to netbox/main. I'll add that separately in case it's useful - would still need to generate query_counts.json on this PR. |
…rt_expected_query_count) Generated serially via UPDATE_QUERY_COUNTS=1 on NetBox 4.6.4 against a fresh test DB built from this branch's migrations; confirmed with a second run without the env var (216 list tests, OK).
…ounts The initial recording ran with -k test_list_objects, so each test class executed only its list test and the process-lifetime ContentType cache was cold: serializing the scope/settings generic relations of BGPScope, BGPRouter, BGPAddressFamily and ISISSetting emitted one extra django_content_type natural-key SELECT. CI (and any full-suite run) executes whole test classes, whose earlier methods warm the cache via get_for_model(), so the list tests observe one query fewer. Recorded the warm-cache counts (matching CI on netbox main) and verified with full-module runs of bgp/isis test_api: zero query-count mismatches.
DanSheps
reviewed
Jul 15, 2026
DanSheps
approved these changes
Jul 15, 2026
Move NET_RE and auth_pair_errors out of models/isis.py into netbox_routing/helpers/isis.py (renamed to public names) so the model module holds models rather than free-standing validation utilities; models/isis.py imports them and the now-unused `import re` is dropped. Behaviour is unchanged - the IS-IS model clean() tests (auth-pair and NET validation) still pass. Also add ISISViewExportsTestCase to the tests/test_views.py aggregate __all__ so the re-export guard test_isis_testcases_reexported_in_all_aggregates passes: the isis submodule exported it but the aggregate omitted it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes: #202 - IS-IS Support
I'm not sure this is in a shape that could be accepted, but if not, happy to just keep it in my fork, as we are using it as-is today.
The only part we don't really use but felt cheap to just add while touching it is area/domain authentication - everything else we are using as-is.
Would close #202 if accepted.
Tried to follow existing style.