[VC-43753] CyberArk(client): add CyberArk snapshot conversion#684
Merged
wallrj-cyberark merged 2 commits intomasterfrom Sep 2, 2025
Merged
[VC-43753] CyberArk(client): add CyberArk snapshot conversion#684wallrj-cyberark merged 2 commits intomasterfrom
wallrj-cyberark merged 2 commits intomasterfrom
Conversation
wallrj-cyberark
commented
Aug 9, 2025
inteon
reviewed
Aug 12, 2025
9606752 to
4d8e691
Compare
88c1cc7 to
8b9a233
Compare
2d44e46 to
34d67da
Compare
inteon
reviewed
Aug 21, 2025
caadaf0 to
e8f50da
Compare
78700c9 to
b7adba8
Compare
d82113d to
8842333
Compare
e8f50da to
7686607
Compare
8842333 to
5a2ae3c
Compare
ee3d84d to
a04cdad
Compare
a04cdad to
00e4e91
Compare
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR adds CyberArk snapshot conversion functionality to process DataReading objects into the snapshot format expected by the CyberArk API.
- Introduced data conversion logic with
convertDataReadingsand extractor functions for processing different resource types - Updated type definitions to use
runtime.Objectinstead ofunstructured.Unstructuredfor better type safety - Enhanced data gatherers to return strongly typed data structures (
DynamicDataandDiscoveryData)
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/client/client_cyberark.go | Added conversion logic and extractor functions for transforming DataReadings to CyberArk snapshots |
| pkg/client/client_cyberark_convertdatareadings_test.go | Added comprehensive test coverage for data conversion functions |
| pkg/client/client_cyberark_test.go | Added helper function for generating test data and updated test cases |
| pkg/internal/cyberark/dataupload/dataupload.go | Changed Snapshot field types from unstructured.Unstructured to runtime.Object |
| pkg/internal/cyberark/dataupload/dataupload_test.go | Updated test to use version constant instead of hardcoded value |
| pkg/internal/cyberark/dataupload/mock.go | Added assertions for cluster ID and agent version validation |
| pkg/datagatherer/k8s/discovery.go | Refactored to return strongly typed DiscoveryData instead of generic map |
| pkg/datagatherer/k8s/dynamic.go | Refactored to return strongly typed DynamicData instead of generic map |
| pkg/datagatherer/k8s/dynamic_test.go | Updated tests to work with new strongly typed return values |
| api/datareading.go | Added DynamicData and DiscoveryData type definitions |
| examples/machinehub.yaml | Updated configuration to include comprehensive resource gathering definitions |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
00e4e91 to
badb795
Compare
wallrj-cyberark
commented
Sep 2, 2025
wallrj-cyberark
commented
Sep 2, 2025
badb795 to
f0a0233
Compare
inteon
reviewed
Sep 2, 2025
f0a0233 to
1970988
Compare
inteon
reviewed
Sep 2, 2025
- Introduced `convertDataReadings` to process `DataReading` objects into snapshots. - Added support for extracting Kubernetes server version and dynamic resources. - Updated `CyberArkClient` to use the new data conversion logic. - Refactored `DiscoveryData` and `DynamicData` structures for better type safety. - Replaced `unstructured.Unstructured` with `runtime.Object` in `Snapshot` fields. - Enhanced `DataGathererDiscovery` and `DataGathererDynamic` to return strongly typed data. - Added unit tests for new data extraction and conversion functions. Signed-off-by: Richard Wall <richard.wall@cyberark.com>
- Added `ClusterID` field to `DiscoveryData` to store the unique Kubernetes cluster identifier derived from the `kube-system` namespace UID. - Updated `DataGathererDiscovery` to fetch and store the cluster ID during initialization. - Refactored `extractServerVersionFromReading` to `extractClusterIDAndServerVersionFromReading` to handle both cluster ID and server version extraction. - Removed `clusteruid` package as its functionality is now integrated into the `discovery` data gatherer. - Updated unit tests to validate the inclusion of `ClusterID` in snapshots. Signed-off-by: Richard Wall <richard.wall@cyberark.com>
1970988 to
6dd5dc2
Compare
inteon
approved these changes
Sep 2, 2025
Contributor
|
The code looks good to me, very clean! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add CyberArk snapshot conversion
Commit 1
convertDataReadingsto processDataReadingobjects into snapshots.CyberArkClientto use the new data conversion logic.DiscoveryDataandDynamicDatastructures for better type safety.unstructured.Unstructuredwithruntime.ObjectinSnapshotfields.DataGathererDiscoveryandDataGathererDynamicto return strongly typed data.Commit 2
ClusterIDfield toDiscoveryDatato store the unique Kubernetescluster identifier derived from the
kube-systemnamespace UID.DataGathererDiscoveryto fetch and store the cluster ID duringinitialization.
extractServerVersionFromReadingtoextractClusterIDAndServerVersionFromReadingto handle both cluster ID and server version extraction.
clusteruidpackage as its functionality is now integrated intothe
discoverydata gatherer.ClusterIDin snapshots.Part of: https://venafi.atlassian.net/browse/VC-43753
Followup PRs
Testing
I re-ran the e2e test script to check that new ClusterID field in the discovery datagatherer doesn't cause any problems for the TLSPK backend:
$ ./hack/e2e/test.sh ... { "ts": 1756801135013.2488, "caller": "transport/round_trippers.go:632", "msg": "Response", "v": 6, "logger": "Run.gatherAndOutputData.postData", "verb": "POST", "url": "https://api.venafi.cloud/v1/tlspk/upload/clusterdata/no?description=QSBraW5kIGNsdXN0ZXIgdXNlZCBmb3IgdGVzdGluZyB0aGUgdmVuYWZpLWt1YmVybmV0ZXMtYWdlbnQuCg&name=venafi-kubernetes-agent-e2e", "status": "200 OK", "milliseconds": 343 } {"ts":1756801135081.4727,"caller":"agent/run.go:417","msg":"Data sent successfully","v":0,"logger":"Run.gatherAndOutputData.postData"} ... + jq 'if .count == 0 then . | halt_error(1) end' { "count": 1, "certificates": [ ... "certificateName": "venafi-kubernetes-agent-e2e.9bc9b2b7-1674-4fc2-93cb-04bee1b27b28", ... } + exit 0