Add a higher-level "canonical" reconciler API

[JamesOwenHall]

Controller-runtime strikes a good balance between simplifying the development of Kubernetes controllers and still being very flexible. The flexibility of the reconcile.Reconciler interface should be maintained but I believe there would be value in also providing a slightly higher-level abstraction to reduce the duplication of common controller patterns.

In my experience, controllers can end up looking like the following before even implementing any business logic.

const ExampleFinalizer = "my-finalizer"

func (r *ExampleReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
	example := &examplev1.Example{}
	if err := r.Get(ctx, req.NamespacedName, example); err != nil {
		return ctrl.Result{}, fmt.Errorf("failed to get example: %w", err)
	}

	if !example.ObjectMeta.DeletionTimestamp.IsZero() {
		if controllerutil.ContainsFinalizer(example, ExampleFinalizer) {
			if err := r.finalize(); err != nil {
				return ctrl.Result{}, fmt.Errorf("failed to finalize example: %w", err)
			}

			controllerutil.RemoveFinalizer(example, ExampleFinalizer)
			if err := r.Update(ctx, example); err != nil {
				return ctrl.Result{}, fmt.Errorf("failed to update example: %w", err)
			}
		}
	
		return ctrl.Result{}, nil
	}

	if !controllerutil.ContainsFinalizer(example, ExampleFinalizer) {
		controllerutil.AddFinalizer(example, ExampleFinalizer)
		if err := r.Update(ctx, example); err != nil {
			return ctrl.Result{}, fmt.Errorf("failed to update example: %w", err)
		}

		return ctrl.Result{}, nil
	}

	original := example.DeepCopy()
	businessLogicErr := r.doBusinessLogic(example)

	if equality.Semantic.DeepEqual(original, example) {
		return ctrl.Result{}, nil
	}

	if err := r.Update(ctx, example); err != nil {
		return ctrl.Result{}, fmt.Errorf("failed to update example: %w", err)
	}

	if businessLogicErr != nil {
		return ctrl.Result{}, fmt.Errorf("failed to do business logic: %w", businessLogicErr)
	}

	return ctrl.Result{}, nil
}

I propose adding a higher-level interface like the following to complement the existing reconcile.Reconciler interface. This builds upon #2221 but extends beyond just the use of generics.

package canonical

// PointerObject is anything that is a pointer AND a client.Object, not a pointer to a client.Object. This type is
// needed to allow us to Get objects using generics. Example:
// 
//     var obj PO = new(T)
//     r.client.Get(ctx, req.NamespacedName, obj)
// 
type PointerObject[T any] interface {
	*T
	client.Object
}

// Reconciler is a type that can be used to reconcile an object in Kubernetes. It behaves similarly to
// reconcile.Reconciler, but it handles some of the boilerplate for you such as handling finalizers and
// updating the object if the reconciler modifies it.
type Reconciler[T any, PO PointerObject[T]] interface {
	// Reconcile is called when the object is created or updated. It is passed the object that was created or updated.
	// Deleted objects are not passed to this method, but instead to Finalize. If Reconcile modifies the input object,
	// the object will be automatically updated in Kubernetes.
	Reconcile(ctx context.Context, obj PO) (ctrl.Result, error)

	// Finalizer is the name of the finalizer that will be added to the object. If this is empty, no finalizer will be
	// added and Finalize will not be called.
	Finalizer() string

	// Finalize is called when the object is deleted if Finalizer returns a non-empty string. It is passed the object
	// that was deleted. If Finalize returns a nil error, the finalizer will be removed from the object in Kubernetes.
	Finalize(ctx context.Context, obj PO) error
}

// New accepts a canonical.Reconciler and returns a reconcile.Reconciler that performs all the boilerplate from the
// example in the first code snippet.
func New[T any, PO PointerObject[T]](inner Reconciler[T, PO], client client.Client) reconcile.Reconciler {
    // …
}

Alternative designs could make Finalizer and Finalize part of a separate optional interface such that reconcilers would only need to implement them if they used finalizers.

