Enable EventBridge API destinations and connections #11896

dfangl · 2024-11-21T13:48:43Z

Motivation

In the current v2 provider, we do not support eventbridge API destinations.

As we do want this in 4.0.1, and I am unsure if we should do such a big code change, I staggered the commits in terms of escalating changes / complexity.

93822dd has the minimal changes necessary to make the feature work, we could cherry pick that into a separate PR.

After this commit, we increasingly increase parity, with more snapshots, creation of eventbridge secrets, bypass of secretmanager name assertions (since official EventBridge secrets contain a ! in the name).

It might be too much for a release tomorrow, but I will let the reviewers judge that.

Changes

Allow persistence for connections and api destinations by moving them into the store (very simply, without dataclasses as for other models)
Detect API destinations and forward events correctly
Connections now correctly create secrets in secretsmanager for connection secrets
Change snapshot transformers to add more assertions on the structure of ARNs
Some minor testing improvements, to increase reliability of the tests (against AWS)
Move the snapshots to an autouse fixture, to be able to use the snapshot name directly (honestly a preference, but I did quite some work in the tests)
Add more snapshot on the event secrets
Unskip test for api destinations
Fix partitions in some ARNs

…re (for now), quick copy of send_event_to_api_destination to get values from new store

… discovered

…secret name validation in secretsmanager, add more tests for secret contents

…ations

dfangl · 2024-11-21T16:39:23Z

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

+        # Some providers need to create keys which are not usually creatable by users
+        if not any(
+            tag_entry["Key"] == "BYPASS_SECRET_ID_VALIDATION"
+            for tag_entry in request.get("Tags", [])
+        ):
+            self._raise_if_invalid_secret_id(request["Name"])
+        else:
+            request["Tags"] = [
+                tag_entry
+                for tag_entry in request.get("Tags", [])
+                if tag_entry["Key"] != "BYPASS_SECRET_ID_VALIDATION"
+            ]


We sadly need something to bypass the name validation here, as eventbridge connection secrets contain a !, which is not allowed. I used a tag here, but there are other options should we decide against it.

which is not allowed

I'm not sure I'm following here. Not allowed as in our SecretsManager service should support it, but doesn't? Or how would this otherwise work on AWS?

Are AWS services able to create keys with names that we (as external users) can't?

It seems that way. The error message clearly states it should not be allowed, the aws console also does not allow it. So maybe aws services have some bypass for it.

github-actions · 2024-11-21T17:33:49Z

LocalStack Community integration with Pro

2 files ±0 2 suites ±0 1h 43m 49s ⏱️ + 2m 11s
3 568 tests +3 3 235 ✅ +6 333 💤 - 3 0 ❌ ±0
3 570 runs +3 3 235 ✅ +6 335 💤 - 3 0 ❌ ±0

Results for commit 02baf71. ± Comparison against base commit 6748e0e.

dominikschubert

Nice! 🏆 Think it's a good idea to move from the transient state in the provider to directly using the entities from the store. Much simpler and less error-prone.

The somewhat weird workaround with the secret names might be worth looking into later again. Maybe we can just detect it as an internal call via headers/request context and then allow arbitrary keys? We can discuss this at a later date though.

dominikschubert · 2024-11-21T18:50:54Z

tests/aws/services/events/test_events.snapshot.json

@@ -1754,7 +1754,7 @@
      ]
    }
  },
