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..b7ae1b6a 100644 --- a/docs/cni/configuration.md +++ b/docs/cni/configuration.md @@ -304,6 +304,47 @@ 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. + +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 82266849..a712630a 100644 --- a/docs/plans/854-vpc-http-ingress-endpointslice.md +++ b/docs/plans/854-vpc-http-ingress-endpointslice.md @@ -3,7 +3,9 @@ - **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. + +> **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 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..76163db5 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" @@ -25,10 +26,14 @@ 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" "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 +447,42 @@ 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), 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() + + if err := vrf.Add(testVPC); err != nil { + t.Fatalf("vrf.Add: %v", err) + } + 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) + 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 +673,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..14cf6976 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,37 @@ 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) + // 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, podNamespace, 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..538fcb55 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" ) @@ -60,18 +64,52 @@ 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 { 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) + // 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, podNamespace, ipamResult.IPv6Subnet.IP, bgp, 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 // 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) } } @@ -97,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 } @@ -163,6 +199,62 @@ 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. 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 is only where the BGP CRDs live. +func checkEndpointSlice( + ctx context.Context, k8s client.Client, pluginConf *PluginConf, podName, podNamespace string, + addr net.IP, bgp bgpConfig, vrfID int32, +) error { + name := crdnames.EndpointSliceName(podName) + slice := &discoveryv1.EndpointSlice{} + if err := k8s.Get(ctx, client.ObjectKey{Name: name, Namespace: podNamespace}, 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 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..8517073f --- /dev/null +++ b/internal/cnibgp/ops_check_test.go @@ -0,0 +1,210 @@ +// 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, testPodNamespace, mustParseAddr(t), bgpConfig{}, 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, testPodNamespace, mustParseAddr(t), bgpConfig{}, 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, testPodNamespace, mustParseAddr(t), bgpConfig{}, 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, bgp not known: 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, testPodNamespace, mustParseAddr(t), bgpConfig{}, 0) + if err != nil { + t.Fatalf("checkEndpointSlice() = %v, want nil", err) + } + }) + + t.Run("bgp known, 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, testPodNamespace, mustParseAddr(t), + bgpConfig{srv6Locator: locator, nodeID: srvNode}, 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("bgp known, 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, testPodNamespace, mustParseAddr(t), + bgpConfig{srv6Locator: locator, nodeID: srvNode}, vrfID, + ); err != nil { + t.Fatalf("checkEndpointSlice() = %v, want nil", err) + } + }) + + t.Run("bgp known, 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, testPodNamespace, mustParseAddr(t), bgpConfig{}, 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..4a6cd0c8 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,40 @@ 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) + 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. +// 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) + 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 + } + + 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, podNamespace, podName); err != nil { + slog.Error("DEL: failed to delete EndpointSlice, cleanup deferred to GC", + "err", err, "containerID", args.ContainerID, "podName", podName, "namespace", podNamespace) + return + } + slog.Info("DEL: EndpointSlice deleted", "containerID", args.ContainerID, "podName", podName, "namespace", podNamespace) +} 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..17c58d67 100644 --- a/internal/crdnames/crdnames_test.go +++ b/internal/crdnames/crdnames_test.go @@ -36,12 +36,22 @@ 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" + testAttachmentBase62 = "00G" +) 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 +67,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,8 +78,8 @@ func TestBGPVRFInstanceNameSharedAcrossAttachments(t *testing.T) { func TestBGPAdvertisementName(t *testing.T) { tests := []struct{ vpc, attachment, want string }{ - {"abc", "def", "98de-c6a7"}, - {testVPCBase62, "00G", "4d2-2a"}, + {testVPC, testAttachment, "98de-c6a7"}, + {testVPCBase62, testAttachmentBase62, "4d2-2a"}, } for _, tt := range tests { got := BGPAdvertisementName(tt.vpc, tt.attachment) @@ -79,6 +89,46 @@ func TestBGPAdvertisementName(t *testing.T) { } } +func TestTenantIdentifier(t *testing.T) { + tests := []struct{ vpc, attachment, want string }{ + {testVPC, testAttachment, "abc-def"}, + {testVPCBase62, testAttachmentBase62, "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" 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()