Skip to content

fix: honor klog -stderrthreshold even when -logtostderr is true#1101

Open
pierluigilenoci wants to merge 3 commits into
netgroup-polito:masterfrom
pierluigilenoci:fix/honor-stderrthreshold
Open

fix: honor klog -stderrthreshold even when -logtostderr is true#1101
pierluigilenoci wants to merge 3 commits into
netgroup-polito:masterfrom
pierluigilenoci:fix/honor-stderrthreshold

Conversation

@pierluigilenoci
Copy link
Copy Markdown

What changed

klog v2 defaults -logtostderr to true, which silently ignores -stderrthreshold — all log levels go to stderr unconditionally. This has been an open issue since 2020.

klog v2.140.0 introduced a fix behind an opt-in flag (legacy_stderr_threshold_behavior). This PR bumps klog to v2.140.0 across both go.mod files and enables the fix in all 7 entry points:

  • operators/cmd/instmetrics/main.go
  • operators/cmd/instance-automation/main.go
  • operators/cmd/bastion-operator/main.go
  • operators/cmd/instance-operator/main.go
  • operators/cmd/operator/main.go
  • operators/pkg/examagent/options.go
  • infrastructure/ingress-controller/custom-error-pages/server/main.go

The fix preserves the current default behavior (stderrthreshold=INFO means all logs still go to stderr unless the user explicitly overrides it on the command line).

References

@pierluigilenoci pierluigilenoci requested a review from a team as a code owner April 23, 2026 08:55
@kingmakerbot
Copy link
Copy Markdown
Collaborator

Hi @pierluigilenoci. Thanks for your PR.

I am @kingmakerbot.
You can interact with me issuing a slash command in the first line of a comment.
Currently, I understand the following commands:

  • /rebase: Rebase this PR onto the master branch
  • /merge: Merge this PR into the master branch
  • /hold: Adds hold label to prevent merging with /merge
  • /unhold: Removes the hold label to allow merging with /merge
  • /deploy-staging: Deploy a staging environment to test this PR (the build-all flag enables user environments building)
  • /undeploy-staging: Manually undeploy the staging environment

Make sure this PR appears in the CrownLabs changelog, adding one of the following labels:

  • kind/breaking: 💥 Breaking Change
  • kind/feature: 🚀 New Feature
  • kind/bug: 🐛 Bug Fix
  • kind/cleanup: 🧹 Code Refactoring
  • kind/docs: 📝 Documentation

@QcFe
Copy link
Copy Markdown
Collaborator

QcFe commented Apr 23, 2026

Hi @pierluigilenoci, thanks for your contribution.
Unfortunately several tests aren't passing, as you may have noticed. Apart from having removed a couple of rather crucial directives without which the applications you touched can't start, changes like these would also require changes to the associated helm charts. Also whenever go linting checks are explicitly disabled, we generally require a comment to explain the reason why they have been.

@pierluigilenoci
Copy link
Copy Markdown
Author

Hi @QcFe, thanks for the review! You're absolutely right — I'm sorry about the broken state.

I've pushed a fix (62f8e4b) that addresses all the issues you raised:

Restored accidentally deleted directives:

  • bastion-operator/main.go: restored the ctrl.NewManager(...) call that was removed
  • instance-automation/main.go: restored ctrl.SetLogger(...), log := ctrl.Log.WithName("setup"), and whiteListMap := parseMap(...) — without these the application can't start
  • instance-operator/main.go: restored ctrl.SetLogger(textlogger.NewLogger(textlogger.NewConfig()))
  • instmetrics/main.go: restored log := textlogger.NewLogger(textlogger.NewConfig()).WithName("instmetrics")
  • operator/main.go: restored the // Set the ctrl.Log to klogr comment
  • examagent/options.go: restored the // Parse and normalize options. doc comment
  • custom-error-pages/server/main.go: restored the // Load response templates comment

Added explanatory nolint comments:
All //nolint:errcheck directives now include a reason: // flag is registered by klog.InitFlags above

Regarding helm chart changes: The flag.Set() calls set defaults before flag.Parse(), so any --stderrthreshold=... passed via helm chart args will override the code defaults. If you'd prefer explicit helm chart values for these flags, happy to add them — please let me know.

Thanks again for catching these!

@pierluigilenoci pierluigilenoci force-pushed the fix/honor-stderrthreshold branch from 39bcebf to bda5974 Compare May 2, 2026 09:44
pierluigilenoci and others added 3 commits May 7, 2026 19:06
klog v2 defaults -logtostderr to true, which silently ignores
-stderrthreshold — all log levels go to stderr unconditionally.
This has been an open issue since 2020 (kubernetes/klog#212).

klog v2.140.0 introduced a fix behind an opt-in flag
(legacy_stderr_threshold_behavior). This commit bumps klog to
v2.140.0 and enables the fix in all operator entry points and the
custom-error-pages server so that -stderrthreshold is honored,
while preserving the current default behavior (stderrthreshold=INFO
means all logs still go to stderr unless the user overrides it on
the command line).

Modified files:
- operators/cmd/instmetrics/main.go
- operators/cmd/instance-automation/main.go
- operators/cmd/bastion-operator/main.go
- operators/cmd/instance-operator/main.go
- operators/cmd/operator/main.go
- operators/pkg/examagent/options.go
- infrastructure/ingress-controller/custom-error-pages/server/main.go

Ref: kubernetes/klog#212
Ref: kubernetes/klog#432
Signed-off-by: Pierluigi Lenoci <pierluigilenoci@gmail.com>
Restore crucial lines that were accidentally removed when adding the
klog stderrthreshold fix:
- bastion-operator: restore ctrl.NewManager call
- instance-automation: restore ctrl.SetLogger, log, and whiteListMap
- instance-operator: restore ctrl.SetLogger
- instmetrics: restore log variable initialization
- operator: restore SetLogger comment
- examagent: restore Parse doc comment
- custom-error-pages: restore "Load response templates" comment

Also add explanatory comments to all //nolint:errcheck directives as
required by project linting conventions.

Signed-off-by: Pierluigi Lenoci <pierluigilenoci@gmail.com>
The nolint:errcheck directive suppresses the errcheck linter but not
gosec, which independently reports G104 (Errors unhandled) for the
same flag.Set() calls. Add gosec to the nolint directive to suppress
both linters consistently across all six files.

Signed-off-by: Pierluigi Lenoci <pierluigilenoci@gmail.com>
@pierluigilenoci pierluigilenoci force-pushed the fix/honor-stderrthreshold branch from bda5974 to f1f68d5 Compare May 7, 2026 17:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants