test: add unit tests for legacy ADS mode bpf programs#1765
test: add unit tests for legacy ADS mode bpf programs#1765Aryanburnwal05 wants to merge 2 commits into
Conversation
Signed-off-by: Aryan Burnwal <burnwalaryan922@gmail.com>
|
[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 introduces comprehensive unit testing for legacy ADS mode eBPF programs. By implementing a custom loader, the changes allow for testing core attach logic and map interactions in isolation, effectively bypassing verifier constraints associated with complex tail-call structures. Highlights
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 the 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 counterproductive. 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 kernels deep where programs run, / We test the code to have some fun. / With loaders smart we skip the weight, / And keep the verifier from its fate. Footnotes
|
![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
This pull request introduces unit tests for the ADS BPF programs, specifically targeting cgroup_sock and sockops functionalities. It adds test source files in C, updates the build script to compile them, and implements Go-based test runners to verify the behavior of the km_manage map. The reviewer feedback highlights critical reliability issues in the Go test code, including potential nil pointer dereferences when loading BPF programs and socket leaks caused by not closing connections returned by net.DialTimeout.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| lk, err := link.AttachCgroup(link.CgroupOptions{ | ||
| Path: cgroupPath, | ||
| Attach: spec.Programs[progName].AttachType, | ||
| Program: coll.Programs[progName], | ||
| }) |
There was a problem hiding this comment.
The program progName is retrieved from spec.Programs and coll.Programs without checking if it exists. If the program is missing, this will cause a nil pointer dereference panic. We should check if the program exists and close the collection before failing the test.
| lk, err := link.AttachCgroup(link.CgroupOptions{ | |
| Path: cgroupPath, | |
| Attach: spec.Programs[progName].AttachType, | |
| Program: coll.Programs[progName], | |
| }) | |
| progSpec, ok := spec.Programs[progName] | |
| if !ok { | |
| coll.Close() | |
| t.Fatalf("Program %s not found in spec", progName) | |
| } | |
| prog := coll.Programs[progName] | |
| if prog == nil { | |
| coll.Close() | |
| t.Fatalf("Program %s not found in collection", progName) | |
| } | |
| lk, err := link.AttachCgroup(link.CgroupOptions{ | |
| Path: cgroupPath, | |
| Attach: progSpec.AttachType, | |
| Program: prog, | |
| }) |
| var prog *ebpf.Program | ||
| for _, p := range coll.Programs { | ||
| if p.Type() == ebpf.SockOps { | ||
| prog = p | ||
| break | ||
| } | ||
| } | ||
|
|
||
| lk, err := link.AttachCgroup(link.CgroupOptions{ | ||
| Path: cgroupPath, | ||
| Attach: ebpf.AttachCGroupSockOps, | ||
| Program: prog, | ||
| }) |
There was a problem hiding this comment.
If no program of type ebpf.SockOps is found in the collection, prog will remain nil. Passing a nil program to link.AttachCgroup can cause a panic or failure. We should check if prog is nil and close the collection before failing the test.
| var prog *ebpf.Program | |
| for _, p := range coll.Programs { | |
| if p.Type() == ebpf.SockOps { | |
| prog = p | |
| break | |
| } | |
| } | |
| lk, err := link.AttachCgroup(link.CgroupOptions{ | |
| Path: cgroupPath, | |
| Attach: ebpf.AttachCGroupSockOps, | |
| Program: prog, | |
| }) | |
| var prog *ebpf.Program | |
| for _, p := range coll.Programs { | |
| if p.Type() == ebpf.SockOps { | |
| prog = p | |
| break | |
| } | |
| } | |
| if prog == nil { | |
| coll.Close() | |
| t.Fatal("No SockOps program found in collection") | |
| } | |
| lk, err := link.AttachCgroup(link.CgroupOptions{ | |
| Path: cgroupPath, | |
| Attach: ebpf.AttachCGroupSockOps, | |
| Program: prog, | |
| }) |
| enableAddr := constants.ControlCommandIp4 + ":" + strconv.Itoa(int(constants.OperEnableControl)) | ||
| net.DialTimeout("tcp4", enableAddr, 2*time.Second) |
There was a problem hiding this comment.
net.DialTimeout returns a net.Conn and an error. Ignoring the returned connection leaks the socket/file descriptor if the connection succeeds. We should close the connection if it was successfully established.
| enableAddr := constants.ControlCommandIp4 + ":" + strconv.Itoa(int(constants.OperEnableControl)) | |
| net.DialTimeout("tcp4", enableAddr, 2*time.Second) | |
| enableAddr := constants.ControlCommandIp4 + ":" + strconv.Itoa(int(constants.OperEnableControl)) | |
| if conn, err := net.DialTimeout("tcp4", enableAddr, 2*time.Second); err == nil { | |
| conn.Close() | |
| } |
| disableAddr := constants.ControlCommandIp4 + ":" + strconv.Itoa(int(constants.OperDisableControl)) | ||
| net.DialTimeout("tcp4", disableAddr, 2*time.Second) |
There was a problem hiding this comment.
net.DialTimeout returns a net.Conn and an error. Ignoring the returned connection leaks the socket/file descriptor if the connection succeeds. We should close the connection if it was successfully established.
| disableAddr := constants.ControlCommandIp4 + ":" + strconv.Itoa(int(constants.OperDisableControl)) | |
| net.DialTimeout("tcp4", disableAddr, 2*time.Second) | |
| disableAddr := constants.ControlCommandIp4 + ":" + strconv.Itoa(int(constants.OperDisableControl)) | |
| if conn, err := net.DialTimeout("tcp4", disableAddr, 2*time.Second); err == nil { | |
| conn.Close() | |
| } |
| enableAddr := constants.ControlCommandIp4 + ":" + strconv.Itoa(int(constants.OperEnableControl)) | ||
| net.DialTimeout("tcp4", enableAddr, 2*time.Second) |
There was a problem hiding this comment.
net.DialTimeout returns a net.Conn and an error. Ignoring the returned connection leaks the socket/file descriptor if the connection succeeds. We should close the connection if it was successfully established.
| enableAddr := constants.ControlCommandIp4 + ":" + strconv.Itoa(int(constants.OperEnableControl)) | |
| net.DialTimeout("tcp4", enableAddr, 2*time.Second) | |
| enableAddr := constants.ControlCommandIp4 + ":" + strconv.Itoa(int(constants.OperEnableControl)) | |
| if conn, err := net.DialTimeout("tcp4", enableAddr, 2*time.Second); err == nil { | |
| conn.Close() | |
| } |
| disableAddr := constants.ControlCommandIp4 + ":" + strconv.Itoa(int(constants.OperDisableControl)) | ||
| net.DialTimeout("tcp4", disableAddr, 2*time.Second) |
There was a problem hiding this comment.
net.DialTimeout returns a net.Conn and an error. Ignoring the returned connection leaks the socket/file descriptor if the connection succeeds. We should close the connection if it was successfully established.
| disableAddr := constants.ControlCommandIp4 + ":" + strconv.Itoa(int(constants.OperDisableControl)) | |
| net.DialTimeout("tcp4", disableAddr, 2*time.Second) | |
| disableAddr := constants.ControlCommandIp4 + ":" + strconv.Itoa(int(constants.OperDisableControl)) | |
| if conn, err := net.DialTimeout("tcp4", disableAddr, 2*time.Second); err == nil { | |
| conn.Close() | |
| } |
There was a problem hiding this comment.
Pull request overview
This PR extends the existing eBPF unit test suite under test/bpf_ut/bpftest to cover legacy ADS-mode programs by adding new Go tests and build steps for ADS-specific eBPF test objects.
Changes:
- Add ADS test entrypoint in the top-level BPF test runner.
- Introduce ADS-specific Go tests that load/attach ADS cgroup_sock and sockops programs and validate
km_managemap behavior. - Extend the test build script to compile new ADS eBPF test object files.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
test/bpf_ut/build_bpf_ut_tests.sh |
Builds new ADS eBPF test objects (ads_cgroup_sock_test.o, ads_sockops_test.o). |
test/bpf_ut/bpftest/bpf_test.go |
Registers the new Ads subtest suite under TestBPF. |
test/bpf_ut/bpftest/ads_test.go |
Adds ADS-mode tests and custom loaders that skip heavy tail-call programs. |
test/bpf_ut/ads_sockops_test.c |
Test object source that includes ADS sockops.c. |
test/bpf_ut/ads_cgroup_sock_test.c |
Test object source that includes ADS cgroup_sock.c. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| lk, err := link.AttachCgroup(link.CgroupOptions{ | ||
| Path: cgroupPath, | ||
| Attach: spec.Programs[progName].AttachType, | ||
| Program: coll.Programs[progName], | ||
| }) | ||
| if err != nil { | ||
| coll.Close() | ||
| t.Fatalf("Failed to attach cgroup: %v", err) | ||
| } |
| var prog *ebpf.Program | ||
| for _, p := range coll.Programs { | ||
| if p.Type() == ebpf.SockOps { | ||
| prog = p | ||
| break | ||
| } | ||
| } | ||
|
|
||
| lk, err := link.AttachCgroup(link.CgroupOptions{ | ||
| Path: cgroupPath, | ||
| Attach: ebpf.AttachCGroupSockOps, | ||
| Program: prog, | ||
| }) | ||
| if err != nil { | ||
| coll.Close() | ||
| t.Fatalf("Failed to attach cgroup: %v", err) | ||
| } | ||
| return coll, lk |
| func testAds(t *testing.T) { | ||
| t.Run("CgroupSock", testAdsCgroupSock) | ||
| t.Run("SockOps", testAdsSockOps) | ||
| } |
Signed-off-by: Aryan Burnwal <burnwalaryan922@gmail.com>
Aryanburnwal05
force-pushed
the
test-ads-bpf
branch
from
684478a to
445b332
Compare
| iter = kmManageMap.Iterate() | ||
| count = 0 | ||
| for iter.Next(&key, &value) { | ||
| count++ | ||
| } | ||
| if count != 0 { | ||
| t.Fatalf("Expected 0 entry in km_manage map, but got %d", count) | ||
| } |
| iter = kmManageMap.Iterate() | ||
| count = 0 | ||
| for iter.Next(&keyBytes, &value) { | ||
| count++ | ||
| } | ||
|
|
||
| if count != 0 { | ||
| t.Fatalf("Expected 0 entry in km_manage map, but got %d", count) | ||
| } |
| prog := coll.Programs["sockops_prog"] | ||
| if prog == nil { | ||
| coll.Close() | ||
| t.Fatal("No SockOps program found in collection") | ||
| } | ||
|
|
||
| lk, err := link.AttachCgroup(link.CgroupOptions{ | ||
| Path: cgroupPath, | ||
| Attach: ebpf.AttachCGroupSockOps, | ||
| Program: prog, | ||
| }) |
| func load_bpf_prog_to_cgroup_ads(t *testing.T, objFilePath string, progName string, cgroupPath string) (*ebpf.Collection, link.Link) { | ||
| spec := loadAndPrepSpec(t, path.Join(*testPath, objFilePath)) | ||
|
|
| func load_bpf_2_cgroup_ads(t *testing.T, objFilePath string, cgroupPath string) (*ebpf.Collection, link.Link) { | ||
| spec := loadAndPrepSpec(t, path.Join(*testPath, objFilePath)) | ||
|
|
| func testAds(t *testing.T) { | ||
| t.Run("CgroupSock", testAdsCgroupSock) | ||
| t.Run("SockOps", testAdsSockOps) | ||
| } |
Added
BPF_PROG_TEST_RUNunit tests for thecgroup_sockandsockopseBPF programs in legacy ADS mode (test/bpf_ut/bpftest/ads_test.go).Also had to add a custom loader (
load_bpf_prog_to_cgroup_ads) to skip loading heavy tail-call programs likecluster_managerduring testing, otherwise the eBPF verifier hits complexity limits and throws aninvalid argumenterror due to the massive unrolled loops. With this bypass the core attach logic and map interactions get tested in isolation perfectly.Tests passed locally :-
go test -v ./test/bpf_ut/bpftest -run TestBPF/Ads