refactor(cyberark): update Snapshot resource types to client.Object

This commit refactors the `Snapshot` resource types to use `client.Object`
instead of `runtime.Object`, aiming to improve type safety.
This reduces the amount of type casting and associated error handling in the
minimizeSnapshot code and allows the logging of dropped secret name and
namespace becasue the secret objects are known to satisfy both metav1.Object and
runtime.Object.
The extractResourceListFromReading function needed to be updated accordingly to
handle generic types.

- Replace runtime.Object with client.Object and *unstructured.Unstructured
  in Snapshot struct and related functions
- Refactor minimizeSnapshot and isExcludableSecret to use new types
- Update extractResourceListFromReading to use generics for resource type
- Adjust tests to match new resource type signatures and expectations
- Add "type" field to example Secret in input.json for clarity

Signed-off-by: Richard Wall <[email protected]>

Author: wallrj-cyberark

examples/machinehub/input.json

@@ -19,7 +19,8 @@
                         "metadata": {
                             "name": "app-1-secret-1",
                             "namespace": "team-1"
-                        }
+                        },
+                        "type": "kubernetes.io/tls"
                     }
                 }
             ]

internal/cyberark/dataupload/dataupload.go

@@ -12,7 +12,8 @@ import (
 	"net/http"
 	"net/url"
 
-	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	"github.com/jetstack/preflight/pkg/version"
 )
@@ -54,29 +55,32 @@ type Snapshot struct {
 	K8SVersion string `json:"k8s_version"`
 	// Secrets is a list of Secret resources in the cluster. Not all Secret
 	// types are included and only a subset of the Secret data is included.
-	Secrets []runtime.Object `json:"secrets"`
+	//
+	// Secrets are obtained by a DynamicClient, so they have type
+	// *unstructured.Unstructured.
+	Secrets []*unstructured.Unstructured `json:"secrets"`
 	// ServiceAccounts is a list of ServiceAccount resources in the cluster.
-	ServiceAccounts []runtime.Object `json:"serviceaccounts"`
+	ServiceAccounts []client.Object `json:"serviceaccounts"`
 	// Roles is a list of Role resources in the cluster.
-	Roles []runtime.Object `json:"roles"`
+	Roles []client.Object `json:"roles"`
 	// ClusterRoles is a list of ClusterRole resources in the cluster.
-	ClusterRoles []runtime.Object `json:"clusterroles"`
+	ClusterRoles []client.Object `json:"clusterroles"`
 	// RoleBindings is a list of RoleBinding resources in the cluster.
-	RoleBindings []runtime.Object `json:"rolebindings"`
+	RoleBindings []client.Object `json:"rolebindings"`
 	// ClusterRoleBindings is a list of ClusterRoleBinding resources in the cluster.
-	ClusterRoleBindings []runtime.Object `json:"clusterrolebindings"`
+	ClusterRoleBindings []client.Object `json:"clusterrolebindings"`
 	// Jobs is a list of Job resources in the cluster.
-	Jobs []runtime.Object `json:"jobs"`
+	Jobs []client.Object `json:"jobs"`
 	// CronJobs is a list of CronJob resources in the cluster.
-	CronJobs []runtime.Object `json:"cronjobs"`
+	CronJobs []client.Object `json:"cronjobs"`
 	// Deployments is a list of Deployment resources in the cluster.
-	Deployments []runtime.Object `json:"deployments"`
+	Deployments []client.Object `json:"deployments"`
 	// Statefulsets is a list of StatefulSet resources in the cluster.
-	Statefulsets []runtime.Object `json:"statefulsets"`
+	Statefulsets []client.Object `json:"statefulsets"`
 	// Daemonsets is a list of DaemonSet resources in the cluster.
-	Daemonsets []runtime.Object `json:"daemonsets"`
+	Daemonsets []client.Object `json:"daemonsets"`
 	// Pods is a list of Pod resources in the cluster.
-	Pods []runtime.Object `json:"pods"`
+	Pods []client.Object `json:"pods"`
 }
 
 // PutSnapshot PUTs the supplied snapshot to an [AWS presigned URL] which it obtains via the CyberArk inventory API.

pkg/client/client_cyberark.go

