Skip to content

Allow container port binding for privileged ports#7719

Merged
dfangl merged 2 commits intomasterfrom
container-port-binding
Feb 20, 2023
Merged

Allow container port binding for privileged ports#7719
dfangl merged 2 commits intomasterfrom
container-port-binding

Conversation

@dfangl
Copy link
Member

@dfangl dfangl commented Feb 20, 2023

Motivation

Currently, we do not allow checking privileged ports to be bound by LS. This leads to a problem with a hostport configuration in ECS for privileged ports - even if they are free.

Also, we are checking for each port we check on the host, if the port is free in the container as well. We should not do this, as we might want to use ports bound inside LS on the host as well.

Changes

  • Introduce separate port ranges for ports returned by reserve_available_container_port and ports which can be reserved on request. Per default, the former only returns non-privileged ports, while the whole port range can be reserved on explicit request for a port
  • Add additional option to override the port range for reserving available ports. This can be used to further limit the random port range, for example for ECS: https://docs.aws.amazon.com/AmazonECS/latest/APIReference/API_PortMapping.html (The default ephemeral port range for Docker version 1.6.0 and later is listed on the instance under /proc/sys/net/ipv4/ip_local_port_range. If this kernel parameter is unavailable, the default ephemeral port range from 49153 through 65535 is used.)

Remaining Problems

If checking is_port_available_for_containers before reserving the port, with the new changes, the port check will be done twice, which can lead to reduced performance.
Where possible, we should try reserving and catching the exception, instead of checking if it is possible first.

@dfangl dfangl requested a review from whummer February 20, 2023 13:58
@dfangl dfangl temporarily deployed to localstack-ext-tests February 20, 2023 13:58 — with GitHub Actions Inactive
Copy link
Member

@whummer whummer left a comment

Choose a reason for hiding this comment

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

Kudos for tackling this @dfangl ! 🚀 As we also briefly discussed, it makes perfect sense to include privileged ports into the container range as well. Nice and clean implementation with the additional _DockerPortRange class! 👍

@coveralls
Copy link

Coverage Status

Coverage: 85.01% (-0.006%) from 85.016% when pulling 4ceeb18 on container-port-binding into 4567080 on master.

@github-actions
Copy link

LocalStack integration with Pro

       3 files  ±0         3 suites  ±0   1h 35m 15s ⏱️ + 3m 38s
1 731 tests ±0  1 382 ✔️ ±0  349 💤 ±0  0 ±0 
2 449 runs  ±0  1 758 ✔️ ±0  691 💤 ±0  0 ±0 

Results for commit 4ceeb18. ± Comparison against base commit 4567080.

@dfangl dfangl merged commit 133eda2 into master Feb 20, 2023
@dfangl dfangl deleted the container-port-binding branch February 20, 2023 16:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants