tcproute/udproute support multiple backends #3212
Conversation
zirain
changed the title
zirain
changed the title
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3212 +/- ##
==========================================
- Coverage 67.11% 66.87% -0.25%
==========================================
Files 164 163 -1
Lines 23818 23307 -511
==========================================
- Hits 15986 15587 -399
+ Misses 6912 6814 -98
+ Partials 920 906 -14 ☔ View full report in Codecov by Sentry. |
Signed-off-by: zirain <zirain2009@gmail.com>
Signed-off-by: zirain <zirain2009@gmail.com>
zirain
force-pushed
the
tcproute-backendrefs
branch
from
b80c66f
to
bf79459
Compare
Signed-off-by: zirain <zirain2009@gmail.com>
internal/xds/translator/route.go
Outdated
| backendWeights := httpRoute.Destination.ToWeightedBackend() | ||
| routeAction := buildXdsRouteAction(backendWeights) | ||
| routeAction.IdleTimeout = idleTimeout(httpRoute) |
There was a problem hiding this comment.
cc @arkodg , this's the key point, use BackendWeights to generate ClusterSpecifier everywhere.
There was a problem hiding this comment.
thanks for adding this note :). the diff is massive, stared with this file and the IR and this approach makes sense
zirain
force-pushed
the
tcproute-backendrefs
branch
from
b0c5c1e
to
16c6627
Compare
| - backendWeights: | ||
| invalid: 1 | ||
| valid: 0 | ||
| directResponse: |
There was a problem hiding this comment.
why is directResponse getting removed
| settings: | ||
| - weight: 1 | ||
| name: tcproute/default/tcproute-1 | ||
| tls: {} |
There was a problem hiding this comment.
curious why we have an empty tls struct here
Signed-off-by: zirain <zirain2009@gmail.com>
| @@ -516,7 +516,8 @@ func (t *Translator) processGRPCRouteRules(grpcRoute *GRPCRouteContext, parentRe | |||
|
|
|||
| // If the route has no valid backends then just use a direct response and don't fuss with weighted responses | |||
| for _, ruleRoute := range ruleRoutes { | |||
| if ruleRoute.Destination == nil && ruleRoute.Redirect == nil { | |||
| noValidBackends := ruleRoute.Destination == nil || ruleRoute.Destination.ToBackendWeights().Valid == 0 | |||
There was a problem hiding this comment.
shouldnt this be invalidBackendsExist instead of noValidBackends i.e. some instead of all ?
There was a problem hiding this comment.
noValidBackends = invalidBackendsExist | emptyBackends
There was a problem hiding this comment.
but noValidBackends also implies validBackends == 0 which is not true here
There was a problem hiding this comment.
in current code validBackends == 0 equals emptyBackends
Signed-off-by: zirain <zirain2009@gmail.com>
| @@ -1158,12 +1136,12 @@ func (t *Translator) processDestination(backendRefContext BackendRefContext, | |||
|
|
|||
| backendNamespace := NamespaceDerefOr(backendRef.Namespace, route.GetNamespace()) | |||
| if !t.validateBackendRef(backendRefContext, parentRef, route, resources, backendNamespace, routeType) { | |||
| return nil, weight | |||
| return &ir.DestinationSetting{Weight: &weight} | |||
There was a problem hiding this comment.
can we add a comment here highlighting that this is referring to an invalid weight
| } | ||
| } | ||
|
|
||
| // If the route has no valid backends then just use a direct response and don't fuss with weighted responses | ||
| for _, ruleRoute := range ruleRoutes { | ||
| if ruleRoute.Destination == nil && ruleRoute.Redirect == nil { | ||
| noValidBackends := ruleRoute.Destination == nil || ruleRoute.Destination.ToBackendWeights().Valid == 0 | ||
| if noValidBackends && ruleRoute.Redirect == nil { |
There was a problem hiding this comment.
isnt something like this needed in processHTTPRouteRules as well ?
|
/retest |
fix: #3174