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

s3: allow defining bucket notifications for unowned buckets #2004

Closed
amirsch opened this issue Mar 12, 2019 · 79 comments · Fixed by #15158
Closed

s3: allow defining bucket notifications for unowned buckets #2004

amirsch opened this issue Mar 12, 2019 · 79 comments · Fixed by #15158
Assignees
Labels
@aws-cdk/aws-s3-notifications @aws-cdk/aws-s3 Related to Amazon S3 effort/large Large work item – several weeks of effort feature/enhancement A new API to make things easier or more intuitive. A catch-all for general feature requests. feature-request A feature should be added or improved. in-progress This issue is being actively worked on. management/roadmap Related to project public timeline/roadmap p1

Comments

@amirsch
Copy link

amirsch commented Mar 12, 2019

The use case is fairly simple, creating a new lambda function and attaching an event source from an existing S3 bucket of object creation.

following example code:

   const exampleBucket = Bucket.import(this, 'example-bucket', {bucketArn: 'arn:aws:s3:::example-bucket-arn'});
   fn.addEventSource(new S3EventSource(exampleBucket, { events: [EventType.ObjectCreated] }));

This code has a type mismatch (ImportedBucket vs Bucket), and doesn't work regardless.
Is there another way to use an existing bucket for an S3 event source?

Additional 2x👍 from duplicated issues

@simonemazzoni
Copy link

I have the same problem too.
It looks like the IBucket interface isn't an extension of the Bucket interface, and doesn't have the onEvent() function.
I found instead the onPutObject() function which is kind of suitable for my intent, but it isn't as general as onEvent().

Is this difference intended? Am I missing something?

Thanks in advance for the clarification.

@eladb
Copy link
Contributor

eladb commented Apr 4, 2019

Due to current limitations with CloudFormation and the way we implemented bucket notifications in the CDK, it is impossible to add bucket notifications on an imported bucket. This is why the event source uses s3.Bucket instead of s3.IBucket.

You could use onPutObject:

const bucket = s3.Bucket.import(this, 'B', {
  bucketName: 'my-bucket'
});

const fn = new lambda.Function(this, 'F', {
  code: lambda.Code.inline('boom'),
  runtime: lambda.Runtime.NodeJS810,
  handler: 'index.handler'
});

bucket.onPutObject('put-object', fn);

@eladb eladb changed the title S3EventSource doesn't accept ImportedBucket as a bucket S3EventSource requires Bucket instead of IBucket Apr 4, 2019
@eladb
Copy link
Contributor

eladb commented Apr 4, 2019

We can vend another lambda event source which will be more limited in capabilities and will use putObject. Maybe something like S3PutObjectEventSource

@simonemazzoni
Copy link

simonemazzoni commented Apr 4, 2019

@eladb

You could use onPutObject:

sure I can, but what if I want to bind the lambda to other bucket events?

Below are the events you can choose from the console, and the checked option is my choice.
I cannot do the same thing from the cdk apparently.
Screen Shot 2019-04-04 at 16 45 27

Am I correct?

@eladb
Copy link
Contributor

eladb commented Apr 4, 2019

Sadly no, and neither from CloudFormation.

@simonemazzoni
Copy link

Oh, I see. Should we point this out to AWS directly then?

@nebur395
Copy link

nebur395 commented Jun 27, 2019

Hi, I'm facing the same problem. Is there any news so far or is the issue planned to be fixed soon?

If I'm understanding correctly what @eladb proposes, it implies a different event notification flow:

  • The first one, which is the one that I need, comes from an event source notification and its reflected as an event setting bucket.
  • The approach from @eladb comes from a cloud trail event and its reflected as an object-level logging adding additional costs to the pipeline.

Is there no way to add an event source to a lambda function from an existing bucket using the aws cdk and without using cloud trail?

This is my use case example using serverless framework and what I'm trying to replicate using the cdk:

plugins:
  - serverless-plugin-existing-s3

functions:
  <function-name>:
    handler: handler.main
    events:
      - existingS3:
          bucket: <bucket-name>
          events:
            - s3:ObjectCreated:*
          rules:
            - Suffix: .foo

@TomDufall
Copy link
Contributor

aws-cdk.aws-s3.Bucket.onPutObject appears to no longer exist.
on_cloud_trail_put_object looks like the closest alternative, but that requires creating a CloudTrail trail to capture the event first.
Is that the best way to do this now?

@TomDufall
Copy link
Contributor

TomDufall commented Jun 28, 2019

Bucket.add_event_notification seems to have replaced Bucket.add_event_source.
Updated minimal example that works with a Bucket but not an IBucket below.

my_bucket.add_event_notification(
    aws_s3.EventType.OBJECT_CREATED,
    aws_s3_notifications.LambdaDestination(my_lambda),
    )

@dandu1008
Copy link

dandu1008 commented Jul 31, 2019

@eladb Is there further updates on this, we have many use cases to put notifications on existing buckets. According the below ticket, it says it is implemented in Serverless, any plans to extend this too CDK.

Add support for existing S3 buckets #6290
serverless/serverless#6290

@nebur395
Copy link

nebur395 commented Aug 2, 2019

+1 @dandu1008 @eladb

It is already implemented in Serverless Framework. We also have a lot of use cases that consist on adding lambda notifications on existing buckets.

Anyway it seems is not a problem related only with existing buckets, but with existing resources. We've tried the same approach but trying to subscribe a lambda to an existing SNS topic instead of an existing bucket (this is also possible using Serverless Framework) and it ends in the same result. You cannot add it because importing an existing SNS topic (using fromTopicArn()) returns an ITopic and addEventSource() expects only a Topic.

It is important for us, and in my opinion it's not an uncommon use case, please, it would be great to have that implementation.

Any feedback from your side would be pleasantly appreciated to know that this ticket has not been forgotten.

Best Regards

@moofish32
Copy link
Contributor

@nebur395 -- looking at the Serverless link you sent, you could approach the problem in a similar method using this experimental package. Obviously experimental, but perhaps your feedback and PR if you find something that is robust and community needed?

@eladb eladb added @aws-cdk/aws-s3 Related to Amazon S3 management/roadmap Related to project public timeline/roadmap labels Aug 13, 2019
@eladb eladb changed the title S3EventSource requires Bucket instead of IBucket Support adding bucket notifications to existing buckets Aug 13, 2019
@eladb eladb changed the title Support adding bucket notifications to existing buckets s3: allow defining bucket notifications for unowned buckets Aug 13, 2019
@reidca
Copy link

reidca commented Oct 3, 2019

Due to current limitations with CloudFormation and the way we implemented bucket notifications in the CDK, it is impossible to add bucket notifications on an imported bucket. This is why the event source uses s3.Bucket instead of s3.IBucket.

You could use onPutObject:

const bucket = s3.Bucket.import(this, 'B', {
  bucketName: 'my-bucket'
});

const fn = new lambda.Function(this, 'F', {
  code: lambda.Code.inline('boom'),
  runtime: lambda.Runtime.NodeJS810,
  handler: 'index.handler'
});

bucket.onPutObject('put-object', fn);

Fundamentally this problem stems from the way the cloudformation team designed the S3 bucket resource. They chose to put the notifications as a property of the bucket itself rather than as a separate resource.

This choice has always baffled me and it results in the inability to do anything with an existing S3 bucket other than set the S3 bucket policy (which is a separate resource in cloudformation).

Until such time as this changes (which is probably never) CDK will not be able to implement notifications on existing buckets without using Custom Resources. This is because CDK is ultimately just producing a cloudformation template. I am hopeful that the CDK team may implement something using built-in custom resources magic behind the scenes that abstracts away the complexity of us having to use Custom Resources directly.

This StackOverflow thread explains the problem well and provides a solution in terms of a lambda function backed custom resource for cloudformation. The next part of the puzzle (for me) is how to implement this in CDK!

@reidca
Copy link

reidca commented Oct 7, 2019

@eladb Is this https://github.com/aws/aws-cdk/blob/master/packages/%40aws-cdk/aws-s3/lib/notifications-resource/notifications-resource.ts intended to solve this problem? Are there any examples of using it?

@SomayaB SomayaB added the feature-request A feature should be added or improved. label Oct 23, 2019
@jd-carroll
Copy link
Contributor

@reidca - No its not, fundamentally CloudFormation only supports two resource types. In order to be able to import a bucket and add event notification targets to it, there would have to be a separate AWS::S3::BucketNotification CloudFormation type.

@jd-carroll
Copy link
Contributor

I'll actually take back my previous comment, I think there could be a way to accomplish this using a lambda function.

Here are some high level thoughts:

  • The stack creating the bucket notification should import the bucket so as to create an import / export relationship between the two stacks (this way it is obvious if someone tries to delete the bucket stack)
  • A lambda function would use the SDK to inspect / add the bucket notification to the original bucket. This lambda function would be similar to other singleton functions already created by the CDK and invoked during CloudFormation deployment as a CustomResource.
  • The lambda function would be invoked by the stack requesting the bucket notification (not the stack which creates the bucket)

The caveat would be if the bucket is in a different account. I'm not sure how the permissions would be sorted out for the lambda trying to modify a bucket in a separate account, but it might be possible. (I don't even know if cross account event notification is possible)

@michaelmoussa
Copy link
Contributor

"A lambda function would use the SDK to inspect / add the bucket notification to the original bucket."

Is there a collection of Custom Resources anywhere in the CDK core where something like this could live? You can do pretty much anything with the SDK even if it's not supported by CloudFormation (after all, that's how tools like Terraform work pretty much). It'd be really helpful if functionality like adding bucket notifications for unowned buckets could be handled "natively" by CDK, sort of like...:

import { S3BucketNotification } from "@aws-cdk/custom-resources";

-- snip --

new S3BucketNotification(this, "<name>", {
    bucket: Bucket.fromBucketArn("...") ,
    events: [...],
    filter_rules: [...],
    target: "..."
});

There's nothing preventing anybody from creating a third-party implementation of this, but having an "official" catalog of solutions for cases where CloudFormation design decisions make other approaches impossible would be quite handy.

@michaelbrewer
Copy link
Contributor

@eladb

I am not sure why addEventNotification function can not be part of IBucket? As it is just using a lambda to setup the bucket notifications, at that would work for existing buckets

Otherwise i did like the "Serverless" implementation of the lambda handler as it makes sure to keep any of the existing bucket notifications. You can see the implementation here: https://github.com/serverless/serverless/tree/master/lib/plugins/aws/customResources/resources/s3

@michaelbrewer
Copy link
Contributor

Also if you destroy the stack it would completely clear out any of the notification configured for that bucket

 if (event.RequestType === 'Delete') {
    props.NotificationConfiguration = { }; // this is how you clean out notifications
  }

@tuliocasagrande
Copy link
Contributor

Hey @marcelcastrobr, this is being worked on PR #11773 🙌

@michaelbrewer
Copy link
Contributor

Hey @marcelcastrobr, this is being worked on PR #11773 🙌

Yep i have decided to go for a python lambda (i just need some guidance on how to attach one when in the aws-s3 module without creating a circular dependency @iliapolo might help :) )

@michaelbrewer
Copy link
Contributor

