Skip to content

Commit 390ec78

Browse files
rix0rrrrix0rrr
authored
fix(codedeploy): cross-region referenced groups use wrong config (#23986)
DeploymentGroups grant IAM permissions to their DeploymentConfigs, by their ARN. When using a predefined DeploymentConfig (like `ECS_ALL_AT_ONCE`) however, we used to use `Aws.ACCOUNT_ID, Aws.REGION` to build the ARN for the DeploymentConfig. This would be incorrect if the DeploymentGroup is referenced in a different region (by using `DeploymentGroup.fromDeploymentGroupArn()`): the `Aws.REGION` token would resolve to the region of the *referencing Stack*, instead of the region of the *referenced DeploymentGroup*. Make all predefined DeploymentConfigs implement a hidden interface that allows the DeploymentGroup to specialize the Config to its own account and region. This behavior is not relevant for user-created deployment configs: those will still be region-bound, so customers won't need access to this interface. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
1 parent 46cadd4 commit 390ec78

10 files changed

+158
-70
lines changed

‎.gitallowed

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,11 @@ account: '012345678913'
1414

1515
# Account patterns used in the CHANGELOG
1616
account: '123456789012'
17+
18+
111111111111
19+
222222222222
1720
123456789012
21+
333333333333
1822

1923
# The account ID's of public facing ECR images for App Mesh Envoy
2024
# https://docs.aws.amazon.com/app-mesh/latest/userguide/envoy.html

‎packages/@aws-cdk/aws-codedeploy/lib/ecs/deployment-group.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -222,7 +222,7 @@ export class EcsDeploymentGroup extends DeploymentGroupBase implements IEcsDeplo
222222
this.alarms = props.alarms || [];
223223

224224
this.role.addManagedPolicy(iam.ManagedPolicy.fromAwsManagedPolicyName('AWSCodeDeployRoleForECS'));
225-
this.deploymentConfig = props.deploymentConfig || EcsDeploymentConfig.ALL_AT_ONCE;
225+
this.deploymentConfig = this._bindDeploymentConfig(props.deploymentConfig || EcsDeploymentConfig.ALL_AT_ONCE);
226226

227227
if (cdk.Resource.isOwnedResource(props.service)) {
228228
const cfnSvc = (props.service as ecs.BaseService).node.defaultChild as ecs.CfnService;
@@ -358,6 +358,6 @@ class ImportedEcsDeploymentGroup extends ImportedDeploymentGroupBase implements
358358
});
359359

360360
this.application = props.application;
361-
this.deploymentConfig = props.deploymentConfig || EcsDeploymentConfig.ALL_AT_ONCE;
361+
this.deploymentConfig = this._bindDeploymentConfig(props.deploymentConfig || EcsDeploymentConfig.ALL_AT_ONCE);
362362
}
363363
}

‎packages/@aws-cdk/aws-codedeploy/lib/lambda/deployment-group.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -162,7 +162,7 @@ export class LambdaDeploymentGroup extends DeploymentGroupBase implements ILambd
162162
this.alarms = props.alarms || [];
163163

164164
this.role.addManagedPolicy(iam.ManagedPolicy.fromAwsManagedPolicyName('service-role/AWSCodeDeployRoleForLambdaLimited'));
165-
this.deploymentConfig = props.deploymentConfig || LambdaDeploymentConfig.CANARY_10PERCENT_5MINUTES;
165+
this.deploymentConfig = this._bindDeploymentConfig(props.deploymentConfig || LambdaDeploymentConfig.CANARY_10PERCENT_5MINUTES);
166166

167167
const resource = new CfnDeploymentGroup(this, 'Resource', {
168168
applicationName: this.application.applicationName,
@@ -290,6 +290,6 @@ class ImportedLambdaDeploymentGroup extends ImportedDeploymentGroupBase implemen
290290
});
291291

292292
this.application = props.application;
293-
this.deploymentConfig = props.deploymentConfig || LambdaDeploymentConfig.CANARY_10PERCENT_5MINUTES;
293+
this.deploymentConfig = this._bindDeploymentConfig(props.deploymentConfig || LambdaDeploymentConfig.CANARY_10PERCENT_5MINUTES);
294294
}
295295
}

