terraform-provider-kubernetes icon indicating copy to clipboard operation
terraform-provider-kubernetes copied to clipboard

Allow setting the deletion strategy for kubernetes_manifest

Open benbertrands opened this issue 1 year ago • 6 comments

Description

The kubernetes api allows setting DeleteOptions. Extra docs: https://kubernetes.io/docs/concepts/architecture/garbage-collection

The default deletion strategy is background cascading deletion. This strategy removes the resource and then returns, while removal of any "child" resources happens out-of-band

For some resources a foreground cascading deletion is more appropriate. This strategy removes the "child" resources before completing the removal of the resource-for-which-deletion-was-requested and only then returns. An example of this is Job. Job was switched to foreground cascading deletion here

The kubernetes_manifest resource would benefit from a property that allows configuring the deletion strategy. I have a case where we need a foreground cascading deletion.

Concrete case

We deployed karpenter using terraform Next we create a set of karpenter nodepools using kubernetes_manifest. These create a bunch of "child" resources (nodeclaims which map to the underlying hosts), they refer to their respective nodepools as owner.

When using terraform to tear things back down:

Current situation

The nodepools are removed using background cascading deletion -> karpenter is "undeployed". The nodeclaims (which have a finalizer) are marked for deletion (out-of-band) but, as the karpenter pods are already gone, are indefinitely stuck waiting for karpenter to remove their finalizer.

To-be situation

The nodepools are removed using foreground cascading deletion -> The nodeclaims (which have a finalizer) are marked for deletion -> karpenter cleans up the underlying resources and removes the finalizer -> nodeclaim resources removed -> karpenter is "undeployed"

Potential Terraform Configuration

resource "kubernetes_manifest" "test-crd" {
  manifest = {
    apiVersion = "karpenter.sh/v1"
    kind       = "NodePool"

    metadata = {
      name = "spot"
    }

    spec = {
      ...
    }
  }
 
 delete_propagation = "Foreground"

}

❗ Feedback on this is most welcome

References

https://github.com/hashicorp/terraform-provider-kubernetes/pull/635

Community Note

  • Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request
  • If you are interested in working on this issue or have submitted a pull request, please leave a comment

benbertrands avatar Nov 27 '24 15:11 benbertrands

I'll submit a pr for this if the comunity agrees with the need for the feature and on the api

benbertrands avatar Nov 27 '24 16:11 benbertrands

I'll submit a pr for this if the comunity agrees with the need for the feature and on the api

@benbertrands Let's discuss before submitting the PR specifically for manifest. There might be an opportunity here to expand this feature beyond manifest as we move towards wider use of server-side-apply across the provider. Just want to make sure we get the UX future-proofed for that.

alexsomesan avatar Dec 03 '24 15:12 alexsomesan

Thanks for raising this. It's a very good idea and we'll definitely look into adding this.

alexsomesan avatar Dec 03 '24 15:12 alexsomesan

Let me know if I can help

benbertrands avatar Dec 03 '24 15:12 benbertrands

Hi @alexsomesan,

Has this made it on the roadmap? Let me know if I can help out

benbertrands avatar May 21 '25 13:05 benbertrands

Hi @alexsomesan,

I may have misinterpreted this

@benbertrands Let's discuss before submitting the PR specifically for manifest. There might be an opportunity here to expand this feature beyond manifest as we move towards wider use of server-side-apply across the provider. Just want to make sure we get the UX future-proofed for that.

I took this as "We (maintainers) will discuss this internally first to make sure the UX will be future-proof" Was this a "Let's talk UX" instead?

benbertrands avatar Jun 18 '25 11:06 benbertrands