Skip to content

CP-25864: fall back cluster_name to cyberark.service_id under Conjur JWT - #827

Open
roeezis wants to merge 4 commits into
jetstack:masterfrom
roeezis:CP-25864-cluster-name-fallback
Open

CP-25864: fall back cluster_name to cyberark.service_id under Conjur JWT#827
roeezis wants to merge 4 commits into
jetstack:masterfrom
roeezis:CP-25864-cluster-name-fallback

Conversation

@roeezis

@roeezis roeezis commented Sep 2, 2026

Copy link
Copy Markdown

Summary

  • MachineHub cluster_name fallback chain (cluster_name -> cluster_id -> ARK_USERNAME -> empty) always resolves empty for Conjur JWT installs: the disco-agent chart never sets cluster_id, and ARK_USERNAME doesn't exist under JWT auth.
  • Adds cyberark.service_id (the Conjur authn-jwt service ID, always set by the CyberArk onboarding wizard) as a final fallback before empty.
  • Combined with the existing config-validation gate (requires service_id or ARK_USERNAME+ARK_SECRET), an empty ClusterName is now unreachable for any MachineHub config that passes validation.

Test plan

  • go test ./pkg/agent/... (excluding the pre-existing envtest-gated Test_ValidateAndCombineConfig_VenafiConnection, unrelated to this change)
  • Updated 2 existing tests whose expected behavior changed, added 1 new fallback test, removed 1 now-unreachable "empty" case with explanatory comment

🤖 Generated with Claude Code

The MachineHub cluster_name fallback chain (cluster_name -> cluster_id ->
ARK_USERNAME -> empty) always resolves empty for wizard-deployed agents:
the disco-agent chart never sets cluster_id, and ARK_USERNAME doesn't
exist under Conjur JWT auth. Add cyberark.service_id (the Conjur
authn-jwt service ID, always set by the onboarding wizard) as a final
fallback before empty.
Comment thread pkg/agent/config.go Outdated
Comment thread pkg/agent/config.go
Comment thread pkg/agent/config_test.go Outdated
Comment thread pkg/agent/config.go
rzisholz added 2 commits September 2, 2026 22:32
Replace the "empty ClusterName is unreachable" prose comment with a test
that asserts the config-validation gate directly, so a future auth-method
change that reopens the empty-cluster_name path fails this test instead
of silently regressing.

Fix values.yaml's stale clusterName fallback description (claimed a
service-account-name fallback that was never implemented in Go) and
regenerate README.md/values.schema.json via `make ark-generate`.
values.yaml's serviceId comment still said "configured for this tenant"
with a "disco-agent" worked example two lines above where this PR's
clusterName fallback comment now says it's used as the cluster name -
the same generated file asserted both "per-tenant" and "becomes the
cluster's display name" for the same field. Correct it to per-cluster,
matching README.md's own hand-written prose, and drop the non-unique
example.

Also log which fallback fired for cluster_name so an operator who sees
a surprising name (e.g. from a reused service_id) can tell where it
came from.
@roeezis

roeezis commented Sep 3, 2026

Copy link
Copy Markdown
Author

All threads resolved, checks green. Let me know if there's anything else you'd like changed before approving — happy to address further.

@wallrj-cyberark wallrj-cyberark left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Approving. The fallback logic is correctly placed, the behaviour is now covered by a test that asserts the validation gate rather than the absence of a value, and the two questions I could not settle from this repository have been answered with evidence from the systems concerned. Thank you for chasing both down rather than waving them through.

Verified locally at 62a26ad: go build ./... is clean and all fourteen subtests of TestConfig_CyberArk_Validation pass. The only failures are the pre-existing KUBEBUILDER_ASSETS envtests, which fail identically on master and are called out in the description.

One documentation point remains, and I am explicitly not blocking on it. README.md:100 is still headed "Per-tenant Conjur onboarding" and the deploy snippet at README.md:125 still passes --set config.cyberark.serviceId=disco-agent, which is the same shared literal removed from the generated comment in 62a26ad for not being unique. As it stands the file asks an operator to paste a fixed value in one place and warns that the value must be cluster-unique in another. Worth correcting the heading, the "tenant must be onboarded" sentence and the literal in the snippet, either before merge or as a follow-up — your call.

The backend overwrite behaviour discussed in the second thread is a pre-existing gap affecting every client, not something this change introduces, and is better tracked separately as you suggested.

"Per-tenant Conjur onboarding" and the worked example's literal
--set config.cyberark.serviceId=disco-agent contradicted the
per-cluster-unique framing this PR already established in the
generated values comment. Retitle the section, correct the onboarding
sentence, and replace the shared literal with a placeholder variable.
@roeezis

roeezis commented Sep 3, 2026

Copy link
Copy Markdown
Author

Fixed in c876edf — retitled to "Per-cluster Conjur onboarding", corrected the onboarding sentence, and replaced the shared disco-agent literal in the worked example with a placeholder $SERVICE_ID variable, with a note that it's cluster-specific.

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