fix: support status reporting and stale status cleanup for GRPCRoute, TCPRoute, and TLSRoute#14294
Conversation
… TCPRoute, and TLSRoute Signed-off-by: ApurveKaranwal <[email protected]>
6c25db8 to
8171154
Compare
There was a problem hiding this comment.
Pull request overview
This PR fixes stale/orphaned status handling for non-HTTP Gateway API routes by extending the status-marker mechanism to GRPCRoute, TCPRoute, and TLSRoute, and by ensuring TLSRoute version normalization preserves Status so marker-based cleanup can work reliably. This fits into kgateway’s status reporting pipeline by making the ProxySyncer’s merged report generation capable of explicitly clearing outdated Route status entries.
Changes:
- Extended
RoutesIndexto track status markers and IR collections for GRPCRoute/TCPRoute/TLSRoute, and to emit “empty” reports for marked routes that weren’t translated (stale cleanup). - Updated route transforms to produce status markers when existing Route status contains this controller’s
ControllerName. - Preserved TLSRoute
Status.RouteStatusduringv1 -> v1alpha2conversion and added/updated unit tests validating the behavior.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/pluginsdk/collections/tlsroute_version.go | Preserve TLSRoute Status.RouteStatus when converting gwv1.TLSRoute to gwv1a2.TLSRoute. |
| pkg/pluginsdk/collections/tlsroute_version_test.go | Add assertions ensuring TLSRoute status survives version conversion. |
| pkg/krtcollections/policy.go | Add GRPC/TCP/TLS status-marker collections, marker processing, and update route transforms to return markers. |
| pkg/krtcollections/policy_test.go | Add unit tests covering empty-report generation, preservation of existing reports, and marker collection inclusion for stale routes. |
| pkg/krtcollections/grpc_route.go | Update GRPCRoute transform to return a status marker when existing status matches controller. |
| pkg/kgateway/proxy_syncer/proxy_syncer.go | Invoke GRPC/TCP/TLS marker processing during merged status report generation (stale cleanup path). |
Signed-off-by: ApurveKaranwal <[email protected]>
|
@davidjumani sir, please retrigger the CI checks. |
| } | ||
| } | ||
| return h.httpRoutes.HasSynced() && h.routes.HasSynced() && h.policies.HasSynced() && h.backends.HasSynced() && h.refgrants.HasSynced() | ||
| return h.httpRoutes.HasSynced() && h.grpcRoutes.HasSynced() && h.tcpRoutes.HasSynced() && h.tlsRoutes.HasSynced() && h.routes.HasSynced() && h.policies.HasSynced() && h.backends.HasSynced() && h.refgrants.HasSynced() |
There was a problem hiding this comment.
You dont need these hassynced and the collections, these are already added to hassync at L1162. HTTP is special because its standalone IR.
There was a problem hiding this comment.
Thanks @alexliu541 sir! Reverted the redundant HasSynced checks since all these route collections are joined into h.routes (which is already checked in the return statement), making the individual checks unnecessary.
| rp.Route(status.Obj) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
lots of repeated code here, can we reuse? something like
// ProcessRouteStatusMarkers adds empty status in the report map for any marked route (HTTP, TCP,
// TLS, GRPC) that has no status reported. Used for clearing stale status for orphaned routes.
func (r *RoutesIndex) ProcessRouteStatusMarkers(kctx krt.HandlerContext, reportMap reports.ReportMap) {
rp := reports.NewReporter(&reportMap)
processRouteStatusMarkers(kctx, r.httpRouteStatusMarkers, reportMap.HTTPRoutes, rp)
processRouteStatusMarkers(kctx, r.tcpRouteStatusMarker, reportMap.TCPRoutes, rp)
processRouteStatusMarkers(kctx, r.tlsRouteStatusMarker, reportMap.TLSRoutes, rp)
processRouteStatusMarkers(kctx, r.grpcRouteStatusMarker, reportMap.GRPCRoutes, rp)
}
// processRouteStatusMarkers fetches the route status markers and adds empty status for routes not
// in the report map to clear stale status.
func processRouteStatusMarkers[T controllers.Object](
kctx krt.HandlerContext,
statusCol krt.StatusCollection[T, StatusMarker],
reportMap map[types.NamespacedName]*reports.RouteReport,
rp reporter.Reporter,
) {
objStatus := krt.Fetch(kctx, statusCol)
for _, status := range objStatus {
routeKey := types.NamespacedName{
Namespace: status.Obj.GetNamespace(),
Name: status.Obj.GetName(),
}
// Add empty status to clear stale status for routes with no valid ParentRefs
if reportMap[routeKey] == nil {
_ = rp.Route(status.Obj)
}
}
}
Then proxysyncer can just call ProcessRouteStatusMarkers, don't even need the getter for the statusmarkers.
There was a problem hiding this comment.
I had these locally for quite sometime, got carried away by other things lol
There was a problem hiding this comment.
Great suggestion @alexliu541 sir! I consolidated the status marker processing into a generic helper function (processRouteStatusMarkers), removed all the individual getters, and exposed a single ProcessRouteStatusMarkers(kctx, reportMap) method on RoutesIndex. The proxy syncer has been simplified to use this single call, and the unit tests have been updated and simplified to match.
alexliu541
left a comment
There was a problem hiding this comment.
Thanks for the quick response, one more minor improvement
| tcpRoutesCollection := krt.NewCollection(tcproutes, func(kctx krt.HandlerContext, i *gwv1a2.TCPRoute) *RouteWrapper { | ||
| t := h.transformTcpRoute(kctx, i) | ||
| return &RouteWrapper{Route: t} | ||
| h.grpcRouteStatusMarkers, h.grpcRoutes = krt.NewStatusCollection(grpcroutes, func(kctx krt.HandlerContext, i *gwv1.GRPCRoute) (*StatusMarker, *ir.HttpRouteIR) { |
There was a problem hiding this comment.
after the hassync is removed, I dont think you need the h.grpcRoutes, just local var for h.routes collection should be enough. something like
var tcpRoutesCollection, tlsRoutesCollection, grpcRoutesCollection krt.Collection[RouteWrapper]
h.tcpRouteStatusMarker, tcpRoutesCollection = krt.NewStatusCollection(tcproutes, func(kctx krt.HandlerContext, i *gwv1a2.TCPRoute) (*StatusMarker, *RouteWrapper) {
status, route := h.transformTcpRoute(kctx, i, controllerName)
return status, &RouteWrapper{Route: route}
}, krtopts.ToOptions("routes-tcp-routes-with-policy")...)
h.tlsRouteStatusMarker, tlsRoutesCollection = krt.NewStatusCollection(tlsroutes, func(kctx krt.HandlerContext, i *gwv1a2.TLSRoute) (*StatusMarker, *RouteWrapper) {
status, route := h.transformTlsRoute(kctx, i, controllerName)
return status, &RouteWrapper{Route: route}
}, krtopts.ToOptions("routes-tls-routes-with-policy")...)
h.grpcRouteStatusMarker, grpcRoutesCollection = krt.NewStatusCollection(grpcroutes, func(kctx krt.HandlerContext, i *gwv1.GRPCRoute) (*StatusMarker, *RouteWrapper) {
status, route := h.transformGRPCRoute(kctx, i, controllerName)
return status, &RouteWrapper{Route: route}
}, krtopts.ToOptions("routes-grpc-routes-with-policy")...)
There was a problem hiding this comment.
Thanks @alexliu541 sir! I've simplified it exactly as suggested. I removed the redundant grpcRoutes, tcpRoutes, and tlsRoutes fields from RoutesIndex, combined the status and wrapper collection creation into a single NewStatusCollection call, and used local variables for the collections.
Signed-off-by: ApurveKaranwal <[email protected]>
Head branch was pushed to by a user without write access
alexliu541
left a comment
There was a problem hiding this comment.
LGTM, thanks for the PR!
|
hey @davidjumani sir, please retrigger the CI checks. |
… TCPRoute, and TLSRoute (kgateway-dev#14294) Signed-off-by: ApurveKaranwal <[email protected]> Co-authored-by: David Jumani <[email protected]>
Description
kgatewaywas not cleaning up stale status reports for detached/orphanedGRPCRoute,TCPRoute, andTLSRouteresources when their parent references were removed, leaving outdated status entries. Additionally, version conversions forTLSRoutewere dropping theStatusfield, preventing status markers from checking existing status.krt.StatusCollectiontracking inRoutesIndex(policy.go) to GRPCRoute, TCPRoute, and TLSRoute.Process*StatusMarkersfor GRPCRoute, TCPRoute, and TLSRoute to generate empty status reports to clear stale status entries for detached routes.Statusfield during API version promotions (v1 -> v1alpha2andv1alpha3 -> v1alpha2) insidetlsroute_version.go.policy_test.goandtlsroute_version_test.goto verify correct behavior.Change Type
/kind fix
Changelog