Skip to content

Commit

Permalink
fix(core): throw on intrinsics in CFN update and create policies (#31578
Browse files Browse the repository at this point in the history
)

### Issue # (if applicable)

Closes #27578, Closes #30740.

### Reason for this change

`cfn-include` only allows Intrinsics in resource update and create policies to wrap primitive values. If Intrinsics are included anywhere else, `cfn-include` silently drops them. 

CDK's type system [does not allow](https://github.com/aws/aws-cdk/blob/main/packages/aws-cdk-lib/core/lib/cfn-resource-policy.ts) intrinsics in resource policies unless they define a primitive value. `cfn-include` adheres to this type system and drops any resource policies that use an intrinsic to define a complex value. This is an example of a forbidden use of intrinsics:

```
  "Resources": {
    "ResourceSignalIntrinsic": {
     // ....
      "CreationPolicy": {
        "ResourceSignal": {
          "Fn::If": [
            "UseCountParameter",
            {
              "Count": { "Ref": "CountParameter" }
            },
            5
          ]
        }
      }
    }
  }
}
```

This is forbidden because an intrinsic contains the `Count` property of this policy. CFN allows this, but CDK's type system does not permit it. 

### Description of changes

`cfn-include` will throw if any intrinsics break the type system, instead of silently dropping them.

CDK's type system is a useful constraint around these resource update / create policies because it allows constructs that modify them, like autoscaling, to not be token-aware. Tokens are not resolved at synthesis time, so it makes it impossible to modify these with simple arithmetic if they contain tokens. 

The CDK will never (or at least should not) generate a token that breaks this type system.

Thus, the only use-case for allowing these tokens is `cfn-include`. Supporting these customers would require the CDK type system to allow these, and thus CDK L2s should handle such cases; except, for L2 customers, this use-case does not happen. Explicitly reject templates that don't conform to this. 

Throwing here is a breaking change, so this is under a feature flag. 

Additionally add a new property, `dehydratedResources` -- a list of logical IDs that `cfn-include` will not parse. Those resources still exist in the final template.

This does not impact L2 users.

### Description of how you validated changes

Unit testing. 

Manually verified that this does not impact any L2s. 

### 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
comcalvi authored Oct 3, 2024
1 parent 0e03d39 commit 9410361
Show file tree
Hide file tree
Showing 30 changed files with 1,162 additions and 82 deletions.
40 changes: 39 additions & 1 deletion packages/aws-cdk-lib/cloudformation-include/lib/cfn-include.ts
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,16 @@ export interface CfnIncludeProps {
* @default - will throw an error on detecting any cyclical references
*/
readonly allowCyclicalReferences?: boolean;

/**
* Specifies a list of LogicalIDs for resources that will be included in the CDK Stack,
* but will not be parsed and converted to CDK types. This allows you to use CFN templates
* that rely on Intrinsic placement that `cfn-include`
* would otherwise reject, such as non-primitive values in resource update policies.
*
* @default - All resources are hydrated
*/
readonly dehydratedResources?: string[];
}

