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(integ-tests-alpha): incorrect sdk client resolution #26271

Merged
merged 5 commits into from
Jul 11, 2023

Conversation

MrArnoldPalmer
Copy link
Contributor

Fixes some issues with finding and calling v3 sdk clients.

Exports some utilities from aws-cdk-lib.AwsCustomResource for finding and calling @aws-sdk/client-* modules to avoid duplicating this logic.


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

Fixes some issues with finding and calling v3 sdk clients.

Exports some utilities from `aws-cdk-lib.AwsCustomResource` for finding
and calling `@aws-sdk/client-*` modules to avoid duplicating this logic.
@gitpod-io
Copy link

gitpod-io bot commented Jul 6, 2023

@aws-cdk-automation aws-cdk-automation requested a review from a team July 6, 2023 21:20
@github-actions github-actions bot added the p2 label Jul 6, 2023
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Jul 6, 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.

@MrArnoldPalmer MrArnoldPalmer added the pr-linter/exempt-integ-test The PR linter will not require integ test changes label Jul 6, 2023
@aws-cdk-automation aws-cdk-automation dismissed their stale review July 6, 2023 21:22

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

@aws-cdk-automation aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Jul 6, 2023
@corymhall corymhall added the pr/do-not-merge This PR should not be merged at this time. label Jul 7, 2023
Copy link
Contributor

@corymhall corymhall left a comment

Choose a reason for hiding this comment

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

