Skip to content

Commit

Permalink
fix(codepipeline): correctly pass the replication Buckets to Action.b…
Browse files Browse the repository at this point in the history
…ind() (#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.
skinny85 authored Jul 2, 2019

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
1 parent 1ba8a14 commit 99ae5e7
Showing 17 changed files with 56 additions and 43 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
incompatible-argument:@aws-cdk/app-delivery.PipelineDeployStackAction.bind
Original file line number Diff line number Diff line change
@@ -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
Original file line number Diff line number Diff line change
@@ -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 {
Original file line number Diff line number Diff line change
@@ -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 = {
Original file line number Diff line number Diff line change
@@ -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({
Original file line number Diff line number Diff line change
@@ -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: {
Original file line number Diff line number Diff line change
@@ -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: {
Original file line number Diff line number Diff line change
@@ -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: {
Original file line number Diff line number Diff line change
@@ -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: {
Original file line number Diff line number Diff line change
@@ -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: {
Original file line number Diff line number Diff line change
@@ -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;
Original file line number Diff line number Diff line change
@@ -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",
},
},
{
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
incompatible-argument:@aws-cdk/aws-codepipeline.IAction.bind
removed:@aws-cdk/aws-codepipeline.IPipeline.artifactBucket
4 changes: 2 additions & 2 deletions packages/@aws-cdk/aws-codepipeline/lib/action.ts
Original file line number Diff line number Diff line change
@@ -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.
*
Original file line number Diff line number Diff line change
@@ -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,27 +44,12 @@ 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,
});
}
}

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());
}
15 changes: 9 additions & 6 deletions packages/@aws-cdk/aws-codepipeline/lib/pipeline.ts
Original file line number Diff line number Diff line change
@@ -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,14 +304,15 @@ 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);

// 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,13 +371,16 @@ export class Pipeline extends PipelineBase {
stack: crossRegionScaffoldStack,
replicationBucket,
};
this.crossRegionReplicationBuckets[region] = replicationBucket;
}
replicationBucket.grantReadWrite(this.role);

this.artifactStores[region] = {
location: replicationBucket.bucketName,
type: 'S3',
};

return replicationBucket;
}

/**
6 changes: 5 additions & 1 deletion scripts/check-api-compatibility.sh
Original file line number Diff line number Diff line change
@@ -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

0 comments on commit 99ae5e7

Please sign in to comment.