From 0060d922a5cf41fefbcc1b2d3805ecd76001e39e Mon Sep 17 00:00:00 2001 From: Peter Sprygada Date: Tue, 18 Aug 2026 16:04:54 -0400 Subject: [PATCH 1/6] feat(cni): publish per-pod EndpointSlice for HTTP ingress backend discovery MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Implements the #854 plan (docs/plans/854-vpc-http-ingress-endpointslice.md, as revised by PR #376), phases 1-10: - internal/crdnames: TenantIdentifier/EndpointSliceName helpers, plus the LabelTenantID/AnnotationTenantID/AnnotationSID key constants. - internal/nadpatch: ParsePodName, sibling to ParsePodNamespace. - internal/cnibgp/bgp.go: compute the SRv6 uSID inside publishBGPState's retry closure once vrfID is allocated (reusing registerEBPFDatapath's own "SRv6 not configured" skip). Also fixes the rollback-risk gap the plan flagged: advertisementCreated is now gated on controllerutil.OperationResultCreated, mirroring vrfInstanceCreated's existing pattern, so a later step's failure can no longer make resourceTracker.cleanup delete a BGPAdvertisement still backing a live sibling attachment. - internal/cnibgp/endpointslice.go (new): publishEndpointSlice/ deleteEndpointSlice. Publish runs as its own step after publishBGPState succeeds, not folded into its retry closure. Sets an ownerReference to the owning Pod (Phase 8's GC backstop) and defends against a name collision with a non-tenant-labeled EndpointSlice. - internal/cnibgp/ops_add.go: wires EndpointSlice publish into cmdAdd, skipped when the attachment has no IPv6 address to carry (nil ipamResult or no IPv6Subnet) — not VM/tap-specific. - internal/cnibgp/ops_del.go: cmdDel now deletes the pod's EndpointSlice (1:1 with one pod, never shared, unlike the BGP CRDs) best-effort, logging and continuing rather than failing DEL. - internal/cnibgp/ops_check.go: cmdCheck now validates the EndpointSlice's address/labels/annotations, including a recomputed-SID check when this node's BGPRouter has SRv6 configured. - config/galactic-cni/rbac.yaml: grants discovery.k8s.io/endpointslices CRUD and pods get (for the ownerReference lookup) to galactic-cni's ClusterRole, shared by galactic-bgp. - docs/cni/configuration.md, docs/agents/ARCHITECTURE-CNI.md: document the new EndpointSlice publish behavior and annotation/label schema. Not yet covered: an e2e case (plan's Phase 10 tail) asserting the EndpointSlice appears on ADD, disappears on DEL, and disappears on Pod force-delete via the ownerReference. Co-Authored-By: Claude Sonnet 5 --- config/galactic-cni/rbac.yaml | 16 ++ docs/agents/ARCHITECTURE-CNI.md | 16 +- docs/cni/configuration.md | 34 +++ .../854-vpc-http-ingress-endpointslice.md | 2 +- internal/cnibgp/bgp.go | 37 ++- internal/cnibgp/bgp_test.go | 167 ++++++++++++ internal/cnibgp/endpointslice.go | 135 ++++++++++ internal/cnibgp/endpointslice_test.go | 241 ++++++++++++++++++ internal/cnibgp/ops_add.go | 25 ++ internal/cnibgp/ops_check.go | 78 ++++++ internal/cnibgp/ops_check_test.go | 208 +++++++++++++++ internal/cnibgp/ops_del.go | 57 ++++- internal/crdnames/crdnames.go | 46 ++++ internal/crdnames/crdnames_test.go | 59 ++++- internal/nadpatch/nadpatch.go | 14 + internal/nadpatch/nadpatch_test.go | 31 +++ 16 files changed, 1144 insertions(+), 22 deletions(-) create mode 100644 internal/cnibgp/endpointslice.go create mode 100644 internal/cnibgp/endpointslice_test.go create mode 100644 internal/cnibgp/ops_check_test.go diff --git a/config/galactic-cni/rbac.yaml b/config/galactic-cni/rbac.yaml index 5d56c3a4..823ebf54 100644 --- a/config/galactic-cni/rbac.yaml +++ b/config/galactic-cni/rbac.yaml @@ -20,6 +20,22 @@ rules: resources: - nodes verbs: ["get"] + # discovery.k8s.io/endpointslices: galactic-bgp's per-pod EndpointSlice + # publish (ADD), delete (DEL), and CHECK — see internal/cnibgp/ + # endpointslice.go and docs/plans/854-vpc-http-ingress-endpointslice.md. + - apiGroups: ["discovery.k8s.io"] + resources: + - endpointslices + verbs: ["get", "list", "create", "update", "patch", "delete"] + # pods (get only): to look up the owning Pod's UID and set it as the + # EndpointSlice's ownerReference, so the k8s garbage collector reclaims + # a force-deleted/never-DEL'd pod's EndpointSlice as a backstop (Phase 8 + # of the #854 plan) -- no create/update/delete needed, this SA never + # writes a Pod. + - apiGroups: [""] + resources: + - pods + verbs: ["get"] --- apiVersion: rbac.authorization.k8s.io/v1 diff --git a/docs/agents/ARCHITECTURE-CNI.md b/docs/agents/ARCHITECTURE-CNI.md index 779151fd..77ea9cad 100644 --- a/docs/agents/ARCHITECTURE-CNI.md +++ b/docs/agents/ARCHITECTURE-CNI.md @@ -5,7 +5,7 @@ > (VRF, veth/tap, SRv6 uSID datapath registration) and writes > `BGPAdvertisement`/`BGPVRFInstance` CRDs for `galactic-router` to pick up. -_Last updated: 2026-08-13_ +_Last updated: 2026-08-18_ This document covers the CNI side of Galactic only. See [ARCHITECTURE-ROUTER.md](ARCHITECTURE-ROUTER.md) for the BGP/EVPN control @@ -23,7 +23,13 @@ When a pod or VM is attached to a VPC, a chain of CNI plugins creates the required kernel state (VRF, veth pair or tap device, host-side routes) and writes a `BGPAdvertisement` CRD. `galactic-router` (see [ARCHITECTURE-ROUTER.md](ARCHITECTURE-ROUTER.md)) watches that CRD and -injects the EVPN path into the node-local GoBGP server. +injects the EVPN path into the node-local GoBGP server. `galactic-bgp` also +publishes a per-pod `discoveryv1.EndpointSlice` (when the attachment has an +IPv6 address to carry) — the mechanism the HTTP-ingress extension server +discovers VPC backends through; see +[docs/plans/854-vpc-http-ingress-endpointslice.md](../plans/854-vpc-http-ingress-endpointslice.md) +and [docs/cni/configuration.md](../cni/configuration.md)'s "EndpointSlice +publish" section. The CNI attach side is a **chain of small binaries**, not one monolithic plugin: a master plugin (`galactic-veth` for containers, `galactic-tap` for @@ -414,15 +420,15 @@ any shared, per-attachment kernel/CRD state — see the `cmdDel` note in | `internal/cni` | galactic-veth | Veth master plugin: `cmdAdd`/`cmdDel`/`cmdCheck`/`cmdStatus`; PluginConf parsing; NAD annotation; host-device delegation; delegates kernel work to plumbing | No | | `internal/hostconf` | every CNI-chain binary | Shared `HostConf` schema + static-conflist loader, plus API-based node-name auto-detect | No | | `internal/hostgw` | galactic-veth, galactic-tap | Host-side gateway address/route configuration for a VPC attachment's allocated IPAM addresses | No | -| `internal/crdnames` | galactic-veth, galactic-bgp | Deterministic `BGPVRFInstance`/`BGPAdvertisement` CRD name + annotation-key derivation (also read by `galactic-router`'s GC — see [ARCHITECTURE-ROUTER.md](ARCHITECTURE-ROUTER.md)) | No | -| `internal/nadpatch` | galactic-veth, galactic-tap | NAD annotation patch (host interface name) + pod-namespace parsing from `CNI_ARGS` | No | +| `internal/crdnames` | galactic-veth, galactic-bgp | Deterministic `BGPVRFInstance`/`BGPAdvertisement`/EndpointSlice CRD name + annotation/label-key derivation (also read by `galactic-router`'s GC — see [ARCHITECTURE-ROUTER.md](ARCHITECTURE-ROUTER.md)) | No | +| `internal/nadpatch` | galactic-veth, galactic-tap, galactic-bgp | NAD annotation patch (host interface name) + pod-name/pod-namespace parsing from `CNI_ARGS` | No | | `internal/cni/ipam` | galactic-ipam | IPv6/IPv4 pool allocators + static IP allocator; on-disk marker-file persistence (flock-guarded, keyed by containerID) | Yes (pool allocations + marker files) | | `internal/cni/route` | galactic-route | Host-side static route add/delete via netlink | No | | `internal/cni/tap` | galactic-tap | Tap interface create/delete for VM workloads (Kata, Firecracker, kraftlet/Unikraft) | No | | `internal/cni/veth` | galactic-veth | veth pair create/delete | No | | `internal/cnitap` | galactic-tap | Tap master plugin (mirrors `internal/cni`; no host-device delegation, no guest netns) | No | | `internal/cniipam` | galactic-ipam | CNI IPAM delegation protocol (`cmdAdd`/`cmdDel`/`cmdCheck`/`cmdStatus`); explicit `"ipam"`-block contract; no k8s dependency | No | -| `internal/cnibgp` | galactic-bgp | BGP/SRv6/eBPF publish: SID/Argument allocation + collision detection, `registerEBPFDatapath`/`unregisterEBPFDatapath`, `BGPVRFInstance`/`BGPAdvertisement` CRUD with retry; learns everything from `prevResult` | No | +| `internal/cnibgp` | galactic-bgp | BGP/SRv6/eBPF publish: SID/Argument allocation + collision detection, `registerEBPFDatapath`/`unregisterEBPFDatapath`, `BGPVRFInstance`/`BGPAdvertisement` CRUD with retry, per-pod EndpointSlice publish/delete/CHECK for HTTP-ingress backend discovery (`endpointslice.go`); learns everything from `prevResult` | No | | `internal/cniroute` | galactic-route | Termination-route plugin: installs/rolls-back VRF-table routes; no k8s dependency | No | | `internal/vmtap` | vmtap-cni | Patches Cilium's own chain conflist to add a tap-interface stage for VM workloads | No | | `internal/installer` | galactic-cni | DaemonSet `init`/`run` support: binary staging (every chain binary), node-identity check, conflist/kubeconfig templating, credential refresh ticker, log rotation, eBPF datapath lifecycle, gRPC health server | No | diff --git a/docs/cni/configuration.md b/docs/cni/configuration.md index 1ed9585e..bef9680a 100644 --- a/docs/cni/configuration.md +++ b/docs/cni/configuration.md @@ -304,6 +304,40 @@ what addresses were allocated entirely from `prevResult` (the accumulated result of every preceding plugin in the chain), never from its own config or a kernel call. +### EndpointSlice publish (HTTP ingress backend discovery) + +Alongside the `BGPVRFInstance`/`BGPAdvertisement` CRDs, `galactic-bgp` +publishes one `discoveryv1.EndpointSlice` per pod — named after the pod, in +the pod's own namespace — whenever the attachment has an IPv6 address to +carry (i.e. `ipam` is configured on the master plugin's stanza; a tap/VM +attachment with no `ipam` block, same as a veth attachment with none, has +nothing to publish and is skipped, not an error). This is the mechanism the +HTTP ingress extension server (datum-cloud/enhancements#854/#796) discovers +VPC backends through — no backing `Service` object exists to key a +Service-generated `EndpointSlice` off of. + +IPv6-only: a dual-stack pod's IPv4 address is not published. The +EndpointSlice carries: + +- `spec.endpoints[].addresses`: the pod's allocated IPv6 address. +- Label `galactic.datum.net/tenant-id`: `TenantIdentifier(vpc, vpcattachment)` + — the discovery mechanism (annotations aren't selectable in a k8s + `List`/`Watch`). +- Annotation `galactic.datum.net/tenant-id`: the same value, for + human-readable detail. +- Annotation `galactic.datum.net/srv6-sid`: the computed SRv6 uSID + (`internal/plumbing/srv6.ComputeSID`) routing to this pod's VRF — absent + when this node's `BGPRouter` has no `srv6Locator`/`nodeID` configured. +- `metadata.ownerReferences`: the owning Pod, so the Kubernetes garbage + collector reclaims it if this plugin's own DEL is never run (a + force-deleted pod). CNI DEL deletes it explicitly and unconditionally on + the normal path — unlike the BGP CRDs, an EndpointSlice is 1:1 with + exactly one pod and is never shared with a sibling attachment, so DEL + deleting it immediately is safe. + +CHECK verifies the EndpointSlice still exists with the expected address and +annotations. + ## Example Configurations Every example below is a full conflist (a `NetworkAttachmentDefinition`'s diff --git a/docs/plans/854-vpc-http-ingress-endpointslice.md b/docs/plans/854-vpc-http-ingress-endpointslice.md index 82266849..20f6e57e 100644 --- a/docs/plans/854-vpc-http-ingress-endpointslice.md +++ b/docs/plans/854-vpc-http-ingress-endpointslice.md @@ -3,7 +3,7 @@ - **Issue:** [datum-cloud/enhancements#854](https://github.com/datum-cloud/enhancements/issues/854) - **Parent:** [datum-cloud/enhancements#796](https://github.com/datum-cloud/enhancements/issues/796) — HTTP Ingress for VPC Networks - **Design doc:** [HTTP Ingress for VPC Networks](https://github.com/datum-cloud/enhancements/blob/main/enhancements/networking/http-ingress-for-vpc-networks.md) (PR [#851](https://github.com/datum-cloud/enhancements/pull/851), resolves [#853](https://github.com/datum-cloud/enhancements/issues/853)) -- **Status:** planning only — no implementation started. Revised 2026-08-13 after a review pass caught drift against the current repo (see the "Revision note" callouts throughout) — most of it stale paths, but two are real design gaps (Phases 4 and 8) worth reading before starting. Revised again 2026-08-17: Open Decision 5 resolved — VM/tap-attached workloads are in scope and are this issue's primary use case, not an implicitly-excluded edge case; Phase 4's nil-`ipamResult` skip already handles this correctly (it's an address-existence check, not a VM exclusion) once confirmed against the current `internal/cnitap`/`internal/cnibgp` code, so no VM-specific implementation work is added by this revision. Open Decision 4 (Phase 4's rollback-risk callout) also resolved — fix #1, narrowing `advertisementCreated` to create-only gating, confirmed safe and adopted. Open Decision 6 (Phase 8's GC mechanism) also resolved — the recommended `ownerReference`-to-Pod approach is adopted as-is; `internal/gc/gc.go`/`config/galactic-router/rbac.yaml` are untouched by this issue. +- **Status:** implemented 2026-08-18, per this plan's sequencing (§4) — Phases 1–2 (`crdnames`/`nadpatch.ParsePodName`), 3 (SID computation), 4 (EndpointSlice publish on ADD + the `advertisementCreated` rollback-risk fix), 5 (DEL), 6 (CHECK), 7 (RBAC), 8 (ownerReference-to-Pod GC backstop, folded into Phase 4's publish step), 9 (docs), and unit tests (10) all landed together. See `internal/cnibgp/endpointslice.go`, the `internal/cnibgp/bgp.go`/`ops_add.go`/`ops_del.go`/`ops_check.go` changes, `internal/crdnames`/`internal/nadpatch`, and `config/galactic-cni/rbac.yaml`. Not yet exercised in `tests/e2e` (§3's suggested e2e case is still open). Previously revised 2026-08-13 after a review pass caught drift against the current repo (see the "Revision note" callouts throughout) — most of it stale paths, but two were real design gaps (Phases 4 and 8). Revised again 2026-08-17: Open Decision 5 resolved — VM/tap-attached workloads are in scope and are this issue's primary use case, not an implicitly-excluded edge case; Phase 4's nil-`ipamResult` skip already handles this correctly (it's an address-existence check, not a VM exclusion), confirmed against `internal/cnitap`/`internal/cnibgp`. Open Decision 4 (Phase 4's rollback-risk callout) also resolved — fix #1, narrowing `advertisementCreated` to create-only gating, confirmed safe and adopted. Open Decision 6 (Phase 8's GC mechanism) also resolved — the recommended `ownerReference`-to-Pod approach is adopted as-is; `internal/gc/gc.go`/`config/galactic-router/rbac.yaml` are untouched by this issue. ## Correction to #854's framing diff --git a/internal/cnibgp/bgp.go b/internal/cnibgp/bgp.go index db912086..20949a8a 100644 --- a/internal/cnibgp/bgp.go +++ b/internal/cnibgp/bgp.go @@ -102,6 +102,14 @@ type publishResult struct { // for why that distinction, not "CreateOrUpdate succeeded" alone, is // what makes rollback-deletion safe. vrfInstanceCreated bool + // sid is the computed SRv6 uSID for this attachment (see + // internal/plumbing/srv6.ComputeSID), valid (netip.Addr.IsValid()) only + // when this node's BGPRouter has SRv6Locator/nodeID configured — the + // same condition registerEBPFDatapath's own skip case checks. Consumed + // by the EndpointSlice publish step (endpointslice.go), which runs as + // its own step after publishBGPState returns, not folded into its retry + // closure — see Phase 4's rollback-risk note in the #854 plan for why. + sid netip.Addr } // isTransientError reports whether err is a transient failure that may @@ -467,6 +475,19 @@ func publishBGPState( // registerEBPFDatapath's own doc comment for why. prefixes, ipv6Subnet, ipv4Addr := ipamAdvertisementPrefixes(ipamResult) + // Reuses registerEBPFDatapath's own "SRv6 not configured, skip + // silently" sentinel: if this node's router has no + // srv6Locator/nodeID configured, there's nothing to publish, so + // leave result.sid at its zero value (IsValid() == false) rather + // than computing a SID for an attachment that has no SRv6 endpoint. + if bgp.srv6Locator != "" && bgp.nodeID != 0 { + sid, err := srv6.ComputeSID(bgp.srv6Locator, bgp.nodeID, vrfID, bgpv1alpha1.SRv6FunctionEndDT46) + if err != nil { + return fmt.Errorf("compute SRv6 uSID: %w", err) + } + result.sid = sid + } + // The return values aren't tracked for rollback: the vrf_table entry // they'd describe is shared by every attachment on this VPC/node, // same as the BGPVRFInstance above — see resourceTracker.cleanup's @@ -484,7 +505,7 @@ func publishBGPState( }, } var mergedPrefixes []string - _, err = controllerutil.CreateOrUpdate(ctx, k8s, adv, func() error { + advOp, err := controllerutil.CreateOrUpdate(ctx, k8s, adv, func() error { if adv.Annotations == nil { adv.Annotations = make(map[string]string) } @@ -519,9 +540,19 @@ func publishBGPState( if err != nil { return fmt.Errorf("apply BGPAdvertisement: %w", err) } - result.advertisementCreated = true + // Gated on OperationResultCreated, mirroring vrfInstanceCreated's + // existing pattern exactly: a BGPAdvertisement is reused (updated, + // not created) across pod churn on the same vpcAttachment, so + // marking it created on every successful write — including a mere + // update of an already-live sibling's CRD — would let + // resourceTracker.cleanup delete a BGPAdvertisement still backing a + // different, live container's route if a later ADD step fails. See + // the #854 plan's Phase 4 rollback-risk note. + if advOp == controllerutil.OperationResultCreated { + result.advertisementCreated = true + } slog.Debug("BGP: BGPAdvertisement applied", "name", adv.Name, "namespace", namespace, - "prefixes", mergedPrefixes, "addedPrefixes", prefixes, "containerID", args.ContainerID) + "prefixes", mergedPrefixes, "addedPrefixes", prefixes, "containerID", args.ContainerID, "operation", advOp) slog.Info("ADD: BGP state published", "containerID", args.ContainerID, "vpc", cfg.vpc, "vpcAttachment", cfg.vpcAttachment) diff --git a/internal/cnibgp/bgp_test.go b/internal/cnibgp/bgp_test.go index 661df4d7..d849a081 100644 --- a/internal/cnibgp/bgp_test.go +++ b/internal/cnibgp/bgp_test.go @@ -10,6 +10,7 @@ import ( "fmt" "maps" "net" + "os" "reflect" "strings" "testing" @@ -27,8 +28,11 @@ import ( "go.datum.net/galactic/internal/cniipam" "go.datum.net/galactic/internal/crdnames" + "go.datum.net/galactic/internal/plumbing/ebpf/attach" "go.datum.net/galactic/internal/plumbing/ebpf/uformat" "go.datum.net/galactic/internal/plumbing/ebpf/usidmap" + "go.datum.net/galactic/internal/plumbing/srv6" + "go.datum.net/galactic/internal/plumbing/vrf" bgpv1alpha1 "go.datum.net/network/api/v1alpha1" ) @@ -442,6 +446,35 @@ func withNetNSExistsFn(t *testing.T, fn func(string) bool) { t.Cleanup(func() { netNSExistsFn = orig }) } +// withTempPinDir points the package-level ebpfPinDir var (see cnibgp.go's +// doc comment) at a throwaway bpffs directory for the duration of the test, +// loading the eBPF datapath's pinned maps into it first (simulating the run +// container having already loaded the datapath) and adding this VPC's real +// kernel VRF — the same two preconditions bgp_ebpf_test.go's own +// requireRoot-gated tests set up before calling registerEBPFDatapath +// directly. Restores ebpfPinDir and tears both down on cleanup. Callers +// must call requireRoot(t) first. +func withTempPinDir(t *testing.T) { + t.Helper() + + if err := vrf.Add(testVPC); err != nil { + t.Fatalf("vrf.Add: %v", err) + } + t.Cleanup(func() { _ = vrf.Delete(testVPC) }) + + pinDir := fmt.Sprintf("/sys/fs/bpf/galactic-bgp-test-%d", os.Getpid()) + t.Cleanup(func() { _ = os.RemoveAll(pinDir) }) + loaderObjs, err := attach.Load(pinDir) + if err != nil { + t.Fatalf("attach.Load: %v", err) + } + t.Cleanup(func() { _ = loaderObjs.Close() }) + + orig := ebpfPinDir + ebpfPinDir = pinDir + t.Cleanup(func() { ebpfPinDir = orig }) +} + func TestPruneDeadContainerAnnotationsRemovesDeadSibling(t *testing.T) { const subnet = "fd00:40:ff01::100:0/96" live := map[string]bool{testLiveNetnsPath: true, testDeadNetnsPath: false} @@ -632,6 +665,140 @@ func TestPublishBGPStateIPAMClearsNoAddressing(t *testing.T) { } } +// TestPublishBGPStateSIDNotComputedWhenSRv6Unconfigured verifies that +// result.sid stays at its zero value (IsValid() == false) when this node's +// BGPRouter has no SRv6Locator/nodeID configured — the same "SRv6 not +// configured, skip silently" sentinel registerEBPFDatapath's own no-op case +// establishes. routerForNode leaves SRv6Locator/NodeID unset, matching that +// case, so this test needs no root privileges (registerEBPFDatapath itself +// never opens a pinned eBPF map here). +func TestPublishBGPStateSIDNotComputedWhenSRv6Unconfigured(t *testing.T) { + const ( + nodeName = "node1" + namespace = "default" + ) + withNetNSExistsFn(t, func(path string) bool { return path == testNetns }) + + router := routerForNode(testRouterName, nodeName, namespace, 65000) + k8s := fakeClient(router) + + ipv6Subnet := mustParseCIDR(t, "fd00:40:ff01::100:0/96") + ipamResult := &cniipam.IPAMResult{IPv6Subnet: ipv6Subnet} + cfg := publishConfig{vpc: testVPC, vpcAttachment: testAttachment, ifaceType: ifaceTypeVeth} + args := &skel.CmdArgs{ContainerID: "unconfigured-srv6-container", Netns: testNetns} + + result, err := publishBGPState(args, cfg, nodeName, namespace, ipamResult, testVPCHex1234, k8s) + if err != nil { + t.Fatalf("publishBGPState: unexpected error: %v", err) + } + if result.sid.IsValid() { + t.Errorf("result.sid = %v, want the zero value when SRv6 is not configured", result.sid) + } +} + +// TestPublishBGPStateComputesSIDWhenSRv6Configured verifies result.sid is +// computed via srv6.ComputeSID once vrfID is allocated, matching what an +// independent call with the same (locator, nodeID, vrfID, function) tuple +// would produce. Requires root: once SRv6Locator/NodeID are configured, the +// same code path also calls registerEBPFDatapath, which opens real pinned +// eBPF maps (see bgp_ebpf_test.go's own requireRoot-gated tests). +func TestPublishBGPStateComputesSIDWhenSRv6Configured(t *testing.T) { + requireRoot(t) + withTempPinDir(t) + + const ( + nodeName = "node1" + namespace = "default" + srv6Locator = "fd00:10::/48" + nodeID = 7 + ) + withNetNSExistsFn(t, func(path string) bool { return path == testNetns }) + + router := routerForNode(testRouterName, nodeName, namespace, 65000) + router.Spec.SRv6Locator = srv6Locator + router.Spec.NodeID = nodeID + k8s := fakeClient(router) + + ipv6Subnet := mustParseCIDR(t, "fd00:40:ff01::100:0/96") + ipamResult := &cniipam.IPAMResult{IPv6Subnet: ipv6Subnet} + cfg := publishConfig{vpc: testVPC, vpcAttachment: testAttachment, ifaceType: ifaceTypeVeth} + args := &skel.CmdArgs{ContainerID: "configured-srv6-container", Netns: testNetns} + + result, err := publishBGPState(args, cfg, nodeName, namespace, ipamResult, testVPCHex1234, k8s) + if err != nil { + t.Fatalf("publishBGPState: unexpected error: %v", err) + } + if !result.sid.IsValid() { + t.Fatal("result.sid is invalid, want a computed SID when SRv6 is configured") + } + + vrfList := &bgpv1alpha1.BGPVRFInstanceList{} + if err := k8s.List(context.Background(), vrfList, client.InNamespace(namespace)); err != nil { + t.Fatalf("list BGPVRFInstances: %v", err) + } + if len(vrfList.Items) != 1 { + t.Fatalf("BGPVRFInstances = %d, want exactly 1", len(vrfList.Items)) + } + want, err := srv6.ComputeSID(srv6Locator, nodeID, vrfList.Items[0].Spec.VRFID, bgpv1alpha1.SRv6FunctionEndDT46) + if err != nil { + t.Fatalf("srv6.ComputeSID: %v", err) + } + if result.sid != want { + t.Errorf("result.sid = %v, want %v", result.sid, want) + } +} + +// TestPublishBGPStateAdvertisementCreatedGatedOnCreate verifies the fix for +// the #854 plan's Phase 4 rollback-risk callout: result.advertisementCreated +// is true only when this call's own CreateOrUpdate genuinely created the +// BGPAdvertisement, not when it merely updated one a sibling attachment's +// earlier ADD already created — mirroring vrfInstanceCreated's existing +// gating exactly. Without this, resourceTracker.cleanup could delete a +// BGPAdvertisement still backing a live sibling's route on an unrelated +// later failure. +func TestPublishBGPStateAdvertisementCreatedGatedOnCreate(t *testing.T) { + const ( + nodeName = "node1" + namespace = "default" + ) + withNetNSExistsFn(t, func(path string) bool { return path == testNetns }) + + router := routerForNode(testRouterName, nodeName, namespace, 65000) + ipv6Subnet := mustParseCIDR(t, "fd00:40:ff01::100:0/96") + ipamResult := &cniipam.IPAMResult{IPv6Subnet: ipv6Subnet} + cfg := publishConfig{vpc: testVPC, vpcAttachment: testAttachment, ifaceType: ifaceTypeVeth} + + t.Run("first attachment: genuine create", func(t *testing.T) { + k8s := fakeClient(router) + args := &skel.CmdArgs{ContainerID: "first-container", Netns: testNetns} + + result, err := publishBGPState(args, cfg, nodeName, namespace, ipamResult, testVPCHex1234, k8s) + if err != nil { + t.Fatalf("publishBGPState: unexpected error: %v", err) + } + if !result.advertisementCreated { + t.Error("advertisementCreated = false, want true for a genuine create") + } + }) + + t.Run("sibling attachment reusing an already-live BGPAdvertisement: update only", func(t *testing.T) { + advName := crdnames.BGPAdvertisementName(testVPC, testAttachment) + existing := &bgpv1alpha1.BGPAdvertisement{ + ObjectMeta: metav1.ObjectMeta{Name: advName, Namespace: namespace}, + } + k8s := fakeClient(router, existing) + args := &skel.CmdArgs{ContainerID: "second-container", Netns: testNetns} + + result, err := publishBGPState(args, cfg, nodeName, namespace, ipamResult, testVPCHex1234, k8s) + if err != nil { + t.Fatalf("publishBGPState: unexpected error: %v", err) + } + if result.advertisementCreated { + t.Error("advertisementCreated = true, want false when the BGPAdvertisement already existed (update only)") + } + }) +} + // ---- buildAdvertisementSpec ------------------------------------------------- func TestBuildAdvertisementSpec(t *testing.T) { diff --git a/internal/cnibgp/endpointslice.go b/internal/cnibgp/endpointslice.go new file mode 100644 index 00000000..3dfab847 --- /dev/null +++ b/internal/cnibgp/endpointslice.go @@ -0,0 +1,135 @@ +// Copyright 2026 Datum Cloud, Inc. +// +// SPDX-License-Identifier: AGPL-3.0-or-later + +package cnibgp + +import ( + "context" + "fmt" + "log/slog" + "net" + "net/netip" + + corev1 "k8s.io/api/core/v1" + discoveryv1 "k8s.io/api/discovery/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" + + "go.datum.net/galactic/internal/crdnames" +) + +// publishEndpointSlice creates or updates the per-pod discoveryv1.EndpointSlice +// that the HTTP-ingress extension server discovers VPC backends through (see +// crdnames.LabelTenantID's doc comment — Open Decision 2 of the #854 plan). +// One EndpointSlice per pod, named after the pod (crdnames.EndpointSliceName), +// IPv6-only (Open Decision 1: a dual-stack pod's IPv4 address is not +// published). +// +// Runs as its own step after publishBGPState returns successfully, not +// folded into its retry closure — see the #854 plan's Phase 4 rollback-risk +// note for why that sequencing, combined with fixing advertisementCreated's +// gating (bgp.go), is what keeps a failure here from ever causing rollback to +// delete a BGPAdvertisement still backing a live sibling. +// +// Also sets metadata.ownerReferences to the owning Pod (Open Decision 6 / +// Phase 8: the k8s garbage collector's own reclaim is the backstop for +// force-deleted/never-DEL'd pods; cmdDel's explicit delete, ops_del.go, is +// the fast, deterministic path for the common case). +func publishEndpointSlice( + ctx context.Context, k8s client.Client, namespace, podName, vpc, vpcAttachment string, addr net.IP, sid netip.Addr, +) error { + pod := &corev1.Pod{} + if err := k8s.Get(ctx, client.ObjectKey{Name: podName, Namespace: namespace}, pod); err != nil { + return fmt.Errorf("get owning Pod %s/%s: %w", namespace, podName, err) + } + + name := crdnames.EndpointSliceName(podName) + tenantID := crdnames.TenantIdentifier(vpc, vpcAttachment) + + slice := &discoveryv1.EndpointSlice{} + getErr := k8s.Get(ctx, client.ObjectKey{Name: name, Namespace: namespace}, slice) + switch { + case getErr == nil: + // Naming-collision defensive check: EndpointSliceName is a trivial + // passthrough of the pod's own name, so nothing but convention stops + // some other EndpointSlice (Service-backed or otherwise) from + // landing on this exact name/namespace. Bail rather than silently + // start mutating an object this plugin doesn't own. + if _, ok := slice.Labels[crdnames.LabelTenantID]; !ok { + return fmt.Errorf( + "EndpointSlice %s/%s already exists without a %s label — refusing to overwrite an object this plugin doesn't own", + namespace, name, crdnames.LabelTenantID) + } + case apierrors.IsNotFound(getErr): + slice = &discoveryv1.EndpointSlice{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: namespace, + }, + } + default: + return fmt.Errorf("get EndpointSlice %s/%s: %w", namespace, name, getErr) + } + + op, err := controllerutil.CreateOrUpdate(ctx, k8s, slice, func() error { + if slice.Labels == nil { + slice.Labels = make(map[string]string, 1) + } + slice.Labels[crdnames.LabelTenantID] = tenantID + + if slice.Annotations == nil { + slice.Annotations = make(map[string]string, 2) + } + slice.Annotations[crdnames.AnnotationTenantID] = tenantID + if sid.IsValid() { + slice.Annotations[crdnames.AnnotationSID] = sid.String() + } else { + delete(slice.Annotations, crdnames.AnnotationSID) + } + + slice.AddressType = discoveryv1.AddressTypeIPv6 + ready := true + slice.Endpoints = []discoveryv1.Endpoint{{ + Addresses: []string{addr.String()}, + Conditions: discoveryv1.EndpointConditions{Ready: &ready}, + TargetRef: &corev1.ObjectReference{ + Kind: "Pod", + Name: pod.Name, + Namespace: pod.Namespace, + UID: pod.UID, + }, + }} + + // Same namespace (Pod and EndpointSlice always are, here), so the + // cross-namespace-owner restriction doesn't apply. Not a controller + // ref (SetOwnerReference, not SetControllerReference) and + // BlockOwnerDeletion left at its default false — ordering doesn't + // matter for this object. + return controllerutil.SetOwnerReference(pod, slice, k8s.Scheme()) + }) + if err != nil { + return fmt.Errorf("apply EndpointSlice: %w", err) + } + slog.Debug("ADD: EndpointSlice applied", "name", name, "namespace", namespace, + "tenantID", tenantID, "operation", op) + return nil +} + +// deleteEndpointSlice deletes the per-pod EndpointSlice cmdAdd published, +// treating not-found as success — see cmdDel's own doc comment for why DEL, +// unlike the rest of the chain's DEL paths, does real work here. +func deleteEndpointSlice(ctx context.Context, k8s client.Client, namespace, podName string) error { + slice := &discoveryv1.EndpointSlice{ + ObjectMeta: metav1.ObjectMeta{ + Name: crdnames.EndpointSliceName(podName), + Namespace: namespace, + }, + } + if err := k8s.Delete(ctx, slice); client.IgnoreNotFound(err) != nil { + return fmt.Errorf("delete EndpointSlice %s/%s: %w", namespace, slice.Name, err) + } + return nil +} diff --git a/internal/cnibgp/endpointslice_test.go b/internal/cnibgp/endpointslice_test.go new file mode 100644 index 00000000..785c0214 --- /dev/null +++ b/internal/cnibgp/endpointslice_test.go @@ -0,0 +1,241 @@ +// Copyright 2026 Datum Cloud, Inc. +// +// SPDX-License-Identifier: AGPL-3.0-or-later + +package cnibgp + +import ( + "context" + "net" + "net/netip" + "strings" + "testing" + + corev1 "k8s.io/api/core/v1" + discoveryv1 "k8s.io/api/discovery/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" + + "go.datum.net/galactic/internal/crdnames" +) + +const ( + testPodName = "web-0" + testPodNamespace = "default" + testPodUID = types.UID("11111111-1111-1111-1111-111111111111") + testPodAddr = "fd00::1" + + // testStaleTenantID stands in for a value some earlier ADD wrote, shared + // across this file's and ops_check_test.go's "stale value gets + // refreshed/detected" cases. + testStaleTenantID = "stale-tenant" +) + +func testPod() *corev1.Pod { + return &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: testPodName, + Namespace: testPodNamespace, + UID: testPodUID, + }, + } +} + +func mustParseAddr(t *testing.T) net.IP { + t.Helper() + ip := net.ParseIP(testPodAddr) + if ip == nil { + t.Fatalf("net.ParseIP(%q) failed", testPodAddr) + } + return ip +} + +func TestPublishEndpointSliceFreshPublish(t *testing.T) { + testSID := netip.MustParseAddr("fd00:1::1") + pod := testPod() + k8s := fakeClient(pod) + + err := publishEndpointSlice( + context.Background(), k8s, testPodNamespace, testPodName, testVPC, testAttachment, + mustParseAddr(t), testSID, + ) + if err != nil { + t.Fatalf("publishEndpointSlice() = %v, want nil", err) + } + + got := &discoveryv1.EndpointSlice{} + if err := k8s.Get(context.Background(), + client.ObjectKey{Name: testPodName, Namespace: testPodNamespace}, got); err != nil { + t.Fatalf("get EndpointSlice after publish: %v", err) + } + + wantTenantID := crdnames.TenantIdentifier(testVPC, testAttachment) + if got.Labels[crdnames.LabelTenantID] != wantTenantID { + t.Errorf("label %s = %q, want %q", crdnames.LabelTenantID, got.Labels[crdnames.LabelTenantID], wantTenantID) + } + if got.Annotations[crdnames.AnnotationTenantID] != wantTenantID { + t.Errorf("annotation %s = %q, want %q", + crdnames.AnnotationTenantID, got.Annotations[crdnames.AnnotationTenantID], wantTenantID) + } + if got.Annotations[crdnames.AnnotationSID] != testSID.String() { + t.Errorf("annotation %s = %q, want %q", + crdnames.AnnotationSID, got.Annotations[crdnames.AnnotationSID], testSID.String()) + } + if got.AddressType != discoveryv1.AddressTypeIPv6 { + t.Errorf("AddressType = %q, want %q", got.AddressType, discoveryv1.AddressTypeIPv6) + } + if len(got.Endpoints) != 1 || len(got.Endpoints[0].Addresses) != 1 || got.Endpoints[0].Addresses[0] != testPodAddr { + t.Errorf("Endpoints = %+v, want a single endpoint with address %q", got.Endpoints, testPodAddr) + } + if got.Endpoints[0].Conditions.Ready == nil || !*got.Endpoints[0].Conditions.Ready { + t.Errorf("Endpoints[0].Conditions.Ready = %v, want true", got.Endpoints[0].Conditions.Ready) + } + + owners := got.GetOwnerReferences() + if len(owners) != 1 || owners[0].Name != testPodName || owners[0].UID != testPodUID { + t.Errorf("OwnerReferences = %+v, want a single owner referencing pod %s (UID %s)", + owners, testPodName, testPodUID) + } + if owners[0].Controller != nil && *owners[0].Controller { + t.Error("owner reference Controller = true, want false/nil (SetOwnerReference, not SetControllerReference)") + } +} + +func TestPublishEndpointSliceSRv6NotConfiguredOmitsSID(t *testing.T) { + pod := testPod() + k8s := fakeClient(pod) + + err := publishEndpointSlice( + context.Background(), k8s, testPodNamespace, testPodName, testVPC, testAttachment, + mustParseAddr(t), netip.Addr{}, // zero value: SID not computed + ) + if err != nil { + t.Fatalf("publishEndpointSlice() = %v, want nil", err) + } + + got := &discoveryv1.EndpointSlice{} + if err := k8s.Get(context.Background(), + client.ObjectKey{Name: testPodName, Namespace: testPodNamespace}, got); err != nil { + t.Fatalf("get EndpointSlice after publish: %v", err) + } + if _, ok := got.Annotations[crdnames.AnnotationSID]; ok { + t.Errorf("annotation %s present = %q, want absent when SID was not computed", + crdnames.AnnotationSID, got.Annotations[crdnames.AnnotationSID]) + } +} + +func TestPublishEndpointSliceUpdateInPlace(t *testing.T) { + testSID := netip.MustParseAddr("fd00:1::1") + pod := testPod() + existing := &discoveryv1.EndpointSlice{ + ObjectMeta: metav1.ObjectMeta{ + Name: testPodName, + Namespace: testPodNamespace, + Labels: map[string]string{crdnames.LabelTenantID: testStaleTenantID}, + }, + AddressType: discoveryv1.AddressTypeIPv6, + Endpoints: []discoveryv1.Endpoint{{Addresses: []string{"fd00::dead"}}}, + } + k8s := fakeClient(pod, existing) + + if err := publishEndpointSlice( + context.Background(), k8s, testPodNamespace, testPodName, testVPC, testAttachment, + mustParseAddr(t), testSID, + ); err != nil { + t.Fatalf("publishEndpointSlice() = %v, want nil", err) + } + + got := &discoveryv1.EndpointSlice{} + if err := k8s.Get(context.Background(), + client.ObjectKey{Name: testPodName, Namespace: testPodNamespace}, got); err != nil { + t.Fatalf("get EndpointSlice after update: %v", err) + } + wantTenantID := crdnames.TenantIdentifier(testVPC, testAttachment) + if got.Labels[crdnames.LabelTenantID] != wantTenantID { + t.Errorf("label %s = %q, want %q (refreshed)", + crdnames.LabelTenantID, got.Labels[crdnames.LabelTenantID], wantTenantID) + } + if len(got.Endpoints) != 1 || got.Endpoints[0].Addresses[0] != testPodAddr { + t.Errorf("Endpoints = %+v, want the refreshed address %q", got.Endpoints, testPodAddr) + } +} + +func TestPublishEndpointSliceNamingCollisionNotOverwritten(t *testing.T) { + testSID := netip.MustParseAddr("fd00:1::1") + pod := testPod() + foreign := &discoveryv1.EndpointSlice{ + ObjectMeta: metav1.ObjectMeta{ + Name: testPodName, + Namespace: testPodNamespace, + Labels: map[string]string{"kubernetes.io/service-name": "some-service"}, + }, + AddressType: discoveryv1.AddressTypeIPv4, + } + k8s := fakeClient(pod, foreign) + + err := publishEndpointSlice( + context.Background(), k8s, testPodNamespace, testPodName, testVPC, testAttachment, + mustParseAddr(t), testSID, + ) + if err == nil { + t.Fatal("publishEndpointSlice() = nil, want an error for a non-tenant-labeled name collision") + } + if !strings.Contains(err.Error(), crdnames.LabelTenantID) { + t.Errorf("error %q does not mention %s", err, crdnames.LabelTenantID) + } + + got := &discoveryv1.EndpointSlice{} + if err := k8s.Get(context.Background(), + client.ObjectKey{Name: testPodName, Namespace: testPodNamespace}, got); err != nil { + t.Fatalf("get EndpointSlice after refused publish: %v", err) + } + if got.AddressType != discoveryv1.AddressTypeIPv4 { + t.Errorf("foreign EndpointSlice was mutated: AddressType = %q, want unchanged %q", + got.AddressType, discoveryv1.AddressTypeIPv4) + } +} + +func TestPublishEndpointSliceOwningPodNotFound(t *testing.T) { + testSID := netip.MustParseAddr("fd00:1::1") + k8s := fakeClient() + + err := publishEndpointSlice( + context.Background(), k8s, testPodNamespace, testPodName, testVPC, testAttachment, + mustParseAddr(t), testSID, + ) + if err == nil { + t.Fatal("publishEndpointSlice() = nil, want an error when the owning Pod does not exist") + } + if client.IgnoreNotFound(err) != nil && !apierrors.IsNotFound(client.IgnoreNotFound(err)) { + t.Errorf("expected error to wrap a not-found status, got: %v", err) + } +} + +func TestDeleteEndpointSlice(t *testing.T) { + t.Run("deletes an existing EndpointSlice", func(t *testing.T) { + slice := &discoveryv1.EndpointSlice{ + ObjectMeta: metav1.ObjectMeta{Name: testPodName, Namespace: testPodNamespace}, + } + k8s := fakeClient(slice) + + if err := deleteEndpointSlice(context.Background(), k8s, testPodNamespace, testPodName); err != nil { + t.Fatalf("deleteEndpointSlice() = %v, want nil", err) + } + + err := k8s.Get(context.Background(), client.ObjectKey{Name: testPodName, Namespace: testPodNamespace}, + &discoveryv1.EndpointSlice{}) + if !apierrors.IsNotFound(err) { + t.Errorf("expected NotFound after delete, got: %v", err) + } + }) + + t.Run("idempotent when EndpointSlice does not exist", func(t *testing.T) { + k8s := fakeClient() + + if err := deleteEndpointSlice(context.Background(), k8s, testPodNamespace, testPodName); err != nil { + t.Fatalf("deleteEndpointSlice() on absent object = %v, want nil", err) + } + }) +} diff --git a/internal/cnibgp/ops_add.go b/internal/cnibgp/ops_add.go index fb992c01..2c5909fe 100644 --- a/internal/cnibgp/ops_add.go +++ b/internal/cnibgp/ops_add.go @@ -12,6 +12,7 @@ import ( "github.com/containernetworking/cni/pkg/skel" "github.com/containernetworking/cni/pkg/types" + "go.datum.net/galactic/internal/nadpatch" "go.datum.net/galactic/internal/plumbing/intf" ) @@ -84,6 +85,30 @@ func cmdAdd(args *skel.CmdArgs) (err error) { return err } + // EndpointSlice publish is a separate step after publishBGPState + // succeeds, not folded into its retry closure — see the #854 plan's + // Phase 4 rollback-risk note. ipamResult == nil or carrying no IPv6 + // address is the same "no address to publish" skip + // registerEBPFDatapath's own SRv6-not-configured case already + // establishes: not an error, and not tap/VM-specific (see Open Decision + // 5) — an attachment with no IPv6 allocation has nothing for an + // EndpointSlice to carry either way. + if ipamResult != nil && ipamResult.IPv6Subnet != nil { + podName := nadpatch.ParsePodName(args.Args) + if podName == "" { + return fmt.Errorf("publish EndpointSlice: no K8S_POD_NAME in CNI_ARGS %q", args.Args) + } + esCtx, esCancel := context.WithTimeout(context.Background(), cniTimeout) + esErr := publishEndpointSlice( + esCtx, k8sClient, namespace, podName, pluginConf.VPC, pluginConf.VPCAttachment, + ipamResult.IPv6Subnet.IP, result.sid, + ) + esCancel() + if esErr != nil { + return fmt.Errorf("publish EndpointSlice: %w", esErr) + } + } + // Pass prevResult through unchanged: this plugin adds no new interfaces // or IPs of its own, so its own CNI result is exactly what it received. // Per the design note's sample conflists, galactic-bgp is the last diff --git a/internal/cnibgp/ops_check.go b/internal/cnibgp/ops_check.go index 3cb973bc..b5526ee5 100644 --- a/internal/cnibgp/ops_check.go +++ b/internal/cnibgp/ops_check.go @@ -9,6 +9,7 @@ import ( "errors" "fmt" "log/slog" + "net" "net/http" "net/netip" "os" @@ -16,14 +17,17 @@ import ( "github.com/containernetworking/cni/pkg/skel" "github.com/containernetworking/cni/pkg/types" + discoveryv1 "k8s.io/api/discovery/v1" "k8s.io/client-go/rest" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "go.datum.net/galactic/internal/config" "go.datum.net/galactic/internal/crdnames" + "go.datum.net/galactic/internal/nadpatch" "go.datum.net/galactic/internal/plumbing/ebpf/uformat" "go.datum.net/galactic/internal/plumbing/ebpf/usidmap" + "go.datum.net/galactic/internal/plumbing/srv6" "go.datum.net/galactic/internal/plumbing/vrf" bgpv1alpha1 "go.datum.net/network/api/v1alpha1" ) @@ -66,6 +70,23 @@ func cmdCheck(args *skel.CmdArgs) error { errs = append(errs, fmt.Errorf("BGPAdvertisement %s: %w", advName, err)) } + // ipamResult == nil or carrying no IPv6 address means cmdAdd never + // published an EndpointSlice for this attachment in the first place — + // same "no address to publish" skip as cmdAdd's own (see ops_add.go), + // not tap/VM-specific (Open Decision 5). + if _, ipamResult, _, prevErr := inferFromPrevResult(pluginConf.RawPrevResult); prevErr != nil { + errs = append(errs, fmt.Errorf("infer from prevResult: %w", prevErr)) + } else if ipamResult != nil && ipamResult.IPv6Subnet != nil { + podName := nadpatch.ParsePodName(args.Args) + if podName == "" { + errs = append(errs, errors.New("EndpointSlice: no K8S_POD_NAME in CNI_ARGS")) + } else if err := checkEndpointSlice( + ctx, k8s, pluginConf, podName, ipamResult.IPv6Subnet.IP, vrfErr == nil, vrfInst.Spec.VRFID, + ); err != nil { + errs = append(errs, err) + } + } + // The eBPF vrf_table entry is only checkable once the BGPVRFInstance // lookup succeeded (it carries the Argument value the entry is keyed // on) and this node's router actually has SRv6 configured — matches @@ -163,6 +184,63 @@ func checkEBPFEntry(ctx context.Context, k8s client.Client, pluginConf *PluginCo return errors.Join(errs...) } +// checkEndpointSlice verifies the per-pod EndpointSlice cmdAdd published +// (endpointslice.go) is still in place: it exists, carries the pod's +// current address, and its tenant-id/SID label and annotations match +// freshly recomputed expected values. vrfIDKnown is false when the +// BGPVRFInstance lookup in cmdCheck above failed, in which case the SID +// can't be recomputed and its annotation is not checked — matches +// registerEBPFDatapath/checkEBPFEntry's own "can't check what we can't +// compute" convention. +func checkEndpointSlice( + ctx context.Context, k8s client.Client, pluginConf *PluginConf, podName string, + addr net.IP, vrfIDKnown bool, vrfID int32, +) error { + name := crdnames.EndpointSliceName(podName) + slice := &discoveryv1.EndpointSlice{} + if err := k8s.Get(ctx, client.ObjectKey{Name: name, Namespace: pluginConf.Namespace}, slice); err != nil { + return fmt.Errorf("EndpointSlice %s: %w", name, err) + } + + var errs []error + + wantAddr := addr.String() + var gotAddr string + if len(slice.Endpoints) > 0 && len(slice.Endpoints[0].Addresses) > 0 { + gotAddr = slice.Endpoints[0].Addresses[0] + } + if gotAddr != wantAddr { + errs = append(errs, fmt.Errorf("EndpointSlice %s address = %q, want %q", name, gotAddr, wantAddr)) + } + + wantTenantID := crdnames.TenantIdentifier(pluginConf.VPC, pluginConf.VPCAttachment) + if got := slice.Labels[crdnames.LabelTenantID]; got != wantTenantID { + errs = append(errs, fmt.Errorf( + "EndpointSlice %s label %s = %q, want %q", name, crdnames.LabelTenantID, got, wantTenantID)) + } + if got := slice.Annotations[crdnames.AnnotationTenantID]; got != wantTenantID { + errs = append(errs, fmt.Errorf( + "EndpointSlice %s annotation %s = %q, want %q", name, crdnames.AnnotationTenantID, got, wantTenantID)) + } + + if vrfIDKnown { + bgp, err := lookupBGPRouter(ctx, k8s, cniConfig.NodeName, pluginConf.Namespace) + if err != nil { + errs = append(errs, fmt.Errorf("look up BGPRouter for EndpointSlice SID check: %w", err)) + } else if bgp.srv6Locator != "" && bgp.nodeID != 0 { + sid, err := srv6.ComputeSID(bgp.srv6Locator, bgp.nodeID, vrfID, bgpv1alpha1.SRv6FunctionEndDT46) + if err != nil { + errs = append(errs, fmt.Errorf("compute expected SRv6 uSID for EndpointSlice check: %w", err)) + } else if got := slice.Annotations[crdnames.AnnotationSID]; got != sid.String() { + errs = append(errs, fmt.Errorf( + "EndpointSlice %s annotation %s = %q, want %q", name, crdnames.AnnotationSID, got, sid.String())) + } + } + } + + return errors.Join(errs...) +} + // cmdStatus implements the CNI spec STATUS operation — galactic-bgp talks // to the API server (BGP CRD reads/writes), so this probes it the same way // internal/cni's own cmdStatus does. diff --git a/internal/cnibgp/ops_check_test.go b/internal/cnibgp/ops_check_test.go new file mode 100644 index 00000000..8d8b6f27 --- /dev/null +++ b/internal/cnibgp/ops_check_test.go @@ -0,0 +1,208 @@ +// Copyright 2026 Datum Cloud, Inc. +// +// SPDX-License-Identifier: AGPL-3.0-or-later + +package cnibgp + +import ( + "context" + "strings" + "testing" + + discoveryv1 "k8s.io/api/discovery/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + "go.datum.net/galactic/internal/config" + "go.datum.net/galactic/internal/crdnames" + "go.datum.net/galactic/internal/plumbing/srv6" + bgpv1alpha1 "go.datum.net/network/api/v1alpha1" +) + +func withNodeName(t *testing.T, nodeName string) { + t.Helper() + t.Setenv(config.EnvCNINodeName, nodeName) + orig := cniConfig + cniConfig = config.NewCNIConfig() + cniConfig.Resolve(&config.ConflistValues{NodeName: nodeName}) + t.Cleanup(func() { cniConfig = orig }) +} + +func testPluginConf() *PluginConf { + return &PluginConf{VPC: testVPC, VPCAttachment: testAttachment, Namespace: testPodNamespace} +} + +func TestCheckEndpointSlice(t *testing.T) { + const nodeName = "node1" + withNodeName(t, nodeName) + + wantTenantID := crdnames.TenantIdentifier(testVPC, testAttachment) + + t.Run("missing EndpointSlice is an error", func(t *testing.T) { + k8s := fakeClient() + err := checkEndpointSlice( + context.Background(), k8s, testPluginConf(), testPodName, mustParseAddr(t), false, 0) + if err == nil { + t.Fatal("checkEndpointSlice() = nil, want an error when the EndpointSlice does not exist") + } + }) + + t.Run("address mismatch is an error", func(t *testing.T) { + slice := &discoveryv1.EndpointSlice{ + ObjectMeta: metav1.ObjectMeta{ + Name: testPodName, + Namespace: testPodNamespace, + Labels: map[string]string{crdnames.LabelTenantID: wantTenantID}, + Annotations: map[string]string{ + crdnames.AnnotationTenantID: wantTenantID, + }, + }, + Endpoints: []discoveryv1.Endpoint{{Addresses: []string{"fd00::dead"}}}, + } + k8s := fakeClient(slice) + err := checkEndpointSlice( + context.Background(), k8s, testPluginConf(), testPodName, mustParseAddr(t), false, 0) + if err == nil { + t.Fatal("checkEndpointSlice() = nil, want an address-mismatch error") + } + if !strings.Contains(err.Error(), "address") { + t.Errorf("error %q does not mention address", err) + } + }) + + t.Run("tenant label/annotation mismatch is an error", func(t *testing.T) { + slice := &discoveryv1.EndpointSlice{ + ObjectMeta: metav1.ObjectMeta{ + Name: testPodName, + Namespace: testPodNamespace, + Labels: map[string]string{crdnames.LabelTenantID: testStaleTenantID}, + Annotations: map[string]string{ + crdnames.AnnotationTenantID: testStaleTenantID, + }, + }, + Endpoints: []discoveryv1.Endpoint{{Addresses: []string{testPodAddr}}}, + } + k8s := fakeClient(slice) + err := checkEndpointSlice( + context.Background(), k8s, testPluginConf(), testPodName, mustParseAddr(t), false, 0) + if err == nil { + t.Fatal("checkEndpointSlice() = nil, want a tenant-id mismatch error") + } + if !strings.Contains(err.Error(), crdnames.LabelTenantID) { + t.Errorf("error %q does not mention %s", err, crdnames.LabelTenantID) + } + }) + + t.Run("everything matches, vrfIDKnown false: SID not checked", func(t *testing.T) { + slice := &discoveryv1.EndpointSlice{ + ObjectMeta: metav1.ObjectMeta{ + Name: testPodName, + Namespace: testPodNamespace, + Labels: map[string]string{crdnames.LabelTenantID: wantTenantID}, + Annotations: map[string]string{ + crdnames.AnnotationTenantID: wantTenantID, + }, + }, + Endpoints: []discoveryv1.Endpoint{{Addresses: []string{testPodAddr}}}, + } + k8s := fakeClient(slice) + err := checkEndpointSlice( + context.Background(), k8s, testPluginConf(), testPodName, mustParseAddr(t), false, 0) + if err != nil { + t.Fatalf("checkEndpointSlice() = %v, want nil", err) + } + }) + + t.Run("vrfIDKnown true, SRv6 configured: SID mismatch is an error", func(t *testing.T) { + const ( + locator = "fd00:10::/48" + srvNode = 7 + vrfID = int32(1234) + ) + router := routerForNode(testRouterName, nodeName, testPodNamespace, 65000) + router.Spec.SRv6Locator = locator + router.Spec.NodeID = srvNode + + slice := &discoveryv1.EndpointSlice{ + ObjectMeta: metav1.ObjectMeta{ + Name: testPodName, + Namespace: testPodNamespace, + Labels: map[string]string{crdnames.LabelTenantID: wantTenantID}, + Annotations: map[string]string{ + crdnames.AnnotationTenantID: wantTenantID, + crdnames.AnnotationSID: "fd00::dead:beef", + }, + }, + Endpoints: []discoveryv1.Endpoint{{Addresses: []string{testPodAddr}}}, + } + k8s := fakeClient(router, slice) + + err := checkEndpointSlice( + context.Background(), k8s, testPluginConf(), testPodName, mustParseAddr(t), true, vrfID) + if err == nil { + t.Fatal("checkEndpointSlice() = nil, want a SID-mismatch error") + } + if !strings.Contains(err.Error(), crdnames.AnnotationSID) { + t.Errorf("error %q does not mention %s", err, crdnames.AnnotationSID) + } + }) + + t.Run("vrfIDKnown true, SRv6 configured: matching SID passes", func(t *testing.T) { + const ( + locator = "fd00:10::/48" + srvNode = 7 + vrfID = int32(1234) + ) + router := routerForNode(testRouterName, nodeName, testPodNamespace, 65000) + router.Spec.SRv6Locator = locator + router.Spec.NodeID = srvNode + + wantSID, err := srv6.ComputeSID(locator, srvNode, vrfID, bgpv1alpha1.SRv6FunctionEndDT46) + if err != nil { + t.Fatalf("srv6.ComputeSID: %v", err) + } + + slice := &discoveryv1.EndpointSlice{ + ObjectMeta: metav1.ObjectMeta{ + Name: testPodName, + Namespace: testPodNamespace, + Labels: map[string]string{crdnames.LabelTenantID: wantTenantID}, + Annotations: map[string]string{ + crdnames.AnnotationTenantID: wantTenantID, + crdnames.AnnotationSID: wantSID.String(), + }, + }, + Endpoints: []discoveryv1.Endpoint{{Addresses: []string{testPodAddr}}}, + } + k8s := fakeClient(router, slice) + + if err := checkEndpointSlice( + context.Background(), k8s, testPluginConf(), testPodName, mustParseAddr(t), true, vrfID, + ); err != nil { + t.Fatalf("checkEndpointSlice() = %v, want nil", err) + } + }) + + t.Run("vrfIDKnown true, SRv6 not configured: SID not checked", func(t *testing.T) { + const vrfID = int32(1234) + router := routerForNode(testRouterName, nodeName, testPodNamespace, 65000) + + slice := &discoveryv1.EndpointSlice{ + ObjectMeta: metav1.ObjectMeta{ + Name: testPodName, + Namespace: testPodNamespace, + Labels: map[string]string{crdnames.LabelTenantID: wantTenantID}, + Annotations: map[string]string{ + crdnames.AnnotationTenantID: wantTenantID, + }, + }, + Endpoints: []discoveryv1.Endpoint{{Addresses: []string{testPodAddr}}}, + } + k8s := fakeClient(router, slice) + + if err := checkEndpointSlice( + context.Background(), k8s, testPluginConf(), testPodName, mustParseAddr(t), true, vrfID, + ); err != nil { + t.Fatalf("checkEndpointSlice() = %v, want nil", err) + } + }) +} diff --git a/internal/cnibgp/ops_del.go b/internal/cnibgp/ops_del.go index 5bd8abba..5b25d9db 100644 --- a/internal/cnibgp/ops_del.go +++ b/internal/cnibgp/ops_del.go @@ -5,20 +5,32 @@ package cnibgp import ( + "context" "log/slog" "github.com/containernetworking/cni/pkg/skel" "github.com/containernetworking/cni/pkg/types" type100 "github.com/containernetworking/cni/pkg/types/100" + + "go.datum.net/galactic/internal/nadpatch" ) -// cmdDel is a no-op, same as every other binary in the chain: the -// BGPVRFInstance/BGPAdvertisement CRDs and eBPF vrf_table entry this -// plugin's own ADD created are keyed by (vpc, vpcAttachment) and may still -// be in use by another pod/VM sharing the same attachment. Deleting them -// here would race with a concurrent ADD during restarts. Cleanup is left -// entirely to galactic-router's GC controller — see internal/cni's own -// cmdDel for the full reasoning, identical here. +// cmdDel deletes the per-pod EndpointSlice cmdAdd published, then falls +// through to the same no-op the rest of the chain's DEL paths follow for +// the BGPVRFInstance/BGPAdvertisement CRDs and eBPF vrf_table entry: those +// are keyed by (vpc, vpcAttachment) and may still be in use by another +// pod/VM sharing the same attachment, so deleting them here would race with +// a concurrent ADD during restarts — cleanup for those stays galactic- +// router's GC controller's job (see internal/cni's own cmdDel for the full +// reasoning, identical here). +// +// The EndpointSlice is a deliberate, correct divergence from that pattern: +// it's 1:1 with exactly one pod, never shared, so there's no "might belong +// to a live sibling" risk to avoid — see the #854 plan's Phase 5. Deletion +// is best-effort: any failure (including failing to build a k8s client at +// all) is logged and DEL still returns success, since a k8s API hiccup +// during pod teardown shouldn't block the pod from actually going away — +// Phase 8's ownerReference-to-Pod is the backstop for exactly this case. func cmdDel(args *skel.CmdArgs) error { // DEL is idempotent per the CNI spec: always return success, even if // parsing the config fails — logging vpc/vpcAttachment (when parseable) @@ -26,7 +38,7 @@ func cmdDel(args *skel.CmdArgs) error { // gate on it. pluginConf, parseErr := parseConf(args.StdinData) if parseErr != nil { - slog.Error("DEL: failed to parse CNI config, skipping cleanup logging", "err", parseErr, + slog.Error("DEL: failed to parse CNI config, skipping cleanup", "err", parseErr, "containerID", args.ContainerID) result := &type100.Result{} _ = types.PrintResult(result, "1.0.0") @@ -36,7 +48,36 @@ func cmdDel(args *skel.CmdArgs) error { slog.Info("DEL: skipping shared resource cleanup (handled by GC)", "containerID", args.ContainerID, "vpc", pluginConf.VPC, "vpcAttachment", pluginConf.VPCAttachment) + deleteEndpointSliceBestEffort(args, pluginConf.Namespace) + result := &type100.Result{} _ = types.PrintResult(result, pluginConf.CNIVersion) return nil } + +// deleteEndpointSliceBestEffort deletes this pod's EndpointSlice, logging +// (never failing DEL) on any error — see cmdDel's own doc comment for why. +func deleteEndpointSliceBestEffort(args *skel.CmdArgs, namespace string) { + podName := nadpatch.ParsePodName(args.Args) + if podName == "" { + slog.Debug("DEL: no K8S_POD_NAME in CNI_ARGS, nothing to delete", + "containerID", args.ContainerID, "cniArgs", args.Args) + return + } + + k8sClient, err := newK8sClient() + if err != nil { + slog.Error("DEL: failed to create k8s client, EndpointSlice cleanup deferred to GC", + "err", err, "containerID", args.ContainerID, "podName", podName) + return + } + + ctx, cancel := context.WithTimeout(context.Background(), cniTimeout) + defer cancel() + if err := deleteEndpointSlice(ctx, k8sClient, namespace, podName); err != nil { + slog.Error("DEL: failed to delete EndpointSlice, cleanup deferred to GC", + "err", err, "containerID", args.ContainerID, "podName", podName, "namespace", namespace) + return + } + slog.Info("DEL: EndpointSlice deleted", "containerID", args.ContainerID, "podName", podName, "namespace", namespace) +} diff --git a/internal/crdnames/crdnames.go b/internal/crdnames/crdnames.go index 49aadded..c5eb8f23 100644 --- a/internal/crdnames/crdnames.go +++ b/internal/crdnames/crdnames.go @@ -62,6 +62,26 @@ const AnnotationNoAddressing = "galactic.datum.net/no-addressing" // callers and tests share one spelling. const AnnotationNoAddressingValue = "true" +// AnnotationSID is the per-pod EndpointSlice annotation key holding the +// computed SRv6 uSID (see internal/plumbing/srv6.ComputeSID) that routes +// traffic to this pod's VRF — human-readable detail, matching the +// annotation-based pattern used elsewhere in this package. Not present when +// this node's BGPRouter has no SRv6Locator/nodeID configured (see +// registerEBPFDatapath's own skip case in internal/cnibgp/bgp.go). +const AnnotationSID = "galactic.datum.net/srv6-sid" + +// LabelTenantID is the per-pod EndpointSlice label carrying the same value +// as TenantIdentifier(vpc, vpcAttachment) — the discovery mechanism the HTTP +// ingress extension server watches/indexes on to find the EndpointSlices for +// a given VPC attachment. A label, not only an annotation, because +// annotations aren't selectable in a k8s List/Watch call. +const LabelTenantID = "galactic.datum.net/tenant-id" + +// AnnotationTenantID is the per-pod EndpointSlice annotation carrying the +// same TenantIdentifier(vpc, vpcAttachment) value as LabelTenantID — +// human-readable detail alongside the label that actually drives discovery. +const AnnotationTenantID = LabelTenantID + // containerIDLen is the number of characters used from a container ID in // annotation keys. Kubernetes limits the name part of an annotation key to // 63 bytes. The longest prefix sharing this constant is @@ -150,3 +170,29 @@ var vipNameReplacer = strings.NewReplacer(":", "-", ".", "-") func ServiceVIPBindingName(nodeName, vip string, port int32, proto string) string { return fmt.Sprintf("%s-%s-%s-%d", nodeName, vipNameReplacer.Replace(vip), proto, port) } + +// TenantIdentifier returns the deterministic value used to identify a VPC +// attachment across the EndpointSlice discovery surface (LabelTenantID/ +// AnnotationTenantID) — a plain (vpc, vpcAttachment) join, deliberately +// *not* run through nameSegment like BGPVRFInstanceName/BGPAdvertisementName +// are. Those two build a Kubernetes object *name*, which must be a +// lowercase RFC 1123 subdomain and so needs nameSegment's hex-safe encoding; +// this builds a label/annotation *value*, which permits uppercase and has +// no such constraint. Staying unencoded matters for a second reason: a +// consumer recovering the original vpc from this value only has to split on +// the first "-", which works because vpc/vpcAttachment are both base62 +// ([0-9a-zA-Z]) and therefore never contain that separator themselves — +// nameSegment's hex encoding would make that split unrecoverable back to +// the original vpc. +func TenantIdentifier(vpc, vpcAttachment string) string { + return fmt.Sprintf("%s-%s", vpc, vpcAttachment) +} + +// EndpointSliceName returns the deterministic name for the per-pod +// discoveryv1.EndpointSlice published by galactic-bgp — a trivial +// passthrough of the pod's own name (EndpointSlices are 1:1 with a pod, one +// per namespace), centralized here like every other name in this package so +// callers never spell the convention out themselves. +func EndpointSliceName(podName string) string { + return podName +} diff --git a/internal/crdnames/crdnames_test.go b/internal/crdnames/crdnames_test.go index 8d52c30e..76580d29 100644 --- a/internal/crdnames/crdnames_test.go +++ b/internal/crdnames/crdnames_test.go @@ -36,12 +36,21 @@ func TestServiceVIPBindingName(t *testing.T) { } } -// testVPCBase62 is base62 for 1234, padded as an interface name carries it. -const testVPCBase62 = "0000000jU" +// testVPC, testVPCBase62, and testAttachment are shared across this file's +// table-driven tests — the same (vpc, attachment)-shaped fixtures recur +// across BGPVRFInstanceName/BGPAdvertisementName/TenantIdentifier. Only the +// first two go through nameSegment's hex encoding (they end up in a +// Kubernetes object name, which must be a lowercase RFC 1123 subdomain); +// TenantIdentifier deliberately doesn't — see its own doc comment. +const ( + testVPC = "abc" + testVPCBase62 = "0000000jU" + testAttachment = "def" +) func TestBGPVRFInstanceName(t *testing.T) { tests := []struct{ vpc, nodeName, want string }{ - {"abc", "worker-1", "98de-worker-1"}, + {testVPC, "worker-1", "98de-worker-1"}, {testVPCBase62, "dfw-worker", "4d2-dfw-worker"}, } for _, tt := range tests { @@ -57,7 +66,7 @@ func TestBGPVRFInstanceName(t *testing.T) { // the identical BGPVRFInstance name — the whole point of keying this by // (vpc, node) instead of (vpc, vpcAttachment). func TestBGPVRFInstanceNameSharedAcrossAttachments(t *testing.T) { - const vpc, nodeName = "abc", "dfw-worker" + const vpc, nodeName = testVPC, "dfw-worker" first := BGPVRFInstanceName(vpc, nodeName) second := BGPVRFInstanceName(vpc, nodeName) if first != second { @@ -68,7 +77,7 @@ func TestBGPVRFInstanceNameSharedAcrossAttachments(t *testing.T) { func TestBGPAdvertisementName(t *testing.T) { tests := []struct{ vpc, attachment, want string }{ - {"abc", "def", "98de-c6a7"}, + {testVPC, testAttachment, "98de-c6a7"}, {testVPCBase62, "00G", "4d2-2a"}, } for _, tt := range tests { @@ -79,6 +88,46 @@ func TestBGPAdvertisementName(t *testing.T) { } } +func TestTenantIdentifier(t *testing.T) { + tests := []struct{ vpc, attachment, want string }{ + {testVPC, testAttachment, "abc-def"}, + {testVPCBase62, "00G", "0000000jU-00G"}, + } + for _, tt := range tests { + got := TenantIdentifier(tt.vpc, tt.attachment) + if got != tt.want { + t.Errorf("TenantIdentifier(%q, %q) = %q, want %q", tt.vpc, tt.attachment, got, tt.want) + } + } +} + +// TestTenantIdentifierDoesNotMatchBGPAdvertisementName documents a +// deliberate divergence: the two used to format a (vpc, attachment) pair +// identically, back when neither went through nameSegment's hex encoding. +// That's no longer true for BGPAdvertisementName (a Kubernetes object name, +// which must be a lowercase RFC 1123 subdomain), but TenantIdentifier (a +// label/annotation *value*, which permits uppercase) deliberately stays +// unencoded — see its own doc comment for why a plain string split +// recovering the original vpc depends on that. +func TestTenantIdentifierDoesNotMatchBGPAdvertisementName(t *testing.T) { + const vpc, attachment = testVPC, testAttachment + if got, other := TenantIdentifier(vpc, attachment), BGPAdvertisementName(vpc, attachment); got == other { + t.Errorf("TenantIdentifier(%q, %q) = %q unexpectedly matches BGPAdvertisementName() -- "+ + "if nameSegment's encoding changed to make these equal again, TenantIdentifier's "+ + "raw-value recoverability guarantee needs re-verifying, not just this test updating", + vpc, attachment, got) + } +} + +func TestEndpointSliceName(t *testing.T) { + tests := []string{"my-pod", "web-0", "vm-workload-abc123"} + for _, podName := range tests { + if got := EndpointSliceName(podName); got != podName { + t.Errorf("EndpointSliceName(%q) = %q, want %q", podName, got, podName) + } + } +} + // TestAnnotationKeyNameLength verifies that every annotation key builder // stays within Kubernetes' 63-byte limit on the "name" part of an // annotation key (the segment after the last "/"), using a realistic diff --git a/internal/nadpatch/nadpatch.go b/internal/nadpatch/nadpatch.go index a7332d76..e3c344c1 100644 --- a/internal/nadpatch/nadpatch.go +++ b/internal/nadpatch/nadpatch.go @@ -49,6 +49,20 @@ func ParsePodNamespace(cniArgs string) string { return "" } +// ParsePodName extracts the K8S_POD_NAME value from the CNI_ARGS +// environment variable string passed as args.Args by Multus. Returns an +// empty string when the value is not present (e.g. standalone CNI +// invocation), same convention as ParsePodNamespace. +func ParsePodName(cniArgs string) string { + for _, part := range strings.Split(cniArgs, ";") { + key, value, ok := strings.Cut(part, "=") + if ok && key == "K8S_POD_NAME" { + return value + } + } + return "" +} + // AnnotateNAD patches the NetworkAttachmentDefinition with the host // interface name. The NAD is expected to already exist (created by the // external VPC operator before the CNI is invoked), so a not-found response diff --git a/internal/nadpatch/nadpatch_test.go b/internal/nadpatch/nadpatch_test.go index e2a9cbc7..8ffab308 100644 --- a/internal/nadpatch/nadpatch_test.go +++ b/internal/nadpatch/nadpatch_test.go @@ -51,6 +51,37 @@ func TestParsePodNamespace(t *testing.T) { } } +func TestParsePodName(t *testing.T) { + tests := []struct { + name string + cniArgs string + expected string + }{ + {name: "empty string", cniArgs: "", expected: ""}, + {name: "name only", cniArgs: "K8S_POD_NAME=my-pod", expected: "my-pod"}, + { + name: "full multus args", + cniArgs: "K8S_POD_NAME=my-pod;K8S_POD_NAMESPACE=galactic-system;K8S_POD_INFRA_CONTAINER_ID=abc123", + expected: "my-pod", + }, + { + name: "name not present", + cniArgs: "K8S_POD_NAMESPACE=galactic-system;K8S_POD_INFRA_CONTAINER_ID=abc123", + expected: "", + }, + {name: "name with hyphens", cniArgs: "K8S_POD_NAME=my-custom-pod-0", expected: "my-custom-pod-0"}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + got := ParsePodName(tc.cniArgs) + if got != tc.expected { + t.Errorf("ParsePodName(%q) = %q, want %q", tc.cniArgs, got, tc.expected) + } + }) + } +} + func TestAnnotateNAD(t *testing.T) { const ( nadName = "test-net" From 96cfae1005b461a9e789438509023d160e280f4e Mon Sep 17 00:00:00 2001 From: Peter Sprygada Date: Wed, 19 Aug 2026 16:14:31 -0400 Subject: [PATCH 2/6] fix(cnibgp): publish/check/delete EndpointSlices in the pod's own namespace ops_add.go, ops_check.go, and ops_del.go were all passing pluginConf.Namespace as the EndpointSlice's namespace -- the namespace the BGP CRDs live in (defaults to galactic-system), not the workload pod's own namespace. That contradicts this feature's own design (docs/plans/854-vpc-http-ingress-endpointslice.md) and docs/cni/configuration.md, both of which say the EndpointSlice is published "in the pod's own namespace". In practice this meant publishEndpointSlice's Get of the owning Pod -- and every later Get/Delete of the EndpointSlice itself -- would look in the wrong namespace for any pod not deployed into galactic-system, i.e. almost every real workload, and fail outright. internal/nadpatch already parses K8S_POD_NAMESPACE out of CNI_ARGS (ParsePodNamespace) for exactly this purpose -- galactic-veth/-tap use it for NAD lookups -- but galactic-bgp's EndpointSlice path never called it. Fixed by parsing podNamespace from CNI_ARGS at all three call sites, mirroring the existing podName parsing, and threading it through to publishEndpointSlice/checkEndpointSlice/deleteEndpointSlice in place of pluginConf.Namespace. The BGPVRFInstance/BGPAdvertisement/ BGPRouter lookups elsewhere in the same functions are unchanged -- pluginConf.Namespace is exactly right for those. checkEndpointSlice's signature grew a podNamespace parameter; updated ops_check_test.go's call sites accordingly. This went undetected by both the unit tests (whose PluginConf fixtures set Namespace equal to the test pod's own namespace) and, until this PR's new e2e case, by tests/e2e (whose Kind cluster happens to default kubectl's namespace to galactic-system too -- see scripts/ci.sh). Co-Authored-By: Claude Sonnet 5 --- docs/cni/configuration.md | 7 +++++++ .../854-vpc-http-ingress-endpointslice.md | 2 ++ internal/cnibgp/ops_add.go | 13 ++++++++++--- internal/cnibgp/ops_check.go | 19 +++++++++++++------ internal/cnibgp/ops_check_test.go | 14 +++++++------- internal/cnibgp/ops_del.go | 18 +++++++++++------- 6 files changed, 50 insertions(+), 23 deletions(-) diff --git a/docs/cni/configuration.md b/docs/cni/configuration.md index bef9680a..b7ae1b6a 100644 --- a/docs/cni/configuration.md +++ b/docs/cni/configuration.md @@ -338,6 +338,13 @@ EndpointSlice carries: CHECK verifies the EndpointSlice still exists with the expected address and annotations. +Both the pod's name and its namespace are parsed from `K8S_POD_NAME`/ +`K8S_POD_NAMESPACE` in `CNI_ARGS` (`internal/nadpatch.ParsePodName`/ +`ParsePodNamespace`) — Multus always sets both for a real pod-scoped +invocation, but a standalone/manual invocation (e.g. one that skips the CNI +runtime, as some `tests/e2e` cases do) must set `CNI_ARGS` itself or ADD +fails outright and CHECK reports an error. + ## Example Configurations Every example below is a full conflist (a `NetworkAttachmentDefinition`'s diff --git a/docs/plans/854-vpc-http-ingress-endpointslice.md b/docs/plans/854-vpc-http-ingress-endpointslice.md index 20f6e57e..a712630a 100644 --- a/docs/plans/854-vpc-http-ingress-endpointslice.md +++ b/docs/plans/854-vpc-http-ingress-endpointslice.md @@ -5,6 +5,8 @@ - **Design doc:** [HTTP Ingress for VPC Networks](https://github.com/datum-cloud/enhancements/blob/main/enhancements/networking/http-ingress-for-vpc-networks.md) (PR [#851](https://github.com/datum-cloud/enhancements/pull/851), resolves [#853](https://github.com/datum-cloud/enhancements/issues/853)) - **Status:** implemented 2026-08-18, per this plan's sequencing (§4) — Phases 1–2 (`crdnames`/`nadpatch.ParsePodName`), 3 (SID computation), 4 (EndpointSlice publish on ADD + the `advertisementCreated` rollback-risk fix), 5 (DEL), 6 (CHECK), 7 (RBAC), 8 (ownerReference-to-Pod GC backstop, folded into Phase 4's publish step), 9 (docs), and unit tests (10) all landed together. See `internal/cnibgp/endpointslice.go`, the `internal/cnibgp/bgp.go`/`ops_add.go`/`ops_del.go`/`ops_check.go` changes, `internal/crdnames`/`internal/nadpatch`, and `config/galactic-cni/rbac.yaml`. Not yet exercised in `tests/e2e` (§3's suggested e2e case is still open). Previously revised 2026-08-13 after a review pass caught drift against the current repo (see the "Revision note" callouts throughout) — most of it stale paths, but two were real design gaps (Phases 4 and 8). Revised again 2026-08-17: Open Decision 5 resolved — VM/tap-attached workloads are in scope and are this issue's primary use case, not an implicitly-excluded edge case; Phase 4's nil-`ipamResult` skip already handles this correctly (it's an address-existence check, not a VM exclusion), confirmed against `internal/cnitap`/`internal/cnibgp`. Open Decision 4 (Phase 4's rollback-risk callout) also resolved — fix #1, narrowing `advertisementCreated` to create-only gating, confirmed safe and adopted. Open Decision 6 (Phase 8's GC mechanism) also resolved — the recommended `ownerReference`-to-Pod approach is adopted as-is; `internal/gc/gc.go`/`config/galactic-router/rbac.yaml` are untouched by this issue. +> **Revision note — CI fix 2026-08-19: EndpointSlice namespace-wiring bug.** Adding the suggested `tests/e2e` case (previous revision's "not yet exercised" callout) surfaced that `ops_add.go`/`ops_check.go`/`ops_del.go` were passing `pluginConf.Namespace` (where the BGP CRDs live, defaulting to `galactic-system`) as the EndpointSlice's namespace, instead of the pod's own namespace parsed from `CNI_ARGS` via `nadpatch.ParsePodNamespace` — contradicting this plan's own Phase 4 description and `docs/cni/configuration.md`'s "in the pod's own namespace." It went undetected because every existing unit test's fixtures set `pluginConf.Namespace` equal to the pod's test namespace, and the e2e cluster's own default `kubectl` namespace happens to be `galactic-system` too (`scripts/ci.sh`'s `kubectl config set-context --current --namespace=galactic-system`) — masking the mismatch everywhere it would otherwise have surfaced. Fixed by parsing `podNamespace` from `CNI_ARGS` at all three call sites (mirroring the existing `podName` parsing) and threading it through instead; `pluginConf.Namespace` is now used only for the BGP CRD/`BGPRouter` lookups it was always meant for. The e2e test's manually-chained `galactic-bgp` invocation (`tests/e2e/e2e_test.go`) also had to start setting `CNI_ARGS` itself (`K8S_POD_NAME`/`K8S_POD_NAMESPACE`), since it invokes the binary directly rather than through the CNI runtime/Multus, which sets these for every real pod-scoped invocation. + ## Correction to #854's framing Galactic-cni has already been split into a chained-plugin architecture. #854 was written before/around that split and refers to "galactic-cni" generically. All of this work belongs in **`internal/cnibgp`** (binary `galactic-bgp`), because it's the only plugin in the chain with all four required inputs simultaneously in scope: the pod's real allocated address (from `prevResult`), `vpc`/`vpcAttachment`, the SRv6 locator/nodeID (from `BGPRouter`), and the allocated VRFID/Argument. diff --git a/internal/cnibgp/ops_add.go b/internal/cnibgp/ops_add.go index 2c5909fe..14cf6976 100644 --- a/internal/cnibgp/ops_add.go +++ b/internal/cnibgp/ops_add.go @@ -95,12 +95,19 @@ func cmdAdd(args *skel.CmdArgs) (err error) { // EndpointSlice to carry either way. if ipamResult != nil && ipamResult.IPv6Subnet != nil { podName := nadpatch.ParsePodName(args.Args) - if podName == "" { - return fmt.Errorf("publish EndpointSlice: no K8S_POD_NAME in CNI_ARGS %q", args.Args) + // The EndpointSlice is created in the pod's own namespace, per + // docs/cni/configuration.md's "EndpointSlice publish" section — + // distinct from `namespace` above, which is where the BGP CRDs + // live (defaults to galactic-system) and is very often a + // different namespace from the workload pod's own. + podNamespace := nadpatch.ParsePodNamespace(args.Args) + if podName == "" || podNamespace == "" { + return fmt.Errorf( + "publish EndpointSlice: no K8S_POD_NAME/K8S_POD_NAMESPACE in CNI_ARGS %q", args.Args) } esCtx, esCancel := context.WithTimeout(context.Background(), cniTimeout) esErr := publishEndpointSlice( - esCtx, k8sClient, namespace, podName, pluginConf.VPC, pluginConf.VPCAttachment, + esCtx, k8sClient, podNamespace, podName, pluginConf.VPC, pluginConf.VPCAttachment, ipamResult.IPv6Subnet.IP, result.sid, ) esCancel() diff --git a/internal/cnibgp/ops_check.go b/internal/cnibgp/ops_check.go index b5526ee5..063cfbbe 100644 --- a/internal/cnibgp/ops_check.go +++ b/internal/cnibgp/ops_check.go @@ -78,10 +78,14 @@ func cmdCheck(args *skel.CmdArgs) error { errs = append(errs, fmt.Errorf("infer from prevResult: %w", prevErr)) } else if ipamResult != nil && ipamResult.IPv6Subnet != nil { podName := nadpatch.ParsePodName(args.Args) - if podName == "" { - errs = append(errs, errors.New("EndpointSlice: no K8S_POD_NAME in CNI_ARGS")) + // The EndpointSlice lives in the pod's own namespace (see ops_add.go's + // cmdAdd), not pluginConf.Namespace — that's only where the BGP CRDs + // checked above live. + podNamespace := nadpatch.ParsePodNamespace(args.Args) + if podName == "" || podNamespace == "" { + errs = append(errs, errors.New("EndpointSlice: no K8S_POD_NAME/K8S_POD_NAMESPACE in CNI_ARGS")) } else if err := checkEndpointSlice( - ctx, k8s, pluginConf, podName, ipamResult.IPv6Subnet.IP, vrfErr == nil, vrfInst.Spec.VRFID, + ctx, k8s, pluginConf, podName, podNamespace, ipamResult.IPv6Subnet.IP, vrfErr == nil, vrfInst.Spec.VRFID, ); err != nil { errs = append(errs, err) } @@ -191,14 +195,17 @@ func checkEBPFEntry(ctx context.Context, k8s client.Client, pluginConf *PluginCo // BGPVRFInstance lookup in cmdCheck above failed, in which case the SID // can't be recomputed and its annotation is not checked — matches // registerEBPFDatapath/checkEBPFEntry's own "can't check what we can't -// compute" convention. +// compute" convention. podNamespace is the pod's own namespace (parsed from +// CNI_ARGS) — where cmdAdd created the EndpointSlice — distinct from +// pluginConf.Namespace, which the BGPRouter lookup below still uses since +// that's where the BGP CRDs live. func checkEndpointSlice( - ctx context.Context, k8s client.Client, pluginConf *PluginConf, podName string, + ctx context.Context, k8s client.Client, pluginConf *PluginConf, podName, podNamespace string, addr net.IP, vrfIDKnown bool, vrfID int32, ) error { name := crdnames.EndpointSliceName(podName) slice := &discoveryv1.EndpointSlice{} - if err := k8s.Get(ctx, client.ObjectKey{Name: name, Namespace: pluginConf.Namespace}, slice); err != nil { + if err := k8s.Get(ctx, client.ObjectKey{Name: name, Namespace: podNamespace}, slice); err != nil { return fmt.Errorf("EndpointSlice %s: %w", name, err) } diff --git a/internal/cnibgp/ops_check_test.go b/internal/cnibgp/ops_check_test.go index 8d8b6f27..d7381b87 100644 --- a/internal/cnibgp/ops_check_test.go +++ b/internal/cnibgp/ops_check_test.go @@ -40,7 +40,7 @@ func TestCheckEndpointSlice(t *testing.T) { t.Run("missing EndpointSlice is an error", func(t *testing.T) { k8s := fakeClient() err := checkEndpointSlice( - context.Background(), k8s, testPluginConf(), testPodName, mustParseAddr(t), false, 0) + context.Background(), k8s, testPluginConf(), testPodName, testPodNamespace, mustParseAddr(t), false, 0) if err == nil { t.Fatal("checkEndpointSlice() = nil, want an error when the EndpointSlice does not exist") } @@ -60,7 +60,7 @@ func TestCheckEndpointSlice(t *testing.T) { } k8s := fakeClient(slice) err := checkEndpointSlice( - context.Background(), k8s, testPluginConf(), testPodName, mustParseAddr(t), false, 0) + context.Background(), k8s, testPluginConf(), testPodName, testPodNamespace, mustParseAddr(t), false, 0) if err == nil { t.Fatal("checkEndpointSlice() = nil, want an address-mismatch error") } @@ -83,7 +83,7 @@ func TestCheckEndpointSlice(t *testing.T) { } k8s := fakeClient(slice) err := checkEndpointSlice( - context.Background(), k8s, testPluginConf(), testPodName, mustParseAddr(t), false, 0) + context.Background(), k8s, testPluginConf(), testPodName, testPodNamespace, mustParseAddr(t), false, 0) if err == nil { t.Fatal("checkEndpointSlice() = nil, want a tenant-id mismatch error") } @@ -106,7 +106,7 @@ func TestCheckEndpointSlice(t *testing.T) { } k8s := fakeClient(slice) err := checkEndpointSlice( - context.Background(), k8s, testPluginConf(), testPodName, mustParseAddr(t), false, 0) + context.Background(), k8s, testPluginConf(), testPodName, testPodNamespace, mustParseAddr(t), false, 0) if err != nil { t.Fatalf("checkEndpointSlice() = %v, want nil", err) } @@ -137,7 +137,7 @@ func TestCheckEndpointSlice(t *testing.T) { k8s := fakeClient(router, slice) err := checkEndpointSlice( - context.Background(), k8s, testPluginConf(), testPodName, mustParseAddr(t), true, vrfID) + context.Background(), k8s, testPluginConf(), testPodName, testPodNamespace, mustParseAddr(t), true, vrfID) if err == nil { t.Fatal("checkEndpointSlice() = nil, want a SID-mismatch error") } @@ -176,7 +176,7 @@ func TestCheckEndpointSlice(t *testing.T) { k8s := fakeClient(router, slice) if err := checkEndpointSlice( - context.Background(), k8s, testPluginConf(), testPodName, mustParseAddr(t), true, vrfID, + context.Background(), k8s, testPluginConf(), testPodName, testPodNamespace, mustParseAddr(t), true, vrfID, ); err != nil { t.Fatalf("checkEndpointSlice() = %v, want nil", err) } @@ -200,7 +200,7 @@ func TestCheckEndpointSlice(t *testing.T) { k8s := fakeClient(router, slice) if err := checkEndpointSlice( - context.Background(), k8s, testPluginConf(), testPodName, mustParseAddr(t), true, vrfID, + context.Background(), k8s, testPluginConf(), testPodName, testPodNamespace, mustParseAddr(t), true, vrfID, ); err != nil { t.Fatalf("checkEndpointSlice() = %v, want nil", err) } diff --git a/internal/cnibgp/ops_del.go b/internal/cnibgp/ops_del.go index 5b25d9db..4a6cd0c8 100644 --- a/internal/cnibgp/ops_del.go +++ b/internal/cnibgp/ops_del.go @@ -48,7 +48,7 @@ func cmdDel(args *skel.CmdArgs) error { slog.Info("DEL: skipping shared resource cleanup (handled by GC)", "containerID", args.ContainerID, "vpc", pluginConf.VPC, "vpcAttachment", pluginConf.VPCAttachment) - deleteEndpointSliceBestEffort(args, pluginConf.Namespace) + deleteEndpointSliceBestEffort(args) result := &type100.Result{} _ = types.PrintResult(result, pluginConf.CNIVersion) @@ -57,10 +57,14 @@ func cmdDel(args *skel.CmdArgs) error { // deleteEndpointSliceBestEffort deletes this pod's EndpointSlice, logging // (never failing DEL) on any error — see cmdDel's own doc comment for why. -func deleteEndpointSliceBestEffort(args *skel.CmdArgs, namespace string) { +// The EndpointSlice lives in the pod's own namespace (parsed from CNI_ARGS +// below, same as podName), not pluginConf.Namespace — that's only where the +// BGP CRDs live, and cmdDel deliberately leaves those alone (see above). +func deleteEndpointSliceBestEffort(args *skel.CmdArgs) { podName := nadpatch.ParsePodName(args.Args) - if podName == "" { - slog.Debug("DEL: no K8S_POD_NAME in CNI_ARGS, nothing to delete", + podNamespace := nadpatch.ParsePodNamespace(args.Args) + if podName == "" || podNamespace == "" { + slog.Debug("DEL: no K8S_POD_NAME/K8S_POD_NAMESPACE in CNI_ARGS, nothing to delete", "containerID", args.ContainerID, "cniArgs", args.Args) return } @@ -74,10 +78,10 @@ func deleteEndpointSliceBestEffort(args *skel.CmdArgs, namespace string) { ctx, cancel := context.WithTimeout(context.Background(), cniTimeout) defer cancel() - if err := deleteEndpointSlice(ctx, k8sClient, namespace, podName); err != nil { + if err := deleteEndpointSlice(ctx, k8sClient, podNamespace, podName); err != nil { slog.Error("DEL: failed to delete EndpointSlice, cleanup deferred to GC", - "err", err, "containerID", args.ContainerID, "podName", podName, "namespace", namespace) + "err", err, "containerID", args.ContainerID, "podName", podName, "namespace", podNamespace) return } - slog.Info("DEL: EndpointSlice deleted", "containerID", args.ContainerID, "podName", podName, "namespace", namespace) + slog.Info("DEL: EndpointSlice deleted", "containerID", args.ContainerID, "podName", podName, "namespace", podNamespace) } From 20d52d7fcf12cfd9111c526c67ef688c74601ce7 Mon Sep 17 00:00:00 2001 From: Peter Sprygada Date: Wed, 19 Aug 2026 16:14:40 -0400 Subject: [PATCH 3/6] test(e2e): set CNI_ARGS on the manually-chained galactic-bgp invocation TestCNITapInterface's testChainedGalacticBGP invokes galactic-bgp directly (not through the CNI runtime/Multus), so it never set CNI_ARGS. That was harmless before this PR, but ops_add.go's new EndpointSlice publish step requires K8S_POD_NAME/K8S_POD_NAMESPACE in CNI_ARGS and hard-fails ADD without them: galactic-bgp ADD failed: exit status 1 output: {"code":999,"msg":"publish EndpointSlice: no K8S_POD_NAME in CNI_ARGS \"\""} Fixed by setting CNI_ARGS="K8S_POD_NAME=...;K8S_POD_NAMESPACE=..." on the chained invocation, matching what Multus always sets for a real pod-scoped CNI call. The namespace is asked of the API (podNamespaceOf, new helper) rather than assumed, since this suite never passes --namespace to kubectl and instead relies on whatever the ambient context defaults to. Co-Authored-By: Claude Sonnet 5 --- tests/e2e/e2e_test.go | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/tests/e2e/e2e_test.go b/tests/e2e/e2e_test.go index 1b908df8..9d0e99dd 100644 --- a/tests/e2e/e2e_test.go +++ b/tests/e2e/e2e_test.go @@ -388,6 +388,15 @@ func testChainedGalacticBGP(t *testing.T, podName string, tapResult map[string]a "prevResult": %s }`, vpc, vpcAttachment, prevResultJSON) + // galactic-bgp's EndpointSlice publish (ops_add.go) Gets the owning Pod + // by name+namespace out of K8S_POD_NAME/K8S_POD_NAMESPACE in CNI_ARGS — + // exactly what a real kubelet-driven invocation always sets for every + // pod-scoped CNI call, via Multus. This chained-plugin test invokes the + // binary directly rather than through the CNI runtime, so it has to set + // CNI_ARGS itself; podName is this test's own workload pod (created + // above), so its namespace is asked of the API rather than assumed. + podNamespace := podNamespaceOf(t, podName) + // Reuses the same netns/containerID/ifname galactic-tap's own step // (above) already set up: a real chained plugin sees the identical // values across every plugin invoked for one CNI ADD. @@ -397,6 +406,7 @@ CNI_COMMAND=$1 \ CNI_CONTAINERID=e2e-tap-001 \ CNI_IFNAME=eth0 \ CNI_PATH=/ \ +CNI_ARGS="K8S_POD_NAME=` + podName + `;K8S_POD_NAMESPACE=` + podNamespace + `" \ NODE_NAME=` + nodeName() + ` \ /galactic-bgp < /tmp/cni-bgp.json ` @@ -482,6 +492,20 @@ func nodeName() string { return "kind-worker" } +// podNamespaceOf returns the namespace of an already-created pod, by +// asking the API rather than assuming it — this suite never passes +// --namespace to kubectl, so the pod actually landed in whatever namespace +// the ambient kubectl context defaults to (scripts/ci.sh points that at +// galactic-system, but nothing here should hard-code that). +func podNamespaceOf(t *testing.T, podName string) string { + t.Helper() + out, err := kubectl(t.Context(), "get", "pod", podName, "-o", "jsonpath={.metadata.namespace}") + if err != nil { + t.Fatalf("get namespace of pod %s: %v\n%s", podName, err, out) + } + return out +} + // kubectl runs kubectl with the given arguments and returns combined output. func kubectl(ctx context.Context, args ...string) (string, error) { out, err := exec.CommandContext(ctx, "kubectl", args...).CombinedOutput() From ba402a0e8097478ac8f7530ee6e953ca3e07dc50 Mon Sep 17 00:00:00 2001 From: Peter Sprygada Date: Thu, 20 Aug 2026 12:51:08 -0400 Subject: [PATCH 4/6] fix(cnibgp): hoist redundant BGPRouter lookup in cmdCheck MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit checkEBPFEntry and checkEndpointSlice's SID check each called lookupBGPRouter independently, keyed on the same cniConfig.NodeName/pluginConf.Namespace pair, during the same cmdCheck invocation — a redundant API round-trip on every CHECK. Look it up once in cmdCheck and hand the result to both, dropping checkEndpointSlice's vrfIDKnown bool in favor of a zero-value bgpConfig (matching the existing SRv6-not-configured convention) and dropping checkEBPFEntry's now-unused k8s/ctx parameters. Addresses review feedback from @0xmc on PR #423. Co-Authored-By: Claude Sonnet 5 --- internal/cnibgp/ops_check.go | 57 +++++++++++++++++-------------- internal/cnibgp/ops_check_test.go | 24 +++++++------ 2 files changed, 45 insertions(+), 36 deletions(-) diff --git a/internal/cnibgp/ops_check.go b/internal/cnibgp/ops_check.go index 063cfbbe..538fcb55 100644 --- a/internal/cnibgp/ops_check.go +++ b/internal/cnibgp/ops_check.go @@ -64,6 +64,19 @@ func cmdCheck(args *skel.CmdArgs) error { errs = append(errs, fmt.Errorf("BGPVRFInstance %s: %w", vrfName, vrfErr)) } + // checkEndpointSlice's SID check and checkEBPFEntry below both need + // this node's BGPRouter (keyed by cniConfig.NodeName/pluginConf.Namespace) + // once the BGPVRFInstance lookup above succeeded; look it up here once + // and hand the result to both instead of each fetching it independently. + var bgp bgpConfig + if vrfErr == nil { + var bgpErr error + bgp, bgpErr = lookupBGPRouter(ctx, k8s, cniConfig.NodeName, pluginConf.Namespace) + if bgpErr != nil { + errs = append(errs, fmt.Errorf("look up BGPRouter: %w", bgpErr)) + } + } + adv := &bgpv1alpha1.BGPAdvertisement{} advName := crdnames.BGPAdvertisementName(pluginConf.VPC, pluginConf.VPCAttachment) if err := k8s.Get(ctx, client.ObjectKey{Name: advName, Namespace: pluginConf.Namespace}, adv); err != nil { @@ -85,7 +98,7 @@ func cmdCheck(args *skel.CmdArgs) error { if podName == "" || podNamespace == "" { errs = append(errs, errors.New("EndpointSlice: no K8S_POD_NAME/K8S_POD_NAMESPACE in CNI_ARGS")) } else if err := checkEndpointSlice( - ctx, k8s, pluginConf, podName, podNamespace, ipamResult.IPv6Subnet.IP, vrfErr == nil, vrfInst.Spec.VRFID, + ctx, k8s, pluginConf, podName, podNamespace, ipamResult.IPv6Subnet.IP, bgp, vrfInst.Spec.VRFID, ); err != nil { errs = append(errs, err) } @@ -96,7 +109,7 @@ func cmdCheck(args *skel.CmdArgs) error { // on) and this node's router actually has SRv6 configured — matches // registerEBPFDatapath's own no-op case. if vrfErr == nil { - if err := checkEBPFEntry(ctx, k8s, pluginConf, uint16(vrfInst.Spec.VRFID)); err != nil { + if err := checkEBPFEntry(pluginConf, uint16(vrfInst.Spec.VRFID), bgp); err != nil { errs = append(errs, err) } } @@ -122,12 +135,10 @@ func cmdCheck(args *skel.CmdArgs) error { // still reporting the attachment healthy. Returns nil (not an error) when // this node's router has no SRv6Locator/nodeID configured — SRv6 was // intentionally never set up for this attachment, matching -// registerEBPFDatapath's own no-op case. -func checkEBPFEntry(ctx context.Context, k8s client.Client, pluginConf *PluginConf, argument uint16) error { - bgp, err := lookupBGPRouter(ctx, k8s, cniConfig.NodeName, pluginConf.Namespace) - if err != nil { - return fmt.Errorf("look up BGPRouter: %w", err) - } +// registerEBPFDatapath's own no-op case. bgp is this node's BGPRouter, +// looked up once by the caller (cmdCheck) and shared with checkEndpointSlice +// rather than each fetching it independently. +func checkEBPFEntry(pluginConf *PluginConf, argument uint16, bgp bgpConfig) error { if bgp.srv6Locator == "" || bgp.nodeID == 0 { return nil } @@ -191,17 +202,18 @@ func checkEBPFEntry(ctx context.Context, k8s client.Client, pluginConf *PluginCo // checkEndpointSlice verifies the per-pod EndpointSlice cmdAdd published // (endpointslice.go) is still in place: it exists, carries the pod's // current address, and its tenant-id/SID label and annotations match -// freshly recomputed expected values. vrfIDKnown is false when the -// BGPVRFInstance lookup in cmdCheck above failed, in which case the SID -// can't be recomputed and its annotation is not checked — matches +// freshly recomputed expected values. bgp is this node's BGPRouter, looked +// up once by the caller (cmdCheck) and shared with checkEBPFEntry rather +// than fetched here independently; a zero-value bgp (as when the +// BGPVRFInstance lookup in cmdCheck above failed) means the SID can't be +// recomputed, so its annotation is not checked — matches // registerEBPFDatapath/checkEBPFEntry's own "can't check what we can't // compute" convention. podNamespace is the pod's own namespace (parsed from // CNI_ARGS) — where cmdAdd created the EndpointSlice — distinct from -// pluginConf.Namespace, which the BGPRouter lookup below still uses since -// that's where the BGP CRDs live. +// pluginConf.Namespace, which is only where the BGP CRDs live. func checkEndpointSlice( ctx context.Context, k8s client.Client, pluginConf *PluginConf, podName, podNamespace string, - addr net.IP, vrfIDKnown bool, vrfID int32, + addr net.IP, bgp bgpConfig, vrfID int32, ) error { name := crdnames.EndpointSliceName(podName) slice := &discoveryv1.EndpointSlice{} @@ -230,18 +242,13 @@ func checkEndpointSlice( "EndpointSlice %s annotation %s = %q, want %q", name, crdnames.AnnotationTenantID, got, wantTenantID)) } - if vrfIDKnown { - bgp, err := lookupBGPRouter(ctx, k8s, cniConfig.NodeName, pluginConf.Namespace) + if bgp.srv6Locator != "" && bgp.nodeID != 0 { + sid, err := srv6.ComputeSID(bgp.srv6Locator, bgp.nodeID, vrfID, bgpv1alpha1.SRv6FunctionEndDT46) if err != nil { - errs = append(errs, fmt.Errorf("look up BGPRouter for EndpointSlice SID check: %w", err)) - } else if bgp.srv6Locator != "" && bgp.nodeID != 0 { - sid, err := srv6.ComputeSID(bgp.srv6Locator, bgp.nodeID, vrfID, bgpv1alpha1.SRv6FunctionEndDT46) - if err != nil { - errs = append(errs, fmt.Errorf("compute expected SRv6 uSID for EndpointSlice check: %w", err)) - } else if got := slice.Annotations[crdnames.AnnotationSID]; got != sid.String() { - errs = append(errs, fmt.Errorf( - "EndpointSlice %s annotation %s = %q, want %q", name, crdnames.AnnotationSID, got, sid.String())) - } + errs = append(errs, fmt.Errorf("compute expected SRv6 uSID for EndpointSlice check: %w", err)) + } else if got := slice.Annotations[crdnames.AnnotationSID]; got != sid.String() { + errs = append(errs, fmt.Errorf( + "EndpointSlice %s annotation %s = %q, want %q", name, crdnames.AnnotationSID, got, sid.String())) } } diff --git a/internal/cnibgp/ops_check_test.go b/internal/cnibgp/ops_check_test.go index d7381b87..8517073f 100644 --- a/internal/cnibgp/ops_check_test.go +++ b/internal/cnibgp/ops_check_test.go @@ -40,7 +40,7 @@ func TestCheckEndpointSlice(t *testing.T) { t.Run("missing EndpointSlice is an error", func(t *testing.T) { k8s := fakeClient() err := checkEndpointSlice( - context.Background(), k8s, testPluginConf(), testPodName, testPodNamespace, mustParseAddr(t), false, 0) + context.Background(), k8s, testPluginConf(), testPodName, testPodNamespace, mustParseAddr(t), bgpConfig{}, 0) if err == nil { t.Fatal("checkEndpointSlice() = nil, want an error when the EndpointSlice does not exist") } @@ -60,7 +60,7 @@ func TestCheckEndpointSlice(t *testing.T) { } k8s := fakeClient(slice) err := checkEndpointSlice( - context.Background(), k8s, testPluginConf(), testPodName, testPodNamespace, mustParseAddr(t), false, 0) + context.Background(), k8s, testPluginConf(), testPodName, testPodNamespace, mustParseAddr(t), bgpConfig{}, 0) if err == nil { t.Fatal("checkEndpointSlice() = nil, want an address-mismatch error") } @@ -83,7 +83,7 @@ func TestCheckEndpointSlice(t *testing.T) { } k8s := fakeClient(slice) err := checkEndpointSlice( - context.Background(), k8s, testPluginConf(), testPodName, testPodNamespace, mustParseAddr(t), false, 0) + context.Background(), k8s, testPluginConf(), testPodName, testPodNamespace, mustParseAddr(t), bgpConfig{}, 0) if err == nil { t.Fatal("checkEndpointSlice() = nil, want a tenant-id mismatch error") } @@ -92,7 +92,7 @@ func TestCheckEndpointSlice(t *testing.T) { } }) - t.Run("everything matches, vrfIDKnown false: SID not checked", func(t *testing.T) { + t.Run("everything matches, bgp not known: SID not checked", func(t *testing.T) { slice := &discoveryv1.EndpointSlice{ ObjectMeta: metav1.ObjectMeta{ Name: testPodName, @@ -106,13 +106,13 @@ func TestCheckEndpointSlice(t *testing.T) { } k8s := fakeClient(slice) err := checkEndpointSlice( - context.Background(), k8s, testPluginConf(), testPodName, testPodNamespace, mustParseAddr(t), false, 0) + context.Background(), k8s, testPluginConf(), testPodName, testPodNamespace, mustParseAddr(t), bgpConfig{}, 0) if err != nil { t.Fatalf("checkEndpointSlice() = %v, want nil", err) } }) - t.Run("vrfIDKnown true, SRv6 configured: SID mismatch is an error", func(t *testing.T) { + t.Run("bgp known, SRv6 configured: SID mismatch is an error", func(t *testing.T) { const ( locator = "fd00:10::/48" srvNode = 7 @@ -137,7 +137,8 @@ func TestCheckEndpointSlice(t *testing.T) { k8s := fakeClient(router, slice) err := checkEndpointSlice( - context.Background(), k8s, testPluginConf(), testPodName, testPodNamespace, mustParseAddr(t), true, vrfID) + context.Background(), k8s, testPluginConf(), testPodName, testPodNamespace, mustParseAddr(t), + bgpConfig{srv6Locator: locator, nodeID: srvNode}, vrfID) if err == nil { t.Fatal("checkEndpointSlice() = nil, want a SID-mismatch error") } @@ -146,7 +147,7 @@ func TestCheckEndpointSlice(t *testing.T) { } }) - t.Run("vrfIDKnown true, SRv6 configured: matching SID passes", func(t *testing.T) { + t.Run("bgp known, SRv6 configured: matching SID passes", func(t *testing.T) { const ( locator = "fd00:10::/48" srvNode = 7 @@ -176,13 +177,14 @@ func TestCheckEndpointSlice(t *testing.T) { k8s := fakeClient(router, slice) if err := checkEndpointSlice( - context.Background(), k8s, testPluginConf(), testPodName, testPodNamespace, mustParseAddr(t), true, vrfID, + context.Background(), k8s, testPluginConf(), testPodName, testPodNamespace, mustParseAddr(t), + bgpConfig{srv6Locator: locator, nodeID: srvNode}, vrfID, ); err != nil { t.Fatalf("checkEndpointSlice() = %v, want nil", err) } }) - t.Run("vrfIDKnown true, SRv6 not configured: SID not checked", func(t *testing.T) { + t.Run("bgp known, SRv6 not configured: SID not checked", func(t *testing.T) { const vrfID = int32(1234) router := routerForNode(testRouterName, nodeName, testPodNamespace, 65000) @@ -200,7 +202,7 @@ func TestCheckEndpointSlice(t *testing.T) { k8s := fakeClient(router, slice) if err := checkEndpointSlice( - context.Background(), k8s, testPluginConf(), testPodName, testPodNamespace, mustParseAddr(t), true, vrfID, + context.Background(), k8s, testPluginConf(), testPodName, testPodNamespace, mustParseAddr(t), bgpConfig{}, vrfID, ); err != nil { t.Fatalf("checkEndpointSlice() = %v, want nil", err) } From 651dd662d122281598e6df1f83ba4ad83eb95c64 Mon Sep 17 00:00:00 2001 From: Peter Sprygada Date: Thu, 20 Aug 2026 13:14:22 -0400 Subject: [PATCH 5/6] test(cnibgp): create the host veth pair withTempPinDir's callers need registerEBPFDatapath's hostInterfaceIndex/attachUsidEgress calls (added by the tc-bpf egress routing work now on main) resolve the attachment's host-side interface by name, so TestPublishBGPStateComputesSIDWhenSRv6Configured started failing CI's root-gated unit test job after rebasing onto main: publishBGPState: unexpected error: register eBPF uSID datapath: resolve host interface ifindex for eBPF registration: look up host interface "G000000abcdefH": Link not found withTempPinDir already set up the other two preconditions bgp_ebpf_test.go's own requireRoot-gated tests rely on (the pinned eBPF maps and this VPC's kernel VRF); add the third, a real veth.Add'd host interface for testVPC/testAttachment, matching what those tests already do directly. Co-Authored-By: Claude Sonnet 5 --- internal/cnibgp/bgp_test.go | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/internal/cnibgp/bgp_test.go b/internal/cnibgp/bgp_test.go index d849a081..76163db5 100644 --- a/internal/cnibgp/bgp_test.go +++ b/internal/cnibgp/bgp_test.go @@ -26,6 +26,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" + "go.datum.net/galactic/internal/cni/veth" "go.datum.net/galactic/internal/cniipam" "go.datum.net/galactic/internal/crdnames" "go.datum.net/galactic/internal/plumbing/ebpf/attach" @@ -449,11 +450,13 @@ func withNetNSExistsFn(t *testing.T, fn func(string) bool) { // withTempPinDir points the package-level ebpfPinDir var (see cnibgp.go's // doc comment) at a throwaway bpffs directory for the duration of the test, // loading the eBPF datapath's pinned maps into it first (simulating the run -// container having already loaded the datapath) and adding this VPC's real -// kernel VRF — the same two preconditions bgp_ebpf_test.go's own -// requireRoot-gated tests set up before calling registerEBPFDatapath -// directly. Restores ebpfPinDir and tears both down on cleanup. Callers -// must call requireRoot(t) first. +// container having already loaded the datapath), adding this VPC's real +// kernel VRF, and creating the testVPC/testAttachment host veth pair — +// the same three preconditions bgp_ebpf_test.go's own requireRoot-gated +// tests set up before calling registerEBPFDatapath directly; the veth pair +// is what registerEBPFDatapath's own hostInterfaceIndex/attachUsidEgress +// calls resolve by name. Restores ebpfPinDir and tears all three down on +// cleanup. Callers must call requireRoot(t) first. func withTempPinDir(t *testing.T) { t.Helper() @@ -462,6 +465,11 @@ func withTempPinDir(t *testing.T) { } t.Cleanup(func() { _ = vrf.Delete(testVPC) }) + if err := veth.Add(testVPC, testAttachment, 1500); err != nil { + t.Fatalf("veth.Add: %v", err) + } + t.Cleanup(func() { _ = veth.Delete(testVPC, testAttachment) }) + pinDir := fmt.Sprintf("/sys/fs/bpf/galactic-bgp-test-%d", os.Getpid()) t.Cleanup(func() { _ = os.RemoveAll(pinDir) }) loaderObjs, err := attach.Load(pinDir) From 0c6fafa90343cbf63e2b1a1fe6c30a03a7743b63 Mon Sep 17 00:00:00 2001 From: Peter Sprygada Date: Thu, 20 Aug 2026 21:42:03 -0400 Subject: [PATCH 6/6] fix(crdnames): dedupe goconst-flagged "00G" literal in tests Rebasing this branch onto main picked up #437's base62-to-hex CRD-name encoding, which required updating TestBGPAdvertisementName/ TestTenantIdentifier's expected values in the same conflict resolution. That reintroduced the literal "00G" a third time across this file, tripping golangci-lint's goconst check. Add testAttachmentBase62 alongside the existing testVPC/testVPCBase62/testAttachment fixture constants and use it at both sites. Co-Authored-By: Claude Sonnet 5 --- internal/crdnames/crdnames_test.go | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/internal/crdnames/crdnames_test.go b/internal/crdnames/crdnames_test.go index 76580d29..17c58d67 100644 --- a/internal/crdnames/crdnames_test.go +++ b/internal/crdnames/crdnames_test.go @@ -43,9 +43,10 @@ func TestServiceVIPBindingName(t *testing.T) { // Kubernetes object name, which must be a lowercase RFC 1123 subdomain); // TenantIdentifier deliberately doesn't — see its own doc comment. const ( - testVPC = "abc" - testVPCBase62 = "0000000jU" - testAttachment = "def" + testVPC = "abc" + testVPCBase62 = "0000000jU" + testAttachment = "def" + testAttachmentBase62 = "00G" ) func TestBGPVRFInstanceName(t *testing.T) { @@ -78,7 +79,7 @@ func TestBGPVRFInstanceNameSharedAcrossAttachments(t *testing.T) { func TestBGPAdvertisementName(t *testing.T) { tests := []struct{ vpc, attachment, want string }{ {testVPC, testAttachment, "98de-c6a7"}, - {testVPCBase62, "00G", "4d2-2a"}, + {testVPCBase62, testAttachmentBase62, "4d2-2a"}, } for _, tt := range tests { got := BGPAdvertisementName(tt.vpc, tt.attachment) @@ -91,7 +92,7 @@ func TestBGPAdvertisementName(t *testing.T) { func TestTenantIdentifier(t *testing.T) { tests := []struct{ vpc, attachment, want string }{ {testVPC, testAttachment, "abc-def"}, - {testVPCBase62, "00G", "0000000jU-00G"}, + {testVPCBase62, testAttachmentBase62, "0000000jU-00G"}, } for _, tt := range tests { got := TenantIdentifier(tt.vpc, tt.attachment)