Just one suggestion

} catch (e) {
// Just import a known client module
throw Error(`Service ${service} client package with name '${clientPackageName}' does not exist.`);
throw Error(`No client constructor found within package: ${sdkPkg.packageName}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we also log the error to make it easier to troubleshoot?

@aws-cdk-automation aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Jul 7, 2023
@MrArnoldPalmer MrArnoldPalmer removed the pr/do-not-merge This PR should not be merged at this time. label Jul 10, 2023
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

@mergify
Copy link
Contributor

mergify bot commented Jul 10, 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 mergify bot merged commit 17e343a into main Jul 11, 2023
@mergify mergify bot deleted the fix/integ-tests-sdk-client-resolution branch July 11, 2023 00:00
mergify bot pushed a commit that referenced this pull request Jul 18, 2023
## Summary
Assertions handler is too large and not working. This PR extracts the SDK v2 to v3 features into a separate package.
Reduces bundled size from `25.8mb` to `1.9mb` and restores the handlers functionality.

```bash
$ esbuild --bundle lib/assertions/providers/lambda-handler/index.ts --target=node14 --platform=node --outfile=lib/assertions/providers/lambda-handler.bundle/index.js

  lib/assertions/providers/lambda-handler.bundle/index.js  1.9mb ⚠️
```

## Why need this change?
After merged #26271, Assertions lambda function throw error at initialize phase and not working. This reason of problem is maybe to import `aws-cdk-lib/custom-resources` to use aws-sdk v2-v3 translate features. 
`aws-cdk-lib/custom-resources` exports the some constructs, so when import the package, then bundle size is too large and lambda function is not working.

```log
INIT_START Runtime Version: nodejs:18.v8	Runtime Version ARN: arn:aws:lambda:us-east-1::runtime:2755dc322c8dbb64760145d6403d14432af527bf4dd3cf03713aae10e0f8b552
--
2023-07-13T05:49:25.501Z	undefined	ERROR	Uncaught Exception     {
    "errorType": "TypeError",
    "errorMessage": "Class extends value undefined is not a constructor or null",
    "stack": [
        "TypeError: Class extends value undefined is not a constructor or null",
        "    at ../../aws-cdk-lib/core/lib/cfn-condition.ts (/var/task/index.js:796267:5)",
        "    at __init (/var/task/index.js:9:56)",
        "    at ../../aws-cdk-lib/core/lib/private/metadata-resource.ts (/var/task/index.js:807546:5)",
        "    at __init (/var/task/index.js:9:56)",
        "    at ../../aws-cdk-lib/core/lib/private/synthesis.ts (/var/task/index.js:814923:5)",
        "    at __init (/var/task/index.js:9:56)",
        "    at ../../aws-cdk-lib/core/lib/app.ts (/var/task/index.js:814951:5)",
        "    at __init (/var/task/index.js:9:56)",
        "    at ../../aws-cdk-lib/core/lib/stack.ts (/var/task/index.js:816555:5)",
        "    at __init (/var/task/index.js:9:56)"
    ]
}
START RequestId: b6da27f5-d7ff-4f76-807b-c7f5d1f02d1f Version: $LATEST
Unknown application error occurredRuntime.Unknown
END RequestId: b6da27f5-d7ff-4f76-807b-c7f5d1f02d1f
REPORT RequestId: b6da27f5-d7ff-4f76-807b-c7f5d1f02d1f	Duration: 13083.91 ms	Billed Duration: 13084 ms	Memory Size: 128 MB	Max Memory Used: 128 MB
```

## Solution

Extract the SDK v2 to v3 features into a new private package that can be bundled safely.
Also required changes to the `aws-custom-resource` v3 handler so it can used the shared code.
To make sure that the bundled files are free of start-up errors, I have added them into a post build script.

## Additional changes

**Update to Encode/Decode logic**
All parameters passed to a Custom Resource will be converted to string when passed to the Lambda Function handler. 
This is a feature/limitation by CloudFormation.
The previous implementation supported special handling for boolean values via a magic string.
However with the SDK v3 parameter types on API Calls are more strictly enforced, e.g. we can't pass a string anymore when a number is expected.
Therefore the encoding/decoding logic has been updated to indiscriminately encode _all_ parameters with `JSON.stringify()` and reverse the operation inside the assertions provider.

**Lambda Invoke Payload**
Withe the SDK v3, the type of the Payload returned from a Lambda Invoke operation is now a Buffer.
This makes sense from an SDK perspective since the returned payload might be a binary.
However assertions can only deal with JSON payloads, so we attempt to convert and parse the payload into a JSON object.
If that fails it stays unchanged.

## Acknowledgements 

Thanks @WinterYukky  for the initial investigation!

Closes #26359
Replaces #26360

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
bmoffatt pushed a commit to bmoffatt/aws-cdk that referenced this pull request Jul 29, 2023
Fixes some issues with finding and calling v3 sdk clients.

Exports some utilities from `aws-cdk-lib.AwsCustomResource` for finding and calling `@aws-sdk/client-*` modules to avoid duplicating this logic.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
bmoffatt pushed a commit to bmoffatt/aws-cdk that referenced this pull request Jul 29, 2023
## Summary
Assertions handler is too large and not working. This PR extracts the SDK v2 to v3 features into a separate package.
Reduces bundled size from `25.8mb` to `1.9mb` and restores the handlers functionality.

```bash
$ esbuild --bundle lib/assertions/providers/lambda-handler/index.ts --target=node14 --platform=node --outfile=lib/assertions/providers/lambda-handler.bundle/index.js

  lib/assertions/providers/lambda-handler.bundle/index.js  1.9mb ⚠️
```

## Why need this change?
After merged aws#26271, Assertions lambda function throw error at initialize phase and not working. This reason of problem is maybe to import `aws-cdk-lib/custom-resources` to use aws-sdk v2-v3 translate features. 
`aws-cdk-lib/custom-resources` exports the some constructs, so when import the package, then bundle size is too large and lambda function is not working.

```log
INIT_START Runtime Version: nodejs:18.v8	Runtime Version ARN: arn:aws:lambda:us-east-1::runtime:2755dc322c8dbb64760145d6403d14432af527bf4dd3cf03713aae10e0f8b552
--
2023-07-13T05:49:25.501Z	undefined	ERROR	Uncaught Exception     {
    "errorType": "TypeError",
    "errorMessage": "Class extends value undefined is not a constructor or null",
    "stack": [
        "TypeError: Class extends value undefined is not a constructor or null",
        "    at ../../aws-cdk-lib/core/lib/cfn-condition.ts (/var/task/index.js:796267:5)",
        "    at __init (/var/task/index.js:9:56)",
        "    at ../../aws-cdk-lib/core/lib/private/metadata-resource.ts (/var/task/index.js:807546:5)",
        "    at __init (/var/task/index.js:9:56)",
        "    at ../../aws-cdk-lib/core/lib/private/synthesis.ts (/var/task/index.js:814923:5)",
        "    at __init (/var/task/index.js:9:56)",
        "    at ../../aws-cdk-lib/core/lib/app.ts (/var/task/index.js:814951:5)",
        "    at __init (/var/task/index.js:9:56)",
        "    at ../../aws-cdk-lib/core/lib/stack.ts (/var/task/index.js:816555:5)",
        "    at __init (/var/task/index.js:9:56)"
    ]
}
START RequestId: b6da27f5-d7ff-4f76-807b-c7f5d1f02d1f Version: $LATEST
Unknown application error occurredRuntime.Unknown
END RequestId: b6da27f5-d7ff-4f76-807b-c7f5d1f02d1f
REPORT RequestId: b6da27f5-d7ff-4f76-807b-c7f5d1f02d1f	Duration: 13083.91 ms	Billed Duration: 13084 ms	Memory Size: 128 MB	Max Memory Used: 128 MB
```

## Solution

Extract the SDK v2 to v3 features into a new private package that can be bundled safely.
Also required changes to the `aws-custom-resource` v3 handler so it can used the shared code.
To make sure that the bundled files are free of start-up errors, I have added them into a post build script.

## Additional changes

**Update to Encode/Decode logic**
All parameters passed to a Custom Resource will be converted to string when passed to the Lambda Function handler. 
This is a feature/limitation by CloudFormation.
The previous implementation supported special handling for boolean values via a magic string.
However with the SDK v3 parameter types on API Calls are more strictly enforced, e.g. we can't pass a string anymore when a number is expected.
Therefore the encoding/decoding logic has been updated to indiscriminately encode _all_ parameters with `JSON.stringify()` and reverse the operation inside the assertions provider.

**Lambda Invoke Payload**
Withe the SDK v3, the type of the Payload returned from a Lambda Invoke operation is now a Buffer.
This makes sense from an SDK perspective since the returned payload might be a binary.
However assertions can only deal with JSON payloads, so we attempt to convert and parse the payload into a JSON object.
If that fails it stays unchanged.

## Acknowledgements 

Thanks @WinterYukky  for the initial investigation!

Closes aws#26359
Replaces aws#26360

----

*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
contribution/core This is a PR that came from AWS. p2 pr-linter/exempt-integ-test The PR linter will not require integ test changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants