materialize-eventbridge: new connector#4359
Conversation
| case p.IsPrimaryKey: | ||
| constraint.Type = pm.Response_Validated_Constraint_LOCATION_REQUIRED | ||
| constraint.Reason = "Document keys must be included" |
There was a problem hiding this comment.
from @mdibaiee:
I believe keys should also be forbidden since we are doing the full document and there is no keying of the events at all?
Looking at google-pubsub it seems we have it as Optional there, but that’s because the key can be used to order the messages published, but here it seems we don’t use the key at all
I did try to switch primary key to forbidden, but the integration tests fail because the flow runtime requires key fields not be forbidden, so the best we can do is optional.
I've checked the other pattern-4 materializers:
- slack requires and uses keys
- pinecone requires and uses keys
- webhook requires, but does not use keys, only
RawJSON - google-pubsub makes keys optional, and uses
.PackedKey
Notably, this comment tags the pinecone logic:
We require collection keys be materialized because it seems pretty reasonable to
require they be included as metadata since the composite key is used as the basis for
the vector ID, and also to avoid complications from
estuary/flow#1057.
|
|
||
| type resource struct { | ||
| Source string `json:"source" jsonschema:"title=Event Source,description=Source field set on every event published from this binding (e.g. \"my.app\")." jsonschema_extras:"x-collection-name=true,order=1"` | ||
| DetailType string `json:"detail_type" jsonschema:"title=Detail Type,description=DetailType field set on every event published from this binding (e.g. \"OrderPlaced\")." jsonschema_extras:"order=2"` |
There was a problem hiding this comment.
from @mdibaiee:
Seems like this is going to be a painful connector to configure for customers because they need to manually set
detail_typefor each binding, which could be a lot... Worth checking with product people what they think we should do here for a better experience: a default? some other form of inference?
I've added jsonschema_extras:"x-collection-name=true" to the DetailType field to signal default value == collection name in the UI.
There was a problem hiding this comment.
@jacobmarble let's ask a potential / first customer of this connector what they think about having the collection name as the DetailType
| if err := describeBus(ctx, d.client, d.cfg.EventBusName); err != nil { | ||
| prereqs.Err(err) | ||
| } | ||
| return prereqs |
There was a problem hiding this comment.
from @mdibaiee:
We need to also check we have write permission for the bus
How do you propose we perform this check? I can think of two ways.
-
Send a test event. This works, but creates clutter. Should we clean the clutter ourselves? Create a dead queue for the clutter? Ignore it?
-
Invoke
sts.NewFromConfig(cfg).GetCallerIdentity(...)andiam.NewFromConfig(cfg).SimulatePrincipalPolicy(...)to directly query for the needed permission. This is elegant, but (1) has no precedent in the repository, (2) there are edge cases like assumed-role ARNs returned byGetCallerIdentitymust be converted to the underlying role ARN (3) the role needs permission to callGetCallerIdentityandSimulatePrincipalPolicy.
There was a problem hiding this comment.
Sending a test event is off the table: our Validate step must be fully read-only
I think the second option is a good best-effort check, doesn't need to be exhaustive
…atePrincipalPolicy
mdibaiee
left a comment
mdibaiee
left a comment
There was a problem hiding this comment.
LGTM % question regarding the check
|
|
||
| callerOut, err := sts.NewFromConfig(awsCfg).GetCallerIdentity(ctx, &sts.GetCallerIdentityInput{}) | ||
| if err != nil { | ||
| log.WithField("error", err).Warn("GetCallerIdentity failed; skipping write-permission check") |
There was a problem hiding this comment.
Shouldn't this also be a cause for failure? Is it possible not to have this permission, but still have the permission to write?
| ResourceArns: []string{busARN}, | ||
| }) | ||
| if err != nil { | ||
| log.WithField("error", err).Warn("SimulatePrincipalPolicy failed; skipping write-permission check") |
2 participants
Description:
New
materialize-eventbridgeconnector — Pattern 4 (push-only, delta-updates) materialization to AWS EventBridge.Source+DetailType. The full materialized document is the eventDetailpayload.credentialsblock —AWSAccessKey(static keys) orAWSIAM(assume-role + STS, same pattern asmaterialize-dynamodb).Applyverifies the bus viaDescribeEventBus; at-least-once delivery; 256 KB per-entry limit enforced client-side.boilerplate.Materializer(the other Pattern-4 connectors hand-roll), so it runs on the standardbptestrig and produces the same snapshot files SQL materializers do.Closes #4262.
Workflow steps:
User picks region, bus name, and auth mode; adds bindings with
source+detail_type. Documents are published viaPutEventsbatched 10/call with boundederrgroupparallelism (storeConcurrency=8); partial-failure responses are classified per-entry — throttle codes retry with exponential backoff, permanent codes fail-fast.Documentation links affected:
User docs for
materialize-eventbridgewill follow in a separate doc PR.Notes for reviewers:
-eventbridge.test-all(resources documented inmaterialize-eventbridge/testdata/README.md— account076183946664, regionus-east-2).aws.EndpointResolverWithOptionsFuncfor consistency with four other AWS-touching connectors; deferred to a repo-wide migration.