Skip to content

Commit 75b27e8

Browse files
johnstcnjohnstcn
authored
fix(agent/agentcontainers): improve testing of convertDockerInspect, return correct host port (coder#16887)
* Improves separation of concerns between `runDockerInspect` and `convertDockerInspect`: `runDockerInspect` now just runs the command and returns the output, while `convertDockerInspect` now does all of the conversion and parsing logic. * Improves testing of `convertDockerInspect` using real test fixtures. * Fixes issue where the container port is returned instead of the host port. * Updates UI to link to correct host port. Container port is still displayed in the button text, but the HostIP:HostPort is shown in a popover. * Adds stories for workspace agent UI
1 parent 13d0dac commit 75b27e8

File tree

22 files changed

+2612
-114
lines changed

22 files changed

+2612
-114
lines changed

‎agent/agentcontainers/containers_dockercli.go

+142-70
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import (
66
"context"
77
"encoding/json"
88
"fmt"
9+
"net"
910
"os/user"
1011
"slices"
1112
"sort"
@@ -164,23 +165,28 @@ func (dei *DockerEnvInfoer) ModifyCommand(cmd string, args ...string) (string, [
164165
// devcontainerEnv is a helper function that inspects the container labels to
165166
// find the required environment variables for running a command in the container.
166167
func devcontainerEnv(ctx context.Context, execer agentexec.Execer, container string) ([]string, error) {
167-
ins, stderr, err := runDockerInspect(ctx, execer, container)
168+
stdout, stderr, err := runDockerInspect(ctx, execer, container)
168169
if err != nil {
169170
return nil, xerrors.Errorf("inspect container: %w: %q", err, stderr)
170171
}
171172

173+
ins, _, err := convertDockerInspect(stdout)
174+
if err != nil {
175+
return nil, xerrors.Errorf("inspect container: %w", err)
176+
}
177+
172178
if len(ins) != 1 {
173179
return nil, xerrors.Errorf("inspect container: expected 1 container, got %d", len(ins))
174180
}
175181

176182
in := ins[0]
177-
if in.Config.Labels == nil {
183+
if in.Labels == nil {
178184
return nil, nil
179185
}
180186

181187
// We want to look for the devcontainer metadata, which is in the
182188
// value of the label `devcontainer.metadata`.
183-
rawMeta, ok := in.Config.Labels["devcontainer.metadata"]
189+
rawMeta, ok := in.Labels["devcontainer.metadata"]
184190
if !ok {
185191
return nil, nil
186192
}
@@ -282,68 +288,63 @@ func (dcl *DockerCLILister) List(ctx context.Context) (codersdk.WorkspaceAgentLi
282288
// will still contain valid JSON. We will just end up missing
283289
// information about the removed container. We could potentially
284290
// log this error, but I'm not sure it's worth it.
285-
ins, dockerInspectStderr, err := runDockerInspect(ctx, dcl.execer, ids...)
291+
dockerInspectStdout, dockerInspectStderr, err := runDockerInspect(ctx, dcl.execer, ids...)
286292
if err != nil {
287293
return codersdk.WorkspaceAgentListContainersResponse{}, xerrors.Errorf("run docker inspect: %w: %s", err, dockerInspectStderr)
288294
}
289295

290-
for _, in := range ins {
291-
out, warns := convertDockerInspect(in)
292-
res.Warnings = append(res.Warnings, warns...)
293-
res.Containers = append(res.Containers, out)
296+
if len(dockerInspectStderr) > 0 {
297+
res.Warnings = append(res.Warnings, string(dockerInspectStderr))
294298
}
295299

296-
if dockerPsStderr != "" {
297-
res.Warnings = append(res.Warnings, dockerPsStderr)
298-
}
299-
if dockerInspectStderr != "" {
300-
res.Warnings = append(res.Warnings, dockerInspectStderr)
300+
outs, warns, err := convertDockerInspect(dockerInspectStdout)
301+
if err != nil {
302+
return codersdk.WorkspaceAgentListContainersResponse{}, xerrors.Errorf("convert docker inspect output: %w", err)
301303
}
304+
res.Warnings = append(res.Warnings, warns...)
305+
res.Containers = append(res.Containers, outs...)
302306

303307
return res, nil
304308
}
305309

306310
// runDockerInspect is a helper function that runs `docker inspect` on the given
307311
// container IDs and returns the parsed output.
308312
// The stderr output is also returned for logging purposes.
309-
func runDockerInspect(ctx context.Context, execer agentexec.Execer, ids ...string) ([]dockerInspect, string, error) {
313+
func runDockerInspect(ctx context.Context, execer agentexec.Execer, ids ...string) (stdout, stderr []byte, err error) {
310314
var stdoutBuf, stderrBuf bytes.Buffer
311315
cmd := execer.CommandContext(ctx, "docker", append([]string{"inspect"}, ids...)...)
312316
cmd.Stdout = &stdoutBuf
313317
cmd.Stderr = &stderrBuf
314-
err := cmd.Run()
315-
stderr := strings.TrimSpace(stderrBuf.String())
318+
err = cmd.Run()
319+
stdout = bytes.TrimSpace(stdoutBuf.Bytes())
320+
stderr = bytes.TrimSpace(stderrBuf.Bytes())
316321
if err != nil {
317-
return nil, stderr, err
318-
}
319-
320-
var ins []dockerInspect
321-
if err := json.NewDecoder(&stdoutBuf).Decode(&ins); err != nil {
322-
return nil, stderr, xerrors.Errorf("decode docker inspect output: %w", err)
322+
return stdout, stderr, err
323323
}
324324

325-
return ins, stderr, nil
325+
return stdout, stderr, nil
326326
}
327327

328328
// To avoid a direct dependency on the Docker API, we use the docker CLI
329329
// to fetch information about containers.
330330
type dockerInspect struct {
331-
ID string `json:"Id"`
332-
Created time.Time `json:"Created"`
333-
Config dockerInspectConfig `json:"Config"`
334-
HostConfig dockerInspectHostConfig `json:"HostConfig"`
335-
Name string `json:"Name"`
336-
Mounts []dockerInspectMount `json:"Mounts"`
337-
State dockerInspectState `json:"State"`
331+
ID string `json:"Id"`
332+
Created time.Time `json:"Created"`
333+
Config dockerInspectConfig `json:"Config"`
334+
Name string `json:"Name"`
335+
Mounts []dockerInspectMount `json:"Mounts"`
336+
State dockerInspectState `json:"State"`
337+
NetworkSettings dockerInspectNetworkSettings `json:"NetworkSettings"`
338338
}
339339

340340
type dockerInspectConfig struct {
341341
Image string `json:"Image"`
342342
Labels map[string]string `json:"Labels"`
343343
}
344344

345-
type dockerInspectHostConfig struct {
346-
PortBindings map[string]any `json:"PortBindings"`
345+
type dockerInspectPort struct {
346+
HostIP string `json:"HostIp"`
347+
HostPort string `json:"HostPort"`
347348
}
348349

349350
type dockerInspectMount struct {
@@ -358,6 +359,10 @@ type dockerInspectState struct {
358359
Error string `json:"Error"`
359360
}
360361

362+
type dockerInspectNetworkSettings struct {
363+
Ports map[string][]dockerInspectPort `json:"Ports"`
364+
}
365+
361366
func (dis dockerInspectState) String() string {
362367
if dis.Running {
363368
return "running"
@@ -375,50 +380,108 @@ func (dis dockerInspectState) String() string {
375380
return sb.String()
376381
}
377382

378-
func convertDockerInspect(in dockerInspect) (codersdk.WorkspaceAgentDevcontainer, []string) {
383+
func convertDockerInspect(raw []byte) ([]codersdk.WorkspaceAgentDevcontainer, []string, error) {
379384
var warns []string
380-
out := codersdk.WorkspaceAgentDevcontainer{
381-
CreatedAt: in.Created,
382-
// Remove the leading slash from the container name
383-
FriendlyName: strings.TrimPrefix(in.Name, "/"),
384-
ID: in.ID,
385-
Image: in.Config.Image,
386-
Labels: in.Config.Labels,
387-
Ports: make([]codersdk.WorkspaceAgentListeningPort, 0),
388-
Running: in.State.Running,
389-
Status: in.State.String(),
390-
Volumes: make(map[string]string, len(in.Mounts)),
391-
}
392-
393-
if in.HostConfig.PortBindings == nil {
394-
in.HostConfig.PortBindings = make(map[string]any)
395-
}
396-
portKeys := maps.Keys(in.HostConfig.PortBindings)
397-
// Sort the ports for deterministic output.
398-
sort.Strings(portKeys)
399-
for _, p := range portKeys {
400-
if port, network, err := convertDockerPort(p); err != nil {
401-
warns = append(warns, err.Error())
402-
} else {
403-
out.Ports = append(out.Ports, codersdk.WorkspaceAgentListeningPort{
404-
Network: network,
405-
Port: port,
406-
})
385+
var ins []dockerInspect
386+
if err := json.NewDecoder(bytes.NewReader(raw)).Decode(&ins); err != nil {
387+
return nil, nil, xerrors.Errorf("decode docker inspect output: %w", err)
388+
}
389+
outs := make([]codersdk.WorkspaceAgentDevcontainer, 0, len(ins))
390+
391+
// Say you have two containers:
392+
// - Container A with Host IP 127.0.0.1:8000 mapped to container port 8001
393+
// - Container B with Host IP [::1]:8000 mapped to container port 8001
394+
// A request to localhost:8000 may be routed to either container.
395+
// We don't know which one for sure, so we need to surface this to the user.
396+
// Keep track of all host ports we see. If we see the same host port
397+
// mapped to multiple containers on different host IPs, we need to
398+
// warn the user about this.
399+
// Note that we only do this for loopback or unspecified IPs.
400+
// We'll assume that the user knows what they're doing if they bind to
401+
// a specific IP address.
402+
hostPortContainers := make(map[int][]string)
403+
404+
for _, in := range ins {
405+
out := codersdk.WorkspaceAgentDevcontainer{
406+
CreatedAt: in.Created,
407+
// Remove the leading slash from the container name
408+
FriendlyName: strings.TrimPrefix(in.Name, "/"),
409+
ID: in.ID,
410+
Image: in.Config.Image,
411+
Labels: in.Config.Labels,
412+
Ports: make([]codersdk.WorkspaceAgentDevcontainerPort, 0),
413+
Running: in.State.Running,
414+
Status: in.State.String(),
415+
Volumes: make(map[string]string, len(in.Mounts)),
416+
}
417+
418+
if in.NetworkSettings.Ports == nil {
419+
in.NetworkSettings.Ports = make(map[string][]dockerInspectPort)
420+
}
421+
portKeys := maps.Keys(in.NetworkSettings.Ports)
422+
// Sort the ports for deterministic output.
423+
sort.Strings(portKeys)
424+
// If we see the same port bound to both ipv4 and ipv6 loopback or unspecified
425+
// interfaces to the same container port, there is no point in adding it multiple times.
426+
loopbackHostPortContainerPorts := make(map[int]uint16, 0)
427+
for _, pk := range portKeys {
428+
for _, p := range in.NetworkSettings.Ports[pk] {
429+
cp, network, err := convertDockerPort(pk)
430+
if err != nil {
431+
warns = append(warns, fmt.Sprintf("convert docker port: %s", err.Error()))
432+
// Default network to "tcp" if we can't parse it.
433+
network = "tcp"
434+
}
435+
hp, err := strconv.Atoi(p.HostPort)
436+
if err != nil {
437+
warns = append(warns, fmt.Sprintf("convert docker host port: %s", err.Error()))
438+
continue
439+
}
440+
if hp > 65535 || hp < 1 { // invalid port
441+
warns = append(warns, fmt.Sprintf("convert docker host port: invalid host port %d", hp))
442+
continue
443+
}
444+
445+
// Deduplicate host ports for loopback and unspecified IPs.
446+
if isLoopbackOrUnspecified(p.HostIP) {
447+
if found, ok := loopbackHostPortContainerPorts[hp]; ok && found == cp {
448+
// We've already seen this port, so skip it.
449+
continue
450+
}
451+
loopbackHostPortContainerPorts[hp] = cp
452+
// Also keep track of the host port and the container ID.
453+
hostPortContainers[hp] = append(hostPortContainers[hp], in.ID)
454+
}
455+
out.Ports = append(out.Ports, codersdk.WorkspaceAgentDevcontainerPort{
456+
Network: network,
457+
Port: cp,
458+
HostPort: uint16(hp),
459+
HostIP: p.HostIP,
460+
})
461+
}
407462
}
408-
}
409463

410-
if in.Mounts == nil {
411-
in.Mounts = []dockerInspectMount{}
464+
if in.Mounts == nil {
465+
in.Mounts = []dockerInspectMount{}
466+
}
467+
// Sort the mounts for deterministic output.
468+
sort.Slice(in.Mounts, func(i, j int) bool {
469+
return in.Mounts[i].Source < in.Mounts[j].Source
470+
})
471+
for _, k := range in.Mounts {
472+
out.Volumes[k.Source] = k.Destination
473+
}
474+
outs = append(outs, out)
412475
}
413-
// Sort the mounts for deterministic output.
414-
sort.Slice(in.Mounts, func(i, j int) bool {
415-
return in.Mounts[i].Source < in.Mounts[j].Source
416-
})
417-
for _, k := range in.Mounts {
418-
out.Volumes[k.Source] = k.Destination
476+
477+
// Check if any host ports are mapped to multiple containers.
478+
for hp, ids := range hostPortContainers {
479+
if len(ids) > 1 {
480+
warns = append(warns, fmt.Sprintf("host port %d is mapped to multiple containers on different interfaces: %s", hp, strings.Join(ids, ", ")))
481+
}
419482
}
420483

421-
return out, warns
484+
return outs, warns, nil
422485
}
423486

424487
// convertDockerPort converts a Docker port string to a port number and network
@@ -445,3 +508,12 @@ func convertDockerPort(in string) (uint16, string, error) {
445508
return 0, "", xerrors.Errorf("invalid port format: %s", in)
446509
}
447510
}
511+
512+
// convenience function to check if an IP address is loopback or unspecified
513+
func isLoopbackOrUnspecified(ips string) bool {
514+
nip := net.ParseIP(ips)
515+
if nip == nil {
516+
return false // technically correct, I suppose
517+
}
518+
return nip.IsLoopback() || nip.IsUnspecified()
519+
}

0 commit comments

Comments
 (0)