Skip to content

Commit c1ca00e

Browse files
mafredrimafredri
committed
fix(devcontainer): parse user in multi-stage builds
Fixes #231
1 parent 9b83cf5 commit c1ca00e

File tree

3 files changed

+281
-41
lines changed

3 files changed

+281
-41
lines changed

‎devcontainer/devcontainer.go

+77-17
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,8 @@ import (
1515
"github.com/go-git/go-billy/v5"
1616
"github.com/google/go-containerregistry/pkg/name"
1717
"github.com/google/go-containerregistry/pkg/v1/remote"
18+
"github.com/moby/buildkit/frontend/dockerfile/instructions"
19+
"github.com/moby/buildkit/frontend/dockerfile/parser"
1820
"github.com/moby/buildkit/frontend/dockerfile/shell"
1921
"github.com/tailscale/hujson"
2022
)
@@ -202,16 +204,9 @@ func (s *Spec) Compile(fs billy.Filesystem, devcontainerDir, scratchDir string,
202204
// We should make a best-effort attempt to find the user.
203205
// Features must be executed as root, so we need to swap back
204206
// to the running user afterwards.
205-
params.User = UserFromDockerfile(params.DockerfileContent)
206-
}
207-
if params.User == "" {
208-
imageRef, err := ImageFromDockerfile(params.DockerfileContent)
207+
params.User, err = UserFromDockerfile(params.DockerfileContent)
209208
if err != nil {
210-
return nil, fmt.Errorf("parse image from dockerfile: %w", err)
211-
}
212-
params.User, err = UserFromImage(imageRef)
213-
if err != nil {
214-
return nil, fmt.Errorf("get user from image: %w", err)
209+
return nil, fmt.Errorf("user from dockerfile: %w", err)
215210
}
216211
}
217212
remoteUser := s.RemoteUser
@@ -313,17 +308,82 @@ func (s *Spec) compileFeatures(fs billy.Filesystem, devcontainerDir, scratchDir
313308

314309
// UserFromDockerfile inspects the contents of a provided Dockerfile
315310
// and returns the user that will be used to run the container.
316-
func UserFromDockerfile(dockerfileContent string) string {
317-
lines := strings.Split(dockerfileContent, "\n")
318-
// Iterate over lines in reverse
319-
for i := len(lines) - 1; i >= 0; i-- {
320-
line := lines[i]
321-
if !strings.HasPrefix(line, "USER ") {
311+
func UserFromDockerfile(dockerfileContent string) (user string, err error) {
312+
res, err := parser.Parse(strings.NewReader(dockerfileContent))
313+
if err != nil {
314+
return "", fmt.Errorf("parse dockerfile: %w", err)
315+
}
316+
317+
// Parse stages and user commands to determine the relevant user
318+
// from the final stage.
319+
var (
320+
stages []*instructions.Stage
321+
stageNames = make(map[string]*instructions.Stage)
322+
stageUser = make(map[*instructions.Stage]*instructions.UserCommand)
323+
currentStage *instructions.Stage
324+
)
325+
for _, child := range res.AST.Children {
326+
inst, err := instructions.ParseInstruction(child)
327+
if err != nil {
328+
return "", fmt.Errorf("parse instruction: %w", err)
329+
}
330+
331+
switch i := inst.(type) {
332+
case *instructions.Stage:
333+
stages = append(stages, i)
334+
if i.Name != "" {
335+
stageNames[i.Name] = i
336+
}
337+
currentStage = i
338+
case *instructions.UserCommand:
339+
if currentStage == nil {
340+
continue
341+
}
342+
stageUser[currentStage] = i
343+
}
344+
}
345+
346+
// Iterate over stages in bottom-up order to find the user,
347+
// skipping any stages not referenced by the final stage.
348+
lookupStage := stages[len(stages)-1]
349+
for i := len(stages) - 1; i >= 0; i-- {
350+
stage := stages[i]
351+
if stage != lookupStage {
322352
continue
323353
}
324-
return strings.TrimSpace(strings.TrimPrefix(line, "USER "))
354+
355+
if user, ok := stageUser[stage]; ok {
356+
return user.User, nil
357+
}
358+
359+
// If we reach the scratch stage, we can't determine the user.
360+
if stage.BaseName == "scratch" {
361+
return "", nil
362+
}
363+
364+
// Check if this FROM references another stage.
365+
if stage.BaseName != "" {
366+
var ok bool
367+
lookupStage, ok = stageNames[stage.BaseName]
368+
if ok {
369+
continue
370+
}
371+
}
372+
373+
// If we can't find a user command, try to find the user from
374+
// the image.
375+
ref, err := name.ParseReference(strings.TrimSpace(stage.BaseName))
376+
if err != nil {
377+
return "", fmt.Errorf("parse image ref %q: %w", stage.BaseName, err)
378+
}
379+
user, err := UserFromImage(ref)
380+
if err != nil {
381+
return "", fmt.Errorf("user from image %s: %w", ref.Name(), err)
382+
}
383+
return user, nil
325384
}
326-
return ""
385+
386+
return "", nil
327387
}
328388

329389
// ImageFromDockerfile inspects the contents of a provided Dockerfile

‎devcontainer/devcontainer_test.go

+147-24
Original file line numberDiff line numberDiff line change
@@ -190,12 +190,6 @@ func TestCompileDevContainer(t *testing.T) {
190190
})
191191
}
192192

193-
func TestUserFromDockerfile(t *testing.T) {
194-
t.Parallel()
195-
user := devcontainer.UserFromDockerfile("FROM ubuntu\nUSER kyle")
196-
require.Equal(t, "kyle", user)
197-
}
198-
199193
func TestImageFromDockerfile(t *testing.T) {
200194
t.Parallel()
201195
for _, tc := range []struct {
@@ -224,27 +218,156 @@ func TestImageFromDockerfile(t *testing.T) {
224218
}
225219
}
226220

227-
func TestUserFromImage(t *testing.T) {
221+
func TestUserFrom(t *testing.T) {
228222
t.Parallel()
229-
registry := registrytest.New(t)
230-
image, err := partial.UncompressedToImage(emptyImage{configFile: &v1.ConfigFile{
231-
Config: v1.Config{
232-
User: "example",
233-
},
234-
}})
235-
require.NoError(t, err)
236223

237-
parsed, err := url.Parse(registry)
238-
require.NoError(t, err)
239-
parsed.Path = "coder/test:latest"
240-
ref, err := name.ParseReference(strings.TrimPrefix(parsed.String(), "http://"))
241-
require.NoError(t, err)
242-
err = remote.Write(ref, image)
243-
require.NoError(t, err)
224+
t.Run("Image", func(t *testing.T) {
225+
t.Parallel()
226+
registry := registrytest.New(t)
227+
image, err := partial.UncompressedToImage(emptyImage{configFile: &v1.ConfigFile{
228+
Config: v1.Config{
229+
User: "example",
230+
},
231+
}})
232+
require.NoError(t, err)
244233

245-
user, err := devcontainer.UserFromImage(ref)
246-
require.NoError(t, err)
247-
require.Equal(t, "example", user)
234+
parsed, err := url.Parse(registry)
235+
require.NoError(t, err)
236+
parsed.Path = "coder/test:latest"
237+
ref, err := name.ParseReference(strings.TrimPrefix(parsed.String(), "http://"))
238+
require.NoError(t, err)
239+
err = remote.Write(ref, image)
240+
require.NoError(t, err)
241+
242+
user, err := devcontainer.UserFromImage(ref)
243+
require.NoError(t, err)
244+
require.Equal(t, "example", user)
245+
})
246+
247+
t.Run("Dockerfile table based test", func(t *testing.T) {
248+
t.Parallel()
249+
tests := []struct {
250+
name string
251+
content string
252+
user string
253+
}{
254+
{
255+
name: "Empty",
256+
content: "FROM scratch",
257+
user: "",
258+
},
259+
{
260+
name: "User",
261+
content: "FROM scrach\nUSER kyle",
262+
user: "kyle",
263+
},
264+
{
265+
name: "Env with default",
266+
content: "FROM scratch\nENV MYUSER=maf\nUSER ${MYUSER}",
267+
user: "${MYUSER}", // This should be "maf" but the current implementation doesn't support this.
268+
},
269+
{
270+
name: "Env var with default",
271+
content: "FROM scratch\nUSER ${MYUSER:-maf}",
272+
user: "${MYUSER:-maf}", // This should be "maf" but the current implementation doesn't support this.
273+
},
274+
{
275+
name: "Arg",
276+
content: "FROM scratch\nARG MYUSER\nUSER ${MYUSER}",
277+
user: "${MYUSER}", // This should be "" or populated but the current implementation doesn't support this.
278+
},
279+
{
280+
name: "Arg with default",
281+
content: "FROM scratch\nARG MYUSER=maf\nUSER ${MYUSER}",
282+
user: "${MYUSER}", // This should be "maf" but the current implementation doesn't support this.
283+
},
284+
}
285+
for _, tt := range tests {
286+
t.Run(tt.name, func(t *testing.T) {
287+
t.Parallel()
288+
user, err := devcontainer.UserFromDockerfile(tt.content)
289+
require.NoError(t, err)
290+
require.Equal(t, tt.user, user)
291+
})
292+
}
293+
})
294+
295+
t.Run("Multi-stage", func(t *testing.T) {
296+
t.Parallel()
297+
298+
registry := registrytest.New(t)
299+
for tag, user := range map[string]string{
300+
"one": "maf",
301+
"two": "fam",
302+
} {
303+
image, err := partial.UncompressedToImage(emptyImage{configFile: &v1.ConfigFile{
304+
Config: v1.Config{
305+
User: user,
306+
},
307+
}})
308+
require.NoError(t, err)
309+
parsed, err := url.Parse(registry)
310+
require.NoError(t, err)
311+
parsed.Path = "coder/test:" + tag
312+
ref, err := name.ParseReference(strings.TrimPrefix(parsed.String(), "http://"))
313+
fmt.Println(ref)
314+
require.NoError(t, err)
315+
err = remote.Write(ref, image)
316+
require.NoError(t, err)
317+
}
318+
319+
tests := []struct {
320+
name string
321+
images map[string]string
322+
content string
323+
user string
324+
}{
325+
{
326+
name: "Single",
327+
content: "FROM coder/test:one",
328+
user: "maf",
329+
},
330+
{
331+
name: "Multi",
332+
content: "FROM ubuntu AS u\nFROM coder/test:two",
333+
user: "fam",
334+
},
335+
{
336+
name: "Multi-2",
337+
content: "FROM coder/test:two AS two\nUSER maffam\nFROM coder/test:one AS one",
338+
user: "maf",
339+
},
340+
{
341+
name: "Multi-3",
342+
content: "FROM coder/test:two AS two\nFROM coder/test:one AS one\nUSER fammaf",
343+
user: "fammaf",
344+
},
345+
{
346+
name: "Multi-4",
347+
content: `FROM ubuntu AS a
348+
USER root
349+
RUN useradd --create-home pickme
350+
USER pickme
351+
FROM a AS other
352+
USER root
353+
RUN useradd --create-home notme
354+
USER notme
355+
FROM a`,
356+
user: "pickme",
357+
},
358+
}
359+
for _, tt := range tests {
360+
t.Run(tt.name, func(t *testing.T) {
361+
t.Parallel()
362+
363+
content := strings.ReplaceAll(tt.content, "coder/test", strings.TrimPrefix(registry, "http://")+"/coder/test")
364+
365+
user, err := devcontainer.UserFromDockerfile(content)
366+
require.NoError(t, err)
367+
require.Equal(t, tt.user, user)
368+
})
369+
}
370+
})
248371
}
249372

250373
type emptyImage struct {

‎integration/integration_test.go

+57
Original file line numberDiff line numberDiff line change
@@ -102,6 +102,63 @@ func TestInitScriptInitCommand(t *testing.T) {
102102
require.NoError(t, ctx.Err(), "init script did not execute for legacy env vars")
103103
}
104104

105+
func TestDanglingBuildStage(t *testing.T) {
106+
t.Parallel()
107+
108+
// Ensures that a Git repository with a devcontainer.json is cloned and built.
109+
srv := gittest.CreateGitServer(t, gittest.Options{
110+
Files: map[string]string{
111+
"devcontainer.json": `{
112+
"name": "Test",
113+
"build": {
114+
"dockerfile": "Dockerfile"
115+
},
116+
}`,
117+
"Dockerfile": fmt.Sprintf(`FROM %s as a
118+
RUN date > /root/date.txt`, testImageUbuntu),
119+
},
120+
})
121+
ctr, err := runEnvbuilder(t, runOpts{env: []string{
122+
envbuilderEnv("GIT_URL", srv.URL),
123+
}})
124+
require.NoError(t, err)
125+
126+
output := execContainer(t, ctr, "cat /date.txt")
127+
require.NotEmpty(t, strings.TrimSpace(output))
128+
}
129+
130+
func TestUserFromMultistage(t *testing.T) {
131+
t.Parallel()
132+
133+
// Ensures that a Git repository with a devcontainer.json is cloned and built.
134+
srv := gittest.CreateGitServer(t, gittest.Options{
135+
Files: map[string]string{
136+
"devcontainer.json": `{
137+
"name": "Test",
138+
"build": {
139+
"dockerfile": "Dockerfile"
140+
},
141+
}`,
142+
"Dockerfile": fmt.Sprintf(`FROM %s AS a
143+
USER root
144+
RUN useradd --create-home pickme
145+
USER pickme
146+
FROM a AS other
147+
USER root
148+
RUN useradd --create-home notme
149+
USER notme
150+
FROM a AS b`, testImageUbuntu),
151+
},
152+
})
153+
ctr, err := runEnvbuilder(t, runOpts{env: []string{
154+
envbuilderEnv("GIT_URL", srv.URL),
155+
}})
156+
require.NoError(t, err)
157+
158+
output := execContainer(t, ctr, "ps aux | awk '/^pickme / {print $1}' | sort -u")
159+
require.Equal(t, "pickme", strings.TrimSpace(output))
160+
}
161+
105162
func TestUidGid(t *testing.T) {
106163
t.Parallel()
107164
t.Run("MultiStage", func(t *testing.T) {

0 commit comments

Comments
 (0)