-    "tests/aws/services/events/test_events.py::TestEvents::test_create_connection_validations": {
+  "tests/aws/services/events/test_events.py::TestEvents::test_create_connection_validations": {


comment: interesting 🤨

Didn’t notice that, that is really interesting. Did someone manually mess with this file?

dominikschubert · 2024-11-21T18:51:23Z

tests/aws/services/events/test_events.snapshot.json

    "recorded-content": {
      "create-connection": {
-        "ConnectionArn": "connection-arn",
+        "ConnectionArn": "arn:<partition>:events:<region>:111111111111:connection/<connection-name>/<resource:1>",


❤️: nice attention to detail with the ARN decomposition in the snapshots 🙌

dominikschubert · 2024-11-21T18:52:08Z

tests/aws/services/events/test_events.snapshot.json

+        "LastAuthorizedTime": "datetime",
+        "LastModifiedTime": "datetime",
+        "Name": "<connection-name>",
+        "SecretArn": "arn:<partition>:secretsmanager:<region>:111111111111:secret:events!connection/<connection-name>/<secret-uuid>-<secret-id-suffix>",


comment: that ! is certainly an interesting choice on AWS's side

dominikschubert · 2024-11-21T19:02:59Z

tests/aws/services/events/conftest.py

+    yield _create_connection
+
+    try:
+        aws_client.events.delete_connection(Name=connection_name)
+    except Exception as e:
+        LOG.debug("Error cleaning up connection: %s", e)


Nice catch. Kinda reminding me of the fact that it might be nice to revive the client-based cleanups at some point 😁

dominikschubert · 2024-11-21T19:05:46Z

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

+        # Some providers need to create keys which are not usually creatable by users
+        if not any(
+            tag_entry["Key"] == "BYPASS_SECRET_ID_VALIDATION"
+            for tag_entry in request.get("Tags", [])
+        ):
+            self._raise_if_invalid_secret_id(request["Name"])
+        else:
+            request["Tags"] = [
+                tag_entry
+                for tag_entry in request.get("Tags", [])
+                if tag_entry["Key"] != "BYPASS_SECRET_ID_VALIDATION"
+            ]


which is not allowed

I'm not sure I'm following here. Not allowed as in our SecretsManager service should support it, but doesn't? Or how would this otherwise work on AWS?

dominikschubert · 2024-11-21T19:29:55Z

tests/aws/services/events/test_events.py

+    @pytest.mark.skipif(
+        is_old_provider(),
+        reason="V1 provider does not support this feature",
+    )


@dfangl going above and beyond 🚀

dominikschubert · 2024-11-21T19:31:24Z

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

+ConnectionDict = dict[ConnectionName, DescribeConnectionResponse]
+ApiDestinationDict = dict[ApiDestinationName, DescribeApiDestinationResponse]


Guess that's something for service owners to judge, but I don't see this blocking for this iteration.

This is really a temporary shortcut to get some typing, I would also think this needs another iteration at some point.

dominikschubert · 2024-11-21T19:33:08Z

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

@@ -390,6 +391,10 @@ class EventsTargetSender(TargetSender):
    def send_event(self, event):
        # TODO add validation and tests for eventbridge to eventbridge requires Detail, DetailType, and Source
        # https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/events/client/put_events.html
+        target_arn = self.target["Arn"]
+        if ":api-destination/" in target_arn or ":destination/" in target_arn:
+            send_event_to_api_destination(target_arn, event, self.target.get("HttpParameters"))


@maxhoheiser Might be something were instrumentation for EventStudio might be a good idea in a follow up

dominikschubert · 2024-11-21T19:43:10Z

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

+        # TODO use service role as described here: https://docs.aws.amazon.com/eventbridge/latest/userguide/using-service-linked-roles-service-action-1.html
+        # not too important as it is created automatically on AWS anyway, with the right permissions


even when using the API or only via the AWS Console?

Should also be the case with the api.

dominikschubert · 2024-11-21T19:45:12Z

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

@@ -339,33 +338,112 @@ def _create_connection_arn(
        self, context: RequestContext, name: str, connection_uuid: str
    ) -> str:
        """Create a standardized connection ARN."""
-        return f"arn:aws:events:{context.region}:{context.account_id}:connection/{name}/{connection_uuid}"
+        return f"arn:{get_partition(context.region)}:events:{context.region}:{context.account_id}:connection/{name}/{connection_uuid}"


nice catch here as well 👍

joe4dev

LGTM 🚀 Thanks a lot @dfangl

Move connections to store, store private connection parameters in sto…

93822dd

…re (for now), quick copy of send_event_to_api_destination to get values from new store

dfangl requested review from maxhoheiser and joe4dev as code owners November 21, 2024 13:48

dfangl added this to the 4.0.1 milestone Nov 21, 2024

dfangl added the semver: patch Non-breaking changes which can be included in patch releases label Nov 21, 2024

dfangl self-assigned this Nov 21, 2024

dfangl requested a review from dominikschubert November 21, 2024 13:48

dfangl marked this pull request as draft November 21, 2024 13:48

dfangl added 6 commits November 21, 2024 15:12

Change snapshot transformers to assert ARN structure, fix uuid issues…

70e44f5

… discovered

Add connection secret test, reformat snapshotting of test class a bit

21de0d5

Create secret lifecycle for connection secrets, add option to bypass …

6819620

…secret name validation in secretsmanager, add more tests for secret contents

Only store public info again, avoid direct store access in api destin…

e51a750

…ations

Migrate api destinations to a store, clean up models

c6cda30

Add skip for v1 provider for new test

227fec2

dfangl marked this pull request as ready for review November 21, 2024 16:33

dfangl requested review from macnev2013, MEPalma and steffyP as code owners November 21, 2024 16:33

dfangl added 2 commits November 21, 2024 17:33

Remove leftover line

3363cfb

Remove leftover TODO

02baf71

dfangl commented Nov 21, 2024

View reviewed changes

dominikschubert approved these changes Nov 21, 2024

View reviewed changes

joe4dev approved these changes Nov 22, 2024

View reviewed changes

dominikschubert merged commit 7c2d1e3 into master Nov 22, 2024
32 checks passed

dominikschubert deleted the events/connections branch November 22, 2024 08:54

		ConnectionDict = dict[ConnectionName, DescribeConnectionResponse]
		ApiDestinationDict = dict[ApiDestinationName, DescribeApiDestinationResponse]

		# TODO use service role as described here: https://docs.aws.amazon.com/eventbridge/latest/userguide/using-service-linked-roles-service-action-1.html
		# not too important as it is created automatically on AWS anyway, with the right permissions

Uh oh!

Enable EventBridge API destinations and connections #11896

Enable EventBridge API destinations and connections #11896

Uh oh!

Conversation

dfangl commented Nov 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Changes

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!

github-actions bot commented Nov 21, 2024

LocalStack Community integration with Pro

Uh oh!

dominikschubert 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!

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!

joe4dev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dfangl commented Nov 21, 2024 •

edited

Loading