Allocate a free host port from the range for a single container port

[s3onghyun]

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

TestParseFlagP covers the single-container-port pool case (binds to the first free port) and keeps the genuine-mismatch error case. go test ./pkg/portutil/ passes.

[haytok]

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]

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. -p 3000-3001:8080), nerdctl now treats the range as a pool and binds the container port to the first free host port in it, using the existing getUsedPorts to skip ports already in use — matching Docker's behavior. The previous silent collapse (dropping the rest of the range) is gone.

A genuine mismatch (both sides are ranges of unequal length, e.g. 3000-3001:8080-8082) is still rejected, since that has no valid mapping.

go test ./pkg/portutil/ passes, including a new case for the single-container-port pool allocation. Let me know if you'd prefer the allocation to scan from a different end of the range, or any other tweak.

[Copilot]

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:singleContainerPort as a pool and select the first free host port.
• Updates the range-length mismatch validation behavior and associated error path.
• Adds a TestParseFlagP case for 127.0.0.1:3000-3001:8080/tcp expecting 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.

[AkihiroSuda]

@s3onghyun Could you check the comments from copilot?

[s3onghyun]

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. 3000-3002:8080-8081) it doesn't enter the pool branch and errors as before. Only the single-container-port + host-range case (3000-3001:8080) is treated as a pool, matching Docker.

Comment 3 (environment-dependent "first free port" assertion): valid — fixed. I removed the table case that asserted an exact HostPort: 3000 and added TestParseFlagPHostRangePool, which asserts the result is a single mapping with the host port within the requested range [3000, 3001] rather than a specific value, so it no longer depends on which port happens to be free in the test environment.

go test ./pkg/portutil/ and go vet pass. The Copilot threads are stale against the updated description — happy to adjust further if you'd prefer different behavior for the single-port + host-range case.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

[AkihiroSuda]

@s3onghyun Could you check the comments from copilot?

[s3onghyun]

@AkihiroSuda went through the two current (non-outdated) Copilot comments:

portutil.go — empty bind IP in the error message: valid, fixed in the latest push. When no host IP is specified (e.g. -p 3000-3001:8080) the bind-failure errors printed an empty address (bind for failed: ...). I now compute an effective bindIP that falls back to 0.0.0.0 — matching the HostIP we actually set below — and use it in both the pool-path and the single-port error messages so they read consistently.

portutil_test.go:377 — asserting only that the port is within the range: this one I kept intentionally, and I think it is the safer assertion. Copilots suggestion (occupy the first port, assert the next one is chosen) reintroduces exactly the environment-dependence its own earlier comment warned about: it assumes a specific adjacent port is free in the test host, which is racy in CI. The implementation does pick the first free port (simple ascending scan, break on the first hit), but pinning the test to an exact port makes it flaky without adding real coverage — "result is a single mapping whose host port lies in the requested range" is the behavior contract that matters here. Happy to switch to a deterministic occupy-and-assert if you would prefer it, and can gate it behind a helper that reserves a known-free adjacent pair.

go test ./pkg/portutil/ and go vet ./pkg/portutil/ pass.

[haytok]

NIT: Overall, the comments here look a bit redundant, so it's better to clean them up for future readers.

[s3onghyun]

Thanks @haytok, addressed both:

1. IP normalization — moved the validation + normalization up to right after splitParts() into a single hostIP, and dropped the duplicated bindIP block and the redundant per-mapping normalization/validation. One caveat I kept on purpose: getUsedPorts/portAllocate still receive the raw ip. getUsedPorts only filters by IP when ip != "" (an empty IP means "all interfaces" there), so normalizing to 0.0.0.0 before that call would narrow the used-port lookup and change collision detection. So hostIP is used only for the error messages and the resulting PortMapping.

2. Test — rewrote TestParseFlagPHostRangePool to be deterministic: it occupies the first port of a two-port range and asserts the container port binds to the free successor. It now fails without the pool fix (verified locally on Linux). Guarded to Linux since getUsedPorts is Linux-only, and skipped in rootless.

[AkihiroSuda]

Please squash the commits

[s3onghyun]

Squashed into a single commit.

[s3onghyun]

Done — updated the doc comment to describe the deterministic logic (occupies the first port of the range and asserts the container port binds to first+1), matching the rewritten test. Kept it as a single squashed commit. Thanks @haytok.

[haytok]

@s3onghyun It looks like the comments haven't been updated.

// port chosen depends on what is free in the environment, so this asserts membership
// in the range rather than a specific port.

[s3onghyun]

Apologies @haytok — my previous amend didn't actually stage the file, so the force-push pushed an identical tree and the comment never changed. Fixed for real this time (43e1974): the doc comment now describes the deterministic logic — it occupies the first port of the range and asserts the container port binds to first+1. Verified the updated text is on the PR head. Still a single squashed commit. Thanks for catching it twice.

[haytok]

LGTM

[s3onghyun]

Thanks @AkihiroSuda — addressed all four in d5361fd2:

• runtime.GOOS != "linux" guard: removed. Moved the test into a new pkg/portutil/portutil_linux_test.go so the Linux constraint is enforced by the filename instead (the other tests in portutil_test.go are cross-platform, so I split just this one out rather than renaming the whole file).
• rootlessutil.IsRootless() skip: removed — you're right, this unit test doesn't run RootlessKit+containerd+nerdctl, so the check was unnecessary.
• could not find an occupied port → t.Skip: changed to t.Fatal. Binding an ephemeral port with a free successor should always succeed within 50 attempts; if it genuinely can't, that's a broken environment worth failing loudly on.
• "Was this comment resolved?": yes — the stale doc comment @haytok flagged was fixed earlier in this same commit; it now describes the deterministic logic (occupy the first port, assert the container port binds to first+1).

Still a single squashed commit. gofmt/go vet (incl. GOOS=linux) clean.

[AkihiroSuda]

Please click the Resolve conversation button for resolved comments

[s3onghyun]

Done — resolved all the review conversations (yours, @haytok's, and the Copilot ones). For the Copilot thread about 3000-3001:8080 being a "mismatch": I left a short note explaining it's intentional — that's the pool case this PR adds, not an error.

On CI: the only red job is test / in-host / docker failing on TestAttachForAutoRemovedContainer, which is an unrelated run --rm+attach integration flake (nothing to do with port allocation). The unit tests, including the new portutil_linux_test.go, pass on all three platforms. Happy to re-run if you'd like.

[AkihiroSuda]

Thanks