Store default account in disk cache #220
Conversation
Given an access key ID, the account ID is fixed. This means that we don't need to look up the default account every time the toolkit is used. This change will store the mapping between access key and account ID in `~/.cdk/cache/accounts.json` and will fetch the account ID from there if it exists.
| * @param filePath Path to the cache file | ||
| */ | ||
| constructor(filePath?: string) { | ||
| this.cacheFile = filePath || path.join(os.homedir(), '.cdk/cache/accounts.json'); |
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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.
| @@ -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'; | |||
packages/aws-cdk/lib/api/util/sdk.ts
Outdated
| const creds = await chain.resolvePromise(); | ||
| const accessKeyId = creds.accessKeyId; | ||
| if (!accessKeyId) { | ||
| throw new Error('Unable to resolve AWS credentials'); |
There was a problem hiding this comment.
configure with "aws configure"?
packages/aws-cdk/lib/api/util/sdk.ts
Outdated
| } 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); |
There was a problem hiding this comment.
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:
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I definitely prefer that.
packages/aws-cdk/lib/api/util/sdk.ts
Outdated
| debug('STS didn\'t return an account ID'); | ||
| return undefined; | ||
| } | ||
| await this.accountCache.put(accessKeyId, accountId); |
There was a problem hiding this comment.
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;
}
}
| public async put(accessKeyId: string, accountId: string) { | ||
| const map = await this.loadMap(); | ||
| map[accessKeyId] = accountId; | ||
| await this.saveMap(map); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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)
There was a problem hiding this comment.
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?
Given an access key ID, the account ID is fixed.
This means that we don't need to look up the default
account every time the toolkit is used.
This change will store the mapping between access key
and account ID in
~/.cdk/cache/accounts.jsonand will fetch the account ID from there if it exists.
Fixes #228