‎packages/@aws-cdk/aws-codedeploy/lib/private/base-deployment-group.ts

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,9 @@
11
import * as iam from '@aws-cdk/aws-iam';
22
import { Resource, IResource, ArnFormat, Arn, Aws } from '@aws-cdk/core';
33
import { Construct } from 'constructs';
4+
import { IBaseDeploymentConfig } from '../base-deployment-config';
45
import { CfnDeploymentGroup } from '../codedeploy.generated';
6+
import { isPredefinedDeploymentConfig } from './predefined-deployment-config';
57
import { validateName } from './utils';
68

79
/**
@@ -52,6 +54,15 @@ export class ImportedDeploymentGroupBase extends Resource {
5254
this.deploymentGroupName = deploymentGroupName;
5355
this.deploymentGroupArn = deploymentGroupArn;
5456
}
57+
58+
/**
59+
* Bind DeploymentGroupConfig to the current group, if supported
60+
*
61+
* @internal
62+
*/
63+
protected _bindDeploymentConfig(config: IBaseDeploymentConfig) {
64+
return isPredefinedDeploymentConfig(config) ? config.bindEnvironment(this) : config;
65+
}
5566
}
5667

5768
export interface DeploymentGroupBaseProps {
@@ -114,6 +125,15 @@ export class DeploymentGroupBase extends Resource {
114125
this.node.addValidation({ validate: () => validateName('Deployment group', this.physicalName) });
115126
}
116127

128+
/**
129+
* Bind DeploymentGroupConfig to the current group, if supported
130+
*
131+
* @internal
132+
*/
133+
protected _bindDeploymentConfig(config: IBaseDeploymentConfig) {
134+
return isPredefinedDeploymentConfig(config) ? config.bindEnvironment(this) : config;
135+
}
136+
117137
/**
118138
* Set name and ARN properties.
119139
*
@@ -135,4 +155,4 @@ export class DeploymentGroupBase extends Resource {
135155
arnFormat: ArnFormat.COLON_RESOURCE_NAME,
136156
});
137157
}
138-
}
158+
}

‎packages/@aws-cdk/aws-codedeploy/lib/private/predefined-deployment-config.ts

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
import { IResource } from '@aws-cdk/core';
2+
import { IBaseDeploymentConfig } from '../base-deployment-config';
3+
4+
/**
5+
* A reference to a DeploymentConfig that is managed by AWS
6+
*
7+
* Since these DeploymentConfigs are present in every region, and we might use
8+
* them in conjunction with cross-region DeploymentGroups, we need to specialize
9+
* the account and region to the DeploymentGroup before using.
10+
*
11+
* A DeploymentGroup must call `bindEnvironment()` first if it detects this type,
12+
* before reading the DeploymentConfig ARN.
13+
*
14+
* This type is fully hidden, which means that the constant objects provided by
15+
* CDK will have magical behavior that customers can't reimplement themselves.
16+
* Not ideal, but our DeploymentConfig type inheritance is already overly
17+
* complicated and to do it properly with the nominal typing we are emplying
18+
* will require adding 4 more empty or nearly empty interfaces, which seems a
19+
* bit silly for a need that's not necessarily clearly needed by customers.
20+
* We can always move to exposing later.
21+
*/
22+
export interface IPredefinedDeploymentConfig {
23+
/**
24+
* Bind the predefined deployment config to the environment of the given resource
25+
*/
26+
bindEnvironment(deploymentGroup: IResource): IBaseDeploymentConfig;
27+
}
28+
29+
export function isPredefinedDeploymentConfig(x: unknown): x is IPredefinedDeploymentConfig {
30+
return typeof x === 'object' && !!x && !!(x as any).bindEnvironment;
31+
}

‎packages/@aws-cdk/aws-codedeploy/lib/private/utils.ts

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,9 @@
11
import * as cloudwatch from '@aws-cdk/aws-cloudwatch';
2-
import { Token, Stack, ArnFormat, Arn, Fn, Aws } from '@aws-cdk/core';
2+
import { Token, Stack, ArnFormat, Arn, Fn, Aws, IResource } from '@aws-cdk/core';
33
import { IBaseDeploymentConfig } from '../base-deployment-config';
44
import { CfnDeploymentGroup } from '../codedeploy.generated';
55
import { AutoRollbackConfig } from '../rollback-config';
6+
import { IPredefinedDeploymentConfig } from './predefined-deployment-config';
67

78
export function arnForApplication(stack: Stack, applicationName: string): string {
89
return stack.formatArn({
@@ -18,11 +19,11 @@ export function nameFromDeploymentGroupArn(deploymentGroupArn: string): string {
1819
return Fn.select(1, Fn.split('/', components.resourceName ?? ''));
1920
}
2021

21-
export function arnForDeploymentConfig(name: string): string {
22+
export function arnForDeploymentConfig(name: string, resource?: IResource): string {
2223
return Arn.format({
2324
partition: Aws.PARTITION,
24-
account: Aws.ACCOUNT_ID,
25-
region: Aws.REGION,
25+
account: resource?.env.account ?? Aws.ACCOUNT_ID,
26+
region: resource?.env.region ?? Aws.REGION,
2627
service: 'codedeploy',
2728
resource: 'deploymentconfig',
2829
resourceName: name,
@@ -41,10 +42,14 @@ CfnDeploymentGroup.AlarmConfigurationProperty | undefined {
4142
};
4243
}
4344

44-
export function deploymentConfig(name: string): IBaseDeploymentConfig {
45+
export function deploymentConfig(name: string): IBaseDeploymentConfig & IPredefinedDeploymentConfig {
4546
return {
4647
deploymentConfigName: name,
4748
deploymentConfigArn: arnForDeploymentConfig(name),
49+
bindEnvironment: (resource) => ({
50+
deploymentConfigName: name,
51+
deploymentConfigArn: arnForDeploymentConfig(name, resource),
52+
}),
4853
};
4954
}
5055

‎packages/@aws-cdk/aws-codedeploy/lib/server/deployment-group.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,7 @@ class ImportedServerDeploymentGroup extends ImportedDeploymentGroupBase implemen
6868
});
6969

7070
this.application = props.application;
71-
this.deploymentConfig = props.deploymentConfig || ServerDeploymentConfig.ONE_AT_A_TIME;
71+
this.deploymentConfig = this._bindDeploymentConfig(props.deploymentConfig || ServerDeploymentConfig.ONE_AT_A_TIME);
7272
}
7373
}
7474

@@ -255,7 +255,7 @@ export class ServerDeploymentGroup extends DeploymentGroupBase implements IServe
255255
this.application = props.application || new ServerApplication(this, 'Application', {
256256
applicationName: props.deploymentGroupName === cdk.PhysicalName.GENERATE_IF_NEEDED ? cdk.PhysicalName.GENERATE_IF_NEEDED : undefined,
257257
});
258-
this.deploymentConfig = props.deploymentConfig || ServerDeploymentConfig.ONE_AT_A_TIME;
258+
this.deploymentConfig = this._bindDeploymentConfig(props.deploymentConfig || ServerDeploymentConfig.ONE_AT_A_TIME);
259259

260260
this.role.addManagedPolicy(iam.ManagedPolicy.fromAwsManagedPolicyName('service-role/AWSCodeDeployRole'));
261261
this._autoScalingGroups = props.autoScalingGroups || [];

‎packages/@aws-cdk/aws-codedeploy/test/ecs/deployment-group.test.ts

Lines changed: 28 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ import * as ecs from '@aws-cdk/aws-ecs';
55
import * as elbv2 from '@aws-cdk/aws-elasticloadbalancingv2';
66
import * as iam from '@aws-cdk/aws-iam';
77
import * as cdk from '@aws-cdk/core';
8-
import { Duration } from '@aws-cdk/core';
8+
import { Duration, Stack } from '@aws-cdk/core';
99
import * as codedeploy from '../../lib';
1010

1111
const mockCluster = 'my-cluster';
@@ -45,7 +45,7 @@ describe('CodeDeploy ECS DeploymentGroup', () => {
4545
deploymentGroupName: 'EcsDeploymentGroup',
4646
});
4747

48-
expect(importedGroup.deploymentConfig).toEqual(codedeploy.EcsDeploymentConfig.ALL_AT_ONCE);
48+
expect(importedGroup.deploymentConfig.deploymentConfigName).toEqual('CodeDeployDefault.ECSAllAtOnce');
4949
});
5050
});
5151

