Feature: Eventbridge v2: CRUD #10613

maxhoheiser · 2024-04-08T12:52:07Z

Motivation

This PR introduces the core structure of the new localstack native implementation of event bridge.
Furthermore, it implements the CRUD functionality based on snapshots of verified AWS tests. A clean pattern for keeping data stored in the AccountRegionBundle store separate from related functionality is required. Separation of different sender functionality based on the target should be handled more efficiently than in a if else chain.
These API operations are implemented:

create_event_bus
delete_event_bus
describe_event_bus
list_event_buses
enable_rule
delete_rule
describe_rule
disable_rule
list_rules
list_rule_names_by_target
put_rule
list_targets_by_rule
put_targets
remove_targets
put_events
put_partner_events

Changes

Implement new provider, and crud API operations. API methods are grouped by resource type and sorted in hierarchical order, each group is sorted alphabetically.
Implement new dataclass definitions for EventBus and Rule
Implement service class composing the dataclass for EventBridgeService, RuleService and TargetService, encapsulating related functionality
A TargetService is instantiated via a base class and a factory containing specific logic in respect to the defined target service
Added multiple validated AWS tests for basic API functionality validation

Limitations

No pattern matching is implemented yet, thus all events are matched to all rules and sed to all targets.

github-actions · 2024-04-08T13:48:30Z

LocalStack Community integration with Pro

2 files ± 0 2 suites ±0 1h 31m 1s ⏱️ +26s
2 914 tests +19 2 624 ✅ +13 290 💤 +6 0 ❌ ±0
2 916 runs +19 2 624 ✅ +13 292 💤 +6 0 ❌ ±0

Results for commit acc72c8. ± Comparison against base commit c860285.

This pull request removes 6 and adds 25 tests. Note that renamed tests count towards both.

tests.aws.services.events.test_events.TestEventsEventBus ‑ test_create_custom_event_bus
tests.aws.services.events.test_events.TestEventsEventBus ‑ test_put_events_into_event_bus[domain]
tests.aws.services.events.test_events.TestEventsEventBus ‑ test_put_events_into_event_bus[path]
tests.aws.services.events.test_events.TestEventsEventBus ‑ test_put_events_into_event_bus[standard]
tests.aws.services.events.test_events.TestEventsEventBus ‑ test_put_events_nonexistent_event_bus
tests.aws.services.events.test_events.TestEventsEventBus ‑ test_put_events_to_default_eventbus_for_custom_eventbus

tests.aws.services.events.test_events.TestEventBus ‑ test_create_list_describe_delete_custom_event_buses[regions0]
tests.aws.services.events.test_events.TestEventBus ‑ test_create_list_describe_delete_custom_event_buses[regions1]
tests.aws.services.events.test_events.TestEventBus ‑ test_create_multiple_event_buses_same_name
tests.aws.services.events.test_events.TestEventBus ‑ test_delete_default_event_bus
tests.aws.services.events.test_events.TestEventBus ‑ test_describe_delete_not_existing_event_bus
tests.aws.services.events.test_events.TestEventBus ‑ test_list_event_buses_with_limit
tests.aws.services.events.test_events.TestEventBus ‑ test_list_event_buses_with_prefix
tests.aws.services.events.test_events.TestEventBus ‑ test_put_events_into_event_bus[domain]
tests.aws.services.events.test_events.TestEventBus ‑ test_put_events_into_event_bus[path]
tests.aws.services.events.test_events.TestEventBus ‑ test_put_events_into_event_bus[standard]
…

♻️ This comment has been updated with latest results.

joe4dev

nice foundation 👏 👏👏
Interesting new composite pattern to handle persistence while using high-level business objects. Looking forward to learning more about this experience 📈

