Merge VHO/RO statuses from snapshots when updating resource status #10745
Conversation
|
Issues linked to changelog: |
ryanrolds
changed the title
| apiVersion: gateway.networking.k8s.io/v1 | ||
| kind: HTTPRoute | ||
| metadata: | ||
| name: example-route | ||
| spec: | ||
| parentRefs: | ||
| - name: gw-1 | ||
| hostnames: | ||
| - "example.com" | ||
| rules: | ||
| - filters: | ||
| - type: ExtensionRef | ||
| extensionRef: | ||
| group: gateway.solo.io | ||
| kind: RouteOption | ||
| name: header-manipulation | ||
| backendRefs: | ||
| - name: example-svc | ||
| port: 8080 | ||
| --- | ||
| apiVersion: gateway.networking.k8s.io/v1 | ||
| kind: HTTPRoute | ||
| metadata: | ||
| name: example-route-2 | ||
| spec: | ||
| parentRefs: | ||
| - name: gw-2 | ||
| hostnames: | ||
| - "example.com" | ||
| rules: | ||
| - filters: | ||
| - type: ExtensionRef | ||
| extensionRef: | ||
| group: gateway.solo.io | ||
| kind: RouteOption | ||
| name: header-manipulation-2 | ||
| backendRefs: | ||
| - name: example-svc | ||
| port: 8080 | ||
| --- |
There was a problem hiding this comment.
I moved these up so it was easier to skim.
|
Visit the preview URL for this PR (updated for commit b65109b): https://gloo-edge--pr10745-rolds-vho-statuses-g39aryiz.web.app (expires Wed, 16 Apr 2025 17:44:39 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 77c2b86e287749579b7ff9cadb81e099042ef677 |
ryanrolds
changed the title
ryanrolds
changed the title
sheidkamp
left a comment
sheidkamp
left a comment
There was a problem hiding this comment.
Would approve, but don't want it to merge before Sam/Steven have a chance to look.
If we were planning on keeping this code around, I'd advocate for some consolidation (reconcile the "classicStatus" and "legacyStatus" objects used by RO and VHO, move the merge functionality into a library for reuse by (Http)ListenerOptions), but we aren't so the effort isn't worth it at the moment.
This is a good callout and I wish we had more clarity on what the plan for the current Gloo's GW API is and if it will eventually be updated with the newer patterns in kgateway. I have no objection to DRYing this out and making a common status struct. |
For the moment, I'm fine keeping it how it is and if we do end up coming back to the code, fixing it up then |
| ) error | ||
| MergeStatusPlugin( | ||
| ctx context.Context, | ||
| sources any, |
There was a problem hiding this comment.
super nit: can we have this be some interface that has the XYZStatusPlugin methods?
There was a problem hiding this comment.
I'm not sure I understand the ask. It's on the StatusPlugin interface.
There was a problem hiding this comment.
just to avoid any, only allow passing in things that implement status methods
nbd, just stylistic
| // MergeStatusPlugin merges the status of the source plugin in to the current plugin. | ||
| // This is used late in the proxy sync process to report the status of the VHO when | ||
| // it references multiple proxies/snapshots. | ||
| func (p *plugin) MergeStatusPlugin(ctx context.Context, source any) error { |
There was a problem hiding this comment.
would it be possible to avoid this method altogether by merging the StatusContext?
There was a problem hiding this comment.
Each plugin may have it's own internal state/struct that need plugin-specific logic. I wanted to make sure to support that that case, just like we do when applying the statuses.
Description
The addition of supporting multiple target refs for VHO exposed a gap with how status are reported for resources that reference multiple Gateways.
This PR resolves that gap by introducing a way for plugins to merge their status caches, allowing the syncer to merge the caches of all status plugins in the snapshots, producing a complete status report for the resource. Only VHO and RO are status plugins, also the RO lacks warnings - all of the key information comes from the proxy reports, so the marge is mostly a no-op.
Checklist: