fix: KubeadmControlPlane should allow orphans

[sethp-nr]

What this PR does / why we need it:

Check for the "orphan" finalizer and avoid deleting dependents if present.

It looks like the change in #2334 was incomplete: my controller manager was refusing to honor the orphan finalizer for some reason I couldn't discern, but the KCP controller was happy to ignore my intent and delete dependent resources.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: sethp-nr
To complete the pull request process, please assign luxas
You can assign the PR to them by writing /assign @luxas in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[sethp-nr]

I'm not too sure about this change – why are we deleting the dependents anyway and pre-empting the GC? Would it make more sense to only do the parallel deletion just in case the foregroundDeletion finalizer is set?

/hold

[detiber]

The current behavior is really no different than other places where we enforce deletion ordering in Cluster API (mainly to avoid the requirement that someone set foregroundDeletion = true) in order to be able to properly delete a Cluster and it's dependents without orphaning resources.

Do you have use cases in mind where deletion should orphan resources?

[sethp-nr]

Oh deletes, such joy you bring me. Specifically the use case I had in mind is one where it's requested by the user, as in:

k delete kcp FOO --cascade=false

The way --cacade=false is implemented is that it sets an orphan finalizer on the parent object and then waves hands the owner references are removed. My controller manager took a nap, and so the KCP controller got there first and cascaded the delete despite my intent.

I could try to remember in the future that "cascade=false" doesn't work for CAPI objects, but that wouldn't be my preference. To that end, I'm wondering where the breakdown occurs if the CAPI controller doesn't issue the DELETE: perhaps kubernetes/kubernetes#86509 is a related concern here?

[detiber]

I'm not necessarily sure it would have an immediate impact on KubeadmControlPlane, but in the case of Cluster -> Machine, Machine -> InfraMachine, Cluster -> InfraCluster, and Cluster -> Machine -> InfraMachine, if the parent object is not present then we would currently have a lot of potential for either breakage or orphaning of cloud resources if we didn't enforce the deletion semantics we currently do.

[sethp-nr]

Yeah, I'm beginning to understand that this is probably an issue with a larger scope than just a PR to the KCP controller: it sounds like we're hanging out at the intersection of at least one api-server bug, one confusingly named field (and/or poor default), and a tremendous number of moving parts.

Especially since this is not something unique to the KCP behavior, I'm thinking the right thing to do is close this PR and open an issue or CAEP for discussion post-v1alpha3. Does that sound right to you?

[detiber]

Especially since this is not something unique to the KCP behavior, I'm thinking the right thing to do is close this PR and open an issue or CAEP for discussion post-v1alpha3. Does that sound right to you?

sgtm, I think we definitely have to have a larger discussion and have a solid plan for any changes :)