-
Notifications
You must be signed in to change notification settings - Fork 691
feat: bind to 127.0.0.1 by default instead of 0.0.0.0 #2812
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,6 +17,7 @@ import ( | |
| ) | ||
|
|
||
| var ( | ||
| host = "127.0.0.1" | ||
| port = 8393 | ||
| uploadURL = "" | ||
| ) | ||
|
|
@@ -28,13 +29,21 @@ func newServeCommand() *cobra.Command { | |
| Long: `Run an HTTP server. | ||
|
|
||
| Builds the model and starts an HTTP server that exposes the model's inputs | ||
| and outputs as a REST API. Compatible with the Cog HTTP protocol.`, | ||
| and outputs as a REST API. Compatible with the Cog HTTP protocol. | ||
|
|
||
| By default the container port is published on 127.0.0.1 (localhost), so the | ||
| server is only reachable from your local machine. The server process inside | ||
| the container binds to 0.0.0.0; use --host to control which host interface | ||
| the Docker port mapping is published on.`, | ||
| Example: ` # Start the server on the default port (8393) | ||
| cog serve | ||
|
|
||
| # Start on a custom port | ||
| cog serve -p 5000 | ||
|
|
||
| # Listen on all interfaces (e.g. to expose to the network) | ||
| cog serve --host 0.0.0.0 | ||
|
|
||
| # Test the server | ||
| curl http://localhost:8393/predictions \ | ||
| -X POST \ | ||
|
|
@@ -51,6 +60,7 @@ and outputs as a REST API. Compatible with the Cog HTTP protocol.`, | |
| addGpusFlag(cmd) | ||
| addConfigFlag(cmd) | ||
|
|
||
| cmd.Flags().StringVar(&host, "host", host, "Host IP to publish the container port on. Use 0.0.0.0 to allow connections from other machines.") | ||
| cmd.Flags().IntVarP(&port, "port", "p", port, "Port on which to listen") | ||
| cmd.Flags().StringVar(&uploadURL, "upload-url", "", "Upload URL for file outputs (e.g. https://example.com/upload/)") | ||
|
|
||
|
|
@@ -158,12 +168,17 @@ func cmdServe(cmd *cobra.Command, arg []string) error { | |
| runOptions.ExtraHosts = []string{"host.docker.internal:host-gateway"} | ||
| } | ||
|
|
||
| runOptions.Ports = append(runOptions.Ports, command.Port{HostPort: port, ContainerPort: 5000}) | ||
| runOptions.Ports = append(runOptions.Ports, command.Port{HostPort: port, ContainerPort: 5000, HostIP: host}) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should-fix — remote-Docker regression. When |
||
|
|
||
| displayHost := host | ||
| if displayHost == "0.0.0.0" { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit — IPv6 display URL. Also: this |
||
| displayHost = "localhost" | ||
| } | ||
|
|
||
| console.Info("") | ||
| console.Infof("Running %[1]s in Docker with the current directory mounted as a volume...", console.Bold(strings.Join(args, " "))) | ||
| console.Info("") | ||
| console.Infof("Serving at %s", console.Bold(fmt.Sprintf("http://127.0.0.1:%v", port))) | ||
| console.Infof("Serving at %s", console.Bold(fmt.Sprintf("http://%s:%v", displayHost, port))) | ||
| console.Info("") | ||
|
|
||
| err = docker.Run(ctx, dockerClient, runOptions) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -94,6 +94,7 @@ type RunOptions struct { | |
| type Port struct { | ||
| HostPort int | ||
| ContainerPort int | ||
| HostIP string // Host IP to bind to. Defaults to "127.0.0.1" if empty. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should-fix — extract a shared constant. |
||
| } | ||
|
|
||
| type Volume struct { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -452,10 +452,14 @@ func (c *apiClient) containerRun(ctx context.Context, options command.RunOptions | |
| if len(options.Ports) > 0 { | ||
| hostCfg.PortBindings = make(nat.PortMap) | ||
| for _, port := range options.Ports { | ||
| hostIP := port.HostIP | ||
| if hostIP == "" { | ||
| hostIP = "127.0.0.1" | ||
| } | ||
| containerPort := nat.Port(fmt.Sprintf("%d/tcp", port.ContainerPort)) | ||
| hostCfg.PortBindings[containerPort] = []nat.PortBinding{ | ||
| { | ||
| HostIP: "", // use empty string to bind to all interfaces | ||
| HostIP: hostIP, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should-fix — untested core change. The empty→ |
||
| HostPort: strconv.Itoa(port.HostPort), | ||
| }, | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -34,9 +34,13 @@ func RunDaemon(ctx context.Context, dockerClient command.Command, options comman | |
| return dockerClient.ContainerStart(ctx, options) | ||
| } | ||
|
|
||
| func GetHostPortForContainer(ctx context.Context, dockerCommand command.Command, containerID string, containerPort int) (int, error) { | ||
| func GetHostPortForContainer(ctx context.Context, dockerCommand command.Command, containerID string, containerPort int, hostIP string) (int, error) { | ||
| console.Debugf("=== DockerCommand.GetPort %s/%d", containerID, containerPort) | ||
|
|
||
| if hostIP == "" { | ||
| hostIP = "127.0.0.1" | ||
| } | ||
|
|
||
| inspect, err := dockerCommand.ContainerInspect(ctx, containerID) | ||
| if err != nil { | ||
| return 0, fmt.Errorf("failed to inspect container %q: %w", containerID, err) | ||
|
|
@@ -56,8 +60,7 @@ func GetHostPortForContainer(ctx context.Context, dockerCommand command.Command, | |
| } | ||
|
|
||
| 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 { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should-fix — brittle exact-string match. |
||
| continue | ||
| } | ||
| hostPort, err := nat.ParsePort(portBinding.HostPort) | ||
|
|
@@ -67,5 +70,5 @@ func GetHostPortForContainer(ctx context.Context, dockerCommand command.Command, | |
| return hostPort, nil | ||
| } | ||
|
|
||
| return 0, fmt.Errorf("container %s does not have a port bound to 0.0.0.0", containerID) | ||
| return 0, fmt.Errorf("container %s does not have a port bound to %s", containerID, hostIP) | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,6 +13,63 @@ import ( | |
|
|
||
| func TestGetHostPortForContainer(t *testing.T) { | ||
| t.Run("WithExposedPort", func(t *testing.T) { | ||
| testClient := dockertest.NewMockCommand2(t) | ||
| testClient.EXPECT().ContainerInspect(t.Context(), "container123").Return(&container.InspectResponse{ | ||
| ContainerJSONBase: &container.ContainerJSONBase{ | ||
| State: &container.State{ | ||
| Status: "running", | ||
| Running: true, | ||
| }, | ||
| }, | ||
| NetworkSettings: &container.NetworkSettings{ | ||
| NetworkSettingsBase: container.NetworkSettingsBase{ | ||
| Ports: nat.PortMap{ | ||
| nat.Port("5678/tcp"): []nat.PortBinding{ | ||
| { | ||
| HostIP: "127.0.0.1", | ||
| HostPort: "12345", | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, nil) | ||
|
|
||
| hostPort, err := GetHostPortForContainer(t.Context(), testClient, "container123", 5678, "127.0.0.1") | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit — table-driven + missing edge cases. Each of the 8 subtests repeats the full |
||
| require.NoError(t, err) | ||
| require.Equal(t, 12345, hostPort) | ||
| }) | ||
|
|
||
| t.Run("WithExposedPortDefaultHostIP", func(t *testing.T) { | ||
| testClient := dockertest.NewMockCommand2(t) | ||
| testClient.EXPECT().ContainerInspect(t.Context(), "container123").Return(&container.InspectResponse{ | ||
| ContainerJSONBase: &container.ContainerJSONBase{ | ||
| State: &container.State{ | ||
| Status: "running", | ||
| Running: true, | ||
| }, | ||
| }, | ||
| NetworkSettings: &container.NetworkSettings{ | ||
| NetworkSettingsBase: container.NetworkSettingsBase{ | ||
| Ports: nat.PortMap{ | ||
| nat.Port("5678/tcp"): []nat.PortBinding{ | ||
| { | ||
| HostIP: "127.0.0.1", | ||
| HostPort: "12345", | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, nil) | ||
|
|
||
| // Empty hostIP should default to 127.0.0.1 | ||
| hostPort, err := GetHostPortForContainer(t.Context(), testClient, "container123", 5678, "") | ||
| require.NoError(t, err) | ||
| require.Equal(t, 12345, hostPort) | ||
| }) | ||
|
|
||
| t.Run("WithExposedPortAllInterfaces", func(t *testing.T) { | ||
| testClient := dockertest.NewMockCommand2(t) | ||
| testClient.EXPECT().ContainerInspect(t.Context(), "container123").Return(&container.InspectResponse{ | ||
| ContainerJSONBase: &container.ContainerJSONBase{ | ||
|
|
@@ -35,7 +92,7 @@ func TestGetHostPortForContainer(t *testing.T) { | |
| }, | ||
| }, nil) | ||
|
|
||
| hostPort, err := GetHostPortForContainer(t.Context(), testClient, "container123", 5678) | ||
| hostPort, err := GetHostPortForContainer(t.Context(), testClient, "container123", 5678, "0.0.0.0") | ||
| require.NoError(t, err) | ||
| require.Equal(t, 12345, hostPort) | ||
| }) | ||
|
|
@@ -54,11 +111,11 @@ func TestGetHostPortForContainer(t *testing.T) { | |
| Ports: nat.PortMap{ | ||
| nat.Port("5678/tcp"): []nat.PortBinding{ | ||
| { | ||
| HostIP: "0.0.0.0", | ||
| HostIP: "127.0.0.1", | ||
| HostPort: "12345", | ||
| }, | ||
| { | ||
| HostIP: "0.0.0.0", | ||
| HostIP: "127.0.0.1", | ||
| HostPort: "54321", | ||
| }, | ||
| }, | ||
|
|
@@ -67,7 +124,7 @@ func TestGetHostPortForContainer(t *testing.T) { | |
| }, | ||
| }, nil) | ||
|
|
||
| hostPort, err := GetHostPortForContainer(t.Context(), testClient, "container123", 5678) | ||
| hostPort, err := GetHostPortForContainer(t.Context(), testClient, "container123", 5678, "127.0.0.1") | ||
| require.NoError(t, err) | ||
| require.Equal(t, 12345, hostPort) | ||
| }) | ||
|
|
@@ -86,7 +143,7 @@ func TestGetHostPortForContainer(t *testing.T) { | |
| Ports: nat.PortMap{ | ||
| nat.Port("5678/tcp"): []nat.PortBinding{ | ||
| { | ||
| HostIP: "127.0.0.1", | ||
| HostIP: "0.0.0.0", | ||
| HostPort: "12345", | ||
| }, | ||
| }, | ||
|
|
@@ -95,8 +152,8 @@ func TestGetHostPortForContainer(t *testing.T) { | |
| }, | ||
| }, nil) | ||
|
|
||
| _, err := GetHostPortForContainer(t.Context(), testClient, "container123", 5678) | ||
| require.ErrorContains(t, err, "does not have a port bound to 0.0.0.0") | ||
| _, err := GetHostPortForContainer(t.Context(), testClient, "container123", 5678, "127.0.0.1") | ||
| require.ErrorContains(t, err, "does not have a port bound to 127.0.0.1") | ||
| }) | ||
|
|
||
| t.Run("WithDifferentPortExposed", func(t *testing.T) { | ||
|
|
@@ -113,7 +170,7 @@ func TestGetHostPortForContainer(t *testing.T) { | |
| Ports: nat.PortMap{ | ||
| nat.Port("1234/tcp"): []nat.PortBinding{ | ||
| { | ||
| HostIP: "0.0.0.0", | ||
| HostIP: "127.0.0.1", | ||
| HostPort: "12345", | ||
| }, | ||
| }, | ||
|
|
@@ -122,8 +179,8 @@ func TestGetHostPortForContainer(t *testing.T) { | |
| }, | ||
| }, nil) | ||
|
|
||
| _, err := GetHostPortForContainer(t.Context(), testClient, "container123", 5678) | ||
| require.ErrorContains(t, err, "does not have a port bound to 0.0.0.0") | ||
| _, err := GetHostPortForContainer(t.Context(), testClient, "container123", 5678, "127.0.0.1") | ||
| require.ErrorContains(t, err, "does not have a port bound to 127.0.0.1") | ||
| }) | ||
|
|
||
| t.Run("WithNoExposedPort", func(t *testing.T) { | ||
|
|
@@ -137,7 +194,7 @@ func TestGetHostPortForContainer(t *testing.T) { | |
| }, | ||
| }, nil) | ||
|
|
||
| _, err := GetHostPortForContainer(t.Context(), testClient, "container123", 5678) | ||
| _, err := GetHostPortForContainer(t.Context(), testClient, "container123", 5678, "127.0.0.1") | ||
| require.ErrorContains(t, err, "does not have expected network configuration") | ||
| }) | ||
|
|
||
|
|
@@ -152,7 +209,7 @@ func TestGetHostPortForContainer(t *testing.T) { | |
| }, | ||
| }, nil) | ||
|
|
||
| _, err := GetHostPortForContainer(t.Context(), testClient, "container123", 5678) | ||
| _, err := GetHostPortForContainer(t.Context(), testClient, "container123", 5678, "127.0.0.1") | ||
| require.ErrorContains(t, err, "is not running") | ||
| }) | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit — generic var name. Package-level
hostcollides with a common local (docker.go:47useshost, err := determineDockerHost()).serveHostorbindHostwould be clearer and lower the shadowing risk. Consistent with the existingport/uploadURLconvention, so low priority.