From f52142f6e69943287ebf583183b34c6eb452a0f3 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Mon, 10 Mar 2025 15:48:02 +0000 Subject: [PATCH 1/2] fix(agent/agentcontainers): return host port instead of container port --- agent/agentcontainers/containers_dockercli.go | 103 ++++++---- .../containers_internal_test.go | 188 +++++++++++++----- 2 files changed, 203 insertions(+), 88 deletions(-) diff --git a/agent/agentcontainers/containers_dockercli.go b/agent/agentcontainers/containers_dockercli.go index 4d4bd68ee0f10..fd5eceb5b8e10 100644 --- 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 index fc3928229f2f5..bd395a9e3032f 100644 --- 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") + } } }) } From 077d5a9849f6f2dcb989d52ea2f327c87573a661 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Mon, 10 Mar 2025 16:05:47 +0000 Subject: [PATCH 2/2] fixup! fix(agent/agentcontainers): return host port instead of container port --- agent/agentcontainers/containers_dockercli.go | 7 ++-- .../containers_internal_test.go | 33 +++---------------- 2 files changed, 8 insertions(+), 32 deletions(-) diff --git a/agent/agentcontainers/containers_dockercli.go b/agent/agentcontainers/containers_dockercli.go index fd5eceb5b8e10..30efeb7502587 100644 --- a/agent/agentcontainers/containers_dockercli.go +++ b/agent/agentcontainers/containers_dockercli.go @@ -400,12 +400,13 @@ func convertDockerInspect(in dockerInspect) (codersdk.WorkspaceAgentDevcontainer // Track host ports to avoid duplicates (same port on multiple interfaces) seenHostPorts := make(map[string]bool) - // Get all port mappings + // Get port mappings in sorted order for deterministic output portKeys := maps.Keys(in.NetworkSettings.Ports) - // Sort the ports for deterministic output sort.Strings(portKeys) - for containerPortSpec, bindings := range in.NetworkSettings.Ports { + for _, containerPortSpec := range portKeys { + bindings := in.NetworkSettings.Ports[containerPortSpec] + // Skip if no bindings exist (don't expose the container port) if len(bindings) == 0 { continue diff --git a/agent/agentcontainers/containers_internal_test.go b/agent/agentcontainers/containers_internal_test.go index bd395a9e3032f..dc625914caa69 100644 --- a/agent/agentcontainers/containers_internal_test.go +++ b/agent/agentcontainers/containers_internal_test.go @@ -4,7 +4,6 @@ import ( "fmt" "os" "slices" - "sort" "strconv" "strings" "testing" @@ -12,6 +11,7 @@ import ( "go.uber.org/mock/gomock" + "github.com/google/go-cmp/cmp" "github.com/google/uuid" "github.com/ory/dockertest/v3" "github.com/ory/dockertest/v3/docker" @@ -307,7 +307,6 @@ func TestContainersHandler(t *testing.T) { }) } -// TestDockerPortBinding tests the port binding handling in convertDockerInspect func TestDockerPortBinding(t *testing.T) { t.Parallel() @@ -421,7 +420,6 @@ func TestDockerPortBinding(t *testing.T) { t.Run(tc.name, func(t *testing.T) { t.Parallel() - // Create a sample docker inspection result dockerData := dockerInspect{ ID: "test-container", Created: time.Now(), @@ -436,34 +434,11 @@ func TestDockerPortBinding(t *testing.T) { }, } - // 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") - } + if diff := cmp.Diff(tc.expectedPorts, container.Ports); diff != "" { + assert.Failf(t, "port mismatch", "(-want +got):\n%s", diff) } + assert.Len(t, warns, tc.expectedWarns, "wrong number of warnings") }) } }