xds: make it possible to create a StringMatcher from arguments outside of test code #8723
Conversation
easwars
added
Area: xDS
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #8723 +/- ##
==========================================
- Coverage 83.21% 83.17% -0.05%
==========================================
Files 419 419
Lines 32427 32423 -4
==========================================
- Hits 26985 26968 -17
- Misses 4054 4066 +12
- Partials 1388 1389 +1
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the StringMatcher API to make it usable outside of test code and fixes a bug where regex matchers incorrectly considered the ignore_case field. The key changes include:
- Renaming
StringMatcherForTestingtoNewStringMatcherto expose it for general use - Creating new string pointers instead of reusing proto pointers to enable earlier garbage collection of large xDS proto structs
- Fixing the regex matcher to correctly ignore the
ignore_casefield as documented
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/xds/matcher/string_matcher.go | Refactored Match() to apply ignoreCase within each case block (fixing regex bug), added newStrPtr() helper for pointer allocation, and renamed StringMatcherForTesting to NewStringMatcher |
| internal/xds/matcher/string_matcher_test.go | Added test cases to verify regex matchers correctly ignore the ignore_case field |
| internal/xds/xdsclient/xdsresource/unmarshal_cds_test.go | Updated test calls from StringMatcherForTesting to NewStringMatcher |
| internal/credentials/xds/handshake_info_test.go | Updated test calls from StringMatcherForTesting to NewStringMatcher |
| credentials/xds/xds_client_test.go | Updated test calls from StringMatcherForTesting to NewStringMatcher |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // NewStringMatcher is a helper function to create a StringMatcher based | ||
| // on the given arguments. At most one of exact, prefix, suffix, contains and | ||
| // regex should be non-nil. | ||
| func NewStringMatcher(exact, prefix, suffix, contains *string, regex *regexp.Regexp, ignoreCase bool) StringMatcher { | ||
| return StringMatcher{ | ||
| exactMatch: newStrPtr(exact, ignoreCase), | ||
| prefixMatch: newStrPtr(prefix, ignoreCase), | ||
| suffixMatch: newStrPtr(suffix, ignoreCase), | ||
| containsMatch: newStrPtr(contains, ignoreCase), | ||
| regexMatch: regex, | ||
| containsMatch: contains, | ||
| ignoreCase: ignoreCase, | ||
| } | ||
| if ignoreCase { | ||
| switch { | ||
| case sm.exactMatch != nil: | ||
| *sm.exactMatch = strings.ToLower(*exact) | ||
| case sm.prefixMatch != nil: | ||
| *sm.prefixMatch = strings.ToLower(*prefix) | ||
| case sm.suffixMatch != nil: | ||
| *sm.suffixMatch = strings.ToLower(*suffix) | ||
| case sm.containsMatch != nil: | ||
| *sm.containsMatch = strings.ToLower(*contains) | ||
| } | ||
| } | ||
| return sm | ||
| } |
There was a problem hiding this comment.
[nitpick] The documentation states "At most one of exact, prefix, suffix, contains and regex should be non-nil," but the function doesn't validate or enforce this constraint. Consider adding validation to return an error or panic if multiple match types are provided, or document that the behavior is undefined when multiple parameters are non-nil.
Since the Match() method uses a switch statement that checks in order (exact, prefix, suffix, contains, regex), if multiple parameters are provided, only the first non-nil one will be used, which could lead to confusing behavior.
| suffixMatch: suffix, | ||
| // NewStringMatcher is a helper function to create a StringMatcher based | ||
| // on the given arguments. At most one of exact, prefix, suffix, contains and | ||
| // regex should be non-nil. |
There was a problem hiding this comment.
The documentation should clarify that the ignoreCase parameter has no effect when regex is provided, as stated in the StringMatcher struct comment (line 44-45). This would help users understand that setting ignoreCase=true with a regex matcher will not perform case-insensitive matching.
| // regex should be non-nil. | |
| // regex should be non-nil. The ignoreCase parameter has no effect when regex is provided. |
2 participants
Change in this PR:
ignore_casefield, but was.RELEASE NOTES:
StringMatcherwhere regexes would match incorrectly when ignore_case is set to true.