Add event matching test suite #10599

joe4dev · 2024-04-03T19:51:54Z

Motivation

Event filtering is insufficiently tested and implemented in too many places in LocalStack 😢
Before we can unify all existing implementations, we need a robust AWS-validated test suite.

Existing implementations in LocalStack:

[localstack] test_event_pattern in the EventBridge provider implements a simplistic check whether a key is present in ~1 LOC.
[localstack] filter_event (inner method!) in the EventBridge provider implements the Amazon EventBridge event patterns in ~200 LOC. The implementation is confusing (e.g., operators are implemented multiple times), hard to maintain, and has several refactoring TODOs.
[moto-ext] _does_event_match within the EventPattern models of moto-ext implements the same Amazon EventBridge event patterns in ~50 LOC. The implementation is short and concise (e.g., mapping operators to functions < => lt) but incomplete. LocalStack patches this implementation in the EventBridge provider. Hence, it is not used within LocalStack.
[localstack-ext] filter_event in the Pipes utils is a copy/pasted implementation from EventBridge. It was temporarily copied because the EventBridge implementation was not re-usable but will be unified within this PR. It implements Amazon EventBridge Pipes filtering, which appears to be the same implementation as for EventBridge.
[localstack] filter_stream_record in the Lambda Event Source Mapping (ESM) utils is ~110 LOC. It implements the Lambda event filtering, which appears to be the same implementation as for Pipes and EventBridge. We know from other sources that ESM and Pipes use the same infrastructure at AWS.
[localstack] _evaluate_nested_filter_policy_on_dict within the "SubscriptionFilter" of SNS is ~130 LOC. It implements Amazon SNS subscription filter policies, which appear very similar to the EventBridge implementation. However, this implementation is not identical (e.g., does not support the wildcard operator).
[moto-ext] _compute_body_checks within the SNS utils in moto-ext implements Amazon SNS subscription filter policies is ~340 LOC. In addition to the matching functionality, moto-ext also implements the filter validation using a complexity calculation in _validate_filter_policy. These extra ~170 LOC also cover several exception cases. According to @bentsku, our moto-ext implementation also got backported to upstream moto.

AWS recently (2024-02-01) open-sourced https://github.com/aws/event-ruler. This rule engine implements the above filtering patterns. It is written in Java in ~6k LOC. Their blog post mentions that the internals are built on top of finite state machines, which can be updated dynamically with new rules.

Changes

Make test_event_pattern in the event provider use our EventBridge filtering implementation
Add a suite of ~70 aws-validated tests based on the documentation of the filtering patterns. The tests mainly based on the original Amazon EventBridge event patterns but also inspired from the open source event ruler and discovered edge cases. The test suite has a good initial operator coverage but misses further edge cases and exception cases.
Extract the implementation from the EventBridge provider into a separate utils file
Refactor (e.g., filter_event => matches_event) and fix several issues in the EventBridge filter_events method and its helpers (e.g., improved handling of numeric operators and improved list intersections handling)
Refactor (e.g., filter_stream_record => does_match_event) and fix several issues (e.g., operator casing, numeric conditions, raising exceptions)

Testing

Within localstack/services/events/provider.py, test_event_pattern can be used to invoke different implementation within LocalStack.

Discussion

Raising exceptions instead of silently ignoring the error case and returning True/False is a behavior change that can help us to make issues more visible. Background processes should handle such exceptions better (e.g., The SQS ESM implementation just drops the entire batch upon unhandled exceptions). At AWS, more exceptions are probably caught earlier because the rule engine can validate rules before actually doing a matching.

github-actions · 2024-04-04T09:33:29Z

LocalStack Community integration with Pro

2 files 2 suites 1h 31m 18s ⏱️
2 831 tests 2 547 ✅ 284 💤 0 ❌
2 833 runs 2 547 ✅ 286 💤 0 ❌

Results for commit bb6614a.

joe4dev · 2024-04-04T09:43:45Z

tests/aws/services/events/test_event_patterns.py

+#  and exception handling.
+@pytest.mark.skipif(is_v2_provider(), reason="V2 provider does not support this feature yet")
+@pytest.mark.parametrize(
+    "request_template,label", request_template_tuples, ids=[t[1] for t in request_template_tuples]


probably debatable whether it's a good idea to parameterize tests based on directory contents. It makes it very easy to add new test cases but add a small delay to test collection because it needs to list the files in the directory 🤷

I think this is fine for tests like this :)

joe4dev · 2024-04-04T09:45:49Z

tests/aws/services/events/test_events_rules.py

            "test2": [{"anything-but": "test2"}],
            "test3": [{"anything-but": ["test3", "test33"]}],
            "test4": [{"anything-but": {"prefix": "test4"}}],
-            "ip": [{"cidr": "10.102.1.0/24"}],


That IP check never worked but it got silently ignored 😬 (just double-checked by running the test suite against the old implementations before the refactorings and fixes)

dfangl

Good test and a nice refacotring, looking forward to this being unified all over the place :) I also prefer the exceptions over implicit failing/succeeding of the match.

dfangl · 2024-04-05T16:32:25Z

tests/aws/services/events/test_event_patterns.py

