Skip to content

feat: add NX-OS to controller registry for architecture detection - #895

Open
ChristopherJHart wants to merge 2 commits into
netascode:mainfrom
ChristopherJHart:feat/nxos-controller
Open

feat: add NX-OS to controller registry for architecture detection#895
ChristopherJHart wants to merge 2 commits into
netascode:mainfrom
ChristopherJHart:feat/nxos-controller

Conversation

@ChristopherJHart

Copy link
Copy Markdown
Contributor

Summary

  • Registers NX-OS as an architecture context in the controller registry
  • Adds "NXOS" to ControllerTypeKey literal type
  • Enables the orchestrator to detect and route NX-OS D2D tests when NXOS_URL is set

Motivation

NX-OS D2D (SSH) operational tests need the orchestrator to recognize NX-OS as a valid architecture context. This follows the same direct-device pattern as IOS-XE (requires only an environment variable for detection, no controller authentication).

Changes

File Change
nac_test/core/types.py Add "NXOS" to ControllerTypeKey
nac_test/utils/controller.py Add NXOS entry to CONTROLLER_REGISTRY

Test plan

  • Existing controller unit tests pass
  • Tested end-to-end with NX-OS D2D tests against live Nexus 9000 switches
  • No impact on existing SDWAN/ACI/IOSXE/FMC detection

🤖 Generated with Claude Code

🤖 AI Generation Metadata

  • AI Generated: Yes
  • AI Tool: claude-code
  • AI Model: claude-opus-4-6
  • AI Contribution: ~90%
  • AI Reason: controller registry addition
  • Human Oversight: Code reviewed and tested by Christopher Hart

@ChristopherJHart
ChristopherJHart force-pushed the feat/nxos-controller branch 3 times, most recently from 537d56e to 7ca616f Compare August 16, 2026 00:15

@oboehmer oboehmer left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR. Can you please align this implementation to the controller detection refactor branch feat/856-controller-resolution-refactor/PR #896 ? This PR still requires a 2nd review from Andrea, but I wouldn't expect major changes.

…tection

Registers NX-OS as an architecture context so the orchestrator can detect
and route NX-OS D2D (SSH) operational tests.

Aligned to the controller-resolution refactor (netascode#896): the registry now
lives in nac_test/core/controller.py and CredentialSet uses the
kind -> env var `fields` mapping rather than a positional `env_vars`
tuple.

Follows the IOS-XE direct-device pattern, with two deliberate
differences:

- Uses NXOS_HOST only. IOSXE carries both IOSXE_URL and IOSXE_HOST
  because IOSXE_URL is being phased out; there is no reason for a new
  architecture to inherit the retiring variable. url_env_var is set to
  NXOS_HOST so get_controller_url() resolves on the primary path.
- Device SSH credentials (NXOS_USERNAME/NXOS_PASSWORD) are part of the
  credential set, matching IOSXE. Detection requires all env vars in a
  set, so this makes credentials mandatory rather than URL-only.

NXOSTestBase is already mapped to "d2d" in BASE_CLASS_MAPPING, and NXOS
needs no entry in _get_auth_callable() - direct device access has no
controller to pre-flight against, same as IOSXE.

AI-Generated: yes
AI-Tool: claude-code
AI-Model: claude-opus-5
AI-Percent: 96
AI-Reason: controller registry addition + test sweep
@ChristopherJHart

Copy link
Copy Markdown
Contributor Author

Thanks for the review @oboehmer — I've force-pushed this branch, realigned to the controller-resolution refactor from #896 (now merged), and opened a companion PR for a doc inconsistency I found along the way.

Realigned to #896

Since #896 landed while this was in review, I rebuilt the change on current main rather than rebasing — the original commit targeted nac_test/utils/controller.py, which is now just an 18-line compatibility shim. Adapted to the new structure:

Was Now
registry in nac_test/utils/controller.py nac_test/core/controller.py
CredentialSet(env_vars=("NXOS_URL",)) CredentialSet(fields={"url": ..., "username": ..., "password": ...})
no insecure_env_var insecure_env_var="NXOS_INSECURE"
test in tests/unit/cli/validators/test_controller_auth.py tests/unit/core/test_controller_auth.py

Two changes from the original diff

1. NXOS_HOST only, no NXOS_URL. IOS-XE carries both because IOSXE_URL is being phased out in favour of IOSXE_HOST; there's no reason for a brand-new architecture to inherit the retiring variable. url_env_var is set to NXOS_HOST so get_controller_url() resolves on its primary path rather than falling through to the alternative-var loop.

2. Device credentials are now required for detection. The original diff had a URL-only credential set, which — since detection requires all env vars in a set to be present — meant NXOS_HOST alone satisfied detection. That was an inconsistency in my PR, not the established pattern: IOS-XE has always required IOSXE_USERNAME/IOSXE_PASSWORD. Now matched.

For anyone else reading: the # Direct device access, no controller credentials required comment on these entries means there's no controller (APIC/Manager) to authenticate against first — device SSH credentials are still needed. I've reworded it on the NXOS entry to avoid the ambiguity.

Companion PR: #936

While checking the naming, I found dev-docs/PRD_AND_ARCHITECTURE.md documents NX-OS Direct as using NXOS_SSH_USERNAME/NXOS_SSH_PASSWORD — names that appear nowhere in the codebase. #936 corrects that row to NXOS_USERNAME/NXOS_PASSWORD to match this PR and the IOS-XE precedent, and also refreshes the CONTROLLER_REGISTRY example in that doc, which still shows the pre-#896 env_vars=[...] form.

I left the ACI and NDFC rows on NXOS_SSH_* in that PR — they're equally unimplemented, but renaming them decides whether device credentials key off device family or controller context, which felt like a separate call. Flagged it there in case you'd rather settle it now.

Nothing else needed

  • NXOSTestBase is already mapped to "d2d" in BASE_CLASS_MAPPING
  • No _get_auth_callable() entry required — direct device access has no controller to pre-flight against, same as IOS-XE

Verification

  • pytest tests/unit — 1084 passed
  • ruff check / ruff format — clean
  • mypy nac_test/ — clean, 100 source files
  • Re-tested end-to-end against live Nexus 9000 switches

Diff is now 5 files, +55/−2. Ready for Andrea's second review whenever convenient.

@ChristopherJHart

Copy link
Copy Markdown
Contributor Author

Small correction to my comment above: the doc work is now split across two PRs rather than one.

I had bundled both into #936. They have different audiences — the env var name is a naming decision tied to this PR, while the example refresh is mechanical cleanup from #896 — so they're easier to review apart. Neither blocks this PR, and #937 doesn't depend on #895 merging.

@oboehmer oboehmer left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for aligning this to the new refactored code, @ChristopherJHart .. I can't look at your original changes, but it looks cleaner.. If you could make one more small change before merging then we reduce the maintenance burden even further (and add test coverage).

Comment thread tests/unit/core/test_controller_auth.py Outdated
Replace the static set of expected controller names with
get_args(ControllerTypeKey) so the test stays in sync automatically
when new controller types are added.

Addresses review feedback from oboehmer on PR netascode#895.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
AI-Generated: yes
AI-Tool: claude-code
AI-Model: claude-opus-4-6
AI-Percent: 27
AI-Reason: review feedback - derive expected controllers from type
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.

2 participants