Update wise, pending that PR being approved (#11773)

There is a python lambda with this PR that you can probably use in the mean time

@violehtone
Copy link

Hi,

I'm rather new to AWS and I've also recently encountered the same problem described on this issue. As far as I've understood, the lambda function described in the @michaelbrewer 's PR (#11773) should currently function as a workaround for this issue, correct? Based on this thread, I've understood that the aim of this lambda function is to add notifications to a S3 bucket, right? So that then I can add my SQS queue as the destination of these notifications, which are then subsequently processed from that queue by another lambda function I've developed.

What I'm not understanding is how to use this lambda function described in the PR for creating the S3 bucket notifications? I.e. how to set it up in cdk? How to forward events to this lambda? Where do the events come from? Thanks in advance!

@iliapolo
Copy link
Contributor

iliapolo commented Apr 2, 2021

@violehtone That lambda function being developed as part #11773 is an internal implementation detail, that you would not be exposed to. As a user, the experience would be exactly the same as adding notification to a bucket that is managed by CDK:

import * as s3n from '@aws-cdk/aws-s3-notifications';
import * as s3 from '@aws-cdk/aws-s3';
import * as sqs from '@aws-cdk/aws-sqs';

const bucket = s3.Bucket.fromBucketName(this, 'Bucket', 'my-existing-bucket');
const queue = new sqs.Queue(stack, 'Queue');

bucket.addEventNotification(s3.EventType.OBJECT_CREATED, new s3n.SqsDestination(queue));

Notice that right now, this is only possible for buckets that are created as part of the CDK application, not pre-existing ones.

https://docs.aws.amazon.com/cdk/api/latest/docs/aws-s3-notifications-readme.html#example

@michaelbrewer
Copy link
Contributor

Hopefully with a little guidance, i can complete the PR to support both.

@shpetimselaci
Copy link

Any updates on this?
Having the same issue here.

@michaelbrewer
Copy link
Contributor

Any updates on this?
Having the same issue here.

Nothing new, i am still waiting on some guidance. The python custom resource is ready, it just needs to be plugged into CDK main code base.

@iliapolo
Copy link
Contributor

@michaelbrewer I did another round of review and believe I provided the necessary guidelines: #11773 (review)

Let me know if anything is still missing.

@ericzbeard ericzbeard added the feature/enhancement A new API to make things easier or more intuitive. A catch-all for general feature requests. label Apr 20, 2021
@michaelbrewer
Copy link
Contributor

michaelbrewer commented Apr 24, 2021

@iliapolo I have made most of your suggested changes. I am still not able to get my environment up and running. I hope there is little enough for this to get merge.

@michaelbrewer
Copy link
Contributor

Update wise, i am not sure if this is any closer to be accepted or not, seems to be going sideways in the reviews. I don't really have the time to work on the PR anymore.

@vzverv
Copy link

vzverv commented May 3, 2021

@michaelbrewer please don't give up. You are so close.

@michaelbrewer
Copy link
Contributor

haha, it is like playing Battleships :)

@vzverv
Copy link

vzverv commented May 7, 2021

@michaelbrewer I looked into the list of the tests and it looks pretty good. Cannot think about any additional use cases right now

@michaelbrewer
Copy link
Contributor

Unfortunately this seemed to have stalled.

From the looks of things adding event notifications to existing buckets might also eventually be fixed at the CFN level.

For people that are inpatient it would have been nice to add this to CDK, otherwise a standalone solution will have to be created.

@blake-enyart
Copy link

Unfortunately this seemed to have stalled.

From the looks of things adding event notifications to existing buckets might also eventually be fixed at the CFN level.

For people that are inpatient it would have been nice to add this to CDK, otherwise a standalone solution will have to be created.

Thanks for all the amazing work you have put into this @michaelbrewer ! I've been following along on this for months so excited about all the work you are doing on this

@vzverv
Copy link

vzverv commented Jun 2, 2021

I would like to join @blake-enyart and thank you @michaelbrewer for all the time and efforts you invested into this project!

@michaelbrewer
Copy link
Contributor

PR has now been approved by i had to move it to my personal repo as mergify has issues (#15158)

@mergify mergify bot closed this as completed in #15158 Jun 16, 2021
mergify bot pushed a commit that referenced this issue Jun 16, 2021
An update custom resource changes to support adding notifications to an existing S3 bucket

Resolves #2004

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@github-actions
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

matthewsvu pushed a commit to matthewsvu/aws-cdk that referenced this issue Jun 22, 2021
An update custom resource changes to support adding notifications to an existing S3 bucket

Resolves aws#2004

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
hollanddd pushed a commit to hollanddd/aws-cdk that referenced this issue Aug 26, 2021
An update custom resource changes to support adding notifications to an existing S3 bucket

Resolves aws#2004

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@drernie
Copy link

drernie commented Nov 27, 2023

@michaelbrewer Can I trouble you to explain what exactly this new PR does?
What is the recommended syntax for setting up notifications for my pre-existing bucket 'BucketName'?
Specifically, does this proposed (identical?) syntax actually work?

const bucket = s3.Bucket.fromBucketName(this, 'Bucket', 'my-existing-bucket');
const queue = new sqs.Queue(stack, 'Queue');
bucket.addEventNotification(s3.EventType.OBJECT_CREATED, new s3n.SqsDestination(queue));

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-s3-notifications @aws-cdk/aws-s3 Related to Amazon S3 effort/large Large work item – several weeks of effort feature/enhancement A new API to make things easier or more intuitive. A catch-all for general feature requests. feature-request A feature should be added or improved. in-progress This issue is being actively worked on. management/roadmap Related to project public timeline/roadmap p1
Projects
None yet