This kind of interface would reduce a reconciler to pretty much just the business logic.

const ExampleFinalizer = "my-finalizer"

func (r *ExampleReconciler) Reconcile(ctx context.Context, ex *examplev1.Example) (ctrl.Result, error) {
    // business logic
}

func (r *ExampleReconciler) Finalizer() string {
    return ExampleFinalizer
}

func (r *ExampleReconciler) Finalize(ctx context.Context, ex *examplev1.Example) error {
    // finalizing logic
}

I believe a higher-level API like this could help reduce boilerplate and therefore reduce bugs and code duplication, and make controller-runtime more approachable. This new API would not replace the existing reconcile.Reconciler API, but rather it would be an opinionated alternative for controllers that fit the "canonical" model.

[alvaroaleman]

Not sure about the finalizer aspect, because that is optional, reconcilers might or might not need one. The rest generally makes sense

[troy0820]

/kind support feature

[JamesOwenHall]

Not sure about the finalizer aspect, because that is optional, reconcilers might or might not need one.

That's a fair point. Reducing boilerplate is my main goal with this proposal so the interface should probably be kept as small as possible. Perhaps finalizers could be supported by implementing an optional interface. That would keep canonical.Reconciler small while still supporting finalizers, which I think is still valuable.

package canonical

// PointerObject is anything that is a pointer AND a client.Object, not a pointer to a client.Object. This type is
// needed to allow us to Get objects using generics. Example:
// 
//     var obj PO = new(T)
//     r.client.Get(ctx, req.NamespacedName, obj)
// 
type PointerObject[T any] interface {
	*T
	client.Object
}

// Reconciler is a type that can be used to reconcile an object in Kubernetes. It behaves similarly to
// reconcile.Reconciler, but it handles some of the boilerplate for you such as handling finalizers and
// updating the object if the reconciler modifies it.
type Reconciler[T any, PO PointerObject[T]] interface {
	// Reconcile is called when the object is created or updated. It is passed the object that was created or updated.
	// Deleted objects are not passed to this method. Deleted objects can be handled by also implementing the Finalizer
	// interface, however that is optional.
	Reconcile(ctx context.Context, obj PO) (ctrl.Result, error)
}

// Finalizer is an interface that can be implemented by a Reconciler to add a finalizer to the object. If the object is
// deleted, Finalize will be called. If Finalize returns a nil error, the finalizer will be removed from the object in
// Kubernetes.
type Finalizer[T any, PO PointerObject[T]] interface {
	// Finalizer is the name of the finalizer that will be added to the object. If this is empty, no finalizer will be
	// added and Finalize will not be called.
	Finalizer() string

	// Finalize is called when the object is deleted if Finalizer returns a non-empty string. It is passed the object
	// that was deleted. If Finalize returns a nil error, the finalizer will be removed from the object in Kubernetes.
	Finalize(ctx context.Context, obj PO) error
}

[alvaroaleman]

I guess optional interface is a possibility but I am not a huge fan of this, as it means that we'd be adding even more inversion of control which IMHO makes the code harder to understand, as you need to know things that are not visible in the code of a given reconciler.

Other than that, the reconciler interface can be simplified to

type Reconciler[T client.Object] interface {
  Reconcile(context.Context, T)(reconcile.Result, error)
}

There is IMHO no need to complicate things in an attempt to enforce the client.Object implementation to be a pointer, it almost always is and if it isn't, people probably have their reasons.

[JamesOwenHall]

I guess optional interface is a possibility but I am not a huge fan of this, as it means that we'd be adding even more inversion of control which IMHO makes the code harder to understand, as you need to know things that are not visible in the code of a given reconciler.

I agree that the visibility of the finalizing behaviour is not great with the use of the optional interface. If the Finalizer and Finalize methods were part of the canonical.Reconciler interface as they were in the initial issue description, then I think the visibility is better because reconcilers would be forced to implement the methods. As you mentioned, not every reconciler needs a finalizer so Finalizer and Finalize could be ~6 lines of unnecessary boilerplate for those reconcilers, but I personally believe it's a worthwhile concession since it would still be less boilerplate overall, especially for those reconcilers that do use a finalizer. What do you think?

Perhaps it would be beneficial to add a simpler typed.Reconciler that the canonical.Reconciler could build upon. This would provide an additional option that doesn't invert much control at all. Would you reconsider the finalizing behaviour in the canonical.Reconciler API if we also provided typed.Reconciler?

package typed

type Reconciler[T client.Object] interface {
	Reconcile(context.Context, T) (reconcile.Result, error)
}

type reconciler[T any, PO PointerObject[T]] struct {
	inner  Reconciler[PO]
	client client.Client
}

func (r *reconciler[T, PO]) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
	// We need the type parameter T here to initialize obj because we can't initialize a generic type with a literal and
	// obj must not be nil. For example `PO{}` is invalid.
	var obj PO = new(T)
	if err := r.client.Get(ctx, req.NamespacedName, obj); err != nil {
		return ctrl.Result{}, err
	}

	return r.inner.Reconcile(ctx, obj)
}

Other than that, the reconciler interface can be simplified to

type Reconciler[T client.Object] interface {
  Reconcile(context.Context, T)(reconcile.Result, error)
}

There is IMHO no need to complicate things in an attempt to enforce the client.Object implementation to be a pointer, it almost always is and if it isn't, people probably have their reasons.

You're right, the Reconciler interface doesn't need to enforce that the type is a pointer. I believe the PointerObject may still be needed to call client.Get() but that shouldn't require Reconciler to have it in its signature.

func (r *typedReconciler[T, PO]) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
	// We need the type parameter T here to initialize obj because we can't initialize a generic type with a literal and
	// obj must not be nil. For example `PO{}` is invalid.
	var obj PO = new(T)
	if err := r.client.Get(ctx, req.NamespacedName, obj); err != nil {
		return ctrl.Result{}, err
	}

	return r.inner.Reconcile(ctx, obj)
}

[alvaroaleman]

I agree that the visibility of the finalizing behaviour is not great with the use of the optional interface

What I meant is that the visibility of the finalizer behavior is not great due to the inversion of control, regardless of if the interface is optional or not. I don't think the six lines or what it is to add the finalizer if obj is undeleted and to finalize and then remove the finalizer if not are that big a deal. If they are to you, you ofc have the option to build something downstream.

I believe the PointerObject may still be needed

You can use reflect to do this (and I seem to remember they wanted go itself to offer something similiar):

var o new(T)
o = reflect.New(reflect.TypeOf(o).Elem()).Interface().(T)

[JamesOwenHall]

Right, it's clear that the finalizing behaviour is too much inversion of control. So what do you think about starting off with simply a "typed" reconciler? By this, I mean the code example in this comment but with the use of reflection to simplify the generics. This would seem to me to cover the most common use cases with the least amount of inversion of control. I'd be willing to open a PR for this.

[troy0820]

Are we just trying to abstract away the ctrl.Request and the initial Get that the reconciler does on each request?

So that we can just get to the business logic around the Reconciler?

From the context of this issue/discussion, it sounds like the boilerplate associated with this is the consensus with how we approach interface we want to export.

The interface defined from both of you all seems to get to the object being the thing we would act upon and allow us to not have to provide the Get/apierrors song and dance.

type Reconciler[T client.Object] interface {
  Reconcile(context.Context, T)(reconcile.Result, error)
}

However, if what is underlying has to account for trying to replicate the logic around both reconciler interfaces (controller using reconciler.Reconciler) we would have to account for that unless we redo the same logic around this newly introduced use case to then not return the c.Do.Reconcile(ctx, req) but `c.Do.Reconcile(ctx, T) if and only if the object exists from the client it has.

Aside from the reflection semantics (generic or otherwise), would this be the direction we want to go in?

[alvaroaleman]

yeah I'm in general open to the idea, which is why I opened #2221 for it. The main questions I can think of would be a) How do we name this (we have to keep the current Reconciler interface) and how would we plug this into the builder?

[JamesOwenHall]

However, if what is underlying has to account for trying to replicate the logic around both reconciler interfaces (controller using reconciler.Reconciler) we would have to account for that unless we redo the same logic around this newly introduced use case to then not return the c.Do.Reconcile(ctx, req) but `c.Do.Reconcile(ctx, T) if and only if the object exists from the client it has.

