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

Fix helm uninstall cleanup by adding finalizers and cleaning them from the controller #2433

Merged
merged 35 commits into from
Apr 3, 2023

Conversation

nikola-jokic
Copy link
Member

@nikola-jokic nikola-jokic commented Mar 22, 2023

To lower the amount of permissions required for the manager to operate, manager role binding is introduced when the autoscaling runner set is applied.

But when doing helm uninstall, role and role bindings are deleted usually before the AutoscalingRunnerSet can clean up resources, causing errors.

This change adds finalizers to resources applied by helm, and extends the manager cluster role, so it can clean up Role in the last step of clean-up.

We added finalizers to the following resources in gha-runner-scale-set in AutoscalingRunnerSet namespace:

  • Manager Role
  • Manager RoleBinding
  • Kubernetes mode Role
  • Kubernetes mode RoleBinding
  • Kubernetes mode ServiceAccount
  • No permission ServiceAccount

Finalizer name is changed for the GitHub secret in gha-runner-scale-set from actions.github.com/secret-protection to actions.github.com/cleanup-protection

The order in which resources are being patched to remove finalizers:

  1. Kubernetes mode RoleBinding
  2. Kubernetes mode Role
  3. Kubernetes mode ServiceAccount
  4. No permission ServiceAccount
  5. GitHub secret
  6. Manager RoleBinding
  7. Manager Role

Note: only resources that are specified in AutoscalingRunnerSet.ObjectMeta.Annotations are going to be patched.

@nikola-jokic nikola-jokic force-pushed the nikola-jokic/finalize-helm-uninstall branch from 96215fe to 254bb1c Compare March 22, 2023 14:02
@nikola-jokic nikola-jokic force-pushed the nikola-jokic/finalize-helm-uninstall branch 2 times, most recently from 010becc to a2f7335 Compare March 28, 2023 12:53
nikola-jokic and others added 2 commits April 3, 2023 17:18
@nikola-jokic nikola-jokic merged commit 5dea6db into master Apr 3, 2023
@nikola-jokic nikola-jokic deleted the nikola-jokic/finalize-helm-uninstall branch April 3, 2023 19:06
@Link- Link- mentioned this pull request Apr 4, 2023
@nikola-jokic nikola-jokic restored the nikola-jokic/finalize-helm-uninstall branch September 26, 2023 09:51
@nikola-jokic nikola-jokic deleted the nikola-jokic/finalize-helm-uninstall branch September 26, 2023 09:55
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.

None yet

2 participants