CP-26533: allow config.cyberark.subdomain in agent YAML by roeezis · Pull Request #838 · jetstack/jetstack-secure

roeezis · 2026-09-16T11:06:58Z

Summary

The CyberArk tenant subdomain is not a credential, but the agent could only read it via the ARK_SUBDOMAIN environment variable, bypassing its own YAML config entirely. This forced every install -- including Conjur-JWT-only installs that need no other credentials -- to provision a Secret just to carry one non-sensitive value.

It's a carry-over from before the Conjur JWT auth path existed, when the subdomain lived in the same Secret as the legacy username/password credentials because that Secret was the only config-delivery mechanism available at the time. The newer service_id/account/jwt_source fields were added to the proper YAML config struct; subdomain never was.

Adds config.cyberark.subdomain as a real config field. The environment variable remains a fallback for installs that still set it via Secret, so this is backward compatible. The chart's ARK_SUBDOMAIN Secret key becomes optional now that the config field can supply it instead.

Changes

pkg/agent/config.go: new CyberArkConfig.Subdomain YAML field
internal/cyberark/client.go: LoadClientConfigFromEnvironment takes an explicit subdomain, falling back to ARK_SUBDOMAIN when empty
pkg/client/client_cyberark.go: NewCyberArk threads the new subdomain param through
Chart: config.cyberark.subdomain value, rendered into config.yaml, ARK_SUBDOMAIN Secret key now optional
Regenerated chart docs/schema/snapshot tests

Test plan

make test-unit passes clean
make ark-verify (helm lint, kubeconform policy checks, chart unittest) passes clean
New test case covering config.cyberark.subdomain set instead of the env var

The CyberArk tenant subdomain is not a credential, but the agent could only read it via the ARK_SUBDOMAIN environment variable, bypassing its own YAML config entirely. This forced every install -- including Conjur-JWT-only installs that need no other credentials -- to provision a Secret just to carry one non-sensitive value. It's a carry-over from before the Conjur JWT auth path existed, when the subdomain lived in the same Secret as the legacy username/password credentials because that Secret was the only config-delivery mechanism available at the time. The newer service_id/account/jwt_source fields were added to the proper YAML config struct; subdomain never was. Add config.cyberark.subdomain as a real config field. The environment variable remains a fallback for installs that still set it via Secret, so this is backward compatible. The chart's ARK_SUBDOMAIN Secret key becomes optional now that the config field can supply it instead.

The four-case matrix (config only / env only / both / neither) is asserted against LoadClientConfigFromEnvironment, which returns an observable Subdomain. The ValidateAndCombineConfig tests can only assert the client type, so they cannot tell which source won.

Two sequential identical empty-checks become one first-non-empty expression, which is the precedence rule stated directly.

…test The prose quick-start (outside the auto-generated block) still told every install to create a Secret for the subdomain, undoing the point of this change. Also pin the subdomain-precedence test's actual resolved value, and add chart-test coverage for the new optional secretKeyRefs — nothing previously asserted on deployment.yaml at all.

wallrj-cyberark

No correctness bugs. The change is narrow and the generated files are in sync. Five low-severity points, four inline.

Not inline, because the file is not in this diff: hack/ark/test-e2e.sh still hard-requires ARK_SUBDOMAIN and writes it into the Secret. So the headline claim of this PR — that a Conjur-JWT-only install needs no Secret — is never exercised against a live cluster. The script also requires ARK_USERNAME/ARK_SECRET, so only the legacy path runs; that part is pre-existing, not yours. Worth a follow-up issue.

I added the test-ark and test-e2e labels and re-opened the PR to make the suites actually start. Note the green ark-test-e2e does not cover the no-Secret path, for the reason above.

What I checked, and how

