[VC-45804] Add cluster name and description to CyberArk Discovery and Context snapshot #730
Conversation
wallrj-cyberark changed the title wallrj-cyberark force-pushed the ark-cluster-name-and-description branch 5 times, most recently from 8160f9f to 2534d5c Compare 
There was a problem hiding this comment.
Pull Request Overview
This PR adds cluster name and description metadata to CyberArk Discovery and Context snapshots to help operators associate uploaded findings with teams or contacts. The key change introduces new optional configuration fields that provide better identification of Kubernetes clusters in security reports.
- Add
ClusterNameandClusterDescriptionfields to data upload snapshots - Implement backward compatibility with existing
cluster_idconfiguration - Default cluster name to
ARK_USERNAMEenvironment variable when not explicitly set
Reviewed Changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/client/client_cyberark.go | Refactors snapshot creation to use new baseSnapshotFromOptions function with cluster metadata |
| pkg/client/client.go | Updates Options struct comments to clarify field usage across different modes |
| pkg/agent/run.go | Changes postData to use ClusterName instead of ClusterID field |
| pkg/agent/config.go | Adds ClusterName/ClusterDescription config fields and validation logic with ARK_USERNAME fallback |
| pkg/agent/config_test.go | Updates tests to use new ClusterName field and adds MachineHub mode test cases |
| internal/cyberark/dataupload/dataupload.go | Adds ClusterName and ClusterDescription fields to Snapshot struct |
| deploy/charts/venafi-kubernetes-agent/templates/configmap.yaml | Changes template to use cluster_name instead of cluster_id |
| deploy/charts/disco-agent/values.yaml | Adds clusterName and clusterDescription Helm values with documentation |
| deploy/charts/disco-agent/values.schema.json | Adds schema definitions for new cluster configuration fields |
| deploy/charts/disco-agent/templates/configmap.yaml | Updates ConfigMap template to include new cluster fields |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
wallrj-cyberark force-pushed the ark-cluster-name-and-description branch 2 times, most recently from 0e764e4 to f0fe260 Compare 
There was a problem hiding this comment.
Generated by make test-helm-snapshot
There was a problem hiding this comment.
Autogenerated by make ark-generate.
There was a problem hiding this comment.
Autogenerated by make ark-generate.
| } | ||
| if cfg.ClusterID != "" { | ||
| log.Info(fmt.Sprintf(`Ignoring the cluster_id field in the config file. This field is not needed in %s mode.`, res.OutputMode)) | ||
| } |
There was a problem hiding this comment.
Added this additional info log in case anyone some how sets one of these unused fields in their disco-agent configmap, or local config file. Very unlikely.
| data: | ||
| config.yaml: |- | ||
| cluster_id: {{ .Values.config.clusterName | quote }} | ||
| cluster_name: {{ .Values.config.clusterName | quote }} |
There was a problem hiding this comment.
I worried that this might be backwards incompatible; I worried that if the user rolls back the agent upgrade they'll be running an old version of the agent and it will fail to parse this new config file field, but actually I think it's safe. If the user has populated the configmap using Helm template and values, then if they roll back the helm release, it should revert to the previous version of the configmap.
| If your configuration contains `cluster_id`, it will continue to work as a | ||
| fallback, but please migrate to `cluster_name` to avoid ambiguity. | ||
| {{- end }} | ||
| |
There was a problem hiding this comment.
I've added a snapshot test for this using matchSnapshotRaw. See:
| clusterName: venafi-kubernetes-agent-e2e | ||
| clusterDescription: | | ||
| A kind cluster used for testing the venafi-kubernetes-agent. | ||
| A cluster used for testing the venafi-kubernetes-agent. |
There was a problem hiding this comment.
This description was out of date. The test runs on GKE.
…apshot The purpose of the change is to give Discovery and Context service operators a clear way to communicate uploaded secret findings and remediations with a team or contact for the origin cluster, by allowing the platform-team to supply these new configuration values when they deploy the agent: - `config.clusterName` — a human‑readable cluster name, and - `config.clusterDescription` — a short description (contact info, purpose). These new fields are optional. If the clusterName is empty, the `ARK_USERNAME` is used as the cluster-name. The rationale is that each agent deployment will be assigned a unique "service account" which should be given a username derived from the name of the target cluster. Therefore the service account username will be sufficient information for the security team to communicate risks and remediations to the platform team responsible for the cluster. This provides an imperfect, but expedient improvement for Web UI users and support for on‑prem / non‑cloud deployments which can be improved in future if with more backend/ cloud discovery work. It wasn't strictly necessary, but I also tried to sort out the confusion around the `cluster_id` and the `cluster_name`. I've added a new `cluster_name` field to the config file and updated the `venafi-kubernetes-agent` chart to set that config field instead of the overloading the `cluster_id` field which is used for other purposes by the much older Jetstack Secure agent. Summary of changes: - Add ClusterName and ClusterDescription fields to Snapshot struct - Populate these fields from Options in PostDataReadingsWithOptions - Add clusterName and clusterDescription Helm values and docs - Populate cluster_id and cluster_description in the rendered configmap - Update values.schema.json to include descriptions for the new values - Add ClusterDescription field to pkg/agent Config and CombinedConfig - Default MachineHub cluster name from ARK_USERNAME env when not set Signed-off-by: Richard Wall <richard.wall@cyberark.com>
This comment was marked as outdated.
This comment was marked as outdated.
wallrj-cyberark force-pushed the ark-cluster-name-and-description branch from f0fe260 to 9dc744c Compare There was a problem hiding this comment.
Generated by make test-helm-snapshot
| configmap: | ||
| name: agent-custom-config | ||
| asserts: | ||
| - matchSnapshotRaw: {} |
There was a problem hiding this comment.
matchSnapshotRaw will also check the NOTES.txt file.
The snapshot contains the expected output:
wallrj-cyberark force-pushed the ark-cluster-name-and-description branch from 9dc744c to a8f7fe8 Compare 
Fixes: https://venafi.atlassian.net/browse/VC-45804
The purpose of the change is to give Discovery and Context service operators a clear way to communicate uploaded
secret findings and remediations with a team or contact for the origin cluster, by allowing the platform-team to supply these new configuration values when they deploy the agent:
config.clusterName— a human‑readable cluster name, andconfig.clusterDescription— a short description (contact info, purpose).These new fields are optional. If the clusterName is empty, the
ARK_USERNAMEis used as the cluster-name. The rationale is that each agent deployment will be
assigned a unique "service account" which should be given a username derived
from the name of the target cluster. Therefore the service account username will
be sufficient information for the security team to communicate risks and
remediations to the platform team responsible for the cluster.
This provides an imperfect, but expedient improvement for Web UI users and
support for on‑prem / non‑cloud deployments which can be improved in future if
with more backend/ cloud discovery work.
It wasn't strictly necessary, but I also tried to sort out the confusion around
the
cluster_idand thecluster_name. I've added a newcluster_namefieldto the config file and updated the
venafi-kubernetes-agentchart to set thatconfig field instead of the overloading the
cluster_idfield which is used forother purposes by the much older Jetstack Secure agent.
Summary of changes:
Testing
Test release:
I ran the E2E tests manually and they passed: