xds: convert rds header exact/prefix/suffix/contains matches to StringMatcher

[nvxbug]

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

[easwars]

What do you think about using a StringMatcher from HeaderMatcher wherever possible, so that we can consolidate all these checks in one place. The StringMatcher type already includes a bunch of checks. See:

grpc-go/internal/xds/matcher/string_matcher.go

Line 35 in f8a85fa

type StringMatcher struct {

[nvxbug]

Makes sense. Routing the exact/prefix/suffix arms through matcher.StringMatcher would let StringMatcherFromProto own the empty checks, and it also lets RouteToMatcher drop its per-field != "" special casing.

One thing worth confirming before I refactor: StringMatcherFromProto rejects an empty prefix/suffix (and contains), but it accepts an empty exact. So prefix/suffix stay a NACK as they are now, but an empty exact_match would go from the current panic to an accepted matcher that matches an empty header value, rather than being rejected. Are you fine with empty exact_match being accepted that way, or would you rather keep rejecting it? Happy to fold the change into this PR once you confirm which behavior you want for exact.

[easwars]

An empty exact matcher being accepted is the correct behavior. So, I'm fine with changing it.

I'm also fine with adding support for the Contains matcher in HeaderMatcher, and also routing it through the StringMatcher.

[nvxbug]

Done. The parser now converts exact/prefix/suffix/contains to the equivalent StringMatcher and runs them through StringMatcherFromProto, so the checks are all in one place: empty exact is accepted, empty prefix/suffix/contains are NACKed, and contains_match is now supported. RouteToMatcher drops its per-field special casing and the ExactMatch/PrefixMatch/SuffixMatch fields are gone from the internal HeaderMatcher type. Updated the PR title and description to match.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.27%. Comparing base (2a112a8) to head (a2fb7c7).
⚠️ Report is 8 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #9223      +/-   ##
==========================================
- Coverage   83.33%   83.27%   -0.07%     
==========================================
  Files         420      421       +1     
  Lines       34028    34087      +59     
==========================================
+ Hits        28358    28386      +28     
- Misses       4253     4270      +17     
- Partials     1417     1431      +14     

Files with missing lines	Coverage Δ
internal/xds/xdsclient/xdsresource/matcher.go	72.07% <ø> (+3.69%)	⬆️
...nternal/xds/xdsclient/xdsresource/unmarshal_rds.go	89.69% <100.00%> (+1.06%)	⬆️

... and 29 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[easwars]

The static checks are failing with the following error:

Error: internal/xds/xdsclient/xdsresource/unmarshal_rds.go:286:102: ht.ContainsMatch is deprecated: Marked as deprecated in envoy/config/route/v3/route_components.proto.  (SA1019)

Could you please add ContainsMatch to the list of ignored deprecation warnings in

grpc-go/scripts/vet.sh

Line 166 in 75f3c0b

# Only ignore the following deprecated types/fields/functions and exclude

[nvxbug]

Added .ContainsMatch to the ignored deprecation list in vet.sh, so the SA1019 failure should be gone. Also picked up the two test nits: a suffix_match case and the compacted single-entry slice literals.

[easwars]

Moving to @mbissa for second set of eyes

[easwars]

/gemini review

[gemini-code-assist]

Code Review

This pull request refactors the handling of deprecated header matchers (ExactMatch, PrefixMatch, SuffixMatch, and ContainsMatch) in the xDS client. These fields are now converted to equivalent StringMatcher proto messages during unmarshaling, allowing StringMatcherFromProto to handle validation consistently. Consequently, the corresponding fields have been removed from the HeaderMatcher struct, and the associated tests and vet scripts have been updated to reflect this change. There are no review comments, and I have no feedback to provide.