feat: bind to 127.0.0.1 by default instead of 0.0.0.0#2812
Conversation
Port bindings now default to localhost-only, preventing prediction endpoints from being exposed to the entire network during development. - Add HostIP field to command.Port struct (defaults to 127.0.0.1) - Add --host flag to cog serve (default 127.0.0.1, use 0.0.0.0 to expose) - Support host:port syntax in cog run -p (e.g. -p 0.0.0.0:8888) - Bind cog predict/train to 127.0.0.1 - Update GetHostPortForContainer to match configured host IP
|
LGTM |
|
@anish-sahoo Bonk workflow was cancelled. View workflow run · To retry, trigger Bonk again. |
|
Changes default port binding from Issues found:
|
|
I'm Bonk, and I've done a quick review of your PR. Summary: Changes default Docker port binding from Issues (highest severity first):
I posted the full review as a top-level PR comment because the batch review API returned a 422 (likely stale line numbers). You can view it here: #2812 (comment) |
michaeldwan
left a comment
michaeldwan
left a comment
There was a problem hiding this comment.
Review
Real security improvement — localhost-by-default is the right call, applied centrally at the bind site, with docs regenerated. One blocker and a few should-fix items below.
Blocker
cog exec -p lost its host:port override in the merge. Commit a4aec985 added host:port parsing (-p 0.0.0.0:8000) and a HostIP to pkg/cli/run.go. Main renamed cog run → cog exec (ba48ff8e); during the merge (88bde6bd) git couldn't carry those changes into the renamed exec.go, so the current loop is the old strconv.Atoi(portString) with no HostIP and no host:port syntax. Because docker.go now defaults empty HostIP to 127.0.0.1, cog exec -p 8888 jupyter notebook silently became localhost-only with no escape hatch — contradicting the commit message's stated intent ("Support host:port syntax in cog run -p"). exec.go isn't in this diff, so this can't be an inline comment — please restore the host:port parsing from the original run.go diff and update the --publish help text.
Should-fix (inline below)
docker.go— the core HostIP propagation change has no unit test.run.go— exact-stringHostIPmatch is brittle; fall back to the single binding when only one exists.command.go— extract a sharedDefaultHostIPconstant ("127.0.0.1" is duplicated in 5 sites).serve.go— remote-Docker (DOCKER_HOST=tcp://…) users get silent breakage; no warning.
Nits (inline below)
serve.go:20— package var namedhostis generic;serveHost/bindHostis clearer.serve.go:174— IPv6--hostvalues produce malformed display URLs (missing brackets);displayHostlogic is also untested.run_test.go— duplicated mock scaffolding, not table-driven; missing edge cases (custom IP, IPv6, inspect returning emptyHostIP).
cog predict/cog train hardcoding 127.0.0.1 is intended (local-only) and correct. pkg/registry_testhelpers using 0.0.0.0 directly via nat.PortBinding is correctly unaffected.
| ) | ||
|
|
||
| var ( | ||
| host = "127.0.0.1" |
There was a problem hiding this comment.
Nit — generic var name. Package-level host collides with a common local (docker.go:47 uses host, err := determineDockerHost()). serveHost or bindHost would be clearer and lower the shadowing risk. Consistent with the existing port/uploadURL convention, so low priority.
There was a problem hiding this comment.
Done in f0f1a08 — renamed the package-level var to serveHost (flag and all references updated).
| } | ||
|
|
||
| runOptions.Ports = append(runOptions.Ports, command.Port{HostPort: port, ContainerPort: 5000}) | ||
| runOptions.Ports = append(runOptions.Ports, command.Port{HostPort: port, ContainerPort: 5000, HostIP: host}) |
There was a problem hiding this comment.
Should-fix — remote-Docker regression. When DOCKER_HOST=tcp://remote:2375, this binding is applied on the remote daemon, so 127.0.0.1 there is unreachable from the cog client. --host 0.0.0.0 is the escape hatch but nothing warns the user. (cog predict was already broken for remote Docker — it connects to localhost — so this is a new issue only for serve.) Consider detecting a non-local DOCKER_HOST and emitting a console warning pointing at --host 0.0.0.0, or at least noting it in the --host help text.
There was a problem hiding this comment.
Done in f0f1a08 — added docker.IsRemoteDockerHost() and cog serve now emits a console.Warnf when the daemon is reached over a non-local DOCKER_HOST (anything but unix/npipe), pointing out the bind lands on the remote host. Left cog predict's localhost assumption alone as pre-existing/out of scope.
| runOptions.Ports = append(runOptions.Ports, command.Port{HostPort: port, ContainerPort: 5000, HostIP: host}) | ||
|
|
||
| displayHost := host | ||
| if displayHost == "0.0.0.0" { |
There was a problem hiding this comment.
Nit — IPv6 display URL. displayHost only special-cases 0.0.0.0. An IPv6 value like ::1 interpolates to http://::1:8393 (missing brackets — invalid URI). Consider bracketing IPv6 literals or validating --host.
Also: this displayHost mapping and the --host defaulting are untested — extracting func displayHostFor(host string) string and table-testing it would cover both.
There was a problem hiding this comment.
Done in f0f1a08 — extracted displayHostForServe and formatServeURL, which build the URL with net.JoinHostPort so IPv6 literals get bracketed (e.g. http://[fe80::1]:8393). Both are table-tested (TestDisplayHostForServe, TestFormatServeURL). As a bonus, --host 0.0.0.0 now prints the navigable http://localhost:PORT alongside the 0.0.0.0 URL.
| type Port struct { | ||
| HostPort int | ||
| ContainerPort int | ||
| HostIP string // Host IP to bind to. Defaults to "127.0.0.1" if empty. |
There was a problem hiding this comment.
Should-fix — extract a shared constant. "127.0.0.1" is duplicated as the default in 5 sites: this comment, serve.go:20, docker.go:457, run.go:41, and predictor.go:101. A single const DefaultHostIP = "127.0.0.1" here, referenced everywhere, removes the magic-string duplication and the drift risk.
There was a problem hiding this comment.
Done in f0f1a08 — added const DefaultHostIP = "127.0.0.1" in command.go and referenced it from serve.go, docker.go, run.go, and predictor.go, removing the duplicated magic string.
| hostCfg.PortBindings[containerPort] = []nat.PortBinding{ | ||
| { | ||
| HostIP: "", // use empty string to bind to all interfaces | ||
| HostIP: hostIP, |
There was a problem hiding this comment.
Should-fix — untested core change. The empty→127.0.0.1 default and this HostIP propagation are the central behavior change of the PR, but containerRun has no unit test. Suggest extracting the PortBindings construction (the loop over options.Ports) into a pure helper like portBindingsFromPorts([]command.Port) nat.PortMap and table-testing it: empty HostIP → 127.0.0.1, explicit 0.0.0.0, explicit 127.0.0.1, custom IP, multiple ports.
There was a problem hiding this comment.
Done in f0f1a08 — extracted portBindingsFromRunOptions (and exposedPortsFromRunOptions) out of containerRun and table-tested both in pkg/docker/ports_test.go: empty HostIP → 127.0.0.1, explicit 0.0.0.0, custom IP, IPv6, and multiple ports.
| for _, portBinding := range inspect.NetworkSettings.Ports[targetPort] { | ||
| // TODO[md]: this should not be hardcoded since docker may be bound to a different address | ||
| if portBinding.HostIP != "0.0.0.0" { | ||
| if portBinding.HostIP != hostIP { |
There was a problem hiding this comment.
Should-fix — brittle exact-string match. portBinding.HostIP != hostIP is stricter than the old != "0.0.0.0" filter (which matched in either direction). Docker runtimes (Docker Desktop VM proxy, rootless, future versions) may normalize the reported IP, causing a silent does not have a port bound to 127.0.0.1 failure on a container that did bind that port. Suggestion: when exactly one PortBinding exists for the target port, return it regardless of HostIP; only filter by hostIP when multiple are present.
There was a problem hiding this comment.
Done in f0f1a08 — GetHostPortForContainer now falls back to the sole binding for the target port when only one exists, so a normalized/wildcard IP no longer trips a false does not have a port bound error. Scoped the fallback to empty/0.0.0.0 HostIP so a genuinely mismatched specific IP still errors; covered by new table rows.
| }, | ||
| }, nil) | ||
|
|
||
| hostPort, err := GetHostPortForContainer(t.Context(), testClient, "container123", 5678, "127.0.0.1") |
There was a problem hiding this comment.
Nit — table-driven + missing edge cases. Each of the 8 subtests repeats the full InspectResponse boilerplate; a single table with hostIP arg, mocked Ports, and expected (hostPort, errContains) would halve the file (AGENTS.md prefers table-driven). Missing cases: a custom (non-127/0.0.0.0) hostIP, IPv6, and Docker inspect returning an empty HostIP (would currently error — worth a row to document that).
There was a problem hiding this comment.
Done in f0f1a08 — rewrote TestGetHostPortForContainer as a single table with shared InspectResponse builders, and added the missing rows: custom non-loopback IP, IPv6 (::1), and empty-HostIP (now documents the single-binding fallback behavior).
- Restore host:port parsing for `cog exec -p` lost in the cog run→exec rename; support plain ports, IPv4, bare and bracketed IPv6, with port range and empty-host validation - Extract shared command.DefaultHostIP constant (was duplicated) - Narrow GetHostPortForContainer fallback to single wildcard/empty bindings - Warn when serving via a remote Docker daemon; show a navigable localhost URL alongside 0.0.0.0; bracket IPv6 display hosts - Rename generic serve `host` var to `serveHost` - Add unit tests for port binding helpers, host parsing, remote-host detection, and serve URL formatting; make run_test table-driven - Regenerate docs/cli.md and docs/llms.txt
|
LGTM |
|
LGTM |
4 participants
Summary
0.0.0.0to127.0.0.1across all commands, preventing prediction endpoints from being accidentally exposed to the entire network during development.--hostflag tocog serveandhost:portsyntax tocog run -pso users can opt into binding to all interfaces when needed.Changes
pkg/docker/command/command.goHostIPfield toPortstructpkg/docker/docker.goport.HostIP(default127.0.0.1) instead of hardcoded""pkg/docker/run.goGetHostPortForContainernow accepts ahostIPparameterpkg/docker/run_test.gopkg/cli/serve.go--hostflag (default127.0.0.1)pkg/cli/run.go-pflag supportshost:portsyntax (e.g.-p 0.0.0.0:8888), defaults to127.0.0.1pkg/predict/predictor.gocog predict/cog trainbind to127.0.0.1User-facing behavior
cog serve0.0.0.0127.0.0.1. Use--host 0.0.0.0to expose.cog run -p 88880.0.0.0:8888127.0.0.1:8888. Use-p 0.0.0.0:8888to expose.cog predict/cog train0.0.0.0127.0.0.1Test plan