Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: stack.partition is never scoped #1763

Merged
merged 2 commits into from
Feb 18, 2019
Merged

fix: stack.partition is never scoped #1763

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
73f1165
fix: stack.partition is never scoped
Feb 14, 2019
73113bc
Make region and account nonscoped attributes
Feb 14, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions packages/@aws-cdk/aws-cloudwatch/lib/graph.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]
},
Expand Down Expand Up @@ -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: {
Expand Down Expand Up @@ -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'))
}
}];
Expand Down
2 changes: 1 addition & 1 deletion packages/@aws-cdk/aws-codepipeline/test/integ.pipeline-cfn-wtih-action-role.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:*')
Expand Down
2 changes: 1 addition & 1 deletion packages/@aws-cdk/aws-dynamodb/lib/table.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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));
}

Expand Down
47 changes: 43 additions & 4 deletions packages/@aws-cdk/cdk/lib/cloudformation/pseudo.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What’s the benefit of having the scope argument in all these ctors?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the consumer is being forced to make a conscious choice about making it scoped or nonscoped. You can't just forget to supply the constructor arg.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But what is the meaning of a scoped AwsRegion? Moreover why do we even need those as separate classes? Aren’t then static properties sufficient?

I feel I am missing something :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is fundamentally correct to treat region as scoped because it depends on the providing stack, but practically useless because in the only situation in which you would return the value and can usefully use it, it would be the same as the consumer's AWS::Region.

So yeah, I'll change 😊

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was trying to figure out why the comments indicated a scope was needed, this explains why the comment is outdated

https://github.com/aws/aws-cdk/blob/v2.173.0/packages/aws-cdk-lib/core/lib/cfn-pseudo.ts#L21

}

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) {
}

Expand Down Expand Up @@ -40,10 +83,6 @@ export class Aws {
public get stackName(): string {
return new AwsStackName(this.scope).toString();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does it make sense to export { AWS::StackName }? Or is it just to discover that things come from a different stack?

Something is oddd

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could make sense:

Properties:
  Thing: 
    Type: AWS::Thing::Thing
Outputs:
  ThingStack:
    Value: { Ref: "AWS::StackName" }
    Export: 
      Name: TheStackWithTheThing

Creates an export with a well-known name pointing to the name under which this stack was ultimately deployed (does not necessarily need to be the name known to the CDK at deployment time).

Or do you mean, why can stackName be scoped? Because then the answer is, yes, to detect that it came from a different Stack object.

const stack1 = new StackWithThing(...);

const stack2 = new Stack(...);
new ThingConsumer(..., {
   thingStackName: stack1.stackName // would not be the right value if nonscoped
});

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

got it, thanks

}

public get noValue(): string {
return new AwsNoValue().toString();
}
}

class PseudoParameter extends CfnReference {
Expand Down
25 changes: 17 additions & 8 deletions packages/@aws-cdk/cdk/lib/cloudformation/stack.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

/**
Expand All @@ -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;
}

/**
Expand All @@ -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;
}

/**
Expand All @@ -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;
}

/**
Expand Down Expand Up @@ -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';

Expand Down
35 changes: 31 additions & 4 deletions packages/@aws-cdk/cdk/test/cloudformation/test.arn.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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) {
Expand All @@ -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`));
Expand Down Expand Up @@ -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`));
Expand All @@ -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`));
Expand Down Expand Up @@ -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();
},
};
41 changes: 33 additions & 8 deletions packages/@aws-cdk/cdk/test/cloudformation/test.stack.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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) {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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 });
Expand All @@ -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
Expand All @@ -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
Expand Down