fix: decouple AttachedRoutes from gateway translation - #2
Open
puertomontt wants to merge 42 commits into
Open
Conversation
Port the agentgateway status-writing machinery: a worker pool (derived from istio's pilot/pkg/status/resourcelock.go) that executes at most one concurrent write per resource with latest-data-wins coalescing, StatusCollections with SetQueue/UnsetQueue so desired statuses are computed on every pod but only flow to the write queue on the leader, and a generic Writer that reads the current object from the shared istio informer cache, merges multi-writer fields (route parents, policy ancestors, gateway addresses) owned by other controllers, and writes via UpdateStatus with optimistic concurrency. Conflicted or dropped writes self-heal without retry bookkeeping: the status collection re-enqueues a resource whenever its live status still differs from the desired status after the informer delivers the newer object. Signed-off-by: omar <omar.hammami@solo.io>
Replace the report-map sweep StatusSyncer with per-object desired-status collections and a worker-pool writer, following the agentgateway design. The old syncer computed reports from the istio/KRT cache but read objects back through the controller-runtime manager cache (and policies through a third path, per-plugin kclient hooks) before every status write. The skew between those caches required point fixes like stamping observedGeneration from the report and retrying NotFound on freshly created resources. Status now flows through a single cache: derived KRT collections join the raw informer-backed objects with the merged translation reports to produce ObjectWithStatus pairs, and events only reach the write queue when the live status differs from the desired status. Writes go through the same istio informers via kclient, so the NotFound race is structurally gone and conflicts self-heal through informer events instead of retry loops. Details: - Gateway, HTTPRoute/GRPCRoute/TCPRoute/TLSRoute, ListenerSet (promoted and legacy XListenerSet), and Backend statuses are derived in proxy_syncer's new status collections; TCP/TLS route write versions are resolved once at startup from CRD discovery instead of per-write GET fallbacks. - Policy plugins register desired-status collections via the new PolicyPlugin.RegisterPolicyStatus hook and a shared pluginutils helper, replacing the GetPolicyStatus/PatchPolicyStatus/BuildPolicyStatus hooks. Policy statuses are built from the union of the gateway and backend report paths, fixing a race where two goroutines wrote competing statuses for a policy attached via both. - Leader election is unchanged externally (StatusSyncer is still a leader-gated runnable) but internally gates only the write queue: desired statuses are computed on all pods and leadership acquisition replays the full current state through krt registration. - Multi-writer status fields (route parents, policy ancestors, gateway addresses) are merged at write time from the freshest informer state, preserving entries owned by other controllers. - Status sync metrics move to the statussync package with per-kind hooks preserving the existing labels. Signed-off-by: omar <omar.hammami@solo.io>
Resource included the object's resourceVersion and is used as the work queue's coalescing key, so every informer update minted a new key: updates for the same object were not coalesced and two workers could write the same object concurrently (optimistic concurrency kept this correct, but with avoidable conflict churn). Drop resourceVersion from Resource so the queue keys on GVK + NamespacedName; writers already read the current resourceVersion from the informer cache at write time. Signed-off-by: omar <omar.hammami@solo.io>
…tatus write retries Signed-off-by: omar <omar.hammami@solo.io>
…-path coverage The v1->v1alpha2 TCPRoute conversion dropped Status, so the declarative status writer saw a perpetually empty live status and re-enqueued a (writer-suppressed) write on every TCPRoute or report event. Also extract the TCP/TLSRoute status write GVR resolution into pure functions with table tests, and add an end-to-end enqueue/write/no-op cycle test for the statussync collection -> queue -> writer path. Signed-off-by: omar <omar.hammami@solo.io>
Nothing reads through the manager's cache-backed client anymore (the status syncer was the last reader), so the cache holds no informers and the WaitForCacheSync call was a no-op. Remove it along with ProxySyncer's now-unused manager reference. Signed-off-by: omar <omar.hammami@solo.io>
MergePolicyAncestorStatuses restored all foreign ancestors after BuildPolicyStatus had truncated to 16, so a policy whose live status carries 16 foreign ancestors produced a 17-entry write that the API server rejects (MaxItems=16) on every attempt, without self-healing. Cap the merged list in the merge helpers themselves (policy ancestors at 16, route parents at 32) so desired-status normalization stays byte-identical to the written result; foreign entries are never dropped in favor of ours. Signed-off-by: omar <omar.hammami@solo.io>
The custom ListenerSet syncer attempted its write once; a transient 429/5xx/network error marked the queue item done with nothing on the informer guaranteed to re-enqueue it, leaving status stale indefinitely. Extract the generic writer's retry policy into an exported RetryStatusWrite helper and wrap the ListenerSet write (both promoted and legacy paths) in it. Signed-off-by: omar <omar.hammami@solo.io>
…e scope Four fixes from review of the declarative status collection switchover. 1. Stop reordering Gateway status.addresses. MergeGatewayAddresses sorted by (type, value), but the deployer decides whether to write using an order-sensitive slices.Equal against the live list and builds that list in source order (LoadBalancer ingress order, then Service ClusterIPs order, then spec.addresses order). Whenever sorted order differed from the deployer's -- an LB with two ingress IPs, IPv6-primary dual-stack, a Hostname-typed spec address alongside a ClusterIP -- the two controllers never agreed: the deployer rewrote its order on every reconcile and we rewrote ours, flip-flopping a user-visible field with two redundant status writes each time. BuildGWStatus already carries the live addresses through verbatim, so the writer now just takes them from the freshest informer read (which also stops a stale desired snapshot from reverting a concurrent deployer update) and MergeGatewayAddresses is gone. 2. Make status collection removal semantics explicit per kind via RemovalPolicy. RegisterStatus computed an empty desired status on delete but ran the suppression check against the pre-removal desired, so whether a stale status got cleared depended on whether the last write had landed. Worse, an empty desired status wipes every condition for single-writer statuses, where the old syncer left objects absent from the report untouched. Routes and policies now clear on removal (status.parents and status.ancestors are multi-writer, so the merge drops only our entries), Gateway/ListenerSet/Backend keep, and the equality check moved after the removal handling so the outcome is deterministic. 3. Add reports.MergePolicyReports so the policy status singleton merges only Policies. MergeReportMaps deep-cloned every gateway, listener set, route and backend report on every translation event -- and made the singleton's Equals compare them -- to serve statuses built from Policies alone. 4. Derive status sync error metrics only from our own status entries. OnSync receives the merged status, so another controller's Accepted=False could flip our result="error" counter for routes and policies. Signed-off-by: omar <omar.hammami@solo.io>
Signed-off-by: omar <omar.hammami@solo.io>
Signed-off-by: omar <omar.hammami@solo.io>
Signed-off-by: omar <omar.hammami@solo.io>
Signed-off-by: omar <omar.hammami@solo.io>
Signed-off-by: omar <omar.hammami@solo.io>
Signed-off-by: omar <omar.hammami@solo.io>
Signed-off-by: omar <omar.hammami@solo.io>
Signed-off-by: omar <omar.hammami@solo.io>
Signed-off-by: omar <omar.hammami@solo.io>
Signed-off-by: omar <omar.hammami@solo.io>
Signed-off-by: omar <omar.hammami@solo.io>
Signed-off-by: omar <omar.hammami@solo.io>
Signed-off-by: omar <omar.hammami@solo.io>
…tion footguns Address review feedback on the keyed status contribution work. Resolve TCP/TLSRoute write versions to a candidate list instead of a single startup guess. When CRD discovery is authoritative the answer is still exactly one version. When it is not - the API server was unavailable, or the CRD is not installed yet, the case the watch path already self-heals through delayed informers - every version we also watch becomes a candidate, and the new statussync.NewFirstPresentSyncer dispatches each write to the first candidate whose informer actually holds the object. Previously a CRD installed later that served only v1alpha2 sent every write through the never-served v1 client, whose Get returns nil, so ApplyStatus skipped silently and no TCP/TLS route carried status until the pod restarted. Both discovery-failure paths now warn, and the TCP/TLS degenerate-fallback asymmetry is resolved in favour of the promoted version. Only end a resource's status sync when the write actually landed. A write that failed after every retry left the resource with a stale status, but still cleared the resources-out-of-sync signal. The new statussync.EndResourceStatusSyncOnWriteSuccess gates on the write error alone, so a status carrying invalid conditions is still counted as synced. Move report cache-sync tracking into statussync.RegisterResourceReports, so calling it directly can no longer bypass the sync barrier, and drop the bypassable wrapper closures from PolicyStatusInputs and StatusRegistrationInputs. Thread the controller's root context into the default policy status builder instead of context.Background(). Make BuildPolicyStatus's ancestor cap ownership-aware so it agrees with MergePolicyAncestorStatuses: foreign ancestors are never dropped in favour of ours. The two caps previously disagreed, letting the first one decide which of our entries survived and defeating the second's documented guarantee. Nits: keep the first writer registered for a GVK and report duplicates instead of last-wins; only error when a registered Backend plugin is missing RawBackends; rename the misleading tlsutils alias in backendtlspolicy; drop the dead policyPlugins params in test/translator. Add tests for the seams this work introduced: pluginutils.RegisterPolicyStatus (builder selection, nil-report clearing, condition-error metric), Writer.ApplyStatus conflict/NotFound/transient paths, WorkerPool concurrency guarantees at the pool level, and ReduceStatusContributions determinism. status_cycle_test.go now drives the real route status builder and asserts the written status is a fixed point of it, rather than assuming builders are normalization-stable. Signed-off-by: omar <omar.hammami@solo.io>
A writer reads through a delayed client, whose Get returns nil until its own informer has been swapped in. That readiness is independent of the raw collection that enqueued the resource: kclient builds a separate delayed wrapper per client, and a delayed client's HasSynced reports true while Get still returns nil, so neither the status syncer's cache-sync barrier nor the HasResource probe can tell "my informer is not ready" from "the object was deleted". Nothing upstream re-fires once the writer's informer does load - the raw collection has already delivered the object and the report reducer has no new reduction to emit - so the resource silently carried no status until something unrelated touched it. That is the same late-CRD-installation outage the versioned writer dispatch was added to prevent, reached by a different route, and it applied to every delayed writer client rather than just TCP/TLS routes. Add NotReadyRequeuer, which re-queues a resource no client could see, backing off from 500ms and capped at 6 attempts so a resource genuinely deleted between enqueue and write - or an API version no cluster ever serves - stops being retried. Writers hand invisible resources to it and clear the budget on any visible pass. StatusCollections.Requeue supplies the schedule hook and drops requeues while this replica is not the leader, since acquiring leadership replays every resource anyway. The dispatcher's "no candidate holds it" fall-through already delegates to the preferred writer, which now makes that path recoverable rather than terminal. Also correct EndResourceStatusSyncOnWriteSuccess's doc comment: it closes a sync when no retry is pending, not when the status was persisted. Writer deliberately reports conflicts as success, so a conflicting write ends the sync without having written anything. That is sound - a conflict means the API server holds a newer resourceVersion, whose delivery both re-enqueues the resource and starts a fresh sync - but the previous wording claimed a guarantee the code does not make. Separating the two would need a persisted/deferred outcome on the OnSync callback; the residual today is a bounded skew in the sync counters, never an unwritten status. Signed-off-by: omar <omar.hammami@solo.io>
Route version handling modelled a pile of intermediate facts - Promoted, PreV1, PreferredPreV1GVR - and then derived watch and write decisions from them separately. The structs permitted contradictory states (PreV1 was redundant with PreferredPreV1GVR, and tlsRouteWriteGVRs carried a defensive Empty() check because of it), and the two decision trees could disagree. They did disagree. With authoritative discovery, a CRD serving only v1alpha2, and experimental Gateway API features disabled, the watch path built no pre-v1 informer while the write path returned the v1alpha2 GVR regardless: the authoritative branch of tcpRouteWriteGVRs and its TLS twin ignored their watchPreV1 argument entirely. Since that version is served, the writer's delayed client did start, giving a full watch and cache of a resource we had deliberately chosen not to watch. Nothing broke - the raw collection stays empty so no write is ever attempted - but it is a wasted informer and exactly the watch/write divergence this area keeps reproducing. Replace both decision trees with one selector over an ordered list of known versions. Discovery now records only the raw fact (was the CRD readable, and which versions does it serve), and selectRouteGVRs turns that plus the experimental flag into the list of versions to use. setup.go builds its informers from that list and hands the same list to the status writers, so the two cannot drift. Behaviour was pinned with a table across every discovery outcome and flag setting before the change. Only the intended cells moved: the divergence above is fixed in both kinds, and three cases stop constructing a promoted-version informer that discovery had already reported as unserved (it never started, so this only removes a dead wrapper). Preference ordering preserves the rule that a cluster serving both pre-v1 TLSRoute versions settles on v1alpha3. Also: share one CRD lookup between the kinds, warn when authoritative discovery finds no usable version rather than failing silently, drop the local GVR aliases in favour of the wellknown values, and add wellknown.TLSRouteCRDName so the TLS CRD name is no longer a local literal. The typed informer and conversion switches stay explicit per kind. TCPRoute and TLSRoute have genuinely different API shapes, and genericizing that would cost more clarity than it saves; the boundary is shared discovery and selection, explicit typed construction. Signed-off-by: omar <omar.hammami@solo.io>
… downstream Promoted TLSRoute was the one route watch still built with istio's kclient.NewDelayedInformer; TCPRoute and both pre-v1 TLSRoute paths already use newDelayedTypedInformer. Istio's CRD watcher keys readiness on <resource>.<group> and ignores the version, so on its own it reports TLSRoute v1 as ready off a CRD that serves no v1, starts an informer against an endpoint the API server does not serve, and never syncs - blocking every collection gated on it, up to the proxy syncer's cache barrier. Istio does not actually fall into that today: minimumVersionFilter drops Gateway API CRDs below a hardcoded per-resource minimum, and its tlsroutes entry is the release where v1 appeared, so ordinary pre-v1 installs are filtered out of the watcher entirely rather than watched at v1. The reachable window is narrower than it first appears - a CRD bundle new enough to pass that filter which nonetheless does not serve v1 - and relying on an istio-internal table staying aligned with kgateway's version selection is a coupling worth removing regardless. newDelayedTypedInformer checks the served version itself. A test pins both halves: that istio's watcher does report v1 as known in that window, and that the typed informer stays unblocked anyway. Separately, pass the shared NotReadyRequeuer through StatusRegistrationInputs. Writer.NotReady is required for any writer over a delayed client, and policy plugins already receive it, but downstream status registrations did not - so they would keep the permanently dropped-status race unless every consumer discovered the problem and built its own requeuer. Sharing one instance also keeps a single requeue budget per resource no matter which writer observed it as invisible. Signed-off-by: omar <omar.hammami@solo.io>
NotReadyRequeuer.Schedule kept a resource's entry once its requeue budget was spent. Done is the only thing that clears an entry and it only runs when a write pass can see the resource, so a resource deleted between enqueue and write - the exact case the budget exists to bound - was never seen again and its entry retained the GVK/namespace/name strings for the controller's lifetime. Create and delete churn accumulated one leaked entry per affected resource. The tombstone was justified as what stops the retries, which is wrong: a chain's next Schedule only happens because the previous one re-queued, so declining to re-queue is what ends it. The map only has to count within one chain. Forgetting the resource afterwards means a later, independent enqueue starts fresh, which is the behaviour we want anyway - a new upstream event is new evidence the resource may be visible now, and the backoff still rate-limits each chain. Rework the tests to drive real chains (each requeue leading to another schedule) rather than consecutive bare calls, which is what the counter actually sees in production, and assert both that a chain terminates at the limit leaving nothing behind and that a later enqueue gets a fresh budget. Signed-off-by: omar <omar.hammami@solo.io>
…tus defects Addresses code review findings on the keyed status contribution work. reports.BuildRouteStatus had no *gwv1a3.TLSRoute case, so it fell through to the default branch and returned nil. The route writer reads a nil desired status as "nothing to report" and publishes an empty one, which the merge applies as "clear every parent this controller owns" - so on Gateway API v1.4.x with experimental features enabled, where v1alpha3 is the selected write version, TLSRoute status was erased rather than written. Add the case, and stop the writer treating a nil build as an erase: with a report in hand the only way to get nil is an unhandled type, which is a bug to log, not a signal to delete status. The legacy XListenerSet status write is a dynamic merge patch carrying only the status body, having moved off a controller-runtime Status().Patch whose object carried metadata.resourceVersion. A merge patch without it applies unconditionally, so a status built from a stale read silently overwrites a newer one with nothing to re-enqueue and correct it. Send the resourceVersion and treat the resulting conflict the way the promoted path does. Backend policy contributions keyed their Source on the backend's ObjectSource resource name, which drops the port and extra key. A Service produces one BackendObjectIR per port, so two ports contributing to the same policy emitted identical KRT keys from a single collection. Key on the backend's own resource name. The merge that publishes RouteStatus.parents and PolicyStatus.ancestors now emits them in ParentString order - the key istio and kgateway's own report builders use - rather than grouping foreign entries first. Write suppression compares with plain equality, so an arbitrary published order would disagree on ordering alone with any peer controller that rewrites the list sorted, and the two would rewrite it back and forth forever. Entries owned by other controllers are still never dropped in favour of ours: the cap still runs on the foreign-first list. BackendTLSPolicy's desired status iterated a map without sorting. The writer's merge sorts what it publishes, but the translator's golden output calls the builder directly and saw nondeterministic ancestor order. Cleanups: fold the three separate calls needed to register a status kind into RegisterKind/RegisterKindByObjectGVK, since omitting any one of them compiled fine and produced a silent status outage; collapse the two line-for-line identical merge functions into one generic; drop the unstructured TLSRoute conversion left dead by the status rewrite; stop adding StatusCollections.HasSynced to the cache-sync barrier twice; and give the per-object contribution paths report maps scoped to the one kind they populate, instead of allocating eight maps per backend per translation to throw seven away. Signed-off-by: omar <omar.hammami@solo.io>
Mechanical follow-up to the rebase onto main. Two repo-wide changes landed upstream while this branch was out: package-level loggers replacing direct slog calls, and the perfsprint linter, which rejects fmt.Errorf for messages with no formatting verbs. Apply both to the code this branch adds, and drop the imports that leaves unused. No behaviour change. Signed-off-by: omar <omar.hammami@solo.io>
…t keys
Quality cleanup over the keyed status contribution work; no behavior change.
Reuse and duplication:
- Collapse PolicyStatusInputs and StatusRegistrationInputs into a single
statussync.RegistrationInputs. They were field-for-field duplicates, which is
how NotReady previously reached only one of them.
- Collapse the seven byte-identical arms of the route type switch in
BuildRouteStatusWithParentRefDefaulting; the parentRef defaulting now runs once
below the switch.
- Add statussync.ConditionError for the Gateway, ListenerSet and policy
"invalid condition" derivations, which were three copies of one loop.
- Export statussync.ReportFor and drop the two inline report lookups.
- Use istio ptr.OrDefault/OrEmpty for the ParentReference deref helpers, keeping
comparePortNumberPtr since an unset port sorts before port 0.
Derivable state and dead code:
- Derive ResourceReports.Target from Resource instead of storing both.
- Drop SetQueue's unused []krt.Syncer return and StatusTarget.String.
- Track only the previous contribution source in ReduceStatusContributions; the
reduced reports already say whether one was seen.
Hot paths:
- Chain the comparisons in compareStatusContributions instead of using cmp.Or,
which evaluates all seven on every comparison, and use SortFunc since the
comparator covers the full identity.
- Precompute ParentString sort keys in mergeOwnedStatusEntries rather than
formatting both operands inside the comparator on every write attempt.
- Skip routeStatusMetricsHook entirely when metrics are inactive, allocate its
error map lazily, and drop the intermediate gateway name slice.
- Gate the per-informer-event debug log on the debug level, and build the sync
metric labels once per record.
Altitude:
- Unexport CommonCollections.{TCP,TLS}RouteWriteGVRs behind accessors that apply
the fallback, so a consumer cannot observe an empty write-version list.
Signed-off-by: omar <omar.hammami@solo.io>
…probe TCP and TLS routes are normalized to one Go type, and the conversions stamped the normalized version into TypeMeta. That erased the only fact the write path needs: which API version the object was actually served as, and therefore which client can persist its status. The write path recovered it by building a writer per candidate version and probing each one's informer at write time (NewFirstPresentSyncer), turning a startup fact into a per-write race. Preserve the served API version in TypeMeta instead. The status collections already key resources by the object's own GVK (RegisterKindByObjectGVK, added for XListenerSet), so an enqueued route now names its version and dispatch is a map lookup. A v1alpha2 route is served rather than converted, so it arrives with empty TypeMeta and the registration fallback names v1alpha2 for it. Drops NewFirstPresentSyncer, ResourcePresenceChecker and Writer.HasResource, along with the derived tcpGVK/tlsGVK selection that had to agree with them. Adding a route version now means adding it to the version list and to the writer switch, with no probing path to keep in sync. Signed-off-by: omar <omar.hammami@solo.io>
Writers read the current object through their own kclient wrapper over a GVR the collection layer already watches. For TCP and TLS routes that meant a per-version read path alongside the per-version write path bd675af introduced: the v1 writer re-read the object as *gwv1.TCPRoute while the collection held the normalized *gwv1a2.TCPRoute. Split Writer into Current (read) and UpdateStatus (write), which decouples the read type from the write type. TCP and TLS routes now read once through their normalized collection and write back through whichever served version the object carries, so their per-version writers differ only in the object they build. Sourcing reads from the enqueuing collection also turns an accidental invariant into a structural one. kclient hands out one shared informer per {GVR, filter}, and every writer passed the same filter the collection layer uses, so the two delayed wrappers were swapped in together; policy plugins never had two wrappers at all, since they wrap the very client instance the writer read through. Correctness rested on those coincidences. A future writer built with a different filter or informer type would get an independent informer whose Get returns nil while HasSynced already reports true — indistinguishable, at the writer, from a deletion — and nothing upstream re-fires once it loads. The symptom would be a resource silently carrying no status, not a failing test. That skew is why NotReadyRequeuer existed: a 6-attempt, ~30s retry budget, plus StatusCollections.Requeue, plus a NotReady field threaded through Writer, the registration inputs and StatusSyncerConfig, documented as "pass this or lose status silently". The outage it was written for — a writer bound to an API version nothing serves — was fixed in bd675af by never selecting that writer. Reading from the enqueuing collection makes the remaining enqueued-but-invisible state unreachable rather than bounded, so the machinery goes with it: requeue.go, its tests, Requeue, StatusSyncerConfig.StatusNotReady and ProxySyncer.StatusNotReady. Two things this is not. It does not reduce informers: the per-writer NewFilteredDelayed calls are all still here, now write-only. And it trades the old coincidence for a new requirement — a normalized collection feeding CollectionSource must preserve ObjectMeta (hence resourceVersion) and status faithfully, or the no-op check and optimistic concurrency both break. convertTCPRouteV1ToV1Alpha2 does, and says so, but that is convention rather than a compiler check. Derived collections also sit a transform hop behind the informer, so a write can be built from a marginally staler object; resourceVersion conflict retry covers that. Signed-off-by: omar <omar.hammami@solo.io>
PolicyStatus.ancestors and RouteStatus.parents were merged twice. The builders in pkg/reports carried foreign entries over from the currentStatus they were handed, sorted, and (for policies) capped at the Gateway API limit; then statussync's Merge re-read the live object, dropped every entry it did not own, re-added the foreign ones from that read, and capped again. The builder half was redundant in production -- Merge discards the foreign entries it produced and re-derives them -- and the two caps had to agree on ownership policy across two packages, which capPolicyStatusAncestors' doc comment spelled out as a hand-maintained invariant. Only the write path holds an authoritative read of the live list, so that is where both belong. Builders now publish only the entries this controller owns, uncapped. They still read currentStatus, for LastTransitionTime and observedGeneration continuity, and still sort, so callers consuming the desired status directly (the golden translator tests) get a deterministic result. Golden outputs are unchanged: they build against an empty current status and stay under the caps. Drops capPolicyStatusAncestors. Signed-off-by: omar <omar.hammami@solo.io>
The report reducer holds an entry for every raw object, so Desired is asked about every route and every policy in the watched namespaces, not just the ones translation produced a report for. When the report is nil it returned an empty desired status, meaning "clear the entries I own" — correct for a resource we had written before, wrong for one we never touched. The write was not suppressed either: mergeOwnedStatusEntries returns a non-nil empty slice, gwv1.RouteStatus and gwv1.PolicyStatus have no Equals and are not protos, so the no-op check falls through to reflect.DeepEqual, which separates nil from empty. So every route and policy with no status of ours got one status write, cluster-wide, on every leadership acquisition. A resource carrying only another controller's entries got its list re-sorted into our canonical order, which is a rewrite war against any peer that compares its own list order-sensitively. Ask whether there is anything to retract before publishing empty. The predicate lives next to the merges, as the ownership question they already answer internally, so a writer cannot answer it differently than the merge does. The Gateway writer never had this: BuildGWStatus returns nil for a nil report, so Desired already declined. This makes routes and policies agree with it. Introduced in 427079e and ef480d5; the four commits since are unaffected. Signed-off-by: omar <omar.hammami@solo.io>
StatusTarget carried a Version that nothing read. Its stated purpose — naming the write version for resources whose promoted and legacy forms have different status writers — stopped being true in bd675af, which made the served version come from the raw object's TypeMeta via objectGVKOrDefault and deleted the informer probe that was the only other candidate consumer. What was left was inconsistent with itself. StatusContribution.ResourceName, the KRT key, omits Version; Equals compares Target whole, so it includes it. Two contributions differing only in version would share a key while comparing unequal — the inverted form of the bug the krtequals analyzer exists to catch. It cannot happen today, since each kind hardcodes one GVK in the split, and the worst case if it did would be a recompute producing no downstream change. Policy contributions did not even set the field, so the split was already emitting two shapes of the same type. Collapsing removes the type, the Key() indirection at four call sites, the unreachable version tiebreak in compareStatusContributions, and the empty-Version inconsistency. The test that asserted Key() projects versions away is replaced by one asserting the key covers every field Equals compares, which is the property that actually needed guarding. Signed-off-by: omar <omar.hammami@solo.io>
Unifying PolicyStatusInputs and StatusRegistrationInputs into one RegistrationInputs (2cc0789) gave the downstream entry point a Ctx field that NewStatusSyncer never populated. The policy path passes the real controller context; WithStatusRegistration passed nothing, so every downstream registration fell through pluginutils' nil guard to context.Background(). The field's documented "may be nil" was really "always nil for one of the two entry points", which is the same divergence the unification was meant to remove, just relocated from the type to its construction. Registrations run during construction, before Start receives a context, so it has to arrive on StatusSyncerConfig. The nil guard stays for inputs built directly in tests, but production now populates both paths identically. Signed-off-by: omar <omar.hammami@solo.io>
The reducer cloned its input purely to sort it. The production caller passes a krt.Fetch result, which is freshly allocated per call: for an index filter, Fetch gets the list from the index Lookup, which builds a new slice on every call, and Fetch then FilterInPlaces it — istio would already be corrupting index state if that slice were shared. So the copy protected nothing. It runs once per status owner on every recompute that touches it, which is the hottest reduction path in the status pipeline, so this is one wasted allocation per owner per event. Sort in place and document that the input is reordered. No caller reads its ordering back. Signed-off-by: omar <omar.hammami@solo.io>
3cc5713 moved the ancestor cap into statussync's write path, where it logs with a package-level logger. That removed the last reader of the ctx that pkg/reports' status builders take: BuildGWStatus, BuildListenerSetStatus, BuildBackendStatus, BuildRouteStatus(WithParentRefDefaulting) and BuildPolicyStatus all accepted a context and none of them used it. d87ca0e threaded the controller context down to those builders through RegistrationInputs.Ctx to fix a real divergence -- the policy entry point passed the root context, WithStatusRegistration passed nothing -- but by then there was nothing at the far end to read it. Unifying the two entry points on a field that feeds a discarded parameter just makes both of them carry the same dead weight, so remove the parameter instead of the divergence. Drops RegistrationInputs.Ctx and its "may be nil, treat as context.Background()" contract, StatusSyncerConfig.Ctx, and pluginutils' nil guard. Drops ctx from the twelve pkg/reports builder entry points and from routeWriter and initStatusInfra, which existed only to forward it; ProxySyncer.Init keeps its own ctx, which the translator still needs. No behavior change: the parameter never reached anything that read it. Golden outputs are untouched. Signed-off-by: omar <omar.hammami@solo.io>
…ons relies on ReduceStatusContributions sorts its input in place, which is only safe because krt.Fetch returns a slice the caller owns rather than a view into the index's storage. That is a coupling to istio krt internals an upstream bump could silently break, and the resulting index corruption would be hard to trace. Add a test that fetches through the same index filter the production reducer uses, overwrites every slot of the result, and asserts the index and source collection are intact. Slot-level writes cover everything a sort could permute, and a counter of overwritten slots keeps the test from passing vacuously if the fetch ever stops matching. Signed-off-by: omar <omar.hammami@solo.io>
Every status write echoes back as an informer event that re-enqueues the
resource, so a writer is always asked a second time about the status it just
wrote. Only the live-vs-desired skip stops that cycle, and it fires only when
rebuilding from what we wrote reproduces what we wrote. A builder that
regenerates LastTransitionTime or renormalizes entries it copied from the live
object breaks that equality and turns the designed one-shot echo into a
permanent write loop -- with every unit test of the skip still passing, because
the skip is working and simply never fires.
The existing cycle test pinned the skip against a hardcoded desired status, so
no production builder was ever exercised. Add:
- statussync.CheckWriterIdempotent, which runs Desired -> Merge -> apply ->
Desired -> Merge and fails if a second write would follow. The build/merge/
compare sequence moves out of ApplyStatus into Writer.decide so the harness
and the writer cannot answer differently. WriterWouldWrite lets a test show
the check is not vacuous. RegistrationInputs documents the obligation:
downstream writers added via WithStatusRegistration run the same harness.
- a cycle test over the real Gateway and HTTPRoute writers, seeded with stale
LastTransitionTimes, an outdated reason and observedGeneration, a foreign
condition and foreign route parents stored out of canonical order. It
asserts one write, waits for that write's own informer echo, and asserts
the writer absorbs it. The Gateway writer moves into gatewayWriter() so the
test builds the writer the controller actually runs.
One fake-client fidelity gap had to be corrected first: client-go's tracker
documents that "subresources are not handled accurately" and stores the request
object verbatim on a status update. Our writers send an ObjectMeta carrying only
identity, so the first status write erased the spec every builder reads, and the
rebuild then legitimately differed forever. apifake.InstallStatusSubresourceReactor
takes only status, as the API server does.
Signed-off-by: omar <omar.hammami@solo.io>
A nil buildDesired selected the default status builder *and* silently turned off
the invalid-condition sync metric, so a plugin that supplied its own builder for
standard-shaped status lost the metric with no signal. The two are independent
questions, and answering them with one argument meant neither was stated at the
call site: a trailing positional nil in a 7-argument generic call says nothing
about either.
Split them:
- RegisterPolicyStatus keeps the standard builder and grades conditions.
- RegisterPolicyStatusWithBuilder takes the builder plus an explicit
ConditionErrorMetric (StandardConditionErrorMetric / NoConditionErrorMetric).
Two constructors rather than variadic options because Go cannot infer the type
parameter for a standalone option call, so every opt-out would have had to spell
out the policy type.
BackendTLSPolicy is the one caller that opts out, and now says so: its
conditions carry the Gateway API's PolicyReasonAccepted, which the standard
grading (Valid, Pending) would score as permanently failing -- exactly what the
old nil bought it by accident. The other four plugins just drop the nil.
The metric test becomes a table over both builder x metric combinations, and the
policy writer now runs the shared idempotence harness, recovering the typed
Writer from the ResourceStatusSyncer the way a downstream registration would.
Signed-off-by: omar <omar.hammami@solo.io>
Gateway/ListenerSet status.listeners[].attachedRoutes was set inside the core gateway translator (setAttachedRoutes), so any route churn that touched a Gateway forced a full re-translation and a new Gateway status report even when nothing about the Gateway itself changed. Scale testing against PR kgateway-dev#14520's harness showed this driving 5-20x more Gateway status writes than main, growing with route count. Move AttachedRoutes to its own krt collection (query.NewTargetAttachedRoutes) that counts routes per Gateway/ListenerSet listener independently of translation, using a shared attachOutcomeForListener predicate extracted from processRoute. BuildGWStatus already had an attachedRoutes override hook for this; BuildListenerSetStatus gains the same one. Route churn now only recomputes this small derived collection instead of the whole Gateway report. This also fixes a latent bug: Gateways translated by a plugin-supplied sdk.KGwTranslator (e.g. waypoint) never ran setAttachedRoutes at all, so their attachedRoutes silently stayed 0 regardless of real attachment. The new collection counts uniformly for every Gateway, since attachment is standard Gateway API status resolved the same way regardless of which translator later renders xDS. Scale-tested 1k/5k/10k routes x2 reps against the pre-refactor tip: Gateway writes during convergence dropped 115->10, 212->10, 264->30 (10-20x, and no longer growing with scale) with every other metric (CPU, alloc, heap, RSS, wall time, write conflicts) flat within noise. Signed-off-by: omar <omar.hammami@solo.io>
puertomontt
force-pushed
the
codex/keyed-status-contributions
branch
from
August 12, 2026 14:30
82e31f0 to
6de1f81
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Follow-up to kgateway-dev#14519.
status.listeners[].attachedRouteswas set inside the core gateway translator (setAttachedRoutes, called fromgateway.translator.Translate), so any route churn touching a Gateway forced a full re-translation and a new Gateway status report, even when nothing about the Gateway itself had changed. Scale-testing kgateway-dev#14519's harness (kgateway-dev#14520) showed this driving 5-8.65x more Gateway status writes thanmain, growing with route count -- the one flagged regression in that PR's own perf writeup.What changed:
AttachedRoutesnow comes from its own krt collection,query.NewTargetAttachedRoutes, which counts routes attached to each Gateway/ListenerSet listener independently of translation, viaRoutesIndex.RoutesFor(already index-backed per target). Route churn now only recomputes this small derived collection instead of the whole Gateway/ListenerSet report.attachOutcomeForListener) out ofprocessRouteso both the translator and the new counting collection use one code path -- table-tested.BuildGWStatusalready had anattachedRoutesoverride hook for exactly this;BuildListenerSetStatusgains the same one.setAttachedRoutes/SetAttachedRoutes, now dead.test/translator) builds the same production counting path, so goldenattachedRoutesvalues exercise the real code.Bug fix along the way: Gateways translated by a plugin-supplied
sdk.KGwTranslator(e.g. waypoint) never ransetAttachedRoutesat all, so theirattachedRoutessilently stayed0regardless of real attachment --pkg/kgateway/extensions2/plugins/waypoint/testdata/output/httproute-gateway.yamlshows the corrected value. The new collection counts uniformly for every Gateway, since attachment is standard Gateway API status resolved the same way regardless of which translator later renders xDS.Verification:
pkg/kgateway/translator/gateway) regenerated withREFRESH_GOLDEN=true: byte-identical except the one waypoint fix above.make analyze: 0 issues (including thekrtequalscheck on the newTargetAttachedRoutestype).Change Type
/kind fix
Changelog
Additional Notes
Based on
codex/keyed-status-contributions(kgateway-dev#14519) rather thanmain, since it depends on that PR's status-contributions architecture (reports.StatusKey, thestatussyncpackage,gatewayWriter/listenerSetStatusSyncer). Should be retargeted tomain(or opened fresh againstkgateway-dev/kgateway:main) once kgateway-dev#14519 lands.