-
Notifications
You must be signed in to change notification settings - Fork 305
Dispose certificates in Kubernetes.Dispose()
#1191
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
Conversation
|
Welcome @shenglol! |
| } | ||
|
|
||
| // Set Credentials | ||
| if (this.ClientCert != null) |
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.
Why are you deleting this?
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.
This is dead code. this.ClientCert is never assigned and is always null before the change. I did a git blame and it seems it's a leftover from a previous refactoring. The logic is covered by the code blow.
|
Generally looks good, one comment. |
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.
/LGTM
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: shenglol, tg123 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:
Approvers can indicate their approval by writing |
The
Kubernetesconstructor creates client and SSL CA certs vianew X509Certificate2(...)if the kubeconfig contains cert path/data. As a result, on Windows, some one-time use files may be created for eachnew Kuberentes(...)call (see The most dangerous constructor in .NET).According to the SO answer, the files will be removed by GC and calling
Disposeis not required. However, since GC is not guaranteed to invoke all finalizers, we cannot rely on it to do the cleanup. I ran a local test and found that the files are not always cleaned up automatically. Thus, callingX509Certificate2.Dispose()is a must. This is also mentioned in the MS doc.The PR updates
Kubernetes.Dispose()to dispose certificates.Closes #1189.