feat(core): controller resolution refactor - single-source-of-truth

[oboehmer]

Description

Problem

Controller credentials were validated and resolved across multiple layers — CLI validators, orchestrators, PyATS subprocesses — with each layer independently reading os.environ, re-detecting the controller type, and making its own assumptions about credential shapes. This created:

• Multiple sources of truth: 3+ detection call sites per execution, each potentially disagreeing
• Drift risk: Adding a new auth method (e.g., SD-WAN JWT tokens in PR Add sdwan (20.18+) jwt token based auth support for pyats nac-test testcases #847) required touching every layer
• Scattered env var literals: SDWAN_URL, ACI_USERNAME, etc. hardcoded across both nac-test and nac-test-pyats-common
• No typed contract: Subprocesses re-derived controller identity from scratch instead of receiving it from the parent

Approach

This PR establishes nac_test/core/controller.py as the single source of truth for all controller metadata and provides a typed resolution flow that eliminates redundant detection.

Architecture (both repos)

CombinedOrchestrator
  └─ resolve_controller()              ← ONE detection call site
       └─ ControllerContext(type, auth_method)
            │
            ├─ preflight_auth_check(ctx)
            │
            └─ PyATSOrchestrator(controller_context=ctx)
                 └─ NAC_TEST_CONTROLLER_CONTEXT (env var, set at subprocess launch)
                      │
                      └─ NACTestBase.setup()
                           ├─ get_controller_context()    ← deserializes identity
                           ├─ get_connection_params()     ← resolves credentials by kind
                           ├─ get_controller_url()        ← reads URL from env
                           └─ should_verify_ssl()         ← reads SSL flag from env

nac-test (this PR) owns the SSOT registry and resolution primitives.
nac-test-pyats-common (#46) consumes them — auth adapters no longer read env vars directly for controller config.

Key design decisions

1. ControllerContext is identity, not state — It carries controller type + auth method selection. Credentials remain in env vars (already there, set by the caller). No duplication into another transport mechanism.

2. CredentialSet.fields — A Mapping[CredentialKind, str] that is the single source for "which env var backs which semantic kind". env_vars and kinds are derived properties that cannot drift. The order of controller env variables in the definition bears no meaning anymore (previously the controller URL had to be the first value in the list).

3. get_connection_params(controller_type, auth_method) — Generic resolver that returns {"url": "...", "username": "...", ...} keyed by semantic kind. Adapters never need to know env var names.

4. should_verify_ssl(controller_type) — Centralized SSL flag resolution from {PREFIX}_INSECURE env vars. Returns True when SSL should be verified.

5. URL normalization is an adapter concern — get_controller_url() returns the raw value (whitespace-stripped only). Each architecture-specific adapter owns rstrip("/") as needed. Integration tests enforce the trailing-slash contract.

Benefits

Before	After
3+ detection calls per execution	1 call in CombinedOrchestrator
Env var names scattered across 2 repos	All in CONTROLLER_REGISTRY
Each adapter reads its own {PREFIX}_INSECURE	should_verify_ssl("ACI") — one call
Auth method inferred per-adapter	Resolved once, passed as auth_method
Subprocess re-detects (21 env var lookups)	Deserializes typed context
Adding a new controller → touch 5+ files	Add one entry to CONTROLLER_REGISTRY

Closes

• Fixes Design smell (main): controller pre-flight env validation split; PR #847 increases drift risk #856
• Fixes Dead abstract method NACTestBase.get_connection_params(self) always raises NotImplementedError #904
• Fixes Delete orphaned nac_test/utils/environment.py (dead code post-EnvironmentValidator removal) #911
• Fixes Move AuthCache from pyats_core/common/ to core/ to restore layering #912
• Fixes Add end-to-end ControllerContext subprocess round-trip test #913
• Fixes Migrate tests/unit/core/test_controller_auth.py to pytest-mock's mocker fixture #916
• Fixes Adopt aci_context/sdwan_context/cc_context fixtures at remaining inline ControllerContext construction sites #919

Related Issue(s)

• Add sdwan (20.18+) jwt token based auth support for pyats nac-test testcases #847 (credential sets / multi-method support)
• nac-test-pyats-common#46 (consumer-side: auth adapters migrate to new API)

Type of Change

• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Refactoring / Technical debt (internal improvements with no user-facing changes)

Both packages published in lockstep.

Test Framework Affected

• PyATS
• Robot Framework
• Both
• N/A (not test-framework specific)

Network as Code (NaC) Architecture Affected

• All architectures

Key Changes

Core SSOT primitives (nac_test/core/)

• ControllerContext dataclass in core/types.py — identity-only, JSON serialization
• resolve_controller() → typed ResolutionError hierarchy (replaces ValueError)
• get_controller_context() — subprocess accessor (deserializes from env var)
• get_connection_params(controller_type, auth_method) → dict[CredentialKind, str]
• should_verify_ssl(controller_type, default=False) → bool
• CredentialSet.fields: Mapping[CredentialKind, str] replaces the old env_vars tuple

Orchestration wiring

• CombinedOrchestrator calls resolve_controller() once, passes context downstream
• PyATSOrchestrator accepts controller_context parameter; serializes at subprocess launch (not __init__)
• NACTestBase.setup() consumes get_controller_context() + get_connection_params() (fails fast)

Cleanup

• controller_auth.py: Consolidated URL lookup to use get_controller_url() from core
• EnvironmentValidator dissolved (redundant layer)
• is_architecture_active() and TerminalColors.format_env_var_error() removed (dead code)
• detect_controller_type() / get_matched_credential_set() deprecated with DeprecationWarning
• detect_controller_type removed from utils.__init__ re-exports
• Bridge shim at utils/controller.py retained for external consumers until Phase 3

Deprecation path

Function	Replacement	Removal
detect_controller_type()	resolve_controller()	Phase 3 (#897)
get_matched_credential_set()	get_controller_context().auth_method	Phase 3 (#897)
utils.controller shim	core.controller direct imports	Phase 3 (#897)

Testing Done

• 2066 tests pass (0 failures, 377 skipped)
• New tests: from_json() error paths, primary context path via NAC_TEST_CONTROLLER_CONTEXT, URL trailing-slash normalization contract (integration)
• Global autouse clean_controller_env fixture (registry-derived, xdist-safe)
• Pre-commit clean (ruff, mypy, bandit)

Opportunistic Fix (unrelated to #856)

Circular import: utils/cleanup.py → discovery/test_type_resolver → full pyats_core.discovery chain. Moved VALID_TEST_TYPES to pyats_core/constants.py.

Checklist

• Code follows project style guidelines (pre-commit run -a passes)
• Self-review of code completed
• Code is commented where necessary (especially complex logic)
• Documentation updated (dev-docs/PRD_AND_ARCHITECTURE.md)
• No new warnings introduced

[aitestino]

Hey @oboehmer, thank you for the PR — the SSOT direction is right, and the internal consolidation has genuinely landed. CONTROLLER_REGISTRY is the single source, CredentialSet.fields deriving env_vars/kinds closes the drift risk between those two, MappingProxyType on the frozen dataclass is the correct immutability pattern, and the ControllerContext-as-identity design (credentials stay in original env vars, only the identity crosses the subprocess boundary) is sound. The deprecation strategy on detect_controller_type()/get_matched_credential_set() is clean — call-time warnings.warn(stacklevel=2), and I verified the warning fires correctly through the utils.controller shim.

I did a thorough review. Most findings are finishing-pass or cross-repo migration items I'd rather track as issues. A handful are worth addressing before merge:

Things that need adjustment before merge:

1. HTML report gives wrong remediation for SDWAN token auth — this is the one real user-facing bug I found. Your banners.py::_credential_remediation_lines() in this PR correctly derives credential vars from CONTROLLER_REGISTRY, but the sibling HTML template at pyats_core/reporting/templates/auth_failure/report.html.j2:395-408, 449-454 still hardcodes _USERNAME/_PASSWORD for only ACI/SDWAN/CC. So an SDWAN token-auth user hitting a preflight failure sees export SDWAN_API_TOKEN=… in the terminal banner but export SDWAN_USERNAME= / SDWAN_PASSWORD=… in the HTML report — contradictory instructions for the same failure. MERAKI/FMC/ISE/IOSXE users get no guidance from that block at all. The fix is to expose _credential_remediation_lines() publicly and drive the template loops from CONTROLLER_REGISTRY the same way banners.py does.

2. ResolutionError doesn't inherit from NacTestError — CLAUDE.md convention. One-line change at core/controller.py:253; the three subclasses get it transitively. nac_test/exceptions.py has zero internal imports so there's no circular-import risk. Downstream except NacTestError handlers currently miss these.

3. NAC_TEST_CONTROLLER_CONTEXT env-var name repeated 4 times with no constant — core/controller.py:353, pyats_core/orchestrator.py:281, 369, 384. The precedent already exists: ENV_TEST_DIR: str = "NAC_TEST_TEST_DIR" at pyats_core/constants.py:124, same shape of problem (orchestrator writes, subprocess-side reads by env var name). Add ENV_CONTROLLER_CONTEXT to core/constants.py — it has to live in core since core/controller.py is the reader and core mustn't import from pyats_core.

4. AuthMethod isn't a real enum — auth_method: str in CredentialSet:56, ControllerContext:48, and get_connection_params:534, compared by == at line 573. The only two values that ever flow are "session" and "token". Since nac-test-pyats-common doesn't consume these APIs yet (grep confirms zero external callers of get_connection_params/get_controller_context), this is the cheapest possible moment to lock the enum in. A StrEnum would catch a typo like auth_method="Token" in a future CredentialSet at mypy instead of silently returning an empty candidates list three calls downstream.

5. Orphan clean_controller_env fixtures shadow the new global one — tests/integration/test_cli_aci_validation.py:29-40, 209-223 still have class-scoped autouse fixtures with the same name as the new global one you added. Pytest fixture resolution means these local copies override for those classes — and they use prefix in key (substring), miss the IOSXE_ prefix, don't clear NAC_TEST_CONTROLLER_CONTEXT, and don't clear _matched_credential_sets. Same consolidation pattern you fixed twice in this PR, just missed in these two spots.

Follow-up issues to track (none block the merge):

I have opened:

• #910 — Wire up should_verify_ssl() across nac-test-pyats-common auth adapters (currently the function exists but has zero production callers on our side; the four hardcoded os.environ.get("*_INSECURE", "True")... sites in aci/sdwan/catc/api_test_base bypass it). Also captures the caveat that the eventual fix isn't a drop-in not get_bool_env(...) — should_verify_ssl calls .strip() while get_bool_env doesn't. Softening the docstring so it doesn't read as "already replaced" would be good in the meantime.
• #911 — Delete nac_test/utils/environment.py (grepped across nac-test AND nac-test-pyats-common: check_required_vars and format_missing_vars_error have zero callers anywhere).
• #912 — Move AuthCache from pyats_core/common/ to core/ (core/controller_auth.py now imports it, which inverts the layering; AuthCache's own docstring says it's controller-agnostic).
• #913 — End-to-end ControllerContext subprocess round-trip test — the serialization side (orchestrator.py:281) and the deserialization side (base_test.py::setup()) are each tested in isolation but never chained. Extending the existing test_orchestrator_env_var.py capture to feed its own output into a real NACTestBase.setup() proves the round-trip without needing an actual OS subprocess.
• #914 — Add nac_test.utils.controller shim to tests/unit/test_compat_shims.py::SHIMS — the DeprecationWarning does fire, but there's no test that either re-exported symbol is still resolvable if core/controller.py renames one in Phase 3.
• #915 — Sanitize controller_url on display: banners.py:295, 332, 335 pass it verbatim to _wrap_url_lines, so ACI_URL=https://admin:pw@apic.local leaks credentials to terminal/CI logs/screenshots. Also AuthCheckResult.controller_url no longer trims trailing slashes (the old _get_controller_url did) — small cosmetic regression, worth locking down.
• #916 — Migrate tests/unit/core/test_controller_auth.py to mocker fixture (from unittest.mock import MagicMock, patch at line 9, 15 usage sites; brand-new file so no legacy excuse).
• #917 — Rewrite TestControllerEdgeCases (13 tests, lines 413-618) to exercise resolve_controller() — currently only calls the deprecated detect_controller_type(), so between now and Phase 3 those edge cases are unverified against the production code path.
• #918 — Add public reset_controller_cache() on core/controller — the new clean_controller_env fixture at tests/conftest.py:73 reaches into controller._matched_credential_sets.clear(). A public reset function keeps future refactors of that private dict from silently breaking test isolation.
• #919 — Adopt aci_context/sdwan_context/cc_context fixtures at the seven remaining inline ControllerContext(...) construction sites (also collapses four two-line field-by-field assertions into single dataclass equality checks).

PyATSTestDirs + pyats_test_dirs fixture is also duplicated verbatim between tests/pyats_core/conftest.py:18-65 and tests/unit/conftest.py:20-25, 126-139, but you already documented that in-file as tracked for #541 — happy to defer to that.

Design decisions I checked and agree with:

• dataclass over Pydantic for ControllerContext — right call for a 2-field IPC envelope; adding pydantic just for that would be over-engineering. One caveat worth doing regardless of container type: from_json currently accepts any string for controller_type/auth_method, so a corrupted env var or version-skew subprocess silently constructs an invalid ControllerContext that only fails much later at a bare KeyError in the registry. A get_args(ControllerTypeKey) check with a clear ValueError at the deserialization boundary is a couple of lines and prevents that. Happy to include this in the "before merge" list if you'd rather — I put it here because you might already be planning it under a different heading.
• Env-var handoff over stdin/file/IPC — right choice; pyats run job owns stdin, and config files add lifecycle overhead. Consistent with the existing DEVICE_INFO/MERGED_DATA_MODEL_TEST_VARIABLES_FILEPATH patterns.
• Bridge shim — thin, time-boxed, Phase-3 removal documented. Solid.

What do you think?

P.S. — This comment was drafted using voice-to-text via Claude Code. If the tone comes across as overly direct or terse, please know that's just how it tends to phrase things. No offense or criticism is intended — this is purely an objective technical review of the PR. Thanks for understanding! 🙂

[oboehmer]

Thanks for the thorough review! Addressed all 5 before-merge points plus several follow-ups in this push. Summary below.

Before-merge points

1. HTML report gives wrong remediation for SDWAN token auth
Fixed. The auth_failure template now derives credential remediation from CONTROLLER_REGISTRY via a new get_credential_vars() primitive in core/controller.py. The detection section no longer duplicates the error detail (which already shows all controllers with correct alternatives from _format_no_credentials_error()). The bad-credentials section correctly shows SDWAN_API_TOKEN for token auth. The grep verification pattern is also dynamically derived from registry keys. → 3d70583

2. ResolutionError does not inherit from NacTestError
Fixed. One-line change + import. Subclasses get it transitively. → 870afca

3. NAC_TEST_CONTROLLER_CONTEXT repeated with no constant
Fixed. ENV_CONTROLLER_CONTEXT added to core/constants.py, all 11 programmatic uses replaced. Docstrings left as human-readable text. → cd319b8

4. AuthMethod is not a real enum
Fixed. Introduced AuthMethod(str, Enum) with SESSION/TOKEN values. Uses str, Enum (not StrEnum) for Python 3.10 mypy compat. from_json() now validates via enum constructor — catches typos like "Token" at deserialization. No breaking change for consumers: AuthMethod.SESSION == "session" is True. JSON wire format unchanged. → 89cc192

5. Orphan clean_controller_env fixtures
Fixed. Removed 4 fixtures: 2 buggy orphans in test_cli_aci_validation.py (substring match, missing IOSXE_, no cache clear) + 2 redundant _clean_env wrappers in unit tests (global autouse already covers them). → ce80549

Follow-up issues addressed in this PR

Issue	Status	Commit
#911 — Delete orphaned utils/environment.py	✅ Closed	2aea719
#912 — Move AuthCache to core/	✅ Closed (re-export shim at old path for main-branch compat)	04935f6
#913 — Round-trip subprocess test	✅ Closed (3 parametrized cases: ACI/session, SDWAN/token, CC/session)	acf80b1
#916 — Migrate test_controller_auth.py to mocker	✅ Closed	7c4473c
#919 — Adopt context fixtures	✅ Closed (4 files, 8 inline sites replaced)	9f2a992

Follow-up issues deferred to Phase 3 (#897)

Comment added to #897 with the following items:

• #910 — should_verify_ssl() wiring (callers are in nac-test-pyats-common, not this repo)
• #914 — Shim regression test (shim goes away in Phase 3 anyway)
• #915 — URL credential sanitization (pre-existing condition, not introduced by this PR)
• #917 — Rewrite TestControllerEdgeCases to use resolve_controller() (Phase 3 scope)
• #918 — Public reset_controller_cache() API
• Auth cache re-export shim removal (pyats_core/common/auth_cache.py)

Hardening

Added from_json() validation at the deserialization boundary for both fields:

• controller_type: validated against get_args(ControllerTypeKey) — catches unknown types with clear ValueError
• auth_method: validated via AuthMethod enum constructor — catches typos immediately

→ 11b02ca + 89cc192

Design decisions

Agreed with all three assessments. On the from_json validation caveat — implemented as suggested (see hardening above).

Test results

1218 passed, 0 failures (1 pre-existing flaky broker integration test excluded).