nd_interface_loopback: policy_type discriminated union - #403
Conversation
67358f0 to
2afa387
Compare
e4da480 to
78cb3f2
Compare
…lasses (#403) Review feedback from @akinross: - Rename LoopbackPolicyBase/LoopbackPolicyModel to NexusLoopbackPolicyBase/ NexusLoopbackPolicyModel so NX-OS-specific classes carry an OS prefix like the Xe* classes do (Nexus chosen to match NexusLoopbackNetworkOSModel). Ipfm/Mpls/Csr* classes keep their names - they already mirror their wire policyType discriminator, which is OS-unambiguous. - Expose policy_type as a property on LoopbackInterfaceModel; drop the orchestrator's getattr-chain _extract_policy_type static method. - Extract bulk-create group construction into _group_by_switch_and_policy_type (the mixed-policy-type TODO marker moves with it). - Replace the group tuples with frozen slotted dataclasses _BulkCreateGroupKey/_BulkCreateItem for readability. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BHGFDqqU37PRqNHhKw1FCQ
There was a problem hiding this comment.
Pull request overview
This PR expands nd_interface_loopback from a single NX-OS loopback template into a discriminated-union driven implementation that supports multiple NX-OS loopback policy templates and a full IOS-XE loopback surface, while hardening create/bulk-create behavior against ND’s per-item 207 Multi-Status failure reporting and policyType-mixing limitations.
Changes:
- Introduces required discriminators
network_os_type(outer union) andpolicy_type(policy union) and adds per-template models/argspec/docs to support NX-OS + IOS-XE loopback templates in one module. - Normalizes loopback IPv4
ipinput to a bare host address for ND 4.2.1 wire compatibility (accepts CIDR input, sends bare IPv4). - Updates the orchestrator to (a) validate per-item
DATA.results[]statuses for create/create_bulk and (b) group bulk creates by(switch_id, policy_type).
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/module_utils/orchestrators/test_loopback_interface.py | Updates orchestrator unit tests for new model types, adds coverage for per-item 207 failures and bulk-create grouping by policy type, and expands query_all filtering expectations. |
| tests/unit/module_utils/models/test_loopback_interface.py | Adds extensive unit coverage for the new discriminated unions (NX + XE) and for IPv4 host normalization + strict/read-tolerant policy validation. |
| tests/unit/module_utils/fixtures/fixture_data/test_loopback_interface.json | Extends fixtures to cover new query_all filtering cases and the new 207-results failure shapes used in create/create_bulk tests. |
| tests/integration/targets/nd_interface_loopback/vars/main.yaml | Updates reserved loopback ID ranges, makes network_os_type/policy_type explicit, and adds IOS-XE test variables gated by nd_test_xe_switch_ip. |
| tests/integration/targets/nd_interface_loopback/tasks/main.yaml | Conditionally includes IOS-XE integration tasks when nd_test_xe_switch_ip is provided. |
| tests/integration/targets/nd_interface_loopback/tasks/setup.yaml | Renumbers reserved loopback IDs used for integration setup cleanup. |
| tests/integration/targets/nd_interface_loopback/tasks/merged.yaml | Updates merged-state integration coverage to include required discriminators and new ID ranges. |
| tests/integration/targets/nd_interface_loopback/tasks/replaced.yaml | Updates replaced-state tasks for required discriminators and renumbered IDs. |
| tests/integration/targets/nd_interface_loopback/tasks/overridden.yaml | Updates overridden-state tasks for required discriminators, renumbered IDs, and system-loopback filtering assumptions. |
| tests/integration/targets/nd_interface_loopback/tasks/deleted.yaml | Updates deleted-state tasks for renumbered IDs and the revised vars structure. |
| tests/integration/targets/nd_interface_loopback/tasks/xe.yaml | Adds IOS-XE integration coverage across all states plus idempotency, including mixed NX+XE overridden coverage when fabrics match. |
| plugins/modules/nd_interface_loopback.py | Updates module docs/examples to expose and describe the new discriminators and per-template policy fields across NX-OS and IOS-XE. |
| plugins/module_utils/orchestrators/loopback_interface.py | Hardens create/create_bulk against per-item failures and changes bulk-create grouping to (switch_id, policy_type); expands query_all filtering to NX + XE managed policy types. |
| plugins/module_utils/models/types.py | Adds IPv4Host type to normalize IPv4 to bare host form while accepting CIDR input. |
| plugins/module_utils/models/interfaces/loopback_interface.py | Reworks loopback models into nested discriminated unions (network OS + policy type), adds strict/read-tolerant behavior for policy objects, and exposes required discriminators via argspec. |
| plugins/module_utils/models/interfaces/enums.py | Adds managed loopback policy-type enums for NX-OS and IOS-XE used by orchestrator filtering and documentation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Copilot review: the ip option description did not mention that CIDR input (e.g. 10.1.1.1/32) is accepted but normalized to the bare address on the wire and in returned state (IPv4Host, models/types.py). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WqAuV2pWYJTno2bfdcNZCm
…lasses (#403) Review feedback from @akinross: - Rename LoopbackPolicyBase/LoopbackPolicyModel to NexusLoopbackPolicyBase/ NexusLoopbackPolicyModel so NX-OS-specific classes carry an OS prefix like the Xe* classes do (Nexus chosen to match NexusLoopbackNetworkOSModel). Ipfm/Mpls/Csr* classes keep their names - they already mirror their wire policyType discriminator, which is OS-unambiguous. - Expose policy_type as a property on LoopbackInterfaceModel; drop the orchestrator's getattr-chain _extract_policy_type static method. - Extract bulk-create group construction into _group_by_switch_and_policy_type (the mixed-policy-type TODO marker moves with it). - Replace the group tuples with frozen slotted dataclasses _BulkCreateGroupKey/_BulkCreateItem for readability. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BHGFDqqU37PRqNHhKw1FCQ
Copilot review: the ip option description did not mention that CIDR input (e.g. 10.1.1.1/32) is accepted but normalized to the bare address on the wire and in returned state (IPv4Host, models/types.py). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WqAuV2pWYJTno2bfdcNZCm
dd5cfc3 to
137bc8d
Compare
…ture formatting - Add missing ## Test section to test_loopback_interface_00760 docstring - Remove redundant inline `from pydantic import ValidationError` in two model tests that already have the module-level import - Normalize test_loopback_interface_00760a/b/c fixture formatting to match surrounding one-key-per-line style
…read path
LoopbackPolicyBase.strip_none_valued_keys now also strips keys not declared on
the model when validation context is {"mode": "read"} (set by
LoopbackInterfaceModel.from_response). This tolerates ND-injected read-only
policy keys (e.g. linkStateRoutingTag, confirmed live on ND 4.2.1) that
previously tripped extra="forbid" and aborted the module on every read.
Write-side input (no read context) remains strict, so wrong-branch fields are
still rejected.
Also fixes a stale unit test (test_loopback_interface_00420) whose premise
predated the ipfmLoopback/mplsLoopback discriminated-union support already on
this branch: policyType="ipfmLoopback" is a supported value, not a rejected
one; only a value outside the three-way union should still fail from_response.
Live ND 4.2.1 400s a loopback create when ip is CIDR (e.g. 10.99.99.250/32) and accepts the bare form. Add IPv4Host to models/types.py (BeforeValidator normalizing IPv4Interface to its bare .ip form) and switch LoopbackPolicyBase.ip from IPv4CIDR to IPv4Host so bare or CIDR input is accepted but always normalized to bare before it reaches the wire; this also keeps idempotency since ND always returns bare on read. ipv6 (IPv6CIDR) is unchanged - not verified against ND in this probe. Updated unit test SAMPLE data/assertions that expected the CIDR ip form to expect the normalized bare form; integration tasks under tests/integration/targets/nd_interface_loopback already use bare ip values and needed no changes.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014aJ3Y2TJEAqJZBeUKdUnGa
Per team decision, the platform/OS selector is a mandatory argspec parameter for now so interface parameters validate before any controller call; a FabricContext.get_platform_type() fallback comes later when the parameter becomes optional. Mirrors the ND schema, where networkOSType is a required create-side discriminator. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014aJ3Y2TJEAqJZBeUKdUnGa
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014aJ3Y2TJEAqJZBeUKdUnGa
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014aJ3Y2TJEAqJZBeUKdUnGa
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ypeEnum Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…-18) ND 4.2.1 echoes csrLoopback on reads despite the OpenAPI read schema listing csrIntLoopback; drop the defensive dual-name tolerance. Drift recorded in the bug-tracker vault (csr-loopback-read-schema-name-drift). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… cleanups - 00770c: nest networkOSType inside networkOS; policy objects mirror the 2026-07-18 lab GET key set; honest TEST_NOTES - XeLoopbackNetworkOSModel: restore not-frozen rationale comment - 00770: assert result length, not just the policy-type set - argspec: secondary_ip/enable_pim placed to match DOCUMENTATION order - integration: XE gate rejects empty nd_test_xe_switch_ip - csr1kv extra_config description aligned with siblings - query_all: hoist invariant managed_policy_types out of the switch loop Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…back100)
The lab's new multisite topology (S1_BG1 <-> WAN1/ISN) auto-provisions
loopback100 (policyType multisiteLoopback, ND's default multisite loopback
ID) on the border gateway. query_all rightly filters system-provisioned
types, so the module cannot see the name squatter and the create 500s
("loopback100 is already in use"). Move the reserved test range clear of
ND defaults: IDs 200-209 (+299 never-exists probe), IPs 10.100.20x.x.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ND rejects mixed-policyType bulk creates (207 with a failed item and nothing created) and the module trusted the 207, reporting changed while creating nothing - surfaced by the XE integration idempotency test. Group one POST per (switch, policyType) and fail on any results[] item whose status is not exactly 'success'. Scoped to the loopback orchestrator; PR #398's NdV1Strategy 207 support supersedes the check. Vault: bulk-interface-create-rejects-mixed-policy-types, multi-status-207-status-field-inconsistent. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The in-repo design and plan docs were POC-phase working artifacts; per the PR description's graduation note they are removed before opening for team review. Archived locally at ~/pr/403 for historical reference. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…lasses (#403) Review feedback from @akinross: - Rename LoopbackPolicyBase/LoopbackPolicyModel to NexusLoopbackPolicyBase/ NexusLoopbackPolicyModel so NX-OS-specific classes carry an OS prefix like the Xe* classes do (Nexus chosen to match NexusLoopbackNetworkOSModel). Ipfm/Mpls/Csr* classes keep their names - they already mirror their wire policyType discriminator, which is OS-unambiguous. - Expose policy_type as a property on LoopbackInterfaceModel; drop the orchestrator's getattr-chain _extract_policy_type static method. - Extract bulk-create group construction into _group_by_switch_and_policy_type (the mixed-policy-type TODO marker moves with it). - Replace the group tuples with frozen slotted dataclasses _BulkCreateGroupKey/_BulkCreateItem for readability. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BHGFDqqU37PRqNHhKw1FCQ
Copilot review: the ip option description did not mention that CIDR input (e.g. 10.1.1.1/32) is accepted but normalized to the bare address on the wire and in returned state (IPv4Host, models/types.py). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WqAuV2pWYJTno2bfdcNZCm
…#403) With validate_assignment=True, every setattr (e.g. inside NDBaseModel.merge()) re-runs the mode="before" model validator over the model's full __dict__. LoopbackPolicyStrictBase.strip_none_valued_keys dropped ALL None-valued keys - declared fields included - so the rebuilt __dict__ lost every unset field and the next getattr mid-merge raised AttributeError. Merging a newly-set field onto an existing model whose corresponding field was None (e.g. adding a description to a loopback created without one) crashed. Declared keys now survive the strip even when None (a declared None never trips extra="forbid"); only undeclared None keys are dropped, and read mode still strips undeclared keys regardless of value. Found live by the nd_interface_ethernet_routed integration run (2026-07-27), which copies this base's pattern; loopback lab flows never merged a new field onto an existing-None field, so the bug was latent here. Regression test test_loopback_interface_00740 pins the merge. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…or (#403 review) NDBaseModel.merge() raised a bare TypeError naming internal Pydantic classes when a proposed nested model's concrete type differed from the existing one. For discriminated-union fields (policy_type, network_os_type) this is a user input scenario, not an internal error: merging across two union branches is undefined because the branches have disjoint field sets. Add a discriminator-aware pre-check in the merge recursion loop: when the field is a discriminated union and the discriminator value would change, raise ValueError with a message naming both values and pointing at state: replaced. Non-union type mismatches keep the existing TypeError. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
PR #398 (merged) centralizes Multi-Status per-item failure detection in NdV1Strategy.is_success, which _request consults on every call, so the loopback orchestrator's _raise_on_failed_result_items and its two call sites are dead code: _request raises before the helper ever ran. The vault note multi-status-207-status-field-inconsistent records the absent-status failure case only on updateGroups/softwareUpdatePlan endpoints, which this orchestrator never calls, so the central literal-gated scan fully covers the interface create paths. Tests 00130/00440 keep their lab-verified 207 wire-shape fixtures; the failure now surfaces via NdV1Strategy through _request. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019XZiWNrVHYXbeNXRM8kUrz
- Add IPv4HostStrict Annotated type: bare IPv4 only, CIDR rejected. Unlike IPv4Host (which normalizes CIDR to bare), the secondaryIpList item carries its mask length in the sibling `prefix` field, so CIDR input is ambiguous and rejected rather than silently normalized. - Use it for SecondaryIpModel.ip (ND item schema is format: ipv4); document the bare-host contract in the module option docs. - Tests: valid bare IPv4 accepted; malformed text, IPv6, and CIDR rejected. - Populate the empty RETURN block using nd_interface_port_channel_access as the canonical pattern, with loopback-specific samples. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XrxedqAeo5ZpMP9iKM3Gg3
0f2fe67 to
a911217
Compare
|
I’m holding approval on this PR for three reasons:
Once these items are addressed, I can re-review promptly. The other tracked follow-up work and low-severity RETURN documentation issue are not blocking approval. |
Do not merge this PR before #422 (replaced/overridden field-removal detection). Reasoning:
NDBaseModel.get_diffplus per-modelreverse_diff_defaultstables (ND echoes template defaults for unset fields, so the reverse pass must strip them or replaced/overridden idempotency breaks). Its entire footprint in the loopback model is one flat table:{"adminState": True, "routeMapTag": "12345"}.policy_typepolicy models (3 NX-OS + 6 IOS-XE), which relocatesrouteMapTagthree levels deep intoNexusLoopbackPolicyModeland means each policy model needs its ownreverse_diff_defaultstable sourced from its own OpenAPI template schema. That per-policy-type work belongs here — this PR introduces the new policy types — and the_strip_reverse_diff_defaultsrecursion in Detect field removals in replaced/overridden states (one-way subset diff) #422 already supports nested per-model tables.loopbackpolicy type that exists ondeveloptoday, and confines the semantic overlap to a well-defined rebase task on this branch.Rebase checklist for this branch after #422 lands:
"routeMapTag": "12345"entry intoNexusLoopbackPolicyModel.reverse_diff_defaults; keepadminStateat the level where it lives post-restructure.reverse_diff_defaultstables for the eight new policy models (ipfmLoopback,mplsLoopback, and the six IOS-XE templates), each sourced from its policyType's OpenAPI template schema; lab-verify echoes where possible. Any policy model without a table silently loses replaced/overridden removal detection.Related Issue(s)
Relates to #401 (develop
ip=IPv4CIDRhandling). The bare-IPv4 normalization on this branch overlaps with open PR #402 — whichever lands second will rebase and drop the duplicate change. No issue is closed by this PR.The design/plan docs that accompanied the POC phase have been removed from the branch and archived outside the repo.
Proposed Changes
Consolidates the loopback interface behind discriminated unions so a single module handles every ND loopback policy template, across both switch OSes.
policy_typediscriminated union (NX-OS):LoopbackPolicyTypeEnumand aLoopbackPolicyBase; makepolicy_typerequired and strict on the write path.IpfmLoopbackPolicyModel,MplsLoopbackPolicyModel, andSecondaryIpModel; wire the discriminated union onpolicy_type.policy_typeand the per-template union fields.query_allmanagesloopback,ipfmLoopback, andmplsLoopbackvia a set filter.extra="forbid"was aborting reads); the write path stays strict.ipto bare IPv4 (ND 4.2.1 rejects CIDR on the wire).policy_typeand the per-template fields.Required
network_os_typediscriminator:network_os_type(mirrors the ND API's requirednetworkOSTypediscriminator) — so all interface parameters validate before any controller call. A future enhancement makes it optional with a per-switchFabricContext.get_platform_type()fallback (FabricContext: expose platform_type, retain switch records, fail closed (#399, #400) #404).IOS-XE loopback surface (
choices: [nx-os, ios-xe]):config_data.network_osis an outer discriminated union (NexusLoopbackNetworkOSModel | XeLoopbackNetworkOSModel), each OS branch carrying its ownpolicy_typeunion: NX-OS keepsloopback/ipfmLoopback/mplsLoopback; IOS-XE addsiosXeLoopback,iosXeLoopbackShutNoshut,iosXeUnderlayLoopback,iosXeInternalLoopback,csrLoopback,csr1kvLoopback(all six real XE templates;userDefinedstays excluded on both OSes). Cross-OS mismatches (nx-os+iosXeLoopback) fail structurally in Pydantic — no hand-written cross-check.LoopbackPolicyStrictBasesplit:extra="forbid"+ read-tolerant validator +admin_stateshared by all nine branches; NX common fields stay onLoopbackPolicyBase.secondary_ip,enable_pim.query_allfilter is the union of the NX and XE policy-type enums (XeLoopbackPolicyTypeEnum).csrIntLoopbacknever appears on the wire — ND echoes the create-sidecsrLoopback— so the model uses the single wire-verified name (drift recorded in the team bug-tracker vault:csr-loopback-read-schema-name-drift); the same probe found no ND-injected keys in XE policy reads.ios-xeplus acsrLoopbackexample.nd_test_xe_switch_ip(skipped unless supplied), including a fabric-equality-guarded mixed NX+XEoverriddenvariant.Lab-surfaced bug fixes:
create_bulknow groups one POST per (switch, policyType) — ND rejects mixed-policyType bulk creates (207 with a failed item, nothing created;TODO(4.2.1)marker referencing vault notebulk-interface-create-rejects-mixed-policy-types). Per-item 207results[]failures on both create paths are detected centrally byNdV1Strategy.is_success(Detect Multi-status per-item failures in NdV1Strategy (#295) #398, merged 2026-08-11) — the orchestrator-level stopgap this branch originally carried was removed in commit "Drop the orchestrator-level 207 results[] stopgap superseded by Detect Multi-status per-item failures in NdV1Strategy (#295) #398" once Detect Multi-status per-item failures in NdV1Strategy (#295) #398 landed ondevelop.loopback100(ND's default multisite loopback ID) on border gateways, colliding with the old reserved range.Test Notes
nd_interface_loopbackintegration tests are green against these changes (merged/replaced/overridden/query, including the addedpolicy_type: loopbacktasks).test_loopback_interface.py), orchestrator, andnd_argument_specssuites.network_os_typecommits): integration tests re-run green against a live ND 4.2.1 lab with the now-requirednetwork_os_type: nx-osin everynetwork_osblock; full unit suite (3195 tests) green;validate-modulessanity clean onnd_interface_loopback.py.validate-modulesclean; black/isort/pylint/mypy clean on changed files; ansible-lint clean on the integration target. Live-lab raw-REST probe verified XE create/read/delete behavior on a real IOS-XE device (intent-only). XE integration run against the live lab: all tests pass (all four states + idempotency on the IOS-XE device,iosXeLoopbackandcsrLoopback), which also verifies the csr single-name decision on the full deploy path — the deployed read echoescsrLoopback, not the read-schema'scsrIntLoopback. The first run surfaced two real module bugs (mixed-policyType bulk-create rejection masked by unchecked 207s) — fixed with TDD unit coverage, then the re-run went green. (No CI on stacked PRs — local verification per team convention.)Cisco Nexus Dashboard Version
4.2.1
Related ND API Resource Category
Checklist