fix(cfn-include): Fn::GetAtt with a string argument fails to include (#…

…10546)

As it turns out, `Fn::GetAtt` can be passed a string argument not only in YAML,
but in JSON CloudFormation templates as well.
Handle that case in our template parser for `cfn-include`.

This handling allows us to stop special-casing transforming the short-form
`!GetAtt` in our YAML parsing.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

packages/@aws-cdk/cloudformation-include/test/test-templates/get-att-string-form.json

@@ -0,0 +1,15 @@
+{
+  "Resources": {
+    "Bucket1": {
+      "Type": "AWS::S3::Bucket"
+    },
+    "Bucket2": {
+      "Type": "AWS::S3::Bucket",
+      "Metadata": {
+        "Bucket1Arn": {
+          "Fn::GetAtt": "Bucket1.Arn"
+        }
+      }
+    }
+  }
+}

packages/@aws-cdk/cloudformation-include/test/valid-templates.test.ts

@@ -232,6 +232,14 @@ describe('CDK Include', () => {
     );
   });
 
+  test('can ingest a JSON template with string-form Fn::GetAtt, and output it unchanged', () => {
+    includeTestTemplate(stack, 'get-att-string-form.json');
+
+    expect(stack).toMatchTemplate(
+      loadTestFileToJsObject('get-att-string-form.json'),
+    );
+  });
+
   test('can ingest a template with Fn::Sub in string form with escaped and unescaped references and output it unchanged', () => {
     includeTestTemplate(stack, 'fn-sub-string.json');
 

packages/@aws-cdk/cloudformation-include/test/yaml-templates.test.ts

@@ -140,15 +140,15 @@ describe('CDK Include', () => {
         "Bucket1": {
           "Type": "AWS::S3::Bucket",
           "Properties": {
-            "BucketName": { "Fn::GetAtt": ["Bucket0", "Arn"] },
+            "BucketName": { "Fn::GetAtt": "Bucket0.Arn" },
             "AccessControl": { "Fn::GetAtt": ["ELB", "SourceSecurityGroup.GroupName"] },
           },
         },
         "Bucket2": {
           "Type": "AWS::S3::Bucket",
           "Properties": {
             "BucketName": { "Fn::GetAtt": ["Bucket1", "Arn"] },
-            "AccessControl": { "Fn::GetAtt": ["ELB", "SourceSecurityGroup.GroupName"] },
+            "AccessControl": { "Fn::GetAtt": "ELB.SourceSecurityGroup.GroupName" },
           },
         },
       },

packages/@aws-cdk/core/lib/cfn-parse.ts

@@ -10,7 +10,7 @@ import {
 } from './cfn-resource-policy';
 import { CfnTag } from './cfn-tag';
 import { Lazy } from './lazy';
-import { CfnReference } from './private/cfn-reference';
+import { CfnReference, ReferenceRendering } from './private/cfn-reference';
 import { IResolvable } from './resolvable';
 import { Mapper, Validator } from './runtime';
 import { isResolvableObject, Token } from './token';
@@ -457,13 +457,29 @@ export class CfnParser {
         }
       }
       case 'Fn::GetAtt': {
-        // Fn::GetAtt takes a 2-element list as its argument
         const value = object[key];
-        const target = this.finder.findResource(value[0]);
+        let logicalId: string, attributeName: string, stringForm: boolean;
+        // Fn::GetAtt takes as arguments either a string...
+        if (typeof value === 'string') {
+          // ...in which case the logical ID and the attribute name are separated with '.'
+          const dotIndex = value.indexOf('.');
+          if (dotIndex === -1) {
+            throw new Error(`Short-form Fn::GetAtt must contain a '.' in its string argument, got: '${value}'`);
+          }
+          logicalId = value.substr(0, dotIndex);
+          attributeName = value.substr(dotIndex + 1); // the +1 is to skip the actual '.'
+          stringForm = true;
+        } else {
+          // ...or a 2-element list
+          logicalId = value[0];
+          attributeName = value[1];
+          stringForm = false;
+        }
+        const target = this.finder.findResource(logicalId);
         if (!target) {
-          throw new Error(`Resource used in GetAtt expression with logical ID: '${value[0]}' not found`);
+          throw new Error(`Resource used in GetAtt expression with logical ID: '${logicalId}' not found`);
         }
-        return target.getAtt(value[1]);
+        return CfnReference.for(target, attributeName, stringForm ? ReferenceRendering.GET_ATT_STRING : undefined);
       }
       case 'Fn::Join': {
         // Fn::Join takes a 2-element list as its argument,
@@ -618,15 +634,15 @@ export class CfnParser {
       if (!refElement) {
         throw new Error(`Element referenced in Fn::Sub expression with logical ID: '${refTarget}' was not found in the template`);
       }
-      return leftHalf + CfnReference.for(refElement, 'Ref', true).toString() + this.parseFnSubString(rightHalf, map);
+      return leftHalf + CfnReference.for(refElement, 'Ref', ReferenceRendering.FN_SUB).toString() + this.parseFnSubString(rightHalf, map);
     } else {
       const targetId = refTarget.substring(0, dotIndex);
       const refResource = this.finder.findResource(targetId);
       if (!refResource) {
         throw new Error(`Resource referenced in Fn::Sub expression with logical ID: '${targetId}' was not found in the template`);
       }
       const attribute = refTarget.substring(dotIndex + 1);
-      return leftHalf + CfnReference.for(refResource, attribute, true).toString() + this.parseFnSubString(rightHalf, map);
+      return leftHalf + CfnReference.for(refResource, attribute, ReferenceRendering.FN_SUB).toString() + this.parseFnSubString(rightHalf, map);
     }
   }
 

packages/@aws-cdk/core/lib/private/cfn-reference.ts

@@ -2,6 +2,24 @@ import { Reference } from '../reference';
 
 const CFN_REFERENCE_SYMBOL = Symbol.for('@aws-cdk/core.CfnReference');
 
+/**
+ * An enum that allows controlling how will the created reference
+ * be rendered in the resulting CloudFormation template.
+ */
+export enum ReferenceRendering {
+  /**
+   * Used for rendering a reference inside Fn::Sub expressions,
+   * which mean these must resolve to "${Sth}" instead of { Ref: "Sth" }.
+   */
+  FN_SUB,
+
+  /**
+   * Used for rendering Fn::GetAtt with its arguments in string form
+   * (as opposed to the more common arguments in array form, which we render by default).
+   */
+  GET_ATT_STRING,
+}
+
 /**
  * A Token that represents a CloudFormation reference to another resource
  *
@@ -34,14 +52,19 @@ export class CfnReference extends Reference {
    *
    *     Lazy.stringValue({ produce: () => new CfnReference(...) })
    *
-   * If fnSub is true, then this reference will resolve as ${logicalID}.
-   * This allows cloudformation-include to correctly handle Fn::Sub.
    */
-  public static for(target: CfnElement, attribute: string, fnSub: boolean = false) {
-    return CfnReference.singletonReference(target, attribute, fnSub, () => {
-      const cfnIntrinsic = fnSub
+  public static for(target: CfnElement, attribute: string, refRender?: ReferenceRendering) {
+    return CfnReference.singletonReference(target, attribute, refRender, () => {
+      const cfnIntrinsic = refRender === ReferenceRendering.FN_SUB
         ? ('${' + target.logicalId + (attribute === 'Ref' ? '' : `.${attribute}`) + '}')
-        : (attribute === 'Ref' ? { Ref: target.logicalId } : { 'Fn::GetAtt': [target.logicalId, attribute] });
+        : (attribute === 'Ref'
+          ? { Ref: target.logicalId }
+          : {
+            'Fn::GetAtt': refRender === ReferenceRendering.GET_ATT_STRING
+              ? `${target.logicalId}.${attribute}`
+              : [target.logicalId, attribute],
+          }
+        );
       return new CfnReference(cfnIntrinsic, attribute, target);
     });
   }
@@ -50,7 +73,7 @@ export class CfnReference extends Reference {
    * Return a CfnReference that references a pseudo referencd
    */
   public static forPseudo(pseudoName: string, scope: Construct) {
-    return CfnReference.singletonReference(scope, `Pseudo:${pseudoName}`, false, () => {
+    return CfnReference.singletonReference(scope, `Pseudo:${pseudoName}`, undefined, () => {
       const cfnIntrinsic = { Ref: pseudoName };
       return new CfnReference(cfnIntrinsic, pseudoName, scope);
     });
@@ -65,13 +88,21 @@ export class CfnReference extends Reference {
    * Get or create the table.
    * Passing fnSub = true allows cloudformation-include to correctly handle Fn::Sub.
    */
-  private static singletonReference(target: Construct, attribKey: string, fnSub: boolean, fresh: () => CfnReference) {
+  private static singletonReference(target: Construct, attribKey: string, refRender: ReferenceRendering | undefined, fresh: () => CfnReference) {
     let attribs = CfnReference.referenceTable.get(target);
     if (!attribs) {
       attribs = new Map();
       CfnReference.referenceTable.set(target, attribs);
     }
-    const cacheKey = attribKey + (fnSub ? 'Fn::Sub' : '');
+    let cacheKey = attribKey;
+    switch (refRender) {
+      case ReferenceRendering.FN_SUB:
+        cacheKey += 'Fn::Sub';
+        break;
+      case ReferenceRendering.GET_ATT_STRING:
+        cacheKey += 'Fn::GetAtt::String';
+        break;
+    }
     let ref = attribs.get(cacheKey);
     if (!ref) {
       ref = fresh();

packages/@aws-cdk/yaml-cfn/lib/yaml.ts

@@ -29,54 +29,26 @@ export function deserialize(str: string): any {
   return parseYamlStrWithCfnTags(str);
 }
 
-function makeTagForCfnIntrinsic(
-  intrinsicName: string, addFnPrefix: boolean = true,
-  resolveFun?: (_doc: yaml.Document, cstNode: yaml_cst.CST.Node) => any): yaml_types.Schema.CustomTag {
-
+function makeTagForCfnIntrinsic(intrinsicName: string, addFnPrefix: boolean): yaml_types.Schema.CustomTag {
   return {
     identify(value: any) { return typeof value === 'string'; },
     tag: `!${intrinsicName}`,
-    resolve: resolveFun || ((_doc: yaml.Document, cstNode: yaml_cst.CST.Node) => {
+    resolve: (_doc: yaml.Document, cstNode: yaml_cst.CST.Node) => {
       const ret: any = {};
       ret[addFnPrefix ? `Fn::${intrinsicName}` : intrinsicName] =
         // the +1 is to account for the ! the short form begins with
         parseYamlStrWithCfnTags(cstNode.toString().substring(intrinsicName.length + 1));
       return ret;
-    }),
+    },
   };
 }
 
 const shortForms: yaml_types.Schema.CustomTag[] = [
   'Base64', 'Cidr', 'FindInMap', 'GetAZs', 'ImportValue', 'Join', 'Sub',
-  'Select', 'Split', 'Transform', 'And', 'Equals', 'If', 'Not', 'Or',
-].map(name => makeTagForCfnIntrinsic(name)).concat(
+  'Select', 'Split', 'Transform', 'And', 'Equals', 'If', 'Not', 'Or', 'GetAtt',
+].map(name => makeTagForCfnIntrinsic(name, true)).concat(
   makeTagForCfnIntrinsic('Ref', false),
   makeTagForCfnIntrinsic('Condition', false),
-  makeTagForCfnIntrinsic('GetAtt', true, (_doc: yaml.Document, cstNode: yaml_cst.CST.Node): any => {
-    const parsedArguments = parseYamlStrWithCfnTags(cstNode.toString().substring('!GetAtt'.length));
-
-    let value: any;
-    if (typeof parsedArguments === 'string') {
-      // if the arguments to !GetAtt are a string,
-      // the part before the first '.' is the logical ID,
-      // and the rest is the attribute name
-      // (which can contain '.')
-      const firstDot = parsedArguments.indexOf('.');
-      if (firstDot === -1) {
-        throw new Error(`Short-form Fn::GetAtt must contain a '.' in its string argument, got: '${parsedArguments}'`);
-      }
-      value = [
-        parsedArguments.substring(0, firstDot),
-        parsedArguments.substring(firstDot + 1), // the + 1 is to skip the actual '.'
-      ];
-    } else {
-      // this is the form where the arguments to Fn::GetAtt are already an array -
-      // in this case, nothing more to do
-      value = parsedArguments;
-    }
-
-    return { 'Fn::GetAtt': value };
-  }),
 );
 
 function parseYamlStrWithCfnTags(text: string): any {