From e8d077628e28ee055ac222d54c5cb4546ab82be3 Mon Sep 17 00:00:00 2001 From: Adam Ruka Date: Fri, 17 Jul 2020 15:38:02 -0700 Subject: [PATCH] fix(cfn-include): fix issues in Conditions handling (#9142) The current Conditions handling in cloudformation-include has a few flaws, that are all corrected in this change: - Do not fail when a Condition and a Resource share the same logical ID - Handle changing the logical IDs of Conditions used in Fn::If expressions - Handle changing the logical IDs of Conditions referenced inside the Conditions section - Do not fail when referring to a Parameter from inside a Condition ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* --- .../cloudformation-include/lib/cfn-include.ts | 40 ++++++++---- .../test/invalid-templates.test.ts | 18 +++++ .../condition-same-name-as-resource.json | 28 ++++++++ .../invalid/getatt-in-conditions.json | 15 +++++ .../non-existent-condition-in-conditions.json | 7 ++ .../invalid/non-existent-condition-in-if.json | 15 +++++ .../test/valid-templates.test.ts | 35 ++++++++++ packages/@aws-cdk/core/lib/cfn-parse.ts | 65 ++++++++++++++----- 8 files changed, 195 insertions(+), 28 deletions(-) create mode 100644 packages/@aws-cdk/cloudformation-include/test/test-templates/condition-same-name-as-resource.json create mode 100644 packages/@aws-cdk/cloudformation-include/test/test-templates/invalid/getatt-in-conditions.json create mode 100644 packages/@aws-cdk/cloudformation-include/test/test-templates/invalid/non-existent-condition-in-conditions.json create mode 100644 packages/@aws-cdk/cloudformation-include/test/test-templates/invalid/non-existent-condition-in-if.json diff --git a/packages/@aws-cdk/cloudformation-include/lib/cfn-include.ts b/packages/@aws-cdk/cloudformation-include/lib/cfn-include.ts index 7be10e373ae45..d8c6abd702448 100644 --- a/packages/@aws-cdk/cloudformation-include/lib/cfn-include.ts +++ b/packages/@aws-cdk/cloudformation-include/lib/cfn-include.ts @@ -52,6 +52,7 @@ export interface IncludedNestedStack { */ export class CfnInclude extends core.CfnElement { private readonly conditions: { [conditionName: string]: core.CfnCondition } = {}; + private readonly conditionsScope: core.Construct; private readonly resources: { [logicalId: string]: core.CfnResource } = {}; private readonly parameters: { [logicalId: string]: core.CfnParameter } = {}; private readonly outputs: { [logicalId: string]: core.CfnOutput } = {}; @@ -75,8 +76,9 @@ export class CfnInclude extends core.CfnElement { } // instantiate the conditions + this.conditionsScope = new core.Construct(this, '$Conditions'); for (const conditionName of Object.keys(this.template.Conditions || {})) { - this.createCondition(conditionName); + this.getOrCreateCondition(conditionName); } this.nestedStacksToInclude = props.nestedStacks || {}; @@ -262,8 +264,8 @@ export class CfnInclude extends core.CfnElement { return self.getCondition(outputAttributes.Condition); } - throw new Error(`Output with name '${logicalId}' refers to a Condition with name\ - '${outputAttributes.Condition}' which was not found in this template`); + throw new Error(`Output with name '${logicalId}' refers to a Condition with name ` + + `'${outputAttributes.Condition}' which was not found in this template`); })(), }); @@ -271,23 +273,35 @@ export class CfnInclude extends core.CfnElement { this.outputs[logicalId] = cfnOutput; } - private createCondition(conditionName: string): void { - // ToDo condition expressions can refer to other conditions - - // will be important when implementing preserveLogicalIds=false - const expression = new cfn_parse.CfnParser({ + private getOrCreateCondition(conditionName: string): core.CfnCondition { + if (conditionName in this.conditions) { + return this.conditions[conditionName]; + } + + const self = this; + const cfnParser = new cfn_parse.CfnParser({ finder: { findResource() { throw new Error('Using GetAtt in Condition definitions is not allowed'); }, - findRefTarget() { throw new Error('Using Ref expressions in Condition definitions is not allowed'); }, - // ToDo handle one Condition referencing another using the { Condition: "ConditionName" } syntax - findCondition() { return undefined; }, + findRefTarget(elementName: string): core.CfnElement | undefined { + // only Parameters can be referenced in the 'Conditions' section + return self.parameters[elementName]; + }, + findCondition(cName: string): core.CfnCondition | undefined { + return cName in (self.template.Conditions || {}) + ? self.getOrCreateCondition(cName) + : undefined; + }, }, - }).parseValue(this.template.Conditions[conditionName]); - const cfnCondition = new core.CfnCondition(this, conditionName, { - expression, + context: cfn_parse.CfnParsingContext.CONDITIONS, }); + const cfnCondition = new core.CfnCondition(this.conditionsScope, conditionName, { + expression: cfnParser.parseValue(this.template.Conditions[conditionName]), + }); + // ToDo handle renaming of the logical IDs of the conditions cfnCondition.overrideLogicalId(conditionName); this.conditions[conditionName] = cfnCondition; + return cfnCondition; } private getOrCreateResource(logicalId: string): core.CfnResource { diff --git a/packages/@aws-cdk/cloudformation-include/test/invalid-templates.test.ts b/packages/@aws-cdk/cloudformation-include/test/invalid-templates.test.ts index 765b0a7de265e..ef93cd9809d82 100644 --- a/packages/@aws-cdk/cloudformation-include/test/invalid-templates.test.ts +++ b/packages/@aws-cdk/cloudformation-include/test/invalid-templates.test.ts @@ -47,12 +47,30 @@ describe('CDK Include', () => { }).toThrow(/Resource 'Bucket2' depends on 'Bucket1' that doesn't exist/); }); + test("throws a validation exception for a template referencing a Condition in the Conditions section that doesn't exist", () => { + expect(() => { + includeTestTemplate(stack, 'non-existent-condition-in-conditions.json'); + }).toThrow(/Referenced Condition with name 'AlwaysFalse' was not found in the template/); + }); + + test('throws a validation exception for a template using Fn::GetAtt in the Conditions section', () => { + expect(() => { + includeTestTemplate(stack, 'getatt-in-conditions.json'); + }).toThrow(/Using GetAtt in Condition definitions is not allowed/); + }); + test("throws a validation exception for a template referencing a Condition resource attribute that doesn't exist", () => { expect(() => { includeTestTemplate(stack, 'non-existent-condition.json'); }).toThrow(/Resource 'Bucket' uses Condition 'AlwaysFalseCond' that doesn't exist/); }); + test("throws a validation exception for a template referencing a Condition in an If expression that doesn't exist", () => { + expect(() => { + includeTestTemplate(stack, 'non-existent-condition-in-if.json'); + }).toThrow(/Condition 'AlwaysFalse' used in an Fn::If expression does not exist in the template/); + }); + test("throws an exception when encountering a CFN function it doesn't support", () => { expect(() => { includeTestTemplate(stack, 'only-codecommit-repo-using-cfn-functions.json'); diff --git a/packages/@aws-cdk/cloudformation-include/test/test-templates/condition-same-name-as-resource.json b/packages/@aws-cdk/cloudformation-include/test/test-templates/condition-same-name-as-resource.json new file mode 100644 index 0000000000000..d7220fd1df1ba --- /dev/null +++ b/packages/@aws-cdk/cloudformation-include/test/test-templates/condition-same-name-as-resource.json @@ -0,0 +1,28 @@ +{ + "Parameters": { + "Param": { + "Type": "String" + } + }, + "Conditions": { + "AlwaysTrue": { + "Fn::Not": [{ "Condition": "AlwaysFalse" }] + }, + "AlwaysFalse": { + "Fn::Equals": [{ "Ref": "Param" }, 2] + } + }, + "Resources": { + "AlwaysTrue": { + "Type": "AWS::S3::Bucket", + "Properties": { + "BucketName": { + "Fn::If": ["AlwaysFalse", + { "Ref": "Param" }, + { "Ref": "AWS::NoValue" } + ] + } + } + } + } +} diff --git a/packages/@aws-cdk/cloudformation-include/test/test-templates/invalid/getatt-in-conditions.json b/packages/@aws-cdk/cloudformation-include/test/test-templates/invalid/getatt-in-conditions.json new file mode 100644 index 0000000000000..31b818da764ed --- /dev/null +++ b/packages/@aws-cdk/cloudformation-include/test/test-templates/invalid/getatt-in-conditions.json @@ -0,0 +1,15 @@ +{ + "Conditions": { + "MyCond": { + "Fn::Equals": [ + { "Fn::GetAtt": ["Bucket", "Arn"] }, + "my-arn" + ] + } + }, + "Resources": { + "Bucket": { + "Type": "AWS::S3::Bucket" + } + } +} diff --git a/packages/@aws-cdk/cloudformation-include/test/test-templates/invalid/non-existent-condition-in-conditions.json b/packages/@aws-cdk/cloudformation-include/test/test-templates/invalid/non-existent-condition-in-conditions.json new file mode 100644 index 0000000000000..c35e3922b357a --- /dev/null +++ b/packages/@aws-cdk/cloudformation-include/test/test-templates/invalid/non-existent-condition-in-conditions.json @@ -0,0 +1,7 @@ +{ + "Conditions": { + "AlwaysTrue": { + "Fn::Not": [{ "Condition": "AlwaysFalse" }] + } + } +} diff --git a/packages/@aws-cdk/cloudformation-include/test/test-templates/invalid/non-existent-condition-in-if.json b/packages/@aws-cdk/cloudformation-include/test/test-templates/invalid/non-existent-condition-in-if.json new file mode 100644 index 0000000000000..fb35fb6d8bff9 --- /dev/null +++ b/packages/@aws-cdk/cloudformation-include/test/test-templates/invalid/non-existent-condition-in-if.json @@ -0,0 +1,15 @@ +{ + "Resources": { + "Bucket": { + "Type": "AWS::S3::Bucket", + "Properties": { + "BucketName": { + "Fn::If": ["AlwaysFalse", + "ThenName", + "ElseName" + ] + } + } + } + } +} diff --git a/packages/@aws-cdk/cloudformation-include/test/valid-templates.test.ts b/packages/@aws-cdk/cloudformation-include/test/valid-templates.test.ts index 83b2370db5fac..751708edadb16 100644 --- a/packages/@aws-cdk/cloudformation-include/test/valid-templates.test.ts +++ b/packages/@aws-cdk/cloudformation-include/test/valid-templates.test.ts @@ -251,6 +251,41 @@ describe('CDK Include', () => { ); }); + test('correctly change references to Conditions when renaming them', () => { + const cfnTemplate = includeTestTemplate(stack, 'condition-same-name-as-resource.json'); + const alwaysFalse = cfnTemplate.getCondition('AlwaysFalse'); + alwaysFalse.overrideLogicalId('TotallyFalse'); + + expect(stack).toMatchTemplate({ + "Parameters": { + "Param": { + "Type": "String", + }, + }, + "Conditions": { + "AlwaysTrue": { + "Fn::Not": [{ "Condition": "TotallyFalse" }], + }, + "TotallyFalse": { + "Fn::Equals": [{ "Ref": "Param" }, 2], + }, + }, + "Resources": { + "AlwaysTrue": { + "Type": "AWS::S3::Bucket", + "Properties": { + "BucketName": { + "Fn::If": ["TotallyFalse", + { "Ref": "Param" }, + { "Ref": "AWS::NoValue" }, + ], + }, + }, + }, + }, + }); + }); + test('correctly parses templates with parameters', () => { const cfnTemplate = includeTestTemplate(stack, 'bucket-with-parameters.json'); const param = cfnTemplate.getParameter('BucketName'); diff --git a/packages/@aws-cdk/core/lib/cfn-parse.ts b/packages/@aws-cdk/core/lib/cfn-parse.ts index 5aae33709a406..756aae374b42a 100644 --- a/packages/@aws-cdk/core/lib/cfn-parse.ts +++ b/packages/@aws-cdk/core/lib/cfn-parse.ts @@ -118,6 +118,20 @@ export class FromCloudFormation { } } +/** + * The context in which the parsing is taking place. + * + * Some fragments of CloudFormation templates behave differently than others + * (for example, the 'Conditions' sections treats { "Condition": "NameOfCond" } + * differently than the 'Resources' section). + * This enum can be used to change the created {@link CfnParser} behavior, + * based on the template context. + */ +export enum CfnParsingContext { + /** We're currently parsing the 'Conditions' section. */ + CONDITIONS, +} + /** * The options for {@link FromCloudFormation.parseValue}. */ @@ -126,6 +140,13 @@ export interface ParseCfnOptions { * The finder interface used to resolve references in the template. */ readonly finder: ICfnFinder; + + /** + * The context we're parsing the template in. + * + * @default - the default context (no special behavior) + */ + readonly context?: CfnParsingContext; } /** @@ -274,7 +295,7 @@ export class CfnParser { } private parseIfCfnIntrinsic(object: any): any { - const key = looksLikeCfnIntrinsic(object); + const key = this.looksLikeCfnIntrinsic(object); switch (key) { case undefined: return undefined; @@ -340,12 +361,14 @@ export class CfnParser { return Fn.base64(value); } case 'Fn::If': { - // Fn::If takes a 3-element list as its argument - // ToDo the first argument is the name of the condition, - // so we will need to retrieve the actual object from the template - // when we handle preserveLogicalIds=false + // Fn::If takes a 3-element list as its argument, + // where the first element is the name of a Condition const value = this.parseValue(object[key]); - return Fn.conditionIf(value[0], value[1], value[2]); + const condition = this.options.finder.findCondition(value[0]); + if (!condition) { + throw new Error(`Condition '${value[0]}' used in an Fn::If expression does not exist in the template`); + } + return Fn.conditionIf(condition.logicalId, value[1], value[2]); } case 'Fn::Equals': { const value = this.parseValue(object[key]); @@ -363,21 +386,33 @@ export class CfnParser { const value = this.parseValue(object[key]); return Fn.conditionOr(...value); } + case 'Condition': { + // a reference to a Condition from another Condition + const condition = this.options.finder.findCondition(object[key]); + if (!condition) { + throw new Error(`Referenced Condition with name '${object[key]}' was not found in the template`); + } + return { Condition: condition.logicalId }; + } default: throw new Error(`Unsupported CloudFormation function '${key}'`); } } -} -function looksLikeCfnIntrinsic(object: object): string | undefined { - const objectKeys = Object.keys(object); - // a CFN intrinsic is always an object with a single key - if (objectKeys.length !== 1) { - return undefined; - } + private looksLikeCfnIntrinsic(object: object): string | undefined { + const objectKeys = Object.keys(object); + // a CFN intrinsic is always an object with a single key + if (objectKeys.length !== 1) { + return undefined; + } - const key = objectKeys[0]; - return key === 'Ref' || key.startsWith('Fn::') ? key : undefined; + const key = objectKeys[0]; + return key === 'Ref' || key.startsWith('Fn::') || + // special intrinsic only available in the 'Conditions' section + (this.options.context === CfnParsingContext.CONDITIONS && key === 'Condition') + ? key + : undefined; + } } function specialCaseRefs(value: any): any {