Add support for ESO resources in disco-agent#780
Conversation
Signed-off-by: Ashley Davis <ashley.davis@cyberark.com>
There was a problem hiding this comment.
Pull request overview
Adds collection and upload support for External Secrets Operator (ESO) resources in the disco-agent, primarily to enable end-to-end test coverage for these CRDs.
Changes:
- Add new default k8s-dynamic data gatherers for ESO resources (ExternalSecret/SecretStore and cluster-scoped variants) in the Helm chart and examples.
- Extend the CyberArk snapshot/extractor pipeline to include ESO resources, with new unit tests for conversion behavior.
- Update e2e script and add sample ESO manifests to exercise discovery in a test cluster.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/client/client_cyberark.go | Adds extractor mappings for ESO gatherers into snapshot fields. |
| internal/cyberark/dataupload/dataupload.go | Extends Snapshot schema with ESO resource lists. |
| pkg/client/client_cyberark_test.go | Updates default dynamic gatherer name list used by tests/fakes. |
| pkg/client/client_cyberark_convertdatareadings_test.go | Adds conversion tests for ESO resources and deleted-item filtering. |
| deploy/charts/disco-agent/templates/configmap.yaml | Enables ESO gatherers by default in the chart config. |
| deploy/charts/disco-agent/tests/snapshot/configmap_test.yaml.snap | Updates Helm snapshot expectations for new gatherers. |
| examples/machinehub.yaml | Adds ESO gatherers to the MachineHub example config. |
| examples/machinehub/input.json | Adds empty ESO readings to the MachineHub example input. |
| hack/ark/test-e2e.sh | Installs ESO via Helm and applies sample ESO CRs for e2e. |
| hack/ark/secret-store.yaml | Adds sample SecretStore manifest for e2e. |
| hack/ark/external-secret.yaml | Adds sample ExternalSecret manifest for e2e. |
| hack/ark/cluster-secret-store.yaml | Adds sample ClusterSecretStore manifest for e2e. |
| hack/ark/cluster-external-secret.yaml | Adds sample ClusterExternalSecret manifest for e2e. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # | ||
| # This is required for the agent to discover ExternalSecret and SecretStore resources. | ||
| echo "Installing External Secrets Operator..." | ||
| helm repo add external-secrets https://charts.external-secrets.io |
There was a problem hiding this comment.
helm repo add external-secrets ... is not idempotent and will exit non-zero if the repo already exists, which breaks reruns because the script is running with set -o errexit. Use --force-update (or otherwise handle the "already exists" case) so the e2e script can be re-run reliably.
| helm repo add external-secrets https://charts.external-secrets.io | |
| helm repo add external-secrets https://charts.external-secrets.io --force-update |
| - 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 |
There was a problem hiding this comment.
These ESO CRD gatherers are enabled by default in the chart, but the chart RBAC currently only grants explicit access to core Secrets/RBAC plus the built-in view role. Custom resources like external-secrets.io/* typically require explicit RBAC (unless the CRDs aggregate into view), otherwise the informer will fail to list/watch and the gatherer will stay empty. Please add explicit ClusterRole rules for externalsecrets, secretstores, clusterexternalsecrets, and clustersecretstores in external-secrets.io (or make these gatherers opt-in).
There was a problem hiding this comment.
The CRD does aggregate into view
| helm repo add external-secrets https://charts.external-secrets.io | ||
| helm repo update | ||
| helm upgrade --install external-secrets \ | ||
| external-secrets/external-secrets \ | ||
| --namespace external-secrets-system \ | ||
| --create-namespace \ | ||
| --wait \ | ||
| --set installCRDs=true |
There was a problem hiding this comment.
This script installs the external-secrets/external-secrets Helm chart from the public https://charts.external-secrets.io repository without pinning to a specific immutable version or verifying integrity, so each test run may execute arbitrary code if the chart repository or DNS is compromised. Because the chart deploys a controller with cluster-level RBAC, a malicious or hijacked chart could access Kubernetes secrets (including the agent-credentials secret created earlier) and other cluster resources. Pin the chart to a specific version or digest and, where possible, add integrity verification to reduce this supply-chain risk.
wallrj-cyberark
left a comment
wallrj-cyberark
left a comment
There was a problem hiding this comment.
Thanks. I think this all looks ok. And I can see that the original PR had multiple rounds of review and improvement.
/approve
|
Thanks a lot everyone! |
|
NB: followup PR to fix permissions has now merged too: #781 |
This is re-raised from #775 to enable e2e tests to run (because of how the repo is set up - in part for security reasons - the e2e tests don't run on PRs from forks)
All of the original work is by @EldarShalev - I've squashed the commits and pushed this branch.