From e82e20848887d1626c35145ad773a786ffc2b5d2 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Tue, 26 Mar 2019 15:07:13 +0100 Subject: [PATCH] feat(core): present reason for cyclic references (#2061) To help people debugging cyclic references, we now trace the "reason" a cyclic reference got added, so that we can present the conflicting references instead of just presenting an error. Fix the order of the error message, which was the wrong way around. Clean up references a little: - Split out `Reference` and `CfnReference`, which got conflated in an undesirable way. `Reference` is now officially the base class for all references, and `CfnReference` is only one implementation of it for CloudFormation references. - Make 'scope' required for references, the only place where it was potentially empty was for CFN Pseudo Parameters, refactored those to not use classes anymore (because there's no need to). - Get rid of 'Ref', the class wasn't being very useful. - Make a dependency Construct => Stack lazy (it was conflicting at load time with the Stack => Construct dependency which is more important). --- .../@aws-cdk/aws-codebuild/lib/project.ts | 2 +- .../lib/shared/imported.ts | 2 +- .../aws-ses/lib/receipt-rule-action.ts | 6 +- packages/@aws-cdk/cdk/lib/cfn-element.ts | 23 ++-- packages/@aws-cdk/cdk/lib/cfn-parameter.ts | 4 +- packages/@aws-cdk/cdk/lib/cfn-reference.ts | 115 ++++++++++++++++++ packages/@aws-cdk/cdk/lib/cfn-resource.ts | 4 +- packages/@aws-cdk/cdk/lib/construct.ts | 29 +++-- packages/@aws-cdk/cdk/lib/index.ts | 1 + packages/@aws-cdk/cdk/lib/pseudo.ts | 97 +++++---------- packages/@aws-cdk/cdk/lib/reference.ts | 113 ++--------------- packages/@aws-cdk/cdk/lib/stack.ts | 41 +++++-- packages/@aws-cdk/cdk/lib/token.ts | 8 -- packages/@aws-cdk/cdk/test/test.logical-id.ts | 4 +- packages/@aws-cdk/cdk/test/test.output.ts | 4 +- packages/@aws-cdk/cdk/test/test.stack.ts | 3 +- 16 files changed, 231 insertions(+), 225 deletions(-) create mode 100644 packages/@aws-cdk/cdk/lib/cfn-reference.ts diff --git a/packages/@aws-cdk/aws-codebuild/lib/project.ts b/packages/@aws-cdk/aws-codebuild/lib/project.ts index 8f1ade3a851d0..f8eb90411569a 100644 --- a/packages/@aws-cdk/aws-codebuild/lib/project.ts +++ b/packages/@aws-cdk/aws-codebuild/lib/project.ts @@ -602,7 +602,7 @@ export class Project extends ProjectBase { let cache: CfnProject.ProjectCacheProperty | undefined; if (props.cacheBucket) { - const cacheDir = props.cacheDir != null ? props.cacheDir : new cdk.AwsNoValue().toString(); + const cacheDir = props.cacheDir != null ? props.cacheDir : cdk.Aws.noValue; cache = { type: 'S3', location: cdk.Fn.join('/', [props.cacheBucket.bucketName, cacheDir]), diff --git a/packages/@aws-cdk/aws-elasticloadbalancingv2/lib/shared/imported.ts b/packages/@aws-cdk/aws-elasticloadbalancingv2/lib/shared/imported.ts index 96f33d4d76645..e2dc46523121b 100644 --- a/packages/@aws-cdk/aws-elasticloadbalancingv2/lib/shared/imported.ts +++ b/packages/@aws-cdk/aws-elasticloadbalancingv2/lib/shared/imported.ts @@ -24,7 +24,7 @@ export abstract class ImportedTargetGroupBase extends cdk.Construct implements I super(scope, id); this.targetGroupArn = props.targetGroupArn; - this.loadBalancerArns = props.loadBalancerArns || new cdk.AwsNoValue().toString(); + this.loadBalancerArns = props.loadBalancerArns || cdk.Aws.noValue; } public export() { diff --git a/packages/@aws-cdk/aws-ses/lib/receipt-rule-action.ts b/packages/@aws-cdk/aws-ses/lib/receipt-rule-action.ts index 36c8a7892e105..8c8b966d97b5e 100644 --- a/packages/@aws-cdk/aws-ses/lib/receipt-rule-action.ts +++ b/packages/@aws-cdk/aws-ses/lib/receipt-rule-action.ts @@ -280,7 +280,7 @@ export class ReceiptRuleLambdaAction implements IReceiptRuleAction { this.props.function.addPermission(permissionId, { action: 'lambda:InvokeFunction', principal: new iam.ServicePrincipal('ses.amazonaws.com'), - sourceAccount: new cdk.ScopedAws().accountId + sourceAccount: cdk.Aws.accountId }); } @@ -344,7 +344,7 @@ export class ReceiptRuleS3Action implements IReceiptRuleAction { .addServicePrincipal('ses.amazonaws.com') .addResource(this.props.bucket.arnForObjects(`${keyPattern}*`)) .addCondition('StringEquals', { - 'aws:Referer': new cdk.ScopedAws().accountId + 'aws:Referer': cdk.Aws.accountId }); this.props.bucket.addToResourcePolicy(s3Statement); @@ -362,7 +362,7 @@ export class ReceiptRuleS3Action implements IReceiptRuleAction { 'kms:EncryptionContext:aws:ses:message-id': 'false' }, StringEquals: { - 'kms:EncryptionContext:aws:ses:source-account': new cdk.ScopedAws().accountId + 'kms:EncryptionContext:aws:ses:source-account': cdk.Aws.accountId } }); diff --git a/packages/@aws-cdk/cdk/lib/cfn-element.ts b/packages/@aws-cdk/cdk/lib/cfn-element.ts index ffd546b0f98cb..2d08e69a619ea 100644 --- a/packages/@aws-cdk/cdk/lib/cfn-element.ts +++ b/packages/@aws-cdk/cdk/lib/cfn-element.ts @@ -126,17 +126,6 @@ export abstract class CfnElement extends Construct { } } -import { Reference } from "./reference"; - -/** - * A generic, untyped reference to a Stack Element - */ -export class Ref extends Reference { - constructor(element: CfnElement) { - super({ Ref: element.logicalId }, 'Ref', element); - } -} - /** * Base class for referenceable CloudFormation constructs which are not Resources * @@ -152,8 +141,16 @@ export abstract class CfnRefElement extends CfnElement { * Returns a token to a CloudFormation { Ref } that references this entity based on it's logical ID. */ public get ref(): string { - return new Ref(this).toString(); + return this.referenceToken.toString(); + } + + /** + * Return a token that will CloudFormation { Ref } this stack element + */ + protected get referenceToken(): Token { + return new CfnReference({ Ref: this.logicalId }, 'Ref', this); } } -import { findTokens } from "./resolve"; +import { CfnReference } from "./cfn-reference"; +import { findTokens } from "./resolve"; \ No newline at end of file diff --git a/packages/@aws-cdk/cdk/lib/cfn-parameter.ts b/packages/@aws-cdk/cdk/lib/cfn-parameter.ts index c5006a6a8d35f..3d8e982f79f75 100644 --- a/packages/@aws-cdk/cdk/lib/cfn-parameter.ts +++ b/packages/@aws-cdk/cdk/lib/cfn-parameter.ts @@ -1,4 +1,4 @@ -import { CfnRefElement, Ref } from './cfn-element'; +import { CfnRefElement } from './cfn-element'; import { Construct } from './construct'; import { Token } from './token'; @@ -99,7 +99,7 @@ export class CfnParameter extends CfnRefElement { constructor(scope: Construct, id: string, props: CfnParameterProps) { super(scope, id); this.properties = props; - this.value = new Ref(this); + this.value = this.referenceToken; this.stringValue = this.value.toString(); this.stringListValue = this.value.toList(); } diff --git a/packages/@aws-cdk/cdk/lib/cfn-reference.ts b/packages/@aws-cdk/cdk/lib/cfn-reference.ts new file mode 100644 index 0000000000000..9d533f4778d79 --- /dev/null +++ b/packages/@aws-cdk/cdk/lib/cfn-reference.ts @@ -0,0 +1,115 @@ +import { Reference } from "./reference"; + +const CFN_REFERENCE_SYMBOL = Symbol('@aws-cdk/cdk.CfnReference'); + +/** + * A Token that represents a CloudFormation reference to another resource + * + * If these references are used in a different stack from where they are + * defined, appropriate CloudFormation `Export`s and `Fn::ImportValue`s will be + * synthesized automatically instead of the regular CloudFormation references. + * + * Additionally, the dependency between the stacks will be recorded, and the toolkit + * will make sure to deploy producing stack before the consuming stack. + * + * This magic happens in the prepare() phase, where consuming stacks will call + * `consumeFromStack` on these Tokens and if they happen to be exported by a different + * Stack, we'll register the dependency. + */ +export class CfnReference extends Reference { + /** + * Check whether this is actually a Reference + */ + public static isCfnReference(x: Token): x is CfnReference { + return (x as any)[CFN_REFERENCE_SYMBOL] === true; + } + + /** + * What stack this Token is pointing to + */ + private readonly producingStack?: Stack; + + /** + * The Tokens that should be returned for each consuming stack (as decided by the producing Stack) + */ + private readonly replacementTokens: Map; + + private readonly originalDisplayName: string; + + constructor(value: any, displayName: string, target: Construct) { + if (typeof(value) === 'function') { + throw new Error('Reference can only hold CloudFormation intrinsics (not a function)'); + } + // prepend scope path to display name + super(value, `${target.node.id}.${displayName}`, target); + this.originalDisplayName = displayName; + this.replacementTokens = new Map(); + + this.producingStack = target.node.stack; + Object.defineProperty(this, CFN_REFERENCE_SYMBOL, { value: true }); + } + + public resolve(context: ResolveContext): any { + // If we have a special token for this consuming stack, resolve that. Otherwise resolve as if + // we are in the same stack. + const token = this.replacementTokens.get(context.scope.node.stack); + if (token) { + return token.resolve(context); + } else { + return super.resolve(context); + } + } + + /** + * Register a stack this references is being consumed from. + */ + public consumeFromStack(consumingStack: Stack, consumingConstruct: IConstruct) { + if (this.producingStack && this.producingStack !== consumingStack && !this.replacementTokens.has(consumingStack)) { + // We're trying to resolve a cross-stack reference + consumingStack.addDependency(this.producingStack, `${consumingConstruct.node.path} -> ${this.target.node.path}.${this.originalDisplayName}`); + this.replacementTokens.set(consumingStack, this.exportValue(this, consumingStack)); + } + } + + /** + * Export a Token value for use in another stack + * + * Works by mutating the producing stack in-place. + */ + private exportValue(tokenValue: Token, consumingStack: Stack): Token { + const producingStack = this.producingStack!; + + if (producingStack.env.account !== consumingStack.env.account || producingStack.env.region !== consumingStack.env.region) { + throw new Error('Can only reference cross stacks in the same region and account.'); + } + + // Ensure a singleton "Exports" scoping Construct + // This mostly exists to trigger LogicalID munging, which would be + // disabled if we parented constructs directly under Stack. + // Also it nicely prevents likely construct name clashes + + const exportsName = 'Exports'; + let stackExports = producingStack.node.tryFindChild(exportsName) as Construct; + if (stackExports === undefined) { + stackExports = new Construct(producingStack, exportsName); + } + + // Ensure a singleton CfnOutput for this value + const resolved = producingStack.node.resolve(tokenValue); + const id = 'Output' + JSON.stringify(resolved); + let output = stackExports.node.tryFindChild(id) as CfnOutput; + if (!output) { + output = new CfnOutput(stackExports, id, { value: tokenValue }); + } + + // We want to return an actual FnImportValue Token here, but Fn.importValue() returns a 'string', + // so construct one in-place. + return new Token({ 'Fn::ImportValue': output.obtainExportName() }); + } + +} + +import { CfnOutput } from "./cfn-output"; +import { Construct, IConstruct } from "./construct"; +import { Stack } from "./stack"; +import { ResolveContext, Token } from "./token"; \ No newline at end of file diff --git a/packages/@aws-cdk/cdk/lib/cfn-resource.ts b/packages/@aws-cdk/cdk/lib/cfn-resource.ts index 74bc60dce7159..9694cdf08d7ed 100644 --- a/packages/@aws-cdk/cdk/lib/cfn-resource.ts +++ b/packages/@aws-cdk/cdk/lib/cfn-resource.ts @@ -1,13 +1,13 @@ import cxapi = require('@aws-cdk/cx-api'); import { CfnCondition } from './cfn-condition'; import { Construct, IConstruct } from './construct'; -import { Reference } from './reference'; import { CreationPolicy, DeletionPolicy, UpdatePolicy } from './resource-policy'; import { TagManager } from './tag-manager'; import { capitalizePropertyNames, ignoreEmpty, PostResolveToken } from './util'; // import required to be here, otherwise causes a cycle when running the generated JavaScript // tslint:disable-next-line:ordered-imports import { CfnRefElement } from './cfn-element'; +import { CfnReference } from './cfn-reference'; export interface CfnResourceProps { /** @@ -133,7 +133,7 @@ export class CfnResource extends CfnRefElement { * @param attributeName The name of the attribute. */ public getAtt(attributeName: string) { - return new Reference({ 'Fn::GetAtt': [this.logicalId, attributeName] }, attributeName, this); + return new CfnReference({ 'Fn::GetAtt': [this.logicalId, attributeName] }, attributeName, this); } /** diff --git a/packages/@aws-cdk/cdk/lib/construct.ts b/packages/@aws-cdk/cdk/lib/construct.ts index 0a2c0ea4856e6..172ed9dc4981e 100644 --- a/packages/@aws-cdk/cdk/lib/construct.ts +++ b/packages/@aws-cdk/cdk/lib/construct.ts @@ -46,11 +46,11 @@ export class ConstructNode { private readonly _children: { [name: string]: IConstruct } = { }; private readonly context: { [key: string]: any } = { }; private readonly _metadata = new Array(); - private readonly references = new Set(); + private readonly references = new Set(); private readonly dependencies = new Set(); /** Will be used to cache the value of ``this.stack``. */ - private _stack?: Stack; + private _stack?: import('./stack').Stack; /** * If this is set to 'true'. addChild() calls for this construct and any child @@ -91,11 +91,13 @@ export class ConstructNode { /** * The stack the construct is a part of. */ - public get stack(): Stack { + public get stack(): import('./stack').Stack { + // Lazy import to break cyclic import + const stack: typeof import('./stack') = require('./stack'); return this._stack || (this._stack = _lookStackUp(this)); - function _lookStackUp(_this: ConstructNode) { - if (Stack.isStack(_this.host)) { + function _lookStackUp(_this: ConstructNode): import('./stack').Stack { + if (stack.Stack.isStack(_this.host)) { return _this.host; } if (!_this.scope) { @@ -471,7 +473,7 @@ export class ConstructNode { */ public recordReference(...refs: Token[]) { for (const ref of refs) { - if (ref.isReference) { + if (Reference.isReference(ref)) { this.references.add(ref); } } @@ -480,12 +482,12 @@ export class ConstructNode { /** * Return all references of the given type originating from this node or any of its children */ - public findReferences(): Token[] { - const ret = new Set(); + public findReferences(): OutgoingReference[] { + const ret = new Set(); function recurse(node: ConstructNode) { - for (const ref of node.references) { - ret.add(ref); + for (const reference of node.references) { + ret.add({ source: node.host, reference }); } for (const child of node.children) { @@ -729,5 +731,10 @@ export interface Dependency { target: IConstruct; } +export interface OutgoingReference { + source: IConstruct; + reference: Reference; +} + // Import this _after_ everything else to help node work the classes out in the correct order... -import { Stack } from './stack'; +import { Reference } from './reference'; \ No newline at end of file diff --git a/packages/@aws-cdk/cdk/lib/index.ts b/packages/@aws-cdk/cdk/lib/index.ts index c525bbed59809..bf7c83310336d 100644 --- a/packages/@aws-cdk/cdk/lib/index.ts +++ b/packages/@aws-cdk/cdk/lib/index.ts @@ -16,6 +16,7 @@ export * from './logical-id'; export * from './cfn-mapping'; export * from './cfn-output'; export * from './cfn-parameter'; +export * from './cfn-reference'; export * from './pseudo'; export * from './cfn-resource'; export * from './resource-policy'; diff --git a/packages/@aws-cdk/cdk/lib/pseudo.ts b/packages/@aws-cdk/cdk/lib/pseudo.ts index 407aa421eb306..93578652afd04 100644 --- a/packages/@aws-cdk/cdk/lib/pseudo.ts +++ b/packages/@aws-cdk/cdk/lib/pseudo.ts @@ -1,7 +1,16 @@ +import { CfnReference } from './cfn-reference'; import { Construct } from './construct'; -import { Reference } from './reference'; import { Token } from './token'; +const AWS_ACCOUNTID = 'AWS::AccountId'; +const AWS_URLSUFFIX = 'AWS::URLSuffix'; +const AWS_NOTIFICATIONARNS = 'AWS::NotificationARNs'; +const AWS_PARTITION = 'AWS::Partition'; +const AWS_REGION = 'AWS::Region'; +const AWS_STACKID = 'AWS::StackId'; +const AWS_STACKNAME = 'AWS::StackName'; +const AWS_NOVALUE = 'AWS::NoValue'; + /** * Accessor for pseudo parameters * @@ -14,35 +23,35 @@ export class Aws { } public static get accountId(): string { - return new AwsAccountId(undefined).toString(); + return new UnscopedPseudo(AWS_ACCOUNTID).toString(); } public static get urlSuffix(): string { - return new AwsURLSuffix(undefined).toString(); + return new UnscopedPseudo(AWS_URLSUFFIX).toString(); } public static get notificationArns(): string[] { - return new AwsNotificationARNs(undefined).toList(); + return new UnscopedPseudo(AWS_NOTIFICATIONARNS).toList(); } public static get partition(): string { - return new AwsPartition(undefined).toString(); + return new UnscopedPseudo(AWS_PARTITION).toString(); } public static get region(): string { - return new AwsRegion(undefined).toString(); + return new UnscopedPseudo(AWS_REGION).toString(); } public static get stackId(): string { - return new AwsStackId(undefined).toString(); + return new UnscopedPseudo(AWS_STACKID).toString(); } public static get stackName(): string { - return new AwsStackName(undefined).toString(); + return new UnscopedPseudo(AWS_STACKNAME).toString(); } public static get noValue(): string { - return new AwsNoValue().toString(); + return new UnscopedPseudo(AWS_NOVALUE).toString(); } } @@ -53,88 +62,46 @@ export class Aws { * tree, and their values will be exported automatically. */ export class ScopedAws { - constructor(private readonly scope?: Construct) { + constructor(private readonly scope: Construct) { } public get accountId(): string { - return new AwsAccountId(this.scope).toString(); + return new ScopedPseudo(AWS_ACCOUNTID, this.scope).toString(); } public get urlSuffix(): string { - return new AwsURLSuffix(this.scope).toString(); + return new ScopedPseudo(AWS_URLSUFFIX, this.scope).toString(); } public get notificationArns(): string[] { - return new AwsNotificationARNs(this.scope).toList(); + return new ScopedPseudo(AWS_NOTIFICATIONARNS, this.scope).toList(); } public get partition(): string { - return new AwsPartition(this.scope).toString(); + return new ScopedPseudo(AWS_PARTITION, this.scope).toString(); } public get region(): string { - return new AwsRegion(this.scope).toString(); + return new ScopedPseudo(AWS_REGION, this.scope).toString(); } public get stackId(): string { - return new AwsStackId(this.scope).toString(); + return new ScopedPseudo(AWS_STACKID, this.scope).toString(); } public get stackName(): string { - return new AwsStackName(this.scope).toString(); + return new ScopedPseudo(AWS_STACKNAME, this.scope).toString(); } } -class PseudoParameter extends Reference { - constructor(name: string, scope: Construct | undefined) { +class ScopedPseudo extends CfnReference { + constructor(name: string, scope: Construct) { super({ Ref: name }, name, scope); } } -class AwsAccountId extends PseudoParameter { - constructor(scope: Construct | undefined) { - super('AWS::AccountId', scope); - } -} - -class AwsURLSuffix extends PseudoParameter { - constructor(scope: Construct | undefined) { - super('AWS::URLSuffix', scope); - } -} - -class AwsNotificationARNs extends PseudoParameter { - constructor(scope: Construct | undefined) { - super('AWS::NotificationARNs', scope); - } -} - -export class AwsNoValue extends Token { - constructor() { - super({ Ref: 'AWS::NoValue' }); +class UnscopedPseudo extends Token { + constructor(name: string) { + super({ Ref: name }, name); } -} - -class AwsPartition extends PseudoParameter { - constructor(scope: Construct | undefined) { - super('AWS::Partition', scope); - } -} - -class AwsRegion extends PseudoParameter { - constructor(scope: Construct | undefined) { - super('AWS::Region', scope); - } -} - -class AwsStackId extends PseudoParameter { - constructor(scope: Construct | undefined) { - super('AWS::StackId', scope); - } -} - -class AwsStackName extends PseudoParameter { - constructor(scope: Construct | undefined) { - super('AWS::StackName', scope); - } -} +} \ No newline at end of file diff --git a/packages/@aws-cdk/cdk/lib/reference.ts b/packages/@aws-cdk/cdk/lib/reference.ts index c7e897a0df26a..e9a91da8c4967 100644 --- a/packages/@aws-cdk/cdk/lib/reference.ts +++ b/packages/@aws-cdk/cdk/lib/reference.ts @@ -1,116 +1,27 @@ -import { ResolveContext, Token } from "./token"; +import { Token } from "./token"; + +const REFERENCE_SYMBOL = Symbol('@aws-cdk/cdk.Reference'); /** - * A Token that represents a CloudFormation reference to another resource - * - * If these references are used in a different stack from where they are - * defined, appropriate CloudFormation `Export`s and `Fn::ImportValue`s will be - * synthesized automatically instead of the regular CloudFormation references. + * A Token that represents a reference between two constructs * - * Additionally, the dependency between the stacks will be recorded, and the toolkit - * will make sure to deploy producing stack before the consuming stack. - * - * This magic happens in the prepare() phase, where consuming stacks will call - * `consumeFromStack` on these Tokens and if they happen to be exported by a different - * Stack, we'll register the dependency. + * References are recorded. */ export class Reference extends Token { /** * Check whether this is actually a Reference */ - public static isReferenceToken(x: Token): x is Reference { - return (x as any).consumeFromStack !== undefined; + public static isReference(x: Token): x is Reference { + return (x as any)[REFERENCE_SYMBOL] === true; } - public readonly isReference?: boolean; + public readonly target: Construct; - /** - * What stack this Token is pointing to - */ - private readonly producingStack?: Stack; - - /** - * The Tokens that should be returned for each consuming stack (as decided by the producing Stack) - */ - private readonly replacementTokens: Map; - - constructor(value: any, displayName?: string, scope?: Construct) { - if (typeof(value) === 'function') { - throw new Error('Reference can only hold CloudFormation intrinsics (not a function)'); - } - // prepend scope path to display name - if (displayName && scope) { - displayName = `${scope.node.path}.${displayName}`; - } + constructor(value: any, displayName: string, target: Construct) { super(value, displayName); - this.replacementTokens = new Map(); - this.isReference = true; - - if (scope !== undefined) { - this.producingStack = scope.node.stack; - } - } - - public resolve(context: ResolveContext): any { - // If we have a special token for this consuming stack, resolve that. Otherwise resolve as if - // we are in the same stack. - const token = this.replacementTokens.get(context.scope.node.stack); - if (token) { - return token.resolve(context); - } else { - return super.resolve(context); - } - } - - /** - * Register a stack this references is being consumed from. - */ - public consumeFromStack(consumingStack: Stack) { - if (this.producingStack && this.producingStack !== consumingStack && !this.replacementTokens.has(consumingStack)) { - // We're trying to resolve a cross-stack reference - consumingStack.addDependency(this.producingStack); - this.replacementTokens.set(consumingStack, this.exportValue(this, consumingStack)); - } + this.target = target; + Object.defineProperty(this, REFERENCE_SYMBOL, { value: true }); } - - /** - * Export a Token value for use in another stack - * - * Works by mutating the producing stack in-place. - */ - private exportValue(tokenValue: Token, consumingStack: Stack): Token { - const producingStack = this.producingStack!; - - if (producingStack.env.account !== consumingStack.env.account || producingStack.env.region !== consumingStack.env.region) { - throw new Error('Can only reference cross stacks in the same region and account.'); - } - - // Ensure a singleton "Exports" scoping Construct - // This mostly exists to trigger LogicalID munging, which would be - // disabled if we parented constructs directly under Stack. - // Also it nicely prevents likely construct name clashes - - const exportsName = 'Exports'; - let stackExports = producingStack.node.tryFindChild(exportsName) as Construct; - if (stackExports === undefined) { - stackExports = new Construct(producingStack, exportsName); - } - - // Ensure a singleton CfnOutput for this value - const resolved = producingStack.node.resolve(tokenValue); - const id = 'Output' + JSON.stringify(resolved); - let output = stackExports.node.tryFindChild(id) as CfnOutput; - if (!output) { - output = new CfnOutput(stackExports, id, { value: tokenValue }); - } - - // We want to return an actual FnImportValue Token here, but Fn.importValue() returns a 'string', - // so construct one in-place. - return new Token({ 'Fn::ImportValue': output.obtainExportName() }); - } - } -import { CfnOutput } from "./cfn-output"; -import { Construct } from "./construct"; -import { Stack } from "./stack"; +import { Construct } from "./construct"; \ No newline at end of file diff --git a/packages/@aws-cdk/cdk/lib/stack.ts b/packages/@aws-cdk/cdk/lib/stack.ts index 6147adaec4f3b..3a309bcba6b99 100644 --- a/packages/@aws-cdk/cdk/lib/stack.ts +++ b/packages/@aws-cdk/cdk/lib/stack.ts @@ -4,7 +4,6 @@ import { CfnParameter } from './cfn-parameter'; import { Construct, IConstruct, PATH_SEP } from './construct'; import { Environment } from './environment'; import { HashedAddressingScheme, IAddressingScheme, LogicalIDs } from './logical-id'; -import { Reference } from './reference'; import { ISynthesisSession } from './synthesis'; import { makeUniqueId } from './uniqueid'; @@ -120,7 +119,7 @@ export class Stack extends Construct { /** * Other stacks this stack depends on */ - private readonly stackDependencies = new Set(); + private readonly stackDependencies = new Set(); /** * Values set for parameters in cloud assembly. @@ -282,19 +281,23 @@ export class Stack extends Construct { /** * Add a dependency between this stack and another stack */ - public addDependency(stack: Stack) { - if (stack.dependsOnStack(this)) { + public addDependency(stack: Stack, reason?: string) { + if (stack === this) { return; } // Can ignore a dependency on self + + reason = reason || 'dependency added using stack.addDependency()'; + const dep = stack.stackDependencyReasons(this); + if (dep !== undefined) { // tslint:disable-next-line:max-line-length - throw new Error(`Stack '${this.name}' already depends on stack '${stack.name}'. Adding this dependency would create a cyclic reference.`); + throw new Error(`'${stack.node.path}' depends on '${this.node.path}' (${dep.join(', ')}). Adding this dependency (${reason}) would create a cyclic reference.`); } - this.stackDependencies.add(stack); + this.stackDependencies.add({ stack, reason }); } /** * Return the stacks this stack depends on */ public dependencies(): Stack[] { - return Array.from(this.stackDependencies.values()); + return Array.from(this.stackDependencies.values()).map(d => d.stack); } /** @@ -466,8 +469,8 @@ export class Stack extends Construct { protected prepare() { // References for (const ref of this.node.findReferences()) { - if (Reference.isReferenceToken(ref)) { - ref.consumeFromStack(this); + if (CfnReference.isCfnReference(ref.reference)) { + ref.reference.consumeFromStack(this, ref.source); } } @@ -545,13 +548,19 @@ export class Stack extends Construct { /** * Check whether this stack has a (transitive) dependency on another stack + * + * Returns the list of reasons on the dependency path, or undefined + * if there is no dependency. */ - private dependsOnStack(other: Stack) { - if (this === other) { return true; } + private stackDependencyReasons(other: Stack): string[] | undefined { + if (this === other) { return []; } for (const dep of this.stackDependencies) { - if (dep.dependsOnStack(other)) { return true; } + const ret = dep.stack.stackDependencyReasons(other); + if (ret !== undefined) { + return [dep.reason].concat(ret); + } } - return false; + return undefined; } private collectMetadata() { @@ -671,6 +680,7 @@ function cfnElements(node: IConstruct, into: CfnElement[] = []): CfnElement[] { // These imports have to be at the end to prevent circular imports import { ArnComponents, arnFromComponents, parseArn } from './arn'; import { CfnElement } from './cfn-element'; +import { CfnReference } from './cfn-reference'; import { CfnResource } from './cfn-resource'; import { Aws, ScopedAws } from './pseudo'; @@ -684,3 +694,8 @@ function findResources(roots: Iterable): CfnResource[] { } return ret; } + +interface StackDependency { + stack: Stack; + reason: string; +} \ No newline at end of file diff --git a/packages/@aws-cdk/cdk/lib/token.ts b/packages/@aws-cdk/cdk/lib/token.ts index 232bd92f1ec97..709f17e1f5201 100644 --- a/packages/@aws-cdk/cdk/lib/token.ts +++ b/packages/@aws-cdk/cdk/lib/token.ts @@ -18,14 +18,6 @@ export const RESOLVE_METHOD = 'resolve'; * semantics. */ export class Token { - /** - * Indicate whether this Token represent a "reference" - * - * The Construct tree can be queried for the Reference Tokens that - * are used in it. - */ - public readonly isReference?: boolean; - private tokenStringification?: string; private tokenListification?: string[]; diff --git a/packages/@aws-cdk/cdk/test/test.logical-id.ts b/packages/@aws-cdk/cdk/test/test.logical-id.ts index a2037722587e3..5b7466cd2cc77 100644 --- a/packages/@aws-cdk/cdk/test/test.logical-id.ts +++ b/packages/@aws-cdk/cdk/test/test.logical-id.ts @@ -1,5 +1,5 @@ import { Test } from 'nodeunit'; -import { CfnResource, Construct, HashedAddressingScheme, IAddressingScheme, Ref, Stack } from '../lib'; +import { CfnResource, Construct, HashedAddressingScheme, IAddressingScheme, Stack } from '../lib'; /** * These tests are executed once (for specific ID schemes) @@ -204,7 +204,7 @@ const allSchemesTests: {[name: string]: (scheme: IAddressingScheme, test: Test) // WHEN const c1 = new CfnResource(stack, 'OriginalName', { type: 'R1' }); - const ref = new Ref(c1); + const ref = c1.ref; const c2 = new CfnResource(stack, 'Construct2', { type: 'R2', properties: { ReferenceToR1: ref } }); c2.node.addDependency(c1); diff --git a/packages/@aws-cdk/cdk/test/test.output.ts b/packages/@aws-cdk/cdk/test/test.output.ts index 639152ad6cbd4..e40cb317b5990 100644 --- a/packages/@aws-cdk/cdk/test/test.output.ts +++ b/packages/@aws-cdk/cdk/test/test.output.ts @@ -1,11 +1,11 @@ import { Test } from 'nodeunit'; -import { CfnOutput, CfnResource, Ref, Stack } from '../lib'; +import { CfnOutput, CfnResource, Stack } from '../lib'; export = { 'outputs can be added to the stack'(test: Test) { const stack = new Stack(); const res = new CfnResource(stack, 'MyResource', { type: 'R' }); - const ref = new Ref(res); + const ref = res.ref; new CfnOutput(stack, 'MyOutput', { export: 'ExportName', diff --git a/packages/@aws-cdk/cdk/test/test.stack.ts b/packages/@aws-cdk/cdk/test/test.stack.ts index 8fd5099d7c750..9a10931f3a663 100644 --- a/packages/@aws-cdk/cdk/test/test.stack.ts +++ b/packages/@aws-cdk/cdk/test/test.stack.ts @@ -286,7 +286,8 @@ export = { test.throws(() => { app.node.prepareTree(); - }, /Adding this dependency would create a cyclic reference/); + // tslint:disable-next-line:max-line-length + }, "'Stack2' depends on 'Stack1' (Stack2/SomeParameter -> Stack1.AWS::AccountId). Adding this dependency (Stack1/SomeParameter -> Stack2.AWS::AccountId) would create a cyclic reference."); test.done(); },