Skip to content

[VC-46370] CyberArk: Skip deleted resources when converting data readings to snapshot #741

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
wallrj-cyberark merged 1 commit into from
Oct 28, 2025
Merged

[VC-46370] CyberArk: Skip deleted resources when converting data readings to snapshot #741

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions examples/machinehub/input.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,17 @@
"namespace": "team-1"
}
}
},
{
"deleted_at": "2024-06-10T12:00:00Z",
"resource": {
"kind": "Secret",
"apiVersion": "v1",
"metadata": {
"name": "deleted-secret-1",
"namespace": "team-2"
}
}
Copy link
Copy Markdown
Member Author

@wallrj-cyberark wallrj-cyberark Oct 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file is used by the tests in cmd/agent_test.go.

jetstack-secure/cmd/agent_test.go

Lines 34 to 41 in ca9ae2e

t.Run("machinehub", func(t *testing.T) {
arktesting.SkipIfNoEnv(t)
runSubprocess(t, repoRoot, []string{
"--agent-config-file", filepath.Join(repoRoot, "examples/machinehub/config.yaml"),
"--input-path", filepath.Join(repoRoot, "examples/machinehub/input.json"),
"--machine-hub",
})
})

By capturing the HTTP request using mitmproxy we can see the deleted items are included in the snapshot before the changes to the implementation and absent after the implementation change:

mitmproxy
 make test-unit HTTPS_PROXY=localhost:8080

BEFORE:

tail -1 request.txt  | jq
{
  "agent_version": "development",
  "cluster_id": "0e069229-d83b-4075-a4c8-95838ff5c437",
  "cluster_name": "github-jetstack-secure-tests@cyberark.cloud.420375",
  "k8s_version": "v1.27.6",
  "secrets": [
    {
      "apiVersion": "v1",
      "kind": "Secret",
      "metadata": {
        "name": "app-1-secret-1",
        "namespace": "team-1"
      }
    },
    {
      "apiVersion": "v1",
      "kind": "Secret",
      "metadata": {
        "name": "deleted-secret-1",
        "namespace": "team-2"
      }
    }
  ],
  "serviceaccounts": [],
  "roles": [],
  "clusterroles": [],
  "rolebindings": [],
  "clusterrolebindings": [],
  "jobs": [],
  "cronjobs": [],
  "deployments": [],
  "statefulsets": [],
  "daemonsets": [],
  "pods": [
    {
      "apiVersion": "v1",
      "kind": "Pod",
      "metadata": {
        "name": "app-1-pod-1",
        "namespace": "team-1"
      }
    },
    {
      "apiVersion": "v1",
      "kind": "Pod",
      "metadata": {
        "name": "deleted-pod-1",
        "namespace": "team-2"
      }
    }
  ]
}

AFTER:

tail -1 request-after.txt  | jq
{
  "agent_version": "development",
  "cluster_id": "0e069229-d83b-4075-a4c8-95838ff5c437",
  "cluster_name": "github-jetstack-secure-tests@cyberark.cloud.420375",
  "k8s_version": "v1.27.6",
  "secrets": [
    {
      "apiVersion": "v1",
      "kind": "Secret",
      "metadata": {
        "name": "app-1-secret-1",
        "namespace": "team-1"
      }
    }
  ],
  "serviceaccounts": [],
  "roles": [],
  "clusterroles": [],
  "rolebindings": [],
  "clusterrolebindings": [],
  "jobs": [],
  "cronjobs": [],
  "deployments": [],
  "statefulsets": [],
  "daemonsets": [],
  "pods": [
    {
      "apiVersion": "v1",
      "kind": "Pod",
      "metadata": {
        "name": "app-1-pod-1",
        "namespace": "team-1"
      }
    }
  ]
}

}
]
}
Expand All @@ -38,6 +49,17 @@
"namespace": "team-1"
}
}
},
{
"deleted_at": "2024-06-10T12:00:00Z",
"resource": {
"kind": "Pod",
"apiVersion": "v1",
"metadata": {
"name": "deleted-pod-1",
"namespace": "team-2"
}
}
}
]
}
Expand Down
16 changes: 14 additions & 2 deletions pkg/client/client_cyberark.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ func NewCyberArk(httpClient *http.Client) (*CyberArkClient, error) {

// PostDataReadingsWithOptions uploads data readings to CyberArk.
// It converts the supplied data readings into a snapshot format expected by CyberArk.
// Deleted resources are excluded from the snapshot because they are not needed by CyberArk.
// It then minimizes the snapshot to avoid uploading unnecessary data.
// It initializes a data upload client with the configured HTTP client and credentials,
// then uploads a snapshot.
Expand Down Expand Up @@ -112,6 +113,8 @@ func extractClusterIDAndServerVersionFromReading(reading *api.DataReading, targe
// extractResourceListFromReading converts the opaque data from a DynamicData
// data reading to runtime.Object resources, to allow access to the metadata and
// other kubernetes API fields.
// Deleted resources are skipped because the CyberArk Discovery and Context service
// does not need to see resources that no longer exist.
func extractResourceListFromReading(reading *api.DataReading, target *[]runtime.Object) error {
if reading == nil {
return fmt.Errorf("programmer mistake: the DataReading must not be nil")
Expand All @@ -122,10 +125,13 @@ func extractResourceListFromReading(reading *api.DataReading, target *[]runtime.
"programmer mistake: the DataReading must have data type *api.DynamicData. "+
"This DataReading (%s) has data type %T", reading.DataGatherer, reading.Data)
}
resources := make([]runtime.Object, len(data.Items))
resources := make([]runtime.Object, 0, len(data.Items))
for i, item := range data.Items {
if !item.DeletedAt.IsZero() {
continue
}
if resource, ok := item.Resource.(runtime.Object); ok {
resources[i] = resource
resources = append(resources, resource)
} else {
return fmt.Errorf(
"programmer mistake: the DynamicData items must have Resource type runtime.Object. "+
Expand All @@ -136,6 +142,11 @@ func extractResourceListFromReading(reading *api.DataReading, target *[]runtime.
return nil
}

// defaultExtractorFunctions maps data gatherer names to functions that extract
// their data from DataReadings into the appropriate fields of a Snapshot.
// Each function takes a DataReading and a pointer to a Snapshot,
// and populates the relevant field(s) of the Snapshot based on the DataReading's data.
// Deleted resources are excluded from the snapshot because they are not needed by CyberArk.
var defaultExtractorFunctions = map[string]func(*api.DataReading, *dataupload.Snapshot) error{
"ark/discovery": extractClusterIDAndServerVersionFromReading,
"ark/secrets": func(r *api.DataReading, s *dataupload.Snapshot) error {
Expand Down Expand Up @@ -184,6 +195,7 @@ var defaultExtractorFunctions = map[string]func(*api.DataReading, *dataupload.Sn
// The extractorFunctions map should contain functions for each expected
// DataGatherer name, which will be called with the corresponding DataReading
// and the target snapshot to populate the relevant fields.
// Deleted resources are excluded from the snapshot because they are not needed by CyberArk.
func convertDataReadings(
extractorFunctions map[string]func(*api.DataReading, *dataupload.Snapshot) error,
readings []*api.DataReading,
Expand Down
23 changes: 23 additions & 0 deletions pkg/client/client_cyberark_convertdatareadings_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,6 +218,19 @@ func TestExtractResourceListFromReading(t *testing.T) {
},
},
},
// Deleted resource should be ignored
{
DeletedAt: api.Time{Time: time.Now()},
Resource: &unstructured.Unstructured{
Object: map[string]interface{}{
"kind": "Namespace",
"metadata": map[string]interface{}{
"name": "kube-system",
"uid": "uid-kube-system",
},
},
},
},
Copy link
Copy Markdown
Member Author

@wallrj-cyberark wallrj-cyberark Oct 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An example of how this test fails before the implementation change.

=== FAIL: pkg/client TestExtractResourceListFromReading/happy_path (0.00s)
    client_cyberark_convertdatareadings_test.go:251:
                Error Trace:    /home/richard/projects/jetstack/jetstack-secure/pkg/client/client_cyberark_convertdatareadings_test.go:251
                Error:          "[0xc00051a028 0xc00051a030 0xc00051a038]" should have 2 item(s), but has 3
                Test:           TestExtractResourceListFromReading/happy_path

},
},
},
Expand Down Expand Up @@ -270,6 +283,16 @@ func TestConvertDataReadings(t *testing.T) {
},
},
},
// Deleted secret should be ignored
{
DeletedAt: api.Time{Time: time.Now()},
Resource: &corev1.Secret{
ObjectMeta: metav1.ObjectMeta{
Name: "deleted-1",
Namespace: "team-1",
},
},
},
Copy link
Copy Markdown
Member Author

@wallrj-cyberark wallrj-cyberark Oct 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An example of how this test fails before the implementation change:

=== FAIL: pkg/client TestConvertDataReadings/happy_path (0.00s)
    client_cyberark_convertdatareadings_test.go:372:
                Error Trace:    /home/richard/projects/jetstack/jetstack-secure/pkg/client/client_cyberark_convertdatareadings_test.go:372
                Error:          Not equal:
                                expected: dataupload.Snapshot{AgentVersion:"", ClusterID:"success-cluster-id", ClusterName:"", ClusterDescription:"", K8SVersion:"v1.21.0", Secrets:[]runtime.Object{(*v1.Secret)(0xc000050780)}, ServiceAccounts:[]runtime.Object(nil), Roles:[]runtime.Object(nil), ClusterRoles:[]runtime.Object(nil), RoleBindings:[]runtime.Object(nil), ClusterRoleBindings:[]runtime.Object(nil), Jobs:[]runtime.Object(nil), CronJobs:[]runtime.Object(nil), Deployments:[]runtime.Object(nil), Statefulsets:[]runtime.Object(nil), Daemonsets:[]runtime.Object(nil), Pods:[]runtime.Object(nil)}
                                actual  : dataupload.Snapshot{AgentVersion:"", ClusterID:"success-cluster-id", ClusterName:"", ClusterDescription:"", K8SVersion:"v1.21.0", Secrets:[]runtime.Object{(*v1.Secret)(0xc000050500), (*v1.Secret)(0xc000050640)}, ServiceAccounts:[]runtime.Object(nil), Roles:[]runtime.Object(nil), ClusterRoles:[]runtime.Object(nil), RoleBindings:[]runtime.Object(nil), ClusterRoleBindings:[]runtime.Object(nil), Jobs:[]runtime.Object(nil), CronJobs:[]runtime.Object(nil), Deployments:[]runtime.Object(nil), Statefulsets:[]runtime.Object(nil), Daemonsets:[]runtime.Object(nil), Pods:[]runtime.Object(nil)}

                                Diff:
                                --- Expected
                                +++ Actual
                                @@ -6,3 +6,3 @@
                                  K8SVersion: (string) (len=7) "v1.21.0",
                                - Secrets: ([]runtime.Object) (len=1) {
                                + Secrets: ([]runtime.Object) (len=2) {
                                   (*v1.Secret)({
                                @@ -14,2 +14,35 @@
                                     Name: (string) (len=5) "app-1",
                                +    GenerateName: (string) "",
                                +    Namespace: (string) (len=6) "team-1",
                                +    SelfLink: (string) "",
                                +    UID: (types.UID) "",
                                +    ResourceVersion: (string) "",
                                +    Generation: (int64) 0,
                                +    CreationTimestamp: (v1.Time) {
                                +     Time: (time.Time) {
                                +      wall: (uint64) 0,
                                +      ext: (int64) 0,
                                +      loc: (*time.Location)(<nil>)
                                +     }
                                +    },
                                +    DeletionTimestamp: (*v1.Time)(<nil>),
                                +    DeletionGracePeriodSeconds: (*int64)(<nil>),
                                +    Labels: (map[string]string) <nil>,
                                +    Annotations: (map[string]string) <nil>,
                                +    OwnerReferences: ([]v1.OwnerReference) <nil>,
                                +    Finalizers: ([]string) <nil>,
                                +    ManagedFields: ([]v1.ManagedFieldsEntry) <nil>
                                +   },
                                +   Immutable: (*bool)(<nil>),
                                +   Data: (map[string][]uint8) <nil>,
                                +   StringData: (map[string]string) <nil>,
                                +   Type: (v1.SecretType) ""
                                +  }),
                                +  (*v1.Secret)({
                                +   TypeMeta: (v1.TypeMeta) {
                                +    Kind: (string) "",
                                +    APIVersion: (string) ""
                                +   },
                                +   ObjectMeta: (v1.ObjectMeta) {
                                +    Name: (string) (len=9) "deleted-1",
                                     GenerateName: (string) "",
                Test:           TestConvertDataReadings/happy_path

},
},
},
Expand Down
36 changes: 36 additions & 0 deletions pkg/datagatherer/k8s/dynamic.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,37 @@
package k8s

// The venafi-kubernetes-agent has a requirement that **all** resources should
// be uploaded, even short-lived secrets, which are created and deleted
// in-between data uploads. A cache was added to the datagatherer code, to
// satisfy this requirement. The cache stores all resources for 5 minutes. And
// the informer event handlers (onAdd, onUpdate, onDelete) update the cache
// accordingly. The onDelete handler does not remove the object from the cache,
// but instead marks the object as deleted by setting the DeletedAt field on the
// GatheredResource. This ensures that deleted resources are still present in
// the cache for the duration of the cache expiry time.
//
// The cache expiry is hard coded to 5 minutes, which is longer than the
// venafi-kubernetes-agent default upload interval of 1 minute. This means that
// even if a resource is created and deleted in-between data gatherer runs, it
// will still be present in the cache when the data gatherer runs.
//
// TODO(wallrj): When the agent is deployed as CyberArk disco-agent, the deleted
// items are currently discarded before upload. If this remains the case, then the cache is unnecessary
// and should be disabled to save memory.
// If, in the future, the CyberArk Discovery and Context service does want to
// see deleted items, the "deleted resource reporting mechanism" will need to be
// redesigned, so that deleted items are retained for the duration of the upload
// interval.
//
// TODO(wallrj): When the agent is deployed as CyberArk disco-agent, the upload
// interval is 12 hours by default, so the 5 minute cache expiry is not
// sufficient.
//
// TODO(wallrj): The shared informer is configured to refresh all relist all
// resources every 1 minute, which will cause unnecessary load on the apiserver.
// We need to look back at the Git history and understand whether this was done
// for good reason or due to some misunderstanding.

Copy link
Copy Markdown
Member Author

@wallrj-cyberark wallrj-cyberark Oct 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added these comments for context and as a reminder that this all needs to be improved. If I had more time, I might have had a go at refactoring all this code allow the cache to be optional; or perhaps having a smaller cache containing only the deleted resources.
But that would have taken more time to code and much more time to test; we'd need to do extensive testing to ensure that the deleted resources are still being reported to Venafi control plane.

import (
"context"
"errors"
Expand Down Expand Up @@ -197,6 +229,8 @@ func (c *ConfigDynamic) newDataGathererWithClient(ctx context.Context, cl dynami

if informerFunc, ok := kubernetesNativeResources[c.GroupVersionResource]; ok {
factory := informers.NewSharedInformerFactoryWithOptions(clientset,
// TODO(wallrj): This causes all resources to be relisted every 1
// minute which will cause unnecessary load on the apiserver.
60*time.Second,
informers.WithNamespace(metav1.NamespaceAll),
informers.WithTweakListOptions(func(options *metav1.ListOptions) {
Expand All @@ -207,6 +241,8 @@ func (c *ConfigDynamic) newDataGathererWithClient(ctx context.Context, cl dynami
} else {
factory := dynamicinformer.NewFilteredDynamicSharedInformerFactory(
cl,
// TODO(wallrj): This causes all resources to be relisted every 1
// minute which will cause unnecessary load on the apiserver.
60*time.Second,
metav1.NamespaceAll,
func(options *metav1.ListOptions) {
Expand Down