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-nodejs): treat @smithy scope identically to @aws-sdk scope if excluding packages #31610

Closed
1 task
kuhe opened this issue Oct 1, 2024 · 4 comments · Fixed by #31639
Closed
1 task
Assignees
Labels
@aws-cdk/aws-lambda-nodejs bug This issue is a bug. effort/medium Medium work item – several days of effort p1

Comments

@kuhe
Copy link

kuhe commented Oct 1, 2024

Describe the bug

In this readme section, https://github.com/aws/aws-cdk/blob/main/packages/aws-cdk-lib/aws-lambda-nodejs/README.md#externals, it states that

When passing a runtime that is known to include a version of the aws sdk, it will be excluded by default. For example, when passing NODEJS_16_X, aws-sdk is excluded. When passing NODEJS_18_X, all @aws-sdk/* packages are excluded.

However, the core runtime of the AWS SDK for JavaScript (v3) is contained in the scope @smithy/* as can be seen in e.g. client-s3/package.json. If a user application has a certain version of the AWS SDK with both core runtime @smithy/ and AWS packages under @aws-sdk/, deleting only the @aws-sdk/ packages can create a fatal version mismatch between the two sets of dependencies.

From the AWS SDK team's perspective, we recommend that the SDK is not removed from user applications due to the AWS Lambda provided SDK having an unpredictable version update cadence. A sudden change in version not initiated by the user may cause errors due to changes in behavior even if technically no "breaking" interface changes occur.

However, if continuing to remove the @aws-sdk node_modules folder, the @smithy folder should be treated identically since they are tightly coupled.

Regression Issue

  • Select this option if this issue appears to be a regression.

Last Known Working CDK Version

No response

Expected Behavior

If deleting @aws-sdk/ folder, also delete @smithy/ folder. Both NPM scopes are fully controlled by AWS teams.

Current Behavior

Only @aws-sdk/ is deleted, causing mismatch between those and the @smithy/ packages.

Reproduction Steps

Use NODEJS_18_X runtime and bundle the @aws-sdk/client-s3 package as an example.

Inspect the node_modules folder for the existence of @smithy/ packages.

Possible Solution

If deleting @aws-sdk/ folder, also delete @smithy/ folder.

Additional Information/Context

No response

CDK CLI Version

n/a

Framework Version

No response

Node.js Version

NODEJS_18_X, possibly others except NODEJS_LATEST

OS

n/a

Language

TypeScript

Language Version

No response

Other information

No response

@kuhe kuhe added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Oct 1, 2024
@kuhe kuhe changed the title (aws-lambda-nodejs): treat @smithy scope identically to @aws-sdk scope (aws-lambda-nodejs): treat @smithy scope identically to @aws-sdk scope if excluding packages Oct 1, 2024
@pahud
Copy link
Contributor

pahud commented Oct 1, 2024

@kuhe Thank you. We'll bring this up to the team immediately.

@pahud pahud added p1 effort/medium Medium work item – several days of effort and removed needs-triage This issue or PR still needs to be triaged. labels Oct 1, 2024
@GavinZZ GavinZZ self-assigned this Oct 2, 2024
@GavinZZ
Copy link
Contributor

GavinZZ commented Oct 2, 2024

Thanks for creating this issue. We had a team discussion and we believe this is a valid issue and I'll kick off a PR to fix this issue.

@GavinZZ
Copy link
Contributor

GavinZZ commented Oct 2, 2024

Discussed with @kuhe offline, and here's the update.

Context

Node 16 or below would by default use AWS SDK v2 and Lambda provides the SDK by default so CDK removed @aws-sdk modules. See this line.

Similarly for Node 18+ runtimes, since AWS Lambda includes AWS SDK v3 by default, and CDK excludes all the @aws-sdk/* packages because they’re expected to already be present. However, the CDK currently removes only the @aws-sdk/* packages when bundling for Node 18+ runtimes, but it does not remove the @smithy/* packages. This can cause a mismatch in versions between the @smithy/* packages and the AWS SDK packages that AWS Lambda provides.

The mismatch can happen in the following scenarios. This is a pretty edge case but customers did encounter this issue.

/user-app/node_modules/
  - /@smithy/* (v123) <-- this gets used because it wasn't deleted
  - /@aws-sdk/*  (v123) <-- CDK removes `@aws-sdk/*` currently
/lambda-hidden-folder/node_modules
  - /@smithy/* (v456)
  - /@aws-sdk/* (v456) <-- this gets used as fallback since the module is removed from node_modules by CDK

The reason that we don't need to remove smithy models for AWS SDK v2 is because that AWS SDK v3 is built on top of Smithy models and SDK v2 is not.

Proposed Solutions

There are two solutions to the issue.

  1. We remove smithy as well from the bundling. This is an acceptable but not the best change from AWS SDK's perspective. Reason being that
let's say the user deploys the SDK at version v123, but it gets removed so they're actually on version v456, then Lambda does an update of the provided SDK and now they're suddenly on v789, it could cause minor issues 
1. it's hard to troubleshoot because they think they're on v123
2. services can rarely change things like endpoints between versions which cause breaking change
  1. We change the default behaviour to not remove @aws-sdk/*. This is not the best change from CDK's perspective as changing default behaviour may bring unexpected change. Although this doesn't seem and shouldn't cause any regression, this is not the safest change from CDK side.

mergify bot pushed a commit that referenced this issue Oct 3, 2024
… runtimes (under feature flag) (#31639)

### Issue # (if applicable)

Closes #31610

### Reason for this change

for Node 18+ runtimes, since AWS Lambda includes AWS SDK v3 by default, and CDK excludes all the `@aws-sdk/*` packages because they’re expected to already be present. However, the CDK currently removes only the `@aws-sdk/*` packages when bundling for Node 18+ runtimes, but it does not remove the `@smithy/*` packages. This can cause a mismatch in versions between the `@smithy/*` packages and the AWS SDK packages that AWS Lambda provides.

The mismatch can happen in the following scenarios. This is a pretty edge case but customers did encounter this issue.
```
/user-app/node_modules/
  - /@smithy/* (v123) <-- this gets used because it wasn't deleted
  - /@aws-sdk/*  (v123) <-- CDK removes `@aws-sdk/*` currently
/lambda-hidden-folder/node_modules
  - /@smithy/* (v456)
  - /@aws-sdk/* (v456) <-- this gets used as fallback since the module is removed from node_modules by CDK
```

### Description of changes

Add a feature flag. When feature flag is set to true, we will also remove smithy models.

### Description of how you validated changes

Added unit tests and integration tests.

### Checklist
- [ ] 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*
@mergify mergify bot closed this as completed in #31639 Oct 3, 2024
Copy link

github-actions bot commented Oct 3, 2024

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
@aws-cdk/aws-lambda-nodejs bug This issue is a bug. effort/medium Medium work item – several days of effort p1
Projects
None yet
3 participants