Skip to content

Commit

Permalink
fix(core): adding warnings to alert when properties will be overwritt…
Browse files Browse the repository at this point in the history
…en (#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*
  • Loading branch information
IkeNefcy authored Feb 20, 2025
1 parent 8728d5b commit b06daf8
Show file tree
Hide file tree
Showing 3 changed files with 40 additions and 2 deletions.
2 changes: 2 additions & 0 deletions packages/aws-cdk-lib/core/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down
23 changes: 21 additions & 2 deletions packages/aws-cdk-lib/core/lib/custom-resource.ts
Original file line number Diff line number Diff line change
@@ -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';
Expand Down Expand Up @@ -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;

Expand All @@ -62,13 +65,19 @@ 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;

/**
* 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 };
Expand Down Expand Up @@ -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,
},
});
Expand Down
17 changes: 17 additions & 0 deletions packages/aws-cdk-lib/core/test/custom-resource.test.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { toCloudFormation } from './util';
import { Annotations } from '../../assertions';
import { CustomResource, Duration, RemovalPolicy, Stack } from '../lib';

describe('custom resource', () => {
Expand Down Expand Up @@ -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]');
});
});

0 comments on commit b06daf8

Please sign in to comment.