feat: Add ingress sidecar for VPC backend connectivity - #424
Draft
privateip wants to merge 9 commits into
Draft
Conversation
The #855 plan modeled the sidecar's VRF and SRv6 route as keyed by the (vpc, vpcAttachment) tenant label, but that contradicts the primitives it's built on: internal/crdnames.BGPVRFInstanceName documents the kernel VRF as keyed by (vpc, node), shared across every attachment of a VPC on a node, and internal/plumbing/vrf's Add/Delete/TableID and intf.GenerateInterfaceNameVRF all take vpc alone — there's no attachment-scoped variant. Each pod's SID also varies by its own hosting node (ComputeSID's nodeID parameter), so pods of the same tenant on different nodes carry different SIDs, meaning "one route per tenant" doesn't hold either. This revises the plan to a two-tier reconcile model: VRF lifecycle keyed by VPC alone, route lifecycle keyed by individual EndpointSlice (pod), with the tenant label used only for slice discovery/grouping, never as the kernel-side key. Also fixes the primitive function signatures in section 2 to match reality, corrects a reference that pointed at bgpvrfinstance_controller.go (an empty passthrough reconciler) instead of internal/reconcile/reconcile.go for the actual desired-state diffing pattern, adds the sidecar's flock-path reuse (/var/lib/cni/galactic-vrf) as a third pre-merge verification item alongside the two already flagged, and adds two new open decisions: RBAC blast radius of the cluster-wide EndpointSlice watch, and the platform-wide kernel state every gateway replica ends up carrying under the corrected #796 scope. No code changes — this is a planning document only, ahead of implementation. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Settles §9's four still-open items from the prior revision pass: - RBAC blast radius (item 8): accept the recommended read-only ClusterRole as final, flag for #856's manifest reviewer - Per-replica platform-wide state (item 9): accept as-is for v1, lean on §6's VRF/route-count metrics and revisit at scale - Teardown grace-period interval (item 1): settle as a configurable knob with a 30s placeholder default, pending real #857 latency data - #856 deployment contract (item 3): confirm it stays a flagged dependency rather than writing it out further No scope or design changes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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>
…visions' into feat/855-ingress-sidecar
Implements docs/plans/855-ingress-sidecar-vpc-backend-connectivity.md (as revised by PR #377) against #854's already-resolved EndpointSlice schema: - internal/ingresssidecar (new): the core mechanism. - desired.go: BuildDesiredRoute translates one EndpointSlice into a DesiredRoute, gated on the tenant label (discovery) and the SID annotation (not always present yet). - backend.go: Backend interface + kernelBackend, wired directly to internal/plumbing/vrf and internal/plumbing/srv6 -- the same primitives galactic-cni's own pod-attachment path uses. Also lists existing kernel VRFs/routes for startup inventory. - store.go: Store, the two-granularity (VRF per-VPC, route per-pod) grace-period-aware reconciler at this package's core. SetDesired applies "up" transitions immediately; Sweep tears down "down" transitions only once their independent grace periods elapse, with a VPC's own grace period never starting while any of its routes are still within their own -- per §9 item 1 of the plan. Inventory seeds truly-orphaned kernel state at boot with a fresh grace period rather than ignoring or immediately deleting it -- §9 item 2. - controller.go: thin controller-runtime Reconciler translating EndpointSlice watch events into Store.SetDesired calls, plus RunSweeper for the periodic teardown pass. - metrics.go: Prometheus surface per §6 (active/pending VRF and route gauges, reconcile errors/latency). - internal/crdnames: adds ParseTenantIdentifier, splitting TenantIdentifier(vpc, vpcAttachment) back into its halves. Needed because the plan's §2 assumed `vpc` arrives on the EndpointSlice verbatim, but #854's actual contract only publishes the combined tenant identifier -- see that function's doc comment and the plan's own now-corrected §2 text. - internal/config: adds VRFConfig (GALACTIC_VRF_* env vars / flags), following RouterConfig/GatewayConfig's three-tier precedence convention. No NodeName field -- this sidecar has no CRD identity keyed by node, unlike its siblings. - cmd/galactic-vrf (new binary): manager wiring, RBAC pre-flight check for the EndpointSlice watch (§9 item 8), startup inventory gated on cache sync, and the periodic sweep goroutine. Deliberately has no gRPC health server -- §5 of the plan notes neither existing binary has an established convention to copy. - containers/galactic-vrf/Dockerfile: minimal distroless build, per §6. Not done here, both flagged in the plan's own §7 as required pre-merge and blocking before this sidecar is trusted with real traffic, neither possible from a sandboxed dev environment: - Real-kernel verification of RouteEgressAdd's netlink.RouteGet assumption and the vrf.Add/Delete flock path's writability, run from an actual Envoy Gateway pod's netns. - End-to-end eBPF decap verification against the existing TC-BPF uSID datapath. Also out of scope, per the plan's own dependency order (§8): #856's deployment/injection manifests (config/, sidecar patch). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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
1 participant
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
HTTP Ingress for VPC Networks (#796) needs a way to attach the shared Envoy Gateway fleet's node-local Linux VRF/SRv6 route state to each VPC backend pod, so Envoy's own upstream sockets can reach a tenant's pods over the SRv6 underlay. This adds that piece:
galactic-vrf, a new sidecar binary that runs as a second container in the Envoy Gateway pod and watches the per-podEndpointSlices galactic-cni publishes (#854).Two kernel resources are reconciled at two different granularities, matching how the underlying VRF/SID primitives actually work rather than the tenant label they're discovered through: one Linux VRF device per VPC (shared across every attachment of that VPC on the node), and one SRv6 egress route per pod (since pods of the same tenant on different nodes carry different SIDs). Teardown of either is delayed by a configurable grace period after the corresponding EndpointSlice disappears, and a VPC's own grace period never starts while any of its routes are still within their own — so a slow config-push on the extension-server side (#856/#857) can't get raced into a blackhole, and a live sibling attachment is never torn down out from under.
Startup inventories existing kernel state before the first reconcile so a restart doesn't either treat live pods as stale or leak truly orphaned VRFs forever.
Full design in
docs/plans/855-ingress-sidecar-vpc-backend-connectivity.md.Test plan
RouteEgressAdd'snetlink.RouteGetassumption and the VRF lock's flock-path writability, run from an actual Envoy Gateway pod's netnsOpening as draft until both required kernel/e2e checks above are done — see §7 of the plan.
Related to datum-cloud/enhancements#855
Related to datum-cloud/enhancements#796
🤖 Generated with Claude Code