From e412b574058e631bcf4724e6885be9b0a196ca59 Mon Sep 17 00:00:00 2001 From: Adam Ruka Date: Fri, 28 Jun 2019 17:24:54 -0700 Subject: [PATCH] fix(codepipeline): correctly pass the replication Buckets to Action.bind(). With the change to generate a Role per Pipeline Action, I realized we were incorrectly setting permissions on cross-region Actions. We were always passing the Pipeline Bucket to the Action.bind() method, while in reality they need permissions to their replication Buckets. I also changed the strategy of generating the names for the replication Buckets to use our new PhysicalName.GENERATE_IF_NEEDED. --- .../lib/cloudformation/pipeline-actions.ts | 6 +++--- .../lib/codebuild/build-action.ts | 6 +++--- .../lib/codecommit/source-action.ts | 2 +- .../lib/codedeploy/server-deploy-action.ts | 6 +++--- .../lib/ecr/source-action.ts | 2 +- .../lib/ecs/deploy-action.ts | 4 ++-- .../lib/s3/deploy-action.ts | 4 ++-- .../lib/s3/source-action.ts | 2 +- .../cloudformation/test.pipeline-actions.ts | 1 + .../test/test.pipeline.ts | 2 +- .../@aws-cdk/aws-codepipeline/lib/action.ts | 4 ++-- .../lib/cross-region-support-stack.ts | 18 +----------------- .../@aws-cdk/aws-codepipeline/lib/pipeline.ts | 15 +++++++++------ 13 files changed, 30 insertions(+), 42 deletions(-) diff --git a/packages/@aws-cdk/aws-codepipeline-actions/lib/cloudformation/pipeline-actions.ts b/packages/@aws-cdk/aws-codepipeline-actions/lib/cloudformation/pipeline-actions.ts index 00a0a7313a255..b47f288b268e8 100644 --- a/packages/@aws-cdk/aws-codepipeline-actions/lib/cloudformation/pipeline-actions.ts +++ b/packages/@aws-cdk/aws-codepipeline-actions/lib/cloudformation/pipeline-actions.ts @@ -75,14 +75,14 @@ abstract class CloudFormationAction extends Action { this.props = props; } - protected bound(_scope: cdk.Construct, stage: codepipeline.IStage, options: codepipeline.ActionBindOptions): + protected bound(_scope: cdk.Construct, _stage: codepipeline.IStage, options: codepipeline.ActionBindOptions): codepipeline.ActionConfig { const singletonPolicy = SingletonPolicy.forRole(options.role); if ((this.actionProperties.outputs || []).length > 0) { - stage.pipeline.artifactBucket.grantReadWrite(singletonPolicy); + options.bucket.grantReadWrite(singletonPolicy); } else if ((this.actionProperties.inputs || []).length > 0) { - stage.pipeline.artifactBucket.grantRead(singletonPolicy); + options.bucket.grantRead(singletonPolicy); } return { diff --git a/packages/@aws-cdk/aws-codepipeline-actions/lib/codebuild/build-action.ts b/packages/@aws-cdk/aws-codepipeline-actions/lib/codebuild/build-action.ts index 521d972cc05e3..dc40545e04e49 100644 --- a/packages/@aws-cdk/aws-codepipeline-actions/lib/codebuild/build-action.ts +++ b/packages/@aws-cdk/aws-codepipeline-actions/lib/codebuild/build-action.ts @@ -83,7 +83,7 @@ export class CodeBuildAction extends Action { this.props = props; } - protected bound(_scope: cdk.Construct, stage: codepipeline.IStage, options: codepipeline.ActionBindOptions): + protected bound(_scope: cdk.Construct, _stage: codepipeline.IStage, options: codepipeline.ActionBindOptions): codepipeline.ActionConfig { // grant the Pipeline role the required permissions to this Project options.role.addToPolicy(new iam.PolicyStatement({ @@ -97,9 +97,9 @@ export class CodeBuildAction extends Action { // allow the Project access to the Pipeline's artifact Bucket if ((this.actionProperties.outputs || []).length > 0) { - stage.pipeline.artifactBucket.grantReadWrite(this.props.project); + options.bucket.grantReadWrite(this.props.project); } else { - stage.pipeline.artifactBucket.grantRead(this.props.project); + options.bucket.grantRead(this.props.project); } const configuration: any = { diff --git a/packages/@aws-cdk/aws-codepipeline-actions/lib/codecommit/source-action.ts b/packages/@aws-cdk/aws-codepipeline-actions/lib/codecommit/source-action.ts index 74afd97c2c3c9..9cc1793aa905f 100644 --- a/packages/@aws-cdk/aws-codepipeline-actions/lib/codecommit/source-action.ts +++ b/packages/@aws-cdk/aws-codepipeline-actions/lib/codecommit/source-action.ts @@ -91,7 +91,7 @@ export class CodeCommitSourceAction extends Action { // the Action will write the contents of the Git repository to the Bucket, // so its Role needs write permissions to the Pipeline Bucket - stage.pipeline.artifactBucket.grantWrite(options.role); + options.bucket.grantWrite(options.role); // https://docs.aws.amazon.com/codecommit/latest/userguide/auth-and-access-control-permissions-reference.html#aa-acp options.role.addToPolicy(new iam.PolicyStatement({ diff --git a/packages/@aws-cdk/aws-codepipeline-actions/lib/codedeploy/server-deploy-action.ts b/packages/@aws-cdk/aws-codepipeline-actions/lib/codedeploy/server-deploy-action.ts index 8b3cc864c1e24..11925ece7a6cd 100644 --- a/packages/@aws-cdk/aws-codepipeline-actions/lib/codedeploy/server-deploy-action.ts +++ b/packages/@aws-cdk/aws-codepipeline-actions/lib/codedeploy/server-deploy-action.ts @@ -35,7 +35,7 @@ export class CodeDeployServerDeployAction extends Action { this.deploymentGroup = props.deploymentGroup; } - protected bound(_scope: Construct, stage: codepipeline.IStage, options: codepipeline.ActionBindOptions): + protected bound(_scope: Construct, _stage: codepipeline.IStage, options: codepipeline.ActionBindOptions): codepipeline.ActionConfig { // permissions, based on: // https://docs.aws.amazon.com/codedeploy/latest/userguide/auth-and-access-control-permissions-reference.html @@ -57,11 +57,11 @@ export class CodeDeployServerDeployAction extends Action { // grant the ASG Role permissions to read from the Pipeline Bucket for (const asg of this.deploymentGroup.autoScalingGroups || []) { - stage.pipeline.artifactBucket.grantRead(asg.role); + options.bucket.grantRead(asg.role); } // the Action's Role needs to read from the Bucket to get artifacts - stage.pipeline.artifactBucket.grantRead(options.role); + options.bucket.grantRead(options.role); return { configuration: { diff --git a/packages/@aws-cdk/aws-codepipeline-actions/lib/ecr/source-action.ts b/packages/@aws-cdk/aws-codepipeline-actions/lib/ecr/source-action.ts index ae41f8b3bbc47..63d5cea9ea831 100644 --- a/packages/@aws-cdk/aws-codepipeline-actions/lib/ecr/source-action.ts +++ b/packages/@aws-cdk/aws-codepipeline-actions/lib/ecr/source-action.ts @@ -63,7 +63,7 @@ export class EcrSourceAction extends Action { }); // the Action Role also needs to write to the Pipeline's bucket - stage.pipeline.artifactBucket.grantWrite(options.role); + options.bucket.grantWrite(options.role); return { configuration: { diff --git a/packages/@aws-cdk/aws-codepipeline-actions/lib/ecs/deploy-action.ts b/packages/@aws-cdk/aws-codepipeline-actions/lib/ecs/deploy-action.ts index b750a27c2499d..6fc5a18da74a8 100644 --- a/packages/@aws-cdk/aws-codepipeline-actions/lib/ecs/deploy-action.ts +++ b/packages/@aws-cdk/aws-codepipeline-actions/lib/ecs/deploy-action.ts @@ -61,7 +61,7 @@ export class EcsDeployAction extends Action { this.props = props; } - protected bound(_scope: Construct, stage: codepipeline.IStage, options: codepipeline.ActionBindOptions): + protected bound(_scope: Construct, _stage: codepipeline.IStage, options: codepipeline.ActionBindOptions): codepipeline.ActionConfig { // permissions based on CodePipeline documentation: // https://docs.aws.amazon.com/codepipeline/latest/userguide/how-to-custom-role.html#how-to-update-role-new-services @@ -90,7 +90,7 @@ export class EcsDeployAction extends Action { } })); - stage.pipeline.artifactBucket.grantRead(options.role); + options.bucket.grantRead(options.role); return { configuration: { diff --git a/packages/@aws-cdk/aws-codepipeline-actions/lib/s3/deploy-action.ts b/packages/@aws-cdk/aws-codepipeline-actions/lib/s3/deploy-action.ts index 75c1ab12781b2..60521a3d362be 100644 --- a/packages/@aws-cdk/aws-codepipeline-actions/lib/s3/deploy-action.ts +++ b/packages/@aws-cdk/aws-codepipeline-actions/lib/s3/deploy-action.ts @@ -49,13 +49,13 @@ export class S3DeployAction extends Action { this.props = props; } - protected bound(_scope: Construct, stage: codepipeline.IStage, options: codepipeline.ActionBindOptions): + protected bound(_scope: Construct, _stage: codepipeline.IStage, options: codepipeline.ActionBindOptions): codepipeline.ActionConfig { // pipeline needs permissions to write to the S3 bucket this.props.bucket.grantWrite(options.role); // the Action Role also needs to read from the Pipeline's bucket - stage.pipeline.artifactBucket.grantRead(options.role); + options.bucket.grantRead(options.role); return { configuration: { diff --git a/packages/@aws-cdk/aws-codepipeline-actions/lib/s3/source-action.ts b/packages/@aws-cdk/aws-codepipeline-actions/lib/s3/source-action.ts index f4242dc40294d..af54ed6e084e7 100644 --- a/packages/@aws-cdk/aws-codepipeline-actions/lib/s3/source-action.ts +++ b/packages/@aws-cdk/aws-codepipeline-actions/lib/s3/source-action.ts @@ -96,7 +96,7 @@ export class S3SourceAction extends Action { this.props.bucket.grantRead(options.role); // ...and write to the Pipeline bucket - stage.pipeline.artifactBucket.grantWrite(options.role); + options.bucket.grantWrite(options.role); return { configuration: { diff --git a/packages/@aws-cdk/aws-codepipeline-actions/test/cloudformation/test.pipeline-actions.ts b/packages/@aws-cdk/aws-codepipeline-actions/test/cloudformation/test.pipeline-actions.ts index c3a18465f371f..534d261a68716 100644 --- a/packages/@aws-cdk/aws-codepipeline-actions/test/cloudformation/test.pipeline-actions.ts +++ b/packages/@aws-cdk/aws-codepipeline-actions/test/cloudformation/test.pipeline-actions.ts @@ -352,6 +352,7 @@ class StageDouble implements codepipeline.IStage { const actionParent = new cdk.Construct(stageParent, action.actionProperties.actionName); fullActions.push(new FullAction(action.actionProperties, action.bind(actionParent, this, { role: pipeline.role, + bucket: pipeline.artifactBucket, }))); } this.actions = fullActions; diff --git a/packages/@aws-cdk/aws-codepipeline-actions/test/test.pipeline.ts b/packages/@aws-cdk/aws-codepipeline-actions/test/test.pipeline.ts index e828ca247690b..ad8e4cb1ac5d4 100644 --- a/packages/@aws-cdk/aws-codepipeline-actions/test/test.pipeline.ts +++ b/packages/@aws-cdk/aws-codepipeline-actions/test/test.pipeline.ts @@ -619,7 +619,7 @@ export = { "Region": "us-east-1", "ArtifactStore": { "Type": "S3", - "Location": "cdk-cross-region-codepipeline-replication-bucket-685c6feea5fb", + "Location": "teststack-support-us-easteplicationbucket1a8063b3cdac6e7e0e73", }, }, { diff --git a/packages/@aws-cdk/aws-codepipeline/lib/action.ts b/packages/@aws-cdk/aws-codepipeline/lib/action.ts index 82158047bb3f6..8ab5c79d88042 100644 --- a/packages/@aws-cdk/aws-codepipeline/lib/action.ts +++ b/packages/@aws-cdk/aws-codepipeline/lib/action.ts @@ -77,6 +77,8 @@ export interface ActionProperties { export interface ActionBindOptions { readonly role: iam.IRole; + + readonly bucket: s3.IBucket; } export interface ActionConfig { @@ -111,8 +113,6 @@ export interface IPipeline extends IResource { */ readonly pipelineArn: string; - readonly artifactBucket: s3.IBucket; - /** * Define an event rule triggered by this CodePipeline. * diff --git a/packages/@aws-cdk/aws-codepipeline/lib/cross-region-support-stack.ts b/packages/@aws-cdk/aws-codepipeline/lib/cross-region-support-stack.ts index 33b6003433631..e916924d3ac24 100644 --- a/packages/@aws-cdk/aws-codepipeline/lib/cross-region-support-stack.ts +++ b/packages/@aws-cdk/aws-codepipeline/lib/cross-region-support-stack.ts @@ -1,6 +1,5 @@ import s3 = require('@aws-cdk/aws-s3'); import cdk = require('@aws-cdk/core'); -import crypto = require('crypto'); /** * Construction properties for {@link CrossRegionSupportStack}. @@ -45,11 +44,8 @@ export class CrossRegionSupportStack extends cdk.Stack { }, }); - const replicationBucketName = generateUniqueName('cdk-cross-region-codepipeline-replication-bucket-', - props.region, props.account, false, 12); - this.replicationBucket = new s3.Bucket(this, 'CrossRegionCodePipelineReplicationBucket', { - bucketName: replicationBucketName, + bucketName: cdk.PhysicalName.GENERATE_IF_NEEDED, }); } } @@ -57,15 +53,3 @@ export class CrossRegionSupportStack extends cdk.Stack { function generateStackName(props: CrossRegionSupportStackProps): string { return `${props.pipelineStackName}-support-${props.region}`; } - -function generateUniqueName(baseName: string, region: string, account: string, - toUpperCase: boolean, hashPartLen: number = 8): string { - const sha256 = crypto.createHash('sha256') - .update(baseName) - .update(region) - .update(account); - - const hash = sha256.digest('hex').slice(0, hashPartLen); - - return baseName + (toUpperCase ? hash.toUpperCase() : hash.toLowerCase()); -} diff --git a/packages/@aws-cdk/aws-codepipeline/lib/pipeline.ts b/packages/@aws-cdk/aws-codepipeline/lib/pipeline.ts index 4b2040acbdfad..cbc3e524f27de 100644 --- a/packages/@aws-cdk/aws-codepipeline/lib/pipeline.ts +++ b/packages/@aws-cdk/aws-codepipeline/lib/pipeline.ts @@ -104,7 +104,6 @@ export interface PipelineProps { abstract class PipelineBase extends Resource implements IPipeline { public abstract pipelineName: string; public abstract pipelineArn: string; - public abstract artifactBucket: s3.IBucket; /** * Defines an event rule triggered by this CodePipeline. @@ -305,7 +304,7 @@ export class Pipeline extends PipelineBase { /** @internal */ public _attachActionToPipeline(stage: Stage, action: IAction, actionScope: Construct): FullActionDescriptor { // handle cross-region actions here - this.ensureReplicationBucketExistsFor(action.actionProperties.region); + const bucket = this.ensureReplicationBucketExistsFor(action.actionProperties.region); // get the role for the given action const actionRole = this.getRoleForAction(stage, action, actionScope); @@ -313,6 +312,7 @@ export class Pipeline extends PipelineBase { // bind the Action const actionDescriptor = action.bind(actionScope, stage, { role: actionRole ? actionRole : this.role, + bucket, }); return new FullActionDescriptor(action, actionDescriptor, actionRole); @@ -335,17 +335,17 @@ export class Pipeline extends PipelineBase { ]; } - private ensureReplicationBucketExistsFor(region?: string) { + private ensureReplicationBucketExistsFor(region?: string): s3.IBucket { if (!region) { - return; + return this.artifactBucket; } // get the region the Pipeline itself is in const pipelineRegion = this.requireRegion(); // if we already have an ArtifactStore generated for this region, or it's the Pipeline's region, nothing to do - if (this.artifactStores[region] || region === pipelineRegion) { - return; + if (region === pipelineRegion) { + return this.artifactBucket; } let replicationBucket = this.crossRegionReplicationBuckets[region]; @@ -371,6 +371,7 @@ export class Pipeline extends PipelineBase { stack: crossRegionScaffoldStack, replicationBucket, }; + this.crossRegionReplicationBuckets[region] = replicationBucket; } replicationBucket.grantReadWrite(this.role); @@ -378,6 +379,8 @@ export class Pipeline extends PipelineBase { location: replicationBucket.bucketName, type: 'S3', }; + + return replicationBucket; } /**