Context providers: return dummy values if env is undefined, but emit …

…errors to fail "cdk synth" (#227) Throwing if "env" is undefined makes it hard to unit test stacks that use context providers. It requires dummy values for account and region, which is boilerplate and non-intuitive. Since we already have a concept of dummy context values, this change short-circuits context resolution and returns the dummy values in case the stack's env is undefined. To ensure dummy values are not used in production, when a dummy value is returned, we attach a metadata entry `aws:cdk:error` to the construct with a message explaining that context cannot be resolved. The toolkit picks up these metadata entries and will fail synthesis, unless --ignore-errors is set. INFO messages can also be emitted now using `context.addInfo(message)`. The metadata keys for info, warning and errors are normalized to use the "aws:cdk" prefix, and formally defined in cxapi.
aws · Jul 19, 2018 · d9e9726 · d9e9726
1 parent 0763282
commit d9e9726
Show file tree

Hide file tree

Showing 7 changed files with 167 additions and 27 deletions.
diff --git a/packages/@aws-cdk/core/lib/context.ts b/packages/@aws-cdk/core/lib/context.ts
@@ -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) {
@@ -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);
 
@@ -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`);
@@ -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`);
+            return undefined;
         }
 
         return [account, region];
@@ -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;
 }
diff --git a/packages/@aws-cdk/core/lib/core/construct.ts b/packages/@aws-cdk/core/lib/core/construct.ts
@@ -1,3 +1,4 @@
+import cxapi = require('@aws-cdk/cx-api');
 export const PATH_SEP = '/';
 
 /**
@@ -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);
     }
 
     /**

diff --git a/packages/@aws-cdk/core/test/core/test.construct.ts b/packages/@aws-cdk/core/test/core/test.construct.ts
@@ -1,3 +1,4 @@
+import cxapi = require('@aws-cdk/cx-api');
 import { Test } from 'nodeunit';
 import { Construct, Root } from '../../lib';
 
@@ -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');

diff --git a/packages/@aws-cdk/core/test/test.context.ts b/packages/@aws-cdk/core/test/test.context.ts
@@ -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) {
@@ -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 {

diff --git a/packages/@aws-cdk/cx-api/lib/cxapi.ts b/packages/@aws-cdk/cx-api/lib/cxapi.ts
@@ -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';
diff --git a/packages/@aws-cdk/ec2/test/test.fleet.ts b/packages/@aws-cdk/ec2/test/test.fleet.ts
@@ -68,7 +68,7 @@ export = {
                   "IamInstanceProfile": {
                     "Ref": "MyFleetInstanceProfile70A58496"
                   },
-                  "ImageId": "",
+                  "ImageId": "dummy",
                   "InstanceType": "m4.micro",
                   "SecurityGroups": [
                     {
@@ -194,7 +194,7 @@ export = {
                   "IamInstanceProfile": {
                     "Ref": "MyFleetInstanceProfile70A58496"
                   },
-                  "ImageId": "",
+                  "ImageId": "dummy",
                   "InstanceType": "m4.micro",
                   "SecurityGroups": [
                     {

diff --git a/packages/aws-cdk/bin/cdk.ts b/packages/aws-cdk/bin/cdk.ts
@@ -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)
@@ -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  ')}`);
+        }
     }
 
     /**
@@ -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)');
             }