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-lambda-event-sources): S3EventSource should take IBucket instead of Bucket #4323

Closed
guangyuzh opened this issue Oct 1, 2019 · 36 comments · Fixed by #28943
Closed

(aws-lambda-event-sources): S3EventSource should take IBucket instead of Bucket #4323

guangyuzh opened this issue Oct 1, 2019 · 36 comments · Fixed by #28943
Labels
@aws-cdk/aws-lambda-event-sources @aws-cdk/aws-s3-notifications @aws-cdk/aws-s3 Related to Amazon S3 effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p1

Comments

@guangyuzh
Copy link

guangyuzh commented Oct 1, 2019

Reproduction Steps

Cannot import an existing s3 bucket when creating Lambda s3 events:

  1. aws_s3.Bucket.from_bucket_name(...) or aws_s3.Bucket.from_bucket_arn(...), etc. returns aws_s3.IBucket type
  2. However, aws_lambda_event_sources.S3EventSource only takes aws_s3.Bucket type as a parameter
  3. Note: aws_s3.Bucket implements aws_s3.IBucket

Error Log

File "/usr/local/lib/python3.6/site-packages/jsii/_runtime.py", line 66, in __call__
    inst = super().__call__(*args, **kwargs)
  File "/usr/local/lib/python3.6/site-packages/aws_cdk/aws_lambda_event_sources/__init__.py", line 199, in __init__
    jsii.create(S3EventSource, self, [bucket, props])
  File "/usr/local/lib/python3.6/site-packages/jsii/_kernel/__init__.py", line 208, in create
    overrides=overrides,
  File "/usr/local/lib/python3.6/site-packages/jsii/_kernel/providers/process.py", line 331, in create
    return self._process.send(request, CreateResponse)
  File "/usr/local/lib/python3.6/site-packages/jsii/_kernel/providers/process.py", line 316, in send
    raise JSIIError(resp.error) from JavaScriptError(resp.stack)
jsii.errors.JSIIError: Object of type @aws-cdk/aws-s3.IBucket is not convertible to @aws-cdk/aws-s3.Bucket

Environment

  • CLI Version : v1.10.1
  • Framework Version: v1.10.1
  • OS : Mac OS
  • Language : Python

Other

aws_lambda_event_sources.S3EventSource should take aws_s3.IBucket as a parameter's type.


This is 🐛 Bug Report

@guangyuzh guangyuzh added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Oct 1, 2019
@SomayaB SomayaB added language/python Related to Python bindings @aws-cdk/aws-s3 Related to Amazon S3 labels Oct 1, 2019
@SomayaB SomayaB added investigating This issue is being investigated and/or work is in progress to resolve the issue. needs-reproduction This issue needs reproduction. and removed needs-triage This issue or PR still needs to be triaged. investigating This issue is being investigated and/or work is in progress to resolve the issue. labels Oct 1, 2019
@eladb eladb added the p2 label Oct 23, 2019
@eladb eladb assigned iliapolo and unassigned eladb Jan 22, 2020
@AjkayAlan
Copy link

+1 - This is causing us some pain around unit testing S3EventSource in C#.

@leantorres73
Copy link

+1 - same issue for Typescript

@TonyFNZ
Copy link

TonyFNZ commented May 6, 2020

The underlying issue here is that CloudFormation can only set the NotificationConfiguration on a S3 Bucket in the CFN template which created the bucket. This means that if an existing S3 bucket is imported into a CDK stack then it is not possible to update the notification configuration of the bucket, even if the parameter types on aws_lambda_event_sources.S3EventSource are changed.

Please +1 to get this changed in CFN aws-cloudformation/cloudformation-coverage-roadmap#79

@purnesh
Copy link

purnesh commented May 6, 2020

+1

@pigna90
Copy link

pigna90 commented Jul 6, 2020

+1

@iliapolo
Copy link
Contributor

iliapolo commented Jul 10, 2020

@TonyFNZ Wrote:

The underlying issue here is that CloudFormation can only set the NotificationConfiguration on a S3 Bucket in the CFN template which created the bucket.

Yes, you can see more details here: #2004

Please +1 to get this changed in CFN aws-cloudformation/cloudformation-coverage-roadmap#79

