Refactor controllers and implementations

[pleshakov]

This PR refactors controllers and implementations. Previously, all controllers and implementations looked almost the same. The commit replaces all controllers with a single controller that handles all the types. Similarly, the PR replaces all implementations with a single one that handles all the types.

The new code relies on generics

[kate-osborn]

What do you think about making this more extensible by mirroring the builder pattern of the controller?

I'm wondering how we can best support the following variation of Register (included in the updated version of PR #221):

// RegisterEndpointSliceController registers the EndpointSliceController in the manager.
func RegisterEndpointSliceController(mgr manager.Manager, impl EndpointSliceImpl) error {
	r := &endpointSliceReconciler{
		Client: mgr.GetClient(),
		scheme: mgr.GetScheme(),
		impl:   impl,
	}

	err := mgr.GetFieldIndexer().IndexField(context.TODO(), &discoveryV1.EndpointSlice{}, KubernetesServiceNameIndexField, ServiceNameIndexFunc)
	if err != nil {
		return fmt.Errorf("failed to add service name index for EndpointSlices: %w", err)
	}

	return ctlr.NewControllerManagedBy(mgr).
		For(&discoveryV1.EndpointSlice{}).
		WithEventFilter(predicate.GenerationChangedPredicate{}).
		Complete(r)
}

One way, I suppose, is to add the field index to the manager before calling register. But I wonder if it makes more sense to expose this option through the reconciler package.

What do you think?

[pleshakov]

The simplest approach I see is to get rid of Register function at all.

So in the manager, do something like this (error handling is not shown):

mgr.GetFieldIndexer().IndexField(context.TODO(), &discoveryV1.EndpointSlice{}, KubernetesServiceNameIndexField, ServiceNameIndexFunc)
ctlr.NewControllerManagedBy(mgr).
		For(&discoveryV1.EndpointSlice{}).
		WithEventFilter(predicate.GenerationChangedPredicate{}).
		Complete(NewReconciler(...))

ctlr.NewControllerManagedBy(mgr).
		For(&discoveryV1.Gateway{}).
		Complete(NewReconciler(...))

... 

What do you think about making this more extensible by mirroring the builder pattern of the controller?

That could also work:

sdk.RegisterControllerManagedBy(mgr).
   For(&discoveryV1.EndpointSlice{}).
   WithEventFilter(predicate.GenerationChangedPredicate{}).
   WithMgrSetup(func (..){ .. set up manager ... }).
    WithImplementation(NewImplementation(...)).
   Complete()

but not sure it adds a lot a benefit, because we're almost duplicating what controller-runtime provide.

[kate-osborn]

Not sure this is a good use case for generics.

If we have to know what type we are working with, it's not generic. Also, how efficient are all of these reflect calls?

[f5yacobucci]

client.Object is a runtime.Object - can we use GetObjectKind here? Reflect will give us the data types, but I suspect we really care about the k8s object kind.

[pleshakov]

Also, how efficient are all of these reflect calls?

it is slower

package sdk

import (
	"reflect"

	"sigs.k8s.io/controller-runtime/pkg/client"
	"sigs.k8s.io/gateway-api/apis/v1beta1"
)

func NewObject1[T ObjectConstraint]() client.Object {
	var obj T
	obj = reflect.New(reflect.TypeOf(obj).Elem()).Interface().(T)
	return obj
}

func NewObject2() client.Object {
	var gw v1beta1.Gateway
	return &gw
}
---
package sdk

import (
	"testing"

	"sigs.k8s.io/gateway-api/apis/v1beta1"
)

func BenchmarkNewObject1(b *testing.B) {
	for i := 0; i < b.N; i++ {
		NewObject1[*v1beta1.Gateway]()
	}
}

func BenchmarkNewObject2(b *testing.B) {
	for i := 0; i < b.N; i++ {
		NewObject2()
	}
}
---
BenchmarkNewObject1-4            5302594               208.5 ns/op
BenchmarkNewObject2-4           1000000000               0.2955 ns/op
PASS
ok      github.com/nginxinc/nginx-kubernetes-gateway/pkg/sdk    1.885s

[pleshakov]

client.Object is a runtime.Object - can we use GetObjectKind here? Reflect will give us the data types, but I suspect we really care about the k8s object kind.

We can get access to ObjectKind if we create an object with populating TypeMeta. Like below:

				&v1beta1.Gateway{
					TypeMeta: metav1.TypeMeta{
						Kind:       "Gateway",
						APIVersion: "gateway.networking.k8s.io/v1beta1",
					},
 }

