CP-25943: document the migration-precedence proof already covered by this test

[roeezis]

Summary

• Test plan Track F, case F2 (migrating install: add serviceId while old ARK_USERNAME/ARK_SECRET creds are still present -> Conjur wins) is already exercised by TestCyberArkClient_PostDataReadingsWithOptions_MockAPI -- it sets both env vars alongside a non-empty serviceID -- but nothing documented that this combination is deliberate proof of precedence, not incidental copy-paste. Adds a doc comment making that explicit.
• Mechanism: FakeCyberArk's Identity.API is an unreachable .invalid host, so if the username/password path were mistakenly chosen instead of Conjur, this test would fail with a connection error rather than silently passing against the wrong path.
• F3 (old creds removed after cutover) is the existing Conjur-only test, no changes needed. F4 (both token shapes routed correctly across tenants) is server-side (regional-resources branch-detection tests, already covered there). F5 (un-onboarded agent fails closed, no silent fallback) is structurally guaranteed by selectAuthenticator's switch/case -- no fallback branch exists once Conjur is chosen. Neither F4 nor F5 needs new agent-side test code.

Jira: CP-25943 (subtask of CP-25824, under epic CP-24571)

Test plan

• go test ./pkg/client/... -run TestCyberArkClient_PostDataReadingsWithOptions_MockAPI -v passes; log line confirms the precedence decision at runtime ("both Conjur service_id and ARK_USERNAME/ARK_SECRET are set; using the Conjur JWT exchange and ignoring the username/password credentials")
• go test ./pkg/client/... ./internal/cyberark/... -- no new failures (2 pre-existing unrelated failures confirmed present on unmodified master too: KUBEBUILDER_ASSETS env requirement, and a known Go jsonv2 error-string drift)

🤖 Generated with Claude Code

[wallrj-cyberark]

Approving in substance — I verified the mechanism this comment claims, and it holds. Two wording points below, neither blocking.

I checked the load-bearing claim empirically rather than by reading. I copied the test, set serviceID to empty so selectAuthenticator picks username/password, and ran it against the same FakeCyberArk fixture. It fails exactly as you describe.

Counterfactual run: forcing the username/password path against FakeCyberArk does fail

COUNTERFACTUAL RESULT err=while initializing data upload client:
CyberArk Identity username/password login failed:
failed to perform HTTP request to start authentication:
Post "https://identity.example.invalid/Security/StartAuthentication":
dial tcp: lookup identity.example.invalid on 127.0.0.53:53: no such host

MockDiscoveryServer returns server.Client(), a real client with a real transport, so nothing rewrites the host and .invalid reaches the resolver. The guarantee is genuine, not an artefact of the mock.

I also confirmed the tests pass and the build is clean at 07acbb6, and I removed my scratch test afterwards.

One suggestion on top of the two comments. The strongest argument for "do not remove these two lines" is not in the comment: 3e753e2 records that CI sets ARK_USERNAME and ARK_SECRET at job level from real secrets, for the ARK_LIVE_TEST live test. So without these t.Setenv calls the test would read ambient real credentials in CI and nothing locally, and would behave differently in the two places. That is a concrete past incident in this repo, and citing it would carry more weight than the instruction not to remove the lines.

[roeezis]

Fixed in 0162d71 — reconciled the two comments (one framing: ignored by design, this test proves it) and trimmed 13 lines to 4, dropping the internal test-plan reference.