-
Notifications
You must be signed in to change notification settings - Fork 1.4k
✨ ClusterCache: Deprecate GetClientCertificatePrivateKey and stop using it in KCP #12846
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
✨ ClusterCache: Deprecate GetClientCertificatePrivateKey and stop using it in KCP #12846
Conversation
|
/assign @fabriziopandini /cc @Karthik-K-N (just fyi) |
|
Makes sense. Looks like everything is covered as far as I can see. Doesn't look like the golangci-lint failure is related to this change. /lgtm |
|
LGTM label has been added. Git tree hash: 2240c6c18f4147bc4e11a860ab475c680250fab4
|
007f4d7 to
b7bcb46
Compare
… it in KCP Signed-off-by: Stefan Büringer [email protected]
b7bcb46 to
e9f9fc3
Compare
|
Sorry had to push a small fix. The cache variable was shadowed in the else branch (https://github.com/kubernetes-sigs/cluster-api/compare/007f4d7ca9a46ba34e55d081b143ba3c3d586240..b7bcb461aaa5e62ce25f4c2723ff38b61228a018) |
|
/test pull-cluster-api-e2e-main |
| if err != nil { | ||
| return nil, err | ||
| // Get client cert from cache if possible, otherwise generate it and add it to the cache. | ||
| // TODO: When we implement ClusterConfiguration.EncryptionAlgorithm we should add it to |
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.
Will be done as part of #10077 (comment)
fabriziopandini
left a 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
/approve
Might be we should add a note to the migration doc about the deprecated method, but no strong opinion (I do not expect someone using this, it is really KCP specific)
|
LGTM label has been added. Git tree hash: 20a7caa5ab7e6ddab3ac21629cc2deff8f267246
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fabriziopandini 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 |
Signed-off-by: Stefan Büringer [email protected]
What this PR does / why we need it:
The client certificate doesn't really make sense in ClusterCache as it has nothing to do with what ClusterCache is otherwise doing. Accordingly this PR deprecates GetClientCertificatePrivateKey.
Additionally KCP is now caching the client cert in a local cache instead of using GetClientCertificatePrivateKey.
Going forward this will also make it easy to use the same EncryptionAlgorithm for the etcd client cert than what we use in other parts of KCP (for CAs, kubeconfig, ...)
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Also a pre-refactoring to simplify #10077