From 6b30969c11df6f805a329d0fd82d3be48b60f182 Mon Sep 17 00:00:00 2001 From: Peter Sprygada Date: Tue, 6 Oct 2026 22:21:53 -0400 Subject: [PATCH 1/3] fix: Keep sidecar and host VRFs apart in the shared eBPF maps The galactic-vrf sidecar writes into the same pinned maps as the host CNI, from inside its pod's network namespace. egress_route_table is keyed by VRF table ID and ifindex_vrf_table by interface index, and each namespace hands both out on its own from 1 upward. A sidecar VRF and a host tenant VRF on one edge node could therefore overwrite each other's routes, and usid_egress could route one's traffic into the other's VRF. The two writers now use disjoint ranges: - Host VRF tables 1 to 0xEFFF, skipping the kernel's reserved tables 253 to 255. The installer's sidecar return-path tables keep 0xF000 to 0xFFFF in the host namespace. - Sidecar VRF tables 0x10000 plus the uSID Argument, through vrf.AddInRange. argumentForTableID becomes an offset from that base. - Sidecar veth peers are created with the explicit index 0x40000000 plus the Argument. IfindexVRFTable.Register rejects a sidecar row with any other index and a host row at or above 0x40000000. SidecarOwnedTableIDs still reads ownership from vrf_table, so sidecar pods running the old version keep being recognised during a rollout. AddInRange rejects an existing VRF whose table is outside the caller's range, so an old pod must be replaced rather than restarted in place on the new image. Fixes #716 Co-Authored-By: Claude Opus 5.5 --- internal/ingresssidecar/backend.go | 2 +- internal/ingresssidecar/ebpfdatapath.go | 59 +++++++++----- .../ebpfdatapath_integration_test.go | 20 +++-- internal/ingresssidecar/ebpfdatapath_test.go | 20 +++-- internal/installer/sidecarreturn.go | 6 +- internal/installer/sidecarreturn_test.go | 14 ++++ .../plumbing/ebpf/egressroutemap/owner.go | 7 +- .../plumbing/ebpf/ifindexvrfmap/ifindexvrf.go | 40 ++++++++++ .../ebpf/ifindexvrfmap/ifindexvrf_test.go | 43 ++++++++++ internal/plumbing/vrf/vrf.go | 61 ++++++++++++--- internal/plumbing/vrf/vrf_internal_test.go | 78 +++++++++++++++++++ internal/plumbing/vrf/vrf_test.go | 27 +++++++ 12 files changed, 327 insertions(+), 50 deletions(-) create mode 100644 internal/plumbing/vrf/vrf_internal_test.go diff --git a/internal/ingresssidecar/backend.go b/internal/ingresssidecar/backend.go index 2f3fa152..1bafc567 100644 --- a/internal/ingresssidecar/backend.go +++ b/internal/ingresssidecar/backend.go @@ -67,7 +67,7 @@ type kernelBackend struct{} func NewKernelBackend() Backend { return kernelBackend{} } func (kernelBackend) EnsureVRF(vpc string) (uint32, error) { - if err := vrf.Add(vpc); err != nil { + if err := vrf.AddInRange(vpc, sidecarTableIDMin, sidecarTableIDMax); err != nil { return 0, fmt.Errorf("create VRF for vpc %s: %w", vpc, err) } tableID, err := vrf.TableID(vpc) diff --git a/internal/ingresssidecar/ebpfdatapath.go b/internal/ingresssidecar/ebpfdatapath.go index a07cc1e6..31d7cdf6 100644 --- a/internal/ingresssidecar/ebpfdatapath.go +++ b/internal/ingresssidecar/ebpfdatapath.go @@ -56,22 +56,31 @@ var ebpfPinDir = attach.PinDir // real locator the way the CNI path does. const ingressSidecarBlock = uformat.BlockIngressSidecar +// Routing table IDs this sidecar allocates its VRFs from: the range just above +// vrf.SidecarTableIDBase, one per possible uSID Argument. Disjoint from the +// host's range, because egress_route_table is keyed by table ID alone and both +// namespaces write it; see vrf.SidecarTableIDBase. +const ( + sidecarTableIDMin = vrf.SidecarTableIDBase + uint32(uformat.ArgumentMin) + sidecarTableIDMax = vrf.SidecarTableIDBase + uint32(uformat.ArgumentMax) +) + // argumentForTableID derives the uSID Argument for a VPC's VRF from its Linux -// routing table ID rather than allocating a separate value. The table ID is -// already unique per VPC on this node, which is the only property Argument -// needs here, so reusing it avoids a second allocator. +// routing table ID rather than allocating a separate value: the table ID's +// offset from vrf.SidecarTableIDBase. The table ID is already unique per VPC +// in this pod, which is the only property Argument needs here, so reusing it +// avoids a second allocator. // -// Fails if tableID does not fit Argument's 12-bit range. Table IDs are -// allocated from 1 upward, so this needs about 4095 live VPCs on one node, but -// a silently aliased Argument would be a cross-VPC datapath bug rather than a +// Fails if tableID is outside [sidecarTableIDMin, sidecarTableIDMax]. A +// silently aliased Argument would be a cross-VPC datapath bug rather than a // safe fallback. func argumentForTableID(tableID uint32) (uint16, error) { - if tableID < uint32(uformat.ArgumentMin) || tableID > uint32(uformat.ArgumentMax) { + if tableID < sidecarTableIDMin || tableID > sidecarTableIDMax { return 0, fmt.Errorf( - "ingresssidecar: VRF table id %d does not fit uSID Argument's range [%#x,%#x]", - tableID, uint16(uformat.ArgumentMin), uint16(uformat.ArgumentMax)) + "ingresssidecar: VRF table id %d is outside the sidecar's range [%d,%d]", + tableID, sidecarTableIDMin, sidecarTableIDMax) } - return uint16(tableID), nil + return uint16(tableID - vrf.SidecarTableIDBase), nil } // vrfLinkForTable returns the kernel VRF link whose routing table is tableID, @@ -93,8 +102,8 @@ func vrfLinkForTable(tableID uint32) (*netlink.Vrf, error) { // egressVethNames derives a VPC's veth pair names from tableID alone, since the // callers carry only a tableID forward. Collision-free because table IDs are -// already unique per VPC on this node, and comfortably inside IFNAMSIZ, since a -// table ID that fits Argument's 12-bit range is at most 4 decimal digits. +// already unique per VPC in this pod, and comfortably inside IFNAMSIZ, since a +// table ID in the sidecar's range is at most 5 decimal digits. func egressVethNames(tableID uint32) (inner, peer string) { return fmt.Sprintf("ivs%d", tableID), fmt.Sprintf("ivp%d", tableID) } @@ -106,19 +115,28 @@ func egressVethNames(tableID uint32) (inner, peer string) { // redoes its route lookup inside the VRF. peer stays outside the VRF, in the // pod's main namespace, and is where usid_egress actually attaches. // -// Idempotent: an existing name counts as already done, and the route is -// replaced rather than added, so it is safe on every call that ensures a -// VRF. -func ensureEgressVeth(vrfLink *netlink.Vrf, inner, peer string) (netlink.Link, error) { +// peer is created with the explicit interface index peerIndex, because its +// index keys ifindex_vrf_table, which the host writes its own indexes into; +// see ifindexvrfmap.SidecarIfindexBase. It is therefore the veth's primary +// link and inner its peer, since netlink only sets the primary's index. +// +// Idempotent: an existing name with the right index counts as already done, +// and the route is replaced rather than added, so it is safe on every call +// that ensures a VRF. An existing peer with any other index is an error, since +// registering it would put a row in the host's index range. +func ensureEgressVeth(vrfLink *netlink.Vrf, inner, peer string, peerIndex uint32) (netlink.Link, error) { peerLink, err := netlink.LinkByName(peer) + if err == nil && uint32(peerLink.Attrs().Index) != peerIndex { + return nil, fmt.Errorf("veth peer %q has index %d, want %d", peer, peerLink.Attrs().Index, peerIndex) + } if err != nil { var notFound netlink.LinkNotFoundError if !errors.As(err, ¬Found) { return nil, fmt.Errorf("look up veth peer %q: %w", peer, err) } veth := &netlink.Veth{ - LinkAttrs: netlink.LinkAttrs{Name: inner}, - PeerName: peer, + LinkAttrs: netlink.LinkAttrs{Name: peer, Index: int(peerIndex)}, + PeerName: inner, } if err := netlink.LinkAdd(veth); err != nil { return nil, fmt.Errorf("add veth pair %q/%q: %w", inner, peer, err) @@ -279,7 +297,7 @@ func ensureEgressDatapath(vpc string, tableID uint32) error { } inner, peer := egressVethNames(tableID) - peerLink, err := ensureEgressVeth(vrfLink, inner, peer) + peerLink, err := ensureEgressVeth(vrfLink, inner, peer, ifindexvrfmap.SidecarIfindex(argument)) if err != nil { return fmt.Errorf("ensure egress veth for VRF table %d: %w", tableID, err) } @@ -313,7 +331,8 @@ func ensureEgressDatapath(vpc string, tableID uint32) error { defer func() { _ = ifindexCloser.Close() }() // Keyed by the veth peer's ifindex, not the VRF's, because usid_egress has - // to see this traffic arrive on that interface's ingress hook. + // to see this traffic arrive on that interface's ingress hook. That index + // is ifindexvrfmap.SidecarIfindex(argument), set by ensureEgressVeth. if err := ifindexTable.Register(uint32(peerLink.Attrs().Index), ingressSidecarBlock, argument); err != nil { return fmt.Errorf("register eBPF ifindex_vrf_table entry: %w", err) } diff --git a/internal/ingresssidecar/ebpfdatapath_integration_test.go b/internal/ingresssidecar/ebpfdatapath_integration_test.go index 4e402206..6cedcadb 100644 --- a/internal/ingresssidecar/ebpfdatapath_integration_test.go +++ b/internal/ingresssidecar/ebpfdatapath_integration_test.go @@ -16,6 +16,7 @@ import ( "go.datum.net/galactic/internal/plumbing/ebpf/attach" "go.datum.net/galactic/internal/plumbing/ebpf/ifindexvrfmap" "go.datum.net/galactic/internal/plumbing/ebpf/usidmap" + "go.datum.net/galactic/internal/plumbing/vrf" ) func requireRoot(t *testing.T) { @@ -99,6 +100,15 @@ func assertMapState( ifindexTable *ifindexvrfmap.IfindexVRFTable, registry *usidmap.Registry, vrfLink *netlink.Vrf, peerLink netlink.Link, tableID uint32, ) error { + argument := uint16(tableID - vrf.SidecarTableIDBase) + + // The peer's index is in the range the host never registers, so a host + // interface with the same number in its own namespace cannot overwrite + // this row (#716). + if want := ifindexvrfmap.SidecarIfindex(argument); uint32(peerLink.Attrs().Index) != want { + return fmt.Errorf("veth peer ifindex = %d, want %d", peerLink.Attrs().Index, want) + } + entry, ok, err := ifindexTable.Get(uint32(peerLink.Attrs().Index)) if err != nil { return fmt.Errorf("look up ifindex_vrf_table[peer]: %w", err) @@ -106,9 +116,9 @@ func assertMapState( if !ok { return errors.New("ifindex_vrf_table has no entry for the veth peer's ifindex") } - if entry.Block != ingressSidecarBlock || entry.Argument != uint16(tableID) { + if entry.Block != ingressSidecarBlock || entry.Argument != argument { return fmt.Errorf("ifindex_vrf_table[peer] = (block=%#x, argument=%d), want (block=%#x, argument=%d)", - entry.Block, entry.Argument, ingressSidecarBlock, tableID) + entry.Block, entry.Argument, ingressSidecarBlock, argument) } if _, ok, err := ifindexTable.Get(uint32(vrfLink.Attrs().Index)); err != nil { @@ -117,7 +127,7 @@ func assertMapState( return errors.New("ifindex_vrf_table has an entry keyed by the VRF's own ifindex, want none") } - vrfEntry, ok, err := registry.VRF.Get(ingressSidecarBlock, uint16(tableID)) + vrfEntry, ok, err := registry.VRF.Get(ingressSidecarBlock, argument) if err != nil { return fmt.Errorf("look up vrf_table entry: %w", err) } @@ -141,7 +151,7 @@ func assertTornDown( } else if ok { return errors.New("ifindex_vrf_table[peer] still present after removeEgressDatapath") } - if _, ok, err := registry.VRF.Get(ingressSidecarBlock, uint16(tableID)); err != nil { + if _, ok, err := registry.VRF.Get(ingressSidecarBlock, uint16(tableID-vrf.SidecarTableIDBase)); err != nil { return fmt.Errorf("look up vrf_table entry after removal: %w", err) } else if ok { return errors.New("vrf_table entry still present after removeEgressDatapath") @@ -166,7 +176,7 @@ func TestEnsureEgressDatapath_AttachesToVethPeerNotVRF(t *testing.T) { } t.Cleanup(func() { _ = nsObj.Close() }) - const tableID = uint32(7) + const tableID = vrf.SidecarTableIDBase + 7 const vrfName = "ivstestvrf" err = nsObj.Do(func(_ ns.NetNS) error { diff --git a/internal/ingresssidecar/ebpfdatapath_test.go b/internal/ingresssidecar/ebpfdatapath_test.go index 7788578e..de5b42e0 100644 --- a/internal/ingresssidecar/ebpfdatapath_test.go +++ b/internal/ingresssidecar/ebpfdatapath_test.go @@ -10,21 +10,26 @@ import ( "golang.org/x/sys/unix" "go.datum.net/galactic/internal/plumbing/ebpf/uformat" + "go.datum.net/galactic/internal/plumbing/vrf" ) func TestArgumentForTableID(t *testing.T) { + const base = vrf.SidecarTableIDBase cases := []struct { name string tableID uint32 want uint16 wantErr bool }{ - {name: "typical small table id", tableID: 1, want: 1}, - {name: "another typical table id", tableID: 3, want: 3}, - {name: "argument min boundary", tableID: uint32(uformat.ArgumentMin), want: uint16(uformat.ArgumentMin)}, - {name: "argument max boundary", tableID: uint32(uformat.ArgumentMax), want: uint16(uformat.ArgumentMax)}, - {name: "zero table id is out of range (Argument 0 is reserved)", tableID: 0, wantErr: true}, - {name: "table id past the 12-bit Argument range", tableID: uint32(uformat.ArgumentMax) + 1, wantErr: true}, + {name: "first sidecar table id", tableID: base + 1, want: 1}, + {name: "another sidecar table id", tableID: base + 3, want: 3}, + {name: "argument max boundary", tableID: base + uformat.ArgumentMax, want: uformat.ArgumentMax}, + {name: "base itself is out of range (Argument 0 is reserved)", tableID: base, wantErr: true}, + {name: "past the 12-bit Argument range", tableID: base + uformat.ArgumentMax + 1, wantErr: true}, + // The host allocates these, so a sidecar must never derive an Argument + // from one. + {name: "host table id 1", tableID: 1, wantErr: true}, + {name: "last host table id", tableID: vrf.HostTableIDMax, wantErr: true}, } for _, tc := range cases { @@ -67,7 +72,8 @@ func TestEgressVethNames(t *testing.T) { inner1, peer1, otherInner, otherPeer) } - for _, name := range []string{inner1, peer1, otherInner, otherPeer} { + maxInner, maxPeer := egressVethNames(sidecarTableIDMax) + for _, name := range []string{inner1, peer1, otherInner, otherPeer, maxInner, maxPeer} { if len(name) > unix.IFNAMSIZ-1 { t.Errorf("egressVethNames produced %q, %d bytes -- too long for IFNAMSIZ (%d)", name, len(name), unix.IFNAMSIZ) } diff --git a/internal/installer/sidecarreturn.go b/internal/installer/sidecarreturn.go index a0fe31de..6160a063 100644 --- a/internal/installer/sidecarreturn.go +++ b/internal/installer/sidecarreturn.go @@ -71,9 +71,9 @@ import ( const ( // sidecarReturnTableBase and sidecarReturnTableMax bound the Linux routing // table IDs this file allocates from. A dedicated range matters twice: - // tenant VRF table IDs are handed out from 1 upward, so anything low would - // eventually collide, and pruning deletes whole tables, which must never - // reach one this file did not create. + // tenant VRF table IDs are handed out up to vrf.HostTableIDMax, so anything + // at or below it would collide, and pruning deletes whole tables, which + // must never reach one this file did not create. // // Sized to hold one table per Argument, the most sidecar VRFs a node can // have. diff --git a/internal/installer/sidecarreturn_test.go b/internal/installer/sidecarreturn_test.go index a96b4f02..8f9941f7 100644 --- a/internal/installer/sidecarreturn_test.go +++ b/internal/installer/sidecarreturn_test.go @@ -160,6 +160,20 @@ func TestSidecarReturnTableID_StaysInTheReservedRange(t *testing.T) { } } +// TestSidecarReturnTableRange_SitsBetweenVRFRanges checks that the return-path +// tables, which live in the host namespace, overlap neither the host's tenant +// VRF tables nor the ingress sidecar's. +func TestSidecarReturnTableRange_SitsBetweenVRFRanges(t *testing.T) { + if sidecarReturnTableBase <= vrf.HostTableIDMax { + t.Errorf("sidecarReturnTableBase %#x overlaps host VRF tables up to %#x", + sidecarReturnTableBase, vrf.HostTableIDMax) + } + if sidecarReturnTableMax >= vrf.SidecarTableIDBase { + t.Errorf("sidecarReturnTableMax %#x reaches the sidecar's VRF tables from %#x", + sidecarReturnTableMax, vrf.SidecarTableIDBase) + } +} + // TestSidecarReturnTableID_RejectsOutOfRange guards the other direction. A // zero Argument is the reserved value uformat excludes, and anything past // ArgumentMax cannot have arrived in a SID at all -- either would compute a diff --git a/internal/plumbing/ebpf/egressroutemap/owner.go b/internal/plumbing/ebpf/egressroutemap/owner.go index 961ed05d..85bf6331 100644 --- a/internal/plumbing/ebpf/egressroutemap/owner.go +++ b/internal/plumbing/ebpf/egressroutemap/owner.go @@ -24,9 +24,10 @@ import ( // // Nothing in egress_route_table's own key or value records who wrote an entry, // and its key carries only a routing table ID, which each namespace allocates -// from 1 upward independently. vrf_table does record it: the sidecar registers -// every VRF it serves under uformat.BlockIngressSidecar, a Block no CNI -// attachment can reach, and does so before installing any route into that +// independently, from disjoint ranges (see vrf.SidecarTableIDBase), so a +// table ID belongs to one writer only. vrf_table records which: the sidecar +// registers every VRF it serves under uformat.BlockIngressSidecar, a Block no +// CNI attachment can reach, and does so before installing any route into that // table. Reading those rows back is therefore enough to tell the two // populations apart without a second copy of the ownership state. func SidecarOwnedTableIDs(vrfTable *usidmap.VRFTable) (map[uint32]struct{}, error) { diff --git a/internal/plumbing/ebpf/ifindexvrfmap/ifindexvrf.go b/internal/plumbing/ebpf/ifindexvrfmap/ifindexvrf.go index aa734ae3..4417492d 100644 --- a/internal/plumbing/ebpf/ifindexvrfmap/ifindexvrf.go +++ b/internal/plumbing/ebpf/ifindexvrfmap/ifindexvrf.go @@ -17,6 +17,26 @@ import ( "go.datum.net/galactic/internal/plumbing/ebpf/usidmap" ) +// SidecarIfindexBase splits the interface index space of ifindex_vrf_table +// between its two writers. The ingress sidecar creates every interface it +// registers with an explicit index of SidecarIfindexBase plus that VRF's uSID +// Argument, and the host registers only indexes below it. +// +// Each network namespace numbers its interfaces on its own, from 1 upward, +// but the sidecar registers indexes from its pod's namespace into the same +// pinned map the host CNI writes host indexes into. Without disjoint ranges, a +// sidecar interface and a host interface with the same index overwrite each +// other's row, and usid_egress routes one's traffic into the other's VRF. +// +// A host namespace allocates indexes sequentially and never gets near 2^30. +const SidecarIfindexBase = uint32(0x40000000) + +// SidecarIfindex returns the interface index the ingress sidecar gives the +// interface it registers for the VRF whose uSID Argument is argument. +func SidecarIfindex(argument uint16) uint32 { + return SidecarIfindexBase + uint32(argument) +} + // IfindexVRFEntry is one decoded ifindex_vrf_table row, kept separate from the // generated kernel layout. type IfindexVRFEntry struct { @@ -75,6 +95,9 @@ func (t *IfindexVRFTable) Register(ifindex uint32, block uint64, argument uint16 if err := uformat.ValidateArgument(argument); err != nil { return fmt.Errorf("ifindexvrfmap: ifindex_vrf_table: register ifindex=%d: %w", ifindex, err) } + if err := validateIfindexOwner(ifindex, block, argument); err != nil { + return fmt.Errorf("ifindexvrfmap: ifindex_vrf_table: register ifindex=%d: %w", ifindex, err) + } value := prog.UsidIfindexVrfValue{Block: block, Argument: argument} if err := t.table.Put(ifindex, value); err != nil { @@ -87,6 +110,23 @@ func (t *IfindexVRFTable) Register(ifindex uint32, block uint64, argument uint16 return nil } +// validateIfindexOwner rejects a registration that would cross into the other +// writer's index range; see SidecarIfindexBase. The ingress sidecar registers +// only under uformat.BlockIngressSidecar, so the Block identifies the writer. +func validateIfindexOwner(ifindex uint32, block uint64, argument uint16) error { + if block == uformat.BlockIngressSidecar { + if want := SidecarIfindex(argument); ifindex != want { + return fmt.Errorf("ingress sidecar interface for argument %#x must have index %d", argument, want) + } + return nil + } + if ifindex >= SidecarIfindexBase { + return fmt.Errorf("host interface index is in the range reserved for the ingress sidecar (>= %d)", + SidecarIfindexBase) + } + return nil +} + // Unregister removes the ifindex_vrf_table entry for ifindex if present. An // already-absent entry is not an error: DEL is idempotent per the CNI spec, and // every caller treats cleanup as best-effort. diff --git a/internal/plumbing/ebpf/ifindexvrfmap/ifindexvrf_test.go b/internal/plumbing/ebpf/ifindexvrfmap/ifindexvrf_test.go index 691710cf..b24eb4fd 100644 --- a/internal/plumbing/ebpf/ifindexvrfmap/ifindexvrf_test.go +++ b/internal/plumbing/ebpf/ifindexvrfmap/ifindexvrf_test.go @@ -7,6 +7,8 @@ package ifindexvrfmap import ( "errors" "testing" + + "go.datum.net/galactic/internal/plumbing/ebpf/uformat" ) const testBlock uint64 = 0x2001_0DB8_FF01 @@ -65,6 +67,47 @@ func TestIfindexVRFTable_RegisterRejectsReservedArgumentZero(t *testing.T) { } } +// TestIfindexVRFTable_RegisterKeepsWritersApart covers #716: the host and the +// ingress sidecar number interfaces in separate namespaces but share this +// map, so each must stay inside its own index range. +func TestIfindexVRFTable_RegisterKeepsWritersApart(t *testing.T) { + const sidecar = uformat.BlockIngressSidecar + cases := []struct { + name string + ifindex uint32 + block uint64 + argument uint16 + wantErr bool + }{ + {name: "host index below the sidecar range", ifindex: 42, block: testBlock, argument: 0x7}, + {name: "last host index", ifindex: SidecarIfindexBase - 1, block: testBlock, argument: 0x7}, + {name: "host index in the sidecar range", ifindex: SidecarIfindex(0x7), block: testBlock, argument: 0x7, + wantErr: true}, + {name: "sidecar index matching its argument", ifindex: SidecarIfindex(0x7), block: sidecar, argument: 0x7}, + {name: "sidecar index for another argument", ifindex: SidecarIfindex(0x8), block: sidecar, argument: 0x7, + wantErr: true}, + {name: "sidecar with a host-range index", ifindex: 0x7, block: sidecar, argument: 0x7, wantErr: true}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + it, ft := newTestIfindexVRFTable(constClock(1)) + err := it.Register(tc.ifindex, tc.block, tc.argument) + if tc.wantErr { + if err == nil { + t.Fatalf("Register(%d, %#x, %#x) = nil, want an error", tc.ifindex, tc.block, tc.argument) + } + if ft.len() != 0 { + t.Errorf("rejected Register wrote %d entries, want 0", ft.len()) + } + return + } + if err != nil { + t.Fatalf("Register(%d, %#x, %#x): %v", tc.ifindex, tc.block, tc.argument, err) + } + }) + } +} + func TestIfindexVRFTable_RegisterRejectsBlockOverflow(t *testing.T) { it, ft := newTestIfindexVRFTable(constClock(1)) diff --git a/internal/plumbing/vrf/vrf.go b/internal/plumbing/vrf/vrf.go index 55ac57cd..94469e08 100644 --- a/internal/plumbing/vrf/vrf.go +++ b/internal/plumbing/vrf/vrf.go @@ -11,7 +11,6 @@ package vrf import ( "errors" "fmt" - "math" "net" "sync" "time" @@ -23,8 +22,25 @@ import ( "go.datum.net/galactic/internal/plumbing/sysctl" ) -const minVRFID = uint32(1) -const maxVRFID = uint32(math.MaxUint32 - 1) +// SidecarTableIDBase splits the routing table ID space between the two +// writers of the shared pinned eBPF maps. The host's VRFs take IDs below it, +// and the ingress sidecar's take IDs above it. +// +// Each network namespace allocates table IDs on its own, but +// egress_route_table is keyed by table ID alone and is shared between the +// host and every sidecar pod on the node. Without disjoint ranges, a sidecar +// VRF and a host tenant VRF both start at table 1 and overwrite each other's +// routes. +const SidecarTableIDBase = uint32(0x10000) + +// HostTableIDMin and HostTableIDMax bound the routing table IDs Add allocates. +// HostTableIDMax stops short of SidecarTableIDBase: the host's installer keeps +// its sidecar return-path tables in the host namespace in the IDs between the +// two (internal/installer/sidecarreturn.go). +const ( + HostTableIDMin = uint32(1) + HostTableIDMax = uint32(0xEFFF) +) // UnreachableDefaultMetric is the metric of the unreachable default route Add // installs in every VRF table, for each family. It is the value the kernel's @@ -49,7 +65,8 @@ var ErrNotFound = errors.New("vrf: no VRF interface for this VPC in this network var vrfMu sync.Mutex // Add creates the Linux VRF interface for a base62-encoded VPC, allocating the -// next available routing table ID and applying the required sysctls. +// next available routing table ID in [HostTableIDMin, HostTableIDMax] and +// applying the required sysctls. // // The VRF is shared by every attachment on this VPC on this node, so concurrent // calls, from goroutines here or from separate plugin processes attaching @@ -62,6 +79,14 @@ var vrfMu sync.Mutex // before those existed therefore gains them on its next attachment's ADD, // with no migration step. func Add(vpc string) error { + return AddInRange(vpc, HostTableIDMin, HostTableIDMax) +} + +// AddInRange is Add with the routing table ID allocated from [minID, maxID]. +// A VRF that already exists under this VPC's name with a table ID outside that +// range is an error: its table may collide with the other writer's, and +// recreating it would strand whatever is already routed through it. +func AddInRange(vpc string, minID, maxID uint32) error { vrfMu.Lock() defer vrfMu.Unlock() @@ -78,10 +103,14 @@ func Add(vpc string) error { if !ok { return fmt.Errorf("interface %q exists but is a %s, not a VRF", name, link.Type()) } + if existing.Table < minID || existing.Table > maxID { + return fmt.Errorf("VRF %q has table %d, outside the range [%d,%d] this caller allocates from", + name, existing.Table, minID, maxID) + } return ensureUnreachableDefaults(existing.Table) } - vrfID, err := findNextAvailableVRFID() + vrfID, err := findNextAvailableVRFID(minID, maxID) if err != nil { return err } @@ -281,7 +310,7 @@ func listVRFLinks() ([]*netlink.Vrf, error) { return ListVRFLinks() } -func findNextAvailableVRFID() (uint32, error) { +func findNextAvailableVRFID(minID, maxID uint32) (uint32, error) { vrfs, err := listVRFLinks() if err != nil { return 0, err @@ -291,14 +320,24 @@ func findNextAvailableVRFID() (uint32, error) { for _, vrf := range vrfs { used[vrf.Table] = struct{}{} } + return nextFreeTableID(used, minID, maxID) +} - for vrfID := minVRFID; vrfID <= maxVRFID; vrfID++ { - if _, ok := used[vrfID]; !ok { - return vrfID, nil +// nextFreeTableID returns the lowest table ID in [minID, maxID] that is +// neither in used nor one of the kernel's reserved tables (253 default, 254 +// main, 255 local). A VRF bound to main would route its tenant through the +// host's own table. +func nextFreeTableID(used map[uint32]struct{}, minID, maxID uint32) (uint32, error) { + for vrfID := uint64(minID); vrfID <= uint64(maxID); vrfID++ { + id := uint32(vrfID) + if id == unix.RT_TABLE_DEFAULT || id == unix.RT_TABLE_MAIN || id == unix.RT_TABLE_LOCAL { + continue + } + if _, ok := used[id]; !ok { + return id, nil } } - - return 0, errors.New("could not find any available VRF id") + return 0, fmt.Errorf("no available VRF table id in [%d,%d]", minID, maxID) } func getVRFIDForInterface(name string) (uint32, error) { diff --git a/internal/plumbing/vrf/vrf_internal_test.go b/internal/plumbing/vrf/vrf_internal_test.go new file mode 100644 index 00000000..7f460faf --- /dev/null +++ b/internal/plumbing/vrf/vrf_internal_test.go @@ -0,0 +1,78 @@ +// Copyright 2026 Datum Cloud, Inc. +// +// SPDX-License-Identifier: AGPL-3.0-or-later + +package vrf + +import ( + "testing" + + "golang.org/x/sys/unix" +) + +func TestNextFreeTableID(t *testing.T) { + used := func(ids ...uint32) map[uint32]struct{} { + m := make(map[uint32]struct{}, len(ids)) + for _, id := range ids { + m[id] = struct{}{} + } + return m + } + cases := []struct { + name string + used map[uint32]struct{} + min, max uint32 + want uint32 + wantErr bool + }{ + {name: "empty host range starts at its minimum", used: used(), min: HostTableIDMin, max: HostTableIDMax, want: 1}, + {name: "skips used ids", used: used(1, 2), min: HostTableIDMin, max: HostTableIDMax, want: 3}, + {name: "skips the kernel's reserved tables", used: used(252), min: 252, max: 300, want: 256}, + { + name: "allocates inside a range above the minimum", used: used(), + min: SidecarTableIDBase + 1, max: SidecarTableIDBase + 0xFFF, want: SidecarTableIDBase + 1, + }, + {name: "full range", used: used(10, 11), min: 10, max: 11, wantErr: true}, + { + name: "range of only reserved tables", used: used(), + min: unix.RT_TABLE_DEFAULT, max: unix.RT_TABLE_LOCAL, wantErr: true, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got, err := nextFreeTableID(tc.used, tc.min, tc.max) + if tc.wantErr { + if err == nil { + t.Fatalf("nextFreeTableID = %d, nil; want an error", got) + } + return + } + if err != nil { + t.Fatalf("nextFreeTableID: %v", err) + } + if got != tc.want { + t.Errorf("nextFreeTableID = %d, want %d", got, tc.want) + } + }) + } +} + +// TestNextFreeTableID_HostAndSidecarNeverOverlap covers #716: the host and the +// ingress sidecar allocate in separate network namespaces, each seeing none of +// the other's VRFs, but write the same egress_route_table keyed by table ID. +func TestNextFreeTableID_HostAndSidecarNeverOverlap(t *testing.T) { + host, err := nextFreeTableID(map[uint32]struct{}{}, HostTableIDMin, HostTableIDMax) + if err != nil { + t.Fatalf("host allocation: %v", err) + } + sidecar, err := nextFreeTableID(map[uint32]struct{}{}, SidecarTableIDBase+1, SidecarTableIDBase+0xFFF) + if err != nil { + t.Fatalf("sidecar allocation: %v", err) + } + if host == sidecar { + t.Fatalf("host and sidecar both allocated table %d from empty namespaces", host) + } + if HostTableIDMax >= SidecarTableIDBase { + t.Fatalf("HostTableIDMax %d reaches SidecarTableIDBase %d", HostTableIDMax, SidecarTableIDBase) + } +} diff --git a/internal/plumbing/vrf/vrf_test.go b/internal/plumbing/vrf/vrf_test.go index 3bc311b1..ed700f02 100644 --- a/internal/plumbing/vrf/vrf_test.go +++ b/internal/plumbing/vrf/vrf_test.go @@ -510,3 +510,30 @@ func TestListVRFLinks_SurvivesLinkChurn(t *testing.T) { } } } + +// TestAddInRange_AllocatesInsideTheRange covers the ingress sidecar's use of +// AddInRange (#716): its VRFs must take table IDs from its own range, never +// the host's. +func TestAddInRange_AllocatesInsideTheRange(t *testing.T) { + requireRoot(t) + const vpc = "vtrng" + t.Cleanup(func() { _ = vrf.Delete(vpc) }) + + minID, maxID := vrf.SidecarTableIDBase+1, vrf.SidecarTableIDBase+0xFFF + if err := vrf.AddInRange(vpc, minID, maxID); err != nil { + t.Fatalf("AddInRange: %v", err) + } + tableID, err := vrf.TableID(vpc) + if err != nil { + t.Fatalf("TableID: %v", err) + } + if tableID < minID || tableID > maxID { + t.Fatalf("table ID %d outside [%d,%d]", tableID, minID, maxID) + } + + // The same VPC from a caller with a different range is an error, not a + // silent reuse of a table that may belong to the other writer. + if err := vrf.Add(vpc); err == nil { + t.Fatalf("Add of a VRF whose table %d is outside the host range = nil, want an error", tableID) + } +} From 7464c6a9ba51df8bce10a151d864d68e2aa84a99 Mon Sep 17 00:00:00 2001 From: Peter Sprygada Date: Tue, 6 Oct 2026 22:29:59 -0400 Subject: [PATCH 2/3] fix: Name the remedy when an older sidecar's VRF or veth is in the way A sidecar container restarted in place on the new image finds the VRF and veth peer an older version created in the same pod network namespace, outside the new ranges. Both errors now say to replace the pod rather than restart the container. vrf.AddInRange wraps a new ErrTableOutOfRange so the sidecar can recognise that case. Related to #716 Co-Authored-By: Claude Opus 5.5 --- internal/ingresssidecar/backend.go | 5 +++++ internal/ingresssidecar/ebpfdatapath.go | 3 ++- internal/plumbing/vrf/vrf.go | 9 +++++++-- internal/plumbing/vrf/vrf_test.go | 4 ++-- 4 files changed, 16 insertions(+), 5 deletions(-) diff --git a/internal/ingresssidecar/backend.go b/internal/ingresssidecar/backend.go index 1bafc567..bf94b4da 100644 --- a/internal/ingresssidecar/backend.go +++ b/internal/ingresssidecar/backend.go @@ -5,6 +5,7 @@ package ingresssidecar import ( + "errors" "fmt" "net" "regexp" @@ -68,6 +69,10 @@ func NewKernelBackend() Backend { return kernelBackend{} } func (kernelBackend) EnsureVRF(vpc string) (uint32, error) { if err := vrf.AddInRange(vpc, sidecarTableIDMin, sidecarTableIDMax); err != nil { + if errors.Is(err, vrf.ErrTableOutOfRange) { + return 0, fmt.Errorf("create VRF for vpc %s: %w; an older sidecar created it in this pod, "+ + "so replace the pod rather than restarting the container", vpc, err) + } return 0, fmt.Errorf("create VRF for vpc %s: %w", vpc, err) } tableID, err := vrf.TableID(vpc) diff --git a/internal/ingresssidecar/ebpfdatapath.go b/internal/ingresssidecar/ebpfdatapath.go index 31d7cdf6..b567c67f 100644 --- a/internal/ingresssidecar/ebpfdatapath.go +++ b/internal/ingresssidecar/ebpfdatapath.go @@ -127,7 +127,8 @@ func egressVethNames(tableID uint32) (inner, peer string) { func ensureEgressVeth(vrfLink *netlink.Vrf, inner, peer string, peerIndex uint32) (netlink.Link, error) { peerLink, err := netlink.LinkByName(peer) if err == nil && uint32(peerLink.Attrs().Index) != peerIndex { - return nil, fmt.Errorf("veth peer %q has index %d, want %d", peer, peerLink.Attrs().Index, peerIndex) + return nil, fmt.Errorf("veth peer %q has index %d, want %d; an older sidecar created it in this pod, "+ + "so replace the pod rather than restarting the container", peer, peerLink.Attrs().Index, peerIndex) } if err != nil { var notFound netlink.LinkNotFoundError diff --git a/internal/plumbing/vrf/vrf.go b/internal/plumbing/vrf/vrf.go index 94469e08..7e12885a 100644 --- a/internal/plumbing/vrf/vrf.go +++ b/internal/plumbing/vrf/vrf.go @@ -59,6 +59,11 @@ const UnreachableDefaultMetric = 4278198272 // either way. var ErrNotFound = errors.New("vrf: no VRF interface for this VPC in this network namespace") +// ErrTableOutOfRange is wrapped by AddInRange when this VPC's VRF already exists +// with a table ID outside the caller's range, for instance one created by an +// ingress sidecar version that allocated from the host's range. +var ErrTableOutOfRange = errors.New("vrf: existing VRF's table is outside the caller's range") + // vrfMu serializes VRF creation and deletion within one process. It does not by // itself protect two separate CNI invocations racing on the same node, each // being its own process, so Add and Delete also take a cross-process lock. @@ -104,8 +109,8 @@ func AddInRange(vpc string, minID, maxID uint32) error { return fmt.Errorf("interface %q exists but is a %s, not a VRF", name, link.Type()) } if existing.Table < minID || existing.Table > maxID { - return fmt.Errorf("VRF %q has table %d, outside the range [%d,%d] this caller allocates from", - name, existing.Table, minID, maxID) + return fmt.Errorf("VRF %q has table %d, not in [%d,%d]: %w", + name, existing.Table, minID, maxID, ErrTableOutOfRange) } return ensureUnreachableDefaults(existing.Table) } diff --git a/internal/plumbing/vrf/vrf_test.go b/internal/plumbing/vrf/vrf_test.go index ed700f02..04008a88 100644 --- a/internal/plumbing/vrf/vrf_test.go +++ b/internal/plumbing/vrf/vrf_test.go @@ -533,7 +533,7 @@ func TestAddInRange_AllocatesInsideTheRange(t *testing.T) { // The same VPC from a caller with a different range is an error, not a // silent reuse of a table that may belong to the other writer. - if err := vrf.Add(vpc); err == nil { - t.Fatalf("Add of a VRF whose table %d is outside the host range = nil, want an error", tableID) + if err := vrf.Add(vpc); !errors.Is(err, vrf.ErrTableOutOfRange) { + t.Fatalf("Add of a VRF whose table %d is outside the host range = %v, want ErrTableOutOfRange", tableID, err) } } From 389c4fb4a94a62253c70c07f34bde6ae56ddf466 Mon Sep 17 00:00:00 2001 From: Peter Sprygada Date: Tue, 6 Oct 2026 22:32:20 -0400 Subject: [PATCH 3/3] test: Drop issue references from the #716 test comments Co-Authored-By: Claude Opus 5.5 --- internal/ingresssidecar/ebpfdatapath_integration_test.go | 2 +- internal/plumbing/ebpf/ifindexvrfmap/ifindexvrf_test.go | 6 +++--- internal/plumbing/vrf/vrf_internal_test.go | 7 ++++--- internal/plumbing/vrf/vrf_test.go | 4 ++-- 4 files changed, 10 insertions(+), 9 deletions(-) diff --git a/internal/ingresssidecar/ebpfdatapath_integration_test.go b/internal/ingresssidecar/ebpfdatapath_integration_test.go index 6cedcadb..43d1617c 100644 --- a/internal/ingresssidecar/ebpfdatapath_integration_test.go +++ b/internal/ingresssidecar/ebpfdatapath_integration_test.go @@ -104,7 +104,7 @@ func assertMapState( // The peer's index is in the range the host never registers, so a host // interface with the same number in its own namespace cannot overwrite - // this row (#716). + // this row. if want := ifindexvrfmap.SidecarIfindex(argument); uint32(peerLink.Attrs().Index) != want { return fmt.Errorf("veth peer ifindex = %d, want %d", peerLink.Attrs().Index, want) } diff --git a/internal/plumbing/ebpf/ifindexvrfmap/ifindexvrf_test.go b/internal/plumbing/ebpf/ifindexvrfmap/ifindexvrf_test.go index b24eb4fd..82f96f5a 100644 --- a/internal/plumbing/ebpf/ifindexvrfmap/ifindexvrf_test.go +++ b/internal/plumbing/ebpf/ifindexvrfmap/ifindexvrf_test.go @@ -67,9 +67,9 @@ func TestIfindexVRFTable_RegisterRejectsReservedArgumentZero(t *testing.T) { } } -// TestIfindexVRFTable_RegisterKeepsWritersApart covers #716: the host and the -// ingress sidecar number interfaces in separate namespaces but share this -// map, so each must stay inside its own index range. +// TestIfindexVRFTable_RegisterKeepsWritersApart checks that the host and the +// ingress sidecar, which number interfaces in separate namespaces but share +// this map, each stay inside their own index range. func TestIfindexVRFTable_RegisterKeepsWritersApart(t *testing.T) { const sidecar = uformat.BlockIngressSidecar cases := []struct { diff --git a/internal/plumbing/vrf/vrf_internal_test.go b/internal/plumbing/vrf/vrf_internal_test.go index 7f460faf..83bf21da 100644 --- a/internal/plumbing/vrf/vrf_internal_test.go +++ b/internal/plumbing/vrf/vrf_internal_test.go @@ -57,9 +57,10 @@ func TestNextFreeTableID(t *testing.T) { } } -// TestNextFreeTableID_HostAndSidecarNeverOverlap covers #716: the host and the -// ingress sidecar allocate in separate network namespaces, each seeing none of -// the other's VRFs, but write the same egress_route_table keyed by table ID. +// TestNextFreeTableID_HostAndSidecarNeverOverlap checks the host and the +// ingress sidecar never pick the same table ID. They allocate in separate +// network namespaces, each seeing none of the other's VRFs, but write the same +// egress_route_table keyed by table ID. func TestNextFreeTableID_HostAndSidecarNeverOverlap(t *testing.T) { host, err := nextFreeTableID(map[uint32]struct{}{}, HostTableIDMin, HostTableIDMax) if err != nil { diff --git a/internal/plumbing/vrf/vrf_test.go b/internal/plumbing/vrf/vrf_test.go index 04008a88..56f69786 100644 --- a/internal/plumbing/vrf/vrf_test.go +++ b/internal/plumbing/vrf/vrf_test.go @@ -512,8 +512,8 @@ func TestListVRFLinks_SurvivesLinkChurn(t *testing.T) { } // TestAddInRange_AllocatesInsideTheRange covers the ingress sidecar's use of -// AddInRange (#716): its VRFs must take table IDs from its own range, never -// the host's. +// AddInRange: its VRFs must take table IDs from its own range, never the +// host's. func TestAddInRange_AllocatesInsideTheRange(t *testing.T) { requireRoot(t) const vpc = "vtrng"