runtime: select BuildKit explicitly + fix tar symlink encoding

feature/dockerfile.go

@@ -108,7 +108,6 @@ func GenerateBuildContext(plan BuildPlan, dst string) error {
 // generateDockerfile returns the contents of the Dockerfile that layers
 // the configured features on top of plan.BaseImage. Structure:
 //
-//	# syntax=docker/dockerfile:1.4
 //	ARG _DEV_CONTAINERS_BASE_IMAGE=...
 //	FROM $_DEV_CONTAINERS_BASE_IMAGE AS devcontainer_target
 //	USER root
@@ -119,9 +118,14 @@ func GenerateBuildContext(plan BuildPlan, dst string) error {
 //	LABEL devcontainer.metadata='[...]'
 //	ARG _DEV_CONTAINERS_IMAGE_USER=root
 //	USER $_DEV_CONTAINERS_IMAGE_USER
+//
+// Intentionally no `# syntax=docker/dockerfile:X` directive: nothing
+// in the emitted file needs a non-builtin frontend, and declaring one
+// forces buildkit to pull `docker/dockerfile:*` from a registry —
+// which requires a session to forward credentials (we don't open
+// one) and hangs indefinitely behind broken registry mirrors.
 func generateDockerfile(plan BuildPlan) (string, error) {
 	var b strings.Builder
-	b.WriteString("# syntax=docker/dockerfile:1.4\n")
 	fmt.Fprintf(&b, "ARG _DEV_CONTAINERS_BASE_IMAGE=%s\n", plan.BaseImage)
 	b.WriteString("FROM $_DEV_CONTAINERS_BASE_IMAGE AS devcontainer_target\n\n")
 

feature/dockerfile_test.go

@@ -58,7 +58,6 @@ func TestGenerateBuildContext_WritesExpectedLayout(t *testing.T) {
 	}
 
 	wantSubstrings := []string{
-		"# syntax=docker/dockerfile:1.4",
 		"ARG _DEV_CONTAINERS_BASE_IMAGE=alpine:3.20",
 		"FROM $_DEV_CONTAINERS_BASE_IMAGE",
 		"COPY ./build-context/ /tmp/dc-features/",
@@ -76,6 +75,12 @@ func TestGenerateBuildContext_WritesExpectedLayout(t *testing.T) {
 			t.Errorf("Dockerfile missing %q", want)
 		}
 	}
+	// Frontend directive must not be reintroduced: buildkit treats it as
+	// a hard frontend-pull requirement, which we don't supply credentials
+	// for (no session) and which hangs behind broken registry mirrors.
+	if strings.Contains(string(df), "# syntax=") {
+		t.Errorf("Dockerfile must not declare a syntax= frontend; built-in is sufficient")
+	}
 
 	// Per-feature dirs populated with run.sh, feature.env, install.sh.
 	for _, idx := range []string{"0", "1"} {

runtime/docker/build.go

@@ -11,6 +11,7 @@ import (
 	"path/filepath"
 	"strings"
 
+	"github.com/moby/moby/api/types/build"
 	"github.com/moby/moby/client"
 
 	"github.com/crunchloop/devcontainer/runtime"
@@ -22,6 +23,15 @@ import (
 // Streaming progress messages are mapped onto the events channel as
 // runtime.BuildEvents (drop-on-full). Build failures surface as a
 // non-nil error including any structured error returned by the daemon.
+//
+// BuildKit is required. The classic builder synthesizes one
+// intermediate container per Dockerfile step and routes every
+// container API through the daemon's authorization pipeline, which
+// — behind an authz plugin — turns a sub-second build into a
+// multi-minute one (~140× slowdown observed in production). BuildKit
+// uses a single streaming session and is unaffected. Docker Engine
+// has shipped with BuildKit enabled by default since 23.0 (Feb 2023);
+// requiring it here is in line with the lib's modern-spec stance.
 func (r *Runtime) BuildImage(ctx context.Context, spec runtime.BuildSpec, events chan<- runtime.BuildEvent) (runtime.ImageRef, error) {
 	if spec.ContextPath == "" {
 		return runtime.ImageRef{}, fmt.Errorf("BuildImage: spec.ContextPath required")
@@ -49,6 +59,16 @@ func (r *Runtime) BuildImage(ctx context.Context, spec runtime.BuildSpec, events
 		args[k] = &v
 	}
 
+	// Pre-pull base images so BuildKit can resolve them from the local
+	// image store. BuildKit refuses remote metadata resolution without
+	// an active session ("no active sessions" — the session is the
+	// callback channel for registry credentials, mandatory even for
+	// anonymous public pulls). Pulling via the classic ImagePull API
+	// (which is unaffected by the session requirement) seeds the local
+	// store; BuildKit then picks up the cached manifest instead of
+	// reaching for the registry.
+	prePullBaseImages(ctx, r, spec.ContextPath, dockerfile, spec.Args, events)
+
 	res, err := r.api.ImageBuild(ctx, pr, client.ImageBuildOptions{
 		Dockerfile: dockerfile,
 		Tags:       []string{spec.Tag},
@@ -57,6 +77,7 @@ func (r *Runtime) BuildImage(ctx context.Context, spec runtime.BuildSpec, events
 		CacheFrom:  spec.CacheFrom,
 		NoCache:    spec.NoCache,
 		Remove:     true,
+		Version:    build.BuilderBuildKit,
 	})
 	if err != nil {
 		return runtime.ImageRef{}, fmt.Errorf("ImageBuild: %w", err)
@@ -81,6 +102,22 @@ func (r *Runtime) BuildImage(ctx context.Context, spec runtime.BuildSpec, events
 // streamBuildOutput parses the JSON-line stream from ImageBuild,
 // emitting BuildEvents and returning a non-nil error if the daemon
 // reports a build failure. Closes body before returning.
+//
+// Two response shapes are handled:
+//
+//   - Classic-builder-style records with `stream` (log lines) and
+//     `status` (layer/pull progress) fields. Pre-BuildKit format.
+//
+//   - BuildKit records of the form `{"id":"moby.buildkit.trace",
+//     "aux":"<base64-protobuf>"}` for per-step progress and
+//     `{"id":"moby.image.id","aux":{"ID":"sha256:..."}}` for the
+//     final image. The aux protobuf is buildkit's `SolveStatus` —
+//     decoding requires the buildkit module. We intentionally don't
+//     pull that dep in: per-step progress events are silently dropped
+//     under BuildKit; BuildStart / BuildCompleted (emitted by the
+//     caller and at the end of BuildImage) still fire correctly, and
+//     errors still propagate via `errorDetail` / `error` fields.
+//     A future PR can revisit if vertex-level progress is needed.
 func streamBuildOutput(ctx context.Context, body io.ReadCloser, events chan<- runtime.BuildEvent) error {
 	defer body.Close()
 
@@ -129,12 +166,15 @@ func streamBuildOutput(ctx context.Context, body io.ReadCloser, events chan<- ru
 }
 
 // tarDirectory writes the contents of dir (recursively) into w as a
-// non-gzipped tar archive. Symlinks are followed (their targets are
-// included as regular files) — the build daemon doesn't need our
-// engineering tmp dirs to preserve link semantics.
+// non-gzipped tar archive. Symlinks are preserved as tar TypeSymlink
+// entries with their original target text; the daemon-side BuildKit
+// frontend handles the resolution.
 //
-// Empty / unreadable files are best-effort logged via the returned
-// error rather than silently dropped.
+// Previously this passed an empty link argument to tar.FileInfoHeader
+// for symlinks, producing tar entries with TypeSymlink + empty
+// Linkname. Some downstream tar readers reject those as malformed and
+// abort the build mid-stream — common in compose-primary contexts
+// containing node_modules/.bin/* or similar bin-symlinks.
 func tarDirectory(dir string, w io.Writer) error {
 	tw := tar.NewWriter(w)
 	defer func() { _ = tw.Close() }()
@@ -157,7 +197,16 @@ func tarDirectory(dir string, w io.Writer) error {
 		if err != nil {
 			return err
 		}
-		hdr, err := tar.FileInfoHeader(info, "")
+
+		var link string
+		isSymlink := info.Mode()&os.ModeSymlink != 0
+		if isSymlink {
+			link, err = os.Readlink(path)
+			if err != nil {
+				return fmt.Errorf("readlink %s: %w", path, err)
+			}
+		}
+		hdr, err := tar.FileInfoHeader(info, link)
 		if err != nil {
 			return fmt.Errorf("tar header for %s: %w", path, err)
 		}
@@ -169,7 +218,7 @@ func tarDirectory(dir string, w io.Writer) error {
 		if err := tw.WriteHeader(hdr); err != nil {
 			return err
 		}
-		if d.IsDir() || (info.Mode()&os.ModeSymlink != 0 && hdr.Typeflag != tar.TypeReg) {
+		if d.IsDir() || isSymlink {
 			return nil
 		}
 		f, err := os.Open(path)
@@ -181,3 +230,180 @@ func tarDirectory(dir string, w io.Writer) error {
 		return err
 	})
 }
+
+// prePullBaseImages reads the Dockerfile at contextPath/dockerfile,
+// extracts FROM image references (resolving simple $ARG substitutions
+// against ARG defaults in the file and the spec's BuildArgs
+// overrides), and pulls each that isn't already in the local image
+// store. Best-effort: any pull error is silently ignored — the
+// subsequent ImageBuild surfaces a clearer "failed to resolve" error
+// if the image really can't be obtained, and images that exist only
+// locally (e.g. dc-go-base-*) correctly skip the pull attempt because
+// ImageInspect finds them.
+func prePullBaseImages(ctx context.Context, r *Runtime, contextPath, dockerfile string, buildArgs map[string]string, events chan<- runtime.BuildEvent) {
+	df, err := os.ReadFile(filepath.Join(contextPath, dockerfile))
+	if err != nil {
+		return
+	}
+	for _, ref := range extractBaseImages(string(df), buildArgs) {
+		if _, err := r.api.ImageInspect(ctx, ref); err == nil {
+			continue
+		}
+		_, _ = r.PullImage(ctx, ref, events)
+	}
+}
+
+// extractBaseImages does a naive line-based parse of Dockerfile
+// content, returning the de-duplicated list of resolved FROM image
+// references. Supports:
+//
+//   - Literal FROM lines (`FROM alpine:3.20`, with optional
+//     `AS <stage>` and `--platform=...` flags).
+//   - $VAR / ${VAR} substitution against the file's `ARG VAR=default`
+//     lines, with overrides from buildArgs taking precedence.
+//
+// FROMs whose ref still contains an unresolved `$` after substitution
+// are dropped — we'd rather miss a pre-pull (and let BuildKit surface
+// its native error) than guess wrong. Stage references (`FROM
+// previous-stage AS …`) are also dropped since they're not image
+// refs; we detect them by skipping refs that match a prior AS name.
+func extractBaseImages(content string, buildArgs map[string]string) []string {
+	args := map[string]string{}
+	for k, v := range buildArgs {
+		args[k] = v
+	}
+	stages := map[string]bool{}
+	seen := map[string]bool{}
+	var out []string
+
+	for _, raw := range strings.Split(content, "\n") {
+		line := strings.TrimSpace(raw)
+		if line == "" || strings.HasPrefix(line, "#") {
+			continue
+		}
+		fields := strings.Fields(line)
+		if len(fields) < 2 {
+			continue
+		}
+		kw := strings.ToUpper(fields[0])
+		switch kw {
+		case "ARG":
+			rest := strings.TrimSpace(line[len("ARG"):])
+			if eq := strings.IndexByte(rest, '='); eq > 0 {
+				name := strings.TrimSpace(rest[:eq])
+				val := strings.TrimSpace(rest[eq+1:])
+				// Only set if not already overridden by buildArgs.
+				if _, override := args[name]; !override {
+					args[name] = val
+				}
+			}
+		case "FROM":
+			rest := strings.TrimSpace(line[len("FROM"):])
+			// Drop leading flags (--platform=..., --link, ...).
+			for strings.HasPrefix(rest, "--") {
+				if sp := strings.IndexByte(rest, ' '); sp > 0 {
+					rest = strings.TrimSpace(rest[sp+1:])
+				} else {
+					rest = ""
+				}
+			}
+			// Trim "AS <stage>" suffix, recording the stage name.
+			ref := rest
+			if idx := indexFoldWord(rest, "AS"); idx > 0 {
+				ref = strings.TrimSpace(rest[:idx])
+				stageName := strings.TrimSpace(rest[idx+len("AS"):])
+				if stageName != "" {
+					stages[stageName] = true
+				}
+			}
+			ref = substituteArgs(ref, args)
+			if ref == "" || strings.Contains(ref, "$") {
+				continue
+			}
+			if stages[ref] {
+				continue
+			}
+			if !seen[ref] {
+				seen[ref] = true
+				out = append(out, ref)
+			}
+		}
+	}
+	return out
+}
+
+// indexFoldWord returns the byte index of the first occurrence of
+// `word` in s as a standalone whitespace-delimited token, matching
+// case-insensitively. Returns -1 if not found.
+func indexFoldWord(s, word string) int {
+	wlen := len(word)
+	for i := 0; i+wlen <= len(s); i++ {
+		if !strings.EqualFold(s[i:i+wlen], word) {
+			continue
+		}
+		// Must be at start or preceded by whitespace.
+		if i > 0 && s[i-1] != ' ' && s[i-1] != '\t' {
+			continue
+		}
+		// Must be at end or followed by whitespace.
+		if i+wlen < len(s) && s[i+wlen] != ' ' && s[i+wlen] != '\t' {
+			continue
+		}
+		return i
+	}
+	return -1
+}
+
+// substituteArgs replaces $VAR and ${VAR} occurrences in ref with the
+// corresponding value from args. Leaves unknown vars unsubstituted so
+// the caller can detect and skip refs with unresolved vars.
+func substituteArgs(ref string, args map[string]string) string {
+	var b strings.Builder
+	i := 0
+	for i < len(ref) {
+		if ref[i] != '$' {
+			b.WriteByte(ref[i])
+			i++
+			continue
+		}
+		// $ at end of string — keep as-is.
+		if i+1 >= len(ref) {
+			b.WriteByte('$')
+			i++
+			continue
+		}
+		var name string
+		var consumed int
+		if ref[i+1] == '{' {
+			end := strings.IndexByte(ref[i+2:], '}')
+			if end < 0 {
+				b.WriteByte('$')
+				i++
+				continue
+			}
+			name = ref[i+2 : i+2+end]
+			consumed = 2 + end + 1
+		} else {
+			j := i + 1
+			for j < len(ref) && (isIdentByte(ref[j])) {
+				j++
+			}
+			name = ref[i+1 : j]
+			consumed = j - i
+		}
+		if val, ok := args[name]; ok {
+			b.WriteString(val)
+		} else {
+			b.WriteString(ref[i : i+consumed])
+		}
+		i += consumed
+	}
+	return b.String()
+}
+
+func isIdentByte(b byte) bool {
+	return b == '_' ||
+		(b >= 'a' && b <= 'z') ||
+		(b >= 'A' && b <= 'Z') ||
+		(b >= '0' && b <= '9')
+}