Feature Request: Finalizer handler scaffolded by default for go/v3

[rashmigottipati]

Overview:
In Kubernetes garbage collection model, OwnerReferences are allowed within a namespace wherein the "dependent" object has anOwnerReference field referencing the "parent" object. If the parent object is being deleted, Kubernetes garbage collector will delete all of its dependent objects. As cross-namespace owner references are not allowed by design; a Finalizer should be used to perform cleanup.

Finalizers are more powerful and broadly applicable than owner references because they permit customizable deletion steps for a controller to define how an object is cleaned up, ex. updating a CR's status during deletion with a particular message. This gives Operator authors control over CR deletion proceeds. Not only that, but operator projects would benefit from some initial finalizer code scaffolded by default which will make it easy for operator authors to fill in operand-specific cleanup code to the existing scaffolded code.

Proposal:
To enable the cleanup process using Finalizers, my proposal is to implement a finalizer "no-op" handler scaffolded by default in the kubebuilder go/v3 plugin. The idea is to always scaffold out a finalizer and some code that adds the finalizer to non-deleted CRs, and a delete event handler in the Reconciler that runs if the finalizer is present on a CR.

The Reconciler can be modified to check for presence of a Finalizer in a CR object. If the CR object is marked for deletion, which is indicated by metadata.deletionTimestamp being set, the Reconciler runs the finalizer logic and removes the finalizer. If the finalization logic fails for some reason, the Reconciler would keep the finalizer to retry during the next reconciliation. Typically, the finalizer logic would entail necessary cleanup steps that the operator needs to perform before the CR can be deleted. The finalizer no-op handler will scaffold some initial finalizer code that does not clean up a CR but has a TODO(user) comment by default for all operators, which lets the operator authors fill in additional cleanup steps as required.

/kind feature

[rashmigottipati]

/assign

[rashmigottipati]

/cc @DirectXMan12 @estroz @jmrodri

[estroz]

Something like this would be nice

import (
    "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
	foov1 "github.com/example/foo-controller/api/v1"
)

const fooFinalizer = "foo.example.com/finalizer"

