docs+fixes(cni): finish the CNI plugin-chain split — docs, containerlab, dead code - #307
Conversation
a0b9606 to
7d64f71
Compare
7d64f71 to
77169b0
Compare
77169b0 to
f780964
Compare
f780964 to
725ef87
Compare
|
Reviewed this one, the last in the stack. Deleting the isTapMode/CNI_NETNS_OVERRIDE bypass here is the right move, that's the exact dead code I flagged back on #303, but removing it changes the failure mode rather than closing it cleanly, and it's worth a look before merge.
A few smaller things. The 11 rewritten NAD YAML fixtures under
And Everything else I checked came back clean: the CNIGetEnableLocalIPAM/isTapMode removal itself, the e2e binary path fixes, the comment-only fixes elsewhere, NAD field names and CIDR correctness, and import/naming/error-handling conventions, all verified against build, vet, tests, and the actual code. The resource-leak-on-retry issue is the one worth taking seriously, since it's a real gap the stack leaves open even though the change that surfaces it is the correct one to make. |
…ab, dead code
Step 4 of the CNI plugin-chain split (galactic/plan-cni-plugin-chain):
brings every doc, fixture, and stray leftover in the repo up to date
with the 5-binary chain steps 0-3 built (galactic-cni/galactic-tap-cni
→ galactic-route → galactic-bgp, galactic-ipam delegated). No new
runtime behavior in this step beyond the fixes below — everything here
is either documentation or cleanup of things the split itself should
already have caught.
internal/installer/installer.go's Bootstrap binary-copy list already
covers all five chain binaries as of steps 0-3 (added incrementally,
one per step) — nothing left to do there. Confirmed the static
host-level conflist Bootstrap writes only ever carries node-level
settings (node_name/kubeconfig/namespace/log), never per-attachment
fields, so its own shape needs no change; the per-attachment chain
conflist is the external companion operator's own NAD-templated JSON
(cross-repo, out of scope).
Docs rewritten for the new chain shape:
- docs/cni-cmd-sequence.md: all 3 sequence diagrams rewritten with
per-binary lifelines (master → galactic-route [optional] →
galactic-bgp for ADD; every binary's own no-op DEL). Also fixes a
pre-existing staleness independent of this split: the old diagrams
showed a kernel seg6local "END.DT46 decap route" install, which the
2026-08-02 eBPF uSID datapath cutover had already replaced with
vrf_table/locator_table/function_table map registration — nothing
had updated these diagrams for that cutover either.
- docs/cni/configuration.md: full rewrite. New "Chain structure"
section explaining the plugins[] conflist shape; per-binary field
tables (master plugin, IPAM, termination, BGP-publish); all 7
example configs rewritten as full multi-plugin conflists instead of
single flat JSON objects. Corrected two inaccuracies while at it:
IPAM.Routes/Addresses (the config-input struct fields) are declared
but never read by any current allocation path — the previous draft
of this doc guessed they were read; pool IPAM's DEL path no longer
reads a BGPAdvertisement CRD annotation at all (superseded by
galactic-ipam's own on-disk marker-file persistence, decision 2).
- docs/agents/ARCHITECTURE.md: Overview, Repository Layout tree,
Components table, Entry Points (new subsections for
galactic-tap-cni/galactic-ipam/galactic-bgp/galactic-route),
Configuration section, Module/Package Reference table, Key Design
Decisions, Testing, Known Constraints, and the "For Claude" tables
all updated for the new package layout. Also fixes the same
pre-existing eBPF-cutover staleness as the sequence-diagram doc: the
"SRv6 SID encoding" section described a kernel route install
(resolveSRv6SID, a function that hasn't existed since the cutover)
instead of the actual current mechanism (registerEBPFDatapath's
locator_table/function_table/vrf_table registration). Added Known
Constraints notes for two coverage gaps this split's own review
surfaced: the e2e suite doesn't exercise galactic-route/galactic-bgp
(pre-existing gap, unchanged by the split — the monolithic
predecessor never had this coverage either), and vmtap-cni/
internal/vmtap (an unrelated, pre-existing binary for a different
Cilium-chain-conflist-patching purpose) has its own separate doc.
Fixed forward, not just documented: a real functional regression in
deploy/containerlab/'s tenant fixtures. All 11 NAD YAML files still
used the pre-split single-plugin shape ("type": "galactic-cni" with
top-level ipv6_subnet/ipv4_subnet/address_families, no "ipam" block,
no "plugins" chain array) — the explicit-contract rewrite in step 1
moved those addressing fields inside an "ipam" block that these
fixtures never gained, and once BGP publish moved out to its own
chained binary in step 2, a single-plugin NAD never invokes
galactic-bgp at all. Left as-is, every attachment in the containerlab
lab would ADD successfully (silently allocating no IPAM address at
all, since the top-level fields are simply unknown/ignored JSON to
galactic-cni's own PluginConf now) and never get BGP-advertised.
Rewrote all 11 as plugins: [galactic-cni, galactic-bgp] conflists with
the addressing fields moved into an explicit ipam block
(type: galactic-ipam); none of them have terminations, so no
galactic-route stage was needed. deploy/containerlab/docs/tenants.md's
own narrative description of the attach flow updated to match.
Also fixed while updating tests/e2e/e2e_test.go's own doc comments for
step 2's function moves: TestCNITapInterface was invoking
/galactic-cni directly with a config carrying interface_type: tap and
a bogus srv6_locator field — both removed in step 0's own split, so
this test has been silently exercising nothing since. Fixed to invoke
/galactic-tap-cni (the real tap master binary now), with the ipam
block's explicit-contract shape (type: galactic-ipam, ipv6_subnet) and
CNI_PATH=/ (where the image actually places every chain binary, not
/opt/cni/bin — that only exists once installer.Bootstrap's init
container stages it onto a real host, which this test's pod never
runs). Removed the eBPF-control-daemon startup this test used to need:
eBPF registration is entirely galactic-bgp's job now, which this test
doesn't invoke (verifying BGP/SRv6/eBPF publish end-to-end would need
a BGPRouter fixture and RBAC this test doesn't set up — a gap flagged
in ARCHITECTURE.md's Known Constraints, not attempted here, and no
regression versus the monolithic predecessor's own e2e coverage).
task test:e2e itself not run in this step (same caveat as steps 0-3);
`go test ./tests/e2e/...` locally confirms the fixed test fails for
the expected "no such cluster/serviceaccount" reasons rather than any
change-related panic, identically to the two untouched tests in the
same file.
Removed dead code discovered while updating these docs, all from step
0's own incomplete cleanup:
- cmd/galactic-cni/main.go: deleted the entire stdin-peek-and-repipe
dance and isTapMode() helper. This was the pre-split monolithic
binary's mechanism for detecting interface_type: tap from raw stdin
JSON before skel's netns validation ran, so it could conditionally
set CNI_NETNS_OVERRIDE. Once galactic-tap-cni became its own binary,
this whole detection path in galactic-cni became unreachable dead
code: galactic-cni is veth-only now, PluginConf has no InterfaceType
field to read even if a config carried one, and it never needs
CNI_NETNS_OVERRIDE at all (it always operates in a real container
netns). cmd/galactic-tap-cni/main.go's own copy-pasted version of the
same dance simplified too: it already set CNI_NETNS_OVERRIDE
unconditionally (correct — every invocation of that binary is tap
mode), so the stdin buffering/repiping around it was buffering
content it never actually inspected. Both binaries build, vet, and
pass their existing tests unchanged after the simplification.
- internal/config/cni.go: removed EnvCNIEnableLocalIPAM
("GALACTIC_CNI_ENABLE_LOCAL_IPAM") and CNIGetEnableLocalIPAM(), plus
their tests. Step 1 introduced the successor
(GALACTIC_IPAM_ENABLE_LOCAL_IPAM, internal/config/ipam.go) but never
removed the predecessor; grep confirmed zero production callers
remained, only its own now-removed unit tests.
- internal/plumbing/ebpf/doc.go, internal/plumbing/ebpf/usidmap/vrf.go,
internal/plumbing/srv6/usid.go, internal/gc/gc.go: updated doc
comments still pointing at internal/cni/bgp.go (deleted in step 2)
to internal/cnibgp/bgp.go.
Verification: task lint (0 issues), task build (all 8 binaries),
task test:unit all green (0 FAIL). task test:e2e not run (same caveat
as every prior step in this stack).
725ef87 to
c690b5b
Compare
Stack (merge bottom to top):
Summary
Fifth and final branch in the CNI plugin-chain split stack (based on #306). Docs, containerlab fixtures, and leftover dead code brought up to date with the 5-binary chain steps 0-3 built. No new runtime behavior beyond the fixes below.
installer.Bootstrap's binary-copy list already covers all five chain binaries (added incrementally per step) — nothing left to do there. Confirmed the static host-level conflist only ever carries node-level settings, never per-attachment fields, so its shape needs no change.Docs rewritten
docs/cni-cmd-sequence.md— all 3 diagrams rewritten with per-binary lifelines. Also fixes a pre-existing staleness: the diagrams showed a kernel seg6local route install, which the 2026-08-02 eBPF uSID datapath cutover had already replaced withvrf_table/locator_table/function_tablemap registration.docs/cni/configuration.md— full rewrite: chain/conflist structure, per-binary field tables, all 7 example configs as multi-plugin conflists. Corrected two inaccuracies:IPAM.Routes/Addressesare declared but never read by any allocation path; pool IPAM's DEL no longer reads aBGPAdvertisementCRD annotation (superseded by on-disk marker files).docs/agents/ARCHITECTURE.md— repo layout, components, entry points, config, module/package reference, key design decisions, testing, known constraints all updated. Same eBPF-cutover fix as the sequence doc. Added Known Constraints notes for two coverage gaps: e2e doesn't exercisegalactic-route/galactic-bgp(pre-existing, unchanged by the split), andvmtap-cni(unrelated binary) has its own doc.Real regression found and fixed: containerlab NAD fixtures
All 11 NAD YAML files under
deploy/containerlab/still used the pre-split single-plugin shape — no"ipam"block, no"plugins"chain array. Left as-is, every attachment would ADD successfully but silently allocate no IPAM address (top-level fields are unknown JSON to the newPluginConf) and never get BGP-advertised (nogalactic-bgpinvocation). Rewrote all 11 asplugins: [galactic-cni, galactic-bgp]conflists with addressing moved into an explicitipamblock.deploy/containerlab/docs/tenants.md's narrative updated to match.e2e test fix
TestCNITapInterfaceinvoked/galactic-cnidirectly withinterface_type: tapand a bogussrv6_locatorfield — both removed in step 0, so it's been exercising nothing since. Fixed to invoke/galactic-tap-cniwith the explicit-contractipamshape andCNI_PATH=/(where the image actually places every binary). Removed the eBPF-control-daemon startup this test no longer needs — eBPF registration isgalactic-bgp's job now, which this test doesn't invoke (verifying that end-to-end needs aBGPRouterfixture, flagged in Known Constraints, not attempted here — no regression versus the monolithic predecessor's own coverage).Dead code removed
cmd/galactic-cni/main.go: deleted the stdin-peek-and-repipe dance andisTapMode()— unreachable oncegalactic-tap-cnibecame its own binary.cmd/galactic-tap-cni/main.go's copy-pasted version simplified too (it already setCNI_NETNS_OVERRIDEunconditionally; the buffering around it inspected nothing).internal/config/cni.go: removedGALACTIC_CNI_ENABLE_LOCAL_IPAM/CNIGetEnableLocalIPAM()— step 1 introduced the successor but never removed the predecessor; zero production callers remained.internal/plumbing/ebpf/doc.go,usidmap/vrf.go,internal/plumbing/srv6/usid.go,internal/gc/gc.gostill pointing at the deletedinternal/cni/bgp.go— updated tointernal/cnibgp/bgp.go.Verification
task lint✅task build(all 8 binaries) ✅task test:unit✅task test:e2enot run, same caveat as every prior PR in this stack.🤖 Generated with Claude Code