fix(events-targets): AwsApi is still using Node 16 #27002

mrgrain · 2023-09-04T15:49:24Z

Migrate the aws-api-handler code to use SDKv3 and move it into custom-resource-handlers.
Updates the Construct config to use the new code and Node18.
To verify functionality, the existing integration test has been extended to include a new rule and target that can be asserted on.

Includes a config change to the custom-resource-handlers package to run test from TS.

Closes #26998

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

aws-cdk-automation

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.

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

packages/@aws-cdk/sdk-v2-to-v3-adapter/lib/sdk-v3-helpers.ts

MrArnoldPalmer

Approved with DNM because this seems like something others may want to give a peek as well.

MrArnoldPalmer · 2023-09-06T01:58:50Z

packages/@aws-cdk/custom-resource-handlers/lib/aws-events-targets/aws-api-handler/index.ts

+ try {
+ const response = await client.send(new Command(commandInput));
+ delete response.$metadata;
+ console.log('Response: %j', response);


I get that this handler is for triggering effects and doesn't return a value, but I still feel like there might be a sharp edge here wrt Uint8Array types. Someone could use this to trigger Kinesis today passing a string payload and that would break right? We just pass parameters as aws-events passes them I suppose, and I'm not even sure how we could place a marker or something for special case deserialization.

Let me double check this,. But I'm quite sure the default logging algorithm does work for every object without fail. Unlike JSON.stringify() which often throws for unknown data.

Checked this. It writes the payload as bytes into the log: {"0":123,"1":34,"2":101,"3":114,"4":114,"5":111,"6":114,"7":84,"8":121, ... but doesn't fail. It's not great, but I think it's okay. If we have a generic transformation helper for API responses, we can add this later.

As for input: The function is called from EvenBridge (and the API directly) which is JSON by definition. Any other invocations would be outside the expected usage pattern and transformation into a string would be the users responsibility.

I think Mitch' concern is the opposite?

If the SDKv2 API used to accept a string and pass it on, but SDKv3 API now accepts a Uint8Array for that same field (but not a string), existing use cases are now broken.

There's also nothing we can do about it, and it's not really possible to pass that Uint8Array properly from EventBridge, ever.

Ah I see. Yes you are right about this.

It would seem like the only way to do this properly is to know exactly what fields are marked as "blob"s in the API

Yes and that seems unreasonable for us to do.

That seems unreasonable, yes. But right now this is breaking certain use cases.

mergify · 2023-09-06T11:38:41Z

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 · 2023-09-06T12:25:40Z

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 · 2023-09-06T12:52:43Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 36f79a6
Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2023-09-06T12:53:09Z

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

…ay (#27034) In #27002 we have migrated the AwsApi Construct to SDKv3. However some SDKv3 Commands expect a `Uint8Array` in places where SDKv2 previously would accept a `string`. This is a problem because handler input is passed directly into the SDK calls and the input can only contain JSON data types. To solves this, we keep a list of SDK actions and parameters that should be coerced to `Uint8Array`. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

Migrate the `aws-api-handler` code to use SDKv3 and move it into `custom-resource-handlers`. Updates the Construct config to use the new code and Node18. To verify functionality, the existing integration test has been extended to include a new rule and target that can be asserted on. Includes a config change to the `custom-resource-handlers` package to run test from TS. Closes #26998 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

…ay (#27034) In #27002 we have migrated the AwsApi Construct to SDKv3. However some SDKv3 Commands expect a `Uint8Array` in places where SDKv2 previously would accept a `string`. This is a problem because handler input is passed directly into the SDK calls and the input can only contain JSON data types. To solves this, we keep a list of SDK actions and parameters that should be coerced to `Uint8Array`. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

aws-cdk-automation requested a review from a team September 4, 2023 15:49

github-actions bot added the p2 label Sep 4, 2023

mrgrain changed the title ~~fix(aws-events-targets): aws-api-handler still uses Node 16~~ fix(events-targets): aws-api-handler still uses Node 16 Sep 4, 2023

mergify bot added the contribution/core This is a PR that came from AWS. label Sep 4, 2023

mrgrain changed the title ~~fix(events-targets): aws-api-handler still uses Node 16~~ fix(events-targets): AwsApi still uses Node 16 Sep 4, 2023

mrgrain marked this pull request as draft September 4, 2023 15:51

aws-cdk-automation previously requested changes Sep 4, 2023

View reviewed changes

mrgrain added p1 and removed p2 labels Sep 4, 2023

mrgrain changed the title ~~fix(events-targets): AwsApi still uses Node 16~~ fix(events-targets): AwsApi is still using Node 16 Sep 4, 2023

rix0rrr reviewed Sep 5, 2023

View reviewed changes

packages/@aws-cdk/sdk-v2-to-v3-adapter/lib/sdk-v3-helpers.ts Show resolved Hide resolved

mrgrain force-pushed the mrgrain/fix/aws-api-event-target-node18 branch 2 times, most recently from cf6e3e0 to 5a2aa36 Compare September 5, 2023 12:06

mrgrain marked this pull request as ready for review September 5, 2023 12:06

mrgrain added sdk-v3-upgrade Tag issues that are associated to SDK V3 upgrade. Not limited to CR usage of SDK only. node18-upgrade Any work (bug, feature) related to Node 18 upgrade labels Sep 5, 2023

mrgrain requested a review from rix0rrr September 5, 2023 15:18

aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Sep 5, 2023

MrArnoldPalmer added the pr/do-not-merge This PR should not be merged at this time. label Sep 6, 2023

MrArnoldPalmer approved these changes Sep 6, 2023

View reviewed changes

aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Sep 6, 2023

mrgrain force-pushed the mrgrain/fix/aws-api-event-target-node18 branch from 3504641 to 4e0e725 Compare September 6, 2023 09:16

iliapolo approved these changes Sep 6, 2023

View reviewed changes

mrgrain force-pushed the mrgrain/fix/aws-api-event-target-node18 branch from e1a7658 to da2a7df Compare September 6, 2023 11:02

mrgrain removed the pr/do-not-merge This PR should not be merged at this time. label Sep 6, 2023

mrgrain mentioned this pull request Sep 6, 2023

chore: replace Node16 runtimes in unit tests with symbolic runtime #27011

Merged

fix(aws-events-targets): aws-api-handler still uses Node 16

c29a105

mrgrain force-pushed the mrgrain/fix/aws-api-event-target-node18 branch from da2a7df to c29a105 Compare September 6, 2023 11:47

Merge branch 'main' into mrgrain/fix/aws-api-event-target-node18

64106d9

Merge branch 'main' into mrgrain/fix/aws-api-event-target-node18

36f79a6

mergify bot merged commit 49e5739 into main Sep 6, 2023
10 checks passed

mergify bot deleted the mrgrain/fix/aws-api-event-target-node18 branch September 6, 2023 12:53

mrgrain mentioned this pull request Sep 7, 2023

fix(events-targets): AwsApi fails when SDKv3 Command expects Uint8Array #27034

Merged

github-actions bot mentioned this pull request Oct 1, 2023

Monthly PRs metrics report - Sep 2023 #27368

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(events-targets): AwsApi is still using Node 16 #27002

fix(events-targets): AwsApi is still using Node 16 #27002

mrgrain commented Sep 4, 2023 •

edited

Loading

aws-cdk-automation left a comment

MrArnoldPalmer left a comment

MrArnoldPalmer Sep 6, 2023

mrgrain Sep 6, 2023

mrgrain Sep 6, 2023 •

edited

Loading

rix0rrr Sep 6, 2023 •

edited

Loading

mrgrain Sep 6, 2023

rix0rrr Sep 6, 2023

mergify bot commented Sep 6, 2023

mergify bot commented Sep 6, 2023

aws-cdk-automation commented Sep 6, 2023

mergify bot commented Sep 6, 2023

fix(events-targets): AwsApi is still using Node 16 #27002

fix(events-targets): AwsApi is still using Node 16 #27002

Conversation

mrgrain commented Sep 4, 2023 • edited Loading

aws-cdk-automation left a comment

Choose a reason for hiding this comment

MrArnoldPalmer left a comment

Choose a reason for hiding this comment

MrArnoldPalmer Sep 6, 2023

Choose a reason for hiding this comment

mrgrain Sep 6, 2023

Choose a reason for hiding this comment

mrgrain Sep 6, 2023 • edited Loading

Choose a reason for hiding this comment

rix0rrr Sep 6, 2023 • edited Loading

Choose a reason for hiding this comment

mrgrain Sep 6, 2023

Choose a reason for hiding this comment

rix0rrr Sep 6, 2023

Choose a reason for hiding this comment

mergify bot commented Sep 6, 2023

mergify bot commented Sep 6, 2023

aws-cdk-automation commented Sep 6, 2023

AWS CodeBuild CI Report

mergify bot commented Sep 6, 2023

mrgrain commented Sep 4, 2023 •

edited

Loading

mrgrain Sep 6, 2023 •

edited

Loading

rix0rrr Sep 6, 2023 •

edited

Loading