Migrate pkg/kubelet/certificate to contextual logging #132193

RashRAJ · 2025-06-09T13:27:11Z

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.

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.:

- 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.

linux-foundation-easycla · 2025-06-09T13:27:15Z

The committers listed above are authorized under a signed CLA.

✅ login: RashRAJ / name: Rasheedat atinuke jamiu (0854a4d, 35c7be8, 2a70d2a)

k8s-ci-robot · 2025-06-09T13:27:19Z

Welcome @RashRAJ!

It looks like this is your first PR to kubernetes/kubernetes 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/kubernetes has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2025-06-09T13:27:20Z

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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

bart0sh · 2025-06-10T10:48:21Z

hack/tools/go.mod

@@ -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


where this change comes from?

bart0sh · 2025-06-10T10:52:14Z

pkg/kubelet/certificate/kubelet.go

@@ -278,18 +278,20 @@ type kubeletServerCertificateDynamicFileManager struct {
 	keyFile                   string
 	dynamicCertificateContent *dynamiccertificates.DynamicCertKeyPairContent
 	currentTLSCertificate     atomic.Pointer[tls.Certificate]
+	ctx                       context.Context


Storing context in a structure instead of passing as a parameter to functions considered anti-pattern in go, please avoid.

completely agree, specially where there is already a cancelFn context.CancelFunc in the same structure

bart0sh · 2025-06-10T10:56:13Z

/triage accepted
/ref #130069
/ok-to-test
/priority important-longterm

bart0sh · 2025-06-10T10:56:26Z

/assign

bart0sh · 2025-06-10T10:56:57Z

/kind cleanup

bart0sh · 2025-06-10T10:58:17Z

/release-note-none

bart0sh · 2025-06-10T10:59:53Z

@RashRAJ Please, update golangci configs as it's done in other PRs mentioned in the #130069

RashRAJ · 2025-06-10T11:02:57Z

@RashRAJ Please, update golangci configs as it's done in other PRs mentioned in the #130069

Sure !!!

aojea · 2025-06-10T11:08:23Z

pkg/kubelet/certificate/transport.go

@@ -79,7 +80,7 @@ func updateTransport(stopCh <-chan struct{}, period time.Duration, clientConfig
 	return d.CloseAll, nil
 }

-func addCertRotation(stopCh <-chan struct{}, period time.Duration, clientConfig *restclient.Config, clientCertificateManager certificate.Manager, exitAfter time.Duration, d *connrotation.Dialer) error {
+func addCertRotation(ctx context.Context, stopCh <-chan struct{}, period time.Duration, clientConfig *restclient.Config, clientCertificateManager certificate.Manager, exitAfter time.Duration, d *connrotation.Dialer) error {


this has a stopCh, we must ensure that if we pass a context it actual cancels the function, as it is what people expect from it ... please avoid passing just the context to plumb down the logger, the context cancellation needs to happen

aojea · 2025-06-10T11:11:17Z

pkg/kubelet/certificate/kubelet.go

-	ctx, m.cancelFn = context.WithCancel(context.Background())
-	go m.dynamicCertificateContent.Run(ctx, 1)
+	m.ctx, m.cancelFn = context.WithCancel(context.Background())
+	go m.dynamicCertificateContent.Run(m.ctx, 1)


didn't dig too much into the code , but it seems this method has to be rewritten to pass the context through Start(ctx)

aojea · 2025-06-10T11:11:53Z

pkg/kubelet/certificate/transport.go

@@ -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) {


the stopCh <-chan struct{} should be derived from the context

k8s-ci-robot · 2025-06-12T11:08:18Z

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.

k8s-ci-robot · 2025-06-12T11:08:30Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: RashRAJ
Once this PR has been reviewed and has the lgtm label, please assign liggitt for approval. For more information see the Code Review Process.

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot · 2025-06-12T11:09:51Z

@RashRAJ: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-kubernetes-e2e-kind-ipv6	`35c7be8`	link	true	`/test pull-kubernetes-e2e-kind-ipv6`
pull-kubernetes-conformance-kind-ga-only-parallel	`0854a4d`	link	true	`/test pull-kubernetes-conformance-kind-ga-only-parallel`
pull-kubernetes-e2e-gce	`0854a4d`	link	true	`/test pull-kubernetes-e2e-gce`
pull-kubernetes-unit	`0854a4d`	link	true	`/test pull-kubernetes-unit`
pull-kubernetes-dependencies	`0854a4d`	link	true	`/test pull-kubernetes-dependencies`
pull-kubernetes-node-e2e-containerd	`0854a4d`	link	true	`/test pull-kubernetes-node-e2e-containerd`
pull-kubernetes-e2e-kind	`0854a4d`	link	true	`/test pull-kubernetes-e2e-kind`
check-dependency-stats	`0854a4d`	link	false	`/test check-dependency-stats`
pull-kubernetes-typecheck	`0854a4d`	link	true	`/test pull-kubernetes-typecheck`
pull-kubernetes-integration	`0854a4d`	link	true	`/test pull-kubernetes-integration`
pull-kubernetes-verify	`0854a4d`	link	true	`/test pull-kubernetes-verify`
pull-kubernetes-cmd	`0854a4d`	link	true	`/test pull-kubernetes-cmd`
pull-kubernetes-linter-hints	`0854a4d`	link	false	`/test pull-kubernetes-linter-hints`
pull-kubernetes-unit-windows-master	`0854a4d`	link	false	`/test pull-kubernetes-unit-windows-master`

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.

RashRAJ added 2 commits June 8, 2025 14:08

Migrate pkg/kubelet/certificate to contextual logging

2a70d2a

Migrate pkg/kubelet/certificate to contextual logging

35c7be8

- 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.

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Jun 9, 2025

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jun 9, 2025

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jun 9, 2025

k8s-ci-robot added the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Jun 9, 2025

k8s-ci-robot requested review from dchen1107, mikedanese and a team June 9, 2025 13:27

k8s-ci-robot added area/dependency Issues or PRs related to dependency changes area/kubelet sig/auth Categorizes an issue or PR as relevant to SIG Auth. sig/node Categorizes an issue or PR as relevant to SIG Node. labels Jun 9, 2025

github-project-automation bot added this to SIG Auth and SIG Node: code and documentation PRs Jun 9, 2025

k8s-ci-robot removed the do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. label Jun 9, 2025

github-project-automation bot moved this to Triage in SIG Node: code and documentation PRs Jun 9, 2025

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Jun 9, 2025

enj moved this to Needs Triage in SIG Auth Jun 9, 2025

bart0sh mentioned this pull request Jun 10, 2025

Migrate Kubelet codebase to contextual logging #130069

Open

49 tasks

bart0sh reviewed Jun 10, 2025

View reviewed changes

k8s-ci-robot assigned bart0sh Jun 10, 2025

k8s-ci-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. and removed do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Jun 10, 2025

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Jun 10, 2025

bart0sh moved this from Triage to Needs Reviewer in SIG Node: code and documentation PRs Jun 10, 2025

aojea reviewed Jun 10, 2025

View reviewed changes

Refactor context handling

0854a4d

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 12, 2025

Migrate pkg/kubelet/certificate to contextual logging #132193

Are you sure you want to change the base?

Migrate pkg/kubelet/certificate to contextual logging #132193

Uh oh!

Conversation

RashRAJ commented Jun 9, 2025

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.:

Uh oh!

linux-foundation-easycla bot commented Jun 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Jun 9, 2025

Uh oh!

k8s-ci-robot commented Jun 9, 2025

Uh oh!

bart0sh Jun 10, 2025

Choose a reason for hiding this comment

Uh oh!

bart0sh Jun 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aojea Jun 10, 2025

Choose a reason for hiding this comment

Uh oh!

bart0sh commented Jun 10, 2025

Uh oh!

bart0sh commented Jun 10, 2025

Uh oh!

bart0sh commented Jun 10, 2025

Uh oh!

bart0sh commented Jun 10, 2025

Uh oh!

bart0sh commented Jun 10, 2025

Uh oh!

RashRAJ commented Jun 10, 2025

Uh oh!

aojea Jun 10, 2025

Choose a reason for hiding this comment

Uh oh!

aojea Jun 10, 2025

Choose a reason for hiding this comment

Uh oh!

aojea Jun 10, 2025

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Jun 12, 2025

Uh oh!

k8s-ci-robot commented Jun 12, 2025

Uh oh!

k8s-ci-robot commented Jun 12, 2025

Uh oh!

Uh oh!

linux-foundation-easycla bot commented Jun 9, 2025 •

edited

Loading

bart0sh Jun 10, 2025 •

edited

Loading