Skip to content

Commit c7b71bd

Browse files
bun913bun913
authored
refactor(ecs): refactor addPortMapping method (#24227)
This PR... - Refactor ecs.ContainerDefintion.addPortMapping method - The addPortMapping method had many if statements and it was difficult to understand what was being determined. - It was also difficult to make changes. - Therefore, I divided the classes by interest to improve visibility. Closes #24170 . ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
1 parent a156221 commit c7b71bd

File tree

2 files changed

+294
-36
lines changed

2 files changed

+294
-36
lines changed

‎packages/@aws-cdk/aws-ecs/lib/container-definition.ts

Lines changed: 144 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -566,43 +566,15 @@ export class ContainerDefinition extends Construct {
566566
*/
567567
public addPortMappings(...portMappings: PortMapping[]) {
568568
this.portMappings.push(...portMappings.map(pm => {
569-
if (this.taskDefinition.networkMode === NetworkMode.AWS_VPC || this.taskDefinition.networkMode === NetworkMode.HOST) {
570-
if (pm.containerPort !== pm.hostPort && pm.hostPort !== undefined) {
571-
throw new Error(`Host port (${pm.hostPort}) must be left out or equal to container port ${pm.containerPort} for network mode ${this.taskDefinition.networkMode}`);
572-
}
573-
}
574-
// No empty strings as port mapping names.
575-
if (pm.name === '') {
576-
throw new Error('Port mapping name cannot be an empty string.');
577-
}
578-
// Service connect logic.
579-
if (pm.name || pm.appProtocol) {
580-
581-
// Service connect only supports Awsvpc and Bridge network modes.
582-
if (![NetworkMode.BRIDGE, NetworkMode.AWS_VPC].includes(this.taskDefinition.networkMode)) {
583-
throw new Error(`Service connect related port mapping fields 'name' and 'appProtocol' are not supported for network mode ${this.taskDefinition.networkMode}`);
584-
}
585-
586-
// Name is not set but App Protocol is; this config is meaningless and we should throw.
587-
if (!pm.name) {
588-
throw new Error('Service connect-related port mapping field \'appProtocol\' cannot be set without \'name\'');
589-
}
590-
591-
if (this._namedPorts.has(pm.name)) {
592-
throw new Error(`Port mapping name '${pm.name}' already exists on this container`);
593-
}
594-
this._namedPorts.set(pm.name, pm);
569+
const portMap = new PortMap(this.taskDefinition.networkMode, pm);
570+
portMap.validate();
571+
const serviceConnect = new ServiceConnect(this.taskDefinition.networkMode, pm);
572+
if (serviceConnect.isServiceConnect()) {
573+
serviceConnect.validate();
574+
this.setNamedPort(pm);
595575
}
596-
597-
if (this.taskDefinition.networkMode === NetworkMode.BRIDGE) {
598-
if (pm.hostPort === undefined) {
599-
pm = {
600-
...pm,
601-
hostPort: 0,
602-
};
603-
}
604-
}
605-
return pm;
576+
const sanitizedPM = this.addHostPortIfNeeded(pm);
577+
return sanitizedPM;
606578
}));
607579
}
608580

@@ -688,6 +660,32 @@ export class ContainerDefinition extends Construct {
688660
return this._namedPorts.get(name);
689661
}
690662

663+
/**
664+
* This method adds an namedPort
665+
*/
666+
private setNamedPort(pm: PortMapping) :void {
667+
if (!pm.name) return;
668+
if (this._namedPorts.has(pm.name)) {
669+
throw new Error(`Port mapping name '${pm.name}' already exists on this container`);
670+
}
671+
this._namedPorts.set(pm.name, pm);
672+
}
673+
674+
675+
/**
676+
* Set HostPort to 0 When netowork mode is Brdige
677+
*/
678+
private addHostPortIfNeeded(pm: PortMapping) :PortMapping {
679+
const newPM = {
680+
...pm,
681+
};
682+
if (this.taskDefinition.networkMode !== NetworkMode.BRIDGE) return newPM;
683+
if (pm.hostPort !== undefined) return newPM;
684+
newPM.hostPort = 0;
685+
return newPM;
686+
}
687+
688+
691689
/**
692690
* Whether this container definition references a specific JSON field of a secret
693691
* stored in Secrets Manager.
@@ -1086,6 +1084,116 @@ export interface PortMapping {
10861084
readonly appProtocol?: AppProtocol;
10871085
}
10881086

1087+
/**
1088+
* PortMap ValueObjectClass having by ContainerDefinition
1089+
*/
1090+
export class PortMap {
1091+
1092+
/**
1093+
* The networking mode to use for the containers in the task.
1094+
*/
1095+
readonly networkmode: NetworkMode;
1096+
1097+
/**
1098+
* Port mappings allow containers to access ports on the host container instance to send or receive traffic.
1099+
*/
1100+
readonly portmapping: PortMapping;
1101+
1102+
constructor(networkmode: NetworkMode, pm: PortMapping) {
1103+
this.networkmode = networkmode;
1104+
this.portmapping = pm;
1105+
}
1106+
1107+
/**
1108+
* validate invalid portmapping and networkmode parameters.
1109+
* throw Error when invalid parameters.
1110+
*/
1111+
public validate(): void {
1112+
if (!this.isvalidPortName()) {
1113+
throw new Error('Port mapping name cannot be an empty string.');
1114+
}
1115+
if (!this.isValidPorts()) {
1116+
const pm = this.portmapping;
1117+
throw new Error(`Host port (${pm.hostPort}) must be left out or equal to container port ${pm.containerPort} for network mode ${this.networkmode}`);
1118+
}
1119+
}
1120+
1121+
private isvalidPortName(): boolean {
1122+
if (this.portmapping.name === '') {
1123+
return false;
1124+
}
1125+
return true;
1126+
}
1127+
1128+
private isValidPorts() :boolean {
1129+
const isAwsVpcMode = this.networkmode == NetworkMode.AWS_VPC;
1130+
const isHostMode = this.networkmode == NetworkMode.HOST;
1131+
if (!isAwsVpcMode && !isHostMode) return true;
1132+
const hostPort = this.portmapping.hostPort;
1133+
const containerPort = this.portmapping.containerPort;
1134+
if (containerPort !== hostPort && hostPort !== undefined ) return false;
1135+
return true;
1136+
}
1137+
1138+
}
1139+
1140+
1141+
/**
1142+
* ServiceConnect ValueObjectClass having by ContainerDefinition
1143+
*/
1144+
export class ServiceConnect {
1145+
/**
1146+
* Port mappings allow containers to access ports on the host container instance to send or receive traffic.
1147+
*/
1148+
readonly portmapping: PortMapping;
1149+
1150+
/**
1151+
* The networking mode to use for the containers in the task.
1152+
*/
1153+
readonly networkmode: NetworkMode;
1154+
1155+
constructor(networkmode: NetworkMode, pm: PortMapping) {
1156+
this.portmapping = pm;
1157+
this.networkmode = networkmode;
1158+
}
1159+
1160+
/**
1161+
* Judge parameters can be serviceconnect logick.
1162+
* If parameters can be serviceConnect return true.
1163+
*/
1164+
public isServiceConnect() :boolean {
1165+
const hasPortname = this.portmapping.name;
1166+
const hasAppProtcol = this.portmapping.appProtocol;
1167+
if (hasPortname || hasAppProtcol) return true;
1168+
return false;
1169+
}
1170+
1171+
/**
1172+
* Judge serviceconnect parametes are valid.
1173+
* If invalid, throw Error.
1174+
*/
1175+
public validate() :void {
1176+
if (!this.isValidNetworkmode()) {
1177+
throw new Error(`Service connect related port mapping fields 'name' and 'appProtocol' are not supported for network mode ${this.networkmode}`);
1178+
}
1179+
if (!this.isValidPortName()) {
1180+
throw new Error('Service connect-related port mapping field \'appProtocol\' cannot be set without \'name\'');
1181+
}
1182+
}
1183+
1184+
private isValidNetworkmode() :boolean {
1185+
const isAwsVpcMode = this.networkmode == NetworkMode.AWS_VPC;
1186+
const isBridgeMode = this.networkmode == NetworkMode.BRIDGE;
1187+
if (isAwsVpcMode || isBridgeMode) return true;
1188+
return false;
1189+
}
1190+
1191+
private isValidPortName() :boolean {
1192+
if (!this.portmapping.name) return false;
1193+
return true;
1194+
}
1195+
}
1196+
10891197
/**
10901198
* Network protocol
10911199
*/

‎packages/@aws-cdk/aws-ecs/test/container-definition.test.ts

Lines changed: 150 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,156 @@ describe('container definition', () => {
3535
});
3636
});
3737

38+
describe('PortMap validates', () => {
39+
test('throws when PortMapping.name is empty string.', () => {
40+
// GIVEN
41+
const portMapping: ecs.PortMapping = {
42+
containerPort: 8080,
43+
name: '',
44+
};
45+
const networkmode = ecs.NetworkMode.AWS_VPC;
46+
const portMap = new ecs.PortMap(networkmode, portMapping);
47+
// THEN
48+
expect(() => {
49+
portMap.validate();
50+
}).toThrow();
51+
});
52+
53+
describe('ContainerPort should not eqaul Hostport', () => {
54+
test('when AWS_VPC Networkmode', () => {
55+
// GIVEN
56+
const portMapping: ecs.PortMapping = {
57+
containerPort: 8080,
58+
hostPort: 8081,
59+
};
60+
const networkmode = ecs.NetworkMode.AWS_VPC;
61+
const portMap = new ecs.PortMap(networkmode, portMapping);
62+
// THEN
63+
expect(() => {
64+
portMap.validate();
65+
}).toThrow();
66+
});
67+
68+
test('when Host Networkmode', () => {
69+
// GIVEN
70+
const portMapping: ecs.PortMapping = {
71+
containerPort: 8080,
72+
hostPort: 8081,
73+
};
74+
const networkmode = ecs.NetworkMode.HOST;
75+
const portMap = new ecs.PortMap(networkmode, portMapping);
76+
// THEN
77+
expect(() => {
78+
portMap.validate();
79+
}).toThrow();
80+
});
81+
});
82+
83+
describe('ContainerPort can equal HostPort cases', () => {
84+
test('when Bridge Networkmode', () => {
85+
// GIVEN
86+
const portMapping: ecs.PortMapping = {
87+
containerPort: 8080,
88+
hostPort: 8080,
89+
};
90+
const networkmode = ecs.NetworkMode.BRIDGE;
91+
const portMap = new ecs.PortMap(networkmode, portMapping);
92+
// THEN
93+
expect(() => {
94+
portMap.validate();
95+
}).not.toThrow();
96+
});
97+
98+
});
99+
100+
});
101+
102+
describe('ServiceConnect class', () => {
103+
describe('isServiceConnect', () => {
104+
test('return true if params has portname', () => {
105+
// GIVEN
106+
const portMapping: ecs.PortMapping = {
107+
containerPort: 8080,
108+
name: 'test',
109+
};
110+
const networkmode = ecs.NetworkMode.AWS_VPC;
111+
const serviceConnect = new ecs.ServiceConnect(networkmode, portMapping);
112+
// THEN
113+
expect(serviceConnect.isServiceConnect()).toEqual(true);
114+
});
115+
116+
test('return true if params has appProtocol', () => {
117+
// GIVEN
118+
const portMapping: ecs.PortMapping = {
119+
containerPort: 8080,
120+
appProtocol: ecs.AppProtocol.http2,
121+
};
122+
const networkmode = ecs.NetworkMode.AWS_VPC;
123+
const serviceConnect = new ecs.ServiceConnect(networkmode, portMapping);
124+
// THEN
125+
expect(serviceConnect.isServiceConnect()).toEqual(true);
126+
});
127+
128+
test('return false if params has not appProtocl and portName ', () => {
129+
// GIVEN
130+
const portMapping: ecs.PortMapping = {
131+
containerPort: 8080,
132+
};
133+
const networkmode = ecs.NetworkMode.AWS_VPC;
134+
const serviceConnect = new ecs.ServiceConnect(networkmode, portMapping);
135+
// THEN
136+
expect(serviceConnect.isServiceConnect()).toEqual(false);
137+
});
138+
139+
});
140+
141+
describe('validate', () => {
142+
test('throw if Host Networkmode', () => {
143+
// GIVEN
144+
const portMapping: ecs.PortMapping = {
145+
containerPort: 8080,
146+
name: 'test',
147+
};
148+
const networkmode = ecs.NetworkMode.HOST;
149+
const serviceConnect = new ecs.ServiceConnect(networkmode, portMapping);
150+
// THEN
151+
expect(() => {
152+
serviceConnect.validate();
153+
}).toThrow();
154+
});
155+
156+
test('throw if has not portmap name', () => {
157+
// GIVEN
158+
const portMapping: ecs.PortMapping = {
159+
containerPort: 8080,
160+
appProtocol: ecs.AppProtocol.http2,
161+
};
162+
const networkmode = ecs.NetworkMode.AWS_VPC;
163+
const serviceConnect = new ecs.ServiceConnect(networkmode, portMapping);
164+
// THEN
165+
expect(() => {
166+
serviceConnect.validate();
167+
}).toThrow('Service connect-related port mapping field \'appProtocol\' cannot be set without \'name\'');
168+
});
169+
170+
test('should not throw if AWS_VPC NetworkMode and has portname', () => {
171+
// GIVEN
172+
const portMapping: ecs.PortMapping = {
173+
containerPort: 8080,
174+
name: 'test',
175+
};
176+
const networkmode = ecs.NetworkMode.AWS_VPC;
177+
const serviceConnect = new ecs.ServiceConnect(networkmode, portMapping);
178+
// THEN
179+
expect(() => {
180+
serviceConnect.validate();
181+
}).not.toThrow();
182+
});
183+
184+
});
185+
186+
});
187+
38188
test('port mapping throws an error when appProtocol is set without name', () => {
39189
// GIVEN
40190
const stack = new cdk.Stack();

0 commit comments

Comments
 (0)