feat: add AWS EventBridge destination - #1059
pryzmatical wants to merge 1 commit into
Conversation
Adds aws_eventbridge as a new destination type, following the existing aws_kinesis/aws_sqs provider pattern (internal/destregistry/providers/ destawseventbridge). Implements the shape discussed on hookdeck#201: - config.event_bus_name (optional, name or ARN) + config.region (required) + config.endpoint (optional, LocalStack/VPC endpoint override) - credentials.key/secret/session are all optional on the destination itself, not hard-required, so a future credential-less publishing mode (the kind some AWS-partnered SaaS platforms use) can be added later without a breaking config change. Static credentials are still always passed to the AWS SDK explicitly, even when empty, rather than omitted -- an empty NewStaticCredentialsProvider fails loudly with an auth error instead of the SDK silently falling back to its default credential chain (env vars, shared config, EC2/ECS/EKS instance role) and authenticating as whatever role the outpost process happens to be running under. - Source is a fixed, app-level setting (DESTINATIONS_EVENTBRIDGE_SOURCE server config, defaulting to "outpost"), not a per-destination field, matching how EventBridge rules commonly key off Source to identify the emitting application. - DetailType is the event's topic; Detail carries metadata+data in the same envelope shape as aws_kinesis's metadata-in-payload mode, since EventBridge has no side channel for metadata the way SQS message attributes do. PutEvents is a batch API even for a single entry, and reports per-entry success/failure in the response body rather than solely via the returned Go error -- a call can return err == nil with FailedEntryCount == 1 if that one entry was rejected. Publish() checks both the top-level error and the entry's own ErrorCode before calling a publish successful, rather than trusting a nil top-level error alone. Error responses (both the top-level and per-entry paths) are sanitized through a shared classifyErrorCode: AWS's own ErrorCode enum values are safe to pass through as-is, but the human-readable message is always authored here, never AWS's own ErrorMessage text, which can embed account IDs and ARNs in permission-denied style errors. Tests: unit coverage for Format() (envelope shape, Source/DetailType/ EventBusName wiring) and the error-classification functions (verifying no AWS-authored message text, including ARNs/account IDs, ever leaks through). No LocalStack-based Publish() integration test is included -- this environment has no Docker available to write and verify one against, and submitting untested integration test code seemed worse than being explicit about the gap. Every other AWS/queue destination's Publish() correctness is likewise only covered by its LocalStack suite, not unit tests, so this matches existing project convention for the parts that are covered. Docs: instructions.md + metadata.json for the new provider, a destinations/aws-eventbridge.mdoc page mirroring aws-kinesis.mdoc, nav.json/ overview.mdoc/concepts.mdoc listing entries, and the DESTINATIONS_EVENTBRIDGE_SOURCE entry in docs/apis/openapi.yaml's managed config schema (hand-maintained alongside config.go, per PR hookdeck#992's precedent). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SNTzCmsH3jiGqgPqi4QMfj
alexluong
left a comment
There was a problem hiding this comment.
Sorry for the late review, and thanks for this! The PR is in good shape and I tested it locally without issues. What's left is mostly cleanup:
key/secretshould be required- OpenAPI is missing the destination schemas (config/credentials, create/update, and
aws_eventbridgein the type enums), so the SDKs can't create this type yet - a LocalStack integration test like the other AWS destinations have (the test compose needs
eventsadded toSERVICES) - a
cmd/destinations/awseventbridgehelper for trying it locally, like the other destination types - a few smaller points inline
Happy to take it from here if you prefer, just let me know.
| "required": false, | ||
| "sensitive": true | ||
| }, | ||
| { | ||
| "key": "secret", | ||
| "type": "text", | ||
| "label": "Secret Access Key", | ||
| "description": "AWS Secret Access Key", | ||
| "required": false, |
There was a problem hiding this comment.
These should be required, same as aws_kinesis. While they're optional, a destination with no credentials is accepted and every publish fails with a generic "the request to EventBridge failed". Making a required field optional later isn't a breaking change, so the credential-less mode can relax this when it lands.
|
|
||
| // AWS EventBridge configuration | ||
| type DestinationAWSEventBridgeConfig struct { | ||
| Source string `yaml:"source" env:"DESTINATIONS_EVENTBRIDGE_SOURCE" desc:"The fixed EventBridge 'Source' field stamped on every published event, shared across all AWS EventBridge destinations in this deployment. Defaults to 'outpost'." required:"N"` |
There was a problem hiding this comment.
Can we name this DESTINATIONS_AWS_EVENTBRIDGE_SOURCE to match DESTINATIONS_AWS_KINESIS_METADATA_IN_PAYLOAD? The OpenAPI entry would change too.
| // toConfig converts DestinationAWSEventBridgeConfig to the provider config | ||
| func (c *DestinationAWSEventBridgeConfig) toConfig() *destregistrydefault.DestAWSEventBridgeConfig { | ||
| source := c.Source | ||
| if source == "" { |
There was a problem hiding this comment.
Destination defaults live in the config defaults in config.go (see AWSKinesis.MetadataInPayload). Can we set Source: "outpost" there, drop this fallback and the "outpost" in the provider's New, and have New return an error when the source is empty? That's how the webhook provider treats its settings.
|
|
||
| output, err := p.client.PutEvents(ctx, input) | ||
| if err != nil { | ||
| code, message := classifyError(err) |
There was a problem hiding this comment.
Kinesis and SQS put the AWS error message in the attempt response, and I'd do the same here. The attempt is only visible to the tenant who owns this AWS account, so the ARN and account ID in an AccessDenied message are their own. The message is also what tells them which action or resource is missing. Non-AWS errors (missing credentials, DNS, timeouts) all end up as "the request to EventBridge failed", which doesn't tell the user what went wrong.
|
|
||
| entry := types.PutEventsRequestEntry{ | ||
| Source: awssdk.String(p.source), | ||
| DetailType: awssdk.String(event.Topic), |
There was a problem hiding this comment.
When a deployment has no TOPICS configured, events can be published without a topic, and DetailType ends up empty, which AWS rejects. Worth a fallback value here?
| eventBusName := destination.Config["event_bus_name"] | ||
| region := destination.Config["region"] | ||
| return destregistry.DestinationTarget{ | ||
| Target: fmt.Sprintf("%s in %s", eventBusName, region), |
There was a problem hiding this comment.
With the default bus this renders as in us-east-1 with no console link. Could it show default in us-east-1 and link to the default bus?
| }) | ||
| } | ||
|
|
||
| if output.FailedEntryCount > 0 && len(output.Entries) > 0 { |
There was a problem hiding this comment.
nit: if FailedEntryCount > 0 but Entries is empty, this falls through to success. AWS shouldn't return that, but treating FailedEntryCount > 0 alone as a failure is safer.
Closes #201.
Adds
aws_eventbridgeas a new destination type, following the existingaws_kinesis/aws_sqsprovider pattern. Implements the shape discussed on #201 with @alexbouchardd:config.event_bus_name(optional, name or ARN — empty uses the account's default bus) +config.region(required) +config.endpoint(optional, LocalStack/VPC endpoint override)credentials.key/secret/sessionare all optional on the destination itself (not hard-required), so a future credential-less publishing mode can be added later without a breaking config change. Static credentials are still always passed to the AWS SDK explicitly, even when empty, rather than omitted — an emptyNewStaticCredentialsProviderfails loudly with an auth error instead of the SDK silently falling back to its default credential chain (env vars, shared config, EC2/ECS/EKS instance role) and authenticating as whatever role the outpost process happens to be running under.Sourceis a fixed, app-level setting (DESTINATIONS_EVENTBRIDGE_SOURCEserver config, defaulting tooutpost), not a per-destination field, matching how EventBridge rules commonly key offSourceto identify the emitting application.DetailTypeis the event's topic;Detailcarriesmetadata+datain the same envelope shape asaws_kinesis's metadata-in-payload mode, since EventBridge has no side channel for metadata the way SQS message attributes do.Batch/error handling
PutEventsis a batch API even for a single entry, and reports per-entry success/failure in the response body rather than solely via the returned Go error — a call can returnerr == nilwithFailedEntryCount == 1if that one entry was rejected.Publish()checks both the top-level error and the entry's ownErrorCodebefore calling a publish successful, rather than trusting a nil top-level error alone.Error responses (both paths) are sanitized through a shared
classifyErrorCode: AWS's ownErrorCodeenum values are safe to pass through as-is, but the human-readable message is always authored here, never AWS's ownErrorMessagetext, which can embed account IDs and ARNs in permission-denied style errors.Tests
Unit coverage for
Format()(envelope shape, Source/DetailType/EventBusName wiring) and the error-classification functions (including a test that explicitly asserts no AWS-authored message text, ARNs, or account IDs ever leak through) — all passing.No LocalStack-based
Publish()integration test is included. My local environment has no Docker available, so I couldn't write and verify one the wayaws_sqs/aws_kinesisdo — I'd rather flag that gap explicitly than submit integration test code I can't confirm actually passes. Every other AWS/queue destination'sPublish()correctness is likewise only covered by its LocalStack suite rather than unit tests, so this matches existing project convention for the parts that are covered; happy to add the integration test in a follow-up once I can verify it, or if a maintainer would rather add it directly.Docs
instructions.md+metadata.jsonfor the new provider, adestinations/aws-eventbridge.mdocpage mirroringaws-kinesis.mdoc,nav.json/overview.mdoc/concepts.mdoclisting entries, and theDESTINATIONS_EVENTBRIDGE_SOURCEentry indocs/apis/openapi.yaml's managed config schema (hand-maintained alongsideconfig.go, per #992's precedent).🤖 Generated with Claude Code
https://claude.ai/code/session_01SNTzCmsH3jiGqgPqi4QMfj