xds: Make regex matchers match on full string, not just partial match #4875
Conversation
zasweq
requested review from
easwars and
menghanl
and removed request for
easwars
| @@ -91,7 +92,8 @@ func (hrm *HeaderRegexMatcher) Match(md metadata.MD) bool { | |||
| if !ok { | |||
| return false | |||
| } | |||
| return hrm.re.MatchString(v) | |||
| rem := hrm.re.FindString(v) | |||
There was a problem hiding this comment.
Make a function (in grpcutil) to
- call longest()
- FindString and compare length
There was a problem hiding this comment.
Isn't calling longest() more than once inefficient...it is cleaner though. It makes regex bool longest = true, then is a no op afterward. Added though.
There was a problem hiding this comment.
I think it's cleaner to make longest() an implementation detail of the compare() function.
And did you forget to push?
There was a problem hiding this comment.
No, it took me longer than expected ngl :P.
zasweq
force-pushed
the
regex-partial-matcher-fix
branch
from
1e546e9
to
ae86d3f
Compare
| want: false, | ||
| }, | ||
| { | ||
| name: "match because fully match", |
There was a problem hiding this comment.
Add a test case for Longest.
Use the regex from the example: https://pkg.go.dev/regexp#Regexp.Longest
Or https://play.golang.org/p/Z682jeK2IH6
There was a problem hiding this comment.
Done. Pinged Eric about interop because of vet problem on master.
|
Try rebasing? The vet failure was fixed by #4877. |
zasweq
force-pushed
the
regex-partial-matcher-fix
branch
from
59ef17a
to
33461b6
Compare
dfawley
changed the title
This PR fixes issue #4852. Previously, regex matchers returned true even if the string had a portion that matched with the regex. The proto documentation mentions that the full value needs to match the regex, not just a portion, and this PR switches the matchers to implement that.
Fixes #4852.
RELEASE NOTES: None