Skip to content

Commit 9cd3375

Browse files
nvxbugeaswars
nvxbug
authored and
easwars
committed
xds: convert rds header exact/prefix/suffix/contains matches to StringMatcher (grpc#9223)
The RDS parser stored a header matcher's exact_match, prefix_match, or suffix_match without checking for the empty string, so a route carrying `exact_match: ""` (a valid proto oneof selection) was accepted. RouteToMatcher later tested those fields with `*h.X != ""` and treated an empty value as unset, so it fell through every arm of its header switch to `panic("illegal route: missing header_match_specifier")`. That runs when the client resolver builds a config selector and when the server processes an inline route config, neither behind a recover, so a management server could crash the client or an xds-enabled server with a single route. Per review feedback, instead of adding per-field empty checks the parser now converts the deprecated exact_match, prefix_match, suffix_match, and contains_match fields to the equivalent StringMatcher and runs them through matcher.StringMatcherFromProto, so validation for all of them is consolidated in one place. As a result: * an empty prefix_match, suffix_match, or contains_match is rejected and the resource is NACKed * an empty exact_match is accepted and matches an empty header value, per StringMatcher semantics, instead of panicking * contains_match is now supported for route header matching * RouteToMatcher drops its per-field `!= ""` special casing, and the ExactMatch/PrefixMatch/SuffixMatch fields are removed from the internal HeaderMatcher type RELEASE NOTES: * xds: fix a panic parsing a route whose header matcher has an empty exact_match, prefix_match, or suffix_match, and add support for contains_match in route header matchers
1 parent 8ce3ebf commit 9cd3375

5 files changed

Lines changed: 165 additions & 33 deletions

File tree

‎internal/xds/xdsclient/xdsresource/matcher.go‎

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -50,14 +50,8 @@ func RouteToMatcher(r *Route) *CompositeMatcher {
5050
var matcherT matcher.HeaderMatcher
5151
invert := h.InvertMatch != nil && *h.InvertMatch
5252
switch {
53-
case h.ExactMatch != nil && *h.ExactMatch != "":
54-
matcherT = matcher.NewHeaderExactMatcher(h.Name, *h.ExactMatch, invert)
5553
case h.RegexMatch != nil:
5654
matcherT = matcher.NewHeaderRegexMatcher(h.Name, h.RegexMatch, invert)
57-
case h.PrefixMatch != nil && *h.PrefixMatch != "":
58-
matcherT = matcher.NewHeaderPrefixMatcher(h.Name, *h.PrefixMatch, invert)
59-
case h.SuffixMatch != nil && *h.SuffixMatch != "":
60-
matcherT = matcher.NewHeaderSuffixMatcher(h.Name, *h.SuffixMatch, invert)
6155
case h.RangeMatch != nil:
6256
matcherT = matcher.NewHeaderRangeMatcher(h.Name, h.RangeMatch.Start, h.RangeMatch.End, invert)
6357
case h.PresentMatch != nil:

‎internal/xds/xdsclient/xdsresource/type_rds.go‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -166,14 +166,12 @@ type WeightedCluster struct {
166166
HTTPFilterConfigOverride map[string]httpfilter.FilterConfig
167167
}
168168

169-
// HeaderMatcher represents header matchers.
169+
// HeaderMatcher represents header matchers. Exact, prefix, suffix and contains
170+
// matches are represented as a StringMatch.
170171
type HeaderMatcher struct {
171172
Name string
172173
InvertMatch *bool
173-
ExactMatch *string
174174
RegexMatch *regexp.Regexp
175-
PrefixMatch *string
176-
SuffixMatch *string
177175
RangeMatch *Int64Range
178176
PresentMatch *bool
179177
StringMatch *matcher.StringMatcher

‎internal/xds/xdsclient/xdsresource/unmarshal_rds.go‎

Lines changed: 20 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@ import (
3333
"google.golang.org/protobuf/types/known/anypb"
3434

3535
v3routepb "github.com/envoyproxy/go-control-plane/envoy/config/route/v3"
36+
v3matcherpb "github.com/envoyproxy/go-control-plane/envoy/type/matcher/v3"
3637
v3typepb "github.com/envoyproxy/go-control-plane/envoy/type/v3"
3738
)
3839

@@ -256,9 +257,13 @@ func routesProtoToSlice(routes []*v3routepb.Route, csps map[string]clusterspecif
256257

257258
for _, h := range match.GetHeaders() {
258259
var header HeaderMatcher
260+
// The deprecated exact/prefix/suffix/contains match fields are
261+
// converted to the equivalent StringMatcher so that
262+
// StringMatcherFromProto owns validation for all of them.
263+
var smProto *v3matcherpb.StringMatcher
259264
switch ht := h.GetHeaderMatchSpecifier().(type) {
260265
case *v3routepb.HeaderMatcher_ExactMatch:
261-
header.ExactMatch = &ht.ExactMatch
266+
smProto = &v3matcherpb.StringMatcher{MatchPattern: &v3matcherpb.StringMatcher_Exact{Exact: ht.ExactMatch}}
262267
case *v3routepb.HeaderMatcher_SafeRegexMatch:
263268
regex := ht.SafeRegexMatch.GetRegex()
264269
re, err := matcher.CompileSafeRegex(regex)
@@ -274,18 +279,26 @@ func routesProtoToSlice(routes []*v3routepb.Route, csps map[string]clusterspecif
274279
case *v3routepb.HeaderMatcher_PresentMatch:
275280
header.PresentMatch = &ht.PresentMatch
276281
case *v3routepb.HeaderMatcher_PrefixMatch:
277-
header.PrefixMatch = &ht.PrefixMatch
282+
smProto = &v3matcherpb.StringMatcher{MatchPattern: &v3matcherpb.StringMatcher_Prefix{Prefix: ht.PrefixMatch}}
278283
case *v3routepb.HeaderMatcher_SuffixMatch:
279-
header.SuffixMatch = &ht.SuffixMatch
284+
smProto = &v3matcherpb.StringMatcher{MatchPattern: &v3matcherpb.StringMatcher_Suffix{Suffix: ht.SuffixMatch}}
285+
case *v3routepb.HeaderMatcher_ContainsMatch:
286+
smProto = &v3matcherpb.StringMatcher{MatchPattern: &v3matcherpb.StringMatcher_Contains{Contains: ht.ContainsMatch}}
280287
case *v3routepb.HeaderMatcher_StringMatch:
281-
sm, err := matcher.StringMatcherFromProto(ht.StringMatch)
282-
if err != nil {
283-
return nil, nil, fmt.Errorf("route %+v has an invalid string matcher: %v", err, ht.StringMatch)
288+
if ht.StringMatch == nil {
289+
return nil, nil, fmt.Errorf("route %+v has an empty string matcher", r)
284290
}
285-
header.StringMatch = &sm
291+
smProto = ht.StringMatch
286292
default:
287293
return nil, nil, fmt.Errorf("route %+v has an unrecognized header matcher: %+v", r, ht)
288294
}
295+
if smProto != nil {
296+
sm, err := matcher.StringMatcherFromProto(smProto)
297+
if err != nil {
298+
return nil, nil, fmt.Errorf("route %+v has an invalid header matcher: %v", r, err)
299+
}
300+
header.StringMatch = &sm
301+
}
289302
header.Name = h.GetName()
290303
invert := h.GetInvertMatch()
291304
header.InvertMatch = &invert

‎internal/xds/xdsclient/xdsresource/unmarshal_rds_test.go‎

Lines changed: 142 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -999,6 +999,10 @@ func (s) TestUnmarshalRouteConfig(t *testing.T) {
999999

10001000
func (s) TestRoutesProtoToSlice(t *testing.T) {
10011001
sm, _ := matcher.StringMatcherFromProto(&v3matcherpb.StringMatcher{MatchPattern: &v3matcherpb.StringMatcher_Exact{Exact: "tv"}})
1002+
prefixSM, _ := matcher.StringMatcherFromProto(&v3matcherpb.StringMatcher{MatchPattern: &v3matcherpb.StringMatcher_Prefix{Prefix: "tv"}})
1003+
suffixSM, _ := matcher.StringMatcherFromProto(&v3matcherpb.StringMatcher{MatchPattern: &v3matcherpb.StringMatcher_Suffix{Suffix: "tv"}})
1004+
containsSM, _ := matcher.StringMatcherFromProto(&v3matcherpb.StringMatcher{MatchPattern: &v3matcherpb.StringMatcher_Contains{Contains: "tv"}})
1005+
emptyExactSM, _ := matcher.StringMatcherFromProto(&v3matcherpb.StringMatcher{MatchPattern: &v3matcherpb.StringMatcher_Exact{Exact: ""}})
10021006
var (
10031007
goodRouteWithFilterConfigs = func(cfgs map[string]*anypb.Any) []*v3routepb.Route {
10041008
// Sets per-filter config in cluster "B" and in the route.
@@ -1113,7 +1117,7 @@ func (s) TestRoutesProtoToSlice(t *testing.T) {
11131117
{
11141118
Name: "th",
11151119
InvertMatch: newBoolP(true),
1116-
PrefixMatch: newStringP("tv"),
1120+
StringMatch: &prefixSM,
11171121
},
11181122
},
11191123
Fraction: newUInt32P(10000),
@@ -1308,20 +1312,142 @@ func (s) TestRoutesProtoToSlice(t *testing.T) {
13081312
wantErr: true,
13091313
},
13101314
{
1311-
name: "unrecognized header match specifier",
1312-
routes: []*v3routepb.Route{
1313-
{
1314-
Match: &v3routepb.RouteMatch{
1315-
PathSpecifier: &v3routepb.RouteMatch_Prefix{Prefix: "/a/"},
1316-
Headers: []*v3routepb.HeaderMatcher{
1317-
{
1318-
Name: "th",
1319-
HeaderMatchSpecifier: &v3routepb.HeaderMatcher_StringMatch{},
1320-
},
1321-
},
1322-
},
1315+
name: "empty exact_match header specifier is accepted",
1316+
routes: []*v3routepb.Route{{
1317+
Match: &v3routepb.RouteMatch{
1318+
PathSpecifier: &v3routepb.RouteMatch_Prefix{Prefix: "/a/"},
1319+
Headers: []*v3routepb.HeaderMatcher{{
1320+
Name: "th",
1321+
HeaderMatchSpecifier: &v3routepb.HeaderMatcher_ExactMatch{ExactMatch: ""},
1322+
}},
13231323
},
1324-
},
1324+
Action: &v3routepb.Route_Route{
1325+
Route: &v3routepb.RouteAction{ClusterSpecifier: &v3routepb.RouteAction_Cluster{Cluster: clusterName}},
1326+
},
1327+
}},
1328+
wantRoutes: []*Route{{
1329+
Prefix: newStringP("/a/"),
1330+
Headers: []*HeaderMatcher{{
1331+
Name: "th",
1332+
InvertMatch: newBoolP(false),
1333+
StringMatch: &emptyExactSM,
1334+
}},
1335+
WeightedClusters: []WeightedCluster{{Name: clusterName, Weight: 1}},
1336+
ActionType: RouteActionRoute,
1337+
}},
1338+
wantErr: false,
1339+
},
1340+
{
1341+
name: "suffix_match header specifier",
1342+
routes: []*v3routepb.Route{{
1343+
Match: &v3routepb.RouteMatch{
1344+
PathSpecifier: &v3routepb.RouteMatch_Prefix{Prefix: "/a/"},
1345+
Headers: []*v3routepb.HeaderMatcher{{
1346+
Name: "th",
1347+
HeaderMatchSpecifier: &v3routepb.HeaderMatcher_SuffixMatch{SuffixMatch: "tv"},
1348+
}},
1349+
},
1350+
Action: &v3routepb.Route_Route{
1351+
Route: &v3routepb.RouteAction{ClusterSpecifier: &v3routepb.RouteAction_Cluster{Cluster: clusterName}},
1352+
},
1353+
}},
1354+
wantRoutes: []*Route{{
1355+
Prefix: newStringP("/a/"),
1356+
Headers: []*HeaderMatcher{{
1357+
Name: "th",
1358+
InvertMatch: newBoolP(false),
1359+
StringMatch: &suffixSM,
1360+
}},
1361+
WeightedClusters: []WeightedCluster{{Name: clusterName, Weight: 1}},
1362+
ActionType: RouteActionRoute,
1363+
}},
1364+
wantErr: false,
1365+
},
1366+
{
1367+
name: "contains_match header specifier",
1368+
routes: []*v3routepb.Route{{
1369+
Match: &v3routepb.RouteMatch{
1370+
PathSpecifier: &v3routepb.RouteMatch_Prefix{Prefix: "/a/"},
1371+
Headers: []*v3routepb.HeaderMatcher{{
1372+
Name: "th",
1373+
HeaderMatchSpecifier: &v3routepb.HeaderMatcher_ContainsMatch{ContainsMatch: "tv"},
1374+
}},
1375+
},
1376+
Action: &v3routepb.Route_Route{
1377+
Route: &v3routepb.RouteAction{ClusterSpecifier: &v3routepb.RouteAction_Cluster{Cluster: clusterName}},
1378+
},
1379+
}},
1380+
wantRoutes: []*Route{{
1381+
Prefix: newStringP("/a/"),
1382+
Headers: []*HeaderMatcher{{
1383+
Name: "th",
1384+
InvertMatch: newBoolP(false),
1385+
StringMatch: &containsSM,
1386+
}},
1387+
WeightedClusters: []WeightedCluster{{Name: clusterName, Weight: 1}},
1388+
ActionType: RouteActionRoute,
1389+
}},
1390+
wantErr: false,
1391+
},
1392+
{
1393+
name: "empty contains_match header specifier",
1394+
routes: []*v3routepb.Route{{
1395+
Match: &v3routepb.RouteMatch{
1396+
PathSpecifier: &v3routepb.RouteMatch_Prefix{Prefix: "/a/"},
1397+
Headers: []*v3routepb.HeaderMatcher{{
1398+
Name: "th",
1399+
HeaderMatchSpecifier: &v3routepb.HeaderMatcher_ContainsMatch{ContainsMatch: ""},
1400+
}},
1401+
},
1402+
Action: &v3routepb.Route_Route{
1403+
Route: &v3routepb.RouteAction{ClusterSpecifier: &v3routepb.RouteAction_Cluster{Cluster: clusterName}},
1404+
},
1405+
}},
1406+
wantErr: true,
1407+
},
1408+
{
1409+
name: "empty prefix_match header specifier",
1410+
routes: []*v3routepb.Route{{
1411+
Match: &v3routepb.RouteMatch{
1412+
PathSpecifier: &v3routepb.RouteMatch_Prefix{Prefix: "/a/"},
1413+
Headers: []*v3routepb.HeaderMatcher{{
1414+
Name: "th",
1415+
HeaderMatchSpecifier: &v3routepb.HeaderMatcher_PrefixMatch{PrefixMatch: ""},
1416+
}},
1417+
},
1418+
Action: &v3routepb.Route_Route{
1419+
Route: &v3routepb.RouteAction{ClusterSpecifier: &v3routepb.RouteAction_Cluster{Cluster: clusterName}},
1420+
},
1421+
}},
1422+
wantErr: true,
1423+
},
1424+
{
1425+
name: "empty suffix_match header specifier",
1426+
routes: []*v3routepb.Route{{
1427+
Match: &v3routepb.RouteMatch{
1428+
PathSpecifier: &v3routepb.RouteMatch_Prefix{Prefix: "/a/"},
1429+
Headers: []*v3routepb.HeaderMatcher{{
1430+
Name: "th",
1431+
HeaderMatchSpecifier: &v3routepb.HeaderMatcher_SuffixMatch{SuffixMatch: ""},
1432+
}},
1433+
},
1434+
Action: &v3routepb.Route_Route{
1435+
Route: &v3routepb.RouteAction{ClusterSpecifier: &v3routepb.RouteAction_Cluster{Cluster: clusterName}},
1436+
},
1437+
}},
1438+
wantErr: true,
1439+
},
1440+
{
1441+
name: "nil string match header specifier",
1442+
routes: []*v3routepb.Route{{
1443+
Match: &v3routepb.RouteMatch{
1444+
PathSpecifier: &v3routepb.RouteMatch_Prefix{Prefix: "/a/"},
1445+
Headers: []*v3routepb.HeaderMatcher{{
1446+
Name: "th",
1447+
HeaderMatchSpecifier: &v3routepb.HeaderMatcher_StringMatch{},
1448+
}},
1449+
},
1450+
}},
13251451
wantErr: true,
13261452
},
13271453
{
@@ -1492,7 +1618,7 @@ func (s) TestRoutesProtoToSlice(t *testing.T) {
14921618
{
14931619
Name: "th",
14941620
InvertMatch: newBoolP(true),
1495-
PrefixMatch: newStringP("tv"),
1621+
StringMatch: &prefixSM,
14961622
},
14971623
},
14981624
Fraction: newUInt32P(10000),
@@ -1552,7 +1678,7 @@ func (s) TestRoutesProtoToSlice(t *testing.T) {
15521678
{
15531679
Name: "th",
15541680
InvertMatch: newBoolP(true),
1555-
PrefixMatch: newStringP("tv"),
1681+
StringMatch: &prefixSM,
15561682
},
15571683
},
15581684
Fraction: newUInt32P(10000),

‎scripts/vet.sh‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -196,6 +196,7 @@ balancer.ErrTransientFailure is deprecated:
196196
grpc/reflection/v1alpha/reflection.proto
197197
SwitchTo is deprecated:
198198
XXXXX xDS deprecated fields we support
199+
.ContainsMatch
199200
.ExactMatch
200201
.PrefixMatch
201202
.SafeRegexMatch

0 commit comments

Comments
 (0)