From be82f8527343ed274e7bb4420098f9d35f2c3bc7 Mon Sep 17 00:00:00 2001 From: Peter Sprygada Date: Tue, 18 Aug 2026 16:44:20 -0400 Subject: [PATCH 1/5] feat(ingress-sidecar): implement #855 VPC backend VRF/SRv6 route sidecar MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- cmd/galactic-vrf/main.go | 87 +++++ cmd/galactic-vrf/root.go | 146 +++++++++ cmd/galactic-vrf/root_test.go | 65 ++++ containers/galactic-vrf/Dockerfile | 56 ++++ ...ngress-sidecar-vpc-backend-connectivity.md | 4 +- internal/config/vrf.go | 141 ++++++++ internal/config/vrf_test.go | 103 ++++++ internal/crdnames/crdnames.go | 26 ++ internal/crdnames/crdnames_test.go | 46 +++ internal/ingresssidecar/backend.go | 154 +++++++++ internal/ingresssidecar/controller.go | 99 ++++++ internal/ingresssidecar/controller_test.go | 110 +++++++ internal/ingresssidecar/desired.go | 84 +++++ internal/ingresssidecar/desired_test.go | 105 ++++++ internal/ingresssidecar/doc.go | 44 +++ internal/ingresssidecar/fakebackend_test.go | 148 +++++++++ internal/ingresssidecar/metrics.go | 69 ++++ internal/ingresssidecar/model.go | 29 ++ internal/ingresssidecar/store.go | 307 ++++++++++++++++++ internal/ingresssidecar/store_test.go | 262 +++++++++++++++ 20 files changed, 2083 insertions(+), 2 deletions(-) create mode 100644 cmd/galactic-vrf/main.go create mode 100644 cmd/galactic-vrf/root.go create mode 100644 cmd/galactic-vrf/root_test.go create mode 100644 containers/galactic-vrf/Dockerfile create mode 100644 internal/config/vrf.go create mode 100644 internal/config/vrf_test.go create mode 100644 internal/ingresssidecar/backend.go create mode 100644 internal/ingresssidecar/controller.go create mode 100644 internal/ingresssidecar/controller_test.go create mode 100644 internal/ingresssidecar/desired.go create mode 100644 internal/ingresssidecar/desired_test.go create mode 100644 internal/ingresssidecar/doc.go create mode 100644 internal/ingresssidecar/fakebackend_test.go create mode 100644 internal/ingresssidecar/metrics.go create mode 100644 internal/ingresssidecar/model.go create mode 100644 internal/ingresssidecar/store.go create mode 100644 internal/ingresssidecar/store_test.go diff --git a/cmd/galactic-vrf/main.go b/cmd/galactic-vrf/main.go new file mode 100644 index 00000000..de893d17 --- /dev/null +++ b/cmd/galactic-vrf/main.go @@ -0,0 +1,87 @@ +// Copyright 2026 Datum Cloud, Inc. +// +// SPDX-License-Identifier: AGPL-3.0-or-later + +// Command galactic-vrf is #855's ingress sidecar: the second container in +// the shared Envoy Gateway fleet's pod, responsible only for VPC backend +// connectivity — Linux VRF device + SRv6 seg6 encap route lifecycle, driven +// entirely by a cluster-scoped watch on discoveryv1.EndpointSlice objects +// galactic-cni (#854) publishes per pod. See +// docs/plans/855-ingress-sidecar-vpc-backend-connectivity.md and +// internal/ingresssidecar's own doc comment for the full design; this +// binary is just the process wiring (config, manager, metrics, RBAC +// pre-flight) around that package. +// +// Unlike galactic-router/galactic-gateway, this binary exposes no gRPC +// health server: neither of this repo's existing binaries has an +// established /healthz convention to copy (galactic-router explicitly +// disables its health probe; galactic-cni has none at all — see §5 of the +// plan), so building one here would be new design work rather than +// following a pattern, and container liveness (process exits, kubelet +// restarts it) is enough for v1. +package main + +import ( + "context" + "os" + "time" + + authorizationv1 "k8s.io/api/authorization/v1" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +const ( + discoveryAPIGroup = "discovery.k8s.io" + discoveryAPIVersion = "v1" + resourceEndpointSlices = "endpointslices" +) + +func main() { + cmd := newRootCommand() + if err := cmd.Execute(); err != nil { + os.Exit(1) + } +} + +// checkWatchPermissions issues a SelfSubjectAccessReview for the "watch" +// verb on endpointslices, the only resource this binary's manager watches. +// If the review denies the request the informer cache will never sync and +// the reconciler will be silently blocked; this logs a clear, actionable +// message at startup so the problem is immediately obvious. Mirrors +// cmd/galactic-router and cmd/galactic-gateway's identically-named +// functions, scoped to this binary's own single resource — see §9 item 8 +// of the plan (the read-only ClusterRole decision this check exists to +// help catch a misconfiguration of). +func checkWatchPermissions(mgr ctrl.Manager) { + c, err := client.New(mgr.GetConfig(), client.Options{Scheme: mgr.GetScheme()}) + if err != nil { + ctrl.Log.Error(err, "RBAC pre-flight: cannot create client, skipping check") + return + } + + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + + logger := ctrl.Log.WithName("rbac-preflight") + + review := &authorizationv1.SelfSubjectAccessReview{ + Spec: authorizationv1.SelfSubjectAccessReviewSpec{ + ResourceAttributes: &authorizationv1.ResourceAttributes{ + Verb: "watch", + Group: discoveryAPIGroup, + Version: discoveryAPIVersion, + Resource: resourceEndpointSlices, + }, + }, + } + if err := c.Create(ctx, review); err != nil { + logger.Error(err, "RBAC pre-flight: failed to submit access review for "+resourceEndpointSlices, "verb", "watch") + return + } + if review.Status.Allowed { + return + } + logger.Error(nil, "missing watch RBAC for "+resourceEndpointSlices, + "verb", "watch", "detail", "informer cache will not sync; add resource to ServiceAccount ClusterRole and restart") +} diff --git a/cmd/galactic-vrf/root.go b/cmd/galactic-vrf/root.go new file mode 100644 index 00000000..3be5df70 --- /dev/null +++ b/cmd/galactic-vrf/root.go @@ -0,0 +1,146 @@ +// Copyright 2026 Datum Cloud, Inc. +// +// SPDX-License-Identifier: AGPL-3.0-or-later + +package main + +import ( + "context" + "fmt" + "log" + "strings" + "time" + + "github.com/spf13/cobra" + "k8s.io/apimachinery/pkg/runtime" + utilruntime "k8s.io/apimachinery/pkg/util/runtime" + clientgoscheme "k8s.io/client-go/kubernetes/scheme" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/log/zap" + ctrlmetrics "sigs.k8s.io/controller-runtime/pkg/metrics" + metricsserver "sigs.k8s.io/controller-runtime/pkg/metrics/server" + + "go.datum.net/galactic/internal/config" + "go.datum.net/galactic/internal/ingresssidecar" + "go.datum.net/galactic/internal/metadata" +) + +const ( + appName = "galactic-vrf" + + appDesc = `Galactic ingress sidecar: per-pod VPC backend VRF/SRv6 route lifecycle + + Find more information at: https://www.datum.net/docs` +) + +// runCmd contains the application startup logic: it registers +// internal/ingresssidecar's Reconciler against a cluster-scoped +// EndpointSlice watch, then runs its startup inventory and periodic sweep +// once the manager's caches have synced. There is no BGP runtime, CRD +// scheme beyond clientgoscheme's built-in discoveryv1 registration, or +// per-node identity of any kind here — see internal/config.VRFConfig's own +// doc comment for why. +func runCmd(cfg *config.VRFConfig) error { + ctrl.SetLogger(zap.New(zap.UseDevMode(true))) + + scheme := runtime.NewScheme() + utilruntime.Must(clientgoscheme.AddToScheme(scheme)) + + mgr, err := ctrl.NewManager(ctrl.GetConfigOrDie(), ctrl.Options{ + Scheme: scheme, + HealthProbeBindAddress: "0", + Metrics: metricsserver.Options{ + BindAddress: fmt.Sprintf(":%d", cfg.MetricsPort), + }, + }) + if err != nil { + return fmt.Errorf("create manager: %w", err) + } + + ctx, cancel := context.WithCancel(ctrl.SetupSignalHandler()) + defer cancel() + + // Pre-flight RBAC check. + checkWatchPermissions(mgr) + + metrics := ingresssidecar.NewMetrics() + metrics.MustRegister(ctrlmetrics.Registry) + + backend := ingresssidecar.NewKernelBackend() + store := ingresssidecar.NewStore(backend, cfg.TeardownGracePeriod, metrics) + + if err := (&ingresssidecar.Reconciler{Store: store}).SetupWithManager(mgr); err != nil { + return fmt.Errorf("setup EndpointSlice controller: %w", err) + } + + // Startup inventory + periodic sweep, gated on the manager's caches + // having synced -- see Store.Inventory's own doc comment for why: every + // EndpointSlice that exists at boot must have already gone through its + // own initial Reconcile (and therefore SetDesired) before Inventory or + // Sweep ever run, or a live VPC/pod could be misjudged as orphaned. + // Mirrors cmd/galactic-router's own GC-ticker startup goroutine. + go func() { + if !mgr.GetCache().WaitForCacheSync(ctx) { + log.Printf("startup inventory: cache sync failed, skipping") + return + } + if err := store.Inventory(ctx, time.Now()); err != nil { + log.Printf("startup inventory: %v", err) + } + ingresssidecar.RunSweeper(ctx, store, cfg.SweepInterval) + }() + + if err := mgr.Start(ctx); err != nil { + return fmt.Errorf("manager exited: %w", err) + } + + // mgr.Start only returns nil once ctx is Done (signal-triggered + // shutdown -- there's no other source of cancellation here, unlike + // cmd/galactic-router/cmd/galactic-gateway's health-server-failure + // case). No proactive VRF/route teardown on exit: §6 of the plan + // leans toward leaving kernel state for the next instance to + // reconcile from scratch, since a live Envoy container next to a + // dying sidecar mid-rollout would otherwise blackhole in-flight + // connections. + return nil +} + +// newRootCommand builds the root cobra command with all flags and the +// application startup logic. +func newRootCommand() *cobra.Command { + cmd := &cobra.Command{ + Use: appName, + Short: strings.Split(appDesc, "\n")[0], + Long: appDesc, + RunE: func(cmd *cobra.Command, _ []string) error { + if ok, _ := cmd.Flags().GetBool("build-info"); ok { + fmt.Println(metadata.BuildInfo(appName)) + return nil + } + if ok, _ := cmd.Flags().GetBool("version"); ok { + fmt.Printf("galactic-vrf version %s\n", metadata.Version) + return nil + } + + cfg := config.NewVRFConfig() + cfg.BindFlags(cmd.Flags()) + if err := cfg.Validate(); err != nil { + return err + } + return runCmd(cfg) + }, + } + + cmd.Flags().IntP("metrics-port", "", + config.DefaultVRFMetricsPort, + "Metrics listen port") + cmd.Flags().DurationP("teardown-grace-period", "", + config.DefaultVRFTeardownGracePeriod, + "Delay before tearing down a route/VRF after it drops out of desired state") + cmd.Flags().DurationP("sweep-interval", "", + config.DefaultVRFSweepInterval, + "How often to re-check pending teardowns") + cmd.Flags().Bool("build-info", false, "Print build information and exit") + cmd.Flags().BoolP("version", "V", false, "Print version and exit") + return cmd +} diff --git a/cmd/galactic-vrf/root_test.go b/cmd/galactic-vrf/root_test.go new file mode 100644 index 00000000..1eadad15 --- /dev/null +++ b/cmd/galactic-vrf/root_test.go @@ -0,0 +1,65 @@ +// Copyright 2026 Datum Cloud, Inc. +// +// SPDX-License-Identifier: AGPL-3.0-or-later + +package main + +import ( + "testing" + + "github.com/spf13/cobra" + + "go.datum.net/galactic/internal/config" + "go.datum.net/galactic/internal/metadata" +) + +// testCmd creates a cobra command with the same flags as newRootCommand. +func testCmd(t *testing.T) *cobra.Command { + t.Helper() + cmd := &cobra.Command{Use: "test"} + cmd.Flags().IntP("metrics-port", "", config.DefaultVRFMetricsPort, "Metrics listen port") + cmd.Flags().DurationP("teardown-grace-period", "", config.DefaultVRFTeardownGracePeriod, "Teardown grace period") + cmd.Flags().DurationP("sweep-interval", "", config.DefaultVRFSweepInterval, "Sweep interval") + return cmd +} + +func TestFlagDefaults(t *testing.T) { + cfg := config.NewVRFConfig() + cmd := testCmd(t) + cfg.BindFlags(cmd.Flags()) + + if cfg.MetricsPort != config.DefaultVRFMetricsPort { + t.Errorf("MetricsPort = %d, want %d", cfg.MetricsPort, config.DefaultVRFMetricsPort) + } + if cfg.TeardownGracePeriod != config.DefaultVRFTeardownGracePeriod { + t.Errorf("TeardownGracePeriod = %v, want %v", cfg.TeardownGracePeriod, config.DefaultVRFTeardownGracePeriod) + } + if cfg.SweepInterval != config.DefaultVRFSweepInterval { + t.Errorf("SweepInterval = %v, want %v", cfg.SweepInterval, config.DefaultVRFSweepInterval) + } + if err := cfg.Validate(); err != nil { + t.Errorf("Validate() with defaults: %v", err) + } +} + +func TestFlagsOverrideEnv(t *testing.T) { + t.Setenv(config.EnvVRFMetricsPort, "9000") + t.Setenv(config.EnvVRFTeardownGracePeriod, "60s") + + cfg := config.NewVRFConfig() + cmd := testCmd(t) + if err := cmd.Flags().Set("metrics-port", "9500"); err != nil { + t.Fatalf("set --metrics-port flag: %v", err) + } + cfg.BindFlags(cmd.Flags()) + + if cfg.MetricsPort != 9500 { + t.Errorf("MetricsPort = %d, want 9500 (flag should override env var)", cfg.MetricsPort) + } +} + +func TestVersionMetadata(t *testing.T) { + if metadata.Version == "" { + t.Error("metadata.Version should not be empty") + } +} diff --git a/containers/galactic-vrf/Dockerfile b/containers/galactic-vrf/Dockerfile new file mode 100644 index 00000000..6f2faa3c --- /dev/null +++ b/containers/galactic-vrf/Dockerfile @@ -0,0 +1,56 @@ +# Build galactic-vrf, #855's ingress sidecar. Unlike containers/galactic-cni's +# Dockerfile (one image bundling every CNI-chain binary), this image ships a +# single binary: galactic-vrf runs as its own container, the second one in +# the shared Envoy Gateway fleet's pod (docs/plans/855-ingress-sidecar-vpc- +# backend-connectivity.md §5/§6), not part of the CNI chain at all. +# +# This repo has no wired-up CI publish pipeline for this image yet -- see +# root CLAUDE.md's CI/CD section and §9 item 3 of the plan (the #856 +# deployment contract is still an open, flagged dependency) -- this +# Dockerfile exists so the binary is buildable/deployable today, ahead of +# that decision. +FROM --platform=$BUILDPLATFORM golang:1.26 AS builder +ARG TARGETOS +ARG TARGETARCH +ARG VERSION=dev +ARG GIT_COMMIT=unknown +ARG GIT_TREE_STATE=unknown +ARG BUILD_DATE=unknown +ARG SPDX_LICENSE=AGPL-3.0-or-later +ARG GIT_URL=https://github.com/datum-cloud/galactic + +WORKDIR /workspace + +COPY go.mod go.mod +COPY go.sum go.sum +RUN go mod download + +COPY cmd/ cmd/ +COPY internal/ internal/ + +RUN CGO_ENABLED=0 GOOS=${TARGETOS:-linux} GOARCH=${TARGETARCH} go build \ + -ldflags "-s -w \ + -X go.datum.net/galactic/internal/metadata.Version=${VERSION} \ + -X go.datum.net/galactic/internal/metadata.GitCommit=${GIT_COMMIT} \ + -X go.datum.net/galactic/internal/metadata.GitTreeState=${GIT_TREE_STATE} \ + -X go.datum.net/galactic/internal/metadata.BuildDate=${BUILD_DATE} \ + -X go.datum.net/galactic/internal/metadata.SPDXLicense=${SPDX_LICENSE} \ + -X go.datum.net/galactic/internal/metadata.GitURL=${GIT_URL}" \ + -o galactic-vrf cmd/galactic-vrf/main.go + +# Minimal static-Go-binary image, matching containers/galactic-cni/ +# Dockerfile's production-stage pattern -- §6 of the plan. Unlike +# galactic-cni's own final image (alpine, for iproute2/nsenter needed by +# e2e tests), this container never shells out to `ip`: every kernel +# operation goes through internal/plumbing/vrf and internal/plumbing/srv6's +# own netlink calls, so distroless is sufficient. +FROM gcr.io/distroless/static:nonroot + +COPY --from=builder /workspace/galactic-vrf /galactic-vrf + +# CAP_NET_ADMIN only, never privileged -- per PR #851's explicit privilege +# split (this sidecar creates VRF devices/routes; Envoy only binds to them). +# The capability itself is granted by the pod's securityContext, not here; +# USER is left at distroless:nonroot's default (65532:65532) since +# CAP_NET_ADMIN does not require running as root. +ENTRYPOINT ["/galactic-vrf"] diff --git a/docs/plans/855-ingress-sidecar-vpc-backend-connectivity.md b/docs/plans/855-ingress-sidecar-vpc-backend-connectivity.md index 2faa052f..2134e37b 100644 --- a/docs/plans/855-ingress-sidecar-vpc-backend-connectivity.md +++ b/docs/plans/855-ingress-sidecar-vpc-backend-connectivity.md @@ -4,7 +4,7 @@ - **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)) - **Sibling plan:** [#854's implementation plan](https://github.com/datum-cloud/galactic/pull/309) (`docs/plans/854-vpc-http-ingress-endpointslice.md`, open/unmerged) — defines the exact annotation/label contract this plan consumes -- **Status:** planning only — no implementation started. Revised 2026-08-18: closed out §9's four still-open items — RBAC blast radius (item 8) and per-replica platform-wide state (item 9) both settled as accept-as-is (read-only `ClusterRole`; instrument via §6 metrics and revisit at scale, respectively), the teardown grace-period interval (item 1) settled as a configurable knob with a 30s placeholder default, and the #856 deployment contract (item 3) confirmed as staying a flagged dependency rather than being written out further. No scope or design changes — see §9 for detail on each. +- **Status:** core mechanism implemented (2026-08-18) — `cmd/galactic-vrf` + `internal/ingresssidecar` (Store/Reconciler/Backend, per §5), `internal/config.VRFConfig` (the §9 item 1 grace-period knob), `internal/crdnames.ParseTenantIdentifier` (recovers `vpc` from the tenant label — see that function's doc comment for why this was needed beyond what §2's original text assumed), and `containers/galactic-vrf/Dockerfile` (§6). Unit-tested (`go test ./internal/ingresssidecar/... ./internal/config/... ./internal/crdnames/... ./cmd/galactic-vrf/...`), full-repo build/vet/test clean otherwise. **Not done:** the two required-pre-merge kernel-verification passes in §7 (real-kernel `RouteEgressAdd`/flock-path checks from an actual Envoy Gateway pod netns, and end-to-end eBPF decap verification) — neither is possible from this sandbox and both remain blocking before this sidecar is trusted with real traffic; the Dockerfile is unbuilt/unpushed (no container runtime available in this sandbox); and #856's deployment/injection manifests are out of scope here as before (see §8's dependency order). ## Correction to #855's framing @@ -57,7 +57,7 @@ This is not new kernel-programming work — `internal/plumbing/vrf` and `interna | `srv6.RouteEgressDel(prefix *net.IPNet, tableID uint32) error` | `internal/plumbing/srv6/egress.go` | Remove it | | `intf.GenerateInterfaceNameVRF(vpc string) string` | `internal/plumbing/intf/intf.go` | Deterministic VRF device naming, keyed by VPC alone — same convention the sidecar and `galactic-cni` both key off | -Per #854's resolved annotation contract (below), `vpc` and each pod's own address/SID arrive on the `EndpointSlice` verbatim — no decoding or translation step. The reconcile loop is concretely, per `EndpointSlice` (per pod): read `vpc` off its tenant annotation → `vrf.Add(vpc)` → `vrf.TableID(vpc)` → `srv6.RouteEgressAdd(podPrefix, podSID, tableID)`, using that same pod's own address as `prefix` and its own SID annotation as `gateway` — never a tenant-aggregate value, since no such value exists (see §1). Teardown mirrors this in reverse, per pod, plus the separate VPC-level `vrf.Delete` once no pod of any attachment of that VPC remains. This is still the single biggest scope-reducer for this issue — it's a thin reconciler wired to existing plumbing, not new VRF/SRv6 management — just wired at pod/VPC granularity rather than tenant granularity. +Per #854's resolved annotation contract (below), each pod's own address/SID arrive on the `EndpointSlice` verbatim — no decoding or translation step. `vpc` itself does not, though, despite this section's earlier wording: the only tenant-scoped value #854 actually publishes is `crdnames.AnnotationTenantID`/`LabelTenantID`, holding `TenantIdentifier(vpc, vpcAttachment)` — the combined, hyphen-joined string, never `vpc` on its own. Recovering it needs one extra step: `crdnames.ParseTenantIdentifier` (implementation-time addition, 2026-08-18), which splits on the first `-` — safe because both halves are non-empty base62 ([0-9a-zA-Z]) and so never contain that separator themselves (see `cnimaster.IsValidBase62`). The reconcile loop is concretely, per `EndpointSlice` (per pod): read the tenant annotation → `crdnames.ParseTenantIdentifier` → `vrf.Add(vpc)` → `vrf.TableID(vpc)` → `srv6.RouteEgressAdd(podPrefix, podSID, tableID)`, using that same pod's own address as `prefix` and its own SID annotation as `gateway` — never a tenant-aggregate value, since no such value exists (see §1). Teardown mirrors this in reverse, per pod, plus the separate VPC-level `vrf.Delete` once no pod of any attachment of that VPC remains. This is still the single biggest scope-reducer for this issue — it's a thin reconciler wired to existing plumbing, not new VRF/SRv6 management — just wired at pod/VPC granularity rather than tenant granularity. **To confirm before relying on this:** - `RouteEgressAdd`'s doc comments assume the SID's route is resolved via `netlink.RouteGet` against the node's own default/main-table route — verified true for `galactic-cni` running in a pod netns; needs a quick check that it still holds running from the Envoy pod's netns/node. (Promoted to a required pre-merge test in §7.) diff --git a/internal/config/vrf.go b/internal/config/vrf.go new file mode 100644 index 00000000..34bac1e5 --- /dev/null +++ b/internal/config/vrf.go @@ -0,0 +1,141 @@ +// Copyright 2026 Datum Cloud, Inc. +// +// SPDX-License-Identifier: AGPL-3.0-or-later + +package config + +import ( + "errors" + "time" + + "github.com/spf13/pflag" + "github.com/spf13/viper" +) + +// --- VRF sidecar defaults --------------------------------------------- + +const ( + // DefaultVRFMetricsPort deliberately avoids the port ranges the other + // binaries in this repo already use (9179 router, 8081 gateway, 9180 + // CNI credential-refresh) for the same reason config.go's own + // DefaultGatewayMetricsPort comment gives: unlike those, though, this + // binary runs as a second container sharing its *pod's* network + // namespace with Envoy (not hostNetwork: true, see §1.5 of the #855 + // plan), so the port only has to avoid Envoy's own well-known ports + // (9901 admin, 10000 default listener) within that one pod, not every + // other galactic-* process on the node. + DefaultVRFMetricsPort = 9182 + + // DefaultVRFTeardownGracePeriod is a conservative placeholder, not a + // tuned value: long enough to plausibly cover a typical Envoy Gateway + // extension server (#856/#857) config-push-plus-drain window, short + // enough not to wedge normal-churn testing. See §9 item 1's + // 2026-08-18 decision in docs/plans/855-ingress-sidecar-vpc-backend- + // connectivity.md -- revisit once #857 exists and its latency is + // observable. + DefaultVRFTeardownGracePeriod = 30 * time.Second + + // DefaultVRFSweepInterval controls how often Store.Sweep re-checks + // pending teardowns -- see internal/ingresssidecar.RunSweeper's doc + // comment for why this has to be polling-driven. Independent of (and + // deliberately much shorter than) DefaultVRFTeardownGracePeriod: this + // is the granularity of the grace-period clock, not the grace period + // itself. + DefaultVRFSweepInterval = 5 * time.Second +) + +// --- VRF sidecar environment variable keys ----------------------------- + +const ( + EnvVRFMetricsPort = "GALACTIC_VRF_METRICS_PORT" + EnvVRFTeardownGracePeriod = "GALACTIC_VRF_TEARDOWN_GRACE_PERIOD" + EnvVRFSweepInterval = "GALACTIC_VRF_SWEEP_INTERVAL" +) + +// --- VRFConfig ----------------------------------------------------------- + +// VRFConfig resolves galactic-vrf (the #855 ingress sidecar) configuration +// with three-tier precedence: CLI flag > env var > compiled-in default. +// Create once via NewVRFConfig(), call BindFlags() to layer CLI flags, then +// read the exported fields. +// +// Unlike RouterConfig/GatewayConfig, there is no NodeName field: this +// sidecar has no CRD identity keyed by node (no BGPRouter TargetRef to +// match) and no ConfigMap/CRD configuration surface at all -- desired state +// derives entirely from the EndpointSlice watch (see §1 of the plan's +// acceptance-criteria table). +type VRFConfig struct { + v *viper.Viper + prefix string + + // Resolved fields. + MetricsPort int + TeardownGracePeriod time.Duration + SweepInterval time.Duration +} + +// NewVRFConfig creates a config resolver with the GALACTIC_VRF env prefix +// and AutomaticEnv enabled. Exported fields are populated from env vars and +// defaults; call BindFlags() to layer CLI overrides. +func NewVRFConfig() *VRFConfig { + v := viper.New() + v.SetEnvPrefix("GALACTIC_VRF") + v.AutomaticEnv() + + v.SetDefault("metrics_port", DefaultVRFMetricsPort) + v.SetDefault("teardown_grace_period", DefaultVRFTeardownGracePeriod.String()) + v.SetDefault("sweep_interval", DefaultVRFSweepInterval.String()) + + cfg := &VRFConfig{ + v: v, + prefix: "GALACTIC_VRF", + } + cfg.readFields() + return cfg +} + +// BindFlags binds Cobra/pflag flags to the config resolver and re-reads the +// exported fields. Each flag is bound to a Viper key using the key argument. +func (c *VRFConfig) BindFlags(flags *pflag.FlagSet) { + bindings := []struct { + flag string + key string + }{ + {"metrics-port", "metrics_port"}, + {"teardown-grace-period", "teardown_grace_period"}, + {"sweep-interval", "sweep_interval"}, + } + for _, b := range bindings { + if flags.Changed(b.flag) { + c.v.Set(b.key, flags.Lookup(b.flag).Value.String()) + } else { + //nolint:errcheck // controlled keys, BindPFlag cannot fail here + c.v.BindPFlag(b.key, flags.Lookup(b.flag)) + } + } + c.readFields() +} + +// readFields populates the exported fields from the current Viper state. +func (c *VRFConfig) readFields() { + c.MetricsPort = c.v.GetInt("metrics_port") + c.TeardownGracePeriod = c.v.GetDuration("teardown_grace_period") + c.SweepInterval = c.v.GetDuration("sweep_interval") +} + +// Validate checks that the resolved configuration is usable. +func (c *VRFConfig) Validate() error { + if c.MetricsPort < 1 || c.MetricsPort > 65535 { + return errors.New("metrics port must be between 1 and 65535") + } + if c.TeardownGracePeriod <= 0 { + return errors.New("teardown grace period must be positive") + } + if c.SweepInterval <= 0 { + return errors.New("sweep interval must be positive") + } + if c.SweepInterval > c.TeardownGracePeriod { + return errors.New("sweep interval must not be greater than the teardown grace period") + } + return nil +} diff --git a/internal/config/vrf_test.go b/internal/config/vrf_test.go new file mode 100644 index 00000000..798694d1 --- /dev/null +++ b/internal/config/vrf_test.go @@ -0,0 +1,103 @@ +// Copyright 2026 Datum Cloud, Inc. +// +// SPDX-License-Identifier: AGPL-3.0-or-later + +package config + +import ( + "strings" + "testing" + "time" +) + +func TestVRFConfigDefaults(t *testing.T) { + cfg := NewVRFConfig() + + if cfg.MetricsPort != DefaultVRFMetricsPort { + t.Errorf("MetricsPort = %d, want %d", cfg.MetricsPort, DefaultVRFMetricsPort) + } + if cfg.TeardownGracePeriod != DefaultVRFTeardownGracePeriod { + t.Errorf("TeardownGracePeriod = %v, want %v", cfg.TeardownGracePeriod, DefaultVRFTeardownGracePeriod) + } + if cfg.SweepInterval != DefaultVRFSweepInterval { + t.Errorf("SweepInterval = %v, want %v", cfg.SweepInterval, DefaultVRFSweepInterval) + } +} + +func TestVRFConfigEnvOverride(t *testing.T) { + t.Setenv(EnvVRFMetricsPort, "9999") + t.Setenv(EnvVRFTeardownGracePeriod, "45s") + t.Setenv(EnvVRFSweepInterval, "10s") + + cfg := NewVRFConfig() + + if cfg.MetricsPort != 9999 { + t.Errorf("MetricsPort = %d, want 9999", cfg.MetricsPort) + } + if cfg.TeardownGracePeriod != 45*time.Second { + t.Errorf("TeardownGracePeriod = %v, want 45s", cfg.TeardownGracePeriod) + } + if cfg.SweepInterval != 10*time.Second { + t.Errorf("SweepInterval = %v, want 10s", cfg.SweepInterval) + } +} + +func TestVRFConfigValidate(t *testing.T) { + tests := []struct { + name string + envVars map[string]string + wantErr string + }{ + { + name: "invalid metrics port", + envVars: map[string]string{EnvVRFMetricsPort: "0"}, + wantErr: "metrics port must be between", + }, + { + name: "non-positive grace period", + envVars: map[string]string{EnvVRFTeardownGracePeriod: "0s"}, + wantErr: "teardown grace period must be positive", + }, + { + name: "non-positive sweep interval", + envVars: map[string]string{EnvVRFSweepInterval: "0s"}, + wantErr: "sweep interval must be positive", + }, + { + name: "sweep interval longer than grace period", + envVars: map[string]string{ + EnvVRFTeardownGracePeriod: "5s", + EnvVRFSweepInterval: "10s", + }, + wantErr: "sweep interval must not be greater than", + }, + { + name: "valid config", + envVars: map[string]string{}, + wantErr: "", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + for k, v := range tc.envVars { + t.Setenv(k, v) + } + cfg := NewVRFConfig() + err := cfg.Validate() + if tc.wantErr == "" { + if err != nil { + t.Errorf("Validate() = %v, want nil", err) + } + return + } + if err == nil { + t.Errorf("Validate() = nil, want error containing %q", tc.wantErr) + return + } + if !strings.Contains(err.Error(), tc.wantErr) { + t.Errorf("Validate() = %q, want error containing %q", err, tc.wantErr) + } + }) + } +} diff --git a/internal/crdnames/crdnames.go b/internal/crdnames/crdnames.go index c5eb8f23..539b7d95 100644 --- a/internal/crdnames/crdnames.go +++ b/internal/crdnames/crdnames.go @@ -196,3 +196,29 @@ func TenantIdentifier(vpc, vpcAttachment string) string { func EndpointSliceName(podName string) string { return podName } + +// ParseTenantIdentifier splits a TenantIdentifier(vpc, vpcAttachment) value +// back into its vpc and vpcAttachment components. This is the ingress +// sidecar's (#855) only way to recover vpc — the value the kernel-side VRF +// primitives are actually keyed by, per docs/plans/855-ingress-sidecar-vpc- +// backend-connectivity.md §1/§2 — since neither LabelTenantID nor +// AnnotationTenantID carries vpc on its own, only the combined identifier. +// +// The split is unambiguous: both components are non-empty base62 strings +// ([0-9a-zA-Z], see internal/cnimaster.IsValidBase62) and base62 never +// contains "-", so vpc can never itself contain the separator +// TenantIdentifier joins the two halves with. This is a stronger guarantee +// than internal/gc's vpcFromVRFName has to work with — that one recovers vpc +// from a zero-padded, lossy kernel interface name and has to tolerate +// stripping leading zeros; this recovers it from the same unpadded string +// TenantIdentifier itself produced, so there is nothing lossy to correct +// for. +// +// Returns ok=false if id contains no "-" or either resulting half is empty. +func ParseTenantIdentifier(id string) (vpc, vpcAttachment string, ok bool) { + vpc, vpcAttachment, found := strings.Cut(id, "-") + if !found || vpc == "" || vpcAttachment == "" { + return "", "", false + } + return vpc, vpcAttachment, true +} diff --git a/internal/crdnames/crdnames_test.go b/internal/crdnames/crdnames_test.go index 17c58d67..c6da8c05 100644 --- a/internal/crdnames/crdnames_test.go +++ b/internal/crdnames/crdnames_test.go @@ -120,6 +120,52 @@ func TestTenantIdentifierDoesNotMatchBGPAdvertisementName(t *testing.T) { } } +func TestParseTenantIdentifier(t *testing.T) { + tests := []struct { + name string + id string + wantVPC string + wantAttachment string + wantOK bool + }{ + {"simple", "abc-def", testVPC, testAttachment, true}, + {"base62 vpc", "0000000jU-00G", testVPCBase62, "00G", true}, + {"no separator", "abcdef", "", "", false}, + {"empty vpc", "-def", "", "", false}, + {"empty attachment", "abc-", "", "", false}, + {"empty string", "", "", "", false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + gotVPC, gotAttachment, gotOK := ParseTenantIdentifier(tt.id) + if gotOK != tt.wantOK || gotVPC != tt.wantVPC || gotAttachment != tt.wantAttachment { + t.Errorf("ParseTenantIdentifier(%q) = (%q, %q, %v), want (%q, %q, %v)", + tt.id, gotVPC, gotAttachment, gotOK, tt.wantVPC, tt.wantAttachment, tt.wantOK) + } + }) + } +} + +// TestParseTenantIdentifierRoundTrip verifies ParseTenantIdentifier inverts +// TenantIdentifier for arbitrary base62 (vpc, vpcAttachment) pairs — the +// property #855's ingress sidecar reconciler actually relies on. +func TestParseTenantIdentifierRoundTrip(t *testing.T) { + tests := []struct{ vpc, attachment string }{ + {testVPC, testAttachment}, + {testVPCBase62, "00G"}, + {"0", "0"}, + {"Zz9", "aB0"}, + } + for _, tt := range tests { + id := TenantIdentifier(tt.vpc, tt.attachment) + gotVPC, gotAttachment, ok := ParseTenantIdentifier(id) + if !ok || gotVPC != tt.vpc || gotAttachment != tt.attachment { + t.Errorf("ParseTenantIdentifier(TenantIdentifier(%q, %q)) = (%q, %q, %v), want (%q, %q, true)", + tt.vpc, tt.attachment, gotVPC, gotAttachment, ok, tt.vpc, tt.attachment) + } + } +} + func TestEndpointSliceName(t *testing.T) { tests := []string{"my-pod", "web-0", "vm-workload-abc123"} for _, podName := range tests { diff --git a/internal/ingresssidecar/backend.go b/internal/ingresssidecar/backend.go new file mode 100644 index 00000000..c417ff9f --- /dev/null +++ b/internal/ingresssidecar/backend.go @@ -0,0 +1,154 @@ +// Copyright 2026 Datum Cloud, Inc. +// +// SPDX-License-Identifier: AGPL-3.0-or-later + +package ingresssidecar + +import ( + "fmt" + "net" + "regexp" + "strings" + + "github.com/vishvananda/netlink" + "golang.org/x/sys/unix" + + "go.datum.net/galactic/internal/plumbing/srv6" + "go.datum.net/galactic/internal/plumbing/vrf" +) + +// Backend is the kernel-facing interface Store converges VRF and SRv6 +// egress-route state against. kernelBackend (below) wires it to +// internal/plumbing/vrf and internal/plumbing/srv6 directly — the same +// primitives galactic-cni's own pod-attachment path uses; see §2 of the +// plan for why this is "not new kernel-programming work." Tests use a fake. +type Backend interface { + // EnsureVRF creates (idempotently) the per-VPC Linux VRF device and + // returns its kernel routing table ID. + EnsureVRF(vpc string) (tableID uint32, err error) + // RemoveVRF tears down the per-VPC VRF device. Callers must only call + // this once no route for this VPC remains live or in its own grace + // period — see vrf.Delete's own doc comment on why deleting out from + // under a still-live sibling breaks it. + RemoveVRF(vpc string) error + // EnsureRoute installs (idempotently — see srv6.RouteEgressAdd's use of + // netlink.RouteReplace) the seg6 ENCAP_RED route for prefix, toward + // sid, in tableID. + EnsureRoute(prefix *net.IPNet, sid net.IP, tableID uint32) error + // RemoveRoute removes the route EnsureRoute installed. + RemoveRoute(prefix *net.IPNet, tableID uint32) error + // ListVRFs returns every Galactic per-VPC VRF device currently present + // on the host, resolved back to its owning VPC — the startup-inventory + // step (§9 item 2 of the plan; see Store.Inventory). + ListVRFs() ([]VRFInfo, error) + // ListRoutes returns every seg6-encapsulated route currently installed + // in tableID — the route half of the same startup-inventory step. + ListRoutes(tableID uint32) ([]RouteInfo, error) +} + +// VRFInfo describes one kernel VRF device discovered by Backend.ListVRFs. +type VRFInfo struct { + VPC string + TableID uint32 +} + +// RouteInfo describes one seg6 egress route discovered by +// Backend.ListRoutes. +type RouteInfo struct { + Prefix *net.IPNet + SID net.IP +} + +// kernelBackend is the production Backend. +type kernelBackend struct{} + +// NewKernelBackend returns the production Backend, wired to real kernel +// state via internal/plumbing/vrf and internal/plumbing/srv6. Requires +// CAP_NET_ADMIN — see §6 of the plan. +func NewKernelBackend() Backend { return kernelBackend{} } + +func (kernelBackend) EnsureVRF(vpc string) (uint32, error) { + if err := vrf.Add(vpc); err != nil { + return 0, fmt.Errorf("create VRF for vpc %s: %w", vpc, err) + } + tableID, err := vrf.TableID(vpc) + if err != nil { + return 0, fmt.Errorf("resolve VRF table ID for vpc %s: %w", vpc, err) + } + return tableID, nil +} + +func (kernelBackend) RemoveVRF(vpc string) error { + if err := vrf.Delete(vpc); err != nil { + return fmt.Errorf("delete VRF for vpc %s: %w", vpc, err) + } + return nil +} + +func (kernelBackend) EnsureRoute(prefix *net.IPNet, sid net.IP, tableID uint32) error { + if err := srv6.RouteEgressAdd(prefix, sid, tableID); err != nil { + return fmt.Errorf("install seg6 route for %s: %w", prefix, err) + } + return nil +} + +func (kernelBackend) RemoveRoute(prefix *net.IPNet, tableID uint32) error { + if err := srv6.RouteEgressDel(prefix, tableID); err != nil { + return fmt.Errorf("remove seg6 route for %s: %w", prefix, err) + } + return nil +} + +// vrfNameRegex matches the interface name intf.GenerateInterfaceNameVRF +// produces for a VPC ("G%09sV" — 'G', 9 zero-padded base62 characters, +// 'V'). Mirrors internal/gc's identically-purposed, unexported +// vrfNameRegex; duplicated rather than imported since that package doesn't +// export it, with the same zero-pad-stripping caveat its parseVRFName +// documents (a vpc value that legitimately begins with '0' round-trips +// lossily through the padded interface name — an existing, accepted +// limitation this doesn't newly introduce). +var vrfNameRegex = regexp.MustCompile(`^G([A-Za-z0-9]{9})V$`) + +func (kernelBackend) ListVRFs() ([]VRFInfo, error) { + links, err := vrf.ListVRFLinks() + if err != nil { + return nil, fmt.Errorf("list VRF interfaces: %w", err) + } + infos := make([]VRFInfo, 0, len(links)) + for _, link := range links { + matches := vrfNameRegex.FindStringSubmatch(link.Name) + if matches == nil { + continue // not one of this sidecar's per-VPC VRFs + } + vpc := strings.TrimLeft(matches[1], "0") + if vpc == "" { + continue // defensive: an all-zero match can't be a real vpc + } + infos = append(infos, VRFInfo{VPC: vpc, TableID: link.Table}) + } + return infos, nil +} + +func (kernelBackend) ListRoutes(tableID uint32) ([]RouteInfo, error) { + var infos []RouteInfo + // Two passes, not AF_UNSPEC, matching internal/plumbing/vrf.FlushTable's + // own approach to listing everything in one table across both families. + for _, family := range []int{unix.AF_INET, unix.AF_INET6} { + routes, err := netlink.RouteListFiltered( + family, + &netlink.Route{Table: int(tableID)}, + netlink.RT_FILTER_TABLE, + ) + if err != nil { + return nil, fmt.Errorf("list routes in table %d: %w", tableID, err) + } + for _, route := range routes { + enc, ok := route.Encap.(*netlink.SEG6Encap) + if !ok || len(enc.Segments) == 0 || route.Dst == nil { + continue // not one of this sidecar's seg6 egress routes + } + infos = append(infos, RouteInfo{Prefix: route.Dst, SID: enc.Segments[0]}) + } + } + return infos, nil +} diff --git a/internal/ingresssidecar/controller.go b/internal/ingresssidecar/controller.go new file mode 100644 index 00000000..f442fd15 --- /dev/null +++ b/internal/ingresssidecar/controller.go @@ -0,0 +1,99 @@ +// Copyright 2026 Datum Cloud, Inc. +// +// SPDX-License-Identifier: AGPL-3.0-or-later + +package ingresssidecar + +import ( + "context" + "fmt" + "time" + + discoveryv1 "k8s.io/api/discovery/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/builder" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/predicate" + + "go.datum.net/galactic/internal/crdnames" +) + +// Reconciler is the thin controller-runtime glue that turns EndpointSlice +// watch events into Store.SetDesired calls — all the actual VRF/route +// lifecycle logic lives in Store. Mirrors this repo's other CRD-to-desired- +// state reconcilers (e.g. internal/controller.BGPAdvertisementReconciler) +// in being a pure translation layer with no state of its own. +type Reconciler struct { + client.Client + Store *Store +} + +// Reconcile implements reconcile.Reconciler. +func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { + slice := &discoveryv1.EndpointSlice{} + err := r.Get(ctx, req.NamespacedName, slice) + switch { + case apierrors.IsNotFound(err): + if serr := r.Store.SetDesired(ctx, req.String(), nil); serr != nil { + return ctrl.Result{}, fmt.Errorf("mark EndpointSlice %s absent: %w", req.NamespacedName, serr) + } + return ctrl.Result{}, nil + case err != nil: + return ctrl.Result{}, fmt.Errorf("get EndpointSlice %s: %w", req.NamespacedName, err) + } + + desired, err := BuildDesiredRoute(slice) + if err != nil { + // Selected but malformed in a way retrying can't fix (a bad + // annotation isn't going to parse differently on the next attempt) + // -- log via the returned error (controller-runtime logs Reconcile + // errors itself) and drop it rather than requeue-looping forever. + ctrl.LoggerFrom(ctx).Error(err, "skipping malformed EndpointSlice", "endpointslice", req.String()) + return ctrl.Result{}, nil + } + if err := r.Store.SetDesired(ctx, req.String(), desired); err != nil { + return ctrl.Result{}, fmt.Errorf("reconcile EndpointSlice %s: %w", req.NamespacedName, err) + } + return ctrl.Result{}, nil +} + +// SetupWithManager registers the controller against mgr, watching every +// EndpointSlice cluster-wide — per §3 of the plan, these land in each pod's +// own namespace, not one fixed namespace, so the cache/watch must not be +// namespace-scoped — filtered to only those carrying crdnames.LabelTenantID +// (§3: "select by label presence"). +func (r *Reconciler) SetupWithManager(mgr ctrl.Manager) error { + r.Client = mgr.GetClient() + return ctrl.NewControllerManagedBy(mgr). + For(&discoveryv1.EndpointSlice{}, builder.WithPredicates(predicate.NewPredicateFuncs(hasTenantLabel))). + Complete(r) +} + +func hasTenantLabel(obj client.Object) bool { + _, ok := obj.GetLabels()[crdnames.LabelTenantID] + return ok +} + +// RunSweeper blocks, calling store.Sweep on a fixed interval tick until ctx +// is done — the periodic mechanism that actually acts on expired teardown +// grace periods (see Store.Sweep's own doc comment for why this has to be +// polling-driven rather than reactive: VRF-level teardown is an aggregate +// condition over potentially many routes, not a single watched object's own +// transition). Mirrors cmd/galactic-router's GC ticker goroutine in shape. +// +// Callers must not start this until the manager's informer cache has +// synced and Store.Inventory has run — see Store.Inventory's own doc +// comment. +func RunSweeper(ctx context.Context, store *Store, interval time.Duration) { + ticker := time.NewTicker(interval) + defer ticker.Stop() + for { + select { + case <-ctx.Done(): + return + case <-ticker.C: + store.Sweep(ctx, time.Now()) + } + } +} diff --git a/internal/ingresssidecar/controller_test.go b/internal/ingresssidecar/controller_test.go new file mode 100644 index 00000000..7af32c0e --- /dev/null +++ b/internal/ingresssidecar/controller_test.go @@ -0,0 +1,110 @@ +// Copyright 2026 Datum Cloud, Inc. +// +// SPDX-License-Identifier: AGPL-3.0-or-later + +package ingresssidecar + +import ( + "context" + "net" + "testing" + + discoveryv1 "k8s.io/api/discovery/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" + clientgoscheme "k8s.io/client-go/kubernetes/scheme" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + + "go.datum.net/galactic/internal/crdnames" +) + +func newTestScheme(t *testing.T) *runtime.Scheme { + t.Helper() + scheme := runtime.NewScheme() + if err := clientgoscheme.AddToScheme(scheme); err != nil { + t.Fatalf("add clientgoscheme: %v", err) + } + return scheme +} + +// TestReconcilerAppliesDesiredRoute verifies a straightforward reconcile of +// an existing, well-formed EndpointSlice installs its VRF and route. +func TestReconcilerAppliesDesiredRoute(t *testing.T) { + scheme := newTestScheme(t) + slice := readySlice("ns", "pod-a", "vpc1-att1", "fd00:1234::1", "fd00::abcd") + c := fake.NewClientBuilder().WithScheme(scheme).WithObjects(slice).Build() + + backend := newFakeBackend() + store := NewStore(backend, testGrace, nil) + r := &Reconciler{Client: c, Store: store} + + _, err := r.Reconcile(context.Background(), ctrl.Request{ + NamespacedName: types.NamespacedName{Namespace: "ns", Name: "pod-a"}, + }) + if err != nil { + t.Fatalf("Reconcile: %v", err) + } + if got := backend.vrfCount(); got != 1 { + t.Errorf("vrfCount = %d, want 1", got) + } + if got := backend.routeCount(); got != 1 { + t.Errorf("routeCount = %d, want 1", got) + } +} + +// TestReconcilerDeletedSliceStartsGrace verifies a Reconcile against a +// missing EndpointSlice marks its route absent (starting its teardown +// grace) rather than erroring or removing it synchronously. +func TestReconcilerDeletedSliceStartsGrace(t *testing.T) { + scheme := newTestScheme(t) + c := fake.NewClientBuilder().WithScheme(scheme).Build() + + backend := newFakeBackend() + store := NewStore(backend, testGrace, nil) + if err := store.SetDesired(context.Background(), "ns/pod-a", + &DesiredRoute{VPC: "vpc1", Prefix: mustPrefix(t, "fd00::1"), SID: net.ParseIP("fd00:99::1")}); err != nil { + t.Fatalf("seed SetDesired: %v", err) + } + r := &Reconciler{Client: c, Store: store} + + req := ctrl.Request{NamespacedName: types.NamespacedName{Namespace: "ns", Name: "pod-a"}} + if _, err := r.Reconcile(context.Background(), req); err != nil { + t.Fatalf("Reconcile: %v", err) + } + + // Not torn down synchronously -- still installed immediately after. + if got := backend.routeCount(); got != 1 { + t.Errorf("routeCount = %d, want 1 (grace not yet elapsed)", got) + } +} + +// TestReconcilerMalformedSliceDoesNotError verifies a selected-but-malformed +// EndpointSlice is dropped (logged, not retried forever) rather than +// returned as a Reconcile error. +func TestReconcilerMalformedSliceDoesNotError(t *testing.T) { + scheme := newTestScheme(t) + slice := &discoveryv1.EndpointSlice{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "ns", Name: "pod-a", + Labels: map[string]string{crdnames.LabelTenantID: "novalidseparator"}, + Annotations: map[string]string{crdnames.AnnotationTenantID: "novalidseparator"}, + }, + } + c := fake.NewClientBuilder().WithScheme(scheme).WithObjects(slice).Build() + + backend := newFakeBackend() + store := NewStore(backend, testGrace, nil) + r := &Reconciler{Client: c, Store: store} + + _, err := r.Reconcile(context.Background(), ctrl.Request{ + NamespacedName: types.NamespacedName{Namespace: "ns", Name: "pod-a"}, + }) + if err != nil { + t.Fatalf("Reconcile: want nil error for malformed-but-selected slice, got %v", err) + } + if got := backend.vrfCount(); got != 0 { + t.Errorf("vrfCount = %d, want 0", got) + } +} diff --git a/internal/ingresssidecar/desired.go b/internal/ingresssidecar/desired.go new file mode 100644 index 00000000..320e45e2 --- /dev/null +++ b/internal/ingresssidecar/desired.go @@ -0,0 +1,84 @@ +// Copyright 2026 Datum Cloud, Inc. +// +// SPDX-License-Identifier: AGPL-3.0-or-later + +package ingresssidecar + +import ( + "fmt" + "net" + + discoveryv1 "k8s.io/api/discovery/v1" + + "go.datum.net/galactic/internal/crdnames" +) + +// IsSelected reports whether slice carries the label this sidecar watches +// EndpointSlices through — presence of crdnames.LabelTenantID, per §3 of +// the plan ("select by label presence, group by its value"). Used both as +// the controller's watch predicate and by BuildDesiredRoute. +func IsSelected(slice *discoveryv1.EndpointSlice) bool { + if slice == nil { + return false + } + _, ok := slice.Labels[crdnames.LabelTenantID] + return ok +} + +// BuildDesiredRoute translates one EndpointSlice into the DesiredRoute this +// sidecar should converge toward. +// +// Returns (nil, nil) — "nothing to do, not an error" — when slice isn't one +// this sidecar owns (IsSelected is false) or hasn't picked up its SID +// annotation yet: crdnames.AnnotationSID is only set once the pod's hosting +// node's BGPRouter has SRv6Locator/NodeID configured (see that constant's +// own doc comment), so a freshly-published EndpointSlice can legitimately +// have the tenant label but no SID yet, pending a later update. +// +// Returns (nil, err) for a slice that IS selected but malformed in a way +// that indicates a real problem worth logging — a bad tenant identifier, an +// unparseable SID/address, or an unsupported (non-IPv6) AddressType, which +// per §3 should never occur for these backends. +func BuildDesiredRoute(slice *discoveryv1.EndpointSlice) (*DesiredRoute, error) { + if !IsSelected(slice) { + return nil, nil + } + + tenantID := slice.Annotations[crdnames.AnnotationTenantID] + vpc, _, ok := crdnames.ParseTenantIdentifier(tenantID) + if !ok { + return nil, fmt.Errorf("EndpointSlice %s/%s: malformed tenant identifier %q (annotation %s)", + slice.Namespace, slice.Name, tenantID, crdnames.AnnotationTenantID) + } + + sidStr, ok := slice.Annotations[crdnames.AnnotationSID] + if !ok || sidStr == "" { + return nil, nil + } + sid := net.ParseIP(sidStr) + if sid == nil { + return nil, fmt.Errorf("EndpointSlice %s/%s: invalid SID annotation %q", + slice.Namespace, slice.Name, sidStr) + } + + if slice.AddressType != discoveryv1.AddressTypeIPv6 { + return nil, fmt.Errorf( + "EndpointSlice %s/%s: unsupported AddressType %q — only IPv6 backends are published (§3 of the plan)", + slice.Namespace, slice.Name, slice.AddressType) + } + if len(slice.Endpoints) == 0 || len(slice.Endpoints[0].Addresses) == 0 { + return nil, fmt.Errorf("EndpointSlice %s/%s: no endpoint address", slice.Namespace, slice.Name) + } + addrStr := slice.Endpoints[0].Addresses[0] + addr := net.ParseIP(addrStr) + if addr == nil { + return nil, fmt.Errorf("EndpointSlice %s/%s: invalid endpoint address %q", + slice.Namespace, slice.Name, addrStr) + } + + return &DesiredRoute{ + VPC: vpc, + Prefix: &net.IPNet{IP: addr, Mask: net.CIDRMask(128, 128)}, + SID: sid, + }, nil +} diff --git a/internal/ingresssidecar/desired_test.go b/internal/ingresssidecar/desired_test.go new file mode 100644 index 00000000..e52d2846 --- /dev/null +++ b/internal/ingresssidecar/desired_test.go @@ -0,0 +1,105 @@ +// Copyright 2026 Datum Cloud, Inc. +// +// SPDX-License-Identifier: AGPL-3.0-or-later + +package ingresssidecar + +import ( + "testing" + + discoveryv1 "k8s.io/api/discovery/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + "go.datum.net/galactic/internal/crdnames" +) + +func readySlice(namespace, name, tenantID, sid, addr string) *discoveryv1.EndpointSlice { + slice := &discoveryv1.EndpointSlice{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: namespace, + Name: name, + Labels: map[string]string{crdnames.LabelTenantID: tenantID}, + Annotations: map[string]string{ + crdnames.AnnotationTenantID: tenantID, + }, + }, + AddressType: discoveryv1.AddressTypeIPv6, + Endpoints: []discoveryv1.Endpoint{{ + Addresses: []string{addr}, + }}, + } + if sid != "" { + slice.Annotations[crdnames.AnnotationSID] = sid + } + return slice +} + +func TestBuildDesiredRouteNotSelected(t *testing.T) { + slice := &discoveryv1.EndpointSlice{} + got, err := BuildDesiredRoute(slice) + if err != nil || got != nil { + t.Fatalf("BuildDesiredRoute(unlabeled) = (%v, %v), want (nil, nil)", got, err) + } +} + +func TestBuildDesiredRouteNoSIDYet(t *testing.T) { + slice := readySlice("ns", "pod-a", "vpc1-att1", "", "fd00::1") + got, err := BuildDesiredRoute(slice) + if err != nil || got != nil { + t.Fatalf("BuildDesiredRoute(no SID) = (%v, %v), want (nil, nil)", got, err) + } +} + +func TestBuildDesiredRouteHappyPath(t *testing.T) { + slice := readySlice("ns", "pod-a", "vpc1-att1", "fd00:1234::1", "fd00::abcd") + got, err := BuildDesiredRoute(slice) + if err != nil { + t.Fatalf("BuildDesiredRoute: unexpected error: %v", err) + } + if got == nil { + t.Fatal("BuildDesiredRoute returned nil, want a DesiredRoute") + } + if got.VPC != "vpc1" { + t.Errorf("VPC = %q, want %q", got.VPC, "vpc1") + } + if got.Prefix.String() != "fd00::abcd/128" { + t.Errorf("Prefix = %q, want %q", got.Prefix.String(), "fd00::abcd/128") + } + if got.SID.String() != "fd00:1234::1" { + t.Errorf("SID = %q, want %q", got.SID.String(), "fd00:1234::1") + } +} + +func TestBuildDesiredRouteMalformedTenantID(t *testing.T) { + slice := readySlice("ns", "pod-a", "notenantsep", "fd00:1234::1", "fd00::abcd") + got, err := BuildDesiredRoute(slice) + if err == nil || got != nil { + t.Fatalf("BuildDesiredRoute(malformed tenant id) = (%v, %v), want (nil, error)", got, err) + } +} + +func TestBuildDesiredRouteInvalidSID(t *testing.T) { + slice := readySlice("ns", "pod-a", "vpc1-att1", "not-an-ip", "fd00::abcd") + got, err := BuildDesiredRoute(slice) + if err == nil || got != nil { + t.Fatalf("BuildDesiredRoute(invalid SID) = (%v, %v), want (nil, error)", got, err) + } +} + +func TestBuildDesiredRouteWrongAddressType(t *testing.T) { + slice := readySlice("ns", "pod-a", "vpc1-att1", "fd00:1234::1", "10.0.0.1") + slice.AddressType = discoveryv1.AddressTypeIPv4 + got, err := BuildDesiredRoute(slice) + if err == nil || got != nil { + t.Fatalf("BuildDesiredRoute(IPv4 AddressType) = (%v, %v), want (nil, error)", got, err) + } +} + +func TestBuildDesiredRouteNoEndpointAddress(t *testing.T) { + slice := readySlice("ns", "pod-a", "vpc1-att1", "fd00:1234::1", "") + slice.Endpoints[0].Addresses = nil + got, err := BuildDesiredRoute(slice) + if err == nil || got != nil { + t.Fatalf("BuildDesiredRoute(no address) = (%v, %v), want (nil, error)", got, err) + } +} diff --git a/internal/ingresssidecar/doc.go b/internal/ingresssidecar/doc.go new file mode 100644 index 00000000..dc5173af --- /dev/null +++ b/internal/ingresssidecar/doc.go @@ -0,0 +1,44 @@ +// Copyright 2026 Datum Cloud, Inc. +// +// SPDX-License-Identifier: AGPL-3.0-or-later + +// Package ingresssidecar implements #855's ingress sidecar: the second +// container in the shared Envoy Gateway fleet's pod, responsible only for +// VPC backend connectivity — Linux VRF device + SRv6 seg6 encap route +// lifecycle — never for xDS/EDS, health checking, or anything Envoy-facing. +// See docs/plans/855-ingress-sidecar-vpc-backend-connectivity.md for the +// full design; this doc comment only orients the code. +// +// Desired state comes from a single source: a cluster-scoped watch on +// discoveryv1.EndpointSlice objects published per pod by galactic-cni +// (#854), selected by the crdnames.LabelTenantID label. That label's value +// — TenantIdentifier(vpc, vpcAttachment) — is used only to decide *which* +// slices this sidecar cares about; it is never the reconcile key for either +// kernel resource this package manages, because the two are keyed at two +// different granularities than the tenant label: +// +// - VRF device: one per VPC (crdnames.ParseTenantIdentifier's vpc half), +// shared by every attachment of that VPC present on this node — +// matching the kernel VRF's identity everywhere else in this codebase +// (internal/plumbing/vrf). +// - SRv6 egress route: one per pod (per EndpointSlice) — pods of the same +// tenant on different nodes carry different SIDs, so there is no +// tenant-aggregate route to install. +// +// Package layout: +// +// - model.go — DesiredRoute, the one value this package's desired-state +// translation produces. +// - desired.go — BuildDesiredRoute: EndpointSlice → DesiredRoute. +// - backend.go — Backend, the kernel-facing interface Store converges +// against, and kernelBackend, its production implementation wired to +// internal/plumbing/vrf and internal/plumbing/srv6 directly. +// - store.go — Store: the mutex-protected, two-granularity, grace-period- +// aware reconciler at this package's core. Mirrors internal/gateway's +// Engine and internal/runtime/gobgp's GoBGPRuntime in shape. +// - metrics.go — Prometheus metrics (§6 of the plan). +// - controller.go — Reconciler: the thin controller-runtime glue that +// turns EndpointSlice watch events into Store.SetDesired calls, plus +// the startup-inventory and periodic-sweep wiring cmd/galactic-vrf's +// root.go drives. +package ingresssidecar diff --git a/internal/ingresssidecar/fakebackend_test.go b/internal/ingresssidecar/fakebackend_test.go new file mode 100644 index 00000000..10918238 --- /dev/null +++ b/internal/ingresssidecar/fakebackend_test.go @@ -0,0 +1,148 @@ +// Copyright 2026 Datum Cloud, Inc. +// +// SPDX-License-Identifier: AGPL-3.0-or-later + +package ingresssidecar + +import ( + "fmt" + "net" + "sync" +) + +// fakeBackend is an in-memory Backend for tests — no real kernel calls, so +// these tests exercise Store's own convergence/grace-period logic in +// isolation from internal/plumbing/vrf and internal/plumbing/srv6, which +// need CAP_NET_ADMIN and a real netlink socket (see §7 of the plan: the +// real-kernel verification pass those packages need is a separate, +// required pre-merge step, not something a unit test can stand in for). +type fakeBackend struct { + mu sync.Mutex + + nextTableID uint32 + vrfs map[string]uint32 // vpc -> tableID + routes map[string]routeRecord // "vpc/prefix" -> record + calls []string // ordered call log, for assertions + + // failEnsureVRF/failEnsureRoute/failRemoveVRF/failRemoveRoute, if set, + // make the matching method return this error instead of succeeding. + failEnsureVRF error + failEnsureRoute error + failRemoveVRF error + failRemoveRoute error +} + +type routeRecord struct { + prefix *net.IPNet + sid net.IP +} + +func newFakeBackend() *fakeBackend { + return &fakeBackend{ + nextTableID: 1, + vrfs: make(map[string]uint32), + routes: make(map[string]routeRecord), + } +} + +func (f *fakeBackend) EnsureVRF(vpc string) (uint32, error) { + f.mu.Lock() + defer f.mu.Unlock() + f.calls = append(f.calls, "EnsureVRF:"+vpc) + if f.failEnsureVRF != nil { + return 0, f.failEnsureVRF + } + if id, ok := f.vrfs[vpc]; ok { + return id, nil + } + id := f.nextTableID + f.nextTableID++ + f.vrfs[vpc] = id + return id, nil +} + +func (f *fakeBackend) RemoveVRF(vpc string) error { + f.mu.Lock() + defer f.mu.Unlock() + f.calls = append(f.calls, "RemoveVRF:"+vpc) + if f.failRemoveVRF != nil { + return f.failRemoveVRF + } + delete(f.vrfs, vpc) + return nil +} + +func (f *fakeBackend) EnsureRoute(prefix *net.IPNet, sid net.IP, tableID uint32) error { + f.mu.Lock() + defer f.mu.Unlock() + key := fmt.Sprintf("%d/%s", tableID, prefix) + f.calls = append(f.calls, "EnsureRoute:"+key) + if f.failEnsureRoute != nil { + return f.failEnsureRoute + } + f.routes[key] = routeRecord{prefix: prefix, sid: sid} + return nil +} + +func (f *fakeBackend) RemoveRoute(prefix *net.IPNet, tableID uint32) error { + f.mu.Lock() + defer f.mu.Unlock() + key := fmt.Sprintf("%d/%s", tableID, prefix) + f.calls = append(f.calls, "RemoveRoute:"+key) + if f.failRemoveRoute != nil { + return f.failRemoveRoute + } + delete(f.routes, key) + return nil +} + +func (f *fakeBackend) ListVRFs() ([]VRFInfo, error) { + f.mu.Lock() + defer f.mu.Unlock() + infos := make([]VRFInfo, 0, len(f.vrfs)) + for vpc, id := range f.vrfs { + infos = append(infos, VRFInfo{VPC: vpc, TableID: id}) + } + return infos, nil +} + +func (f *fakeBackend) ListRoutes(tableID uint32) ([]RouteInfo, error) { + f.mu.Lock() + defer f.mu.Unlock() + var infos []RouteInfo + prefixStr := fmt.Sprintf("%d/", tableID) + for key, rec := range f.routes { + if len(key) > len(prefixStr) && key[:len(prefixStr)] == prefixStr { + infos = append(infos, RouteInfo{Prefix: rec.prefix, SID: rec.sid}) + } + } + return infos, nil +} + +// routeCount/vrfCount let tests assert on the fake's installed state +// directly, independent of Store's own bookkeeping. +func (f *fakeBackend) routeCount() int { + f.mu.Lock() + defer f.mu.Unlock() + return len(f.routes) +} + +func (f *fakeBackend) vrfCount() int { + f.mu.Lock() + defer f.mu.Unlock() + return len(f.vrfs) +} + +// seedRoute directly inserts a route into the fake's kernel-side state +// without going through Store — used to simulate pre-existing kernel state +// for Store.Inventory tests. +func (f *fakeBackend) seedRoute(vpc string, tableID uint32, prefix *net.IPNet, sid net.IP) { + f.mu.Lock() + defer f.mu.Unlock() + f.vrfs[vpc] = tableID + if tableID >= f.nextTableID { + f.nextTableID = tableID + 1 + } + key := fmt.Sprintf("%d/%s", tableID, prefix) + f.routes[key] = routeRecord{prefix: prefix, sid: sid} +} diff --git a/internal/ingresssidecar/metrics.go b/internal/ingresssidecar/metrics.go new file mode 100644 index 00000000..0e532be5 --- /dev/null +++ b/internal/ingresssidecar/metrics.go @@ -0,0 +1,69 @@ +// Copyright 2026 Datum Cloud, Inc. +// +// SPDX-License-Identifier: AGPL-3.0-or-later + +package ingresssidecar + +import "github.com/prometheus/client_golang/prometheus" + +const metricsNamespace = "galactic_ingress_sidecar" + +// Metrics is this sidecar's Prometheus surface — §6 of the plan: "active +// VRF (per-VPC) count, active route (per-pod) count, reconcile error rate, +// reconcile latency, teardown-grace-period queue depth for both." Mirrors +// internal/gateway's PrometheusTelemetryEmitter in shape: build once via +// NewMetrics, MustRegister once at startup, then pass into NewStore. +type Metrics struct { + VRFActive prometheus.Gauge + RouteActive prometheus.Gauge + VRFPending prometheus.Gauge + RoutePending prometheus.Gauge + ReconcileErrs *prometheus.CounterVec + ReconcileTime prometheus.Histogram +} + +// NewMetrics builds a fresh, unregistered Metrics. Call MustRegister once +// at process startup. +func NewMetrics() *Metrics { + return &Metrics{ + VRFActive: prometheus.NewGauge(prometheus.GaugeOpts{ + Namespace: metricsNamespace, + Name: "vrf_active", + Help: "Number of per-VPC VRF devices this sidecar currently has installed.", + }), + RouteActive: prometheus.NewGauge(prometheus.GaugeOpts{ + Namespace: metricsNamespace, + Name: "route_active", + Help: "Number of per-pod seg6 egress routes this sidecar currently has installed.", + }), + VRFPending: prometheus.NewGauge(prometheus.GaugeOpts{ + Namespace: metricsNamespace, + Name: "vrf_teardown_pending", + Help: "Number of VRF devices past their last live pod, waiting out their teardown grace period.", + }), + RoutePending: prometheus.NewGauge(prometheus.GaugeOpts{ + Namespace: metricsNamespace, + Name: "route_teardown_pending", + Help: "Number of routes whose EndpointSlice disappeared, waiting out their teardown grace period.", + }), + ReconcileErrs: prometheus.NewCounterVec(prometheus.CounterOpts{ + Namespace: metricsNamespace, + Name: "reconcile_errors_total", + Help: "Reconcile errors, by kind (ensure_vrf, ensure_route, remove_vrf, remove_route).", + }, []string{"kind"}), + ReconcileTime: prometheus.NewHistogram(prometheus.HistogramOpts{ + Namespace: metricsNamespace, + Name: "reconcile_duration_seconds", + Help: "Time taken by each Store.SetDesired call.", + Buckets: prometheus.DefBuckets, + }), + } +} + +// MustRegister registers every metric this type owns against reg. Panics on +// a duplicate registration — callers only ever do this once per process, at +// startup, same convention as internal/gateway.PrometheusTelemetryEmitter's +// own MustRegister. +func (m *Metrics) MustRegister(reg prometheus.Registerer) { + reg.MustRegister(m.VRFActive, m.RouteActive, m.VRFPending, m.RoutePending, m.ReconcileErrs, m.ReconcileTime) +} diff --git a/internal/ingresssidecar/model.go b/internal/ingresssidecar/model.go new file mode 100644 index 00000000..3da4638b --- /dev/null +++ b/internal/ingresssidecar/model.go @@ -0,0 +1,29 @@ +// Copyright 2026 Datum Cloud, Inc. +// +// SPDX-License-Identifier: AGPL-3.0-or-later + +package ingresssidecar + +import "net" + +// DesiredRoute is the desired SRv6 egress-route state for one pod, derived +// from a single EndpointSlice — see BuildDesiredRoute. Store.SetDesired +// keys each DesiredRoute by that EndpointSlice's own namespace/name; VPC is +// the separate, coarser key its VRF device lifecycle rolls up to (see +// Store's doc comment and §1 of the plan). +type DesiredRoute struct { + // VPC is the base62 VPC identifier this pod is attached to, recovered + // via crdnames.ParseTenantIdentifier — never the combined tenant + // identifier (vpc-vpcAttachment) itself, which is not a value any + // kernel-side primitive in this codebase accepts. + VPC string + // Prefix is the pod's own address as a host route (/128 for the + // IPv6-only backends this sidecar handles per §3 of the plan) — never a + // tenant/subnet aggregate, since none exists: SIDs vary per hosting + // node, so pods of the same tenant on different nodes need independent + // routes even though they share a VPC. + Prefix *net.IPNet + // SID is the pod's own computed SRv6 uSID — the seg6 encap gateway + // address for Prefix's route. + SID net.IP +} diff --git a/internal/ingresssidecar/store.go b/internal/ingresssidecar/store.go new file mode 100644 index 00000000..2d295abc --- /dev/null +++ b/internal/ingresssidecar/store.go @@ -0,0 +1,307 @@ +// Copyright 2026 Datum Cloud, Inc. +// +// SPDX-License-Identifier: AGPL-3.0-or-later + +package ingresssidecar + +import ( + "context" + "fmt" + "log/slog" + "net" + "sync" + "time" +) + +// routeState tracks one pod's route lifecycle, keyed by its EndpointSlice's +// namespace/name. +type routeState struct { + vpc string + prefix *net.IPNet + sid net.IP + installed bool + // absentSince is the zero Time while this route is desired. SetDesired + // sets it the moment a nil desired value is first observed for this + // key, and clears it again if the route is reactivated before Sweep + // tears it down — see SetDesired and Sweep. + absentSince time.Time +} + +// vrfState tracks one VPC's VRF device lifecycle, keyed by vpc. +type vrfState struct { + tableID uint32 + installed bool + // absentSince is the zero Time while at least one route still + // references this VPC (installed or itself still within its own grace + // period — see Sweep). Only once every such route is gone does this + // VPC's own teardown grace period start. + absentSince time.Time +} + +// Store is the in-process desired/applied-state reconciler for this +// sidecar's two granularities (§1 of the plan): route lifecycle keyed per +// pod, VRF lifecycle keyed per VPC and rolled up from every route +// referencing it. Mirrors internal/gateway's Engine and +// internal/runtime/gobgp's GoBGPRuntime in shape — a mutex-protected map of +// applied state, converged via Backend calls — except teardown here is +// intentionally delayed by a grace period rather than applied synchronously +// (§9 item 1 of the plan's teardown-race decision), so SetDesired/Sweep +// replace a single Reconcile/Apply call: SetDesired applies "up" transitions +// immediately and only starts a clock on "down" ones; Sweep is what actually +// acts once that clock expires. +type Store struct { + mu sync.Mutex + backend Backend + grace time.Duration + metrics *Metrics + + routes map[string]*routeState + vrfs map[string]*vrfState +} + +// NewStore returns a Store that converges against backend, delaying +// teardown of any route or VRF by grace after it drops out of desired +// state. metrics may be nil (tests commonly pass nil; production callers +// always pass a real *Metrics). +func NewStore(backend Backend, grace time.Duration, metrics *Metrics) *Store { + return &Store{ + backend: backend, + grace: grace, + metrics: metrics, + routes: make(map[string]*routeState), + vrfs: make(map[string]*vrfState), + } +} + +// SetDesired updates the desired state for the route identified by key — +// an EndpointSlice's namespace/name (see Reconciler). desired == nil means +// the EndpointSlice is gone or no longer selected (BuildDesiredRoute +// returned nil for a not-yet-ready one, or the object was deleted): this +// starts (or leaves running) that route's teardown grace period rather than +// removing it immediately. desired != nil ensures the route's VRF and its +// own seg6 route exist immediately — no delay on the way up, only on the +// way down, the asymmetry §9 item 1 of the plan calls for. A route +// reappearing before its own grace period elapses, or a VPC gaining a new +// route before its VRF's grace period elapses, cancels that pending +// teardown outright. +func (s *Store) SetDesired(ctx context.Context, key string, desired *DesiredRoute) (err error) { + if s.metrics != nil { + timer := prometheusTimer(s.metrics) + defer func() { timer() }() + } + + s.mu.Lock() + defer s.mu.Unlock() + + if desired == nil { + if r, ok := s.routes[key]; ok && r.absentSince.IsZero() { + r.absentSince = time.Now() + } + return nil + } + + v, ok := s.vrfs[desired.VPC] + if !ok { + v = &vrfState{} + s.vrfs[desired.VPC] = v + } + v.absentSince = time.Time{} // this VPC has a live pod again + + if !v.installed { + tableID, verr := s.backend.EnsureVRF(desired.VPC) + if verr != nil { + s.countError("ensure_vrf") + return fmt.Errorf("ensure VRF for vpc %s: %w", desired.VPC, verr) + } + v.installed = true + v.tableID = tableID + s.vrfActiveDelta(1) + } + + if rerr := s.backend.EnsureRoute(desired.Prefix, desired.SID, v.tableID); rerr != nil { + s.countError("ensure_route") + return fmt.Errorf("ensure route for %s: %w", desired.Prefix, rerr) + } + + r, ok := s.routes[key] + if !ok { + r = &routeState{} + s.routes[key] = r + } + r.vpc = desired.VPC + r.prefix = desired.Prefix + r.sid = desired.SID + r.absentSince = time.Time{} // (re)activated -- cancel any pending teardown + if !r.installed { + r.installed = true + s.routeActiveDelta(1) + } + return nil +} + +// Sweep advances every pending teardown whose grace period has elapsed as +// of now, removing kernel state and forgetting it. Routes are processed +// first; a VPC's own grace period only starts once Sweep observes no +// remaining route — installed or still within its own grace — referencing +// it, so the two timers can never overlap: a VPC is never torn down while +// any of its routes still might come back. Call this periodically (see +// RunSweeper), never reactively — VRF-level teardown is an aggregate +// condition over potentially many routes, not a single watched object's own +// transition. +func (s *Store) Sweep(ctx context.Context, now time.Time) { + s.mu.Lock() + defer s.mu.Unlock() + + pendingRoutes, pendingVRFs := 0, 0 + + liveVPCs := make(map[string]struct{}, len(s.vrfs)) + for key, r := range s.routes { + if r.absentSince.IsZero() { + liveVPCs[r.vpc] = struct{}{} + continue + } + if now.Sub(r.absentSince) < s.grace { + liveVPCs[r.vpc] = struct{}{} // still in its own grace -- keeps the VPC live too + pendingRoutes++ + continue + } + if r.installed { + v, ok := s.vrfs[r.vpc] + if !ok { + slog.Error("ingresssidecar: sweep found route with no tracked VRF", "key", key, "vpc", r.vpc) + } else if err := s.backend.RemoveRoute(r.prefix, v.tableID); err != nil { + s.countError("remove_route") + slog.Error("ingresssidecar: remove route", "key", key, "vpc", r.vpc, "error", err) + liveVPCs[r.vpc] = struct{}{} // keep the VPC alive; retry next sweep + pendingRoutes++ + continue + } + s.routeActiveDelta(-1) + } + delete(s.routes, key) + } + + for vpc, v := range s.vrfs { + if _, live := liveVPCs[vpc]; live { + v.absentSince = time.Time{} + continue + } + if v.absentSince.IsZero() { + v.absentSince = now + pendingVRFs++ + continue + } + if now.Sub(v.absentSince) < s.grace { + pendingVRFs++ + continue + } + if v.installed { + if err := s.backend.RemoveVRF(vpc); err != nil { + s.countError("remove_vrf") + slog.Error("ingresssidecar: remove VRF", "vpc", vpc, "error", err) + pendingVRFs++ + continue + } + s.vrfActiveDelta(-1) + } + delete(s.vrfs, vpc) + } + + if s.metrics != nil { + s.metrics.RoutePending.Set(float64(pendingRoutes)) + s.metrics.VRFPending.Set(float64(pendingVRFs)) + } +} + +// Inventory seeds Store with every Galactic-managed VRF device (and its +// currently-installed seg6 routes) already present on the host at process +// start — §9 item 2 of the plan's startup-reconcile-safety decision. +// +// Call this once, after the manager's caches have synced (so every +// EndpointSlice existing at boot has already been through SetDesired via +// the controller's own initial reconcile pass — see Reconciler) but before +// the first Sweep runs. A VPC/route already known by that point is left +// alone: a live EndpointSlice's reconcile beat Inventory here, so its +// absentSince is already clear. Anything Inventory itself has to seed is, +// by construction, missing that reconcile — either a VPC/pod truly orphaned +// while this sidecar was down, or one whose EndpointSlice hasn't reconciled +// yet for some other reason — so it's seeded with an ordinary grace period +// starting now rather than torn down on sight (giving a slightly late +// EndpointSlice reconcile a chance to reclaim it) and rather than kept +// alive forever (the pre-#377-revision failure mode this decision exists to +// avoid). +func (s *Store) Inventory(ctx context.Context, now time.Time) error { + infos, err := s.backend.ListVRFs() + if err != nil { + return fmt.Errorf("list existing VRFs: %w", err) + } + + s.mu.Lock() + defer s.mu.Unlock() + + for _, info := range infos { + v, ok := s.vrfs[info.VPC] + if !ok { + v = &vrfState{tableID: info.TableID, installed: true, absentSince: now} + s.vrfs[info.VPC] = v + s.vrfActiveDelta(1) + } + + routes, err := s.backend.ListRoutes(v.tableID) + if err != nil { + return fmt.Errorf("list existing routes for vpc %s (table %d): %w", info.VPC, v.tableID, err) + } + for _, route := range routes { + if s.routeKnownLocked(info.VPC, route.Prefix) { + continue // a live EndpointSlice's reconcile already claimed this one + } + key := fmt.Sprintf("boot/%s/%s", info.VPC, route.Prefix.String()) + s.routes[key] = &routeState{ + vpc: info.VPC, prefix: route.Prefix, sid: route.SID, + installed: true, absentSince: now, + } + s.routeActiveDelta(1) + } + } + return nil +} + +// routeKnownLocked reports whether some already-tracked route shares vpc +// and prefix with the given kernel route — i.e. it's not orphaned, a live +// EndpointSlice already claims it. Callers must hold s.mu. +func (s *Store) routeKnownLocked(vpc string, prefix *net.IPNet) bool { + for _, r := range s.routes { + if r.vpc == vpc && r.prefix.String() == prefix.String() { + return true + } + } + return false +} + +func (s *Store) countError(kind string) { + if s.metrics != nil { + s.metrics.ReconcileErrs.WithLabelValues(kind).Inc() + } +} + +// vrfActiveDelta and routeActiveDelta adjust the vrf_active/route_active +// gauges by delta, no-oping if metrics weren't configured (tests commonly +// pass nil — see NewStore). +func (s *Store) vrfActiveDelta(delta float64) { + if s.metrics != nil { + s.metrics.VRFActive.Add(delta) + } +} + +func (s *Store) routeActiveDelta(delta float64) { + if s.metrics != nil { + s.metrics.RouteActive.Add(delta) + } +} + +// prometheusTimer starts a wall-clock timer and returns a function that, on +// its own call, records the elapsed duration against m.ReconcileTime. +func prometheusTimer(m *Metrics) func() { + start := time.Now() + return func() { m.ReconcileTime.Observe(time.Since(start).Seconds()) } +} diff --git a/internal/ingresssidecar/store_test.go b/internal/ingresssidecar/store_test.go new file mode 100644 index 00000000..5923f444 --- /dev/null +++ b/internal/ingresssidecar/store_test.go @@ -0,0 +1,262 @@ +// Copyright 2026 Datum Cloud, Inc. +// +// SPDX-License-Identifier: AGPL-3.0-or-later + +package ingresssidecar + +import ( + "context" + "net" + "testing" + "time" +) + +func mustPrefix(t *testing.T, s string) *net.IPNet { + t.Helper() + ip := net.ParseIP(s) + if ip == nil { + t.Fatalf("bad IP %q", s) + } + return &net.IPNet{IP: ip, Mask: net.CIDRMask(128, 128)} +} + +const testGrace = 10 * time.Second + +// TestStoreRouteAndVRFAppear verifies a pod's first appearance creates both +// its VRF (per §1: one per VPC) and its own route. +func TestStoreRouteAndVRFAppear(t *testing.T) { + ctx := context.Background() + backend := newFakeBackend() + store := NewStore(backend, testGrace, nil) + + desired := &DesiredRoute{VPC: "vpc1", Prefix: mustPrefix(t, "fd00::1"), SID: net.ParseIP("fd00:99::1")} + if err := store.SetDesired(ctx, "ns/pod-a", desired); err != nil { + t.Fatalf("SetDesired: %v", err) + } + + if got := backend.vrfCount(); got != 1 { + t.Errorf("vrfCount = %d, want 1", got) + } + if got := backend.routeCount(); got != 1 { + t.Errorf("routeCount = %d, want 1", got) + } +} + +// TestStoreSecondAttachmentSharesVRF verifies a second pod on the same VPC +// (a different attachment) reuses the existing VRF rather than creating a +// second one, and doesn't disturb the first pod's own route — the core +// claim of §1's VRF-granularity correction. +func TestStoreSecondAttachmentSharesVRF(t *testing.T) { + ctx := context.Background() + backend := newFakeBackend() + store := NewStore(backend, testGrace, nil) + + first := &DesiredRoute{VPC: "vpc1", Prefix: mustPrefix(t, "fd00::1"), SID: net.ParseIP("fd00:99::1")} + second := &DesiredRoute{VPC: "vpc1", Prefix: mustPrefix(t, "fd00::2"), SID: net.ParseIP("fd00:99::2")} + + if err := store.SetDesired(ctx, "ns/pod-a", first); err != nil { + t.Fatalf("SetDesired(pod-a): %v", err) + } + if err := store.SetDesired(ctx, "ns/pod-b", second); err != nil { + t.Fatalf("SetDesired(pod-b): %v", err) + } + + if got := backend.vrfCount(); got != 1 { + t.Errorf("vrfCount = %d, want 1 (shared VRF)", got) + } + if got := backend.routeCount(); got != 2 { + t.Errorf("routeCount = %d, want 2 (independent routes)", got) + } + + // Removing pod-b's route must not touch pod-a's, or the shared VRF. + if err := store.SetDesired(ctx, "ns/pod-b", nil); err != nil { + t.Fatalf("SetDesired(pod-b, nil): %v", err) + } + store.Sweep(ctx, time.Now().Add(2*testGrace)) + + if got := backend.vrfCount(); got != 1 { + t.Errorf("after pod-b removal: vrfCount = %d, want 1 (pod-a still live)", got) + } + if got := backend.routeCount(); got != 1 { + t.Errorf("after pod-b removal: routeCount = %d, want 1 (pod-a's route untouched)", got) + } +} + +// TestStoreRouteTeardownGrace verifies a route is not removed before its +// grace period elapses, and is removed once it has. +func TestStoreRouteTeardownGrace(t *testing.T) { + ctx := context.Background() + backend := newFakeBackend() + store := NewStore(backend, testGrace, nil) + + desired := &DesiredRoute{VPC: "vpc1", Prefix: mustPrefix(t, "fd00::1"), SID: net.ParseIP("fd00:99::1")} + if err := store.SetDesired(ctx, "ns/pod-a", desired); err != nil { + t.Fatalf("SetDesired: %v", err) + } + if err := store.SetDesired(ctx, "ns/pod-a", nil); err != nil { + t.Fatalf("SetDesired(nil): %v", err) + } + + // Sweep well before the grace period elapses: route (and its VRF) + // must still be installed. + store.Sweep(ctx, time.Now().Add(1*time.Second)) + if got := backend.routeCount(); got != 1 { + t.Errorf("mid-grace: routeCount = %d, want 1 (not yet torn down)", got) + } + if got := backend.vrfCount(); got != 1 { + t.Errorf("mid-grace: vrfCount = %d, want 1 (route's own grace still pending)", got) + } + + // Sweep well after: both should be gone. The VRF's own grace only + // starts once the route is actually swept, so sweep twice with time + // advanced far enough past two grace periods. + store.Sweep(ctx, time.Now().Add(2*testGrace)) + store.Sweep(ctx, time.Now().Add(4*testGrace)) + if got := backend.routeCount(); got != 0 { + t.Errorf("post-grace: routeCount = %d, want 0", got) + } + if got := backend.vrfCount(); got != 0 { + t.Errorf("post-grace: vrfCount = %d, want 0 (last pod gone)", got) + } +} + +// TestStoreVRFOutlivesRouteGrace verifies the VRF's own teardown never +// fires while any of its routes are still within their own grace period — +// §9 item 1's "must not overlap" requirement. +func TestStoreVRFOutlivesRouteGrace(t *testing.T) { + ctx := context.Background() + backend := newFakeBackend() + store := NewStore(backend, testGrace, nil) + + desired := &DesiredRoute{VPC: "vpc1", Prefix: mustPrefix(t, "fd00::1"), SID: net.ParseIP("fd00:99::1")} + if err := store.SetDesired(ctx, "ns/pod-a", desired); err != nil { + t.Fatalf("SetDesired: %v", err) + } + if err := store.SetDesired(ctx, "ns/pod-a", nil); err != nil { + t.Fatalf("SetDesired(nil): %v", err) + } + + // A single sweep just past the route's own grace: the route is removed + // in this same pass, but the VRF's grace clock only starts now — it + // must NOT be removed in this same sweep. + store.Sweep(ctx, time.Now().Add(testGrace+time.Millisecond)) + if got := backend.routeCount(); got != 0 { + t.Errorf("routeCount = %d, want 0", got) + } + if got := backend.vrfCount(); got != 1 { + t.Errorf("vrfCount = %d, want 1 (VRF grace must start only now, not fire in the same sweep)", got) + } +} + +// TestStoreReactivationCancelsTeardown verifies a route reappearing before +// its grace period elapses cancels the pending teardown. +func TestStoreReactivationCancelsTeardown(t *testing.T) { + ctx := context.Background() + backend := newFakeBackend() + store := NewStore(backend, testGrace, nil) + + desired := &DesiredRoute{VPC: "vpc1", Prefix: mustPrefix(t, "fd00::1"), SID: net.ParseIP("fd00:99::1")} + if err := store.SetDesired(ctx, "ns/pod-a", desired); err != nil { + t.Fatalf("SetDesired: %v", err) + } + if err := store.SetDesired(ctx, "ns/pod-a", nil); err != nil { + t.Fatalf("SetDesired(nil): %v", err) + } + // Reactivate before any sweep has torn it down. + if err := store.SetDesired(ctx, "ns/pod-a", desired); err != nil { + t.Fatalf("SetDesired(reactivate): %v", err) + } + + store.Sweep(ctx, time.Now().Add(4*testGrace)) + if got := backend.routeCount(); got != 1 { + t.Errorf("routeCount = %d, want 1 (reactivation should have cancelled teardown)", got) + } + if got := backend.vrfCount(); got != 1 { + t.Errorf("vrfCount = %d, want 1", got) + } +} + +// TestStoreInventorySeedsOrphan verifies Inventory picks up a kernel VRF/ +// route with no corresponding tracked state and gives it a fresh grace +// period, rather than tearing it down immediately or ignoring it forever. +func TestStoreInventorySeedsOrphan(t *testing.T) { + ctx := context.Background() + backend := newFakeBackend() + store := NewStore(backend, testGrace, nil) + + prefix := mustPrefix(t, "fd00::1") + backend.seedRoute("vpc1", 5, prefix, net.ParseIP("fd00:99::1")) + + if err := store.Inventory(ctx, time.Now()); err != nil { + t.Fatalf("Inventory: %v", err) + } + + // Not torn down immediately. + store.Sweep(ctx, time.Now().Add(1*time.Second)) + if got := backend.routeCount(); got != 1 { + t.Errorf("mid-grace: routeCount = %d, want 1", got) + } + + // Torn down once its grace period (started at Inventory time) elapses, + // with no SetDesired call ever having reclaimed it. + store.Sweep(ctx, time.Now().Add(2*testGrace)) + store.Sweep(ctx, time.Now().Add(4*testGrace)) + if got := backend.routeCount(); got != 0 { + t.Errorf("post-grace: routeCount = %d, want 0 (truly orphaned)", got) + } + if got := backend.vrfCount(); got != 0 { + t.Errorf("post-grace: vrfCount = %d, want 0", got) + } +} + +// TestStoreInventorySkipsKnownRoute verifies Inventory does not +// double-track (and therefore does not risk deleting out from under) a +// route a live EndpointSlice's reconcile already claimed via SetDesired. +func TestStoreInventorySkipsKnownRoute(t *testing.T) { + ctx := context.Background() + backend := newFakeBackend() + store := NewStore(backend, testGrace, nil) + + prefix := mustPrefix(t, "fd00::1") + desired := &DesiredRoute{VPC: "vpc1", Prefix: prefix, SID: net.ParseIP("fd00:99::1")} + if err := store.SetDesired(ctx, "ns/pod-a", desired); err != nil { + t.Fatalf("SetDesired: %v", err) + } + + // Inventory runs after the reconcile already claimed this exact + // (vpc, prefix) — the fake backend now genuinely has it installed. + if err := store.Inventory(ctx, time.Now()); err != nil { + t.Fatalf("Inventory: %v", err) + } + + // A very long sweep must not remove it: it's tracked under the real + // key with no absentSince set, not under a synthetic boot/ key racing + // its own independent grace period. + store.Sweep(ctx, time.Now().Add(100*testGrace)) + if got := backend.routeCount(); got != 1 { + t.Errorf("routeCount = %d, want 1 (live route must never be torn down)", got) + } +} + +// TestStoreEnsureVRFErrorNotTracked verifies a failed EnsureVRF call +// doesn't leave a route marked installed with no backing kernel state. +func TestStoreEnsureVRFErrorNotTracked(t *testing.T) { + ctx := context.Background() + backend := newFakeBackend() + backend.failEnsureVRF = errTest + store := NewStore(backend, testGrace, nil) + + desired := &DesiredRoute{VPC: "vpc1", Prefix: mustPrefix(t, "fd00::1"), SID: net.ParseIP("fd00:99::1")} + if err := store.SetDesired(ctx, "ns/pod-a", desired); err == nil { + t.Fatal("SetDesired: want error, got nil") + } + if got := backend.routeCount(); got != 0 { + t.Errorf("routeCount = %d, want 0 (route must not be installed without its VRF)", got) + } +} + +var errTest = &testError{"boom"} + +type testError struct{ msg string } + +func (e *testError) Error() string { return e.msg } From f0e6a9f6a82254f413a68a1852a55a100f055d04 Mon Sep 17 00:00:00 2001 From: Peter Sprygada Date: Wed, 19 Aug 2026 16:32:37 -0400 Subject: [PATCH 2/5] fix(lint): dedupe goconst-flagged literals, drop unused readySlice params golangci-lint's goconst flags exact string literals repeated three or more times within a package when used as composite-literal field values or in comparisons (positional call arguments are exempt): - internal/config: gateway.go/router.go/vrf.go each bind their own metrics-port flag to a metrics_port Viper key and share an identical out-of-range Validate() message -- extract flagMetricsPort, keyMetricsPort, and errMetricsPortRange into config.go, and the matching test name/wantErr strings into config_test.go. - internal/crdnames/crdnames_test.go: the formatted (vpc, attachment) identifiers abc-def and 0000000jU-00G recur across BGPAdvertisementName/TenantIdentifier/ParseTenantIdentifier test tables -- join them from the existing raw-part constants instead of repeating the formatted string, and add testAttachmentBase62 for the 00G part that was also repeated. - internal/ingresssidecar: vpc1 and pod-a recur across controller_test.go/desired_test.go/store_test.go -- add testVPC1/ testPodName constants shared via store_test.go. Fixing pod-a's repetition also let unparam catch readySlice's namespace parameter (every caller passed "ns") and, once that value became the constant testPodName, its name parameter too (every caller passed testPodName) -- both are now hardcoded in the function body instead of threaded through as parameters. No behavior change; verified with go build/vet/test and a full golangci-lint run across the repo. Co-Authored-By: Claude Sonnet 5 --- internal/config/config.go | 13 +++++++++++ internal/config/gateway.go | 2 +- internal/config/router.go | 2 +- internal/config/vrf.go | 8 +++---- internal/config/vrf_test.go | 4 ++-- internal/crdnames/crdnames_test.go | 17 ++++++++++----- internal/ingresssidecar/controller_test.go | 12 +++++------ internal/ingresssidecar/desired_test.go | 25 ++++++++++++---------- internal/ingresssidecar/store_test.go | 25 ++++++++++++++-------- 9 files changed, 69 insertions(+), 39 deletions(-) diff --git a/internal/config/config.go b/internal/config/config.go index 044b593e..453dc88a 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -27,6 +27,19 @@ const ( LogLevelWarn = "warn" LogLevelWarning = "warning" LogLevelError = "error" + + // flagMetricsPort is the CLI flag name shared by RouterConfig, + // GatewayConfig, and VRFConfig's BindFlags -- each binds it to the same + // keyMetricsPort Viper key. + flagMetricsPort = "metrics-port" + + // keyMetricsPort is the Viper key each component's MetricsPort field + // resolves from (SetDefault/BindFlags/GetInt). + keyMetricsPort = "metrics_port" + + // errMetricsPortRange is the out-of-range validation message shared by + // RouterConfig, GatewayConfig, and VRFConfig's Validate. + errMetricsPortRange = "metrics port must be between 1 and 65535" ) // --- Shared CLI flag names -------------------------------------------- diff --git a/internal/config/gateway.go b/internal/config/gateway.go index 9b2533d5..b7836b0c 100644 --- a/internal/config/gateway.go +++ b/internal/config/gateway.go @@ -160,7 +160,7 @@ func (c *GatewayConfig) Validate() error { return fmt.Errorf("SRv6 address %q must be a native IPv6 address, not IPv4", c.SRv6Address) } if c.MetricsPort < 1 || c.MetricsPort > 65535 { - return errors.New("metrics port must be between 1 and 65535") + return errors.New(errMetricsPortRange) } if c.GRPCHealthPort < 1 || c.GRPCHealthPort > 65535 { return errors.New("grpc health port must be between 1 and 65535") diff --git a/internal/config/router.go b/internal/config/router.go index f5ed9cda..65c27bc6 100644 --- a/internal/config/router.go +++ b/internal/config/router.go @@ -167,7 +167,7 @@ func (c *RouterConfig) Validate() error { return errors.New("bgp listen port must be between 1 and 65535, or -1 for outbound-only mode") } if c.MetricsPort < 1 || c.MetricsPort > 65535 { - return errors.New("metrics port must be between 1 and 65535") + return errors.New(errMetricsPortRange) } if c.GRPCHealthPort < 1 || c.GRPCHealthPort > 65535 { return errors.New("grpc health port must be between 1 and 65535") diff --git a/internal/config/vrf.go b/internal/config/vrf.go index 34bac1e5..0dc778ea 100644 --- a/internal/config/vrf.go +++ b/internal/config/vrf.go @@ -82,7 +82,7 @@ func NewVRFConfig() *VRFConfig { v.SetEnvPrefix("GALACTIC_VRF") v.AutomaticEnv() - v.SetDefault("metrics_port", DefaultVRFMetricsPort) + v.SetDefault(keyMetricsPort, DefaultVRFMetricsPort) v.SetDefault("teardown_grace_period", DefaultVRFTeardownGracePeriod.String()) v.SetDefault("sweep_interval", DefaultVRFSweepInterval.String()) @@ -101,7 +101,7 @@ func (c *VRFConfig) BindFlags(flags *pflag.FlagSet) { flag string key string }{ - {"metrics-port", "metrics_port"}, + {flagMetricsPort, keyMetricsPort}, {"teardown-grace-period", "teardown_grace_period"}, {"sweep-interval", "sweep_interval"}, } @@ -118,7 +118,7 @@ func (c *VRFConfig) BindFlags(flags *pflag.FlagSet) { // readFields populates the exported fields from the current Viper state. func (c *VRFConfig) readFields() { - c.MetricsPort = c.v.GetInt("metrics_port") + c.MetricsPort = c.v.GetInt(keyMetricsPort) c.TeardownGracePeriod = c.v.GetDuration("teardown_grace_period") c.SweepInterval = c.v.GetDuration("sweep_interval") } @@ -126,7 +126,7 @@ func (c *VRFConfig) readFields() { // Validate checks that the resolved configuration is usable. func (c *VRFConfig) Validate() error { if c.MetricsPort < 1 || c.MetricsPort > 65535 { - return errors.New("metrics port must be between 1 and 65535") + return errors.New(errMetricsPortRange) } if c.TeardownGracePeriod <= 0 { return errors.New("teardown grace period must be positive") diff --git a/internal/config/vrf_test.go b/internal/config/vrf_test.go index 798694d1..230828c0 100644 --- a/internal/config/vrf_test.go +++ b/internal/config/vrf_test.go @@ -49,9 +49,9 @@ func TestVRFConfigValidate(t *testing.T) { wantErr string }{ { - name: "invalid metrics port", + name: testCaseInvalidMetricsPort, envVars: map[string]string{EnvVRFMetricsPort: "0"}, - wantErr: "metrics port must be between", + wantErr: testErrMetricsPortRange, }, { name: "non-positive grace period", diff --git a/internal/crdnames/crdnames_test.go b/internal/crdnames/crdnames_test.go index c6da8c05..15768982 100644 --- a/internal/crdnames/crdnames_test.go +++ b/internal/crdnames/crdnames_test.go @@ -47,6 +47,13 @@ const ( testVPCBase62 = "0000000jU" testAttachment = "def" testAttachmentBase62 = "00G" + + // testTenantID and testTenantIDBase62 are the already-joined + // (vpc, attachment) identifiers for the pairs above -- shared across + // the table-driven tests that need the formatted form rather than the + // two raw parts. + testTenantID = testVPC + "-" + testAttachment + testTenantIDBase62 = testVPCBase62 + "-" + testAttachmentBase62 ) func TestBGPVRFInstanceName(t *testing.T) { @@ -91,8 +98,8 @@ func TestBGPAdvertisementName(t *testing.T) { func TestTenantIdentifier(t *testing.T) { tests := []struct{ vpc, attachment, want string }{ - {testVPC, testAttachment, "abc-def"}, - {testVPCBase62, testAttachmentBase62, "0000000jU-00G"}, + {testVPC, testAttachment, testTenantID}, + {testVPCBase62, testAttachmentBase62, testTenantIDBase62}, } for _, tt := range tests { got := TenantIdentifier(tt.vpc, tt.attachment) @@ -128,8 +135,8 @@ func TestParseTenantIdentifier(t *testing.T) { wantAttachment string wantOK bool }{ - {"simple", "abc-def", testVPC, testAttachment, true}, - {"base62 vpc", "0000000jU-00G", testVPCBase62, "00G", true}, + {"simple", testTenantID, testVPC, testAttachment, true}, + {"base62 vpc", testTenantIDBase62, testVPCBase62, testAttachmentBase62, true}, {"no separator", "abcdef", "", "", false}, {"empty vpc", "-def", "", "", false}, {"empty attachment", "abc-", "", "", false}, @@ -152,7 +159,7 @@ func TestParseTenantIdentifier(t *testing.T) { func TestParseTenantIdentifierRoundTrip(t *testing.T) { tests := []struct{ vpc, attachment string }{ {testVPC, testAttachment}, - {testVPCBase62, "00G"}, + {testVPCBase62, testAttachmentBase62}, {"0", "0"}, {"Zz9", "aB0"}, } diff --git a/internal/ingresssidecar/controller_test.go b/internal/ingresssidecar/controller_test.go index 7af32c0e..869f0ad6 100644 --- a/internal/ingresssidecar/controller_test.go +++ b/internal/ingresssidecar/controller_test.go @@ -33,7 +33,7 @@ func newTestScheme(t *testing.T) *runtime.Scheme { // an existing, well-formed EndpointSlice installs its VRF and route. func TestReconcilerAppliesDesiredRoute(t *testing.T) { scheme := newTestScheme(t) - slice := readySlice("ns", "pod-a", "vpc1-att1", "fd00:1234::1", "fd00::abcd") + slice := readySlice("vpc1-att1", "fd00:1234::1", "fd00::abcd") c := fake.NewClientBuilder().WithScheme(scheme).WithObjects(slice).Build() backend := newFakeBackend() @@ -41,7 +41,7 @@ func TestReconcilerAppliesDesiredRoute(t *testing.T) { r := &Reconciler{Client: c, Store: store} _, err := r.Reconcile(context.Background(), ctrl.Request{ - NamespacedName: types.NamespacedName{Namespace: "ns", Name: "pod-a"}, + NamespacedName: types.NamespacedName{Namespace: "ns", Name: testPodName}, }) if err != nil { t.Fatalf("Reconcile: %v", err) @@ -64,12 +64,12 @@ func TestReconcilerDeletedSliceStartsGrace(t *testing.T) { backend := newFakeBackend() store := NewStore(backend, testGrace, nil) if err := store.SetDesired(context.Background(), "ns/pod-a", - &DesiredRoute{VPC: "vpc1", Prefix: mustPrefix(t, "fd00::1"), SID: net.ParseIP("fd00:99::1")}); err != nil { + &DesiredRoute{VPC: testVPC1, Prefix: mustPrefix(t, "fd00::1"), SID: net.ParseIP("fd00:99::1")}); err != nil { t.Fatalf("seed SetDesired: %v", err) } r := &Reconciler{Client: c, Store: store} - req := ctrl.Request{NamespacedName: types.NamespacedName{Namespace: "ns", Name: "pod-a"}} + req := ctrl.Request{NamespacedName: types.NamespacedName{Namespace: "ns", Name: testPodName}} if _, err := r.Reconcile(context.Background(), req); err != nil { t.Fatalf("Reconcile: %v", err) } @@ -87,7 +87,7 @@ func TestReconcilerMalformedSliceDoesNotError(t *testing.T) { scheme := newTestScheme(t) slice := &discoveryv1.EndpointSlice{ ObjectMeta: metav1.ObjectMeta{ - Namespace: "ns", Name: "pod-a", + Namespace: "ns", Name: testPodName, Labels: map[string]string{crdnames.LabelTenantID: "novalidseparator"}, Annotations: map[string]string{crdnames.AnnotationTenantID: "novalidseparator"}, }, @@ -99,7 +99,7 @@ func TestReconcilerMalformedSliceDoesNotError(t *testing.T) { r := &Reconciler{Client: c, Store: store} _, err := r.Reconcile(context.Background(), ctrl.Request{ - NamespacedName: types.NamespacedName{Namespace: "ns", Name: "pod-a"}, + NamespacedName: types.NamespacedName{Namespace: "ns", Name: testPodName}, }) if err != nil { t.Fatalf("Reconcile: want nil error for malformed-but-selected slice, got %v", err) diff --git a/internal/ingresssidecar/desired_test.go b/internal/ingresssidecar/desired_test.go index e52d2846..ed17c1a0 100644 --- a/internal/ingresssidecar/desired_test.go +++ b/internal/ingresssidecar/desired_test.go @@ -13,11 +13,14 @@ import ( "go.datum.net/galactic/internal/crdnames" ) -func readySlice(namespace, name, tenantID, sid, addr string) *discoveryv1.EndpointSlice { +// readySlice always builds its EndpointSlice as testPodName in the "ns" +// namespace -- every caller across this package's tests uses that same +// fixture identity, so there's no name/namespace parameter to vary. +func readySlice(tenantID, sid, addr string) *discoveryv1.EndpointSlice { slice := &discoveryv1.EndpointSlice{ ObjectMeta: metav1.ObjectMeta{ - Namespace: namespace, - Name: name, + Namespace: "ns", + Name: testPodName, Labels: map[string]string{crdnames.LabelTenantID: tenantID}, Annotations: map[string]string{ crdnames.AnnotationTenantID: tenantID, @@ -43,7 +46,7 @@ func TestBuildDesiredRouteNotSelected(t *testing.T) { } func TestBuildDesiredRouteNoSIDYet(t *testing.T) { - slice := readySlice("ns", "pod-a", "vpc1-att1", "", "fd00::1") + slice := readySlice("vpc1-att1", "", "fd00::1") got, err := BuildDesiredRoute(slice) if err != nil || got != nil { t.Fatalf("BuildDesiredRoute(no SID) = (%v, %v), want (nil, nil)", got, err) @@ -51,7 +54,7 @@ func TestBuildDesiredRouteNoSIDYet(t *testing.T) { } func TestBuildDesiredRouteHappyPath(t *testing.T) { - slice := readySlice("ns", "pod-a", "vpc1-att1", "fd00:1234::1", "fd00::abcd") + slice := readySlice("vpc1-att1", "fd00:1234::1", "fd00::abcd") got, err := BuildDesiredRoute(slice) if err != nil { t.Fatalf("BuildDesiredRoute: unexpected error: %v", err) @@ -59,8 +62,8 @@ func TestBuildDesiredRouteHappyPath(t *testing.T) { if got == nil { t.Fatal("BuildDesiredRoute returned nil, want a DesiredRoute") } - if got.VPC != "vpc1" { - t.Errorf("VPC = %q, want %q", got.VPC, "vpc1") + if got.VPC != testVPC1 { + t.Errorf("VPC = %q, want %q", got.VPC, testVPC1) } if got.Prefix.String() != "fd00::abcd/128" { t.Errorf("Prefix = %q, want %q", got.Prefix.String(), "fd00::abcd/128") @@ -71,7 +74,7 @@ func TestBuildDesiredRouteHappyPath(t *testing.T) { } func TestBuildDesiredRouteMalformedTenantID(t *testing.T) { - slice := readySlice("ns", "pod-a", "notenantsep", "fd00:1234::1", "fd00::abcd") + slice := readySlice("notenantsep", "fd00:1234::1", "fd00::abcd") got, err := BuildDesiredRoute(slice) if err == nil || got != nil { t.Fatalf("BuildDesiredRoute(malformed tenant id) = (%v, %v), want (nil, error)", got, err) @@ -79,7 +82,7 @@ func TestBuildDesiredRouteMalformedTenantID(t *testing.T) { } func TestBuildDesiredRouteInvalidSID(t *testing.T) { - slice := readySlice("ns", "pod-a", "vpc1-att1", "not-an-ip", "fd00::abcd") + slice := readySlice("vpc1-att1", "not-an-ip", "fd00::abcd") got, err := BuildDesiredRoute(slice) if err == nil || got != nil { t.Fatalf("BuildDesiredRoute(invalid SID) = (%v, %v), want (nil, error)", got, err) @@ -87,7 +90,7 @@ func TestBuildDesiredRouteInvalidSID(t *testing.T) { } func TestBuildDesiredRouteWrongAddressType(t *testing.T) { - slice := readySlice("ns", "pod-a", "vpc1-att1", "fd00:1234::1", "10.0.0.1") + slice := readySlice("vpc1-att1", "fd00:1234::1", "10.0.0.1") slice.AddressType = discoveryv1.AddressTypeIPv4 got, err := BuildDesiredRoute(slice) if err == nil || got != nil { @@ -96,7 +99,7 @@ func TestBuildDesiredRouteWrongAddressType(t *testing.T) { } func TestBuildDesiredRouteNoEndpointAddress(t *testing.T) { - slice := readySlice("ns", "pod-a", "vpc1-att1", "fd00:1234::1", "") + slice := readySlice("vpc1-att1", "fd00:1234::1", "") slice.Endpoints[0].Addresses = nil got, err := BuildDesiredRoute(slice) if err == nil || got != nil { diff --git a/internal/ingresssidecar/store_test.go b/internal/ingresssidecar/store_test.go index 5923f444..24da6c8f 100644 --- a/internal/ingresssidecar/store_test.go +++ b/internal/ingresssidecar/store_test.go @@ -22,6 +22,13 @@ func mustPrefix(t *testing.T, s string) *net.IPNet { const testGrace = 10 * time.Second +// testVPC1 and testPodName are the fixture VPC/pod-name values shared +// across this package's tests. +const ( + testVPC1 = "vpc1" + testPodName = "pod-a" +) + // TestStoreRouteAndVRFAppear verifies a pod's first appearance creates both // its VRF (per §1: one per VPC) and its own route. func TestStoreRouteAndVRFAppear(t *testing.T) { @@ -29,7 +36,7 @@ func TestStoreRouteAndVRFAppear(t *testing.T) { backend := newFakeBackend() store := NewStore(backend, testGrace, nil) - desired := &DesiredRoute{VPC: "vpc1", Prefix: mustPrefix(t, "fd00::1"), SID: net.ParseIP("fd00:99::1")} + desired := &DesiredRoute{VPC: testVPC1, Prefix: mustPrefix(t, "fd00::1"), SID: net.ParseIP("fd00:99::1")} if err := store.SetDesired(ctx, "ns/pod-a", desired); err != nil { t.Fatalf("SetDesired: %v", err) } @@ -51,8 +58,8 @@ func TestStoreSecondAttachmentSharesVRF(t *testing.T) { backend := newFakeBackend() store := NewStore(backend, testGrace, nil) - first := &DesiredRoute{VPC: "vpc1", Prefix: mustPrefix(t, "fd00::1"), SID: net.ParseIP("fd00:99::1")} - second := &DesiredRoute{VPC: "vpc1", Prefix: mustPrefix(t, "fd00::2"), SID: net.ParseIP("fd00:99::2")} + first := &DesiredRoute{VPC: testVPC1, Prefix: mustPrefix(t, "fd00::1"), SID: net.ParseIP("fd00:99::1")} + second := &DesiredRoute{VPC: testVPC1, Prefix: mustPrefix(t, "fd00::2"), SID: net.ParseIP("fd00:99::2")} if err := store.SetDesired(ctx, "ns/pod-a", first); err != nil { t.Fatalf("SetDesired(pod-a): %v", err) @@ -89,7 +96,7 @@ func TestStoreRouteTeardownGrace(t *testing.T) { backend := newFakeBackend() store := NewStore(backend, testGrace, nil) - desired := &DesiredRoute{VPC: "vpc1", Prefix: mustPrefix(t, "fd00::1"), SID: net.ParseIP("fd00:99::1")} + desired := &DesiredRoute{VPC: testVPC1, Prefix: mustPrefix(t, "fd00::1"), SID: net.ParseIP("fd00:99::1")} if err := store.SetDesired(ctx, "ns/pod-a", desired); err != nil { t.Fatalf("SetDesired: %v", err) } @@ -128,7 +135,7 @@ func TestStoreVRFOutlivesRouteGrace(t *testing.T) { backend := newFakeBackend() store := NewStore(backend, testGrace, nil) - desired := &DesiredRoute{VPC: "vpc1", Prefix: mustPrefix(t, "fd00::1"), SID: net.ParseIP("fd00:99::1")} + desired := &DesiredRoute{VPC: testVPC1, Prefix: mustPrefix(t, "fd00::1"), SID: net.ParseIP("fd00:99::1")} if err := store.SetDesired(ctx, "ns/pod-a", desired); err != nil { t.Fatalf("SetDesired: %v", err) } @@ -155,7 +162,7 @@ func TestStoreReactivationCancelsTeardown(t *testing.T) { backend := newFakeBackend() store := NewStore(backend, testGrace, nil) - desired := &DesiredRoute{VPC: "vpc1", Prefix: mustPrefix(t, "fd00::1"), SID: net.ParseIP("fd00:99::1")} + desired := &DesiredRoute{VPC: testVPC1, Prefix: mustPrefix(t, "fd00::1"), SID: net.ParseIP("fd00:99::1")} if err := store.SetDesired(ctx, "ns/pod-a", desired); err != nil { t.Fatalf("SetDesired: %v", err) } @@ -185,7 +192,7 @@ func TestStoreInventorySeedsOrphan(t *testing.T) { store := NewStore(backend, testGrace, nil) prefix := mustPrefix(t, "fd00::1") - backend.seedRoute("vpc1", 5, prefix, net.ParseIP("fd00:99::1")) + backend.seedRoute(testVPC1, 5, prefix, net.ParseIP("fd00:99::1")) if err := store.Inventory(ctx, time.Now()); err != nil { t.Fatalf("Inventory: %v", err) @@ -218,7 +225,7 @@ func TestStoreInventorySkipsKnownRoute(t *testing.T) { store := NewStore(backend, testGrace, nil) prefix := mustPrefix(t, "fd00::1") - desired := &DesiredRoute{VPC: "vpc1", Prefix: prefix, SID: net.ParseIP("fd00:99::1")} + desired := &DesiredRoute{VPC: testVPC1, Prefix: prefix, SID: net.ParseIP("fd00:99::1")} if err := store.SetDesired(ctx, "ns/pod-a", desired); err != nil { t.Fatalf("SetDesired: %v", err) } @@ -246,7 +253,7 @@ func TestStoreEnsureVRFErrorNotTracked(t *testing.T) { backend.failEnsureVRF = errTest store := NewStore(backend, testGrace, nil) - desired := &DesiredRoute{VPC: "vpc1", Prefix: mustPrefix(t, "fd00::1"), SID: net.ParseIP("fd00:99::1")} + desired := &DesiredRoute{VPC: testVPC1, Prefix: mustPrefix(t, "fd00::1"), SID: net.ParseIP("fd00:99::1")} if err := store.SetDesired(ctx, "ns/pod-a", desired); err == nil { t.Fatal("SetDesired: want error, got nil") } From d7bece36c5291a2d084e90c19d278896ccc5fce7 Mon Sep 17 00:00:00 2001 From: Peter Sprygada Date: Wed, 19 Aug 2026 20:24:42 -0400 Subject: [PATCH 3/5] fix(e2e): set CNI_ARGS on galactic-bgp's chained ADD in TestCNITapInterface testChainedGalacticBGP invokes galactic-bgp's cmdAdd with an ipv6_subnet carried through from the tap step's prevResult, so ipamResult.IPv6Subnet is non-nil and ADD takes the EndpointSlice-publish branch added in internal/cnibgp/ops_add.go. That branch requires nadpatch.ParsePodName(args.Args) to resolve a real K8S_POD_NAME, but the test never set CNI_ARGS on this step, so args.Args was empty and ADD failed with "publish EndpointSlice: no K8S_POD_NAME in CNI_ARGS \"\"". Set CNI_ARGS="IgnoreUnknown=1;K8S_POD_NAMESPACE=default;K8S_POD_NAME=" on the galactic-bgp step only, mirroring what Multus actually sets on a real invocation. The tap step above deliberately keeps CNI_ARGS unset (see its own comment) so nadpatch.VerifyChainComplete/AnnotateNAD skip NAD lookups the test doesn't set up a NAD for; galactic-bgp's own cmdAdd never calls either of those, so setting CNI_ARGS only here is safe. Verified locally: task test:e2e passes (TestCNITapInterface, including the chained galactic-bgp ADD/CHECK), plus task lint and task test:unit. Co-Authored-By: Claude Sonnet 5 --- tests/e2e/e2e_test.go | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/tests/e2e/e2e_test.go b/tests/e2e/e2e_test.go index 9d0e99dd..3d175996 100644 --- a/tests/e2e/e2e_test.go +++ b/tests/e2e/e2e_test.go @@ -399,7 +399,14 @@ func testChainedGalacticBGP(t *testing.T, podName string, tapResult map[string]a // Reuses the same netns/containerID/ifname galactic-tap's own step // (above) already set up: a real chained plugin sees the identical - // values across every plugin invoked for one CNI ADD. + // values across every plugin invoked for one CNI ADD. Unlike the tap + // step, this one does set CNI_ARGS: the tap config carries an + // ipv6_subnet, so ADD's ipamResult.IPv6Subnet is non-nil and cmdAdd + // takes the EndpointSlice-publish branch, which requires + // nadpatch.ParsePodName(args.Args) to resolve a real K8S_POD_NAME — + // Multus always sets this on a real invocation, so this mirrors that + // rather than exercising a standalone/manual-chain invocation the way + // the tap step above deliberately does for AnnotateNAD/VerifyChainComplete. bgpScript := `#!/bin/sh CNI_NETNS=/var/run/netns/e2e-tap-ns \ CNI_COMMAND=$1 \ From 85eb94a0b4dcdf8e366d8dd531cf1f20912e8198 Mon Sep 17 00:00:00 2001 From: Peter Sprygada Date: Thu, 20 Aug 2026 14:16:27 -0400 Subject: [PATCH 4/5] test(config): dedupe the newly-3x "valid config" literal (goconst) Rebasing this branch onto main brought gateway_test.go and nat66_test.go's own pre-existing "valid config" test-case name together with this branch's new vrf_test.go, pushing golangci-lint's goconst threshold from 2 (safe) to 3 (flagged) occurrences across the config package. Add testCaseValidConfig alongside this file's other shared test literals (config_test.go's existing dedupe pattern for exactly this situation) and use it in all three. Co-Authored-By: Claude Sonnet 5 --- internal/config/config_test.go | 1 + internal/config/gateway_test.go | 2 +- internal/config/nat66_test.go | 2 +- internal/config/vrf_test.go | 2 +- 4 files changed, 4 insertions(+), 3 deletions(-) diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 59af75bc..f06e7413 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -12,6 +12,7 @@ import "testing" // substring -- see config.go's identical "shared CLI flag names" // rationale for the production-code half of this pattern. const ( + testCaseValidConfig = "valid config" testCaseMissingNodeName = "missing node name" testErrNodeNameRequired = "node name is required" testCaseInvalidMetricsPort = "invalid metrics port" diff --git a/internal/config/gateway_test.go b/internal/config/gateway_test.go index 9337da68..ff7b78fc 100644 --- a/internal/config/gateway_test.go +++ b/internal/config/gateway_test.go @@ -123,7 +123,7 @@ func TestGatewayConfigValidate(t *testing.T) { wantErr: testErrGRPCHealthPortRange, }, { - name: "valid config", + name: testCaseValidConfig, envVars: map[string]string{ EnvGatewayNodeName: testGatewayNodeName, EnvGatewayPublicInterface: testGatewayIface, diff --git a/internal/config/nat66_test.go b/internal/config/nat66_test.go index 431ffa37..7e4ab759 100644 --- a/internal/config/nat66_test.go +++ b/internal/config/nat66_test.go @@ -164,7 +164,7 @@ func TestNAT66ConfigValidate(t *testing.T) { wantErr: testErrGRPCHealthPortRange, }, { - name: "valid config", + name: testCaseValidConfig, envVars: map[string]string{ EnvNAT66NodeName: testNAT66NodeName, EnvNAT66UplinkInterface: testNAT66Iface, diff --git a/internal/config/vrf_test.go b/internal/config/vrf_test.go index 230828c0..9249dddd 100644 --- a/internal/config/vrf_test.go +++ b/internal/config/vrf_test.go @@ -72,7 +72,7 @@ func TestVRFConfigValidate(t *testing.T) { wantErr: "sweep interval must not be greater than", }, { - name: "valid config", + name: testCaseValidConfig, envVars: map[string]string{}, wantErr: "", }, From 1eac52d68c97d6e01999cf17edf48cbb8ed7f708 Mon Sep 17 00:00:00 2001 From: Peter Sprygada Date: Thu, 20 Aug 2026 21:21:33 -0400 Subject: [PATCH 5/5] fix(ingresssidecar): close startup inventory/reconcile race MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 0xmc's review on PR #424 flagged a real race in the startup sequence: Store.Inventory was gated on mgr.GetCache().WaitForCacheSync(ctx) alone, on the assumption that a synced informer cache implies every pre-existing EndpointSlice has already gone through the controller's own Reconcile (and therefore Store.SetDesired). That assumption is false — WaitForCacheSync only guarantees the informer's initial List landed in the cache, not that the workqueue it fed has been drained by Reconcile. On a busy node at boot the two can race: Store.Inventory could see a still-live pod's kernel route as orphaned (routeKnownLocked found no tracked state for it yet) and seed it under a synthetic "boot//" key with its own independent grace period. The real Reconcile, once it caught up, would create/update the real "namespace/name" key and never touch the stale boot entry. Once that entry's grace period elapsed, Sweep would call backend.RemoveRoute — deleting the actual kernel route (routes are addressed by prefix+table, not by Store's map key) out from under the still-live pod, with nothing left to notice or recover. Fix: internal/ingresssidecar.SeedFromAPI lists every tenant-labeled EndpointSlice directly via the manager's uncached reader (mgr.GetAPIReader(), safe to use before mgr.Start) and applies each one's desired route through the same Store.SetDesired path Reconciler itself uses — before Store.Inventory ever runs. This closes the race independently of cache/workqueue timing rather than trying to wait it out; SetDesired is idempotent, so the controller's own later, redundant Reconcile of the same objects is harmless. cmd/galactic-vrf/root.go's startup goroutine now calls SeedFromAPI before Inventory and drops the WaitForCacheSync gate, which was never sufficient on its own. Updated Store.Inventory/RunSweeper's doc comments and docs/plans/855's §9 item 2 decision to describe the corrected mechanism instead of the broken assumption. Added internal/ingresssidecar/seed_test.go, including TestSeedFromAPIThenInventoryDoesNotOrphanLiveRoute, which runs the fixed startup order directly (SeedFromAPI claiming a route from live API state with no completed Reconcile standing in for it) and asserts a long sweep afterward never removes it — the exact gap 0xmc called out as uncovered by existing tests. Co-Authored-By: Claude Sonnet 5 --- cmd/galactic-vrf/root.go | 25 +-- ...ngress-sidecar-vpc-backend-connectivity.md | 3 +- internal/ingresssidecar/controller.go | 5 +- internal/ingresssidecar/controller_test.go | 4 +- internal/ingresssidecar/seed.go | 79 +++++++++ internal/ingresssidecar/seed_test.go | 152 ++++++++++++++++++ internal/ingresssidecar/store.go | 24 +-- internal/ingresssidecar/store_test.go | 9 +- 8 files changed, 271 insertions(+), 30 deletions(-) create mode 100644 internal/ingresssidecar/seed.go create mode 100644 internal/ingresssidecar/seed_test.go diff --git a/cmd/galactic-vrf/root.go b/cmd/galactic-vrf/root.go index 3be5df70..2c864928 100644 --- a/cmd/galactic-vrf/root.go +++ b/cmd/galactic-vrf/root.go @@ -35,8 +35,8 @@ const ( // runCmd contains the application startup logic: it registers // internal/ingresssidecar's Reconciler against a cluster-scoped -// EndpointSlice watch, then runs its startup inventory and periodic sweep -// once the manager's caches have synced. There is no BGP runtime, CRD +// EndpointSlice watch, then seeds Store from the live API state and runs +// its startup inventory and periodic sweep. There is no BGP runtime, CRD // scheme beyond clientgoscheme's built-in discoveryv1 registration, or // per-node identity of any kind here — see internal/config.VRFConfig's own // doc comment for why. @@ -73,15 +73,20 @@ func runCmd(cfg *config.VRFConfig) error { return fmt.Errorf("setup EndpointSlice controller: %w", err) } - // Startup inventory + periodic sweep, gated on the manager's caches - // having synced -- see Store.Inventory's own doc comment for why: every - // EndpointSlice that exists at boot must have already gone through its - // own initial Reconcile (and therefore SetDesired) before Inventory or - // Sweep ever run, or a live VPC/pod could be misjudged as orphaned. - // Mirrors cmd/galactic-router's own GC-ticker startup goroutine. + // Startup seed + inventory + periodic sweep. Every EndpointSlice that + // exists at boot must be visible to Store *before* Inventory or Sweep + // ever run, or a live VPC/pod could be misjudged as orphaned -- see + // ingresssidecar.SeedFromAPI's own doc comment for why that can no + // longer be mgr.GetCache().WaitForCacheSync's job: a synced cache only + // guarantees the informer's initial List landed in the cache, not that + // the controller's own Reconcile has drained the workqueue that same + // List fed, so on a busy node at boot the two could race. SeedFromAPI + // uses mgr.GetAPIReader(), the manager's uncached reader, so it + // doesn't depend on cache/workqueue timing at all. Mirrors + // cmd/galactic-router's own GC-ticker startup goroutine. go func() { - if !mgr.GetCache().WaitForCacheSync(ctx) { - log.Printf("startup inventory: cache sync failed, skipping") + if err := ingresssidecar.SeedFromAPI(ctx, mgr.GetAPIReader(), store); err != nil { + log.Printf("startup seed: %v", err) return } if err := store.Inventory(ctx, time.Now()); err != nil { diff --git a/docs/plans/855-ingress-sidecar-vpc-backend-connectivity.md b/docs/plans/855-ingress-sidecar-vpc-backend-connectivity.md index 2134e37b..583f8abd 100644 --- a/docs/plans/855-ingress-sidecar-vpc-backend-connectivity.md +++ b/docs/plans/855-ingress-sidecar-vpc-backend-connectivity.md @@ -110,7 +110,8 @@ Per #854's resolved annotation contract (below), each pod's own address/SID arri 1. ~~**Teardown race**~~ — **Decision (2026-08-08): time-based grace period.** No ordering guarantee exists between this sidecar's route deletion and the extension server's config-push for the same backend, and #854 confirmed `galactic-cni` does nothing to help narrow the window (immediate, unconditional `EndpointSlice` removal on DEL, no drain signal). For v1, delay teardown by a fixed interval after dropping out of desired state, rather than tearing down synchronously on the watch event — simple to implement with the existing reconciler (a requeue-after on the "absent" transition), and it bounds the blackhole window even without a proper handshake. This applies at both granularities from §1, with independent timers: a **route**'s grace period starts when its own `EndpointSlice` (pod) disappears; a **VRF**'s grace period starts only once the last pod across every attachment of that VPC on this node is gone, and must not fire while any of that VPC's routes are still within their own grace period. **Decision (2026-08-18): the interval is a configurable knob (env var/flag), not hardcoded**, with a conservative placeholder default of 30s — long enough to plausibly cover a typical config-push-plus-drain window, short enough not to wedge normal-churn testing. The default is explicitly a guess pending real #857 config-push-latency data, not a tuned value; revisit once #857 exists and is observable. **Flagged for follow-up:** this is a stopgap, not a fix — it's a race disguised as a wait, not closed. A time-based delay can still blackhole a slow config-push under load, or hold VRF/route state longer than necessary under fast churn. A real fix (e.g. a signal-based handshake once a channel exists between this sidecar and the extension server, or a readiness/ack mechanism) should be investigated once both components exist and their actual latency characteristics are observable — track as an explicit follow-up, not something this plan resolves. -2. ~~**Startup reconcile safety**~~ — **Decision (2026-08-08): inventory-before-reconcile.** On startup, list existing kernel state tagged as this component's own (VRF devices matching the `intf.GenerateInterfaceNameVRF` naming convention, and their currently-installed seg6 routes) into a "known at boot" set before running any teardown logic, and block on the watch cache's first full sync (`WaitForCacheSync`) before treating anything absent from it as stale. A pre-existing VRF or route with no corresponding `EndpointSlice` yet is provisionally valid until the cache has actually finished its initial list, not immediately eligible for teardown — kept as a separate mechanism from the #1 grace-period timer rather than reusing it, since startup is a one-time, boot-scoped condition (bounded by cache sync completing) and not an ongoing per-pod/per-VPC transition. +2. ~~**Startup reconcile safety**~~ — **Decision (2026-08-08): inventory-before-reconcile.** On startup, list existing kernel state tagged as this component's own (VRF devices matching the `intf.GenerateInterfaceNameVRF` naming convention, and their currently-installed seg6 routes) into a "known at boot" set before running any teardown logic. A pre-existing VRF or route with no corresponding `EndpointSlice` yet is provisionally valid, not immediately eligible for teardown — kept as a separate mechanism from the #1 grace-period timer rather than reusing it, since startup is a one-time, boot-scoped condition and not an ongoing per-pod/per-VPC transition. + **Correction (2026-08-20, PR #424 review):** the original mechanism for "known at boot" was blocking on the watch cache's first full sync (`WaitForCacheSync`) before running `Store.Inventory`, on the assumption that a synced cache implies every pre-existing `EndpointSlice` has already gone through the controller's own `Reconcile` (and therefore `Store.SetDesired`). That assumption is false: `WaitForCacheSync` only guarantees the informer's initial `List` landed in the cache, not that the resulting workqueue has been drained by `Reconcile`. On a busy node at boot the two can race — `Store.Inventory` could observe a still-live pod's kernel route as orphaned before its `Reconcile` catches up, seed it under a synthetic `boot//` key with its own independent grace period, and once that period elapsed, `Store.Sweep` would delete the underlying kernel route (routes are addressed by prefix+table, not by `Store`'s map key) out from under the still-live pod — silently, since nothing re-triggers `SetDesired` for that pod absent a further `EndpointSlice` change. Fixed by `ingresssidecar.SeedFromAPI`: before `Store.Inventory` ever runs, list every tenant-labeled `EndpointSlice` directly via the manager's uncached reader (`mgr.GetAPIReader()`, safe to use before `mgr.Start`) and apply each one's desired route via the same `Store.SetDesired` path `Reconciler` itself uses — closing the race independently of cache/workqueue timing rather than trying to wait it out. 3. **Deployment injection contract with #856** — partially addressed: **recommend container name `galactic-vrf`**, matching the binary (`cmd/galactic-vrf`), so #856's strategic-merge patch has a concrete name to add/find by. This is a recommendation, not a locked contract — the team implementing #856 has latitude to rename it if the generated Envoy Deployment's shape forces a collision or a different convention. Everything else in the contract (image location/publishing, required env vars, the `CAP_NET_ADMIN`-only `securityContext` shape) is still uncoordinated and has no owner on the #856 side yet. **Decision (2026-08-18): stays a flagged dependency, not resolved further here.** Considered writing the env-var/`securityContext` portion of the contract concretely now (both are largely derivable from this plan already) rather than leaving them TBD, but decided against pre-committing #856's side of the contract before that work has an owner — only image location/publishing genuinely can't be decided from this plan alone (this repo has no production image build today, a repo-wide gap, not specific to this sidecar). No change to scope. 4. ~~**Health-checking VPC backends**~~ — **Decision (2026-08-08): punt as a separate future issue.** In scope per #855's original body (including a specific rationale for why Envoy's native health checks might not be enough — VRF/SRv6 reachability isn't something Envoy can observe from the overlay), but absent from PR #851's accepted design entirely, with no owner anywhere in #855/#856/#857. Explicitly out of scope for this sidecar and for the current #796 workstream as a whole — to be filed as its own follow-up issue once the rest of the mechanism is live and real failure modes are observable, rather than designed speculatively now. diff --git a/internal/ingresssidecar/controller.go b/internal/ingresssidecar/controller.go index f442fd15..61d8189b 100644 --- a/internal/ingresssidecar/controller.go +++ b/internal/ingresssidecar/controller.go @@ -82,9 +82,8 @@ func hasTenantLabel(obj client.Object) bool { // condition over potentially many routes, not a single watched object's own // transition). Mirrors cmd/galactic-router's GC ticker goroutine in shape. // -// Callers must not start this until the manager's informer cache has -// synced and Store.Inventory has run — see Store.Inventory's own doc -// comment. +// Callers must not start this until SeedFromAPI and Store.Inventory have +// both run — see Store.Inventory's own doc comment. func RunSweeper(ctx context.Context, store *Store, interval time.Duration) { ticker := time.NewTicker(interval) defer ticker.Stop() diff --git a/internal/ingresssidecar/controller_test.go b/internal/ingresssidecar/controller_test.go index 869f0ad6..f08cca05 100644 --- a/internal/ingresssidecar/controller_test.go +++ b/internal/ingresssidecar/controller_test.go @@ -88,8 +88,8 @@ func TestReconcilerMalformedSliceDoesNotError(t *testing.T) { slice := &discoveryv1.EndpointSlice{ ObjectMeta: metav1.ObjectMeta{ Namespace: "ns", Name: testPodName, - Labels: map[string]string{crdnames.LabelTenantID: "novalidseparator"}, - Annotations: map[string]string{crdnames.AnnotationTenantID: "novalidseparator"}, + Labels: map[string]string{crdnames.LabelTenantID: testMalformedTenantID}, + Annotations: map[string]string{crdnames.AnnotationTenantID: testMalformedTenantID}, }, } c := fake.NewClientBuilder().WithScheme(scheme).WithObjects(slice).Build() diff --git a/internal/ingresssidecar/seed.go b/internal/ingresssidecar/seed.go new file mode 100644 index 00000000..95dc684f --- /dev/null +++ b/internal/ingresssidecar/seed.go @@ -0,0 +1,79 @@ +// Copyright 2026 Datum Cloud, Inc. +// +// SPDX-License-Identifier: AGPL-3.0-or-later + +package ingresssidecar + +import ( + "context" + "fmt" + + discoveryv1 "k8s.io/api/discovery/v1" + "k8s.io/apimachinery/pkg/labels" + "k8s.io/apimachinery/pkg/selection" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + + "go.datum.net/galactic/internal/crdnames" +) + +// SeedFromAPI lists every EndpointSlice this sidecar selects on directly +// from reader and applies each one's desired route to store synchronously, +// via the same SetDesired path Reconciler itself uses. +// +// Call this once at startup, before Store.Inventory, passing +// mgr.GetAPIReader() as reader — the manager's uncached reader, which talks +// straight to the API server and is safe to use before mgr.Start. That +// matters because it's what lets this run without depending on the +// manager's informer cache or controller workqueue at all: Store.Inventory +// used to be gated on mgr.GetCache().WaitForCacheSync(ctx) alone, on the +// assumption that a synced cache implied every pre-existing EndpointSlice +// had already gone through the controller's own Reconcile (and therefore +// SetDesired). That assumption is false — WaitForCacheSync only guarantees +// the informer's initial List landed in the cache; it says nothing about +// whether the workqueue that same initial List fed into has been drained +// by the controller's Reconcile loop yet. On a busy node at boot those two +// things race: Inventory could observe a live pod's kernel route as +// orphaned (routeKnownLocked found no tracked state for it yet) and seed it +// under a synthetic "boot/..." key with its own grace period, independently +// of the real key the delayed Reconcile eventually creates -- and once that +// synthetic entry's grace elapsed, Sweep would delete the underlying kernel +// route (routes are addressed by prefix+table, not by Store's map key) out +// from under the still-live pod. SeedFromAPI closes that race by making +// every live EndpointSlice's desired route visible to Store before +// Inventory ever runs, independent of cache/queue timing entirely. +// SetDesired is idempotent (EnsureVRF/EnsureRoute no-op once installed), so +// the controller's own later, now-redundant Reconcile of the same objects +// is harmless. +func SeedFromAPI(ctx context.Context, reader client.Reader, store *Store) error { + req, err := labels.NewRequirement(crdnames.LabelTenantID, selection.Exists, nil) + if err != nil { + return fmt.Errorf("build tenant-label selector: %w", err) + } + sel := labels.NewSelector().Add(*req) + + var list discoveryv1.EndpointSliceList + if err := reader.List(ctx, &list, client.MatchingLabelsSelector{Selector: sel}); err != nil { + return fmt.Errorf("list EndpointSlices: %w", err) + } + + for i := range list.Items { + slice := &list.Items[i] + desired, err := BuildDesiredRoute(slice) + if err != nil { + // Same handling as Reconciler.Reconcile: malformed-but-selected + // is worth logging, not worth failing startup over. + ctrl.LoggerFrom(ctx).Error(err, "skipping malformed EndpointSlice", + "endpointslice", fmt.Sprintf("%s/%s", slice.Namespace, slice.Name)) + continue + } + if desired == nil { + continue // not yet ready (no SID annotation) -- nothing to seed + } + key := fmt.Sprintf("%s/%s", slice.Namespace, slice.Name) + if err := store.SetDesired(ctx, key, desired); err != nil { + return fmt.Errorf("seed EndpointSlice %s: %w", key, err) + } + } + return nil +} diff --git a/internal/ingresssidecar/seed_test.go b/internal/ingresssidecar/seed_test.go new file mode 100644 index 00000000..01a81f3d --- /dev/null +++ b/internal/ingresssidecar/seed_test.go @@ -0,0 +1,152 @@ +// Copyright 2026 Datum Cloud, Inc. +// +// SPDX-License-Identifier: AGPL-3.0-or-later + +package ingresssidecar + +import ( + "context" + "testing" + "time" + + discoveryv1 "k8s.io/api/discovery/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + + "go.datum.net/galactic/internal/crdnames" +) + +// TestSeedFromAPIAppliesReadySlice verifies a well-formed, ready +// EndpointSlice found via the API reader is applied to Store just like a +// completed Reconcile would. +func TestSeedFromAPIAppliesReadySlice(t *testing.T) { + scheme := newTestScheme(t) + slice := readySlice("vpc1-att1", "fd00:99::1", "fd00::1") + c := fake.NewClientBuilder().WithScheme(scheme).WithObjects(slice).Build() + + backend := newFakeBackend() + store := NewStore(backend, testGrace, nil) + + if err := SeedFromAPI(context.Background(), c, store); err != nil { + t.Fatalf("SeedFromAPI: %v", err) + } + if got := backend.vrfCount(); got != 1 { + t.Errorf("vrfCount = %d, want 1", got) + } + if got := backend.routeCount(); got != 1 { + t.Errorf("routeCount = %d, want 1", got) + } +} + +// TestSeedFromAPISkipsNotYetReady verifies a tenant-labeled EndpointSlice +// with no SID annotation yet (BuildDesiredRoute's (nil, nil) case) is +// skipped without error, same as Reconciler.Reconcile would. +func TestSeedFromAPISkipsNotYetReady(t *testing.T) { + scheme := newTestScheme(t) + slice := readySlice("vpc1-att1", "", "fd00::1") // no SID yet + c := fake.NewClientBuilder().WithScheme(scheme).WithObjects(slice).Build() + + backend := newFakeBackend() + store := NewStore(backend, testGrace, nil) + + if err := SeedFromAPI(context.Background(), c, store); err != nil { + t.Fatalf("SeedFromAPI: %v", err) + } + if got := backend.vrfCount(); got != 0 { + t.Errorf("vrfCount = %d, want 0 (not ready)", got) + } +} + +// TestSeedFromAPISkipsMalformedSlice verifies a selected-but-malformed +// EndpointSlice is logged and skipped rather than failing the whole seed +// pass, matching Reconciler.Reconcile's own handling of the same case. +func TestSeedFromAPISkipsMalformedSlice(t *testing.T) { + scheme := newTestScheme(t) + slice := &discoveryv1.EndpointSlice{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "ns", Name: testPodName, + Labels: map[string]string{crdnames.LabelTenantID: testMalformedTenantID}, + Annotations: map[string]string{crdnames.AnnotationTenantID: testMalformedTenantID}, + }, + } + c := fake.NewClientBuilder().WithScheme(scheme).WithObjects(slice).Build() + + backend := newFakeBackend() + store := NewStore(backend, testGrace, nil) + + if err := SeedFromAPI(context.Background(), c, store); err != nil { + t.Fatalf("SeedFromAPI: want nil error for malformed-but-selected slice, got %v", err) + } + if got := backend.vrfCount(); got != 0 { + t.Errorf("vrfCount = %d, want 0", got) + } +} + +// TestSeedFromAPIIgnoresUnlabeledSlices verifies the tenant-label selector +// actually filters the List call, not just BuildDesiredRoute's own +// IsSelected check downstream -- an EndpointSlice with no tenant label at +// all must never even be considered. +func TestSeedFromAPIIgnoresUnlabeledSlices(t *testing.T) { + scheme := newTestScheme(t) + slice := &discoveryv1.EndpointSlice{ + ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: testPodName}, + AddressType: discoveryv1.AddressTypeIPv6, + } + c := fake.NewClientBuilder().WithScheme(scheme).WithObjects(slice).Build() + + backend := newFakeBackend() + store := NewStore(backend, testGrace, nil) + + if err := SeedFromAPI(context.Background(), c, store); err != nil { + t.Fatalf("SeedFromAPI: %v", err) + } + if got := backend.vrfCount(); got != 0 { + t.Errorf("vrfCount = %d, want 0 (unlabeled slice must be filtered by the selector)", got) + } +} + +// TestSeedFromAPIThenInventoryDoesNotOrphanLiveRoute reproduces the startup +// race flagged in PR #424's review: Store.Inventory used to be gated only +// on mgr.GetCache().WaitForCacheSync, which guarantees the informer's +// initial List landed in the cache but not that the controller's own +// Reconcile had drained the workqueue that same List fed. On a busy node +// at boot those could race, so Inventory could see a still-live pod's +// kernel route as orphaned and seed it under a synthetic "boot/..." key +// with its own independent grace period -- and once that synthetic entry's +// grace elapsed, Sweep would delete the underlying kernel route (routes are +// addressed by prefix+table, not by Store's map key) out from under the +// still-live pod, even though the real key (from the eventually-completed +// Reconcile) still believed it was installed. +// +// This test runs the fixed startup order directly -- SeedFromAPI, claiming +// the route from live API state, strictly before Inventory sees the +// matching kernel state -- with no Reconcile/SetDesired call standing in +// for "the controller's workqueue happened to drain in time". A long sweep +// afterward must never remove the route. +func TestSeedFromAPIThenInventoryDoesNotOrphanLiveRoute(t *testing.T) { + scheme := newTestScheme(t) + slice := readySlice("vpc1-att1", "fd00:99::1", "fd00::1") + c := fake.NewClientBuilder().WithScheme(scheme).WithObjects(slice).Build() + + backend := newFakeBackend() + store := NewStore(backend, testGrace, nil) + + ctx := context.Background() + if err := SeedFromAPI(ctx, c, store); err != nil { + t.Fatalf("SeedFromAPI: %v", err) + } + if err := store.Inventory(ctx, time.Now()); err != nil { + t.Fatalf("Inventory: %v", err) + } + + // Sweep well past the grace period: the route must survive, tracked + // under its real EndpointSlice key with no absentSince set -- never + // seeded as a synthetic "boot/..." entry racing its own timer. + store.Sweep(ctx, time.Now().Add(100*testGrace)) + if got := backend.routeCount(); got != 1 { + t.Errorf("routeCount = %d, want 1 (live route must survive the boot race)", got) + } + if got := backend.vrfCount(); got != 1 { + t.Errorf("vrfCount = %d, want 1", got) + } +} diff --git a/internal/ingresssidecar/store.go b/internal/ingresssidecar/store.go index 2d295abc..5cc8caa7 100644 --- a/internal/ingresssidecar/store.go +++ b/internal/ingresssidecar/store.go @@ -217,19 +217,19 @@ func (s *Store) Sweep(ctx context.Context, now time.Time) { // currently-installed seg6 routes) already present on the host at process // start — §9 item 2 of the plan's startup-reconcile-safety decision. // -// Call this once, after the manager's caches have synced (so every -// EndpointSlice existing at boot has already been through SetDesired via -// the controller's own initial reconcile pass — see Reconciler) but before +// Call this once, after every EndpointSlice existing at boot has already +// been through SetDesired — see SeedFromAPI, which callers must run first +// for exactly that reason (its own doc comment covers why +// mgr.GetCache().WaitForCacheSync alone isn't sufficient here) — but before // the first Sweep runs. A VPC/route already known by that point is left -// alone: a live EndpointSlice's reconcile beat Inventory here, so its -// absentSince is already clear. Anything Inventory itself has to seed is, -// by construction, missing that reconcile — either a VPC/pod truly orphaned -// while this sidecar was down, or one whose EndpointSlice hasn't reconciled -// yet for some other reason — so it's seeded with an ordinary grace period -// starting now rather than torn down on sight (giving a slightly late -// EndpointSlice reconcile a chance to reclaim it) and rather than kept -// alive forever (the pre-#377-revision failure mode this decision exists to -// avoid). +// alone: SeedFromAPI's call already claimed it, so its absentSince is +// already clear. Anything Inventory itself has to seed is, by construction, +// missing that claim — either a VPC/pod truly orphaned while this sidecar +// was down, or one whose EndpointSlice is itself gone/unready for some +// other reason — so it's seeded with an ordinary grace period starting now +// rather than torn down on sight (giving a slightly late EndpointSlice +// update a chance to reclaim it) and rather than kept alive forever (the +// pre-#377-revision failure mode this decision exists to avoid). func (s *Store) Inventory(ctx context.Context, now time.Time) error { infos, err := s.backend.ListVRFs() if err != nil { diff --git a/internal/ingresssidecar/store_test.go b/internal/ingresssidecar/store_test.go index 24da6c8f..0021a1f2 100644 --- a/internal/ingresssidecar/store_test.go +++ b/internal/ingresssidecar/store_test.go @@ -22,11 +22,16 @@ func mustPrefix(t *testing.T, s string) *net.IPNet { const testGrace = 10 * time.Second -// testVPC1 and testPodName are the fixture VPC/pod-name values shared -// across this package's tests. +// testVPC1, testPodName, and testMalformedTenantID are fixture values +// shared across this package's tests. const ( testVPC1 = "vpc1" testPodName = "pod-a" + // testMalformedTenantID has no "-" separator, so + // crdnames.ParseTenantIdentifier rejects it -- used by both + // controller_test.go and seed_test.go to build a selected-but- + // malformed EndpointSlice fixture. + testMalformedTenantID = "novalidseparator" ) // TestStoreRouteAndVRFAppear verifies a pod's first appearance creates both