From daa30101648f7d8b2d425aa429a3641bbaef72cc Mon Sep 17 00:00:00 2001 From: Ashley Davis Date: Wed, 22 Apr 2026 10:52:33 +0100 Subject: [PATCH 1/3] force TSG IDs to be strings Signed-off-by: Ashley Davis --- deploy/charts/discovery-agent/README.md | 4 ++-- deploy/charts/discovery-agent/values.schema.json | 3 ++- deploy/charts/discovery-agent/values.yaml | 6 +++--- 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/deploy/charts/discovery-agent/README.md b/deploy/charts/discovery-agent/README.md index 378c5397..9808f4b4 100644 --- a/deploy/charts/discovery-agent/README.md +++ b/deploy/charts/discovery-agent/README.md @@ -6,13 +6,13 @@ The Discovery Agent connects your Kubernetes or OpenShift cluster to Palo Alto N -#### **config.tsgID** ~ `string,number` +#### **config.tsgID** ~ `string` > Default value: > ```yaml > "" > ``` -Required: The TSG (Tenant Service Group) ID to use when connecting to SCM. NB: TSG IDs are numeric, but should be treated as strings. If being set with the Helm CLI prefer `--set-string`. +Required: The TSG (Tenant Service Group) ID to use when connecting to SCM. NB: TSG IDs are numeric, but must be treated as strings to avoid issues with YAML data types. With the Helm CLI use `--set-string`; with YAML always pass TSG IDs in double quotes. #### **config.clusterName** ~ `string` diff --git a/deploy/charts/discovery-agent/values.schema.json b/deploy/charts/discovery-agent/values.schema.json index a6c6cb28..804d6986 100644 --- a/deploy/charts/discovery-agent/values.schema.json +++ b/deploy/charts/discovery-agent/values.schema.json @@ -167,7 +167,8 @@ }, "helm-values.config.tsgID": { "default": "", - "description": "Required: The TSG (Tenant Service Group) ID to use when connecting to SCM. NB: TSG IDs are numeric, but should be treated as strings. If being set with the Helm CLI prefer `--set-string`." + "description": "Required: The TSG (Tenant Service Group) ID to use when connecting to SCM. NB: TSG IDs are numeric, but must be treated as strings to avoid issues with YAML data types. With the Helm CLI use `--set-string`; with YAML always pass TSG IDs in double quotes.", + "type": "string" }, "helm-values.extraArgs": { "default": [], diff --git a/deploy/charts/discovery-agent/values.yaml b/deploy/charts/discovery-agent/values.yaml index 454c2a30..504f9261 100644 --- a/deploy/charts/discovery-agent/values.yaml +++ b/deploy/charts/discovery-agent/values.yaml @@ -1,10 +1,10 @@ # Configuration for the Discovery Agent config: # Required: The TSG (Tenant Service Group) ID to use when connecting to SCM. - # NB: TSG IDs are numeric, but should be treated as strings. - # If being set with the Helm CLI prefer `--set-string`. + # NB: TSG IDs are numeric, but must be treated as strings to avoid issues with YAML data types. + # With the Helm CLI use `--set-string`; with YAML always pass TSG IDs in double quotes. # +docs:property - # +docs:type=string,number + # +docs:type=string tsgID: "" # Required: A human readable name for the cluster into which the agent is being deployed. From 65a006c223cec5c3875451a716ac499eefe92388 Mon Sep 17 00:00:00 2001 From: Ashley Davis Date: Wed, 22 Apr 2026 11:37:48 +0100 Subject: [PATCH 2/3] add several AI-generated helm unittests I mostly wanted to add tests for the TSG ID, but there are plenty of tests here which are going to be useful and shouldn't cause any harm Signed-off-by: Ashley Davis --- .../discovery-agent/tests/configmap_test.yaml | 91 +++++ .../tests/deployment_test.yaml | 330 ++++++++++++++++++ .../tests/poddisruptionbudget_test.yaml | 102 ++++++ .../tests/podmonitor_test.yaml | 162 +++++++++ .../discovery-agent/tests/rbac_test.yaml | 183 ++++++++++ .../tests/serviceaccount_test.yaml | 78 +++++ 6 files changed, 946 insertions(+) create mode 100644 deploy/charts/discovery-agent/tests/configmap_test.yaml create mode 100644 deploy/charts/discovery-agent/tests/deployment_test.yaml create mode 100644 deploy/charts/discovery-agent/tests/poddisruptionbudget_test.yaml create mode 100644 deploy/charts/discovery-agent/tests/podmonitor_test.yaml create mode 100644 deploy/charts/discovery-agent/tests/rbac_test.yaml create mode 100644 deploy/charts/discovery-agent/tests/serviceaccount_test.yaml diff --git a/deploy/charts/discovery-agent/tests/configmap_test.yaml b/deploy/charts/discovery-agent/tests/configmap_test.yaml new file mode 100644 index 00000000..76d0acfd --- /dev/null +++ b/deploy/charts/discovery-agent/tests/configmap_test.yaml @@ -0,0 +1,91 @@ +suite: test configmap +templates: + - configmap.yaml + +tests: + # Test basic ConfigMap rendering + - it: should create ConfigMap with required values + set: + config.clusterName: my-test-cluster + config.tsgID: "123456" + asserts: + - isKind: + of: ConfigMap + - equal: + path: metadata.name + value: RELEASE-NAME-discovery-agent-config + - matchRegex: + path: data["config.yaml"] + pattern: 'cluster_name: "my-test-cluster"' + + # Test cluster description + - it: should include cluster description when set + set: + config.clusterName: test-cluster + config.tsgID: "123456" + config.clusterDescription: "This is a test cluster" + asserts: + - matchRegex: + path: data["config.yaml"] + pattern: 'cluster_description: "This is a test cluster"' + + # Test period configuration + - it: should set custom period + set: + config.clusterName: test-cluster + config.tsgID: "123456" + config.period: "0h5m0s" + asserts: + - matchRegex: + path: data["config.yaml"] + pattern: 'period: "0h5m0s"' + + # Test exclude annotation keys regex + - it: should include excludeAnnotationKeysRegex when set + set: + config.clusterName: test-cluster + config.tsgID: "123456" + config.excludeAnnotationKeysRegex: + - "^kapp\\.k14s\\.io/original.*" + - "^kubectl\\.kubernetes\\.io/.*" + asserts: + - matchRegex: + path: data["config.yaml"] + pattern: 'exclude-annotation-keys-regex:' + - matchRegex: + path: data["config.yaml"] + pattern: '\^kapp\\\.k14s\\\.io/original\.\*' + + # Test exclude label keys regex + - it: should include excludeLabelKeysRegex when set + set: + config.clusterName: test-cluster + config.tsgID: "123456" + config.excludeLabelKeysRegex: + - "^helm\\.sh/.*" + asserts: + - matchRegex: + path: data["config.yaml"] + pattern: 'exclude-label-keys-regex:' + - matchRegex: + path: data["config.yaml"] + pattern: '\^helm\\\.sh/\.\*' + + # Test data-gatherers are present + - it: should include all data-gatherers + set: + config.clusterName: test-cluster + config.tsgID: "123456" + asserts: + - matchRegex: + path: data["config.yaml"] + pattern: 'kind: k8s-discovery' + - matchRegex: + path: data["config.yaml"] + pattern: 'name: k8s/secrets' + - matchRegex: + path: data["config.yaml"] + pattern: 'name: k8s/jobs' + - matchRegex: + path: data["config.yaml"] + pattern: 'name: k8s/deployments' diff --git a/deploy/charts/discovery-agent/tests/deployment_test.yaml b/deploy/charts/discovery-agent/tests/deployment_test.yaml new file mode 100644 index 00000000..b47bc42c --- /dev/null +++ b/deploy/charts/discovery-agent/tests/deployment_test.yaml @@ -0,0 +1,330 @@ +suite: test deployment +templates: + - deployment.yaml + +tests: + # Test that tsgID is rendered correctly as a string + - it: tsgID is rendered as string in deployment args + set: + config.clusterName: test-cluster + config.tsgID: "987654321" + template: deployment.yaml + asserts: + - isKind: + of: Deployment + - contains: + path: spec.template.spec.containers[0].args + content: --tsg-id + - contains: + path: spec.template.spec.containers[0].args + content: "987654321" + + # Test that tsgID preserves leading zeros (only possible with string type) + # NB: TSG IDs are defined to start with "1", but this test is defence in depth + - it: tsgID preserves leading zeros when provided as string + set: + config.clusterName: test-cluster + config.tsgID: "0001234" + template: deployment.yaml + asserts: + - isKind: + of: Deployment + - contains: + path: spec.template.spec.containers[0].args + content: --tsg-id + - contains: + path: spec.template.spec.containers[0].args + content: "0001234" + + # Test basic deployment rendering with all required values + - it: deployment templates correctly with required values + set: + config.clusterName: my-test-cluster + config.tsgID: "123456" + template: deployment.yaml + asserts: + - isKind: + of: Deployment + - matchRegex: + path: metadata.name + pattern: ^.*-discovery-agent$ + + # Test replica count + - it: should set replica count correctly + set: + config.clusterName: test-cluster + config.tsgID: "123456" + replicaCount: 3 + asserts: + - equal: + path: spec.replicas + value: 3 + + # Test security contexts + - it: should apply pod security context + set: + config.clusterName: test-cluster + config.tsgID: "123456" + podSecurityContext: + fsGroup: 2000 + asserts: + - equal: + path: spec.template.spec.securityContext.fsGroup + value: 2000 + + - it: should apply container security context defaults + set: + config.clusterName: test-cluster + config.tsgID: "123456" + asserts: + - equal: + path: spec.template.spec.containers[0].securityContext.readOnlyRootFilesystem + value: true + - equal: + path: spec.template.spec.containers[0].securityContext.runAsNonRoot + value: true + - equal: + path: spec.template.spec.containers[0].securityContext.allowPrivilegeEscalation + value: false + + # Test resources + - it: should set resources when specified + set: + config.clusterName: test-cluster + config.tsgID: "123456" + resources: + limits: + cpu: 200m + memory: 256Mi + requests: + cpu: 100m + memory: 128Mi + asserts: + - equal: + path: spec.template.spec.containers[0].resources.limits.cpu + value: 200m + - equal: + path: spec.template.spec.containers[0].resources.requests.memory + value: 128Mi + + # Test environment variables + - it: should set HTTP_PROXY environment variable + set: + config.clusterName: test-cluster + config.tsgID: "123456" + http_proxy: "http://proxy:8080" + asserts: + - contains: + path: spec.template.spec.containers[0].env + content: + name: HTTP_PROXY + value: "http://proxy:8080" + + - it: should set HTTPS_PROXY environment variable + set: + config.clusterName: test-cluster + config.tsgID: "123456" + https_proxy: "https://proxy:8443" + asserts: + - contains: + path: spec.template.spec.containers[0].env + content: + name: HTTPS_PROXY + value: "https://proxy:8443" + + - it: should set NO_PROXY environment variable + set: + config.clusterName: test-cluster + config.tsgID: "123456" + no_proxy: "127.0.0.1,localhost" + asserts: + - contains: + path: spec.template.spec.containers[0].env + content: + name: NO_PROXY + value: "127.0.0.1,localhost" + + # Test command line arguments + - it: should include metrics flag when enabled + set: + config.clusterName: test-cluster + config.tsgID: "123456" + metrics.enabled: true + asserts: + - contains: + path: spec.template.spec.containers[0].args + content: --enable-metrics + + - it: should include pprof flag when enabled + set: + config.clusterName: test-cluster + config.tsgID: "123456" + pprof.enabled: true + asserts: + - contains: + path: spec.template.spec.containers[0].args + content: --enable-pprof + + - it: should include custom server URL when set + set: + config.clusterName: test-cluster + config.tsgID: "123456" + config.serverURL: "https://custom.example.com" + asserts: + - contains: + path: spec.template.spec.containers[0].args + content: --ngts-server-url + - contains: + path: spec.template.spec.containers[0].args + content: "https://custom.example.com" + + - it: should include client ID when set + set: + config.clusterName: test-cluster + config.tsgID: "123456" + config.clientID: "test-client-id" + asserts: + - contains: + path: spec.template.spec.containers[0].args + content: --client-id + - contains: + path: spec.template.spec.containers[0].args + content: test-client-id + + - it: should include extra args + set: + config.clusterName: test-cluster + config.tsgID: "123456" + extraArgs: + - --log-level=6 + - --custom-flag=value + asserts: + - contains: + path: spec.template.spec.containers[0].args + content: "--log-level=6" + - contains: + path: spec.template.spec.containers[0].args + content: "--custom-flag=value" + + # Test volumes and volume mounts + - it: should mount config and credentials volumes + set: + config.clusterName: test-cluster + config.tsgID: "123456" + asserts: + - contains: + path: spec.template.spec.containers[0].volumeMounts + content: + name: config + mountPath: "/etc/discovery-agent" + readOnly: true + - contains: + path: spec.template.spec.containers[0].volumeMounts + content: + name: credentials + mountPath: "/etc/discovery-agent/credentials" + readOnly: true + - contains: + path: spec.template.spec.volumes + content: + name: config + configMap: + name: RELEASE-NAME-discovery-agent-config + optional: false + + - it: should use custom secret name + set: + config.clusterName: test-cluster + config.tsgID: "123456" + config.secretName: custom-secret + asserts: + - contains: + path: spec.template.spec.volumes + content: + name: credentials + secret: + secretName: custom-secret + optional: false + + # Test pod annotations and labels + - it: should apply pod annotations + set: + config.clusterName: test-cluster + config.tsgID: "123456" + podAnnotations: + annotation-key: annotation-value + asserts: + - equal: + path: spec.template.metadata.annotations.annotation-key + value: annotation-value + + - it: should apply pod labels + set: + config.clusterName: test-cluster + config.tsgID: "123456" + podLabels: + custom-label: label-value + asserts: + - equal: + path: spec.template.metadata.labels.custom-label + value: label-value + + # Test node selector, tolerations, and affinity + - it: should apply node selector + set: + config.clusterName: test-cluster + config.tsgID: "123456" + nodeSelector: + disktype: ssd + asserts: + - equal: + path: spec.template.spec.nodeSelector.disktype + value: ssd + + - it: should apply tolerations + set: + config.clusterName: test-cluster + config.tsgID: "123456" + tolerations: + - key: "key1" + operator: "Equal" + value: "value1" + effect: "NoSchedule" + asserts: + - contains: + path: spec.template.spec.tolerations + content: + key: "key1" + operator: "Equal" + value: "value1" + effect: "NoSchedule" + + - it: should apply affinity + set: + config.clusterName: test-cluster + config.tsgID: "123456" + affinity: + nodeAffinity: + requiredDuringSchedulingIgnoredDuringExecution: + nodeSelectorTerms: + - matchExpressions: + - key: kubernetes.io/hostname + operator: In + values: + - node1 + asserts: + - isNotEmpty: + path: spec.template.spec.affinity.nodeAffinity + + # Test image pull secrets + - it: should apply image pull secrets + set: + config.clusterName: test-cluster + config.tsgID: "123456" + imagePullSecrets: + - name: my-secret + asserts: + - contains: + path: spec.template.spec.imagePullSecrets + content: + name: my-secret diff --git a/deploy/charts/discovery-agent/tests/poddisruptionbudget_test.yaml b/deploy/charts/discovery-agent/tests/poddisruptionbudget_test.yaml new file mode 100644 index 00000000..13cb044d --- /dev/null +++ b/deploy/charts/discovery-agent/tests/poddisruptionbudget_test.yaml @@ -0,0 +1,102 @@ +suite: test poddisruptionbudget +templates: + - poddisruptionbudget.yaml + +tests: + # Test PodDisruptionBudget is not created by default + - it: should not create PodDisruptionBudget when disabled + set: + config.clusterName: test-cluster + config.tsgID: "123456" + podDisruptionBudget.enabled: false + asserts: + - hasDocuments: + count: 0 + + # Test PodDisruptionBudget is created when enabled + - it: should create PodDisruptionBudget when enabled + set: + config.clusterName: test-cluster + config.tsgID: "123456" + podDisruptionBudget.enabled: true + asserts: + - isKind: + of: PodDisruptionBudget + - equal: + path: metadata.name + value: RELEASE-NAME-discovery-agent + + # Test default minAvailable when neither minAvailable nor maxUnavailable is set + - it: should set default minAvailable when no disruption values are set + set: + config.clusterName: test-cluster + config.tsgID: "123456" + podDisruptionBudget.enabled: true + asserts: + - equal: + path: spec.minAvailable + value: 1 + - isNull: + path: spec.maxUnavailable + + # Test custom minAvailable + - it: should set custom minAvailable + set: + config.clusterName: test-cluster + config.tsgID: "123456" + podDisruptionBudget.enabled: true + podDisruptionBudget.minAvailable: 2 + asserts: + - equal: + path: spec.minAvailable + value: 2 + - isNull: + path: spec.maxUnavailable + + # Test minAvailable as percentage + - it: should set minAvailable as percentage + set: + config.clusterName: test-cluster + config.tsgID: "123456" + podDisruptionBudget.enabled: true + podDisruptionBudget.minAvailable: "50%" + asserts: + - equal: + path: spec.minAvailable + value: "50%" + + # Test custom maxUnavailable + - it: should set custom maxUnavailable + set: + config.clusterName: test-cluster + config.tsgID: "123456" + podDisruptionBudget.enabled: true + podDisruptionBudget.maxUnavailable: 1 + asserts: + - equal: + path: spec.maxUnavailable + value: 1 + - isNull: + path: spec.minAvailable + + # Test maxUnavailable as percentage + - it: should set maxUnavailable as percentage + set: + config.clusterName: test-cluster + config.tsgID: "123456" + podDisruptionBudget.enabled: true + podDisruptionBudget.maxUnavailable: "25%" + asserts: + - equal: + path: spec.maxUnavailable + value: "25%" + + # Test selector labels + - it: should use correct selector labels + set: + config.clusterName: test-cluster + config.tsgID: "123456" + podDisruptionBudget.enabled: true + asserts: + - isNotEmpty: + path: spec.selector.matchLabels diff --git a/deploy/charts/discovery-agent/tests/podmonitor_test.yaml b/deploy/charts/discovery-agent/tests/podmonitor_test.yaml new file mode 100644 index 00000000..ff4afc0d --- /dev/null +++ b/deploy/charts/discovery-agent/tests/podmonitor_test.yaml @@ -0,0 +1,162 @@ +suite: test podmonitor +templates: + - podmonitor.yaml + +tests: + # Test PodMonitor is not created by default + - it: should not create PodMonitor when metrics.podmonitor.enabled is false + set: + config.clusterName: test-cluster + config.tsgID: "123456" + metrics.enabled: true + metrics.podmonitor.enabled: false + asserts: + - hasDocuments: + count: 0 + + # Test PodMonitor is not created when metrics are disabled + - it: should not create PodMonitor when metrics.enabled is false + set: + config.clusterName: test-cluster + config.tsgID: "123456" + metrics.enabled: false + metrics.podmonitor.enabled: true + asserts: + - hasDocuments: + count: 0 + + # Test PodMonitor is created when both metrics and podmonitor are enabled + - it: should create PodMonitor when both metrics and podmonitor are enabled + set: + config.clusterName: test-cluster + config.tsgID: "123456" + metrics.enabled: true + metrics.podmonitor.enabled: true + asserts: + - isKind: + of: PodMonitor + - equal: + path: metadata.name + value: RELEASE-NAME-discovery-agent + + # Test PodMonitor namespace defaults to Release namespace + - it: should use Release namespace by default + set: + config.clusterName: test-cluster + config.tsgID: "123456" + metrics.enabled: true + metrics.podmonitor.enabled: true + release: + namespace: my-namespace + asserts: + - equal: + path: metadata.namespace + value: my-namespace + + # Test custom PodMonitor namespace + - it: should use custom namespace when specified + set: + config.clusterName: test-cluster + config.tsgID: "123456" + metrics.enabled: true + metrics.podmonitor.enabled: true + metrics.podmonitor.namespace: monitoring + release: + namespace: default + asserts: + - equal: + path: metadata.namespace + value: monitoring + - contains: + path: spec.namespaceSelector.matchNames + content: default + + # Test prometheus instance label + - it: should set prometheus instance label + set: + config.clusterName: test-cluster + config.tsgID: "123456" + metrics.enabled: true + metrics.podmonitor.enabled: true + metrics.podmonitor.prometheusInstance: custom-prometheus + asserts: + - equal: + path: metadata.labels.prometheus + value: custom-prometheus + + # Test custom labels + - it: should apply custom labels + set: + config.clusterName: test-cluster + config.tsgID: "123456" + metrics.enabled: true + metrics.podmonitor.enabled: true + metrics.podmonitor.labels: + custom-label: custom-value + another-label: another-value + asserts: + - equal: + path: metadata.labels.custom-label + value: custom-value + - equal: + path: metadata.labels.another-label + value: another-value + + # Test custom annotations + - it: should apply custom annotations + set: + config.clusterName: test-cluster + config.tsgID: "123456" + metrics.enabled: true + metrics.podmonitor.enabled: true + metrics.podmonitor.annotations: + custom-annotation: custom-value + asserts: + - equal: + path: metadata.annotations.custom-annotation + value: custom-value + + # Test scrape configuration + - it: should configure scrape interval and timeout + set: + config.clusterName: test-cluster + config.tsgID: "123456" + metrics.enabled: true + metrics.podmonitor.enabled: true + metrics.podmonitor.interval: 30s + metrics.podmonitor.scrapeTimeout: 15s + asserts: + - equal: + path: spec.podMetricsEndpoints[0].interval + value: 30s + - equal: + path: spec.podMetricsEndpoints[0].scrapeTimeout + value: 15s + + # Test honorLabels setting + - it: should set honorLabels correctly + set: + config.clusterName: test-cluster + config.tsgID: "123456" + metrics.enabled: true + metrics.podmonitor.enabled: true + metrics.podmonitor.honorLabels: true + asserts: + - equal: + path: spec.podMetricsEndpoints[0].honorLabels + value: true + + # Test metrics endpoint configuration + - it: should configure metrics endpoint correctly + set: + config.clusterName: test-cluster + config.tsgID: "123456" + metrics.enabled: true + metrics.podmonitor.enabled: true + asserts: + - equal: + path: spec.podMetricsEndpoints[0].port + value: agent-api + - equal: + path: spec.podMetricsEndpoints[0].path + value: /metrics diff --git a/deploy/charts/discovery-agent/tests/rbac_test.yaml b/deploy/charts/discovery-agent/tests/rbac_test.yaml new file mode 100644 index 00000000..9eb08639 --- /dev/null +++ b/deploy/charts/discovery-agent/tests/rbac_test.yaml @@ -0,0 +1,183 @@ +suite: test rbac +templates: + - rbac.yaml + +tests: + # Test that all RBAC resources are created + - it: should create all RBAC resources + set: + config.clusterName: test-cluster + config.tsgID: "123456" + asserts: + - hasDocuments: + count: 8 + + # Test Role for event emission + - it: should create Role for event emission + set: + config.clusterName: test-cluster + config.tsgID: "123456" + documentIndex: 0 + asserts: + - isKind: + of: Role + - equal: + path: metadata.name + value: RELEASE-NAME-discovery-agent-event-emitted + - contains: + path: rules + content: + apiGroups: [""] + resources: ["events"] + verbs: ["create"] + + # Test RoleBinding for event emission + - it: should create RoleBinding for event emission + set: + config.clusterName: test-cluster + config.tsgID: "123456" + documentIndex: 1 + asserts: + - isKind: + of: RoleBinding + - equal: + path: metadata.name + value: RELEASE-NAME-discovery-agent-event-emitted + - equal: + path: roleRef.kind + value: Role + - equal: + path: roleRef.name + value: RELEASE-NAME-discovery-agent-event-emitted + - contains: + path: subjects + content: + kind: ServiceAccount + name: RELEASE-NAME-discovery-agent + namespace: NAMESPACE + + # Test ClusterRoleBinding for cluster viewer + - it: should create ClusterRoleBinding for cluster viewer + set: + config.clusterName: test-cluster + config.tsgID: "123456" + documentIndex: 2 + asserts: + - isKind: + of: ClusterRoleBinding + - equal: + path: metadata.name + value: RELEASE-NAME-discovery-agent-cluster-viewer + - equal: + path: roleRef.kind + value: ClusterRole + - equal: + path: roleRef.name + value: view + - contains: + path: subjects + content: + kind: ServiceAccount + name: RELEASE-NAME-discovery-agent + namespace: NAMESPACE + + # Test ClusterRole for secret reader + - it: should create ClusterRole for secret reader + set: + config.clusterName: test-cluster + config.tsgID: "123456" + documentIndex: 3 + asserts: + - isKind: + of: ClusterRole + - equal: + path: metadata.name + value: RELEASE-NAME-discovery-agent-secret-reader + - contains: + path: rules + content: + apiGroups: [""] + resources: ["secrets"] + verbs: ["get", "list", "watch"] + + # Test ClusterRoleBinding for secret reader + - it: should create ClusterRoleBinding for secret reader + set: + config.clusterName: test-cluster + config.tsgID: "123456" + documentIndex: 4 + asserts: + - isKind: + of: ClusterRoleBinding + - equal: + path: metadata.name + value: RELEASE-NAME-discovery-agent-secret-reader + - equal: + path: roleRef.kind + value: ClusterRole + - equal: + path: roleRef.name + value: RELEASE-NAME-discovery-agent-secret-reader + + # Test ClusterRole for RBAC reader + - it: should create ClusterRole for RBAC reader + set: + config.clusterName: test-cluster + config.tsgID: "123456" + documentIndex: 5 + asserts: + - isKind: + of: ClusterRole + - equal: + path: metadata.name + value: RELEASE-NAME-discovery-agent-rbac-reader + - contains: + path: rules[0].resources + content: roles + - contains: + path: rules[0].resources + content: clusterroles + - contains: + path: rules[0].resources + content: rolebindings + - contains: + path: rules[0].resources + content: clusterrolebindings + + # Test ClusterRoleBinding for RBAC reader + - it: should create ClusterRoleBinding for RBAC reader + set: + config.clusterName: test-cluster + config.tsgID: "123456" + documentIndex: 6 + asserts: + - isKind: + of: ClusterRoleBinding + - equal: + path: metadata.name + value: RELEASE-NAME-discovery-agent-rbac-reader + - equal: + path: roleRef.kind + value: ClusterRole + - equal: + path: roleRef.name + value: RELEASE-NAME-discovery-agent-rbac-reader + + # Test ClusterRoleBinding for OIDC discovery + - it: should create ClusterRoleBinding for OIDC discovery + set: + config.clusterName: test-cluster + config.tsgID: "123456" + documentIndex: 7 + asserts: + - isKind: + of: ClusterRoleBinding + - equal: + path: metadata.name + value: RELEASE-NAME-discovery-agent-oidc-discovery + - equal: + path: roleRef.kind + value: ClusterRole + - equal: + path: roleRef.name + value: system:service-account-issuer-discovery diff --git a/deploy/charts/discovery-agent/tests/serviceaccount_test.yaml b/deploy/charts/discovery-agent/tests/serviceaccount_test.yaml new file mode 100644 index 00000000..5547fa2b --- /dev/null +++ b/deploy/charts/discovery-agent/tests/serviceaccount_test.yaml @@ -0,0 +1,78 @@ +suite: test serviceaccount +templates: + - serviceaccount.yaml + +tests: + # Test ServiceAccount is created by default + - it: should create ServiceAccount when serviceAccount.create is true + set: + config.clusterName: test-cluster + config.tsgID: "123456" + serviceAccount.create: true + asserts: + - isKind: + of: ServiceAccount + - equal: + path: metadata.name + value: RELEASE-NAME-discovery-agent + + # Test ServiceAccount is not created when disabled + - it: should not create ServiceAccount when serviceAccount.create is false + set: + config.clusterName: test-cluster + config.tsgID: "123456" + serviceAccount.create: false + asserts: + - hasDocuments: + count: 0 + + # Test custom ServiceAccount name + - it: should use custom name when serviceAccount.name is set + set: + config.clusterName: test-cluster + config.tsgID: "123456" + serviceAccount.create: true + serviceAccount.name: custom-sa-name + asserts: + - equal: + path: metadata.name + value: custom-sa-name + + # Test automountServiceAccountToken setting + - it: should set automountServiceAccountToken correctly + set: + config.clusterName: test-cluster + config.tsgID: "123456" + serviceAccount.create: true + serviceAccount.automount: false + asserts: + - equal: + path: automountServiceAccountToken + value: false + + - it: should enable automountServiceAccountToken by default + set: + config.clusterName: test-cluster + config.tsgID: "123456" + serviceAccount.create: true + asserts: + - equal: + path: automountServiceAccountToken + value: true + + # Test ServiceAccount annotations + - it: should apply annotations to ServiceAccount + set: + config.clusterName: test-cluster + config.tsgID: "123456" + serviceAccount.create: true + serviceAccount.annotations: + eks.amazonaws.com/role-arn: arn:aws:iam::123456789012:role/my-role + custom-annotation: custom-value + asserts: + - equal: + path: metadata.annotations["eks.amazonaws.com/role-arn"] + value: arn:aws:iam::123456789012:role/my-role + - equal: + path: metadata.annotations.custom-annotation + value: custom-value From 9ed43ba9673a0781dd5962046e8cf48986f0f715 Mon Sep 17 00:00:00 2001 From: Ashley Davis Date: Wed, 22 Apr 2026 12:27:48 +0100 Subject: [PATCH 3/3] support clientId as well as clientID This allows easier migration from the old chart, while also allowing a more consistent style with initialisms Signed-off-by: Ashley Davis --- .../discovery-agent/templates/deployment.yaml | 4 +- .../tests/deployment_test.yaml | 27 ++++++ .../charts/discovery-agent/values.schema.json | 8 ++ deploy/charts/discovery-agent/values.yaml | 7 ++ pkg/client/client_ngts.go | 18 +++- pkg/client/client_ngts_test.go | 90 ++++++++++++++++++- 6 files changed, 146 insertions(+), 8 deletions(-) diff --git a/deploy/charts/discovery-agent/templates/deployment.yaml b/deploy/charts/discovery-agent/templates/deployment.yaml index 3425de01..c2fd4e9a 100644 --- a/deploy/charts/discovery-agent/templates/deployment.yaml +++ b/deploy/charts/discovery-agent/templates/deployment.yaml @@ -78,9 +78,9 @@ spec: - --ngts-server-url - {{ . | quote }} {{- end }} - {{- if .Values.config.clientID }} + {{- if or .Values.config.clientID .Values.config.clientId }} - --client-id - - {{ .Values.config.clientID }} + - {{ .Values.config.clientID | default .Values.config.clientId }} {{- end }} - --private-key-path - /etc/discovery-agent/credentials/privatekey.pem diff --git a/deploy/charts/discovery-agent/tests/deployment_test.yaml b/deploy/charts/discovery-agent/tests/deployment_test.yaml index b47bc42c..d506f9fa 100644 --- a/deploy/charts/discovery-agent/tests/deployment_test.yaml +++ b/deploy/charts/discovery-agent/tests/deployment_test.yaml @@ -191,6 +191,33 @@ tests: path: spec.template.spec.containers[0].args content: test-client-id + - it: should include client ID when clientId is set (lowercase d) + set: + config.clusterName: test-cluster + config.tsgID: "123456" + config.clientId: "test-client-id-lowercase" + asserts: + - contains: + path: spec.template.spec.containers[0].args + content: --client-id + - contains: + path: spec.template.spec.containers[0].args + content: test-client-id-lowercase + + - it: should prefer clientID over clientId when both are set + set: + config.clusterName: test-cluster + config.tsgID: "123456" + config.clientID: "uppercase-takes-precedence" + config.clientId: "lowercase-ignored" + asserts: + - contains: + path: spec.template.spec.containers[0].args + content: --client-id + - contains: + path: spec.template.spec.containers[0].args + content: uppercase-takes-precedence + - it: should include extra args set: config.clusterName: test-cluster diff --git a/deploy/charts/discovery-agent/values.schema.json b/deploy/charts/discovery-agent/values.schema.json index 804d6986..489328b7 100644 --- a/deploy/charts/discovery-agent/values.schema.json +++ b/deploy/charts/discovery-agent/values.schema.json @@ -97,6 +97,9 @@ "clientID": { "$ref": "#/$defs/helm-values.config.clientID" }, + "clientId": { + "$ref": "#/$defs/helm-values.config.clientId" + }, "clusterDescription": { "$ref": "#/$defs/helm-values.config.clusterDescription" }, @@ -129,6 +132,11 @@ "description": "Deprecated: Client ID for the configured service account. The client ID should be provided in the \"clientID\" field of the authentication secret (see config.secretName). This field is provided for compatibility for users migrating from the \"venafi-kubernetes-agent\" chart.", "type": "string" }, + "helm-values.config.clientId": { + "default": "", + "description": "Deprecated: Client ID for the configured service account (alternative to clientID). The client ID should be provided in the \"clientID\" field of the authentication secret (see config.secretName). This field is provided for compatibility for users migrating from the \"venafi-kubernetes-agent\" chart. If both clientID and clientId are set, clientID takes precedence.", + "type": "string" + }, "helm-values.config.clusterDescription": { "default": "", "description": "A short description of the cluster where the agent is deployed (optional).\n\nThis description will be associated with the data that the agent uploads to the backend.", diff --git a/deploy/charts/discovery-agent/values.yaml b/deploy/charts/discovery-agent/values.yaml index 504f9261..50a99596 100644 --- a/deploy/charts/discovery-agent/values.yaml +++ b/deploy/charts/discovery-agent/values.yaml @@ -42,6 +42,13 @@ config: # +docs:property clientID: "" + # Deprecated: Client ID for the configured service account (alternative to clientID). + # The client ID should be provided in the "clientID" field of the authentication secret (see config.secretName). + # This field is provided for compatibility for users migrating from the "venafi-kubernetes-agent" chart. + # If both clientID and clientId are set, clientID takes precedence. + # +docs:hidden + clientId: "" + # The name of the Secret containing the NGTS built-in service account credentials. # The Secret must contain the following key: # - privatekey.pem: PEM-encoded private key for the service account diff --git a/pkg/client/client_ngts.go b/pkg/client/client_ngts.go index 8de67901..145e33c2 100644 --- a/pkg/client/client_ngts.go +++ b/pkg/client/client_ngts.go @@ -158,6 +158,8 @@ func NewNGTSClient(agentMetadata *api.AgentMetadata, credentials *NGTSServiceAcc // LoadClientIDIfNeeded attempts to load the ClientID from a file if it is not already set. // It looks for a "clientID" file in the same directory as the PrivateKeyFile. +// For compatibility with the venafi-kubernetes-agent chart, it also supports "clientId" (lowercase 'd'). +// If both files exist, "clientID" takes precedence. // This allows the ClientID to be provided either as a direct value or via a Kubernetes secret. func (c *NGTSServiceAccountCredentials) LoadClientIDIfNeeded() error { if c == nil { @@ -178,13 +180,21 @@ func (c *NGTSServiceAccountCredentials) LoadClientIDIfNeeded() error { return nil // This is actually a fatal error but will be caught by Validate() later } + baseDir := path.Dir(c.PrivateKeyFile) + // Try to load ClientID from a file in the same directory as the private key - clientIDPath := path.Dir(c.PrivateKeyFile) + "/clientID" + // Try "clientID" first (takes precedence), then "clientId" for backward compatibility + clientIDPath := baseDir + "/clientID" clientIDBytes, err := os.ReadFile(clientIDPath) if err != nil { - // If the file doesn't exist, that's okay - we'll let Validate() catch the empty ClientID error later - klog.V(2).Info("Could not read clientID from file", "path", clientIDPath, "error", err) - return nil + // Try the alternative "clientId" (lowercase 'd') for compatibility with venafi-kubernetes-agent + clientIDPath = baseDir + "/clientId" + clientIDBytes, err = os.ReadFile(clientIDPath) + if err != nil { + // If neither file exists, that's okay - we'll let Validate() catch the empty ClientID error later + klog.V(2).Info("Could not read clientID from file", "path", clientIDPath, "error", err) + return nil + } } // Trim whitespace from the clientID diff --git a/pkg/client/client_ngts_test.go b/pkg/client/client_ngts_test.go index 06fd027d..2229ea52 100644 --- a/pkg/client/client_ngts_test.go +++ b/pkg/client/client_ngts_test.go @@ -132,12 +132,12 @@ func TestNGTSClient_LoadClientIDFromFile(t *testing.T) { // Create the private key file keyFile := tmpDir + "/privatekey.pem" - err := os.WriteFile(keyFile, []byte(fakePrivKeyPEM), 0600) + err := os.WriteFile(keyFile, []byte(fakePrivKeyPEM), 0o600) require.NoError(t, err) // Create the clientID file in the same directory clientIDFile := tmpDir + "/clientID" - err = os.WriteFile(clientIDFile, []byte("test-client-from-file\n"), 0600) + err = os.WriteFile(clientIDFile, []byte("test-client-from-file\n"), 0o600) require.NoError(t, err) tests := []struct { @@ -187,6 +187,92 @@ func TestNGTSClient_LoadClientIDFromFile(t *testing.T) { } } +func TestNGTSClient_LoadClientIDFromFileAlternativeNames(t *testing.T) { + tests := []struct { + name string + setupFiles func(tmpDir string) string // returns keyFile path + wantClientID string + wantErr bool + wantErrContain string + }{ + { + // Note: venafi-kubernetes-agent didn't support storing the client ID in the secret, but + // we don't want users moving to discovery-agent to be caught out by such a trivial mistake. + name: "load from clientId (lowercase d) for venafi-kubernetes-agent compatibility", + setupFiles: func(tmpDir string) string { + keyFile := tmpDir + "/privatekey.pem" + err := os.WriteFile(keyFile, []byte(fakePrivKeyPEM), 0o600) + require.NoError(t, err) + // Create clientId file (lowercase 'd') + clientIdFile := tmpDir + "/clientId" + err = os.WriteFile(clientIdFile, []byte("test-client-from-clientId\n"), 0o600) + require.NoError(t, err) + return keyFile + }, + wantClientID: "test-client-from-clientId", + wantErr: false, + }, + { + name: "load from clientID (uppercase D)", + setupFiles: func(tmpDir string) string { + keyFile := tmpDir + "/privatekey.pem" + err := os.WriteFile(keyFile, []byte(fakePrivKeyPEM), 0o600) + require.NoError(t, err) + // Create only clientID file (uppercase 'D') + clientIDFile := tmpDir + "/clientID" + err = os.WriteFile(clientIDFile, []byte("from-clientID"), 0o600) + require.NoError(t, err) + return keyFile + }, + wantClientID: "from-clientID", + wantErr: false, + }, + { + name: "error when no clientID file exists", + setupFiles: func(tmpDir string) string { + keyFile := tmpDir + "/privatekey.pem" + err := os.WriteFile(keyFile, []byte(fakePrivKeyPEM), 0o600) + require.NoError(t, err) + // Don't create any clientID file + return keyFile + }, + wantErr: true, + wantErrContain: "client_id cannot be empty", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + tmpDir := t.TempDir() + keyFile := tt.setupFiles(tmpDir) + + credentials := &NGTSServiceAccountCredentials{ + ClientID: "", // Empty - should be loaded from file + PrivateKeyFile: keyFile, + } + + metadata := &api.AgentMetadata{ + Version: "test-version", + ClusterID: "test-cluster", + } + + client, err := NewNGTSClient(metadata, credentials, "https://test.example.com", "test-tsg", nil) + + if tt.wantErr { + require.Error(t, err) + if tt.wantErrContain != "" { + assert.Contains(t, err.Error(), tt.wantErrContain) + } + return + } + + require.NoError(t, err) + assert.NotNil(t, client) + assert.Equal(t, tt.wantClientID, client.credentials.ClientID) + }) + } +} + func TestNGTSClient_PostDataReadingsWithOptions(t *testing.T) { keyFile := withFile(t, fakePrivKeyPEM)