Skip to content

fix(agent): resolve secrets_manager alongside identity_administration - #820

Open
roeezis wants to merge 3 commits into
jetstack:masterfrom
roeezis:split/03-servicediscovery-secretsmanager
Open

fix(agent): resolve secrets_manager alongside identity_administration#820
roeezis wants to merge 3 commits into
jetstack:masterfrom
roeezis:split/03-servicediscovery-secretsmanager

Conversation

@roeezis

@roeezis roeezis commented Aug 23, 2026

Copy link
Copy Markdown

Summary

Part 3 of the SMS/Conjur JWT authentication series (split out of #817). Stacked on #818, #819 — diff will shrink once those merge.

The Service Discovery API returns several independently-hosted services; the authn-jwt exchange this series adds is served by secrets_manager, a different host from identity_administration. Adds a SecretsManager field to Services and parses it, so callers have it available — nothing reads it yet, that lands in a later PR alongside the client that needs it.

Test plan

  • go build ./...
  • go test ./internal/cyberark/servicediscovery/... ./internal/envelope/...

@mladen-rusev-cyberark mladen-rusev-cyberark 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.

Correct and minimal. The assert.NotEqual(t, services.Identity.API, services.SecretsManager.API) assertion is the right one — it defends against the exact failure mode where a downstream mock points at whichever field the code reads, so tests pass and production 404s.

Approving in spirit; the one comment below is a docs request, not a change request.

Comment thread internal/cyberark/servicediscovery/discovery.go
@roeezis
roeezis force-pushed the split/03-servicediscovery-secretsmanager branch from 7808a1b to 95e3bd4 Compare August 24, 2026 11:42
rzisholz added 2 commits August 24, 2026 14:43
Introduces a small, isolated interface for reading a JWT from a file
path — the first piece of the upcoming Conjur JWT authentication path,
split out on its own since nothing else in this PR depends on it yet.
identity.go mixed the shared client/token-cache plumbing with the
CyberArk Identity username/password (UP) login flow. Move the UP-specific
code into username_password.go so the shared plumbing stays easy to find
once a second login mechanism (Conjur JWT) is added alongside it.

No behavior change — pure extraction, plus exporting the mock's success
credentials for other packages' tests.
@roeezis
roeezis force-pushed the split/03-servicediscovery-secretsmanager branch from 95e3bd4 to 0bd93b5 Compare August 24, 2026 11:45
@roeezis

roeezis commented Aug 24, 2026

Copy link
Copy Markdown
Author

All three addressed:

  • Added a comment above the identityAPI check making explicit that secretsManagerAPI/discoveryContextAPI are intentionally not required there, with a note that not every caller validates discoveryContextAPI yet (keyfetch doesn't).
  • Answered the discoveryContextAPI TODO in that same comment rather than leaving it as an open question.
  • Factored the three identical endpoint-selection loops into a mainActiveAPI helper.

Comment thread internal/cyberark/servicediscovery/discovery.go
The Service Discovery API returns several independently-hosted services;
the authn-jwt exchange this PR series adds is served by secrets_manager,
a different host from identity_administration. Add a SecretsManager
field to Services and parse it, so callers have it available — nothing
reads it yet, that lands in a later PR alongside the client that needs
it.

secrets_manager and discoveryContext are deliberately not required here
unlike identity, since not every caller needs them and requiring
secrets_manager would break every existing username/password install on
a tenant not yet onboarded to Conjur — each caller validates what it
needs at its own point of use instead. identity, by contrast, is
required unconditionally: it's present and active for every healthy
tenant, so callers may rely on it without re-checking.

Factor the repeated "find the first active main endpoint" loop into a
mainActiveAPI helper now that there are three near-identical copies.
@roeezis
roeezis force-pushed the split/03-servicediscovery-secretsmanager branch from 0bd93b5 to ee1907b Compare August 24, 2026 16:38
@roeezis

roeezis commented Aug 24, 2026

Copy link
Copy Markdown
Author

Added the note above identityAPI == "" per your first optional follow-on, and updated pkg/testutil/envtest.go:291's comment to cite it instead of reading like a workaround.

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