Bugfix/eventbridge/process to all matching rules #12090

maxhoheiser · 2025-01-02T11:54:56Z

Motivation

Internally target_services are stored in a dictionary _target_servuce_store and identified via the target_arn, this leads to problems since the target_arn is not unique for targets globally and across rules.
The target_id is unique across targets for a single rule, thus a globally unique id is a combination of rule_arn and target_id

This problem also solves the following github bug report: #11335

Changes

Changing the key for _target_servuce_store to rule_arn-target_id

Testing

Adding test for multiple rules with single or multiple patterns invoking the same target.
Adding tests for uniqueness of target_id across targets of single rule and across multiple rules
Adding tests for uniqueness of target_arn across targets of single rule and across multiple rules

github-actions · 2025-01-02T13:38:52Z

LocalStack Community integration with Pro

2 files ± 0 2 suites ±0 1h 7m 15s ⏱️ - 45m 52s
2 373 tests - 1 546 2 278 ✅ - 1 334 95 💤 - 212 0 ❌ ±0
2 375 runs - 1 546 2 278 ✅ - 1 334 97 💤 - 212 0 ❌ ±0

Results for commit a91d8fc. ± Comparison against base commit 3e1471a.

This pull request removes 1555 and adds 9 tests. Note that renamed tests count towards both.

tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_lambda_dynamodb
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_opensearch_crud
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_search_books
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_setup
tests.aws.scenario.kinesis_firehose.test_kinesis_firehose.TestKinesisFirehoseScenario ‑ test_kinesis_firehose_s3
tests.aws.scenario.lambda_destination.test_lambda_destination_scenario.TestLambdaDestinationScenario ‑ test_destination_sns
tests.aws.scenario.lambda_destination.test_lambda_destination_scenario.TestLambdaDestinationScenario ‑ test_infra
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_prefill_dynamodb_table
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_stepfunctions_input_recipient_list[step_function_input0-SUCCEEDED]
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_stepfunctions_input_recipient_list[step_function_input1-SUCCEEDED]
…

tests.aws.services.events.test_events.TestEventRule ‑ test_process_pattern_to_single_matching_rules_single_target
tests.aws.services.events.test_events.TestEventRule ‑ test_process_to_multiple_matching_rules_different_targets
tests.aws.services.events.test_events.TestEventRule ‑ test_process_to_multiple_matching_rules_single_target
tests.aws.services.events.test_events.TestEventRule ‑ test_process_to_single_matching_rules_single_target
tests.aws.services.events.test_events.TestEventTarget ‑ test_put_multiple_targets_with_same_arn_across_different_rules
tests.aws.services.events.test_events.TestEventTarget ‑ test_put_multiple_targets_with_same_arn_single_rule
tests.aws.services.events.test_events.TestEventTarget ‑ test_put_multiple_targets_with_same_id_across_different_rules
tests.aws.services.events.test_events.TestEventTarget ‑ test_put_multiple_targets_with_same_id_single_rule
tests.aws.services.lambda_.test_lambda_api.TestLambdaLayer ‑ test_layer_deterministic_version

♻️ This comment has been updated with latest results.

bentsku

LGTM! This does fix the issue, and adds quite a big (massive!) amount of tests! It should not break persistence either because this is done manually when restoring.

I have a nit concerning deleting the target senders when deleting rules and event buses, but this can be tackled in a follow up, as it's not exactly the scope of this PR and was already an issue before.

Thanks a lot for fixing this! 💯

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

tests/aws/services/events/test_events.py

bentsku · 2025-01-03T16:08:36Z

tests/aws/services/events/test_events.py

+        snapshot.add_transformers_list(
+            [
+                snapshot.transform.regex(target_id, "target-id"),
+                snapshot.transform.regex(queue_arn, "target-arn"),
+            ]
+        )


nit: I think it'd be better to use key_value transformers here in order to have the reference counting, but it also work that way, just less explicit

bentsku · 2025-01-03T16:14:07Z

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

@@ -1835,11 +1835,11 @@ def _delete_rule_services(self, rules: RuleDict | Rule) -> None:
    def _delete_target_sender(self, ids: TargetIdList, rule) -> None:


I can see this is only used when deleting targets. However, from everything I can see in this PR, Targets are living inside a Rule. Shouldn't we delete all TargetSender when deleting a rule? As we're caching the clients inside the sender, it could lead to quite some memory being taken and never released.

I think that we should delete all the TargetSender when deleting a rule. This is somewhat outside of the scope of this PR, but also somewhat in it, because you chose to go the "nested" route in the dictionary key, when in reality, it seems the structure is more rule_arn -> target_id -> TargetSender for the dict (self._target_sender_store[rule_arn][target_id]), so it should have another level of nesting, so that when you delete the rule, you can do del self._target_sender_store[rule_arn] instead of having to iterate over all target and create the "nested key" every time. Both works, you just need to manipulate the key every time.

So I like your approach for nesting targets under rules. Regarding deleting them - aws does not allow the deleting of a rule that still has targets - first all targets must be manually deleted - this takes care of the above-mentioned problem.

Co-authored-by: Ben Simon Hartung <42031100+bentsku@users.noreply.github.com>

maxhoheiser added aws:events Amazon EventBridge semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases labels Jan 2, 2025

maxhoheiser self-assigned this Jan 2, 2025

maxhoheiser added 5 commits January 2, 2025 13:51

feat: add test with multiple rules and multiple targets

513e4d6

feat: update default test event detail name

2dc4c3b

feat: add test multiple rules single target

44e84e0

feat: add test same target multiple rules different conditions

671acad

feat: add test multiple rules multiple patterns same target

718f429

maxhoheiser force-pushed the bugfix/eventbridge/process-to-all-matching-rules branch from c81a784 to 718f429 Compare January 2, 2025 12:53

fix: skip test for v1 provider

f364d37

maxhoheiser added 4 commits January 3, 2025 13:08

feat: add test target ids

8c430dc

feat: test targets with same arn

a960287

fix: add input transformers

3aad0af

feat: switch to unique_id for target factory store

cddfad5

maxhoheiser marked this pull request as ready for review January 3, 2025 13:15

maxhoheiser requested a review from joe4dev as a code owner January 3, 2025 13:15

maxhoheiser requested a review from bentsku January 3, 2025 13:15

maxhoheiser mentioned this pull request Jan 3, 2025

bug: Events.Rule is not filtering by EventPattern #11335

Closed

1 task

bentsku approved these changes Jan 3, 2025

View reviewed changes

maxhoheiser and others added 2 commits January 7, 2025 11:50

Update localstack-core/localstack/services/events/target.py

5098065

Co-authored-by: Ben Simon Hartung <42031100+bentsku@users.noreply.github.com>

Update tests/aws/services/events/test_events.py

a91d8fc

Co-authored-by: Ben Simon Hartung <42031100+bentsku@users.noreply.github.com>

maxhoheiser merged commit 7795211 into master Jan 7, 2025
31 checks passed

maxhoheiser deleted the bugfix/eventbridge/process-to-all-matching-rules branch January 7, 2025 12:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Bugfix/eventbridge/process to all matching rules #12090

Bugfix/eventbridge/process to all matching rules #12090

Uh oh!

maxhoheiser commented Jan 2, 2025 •

edited

Loading

Uh oh!

github-actions bot commented Jan 2, 2025 •

edited

Loading

Uh oh!

bentsku left a comment

Uh oh!

Uh oh!

Uh oh!

bentsku Jan 3, 2025

Uh oh!

bentsku Jan 3, 2025

Uh oh!

maxhoheiser Jan 7, 2025 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

		@@ -1835,11 +1835,11 @@ def _delete_rule_services(self, rules: RuleDict \| Rule) -> None:
		def _delete_target_sender(self, ids: TargetIdList, rule) -> None:

Uh oh!

Bugfix/eventbridge/process to all matching rules #12090

Bugfix/eventbridge/process to all matching rules #12090

Uh oh!

Conversation

maxhoheiser commented Jan 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Changes

Testing

Uh oh!

github-actions bot commented Jan 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

LocalStack Community integration with Pro

Uh oh!

bentsku left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bentsku Jan 3, 2025

Choose a reason for hiding this comment

Uh oh!

bentsku Jan 3, 2025

Choose a reason for hiding this comment

Uh oh!

maxhoheiser Jan 7, 2025 • 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!

Uh oh!

Uh oh!

maxhoheiser commented Jan 2, 2025 •

edited

Loading

github-actions bot commented Jan 2, 2025 •

edited

Loading

maxhoheiser Jan 7, 2025 •

edited

Loading