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

Toolkit: support profiles #517

Merged
merged 3 commits into from
Aug 7, 2018
Merged

Toolkit: support profiles #517

merged 3 commits into from
Aug 7, 2018

Conversation

rix0rrr
Copy link
Contributor

@rix0rrr rix0rrr commented Aug 7, 2018

This adds support for AWS profiles to the CDK toolkit.

At the same time, it overhauls how the AWS SDK is configured. The
configuration via environment variables set at just the right time
is removed, and we reimplement some parts of the SDK in an
AWS CLI-compatible way to get a consistent view on the account ID
and region based on the provided configuration.

Fixes a bug in the AWS STS call where it would do two default
credential lookups (down to one now).

Fixes #480.

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

This adds support for AWS profiles to the CDK toolkit.

At the same time, it overhauls how the AWS SDK is configured. The
configuration via environment variables set at just the right time
is removed, and we reimplement some parts of the SDK in an
AWS CLI-compatible way to get a consistent view on the account ID
and region based on the provided configuration.

Fixes a bug in the AWS STS call where it would do two default
credential lookups (down to one now).

Fixes #480.
@rix0rrr rix0rrr requested review from RomainMuller and eladb August 7, 2018 10:32
@@ -49,6 +46,7 @@ async function parseCommandLineArguments() {
.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' })
.option('profile', { type: 'string', desc: 'Use the indicated AWS profile' })
Copy link
Contributor

Choose a reason for hiding this comment

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

“...to obtain information for the default environment”

const aws = new SDK();
const aws = new SDK(argv.profile);
// tslint:disable-next-line:no-console
console.log("Account: ", await aws.defaultAccount(), " region: ", aws.defaultRegion());
Copy link
Contributor

Choose a reason for hiding this comment

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

Use logger

// Find the package.json from the main toolkit
const pkg = (require.main as any).require('../package.json');
this.userAgent = `${pkg.name}/${pkg.version}`;

// tslint:disable-next-line:no-console
console.log(new Error().stack);
Copy link
Contributor

Choose a reason for hiding this comment

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

Delete

console.log(new Error().stack);

// tslint:disable-next-line:no-console
console.log('Profile', profile);
Copy link
Contributor

Choose a reason for hiding this comment

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

Logger

const configFile = new SharedIniFile(toCheck.shift());
const section = configFile.getProfile(profile);
region = section && section.region;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Indentation

* variables to be used to determine the region.
*/
function getCLICompatibleDefaultRegion(profile: string | undefined): string | undefined {
let region = process.env.AWS_REGION || process.env.AMAZON_REGION ||
Copy link
Contributor

Choose a reason for hiding this comment

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

I’d move this just before the loop so it’s clear that these take precedence

* A reimplementation of JS AWS SDK's SharedIniFile class
*
* We need that class to parse the ~/.aws/config file to determine the correct
* region at runtime, but unfortunately it is private upstream.
Copy link
Contributor

Choose a reason for hiding this comment

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

Open issue?

Copy link
Contributor

Choose a reason for hiding this comment

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

Mention that we are using an undocumented api in the js sdk to actually parse the ini file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I definitely wanted to open an issue, but I'd like to land this first so that I can point them to the code and say "see, this is what we've had to do to work around the API deficiencies".

Pointing to code is easier than describing :)

@rix0rrr rix0rrr merged commit 6846b60 into master Aug 7, 2018
@rix0rrr rix0rrr deleted the huijbers/cli-profiles branch August 7, 2018 16:14
@eladb eladb mentioned this pull request Aug 8, 2018
RomainMuller pushed a commit that referenced this pull request Aug 8, 2018
* Bump
* Added a `bump.sh` script ;-)

Fixes #467 (via aws/jsii#139)
Fixes #503 (via #517)
Fixes #502 (via #490)
@NGL321 NGL321 added the contribution/core This is a PR that came from AWS. label Sep 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Does cdk support IAM profile
3 participants