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: add finalizer to credentials reference #288

Merged
merged 3 commits into from
May 2, 2024
Merged

Conversation

cbzzz
Copy link
Contributor

@cbzzz cbzzz commented Apr 26, 2024

What type of PR is this?
/kind cleanup

What this PR does / why we need it:
Adds a unique finalizer to the credentials reference Secret (if .spec.CredentialsRef is set) when creating Linode{Cluster,Machine,VPC} resources and removes it upon deletion. The finalizer is unique in case you have a shared Secret (e.g. KIND.GROUP/NAMESPACE.NAME).

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:
This allows users to use kubectl delete with the output of clusterctl generate, e.g.:

clusterctl generate cluster $CLUSTER_NAME \
  --kubernetes-version v1.29.1 \
  --infrastructure akamai-linode:v0.0.0 \
  --flavor clusterclass-kubeadm \
  | kubectl delete -f -

TODOs:

  • squashed commits
  • includes documentation
  • adds unit tests
  • adds or updates e2e tests

Testing:

  1. Create a cluster:
$ clusterctl generate cluster $CLUSTER_NAME \
  --kubernetes-version v1.29.1 \
  --infrastructure linode:0.0.0 \
  | kubectl apply -f -
  1. Check the cluster credentials Secret for an object finalizer:
$ kubectl get secret ${CLUSTER_NAME}-credentials -o yaml | yq .metadata.finalizers
- linodecluster.infrastructure.cluster.x-k8s.io/default.${CLUSTER_NAME}
  1. Delete the cluster:
$ clusterctl generate cluster $CLUSTER_NAME \
  --kubernetes-version v1.29.1 \
  --infrastructure linode:0.0.0 \
  | kubectl delete -f -
  1. All cluster resources should clean up successfully:
$ kubectl get cluster-api,secrets
No resources found in default namespace.

Copy link

codecov bot commented Apr 26, 2024

Codecov Report

Attention: Patch coverage is 75.00000% with 24 lines in your changes are missing coverage. Please review.

Project coverage is 64.87%. Comparing base (2f81203) to head (286ac18).

Files Patch % Lines
controller/linodecluster_controller.go 0.00% 6 Missing and 3 partials ⚠️
controller/linodemachine_controller.go 0.00% 8 Missing and 1 partial ⚠️
controller/linodevpc_controller.go 0.00% 4 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #288      +/-   ##
==========================================
+ Coverage   64.40%   64.87%   +0.47%     
==========================================
  Files          30       30              
  Lines        1750     1842      +92     
==========================================
+ Hits         1127     1195      +68     
- Misses        574      592      +18     
- Partials       49       55       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@eljohnson92
Copy link
Collaborator

This change looks good to me overall, can we just add some unit tests for the new functionality where unit test frameworks are already in place?

@AshleyDumaine
Copy link
Member

AshleyDumaine commented Apr 26, 2024

This might be out of scope for this PR, but we might want to prefix some extra resources with the cluster name like the common-init-files Secret and the helmchartproxies to support the k delete -f workflow in the case there are multiple workload clusters in one namespace:

clusterctl generate cluster $CLUSTER_NAME \
  --kubernetes-version v1.29.1 \
  --infrastructure linode:v0.0.0 \
  | kubectl delete -f -
secret "cleanup-credentials" deleted
secret "common-init-files" deleted
secret "linode-cleanup-crs-0" deleted
helmchartproxy.addons.cluster.x-k8s.io "cilium" deleted
helmchartproxy.addons.cluster.x-k8s.io "cilium-ipv6" deleted
helmchartproxy.addons.cluster.x-k8s.io "csi-driver-linode" deleted
helmchartproxy.addons.cluster.x-k8s.io "linode-cloud-controller-manager" deleted
clusterresourceset.addons.cluster.x-k8s.io "cleanup-crs-0" deleted
kubeadmconfigtemplate.bootstrap.cluster.x-k8s.io "cleanup-md-0" deleted
cluster.cluster.x-k8s.io "cleanup" deleted
machinedeployment.cluster.x-k8s.io "cleanup-md-0" deleted
kubeadmcontrolplane.controlplane.cluster.x-k8s.io "cleanup-control-plane" deleted
linodecluster.infrastructure.cluster.x-k8s.io "cleanup" deleted
linodemachinetemplate.infrastructure.cluster.x-k8s.io "cleanup-control-plane" deleted
linodemachinetemplate.infrastructure.cluster.x-k8s.io "cleanup-md-0" deleted
linodevpc.infrastructure.cluster.x-k8s.io "cleanup" deleted

@eljohnson92
Copy link
Collaborator

agreed we should scope addons to the cluster they are associated with, I think that is out of scope for this PR but should be addressed in #289 for CCM/Cilium and we can create a separate follow up for the remaining addons

@cbzzz cbzzz force-pushed the fix.credentials.finalizer branch 3 times, most recently from cfb60ca to 90c53fe Compare May 2, 2024 14:46
eljohnson92
eljohnson92 previously approved these changes May 2, 2024
@cbzzz cbzzz merged commit 3ac4ac0 into main May 2, 2024
9 checks passed
@cbzzz cbzzz deleted the fix.credentials.finalizer branch May 2, 2024 16:14
amold1 pushed a commit that referenced this pull request May 17, 2024
* fix: linodecluster: add finalizer to credentials reference

* fix: linodemachine: add finalizer to credentials reference

* fix: linodevpc: add finalizer to credentials reference
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.

3 participants