From 93d4240bd4ea0be9c295e88901d7618d53b4928e Mon Sep 17 00:00:00 2001 From: rzisholz Date: Wed, 16 Sep 2026 11:57:24 +0300 Subject: [PATCH 1/6] CP-26533: allow config.cyberark.subdomain in agent YAML 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. --- deploy/charts/disco-agent/README.md | 7 +++++ .../disco-agent/templates/configmap.yaml | 1 + .../disco-agent/templates/deployment.yaml | 2 ++ .../__snapshot__/configmap_test.yaml.snap | 4 +++ deploy/charts/disco-agent/values.schema.json | 8 ++++++ deploy/charts/disco-agent/values.yaml | 4 +++ internal/cyberark/client.go | 26 +++++++++---------- internal/cyberark/client_test.go | 2 +- pkg/agent/config.go | 5 +++- pkg/agent/config_test.go | 20 ++++++++++++-- pkg/client/client_cyberark.go | 17 +++++------- pkg/client/client_cyberark_test.go | 10 +++---- 12 files changed, 73 insertions(+), 33 deletions(-) diff --git a/deploy/charts/disco-agent/README.md b/deploy/charts/disco-agent/README.md index 4404a398..300eeb1b 100644 --- a/deploy/charts/disco-agent/README.md +++ b/deploy/charts/disco-agent/README.md @@ -422,6 +422,13 @@ This description will be associated with the data that the agent uploads to the Enable sending of Secret values to CyberArk in addition to metadata. Metadata is always sent, and Secret values are sent by default too. Set this to false to send metadata only. When enabled, Secret data is encrypted using envelope encryption using a key managed by CyberArk, fetched from the Discovery and Context service. +#### **config.cyberark.subdomain** ~ `string` +> Default value: +> ```yaml +> "" +> ``` + +CyberArk tenant subdomain. Not a credential. Leave empty to keep sourcing it from the authentication Secret's ARK_SUBDOMAIN key instead. #### **config.cyberark.serviceId** ~ `string` > Default value: > ```yaml diff --git a/deploy/charts/disco-agent/templates/configmap.yaml b/deploy/charts/disco-agent/templates/configmap.yaml index 3d712ce1..64edd0e6 100644 --- a/deploy/charts/disco-agent/templates/configmap.yaml +++ b/deploy/charts/disco-agent/templates/configmap.yaml @@ -11,6 +11,7 @@ data: cluster_description: {{ .Values.config.clusterDescription | quote }} period: {{ .Values.config.period | quote }} cyberark: + subdomain: {{ .Values.config.cyberark.subdomain | quote }} service_id: {{ .Values.config.cyberark.serviceId | quote }} account: {{ .Values.config.cyberark.account | quote }} jwt_source: {{ .Values.config.cyberark.jwtSource | quote }} diff --git a/deploy/charts/disco-agent/templates/deployment.yaml b/deploy/charts/disco-agent/templates/deployment.yaml index 0d3e1d17..3184cb08 100644 --- a/deploy/charts/disco-agent/templates/deployment.yaml +++ b/deploy/charts/disco-agent/templates/deployment.yaml @@ -58,11 +58,13 @@ spec: valueFrom: fieldRef: fieldPath: spec.nodeName + # Not a credential; only used when config.cyberark.subdomain is empty. - name: ARK_SUBDOMAIN valueFrom: secretKeyRef: name: {{ .Values.authentication.secretName }} key: ARK_SUBDOMAIN + optional: true - name: ARK_DISCOVERY_API valueFrom: secretKeyRef: diff --git a/deploy/charts/disco-agent/tests/__snapshot__/configmap_test.yaml.snap b/deploy/charts/disco-agent/tests/__snapshot__/configmap_test.yaml.snap index db251a92..6c0c4ea8 100644 --- a/deploy/charts/disco-agent/tests/__snapshot__/configmap_test.yaml.snap +++ b/deploy/charts/disco-agent/tests/__snapshot__/configmap_test.yaml.snap @@ -7,6 +7,7 @@ custom-cluster-description: cluster_description: "A cloud hosted Kubernetes cluster hosting production workloads.\n\nteam: team-1\nemail: team-1@example.com\npurpose: Production workloads\n" period: "12h0m0s" cyberark: + subdomain: "" service_id: "" account: "conjur" jwt_source: "file" @@ -170,6 +171,7 @@ custom-cluster-name: cluster_description: "" period: "12h0m0s" cyberark: + subdomain: "" service_id: "" account: "conjur" jwt_source: "file" @@ -333,6 +335,7 @@ custom-period: cluster_description: "" period: "1m" cyberark: + subdomain: "" service_id: "" account: "conjur" jwt_source: "file" @@ -496,6 +499,7 @@ defaults: cluster_description: "" period: "12h0m0s" cyberark: + subdomain: "" service_id: "" account: "conjur" jwt_source: "file" diff --git a/deploy/charts/disco-agent/values.schema.json b/deploy/charts/disco-agent/values.schema.json index a5d3c48b..3608cc54 100644 --- a/deploy/charts/disco-agent/values.schema.json +++ b/deploy/charts/disco-agent/values.schema.json @@ -163,6 +163,9 @@ }, "serviceId": { "$ref": "#/$defs/helm-values.config.cyberark.serviceId" + }, + "subdomain": { + "$ref": "#/$defs/helm-values.config.cyberark.subdomain" } }, "type": "object" @@ -182,6 +185,11 @@ "description": "The Conjur authn-jwt authenticator service ID configured for this cluster (one authenticator per cluster, not shared across a tenant's clusters — see config.clusterName above, which falls back to this value and depends on it being cluster-unique). Set this to use the Conjur JWT exchange (preferred). Leave empty to use the legacy CyberArk Identity username/password method (ARK_USERNAME/ARK_SECRET in the credentials Secret) for backward compatibility. If both are set, the serviceId (Conjur) wins. NOTE: bare service-id segment (e.g. a UUID chosen at onboarding), NOT the policy path \"conjur/authn-jwt/\" — the agent builds the URL as\n/authn-jwt///authenticate.", "type": "string" }, + "helm-values.config.cyberark.subdomain": { + "default": "", + "description": "CyberArk tenant subdomain. Not a credential. Leave empty to keep sourcing it from the authentication Secret's ARK_SUBDOMAIN key instead.", + "type": "string" + }, "helm-values.config.excludeAnnotationKeysRegex": { "default": [], "description": "You can configure the agent to exclude some annotations or labels from being pushed . All Kubernetes objects are affected. The objects are still pushed, but the specified annotations and labels are removed before being pushed.\n\nDots is the only character that needs to be escaped in the regex. Use either double quotes with escaped single quotes or unquoted strings for the regex to avoid YAML parsing issues with `\\.`.\n\nExample: excludeAnnotationKeysRegex: ['^kapp\\.k14s\\.io/original.*']", diff --git a/deploy/charts/disco-agent/values.yaml b/deploy/charts/disco-agent/values.yaml index e9df3484..7194551b 100644 --- a/deploy/charts/disco-agent/values.yaml +++ b/deploy/charts/disco-agent/values.yaml @@ -211,6 +211,10 @@ config: # short-lived Conjur access token, then uses that token to authenticate to the # Discovery & Context upload API. cyberark: + # CyberArk tenant subdomain. Not a credential. Leave empty to keep + # sourcing it from the authentication Secret's ARK_SUBDOMAIN key instead. + subdomain: "" + # The Conjur authn-jwt authenticator service ID configured for this # cluster (one authenticator per cluster, not shared across a tenant's # clusters — see config.clusterName above, which falls back to this diff --git a/internal/cyberark/client.go b/internal/cyberark/client.go index 355e8840..309161c0 100644 --- a/internal/cyberark/client.go +++ b/internal/cyberark/client.go @@ -61,26 +61,24 @@ type ClientConfig struct { // ClientConfigLoader is a function type that loads and returns a ClientConfig. type ClientConfigLoader func() (ClientConfig, error) -// ErrMissingEnvironmentVariables is returned when required environment variables are not set. -var ErrMissingEnvironmentVariables = errors.New("missing environment variables: ARK_SUBDOMAIN") +// ErrMissingSubdomain is returned when no subdomain is configured, either via +// config.cyberark.subdomain or the ARK_SUBDOMAIN environment variable. +var ErrMissingSubdomain = errors.New("no CyberArk subdomain configured: set config.cyberark.subdomain or the ARK_SUBDOMAIN environment variable") // ErrNoAuthMethod is returned when neither a Conjur service-id nor // username/password credentials are configured. var ErrNoAuthMethod = errors.New("no CyberArk authentication method configured: set config.cyberark.service_id (Conjur JWT) or ARK_USERNAME + ARK_SECRET (legacy username/password)") -// LoadClientConfigFromEnvironment loads the CyberArk client configuration from environment variables. -// It expects the following environment variable to be set: -// - ARK_SUBDOMAIN: The CyberArk subdomain to use (required). -// -// It also reads the optional legacy username/password credentials: -// - ARK_USERNAME, ARK_SECRET: used only when no Conjur service-id is configured. -// -// Behavioral keys (ServiceID, Account, JWTSource, JWTFilePath) are set by the -// caller from the agent YAML config (config.cyberark.*). -func LoadClientConfigFromEnvironment() (ClientConfig, error) { - subdomain := os.Getenv("ARK_SUBDOMAIN") +// LoadClientConfigFromEnvironment loads the CyberArk client config. subdomain +// (from config.cyberark.subdomain) takes precedence; falls back to +// ARK_SUBDOMAIN when empty. Also reads legacy ARK_USERNAME/ARK_SECRET, used +// only when no Conjur service-id is configured. +func LoadClientConfigFromEnvironment(subdomain string) (ClientConfig, error) { + if subdomain == "" { + subdomain = os.Getenv("ARK_SUBDOMAIN") + } if subdomain == "" { - return ClientConfig{}, ErrMissingEnvironmentVariables + return ClientConfig{}, ErrMissingSubdomain } cfg := ClientConfig{ Subdomain: subdomain, diff --git a/internal/cyberark/client_test.go b/internal/cyberark/client_test.go index d111f99e..a62583c8 100644 --- a/internal/cyberark/client_test.go +++ b/internal/cyberark/client_test.go @@ -169,7 +169,7 @@ func TestCyberArkClient_PutSnapshot_RealAPI(t *testing.T) { }, } - cfg, err := cyberark.LoadClientConfigFromEnvironment() + cfg, err := cyberark.LoadClientConfigFromEnvironment("") require.NoError(t, err) discoveryClient, err := servicediscovery.New(httpClient, cfg.Subdomain) diff --git a/pkg/agent/config.go b/pkg/agent/config.go index aa3f0356..91df2fcb 100644 --- a/pkg/agent/config.go +++ b/pkg/agent/config.go @@ -107,6 +107,9 @@ type VenafiCloudConfig struct { // CyberArkConfig holds YAML configuration for MachineHub (CyberArk) mode (POC). type CyberArkConfig struct { + // Subdomain is the CyberArk tenant subdomain. Not a credential; falls + // back to the ARK_SUBDOMAIN environment variable when empty. + Subdomain string `yaml:"subdomain"` // ServiceID is the authn-jwt service ID configured in Conjur (e.g. "dev-cluster"). ServiceID string `yaml:"service_id"` // Account is the Conjur account name. Defaults to "conjur" when empty. @@ -1052,7 +1055,7 @@ func validateCredsAndCreateClient(log logr.Logger, flagCredentialsPath, flagClie rootCAs *x509.CertPool ) httpClient := http_client.NewDefaultClient(version.UserAgent(), rootCAs) - outputClient, err = client.NewCyberArk(httpClient, cfg.CyberArk.ServiceID, cfg.CyberArk.Account, cfg.CyberArk.JWTSource, cfg.CyberArk.JWTFilePath) + outputClient, err = client.NewCyberArk(httpClient, cfg.CyberArk.Subdomain, cfg.CyberArk.ServiceID, cfg.CyberArk.Account, cfg.CyberArk.JWTSource, cfg.CyberArk.JWTFilePath) if err != nil { errs = multierror.Append(errs, err) } diff --git a/pkg/agent/config_test.go b/pkg/agent/config_test.go index 05909879..7db5c17c 100644 --- a/pkg/agent/config_test.go +++ b/pkg/agent/config_test.go @@ -711,7 +711,23 @@ func Test_ValidateAndCombineConfig(t *testing.T) { assert.IsType(t, &client.CyberArkClient{}, cl) }) - t.Run("--machine-hub without ARK_SUBDOMAIN environment variable", func(t *testing.T) { + t.Run("--machine-hub with config.cyberark.subdomain instead of ARK_SUBDOMAIN environment variable", func(t *testing.T) { + t.Setenv("POD_NAMESPACE", "venafi") + t.Setenv("KUBECONFIG", withFile(t, fakeKubeconfig)) + t.Setenv("ARK_SUBDOMAIN", "") + got, cl, err := ValidateAndCombineConfig(discardLogs(), + withConfig(testutil.Undent(` + cyberark: + subdomain: tlspk + service_id: dev-cluster + `)), + withCmdLineFlags("--period", "1m", "--machine-hub")) + require.NoError(t, err) + assert.Equal(t, MachineHub, got.OutputMode) + assert.IsType(t, &client.CyberArkClient{}, cl) + }) + + t.Run("--machine-hub without ARK_SUBDOMAIN environment variable or config.cyberark.subdomain", func(t *testing.T) { t.Setenv("POD_NAMESPACE", "venafi") t.Setenv("KUBECONFIG", withFile(t, fakeKubeconfig)) t.Setenv("ARK_SUBDOMAIN", "") @@ -725,7 +741,7 @@ func Test_ValidateAndCombineConfig(t *testing.T) { assert.Nil(t, cl) assert.EqualError(t, err, testutil.Undent(` validating creds: failed loading config using the MachineHub mode: 1 error occurred: - * missing environment variables: ARK_SUBDOMAIN + * no CyberArk subdomain configured: set config.cyberark.subdomain or the ARK_SUBDOMAIN environment variable `)) }) diff --git a/pkg/client/client_cyberark.go b/pkg/client/client_cyberark.go index e072b450..9c69eede 100644 --- a/pkg/client/client_cyberark.go +++ b/pkg/client/client_cyberark.go @@ -34,16 +34,13 @@ type CyberArkClient struct { var _ Client = &CyberArkClient{} -// NewCyberArk initializes a CyberArk client. -// Subdomain, and the legacy username/password credentials, are loaded from the -// environment (ARK_SUBDOMAIN, ARK_USERNAME, ARK_SECRET). The remaining fields -// (serviceID, account, jwtSource, jwtFilePath) come from the agent YAML config -// (config.cyberark.*) and select the Conjur JWT exchange when serviceID is set. -// Sending secrets is controlled by the ARK_SEND_SECRETS environment variable -// (defaults to "false"). If the configuration is invalid or missing, an error -// is returned. -func NewCyberArk(httpClient *http.Client, serviceID, account, jwtSource, jwtFilePath string) (*CyberArkClient, error) { - cfg, err := cyberark.LoadClientConfigFromEnvironment() +// NewCyberArk initializes a CyberArk client. subdomain, serviceID, account, +// jwtSource, and jwtFilePath come from the agent YAML config +// (config.cyberark.*); subdomain falls back to ARK_SUBDOMAIN when empty. +// Legacy username/password credentials remain env-var-only (ARK_USERNAME, +// ARK_SECRET) since they're real credentials. +func NewCyberArk(httpClient *http.Client, subdomain, serviceID, account, jwtSource, jwtFilePath string) (*CyberArkClient, error) { + cfg, err := cyberark.LoadClientConfigFromEnvironment(subdomain) if err != nil { return nil, err } diff --git a/pkg/client/client_cyberark_test.go b/pkg/client/client_cyberark_test.go index 47fe7abf..a818ff85 100644 --- a/pkg/client/client_cyberark_test.go +++ b/pkg/client/client_cyberark_test.go @@ -42,7 +42,7 @@ func TestCyberArkClient_PostDataReadingsWithOptions_MockAPI(t *testing.T) { httpClient, jwtFilePath := testutil.FakeCyberArk(t) - c, err := client.NewCyberArk(httpClient, "test-service", "", "file", jwtFilePath) + c, err := client.NewCyberArk(httpClient, "", "test-service", "", "file", jwtFilePath) require.NoError(t, err) readings := fakeReadings() @@ -64,7 +64,7 @@ func TestCyberArkClient_PostDataReadingsWithOptions_UsernamePasswordMockAPI(t *t logger := ktesting.NewLogger(t, ktesting.DefaultConfig) ctx := klog.NewContext(t.Context(), logger) - c, err := client.NewCyberArk(httpClient, "", "", "", "") + c, err := client.NewCyberArk(httpClient, "", "", "", "", "") require.NoError(t, err) readings := fakeReadings() @@ -84,7 +84,7 @@ func TestCyberArkClient_PostDataReadingsWithOptions_UsernamePasswordSecondUpload t.Setenv("ARK_USERNAME", username) t.Setenv("ARK_SECRET", password) - c, err := client.NewCyberArk(httpClient, "", "", "", "") + c, err := client.NewCyberArk(httpClient, "", "", "", "", "") require.NoError(t, err) readings := fakeReadings() @@ -113,9 +113,9 @@ func TestCyberArkClient_PostDataReadingsWithOptions_RealAPI(t *testing.T) { httpClient := http_client.NewDefaultClient(version.UserAgent(), rootCAs) serviceID := os.Getenv("ARK_SERVICE_ID") - c, err := client.NewCyberArk(httpClient, serviceID, "", "", "") + c, err := client.NewCyberArk(httpClient, "", serviceID, "", "", "") if err != nil { - if errors.Is(err, cyberark.ErrMissingEnvironmentVariables) { + if errors.Is(err, cyberark.ErrMissingSubdomain) { t.Skipf("Skipping: %s", err) } require.NoError(t, err) From 91e8cde25edc60b30cc137fbe81d8b2f371d1a03 Mon Sep 17 00:00:00 2001 From: rzisholz Date: Wed, 16 Sep 2026 15:50:17 +0300 Subject: [PATCH 2/6] CP-26533: test the subdomain config-over-env precedence rule 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. --- internal/cyberark/subdomain_source_test.go | 50 ++++++++++++++++++++++ 1 file changed, 50 insertions(+) create mode 100644 internal/cyberark/subdomain_source_test.go diff --git a/internal/cyberark/subdomain_source_test.go b/internal/cyberark/subdomain_source_test.go new file mode 100644 index 00000000..ac6ef52d --- /dev/null +++ b/internal/cyberark/subdomain_source_test.go @@ -0,0 +1,50 @@ +package cyberark_test + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/jetstack/preflight/internal/cyberark" +) + +// The subdomain is not a credential, so it is settable from the agent YAML +// config as well as the ARK_SUBDOMAIN environment variable that predates it. +// These tests pin the precedence rule in LoadClientConfigFromEnvironment: +// - config only → config +// - env only → env (backward compatible with Secret-provided installs) +// - both set → config wins +// - neither → ErrMissingSubdomain +func TestLoadClientConfigFromEnvironment_SubdomainPrecedence(t *testing.T) { + t.Run("config only -> config", func(t *testing.T) { + t.Setenv("ARK_SUBDOMAIN", "") + + cfg, err := cyberark.LoadClientConfigFromEnvironment("from-config") + require.NoError(t, err) + assert.Equal(t, "from-config", cfg.Subdomain) + }) + + t.Run("env only -> env", func(t *testing.T) { + t.Setenv("ARK_SUBDOMAIN", "from-env") + + cfg, err := cyberark.LoadClientConfigFromEnvironment("") + require.NoError(t, err) + assert.Equal(t, "from-env", cfg.Subdomain) + }) + + t.Run("both set -> config wins", func(t *testing.T) { + t.Setenv("ARK_SUBDOMAIN", "from-env") + + cfg, err := cyberark.LoadClientConfigFromEnvironment("from-config") + require.NoError(t, err) + assert.Equal(t, "from-config", cfg.Subdomain) + }) + + t.Run("neither set -> ErrMissingSubdomain", func(t *testing.T) { + t.Setenv("ARK_SUBDOMAIN", "") + + _, err := cyberark.LoadClientConfigFromEnvironment("") + require.ErrorIs(t, err, cyberark.ErrMissingSubdomain) + }) +} From f650d942de5a5c2fdd9e30b9f76ac62cdeaf539f Mon Sep 17 00:00:00 2001 From: rzisholz Date: Wed, 16 Sep 2026 16:06:32 +0300 Subject: [PATCH 3/6] CP-26533: collapse the subdomain fallback into cmp.Or Two sequential identical empty-checks become one first-non-empty expression, which is the precedence rule stated directly. --- internal/cyberark/client.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/internal/cyberark/client.go b/internal/cyberark/client.go index 309161c0..30600e1c 100644 --- a/internal/cyberark/client.go +++ b/internal/cyberark/client.go @@ -1,6 +1,7 @@ package cyberark import ( + "cmp" "context" "errors" "fmt" @@ -74,9 +75,7 @@ var ErrNoAuthMethod = errors.New("no CyberArk authentication method configured: // ARK_SUBDOMAIN when empty. Also reads legacy ARK_USERNAME/ARK_SECRET, used // only when no Conjur service-id is configured. func LoadClientConfigFromEnvironment(subdomain string) (ClientConfig, error) { - if subdomain == "" { - subdomain = os.Getenv("ARK_SUBDOMAIN") - } + subdomain = cmp.Or(subdomain, os.Getenv("ARK_SUBDOMAIN")) if subdomain == "" { return ClientConfig{}, ErrMissingSubdomain } From 37e5ee3c309122d607880781f35e4a99de08c770 Mon Sep 17 00:00:00 2001 From: rzisholz Date: Wed, 16 Sep 2026 16:13:45 +0300 Subject: [PATCH 4/6] CP-26533: fix README claim that ARK_SUBDOMAIN is required, add chart test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- deploy/charts/disco-agent/README.md | 11 +++++--- .../disco-agent/tests/deployment_test.yaml | 25 +++++++++++++++++++ pkg/agent/config_test.go | 1 + 3 files changed, 34 insertions(+), 3 deletions(-) create mode 100644 deploy/charts/disco-agent/tests/deployment_test.yaml diff --git a/deploy/charts/disco-agent/README.md b/deploy/charts/disco-agent/README.md index 300eeb1b..fbef2907 100644 --- a/deploy/charts/disco-agent/README.md +++ b/deploy/charts/disco-agent/README.md @@ -28,8 +28,10 @@ If **both** are set, the Conjur `serviceId` wins (so a migrating install can add the service-id before removing its old credentials) and a warning is logged. If **neither** is set, the agent fails closed at startup. -The only credential always required in the Kubernetes Secret is the CyberArk -tenant subdomain (`ARK_SUBDOMAIN`). +The agent also needs your CyberArk tenant subdomain, but it is **not a +credential** — set it via `config.cyberark.subdomain` (see below) and skip the +Secret entirely for a Conjur-JWT-only install. `ARK_SUBDOMAIN` in the Secret +still works as a fallback for existing installs that already set it there. ```sh export ARK_SUBDOMAIN= # your CyberArk tenant subdomain, e.g. tlskp-test @@ -37,7 +39,8 @@ export ARK_SUBDOMAIN= # your CyberArk tenant subdomain, e.g. tlskp-test export ARK_DISCOVERY_API=https://platform-discovery.integration-cyberark.cloud/ ``` -Create the Secret: +Create the Secret (only needed for the legacy username/password method, or if +you'd rather set the subdomain here than in `config.cyberark.subdomain`): ```sh # Production (no ARK_DISCOVERY_API override needed): @@ -119,11 +122,13 @@ value for `config.cyberark.serviceId` below. ```sh # $SERVICE_ID is this cluster's own authn-jwt service ID from onboarding above — do not reuse it across clusters. +# No Secret needed for this Conjur-JWT install — the subdomain isn't a credential. helm upgrade agent "oci://${OCI_BASE}/charts/disco-agent" \ --install \ --create-namespace \ --namespace "$NAMESPACE" \ --set fullnameOverride=disco-agent \ + --set config.cyberark.subdomain="$ARK_SUBDOMAIN" \ --set config.cyberark.serviceId="$SERVICE_ID" \ --set acceptTerms=true ``` diff --git a/deploy/charts/disco-agent/tests/deployment_test.yaml b/deploy/charts/disco-agent/tests/deployment_test.yaml new file mode 100644 index 00000000..f2d78be0 --- /dev/null +++ b/deploy/charts/disco-agent/tests/deployment_test.yaml @@ -0,0 +1,25 @@ +suite: test the deployment's credential env vars +templates: + - deployment.yaml +release: + name: test + namespace: test-ns +set: + acceptTerms: true +tests: + - it: ARK_SUBDOMAIN and ARK_DISCOVERY_API secretKeyRefs are optional, so a Conjur-JWT-only install needs no Secret + asserts: + - isKind: + of: Deployment + - equal: + path: spec.template.spec.containers[0].env[?(@.name == "ARK_SUBDOMAIN")].valueFrom.secretKeyRef.optional + value: true + - equal: + path: spec.template.spec.containers[0].env[?(@.name == "ARK_DISCOVERY_API")].valueFrom.secretKeyRef.optional + value: true + - equal: + path: spec.template.spec.containers[0].env[?(@.name == "ARK_USERNAME")].valueFrom.secretKeyRef.optional + value: true + - equal: + path: spec.template.spec.containers[0].env[?(@.name == "ARK_SECRET")].valueFrom.secretKeyRef.optional + value: true diff --git a/pkg/agent/config_test.go b/pkg/agent/config_test.go index 7db5c17c..71cd494e 100644 --- a/pkg/agent/config_test.go +++ b/pkg/agent/config_test.go @@ -724,6 +724,7 @@ func Test_ValidateAndCombineConfig(t *testing.T) { withCmdLineFlags("--period", "1m", "--machine-hub")) require.NoError(t, err) assert.Equal(t, MachineHub, got.OutputMode) + assert.Equal(t, "tlspk", got.CyberArk.Subdomain) assert.IsType(t, &client.CyberArkClient{}, cl) }) From 12ee4767ed6f12ab73885736a0fe6d16a81fe162 Mon Sep 17 00:00:00 2001 From: rzisholz Date: Thu, 17 Sep 2026 13:08:18 +0300 Subject: [PATCH 5/6] =?UTF-8?q?CP-26533:=20address=20wallrj=20review=20?= =?UTF-8?q?=E2=80=94=20README=20walkthrough,=20dual-source=20warning,=20re?= =?UTF-8?q?name,=20chart=20test?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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. --- deploy/charts/disco-agent/README.md | 24 +-- .../__snapshot__/configmap_test.yaml.snap | 164 ++++++++++++++++++ .../disco-agent/tests/configmap_test.yaml | 6 + internal/cyberark/client.go | 8 +- internal/cyberark/client_test.go | 2 +- internal/cyberark/subdomain_source_test.go | 40 ++++- pkg/agent/config.go | 2 +- pkg/client/client_cyberark.go | 4 +- pkg/client/client_cyberark_test.go | 9 +- 9 files changed, 231 insertions(+), 28 deletions(-) diff --git a/deploy/charts/disco-agent/README.md b/deploy/charts/disco-agent/README.md index fbef2907..13093e46 100644 --- a/deploy/charts/disco-agent/README.md +++ b/deploy/charts/disco-agent/README.md @@ -14,6 +14,18 @@ export NAMESPACE=cyberark kubectl create ns "$NAMESPACE" || true ``` +### Set your CyberArk tenant subdomain + +Always required, but **not a credential**. Set it via `config.cyberark.subdomain` +on the `helm upgrade` command below, or via `ARK_SUBDOMAIN` in the Secret if +you're already creating one for the legacy username/password method. + +```sh +export ARK_SUBDOMAIN= # your CyberArk tenant subdomain, e.g. tlskp-test +# OPTIONAL: Discovery API URL for non-production environments +export ARK_DISCOVERY_API=https://platform-discovery.integration-cyberark.cloud/ +``` + ### Add credentials to a Secret The agent supports **two authentication methods**, selected automatically by @@ -28,16 +40,8 @@ If **both** are set, the Conjur `serviceId` wins (so a migrating install can add the service-id before removing its old credentials) and a warning is logged. If **neither** is set, the agent fails closed at startup. -The agent also needs your CyberArk tenant subdomain, but it is **not a -credential** — set it via `config.cyberark.subdomain` (see below) and skip the -Secret entirely for a Conjur-JWT-only install. `ARK_SUBDOMAIN` in the Secret -still works as a fallback for existing installs that already set it there. - -```sh -export ARK_SUBDOMAIN= # your CyberArk tenant subdomain, e.g. tlskp-test -# OPTIONAL: Discovery API URL for non-production environments -export ARK_DISCOVERY_API=https://platform-discovery.integration-cyberark.cloud/ -``` +Skip this section entirely for a Conjur-JWT-only install: `config.cyberark.subdomain` +above already covers the one non-credential value this Secret would otherwise carry. Create the Secret (only needed for the legacy username/password method, or if you'd rather set the subdomain here than in `config.cyberark.subdomain`): diff --git a/deploy/charts/disco-agent/tests/__snapshot__/configmap_test.yaml.snap b/deploy/charts/disco-agent/tests/__snapshot__/configmap_test.yaml.snap index 6c0c4ea8..2e5b4a8d 100644 --- a/deploy/charts/disco-agent/tests/__snapshot__/configmap_test.yaml.snap +++ b/deploy/charts/disco-agent/tests/__snapshot__/configmap_test.yaml.snap @@ -490,6 +490,170 @@ custom-period: helm.sh/chart: disco-agent-0.0.0 name: test-disco-agent-config namespace: test-ns +custom-subdomain: + 1: | + apiVersion: v1 + data: + config.yaml: |- + cluster_name: "" + cluster_description: "" + period: "12h0m0s" + cyberark: + subdomain: "tlskp-test" + service_id: "" + account: "conjur" + jwt_source: "file" + data-gatherers: + - kind: oidc + name: ark/oidc + - kind: k8s-discovery + name: ark/discovery + - kind: k8s-dynamic + name: ark/secrets + config: + resource-type: + version: v1 + resource: secrets + field-selectors: + - type!=kubernetes.io/dockercfg + - type!=kubernetes.io/dockerconfigjson + - type!=bootstrap.kubernetes.io/token + - type!=helm.sh/release.v1 + - kind: k8s-dynamic + name: ark/serviceaccounts + config: + resource-type: + resource: serviceaccounts + version: v1 + - kind: k8s-dynamic + name: ark/roles + config: + resource-type: + version: v1 + group: rbac.authorization.k8s.io + resource: roles + - kind: k8s-dynamic + name: ark/clusterroles + config: + resource-type: + version: v1 + group: rbac.authorization.k8s.io + resource: clusterroles + - kind: k8s-dynamic + name: ark/rolebindings + config: + resource-type: + version: v1 + group: rbac.authorization.k8s.io + resource: rolebindings + - kind: k8s-dynamic + name: ark/clusterrolebindings + config: + resource-type: + version: v1 + group: rbac.authorization.k8s.io + resource: clusterrolebindings + - kind: k8s-dynamic + name: ark/jobs + config: + resource-type: + version: v1 + group: batch + resource: jobs + - kind: k8s-dynamic + name: ark/cronjobs + config: + resource-type: + version: v1 + group: batch + resource: cronjobs + - kind: k8s-dynamic + name: ark/deployments + config: + resource-type: + version: v1 + group: apps + resource: deployments + - kind: k8s-dynamic + name: ark/statefulsets + config: + resource-type: + version: v1 + group: apps + resource: statefulsets + - kind: k8s-dynamic + name: ark/daemonsets + config: + resource-type: + version: v1 + group: apps + resource: daemonsets + - kind: k8s-dynamic + name: ark/pods + config: + resource-type: + version: v1 + resource: pods + - kind: k8s-dynamic + name: ark/configmaps + config: + resource-type: + resource: configmaps + version: v1 + label-selectors: + - conjur.org/name=conjur-connect-configmap + - kind: k8s-dynamic + name: ark/esoexternalsecrets + config: + resource-type: + group: external-secrets.io + version: v1 + resource: externalsecrets + - kind: k8s-dynamic + name: ark/esosecretstores + config: + resource-type: + group: external-secrets.io + version: v1 + resource: secretstores + - kind: k8s-dynamic + name: ark/esoclusterexternalsecrets + config: + resource-type: + group: external-secrets.io + version: v1 + resource: clusterexternalsecrets + - kind: k8s-dynamic + name: ark/esoclustersecretstores + config: + resource-type: + group: external-secrets.io + version: v1 + resource: clustersecretstores + - kind: k8s-dynamic + name: ark/secretproviderclasses + config: + resource-type: + group: secrets-store.csi.x-k8s.io + version: v1 + resource: secretproviderclasses + - kind: k8s-dynamic + name: ark/secretproviderclasspodstatuses + config: + resource-type: + group: secrets-store.csi.x-k8s.io + version: v1 + resource: secretproviderclasspodstatuses + kind: ConfigMap + metadata: + labels: + app.kubernetes.io/instance: test + app.kubernetes.io/managed-by: Helm + app.kubernetes.io/name: disco-agent + app.kubernetes.io/version: v0.0.0 + helm.sh/chart: disco-agent-0.0.0 + name: test-disco-agent-config + namespace: test-ns defaults: 1: | apiVersion: v1 diff --git a/deploy/charts/disco-agent/tests/configmap_test.yaml b/deploy/charts/disco-agent/tests/configmap_test.yaml index 59c81576..b2da54ba 100644 --- a/deploy/charts/disco-agent/tests/configmap_test.yaml +++ b/deploy/charts/disco-agent/tests/configmap_test.yaml @@ -31,3 +31,9 @@ tests: purpose: Production workloads asserts: - matchSnapshot: {} + + - it: custom-subdomain + set: + config.cyberark.subdomain: tlskp-test + asserts: + - matchSnapshot: {} diff --git a/internal/cyberark/client.go b/internal/cyberark/client.go index 30600e1c..9f98709b 100644 --- a/internal/cyberark/client.go +++ b/internal/cyberark/client.go @@ -8,6 +8,7 @@ import ( "net/http" "os" + "github.com/go-logr/logr" "k8s.io/klog/v2" "github.com/jetstack/preflight/internal/cyberark/conjur" @@ -70,11 +71,14 @@ var ErrMissingSubdomain = errors.New("no CyberArk subdomain configured: set conf // username/password credentials are configured. var ErrNoAuthMethod = errors.New("no CyberArk authentication method configured: set config.cyberark.service_id (Conjur JWT) or ARK_USERNAME + ARK_SECRET (legacy username/password)") -// LoadClientConfigFromEnvironment loads the CyberArk client config. subdomain +// LoadClientConfig loads the CyberArk client config. subdomain // (from config.cyberark.subdomain) takes precedence; falls back to // ARK_SUBDOMAIN when empty. Also reads legacy ARK_USERNAME/ARK_SECRET, used // only when no Conjur service-id is configured. -func LoadClientConfigFromEnvironment(subdomain string) (ClientConfig, error) { +func LoadClientConfig(log logr.Logger, subdomain string) (ClientConfig, error) { + if envSubdomain := os.Getenv("ARK_SUBDOMAIN"); subdomain != "" && envSubdomain != "" { + log.Info("both config.cyberark.subdomain and ARK_SUBDOMAIN are set; using config.cyberark.subdomain and ignoring the environment variable") + } subdomain = cmp.Or(subdomain, os.Getenv("ARK_SUBDOMAIN")) if subdomain == "" { return ClientConfig{}, ErrMissingSubdomain diff --git a/internal/cyberark/client_test.go b/internal/cyberark/client_test.go index a62583c8..fffb144d 100644 --- a/internal/cyberark/client_test.go +++ b/internal/cyberark/client_test.go @@ -169,7 +169,7 @@ func TestCyberArkClient_PutSnapshot_RealAPI(t *testing.T) { }, } - cfg, err := cyberark.LoadClientConfigFromEnvironment("") + cfg, err := cyberark.LoadClientConfig(logger, "") require.NoError(t, err) discoveryClient, err := servicediscovery.New(httpClient, cfg.Subdomain) diff --git a/internal/cyberark/subdomain_source_test.go b/internal/cyberark/subdomain_source_test.go index ac6ef52d..d48211ea 100644 --- a/internal/cyberark/subdomain_source_test.go +++ b/internal/cyberark/subdomain_source_test.go @@ -3,6 +3,7 @@ package cyberark_test import ( "testing" + "github.com/go-logr/logr" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -11,16 +12,16 @@ import ( // The subdomain is not a credential, so it is settable from the agent YAML // config as well as the ARK_SUBDOMAIN environment variable that predates it. -// These tests pin the precedence rule in LoadClientConfigFromEnvironment: +// These tests pin the precedence rule in LoadClientConfig: // - config only → config // - env only → env (backward compatible with Secret-provided installs) -// - both set → config wins +// - both set → config wins, with a warning logged // - neither → ErrMissingSubdomain -func TestLoadClientConfigFromEnvironment_SubdomainPrecedence(t *testing.T) { +func TestLoadClientConfig_SubdomainPrecedence(t *testing.T) { t.Run("config only -> config", func(t *testing.T) { t.Setenv("ARK_SUBDOMAIN", "") - cfg, err := cyberark.LoadClientConfigFromEnvironment("from-config") + cfg, err := cyberark.LoadClientConfig(logr.Discard(), "from-config") require.NoError(t, err) assert.Equal(t, "from-config", cfg.Subdomain) }) @@ -28,23 +29,46 @@ func TestLoadClientConfigFromEnvironment_SubdomainPrecedence(t *testing.T) { t.Run("env only -> env", func(t *testing.T) { t.Setenv("ARK_SUBDOMAIN", "from-env") - cfg, err := cyberark.LoadClientConfigFromEnvironment("") + cfg, err := cyberark.LoadClientConfig(logr.Discard(), "") require.NoError(t, err) assert.Equal(t, "from-env", cfg.Subdomain) }) - t.Run("both set -> config wins", func(t *testing.T) { + t.Run("both set -> config wins, with a warning logged", func(t *testing.T) { t.Setenv("ARK_SUBDOMAIN", "from-env") - cfg, err := cyberark.LoadClientConfigFromEnvironment("from-config") + sink := &capturingSink{} + cfg, err := cyberark.LoadClientConfig(logr.New(sink), "from-config") require.NoError(t, err) assert.Equal(t, "from-config", cfg.Subdomain) + assert.Len(t, sink.messages, 1) + if len(sink.messages) == 1 { + assert.Contains(t, sink.messages[0], "both config.cyberark.subdomain and ARK_SUBDOMAIN are set") + } }) t.Run("neither set -> ErrMissingSubdomain", func(t *testing.T) { t.Setenv("ARK_SUBDOMAIN", "") - _, err := cyberark.LoadClientConfigFromEnvironment("") + _, err := cyberark.LoadClientConfig(logr.Discard(), "") require.ErrorIs(t, err, cyberark.ErrMissingSubdomain) }) } + +// 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 { + messages []string +} + +func (s *capturingSink) Init(logr.RuntimeInfo) {} +func (s *capturingSink) Enabled(int) bool { return true } +func (s *capturingSink) Error(error, string, ...any) {} + +func (s *capturingSink) Info(_ int, msg string, _ ...any) { + s.messages = append(s.messages, msg) +} + +func (s *capturingSink) WithValues(...any) logr.LogSink { return s } +func (s *capturingSink) WithName(string) logr.LogSink { return s } diff --git a/pkg/agent/config.go b/pkg/agent/config.go index 91df2fcb..e8a59799 100644 --- a/pkg/agent/config.go +++ b/pkg/agent/config.go @@ -1055,7 +1055,7 @@ func validateCredsAndCreateClient(log logr.Logger, flagCredentialsPath, flagClie rootCAs *x509.CertPool ) httpClient := http_client.NewDefaultClient(version.UserAgent(), rootCAs) - outputClient, err = client.NewCyberArk(httpClient, cfg.CyberArk.Subdomain, cfg.CyberArk.ServiceID, cfg.CyberArk.Account, cfg.CyberArk.JWTSource, cfg.CyberArk.JWTFilePath) + outputClient, err = client.NewCyberArk(log, httpClient, cfg.CyberArk.Subdomain, cfg.CyberArk.ServiceID, cfg.CyberArk.Account, cfg.CyberArk.JWTSource, cfg.CyberArk.JWTFilePath) if err != nil { errs = multierror.Append(errs, err) } diff --git a/pkg/client/client_cyberark.go b/pkg/client/client_cyberark.go index 9c69eede..c3adc76b 100644 --- a/pkg/client/client_cyberark.go +++ b/pkg/client/client_cyberark.go @@ -39,8 +39,8 @@ var _ Client = &CyberArkClient{} // (config.cyberark.*); subdomain falls back to ARK_SUBDOMAIN when empty. // Legacy username/password credentials remain env-var-only (ARK_USERNAME, // ARK_SECRET) since they're real credentials. -func NewCyberArk(httpClient *http.Client, subdomain, serviceID, account, jwtSource, jwtFilePath string) (*CyberArkClient, error) { - cfg, err := cyberark.LoadClientConfigFromEnvironment(subdomain) +func NewCyberArk(log logr.Logger, httpClient *http.Client, subdomain, serviceID, account, jwtSource, jwtFilePath string) (*CyberArkClient, error) { + cfg, err := cyberark.LoadClientConfig(log, subdomain) if err != nil { return nil, err } diff --git a/pkg/client/client_cyberark_test.go b/pkg/client/client_cyberark_test.go index a818ff85..c4d206a7 100644 --- a/pkg/client/client_cyberark_test.go +++ b/pkg/client/client_cyberark_test.go @@ -7,6 +7,7 @@ import ( "strings" "testing" + "github.com/go-logr/logr" "github.com/jetstack/venafi-connection-lib/http_client" "github.com/stretchr/testify/require" k8sversion "k8s.io/apimachinery/pkg/version" @@ -42,7 +43,7 @@ func TestCyberArkClient_PostDataReadingsWithOptions_MockAPI(t *testing.T) { httpClient, jwtFilePath := testutil.FakeCyberArk(t) - c, err := client.NewCyberArk(httpClient, "", "test-service", "", "file", jwtFilePath) + c, err := client.NewCyberArk(logger, httpClient, "", "test-service", "", "file", jwtFilePath) require.NoError(t, err) readings := fakeReadings() @@ -64,7 +65,7 @@ func TestCyberArkClient_PostDataReadingsWithOptions_UsernamePasswordMockAPI(t *t logger := ktesting.NewLogger(t, ktesting.DefaultConfig) ctx := klog.NewContext(t.Context(), logger) - c, err := client.NewCyberArk(httpClient, "", "", "", "", "") + c, err := client.NewCyberArk(logger, httpClient, "", "", "", "", "") require.NoError(t, err) readings := fakeReadings() @@ -84,7 +85,7 @@ func TestCyberArkClient_PostDataReadingsWithOptions_UsernamePasswordSecondUpload t.Setenv("ARK_USERNAME", username) t.Setenv("ARK_SECRET", password) - c, err := client.NewCyberArk(httpClient, "", "", "", "", "") + c, err := client.NewCyberArk(logr.Discard(), httpClient, "", "", "", "", "") require.NoError(t, err) readings := fakeReadings() @@ -113,7 +114,7 @@ func TestCyberArkClient_PostDataReadingsWithOptions_RealAPI(t *testing.T) { httpClient := http_client.NewDefaultClient(version.UserAgent(), rootCAs) serviceID := os.Getenv("ARK_SERVICE_ID") - c, err := client.NewCyberArk(httpClient, "", serviceID, "", "", "") + c, err := client.NewCyberArk(logger, httpClient, "", serviceID, "", "", "") if err != nil { if errors.Is(err, cyberark.ErrMissingSubdomain) { t.Skipf("Skipping: %s", err) From ed4296b28bfe6c271969ea653e636ba2267aba6a Mon Sep 17 00:00:00 2001 From: rzisholz Date: Thu, 17 Sep 2026 13:15:47 +0300 Subject: [PATCH 6/6] CP-26533: gofmt the new capturingSink method alignment --- internal/cyberark/subdomain_source_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/cyberark/subdomain_source_test.go b/internal/cyberark/subdomain_source_test.go index d48211ea..0a1dde5e 100644 --- a/internal/cyberark/subdomain_source_test.go +++ b/internal/cyberark/subdomain_source_test.go @@ -62,8 +62,8 @@ type capturingSink struct { messages []string } -func (s *capturingSink) Init(logr.RuntimeInfo) {} -func (s *capturingSink) Enabled(int) bool { return true } +func (s *capturingSink) Init(logr.RuntimeInfo) {} +func (s *capturingSink) Enabled(int) bool { return true } func (s *capturingSink) Error(error, string, ...any) {} func (s *capturingSink) Info(_ int, msg string, _ ...any) {