-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
🐛 delete kubeconfig secret as part of cluster deletion #1431
Conversation
Signed-off-by: Andrew Sy Kim <kiman@vmware.com>
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: andrewsykim The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
@@ -243,6 +244,14 @@ func (r *ClusterReconciler) listChildren(ctx context.Context, cluster *clusterv1 | |||
} | |||
controlPlaneMachines, machines := splitMachineList(allMachines) | |||
|
|||
secrets := &corev1.SecretList{} | |||
secretsListOptions := []client.ListOption{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Secret list has it's own list options because we don't apply the cluster.x-k8s.io/cluster-name
label to secret resources (maybe we should?).
secretsListOptions := []client.ListOption{ | ||
client.InNamespace(cluster.Namespace), | ||
} | ||
if err := r.Client.List(ctx, secrets, secretsListOptions...); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't delete all secrets in the namespace!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reconcileDelete
does a check for OwnerRef before deleting though?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, right (but it's in eachFunc
)
The kubeconfig secret has an OwnerReference to the cluster. It is removed when the cluster is deleted. |
This should have been fixed with #1403 |
We explicitly deal with MachineDeployments, MachineSets, and Machines because we have to delete them in the right order, and kubectl by default does background deletion. We need foreground (we have to delete everything before we delete the cluster), and there's no way for a user to do that with kubectl today, and it's bad UX if we make the user responsible for deleting it "the right way" |
@andrewsykim: Closed this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Signed-off-by: Andrew Sy Kim kiman@vmware.com
What this PR does / why we need it:
Deletes kubeconfig secret as part of cluster reconcile on delete. Wasn't sure if we intentionally left out kubeconfig, but the patch was small enough so I didn't mind opening the PR anyways.
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 #