Skip to content

Commit 9fe2409

Browse files
johnstcnjohnstcn
committed
address code review comments
1 parent 9342c69 commit 9fe2409

File tree

15 files changed

+325
-117
lines changed

15 files changed

+325
-117
lines changed

‎agent/api.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ func (a *agent) apiHandler() http.Handler {
3535
ignorePorts: cpy,
3636
cacheDuration: cacheDuration,
3737
}
38-
ch := &containersHandler{
38+
ch := &devcontainersHandler{
3939
cacheDuration: defaultGetContainersCacheDuration,
4040
}
4141
promHandler := PrometheusMetricsHandler(a.prometheusRegistry, a.logger)

‎agent/containers.go

Lines changed: 51 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,9 @@ package agent
44

55
import (
66
"context"
7+
"errors"
78
"net/http"
9+
"slices"
810
"sync"
911
"time"
1012

@@ -21,19 +23,29 @@ const (
2123
getContainersTimeout = 5 * time.Second
2224
)
2325

24-
type containersHandler struct {
26+
type devcontainersHandler struct {
2527
cacheDuration time.Duration
2628
cl ContainerLister
2729
clock quartz.Clock
2830

29-
mu sync.Mutex // protects the below
30-
containers []codersdk.WorkspaceAgentContainer
31+
initLockOnce sync.Once // ensures we don't get a race when initializing lockCh
32+
// lockCh protects the below fields. We use a channel instead of a mutex so we
33+
// can handle cancellation properly.
34+
lockCh chan struct{}
35+
containers *codersdk.WorkspaceAgentListContainersResponse
3136
mtime time.Time
3237
}
3338

34-
func (ch *containersHandler) handler(rw http.ResponseWriter, r *http.Request) {
39+
func (ch *devcontainersHandler) handler(rw http.ResponseWriter, r *http.Request) {
3540
ct, err := ch.getContainers(r.Context())
3641
if err != nil {
42+
if errors.Is(err, context.Canceled) {
43+
httpapi.Write(r.Context(), rw, http.StatusRequestTimeout, codersdk.Response{
44+
Message: "Could not get containers.",
45+
Detail: "Took too long to list containers.",
46+
})
47+
return
48+
}
3749
httpapi.Write(r.Context(), rw, http.StatusInternalServerError, codersdk.Response{
3850
Message: "Could not get containers.",
3951
Detail: err.Error(),
@@ -44,9 +56,21 @@ func (ch *containersHandler) handler(rw http.ResponseWriter, r *http.Request) {
4456
httpapi.Write(r.Context(), rw, http.StatusOK, ct)
4557
}
4658

47-
func (ch *containersHandler) getContainers(ctx context.Context) ([]codersdk.WorkspaceAgentContainer, error) {
48-
ch.mu.Lock()
49-
defer ch.mu.Unlock()
59+
func (ch *devcontainersHandler) getContainers(ctx context.Context) (codersdk.WorkspaceAgentListContainersResponse, error) {
60+
ch.initLockOnce.Do(func() {
61+
if ch.lockCh == nil {
62+
ch.lockCh = make(chan struct{}, 1)
63+
}
64+
})
65+
select {
66+
case <-ctx.Done():
67+
return codersdk.WorkspaceAgentListContainersResponse{}, ctx.Err()
68+
default:
69+
ch.lockCh <- struct{}{}
70+
}
71+
defer func() {
72+
<-ch.lockCh
73+
}()
5074

5175
// make zero-value usable
5276
if ch.cacheDuration == 0 {
@@ -58,34 +82,42 @@ func (ch *containersHandler) getContainers(ctx context.Context) ([]codersdk.Work
5882
ch.cl = &dockerCLIContainerLister{}
5983
}
6084
if ch.containers == nil {
61-
ch.containers = make([]codersdk.WorkspaceAgentContainer, 0)
85+
ch.containers = &codersdk.WorkspaceAgentListContainersResponse{}
6286
}
6387
if ch.clock == nil {
6488
ch.clock = quartz.NewReal()
6589
}
6690

6791
now := ch.clock.Now()
6892
if now.Sub(ch.mtime) < ch.cacheDuration {
69-
cpy := make([]codersdk.WorkspaceAgentContainer, len(ch.containers))
70-
copy(cpy, ch.containers)
93+
// Return a copy of the cached data to avoid accidental modification by the caller.
94+
cpy := codersdk.WorkspaceAgentListContainersResponse{
95+
Containers: slices.Clone(ch.containers.Containers),
96+
}
7197
return cpy, nil
7298
}
7399

74-
cancelCtx, cancelFunc := context.WithTimeout(ctx, getContainersTimeout)
75-
defer cancelFunc()
76-
updated, err := ch.cl.List(cancelCtx)
100+
timeoutCtx, timeoutCancel := context.WithTimeout(ctx, getContainersTimeout)
101+
defer timeoutCancel()
102+
updated, err := ch.cl.List(timeoutCtx)
77103
if err != nil {
78-
return nil, xerrors.Errorf("get containers: %w", err)
104+
return codersdk.WorkspaceAgentListContainersResponse{}, xerrors.Errorf("get containers: %w", err)
79105
}
80-
ch.containers = updated
106+
ch.containers = &updated
81107
ch.mtime = now
82108

83-
// return a copy
84-
cpy := make([]codersdk.WorkspaceAgentContainer, len(ch.containers))
85-
copy(cpy, ch.containers)
109+
// Return a copy of the cached data to avoid accidental modification by the
110+
// caller.
111+
cpy := codersdk.WorkspaceAgentListContainersResponse{
112+
Containers: slices.Clone(ch.containers.Containers),
113+
}
86114
return cpy, nil
87115
}
88116

117+
// ContainerLister is an interface for listing containers visible to the
118+
// workspace agent.
89119
type ContainerLister interface {
90-
List(ctx context.Context) ([]codersdk.WorkspaceAgentContainer, error)
120+
// List returns a list of containers visible to the workspace agent.
121+
// This should include running and stopped containers.
122+
List(ctx context.Context) (codersdk.WorkspaceAgentListContainersResponse, error)
91123
}

‎agent/containers_dockercli.go

Lines changed: 68 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package agent
22

33
import (
4+
"bufio"
45
"bytes"
56
"context"
67
"encoding/json"
@@ -22,46 +23,68 @@ type dockerCLIContainerLister struct{}
2223

2324
var _ ContainerLister = &dockerCLIContainerLister{}
2425

25-
func (*dockerCLIContainerLister) List(ctx context.Context) ([]codersdk.WorkspaceAgentContainer, error) {
26-
var buf bytes.Buffer
26+
func (*dockerCLIContainerLister) List(ctx context.Context) (codersdk.WorkspaceAgentListContainersResponse, error) {
27+
var stdoutBuf, stderrBuf bytes.Buffer
2728
// List all container IDs, one per line, with no truncation
2829
cmd := exec.CommandContext(ctx, "docker", "ps", "--all", "--quiet", "--no-trunc")
29-
cmd.Stdout = &buf
30+
cmd.Stdout = &stdoutBuf
31+
cmd.Stderr = &stderrBuf
3032
if err := cmd.Run(); err != nil {
31-
return nil, xerrors.Errorf("run docker ps: %w", err)
33+
// TODO(Cian): detect specific errors:
34+
// - docker not installed
35+
// - docker not running
36+
// - no permissions to talk to docker
37+
return codersdk.WorkspaceAgentListContainersResponse{}, xerrors.Errorf("run docker ps: %w: %q", err, strings.TrimSpace(stderrBuf.String()))
3238
}
3339

3440
ids := make([]string, 0)
35-
for _, line := range strings.Split(buf.String(), "\n") {
36-
tmp := strings.TrimSpace(line)
41+
scanner := bufio.NewScanner(&stdoutBuf)
42+
for scanner.Scan() {
43+
tmp := strings.TrimSpace(scanner.Text())
3744
if tmp == "" {
3845
continue
3946
}
4047
ids = append(ids, tmp)
4148
}
49+
if err := scanner.Err(); err != nil {
50+
return codersdk.WorkspaceAgentListContainersResponse{}, xerrors.Errorf("scan docker ps output: %w", err)
51+
}
4252

4353
// now we can get the detailed information for each container
4454
// Run `docker inspect` on each container ID
45-
buf.Reset()
46-
execArgs := []string{"inspect"}
47-
execArgs = append(execArgs, ids...)
48-
cmd = exec.CommandContext(ctx, "docker", execArgs...)
49-
cmd.Stdout = &buf
55+
stdoutBuf.Reset()
56+
stderrBuf.Reset()
57+
// nolint: gosec // We are not executing user input, these IDs come from
58+
// `docker ps`.
59+
cmd = exec.CommandContext(ctx, "docker", append([]string{"inspect"}, ids...)...)
60+
cmd.Stdout = &stdoutBuf
61+
cmd.Stderr = &stderrBuf
5062
if err := cmd.Run(); err != nil {
51-
return nil, xerrors.Errorf("run docker inspect: %w", err)
63+
return codersdk.WorkspaceAgentListContainersResponse{}, xerrors.Errorf("run docker inspect: %w: %s", err, strings.TrimSpace(stderrBuf.String()))
5264
}
5365

54-
ins := make([]dockerInspect, 0)
55-
if err := json.NewDecoder(&buf).Decode(&ins); err != nil {
56-
return nil, xerrors.Errorf("decode docker inspect output: %w", err)
66+
// NOTE: There is an unavoidable potential race condition where a
67+
// container is removed between `docker ps` and `docker inspect`.
68+
// In this case, stderr will contain an error message but stdout
69+
// will still contain valid JSON. We will just end up missing
70+
// information about the removed container. We could potentially
71+
// log this error, but I'm not sure it's worth it.
72+
ins := make([]dockerInspect, 0, len(ids))
73+
if err := json.NewDecoder(&stdoutBuf).Decode(&ins); err != nil {
74+
// However, if we just get invalid JSON, we should absolutely return an error.
75+
return codersdk.WorkspaceAgentListContainersResponse{}, xerrors.Errorf("decode docker inspect output: %w", err)
5776
}
5877

59-
out := make([]codersdk.WorkspaceAgentContainer, 0)
60-
for _, in := range ins {
61-
out = append(out, convertDockerInspect(in))
78+
res := codersdk.WorkspaceAgentListContainersResponse{
79+
Containers: make([]codersdk.WorkspaceAgentDevcontainer, len(ins)),
80+
}
81+
for idx, in := range ins {
82+
out, warns := convertDockerInspect(in)
83+
res.Warnings = append(res.Warnings, warns...)
84+
res.Containers[idx] = out
6285
}
6386

64-
return out, nil
87+
return res, nil
6588
}
6689

6790
// To avoid a direct dependency on the Docker API, we use the docker CLI
@@ -104,44 +127,47 @@ func (dis dockerInspectState) String() string {
104127
return sb.String()
105128
}
106129

107-
func convertDockerInspect(in dockerInspect) codersdk.WorkspaceAgentContainer {
108-
out := codersdk.WorkspaceAgentContainer{
130+
func convertDockerInspect(in dockerInspect) (codersdk.WorkspaceAgentDevcontainer, []string) {
131+
var warns []string
132+
out := codersdk.WorkspaceAgentDevcontainer{
109133
CreatedAt: in.Created,
110134
// Remove the leading slash from the container name
111135
FriendlyName: strings.TrimPrefix(in.Name, "/"),
112136
ID: in.ID,
113137
Image: in.Config.Image,
114138
Labels: in.Config.Labels,
115-
Ports: make([]codersdk.WorkspaceAgentListeningPort, 0),
139+
Ports: make([]codersdk.WorkspaceAgentListeningPort, 0, len(in.Config.ExposedPorts)),
116140
Running: in.State.Running,
117141
Status: in.State.String(),
118-
Volumes: make(map[string]string),
142+
Volumes: make(map[string]string, len(in.Config.Volumes)),
119143
}
120144

121145
// sort the keys for deterministic output
122146
portKeys := maps.Keys(in.Config.ExposedPorts)
123147
sort.Strings(portKeys)
124148
for _, p := range portKeys {
125-
port, network, err := convertDockerPort(p)
126-
if err != nil {
127-
// ignore invalid ports
128-
continue
149+
if port, network, err := convertDockerPort(p); err != nil {
150+
warns = append(warns, err.Error())
151+
} else {
152+
out.Ports = append(out.Ports, codersdk.WorkspaceAgentListeningPort{
153+
Network: network,
154+
Port: port,
155+
})
129156
}
130-
out.Ports = append(out.Ports, codersdk.WorkspaceAgentListeningPort{
131-
Network: network,
132-
Port: port,
133-
})
134157
}
135158

136159
// sort the keys for deterministic output
137160
volKeys := maps.Keys(in.Config.Volumes)
138161
sort.Strings(volKeys)
139162
for _, k := range volKeys {
140-
v0, v1 := convertDockerVolume(k)
141-
out.Volumes[v0] = v1
163+
if v0, v1, err := convertDockerVolume(k); err != nil {
164+
warns = append(warns, err.Error())
165+
} else {
166+
out.Volumes[v0] = v1
167+
}
142168
}
143169

144-
return out
170+
return out, warns
145171
}
146172

147173
// convertDockerPort converts a Docker port string to a port number and network
@@ -151,21 +177,21 @@ func convertDockerInspect(in dockerInspect) codersdk.WorkspaceAgentContainer {
151177
func convertDockerPort(in string) (uint16, string, error) {
152178
parts := strings.Split(in, "/")
153179
switch len(parts) {
154-
case 0:
155-
return 0, "", xerrors.Errorf("invalid port format: %s", in)
156180
case 1:
157181
// assume it's a TCP port
158182
p, err := strconv.Atoi(parts[0])
159183
if err != nil {
160184
return 0, "", xerrors.Errorf("invalid port format: %s", in)
161185
}
162186
return uint16(p), "tcp", nil
163-
default:
187+
case 2:
164188
p, err := strconv.Atoi(parts[0])
165189
if err != nil {
166190
return 0, "", xerrors.Errorf("invalid port format: %s", in)
167191
}
168192
return uint16(p), parts[1], nil
193+
default:
194+
return 0, "", xerrors.Errorf("invalid port format: %s", in)
169195
}
170196
}
171197

@@ -175,14 +201,14 @@ func convertDockerPort(in string) (uint16, string, error) {
175201
// example: "/host/path=/container/path" -> "/host/path", "/container/path"
176202
//
177203
// "/container/path" -> "/container/path", "/container/path"
178-
func convertDockerVolume(in string) (hostPath, containerPath string) {
204+
func convertDockerVolume(in string) (hostPath, containerPath string, err error) {
179205
parts := strings.Split(in, "=")
180206
switch len(parts) {
181-
case 0:
182-
return in, in
183207
case 1:
184-
return parts[0], parts[0]
208+
return parts[0], parts[0], nil
209+
case 2:
210+
return parts[0], parts[1], nil
185211
default:
186-
return parts[0], parts[1]
212+
return "", "", xerrors.Errorf("invalid volume format: %s", in)
187213
}
188214
}

0 commit comments

Comments
 (0)