generalize derive attrs for kube-derive

[clux]

• adds #[kube(finalizer = "magic.finalizer.string")] attr to kube-derive which is hooked into Foo::new
• adds serde_json to kube-derive so we use less hacky json serialization within the crate
• fixes Derive derive attributes #243 by allowing customizing additional derives for the generated struct

[nightkr]

This looks more like something that belongs in the runtime IMO, since we don't want to forget adding finalizers for objects created by others (kubectl, dashboard, other controllers, etc).

[clux]

if objects are created by others, or if you get it from somewhere else, the finalizer label should already be there

[clux]

thoulgh we should definitely have some functionality for marking finalization as done at least.
(i.e. remove the same magic string from the finalizer list in the object we are a finalizer for, so deletion can continue)

[nightkr]

But that makes the finalizer an unchecked part of the public CRD API that we expose to outsiders.

To me, finalizers are a statement made by a controller that "I want to know when this disappears". It's primarily useful for the CRDs that your controller owns, but it's fully legal to apply it to anything else (for example, imagine a logging system that wants to reliably log all deleted long-lived objects).

[clux]

It sounds like you're talking about deleting everything related to a CRD? ownerReferences?
If your controller manages many CRDs then it's a bit of a bad pattern.

To me, the finalizer is an arbitrary named string telling kubernetes not to delete the object until the finalizer that knows that name has ensured external garbage collection has been done (stuff that can't be done simply with ownerReferences), and that finalizer then removes its name from the list, so that any other finalizers can continue.

If you had one controller that handled all the deletion, you wouldn't need finalizers, but you would also have a monolith. This feels like a concern not simply for a controller, but the creator (declaring what it wants garbage collected), and the controller (watching what is required to garbage collect).

[nightkr]

Yeah.. that might not have been the best example.

What I meant is that, to me, the finalizer is a concern of the controller. The typical K8s behaviour is to delete the backing resource when an object is deleted. Ingress-nginx doesn't keep respecting deleted Ingress objects just because you didn't specify the unregister.ingress-nginx.k8s.io finalizer, and Pods aren't kept running on the node just because you forgot to add a kill-pod.k8s.io finalizer.

There is also the consistency argument.. There are other mechanisms for cleanup that do not require finalizers on the externally created object (such as ownerReferences, as used in the FooClaim-owns-Foo pattern), so I don't really think the object creator should have to care about which mechanism the controller uses for its cleanup.

[clux]

The consistency argument should ideally make everyone use ownerReferences, because that's a nice branching tree of cleanup with a nice model, and want to heavily advocate for using those everywhere.

However, there is a case when that's not nice: blocking third party api cleanup

Haven't dealt with the nginx ingress myself, but it sounds like it offers the choice between blocking and non-blocking deletion by supporting both a finalizer and ownerRefs.

Example: I have a case where I need blocking third party api cleanup (external ping status service that needs to have its entry removed before the service is dead - otherwise pagers hit). This is what the finalizer is preventing, and the creator is enabling a delay on the main service's deletion by adding the finalizer it feels should handle it.

[nightkr]

ownerReferences can also block deletion via blockOwnerDeletion: https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.18/#ownerreference-v1-meta

It does require a separate finalizer (foregroundDeletion), but it's a binary switch: there is no way to enable foregroundDeletion for one owned Kind but not another.

[clux]

oh, that's cool. That's also useful. For other use cases at least. At least I don't see this helping with third party resources, which I can't really shoehorn under ownerReferences.

Stepping back; I feel the core thing you are concerned about is exposing an unchecked api for Foo::new that sets only a subset of the finalizers. Is that correct?

Even if you have both a controller (creating child objects and updating the main crd's .status), and a finalizer controller (gc'ing third party objects and modifying crd.metadata.finalizers) as separate programs, it's not really a problem that the finalizer string is unchecked. Neither of these programs are in charge of creating the CRD (with Foo::new), they only handle objects received from the network and that will already have the label.

[nightkr]

My core concern isn't with the Rust API itself, but with the Kubernetes API that the Rust API ends up encouraging.

Let's say we have a regular Foo+FooClaim pair. Foo requires some cleanup and thus has a finalizer, and is owned by the FooClaim. If the user always creates the Foo via FooClaims then this approach is fine: the FooClaim controller always calls Foo::new() which always sets the finalizer. But we can't prevent the user (or some external controller) from creating a Foo directly, in which case suddenly that external actor has to know about the implementation detail of how Foo's controller implements cleanup.

You can argue about whether creating the Foo directly is a good idea, but it's still a fairly common thing to do. For example, we usually manage Pods via ReplicaSets (or at least Jobs), but sometimes you really do want to manage the Pod directly.

[clux]

Creating should only be the done by a single creator. At least that's what we should advocate. Foo::new if via the API, or kubectl apply -f through CI.

Sure, low level kube objects like Pod do experience usage from other controllers as it's a core workload, but that multi-controller setup is generally the exception (and they also generally tend not have finalizers).

That said, if you do need to have multiple appliers, with finalizers, yeah, you have perhaps dug yourself into a bit of a hole. But simultaneously; by choosing to use the CRD in the second place you are also in a position to think about what features you need. E.g. it's perfectly reasonable to not run extraneous finalizers for api gc if you're in a test environment.

The external actor doesn't need to know about how anything is cleaned up, it simply needs to opt in to that type of garbage collection.

[clux]

Having thought some more about this on the weekend; even if finalizers are opt-in, I'm starting to realise it's still a bit of an encapsulation concern to have the finalizer settings defined on the struct itself (with a derive attr). Because as Foo::new now come with a default set of finalizers it effectively couples the struct definition to the behaviour (selection of gcs).

Maybe this is what you meant all along (sorry if it was).

At any rate, it's probably better to explicitly move this type of behavioural logic to appliers (or inside yaml), as that lets the real owner dictate how gc is done (not how the definitions feel it should be by default - forcing people undoing my magic by clearing finalizers).

Will remove the finalizer derive attr (and its part in Foo::new) in the pr before progressing (the rest should be less controversial).