@@ -849,25 +849,34 @@ describe('CodeDeploy ECS DeploymentGroup', () => {
849849
});
850850
});
851851

852-
test('deploymentGroup from Arn knows its account and region', () => {
853-
// GIVEN
854-
const stack = new cdk.Stack(undefined, 'Stack', { env: { account: '111111111111', region: 'blabla-1' } });
852+
describe('deploymentGroup from ARN in different account and region', () => {
853+
let stack: Stack;
854+
let application: codedeploy.IEcsApplication;
855+
let group: codedeploy.IEcsDeploymentGroup;
855856

856-
// WHEN
857-
const application = codedeploy.EcsApplication.fromEcsApplicationArn(stack, 'Application', 'arn:aws:codedeploy:theregion-1:222222222222:application:MyApplication');
858-
const group = codedeploy.EcsDeploymentGroup.fromEcsDeploymentGroupAttributes(stack, 'Group', {
859-
application,
860-
deploymentGroupName: 'DeploymentGroup',
857+
const account = '222222222222';
858+
const region = 'theregion-1';
859+
860+
beforeEach(() => {
861+
stack = new cdk.Stack(undefined, 'Stack', { env: { account: '111111111111', region: 'blabla-1' } });
862+
863+
application = codedeploy.EcsApplication.fromEcsApplicationArn(stack, 'Application', `arn:aws:codedeploy:${region}:${account}:application:MyApplication`);
864+
group = codedeploy.EcsDeploymentGroup.fromEcsDeploymentGroupAttributes(stack, 'Group', {
865+
application,
866+
deploymentGroupName: 'DeploymentGroup',
867+
});
861868
});
862869

863-
// THEN
864-
expect(application.env).toEqual(expect.objectContaining({
865-
account: '222222222222',
866-
region: 'theregion-1',
867-
}));
868-
expect(group.env).toEqual(expect.objectContaining({
869-
account: '222222222222',
870-
region: 'theregion-1',
871-
}));
870+
test('knows its account and region', () => {
871+
// THEN
872+
expect(application.env).toEqual(expect.objectContaining({ account, region }));
873+
expect(group.env).toEqual(expect.objectContaining({ account, region }));
874+
});
875+
876+
test('references the predefined DeploymentGroupConfig in the right region', () => {
877+
expect(group.deploymentConfig.deploymentConfigArn).toEqual(expect.stringContaining(
878+
`:codedeploy:${region}:${account}:deploymentconfig:CodeDeployDefault.ECSAllAtOnce`,
879+
));
880+
});
872881
});
873882
});

‎packages/@aws-cdk/aws-codedeploy/test/lambda/deployment-group.test.ts

Lines changed: 30 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ import * as cloudwatch from '@aws-cdk/aws-cloudwatch';
33
import * as iam from '@aws-cdk/aws-iam';
44
import * as lambda from '@aws-cdk/aws-lambda';
55
import * as cdk from '@aws-cdk/core';
6+
import { Stack } from '@aws-cdk/core';
67
import * as codedeploy from '../../lib';
78
import { TrafficRouting } from '../../lib';
89

@@ -616,26 +617,35 @@ describe('CodeDeploy Lambda DeploymentGroup', () => {
616617
});
617618
});
618619

