feat: eBPF codegen toolchain for the uSID TC-BPF program - #282
Conversation
|
Same disclosure as on #281 — ecv reviewed this through an AI shell, apologies. Questions, not verdicts.
One note rather than a question: because this PR targets |
ecv reviewed PR #282 and raised six questions about the eBPF codegen toolchain (commit 1e4a2e1). This addresses each: - Pin the drift check's compiler (clang-18/llvm-18, explicit versioned packages) instead of the unversioned clang/llvm meta-packages, whose underlying version moves with runner-image bumps and would otherwise flip `git diff --exit-code` red on PRs that never touched usid.c. doc.go's go:generate directive drops the hardcoded `-cc clang` so bpf2go falls back to its own $BPF2GO_CC env override, letting CI pin the compiler without the directive itself needing to change. - Stop regenerating the eBPF object during the container build. The Dockerfile now ships the committed usid_bpfel.o/usid_bpfeb.o as-is (go:embed'd into the binary), so the object shipped in the image is bit-for-bit the one committed to git, reviewed in the PR, and verified against usid.c by CI's drift check -- not a separate build produced by the builder image's own clang. This also means the Dockerfile no longer needs a toolchain at all, which resolves the apt dependency-list mismatch between it and CI in the same move. - `build:ebpf` now warns and skips (building against whatever's already committed) instead of hard-failing when clang isn't installed, so contributors touching unrelated Go don't need the eBPF toolchain. CI's drift check remains the actual correctness gate regardless of what any individual contributor has installed locally. - CI's `build` job now installs linux-libc-dev explicitly alongside the pinned clang/llvm, rather than relying on it happening to be preinstalled on the runner image -- doc.go's -idirafter workaround exists specifically to find that package's headers. - `test-unit-root` is now in test-e2e's `needs`, so a failure there blocks the merge chain instead of being advisory-only. (Branch protection's required-status-checks list should also list it directly -- that's a repo setting, not something this commit can change.) - `test-unit-root` now runs scripts/ci.sh's new `unittest-root` case, which discovers requireRoot(t)-gated packages by grep instead of re-running the entire suite as root. Scopes the real-kernel run to just the packages that need it (today: internal/cni, internal/plumbing/ebpf/prog, internal/plumbing/loaddr, internal/vmtap) and picks up future root-gated packages automatically as later milestones add them. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
7a672d4 to
10d1fe8
Compare
ecv reviewed PR #282 and raised six questions about the eBPF codegen toolchain (commit 1e4a2e1). This addresses each: - Pin the drift check's compiler (clang-18/llvm-18, explicit versioned packages) instead of the unversioned clang/llvm meta-packages, whose underlying version moves with runner-image bumps and would otherwise flip `git diff --exit-code` red on PRs that never touched usid.c. doc.go's go:generate directive drops the hardcoded `-cc clang` so bpf2go falls back to its own $BPF2GO_CC env override, letting CI pin the compiler without the directive itself needing to change. - Stop regenerating the eBPF object during the container build. The Dockerfile now ships the committed usid_bpfel.o/usid_bpfeb.o as-is (go:embed'd into the binary), so the object shipped in the image is bit-for-bit the one committed to git, reviewed in the PR, and verified against usid.c by CI's drift check -- not a separate build produced by the builder image's own clang. This also means the Dockerfile no longer needs a toolchain at all, which resolves the apt dependency-list mismatch between it and CI in the same move. - `build:ebpf` now warns and skips (building against whatever's already committed) instead of hard-failing when clang isn't installed, so contributors touching unrelated Go don't need the eBPF toolchain. CI's drift check remains the actual correctness gate regardless of what any individual contributor has installed locally. - CI's `build` job now installs linux-libc-dev explicitly alongside the pinned clang/llvm, rather than relying on it happening to be preinstalled on the runner image -- doc.go's -idirafter workaround exists specifically to find that package's headers. - `test-unit-root` is now in test-e2e's `needs`, so a failure there blocks the merge chain instead of being advisory-only. (Branch protection's required-status-checks list should also list it directly -- that's a repo setting, not something this commit can change.) - `test-unit-root` now runs scripts/ci.sh's new `unittest-root` case, which discovers requireRoot(t)-gated packages by grep instead of re-running the entire suite as root. Scopes the real-kernel run to just the packages that need it (today: internal/cni, internal/plumbing/ebpf/prog, internal/plumbing/loaddr, internal/vmtap) and picks up future root-gated packages automatically as later milestones add them. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
10d1fe8 to
c54f5ed
Compare
ef73ddf changed the BPF ELF license section from "Dual BSD/GPL" to "AGPL-3.0-or-later" on the premise that "none of the helpers this program calls today are gpl_only." That premise is false: the VRF FIB lookup already calls bpf_fib_lookup(), which the kernel marks gpl_only (net/core/filter.c's bpf_fib_lookup_proto). The kernel's license_is_gpl_compatible() check only recognizes a fixed whitelist of exact strings, and "AGPL-3.0-or-later" isn't on it, so the program now fails to load at all: "cannot call GPL-restricted function from non-GPL compatible program". This surfaced as PR #282's new CI jobs failing -- the Build job's drift check flagged the stale committed .o/.go (usid.c had changed but they hadn't been regenerated since), and once regenerated, Unit Tests (root) then hit the verifier rejection above on every FIB-lookup-path test. Revert the license string to "Dual BSD/GPL" and correct the surrounding comment accordingly -- the ELF license section governs which helpers the verifier allows and is independent of (doesn't relicense) the file's own AGPL-3.0-or-later SPDX header, same as Cilium, Katran, and every other AGPL/Apache/BSD-licensed project embedding a BPF datapath. Regenerate usid_bpfel.o/usid_bpfeb.o and their Go bindings from the corrected usid.c (also picking up the vrf_value.dropped_packets field ef73ddf added, which needed the same regeneration its own commit message flagged as still pending). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
ef73ddf changed the BPF ELF license section from "Dual BSD/GPL" to "AGPL-3.0-or-later" on the premise that "none of the helpers this program calls today are gpl_only." That premise is false: the VRF FIB lookup already calls bpf_fib_lookup(), which the kernel marks gpl_only (net/core/filter.c's bpf_fib_lookup_proto). The kernel's license_is_gpl_compatible() check only recognizes a fixed whitelist of exact strings, and "AGPL-3.0-or-later" isn't on it, so the program now fails to load at all: "cannot call GPL-restricted function from non-GPL compatible program". This surfaced as PR #282's new CI jobs failing -- the Build job's drift check flagged the stale committed .o/.go (usid.c had changed but they hadn't been regenerated since), and once regenerated, Unit Tests (root) then hit the verifier rejection above on every FIB-lookup-path test. Set the license string to plain "GPL" -- not "Dual BSD/GPL"; this program isn't itself dual-licensed, so it declares the license it actually means rather than a disjunction it doesn't. Correct the surrounding comment accordingly: the ELF license section governs which helpers the verifier allows and is independent of (doesn't relicense) the file's own AGPL-3.0-or-later SPDX header, same as Cilium, Katran, and every other AGPL/Apache/BSD-licensed project embedding a BPF datapath. Regenerate usid_bpfel.o/usid_bpfeb.o and their Go bindings from the corrected usid.c (also picking up the vrf_value.dropped_packets field ef73ddf added, which needed the same regeneration its own commit message flagged as still pending). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
this one's got failing tests @privateip |
c54f5ed to
7ac4585
Compare
ecv reviewed PR #282 and raised six questions about the eBPF codegen toolchain (commit 1e4a2e1). This addresses each: - Pin the drift check's compiler (clang-18/llvm-18, explicit versioned packages) instead of the unversioned clang/llvm meta-packages, whose underlying version moves with runner-image bumps and would otherwise flip `git diff --exit-code` red on PRs that never touched usid.c. doc.go's go:generate directive drops the hardcoded `-cc clang` so bpf2go falls back to its own $BPF2GO_CC env override, letting CI pin the compiler without the directive itself needing to change. - Stop regenerating the eBPF object during the container build. The Dockerfile now ships the committed usid_bpfel.o/usid_bpfeb.o as-is (go:embed'd into the binary), so the object shipped in the image is bit-for-bit the one committed to git, reviewed in the PR, and verified against usid.c by CI's drift check -- not a separate build produced by the builder image's own clang. This also means the Dockerfile no longer needs a toolchain at all, which resolves the apt dependency-list mismatch between it and CI in the same move. - `build:ebpf` now warns and skips (building against whatever's already committed) instead of hard-failing when clang isn't installed, so contributors touching unrelated Go don't need the eBPF toolchain. CI's drift check remains the actual correctness gate regardless of what any individual contributor has installed locally. - CI's `build` job now installs linux-libc-dev explicitly alongside the pinned clang/llvm, rather than relying on it happening to be preinstalled on the runner image -- doc.go's -idirafter workaround exists specifically to find that package's headers. - `test-unit-root` is now in test-e2e's `needs`, so a failure there blocks the merge chain instead of being advisory-only. (Branch protection's required-status-checks list should also list it directly -- that's a repo setting, not something this commit can change.) - `test-unit-root` now runs scripts/ci.sh's new `unittest-root` case, which discovers requireRoot(t)-gated packages by grep instead of re-running the entire suite as root. Scopes the real-kernel run to just the packages that need it (today: internal/cni, internal/plumbing/ebpf/prog, internal/plumbing/loaddr, internal/vmtap) and picks up future root-gated packages automatically as later milestones add them. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
ef73ddf changed the BPF ELF license section from "Dual BSD/GPL" to "AGPL-3.0-or-later" on the premise that "none of the helpers this program calls today are gpl_only." That premise is false: the VRF FIB lookup already calls bpf_fib_lookup(), which the kernel marks gpl_only (net/core/filter.c's bpf_fib_lookup_proto). The kernel's license_is_gpl_compatible() check only recognizes a fixed whitelist of exact strings, and "AGPL-3.0-or-later" isn't on it, so the program now fails to load at all: "cannot call GPL-restricted function from non-GPL compatible program". This surfaced as PR #282's new CI jobs failing -- the Build job's drift check flagged the stale committed .o/.go (usid.c had changed but they hadn't been regenerated since), and once regenerated, Unit Tests (root) then hit the verifier rejection above on every FIB-lookup-path test. Set the license string to plain "GPL" -- not "Dual BSD/GPL"; this program isn't itself dual-licensed, so it declares the license it actually means rather than a disjunction it doesn't. Correct the surrounding comment accordingly: the ELF license section governs which helpers the verifier allows and is independent of (doesn't relicense) the file's own AGPL-3.0-or-later SPDX header, same as Cilium, Katran, and every other AGPL/Apache/BSD-licensed project embedding a BPF datapath. Regenerate usid_bpfel.o/usid_bpfeb.o and their Go bindings from the corrected usid.c (also picking up the vrf_value.dropped_packets field ef73ddf added, which needed the same regeneration its own commit message flagged as still pending). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
ecv reviewed PR #282 and raised six questions about the eBPF codegen toolchain (commit 1e4a2e1). This addresses each: - Pin the drift check's compiler (clang-18/llvm-18, explicit versioned packages) instead of the unversioned clang/llvm meta-packages, whose underlying version moves with runner-image bumps and would otherwise flip `git diff --exit-code` red on PRs that never touched usid.c. doc.go's go:generate directive drops the hardcoded `-cc clang` so bpf2go falls back to its own $BPF2GO_CC env override, letting CI pin the compiler without the directive itself needing to change. - Stop regenerating the eBPF object during the container build. The Dockerfile now ships the committed usid_bpfel.o/usid_bpfeb.o as-is (go:embed'd into the binary), so the object shipped in the image is bit-for-bit the one committed to git, reviewed in the PR, and verified against usid.c by CI's drift check -- not a separate build produced by the builder image's own clang. This also means the Dockerfile no longer needs a toolchain at all, which resolves the apt dependency-list mismatch between it and CI in the same move. - `build:ebpf` now warns and skips (building against whatever's already committed) instead of hard-failing when clang isn't installed, so contributors touching unrelated Go don't need the eBPF toolchain. CI's drift check remains the actual correctness gate regardless of what any individual contributor has installed locally. - CI's `build` job now installs linux-libc-dev explicitly alongside the pinned clang/llvm, rather than relying on it happening to be preinstalled on the runner image -- doc.go's -idirafter workaround exists specifically to find that package's headers. - `test-unit-root` is now in test-e2e's `needs`, so a failure there blocks the merge chain instead of being advisory-only. (Branch protection's required-status-checks list should also list it directly -- that's a repo setting, not something this commit can change.) - `test-unit-root` now runs scripts/ci.sh's new `unittest-root` case, which discovers requireRoot(t)-gated packages by grep instead of re-running the entire suite as root. Scopes the real-kernel run to just the packages that need it (today: internal/cni, internal/plumbing/ebpf/prog, internal/plumbing/loaddr, internal/vmtap) and picks up future root-gated packages automatically as later milestones add them. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
7ac4585 to
1d36cd8
Compare
ecv reviewed PR #282 and raised six questions about the eBPF codegen toolchain (commit 1e4a2e1). This addresses each: - Pin the drift check's compiler (clang-18/llvm-18, explicit versioned packages) instead of the unversioned clang/llvm meta-packages, whose underlying version moves with runner-image bumps and would otherwise flip `git diff --exit-code` red on PRs that never touched usid.c. doc.go's go:generate directive drops the hardcoded `-cc clang` so bpf2go falls back to its own $BPF2GO_CC env override, letting CI pin the compiler without the directive itself needing to change. - Stop regenerating the eBPF object during the container build. The Dockerfile now ships the committed usid_bpfel.o/usid_bpfeb.o as-is (go:embed'd into the binary), so the object shipped in the image is bit-for-bit the one committed to git, reviewed in the PR, and verified against usid.c by CI's drift check -- not a separate build produced by the builder image's own clang. This also means the Dockerfile no longer needs a toolchain at all, which resolves the apt dependency-list mismatch between it and CI in the same move. - `build:ebpf` now warns and skips (building against whatever's already committed) instead of hard-failing when clang isn't installed, so contributors touching unrelated Go don't need the eBPF toolchain. CI's drift check remains the actual correctness gate regardless of what any individual contributor has installed locally. - CI's `build` job now installs linux-libc-dev explicitly alongside the pinned clang/llvm, rather than relying on it happening to be preinstalled on the runner image -- doc.go's -idirafter workaround exists specifically to find that package's headers. - `test-unit-root` is now in test-e2e's `needs`, so a failure there blocks the merge chain instead of being advisory-only. (Branch protection's required-status-checks list should also list it directly -- that's a repo setting, not something this commit can change.) - `test-unit-root` now runs scripts/ci.sh's new `unittest-root` case, which discovers requireRoot(t)-gated packages by grep instead of re-running the entire suite as root. Scopes the real-kernel run to just the packages that need it (today: internal/cni, internal/plumbing/ebpf/prog, internal/plumbing/loaddr, internal/vmtap) and picks up future root-gated packages automatically as later milestones add them. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
a8f7afc to
0e45a3c
Compare
Teaches task build to regenerate the TC-BPF usid program's Go bindings (bpf2go via go generate, split into a new build:ebpf step ahead of build:binaries) and the CNI container image to do the same at image-build time, so clang/llvm is a build-time-only dependency and never ships in the runtime image. Adds a root-privileged rerun of the unit test suite in CI (test-unit-root) so tests gated on requireRoot(t) -- the BPF_PROG_TEST_RUN cases in usid_test.go, plus later root-gated tests -- actually execute instead of skipping, and a step that diffs the regenerated eBPF artifacts against the committed ones so usid.c and its compiled output can't silently drift apart. Stacked on #281 (adds the usid.c program this generates from). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
ecv reviewed PR #282 and raised six questions about the eBPF codegen toolchain (commit 1e4a2e1). This addresses each: - Pin the drift check's compiler (clang-18/llvm-18, explicit versioned packages) instead of the unversioned clang/llvm meta-packages, whose underlying version moves with runner-image bumps and would otherwise flip `git diff --exit-code` red on PRs that never touched usid.c. doc.go's go:generate directive drops the hardcoded `-cc clang` so bpf2go falls back to its own $BPF2GO_CC env override, letting CI pin the compiler without the directive itself needing to change. - Stop regenerating the eBPF object during the container build. The Dockerfile now ships the committed usid_bpfel.o/usid_bpfeb.o as-is (go:embed'd into the binary), so the object shipped in the image is bit-for-bit the one committed to git, reviewed in the PR, and verified against usid.c by CI's drift check -- not a separate build produced by the builder image's own clang. This also means the Dockerfile no longer needs a toolchain at all, which resolves the apt dependency-list mismatch between it and CI in the same move. - `build:ebpf` now warns and skips (building against whatever's already committed) instead of hard-failing when clang isn't installed, so contributors touching unrelated Go don't need the eBPF toolchain. CI's drift check remains the actual correctness gate regardless of what any individual contributor has installed locally. - CI's `build` job now installs linux-libc-dev explicitly alongside the pinned clang/llvm, rather than relying on it happening to be preinstalled on the runner image -- doc.go's -idirafter workaround exists specifically to find that package's headers. - `test-unit-root` is now in test-e2e's `needs`, so a failure there blocks the merge chain instead of being advisory-only. (Branch protection's required-status-checks list should also list it directly -- that's a repo setting, not something this commit can change.) - `test-unit-root` now runs scripts/ci.sh's new `unittest-root` case, which discovers requireRoot(t)-gated packages by grep instead of re-running the entire suite as root. Scopes the real-kernel run to just the packages that need it (today: internal/cni, internal/plumbing/ebpf/prog, internal/plumbing/loaddr, internal/vmtap) and picks up future root-gated packages automatically as later milestones add them. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The new test-unit-root CI job (this PR) was the first thing to ever actually run these requireRoot(t)-gated tests -- and several failed: TestCleanupContainerNetnsNonVeth, TestCmdDelFlushesGuestNetnsConfig, TestConfigureInterfaceInNetns*, TestFlushGuestNetnsConfig*. All of them go through createTestNetnsWithDummy, which used ns.TempNetNS(). Its Path() is /proc/<pid>/task/<tid>/ns/net -- valid only while that specific OS thread stays alive and stays in that netns. Every caller of the helper immediately hands netnsPath to production code (configureInterfaceInNetns, cleanupContainerNetns, flushGuestNetnsConfig) that reopens it independently via its own ns.GetNS(netnsPath) call, and by then nsObj.Do() has already returned and unlocked its OS thread, which the Go runtime is free to reuse for an unrelated goroutine in a different netns. Confirmed with a minimal repro: a second ns.GetNS(nsObj.Path()).Do() after the first Do() returns can no longer see interfaces created in the first call. Switch to testutils.NewNS() (github.com/containernetworking/plugins/ pkg/testutils), which bind-mounts the netns to a stable path under /var/run/netns instead of pinning it to one thread's lifetime -- the same repro passes cleanly with it. Also fixes TestCleanupContainerNetnsVeth's own separate ns.TempNetNS() usage, and removes its "peer survives in host namespace" assertions: netlink.Veth's LinkAdd creates both ends of a pair in whatever netns the call runs in (no automatic host/guest split without an explicit peer-namespace move, which this test never did), and a veth pair is one kernel object -- LinkDel on either end removes both, wherever they live -- so those assertions could never have passed and weren't testing anything cleanupContainerNetns actually does. This is a pre-existing bug on main, unrelated to the eBPF work in this PR stack; it surfaced here because test-unit-root is the first CI job to ever run these tests as root instead of skipping them. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
68ba958 to
5ae36ae
Compare
Adds the Go control-plane library for the TC-BPF uSID datapath: - preflight: kernel capability gate (BTF, HASH maps, SCHED_CLS, bpf_fib_lookup with VRF-tbid support) run before attempting to load the program at all. - usidmap: typed read/write/reconcile API for the three kernel maps (locator_table, function_table, vrf_table) the program consults. - attach: load/pin/attach/detach/watch lifecycle for the TC-BPF ingress hook, including netlink-driven re-attachment on interface or route change, gated by the new GALACTIC_CNI_EBPF_INTERFACES env var (internal/config) for multi-homed nodes where auto-detection is ambiguous. - metrics: Prometheus collector plus load/attach event counters. Every package here is independently unit-testable against fakes (faketable_test.go, a mock kernel prober, fake closers) -- nothing outside this tree calls any of it yet. Stacked on #281 (usid.c/uformat) and #282 (codegen toolchain). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
ecv
left a comment
There was a problem hiding this comment.
can you tech-debt ticket yourself to move the comments into documentation, AFTER merge?
Adds the Go control-plane library for the TC-BPF uSID datapath: - preflight: kernel capability gate (BTF, HASH maps, SCHED_CLS, bpf_fib_lookup with VRF-tbid support) run before attempting to load the program at all. - usidmap: typed read/write/reconcile API for the three kernel maps (locator_table, function_table, vrf_table) the program consults. - attach: load/pin/attach/detach/watch lifecycle for the TC-BPF ingress hook, including netlink-driven re-attachment on interface or route change, gated by the new GALACTIC_CNI_EBPF_INTERFACES env var (internal/config) for multi-homed nodes where auto-detection is ambiguous. - metrics: Prometheus collector plus load/attach event counters. Every package here is independently unit-testable against fakes (faketable_test.go, a mock kernel prober, fake closers) -- nothing outside this tree calls any of it yet. Stacked on #281 (usid.c/uformat) and #282 (codegen toolchain). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Summary
Second PR in the eBPF uSID datapath stack (on top of #281, which adds the
usid.cprogram this generates from).Teaches
task buildto regenerate the TC-BPF program's Go bindings viago generate(a newbuild:ebpfstep ahead ofbuild:binaries) and the CNI container image to do the same at image-build time —clang/llvmstays a build-time-only dependency and never ships in the runtime image.Also adds a root-privileged rerun of the unit test suite in CI (
test-unit-root): the existingtest-unitjob runs as an unprivileged user, so every test gated onrequireRoot(t)— including theBPF_PROG_TEST_RUNcases inusid_test.go— skips instead of running. And a step that diffs the regenerated eBPF artifacts against the committed ones, sousid.cand its compiled output can't silently drift apart.Note:
test-e2e's dependency list isn't updated to includetest-unit-rootyet — that lands in the deploy/e2e PR later in this stack, once there's an eBPF datapath for e2e to actually depend on.Test plan
go build ./...task --list(Taskfile parses,build:ebpftask registered)clangavailable in this local sandbox to runbuild:ebpfdirectly)Part of the eBPF uSID datapath cutover stack (base: #281).