fix: stack.partition is never scoped (#1763)

A fully specified ARN can now be used in a different stack without leading to a Fn::ImportValue (used to be that we incurrend an ImportValue because of the Partition, but that is silly because the variable is known anyway). BREAKING CHANGE: 'Aws' class returns unscoped Tokens, introduce a new class 'ScopedAws' which returns scoped Tokens.
aws · Feb 18, 2019 · c968588 · c968588
1 parent 9af36af
commit c968588
Show file tree

Hide file tree

Showing 7 changed files with 129 additions and 29 deletions.
diff --git a/packages/@aws-cdk/aws-cloudwatch/lib/graph.ts b/packages/@aws-cdk/aws-cloudwatch/lib/graph.ts
@@ -73,7 +73,7 @@ export class AlarmWidget extends ConcreteWidget {
       properties: {
         view: 'timeSeries',
         title: this.props.title,
-        region: this.props.region || new cdk.Aws().region,
+        region: this.props.region || cdk.Aws.region,
         annotations: {
           alarms: [this.props.alarm.alarmArn]
         },
@@ -150,7 +150,7 @@ export class GraphWidget extends ConcreteWidget {
       properties: {
         view: 'timeSeries',
         title: this.props.title,
-        region: this.props.region || new cdk.Aws().region,
+        region: this.props.region || cdk.Aws.region,
         metrics: (this.props.left || []).map(m => metricJson(m, 'left')).concat(
              (this.props.right || []).map(m => metricJson(m, 'right'))),
         annotations: {
@@ -197,7 +197,7 @@ export class SingleValueWidget extends ConcreteWidget {
       properties: {
         view: 'singleValue',
         title: this.props.title,
-        region: this.props.region || new cdk.Aws().region,
+        region: this.props.region || cdk.Aws.region,
         metrics: this.props.metrics.map(m => metricJson(m, 'left'))
       }
     }];

diff --git a/packages/@aws-cdk/aws-codepipeline/test/integ.pipeline-cfn-wtih-action-role.ts b/packages/@aws-cdk/aws-codepipeline/test/integ.pipeline-cfn-wtih-action-role.ts
@@ -23,7 +23,7 @@ const sourceStage = {
 };
 
 const role = new iam.Role(stack, 'ActionRole', {
-  assumedBy: new iam.AccountPrincipal(new cdk.Aws().accountId)
+  assumedBy: new iam.AccountPrincipal(cdk.Aws.accountId)
 });
 role.addToPolicy(new iam.PolicyStatement()
   .addAction('sqs:*')

diff --git a/packages/@aws-cdk/aws-dynamodb/lib/table.ts b/packages/@aws-cdk/aws-dynamodb/lib/table.ts
@@ -412,7 +412,7 @@ export class Table extends Construct {
       return;
     }
     principal.addToPolicy(new iam.PolicyStatement()
-      .addResources(this.tableArn, new cdk.Token(() => this.hasIndex ? `${this.tableArn}/index/*` : new cdk.Aws().noValue).toString())
+      .addResources(this.tableArn, new cdk.Token(() => this.hasIndex ? `${this.tableArn}/index/*` : cdk.Aws.noValue).toString())
       .addActions(...actions));
   }
 

diff --git a/packages/@aws-cdk/cdk/lib/cloudformation/pseudo.ts b/packages/@aws-cdk/cdk/lib/cloudformation/pseudo.ts
@@ -10,6 +10,49 @@ import { CfnReference } from './cfn-tokens';
  * values can be obtained as properties from an scoped object.
  */
 export class Aws {
+  private constructor() {
+  }
+
+  public static get accountId(): string {
+    return new AwsAccountId(undefined).toString();
+  }
+
+  public static get urlSuffix(): string {
+    return new AwsURLSuffix(undefined).toString();
+  }
+
+  public static get notificationArns(): string[] {
+    return new AwsNotificationARNs(undefined).toList();
+  }
+
+  public static get partition(): string {
+    return new AwsPartition(undefined).toString();
+  }
+
+  public static get region(): string {
+    return new AwsRegion(undefined).toString();
+  }
+
+  public static get stackId(): string {
+    return new AwsStackId(undefined).toString();
+  }
+
+  public static get stackName(): string {
+    return new AwsStackName(undefined).toString();
+  }
+
+  public static get noValue(): string {
+    return new AwsNoValue().toString();
+  }
+}
+
+/**
+ * Accessor for scoped pseudo parameters
+ *
+ * These pseudo parameters are anchored to a stack somewhere in the construct
+ * tree, and their values will be exported automatically.
+ */
+export class ScopedAws {
   constructor(private readonly scope?: Construct) {
   }
 
@@ -40,10 +83,6 @@ export class Aws {
   public get stackName(): string {
     return new AwsStackName(this.scope).toString();
   }
-
-  public get noValue(): string {
-    return new AwsNoValue().toString();
-  }
 }
 
 class PseudoParameter extends CfnReference {

diff --git a/packages/@aws-cdk/cdk/lib/cloudformation/stack.ts b/packages/@aws-cdk/cdk/lib/cloudformation/stack.ts
@@ -248,7 +248,10 @@ export class Stack extends Construct {
     if (this.props && this.props.env && this.props.env.account) {
       return this.props.env.account;
     }
-    return new Aws(this).accountId;
+    // Does not need to be scoped, the only situation in which
+    // Export/Fn::ImportValue would work if { Ref: "AWS::AccountId" } is the
+    // same for provider and consumer anyway.
+    return Aws.accountId;
   }
 
   /**
@@ -262,21 +265,27 @@ export class Stack extends Construct {
     if (this.props && this.props.env && this.props.env.region) {
       return this.props.env.region;
     }
-    return new Aws(this).region;
+    // Does not need to be scoped, the only situation in which
+    // Export/Fn::ImportValue would work if { Ref: "AWS::AccountId" } is the
+    // same for provider and consumer anyway.
+    return Aws.region;
   }
 
   /**
    * The partition in which this stack is defined
    */
   public get partition(): string {
-    return new Aws(this).partition;
+    // Always return a non-scoped partition intrinsic. These will usually
+    // be used to construct an ARN, but there are no cross-partition
+    // calls anyway.
+    return Aws.partition;
   }
 
   /**
    * The Amazon domain suffix for the region in which this stack is defined
    */
   public get urlSuffix(): string {
-    return new Aws(this).urlSuffix;
+    return new ScopedAws(this).urlSuffix;
   }
 
   /**
@@ -285,7 +294,7 @@ export class Stack extends Construct {
    * @example After resolving, looks like arn:aws:cloudformation:us-west-2:123456789012:stack/teststack/51af3dc0-da77-11e4-872e-1234567db123
    */
   public get stackId(): string {
-    return new Aws(this).stackId;
+    return new ScopedAws(this).stackId;
   }
 
   /**
@@ -294,14 +303,14 @@ export class Stack extends Construct {
    * Only available at deployment time.
    */
   public get stackName(): string {
-    return new Aws(this).stackName;
+    return new ScopedAws(this).stackName;
   }
 
   /**
    * Returns the list of notification Amazon Resource Names (ARNs) for the current stack.
    */
   public get notificationArns(): string[] {
-    return new Aws(this).notificationArns;
+    return new ScopedAws(this).notificationArns;
   }
 
   /**
@@ -507,7 +516,7 @@ function stackElements(node: IConstruct, into: StackElement[] = []): StackElemen
 
 // These imports have to be at the end to prevent circular imports
 import { ArnComponents, arnFromComponents, parseArn } from './arn';
-import { Aws } from './pseudo';
+import { Aws, ScopedAws } from './pseudo';
 import { Resource } from './resource';
 import { StackElement } from './stack-element';
 

diff --git a/packages/@aws-cdk/cdk/test/cloudformation/test.arn.ts b/packages/@aws-cdk/cdk/test/cloudformation/test.arn.ts
@@ -1,5 +1,5 @@
 import { Test } from 'nodeunit';
-import { ArnComponents, Aws, Stack, Token } from '../../lib';
+import { ArnComponents, Output, ScopedAws, Stack, Token } from '../../lib';
 
 export = {
   'create from components with defaults'(test: Test) {
@@ -10,7 +10,7 @@ export = {
       resource: 'myqueuename'
     });
 
-    const pseudo = new Aws(stack);
+    const pseudo = new ScopedAws(stack);
 
     test.deepEqual(stack.node.resolve(arn),
                    stack.node.resolve(`arn:${pseudo.partition}:sqs:${pseudo.region}:${pseudo.accountId}:myqueuename`));
@@ -61,7 +61,7 @@ export = {
       resourceName: 'WordPress_App'
     });
 
-    const pseudo = new Aws(stack);
+    const pseudo = new ScopedAws(stack);
 
     test.deepEqual(stack.node.resolve(arn),
                    stack.node.resolve(`arn:${pseudo.partition}:codedeploy:${pseudo.region}:${pseudo.accountId}:application:WordPress_App`));
@@ -78,7 +78,7 @@ export = {
       resourceName: '/parameter-name'
     });
 
-    const pseudo = new Aws(stack);
+    const pseudo = new ScopedAws(stack);
 
     test.deepEqual(stack.node.resolve(arn),
                    stack.node.resolve(`arn:${pseudo.partition}:ssm:${pseudo.region}:${pseudo.accountId}:parameter/parameter-name`));
@@ -204,4 +204,31 @@ export = {
     }
   },
 
+  'can use a fully specified ARN from a different stack without incurring an import'(test: Test) {
+    // GIVEN
+    const stack1 = new Stack(undefined, 'Stack1', { env: { account: '12345678', region: 'us-turbo-5' }});
+    const stack2 = new Stack(undefined, 'Stack2', { env: { account: '87654321', region: 'us-turbo-1' }});
+
+    // WHEN
+    const arn = stack1.formatArn({
+      // No partition specified here
+      service: 'bla',
+      resource: 'thing',
+      resourceName: 'thong',
+    });
+    new Output(stack2, 'SomeValue', { value: arn });
+
+    // THEN
+    test.deepEqual(stack2.node.resolve(stack2.toCloudFormation()), {
+      Outputs: {
+        SomeValue: {
+          Value: {
+            // Look ma, no Fn::ImportValue!
+            'Fn::Join': ['', ['arn:', { Ref: 'AWS::Partition' }, ':bla:us-turbo-5:12345678:thing/thong']] }
+        }
+      }
+    });
+
+    test.done();
+  },
 };
diff --git a/packages/@aws-cdk/cdk/test/cloudformation/test.stack.ts b/packages/@aws-cdk/cdk/test/cloudformation/test.stack.ts
@@ -1,6 +1,6 @@
 import cxapi = require('@aws-cdk/cx-api');
 import { Test } from 'nodeunit';
-import { App, Aws, Condition, Construct, Include, Output, Parameter, Resource, Stack, Token } from '../../lib';
+import { App, Condition, Construct, Include, Output, Parameter, Resource, ScopedAws, Stack, Token } from '../../lib';
 
 export = {
   'a stack can be serialized into a CloudFormation template, initially it\'s empty'(test: Test) {
@@ -156,7 +156,7 @@ export = {
     // GIVEN
     const app = new App();
     const stack1 = new Stack(app, 'Stack1');
-    const account1 = new Aws(stack1).accountId;
+    const account1 = new ScopedAws(stack1).accountId;
     const stack2 = new Stack(app, 'Stack2');
 
     // WHEN - used in another stack
@@ -192,7 +192,7 @@ export = {
     // GIVEN
     const app = new App();
     const stack1 = new Stack(app, 'Stack1');
-    const account1 = new Aws(stack1).accountId;
+    const account1 = new ScopedAws(stack1).accountId;
     const stack2 = new Stack(app, 'Stack2');
 
     // WHEN - used in another stack
@@ -222,11 +222,36 @@ export = {
     test.done();
   },
 
+  'Cross-stack use of Region returns nonscoped intrinsic'(test: Test) {
+    // GIVEN
+    const app = new App();
+    const stack1 = new Stack(app, 'Stack1');
+    const stack2 = new Stack(app, 'Stack2');
+
+    // WHEN - used in another stack
+    new Output(stack2, 'DemOutput', { value: stack1.region });
+
+    // THEN
+    // Need to do this manually now, since we're in testing mode. In a normal CDK app,
+    // this happens as part of app.run().
+    app.node.prepareTree();
+
+    test.deepEqual(stack2.toCloudFormation(), {
+      Outputs: {
+        DemOutput: {
+          Value: { Ref: 'AWS::Region' },
+        }
+      }
+    });
+
+    test.done();
+  },
+
   'cross-stack references in strings work'(test: Test) {
     // GIVEN
     const app = new App();
     const stack1 = new Stack(app, 'Stack1');
-    const account1 = new Aws(stack1).accountId;
+    const account1 = new ScopedAws(stack1).accountId;
     const stack2 = new Stack(app, 'Stack2');
 
     // WHEN - used in another stack
@@ -251,9 +276,9 @@ export = {
     // GIVEN
     const app = new App();
     const stack1 = new Stack(app, 'Stack1');
-    const account1 = new Aws(stack1).accountId;
+    const account1 = new ScopedAws(stack1).accountId;
     const stack2 = new Stack(app, 'Stack2');
-    const account2 = new Aws(stack2).accountId;
+    const account2 = new ScopedAws(stack2).accountId;
 
     // WHEN
     new Parameter(stack2, 'SomeParameter', { type: 'String', default: account1 });
@@ -270,7 +295,7 @@ export = {
     // GIVEN
     const app = new App();
     const stack1 = new Stack(app, 'Stack1');
-    const account1 = new Aws(stack1).accountId;
+    const account1 = new ScopedAws(stack1).accountId;
     const stack2 = new Stack(app, 'Stack2');
 
     // WHEN
@@ -288,7 +313,7 @@ export = {
     // GIVEN
     const app = new App();
     const stack1 = new Stack(app, 'Stack1', { env: { account: '123456789012', region: 'es-norst-1' }});
-    const account1 = new Aws(stack1).accountId;
+    const account1 = new ScopedAws(stack1).accountId;
     const stack2 = new Stack(app, 'Stack2', { env: { account: '123456789012', region: 'es-norst-2' }});
 
     // WHEN