ut_cgroup_sock #1453
ut_cgroup_sock #1453
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @wxnzb, 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 introduces a new set of unit tests for BPF cgroup socket programs. These tests are designed to validate the behavior of connection handling and redirection logic within a BPF-based service mesh environment, ensuring proper management of network namespace cookies and correct routing of connections through various service configurations, including those involving waypoints.
Highlights
- New BPF Unit Test: Added
workload_cgroup_sock_test.oto the build system, enabling the compilation of a new BPF program specifically for unit testing cgroup socket functionality. - Cgroup Socket Program Testing: Integrated a new
testCgroupSocksuite into the existing Go-based BPF unit test framework, expanding test coverage for BPF programs interacting with cgroup sockets. - Comprehensive Connection Redirection Tests: Included detailed tests for the
BPF_CGROUP_SOCK_CONNECT4hook, covering critical scenarios such as recording and removing kmesh managed network namespace cookies, and connection redirection to backend services both with and without a configured waypoint, as well as service-level waypoint redirection. - eBPF Program Type Support: Updated the BPF test framework to correctly recognize and handle
ebpf.CGroupSockAddrprogram types during the loading and preparation of eBPF specifications.
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.
Footnotes
-
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. ↩
There was a problem hiding this comment.
Code Review
This pull request adds unit tests for cgroup_sock BPF programs. The changes are well-structured, introducing a new test file, updating the Makefile for compilation, and adding the test logic in Go.
My review focuses on improving the robustness and maintainability of the new test code. I've identified a few critical and high-severity issues that should be addressed:
- The use of
log.Fatalfin tests, which can prematurely terminate the entire test suite. - Potential panics from unchecked errors and improper handling of network connections.
- A resource leak due to an unclosed network listener.
Additionally, I've included some medium-severity suggestions to enhance code quality by reducing duplication and adhering to language-specific conventions. Addressing these points will make the new tests more reliable and easier to maintain in the long run.
test/bpf_ut/bpftest/workload_test.go
Outdated
| UpstreamID: 1, | ||
| } | ||
| if err := FrontendMap.Update(&f_key, &f_val, ebpf.UpdateAny); err != nil { | ||
| log.Fatalf("Update failed: %v", err) |
There was a problem hiding this comment.
Using log.Fatalf inside a test is a critical issue because it will terminate the entire test process, preventing other tests from running. You should use t.Fatalf instead, which will only fail the current test and allow the test suite to continue with other tests.
This issue occurs in multiple places in this file:
- line 894
- line 924
- line 1017
- line 1049
- line 1144
- line 1174
Please replace all instances of log.Fatalf with t.Fatalf.
| log.Fatalf("Update failed: %v", err) | |
| t.Fatalf("Update failed: %v", err) |
| conn, err := (&net.Dialer{ | ||
| LocalAddr: &net.TCPAddr{ | ||
| IP: net.ParseIP(localIP), | ||
| Port: clientPort, | ||
| }, | ||
| Timeout: 2 * time.Second, | ||
| }).Dial("tcp4", serverSocket) | ||
| defer conn.Close() |
There was a problem hiding this comment.
The error returned from Dial is not being checked. If Dial fails, conn will be nil, and the subsequent defer conn.Close() will cause a panic when the test function returns. You must check the error and handle it appropriately.
| conn, err := (&net.Dialer{ | |
| LocalAddr: &net.TCPAddr{ | |
| IP: net.ParseIP(localIP), | |
| Port: clientPort, | |
| }, | |
| Timeout: 2 * time.Second, | |
| }).Dial("tcp4", serverSocket) | |
| defer conn.Close() | |
| conn, err := (&net.Dialer{ | |
| LocalAddr: &net.TCPAddr{ | |
| IP: net.ParseIP(localIP), | |
| Port: clientPort, | |
| }, | |
| Timeout: 2 * time.Second, | |
| }).Dial("tcp4", serverSocket) | |
| if err != nil { | |
| t.Fatalf("Dial failed: %v", err) | |
| } | |
| defer conn.Close() |
test/bpf_ut/bpftest/workload_test.go
Outdated
| serverSocket := localIP + ":" + strconv.Itoa(serverPort) | ||
| var testPort uint16 = 55555 | ||
| testIpPort := localIP + ":" + strconv.Itoa(int(htons(testPort))) | ||
| _, err := net.Listen("tcp4", testIpPort) |
There was a problem hiding this comment.
The net.Listen call can fail, but the error is not being checked. Additionally, the returned listener is not being closed, which will cause a resource leak. You should check the error and ensure the listener is closed using defer.
| _, err := net.Listen("tcp4", testIpPort) | |
| testListener, err := net.Listen("tcp4", testIpPort) | |
| if err != nil { | |
| t.Fatalf("Failed to listen on test port %s: %v", testIpPort, err) | |
| } | |
| defer testListener.Close() |
test/bpf_ut/Makefile
Outdated
| WORKLOAD_CGROUP_SOCK_FLAGS = -I$(ROOT_DIR)/bpf/kmesh/ \ | ||
| -I$(ROOT_DIR)/bpf/kmesh/probes \ | ||
| -I$(ROOT_DIR)/bpf/kmesh/workload/include \ | ||
| -I$(ROOT_DIR)/api/v2-c | ||
| workload_cgroup_sock_test.o: workload_cgroup_sock_test.c | ||
| $(QUIET) $(CLANG) $(CLANG_FLAGS) $(WORKLOAD_CGROUP_SOCK_FLAGS) -c $< -o $@ |
There was a problem hiding this comment.
The variable WORKLOAD_CGROUP_SOCK_FLAGS is a duplicate of the existing WORKLOAD_SOCKOPS_FLAGS defined on line 41. To improve maintainability and avoid redundancy, you can reuse the existing variable for the new build rule.
workload_cgroup_sock_test.o: workload_cgroup_sock_test.c
$(QUIET) $(CLANG) $(CLANG_FLAGS) $(WORKLOAD_SOCKOPS_FLAGS) -c $< -o $@
test/bpf_ut/bpftest/workload_test.go
Outdated
| type ip_addr struct { | ||
| Raw [16]byte | ||
| } | ||
|
|
||
| type frontend_key struct { | ||
| Addr ip_addr | ||
| } | ||
|
|
||
| type frontend_value struct { | ||
| UpstreamID uint32 | ||
| } | ||
|
|
||
| FrontendMap := coll.Maps["km_frontend"] | ||
| var f_key frontend_key | ||
| ip4 := net.ParseIP(localIP).To4() | ||
| if ip4 == nil { | ||
| t.Fatalf("invalid IPv4 address") | ||
| } | ||
| copy(f_key.Addr.Raw[0:4], ip4) | ||
| //construct the value | ||
| f_val := frontend_value{ | ||
| UpstreamID: 1, | ||
| } | ||
| if err := FrontendMap.Update(&f_key, &f_val, ebpf.UpdateAny); err != nil { | ||
| log.Fatalf("Update failed: %v", err) | ||
| } | ||
| //populate the km_backend | ||
| BackendMap := coll.Maps["km_backend"] | ||
| const MAX_SERVICE_COUNT = 10 | ||
| type backend_key struct { | ||
| BackendUID uint32 | ||
| } | ||
|
|
||
| type backend_value struct { | ||
| Addr ip_addr | ||
| ServiceCount uint32 | ||
| Service [MAX_SERVICE_COUNT]uint32 | ||
| WpAddr ip_addr | ||
| WaypointPort uint32 | ||
| } | ||
| //construct the key | ||
| b_key := backend_key{ | ||
| BackendUID: 1, | ||
| } | ||
| //construct the value | ||
| b_val := backend_value{ | ||
| Addr: ip_addr{Raw: [16]byte{}}, | ||
| ServiceCount: 0, | ||
| Service: [MAX_SERVICE_COUNT]uint32{}, | ||
| WpAddr: ip_addr{Raw: [16]byte{}}, | ||
| WaypointPort: 0, | ||
| } |
There was a problem hiding this comment.
These struct types (ip_addr, frontend_key, frontend_value, backend_key, backend_value) are redefined in each test case. This leads to significant code duplication and makes the code harder to maintain.
Consider defining these types once at a shared scope, such as at the beginning of the testCgroupSock function or at the package level, and reuse them across all test cases.
test/bpf_ut/bpftest/workload_test.go
Outdated
| if ip4 == nil { | ||
| t.Fatalf("invalid IPv4 address") | ||
| } | ||
| copy(f_key.Addr.Raw[0:4], ip4) // 高位放IPv4 |
There was a problem hiding this comment.
This comment is in Chinese. To ensure the codebase is accessible and maintainable for all contributors, please write comments in English.
| copy(f_key.Addr.Raw[0:4], ip4) // 高位放IPv4 | |
| copy(f_key.Addr.Raw[0:4], ip4) // Store IPv4 in the high bytes of the address |
test/bpf_ut/bpftest/workload_test.go
Outdated
| t.Run(tt.objFilename, tt.run()) | ||
| } | ||
| } | ||
| func load_bpf_prog_to_cgroup(t *testing.T, objFilename string, progName string, cgroupPath string) (*ebpf.Collection, link.Link) { |
There was a problem hiding this comment.
In Go, function names should use CamelCase instead of snake_case. Please rename load_bpf_prog_to_cgroup to loadBpfProgToCgroup to follow idiomatic Go style. This also applies to the htons function on line 1252.
| func load_bpf_prog_to_cgroup(t *testing.T, objFilename string, progName string, cgroupPath string) (*ebpf.Collection, link.Link) { | |
| func loadBpfProgToCgroup(t *testing.T, objFilename string, progName string, cgroupPath string) (*ebpf.Collection, link.Link) { |
There was a problem hiding this comment.
Pull Request Overview
This PR adds unit tests for the workload_cgroup_sock component, introducing comprehensive testing for BPF CGROUP socket connection handling including managed process tracking, backend routing with and without waypoints, and service-based waypoint routing.
Key changes:
- Adds new test file to include the workload cgroup socket source code
- Implements 4 comprehensive test cases covering different BPF cgroup socket scenarios
- Updates build configuration to compile the new test object file
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| test/bpf_ut/workload_cgroup_sock_test.c | Simple include directive to incorporate cgroup socket source code for testing |
| test/bpf_ut/bpftest/workload_test.go | Main test implementation with 4 test scenarios for cgroup socket functionality |
| test/bpf_ut/bpftest/bpf_test.go | Updates program type filter to include CGroupSockAddr for test compatibility |
| test/bpf_ut/Makefile | Adds build rules and flags for compiling the new cgroup socket test object |
Comments suppressed due to low confidence (3)
test/bpf_ut/bpftest/workload_test.go:772
- The type name 'unitTests_BUILD_CONTEXT' uses inconsistent naming convention. Consider using 'unitTestsBuildContext' or 'UnitTestsBuildContext' to follow Go naming conventions.
tests := []unitTests_BUILD_CONTEXT{
test/bpf_ut/bpftest/workload_test.go:775
- The type name 'unitTest_BUILD_CONTEXT' uses inconsistent naming convention. Consider using 'unitTestBuildContext' or 'UnitTestBuildContext' to follow Go naming conventions.
uts: []unitTest_BUILD_CONTEXT{
test/bpf_ut/bpftest/workload_test.go:1146
- Variable 'BackendMap' is misleading as it actually references the 'km_service' map, not a backend map. Consider renaming to 'ServiceMap' for clarity.
BackendMap := coll.Maps["km_service"]
test/bpf_ut/bpftest/workload_test.go
Outdated
| serverSocket := localIP + ":" + strconv.Itoa(serverPort) | ||
| var testPort uint16 = 55555 | ||
| testIpPort := localIP + ":" + strconv.Itoa(int(htons(testPort))) | ||
| _, err := net.Listen("tcp4", testIpPort) |
There was a problem hiding this comment.
The listener created here is not stored or closed, which may cause resource leaks. The error is also ignored. Either store the listener and defer its closure, or handle the error appropriately.
| _, err := net.Listen("tcp4", testIpPort) | |
| listener, err := net.Listen("tcp4", testIpPort) | |
| if err != nil { | |
| t.Fatalf("Failed to create listener on %s: %v", testIpPort, err) | |
| } | |
| defer listener.Close() |
test/bpf_ut/bpftest/workload_test.go
Outdated
| wpIP := net.ParseIP(localIP).To4() | ||
| //insert value | ||
| s_val := service_value{ | ||
| WpAddr: ip_addr{Raw: [16]byte{}}, // waypoint IP全0，表示无 |
There was a problem hiding this comment.
The comment contains Chinese characters. Consider using English for consistency: '// waypoint IP all zeros, indicates none'
| WpAddr: ip_addr{Raw: [16]byte{}}, // waypoint IP全0，表示无 | |
| WpAddr: ip_addr{Raw: [16]byte{}}, // waypoint IP all zeros, indicates none |
test/bpf_ut/bpftest/workload_test.go
Outdated
| //insert value | ||
| s_val := service_value{ | ||
| WpAddr: ip_addr{Raw: [16]byte{}}, // waypoint IP全0，表示无 | ||
| WaypointPort: uint32(testPort), // |
There was a problem hiding this comment.
Empty comment should either be removed or completed with meaningful description of what the waypoint port represents.
| WaypointPort: uint32(testPort), // | |
| WaypointPort: uint32(testPort), // Port used for waypoint communication in the service |
test/bpf_ut/bpftest/workload_test.go
Outdated
| } | ||
| } | ||
| lk, err := link.AttachCgroup(link.CgroupOptions{ | ||
| Path: constants.Cgroup2Path, |
There was a problem hiding this comment.
The cgroup attachment uses 'constants.Cgroup2Path' instead of the provided 'cgroupPath' parameter, which could cause the program to attach to the wrong cgroup.
| Path: constants.Cgroup2Path, | |
| Path: cgroupPath, |
test/bpf_ut/Makefile
Outdated
| tc_mark_encrypt_test.o \ | ||
| tc_mark_decrypt_test.o | ||
| tc_mark_decrypt_test.o \ | ||
| workload_cgroup_sock_test.o |
There was a problem hiding this comment.
Trailing whitespace should be removed for consistency with the rest of the Makefile.
| workload_cgroup_sock_test.o | |
| workload_cgroup_sock_test.o |
|
Please fix lint error and use |
Signed-off-by: sweeywu <sweetwx6@gmail.com>
Signed-off-by: sweeywu <sweetwx6@gmail.com>
Signed-off-by: sweeywu <sweetwx6@gmail.com>
Signed-off-by: sweeywu <sweetwx6@gmail.com>
Signed-off-by: sweeywu <sweetwx6@gmail.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Signed-off-by: sweeywu <sweetwx6@gmail.com>
Signed-off-by: sweeywu <sweetwx6@gmail.com>
Signed-off-by: sweeywu <sweetwx6@gmail.com>
hzxuzhonghu
left a comment
hzxuzhonghu
left a comment
There was a problem hiding this comment.
just one comments, can you abstract the redundant code{like simulate kmesh manage and check map} into help functions
Signed-off-by: sweeywu <sweetwx6@gmail.com>
Signed-off-by: sweeywu <sweetwx6@gmail.com>
Signed-off-by: sweeywu <sweetwx6@gmail.com>
Signed-off-by: sweeywu <sweetwx6@gmail.com>
Signed-off-by: sweeywu <sweetwx6@gmail.com>
|
Adding label Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Signed-off-by: sweeywu <sweetwx6@gmail.com>
Signed-off-by: sweeywu <sweetwx6@gmail.com>
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hzxuzhonghu, LiZhenCheng9527 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
4 participants
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?: