Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(codedeploy): LambdaDeploymentGroup now takes IRole #1840

Merged
merged 2 commits into from
Feb 27, 2019
Merged

fix(codedeploy): LambdaDeploymentGroup now takes IRole #1840

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
59bc347
fix(codedeploy): LambdaDeploymentGroup now takes IRole
rix0rrr Feb 22, 2019
a09cb25
Fix test that asserts we add principal to provided role
rix0rrr Feb 27, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 9 additions & 18 deletions packages/@aws-cdk/aws-codedeploy/lib/lambda/deployment-group.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ export interface LambdaDeploymentGroupProps {
*
* @default one will be created for you
*/
application?: LambdaApplication;
application?: ILambdaApplication;

/**
* The physical, human-readable name of the CodeDeploy Deployment Group.
Expand Down Expand Up @@ -76,7 +76,7 @@ export interface LambdaDeploymentGroupProps {
*
* @default a new Role will be created.
*/
role?: iam.Role;
role?: iam.IRole;

/**
* Lambda Alias to shift traffic. Updating the version
Expand Down Expand Up @@ -124,7 +124,7 @@ export class LambdaDeploymentGroup extends cdk.Construct implements ILambdaDeplo
public readonly application: ILambdaApplication;
public readonly deploymentGroupName: string;
public readonly deploymentGroupArn: string;
public readonly role: iam.Role;
public readonly role: iam.IRole;

private readonly alarms: cloudwatch.Alarm[];
private preHook?: lambda.IFunction;
Expand All @@ -136,24 +136,15 @@ export class LambdaDeploymentGroup extends cdk.Construct implements ILambdaDeplo
this.application = props.application || new LambdaApplication(this, 'Application');
this.alarms = props.alarms || [];

let serviceRole: iam.Role | undefined = props.role;
if (serviceRole) {
if (serviceRole.assumeRolePolicy) {
serviceRole.assumeRolePolicy.addStatement(new iam.PolicyStatement()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part of the behavior is not retained by the new code. That constitutes a breaking change.

Either it needs to be restored (if props.role is set, then make the role assumable by code deploy.amazonaws.com), or the breaking change needs to be mentioned.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right.

I removed it because this doesn't match our behavior anywhere else where we do BOYR, but it does constitute a breaking change. And the build is probably failing because there's a test asserting that we DO do this :).

.addAction('sts:AssumeRole')
.addServicePrincipal('codedeploy.amazonaws.com'));
}
} else {
serviceRole = new iam.Role(this, 'ServiceRole', {
assumedBy: new iam.ServicePrincipal('codedeploy.amazonaws.com')
});
}
serviceRole.attachManagedPolicy('arn:aws:iam::aws:policy/service-role/AWSCodeDeployRoleForLambda');
this.role = serviceRole;
this.role = props.role || new iam.Role(this, 'ServiceRole', {
assumedBy: new iam.ServicePrincipal('codedeploy.amazonaws.com')
});

this.role.attachManagedPolicy('arn:aws:iam::aws:policy/service-role/AWSCodeDeployRoleForLambda');

const resource = new CfnDeploymentGroup(this, 'Resource', {
applicationName: this.application.applicationName,
serviceRoleArn: serviceRole.roleArn,
serviceRoleArn: this.role.roleArn,
deploymentGroupName: props.deploymentGroupName,
deploymentConfigName: (props.deploymentConfig || LambdaDeploymentConfig.AllAtOnce).deploymentConfigName,
deploymentStyle: {
Expand Down