/**
Expand Down Expand Up @@ -109,6 +119,7 @@ export class CfnInclude extends core.CfnElement {
private readonly template: any;
private readonly preserveLogicalIds: boolean;
private readonly allowCyclicalReferences: boolean;
private readonly dehydratedResources: string[];
private logicalIdToPlaceholderMap: Map<string, string>;

constructor(scope: Construct, id: string, props: CfnIncludeProps) {
Expand All @@ -125,6 +136,14 @@ export class CfnInclude extends core.CfnElement {

this.preserveLogicalIds = props.preserveLogicalIds ?? true;

this.dehydratedResources = props.dehydratedResources ?? [];

for (const logicalId of this.dehydratedResources) {
if (!Object.keys(this.template.Resources).includes(logicalId)) {
throw new Error(`Logical ID '${logicalId}' was specified in 'dehydratedResources', but does not belong to a resource in the template.`);
}
}

// check if all user specified parameter values exist in the template
for (const logicalId of Object.keys(this.parametersToReplace)) {
if (!(logicalId in (this.template.Parameters || {}))) {
Expand Down Expand Up @@ -663,8 +682,27 @@ export class CfnInclude extends core.CfnElement {

const resourceAttributes: any = this.template.Resources[logicalId];
let l1Instance: core.CfnResource;
if (this.nestedStacksToInclude[logicalId]) {
if (this.nestedStacksToInclude[logicalId] && this.dehydratedResources.includes(logicalId)) {
throw new Error(`nested stack '${logicalId}' was marked as dehydrated - nested stacks cannot be dehydrated`);
} else if (this.nestedStacksToInclude[logicalId]) {
l1Instance = this.createNestedStack(logicalId, cfnParser);
} else if (this.dehydratedResources.includes(logicalId)) {

l1Instance = new core.CfnResource(this, logicalId, {
type: resourceAttributes.Type,
properties: resourceAttributes.Properties,
});
const cfnOptions = l1Instance.cfnOptions;
cfnOptions.creationPolicy = resourceAttributes.CreationPolicy;
cfnOptions.updatePolicy = resourceAttributes.UpdatePolicy;
cfnOptions.deletionPolicy = resourceAttributes.DeletionPolicy;
cfnOptions.updateReplacePolicy = resourceAttributes.UpdateReplacePolicy;
cfnOptions.version = resourceAttributes.Version;
cfnOptions.description = resourceAttributes.Description;
cfnOptions.metadata = resourceAttributes.Metadata;
this.resources[logicalId] = l1Instance;

return l1Instance;
} else {
const l1ClassFqn = cfn_type_to_l1_mapping.lookup(resourceAttributes.Type);
// The AWS::CloudFormation::CustomResource type corresponds to the CfnCustomResource class.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -245,17 +245,201 @@ describe('CDK Include', () => {
},
);
});

test('throws an exception if Tags contains invalid intrinsics', () => {
expect(() => {
includeTestTemplate(stack, 'tags-with-invalid-intrinsics.json');
}).toThrow(/expression does not exist in the template/);
});

test('non-leaf Intrinsics cannot be used in the top-level creation policy', () => {
stack.node.setContext(cxapi.CFN_INCLUDE_REJECT_COMPLEX_RESOURCE_UPDATE_CREATE_POLICY_INTRINSICS, true);
expect(() => {
includeTestTemplate(stack, 'intrinsics-create-policy.json');
}).toThrow(/Cannot convert resource 'CreationPolicyIntrinsic' to CDK objects: it uses an intrinsic in a resource update or deletion policy to represent a non-primitive value. Specify 'CreationPolicyIntrinsic' in the 'dehydratedResources' prop to skip parsing this resource, while still including it in the output./);
});

test('Intrinsics cannot be used in the autoscaling creation policy', () => {
stack.node.setContext(cxapi.CFN_INCLUDE_REJECT_COMPLEX_RESOURCE_UPDATE_CREATE_POLICY_INTRINSICS, true);
expect(() => {
includeTestTemplate(stack, 'intrinsics-create-policy-autoscaling.json');
}).toThrow(/Cannot convert resource 'AutoScalingCreationPolicyIntrinsic' to CDK objects: it uses an intrinsic in a resource update or deletion policy to represent a non-primitive value. Specify 'AutoScalingCreationPolicyIntrinsic' in the 'dehydratedResources' prop to skip parsing this resource, while still including it in the output./);
});

test('Intrinsics cannot be used in the create policy resource signal', () => {
stack.node.setContext(cxapi.CFN_INCLUDE_REJECT_COMPLEX_RESOURCE_UPDATE_CREATE_POLICY_INTRINSICS, true);
expect(() => {
includeTestTemplate(stack, 'intrinsics-create-policy-resource-signal.json');
}).toThrow(/Cannot convert resource 'ResourceSignalIntrinsic' to CDK objects: it uses an intrinsic in a resource update or deletion policy to represent a non-primitive value. Specify 'ResourceSignalIntrinsic' in the 'dehydratedResources' prop to skip parsing this resource, while still including it in the output./);
});

test('Intrinsics cannot be used in the top-level update policy', () => {
stack.node.setContext(cxapi.CFN_INCLUDE_REJECT_COMPLEX_RESOURCE_UPDATE_CREATE_POLICY_INTRINSICS, true);
expect(() => {
includeTestTemplate(stack, 'intrinsics-update-policy.json');
}).toThrow(/Cannot convert resource 'ASG' to CDK objects: it uses an intrinsic in a resource update or deletion policy to represent a non-primitive value. Specify 'ASG' in the 'dehydratedResources' prop to skip parsing this resource, while still including it in the output./);
});

test('Intrinsics cannot be used in the auto scaling rolling update update policy', () => {
stack.node.setContext(cxapi.CFN_INCLUDE_REJECT_COMPLEX_RESOURCE_UPDATE_CREATE_POLICY_INTRINSICS, true);
expect(() => {
includeTestTemplate(stack, 'intrinsics-update-policy-autoscaling-rolling-update.json');
}).toThrow(/Cannot convert resource 'ASG' to CDK objects: it uses an intrinsic in a resource update or deletion policy to represent a non-primitive value. Specify 'ASG' in the 'dehydratedResources' prop to skip parsing this resource, while still including it in the output./);
});

test('Intrinsics cannot be used in the auto scaling replacing update update policy', () => {
stack.node.setContext(cxapi.CFN_INCLUDE_REJECT_COMPLEX_RESOURCE_UPDATE_CREATE_POLICY_INTRINSICS, true);
expect(() => {
includeTestTemplate(stack, 'intrinsics-update-policy-autoscaling-replacing-update.json');
}).toThrow(/Cannot convert resource 'ASG' to CDK objects: it uses an intrinsic in a resource update or deletion policy to represent a non-primitive value. Specify 'ASG' in the 'dehydratedResources' prop to skip parsing this resource, while still including it in the output./);
});

test('Intrinsics cannot be used in the auto scaling scheduled action update policy', () => {
stack.node.setContext(cxapi.CFN_INCLUDE_REJECT_COMPLEX_RESOURCE_UPDATE_CREATE_POLICY_INTRINSICS, true);
expect(() => {
includeTestTemplate(stack, 'intrinsics-update-policy-autoscaling-scheduled-action.json');
}).toThrow(/Cannot convert resource 'ASG' to CDK objects: it uses an intrinsic in a resource update or deletion policy to represent a non-primitive value. Specify 'ASG' in the 'dehydratedResources' prop to skip parsing this resource, while still including it in the output./);
});

test('Intrinsics cannot be used in the code deploy lambda alias update policy', () => {
stack.node.setContext(cxapi.CFN_INCLUDE_REJECT_COMPLEX_RESOURCE_UPDATE_CREATE_POLICY_INTRINSICS, true);
expect(() => {
includeTestTemplate(stack, 'intrinsics-update-policy-code-deploy-lambda-alias-update.json');
}).toThrow(/Cannot convert resource 'Alias' to CDK objects: it uses an intrinsic in a resource update or deletion policy to represent a non-primitive value. Specify 'Alias' in the 'dehydratedResources' prop to skip parsing this resource, while still including it in the output./);
});

test('FF toggles error checking', () => {
stack.node.setContext(cxapi.CFN_INCLUDE_REJECT_COMPLEX_RESOURCE_UPDATE_CREATE_POLICY_INTRINSICS, false);
expect(() => {
includeTestTemplate(stack, 'intrinsics-update-policy-code-deploy-lambda-alias-update.json');
}).not.toThrow();
});

test('FF disabled with dehydratedResources does not throw', () => {
stack.node.setContext(cxapi.CFN_INCLUDE_REJECT_COMPLEX_RESOURCE_UPDATE_CREATE_POLICY_INTRINSICS, false);
expect(() => {
includeTestTemplate(stack, 'intrinsics-update-policy-code-deploy-lambda-alias-update.json', {
dehydratedResources: ['Alias'],
});
}).not.toThrow();
});

test('dehydrated resources retain attributes with complex Intrinsics', () => {
stack.node.setContext(cxapi.CFN_INCLUDE_REJECT_COMPLEX_RESOURCE_UPDATE_CREATE_POLICY_INTRINSICS, true);
includeTestTemplate(stack, 'intrinsics-update-policy-code-deploy-lambda-alias-update.json', {
dehydratedResources: ['Alias'],
});

expect(Template.fromStack(stack).hasResource('AWS::Lambda::Alias', {
UpdatePolicy: {
CodeDeployLambdaAliasUpdate: {
'Fn::If': [
'SomeCondition',
{
AfterAllowTrafficHook: 'SomeOtherHook',
ApplicationName: 'SomeApp',
BeforeAllowTrafficHook: 'SomeHook',
DeploymentGroupName: 'SomeDeploymentGroup',
},
{
AfterAllowTrafficHook: 'SomeOtherOtherHook',
ApplicationName: 'SomeOtherApp',
BeforeAllowTrafficHook: 'SomeOtherHook',
DeploymentGroupName: 'SomeOtherDeploymentGroup',

},
],
},
},
}));
});

test('dehydrated resources retain all attributes', () => {
includeTestTemplate(stack, 'resource-all-attributes.json', {
dehydratedResources: ['Foo'],
});

expect(Template.fromStack(stack).hasResource('AWS::Foo::Bar', {
Properties: { Blinky: 'Pinky' },
Type: 'AWS::Foo::Bar',
CreationPolicy: { Inky: 'Clyde' },
DeletionPolicy: { DeletionPolicyKey: 'DeletionPolicyValue' },
Metadata: { SomeKey: 'SomeValue' },
Version: '1.2.3.4.5.6',
UpdateReplacePolicy: { Oh: 'No' },
Description: 'This resource does not match the spec, but it does have every possible attribute',
UpdatePolicy: {
Foo: 'Bar',
},
}));
});

test('synth-time validation does not run on dehydrated resources', () => {
// synth-time validation fails if resource is hydrated
expect(() => {
includeTestTemplate(stack, 'intrinsics-tags-resource-validation.json');
Template.fromStack(stack);
}).toThrow(`Resolution error: Supplied properties not correct for \"CfnLoadBalancerProps\"
tags: element 1: {} should have a 'key' and a 'value' property.`);

app = new core.App({ context: { [cxapi.NEW_STYLE_STACK_SYNTHESIS_CONTEXT]: false } });
stack = new core.Stack(app);

// synth-time validation not run if resource is dehydrated
includeTestTemplate(stack, 'intrinsics-tags-resource-validation.json', {
dehydratedResources: ['MyLoadBalancer'],
});

expect(Template.fromStack(stack).hasResource('AWS::ElasticLoadBalancingV2::LoadBalancer', {
Properties: {
Tags: [
{
Key: 'Name',
Value: 'MyLoadBalancer',
},
{
data: [
'IsExtraTag',
{
Key: 'Name2',
Value: 'MyLoadBalancer2',
},
{
data: 'AWS::NoValue',
type: 'Ref',
isCfnFunction: true,
},
],
type: 'Fn::If',
isCfnFunction: true,
},
],
},
}));
});

test('throws on dehydrated resources not present in the template', () => {
expect(() => {
includeTestTemplate(stack, 'intrinsics-tags-resource-validation.json', {
dehydratedResources: ['ResourceNotExistingHere'],
});
}).toThrow(/Logical ID 'ResourceNotExistingHere' was specified in 'dehydratedResources', but does not belong to a resource in the template./);
});
});

interface IncludeTestTemplateProps {
/** @default false */
readonly allowCyclicalReferences?: boolean;

