feat(aww-eventbridge-sqs): add a dlq for the event rule #1253
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
biffgaut
requested review from
hayesry,
mobri2a,
emcfins and
georgebearden
as code owners
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
hayesry
reviewed
Jan 29, 2025
| @@ -115,6 +115,7 @@ constructStack.getEncryptionKey().addToResourcePolicy(policyStatement); | |||
| |existingEventBusInterface?|[`events.IEventBus`](https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_events.IEventBus.html)| Optional user-provided custom EventBus for construct to use. Providing both this and `eventBusProps` results an error.| | |||
| |eventBusProps?|[`events.EventBusProps`](https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_events.EventBusProps.html)|Optional user-provided properties to override the default properties when creating a custom EventBus. Setting this value to `{}` will create a custom EventBus using all default properties. If neither this nor `existingEventBusInterface` is provided the construct will use the `default` EventBus. Providing both this and `existingEventBusInterface` results an error.| | |||
| |eventRuleProps|[`events.RuleProps`](https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_events.RuleProps.html)|User provided eventRuleProps to override the defaults. | | |||
| |deployRuleDlq?|boolean|Whether to deploy a DLQ for the Rule itself (this DLQ is would receive messages that can't be delivered to the target SQS queue). This is new, so defaulting to false to avoid surprising existing clients. Defaults to false| | |||
There was a problem hiding this comment.
nit: Proposing this language for the property description:
|deployRuleDlq?|boolean|Whether to deploy a DLQ for the Event Rule. If set to `true`, this DLQ will receive any messages that can't be delivered to the target SQS queue. Defaults to `false`.|
hayesry
reviewed
Jan 29, 2025
| @@ -163,7 +181,5 @@ export class EventbridgeToSqs extends Construct { | |||
| this.sqsQueue.grantPurge(new ServicePrincipal('events.amazonaws.com')); | |||
| } | |||
|
|
|||
| // Policy for event to be able to send messages to the queue and Grant Event Bridge service access to the SQS queue encryption key | |||
There was a problem hiding this comment.
What's allowing events to be sent to the queue now without this in there anymore? (Mostly curious but wanted to point it out just in case too)
There was a problem hiding this comment.
There's a new aws-event-targets library since we first wrote this, the SqsQueue target object adds the permissions automatically. It doesn't add Purge however.
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
hayesry
reviewed
Jan 31, 2025
| @@ -115,7 +115,9 @@ constructStack.getEncryptionKey().addToResourcePolicy(policyStatement); | |||
| |existingEventBusInterface?|[`events.IEventBus`](https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_events.IEventBus.html)| Optional user-provided custom EventBus for construct to use. Providing both this and `eventBusProps` results an error.| | |||
| |eventBusProps?|[`events.EventBusProps`](https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_events.EventBusProps.html)|Optional user-provided properties to override the default properties when creating a custom EventBus. Setting this value to `{}` will create a custom EventBus using all default properties. If neither this nor `existingEventBusInterface` is provided the construct will use the `default` EventBus. Providing both this and `existingEventBusInterface` results an error.| | |||
| |eventRuleProps|[`events.RuleProps`](https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_events.RuleProps.html)|User provided eventRuleProps to override the defaults. | | |||
| |deployRuleDlq?|boolean|Whether to deploy a DLQ for the Rule itself (this DLQ is would receive messages that can't be delivered to the target SQS queue). This is new, so defaulting to false to avoid surprising existing clients. Defaults to false| | |||
| |targetProps?|[`eventtargets.SqsQueueProps`](https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_events_targets.SqsQueueProps.html)|Optional user provided properties to define the SQS target on the Event Rule. If you specify a deadLetterQueue for the rule here, you are responsible for adding a resource polic. to the queue allowing events.amazonaws.com permission to SendMessage, GetQueueUrl and GetQueueAttributes. You cannot send a DLQ in this property and set deployEventRuleDlq to true. Default is undefined and all system defaults are used.| | |||
There was a problem hiding this comment.
nit: There's a typo here in the middle of 118, should be:
...responsible for adding a resource policy to the queue allowing...
hayesry
reviewed
Jan 31, 2025
| readonly deployEventRuleDlq?: boolean; | ||
| /** | ||
| * Properties to define the key created to protect the ruleDlq | ||
| * Only valid if deployRuleDlq is set to true |
There was a problem hiding this comment.
The comment on L55 has the old deployRuleDlq, should be deployEventRuleDlq
hayesry
reviewed
Jan 31, 2025
| ruleDlqKey?.grantEncryptDecrypt(new ServicePrincipal('events.amazonaws.com')); | ||
| this.eventRuleDlqKey = ruleDlqKey; | ||
|
|
||
| // TODO: Update docs on encrpytWithCustomerMasterKey |
There was a problem hiding this comment.
Just checking, did we want to keep this TODO in here for future or were we supposed to action on this as part of this PR?
hayesry
previously approved these changes
Jan 31, 2025
hayesry
approved these changes
Jan 31, 2025
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #1251
Provides a new Construct property to create a DLQ for the Event Rule - new property defaults to false to avoid changing behavior for existing stacks.
A subsequent PR should add this functionality to other eventbridge constructs, moving the code into core/lib/eventbridge-helper.ts in the process.