feat(codepipeline): cross-environment (account+region) actions by skinny85 · Pull Request #3694 · aws/aws-cdk

skinny85 · 2019-08-17T00:51:43Z

This changes the scaffolding stack logic for the cross-region CodePipelines to include a KMS key and alias as part of it, which are required if an action is simultaneously cross-region and cross-account. We also change to use the KMS key ID instead of the key ARN when rendering the ArtifactStores property.

We also add an alias to the default pipeline artifact bucket.

This required a bunch of changes to the KMS and S3 modules:

Alias now implements IKey
Added the keyId property to IKey
Added removalPolicy property to Alias
Granting permissions to a key works if the principal belongs to a stack that is a dependent of the key stack
Allow specifying a key when importing a bucket

Fixes #52
Concerns #1584
Fixes #2517
Fixes #2569
Concerns #3275
Fixes #3138
Fixes #3388

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

mergify · 2019-08-17T00:52:21Z

Pull Request Checklist

Testing

Unit test added (prefer to add a new test rather than modify existing tests)
CLI change? Re-run/add CLI integration tests

Documentation

Inline docs: make sure all public APIs are documented (copy & paste from official AWS docs)
README: update module README
Design: for significant features, follow the design process

Title uses the format type(scope): text

Type: fix, feat, refactor go into CHANGELOG, chore is hidden
Scope: name of the module without the aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
Style: use all lower-case, do not end with a period

Description

Rationale: describe rationale of change and approach taken
Issues: Indicate issues fixed via: fixes #xxx or closes #xxx
Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>

Sensitive Modules (requires 2 PR approvers)

IAM document library (in @aws-cdk/aws-iam)
EC2 security groups and ACLs (in @aws-cdk/aws-ec2)
Grant APIs (if not based on official documentation with a reference)

mergify · 2019-08-17T00:52:23Z

Codebuild (Continuous Integration) build failed for current commits. Please check log and resolve before PR is merged.

mergify · 2019-08-17T00:52:25Z

Codebuild (Continuous Integration) build failed for current commits. Please check log and resolve before PR is merged.

mergify · 2019-08-17T01:32:42Z

Codebuild (Continuous Integration) build failed for current commits. Please check log and resolve before PR is merged.

mergify · 2019-08-19T15:33:12Z

Pull Request Checklist

Testing

Unit test added (prefer to add a new test rather than modify existing tests)
CLI change? Re-run/add CLI integration tests

Documentation

Inline docs: make sure all public APIs are documented (copy & paste from official AWS docs)
README: update module README
Design: for significant features, follow the design process

Title uses the format type(scope): text

Type: fix, feat, refactor go into CHANGELOG, chore is hidden
Scope: name of the module without the aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
Style: use all lower-case, do not end with a period

Description

Rationale: describe rationale of change and approach taken
Issues: Indicate issues fixed via: fixes #xxx or closes #xxx
Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>

Sensitive Modules (requires 2 PR approvers)

IAM document library (in @aws-cdk/aws-iam)
EC2 security groups and ACLs (in @aws-cdk/aws-ec2)
Grant APIs (if not based on official documentation with a reference)

skinny85 · 2019-08-19T20:27:43Z

Updated the decdk test.

rix0rrr · 2019-08-20T08:30:52Z

packages/@aws-cdk/aws-codepipeline/lib/pipeline.ts

+      new kms.Alias(this, 'ArtifactsBucketEncryptionKeyAlias', {
+        aliasName: this.generateNameForDefaultBucketKeyAlias(),
+        targetKey: encryptionKey,
+        removalPolicy: RemovalPolicy.RETAIN, // alias should be retained, like the key


Does that not mean that destroying and redeploying the stack will fail?

Yes, but that was always true, even before adding the Alias (because of the orphaned, physically-named S3 Bucket backing the Pipeline).

Pinging about this.

skinny85 · 2019-08-22T18:05:15Z

Added retention policy Retain to the scaffold Alias.

mergify · 2019-08-22T18:52:58Z

Codebuild (Continuous Integration) build failed for current commits. Please check log and resolve before PR is merged.

mergify · 2019-08-23T00:00:21Z

Pull Request Checklist

Testing

Unit test added (prefer to add a new test rather than modify existing tests)
CLI change? Re-run/add CLI integration tests

Documentation

Inline docs: make sure all public APIs are documented (copy & paste from official AWS docs)
README: update module README
Design: for significant features, follow the design process

Title uses the format type(scope): text

Type: fix, feat, refactor go into CHANGELOG, chore is hidden
Scope: name of the module without the aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
Style: use all lower-case, do not end with a period

Description

Rationale: describe rationale of change and approach taken
Issues: Indicate issues fixed via: fixes #xxx or closes #xxx
Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>

Sensitive Modules (requires 2 PR approvers)

IAM document library (in @aws-cdk/aws-iam)
EC2 security groups and ACLs (in @aws-cdk/aws-ec2)
Grant APIs (if not based on official documentation with a reference)

mergify · 2019-08-28T22:41:30Z

Pull Request Checklist

Testing

Unit test added (prefer to add a new test rather than modify existing tests)
CLI change? Re-run/add CLI integration tests

Documentation

Inline docs: make sure all public APIs are documented (copy & paste from official AWS docs)
README: update module README
Design: for significant features, follow the design process

Title uses the format type(scope): text

Type: fix, feat, refactor go into CHANGELOG, chore is hidden
Scope: name of the module without the aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
Style: use all lower-case, do not end with a period

Description

Rationale: describe rationale of change and approach taken
Issues: Indicate issues fixed via: fixes #xxx or closes #xxx
Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>

Sensitive Modules (requires 2 PR approvers)

IAM document library (in @aws-cdk/aws-iam)
EC2 security groups and ACLs (in @aws-cdk/aws-ec2)
Grant APIs (if not based on official documentation with a reference)

eladb

Why is the title talks about "simultaneous". You basically mean "support cross account and region actions"?

eladb · 2019-08-28T17:49:06Z

packages/@aws-cdk/aws-codepipeline/README.md

+const replicationBucket = new s3.Bucket(replicationStack, 'ReplicationBucket', {
+  // like was said above - replication buckets need a set physical name
+  bucketName: PhysicalName.GENERATE_IF_NEEDED,
+  encryptionKey: key, // does not work!


what "does not work!" mean?

You'll get a "Can only reference cross stacks in the same region and account." error.

Please follow up with @jerry-aws in a separate PR and revisit this doc. I think he will be able to help.

eladb · 2019-08-28T17:53:38Z

packages/@aws-cdk/aws-codepipeline/README.md


+#### Creating an encrypted replication bucket
+
+If you're passing a replication bucket created in a different stack,


This documentation is weird. Should mostly describe how to do things and not what doesn't work...

I just wanted to point out a potential "gotcha!" that might trip people up.

If you have a suggestion how to re-structure this documentation, I'm all ears.

eladb

I need a walk through this code. I am really struggling to understand what's going on here (it's me!).
Any chance you can schedule some time next week?

packages/@aws-cdk/aws-kms/lib/alias.ts

eladb · 2019-08-29T14:48:52Z

packages/@aws-cdk/aws-kms/lib/key.ts

+    // This is a problem if the stack the grantee is part of depends on the key stack
+    // (as it won't exist before the key policy is attempted to be created).
+    // In that case, make the account the resource policy principal
+    const granteeStackDependsOnKeyStack = this.granteeStackDependsOnKeyStack(grantee);


Do we have some doc reference (or appsec consultation) that this is the best practice?

What would you suggest as an alternative here, given the comment?

packages/@aws-cdk/aws-kms/lib/key.ts

packages/@aws-cdk/aws-kms/test/test.key.ts

eladb · 2019-08-29T14:54:50Z

packages/@aws-cdk/aws-iam/lib/grant.ts

      actions: options.actions,
      resources: (options.resourceSelfArns || options.resourceArns),
-      principals: [options.grantee!.grantPrincipal]
+      principals: [options.resourcePolicyPrincipal || options.grantee!.grantPrincipal]


missing test for this

We don't have separate tests in the IAM library for methods like addToPrincipalAndResource - they're exercised only through tests in the particular Construct Libraries that use them. I'd rather not add them as part of this change. With the KMS test added above, this is now covered as well.

skinny85 · 2019-09-10T00:44:53Z

Why is the title talks about "simultaneous". You basically mean "support cross account and region actions"?

"Simultaneous" means "at the same time", because this PR deals specifically with actions that are both cross-region and cross-account at the same time. "Simultaneous" is just shorter.

Pull request has been modified.

skinny85 · 2019-09-10T01:51:22Z

Updated with feedback from comments & rebased.

packages/@aws-cdk/aws-kms/lib/key.ts

eladb · 2019-09-10T09:54:56Z

packages/@aws-cdk/aws-codepipeline/README.md

+const replicationBucket = new s3.Bucket(replicationStack, 'ReplicationBucket', {
+  // like was said above - replication buckets need a set physical name
+  bucketName: PhysicalName.GENERATE_IF_NEEDED,
+  encryptionKey: key, // does not work!


Please follow up with @jerry-aws in a separate PR and revisit this doc. I think he will be able to help.

packages/@aws-cdk/aws-codepipeline/lib/pipeline.ts

mergify · 2019-09-10T10:15:45Z

Continuous integration build failed

Pull request has been modified.

…on actions

skinny85 · 2019-09-10T22:46:41Z

Fixed the Import class in fromKeyArn and rebased over 1.8.0.

skinny85 · 2019-09-11T00:58:11Z

Please follow up with @jerry-aws in a separate PR and revisit this doc. I think he will be able to help.

Done: #4031

skinny85 requested a review from eladb August 17, 2019 00:51

skinny85 mentioned this pull request Aug 17, 2019

feat(code pipeline): support KMS materials for replication stores #1584

Closed

2 tasks

mergify bot added the automerge-enabled label Aug 17, 2019

skinny85 mentioned this pull request Aug 17, 2019

fix(codepipeline): set correct prop names for KMS key #2517

Closed

4 tasks

skinny85 added the pr/do-not-merge This PR should not be merged at this time. label Aug 17, 2019

skinny85 mentioned this pull request Aug 17, 2019

Don't create a gazillion kms keys #3275

Closed

5 tasks

skinny85 mentioned this pull request Aug 17, 2019

CodePipeline: allow alias to be assigned to autogenerated KMS key #2569

Closed

skinny85 removed the automerge-enabled label Aug 19, 2019

mergify bot added the automerge-enabled label Aug 19, 2019

skinny85 force-pushed the feat/cross-region-and-account-cfn-pipelines branch from c5ac2d8 to 65b7cd3 Compare August 19, 2019 20:27

rix0rrr reviewed Aug 20, 2019

View reviewed changes

skinny85 force-pushed the feat/cross-region-and-account-cfn-pipelines branch from 65b7cd3 to 2556de0 Compare August 22, 2019 18:04

skinny85 removed the automerge-enabled label Aug 22, 2019

mergify bot added the automerge-enabled label Aug 23, 2019

nija-at requested a review from rix0rrr August 23, 2019 07:02

rix0rrr previously approved these changes Aug 23, 2019

View reviewed changes

skinny85 mentioned this pull request Aug 24, 2019

feat(codepipeline): handle non-CFN cross-region actions #3777

Merged

RomainMuller assigned skinny85 Aug 26, 2019

skinny85 removed the automerge-enabled label Aug 28, 2019

mergify bot added the automerge-enabled label Aug 28, 2019

eladb reviewed Aug 29, 2019

View reviewed changes

eladb previously requested changes Aug 29, 2019

View reviewed changes

skinny85 force-pushed the feat/cross-region-and-account-cfn-pipelines branch from 2556de0 to e71e78c Compare September 10, 2019 01:45

eladb changed the title ~~feat(codepipeline): support simultaneous cross-account and cross-region actions~~ feat(codepipeline): cross-environment (account+region) actions Sep 10, 2019

eladb previously approved these changes Sep 10, 2019

View reviewed changes

skinny85 and others added 4 commits September 10, 2019 14:45

feat(codepipeline): support simultaneous cross-account and cross-regi…

17a15cf

…on actions

use keyResourceName since it is endured not to be undefined

a814ecc

revert change to fromKeyArn

f3f0c1f

Fix the Import class in fromKeyArn.

898cd8e

skinny85 force-pushed the feat/cross-region-and-account-cfn-pipelines branch from 73df832 to 898cd8e Compare September 10, 2019 22:45

skinny85 merged commit 69bff3d into aws:master Sep 11, 2019

skinny85 deleted the feat/cross-region-and-account-cfn-pipelines branch September 11, 2019 00:15

This was referenced Sep 18, 2019

S3 encryption cross-stack permission causes cyclic reference #3732

Closed

Circular dependency on s3 notification to a destination when both destination and s3 are encrypted by same CMK #3067

Open

NGL321 added the contribution/core This is a PR that came from AWS. label Sep 27, 2019

skinny85 mentioned this pull request Apr 8, 2020

Add a pathHash method to ConstructNode aws/constructs#48

Closed

2 tasks


		#### Creating an encrypted replication bucket

		If you're passing a replication bucket created in a different stack,

Conversation

skinny85 commented Aug 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mergify bot commented Aug 17, 2019

Pull Request Checklist

Uh oh!

mergify bot commented Aug 17, 2019

Uh oh!

mergify bot commented Aug 17, 2019

Uh oh!

mergify bot commented Aug 17, 2019

Uh oh!

mergify bot commented Aug 19, 2019

Pull Request Checklist

Uh oh!

skinny85 commented Aug 19, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skinny85 commented Aug 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mergify bot commented Aug 22, 2019

Uh oh!

mergify bot commented Aug 23, 2019

Pull Request Checklist

Uh oh!

mergify bot commented Aug 28, 2019

Pull Request Checklist

Uh oh!

eladb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eladb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skinny85 commented Sep 10, 2019

Uh oh!

skinny85 commented Sep 10, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mergify bot commented Sep 10, 2019

Uh oh!

skinny85 commented Sep 10, 2019

Uh oh!

skinny85 commented Sep 11, 2019

Uh oh!

Reviewers

Assignees

Labels

skinny85 commented Aug 17, 2019 •

edited

Loading

skinny85 commented Aug 22, 2019 •

edited

Loading