From 2ccef4b643e693f8baffd61b799b1a3e6dbc2ac6 Mon Sep 17 00:00:00 2001 From: "David L. Chandler" Date: Fri, 26 Jun 2026 16:21:46 -0400 Subject: [PATCH 01/10] fix: stop leaking cached envoy snapshots For #7086 Signed-off-by: David L. Chandler --- .../gateway2/proxy_syncer/proxy_syncer.go | 145 +++++++++++++++++- .../proxy_syncer/status_gc_client_test.go | 145 ++++++++++++++++++ .../gateway2/proxy_syncer/status_gc_test.go | 59 +++++++ 3 files changed, 344 insertions(+), 5 deletions(-) create mode 100644 projects/gateway2/proxy_syncer/status_gc_client_test.go create mode 100644 projects/gateway2/proxy_syncer/status_gc_test.go diff --git a/projects/gateway2/proxy_syncer/proxy_syncer.go b/projects/gateway2/proxy_syncer/proxy_syncer.go index 54bf5691c17..e295e98dc7d 100644 --- a/projects/gateway2/proxy_syncer/proxy_syncer.go +++ b/projects/gateway2/proxy_syncer/proxy_syncer.go @@ -8,6 +8,7 @@ import ( "maps" "reflect" "slices" + "sync" "time" "go.uber.org/zap" @@ -25,6 +26,8 @@ import ( "github.com/solo-io/gloo/projects/gloo/pkg/syncer/setup" "github.com/solo-io/gloo/projects/gloo/pkg/xds" rlkubev1a1 "github.com/solo-io/solo-apis/pkg/api/ratelimit.solo.io/v1alpha1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/sets" "github.com/solo-io/solo-kit/pkg/api/v1/clients/common" @@ -103,6 +106,14 @@ type ProxySyncer struct { allowedGatewayClasses sets.Set[string] waitForSync []cache.InformerSynced + + // statusGCMu guards the prev*RouteKeys sets used to garbage-collect stale + // route status (#7086). syncRouteStatus currently runs on a single + // goroutine; the mutex keeps tracking safe if that ever changes. + statusGCMu sync.Mutex + prevHTTPRouteKeys sets.Set[types.NamespacedName] + prevTCPRouteKeys sets.Set[types.NamespacedName] + prevTLSRouteKeys sets.Set[types.NamespacedName] } type GatewayInputChannels struct { @@ -582,7 +593,10 @@ func (s *ProxySyncer) Start(ctx context.Context) error { // caches are warm, now we can do registrations s.statusReport.Register(func(o krt.Event[report]) { if o.Event == controllers.EventDelete { - // TODO: handle garbage collection (see: https://github.com/solo-io/solo-projects/issues/7086) + // The merged status report is a Singleton, so a delete here means the + // whole report collection went away (shutdown); there is no per-object + // report to act on. Stale per-route status GC for routes that leave the + // report is handled in syncRouteStatus -> gcStaleRouteStatus (#7086). return } latestReportQueue.Enqueue(o.Latest().ReportMap) @@ -643,10 +657,17 @@ func (s *ProxySyncer) Start(ctx context.Context) error { snapWrap := e.Latest() s.proxyTranslator.syncXds(ctx, snapWrap.snap, snapWrap.proxyKey) } else { - // key := e.Latest().proxyKey - // if _, err := s.proxyTranslator.xdsCache.GetSnapshot(key); err == nil { - // s.proxyTranslator.xdsCache.ClearSnapshot(e.Latest().proxyKey) - // } + // Evict the snapshot for a disconnected/obsolete unique client. + // Without this, the xds SnapshotCache grows unbounded under client + // churn (each new role+labels+namespace key leaves a snapshot behind + // forever). The delete event only fires once the last stream for this + // proxyKey is gone (uniqueClients is refcounted), so no connected envoy + // still needs it; if a client with the same key reconnects, the + // snapshot is rebuilt and re-set. See solo-projects#7086. + key := e.Latest().proxyKey + if _, err := s.proxyTranslator.xdsCache.GetSnapshot(key); err == nil { + s.proxyTranslator.xdsCache.ClearSnapshot(key) + } } } }, true) @@ -1018,6 +1039,120 @@ func (s *ProxySyncer) syncRouteStatus(ctx context.Context, rm reports.ReportMap) logger.Errorw("all attempts failed at updating TLSRoute status", "error", err, "route", rnn) } } + + // Garbage-collect status from routes that are no longer in the report + // (detached or deleted) but to which we previously wrote status. See #7086. + s.gcStaleRouteStatus(ctx, rm) +} + +// gcStaleRouteStatus clears status this controller previously wrote to routes +// that are no longer present in the report. Without this, stale conditions +// (e.g. Accepted/ResolvedRefs) linger on a route forever once it detaches from +// the gateway, since the normal sync path only iterates routes still in the +// report. See https://github.com/solo-io/solo-projects/issues/7086. +func (s *ProxySyncer) gcStaleRouteStatus(ctx context.Context, rm reports.ReportMap) { + s.statusGCMu.Lock() + defer s.statusGCMu.Unlock() + + cl := s.mgr.GetClient() + s.prevHTTPRouteKeys = clearStaleRouteStatus(ctx, cl, s.controllerName, s.prevHTTPRouteKeys, sets.KeySet(rm.HTTPRoutes), + wellknown.HTTPRouteKind, func() client.Object { return new(gwv1.HTTPRoute) }) + s.prevTCPRouteKeys = clearStaleRouteStatus(ctx, cl, s.controllerName, s.prevTCPRouteKeys, sets.KeySet(rm.TCPRoutes), + wellknown.TCPRouteKind, func() client.Object { return new(gwv1a2.TCPRoute) }) + s.prevTLSRouteKeys = clearStaleRouteStatus(ctx, cl, s.controllerName, s.prevTLSRouteKeys, sets.KeySet(rm.TLSRoutes), + wellknown.TLSRouteKind, func() client.Object { return new(gwv1a2.TLSRoute) }) +} + +// Retry settings for stale-status GC writes; vars (not consts) so tests can +// shrink the delay. +var ( + statusGCRetryAttempts uint = 5 + statusGCRetryDelay = 100 * time.Millisecond +) + +// clearStaleRouteStatus removes controllerName's parent status from every route +// in prev that is no longer in current. It returns the tracking set for the next +// round: routes still in current, plus any stale route whose cleanup could not +// be completed (e.g. a conflict that outlasts the in-line retry), so the next +// sync retries it instead of silently leaking stale status. +// +// Note: tracking is in-memory, so a crash between a route leaving the report and +// its GC drops that route from tracking; such a route's stale status is not +// revisited until it is reconciled again. Full robustness would require listing +// routes at startup and reclaiming ones bearing our controller's status. +func clearStaleRouteStatus( + ctx context.Context, + cl client.Client, + controllerName string, + prev, current sets.Set[types.NamespacedName], + routeType string, + newObj func() client.Object, +) sets.Set[types.NamespacedName] { + logger := contextutils.LoggerFrom(ctx) + + // Start from the routes still in the report; re-add any we fail to clean. + next := current.Clone() + + for rnn := range prev.Difference(current) { + err := retry.Do(func() error { + // Re-Get inside the retry so each attempt has a fresh resourceVersion; + // otherwise a conflict just repeats forever. + route := newObj() + if err := cl.Get(ctx, rnn, route); err != nil { + if apierrors.IsNotFound(err) { + return nil // route deleted; status went with it + } + return err + } + if !removeControllerRouteStatus(route, controllerName) { + return nil // we held no status on this route + } + return cl.Status().Update(ctx, route) + }, + retry.Attempts(statusGCRetryAttempts), + retry.Delay(statusGCRetryDelay), + retry.DelayType(retry.BackOffDelay), + // Only conflicts are worth retrying in-line (the re-Get above clears + // them). Other errors fail fast and are retried on the next sync. + retry.RetryIf(apierrors.IsConflict), + retry.LastErrorOnly(true), + ) + if err != nil { + logger.Errorw("failed to clear stale route status; will retry next sync", + "error", err, "type", routeType, "route", rnn) + next.Insert(rnn) + continue + } + logger.Debugf("cleared stale %s status from %s", routeType, rnn) + } + return next +} + +// removeControllerRouteStatus removes RouteParentStatus entries owned by +// controllerName. It returns true if the route's status was modified. +func removeControllerRouteStatus(route client.Object, controllerName string) bool { + var rs *gwv1.RouteStatus + switch r := route.(type) { + case *gwv1.HTTPRoute: + rs = &r.Status.RouteStatus + case *gwv1a2.TCPRoute: + rs = &r.Status.RouteStatus + case *gwv1a2.TLSRoute: + rs = &r.Status.RouteStatus + default: + return false + } + kept := make([]gwv1.RouteParentStatus, 0, len(rs.Parents)) + for _, p := range rs.Parents { + if string(p.ControllerName) != controllerName { + kept = append(kept, p) + } + } + if len(kept) == len(rs.Parents) { + return false + } + rs.Parents = kept + return true } // syncGatewayStatus will build and update status for all Gateways in a reportMap diff --git a/projects/gateway2/proxy_syncer/status_gc_client_test.go b/projects/gateway2/proxy_syncer/status_gc_client_test.go new file mode 100644 index 00000000000..68d030ddfa3 --- /dev/null +++ b/projects/gateway2/proxy_syncer/status_gc_client_test.go @@ -0,0 +1,145 @@ +package proxy_syncer + +import ( + "context" + "errors" + "testing" + "time" + + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/types" + "k8s.io/apimachinery/pkg/util/sets" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" + gwv1 "sigs.k8s.io/gateway-api/apis/v1" + + "github.com/solo-io/gloo/pkg/schemes" +) + +const testController = "solo.io/gloo-gateway" + +func staleHTTPRoute() *gwv1.HTTPRoute { + return &gwv1.HTTPRoute{ + ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "r"}, + Status: gwv1.HTTPRouteStatus{RouteStatus: gwv1.RouteStatus{ + Parents: []gwv1.RouteParentStatus{{ + ControllerName: gwv1.GatewayController(testController), + ParentRef: gwv1.ParentReference{Name: "gw"}, + }}, + }}, + } +} + +func httpRouteKey() types.NamespacedName { + return types.NamespacedName{Namespace: "ns", Name: "r"} +} + +func newHTTPRouteObj() client.Object { return new(gwv1.HTTPRoute) } + +func conflictErr() error { + return apierrors.NewConflict( + schema.GroupResource{Group: gwv1.GroupName, Resource: "httproutes"}, + "r", errors.New("the object has been modified")) +} + +func init() { + // keep retries fast in tests + statusGCRetryDelay = time.Millisecond +} + +// A transient conflict on the status write should be retried in-line (with a +// fresh Get) and ultimately succeed, so the route is cleaned and dropped from +// tracking. +func TestClearStaleRouteStatus_TransientConflictThenSuccess(t *testing.T) { + var updateCalls int + cl := fake.NewClientBuilder(). + WithScheme(schemes.GatewayScheme()). + WithObjects(staleHTTPRoute()). + WithStatusSubresource(&gwv1.HTTPRoute{}). + WithInterceptorFuncs(interceptor.Funcs{ + SubResourceUpdate: func(ctx context.Context, c client.Client, sr string, obj client.Object, opts ...client.SubResourceUpdateOption) error { + updateCalls++ + if updateCalls == 1 { + return conflictErr() + } + return c.SubResource(sr).Update(ctx, obj, opts...) + }, + }). + Build() + + prev := sets.New(httpRouteKey()) + current := sets.New[types.NamespacedName]() // route left the report + + next := clearStaleRouteStatus(context.Background(), cl, testController, prev, current, + "HTTPRoute", newHTTPRouteObj) + + if next.Has(httpRouteKey()) { + t.Fatalf("route should have been cleaned and dropped from tracking, but it was retained") + } + if updateCalls < 2 { + t.Fatalf("expected the write to be retried after the conflict, got %d update calls", updateCalls) + } + + got := &gwv1.HTTPRoute{} + if err := cl.Get(context.Background(), httpRouteKey(), got); err != nil { + t.Fatalf("get: %v", err) + } + if len(got.Status.Parents) != 0 { + t.Fatalf("expected our parent status to be cleared, got %d parents", len(got.Status.Parents)) + } +} + +// A persistent non-conflict error should fail fast (not retried in-line) and the +// route should be RETAINED in tracking so the next sync retries it; the next +// pass (no error) clears it. +func TestClearStaleRouteStatus_PersistentErrorRetainsThenClears(t *testing.T) { + failWrites := true + var updateCalls int + cl := fake.NewClientBuilder(). + WithScheme(schemes.GatewayScheme()). + WithObjects(staleHTTPRoute()). + WithStatusSubresource(&gwv1.HTTPRoute{}). + WithInterceptorFuncs(interceptor.Funcs{ + SubResourceUpdate: func(ctx context.Context, c client.Client, sr string, obj client.Object, opts ...client.SubResourceUpdateOption) error { + updateCalls++ + if failWrites { + return errors.New("status update boom") // non-conflict => not retried in-line + } + return c.SubResource(sr).Update(ctx, obj, opts...) + }, + }). + Build() + + prev := sets.New(httpRouteKey()) + current := sets.New[types.NamespacedName]() + + // First pass: write fails, route must be retained for retry next cycle. + next := clearStaleRouteStatus(context.Background(), cl, testController, prev, current, + "HTTPRoute", newHTTPRouteObj) + if !next.Has(httpRouteKey()) { + t.Fatalf("route should be retained in tracking after a failed cleanup") + } + if updateCalls != 1 { + t.Fatalf("non-conflict error must not be retried in-line; got %d update calls", updateCalls) + } + + // Next sync cycle: writes succeed; the retained route should now be cleared + // and dropped from tracking. + failWrites = false + next2 := clearStaleRouteStatus(context.Background(), cl, testController, next, current, + "HTTPRoute", newHTTPRouteObj) + if next2.Has(httpRouteKey()) { + t.Fatalf("route should be cleaned and dropped from tracking on the retry cycle") + } + + got := &gwv1.HTTPRoute{} + if err := cl.Get(context.Background(), httpRouteKey(), got); err != nil { + t.Fatalf("get: %v", err) + } + if len(got.Status.Parents) != 0 { + t.Fatalf("expected our parent status cleared on retry cycle, got %d parents", len(got.Status.Parents)) + } +} diff --git a/projects/gateway2/proxy_syncer/status_gc_test.go b/projects/gateway2/proxy_syncer/status_gc_test.go new file mode 100644 index 00000000000..99ac522a24e --- /dev/null +++ b/projects/gateway2/proxy_syncer/status_gc_test.go @@ -0,0 +1,59 @@ +package proxy_syncer + +import ( + "testing" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + gwv1 "sigs.k8s.io/gateway-api/apis/v1" +) + +func httpRouteWithParents(controllers ...string) *gwv1.HTTPRoute { + r := &gwv1.HTTPRoute{ + ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "r"}, + } + for _, c := range controllers { + r.Status.Parents = append(r.Status.Parents, gwv1.RouteParentStatus{ + ControllerName: gwv1.GatewayController(c), + ParentRef: gwv1.ParentReference{Name: "gw"}, + }) + } + return r +} + +func TestRemoveControllerRouteStatus(t *testing.T) { + const me = "solo.io/gloo-gateway" + const other = "example.com/other-controller" + + t.Run("removes only our parent status", func(t *testing.T) { + r := httpRouteWithParents(me, other) + if !removeControllerRouteStatus(r, me) { + t.Fatalf("expected status to be modified") + } + if len(r.Status.Parents) != 1 { + t.Fatalf("expected 1 parent left, got %d", len(r.Status.Parents)) + } + if string(r.Status.Parents[0].ControllerName) != other { + t.Fatalf("expected the other controller's status to remain, got %q", r.Status.Parents[0].ControllerName) + } + }) + + t.Run("no-op when we own no status", func(t *testing.T) { + r := httpRouteWithParents(other) + if removeControllerRouteStatus(r, me) { + t.Fatalf("expected no modification when controller owns no parent status") + } + if len(r.Status.Parents) != 1 { + t.Fatalf("expected the other controller's status untouched") + } + }) + + t.Run("clears all when we own everything", func(t *testing.T) { + r := httpRouteWithParents(me, me) + if !removeControllerRouteStatus(r, me) { + t.Fatalf("expected status to be modified") + } + if len(r.Status.Parents) != 0 { + t.Fatalf("expected all parents removed, got %d", len(r.Status.Parents)) + } + }) +} From 314bc7eb2b523710a68f9f3ab8decade075a7045 Mon Sep 17 00:00:00 2001 From: "David L. Chandler" Date: Wed, 8 Jul 2026 17:48:53 -0400 Subject: [PATCH 02/10] fix: evict xds snapshots on unique-client disconnect, harden route status GC Rework the snapshot eviction and stale-route-status GC on this branch based on review findings. xds eviction: - Register eviction on uniqueClients delete events (the refcounted "last stream gone" signal) instead of perclientSnapCollection deletes, which also fire transiently for still-connected clients when snapshotPerClient returns nil (translation blips, the defer-building-snapshot hack). Clearing on those would withdraw a live envoy's last coherent config and force-close its watches, resetting healthy ADS streams with codes.Unavailable. - Guard ClearSnapshot with GetStatusInfo instead of GetSnapshot: the status entry is what ClearSnapshot nil-derefs when absent (control plane panic), and the GetSnapshot probe deep-cloned the entire snapshot just to test existence. route status GC: - Skip (and keep tracking) routes that still reference a live Gateway of a managed class: absence from the report is then likely a transient translation failure, and stripping status would wipe valid Accepted/ResolvedRefs conditions off attached, serving routes. - Read through mgr.GetAPIReader() so conflict retries actually see a fresh resourceVersion and a lagging informer cache can never hide our own just-written status (which would drop the route from tracking with its stale status left behind). - Collapse the three per-kind tracking fields and mutex into one kind-keyed map driven by a table. The GC only runs on the single status-sync goroutine, and a mutex alone would not make concurrent use safe anyway - correctness also depends on report ordering. - Simplify removeControllerRouteStatus with slices.DeleteFunc. Add a regression test covering the still-referenced-gateway guard. Signed-off-by: David L. Chandler --- ...-stale-xds-snapshots-and-route-status.yaml | 12 ++ .../gateway2/proxy_syncer/proxy_syncer.go | 198 +++++++++++++----- .../proxy_syncer/status_gc_client_test.go | 61 +++++- 3 files changed, 217 insertions(+), 54 deletions(-) create mode 100644 changelog/v1.22.0-beta11/gc-stale-xds-snapshots-and-route-status.yaml diff --git a/changelog/v1.22.0-beta11/gc-stale-xds-snapshots-and-route-status.yaml b/changelog/v1.22.0-beta11/gc-stale-xds-snapshots-and-route-status.yaml new file mode 100644 index 00000000000..6e7260b7c03 --- /dev/null +++ b/changelog/v1.22.0-beta11/gc-stale-xds-snapshots-and-route-status.yaml @@ -0,0 +1,12 @@ +changelog: + - type: FIX + issueLink: https://github.com/solo-io/solo-projects/issues/7086 + resolvesIssue: true + description: >- + Fix two garbage-collection gaps in the Kubernetes Gateway proxy syncer: + cached envoy xds snapshots are now evicted when the last stream for a + unique client disconnects (previously the snapshot cache grew unbounded + under client churn), and route status written by the controller is now + cleared from HTTPRoutes/TCPRoutes/TLSRoutes that detach from their + gateway (previously stale Accepted/ResolvedRefs conditions lingered + forever). diff --git a/projects/gateway2/proxy_syncer/proxy_syncer.go b/projects/gateway2/proxy_syncer/proxy_syncer.go index af03ca47981..82aaf6f580a 100644 --- a/projects/gateway2/proxy_syncer/proxy_syncer.go +++ b/projects/gateway2/proxy_syncer/proxy_syncer.go @@ -8,7 +8,6 @@ import ( "maps" "reflect" "slices" - "sync" "time" "go.uber.org/zap" @@ -107,13 +106,12 @@ type ProxySyncer struct { waitForSync []cache.InformerSynced - // statusGCMu guards the prev*RouteKeys sets used to garbage-collect stale - // route status (#7086). syncRouteStatus currently runs on a single - // goroutine; the mutex keeps tracking safe if that ever changes. - statusGCMu sync.Mutex - prevHTTPRouteKeys sets.Set[types.NamespacedName] - prevTCPRouteKeys sets.Set[types.NamespacedName] - prevTLSRouteKeys sets.Set[types.NamespacedName] + // prevRouteKeys tracks, per route kind, the routes most recently present in + // the status report, so stale route status can be garbage-collected (#7086). + // It must only be touched from the single status-sync goroutine in Start: + // a mutex alone would not make concurrent use safe, because correctness + // also depends on reports being applied in order. + prevRouteKeys map[string]sets.Set[types.NamespacedName] } type GatewayInputChannels struct { @@ -628,24 +626,47 @@ func (s *ProxySyncer) Start(ctx context.Context) error { } }() + // Evict the xds snapshot once the last stream for a unique client is gone + // (uniqueClients is refcounted per proxyKey, so a delete here really means + // no connected envoy still needs it; if a client with the same key + // reconnects, the snapshot is rebuilt and re-set). Without this, the xds + // SnapshotCache grows unbounded under client churn: each new + // role+labels+namespace key leaves a snapshot behind forever. See + // solo-projects#7086. + // + // Eviction deliberately does NOT hang off perclientSnapCollection deletes: + // that derived collection also emits deletes for still-connected clients + // whenever snapshotPerClient transiently returns nil (translation blips, + // the defer-building-snapshot hack), and clearing then would withdraw a + // live envoy's last coherent config and force-close its open watches. + s.uniqueClients.Register(func(o krt.Event[krtcollections.UniqlyConnectedClient]) { + if o.Event != controllers.EventDelete { + return + } + key := o.Latest().ResourceName() + // ClearSnapshot nil-derefs the cache's status entry when it is absent + // (the entry is created by the first CreateWatch and deleted only by + // ClearSnapshot itself), so guard on the status entry — not on + // GetSnapshot, which both checks the wrong map for that panic and + // deep-clones the entire snapshot just to test existence. + if s.proxyTranslator.xdsCache.GetStatusInfo(key) != nil { + s.proxyTranslator.xdsCache.ClearSnapshot(key) + } + }) + s.perclientSnapCollection.RegisterBatch(func(o []krt.Event[XdsSnapWrapper]) { for _, e := range o { - if e.Event != controllers.EventDelete { - snapWrap := e.Latest() - s.proxyTranslator.syncXds(ctx, snapWrap.snap, snapWrap.proxyKey) - } else { - // Evict the snapshot for a disconnected/obsolete unique client. - // Without this, the xds SnapshotCache grows unbounded under client - // churn (each new role+labels+namespace key leaves a snapshot behind - // forever). The delete event only fires once the last stream for this - // proxyKey is gone (uniqueClients is refcounted), so no connected envoy - // still needs it; if a client with the same key reconnects, the - // snapshot is rebuilt and re-set. See solo-projects#7086. - key := e.Latest().proxyKey - if _, err := s.proxyTranslator.xdsCache.GetSnapshot(key); err == nil { - s.proxyTranslator.xdsCache.ClearSnapshot(key) - } + if e.Event == controllers.EventDelete { + // Skip deletes: this derived collection emits them transiently for + // still-connected clients (snapshotPerClient returns nil during + // translation blips and the defer-building-snapshot hack), so a + // delete here does not mean the client is gone. Snapshot eviction + // for genuinely disconnected clients hangs off the uniqueClients + // delete handler registered above. + continue } + snapWrap := e.Latest() + s.proxyTranslator.syncXds(ctx, snapWrap.snap, snapWrap.proxyKey) } }, true) @@ -1027,17 +1048,33 @@ func (s *ProxySyncer) syncRouteStatus(ctx context.Context, rm reports.ReportMap) // (e.g. Accepted/ResolvedRefs) linger on a route forever once it detaches from // the gateway, since the normal sync path only iterates routes still in the // report. See https://github.com/solo-io/solo-projects/issues/7086. +// +// Must only be called from the single status-sync goroutine in Start (see the +// prevRouteKeys field comment). func (s *ProxySyncer) gcStaleRouteStatus(ctx context.Context, rm reports.ReportMap) { - s.statusGCMu.Lock() - defer s.statusGCMu.Unlock() + if s.prevRouteKeys == nil { + s.prevRouteKeys = make(map[string]sets.Set[types.NamespacedName], 3) + } + // Reads go through the API reader (not the informer cache): a cached Get + // could miss our own just-written status (dropping a route from tracking + // with its stale status left behind) and would defeat the fresh-Get retry + // on write conflicts. Writes still go through the manager client. The + // stale set is empty on steady-state syncs, so this adds no API load then. + reader := s.mgr.GetAPIReader() cl := s.mgr.GetClient() - s.prevHTTPRouteKeys = clearStaleRouteStatus(ctx, cl, s.controllerName, s.prevHTTPRouteKeys, sets.KeySet(rm.HTTPRoutes), - wellknown.HTTPRouteKind, func() client.Object { return new(gwv1.HTTPRoute) }) - s.prevTCPRouteKeys = clearStaleRouteStatus(ctx, cl, s.controllerName, s.prevTCPRouteKeys, sets.KeySet(rm.TCPRoutes), - wellknown.TCPRouteKind, func() client.Object { return new(gwv1a2.TCPRoute) }) - s.prevTLSRouteKeys = clearStaleRouteStatus(ctx, cl, s.controllerName, s.prevTLSRouteKeys, sets.KeySet(rm.TLSRoutes), - wellknown.TLSRouteKind, func() client.Object { return new(gwv1a2.TLSRoute) }) + for _, rt := range []struct { + kind string + current sets.Set[types.NamespacedName] + newObj func() client.Object + }{ + {wellknown.HTTPRouteKind, sets.KeySet(rm.HTTPRoutes), func() client.Object { return new(gwv1.HTTPRoute) }}, + {wellknown.TCPRouteKind, sets.KeySet(rm.TCPRoutes), func() client.Object { return new(gwv1a2.TCPRoute) }}, + {wellknown.TLSRouteKind, sets.KeySet(rm.TLSRoutes), func() client.Object { return new(gwv1a2.TLSRoute) }}, + } { + s.prevRouteKeys[rt.kind] = clearStaleRouteStatus(ctx, reader, cl, s.controllerName, s.allowedGatewayClasses, + s.prevRouteKeys[rt.kind], rt.current, rt.kind, rt.newObj) + } } // Retry settings for stale-status GC writes; vars (not consts) so tests can @@ -1047,11 +1084,18 @@ var ( statusGCRetryDelay = 100 * time.Millisecond ) +// errRouteStillReferenced signals that a route absent from the report still +// references a live Gateway of a class this syncer manages, so its absence is +// likely a transient translation failure rather than a detach; stripping +// status then would wipe valid conditions from an attached route. +var errRouteStillReferenced = errors.New("route still references a managed gateway") + // clearStaleRouteStatus removes controllerName's parent status from every route // in prev that is no longer in current. It returns the tracking set for the next -// round: routes still in current, plus any stale route whose cleanup could not -// be completed (e.g. a conflict that outlasts the in-line retry), so the next -// sync retries it instead of silently leaking stale status. +// round: routes still in the report, plus any stale route whose cleanup could +// not (yet) be completed — a still-standing managed-gateway reference, or a +// write failure that outlasts the in-line retry — so the next sync retries it +// instead of silently leaking stale status. // // Note: tracking is in-memory, so a crash between a route leaving the report and // its GC drops that route from tracking; such a route's stale status is not @@ -1059,8 +1103,10 @@ var ( // routes at startup and reclaiming ones bearing our controller's status. func clearStaleRouteStatus( ctx context.Context, + reader client.Reader, cl client.Client, controllerName string, + allowedGatewayClasses sets.Set[string], prev, current sets.Set[types.NamespacedName], routeType string, newObj func() client.Object, @@ -1068,19 +1114,25 @@ func clearStaleRouteStatus( logger := contextutils.LoggerFrom(ctx) // Start from the routes still in the report; re-add any we fail to clean. + // Clone rather than adopt: mutating the caller's set on failure would + // corrupt a reused current set (the failure path inserts into next). next := current.Clone() for rnn := range prev.Difference(current) { err := retry.Do(func() error { - // Re-Get inside the retry so each attempt has a fresh resourceVersion; - // otherwise a conflict just repeats forever. + // Re-Get inside the retry so each attempt has a fresh resourceVersion + // (reader bypasses the informer cache); otherwise a conflict just + // repeats forever. route := newObj() - if err := cl.Get(ctx, rnn, route); err != nil { + if err := reader.Get(ctx, rnn, route); err != nil { if apierrors.IsNotFound(err) { return nil // route deleted; status went with it } return err } + if routeStillReferencesManagedGateway(ctx, reader, route, allowedGatewayClasses) { + return errRouteStillReferenced + } if !removeControllerRouteStatus(route, controllerName) { return nil // we held no status on this route } @@ -1094,17 +1146,67 @@ func clearStaleRouteStatus( retry.RetryIf(apierrors.IsConflict), retry.LastErrorOnly(true), ) - if err != nil { + switch { + case errors.Is(err, errRouteStillReferenced): + logger.Debugw("route absent from report but still references a managed gateway; keeping status", + "type", routeType, "route", rnn) + next.Insert(rnn) + case err != nil: logger.Errorw("failed to clear stale route status; will retry next sync", "error", err, "type", routeType, "route", rnn) next.Insert(rnn) - continue + default: + logger.Debugf("cleared stale %s status from %s", routeType, rnn) } - logger.Debugf("cleared stale %s status from %s", routeType, rnn) } return next } +// routeStillReferencesManagedGateway reports whether the route's spec still has +// a parentRef to an existing Gateway whose class this syncer manages. A route +// can be missing from the report while still attached — most commonly when a +// transient translation failure drops its Gateway's proxy from the report — and +// its status must not be stripped then; it stays tracked and is re-evaluated on +// later syncs (the normal sync path re-adopts it once translation recovers). +func routeStillReferencesManagedGateway( + ctx context.Context, + reader client.Reader, + route client.Object, + allowedGatewayClasses sets.Set[string], +) bool { + var parentRefs []gwv1.ParentReference + switch r := route.(type) { + case *gwv1.HTTPRoute: + parentRefs = r.Spec.ParentRefs + case *gwv1a2.TCPRoute: + parentRefs = r.Spec.ParentRefs + case *gwv1a2.TLSRoute: + parentRefs = r.Spec.ParentRefs + default: + return false + } + for _, pr := range parentRefs { + if pr.Group != nil && string(*pr.Group) != gwv1.GroupName { + continue + } + if pr.Kind != nil && string(*pr.Kind) != wellknown.GatewayKind { + continue + } + ns := route.GetNamespace() + if pr.Namespace != nil { + ns = string(*pr.Namespace) + } + gw := new(gwv1.Gateway) + if err := reader.Get(ctx, types.NamespacedName{Namespace: ns, Name: string(pr.Name)}, gw); err != nil { + continue // gateway gone (or unreadable): not a reason to keep status + } + if allowedGatewayClasses.Has(string(gw.Spec.GatewayClassName)) { + return true + } + } + return false +} + // removeControllerRouteStatus removes RouteParentStatus entries owned by // controllerName. It returns true if the route's status was modified. func removeControllerRouteStatus(route client.Object, controllerName string) bool { @@ -1119,17 +1221,11 @@ func removeControllerRouteStatus(route client.Object, controllerName string) boo default: return false } - kept := make([]gwv1.RouteParentStatus, 0, len(rs.Parents)) - for _, p := range rs.Parents { - if string(p.ControllerName) != controllerName { - kept = append(kept, p) - } - } - if len(kept) == len(rs.Parents) { - return false - } - rs.Parents = kept - return true + before := len(rs.Parents) + rs.Parents = slices.DeleteFunc(rs.Parents, func(p gwv1.RouteParentStatus) bool { + return string(p.ControllerName) == controllerName + }) + return len(rs.Parents) != before } // syncGatewayStatus will build and update status for all Gateways in a reportMap diff --git a/projects/gateway2/proxy_syncer/status_gc_client_test.go b/projects/gateway2/proxy_syncer/status_gc_client_test.go index 68d030ddfa3..56ab6d18e9f 100644 --- a/projects/gateway2/proxy_syncer/status_gc_client_test.go +++ b/projects/gateway2/proxy_syncer/status_gc_client_test.go @@ -73,7 +73,7 @@ func TestClearStaleRouteStatus_TransientConflictThenSuccess(t *testing.T) { prev := sets.New(httpRouteKey()) current := sets.New[types.NamespacedName]() // route left the report - next := clearStaleRouteStatus(context.Background(), cl, testController, prev, current, + next := clearStaleRouteStatus(context.Background(), cl, cl, testController, sets.New[string](), prev, current, "HTTPRoute", newHTTPRouteObj) if next.Has(httpRouteKey()) { @@ -117,7 +117,7 @@ func TestClearStaleRouteStatus_PersistentErrorRetainsThenClears(t *testing.T) { current := sets.New[types.NamespacedName]() // First pass: write fails, route must be retained for retry next cycle. - next := clearStaleRouteStatus(context.Background(), cl, testController, prev, current, + next := clearStaleRouteStatus(context.Background(), cl, cl, testController, sets.New[string](), prev, current, "HTTPRoute", newHTTPRouteObj) if !next.Has(httpRouteKey()) { t.Fatalf("route should be retained in tracking after a failed cleanup") @@ -129,7 +129,7 @@ func TestClearStaleRouteStatus_PersistentErrorRetainsThenClears(t *testing.T) { // Next sync cycle: writes succeed; the retained route should now be cleared // and dropped from tracking. failWrites = false - next2 := clearStaleRouteStatus(context.Background(), cl, testController, next, current, + next2 := clearStaleRouteStatus(context.Background(), cl, cl, testController, sets.New[string](), next, current, "HTTPRoute", newHTTPRouteObj) if next2.Has(httpRouteKey()) { t.Fatalf("route should be cleaned and dropped from tracking on the retry cycle") @@ -143,3 +143,58 @@ func TestClearStaleRouteStatus_PersistentErrorRetainsThenClears(t *testing.T) { t.Fatalf("expected our parent status cleared on retry cycle, got %d parents", len(got.Status.Parents)) } } + +// A route absent from the report but still referencing a live Gateway of a +// managed class is likely mid-translation-blip, not detached: its status must +// be kept and the route retained in tracking. Once the Gateway is gone, the +// next cycle cleans it up. +func TestClearStaleRouteStatus_StillReferencedGatewayKeepsStatus(t *testing.T) { + const className = "test-gateway-class" + + route := staleHTTPRoute() + route.Spec.ParentRefs = []gwv1.ParentReference{{Name: "gw"}} + gateway := &gwv1.Gateway{ + ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "gw"}, + Spec: gwv1.GatewaySpec{GatewayClassName: className}, + } + + cl := fake.NewClientBuilder(). + WithScheme(schemes.GatewayScheme()). + WithObjects(route, gateway). + WithStatusSubresource(&gwv1.HTTPRoute{}). + Build() + + allowed := sets.New(className) + prev := sets.New(httpRouteKey()) + current := sets.New[types.NamespacedName]() // route left the report... + + next := clearStaleRouteStatus(context.Background(), cl, cl, testController, allowed, prev, current, + "HTTPRoute", newHTTPRouteObj) + if !next.Has(httpRouteKey()) { + t.Fatalf("route referencing a live managed gateway should stay tracked") + } + got := &gwv1.HTTPRoute{} + if err := cl.Get(context.Background(), httpRouteKey(), got); err != nil { + t.Fatalf("get: %v", err) + } + if len(got.Status.Parents) != 1 { + t.Fatalf("status must not be stripped while the managed gateway exists; got %d parents", len(got.Status.Parents)) + } + + // Gateway deleted: the reference no longer resolves, so the next cycle + // clears the stale status and drops the route from tracking. + if err := cl.Delete(context.Background(), gateway); err != nil { + t.Fatalf("delete gateway: %v", err) + } + next2 := clearStaleRouteStatus(context.Background(), cl, cl, testController, allowed, next, current, + "HTTPRoute", newHTTPRouteObj) + if next2.Has(httpRouteKey()) { + t.Fatalf("route should be cleaned and dropped from tracking once the gateway is gone") + } + if err := cl.Get(context.Background(), httpRouteKey(), got); err != nil { + t.Fatalf("get: %v", err) + } + if len(got.Status.Parents) != 0 { + t.Fatalf("expected our parent status cleared after gateway deletion, got %d parents", len(got.Status.Parents)) + } +} From 0760f614a2a37b6446f75c949ef2044e3152b4c3 Mon Sep 17 00:00:00 2001 From: changelog-bot Date: Fri, 31 Jul 2026 18:03:32 +0000 Subject: [PATCH 03/10] Adding changelog file to new location --- .../gc-stale-xds-snapshots-and-route-status.yaml | 12 ++++++++++++ 1 file changed, 12 insertions(+) create mode 100644 changelog/v1.22.0-beta12/gc-stale-xds-snapshots-and-route-status.yaml diff --git a/changelog/v1.22.0-beta12/gc-stale-xds-snapshots-and-route-status.yaml b/changelog/v1.22.0-beta12/gc-stale-xds-snapshots-and-route-status.yaml new file mode 100644 index 00000000000..6e7260b7c03 --- /dev/null +++ b/changelog/v1.22.0-beta12/gc-stale-xds-snapshots-and-route-status.yaml @@ -0,0 +1,12 @@ +changelog: + - type: FIX + issueLink: https://github.com/solo-io/solo-projects/issues/7086 + resolvesIssue: true + description: >- + Fix two garbage-collection gaps in the Kubernetes Gateway proxy syncer: + cached envoy xds snapshots are now evicted when the last stream for a + unique client disconnects (previously the snapshot cache grew unbounded + under client churn), and route status written by the controller is now + cleared from HTTPRoutes/TCPRoutes/TLSRoutes that detach from their + gateway (previously stale Accepted/ResolvedRefs conditions lingered + forever). From b74bc4be35a3a265c7d89ec5b25b35046f2a5d75 Mon Sep 17 00:00:00 2001 From: changelog-bot Date: Fri, 31 Jul 2026 18:03:33 +0000 Subject: [PATCH 04/10] Deleting changelog file from old location --- .../gc-stale-xds-snapshots-and-route-status.yaml | 12 ------------ 1 file changed, 12 deletions(-) delete mode 100644 changelog/v1.22.0-beta11/gc-stale-xds-snapshots-and-route-status.yaml diff --git a/changelog/v1.22.0-beta11/gc-stale-xds-snapshots-and-route-status.yaml b/changelog/v1.22.0-beta11/gc-stale-xds-snapshots-and-route-status.yaml deleted file mode 100644 index 6e7260b7c03..00000000000 --- a/changelog/v1.22.0-beta11/gc-stale-xds-snapshots-and-route-status.yaml +++ /dev/null @@ -1,12 +0,0 @@ -changelog: - - type: FIX - issueLink: https://github.com/solo-io/solo-projects/issues/7086 - resolvesIssue: true - description: >- - Fix two garbage-collection gaps in the Kubernetes Gateway proxy syncer: - cached envoy xds snapshots are now evicted when the last stream for a - unique client disconnects (previously the snapshot cache grew unbounded - under client churn), and route status written by the controller is now - cleared from HTTPRoutes/TCPRoutes/TLSRoutes that detach from their - gateway (previously stale Accepted/ResolvedRefs conditions lingered - forever). From 9973d004a20a7a5667b001a06b89f427eed5fca4 Mon Sep 17 00:00:00 2001 From: changelog-bot Date: Sun, 2 Aug 2026 23:12:23 +0000 Subject: [PATCH 05/10] Adding changelog file to new location --- .../gc-stale-xds-snapshots-and-route-status.yaml | 12 ++++++++++++ 1 file changed, 12 insertions(+) create mode 100644 changelog/v1.22.0-beta13/gc-stale-xds-snapshots-and-route-status.yaml diff --git a/changelog/v1.22.0-beta13/gc-stale-xds-snapshots-and-route-status.yaml b/changelog/v1.22.0-beta13/gc-stale-xds-snapshots-and-route-status.yaml new file mode 100644 index 00000000000..6e7260b7c03 --- /dev/null +++ b/changelog/v1.22.0-beta13/gc-stale-xds-snapshots-and-route-status.yaml @@ -0,0 +1,12 @@ +changelog: + - type: FIX + issueLink: https://github.com/solo-io/solo-projects/issues/7086 + resolvesIssue: true + description: >- + Fix two garbage-collection gaps in the Kubernetes Gateway proxy syncer: + cached envoy xds snapshots are now evicted when the last stream for a + unique client disconnects (previously the snapshot cache grew unbounded + under client churn), and route status written by the controller is now + cleared from HTTPRoutes/TCPRoutes/TLSRoutes that detach from their + gateway (previously stale Accepted/ResolvedRefs conditions lingered + forever). From 44d9c9f0a7e812a1ac4e91ad385f1cfb5ac9cf55 Mon Sep 17 00:00:00 2001 From: changelog-bot Date: Sun, 2 Aug 2026 23:12:23 +0000 Subject: [PATCH 06/10] Deleting changelog file from old location --- .../gc-stale-xds-snapshots-and-route-status.yaml | 12 ------------ 1 file changed, 12 deletions(-) delete mode 100644 changelog/v1.22.0-beta12/gc-stale-xds-snapshots-and-route-status.yaml diff --git a/changelog/v1.22.0-beta12/gc-stale-xds-snapshots-and-route-status.yaml b/changelog/v1.22.0-beta12/gc-stale-xds-snapshots-and-route-status.yaml deleted file mode 100644 index 6e7260b7c03..00000000000 --- a/changelog/v1.22.0-beta12/gc-stale-xds-snapshots-and-route-status.yaml +++ /dev/null @@ -1,12 +0,0 @@ -changelog: - - type: FIX - issueLink: https://github.com/solo-io/solo-projects/issues/7086 - resolvesIssue: true - description: >- - Fix two garbage-collection gaps in the Kubernetes Gateway proxy syncer: - cached envoy xds snapshots are now evicted when the last stream for a - unique client disconnects (previously the snapshot cache grew unbounded - under client churn), and route status written by the controller is now - cleared from HTTPRoutes/TCPRoutes/TLSRoutes that detach from their - gateway (previously stale Accepted/ResolvedRefs conditions lingered - forever). From 8c750c07172d47a3bc463a2917f1a2b777d60d74 Mon Sep 17 00:00:00 2001 From: changelog-bot Date: Tue, 4 Aug 2026 01:01:07 +0000 Subject: [PATCH 07/10] Adding changelog file to new location --- .../gc-stale-xds-snapshots-and-route-status.yaml | 12 ++++++++++++ 1 file changed, 12 insertions(+) create mode 100644 changelog/v1.22.1/gc-stale-xds-snapshots-and-route-status.yaml diff --git a/changelog/v1.22.1/gc-stale-xds-snapshots-and-route-status.yaml b/changelog/v1.22.1/gc-stale-xds-snapshots-and-route-status.yaml new file mode 100644 index 00000000000..6e7260b7c03 --- /dev/null +++ b/changelog/v1.22.1/gc-stale-xds-snapshots-and-route-status.yaml @@ -0,0 +1,12 @@ +changelog: + - type: FIX + issueLink: https://github.com/solo-io/solo-projects/issues/7086 + resolvesIssue: true + description: >- + Fix two garbage-collection gaps in the Kubernetes Gateway proxy syncer: + cached envoy xds snapshots are now evicted when the last stream for a + unique client disconnects (previously the snapshot cache grew unbounded + under client churn), and route status written by the controller is now + cleared from HTTPRoutes/TCPRoutes/TLSRoutes that detach from their + gateway (previously stale Accepted/ResolvedRefs conditions lingered + forever). From e170a00813fa69e8dfedcc9f7bbb7a297f246629 Mon Sep 17 00:00:00 2001 From: changelog-bot Date: Tue, 4 Aug 2026 01:01:08 +0000 Subject: [PATCH 08/10] Deleting changelog file from old location --- .../gc-stale-xds-snapshots-and-route-status.yaml | 12 ------------ 1 file changed, 12 deletions(-) delete mode 100644 changelog/v1.22.0-beta13/gc-stale-xds-snapshots-and-route-status.yaml diff --git a/changelog/v1.22.0-beta13/gc-stale-xds-snapshots-and-route-status.yaml b/changelog/v1.22.0-beta13/gc-stale-xds-snapshots-and-route-status.yaml deleted file mode 100644 index 6e7260b7c03..00000000000 --- a/changelog/v1.22.0-beta13/gc-stale-xds-snapshots-and-route-status.yaml +++ /dev/null @@ -1,12 +0,0 @@ -changelog: - - type: FIX - issueLink: https://github.com/solo-io/solo-projects/issues/7086 - resolvesIssue: true - description: >- - Fix two garbage-collection gaps in the Kubernetes Gateway proxy syncer: - cached envoy xds snapshots are now evicted when the last stream for a - unique client disconnects (previously the snapshot cache grew unbounded - under client churn), and route status written by the controller is now - cleared from HTTPRoutes/TCPRoutes/TLSRoutes that detach from their - gateway (previously stale Accepted/ResolvedRefs conditions lingered - forever). From 9e6b449aae43a46422e09d62cfd9e7568443dd62 Mon Sep 17 00:00:00 2001 From: changelog-bot Date: Tue, 4 Aug 2026 22:04:52 +0000 Subject: [PATCH 09/10] Adding changelog file to new location --- .../gc-stale-xds-snapshots-and-route-status.yaml | 12 ++++++++++++ 1 file changed, 12 insertions(+) create mode 100644 changelog/v1.22.2/gc-stale-xds-snapshots-and-route-status.yaml diff --git a/changelog/v1.22.2/gc-stale-xds-snapshots-and-route-status.yaml b/changelog/v1.22.2/gc-stale-xds-snapshots-and-route-status.yaml new file mode 100644 index 00000000000..6e7260b7c03 --- /dev/null +++ b/changelog/v1.22.2/gc-stale-xds-snapshots-and-route-status.yaml @@ -0,0 +1,12 @@ +changelog: + - type: FIX + issueLink: https://github.com/solo-io/solo-projects/issues/7086 + resolvesIssue: true + description: >- + Fix two garbage-collection gaps in the Kubernetes Gateway proxy syncer: + cached envoy xds snapshots are now evicted when the last stream for a + unique client disconnects (previously the snapshot cache grew unbounded + under client churn), and route status written by the controller is now + cleared from HTTPRoutes/TCPRoutes/TLSRoutes that detach from their + gateway (previously stale Accepted/ResolvedRefs conditions lingered + forever). From 6e54c42987f5ace9361eaed647dd0255d8ec23c9 Mon Sep 17 00:00:00 2001 From: changelog-bot Date: Tue, 4 Aug 2026 22:04:53 +0000 Subject: [PATCH 10/10] Deleting changelog file from old location --- .../gc-stale-xds-snapshots-and-route-status.yaml | 12 ------------ 1 file changed, 12 deletions(-) delete mode 100644 changelog/v1.22.1/gc-stale-xds-snapshots-and-route-status.yaml diff --git a/changelog/v1.22.1/gc-stale-xds-snapshots-and-route-status.yaml b/changelog/v1.22.1/gc-stale-xds-snapshots-and-route-status.yaml deleted file mode 100644 index 6e7260b7c03..00000000000 --- a/changelog/v1.22.1/gc-stale-xds-snapshots-and-route-status.yaml +++ /dev/null @@ -1,12 +0,0 @@ -changelog: - - type: FIX - issueLink: https://github.com/solo-io/solo-projects/issues/7086 - resolvesIssue: true - description: >- - Fix two garbage-collection gaps in the Kubernetes Gateway proxy syncer: - cached envoy xds snapshots are now evicted when the last stream for a - unique client disconnects (previously the snapshot cache grew unbounded - under client churn), and route status written by the controller is now - cleared from HTTPRoutes/TCPRoutes/TLSRoutes that detach from their - gateway (previously stale Accepted/ResolvedRefs conditions lingered - forever).