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

Jump to bottom

(aws-ecs): PortMapping should support ContainerPortRange #23509

Closed
2 tasks
mneirynck opened this issue Dec 30, 2022 · 5 comments · Fixed by #26692
Closed
2 tasks

(aws-ecs): PortMapping should support ContainerPortRange #23509

mneirynck opened this issue Dec 30, 2022 · 5 comments · Fixed by #26692
Labels
@aws-cdk/aws-ecs Related to Amazon Elastic Container effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p1

Comments

@mneirynck
Copy link

Describe the feature

This feature would make CDK on par with Cloudformation for the places where PortMapping is used.

Use Case

We're working on a project that uses an RTPEngine hosted on ECS Fargate, but this needs a specified range of UDP ports to be opened (ie 30000:40000).

Proposed Solution

Add the ContainerPortRange property to PortMapping, Cloudformation already supports it.

Or give the option to do a property override to add this property yourself.

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.57.0

Environment details (OS name and version, etc.)

Arch, Python 3.10
@mneirynck mneirynck added feature-request A feature should be added or improved. needs-triage This issue or PR still needs to be triaged. labels Dec 30, 2022
@github-actions github-actions bot added the @aws-cdk/aws-ecs Related to Amazon Elastic Container label Dec 30, 2022
@peterwoodworth peterwoodworth added p1 effort/small Small work item – less than a day of effort and removed needs-triage This issue or PR still needs to be triaged. labels Dec 30, 2022
@bun913
Copy link
Contributor

bun913 commented Feb 13, 2023

Hello! Can I challenge this Issue?

I think this can be achieved by changing the following code and related parts

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

Lines 1115 to 1123 in 09f8e9f

function renderPortMapping(pm: PortMapping): CfnTaskDefinition.PortMappingProperty {
return {
containerPort: pm.containerPort,
hostPort: pm.hostPort,
protocol: pm.protocol || Protocol.TCP,
appProtocol: pm.appProtocol?.value,
name: pm.name ? pm.name : undefined,
};
}

@bun913 bun913 mentioned this issue Feb 14, 2023
Closed
2 tasks
@bun913
Copy link
Contributor

bun913 commented Feb 14, 2023

Sorry.
The problem here is not easy to change the existing system and it is not likely to be started for a while.

@peterwoodworth
Copy link
Contributor

Thanks for submitting the new issue @bun913, it's much appreciated and may help us deliver this particular feature faster

@TristanBarlow
Copy link

I would also like to see this feature implemented

@ste93cry ste93cry mentioned this issue Aug 9, 2023
Merged
@mergify mergify bot closed this as completed in #26692 Aug 22, 2023
mergify bot pushed a commit that referenced this issue Aug 22, 2023
@ste93cry
feat(ecs): container port ranges in port mappings (#26692)
4997bca 
Amazon ECS [supports](https://aws.amazon.com/it/about-aws/whats-new/2022/12/amazon-ecs-supports-container-port-ranges-port-mapping/) container port ranges for port mapping since quite a bit, however the `ContainerDefinition` L2 construct does not expose a way to set them. Right now, the only viable solution I found to use the feature is using Escape Hatches. Within this PR, the `containerPortRange` property is added to the `PortMapping` object and mapped back to the underlying L1 construct.

The current implementation contains a breaking change: since setting both a port range and a single fixed port doesn't make sense, I had to mark optional all the properties of `PortMapping`, hence the `containerPort` property changed its type from `non-nullable` to `nullable`. The downside of the proposed solution is that now all the properties are optional and the compiler doesn't complain if an empty object is passed as port mapping. I added some runtime checks to ensure that a valid object is created, but I didn't find a better way to do it at compile time that is also compatible with the transformation done by `jsii`.

Closes #23509.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@github-actions
Copy link

⚠️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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
@aws-cdk/aws-ecs Related to Amazon Elastic Container effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p1
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

feat(ecs): container port ranges in port mappings
4 participants
@TristanBarlow @mneirynck @peterwoodworth @bun913