-
Notifications
You must be signed in to change notification settings - Fork 607
fix: prevent skeleton route status entries for unmanaged GatewayClasses #7536
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: prevent skeleton route status entries for unmanaged GatewayClasses #7536
Conversation
50ea1e8 to
f9767f0
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7536 +/- ##
==========================================
- Coverage 72.29% 71.16% -1.14%
==========================================
Files 231 274 +43
Lines 34084 34862 +778
==========================================
+ Hits 24641 24809 +168
- Misses 7670 8259 +589
- Partials 1773 1794 +21 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
When processing policies (EnvoyExtensionPolicy, SecurityPolicy), the translator was calling GetRouteParentContext for ALL parentRefs in a route, even those referencing gateways with different GatewayClasses not managed by this translator. GetRouteParentContext creates a skeleton RouteParentStatus entry with just the controllerName when called on a parentRef that hasn't been processed yet. Since all GatewayClass instances share the same controller name, these skeleton entries persisted in status without conditions. The fix checks if a parentRef context already exists before attempting to apply policy configuration to it. If the context doesn't exist, it means this parentRef wasn't processed by this translator and should be skipped. Signed-off-by: Raj Singh <[email protected]>
f9767f0 to
255539c
Compare
|
can you add a test case for this? |
|
does this fix have any intersection with #7307 (haven't looked closely yet) |
|
thanks @rajsinghtech, I took a real look at this and changes look good also is a change needed in BTP as well ? |
|
Will add the test shortly, wasn't sure if I captured the full bug fix. |
|
yeah codex says, needs some changes in BTP |
No, #7307 addresses another issue: it aggregates status across all GatewayClasses and performs a single consolidated update at the end, ensuring we can both preserve valid parent statuses and correctly remove those that no longer exist. |
The same issue exists in BackendTrafficPolicy route processing - calling GetRouteParentContext for all parentRefs creates skeleton status entries. Apply the same fix: check if parentRef context exists before adding to list. Signed-off-by: Raj Singh <[email protected]>
|
Added fix for BackendTrafficPolicy as well in commit 9b97c08. Regarding tests: The existing test framework () simulates a single translator instance with a hardcoded GatewayClassName. The bug only manifests when multiple translators (one per GatewayClass) process the same HTTPRoute, which isn't directly testable in the current unit test structure. However, the fix is straightforward:
The change prevents skeleton entries from being created when policies are applied to routes with multiple GatewayClasses. |
you could achieve something similar in a test by adding a parentRef thats not part of the input, there will be failure in route status but also the before / after policy status should look different |
|
@arkodg I think tests in https://github.com/envoyproxy/gateway/pull/7558/files#diff-c7323da3d9dfb378a30bb7b5a00c13ab9255ac810e2adfd9ec6dec6c9ef88a67R60-R116 cover this? The sequence is:
If they do, then I suggest we merge this PR without tests and use the tests in the other one, just for less conflicts and duplicated tests. |
arkodg
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM thanks
…es (envoyproxy#7536) * fix: prevent skeleton route status entries for unmanaged GatewayClasses When processing policies (EnvoyExtensionPolicy, SecurityPolicy), the translator was calling GetRouteParentContext for ALL parentRefs in a route, even those referencing gateways with different GatewayClasses not managed by this translator. GetRouteParentContext creates a skeleton RouteParentStatus entry with just the controllerName when called on a parentRef that hasn't been processed yet. Since all GatewayClass instances share the same controller name, these skeleton entries persisted in status without conditions. The fix checks if a parentRef context already exists before attempting to apply policy configuration to it. If the context doesn't exist, it means this parentRef wasn't processed by this translator and should be skipped. Signed-off-by: Raj Singh <[email protected]> * fix: also prevent skeleton entries in BackendTrafficPolicy processing The same issue exists in BackendTrafficPolicy route processing - calling GetRouteParentContext for all parentRefs creates skeleton status entries. Apply the same fix: check if parentRef context exists before adding to list. Signed-off-by: Raj Singh <[email protected]> --------- Signed-off-by: Raj Singh <[email protected]> (cherry picked from commit ff13742) Signed-off-by: Huabing Zhao <[email protected]>
…es (envoyproxy#7536) * fix: prevent skeleton route status entries for unmanaged GatewayClasses When processing policies (EnvoyExtensionPolicy, SecurityPolicy), the translator was calling GetRouteParentContext for ALL parentRefs in a route, even those referencing gateways with different GatewayClasses not managed by this translator. GetRouteParentContext creates a skeleton RouteParentStatus entry with just the controllerName when called on a parentRef that hasn't been processed yet. Since all GatewayClass instances share the same controller name, these skeleton entries persisted in status without conditions. The fix checks if a parentRef context already exists before attempting to apply policy configuration to it. If the context doesn't exist, it means this parentRef wasn't processed by this translator and should be skipped. Signed-off-by: Raj Singh <[email protected]> * fix: also prevent skeleton entries in BackendTrafficPolicy processing The same issue exists in BackendTrafficPolicy route processing - calling GetRouteParentContext for all parentRefs creates skeleton status entries. Apply the same fix: check if parentRef context exists before adding to list. Signed-off-by: Raj Singh <[email protected]> --------- Signed-off-by: Raj Singh <[email protected]> Signed-off-by: jukie <[email protected]>
…es (envoyproxy#7536) * fix: prevent skeleton route status entries for unmanaged GatewayClasses When processing policies (EnvoyExtensionPolicy, SecurityPolicy), the translator was calling GetRouteParentContext for ALL parentRefs in a route, even those referencing gateways with different GatewayClasses not managed by this translator. GetRouteParentContext creates a skeleton RouteParentStatus entry with just the controllerName when called on a parentRef that hasn't been processed yet. Since all GatewayClass instances share the same controller name, these skeleton entries persisted in status without conditions. The fix checks if a parentRef context already exists before attempting to apply policy configuration to it. If the context doesn't exist, it means this parentRef wasn't processed by this translator and should be skipped. Signed-off-by: Raj Singh <[email protected]> * fix: also prevent skeleton entries in BackendTrafficPolicy processing The same issue exists in BackendTrafficPolicy route processing - calling GetRouteParentContext for all parentRefs creates skeleton status entries. Apply the same fix: check if parentRef context exists before adding to list. Signed-off-by: Raj Singh <[email protected]> --------- Signed-off-by: Raj Singh <[email protected]> Signed-off-by: jukie <[email protected]>
…es (envoyproxy#7536) * fix: prevent skeleton route status entries for unmanaged GatewayClasses When processing policies (EnvoyExtensionPolicy, SecurityPolicy), the translator was calling GetRouteParentContext for ALL parentRefs in a route, even those referencing gateways with different GatewayClasses not managed by this translator. GetRouteParentContext creates a skeleton RouteParentStatus entry with just the controllerName when called on a parentRef that hasn't been processed yet. Since all GatewayClass instances share the same controller name, these skeleton entries persisted in status without conditions. The fix checks if a parentRef context already exists before attempting to apply policy configuration to it. If the context doesn't exist, it means this parentRef wasn't processed by this translator and should be skipped. Signed-off-by: Raj Singh <[email protected]> * fix: also prevent skeleton entries in BackendTrafficPolicy processing The same issue exists in BackendTrafficPolicy route processing - calling GetRouteParentContext for all parentRefs creates skeleton status entries. Apply the same fix: check if parentRef context exists before adding to list. Signed-off-by: Raj Singh <[email protected]> --------- Signed-off-by: Raj Singh <[email protected]> Signed-off-by: jukie <[email protected]>
* fix(xds-server): clear snapshot on stream close (#6618) * fix(xds-server): clear snapshot on stream close Signed-off-by: Zachary Vacura <[email protected]> * check if there are other active connections before clearning the snapshot Signed-off-by: Zachary Vacura <[email protected]> Signed-off-by: jukie <[email protected]> * fix: oidc authentication endpoint was overwritten by discovered value (#7460) fix: oid authentication endpoint was overriden by discovered value Signed-off-by: Huabing Zhao <[email protected]> Signed-off-by: Huabing (Robin) Zhao <[email protected]> Signed-off-by: jukie <[email protected]> * ci: add script to free disk space (#7534) * feat: free disk space Signed-off-by: Shreemaan Abhishek <[email protected]> * lint Signed-off-by: Shreemaan Abhishek <[email protected]> * cleanup Signed-off-by: Shreemaan Abhishek <[email protected]> * make target and tools/hack Signed-off-by: Shreemaan Abhishek <[email protected]> * lint Signed-off-by: Shreemaan Abhishek <[email protected]> * modular action Signed-off-by: Shreemaan Abhishek <[email protected]> --------- Signed-off-by: Shreemaan Abhishek <[email protected]> Signed-off-by: jukie <[email protected]> * treat too many addresses as programmed (#7542) Signed-off-by: cong <[email protected]> Signed-off-by: jukie <[email protected]> * feat: reclaim space in release pipeline (#7587) Signed-off-by: Shreemaan Abhishek <[email protected]> Signed-off-by: jukie <[email protected]> * chore: bump golang.org/x/crypto (#7588) * chore: bump golang.org/x/crypto Signed-off-by: zirain <[email protected]> * fix gen Signed-off-by: zirain <[email protected]> --------- Signed-off-by: zirain <[email protected]> Signed-off-by: jukie <[email protected]> * findOwningGateway should return controller based on linked GatewayClass (#7611) * fix: filter Gateway by controller in findOwningGateway Prevent cross-controller Gateway mutations by validating GatewayClass Signed-off-by: Sudipto Baral <[email protected]> Signed-off-by: jukie <[email protected]> * fix: use default when namespace is unset (#7612) * fix: use default when namespace is unset Signed-off-by: zirain <[email protected]> * fix Signed-off-by: zirain <[email protected]> * fix test Signed-off-by: zirain <[email protected]> --------- Signed-off-by: zirain <[email protected]> Signed-off-by: jukie <[email protected]> * fix: prevent skeleton route status entries for unmanaged GatewayClasses (#7536) * fix: prevent skeleton route status entries for unmanaged GatewayClasses When processing policies (EnvoyExtensionPolicy, SecurityPolicy), the translator was calling GetRouteParentContext for ALL parentRefs in a route, even those referencing gateways with different GatewayClasses not managed by this translator. GetRouteParentContext creates a skeleton RouteParentStatus entry with just the controllerName when called on a parentRef that hasn't been processed yet. Since all GatewayClass instances share the same controller name, these skeleton entries persisted in status without conditions. The fix checks if a parentRef context already exists before attempting to apply policy configuration to it. If the context doesn't exist, it means this parentRef wasn't processed by this translator and should be skipped. Signed-off-by: Raj Singh <[email protected]> * fix: also prevent skeleton entries in BackendTrafficPolicy processing The same issue exists in BackendTrafficPolicy route processing - calling GetRouteParentContext for all parentRefs creates skeleton status entries. Apply the same fix: check if parentRef context exists before adding to list. Signed-off-by: Raj Singh <[email protected]> --------- Signed-off-by: Raj Singh <[email protected]> Signed-off-by: jukie <[email protected]> * lint Signed-off-by: jukie <[email protected]> --------- Signed-off-by: Zachary Vacura <[email protected]> Signed-off-by: jukie <[email protected]> Signed-off-by: Huabing Zhao <[email protected]> Signed-off-by: Huabing (Robin) Zhao <[email protected]> Signed-off-by: Shreemaan Abhishek <[email protected]> Signed-off-by: cong <[email protected]> Signed-off-by: zirain <[email protected]> Signed-off-by: Sudipto Baral <[email protected]> Signed-off-by: Raj Singh <[email protected]> Co-authored-by: Zach Vacura <[email protected]> Co-authored-by: Huabing (Robin) Zhao <[email protected]> Co-authored-by: shreealt <[email protected]> Co-authored-by: 聪 <[email protected]> Co-authored-by: zirain <[email protected]> Co-authored-by: Sudipto Baral <[email protected]> Co-authored-by: Raj Singh <[email protected]>
* fix: oidc authentication endpoint was overwritten by discovered value (#7460) fix: oid authentication endpoint was overriden by discovered value Signed-off-by: Huabing Zhao <[email protected]> Signed-off-by: Huabing (Robin) Zhao <[email protected]> (cherry picked from commit 50dcb15) Signed-off-by: Huabing Zhao <[email protected]> * fix: do not return 500 for all requests when part of BackendRefs are invalid (#7488) * do not return 500 for all requests when part of BackendRefs are invalid Signed-off-by: Huabing Zhao <[email protected]> Signed-off-by: Huabing (Robin) Zhao <[email protected]> (cherry picked from commit 2899416) Signed-off-by: Huabing Zhao <[email protected]> * fix: prevent skeleton route status entries for unmanaged GatewayClasses (#7536) * fix: prevent skeleton route status entries for unmanaged GatewayClasses When processing policies (EnvoyExtensionPolicy, SecurityPolicy), the translator was calling GetRouteParentContext for ALL parentRefs in a route, even those referencing gateways with different GatewayClasses not managed by this translator. GetRouteParentContext creates a skeleton RouteParentStatus entry with just the controllerName when called on a parentRef that hasn't been processed yet. Since all GatewayClass instances share the same controller name, these skeleton entries persisted in status without conditions. The fix checks if a parentRef context already exists before attempting to apply policy configuration to it. If the context doesn't exist, it means this parentRef wasn't processed by this translator and should be skipped. Signed-off-by: Raj Singh <[email protected]> * fix: also prevent skeleton entries in BackendTrafficPolicy processing The same issue exists in BackendTrafficPolicy route processing - calling GetRouteParentContext for all parentRefs creates skeleton status entries. Apply the same fix: check if parentRef context exists before adding to list. Signed-off-by: Raj Singh <[email protected]> --------- Signed-off-by: Raj Singh <[email protected]> (cherry picked from commit ff13742) Signed-off-by: Huabing Zhao <[email protected]> * treat too many addresses as programmed (#7542) Signed-off-by: cong <[email protected]> (cherry picked from commit 7cb5f72) Signed-off-by: Huabing Zhao <[email protected]> * bechmark: fix cpu sampling (#7581) use fixed duration for cpu rate Signed-off-by: Huabing Zhao <[email protected]> (cherry picked from commit 536486f) Signed-off-by: Huabing Zhao <[email protected]> * chore: bump golang.org/x/crypto (#7588) * chore: bump golang.org/x/crypto Signed-off-by: zirain <[email protected]> * fix gen Signed-off-by: zirain <[email protected]> --------- Signed-off-by: zirain <[email protected]> (cherry picked from commit 70fa59a) Signed-off-by: Huabing Zhao <[email protected]> * findOwningGateway should return controller based on linked GatewayClass (#7611) * fix: filter Gateway by controller in findOwningGateway Prevent cross-controller Gateway mutations by validating GatewayClass Signed-off-by: Sudipto Baral <[email protected]> (cherry picked from commit ba8e0e2) Signed-off-by: Huabing Zhao <[email protected]> * fix: use default when namespace is unset (#7612) * fix: use default when namespace is unset Signed-off-by: zirain <[email protected]> * fix Signed-off-by: zirain <[email protected]> * fix test Signed-off-by: zirain <[email protected]> --------- Signed-off-by: zirain <[email protected]> (cherry picked from commit be2cc73) Signed-off-by: Huabing Zhao <[email protected]> * bump Gateway API v1.4.1 (#7653) Signed-off-by: zirain <[email protected]> (cherry picked from commit 0fa26d7) Signed-off-by: Huabing Zhao <[email protected]> * update release note Signed-off-by: Huabing Zhao <[email protected]> * fix gen check Signed-off-by: Huabing Zhao <[email protected]> * ci: add script to free disk space (#7534) * feat: free disk space Signed-off-by: Shreemaan Abhishek <[email protected]> * lint Signed-off-by: Shreemaan Abhishek <[email protected]> * cleanup Signed-off-by: Shreemaan Abhishek <[email protected]> * make target and tools/hack Signed-off-by: Shreemaan Abhishek <[email protected]> * lint Signed-off-by: Shreemaan Abhishek <[email protected]> * modular action Signed-off-by: Shreemaan Abhishek <[email protected]> --------- Signed-off-by: Shreemaan Abhishek <[email protected]> (cherry picked from commit 4312f38) Signed-off-by: Huabing Zhao <[email protected]> --------- Signed-off-by: Huabing Zhao <[email protected]> Signed-off-by: Huabing (Robin) Zhao <[email protected]> Signed-off-by: Raj Singh <[email protected]> Signed-off-by: cong <[email protected]> Signed-off-by: zirain <[email protected]> Signed-off-by: Sudipto Baral <[email protected]> Signed-off-by: Shreemaan Abhishek <[email protected]> Co-authored-by: Raj Singh <[email protected]> Co-authored-by: 聪 <[email protected]> Co-authored-by: zirain <[email protected]> Co-authored-by: Sudipto Baral <[email protected]> Co-authored-by: shreealt <[email protected]>
Description
Fixes a bug where HTTPRoutes referencing gateways with multiple different GatewayClasses would have incomplete status conditions.
Problem
When an HTTPRoute references gateways using different GatewayClasses (e.g.,
private,public,home-ts), the status showed all parentRefs withcontrollerNameset, but only some had actual conditions (Accepted, ResolvedRefs). Others had just the controllerName field with no conditions array.Example:
Root Cause
When processing EnvoyExtensionPolicy and SecurityPolicy, the code called
GetRouteParentContext()for ALL parentRefs in the route, including those referencing gateways with different GatewayClasses not managed by the current translator instance.GetRouteParentContext()creates a skeleton RouteParentStatus entry (with just controllerName and parentRef) when called on a parentRef that hasn't been processed yet. Since all GatewayClass instances use the same controller name (gateway.envoyproxy.io/gatewayclass-controller), these skeleton entries persisted without conditions.Solution
Check if a parentRef context already exists before attempting to apply policy configuration. If it doesn't exist, skip it - this means the parentRef references a gateway not managed by this translator instance.
Fixes #7537