feat: add cgroup_skb eBPF program #1474

wxnzb · 2025-08-06T13:37:34Z

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Signed-off-by: sweeywu <sweetwx6@gmail.com>

gemini-code-assist

Summary of Changes

Hello @wxnzb, I'm Gemini Code Assist¹! 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 introduces a new configuration option for eBPF programs to enable periodic reporting and significantly enhances the unit test coverage for cgroup_skb eBPF programs. The added tests validate the functionality of ingress and egress cgroup_skb programs, ensuring their correct operation in monitoring and managing TCP connection details within cgroups.

Highlights

New BPF Configuration: Added EnablePeriodicReport to the GlobalBpfConfig struct, introducing a new configuration option for controlling periodic reporting within BPF programs.
Expanded BPF Test Suite: Introduced a comprehensive new test suite (testCgroupSkb) specifically for cgroup_skb eBPF programs, covering both ingress and egress traffic scenarios to ensure proper network monitoring and handling.
BPF Test Mocking Framework: Created a new eBPF test program (workload_cgroup_skb_test.c) that includes a mock implementation for bpf_sk_storage_get. This allows for more controlled and isolated testing of cgroup_skb logic by simulating BPF socket storage behavior.

Using Gemini Code Assist

The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature	Command	Description
Code Review	`/gemini review`	Performs a code review for the current pull request in its current state.
Pull Request Summary	`/gemini summary`	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in issue comments and review comments.
Help	`/gemini help`	Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

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 or fill out our survey to provide feedback.

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.

BPF code takes flight, Network packets, swift and bright, Tests ensure it's right.

Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

gemini-code-assist

Code Review

This pull request introduces eBPF programs attached to cgroups for sk_buff processing, along with corresponding unit tests. I've identified a few critical issues that should be addressed to ensure the code is robust and maintainable, including code duplication, a hardcoded cgroup path, and a recursive call in a BPF mock function.

gemini-code-assist · 2025-08-06T13:40:24Z

test/bpf_ut/bpftest/workload_test.go

+		}
+	}
+	lk, err := link.AttachCgroup(link.CgroupOptions{
+		Path:    constants.Cgroup2Path,


The cgroupPath parameter is passed to this function but is not used. Instead, a hardcoded value constants.Cgroup2Path is used when attaching the cgroup program. This is a bug that makes the function's behavior differ from its signature and could lead to tests attaching to the wrong cgroup if the path were to change.

Path: cgroupPath,

gemini-code-assist · 2025-08-06T13:40:24Z

test/bpf_ut/workload_cgroup_skb_test.c

+{
+    struct bpf_sock *sk_sock = (struct bpf_sock *)sk;
+    void *storage = NULL;
+    storage = bpf_sk_storage_get(map, sk_sock, value, flags);
+    if (!storage && map == &map_of_sock_storage) {
+        storage = &mock_storage;
+    }
+
+    return storage;
+}


The implementation of this mock function is incorrect. Line 21 causes infinite recursion because bpf_sk_storage_get is macro-defined as mock_bpf_sk_storage_get. This will lead to a stack overflow at runtime. BPF helpers cannot be called from userspace test code; they must be fully mocked.

The function body should be replaced with logic that simulates the helper's behavior without calling the real helper.

Suggested change

{

struct bpf_sock *sk_sock = (struct bpf_sock *)sk;

void *storage = NULL;

storage = bpf_sk_storage_get(map, sk_sock, value, flags);

if (!storage && map == &map_of_sock_storage) {

storage = &mock_storage;

}

return storage;

}

{

// Unused parameters must be cast to void to prevent compiler warnings.

(void)sk;

(void)value;

(void)flags;

// The mock should not call the real bpf_sk_storage_get helper.

// It should only return the mock data for the specific map being tested.

if (map == &map_of_sock_storage) {

return &mock_storage;

}

return NULL;

}

gemini-code-assist · 2025-08-06T13:40:25Z

test/bpf_ut/bpftest/workload_test.go

+					name: "CgroupEgress_is_managed_by_kmesh_skb",
+					workFunc: func(t *testing.T, cgroupPath, objFilePath string) {


The workFunc for CgroupEgress_is_managed_by_kmesh_skb is almost an exact copy of the one for CgroupIngress_is_managed_by_kmesh_skb. This large-scale code duplication (over 100 lines) is a significant maintainability concern. Future changes will need to be made in two places, which is error-prone.

Please consider refactoring the common logic into a shared helper function that accepts the BPF program name as a parameter. This will greatly reduce duplication and improve the long-term health of the test suite.

gemini-code-assist · 2025-08-06T13:40:25Z

test/bpf_ut/bpftest/workload_test.go

+// converts a uint32 IPv4 address into a dotted-decimal string
+func printIPv4(ip uint32) string {
+	b := make([]byte, 4)
+	b[0] = byte(ip & 0xff)
+	b[1] = byte((ip >> 8) & 0xff)
+	b[2] = byte((ip >> 16) & 0xff)
+	b[3] = byte((ip >> 24) & 0xff)
+	return fmt.Sprintf("%d.%d.%d.%d", b[3], b[2], b[1], b[0])
+}


The current implementation of printIPv4 is functionally correct but unnecessarily complex and hard to follow. It manually extracts bytes from the uint32 and reassembles them for printing. A more idiomatic and readable approach would be to use the standard library's binary and net packages. This improves maintainability and makes the code's intent clearer.

The function comment should also be updated to clarify the expected endianness of the input IP address.

// converts a uint32 IPv4 address in host byte order into a dotted-decimal string func printIPv4(ip uint32) string { b := make([]byte, 4) binary.BigEndian.PutUint32(b, ip) return net.IP(b).String() }

Signed-off-by: sweeywu <sweetwx6@gmail.com>

codecov · 2025-08-07T09:51:41Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
⚠️ Please upload report for BASE (main@af3e8e5). Learn more about missing BASE report.
⚠️ Report is 87 commits behind head on main.

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update af3e8e5...5e179d5. Read the comment docs.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Signed-off-by: sweeywu <sweetwx6@gmail.com>

LiZhenCheng9527

Almost lgtm

LiZhenCheng9527 · 2025-08-21T07:11:46Z

test/bpf_ut/bpftest/workload_test.go

+						if err := binary.Read(bytes.NewBuffer(record.RawSample), binary.LittleEndian, &info); err != nil {
+							log.Printf("parsing ringbuf event: %s", err)
+						} else {
+							fmt.Printf("tcp_probe_info:\n")


Is this the necessary output information?

LiZhenCheng9527 · 2025-08-21T07:17:29Z

@hzxuzhonghu @sancppp PTAL

hzxuzhonghu

/lgtm

Please make the pr title more descriptive

wxnzb · 2025-08-24T09:26:16Z

/lgtm

Please make the pr title more descriptive请使 pr 标题更具描述性

Got it, I will update the PR title. Thanks for the suggestion!

hzxuzhonghu · 2025-08-25T07:30:58Z

/lgtm

LiZhenCheng9527 · 2025-09-04T07:12:49Z

/approve

kmesh-bot · 2025-09-04T07:12:57Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: LiZhenCheng9527

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:

~~pkg/OWNERS~~ [LiZhenCheng9527]
~~test/OWNERS~~ [LiZhenCheng9527]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

add_cgroup_ipv4_ingress_and_egress

7f5a895

Signed-off-by: sweeywu <sweetwx6@gmail.com>

kmesh-bot requested review from Okabe-Rintarou-0 and YaoZengzeng August 6, 2025 13:37

kmesh-bot added the size/L label Aug 6, 2025

gemini-code-assist bot reviewed Aug 6, 2025

View reviewed changes

wxnzb added 2 commits August 6, 2025 22:33

add_makefile_about_cgroup_skb

467d055

Signed-off-by: sweeywu <sweetwx6@gmail.com>

fix makefile

d7d53cb

Signed-off-by: sweeywu <sweetwx6@gmail.com>

wxnzb added 2 commits August 11, 2025 11:41

add ingress/egress direction

1c8a88f

Signed-off-by: sweeywu <sweetwx6@gmail.com>

fix cgroup_skb_ingress_direction

5e179d5

Signed-off-by: sweeywu <sweetwx6@gmail.com>

LiZhenCheng9527 reviewed Aug 21, 2025

View reviewed changes

hzxuzhonghu reviewed Aug 22, 2025

View reviewed changes

kmesh-bot assigned hzxuzhonghu Aug 22, 2025

kmesh-bot added the lgtm label Aug 22, 2025

wxnzb changed the title ~~cgroup_skb~~ feat: add cgroup_skb eBPF program Aug 24, 2025

kmesh-bot added the approved label Sep 4, 2025

kmesh-bot merged commit 7193230 into kmesh-net:main Sep 4, 2025
12 checks passed

wxnzb mentioned this pull request Sep 11, 2025

New membership for Xi Wu #1502

Closed

6 tasks

-{
-    struct bpf_sock *sk_sock = (struct bpf_sock *)sk;
-    void *storage = NULL;
-    storage = bpf_sk_storage_get(map, sk_sock, value, flags);
-    if (!storage && map == &map_of_sock_storage) {
-        storage = &mock_storage;
-    }
-    return storage;
-}
+{
+    // Unused parameters must be cast to void to prevent compiler warnings.
+    (void)sk;
+    (void)value;
+    (void)flags;
+    // The mock should not call the real bpf_sk_storage_get helper.
+    // It should only return the mock data for the specific map being tested.
+    if (map == &map_of_sock_storage) {
+        return &mock_storage;
+    }
+    return NULL;
+}

		name: "CgroupEgress_is_managed_by_kmesh_skb",
		workFunc: func(t *testing.T, cgroupPath, objFilePath string) {

feat: add cgroup_skb eBPF program #1474

feat: add cgroup_skb eBPF program #1474

Uh oh!

Conversation

wxnzb commented Aug 6, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Summary of Changes

Highlights

Footnotes

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Aug 6, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Aug 6, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Aug 6, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Aug 6, 2025

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Aug 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

LiZhenCheng9527 left a comment

Choose a reason for hiding this comment

Uh oh!

LiZhenCheng9527 Aug 21, 2025

Choose a reason for hiding this comment

Uh oh!

LiZhenCheng9527 commented Aug 21, 2025

Uh oh!

hzxuzhonghu left a comment

Choose a reason for hiding this comment

Uh oh!

wxnzb commented Aug 24, 2025

Uh oh!

hzxuzhonghu commented Aug 25, 2025

Uh oh!

LiZhenCheng9527 commented Sep 4, 2025

Uh oh!

kmesh-bot commented Sep 4, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented Aug 7, 2025 •

edited

Loading