Skip to content

refactor(cni): extract galactic-route as its own CNI chain plugin - #306

Open
privateip wants to merge 1 commit into
refactor/cni-chain-2-galactic-bgpfrom
refactor/cni-chain-3-galactic-route
Open

refactor(cni): extract galactic-route as its own CNI chain plugin#306
privateip wants to merge 1 commit into
refactor/cni-chain-2-galactic-bgpfrom
refactor/cni-chain-3-galactic-route

Conversation

@privateip

@privateip privateip commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

Stack (merge bottom to top):


Summary

Fourth branch in the CNI plugin-chain split stack (based on #305). galactic-route is now its own chained CNI plugin — invoked between the master plugin and galactic-bgp per conflist order — instead of termination-route installation inside galactic-cni/galactic-tap-cni. Unlike every other binary in the chain, it has zero Kubernetes dependency: no CRD reads or writes, no namespace.

What moved

internal/cniroute mirrors cniipam/cnibgp's shape: RunPlugin(), PluginConf{VPC, VPCAttachment, Terminations}. parseConf reuses config.CNIConfig for LogFile/LogLevel precedence but never resolves NodeName or Kubeconfig — nothing here talks to the API server.

cmdAdd installs each termination as a VRF route via the existing internal/cni/route package, deriving the host device name from (vpc, vpcAttachment) alone — identical for a veth master's host end and a tap master's tap device, so no interface-kind inference is needed the way galactic-bgp needs it. It requires a non-nil prevResult and passes it through unchanged.

cmdDel is a no-op, matching the pre-split behavior: the old monolithic plugin's own DEL never deleted termination routes either — they're keyed by (vpc, vpcAttachment) and may still be in use by another pod/VM, so cleanup stays with galactic-router's GC controller. cmdCheck is checkTerminationRoutes, moved unchanged. cmdStatus is a trivial always-ready success, matching galactic-ipam's own STATUS.

Rollback scoping

resourceTracker covers only the termination routes this plugin's own ADD actually installed (route-delete only) — rollback never touches a route a failed route.Add call never reached.

Master plugin cleanup

internal/cni/internal/cnitap drop the Terminations field from their own PluginConf (each had its own copy of a Termination type — now lives only in cniroute), the route.Add loop and routesCreated tracker field, and the checkTerminationRoutes call from CHECK.

Verification

🤖 Generated with Claude Code

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).
@privateip
privateip force-pushed the refactor/cni-chain-3-galactic-route branch from 96f0f09 to c1704f7 Compare August 8, 2026 12:37
@mattdjenkinson

Copy link
Copy Markdown

Reviewed this one. Overall it's a clean, careful mirror of the sibling packages (cnibgp, cnitap), the resource tracking, rollback, and prevResult handling all follow the established pattern. Two real bugs turned up, though, one of which will break at runtime despite tests and vet passing.

cmd/galactic-route/main.go:67 never sets CNI_NETNS_OVERRIDE, and that will fail ADD/DEL for every VM/tap-mode attachment that has terminations. For a VM/tap-mode attachment, CNI_NETNS is deliberately set to the host's own root netns since there's no per-VM netns to enter, and galactic-route runs natively in that same host netns. Both galactic-cni and galactic-tap-cni handle this today by setting CNI_NETNS_OVERRIDE=true in their own process before calling into the skel library. galactic-route skips that step. The vendored CNI skel dispatcher checks ns.CheckNetNS(cmdArgs.Netns) after the plugin's own Add/Del already ran, sees that the plugin's namespace equals CNI_NETNS, and returns ErrInvalidNetNS unless that override env var is set in its own process, which it isn't here since it's a separately exec'd binary and env doesn't propagate across the chain. So the route actually gets installed correctly, but the CNI runtime sees ADD/DEL reported as failed for every VM/tap-mode attachment with terminations. The doc comment's reasoning, that galactic-route "never enters any network namespace" and so needs neither netns handling nor the override, conflates entering a namespace with the ambient-namespace-equality check the library actually performs.

internal/cniroute/ops_check.go:82 rejects valid on-link routes in CHECK. checkTerminationRoutes runs net.ParseIP(term.Via) and treats a nil result as an error before ever looking at the actual route table. But Termination.Via is omitempty, and assembleRoute in route.go has a dedicated branch for an empty Via that installs a valid on-link route, device-scoped, no gateway. cmdAdd installs this fine. CHECK, though, always fails with "invalid termination gateway """ for any on-link termination even though the route is present and correct. This logic was carried over byte for byte from the pre-split internal/cni/ops_check.go, so the PR relocated an existing bug into galactic-route's CHECK implementation rather than introducing a new one.

Two lower-severity things. docs/cni/configuration.md:100 still documents "terminations" as a field of the master plugin's own JSON config, which this PR removes. An operator following the doc would put terminations inside a galactic-cni or galactic-tap-cni stanza instead of a galactic-route one; the now-slimmer PluginConf silently drops the unknown field on unmarshal, ADD succeeds with no routes installed, and the master's own CHECK no longer checks routes either since that was removed here, so nothing signals that anything's wrong. And internal/cniroute/config.go:205's if conf.PrevResult != nil branch in parseConf is dead code, since PluginConf.PrevResult has json tag "-" and is never populated by json.Unmarshal. Production stays safe because cmdAdd separately validates through parsePrevResult, but this exact pattern is copy-pasted from internal/cni, internal/cnitap, and internal/cnibgp, so it predates this PR and is more of a systemic cleanup item than a regression here.

The netns-override miss is the one I'd block on, it'll break every VM/tap workload with terminations despite the test suite being green. The CHECK bug on on-link routes is worth fixing too since it's a genuine functional bug, just inherited rather than introduced by this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants