feat(core): context lookup errors are reported to CX app by rix0rrr · Pull Request #3772 · aws/aws-cdk

rix0rrr · 2019-08-23T15:38:38Z

Instead of the toolkit failing and stopping when a context provider
lookup fails, the error is reported to the CDK app, which can then
decide what to do with it.

In practice, it will attach a (localized) error to the Construct tree.
This has the following advantages:

Dependent context resolution can proceed in a stepwise fashion,
without the toolkit stopping at the first failure. For example,
a VPC lookup from a value found in SSM will work.
Stacks in multiple accounts that need context lookup can be used
in a single app with cold context. Without this change they couldn't,
because you could only have credentials for a single account at a
time available so lookup for one of the accounts was bound to fail.

Fixes #3654.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

Instead of the toolkit failing and stopping when a context provider lookup fails, the error is reported to the CDK app, which can then decide what to do with it. In practice, it will attach a (localized) error to the Construct tree. This has the following advantages: * Dependent context resolution can proceed in a stepwise fashion, without the toolkit stopping at the first failure. For example, a VPC lookup from a value found in SSM will work. * Stacks in multiple accounts that need context lookup can be used in a single app with cold context. Without this change they couldn't, because you could only have credentials for a single account at a time available so lookup for one of the accounts was bound to fail. Fixes #3654.

rix0rrr · 2019-08-23T15:38:48Z

TODO: tests and fixing build

mergify · 2019-08-23T15:39:17Z

Pull Request Checklist

Testing

Unit test added (prefer to add a new test rather than modify existing tests)
CLI change? Re-run/add CLI integration tests

Documentation

Inline docs: make sure all public APIs are documented (copy & paste from official AWS docs)
README: update module README
Design: for significant features, follow the design process

Title uses the format type(scope): text

Type: fix, feat, refactor go into CHANGELOG, chore is hidden
Scope: name of the module without the aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
Style: use all lower-case, do not end with a period

Description

Rationale: describe rationale of change and approach taken
Issues: Indicate issues fixed via: fixes #xxx or closes #xxx
Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>

Sensitive Modules (requires 2 PR approvers)

IAM document library (in @aws-cdk/aws-iam)
EC2 security groups and ACLs (in @aws-cdk/aws-ec2)
Grant APIs (if not based on official documentation with a reference)

mergify · 2019-08-23T15:42:48Z

Codebuild (Continuous Integration) build failed for current commits. Please check log and resolve before PR is merged.

mergify · 2019-08-26T09:14:43Z

Codebuild (Continuous Integration) build failed for current commits. Please check log and resolve before PR is merged.

mergify · 2019-08-26T09:14:44Z

Codebuild (Continuous Integration) build failed for current commits. Please check log and resolve before PR is merged.

rix0rrr · 2019-08-26T09:18:21Z

Before I lock in the error and nosave protocols using tests I want a second pair of eyes on this. No point spending effort locking down this behavior if other people are going to make me change it.

Some motivation/explanation:

I want to attach some metadata to context values. The protocol will be that if the value is an object and it has certain magic keys, it will be treated differently. If the value is not an object or does not have the magic keys, it will be treated as a regular, un-annotated value. For now, we don't need anything other than reporting errors, so we don't need to transport a value AND an annotation to the CX app. If we wanted to do that, we would add a key like _contextValue which would contain the actual value. For now, no need for that.
_dontSaveContext is purely CLI-internal: it's a protocol between context providers and the toolkit itself, and is an indication that the value should not be persisted between runs (if we don't add this, errors would never be tried to be looked up again upon subsequent runs, which is incorrect behavior). It happens to be transmitted to the CX app (simply because it is not being actively ignored anywhere) but is unused there.
_providerError is interpreted by context providers as the error that occurred during lookup and attached as metadata.

mergify · 2019-08-26T09:25:09Z

Codebuild (Continuous Integration) build failed for current commits. Please check log and resolve before PR is merged.

packages/@aws-cdk/aws-ssm/lib/parameter.ts

RomainMuller · 2019-08-26T10:09:18Z

packages/@aws-cdk/cx-api/lib/versioning.ts

 * - The request does not have versioning yet, only the response.
 */
-export const CLOUD_ASSEMBLY_VERSION = '0.36.0';
+export const CLOUD_ASSEMBLY_VERSION = '1.6.0';


Isn't this a breaking change?

It certainly means you will have to get a newer CLI version to use this functionality. Is that a breaking change? Does that mean we can never evolve the CX protocol ever again?

I don't think this constitutes a breaking change as long as there's a good error (and there should be)

packages/aws-cdk/lib/settings.ts

RomainMuller · 2019-08-26T10:12:34Z

packages/aws-cdk/lib/settings.ts

+ */
+function stripTransientValues(obj: any) {
+  const ret: any = {};
+  for (const [key, value] of Object.entries(obj)) {


obj might be null or undefined, in which case Object.entries will raise.

It couldn't really, but I think you're telling me to shore up the input type definition.

packages/aws-cdk/lib/settings.ts

packages/@aws-cdk/core/test/test.synthesis.ts

packages/@aws-cdk/cx-api/lib/cxapi.ts

eladb · 2019-08-26T11:14:48Z

packages/@aws-cdk/cx-api/lib/versioning.ts

 * - The request does not have versioning yet, only the response.
 */
-export const CLOUD_ASSEMBLY_VERSION = '0.36.0';
+export const CLOUD_ASSEMBLY_VERSION = '1.6.0';


I don't think this constitutes a breaking change as long as there's a good error (and there should be)

eladb · 2019-08-26T11:16:36Z

packages/@aws-cdk/cx-api/lib/versioning.ts

  if (semver.lt(frameworkVersion, toolkitVersion)) {
    throw new Error(
-      `CDK CLI can only be used with apps created by CDK >= ${CLOUD_ASSEMBLY_VERSION}`);
+      `CDK CLI can only be used with apps created by CDK >= ${toolkitVersion} (got ${frameworkVersion})`);


frameworkVersion and toolkitVersion do not represent the CDK version but rather the manifest version. The addition of "got ${x}" will be misleading (it will report the manifest version of the toolkit/framework instead of the actual version). I would recommend not adding it.

We purposely used CDK version numbers as manifest version numbers so that we can generate error messages like this. The alternatives would be:

# no information The assembly is too new, get a newer CLI. # information that's hard to map to software versions The assembly has version 5, but this CLI can only read assembly version 3. Get a newer CLI.

Both of which are horrible in terms of what a user should do about it, or what versions they should be looking for.

My reservation is only with the got ${frameworkVersion}. It will be confusing because it will not print the framework version but rather the cloud assembly version that the framework presents. Either we should find some other phrasing or avoid this part of the message. The first part is useful.

Oh I see what you mean. Agreed.

eladb · 2019-08-26T11:18:05Z

packages/@aws-cdk/core/lib/context-provider.ts

  }

-  public static getValue(scope: Construct, options: GetContextValueOptions): any {
+  public static getValue(scope: Construct, options: GetContextValueOptions): GetContextValueResult {


I am wondering why did we need to change the return type here. Seems like it doesn't provide any benefit.

Consistency with getKey().

eladb · 2019-08-26T11:24:24Z

packages/aws-cdk/lib/context-providers/index.ts

-    const value = await provider.getValue(missingContext.props);
+    let value;
+    try {
+      value = await provider.getValue(missingContext.props);


I feel like I am missing something here. Why not just set the key to undefined and let the system stabilize itself?

First, because we couldn't tell the difference between a value that hasn't been looked up yet, and a value that we tried to look up but the lookup failed.

Second, because the point is to catch the error and transport it to the CX.

Not sure I understand the response. Why do we need to tell the difference?

At some point I used to tell the difference between failed lookups and untried lookups, that's no longer the case. But we still need to transport the error to the CX, so it can attach a useful error message to the construct tree.

rix0rrr · 2019-09-03T08:32:21Z

Since I didn't get any substantial feedback on the approach, I'm going to assume you all are okay with the direction of this change, and I will start adding unit tests now.

Pull request has been modified.

mergify · 2019-09-03T08:35:49Z

Codebuild (Continuous Integration) build failed for current commits. Please check log and resolve before PR is merged.

mergify · 2019-09-03T09:33:38Z

Codebuild (Continuous Integration) build failed for current commits. Please check log and resolve before PR is merged.

mergify · 2019-09-04T12:01:18Z

Codebuild (Continuous Integration) build failed for current commits. Please check log and resolve before PR is merged.

mergify · 2019-09-13T00:18:42Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

…port-error

aws-cdk-automation · 2019-09-13T10:35:50Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2019-09-16T11:52:34Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

…k into huijbers/context-report-error

aws-cdk-automation · 2019-09-16T13:10:59Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2019-09-18T10:26:57Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2019-09-18T10:59:22Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

Fix breakage introduced by #3782, which changed the wrong string. Update other expectation to match change introduced in #3772.

Instead of the toolkit failing and stopping when a context provider lookup fails, the error is reported to the CDK app, which can then decide what to do with it. In practice, it will attach a (localized) error to the Construct tree. This has the following advantages: * Dependent context resolution can proceed in a stepwise fashion, without the toolkit stopping at the first failure. For example, a VPC lookup from a value found in SSM will work. * Stacks in multiple accounts that need context lookup can be used in a single app with cold context. Without this change they couldn't, because you could only have credentials for a single account at a time available so lookup for one of the accounts was bound to fail. Fixes #3654.

* chore: fix CLI integration tests Fix breakage introduced by #3782, which changed the wrong string. Update other expectation to match change introduced in #3772. * Use substitutions in error message * Rewrite how CLI tests are being run * Extract environment setup from tests * Need to not ignore integ test Javascript files

mergify bot added the automerge-enabled label Aug 23, 2019

Update test snapshots

8b3c093

Another stray assembly version in tests

8c41f34

RomainMuller reviewed Aug 26, 2019

View reviewed changes

eladb previously requested changes Aug 26, 2019

View reviewed changes

rix0rrr self-assigned this Aug 26, 2019

Rename some things and add types

e2e57b2

Undo version change

1fba87c

Remove 'got' message

c026ce3

NGL321 removed the automerge-enabled label Sep 13, 2019

mergify bot added the automerge-enabled label Sep 13, 2019

NGL321 removed the automerge-enabled label Sep 13, 2019

Merge remote-tracking branch 'origin/master' into huijbers/context-re…

65bb574

…port-error

Merge branch 'master' into huijbers/context-report-error

27e1829

rix0rrr requested a review from shivlaks as a code owner September 16, 2019 11:07

Add tests

8106182

rix0rrr added the pr/do-not-merge This PR should not be merged at this time. label Sep 16, 2019

Merge branch 'huijbers/context-report-error' of github.com:aws/aws-cd…

d20abd1

…k into huijbers/context-report-error

rix0rrr requested review from RomainMuller and eladb September 17, 2019 09:27

eladb approved these changes Sep 18, 2019

View reviewed changes

Merge branch 'master' into huijbers/context-report-error

78a8224

rix0rrr added the effort/medium Medium work item – several days of effort label Sep 18, 2019

Merge branch 'master' into huijbers/context-report-error

db01b39

rix0rrr merged commit b0267e4 into master Sep 20, 2019

rix0rrr deleted the huijbers/context-report-error branch September 20, 2019 09:14

rix0rrr added a commit that referenced this pull request Sep 23, 2019

chore: fix CLI integration tests

ee90fba

Fix breakage introduced by #3782, which changed the wrong string. Update other expectation to match change introduced in #3772.

rix0rrr mentioned this pull request Sep 23, 2019

chore: fix CLI integration tests #4198

Merged

NGL321 added the contribution/core This is a PR that came from AWS. label Sep 27, 2019

Conversation

rix0rrr commented Aug 23, 2019

Uh oh!

rix0rrr commented Aug 23, 2019

Uh oh!

mergify bot commented Aug 23, 2019

Pull Request Checklist

Uh oh!

mergify bot commented Aug 23, 2019

Uh oh!

mergify bot commented Aug 26, 2019

Uh oh!

mergify bot commented Aug 26, 2019

Uh oh!

rix0rrr commented Aug 26, 2019

Uh oh!

mergify bot commented Aug 26, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rix0rrr Sep 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rix0rrr commented Sep 3, 2019

Uh oh!

mergify bot commented Sep 3, 2019

Uh oh!

mergify bot commented Sep 3, 2019

Uh oh!

mergify bot commented Sep 4, 2019

Uh oh!

mergify bot commented Sep 13, 2019

Uh oh!

aws-cdk-automation commented Sep 13, 2019

AWS CodeBuild CI Report

Uh oh!

aws-cdk-automation commented Sep 16, 2019

AWS CodeBuild CI Report

Uh oh!

aws-cdk-automation commented Sep 16, 2019

AWS CodeBuild CI Report

Uh oh!

aws-cdk-automation commented Sep 18, 2019

AWS CodeBuild CI Report

Uh oh!

aws-cdk-automation commented Sep 18, 2019

rix0rrr Sep 3, 2019 •

edited

Loading