-
Notifications
You must be signed in to change notification settings - Fork 583
fix(translator): Fix panic with request mirror + grpcroute #6875
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(translator): Fix panic with request mirror + grpcroute #6875
Conversation
0261526 to
da5dfd6
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6875 +/- ##
==========================================
- Coverage 71.10% 71.08% -0.03%
==========================================
Files 225 225
Lines 39859 39868 +9
==========================================
- Hits 28343 28340 -3
- Misses 9848 9857 +9
- Partials 1668 1671 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
c2e5f76 to
fea450c
Compare
internal/gatewayapi/filters.go
Outdated
| return nil | ||
| } | ||
|
|
||
| func (t *Translator) processRequestMirrorFilter( |
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.
can we eliminate this ?
validateBackendRef already accepts a backend interface
gateway/internal/gatewayapi/validate.go
Line 29 in c181d47
| func (t *Translator) validateBackendRef(backendRefContext BackendRefContext, route RouteContext, |
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.
@arkodg I think we need some kind of split in the concrete type of the iface approximately here, because validateBackendRefFilters needs GRPCRoutes to have a GRPCRouteFilter-containing-backendref, and HTTPRoutes to have an HTTPRouteFilter-containing-backendref -- the panic was caused by the existing unification causing the downstream cast to fail.
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.
(I'm not a go expert or familiar with the types in play, but this was the smallest code diff way I found to avoid the unchecked cast panic)
|
@AndyMoreland e.g. var mirrorBackendRef BackendRefContext
routeType := GetRouteType(filterContext.Route)
if routeType == resource.KindGRPCRoute {
mirrorBackendRef = gwapiv1.GRPCBackendRef{
BackendRef: gwapiv1.BackendRef{
BackendObjectReference: mirrorBackend,
Weight: &weight,
},
}
} else {
mirrorBackendRef = gwapiv1.HTTPBackendRef{
BackendRef: gwapiv1.BackendRef{
BackendObjectReference: mirrorBackend,
Weight: &weight,
},
}
}
// This sets the status on the HTTPRoute, should the usage be changed so that the status message reflects that the backendRef is from the filter?
filterNs := filterContext.Route.GetNamespace()
serviceNamespace := NamespaceDerefOr(mirrorBackend.Namespace, filterNs)
err = t.validateBackendRef(mirrorBackendRef, filterContext.Route,
resources, serviceNamespace, routeType) |
|
+1 to #6875 (comment) |
Signed-off-by: Andrew Moreland <andy@andymoreland.com>
Signed-off-by: Andrew Moreland <andy@andymoreland.com>
Signed-off-by: Andrew Moreland <andy@andymoreland.com>
Signed-off-by: Andrew Moreland <andy@andymoreland.com>
Signed-off-by: Andrew Moreland <andy@andymoreland.com>
The validateBackendRefFilters function casts the Filters field to the specific filter type (HTTPRouteFilter vs GRPCRouteFilter) based on the routeKind. Using the wrong wrapper type causes a type assertion panic, so we must keep the typed wrappers despite the duplication. Signed-off-by: Andrew Moreland <andy@andymoreland.com>
Simplified the RequestMirror filter processing by: - Getting RouteType directly from filterContext - Creating appropriate BackendRef type (HTTP or gRPC) based on RouteType - Consolidating logic into single processRequestMirrorFilter function - Keeping processGRPCRequestMirrorFilter as a delegate for compatibility This approach follows the review feedback and maintains the fix for GRPCRoute panic while simplifying the code structure. Signed-off-by: Andrew Moreland <andy@andymoreland.com>
521204e to
4740589
Compare
|
I've applied the suggested change |
ProcessGRPCFilters now calls processRequestMirrorFilter directly since it already handles both HTTP and gRPC routes based on RouteType. Signed-off-by: Andrew Moreland <andy@andymoreland.com>
|
thanks for addressing the comments @AndyMoreland, added one minor comment |
Co-authored-by: Arko Dasgupta <arkodg@users.noreply.github.com> Signed-off-by: Andrew Moreland <andy@andymo.org>
|
I've merged your suggestion |
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
…y#6875) Signed-off-by: Andrew Moreland <andy@andymoreland.com>
…y#6875) Signed-off-by: Andrew Moreland <andy@andymoreland.com> Signed-off-by: shawnh2 <shawnhxh@outlook.com>
* fix(xds-server): clear snapshot on stream close (#6618) * fix(xds-server): clear snapshot on stream close Signed-off-by: Zachary Vacura <zvacura@digitalocean.com> * check if there are other active connections before clearning the snapshot Signed-off-by: Zachary Vacura <zvacura@digitalocean.com> Signed-off-by: shawnh2 <shawnhxh@outlook.com> * bug: disable x-envoy-ratelimited by default (#7110) * bug: disable x-envoy-ratelimited by default * can be enabled with `enableEnvoyHeaders` in CTP Relates to #7034 Signed-off-by: Arko Dasgupta <arko@tetrate.io> * fix tests and release note Signed-off-by: Arko Dasgupta <arko@tetrate.io> * fix testdata Signed-off-by: Arko Dasgupta <arko@tetrate.io> --------- Signed-off-by: Arko Dasgupta <arko@tetrate.io> Signed-off-by: shawnh2 <shawnhxh@outlook.com> * fix(translator): Fix panic with request mirror + grpcroute (#6875) Signed-off-by: Andrew Moreland <andy@andymoreland.com> Signed-off-by: shawnh2 <shawnhxh@outlook.com> * fix: bug in overlap detection of cert SANs (#7234) Signed-off-by: shawnh2 <shawnhxh@outlook.com> * bump golang for crypto/x509 reggression (#7236) Signed-off-by: zirain <zirain2009@gmail.com> Signed-off-by: shawnh2 <shawnhxh@outlook.com> * fix gen-check Signed-off-by: shawnh2 <shawnhxh@outlook.com> --------- Signed-off-by: Zachary Vacura <zvacura@digitalocean.com> Signed-off-by: shawnh2 <shawnhxh@outlook.com> Signed-off-by: Arko Dasgupta <arko@tetrate.io> Signed-off-by: Andrew Moreland <andy@andymoreland.com> Signed-off-by: zirain <zirain2009@gmail.com> Co-authored-by: Zach Vacura <zach@hackzzila.com> Co-authored-by: Arko Dasgupta <arkodg@users.noreply.github.com> Co-authored-by: Andrew Moreland <andy@andymo.org> Co-authored-by: Rudrakh Panigrahi <rudrakh97@gmail.com> Co-authored-by: zirain <zirain2009@gmail.com>
What this PR does / why we need it:
This fixes an unconditional panic when the RequestMirror filter is used with GRPCRoutes
Which issue(s) this PR fixes:
Fixes #6874
Release Notes: No