Finalizer Removal Race Condition / Json StrategicMergePatch Delete by Value

[ellistarn]

What happened?

I work on an operator that applies a finalizer on node objects. When a nodes is deleted, my operator checks for the existence of the underlying instance object and removes the finalizer if the instance is gone. Additionally, my operator applies ownerreferences to node objects that reference a parent object.

Here's the order of operations:

1. The parent object is deleted with foregroundDeletion
2. The foregroundDeletion finalizer propagates to the nodes
3. My finalizer controller observes the node object finalizers = { foregroundDeletion, myotherfinalizer }
4. My finalizer controller sends a mergepatch to the node object with finalizers = { foregroundDeletion }
5. I get the following error from the API Server

terminating node ip-192-168-53-118.us-west-2.compute.internal, removing finalizer from node, Node \"ip-192-168-53-118.us-west-2.compute.internal\" is invalid: metadata.finalizers: Forbidden: no new finalizers can be added if the object is being deleted, found new finalizers []string{\"foregroundDeletion\"}"

This quickly self heals and retries, but results in pretty noisy logs. My guess is that the API Server I'm receiving the watch event from is different from the API Server I'm sending the PATCH request to, and the second API Server doesn't yet have knowledge of the foregroundDeletion finalizer.

I was looking into ways to delete an object by value via a merge patch (e.g., ask specifically for my finalizer to be deleted, w/o knowledge of other finalizers), but it looks like this isn't currently supported by the spec: json-patch/json-patch2#18.

What did you expect to happen?

There are a couple of paths I see here:

1. Support for Json StrategicMergePatch Delete by Value
2. The admission controller checking for finalizers should do a consistent read to avoid stale data

How can we reproduce it (as minimally and precisely as possible)?

Minimal controller-runtime controller that removes a finalizer and trigger w/ foreground deletion request.

Anything else we need to know?

No response

Kubernetes version

kubectl version
WARNING: This version information is deprecated and will be replaced with the output from kubectl version --short.  Use --output=yaml|json to get the full version.
Client Version: version.Info{Major:"1", Minor:"24", GitVersion:"v1.24.2", GitCommit:"f66044f4361b9f1f96f0053dd46cb7dce5e990a8", GitTreeState:"clean", BuildDate:"2022-06-15T14:14:10Z", GoVersion:"go1.18.3", Compiler:"gc", Platform:"darwin/amd64"}
Kustomize Version: v4.5.4
Server Version: version.Info{Major:"1", Minor:"22+", GitVersion:"v1.22.10-eks-84b4fe6", GitCommit:"cc6a1b4915a99f49f5510ef0667f94b9ca832a8a", GitTreeState:"clean", BuildDate:"2022-06-09T18:24:04Z", GoVersion:"go1.16.15", Compiler:"gc", Platform:"linux/amd64"}
WARNING: version difference between client (1.24) and server (1.22) exceeds the supported minor version skew of +/-1

Cloud provider

EKS

OS version

# On Linux:
$ cat /etc/os-release
# paste output here
$ uname -a
# paste output here

# On Windows:
C:\> wmic os get Caption, Version, BuildNumber, OSArchitecture
# paste output here

Install tools

Container runtime (CRI) and version (if applicable)

Related plugins (CNI, CSI, ...) and versions (if applicable)

[ellistarn]

/sig api-machinery

[249043822]

your custom resource set ownerreferences to node, so while deleting node, the garbage collector controller will wait your object to be deleted and then remove foregroundDeletion finilizer, if just this time, your operator also remove your customer finilizer myotherfinalizer, it may cause the error you said above as conflict.
I think if you only want to delete node to cascade deleting your custom resource, you can only set ownerreferences and don't need set a finalizer, because the node resource already supports foregroundDeletion

[ellistarn]

I need my finalizer in all cases, even if the user uses --orphan on the parent resource. The challenge here is that I don't have a mechanism via the Kubernetes API to remove a single finalizer, and the API Server appears to have an eventually consistent view of the world when checking my patch.

[cici37]

/assign @caesarxuchao @lavalamp
/triage accepted

[lavalamp]

Have you tried with serverside apply? If we have annotated the type correctly, you should be able to do this.

[ellistarn]

Will give it a shot. Will that merge lists?

[lavalamp]

It will.

Lists of primitives don't have a direct deletion incantation. You may have to own it and then send a change omitting it to have the server remove it.

Fortunately you probably already own it -- you can look at existing objects to see if it is owned separately and what the manager's name is if so.

cc @apelisse

[apelisse]

Lists of primitives don't have a direct deletion incantation. You may have to own it and then send a change omitting it to have the server remove it.

What this means is that every time you do a server-side apply, you MUST specify the finalizer, until you want to remove it, then you should apply without it and it will drop it.
Technically, another way to do that would be to have a "finalizer-applier" that applies the finalizer once and then applies without it at the end to remove it, not sure if I would recommend that.

[k8s-triage-robot]

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

This bot triages issues and PRs 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 or PR as fresh with /remove-lifecycle stale
• Mark this issue or PR as rotten with /lifecycle rotten
• Close this issue or PR with /close
• Offer to help out with Issue Triage

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

/lifecycle stale

[ellistarn]

Thanks!

[jonathan-innis]

Just in case anyone else runs across this issue, this is also solved by strategic merge patching (which has a delete by key concept on primitive lists), the only problem with that solution is that it only works for core API types, CRDs still don't have support for strategic merge patching; so, a good out if you don't want to go down the SSA path is just to do a plain Update and deal with the conflicts.

[dims]

CRDs still don't have support for strategic merge patching

@jonathan-innis wanna open an issue for this? (is there already one?)

[liggitt]

There is no plan to support strategic merge patch on CRDs. JSON patch, merge patch, and server-side apply are it.