🐛 (go/v4)(fix): (e2e) delete CertManager leftover leases in kube-system (not cleaned by default)

[camilamacedo86]

Closes: #4712

NOTE: The E2E tests are designed to run in isolated environments intended specifically for them,
such as the GitHub Actions workflows scaffolded by default.

c/c @wallrj @erikgb @mahmoudhossam

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: camilamacedo86

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [camilamacedo86]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[camilamacedo86]

Hi @mahmoudhossam

Here is the fix for the issue that you opened !!!
It would be nice to get your help to review.

[mahmoudhossam]

Hi @mahmoudhossam

Here is the fix for the issue that you opened !!! It would be nice to get your help to review.

Hi @camilamacedo86, thank you for this!

I haven't touched the relevant part of the tests so didn't have any feedback for you, sorry about that.

However, looking at the PR code I don't see any mention of the leases.coordination.k8s.io API group, can you point me to where this is addressed in the code?

Bear in mind that deleting the cert-manager namespace won't take care of that as the resources in #4712 are in the kube-system namespace.

[camilamacedo86]

/hold
I will need work more on this one

[camilamacedo86]

/hold cancel

[erikgb]

I don't understand why you're rewriting the uninstall. Shouldn't the change just be to add the cleanup of leases in kube-system namespace? Reversing the installation procedure with kubectl delete should work except from this.

[camilamacedo86]

I don't understand why you're rewriting the uninstall. Shouldn't the change just be to add the cleanup of leases in kube-system namespace? Reversing the installation procedure with kubectl delete should work except from this.

Hi @erikgb

My motivation was to cover all: https://cert-manager.io/docs/installation/kubectl/#uninstalling
To avoid any other possible scenario.

However, keep it as before, with only the specific caveat that it would be a less verbose, cleaner solution.
I think that is a good idea since it is either a bug fix only

[erikgb]

My motivation was to cover all: https://cert-manager.io/docs/installation/kubectl/#uninstalling To avoid any other possible scenario.

Your current approach will make this more fragile to future changes in cert-manager. So I would recommend to just do the fix for leases.

[camilamacedo86]

Hi @erikgb

Thank you for the amazing help
Done 🎉

[erikgb]

/lgtm

🚀