⚠️ Source, Event, Predicate, Handler: Add generics support

[alvaroaleman]

This change adds support for generics into the Event, Predicate and Handler chain. The motivation for this is explained in #2214, but basically the current client.Object typing frequently requires type assertions when using custom handlers or predicates and is completely inappropriate when building a controller that is not based on Kubernetes objects.

It aims to minimize breaking changes as much as possible. It does so in two parts:

• Source: Move Handler and Predicate out from Start and into constructor: This is needed because of proposal: spec: allow type parameters in methods golang/go#49085, otherwise we either
  • Can only support one type per Controller which makes no sense
  • Would need to change controller.Watch to take a wrapper type that encapsulates the generic part and hides it from the controller. This is pretty much the same, but more complicated
  • This change means that the concept of a Handler and Predicate effectively becomes optional for a Source which I think makes sense, as a custom source may decide it is easier not to implement these. The existing concept is also visibly shoehorned insofar as that Event/PredicateHandler have Create/Update/Delete and Generic variants, whereas the first three are only used in the context of a kube watch and the latter only outside of it
• Source, Event, Predicate, Handler: Add generics support. This is achieved by adding a Typed variant to all constructors/types in the latter three packages and making the Kind and Channel source constructors generic. This means that this change is 100% backwards-compatible in the builder

This change will require users to adjust their code only if:

• They directly call controller.Watch rather than the builder
• They use a Channel source
• They call builder.WatchRawSource with ...builder.WatchesOption
• They built their own source.Source

The above is IMHO likely a relatively small fraction of our users and can be assumed to have advanced knowledge of controller-runtime.

As a side-effect of keeping the builder compatible, users of it will not see this feature. Finding a way to support this in the builder outside of WatchesRawSource is left as a future task and will likely not have a solution with a nice UX due to golang/go#49085

Fixes #2214

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alvaroaleman

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [alvaroaleman]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[sbueringer]

Just did a first high-level review. Overall pretty nice!

[Danil-Grigorev]

It seems to me that having a typed EventHandler here is not as convenient as to have a method which will infer the type of the object. Client-go works with 2 interfaces under the hood, passed in a method if I’m not mistaken. Can this be implemented this way but with generics?

[alvaroaleman]

You mean a constructor for events and unexporting the current types? That would be a breaking change.

[Danil-Grigorev]

Rather leave Event structures for non generic types without any change, and introduce a new handler for generic types. I did it this way: https://github.com/Danil-Grigorev/controller-runtime/blob/4d770047de3a10110f5ded3bce46c178d68d0500/pkg/handler/eventhandler.go#L61-L73

[alvaroaleman]

What is the advantage of that? That means sources have to be able to deal with either or or there needs to be some sort of adapter to convert, both of which is very inconvenient. How does this improve the overall UX?

[Danil-Grigorev]

Mostly for the ability to infer type, I’d say. Anytime that you specify [anything] before the method, the syntax highlighting breaks. Might be only my personal observation, env, etc. The goal is that as controller-runtime is a library, exposing a list of interfaces to extend (in case you need this), having one which unconditionally requires to use generics might make it hard for newcomers to implement custom logic. With a method one can always use just *corev1.Pod as an arg to custom EventHandler implementation and won’t have to use advanced features immediately. And wrapping EventHandle into constructors to eliminate the issue didn’t make sense to me. I can’t imagine how a structure for events may grow beyond just hosting old/new objects, which is low enough to consider a method.

[alvaroaleman]

The goal is that as controller-runtime is a library, exposing a list of interfaces to extend (in case you need this), having one which unconditionally requires to use generics might make it hard for newcomers to implement custom logic.

Both your and my approach leave the currently-exported interfaces in place, they have the same shape in both approaches. The difference in your approach is that because EventHandler is not an alias to TypedEventHandler, a Source can only support one of the two, because for the go type system those are different types.

[Danil-Grigorev]

I’m trying to find a reason why I abandoned aliasing in the first place. Yes, it seems to be working, and having to specify a typedEvent[*Pod] is a minor inconvenience. My guess would be that the issue discussion lead to overcomplicating the solution. But I see a difference with using Source().Prepare() that the current builder logic was not refactored in my implementation. This means there will not be issues with existing functionality

[Danil-Grigorev]

Here is one problem as I can see. There should be an adapter method for EventHandler for a non-generic controller using type specific generic map method. Something like:

func EnqueueRequestsFromTypedMapFunc[T client.Object](fn TypedMapFunc[T]) EventHandler

The problem now is that EventHandler = TypedEventHandler[client.Object] and you can’t type cast from one generic type to another, regardless of the fact that both implement client.Object. That’s where the simpler form of type aliasing broke for me, and where this (obviously simpler) design is a breaking change. Sometimes you can’t pair generics with a project due to reasons like the one we are facing - using builder or any sort of nested structures. Go generics just don’t work where there is a type ambiguity. But using generics should not be either/or decision either, it just needs a way to make a split at some point where it no longer works or does not make sense, I think.

As for real world example where this is important - I rebased CAPI testing PR on your fork + new builder methods, all works well except one common method. It is the only blocker for it to compile and work, but it is a deal breaker if this has to be continued (cc @sbueringer):

return r.Tracker.Watch(ctx, remote.WatchInput{
    Name:         "machinepool-watchNodes",
 	Cluster:      util.ObjectKey(cluster),
 	Watcher:      &controller.ControllerAdapter{Controller: r.controller},
 	Kind:         &corev1.Node{},
 	EventHandler: handler.EnqueueRequestsFromMapFunc(r.nodeToMachinePool), // Does not compile because expects TypedMapFunc[client.Object] and gets TypedMapFunc[*corev1.Node]
 })

func (r *MachinePoolReconciler) nodeToMachinePool(ctx context.Context, node *corev1.Node) []reconcile.Request {

And there is no workaround except for having TypedEventHandler and EventHandler being separate interfaces with different methods working on client.Object and T simultaneously. Here is how I did it originally. I didn’t see a point in having TypedEvent structure either, as a simplification. Another user story for this - one less controller-runtime dependency for reusable business logic around event mapping exported in a package at the API level.

Edit: probably not that definite as I thought in CAPI scenario, as methods defined there for tracker are only used with remote tracker, so can still use client.Object. Still I think this is a limitation worth knowing about/decided.

[alvaroaleman]

Cluster-API was also bumped here and it doesn't look like the problem you are describing happened there: kubernetes-sigs/cluster-api#10460

The problem now is that EventHandler = TypedEventHandler[client.Object] and you can’t type cast from one generic type to another,

You can not type cast but you can use them interchangably. I still do not quite understand what this breaks that your approach doesn't. I am not very familiar with cluster-api, can you give a simpler example?

[sbueringer]

Cluster-API was also bumped here and it doesn't look like the problem you are describing happened there: kubernetes-sigs/cluster-api#10460

I think this didn't happen here because Christian rebased on this PR, and not on "your fork + new builder methods". Otherwise I don't follow, to be honest :)

[sbueringer]

@Danil-Grigorev Can you provide more details how this func is broken? (and push a branch somewhere where it is broken)

(I looked at kubernetes-sigs/cluster-api#10464 and that one is not broken, but I assume you're describing some other version of the code)

[alvaroaleman]

One detail that maybe is what you are referring to but I am not sure: It makes a difference if you do

type foo = bar[something]

or

type foo bar[something]

The latter means that you do not get the methods of bar and as a result, foo can not be used in place of bar[something]

But yeah, I also don't really understand the example given so I don't know if this might be what you mean.

[Danil-Grigorev]

Yes, as I mentioned, I just had to stick with non generic nodeToMachinePool method which kept using client.Object and that probably it wasn’t the right example, because nodeToMachinePool is only used once - in the tracker setup.

My concern is about more complex scenarios, when the same method has to be used with generic implementation elsewhere. It can’t be done without an adapter or code duplication purely for type casting. Which might lead to decision against using generics due to added complexity in having use methods interchangeably.

The example I provided is good in outlining the second problem related to golang/go#49085 as in builder pattern UX changes. We can’t make Tracker a generic method with specialization on *corev1.Node for the CAPI use case to make it simple, because the Tracker is stored in MachinePoolReconciler which works with MachinePools - it would look strange and may block MachinePoolReconciler from being a generic method on MachinePoolReconciler[*MachinePool] for example if needed.

This is theoretical, I’m struggling to find a real world example which would explain the point better, still looking though. I think controller-runtime should take a couple more steps further to make the library UX better aligned with generics, despite the golang/go#49085 limitation, and get the design of how it looks in the end with possible edge scenarios covered.

[alvaroaleman]

The only case I am aware of where in type foo = bar[something] foo and bar can not be used interchangably are slices: a []foo can not be used in place of []bar[something] but you can convert between them by iterating

[alvaroaleman]

This is theoretical, I’m struggling to find a real world example which would explain the point better, still looking though. I think controller-runtime should take a couple more steps further to make the library UX better aligned with generics, despite the golang/go#49085 limitation, and get the design of how it looks in the end with possible edge scenarios covered.

I am not disagreeing. But I do not think that your approach is better at that than the aliasing. The only thing that you added is the func builder.For(stuff) source.Source/func builder.Owns(stuff) source.Source. I think the only case where this might be useful is if you have predicates, as you can have proper types in there as the Handler is implicit and I am not fully convinced that warrants including such helpers in controller-runtime.

That being said, this change does nothing that would keep us from in the future adding such helpers or improving the UX further if golang/go#49085 gets resolved or we think of other ways to work around it.

[Danil-Grigorev]

Sounds good to me. Tried this once again and this approach seems to work:

func convert[T any](fn TypedMapFunc[T]) MapFunc {
	return func(ctx context.Context, obj client.Object) (req []reconcile.Request) {
		if typed, ok := obj.(T); !ok {
			return req
		} else {
			return fn(ctx, typed)
		}
	}
}

func EnqueueRequestsFromTypedMapFunc[T any](fn TypedMapFunc[T]) EventHandler {
	return &enqueueRequestsFromMapFunc[client.Object]{
		toRequests: convert(fn),
	}
}

[sbueringer]

Overall looks good

[Danil-Grigorev]

I think here a change suggested in #2685 will fit better. Projection might not work for different sources.

[alvaroaleman]

Projection might not work for different sources.

This is only used to construct a source.Kind.

I think here a change suggested in #2685 will fit better.

I disagree. Both what we did previously and what is done over there is IMHO hacky and will not work after the Source was started. Deferring the creation and creating it with the right args rather than manipulating it later on is IMHO much cleaner.

[Danil-Grigorev]

This can also use EventHandler[T any] without changes to allow watching non-kubernetes resources.

[alvaroaleman]

This is internal and only used by source.Kind which only works with client.Object, so there is no reason to have a different type constraint here