Traced every caller of the two changed signatures. LoadClientConfigFromEnvironment and NewCyberArk each have exactly one non-test production caller, both updated. Nothing else in the tree reads ARK_SUBDOMAIN or builds a cyberark.ClientConfig outside tests.
go test ./internal/cyberark/ passes, including the four --machine-hub subtests in Test_ValidateAndCombineConfig. The only failures in pkg/agent and pkg/client are the pre-existing KUBEBUILDER_ASSETS skips.
helm unittest ./deploy/charts/disco-agent passes. I confirmed the new deployment_test.yaml is not vacuous by flipping an expected value: it fails as it should.
Ran make ark-generate on the PR head. Zero drift: the regenerated README.md and values.schema.json match what is committed, including alphabetical $defs placement.
pkg/agent/config.go uses plain yaml.Unmarshal, not strict. The new always-present subdomain: "" key will not break an older pinned agent image.
Checked the other two charts (venafi-kubernetes-agent, discovery-agent). Neither renders cyberark config, so nothing was missed there.
res.CyberArk is a value type, not a pointer, so cfg.CyberArk.Subdomain at the call site cannot nil-deref.
optional: true on a secretKeyRef covers a missing Secret as well as a missing key, so the "no Secret needed" claim is sound.

wallrj-cyberark · 2026-09-16T14:44:35Z

The test-e2e failure is not yours — ignore it.

I dispatched the same suite against master (run 35108371622) and it fails identically:

Timed out after 300s waiting for certificate venafi-kubernetes-agent-e2e.<uuid> to appear in the Venafi inventory
make: *** [make/02_mod.mk:53: test-e2e-gke] Error 1

In both runs the agent itself is healthy: it acquires its JWT and OAuth token and logs Data sent successfully with 36 data readings. The certificate then never appears in the inventory. That is a backend condition, and it is the exact failure mode #835 was written to surface rather than hang on.

ark-test-e2e, ngts-test-e2e, test and verify are all green on master, and ark-test-e2e, test and verify are green here.

…ning, rename, chart test - README: $ARK_SUBDOMAIN was only exported inside the skippable Secret section, so the Conjur-JWT-only walkthrough broke its own deploy command. Moved the export to its own step before either path forks. - LoadClientConfig now warns when both config.cyberark.subdomain and ARK_SUBDOMAIN are set, matching selectAuthenticator's existing dual-source convention for the service_id/username-password case. Threaded logr.Logger through NewCyberArk from the one caller that already has it. - Renamed LoadClientConfigFromEnvironment -> LoadClientConfig: the old name said the opposite of the contract once the parameter started winning over the environment variable. - Added a chart test setting a non-empty config.cyberark.subdomain; every existing case only covered the empty default.

wallrj-cyberark · 2026-09-17T11:45:30Z

All four review points are addressed in 12ee476. I have verified each one against ed4296b rather than taking the replies on trust, and I have no further findings.

The test-e2e failure is still the pre-existing master breakage, not this PR. Same signature as before: Timed out after 300s waiting for certificate ... to appear in the Venafi inventory. ark-test-e2e, test and verify are all green.

One thing this PR does not close, which I raised earlier and is worth a follow-up issue rather than more work here: hack/ark/test-e2e.sh still hard-requires ARK_SUBDOMAIN and writes it into the Secret. So the Conjur-JWT-only install, the case this PR exists to enable, is still never exercised against a live cluster.

What I ran to verify the four fixes

go build ./... passes on the new signatures.
go test ./internal/cyberark/... passes, all packages. The only failure anywhere is the pre-existing KUBEBUILDER_ASSETS skip in pkg/client, which is unrelated.
TestLoadClientConfig_SubdomainPrecedence runs all four subtests, none skipped.
I removed the log.Info call locally and re-ran that test. It fails, so the dual-source warning assertion is real rather than incidental.
The snapshot for the new custom-subdomain case renders subdomain: "tlskp-test", so the non-default value is pinned.
Read the regenerated README top to bottom. export ARK_SUBDOMAIN= now precedes both authentication paths, so neither walkthrough depends on a variable the reader was told to skip.
ClientConfigLoader is still used by pkg/client/client_cyberark.go on both master and this branch, so the rename left nothing stranded.

roeezis · 2026-09-17T11:56:09Z

Confirmed: hack/ark/test-e2e.sh:23 hard-requires ARK_SUBDOMAIN and lines 61/65 always write it into the Secret, never exercising --set config.cyberark.subdomain. Agreed this is a follow-up, not scope for this PR — will open an issue to track adapting that script to the Secret-free path.

roeezis · 2026-09-17T12:03:24Z

Opened #839 to track adding a live e2e path for the Conjur-JWT-only, Secret-free install. Not touching hack/ark/test-e2e.sh's existing legacy path — that stays as-is for backward compatibility, this would be a second path alongside it.

wallrj-cyberark