@@ -11,9 +11,9 @@ import (
 	"github.com/go-logr/logr"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
-	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/klog/v2"
+	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	"github.com/jetstack/preflight/api"
 	"github.com/jetstack/preflight/internal/cyberark"
@@ -60,7 +60,7 @@ func (o *CyberArkClient) PostDataReadingsWithOptions(ctx context.Context, readin
 	}
 
 	// Minimize the snapshot to reduce size and improve privacy
-	minimizeSnapshot(log.V(logs.Debug), &snapshot)
+	minimizeSnapshot(log.V(logs.Debug).WithName("minimizeSnapshot"), &snapshot)
 
 	snapshot.AgentVersion = version.PreflightVersion
 
@@ -100,9 +100,9 @@ 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
+// data reading to T resources, to allow access to the metadata and
 // other kubernetes API fields.
-func extractResourceListFromReading(reading *api.DataReading, target *[]runtime.Object) error {
+func extractResourceListFromReading[T client.Object](reading *api.DataReading, target *[]T) error {
 	if reading == nil {
 		return fmt.Errorf("programmer mistake: the DataReading must not be nil")
 	}
@@ -112,14 +112,15 @@ 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([]T, len(data.Items))
 	for i, item := range data.Items {
-		if resource, ok := item.Resource.(runtime.Object); ok {
+		if resource, ok := item.Resource.(T); ok {
 			resources[i] = resource
 		} else {
+			expectedType := fmt.Sprintf("%T", new(T))[1:] // strip leading '*'
 			return fmt.Errorf(
-				"programmer mistake: the DynamicData items must have Resource type runtime.Object. "+
-					"This item (%d) has Resource type %T", i, item.Resource)
+				"programmer mistake: the DynamicData items must have Resource type %s. "+
+					"This item (%d) has Resource type %T", expectedType, i, item.Resource)
 		}
 	}
 	*target = resources
@@ -223,9 +224,11 @@ func convertDataReadings(
 //     service.
 func minimizeSnapshot(log logr.Logger, snapshot *dataupload.Snapshot) {
 	originalSecretCount := len(snapshot.Secrets)
-	filteredSecrets := make([]runtime.Object, 0, originalSecretCount)
+	filteredSecrets := make([]*unstructured.Unstructured, 0, originalSecretCount)
 	for _, secret := range snapshot.Secrets {
+		log := log.WithValues("name", secret.GetName(), "namespace", secret.GetNamespace())
 		if isExcludableSecret(log, secret) {
+			log.Info("Dropped")
 			continue
 		}
 		filteredSecrets = append(filteredSecrets, secret)
@@ -240,24 +243,9 @@ func minimizeSnapshot(log logr.Logger, snapshot *dataupload.Snapshot) {
 //
 // The Secret is kept if there is any doubt or if there is a problem decoding
 // its contents.
-//
-// Secrets are obtained by a DynamicClient, so they have type
-// *unstructured.Unstructured.
-func isExcludableSecret(log logr.Logger, obj runtime.Object) bool {
-	// Fast path: type assertion and kind/type checks
-	unstructuredObj, ok := obj.(*unstructured.Unstructured)
-	if !ok {
-		log.Info("Object is not a Unstructured", "type", fmt.Sprintf("%T", obj))
-		return false
-	}
+func isExcludableSecret(log logr.Logger, unstructuredObj *unstructured.Unstructured) bool {
 	if unstructuredObj.GetKind() != "Secret" || unstructuredObj.GetAPIVersion() != "v1" {
-		return false
-	}
-
-	log = log.WithValues("namespace", unstructuredObj.GetNamespace(), "name", unstructuredObj.GetName())
-	dataMap, found, err := unstructured.NestedMap(unstructuredObj.Object, "data")
-	if err != nil || !found {
-		log.Info("Secret data missing or not a map")
+		log.Info("Object is not a core/v1 Secret", "apiVersion", unstructuredObj.GetAPIVersion(), "kind", unstructuredObj.GetKind())
 		return false
 	}
 
@@ -268,10 +256,16 @@ func isExcludableSecret(log logr.Logger, obj runtime.Object) bool {
 	}
 
 	if corev1.SecretType(secretType) != corev1.SecretTypeTLS {
-		log.Info("Secret of this type are never excluded", "type", secretType)
+		log.Info("Secret of this type are never dropped", "type", secretType)
 		return false
 	}
 
+	dataMap, found, err := unstructured.NestedMap(unstructuredObj.Object, "data")
+	if err != nil || !found {
+		log.Info("Secret data missing or not a map", "error", err, "decision", "drop")
+		return true
+	}
+
 	return isExcludableTLSSecret(log, dataMap)
 }
 

pkg/client/client_cyberark_convertdatareadings_test.go

@@ -14,12 +14,10 @@ import (
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
-	corev1 "k8s.io/api/core/v1"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
-	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/version"
 	"k8s.io/klog/v2/ktesting"
+	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	"github.com/jetstack/preflight/api"
 	"github.com/jetstack/preflight/internal/cyberark/dataupload"
@@ -155,7 +153,7 @@ func TestExtractResourceListFromReading(t *testing.T) {
 					},
 				},
 			},
-			expectError: `programmer mistake: the DynamicData items must have Resource type runtime.Object. ` +
+			expectError: `programmer mistake: the DynamicData items must have Resource type *unstructured.Unstructured. ` +
 				`This item (0) has Resource type *api.DiscoveryData`,
 		},
 		{
@@ -194,7 +192,7 @@ func TestExtractResourceListFromReading(t *testing.T) {
 	}
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
-			var resources []runtime.Object
+			var resources []*unstructured.Unstructured
 			err := extractResourceListFromReading(test.reading, &resources)
 			if test.expectError != "" {
 				assert.EqualError(t, err, test.expectError)
@@ -231,10 +229,14 @@ func TestConvertDataReadings(t *testing.T) {
 			Data: &api.DynamicData{
 				Items: []*api.GatheredResource{
 					{
-						Resource: &corev1.Secret{
-							ObjectMeta: metav1.ObjectMeta{
-								Name:      "app-1",
-								Namespace: "team-1",
+						Resource: &unstructured.Unstructured{
+							Object: map[string]interface{}{
+								"apiVersion": "v1",
+								"kind":       "Secret",
+								"metadata": map[string]interface{}{
+									"name":      "app-1",
+									"namespace": "team-1",
+								},
 							},
 						},
 					},
@@ -293,11 +295,15 @@ func TestConvertDataReadings(t *testing.T) {
 			expectedSnapshot: dataupload.Snapshot{
 				ClusterID:  "success-cluster-id",
 				K8SVersion: "v1.21.0",
-				Secrets: []runtime.Object{
-					&corev1.Secret{
-						ObjectMeta: metav1.ObjectMeta{
-							Name:      "app-1",
-							Namespace: "team-1",
+				Secrets: []*unstructured.Unstructured{
+					{
+						Object: map[string]interface{}{
+							"apiVersion": "v1",
+							"kind":       "Secret",
+							"metadata": map[string]interface{}{
+								"name":      "app-1",
+								"namespace": "team-1",
+							},
 						},
 					},
 				},
@@ -351,17 +357,17 @@ func TestMinimizeSnapshot(t *testing.T) {
 				AgentVersion:    "v1.0.0",
 				ClusterID:       "cluster-1",
 				K8SVersion:      "v1.21.0",
-				Secrets:         []runtime.Object{},
-				ServiceAccounts: []runtime.Object{},
-				Roles:           []runtime.Object{},
+				Secrets:         []*unstructured.Unstructured{},
+				ServiceAccounts: []client.Object{},
+				Roles:           []client.Object{},
 			},
 			expectedSnapshot: dataupload.Snapshot{
 				AgentVersion:    "v1.0.0",
 				ClusterID:       "cluster-1",
 				K8SVersion:      "v1.21.0",
-				Secrets:         []runtime.Object{},
-				ServiceAccounts: []runtime.Object{},
-				Roles:           []runtime.Object{},
+				Secrets:         []*unstructured.Unstructured{},
+				ServiceAccounts: []client.Object{},
+				Roles:           []client.Object{},
 			},
 		},
 		{
@@ -370,28 +376,28 @@ func TestMinimizeSnapshot(t *testing.T) {
 				AgentVersion: "v1.0.0",
 				ClusterID:    "cluster-1",
 				K8SVersion:   "v1.21.0",
-				Secrets: []runtime.Object{
+				Secrets: []*unstructured.Unstructured{
 					secretWithClientCert,
 					secretWithoutClientCert,
 					opaqueSecret,
 				},
-				ServiceAccounts: []runtime.Object{
+				ServiceAccounts: []client.Object{
 					serviceAccount,
 				},
-				Roles: []runtime.Object{},
+				Roles: []client.Object{},
 			},
 			expectedSnapshot: dataupload.Snapshot{
 				AgentVersion: "v1.0.0",
 				ClusterID:    "cluster-1",
 				K8SVersion:   "v1.21.0",
-				Secrets: []runtime.Object{
+				Secrets: []*unstructured.Unstructured{
 					secretWithClientCert,
 					opaqueSecret,
 				},
-				ServiceAccounts: []runtime.Object{
+				ServiceAccounts: []client.Object{
 					serviceAccount,
 				},
-				Roles: []runtime.Object{},
+				Roles: []client.Object{},
 			},
 		},
 	}
@@ -408,7 +414,7 @@ func TestMinimizeSnapshot(t *testing.T) {
 func TestIsExcludableSecret(t *testing.T) {
 	type testCase struct {
 		name    string
-		secret  runtime.Object
+		secret  *unstructured.Unstructured
 		exclude bool
 	}
 
@@ -423,16 +429,6 @@ func TestIsExcludableSecret(t *testing.T) {
 			secret:  newTLSSecret("tls-secret-without-client", sampleCertificateChain(t, x509.ExtKeyUsageServerAuth)),
 			exclude: true,
 		},
-		{
-			name: "Non-unstructured",
-			secret: &corev1.Pod{
-				ObjectMeta: metav1.ObjectMeta{
-					Name:      "non-unstructured-secret",
-					Namespace: "default",
-				},
-			},
-			exclude: false,
-		},
 		{
 			name: "Non-secret",
 			secret: &unstructured.Unstructured{