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

fix: lambda-nodejs default runtime regression #26763

Merged
merged 10 commits into from
Aug 16, 2023
Merged

fix: lambda-nodejs default runtime regression #26763

merged 10 commits into from
Aug 16, 2023

Conversation

MrArnoldPalmer
Copy link
Contributor

@MrArnoldPalmer MrArnoldPalmer commented Aug 15, 2023

Previously we changed the default version of the lambda-nodejs Function construct to go from using the builtInNodeJsCustomResourceRuntime, a map of regions to available versions, to lambda.Runtime.NODEJS_18_X. The default externalModule configuration excluded the aws-sdk version based on the runtime passed, excluding v2 for Node16 and under, and v3 for Node18 and up, but users can pass their own bundling configuration excluding aws-sdk while not explicitly passing a runtime, which caused their functions to break.

Adds a new lambda.Runtime value for NODEJS_LATEST. This is central reference for the latest version of NodeJS provided by the lamdba service. It also includes a new property isLatest which can be used to indicate that the runtime version may change over time. This can used to indicate that relying on packages shipped with the environment may not be relied upon if the version changes. We default to using the NODEJS_LATEST runtime only if the feature flag is enabled. If the flag is not enabled, use NODEJS_16_X to keep supporting users current bundling configurations.

Additionally, add a warning to tell users if they are excluding a package from their bundling that we know doesn't exist within the runtime they are using. IE, if using NODEJS_18_X and the exclude list includes aws-sdk, warn users that it won't be present.

Fixes #26732


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

Previously we changed the default version of the lambda-nodejs Function
construct to go from using the `builtInNodeJsCustomResourceRuntime`, a
map of regions to available versions, to `lambda.Runtime.NODEJS_18_X`.
The default `externalModule` configuration excluded the aws-sdk
version based on the runtime passed, excluding v2 for Node16 and under,
and v3 for Node18 and up, but users can pass their own bundling
configuration excluding `aws-sdk` while not explicitly passing a
runtime, which caused their functions to break.

Adds a new `lambda.Runtime` value for `NODEJS_LATEST`. This is central
reference for the latest version of NodeJS provided by the lamdba
service. It also includes a new property `isLatest` which can be used to
indicate that the runtime version may change over time. This can used to
indicate that relying on packages shipped with the environment may not
be relied upon if the version changes. We default to using the
`NODEJS_LATEST` runtime only if the feature flag is enabled. If the flag
is not enabled, use `NODEJS_16_X` to keep supporting users current
bundling configurations.

Additionally, add a warning to tell users if they are excluding a
package from their bundling that we know doesn't exist within the
runtime they are using. IE, if using `NODEJS_18_X` and the exclude list
includes `aws-sdk`, warn users that it won't be present.

fixes: #26732
@aws-cdk-automation aws-cdk-automation requested a review from a team August 15, 2023 18:21
@github-actions github-actions bot added the p2 label Aug 15, 2023
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Aug 15, 2023
@aws-cdk-automation aws-cdk-automation dismissed their stale review August 15, 2023 19:07

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

mergify bot pushed a commit that referenced this pull request Aug 15, 2023
Apparently the same user can have both a `COMMENTED` review and a `APPROVED` review. See #26763 and the logs from its prlinter github action:

```
evaluation:  {
  "draft": false,
  "mergeable_state": "behind",
  "maintainerRequestedChanges": false,
  "maintainerApproved": false,
  "communityRequestedChanges": true, // also requested changes
  "communityApproved": true, // approved
  "userRequestsExemption": false
}
```

Also added more logging so that we can see the full data next time. This PR solves the issue by respecting `APPROVED` over `COMMENTED`. Any trusted reviewer who has `APPROVED` a PR will get the PR to `pr/needs-maintainer-review`. Maintainers can always dismiss those reviews if we find that we want to respect someone else's `COMMENTED` review.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@aws-cdk-automation aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Aug 16, 2023
@rix0rrr rix0rrr added the pr/do-not-merge This PR should not be merged at this time. label Aug 16, 2023
Copy link
Contributor

@rix0rrr rix0rrr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is kind of important to get out quickly, I'm doing a conditional approval so you don't need to wait for another round from me.

I am a little concerned though about the current behavior of the feature flag chosen just recreating the same problem again when Node24/SDKv4 rolls around, and so while helping right now it feels a little like kicking the can down the road; and also around us needing to document the chosen behavior better in the README.

I'll leave you to decide what you think is best.

