Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add support for finalizers in application resource #159

Merged
merged 1 commit into from
Jun 12, 2024

Conversation

amotolani
Copy link
Contributor

@amotolani amotolani commented Apr 4, 2024

Description of your changes

  • feat: add support for finalizers in application resource

Fixes #

I have:

  • Read and followed Crossplane's contribution process.
  • Run make reviewable test to ensure this PR is ready for review.

How has this code been tested

  • unit tests

@amotolani amotolani force-pushed the add-finalizers-option branch from dea40c8 to 18d180f Compare April 4, 2024 19:39
@MisterMX
Copy link
Collaborator

MisterMX commented Apr 8, 2024

The idea of finalizers in K8s is to have an operator (like, a Crossplane provider) suspend the deletion of a resource to perform some cleanup steps. Usually you would have a single finalizer added and removed by each operator.

@amotolani can you describe the use case why you would like to have a bunch of finalizers statically added here? It would forever block the deletion of the resource unless the user explicitly removes it before deleting the MR.

@amotolani
Copy link
Contributor Author

amotolani commented Apr 8, 2024

The idea of finalizers in K8s is to have an operator (like, a Crossplane provider) suspend the deletion of a resource to perform some cleanup steps. Usually you would have a single finalizer added and removed by each operator.

@amotolani can you describe the use case why you would like to have a bunch of finalizers statically added here? It would forever block the deletion of the resource unless the user explicitly removes it before deleting the MR.

@MisterMX
The Operator that uses these finalizers is the ArgoCD Operator. Adding this allows us to create the ArgoCD App with these finalizers to control behaviour of the ArgoCD App.

A specific use case for this is controlling the deletion behaviour for ArgoCD apps. Without deletion finalizers the default behaviour is to delete the ArgoCD App without cleaning up the resources created by the ArgoCD app in a cluster, adding finalizers will enable cascade deletion for ArgoCD Apps.

Reference Docs

@MisterMX
Copy link
Collaborator

MisterMX commented Apr 9, 2024

Does this work with the delete flow of this controller? If a users adds arbitrary finalizers to an MR application and then deletes it with kubectl delete. Does ArgoCD cleanup all finalizers and finishes the deletion so the MR gets removed? Or is the controller stuck in a never ending delete reconcile if it is observing an ArgoCD app that is not going to be deleted due to a finalizer that is never removed?

@amotolani
Copy link
Contributor Author

amotolani commented May 3, 2024

Does this work with the delete flow of this controller? If a users adds arbitrary finalizers to an MR application and then deletes it with kubectl delete. Does ArgoCD cleanup all finalizers and finishes the deletion so the MR gets removed? Or is the controller stuck in a never ending delete reconcile if it is observing an ArgoCD app that is not going to be deleted due to a finalizer that is never removed?

@MisterMX Sorry it took me a while to get back here.

ArgoCD will only cleanup valid finalizers. So if a user adds an arbitrary finalizer that is not valid it would block deletion of the ArgoCD Application and by extension the MR.

IMO this is an ArgoCD concern and not really one for the provider. Taking aside crossplane and provider-argocd, the same thing occurs when you create an ArgoCD application declaratively using the ArgoCD CRD application.argoproj.io

AFAIK Finalizers are the only way to control this deletion behaviour declaratively, and it comes with this issue you highlighted

Copy link
Collaborator

@MisterMX MisterMX left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the clarification. I have one other remark.

pkg/controller/applications/comp.go Outdated Show resolved Hide resolved
@amotolani amotolani force-pushed the add-finalizers-option branch from 3e133ad to 930b982 Compare June 6, 2024 14:10
@amotolani amotolani requested a review from MisterMX June 6, 2024 14:29
@amotolani
Copy link
Contributor Author

@MisterMX I made the suggested improvements

@MisterMX MisterMX force-pushed the add-finalizers-option branch 2 times, most recently from 4628b91 to 412961f Compare June 12, 2024 15:15
@MisterMX MisterMX force-pushed the add-finalizers-option branch from 412961f to 3f80d07 Compare June 12, 2024 15:18
Copy link
Collaborator

@MisterMX MisterMX left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thank you very much @amotolani!

@MisterMX MisterMX merged commit 72a5615 into crossplane-contrib:main Jun 12, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants