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.

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

Framework: Tokens can be converted to strings #518

Merged
merged 20 commits into from
Aug 15, 2018
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
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
4 changes: 2 additions & 2 deletions packages/@aws-cdk/aws-cloudwatch/lib/dashboard.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { Construct, Stack, Token, tokenAwareJsonify } from "@aws-cdk/cdk";
import { Construct, Stack, Token, TokenJSON } from "@aws-cdk/cdk";
import { cloudformation } from './cloudwatch.generated';
import { Column, Row } from "./layout";
import { IWidget } from "./widget";
Expand Down Expand Up @@ -33,7 +33,7 @@ export class Dashboard extends Construct {
dashboardBody: new Token(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

This wraps a token and CloudFormationJSON.stringify also returns a Token. Can we eliminate one layer?

const column = new Column(...this.rows);
column.position(0, 0);
return tokenAwareJsonify({ widgets: column.toJson() });
return TokenJSON.stringify({ widgets: column.toJson() });
})
});
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,11 @@
"Type": "AWS::CloudWatch::Alarm",
"Properties": {
"ComparisonOperator": "GreaterThanOrEqualToThreshold",
"EvaluationPeriods": 3,
"MetricName": "ApproximateNumberOfMessagesVisible",
"Namespace": "AWS/SQS",
"Period": 300,
"Threshold": 100,
"Dimensions": [
{
"Name": "QueueName",
Expand All @@ -18,41 +23,55 @@
}
}
],
"EvaluationPeriods": 3,
"MetricName": "ApproximateNumberOfMessagesVisible",
"Namespace": "AWS/SQS",
"Period": 300,
"Statistic": "Average",
"Threshold": 100
"Statistic": "Average"
}
},
"DashCCD7F836": {
"Type": "AWS::CloudWatch::Dashboard",
"Properties": {
"DashboardName": "aws-cdk-cloudwatch-DashCCD7F836",
"DashboardBody": {
"Fn::Sub": [
"{\"widgets\":[{\"type\":\"text\",\"width\":6,\"height\":2,\"x\":0,\"y\":0,\"properties\":{\"markdown\":\"# This is my dashboard\"}},{\"type\":\"text\",\"width\":6,\"height\":2,\"x\":6,\"y\":0,\"properties\":{\"markdown\":\"you like?\"}},{\"type\":\"metric\",\"width\":6,\"height\":6,\"x\":0,\"y\":2,\"properties\":{\"view\":\"timeSeries\",\"title\":\"Messages in queue\",\"region\":\"${ref0}\",\"annotations\":{\"alarms\":[\"${ref1}\"]},\"yAxis\":{\"left\":{\"min\":0}}}},{\"type\":\"metric\",\"width\":6,\"height\":6,\"x\":0,\"y\":8,\"properties\":{\"view\":\"timeSeries\",\"title\":\"More messages in queue with alarm annotation\",\"region\":\"${ref0}\",\"metrics\":[[\"AWS/SQS\",\"ApproximateNumberOfMessagesVisible\",\"QueueName\",\"${ref2}\",{\"yAxis\":\"left\",\"period\":300,\"stat\":\"Average\"}]],\"annotations\":{\"horizontal\":[{\"label\":\"ApproximateNumberOfMessagesVisible >= 100 for 3 datapoints within 15 minutes\",\"value\":100,\"yAxis\":\"left\"}]},\"yAxis\":{\"left\":{\"min\":0},\"right\":{\"min\":0}}}},{\"type\":\"metric\",\"width\":6,\"height\":3,\"x\":0,\"y\":14,\"properties\":{\"view\":\"singleValue\",\"title\":\"Current messages in queue\",\"region\":\"${ref0}\",\"metrics\":[[\"AWS/SQS\",\"ApproximateNumberOfMessagesVisible\",\"QueueName\",\"${ref2}\",{\"yAxis\":\"left\",\"period\":300,\"stat\":\"Average\"}]]}}]}",
{
"ref0": {
"Fn::Join": [
"",
[
"{\"widgets\":[{\"type\":\"text\",\"width\":6,\"height\":2,\"x\":0,\"y\":0,\"properties\":{\"markdown\":\"# This is my dashboard\"}},{\"type\":\"text\",\"width\":6,\"height\":2,\"x\":6,\"y\":0,\"properties\":{\"markdown\":\"you like?\"}},{\"type\":\"metric\",\"width\":6,\"height\":6,\"x\":0,\"y\":2,\"properties\":{\"view\":\"timeSeries\",\"title\":\"Messages in queue\",\"region\":\"",
{
"Ref": "AWS::Region"
},
"ref1": {
"\",\"annotations\":{\"alarms\":[\"",
{
"Fn::GetAtt": [
"Alarm7103F465",
"Arn"
]
},
"ref2": {
"\"]},\"yAxis\":{\"left\":{\"min\":0}}}},{\"type\":\"metric\",\"width\":6,\"height\":6,\"x\":0,\"y\":8,\"properties\":{\"view\":\"timeSeries\",\"title\":\"More messages in queue with alarm annotation\",\"region\":\"",
{
"Ref": "AWS::Region"
},
"\",\"metrics\":[[\"AWS/SQS\",\"ApproximateNumberOfMessagesVisible\",\"QueueName\",\"",
{
"Fn::GetAtt": [
"queue",
"QueueName"
]
}
}
},
"\",{\"yAxis\":\"left\",\"period\":300,\"stat\":\"Average\"}]],\"annotations\":{\"horizontal\":[{\"label\":\"ApproximateNumberOfMessagesVisible >= 100 for 3 datapoints within 15 minutes\",\"value\":100,\"yAxis\":\"left\"}]},\"yAxis\":{\"left\":{\"min\":0},\"right\":{\"min\":0}}}},{\"type\":\"metric\",\"width\":6,\"height\":3,\"x\":0,\"y\":14,\"properties\":{\"view\":\"singleValue\",\"title\":\"Current messages in queue\",\"region\":\"",
{
"Ref": "AWS::Region"
},
"\",\"metrics\":[[\"AWS/SQS\",\"ApproximateNumberOfMessagesVisible\",\"QueueName\",\"",
{
"Fn::GetAtt": [
"queue",
"QueueName"
]
},
"\",{\"yAxis\":\"left\",\"period\":300,\"stat\":\"Average\"}]]}}]}"
]
]
}
},
"DashboardName": "aws-cdk-cloudwatch-DashCCD7F836"
}
}
}
}
}
15 changes: 6 additions & 9 deletions packages/@aws-cdk/aws-cloudwatch/test/test.dashboard.ts
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ export = {
test.done();
},

