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.

Sign up for GitHub

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

fix(cli): failure if account cache is malformed #10887

Merged
merged 3 commits into from
Oct 16, 2020
Merged

fix(cli): failure if account cache is malformed #10887

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2193b72
fix(cli): failure if account cache is malformed
rix0rrr Oct 15, 2020
fb22261
Update cdk.ts
rix0rrr Oct 16, 2020
c0bbcad
Merge branch 'master' into huijbers/survive-malformed-cache
rix0rrr Oct 16, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions packages/aws-cdk/bin/cdk.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,6 +229,9 @@ async function initCommandLine() {
return cli.list(args.STACKS, { long: args.long });

case 'diff':
console.log(configuration.context);
Copy link
Contributor

Choose a reason for hiding this comment

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

Intentional...?

console.log(configuration.context.all);
console.log(configuration.context.get(cxapi.ENABLE_DIFF_NO_FAIL));
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume these were for diagnostic purposes and should be removed.

const enableDiffNoFail = isFeatureEnabled(configuration, cxapi.ENABLE_DIFF_NO_FAIL);
return cli.diff({
stackNames: args.STACKS,
Expand Down
3 changes: 3 additions & 0 deletions packages/aws-cdk/lib/api/aws-auth/account-cache.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,9 @@ export class AccountAccessKeyCache {
// File doesn't exist or is not readable. This is a cache,
// pretend we successfully loaded an empty map.
if (e.code === 'ENOENT' || e.code === 'EACCES') { return {}; }
// File is not JSON, could be corrupted because of concurrent writes.
// Again, an empty cache is fine.
if (e instanceof SyntaxError) { return {}; }
Copy link
Contributor

@iliapolo iliapolo Oct 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Im actually not sure this is because of the parallelism of the integ tests, i remember @rix0rrr and I encountered this issue a few months ago and ended up in a rabbit hole of sdk code.

What if we set the CDK_HOME env variable to be unique per execution before we resort to this? I'm not strictly opposed to it but i feel like we are doing it for the wrong reasons now and we might be missing something else.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This specifically has nothing to do with the SDK. This file is completely under our control, written and read by us only.

I can't imagine it being anything other than the parallelism we introduced recently, given that I haven't seen this error crop up before that.

I'm open to another explanation of the available facts if you have them, and I'll agree it breaks my mental model of how I thought NodeJS worked as well. Even concurrency isn't the ultimate root cause, I'm not concerned about this fix too much, which is why I'm hesitant to do more complicated workarounds.

This is all to say: I say we ship it :).

Copy link
Contributor Author

@rix0rrr rix0rrr Oct 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Uh umm. It might also be this:

aws-cdk/packages/aws-cdk/lib/api/aws-auth/account-cache.ts

Lines 92 to 93 in ddff369

await fs.ensureFile(this.cacheFile);
await fs.writeJson(this.cacheFile, map, { spaces: 2 });

But even given that I don't mind too much to put this workaround in. It will also catch malformed caches for other reasons (outside our control).

Copy link
Contributor

Choose a reason for hiding this comment

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

Ship and may the force be with you 👍

throw e;
}
}
Expand Down
11 changes: 11 additions & 0 deletions packages/aws-cdk/test/account-cache.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,3 +111,14 @@ test(`cache is nuked if it exceeds ${AccountAccessKeyCache.MAX_ENTRIES} entries`
await nukeCache(cacheDir);
}
});

test('cache pretends to be empty if cache file does not contain JSON', async() => {
const { cacheDir, cacheFile, cache } = await makeCache();
try {
await fs.writeFile(cacheFile, '');

await expect(cache.get('abc')).resolves.toEqual(undefined);
} finally {
await nukeCache(cacheDir);
}
});