Extend `DependsOn` to add `ownerReferences`

[ytsarev]

What problem are you facing?

Currently DependsOn is setting only finalizers on the dependant object and it is not enough to handle the cases like ordered deletion.

How could Crossplane help solve your problem?

Set ownerReferences on the parent object.

It will enable controlled ordered deletion with kubectl delete --cascade=foreground by deleting the child objects first.

Related links:

• https://kubernetes.io/blog/2021/05/14/using-finalizers-to-control-deletion/#owner-references
• https://github.com/crossplane/crossplane/discussions/3116

[bobh66]

Hi @ytsarev - I have a PR with these changes and it looks like they work - I assume we want blockOwnerDeletion set to false? Is there a scenario where it would need to be set to true? I'm thinking about leaving it false for now and if a scenario comes up where it needs to be true we can update the API at that point and let the user set the flag. Thoughts? Thanks!

[bobh66]

Also note that https://github.com/crossplane/crossplane/pull/3213 will need to be merged before this will work on Composite and Managed resources. At the moment the new ownerReference is removed from the target resource every time it is processed by the internal/controller/apiextensions/composite/composed.go Render() function.

[ytsarev]

@bobh66 As in you mentioned in #3116

Use a combination of cascading forward deletion and the blockOwnerDeletion flag to cause a "bottom up" deletion path so that child resources are deleted before their owners. 

I was working with the case provider-kubernetes Object of Azure Posrtgresql-server was owning the Database resource created by provider-sql. In this case, we had to clean up the child Database before the parent PGServer disappears.

[bobh66]

In this scenario you want the database server to be the "owner" of the database, and to ensure that the database gets deleted before the database server.

If the Object is the Database Server and you configure a "dependsOn" reference to the Database resource, that would place a finalizer on the Database resource so that it cannot be deleted (by Kubernetes Garbage Collection) before the Object (Database Server) which I don't think is what you want.

If you create an Object to "Observe" the Database resource, and specify a dependsOn reference to the Database Server (which I think makes more logical sense), that will place a finalizer on the "Database Server" Object to ensure that it cannot be deleted before the Database.

So now there is a finalizer on the "Database Server" Object, and with #53 there will also be an ownerReference on the Database Server Object pointing at the Database "Observe" object, which I don't think makes any sense. The ownerReference should be on the Database "Observe" object (or on the actual Database object itself - or both) to indicate that it is owned by the Database Server.

Assuming that there is an ownerReference on the Database object, if blockOwnerDeletion is "true" and cascading forward deletion is triggered, kubernetes will place a blockOwnerDeletion finalizer on the Database Server as the "owner" of the Database object, so that it cannot be deleted before the Database object is deleted. That might make the provider-kubernetes custom finalizer unnecessary in this specific scenario, but I don't think it hurts anything.

I don't think the solution in #53 makes sense - having the ownerReference on the thing that we are depending on is not helpful.

I'll rework the solution to put the ownerReference on the either the Object itself or on the resource that the Object is Creating/Observing/etc or both. An ownerReference on the local Object doesn't help when the resource is remote, so I think it needs to be on the remote resource as well.

[turkenh]

I didn't have a chance to read this through yet, but since you are discussing some solutions with ownerReferences I would like to remind that any solution we implement should work with external Kubernetes clusters as well and ownerReferences wouldn't work across clusters.

Running provider-kubernetes to provision in cluster workloads is more a workaround (mostly for XP limitations) than the primary purpose of the provider which is managing external objects just like any other crossplane provider.

[bobh66]

@turkenh - agreed, it only makes sense to put the ownerReference on the remote cluster

[bobh66]

@turkenh - if I'm reading this right it appears that the provider only supports references on the local cluster, when resolving patches:

https://github.com/crossplane-contrib/provider-kubernetes/blob/main/internal/controller/object/object.go#L393

and when creating a kubeclient for finalizer handling:

https://github.com/crossplane-contrib/provider-kubernetes/blob/main/internal/controller/object/object.go#L97

So if we're going to support external cluster ownerReferences that implies a larger change in functionality. Presumably we would need to add a flag to indicate whether the reference is "Local" or "Remote" and use the appropriate client.

Maybe a short-term change is to support local cluster ownerReferences only and expand to remote cluster support in a different PR?

[ytsarev]

@bobh66 just to explicitly mention it here, the presence of a finalizer on the MR object will not block the actual deletion in the target cloud environment. Crossplane will reconcile deletion as soon as it gets deletionTimestamps set on the Object, so finalizers are blocking only the deletion of MR in k8s API representation, not in a target cloud( that is what we are trying to achieve) . So the need for ownerReferences + blockOwnerDeletion seems unavoidable.

[bobh66]

@ytsarev That's correct - there are a set of changes to the way Crossplane handles finalizers that are required for this functionality to work as described/desired. I'm working on a proposal that describes those changes, hoping to have it ready soon.

[bobh66]

@ytsarev - A first step toward supporting this in Crossplane is described here - https://github.com/crossplane/crossplane/issues/3225

[bobh66]

I think this should put an ownerReference on the Object pointing at the dependency that has the finalizer. That will cause kubernetes to put a foregroundDeletion finalizer on the dependency so it cannot be deleted before the Object. That only applies to the foreground deletion scenario so we still want the existing finalizer on the target so that it cannot be deleted before the dependent Object in either the foreground or background deletion scenario. The owner reference should have controller set to false and blockOwnerDeletion set to true. I think this is consistent with the fact that dependencies will only work on the local cluster as noted above.

[negz]

My two cents - I'd definitely like to understand what path we're taking in Crossplane (if any) before extending this provider further. I could imagine a path where we don't need a "special" implementation in provider-kubernetes to handle dependencies anymore.