/** @default none */
readonly dehydratedResources?: string[];
}

function includeTestTemplate(scope: constructs.Construct, testTemplate: string, props: IncludeTestTemplateProps = {}): inc.CfnInclude {
return new inc.CfnInclude(scope, 'MyScope', {
templateFile: _testTemplateFilePath(testTemplate),
allowCyclicalReferences: props.allowCyclicalReferences,
dehydratedResources: props.dehydratedResources,
});
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -743,6 +743,77 @@ describe('CDK Include for nested stacks', () => {
});
});
});

describe('dehydrated resources', () => {
let parentStack: core.Stack;
let childStack: core.Stack;

beforeEach(() => {
parentStack = new core.Stack();
});

test('dehydrated resources are included in child templates, even if they are otherwise invalid', () => {
const parentTemplate = new inc.CfnInclude(parentStack, 'ParentStack', {
templateFile: testTemplateFilePath('parent-dehydrated.json'),
dehydratedResources: ['ASG'],
loadNestedStacks: {
'ChildStack': {
templateFile: testTemplateFilePath('child-dehydrated.json'),
dehydratedResources: ['ChildASG'],
},
},
});
childStack = parentTemplate.getNestedStack('ChildStack').stack;

Template.fromStack(childStack).templateMatches({
"Conditions": {
"SomeCondition": {
"Fn::Equals": [
2,
2,
],
},
},
"Resources": {
"ChildStackChildASGF815DFE9": {
"Type": "AWS::AutoScaling::AutoScalingGroup",
"Properties": {
"MaxSize": 10,
"MinSize": 1,
},
"UpdatePolicy": {
"AutoScalingScheduledAction": {
"Fn::If": [
"SomeCondition",
{
"IgnoreUnmodifiedGroupSizeProperties": true,
},
{
"IgnoreUnmodifiedGroupSizeProperties": false,
},
],
},
},
},
},
});
});

test('throws if a nested stack is marked dehydrated', () => {
expect(() => {
new inc.CfnInclude(parentStack, 'ParentStack', {
templateFile: testTemplateFilePath('parent-dehydrated.json'),
dehydratedResources: ['ChildStack'],
loadNestedStacks: {
'ChildStack': {
templateFile: testTemplateFilePath('child-dehydrated.json'),
dehydratedResources: ['ChildASG'],
},
},
});
}).toThrow(/nested stack 'ChildStack' was marked as dehydrated - nested stacks cannot be dehydrated/);
});
});
});

function loadTestFileToJsObject(testTemplate: string): any {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
{
"Parameters": {
"MinSuccessfulInstancesPercent": {
"Type": "Number"
}
},
"Resources": {
"AutoScalingCreationPolicyIntrinsic": {
"Type": "AWS::AutoScaling::AutoScalingGroup",
"Properties": {
"MinSize": "1",
"MaxSize": "5"
},
"CreationPolicy": {
"AutoScalingCreationPolicy": {
"MinSuccessfulInstancesPercent": {
"Ref": "MinSuccessfulInstancesPercent"
}
}
}
}
}
}
Loading

0 comments on commit 9410361

Please sign in to comment.