From b06daf836bcbb690d34d1a3439fd6db36cb1f203 Mon Sep 17 00:00:00 2001 From: Ike Nefcy Date: Thu, 20 Feb 2025 10:58:26 -0700 Subject: [PATCH] fix(core): adding warnings to alert when properties will be overwritten (#33507) ### Issue # Closes #32468 ### Reason for this change When using a custom resource, the values for `serviceToken` and `serviceTimeout` are added to the construct prop `properties` during the synth. Thus passing those values through to the lambda. The issue is that these values can be overwritten if you also include the exact keys in your own properties argument. So if I include `serviceToken`, which is a required arg, then I set properties ``` properties: { ServiceToken: 'something else', }, ``` the value of `serviceToken` is set to `ServiceToken`, then my property I wrote to `ServiceToken` takes over and replaces the value with my own. This change is to add a warning to the user so they can understand that what they are doing is overwriting that key, as well as add some more detailed flavor text to the properties and readme to help convey this. ### Description of changes Previously the props like `serviceToken` were being written directly to properties, along with the user provided properties broken out with `...` notation. I moved the automatically added props out of this ``` const constructPropertiesPassed = { ServiceToken: props.serviceToken, ServiceTimeout: props.serviceTimeout?.toSeconds().toString(), }; const hasCommonKeys = Object.keys(properties).some(key => key in constructPropertiesPassed); if (hasCommonKeys) { Annotations.of(this).addWarningV2('@aws-cdk/core:customResourcePropDuplicate', `CustomResource properties should not contain keys that are automatically added by the CDK. Found: ${Object.keys(properties).filter(key => key in constructPropertiesPassed)}`); } this.resource = new CfnResource(this, 'Default', { type, properties: { ...constructPropertiesPassed, ...properties, }, }); ``` This allowed for a simple comparison between the 2 dicts, which allows for the warning to be initiated from. ### Description of how you validated changes I added a test to check if this warning is being generated. I did not change any integs because the actual synth in the end is the exact same as before. ### Checklist - [X] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* --- packages/aws-cdk-lib/core/README.md | 2 ++ .../aws-cdk-lib/core/lib/custom-resource.ts | 23 +++++++++++++++++-- .../core/test/custom-resource.test.ts | 17 ++++++++++++++ 3 files changed, 40 insertions(+), 2 deletions(-) diff --git a/packages/aws-cdk-lib/core/README.md b/packages/aws-cdk-lib/core/README.md index e0c5ff55c501e..d24d240503b7c 100644 --- a/packages/aws-cdk-lib/core/README.md +++ b/packages/aws-cdk-lib/core/README.md @@ -594,6 +594,8 @@ new CustomResource(this, 'MyMagicalResource', { resourceType: 'Custom::MyCustomResource', // must start with 'Custom::' // the resource properties + // properties like serviceToken or serviceTimeout are ported into properties automatically + // try not to use key names similar to these or there will be a risk of overwriting those values properties: { Property1: 'foo', Property2: 'bar', diff --git a/packages/aws-cdk-lib/core/lib/custom-resource.ts b/packages/aws-cdk-lib/core/lib/custom-resource.ts index 3ae49c789c86d..d5a8bb5d78d73 100644 --- a/packages/aws-cdk-lib/core/lib/custom-resource.ts +++ b/packages/aws-cdk-lib/core/lib/custom-resource.ts @@ -1,4 +1,5 @@ import { Construct } from 'constructs'; +import { Annotations } from './annotations'; import { CfnResource } from './cfn-resource'; import { Duration } from './duration'; import { addConstructMetadata, MethodMetadata } from './metadata-resource'; @@ -54,6 +55,8 @@ export interface CustomResourceProps { * serviceToken: myTopic.topicArn, * }); * ``` + * + * Maps to [ServiceToken](https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-cloudformation-customresource.html#cfn-cloudformation-customresource-servicetoken) property for the `AWS::CloudFormation::CustomResource` resource */ readonly serviceToken: string; @@ -62,6 +65,8 @@ export interface CustomResourceProps { * * The value must be between 1 second and 3600 seconds. * + * Maps to [ServiceTimeout](https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-cloudformation-customresource.html#cfn-cloudformation-customresource-servicetimeout) property for the `AWS::CloudFormation::CustomResource` resource + * * @default Duration.seconds(3600) */ readonly serviceTimeout?: Duration; @@ -69,6 +74,10 @@ export interface CustomResourceProps { /** * Properties to pass to the Lambda * + * Values in this `properties` dictionary can possibly overwrite other values in `CustomResourceProps` + * E.g. `ServiceToken` and `ServiceTimeout` + * It is recommended to avoid using same keys that exist in `CustomResourceProps` + * * @default - No properties. */ readonly properties?: { [key: string]: any }; @@ -154,11 +163,21 @@ export class CustomResource extends Resource { } } + const constructPropertiesPassed = { + ServiceToken: props.serviceToken, + ServiceTimeout: props.serviceTimeout?.toSeconds().toString(), + }; + + const hasCommonKeys = Object.keys(properties).some(key => key in constructPropertiesPassed); + + if (hasCommonKeys) { + Annotations.of(this).addWarningV2('@aws-cdk/core:customResourcePropConflict', `The following keys will be overwritten as they exist in the 'properties' prop. Keys found: ${Object.keys(properties).filter(key => key in constructPropertiesPassed)}`); + } + this.resource = new CfnResource(this, 'Default', { type, properties: { - ServiceToken: props.serviceToken, - ServiceTimeout: props.serviceTimeout?.toSeconds().toString(), + ...constructPropertiesPassed, ...properties, }, }); diff --git a/packages/aws-cdk-lib/core/test/custom-resource.test.ts b/packages/aws-cdk-lib/core/test/custom-resource.test.ts index ee102ff469880..3334a0782c296 100644 --- a/packages/aws-cdk-lib/core/test/custom-resource.test.ts +++ b/packages/aws-cdk-lib/core/test/custom-resource.test.ts @@ -1,4 +1,5 @@ import { toCloudFormation } from './util'; +import { Annotations } from '../../assertions'; import { CustomResource, Duration, RemovalPolicy, Stack } from '../lib'; describe('custom resource', () => { @@ -212,4 +213,20 @@ describe('custom resource', () => { }); }).toThrow(`serviceTimeout must either be between 1 and 3600 seconds, got ${invalidSeconds}`); }); + + test('send warning if customResource construct property key is added to properties', () => { + // GIVEN + const stack = new Stack(); + + // WHEN + new CustomResource(stack, 'MyCustomResource', { + serviceToken: 'MyServiceToken', + properties: { + ServiceToken: 'RepeatedToken', // this is repeated because serviceToken prop above will resolve as property ServiceToken + }, + }); + + // THEN + Annotations.fromStack(stack).hasWarning('/Default/MyCustomResource', 'The following keys will be overwritten as they exist in the \'properties\' prop. Keys found: ServiceToken [ack: @aws-cdk/core:customResourcePropConflict]'); + }); });