Migrate all backend controllers to controller-runtime by mjudeikis · Pull Request #274 · kbind-dev/kbind

mjudeikis · 2025-08-08T11:02:49Z

Summary

Migrate all controllers to controller-runtime

What Type of PR Is This?

/kind cleanup
/kind feature

Related Issue(s)

Fixes #267

Release Notes

Migrate backend to controller-runtime

xrstf · 2025-08-11T08:28:16Z

+				}
+				var exports []*kubebindv1alpha2.APIServiceExport
+				for i := range list.Items {
+					if list.Items[i].Namespace == ns || ns == "" {


Why do we support clusterscoped and namespaced mode for the same resource? The original code didn't do it, did it?

xrstf · 2025-08-11T08:29:38Z

+		{
+			NamespacedName: types.NamespacedName{
+				Namespace: serviceExport.Namespace,
+				Name:      "cluster", // This matches the original logic: ns + "/cluster"


This comment might be less useful after this is merged, maybe it should just be a review comment. Same for other "original logic" comments.

xrstf · 2025-08-11T08:31:39Z

+			logger.Error(err, "Failed to update ClusterBinding status")
+			return ctrl.Result{}, fmt.Errorf("failed to update ClusterBinding status: %w", err)
+		}
+		logger.Info("ClusterBinding status updated", "namespace", clusterBinding.Namespace, "name", clusterBinding.Name)


Instead of duplicating context here, why not extend the logger := log.FromContext(ctx) part to store some context on the local logger. Then all the other log messages, like logger.Info("ClusterBinding not found, ignoring"), are also properly annotated.

removed duplicates

xrstf · 2025-08-11T08:37:01Z

-		return false
+	// Update status if it has changed
+	if !reflect.DeepEqual(original, apiServiceExport) {
+		err := r.Update(ctx, apiServiceExport)


If we already have original and updated object, we could send a smart patch here instead of an update.

wdym? What is smart patch?

xrstf · 2025-08-11T08:41:32Z

-		errs = append(errs, err)
+	// Update status if it has changed
+	if !reflect.DeepEqual(original.Status, apiServiceExportRequest.Status) {
+		err := r.Status().Update(ctx, apiServiceExportRequest)


Here we indeed update the status subresource.

xrstf · 2025-08-11T08:46:33Z

 }
+
+// IndexServiceExportByCustomResourceDefinitionControllerRuntime is a controller-runtime compatible indexer function
+// that indexes APIServiceExports by their CustomResourceDefinition name.


It looks though as if APIServiceExports are indexed by their name, not their CRD's name. Is that the same?

This stinks. I think this is wrong (old and new ones, both). Thanks for poking this.

xrstf · 2025-08-11T19:13:26Z

/lgtm

xrstf

/lgtm

under the assumption that my review comments are adressed later

Signed-off-by: Mangirdas Judeikis <mangirdas@judeikis.lt> On-behalf-of: @SAP mangirdas.judeikis@sap.com

mjudeikis · 2025-08-11T19:17:05Z

TODO:

move rest of the clients to move client
check what is happening with apiserviceexport index

On-behalf-of: @SAP mangirdas.judeikis@sap.com Signed-off-by: Mangirdas Judeikis <mangirdas@judeikis.lt>

mjudeikis marked this pull request as ready for review August 9, 2025 12:19

mjudeikis requested a review from a team as a code owner August 9, 2025 12:19

mjudeikis mentioned this pull request Aug 9, 2025

Move backend to Multicluster runtime #275

Merged