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

Adopt SDK-standard behavior when no environment is specified #128

Merged
merged 3 commits into from
Jun 19, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
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
5 changes: 3 additions & 2 deletions packages/@aws-cdk/assert/lib/integ-helpers.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
// Helper functions for integration tests
import { DEFAULT_ACCOUNT_CONTEXT_KEY, DEFAULT_REGION_CONTEXT_KEY } from '@aws-cdk/cx-api';
import { spawnSync } from 'child_process';
import fs = require('fs');
import path = require('path');
Expand Down Expand Up @@ -80,8 +81,8 @@ export class IntegrationTest {
// Default context we run all integ tests with, so they don't depend on the
// account of the exercising user.
export const STATIC_TEST_CONTEXT = {
"default-account": "12345678",
"default-region": "test-region",
[DEFAULT_ACCOUNT_CONTEXT_KEY]: "12345678",
[DEFAULT_REGION_CONTEXT_KEY]: "test-region",
"availability-zones:12345678:test-region": [ "test-region-1a", "test-region-1b", "test-region-1c" ],
"ssm:12345678:test-region:/aws/service/ami-amazon-linux-latest/amzn-ami-hvm-x86_64-gp2": "ami-1234",
};
Expand Down
11 changes: 6 additions & 5 deletions packages/@aws-cdk/core/test/test.environment.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { DEFAULT_ACCOUNT_CONTEXT_KEY, DEFAULT_REGION_CONTEXT_KEY } from '@aws-cdk/cx-api';
import { Test } from 'nodeunit';
import { App, Stack } from '../lib';

Expand All @@ -10,11 +11,11 @@ export = {
test.done();
},

'Default account and region can be set in contrext (`default-account` and `default-region`)'(test: Test) {
'Default account and region can be set in context (`aws:cdk:toolkit:default-account` and `aws:cdk:toolkit:default-region`)'(test: Test) {
const app = new App();

app.setContext('default-account', 'my-default-account');
app.setContext('default-region', 'my-default-region');
app.setContext(DEFAULT_ACCOUNT_CONTEXT_KEY, 'my-default-account');
app.setContext(DEFAULT_REGION_CONTEXT_KEY, 'my-default-region');

const stack = new Stack(app);

Expand All @@ -27,8 +28,8 @@ export = {
'If only `env.region` or `env.account` are specified, defaults will be used for the other'(test: Test) {
const app = new App();

app.setContext('default-account', 'my-default-account');
app.setContext('default-region', 'my-default-region');
app.setContext(DEFAULT_ACCOUNT_CONTEXT_KEY, 'my-default-account');
app.setContext(DEFAULT_REGION_CONTEXT_KEY, 'my-default-region');

const stack1 = new Stack(app, 'S1', { env: { region: 'only-region' } });
const stack2 = new Stack(app, 'S2', { env: { account: 'only-account' } });
Expand Down
4 changes: 2 additions & 2 deletions packages/@aws-cdk/cx-api/lib/cxapi.ts
Original file line number Diff line number Diff line change
Expand Up @@ -88,9 +88,9 @@ export type StackMetadata = { [path: string]: MetadataEntry[] };
/**
* Context parameter for the default AWS account to use if a stack's environment is not set.
*/
export const DEFAULT_ACCOUNT_CONTEXT_KEY = 'default-account';
export const DEFAULT_ACCOUNT_CONTEXT_KEY = 'aws:cdk:toolkit:default-account';

/**
* Context parameter for the default AWS region to use if a stack's environment is not set.
*/
export const DEFAULT_REGION_CONTEXT_KEY = 'default-region';
export const DEFAULT_REGION_CONTEXT_KEY = 'aws:cdk:toolkit:default-region';
21 changes: 14 additions & 7 deletions packages/aws-cdk/bin/cdk.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
#!/usr/bin/env node
import 'source-map-support/register';

// Ensure the AWS SDK is properly initialized before anything else.
import '../lib/api/util/sdk-load-aws-config';

import * as cxapi from '@aws-cdk/cx-api';
import { deepMerge, isEmpty, partition } from '@aws-cdk/util';
import { exec, spawn } from 'child_process';
Expand Down Expand Up @@ -91,8 +94,8 @@ async function initCommandLine() {
'ssm': new contextplugins.SSMContextProviderPlugin(aws),
};

const userConfig = new Settings().load(PER_USER_DEFAULTS);
const projectConfig = new Settings().load(DEFAULTS);
const userConfig = await new Settings().load(PER_USER_DEFAULTS);
const projectConfig = await new Settings().load(DEFAULTS);
const commandLineArguments = argumentsToSettings();
const renames = parseRenames(argv.rename);

Expand Down Expand Up @@ -341,7 +344,7 @@ async function initCommandLine() {
await contextplugins.provideContextValues(allMissing, projectConfig, availableContextProviders);

// Cache the new context to disk
projectConfig.save(DEFAULTS);
await projectConfig.save(DEFAULTS);
continue;
}

Expand Down Expand Up @@ -401,7 +404,8 @@ async function initCommandLine() {
for (const stack of synthesizedStacks) {
if (stackIds.length !== 1) { highlight(stack.name); }
if (!stack.environment) {
throw new Error(`Stack ${stack.name} has no environment`);
// tslint:disable-next-line:max-line-length
throw new Error(`Stack ${stack.name} does not define an environment, and AWS credentails could not be obtained from standard locations.`);
}
const toolkitInfo = await loadToolkitInfo(stack.environment, aws, toolkitStackName);
const deployName = renames.finalName(stack.name);
Expand All @@ -414,12 +418,12 @@ async function initCommandLine() {

try {
const result = await deployStack(stack, aws, toolkitInfo, deployName);
const message = result.noOp ? ' ✅ Stack was already up-to-date!'
: ' ✅ Deployment of stack %s completed successfully!';
const message = result.noOp ? ` ✅ Stack was already up-to-date, it has ARN ${blue(result.stackArn)}`
: ` ✅ Deployment of stack %s completed successfully, it has ARN ${blue(result.stackArn)}`;
success(message, blue(stack.name));
for (const name of Object.keys(result.outputs)) {
const value = result.outputs[name];
print('Output %s = %s', blue(name), green(value));
print('%s.%s = %s', blue(deployName), blue(name), green(value));
}
} catch (e) {
error(' ❌ Deployment of stack %s failed: %s', blue(stack.name), e);
Expand Down Expand Up @@ -593,6 +597,9 @@ async function initCommandLine() {
const parts = assignment.split('=', 2);
if (parts.length === 2) {
debug('CLI argument context: %s=%s', parts[0], parts[1]);
if (parts[0].match(/^aws:.+/)) {
throw new Error(`User-provided context cannot use keys prefixed with 'aws:', but ${parts[0]} was provided.`);
}
context[parts[0]] = parts[1];
} else {
warning('Context argument is not an assignment (key=value): %s', assignment);
Expand Down
5 changes: 3 additions & 2 deletions packages/aws-cdk/lib/api/deploy-stack.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ type TemplateBodyParameter = {
export interface DeployStackResult {
readonly noOp: boolean;
readonly outputs: { [name: string]: string };
readonly stackArn: string;
}

export async function deployStack(stack: SynthesizedStack,
Expand Down Expand Up @@ -60,7 +61,7 @@ export async function deployStack(stack: SynthesizedStack,
if (!changeSetDescription || !changeSetDescription.Changes || changeSetDescription.Changes.length === 0) {
debug('No changes are to be performed on %s, assuming success.', deployName);
await cfn.deleteChangeSet({ StackName: deployName, ChangeSetName: changeSetName }).promise();
return { noOp: true, outputs: await getStackOutputs(cfn, deployName) };
return { noOp: true, outputs: await getStackOutputs(cfn, deployName), stackArn: changeSet.StackId! };
}

debug('Initiating execution of changeset %s on stack %s', changeSetName, deployName);
Expand All @@ -70,7 +71,7 @@ export async function deployStack(stack: SynthesizedStack,
await waitForStack(cfn, deployName);
if (monitor) { monitor.stop(); }
debug('Stack %s has completed updating', deployName);
return { noOp: false, outputs: await getStackOutputs(cfn, deployName) };
return { noOp: false, outputs: await getStackOutputs(cfn, deployName), stackArn: changeSet.StackId! };
}

async function getStackOutputs(cfn: CloudFormation, stackName: string): Promise<{ [name: string]: string }> {
Expand Down
33 changes: 33 additions & 0 deletions packages/aws-cdk/lib/api/util/sdk-load-aws-config.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
/**
* IMPORTANT: This **must** be required _before_ 'aws-sdk' is.
*
* This ensures the correct environment is set-up so the AWS SDK properly
* loads up configruation stored in the shared credentials file (usually
* found at ~/.aws/credentials) and the aws config file (usually found at
* ~/.aws/config), if either is present.
Copy link
Contributor

Choose a reason for hiding this comment

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

Add the URL of this pull request here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK

*
* @see https://github.com/awslabs/aws-cdk/pull/128
*/

import * as fs from 'fs';
import * as os from 'os';
import * as path from 'path';

const sharedCredentialsFile =
process.env.AWS_SHARED_CREDENTIALS_FILE ? process.env.AWS_SHARED_CREDENTIALS_FILE
: path.join(os.homedir(), '.aws', 'credentials');
const awsConfigFile =
process.env.AWS_CONFIG_FILE ? process.env.AWS_CONFIG_FILE
: path.join(os.homedir(), '.aws', 'config');

if (fs.existsSync(awsConfigFile) && !fs.existsSync(sharedCredentialsFile)) {
/*
* Write an empty credentials file if there's a config file, otherwise the SDK will simply bail out,
* since the credentials file is loaded before the config file is.
*/
fs.writeFileSync(sharedCredentialsFile, '');
}
if (fs.existsSync(sharedCredentialsFile)) {
// Ensures that region is loaded from ~/.aws/config (https://github.com/aws/aws-sdk-js/pull/1391)
process.env.AWS_SDK_LOAD_CONFIG = '1';
}
27 changes: 2 additions & 25 deletions packages/aws-cdk/lib/api/util/sdk.ts
Original file line number Diff line number Diff line change
@@ -1,15 +1,3 @@
import * as fs from 'fs';
import * as os from 'os';
import * as path from 'path';

if (fs.existsSync(path.join(os.homedir(), ".aws", "credentials")) && fs.existsSync(path.join(os.homedir(), ".aws", "config"))) {
// Ensures that region is loaded from ~/.aws/config (https://github.com/aws/aws-sdk-js/pull/1391)

// Only set this value if if the requisite files exist, otherwise this is
// just going to throw an unhelpful error.
process.env.AWS_SDK_LOAD_CONFIG = '1';
}

import { Environment} from '@aws-cdk/cx-api';
import { CloudFormation, config, CredentialProviderChain , EC2, S3, SSM, STS } from 'aws-sdk';
import { debug } from '../../logging';
Expand All @@ -28,12 +16,9 @@ import { CredentialProviderSource, Mode } from '../aws-auth/credentials';
export class SDK {
private defaultAccountFetched = false;
private defaultAccountId?: string = undefined;
private credentialSources: CredentialProviderSource[];
private readonly userAgent: string;

constructor() {
this.credentialSources = PluginHost.instance.credentialProviderSources;

// Find the package.json from the main toolkit
const pkg = (require.main as any).require('../package.json');
this.userAgent = `${pkg.name}/${pkg.version}`;
Expand Down Expand Up @@ -72,15 +57,7 @@ export class SDK {
}

public defaultRegion() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we need to read the account ID anyway by issuing a request to AWS, wouldn't it make more sense to read the region this way as well and avoid duplicating the SDKs behavior?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Which region to I configure in order to build the service endpoint URL that I will call in order to get the region? OH WAIT!

Copy link
Contributor

Choose a reason for hiding this comment

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

Obvsiouly you shouldn't configure a region. I believe the SDKs have a default region and we want the toolkit to behave exactly like them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah right. It didn't work when I tested but that was because of initialization order. Now only using config.region.

if (process.env.AWS_DEFAULT_REGION) {
debug('Obtaining default region from environment');
return process.env.AWS_DEFAULT_REGION;
}
if (config.region) {
debug('Obtaining default region from AWS configuration');
return config.region;
}
return undefined;
return config.region;
}

public async defaultAccount() {
Expand Down Expand Up @@ -113,7 +90,7 @@ export class SDK {
const triedSources: CredentialProviderSource[] = [];

// Otherwise, inspect the various credential sources we have
for (const source of this.credentialSources) {
for (const source of PluginHost.instance.credentialProviderSources) {
if (!(await source.isAvailable())) {
debug('Credentials source %s is not available, ignoring it.', source.name);
continue;
Expand Down
39 changes: 32 additions & 7 deletions packages/aws-cdk/lib/settings.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
import { deepClone, deepGet, deepMerge, deepSet } from '@aws-cdk/util';
import * as fs from 'fs-extra';
import * as os from 'os';
import * as fs_path from 'path';
import { warning } from './logging';

type SettingsMap = {[key: string]: any};
export type SettingsMap = {[key: string]: any};

export class Settings {
public static mergeAll(...settings: Settings[]): Settings {
Expand All @@ -19,20 +21,43 @@ export class Settings {
this.settings = settings || {};
}

public load(fileName: string) {
public async load(fileName: string) {
this.settings = {};

const expanded = expandHomeDir(fileName);
if (fs.existsSync(expanded)) {
this.settings = JSON.parse(fs.readFileSync(expanded, { encoding: 'utf-8' }));
if (await fs.pathExists(expanded)) {
this.settings = await fs.readJson(expanded);
}

// See https://github.com/awslabs/aws-cdk/issues/59
prohibitContextKey(this, 'default-account');
prohibitContextKey(this, 'default-region');
warnAboutContextKey(this, 'aws:');

return this;

function prohibitContextKey(self: Settings, key: string) {
if (!self.settings.context) { return; }
if (key in self.settings.context) {
// tslint:disable-next-line:max-line-length
throw new Error(`The 'context.${key}' key was found in ${fs_path.resolve(fileName)}, but it is no longer supported. Please remove it.`);
}
}

function warnAboutContextKey(self: Settings, prefix: string) {
if (!self.settings.context) { return; }
for (const contextKey of Object.keys(self.settings.context)) {
if (contextKey.startsWith(prefix)) {
// tslint:disable-next-line:max-line-length
warning(`A reserved context key ('context.${prefix}') key was found in ${fs_path.resolve(fileName)}, it might cause surprising behavior and should be removed.`);
}
}
}
}

public save(fileName: string) {
public async save(fileName: string) {
const expanded = expandHomeDir(fileName);
fs.writeFileSync(expanded, JSON.stringify(this.settings, undefined, 2), { encoding: 'utf-8' });
await fs.writeJson(expanded, this.settings, { spaces: 2 });

return this;
}
Expand All @@ -57,7 +82,7 @@ export class Settings {

function expandHomeDir(x: string) {
if (x.startsWith('~')) {
return os.homedir + '/' + x.substr(1);
return fs_path.join(os.homedir(), x.substr(1));
}
return x;
}