-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
implement new native EventBridge EventRuleEngine #11960
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
Conversation
LocalStack Community integration with Pro 2 files ± 0 2 suites ±0 1h 49m 0s ⏱️ -18s Results for commit 29652a1. ± Comparison against base commit 2b405dc. This pull request removes 1 and adds 37 tests. Note that renamed tests count towards both.
♻️ This comment has been updated with latest results. |
f0fcef2
to
d6e926c
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.
Wow, that's amazing work @bentsku 👏 👏👏
Thank you very much also for extending the test suite and even fixing the exception cases 😮
LGTM, just added some small suggestions, but these are totally fine in a follow up. The main part where we need to be careful is not to re-enable the JPype engine here.
|
||
|
||
class EventRuleEngine: | ||
def evaluate_pattern_on_event(self, event_pattern: dict, message_body: str | dict): |
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.
I see. We now have the assumption that we validate the pattern for every matching.
That's not really necessary for ESM and Pipes, but better safe than sorry for now 👍
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.
How does it work in ESM and Pipes if the pattern is invalid? I didn't really find it.
The issue is that if the pattern is invalid, the whole matching logic will fail down the line, and it's part of the "compilation": it does not only validate, but compile the rules, unfolding the $or
and such. I'll try to document that better
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.
I've renamed the Validator
into Compiler
as it's closer to what it really does, it translates the rules into something the engine can properly interprets. I guess that for ESM/Pipes, those rules would need to be evaluated only once, and could be cached then, instead of calling matches_event
directly that way. I'm sure we could have some improvements, we should look at it in a follow up!
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.
Thank you @joe4dev for the thorough review! I've addressed all the comments, just the one about the string matching that I'm not sure how to address. Should we look at it quickly?
|
||
|
||
class EventRuleEngine: | ||
def evaluate_pattern_on_event(self, event_pattern: dict, message_body: str | dict): |
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.
How does it work in ESM and Pipes if the pattern is invalid? I didn't really find it.
The issue is that if the pattern is invalid, the whole matching logic will fail down the line, and it's part of the "compilation": it does not only validate, but compile the rules, unfolding the $or
and such. I'll try to document that better
Motivation
This PR copy-paste the SNS Filter Policy engine and modify it to be an
EventPatternEngine
. Implementations are close, but differs in quite a few ways. SNS has less features, maybe linked to complexity issues. They also allow way less complexity in rules.There a few differences with SNS in almost all methods, could maybe be consolidated by testing all the edge cases for SNS as well, but keeping the implementation different might just be a better idea, even if the base looks similar. Those are 2 different services and could evolve in different directions.
Question: should it be behind a feature flag?
Should probably be released after. It shouldn't be worse than the current implementation, but could probably have a few regression concerning validation (we might be stricter and wrong).4.0.3
edit: as the current Python implementation has a lot of flaws, merging this is most probably beneficial and should go in
4.0.3
, but I'll be on the lookout for any potential regression and jump on it next week if needed.Note:
In SNS, the filter policies are validated in a different step than the matching, which is why the current implementation is split into 2 classes. The validation (
EventPatternValidator
) could probably be consolidated into theEventRuleEngine
forevents
.Changes
events
, modify what was needed and add the missing implementation:wildcard
operatorcidr
operator (should be ported back to SNS)anything-but
withsuffix
,equals-ignore-case
andwildcard
(untested before)equals-ignore-case
,wildcard
,suffix
andprefix
list values only when nested underanything-but
equals-ignore-case
undersuffix
andprefix
events
Testing
All tests are passing, even the
_EXC
ones, I've added a few more as I had a few unanswered questions by doing the implementation.Also testing downstream dependencies with a full run at #\2193 (seems to be failing for unrelated changes and #11961), new full run at #\2208 ✅ for AMD64, ARM timeout but surely unrelated
TODO
What's left to do:
matches_event
so that it used by Pipes, but this could be done in a follow up