feature: clean reconcilers casting to types

[mjudeikis]

Feature Description

We have bunch of reconcilers doing these castings:

_, _ = secretInformer.Informer().AddEventHandler(events.WithoutSyncs(cache.ResourceEventHandlerFuncs{
		AddFunc: func(obj interface{}) {
			c.enqueueSecret(obj.(*corev1.Secret))
		},
		UpdateFunc: func(_, newObj interface{}) {
			c.enqueueSecret(newObj.(*corev1.Secret))
		},
		DeleteFunc: func(obj interface{}) {
			c.enqueueSecret(obj.(*corev1.Secret))
		},
	}))

or

func (c *controller) enqueueAPIBinding(obj interface{}, logger logr.Logger) {
	key, err := kcpcache.DeletionHandlingMetaClusterNamespaceKeyFunc(obj)
	if err != nil {
		utilruntime.HandleError(err)
		return
	}

	logging.WithQueueKey(logger, key).V(4).Info("queueing APIBinding")
	c.queue.Add(key)
}

We been standartizing around:

AddFunc: func(obj interface{}) {
			c.enqueueAllAPIExportEndpointSlices(objOrTombstone[*corev1alpha1.Shard](obj), logger, "")
		},
		UpdateFunc: func(oldObj, newObj interface{}) {
			if filterShardEvent(oldObj, newObj) {
				c.enqueueAllAPIExportEndpointSlices(objOrTombstone[*corev1alpha1.Shard](newObj), logger, "")
			}
		},
		DeleteFunc: func(obj interface{}) {
			c.enqueueAllAPIExportEndpointSlices(objOrTombstone[*corev1alpha1.Shard](obj), logger, "")
		},

where

func objOrTombstone[T runtime.Object](obj any) T {
	if t, ok := obj.(T); ok {
		return t
	}
	if tombstone, ok := obj.(cache.DeletedFinalStateUnknown); ok {
		if t, ok := tombstone.Obj.(T); ok {
			return t
		}

		panic(fmt.Errorf("tombstone %T is not a %T", tombstone, new(T)))
	}

	panic(fmt.Errorf("%T is not a %T", obj, new(T)))
}

Meaning this makes it safer casting and makes it less boilerpaltes.

Would be good to find shared place for func objOrTombstone[T runtime.Object](obj any) T as now we copy it around. I don't like shared utils packages but maybe this one would be fine?

Proposed Solution

fix as above ^

Alternative Solutions

No response

Want to contribute?

• [ ] I would like to work on this issue.

Additional Context

No response

[yashvardhan-kukreja]

/assign

[yashvardhan-kukreja]

Hi I'd like to take this issue up.

As you mentioned, I think as well that a shared utils package would make sense here. wdyt about pkg/util ?

Moreover, while navigating the codebase, I got curious around the significance of the package k8s.io/apimachinery/pkg/util/ and when would one approach adding some feature to it. I'd appreciate if you can shed light upon it.

[lterrac]

Hi all! Jumping into the discussion because I would like to contribute 😄 @yashvardhan-kukreja are you currently working on this? Do you need a hand or can I take over the whole issue?

Since those casting are in many parts of the codebase and the resulting PR can be quite big, I am wondering whether it should be divided into smaller ones. Does it make sense? If so, which are the high priority controllers to adapt to this? @mjudeikis

Also, does this code fall under the modifications indicated by this issue?

crb := obj.(*rbacv1.ClusterRoleBinding)

Here there is no check on the success of the cast, should we enforce it?

[embik]

Hi folks! @yashvardhan-kukreja are you still working on this?

As you mentioned, I think as well that a shared utils package would make sense here. wdyt about pkg/util ?

My take on this is that it should be in pkg/reconciler/utils since it's a util specifically for reconcilers. We should avoid big globs of util packages that cover everything.

Since those casting are in many parts of the codebase and the resulting PR can be quite big, I am wondering whether it should be divided into smaller ones. Does it make sense? If so, which are the high priority controllers to adapt to this?

I would say start with whatever looks good to you. As soon as we have established a package for this, we can incrementally migrate basically whatever controllers are low hanging fruits.

[embik]

And apologies for the lack of replies on this issue, we very much appreciate external contributions and hope to enable you to become kcp contributors here!

[embik]

/unassign @yashvardhan-kukreja