packages/aws-cdk-lib/aws-lambda/lib/runtime.ts Outdated Show resolved Hide resolved

* `@aws-cdk/aws-lambda-nodejs:useLatestRuntimeVersion`

Enable this feature flag to automatically use the latest available NodeJS version in the aws-lambda-nodejse.Function construct.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This solves the problem for now, but isn't this going to cause the exact same problem on the next Node version upgrade?

Effectively, the implicit behavior today was "latest NodeJS version" (because we updated the version from under people without them changing anything), and it broke users in ways they didn't expect.

We can use a flag to make the "hop" between Node <=16 and Node >= 18 an explicit action, but effectively this just keeps the de-facto original behavior of transparently upgrading aws-lambda-nodejs versions out from under users.

Can we do one of the following:

  • Name this @aws-cdk/aws-lambda-nodejs:useSdkV3Runtime (or similar) and make it very clear in the README with a big fat warning block that we will always transparently pick the latest runtime for you if you don't give us one, and you should write your config to be robust against that; OR

  • Name this @aws-cdk/aws-lambda-nodejs:requireRuntime and use that to make the runtime flag effectively a required property via runtime checking. Then, people will need to make a conscious choice whether they want the auto-upgrading behavior or no (when they pick NODEJS_LATEST or NODEJS_XX), and update all the README examples to use NODEJS_LATEST (while dropping a line saying that this may have an effect if they rely on provided packages by the runtime).

In fact we could always drop a warning if people pick NODEJS_LATEST combined with externalModules...?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I want to go with

Name this @aws-cdk/aws-lambda-nodejs:useSdkV3Runtime (or similar) and make it very clear in the README with a big fat warning block that we will always transparently pick the latest runtime for you if you don't give us one, and you should write your config to be robust against that

But also keep the naming of @aws-cdk/aws-lambda-nodejs:useLatestRuntimeVersion since we should be in theory able to keep updating into sdkv4 environments.

In fact we could always drop a warning if people pick NODEJS_LATEST combined with externalModules...?

Adding a warning when externalModules has anything and using NODEJS_LATEST.

@aws-cdk-automation aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Aug 16, 2023
@github-actions github-actions bot added bug This issue is a bug. p1 and removed p2 labels Aug 16, 2023
Adds to the `externalModules` doc string to tell the default is `[]`
when `NODEJS_LATEST` runtime is used.

Adds to the README to detail that `externalModules` defaults to empty
when using `NODEJS_LATEST` and that `NODEJS_LATEST` is the default when
the feature flag is enabled. Also detail that `NODEJS_LATEST` is
designed to update as new versions are available and packages vended
with the environment may change.

Add a warning that is emitted when using `NODEJS_LATEST` and any
dependencies are external.
@MrArnoldPalmer
Copy link
Contributor Author

MrArnoldPalmer commented Aug 16, 2023

I think the solution here is pretty good now, except that the warning we emit may be too aggressive for users that are using NODEJS_LATEST with externalModules that they may be including via layers. However, the warning itself says

When using NODEJS_LATEST the runtime version may change as new runtimes are released, this may affect the availability of packages shipped with the environment. Ensure that any external dependencies are available through layers or specify a specific runtime version.

If a user does see this consistently and wish to silence it, they can pass their own runtime as stated.

Also created #26778 which describes what we need to be able to emit the warning only when layers are not specified. However without knowing that specific layers contain specific external packages, an acknowledgement may be better anyway instead of us just not emitting it when layers.length > 0. That ticket also potentially covers acknowledging annotations in addition to removal.

@MrArnoldPalmer MrArnoldPalmer removed the pr/do-not-merge This PR should not be merged at this time. label Aug 16, 2023
@mergify
Copy link
Contributor

mergify bot commented Aug 16, 2023

Thank you for contributing! Your pull request will be updated from main 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: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: 9203317
  • 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 2aa4733 into main Aug 16, 2023
8 checks passed
@mergify mergify bot deleted the fix/26732 branch August 16, 2023 21:10
@mergify
Copy link
Contributor

mergify bot commented Aug 16, 2023

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

mergify bot pushed a commit that referenced this pull request Nov 15, 2023
With #26763 the default runtime was reverted from NODEJS_18_X to NODEJS_16_X
This PR reflects this in the docs of the NodejsFunctionProps


----

*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
Labels
bug This issue is a bug. contribution/core This is a PR that came from AWS. p1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(custom-resource): node18 doesn't import sdk v2, and framework depends on sdk v2.
4 participants