From 20e393770aaba0a308eb9a84f4c382025808d3f0 Mon Sep 17 00:00:00 2001 From: Kacper Sawicki Date: Wed, 11 Feb 2026 13:57:37 +0000 Subject: [PATCH 1/3] fix: substitute devcontainer.json build args into Dockerfile ARG variables (#455) ImageFromDockerfile and UserFromDockerfile now accept external build args from devcontainer.json, allowing them to override ARG defaults and supply values for ARGs without defaults. This fixes the error when a Dockerfile uses ARG VARIANT + FROM ...${VARIANT} with the value provided in devcontainer.json build.args. --- devcontainer/devcontainer.go | 56 +++++++++++++++++++++++++++++-- devcontainer/devcontainer_test.go | 28 ++++++++++++++-- envbuilder.go | 2 +- 3 files changed, 79 insertions(+), 7 deletions(-) diff --git a/devcontainer/devcontainer.go b/devcontainer/devcontainer.go index 0bf7cc35..c002f649 100644 --- a/devcontainer/devcontainer.go +++ b/devcontainer/devcontainer.go @@ -204,7 +204,7 @@ func (s *Spec) Compile(fs billy.Filesystem, devcontainerDir, scratchDir string, // We should make a best-effort attempt to find the user. // Features must be executed as root, so we need to swap back // to the running user afterwards. - params.User, err = UserFromDockerfile(params.DockerfileContent) + params.User, err = UserFromDockerfile(params.DockerfileContent, params.BuildArgs) if err != nil { return nil, fmt.Errorf("user from dockerfile: %w", err) } @@ -308,12 +308,42 @@ func (s *Spec) compileFeatures(fs billy.Filesystem, devcontainerDir, scratchDir // UserFromDockerfile inspects the contents of a provided Dockerfile // and returns the user that will be used to run the container. -func UserFromDockerfile(dockerfileContent string) (user string, err error) { +func UserFromDockerfile(dockerfileContent string, buildArgs []string) (user string, err error) { res, err := parser.Parse(strings.NewReader(dockerfileContent)) if err != nil { return "", fmt.Errorf("parse dockerfile: %w", err) } + // Collect ARG values (defaults + overrides from buildArgs) for + // substitution into FROM image refs. + lexer := shell.NewLex('\\') + var argEnvs []string + for _, child := range res.AST.Children { + if strings.EqualFold(child.Value, "arg") { + arg := strings.TrimSpace(child.Original[len("ARG "):]) + if strings.Contains(arg, "=") { + parts := strings.SplitN(arg, "=", 2) + key := parts[0] + val := parts[1] + // Allow buildArgs to override. + for _, ba := range buildArgs { + if k, v, ok := strings.Cut(ba, "="); ok && k == key { + val = v + break + } + } + argEnvs = append(argEnvs, key+"="+val) + } else { + for _, ba := range buildArgs { + if k, v, ok := strings.Cut(ba, "="); ok && k == arg { + argEnvs = append(argEnvs, k+"="+v) + break + } + } + } + } + } + // Parse stages and user commands to determine the relevant user // from the final stage. var ( @@ -330,6 +360,11 @@ func UserFromDockerfile(dockerfileContent string) (user string, err error) { switch i := inst.(type) { case *instructions.Stage: + // Substitute ARG values in the base image name. + baseName, _, err := lexer.ProcessWord(i.BaseName, shell.EnvsFromSlice(argEnvs)) + if err == nil { + i.BaseName = baseName + } stages = append(stages, i) if i.Name != "" { stageNames[i.Name] = i @@ -388,7 +423,7 @@ func UserFromDockerfile(dockerfileContent string) (user string, err error) { // ImageFromDockerfile inspects the contents of a provided Dockerfile // and returns the image that will be used to run the container. -func ImageFromDockerfile(dockerfileContent string) (name.Reference, error) { +func ImageFromDockerfile(dockerfileContent string, buildArgs []string) (name.Reference, error) { lexer := shell.NewLex('\\') var args []string var imageRef string @@ -408,7 +443,22 @@ func ImageFromDockerfile(dockerfileContent string) (name.Reference, error) { if err != nil { return nil, fmt.Errorf("processing %q: %w", line, err) } + // Allow buildArgs to override Dockerfile ARG defaults. + for _, ba := range buildArgs { + if k, v, ok := strings.Cut(ba, "="); ok && k == key { + val = v + break + } + } args = append(args, key+"="+val) + } else { + // ARG without a default — look up in buildArgs. + for _, ba := range buildArgs { + if k, v, ok := strings.Cut(ba, "="); ok && k == arg { + args = append(args, k+"="+v) + break + } + } } continue } diff --git a/devcontainer/devcontainer_test.go b/devcontainer/devcontainer_test.go index 81d2b63b..a831983e 100644 --- a/devcontainer/devcontainer_test.go +++ b/devcontainer/devcontainer_test.go @@ -213,13 +213,35 @@ func TestImageFromDockerfile(t *testing.T) { tc := tc t.Run(tc.image, func(t *testing.T) { t.Parallel() - ref, err := devcontainer.ImageFromDockerfile(tc.content) + ref, err := devcontainer.ImageFromDockerfile(tc.content, nil) require.NoError(t, err) require.Equal(t, tc.image, ref.Name()) }) } } +func TestImageFromDockerfile_BuildArgs(t *testing.T) { + t.Parallel() + + // Test that build args override ARG defaults. + t.Run("OverridesDefault", func(t *testing.T) { + t.Parallel() + content := "ARG VARIANT=3.10\nFROM mcr.microsoft.com/devcontainers/python:0-${VARIANT}" + ref, err := devcontainer.ImageFromDockerfile(content, []string{"VARIANT=3.11-bookworm"}) + require.NoError(t, err) + require.Equal(t, "mcr.microsoft.com/devcontainers/python:0-3.11-bookworm", ref.Name()) + }) + + // Test that build args supply values for ARGs without defaults. + t.Run("SuppliesArgWithoutDefault", func(t *testing.T) { + t.Parallel() + content := "ARG VARIANT\nFROM mcr.microsoft.com/devcontainers/python:1-${VARIANT}" + ref, err := devcontainer.ImageFromDockerfile(content, []string{"VARIANT=3.11-bookworm"}) + require.NoError(t, err) + require.Equal(t, "mcr.microsoft.com/devcontainers/python:1-3.11-bookworm", ref.Name()) + }) +} + func TestUserFrom(t *testing.T) { t.Parallel() @@ -287,7 +309,7 @@ func TestUserFrom(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() - user, err := devcontainer.UserFromDockerfile(tt.content) + user, err := devcontainer.UserFromDockerfile(tt.content, nil) require.NoError(t, err) require.Equal(t, tt.user, user) }) @@ -364,7 +386,7 @@ FROM a`, content := strings.ReplaceAll(tt.content, "coder/test", strings.TrimPrefix(registry, "http://")+"/coder/test") - user, err := devcontainer.UserFromDockerfile(content) + user, err := devcontainer.UserFromDockerfile(content, nil) require.NoError(t, err) require.Equal(t, tt.user, user) }) diff --git a/envbuilder.go b/envbuilder.go index 34adb9c1..8b019d51 100644 --- a/envbuilder.go +++ b/envbuilder.go @@ -493,7 +493,7 @@ func run(ctx context.Context, opts options.Options, execArgs *execArgsInfo) erro defer cleanupBuildContext() if runtimeData.Built && opts.SkipRebuild { endStage := startStage("🏗️ Skipping build because of cache...") - imageRef, err := devcontainer.ImageFromDockerfile(buildParams.DockerfileContent) + imageRef, err := devcontainer.ImageFromDockerfile(buildParams.DockerfileContent, buildParams.BuildArgs) if err != nil { return nil, fmt.Errorf("image from dockerfile: %w", err) } From bbbbf9846babbad24e3efc0a52b84a634a991659 Mon Sep 17 00:00:00 2001 From: Kacper Sawicki Date: Wed, 18 Feb 2026 14:26:02 +0000 Subject: [PATCH 2/3] refactor: address PR review comments - Change buildArgs parameter from []string to map[string]string - Add exported BuildArgsMap helper for call-site conversion - Use child.Next.Value instead of fragile child.Original slicing - Use strings.Cut instead of strings.Contains + SplitN - Return ProcessWord errors instead of silently swallowing them - Add UserFromDockerfile_BuildArgs test with registry - Add MissingBuildArgUsesEmpty test case --- devcontainer/devcontainer.go | 76 +++++++++++++++---------------- devcontainer/devcontainer_test.go | 40 +++++++++++++++- envbuilder.go | 2 +- 3 files changed, 75 insertions(+), 43 deletions(-) diff --git a/devcontainer/devcontainer.go b/devcontainer/devcontainer.go index c002f649..ea07bfcd 100644 --- a/devcontainer/devcontainer.go +++ b/devcontainer/devcontainer.go @@ -204,7 +204,7 @@ func (s *Spec) Compile(fs billy.Filesystem, devcontainerDir, scratchDir string, // We should make a best-effort attempt to find the user. // Features must be executed as root, so we need to swap back // to the running user afterwards. - params.User, err = UserFromDockerfile(params.DockerfileContent, params.BuildArgs) + params.User, err = UserFromDockerfile(params.DockerfileContent, BuildArgsMap(params.BuildArgs)) if err != nil { return nil, fmt.Errorf("user from dockerfile: %w", err) } @@ -306,9 +306,20 @@ func (s *Spec) compileFeatures(fs billy.Filesystem, devcontainerDir, scratchDir return strings.Join(lines, "\n"), featureContexts, err } +// BuildArgsMap converts a slice of "KEY=VALUE" strings to a map. +func BuildArgsMap(buildArgs []string) map[string]string { + m := make(map[string]string, len(buildArgs)) + for _, arg := range buildArgs { + if key, val, ok := strings.Cut(arg, "="); ok { + m[key] = val + } + } + return m +} + // UserFromDockerfile inspects the contents of a provided Dockerfile // and returns the user that will be used to run the container. -func UserFromDockerfile(dockerfileContent string, buildArgs []string) (user string, err error) { +func UserFromDockerfile(dockerfileContent string, buildArgs map[string]string) (user string, err error) { res, err := parser.Parse(strings.NewReader(dockerfileContent)) if err != nil { return "", fmt.Errorf("parse dockerfile: %w", err) @@ -319,27 +330,18 @@ func UserFromDockerfile(dockerfileContent string, buildArgs []string) (user stri lexer := shell.NewLex('\\') var argEnvs []string for _, child := range res.AST.Children { - if strings.EqualFold(child.Value, "arg") { - arg := strings.TrimSpace(child.Original[len("ARG "):]) - if strings.Contains(arg, "=") { - parts := strings.SplitN(arg, "=", 2) - key := parts[0] - val := parts[1] - // Allow buildArgs to override. - for _, ba := range buildArgs { - if k, v, ok := strings.Cut(ba, "="); ok && k == key { - val = v - break - } - } - argEnvs = append(argEnvs, key+"="+val) - } else { - for _, ba := range buildArgs { - if k, v, ok := strings.Cut(ba, "="); ok && k == arg { - argEnvs = append(argEnvs, k+"="+v) - break - } - } + if !strings.EqualFold(child.Value, "arg") || child.Next == nil { + continue + } + if key, val, ok := strings.Cut(child.Next.Value, "="); ok { + if override, has := buildArgs[key]; has { + val = override + } + argEnvs = append(argEnvs, key+"="+val) + } else { + arg := child.Next.Value + if val, has := buildArgs[arg]; has { + argEnvs = append(argEnvs, arg+"="+val) } } } @@ -362,9 +364,10 @@ func UserFromDockerfile(dockerfileContent string, buildArgs []string) (user stri case *instructions.Stage: // Substitute ARG values in the base image name. baseName, _, err := lexer.ProcessWord(i.BaseName, shell.EnvsFromSlice(argEnvs)) - if err == nil { - i.BaseName = baseName + if err != nil { + return "", fmt.Errorf("processing ARG substitution in FROM %q: %w", i.BaseName, err) } + i.BaseName = baseName stages = append(stages, i) if i.Name != "" { stageNames[i.Name] = i @@ -423,7 +426,7 @@ func UserFromDockerfile(dockerfileContent string, buildArgs []string) (user stri // ImageFromDockerfile inspects the contents of a provided Dockerfile // and returns the image that will be used to run the container. -func ImageFromDockerfile(dockerfileContent string, buildArgs []string) (name.Reference, error) { +func ImageFromDockerfile(dockerfileContent string, buildArgs map[string]string) (name.Reference, error) { lexer := shell.NewLex('\\') var args []string var imageRef string @@ -433,31 +436,24 @@ func ImageFromDockerfile(dockerfileContent string, buildArgs []string) (name.Ref line := lines[i] if arg, ok := strings.CutPrefix(line, "ARG "); ok { arg = strings.TrimSpace(arg) - if strings.Contains(arg, "=") { - parts := strings.SplitN(arg, "=", 2) - key, _, err := lexer.ProcessWord(parts[0], shell.EnvsFromSlice(args)) + if key, val, ok := strings.Cut(arg, "="); ok { + key, _, err := lexer.ProcessWord(key, shell.EnvsFromSlice(args)) if err != nil { return nil, fmt.Errorf("processing %q: %w", line, err) } - val, _, err := lexer.ProcessWord(parts[1], shell.EnvsFromSlice(args)) + val, _, err := lexer.ProcessWord(val, shell.EnvsFromSlice(args)) if err != nil { return nil, fmt.Errorf("processing %q: %w", line, err) } // Allow buildArgs to override Dockerfile ARG defaults. - for _, ba := range buildArgs { - if k, v, ok := strings.Cut(ba, "="); ok && k == key { - val = v - break - } + if override, has := buildArgs[key]; has { + val = override } args = append(args, key+"="+val) } else { // ARG without a default — look up in buildArgs. - for _, ba := range buildArgs { - if k, v, ok := strings.Cut(ba, "="); ok && k == arg { - args = append(args, k+"="+v) - break - } + if val, has := buildArgs[arg]; has { + args = append(args, arg+"="+val) } } continue diff --git a/devcontainer/devcontainer_test.go b/devcontainer/devcontainer_test.go index a831983e..dcc23e12 100644 --- a/devcontainer/devcontainer_test.go +++ b/devcontainer/devcontainer_test.go @@ -227,7 +227,7 @@ func TestImageFromDockerfile_BuildArgs(t *testing.T) { t.Run("OverridesDefault", func(t *testing.T) { t.Parallel() content := "ARG VARIANT=3.10\nFROM mcr.microsoft.com/devcontainers/python:0-${VARIANT}" - ref, err := devcontainer.ImageFromDockerfile(content, []string{"VARIANT=3.11-bookworm"}) + ref, err := devcontainer.ImageFromDockerfile(content, map[string]string{"VARIANT": "3.11-bookworm"}) require.NoError(t, err) require.Equal(t, "mcr.microsoft.com/devcontainers/python:0-3.11-bookworm", ref.Name()) }) @@ -236,10 +236,46 @@ func TestImageFromDockerfile_BuildArgs(t *testing.T) { t.Run("SuppliesArgWithoutDefault", func(t *testing.T) { t.Parallel() content := "ARG VARIANT\nFROM mcr.microsoft.com/devcontainers/python:1-${VARIANT}" - ref, err := devcontainer.ImageFromDockerfile(content, []string{"VARIANT=3.11-bookworm"}) + ref, err := devcontainer.ImageFromDockerfile(content, map[string]string{"VARIANT": "3.11-bookworm"}) require.NoError(t, err) require.Equal(t, "mcr.microsoft.com/devcontainers/python:1-3.11-bookworm", ref.Name()) }) + + // Test that a missing build arg for an ARG without default results + // in the variable being substituted as empty string. + t.Run("MissingBuildArgUsesEmpty", func(t *testing.T) { + t.Parallel() + content := "ARG VARIANT\nFROM mcr.microsoft.com/devcontainers/python:1-${VARIANT}" + ref, err := devcontainer.ImageFromDockerfile(content, nil) + require.NoError(t, err) + require.Equal(t, "mcr.microsoft.com/devcontainers/python:1-", ref.Name()) + }) +} + +func TestUserFromDockerfile_BuildArgs(t *testing.T) { + t.Parallel() + + t.Run("SubstitutesARGInFROM", func(t *testing.T) { + t.Parallel() + registry := registrytest.New(t) + image, err := partial.UncompressedToImage(emptyImage{configFile: &v1.ConfigFile{ + Config: v1.Config{ + User: "testuser", + }, + }}) + require.NoError(t, err) + ref := strings.TrimPrefix(registry, "http://") + "/coder/test:latest" + parsed, err := name.ParseReference(ref) + require.NoError(t, err) + err = remote.Write(parsed, image) + require.NoError(t, err) + + // Dockerfile uses ARG without default for the image ref. + content := fmt.Sprintf("ARG TAG\nFROM %s/coder/test:${TAG}", strings.TrimPrefix(registry, "http://")) + user, err := devcontainer.UserFromDockerfile(content, map[string]string{"TAG": "latest"}) + require.NoError(t, err) + require.Equal(t, "testuser", user) + }) } func TestUserFrom(t *testing.T) { diff --git a/envbuilder.go b/envbuilder.go index 8b019d51..5c1a6910 100644 --- a/envbuilder.go +++ b/envbuilder.go @@ -493,7 +493,7 @@ func run(ctx context.Context, opts options.Options, execArgs *execArgsInfo) erro defer cleanupBuildContext() if runtimeData.Built && opts.SkipRebuild { endStage := startStage("🏗️ Skipping build because of cache...") - imageRef, err := devcontainer.ImageFromDockerfile(buildParams.DockerfileContent, buildParams.BuildArgs) + imageRef, err := devcontainer.ImageFromDockerfile(buildParams.DockerfileContent, devcontainer.BuildArgsMap(buildParams.BuildArgs)) if err != nil { return nil, fmt.Errorf("image from dockerfile: %w", err) } From 57c80b432a2650b1a99cc201b4c1f35591bf5899 Mon Sep 17 00:00:00 2001 From: Kacper Sawicki Date: Thu, 19 Feb 2026 09:17:20 +0000 Subject: [PATCH 3/3] test: remove MissingBuildArgUsesEmpty test The test asserted that an ARG without a default used in FROM would produce an empty substitution (e.g. python:1-), but in practice Docker would error on such an invalid reference. Remove the unrealistic test case. --- devcontainer/devcontainer_test.go | 9 --------- 1 file changed, 9 deletions(-) diff --git a/devcontainer/devcontainer_test.go b/devcontainer/devcontainer_test.go index dcc23e12..97fd7f93 100644 --- a/devcontainer/devcontainer_test.go +++ b/devcontainer/devcontainer_test.go @@ -241,15 +241,6 @@ func TestImageFromDockerfile_BuildArgs(t *testing.T) { require.Equal(t, "mcr.microsoft.com/devcontainers/python:1-3.11-bookworm", ref.Name()) }) - // Test that a missing build arg for an ARG without default results - // in the variable being substituted as empty string. - t.Run("MissingBuildArgUsesEmpty", func(t *testing.T) { - t.Parallel() - content := "ARG VARIANT\nFROM mcr.microsoft.com/devcontainers/python:1-${VARIANT}" - ref, err := devcontainer.ImageFromDockerfile(content, nil) - require.NoError(t, err) - require.Equal(t, "mcr.microsoft.com/devcontainers/python:1-", ref.Name()) - }) } func TestUserFromDockerfile_BuildArgs(t *testing.T) {