I guess one of the main challenges is keeping the dataclasses and business objects perfectly synced (don't create, delete, or update one in some different way).

localstack/services/events/models_v2.py

localstack/services/events/rule.py

localstack/services/events/provider_v2.py

localstack/services/events/rule.py

tests/aws/services/events/test_events.py

localstack/services/events/rule.py

localstack/services/events/provider_v2.py

…d delete

tests/aws/services/events/test_events.py

joe4dev · 2024-04-19T09:09:49Z

tests/aws/services/events/test_events.py

+        reason="V1 provider does not support this feature",
+    )
+    @pytest.mark.parametrize("regions", [["us-east-1"], ["us-east-1", "us-west-1", "eu-central-1"]])
+    def test_create_list_describe_delete_custom_event_buses(


These tests seem flaky and we should fix them: https://app.circleci.com/pipelines/github/localstack/localstack/24319/workflows/83b9939c-a5e2-4b19-becd-2c40030242d4/jobs/201263/tests

That's related to the previous concurrency comment, because snapshotting lists can be flaky when other tests don't clean up properly (as you guessed Max).

We can xFail them to move on but should fix them ultimately.

v2 tests all clean up correctly so far, and this is a core test for the v2 functionality this is why I would strongly prefer to keep it in for the v2 provider, non the less it is already a part of the refactor tests backlog.

as discussed, we cannot reasonably assume that all current and future tests perform perfect cleanup, and therefore we should take pre-cautions such that these tests don't flake.

dfangl

I am not really happy with the whole EventBusService and RuleService yet, but I get the workaround around the persistence restrictions while still getting a somewhat close cohesion between data and logic. I think we can continue to talk and reiterate over it after this is merged as well.

I have some additional concerns about the service principal set to the target service, and some additional comments. Sorry the review took so long, it is quite a long PR and a lot of related reading.
I think for further work on the provider, it might be nice getting smaller PRs, instead of these big bangs :)

localstack/services/events/provider_v2.py

localstack/services/events/rule.py

dfangl · 2024-04-19T11:57:27Z

localstack/services/events/provider_v2.py

+        self._event_bus_services_store: EventBusServiceDict = {}
+        self._rule_services_store: RuleServiceDict = {}


No state lives in this "Service" objects, why do we store them? For persistence, we would need to regenerate them on restore, right?

Yes, this is mostly preparation for the future implementation dealing with persistence. My goal is to add an additional abstraction on top of the AccountRegionBundle, that combines state that is persistent and logic into an object, so that the provider only sees this object.

As discussed, the question was less related to the necessity of the ...Service objects themselves, but whether we need to store them if there is no state in them. We can keep it for now, and see in a future iteration how they will be handled.

dfangl · 2024-04-19T12:08:43Z

localstack/services/events/target.py

+class KinesisTargetService(TargetService):
+    def send_event(self, event):
+        kinesis_client = self.client.kinesis.request_metadata(
+            service_principal=self.service, source_arn=self.rule_arn


The service principal should always be events, it is the service principal making the request, not the target.

change to initializing the service client directly to avoid requesting metadata on each send in: eb3536a

added input validation for targets requiring role arn in: 8cbffff

dfangl · 2024-04-19T12:09:19Z

localstack/services/events/target.py

+            role_arn=role_arn, service_principal=ServicePrincipal.events, region_name=region_name
+        )
+    except ValueError as e:
+        LOG.debug(f"Could not connect with assumed role {role_arn}. Error: {e}")


In general, services get either an role passed, or use their service principal for these kind of requests. Some also support both, if you do not supply a role, it will use the service principal.
I am not sure what events supports, and it might differ by target, but we should definitely check that and only allow the right method.

dfangl · 2024-04-19T12:10:59Z

tests/aws/services/events/test_events.py

+        response = aws_client.events.list_rules(
+            Limit=int(count / 2) + 2, NextToken=response["NextToken"]
+        )
+        snapshot.match("list-rules-limit-next-token", response)


I guess there are ways around it. I think the method supports filtering by nameprefix or event bus? If we make those unique, we can avoid concurrent / leftover requests making the tests flake, but still snapshot it.

dfangl · 2024-04-19T12:12:00Z

localstack/services/events/event_bus.py

+class EventBusService:
+    def __init__(
+        self,
+        name: EventBusName,
+        region: str,
+        account_id: str,
+        event_source_name: Optional[str] = None,
+        tags: Optional[TagList] = None,
+        policy: Optional[str] = None,
+        rules: Optional[RuleDict] = None,
+    ):
+        self.event_bus = EventBus(
+            name,
+            region,
+            account_id,
+            event_source_name,
+            tags,
+            policy,
+            rules,
+        )
+
+    @property
+    def arn(self):
+        return self.event_bus.arn
+
+
+EventBusServiceDict = dict[Arn, EventBusService]


