DO NOT MERGE — CI run for #315's merge ref - #323
Closed
ecv wants to merge 15 commits into
Closed
Conversation
Sequencing step 0 of the CNI plugin-chain split (design plan in ~/.vaults/notebook/projects/galactic/plan-cni-plugin-chain.md). galactic-cni is now veth-only; a new galactic-tap-cni binary owns tap-mode workloads (Kata, Firecracker, kraftlet/Unikraft). The interface_type config field and its runtime switch are gone entirely from both binaries — which binary runs is now the conflist-authoring decision, matching the design note's own rationale for the split. Getting there required extracting a few things this repo already had duplicated or coupled in ways that would have blocked genuine package separation: - internal/cni/hostconf: HostConf/loadHostConf/conflistEnvelope were duplicated near-verbatim between internal/cni and internal/installer, and hardcoded a match on plugin type "galactic-cni". Generalized into one shared package both binaries (and the installer) now call, with the accepted-type list passed in by the caller so a future binary doesn't need this package touched again. - internal/cni/crdnames: the annotation-key and BGP CRD naming helpers, shared between whatever writes BGP state and whatever reads it back (galactic-ipam's dealloc, until its own local persistence work lands). - internal/cni/nadpatch: NAD annotation logic, identical for both interface types, previously living unexported inside internal/cni. - internal/cniipam: the interface-agnostic IPAM allocation/deallocation logic (ipam_ops.go), now a shared library both master plugins import instead of galactic-tap-cni needing its own copy. Not yet a delegated CNI IPAM plugin of its own — that, plus dropping the CRD read dependency via local marker-file persistence for the IPv6 allocator, is sequencing step 1. - internal/cnibgp: the BGP/SRv6/eBPF publish logic (bgp.go), same shared- library treatment. Its own chain-invoked binary, and the CRD/eBPF-state CHECK logic it doesn't have yet, are sequencing step 2. internal/cni keeps its resourceTracker and CHECK/STATUS shape, just trimmed to veth-only; internal/cnitap gets its own smaller tracker with no guest- netns concerns at all. Taskfile.yaml and the galactic-cni Dockerfile build and stage galactic-tap-cni alongside the existing binaries in the same image, and installer.Bootstrap now copies it to the host too. Existing test coverage moved with the code it tests rather than being dropped: internal/cni/bgp_test.go and bgp_ebpf_test.go became internal/cnibgp's own tests (plus a small resource_ebpf_test.go left behind in internal/cni for resourceTracker's own eBPF-cleanup rollback path, which still lives there), ipam_ops_test.go became internal/cniipam's, nad_test.go became internal/cni/nadpatch's, and the tap-mode cases from cni_test.go became internal/cnitap's own suite. Verified: task lint, task build, and task test:unit are all green. task test:e2e needs a real Kind cluster bring-up (sudo modprobe vrf, kind create cluster) that wasn't attempted in this pass — the static per-node conflist Bootstrap writes is unchanged (still single-entry, node-settings only), so the existing veth-path e2e coverage should be unaffected, but that's unverified here. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…-cni This step's own split moved tap-mode ADD entirely into galactic-tap-cni's internal/cnitap and removed interface_type from internal/cni's own PluginConf — galactic-cni is veth-only now. This e2e test still invoked /galactic-cni directly with a config carrying interface_type: tap, a field that binary no longer reads at all, so its cmdAdd fell straight into the veth path and produced a 2-interface (host + guest) result instead of the 1-interface (host tap only) result the test's own assertions expect. Fixed to exec /galactic-tap-cni instead, with the config's "type" field updated to match (cosmetic here — this test invokes the binary directly by path, not through a real CNI runtime resolving "type" to a binary — but should still describe reality). Also dropped the srv6_locator field: PluginConf never had a field by that name at any point in this split: it was already dead, ignored JSON before this change and remains so after. Left everything else alone: the eBPF control daemon startup, bpf-fs mount, hostNetwork pod spec, "ipam" block, and GALACTIC_CNI_ENABLE_LOCAL_IPAM are all still exactly as correct for galactic-tap-cni at this point in the plugin-chain split as they were for the old monolithic galactic-cni's own tap branch — BGP/SRv6/eBPF publish is still called inline from cmdAdd here (that only moves out to its own chained binary, galactic-bgp, in a later step), so this test still needs the same BGPRouter fixture (scripts/ci.sh's e2etest case already creates one) and the same pre-pinned eBPF maps. Verification: task lint (0 issues), go build/vet clean, task test:unit full suite green. Not verified against a live cluster locally (no Kind/root here); pushing to let CI's own test-e2e job confirm.
Sequencing step 1 of the CNI plugin-chain split. galactic-ipam is now a
genuine CNI IPAM delegation target (github.com/containernetworking/plugins/
pkg/ipam.ExecAdd/ExecDel), not the in-process library call internal/cniipam
was left as at the end of step 0 — galactic-cni and galactic-tap-cni now
exec it, passing their own StdinData straight through (it already carries
the "ipam" block; the delegate ignores everything else in that JSON).
Lands the explicit IPAM contract the design note called for, ahead of any
real external IPAM system needing it:
- Whether IPAM runs at all is decided solely by whether "ipam" is present
in a master plugin's own config — no environment variable can trigger or
suppress that anymore. The old GALACTIC_CNI_ENABLE_LOCAL_IPAM "enforce
ipam block" check in internal/cni/cnitap's parseConf is gone outright.
- ipv6_subnet/ipv4_subnet/address_families move inside the "ipam" block
(internal/cniipam.IPAM's own fields) instead of living as PluginConf
siblings. Their CIDR validation moved with them, into galactic-ipam's own
parseConf — the master plugins no longer validate fields they don't own.
- ipam.type now names the delegated binary ("galactic-ipam"), not a
pool-vs-static mode selector. Mode is decided internally, from field
presence: static_ip selects the static path, otherwise ipv6_subnet/
ipv4_subnet select pool mode (either family alone, or both).
- enableLocalIPAM is renamed GALACTIC_IPAM_ENABLE_LOCAL_IPAM
(internal/config/ipam.go) and moves entirely into galactic-ipam's own
process: it's read once, by the delegate, and only fills in a default
IPv6 pool CIDR when "ipam" is present but specifies neither static_ip
nor a subnet for either family. It can no longer manufacture an "ipam"
block out of thin air.
The other half of this step — and the reason delegation could become
self-contained at all — is IPv6 pool persistence. PoolAllocator
(internal/cni/ipam) tracked allocations in memory only, which is silently
safe for ADD (each pool belongs to exactly one VPCAttachment, so there's
never a collision to guard against) but useless for DEL: a fresh process
has nothing to look up. Deallocation used to close that gap by reading the
allocated value back out of the BGPAdvertisement CRD annotation galactic-bgp
wrote — the one thing that would have kept galactic-ipam coupled to
galactic-bgp's CRD-naming scheme and a Kubernetes client it otherwise has
no use for at all. Fixed by giving PoolAllocator the same on-disk marker
file IPv4PoolAllocator already had (keyed by containerID, guarded by an
flock, one lock directory shared by both families now:
ipam.DefaultLockDir). Both allocators gained a DeallocateContainer(
containerID) and a peek-only LookupContainer(containerID), so galactic-ipam
now looks its own allocations up locally, and doesn't import a k8s client,
crdnames, or bgpv1alpha1 at all — decision 2 from the review notes, fully
realized: no kubeconfig, no RBAC surface, nothing.
internal/cniipam's shape changed accordingly: no more AllocConfig wrapper
(nothing needs vpc/vpcattachment/namespace once dealloc is local) — Allocate
and Deallocate/DeallocateContainer now take *IPAM directly. It gained its
own cmdAdd/cmdDel/cmdCheck/cmdStatus (github.com/containernetworking/cni/
pkg/skel) and CHECK logic that genuinely didn't exist before (confirming a
containerID's marker file is still present), plus a small result-conversion
helper (ResultToIPAMResult/BuildCNIResult) both sides of the delegation
boundary use to stay in the ips/routes-only shape the IPAM protocol expects
— no interfaces, ever; that stays the master plugin's job.
cmd/galactic-ipam, Taskfile.yaml, the galactic-cni Dockerfile, and
installer.Bootstrap all pick up the new binary the same way step 0 wired in
galactic-tap-cni.
Verified: task lint, task build, and task test:unit are all green
(internal/cniipam at 91% coverage, including a real ADD -> CHECK -> DEL ->
CHECK round trip proving the self-contained-by-containerID design holds
end to end). task test:e2e not run, same caveat as step 0's PR.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ation
This step rewired galactic-cni/galactic-tap-cni's own IPAM handling
from an in-process ipam.Allocate() call into real CNI IPAM delegation
(ipam.ExecAdd, execing whatever binary "ipam.type" names). The e2e
test's config still had the old "ipam": {"type": "pool"} block from
before this step -- "pool" is no longer a mode selector, it's now
looked up as a literal binary name, so ADD failed with "failed to find
plugin \"pool\" in path [/opt/cni/bin]" (confirmed on PR #303/#304's
own CI run, the first time e2e has actually executed against a live
cluster in this whole plugin-chain-split stack, now that a separate
pre-existing eBPF-artifact-drift issue blocking Build no longer blocks
it).
Fixed to "ipam": {"type": "galactic-ipam", "ipv6_subnet": "..."} --
the explicit contract this step establishes (see internal/cniipam's
doc comment). Also fixed CNI_PATH from /opt/cni/bin to / so
ipam.ExecAdd's lookup can actually find /galactic-ipam: every binary
in the chain lands at the image root
(containers/galactic-cni/Dockerfile), not /opt/cni/bin -- that path
only exists on a real host once installer.Bootstrap's init container
stages it there, which this test's pod never runs. Dropped
GALACTIC_CNI_ENABLE_LOCAL_IPAM=true from the script: it's now inert
for this config (an explicit ipv6_subnet doesn't need the local-IPAM
fallback), though harmless either way since this step doesn't remove
the env var itself.
BGP/SRv6/eBPF publish is still inline in galactic-tap-cni's own cmdAdd
at this point in the split (that only moves out to its own chained
binary in a later step), so the eBPF control daemon startup, bpf-fs
mount, and hostNetwork pod spec stay exactly as they were -- this
config still needs the same BGPRouter fixture and pre-pinned eBPF maps
the previous step's fix already established.
Verification: task lint (0 issues), go build/vet clean, task test:unit
full suite green. Not verified against a live cluster locally;
pushing to let CI's own test-e2e job confirm, same as the previous
step's fix.
"fd00:e2e:tap::/48" isn't a valid IPv6 literal -- "tap" contains 'p', not a hex digit. cniipam's own parseConf validates ipv6_subnet with net.ParseCIDR (see internal/cniipam/config.go's validateIPv6Subnet), so ADD failed with "invalid CIDR value for field 'ipam.ipv6_subnet'" as soon as this step's own IPAM-delegation fix (previous commit) made that field's value actually reach real validation for the first time -- it was never exercised before since PR #303/#304's e2e job never got this far until the eBPF-artifact-drift and wrong-binary-target issues in earlier commits on this branch were fixed. Fixed to "fd00:e2e::/48": drops the invalid "tap" hextet, keeps "e2e" (itself valid hex) as the mnemonic. Verification: task lint (0 issues), go build/vet clean, task test:unit full suite green, plus a plain net.ParseCIDR-equivalent check (Python's ipaddress.ip_network) confirming the literal parses.
Fixes raised by @mattdjenkinson on PR #304, roughly in the order he ranked them: - Rollback on a failed ADD never released the IPAM allocation. resourceTracker (internal/cni and internal/cnitap) now records IPAM delegation intent up front and calls ipam.ExecDel during cleanup — safe unconditionally since CNI IPAM delegation's DEL is idempotent, so this covers every failure path, not just ones past a confirmed ExecAdd. - The checked-in containerlab NAD configs (all 10 tenant nad.yaml files) still set ipv6_subnet/ipv4_subnet/address_families at the config root instead of nested under "ipam", so PluginConf.IPAM stayed nil and ADD silently produced no address. Migrated all of them to the nested "ipam" block shape with "type": "galactic-ipam". - Allocate() didn't check for an existing allocation before handing out a new one, so a CNI ADD retry for the same container leaked the prior marker file. Both PoolAllocator (IPv6) and IPv4PoolAllocator now look up containerID first and return the existing allocation. Surfaced a related bug while fixing this: reconstructing the stored subnet via net.ParseCIDR re-masks the IP and zeroes the byte incSubnet uses to distinguish subnets, collapsing every allocation back to the reserved gateway subnet — replaced with a mask- preserving parse (parseAllocatedSubnet). - CHECK never validated IPAM state. Both ops_check.go now delegate to ipam.ExecCheck when an "ipam" block is configured. - GALACTIC_IPAM_ENABLE_LOCAL_IPAM was re-read independently by deallocate/checkAllocation, so it flipping between a pod's ADD and its later DEL/CHECK silently skipped cleanup. Both now resolve the same default pool whenever neither subnet was ever configured, without re-consulting the env var. - DeallocateContainer's scan-then-remove ran under two separate flock acquisitions, leaving a window for a concurrent process to interleave. New internal/cni/ipam/lockedstate.go makes every locked operation atomic under one flock, shared by both allocators. - Extracted the shared locking/scanning logic into lockedState so IPv6 and IPv4 allocators no longer duplicate it. - Renamed internal/cniipam/config_test.go subtests to UpperCamelCase per repo convention. The O(n) pool-scan performance concern (findContainerMarker reading every marker file under the site-wide IPv4 pool's flock) is left as a follow-up — it needs an on-disk format change. Verified: go build ./..., go vet ./..., go test ./internal/... -race (one unrelated pre-existing failure: iptables binary not available in this sandbox), golangci-lint run ./... (0 issues), yamlfmt -lint (clean). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Step 2 of the CNI plugin-chain split (galactic/plan-cni-plugin-chain):
pulls BGP/SRv6/eBPF publishing out of the veth and tap master plugins
into its own chained CNI binary, galactic-bgp, invoked last in the
conflist after galactic-cni/galactic-tap-cni and galactic-ipam.
internal/cnibgp is the new plugin package:
- cnibgp.go: RunPlugin() entrypoint (skel.PluginMainFuncs, ADD/DEL/
CHECK/STATUS/VERSION), mirroring the shape of cniipam/cnitap.
- types.go, config.go: PluginConf{VPC, VPCAttachment, Namespace},
parsed from stdin. Deliberately reuses internal/config.CNIConfig
(GALACTIC_CNI_* env vars) rather than inventing its own prefix like
galactic-ipam did — these are shared node-level settings (API server
address, namespace, etc.), not a BGP-specific concern, so there is
no reason to duplicate or rename them for this binary.
- prevresult.go: inferFromPrevResult() reconstructs everything
galactic-bgp needs (interface kind — veth vs tap, and the allocated
IPAM result) purely from the previous plugin's prevResult, since
galactic-bgp itself never touches the container's network namespace.
Interface kind is inferred from prevResult shape: two interfaces
means veth (host+container), one means tap.
- resource.go: a resourceTracker scoped to exactly what galactic-bgp's
own ADD creates — BGPVRFInstance, BGPAdvertisement, and the eBPF
vrf_table entry. This is intentionally smaller than the old single
process-wide tracker: the kernel-interface/VRF cleanup that used to
live alongside these now belongs to each master plugin's own
tracker (internal/cni, internal/cnitap), scoped to exactly what its
own ADD creates. Selective rollback stays correct because each
plugin only ever rolls back what it itself created.
- ops_add.go/ops_del.go/ops_check.go: cmdAdd parses config, infers
from prevResult, publishes BGP state, and passes prevResult through
unchanged (galactic-bgp is the last plugin in the chain). cmdDel is
a no-op everywhere in the chain, as decided in the plan — orphaned
VRF/CRD state is reconciled independently by galactic-router's GC
controller, not by any CNI DEL path, so cmdDel does not need to
distinguish "resources this plugin created" from anything else.
cmdCheck is new logic (the old bgp.go had no CHECK story of its own):
it verifies the BGPVRFInstance and BGPAdvertisement CRDs exist and
cross-checks the eBPF vrf_table entry via a new checkEBPFEntry
helper. cmdStatus probes the API server, matching the STATUS story
every other binary in the chain now implements (per plan decision).
internal/cnibgp/bgp.go keeps the actual BGP/SRv6/eBPF logic moved over
from internal/cni/bgp.go, with everything now unexported since it is
package-internal to cnibgp rather than shared across internal/cni:
publishConfig, publishResult, publishBGPState, egressKindForInterface-
Type, unregisterEBPFDatapath, plus the untouched allocation/collision/
CRD-building helpers (allocateArgument, checkArgumentCollision,
lookupBGPRouter, buildVRFInstanceSpec, buildAdvertisementSpec,
ipamAdvertisementPrefixes, allAdvertisedPrefixes, registerEBPFDatapath,
isTransientError/retryK8sOps).
Design refinement beyond the original plan: ConfigureHostGateway and
its helpers (installGatewayNeighbor, ipv4GatewayAddrParams,
installGatewayRoute, routeConflicts) do NOT move into galactic-bgp.
They configure the container's default route to the VRF gateway
address, which is kernel-interface/netns work — exactly the kind of
dependency the plan's own rationale for splitting BGP out says
galactic-bgp should have zero of. Moving them into galactic-bgp would
have reintroduced that dependency one step later in the chain instead
of removing it. They now live in a new internal/cni/hostgw package,
called directly by both master plugins (galactic-cni and
galactic-tap-cni) right after they configure the container interface,
before handing off down the chain.
Also discovered while wiring inferFromPrevResult: types.PluginConf's
PrevResult field (from containernetworking/cni/pkg/types) has a json
tag of "-" and is never populated by json.Unmarshal; only the sibling
RawPrevResult map[string]interface{} field (tag "prevResult,omitempty")
actually receives the previous plugin's result. This is a pre-existing
quirk of that library, not something introduced by this split, and
existing code elsewhere in this repo already works around it by
reading RawPrevResult directly (e.g. ops_check.go). inferFromPrevResult
follows that same existing pattern. Left as-is rather than fixed here,
since fixing it is unrelated to this split's scope.
Taskfile.yaml, containers/galactic-cni/Dockerfile, and
internal/installer/installer.go (SourceBGPBinary) gain galactic-bgp
alongside galactic-cni/galactic-tap-cni/galactic-ipam, following the
exact same pattern established for those two in prior steps.
internal/cni/resource.go's resourceTracker drops all BGP/eBPF fields,
leaving only vpc, vpcAttachment, vrfCreated, routesCreated — cleanup()
no longer needs a Kubernetes client at all, since it never touches BGP
CRDs. internal/cni/result.go's buildVethResult calls hostgw.Configure-
HostGateway directly and returns only an error rather than threading
an IPAMResult/MAC address back up for the caller to hand to a bgp
helper that no longer lives in this package. Mirrored identically in
internal/cnitap.
Verification: task lint (0 issues), task build, task test:unit all
green. task test:e2e not run in this step (matches prior two steps in
this stack — requires sudo modprobe vrf plus a Kind cluster bring-up,
deferred to the end of the full stack per the plan's verification
approach).
Rollback context (blocking): rollbackCtx was created up front with a single 10s budget and reused for tracker.cleanup() after publishBGPState's own retryK8sOps could already burn ~30s across its retries. A slow API server could leave cleanup with an expired context, turning Delete's "NotFound" into "context deadline exceeded" (which client.IgnoreNotFound doesn't catch), leaking the just-created CRDs. rollbackCancel was also only called on the error branch, leaking a timer on every successful ADD. Now rollbackCtx is created fresh, with its own full budget, only inside the failure branch. CHECK coverage (blocking): checkEBPFEntry only read back vrf_table, so a corrupted/missing locator_table or function_table entry, or a nodeID that drifted out of range after ADD, still reported the attachment healthy. It now also verifies locator_table, function_table, and the nodeID range, matching what ADD treats as a hard error. e2e coverage (blocking): TestCNITapInterface's comment claimed eBPF registration happens inline in galactic-tap-cni's cmdAdd, which stopped being true with the plugin-chain split, and the test never exercised galactic-bgp at all -- silently losing coverage of BGP CRD creation and eBPF registration on the ADD path. It now chains /galactic-bgp ADD (fed the tap master's real result as prevResult) and CHECK after the tap master's own ADD, asserting the BGPVRFInstance/BGPAdvertisement CRDs exist and that CHECK -- which reads back all three eBPF tables -- passes. cniVersion constraint: type100.NewResult only accepts "1.0.0"/"1.1.0", and the master plugin echoes the conflist's own cniVersion straight into its printed Result, so an older value fails galactic-bgp's ADD for every attachment. Documented as an explicit, intentional requirement in prevresult.go and docs/cni/configuration.md, with a test locking in the rejection of older versions. DEL version/logging: cmdDel never parsed args.StdinData, always printed cniVersion "1.0.0" unconditionally (unlike internal/cni/ops_del.go, which only falls back to that on a parse failure), and never logged vpc/vpcAttachment. It now parses the conflist, logs the attachment, and uses pluginConf.CNIVersion with the same fallback-on-parse-failure pattern as the other DEL implementations in this chain. Interface-kind inference: veth/tap was inferred purely from len(Interfaces) (1 vs 2), enforced only in comments -- #306 chaining galactic-route into this same prevResult could add a host-side interface and silently misclassify tap as veth (or vice versa) instead of failing loudly, since both counts are valid switch cases. inferFromPrevResult now counts Sandbox-carrying interfaces instead: the actual property that distinguishes veth (guest end moved into the container netns) from tap (host-only), which survives an extra host-side interface without misclassifying. Smaller cleanup: - Removed the stale bgpv1alpha1.AddToScheme registration (with a circular justifying comment) from internal/cni/resource.go and internal/cnitap/resource.go -- neither package touches BGP CRDs anymore. - Added resource_test.go covering resourceTracker.cleanup's own wiring end to end (all three resource kinds, plus the rollback-collision race at that level) -- previously only the standalone unregisterEBPFDatapath was tested directly. - resourceTracker now embeds publishResult instead of cmdAdd copying its five tracking fields over one by one, so a future field added to one can't silently stop being tracked in the other. - Removed egressKindForInterfaceType's dead empty-string branch -- inferFromPrevResult always produces "veth" or "tap" now. Also added prevresult_test.go and ops_del_test.go, and introduced a package-level ebpfPinDir var (defaulting to attach.PinDir) so tests can redirect galactic-bgp's own eBPF registration/rollback/CHECK reads to a throwaway pin directory instead of the real production one. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Step 3 of the CNI plugin-chain split (galactic/plan-cni-plugin-chain):
pulls termination-route installation out of the veth and tap master
plugins into its own chained CNI binary, galactic-route, invoked
between the master plugin and galactic-bgp per conflist order. Unlike
every other binary in the chain, galactic-route has zero Kubernetes
dependency — it neither reads nor writes any CRD, and never needs a
namespace.
internal/cniroute is the new plugin package, mirroring the shape
established by cniipam/cnibgp:
- cniroute.go: RunPlugin() entrypoint (skel.PluginMainFuncs, ADD/DEL/
CHECK/STATUS/VERSION).
- types.go/config.go: PluginConf{VPC, VPCAttachment, Terminations},
parsed from stdin — the same document the master plugin itself
received, since the CNI runtime passes every chain entry its own
stanza plus prevResult. parseConf still reuses config.CNIConfig for
LogFile/LogLevel's env-var > conflist > default precedence (so
logging behaves identically to every other binary), but — unlike
galactic-bgp — never resolves NodeName or Kubeconfig, since nothing
here ever talks to the API server.
- ops_add.go: cmdAdd installs each termination as a VRF route via the
existing internal/cni/route package (route.Add), deriving the host
device name from (vpc, vpcAttachment) alone via
intf.GenerateInterfaceNameHost — identical for a veth master's host
end and a tap master's tap device, so galactic-route needs no
interface-kind inference the way galactic-bgp does. It then passes
prevResult through unchanged, adding no interfaces or IPs of its
own. Requires a non-nil prevResult (galactic-route must be chained
after a master plugin) and reads it from RawPrevResult, not the
never-populated typed PrevResult field.
- ops_del.go: cmdDel is a no-op, same as every other binary in the
chain — termination routes are keyed by (vpc, vpcAttachment) and may
still be in use by another pod/VM sharing the same attachment, so
cleanup is left entirely to galactic-router's GC controller. This
matches the pre-split behavior too: the old monolithic plugin's own
DEL never deleted termination routes either, for the same reason —
extracting this into its own binary changes nothing about when
routes actually get removed.
- ops_check.go: cmdCheck is checkTerminationRoutes, moved unchanged
from internal/cni/ops_check.go (also mirrored in internal/cnitap).
cmdStatus is a trivial always-ready success — galactic-route has
nothing external to probe, matching galactic-ipam's own STATUS,
implemented for uniformity across the chain per the plan's decision
rather than skipped.
- resource.go: a resourceTracker scoped to exactly what galactic-
route's own ADD creates — the termination routes it actually
installed (route-delete only). Rollback deletes only the routes
recorded as added, never routes a failed route.Add call never
reached.
internal/cni and internal/cnitap: dropped the Terminations field from
each PluginConf (both packages had their own copy of a Termination
type, now living only in cniroute since neither master plugin reads
"terminations" out of its own stanza anymore), the route.Add loop and
routesCreated tracker field from ops_add.go/resource.go, and the
checkTerminationRoutes call from ops_check.go's CHECK path (the
function itself moved to cniroute, verbatim).
Taskfile.yaml, containers/galactic-cni/Dockerfile, and
internal/installer/installer.go (SourceRouteBinary) gain galactic-route
alongside the four other chain binaries, following the exact pattern
established for those in steps 0-2.
Verification: task lint (0 issues), task build (all 8 binaries,
including galactic-route), task test:unit all green.
internal/cniroute lands at 62.9% coverage — its first-ever test
coverage, since internal/cni/route (the package it wraps) had none
before this split either; backfilling that package's own tests is
unrelated to this split's scope and left as-is. task test:e2e not run
in this step, same caveat as steps 0-2 (requires sudo modprobe vrf plus
a Kind cluster bring-up, deferred to the end of the full stack per the
plan's verification approach).
CNI_NETNS_OVERRIDE (blocking): galactic-route never entered any netns, so cmd/galactic-route/main.go assumed it never needed the stdin peek-and-repipe dance or CNI_NETNS_OVERRIDE that galactic-cni/ galactic-tap-cni use. That's true for veth-mode attachments, where CNI_NETNS points at the container's netns and differs from this process's own ambient (host) netns. It's false for tap-mode attachments: CNI_NETNS is deliberately set to the host's own root netns there (no per-VM netns exists), which equals this process's ambient netns, so skel's post-Add/Del same-netns check rejected every tap-mode ADD/DEL with terminations even though the route was already installed correctly. Now peeks stdin for interface_type the same way galactic-cni does and sets CNI_NETNS_OVERRIDE=true only for tap mode. CHECK on-link routes: checkTerminationRoutes unconditionally called net.ParseIP on Via and errored on nil, but Via is omitempty and assembleRoute (route.go) has a real branch that installs a valid on-link route for an empty Via, which cmdAdd installs fine. CHECK always failed with "invalid termination gateway" for those regardless. Restructured the match loop to treat an empty Via as looking for a gateway-less, device-scoped route instead of erroring immediately. Carried over byte for byte from the pre-split internal/cni/ops_check.go (also reachable via internal/cnitap), so this fixes the same bug there too by virtue of the code having moved. Docs: docs/cni/configuration.md still listed terminations as a galactic-cni/galactic-tap-cni field and showed it inline in the master's own JSON, which this PR's PluginConf split made wrong -- the master's slimmer struct silently drops the field on unmarshal, so an operator following the doc gets a silent no-op. Moved the field out of the master's Top-Level Fields table, reworded Termination Fields to attribute it to galactic-route's own conflist stanza (including that cmdDel is a no-op, not "deleted in reverse order"), and rewrote the worked example as a chained plugins array. Deferred per the review: the internal/cniroute/config.go:205 dead `if conf.PrevResult != nil` branch is copy-pasted across internal/cni, internal/cnitap, and internal/cnibgp too, predating this PR -- left for #307, which already scopes "dead code" cleanup for the chain split. Verification: task lint, task build (all 8 binaries), task test:unit all pass. task test:e2e not run, same caveat as #303/#304/#305 -- this repo has no root/CAP_NET_ADMIN available, and the existing checkTerminationRoutes tests already can't get past the vrf.TableID lookup without a real kernel VRF, so the on-link CHECK fix has no new automated regression test beyond what task test:e2e's Kind cluster would exercise. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ab, dead code Rebased onto the updated refactor/cni-chain-3-galactic-route (which picked up PR #306's own review-feedback fix commit, fe72a1e) after that branch's history moved out from under this one. Conflicts resolved: - All 11 containerlab tenant NAD manifests: kept this PR's plugins-array chain wrapper (adding the galactic-bgp stage) around the nested "ipam": {"type": "galactic-ipam", ...} block that PR #305's own fix commit had already introduced independently of this PR -- the two changes were orthogonal, so the merge is additive. - docs/cni/configuration.md: took this PR's fuller rewrite throughout (it supersedes PR #306's narrower doc fix -- e.g. this PR already covers the interface_type/terminations field removal and the renamed GALACTIC_IPAM_ENABLE_LOCAL_IPAM env var more completely), but preserved two things #306 fixed that this PR's diff predates and doesn't otherwise cover: the cniVersion 1.0.0/1.1.0 prevResult constraint paragraph, and "on-link route" (not "link-local route" -- fd01::/48 in the example isn't a link-local address) in the terminations example. - tests/e2e/e2e_test.go: this PR's diff removed startEBPFControlDaemon (call, definition, and the attach import) on the theory that TestCNITapInterface never touches the eBPF datapath. That was true when this PR's diff was authored, but PR #305's own fix commit (7cf773a) had independently added testChainedGalacticBGP, chaining galactic-bgp after the tap master's ADD and asserting BGPVRFInstance/ BGPAdvertisement CRD creation -- and registerEBPFDatapath's usidmap.OpenPinnedRegistry only opens already-pinned maps, it never loads/pins the eBPF program itself, so testChainedGalacticBGP can't succeed without startEBPFControlDaemon having run first. Restored the call, its definition, and the import, and updated the doc comments this PR had already rewritten (which claimed the test "does not chain into ... galactic-bgp") to describe the merged reality instead. Verification: task lint, task build (all 8 binaries), task test:unit all pass on the rebased tree. tests/e2e not run in this sandbox (no Kind cluster / root), same caveat as every PR in this stack. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…nadpatch Rebased onto the updated refactor/cni-chain-4-installer-docs (which picked up PR #307's own review-feedback fix commit) after that branch's history moved out from under this one -- same ripple as #306 -> #307 -> #311. Conflicts resolved: internal/cnibgp/bgp.go, ops_check.go, and resource.go each had an import-block conflict from this PR's internal/cni/crdnames -> internal/crdnames promotion landing on lines the current tree had already changed independently (PR #305's own fix commit moved cnibgp's eBPF pin-dir handling behind a package- level ebpfPinDir var in cnibgp.go, so bgp.go/ops_check.go/resource.go no longer import internal/plumbing/ebpf/attach directly the way this PR's diff -- authored before that -- expected). Resolved by applying just the crdnames rename to each file and leaving the attach import out, matching how the current tree already gets ebpfPinDir. Also fixed one file this PR's diff never touched: internal/cnibgp/ resource_test.go didn't exist yet at this PR's original base -- it was added later by #305's own fix commit -- so its own "go.datum.net/galactic/internal/cni/crdnames" import needed the same promotion this PR already applied everywhere else, or the package wouldn't build. Verification: task lint, task build (all 8 binaries), task test:unit all pass on the rebased tree, including the three promoted packages (internal/crdnames, internal/hostconf, internal/nadpatch). tests/e2e not run in this sandbox (no Kind cluster / root), same caveat as every PR in this stack. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
TestCNITapInterface now chains galactic-bgp after the tap master plugin, and galactic-bgp registers the eBPF uSID datapath. Pinning its maps needs the node's real bpffs visible inside the test pod, so the pod override must carry the same bpf-fs hostPath volume and mount that config/cni/daemonset.yaml uses in production. Without it the test fails with: start eBPF uSID datapath: attach: create bpf map pin directory "/sys/fs/bpf/galactic": mkdir /sys/fs/bpf/galactic: no such file or directory Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
fix(e2e): restore bpf-fs mount for the tap chain test
Contributor
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Throwaway. Replicates what #315 would test now: its base branch
refactor/cni-chain-4-installer-docs(which now carries the #317bpf-fsfix) merged with #315's head49bc55f. Closed as soon as the checks report.Peter's branches are untouched — #315 itself has never had a CI run of its own, per #319.
Related to #315, #319