Yes, please do that :) (rather than on this cdk issue).

@iliapolo iliapolo added feature-request A feature should be added or improved. and removed needs-reproduction This issue needs reproduction. language/python Related to Python bindings bug This issue is a bug. labels Jul 10, 2020
@iliapolo
Copy link
Contributor

Converting to a feature-request which is somewhat a duplicate of #2004.

@iliapolo iliapolo added the effort/large Large work item – several weeks of effort label Aug 10, 2020
@ThomasKiec
Copy link

ThomasKiec commented Aug 21, 2020

@TonyFNZ I think there should be another way as well. Maybe some workaround in the background without Cloudformation.

The serverless team achieved it(See https://www.serverless.com/framework/docs/providers/aws/events/s3#using-existing-buckets). Then the @aws-cdk team should also find a way to solve this?

@iliapolo
Copy link
Contributor

@ThomasKiec Yes - we actually have a solution in mind and probably won't be waiting on CFN to add this support. So stay tuned.

@ireneaguilar-seat
Copy link

Any update for this issue?

@itharavi
Copy link

itharavi commented Jul 3, 2021

+1. Facing the same issue in our case too.

@istvanszabo92
Copy link

Similar problem with 1.115.0 (Object of type @aws-cdk/aws-s3.BucketBase is not convertible to @aws-cdk/aws-s3.Bucket)

@farshadniayeshpour
Copy link

farshadniayeshpour commented Sep 8, 2021

@eladb @iliapolo Unfortunately, this is something that forces me not to use CDK on a monthly basis even though I start writing my app in CDK but then realize we cannot create a lambda event source with an existing bucket. I would really appreciate it if you could resolve this or provide some type of recommendations so I can start working on it.

@nija-at
Copy link
Contributor

nija-at commented Sep 10, 2021

@farshadniayeshpour - have you tried using the bucket notification mechanism in the s3 module - https://github.com/aws/aws-cdk/tree/master/packages/%40aws-cdk/aws-s3#bucket-notifications. This supports setting up notification on imported buckets to invoke a lambda function.

@LucasOliveiraS
Copy link

+1. Facing the same issue.

@kornicameister
Copy link
Contributor

@nija-at using this module you can't exactly submit to objectcreated:*. You can choose one event type like objectcreated:post but if you try to submit another notification for, say, multipart it will tell you that you have two notifications with overlapping configuration (prefix, suffix). Perhaps that would be a workaround if there were ability to specify wildcard event?

Hope I didn't confuse things up ;P

@ThomasKiec
Copy link

@kornicameister Subscribing all object created events is possible. When adding the event notification via addEventNotification you can simply use the EventType.OBJECT_CREATED (=s3:ObjectCreated:*).

See @aws-cdk/aws-s3: EventType interface

A wildcard pattern is probably not useful, since currently only the event types listed here are supported by AWS. Also, the above interface contains all supported event types

@ericbn
Copy link

ericbn commented Oct 7, 2021

Since cdk 1.110.0, this is possible for existing buckets using IBucket.add_event_notification as @ThomasKiec mentioned.

Here's an example for reference:

import aws_cdk.aws_s3 as s3
import aws_cdk.aws_s3_notifications as s3n

bucket = s3.Bucket.from_bucket_name(scope, 'bucket-id', bucket_name='bucket-name')
bucket.add_event_notification(s3.EventType.OBJECT_CREATED, s3n.LambdaDestination(lambda_fn), prefix="prefix/")

The enhancement for this was merged in this PR: #15158

@deepak3082
Copy link

Does this fix also work for lambda.add_event_source(S3EventSource) . I am still getting an error with the following code block. I have this code in my create-lambda stack and i am on cdk 1.122.0 version.

myexistingbucket = _s3.Bucket.from_bucket_name(self, 'existingbucket', 'mybucketName')

lambda_trigger.add_event_source(S3EventSource(myexistingbucket,
                                                      events=[_s3.EventType.OBJECT_CREATED],
                                                      filters=[_s3.NotificationKeyFilter(prefix="lambdacode/")]
                                                      )
                                        )

@ericbn
Copy link

ericbn commented Nov 19, 2021

@deepak3082, not sure about versions after cdk 1.110.0. But up to that version, only IBucket.add_event_notification works. And it actually will create the exact CloudFormation structure as expected. You're just defining the event notification from the bucket to the lambda, instead of from the lambda to the bucket.

@deepak3082
Copy link

@ericbn .yes, my use case is to trigger lambda once a file is created/updated in s3 and i can not make any code change in the create-s3-stack. Hence, i want to update the create-lambda-stack code and add the eventsource method per aws cdk docs and I am not able to use IBucket.

@bchathoth
Copy link

+1. Facing same issue with cdk 1.130.0 version

@Xyz426
Copy link

Xyz426 commented Dec 28, 2021

+1, Any update? CDK V2 still can not use IBucket.

@rix0rrr rix0rrr added p1 and removed p2 labels Mar 16, 2022
@alxojy
Copy link

alxojy commented Apr 12, 2022

any updates on this issue? 3 years later, still not fixed

@weijiany
Copy link

+1

@wcheek
Copy link

wcheek commented Jun 2, 2022

I still get an error when trying to define the event notification on the lambda function, but things work when defining on the bucket as suggested by @ericbn. So although this issue should still be fixed, it's no longer a breaking problem. Simply create an IBucket object using its name or arn and then add_event_notification()

@luca-ferreri
Copy link

In my case the solution of @ericbn proposed here worked only partially. In facts, with .add_event_notifications I got the error

AttributeError: '_BucketBaseProxy' object has no attribute 'add_event_notfication'

while I had no problem with .add_objcet_created_notification.

My working snippet follows for reference

bucket = aws_s3.Bucket.from_bucket_name(
    self, id="id-bucket", bucket_name="bucket-name"
)
bucket.add_object_created_notification(
    aws_s3_notifications.LambdaDestination(lambda_fn),
    aws_s3.NotificationKeyFilter(prefix="someprefix/"),
)

@Smikha
Copy link

Smikha commented Oct 19, 2022

Using aws_s3.Bucket.from_bucket_name(self, id='bucket-name', bucket_name="some_bucket_name")
Causes my lambda stacks to throw the following error:

Received response status [FAILED] from custom resource. Message returned: Error: An error occurred (NoSuchBucket) when calling the PutBucketNotificationConfiguration operation: The specified bucket does not exist.

Alternatively, using aws_s3.Bucket.from_bucket_arn(self, id='bucket-name',bucket_arn='some-bucket-arn') works fine.

Any hope for a fix?

@kornicameister
Copy link
Contributor

I think from importing by name implies both account and region you are currently deploying to whereas the ARN version allows to explicitly set both of those.

@lkr2des
Copy link

lkr2des commented Sep 5, 2023

+1

@mergify mergify bot closed this as completed in #28943 Feb 8, 2024
mergify bot pushed a commit that referenced this issue Feb 8, 2024
…28943)

