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

Context providers: return dummy values if env is undefined, but emit errors to fail "cdk synth" #227

Merged
merged 12 commits into from
Jul 19, 2018
55 changes: 48 additions & 7 deletions packages/@aws-cdk/core/lib/context.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,22 @@ export class ContextProvider {

/**
* Read a provider value, verifying it's a string
* @param provider The name of the context provider
* @param scope The scope (e.g. account/region) for the value
* @param args Any arguments
* @param defaultValue The value to return if there is no value defined for this context key
*/
public getStringValue(provider: string, scope: string[], args: string[]): string {
public getStringValue(
provider: string,
scope: undefined | string[],
args: string[],
defaultValue: string): string {
// if scope is undefined, this is probably a test mode, so we just
// return the default value
if (!scope) {
this.context.addError(formatMissingScopeError(provider, args));
return defaultValue;
}
const key = colonQuote([provider].concat(scope).concat(args)).join(':');
const value = this.context.getContext(key);
if (value != null) {
Expand All @@ -35,13 +49,30 @@ export class ContextProvider {
}

this.stack.reportMissingContext(key, { provider, scope, args });
return '';
return defaultValue;
}

/**
* Read a provider value, verifying it's a list
* @param provider The name of the context provider
* @param scope The scope (e.g. account/region) for the value
* @param args Any arguments
* @param defaultValue The value to return if there is no value defined for this context key
*/
public getStringListValue(provider: string, scope: string[], args: string[], defaultValue = ['']): string[] {
public getStringListValue(
provider: string,
scope: undefined | string[],
args: string[],
defaultValue: string[]): string[] {
// if scope is undefined, this is probably a test mode, so we just
// return the default value and report an error so this in not accidentally used
// in the toolkit
if (!scope) {
// tslint:disable-next-line:max-line-length
this.context.addError(formatMissingScopeError(provider, args));
return defaultValue;
}

const key = colonQuote([provider].concat(scope).concat(args)).join(':');
const value = this.context.getContext(key);

Expand All @@ -59,7 +90,7 @@ export class ContextProvider {
/**
* Helper function to wrap up account and region into a scope tuple
*/
public accountRegionScope(providerDescription: string): string[] {
public accountRegionScope(providerDescription: string): undefined | string[] {
const stack = Stack.find(this.context);
if (!stack) {
throw new Error(`${providerDescription}: construct must be in a stack`);
Expand All @@ -69,8 +100,7 @@ export class ContextProvider {
const region = stack.env.region;

if (account == null || region == null) {
// tslint:disable-next-line:max-line-length
throw new Error(`${providerDescription}: requires account and region information, but ${stack.name} doesn't have an "env" defined`);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not quite comfortable with this. How are we still guaranteeing that account and region information are available when invoking a context provider "for reals"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the realz worldz, a stack must be associated with an environment, either explicitly or implicitly. I don't think there's a use case for a stack without an env, is there?

Copy link
Contributor

Choose a reason for hiding this comment

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

As long as you don't use a context provider there's no reason to require it, but we could.

But what's the mechanism that's making sure this is true, once you remove this error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me get back to you 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@RomainMuller Is there a case where the toolkit invokes a CDK program with an undefined default environment (which is what "undefined env" translates to in the real world)?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't believe there's any case where it does so intentionally (meaning besides those cases when attempts to figure out a default environment failed).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As suggested by @rix0rrr, we decided that what we want is that the toolkit will fail synthesis with a meaningful error, but not through an exception but through error emitted as metadata during synthesis (similar to the warnings we have today). We will also add "info" messages, which will allow construct authors "communicate" information to users.

return undefined;
}

return [account, region];
Expand Down Expand Up @@ -123,6 +153,17 @@ export class SSMParameterProvider {
*/
public getString(parameterName: string): any {
const scope = this.provider.accountRegionScope('SSMParameterProvider');
return this.provider.getStringValue(SSM_PARAMETER_PROVIDER, scope, [parameterName]);
return this.provider.getStringValue(SSM_PARAMETER_PROVIDER, scope, [parameterName], 'dummy');
}
}

function formatMissingScopeError(provider: string, args: string[]) {
let s = `Cannot determine scope for context provider ${provider}`;
if (args.length > 0) {
s += JSON.stringify(args);
}
s += '.';
s += '\n';
s += 'This usually happens when AWS credentials are not available and the default account/region cannot be determined.';
return s;
}
23 changes: 22 additions & 1 deletion packages/@aws-cdk/core/lib/core/construct.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import cxapi = require('@aws-cdk/cx-api');
export const PATH_SEP = '/';

/**
Expand Down Expand Up @@ -208,12 +209,32 @@ export class Construct {
return this;
}

/**
* Adds a { "aws:cdk:info": <message> } metadata entry to this construct.
* The toolkit will display the info message when apps are synthesized.
* @param message The info message.
*/
public addInfo(message: string): Construct {
return this.addMetadata(cxapi.INFO_METADATA_KEY, message);
}

/**
* Adds a { warning: <message> } metadata entry to this construct.
* The toolkit will display the warning when an app is synthesized, or fail
* if run in --strict mode.
* @param message The warning message.
*/
public addWarning(message: string): Construct {
return this.addMetadata('warning', message);
return this.addMetadata(cxapi.WARNING_METADATA_KEY, message);
}

/**
* Adds an { error: <message> } metadata entry to this construct.
* The toolkit will fail synthesis when errors are reported.
* @param message The error message.
*/
public addError(message: string): Construct {
return this.addMetadata(cxapi.ERROR_METADATA_KEY, message);
}

/**
Expand Down
25 changes: 23 additions & 2 deletions packages/@aws-cdk/core/test/core/test.construct.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import cxapi = require('@aws-cdk/cx-api');
import { Test } from 'nodeunit';
import { Construct, Root } from '../../lib';

Expand Down Expand Up @@ -206,16 +207,36 @@ export = {
test.done();
},

'addWarning(message) can be used to add a "warning" metadata entry to the construct'(test: Test) {
'addWarning(message) can be used to add a "WARNING" message entry to the construct'(test: Test) {
const root = new Root();
const con = new Construct(root, 'MyConstruct');
con.addWarning('This construct is deprecated, use the other one instead');
test.deepEqual(con.metadata[0].type, 'warning');
test.deepEqual(con.metadata[0].type, cxapi.WARNING_METADATA_KEY);
test.deepEqual(con.metadata[0].data, 'This construct is deprecated, use the other one instead');
test.ok(con.metadata[0].trace.length > 0);
test.done();
},

'addError(message) can be used to add a "ERROR" message entry to the construct'(test: Test) {
const root = new Root();
const con = new Construct(root, 'MyConstruct');
con.addError('Stop!');
test.deepEqual(con.metadata[0].type, cxapi.ERROR_METADATA_KEY);
test.deepEqual(con.metadata[0].data, 'Stop!');
test.ok(con.metadata[0].trace.length > 0);
test.done();
},

'addInfo(message) can be used to add an "INFO" message entry to the construct'(test: Test) {
const root = new Root();
const con = new Construct(root, 'MyConstruct');
con.addInfo('Hey there, how do you do?');
test.deepEqual(con.metadata[0].type, cxapi.INFO_METADATA_KEY);
test.deepEqual(con.metadata[0].data, 'Hey there, how do you do?');
test.ok(con.metadata[0].trace.length > 0);
test.done();
},

'multiple children of the same type, with explicit names are welcome'(test: Test) {
const root = new Root();
new MyBeautifulConstruct(root, 'mbc1');
Expand Down
25 changes: 23 additions & 2 deletions packages/@aws-cdk/core/test/test.context.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import cxapi = require('@aws-cdk/cx-api');
import { Test } from 'nodeunit';
import { AvailabilityZoneProvider, resolve, SSMParameterProvider, Stack } from '../lib';
import { App, AvailabilityZoneProvider, Construct, MetadataEntry, resolve, SSMParameterProvider, Stack } from '../lib';

export = {
'AvailabilityZoneProvider returns a list with dummy values if the context is not available'(test: Test) {
Expand Down Expand Up @@ -50,7 +51,27 @@ export = {
test.deepEqual(azs, 'abc');

test.done();
}
},

'Return default values if "env" is undefined to facilitate unit tests, but also expect metadata to include "error" messages'(test: Test) {
const app = new App();
const stack = new Stack(app, 'test-stack');

const child = new Construct(stack, 'ChildConstruct');

test.deepEqual(new AvailabilityZoneProvider(stack).availabilityZones, [ 'dummy1a', 'dummy1b', 'dummy1c' ]);
test.deepEqual(new SSMParameterProvider(child).getString('foo'), 'dummy');

const output = app.synthesizeStack(stack.name);

const azError: MetadataEntry | undefined = output.metadata['/test-stack'].find(x => x.type === cxapi.ERROR_METADATA_KEY);
const ssmError: MetadataEntry | undefined = output.metadata['/test-stack/ChildConstruct'].find(x => x.type === cxapi.ERROR_METADATA_KEY);

test.ok(azError && (azError.data as string).includes('Cannot determine scope for context provider availability-zones.'));
test.ok(ssmError && (ssmError.data as string).includes('Cannot determine scope for context provider ssm["foo"].'));

test.done();
},
};

function firstKey(obj: any): string {
Expand Down
17 changes: 16 additions & 1 deletion packages/@aws-cdk/cx-api/lib/cxapi.ts
Original file line number Diff line number Diff line change
Expand Up @@ -102,4 +102,19 @@ export interface AssetMetadataEntry {
packaging: 'zip' | 'file';
s3BucketParameter: string;
s3KeyParameter: string;
}
}

/**
* Metadata key used to print INFO-level messages by the toolkit when an app is syntheized.
*/
export const INFO_METADATA_KEY = 'aws:cdk:info';

/**
* Metadata key used to print WARNING-level messages by the toolkit when an app is syntheized.
*/
export const WARNING_METADATA_KEY = 'aws:cdk:warning';

/**
* Metadata key used to print ERROR-level messages by the toolkit when an app is syntheized.
*/
export const ERROR_METADATA_KEY = 'aws:cdk:error';
4 changes: 2 additions & 2 deletions packages/@aws-cdk/ec2/test/test.fleet.ts
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ export = {
"IamInstanceProfile": {
"Ref": "MyFleetInstanceProfile70A58496"
},
"ImageId": "",
"ImageId": "dummy",
"InstanceType": "m4.micro",
"SecurityGroups": [
{
Expand Down Expand Up @@ -194,7 +194,7 @@ export = {
"IamInstanceProfile": {
"Ref": "MyFleetInstanceProfile70A58496"
},
"ImageId": "",
"ImageId": "dummy",
"InstanceType": "m4.micro",
"SecurityGroups": [
{
Expand Down
45 changes: 33 additions & 12 deletions packages/aws-cdk/bin/cdk.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ async function parseCommandLineArguments() {
.option('rename', { type: 'string', desc: 'Rename stack name if different then the one defined in the cloud executable', requiresArg: '[ORIGINAL:]RENAMED' })
.option('trace', { type: 'boolean', desc: 'Print trace for stack warnings' })
.option('strict', { type: 'boolean', desc: 'Do not construct stacks with warnings' })
.option('ignore-errors', { type: 'boolean', default: false, desc: 'Ignores synthesis errors, which will likely produce an invalid output' })
.option('json', { type: 'boolean', alias: 'j', desc: 'Use JSON output instead of YAML' })
.option('verbose', { type: 'boolean', alias: 'v', desc: 'Show debug logs' })
.demandCommand(1)
Expand Down Expand Up @@ -208,26 +209,40 @@ async function initCommandLine() {
}

/**
* Extracts 'warning' metadata entries from the stack synthesis
* Extracts 'aws:cdk:warning|info|error' metadata entries from the stack synthesis
*/
function printWarnings(stacks: cxapi.SynthesizeResponse) {
let found = false;
function processMessages(stacks: cxapi.SynthesizeResponse): { errors: boolean, warnings: boolean } {
let warnings = false;
let errors = false;
for (const stack of stacks.stacks) {
for (const id of Object.keys(stack.metadata)) {
const metadata = stack.metadata[id];
for (const entry of metadata) {
if (entry.type === 'warning') {
found = true;
warning(`Warning: ${entry.data} (at ${stack.name}:${id})`);

if (argv.trace) {
warning(` ${entry.trace.join('\n ')}`);
}
switch (entry.type) {
case cxapi.WARNING_METADATA_KEY:
warnings = true;
printMessage(warning, 'Warning', id, entry);
break;
case cxapi.ERROR_METADATA_KEY:
errors = true;
printMessage(error, 'Error', id, entry);
break;
case cxapi.INFO_METADATA_KEY:
printMessage(print, 'Info', id, entry);
break;
}
}
}
}
return found;
return { warnings, errors };
}

function printMessage(logFn: (s: string) => void, prefix: string, id: string, entry: cxapi.MetadataEntry) {
logFn(`[${prefix} at ${id}] ${entry.data}`);

if (argv.trace || argv.verbose) {
logFn(` ${entry.trace.join('\n ')}`);
}
}

/**
Expand Down Expand Up @@ -339,7 +354,13 @@ async function initCommandLine() {
continue;
}

if (printWarnings(response) && argv.strict) {
const { errors, warnings } = processMessages(response);

if (errors && !argv.ignoreErrors) {
throw new Error('Found errors');
}

if (argv.strict && warnings) {
throw new Error('Found warnings (--strict mode)');
}

Expand Down