address code review comments

coder · johnstcn · Feb 10, 2025 · Jan 30, 2025 · Feb 5, 2025 · Feb 5, 2025
commit 9fe24099ff2a1c3ba618b4a8ab1331da01fb4686
diff --git a/agent/api.go b/agent/api.go
@@ -35,7 +35,7 @@ func (a *agent) apiHandler() http.Handler {
 		ignorePorts:   cpy,
 		cacheDuration: cacheDuration,
 	}
-	ch := &containersHandler{
+	ch := &devcontainersHandler{
 		cacheDuration: defaultGetContainersCacheDuration,
 	}
 	promHandler := PrometheusMetricsHandler(a.prometheusRegistry, a.logger)

diff --git a/agent/containers.go b/agent/containers.go
@@ -4,7 +4,9 @@ package agent
 
 import (
 	"context"
+	"errors"
 	"net/http"
+	"slices"
 	"sync"
 	"time"
 
@@ -21,19 +23,29 @@ const (
 	getContainersTimeout              = 5 * time.Second
 )
 
-type containersHandler struct {
+type devcontainersHandler struct {
 	cacheDuration time.Duration
 	cl            ContainerLister
 	clock         quartz.Clock
 
-	mu         sync.Mutex // protects the below
-	containers []codersdk.WorkspaceAgentContainer
+	initLockOnce sync.Once // ensures we don't get a race when initializing lockCh
+	// lockCh protects the below fields. We use a channel instead of a mutex so we
+	// can handle cancellation properly.
+	lockCh     chan struct{}
+	containers *codersdk.WorkspaceAgentListContainersResponse
 	mtime      time.Time
 }
 
-func (ch *containersHandler) handler(rw http.ResponseWriter, r *http.Request) {
+func (ch *devcontainersHandler) handler(rw http.ResponseWriter, r *http.Request) {
 	ct, err := ch.getContainers(r.Context())
 	if err != nil {
+		if errors.Is(err, context.Canceled) {
+			httpapi.Write(r.Context(), rw, http.StatusRequestTimeout, codersdk.Response{
+				Message: "Could not get containers.",
+				Detail:  "Took too long to list containers.",
+			})
+			return
+		}
 		httpapi.Write(r.Context(), rw, http.StatusInternalServerError, codersdk.Response{
 			Message: "Could not get containers.",
 			Detail:  err.Error(),
@@ -44,9 +56,21 @@ func (ch *containersHandler) handler(rw http.ResponseWriter, r *http.Request) {
 	httpapi.Write(r.Context(), rw, http.StatusOK, ct)
 }
 
-func (ch *containersHandler) getContainers(ctx context.Context) ([]codersdk.WorkspaceAgentContainer, error) {
-	ch.mu.Lock()
-	defer ch.mu.Unlock()
+func (ch *devcontainersHandler) getContainers(ctx context.Context) (codersdk.WorkspaceAgentListContainersResponse, error) {
+	ch.initLockOnce.Do(func() {
+		if ch.lockCh == nil {
+			ch.lockCh = make(chan struct{}, 1)
+		}
+	})
+	select {
+	case <-ctx.Done():
+		return codersdk.WorkspaceAgentListContainersResponse{}, ctx.Err()
+	default:
+		ch.lockCh <- struct{}{}
+	}
+	defer func() {
+		<-ch.lockCh
+	}()
 
 	// make zero-value usable
 	if ch.cacheDuration == 0 {
@@ -58,34 +82,42 @@ func (ch *containersHandler) getContainers(ctx context.Context) ([]codersdk.Work
 		ch.cl = &dockerCLIContainerLister{}
 	}
 	if ch.containers == nil {
-		ch.containers = make([]codersdk.WorkspaceAgentContainer, 0)
+		ch.containers = &codersdk.WorkspaceAgentListContainersResponse{}
 	}
 	if ch.clock == nil {
 		ch.clock = quartz.NewReal()
 	}
 
 	now := ch.clock.Now()
 	if now.Sub(ch.mtime) < ch.cacheDuration {
-		cpy := make([]codersdk.WorkspaceAgentContainer, len(ch.containers))
-		copy(cpy, ch.containers)
+		// Return a copy of the cached data to avoid accidental modification by the caller.
+		cpy := codersdk.WorkspaceAgentListContainersResponse{
+			Containers: slices.Clone(ch.containers.Containers),
+		}
 		return cpy, nil
 	}
 
-	cancelCtx, cancelFunc := context.WithTimeout(ctx, getContainersTimeout)
-	defer cancelFunc()
-	updated, err := ch.cl.List(cancelCtx)
+	timeoutCtx, timeoutCancel := context.WithTimeout(ctx, getContainersTimeout)
+	defer timeoutCancel()
+	updated, err := ch.cl.List(timeoutCtx)
 	if err != nil {
-		return nil, xerrors.Errorf("get containers: %w", err)
+		return codersdk.WorkspaceAgentListContainersResponse{}, xerrors.Errorf("get containers: %w", err)
 	}
-	ch.containers = updated
+	ch.containers = &updated
 	ch.mtime = now
 
-	// return a copy
-	cpy := make([]codersdk.WorkspaceAgentContainer, len(ch.containers))
-	copy(cpy, ch.containers)
+	// Return a copy of the cached data to avoid accidental modification by the
+	// caller.
+	cpy := codersdk.WorkspaceAgentListContainersResponse{
+		Containers: slices.Clone(ch.containers.Containers),
+	}
 	return cpy, nil
 }
 
+// ContainerLister is an interface for listing containers visible to the
+// workspace agent.
 type ContainerLister interface {
-	List(ctx context.Context) ([]codersdk.WorkspaceAgentContainer, error)
+	// List returns a list of containers visible to the workspace agent.
+	// This should include running and stopped containers.
+	List(ctx context.Context) (codersdk.WorkspaceAgentListContainersResponse, error)
 }
diff --git a/agent/containers_dockercli.go b/agent/containers_dockercli.go
@@ -1,6 +1,7 @@
 package agent
 
 import (
+	"bufio"
 	"bytes"
 	"context"
 	"encoding/json"
@@ -22,46 +23,68 @@ type dockerCLIContainerLister struct{}
 
 var _ ContainerLister = &dockerCLIContainerLister{}
 
-func (*dockerCLIContainerLister) List(ctx context.Context) ([]codersdk.WorkspaceAgentContainer, error) {
-	var buf bytes.Buffer
+func (*dockerCLIContainerLister) List(ctx context.Context) (codersdk.WorkspaceAgentListContainersResponse, error) {
+	var stdoutBuf, stderrBuf bytes.Buffer
 	// List all container IDs, one per line, with no truncation
 	cmd := exec.CommandContext(ctx, "docker", "ps", "--all", "--quiet", "--no-trunc")
-	cmd.Stdout = &buf
+	cmd.Stdout = &stdoutBuf
+	cmd.Stderr = &stderrBuf
 	if err := cmd.Run(); err != nil {
-		return nil, xerrors.Errorf("run docker ps: %w", err)
+		// TODO(Cian): detect specific errors:
+		// - docker not installed
+		// - docker not running
+		// - no permissions to talk to docker
+		return codersdk.WorkspaceAgentListContainersResponse{}, xerrors.Errorf("run docker ps: %w: %q", err, strings.TrimSpace(stderrBuf.String()))
 	}
 
 	ids := make([]string, 0)
-	for _, line := range strings.Split(buf.String(), "\n") {
-		tmp := strings.TrimSpace(line)
+	scanner := bufio.NewScanner(&stdoutBuf)
+	for scanner.Scan() {
+		tmp := strings.TrimSpace(scanner.Text())
 		if tmp == "" {
 			continue
 		}
 		ids = append(ids, tmp)
 	}
+	if err := scanner.Err(); err != nil {
+		return codersdk.WorkspaceAgentListContainersResponse{}, xerrors.Errorf("scan docker ps output: %w", err)
+	}
 
 	// now we can get the detailed information for each container
 	// Run `docker inspect` on each container ID
-	buf.Reset()
-	execArgs := []string{"inspect"}
-	execArgs = append(execArgs, ids...)
-	cmd = exec.CommandContext(ctx, "docker", execArgs...)
-	cmd.Stdout = &buf
+	stdoutBuf.Reset()
+	stderrBuf.Reset()
+	// nolint: gosec // We are not executing user input, these IDs come from
+	// `docker ps`.
+	cmd = exec.CommandContext(ctx, "docker", append([]string{"inspect"}, ids...)...)
+	cmd.Stdout = &stdoutBuf
+	cmd.Stderr = &stderrBuf
 	if err := cmd.Run(); err != nil {
-		return nil, xerrors.Errorf("run docker inspect: %w", err)
+		return codersdk.WorkspaceAgentListContainersResponse{}, xerrors.Errorf("run docker inspect: %w: %s", err, strings.TrimSpace(stderrBuf.String()))
 	}
 
-	ins := make([]dockerInspect, 0)
-	if err := json.NewDecoder(&buf).Decode(&ins); err != nil {
-		return nil, xerrors.Errorf("decode docker inspect output: %w", err)
+	// NOTE: There is an unavoidable potential race condition where a
+	// container is removed between `docker ps` and `docker inspect`.
+	// In this case, stderr will contain an error message but stdout
+	// 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 := make([]dockerInspect, 0, len(ids))
+	if err := json.NewDecoder(&stdoutBuf).Decode(&ins); err != nil {
+		// However, if we just get invalid JSON, we should absolutely return an error.
+		return codersdk.WorkspaceAgentListContainersResponse{}, xerrors.Errorf("decode docker inspect output: %w", err)
 	}
 
-	out := make([]codersdk.WorkspaceAgentContainer, 0)
-	for _, in := range ins {
-		out = append(out, convertDockerInspect(in))
+	res := codersdk.WorkspaceAgentListContainersResponse{
+		Containers: make([]codersdk.WorkspaceAgentDevcontainer, len(ins)),
+	}
+	for idx, in := range ins {
+		out, warns := convertDockerInspect(in)
+		res.Warnings = append(res.Warnings, warns...)
+		res.Containers[idx] = out
 	}
 
-	return out, nil
+	return res, nil
 }
 
 // To avoid a direct dependency on the Docker API, we use the docker CLI
@@ -104,44 +127,47 @@ func (dis dockerInspectState) String() string {
 	return sb.String()
 }
 
-func convertDockerInspect(in dockerInspect) codersdk.WorkspaceAgentContainer {
-	out := codersdk.WorkspaceAgentContainer{
+func convertDockerInspect(in dockerInspect) (codersdk.WorkspaceAgentDevcontainer, []string) {
+	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),
+		Ports:        make([]codersdk.WorkspaceAgentListeningPort, 0, len(in.Config.ExposedPorts)),
 		Running:      in.State.Running,
 		Status:       in.State.String(),
-		Volumes:      make(map[string]string),
+		Volumes:      make(map[string]string, len(in.Config.Volumes)),
 	}
 
 	// sort the keys for deterministic output
 	portKeys := maps.Keys(in.Config.ExposedPorts)
 	sort.Strings(portKeys)
 	for _, p := range portKeys {
-		port, network, err := convertDockerPort(p)
-		if err != nil {
-			// ignore invalid ports
-			continue
+		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,
+			})
 		}
-		out.Ports = append(out.Ports, codersdk.WorkspaceAgentListeningPort{
-			Network: network,
-			Port:    port,
-		})
 	}
 
 	// sort the keys for deterministic output
 	volKeys := maps.Keys(in.Config.Volumes)
 	sort.Strings(volKeys)
 	for _, k := range volKeys {
-		v0, v1 := convertDockerVolume(k)
-		out.Volumes[v0] = v1
+		if v0, v1, err := convertDockerVolume(k); err != nil {
+			warns = append(warns, err.Error())
+		} else {
+			out.Volumes[v0] = v1
+		}
 	}
 
-	return out
+	return out, warns
 }
 
 // convertDockerPort converts a Docker port string to a port number and network
@@ -151,21 +177,21 @@ func convertDockerInspect(in dockerInspect) codersdk.WorkspaceAgentContainer {
 func convertDockerPort(in string) (uint16, string, error) {
 	parts := strings.Split(in, "/")
 	switch len(parts) {
-	case 0:
-		return 0, "", xerrors.Errorf("invalid port format: %s", in)
 	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
-	default:
+	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)
 	}
 }
 
@@ -175,14 +201,14 @@ func convertDockerPort(in string) (uint16, string, error) {
 // example: "/host/path=/container/path" -> "/host/path", "/container/path"
 //
 //	"/container/path" -> "/container/path", "/container/path"
-func convertDockerVolume(in string) (hostPath, containerPath string) {
+func convertDockerVolume(in string) (hostPath, containerPath string, err error) {
 	parts := strings.Split(in, "=")
 	switch len(parts) {
-	case 0:
-		return in, in
 	case 1:
-		return parts[0], parts[0]
+		return parts[0], parts[0], nil
+	case 2:
+		return parts[0], parts[1], nil
 	default:
-		return parts[0], parts[1]
+		return "", "", xerrors.Errorf("invalid volume format: %s", in)
 	}
 }