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

feat(cdk): transparent cross-stack references #1436

Merged
merged 44 commits into from
Jan 9, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
44 commits
Select commit Hold shift + click to select a range
7b97ebb
Fix tag manager tests to use resolve(token) instead of token.resolve()
rix0rrr Sep 29, 2018
1f938cf
Introduce StackAwareToken, make all pseudos Stack-Aware.
rix0rrr Sep 29, 2018
929b003
Merge branch 'master' into huijbers/x-stack-references
Dec 18, 2018
d02e16d
Remove concept of freezing
Dec 18, 2018
ac062fc
Add explicit preprocessor step for references
Dec 18, 2018
935fd82
Get rid of "context" parameter for Token resolution
Dec 18, 2018
9f40aa2
Make refs and attributes Stack-aware
Dec 21, 2018
c1d4dc1
Create tests for things I'll have to do later
Dec 21, 2018
5d85318
Fix core tests
Dec 26, 2018
4219539
Merge branch 'master' into huijbers/x-stack-references
rix0rrr Dec 28, 2018
d8876bf
Remove CloudFormationToken
rix0rrr Dec 28, 2018
0a724b4
Rename StackAwareCloudFormationToken -> StackAwareToken
rix0rrr Dec 28, 2018
445805f
CloudFormationConcat is no longer configurable, but just the default
rix0rrr Dec 28, 2018
cc928f8
Remove sourceStack argument to substitution
rix0rrr Dec 28, 2018
3cb539f
Make JSII happy
rix0rrr Dec 28, 2018
0840d76
Make Pseudo parameters (StackId etc.) attributes of Stack
rix0rrr Dec 28, 2018
73353c0
Add cdk deployment ordering; WIP - fighting with dependency cycles
rix0rrr Dec 28, 2018
f161a4d
Reorganizing to get rid of cycles, add integ test
rix0rrr Dec 31, 2018
7a105e9
Merge remote-tracking branch 'origin/master' into huijbers/x-stack-re…
rix0rrr Jan 4, 2019
ba3a4db
Rename StackAwareToken => CfnReference
rix0rrr Jan 4, 2019
8093361
Move resolve() into the context of a Construct, and hide the global m…
rix0rrr Jan 4, 2019
6330ab5
Introduce concept of references and use it to implement cross-stack refs
rix0rrr Jan 4, 2019
2c224c4
Fix some bugs
rix0rrr Jan 4, 2019
b17d811
More context attachments trying to make the tests work
rix0rrr Jan 4, 2019
a8e00d9
More work to get tests to pass
rix0rrr Jan 7, 2019
eef74a5
Also generate private (tools) packages in global tsconfig
Jan 7, 2019
5978c6b
Fix examples
Jan 7, 2019
996f998
Merge branch 'master' into huijbers/x-stack-references
Jan 8, 2019
e0ec521
Review comments
rix0rrr Jan 8, 2019
3a0d5b5
Fix compilation
rix0rrr Jan 8, 2019
1d5388d
Review comments
rix0rrr Jan 8, 2019
3ee48f7
Undo more refactoring breakage
rix0rrr Jan 8, 2019
d1b3c22
Fold ArnUtils.fromComponents() and ArnUtils.parse() into Stack
rix0rrr Jan 8, 2019
85cc4d2
Missing commit
rix0rrr Jan 8, 2019
8e64bc2
Replace construct library's use of CloudFormationJSON with stringifyJ…
rix0rrr Jan 8, 2019
5694b9c
Remove additional scope argument for IAM policies
rix0rrr Jan 8, 2019
d1a643e
Merge remote-tracking branch 'origin/master' into huijbers/x-stack-re…
rix0rrr Jan 8, 2019
12854e6
arnFromComponents() -> formatArn()
rix0rrr Jan 8, 2019
c7d4314
Move logic for collecting tokens to token layer
Jan 8, 2019
d2a656f
validate() is now protected
Jan 8, 2019
fdeaa55
Move protected() methods down in source
Jan 8, 2019
00cbe07
Call validateTree() instead of protected validate() in tests
Jan 8, 2019
0346946
Fix test
Jan 8, 2019
34bfb34
Fix another test
Jan 8, 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
2 changes: 1 addition & 1 deletion design/aws-guidelines.md
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,7 @@ properties that allow the user to specify an external resource identity, usually
by providing one or more resource attributes such as ARN, physical name, etc.

The import interface should have the minimum required properties, that is: if it
is possible to parse the resource name from the ARN (using `cdk.ArnUtils.parse`),
is possible to parse the resource name from the ARN (using `cdk.Stack.parseArn`),
then only the ARN should be required. In cases where it
is not possible to parse the ARN (e.g. if it is a token and the resource name
might have use "/" characters), both the ARN and the name should be optional and
Expand Down
17 changes: 8 additions & 9 deletions examples/cdk-examples-typescript/advanced-usage/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,7 @@ class CloudFormationExample extends cdk.Stack {
// outputs are constructs the synthesize into the template's "Outputs" section
new cdk.Output(this, 'Output', {
description: 'This is an output of the template',
value: `${new cdk.AwsAccountId()}/${param.ref}`
value: `${this.accountId}/${param.ref}`
});

// stack.templateOptions can be used to specify template-level options
Expand All @@ -166,14 +166,13 @@ class CloudFormationExample extends cdk.Stack {

// all CloudFormation's pseudo-parameters are supported via the `cdk.AwsXxx` classes
PseudoParameters: [
new cdk.AwsAccountId(),
new cdk.AwsDomainSuffix(),
new cdk.AwsNotificationARNs(),
new cdk.AwsNoValue(),
new cdk.AwsPartition(),
new cdk.AwsRegion(),
new cdk.AwsStackId(),
new cdk.AwsStackName(),
this.accountId,
this.urlSuffix,
this.notificationArns,
this.partition,
this.region,
this.stackId,
this.stackName,
],

// all CloudFormation's intrinsic functions are supported via the `cdk.Fn.xxx` static methods.
Expand Down
20 changes: 10 additions & 10 deletions packages/@aws-cdk/app-delivery/lib/pipeline-deploy-stack-action.ts
Original file line number Diff line number Diff line change
Expand Up @@ -140,16 +140,6 @@ export class PipelineDeployStackAction extends cdk.Construct {
});
}

public validate(): string[] {
const result = super.validate();
const assets = this.stack.node.metadata.filter(md => md.type === cxapi.ASSET_METADATA);
if (assets.length > 0) {
// FIXME: Implement the necessary actions to publish assets
result.push(`Cannot deploy the stack ${this.stack.name} because it references ${assets.length} asset(s)`);
}
return result;
}

/**
* Add policy statements to the role deploying the stack.
*
Expand All @@ -162,6 +152,16 @@ export class PipelineDeployStackAction extends cdk.Construct {
public addToRolePolicy(statement: iam.PolicyStatement) {
this.role.addToPolicy(statement);
}

protected validate(): string[] {
const result = super.validate();
const assets = this.stack.node.metadata.filter(md => md.type === cxapi.ASSET_METADATA);
if (assets.length > 0) {
// FIXME: Implement the necessary actions to publish assets
result.push(`Cannot deploy the stack ${this.stack.name} because it references ${assets.length} asset(s)`);
}
return result;
}
}

function cfnCapabilities(adminPermissions: boolean, capabilities?: cfn.CloudFormationCapabilities): cfn.CloudFormationCapabilities {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -274,7 +274,7 @@ export = nodeunit.testCase({
for (let i = 0 ; i < assetCount ; i++) {
deployedStack.node.addMetadata(cxapi.ASSET_METADATA, {});
}
test.deepEqual(action.validate(),
test.deepEqual(action.node.validateTree().map(x => x.message),
[`Cannot deploy the stack DeployedStack because it references ${assetCount} asset(s)`]);
}
)
Expand Down
5 changes: 4 additions & 1 deletion packages/@aws-cdk/assert/lib/expect.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,9 @@ export function expect(stack: api.SynthesizedStack | cdk.Stack, skipValidation =

if (isStackClassInstance(stack)) {
if (!skipValidation) {
// Do a prepare-and-validate run over the given stack
stack.node.prepareTree();

const errors = stack.node.validateTree();
if (errors.length > 0) {
throw new Error(`Stack validation failed:\n${errors.map(e => `${e.message} at: ${e.source.node.scope}`).join('\n')}`);
Expand All @@ -34,4 +37,4 @@ export function expect(stack: api.SynthesizedStack | cdk.Stack, skipValidation =

function isStackClassInstance(x: api.SynthesizedStack | cdk.Stack): x is cdk.Stack {
return 'toCloudFormation' in x;
}
}
4 changes: 2 additions & 2 deletions packages/@aws-cdk/assets-docker/lib/adopted-repository.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ export class AdoptedRepository extends ecr.RepositoryBase {
});

fn.addToRolePolicy(new iam.PolicyStatement()
.addResource(ecr.Repository.arnForLocalRepository(props.repositoryName))
.addResource(ecr.Repository.arnForLocalRepository(props.repositoryName, this))
.addActions(
'ecr:GetRepositoryPolicy',
'ecr:SetRepositoryPolicy',
Expand All @@ -67,7 +67,7 @@ export class AdoptedRepository extends ecr.RepositoryBase {

// this this repository is "local" to the stack (in the same region/account)
// we can render it's ARN from it's name.
this.repositoryArn = ecr.Repository.arnForLocalRepository(this.repositoryName);
this.repositoryArn = ecr.Repository.arnForLocalRepository(this.repositoryName, this);
}

/**
Expand Down
15 changes: 6 additions & 9 deletions packages/@aws-cdk/aws-apigateway/lib/deployment.ts
Original file line number Diff line number Diff line change
Expand Up @@ -107,13 +107,16 @@ class LatestDeploymentResource extends CfnDeployment {
private originalLogicalId?: string;
private lazyLogicalIdRequired: boolean;
private lazyLogicalId?: string;
private logicalIdToken: cdk.Token;
private hashComponents = new Array<any>();

constructor(scope: cdk.Construct, id: string, props: CfnDeploymentProps) {
super(scope, id, props);

// from this point, don't allow accessing logical ID before synthesis
this.lazyLogicalIdRequired = true;

this.logicalIdToken = new cdk.Token(() => this.lazyLogicalId);
}

/**
Expand All @@ -124,11 +127,7 @@ class LatestDeploymentResource extends CfnDeployment {
return this.originalLogicalId!;
}

if (!this.lazyLogicalId) {
throw new Error('This resource has a lazy logical ID which is calculated just before synthesis. Use a cdk.Token to evaluate');
}

return this.lazyLogicalId;
return this.logicalIdToken.toString();
}

/**
Expand Down Expand Up @@ -170,20 +169,18 @@ class LatestDeploymentResource extends CfnDeployment {
* Hooks into synthesis to calculate a logical ID that hashes all the components
* add via `addToLogicalId`.
*/
public validate() {
protected prepare() {
rix0rrr marked this conversation as resolved.
Show resolved Hide resolved
// if hash components were added to the deployment, we use them to calculate
// a logical ID for the deployment resource.
if (this.hashComponents.length === 0) {
this.lazyLogicalId = this.originalLogicalId;
} else {
const md5 = crypto.createHash('md5');
this.hashComponents
.map(c => cdk.resolve(c))
.map(c => this.node.resolve(c))
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 wondering if it would make sense to only expose resolve during prepare and synthesis by passing in something like SynthesisContext to the prepare method? This will sure people don't abuse this capability before the tree is ready.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's interesting - /me likey.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

@rix0rrr thanks for the due diligence. It's so much nicer to have these discussions over actual examples.

Let's punt this change for now for the sake of progress, but I think we should still explore this. The examples you provided are interesting and actually expose some issues, which is precisely

  • Error messages: the practice of JSON.stringify(resolve) feels like a lucky hack anyway, and only works when specific construct authors are aware that a value might be a token. I think a more robust solution to this problem would be to improve the string representation of tokens of resource references (GetAtt/Refs) in strings so they will be less intimidating. For instance, one can argue that something like ${{ "Fn::GetAtt": [ Logical, "Attribute" ]}} can be used for GetAtt tokens (and this can be used as the map key to the token object.

  • I'd argue that this use case should actually query unresolved and then just type-check. There is no need to actually resolve the values:

https://github.com/awslabs/aws-cdk/blob/3a0d5b5cf98e3cbcbd6139ef1e07135b4bece5f7/packages/@aws-cdk/aws-s3/lib/util.ts#L35

  • As for lazy evaluation: totally agree. Lazy evaluation is basically a hook into the synthesis phase. If you recall, I proposed in the past to stop allowing "free floating" lazy tokens have have something like: this.node.onSynthesize(synthesisContext => bla), so yes, passing synthesisContext to such a lazy handler makes total sense to me.

.forEach(c => md5.update(JSON.stringify(c)));

this.lazyLogicalId = this.originalLogicalId + md5.digest("hex");
}

return [];
}
}
22 changes: 16 additions & 6 deletions packages/@aws-cdk/aws-apigateway/lib/integrations/aws.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import cdk = require('@aws-cdk/cdk');
import { Integration, IntegrationOptions, IntegrationType } from '../integration';
import { Method } from '../method';
import { parseAwsApiCall } from '../util';

export interface AwsIntegrationProps {
Expand Down Expand Up @@ -61,21 +62,30 @@ export interface AwsIntegrationProps {
* technology.
*/
export class AwsIntegration extends Integration {
private scope?: cdk.IConstruct;

constructor(props: AwsIntegrationProps) {
const backend = props.subdomain ? `${props.subdomain}.${props.service}` : props.service;
const type = props.proxy ? IntegrationType.AwsProxy : IntegrationType.Aws;
const { apiType, apiValue } = parseAwsApiCall(props.path, props.action, props.actionParameters);
super({
type,
integrationHttpMethod: 'POST',
uri: cdk.ArnUtils.fromComponents({
service: 'apigateway',
account: backend,
resource: apiType,
sep: '/',
resourceName: apiValue,
uri: new cdk.Token(() => {
if (!this.scope) { throw new Error('AwsIntegration must be used in API'); }
return cdk.Stack.find(this.scope).formatArn({
service: 'apigateway',
account: backend,
resource: apiType,
sep: '/',
resourceName: apiValue,
});
}),
options: props.options,
});
}

public bind(method: Method) {
this.scope = method;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ export class LambdaIntegration extends AwsIntegration {
}

public bind(method: Method) {
super.bind(method);
const principal = new iam.ServicePrincipal('apigateway.amazonaws.com');

const desc = `${method.httpMethod}.${method.resource.resourcePath.replace(/\//g, '.')}`;
Expand Down
3 changes: 2 additions & 1 deletion packages/@aws-cdk/aws-apigateway/lib/method.ts
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,8 @@ export class Method extends cdk.Construct {
credentials = options.credentialsRole.roleArn;
} else if (options.credentialsPassthrough) {
// arn:aws:iam::*:user/*
credentials = cdk.ArnUtils.fromComponents({ service: 'iam', region: '', account: '*', resource: 'user', sep: '/', resourceName: '*' });
// tslint:disable-next-line:max-line-length
credentials = cdk.Stack.find(this).formatArn({ service: 'iam', region: '', account: '*', resource: 'user', sep: '/', resourceName: '*' });
}

return {
Expand Down
24 changes: 11 additions & 13 deletions packages/@aws-cdk/aws-apigateway/lib/restapi.ts
Original file line number Diff line number Diff line change
Expand Up @@ -301,33 +301,33 @@ export class RestApi extends cdk.Construct implements cdk.IDependable, IRestApi
method = '*';
}

return cdk.ArnUtils.fromComponents({
return cdk.Stack.find(this).formatArn({
service: 'execute-api',
resource: this.restApiId,
sep: '/',
resourceName: `${stage}/${method}${path}`
});
}

/**
* Internal API used by `Method` to keep an inventory of methods at the API
* level for validation purposes.
*/
public _attachMethod(method: Method) {
this.methods.push(method);
}

/**
* Performs validation of the REST API.
*/
public validate() {
protected validate() {
if (this.methods.length === 0) {
return [ `The REST API doesn't contain any methods` ];
}

return [];
}

/**
* Internal API used by `Method` to keep an inventory of methods at the API
* level for validation purposes.
*/
public _attachMethod(method: Method) {
this.methods.push(method);
}

private configureDeployment(props: RestApiProps) {
const deploy = props.deploy === undefined ? true : props.deploy;
if (deploy) {
Expand Down Expand Up @@ -358,7 +358,7 @@ export class RestApi extends cdk.Construct implements cdk.IDependable, IRestApi
private configureCloudWatchRole(apiResource: CfnRestApi) {
const role = new iam.Role(this, 'CloudWatchRole', {
assumedBy: new iam.ServicePrincipal('apigateway.amazonaws.com'),
managedPolicyArns: [ cdk.ArnUtils.fromComponents({
managedPolicyArns: [ cdk.Stack.find(this).formatArn({
service: 'iam',
region: '',
account: 'aws',
Expand Down Expand Up @@ -405,8 +405,6 @@ export enum EndpointType {
Private = 'PRIVATE'
}

export class RestApiUrl extends cdk.CloudFormationToken { }

class ImportedRestApi extends cdk.Construct implements IRestApi {
public restApiId: string;

Expand Down
4 changes: 3 additions & 1 deletion packages/@aws-cdk/aws-apigateway/lib/stage.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import cdk = require('@aws-cdk/cdk');
import { Stack } from '@aws-cdk/cdk';
import { CfnStage } from './apigateway.generated';
import { Deployment } from './deployment';
import { IRestApi } from './restapi';
Expand Down Expand Up @@ -180,7 +181,8 @@ export class Stage extends cdk.Construct implements cdk.IDependable {
if (!path.startsWith('/')) {
throw new Error(`Path must begin with "/": ${path}`);
}
return `https://${this.restApi.restApiId}.execute-api.${new cdk.AwsRegion()}.amazonaws.com/${this.stageName}${path}`;
const stack = Stack.find(this);
return `https://${this.restApi.restApiId}.execute-api.${stack.region}.${stack.urlSuffix}/${this.stageName}${path}`;
}

private renderMethodSettings(props: StageProps): CfnStage.MethodSettingProperty[] | undefined {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -836,7 +836,9 @@
{
"Ref": "AWS::Region"
},
".amazonaws.com/",
".",
{ "Ref": "AWS::URLSuffix" },
"/",
{
"Ref": "booksapiDeploymentStageprod55D8E03E"
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,9 @@
{
"Ref": "AWS::Region"
},
".amazonaws.com/",
".",
{ "Ref": "AWS::URLSuffix" },
"/",
{
"Ref": "myapiDeploymentStageprod298F01AF"
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -601,7 +601,9 @@
{
"Ref": "AWS::Region"
},
".amazonaws.com/",
".",
{ "Ref": "AWS::URLSuffix" },
"/",
{
"Ref": "myapiDeploymentStagebeta96434BEB"
},
Expand Down
2 changes: 1 addition & 1 deletion packages/@aws-cdk/aws-apigateway/test/test.deployment.ts
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ export = {
test.done();

function synthesize() {
stack.node.validateTree();
stack.node.prepareTree();
return stack.toCloudFormation();
}
},
Expand Down
4 changes: 2 additions & 2 deletions packages/@aws-cdk/aws-apigateway/test/test.method.ts
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ export = {
});

// THEN
test.deepEqual(cdk.resolve(method.methodArn), {
test.deepEqual(method.node.resolve(method.methodArn), {
"Fn::Join": [
"",
[
Expand Down Expand Up @@ -157,7 +157,7 @@ export = {
});

// THEN
test.deepEqual(cdk.resolve(method.testMethodArn), {
test.deepEqual(method.node.resolve(method.testMethodArn), {
"Fn::Join": [
"",
[
Expand Down
Loading