Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
roeezis wants to merge 2 commits into
base: master
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3135c84
CP-25864: fall back cluster_name to cyberark.service_id under Conjur JWT
Sep 2, 2026
777a67c
CP-25864: address review — assertive test, docs correction
Sep 2, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion deploy/charts/disco-agent/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -401,7 +401,7 @@ Example: excludeAnnotationKeysRegex: ['^kapp\.k14s\.io/original.*']

A human readable name for the cluster where the agent is deployed (optional).

This cluster name will be associated with the data that the agent uploads to the Discovery and Context service. If empty (the default), the service account name will be used instead.
This cluster name will be associated with the data that the agent uploads to the Discovery and Context service. If empty (the default) and using the legacy username/password authentication, ARK_USERNAME is used instead. If empty and using Conjur JWT authentication (no ARK_USERNAME), the cyberark.serviceId below is used instead. There is no service-account-name fallback — set this explicitly for a meaningful cluster name.
#### **config.clusterDescription** ~ `string`
> Default value:
> ```yaml
Expand Down
2 changes: 1 addition & 1 deletion deploy/charts/disco-agent/values.schema.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@
},
"helm-values.config.clusterName": {
"default": "",
"description": "A human readable name for the cluster where the agent is deployed (optional).\n\nThis cluster name will be associated with the data that the agent uploads to the Discovery and Context service. If empty (the default), the service account name will be used instead.",
"description": "A human readable name for the cluster where the agent is deployed (optional).\n\nThis cluster name will be associated with the data that the agent uploads to the Discovery and Context service. If empty (the default) and using the legacy username/password authentication, ARK_USERNAME is used instead. If empty and using Conjur JWT authentication (no ARK_USERNAME), the cyberark.serviceId below is used instead. There is no service-account-name fallback — set this explicitly for a meaningful cluster name.",
"type": "string"
},
"helm-values.config.cyberark": {
Expand Down
7 changes: 5 additions & 2 deletions deploy/charts/disco-agent/values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,8 +183,11 @@ config:
# A human readable name for the cluster where the agent is deployed (optional).
#
# This cluster name will be associated with the data that the agent uploads to
# the Discovery and Context service. If empty (the default), the service
# account name will be used instead.
# the Discovery and Context service. If empty (the default) and using the
# legacy username/password authentication, ARK_USERNAME is used instead. If
# empty and using Conjur JWT authentication (no ARK_USERNAME), the
# cyberark.serviceId below is used instead. There is no service-account-name
# fallback — set this explicitly for a meaningful cluster name.
clusterName: ""

# A short description of the cluster where the agent is deployed (optional).
Expand Down
8 changes: 8 additions & 0 deletions pkg/agent/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -793,6 +793,14 @@ func ValidateAndCombineConfig(log logr.Logger, cfg Config, flags AgentCmdFlags)
clusterName = arkUsername
}
}
if clusterName == "" && cfg.CyberArk != nil && cfg.CyberArk.ServiceID != "" {
Comment thread
roeezis marked this conversation as resolved.
// Conjur JWT installs have no ARK_USERNAME to fall back to.
// cyberark.service_id (the Conjur authn-jwt service ID,
// typically the cluster UUID) is always set for these

@wallrj-cyberark wallrj-cyberark Sep 2, 2026

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.

typically the cluster UUID does not match what this repository tells users about service_id.

  • deploy/charts/disco-agent/values.yaml describes config.cyberark.serviceId as "The Conjur authn-jwt authenticator service ID configured for this tenant".
  • The README section explaining it is titled "Per-tenant Conjur onboarding".
  • The worked helm upgrade example in that README passes the literal --set config.cyberark.serviceId=disco-agent.
  • internal/cyberark/client.go:50 hedges the other way: // authn-jwt service id (POC: per-cluster, e.g. "dev-cluster").

The genuine per-cluster UUID is already collected elsewhere: Snapshot.ClusterID, sourced from the kube-system namespace UID in pkg/datagatherer/k8sdiscovery/discovery.go.

Concrete failure: a tenant onboards one authn-jwt authenticator and installs the agent on ten clusters with the wizard's Helm command, which omits cluster_name. All ten upload cluster_name: "disco-agent". In Discovery and Context they become ten identically named clusters. That is arguably worse than the current empty name, because a name that looks deliberately chosen gives the operator no signal that a fallback fired, whereas an unnamed cluster does.

If the wizard genuinely mints a per-cluster service ID, please say so here and cite it, because the chart documentation currently says the opposite and one of the two needs correcting.

@roeezis roeezis Sep 2, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Verified against the actual wizard code (not just the chart README), since this determines whether the fallback is safe:

  • RegisterKubernetesAgentWizard.tsx:79: --set config.cyberark.serviceId=${wizardData.clusterUuid ?? ''}
  • useRegisterCluster.ts:14-17,52-53: every Conjur identity built from that same clusterUuid — conjur/authn-jwt/${clusterUuid}/apps, data/.../${clusterUuid}/workloads/... — a distinct authn-jwt authenticator and policy branch per cluster registration, not one shared per tenant.

So two clusters onboarded by the same tenant get different clusterUuid values and thus different serviceIds — the wizard flow this fallback exists for is genuinely per-cluster. The chart README's --set config.cyberark.serviceId=disco-agent is a placeholder literal for the generic manual-deploy path, unrelated to what the wizard generates — but you're right that it reads as per-tenant, and that's exactly the kind of thing an operator following the manual path would misread. I've updated the clusterName doc in a follow-up commit to stop implying a fallback that doesn't exist; happy to also clarify the README's per-tenant wording in a separate PR if you'd like, since it's pre-existing and out of scope here.

One caveat I can't verify from code: clusterUuid is operator-typed free text (validated only by validateUuid()), not auto-derived — so per-cluster-uniqueness holds structurally, but depends on the operator actually entering distinct values per cluster. Not a code defect, just worth knowing.

// installs, so it's a better last resort than an empty name.
log.Info("Using cyberark.service_id as cluster name because cluster_name, cluster_id, and ARK_USERNAME are all unset; prefer setting cluster_name explicitly", "clusterName", cfg.CyberArk.ServiceID)
clusterName = cfg.CyberArk.ServiceID

@wallrj-cyberark wallrj-cyberark Sep 2, 2026

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.

Worth confirming against the backend before merging: this changes what existing Conjur JWT installs send, not only what new ones send.

dataupload.Snapshot.ClusterName has no omitempty (internal/cyberark/dataupload/dataupload.go:62), so today every existing Conjur JWT install PUTs "cluster_name": "" once per period. After this change the same installs start PUTting "cluster_name": "<service_id>" on their first upload after upgrade. If the Discovery and Context API upserts the display name keyed on cluster_id, a name an operator set in the UI is silently overwritten, and the only way to opt out is to start setting config.clusterName in the chart.

The other clients deliberately avoid this: pkg/client/client_venafi_cloud.go:195 and pkg/client/client_ngts.go:241 both add the name query parameter only if opts.ClusterName != "", so empty means "leave the name alone". The CyberArk snapshot path has no equivalent guard, which is what turns a config-time default into an unconditional rename.

@roeezis roeezis Sep 2, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Checked this against the actual DCS/FIS persistence path, since it's the right question:

  • FIS: origin_store_processor.py:130-140 builds OriginStore(name=str(global_metadata.cluster_name), ...) straight from the snapshot, no transform.
  • DCS: aurora_postgres_db_handler_origin_stores.py:208-236 -> make_insert_stmt (aurora_postgres_db_utils.py:142-158) does stmt.on_conflict_do_update(set_={col: excluded[col] for col in payload_cols}, where=changed) — name is in payload_cols, so it's unconditionally overwritten on every scan cycle whenever it differs. No first-write-wins, no separate display-name column, no override flag.

So the mechanism you're describing is real: every periodic upload replaces the stored name. However — I searched discoverycontext-frontend for any existing rename/update-name mutation against this field and found none; only read paths exist today. There's no UI feature yet that lets an operator set a name for this to clobber. So the specific failure (operator renames in UI, agent silently reverts it) is currently hypothetical, not a live regression this PR introduces.

What this PR does change for already-deployed Conjur JWT installs: they go from uploading cluster_name: "" every cycle (no fallback existed) to uploading a non-empty fallback value every cycle — which is a net improvement given there's nothing today to overwrite. But your point stands as a real architectural gap: the moment a rename UI ships, this upsert-every-cycle behavior will clobber it, for every client (not just CyberArk — worth someone opening a tracking ticket for the DCS side, since the fix belongs there, not in this fallback chain). I don't think this PR should be blocked on that pre-existing gap, but wanted to give you the grounded answer rather than assert it away. Let me know if you'd still like this held pending backend sign-off.

}
if cfg.OrganizationID != "" {
log.Info(fmt.Sprintf(`Ignoring the organization_id field in the config file. This field is not needed in %s mode.`, res.OutputMode))
}
Expand Down
35 changes: 28 additions & 7 deletions pkg/agent/config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1388,14 +1388,17 @@ func TestConfig_CyberArk_Validation(t *testing.T) {
})

// cluster_name fallback order: cluster_name > cluster_id > ARK_USERNAME >
// empty. Explicit config (cluster_name, cluster_id) always wins over the
// env var; ARK_USERNAME is a legacy fallback kept only for pre-Conjur
// installs that set neither config field — the chart never emits
// cluster_id, so a chart-installed agent can't have set it, and
// ARK_USERNAME still applies. This also covers a migrating install
// cyberark.service_id > empty. Explicit config (cluster_name, cluster_id)
// always wins over the env var; ARK_USERNAME is a legacy fallback kept
// only for pre-Conjur installs that set neither config field — the chart
// never emits cluster_id, so a chart-installed agent can't have set it,
// and ARK_USERNAME still applies. This also covers a migrating install
// (service_id set to test Conjur alongside still-present ARK_USERNAME,
// no cluster_id yet): its reported name doesn't change just because
// service_id was added.
// service_id was added. service_id is checked last, below ARK_USERNAME,
// so it only kicks in for pure Conjur-JWT installs that have nothing
// else set — the onboarding wizard always sets it, so it's a real
// fallback even if the generated Helm command omits cluster_name.
t.Run("cluster_name falls back to cluster_id when set, even if ARK_USERNAME is also set", func(t *testing.T) {
setEnv(t)
t.Setenv("ARK_USERNAME", "svc-agent@tenant")
Expand Down Expand Up @@ -1434,7 +1437,7 @@ func TestConfig_CyberArk_Validation(t *testing.T) {
assert.Equal(t, "svc-agent@tenant", got.ClusterName)
})

t.Run("cluster_name is empty when cluster_name, cluster_id, and ARK_USERNAME are all unset", func(t *testing.T) {
t.Run("cluster_name falls back to cyberark.service_id when cluster_name, cluster_id, and ARK_USERNAME are all unset", func(t *testing.T) {
setEnv(t)
got, _, err := ValidateAndCombineConfig(discardLogs(),
withConfig(testutil.Undent(`
Expand All @@ -1443,6 +1446,24 @@ func TestConfig_CyberArk_Validation(t *testing.T) {
`)),
withCmdLineFlags("--period", "1m", "--machine-hub"))
require.NoError(t, err)
assert.Equal(t, "dev-cluster", got.ClusterName)
})

// The only way ClusterName ends up empty is if config-validation already
// failed: whichever of the two auth methods (service_id, or
// ARK_USERNAME+ARK_SECRET) satisfies the gate at config.go's cyberark
// validation block also satisfies a cluster_name fallback above. Assert
// the gate directly, rather than only in prose, so a future change that
// makes either auth method optional without updating the fallback chain
// fails this test instead of silently reintroducing an empty
// cluster_name for a config that validates successfully.
t.Run("empty ClusterName only occurs when config validation also fails", func(t *testing.T) {
setEnv(t)
got, _, err := ValidateAndCombineConfig(discardLogs(),
withConfig(""),
withCmdLineFlags("--period", "1m", "--machine-hub"))
require.Error(t, err)
assert.Contains(t, err.Error(), "MachineHub mode requires either cyberark.service_id or ARK_USERNAME/ARK_SECRET")
assert.Equal(t, "", got.ClusterName)
})

Expand Down
Loading