Skip to content

Commit

Permalink
fix(core): automatic cross-stack refs for CFN resources (#1510)
Browse files Browse the repository at this point in the history
The "toCloudFormation" method of generated CFN resources invoke
"resolve" so that any lazy tokens are evaluated. This escaped the
global settings set by `findTokens` which collect tokens so they
can be reported as references by `StackElement.prepare`.

To solve this, findTokens now accepts a function instead of an object
and basically just wraps it's invocation with settings that will collect
all tokens resolved within that scope. Not an ideal solution but
simple enough.

This was not discovered because we didn't have any tests that validated
the behavior of the generated CFN resources (they are not accessible
from the @aws-cdk/cdk library). We add a unit test in the IAM library to cover this use case.
  • Loading branch information
Elad Ben-Israel committed Jan 10, 2019
1 parent b909dcd commit ca5ee35
Show file tree
Hide file tree
Showing 3 changed files with 59 additions and 7 deletions.
52 changes: 52 additions & 0 deletions packages/@aws-cdk/aws-iam/test/test.auto-cross-stack-refs.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
import { expect } from '@aws-cdk/assert';
import cdk = require('@aws-cdk/cdk');
import { Test } from 'nodeunit';
import iam = require('../lib');

export = {
'automatic exports are created when attributes are referneced across stacks'(test: Test) {
// GIVEN
const stackWithUser = new cdk.Stack();
const stackWithGroup = new cdk.Stack();
const user = new iam.User(stackWithUser, 'User');
const group = new iam.Group(stackWithGroup, 'Group');

// WHEN
group.addUser(user);

//
// `group.addUser` adds the group to the user resource definition, so we expect
// that an automatic export will be created for the group and the user's stack
// to use ImportValue to import it.
// note that order of "expect"s matters. we first need to synthesize the user's
// stack so that the cross stack reference will be reported and only then the
// group's stack. in the real world, App will take care of this.
//

// THEN
expect(stackWithUser).toMatch({
Resources: {
User00B015A1: {
Type: "AWS::IAM::User",
Properties: {
Groups: [ { "Fn::ImportValue": "ExportsOutputRefGroupC77FDACD8CF7DD5B" } ]
}
}
}
});
expect(stackWithGroup).toMatch({
Outputs: {
ExportsOutputRefGroupC77FDACD8CF7DD5B: {
Value: { Ref: "GroupC77FDACD" },
Export: { Name: "ExportsOutputRefGroupC77FDACD8CF7DD5B" }
}
},
Resources: {
GroupC77FDACD: {
Type: "AWS::IAM::Group"
}
}
});
test.done();
}
};
5 changes: 1 addition & 4 deletions packages/@aws-cdk/cdk/lib/cloudformation/stack-element.ts
Original file line number Diff line number Diff line change
Expand Up @@ -128,10 +128,7 @@ export abstract class StackElement extends Construct implements IDependable {
// This does make the assumption that the error will not be rectified,
// but the error will be thrown later on anyway. If the error doesn't
// get thrown down the line, we may miss references.
this.node.recordReference(...findTokens(this.toCloudFormation(), {
scope: this,
prefix: []
}));
this.node.recordReference(...findTokens(this, () => this.toCloudFormation()));
} catch (e) {
if (e.type !== 'CfnSynthesisError') { throw e; }
}
Expand Down
9 changes: 6 additions & 3 deletions packages/@aws-cdk/cdk/lib/core/tokens/resolve.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { IConstruct } from '../construct';
import { containsListToken, TOKEN_MAP } from "./encoding";
import { RESOLVE_OPTIONS } from "./options";
import { RESOLVE_METHOD, ResolveContext, Token } from "./token";
Expand Down Expand Up @@ -120,13 +121,15 @@ export function resolve(obj: any, context: ResolveContext): any {
/**
* Find all Tokens that are used in the given structure
*/
export function findTokens(obj: any, context: ResolveContext): Token[] {
export function findTokens(scope: IConstruct, fn: () => any): Token[] {
const ret = new Array<Token>();

const options = RESOLVE_OPTIONS.push({ collect: ret.push.bind(ret) });
try {
// resolve() for side effect of calling 'preProcess', which adds to the
resolve(obj, context);
resolve(fn(), {
scope,
prefix: []
});
} finally {
options.pop();
}
Expand Down

0 comments on commit ca5ee35

Please sign in to comment.