8000 implement new native EventBridge EventRuleEngine by bentsku · Pull Request #11960 · localstack/localstack · GitHub
[go: up one dir, main page]

Skip to content

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

Merged
merged 12 commits into from
Nov 29, 2024
Merged

Conversation

bentsku
Copy link
Contributor
@bentsku bentsku commented Nov 28, 2024

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 4.0.3. It shouldn't be worse than the current implementation, but could probably have a few regression concerning validation (we might be stricter and wrong).
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 the EventRuleEngine for events.

Changes

  • copy-paste the SNS filter policy engine to events, modify what was needed and add the missing implementation:
    • wildcard operator
    • cidr operator (should be ported back to SNS)
    • nested anything-but with suffix, equals-ignore-case and wildcard (untested before)
      • equals-ignore-case, wildcard, suffix and prefix list values only when nested under anything-but
    • nested equals-ignore-case under suffix and prefix
  • modify the validation engine to be less strict than SNS, might still be too strict, we need to implement more tests in events
  • add over 30 test cases for missing validation, new implementation has 114/114 🥳

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:

  • add the few tests marked with TODO in the implementation
  • maybe replace the logic in matches_event so that it used by Pipes, but this could be done in a follow up

@bentsku bentsku added aws:events Amazon EventBridge semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases labels Nov 28, 2024
@bentsku bentsku added this to the 4.1 milestone Nov 28, 2024
@bentsku bentsku self-assigned this Nov 28, 2024
Copy link
github-actions bot commented Nov 29, 2024

LocalStack Community integration with Pro

    2 files  ± 0      2 suites  ±0   1h 49m 0s ⏱️ -18s
3 811 tests +36  3 494 ✅ +65  317 💤  - 29  0 ❌ ±0 
3 813 runs  +36  3 494 ✅ +65  319 💤  - 29  0 ❌ ±0 

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.
tests.aws.services.stepfunctions.v2.context_object.test_context_object.TestSnfBase ‑ test_items_path
tests.aws.services.events.test_events.TestEvents ‑ test_put_events_with_time_field
tests.aws.services.events.test_events_patterns.TestEventPattern ‑ test_event_pattern[content_anything_but_ignorecase_EXC]
tests.aws.services.events.test_events_patterns.TestEventPattern ‑ test_event_pattern[content_anything_but_ignorecase_list_EXC]
tests.aws.services.events.test_events_patterns.TestEventPattern ‑ test_event_pattern[content_anything_but_string_null]
tests.aws.services.events.test_events_patterns.TestEventPattern ‑ test_event_pattern[content_anything_prefix_ignorecase_EXC]
tests.aws.services.events.test_events_patterns.TestEventPattern ‑ test_event_pattern[content_anything_prefix_int_EXC]
tests.aws.services.events.test_events_patterns.TestEventPattern ‑ test_event_pattern[content_anything_prefix_list]
tests.aws.services.events.test_events_patterns.TestEventPattern ‑ test_event_pattern[content_anything_prefix_list_NEG]
tests.aws.services.events.test_events_patterns.TestEventPattern ‑ test_event_pattern[content_anything_prefix_list_type_EXC]
tests.aws.services.events.test_events_patterns.TestEventPattern ‑ test_event_pattern[content_anything_suffix_ignorecase_EXC]
…

♻️ This comment has been updated with latest results.

@bentsku bentsku force-pushed the eventbridge-sns-engine branch from f0fcef2 to d6e926c Compare November 29, 2024 10:05
@bentsku bentsku added semver: patch Non-breaking changes which can be included in patch releases and removed semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases labels Nov 29, 2024
@bentsku bentsku modified the milestones: 4.1, 4.0.3 Nov 29, 2024
@bentsku bentsku marked this pull request as ready for review November 29, 2024 10:39
@bentsku bentsku changed the title use SNS as base for EventBridge EventRuleEngine implement new python native EventBridge EventRuleEngine Nov 29, 2024
@bentsku bentsku changed the title implement new python native EventBridge EventRuleEngine implement new native EventBridge EventRuleEngine Nov 29, 2024
Copy link
Member
@joe4dev joe4dev left a 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):
Copy link
Member

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 👍

Copy link
Contributor Author

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

Copy link
Contributor Author
@bentsku bentsku Nov 29, 2024

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!

Copy link
Contributor Author
@bentsku bentsku left a 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):
Copy link
Contributor Author

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

@joe4dev joe4dev merged commit d5b43de into master Nov 29, 2024
32 checks passed
@joe4dev joe4dev deleted the eventbridge-sns-engine branch November 29, 2024 15:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
aws:events Amazon EventBridge semver: patch Non-breaking changes which can be included in patch releases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants
0