Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

matchers: add CEL matcher, input #31

Merged
merged 12 commits into from
May 18, 2022
Merged

matchers: add CEL matcher, input #31

merged 12 commits into from
May 18, 2022

Conversation

sergiitk
Copy link
Contributor

@sergiitk sergiitk commented Mar 8, 2022 •
edited

Common Expression Language (CEL) matchers.

@markdroth @kyessenov @yanavlasov

@kyessenov kyessenov mentioned this pull request Apr 1, 2022
Merged
@kyessenov
Copy link
Contributor

It might be worth adding something like skip_if_error from https://www.envoyproxy.io/docs/envoy/latest/api-v3/extensions/rate_limit_descriptors/expr/v3/expr.proto#envoy-v3-api-msg-extensions-rate-limit-descriptors-expr-v3-descriptor. If there is an error evaluating the expression, should the input produce anything?

@sergiitk
Copy link
Contributor Author

Thanks - I'll take a look. I just rebased it for now, and making the changes per our discussion here #31 (comment), and some other updates I have.

@sergiitk sergiitk force-pushed the xds-matchers-cel branch 2 times, most recently from 063fc8e to c4dfa30 Compare May 10, 2022 21:18
@sergiitk
Copy link
Contributor Author

Re: skip_if_error discussed IRL. Decided for this particular purpose of using in unified matchers, makes more sense to just clarify it in the comment.

@sergiitk sergiitk marked this pull request as ready for review May 12, 2022 01:03
@sergiitk
Copy link
Contributor Author

@markdroth @yanavlasov @tyxia Please take a look. I'm yet to fix the build (the problem with including googleapis dependencies need to be solved), but the protos are ready.

@sergiitk
Copy link
Contributor Author

The doc will look something like this:

image

@sergiitk sergiitk force-pushed the xds-matchers-cel branch 2 times, most recently from 454b573 to d551ff0 Compare May 13, 2022 17:54
markdroth
markdroth reviewed May 13, 2022
View reviewed changes
Copy link
Contributor

@markdroth markdroth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall structure looks good! Just a couple of organizational comments.

xds/type/matcher/v3/cel.proto Outdated Show resolved Hide resolved
xds/type/matcher/v3/cel.proto Outdated Show resolved Hide resolved
@sergiitk
Copy link
Contributor Author

Thanks for the quick review, @markdroth. Addressed your comments.

@sergiitk
matchers: add CEL matcher, input
22cdc82 
Signed-off-by: Sergii Tkachenko <sergiitk@google.com>
@sergiitk
Update HttpCelMatchInput per review
e1918a5 
- Remove Attributes from HttpCelMatchInput
- Rename to HttpAttributesMatchInput

Signed-off-by: Sergii Tkachenko <sergiitk@google.com>
@sergiitk
Use rst reference instead.
4a2f696 
Signed-off-by: Sergii Tkachenko <sergiitk@google.com>
@sergiitk
CelExpression WIP
12fc6ba 
Signed-off-by: Sergii Tkachenko <sergiitk@google.com>
@sergiitk
HttpAttributesCelMatchInput WIP
47eb9f1 
Signed-off-by: Sergii Tkachenko <sergiitk@google.com>
@sergiitk
Reset xds/type/matcher/v3/BUILD
a5397d2 
Signed-off-by: Sergii Tkachenko <sergiitk@google.com>
@sergiitk
Finish HttpAttributesCelMatchInput - address skip_if_error
d95408c 
Signed-off-by: Sergii Tkachenko <sergiitk@google.com>
@sergiitk
Revert to cel-as-matcher approach
ddda424 
Signed-off-by: Sergii Tkachenko <sergiitk@google.com>
@markdroth
Copy link
Contributor

For future reference, please don't force-push once a review has started; that makes it harder for the reviewer to see what's changed. If you need to merge master, just merge master into your branch and push that merge commit.

@markdroth
Copy link
Contributor

This looks good to me! Just need to fix CI.

@sergiitk
Copy link
Contributor Author

For future reference, please don't force-push once a review has started; that makes it harder for the reviewer to see what's changed. If you need to merge master, just merge master into your branch and push that merge commit.

My apologies, normally I reserve to this in exceptional circumstances. This time I have accidentally overridden AuthorDate in all commits, when was adding missing --signoff.

@sergiitk
Fixes per review: CEL matcher name, CelExtractString package
dfb64cd 
Signed-off-by: Sergii Tkachenko <sergiitk@google.com>
@sergiitk
Copy link
Contributor Author

Force-pushed it again while setting up automatic signoff :( Sorry

@sergiitk
Build fix, except //test/build:go_build_test
9e3d443 
Signed-off-by: Sergii Tkachenko <sergiitk@google.com>
@sergiitk
Address //test/build:go_build_test build error
e533e3d 
Signed-off-by: Sergii Tkachenko <sergiitk@google.com>
@sergiitk
Generate protobuf for the new files
8aa54f1 
Signed-off-by: Sergii Tkachenko <sergiitk@google.com>
@sergiitk sergiitk requested a review from markdroth May 17, 2022 18:08
@sergiitk
Copy link
Contributor Author

@kyessenov @markdroth ready for re-review. The build is fixed.

Note: there's a couple of TODOs. I'm planning to address them in a follow up PR, once envoy picks up this change, and doc generation works.

@sergiitk sergiitk mentioned this pull request May 17, 2022
Closed
markdroth
markdroth approved these changes May 18, 2022
View reviewed changes
Copy link
Contributor

@markdroth markdroth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

@markdroth markdroth merged commit d35b9e6 into cncf:main May 18, 2022
@sergiitk sergiitk deleted the xds-matchers-cel branch May 19, 2022 00:09
@zhxie zhxie mentioned this pull request May 26, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kyessenov kyessenov kyessenov left review comments

@markdroth markdroth markdroth approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@sergiitk @kyessenov @markdroth