From 99ae5e7dac16349f12b19167b09887fe06a69cda Mon Sep 17 00:00:00 2001 From: Adam Ruka Date: Mon, 1 Jul 2019 17:17:45 -0700 Subject: [PATCH] fix(codepipeline): correctly pass the replication Buckets to Action.bind() (#3131) 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. As this is not a strictly backwards-compatible change (it is for customers of `IAction`, but not for Action vendors), I added a mechanism for exempting changes from our compatibility checker. You drop a file called `allowed-breaking-changes-${version}.txt` to the root of your package, where `${version}` is the version of your package (for example, `0.36.1`), and the checker will exempt the violations present in that file. --- .../allowed-breaking-changes-0.36.1.txt | 1 + .../allowed-breaking-changes-0.36.1.txt | 18 ++++++++++++++++++ .../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 +- .../allowed-breaking-changes-0.36.1.txt | 2 ++ .../@aws-cdk/aws-codepipeline/lib/action.ts | 4 ++-- .../lib/cross-region-support-stack.ts | 18 +----------------- .../@aws-cdk/aws-codepipeline/lib/pipeline.ts | 15 +++++++++------ scripts/check-api-compatibility.sh | 6 +++++- 17 files changed, 56 insertions(+), 43 deletions(-) create mode 100644 packages/@aws-cdk/app-delivery/allowed-breaking-changes-0.36.1.txt create mode 100644 packages/@aws-cdk/aws-codepipeline-actions/allowed-breaking-changes-0.36.1.txt create mode 100644 packages/@aws-cdk/aws-codepipeline/allowed-breaking-changes-0.36.1.txt diff --git a/packages/@aws-cdk/app-delivery/allowed-breaking-changes-0.36.1.txt b/packages/@aws-cdk/app-delivery/allowed-breaking-changes-0.36.1.txt new file mode 100644 index 0000000000000..390b5a9625102 --- /dev/null +++ b/packages/@aws-cdk/app-delivery/allowed-breaking-changes-0.36.1.txt @@ -0,0 +1 @@ +incompatible-argument:@aws-cdk/app-delivery.PipelineDeployStackAction.bind diff --git a/packages/@aws-cdk/aws-codepipeline-actions/allowed-breaking-changes-0.36.1.txt b/packages/@aws-cdk/aws-codepipeline-actions/allowed-breaking-changes-0.36.1.txt new file mode 100644 index 0000000000000..4c081c600b843 --- /dev/null +++ b/packages/@aws-cdk/aws-codepipeline-actions/allowed-breaking-changes-0.36.1.txt @@ -0,0 +1,18 @@ +incompatible-argument:@aws-cdk/aws-codepipeline-actions.Action.bind +incompatible-argument:@aws-cdk/aws-codepipeline-actions.Action.bound +incompatible-argument:@aws-cdk/aws-codepipeline-actions.AlexaSkillDeployAction.bound +incompatible-argument:@aws-cdk/aws-codepipeline-actions.CodeDeployServerDeployAction.bound +incompatible-argument:@aws-cdk/aws-codepipeline-actions.CloudFormationCreateReplaceChangeSetAction.bound +incompatible-argument:@aws-cdk/aws-codepipeline-actions.CloudFormationCreateUpdateStackAction.bound +incompatible-argument:@aws-cdk/aws-codepipeline-actions.CloudFormationDeleteStackAction.bound +incompatible-argument:@aws-cdk/aws-codepipeline-actions.CloudFormationExecuteChangeSetAction.bound +incompatible-argument:@aws-cdk/aws-codepipeline-actions.CodeBuildAction.bound +incompatible-argument:@aws-cdk/aws-codepipeline-actions.CodeCommitSourceAction.bound +incompatible-argument:@aws-cdk/aws-codepipeline-actions.EcrSourceAction.bound +incompatible-argument:@aws-cdk/aws-codepipeline-actions.EcsDeployAction.bound +incompatible-argument:@aws-cdk/aws-codepipeline-actions.GitHubSourceAction.bound +incompatible-argument:@aws-cdk/aws-codepipeline-actions.JenkinsAction.bound +incompatible-argument:@aws-cdk/aws-codepipeline-actions.LambdaInvokeAction.bound +incompatible-argument:@aws-cdk/aws-codepipeline-actions.ManualApprovalAction.bound +incompatible-argument:@aws-cdk/aws-codepipeline-actions.S3DeployAction.bound +incompatible-argument:@aws-cdk/aws-codepipeline-actions.S3SourceAction.bound 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/allowed-breaking-changes-0.36.1.txt b/packages/@aws-cdk/aws-codepipeline/allowed-breaking-changes-0.36.1.txt new file mode 100644 index 0000000000000..b344d13444ebc --- /dev/null +++ b/packages/@aws-cdk/aws-codepipeline/allowed-breaking-changes-0.36.1.txt @@ -0,0 +1,2 @@ +incompatible-argument:@aws-cdk/aws-codepipeline.IAction.bind +removed:@aws-cdk/aws-codepipeline.IPipeline.artifactBucket 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; } /** diff --git a/scripts/check-api-compatibility.sh b/scripts/check-api-compatibility.sh index ad63043b13427..609eac839fb81 100755 --- a/scripts/check-api-compatibility.sh +++ b/scripts/check-api-compatibility.sh @@ -44,12 +44,16 @@ fi #---------------------------------------------------------------------- +# get the current version from Lerna +current_version=$(npx lerna ls -pl | head -n 1 | cut -d ':' -f 3) + echo "Checking compatibility..." >&2 success=true for i in ${!package_dirs[*]}; do if [[ ! -d $tmpdir/node_modules/${package_names[$i]} ]]; then continue; fi echo -n "${package_names[$i]}... " - if npx jsii-diff --experimental-errors $tmpdir/node_modules/${package_names[$i]} ${package_dirs[$i]} 2>$tmpdir/output.txt; then + if npx jsii-diff --experimental-errors $tmpdir/node_modules/${package_names[$i]} ${package_dirs[$i]} \ + --ignore-file ${package_dirs[$i]}/allowed-breaking-changes-${current_version}.txt 2>$tmpdir/output.txt; then echo "OK." else success=false