Add ND 4.2 Links module (nd_links) - #312
Conversation
3d0806c to
135f6b0
Compare
|
@shrsr Merge conflicts need to be resolved |
|
@mikewiebe yes, a few PRs were merged just now causing the conflicts. Let me look into it |
def7450 to
bfabb58
Compare
|
Issue There is a potential performance and scale problem. Small link changes appear to start with a full fabric link inventory read. Evidence
Impact On large data center fabrics, small changes pay the cost of a full-fabric link Suggested fix Prepare enough proposed identity data before querying, then use scoped query |
|
Issue The PR does not include unit tests for the new model, Evidence
Impact The most important runtime contracts here live in Pydantic and orchestrator call Suggested fix Add unit tests for discriminated-union validation, secret masking, |
| module swaps in MANAGE_SCOPE_IDENTIFIERS (no cluster names) at runtime. | ||
| """ | ||
|
|
||
| identifiers: ClassVar[list[str] | None] = [ |
There was a problem hiding this comment.
High: A physically equivalent reversed eBGP link plans an update
Issue
@shrsr partial fix, but details here on what remains...
The orientation-independent identity and symmetric field swapping added since
the original review fix most of the reversed-endpoint issue. A residual
idempotency defect remains for policies such as ebgpVrfLite that represent an
endpoint pair with asymmetric wire fields:
srcIpAddressMask and dstIpAddress.
The implementation deliberately does not reorient that pair. A physically
equivalent declaration written from the opposite endpoint therefore plans an
update even though each switch retains the same ASN and IP address.
Evidence
plugins/module_utils/models/links/links.pylines 202-257
documents the limitation and excludes asymmetric address pairs from
reorientation.tests/unit/module_utils/models/test_links.pylines 482-490
explicitly asserts thatsrcIpAddressMask/dstIpAddressis not reoriented.- A read-only test against Nexus Dashboard 4.2 at
10.122.84.112, using fabric
Siva_External_Fabric, selected liveebgpVrfLiterecord
LINK-UUID-3401130.
The controller and reversed declarations assign the same ASN and IPv4 address
to each physical endpoint:
| Endpoint | Controller orientation | Equivalent reversed declaration |
|---|---|---|
| Source | Siva_Fabric_Adv / NX_203 / Ethernet1/1 |
Siva_External_Fabric / netascode-leaf1 / Ethernet1/1 |
| Source ASN | 123124 |
3345 |
| Source address | 10.33.0.2/24 |
10.33.0.1/24 |
| Destination | Siva_External_Fabric / netascode-leaf1 / Ethernet1/1 |
Siva_Fabric_Adv / NX_203 / Ethernet1/1 |
| Destination ASN | 3345 |
123124 |
| Destination address | 10.33.0.1 |
10.33.0.2 |
In both orientations:
netascode-leaf1owns ASN3345and10.33.0.1/24.NX_203owns ASN123124and10.33.0.2/24.
After the PR canonicalizes endpoint orientation and swaps the symmetric ASN
pair, these are the values it compares:
| Canonical field | Controller record | Reversed declaration |
|---|---|---|
srcEbgpAsn |
3345 |
3345 |
dstEbgpAsn |
123124 |
123124 |
srcIpAddressMask |
10.33.0.2/24 |
10.33.0.1/24 |
dstIpAddress |
10.33.0.1 |
10.33.0.2 |
The symmetric values compare correctly. Only the asymmetric IPv4 pair remains
attached to the wire field names rather than the physical endpoints.
Minimum live reproduction
---
- name: Reproduce asymmetric reversed-link change without mutation
hosts: nd_112
gather_facts: false
tasks:
- name: Merge the existing physical link from its opposite orientation
cisco.nd.nd_manage_links:
fabric_name: Siva_External_Fabric
link_scope: manage
output_level: debug
state: merged
config:
- src_fabric_name: Siva_External_Fabric
src_switch_name: netascode-leaf1
src_interface_name: Ethernet1/1
dst_fabric_name: Siva_Fabric_Adv
dst_switch_name: NX_203
dst_interface_name: Ethernet1/1
config_data:
policy_type: ebgpVrfLite
template_inputs:
src_ebgp_asn: "3345"
dst_ebgp_asn: "123124"
src_ip_address_mask: 10.33.0.1/24
dst_ip_address: 10.33.0.2
check_mode: trueThis static playbook supplies only the reversed endpoints, ASNs, and IPv4 pair,
eliminating unrelated template inputs as a possible cause. Against exact PR
head 42acbbbe9ef24a25755ff5ea41cce67c18ed88b7, it returned:
changed=true
requested_count=1
before_count=6
after_count=6
diff_count=0
proposed_count=1
The run used check mode and did not mutate controller configuration.
Existing PR overlap
This is an update to the original reversed-endpoint finding. The broad ordered
identity issue is fixed; this live-confirmed asymmetric address case remains.
Existing open issue overlap
No matching open issue found.
Impact
Equivalent intent is non-idempotent. A normal merged run can issue an
unnecessary update, and users must know and preserve the controller's arbitrary
source/destination orientation to avoid changes. The same endpoint-ownership
problem can affect reversed delete and authoritative workflows for policies
with asymmetric address fields.
Suggested fix
Represent addresses internally as endpoint-owned values, including prefix
length, and render the policy's asymmetric wire field names only after
canonical comparison. Add live-shaped tests for reversed merge, delete, and
override across every inter-fabric policy that uses asymmetric address fields.
There was a problem hiding this comment.
Done. Intra fabric links use an orientation independent identity and canonical diff, so a reversed link is matched instead of duplicated or recreated.
There was a problem hiding this comment.
Jumping into Mike's thread with one observation from re-review, since the fix is close: the orientation-independent identity plus canonicalized diff view looks right, and keeping payloads in the user's orientation is the correct call.
One gap in the swap list: _TEMPLATE_DIRECTIONAL_ALIAS_PAIRS (links.py:213) covers srcIp/dstIp, srcIpv6/dstIpv6, the interface descriptions, and interface configs — but not dhcpRelayOnSrcInterface/dhcpRelayOnDstInterface or bfdEchoOnSrcInterface/bfdEchoOnDstInterface, which are directional fields on the intra-fabric numbered/unnumbered policies. A reversed link with asymmetric settings (e.g. dhcp_relay_on_src_interface: true, dst false) will canonicalize the identity but leave those toggles attached to the wrong end, producing a spurious diff (non-idempotent update) or masking a real one. Suggest adding those two pairs and extending the reversed-orientation test with an asymmetric toggle. Deferring to Mike on whether that's a blocker for this thread or a follow-up.
There was a problem hiding this comment.
Good catch, added the dhcpRelayOnSrc/DstInterface and bfdEchoOnSrc/DstInterface pairs to the swap list and extended the reversed orientation test with an asymmetric toggle, plus a companion test asserting a genuinely asymmetric difference still diffs.
There was a problem hiding this comment.
Thanks, the intra-fabric portion of this finding is addressed, including the additional DHCP relay and BFD echo directional alias pairs. However, the original finding also applies to physical inter-fabric links.
The current implementation explicitly limits orientation-independent identity to links where src_fabric_name == dst_fabric_name. For VRF-lite, DCI, and multisite links, get_identifier_value() still falls back to the ordered source/destination identity. Declaring the same inter-fabric cable with its endpoints reversed can therefore still be treated as a different object, causing merged to attempt a duplicate, deleted to miss the link, or overridden to delete and recreate it.
Could we please keep this thread open until:
inter-fabric physical links also use orientation-independent identity with the appropriate policy-specific directional field swaps; or
The intra-fabric fix is good; this remaining point is specifically about links whose source and destination fabric names differ.
There was a problem hiding this comment.
@shrsr the live testing example above has been updated to show that a problem still exists for assymetric fields
| template_inputs: LinkTemplateInputs | None = Field(default=None, alias="templateInputs") | ||
| template_name: str | None = Field(default=None, alias="templateName") | ||
|
|
||
| def merge(self, other: LinkConfigDataModel) -> LinkConfigDataModel: |
There was a problem hiding this comment.
High: Controller-realized preprovision links make the original declaration non-idempotent
@shrsr Let's discuss this when you get a chance. This is an interesting one and we need to determine what behavior we should support.
@shrsr I Updated this comment after checking our legacy / NaC behavior and discussing with Matt and Pete
Issue
ND correctly turns a planned preprovision link into a real numbered link
with controller-assigned settings after discovery and save/recalculation. This
finding is not asking the module to change that real link back to
preprovision.
The problem is that rerunning the unchanged declarative source now fails. The
module compares the original policy_type: preprovision declaration with ND's
realized policy_type: numbered state and treats the expected lifecycle as a
new, unsupported policy conversion. It recommends deleting and recreating the
valid realized link instead of defining how the original intent should be
reconciled.
This matters for NaC and other Git-driven workflows that repeatedly apply the
same source configuration. The current NaC workflow specifically recognizes
this transition and preserves the controller-assigned values.
Evidence
plugins/module_utils/models/links/links.pyline 22
raises whenever existing and proposedpolicy_typevalues differ, including
the controller-drivennumberedversus originalpreprovisioncase.plugins/module_utils/orchestrators/links.pyline 233
rejects the difference again before update.- NaC
existing_links_check.pylines 182-203
recognizes ND's preprovision-to-numbered conversion and retains the
controller-assigned addresses. - The integration tests exercise links that remain
preprovision; they do not
rerun the original declaration after ND returns the same identity as
numbered.
Live testing on ND 4.2 with exact PR head
5c7d3647ff50328213990f01e75dd7fb7a2c7f25 confirmed:
-
The initial task created the link as
preprovision. -
After the physical link came up, ND
configSavereturned HTTP 200 and
changed the same identity tonumbered, assigning10.4.0.1and
10.4.0.2. -
The unchanged original task failed with
changed: falseand:Cannot change policy_type from 'numbered' to 'preprovision' on an existing link. ND requires deleting the link first and recreating with the new policy_type.
Minimum reproducer
Prerequisite: start with two onboarded switches whose selected physical link is
down. At the pause, bring the link up and run Save/Recalculate in ND so the
controller realizes it as a numbered link.
---
- name: Reproduce realized preprovision reconciliation failure
hosts: nd
gather_facts: false
vars:
test_fabric: example_fabric
test_link: &test_link
src_fabric_name: "{{ test_fabric }}"
dst_fabric_name: "{{ test_fabric }}"
src_switch_ip: 192.0.2.10
dst_switch_ip: 192.0.2.11
src_interface_name: Ethernet1/1
dst_interface_name: Ethernet1/1
module_defaults:
cisco.nd.nd_manage_links:
fabric_name: "{{ test_fabric }}"
link_scope: manage
tasks:
- name: Create the planned link
cisco.nd.nd_manage_links:
state: merged
config:
- <<: *test_link
config_data:
policy_type: preprovision
template_inputs:
src_interface_description: planned source
dst_interface_description: planned destination
- name: Realize the physical link in ND
ansible.builtin.pause:
prompt: >-
Bring both interfaces up, run Save/Recalculate in ND, then press Enter.
- name: Wait for ND to report the realized link as numbered
cisco.nd.nd_manage_links:
state: gathered
register: realized_links
until: >-
realized_links.gathered
| selectattr('src_interface_name', 'equalto', 'Ethernet1/1')
| selectattr('dst_interface_name', 'equalto', 'Ethernet1/1')
| selectattr('config_data.policy_type', 'equalto', 'numbered')
| list
| length == 1
retries: 30
delay: 10
- name: Repeat the identical original declaration
cisco.nd.nd_manage_links:
state: merged
config:
- <<: *test_link
config_data:
policy_type: preprovision
template_inputs:
src_interface_description: planned source
dst_interface_description: planned destinationThe final task fails with the numbered-to-preprovision message even though the
playbook input did not change and ND performed the policy transition.
Legacy module and NaC cross-reference
The history confirms that this lifecycle behavior currently lives in NaC as an
adapter around legacy dcnm_links, with one provenance distinction: the action
plugin itself began as a performance refactor of an existing prefilter, while
its numbered-link enrichment was added later as the compatibility behavior.
- In PR #194,
the workflow queried links withdcnm_links, removed links that already
matched the physical endpoints, and passed only the remaining list back to
dcnm_links. The YAML loop was moved into an action plugin for
performance. - Commit
38f4a4f
added the realized-link handling: when ND reportsint_intra_fabric_num_link,
NaC changes the desired template to numbered and copies ND-assigned
PEER1_IPandPEER2_IPinto the configuration before invoking
dcnm_links. - The current adapter
also suppresses the downstream module call when the enriched profile already
matches. The current task flow
still queries withdcnm_links, runs this adapter, and gives only its
transformedrequired_linkstodcnm_links. - The contemporaneous legacy module treated every template mismatch as
DCNM_LINK_MERGE
and then sent a PUT
for the existing link UUID. It did not distinguish a controller-realized
preprovision link from a user-requested template change.
Functionally the adapter prevents dcnm_links from
changing the realized link back to preprovision and supplies the controller
values that the original declaration does not contain.
Existing PR overlap
No matching existing PR comment found.
Existing open issue overlap
No matching open issue found.
Impact
If preprovision is persistent declarative intent, the same source cannot be
applied after Day 0: every later run fails, and the suggested deletion can
discard a valid physical link and ND-assigned settings.
If preprovision is intentionally only a one-time bootstrap instruction, the
runtime behavior may be expected, but that lifecycle is not currently defined
in the module documentation. Automation would need a documented way to detect
the transition, acquire ND-assigned values, and rewrite its source as
numbered. The current error instead directs users to delete and recreate the
link, which repeats the same lifecycle.
Suggested fix
Absorb the lifecycle behavior currently implemented by NaC directly into
nd_manage_links instead of requiring another external adapter. The module has
both desired and controller state and is the correct ownership boundary for
recognizing this controller-realized equivalence, preserving ND-assigned
values, and remaining idempotent while still rejecting arbitrary policy
changes.
Please define the intended public contract and cover it with documentation and
tests:
- If
preprovisionis persistent declarative intent, treat an existing
numberedlink with the same physical identity as the realized form of that
intent. Preserve ND-assigned values and returnchanged: false; do not send
an update that changes it back topreprovision. - If it is a one-time bootstrap instruction, explicitly document that users
must replace it withnumberedafter realization, document how automation
obtains the assigned values, add a two-phase example, and replace the current
delete-and-recreate guidance with an actionable lifecycle message.
The finding is valid as a runtime idempotency defect under the first contract.
Under the second contract, the required outcome is a clearly documented and
automatable lifecycle rather than silently treating an unchanged declarative
run as an arbitrary policy-change attempt.
There was a problem hiding this comment.
Done. An existing numbered link is recognized as the realized form of a preprovision declaration and left unchanged with changed false.
There was a problem hiding this comment.
Thanks, the unchanged-rerun case is now addressed: an ND-realized numbered link is no longer treated as an illegal attempt to change it back to preprovision.
One lifecycle question remains. _is_realized_preprovision_of() checks only that the existing policy is numbered and the proposed policy is preprovision; get_diff() then immediately returns no diff without comparing the user-managed preprovision fields. As a result, after realization, a later change to src_interface_description, dst_interface_description, src_interface_config, or dst_interface_config is silently ignored and the module returns changed: false.
For example, if the original preprovision declaration used src_interface_description: planned source, ND realizes the link as numbered, and the declaration is later changed to src_interface_description: updated description, the current shortcut still reports no difference.
The eventual behavior likely needs to be one of the following:
- Treat preprovision as persistent declarative intent: normalize the desired declaration into the realized numbered form, preserve ND-assigned addresses and numbered-only fields, and continue comparing/updating the user-controlled description and interface-configuration fields.
- Treat preprovision as a one-time bootstrap instruction: explicitly document that automation must replace it with a numbered declaration after realization and explain how it acquires ND-assigned values.
We should discuss which lifecycle contract we want during our next standup call before deciding the exact implementation. Please keep this thread open until that behavior is agreed and documented.
There was a problem hiding this comment.
@mikewiebe Went with persistent intent. After realization the declaration still governs the interface description and config fields, so a later edit is a real change applied as a numbered PUT that preserves ND assigned addresses. An unchanged reapply stays idempotent. Is this ok?
| ) | ||
| return super().merge(other) | ||
|
|
||
| @model_validator(mode="before") |
There was a problem hiding this comment.
Medium: Policy-specific validation is caller-controlled and alias-sensitive
Issue
The internal policy_type_marker can be supplied through the free-form input
and is trusted over the outer policy_type. The injection logic also handles
an empty alias-form dictionary differently from an empty snake-case dictionary,
and userDefined does not enforce its required template_name.
The public policy_type should be the sole authority that selects the
policy-specific template_inputs model. policy_type_marker is an internal
Pydantic discriminator and should never let caller input select a different
schema.
Concrete behavior
-
A caller can bypass policy-specific validation. This input should be
rejected becauseunexpectedis not a validnumberedfield:config_data: policy_type: numbered template_inputs: policy_type_marker: userDefined unexpected: value
_inject_policy_marker()preserves the supplied marker, so Pydantic parses
the dictionary as the open-endedUserDefinedTemplateInputsmodel while
the outgoing link still declarespolicy_type: numbered. -
Equivalent supported aliases produce different results:
template_inputs: {}
fails validation, while:
templateInputs: {}
succeeds. The expression
data.get("template_inputs") or data.get("templateInputs")treats the first empty dictionary as false and
falls through toNone; when the alias is used, the second lookup returns
the empty dictionary. The discriminator is therefore injected only for one
spelling of the same logical input. -
A custom policy is accepted without identifying its custom template:
config_data: policy_type: userDefined template_inputs: custom_setting: value
The documentation and OpenAPI contract require
template_namefor
userDefined, buttemplate_nameis optional inLinkConfigDataModeland
no policy-dependent validator enforces it.
Evidence
plugins/module_utils/models/links/links.pyline 33
preserves a caller-supplied marker and usesorwhen selecting
template_inputs, so explicit snake-case{}becomesNone.plugins/module_utils/models/links/templates/user_defined.pyline 13
permits arbitrary extra values.tests/unit/module_utils/models/test_links.pyline 72
explicitly acceptsuserDefinedwithout thetemplate_namerequired by the
docs and OpenAPI.- A direct probe sent outer
policy_type: numberedwith
policy_type_marker: userDefined; it was accepted as
UserDefinedTemplateInputswith an otherwise invalidunexpectedkey. The
equivalent snake-casetemplate_inputs: {}path raisedValidationError.
Existing PR overlap
No matching existing PR comment found.
Existing open issue overlap
No matching open issue found.
Impact
Users can bypass policy-specific validation and send incompatible fields under
the wrong policyType. Equivalent documented snake-case and API-alias inputs
also have different outcomes, and malformed custom-template requests fail at
the controller instead of locally. This defeats the module's stated local
validation contract and replaces actionable input errors with later, less
specific controller failures.
Suggested fix
Derive and overwrite the internal discriminator exclusively from the outer
policy_type, or remove any caller-supplied marker before model selection.
Select template_inputs by key presence rather than truthiness, and validate
template_name when policy_type=userDefined. Add tests through
NDLinkModel.from_config() for conflicting markers, empty dictionaries under
both aliases, missing user-defined template names, and required
policy-specific fields.
There was a problem hiding this comment.
Done. The discriminator is derived only from policy_type, template_inputs is selected by key presence, and template_name is required for userDefined.
9cddbaa to
7ff0365
Compare
44bc963 to
9b569f1
Compare
allenrobel
left a comment
allenrobel
left a comment
There was a problem hiding this comment.
LGTM. One last straggler not worth holding the PR unapproved: The _NO_DEFAULT sentinel comment at plugins/module_utils/models/links/templates/base.py:26 still says "(omitted from the payload when unset)", but an unset field with no documented default is sent as a typed empty, not omitted. Could you fix that one-liner in your next push?
| class InterfaceBasicsMixin(BaseModel): | ||
| """Common interface level settings shared by most policy types.""" | ||
|
|
||
| interface_admin_state: bool | None = Field(default=None, alias="interfaceAdminState", json_schema_extra=pd(True)) |
There was a problem hiding this comment.
High: Policy-specific Pydantic models do not enforce the OpenAPI write contract
Issue
template_inputs is intentionally a generic dictionary in the Ansible argument
specification, which makes the selected policy-specific Pydantic model the
authoritative deep-validation boundary for user configuration. Those models
currently enforce basic Python types and reject properties belonging to a
different policy, but they do not enforce required fields, enum choices, numeric
bounds, or conditional requirements from OpenAPI.
For example, InterfaceBasicsMixin.mtu is an unconstrained int | None, while
speed and fec are unrestricted str | None. Required
vpcPeerKeepalive properties such as src_ip, dst_ip, and interface_vrf
are optional. apply_payload_defaults() then converts omitted fields without a
documented default into typed empty values after model validation.
Evidence
templates/base.pylines 38-41
declaremtuas an unconstrainedint | Noneandspeed/fecas
unrestrictedstr | Nonevalues.vpc_peer_keepalive.pylines 21-29
make the policy's addresses andinterface_vrfoptional.templates/base.pylines 160-186
fill fields without documented defaults with typed empty values after model
validation.
I reproduced the resulting behavior on ND 10.122.84.112 using exact PR head
369ceb45d9d64992fd0af84a15f36ce98a91b3ca in an isolated, undeployed
VXLAN_EVPN fabric:
-
A
numberedlink withmtu: 0passed check mode withchanged: true. -
The live request also succeeded, and
state: gatheredreturned the link
withmtu: 0. -
ND then refused to delete that link:
Template [int_routed_host] - Validation failed for following fields: [MTU] -
The link had to be repaired to
mtu: 9216before deletion succeeded.
An invalid enum demonstrates the other failure mode: speed: invalid-speed
passed check mode but failed only after reaching ND:
PolicyType [numbered] - Validation failed for following fields: [speed]
Existing PR overlap
No matching existing PR comment found. The earlier typed-empty/default thread
addresses values synthesized for omitted properties and was corrected by
sending documented defaults. This finding covers explicit invalid user values
and required per-policy constraints that still pass Pydantic validation and
check mode. The resolved policy-discriminator thread likewise does not cover
required fields, ranges, or enums.
Existing open issue overlap
No matching open issue found.
Impact
Check mode reports invalid intent as a viable change, some bad values fail only
after a controller API call, and others can be persisted into a link that ND
then refuses to delete. Users receive inconsistent late failures and may need
to repair invalid controller state before normal lifecycle operations work.
Suggested fix
Make the policy-specific Pydantic configuration models enforce the applicable
OpenAPI write contract, including required fields, enum choices, numeric bounds,
nullability, defaults, and conditional requirements. Invalid user intent should
fail before check mode proposes a change or any mutating API request is sent.
Controller-response parsing should remain tolerant so state: gathered can
still inspect legacy, forward-compatible, or already-invalid ND records. This
could use separate request/response models, validation context, or the existing
opaque-response fallback. Add invalid from_config tests for every advertised
policy, response-compatibility tests, and check-mode coverage.
There was a problem hiding this comment.
Medium: The shipped MPLS-underlay example fails local validation
Issue
The segment-routing example omits required mpls_fabric_type and uses
dci_routing_protocol: isis, while the model and OpenAPI accept is-is.
Evidence
plugins/modules/nd_manage_links.pylines 457-478
contains the invalid example.plugins/module_utils/models/links/templates/mpls_underlay.pylines 18-35
requires the fabric type and definesospf/is-isas the choices.- Running the documented inputs in check mode with valid live
fabric/switch/interface identities failed first for missing
mplsFabricType. Addingmpls_fabric_type: mplsSrthen failed the
dciRoutingProtocolchoice validation.
Existing PR overlap
This is a focused follow-up to the
Pydantic/OpenAPI thread.
The author replied “Done,” but the current exact head still contains both
defects in the shipped example.
Existing open issue overlap
#234 is the parent
feature issue only; it does not track this documentation defect.
Impact
A user copying the module's primary MPLS-SR example gets two consecutive local
validation failures before check mode can evaluate the intended link.
Suggested fix
Add mpls_fabric_type: mplsSr, change the protocol to is-is, and add a
documentation test that parses every EXAMPLES module call through the actual
argument spec and Pydantic model.
| ticket_id=ticket_id, | ||
| ) | ||
|
|
||
| if link_scope == "auto": |
There was a problem hiding this comment.
Medium: Default state: gathered crashes during scope auto-detection
Issue
The documented minimal state: gathered call fails when both config and
link_scope are omitted. Ansible supplies the optional config value as
None, while the default link_scope: auto path iterates it during strategy
selection.
Evidence
determine_strategy()lines 727-729
usesmodule.params.get("config", []); because the key exists with value
None, the list fallback is not used and the followingany(... for item in config)raises.- Live testing on 2026-07-20 used ND
10.122.84.112, fabric
pr338-manual-ibgp-0717, and exact PR head
369ceb45d9d64992fd0af84a15f36ce98a91b3ca. The one-task reproducer failed
withchanged: falseand:
Unexpected error: 'NoneType' object is not iterable
- The same live call succeeded with
changed: falsewhen only
link_scope: managewas added, confirming that authentication, fabric lookup,
and link gathering work and that the failure is isolated to auto-scope
selection.
Minimum reproducer
---
- name: Reproduce gathered auto-scope failure
hosts: nd
gather_facts: false
tasks:
- name: Gather links with config and link_scope omitted
cisco.nd.nd_manage_links:
fabric_name: example_fabric
state: gatheredThe integration gathered case forces link_scope: manage, bypassing the
default auto branch and therefore does not detect this failure.
Existing PR overlap
No matching existing PR comment found.
Existing open issue overlap
No matching open issue found. Broader issue
#340 tracks consistent
repository-wide state: gathered support and explicitly shows a no-config
call, but it does not record this PR-specific auto-scope exception or its
one-line repair.
Impact
The shortest documented way to gather links crashes before the module selects
a strategy or sends a controller request. Users must know to provide the
otherwise optional link_scope: manage workaround.
Suggested fix
Normalize an omitted configuration before iterating it, for example
config = module.params.get("config") or []. Add a module-wrapper test and an
integration task for state: gathered with both config and link_scope
omitted, plus a OneManage auto-detection control containing cluster fields.
There was a problem hiding this comment.
Thanks, the crash is fixed, but please add tests to exercise omitted config with default link_scope: auto
|
|
||
| def update(self, model_instance: NDLinkModel, **kwargs: Any) -> ResponseType: | ||
| """PUT /links/{linkId}; rejects cross policy updates (needs delete and recreate).""" | ||
| if self._is_policy_type_change(model_instance): |
There was a problem hiding this comment.
Medium: Check mode approves policy transitions that normal mode rejects
Check mode and normal mode do not apply the same policy-transition validation. An existing link cannot be changed from numbered to unnumbered with a PUT, but state: replaced in check mode reports that the request is valid and would change the controller.
The state machine skips the supplied operation in check mode. For state: replaced, it calculates a change and queues update() without otherwise validating the policy transition. The numbered-to-unnumbered guard is inside LinksOrchestrator.update(), which check mode never calls.
I reproduced this against PR head 369ceb45d9d64992fd0af84a15f36ce98a91b3ca on a live ND:
- Created a valid
numberedlink. - Requested the same physical link as
unnumberedusingstate: replacedand check mode. - Repeated the identical request in normal mode.
- Gathered the final controller state.
Observed result:
check_mode_changed: true
check_mode_failed: false
check_mode_proposed_policy: unnumbered
normal_mode_failed: true
normal_mode_message: Cannot change policy_type from 'numbered' to 'unnumbered' ...
final_controller_policy: numbered
The essential reproduction is:
- name: Request numbered to unnumbered replacement in check mode
cisco.nd.nd_manage_links: &transition
fabric_name: "{{ test_fabric }}"
link_scope: manage
output_level: info
state: replaced
config:
- src_fabric_name: "{{ test_fabric }}"
dst_fabric_name: "{{ test_fabric }}"
src_switch_ip: "{{ src_switch_ip }}"
dst_switch_ip: "{{ dst_switch_ip }}"
src_interface_name: Ethernet1/34
dst_interface_name: Ethernet1/34
config_data:
policy_type: unnumbered
template_inputs:
mtu: 9216
speed: auto
fec: auto
interface_admin_state: true
check_mode: true
register: check_transition
- name: Request the identical replacement in normal mode
cisco.nd.nd_manage_links:
<<: *transition
register: normal_transition
ignore_errors: trueThis means check mode can successfully approve a change that will certainly fail when applied, making it unreliable as a preflight gate for link replacements.
Move policy-transition validation into a shared preflight path that executes before _execute_operation() and therefore runs in both modes. Keep the validation in update() as defense in depth, and add paired normal/check-mode tests for merged, replaced, and overridden policy transitions.
There was a problem hiding this comment.
The policy transition guard moved into a preflight path the state machine runs in both modes, so check mode no longer approves a numbered to unnumbered change that normal mode rejects. The update guard stays as defense in depth.
| options=dict( | ||
| src_cluster_name=dict(type="str"), | ||
| dst_cluster_name=dict(type="str"), | ||
| src_fabric_name=dict(type="str"), |
There was a problem hiding this comment.
Medium: Required link identity fields are absent from the argspec contract
Issue
The public Ansible argument specification accepts a link item that does not
contain enough information for the module to identify a link. It requires one
switch selector for each endpoint, but leaves source and destination fabric and
interface names optional even though all four are mandatory parts of the
Manage-scope composite identity.
For example, this passes argument-spec and Pydantic model validation:
config:
- src_switch_name: leaf1
dst_switch_name: leaf2
config_data:
policy_type: unnumberedIt cannot identify a physical link because neither endpoint includes a fabric
or interface.
Evidence
links.pyline 419
declaressrc_fabric_name,dst_fabric_name,src_interface_name, and
dst_interface_namewithoutrequired: true.links.pylines 456-459
require only one source and destination switch selector.links.pylines 124-134
include all fabric, switch, and interface fields in the composite identity,
while the corresponding Pydantic fields remain optional.base.pylines 238-248
detects missing composite fields only when the runtime asks for the identifier.
An exact-head model probe with the configuration above produced:
model_validation=accepted
ValueError: Composite identifier fields ['src_fabric_name',
'dst_fabric_name', 'src_interface_name', 'dst_interface_name'] are None.
All required: ['src_fabric_name', 'dst_fabric_name', 'src_switch_name',
'dst_switch_name', 'src_interface_name', 'dst_interface_name']
The valid minimum shape is instead:
config:
- src_fabric_name: fabric1
dst_fabric_name: fabric1
src_switch_name: leaf1
dst_switch_name: leaf2
src_interface_name: Ethernet1/1
dst_interface_name: Ethernet1/1
config_data:
policy_type: unnumberedExisting PR overlap
No matching existing PR comment found. The earlier
switch-selector thread
covers conflicting name/IP/ID selectors and their priority. It does not cover
the missing fabric and interface fields required by every link identity.
Existing open issue overlap
No matching open issue found. Issue
#378 mentions composite
identifiers in the different context of port-channel name normalization and
does not cover nd_manage_links identity requiredness.
Impact
The module advertises incomplete input as structurally valid, then fails later
with an implementation-oriented composite-identifier exception. Check mode has
the same late failure. The defect normally occurs before a controller mutation,
so the primary risks are an inaccurate public contract, poor diagnostics, and
automation that cannot validate its input at the Ansible boundary.
Suggested fix
Mark src_fabric_name, dst_fabric_name, src_interface_name, and
dst_interface_name as required within every supplied config item. Keep the
top-level config option optional so state: gathered can still run without
configuration. For OneManage scope, validate the additional source and
destination cluster identity after link_scope is resolved. Add argspec and
module-wrapper tests proving incomplete items fail early with a field-specific
message for write and delete states, while no-config gathered remains valid.
Adds the nd_links module for ND 4.2 with manage / one_manage link scopes, template models, endpoints, orchestrator, strategies and fabric inventory support, plus integration tests.
…d policies on read, add iosXeNumbered, orientation independent identity, realized preprovision, and address remaining review feedback
…, identity, preflight, preprovision, and orientation Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
| cluster-name keys. | ||
| """ | ||
|
|
||
| identifiers: ClassVar[list[str] | None] = [ |
There was a problem hiding this comment.
High: Live duplicate endpoint records abort every state during initialization
Issue
The controller can return multiple link records with distinct linkId values but
the same endpoint tuple. The PR excludes linkId from identity, so
NDConfigCollection rejects the second record before any state can run.
Evidence
-
plugins/module_utils/models/links/links.pylines 128-138 and 259-276
defines identity only from the endpoint fields and canonicalizes orientation. -
plugins/module_utils/nd_config_collection.pylines 51-65
raises when that identity is already present. -
A read-only test against Nexus Dashboard 4.2 at
10.122.84.112, using fabric
Deeksha_test, returned the complete 12-record collection:
meta.counts.total=12andmeta.counts.remaining=0. Repeating the GET with
max=10000returned the same 12 records. -
The collection contained these two records with distinct controller IDs but
exactly the same raw endpoint orientation:linkIdSource Destination linkTypeLINK-UUID-2072900Deeksha_test / Deeksha_Switch / mgmt0nikhil-ibgp / switch / mgmt0ethislLINK-UUID-2074010Deeksha_test / Deeksha_Switch / mgmt0nikhil-ibgp / switch / mgmt0ethisl -
All 12 records lacked
configDataandpolicyType; this was the controller's
live link inventory rather than two duplicate declarations supplied to the
module. Exact-headstate: gatheredfailed while initializing
NDConfigCollection, before it could return the gathered records. -
A second live fabric with eight records reproduced the same collision and module
failure.
Minimum live reproduction
Inventory:
all:
hosts:
nd_112:
ansible_host: 10.122.84.112
ansible_connection: ansible.netcommon.httpapi
ansible_network_os: cisco.nd.nd
ansible_httpapi_use_ssl: true
ansible_httpapi_validate_certs: false
ansible_httpapi_login_domain: local
ansible_user: admin
ansible_password: "{{ lookup('env', 'ND_PASSWORD') }}"Playbook:
---
- name: Reproduce duplicate link identity failure
hosts: nd_112
gather_facts: false
tasks:
- name: Gather links
cisco.nd.nd_manage_links:
fabric_name: Deeksha_test
link_scope: manage
state: gatheredRun this against the exact PR head with ND_PASSWORD set. The task fails
read-only with:
Initialization failed: Item with identifier (...) already exists. Use replace() to update
Existing PR overlap
No matching existing PR comment found. The reversed-endpoint thread discusses two
orientations of one physical link, not two controller IDs with the same raw
orientation.
Existing open issue overlap
No matching open issue found. #234
is the broad Links feature issue but does not track this collision.
Impact
A valid controller inventory can make gathered, merged, replaced,
overridden, and deleted unusable because every state constructs the full
collection first. Even if collection construction were relaxed, the current
single-value link-ID cache would silently choose one of the colliding records.
Suggested fix
Preserve controller linkId as the unique read-side identity and maintain an
endpoint-identity multimap for intent matching. When an update or delete is
ambiguous, fail with a focused message that lists the number of matching records
without discarding the inventory. Add fixtures containing two distinct IDs for one
endpoint tuple and cover every state.
Add gathered_all state to the state machine for modules that retrieve all instances, mask_secrets helper for sensitive field redaction, and associated unit tests. Note: gathered-all framework cherry-picked from PR CiscoDevNet#312
5 participants
Adds the nd_links module for ND 4.2 with manage/one_manage link scopes, template models, endpoints, orchestrator, strategies and fabric inventory support, plus integration tests.
Related Issue(s)
Proposed Changes
Test Notes
Cisco Nexus Dashboard Version
Related ND API Resource Category
Checklist