### Issue # (if applicable)

Closes #4323 

### Reason for this change

S3EventSource should accept `IBucket` instead of `Bucket`.
`aws_s3.Bucket.from_bucket_name(...)` or `aws_s3.Bucket.from_bucket_arn(...)`, etc. returns aws_s3.IBucket type

### Description of changes

Based on @otaviomacedo 's comment in #25782 , a new class `S3EventSourceV2` is implementd to accept `IBucket` instead of `Bucket`. And avoids breaking changes.

### Description of how you validated changes

- unit test
- integration test

### Checklist
- [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

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

github-actions bot commented Feb 8, 2024

⚠️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.

TheRealAmazonKendra pushed a commit that referenced this issue Feb 9, 2024
…28943)

### Issue # (if applicable)

Closes #4323 

### Reason for this change

S3EventSource should accept `IBucket` instead of `Bucket`.
`aws_s3.Bucket.from_bucket_name(...)` or `aws_s3.Bucket.from_bucket_arn(...)`, etc. returns aws_s3.IBucket type

### Description of changes

Based on @otaviomacedo 's comment in #25782 , a new class `S3EventSourceV2` is implementd to accept `IBucket` instead of `Bucket`. And avoids breaking changes.

### Description of how you validated changes

- unit test
- integration test

### Checklist
- [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment