-
Notifications
You must be signed in to change notification settings - Fork 394
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.
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
feat: implement Lua EnvoyExtensionPolicy #5171
Conversation
8c8ef7b
to
4b9c54c
Compare
@arkodg @zhaohuabing Wanted to get an initial buy in regarding the approach while I add more unit tests and E2E, please do review. Thanks! |
4b9c54c
to
93c9c44
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5171 +/- ##
========================================
Coverage 66.94% 66.95%
========================================
Files 211 213 +2
Lines 33176 33360 +184
========================================
+ Hits 22210 22336 +126
- Misses 9620 9664 +44
- Partials 1346 1360 +14 ☔ View full report in Codecov by Sentry. |
hey @rudrakhp the code looks great ! please feel free to continue with unit tests and e2e |
b256946
to
9e01f0e
Compare
3bce57a
to
79bb214
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @rudrakhp we need to set the default order for the Lua filters in the filter chain, similart to what have been done to the Wasm and other filters.
case isFilterType(filter, egv1a1.EnvoyFilterWasm): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lua filter has built-in perRoute support, so we won't need to create a seperate lua filter for each route.
I think it's fine to keep the current implmentation as it's a generic approach to support configuration per route. If we find any issues with the multple-filter approach, we can then switch to the built-in perRoute approach.
Thanks for the comments @zhaohuabing |
Yes I started with Reason is that without a Lua filter, LuaPerRoute doesn't work. So if we want to define a |
I'm fine with putting Lua infront of ExtProc and Wasm. The order is adjustable if users want to change the default filter order in the filter chain. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please also inclue lua filter to the EnvoyFilter
API CEL rule and update the API doc for FilterOrder
? Thanks!
79bb214
to
7ef5c21
Compare
Signed-off-by: Rudrakh Panigrahi <[email protected]>
7ef5c21
to
330e1d8
Compare
@zhaohuabing done! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on it! Hope to have a user-guide docs for a following PR.
What type of PR is this?
feat: implement Lua feature in EnvoyExtensionPolicy
What this PR does / why we need it:
Implement API introduced in #4932
Which issue(s) this PR fixes:
Related #4627
Release Notes: No