feat: Support for DeleteNamespace SyncOption(#4435, #7875)

[toyamagu-2021]

Closes #4435
Closes #7875

Description

• After this PR is merged, we can specify DeleteNamespace syncOption, which enables a feature to delete an auto-created namespace.
• We can delete a namespace only when:
  • CreateNamespace=true and DeleteNamespace=true
  • The target namespace is tracked by an application.
    • e.g.) The namespace must have a label, app.kubernetes.io/instance=${applicationName}

Test

We can test this feature by the following manifest.
After PR #11350 merged, I think app.kubernetes.io/instance is automatically labeled.

apiVersion: argoproj.io/v1alpha1
kind: Application
metadata:
  name: guestbook
  namespace: argocd
spec:
  project: default
  destination:
    server: https://kubernetes.default.svc/
    namespace: test
  source:
    repoURL: https://github.com/argoproj/argocd-example-apps.git
    path: guestbook
    targetRevision: HEAD
  syncPolicy:
    syncOptions:
    - CreateNamespace=true
    - DeleteNamespace=true
    managedNamespaceMetadata:
      labels:
        app.kubernetes.io/instance: guestbook

Notes

Because deleting a namespace destroys all resources in the namespace, I think this feature is not breaking but dangerous.
So any suggestions to increase safety would be greatly appreciated.

Template

Signed-off-by: toyamagu2021@gmail.com toyamagu2021@gmail.com

Note on DCO:

If the DCO action in the integration test fails, one or more of your commits are not signed off. Please click on the Details link next to the DCO action for instructions on how to resolve this.

Checklist:

• Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
• The title of the PR states what changed and the related issues number (used for the release note).
• I've included "Closes [ISSUE #]" or "Fixes [ISSUE #]" in the description to automatically close the associated issue.
• I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
• Does this PR require documentation updates?
• I've updated documentation as required by this PR.
• Optional. My organization is added to USERS.md.
• I have signed off all my commits as required by DCO
• I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
• My build is green (troubleshooting builds).

[codecov]

Codecov Report

Base: 47.32% // Head: 47.34% // Increases project coverage by +0.01% 🎉

Coverage data is based on head (84e9afe) compared to base (566d50f).
Patch coverage: 66.66% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #11821      +/-   ##
==========================================
+ Coverage   47.32%   47.34%   +0.01%     
==========================================
  Files         245      245              
  Lines       41545    41584      +39     
==========================================
+ Hits        19661    19687      +26     
- Misses      19903    19912       +9     
- Partials     1981     1985       +4     

Impacted Files	Coverage Δ
util/argo/resource_tracking.go	69.56% <ø> (ø)
server/application/application.go	28.92% <66.66%> (+0.79%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[blakepettersson]

How will the app ownership of the namespaces be set? Unless/until something like #11350 is merged, this would need to be a manual process?

[blakepettersson]

In general any k8s actions happens in gitops-engine, and would probably be the place we'd want to add this.

I suspect that we could do this in a simpler manner though, by force-pruning managed namespaces when an application is deleted (this would likely need to be added in controller/state.go)

[blakepettersson]

Thanks for the PR! There are a few outstanding questions which needs to be addressed before definitely saying yay or nay though, the main one being the ownership concept itself - which is the main reason why #11350 has not yet been merged and is currently targeted for 2.7.

Once that has been done, we can revisit this and I suspect this will then need to go through a few further rounds of discussion (cc @leoluz, @crenshaw-dev)

[toyamagu-2021]

@blakepettersson
Thank you for your comments.
Yes, the concept itself is under discussion, so I will wait for that.
But comments might be addressed partially, and I will try to consider that.

[sqaisar]

Is this feature going to be merged/released? or this is closed?

[toyamagu-2021]

Sorry for left this. I'm afraid this PR should be closed and think more carefully as in #.7875.

#.13999, #.15670, and successors might implement this feature.