(ecs): small refactor for PortMapping ease of change

[bun913]

Describe the feature

Sorry if I have the wrong category for the Issue or the wrong place for the discussion!

While looking at Issue #23509, I was surprised by the following addPortMappings conditional branch.

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

Lines 567 to 607 in de097ee

	public addPortMappings(...portMappings: PortMapping[]) {
	this.portMappings.push(...portMappings.map(pm => {
	if (this.taskDefinition.networkMode === NetworkMode.AWS_VPC || this.taskDefinition.networkMode === NetworkMode.HOST) {
	if (pm.containerPort !== pm.hostPort && pm.hostPort !== undefined) {
	throw new Error(`Host port (${pm.hostPort}) must be left out or equal to container port ${pm.containerPort} for network mode ${this.taskDefinition.networkMode}`);
	}
	}
	// No empty strings as port mapping names.
	if (pm.name === '') {
	throw new Error('Port mapping name cannot be an empty string.');
	}
	// Service connect logic.
	if (pm.name || pm.appProtocol) {
	// Service connect only supports Awsvpc and Bridge network modes.
	if (![NetworkMode.BRIDGE, NetworkMode.AWS_VPC].includes(this.taskDefinition.networkMode)) {
	throw new Error(`Service connect related port mapping fields 'name' and 'appProtocol' are not supported for network mode ${this.taskDefinition.networkMode}`);
	}
	// Name is not set but App Protocol is; this config is meaningless and we should throw.
	if (!pm.name) {
	throw new Error('Service connect-related port mapping field \'appProtocol\' cannot be set without \'name\'');
	}
	if (this._namedPorts.has(pm.name)) {
	throw new Error(`Port mapping name '${pm.name}' already exists on this container`);
	}
	this._namedPorts.set(pm.name, pm);
	}
	if (this.taskDefinition.networkMode === NetworkMode.BRIDGE) {
	if (pm.hostPort === undefined) {
	pm = {
	...pm,
	hostPort: 0,
	};
	}
	}
	return pm;
	}));
	}

The ContainerDefinition class is responsible for many of the responsibilities, and many if statements are added to verify PortMapping's sanity.

I feel that by writing down if statements like this, we lose the ease of modification.

Therefore, I would like to refactor it so that many contributors can easily modify it here in a way that does not affect the existing system!

Use Case

ECS is a very widely used service, and I believe it is a service that will continue to see many bug fixes and feature additions.

I think that every time we add more properties in the future, we will make it harder to make changes by making miscellaneous changes!

I want to make it easy for many developers to contribute to this project!

Proposed Solution

I think ContainerDefinition has too many responsibility.

An if statement is written in addPortMappings that assumes all contexts.

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

Lines 567 to 607 in de097ee

	public addPortMappings(...portMappings: PortMapping[]) {
	this.portMappings.push(...portMappings.map(pm => {
	if (this.taskDefinition.networkMode === NetworkMode.AWS_VPC || this.taskDefinition.networkMode === NetworkMode.HOST) {
	if (pm.containerPort !== pm.hostPort && pm.hostPort !== undefined) {
	throw new Error(`Host port (${pm.hostPort}) must be left out or equal to container port ${pm.containerPort} for network mode ${this.taskDefinition.networkMode}`);
	}
	}
	// No empty strings as port mapping names.
	if (pm.name === '') {
	throw new Error('Port mapping name cannot be an empty string.');
	}
	// Service connect logic.
	if (pm.name || pm.appProtocol) {
	// Service connect only supports Awsvpc and Bridge network modes.
	if (![NetworkMode.BRIDGE, NetworkMode.AWS_VPC].includes(this.taskDefinition.networkMode)) {
	throw new Error(`Service connect related port mapping fields 'name' and 'appProtocol' are not supported for network mode ${this.taskDefinition.networkMode}`);
	}
	// Name is not set but App Protocol is; this config is meaningless and we should throw.
	if (!pm.name) {
	throw new Error('Service connect-related port mapping field \'appProtocol\' cannot be set without \'name\'');
	}
	if (this._namedPorts.has(pm.name)) {
	throw new Error(`Port mapping name '${pm.name}' already exists on this container`);
	}
	this._namedPorts.set(pm.name, pm);
	}
	if (this.taskDefinition.networkMode === NetworkMode.BRIDGE) {
	if (pm.hostPort === undefined) {
	pm = {
	...pm,
	hostPort: 0,
	};
	}
	}
	return pm;
	}));
	}

For example, this nested if statement is also commented as Sevice Connect logick, but now I don't know what the decision logic is for.

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

Lines 578 to 606 in de097ee

	// Service connect logic.
	if (pm.name || pm.appProtocol) {
	// Service connect only supports Awsvpc and Bridge network modes.
	if (![NetworkMode.BRIDGE, NetworkMode.AWS_VPC].includes(this.taskDefinition.networkMode)) {
	throw new Error(`Service connect related port mapping fields 'name' and 'appProtocol' are not supported for network mode ${this.taskDefinition.networkMode}`);
	}
	// Name is not set but App Protocol is; this config is meaningless and we should throw.
	if (!pm.name) {
	throw new Error('Service connect-related port mapping field \'appProtocol\' cannot be set without \'name\'');
	}
	if (this._namedPorts.has(pm.name)) {
	throw new Error(`Port mapping name '${pm.name}' already exists on this container`);
	}
	this._namedPorts.set(pm.name, pm);
	}
	if (this.taskDefinition.networkMode === NetworkMode.BRIDGE) {
	if (pm.hostPort === undefined) {
	pm = {
	...pm,
	hostPort: 0,
	};
	}
	}
	return pm;
	}));

So, I suggest creating a PortMap or similarly named class (I think it would be a Value Object) like below.

export class PortMap {
  readonly portmap: PortMapping;
  readonly namedPorts: Map<string, PortMapping>;
  readonly networkMode: NetworkMode;

  constructor(pm: PortMapping, np: Map<string, PortMapping>, nwm: NetworkMode) {
    this.portmap = pm;
    this.namedPorts = np;
    this.networkMode = nwm;
    this.hasRequiredProp();
    this.canServiceConnect();
  }

  private hasRequiredProp() {
    if ( this.portmap.name == '') {
      throw new Error('Port mapping name cannot be an empty string.');
    }
  }
  private canServiceConnect() {
    // validation
  }
  private addNamedPort() {
    // return new NamedPort(Don'n change curret taskdef propertiy)
  }
}

I thought it would make for a more prospective code by taking the responsibility for checking the sanity of PortMapping related properties out of the picture!

Other Information

No response

Acknowledgements

• I may be able to implement this feature request
• This feature might incur a breaking change

CDK version used

2.64.0

Environment details (OS name and version, etc.)

macOS Monterey

[peterwoodworth]

Thanks for providing this feedback and your suggestions on how to improve this code after your previous experience of attempting to change it. @madeline-k you have some good context around this library, what do you think about this refactoring suggestion?

[github-actions]

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.