coder · johnstcn · Mar 18, 2025 · Mar 11, 2025 · Mar 11, 2025 · Mar 11, 2025
diff --git a/agent/agentcontainers/containers_dockercli.go b/agent/agentcontainers/containers_dockercli.go
@@ -6,6 +6,7 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
+	"net"
 	"os/user"
 	"slices"
 	"sort"
@@ -162,23 +163,28 @@ func (dei *DockerEnvInfoer) ModifyCommand(cmd string, args ...string) (string, [
 // devcontainerEnv is a helper function that inspects the container labels to
 // find the required environment variables for running a command in the container.
 func devcontainerEnv(ctx context.Context, execer agentexec.Execer, container string) ([]string, error) {
-	ins, stderr, err := runDockerInspect(ctx, execer, container)
+	stdout, stderr, err := runDockerInspect(ctx, execer, container)
 	if err != nil {
 		return nil, xerrors.Errorf("inspect container: %w: %q", err, stderr)
 	}
 
+	ins, _, err := convertDockerInspect(stdout)
+	if err != nil {
+		return nil, xerrors.Errorf("inspect container: %w", err)
+	}
+
 	if len(ins) != 1 {
 		return nil, xerrors.Errorf("inspect container: expected 1 container, got %d", len(ins))
 	}
 
 	in := ins[0]
-	if in.Config.Labels == nil {
+	if in.Labels == nil {
 		return nil, nil
 	}
 
 	// We want to look for the devcontainer metadata, which is in the
 	// value of the label `devcontainer.metadata`.
-	rawMeta, ok := in.Config.Labels["devcontainer.metadata"]
+	rawMeta, ok := in.Labels["devcontainer.metadata"]
 	if !ok {
 		return nil, nil
 	}
@@ -274,68 +280,63 @@ func (dcl *DockerCLILister) List(ctx context.Context) (codersdk.WorkspaceAgentLi
 	// will still contain valid JSON. We will just end up missing
 	// information about the removed container. We could potentially
 	// log this error, but I'm not sure it's worth it.
-	ins, dockerInspectStderr, err := runDockerInspect(ctx, dcl.execer, ids...)
+	dockerInspectStdout, dockerInspectStderr, err := runDockerInspect(ctx, dcl.execer, ids...)
 	if err != nil {
 		return codersdk.WorkspaceAgentListContainersResponse{}, xerrors.Errorf("run docker inspect: %w", err)
 	}
 
-	for _, in := range ins {
-		out, warns := convertDockerInspect(in)
-		res.Warnings = append(res.Warnings, warns...)
-		res.Containers = append(res.Containers, out)
+	if len(dockerInspectStderr) > 0 {
+		res.Warnings = append(res.Warnings, string(dockerInspectStderr))
 	}
 
-	if dockerPsStderr != "" {
-		res.Warnings = append(res.Warnings, dockerPsStderr)
-	}
-	if dockerInspectStderr != "" {
-		res.Warnings = append(res.Warnings, dockerInspectStderr)
+	outs, warns, err := convertDockerInspect(dockerInspectStdout)
+	if err != nil {
+		return codersdk.WorkspaceAgentListContainersResponse{}, xerrors.Errorf("convert docker inspect output: %w", err)
 	}
+	res.Warnings = append(res.Warnings, warns...)
+	res.Containers = append(res.Containers, outs...)
 
 	return res, nil
 }
 
 // runDockerInspect is a helper function that runs `docker inspect` on the given
 // container IDs and returns the parsed output.
 // The stderr output is also returned for logging purposes.
-func runDockerInspect(ctx context.Context, execer agentexec.Execer, ids ...string) ([]dockerInspect, string, error) {
+func runDockerInspect(ctx context.Context, execer agentexec.Execer, ids ...string) (stdout, stderr []byte, err error) {
 	var stdoutBuf, stderrBuf bytes.Buffer
 	cmd := execer.CommandContext(ctx, "docker", append([]string{"inspect"}, ids...)...)
 	cmd.Stdout = &stdoutBuf
 	cmd.Stderr = &stderrBuf
-	err := cmd.Run()
-	stderr := strings.TrimSpace(stderrBuf.String())
+	err = cmd.Run()
+	stdout = bytes.TrimSpace(stdoutBuf.Bytes())
+	stderr = bytes.TrimSpace(stderrBuf.Bytes())
 	if err != nil {
-		return nil, stderr, err
-	}
-
-	var ins []dockerInspect
-	if err := json.NewDecoder(&stdoutBuf).Decode(&ins); err != nil {
-		return nil, stderr, xerrors.Errorf("decode docker inspect output: %w", err)
+		return stdout, stderr, err
 	}
 
-	return ins, stderr, nil
+	return stdout, stderr, nil
 }
 
 // To avoid a direct dependency on the Docker API, we use the docker CLI
 // to fetch information about containers.
 type dockerInspect struct {
-	ID         string                  `json:"Id"`
-	Created    time.Time               `json:"Created"`
-	Config     dockerInspectConfig     `json:"Config"`
-	HostConfig dockerInspectHostConfig `json:"HostConfig"`
-	Name       string                  `json:"Name"`
-	Mounts     []dockerInspectMount    `json:"Mounts"`
-	State      dockerInspectState      `json:"State"`
+	ID              string                       `json:"Id"`
+	Created         time.Time                    `json:"Created"`
+	Config          dockerInspectConfig          `json:"Config"`
+	Name            string                       `json:"Name"`
+	Mounts          []dockerInspectMount         `json:"Mounts"`
+	State           dockerInspectState           `json:"State"`
+	NetworkSettings dockerInspectNetworkSettings `json:"NetworkSettings"`
 }
 
 type dockerInspectConfig struct {
 	Image  string            `json:"Image"`
 	Labels map[string]string `json:"Labels"`
 }
 
-type dockerInspectHostConfig struct {
-	PortBindings map[string]any `json:"PortBindings"`
+type dockerInspectPort struct {
+	HostIP   string `json:"HostIp"`
+	HostPort string `json:"HostPort"`
 }
 
 type dockerInspectMount struct {
@@ -350,6 +351,10 @@ type dockerInspectState struct {
 	Error    string `json:"Error"`
 }
 
+type dockerInspectNetworkSettings struct {
+	Ports map[string][]dockerInspectPort `json:"Ports"`
+}
+
 func (dis dockerInspectState) String() string {
 	if dis.Running {
 		return "running"
@@ -367,50 +372,108 @@ func (dis dockerInspectState) String() string {
 	return sb.String()
 }
 
-func convertDockerInspect(in dockerInspect) (codersdk.WorkspaceAgentDevcontainer, []string) {
+func convertDockerInspect(raw []byte) ([]codersdk.WorkspaceAgentDevcontainer, []string, error) {
 	var warns []string
-	out := codersdk.WorkspaceAgentDevcontainer{
-		CreatedAt: in.Created,
-		// Remove the leading slash from the container name
-		FriendlyName: strings.TrimPrefix(in.Name, "/"),
-		ID:           in.ID,
-		Image:        in.Config.Image,
-		Labels:       in.Config.Labels,
-		Ports:        make([]codersdk.WorkspaceAgentListeningPort, 0),
-		Running:      in.State.Running,
-		Status:       in.State.String(),
-		Volumes:      make(map[string]string, len(in.Mounts)),
-	}
-
-	if in.HostConfig.PortBindings == nil {
-		in.HostConfig.PortBindings = make(map[string]any)
-	}
-	portKeys := maps.Keys(in.HostConfig.PortBindings)
-	// Sort the ports for deterministic output.
-	sort.Strings(portKeys)
-	for _, p := range portKeys {
-		if port, network, err := convertDockerPort(p); err != nil {
-			warns = append(warns, err.Error())
-		} else {
-			out.Ports = append(out.Ports, codersdk.WorkspaceAgentListeningPort{
-				Network: network,
-				Port:    port,
-			})
+	var ins []dockerInspect
+	if err := json.NewDecoder(bytes.NewReader(raw)).Decode(&ins); err != nil {
+		return nil, nil, xerrors.Errorf("decode docker inspect output: %w", err)
+	}
+	outs := make([]codersdk.WorkspaceAgentDevcontainer, 0, len(ins))
+
+	// Say you have two containers:
+	//  - Container A with Host IP 127.0.0.1:8000 mapped to container port 8001
+	//  - Container B with Host IP [::1]:8000 mapped to container port 8001
+	// A request to localhost:8000 may be routed to either container.
+	// We don't know which one for sure, so we need to surface this to the user.
+	// Keep track of all host ports we see. If we see the same host port
+	// mapped to multiple containers on different host IPs, we need to
+	// warn the user about this.
+	// Note that we only do this for loopback or unspecified IPs.
+	// We'll assume that the user knows what they're doing if they bind to
+	// a specific IP address.
+	hostPortContainers := make(map[int][]string)
+
+	for _, in := range ins {
+		out := codersdk.WorkspaceAgentDevcontainer{
+			CreatedAt: in.Created,
+			// Remove the leading slash from the container name
+			FriendlyName: strings.TrimPrefix(in.Name, "/"),
+			ID:           in.ID,
+			Image:        in.Config.Image,
+			Labels:       in.Config.Labels,
+			Ports:        make([]codersdk.WorkspaceAgentDevcontainerPort, 0),
+			Running:      in.State.Running,
+			Status:       in.State.String(),
+			Volumes:      make(map[string]string, len(in.Mounts)),
 		}
-	}
 
-	if in.Mounts == nil {
-		in.Mounts = []dockerInspectMount{}
+		if in.NetworkSettings.Ports == nil {
+			in.NetworkSettings.Ports = make(map[string][]dockerInspectPort)
+		}
+		portKeys := maps.Keys(in.NetworkSettings.Ports)
+		// Sort the ports for deterministic output.
+		sort.Strings(portKeys)
+		// If we see the same port bound to both ipv4 and ipv6 loopback or unspecified
+		// interfaces to the same container port, there is no point in adding it multiple times.
+		loopbackHostPortContainerPorts := make(map[int]uint16, 0)
+		for _, pk := range portKeys {
+			for _, p := range in.NetworkSettings.Ports[pk] {
+				cp, network, err := convertDockerPort(pk)
+				if err != nil {
+					warns = append(warns, fmt.Sprintf("convert docker port: %s", err.Error()))
+					// Default network to "tcp" if we can't parse it.
+					network = "tcp"
+				}
+				hp, err := strconv.Atoi(p.HostPort)
+				if err != nil {
+					warns = append(warns, fmt.Sprintf("convert docker host port: %s", err.Error()))
+					continue
+				}
+				if hp > 65535 || hp < 1 { // invalid port
+					warns = append(warns, fmt.Sprintf("convert docker host port: invalid host port %d", hp))
+					continue
+				}
+
+				// Deduplicate host ports for loopback and unspecified IPs.
+				if isLoopbackOrUnspecified(p.HostIP) {
+					if found, ok := loopbackHostPortContainerPorts[hp]; ok && found == cp {
+						// We've already seen this port, so skip it.
+						continue
+					}
+					loopbackHostPortContainerPorts[hp] = cp
+					// Also keep track of the host port and the container ID.
+					hostPortContainers[hp] = append(hostPortContainers[hp], in.ID)
+				}
+				out.Ports = append(out.Ports, codersdk.WorkspaceAgentDevcontainerPort{
+					Network:  network,
+					Port:     cp,
+					HostPort: uint16(hp),
+					HostIP:   p.HostIP,
+				})
+			}
+		}
+
+		if in.Mounts == nil {
+			in.Mounts = []dockerInspectMount{}
+		}
+		// Sort the mounts for deterministic output.
+		sort.Slice(in.Mounts, func(i, j int) bool {
+			return in.Mounts[i].Source < in.Mounts[j].Source
+		})
+		for _, k := range in.Mounts {
+			out.Volumes[k.Source] = k.Destination
+		}
+		outs = append(outs, out)
 	}
-	// Sort the mounts for deterministic output.
-	sort.Slice(in.Mounts, func(i, j int) bool {
-		return in.Mounts[i].Source < in.Mounts[j].Source
-	})
-	for _, k := range in.Mounts {
-		out.Volumes[k.Source] = k.Destination
+
+	// Check if any host ports are mapped to multiple containers.
+	for hp, ids := range hostPortContainers {
+		if len(ids) > 1 {
+			warns = append(warns, fmt.Sprintf("host port %d is mapped to multiple containers on different interfaces: %s", hp, strings.Join(ids, ", ")))
+		}
 	}
 
-	return out, warns
+	return outs, warns, nil
 }
 
 // convertDockerPort converts a Docker port string to a port number and network
@@ -437,3 +500,12 @@ func convertDockerPort(in string) (uint16, string, error) {
 		return 0, "", xerrors.Errorf("invalid port format: %s", in)
 	}
 }
+
+// convenience function to check if an IP address is loopback or unspecified
+func isLoopbackOrUnspecified(ips string) bool {
+	nip := net.ParseIP(ips)
+	if nip == nil {
+		return false // technically correct, I suppose
+	}
+	return nip.IsLoopback() || nip.IsUnspecified()
+}