From 0c09ed63fe080b6cad9700e64203091027d3f392 Mon Sep 17 00:00:00 2001 From: Peter Sprygada Date: Thu, 13 Aug 2026 12:50:17 -0400 Subject: [PATCH 1/4] docs: Revise 854 EndpointSlice plan after review A review pass against the current repo found the plan's file paths and CNI chain topology were stale after a refactor, and surfaced two real design gaps: wiring EndpointSlice publish as a step after publishBGPState can trigger the existing rollback path to delete a still-live, shared BGPAdvertisement, and the proposed GC extension assumed a per-pod liveness signal that doesn't exist in the current annotation scheme. Revise the plan to fix the stale references, flag the rollback risk with three candidate fixes, and replace the GC extension with a recommendation to use a Kubernetes ownerReference to the owning Pod as the primary cleanup mechanism, keeping the explicit DEL as the fast path. Add two new open decisions for VM/tap-workload scope and the GC mechanism choice. Related to datum-cloud/enhancements#854 Co-Authored-By: Claude Sonnet 5 --- .../854-vpc-http-ingress-endpointslice.md | 99 ++++++++++++------- 1 file changed, 66 insertions(+), 33 deletions(-) diff --git a/docs/plans/854-vpc-http-ingress-endpointslice.md b/docs/plans/854-vpc-http-ingress-endpointslice.md index 6f9b8646..b7973e5b 100644 --- a/docs/plans/854-vpc-http-ingress-endpointslice.md +++ b/docs/plans/854-vpc-http-ingress-endpointslice.md @@ -3,11 +3,13 @@ - **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 +- **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. ## Correction to #854's framing -Galactic-cni has already been split into a chained-plugin architecture (commits `c1704f7`/`e33092c`/`725ef87`). `internal/cni` (the master plugin) no longer talks to Kubernetes for BGP state at all — that moved to `internal/cnibgp`, the **last plugin in the chain** (`galactic-cni` → `galactic-ipam` → `galactic-route` → `galactic-bgp`). #854 was written before/around that split and refers to "galactic-cni" generically. All of this work belongs in **`internal/cnibgp`**, 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. +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. + +> **Revision note:** the chain topology has moved on further since this framing was first written. It is no longer `galactic-cni → galactic-ipam → galactic-route → galactic-bgp`. Current shape: a **master plugin** creates the VRF and host-side interface — `galactic-veth` for containers or `galactic-tap` for VM workloads — IPAM is **delegated** (not chained) to `galactic-ipam` via the standard CNI IPAM protocol, an optional `galactic-route` installs termination routes, and `galactic-bgp` runs last. `galactic-cni` itself is now purely a host installer/stager (`init`/`run` DaemonSet containers) and is never itself a CNI plugin — no NAD ever names it in a `"type"` field. This matters here because `galactic-bgp` is chain-invoked after **either** master plugin, so this work needs an explicit answer for the `galactic-tap`/VM path — see Open Decision 5. ## 1. Scope recap (resolved state of #854) @@ -21,78 +23,109 @@ Galactic-cni has already been split into a chained-plugin architecture (commits | Concern | Binary | Package | Existing anchor | |---|---|---|---| -| SID computation | `galactic-bgp` | `internal/cnibgp` | `publishBGPState`, `bgp.go:318-408` | -| EndpointSlice publish (ADD) | `galactic-bgp` | `internal/cnibgp` (new file) | after `bgp.go:373-398` (BGPAdvertisement CreateOrUpdate) | -| EndpointSlice delete (DEL) | `galactic-bgp` | `internal/cnibgp/ops_del.go` | currently a 6-line no-op (`ops_del.go:22-28`) | -| EndpointSlice validate (CHECK) | `galactic-bgp` | `internal/cnibgp/ops_check.go` | after BGPAdvertisement Get (`ops_check.go:64-68`) | -| Naming/annotation vocabulary | both | `internal/cni/crdnames` | already the shared cross-plugin vocab package | -| Pod-name parsing | `galactic-bgp` (new dependency) | `internal/cni/nadpatch` | sibling to existing `ParsePodNamespace` (`nadpatch.go:40-48`) | -| RBAC (publish/delete) | `galactic-bgp`'s SA | `config/galactic-cni/rbac.yaml` | shares the `galactic-cni` SA (`daemonset.yaml:17`) — confirmed | -| GC backstop | `galactic-router` | `internal/gc/gc.go`, `internal/controller/gc_controller.go` | extend `RemoveOrphanedCRDs` (`gc.go:189-231`), not the CNI's eBPF sweep | -| GC RBAC | `galactic-router`'s SA | `config/galactic-router/rbac.yaml` | mirror existing `bgpadvertisements` delete grant (`rbac.yaml:15-19`) | +| SID computation | `galactic-bgp` | `internal/cnibgp` | `publishBGPState`, `bgp.go:316-421` | +| EndpointSlice publish (ADD) | `galactic-bgp` | `internal/cnibgp` (new file) | after `publishBGPState` succeeds (`ops_add.go`), **not** folded into its retry closure — see Phase 4's rollback-risk callout | +| EndpointSlice delete (DEL) | `galactic-bgp` | `internal/cnibgp/ops_del.go` | currently a no-op (delegates all cleanup to GC) | +| EndpointSlice validate (CHECK) | `galactic-bgp` | `internal/cnibgp/ops_check.go` | after the existing BGPAdvertisement `Get` | +| Naming/annotation vocabulary | both | `internal/crdnames` | already the shared cross-plugin vocab package (moved out from under `internal/cni`) | +| Pod-name parsing | `galactic-bgp` (new dependency) | `internal/nadpatch` | sibling to existing `ParsePodNamespace` (moved out from under `internal/cni`) | +| RBAC (publish/delete) | `galactic-bgp`'s SA | `config/galactic-cni/rbac.yaml` | shares the `galactic-cni` ClusterRole/SA — confirmed | +| GC / cleanup backstop | `galactic-router` (and/or the k8s garbage collector) | `internal/gc/gc.go` | see Phase 8 — reconsidered, not a simple extension of `RemoveOrphanedCRDs` | +| GC RBAC | `galactic-router`'s SA | `config/galactic-router/rbac.yaml` | mirror existing `bgpadvertisements`/`bgpvrfinstances` delete grant | + +> **Revision note:** `internal/cni/crdnames` and `internal/cni/nadpatch` are stale paths — both packages have since moved to top-level leaf packages, `internal/crdnames` and `internal/nadpatch`. No structural change to Phase 1/2's plan, just update imports accordingly. (Also note: `config/cni/` and `config/router/` have since been renamed to `config/galactic-cni/` and `config/galactic-router/` respectively — paths above already reflect the new names.) ## 3. Work items, in dependency order -### Phase 1 — Shared vocabulary (`internal/cni/crdnames`) +### Phase 1 — Shared vocabulary (`internal/crdnames`) -Add annotation-key constants (SID, tenant identifier) alongside the existing ones (`crdnames.go:20,26,35`), plus a `TenantIdentifier(vpc, vpcAttachment string) string` helper formatted the same way as `BGPVRFInstanceName`/`BGPAdvertisementName` (`crdnames.go:74-83`) so the value is consistent with names used elsewhere, and an `EndpointSliceName(podName string) string` helper (trivial passthrough, but centralizes the convention like everything else in this package does). +Add annotation-key constants (SID, tenant identifier) alongside the existing ones (`AnnotationAllocatedSubnetIPv6`/`IPv4`/`NetNS`), plus a `TenantIdentifier(vpc, vpcAttachment string) string` helper formatted the same way as `BGPVRFInstanceName`/`BGPAdvertisementName` so the value is consistent with names used elsewhere, and an `EndpointSliceName(podName string) string` helper (trivial passthrough, but centralizes the convention like everything else in this package does — see Phase 4's naming-collision note for why "trivial" still needs a defensive check downstream). -Also add a **label** key constant, `galactic.datum.net/tenant-id`, carrying the same `TenantIdentifier(vpc, vpcAttachment)` value as the annotation (see Discovery label, below) — annotations aren't selectable in a k8s `List`/`Watch` call, so the extension server needs this value present as a label, not only an annotation, to find these EndpointSlices at all. Unit tests alongside `crdnames_test.go`. +Also add a **label** key constant, `galactic.datum.net/tenant-id`, carrying the same `TenantIdentifier(vpc, vpcAttachment)` value as the annotation (see Discovery label, Open Decision 2) — annotations aren't selectable in a k8s `List`/`Watch` call, so the extension server needs this value present as a label, not only an annotation, to find these EndpointSlices at all. Unit tests alongside `crdnames_test.go`. ### Phase 2 — Pod-name parsing -`internal/cnibgp` currently has no notion of pod name (only `internal/cni` parses `K8S_POD_NAMESPACE` via `nadpatch.ParsePodNamespace`). Add a sibling `ParsePodName` to `nadpatch` and import `nadpatch` from `internal/cnibgp` (new import edge — confirm no cycle; `nadpatch` has no dependency back on `cnibgp` today so this should be clean). Tests mirroring `nadpatch_test.go`'s existing fixture style. +`internal/cnibgp` currently has no notion of pod name (only the master plugins parse `K8S_POD_NAMESPACE` via `nadpatch.ParsePodNamespace`). Add a sibling `ParsePodName` to `internal/nadpatch` and import it from `internal/cnibgp` (new import edge — confirm no cycle; `nadpatch` has no dependency back on `cnibgp` today so this should be clean). Tests mirroring `nadpatch_test.go`'s existing fixture style. ### Phase 3 — SID computation in `galactic-bgp` -Import `internal/plumbing/srv6` into `internal/cnibgp` (net-new; today `ComputeSID`'s only caller is `galactic-router`'s `resolveSRv6SID`, `reconcile.go:376-386`). Inside `publishBGPState`'s retry closure, once `vrfID` is allocated (`bgp.go:329`) and `bgp.srv6Locator`/`bgp.nodeID` are in scope (`bgp.go:324`), call `ComputeSID(bgp.srv6Locator, bgp.nodeID, vrfID, bgpv1alpha1.SRv6FunctionEndDT46)`. Reuse the existing "SRv6 not configured, skip silently" sentinel already established at `bgp.go:418-420` (`registerEBPFDatapath`'s guard) — if SRv6 isn't configured on this node, there's nothing to publish, so EndpointSlice publish should skip the same way. +Import `internal/plumbing/srv6` into `internal/cnibgp` (net-new; today `ComputeSID`'s only other caller is `galactic-router`'s reconciler). Inside `publishBGPState`'s retry closure, once `vrfID` is allocated and `bgp.srv6Locator`/`bgp.nodeID` are in scope, call `ComputeSID(bgp.srv6Locator, bgp.nodeID, vrfID, bgpv1alpha1.SRv6FunctionEndDT46)`. Reuse the existing "SRv6 not configured, skip silently" sentinel already established by `registerEBPFDatapath`'s guard (`bgp.srv6Locator == "" || bgp.nodeID == 0` → `registered=false, err=nil`) — if SRv6 isn't configured on this node, there's nothing to publish, so EndpointSlice publish should skip the same way. ### Phase 4 — EndpointSlice publish (ADD) -New file `internal/cnibgp/endpointslice.go`, structurally mirroring the existing CreateOrUpdate pattern used for BGPVRFInstance/BGPAdvertisement (`bgp.go:341-353`). Inputs: namespace (already parsed for the BGP path), pod name (Phase 2), the pod's address (already available as `ipamResult`, passed into `publishBGPState` per `ops_add.go:69`), the computed SID (Phase 3), and `(vpc, vpcAttachment)`. Wire the call into `cmdAdd` right after `publishBGPState` succeeds, as its own step under the existing `resourceTracker` rollback (`ops_add.go:41-55`) rather than folded inside the BGP retry closure — keeps failure/rollback attribution clear. +New file `internal/cnibgp/endpointslice.go`, structurally mirroring the existing CreateOrUpdate pattern used for BGPVRFInstance/BGPAdvertisement. Inputs: namespace (already parsed for the BGP path), pod name (Phase 2), the pod's address (already available as `ipamResult`, passed into `publishBGPState`), the computed SID (Phase 3), and `(vpc, vpcAttachment)`. + +> **Revision note — implementation detail:** `ipamResult.IPv6Subnet` is a `*net.IPNet` whose `.IP` is the pod's actual single address, assigned to the guest interface with a `/96` mask (it's not a distinct subnet block from the address). The EndpointSlice's `Endpoints[].Addresses` wants the bare address — use `.IP` alone, not the CIDR string. + +> **⚠️ Revision note — rollback risk, read before wiring this in.** The original plan called for wiring this call in "right after `publishBGPState` succeeds, as its own step under the existing `resourceTracker` rollback — rather than folded inside the BGP retry closure — keeps failure/rollback attribution clear." That sequencing is unsafe as-is. `publishBGPState` sets `result.advertisementCreated = true` **unconditionally** after any successful `CreateOrUpdate` on the `BGPAdvertisement` — unlike `vrfInstanceCreated`, which correctly gates on `op == OperationResultCreated`. `resourceTracker.cleanup` then deletes the `BGPAdvertisement` unconditionally whenever `advertisementCreated` is true, with no "did I create this or just reuse a live sibling's" distinction. `BGPAdvertisement` objects are reused (updated, not created) across pod churn on the same `vpcAttachment`, so if EndpointSlice publish is a separate downstream step and it fails, the deferred rollback can delete a `BGPAdvertisement` that's still backing a different, live container's route — not just this ADD's own. Pick one before implementing: +> 1. Fix `advertisementCreated` to mirror `vrfInstanceCreated`'s create-only semantics first, or +> 2. Fold EndpointSlice publish inside `publishBGPState`'s own retry closure after all (reverting the "keep it separate" call), or +> 3. Make `resourceTracker.cleanup` never delete a `BGPAdvertisement` it only updated. +> +> This didn't matter before this issue because nothing failure-prone ran between the `CreateOrUpdate` and `cmdAdd` returning; adding a whole new k8s write in between is exactly what turns this from a theoretical crack into a real footgun. + +> **Revision note — naming collision.** `EndpointSliceName(podName)` keys `CreateOrUpdate` purely on the pod's name in its own namespace. If any other EndpointSlice (Service-backed or otherwise) ever ends up with that exact name, this code will silently start mutating an object it doesn't own. Add a defensive check — e.g. bail if a pre-existing object at that name/namespace lacks the `tenant-id` label — rather than trusting name uniqueness alone. + +> **Revision note — VM/tap and no-IPAM skip.** `ipamResult` can be `nil` — both for `galactic-tap` (VM) attachments, which manage their own addressing, and for any veth attachment with no `"ipam"` block configured. Phase 4 needs an explicit skip for this case (same pattern as the SRv6-not-configured sentinel: no address to publish, no EndpointSlice, not an error) — see Open Decision 5 for the product-level question of whether VM workloads should ever get one. Object carries, per pod: -- **Annotations**: SID (`galactic.datum.net/srv6-sid`), tenant identifier (`galactic.datum.net/tenant-id`) — human-readable detail, matches the existing annotation-based pattern used elsewhere in this codebase (e.g. the `galactic.datum.net/netns.` annotation GC already reads). -- **Label**: `galactic.datum.net/tenant-id` set to the same `TenantIdentifier(vpc, vpcAttachment)` value — this is the discovery mechanism (see Open Decisions, item 2): its mere presence marks an EndpointSlice as galactic-published (distinguishing it from ordinary Service-backed slices in the same cluster), and its value lets a consumer select/group by tenant, without a backing `Service` object or the standard `kubernetes.io/service-name` label. +- **Annotations**: SID (`galactic.datum.net/srv6-sid`), tenant identifier (`galactic.datum.net/tenant-id`) — human-readable detail, matches the existing annotation-based pattern used elsewhere in this codebase. +- **Label**: `galactic.datum.net/tenant-id` set to the same `TenantIdentifier(vpc, vpcAttachment)` value — this is the discovery mechanism (see Open Decision 2). -**IPv6-only** (see Open Decisions, item 1) — one `EndpointSlice`, `AddressType: IPv6`, carrying the pod's ULA `/96` address. A dual-stack pod's IPv4 address is not published; IPv4 VPC backends are out of scope for this issue. +**IPv6-only** (see Open Decision 1) — one `EndpointSlice`, `AddressType: IPv6`, carrying the pod's address. A dual-stack pod's IPv4 address is not published; IPv4 VPC backends are out of scope for this issue. ### Phase 5 — EndpointSlice delete (DEL) -`internal/cnibgp/ops_del.go:22-28` is currently a complete no-op — this is the first real work it does. Parse config, build the k8s client, parse pod name/namespace from `args.Args`, delete by name+namespace, treat NotFound as success (idempotent, per the acceptance criteria). On any other failure, follow the existing convention at `internal/cni/ops_del.go:72-81` — log and continue rather than fail DEL, since a k8s API hiccup during pod teardown shouldn't block the pod from actually going away; GC is the backstop. +`internal/cnibgp/ops_del.go`'s `cmdDel` is currently a complete no-op that defers all cleanup to GC, on the reasoning that the shared BGP CRDs may still be in use by another pod's attachment — this is the first real work it does. Parse config, build the k8s client, parse pod name/namespace from `args.Args`, delete by name+namespace, treat NotFound as success (idempotent, per the acceptance criteria). On any other failure, follow the existing best-effort convention elsewhere in the chain's own DEL paths — log and continue rather than fail DEL, since a k8s API hiccup during pod teardown shouldn't block the pod from actually going away; GC/owner-GC is the backstop (Phase 8). + +This is a deliberate, correct divergence from the "DEL is a no-op" pattern the rest of the chain follows: the reasoning behind that pattern (the resource might be shared with a live sibling) doesn't apply here — an EndpointSlice is 1:1 with exactly one pod, never shared. ### Phase 6 — CHECK validation -`internal/cnibgp/ops_check.go`: after the existing BGPAdvertisement Get (`:64-68`), Get the EndpointSlice by `crdnames.EndpointSliceName(podName)`, compare its address against the current IPAM/prevResult address and its annotations against freshly recomputed expected values, append mismatches into the same `errs` joined at `:81`. Needs pod-name parsing in CHECK's scope too (Phase 2 covers this). +`internal/cnibgp/ops_check.go`'s `cmdCheck`: after the existing BGPAdvertisement `Get`, `Get` the EndpointSlice by `crdnames.EndpointSliceName(podName)`, compare its address against the current IPAM/prevResult address and its annotations against freshly recomputed expected values, append mismatches into the same joined error. Needs pod-name parsing in CHECK's scope too (Phase 2 covers this). ### Phase 7 — RBAC -Add `discovery.k8s.io`/`endpointslices` (get, list, create, update, patch, delete) to `config/galactic-cni/rbac.yaml`. Confirmed: `config/galactic-cni/daemonset.yaml:17` sets a single `serviceAccountName: galactic-cni` for the whole pod — CNI chain plugins aren't separate pods/containers, they're binaries the kubelet execs on the host, all reading the kubeconfig the installer wrote from this one SA. `galactic-bgp` shares it, so this grant is the only RBAC change needed on the CNI side. +Add `discovery.k8s.io`/`endpointslices` (get, list, create, update, patch, delete) to `config/galactic-cni/rbac.yaml`. Confirmed: the CNI DaemonSet sets a single `serviceAccountName: galactic-cni` for the whole pod — CNI chain plugins aren't separate pods/containers, they're binaries the kubelet execs on the host, all reading the kubeconfig the installer wrote from this one SA. `galactic-bgp` shares it, so this grant is the only RBAC change strictly needed on the CNI side for publish/delete/CHECK. -### Phase 8 — GC backstop +> **Revision note:** if Open Decision 5 lands on the ownerReference approach, this phase also needs a `pods` `get` grant (to look up the owning Pod's UID) — not present in `config/galactic-cni/rbac.yaml` today. -EndpointSlice cleanup only needs k8s API access — no kernel/eBPF state — so it belongs in **galactic-router's `GCReconciler`** (ticker-driven, `cmd/galactic-router/root.go:212-231`), not the CNI's separate eBPF sweep (`internal/installer`'s `SweepEBPFVRFTable`, which exists specifically because that state is only reachable from inside the CNI's `run` container — see the precedent comment at `gc.go:326-348`). Recommend deleting the stale EndpointSlice as a side effect of the existing `RemoveOrphanedCRDs` pass (`gc.go:189-231`), keyed by the same per-pod netns-liveness signal already computed for BGPAdvertisement orphan detection (`gc.go:497-510`) — a pod's EndpointSlice is stale under exactly the same condition its BGPAdvertisement is. Add `discovery.k8s.io`/`endpointslices` (get, list, watch, delete) to `config/galactic-router/rbac.yaml`, mirroring the existing delete grant already carved out there specifically for this GC reconciler (`rbac.yaml:15-19`). +### Phase 8 — Cleanup backstop (reconsidered) + +> **Revision note — this phase needed a rethink, not just a path fix.** The original plan was to extend `RemoveOrphanedCRDs` in `internal/gc/gc.go`, "keyed by the same per-pod netns-liveness signal already computed for BGPAdvertisement orphan detection." That signal doesn't exist at pod granularity: `CollectOrphanedCRDs` judges a `BGPAdvertisement` orphaned only once *every* containerID ever recorded on it (across pod churn reusing the same `vpcAttachment`) is dead — it's deliberately vpcAttachment-scoped, not pod-scoped, and there is no persisted pod-name↔containerID mapping anywhere GC could read to ask "is *this specific pod's* EndpointSlice stale." That vpcAttachment-level sharing is also exactly why the BGP CRDs need a kernel-netns heuristic at all: there's no single object they belong to that the k8s garbage collector could key off. +> +> An EndpointSlice doesn't have that problem — it's genuinely 1:1 with one pod. That makes Kubernetes' own garbage collector a better fit than another netns sweep: +> +> **Recommended approach:** during ADD, `Get` the owning Pod (namespace + name, already known) and set `metadata.ownerReferences` on the EndpointSlice to it (same namespace, so the cross-namespace-owner restriction doesn't apply; `blockOwnerDeletion: false` since ordering doesn't matter here). When the Pod object is deleted, the API server's own garbage collector deletes the EndpointSlice automatically — no polling, no kernel-state heuristic, no new RBAC on `galactic-router`'s side at all. Phase 5's explicit delete-on-DEL stays as the fast, deterministic path for the common case; the ownerReference is the backstop for force-deleted/never-DEL'd pods, which is exactly the scenario the original GC-reconciler idea was trying to cover. +> +> **If a backstop independent of Pod-object presence is still wanted** (e.g. to cover a pod stuck in a bad state where the API object lingers but the workload is clearly gone), the fallback is to give the EndpointSlice its own per-container netns/containerID annotation — mirroring `crdnames.AnnotationNetNS`'s convention — so `galactic-router`'s GC reconciler can run an independent, EndpointSlice-scoped sweep shaped like `CollectOrphanedCRDs`, rather than trying to derive per-pod liveness from the BGPAdvertisement's aggregate annotations. This is more moving parts than the ownerReference approach and should only be added if a concrete gap in owner-based GC shows up. +> +> Either way, this phase is now smaller than originally scoped, and doesn't necessarily touch `internal/gc/gc.go`/`config/galactic-router/rbac.yaml` at all if the ownerReference approach fully covers it. ### Phase 9 — Config & docs -No new CNI config fields — `vpc`/`vpcAttachment`/`namespace` already exist on `internal/cnibgp/types.go:19-24`; pod name comes from `CNI_ARGS`, not the JSON config. Update `docs/cni/configuration.md`'s galactic-bgp section (currently states it "carries only vpc, vpcattachment, and namespace — nothing else," `:283-286`) to document the EndpointSlice side effect and its annotation schema. Touch `docs/agents/ARCHITECTURE.md` if it enumerates per-binary responsibilities. +No new CNI config fields — `vpc`/`vpcAttachment`/`namespace` already exist on `galactic-bgp`'s own config type; pod name comes from `CNI_ARGS`, not the JSON config. Update `docs/cni/configuration.md`'s galactic-bgp section (currently states its stanza "carries only vpc, vpcattachment, and namespace — nothing else") to document the EndpointSlice side effect and its annotation schema. + +> **Revision note:** the old monolithic `docs/agents/ARCHITECTURE.md` has been split into three component-scoped docs. This work touches `docs/agents/ARCHITECTURE-CNI.md` (per-binary responsibilities, module reference) and, if the ownerReference/GC approach in Phase 8 touches `galactic-router`, `docs/agents/ARCHITECTURE-ROUTER.md`'s GC section too. ### Phase 10 — Tests -`crdnames_test.go` (new helpers) → `nadpatch_test.go` (`ParsePodName`, valid/missing/malformed cases) → `bgp_test.go` or a new `endpointslice_test.go` using the existing `fakeClient(objs...)` helper (`bgp_test.go:50`), with `discoveryv1` added to `testScheme` (`bgp_test.go:41-46`) — cover fresh publish, update-in-place, and the SRv6-not-configured skip path → DEL idempotency and best-effort-on-failure tests → CHECK drift-detection tests → extend `gc_test.go`/`gc_ebpf_test.go` for EndpointSlice orphan removal → an e2e case in `tests/e2e/e2e_test.go` asserting the EndpointSlice appears on ADD and disappears on DEL. +`crdnames_test.go` (new helpers) → `nadpatch_test.go` (`ParsePodName`, valid/missing/malformed cases) → `bgp_test.go` or a new `endpointslice_test.go` using the existing `fakeClient(objs...)` helper, with `discoveryv1` added to `testScheme` (not present today) — cover fresh publish, update-in-place, the SRv6-not-configured skip path, **and the nil-`ipamResult` skip path** (Phase 4's VM/tap note) → DEL idempotency and best-effort-on-failure tests → CHECK drift-detection tests → the naming-collision defensive check (Phase 4) → ownerReference-GC behavior if that's the Phase 8 approach, else extend `gc_test.go`/`gc_ebpf_test.go` for EndpointSlice orphan removal → an e2e case in `tests/e2e/e2e_test.go` asserting the EndpointSlice appears on ADD and disappears on DEL (and, ideally, on Pod force-delete if ownerReference is used). ## 4. Suggested PR sequencing 1. `crdnames` + `nadpatch.ParsePodName` (small, foundational, reviewable alone) 2. SID computation wiring in `galactic-bgp` (Phase 3) — proves the `ComputeSID` call site and skip-sentinel, no EndpointSlice yet -3. EndpointSlice publish on ADD + RBAC (Phases 4, 7) +3. EndpointSlice publish on ADD + RBAC (Phases 4, 7) — **must resolve the rollback-risk and nil-`ipamResult` callouts in Phase 4 before merge** 4. EndpointSlice DEL + CHECK (Phases 5, 6) -5. GC backstop in `galactic-router` + its RBAC (Phase 8) +5. Cleanup backstop (Phase 8) + any RBAC it ends up needing 6. Docs + e2e (Phase 9, tail of Phase 10) ## 5. Open engineering decisions surfaced while planning -1. **Dual-stack shape — resolved: IPv6-only.** `EndpointSlice.AddressType` is singular per object (unlike the custom `BGPAdvertisement` CRD, which packs both families into one object today, e.g. `docs/cni/configuration.md`'s dual-stack example). This issue publishes a single `AddressType: IPv6` `EndpointSlice` per pod, carrying the pod's ULA `/96` address; a dual-stack pod's IPv4 address is not published. IPv4 VPC backends for HTTP ingress are out of scope here — consistent with the tenant addressing design being IPv6-primary and every example in the ingress design doc using ULA addresses. Publishing an IPv4 `EndpointSlice` alongside it, if ever needed, would be a follow-up issue, not an implicit extension of this one. -2. **Discovery label for the extension server — resolved.** No backing `Service` object, so no `kubernetes.io/service-name` label to key off (deliberately, per the design doc's "not synthesize a second EndpointSlice" point). **Decision: a new label, `galactic.datum.net/tenant-id`, carrying the same value as the `TenantIdentifier(vpc, vpcAttachment)` annotation.** galactic-cni sets this unilaterally as part of Phase 1/4 — it doesn't require the extension server to exist first, but it is the contract that component's future watch/index logic needs to consume. Worth flagging to whoever picks up that work so they don't invent a different key independently. -3. **`galactic-bgp`'s ServiceAccount — resolved.** Single shared `galactic-cni` SA confirmed via `config/galactic-cni/daemonset.yaml:17`; Phase 7 needs no split. -4. **Rollback semantics if EndpointSlice publish fails after BGPAdvertisement already succeeded — resolved.** `publishBGPState`'s writes are all `CreateOrUpdate` keyed by deterministic names; a failure just fails `cmdAdd`, kubelet retries, and the retry's `CreateOrUpdate` calls land on the same already-created objects rather than duplicating them. GC (Phase 8) covers the case where the pod never comes up at all. No new rollback code needed. +1. **Dual-stack shape — resolved: IPv6-only.** `EndpointSlice.AddressType` is singular per object (unlike the custom `BGPAdvertisement` CRD, which packs both families into one object today). This issue publishes a single `AddressType: IPv6` `EndpointSlice` per pod, carrying the pod's address; a dual-stack pod's IPv4 address is not published. IPv4 VPC backends for HTTP ingress are out of scope here — consistent with the tenant addressing design being IPv6-primary. Publishing an IPv4 `EndpointSlice` alongside it, if ever needed, would be a follow-up issue, not an implicit extension of this one. +2. **Discovery label for the extension server — resolved.** No backing `Service` object, so no `kubernetes.io/service-name` label to key off (deliberately, per the design doc's "not synthesize a second EndpointSlice" point). **Decision: a new label, `galactic.datum.net/tenant-id`, carrying the same value as the `TenantIdentifier(vpc, vpcAttachment)` annotation.** `galactic-bgp` sets this unilaterally as part of Phase 1/4 — it doesn't require the extension server to exist first, but it is the contract that component's future watch/index logic needs to consume. Worth flagging to whoever picks up that work so they don't invent a different key independently. +3. **`galactic-bgp`'s ServiceAccount — resolved.** Single shared `galactic-cni` SA confirmed; Phase 7 needs no split (beyond possibly adding a `pods` `get` grant — see Open Decision 5). +4. **Rollback semantics if EndpointSlice publish fails after BGPAdvertisement already succeeded — revisited, not fully resolved.** The original take ("`publishBGPState`'s writes are all idempotent `CreateOrUpdate`s, a failure just fails `cmdAdd` and retries land on the same objects, no new rollback code needed") is true of `publishBGPState`'s *own* retryable failures, but doesn't cover the case this issue actually introduces: a **separate step after** `publishBGPState` returns successfully, whose failure triggers the *existing* `resourceTracker` rollback. See Phase 4's rollback-risk callout — `advertisementCreated`'s current unconditional semantics mean that rollback path can delete a `BGPAdvertisement` still in use by an unrelated, live attachment. This needs one of the three fixes listed there before Phase 4 ships, not just "no new rollback code needed." +5. **New — VM/tap-attached workloads: in scope or not?** `galactic-bgp` runs after `galactic-tap` for VM workloads exactly as it does after `galactic-veth`, and `ipamResult` is `nil` whenever there's no IPAM allocation to publish (VM workloads managing their own addressing, or any attachment with no `"ipam"` block). Does a VM-backed attachment ever get an EndpointSlice, or is this issue implicitly veth/pod-only? If the latter (my read of intent, given the design doc's Envoy/HTTP-ingress framing), the code should skip explicitly on `ipamResult == nil` using the same "no error, just nothing to publish" pattern as the SRv6 skip, and that scope boundary should be stated outright rather than left implicit. If VM workloads are meant to be included, EndpointSlice semantics (a resource conventionally describing Pod endpoints) need a second look for a non-Pod-backed netns. +6. **New — GC mechanism: ownerReference vs. netns-heuristic sweep.** See Phase 8. Recommendation is ownerReference to the Pod as the primary mechanism (idiomatic, no polling, no new `galactic-router` RBAC), with Phase 5's explicit DEL as the fast path and no netns-based backstop unless a concrete gap turns up. Flagging as an open decision rather than folding it silently into Phase 8 because it changes what Phase 7's RBAC and Phase 9's `ARCHITECTURE-ROUTER.md` touch depend on. From 5497eeb6b63871215f1022dc64d92184c7d6b8d3 Mon Sep 17 00:00:00 2001 From: Peter Sprygada Date: Mon, 17 Aug 2026 09:07:09 -0400 Subject: [PATCH 2/4] docs: resolve VM/tap scope as in-scope primary use case for #854 Open Decision 5 asked whether VM/tap-attached workloads were in scope for EndpointSlice publication, or implicitly excluded. They are in scope, and are this issue's primary use case. Confirmed against internal/cnitap and internal/cnibgp/prevresult.go that Phase 4's nil-ipamResult skip was never a VM-exclusion mechanism to begin with: ipamResult is nil only when no "ipam" block is configured on the master plugin's stanza, which happens for galactic-veth and galactic-tap alike. A tap stanza configured with IPAM produces the same pool-allocated address publishBGPState already advertises via BGP for that attachment today, so it flows through Phase 4's existing publish path with no VM-specific branch needed. Practical implication for operators: tap-backed VMs need an "ipam" block configured for an EndpointSlice (or a BGP advertisement) to exist at all. Also flagged as a caveat, not a blocker: the pool-allocated address is never confirmed as actually bound inside the guest (no DHCP push, no ARP/NDP snooping in the current chain) - a pre-existing property of tap addressing, not something this issue introduces. Also fixed two pre-existing misnumbered cross-references to "Open Decision 5" that meant the GC/ownerReference decision (Decision 6). Co-Authored-By: Claude Sonnet 5 --- docs/plans/854-vpc-http-ingress-endpointslice.md | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/docs/plans/854-vpc-http-ingress-endpointslice.md b/docs/plans/854-vpc-http-ingress-endpointslice.md index b7973e5b..92db00db 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. +- **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. ## Correction to #854's framing @@ -66,7 +66,7 @@ New file `internal/cnibgp/endpointslice.go`, structurally mirroring the existing > **Revision note — naming collision.** `EndpointSliceName(podName)` keys `CreateOrUpdate` purely on the pod's name in its own namespace. If any other EndpointSlice (Service-backed or otherwise) ever ends up with that exact name, this code will silently start mutating an object it doesn't own. Add a defensive check — e.g. bail if a pre-existing object at that name/namespace lacks the `tenant-id` label — rather than trusting name uniqueness alone. -> **Revision note — VM/tap and no-IPAM skip.** `ipamResult` can be `nil` — both for `galactic-tap` (VM) attachments, which manage their own addressing, and for any veth attachment with no `"ipam"` block configured. Phase 4 needs an explicit skip for this case (same pattern as the SRv6-not-configured sentinel: no address to publish, no EndpointSlice, not an error) — see Open Decision 5 for the product-level question of whether VM workloads should ever get one. +> **Revision note — VM/tap and no-IPAM skip (resolved: not a VM-exclusion, an address-existence check).** `ipamResult` is `nil` whenever no `"ipam"` block is configured on the master plugin's stanza — this happens for both `galactic-veth` and `galactic-tap`, and is not tap-specific. Per Open Decision 5, VM/tap attachments are in scope and are this issue's primary use case; a tap stanza with an `"ipam"` block configured gets exactly the same `ipamResult` (the pool-allocated host/VRF-side address, already the same value `publishBGPState` advertises via BGP for that attachment today) that a veth attachment would, so Phase 4's publish path needs no tap-specific branch. The nil-skip only fires for a genuinely address-less attachment — tap with no `"ipam"` block (guest fully self-manages, host has no address to know about) or veth with none configured — same pattern as the SRv6-not-configured sentinel: no address to publish, no EndpointSlice, not an error. Operators running tap-backed VMs as HTTP ingress backends need an `"ipam"` block configured for this issue's EndpointSlice publish (and the existing BGP advertisement) to have anything to carry; note that neither this issue nor the existing BGP path confirms the guest actually bound the pool-allocated address (no DHCP push, no ARP/NDP snooping) — that's a pre-existing property of tap addressing generally, not a gap this issue introduces or is scoped to close. Object carries, per pod: - **Annotations**: SID (`galactic.datum.net/srv6-sid`), tenant identifier (`galactic.datum.net/tenant-id`) — human-readable detail, matches the existing annotation-based pattern used elsewhere in this codebase. @@ -88,7 +88,7 @@ This is a deliberate, correct divergence from the "DEL is a no-op" pattern the r Add `discovery.k8s.io`/`endpointslices` (get, list, create, update, patch, delete) to `config/galactic-cni/rbac.yaml`. Confirmed: the CNI DaemonSet sets a single `serviceAccountName: galactic-cni` for the whole pod — CNI chain plugins aren't separate pods/containers, they're binaries the kubelet execs on the host, all reading the kubeconfig the installer wrote from this one SA. `galactic-bgp` shares it, so this grant is the only RBAC change strictly needed on the CNI side for publish/delete/CHECK. -> **Revision note:** if Open Decision 5 lands on the ownerReference approach, this phase also needs a `pods` `get` grant (to look up the owning Pod's UID) — not present in `config/galactic-cni/rbac.yaml` today. +> **Revision note:** if Open Decision 6 lands on the ownerReference approach, this phase also needs a `pods` `get` grant (to look up the owning Pod's UID) — not present in `config/galactic-cni/rbac.yaml` today. ### Phase 8 — Cleanup backstop (reconsidered) @@ -125,7 +125,7 @@ No new CNI config fields — `vpc`/`vpcAttachment`/`namespace` already exist on 1. **Dual-stack shape — resolved: IPv6-only.** `EndpointSlice.AddressType` is singular per object (unlike the custom `BGPAdvertisement` CRD, which packs both families into one object today). This issue publishes a single `AddressType: IPv6` `EndpointSlice` per pod, carrying the pod's address; a dual-stack pod's IPv4 address is not published. IPv4 VPC backends for HTTP ingress are out of scope here — consistent with the tenant addressing design being IPv6-primary. Publishing an IPv4 `EndpointSlice` alongside it, if ever needed, would be a follow-up issue, not an implicit extension of this one. 2. **Discovery label for the extension server — resolved.** No backing `Service` object, so no `kubernetes.io/service-name` label to key off (deliberately, per the design doc's "not synthesize a second EndpointSlice" point). **Decision: a new label, `galactic.datum.net/tenant-id`, carrying the same value as the `TenantIdentifier(vpc, vpcAttachment)` annotation.** `galactic-bgp` sets this unilaterally as part of Phase 1/4 — it doesn't require the extension server to exist first, but it is the contract that component's future watch/index logic needs to consume. Worth flagging to whoever picks up that work so they don't invent a different key independently. -3. **`galactic-bgp`'s ServiceAccount — resolved.** Single shared `galactic-cni` SA confirmed; Phase 7 needs no split (beyond possibly adding a `pods` `get` grant — see Open Decision 5). +3. **`galactic-bgp`'s ServiceAccount — resolved.** Single shared `galactic-cni` SA confirmed; Phase 7 needs no split (beyond possibly adding a `pods` `get` grant — see Open Decision 6). 4. **Rollback semantics if EndpointSlice publish fails after BGPAdvertisement already succeeded — revisited, not fully resolved.** The original take ("`publishBGPState`'s writes are all idempotent `CreateOrUpdate`s, a failure just fails `cmdAdd` and retries land on the same objects, no new rollback code needed") is true of `publishBGPState`'s *own* retryable failures, but doesn't cover the case this issue actually introduces: a **separate step after** `publishBGPState` returns successfully, whose failure triggers the *existing* `resourceTracker` rollback. See Phase 4's rollback-risk callout — `advertisementCreated`'s current unconditional semantics mean that rollback path can delete a `BGPAdvertisement` still in use by an unrelated, live attachment. This needs one of the three fixes listed there before Phase 4 ships, not just "no new rollback code needed." -5. **New — VM/tap-attached workloads: in scope or not?** `galactic-bgp` runs after `galactic-tap` for VM workloads exactly as it does after `galactic-veth`, and `ipamResult` is `nil` whenever there's no IPAM allocation to publish (VM workloads managing their own addressing, or any attachment with no `"ipam"` block). Does a VM-backed attachment ever get an EndpointSlice, or is this issue implicitly veth/pod-only? If the latter (my read of intent, given the design doc's Envoy/HTTP-ingress framing), the code should skip explicitly on `ipamResult == nil` using the same "no error, just nothing to publish" pattern as the SRv6 skip, and that scope boundary should be stated outright rather than left implicit. If VM workloads are meant to be included, EndpointSlice semantics (a resource conventionally describing Pod endpoints) need a second look for a non-Pod-backed netns. +5. **VM/tap-attached workloads — resolved: in scope, and the primary use case.** `galactic-bgp` runs after `galactic-tap` for VM workloads exactly as it does after `galactic-veth`. Confirmed against the current code (`internal/cnitap`, `internal/cnibgp/prevresult.go`): `ipamResult` is nil only when no `"ipam"` block is configured on the master plugin's stanza — this is not tap-specific, and a tap stanza *with* `"ipam"` configured produces the same pool-allocated address `publishBGPState` already advertises via BGP for that attachment. So no VM-specific branch is needed in Phase 4 — the existing nil-skip already does the right thing (skip only when there's genuinely no address to publish), and tap attachments configured with IPAM flow through the normal publish path unchanged. Practical implication: operators running tap-backed VMs as HTTP-ingress backends need an `"ipam"` block on the tap stanza for an EndpointSlice (or a BGP advertisement) to exist at all. Since a VM is a Pod-backed netns exactly like a container attachment (the VM's guest runs inside a Pod's netns; there's no non-Pod-backed case to special-case), EndpointSlice's Pod-endpoint semantics need no rework, and Phase 8's ownerReference-to-Pod GC approach applies unchanged to tap attachments too. One caveat worth calling out to whoever owns the parent design (#796): the pool-allocated address is never confirmed as actually bound inside the guest (no DHCP push, no ARP/NDP snooping in the current chain) — a pre-existing property of tap addressing, not something this issue introduces, but worth being explicit about now that VM/tap is confirmed primary rather than a maybe-later extension. 6. **New — GC mechanism: ownerReference vs. netns-heuristic sweep.** See Phase 8. Recommendation is ownerReference to the Pod as the primary mechanism (idiomatic, no polling, no new `galactic-router` RBAC), with Phase 5's explicit DEL as the fast path and no netns-based backstop unless a concrete gap turns up. Flagging as an open decision rather than folding it silently into Phase 8 because it changes what Phase 7's RBAC and Phase 9's `ARCHITECTURE-ROUTER.md` touch depend on. From a9f19814aba717d152009875c72debbc7971b427 Mon Sep 17 00:00:00 2001 From: Peter Sprygada Date: Mon, 17 Aug 2026 09:12:18 -0400 Subject: [PATCH 3/4] docs: resolve Phase 4 rollback-risk decision for #854 Open Decision 4 (Phase 4's rollback-risk callout) offered three candidate fixes for the BGPAdvertisement wrongful-deletion hole and left the pick open. Resolved: fix #1, narrowing advertisementCreated's assignment (bgp.go:412) to gate on op == controllerutil.OperationResultCreated, mirroring vrfInstanceCreated's existing pattern (bgp.go:353-362) exactly. Verified by reading the actual code, not just the two candidates in isolation: - advertisementCreated has exactly one setter (bgp.go:412) and one reader (resourceTracker.cleanup, resource.go:92) across the whole repo. No DEL/CHECK/GC path depends on its current over-broad "true on any successful write" semantics, so narrowing it is a ~2-line diff with zero other blast radius. - Fix #3 ("cleanup never deletes an only-updated advertisement") needs the identical op-result signal captured at the same CreateOrUpdate call site to know create-vs-update after the fact - any real implementation of it collapses into fix #1 byte-for-byte. The only way to make it a distinct change is a second, redundant field nothing else would consume. - Fix #2 (fold EndpointSlice publish into publishBGPState's retry closure) doesn't close the gap by itself: cmdAdd's deferred rollback fires on any non-nil error regardless of which step produced it, so a failure there still hits the same wrongful-deletion path unless advertisementCreated's semantics are also fixed. Reasonable as a complementary retry-robustness improvement, but not a substitute, and not adopted here. Co-Authored-By: Claude Sonnet 5 --- docs/plans/854-vpc-http-ingress-endpointslice.md | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/docs/plans/854-vpc-http-ingress-endpointslice.md b/docs/plans/854-vpc-http-ingress-endpointslice.md index 92db00db..73b3051b 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. +- **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. ## Correction to #854's framing @@ -57,12 +57,13 @@ New file `internal/cnibgp/endpointslice.go`, structurally mirroring the existing > **Revision note — implementation detail:** `ipamResult.IPv6Subnet` is a `*net.IPNet` whose `.IP` is the pod's actual single address, assigned to the guest interface with a `/96` mask (it's not a distinct subnet block from the address). The EndpointSlice's `Endpoints[].Addresses` wants the bare address — use `.IP` alone, not the CIDR string. -> **⚠️ Revision note — rollback risk, read before wiring this in.** The original plan called for wiring this call in "right after `publishBGPState` succeeds, as its own step under the existing `resourceTracker` rollback — rather than folded inside the BGP retry closure — keeps failure/rollback attribution clear." That sequencing is unsafe as-is. `publishBGPState` sets `result.advertisementCreated = true` **unconditionally** after any successful `CreateOrUpdate` on the `BGPAdvertisement` — unlike `vrfInstanceCreated`, which correctly gates on `op == OperationResultCreated`. `resourceTracker.cleanup` then deletes the `BGPAdvertisement` unconditionally whenever `advertisementCreated` is true, with no "did I create this or just reuse a live sibling's" distinction. `BGPAdvertisement` objects are reused (updated, not created) across pod churn on the same `vpcAttachment`, so if EndpointSlice publish is a separate downstream step and it fails, the deferred rollback can delete a `BGPAdvertisement` that's still backing a different, live container's route — not just this ADD's own. Pick one before implementing: -> 1. Fix `advertisementCreated` to mirror `vrfInstanceCreated`'s create-only semantics first, or -> 2. Fold EndpointSlice publish inside `publishBGPState`'s own retry closure after all (reverting the "keep it separate" call), or -> 3. Make `resourceTracker.cleanup` never delete a `BGPAdvertisement` it only updated. +> **⚠️ Revision note — rollback risk, resolved: fix `advertisementCreated`'s gating first.** The original plan called for wiring this call in "right after `publishBGPState` succeeds, as its own step under the existing `resourceTracker` rollback — rather than folded inside the BGP retry closure — keeps failure/rollback attribution clear." That sequencing is unsafe as-is. `publishBGPState` sets `result.advertisementCreated = true` **unconditionally** after any successful `CreateOrUpdate` on the `BGPAdvertisement` (`bgp.go:394,412` — the op result is currently discarded via `_, err = ...`) — unlike `vrfInstanceCreated`, which correctly gates on `op == OperationResultCreated` (`bgp.go:353-362`). `resourceTracker.cleanup` (`resource.go:88-121`) then deletes the `BGPAdvertisement` unconditionally whenever `advertisementCreated` is true, with no "did I create this or just reuse a live sibling's" distinction. `BGPAdvertisement` objects are reused (updated, not created) across pod churn on the same `vpcAttachment`, so if EndpointSlice publish is a separate downstream step and it fails, the deferred rollback can delete a `BGPAdvertisement` that's still backing a different, live container's route — not just this ADD's own. > -> This didn't matter before this issue because nothing failure-prone ran between the `CreateOrUpdate` and `cmdAdd` returning; adding a whole new k8s write in between is exactly what turns this from a theoretical crack into a real footgun. +> **Decision: fix #1 — capture `CreateOrUpdate`'s op result at `bgp.go:394` and gate `result.advertisementCreated = true` on `op == controllerutil.OperationResultCreated`**, mirroring `vrfInstanceCreated`'s existing pattern exactly. Confirmed by a full-repo grep that `advertisementCreated` has exactly one setter (`bgp.go:412`) and one reader (`resource.go:92`) — no DEL/CHECK/GC path depends on its current over-broad "true on any successful write" semantics, so narrowing it is a ~2-line diff with zero other blast radius. The other two candidates were considered and rejected: +> - *Fix #3 ("make `resourceTracker.cleanup` never delete an only-updated `BGPAdvertisement`")* needs the identical op-result signal captured at the same call site to know create-vs-update after the fact — any real implementation of it collapses into fix #1 byte-for-byte. The only way to make it a distinct change is to add a second, redundant field nothing else would consume — strictly more code for the same outcome. +> - *Fix #2 ("fold EndpointSlice publish inside `publishBGPState`'s own retry closure")* doesn't close the gap by itself: `cmdAdd`'s deferred rollback fires on any non-nil error regardless of which step produced it or whether that step ran inside the closure or as a separate call afterward, so a failure there still hits the same wrongful-deletion path unless `advertisementCreated`'s semantics are also fixed. It's a reasonable complementary retry-robustness improvement (an EndpointSlice write failure would become eligible for the same transient-error retry/backoff as the BGP CRD writes) but not a substitute for fix #1, and isn't needed to close this specific hole — not adopted as part of this issue. +> +> This didn't matter before this issue because nothing failure-prone ran between the `CreateOrUpdate` and `cmdAdd` returning; adding a whole new k8s write in between is exactly what turns this from a theoretical crack into a real footgun. Land this fix (and its own unit test asserting an update-only path leaves `advertisementCreated` false) as part of Phase 4, before or alongside the EndpointSlice publish code itself — not as a follow-up. > **Revision note — naming collision.** `EndpointSliceName(podName)` keys `CreateOrUpdate` purely on the pod's name in its own namespace. If any other EndpointSlice (Service-backed or otherwise) ever ends up with that exact name, this code will silently start mutating an object it doesn't own. Add a defensive check — e.g. bail if a pre-existing object at that name/namespace lacks the `tenant-id` label — rather than trusting name uniqueness alone. @@ -126,6 +127,6 @@ No new CNI config fields — `vpc`/`vpcAttachment`/`namespace` already exist on 1. **Dual-stack shape — resolved: IPv6-only.** `EndpointSlice.AddressType` is singular per object (unlike the custom `BGPAdvertisement` CRD, which packs both families into one object today). This issue publishes a single `AddressType: IPv6` `EndpointSlice` per pod, carrying the pod's address; a dual-stack pod's IPv4 address is not published. IPv4 VPC backends for HTTP ingress are out of scope here — consistent with the tenant addressing design being IPv6-primary. Publishing an IPv4 `EndpointSlice` alongside it, if ever needed, would be a follow-up issue, not an implicit extension of this one. 2. **Discovery label for the extension server — resolved.** No backing `Service` object, so no `kubernetes.io/service-name` label to key off (deliberately, per the design doc's "not synthesize a second EndpointSlice" point). **Decision: a new label, `galactic.datum.net/tenant-id`, carrying the same value as the `TenantIdentifier(vpc, vpcAttachment)` annotation.** `galactic-bgp` sets this unilaterally as part of Phase 1/4 — it doesn't require the extension server to exist first, but it is the contract that component's future watch/index logic needs to consume. Worth flagging to whoever picks up that work so they don't invent a different key independently. 3. **`galactic-bgp`'s ServiceAccount — resolved.** Single shared `galactic-cni` SA confirmed; Phase 7 needs no split (beyond possibly adding a `pods` `get` grant — see Open Decision 6). -4. **Rollback semantics if EndpointSlice publish fails after BGPAdvertisement already succeeded — revisited, not fully resolved.** The original take ("`publishBGPState`'s writes are all idempotent `CreateOrUpdate`s, a failure just fails `cmdAdd` and retries land on the same objects, no new rollback code needed") is true of `publishBGPState`'s *own* retryable failures, but doesn't cover the case this issue actually introduces: a **separate step after** `publishBGPState` returns successfully, whose failure triggers the *existing* `resourceTracker` rollback. See Phase 4's rollback-risk callout — `advertisementCreated`'s current unconditional semantics mean that rollback path can delete a `BGPAdvertisement` still in use by an unrelated, live attachment. This needs one of the three fixes listed there before Phase 4 ships, not just "no new rollback code needed." +4. **Rollback semantics if EndpointSlice publish fails after BGPAdvertisement already succeeded — resolved.** The original take ("`publishBGPState`'s writes are all idempotent `CreateOrUpdate`s, a failure just fails `cmdAdd` and retries land on the same objects, no new rollback code needed") is true of `publishBGPState`'s *own* retryable failures, but doesn't cover the case this issue actually introduces: a **separate step after** `publishBGPState` returns successfully, whose failure triggers the *existing* `resourceTracker` rollback. **Decision: fix `advertisementCreated` to gate on `op == controllerutil.OperationResultCreated`** (candidate fix #1 in Phase 4's rollback-risk callout), mirroring `vrfInstanceCreated`'s existing pattern — confirmed safe via a full-repo grep showing no other code path depends on the field's current broader semantics. Land this fix as part of Phase 4, before or alongside the EndpointSlice publish code itself. 5. **VM/tap-attached workloads — resolved: in scope, and the primary use case.** `galactic-bgp` runs after `galactic-tap` for VM workloads exactly as it does after `galactic-veth`. Confirmed against the current code (`internal/cnitap`, `internal/cnibgp/prevresult.go`): `ipamResult` is nil only when no `"ipam"` block is configured on the master plugin's stanza — this is not tap-specific, and a tap stanza *with* `"ipam"` configured produces the same pool-allocated address `publishBGPState` already advertises via BGP for that attachment. So no VM-specific branch is needed in Phase 4 — the existing nil-skip already does the right thing (skip only when there's genuinely no address to publish), and tap attachments configured with IPAM flow through the normal publish path unchanged. Practical implication: operators running tap-backed VMs as HTTP-ingress backends need an `"ipam"` block on the tap stanza for an EndpointSlice (or a BGP advertisement) to exist at all. Since a VM is a Pod-backed netns exactly like a container attachment (the VM's guest runs inside a Pod's netns; there's no non-Pod-backed case to special-case), EndpointSlice's Pod-endpoint semantics need no rework, and Phase 8's ownerReference-to-Pod GC approach applies unchanged to tap attachments too. One caveat worth calling out to whoever owns the parent design (#796): the pool-allocated address is never confirmed as actually bound inside the guest (no DHCP push, no ARP/NDP snooping in the current chain) — a pre-existing property of tap addressing, not something this issue introduces, but worth being explicit about now that VM/tap is confirmed primary rather than a maybe-later extension. 6. **New — GC mechanism: ownerReference vs. netns-heuristic sweep.** See Phase 8. Recommendation is ownerReference to the Pod as the primary mechanism (idiomatic, no polling, no new `galactic-router` RBAC), with Phase 5's explicit DEL as the fast path and no netns-based backstop unless a concrete gap turns up. Flagging as an open decision rather than folding it silently into Phase 8 because it changes what Phase 7's RBAC and Phase 9's `ARCHITECTURE-ROUTER.md` touch depend on. From c0ad78f417c32c3572c596e71e25750cd1784cc0 Mon Sep 17 00:00:00 2001 From: Peter Sprygada Date: Mon, 17 Aug 2026 09:18:31 -0400 Subject: [PATCH 4/4] docs: adopt recommended GC mechanism for #854 (Open Decision 6) Open Decision 6 flagged a choice between the ownerReference and netns-heuristic-sweep approaches for Phase 8's cleanup backstop, with a recommendation but no adopted decision. Adopted the recommended ownerReference-to-Pod approach as-is: - Phase 8: during ADD, set metadata.ownerReferences on the EndpointSlice to the owning Pod; the API server's own garbage collector deletes it when the Pod is deleted. Phase 5's explicit delete-on-DEL remains the fast path. The netns-heuristic fallback is not adopted - more moving parts for no concrete gap identified. - Phase 7: needs a pods get grant on galactic-cni's ServiceAccount (to look up the owning Pod's UID) - the only RBAC this decision requires. - Phase 9: does NOT touch ARCHITECTURE-ROUTER.md's GC section - internal/gc/gc.go and config/router/rbac.yaml are untouched by this issue, since the ownerReference approach needs no galactic-router-side changes at all. - Phase 10: gc_test.go/gc_ebpf_test.go need no changes; the e2e case should additionally assert the EndpointSlice disappears on Pod force-delete via the ownerReference. Threaded the resolution through every cross-reference that depended on this decision (Phase 7's revision note, Phase 9's revision note, Phase 10's test note, PR sequencing item 5, and Open Decision 6 itself). Co-Authored-By: Claude Sonnet 5 --- .../854-vpc-http-ingress-endpointslice.md | 22 ++++++++++--------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/docs/plans/854-vpc-http-ingress-endpointslice.md b/docs/plans/854-vpc-http-ingress-endpointslice.md index 73b3051b..82266849 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. +- **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. ## Correction to #854's framing @@ -89,29 +89,31 @@ This is a deliberate, correct divergence from the "DEL is a no-op" pattern the r Add `discovery.k8s.io`/`endpointslices` (get, list, create, update, patch, delete) to `config/galactic-cni/rbac.yaml`. Confirmed: the CNI DaemonSet sets a single `serviceAccountName: galactic-cni` for the whole pod — CNI chain plugins aren't separate pods/containers, they're binaries the kubelet execs on the host, all reading the kubeconfig the installer wrote from this one SA. `galactic-bgp` shares it, so this grant is the only RBAC change strictly needed on the CNI side for publish/delete/CHECK. -> **Revision note:** if Open Decision 6 lands on the ownerReference approach, this phase also needs a `pods` `get` grant (to look up the owning Pod's UID) — not present in `config/galactic-cni/rbac.yaml` today. +> **Revision note:** per Open Decision 6's resolution (ownerReference approach), this phase also needs a `pods` `get` grant (to look up the owning Pod's UID) — not present in `config/galactic-cni/rbac.yaml` today. -### Phase 8 — Cleanup backstop (reconsidered) +### Phase 8 — Cleanup backstop (reconsidered, resolved) > **Revision note — this phase needed a rethink, not just a path fix.** The original plan was to extend `RemoveOrphanedCRDs` in `internal/gc/gc.go`, "keyed by the same per-pod netns-liveness signal already computed for BGPAdvertisement orphan detection." That signal doesn't exist at pod granularity: `CollectOrphanedCRDs` judges a `BGPAdvertisement` orphaned only once *every* containerID ever recorded on it (across pod churn reusing the same `vpcAttachment`) is dead — it's deliberately vpcAttachment-scoped, not pod-scoped, and there is no persisted pod-name↔containerID mapping anywhere GC could read to ask "is *this specific pod's* EndpointSlice stale." That vpcAttachment-level sharing is also exactly why the BGP CRDs need a kernel-netns heuristic at all: there's no single object they belong to that the k8s garbage collector could key off. > > An EndpointSlice doesn't have that problem — it's genuinely 1:1 with one pod. That makes Kubernetes' own garbage collector a better fit than another netns sweep: > -> **Recommended approach:** during ADD, `Get` the owning Pod (namespace + name, already known) and set `metadata.ownerReferences` on the EndpointSlice to it (same namespace, so the cross-namespace-owner restriction doesn't apply; `blockOwnerDeletion: false` since ordering doesn't matter here). When the Pod object is deleted, the API server's own garbage collector deletes the EndpointSlice automatically — no polling, no kernel-state heuristic, no new RBAC on `galactic-router`'s side at all. Phase 5's explicit delete-on-DEL stays as the fast, deterministic path for the common case; the ownerReference is the backstop for force-deleted/never-DEL'd pods, which is exactly the scenario the original GC-reconciler idea was trying to cover. +> **Decision: adopted the recommended approach.** During ADD, `Get` the owning Pod (namespace + name, already known) and set `metadata.ownerReferences` on the EndpointSlice to it (same namespace, so the cross-namespace-owner restriction doesn't apply; `blockOwnerDeletion: false` since ordering doesn't matter here). When the Pod object is deleted, the API server's own garbage collector deletes the EndpointSlice automatically — no polling, no kernel-state heuristic, no new RBAC on `galactic-router`'s side at all. Phase 5's explicit delete-on-DEL stays as the fast, deterministic path for the common case; the ownerReference is the backstop for force-deleted/never-DEL'd pods, which is exactly the scenario the original GC-reconciler idea was trying to cover. > -> **If a backstop independent of Pod-object presence is still wanted** (e.g. to cover a pod stuck in a bad state where the API object lingers but the workload is clearly gone), the fallback is to give the EndpointSlice its own per-container netns/containerID annotation — mirroring `crdnames.AnnotationNetNS`'s convention — so `galactic-router`'s GC reconciler can run an independent, EndpointSlice-scoped sweep shaped like `CollectOrphanedCRDs`, rather than trying to derive per-pod liveness from the BGPAdvertisement's aggregate annotations. This is more moving parts than the ownerReference approach and should only be added if a concrete gap in owner-based GC shows up. +> The independent-of-Pod-object-presence fallback (a per-container netns/containerID annotation mirroring `crdnames.AnnotationNetNS`, letting `galactic-router`'s GC reconciler run an EndpointSlice-scoped sweep shaped like `CollectOrphanedCRDs`) is **not adopted** — more moving parts than the ownerReference approach for no concrete gap identified yet. Revisit only if one turns up in practice. > -> Either way, this phase is now smaller than originally scoped, and doesn't necessarily touch `internal/gc/gc.go`/`config/galactic-router/rbac.yaml` at all if the ownerReference approach fully covers it. +> Concrete consequence of this decision: this phase does **not** touch `internal/gc/gc.go` or `config/galactic-router/rbac.yaml` at all — the only RBAC this needs is the `pods` `get` grant on `galactic-cni`'s ServiceAccount, called out in Phase 7. +> +> This phase is now smaller than originally scoped, and doesn't touch `internal/gc/gc.go`/`config/galactic-router/rbac.yaml` at all. ### Phase 9 — Config & docs No new CNI config fields — `vpc`/`vpcAttachment`/`namespace` already exist on `galactic-bgp`'s own config type; pod name comes from `CNI_ARGS`, not the JSON config. Update `docs/cni/configuration.md`'s galactic-bgp section (currently states its stanza "carries only vpc, vpcattachment, and namespace — nothing else") to document the EndpointSlice side effect and its annotation schema. -> **Revision note:** the old monolithic `docs/agents/ARCHITECTURE.md` has been split into three component-scoped docs. This work touches `docs/agents/ARCHITECTURE-CNI.md` (per-binary responsibilities, module reference) and, if the ownerReference/GC approach in Phase 8 touches `galactic-router`, `docs/agents/ARCHITECTURE-ROUTER.md`'s GC section too. +> **Revision note:** the old monolithic `docs/agents/ARCHITECTURE.md` has been split into three component-scoped docs. This work touches `docs/agents/ARCHITECTURE-CNI.md` (per-binary responsibilities, module reference) only — per Open Decision 6's resolution, Phase 8's ownerReference approach doesn't touch `galactic-router`, so `docs/agents/ARCHITECTURE-ROUTER.md`'s GC section needs no update. ### Phase 10 — Tests -`crdnames_test.go` (new helpers) → `nadpatch_test.go` (`ParsePodName`, valid/missing/malformed cases) → `bgp_test.go` or a new `endpointslice_test.go` using the existing `fakeClient(objs...)` helper, with `discoveryv1` added to `testScheme` (not present today) — cover fresh publish, update-in-place, the SRv6-not-configured skip path, **and the nil-`ipamResult` skip path** (Phase 4's VM/tap note) → DEL idempotency and best-effort-on-failure tests → CHECK drift-detection tests → the naming-collision defensive check (Phase 4) → ownerReference-GC behavior if that's the Phase 8 approach, else extend `gc_test.go`/`gc_ebpf_test.go` for EndpointSlice orphan removal → an e2e case in `tests/e2e/e2e_test.go` asserting the EndpointSlice appears on ADD and disappears on DEL (and, ideally, on Pod force-delete if ownerReference is used). +`crdnames_test.go` (new helpers) → `nadpatch_test.go` (`ParsePodName`, valid/missing/malformed cases) → `bgp_test.go` or a new `endpointslice_test.go` using the existing `fakeClient(objs...)` helper, with `discoveryv1` added to `testScheme` (not present today) — cover fresh publish, update-in-place, the SRv6-not-configured skip path, **and the nil-`ipamResult` skip path** (Phase 4's VM/tap note) → DEL idempotency and best-effort-on-failure tests → CHECK drift-detection tests → the naming-collision defensive check (Phase 4) → ownerReference-GC behavior (Phase 8's adopted approach — `gc_test.go`/`gc_ebpf_test.go` need no changes, since this issue doesn't touch `internal/gc/gc.go`) → an e2e case in `tests/e2e/e2e_test.go` asserting the EndpointSlice appears on ADD, disappears on DEL, and disappears on Pod force-delete via the ownerReference. ## 4. Suggested PR sequencing @@ -119,7 +121,7 @@ No new CNI config fields — `vpc`/`vpcAttachment`/`namespace` already exist on 2. SID computation wiring in `galactic-bgp` (Phase 3) — proves the `ComputeSID` call site and skip-sentinel, no EndpointSlice yet 3. EndpointSlice publish on ADD + RBAC (Phases 4, 7) — **must resolve the rollback-risk and nil-`ipamResult` callouts in Phase 4 before merge** 4. EndpointSlice DEL + CHECK (Phases 5, 6) -5. Cleanup backstop (Phase 8) + any RBAC it ends up needing +5. Cleanup backstop (Phase 8, ownerReference approach) + its `pods` `get` RBAC grant (Phase 7) 6. Docs + e2e (Phase 9, tail of Phase 10) ## 5. Open engineering decisions surfaced while planning @@ -129,4 +131,4 @@ No new CNI config fields — `vpc`/`vpcAttachment`/`namespace` already exist on 3. **`galactic-bgp`'s ServiceAccount — resolved.** Single shared `galactic-cni` SA confirmed; Phase 7 needs no split (beyond possibly adding a `pods` `get` grant — see Open Decision 6). 4. **Rollback semantics if EndpointSlice publish fails after BGPAdvertisement already succeeded — resolved.** The original take ("`publishBGPState`'s writes are all idempotent `CreateOrUpdate`s, a failure just fails `cmdAdd` and retries land on the same objects, no new rollback code needed") is true of `publishBGPState`'s *own* retryable failures, but doesn't cover the case this issue actually introduces: a **separate step after** `publishBGPState` returns successfully, whose failure triggers the *existing* `resourceTracker` rollback. **Decision: fix `advertisementCreated` to gate on `op == controllerutil.OperationResultCreated`** (candidate fix #1 in Phase 4's rollback-risk callout), mirroring `vrfInstanceCreated`'s existing pattern — confirmed safe via a full-repo grep showing no other code path depends on the field's current broader semantics. Land this fix as part of Phase 4, before or alongside the EndpointSlice publish code itself. 5. **VM/tap-attached workloads — resolved: in scope, and the primary use case.** `galactic-bgp` runs after `galactic-tap` for VM workloads exactly as it does after `galactic-veth`. Confirmed against the current code (`internal/cnitap`, `internal/cnibgp/prevresult.go`): `ipamResult` is nil only when no `"ipam"` block is configured on the master plugin's stanza — this is not tap-specific, and a tap stanza *with* `"ipam"` configured produces the same pool-allocated address `publishBGPState` already advertises via BGP for that attachment. So no VM-specific branch is needed in Phase 4 — the existing nil-skip already does the right thing (skip only when there's genuinely no address to publish), and tap attachments configured with IPAM flow through the normal publish path unchanged. Practical implication: operators running tap-backed VMs as HTTP-ingress backends need an `"ipam"` block on the tap stanza for an EndpointSlice (or a BGP advertisement) to exist at all. Since a VM is a Pod-backed netns exactly like a container attachment (the VM's guest runs inside a Pod's netns; there's no non-Pod-backed case to special-case), EndpointSlice's Pod-endpoint semantics need no rework, and Phase 8's ownerReference-to-Pod GC approach applies unchanged to tap attachments too. One caveat worth calling out to whoever owns the parent design (#796): the pool-allocated address is never confirmed as actually bound inside the guest (no DHCP push, no ARP/NDP snooping in the current chain) — a pre-existing property of tap addressing, not something this issue introduces, but worth being explicit about now that VM/tap is confirmed primary rather than a maybe-later extension. -6. **New — GC mechanism: ownerReference vs. netns-heuristic sweep.** See Phase 8. Recommendation is ownerReference to the Pod as the primary mechanism (idiomatic, no polling, no new `galactic-router` RBAC), with Phase 5's explicit DEL as the fast path and no netns-based backstop unless a concrete gap turns up. Flagging as an open decision rather than folding it silently into Phase 8 because it changes what Phase 7's RBAC and Phase 9's `ARCHITECTURE-ROUTER.md` touch depend on. +6. **GC mechanism: ownerReference vs. netns-heuristic sweep — resolved: ownerReference (recommended approach adopted).** See Phase 8. `ownerReference` to the Pod is the primary mechanism (idiomatic, no polling, no new `galactic-router` RBAC), with Phase 5's explicit DEL as the fast path and no netns-based backstop unless a concrete gap turns up in practice. This settles what Phase 7's RBAC and Phase 9's `ARCHITECTURE-ROUTER.md` touch depend on: Phase 7 needs a `pods` `get` grant on `galactic-cni`'s SA; Phase 9 does not need to touch `ARCHITECTURE-ROUTER.md`'s GC section, since `internal/gc/gc.go`/`config/galactic-router/rbac.yaml` are untouched by this issue.