619-
test('deploymentGroup from Arn knows its account and region', () => {
620-
// GIVEN
621-
const stack = new cdk.Stack(undefined, 'Stack', { env: { account: '111111111111', region: 'blabla-1' } });
620+
describe('deploymentGroup from ARN in different account and region', () => {
621+
let stack: Stack;
622+
let application: codedeploy.ILambdaApplication;
623+
let group: codedeploy.ILambdaDeploymentGroup;
622624

623-
// WHEN
624-
const application = codedeploy.LambdaApplication.fromLambdaApplicationArn(stack, 'Application', 'arn:aws:codedeploy:theregion-1:222222222222:application:MyApplication');
625-
const group = codedeploy.LambdaDeploymentGroup.fromLambdaDeploymentGroupAttributes(stack, 'Group', {
626-
application,
627-
deploymentGroupName: 'DeploymentGroup',
628-
});
629-
630-
// THEN
631-
expect(application.env).toEqual(expect.objectContaining({
632-
account: '222222222222',
633-
region: 'theregion-1',
634-
}));
635-
expect(group.env).toEqual(expect.objectContaining({
636-
account: '222222222222',
637-
region: 'theregion-1',
638-
}));
625+
const account = '222222222222';
626+
const region = 'theregion-1';
627+
628+
beforeEach(() => {
629+
stack = new cdk.Stack(undefined, 'Stack', { env: { account: '111111111111', region: 'blabla-1' } });
630+
631+
application = codedeploy.LambdaApplication.fromLambdaApplicationArn(stack, 'Application', `arn:aws:codedeploy:${region}:${account}:application:MyApplication`);
632+
group = codedeploy.LambdaDeploymentGroup.fromLambdaDeploymentGroupAttributes(stack, 'Group', {
633+
application,
634+
deploymentGroupName: 'DeploymentGroup',
635+
});
636+
});
637+
638+
test('knows its account and region', () => {
639+
// THEN
640+
expect(application.env).toEqual(expect.objectContaining({ account, region }));
641+
expect(group.env).toEqual(expect.objectContaining({ account, region }));
642+
});
643+
644+
test('references the predefined DeploymentGroupConfig in the right region', () => {
645+
expect(group.deploymentConfig.deploymentConfigArn).toEqual(expect.stringContaining(
646+
`:codedeploy:${region}:${account}:deploymentconfig:CodeDeployDefault.LambdaCanary10Percent5Minutes`,
647+
));
648+
});
639649
});
640650
});
641651