Approved. All four points from my earlier review are fixed in 12ee476, and I verified each one rather than taking the replies on trust — details are in the comment above.

The failing test-e2e is the pre-existing master breakage, not this PR, and there are no required status checks on master, so it does not block the merge.

One non-blocking note on the test logger, for next time rather than for this PR.

wallrj-cyberark · 2026-09-17T14:36:21Z

+// capturingSink is a minimal logr.LogSink that records Info() messages, used
+// to assert the dual-source warning actually fires rather than just trusting
+// the code path was reached.
+type capturingSink struct {


Non-blocking, and please do not change it in this PR — I am approving as is.

For next time: k8s.io/klog/v2/ktesting already does this, so the hand-rolled sink and its six methods are not needed. auth_select_test.go in this same package already uses ktesting.NewLogger.

One catch — buffering is off by default, so ktesting.DefaultConfig captures nothing. You need BufferLogs:

logger := ktesting.NewLogger(t, ktesting.NewConfig(ktesting.BufferLogs(true))) cfg, err := cyberark.LoadClientConfig(logger, "from-config") require.NoError(t, err) underlier, ok := logger.GetSink().(ktesting.Underlier) require.True(t, ok) assert.Contains(t, underlier.GetBuffer().String(), "both config.cyberark.subdomain and ARK_SUBDOMAIN are set")

GetBuffer().Data() gives you the structured entries if you would rather assert on fields than on text.

roeezis requested review from George-Yanev, achuchev and wallrj-cyberark September 16, 2026 11:39

roeezis force-pushed the add-cyberark-subdomain-config branch from 398c158 to 93d4240 Compare September 16, 2026 11:50

rzisholz added 3 commits September 16, 2026 15:50

CP-26533: collapse the subdomain fallback into cmp.Or

f650d94

Two sequential identical empty-checks become one first-non-empty expression, which is the precedence rule stated directly.

wallrj-cyberark added test-ark test-e2e To signal e2e test job to be run labels Sep 16, 2026

wallrj-cyberark closed this Sep 16, 2026

wallrj-cyberark reopened this Sep 16, 2026

wallrj-cyberark reviewed Sep 16, 2026

View reviewed changes

Comment thread deploy/charts/disco-agent/README.md

Comment thread internal/cyberark/client.go

Comment thread internal/cyberark/client.go Outdated

Comment thread deploy/charts/disco-agent/tests/__snapshot__/configmap_test.yaml.snap

rzisholz added 2 commits September 17, 2026 13:08

CP-26533: gofmt the new capturingSink method alignment

ed4296b

roeezis mentioned this pull request Sep 17, 2026

Add a live e2e test for the Conjur-JWT-only, Secret-free install path #839

Open

roeezis mentioned this pull request Sep 17, 2026

CP-26533: add a local e2e script for Conjur-JWT-only installs #840

Open

wallrj-cyberark approved these changes Sep 17, 2026

View reviewed changes

wallrj-cyberark merged commit 2b69e98 into master Sep 17, 2026
4 of 5 checks passed

wallrj-cyberark deleted the add-cyberark-subdomain-config branch September 17, 2026 14:37

Navigation Menu

CP-26533: allow config.cyberark.subdomain in agent YAML - #838

CP-26533: allow config.cyberark.subdomain in agent YAML#838
wallrj-cyberark merged 6 commits into
masterfrom
add-cyberark-subdomain-config

roeezis commented Sep 16, 2026

Uh oh!

wallrj-cyberark left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wallrj-cyberark commented Sep 16, 2026

Uh oh!

wallrj-cyberark commented Sep 17, 2026

Uh oh!

roeezis commented Sep 17, 2026

Uh oh!

roeezis commented Sep 17, 2026

Uh oh!

wallrj-cyberark left a comment

Uh oh!

wallrj-cyberark Sep 17, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

roeezis commented Sep 16, 2026

Summary

Changes

Test plan

Uh oh!

wallrj-cyberark left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wallrj-cyberark commented Sep 16, 2026

Uh oh!

wallrj-cyberark commented Sep 17, 2026

Uh oh!

roeezis commented Sep 17, 2026

Uh oh!

roeezis commented Sep 17, 2026

Uh oh!

wallrj-cyberark left a comment

Choose a reason for hiding this comment

Uh oh!

wallrj-cyberark Sep 17, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants