Migrate pkg/kubelet/certificate to contextual logging #132193
Conversation
- Add context parameters to certificate transport functions - Replace klog.ErrorS/InfoS with logger.Error/Info calls Fixes 8 contextual logging calls in kubelet certificate package.
|
Welcome @RashRAJ! |
|
Hi @RashRAJ. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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-sigs/prow repository. |
hack/tools/go.mod
Outdated
| @@ -49,6 +49,7 @@ require ( | |||
| github.com/spf13/viper v1.20.0 // indirect | |||
| github.com/subosito/gotenv v1.6.0 // indirect | |||
| go.uber.org/multierr v1.11.0 // indirect | |||
| golang.org/x/exp v0.0.0-20231110203233-9a3e6036ecaa // indirect | |||
There was a problem hiding this comment.
where this change comes from?
pkg/kubelet/certificate/kubelet.go
Outdated
| @@ -278,18 +278,20 @@ type kubeletServerCertificateDynamicFileManager struct { | |||
| keyFile string | |||
| dynamicCertificateContent *dynamiccertificates.DynamicCertKeyPairContent | |||
| currentTLSCertificate atomic.Pointer[tls.Certificate] | |||
| ctx context.Context | |||
There was a problem hiding this comment.
Storing context in a structure instead of passing as a parameter to functions considered anti-pattern in go, please avoid.
There was a problem hiding this comment.
completely agree, specially where there is already a cancelFn context.CancelFunc in the same structure
pkg/kubelet/certificate/transport.go
Outdated
| @@ -55,21 +56,21 @@ import ( | |||
| // | |||
| // stopCh should be used to indicate when the transport is unused and doesn't need | |||
| // to continue checking the manager. | |||
| func UpdateTransport(stopCh <-chan struct{}, clientConfig *restclient.Config, clientCertificateManager certificate.Manager, exitAfter time.Duration) (func(), error) { | |||
| return updateTransport(stopCh, 10*time.Second, clientConfig, clientCertificateManager, exitAfter) | |||
| func UpdateTransport(ctx context.Context, stopCh <-chan struct{}, clientConfig *restclient.Config, clientCertificateManager certificate.Manager, exitAfter time.Duration) (func(), error) { | |||
There was a problem hiding this comment.
the stopCh <-chan struct{} should be derived from the context
- Add context parameters to certificate transport functions - Replace klog.ErrorS/InfoS with logger.Error/Info calls -clean up - Update dependencies after rebase and conflict resolution Fixes 8 contextual logging calls in kubelet certificate package.
|
@RashRAJ Why is this closed? Please, continue here instead of creating new PRs. Reviewers need to see previous review comments. |
|
Adding label 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-sigs/prow repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: RashRAJ 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 |
|
@RashRAJ: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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-sigs/prow repository. I understand the commands that are listed here. |
|
PR needs rebase. 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-sigs/prow repository. |
What type of PR is this?
What this PR does / why we need it:
Which issue(s) this PR is related to:
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: