Added validation for Gather filter supported properties - #423
Added validation for Gather filter supported properties#423jeetugangwar11 wants to merge 5 commits into
Conversation
allenrobel
left a comment
There was a problem hiding this comment.
Code review
Two inline comments below (one confirmed functional regression, one documentation-policy issue), plus one finding that lands on lines outside the diff:
- Stale "all fields optional" docstrings — the new
validate_gathered_propertiesvalidator rejects a gathered filter item with zero active properties, so "all fields optional" is no longer accurate for gathered state. The moduleDOCUMENTATIONwas updated with the new "at least one supported property" constraint, but the model's own docstrings still make the old claim in two places: the class docstring (query/gathered: all fields optional (used as filters).) and theapply_state_validationdocstring (all fields remain optional and serve as filters). Please update both to reflect the new constraint.
🤖 Generated with Claude Code
|
I have completed my review and have no additional findings. Once the outstanding review comments from @allenrobel are resolved, this PR is good to merge from my perspective. |
There was a problem hiding this comment.
LGTM
The TODO is in the wrong format for an ND workaround since the version is not specified. But we can fix that later. Suggested fix:
# TODO(4.2.1) resource-manager-gathered-entity-order-lucene
# ND echoes canonical endpoint order for multi-switch entity names, so exact Lucene filters on
# user-supplied entity_name can miss matches; gathered filtering matches entity names locally instead.|
@jeetugangwar11 when you get a chance later, below is the workaround note for the TODO(4.2.1) with the link back slug ( Thanks ---
id: resource-manager-gathered-entity-order-lucene
endpoints:
- GET /api/v1/manage/fabrics/{fabricName}/resources
- POST /api/v1/manage/fabrics/{fabricName}/resources
tags:
- deviation
found: 4.2.1
fixed:
status: workaround
severity: medium
guidance: "Do not filter resources server-side by entityName (Lucene `filter=entityName:...`): ND echoes multi-switch entity names (devicePair/link scopes) in canonical endpoint order, so an exact match against the user-supplied tilde-separated serial order can silently miss resources. Fetch broader (poolName/switchId query params + pagination) and match entityName locally, order-insensitively on the tilde-separated segments."
---
# Summary
For multi-switch resources (`scopeType: devicePair` and `link`), `entityName` is a tilde-joined composite of switch serials plus an entity suffix (spec example: `FDO222419B7~FDO222419B8~vpc55`). ND normalizes these names to a **canonical endpoint order** on read: the order of the serial segments echoed by `GET /api/v1/manage/fabrics/{fabricName}/resources` is ND's, not necessarily the order the client supplied at allocation time.
The endpoint's `filter` query parameter is a Lucene **exact-match** filter (the spec documents it only as "Lucene format filter" with no field list). An exact `filter=entityName:<user-supplied-name>` therefore silently excludes resources whose stored name differs from the query only by endpoint order — the resource exists and would match after order-insensitive normalization, but the server-side filter never returns it.
## Impact
- Any client that pushes `entityName` filtering to the server via Lucene gets **silently incomplete results** for devicePair/link resources — no error, just missing rows.
- Clients must instead over-fetch and match locally, which costs extra transfer/pagination on large fabrics. In `cisco.nd`'s `nd_manage_resource_manager` (`state: gathered`), this is why `entity_name` is a local-match-only filter while only `poolName` and `switchId` are pushed as GET query parameters.
- Single-switch scopes (`fabric`, `device`, `deviceInterface`) have no serial-order ambiguity in `entityName`, but the collection matches locally across the board for consistency.
## Workaround
### Full
Fetch with the documented first-class query parameters (`poolName`, `switchId`) plus `max`/`offset` pagination to bound the result set, then match `entityName` client-side, comparing tilde-separated segments order-insensitively. This is what `nd_manage_resource_manager` does (see `_build_gathered_resource_criteria` in `plugins/module_utils/manage_resource_manager/nd_manage_resource_manager_resources.py`; workaround marker `TODO(4.2.1) resource-manager-gathered-entity-order-lucene`).
## Reproduction
Not yet independently lab-reproduced. Sourced from `cisco.nd` resource-manager module development (jeetugangwar11, PRs [[#333](https://github.com/CiscoDevNet/ansible-nd/pull/333)](https://github.com/CiscoDevNet/ansible-nd/pull/333) and [[#423](https://github.com/CiscoDevNet/ansible-nd/pull/423)](https://github.com/CiscoDevNet/ansible-nd/pull/423)): the original implementation carried the in-code observation that "ND may return canonical endpoint order for multi-switch resources, and some simple entity-only Lucene filters can exclude resources that should match after local normalization", and the module documents "Entity name matching is order-insensitive for tilde-separated serial numbers" as a consequence.
To reproduce on the lab (SITE1, 192.168.7.7):
1. Allocate a devicePair resource (e.g. a `vpcId`) naming the entity with serials in a chosen order: `SERIAL-B~SERIAL-A~vpc10`.
2. `GET /api/v1/manage/fabrics/{fabricName}/resources` and inspect the echoed `entityName` — expect canonical (reordered) serials.
3. `GET /api/v1/manage/fabrics/{fabricName}/resources?filter=entityName:SERIAL-B~SERIAL-A~vpc10` — expect the resource to be absent from the filtered response while present unfiltered.
Update this section (and consider promoting `tags` to include `bug`) once the lab run confirms steps 2–3.
## Expected behavior
Either the read echoes `entityName` exactly as allocated, or the server-side `filter` matches entity names order-insensitively — so a client can round-trip the name it allocated into a Lucene filter and find the resource.
## Actual behavior
The stored/echoed `entityName` is canonicalized, and the Lucene filter is exact, so a round-tripped user-supplied name can silently match nothing.
## Related
- [[[vPC per-peer VLAN fields collapse on read](https://github.com/CiscoDevNet/ansible-nd/pull/vPC%20per-peer%20VLAN%20fields%20collapse%20on%20read)]] — same family: ND normalizes the echo of what was written. |
|
Hi @allenrobel, I have made the changes for TODO, could you please have a look and verify if this looks good. |
Proposed Changes
state: gatheredfilter properties:entity_namepool_nameswitchesresourcescope_typepool_typepool_nameandswitchesas GET API query parameters.config: [{}].configis omitted or specified as an empty list.Test Notes
The following validation was completed:
22 passed291 passedansible-test sanity --test validate-modules --venv plugins/modules/nd_manage_resource_manager.pysuccessfully.Cisco Nexus Dashboard Version
Cisco Nexus Dashboard 4.3x
Related ND API Resource Category
Resource category: Manage resource manager API (
/manage/api/v1).Checklist