From 07acbb6b37687eb562a549b6c33f2f9dcb956fb6 Mon Sep 17 00:00:00 2001 From: rzisholz Date: Thu, 3 Sep 2026 12:49:19 +0300 Subject: [PATCH 1/2] CP-25943: document the migration-precedence proof already covered by this test Test plan Track F, case F2 (migrating install: add serviceId while old ARK_USERNAME/ARK_SECRET creds are still present -> Conjur wins). This is already exercised by TestCyberArkClient_PostDataReadingsWithOptions_MockAPI -- it sets both env vars alongside a non-empty serviceID -- but nothing in the test documented that this combination is deliberate proof of precedence, not incidental. 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. F3 (old creds removed after cutover) is the existing Conjur-only test with no changes needed. F4 (both token shapes routed correctly across tenants) and F5 (un-onboarded agent fails closed, no fallback) are server-side (regional-resources branch-detection tests) and structurally guaranteed by selectAuthenticator's switch/case (no fallback branch exists once Conjur is chosen) respectively -- neither needs new agent-side test code. Verified: go test ./pkg/client/... -run TestCyberArkClient_PostDataReadingsWithOptions_MockAPI passes, with the log line confirming the precedence decision at runtime. --- pkg/client/client_cyberark_test.go | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/pkg/client/client_cyberark_test.go b/pkg/client/client_cyberark_test.go index 747742b9..73330dbe 100644 --- a/pkg/client/client_cyberark_test.go +++ b/pkg/client/client_cyberark_test.go @@ -27,6 +27,19 @@ import ( // dataupload code works with the mock CyberArk APIs. // The environment variables are chosen to match those expected by the mock // server. +// +// ARK_USERNAME/ARK_SECRET are set here deliberately, alongside a non-empty +// serviceID (test plan Track F, case F2 -- migrating install, add serviceId +// while old creds are still present). This is not incidental: it is the +// integration-level proof that selectAuthenticator's Conjur-wins precedence +// actually holds through the full NewCyberArk/PostDataReadingsWithOptions +// path, not just in the selectAuthenticator unit tests (auth_select_test.go). +// It works because FakeCyberArk's Identity.API is an unreachable +// `.invalid` host: if the username/password path were mistakenly chosen +// instead of Conjur, the login attempt would fail with a connection error +// and this test would fail. Do not remove these two lines as "unnecessary" -- +// doing so would silently delete this test's only coverage of migration +// precedence. func TestCyberArkClient_PostDataReadingsWithOptions_MockAPI(t *testing.T) { t.Setenv("ARK_SUBDOMAIN", servicediscovery.MockDiscoverySubdomain) t.Setenv("ARK_USERNAME", "test@example.com") From 0162d7160bb332fd2b2af974c9b41d225f0b9b3a Mon Sep 17 00:00:00 2001 From: rzisholz Date: Thu, 3 Sep 2026 15:38:30 +0300 Subject: [PATCH 2/2] Review: reconcile the two comments and trim to the point Per wallrj-cyberark's review on #828: my comment said the env vars were "not incidental" proof of precedence; the sibling comment 30 lines below said the same test "silently exercises Conjur regardless" of them -- read as a contradiction. Reconciled to one framing (ignored by design, this test proves it) and had the sibling point at it instead of restating. Also dropped the "test plan Track F, case F2" reference (meaningless in this public repo, not checkable or maintainable by a future reader) and compressed 13 lines down to 4, keeping only the genuinely non-obvious part (the .invalid host trick). --- pkg/client/client_cyberark_test.go | 25 +++++++------------------ 1 file changed, 7 insertions(+), 18 deletions(-) diff --git a/pkg/client/client_cyberark_test.go b/pkg/client/client_cyberark_test.go index 73330dbe..47fe7abf 100644 --- a/pkg/client/client_cyberark_test.go +++ b/pkg/client/client_cyberark_test.go @@ -28,18 +28,10 @@ import ( // The environment variables are chosen to match those expected by the mock // server. // -// ARK_USERNAME/ARK_SECRET are set here deliberately, alongside a non-empty -// serviceID (test plan Track F, case F2 -- migrating install, add serviceId -// while old creds are still present). This is not incidental: it is the -// integration-level proof that selectAuthenticator's Conjur-wins precedence -// actually holds through the full NewCyberArk/PostDataReadingsWithOptions -// path, not just in the selectAuthenticator unit tests (auth_select_test.go). -// It works because FakeCyberArk's Identity.API is an unreachable -// `.invalid` host: if the username/password path were mistakenly chosen -// instead of Conjur, the login attempt would fail with a connection error -// and this test would fail. Do not remove these two lines as "unnecessary" -- -// doing so would silently delete this test's only coverage of migration -// precedence. +// ARK_USERNAME/ARK_SECRET are set here on purpose alongside a non-empty +// serviceID: selectAuthenticator ignores them once serviceID is set, and +// FakeCyberArk's Identity.API is an unreachable `.invalid` host -- a +// mistaken fallback to username/password would fail this test. func TestCyberArkClient_PostDataReadingsWithOptions_MockAPI(t *testing.T) { t.Setenv("ARK_SUBDOMAIN", servicediscovery.MockDiscoverySubdomain) t.Setenv("ARK_USERNAME", "test@example.com") @@ -60,12 +52,9 @@ func TestCyberArkClient_PostDataReadingsWithOptions_MockAPI(t *testing.T) { } // TestCyberArkClient_PostDataReadingsWithOptions_UsernamePasswordMockAPI is -// the legacy-auth-path counterpart to the Conjur test above. Without it, the -// only integration-level test of NewCyberArk's username/password path is one -// that sets ARK_USERNAME/ARK_SECRET but then also passes a non-empty -// serviceID — which selectAuthenticator prioritises, so it silently exercises -// Conjur regardless of those env vars. This test leaves serviceID empty so -// the username/password path is what's actually under test. +// the legacy-auth-path counterpart to the Conjur test above (see its comment +// for why serviceID must stay empty here for this test to actually exercise +// the username/password path). func TestCyberArkClient_PostDataReadingsWithOptions_UsernamePasswordMockAPI(t *testing.T) { t.Setenv("ARK_SUBDOMAIN", servicediscovery.MockDiscoverySubdomain) httpClient, username, password := testutil.FakeCyberArkUsernamePassword(t)