implement new native EventBridge EventRuleEngine #11960

bentsku · 2024-11-28T23:23:36Z

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

github-actions · 2024-11-29T01:55:27Z

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.

joe4dev

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.

joe4dev · 2024-11-29T13:46:50Z

localstack-core/localstack/services/events/event_rule_engine.py

+
+
+class EventRuleEngine:
+    def evaluate_pattern_on_event(self, event_pattern: dict, message_body: str | dict):


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 👍

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

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!

localstack-core/localstack/services/events/event_rule_engine.py

tests/aws/services/events/test_events_patterns.py

tests/unit/utils/test_event_matcher.py

…e than validate the rule

bentsku

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?

tests/unit/utils/test_event_matcher.py

localstack-core/localstack/services/events/event_rule_engine.py

bentsku · 2024-11-29T14:21:04Z

localstack-core/localstack/services/events/event_rule_engine.py

+
+
+class EventRuleEngine:
+    def evaluate_pattern_on_event(self, event_pattern: dict, message_body: str | dict):


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

localstack-core/localstack/services/events/event_rule_engine.py

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 added this to the 4.1 milestone Nov 28, 2024

bentsku self-assigned this Nov 28, 2024

bentsku added 9 commits November 29, 2024 11:05

use SNS as base for EventBridge EventRuleEngine

83867f3

update comments

c1d06c3

add validation for unclear implementation

b469703

use new EventRuleEngine in matches_event

cd1fb76

fix anything-but with null value

cb597a9

update error handling

99ecbb5

fix unit test letting exception bubble up to provider

9d52489

fix unit test to also work with java if ever we fix it

f953795

add more cases around anything-but and type matching for str checks

d6e926c

bentsku force-pushed the eventbridge-sns-engine branch from f0fcef2 to d6e926c Compare November 29, 2024 10:05

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 modified the milestones: 4.1, 4.0.3 Nov 29, 2024

bentsku marked this pull request as ready for review November 29, 2024 10:39

bentsku requested review from maxhoheiser and joe4dev as code owners November 29, 2024 10:39

bentsku changed the title ~~use SNS as base for EventBridge EventRuleEngine~~ implement new python native EventBridge EventRuleEngine Nov 29, 2024

bentsku changed the title ~~implement new python native EventBridge EventRuleEngine~~ implement new native EventBridge EventRuleEngine Nov 29, 2024

joe4dev approved these changes Nov 29, 2024

View reviewed changes

bentsku added 3 commits November 29, 2024 15:16

switch order of enabling unit test jpype

f763ca4

add link to docs

99fdcff

rename EventPatternValidator into EventPatternCompiler as it does mor…

29652a1

…e than validate the rule

bentsku commented Nov 29, 2024

View reviewed changes

joe4dev mentioned this pull request Nov 29, 2024

Add deprecation for Java-based event ruler #11964

Merged

joe4dev merged commit d5b43de into master Nov 29, 2024
32 checks passed

joe4dev deleted the eventbridge-sns-engine branch November 29, 2024 15:51

bentsku mentioned this pull request Dec 3, 2024

SNS: improve SNS Filter Policy engine and implement cidr operator #11979

Merged

This was referenced Dec 4, 2024

Remove legacy event filtering implementation #11985

Merged

Deprecate event rule engine config localstack/docs#1587

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

implement new native EventBridge EventRuleEngine #11960

implement new native EventBridge EventRuleEngine #11960

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!



		class EventRuleEngine:
		def evaluate_pattern_on_event(self, event_pattern: dict, message_body: str \| dict):

Uh oh!

implement new native EventBridge EventRuleEngine #11960

implement new native EventBridge EventRuleEngine #11960

Uh oh!

Conversation

Uh oh!

Motivation

Changes

Testing

TODO

Uh oh!

Uh oh!

LocalStack Community integration with Pro

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!