Conversation
|
[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 observability of eBPF programs by refining their logging mechanisms. The changes focus on improving clarity, consistency, and safety of log messages, making it easier to diagnose and understand the flow of network traffic and policy enforcement. By standardizing log formats, normalizing log levels, and reducing noise in critical paths, the update provides more actionable insights without altering any functional or datapath behavior. 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. 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. In eBPF's realm, where packets fly, Logs now shine clear, beneath the sky. No longer vague, but sharp and bright, Guiding the way, through day and night. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request significantly improves BPF logging by standardizing formats, normalizing log levels, and enhancing context in log messages. The changes to support multiple IP-to-string conversions in a single log statement are well-implemented. I have one suggestion to further improve observability by including the policy action in the policy match log message, which is crucial for debugging now that logs have been removed from the should_shutdown hot path.
There was a problem hiding this comment.
Pull request overview
This PR focuses on improving observability across the workload eBPF programs by standardizing log levels and formats, reducing hot-path log noise, and enhancing IP-to-string logging safety.
Changes:
- Normalize log levels (INFO/DEBUG/ERR) and standardize
src=[%s:%u], dst=[%s:%u]formatting across programs. - Reduce XDP hot-path logging noise and keep logs at key decision points.
- Extend the per-CPU temp buffer approach (
ip2str_idx) to support multiple IP conversions within a single log statement.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| bpf/kmesh/workload/xdp.c | Removes hot-path auth-deny logs; improves frontend lookup/update failure logging. |
| bpf/kmesh/workload/sockops.c | Adds richer error logs with src/dst tuple context. |
| bpf/kmesh/workload/sendmsg.c | Adds dst context to encoding debug logs. |
| bpf/kmesh/workload/include/service.h | Promotes key service decision logs to INFO and adds dst context. |
| bpf/kmesh/workload/include/frontend.h | Promotes waypoint decision logs to INFO and standardizes dst formatting. |
| bpf/kmesh/workload/include/backend.h | Standardizes dst formatting and promotes key routing/DNAT logs to INFO. |
| bpf/kmesh/workload/include/authz.h | Adds src/dst context to policy/port matching logs; promotes “policy matched” to INFO. |
| bpf/kmesh/workload/cgroup_sock.c | Adds dst context to cgroup connect/TC logs and error cases. |
| bpf/include/common.h | Adds ip2str_idx() and increases tmp per-CPU buffers to allow multiple conversions per log statement. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
062ff21 to
5a0097e
Compare
Signed-off-by: Aaravanand00 <aaravanand5749@gmail.com>
Signed-off-by: Aaravanand00 <aaravanand5749@gmail.com>
…mits Signed-off-by: Aaravanand00 <aaravanand5749@gmail.com>
5a0097e to
45dc068
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| BPF_LOG(ERR, SOCKOPS, "set sockops cb failed for src=[%s:%u], dst=[%s:%u]\n", | ||
| ip2str_idx((__u32 *)&skops->local_ip4, (skops->family == AF_INET), 0), skops->local_port, | ||
| ip2str_idx((__u32 *)&skops->remote_ip4, (skops->family == AF_INET), 1), bpf_ntohs(GET_SKOPS_REMOTE_PORT(skops))); |
There was a problem hiding this comment.
This log always reads skops->local_ip4/remote_ip4, even when skops->family == AF_INET6. Besides producing incorrect addresses for IPv6, this unconditional field access can also be rejected by the verifier on some kernels. Prefer selecting local_ip4/remote_ip4 vs local_ip6/remote_ip6 based on skops->family (or build a bpf_sock_tuple via extract_skops_to_tuple and log from that).
| BPF_LOG(ERR, AUTH, "failed to retrieve match_context from map for src=[%s:%u]\n", | ||
| ip2str((__u32 *)&tuple_key.ipv4.saddr, (info.iph->version == 4)), tuple_key.ipv4.sport); | ||
| return XDP_PASS; |
There was a problem hiding this comment.
This error log prints src=[%s:%u] using tuple_key.ipv4.sport regardless of IP version. For IPv6 tuples, ipv4.sport does not alias ipv6.sport, so the port logged will be incorrect. Use info.iph->version to select ipv4 vs ipv6 fields (and consider using ip2str_idx with separate buffers if you ever log both src/dst in the same statement).
| BPF_LOG(INFO, AUTH, "policy %s matched, src=[%s:%u], dst=[%s:%u]\n", | ||
| match_ctx->policy_name, | ||
| ip2str_idx((__u32 *)&tuple_key.ipv4.saddr, (info.iph->version == 4), 0), tuple_key.ipv4.sport, | ||
| ip2str_idx((__u32 *)&tuple_key.ipv4.daddr, (info.iph->version == 4), 1), tuple_key.ipv4.dport); |
There was a problem hiding this comment.
In this INFO log, src/dst ports are taken from tuple_key.ipv4.{sport,dport} even when processing IPv6 packets; those fields don’t overlap the IPv6 port offsets, so ports will be wrong. Also, for IPv6 dst, using &tuple_key.ipv4.daddr does not point at tuple_key.ipv6.daddr, so the destination address string will be wrong. Please branch on info.iph->version and use tuple_key.ipv6.{saddr,daddr,sport,dport} for IPv6.
| if (bpf_map_delete_elem(&kmesh_tc_args, &tuple_key) != 0) { | ||
| BPF_LOG(ERR, AUTH, "failed to delete tail call context from map"); | ||
| BPF_LOG(ERR, AUTH, "failed to delete tail call context from map for src=[%s:%u]\n", | ||
| ip2str((__u32 *)&tuple_key.ipv4.saddr, (info.iph->version == 4)), tuple_key.ipv4.sport); | ||
| } |
There was a problem hiding this comment.
This delete-failure log prints src=[%s:%u] using tuple_key.ipv4.sport regardless of IP version. For IPv6 packets, ipv4.sport does not alias ipv6.sport, so the logged port will be incorrect. Use info.iph->version to choose the correct port/address fields before logging.
| __u32 auth_result = match_ctx->action == ISTIO__SECURITY__ACTION__DENY ? AUTH_DENY : AUTH_ALLOW; | ||
| if (bpf_map_update_elem(&map_of_auth_result, &tuple_key, &auth_result, BPF_ANY) != 0) { | ||
| BPF_LOG(ERR, AUTH, "failed to update auth result in map_of_auth_result"); | ||
| BPF_LOG(ERR, AUTH, "failed to update auth result in map_of_auth_result for src=[%s:%u]\n", | ||
| ip2str((__u32 *)&tuple_key.ipv4.saddr, (info.iph->version == 4)), tuple_key.ipv4.sport); | ||
| } |
There was a problem hiding this comment.
This map update failure log prints src=[%s:%u] using tuple_key.ipv4.sport regardless of IP version. For IPv6 packets, ipv4.sport does not alias ipv6.sport, so the logged port will be incorrect. Use info.iph->version to select ipv4 vs ipv6 fields (ports and addresses) for accurate logging.
Summary
Improved BPF logging clarity and consistency across multiple eBPF programs.
Changes
Normalized log levels:
Standardized log format:
src=[%s:%u], dst=[%s:%u]dest=→dst=)Reduced log noise:
should_shutdown)Improved logging safety:
Fixed issues:
info.iph.version→info->iph->version)Notes
bpf_map_delete_elemlogic (Q2) was intentionally not modified