+#  and exception handling.
+@pytest.mark.skipif(is_v2_provider(), reason="V2 provider does not support this feature yet")
+@pytest.mark.parametrize(
+    "request_template,label", request_template_tuples, ids=[t[1] for t in request_template_tuples]


I think this is fine for tests like this :)

dfangl · 2024-04-05T16:34:31Z

tests/aws/services/events/test_event_patterns.py

+    event = request_template["Event"]
+    event_pattern = request_template["EventPattern"]
+
+    if label.endswith("_EXC"):


What was your reasoning here for encoding this in the file name, instead of having another field in the json5 file?

Good question. It kinda evolved because I wanted to have an explicit way to express the intent of the test. The surprise effect helps to write better test cases. It could as well be a field in the the json5 file though.

Not really a strong reason, some thoughts:

The intention of the test is easy to see in the file explorer.

It currently matches the TestEventPattern API. Maybe, it would be easier to copy/paste and test in the AWS console.

I initially built a small templating engine supporting variables but noticed we can simplify and don't care about any dynamic values 🤦 So the entire request "templating" part comes from there 😅

dfangl · 2024-04-05T16:36:11Z

tests/aws/services/events/test_events_rules.py

+    # EventBridge apparently converts some fields, for example: Source=>source, DetailType=>detail-type
+    # but the actual pattern matching is case-sensitive by key!


joe4dev · 2024-04-05T19:18:59Z

tests/aws/services/events/test_event_patterns.py

+        )
+
+        # Validate the test intention: The _NEG suffix indicates negative tests (i.e., a pattern not matching the event)
+        if label.endswith("_NEG"):


It might be useful in the future to snapshot the result together with a hash of the request because currently, it's possible to modify the test case after being AWS-validated; kinda proper testing police ;)

joe4dev added the semver: patch Non-breaking changes which can be included in patch releases label Apr 3, 2024

joe4dev self-assigned this Apr 3, 2024

joe4dev added 17 commits April 4, 2024 10:31

Add parametrized test for event patterns

0c91723

Externalize event samples

3a6dcee

Add test cases based on AWS docs

9c6e183

Refactor event filtering into eventbridge utils

aa39fb7

Separate special multi-key test and improve test setup

c01f81c

Add negative tests

4df8aca

Remove templating functionality

f558f2a

Remove unnecessary templating variables

dabecbf

Add case sensitive key test

03debf4

Fix null handling for values and arrays

d677aa0

Refine test cases and add alternative implementations

c9fc3cc

Refactor and fix some event bridge implementation issues

d0a3173

Refactor ESM event filtering and fix some issues

1a2ccc1

Rename event matching method

b7d7ae3

Skip new tests for the eventbridge v2 native provider

b6a536a

Add nested JSON test case

0c1adcc

Skip unsupported IP matching and JSON nesting tests

bb6614a

joe4dev force-pushed the add-event-filtering-test-suite branch from 22b67cf to bb6614a Compare April 4, 2024 08:33

joe4dev marked this pull request as ready for review April 4, 2024 09:34

joe4dev requested review from dfangl and dominikschubert as code owners April 4, 2024 09:34

joe4dev commented Apr 4, 2024

View reviewed changes

joe4dev requested a review from bentsku April 4, 2024 16:38

dfangl approved these changes Apr 5, 2024

View reviewed changes

joe4dev commented Apr 5, 2024

View reviewed changes

joe4dev merged commit 389d942 into master Apr 8, 2024

joe4dev deleted the add-event-filtering-test-suite branch April 8, 2024 07:37

This was referenced Apr 8, 2024

eventbridge: fix handling of list elements #10600

Merged

Add experimental event ruler #10615

Merged

This was referenced Apr 17, 2024

fix SNS filtering when payload contains arrays #10684

Merged

bug: Lambda Event Source Mapping Filter Criteria not respecting comparison operator exists: false #10325

Closed

joe4dev mentioned this pull request Oct 23, 2024

Remove legacy Lambda ESM v1 feature #11733

Merged

9 tasks

This was referenced Nov 7, 2024

Switch default event rule engine from python to java #11807

Closed

Added EVENT_RULE_ENGINE flag based opt-in #11816

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add event matching test suite #10599

Add event matching test suite #10599

Uh oh!

joe4dev commented Apr 3, 2024 •

edited

Loading

Uh oh!

github-actions bot commented Apr 4, 2024

Uh oh!

joe4dev Apr 4, 2024

Uh oh!

dfangl Apr 5, 2024

Uh oh!

joe4dev Apr 4, 2024

Uh oh!

dfangl left a comment •

edited

Loading

Uh oh!

dfangl Apr 5, 2024

Uh oh!

dfangl Apr 5, 2024

Uh oh!

joe4dev Apr 5, 2024

Uh oh!

joe4dev Apr 5, 2024

Uh oh!

dfangl Apr 5, 2024

Uh oh!

joe4dev Apr 5, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		# EventBridge apparently converts some fields, for example: Source=>source, DetailType=>detail-type
		# but the actual pattern matching is case-sensitive by key!

Uh oh!

Add event matching test suite #10599

Add event matching test suite #10599

Uh oh!

Conversation

joe4dev commented Apr 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Changes

Testing

Discussion

Uh oh!

github-actions bot commented Apr 4, 2024

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!

dfangl left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

joe4dev commented Apr 3, 2024 •

edited

Loading

dfangl left a comment •

edited

Loading