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

Simplify s3 event with custom bucket configuration #7156

Open
wants to merge 6 commits into
base: master
from

Conversation

@richarddd
Copy link
Contributor

richarddd commented Jan 2, 2020

What did you implement

S3 events can now point to custom CF resources using the resourceId property:

functions:
  resize:
    handler: hello.handler
    events:
      - s3: 
          resouceId: MyAwesomeS3Bucket

resources:
  Resources:
    MyAwesomeS3Bucket:
      Type: AWS::S3::Bucket
      Properties:
        BucketName: my-custom-bucket-name
        # add additional custom bucket configuration here

This removes a lot of boilerplate and code repetition when wanting to customize the bucket and use it as an event source.

How does it work

If the resourceId property exists we will check if there is an existing resource with that logical id. If so, the bucket name is fetched from that resource and used rather than the bucketName of the event property. The logical id is also used in the new logical id of permission resources to keep it consistent with other resources.

How can we verify it

Create a new project with the configuration above.

Todos

  • Write and run all tests
  • Write documentation
  • Enable "Allow edits from maintainers" for this PR
  • Update the messages below

Is this ready for review?: YES
Is it a breaking change?: NO

richarddd added 3 commits Jan 2, 2020
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jan 2, 2020

Codecov Report

Merging #7156 into master will decrease coverage by <.01%.
The diff coverage is 84.61%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7156      +/-   ##
==========================================
- Coverage   88.43%   88.42%   -0.01%     
==========================================
  Files         235      235              
  Lines        8580     8591      +11     
==========================================
+ Hits         7588     7597       +9     
- Misses        992      994       +2
Impacted Files Coverage Δ
lib/plugins/aws/package/compile/events/s3/index.js 98.48% <84.61%> (-1.52%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7a804e1...0d077f7. Read the comment docs.

Copy link
Member

medikoo left a comment

Thanks @richarddd ! Looks as a worthwhile improvement. I've suggested few minor improvements

docs/providers/aws/events/s3.md Outdated Show resolved Hide resolved
lib/plugins/aws/package/compile/events/s3/index.js Outdated Show resolved Hide resolved
lib/plugins/aws/package/compile/events/s3/index.js Outdated Show resolved Hide resolved
Copy link
Member

medikoo left a comment

Looks good, just spotted a few typos

@richarddd

This comment has been minimized.

Copy link
Contributor Author

richarddd commented Jan 2, 2020

Looks good, just spotted a few typos

Lol you misspelled it in your first comment and i was too lazy to write resourceId all over the place so copied it everywhere and introduced typos all over the place 😆

Will fix right away!

Copy link
Member

medikoo left a comment

Thank you @richarddd, we're near there :) There's just two more spots where "id" is put instead of "resourceId". I commented them out

lib/plugins/aws/package/compile/events/s3/index.js Outdated Show resolved Hide resolved
@richarddd

This comment has been minimized.

Copy link
Contributor Author

richarddd commented Jan 3, 2020

Thank you @richarddd, we're near there :) There's just two more spots where "id" is put instead of "resourceId". I commented them out

Doh! Great catch! They are now removed and (hopefully) ready to be merged :)

Copy link
Member

medikoo left a comment

Great thanks @richarddd looks good to me!

@pmuens can you take a second look? What do you think about such improvement?

@medikoo medikoo added this to the 1.61.0 milestone Jan 3, 2020
@medikoo medikoo requested a review from pmuens Jan 7, 2020
Copy link
Member

pmuens left a comment

Hey @richarddd thanks for working on this 👍

This looks good from a code perspective. While I see the benefit I'm a little bit concerned about the contract / abstraction we're trying to establish here. There's #3448 which proposes to implement the missing s3 event configs.

I personally prefer to add the config options rather than having a config to point to CloudFormation.

If we go down that route we should add this option to all the other event sources as well. Otherwise it'll be rather inconsistent.

@richarddd

This comment has been minimized.

Copy link
Contributor Author

richarddd commented Jan 8, 2020

Hey @richarddd thanks for working on this

This looks good from a code perspective. While I see the benefit I'm a little bit concerned about the contract / abstraction we're trying to establish here. There's #3448 which proposes to implement the missing s3 event configs.

I personally prefer to add the config options rather than having a config to point to CloudFormation.

If we go down that route we should add this option to all the other event sources as well. Otherwise it'll be rather inconsistent.

Agree @pmuens . I think we should add the resouceId ability to all other event sources as well. Adding more configuration to the event property of the function will create issues if we were to use the same event source for multiple functions. IMO it's strange that one event specification should be "master" and other consumers. Consider this use case:

function1:
  event:
    - s3:
        bucket: s3bucketname
        acessControll:...
        cors:...
        event: s3:ObjectRemoved:*
function2:
  event:
    - s3:
        bucket: s3bucketname
        event: s3:ObjectCreated:*

Or if we use the same bucket for multiple events on the same function:


function1:
  event:
    - s3:
        bucket: s3bucketname
        acessControll:...
        cors:...
        event: s3:ObjectRemoved:*
    - s3:
        bucket: s3bucketname
        event: s3:ObjectCreated:*

It can become problematic to know which function generated the "advanced" configuration.
The same principle applies to all other event sources as well.

This will be a bigger PR and maybe we could do it in steps. There are some shared code here which could be refactored out in a utils file such as checking if the resourceId exists in userResources and that the type matches the event type.

@medikoo

This comment has been minimized.

Copy link
Member

medikoo commented Jan 8, 2020

In case where we want to create an S3 bucket that triggers multiple lambdas, I think it's definitely better to not put general S3 configuration under one lambda.

e.g. in case of API Gateway it's sorted in a way that we put general API Gateway config into provider.apiGateway. Still we do not support more than one apiGateway that way.
I guess in case of S3 buckets we should be able to support multiple ones so e.g. as:

provider:
  s3:
    photos:
      prop: ...
    videos:
      prop: ...

And adding support for such, would be probably best solution for this case, and #3448

@medikoo medikoo dismissed their stale review Jan 8, 2020

Change the approach

@richarddd

This comment has been minimized.

Copy link
Contributor Author

richarddd commented Jan 8, 2020

In case where we want to create an S3 bucket that triggers multiple lambdas, I think it's definitely better to not put general S3 configuration under one lambda.

e.g. in case of API Gateway it's sorted in a way that we put general API Gateway config into provider.apiGateway. Still we do not support more than one apiGateway that way.
I guess in case of S3 buckets we should be able to support multiple ones so e.g. as:

provider:
  s3:
    photos:
      prop: ...
    videos:
      prop: ...

And adding support for such, would be probably best solution for this case, and #3448

Well, it then introduces a lot of repetition when we use stage (or other) variables in our bucket name as we would need to specify those everywhere. That could be partially solved by using a custom value but still a bit tedius to use something like: ${self: custom.myBucketName} in several places.

My point is that using dynamic naming as a key (reference if you will) is not so ideal.

@medikoo

This comment has been minimized.

Copy link
Member

medikoo commented Jan 9, 2020

Well, it then introduces a lot of repetition when we use stage (or other) variables in our bucket name

Technically we could also support format as:

provider:
  s3:
    - name: photos
      prop: ...
    - name: videos
      prop: ...

Or implement support for vars resolution in property names (doable, but probably more challenging)

@richarddd

This comment has been minimized.

Copy link
Contributor Author

richarddd commented Jan 9, 2020

photos

And how would you reference it then in your event? It would be the same problem, right?

What I'm trying to avoid is something like this:

custom:
   myBucketNameOne: ${self:provider.stage}-my-bucket-one
   myBucketNameTwo: ${self:provider.stage}-my-bucket-two

provider:
  s3:
    - name: ${self:custom.myBucketNameOne}
      prop: ...
    - name: ${self:custom.myBucketNameTwo}
      prop: ...

functions:
   functionOne:
      events:
         - s3:
               bucket: ${self:custom.myBucketNameOne}
               event: s3:ObjectCreated:*
         - s3:
               bucket: ${self:custom.myBucketNameOne}
               event: s3: ObjectRemoved:*
   functionTwo:
      events:
         - s3:
               bucket: ${self:custom.myBucketNameTwo}
               event: s3:ObjectCreated:*
         - s3:
               bucket: ${self:custom.myBucketNameTwo}
               event: s3: ObjectRemoved:*

If we could do something like:

provider:
  s3:
    - referenceId: bucketOne 
      name: ${self:provider.stage}-my-bucket-one
      prop: ...
     - referenceId: bucketTwo
      name: ${self:provider.stage}-my-bucket-two
      prop: ...

functions:
   functionOne:
      events:
         - s3:
               referenceId: bucketOne
               event: s3:ObjectCreated:*

It would work as well

@medikoo

This comment has been minimized.

Copy link
Member

medikoo commented Jan 9, 2020

If we could do something like (...) It would work as well

That's what I had in mind in last comment

@richarddd

This comment has been minimized.

Copy link
Contributor Author

richarddd commented Jan 9, 2020

If we could do something like (...) It would work as well

That's what I had in mind in last comment

Cool! Will implement this instead then! What should be call the id prop then? name could be confused with bucketName IMO

@medikoo

This comment has been minimized.

Copy link
Member

medikoo commented Jan 10, 2020

@richarddd Sorry I quickly replied before, without fully understanding your point.

I think we can do better, and follow configuration method that's already used for naming functions. So:

provider:
  s3:
    bucketOne:
      # name is optional, if not provided it's `bucketOne` that's picked for bucket name
      name: ${self:provider.stage}-my-bucket-one
      prop: ...
  functions:
    function:
      events:
        - s3:
          bucket: bucketOne

@pmuens what do you think?

@richarddd

This comment has been minimized.

Copy link
Contributor Author

richarddd commented Jan 15, 2020

@richarddd Sorry I quickly replied before, without fully understanding your point.

I think we can do better, and follow configuration method that's already used for naming functions. So:

provider:
  s3:
    bucketOne:
      # name is optional, if not provided it's `bucketOne` that's picked for bucket name
      name: ${self:provider.stage}-my-bucket-one
      prop: ...
  functions:
    function:
      events:
        - s3:
          bucket: bucketOne

@pmuens what do you think?

@medikoo @pmuens what do you guys say about the above? It would work perfect IMO :)

@medikoo medikoo requested a review from pmuens Jan 15, 2020
@pmuens

This comment has been minimized.

Copy link
Member

pmuens commented Jan 15, 2020

I'm still not sold on the idea of referencing raw CloudFormation within an abstraction.

However putting the logic under provider makes sense given the discussion both of you had.

@pmuens pmuens removed their request for review Jan 15, 2020
@medikoo

This comment has been minimized.

Copy link
Member

medikoo commented Jan 15, 2020

I'm still not sold on the idea of referencing raw CloudFormation within an abstraction.

Yes, we definitely revert from that

However putting the logic under provider makes sense given the discussion both of you had.

That's the plan.

@richarddd therefore we're all after having it as proposed in my last comment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.