Skip to content

fix: clean up malformed CNI error output#762

Open
immanuwell wants to merge 1 commit into
linkerd:mainfrom
immanuwell:fix/cni-error-formatting
Open

fix: clean up malformed CNI error output#762
immanuwell wants to merge 1 commit into
linkerd:mainfrom
immanuwell:fix/cni-error-formatting

Conversation

@immanuwell

Copy link
Copy Markdown

This fixes a small but real logging bug in cni-plugin.

Some CNI error paths use %e for error, so Go prints %!e(...) junk instead of the actual message. That shows up on normal failures like bad CNI JSON, bad kube config, or missing integration mounts. Kinda noisy, and it hides the real problem.

What changed

  • switch those log and test format strings to %v or %s
  • add a small regression test for the invalid JSON path
  • fix the proxy-gid mismatch message in the integration test helper

Repro

  1. On main, run go test ./cni-plugin/integration/tests/flannel -run TestLinkerdIsLastCNIPlugin -v
  2. In a plain local env, the expected mount is missing, so the test fails
  3. Before this patch the failure contains %!e(...)
  4. With this patch it prints the real path error

Checks

  • go test ./cni-plugin ./proxy-init/... ./pkg/...

@immanuwell immanuwell requested a review from a team as a code owner June 8, 2026 09:41

@raykroeker raykroeker left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks @immanuwell for the PR. Instead of using format strings, please use the structured error field on the logger.

Comment thread cni-plugin/main.go Outdated
Signed-off-by: immanuwell <pchpr.00@list.ru>
@immanuwell immanuwell force-pushed the fix/cni-error-formatting branch from 78f82d6 to 3c7b884 Compare June 29, 2026 17:23
@immanuwell immanuwell requested a review from raykroeker June 29, 2026 17:32
@immanuwell

Copy link
Copy Markdown
Author

@raykroeker done, addressed all the requested changes, PTAL

@alpeb alpeb left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks @immanuwell , the main change looks good. I don't think you should add a unit test for logging though; it tests the logging framework not our behavior and I can see this sort of thing can become a burden to maintain when the framework changes.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants