fix(cli): can't bootstrap environment not in app #7510

rix0rrr · 2020-04-22T11:56:32Z

Another commit split off from #6366 to make that other one more manageable to review :).

Commit Message

fix(cli): can't bootstrap environment not in app

It used to be that if we had an --app argument, we would always glob
arguments to cdk bootstrap <ENV> through the environments of stacks
in the app.

This makes it super hard/annoying to run cdk bootstrap aws://1234/us-somewhere in a CI/CD project; you have to add a stack
there first and compile before you're allowed to do that, which is
kinda silly.

Change behavior to only glob environment from the environments in the
app if it looks like the user is supplying a glob (if it contains *).

If the user is supplying a concrete environment name, just use it
directly.

Also in this commit:

refactor: lots of places where we were passing around a pair
of (account, region). Replace those by passing a cxapi.Environment
instead (most of the changes in this PR).
refactor: the old and new bootstrapping experience had a lot of
copy/pasta between them. Refactored to make them share code.
(prerelease) feat: add a version check to the bootstrapping operation,
so that users won't accidentally downgrade a bootstrap stack to an older
version (This happened to 2 devs already, and is an easy mistake to
make. Protect against it.)

End Commit Message

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

It used to be that if we had an `--app` argument, we would always glob arguments to `cdk bootstrap <ENV>` through the environments of stacks in the app. This makes it super hard/annoying to run `cdk bootstrap aws://1234/us-somewhere` in a CI/CD project; you have to add a stack there first and compile before you're allowed to do that, which is kinda silly. Change behavior to only glob environment from the environments in the app if it looks like the user is supplying a glob (if it contains `*`). If the user is supplying a concrete environment name, just use it directly. Also in this commit: - refactor: lots of places where we were passing around a pair of `(account, region)`. Replace those by passing a `cxapi.Environment` instead (most of the changes in this PR). - refactor: the old and new bootstrapping experience had a lot of copy/pasta between them. Refactored to make them share code. - (prerelease) feat: add a version check to the bootstrapping operation, so that users won't accidentally downgrade a bootstrap stack to an older version (This happened to 2 devs already, and is an easy mistake to make. Protect against it.)

aws-cdk-automation · 2020-04-22T12:02:07Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: b1422bf
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2020-04-22T12:55:32Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 0dc5313
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

shivlaks

provisional - can you also update the title... the double negatives got me for a second

shivlaks · 2020-04-24T01:59:41Z

packages/aws-cdk/bin/cdk.ts

@@ -56,7 +56,8 @@ async function parseCommandLineArguments() {
      .option('tags', { type: 'array', alias: 't', desc: 'Tags to add for the stack (KEY=VALUE)', nargs: 1, requiresArg: true, default: [] })
      .option('execute', {type: 'boolean', desc: 'Whether to execute ChangeSet (--no-execute will NOT execute the ChangeSet)', default: true})
      .option('trust', { type: 'array', desc: 'The (space-separated) list of AWS account IDs that should be trusted to perform deployments into this environment', default: [], hidden: true })
-      .option('cloudformation-execution-policies', { type: 'array', desc: 'The (space-separated) list of Managed Policy ARNs that should be attached to the role performing deployments into this environment. Required if --trust was passed', default: [], hidden: true }),
+      .option('cloudformation-execution-policies', { type: 'array', desc: 'The (space-separated) list of Managed Policy ARNs that should be attached to the role performing deployments into this environment. Required if --trust was passed', default: [], hidden: true })
+      .option('force', { alias: 'f', type: 'boolean', desc: 'Always bootstrap even if it would downgrade template version', default: false }),


makes sense to protect against it, but what's the use case behind forcing a bootstrap and the implication of using a downgraded template version?

when would it be used?

I don't want to prevent people from doing what they're trying to do, in case we get it wrong (for whatever reason).

Just feels like any protection should have an 'override' feature'.

fair enough

shivlaks · 2020-04-24T02:01:34Z

packages/aws-cdk/lib/api/aws-auth/sdk-provider.ts

   * The `region` and `accountId` parameters are interpreted as in `resolveEnvironment()` (which is to
   * say, `undefined` doesn't do what you expect).


this comment needs updating

shivlaks · 2020-04-24T02:02:25Z

packages/aws-cdk/lib/api/aws-auth/sdk-provider.ts

   * `undefined` actually means `undefined`, and is NOT changed to default values! Only the magic values UNKNOWN_REGION
   * and UNKNOWN_ACCOUNT will be replaced with looked-up values!


shivlaks · 2020-04-24T02:14:26Z

packages/aws-cdk/lib/api/aws-auth/sdk-provider.ts

-  public async resolveEnvironmentObject(env: cxapi.Environment) {
-    return this.resolveEnvironment(env.account, env.region);


there was nothing using this?

There was one place, but this was the method that resolveEnvironment should have been all along.

shivlaks · 2020-04-24T02:21:23Z

packages/aws-cdk/lib/api/aws-auth/sdk-provider.ts

-    if (!region) {
-      throw new Error('AWS region must be configured either when you configure your CDK stack or through the environment');
-    }


was this dead code?

shivlaks · 2020-04-24T02:46:10Z

packages/aws-cdk/lib/api/bootstrap/deploy-bootstrap.ts

+  const newVersion = bootstrapVersionFromTemplate(template);
+  const currentBootstrapStack = await ToolkitInfo.lookup(resolvedEnvironment, sdk, toolkitStackName);
+  if (currentBootstrapStack && newVersion < currentBootstrapStack.version && !options.force) {
+    throw new Error(`Refusing to downgrade existing bootstrap stack from version '${currentBootstrapStack.version}' to version '${newVersion}'. Use --force to force.`);


Suggested change

throw new Error(`Refusing to downgrade existing bootstrap stack from version '${currentBootstrapStack.version}' to version '${newVersion}'. Use --force to force.`);

throw new Error(`Downgrade not allowed. The existing bootstrap stack version '${currentBootstrapStack.version}' is higher than version '${newVersion}'. Use --force to force.`);

maybe it's just the word "refusing" i don't like

I modeled this after tar's "Cowardly refusing to create an empty archive" :).

I'll rephrase.

shivlaks · 2020-04-24T02:51:59Z

packages/aws-cdk/lib/api/toolkit-info.ts

@@ -29,6 +29,7 @@ export class ToolkitInfo {
      sdk, environment,
      bucketName: requireOutput(BUCKET_NAME_OUTPUT),
      bucketEndpoint: requireOutput(BUCKET_DOMAIN_NAME_OUTPUT),
+      version: parseInt(outputs[BOOTSTRAP_VERSION_OUTPUT] ?? '0', 10),


is the bootstrap version always going to be bumped up by numbers? (i.e. as opposed to 1.1.0 vs 1.2.0 would both parse to the same int)

shivlaks · 2020-04-24T02:54:23Z

packages/aws-cdk/lib/cdk-toolkit.ts

+    // Partition into globs and non-globs (this will mutate environmentSpecs).
+    const globSpecs = partition(environmentSpecs, looksLikeGlob);
+    if (globSpecs.length > 0 && !this.props.cloudExecutable.hasApp) {
+      throw new Error(`Don't know how to bootstrap environment '${globSpecs}'. Run in app directory or specify an environment name like \'aws://123456789012/us-east-1\'.`);


Suggested change

throw new Error(`Don't know how to bootstrap environment '${globSpecs}'. Run in app directory or specify an environment name like \'aws://123456789012/us-east-1\'.`);

throw new Error(`Unable to bootstrap environment '${globSpecs}'. Run in app directory or specify an environment name like \'aws://123456789012/us-east-1\'.`);

I don't think "don't know how to" adds as much as the guidance that follows after it.

aws-cdk-automation · 2020-04-24T08:21:54Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 1ab47ba
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2020-04-24T08:22:45Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2020-04-24T09:07:59Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 7c347aa
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2020-04-24T09:09:23Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

…gument If a user supplied the `--app` argument, we select the environments from the app. However, we did not have a check that precludes this selection when the argument is not provided. We recently refactored this area in #7510 Closes #7906

…gument (#7970) If a user supplied the `--app` argument, we select the environments from the app. However, we did not have a check that precludes this selection when the argument is not provided. We recently refactored this area in #7510 Closes #7906

…gument (aws#7970) If a user supplied the `--app` argument, we select the environments from the app. However, we did not have a check that precludes this selection when the argument is not provided. We recently refactored this area in aws#7510 Closes aws#7906

rix0rrr requested a review from a team April 22, 2020 11:56

rix0rrr self-assigned this Apr 22, 2020

mergify bot added contribution/core This is a PR that came from AWS. labels Apr 22, 2020

More coalescing

0dc5313

shivlaks added the pr/do-not-merge This PR should not be merged at this time. label Apr 24, 2020

shivlaks approved these changes Apr 24, 2020

View reviewed changes

Error message and documentation changes

1ab47ba

rix0rrr removed the pr/do-not-merge This PR should not be merged at this time. label Apr 24, 2020

Merge branch 'master' into huijbers/bootstrappin-across-the-universe

7c347aa

mergify bot merged commit 9566cca into master Apr 24, 2020

rix0rrr deleted the huijbers/bootstrappin-across-the-universe branch April 29, 2020 08:59

chrisb92 mentioned this pull request May 11, 2020

CDK Bootstrap requires --app #7906

Closed

shivlaks mentioned this pull request May 14, 2020

fix(cli): cdk bootstrap cannot be used without supplying the --app argument #7970

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(cli): can't bootstrap environment not in app #7510

fix(cli): can't bootstrap environment not in app #7510

rix0rrr commented Apr 22, 2020

aws-cdk-automation commented Apr 22, 2020

aws-cdk-automation commented Apr 22, 2020

shivlaks left a comment •

edited

Loading

shivlaks Apr 24, 2020

rix0rrr Apr 24, 2020

shivlaks Apr 24, 2020

shivlaks Apr 24, 2020

shivlaks Apr 24, 2020

shivlaks Apr 24, 2020

rix0rrr Apr 24, 2020

shivlaks Apr 24, 2020

rix0rrr Apr 24, 2020

shivlaks Apr 24, 2020

rix0rrr Apr 24, 2020

shivlaks Apr 24, 2020

rix0rrr Apr 24, 2020

shivlaks Apr 24, 2020

aws-cdk-automation commented Apr 24, 2020

mergify bot commented Apr 24, 2020

aws-cdk-automation commented Apr 24, 2020

mergify bot commented Apr 24, 2020

		* The `region` and `accountId` parameters are interpreted as in `resolveEnvironment()` (which is to
		* say, `undefined` doesn't do what you expect).

		* `undefined` actually means `undefined`, and is NOT changed to default values! Only the magic values UNKNOWN_REGION
		* and UNKNOWN_ACCOUNT will be replaced with looked-up values!

		public async resolveEnvironmentObject(env: cxapi.Environment) {
		return this.resolveEnvironment(env.account, env.region);

	throw new Error(`Refusing to downgrade existing bootstrap stack from version '${currentBootstrapStack.version}' to version '${newVersion}'. Use --force to force.`);
	throw new Error(`Downgrade not allowed. The existing bootstrap stack version '${currentBootstrapStack.version}' is higher than version '${newVersion}'. Use --force to force.`);

	throw new Error(`Don't know how to bootstrap environment '${globSpecs}'. Run in app directory or specify an environment name like \'aws://123456789012/us-east-1\'.`);
	throw new Error(`Unable to bootstrap environment '${globSpecs}'. Run in app directory or specify an environment name like \'aws://123456789012/us-east-1\'.`);

fix(cli): can't bootstrap environment not in app #7510

fix(cli): can't bootstrap environment not in app #7510

Conversation

rix0rrr commented Apr 22, 2020

Commit Message

End Commit Message

aws-cdk-automation commented Apr 22, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Apr 22, 2020

AWS CodeBuild CI Report

shivlaks left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aws-cdk-automation commented Apr 24, 2020

AWS CodeBuild CI Report

mergify bot commented Apr 24, 2020

aws-cdk-automation commented Apr 24, 2020

AWS CodeBuild CI Report

mergify bot commented Apr 24, 2020

shivlaks left a comment •

edited

Loading