fix(lambda-nodejs): AWS SDK v2 is not available for node18.x runtime

[yamatatsu]

This PR set default of externalModules aws-sdk v3 when use greater than or equal nodejs v18 runtime, because Lambda runtime nodejs v18 includes aws-sdk v3.

fix #22976

---

All Submissions:

• Have you followed the guidelines in our Contributing guide?

Adding new Unconventional Dependencies:

• This PR adds new unconventional dependencies following the process described here

New Features

• Have you added the new feature to an integration test?
  • Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?

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

[gitpod-io]

[jogold]

We can also maybe remove this

aws-cdk/packages/@aws-cdk/aws-lambda-nodejs/lib/function.ts

Line 115 in d7e0647

if (props.awsSdkConnectionReuse ?? true) {

with Node.js 18.x

[yamatatsu]

@jogold
Thank you for your review!

We can also maybe remove this with Node.js 18.x

Sorry, I don't know about it. And I think it is better not to mix other improvement, because this PR will fix a bug. So I change the name of this PR.

[jogold]

@jogold Thank you for your review!

We can also maybe remove this with Node.js 18.x

Sorry, I don't know about it. And I think it is better not to mix other improvement, because this PR will fix a bug. So I change the name of this PR.

Sounds good. Can you confirm that the deployed Lambda runs without issues?

Hope someone from the CDK team can review this soon.

[yamatatsu]

@jogold

Can you confirm that the deployed Lambda runs without issues?

I've added tests to confirm lambda functions can be invoked without runtime error in 2900339 (#22989).

[corymhall]

Just one comment on the integ test

[corymhall]

Suggested change

	for (const testCase of [sdkV2testCase, sdkV3testCase]) {
	const integ = new IntegTest(app, `LambdaDependencies-${testCase.stackId}`, {
	testCases: [testCase],
	});
	const response = integ.assertions.awsApiCall('Lambda', 'invoke', {
	FunctionName: testCase.lambdaFunction.functionName,
	});
	response.provider.addToRolePolicy({
	Effect: 'Allow',
	Action: ['lambda:InvokeFunction'],
	Resource: [testCase.lambdaFunction.functionArn],
	});
	response.expect(ExpectedResult.objectLike({
	StatusCode: 200,
	}));
	}
	const integ = new IntegTest(app, 'LambdaDependencies', {
	testCases: [sdkV2testCase, sdkV3testCase],
	});
	for (const testCase of [sdkV2testCase, sdkV3testCase]) {
	const response = integ.assertions.awsApiCall('Lambda', 'invoke', {
	FunctionName: testCase.lambdaFunction.functionName,
	});
	response.provider.addToRolePolicy({
	Effect: 'Allow',
	Action: ['lambda:InvokeFunction'],
	Resource: [testCase.lambdaFunction.functionArn],
	});
	response.expect(ExpectedResult.objectLike({
	StatusCode: 200,
	}));
	}

There should be one IntegTest per test, but it can have multiple test cases.

[yamatatsu]

@corymhall
I tried your code, but not work. Because when we use awsApiCall() twice with same service and action, the ids of AwsApiCall construct is duplicated.

Error message:

/aws-cdk/node_modules/constructs/lib/construct.js:334
            throw new Error(`There is already a Construct with name '${childName}' in ${typeName}${name.length > 0 ? ' [' + name + ']' : ''}`);
            ^

Error: There is already a Construct with name 'AwsApiCallLambdainvoke' in Stack [DeployAssert]
    at Node.addChild (/aws-cdk/node_modules/constructs/lib/construct.js:334:19)
    at new Node (/aws-cdk/node_modules/constructs/lib/construct.js:29:21)
    at new Construct (/aws-cdk/node_modules/constructs/lib/construct.js:368:21)
    at new ApiCallBase (/aws-cdk/packages/@aws-cdk/integ-tests/lib/assertions/api-call-base.js:13:9)
    at new AwsApiCall (/aws-cdk/packages/@aws-cdk/integ-tests/lib/assertions/sdk.js:17:9)

AwsApiCall's id made by string literal as following:

`AwsApiCall${service}${api}`

at here.

I'll create issue about it.

[yamatatsu]

Ah.. I realized it is work if we use integ.assertions.invokeFunction() or new AwsApiCall() instead of integ.assertions.awsApiCall(). Now I'm fixing.

Pull request has been modified.

[mrgrain]

Looks like everything is resolved now. Made a minor adjustment to the integ test because I found just having Payload: 'null' (and 'null' being a string) slightly confusing.

Thanks everyone involved on this, especially @yamatatsu and @jogold !
That's the power of this community 🚀 🚀 🚀

[aws-cdk-automation]

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
• Commit ID: 77cf98d
• Result: SUCCEEDED
• Build Logs (available for 30 days)

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

[mergify]

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).

[dan753722]

I am curious why this is an expected behaviour for CDK to play nice with the AWS SDK v2 / node18.x runtime combo?

I love how CDK allows me to write aws infrastructure in a language not yaml.

However, I don't think this PR helps customers move to AWS SDK v3.

• aws-sdk-v2 has "we are getting deprecated, migrate away" everywhere.
• node18 runtime drops aws-sdk-v2 support, (easily searchable knowledge on google.)
• https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_lambda.Runtime.html this cdk doc has no mention of the dark magic behind this PR.
• this PR just gave cdk customers one less reason to upgrade to aws-sdk-v2.

I am sure there are tradeoffs to consider, everyone is busy!

[mrgrain]

I am curious why this is an expected behaviour for CDK to play nice with the AWS SDK v2 / node18.x runtime combo?

Hi @dan753722 I'm not sure I understand your questions/feedback correctly. CDK doesn't currently do anything in particular to make the AWS SDK v2 / node18.x combo nice or easy.

However, I don't think this PR helps customers move to AWS SDK v3.

You are correct it doesn't. Customers have to re-write any handler code themselves to move from SDKv2 to SDKv3

https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_lambda.Runtime.html this cdk doc has no mention of the dark magic behind this PR.

Point taken about improving our documentation. The reason Runtime mentions is that Runtime is a very generic concept. However the feature this PR touches on is local to the NodeJsFunction Construct. I'm sure we can improve the docs though.

this PR just gave cdk customers one less reason to upgrade to aws-sdk-v2

I don't understand what you mean by this. CDK doesn't force users to upgrade to SDKv3. Everyone can decided for themselves if they would like to upgrade or not. This PR fixes a bug for Customers that decided to use SDKv2 with Node18. Without the fix this wouldn't be possible without additional configuration. This PR does not take anything away from the SDKv3 experience.