feat: Publish EndpointSlices for VPC HTTP ingress backend discovery - #423
Draft
privateip wants to merge 5 commits into
Draft
feat: Publish EndpointSlices for VPC HTTP ingress backend discovery#423privateip wants to merge 5 commits into
privateip wants to merge 5 commits into
Conversation
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
…covery Implements the #854 plan (docs/plans/854-vpc-http-ingress-endpointslice.md, as revised by PR #376), phases 1-10: - internal/crdnames: TenantIdentifier/EndpointSliceName helpers, plus the LabelTenantID/AnnotationTenantID/AnnotationSID key constants. - internal/nadpatch: ParsePodName, sibling to ParsePodNamespace. - internal/cnibgp/bgp.go: compute the SRv6 uSID inside publishBGPState's retry closure once vrfID is allocated (reusing registerEBPFDatapath's own "SRv6 not configured" skip). Also fixes the rollback-risk gap the plan flagged: advertisementCreated is now gated on controllerutil.OperationResultCreated, mirroring vrfInstanceCreated's existing pattern, so a later step's failure can no longer make resourceTracker.cleanup delete a BGPAdvertisement still backing a live sibling attachment. - internal/cnibgp/endpointslice.go (new): publishEndpointSlice/ deleteEndpointSlice. Publish runs as its own step after publishBGPState succeeds, not folded into its retry closure. Sets an ownerReference to the owning Pod (Phase 8's GC backstop) and defends against a name collision with a non-tenant-labeled EndpointSlice. - internal/cnibgp/ops_add.go: wires EndpointSlice publish into cmdAdd, skipped when the attachment has no IPv6 address to carry (nil ipamResult or no IPv6Subnet) — not VM/tap-specific. - internal/cnibgp/ops_del.go: cmdDel now deletes the pod's EndpointSlice (1:1 with one pod, never shared, unlike the BGP CRDs) best-effort, logging and continuing rather than failing DEL. - internal/cnibgp/ops_check.go: cmdCheck now validates the EndpointSlice's address/labels/annotations, including a recomputed-SID check when this node's BGPRouter has SRv6 configured. - config/galactic-cni/rbac.yaml: grants discovery.k8s.io/endpointslices CRUD and pods get (for the ownerReference lookup) to galactic-cni's ClusterRole, shared by galactic-bgp. - docs/cni/configuration.md, docs/agents/ARCHITECTURE-CNI.md: document the new EndpointSlice publish behavior and annotation/label schema. Not yet covered: an e2e case (plan's Phase 10 tail) asserting the EndpointSlice appears on ADD, disappears on DEL, and disappears on Pod force-delete via the ownerReference. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
ecv
approved these changes
Aug 18, 2026
privateip
marked this pull request as draft
August 19, 2026 13:11
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
VPC HTTP ingress needs a way to discover which pods back a given VPC attachment, but there's no
Serviceobject behind these pods to generate one automatically. This adds that discovery mechanism:galactic-bgpnow publishes adiscoveryv1.EndpointSlicefor each pod it attaches to a VPC, alongside theBGPAdvertisement/BGPVRFInstanceCRDs it already writes.Each EndpointSlice carries the pod's IPv6 address, a tenant label the ingress extension server can watch/index on, and the SRv6 routing SID for that pod. It's created when the pod is attached, deleted when the pod detaches, and verified on every CNI CHECK. If a pod is force-deleted without a clean detach, the EndpointSlice is still cleaned up automatically via a Kubernetes owner reference to the pod — no separate garbage-collection pass needed.
Along the way, this closes a latent bug in the existing BGP-advertisement bookkeeping: a shared
BGPAdvertisementcould be mistakenly marked for deletion during a failed pod attach even when it was just updated (not created), which could have deleted routing state still in use by another pod. That's now fixed so only a genuine first-time creation is eligible for rollback.This is the implementation of the plan in
docs/plans/854-vpc-http-ingress-endpointslice.md, following review revisions in #376.Test plan
go build/go vet/golangci-lint/go test -raceall clean on every touched packagetests/e2eFixes datum-cloud/enhancements#854
Related to #376