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.

Sign up for GitHub

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

Jump to bottom

chore(s3): ensure Lambda size doesn't grow too large #18660

Merged
merged 2 commits into from
Jan 26, 2022
Merged

chore(s3): ensure Lambda size doesn't grow too large #18660

merged 2 commits into from
Jan 26, 2022

Conversation

rix0rrr
Copy link
Contributor

@rix0rrr rix0rrr commented Jan 26, 2022

In #18150, a change was merged that blew up the size of the inline
Lambda beyond its limit of 4096 characters. This change was not
detected because the Lambda constructs being used there didn't use
the regular aws-lambda module, but escape hatches that bypass
the regular validation (released in 1.139.0, 2.8.0).

Because this effectively broke S3 notifications, it was rolled back
in #18507 (released in 1.140.0, not yet released in 2.x line).

In this PR, add validation to make sure an event like this doesn't
happen again. This will be relevant for #18614.

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

@rix0rrr
chore(s3): ensure Lambda size doesn't grow too large
fc24f23 
In #18150, a change was merged that blew up the size of the inline
Lambda beyond its limit of 4096 characters. This change was not
detected because the Lambda constructs being used there didn't use
the regular `aws-lambda` module, but escape hatches that bypass
the regular validation (released in 1.139.0, 2.8.0).

Because this effectively broke S3 notifications, it was rolled back
in #18507 (released in 1.140.0, not yet released in 2.x line).

In this PR, add validation to make sure an event like this doesn't
happen again. This will be relevant for #18614.
@rix0rrr rix0rrr requested a review from a team January 26, 2022 10:42
@rix0rrr rix0rrr self-assigned this Jan 26, 2022
@gitpod-io
Copy link

gitpod-io bot commented Jan 26, 2022

@rix0rrr
Copy link
Contributor Author

rix0rrr commented Jan 26, 2022

cc @yerzhan7 @otaviomacedo

@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Jan 26, 2022
@rix0rrr rix0rrr mentioned this pull request Jan 26, 2022
Merged
@mergify
Copy link
Contributor

mergify bot commented Jan 26, 2022

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
  • Commit ID: 2d928f0
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@mergify mergify bot merged commit 94b0ade into master Jan 26, 2022
@mergify mergify bot deleted the huijbers/check-lambda-size branch January 26, 2022 13:00
@mergify
Copy link
Contributor

mergify bot commented Jan 26, 2022

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@yerzhan7
Copy link
Contributor

@rix0rrr , thanks for this PR. However, somehow when I was running my integration test for the rolled back PR #18150 (yarn integ integ.bucket.notifications.js) the deployment of resource AWS::Lambda::Function was always successful. This suggests that the limit is higher than the documented 4096.

Were there any customer reports of the failed deployments?

You can reproduce this successful deployment by:

  1. Downloading the template for integration test from my PR (note: in there size is 4809): https://github.com/yerzhan7/aws-cdk/blob/62e47e50b9d0c80f8dfaeb1afb0d073aa129d4d3/packages/%40aws-cdk/aws-s3/test/integ.bucket.notifications.expected.json
  2. Remove line 16 (otherwise, CFN deployment will fail for the other reason mentioned in the PR (outdated SDK version))
  3. Deploy template in us-east-1 region. (I also tried eu-west-2 - same result)

image

TikiTDO pushed a commit to TikiTDO/aws-cdk that referenced this pull request Feb 21, 2022
@rix0rrr @TikiTDO
chore(s3): ensure Lambda size doesn't grow too large (aws#18660)
f91b2f4 
In aws#18150, a change was merged that blew up the size of the inline
Lambda beyond its limit of 4096 characters. This change was not
detected because the Lambda constructs being used there didn't use
the regular `aws-lambda` module, but escape hatches that bypass
the regular validation (released in 1.139.0, 2.8.0).

Because this effectively broke S3 notifications, it was rolled back
in aws#18507 (released in 1.140.0, not yet released in 2.x line).

In this PR, add validation to make sure an event like this doesn't
happen again. This will be relevant for aws#18614.


----

*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
Reviewers

@otaviomacedo otaviomacedo otaviomacedo approved these changes

Assignees

@rix0rrr rix0rrr

Labels
contribution/core This is a PR that came from AWS.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@rix0rrr @aws-cdk-automation @yerzhan7 @otaviomacedo