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

Store default account in disk cache #220

Merged
merged 3 commits into from
Jul 4, 2018
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
49 changes: 49 additions & 0 deletions packages/aws-cdk/lib/api/util/account-cache.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
import * as fs from 'fs-extra';
import * as os from 'os';
import * as path from 'path';

/**
* Disk cache which maps access key IDs to account IDs.
* Usage:
* cache.get(accessKey) => accountId | undefined
* cache.put(accessKey, accountId)
*/
export class AccountAccessKeyCache {
private readonly cacheFile: string;

/**
* @param filePath Path to the cache file
*/
constructor(filePath?: string) {
this.cacheFile = filePath || path.join(os.homedir(), '.cdk/cache/accounts.json');
Copy link
Contributor

Choose a reason for hiding this comment

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

If you want to use path.join, you should do it all the way...

path.join(os.homedir(), '.cdk', 'cache', 'accounts.json');

Otherwise it's a ridiculous way to write:

`${os.homedir()}/.cdk/cache/accounts.json`

}

/** Get the account ID from an access key or undefined if not in cache */
public async get(accessKeyId: string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

That is ridiculously loosely typed (for loadMap returns any). I would add an explicit typing in there, that this returns string | undefined. I guess otherwise you could make loadMap return { [accessKeyId: string]: string | undefined } to the same effect.

const map = await this.loadMap();
return map[accessKeyId];
}

/** Put a mapping betweenn access key and account ID */
public async put(accessKeyId: string, accountId: string) {
const map = await this.loadMap();
map[accessKeyId] = accountId;
await this.saveMap(map);
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we going to take any precautions to prevent this map from growing without bounds? Session credentials will have a new access key every time, and this file may become unwieldy after a while.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rix0rrr are you okay with me punting this for later or you feel it's important to take care of it now? (I am on the fence)

Copy link
Contributor

Choose a reason for hiding this comment

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

Don't like kicking work down the road in this way, I'd prefer finishing the feature so we don't have to come back to it.

If we do 'wipe on # of entries limit' (let's say 100) then it should be 5 mins to implement, no?

}

private async loadMap() {
if (!(await fs.pathExists(this.cacheFile))) {
return { };
}

return await fs.readJson(this.cacheFile);
}

private async saveMap(map: { [accessKeyId: string]: string }) {
if (!(await fs.pathExists(this.cacheFile))) {
await fs.mkdirs(path.dirname(this.cacheFile));
}

await fs.writeJson(this.cacheFile, map, { spaces: 2 });
}
}
31 changes: 28 additions & 3 deletions packages/aws-cdk/lib/api/util/sdk.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
import { Environment} from '@aws-cdk/cx-api';
import { CloudFormation, config, CredentialProviderChain , EC2, S3, SSM, STS } from 'aws-sdk';
import { CloudFormation, config, CredentialProviderChain, EC2, S3, SSM, STS } from 'aws-sdk';
Copy link
Contributor

Choose a reason for hiding this comment

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

👍🏻

import { debug } from '../../logging';
import { PluginHost } from '../../plugin';
import { CredentialProviderSource, Mode } from '../aws-auth/credentials';
import { AccountAccessKeyCache } from './account-cache';

/**
* Source for SDK client objects
Expand All @@ -17,6 +18,7 @@ export class SDK {
private defaultAccountFetched = false;
private defaultAccountId?: string = undefined;
private readonly userAgent: string;
private readonly accountCache = new AccountAccessKeyCache();

constructor() {
// Find the package.json from the main toolkit
Expand Down Expand Up @@ -70,11 +72,33 @@ export class SDK {

private async lookupDefaultAccount() {
try {
debug('Resolving default credentials');
const chain = new CredentialProviderChain();
const creds = await chain.resolvePromise();
const accessKeyId = creds.accessKeyId;
if (!accessKeyId) {
throw new Error('Unable to resolve AWS credentials');
Copy link
Contributor

Choose a reason for hiding this comment

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

configure with "aws configure"?

}

// try to get account ID based on this access key ID from disk.
const cached = await this.accountCache.get(creds.accessKeyId);
if (cached) {
debug(`Retrieved account ID ${cached} from disk cache`);
return cached;
}

// if we don't have one, resolve from STS and store in cache.
debug('Looking up default account ID from STS');
const result = await new STS().getCallerIdentity().promise();
return result.Account;
const accountId = result.Account;
if (!accountId) {
debug('STS didn\'t return an account ID');
return undefined;
}
await this.accountCache.put(accessKeyId, accountId);
Copy link
Contributor

Choose a reason for hiding this comment

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

I see this can be easy to forget, and then the cache would become useless...

Would be nice if the cache class instead had a:

export class AccountCache {
  // ...

  /**
   * @param loader an object to be used for resolving the account ID if it is not already in the cache.
   */
  public fetch(key: string, loader: IAccountIdResolver, reload: boolean = false) {
    if (!reload) {
      const cached = this.get(key);
      if (cached) { return cached; }
    }
    const loaded = loader.load();
    this.put(key, loaded);
    return loaded;
  }
}

return accountId;
} catch (e) {
debug('Unable to retrieve default account from STS:', e);
debug('Unable to determine the default AWS account (use "aws configure" to set up):', e);
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a bunch of ways here that you could have configured it w/ aws configure and you get there... You'll then be giving the user a pretty direct "steps to fix" that they've already followed, and they'll probably end up looking something like:

FUUUUUUUUU

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to

Unable to determine the default AWS account (did you configure "aws configure"?)

Not much better, but it's now it's less directive and more of a hint.

Copy link
Contributor

Choose a reason for hiding this comment

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

I definitely prefer that.

return undefined;
}
}
Expand Down Expand Up @@ -108,3 +132,4 @@ export class SDK {
throw new Error(`Need to perform AWS calls for account ${awsAccountId}, but no credentials found. Tried: ${sourceNames}.`);
}
}

49 changes: 49 additions & 0 deletions packages/aws-cdk/test/test.account-cache.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
import * as fs from 'fs-extra';
import { ICallbackFunction, Test } from 'nodeunit';
import * as path from 'path';
import { AccountAccessKeyCache } from '../lib/api/util/account-cache';

export = {
'setUp'(cb: ICallbackFunction) {
const self = this as any;
fs.mkdtemp('/tmp/account-cache-test').then(dir => {
self.file = path.join(dir, 'cache.json');
self.cache = new AccountAccessKeyCache(self.file);
return cb();
});
},

'tearDown'(cb: ICallbackFunction) {
const self = this as any;
fs.remove(path.dirname(self.file)).then(cb);
},

async 'get(k) when cache is empty'(test: Test) {
const self = this as any;
const cache: AccountAccessKeyCache = self.cache;
test.equal(await cache.get('foo'), undefined, 'get returns undefined');
test.equal(await fs.pathExists(self.file), false, 'cache file is not created');
test.done();
},

async 'put(k,v) and then get(k)'(test: Test) {
const self = this as any;
const cache: AccountAccessKeyCache = self.cache;

await cache.put('key', 'value');
await cache.put('boo', 'bar');
test.deepEqual(await cache.get('key'), 'value', '"key" is mapped to "value"');

// create another cache instance on the same file, should still work
const cache2 = new AccountAccessKeyCache(self.file);
test.deepEqual(await cache2.get('boo'), 'bar', '"boo" is mapped to "bar"');

// whitebox: read the file
test.deepEqual(await fs.readJson(self.file), {
key: 'value',
boo: 'bar'
});

test.done();
}
};