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): use transformToString on API call response #27122

Merged
merged 14 commits into from
Sep 13, 2023

Conversation

colifran
Copy link
Contributor

@colifran colifran commented Sep 13, 2023

This PR fixes an issue where our assertions handler will fail when logging the response of an API call if the response body is a Blob. Specifically, the following error is thrown:

Converting circular structure to JSON
    --> starting at object with constructor 'TLSSocket'
    |     property 'parser' -> object with constructor 'HTTPParser'
    --- property 'socket' closes the circle (RequestId: 50c1b6cd-47d2-494f-baf1-d22646cd4e5f)

The fix for this issue was to call the transformToString method on the response body. This is mentioned in aws-sdk-js-v3 UPGRADE.md as the solution for Lambda::Invoke. Its unclear why S3::GetObject isn't mentioned as well, but it works for that too.

However, instead of explicitly extracting the Body property from a response, we now recursively traverse the response and detect any such blob types that should be converted to strings - this should protect us against any other APIs that may exhibit this behavior.

transformToString is implemented as part of the SdkStreamMixin interface.

Included in this PR is a fix for aws-s3-deployment/test/integ.bucket-deployment-substitution.ts which was previously failing with the error shown above.

Co-authored by: @iliapolo
Co-authored by: @vinayak-kukreja
Co-authored by: @scanlonp

Closes #27114


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

Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
@github-actions github-actions bot added bug This issue is a bug. p2 labels Sep 13, 2023
@aws-cdk-automation aws-cdk-automation requested a review from a team September 13, 2023 02:00
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Sep 13, 2023
Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
@aws-cdk-automation aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Sep 13, 2023
@iliapolo
Copy link
Contributor

For anyone looking at this, i'll take over the PR now and address my own comments :)

Copy link
Contributor

@mrgrain mrgrain left a comment

Choose a reason for hiding this comment

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

Can you add unit tests for coerceResponse?

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: 498ddeb
  • 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 Sep 13, 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 b0bbd5e into main Sep 13, 2023
10 checks passed
@mergify mergify bot deleted the colifran/log-response branch September 13, 2023 15:39
@aws-cdk-automation aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Sep 13, 2023
@iliapolo iliapolo changed the title fix(integ-tests): use transformToString on API call response body fix(integ-tests): use transformToString on API call response Sep 13, 2023
@iliapolo iliapolo changed the title fix(integ-tests): use transformToString on API call response fix(integ-tests): use transformToString on API call response Sep 13, 2023
mikewrighton pushed a commit that referenced this pull request Sep 14, 2023
…7122)

This PR fixes an issue where our assertions handler will fail when logging the response of an API call if the response body is a Blob. Specifically, the following error is thrown:

```
Converting circular structure to JSON
    --> starting at object with constructor 'TLSSocket'
    |     property 'parser' -> object with constructor 'HTTPParser'
    --- property 'socket' closes the circle (RequestId: 50c1b6cd-47d2-494f-baf1-d22646cd4e5f)
```

The fix for this issue was to call the `transformToString` method on the response body. This is mentioned in aws-sdk-js-v3 [`UPGRADE.md`](https://github.com/aws/aws-sdk-js-v3/blob/main/UPGRADING.md?plain=1#L573-L576) as the solution for `Lambda::Invoke`. Its unclear why `S3::GetObject` isn't mentioned as well, but it works for that too. 

However, instead of explicitly extracting the `Body` property from a response, we now recursively traverse the response and detect any such blob types that should be converted to strings - this should protect us against any other APIs that may exhibit this behavior. 

> `transformToString` is implemented as part of the [`SdkStreamMixin` interface](https://docs.aws.amazon.com/AWSJavaScriptSDK/v3/latest/Package/-smithy-types/Interface/SdkStreamMixin/).

Included in this PR is a fix for `aws-s3-deployment/test/integ.bucket-deployment-substitution.ts` which was previously failing with the error shown above.

Co-authored by: @iliapolo
Co-authored by: @vinayak-kukreja 
Co-authored by: @scanlonp 

Closes #27114

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@msambol
Copy link
Contributor

msambol commented Sep 16, 2023

@colifran I'm getting the following when running integration tests in aws-lambda-python-alpha. Still investigating but wondering if it's related to this PR.

2023-09-16T13:54:21.340Z	4a446e5c-4fc1-4c97-8cb7-e8316ee45075	INFO	TypeError: Cannot read properties of undefined (reading 'req')
    at AwsApiCallHandler.processEvent (/var/task/index.js:32693:26)
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
    at async AwsApiCallHandler.handle (/var/task/index.js:32360:26)
    at async Runtime.handler (/var/task/index.js:32753:20)

This is directly after the following log line:

2023-09-16T13:54:19.940Z	4a446e5c-4fc1-4c97-8cb7-e8316ee45075	INFO	SDK request to Lambda.invoke with parameters {
    "FunctionName": "integ-lambda-python-poetr-myhandlerpython38withhas-YlEnyP5nCKSl"
}

@rix0rrr
Copy link
Contributor

rix0rrr commented Sep 18, 2023

Hey @msambol,

That's worrying. Can you go into a bit of detail what test is triggering this problem?

@msambol
Copy link
Contributor

msambol commented Sep 18, 2023

@rix0rrr / @colifran I went back to main to make sure I wasn't missing something. I updated this line to force a change in the integration test (renamed my_handler_inline_with_hashes to my_handler_inline_foo) and got this:

The stack named poetryDefaultTestDeployAssertE9C9CB8F failed to deploy: CREATE_FAILED (The following resource(s) failed to create: [LambdaInvoke631dde0680edf7d2f0eea8d9b9c06c75, LambdaInvokefbdb1fc975f2f372fd64335aa07b46b7, LambdaInvoke26ce5a20d1ffee210d056cd2aa005172, LambdaInvokeead1d1e2cc2d2a843a514ffdc015b2c4, LambdaInvoke26497bb1c12c14ed3deada8d7d76b39e, LambdaInvoke8cbaf98b5be6e4f6f81f5f10b5b8dc89, LambdaInvoke6fd2d93b0d07a2b4220fae7c91786512, LambdaInvoke9a0beb4ea6cc38db92e9ff664c085292]. ): Response object is too long., Response object is too long., Response object is too long., Response object is too long., Response object is too long., Response object is too long.

Different error than I originally got but I think related? Still digging.

@msambol
Copy link
Contributor

msambol commented Sep 19, 2023

@rix0rrr / @colifran One issue at a time... In this line, I logged the size of responseBody and it is 13611 bytes for tests in integ.function.poetry.ts. Maximum size is 4096 bytes (see here).

@mrgrain
Copy link
Contributor

mrgrain commented Sep 22, 2023

@msambol FYI you can pass --force in to always run the integration test.

It's possible this has broken one of the existing tests, 'cause you are absolutely right: They don't get automatically re-run for changes to integ-tests-alpha.

If the response body is too large, we will have to set output paths to limit the response.

@msambol
Copy link
Contributor

msambol commented Sep 22, 2023

Happy to submit a PR with some guidance.

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. p2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

IntegTest Assertions fails while logging response
6 participants