xds: fix header matcher for null value

[YifeiZhuang]

https://github.com/envoyproxy/envoy/blob/0fae6970ddaf93f024908ba304bbd2b34e997a51/source/common/http/header_utility.cc#L130

[ejona86]

This only inverts the value if present() is set. Is that right? It also looks like it has been "simplified" in such a way that makes it harder to see the semantics. I sort of have to make a truth table to make sense of it.

Should it be instead:

return (present() != null && !present()) != inverted();

[YifeiZhuang]

This only inverts the value if present() is set. Is that right?

oh I see the difference between them.

matcher = HeaderMatcher.forExactValue("version", "v1", true);
 assertThat(matcher.matches(null)).isFalse(); // result is different

but isn't this only inverts the value if present() is set?
https://github.com/envoyproxy/envoy/blob/0fae6970ddaf93f024908ba304bbd2b34e997a51/source/common/http/header_utility.cc#L130

[ejona86]

Yeah, I agree that is what that code does. Okay. That is subtle. I can see how the documentation could imply that behavior, but it is something to be strongly careful of.

CC @yashykt, @zasweq, this is something we should audit in each language, to make sure we are consistent. (See the envoy link for the precise behavior)

[YifeiZhuang]

The semantics was that: if value is null, then it first must be a present matcher, then do a invert of the present() because negative present() is a match.
Your semantics is that:
If value is null, base match is true only if it is a present matcher and present() is defined negative, then do invert of the base match.

[ejona86]

The key is that "invert the matcher" does not change the behavior for null-ness. It is more SQL-style behavior where col = 'hi' and col <> 'hi' could both be false when col is NULL. All matchers except for present_match require the header to be present.