'tokens in widgets are retained through FnSub'(test: Test) {
'tokens in widgets are retained'(test: Test) {
// GIVEN
const stack = new Stack();
const dashboard = new Dashboard(stack, 'Dash');
Expand All @@ -82,14 +82,11 @@ export = {

// THEN
expect(stack).to(haveResource('AWS::CloudWatch::Dashboard', {
DashboardBody: { "Fn::Sub": [
// tslint:disable-next-line:max-line-length
"{\"widgets\":[{\"type\":\"metric\",\"width\":1,\"height\":1,\"x\":0,\"y\":0,\"properties\":{\"view\":\"timeSeries\",\"region\":\"${ref0}\",\"metrics\":[],\"annotations\":{\"horizontal\":[]},\"yAxis\":{\"left\":{\"min\":0},\"right\":{\"min\":0}}}}]}",
{
ref0: { Ref: "AWS::Region" }
}
]
}
DashboardBody: { "Fn::Join": [ "", [
"{\"widgets\":[{\"type\":\"metric\",\"width\":1,\"height\":1,\"x\":0,\"y\":0,\"properties\":{\"view\":\"timeSeries\",\"region\":\"",
{ Ref: "AWS::Region" },
"\",\"metrics\":[],\"annotations\":{\"horizontal\":[]},\"yAxis\":{\"left\":{\"min\":0},\"right\":{\"min\":0}}}}]}"
]]}
}));

test.done();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ export = {
new codepipeline.GitHubSource(new codepipeline.Stage(p, 'Source'), 'GH', {
artifactName: 'A',
branch: 'branch',
oauthToken: secret,
oauthToken: secret.value,
owner: 'foo',
repo: 'bar'
});
Expand Down
4 changes: 1 addition & 3 deletions packages/@aws-cdk/aws-logs/lib/cross-account-destination.ts
Original file line number Diff line number Diff line change
Expand Up @@ -59,14 +59,12 @@ export class CrossAccountDestination extends cdk.Construct implements ILogSubscr
constructor(parent: cdk.Construct, id: string, props: CrossAccountDestinationProps) {
super(parent, id);

this.policyDocument = new cdk.PolicyDocument();

// In the underlying model, the name is not optional, but we make it so anyway.
const destinationName = props.destinationName || new cdk.Token(() => this.generateUniqueName());

this.resource = new cloudformation.DestinationResource(this, 'Resource', {
destinationName,
destinationPolicy: new cdk.Token(() => !this.policyDocument.isEmpty ? JSON.stringify(this.policyDocument.resolve()) : ""),
destinationPolicy: new cdk.Token(() => !this.policyDocument.isEmpty ? JSON.stringify(cdk.resolve(this.policyDocument)) : ""),
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should have the exact same effect:

destinationPolicy: this.policyDocument

roleArn: props.role.roleArn,
targetArn: props.targetArn
});
Expand Down
3 changes: 1 addition & 2 deletions packages/@aws-cdk/aws-logs/test/test.destination.ts
Original file line number Diff line number Diff line change
Expand Up @@ -48,10 +48,9 @@ export = {

// THEN
expect(stack).to(haveResource('AWS::Logs::Destination', (props: any) => {
// tslint:disable-next-line:no-console
const pol = JSON.parse(props.DestinationPolicy);

return pol.Statement[0].action[0] === 'logs:TalkToMe';
return pol.Statement[0].Action === 'logs:TalkToMe';
}));

test.done();
Expand Down
4 changes: 2 additions & 2 deletions packages/@aws-cdk/cdk/lib/cloudformation/fn.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
import { Token } from '../core/tokens';
import { CloudFormationIntrinsicToken } from './intrinsics';
// tslint:disable:max-line-length

/**
* CloudFormation intrinsic functions.
* http://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/intrinsic-function-reference.html
*/
export class Fn extends Token {
export class Fn extends CloudFormationIntrinsicToken {
constructor(name: string, value: any) {
super(() => ({ [name]: value }));
}
Expand Down
28 changes: 28 additions & 0 deletions packages/@aws-cdk/cdk/lib/cloudformation/intrinsics.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
import { DEFAULT_ENGINE_NAME, ProvisioningEngine, StringFragment } from "../core/engine-strings";
Copy link
Contributor

Choose a reason for hiding this comment

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

This whole “provisioning engine framework” feels like it’s future proofing for something we really don’t know how to design yet. May I suggest that the token plugin system will be kept within the confines of the “tokens framework” and use domain-specific semantics.

For example, if the CDK will be used to synthesize pure Amazon State Language documents, then there is no provisioning engine involved, but there still might be tokenization heuristics.

Quite possibly, we might want to extract the tokens framework into a separate general-purpose and independent npm module, unrelated to cloud resource provisioning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well I did have it all in one file first, but it became quite large and mixed too many different concerns for my tastes. I just need some place to put this logic which is preferably a separate file, so it is "string handling for engines in an IoC fashion", which is this.

then there is no provisioning engine involved, but there still might be tokenization heuristics.

Regardless of the language that is being stringified (ASL or otherwise), if there is Token stringification to be done it will involve stitching string literals and intrinsics together. This must be done with an engine-specific FnConcat mechanism, which is what we model here.

If you use Amazon State Language on Terraform, we need terraform.concat, etc, and there would be a terraform/intrinsic.ts module.

Granted, maybe the filename is not fantastic here, and maybe the interface could be replaced with a single function, but that's about all the undesigning leeway we have.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not referring to the case where some other provisioning engine is used, I am referring to the case where the CDK is used to just synthesize other types of documents. I agree, tokenization/concat will still need to happen in those cases, but the provider will not be a "provisioning engine" but some other document format.

That's why I suggested to bind this terminology to the resolve/tokens domain and not to the resource provisioning domain. Then perhaps some of the abstractions will be an overkill. For example IProvisioningEngine.

import { IntrinsicToken } from "../core/tokens";

/**
* The default intrinsics Token engine for CloudFormation
*/
export const CLOUDFORMATION_ENGINE = 'cloudformation';

/**
* Base class for CloudFormation built-ins
*/
export class CloudFormationIntrinsicToken extends IntrinsicToken {
protected readonly engine: string = CLOUDFORMATION_ENGINE;
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of using a string value for engine, why not just point to a token resolver function?

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's not a resolver function, it's a join strings together in the engine function. Feels overly specific to me for intrinsics to carry around a function pointer to that.

But I guess I'm with ya on them having a direct reference to the engine, instead of indirected through a string pointing to a table.

Copy link
Contributor

Choose a reason for hiding this comment

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

joiner?

}

import { FnConcat } from "./fn";

const cloudFormationEngine = {
/**
* In CloudFormation, we combine strings by wrapping them in FnConcat
*/
combineStringFragments(fragments: StringFragment[]) {
return new FnConcat(...fragments.map(f => f.value));
}
};

ProvisioningEngine.register(CLOUDFORMATION_ENGINE, cloudFormationEngine);
ProvisioningEngine.register(DEFAULT_ENGINE_NAME, cloudFormationEngine);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need the default?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We used to at some point because we could at some point stitch values together where none of them would be an intrinsic (and so it would not be obvious which engine to use) but I guess at this point that's something I can do in the framework.

6 changes: 3 additions & 3 deletions packages/@aws-cdk/cdk/lib/cloudformation/pseudo.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import { Token } from '../core/tokens';
import { CloudFormationIntrinsicToken } from './intrinsics';

export class PseudoParameter extends Token {
export class PseudoParameter extends CloudFormationIntrinsicToken {
constructor(name: string) {
super(() => ({ Ref: name }));
super(() => ({ Ref: name }), name);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does this need to be lazy? We can plug in a concrete value

}
}

Expand Down
3 changes: 2 additions & 1 deletion packages/@aws-cdk/cdk/lib/cloudformation/resource.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { Construct } from '../core/construct';
import { Token } from '../core/tokens';
import { capitalizePropertyNames, ignoreEmpty } from '../core/util';
import { Condition } from './condition';
import { CloudFormationIntrinsicToken } from './intrinsics';
import { CreationPolicy, DeletionPolicy, UpdatePolicy } from './resource-policy';
import { IDependable, Referenceable, StackElement } from './stack';

Expand Down Expand Up @@ -82,7 +83,7 @@ export class Resource extends Referenceable {
* @param attributeName The name of the attribute.
*/
public getAtt(attributeName: string): Token {
return new Token(() => ({ 'Fn::GetAtt': [this.logicalId, attributeName] }));
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn’t need to be lazy

return new CloudFormationIntrinsicToken(() => ({ 'Fn::GetAtt': [this.logicalId, attributeName] }), `${this.logicalId}.${attributeName}`);
}

/**
Expand Down
3 changes: 1 addition & 2 deletions packages/@aws-cdk/cdk/lib/cloudformation/rule.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import { Construct } from '../core/construct';
import { Token } from '../core/tokens';
import { capitalizePropertyNames } from '../core/util';
import { FnCondition } from './fn';
import { Referenceable } from './stack';
Expand Down Expand Up @@ -30,7 +29,7 @@ export interface RuleProps {
* If the rule condition evaluates to false, the rule doesn't take effect.
* If the function in the rule condition evaluates to true, expressions in each assert are evaluated and applied.
*/
ruleCondition?: Token;
ruleCondition?: FnCondition;

/**
* Assertions which define the rule.
Expand Down
9 changes: 2 additions & 7 deletions packages/@aws-cdk/cdk/lib/cloudformation/secret.ts
Original file line number Diff line number Diff line change
Expand Up @@ -72,9 +72,9 @@ export interface SecretProps {
*/
export class SecretParameter extends Construct {
/**
* A token for the secret value.
* The value of the secret parameter.
*/
public value: Token;
public value: Secret;

constructor(parent: Construct, name: string, props: SecretProps) {
super(parent, name);
Expand All @@ -93,9 +93,4 @@ export class SecretParameter extends Construct {

this.value = param.ref;
}

// implicitly implements Token, and therefore Secret.
public resolve(): any {
return this.value;
}
}
5 changes: 3 additions & 2 deletions packages/@aws-cdk/cdk/lib/cloudformation/stack.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { App } from '../app';
import { Construct, PATH_SEP } from '../core/construct';
import { resolve, Token } from '../core/tokens';
import { Environment } from '../environment';
import { CloudFormationIntrinsicToken } from './intrinsics';
import { HashedAddressingScheme, IAddressingScheme, LogicalIDs } from './logical-id';
import { Resource } from './resource';

Expand Down Expand Up @@ -391,8 +392,8 @@ export abstract class Referenceable extends StackElement {
/**
* Returns a token to a CloudFormation { Ref } that references this entity based on it's logical ID.
*/
public get ref() {
return new Token(() => ({ Ref: this.logicalId }));
public get ref(): Token {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should just define a ‘Ref’ class for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No need for that. It doesn't add a lot of value and no user is ever going to instantiate it.

return new CloudFormationIntrinsicToken(() => ({ Ref: this.logicalId }), `${this.logicalId}.Ref`);
}
}

Expand Down
Loading