What value does this class add? Is this a preparation for some future part of the provider, or just to match the RuleService?

Yes, this is mostly preparation for the future implementation dealing with persistence. My goal is to add an additional abstraction on top of the AccountRegionBundle, that combines state that is persistent and logic into an object, so that the provider only sees this object.

The question was more related to this specific class, since it does not feature any additional logic other than holding the event bus state object. As discussed in slack, it is part a remnant of early implementation, and will be used when more logic regarding event busses arrives. Fine with me to keep it for now.

dfangl · 2024-04-19T12:16:05Z

localstack/services/events/target.py

+    return client
+
+
+class TargetService(ABC):


Not really happy with the naming here. Could we perhaps get rid of the Service? I am aware of the overlap with Target from the API spec then, but we could maybe call it RuleTarget or EventTarget or even TargetPublisher TargetSender or something the likes, but I am not a fan of TargetService, personally.

Since this is more a naming debate, please do not let this hold back the merge.

+1 for TargetSender in line with EventBridge Pipes
It implements a send method, so sender seems appropriate

for targets it indeed makes much more sense to call it sender, renamed in: 344f54c

dfangl

This looks good for a first iteration for me now! Please also let Joel take a final look, but no more blockers from my side!

dfangl · 2024-04-22T13:09:21Z

localstack/services/events/event_bus.py

+class EventBusService:
+    def __init__(
+        self,
+        name: EventBusName,
+        region: str,
+        account_id: str,
+        event_source_name: Optional[str] = None,
+        tags: Optional[TagList] = None,
+        policy: Optional[str] = None,
+        rules: Optional[RuleDict] = None,
+    ):
+        self.event_bus = EventBus(
+            name,
+            region,
+            account_id,
+            event_source_name,
+            tags,
+            policy,
+            rules,
+        )
+
+    @property
+    def arn(self):
+        return self.event_bus.arn
+
+
+EventBusServiceDict = dict[Arn, EventBusService]


The question was more related to this specific class, since it does not feature any additional logic other than holding the event bus state object. As discussed in slack, it is part a remnant of early implementation, and will be used when more logic regarding event busses arrives. Fine with me to keep it for now.

dfangl · 2024-04-22T13:15:23Z

localstack/services/events/provider_v2.py

+        self._event_bus_services_store: EventBusServiceDict = {}
+        self._rule_services_store: RuleServiceDict = {}


As discussed, the question was less related to the necessity of the ...Service objects themselves, but whether we need to store them if there is no state in them. We can keep it for now, and see in a future iteration how they will be handled.

dfangl · 2024-04-22T13:20:13Z

localstack/services/events/target.py

+    def _initialize_client(self) -> BaseClient:
+        """Initializes internal botocore client.
+        If a role from a target is provided, the client will be initialized with the assumed role.
+        If no role is provided the client will be initialized with the account ID and region.
+        In both cases event bridge is requested as service principal"""
+        service_principal = ServicePrincipal.events
+        if role_arn := self.target.get("role_arn"):
+            # assumed role sessions expires after 6 hours in AWS, currently no expiration in LocalStack
+            client_factory = connect_to.with_assumed_role(
+                role_arn=role_arn, service_principal=service_principal, region_name=self.region
+            )
+        else:
+            client_factory = connect_to(aws_access_key_id=self.account_id, region_name=self.region)
+        client = client_factory.get_client(self.service)
+        client = client.request_metadata(
+            service_principal=service_principal, source_arn=self.rule_arn
+        )
+        return client


This looks better, thanks! As discussed, it is necessary to set the events service principal to make sure all requests are recognized as coming from events :)

joe4dev

LGTM for the first iteration 👍 Please double-check whether test_create_list_describe_delete_custom_event_buses is multi-region compatible (see comment) before merging!
Thanks for addressing our comments and tracking pending comments in backlog items 🙏 .

Let's iterate quickly toward v1 parity in smaller more manageable steps 📈 🚀🚀🚀

joe4dev · 2024-04-22T14:17:40Z

localstack/services/events/target.py


+    def _validate_input(self, target: Target):
+        super()._validate_input(target)
+        if not collections.get_safe(target, "$.RoleArn"):