func (r *FooReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {

    foo := &foov1.Foo{}
    err := r.Get(ctx, req.NamespacedName, foo)

    if !controllerutil.ContainsFinalizer(foo, fooFinalizer) {
        controllerutil.AddFinalizer(foo, fooFinalizer)
        if err := r.Update(ctx, foo); err != nil {
            return ctrl.Result{}, err
        }
    }

    if foo.GetDeletionTimestamp() != nil {
        return ctrl.Result{}, r.handleDeleteFoo(foo)
    }

}

func (r *FooReconciler) handleDeleteFoo(o *foov1.Foo) error {
	if controllerutil.ContainsFinalizer(foo, fooFinalizer) {

		// TODO(user): Add the cleanup logic here

        controllerutil.RemoveFinalizer(foo, fooFinalizer)
      	if err := r.Update(ctx, foo); err != nil {
    		return ctrl.Result{}, err
	    }
    }
    return nil
}

[camilamacedo86]

From v1 to v2 one of the changes was simplified the layout. It generates a clean project, without any code in the reconcile, since v2.

IMO this implementation should be part of the EP: #1803 where we would have a plugin responsible to generate the full code to install/manage an image follow all good practices. (arch-type feature)

Then, by following the good practices this code would also have the finalizer, such as the status conditionals and etc.

In this way, I do not think that we should start to add code in the reconcile. Otherwise, what would be the rule over what can be accepted or not be in the default/clean code? Is a Finalizer a requiriment for any project? I do not think so.

However, it might a good subject to be brought into the "Kubebuilder, Controller Runtime, and Controller Tools Meeting"

[estroz]

Otherwise, what would be the rule over what can be accepted or not be in the default/clean code?

If something is generally useful I’m in favor of adding it to a project, as the key reasons to being opinionated about how a controller should look is to ease development and enforce best practices. Finalizers are generally useful and are a best practice for complex CRs imo. However I sympathize with the argument that not all CRs need to be finalized (even though it’s probably worth getting devs into the habit of doing so).

IMO this implementation should be part of the EP: #1803 where we would have a plugin responsible to generate the full code to install/manage an image follow all good practices.

This is a reasonable suggestion if it’s decided to not be part of default scaffolding. A plugin makes more sense if finalizer handling code only needs to be scaffolded once.

However, it might a good subject to be brought into the "Kubebuilder, Controller Runtime, and Controller Tools Meeting"

Sounds good.

[Adirio]

I definetely like the idea of implementing this as a plugin. Currently the go plugin is doing too many things and we should move some of them to separate plugins so that different parts can be removed/replaced. Once plugin phase 1.5 (#1990) is operational, this could be implemented as a plugin that is chained with the go plugin. finalizer.go.kubebuilder.io seems like a good name as this plugin wouldnt make sense on non-go projects.

A different topic is if this should be the default or non-default behavior. Right now go.kubebuilder.io/v3 is the default plugin but when plugin phase 1.5 is implemented, we could choose that the default set of plugins is go,kubebuilder.io/v3,finalizer.go.kubebuilder.io.

Just as an example, helm and go plugins share the generation of the config directory, we may want to extract this to a different plugin so that we can use config,go as default, config,helm for helm only projects, or even config,go,helm for hybrid projects. And in a future config could be replaced by the alternative that was demoed some meetings ago (#1831) instead of scaffolding the config directory.

[camilamacedo86]

HI @estroz, @Adirio, @rashmigottipati

I updated the EP for its implementation be a plugin as suggested before and also I added the finalizer as a requirement for that. See; #1803

Also, IMO we should not add the code suggested in #2010 (comment) and instead of that, we could scaffold the code/example described in the SDK docs with the TODO:// add here your finalizer conditionals. See: https://sdk.operatorframework.io/docs/building-operators/golang/advanced-topics/#handle-cleanup-on-deletion which shows more understandable and maintainable.

[camilamacedo86]

We reach a consensus that it would be very nice as a plugin. We just need to get merged first the implementation of the 1.5 EP proposal first. see: #1991.

After that, would be possible to run create api --plugins=your new plugin here to generate the controller with.

[joelanford]

I've been thinking a bit about proposing a finalizer helper library for inclusion in controller-runtime, and I just haven't had the time.

My high level thought was to have something like this:

package finalizer

type Registerer interface {
	Register(key string, f Finalizer) error
}

type Finalizer interface {
	Finalize(client.Object) (requeue bool, err error)
}

type Finalizers interface {
	// implements Registerer and Finalizer to finalize
    // all registered finalizers if the provided object
    // has a deletion timestamp or set all registered
    // finalizers if it doesn't
	Registerer
	Finalizer
}

func NewFinalizers() Finalizers {
	...
}

With something like this users could define their own finalizer struct implementations, register them with the Finalizers and then hand the Finalizers to their reconciler and simply call Finalize:

obj := groupv1.MyObject
_ = r.Client.Get(ctx, req, &obj)
requeue, err :=; r.finalizers.Finalize(obj)
if requeue {
	return ctrl.Result{}, err
}

I haven't prototyped this out yet, so this probably isn't exactly right, but I do feel like there are some abstractions we could include in controller-runtime to make finalizers easier to use. If folks agree, perhaps we could take some time to explore this before scaffolding anything in the Go plugin?

[camilamacedo86]

I liked much more the idea of it be provided as lib from controller-runtime than the plugin 👍
Then, it would be a helper for any person who uses the lib and I think it fits better as well.

[Adirio]

I liked much more the idea of it be provided as lib from controller-runtime than the plugin 👍

I agree that direct support in controller-runtime would be a better implementation but I think that the plugin still makes sense. I'm not sure if I'm picturing it as Joe is picturing but in my mind conceptually this is similar to webhooks. Controller-runtime defines the required interfaces and kubebuilder scaffolds the methods required to implement these interfaces. So the plugin still would make sense.

[camilamacedo86]

I also think that plugins are nice. However, if we push the impl for the controller-runtime then, I do not see value in having a plugin only to call its usage. I think that the plugins can add a lot of value but in this case, I do not think that implement a plugin and keep it maintained just to scaffold a code that uses controller-runtime adds value which justifies this effort.

[Adirio]

The whole kubebuilder project is scaffolding code that uses controller-runtime. The value provided will depend on how it is implemented in controller-runtime and what does the user have to code himself. And as that is still not defined there is no point in discussing if the plugin would be useful or not right now, we should wait for controller-runtime to implement it.

[rashmigottipati]

Would it be better to have this discussion in the "Kubebuilder, Controller Runtime Meeting" to reach consensus on whether it would fit better as a plugin or write a finalizer helper library in controller-runtime?

[camilamacedo86]

Hi @rashmigottipati,

I think it was not added in the controller-runtime meeting. WDYT about follow up on the @joelanford suggestion #2010 (comment) and raise an issue controller-runtime?

see that we also added it in EP https://github.com/kubernetes-sigs/kubebuilder/blob/master/designs/code-generate-image-plugin.md as well.

[rashmigottipati]

Hi @camilamacedo86,

As per the offline discussion, will raise an issue on controller-runtime and explore the possibility of implementing this as a finalizer helper library.

Everyone - thanks for your valuable inputs and suggestions!

[camilamacedo86]

Hi @rashmigottipati,

Could you please like the controller-runtime issue here? Also, wdyt bout close this one and move with the controller-runtime one? We can re-open it if we need always it as well.

[rashmigottipati]

Closing this in favor of kubernetes-sigs/controller-runtime#1453 in controller-runtime.