fix(agent/agentcontainers): return host port instead of container port

johnstcn · johnstcn · commit f52142f6e699 · 2025-03-10T15:48:02.000Z
diff --git a/agent/agentcontainers/containers_dockercli.go b/agent/agentcontainers/containers_dockercli.go
@@ -320,13 +320,14 @@ func runDockerInspect(ctx context.Context, execer agentexec.Execer, ids ...strin
 // 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"`
+	HostConfig      dockerInspectHostConfig `json:"HostConfig"`
+	Name            string                  `json:"Name"`
+	Mounts          []dockerInspectMount    `json:"Mounts"`
+	State           dockerInspectState      `json:"State"`
+	NetworkSettings dockerNetworkSettings   `json:"NetworkSettings"`
 }
 
 type dockerInspectConfig struct {
@@ -335,7 +336,16 @@ type dockerInspectConfig struct {
 }
 
 type dockerInspectHostConfig struct {
-	PortBindings map[string]any `json:"PortBindings"`
+	PortBindings map[string][]dockerPortBinding `json:"PortBindings"`
+}
+
+type dockerPortBinding struct {
+	HostIP   string `json:"HostIp"`
+	HostPort string `json:"HostPort"`
+}
+
+type dockerNetworkSettings struct {
+	Ports map[string][]dockerPortBinding `json:"Ports"`
 }
 
 type dockerInspectMount struct {
@@ -382,20 +392,54 @@ func convertDockerInspect(in dockerInspect) (codersdk.WorkspaceAgentDevcontainer
 		Volumes:      make(map[string]string, len(in.Mounts)),
 	}
 
-	if in.HostConfig.PortBindings == nil {
-		in.HostConfig.PortBindings = make(map[string]any)
+	// Use NetworkSettings.Ports for externally accessible ports
+	if in.NetworkSettings.Ports == nil {
+		in.NetworkSettings.Ports = make(map[string][]dockerPortBinding)
 	}
-	portKeys := maps.Keys(in.HostConfig.PortBindings)
-	// Sort the ports for deterministic output.
+
+	// Track host ports to avoid duplicates (same port on multiple interfaces)
+	seenHostPorts := make(map[string]bool)
+
+	// Get all port mappings
+	portKeys := maps.Keys(in.NetworkSettings.Ports)
+	// 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 {
+
+	for containerPortSpec, bindings := range in.NetworkSettings.Ports {
+		// Skip if no bindings exist (don't expose the container port)
+		if len(bindings) == 0 {
+			continue
+		}
+
+		// Parse the containerPortSpec (e.g., "8080/tcp")
+		parts := strings.Split(containerPortSpec, "/")
+
+		// Get the network protocol
+		network := "tcp" // Default to tcp
+		if len(parts) == 2 {
+			network = parts[1]
+		}
+
+		// Process all bindings for this port (usually different interfaces)
+		for _, binding := range bindings {
+			// Skip if we've already seen this host port
+			if seenHostPorts[binding.HostPort] {
+				continue
+			}
+
+			hostPort, err := strconv.Atoi(binding.HostPort)
+			if err != nil {
+				warns = append(warns, fmt.Sprintf("invalid host port format: %s", binding.HostPort))
+				continue
+			}
+
 			out.Ports = append(out.Ports, codersdk.WorkspaceAgentListeningPort{
 				Network: network,
-				Port:    port,
+				Port:    uint16(hostPort),
 			})
+
+			// Mark this host port as seen
+			seenHostPorts[binding.HostPort] = true
 		}
 	}
 
@@ -412,28 +456,3 @@ func convertDockerInspect(in dockerInspect) (codersdk.WorkspaceAgentDevcontainer
 
 	return out, warns
 }
-
-// convertDockerPort converts a Docker port string to a port number and network
-// example: "8080/tcp" -> 8080, "tcp"
-//
-//	"8080" -> 8080, "tcp"
-func convertDockerPort(in string) (uint16, string, error) {
-	parts := strings.Split(in, "/")
-	switch len(parts) {
-	case 1:
-		// assume it's a TCP port
-		p, err := strconv.Atoi(parts[0])
-		if err != nil {
-			return 0, "", xerrors.Errorf("invalid port format: %s", in)
-		}
-		return uint16(p), "tcp", nil
-	case 2:
-		p, err := strconv.Atoi(parts[0])
-		if err != nil {
-			return 0, "", xerrors.Errorf("invalid port format: %s", in)
-		}
-		return uint16(p), parts[1], nil
-	default:
-		return 0, "", xerrors.Errorf("invalid port format: %s", in)
-	}
-}
diff --git a/agent/agentcontainers/containers_internal_test.go b/agent/agentcontainers/containers_internal_test.go
@@ -4,6 +4,7 @@ import (
 	"fmt"
 	"os"
 	"slices"
+	"sort"
 	"strconv"
 	"strings"
 	"testing"
@@ -46,7 +47,7 @@ func TestIntegrationDocker(t *testing.T) {
 	// Create a temporary directory to validate that we surface mounts correctly.
 	testTempDir := t.TempDir()
 	// Pick a random port to expose for testing port bindings.
-	testRandPort := testutil.RandomPortNoListen(t)
+	testRandHostPort, testRandContainerPort := testutil.RandomPortNoListen(t), testutil.RandomPortNoListen(t)
 	ct, err := pool.RunWithOptions(&dockertest.RunOptions{
 		Repository: "busybox",
 		Tag:        "latest",
@@ -56,12 +57,12 @@ func TestIntegrationDocker(t *testing.T) {
 			"devcontainer.metadata": `[{"remoteEnv": {"FOO": "bar", "MULTILINE": "foo\nbar\nbaz"}}]`,
 		},
 		Mounts:       []string{testTempDir + ":" + testTempDir},
-		ExposedPorts: []string{fmt.Sprintf("%d/tcp", testRandPort)},
+		ExposedPorts: []string{fmt.Sprintf("%d/tcp", testRandContainerPort)},
 		PortBindings: map[docker.Port][]docker.PortBinding{
-			docker.Port(fmt.Sprintf("%d/tcp", testRandPort)): {
+			docker.Port(fmt.Sprintf("%d/tcp", testRandContainerPort)): {
 				{
 					HostIP:   "0.0.0.0",
-					HostPort: strconv.FormatInt(int64(testRandPort), 10),
+					HostPort: strconv.FormatInt(int64(testRandHostPort), 10),
 				},
 			},
 		},
@@ -99,7 +100,7 @@ func TestIntegrationDocker(t *testing.T) {
 			assert.True(t, foundContainer.Running)
 			assert.Equal(t, "running", foundContainer.Status)
 			if assert.Len(t, foundContainer.Ports, 1) {
-				assert.Equal(t, testRandPort, foundContainer.Ports[0].Port)
+				assert.Equal(t, testRandHostPort, foundContainer.Ports[0].Port)
 				assert.Equal(t, "tcp", foundContainer.Ports[0].Network)
 			}
 			if assert.Len(t, foundContainer.Volumes, 1) {
@@ -306,67 +307,162 @@ func TestContainersHandler(t *testing.T) {
 	})
 }
 
-func TestConvertDockerPort(t *testing.T) {
+// TestDockerPortBinding tests the port binding handling in convertDockerInspect
+func TestDockerPortBinding(t *testing.T) {
 	t.Parallel()
 
-	for _, tc := range []struct {
+	testCases := []struct {
 		name          string
-		in            string
-		expectPort    uint16
-		expectNetwork string
-		expectError   string
+		networkPorts  map[string][]dockerPortBinding
+		expectedPorts []codersdk.WorkspaceAgentListeningPort
+		expectedWarns int
 	}{
 		{
-			name:        "empty port",
-			in:          "",
-			expectError: "invalid port",
+			name:          "nil",
+			networkPorts:  nil,
+			expectedPorts: []codersdk.WorkspaceAgentListeningPort{},
 		},
 		{
-			name:          "valid tcp port",
-			in:            "8080/tcp",
-			expectPort:    8080,
-			expectNetwork: "tcp",
+			name: "simple port binding",
+			networkPorts: map[string][]dockerPortBinding{
+				"8080/tcp": {
+					{HostIP: "0.0.0.0", HostPort: "9090"},
+				},
+			},
+			expectedPorts: []codersdk.WorkspaceAgentListeningPort{
+				{Network: "tcp", Port: 9090},
+			},
 		},
 		{
-			name:          "valid udp port",
-			in:            "8080/udp",
-			expectPort:    8080,
-			expectNetwork: "udp",
+			name: "multiple port bindings",
+			networkPorts: map[string][]dockerPortBinding{
+				"8080/tcp": {
+					{HostIP: "0.0.0.0", HostPort: "9090"},
+				},
+				"8081/tcp": {
+					{HostIP: "0.0.0.0", HostPort: "9091"},
+				},
+			},
+			expectedPorts: []codersdk.WorkspaceAgentListeningPort{
+				{Network: "tcp", Port: 9090},
+				{Network: "tcp", Port: 9091},
+			},
 		},
 		{
-			name:          "valid port no network",
-			in:            "8080",
-			expectPort:    8080,
-			expectNetwork: "tcp",
+			name: "duplicate host ports on different interfaces",
+			networkPorts: map[string][]dockerPortBinding{
+				"8080/tcp": {
+					{HostIP: "0.0.0.0", HostPort: "9090"},
+					{HostIP: "127.0.0.1", HostPort: "9090"},
+				},
+			},
+			expectedPorts: []codersdk.WorkspaceAgentListeningPort{
+				{Network: "tcp", Port: 9090},
+			},
 		},
 		{
-			name:        "invalid port",
-			in:          "invalid/tcp",
-			expectError: "invalid port",
+			name: "udp protocol",
+			networkPorts: map[string][]dockerPortBinding{
+				"8080/udp": {
+					{HostIP: "0.0.0.0", HostPort: "9090"},
+				},
+			},
+			expectedPorts: []codersdk.WorkspaceAgentListeningPort{
+				{Network: "udp", Port: 9090},
+			},
 		},
 		{
-			name:        "invalid port no network",
-			in:          "invalid",
-			expectError: "invalid port",
+			name: "no protocol defaults to tcp",
+			networkPorts: map[string][]dockerPortBinding{
+				"8080": {
+					{HostIP: "0.0.0.0", HostPort: "9090"},
+				},
+			},
+			expectedPorts: []codersdk.WorkspaceAgentListeningPort{
+				{Network: "tcp", Port: 9090},
+			},
 		},
 		{
-			name:        "multiple network",
-			in:          "8080/tcp/udp",
-			expectError: "invalid port",
+			name: "no bindings should not create ports",
+			networkPorts: map[string][]dockerPortBinding{
+				"8080/tcp": {},
+			},
+			expectedPorts: []codersdk.WorkspaceAgentListeningPort{},
 		},
-	} {
-		tc := tc // not needed anymore but makes the linter happy
+		{
+			name: "invalid host port",
+			networkPorts: map[string][]dockerPortBinding{
+				"8080/tcp": {
+					{HostIP: "0.0.0.0", HostPort: "invalid"},
+				},
+			},
+			expectedPorts: []codersdk.WorkspaceAgentListeningPort{},
+			expectedWarns: 1,
+		},
+		{
+			name: "mix of valid and invalid ports",
+			networkPorts: map[string][]dockerPortBinding{
+				"8080/tcp": {
+					{HostIP: "0.0.0.0", HostPort: "9090"},
+				},
+				"8081/tcp": {
+					{HostIP: "0.0.0.0", HostPort: "invalid"},
+				},
+			},
+			expectedPorts: []codersdk.WorkspaceAgentListeningPort{
+				{Network: "tcp", Port: 9090},
+			},
+			expectedWarns: 1,
+		},
+	}
+
+	for _, tc := range testCases {
+		tc := tc
 		t.Run(tc.name, func(t *testing.T) {
 			t.Parallel()
-			actualPort, actualNetwork, actualErr := convertDockerPort(tc.in)
-			if tc.expectError != "" {
-				assert.Zero(t, actualPort, "expected no port")
-				assert.Empty(t, actualNetwork, "expected no network")
-				assert.ErrorContains(t, actualErr, tc.expectError)
-			} else {
-				assert.NoError(t, actualErr, "expected no error")
-				assert.Equal(t, tc.expectPort, actualPort, "expected port to match")
-				assert.Equal(t, tc.expectNetwork, actualNetwork, "expected network to match")
+
+			// Create a sample docker inspection result
+			dockerData := dockerInspect{
+				ID:      "test-container",
+				Created: time.Now(),
+				Config: dockerInspectConfig{
+					Image:  "test-image",
+					Labels: map[string]string{"test": "value"},
+				},
+				Name:  "test-container",
+				State: dockerInspectState{Running: true},
+				NetworkSettings: dockerNetworkSettings{
+					Ports: tc.networkPorts,
+				},
+			}
+
+			// Process the docker data
+			container, warns := convertDockerInspect(dockerData)
+
+			// Verify the ports
+			assert.Len(t, container.Ports, len(tc.expectedPorts), "wrong number of ports")
+			assert.Len(t, warns, tc.expectedWarns, "wrong number of warnings")
+
+			// Sort ports for consistent comparison (order may vary)
+			sort.Slice(container.Ports, func(i, j int) bool {
+				if container.Ports[i].Network == container.Ports[j].Network {
+					return container.Ports[i].Port < container.Ports[j].Port
+				}
+				return container.Ports[i].Network < container.Ports[j].Network
+			})
+			sort.Slice(tc.expectedPorts, func(i, j int) bool {
+				if tc.expectedPorts[i].Network == tc.expectedPorts[j].Network {
+					return tc.expectedPorts[i].Port < tc.expectedPorts[j].Port
+				}
+				return tc.expectedPorts[i].Network < tc.expectedPorts[j].Network
+			})
+
+			// Compare ports
+			for i, expected := range tc.expectedPorts {
+				if i < len(container.Ports) {
+					assert.Equal(t, expected.Network, container.Ports[i].Network, "network mismatch")
+					assert.Equal(t, expected.Port, container.Ports[i].Port, "port mismatch")
+				}
 			}
 		})
 	}
