Fix: Use Patch instead of Update to add/remove finalizers #1801
Conversation
Baarsgaard
requested review from
NissesSenap,
weisdd,
ishanjainn,
theSuess,
hubeadmin and
pb82
as code owners
Baarsgaard
force-pushed
the
fix_finalizer_cfg_drift
branch
from
172cf54 to
1fa382c
Compare
Baarsgaard
changed the title
| // Remove finalizer through a MergePatch | ||
| // Avoids updating the entire object and only changes the finalizers | ||
| func removeFinalizer(ctx context.Context, cl client.Client, cr client.Object) error { | ||
| patch := []byte(`{"metadata":{"finalizers":null}}`) |
There was a problem hiding this comment.
Although it's nice that you can fix the drift with these changes, I would disagree with the way how patches are formed. - Kubernetes objects may have multiple finalizers, whereas, in this PR, there is an assumption that there will always be a single finalizer (owned by the grafana-operator). - Some users might have their own finalizers (think of custom operators).
Since controllerutil has two functions that safely add (AddFinalizer) / remove (RemoveFinalizer ) a particular finalizer, you can, for instance, have them modify finalizers in a CR object and then prepare a patch by copying value from metadata.finalizers.
There was a problem hiding this comment.
Sure, I'll try and whip up a solution that safely patches the finalizers.
Managed to solve the perpetual drift noted in #1776 that is rooted in us applying finalizers and updating unintended fields on CRs.
I first attempted to use
JSONPatch, but I could not for the life of me get the libraries to comply, despite handcrafting a working patches with kubectl.MergePatchworked immediately.Currently, the implementation is hardcoded to only support arrays of one finalizer. More work can be done to make this support multiple if the need ever arises.