Skip to content

Commit

Permalink
fix(core): overrideLogicalId validation (aws#29708)
Browse files Browse the repository at this point in the history
### Issue # (if applicable)

Closes aws#29701

### Reason for this change

Calling `overrideLogicalId` on a `Construct` with an invalid logical ID ([docs](https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/resources-section-structure.html#resources-section-structure-logicalid)) would not throw an error at synthesis time. CloudFormation would 

### Description of changes

* Validate `overrideLogicalId` (must not be empty, must not be over 255 characters, must match `/^[A-Za-z0-9]+$/`
* Document exceptions with `@error` JSDoc tags

### Description of how you validated changes

I've added unit tests, integration tests should not be necessary

### 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
nmussy authored Jun 17, 2024
1 parent 25835e4 commit b196b13
Show file tree
Hide file tree
Showing 2 changed files with 99 additions and 8 deletions.
23 changes: 21 additions & 2 deletions packages/aws-cdk-lib/core/lib/cfn-element.ts
Original file line number Diff line number Diff line change
Expand Up @@ -81,14 +81,33 @@ export abstract class CfnElement extends Construct {
/**
* Overrides the auto-generated logical ID with a specific ID.
* @param newLogicalId The new logical ID to use for this stack element.
*
* @throws an error if `logicalId` has already been locked
* @throws an error if `newLogicalId` is empty
* @throws an error if `newLogicalId` contains more than 255 characters
* @throws an error if `newLogicalId` contains non-alphanumeric characters
*
* @see https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/resources-section-structure.html#resources-section-structure-logicalid
*/
public overrideLogicalId(newLogicalId: string) {
if (this._logicalIdLocked) {
throw new Error(`The logicalId for resource at path ${Node.of(this).path} has been locked and cannot be overridden\n` +
'Make sure you are calling "overrideLogicalId" before Stack.exportValue');
} else {
this._logicalIdOverride = newLogicalId;
}

if (!Token.isUnresolved(newLogicalId)) {
if (!newLogicalId) {
throw new Error('Cannot set an empty logical ID');
}
if (newLogicalId.length > 255) {
throw new Error(`Invalid logical ID override: '${newLogicalId}'. It must be at most 255 characters long, got ${newLogicalId.length} characters.`);
}
if (!newLogicalId.match(/^[A-Za-z0-9]+$/)) {
throw new Error(`Invalid logical ID override: '${newLogicalId}'. It must only contain alphanumeric characters.`);
}
}

this._logicalIdOverride = newLogicalId;
}

/**
Expand Down
84 changes: 78 additions & 6 deletions packages/aws-cdk-lib/core/test/stack.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1370,37 +1370,109 @@ describe('stack', () => {

// THEN - producers are the same
expect(() => {
resourceM.overrideLogicalId('OVERRIDE_LOGICAL_ID');
resourceM.overrideLogicalId('OVERRIDELOGICALID');
}).toThrow(/The logicalId for resource at path Producer\/ResourceXXX has been locked and cannot be overridden/);
});

test('throw error if overrideLogicalId contains non-alphanumeric characters', () => {
// GIVEN: manual
const appM = new App();
const producerM = new Stack(appM, 'Producer');
const resourceM = new CfnResource(producerM, 'ResourceXXX', { type: 'AWS::Resource' });

// THEN - producers are the same
expect(() => {
resourceM.overrideLogicalId('INVALID_LOGICAL_ID');
}).toThrow(/must only contain alphanumeric characters/);
});

test('throw error if overrideLogicalId is over 255 characters', () => {
// GIVEN: manual
const appM = new App();
const producerM = new Stack(appM, 'Producer');
const resourceM = new CfnResource(producerM, 'ResourceXXX', { type: 'AWS::Resource' });

// THEN - producers are the same
expect(() => {
resourceM.overrideLogicalId(
// 256 character long string of "aaaa..."
Array(256).fill('a').join(''),
);
}).toThrow(/must be at most 255 characters long, got 256 characters/);
});

test('throw error if overrideLogicalId is an empty string', () => {
// GIVEN: manual
const appM = new App();
const producerM = new Stack(appM, 'Producer');
const resourceM = new CfnResource(producerM, 'ResourceXXX', { type: 'AWS::Resource' });

// THEN - producers are the same
expect(() => {
resourceM.overrideLogicalId('');
}).toThrow('Cannot set an empty logical ID');
});

test('do not throw error if overrideLogicalId is used and logicalId is not locked', () => {
// GIVEN: manual
const appM = new App();
const producerM = new Stack(appM, 'Producer');
const resourceM = new CfnResource(producerM, 'ResourceXXX', { type: 'AWS::Resource' });

// THEN - producers are the same
resourceM.overrideLogicalId('OVERRIDE_LOGICAL_ID');
resourceM.overrideLogicalId('OVERRIDELOGICALID');
producerM.exportValue(resourceM.getAtt('Att'));

const template = appM.synth().getStackByName(producerM.stackName).template;
expect(template).toMatchObject({
Outputs: {
ExportsOutputFnGetAttOVERRIDELOGICALIDAtt76AC816F: {
Export: {
Name: 'Producer:ExportsOutputFnGetAttOVERRIDELOGICALIDAtt76AC816F',
},
Value: {
'Fn::GetAtt': [
'OVERRIDELOGICALID',
'Att',
],
},
},
},
Resources: {
OVERRIDELOGICALID: {
Type: 'AWS::Resource',
},
},
});
});

test('do not throw if overrideLogicalId is unresolved', () => {
// GIVEN: manual
const appM = new App();
const producerM = new Stack(appM, 'Producer');
const resourceM = new CfnResource(producerM, 'ResourceXXX', { type: 'AWS::Resource' });

// THEN - producers are the same
resourceM.overrideLogicalId(Lazy.string({ produce: () => 'INVALID_LOGICAL_ID' }));
producerM.exportValue(resourceM.getAtt('Att'));

const template = appM.synth().getStackByName(producerM.stackName).template;
expect(template).toMatchObject({
Outputs: {
ExportsOutputFnGetAttOVERRIDELOGICALIDAtt2DD28019: {
ExportsOutputFnGetAttINVALIDLOGICALIDAtt6CB9E5B9: {
Export: {
Name: 'Producer:ExportsOutputFnGetAttOVERRIDELOGICALIDAtt2DD28019',
Name: 'Producer:ExportsOutputFnGetAttINVALIDLOGICALIDAtt6CB9E5B9',
},
Value: {
'Fn::GetAtt': [
'OVERRIDE_LOGICAL_ID',
'INVALID_LOGICAL_ID',
'Att',
],
},
},
},
Resources: {
OVERRIDE_LOGICAL_ID: {
INVALID_LOGICAL_ID: {
Type: 'AWS::Resource',
},
},
Expand Down

0 comments on commit b196b13

Please sign in to comment.