Add type logging to certificate manager #101252
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
k8s-ci-robot
added
release-note
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: smarterclayton 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 |
k8s-ci-robot
added
sig/api-machinery
ehashman
reviewed
Apr 19, 2021
Comment on lines
+262
to
+264
| if m.logf == nil { | ||
| m.logf = func(format string, args ...interface{}) { klog.V(2).Infof(format, args...) } | ||
| } |
There was a problem hiding this comment.
As a small non-blocking nit I'd suggest not doing this because it'll likely need to get ripped out when we migrate this to structured logs.
There was a problem hiding this comment.
We can't just slam structured logs into client-go - it's a library, not a full featured function. Or are you saying that everyone who uses client-go is required to use klog (which is something historically we've said we wouldn't do)?
There was a problem hiding this comment.
Oh, right, this is client-go and you're correct that we're not doing that; morning brain fog :)
There was a problem hiding this comment.
it’s ok, i wasn’t positive it wasn’t me that missed something :)
k8s-ci-robot
added
priority/important-soon
|
/retest |
|
will sort out the unit panic |
Kubelet cert rotation involves two certificate manager instances (one for client and one for server certs) and the log lines are identical and confusing. Since certificate manager is a utility library it is also inappropriate to simply assume klog output is sufficient. certificate.Manager now accepts a Name and Logf function on its config struct to identify the purpose of the manager and to provide a way to redirect where output should go. If Name is absent, the name is defaulted from the SignerName, and if that is not found then the name is set to "client auth" if that is a provided key usage, or "certificate" otherwise. If Logf is not provided it defaults to klog.V(2). as today. The name is printed in "foo: bar" form on every line, but can be converted to structured logging in the future. The log level is not customizable and it is up to the caller to decide whether that is an issue. Some log messages are slightly cleaned up to more clearly indicate their intent. One log message is removed in a utility function that was already at v(4) and less likely to be needed. The default behavior of the certificate manager is as before and the kubelet now identifies the server and client signerName as separate entities: I0414 19:07:33.590419 1539 certificate_manager.go:263] kubernetes.io/kube-apiserver-client-kubelet: Rotating certificates E0414 19:07:33.594154 1539 certificate_manager.go:464] kubernetes.io/kube-apiserver-client-kubelet: Failed while requesting a signed certificate from the master: cannot create certificate signing request: Post "https://...
smarterclayton
force-pushed
the
certificate_logging_upstream
branch
from
bfea9b0
to
64c669b
Compare
|
/assign @deads2k |
k8s-ci-robot
added
triage/accepted
|
panic was sorted, more reviews please |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Kubelet cert rotation involves two certificate manager instances (one for client and one for server certs) and the log lines are
identical and confusing. Since certificate manager is a utility library it is also inappropriate to simply assume klog output is
sufficient.
certificate.Manager now accepts a Name and Logf function on its config struct to identify the purpose of the manager and to
provide a way to redirect where output should go. If Name is absent, the name is defaulted from the SignerName, and if that
is not found then the name is set to "client auth" if that is a provided key usage, or "certificate" otherwise. If Logf is
not provided it defaults to klog.V(2). as today. The name is printed in "foo: bar" form on every line, but can be converted to structured logging in the future. The log level is not customizable and it is up to the caller to decide whether that is an issue.
Some log messages are slightly cleaned up to more clearly indicate their intent. One log message is removed in a utility function that was already at v(4) and less likely to be needed.
The default behavior of the certificate manager is as before and the kubelet now identifies the server and client signerName as
separate entities:
/kind bug
/kind cleanup