If we just create an object like this &v1beta1.Gateway{} - which is what we do in our code -- GetObjectKind will not return anything

[pleshakov]

If we have to know what type we are working with, it's not generic.

I agree.

I think the problem is here is that the new generic code still uses non-generic functions (like r.Get(ctx, req.NamespacedName, obj)) and to use them, we need to somehow get client.Object's . But not sure how to do that in a better way.

[pleshakov]

client.Object is a runtime.Object - can we use GetObjectKind here? Reflect will give us the data types, but I suspect we really care about the k8s object kind.

UPDATE:

I was wrong - it is possible, provided we have a scheme defined -- https://pkg.go.dev/sigs.k8s.io/controller-runtime/pkg/client/apiutil#GVKForObject

For example:

gvk, err := apiutil.GVKForObject(options.ObjectType, mgr.GetScheme())

[kate-osborn]

Would it be possible to register multiple implementations in one call, rather than registering each one individually?

[pleshakov]

definitely!

[f5yacobucci]

Did you try the builtin new? The interaction between template functions and their non-template counterparts is interesting.

var obj *T
obj = new(T)
builder := ctlr.NewControllerManagedBy(mgr).For(*obj)

A buildable experiment:

package main

import (
	"fmt"
	"reflect"
	"time"
)

type foo struct {
	t time.Time
}

type bar struct {
	t time.Time
}

type timer interface {
	CreatedAt() time.Time
}

func (f *foo) CreatedAt() time.Time {
	return f.t
}

func (b *bar) CreatedAt() time.Time {
	return b.t
}

func NewFoo(t time.Time) *foo {
	return &foo{
		t: t,
	}
}

func NewBar(t time.Time) *bar {
	return &bar{
		t: t,
	}
}

type ObjectConstraint interface {
	timer
}

func checkCreated[T ObjectConstraint](check T) {
	var obj T
	fmt.Printf("TypeOf: %v\n", reflect.TypeOf(obj))
	fmt.Printf("TypeOf.Elem: %v\n", reflect.TypeOf(obj).Elem())
	fmt.Printf("TypeOf.Elem.Name: %v\n", reflect.TypeOf(obj).Elem().Name())
	
	fmt.Printf("New(TypeOf.Elem).Interface: %v\n", 
		reflect.New(reflect.TypeOf(obj).Elem()).Interface())
	fmt.Println()
	fmt.Println()

	obj = reflect.New(reflect.TypeOf(obj).Elem()).Interface().(T)
  fmt.Printf("TypeOf: %v\n", reflect.TypeOf(obj))
  fmt.Printf("TypeOf.Elem: %v\n", reflect.TypeOf(obj).Elem())
  fmt.Printf("TypeOf.Elem.Name: %v\n", reflect.TypeOf(obj).Elem().Name())

  fmt.Printf("New(TypeOf.Elem).Interface: %v\n",
    reflect.New(reflect.TypeOf(obj).Elem()).Interface())
	fmt.Println()
	fmt.Println()

	obj2 := new(T)
  fmt.Printf("TypeOf: %v\n", reflect.TypeOf(*obj2))
  fmt.Printf("TypeOf.Elem: %v\n", reflect.TypeOf(*obj2).Elem())
  fmt.Printf("TypeOf.Elem.Name: %v\n", reflect.TypeOf(*obj2).Elem().Name())

  fmt.Printf("New(TypeOf.Elem).Interface: %v\n",
    reflect.New(reflect.TypeOf(*obj2).Elem()).Interface())

	fmt.Println(check.CreatedAt())
}

func main() {
	f := NewFoo(time.Now())
	//b := NewBar(time.Now())

	checkCreated(f)
	//checkCreated(b)
}

[pleshakov]

Did you try the builtin new? The interaction between template functions and their non-template counterparts is interesting.

var obj *T
obj = new(T)
builder := ctlr.NewControllerManagedBy(mgr).For(*obj)

In this case, *obj will be pointer to a nil value. This in turn will make the builder fail on the Complete() or Build() call because of the nil check there.

[pleshakov]

	obj2 := new(T)
	timerRef := *obj2
	fmt.Printf("Value: %v\n", timerRef)

leads to

Value: <nil>

[pleshakov]

closing this PR.
based on the offline discussion, I will prepare a new PR with a single reconciler without using generics and without any implementations - the reconciler will be responsible to sending events to the Event channel.