nit: can't we use a simpler get here?

joe4dev · 2024-04-22T14:20:06Z

localstack/services/events/target.py

            ]
        )

+    def _validate_input(self, target: Target):


This RoleArn validation looks redundant. Consider moving it to the TargetService base class in a follow-up.

it is only required for specific services, this is why it is not in the base class.

joe4dev · 2024-04-22T14:27:56Z

localstack/services/events/target.py

+    def _initialize_client(self) -> BaseClient:
+        """Initializes internal botocore client.
+        If a role from a target is provided, the client will be initialized with the assumed role.
+        If no role is provided the client will be initialized with the account ID and region.


In which cases does this apply? Isn't a role ARN mandatory for EventBridge?

role arn is only required for EC2 instances, Kinesis Data Streams, Step Functions state machines and API Gateway APIs

joe4dev · 2024-04-22T14:30:23Z

tests/aws/services/events/test_events.py

+        reason="V1 provider does not support this feature",
+    )
+    @pytest.mark.parametrize("regions", [["us-east-1"], ["us-east-1", "us-west-1", "eu-central-1"]])
+    def test_create_list_describe_delete_custom_event_buses(


as discussed, we cannot reasonably assume that all current and future tests perform perfect cleanup, and therefore we should take pre-cautions such that these tests don't flake.

joe4dev · 2024-04-22T14:32:54Z

tests/aws/services/events/test_events.py

+        not is_v2_provider() and not is_aws_cloud(),
+        reason="V1 provider does not support this feature",
+    )
+    @pytest.mark.parametrize("regions", [["us-east-1"], ["us-east-1", "us-west-1", "eu-central-1"]])


@maxhoheiser ⚠️ Is this test compatible with the multi-region tests we run on master?
https://docs.localstack.cloud/contributing/multi-account-region-testing/

all tests use prefixes or custom event buses where list operations are involved to limit it to the current test scope.

I meant region randomization here, which surfaced some issues. Glad we caught them 👍

Your answer here seems related to another comment about list snapshots.

joe4dev · 2024-04-22T14:37:26Z

tests/aws/services/events/test_events.py

+        response = events.list_event_buses(NamePrefix=bus_name)
+        snapshot.match("list-event-buses-prefix-complete-name", response)
+
+        response = events.list_event_buses(NamePrefix=bus_name.split("-")[0])


nit: I'd prefer hardcoded clarity here

joe4dev · 2024-04-22T14:38:53Z

tests/aws/services/events/test_events.py

+        snapshot.match("list-event-buses-limit-next-token", response)

    @markers.aws.unknown
+    @pytest.mark.skipif(is_aws_cloud(), reason="not validated")


super nit: there's a needs_fixing aws marker 💡

tests/aws/services/events/test_events.py

joe4dev · 2024-04-22T14:44:08Z

tests/aws/services/events/test_events.py

+
+    @markers.aws.validated
+    def test_delete_rule_with_targets(
+        self, put_rule, sqs_create_queue, sqs_get_queue_arn, aws_client, snapshot


good to know: the put_rule factory cleanup fails if the test case deletes the rule and pollutes the CI logs. More of a comment because we're doing quite badly everywhere with this.

joe4dev · 2024-04-22T14:51:14Z

tests/aws/services/events/test_events.py

+        snapshot.add_transformer(snapshot.transform.regex(bus_name, "<bus-name>"))
+
+        for region in regions:
+            events = aws_client_factory(region_name=region).events


I guess this overwrites the randomized region and should work, but it's probably better to double-check this assumption. Just be aware that any other fixture-based client could use a randomized region. That's probably worth mentioning in a comment.

I manually run the CircleCi pipeline with the flag randomize-aws-credentials set to True, and it indeed failed to correctly template replace the region in the snapshot.

fixed template replacement of region in snapshots for setting a region different than the default region in: 82420ae

maxhoheiser self-assigned this Apr 8, 2024

maxhoheiser added type: feature New feature, or improvement to an existing feature aws:events Amazon EventBridge semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases labels Apr 8, 2024

maxhoheiser requested a review from joe4dev April 8, 2024 12:52

joe4dev reviewed Apr 8, 2024

View reviewed changes

maxhoheiser force-pushed the feature/eventbridge_v2-curd-alpha branch 8 times, most recently from acee7ad to 6820b69 Compare April 11, 2024 14:45

maxhoheiser added 15 commits April 15, 2024 21:34

feat: add create event bus

cd97846

feat: extend create custom event bus test to include describe list an…

4fce985

…d delete

feat: add test list event buses with prefix

4661231

feat: add test list event buses with limit

2d6b246

feat: move new classes into separate files

dd21a3d

fix: align resource not found message for event bus

28d6b66

feat: add list event bus

0c47efd

feat: add describe event bus

bd14f48

feat: add limit and next token to list_event_bus

291eda0

feat: add test list delete non existing event bus

d3306fa

feat: add multi region for test create list describe delete event bus

62d9ca4

fix: use unique name for non existing event bus

c91077e

feat: add test create multiple event buses with same name in same region

1be1094

feat: add model

447ac42

fix: event bus crud

34bed73

maxhoheiser added 2 commits April 17, 2024 19:33

refactor: align name of event bus test class

dc318af

feat: add uuids to each name for event bus tests for concurrency safety

c671b7f

maxhoheiser marked this pull request as ready for review April 17, 2024 17:59

maxhoheiser requested review from dfangl and dominikschubert as code owners April 17, 2024 17:59

maxhoheiser requested a review from joe4dev April 17, 2024 17:59

maxhoheiser added 2 commits April 18, 2024 17:49

refactor: remove duplicated character

3574914

refactor: fix apostrophe placement

1808090

maxhoheiser force-pushed the feature/eventbridge_v2-curd-alpha branch from 32ff150 to 1808090 Compare April 18, 2024 15:49

fix: skip test for v1 provider

9550f74

joe4dev reviewed Apr 19, 2024

View reviewed changes

tests/aws/services/events/test_events.py Show resolved Hide resolved

joe4dev reviewed Apr 19, 2024

View reviewed changes

dfangl requested changes Apr 19, 2024

View reviewed changes

maxhoheiser added 3 commits April 19, 2024 15:29

feat: access target with default value None

e277278

feat: use del instead of pop

a86b6f3

feat: add policy key on demand

c9d6ca3

maxhoheiser force-pushed the feature/eventbridge_v2-curd-alpha branch from 4e2400c to c9d6ca3 Compare April 19, 2024 13:48

maxhoheiser added 2 commits April 19, 2024 15:49

refactor: use system function to extract service

f872ce7

feat: instantiate base client instead of factory

eb3536a

maxhoheiser requested a review from dfangl April 22, 2024 08:36

dfangl approved these changes Apr 22, 2024

View reviewed changes

joe4dev approved these changes Apr 22, 2024

View reviewed changes

maxhoheiser added 3 commits April 22, 2024 23:10

feat: add validation requiring input role

eb205b7

feat: rename TargetService to TargetSender

98e61a7

feat: test limit listing to context of test

24ed318

maxhoheiser force-pushed the feature/eventbridge_v2-curd-alpha branch from a5ce64c to 24ed318 Compare April 22, 2024 21:10

fix: template randomized during region tests

acc72c8

maxhoheiser force-pushed the feature/eventbridge_v2-curd-alpha branch from 82420ae to acc72c8 Compare April 23, 2024 06:12

maxhoheiser merged commit ba58014 into master Apr 23, 2024

maxhoheiser deleted the feature/eventbridge_v2-curd-alpha branch April 23, 2024 08:37

		self._event_bus_services_store: EventBusServiceDict = {}
		self._rule_services_store: RuleServiceDict = {}

Uh oh!

Feature: Eventbridge v2: CRUD #10613

Feature: Eventbridge v2: CRUD #10613

Uh oh!

Conversation

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

Uh oh!

Motivation

Changes

Limitations

Uh oh!

github-actions bot commented Apr 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

LocalStack Community integration with Pro

Uh oh!

joe4dev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

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!

dfangl left a comment

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!

joe4dev left a comment

Choose a reason for hiding this comment

Uh oh!

maxhoheiser commented Apr 8, 2024 •

edited

Loading

github-actions bot commented Apr 8, 2024 •

edited

Loading

maxhoheiser Apr 22, 2024 •

edited

Loading