Skip to content

✨ (go/v4): Add custom k8s linter for logs to promote better observability#5396

Merged
k8s-ci-robot merged 1 commit intokubernetes-sigs:masterfrom
camilamacedo86:log-linter-check
Jan 29, 2026
Merged

✨ (go/v4): Add custom k8s linter for logs to promote better observability#5396
k8s-ci-robot merged 1 commit intokubernetes-sigs:masterfrom
camilamacedo86:log-linter-check

Conversation

@camilamacedo86
Copy link
Member

@camilamacedo86 camilamacedo86 commented Jan 26, 2026

What this PR does

Adds the custom linter sigs.k8s.io/logtools/logcheck to validate logs in Kubebuilder scaffolds.

This linter checks for common problematic logging scenarios and fails CI with actionable errors. Example failure output:
https://github.com/kubernetes-sigs/kubebuilder/actions/runs/21430987832/job/61710036984?pr=5396

Why we’re doing this

We checked with the logcheck maintainers and, while adoption is relatively low, the project appears stable and maintained:
https://kubernetes.slack.com/archives/C020CCMUEAX/p1769592452395209

Given there have been no reported bugs since 2024, it seems like a good fit for improving the quality of scaffolded logging patterns in Kubebuilder.

Links

Fixes: #5330

@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 26, 2026
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 26, 2026
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 28, 2026
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 28, 2026
@camilamacedo86 camilamacedo86 force-pushed the log-linter-check branch 2 times, most recently from 9b1bdaf to f9f164b Compare January 28, 2026 08:08
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 28, 2026
@camilamacedo86 camilamacedo86 force-pushed the log-linter-check branch 2 times, most recently from 1261225 to 72064ab Compare January 28, 2026 08:11
@camilamacedo86 camilamacedo86 changed the title WIP: (chore): Add linter for logs ✨ (go/v4): Add custom k8s linter for logs to promote better observability Jan 28, 2026
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 28, 2026
@camilamacedo86 camilamacedo86 changed the title ✨ (go/v4): Add custom k8s linter for logs to promote better observability ✨ (go/v4): Add custom k8s linter for logs to promote better observability Jan 28, 2026
@camilamacedo86 camilamacedo86 changed the title ✨ (go/v4): Add custom k8s linter for logs to promote better observability WIP ✨ (go/v4): Add custom k8s linter for logs to promote better observability Jan 28, 2026
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 28, 2026
@camilamacedo86 camilamacedo86 force-pushed the log-linter-check branch 3 times, most recently from d1006e6 to 4f14542 Compare January 28, 2026 09:23
@camilamacedo86 camilamacedo86 changed the title WIP ✨ (go/v4): Add custom k8s linter for logs to promote better observability ✨ (go/v4): Add custom k8s linter for logs to promote better observability Jan 28, 2026
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 28, 2026
log.Error(err, "unable to delete old successful job", "job", job)
} else {
log.V(0).Info("deleted old successful job", "job", job)
log.V(1).Info("deleted old successful job", "job", job)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix issue found in the docs samples within

log.Error(err, "unable to delete old successful job", "job", job)
} else {
log.V(0).Info("deleted old successful job", "job", job)
log.V(1).Info("deleted old successful job", "job", job)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix issue found in the docs samples within

log.Error(err, "unable to delete old successful job", "job", job)
} else {
log.V(0).Info("deleted old successful job", "job", job)
log.V(1).Info("deleted old successful job", "job", job)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix issue found in the docs samples within

Copy link
Contributor

@mloskot mloskot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great stuff, thank you!

Comment on lines +120 to +123
log.Warn("Unable to find boilerplate file. "+
"This file is used to generate the license header in the project. "+
"Note that controller-gen will also use this. Therefore, ensure that you "+
"add the license file or configure your project accordingly.",
log.Warn("Unable to find boilerplate file "+
"used to generate the license header in the project "+
"Note that controller-gen will also use this Therefore ensure that you "+
"add the license file or configure your project accordingly",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a dup change from PR #5406
See also my #5406 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you .. it should not be part of the changes in this PR.
Out of scope.

@camilamacedo86
Copy link
Member Author

@vitorfloriano any reason for we not LGTM this one?
WDYT?

Copy link
Contributor

@vitorfloriano vitorfloriano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was a bit unsure at first because we use log/slog in Kubebuilder itself, but turns out the scaffolds use the logger from controller-runtime, which use logr and klog, so the linter works for the scaffolds.

\lgtm

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: camilamacedo86, vitorfloriano

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details 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

@camilamacedo86
Copy link
Member Author

camilamacedo86 commented Jan 29, 2026

I was a bit unsure at first because we use log/slog in Kubebuilder itself, but turns out the scaffolds use the logger from controller-runtime, which use logr and klog, so the linter works for the scaffolds.

We were able to reproduce the issue as well, and it shows up in our logs, so the current setup appears to be working correctly on our side.
That said, I’m going to raise a question for C+R: why aren’t we using klog here? Since Kubernetes uses klog, we often prefer to align with Kubernetes standards—so understanding the reasoning would help.

@camilamacedo86 camilamacedo86 added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 29, 2026
@mloskot
Copy link
Contributor

mloskot commented Jan 29, 2026

@camilamacedo86

why aren’t we using klog here? Since Kubernetes uses klog

Long ago I asked similar question on the Slack channel #klog and I think you may find the answers I received there from @pohly (*) informative in the context here, quoting:

In Kubernetes we are moving away from using "traditional" klog towards structured, contextual logging via the go-logr API.
See https://github.com/kubernetes/component-base/tree/master/logs/example for an example that enables -v/-vmodule and JSON log output.
(...) klog has lots of command line flags and function calls which are not used and not recommended by Kubernetes.

(*) I hope Patrick doesn't mind the mention, but I wanted to include persistent reference to source of that quote - I'm not 100% if the history of the Kubernetes Slack space persist.

@camilamacedo86
Copy link
Member Author

@mloskot I understand the the linter still valid right?
I will ask for its mantainers too but let me know if you see any reason for not

@camilamacedo86
Copy link
Member Author

/override pull-kubebuilder-e2e-k8s-1-34-0

@k8s-ci-robot
Copy link
Contributor

@camilamacedo86: Overrode contexts on behalf of camilamacedo86: pull-kubebuilder-e2e-k8s-1-34-0

Details

In response to this:

/override pull-kubebuilder-e2e-k8s-1-34-0

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 k8s-ci-robot merged commit 15eb316 into kubernetes-sigs:master Jan 29, 2026
40 of 41 checks passed
@camilamacedo86 camilamacedo86 deleted the log-linter-check branch January 29, 2026 14:56
@mloskot
Copy link
Contributor

mloskot commented Jan 29, 2026

@camilamacedo86 I'd expect the linter to be still valid, but I don't know for sure as I did not dig the topic of future of klog in K8s any further.

@vitorfloriano
Copy link
Contributor

vitorfloriano commented Jan 29, 2026

I believe logr+klog was preferred in Kubernetes to avoid breaking changes. It seems that by passing the logger by context (contextual logging), they could work around that constraint. That's what I understood from KEP 3077.

log/slog was not considered a viable alternative because it would represent a breaking change in the API of tools like controller-runtime, as per this consideration in KEP 3077:

Use log/slog instead of klog+logr

This isn't viable because slog doesn't provide a mechanism to pass a logger through a context. Therefore it would not be possible to support contextual logging in packages like client-go where adding an explicit logger parameter would be a major API break.

But that's just my reading of the whole thing.

@mloskot
Copy link
Contributor

mloskot commented Jan 29, 2026

@vitorfloriano Thanks for those references. It looks like there was a plan, that had changed along the way. Here is the part that, I think, can be considered as an update to the quote I posted in #5396 (comment), in this section:

Clean separation of contextual logging and traditional klog logging:

The initial revision of this KEP described a plan for moving all code for contextual logging into a k8s.io/klogr repository (...) However, that transition would have been complicated (...) Therefore the scope of the KEP was reduced from "remove dependency on klog" to "remove dependency on global logger in klog".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Explore adding logging best-practices + logcheck to the Kubebuilder default scaffold or optional plugin for k8s conventions linter

4 participants