From 17a229fc38e3199d69a823e3b7fe8d5af3ed997f Mon Sep 17 00:00:00 2001 From: terra tauri Date: Tue, 29 Sep 2026 17:17:57 -0700 Subject: [PATCH 1/3] chore: update Go to 1.27.1 and refresh dependencies go directive 1.26.4 -> 1.27.1; Docker builder images golang:1.26 -> golang:1.27. Direct deps: containernetworking/cni v1.3.1, ginkgo v2.33.0, gomega v1.44.0, x/sys v0.48.0 (yaml/v3 v3.0.5 indirect). golangci-lint v2.5.0 can't typecheck against 1.27 (its x/tools reads export data up to v2; 1.27 writes v4), so the pin moves to v2.14.0. That version flags three goconst/prealloc hits in perfrig tests; fixed with named constants for the fake substrate's call log and a preallocated slice. Claude-Session: https://claude.ai/code/session_01RyYnvjh5DBevutC4HTgELn --- .github/workflows/ci.yml | 2 +- Makefile | 12 ++++++------ deploy/docker/Dockerfile.cni | 2 +- deploy/docker/Dockerfile.perfclient | 2 +- go.mod | 12 ++++++------ go.sum | 22 ++++++++++------------ internal/perfrig/apply_test.go | 11 +++++++---- internal/perfrig/executor_test.go | 10 +++++----- internal/perfrig/substrate_fake.go | 13 ++++++++++--- scripts/run-in-docker.sh | 4 ++-- scripts/vm-rig/README.md | 2 +- 11 files changed, 50 insertions(+), 42 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 8d5e0d7..79aaf07 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -30,7 +30,7 @@ jobs: # `make pre-commit` is the target the .githooks/pre-commit hook # invokes locally. Running the same target here means anything # caught locally is caught here, and vice versa — no drift. The - # Makefile pins golangci-lint v2.5.0 (handles go.mod's 1.26 + # Makefile pins golangci-lint v2.14.0 (handles go.mod's 1.27 # requirement) and installs it into ./bin via go-install. - run: make pre-commit diff --git a/Makefile b/Makefile index 49332a4..5da5669 100644 --- a/Makefile +++ b/Makefile @@ -322,12 +322,12 @@ $(LOCALBIN): mkdir -p $(LOCALBIN) GOLANGCI_LINT = $(LOCALBIN)/golangci-lint -# v2.5.0 handles natra's go.mod (go 1.26). v2.3.0 was built against -# go 1.24 and rejects the config when run against newer-targeted code -# — see ci.yml for the same constraint. Newer v2.12.x enables stricter -# linters (goconst/prealloc) that would flag unrelated pre-existing -# test code; hold here until those are addressed deliberately. -GOLANGCI_LINT_VERSION ?= v2.5.0 +# golangci-lint must be built with a Go at least as new as go.mod's +# directive, and its bundled x/tools must read that Go's export data. +# v2.5.0 fails on go 1.27 ("export data version 4 is greater than +# maximum supported version 2"); v2.14.0 handles it. See ci.yml for +# the same constraint. +GOLANGCI_LINT_VERSION ?= v2.14.0 # golangci-lint refuses to run when built with a Go older than # go.mod's directive, so the cached binary is keyed on the Go version diff --git a/deploy/docker/Dockerfile.cni b/deploy/docker/Dockerfile.cni index ecab3ba..450404e 100644 --- a/deploy/docker/Dockerfile.cni +++ b/deploy/docker/Dockerfile.cni @@ -27,7 +27,7 @@ COPY Makefile Makefile RUN make build-bpf # ----- Stage 2: Go ----- -FROM golang:1.26 AS go-builder +FROM golang:1.27 AS go-builder WORKDIR /workspace diff --git a/deploy/docker/Dockerfile.perfclient b/deploy/docker/Dockerfile.perfclient index bdfda12..5c2fcf0 100644 --- a/deploy/docker/Dockerfile.perfclient +++ b/deploy/docker/Dockerfile.perfclient @@ -5,7 +5,7 @@ # # Multi-stage so the final image stays small and contains no Go # toolchain — just the two static binaries plus iperf3 from alpine. -FROM golang:1.26-alpine AS build +FROM golang:1.27-alpine AS build RUN go install github.com/rakyll/hey@latest FROM alpine:3.19 diff --git a/go.mod b/go.mod index cc837f6..bb3cff0 100644 --- a/go.mod +++ b/go.mod @@ -1,15 +1,15 @@ module github.com/terraboops/natra -go 1.26.4 +go 1.27.1 require ( github.com/cilium/ebpf v0.22.0 - github.com/containernetworking/cni v1.3.0 - github.com/onsi/ginkgo/v2 v2.32.0 - github.com/onsi/gomega v1.42.1 + github.com/containernetworking/cni v1.3.1 + github.com/onsi/ginkgo/v2 v2.33.0 + github.com/onsi/gomega v1.44.0 github.com/vishvananda/netlink v1.3.1 github.com/vishvananda/netns v0.0.5 - golang.org/x/sys v0.46.0 + golang.org/x/sys v0.48.0 ) require ( @@ -18,7 +18,7 @@ require ( github.com/go-task/slim-sprig/v3 v3.0.0 // indirect github.com/google/go-cmp v0.7.0 // indirect github.com/google/pprof v0.0.0-20260604005048-7023385849c0 // indirect - go.yaml.in/yaml/v3 v3.0.4 // indirect + go.yaml.in/yaml/v3 v3.0.5 // indirect golang.org/x/mod v0.37.0 // indirect golang.org/x/net v0.56.0 // indirect golang.org/x/sync v0.21.0 // indirect diff --git a/go.sum b/go.sum index d285f8c..ddec603 100644 --- a/go.sum +++ b/go.sum @@ -2,8 +2,8 @@ github.com/Masterminds/semver/v3 v3.5.0 h1:kQceYJfbupGfZOKZQg0kou0DgAKhzDg2NZPAw github.com/Masterminds/semver/v3 v3.5.0/go.mod h1:4V+yj/TJE1HU9XfppCwVMZq3I84lprf4nC11bSS5beM= github.com/cilium/ebpf v0.22.0 h1:v2ktp0roffpMOj2MMf3idtCQZOsAoC4BJbAJN+ke2bY= github.com/cilium/ebpf v0.22.0/go.mod h1:CDzZbe2hC5JjlDC+CY3KFCzlYwN4gbxppYM+Z10bQt4= -github.com/containernetworking/cni v1.3.0 h1:v6EpN8RznAZj9765HhXQrtXgX+ECGebEYEmnuFjskwo= -github.com/containernetworking/cni v1.3.0/go.mod h1:Bs8glZjjFfGPHMw6hQu82RUgEPNGEaBb9KS5KtNMnJ4= +github.com/containernetworking/cni v1.3.1 h1:gnHlU/YC1bJcS+CSh+pYQKeuu4O4gsMGW1NMaLhkzW4= +github.com/containernetworking/cni v1.3.1/go.mod h1:OtVXL0yXMfQb+p93OC/vRuhjpxsO1gAV0smjt28UQUQ= github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc h1:U9qPSI2PIWSS1VwoXQT9A3Wy9MM3WgvqSxFWenqJduM= github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/gkampitakis/ciinfo v0.3.2 h1:JcuOPk8ZU7nZQjdUhctuhQofk7BGHuIy0c9Ez8BNhXs= @@ -42,10 +42,10 @@ github.com/mdlayher/socket v0.5.1 h1:VZaqt6RkGkt2OE9l3GcC6nZkqD3xKeQLyfleW/uBcos github.com/mdlayher/socket v0.5.1/go.mod h1:TjPLHI1UgwEv5J1B5q0zTZq12A/6H7nKmtTanQE37IQ= github.com/mfridman/tparse v0.18.0 h1:wh6dzOKaIwkUGyKgOntDW4liXSo37qg5AXbIhkMV3vE= github.com/mfridman/tparse v0.18.0/go.mod h1:gEvqZTuCgEhPbYk/2lS3Kcxg1GmTxxU7kTC8DvP0i/A= -github.com/onsi/ginkgo/v2 v2.32.0 h1:Hw7s2pVrQo/8Yz5N77qdnpHaoc+c6cC9WIV1Jce+J6E= -github.com/onsi/ginkgo/v2 v2.32.0/go.mod h1:+aXOY+vzZ5mu2iI2HpTZUPmM//oQfsNFX6gU9kNcA44= -github.com/onsi/gomega v1.42.1 h1:iN1rCUX+44NZ1Dc97MPoeFYbFR0vh8zxoxMFwKdyZ6I= -github.com/onsi/gomega v1.42.1/go.mod h1:REff/hsDsodHoKlWsP2mAPhu1+5/6hVYNf9rIEBpeSg= +github.com/onsi/ginkgo/v2 v2.33.0 h1:C8gBA6Uc2ZEubiV+SXiu5tZnMTwEmXHgkJwGozKtZf8= +github.com/onsi/ginkgo/v2 v2.33.0/go.mod h1:+aXOY+vzZ5mu2iI2HpTZUPmM//oQfsNFX6gU9kNcA44= +github.com/onsi/gomega v1.44.0 h1:eAiGl3Pw5jz5GQdDff0BcxYpAX1JxW8xD7mFUuwNfZQ= +github.com/onsi/gomega v1.44.0/go.mod h1:e/C2HwaZ1DhvjzXXuFhcR7hY7Sh9pl7MmoWKEjzwcdA= github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2 h1:Jamvg5psRIccs7FGNTlIRMkT8wgtp5eCXdBlqhYGL6U= github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= github.com/rogpeppe/go-internal v1.14.1 h1:UQB4HGPB6osV0SQTLymcB4TgvyWu6ZyliaW0tI/otEQ= @@ -64,8 +64,8 @@ github.com/vishvananda/netlink v1.3.1 h1:3AEMt62VKqz90r0tmNhog0r/PpWKmrEShJU0wJW github.com/vishvananda/netlink v1.3.1/go.mod h1:ARtKouGSTGchR8aMwmkzC0qiNPrrWO5JS/XMVl45+b4= github.com/vishvananda/netns v0.0.5 h1:DfiHV+j8bA32MFM7bfEunvT8IAqQ/NzSJHtcmW5zdEY= github.com/vishvananda/netns v0.0.5/go.mod h1:SpkAiCQRtJ6TvvxPnOSyH3BMl6unz3xZlaprSwhNNJM= -go.yaml.in/yaml/v3 v3.0.4 h1:tfq32ie2Jv2UxXFdLJdh3jXuOzWiL1fo0bu/FbuKpbc= -go.yaml.in/yaml/v3 v3.0.4/go.mod h1:DhzuOOF2ATzADvBadXxruRBLzYTpT36CKvDb3+aBEFg= +go.yaml.in/yaml/v3 v3.0.5 h1:N6y/pJk8buWs9NY5ERU2HSMfm+IuD/OtfdAnq6kESPw= +go.yaml.in/yaml/v3 v3.0.5/go.mod h1:HVTZu1O7/Vkt2N+BFy8Zza+lnLsABggaTM2ZpNIGuKg= golang.org/x/mod v0.37.0 h1:vF1DjpVEshcIqoEaauuHebaLk1O1forxjxBaVn884JQ= golang.org/x/mod v0.37.0/go.mod h1:m8S8VeM9r4dzDwjrKO0a1sZP3YjeMamRRlD+fmR2Q/0= golang.org/x/net v0.56.0 h1:Rw8j/hFzGvJUZwNBXnAtf5sVDVt+65SK2C7IxCxZt5o= @@ -74,15 +74,13 @@ golang.org/x/sync v0.21.0 h1:HLII4xRRTtCRkxYp4HNFF0Js/Og6q2i++KXbg0gHCwM= golang.org/x/sync v0.21.0/go.mod h1:9xrNwdLfx4jkKbNva9FpL6vEN7evnE43NNNJQ2LF3+0= golang.org/x/sys v0.2.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.10.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= -golang.org/x/sys v0.46.0 h1:noSf2Fq6F8DBgS+LysIkx7rIExoNHJsxOAtPp4rthXw= -golang.org/x/sys v0.46.0/go.mod h1:4GL1E5IUh+htKOUEOaiffhrAeqysfVGipDYzABqnCmw= +golang.org/x/sys v0.48.0 h1:bbX/i/6MgT9BVLM9RT1thmxL04yeTAhbEz4SyadbXoo= +golang.org/x/sys v0.48.0/go.mod h1:hNLxWAXmnKAxqDtdwIYC4bM9oQPEecfsnNMuSxOs3og= golang.org/x/text v0.38.0 h1:sXmwo9DwP3OK9EZ7PqAdaooSGozfl/3a6/xJcbzPRhE= golang.org/x/text v0.38.0/go.mod h1:YXZt3QhHUKYT53r2lLKFIVi6Ao1jdzrTR/KQ09qyxF4= golang.org/x/tools v0.47.0 h1:7Kn5x/d1svx/PzryTsqeoZN4TZwqeH5pGWjefhLi/1Q= golang.org/x/tools v0.47.0/go.mod h1:dFHnyTvFWY212G+h7ZY4Vsp/K3U4/7W9TyVaAul8uCA= google.golang.org/protobuf v1.36.7 h1:IgrO7UwFQGJdRNXH/sQux4R1Dj1WAKcLElzeeRaXV2A= google.golang.org/protobuf v1.36.7/go.mod h1:jduwjTPXsFjZGTmRluh+L6NjiWu7pchiJ2/5YcXBHnY= -gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM= -gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA= gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= diff --git a/internal/perfrig/apply_test.go b/internal/perfrig/apply_test.go index 69b2830..8859243 100644 --- a/internal/perfrig/apply_test.go +++ b/internal/perfrig/apply_test.go @@ -63,13 +63,16 @@ func TestApply_CISubsetOfFull(t *testing.T) { } } +// badProfileName names the invalid profiles the rejection tests build. +const badProfileName = "bad" + // TestApply_RejectsRateOutsideSpec — a profile that names a rate // the spec doesn't have should fail at Apply time, not at run time. // This catches drift like "someone added Rate100G to ProfileCI but // forgot DefaultSpec". func TestApply_RejectsRateOutsideSpec(t *testing.T) { bad := Profile{ - Name: "bad", + Name: badProfileName, Rates: []Rate{Rate10M, "100G"}, // 100G not in DefaultSpec Workloads: []WorkloadKind{WorkloadIperfSweep}, Samples: 1, @@ -87,7 +90,7 @@ func TestApply_RejectsRateOutsideSpec(t *testing.T) { // for workload kinds. func TestApply_RejectsWorkloadOutsideSpec(t *testing.T) { bad := Profile{ - Name: "bad", + Name: badProfileName, Rates: []Rate{Rate10M}, Workloads: []WorkloadKind{"someFutureWorkload"}, Samples: 1, @@ -105,7 +108,7 @@ func TestApply_RejectsWorkloadOutsideSpec(t *testing.T) { // sample count beyond the spec's maximum. func TestApply_RejectsSamplesOverSpec(t *testing.T) { bad := Profile{ - Name: "bad", + Name: badProfileName, Rates: []Rate{Rate10M}, Workloads: []WorkloadKind{WorkloadIperfSweep}, Samples: DefaultSpec.Samples + 1, @@ -124,7 +127,7 @@ func TestApply_RejectsSamplesOverSpec(t *testing.T) { // error rather than emitting an empty report. func TestApply_RejectsZeroSamples(t *testing.T) { bad := Profile{ - Name: "bad", + Name: badProfileName, Rates: []Rate{Rate10M}, Workloads: []WorkloadKind{WorkloadIperfSweep}, Samples: 0, diff --git a/internal/perfrig/executor_test.go b/internal/perfrig/executor_test.go index 1b8ec9b..8df54c3 100644 --- a/internal/perfrig/executor_test.go +++ b/internal/perfrig/executor_test.go @@ -53,11 +53,11 @@ func TestExecutor_FreshClusterPerPhase(t *testing.T) { var ups, downs, installs int for _, c := range fake.Calls { switch c { - case "Up": + case callUp: ups++ - case "Down": + case callDown: downs++ - case "InstallNatra": + case callInstallNatra: installs++ } } @@ -87,7 +87,7 @@ func TestExecutor_PhaseOrder(t *testing.T) { if err != nil { t.Fatalf("Run: %v", err) } - got := []Phase{} + got := make([]Phase, 0, len(rep.Phases)) for _, p := range rep.Phases { got = append(got, p.Phase) } @@ -144,7 +144,7 @@ func TestExecutor_DeferredDownOnError(t *testing.T) { // One opening Down, then a failed Up, then the deferred Down. // The executor returns from the first phase, so only that // phase's calls are present. - want := []string{"Down", "Up", "Down"} + want := []string{callDown, callUp, callDown} if len(fake.Calls) != len(want) { t.Fatalf("call sequence: got %v, want %v", fake.Calls, want) } diff --git a/internal/perfrig/substrate_fake.go b/internal/perfrig/substrate_fake.go index 19e031a..b941f7e 100644 --- a/internal/perfrig/substrate_fake.go +++ b/internal/perfrig/substrate_fake.go @@ -6,6 +6,13 @@ import ( "sync" ) +// Method names FakeSubstrate records in Calls. +const ( + callUp = "Up" + callDown = "Down" + callInstallNatra = "InstallNatra" +) + // FakeSubstrate is the unit-test stand-in. It records every method // call and returns canned outputs for NodeShell so executor logic // (phase loop, parsing of bpftool / meminfo / iperf output) is @@ -45,17 +52,17 @@ func (f *FakeSubstrate) record(call string) { } func (f *FakeSubstrate) Up(_ context.Context) error { - f.record("Up") + f.record(callUp) return f.UpErr } func (f *FakeSubstrate) Down(_ context.Context) error { - f.record("Down") + f.record(callDown) return f.DownErr } func (f *FakeSubstrate) InstallNatra(_ context.Context) error { - f.record("InstallNatra") + f.record(callInstallNatra) return f.InstallErr } diff --git a/scripts/run-in-docker.sh b/scripts/run-in-docker.sh index 34037ec..ce030b2 100755 --- a/scripts/run-in-docker.sh +++ b/scripts/run-in-docker.sh @@ -7,11 +7,11 @@ # scripts/run-in-docker.sh [args...] # # Environment: -# NATRA_DOCKER_IMAGE override the image (default: golang:1.26) +# NATRA_DOCKER_IMAGE override the image (default: golang:1.27) set -euo pipefail -IMAGE="${NATRA_DOCKER_IMAGE:-golang:1.26}" +IMAGE="${NATRA_DOCKER_IMAGE:-golang:1.27}" REPO_ROOT="$(cd "$(dirname "$0")/.." && pwd)" # Missing or stopped Docker exits 69 (EX_UNAVAILABLE) rather than 0: diff --git a/scripts/vm-rig/README.md b/scripts/vm-rig/README.md index cf269c2..48c3c85 100644 --- a/scripts/vm-rig/README.md +++ b/scripts/vm-rig/README.md @@ -47,7 +47,7 @@ for what cloud-VM / metal would add on top. predates lima managing socket_vmnet itself. Linux doesn't need socket_vmnet — lima uses libvirt/KVM bridged networks directly. - `kubectl`, `docker` on PATH. -- Go 1.26+ (to run `cmd/vm-rig`; the binary is built on demand by +- Go 1.27+ (to run `cmd/vm-rig`; the binary is built on demand by `go run`, no separate install step). - ~6 GiB free disk per VM; ~30 GiB headroom overall is comfortable. From 016cc079a6f9dba79baa170ac14118c41db18818 Mon Sep 17 00:00:00 2001 From: terra tauri Date: Tue, 29 Sep 2026 20:31:22 -0700 Subject: [PATCH 2/3] cmd/natra: keep regexp and pprof out of the CNI binary Go 1.27 made the CNI plugin's peak RSS per invocation ~465 KB larger (4817 -> 5282 KB, n=30, CNI_COMMAND=VERSION). About 290 KB of that is encoding/json now being backed by json/v2; the rest is stdlib code growth. Nearly all of it is file-backed text/rodata: a CNI call pages in ~85% of the binary, so RSS tracks binary size. This takes out more than that: - internal/cniskel: libcni's skel v1.3.1 with its name validators reimplemented without regexp. cni/pkg/utils compiles a regexp at init, which the linker can't drop; removing it saves ~350 KB of RSS. Upstream's skel tests run against the copy, and names_test.go checks the validators against the upstream regexp (table + fuzz). The copy is excluded from goconst/gocyclo so it stays diffable against upstream. - cmd/natra-tools: install-cni-chain, dump-stats and profile move to a sibling binary. `natra ` execs it from the same directory, so the documented commands are unchanged. The installer DaemonSet and image ship both binaries. - The ADD-path heap-profile hook needs -tags heapprofile, so default builds don't link runtime/pprof. - cmd/natra/deps_test.go fails if regexp, runtime/pprof or libcni's skel/utils come back into the plugin. make test-unit and the CI unit job only covered ./pkg (and ./internal locally), so cmd/ and internal/ tests never ran in CI; both now run. Claude-Session: https://claude.ai/code/session_01RyYnvjh5DBevutC4HTgELn --- .github/workflows/unit.yml | 2 +- .golangci.yml | 6 + Makefile | 8 +- cmd/{natra => natra-tools}/dump_linux.go | 4 +- cmd/{natra => natra-tools}/dump_other.go | 0 cmd/{natra => natra-tools}/install.go | 0 cmd/natra-tools/main.go | 41 ++ cmd/{natra => natra-tools}/profile_linux.go | 4 +- cmd/{natra => natra-tools}/profile_other.go | 0 cmd/natra/deps_test.go | 38 + cmd/natra/heapprofile.go | 40 ++ cmd/natra/heapprofile_off.go | 6 + cmd/natra/main.go | 92 +-- deploy/cni-installer.yaml | 3 + deploy/docker/Dockerfile.cni | 7 +- docs/development.md | 6 +- docs/install.md | 6 +- internal/cniskel/names.go | 94 +++ internal/cniskel/names_test.go | 60 ++ internal/cniskel/skel.go | 443 ++++++++++++ internal/cniskel/skel_suite_test.go | 27 + internal/cniskel/skel_test.go | 733 ++++++++++++++++++++ pkg/bpf/pindir.go | 8 + 23 files changed, 1556 insertions(+), 72 deletions(-) rename cmd/{natra => natra-tools}/dump_linux.go (97%) rename cmd/{natra => natra-tools}/dump_other.go (100%) rename cmd/{natra => natra-tools}/install.go (100%) create mode 100644 cmd/natra-tools/main.go rename cmd/{natra => natra-tools}/profile_linux.go (98%) rename cmd/{natra => natra-tools}/profile_other.go (100%) create mode 100644 cmd/natra/deps_test.go create mode 100644 cmd/natra/heapprofile.go create mode 100644 cmd/natra/heapprofile_off.go create mode 100644 internal/cniskel/names.go create mode 100644 internal/cniskel/names_test.go create mode 100644 internal/cniskel/skel.go create mode 100644 internal/cniskel/skel_suite_test.go create mode 100644 internal/cniskel/skel_test.go create mode 100644 pkg/bpf/pindir.go diff --git a/.github/workflows/unit.yml b/.github/workflows/unit.yml index b15afc7..cd00a3a 100644 --- a/.github/workflows/unit.yml +++ b/.github/workflows/unit.yml @@ -19,7 +19,7 @@ jobs: go-version-file: go.mod - uses: ./.github/actions/build-bpf - name: Run unit tests - run: go test -race ./pkg/... -coverprofile cover.out + run: go test -race ./pkg/... ./internal/... ./cmd/... -coverprofile cover.out - name: Upload coverage uses: codecov/codecov-action@v4 if: always() diff --git a/.golangci.yml b/.golangci.yml index 689806b..2fde8a2 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -36,6 +36,12 @@ linters: - dupl - lll path: internal/* + # Verbatim copy of libcni's skel (see internal/cniskel/skel.go); + # kept unrestyled so it diffs cleanly against upstream. + - linters: + - goconst + - gocyclo + path: internal/cniskel/ paths: - third_party$ - builtin$ diff --git a/Makefile b/Makefile index 5da5669..74cb442 100644 --- a/Makefile +++ b/Makefile @@ -27,6 +27,7 @@ BPF_CLANG ?= clang endif CNI_BINARY := bin/natra +TOOLS_BINARY := bin/natra-tools BPF_OBJS := bpf/natra.bpf.o bpf/placeholder.bpf.o bpf/vanilla.bpf.o # Intentionally-invalid programs used by the L3 chaos suite. They MUST # build (clang accepts them) but FAIL to load (verifier rejects). Listed @@ -137,7 +138,8 @@ build-cni-inner: @mkdir -p bin pkg/bpf @cp bpf/natra.bpf.o pkg/bpf/natra.bpf.o @GOOS=linux CGO_ENABLED=0 go build -buildvcs=false -o $(CNI_BINARY) ./cmd/natra - @echo "Built $(CNI_BINARY) ($$(file $(CNI_BINARY) | cut -d',' -f2))" + @GOOS=linux CGO_ENABLED=0 go build -buildvcs=false -o $(TOOLS_BINARY) ./cmd/natra-tools + @echo "Built $(CNI_BINARY) ($$(file $(CNI_BINARY) | cut -d',' -f2)) and $(TOOLS_BINARY)" .PHONY: build-bpf build-bpf: $(BPF_OBJS) $(BPF_TESTDATA_OBJS) ## Compile BPF C sources (incl. chaos testdata) to bytecode (.o). @@ -177,7 +179,7 @@ bpf/%.bpf.o: bpf/%.bpf.c .PHONY: clean clean: ## Clean build artifacts. - rm -f $(CNI_BINARY) $(BPF_OBJS) $(BPF_TESTDATA_OBJS) cover.out + rm -f $(CNI_BINARY) $(TOOLS_BINARY) $(BPF_OBJS) $(BPF_TESTDATA_OBJS) cover.out ##@ Testing @@ -186,7 +188,7 @@ test: test-unit ## Default test target — Layer 1 only (fast). .PHONY: test-unit test-unit: ## Layer 1a — Ginkgo unit tests. - go test ./pkg/... ./internal/... -coverprofile cover.out + go test ./pkg/... ./internal/... ./cmd/... -coverprofile cover.out .PHONY: test-fuzz test-fuzz: ## Layer 1b — 30s fuzz against the bandwidth annotation parser. diff --git a/cmd/natra/dump_linux.go b/cmd/natra-tools/dump_linux.go similarity index 97% rename from cmd/natra/dump_linux.go rename to cmd/natra-tools/dump_linux.go index c34054c..0219686 100644 --- a/cmd/natra/dump_linux.go +++ b/cmd/natra-tools/dump_linux.go @@ -40,7 +40,7 @@ func dumpStats(args []string) error { } open := func(name string) (*ebpf.Map, error) { - path := filepath.Join(pinDir, containerID+"-"+name+"-map") + path := filepath.Join(bpf.PinDir, containerID+"-"+name+"-map") return ebpf.LoadPinnedMap(path, nil) } @@ -151,7 +151,7 @@ func dumpStats(args []string) error { } // Self-check: pin path stats can drift from /sys/fs/bpf/natra layout. - if entries, err := os.ReadDir(pinDir); err == nil { + if entries, err := os.ReadDir(bpf.PinDir); err == nil { var pinned []string for _, e := range entries { if strings.HasPrefix(e.Name(), containerID) { diff --git a/cmd/natra/dump_other.go b/cmd/natra-tools/dump_other.go similarity index 100% rename from cmd/natra/dump_other.go rename to cmd/natra-tools/dump_other.go diff --git a/cmd/natra/install.go b/cmd/natra-tools/install.go similarity index 100% rename from cmd/natra/install.go rename to cmd/natra-tools/install.go diff --git a/cmd/natra-tools/main.go b/cmd/natra-tools/main.go new file mode 100644 index 0000000..6170f87 --- /dev/null +++ b/cmd/natra-tools/main.go @@ -0,0 +1,41 @@ +// natra-tools holds natra's operator subcommands. They live outside +// the CNI binary so the per-pod `natra` invocation doesn't page in +// their code (runtime/pprof alone is ~100 KB of peak RSS). `natra +// ` execs this binary from the same directory, so the +// documented commands are unchanged. +package main + +import ( + "fmt" + "os" +) + +const usage = `usage: natra-tools [args] + +commands: + install-cni-chain chain natra into existing conflists + dump-stats print a pod's pinned BPF stats + profile [flags] sample BPF program runtime stats` + +func main() { + if len(os.Args) < 2 { + fmt.Fprintln(os.Stderr, usage) + os.Exit(64) + } + var err error + switch os.Args[1] { + case "install-cni-chain": + err = installCNIChain(os.Args[2:]) + case "dump-stats": + err = dumpStats(os.Args[2:]) + case "profile": + err = profileCmd(os.Args[2:]) + default: + fmt.Fprintf(os.Stderr, "natra-tools: unknown command %q\n\n%s\n", os.Args[1], usage) + os.Exit(64) + } + if err != nil { + fmt.Fprintln(os.Stderr, err) + os.Exit(1) + } +} diff --git a/cmd/natra/profile_linux.go b/cmd/natra-tools/profile_linux.go similarity index 98% rename from cmd/natra/profile_linux.go rename to cmd/natra-tools/profile_linux.go index e004b3b..fe16a26 100644 --- a/cmd/natra/profile_linux.go +++ b/cmd/natra-tools/profile_linux.go @@ -38,6 +38,8 @@ import ( "github.com/cilium/ebpf" "golang.org/x/sys/unix" + + "github.com/terraboops/natra/pkg/bpf" ) // progSnapshot is a per-program datum at a single tick. Runtime/RunCount @@ -92,7 +94,7 @@ func profileCmd(args []string) error { interval := fs.Duration("interval", 5*time.Second, "snapshot interval") output := fs.String("output", "", "JSONL output file (default stdout)") heapDir := fs.String("heap-dir", "", "if set, write a heap pprof of this process per tick") - pinPath := fs.String("pin-dir", pinDir, "bpffs directory where natra pins maps") + pinPath := fs.String("pin-dir", bpf.PinDir, "bpffs directory where natra pins maps") once := fs.Bool("once", false, "take a single snapshot and exit (foreground use; soak rig)") if err := fs.Parse(args); err != nil { return err diff --git a/cmd/natra/profile_other.go b/cmd/natra-tools/profile_other.go similarity index 100% rename from cmd/natra/profile_other.go rename to cmd/natra-tools/profile_other.go diff --git a/cmd/natra/deps_test.go b/cmd/natra/deps_test.go new file mode 100644 index 0000000..b17afef --- /dev/null +++ b/cmd/natra/deps_test.go @@ -0,0 +1,38 @@ +package main + +import ( + "os" + "os/exec" + "strings" + "testing" +) + +// The CNI binary runs once per pod sandbox, and its peak RSS scales +// with how much of the binary gets paged in. These packages cost +// hundreds of KB each without being needed on the CNI path: regexp +// (compiled at init by cni/pkg/utils, hence internal/cniskel), +// runtime/pprof (natra-tools, or -tags heapprofile). +var forbiddenInPlugin = []string{ + "regexp", + "runtime/pprof", + "github.com/containernetworking/cni/pkg/skel", + "github.com/containernetworking/cni/pkg/utils", +} + +func TestPluginDoesNotLinkHeavyPackages(t *testing.T) { + cmd := exec.Command("go", "list", "-deps", ".") + cmd.Env = append(os.Environ(), "GOOS=linux", "CGO_ENABLED=0") + out, err := cmd.Output() + if err != nil { + t.Skipf("go list unavailable: %v", err) + } + deps := map[string]bool{} + for _, p := range strings.Fields(string(out)) { + deps[p] = true + } + for _, p := range forbiddenInPlugin { + if deps[p] { + t.Errorf("cmd/natra links %s; keep it out of the CNI binary (see deps_test.go)", p) + } + } +} diff --git a/cmd/natra/heapprofile.go b/cmd/natra/heapprofile.go new file mode 100644 index 0000000..c400bf5 --- /dev/null +++ b/cmd/natra/heapprofile.go @@ -0,0 +1,40 @@ +//go:build heapprofile + +package main + +import ( + "fmt" + "os" + "path/filepath" + "runtime" + "runtime/pprof" + "time" +) + +// maybeWriteHeapProfile dumps the Go heap profile of the natra CNI +// process at end of cmdAdd when NATRA_HEAP_PROFILE_DIR is set in the +// environment. Only in builds with `-tags heapprofile`: linking +// runtime/pprof into every CNI invocation costs ~100 KB of peak RSS. Files land at /cmdadd--.pprof, +// one per invocation — kubelet typically calls a CNI plugin once per +// pod sandbox, so the file count grows with pod churn. +// +// Aggregated across many ADDs during the perf-vs-vanilla rig, the +// profile shows what natra allocates per-invocation; useful for +// catching allocator regressions in the ADD hot path. +func maybeWriteHeapProfile(containerID string) { + dir := os.Getenv("NATRA_HEAP_PROFILE_DIR") + if dir == "" { + return + } + if err := os.MkdirAll(dir, 0o755); err != nil { + return + } + runtime.GC() + path := filepath.Join(dir, fmt.Sprintf("cmdadd-%d-%s.pprof", time.Now().UnixNano(), containerID)) + f, err := os.Create(path) + if err != nil { + return + } + defer func() { _ = f.Close() }() + _ = pprof.WriteHeapProfile(f) +} diff --git a/cmd/natra/heapprofile_off.go b/cmd/natra/heapprofile_off.go new file mode 100644 index 0000000..af42b7a --- /dev/null +++ b/cmd/natra/heapprofile_off.go @@ -0,0 +1,6 @@ +//go:build !heapprofile + +package main + +// maybeWriteHeapProfile is a no-op unless built with -tags heapprofile. +func maybeWriteHeapProfile(string) {} diff --git a/cmd/natra/main.go b/cmd/natra/main.go index b299cc9..fd83b1a 100644 --- a/cmd/natra/main.go +++ b/cmd/natra/main.go @@ -8,24 +8,20 @@ import ( "os" "path/filepath" "runtime" - "runtime/pprof" "strings" - "time" + "syscall" - "github.com/containernetworking/cni/pkg/skel" "github.com/containernetworking/cni/pkg/types" current "github.com/containernetworking/cni/pkg/types/100" "github.com/containernetworking/cni/pkg/version" + "github.com/terraboops/natra/internal/cniskel" "github.com/terraboops/natra/pkg/bpf" "github.com/terraboops/natra/pkg/cni/config" ) -// pinDir holds the bpffs paths for natra's per-pod tcx-link pins and -// per-pod map pins. A dedicated subdir keeps natra's pins separate from -// other tooling on the node and makes cleanup straightforward (cmdDel -// removes the per-container files). -const pinDir = "/sys/fs/bpf/natra" +// pinDir is where the plugin pins per-pod links and maps. +const pinDir = bpf.PinDir // pinPathFor is the bpffs path for the pinned tcx link of a given // container's veth in a given (side, direction). Side is embedded @@ -217,34 +213,17 @@ func resolveAttachStrategy(s string, edt edtMode) ([]attachAttempt, error) { func main() { // natra is normally invoked by kubelet via the CNI ABI (no CLI - // args; stdin + env vars). The exceptions are subcommands the - // DaemonSet uses: `install-cni-chain` patches existing conflists - // to chain natra in, and `dump-stats` reads the pinned maps for - // a given containerID. - if len(os.Args) > 1 && os.Args[1] == "install-cni-chain" { - if err := installCNIChain(os.Args[2:]); err != nil { - fmt.Fprintln(os.Stderr, err) - os.Exit(1) + // args; stdin + env vars). Operator subcommands live in + // natra-tools, installed next to this binary. + if len(os.Args) > 1 { + switch os.Args[1] { + case "install-cni-chain", "dump-stats", "profile": + execTools(os.Args[1:]) } - return - } - if len(os.Args) > 1 && os.Args[1] == "dump-stats" { - if err := dumpStats(os.Args[2:]); err != nil { - fmt.Fprintln(os.Stderr, err) - os.Exit(1) - } - return - } - if len(os.Args) > 1 && os.Args[1] == "profile" { - if err := profileCmd(os.Args[2:]); err != nil { - fmt.Fprintln(os.Stderr, err) - os.Exit(1) - } - return } - skel.PluginMainFuncs( - skel.CNIFuncs{ + cniskel.PluginMainFuncs( + cniskel.CNIFuncs{ Add: cmdAdd, Del: cmdDel, Check: cmdCheck, @@ -273,7 +252,7 @@ func main() { // on plugin error. We also append every log line to /var/log/natra-cni.log // so successful invocations leave a trace — useful for "is natra even // being called?" without cranking up runtime log levels. -func cmdAdd(args *skel.CmdArgs) error { +func cmdAdd(args *cniskel.CmdArgs) error { defer maybeWriteHeapProfile(args.ContainerID) logf("ADD containerID=%s netns=%s ifname=%s", args.ContainerID, args.Netns, args.IfName) logCaps() @@ -366,7 +345,7 @@ func logCaps() { // prefix. CNI DEL is idempotent — missing files are not errors. We // don't bother distinguishing modes because both branches end at the // same result: the container's pins are gone. -func cmdDel(args *skel.CmdArgs) error { +func cmdDel(args *cniskel.CmdArgs) error { prefix := args.ContainerID + "-" entries, err := os.ReadDir(pinDir) if err != nil { @@ -392,7 +371,7 @@ func cmdDel(args *skel.CmdArgs) error { // clsact filter is attached would require listing tc filters per // ifindex; kubelet uses CHECK as a liveness hint and a false positive // is no worse than the fail-open path elsewhere. -func cmdCheck(*skel.CmdArgs) error { +func cmdCheck(*cniskel.CmdArgs) error { return nil } @@ -565,7 +544,7 @@ func resolveHHThreshold(conf *NetConf, rateBps int64) int64 { // For HookClsact, the kernel holds the program reference via the // qdisc tree. func attachBPF( - args *skel.CmdArgs, + args *cniskel.CmdArgs, ingressCfg, egressCfg *config.Config, strategy []attachAttempt, edt edtMode, @@ -617,7 +596,7 @@ func attachBPF( // our skb->tstamp is ignored by noqueue and the rate limit silently // passes packets at line rate. func tryAttachOne( - args *skel.CmdArgs, + args *cniskel.CmdArgs, ingressCfg, egressCfg *config.Config, attempt attachAttempt, edt edtMode, @@ -746,31 +725,22 @@ func resolveIfIndex(netnsPath, ifName string, side bpf.Side) (int, func(), error } } -// maybeWriteHeapProfile dumps the Go heap profile of the natra CNI -// process at end of cmdAdd when NATRA_HEAP_PROFILE_DIR is set in the -// environment. Files land at /cmdadd--.pprof, -// one per invocation — kubelet typically calls a CNI plugin once per -// pod sandbox, so the file count grows with pod churn. -// -// Aggregated across many ADDs during the perf-vs-vanilla rig, the -// profile shows what natra allocates per-invocation; useful for -// catching allocator regressions in the ADD hot path. -func maybeWriteHeapProfile(containerID string) { - dir := os.Getenv("NATRA_HEAP_PROFILE_DIR") - if dir == "" { - return - } - if err := os.MkdirAll(dir, 0o755); err != nil { - return +// execTools replaces this process with natra-tools from the same +// directory, so `natra dump-stats` and friends keep working while +// their code stays out of the CNI binary. +func execTools(args []string) { + self, err := os.Executable() + if err == nil { + self, err = filepath.EvalSymlinks(self) } - runtime.GC() - path := filepath.Join(dir, fmt.Sprintf("cmdadd-%d-%s.pprof", time.Now().UnixNano(), containerID)) - f, err := os.Create(path) if err != nil { - return + fmt.Fprintf(os.Stderr, "natra: locate executable: %v\n", err) + os.Exit(1) } - defer func() { _ = f.Close() }() - _ = pprof.WriteHeapProfile(f) + tools := filepath.Join(filepath.Dir(self), "natra-tools") + err = syscall.Exec(tools, append([]string{tools}, args...), os.Environ()) + fmt.Fprintf(os.Stderr, "natra: %s is provided by natra-tools, expected at %s: %v\n", args[0], tools, err) + os.Exit(1) } // announce writes the "attached" line to stderr (kubelet captures it) @@ -784,7 +754,7 @@ func announce(ifIndex int, side bpf.Side, dir bpf.Direction, cfg *config.Config) side, dir, ifIndex, cfg.Rate, cfg.Burst, cfg.HeavyHitterThreshold) } -func passthrough(args *skel.CmdArgs, conf *NetConf) error { +func passthrough(args *cniskel.CmdArgs, conf *NetConf) error { // In a chained call kubelet writes the upstream plugin's Result // into stdin as `prevResult`. encoding/json fills conf.RawPrevResult // (a generic map), but conf.PrevResult only gets populated after diff --git a/deploy/cni-installer.yaml b/deploy/cni-installer.yaml index 0412bba..e47c774 100644 --- a/deploy/cni-installer.yaml +++ b/deploy/cni-installer.yaml @@ -129,6 +129,9 @@ spec: for dst in /host/opt/cni/bin /host/var-k3s-cni /host/bin; do if [ -d "$dst" ]; then install -m 0755 /usr/local/bin/natra "$dst/natra" + # Operator subcommands (dump-stats, profile); + # `natra ` execs it from the same dir. + install -m 0755 /usr/local/bin/natra-tools "$dst/natra-tools" if command -v setcap >/dev/null 2>&1; then setcap cap_bpf,cap_net_admin,cap_perfmon,cap_sys_resource+ep "$dst/natra" || true fi diff --git a/deploy/docker/Dockerfile.cni b/deploy/docker/Dockerfile.cni index 450404e..04e6e08 100644 --- a/deploy/docker/Dockerfile.cni +++ b/deploy/docker/Dockerfile.cni @@ -36,6 +36,7 @@ RUN go mod download COPY cmd/ cmd/ COPY pkg/ pkg/ +COPY internal/ internal/ # Bring the freshly-built BPF object into pkg/bpf/ where loader.go's # go:embed expects it. Architecturally important: the .bpf.o is @@ -44,7 +45,8 @@ COPY pkg/ pkg/ # silently embed the wrong build. COPY --from=bpf-builder /workspace/bpf/natra.bpf.o pkg/bpf/natra.bpf.o -RUN CGO_ENABLED=0 GOOS=linux go build -buildvcs=false -o bin/natra ./cmd/natra +RUN CGO_ENABLED=0 GOOS=linux go build -buildvcs=false -o bin/natra ./cmd/natra && \ + CGO_ENABLED=0 GOOS=linux go build -buildvcs=false -o bin/natra-tools ./cmd/natra-tools # ----- Stage 3: runtime ----- FROM alpine:latest @@ -54,6 +56,9 @@ FROM alpine:latest # CNI plugin can pin BPF links regardless of the caller's caps. RUN apk --no-cache add ca-certificates libcap +# natra-tools carries the operator subcommands; `natra ` +# execs it from the same directory. COPY --from=go-builder /workspace/bin/natra /usr/local/bin/natra +COPY --from=go-builder /workspace/bin/natra-tools /usr/local/bin/natra-tools ENTRYPOINT ["/usr/local/bin/natra"] diff --git a/docs/development.md b/docs/development.md index 5e74156..d6edbda 100644 --- a/docs/development.md +++ b/docs/development.md @@ -128,7 +128,11 @@ NATRA_E2E_ATTACH_MODE=clsact-podside make test-e2e # or tcx-podside, clsact-ho ## Layout ``` -cmd/natra/ CNI plugin entry point + install + dump-stats +cmd/natra/ CNI plugin entry point; `natra ` + execs natra-tools. deps_test.go keeps regexp + and runtime/pprof out of this binary. +cmd/natra-tools/ install-cni-chain, dump-stats, profile +internal/cniskel/ libcni's skel with a regexp-free name check cmd/perfrig/ k3d substrate frontend; invoked by make perf-vs-vanilla and the GH CI perf-vs-vanilla job. Lima vm-rig has its own entry under diff --git a/docs/install.md b/docs/install.md index 7f1ed7a..bcdaeb6 100644 --- a/docs/install.md +++ b/docs/install.md @@ -101,8 +101,10 @@ For one-off deployments, lab clusters, or a single node you're debugging on: ```bash -# Get the binary onto the node -scp natra :/opt/cni/bin/natra +# Get the binaries onto the node. natra-tools carries the operator +# subcommands (install-cni-chain, dump-stats, profile); `natra +# ` execs it from the same directory. +scp natra natra-tools :/opt/cni/bin/ # Set caps and chain natra into the existing CNI config ssh sudo bash -c ' diff --git a/internal/cniskel/names.go b/internal/cniskel/names.go new file mode 100644 index 0000000..4784734 --- /dev/null +++ b/internal/cniskel/names.go @@ -0,0 +1,94 @@ +// Copyright 2019 CNI authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +// Name validators from github.com/containernetworking/cni/pkg/utils +// v1.3.1. validName replaces utils' regexp +// `^[a-zA-Z0-9][a-zA-Z0-9_.\-]*$` with a byte loop; messages and +// error codes are unchanged. + +package cniskel + +import ( + "bytes" + "fmt" + "unicode" + + "github.com/containernetworking/cni/pkg/types" +) + +// maxInterfaceNameLength is the length max of a valid interface name +const maxInterfaceNameLength = 15 + +// validName matches `^[a-zA-Z0-9][a-zA-Z0-9_.\-]*$`. Like the regexp, +// it accepts the empty string; callers reject that separately. +func validName(s string) bool { + for i := 0; i < len(s); i++ { + c := s[i] + if c >= 'a' && c <= 'z' || c >= 'A' && c <= 'Z' || c >= '0' && c <= '9' { + continue + } + if i > 0 && (c == '_' || c == '.' || c == '-') { + continue + } + return false + } + return true +} + +// validateContainerID will validate that the supplied containerID is not empty does not contain invalid characters +func validateContainerID(containerID string) *types.Error { + if containerID == "" { + return types.NewError(types.ErrUnknownContainer, "missing containerID", "") + } + if !validName(containerID) { + return types.NewError(types.ErrInvalidEnvironmentVariables, "invalid characters in containerID", containerID) + } + return nil +} + +// validateNetworkName will validate that the supplied networkName does not contain invalid characters +func validateNetworkName(networkName string) *types.Error { + if networkName == "" { + return types.NewError(types.ErrInvalidNetworkConfig, "missing network name:", "") + } + if !validName(networkName) { + return types.NewError(types.ErrInvalidNetworkConfig, "invalid characters found in network name", networkName) + } + return nil +} + +// validateInterfaceName will validate the interface name based on the four rules below +// 1. The name must not be empty +// 2. The name must be less than 16 characters +// 3. The name must not be "." or ".." +// 4. The name must not contain / or : or any whitespace characters +// ref to https://github.com/torvalds/linux/blob/master/net/core/dev.c#L1024 +func validateInterfaceName(ifName string) *types.Error { + if len(ifName) == 0 { + return types.NewError(types.ErrInvalidEnvironmentVariables, "interface name is empty", "") + } + if len(ifName) > maxInterfaceNameLength { + return types.NewError(types.ErrInvalidEnvironmentVariables, "interface name is too long", fmt.Sprintf("interface name should be less than %d characters", maxInterfaceNameLength+1)) + } + if ifName == "." || ifName == ".." { + return types.NewError(types.ErrInvalidEnvironmentVariables, "interface name is . or ..", "") + } + for _, r := range bytes.Runes([]byte(ifName)) { + if r == '/' || r == ':' || unicode.IsSpace(r) { + return types.NewError(types.ErrInvalidEnvironmentVariables, "interface name contains / or : or whitespace characters", "") + } + } + + return nil +} diff --git a/internal/cniskel/names_test.go b/internal/cniskel/names_test.go new file mode 100644 index 0000000..80768f0 --- /dev/null +++ b/internal/cniskel/names_test.go @@ -0,0 +1,60 @@ +package cniskel + +import ( + "regexp" + "testing" + + "github.com/containernetworking/cni/pkg/types" + "github.com/containernetworking/cni/pkg/utils" +) + +// upstreamName is the regexp cni/pkg/utils uses. Tests may link +// regexp; only the plugin binary must not. +var upstreamName = regexp.MustCompile(`^[a-zA-Z0-9][a-zA-Z0-9_.\-]*$`) + +var nameCases = []string{ + "", "a", "A", "0", "_", ".", "-", "a_", "a.", "a-", "_a", ".a", "-a", + "abc123", "3f9a1c0e2b7d", "k8s_POD_default-1.2", "a b", "a/b", "a:b", + "a\x00", "é", "aé", "a\n", "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef", +} + +func TestValidNameMatchesUpstreamRegexp(t *testing.T) { + for _, s := range nameCases { + want := s == "" || upstreamName.MatchString(s) + if got := validName(s); got != want { + t.Errorf("validName(%q) = %v, upstream regexp says %v", s, got, want) + } + } +} + +// The three validators must return exactly what cni/pkg/utils does, +// code and message included. +func TestValidatorsMatchUpstream(t *testing.T) { + ifNames := append([]string{"eth0", "..", "0123456789abcdef", "a\tb", "ok:no"}, nameCases...) + for _, s := range nameCases { + assertSameError(t, "containerID", s, validateContainerID(s), utils.ValidateContainerID(s)) + assertSameError(t, "networkName", s, validateNetworkName(s), utils.ValidateNetworkName(s)) + } + for _, s := range ifNames { + assertSameError(t, "ifName", s, validateInterfaceName(s), utils.ValidateInterfaceName(s)) + } +} + +func assertSameError(t *testing.T, what, in string, got, want *types.Error) { + t.Helper() + if (got == nil) != (want == nil) || (got != nil && *got != *want) { + t.Errorf("%s(%q): got %v, upstream %v", what, in, got, want) + } +} + +func FuzzValidNameMatchesUpstreamRegexp(f *testing.F) { + for _, s := range nameCases { + f.Add(s) + } + f.Fuzz(func(t *testing.T, s string) { + want := s == "" || upstreamName.MatchString(s) + if got := validName(s); got != want { + t.Fatalf("validName(%q) = %v, upstream regexp says %v", s, got, want) + } + }) +} diff --git a/internal/cniskel/skel.go b/internal/cniskel/skel.go new file mode 100644 index 0000000..12b30c9 --- /dev/null +++ b/internal/cniskel/skel.go @@ -0,0 +1,443 @@ +// Copyright 2014-2016 CNI authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +// Package cniskel is github.com/containernetworking/cni/pkg/skel +// v1.3.1 with one change: the name validators it called from +// cni/pkg/utils are reimplemented in names.go without regexp. utils +// compiles a regexp at package init, which the linker can't drop, and +// that alone costs natra ~350 KB of peak RSS per CNI invocation. +// Behavior is otherwise identical; see names_test.go for the +// equivalence check against the upstream regexp. +package cniskel + +import ( + "bytes" + "encoding/json" + "errors" + "fmt" + "io" + "log" + "os" + "strings" + + "github.com/containernetworking/cni/pkg/ns" + "github.com/containernetworking/cni/pkg/types" + "github.com/containernetworking/cni/pkg/version" +) + +// CmdArgs captures all the arguments passed in to the plugin +// via both env vars and stdin +type CmdArgs struct { + ContainerID string + Netns string + IfName string + Args string + Path string + NetnsOverride string + StdinData []byte +} + +type dispatcher struct { + Getenv func(string) string + Stdin io.Reader + Stdout io.Writer + Stderr io.Writer + + ConfVersionDecoder version.ConfigDecoder + VersionReconciler version.Reconciler +} + +type reqForCmdEntry map[string]bool + +func (t *dispatcher) getCmdArgsFromEnv() (string, *CmdArgs, *types.Error) { + var cmd, contID, netns, ifName, args, path, netnsOverride string + + vars := []struct { + name string + val *string + reqForCmd reqForCmdEntry + validateFn func(string) *types.Error + }{ + { + "CNI_COMMAND", + &cmd, + reqForCmdEntry{ + "ADD": true, + "CHECK": true, + "DEL": true, + "GC": true, + "STATUS": true, + }, + nil, + }, + { + "CNI_CONTAINERID", + &contID, + reqForCmdEntry{ + "ADD": true, + "CHECK": true, + "DEL": true, + }, + validateContainerID, + }, + { + "CNI_NETNS", + &netns, + reqForCmdEntry{ + "ADD": true, + "CHECK": true, + "DEL": false, + }, + nil, + }, + { + "CNI_IFNAME", + &ifName, + reqForCmdEntry{ + "ADD": true, + "CHECK": true, + "DEL": true, + }, + validateInterfaceName, + }, + { + "CNI_ARGS", + &args, + reqForCmdEntry{ + "ADD": false, + "CHECK": false, + "DEL": false, + }, + nil, + }, + { + "CNI_PATH", + &path, + reqForCmdEntry{ + "ADD": true, + "CHECK": true, + "DEL": true, + "GC": true, + "STATUS": true, + }, + nil, + }, + { + "CNI_NETNS_OVERRIDE", + &netnsOverride, + reqForCmdEntry{ + "ADD": false, + "CHECK": false, + "DEL": false, + }, + nil, + }, + } + + argsMissing := make([]string, 0) + for _, v := range vars { + *v.val = t.Getenv(v.name) + if *v.val == "" { + if v.reqForCmd[cmd] || v.name == "CNI_COMMAND" { + argsMissing = append(argsMissing, v.name) + } + } else if v.reqForCmd[cmd] && v.validateFn != nil { + if err := v.validateFn(*v.val); err != nil { + return "", nil, err + } + } + } + + if len(argsMissing) > 0 { + joined := strings.Join(argsMissing, ",") + return "", nil, types.NewError(types.ErrInvalidEnvironmentVariables, fmt.Sprintf("required env variables [%s] missing", joined), "") + } + + if cmd == "VERSION" { + t.Stdin = bytes.NewReader(nil) + } + + stdinData, err := io.ReadAll(t.Stdin) + if err != nil { + return "", nil, types.NewError(types.ErrIOFailure, fmt.Sprintf("error reading from stdin: %v", err), "") + } + + if cmd != "VERSION" { + if err := validateConfig(stdinData); err != nil { + return "", nil, err + } + } + + cmdArgs := &CmdArgs{ + ContainerID: contID, + Netns: netns, + IfName: ifName, + Args: args, + Path: path, + StdinData: stdinData, + NetnsOverride: netnsOverride, + } + return cmd, cmdArgs, nil +} + +func (t *dispatcher) checkVersionAndCall(cmdArgs *CmdArgs, pluginVersionInfo version.PluginInfo, toCall func(*CmdArgs) error) *types.Error { + configVersion, err := t.ConfVersionDecoder.Decode(cmdArgs.StdinData) + if err != nil { + return types.NewError(types.ErrDecodingFailure, err.Error(), "") + } + verErr := t.VersionReconciler.Check(configVersion, pluginVersionInfo) + if verErr != nil { + return types.NewError(types.ErrIncompatibleCNIVersion, "incompatible CNI versions", verErr.Details()) + } + + if toCall == nil { + return nil + } + + if err = toCall(cmdArgs); err != nil { + var e *types.Error + if errors.As(err, &e) { + // don't wrap Error in Error + return e + } + return types.NewError(types.ErrInternal, err.Error(), "") + } + + return nil +} + +func validateConfig(jsonBytes []byte) *types.Error { + var conf struct { + Name string `json:"name"` + } + if err := json.Unmarshal(jsonBytes, &conf); err != nil { + return types.NewError(types.ErrDecodingFailure, fmt.Sprintf("error unmarshall network config: %v", err), "") + } + if conf.Name == "" { + return types.NewError(types.ErrInvalidNetworkConfig, "missing network name", "") + } + if err := validateNetworkName(conf.Name); err != nil { + return err + } + return nil +} + +func (t *dispatcher) pluginMain(funcs CNIFuncs, versionInfo version.PluginInfo, about string) *types.Error { + cmd, cmdArgs, err := t.getCmdArgsFromEnv() + if err != nil { + // Print the about string to stderr when no command is set + if err.Code == types.ErrInvalidEnvironmentVariables && t.Getenv("CNI_COMMAND") == "" && about != "" { + _, _ = fmt.Fprintln(t.Stderr, about) + _, _ = fmt.Fprintf(t.Stderr, "CNI protocol versions supported: %s\n", strings.Join(versionInfo.SupportedVersions(), ", ")) + return nil + } + return err + } + + switch cmd { + case "ADD": + err = t.checkVersionAndCall(cmdArgs, versionInfo, funcs.Add) + if err != nil { + return err + } + if strings.ToUpper(cmdArgs.NetnsOverride) != "TRUE" && cmdArgs.NetnsOverride != "1" { + isPluginNetNS, checkErr := ns.CheckNetNS(cmdArgs.Netns) + if checkErr != nil { + return checkErr + } else if isPluginNetNS { + return types.NewError(types.ErrInvalidNetNS, "plugin's netns and netns from CNI_NETNS should not be the same", "") + } + } + case "CHECK": + configVersion, err := t.ConfVersionDecoder.Decode(cmdArgs.StdinData) + if err != nil { + return types.NewError(types.ErrDecodingFailure, err.Error(), "") + } + if gtet, err := version.GreaterThanOrEqualTo(configVersion, "0.4.0"); err != nil { + return types.NewError(types.ErrDecodingFailure, err.Error(), "") + } else if !gtet { + return types.NewError(types.ErrIncompatibleCNIVersion, "config version does not allow CHECK", "") + } + for _, pluginVersion := range versionInfo.SupportedVersions() { + gtet, err := version.GreaterThanOrEqualTo(pluginVersion, configVersion) + if err != nil { + return types.NewError(types.ErrDecodingFailure, err.Error(), "") + } else if gtet { + if err := t.checkVersionAndCall(cmdArgs, versionInfo, funcs.Check); err != nil { + return err + } + return nil + } + } + return types.NewError(types.ErrIncompatibleCNIVersion, "plugin version does not allow CHECK", "") + case "DEL": + err = t.checkVersionAndCall(cmdArgs, versionInfo, funcs.Del) + if err != nil { + return err + } + if strings.ToUpper(cmdArgs.NetnsOverride) != "TRUE" && cmdArgs.NetnsOverride != "1" { + isPluginNetNS, checkErr := ns.CheckNetNS(cmdArgs.Netns) + if checkErr != nil { + return checkErr + } else if isPluginNetNS { + return types.NewError(types.ErrInvalidNetNS, "plugin's netns and netns from CNI_NETNS should not be the same", "") + } + } + case "GC": + configVersion, err := t.ConfVersionDecoder.Decode(cmdArgs.StdinData) + if err != nil { + return types.NewError(types.ErrDecodingFailure, err.Error(), "") + } + if gtet, err := version.GreaterThanOrEqualTo(configVersion, "1.1.0"); err != nil { + return types.NewError(types.ErrDecodingFailure, err.Error(), "") + } else if !gtet { + return types.NewError(types.ErrIncompatibleCNIVersion, "config version does not allow GC", "") + } + for _, pluginVersion := range versionInfo.SupportedVersions() { + gtet, err := version.GreaterThanOrEqualTo(pluginVersion, configVersion) + if err != nil { + return types.NewError(types.ErrDecodingFailure, err.Error(), "") + } else if gtet { + if err := t.checkVersionAndCall(cmdArgs, versionInfo, funcs.GC); err != nil { + return err + } + return nil + } + } + return types.NewError(types.ErrIncompatibleCNIVersion, "plugin version does not allow GC", "") + case "STATUS": + configVersion, err := t.ConfVersionDecoder.Decode(cmdArgs.StdinData) + if err != nil { + return types.NewError(types.ErrDecodingFailure, err.Error(), "") + } + if gtet, err := version.GreaterThanOrEqualTo(configVersion, "1.1.0"); err != nil { + return types.NewError(types.ErrDecodingFailure, err.Error(), "") + } else if !gtet { + return types.NewError(types.ErrIncompatibleCNIVersion, "config version does not allow STATUS", "") + } + for _, pluginVersion := range versionInfo.SupportedVersions() { + gtet, err := version.GreaterThanOrEqualTo(pluginVersion, configVersion) + if err != nil { + return types.NewError(types.ErrDecodingFailure, err.Error(), "") + } else if gtet { + if err := t.checkVersionAndCall(cmdArgs, versionInfo, funcs.Status); err != nil { + return err + } + return nil + } + } + return types.NewError(types.ErrIncompatibleCNIVersion, "plugin version does not allow STATUS", "") + case "VERSION": + if err := versionInfo.Encode(t.Stdout); err != nil { + return types.NewError(types.ErrIOFailure, err.Error(), "") + } + default: + return types.NewError(types.ErrInvalidEnvironmentVariables, fmt.Sprintf("unknown CNI_COMMAND: %v", cmd), "") + } + + return err +} + +// PluginMainWithError is the core "main" for a plugin. It accepts +// callback functions for add, check, and del CNI commands and returns an error. +// +// The caller must also specify what CNI spec versions the plugin supports. +// +// It is the responsibility of the caller to check for non-nil error return. +// +// For a plugin to comply with the CNI spec, it must print any error to stdout +// as JSON and then exit with nonzero status code. +// +// To let this package automatically handle errors and call os.Exit(1) for you, +// use PluginMain() instead. +// +// Deprecated: Use github.com/containernetworking/cni/pkg/skel.PluginMainFuncsWithError instead. +func PluginMainWithError(cmdAdd, cmdCheck, cmdDel func(_ *CmdArgs) error, versionInfo version.PluginInfo, about string) *types.Error { + return PluginMainFuncsWithError(CNIFuncs{Add: cmdAdd, Check: cmdCheck, Del: cmdDel}, versionInfo, about) +} + +// CNIFuncs contains a group of callback command funcs to be passed in as +// parameters to the core "main" for a plugin. +type CNIFuncs struct { + Add func(_ *CmdArgs) error + Del func(_ *CmdArgs) error + Check func(_ *CmdArgs) error + GC func(_ *CmdArgs) error + Status func(_ *CmdArgs) error +} + +// PluginMainFuncsWithError is the core "main" for a plugin. It accepts +// callback functions defined within CNIFuncs and returns an error. +// +// The caller must also specify what CNI spec versions the plugin supports. +// +// It is the responsibility of the caller to check for non-nil error return. +// +// For a plugin to comply with the CNI spec, it must print any error to stdout +// as JSON and then exit with nonzero status code. +// +// To let this package automatically handle errors and call os.Exit(1) for you, +// use PluginMainFuncs() instead. +func PluginMainFuncsWithError(funcs CNIFuncs, versionInfo version.PluginInfo, about string) *types.Error { + return (&dispatcher{ + Getenv: os.Getenv, + Stdin: os.Stdin, + Stdout: os.Stdout, + Stderr: os.Stderr, + }).pluginMain(funcs, versionInfo, about) +} + +// PluginMainFuncs is the core "main" for a plugin which includes automatic error handling. +// This is a newer alternative func to PluginMain which abstracts CNI commands within a +// CNIFuncs interface. +// +// The caller must also specify what CNI spec versions the plugin supports. +// +// The caller can specify an "about" string, which is printed on stderr +// when no CNI_COMMAND is specified. The recommended output is "CNI plugin v" +// +// When an error occurs in any func in CNIFuncs, PluginMainFuncs will print the error +// as JSON to stdout and call os.Exit(1). +// +// To have more control over error handling, use PluginMainFuncsWithError() instead. +func PluginMainFuncs(funcs CNIFuncs, versionInfo version.PluginInfo, about string) { + if e := PluginMainFuncsWithError(funcs, versionInfo, about); e != nil { + if err := e.Print(); err != nil { + log.Print("Error writing error JSON to stdout: ", err) + } + os.Exit(1) + } +} + +// PluginMain is the core "main" for a plugin which includes automatic error handling. +// +// The caller must also specify what CNI spec versions the plugin supports. +// +// The caller can specify an "about" string, which is printed on stderr +// when no CNI_COMMAND is specified. The recommended output is "CNI plugin v" +// +// When an error occurs in either cmdAdd, cmdCheck, or cmdDel, PluginMain will print the error +// as JSON to stdout and call os.Exit(1). +// +// To have more control over error handling, use PluginMainWithError() instead. +// +// Deprecated: Use github.com/containernetworking/cni/pkg/skel.PluginMainFuncs instead. +func PluginMain(cmdAdd, cmdCheck, cmdDel func(_ *CmdArgs) error, versionInfo version.PluginInfo, about string) { + if e := PluginMainWithError(cmdAdd, cmdCheck, cmdDel, versionInfo, about); e != nil { + if err := e.Print(); err != nil { + log.Print("Error writing error JSON to stdout: ", err) + } + os.Exit(1) + } +} diff --git a/internal/cniskel/skel_suite_test.go b/internal/cniskel/skel_suite_test.go new file mode 100644 index 0000000..d38214d --- /dev/null +++ b/internal/cniskel/skel_suite_test.go @@ -0,0 +1,27 @@ +// Copyright 2016 CNI authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package cniskel + +import ( + "testing" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +func TestSkel(t *testing.T) { + RegisterFailHandler(Fail) + RunSpecs(t, "Skel Suite") +} diff --git a/internal/cniskel/skel_test.go b/internal/cniskel/skel_test.go new file mode 100644 index 0000000..60ab7a3 --- /dev/null +++ b/internal/cniskel/skel_test.go @@ -0,0 +1,733 @@ +// Copyright 2016 CNI authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package cniskel + +import ( + "bytes" + "errors" + "fmt" + "strings" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + + "github.com/containernetworking/cni/pkg/types" + "github.com/containernetworking/cni/pkg/version" +) + +type fakeCmd struct { + CallCount int + Returns struct { + Error error + } + Received struct { + CmdArgs *CmdArgs + } +} + +func (c *fakeCmd) Func(args *CmdArgs) error { + c.CallCount++ + c.Received.CmdArgs = args + return c.Returns.Error +} + +var _ = Describe("dispatching to the correct callback", func() { + var ( + environment map[string]string + stdinData string + stdout, stderr *bytes.Buffer + cmdAdd, cmdCheck, cmdDel, cmdGC *fakeCmd + dispatch *dispatcher + expectedCmdArgs *CmdArgs + versionInfo version.PluginInfo + funcs CNIFuncs + ) + + BeforeEach(func() { + environment = map[string]string{ + "CNI_COMMAND": "ADD", + "CNI_CONTAINERID": "some-container-id", + "CNI_NETNS": "/some/netns/path", + "CNI_IFNAME": "eth0", + "CNI_ARGS": "some;extra;args", + "CNI_PATH": "/some/cni/path", + } + + stdinData = `{ "name":"skel-test", "some": "config", "cniVersion": "9.8.7" }` + stdout = &bytes.Buffer{} + stderr = &bytes.Buffer{} + versionInfo = version.PluginSupports("9.8.7", "10.0.0") + dispatch = &dispatcher{ + Getenv: func(key string) string { return environment[key] }, + Stdin: strings.NewReader(stdinData), + Stdout: stdout, + Stderr: stderr, + } + cmdAdd = &fakeCmd{} + cmdCheck = &fakeCmd{} + cmdDel = &fakeCmd{} + cmdGC = &fakeCmd{} + funcs = CNIFuncs{ + Add: cmdAdd.Func, + Del: cmdDel.Func, + Check: cmdCheck.Func, + GC: cmdGC.Func, + } + + expectedCmdArgs = &CmdArgs{ + ContainerID: "some-container-id", + Netns: "/some/netns/path", + IfName: "eth0", + Args: "some;extra;args", + Path: "/some/cni/path", + StdinData: []byte(stdinData), + } + }) + + envVarChecker := func(envVar string, isRequired bool) { + delete(environment, envVar) + + err := dispatch.pluginMain(funcs, versionInfo, "") + if isRequired { + Expect(err).To(Equal(&types.Error{ + Code: types.ErrInvalidEnvironmentVariables, + Msg: "required env variables [" + envVar + "] missing", + })) + } else { + Expect(err).NotTo(HaveOccurred()) + } + } + + Context("when the CNI_COMMAND is ADD", func() { + expectedCmdArgs = &CmdArgs{ + ContainerID: "some-container-id", + Netns: "/some/netns/path", + IfName: "eth0", + Args: "some;extra;args", + Path: "/some/cni/path", + StdinData: []byte(stdinData), + } + + It("extracts env vars and stdin data and calls cmdAdd", func() { + err := dispatch.pluginMain(funcs, versionInfo, "") + + Expect(err).NotTo(HaveOccurred()) + Expect(cmdAdd.CallCount).To(Equal(1)) + Expect(cmdCheck.CallCount).To(Equal(0)) + Expect(cmdDel.CallCount).To(Equal(0)) + Expect(cmdAdd.Received.CmdArgs).To(Equal(expectedCmdArgs)) + }) + + It("returns an error when containerID has invalid characters", func() { + environment["CNI_CONTAINERID"] = "some-%%container-id" + err := dispatch.pluginMain(funcs, versionInfo, "") + Expect(err).To(HaveOccurred()) + Expect(err).To(Equal(&types.Error{ + Code: types.ErrInvalidEnvironmentVariables, + Msg: "invalid characters in containerID", + Details: "some-%%container-id", + })) + }) + + Context("return errors when interface name is invalid", func() { + It("interface name is too long", func() { + environment["CNI_IFNAME"] = "1234567890123456" + + err := dispatch.pluginMain(funcs, versionInfo, "") + Expect(err).To(HaveOccurred()) + Expect(err).To(Equal(&types.Error{ + Code: types.ErrInvalidEnvironmentVariables, + Msg: "interface name is too long", + Details: "interface name should be less than 16 characters", + })) + }) + + It("interface name is .", func() { + environment["CNI_IFNAME"] = "." + + err := dispatch.pluginMain(funcs, versionInfo, "") + Expect(err).To(HaveOccurred()) + Expect(err).To(Equal(&types.Error{ + Code: types.ErrInvalidEnvironmentVariables, + Msg: "interface name is . or ..", + Details: "", + })) + }) + + It("interface name is ..", func() { + environment["CNI_IFNAME"] = ".." + + err := dispatch.pluginMain(funcs, versionInfo, "") + Expect(err).To(HaveOccurred()) + Expect(err).To(Equal(&types.Error{ + Code: types.ErrInvalidEnvironmentVariables, + Msg: "interface name is . or ..", + Details: "", + })) + }) + + It("interface name contains invalid characters /", func() { + environment["CNI_IFNAME"] = "test/test" + + err := dispatch.pluginMain(funcs, versionInfo, "") + Expect(err).To(HaveOccurred()) + Expect(err).To(Equal(&types.Error{ + Code: types.ErrInvalidEnvironmentVariables, + Msg: "interface name contains / or : or whitespace characters", + Details: "", + })) + }) + + It("interface name contains invalid characters :", func() { + environment["CNI_IFNAME"] = "test:test" + + err := dispatch.pluginMain(funcs, versionInfo, "") + Expect(err).To(HaveOccurred()) + Expect(err).To(Equal(&types.Error{ + Code: types.ErrInvalidEnvironmentVariables, + Msg: "interface name contains / or : or whitespace characters", + Details: "", + })) + }) + + It("interface name contains invalid characters whitespace", func() { + environment["CNI_IFNAME"] = "test test" + + err := dispatch.pluginMain(funcs, versionInfo, "") + Expect(err).To(HaveOccurred()) + Expect(err).To(Equal(&types.Error{ + Code: types.ErrInvalidEnvironmentVariables, + Msg: "interface name contains / or : or whitespace characters", + Details: "", + })) + }) + }) + + It("does not call cmdCheck or cmdDel", func() { + err := dispatch.pluginMain(funcs, versionInfo, "") + + Expect(err).NotTo(HaveOccurred()) + Expect(cmdCheck.CallCount).To(Equal(0)) + Expect(cmdDel.CallCount).To(Equal(0)) + }) + + DescribeTable("required / optional env vars", envVarChecker, + Entry("command", "CNI_COMMAND", true), + Entry("container id", "CNI_CONTAINERID", true), + Entry("net ns", "CNI_NETNS", true), + Entry("if name", "CNI_IFNAME", true), + Entry("args", "CNI_ARGS", false), + Entry("path", "CNI_PATH", true), + ) + + Context("when multiple required env vars are missing", func() { + BeforeEach(func() { + delete(environment, "CNI_NETNS") + delete(environment, "CNI_IFNAME") + delete(environment, "CNI_PATH") + }) + + It("reports that all of them are missing, not just the first", func() { + err := dispatch.pluginMain(funcs, versionInfo, "") + Expect(err).To(HaveOccurred()) + + Expect(err).To(Equal(&types.Error{ + Code: types.ErrInvalidEnvironmentVariables, + Msg: "required env variables [CNI_NETNS,CNI_IFNAME,CNI_PATH] missing", + })) + }) + }) + + Context("when the stdin data is missing the required cniVersion config", func() { + BeforeEach(func() { + dispatch.Stdin = strings.NewReader(`{ "name": "skel-test", "some": "config" }`) + }) + + Context("when the plugin supports version 0.1.0", func() { + BeforeEach(func() { + versionInfo = version.PluginSupports("0.1.0") + expectedCmdArgs.StdinData = []byte(`{ "name": "skel-test", "some": "config" }`) + }) + + It("infers the config is 0.1.0 and calls the cmdAdd callback", func() { + err := dispatch.pluginMain(funcs, versionInfo, "") + Expect(err).NotTo(HaveOccurred()) + + Expect(cmdAdd.CallCount).To(Equal(1)) + Expect(cmdAdd.Received.CmdArgs).To(Equal(expectedCmdArgs)) + }) + }) + + Context("when the plugin does not support 0.1.0", func() { + BeforeEach(func() { + versionInfo = version.PluginSupports("4.3.2") + }) + + It("immediately returns a useful error", func() { + err := dispatch.pluginMain(funcs, versionInfo, "") + Expect(err.Code).To(Equal(types.ErrIncompatibleCNIVersion)) // see https://github.com/containernetworking/cni/blob/main/SPEC.md#well-known-error-codes + Expect(err.Msg).To(Equal("incompatible CNI versions")) + Expect(err.Details).To(Equal(`config is "0.1.0", plugin supports ["4.3.2"]`)) + }) + + It("does not call either callback", func() { + err := dispatch.pluginMain(funcs, versionInfo, "") + Expect(err).To(HaveOccurred()) + Expect(cmdAdd.CallCount).To(Equal(0)) + Expect(cmdCheck.CallCount).To(Equal(0)) + Expect(cmdDel.CallCount).To(Equal(0)) + }) + }) + }) + }) + + Context("when the CNI_COMMAND is CHECK", func() { + BeforeEach(func() { + environment["CNI_COMMAND"] = "CHECK" + }) + + It("extracts env vars and stdin data and calls cmdCheck", func() { + err := dispatch.pluginMain(funcs, versionInfo, "") + + Expect(err).NotTo(HaveOccurred()) + Expect(cmdAdd.CallCount).To(Equal(0)) + Expect(cmdCheck.CallCount).To(Equal(1)) + Expect(cmdDel.CallCount).To(Equal(0)) + Expect(cmdCheck.Received.CmdArgs).To(Equal(expectedCmdArgs)) + }) + + It("does not call cmdAdd or cmdDel", func() { + err := dispatch.pluginMain(funcs, versionInfo, "") + + Expect(err).NotTo(HaveOccurred()) + Expect(cmdAdd.CallCount).To(Equal(0)) + Expect(cmdDel.CallCount).To(Equal(0)) + }) + + DescribeTable("required / optional env vars", envVarChecker, + Entry("command", "CNI_COMMAND", true), + Entry("container id", "CNI_CONTAINERID", true), + Entry("net ns", "CNI_NETNS", true), + Entry("if name", "CNI_IFNAME", true), + Entry("args", "CNI_ARGS", false), + Entry("path", "CNI_PATH", true), + ) + + Context("when multiple required env vars are missing", func() { + BeforeEach(func() { + delete(environment, "CNI_NETNS") + delete(environment, "CNI_IFNAME") + delete(environment, "CNI_PATH") + }) + + It("reports that all of them are missing, not just the first", func() { + err := dispatch.pluginMain(funcs, versionInfo, "") + Expect(err).To(HaveOccurred()) + + Expect(err).To(Equal(&types.Error{ + Code: types.ErrInvalidEnvironmentVariables, + Msg: "required env variables [CNI_NETNS,CNI_IFNAME,CNI_PATH] missing", + })) + }) + }) + + Context("when cniVersion is less than 0.4.0", func() { + It("immediately returns a useful error", func() { + dispatch.Stdin = strings.NewReader(`{ "name": "skel-test", "cniVersion": "0.3.0", "some": "config" }`) + err := dispatch.pluginMain(funcs, versionInfo, "") + Expect(err.Code).To(Equal(types.ErrIncompatibleCNIVersion)) // see https://github.com/containernetworking/cni/blob/main/SPEC.md#well-known-error-codes + Expect(err.Msg).To(Equal("config version does not allow CHECK")) + Expect(cmdAdd.CallCount).To(Equal(0)) + Expect(cmdCheck.CallCount).To(Equal(0)) + Expect(cmdDel.CallCount).To(Equal(0)) + }) + }) + + Context("when plugin does not support 0.4.0", func() { + It("immediately returns a useful error", func() { + dispatch.Stdin = strings.NewReader(`{ "name": "skel-test", "cniVersion": "0.4.0", "some": "config" }`) + versionInfo = version.PluginSupports("0.1.0", "0.2.0", "0.3.0") + err := dispatch.pluginMain(funcs, versionInfo, "") + Expect(err.Code).To(Equal(types.ErrIncompatibleCNIVersion)) // see https://github.com/containernetworking/cni/blob/main/SPEC.md#well-known-error-codes + Expect(err.Msg).To(Equal("plugin version does not allow CHECK")) + Expect(cmdAdd.CallCount).To(Equal(0)) + Expect(cmdCheck.CallCount).To(Equal(0)) + Expect(cmdDel.CallCount).To(Equal(0)) + }) + }) + + Context("when the config has a bad version", func() { + It("immediately returns a useful error", func() { + dispatch.Stdin = strings.NewReader(`{ "cniVersion": "adsfsadf", "some": "config", "name": "test" }`) + versionInfo = version.PluginSupports("0.1.0", "0.2.0", "0.3.0") + err := dispatch.pluginMain(funcs, versionInfo, "") + Expect(err.Code).To(Equal(types.ErrDecodingFailure)) + Expect(cmdAdd.CallCount).To(Equal(0)) + Expect(cmdCheck.CallCount).To(Equal(0)) + Expect(cmdDel.CallCount).To(Equal(0)) + }) + }) + + Context("when the config has a bad name", func() { + It("immediately returns invalid network config", func() { + dispatch.Stdin = strings.NewReader(`{ "cniVersion": "0.4.0", "some": "config", "name": "te%%st" }`) + versionInfo = version.PluginSupports("0.1.0", "0.2.0", "0.3.0", "0.4.0") + err := dispatch.pluginMain(funcs, versionInfo, "") + Expect(err.Code).To(Equal(types.ErrInvalidNetworkConfig)) + Expect(cmdAdd.CallCount).To(Equal(0)) + Expect(cmdCheck.CallCount).To(Equal(0)) + Expect(cmdDel.CallCount).To(Equal(0)) + }) + }) + + Context("when the plugin has a bad version", func() { + It("immediately returns a useful error", func() { + dispatch.Stdin = strings.NewReader(`{ "cniVersion": "0.4.0", "some": "config", "name": "test" }`) + versionInfo = version.PluginSupports("0.1.0", "0.2.0", "adsfasdf") + err := dispatch.pluginMain(funcs, versionInfo, "") + Expect(err.Code).To(Equal(types.ErrDecodingFailure)) + Expect(cmdAdd.CallCount).To(Equal(0)) + Expect(cmdCheck.CallCount).To(Equal(0)) + Expect(cmdDel.CallCount).To(Equal(0)) + }) + }) + }) + + Context("when the CNI_COMMAND is GC", func() { + BeforeEach(func() { + environment["CNI_COMMAND"] = "GC" + delete(environment, "CNI_NETNS") + delete(environment, "CNI_IFNAME") + delete(environment, "CNI_CONTAINERID") + delete(environment, "CNI_ARGS") + + expectedCmdArgs = &CmdArgs{ + Path: "/some/cni/path", + StdinData: []byte(stdinData), + } + + dispatch = &dispatcher{ + Getenv: func(key string) string { + return environment[key] + }, + Stdin: strings.NewReader(stdinData), + Stdout: stdout, + Stderr: stderr, + } + }) + + It("extracts env vars and stdin data and calls cmdGC", func() { + err := dispatch.pluginMain(funcs, versionInfo, "") + + Expect(err).NotTo(HaveOccurred()) + Expect(cmdAdd.CallCount).To(Equal(0)) + Expect(cmdCheck.CallCount).To(Equal(0)) + Expect(cmdDel.CallCount).To(Equal(0)) + Expect(cmdGC.CallCount).To(Equal(1)) + Expect(cmdGC.Received.CmdArgs).To(Equal(expectedCmdArgs)) + }) + + It("does not call cmdAdd or cmdDel", func() { + err := dispatch.pluginMain(funcs, versionInfo, "") + + Expect(err).NotTo(HaveOccurred()) + Expect(cmdAdd.CallCount).To(Equal(0)) + Expect(cmdDel.CallCount).To(Equal(0)) + }) + + DescribeTable("required / optional env vars", envVarChecker, + Entry("command", "CNI_COMMAND", true), + Entry("container id", "CNI_CONTAINERID", false), + Entry("net ns", "CNI_NETNS", false), + Entry("if name", "CNI_IFNAME", false), + Entry("args", "CNI_ARGS", false), + Entry("path", "CNI_PATH", true), + ) + + Context("when multiple required env vars are missing", func() { + BeforeEach(func() { + delete(environment, "CNI_PATH") + }) + + It("reports that all of them are missing, not just the first", func() { + err := dispatch.pluginMain(funcs, versionInfo, "") + Expect(err).To(HaveOccurred()) + + Expect(err).To(Equal(&types.Error{ + Code: types.ErrInvalidEnvironmentVariables, + Msg: "required env variables [CNI_PATH] missing", + })) + }) + }) + + Context("when cniVersion is less than 1.1.0", func() { + It("immediately returns a useful error", func() { + dispatch.Stdin = strings.NewReader(`{ "name": "skel-test", "cniVersion": "0.3.0", "some": "config" }`) + err := dispatch.pluginMain(funcs, versionInfo, "") + Expect(err.Code).To(Equal(types.ErrIncompatibleCNIVersion)) // see https://github.com/containernetworking/cni/blob/main/SPEC.md#well-known-error-codes + Expect(err.Msg).To(Equal("config version does not allow GC")) + Expect(cmdGC.CallCount).To(Equal(0)) + }) + }) + + Context("when plugin does not support 1.1.0", func() { + It("immediately returns a useful error", func() { + dispatch.Stdin = strings.NewReader(`{ "name": "skel-test", "cniVersion": "1.1.0", "some": "config" }`) + versionInfo = version.PluginSupports("0.1.0", "0.2.0", "0.3.0") + err := dispatch.pluginMain(funcs, versionInfo, "") + Expect(err.Code).To(Equal(types.ErrIncompatibleCNIVersion)) // see https://github.com/containernetworking/cni/blob/main/SPEC.md#well-known-error-codes + Expect(err.Msg).To(Equal("plugin version does not allow GC")) + Expect(cmdGC.CallCount).To(Equal(0)) + }) + }) + + Context("when the config has a bad version", func() { + It("immediately returns a useful error", func() { + dispatch.Stdin = strings.NewReader(`{ "cniVersion": "adsfsadf", "some": "config", "name": "test" }`) + versionInfo = version.PluginSupports("1.1.0") + err := dispatch.pluginMain(funcs, versionInfo, "") + Expect(err.Code).To(Equal(types.ErrDecodingFailure)) + Expect(cmdGC.CallCount).To(Equal(0)) + }) + }) + + Context("when the config has a bad name", func() { + It("immediately returns invalid network config", func() { + dispatch.Stdin = strings.NewReader(`{ "cniVersion": "0.4.0", "some": "config", "name": "te%%st" }`) + versionInfo = version.PluginSupports("1.1.0") + err := dispatch.pluginMain(funcs, versionInfo, "") + Expect(err.Code).To(Equal(types.ErrInvalidNetworkConfig)) + Expect(cmdGC.CallCount).To(Equal(0)) + }) + }) + + Context("when the plugin has a bad version", func() { + It("immediately returns a useful error", func() { + dispatch.Stdin = strings.NewReader(`{ "cniVersion": "1.1.0", "some": "config", "name": "test" }`) + versionInfo = version.PluginSupports("0.1.0", "0.2.0", "adsfasdf") + err := dispatch.pluginMain(funcs, versionInfo, "") + Expect(err.Code).To(Equal(types.ErrDecodingFailure)) + Expect(cmdGC.CallCount).To(Equal(0)) + }) + }) + }) + + Context("when the CNI_COMMAND is DEL", func() { + BeforeEach(func() { + environment["CNI_COMMAND"] = "DEL" + }) + + It("calls cmdDel with the env vars and stdin data", func() { + err := dispatch.pluginMain(funcs, versionInfo, "") + + Expect(err).NotTo(HaveOccurred()) + Expect(cmdDel.CallCount).To(Equal(1)) + Expect(cmdDel.Received.CmdArgs).To(Equal(expectedCmdArgs)) + }) + + It("does not call cmdAdd", func() { + err := dispatch.pluginMain(funcs, versionInfo, "") + + Expect(err).NotTo(HaveOccurred()) + Expect(cmdAdd.CallCount).To(Equal(0)) + }) + + DescribeTable("required / optional env vars", envVarChecker, + Entry("command", "CNI_COMMAND", true), + Entry("container id", "CNI_CONTAINERID", true), + Entry("net ns", "CNI_NETNS", false), + Entry("if name", "CNI_IFNAME", true), + Entry("args", "CNI_ARGS", false), + Entry("path", "CNI_PATH", true), + ) + }) + + Context("when the CNI_COMMAND is VERSION", func() { + BeforeEach(func() { + environment["CNI_COMMAND"] = "VERSION" + }) + + It("prints the version to stdout", func() { + err := dispatch.pluginMain(funcs, versionInfo, "") + + Expect(err).NotTo(HaveOccurred()) + Expect(stdout).To(MatchJSON(fmt.Sprintf(`{ + "cniVersion": "%s", + "supportedVersions": ["9.8.7", "10.0.0"] + }`, version.Current()))) + }) + + It("does not call cmdAdd or cmdDel", func() { + err := dispatch.pluginMain(funcs, versionInfo, "") + + Expect(err).NotTo(HaveOccurred()) + Expect(cmdAdd.CallCount).To(Equal(0)) + Expect(cmdDel.CallCount).To(Equal(0)) + }) + + DescribeTable("VERSION does not need the usual env vars", envVarChecker, + Entry("command", "CNI_COMMAND", true), + Entry("container id", "CNI_CONTAINERID", false), + Entry("net ns", "CNI_NETNS", false), + Entry("if name", "CNI_IFNAME", false), + Entry("args", "CNI_ARGS", false), + Entry("path", "CNI_PATH", false), + ) + + It("does not read from Stdin", func() { + r := &BadReader{} + dispatch.Stdin = r + + err := dispatch.pluginMain(funcs, versionInfo, "") + + Expect(err).NotTo(HaveOccurred()) + Expect(r.ReadCount).To(Equal(0)) + Expect(stdout).To(MatchJSON(fmt.Sprintf(`{ + "cniVersion": "%s", + "supportedVersions": ["9.8.7", "10.0.0"] + }`, version.Current()))) + }) + }) + + Context("when the CNI_COMMAND is unrecognized", func() { + BeforeEach(func() { + environment["CNI_COMMAND"] = "NOPE" + }) + + It("does not call any cmd callback", func() { + err := dispatch.pluginMain(funcs, versionInfo, "") + Expect(err).To(HaveOccurred()) + Expect(cmdAdd.CallCount).To(Equal(0)) + Expect(cmdDel.CallCount).To(Equal(0)) + }) + + It("returns an error", func() { + err := dispatch.pluginMain(funcs, versionInfo, "") + + Expect(err).To(Equal(&types.Error{ + Code: types.ErrInvalidEnvironmentVariables, + Msg: "unknown CNI_COMMAND: NOPE", + })) + }) + + It("prints the about string when the command is blank", func() { + environment["CNI_COMMAND"] = "" + err := dispatch.pluginMain(funcs, versionInfo, "test framework v42") + Expect(err).NotTo(HaveOccurred()) + Expect(stderr.String()).To(ContainSubstring("test framework v42")) + }) + }) + + Context("when the CNI_COMMAND is missing", func() { + It("prints the about string to stderr", func() { + environment = map[string]string{} + err := dispatch.pluginMain(funcs, versionInfo, "AWESOME PLUGIN") + Expect(err).NotTo(HaveOccurred()) + + Expect(cmdAdd.CallCount).To(Equal(0)) + Expect(cmdDel.CallCount).To(Equal(0)) + log := stderr.String() + Expect(log).To(Equal("AWESOME PLUGIN\nCNI protocol versions supported: 9.8.7, 10.0.0\n")) + }) + + It("fails if there is no about string", func() { + environment = map[string]string{} + err := dispatch.pluginMain(funcs, versionInfo, "") + Expect(err).To(HaveOccurred()) + + Expect(cmdAdd.CallCount).To(Equal(0)) + Expect(cmdDel.CallCount).To(Equal(0)) + Expect(err).To(Equal(&types.Error{ + Code: types.ErrInvalidEnvironmentVariables, + Msg: "required env variables [CNI_COMMAND] missing", + })) + }) + }) + + Context("when stdin cannot be read", func() { + BeforeEach(func() { + dispatch.Stdin = &BadReader{} + }) + + It("does not call any cmd callback", func() { + err := dispatch.pluginMain(funcs, versionInfo, "") + Expect(err).To(HaveOccurred()) + Expect(cmdAdd.CallCount).To(Equal(0)) + Expect(cmdDel.CallCount).To(Equal(0)) + }) + + It("wraps and returns the error", func() { + err := dispatch.pluginMain(funcs, versionInfo, "") + + Expect(err).To(Equal(&types.Error{ + Code: types.ErrIOFailure, + Msg: "error reading from stdin: banana", + })) + }) + }) + + Context("when the callback returns an error", func() { + Context("when it is a typed Error", func() { + BeforeEach(func() { + cmdAdd.Returns.Error = &types.Error{ + Code: 1234, + Msg: "insufficient something", + } + }) + + It("returns the error as-is", func() { + err := dispatch.pluginMain(funcs, versionInfo, "") + + Expect(err).To(Equal(&types.Error{ + Code: 1234, + Msg: "insufficient something", + })) + }) + }) + + Context("when it is an unknown error", func() { + BeforeEach(func() { + cmdAdd.Returns.Error = errors.New("potato") + }) + + It("wraps and returns the error", func() { + err := dispatch.pluginMain(funcs, versionInfo, "") + + Expect(err).To(Equal(&types.Error{ + Code: types.ErrInternal, + Msg: "potato", + })) + }) + }) + }) +}) + +// BadReader is an io.Reader which always errors +type BadReader struct { + Error error + ReadCount int +} + +func (r *BadReader) Read(buffer []byte) (int, error) { + r.ReadCount++ + if r.Error != nil { + return 0, r.Error + } + return 0, errors.New("banana") +} + +func (r *BadReader) Close() error { + return nil +} diff --git a/pkg/bpf/pindir.go b/pkg/bpf/pindir.go new file mode 100644 index 0000000..cab780e --- /dev/null +++ b/pkg/bpf/pindir.go @@ -0,0 +1,8 @@ +package bpf + +// PinDir holds the bpffs paths for natra's per-pod tcx-link pins and +// per-pod map pins. A dedicated subdir keeps natra's pins separate from +// other tooling on the node and makes cleanup straightforward (DEL +// removes the per-container files). Shared by the CNI plugin, which +// writes the pins, and natra-tools, which reads them. +const PinDir = "/sys/fs/bpf/natra" From da67f68e02584e1ad9b55ba0c54cf7383fbed26f Mon Sep 17 00:00:00 2001 From: terra tauri Date: Tue, 29 Sep 2026 20:50:15 -0700 Subject: [PATCH 3/3] ci: bench gate compares toolchains and requires regressions to reproduce Two gaps showed up on this PR's first gated run: - The main side was built with HEAD's Go (setup-go installs the toolchain from HEAD's go.mod, and main's older go line doesn't force its own), so a toolchain bump was never measured. bench-ab.sh now runs each side with GOTOOLCHAIN from that side's go.mod. - ParseBandwidthAnnotationJSON came out +0.65% (p<0.001) on code this PR doesn't touch, while a sibling run of the same commit showed no change: alignment/runner noise. scripts/bench-check.sh now runs a second full A/B when the first flags anything and fails only on benchmarks that regress in both, so a real regression of any size still fails. Claude-Session: https://claude.ai/code/session_01RyYnvjh5DBevutC4HTgELn --- .claude/maintain.md | 12 ++++++++--- .github/workflows/unit.yml | 11 +++++----- scripts/bench-ab.sh | 31 ++++++++++++++++++++++------ scripts/bench-check.sh | 41 ++++++++++++++++++++++++++++++++++++++ scripts/bench-gate.sh | 13 +++++++++--- 5 files changed, 90 insertions(+), 18 deletions(-) create mode 100755 scripts/bench-check.sh diff --git a/.claude/maintain.md b/.claude/maintain.md index 084aa3a..ea31418 100644 --- a/.claude/maintain.md +++ b/.claude/maintain.md @@ -17,9 +17,15 @@ - `make test-perf` (L5) - `make ci` runs every layer and prints a per-layer summary. - Benchmark results must never get worse. CI's "Bench (compare to - main)" job runs `scripts/bench-ab.sh` (interleaved main/HEAD rounds, - Go benchmarks + BPF hot-path `BenchmarkBPF`) and - `scripts/bench-gate.sh`, which fails on any significant regression. + main)" job runs `scripts/bench-check.sh`: interleaved main/HEAD + rounds (`bench-ab.sh`, each side on its own go.mod toolchain; Go + benchmarks + BPF hot-path `BenchmarkBPF`), gated by `bench-gate.sh`. + Any significant regression triggers a second A/B; it fails if the + same benchmark regresses again. +- Sequential local microbenchmarks are unreliable on a laptop (the same + binary measured 60% apart an hour later). Compare interleaved, and + measure CNI invoke RSS with many interleaved `/usr/bin/time -v` runs + rather than the k3d rig's 3-sample poll. For dataplane changes also compare `PERF_PROFILE=full make perf-vs-vanilla` on main vs the branch before merging. - Head-to-head numbers: `make perf-vs-vanilla` (k3d, ~20 min) and the diff --git a/.github/workflows/unit.yml b/.github/workflows/unit.yml index cd00a3a..b6ce719 100644 --- a/.github/workflows/unit.yml +++ b/.github/workflows/unit.yml @@ -69,15 +69,14 @@ jobs: cd "$RUNNER_TEMP/base" make build-bpf cp bpf/natra.bpf.o pkg/bpf/natra.bpf.o - # Interleaved rounds, 10 samples per side. Benchmark results must - # never get worse: the gate fails on any significant regression. + # Interleaved rounds, 10 samples per side, each side on its own + # go.mod toolchain. Benchmark results must never get worse: any + # significant regression that reproduces on a second A/B fails. - name: Bench main vs HEAD - run: scripts/bench-ab.sh "$RUNNER_TEMP/base" . bench-main.txt bench-head.txt - - name: Regression gate - run: scripts/bench-gate.sh bench-main.txt bench-head.txt + run: scripts/bench-check.sh "$RUNNER_TEMP/base" . bench-out - name: Upload artifacts if: always() uses: actions/upload-artifact@v4 with: name: bench-results - path: bench-*.txt + path: bench-out/ diff --git a/scripts/bench-ab.sh b/scripts/bench-ab.sh index fdd4a14..4fb9828 100755 --- a/scripts/bench-ab.sh +++ b/scripts/bench-ab.sh @@ -9,6 +9,9 @@ # so they go through sudo when not already root). Each checkout needs # its bpf/natra.bpf.o built and copied to pkg/bpf/ first. # +# Each side builds with the Go toolchain its own go.mod names, so a +# toolchain bump is part of what gets compared. +# # Usage: scripts/bench-ab.sh # Env: BENCH_ROUNDS (default 5), BENCH_COUNT per round (default 2) @@ -25,6 +28,22 @@ head_out="$(realpath "$4" 2>/dev/null || echo "$PWD/$4")" rounds="${BENCH_ROUNDS:-5}" count="${BENCH_COUNT:-2}" +# go_toolchain prints the GOTOOLCHAIN value for a checkout: its +# `toolchain` line if present, else its `go` line (go1.N -> go1.N.0). +go_toolchain() { + awk ' + $1 == "toolchain" { tc = $2 } + $1 == "go" && !gv { gv = $2 } + END { + if (tc != "") { print tc; exit } + if (split(gv, p, ".") == 2) gv = gv ".0" + print "go" gv + }' "$1/go.mod" +} +base_tc="$(go_toolchain "$base_dir")" +head_tc="$(go_toolchain "$head_dir")" +echo "base toolchain $base_tc, head toolchain $head_tc" >&2 + sudo_cmd=() if [ "$(id -u)" -ne 0 ]; then sudo_cmd=(sudo) @@ -38,16 +57,16 @@ trap 'rm -rf "$bin_dir"' EXIT bpf_bench=0 if [ "$(uname -s)" = Linux ]; then bpf_bench=1 - (cd "$base_dir" && go test -c -tags=perf -o "$bin_dir/base-perf.test" ./test/perf/) - (cd "$head_dir" && go test -c -tags=perf -o "$bin_dir/head-perf.test" ./test/perf/) + (cd "$base_dir" && GOTOOLCHAIN="$base_tc" go test -c -tags=perf -o "$bin_dir/base-perf.test" ./test/perf/) + (cd "$head_dir" && GOTOOLCHAIN="$head_tc" go test -c -tags=perf -o "$bin_dir/head-perf.test" ./test/perf/) fi : >"$base_out" : >"$head_out" run_side() { - local side="$1" dir="$2" out="$3" - (cd "$dir" && go test -run='^$' -bench=. -benchmem -count="$count" ./pkg/... >>"$out") + local side="$1" dir="$2" out="$3" tc="$4" + (cd "$dir" && GOTOOLCHAIN="$tc" go test -run='^$' -bench=. -benchmem -count="$count" ./pkg/... >>"$out") if [ "$bpf_bench" = 1 ]; then (cd "$dir/test/perf" && "${sudo_cmd[@]}" "$bin_dir/$side-perf.test" \ -test.run='^$' -test.bench=BPF -test.count="$count" >>"$out") @@ -56,6 +75,6 @@ run_side() { for i in $(seq "$rounds"); do echo "round $i/$rounds" >&2 - run_side base "$base_dir" "$base_out" - run_side head "$head_dir" "$head_out" + run_side base "$base_dir" "$base_out" "$base_tc" + run_side head "$head_dir" "$head_out" "$head_tc" done diff --git a/scripts/bench-check.sh b/scripts/bench-check.sh new file mode 100755 index 0000000..f26e9cf --- /dev/null +++ b/scripts/bench-check.sh @@ -0,0 +1,41 @@ +#!/usr/bin/env bash +# CI benchmark gate. Benchmark results must never get worse. +# +# Runs an interleaved A/B (bench-ab.sh) and gates it (bench-gate.sh). +# If anything regresses, runs a second full A/B and fails only on +# benchmarks that regress in both runs: a real regression of any size +# reproduces, while code-alignment and runner noise mostly doesn't. +# +# Usage: scripts/bench-check.sh + +set -euo pipefail + +if [ "$#" -ne 3 ]; then + echo "usage: $0 " >&2 + exit 64 +fi +here="$(cd "$(dirname "$0")" && pwd)" +base_dir="$1" +head_dir="$2" +out="$3" +mkdir -p "$out" + +"$here/bench-ab.sh" "$base_dir" "$head_dir" "$out/bench-main-1.txt" "$out/bench-head-1.txt" +if "$here/bench-gate.sh" "$out/bench-main-1.txt" "$out/bench-head-1.txt" "$out/regressed-1.txt"; then + exit 0 +fi + +echo +echo "Re-running the A/B to check whether the regressions reproduce." +"$here/bench-ab.sh" "$base_dir" "$head_dir" "$out/bench-main-2.txt" "$out/bench-head-2.txt" +"$here/bench-gate.sh" "$out/bench-main-2.txt" "$out/bench-head-2.txt" "$out/regressed-2.txt" || true + +confirmed="$(sort "$out/regressed-1.txt" | comm -12 - <(sort "$out/regressed-2.txt"))" +if [ -n "$confirmed" ]; then + echo + echo "Regressions that reproduced in both runs:" + printf '%s\n' "$confirmed" | sed 's/^/ /' + exit 1 +fi +echo +echo "No regression reproduced across both runs." diff --git a/scripts/bench-gate.sh b/scripts/bench-gate.sh index c4f6d36..979b056 100755 --- a/scripts/bench-gate.sh +++ b/scripts/bench-gate.sh @@ -8,16 +8,19 @@ # bpf-ns/op) is lower-is-better, so any "+" is a regression. Run each # side with -count=10 or more so benchstat can detect a difference. # -# Usage: scripts/bench-gate.sh +# Usage: scripts/bench-gate.sh [regressed-out] +# regressed-out, if given, receives one " " line per +# regression (scripts/bench-check.sh uses it to confirm on a rerun). set -euo pipefail -if [ "$#" -ne 2 ]; then - echo "usage: $0 " >&2 +if [ "$#" -lt 2 ] || [ "$#" -gt 3 ]; then + echo "usage: $0 [regressed-out]" >&2 exit 64 fi base="$1" head="$2" +regressed_out="${3:-}" benchstat "$base" "$head" @@ -28,6 +31,10 @@ regressions="$(benchstat -format csv "$base" "$head" 2>/dev/null | awk -F, ' $6 ~ /^\+/ { printf " %s %s %s (%s)\n", $1, unit, $6, $7 } ')" +if [ -n "$regressed_out" ]; then + printf '%s\n' "$regressions" | awk 'NF { print $1, $2 }' >"$regressed_out" +fi + if [ -n "$regressions" ]; then echo echo "Benchmark regressions vs base (p < 0.05):"