refactor: use %w for proper error wrapping across codebase#1613
refactor: use %w for proper error wrapping across codebase#1613imshubham22apr-gif wants to merge 2 commits intokmesh-net:mainfrom
Conversation
This PR significantly enhances the kmeshctl CLI tool and the Kmesh status server to improve testability, robustness, and cross-platform support. Key changes include: - Refactored kmeshctl subcommands (authz, dump, monitoring, secret, version, log) to return errors and use RunE instead of os.Exit, enabling reliable unit testing. - Extracted core CLI logic into modular, testable functions and added unit tests using httptest to mock Kmesh daemon responses. - Fixed resource leaks in the eBPF logger by ensuring ringbuf readers are properly closed and added bounds checks in decodeRecord. - Isolated eBPF logging code for Linux and added stubs for non-Linux builds, fixing Windows build failures. - Corrected HTTP header ordering in status server handlers. - Added new debug endpoints for services, policies, and certificates config dumps. - Fixed typos in monitoring command error messages and renamed misspelled variables. Signed-off-by: imshubham22apr-gif <imshubham.22apr@gmail.com>
kmesh-bot
requested review from
Okabe-Rintarou-0,
YaoZengzeng,
hzxuzhonghu and
lec-bit
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the error handling mechanisms throughout the codebase by adopting Go's structured error wrapping ( Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Errors now wrap with care, Chain of cause, a clearer view, Debugging's light shines. Footnotes
|
There was a problem hiding this comment.
Pull request overview
This PR aims to improve error-chain preservation across the project by switching many fmt.Errorf call sites to use %w when formatting underlying errors, enabling errors.Is/As in callers. It also includes some related cleanup in the CLI and status server, plus additional unit tests.
Changes:
- Refactor many
fmt.Errorf(..., err)call sites from%v/%s/%+vto%wfor proper wrapping. - Add new status server config-dump endpoints for services and policies (and tests for them).
- Refactor several
kmeshctlcommands to return errors (instead of exiting) and add unit tests for CLI packages.
Reviewed changes
Copilot reviewed 58 out of 58 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| test/e2e/gateway_test.go | Wraps underlying error with %w in e2e helper. |
| test/bpf_ut/bpftest/workload_test.go | Wraps syscall-related errors with %w. |
| test/bpf_ut/bpftest/tlv_utils.go | Wraps parse error with %w. |
| pkg/utils/tc.go | Wraps netlink/ethtool errors with %w. |
| pkg/utils/fileop.go | Wraps file operation errors with %w before logging/returning. |
| pkg/utils/enroll.go | Wraps netns execution error with %w. |
| pkg/utils/ebpf.go | Wraps eBPF iteration/lookup errors with %w. |
| pkg/status/status_server_test.go | Adds tests for new config-dump endpoints. |
| pkg/status/status_server.go | Adds /debug/config_dump/services and /policies, plus a generic JSON dump helper and a 200 header for logger names. |
| pkg/logger/logger_stub.go | Adds non-linux stub for BPF log reader. |
| pkg/logger/logger_linux.go | Moves BPF ringbuf log reader into linux-only file and hardens decode/bounds checks. |
| pkg/logger/logger.go | Removes BPF log reader implementation from generic logger file. |
| pkg/kube/portforwarder.go | Wraps kubectl port-forward errors with %w. |
| pkg/kube/client.go | Wraps “available port” error with %w. |
| pkg/dns/dns.go | Wraps resolve error with %w. |
| pkg/controller/xdstest/fake_xdsclient.go | Wraps gRPC client/stream errors with %w. |
| pkg/controller/workload/workload_processor.go | Wraps many workload processing errors with %w. |
| pkg/controller/workload/workload_controller.go | Wraps stream/send/recv errors with %w. |
| pkg/controller/telemetry/map_metric.go | Wraps iterator error with %w. |
| pkg/controller/security/mock/mockcaclient.go | Wraps cert/key parsing and CSR errors with %w. |
| pkg/controller/security/manager.go | Trailing whitespace change only. |
| pkg/controller/security/caclient.go | Wraps gRPC/cert parsing errors with %w. |
| pkg/controller/manage/manage_controller.go | Wraps informer/netns/tc errors with %w. |
| pkg/controller/encryption/ipsec/ipsec_handler_test.go | Wraps XFRM list error with %w. |
| pkg/controller/encryption/ipsec/ipsec_handler.go | Wraps file decode/watcher/xfrm errors with %w. |
| pkg/controller/encryption/ipsec/ipsec_controller.go | Wraps informer/k8s/tc errors with %w. |
| pkg/controller/controller.go | Wraps controller startup errors with %w. |
| pkg/controller/client.go | Wraps grpc connect/stream creation errors with %w. |
| pkg/controller/bypass/bypass_controller.go | Wraps namespace/iptables execution errors with %w. |
| pkg/controller/ads/ads_controller.go | Wraps ADS stream errors with %w. |
| pkg/consistenthash/maglev/maglev.go | Wraps map load/update errors with %w. |
| pkg/cni/plugin/plugin.go | Wraps CNI attach/k8s lookup errors with %w. |
| pkg/cni/install_test.go | Wraps test retry errors with %w. |
| pkg/cni/install.go | Wraps watcher add error with %w. |
| pkg/cni/chained.go | Wraps CNI config parsing/marshal/copy errors with %w. |
| pkg/cache/v2/maps/route.go | Wraps route conversion error with %w. |
| pkg/cache/v2/maps/listener.go | Wraps socket/listener conversion errors with %w. |
| pkg/cache/v2/maps/cluster.go | Wraps cluster conversion error with %w. |
| pkg/cache/v2/maps/authz.go | Wraps authz conversion error with %w. |
| pkg/bpf/workload/loader.go | Wraps BPF load/attach/init errors with %w. |
| pkg/bpf/utils/pin.go | Wraps pin/unpin errors with %w. |
| pkg/bpf/bpf_test.go | Wraps mount/rlimit errors with %w. |
| pkg/bpf/ads/loader_enhanced.go | Wraps BPF load/attach errors with %w. |
| pkg/bpf/ads/loader.go | Wraps BPF load/attach/init errors with %w. |
| ctl/waypoint/waypoint_test.go | Adds basic CLI command registration tests. |
| ctl/version/version.go | Converts to RunE and returns errors instead of exiting; updates getVersion signature. |
| ctl/utils/utils.go | Wraps kube client/port-forwarder creation errors with %w. |
| ctl/secret/secret_test.go | Adds CLI command registration tests. |
| ctl/secret/secret.go | Converts to RunE and returns errors instead of exiting; command operations now return error. |
| ctl/monitoring/monitoring_test.go | Adds tests for command flags, arg parsing, and HTTP posting behavior. |
| ctl/monitoring/monitoring.go | Converts to RunE and returns errors instead of exiting; refactors HTTP calls into helper. |
| ctl/log/log_test.go | Adds tests for command and HTTP helpers. |
| ctl/log/log.go | Converts to RunE, returns errors instead of exiting, and returns output via cmd writer. |
| ctl/dump/dump_test.go | Adds tests for command flags, helpers, and config dump fetching. |
| ctl/dump/dump.go | Converts to RunE, returns errors instead of exiting; factors GET logic into GetConfigDump. |
| ctl/common/common_test.go | Adds tests ensuring root command registers subcommands. |
| ctl/authz/authz_test.go | Adds tests for command registration and HTTP helpers. |
| ctl/authz/authz.go | Converts to RunE, returns errors instead of exiting; refactors POST/GET into helpers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| if err := utils.Execute("iptables", args); err != nil { | ||
| return fmt.Errorf("failed to exec command: iptables %v\", err: %v", args, err) | ||
| return fmt.Errorf("failed to exec command: iptables %v\", err: %w", args, err) | ||
| } |
| cKey, err := socketAddressToClang(key) | ||
| if err != nil { | ||
| return fmt.Errorf("ListenerLookup %s", err) | ||
| return fmt.Errorf("ListenerLookup %w", err) | ||
| } |
| if !cmd.Flags().Changed("key") { | ||
| aeadKey = make([]byte, AeadKeyLength) | ||
| _, err := rand.Read(aeadKey) | ||
| if err != nil { | ||
| log.Errorf("failed to generate random key: %v", err) | ||
| os.Exit(1) | ||
| return fmt.Errorf("failed to generate random key: %v", err) | ||
| } | ||
| } else { | ||
| aeadKey, err = hex.DecodeString(aeadKeyArg) | ||
| if err != nil { | ||
| log.Errorf("failed to decode hex string: %v, input: %v", err, aeadKeyArg) | ||
| os.Exit(1) | ||
| return fmt.Errorf("failed to decode hex string: %v, input: %v", err, aeadKeyArg) | ||
| } |
| client, err := utils.CreateKubeClient() | ||
| if err != nil { | ||
| log.Errorf("failed to create cli client: %v", err) | ||
| os.Exit(1) | ||
| return fmt.Errorf("failed to create cli client: %v", err) | ||
| } |
| fw, err := utils.CreateKmeshPortForwarder(cli, podName) | ||
| if err != nil { | ||
| log.Errorf("failed to create port forwarder for Kmesh daemon pod %s: %v", podName, err) | ||
| os.Exit(1) | ||
| return fmt.Errorf("failed to create port forwarder for Kmesh daemon pod %s: %v", podName, err) | ||
| } | ||
| if err := fw.Start(); err != nil { | ||
| log.Errorf("failed to start port forwarder for Kmesh daemon pod %s: %v", podName, err) | ||
| os.Exit(1) | ||
| return fmt.Errorf("failed to start port forwarder for Kmesh daemon pod %s: %v", podName, err) | ||
| } |
| cli, err := utils.CreateKubeClient() | ||
| if err != nil { | ||
| log.Errorf("failed to create cli client: %v", err) | ||
| os.Exit(1) | ||
| return fmt.Errorf("failed to create cli client: %v", err) | ||
| } | ||
|
|
||
| fw, err := utils.CreateKmeshPortForwarder(cli, podName) | ||
| if err != nil { | ||
| log.Errorf("failed to create port forwarder for Kmesh daemon pod %s: %v", podName, err) | ||
| os.Exit(1) | ||
| return fmt.Errorf("failed to create port forwarder for Kmesh daemon pod %s: %v", podName, err) | ||
| } | ||
| if err := fw.Start(); err != nil { | ||
| log.Errorf("failed to start port forwarder for Kmesh daemon pod %s: %v", podName, err) | ||
| os.Exit(1) | ||
| return fmt.Errorf("failed to start port forwarder for Kmesh daemon pod %s: %v", podName, err) | ||
| } |
| return | ||
| return v, fmt.Errorf("failed to read HTTP response body: %w", err) | ||
| } | ||
|
|
| cKey, err := socketAddressToClang(key) | ||
| if err != nil { | ||
| return fmt.Errorf("ListenerLookup %s", err) | ||
| return fmt.Errorf("ListenerLookup %w", err) |
| func createKubeClient() (kube.CLIClient, error) { | ||
| clientset, err := utils.CreateKubeClient() | ||
| if err != nil { | ||
| log.Errorf("failed to connect k8s client, %v", err) | ||
| os.Exit(1) | ||
| return nil, fmt.Errorf("failed to connect k8s client, %v", err) | ||
| } |
| services := s.xdsClient.WorkloadController.Processor.ServiceCache.List() | ||
| dumpJSON(w, services, ConvertService, "services") | ||
| } |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request refactors the kmeshctl command-line tool by converting Cobra Run functions to RunE for improved error propagation and replaces direct os.Exit calls with proper error returns. It extracts several helper functions (e.g., SetAuthz, GetConfigDump, SetLoggerLevel, SetObservability) to enhance modularity and testability, accompanied by new unit tests for these functionalities. Additionally, the logger package is updated for better cross-platform compatibility by separating Linux-specific eBPF log handling, and the status server gains new endpoints for dumping K8s services and security policies. The primary feedback from the review comments points to a widespread issue where fmt.Errorf uses %v instead of %w for error wrapping, hindering proper error inspection and requiring correction across numerous locations.
| if err != nil { | ||
| log.Errorf("failed to create cli client: %v", err) | ||
| os.Exit(1) | ||
| return fmt.Errorf("failed to create cli client: %v", err) |
There was a problem hiding this comment.
Throughout this file, fmt.Errorf is used with %v to wrap errors, which contradicts the PR's goal of using %w for proper error wrapping. This prevents error inspection with errors.Is or errors.As. Please change %v to %w in the following locations:
- line 101
- line 109
- line 155
- line 162
- line 185
- line 188
- line 200
- line 207
- line 237
- line 244
- line 254
| return fmt.Errorf("failed to create cli client: %v", err) | |
| return fmt.Errorf("failed to create cli client: %w", err) |
| if err != nil { | ||
| log.Errorf("failed to create cli client: %v", err) | ||
| os.Exit(1) | ||
| return fmt.Errorf("failed to create cli client: %v", err) |
There was a problem hiding this comment.
Throughout this file, fmt.Errorf is used with %v to wrap errors, which contradicts the PR's goal of using %w for proper error wrapping. This prevents error inspection with errors.Is or errors.As. Please change %v to %w in the following locations:
- line 78
- line 83
- line 86
- line 115
- line 125
| return fmt.Errorf("failed to create cli client: %v", err) | |
| return fmt.Errorf("failed to create cli client: %w", err) |
| if err != nil { | ||
| log.Errorf("failed to create cli client: %v", err) | ||
| os.Exit(1) | ||
| return fmt.Errorf("failed to create cli client: %v", err) |
There was a problem hiding this comment.
Throughout this file, fmt.Errorf is used with %v to wrap errors, which contradicts the PR's goal of using %w for proper error wrapping. This prevents error inspection with errors.Is or errors.As. Please change %v to %w in the following locations:
- line 93
- line 131
- line 181
- line 184
- line 196
- line 203
- line 213
| return fmt.Errorf("failed to create cli client: %v", err) | |
| return fmt.Errorf("failed to create cli client: %w", err) |
| if err != nil { | ||
| log.Errorf("failed to connect k8s client, %v", err) | ||
| os.Exit(1) | ||
| return nil, fmt.Errorf("failed to connect k8s client, %v", err) |
There was a problem hiding this comment.
Throughout this file, fmt.Errorf is used with %v to wrap errors, which contradicts the PR's goal of using %w for proper error wrapping. This prevents error inspection with errors.Is or errors.As. Please change %v to %w in the following locations:
- line 113
- line 136
- line 141
- line 156
- line 162
- line 169
- line 185
- line 190
- line 207
- line 217
- line 235
- line 257
| return nil, fmt.Errorf("failed to connect k8s client, %v", err) | |
| return nil, fmt.Errorf("failed to connect k8s client: %w", err) |
What type of PR is this? /kind cleanup
What this PR does / why we need it: This PR refactors fmt.Errorf calls throughout the codebase to consistently use the %w verb instead of %v, %s, or %+v when formatting error variables (typically named err).
Why we need it: Using verbs like %v or %s converts an error into a simple text string, which destroys the original error's identity. This prevents caller functions from using errors.Is() or errors.As() to programmatically inspect or unwrap the error. By switching to %w, we ensure that the error is properly wrapped, preserving the error chain and making the codebase more robust for error domestic logic.
Which issue(s) this PR fixes: Fixes #
Special notes for your reviewer:
A systematic scan was performed to identify fmt.Errorf occurrences using grep.
Changes were strictly limited to instances where the argument being formatted is of an error type.
Formatting for other variables (strings, integers, etc.) within the same fmt.Errorf calls has been left unchanged to maintain existing log/error message clarity.
Does this PR introduce a user-facing change?: NONE