Conceptually, I think of a typed reconciler as being something that is slightly higher-level than the reconcile.Reconciler interface. Therefore, it would seem appropriate to me for the Builder to only deal with reconcile.Reconciler and have an adapter from a "typed" reconciler to a conventional reconcile.Reconciler. For example.

// Maybe in sigs.k8s.io/controller-runtime/pkg/reconcile/typed
package typed

type Reconciler[T client.Object] interface {
    Reconcile(context.Context, T) (reconcile.Result, error)
}

// New returns an adapter that accepts a typed.Reconciler and returns a reconcile.Reconciler.
func New[T client.Object](client client.Client, rec Reconciler[T]) reconcile.Reconciler {
    // …
}

how would we plug this into the builder?

// Users of controller-runtime can then use the existing builder as-is simply by calling `typed.New()`.

func (r *MyTypedReconciler) SetupWithManager(mgr ctrl.Manager) error {
	return ctrl.NewControllerManagedBy(mgr).
		For(&v1.Example{}).
		Complete(typed.New(r.client, r))
}

What do you think?

[alvaroaleman]

I think I'd prefer to keep things on the same package, otherwise it might be confusing (hence the question for the name). Maybe KubernetesReconciler?

Using a constructor works. An alternative might be to have a CompleteKubernetesReconciler or such, it might be easier to discover.

[JamesOwenHall]

Personally, I find the name KubernetesReconciler pretty vague. All reconcilers are responding to Kubernetes events. The main differentiator for this kind of reconciler is that it has deserialized the object into a concrete type, isn't it? This is why I initially suggested the word "typed".

If a subpackage is too confusing, then perhaps simply TypedReconciler?

[alvaroaleman]

All reconcilers are responding to Kubernetes events.

No. If they were, there would be no reason to keep the existing reconcile.Reconciler interface. And the concrete type in this interface is not any concrete type, its a Kubernetes object, that is how I arrived at the name. I do agree my suggestion isn't particularly great, but I can't come up with something better offhand and I am not a fan of TypedReconciler because reconcilers might act on non-Kubernetes objects.

[JamesOwenHall]

Ah okay, apologies. There's a gap in my understand of controller-runtime reconcilers. My understanding was that reconcilers were always tied to Kubernetes objects. If that's not the case, then perhaps KubernetesReconciler isn't such a bad name.

[vincepri]

Either KindReconciler (related to Kind source and Kubernetes' Kind fields), or ObjectReconciler (e.g. client.Object, a runtime object) would be a bit more clear

[sbueringer]

I would pick ObjectReconciler

[JamesOwenHall]

Correct me if I'm wrong, but it seems like everyone who has commented on this issue likes the idea even if there's still some open debate on the exact naming of the API. I haven't contributed to controller-runtime yet, but I see that the contributing guidelines state that the proposal should be accepted before opening a PR. Is the naming something you'd like to sort out before accepting the proposal, or should that be discussed in a PR?

[vincepri]

Naming and technical design is usually sorted in a proposal PR

[JamesOwenHall]

Ok thanks for clarifying. Is there a formal way a proposal is accepted or should I just get started on a PR (after I sign the CLA)?

[alvaroaleman]

Just create a PR, no need to be so hesitant :) We can discuss details on the PR.

[troy0820]

This being merged should close this? Do we want to keep this open to keep the discussion going? @alvaroaleman @sbueringer @vincepri

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue as fresh with /remove-lifecycle stale
• Close this issue with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[JamesOwenHall]

/close

[k8s-ci-robot]

@JamesOwenHall: Closing this issue.

In response to this:

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.