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
xds: NACK route configuration if sum of weights of weighted clusters exceeds uint32_max #6085
Conversation
…exceeds uint32_max
@@ -326,17 +326,13 @@ func routesProtoToSlice(routes []*v3routepb.Route, csps map[string]clusterspecif | |||
} | |||
wc.HTTPFilterConfigOverride = cfgs | |||
route.WeightedClusters[c.GetName()] = wc | |||
|
|||
// Sum of all weights cannot exceed MaxUint32. Checking for overflow. | |||
if totalWeight > (totalWeight + w) { |
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.
What if we declared totalWeight
as a uint64
instead? Then we can unconditionally add w
to totalWeight
and simply check if totalWeight > math.MaxUint32
. I feel that will be more readable. What do you think?
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 agree. Updating.
|
||
// Sum of all weights cannot exceed MaxUint32. Checking for overflow. | ||
if totalWeight > (totalWeight + w) { | ||
return nil, nil, fmt.Errorf("route %+v, action %+v, has no valid cluster in WeightedCluster action", r, a) |
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.
This error message needs updating.
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.
oops.. done
} | ||
if totalWeight != wantTotalWeight { | ||
return nil, nil, fmt.Errorf("route %+v, action %+v, weights of clusters do not add up to total total weight, got: %v, expected total weight from response: %v", r, a, totalWeight, wantTotalWeight) | ||
totalWeight += uint64(w) |
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.
Minor nit. If you move this line to 322, then it will look simpler I feel:
totalWeight += uint64(w)
if totalWeight > math.MaxUint32 {
return nil, nil, fmt.Errorf("xds: total weight of clusters exceeds MaxUint32")
}
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.
Agreed. updating..
for _, c := range wcs.Clusters { | ||
w := c.GetWeight().GetValue() | ||
if w == 0 { | ||
continue | ||
} | ||
if (totalWeight + uint64(w)) > math.MaxUint32 { | ||
return nil, nil, fmt.Errorf("total weight of clusters exceed MaxUint32") |
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.
Nit: s/exceed/exceeds/
Also, add an xds:
prefix to the error string.
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.
done.
fixes #6053
RELEASE NOTES: