Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

aws-s3: eventBridgeEnabled not working as expected. #26772

Open
yatin2410 opened this issue Aug 16, 2023 · 9 comments
Open

aws-s3: eventBridgeEnabled not working as expected. #26772

yatin2410 opened this issue Aug 16, 2023 · 9 comments
Labels
@aws-cdk/aws-s3 Related to Amazon S3 bug This issue is a bug. effort/medium Medium work item – several days of effort p2

Comments

@yatin2410
Copy link

Describe the bug

Take this simple example :

const bucket = new s3.Bucket(this, 'MyEventBridgeBucket', { eventBridgeEnabled: true, });

CDK is supposed to enable only event bridge notification. It is available on UI as "Send notifications to Amazon EventBridge for all events in this bucket" but instead CDK is creating IAM role + Policy, Lambda function which I have not defined anywhere and do not want.

Expected Behavior

Expected behaviour should be S3 bucket creation with event bridge notification enabled without extra IAM/Lambda resource creations.

Current Behavior

It is creating extra resources like IAM role+policy, Lambda function which is not mentioned and required to enable event bridge notification.

Reproduction Steps

Create new bucket with below code:

const bucket = new s3.Bucket(this, 'MyEventBridgeBucket', { eventBridgeEnabled: true, });

You will be able to see extra resources in cloud formation.

Possible Solution

No response

Additional Information/Context

No response

CDK CLI Version

4.0

Framework Version

^2.17.0

Node.js Version

18

OS

mac OS 13.4

Language

Typescript

Language Version

No response

Other information

No response

@yatin2410 yatin2410 added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Aug 16, 2023
@github-actions github-actions bot added the @aws-cdk/aws-s3 Related to Amazon S3 label Aug 16, 2023
@pahud
Copy link
Contributor

pahud commented Aug 16, 2023

Makes sense. Looks like current implementation requires NotificationsResourceHandler as a custom resource handler.

const handler = NotificationsResourceHandler.singleton(this, {

And this will be created once when when enableEventBridgeNotification()

public enableEventBridgeNotification() {
this.createResourceOnce();
this.eventBridgeEnabled = true;
}

I am not sure if NotificationsResourceHandler is still required. If yes, we probably still require this custom resource and relevant iam role for the lambda function.

/**
* A custom CloudFormation resource that updates bucket notifications for a
* bucket. The reason we need it is because the AWS::S3::Bucket notification
* configuration is defined on the bucket itself, which makes it impossible to
* provision notifications at the same time as the target (since
* PutBucketNotifications validates the targets).
*
* Since only a single BucketNotifications resource is allowed for each Bucket,
* this construct is not exported in the public API of this module. Instead, it
* is created just-in-time by `s3.Bucket.onEvent`, so a 1:1 relationship is
* ensured.
*
* @see
* https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-s3-bucket-notificationconfig.html
*/

@pahud pahud added response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. p2 effort/medium Medium work item – several days of effort and removed needs-triage This issue or PR still needs to be triaged. labels Aug 16, 2023
@github-actions
Copy link

This issue has not received a response in a while. If you want to keep this issue open, please leave a comment below and auto-close will be canceled.

@github-actions github-actions bot added closing-soon This issue will automatically close in 4 days unless further comments are made. closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. and removed closing-soon This issue will automatically close in 4 days unless further comments are made. labels Aug 18, 2023
@hoangduc67
Copy link

same problem on my situation, on enterprise account can not create many lambda as we want, so I think that if enable event bridge it not really need NotificationsResourceHandler then we should not create this lambda

@blaxx
Copy link

blaxx commented Apr 23, 2024

Same problem here. Would appreciate a fix :)

@pahud
Copy link
Contributor

pahud commented Apr 23, 2024

reopening this issue as it's still relevant

@pahud
Copy link
Contributor

pahud commented Apr 23, 2024

Hi @hoangduc67 @blaxx

As I mentioned above, the reason we need the custom resource is per description below:

/**
* A custom CloudFormation resource that updates bucket notifications for a
* bucket. The reason we need it is because the AWS::S3::Bucket notification
* configuration is defined on the bucket itself, which makes it impossible to
* provision notifications at the same time as the target (since
* PutBucketNotifications validates the targets).
*
* Since only a single BucketNotifications resource is allowed for each Bucket,
* this construct is not exported in the public API of this module. Instead, it
* is created just-in-time by `s3.Bucket.onEvent`, so a 1:1 relationship is
* ensured.
*
* @see
* https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-s3-bucket-notificationconfig.html
*/

I am asking a second review from the maintainers and see if we could get rid of it.

@pahud pahud removed closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. labels Apr 23, 2024
@moelasmar moelasmar self-assigned this Aug 15, 2024
@paulhcsun
Copy link
Contributor

We're not able to remove the NotificationsResourceHandler as that would be a breaking change in existing stacks that are using it. If there comes a day when a Cloudformation L1 for it becomes supported then it would be possible to replace the custom resource but currently that's not the case.

The IAM role + policy do not seem to be overly permissive or anything to me. I understand that it was not something explicitly defined in your stack but as mentioned in

/**
* A custom CloudFormation resource that updates bucket notifications for a
* bucket. The reason we need it is because the AWS::S3::Bucket notification
* configuration is defined on the bucket itself, which makes it impossible to
* provision notifications at the same time as the target (since
* PutBucketNotifications validates the targets).
*
* Since only a single BucketNotifications resource is allowed for each Bucket,
* this construct is not exported in the public API of this module. Instead, it
* is created just-in-time by `s3.Bucket.onEvent`, so a 1:1 relationship is
* ensured.
*
* @see
* https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-s3-bucket-notificationconfig.html
*/

this is a necessary permission.

@paulhcsun
Copy link
Contributor

Apologies I was mistaken about there not being any CFN L1 resources available to replace the custom resource. It should be possible to implement with the following CFN S3 EventBridgeConfiguration resource.

This would likely be implemented as a new property and we would add a deprecation notice and warning to anyone using the old property. The implementation should also make it possible and not overly difficult for customers using the custom resource to migrate off of it.

@yerzhan7
Copy link
Contributor

See also comment here d8e602b for context on why it was done this way:

  1. Why not simply expose EventBridge Cfn property via S3 BucketProps?
    Currently CDK manages NotificationConfigurations via CustomResource. If we were to expose that way, then e.g. SNS configuration would override EventBridge config.

For now you can do the following workaround:

(bucket.node.defaultChild as CfnBucket).notificationConfiguration = {
  eventBridgeConfiguration: {
    eventBridgeEnabled: true,
  },
};

But be aware that if you also add SQS/SNS/Lambda notifications (via native CDK L2 methods) on top of EventBridge then CDK will override it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-s3 Related to Amazon S3 bug This issue is a bug. effort/medium Medium work item – several days of effort p2
Projects
None yet
Development

No branches or pull requests

7 participants