xds: Fix invert functionality for header matcher #4902
Conversation
| } | ||
|
|
||
| func (hcm *HeaderContainsMatcher) String() string { | ||
| return fmt.Sprintf("headerContains:%v%v", hcm.key, hcm.contains) | ||
| } | ||
|
|
||
| /* |
| @@ -66,7 +67,7 @@ func (hem *HeaderExactMatcher) Match(md metadata.MD) bool { | |||
| if !ok { | |||
| return false | |||
| } | |||
| return v == hem.exact | |||
| return (v == hem.exact) != hem.invert | |||
There was a problem hiding this comment.
Copied from Envoy haha I didn't come up with this!!!!
| func NewHeaderPresentMatcher(key string, present bool) *HeaderPresentMatcher { | ||
| return &HeaderPresentMatcher{key: key, present: present} | ||
| func NewHeaderPresentMatcher(key string, present bool, invert bool) *HeaderPresentMatcher { | ||
| return &HeaderPresentMatcher{key: key, present: present, invert: invert} |
There was a problem hiding this comment.
Invert present? Then you don't need the invert bool field.
| { | ||
| name: "no match exact", | ||
| m: NewHeaderExactMatcher(":method", "GET", true), | ||
| md: metadata.Pairs("th", "GET"), |
There was a problem hiding this comment.
This only covers presence check, not the value check.
To be a bit paranoid, I think we need to test
- invert=true, key doesn't exist (returns false, except for PresentMatcher)
- invert=true, key exists, value matches (returns false)
- invert=true, key exists, value doesn't match (returns true)
And let's cover this for all the matchers.
You can add them to the each matcher test (add to TestHeaderExactMatcherMatch, instead of a new TestInvertWhenNotPresent
There was a problem hiding this comment.
Added these three tests for all the matchers. Deleted the invert tests, since those are not part of each matchers test.
| } | ||
|
|
||
| func (hcm *HeaderContainsMatcher) String() string { | ||
| return fmt.Sprintf("headerContains:%v%v", hcm.key, hcm.contains) | ||
| } | ||
|
|
||
| /* |
| @@ -66,7 +67,7 @@ func (hem *HeaderExactMatcher) Match(md metadata.MD) bool { | |||
| if !ok { | |||
| return false | |||
| } | |||
| return v == hem.exact | |||
| return (v == hem.exact) != hem.invert | |||
There was a problem hiding this comment.
Copied from Envoy haha I didn't come up with this!!!!
| func NewHeaderPresentMatcher(key string, present bool) *HeaderPresentMatcher { | ||
| return &HeaderPresentMatcher{key: key, present: present} | ||
| func NewHeaderPresentMatcher(key string, present bool, invert bool) *HeaderPresentMatcher { | ||
| return &HeaderPresentMatcher{key: key, present: present, invert: invert} |
| { | ||
| name: "no match exact", | ||
| m: NewHeaderExactMatcher(":method", "GET", true), | ||
| md: metadata.Pairs("th", "GET"), |
There was a problem hiding this comment.
Added these three tests for all the matchers. Deleted the invert tests, since those are not part of each matchers test.
dfawley
changed the title
Fixes #4896. This PR makes the header matchers present in grpc-go consistent with Envoy, as Envoy has a gatekeeper for each matcher that isn't a present matcher on whether the header is actually there, and if so returns false, regardless of the configured matchers comparator or the invert knob.
RELEASE NOTES: None