fix(core): s3-deployments don't work with new bootstrap stack #8578

rix0rrr · 2020-06-16T14:50:08Z

We added a KMS key, so the roles that want to read from the asset bucket should have KMS permissions.

Using aws-s3-deployments in combination with the new bootstrap stack requires a bootstrap stack update. You will see the following message if that is the case: Stack failed: Error: Stack: publishing assets requires bootstrap stack version '3', found '2'. Please run 'cdk bootstrap' with a newer CLI version.

Fixes #8541.

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

We added a KMS key, so the roles that want to read from the asset bucket should have KMS permissions. Fixes #8541.

aws-cdk-automation · 2020-06-16T15:36:02Z

AWS CodeBuild CI Report

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

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

eladb

I am not comfortable with granting decrypt permissions with any key. Feels unnecessarily promiscuous. I believe we should have an alias with a well known name in the bootstrap stack. Can we only grant permissions to it?

packages/@aws-cdk/core/lib/assets.ts

packages/@aws-cdk/core/lib/stack-synthesizers/default-synthesizer.ts

rix0rrr · 2020-06-17T07:56:17Z

I am not comfortable with granting decrypt permissions with any key. Feels unnecessarily promiscuous. I believe we should have an alias with a well known name in the bootstrap stack. Can we only grant permissions to it?

We don't have an alias.

We can add an alias, but it wouldn't help. It's not possible to add permissions to an alias, it must be on the key ID (which is a UUID).

hoegertn · 2020-06-17T08:10:18Z

What about a CFN Export of the key ID?

rix0rrr · 2020-06-17T09:49:15Z

What about a CFN Export of the key ID?

Yep, good call. We ended up doing that.

aws-cdk-automation · 2020-06-17T10:35:34Z

AWS CodeBuild CI Report

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

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

eladb · 2020-06-17T11:51:19Z

packages/@aws-cdk/core/lib/stack-synthesizers/default-synthesizer.ts

@@ -247,6 +267,7 @@ export class DefaultStackSynthesizer implements IStackSynthesizer {
      httpUrl,
      s3ObjectUrl,
      s3Url: httpUrl,
+      kmsKeyArn: Fn.importValue(cfnify(this._kmsKeyArnExportName!)),


curious, why do we need ! here and in for this.bucketName? Generally I think we should completely avoid !. It's better to write the code that checks for nullity and throws an "unexpected" exception (or some other meaningful message) then let the null propagate to an unexpected place.

packages/aws-cdk/lib/api/bootstrap/bootstrap-template.yaml

…l add the right permissions

aws-cdk-automation · 2020-06-17T12:00:17Z

AWS CodeBuild CI Report

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

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

aws-cdk-automation · 2020-06-17T13:12:44Z

AWS CodeBuild CI Report

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

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

aws-cdk-automation · 2020-06-17T13:18:30Z

AWS CodeBuild CI Report

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

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

aws-cdk-automation · 2020-06-17T13:21:43Z

AWS CodeBuild CI Report

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

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

eladb

I think you accidentally added the imported Key to the parent scope

packages/@aws-cdk/aws-s3-assets/lib/asset.ts

packages/@aws-cdk/aws-s3-assets/test/asset.test.ts

packages/aws-cdk/lib/api/bootstrap/bootstrap-template.yaml

Co-authored-by: Elad Ben-Israel <benisrae@amazon.com>

aws-cdk-automation · 2020-06-17T15:55:32Z

AWS CodeBuild CI Report

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

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

mergify · 2020-06-17T18:29:07Z

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-06-17T19:17:05Z

AWS CodeBuild CI Report

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

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

mergify · 2020-06-17T19:17: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).

fix(core): s3-deployments don't work with new bootstrap stack

841e672

We added a KMS key, so the roles that want to read from the asset bucket should have KMS permissions. Fixes #8541.

rix0rrr requested a review from a team June 16, 2020 14:50

rix0rrr self-assigned this Jun 16, 2020

mergify bot added the contribution/core This is a PR that came from AWS. label Jun 16, 2020

eladb suggested changes Jun 17, 2020

View reviewed changes

packages/@aws-cdk/core/lib/assets.ts Outdated Show resolved Hide resolved

packages/@aws-cdk/core/lib/stack-synthesizers/default-synthesizer.ts Outdated Show resolved Hide resolved

rix0rrr requested review from eladb and a team June 17, 2020 07:56

Make the KMS key explicit, exporting it from the bootstrap stack

0062a83

rix0rrr added the pr/do-not-merge This PR should not be merged at this time. label Jun 17, 2020

eladb suggested changes Jun 17, 2020

View reviewed changes

Load the key directly into the bucket, so that bucket.grantRead() wil…

009ed01

…l add the right permissions

rix0rrr added 3 commits June 17, 2020 14:24

Add test, switch default of usePreviousParameters around

646e9f5

Undo changes that accidentally slipped in here

6509d11

Merge branch 'master' into huijbers/s3-deployment-newassets

bcf0287

rix0rrr requested review from eladb and a team June 17, 2020 13:30

eladb suggested changes Jun 17, 2020

View reviewed changes

packages/@aws-cdk/aws-s3-assets/lib/asset.ts Outdated Show resolved Hide resolved

packages/@aws-cdk/aws-s3-assets/test/asset.test.ts Show resolved Hide resolved

packages/aws-cdk/lib/api/bootstrap/bootstrap-template.yaml Show resolved Hide resolved

Update packages/@aws-cdk/aws-s3-assets/lib/asset.ts

9f0ace8

Co-authored-by: Elad Ben-Israel <benisrae@amazon.com>

rix0rrr requested a review from eladb June 17, 2020 15:10

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

rix0rrr requested a review from a team June 17, 2020 16:02

eladb approved these changes Jun 17, 2020

View reviewed changes

Merge branch 'master' into huijbers/s3-deployment-newassets

129d2dc

mergify bot merged commit b2006c3 into master Jun 17, 2020

mergify bot deleted the huijbers/s3-deployment-newassets branch June 17, 2020 19:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(core): s3-deployments don't work with new bootstrap stack #8578

fix(core): s3-deployments don't work with new bootstrap stack #8578

rix0rrr commented Jun 16, 2020 •

edited

aws-cdk-automation commented Jun 16, 2020

eladb left a comment

rix0rrr commented Jun 17, 2020

hoegertn commented Jun 17, 2020

rix0rrr commented Jun 17, 2020

aws-cdk-automation commented Jun 17, 2020

eladb Jun 17, 2020

aws-cdk-automation commented Jun 17, 2020

aws-cdk-automation commented Jun 17, 2020

aws-cdk-automation commented Jun 17, 2020

aws-cdk-automation commented Jun 17, 2020

eladb left a comment

aws-cdk-automation commented Jun 17, 2020

mergify bot commented Jun 17, 2020

aws-cdk-automation commented Jun 17, 2020

mergify bot commented Jun 17, 2020

fix(core): s3-deployments don't work with new bootstrap stack #8578

fix(core): s3-deployments don't work with new bootstrap stack #8578

Conversation

rix0rrr commented Jun 16, 2020 • edited

aws-cdk-automation commented Jun 16, 2020

AWS CodeBuild CI Report

eladb left a comment

Choose a reason for hiding this comment

rix0rrr commented Jun 17, 2020

hoegertn commented Jun 17, 2020

rix0rrr commented Jun 17, 2020

aws-cdk-automation commented Jun 17, 2020

AWS CodeBuild CI Report

eladb Jun 17, 2020

Choose a reason for hiding this comment

aws-cdk-automation commented Jun 17, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Jun 17, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Jun 17, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Jun 17, 2020

AWS CodeBuild CI Report

eladb left a comment

Choose a reason for hiding this comment

aws-cdk-automation commented Jun 17, 2020

AWS CodeBuild CI Report

mergify bot commented Jun 17, 2020

aws-cdk-automation commented Jun 17, 2020

AWS CodeBuild CI Report

mergify bot commented Jun 17, 2020

rix0rrr commented Jun 16, 2020 •

edited