don't use new(T) (#374)

* don't use new(T) * fix mc code * fix tmpl * oops * changelog Co-authored-by: Jenny Shu <[email protected]>
solo-io · Oct 11, 2022 · 8a5f20f · 8a5f20f
1 parent 1426888
commit 8a5f20f
Show file tree

Hide file tree

Showing 4 changed files with 66 additions and 31 deletions.
diff --git a/changelog/v0.23.8/no-new-T.yaml b/changelog/v0.23.8/no-new-T.yaml
@@ -0,0 +1,3 @@
+changelog:
+  - type: NON_USER_FACING
+    description: Don't use new(T) in generic reconciler. It instantiates a `nil` object.
diff --git a/contrib/codegen/templates/input/input_reconciler.gotmpl b/contrib/codegen/templates/input/input_reconciler.gotmpl
@@ -85,7 +85,7 @@ func RegisterMultiCluster{{ $snapshotName }}Reconciler(
 {{- $kindLowerCamel := lower_camel $resource.Kind }}
 {{- $kindLowerCamelPlural := pluralize $kindLowerCamel }}
 
-    multicluster_reconcile_v2.NewLoop[*{{ $types_import_prefix }}.{{ $resource.Kind }}]("das", clusters, reconcile_v2.Options{}).
+    multicluster_reconcile_v2.NewLoop("das", clusters, &{{ $types_import_prefix }}.{{ $resource.Kind }}{}, reconcile_v2.Options{}).
         AddReconciler(ctx, &multicluster_v2.ReconcilerFuncs[*{{ $types_import_prefix }}.{{ $resource.Kind }}]{
             ReconcileFunc: func(ctx context.Context, clusterName string, obj *{{ $types_import_prefix }}.{{ $resource.Kind }}) (reconcile.Result, error) {
                 obj.ClusterName = clusterName
@@ -142,7 +142,7 @@ func RegisterSingleCluster{{ $snapshotName }}Reconciler(
 {{- $kindPlural := pluralize $resource.Kind }}
 {{- $kindLowerCamel := lower_camel $resource.Kind }}
 {{- $kindLowerCamelPlural := pluralize $kindLowerCamel }}
-    if err := reconcile_v2.NewLoop[*{{ $types_import_prefix }}.{{ $resource.Kind }}]("Service", "", mgr, options).
+    if err := reconcile_v2.NewLoop("Service", "", mgr, &{{ $types_import_prefix }}.{{ $resource.Kind }}{}, options).
         RunReconciler(ctx, &reconcile_v2.ReconcileFuncs[*{{ $types_import_prefix }}.{{ $resource.Kind }}]{
         ReconcileFunc:func(ctx context.Context, object *{{ $types_import_prefix }}.{{ $resource.Kind }}) (reconcile.Result, error){
             return r.base.ReconcileRemoteGeneric(object)

diff --git a/pkg/multicluster/reconcile/v2/reconcile.go b/pkg/multicluster/reconcile/v2/reconcile.go
@@ -20,18 +20,25 @@ type clusterLoopRunner[T client.Object] struct {
 	clusterLoops *clusterLoopSet[T]
 	reconcilers  *reconcilerList[T]
 	options      reconcile_v2.Options
+	t            T
 }
 
 var _ multicluster_v2.Loop[client.Object] = &clusterLoopRunner[client.Object]{}
 var _ multicluster.ClusterHandler = &clusterLoopRunner[client.Object]{}
 var _ multicluster.ClusterRemovedHandler = &clusterLoopRunner[client.Object]{}
 
-func NewLoop[T client.Object](name string, cw multicluster.ClusterWatcher, options reconcile_v2.Options) *clusterLoopRunner[T] {
+func NewLoop[T client.Object](
+	name string,
+	cw multicluster.ClusterWatcher,
+	t T,
+	options reconcile_v2.Options,
+) *clusterLoopRunner[T] {
 	runner := &clusterLoopRunner[T]{
 		name:         name,
 		clusterLoops: newClusterLoopSet[T](),
 		reconcilers:  newReconcilerList[T](),
 		options:      options,
+		t:            t,
 	}
 	cw.RegisterClusterHandler(runner)
 
@@ -40,7 +47,7 @@ func NewLoop[T client.Object](name string, cw multicluster.ClusterWatcher, optio
 
 // AddCluster creates a reconcile_v2 loop for the cluster.
 func (r *clusterLoopRunner[T]) AddCluster(ctx context.Context, cluster string, mgr manager.Manager) {
-	loopForCluster := reconcile_v2.NewLoop[T](r.name+"-"+cluster, cluster, mgr, r.options)
+	loopForCluster := reconcile_v2.NewLoop(r.name+"-"+cluster, cluster, mgr, r.t, r.options)
 
 	// Add the cluster loop to the set of active loops and start reconcilers.
 	r.clusterLoops.add(cluster, loopForCluster)
@@ -54,7 +61,11 @@ func (r *clusterLoopRunner[T]) RemoveCluster(cluster string) {
 }
 
 // AddReconciler registers a cluster handler for the reconciler.
-func (r *clusterLoopRunner[T]) AddReconciler(ctx context.Context, reconciler multicluster_v2.Reconciler[T], predicates ...predicate.Predicate) {
+func (r *clusterLoopRunner[T]) AddReconciler(
+	ctx context.Context,
+	reconciler multicluster_v2.Reconciler[T],
+	predicates ...predicate.Predicate,
+) {
 	r.reconcilers.add(ctx, reconciler, predicates...)
 	r.clusterLoops.ensureReconcilers(r.reconcilers)
 }
@@ -128,16 +139,22 @@ func newReconcilerList[T client.Object]() *reconcilerList[T] {
 	}
 }
 
-func (r *reconcilerList[T]) add(ctx context.Context, reconciler multicluster_v2.Reconciler[T], predicates ...predicate.Predicate) {
+func (r *reconcilerList[T]) add(
+	ctx context.Context,
+	reconciler multicluster_v2.Reconciler[T],
+	predicates ...predicate.Predicate,
+) {
 	r.mutex.Lock()
 	defer r.mutex.Unlock()
 
-	r.reconcilers = append(r.reconcilers, runnableReconciler[T]{
-		ctx:            ctx,
-		reconciler:     reconciler,
-		predicates:     predicates,
-		activeClusters: sets.String{},
-	})
+	r.reconcilers = append(
+		r.reconcilers, runnableReconciler[T]{
+			ctx:            ctx,
+			reconciler:     reconciler,
+			predicates:     predicates,
+			activeClusters: sets.String{},
+		},
+	)
 }
 
 // runAll runs all reconcilers in the list on the given loop.
@@ -157,9 +174,11 @@ func (r *reconcilerList[T]) runAll(cluster string, loop reconcile_v2.Loop[T]) {
 		}
 		err := loop.RunReconciler(rr.ctx, mcReconciler, rr.predicates...)
 		if err != nil {
-			contextutils.LoggerFrom(rr.ctx).Debug("Error occurred when adding reconciler to cluster loop",
+			contextutils.LoggerFrom(rr.ctx).Debug(
+				"Error occurred when adding reconciler to cluster loop",
 				zap.Error(err),
-				zap.String("cluster", cluster))
+				zap.String("cluster", cluster),
+			)
 		}
 		rr.activeClusters.Insert(cluster)
 	}

diff --git a/pkg/reconcile/v2/runner.go b/pkg/reconcile/v2/runner.go
@@ -78,6 +78,7 @@ type runner[T client.Object] struct {
 	mgr     manager.Manager
 	options Options
 	cluster string
+	t       T
 }
 
 type Options struct {
@@ -91,24 +92,31 @@ type Options struct {
 func NewLoop[T client.Object](
 	name, cluster string,
 	mgr manager.Manager,
+	t T,
 	options Options,
 ) *runner[T] {
 	return &runner[T]{
 		name:    name,
 		mgr:     mgr,
 		cluster: cluster,
 		options: options,
+		t:       t,
 	}
 }
 
 type runnerReconciler[T client.Object] struct {
 	mgr        manager.Manager
 	reconciler Reconciler[T]
+	t          T
 }
 
-func (r *runner[T]) RunReconciler(ctx context.Context, reconciler Reconciler[T], predicates ...predicate.Predicate) error {
-	obj := new(T)
-	gvk, err := apiutil.GVKForObject(*obj, r.mgr.GetScheme())
+func (r *runner[T]) RunReconciler(
+	ctx context.Context,
+	reconciler Reconciler[T],
+	predicates ...predicate.Predicate,
+) error {
+	obj := r.t.DeepCopyObject().(T)
+	gvk, err := apiutil.GVKForObject(obj, r.mgr.GetScheme())
 	if err != nil {
 		return err
 	}
@@ -125,17 +133,20 @@ func (r *runner[T]) RunReconciler(ctx context.Context, reconciler Reconciler[T],
 	rec := &runnerReconciler[T]{
 		mgr:        r.mgr,
 		reconciler: reconciler,
+		t:          obj,
 	}
 
-	ctl, err := controller.New(r.name, r.mgr, controller.Options{
-		Reconciler: rec,
-	})
+	ctl, err := controller.New(
+		r.name, r.mgr, controller.Options{
+			Reconciler: rec,
+		},
+	)
 	if err != nil {
 		return err
 	}
 
 	// send us watch events
-	if err := ctl.Watch(&source.Kind{Type: *obj}, &handler.EnqueueRequestForObject{}, predicates...); err != nil {
+	if err := ctl.Watch(&source.Kind{Type: obj}, &handler.EnqueueRequestForObject{}, predicates...); err != nil {
 		return err
 	}
 
@@ -150,13 +161,13 @@ func (r *runner[T]) RunReconciler(ctx context.Context, reconciler Reconciler[T],
 	return nil
 }
 
-func (ec *runnerReconciler[T]) Reconcile(ctx context.Context, request Request) (reconcile.Result, error) {
+func (r *runnerReconciler[T]) Reconcile(ctx context.Context, request Request) (reconcile.Result, error) {
 	contextutils.LoggerFrom(ctx).Debug("handling event", "event", request)
 
 	// get the object from our cache
-	restClient := ezkube.NewRestClient(ec.mgr)
+	restClient := ezkube.NewRestClient(r.mgr)
 
-	obj := *(new(T))
+	obj := r.t.DeepCopyObject().(T)
 	obj.SetName(request.Name)
 	obj.SetNamespace(request.Namespace)
 	if err := restClient.Get(ctx, obj); err != nil {
@@ -166,15 +177,15 @@ func (ec *runnerReconciler[T]) Reconcile(ctx context.Context, request Request) (
 		contextutils.LoggerFrom(ctx).Debug(fmt.Sprintf("unable to fetch %T", obj), "request", request, "err", err)
 
 		// call OnDelete
-		if deletionReconciler, ok := ec.reconciler.(DeletionReconciler); ok {
+		if deletionReconciler, ok := r.reconciler.(DeletionReconciler); ok {
 			return reconcile.Result{}, deletionReconciler.ReconcileDeletion(ctx, request)
 		}
 
 		return reconcile.Result{}, nil
 	}
 
 	// if the handler is a finalizer, check if we need to finalize
-	if finalizer, ok := ec.reconciler.(FinalizingReconciler[T]); ok {
+	if finalizer, ok := r.reconciler.(FinalizingReconciler[T]); ok {
 		finalizers := obj.GetFinalizers()
 		finalizerName := finalizer.FinalizerName()
 		if obj.GetDeletionTimestamp().IsZero() {
@@ -183,10 +194,12 @@ func (ec *runnerReconciler[T]) Reconcile(ctx context.Context, request Request) (
 			// registering our finalizer.
 
 			if !utils.ContainsString(finalizers, finalizerName) {
-				obj.SetFinalizers(append(
-					finalizers,
-					finalizerName,
-				))
+				obj.SetFinalizers(
+					append(
+						finalizers,
+						finalizerName,
+					),
+				)
 				if err := restClient.Update(ctx, obj); err != nil {
 					return reconcile.Result{}, err
 				}
@@ -214,7 +227,7 @@ func (ec *runnerReconciler[T]) Reconcile(ctx context.Context, request Request) (
 		}
 	}
 
-	result, err := ec.reconciler.Reconcile(ctx, obj)
+	result, err := r.reconciler.Reconcile(ctx, obj)
 	if err != nil {
 		return result, eris.Wrap(err, "handler error. retrying")
 	}