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)) + } + }) +}