@@ -649,7 +659,7 @@ describe('imported with fromLambdaDeploymentGroupAttributes', () => {
649659
deploymentGroupName: 'LambdaDeploymentGroup',
650660
});
651661

652-
expect(importedGroup.deploymentConfig).toEqual(codedeploy.LambdaDeploymentConfig.CANARY_10PERCENT_5MINUTES);
662+
expect(importedGroup.deploymentConfig.deploymentConfigName).toEqual('CodeDeployDefault.LambdaCanary10Percent5Minutes');
653663
});
654664
});
655665

‎packages/@aws-cdk/aws-codedeploy/test/server/deployment-group.test.ts

Lines changed: 28 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -494,25 +494,34 @@ describe('CodeDeploy Server Deployment Group', () => {
494494
expect(() => app.synth()).toThrow('Deployment group name: "my name" can only contain letters (a-z, A-Z), numbers (0-9), periods (.), underscores (_), + (plus signs), = (equals signs), , (commas), @ (at signs), - (minus signs).');
495495
});
496496

497-
test('deploymentGroup from Arn knows its account and region', () => {
498-
// GIVEN
499-
const stack = new cdk.Stack(undefined, 'Stack', { env: { account: '111111111111', region: 'blabla-1' } });
497+
describe('deploymentGroup from ARN in different account and region', () => {
498+
let stack: cdk.Stack;
499+
let application: codedeploy.IServerApplication;
500+
let group: codedeploy.IServerDeploymentGroup;
500501

501-
// WHEN
502-
const application = codedeploy.ServerApplication.fromServerApplicationArn(stack, 'Application', 'arn:aws:codedeploy:theregion-1:222222222222:application:MyApplication');
503-
const group = codedeploy.ServerDeploymentGroup.fromServerDeploymentGroupAttributes(stack, 'Group', {
504-
application,
505-
deploymentGroupName: 'DeploymentGroup',
506-
});
507-
508-
// THEN
509-
expect(application.env).toEqual(expect.objectContaining({
510-
account: '222222222222',
511-
region: 'theregion-1',
512-
}));
513-
expect(group.env).toEqual(expect.objectContaining({
514-
account: '222222222222',
515-
region: 'theregion-1',
516-
}));
502+
const account = '222222222222';
503+
const region = 'theregion-1';
504+
505+
beforeEach(() => {
506+
stack = new cdk.Stack(undefined, 'Stack', { env: { account: '111111111111', region: 'blabla-1' } });
507+
508+
application = codedeploy.ServerApplication.fromServerApplicationArn(stack, 'Application', `arn:aws:codedeploy:${region}:${account}:application:MyApplication`);
509+
group = codedeploy.ServerDeploymentGroup.fromServerDeploymentGroupAttributes(stack, 'Group', {
510+
application,
511+
deploymentGroupName: 'DeploymentGroup',
512+
});
513+
});
514+
515+
test('knows its account and region', () => {
516+
// THEN
517+
expect(application.env).toEqual(expect.objectContaining({ account, region }));
518+
expect(group.env).toEqual(expect.objectContaining({ account, region }));
519+
});
520+
521+
test('references the predefined DeploymentGroupConfig in the right region', () => {
522+
expect(group.deploymentConfig.deploymentConfigArn).toEqual(expect.stringContaining(
523+
`:codedeploy:${region}:${account}:deploymentconfig:CodeDeployDefault.OneAtATime`,
524+
));
525+
});
517526
});
518527
});

0 commit comments

Comments
 (0)