Allocate a free host port from the range for a single container port#4988
Allocate a free host port from the range for a single container port#4988s3onghyun wants to merge 1 commit into
Conversation
haytok
left a comment
haytok
left a comment
There was a problem hiding this comment.
Thanks for creating this PR!
When the container port is a single port and the host port is specified as a range, Moby uses that range as a pool and dynamically allocates a free host port from it.
- https://github.com/docker/cli/blob/master/cli/command/container/opts.go#L432
- https://github.com/docker/go-connections/blob/main/nat/nat.go#L208C3-L215C4
- https://github.com/moby/moby/blob/master/daemon/libnetwork/portallocator/portallocator.go#L245C2-L281C32
Details of Docker's behavior:
Details
> docker ps
CONTAINER ID IMAGE COMMAND CREATED STATUS PORTS NAMES
> docker run -d -p 127.0.0.1:3000-3001:8080/tcp nginx
c1b08c490d3c80f9614c4dce6259f2699020c0c83c75eb7964913cbace9328eb
> docker ps
CONTAINER ID IMAGE COMMAND CREATED STATUS PORTS NAMES
c1b08c490d3c nginx "/docker-entrypoint.…" 4 seconds ago Up 4 seconds 80/tcp, 127.0.0.1:3000->8080/tcp nervous_feynman
> docker run -d -p 127.0.0.1:3000-3001:8080/tcp nginx
f927b67be40779c03ee7c4dd4fb15ba74de2aece81683ec7cd5c91311097423d
> docker ps
CONTAINER ID IMAGE COMMAND CREATED STATUS PORTS NAMES
f927b67be407 nginx "/docker-entrypoint.…" 1 second ago Up 1 second 80/tcp, 127.0.0.1:3001->8080/tcp thirsty_yalow
c1b08c490d3c nginx "/docker-entrypoint.…" 10 seconds ago Up 10 seconds 80/tcp, 127.0.0.1:3000->8080/tcp nervous_feynman
nerdctl does not implement dynamic allocation within an explicit host range
As you know, nerdctl currently does not allocate a free host port from the range in this case, so implementing that could be one option.
Therefore, IMO there might be a better approach than this PR.
WDYT?
s3onghyun
force-pushed
the
fix-port-range-mismatch
branch
from
ec1ab4c to
40d493f
Compare
|
Great point, @haytok — thank you for the references and the clear Docker walkthrough. You're right that the better fix is to implement the pool behavior rather than reject the spec. I've reworked the PR to do exactly that. When the container side is a single port and the host side is a range (e.g. A genuine mismatch (both sides are ranges of unequal length, e.g.
|
There was a problem hiding this comment.
Pull request overview
This PR modifies pkg/portutil.ParseFlagP to change how it handles port specs where the host side is a range but the container side is a single port (e.g. 3000-3001:8080). The stated goal in the PR description is to reject host/container range-length mismatches to avoid silently dropping ports, but the current diff instead implements Docker-style “range as a pool” behavior and updates the unit test to expect success.
Changes:
- Adds logic to treat
hostRange:singleContainerPortas a pool and select the first free host port. - Updates the range-length mismatch validation behavior and associated error path.
- Adds a
TestParseFlagPcase for127.0.0.1:3000-3001:8080/tcpexpecting a successful mapping.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| pkg/portutil/portutil.go | Adds host-range-as-pool selection logic and adjusts mismatch validation. |
| pkg/portutil/portutil_test.go | Adds a new ParseFlagP test case for host range with single container port. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if startPort == endPort && startHostPort != endHostPort { | ||
| // Docker-compatible behavior: a single container port with a host port | ||
| // range (e.g. "3000-3001:8080") treats the range as a pool and binds the | ||
| // container port to the first free host port in it, rather than silently | ||
| // collapsing to the first port and dropping the rest of the range. | ||
| // https://github.com/moby/moby/blob/master/daemon/libnetwork/portallocator/portallocator.go | ||
| found := false | ||
| for p := startHostPort; p <= endHostPort; p++ { | ||
| if !usedPorts[p] { | ||
| startHostPort, endHostPort = p, p | ||
| found = true | ||
| break | ||
| } | ||
| } | ||
| if !found { | ||
| return nil, fmt.Errorf("bind for %s failed: all ports in range %s are already allocated", ip, hostPort) | ||
| } | ||
| } else { | ||
| for i := startHostPort; i <= endHostPort; i++ { | ||
| if usedPorts[i] { | ||
| return nil, fmt.Errorf("bind for %s:%d failed: port is already allocated", ip, i) | ||
| } | ||
| } | ||
| } |
| if hostPort != "" && (endPort-startPort) != (endHostPort-startHostPort) { | ||
| if endPort != startPort { | ||
| return nil, fmt.Errorf("invalid ranges specified for container and host Ports: %s and %s", containerPort, hostPort) | ||
| } | ||
| // Both container and host sides are ranges but of unequal length — a genuine | ||
| // mismatch (the single-container-port pool case above has already collapsed | ||
| // the host range to one port, so it does not reach here). | ||
| return nil, fmt.Errorf("invalid ranges specified for container and host Ports: %s and %s", containerPort, hostPort) |
| { | ||
| // Docker-compatible: a single container port with a host port range binds | ||
| // to the first free host port in the range (here 3000, as nothing is used | ||
| // in the test environment), instead of dropping the rest of the range. | ||
| name: "host port range with single container port", | ||
| args: args{ | ||
| s: "127.0.0.1:3000-3001:8080/tcp", | ||
| }, | ||
| want: []cni.PortMapping{ | ||
| { | ||
| HostPort: 3000, | ||
| ContainerPort: 8080, | ||
| Protocol: "tcp", | ||
| HostIP: "127.0.0.1", | ||
| }, | ||
| }, | ||
| wantErrMsg: "", | ||
| }, |
s3onghyun
changed the title
|
@s3onghyun Could you check the comments from copilot? |
Docker treats `-p 3000-3001:8080` as a host-port pool: it binds the container port to a free host port from the range, rather than dropping the rest. nerdctl previously collapsed this to the first host port, silently discarding the rest of the range. Implement the Docker-compatible behavior: when the container side is a single port and the host side is a range, pick the first free port in that range (via the existing getUsedPorts) and map the container port to it. A genuine mismatch (both sides are ranges of unequal length) is still rejected. Signed-off-by: Seonghyun Hong <s3onghyun.hong@gmail.com>
s3onghyun
force-pushed
the
fix-port-range-mismatch
branch
from
40d493f to
2f7e70f
Compare
|
Thanks @AkihiroSuda — went through all three Copilot comments: Comments 1 & 2 (mismatch "contradicts the PR intent"): these were written against the original PR description, which said to reject the spec. After @haytok's review I reworked it from reject → Docker-style pool allocation and updated the title/description to match, so that contradiction no longer applies. The logic still rejects a genuine mismatch — when both sides are ranges of unequal length (e.g. Comment 3 (environment-dependent "first free port" assertion): valid — fixed. I removed the table case that asserted an exact
|
| if !found { | ||
| return nil, fmt.Errorf("bind for %s failed: all ports in range %s are already allocated", ip, hostPort) | ||
| } |
| // TestParseFlagPHostRangePool verifies the Docker-compatible behavior for a single | ||
| // container port with a host port range (e.g. "3000-3001:8080"): the container port | ||
| // is bound to one free host port from the range, not collapsed-and-dropped. The exact | ||
| // port chosen depends on what is free in the environment, so this asserts membership | ||
| // in the range rather than a specific port. | ||
| func TestParseFlagPHostRangePool(t *testing.T) { | ||
| got, err := ParseFlagP("127.0.0.1:3000-3001:8080/tcp") | ||
| assert.NilError(t, err) | ||
| assert.Equal(t, len(got), 1) | ||
| assert.Equal(t, got[0].ContainerPort, int32(8080)) | ||
| assert.Equal(t, got[0].Protocol, "tcp") | ||
| assert.Equal(t, got[0].HostIP, "127.0.0.1") | ||
| if got[0].HostPort < 3000 || got[0].HostPort > 3001 { | ||
| t.Errorf("host port %d is outside the requested range 3000-3001", got[0].HostPort) | ||
| } |
What
When the container side is a single port and the host side is a range (e.g.
-p 3000-3001:8080), Docker treats the range as a pool and binds the container port to a free host port from it. nerdctl previously collapsed this to the first host port, silently dropping the rest of the range.This PR implements the Docker-compatible behavior: pick the first free port in the host range (via the existing
getUsedPorts) and map the container port to it.Note
This supersedes the original approach in this PR (which rejected the spec). Per @haytok's review, implementing Docker's pool allocation is the better fix, so the PR was reworked accordingly. A genuine mismatch — both sides are ranges of unequal length, e.g.
3000-3001:8080-8082— is still rejected, since it has no valid mapping.Test
TestParseFlagPcovers the single-container-port pool case (binds to the first free port) and keeps the genuine-mismatch error case.go test ./pkg/portutil/passes.