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(custom-resource): custom resources fail with data containing multi-byte utf8 chars #24501

Merged
merged 8 commits into from Mar 10, 2023
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -188,7 +188,10 @@ export async function handler(event: AWSLambda.CloudFormationCustomResourceEvent
hostname: parsedUrl.hostname,
path: parsedUrl.path,
method: 'PUT',
headers: { 'content-type': '', 'content-length': responseBody.length },
headers: {
'content-type': '',
'content-length': Buffer.byteLength(responseBody, 'utf8'),
},
};

return new Promise((resolve, reject) => {
Expand Down
Expand Up @@ -115,7 +115,10 @@ async function submitResponse(status: 'SUCCESS' | 'FAILED', event: Response) {
hostname: parsedUrl.hostname,
path: parsedUrl.path,
method: 'PUT',
headers: { 'content-type': '', 'content-length': responseBody.length },
headers: {
'content-type': '',
'content-length': Buffer.byteLength(responseBody, 'utf8'),
},
};

const retryOptions = {
Expand Down
Expand Up @@ -14,20 +14,19 @@ describe('nodejs entrypoint', () => {
const createEvent = makeEvent({ RequestType: 'Create' });

// WHEN
const response = await invokeHandler(createEvent, async _ => ({ PhysicalResourceId: 'returned-from-handler' }));
const { response } = await invokeHandler(createEvent, async _ => ({ PhysicalResourceId: 'returned-from-handler' }));

// THEN
expect(response.Status).toEqual('SUCCESS');
expect(response.PhysicalResourceId).toEqual('returned-from-handler');

});

test('data (attributes)', async () => {
// GIVEN
const createEvent = makeEvent({ RequestType: 'Create' });

// WHEN
const response = await invokeHandler(createEvent, async _ => {
const { response } = await invokeHandler(createEvent, async _ => {
return {
Data: {
Attribute1: 'hello',
Expand All @@ -47,33 +46,52 @@ describe('nodejs entrypoint', () => {
Foo: 1111,
},
});

});

test('no echo', async () => {
// GIVEN
const createEvent = makeEvent({ RequestType: 'Create' });

// WHEN
const response = await invokeHandler(createEvent, async _ => ({ NoEcho: true }));
const { response } = await invokeHandler(createEvent, async _ => ({ NoEcho: true }));

// THEN
expect(response.Status).toEqual('SUCCESS');
expect(response.NoEcho).toEqual(true);

});

test('reason', async () => {
// GIVEN
const createEvent = makeEvent({ RequestType: 'Create' });

// WHEN
const response = await invokeHandler(createEvent, async _ => ({ Reason: 'hello, reason' }));
const { response } = await invokeHandler(createEvent, async _ => ({ Reason: 'hello, reason' }));

// THEN
expect(response.Status).toEqual('SUCCESS');
expect(response.Reason).toEqual('hello, reason');
});

test('utf8 is supported', async () => {
// GIVEN
const createEvent = makeEvent({ RequestType: 'Create' });
const { request: emptyDataRequest } = await invokeHandler(createEvent, async _ => ({
Data: {
Attribute: '', // 0 bytes
},
}));

// WHEN
const { request: utf8DataRequest } = await invokeHandler(createEvent, async _ => ({
Data: {
Attribute: 'ÅÄÖ', // 6 bytes
},
}));

// THEN
const emptyLength = emptyDataRequest.headers?.['content-length'] as number;
const utf8Length = utf8DataRequest.headers?.['content-length'] as number;
expect(utf8Length - emptyLength).toEqual(6);
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems unnecessarily elaborate, but ok 😀

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, I thought about this. But if I manually counted the bytes it is just a magic number.

});
});

Expand All @@ -82,7 +100,7 @@ describe('nodejs entrypoint', () => {
const createEvent = makeEvent({ RequestType: 'Create' });

// WHEN
const response = await invokeHandler(createEvent, async _ => {
const { response } = await invokeHandler(createEvent, async _ => {
throw new Error('this is an error');
});

Expand All @@ -95,16 +113,14 @@ describe('nodejs entrypoint', () => {
PhysicalResourceId: 'AWSCDK::CustomResourceProviderFramework::CREATE_FAILED',
LogicalResourceId: '<LogicalResourceId>',
});


});

test('physical resource id cannot be changed in DELETE', async () => {
// GIVEN
const event = makeEvent({ RequestType: 'Delete' });

// WHEN
const response = await invokeHandler(event, async _ => ({
const { response } = await invokeHandler(event, async _ => ({
PhysicalResourceId: 'Changed',
}));

Expand All @@ -117,8 +133,6 @@ describe('nodejs entrypoint', () => {
PhysicalResourceId: 'AWSCDK::CustomResourceProviderFramework::MISSING_PHYSICAL_ID',
LogicalResourceId: '<LogicalResourceId>',
});


});

test('DELETE after CREATE is ignored with success', async () => {
Expand All @@ -129,7 +143,7 @@ describe('nodejs entrypoint', () => {
});

// WHEN
const response = await invokeHandler(event, async _ => {
const { response } = await invokeHandler(event, async _ => {
throw new Error('handler should not be called');
});

Expand All @@ -142,7 +156,6 @@ describe('nodejs entrypoint', () => {
PhysicalResourceId: 'AWSCDK::CustomResourceProviderFramework::CREATE_FAILED',
LogicalResourceId: '<LogicalResourceId>',
});

});
});

Expand Down Expand Up @@ -179,17 +192,22 @@ async function invokeHandler(req: AWSLambda.CloudFormationCustomResourceEvent, u
};

let actualResponse;
let actualRequest;
entrypoint.external.sendHttpRequest = async (options: https.RequestOptions, responseBody: string): Promise<void> => {
assert(options.hostname === parsedResponseUrl.hostname, 'request hostname expected to be based on response URL');
assert(options.path === parsedResponseUrl.path, 'request path expected to be based on response URL');
assert(options.method === 'PUT', 'request method is expected to be PUT');
actualResponse = responseBody;
actualRequest = options;
};

await entrypoint.handler(req, {} as AWSLambda.Context);
if (!actualResponse) {
if (!actualRequest || !actualResponse) {
throw new Error('no response sent to cloudformation');
}

return JSON.parse(actualResponse) as AWSLambda.CloudFormationCustomResourceResponse;
return {
response: JSON.parse(actualResponse) as AWSLambda.CloudFormationCustomResourceResponse,
request: actualRequest as https.RequestOptions,
};
}
9 changes: 0 additions & 9 deletions packages/@aws-cdk/custom-resources/.eslintrc.js
@@ -1,13 +1,4 @@
const baseConfig = require('@aws-cdk/cdk-build-tools/config/eslintrc');
baseConfig.parserOptions.project = __dirname + '/tsconfig.json';

baseConfig.overrides = [{
// @TODO Fixing the import order will cause custom resources to updated due to a hash change
// We should apply the fix the next time `framework.ts` is meaningfully changed to avoid unnecessary resource updates
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice! Good opportunity to fix this todo 🥳

"files": ["lib/provider-framework/runtime/framework.ts"],
"rules": {
'import/order': 'warn', // this should be 'error'
}
}]

module.exports = baseConfig;
Expand Up @@ -242,7 +242,10 @@ export async function handler(event: AWSLambda.CloudFormationCustomResourceEvent
hostname: parsedUrl.hostname,
path: parsedUrl.path,
method: 'PUT',
headers: { 'content-type': '', 'content-length': responseBody.length },
headers: {
'content-type': '',
'content-length': Buffer.byteLength(responseBody, 'utf8'),
},
};

return new Promise((resolve, reject) => {
Expand Down
Expand Up @@ -49,7 +49,7 @@ export async function submitResponse(status: 'SUCCESS' | 'FAILED', event: CloudF
method: 'PUT',
headers: {
'content-type': '',
'content-length': responseBody.length,
'content-length': Buffer.byteLength(responseBody, 'utf8'),
},
}, responseBody);
}
Expand Down
@@ -1,10 +1,10 @@
/* eslint-disable max-len */
/* eslint-disable no-console */
import { IsCompleteResponse, OnEventResponse } from '../types';
import * as cfnResponse from './cfn-response';
import * as consts from './consts';
import { invokeFunction, startExecution } from './outbound';
import { getEnv, log } from './util';
import { IsCompleteResponse, OnEventResponse } from '../types';

// use consts for handler names to compiler-enforce the coupling with construction code.
export = {
Expand Down
@@ -0,0 +1,19 @@
{
"version": "30.1.0",
"files": {
"21fbb51d7b23f6a6c262b46a9caee79d744a3ac019fd45422d988b96d44b2a22": {
"source": {
"path": "AwsCustomResourceTestDefaultTestDeployAssert289A7DC5.template.json",
"packaging": "file"
},
"destinations": {
"current_account-current_region": {
"bucketName": "cdk-hnb659fds-assets-${AWS::AccountId}-${AWS::Region}",
"objectKey": "21fbb51d7b23f6a6c262b46a9caee79d744a3ac019fd45422d988b96d44b2a22.json",
"assumeRoleArn": "arn:${AWS::Partition}:iam::${AWS::AccountId}:role/cdk-hnb659fds-file-publishing-role-${AWS::AccountId}-${AWS::Region}"
}
}
}
},
"dockerImages": {}
}
@@ -0,0 +1,36 @@
{
"Parameters": {
"BootstrapVersion": {
"Type": "AWS::SSM::Parameter::Value<String>",
"Default": "/cdk-bootstrap/hnb659fds/version",
"Description": "Version of the CDK Bootstrap resources in this environment, automatically retrieved from SSM Parameter Store. [cdk:skip]"
}
},
"Rules": {
"CheckBootstrapVersion": {
"Assertions": [
{
"Assert": {
"Fn::Not": [
{
"Fn::Contains": [
[
"1",
"2",
"3",
"4",
"5"
],
{
"Ref": "BootstrapVersion"
}
]
}
]
},
"AssertDescription": "CDK bootstrap stack version 6 required. Please run 'cdk bootstrap' with a recent version of the CDK CLI."
}
]
}
}
}

Large diffs are not rendered by default.

@@ -1,28 +1,28 @@
{
"version": "22.0.0",
"version": "30.1.0",
"files": {
"a268caa53756f51bda8ad5f499be4ed8484a81b314811806fbb66f874837c476": {
"8c980c60f4c1c0edebd987e6043e356b8d439b2d731c5af3329df082ca5a6a79": {
"source": {
"path": "asset.a268caa53756f51bda8ad5f499be4ed8484a81b314811806fbb66f874837c476",
"path": "asset.8c980c60f4c1c0edebd987e6043e356b8d439b2d731c5af3329df082ca5a6a79",
"packaging": "zip"
},
"destinations": {
"current_account-current_region": {
"bucketName": "cdk-hnb659fds-assets-${AWS::AccountId}-${AWS::Region}",
"objectKey": "a268caa53756f51bda8ad5f499be4ed8484a81b314811806fbb66f874837c476.zip",
"objectKey": "8c980c60f4c1c0edebd987e6043e356b8d439b2d731c5af3329df082ca5a6a79.zip",
"assumeRoleArn": "arn:${AWS::Partition}:iam::${AWS::AccountId}:role/cdk-hnb659fds-file-publishing-role-${AWS::AccountId}-${AWS::Region}"
}
}
},
"fcfb1dd36ebebdb91b2be028c7fedf6fec075728ba6c5165e04b3d0be259c783": {
"eaaca485f9a98a657308ad4e676057416abc8024d04826b9c08aa25d37293c94": {
"source": {
"path": "aws-cdk-sdk-js.template.json",
"packaging": "file"
},
"destinations": {
"current_account-current_region": {
"bucketName": "cdk-hnb659fds-assets-${AWS::AccountId}-${AWS::Region}",
"objectKey": "fcfb1dd36ebebdb91b2be028c7fedf6fec075728ba6c5165e04b3d0be259c783.json",
"objectKey": "eaaca485f9a98a657308ad4e676057416abc8024d04826b9c08aa25d37293c94.json",
"assumeRoleArn": "arn:${AWS::Partition}:iam::${AWS::AccountId}:role/cdk-hnb659fds-file-publishing-role-${AWS::AccountId}-${AWS::Region}"
}
}
Expand Down