Skip to content

Commit

Permalink
xds: NACK route configuration if sum of weights of weighted clusters …
Browse files Browse the repository at this point in the history 
…exceeds uint32_max (#6085)
  • Loading branch information
@arvindbr8
arvindbr8 committed Mar 9, 2023
1 parent d02039b commit 92d9e77
Show file tree
Hide file tree
Showing 2 changed files with 10 additions and 78 deletions.
18 changes: 6 additions & 12 deletions xds/internal/xdsclient/xdsresource/unmarshal_rds.go
View file
Expand Up @@ -19,6 +19,7 @@ package xdsresource

import (
"fmt"
"math"
"regexp"
"strings"
"time"
Expand Down Expand Up @@ -313,29 +314,23 @@ func routesProtoToSlice(routes []*v3routepb.Route, csps map[string]clusterspecif
route.WeightedClusters[a.Cluster] = WeightedCluster{Weight: 1}
case *v3routepb.RouteAction_WeightedClusters:
wcs := a.WeightedClusters
var totalWeight uint32
var totalWeight uint64
for _, c := range wcs.Clusters {
w := c.GetWeight().GetValue()
if w == 0 {
continue
}
totalWeight += uint64(w)
if totalWeight > math.MaxUint32 {
return nil, nil, fmt.Errorf("xds: total weight of clusters exceeds MaxUint32")
}
wc := WeightedCluster{Weight: w}
cfgs, err := processHTTPFilterOverrides(c.GetTypedPerFilterConfig())
if err != nil {
return nil, nil, fmt.Errorf("route %+v, action %+v: %v", r, a, err)
}
wc.HTTPFilterConfigOverride = cfgs
route.WeightedClusters[c.GetName()] = wc
totalWeight += w
}
// envoy xds doc
// default TotalWeight https://www.envoyproxy.io/docs/envoy/latest/api-v3/config/route/v3/route_components.proto.html#envoy-v3-api-field-config-route-v3-weightedcluster-total-weight
wantTotalWeight := uint32(100)
if tw := wcs.GetTotalWeight(); tw != nil {
wantTotalWeight = tw.GetValue()
}
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)
}
if totalWeight == 0 {
return nil, nil, fmt.Errorf("route %+v, action %+v, has no valid cluster in WeightedCluster action", r, a)
Expand All @@ -347,7 +342,6 @@ func routesProtoToSlice(routes []*v3routepb.Route, csps map[string]clusterspecif
// cluster_specifier:
// - Can be Cluster
// - Can be Weighted_clusters
// - The sum of weights must add up to the total_weight.
// - Can be unset or an unsupported field. The route containing
// this action will be ignored.
//
Expand Down
70 changes: 4 additions & 66 deletions xds/internal/xdsclient/xdsresource/unmarshal_rds_test.go
View file
Expand Up @@ -20,6 +20,7 @@ package xdsresource
import (
"errors"
"fmt"
"math"
"regexp"
"testing"
"time"
Expand Down Expand Up @@ -411,38 +412,6 @@ func (s) TestRDSGenerateRDSUpdateFromRouteConfiguration(t *testing.T) {
},
},
},
{
// weights not add up to total-weight.
name: "route-config-with-weighted_clusters_weights_not_add_up",
rc: &v3routepb.RouteConfiguration{
Name: routeName,
VirtualHosts: []*v3routepb.VirtualHost{
{
Domains: []string{ldsTarget},
Routes: []*v3routepb.Route{
{
Match: &v3routepb.RouteMatch{PathSpecifier: &v3routepb.RouteMatch_Prefix{Prefix: "/"}},
Action: &v3routepb.Route_Route{
Route: &v3routepb.RouteAction{
ClusterSpecifier: &v3routepb.RouteAction_WeightedClusters{
WeightedClusters: &v3routepb.WeightedCluster{
Clusters: []*v3routepb.WeightedCluster_ClusterWeight{
{Name: "a", Weight: &wrapperspb.UInt32Value{Value: 2}},
{Name: "b", Weight: &wrapperspb.UInt32Value{Value: 3}},
{Name: "c", Weight: &wrapperspb.UInt32Value{Value: 5}},
},
TotalWeight: &wrapperspb.UInt32Value{Value: 30},
},
},
},
},
},
},
},
},
},
wantError: true,
},
{
name: "good-route-config-with-weighted_clusters",
rc: &v3routepb.RouteConfiguration{
Expand All @@ -462,7 +431,6 @@ func (s) TestRDSGenerateRDSUpdateFromRouteConfiguration(t *testing.T) {
{Name: "b", Weight: &wrapperspb.UInt32Value{Value: 3}},
{Name: "c", Weight: &wrapperspb.UInt32Value{Value: 5}},
},
TotalWeight: &wrapperspb.UInt32Value{Value: 10},
},
},
},
Expand Down Expand Up @@ -971,7 +939,6 @@ func (s) TestRoutesProtoToSlice(t *testing.T) {
{Name: "B", Weight: &wrapperspb.UInt32Value{Value: 60}, TypedPerFilterConfig: cfgs},
{Name: "A", Weight: &wrapperspb.UInt32Value{Value: 40}},
},
TotalWeight: &wrapperspb.UInt32Value{Value: 100},
}}}},
TypedPerFilterConfig: cfgs,
}}
Expand Down Expand Up @@ -1016,7 +983,6 @@ func (s) TestRoutesProtoToSlice(t *testing.T) {
{Name: "B", Weight: &wrapperspb.UInt32Value{Value: 60}},
{Name: "A", Weight: &wrapperspb.UInt32Value{Value: 40}},
},
TotalWeight: &wrapperspb.UInt32Value{Value: 100},
}}}},
}},
wantRoutes: []*Route{{
Expand Down Expand Up @@ -1056,7 +1022,6 @@ func (s) TestRoutesProtoToSlice(t *testing.T) {
{Name: "B", Weight: &wrapperspb.UInt32Value{Value: 60}},
{Name: "A", Weight: &wrapperspb.UInt32Value{Value: 40}},
},
TotalWeight: &wrapperspb.UInt32Value{Value: 100},
}}}},
},
},
Expand Down Expand Up @@ -1102,7 +1067,6 @@ func (s) TestRoutesProtoToSlice(t *testing.T) {
{Name: "B", Weight: &wrapperspb.UInt32Value{Value: 60}},
{Name: "A", Weight: &wrapperspb.UInt32Value{Value: 40}},
},
TotalWeight: &wrapperspb.UInt32Value{Value: 100},
}}}},
},
},
Expand Down Expand Up @@ -1136,7 +1100,6 @@ func (s) TestRoutesProtoToSlice(t *testing.T) {
{Name: "B", Weight: &wrapperspb.UInt32Value{Value: 60}},
{Name: "A", Weight: &wrapperspb.UInt32Value{Value: 40}},
},
TotalWeight: &wrapperspb.UInt32Value{Value: 100},
}}}},
},
{
Expand Down Expand Up @@ -1252,14 +1215,13 @@ func (s) TestRoutesProtoToSlice(t *testing.T) {
{Name: "B", Weight: &wrapperspb.UInt32Value{Value: 0}},
{Name: "A", Weight: &wrapperspb.UInt32Value{Value: 0}},
},
TotalWeight: &wrapperspb.UInt32Value{Value: 0},
}}}},
},
},
wantErr: true,
},
{
name: "totalWeight is nil in weighted clusters action",
name: "The sum of all weighted clusters is more than uint32",
routes: []*v3routepb.Route{
{
Match: &v3routepb.RouteMatch{
Expand All @@ -1270,30 +1232,9 @@ func (s) TestRoutesProtoToSlice(t *testing.T) {
ClusterSpecifier: &v3routepb.RouteAction_WeightedClusters{
WeightedClusters: &v3routepb.WeightedCluster{
Clusters: []*v3routepb.WeightedCluster_ClusterWeight{
{Name: "B", Weight: &wrapperspb.UInt32Value{Value: 20}},
{Name: "A", Weight: &wrapperspb.UInt32Value{Value: 30}},
},
}}}},
},
},
wantErr: true,
},
{
name: "The sum of all weighted clusters is not equal totalWeight",
routes: []*v3routepb.Route{
{
Match: &v3routepb.RouteMatch{
PathSpecifier: &v3routepb.RouteMatch_Prefix{Prefix: "/a/"},
},
Action: &v3routepb.Route_Route{
Route: &v3routepb.RouteAction{
ClusterSpecifier: &v3routepb.RouteAction_WeightedClusters{
WeightedClusters: &v3routepb.WeightedCluster{
Clusters: []*v3routepb.WeightedCluster_ClusterWeight{
{Name: "B", Weight: &wrapperspb.UInt32Value{Value: 50}},
{Name: "A", Weight: &wrapperspb.UInt32Value{Value: 20}},
{Name: "B", Weight: &wrapperspb.UInt32Value{Value: math.MaxUint32}},
{Name: "A", Weight: &wrapperspb.UInt32Value{Value: math.MaxUint32}},
},
TotalWeight: &wrapperspb.UInt32Value{Value: 100},
}}}},
},
},
Expand Down Expand Up @@ -1353,7 +1294,6 @@ func (s) TestRoutesProtoToSlice(t *testing.T) {
{Name: "B", Weight: &wrapperspb.UInt32Value{Value: 30}},
{Name: "A", Weight: &wrapperspb.UInt32Value{Value: 20}},
},
TotalWeight: &wrapperspb.UInt32Value{Value: 50},
}}}},
},
},
Expand Down Expand Up @@ -1394,7 +1334,6 @@ func (s) TestRoutesProtoToSlice(t *testing.T) {
{Name: "B", Weight: &wrapperspb.UInt32Value{Value: 60}},
{Name: "A", Weight: &wrapperspb.UInt32Value{Value: 40}},
},
TotalWeight: &wrapperspb.UInt32Value{Value: 100},
}},
HashPolicy: []*v3routepb.RouteAction_HashPolicy{
{PolicySpecifier: &v3routepb.RouteAction_HashPolicy_FilterState_{FilterState: &v3routepb.RouteAction_HashPolicy_FilterState{Key: "io.grpc.channel_id"}}},
Expand Down Expand Up @@ -1452,7 +1391,6 @@ func (s) TestRoutesProtoToSlice(t *testing.T) {
{Name: "B", Weight: &wrapperspb.UInt32Value{Value: 60}},
{Name: "A", Weight: &wrapperspb.UInt32Value{Value: 40}},
},
TotalWeight: &wrapperspb.UInt32Value{Value: 100},
}},
HashPolicy: []*v3routepb.RouteAction_HashPolicy{
{PolicySpecifier: &v3routepb.RouteAction_HashPolicy_Header_{Header: &v3routepb.RouteAction_HashPolicy_Header{HeaderName: ":path"}}},
Expand Down

0 comments on commit 92d9e77

Please sign in to comment.