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

Allow control of finalization garbage collection with .spec.deletionPolicy #1314

Merged
merged 1 commit into from
Dec 19, 2024

Conversation

erikgb
Copy link
Contributor

@erikgb erikgb commented Dec 15, 2024

The prune setting of Kustomizations has a two-fold behavior. When set to true FLux will:

  1. Ensure stale managed resources are deleted.
  2. Garbage collect managed resources when Kustomization is deleted (using a finalizer).

We would like to have 1, but not 2, and this is currently not possible. This PR introduces a new API field allowing control over finalization garbage collection independently of prune.

Some background/motivation for this proposed enhancement:

We are using Flux to provision review environments in a multi-tenant environment (shared cluster), and our tenants are not allowed to create Kubernetes namespaces directly. Instead they have to use a custom namespace-scoped resource as a controller for the namespace (and RBAC). This indirect management of namespaces makes it hard to create Kustomizations inside review namespaces. To overcome this, we use the targetNamespace field. This works well when the review namespace is created. But when the review namespace is teared down by our review environment automation, there is a finalization race condition between Flux and our namespace controller. If Flux looses this race, the Kustomization is stuck in non-finalized state (because the namespace is gone) and requires manual intervention from a cluster-admin. This is something we definitely would like to avoid! Since the namespace is controlled and eventually deleted by our namespace controller, no resources are in fact orphaned if we disable FLux garbage collection.

Orphaning resources is generally a bad idea, but is sometimes required. 😉
Once we agree on the naming, I will submit a PR to update the docs in the website repo.

Close #662

Copy link
Member

@stefanprodan stefanprodan left a comment

Choose a reason for hiding this comment

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

api/v1/kustomization_types.go Outdated Show resolved Hide resolved
Copy link
Member

@matheuscscp matheuscscp 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 this contribution! I think it's a really valuable one.

api/v1/kustomization_types.go Outdated Show resolved Hide resolved
docs/spec/v1/kustomizations.md Outdated Show resolved Hide resolved
docs/spec/v1/kustomizations.md Outdated Show resolved Hide resolved
docs/spec/v1/kustomizations.md Outdated Show resolved Hide resolved
@erikgb erikgb requested a review from stefanprodan December 16, 2024 20:52
@stefanprodan stefanprodan added enhancement New feature or request area/api API related issues and pull requests labels Dec 16, 2024
@erikgb
Copy link
Contributor Author

erikgb commented Dec 16, 2024

Ok, it seems like we are approaching something here. 😸 I have just rebased and squashed all commits. The PR should be ready to merge if you are satisfied with the proposed changes. Thanks for all the reviews! 🙏

Copy link
Contributor

@tenstad tenstad left a comment

Choose a reason for hiding this comment

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

Looking good! Just some minor nitpicks/thoughts regarding text.

docs/spec/v1/kustomizations.md Outdated Show resolved Hide resolved
docs/spec/v1/kustomizations.md Outdated Show resolved Hide resolved
docs/spec/v1/kustomizations.md Outdated Show resolved Hide resolved
docs/spec/v1/kustomizations.md Outdated Show resolved Hide resolved
api/v1/kustomization_types.go Outdated Show resolved Hide resolved
Copy link
Member

@matheuscscp matheuscscp left a comment

Choose a reason for hiding this comment

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

Great stuff, I'm ready to give my approval after all the open discussions are resolved!

docs/spec/v1/kustomizations.md Outdated Show resolved Hide resolved
docs/spec/v1/kustomizations.md Show resolved Hide resolved
internal/controller/kustomization_deletion_policy_test.go Outdated Show resolved Hide resolved
internal/controller/kustomization_deletion_policy_test.go Outdated Show resolved Hide resolved
internal/controller/kustomization_deletion_policy_test.go Outdated Show resolved Hide resolved
internal/controller/kustomization_deletion_policy_test.go Outdated Show resolved Hide resolved
internal/controller/kustomization_deletion_policy_test.go Outdated Show resolved Hide resolved
Signed-off-by: Erik Godding Boye <[email protected]>
Co-authored-by: Stefan Prodan <[email protected]>
Co-authored-by: Amund Tenstad <[email protected]>
@erikgb
Copy link
Contributor Author

erikgb commented Dec 18, 2024

@matheuscscp All discussions should now be resolved. Can you please approve CI to see if I made any mistakes in the test refactoring?

Copy link
Member

@matheuscscp matheuscscp left a comment

Choose a reason for hiding this comment

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

Great contribution! Thanks @erikgb

@stefanprodan stefanprodan changed the title Allow control of finalization garbage collection Allow control of finalization garbage collection with .spec.deletionPolicy Dec 19, 2024
Copy link
Member

@stefanprodan stefanprodan left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks @erikgb 🏅

@stefanprodan stefanprodan merged commit c41cb82 into fluxcd:main Dec 19, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/api API related issues and pull requests enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow disabling a Kustomization's finalizer
4 participants