Matcher: add a "not" matcher

[aguinet]

For an explanation of how to fill out the fields, please see the relevant section
in PULL_REQUESTS.md

Commit Message: This adds a "not_matcher" matching type to MatcherList.
Additional Description:
Risk Level: low
Testing: unit test (extended field_matcher_test.cc)
Docs Changes: N/A
Release Notes: N/A
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]
[Optional API Considerations:]

[repokitteh-read-only]

Hi @aguinet, welcome and thank you for your contribution.

We will try to review your Pull Request as quickly as possible.

In the meantime, please take a look at the contribution guidelines if you have not done so already.

🐱

Caused by: #16149 was opened by aguinet.

see: more, trace.

[repokitteh-read-only]

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
envoyproxy/api-shepherds assignee is @htuch
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #16149 was opened by aguinet.

see: more, trace.

[htuch]

@aguinet this makes sense API-wise. Given how simple the API change is, I'd suggest folding the implementation & tests into this PR. Thanks.

@snowp FYI, how come we didn't include this to start with?

/wait

[aguinet]

@htuch I'm not sure to fully understand your suggestion. The implementation and tests are already in this PR. Do you want them in a separate commit/PR?

Thanks!

[snowp]

I think I didn't include it because I thought the on_no_match mechanism would be sufficient, but to get that working in the trivial case you need a noop action to terminate the match path (since we required an on_match for each matcher), which seems like a less than ideal pattern

[htuch]

Looks good, @aguinet can you fix DCO? Thanks
/wait

[aguinet]

Looks good, @aguinet can you fix DCO? Thanks
/wait

DCO fixed! Pushed --force

[htuch]

Looks like a legitimate drop in coverage reported by CI, see https://dev.azure.com/cncf/envoy/_build/results?buildId=73473&view=logs&j=bbe4b42d-86e6-5e9c-8a0b-fea01d818a24&t=e00c5a13-c6dc-5e9a-6104-69976170e881

[aguinet]

Looks like a legitimate drop in coverage reported by CI, see https://dev.azure.com/cncf/envoy/_build/results?buildId=73473&view=logs&j=bbe4b42d-86e6-5e9c-8a0b-fea01d818a24&t=e00c5a13-c6dc-5e9a-6104-69976170e881

Thanks @htuch for the heads up! I can't seem to find any sort of coverage report that would show the untested lines.

Indeed, the diff is pretty small, and it looks like I can't find what I don't test:

• https://github.com/envoyproxy/envoy/pull/16149/files#diff-3011cc577bd77d94e9ed8a6a069bb76226501c2460f563effaef5a7c11f4083fR109-R113 test the nominal case, which is basically when the branch here https://github.com/envoyproxy/envoy/pull/16149/files#diff-9b08faf2468daec26cd9cef36267762c52d4ea2cd6350a78cebce7598112fbc8R125 is not taken

• https://github.com/envoyproxy/envoy/pull/16149/files#diff-3011cc577bd77d94e9ed8a6a069bb76226501c2460f563effaef5a7c11f4083fR115-R124 should test when that branch is taken (but there might be a failure here)

Thanks!

[snowp]

https://storage.googleapis.com/envoy-pr/678e161/coverage/source/common/matcher/matcher.h.gcov.html shows that the factory branch is not hit

[aguinet]

https://storage.googleapis.com/envoy-pr/678e161/coverage/source/common/matcher/matcher.h.gcov.html shows that the factory branch is not hit

Thanks @snowp for the link. It was indeed a bit obviousy, sorry for this... I added a test (https://github.com/envoyproxy/envoy/pull/16149/files#diff-c8bf711a6054030b05da908d5fb918835cd40ced54f6e557ef26bff3b0ebaefcR276) that should fix it!

[htuch]

LGTM, thanks!

Please avoid force pushing in future PRs BTW, it makes it hard to follow changes.

[aguinet]

Okay @htuch I'll keep it in mind! Thanks for the merge and reviews!