From 2fd054a7f4eabd7e1484e05386dcc366981ad1fc Mon Sep 17 00:00:00 2001 From: Amp Date: Sat, 22 Aug 2026 06:59:46 +0000 Subject: [PATCH 01/15] Support bounded job container options and volumes Amp-Thread-ID: https://ampcode.com/threads/T-01a02836-fd56-769a-b44a-a7412431fd1d Co-authored-by: Lachlan Donald --- docs/compatibility.md | 10 +- docs/security.md | 9 +- internal/compiler/bundle_test.go | 8 +- internal/compiler/compiler.go | 1 + internal/compiler/compiler_test.go | 4 +- internal/compiler/plan_builder.go | 8 +- internal/containerpolicy/policy.go | 191 ++++++++++++++++++++++++ internal/containerpolicy/policy_test.go | 44 ++++++ internal/plan/plan.go | 27 ++-- internal/plan/plan_test.go | 29 +++- internal/runtime/containers.go | 77 ++++------ internal/runtime/containers_test.go | 72 +++++++++ internal/workflow/model.go | 10 +- internal/workflow/parse.go | 96 +++++++----- internal/workflow/parse_test.go | 29 +++- schemas/job-plan.schema.json | 2 +- 16 files changed, 500 insertions(+), 117 deletions(-) create mode 100644 internal/containerpolicy/policy.go create mode 100644 internal/containerpolicy/policy_test.go diff --git a/docs/compatibility.md b/docs/compatibility.md index 94a7c6ab..b25073be 100644 --- a/docs/compatibility.md +++ b/docs/compatibility.md @@ -645,14 +645,16 @@ services: --health-retries 10 ``` -Services support `image`, `credentials`, `env`, `ports`, `volumes`, `options`, `command`, and `entrypoint`. +Job containers support `image`, `env`, `ports`, and a bounded subset of `volumes` and `options`. Services support `image`, `credentials`, `env`, `ports`, `volumes`, `options`, `command`, and `entrypoint`. - Job container images can use compile-time `github`, `inputs`, `vars`, `strategy`, and `matrix` values. The complete image must resolve to a non-empty string and a valid image reference during compilation. Secrets, `needs`, step outputs, and whole or dynamic contexts are unsupported. +- Job container volumes accept `NAME:/absolute/container/path`, optionally followed by `:ro` or `:rw`. Volume names contain up to 128 letters, digits, periods, underscores, or hyphens and start with a letter or digit. A job can define 32 unique volumes. Host bind mounts, expressions, and targets under `/__w` or `/__buildkite-gha` are unsupported. +- Job container options accept `--cpus`, `--cpuset-cpus`, `--memory` (or `-m`), `--memory-reservation`, `--memory-swap`, `--pids-limit`, and `--shm-size`. Use `--flag value` or `--flag=value`, without quotes or expressions. Each option can appear once. Other Docker options are unsupported because they can override runner-owned lifecycle, isolation, networking, mounts, or privileges. - Service fields can use compile-time `github`, `inputs`, `vars`, `strategy`, and `matrix` values or runtime `needs` outputs. An empty evaluated image skips the service. - A complete non-credential service map can use `${{ fromJSON(needs..outputs.) }}`. Declare credentials statically so the compiler can prove their secret authority. - Credentials accept direct values and `github`, `vars`, `secrets`, or `env` expressions. Passwords pass to `docker login` through standard input. Authentication uses a private per-job Docker configuration and never reads ambient Docker credentials. -- Docker options pass through except `--network` and its `--net` aliases, which GitHub Actions does not support. Options can grant privileges, mount host paths, publish ports, and change resource settings. -- Named, anonymous, and absolute bind volumes are supported. +- Service Docker options pass through except `--network` and its `--net` aliases, which GitHub Actions does not support. Service options can grant privileges, mount host paths, publish ports, and change resource settings. +- Service named, anonymous, and absolute bind volumes are supported. - A job can define 32 services. Each service can define 256 environment entries and 128 ports or volumes. Implicit GHCR authentication is unsupported; provide explicit credentials. Mutable tags resolve at job start. Use a digest when image immutability matters. Job container images must provide `sh` and run the mounted self-contained Linux runtime executable. @@ -661,7 +663,7 @@ Each job uses a private Docker bridge network. Container jobs reach services by A service with a Docker health check must become healthy before steps run. A service without one is ready after it starts. Failures include bounded status, health, port, and log diagnostics. -Cleanup removes the job container, emits masked and bounded service logs, then removes services in declaration order, the network, newly created volumes, and private Docker configuration. Remaining owned resources fail the job. Docker resources are not a security or resource-isolation boundary: the hosted queue must isolate the whole job and enforce host CPU, memory, disk, and network limits. See the [security model](security.md#isolate-the-whole-job). +Cleanup removes the job container, emits masked and bounded service logs, then removes services in declaration order, the network, newly created job and service volumes, and private Docker configuration. A job container volume name that existed before the job is rejected rather than attached. Remaining owned resources fail the job. Docker resources are not a security or resource-isolation boundary: the hosted queue must isolate the whole job and enforce host CPU, memory, disk, and network limits. See the [security model](security.md#isolate-the-whole-job). macOS jobs reject containers, services, Docker actions, and Docker capability. diff --git a/docs/security.md b/docs/security.md index 7e66bced..fbdf631f 100644 --- a/docs/security.md +++ b/docs/security.md @@ -45,9 +45,12 @@ Run untrusted jobs on a queue with: - a clean environment for every job - host-level CPU, memory, disk, and network limits -Service options can grant privileges, mount host paths, and publish ports. The -private Docker network, ownership labels, and cleanup checks reduce accidental -residue. They do not contain hostile code. +Job container options are limited to resource controls. Job container volumes +accept named Docker volumes, not host bind mounts, and cannot replace the +runner-owned workspace or runtime mounts. Service options can grant privileges, +mount host paths, and publish ports. The private Docker network, ownership +labels, and cleanup checks reduce accidental residue. They do not contain +hostile code. On a persistent self-hosted agent, workflow code can read exposed host resources and leave state for later jobs. diff --git a/internal/compiler/bundle_test.go b/internal/compiler/bundle_test.go index 95b29f54..c44799c6 100644 --- a/internal/compiler/bundle_test.go +++ b/internal/compiler/bundle_test.go @@ -68,7 +68,10 @@ jobs: strategy: matrix: image: [node:24, node:25] - container: ${{ matrix.image }} + container: + image: ${{ matrix.image }} + volumes: [cache:/cache:ro] + options: --cpus 2 steps: [{run: true}] `) event := readFile(t, smokePath("events", "push.json")) @@ -84,7 +87,8 @@ jobs: t.Fatalf("container bundle was not deterministic: %#v", first) } for i, image := range []string{"node:24", "node:25"} { - if first.Plans[i].Job.Container == nil || first.Plans[i].Job.Container.Image != image || bytes.Contains(first.Plans[i].Contents, []byte("${{")) { + container := first.Plans[i].Job.Container + if container == nil || container.Image != image || !slices.Equal(container.Volumes, []string{"cache:/cache:ro"}) || container.Options != "--cpus 2" || bytes.Contains(first.Plans[i].Contents, []byte("${{")) { t.Fatalf("plan %d container = %#v", i, first.Plans[i].Job.Container) } } diff --git a/internal/compiler/compiler.go b/internal/compiler/compiler.go index 52f18b35..efc300d4 100644 --- a/internal/compiler/compiler.go +++ b/internal/compiler/compiler.go @@ -476,6 +476,7 @@ func resolveCompileContainer(container *workflow.Container, context expression.C resolved := *container resolved.Env = cloneMap(container.Env) resolved.Ports = append([]string(nil), container.Ports...) + resolved.Volumes = append([]string(nil), container.Volumes...) if strings.Contains(resolved.Image, "${{") { image, err := expression.EvaluateCompileStringTemplate(resolved.Image, context) if err != nil { diff --git a/internal/compiler/compiler_test.go b/internal/compiler/compiler_test.go index 961f0bda..9ea2f19d 100644 --- a/internal/compiler/compiler_test.go +++ b/internal/compiler/compiler_test.go @@ -4173,12 +4173,12 @@ func readFile(t *testing.T, path string) []byte { } func TestCompilePlansEmitV8ForContainers(t *testing.T) { - workflowSource := []byte("on: push\njobs:\n test:\n runs-on: ubuntu-latest\n container: node:24\n services:\n redis: {image: redis:7}\n steps:\n - run: true\n") + workflowSource := []byte("on: push\njobs:\n test:\n runs-on: ubuntu-latest\n container:\n image: node:24\n volumes: ['cache:/cache:ro']\n options: --cpus=2 --memory 1g\n services:\n redis: {image: redis:7}\n steps:\n - run: true\n") plans, err := compileUntrustedPlans("containers.yml", workflowSource, readFile(t, smokePath("events", "push.json")), "0.0.0-test", "sha256:"+strings.Repeat("1", 64), "gha-untrusted") if err != nil { t.Fatal(err) } - if len(plans) != 1 || plans[0].Schema != plan.Schema || plans[0].Container == nil || len(plans[0].Services) != 1 || !slices.Equal(plans[0].RequiredCapabilities, []string{"docker", "network"}) { + if len(plans) != 1 || plans[0].Schema != plan.Schema || plans[0].Container == nil || !slices.Equal(plans[0].Container.Volumes, []string{"cache:/cache:ro"}) || plans[0].Container.Options != "--cpus=2 --memory 1g" || len(plans[0].Services) != 1 || !slices.Equal(plans[0].RequiredCapabilities, []string{"docker", "network"}) { t.Fatalf("container plan = %#v", plans) } } diff --git a/internal/compiler/plan_builder.go b/internal/compiler/plan_builder.go index d51a30d9..4dd4af47 100644 --- a/internal/compiler/plan_builder.go +++ b/internal/compiler/plan_builder.go @@ -631,9 +631,11 @@ func (b planBuilder) lowerPlanJob(instance JobInstance, workflowProgram program. job.RequiresMise = &actions.requiresMise if programJob.Container != nil { job.Container = &plan.Container{ - Image: programJob.Container.Image.Source, - Env: programBindingMap(programJob.Container.Env), - Ports: programSiteSources(programJob.Container.Ports), + Image: programJob.Container.Image.Source, + Env: programBindingMap(programJob.Container.Env), + Ports: programSiteSources(programJob.Container.Ports), + Volumes: programSiteSources(programJob.Container.Volumes), + Options: programJob.Container.Options.Source, } } if programJob.Services.Dynamic != nil { diff --git a/internal/containerpolicy/policy.go b/internal/containerpolicy/policy.go new file mode 100644 index 00000000..33fa54f2 --- /dev/null +++ b/internal/containerpolicy/policy.go @@ -0,0 +1,191 @@ +// Package containerpolicy owns the job-container Docker syntax that workflows +// may control. +package containerpolicy + +import ( + "fmt" + "path" + "regexp" + "strconv" + "strings" +) + +const MaxJobVolumes = 32 + +var ( + volumeNamePattern = regexp.MustCompile(`^[A-Za-z0-9][A-Za-z0-9_.-]{0,127}$`) + sizePattern = regexp.MustCompile(`^[1-9][0-9]*[bBkKmMgG]?$`) + cpusPattern = regexp.MustCompile(`^(?:[0-9]+(?:\.[0-9]+)?|\.[0-9]+)$`) + cpusetPattern = regexp.MustCompile(`^[0-9]+(?:-[0-9]+)?(?:,[0-9]+(?:-[0-9]+)?)*$`) + positivePattern = regexp.MustCompile(`^[1-9][0-9]*$`) +) + +// JobOptions splits options using the GitHub runner's argument rules and +// accepts only bounded resource controls. It returns the exact Docker argv. +func JobOptions(value string) ([]string, error) { + if len(value) > 4096 || hasASCIIControl(value) || strings.Contains(value, "\"") { + return nil, fmt.Errorf("options exceed 4096 bytes or contain a control or quote character") + } + args := ArgumentList(value) + if len(args) > 16 { + return nil, fmt.Errorf("options contain more than 16 arguments") + } + seen := map[string]bool{} + for i := 0; i < len(args); i++ { + option, optionValue, inline := strings.Cut(args[i], "=") + if !inline { + if i+1 == len(args) { + return nil, fmt.Errorf("option %q requires a value", option) + } + i++ + optionValue = args[i] + } + canonical := option + if canonical == "-m" { + canonical = "--memory" + } + if seen[canonical] { + return nil, fmt.Errorf("option %q is repeated", option) + } + seen[canonical] = true + valid := false + switch canonical { + case "--cpus": + cpus, err := strconv.ParseFloat(optionValue, 64) + valid = cpusPattern.MatchString(optionValue) && err == nil && cpus > 0 + case "--cpuset-cpus": + valid = validCPUSet(optionValue) + case "--memory", "--memory-reservation", "--memory-swap", "--shm-size": + valid = sizePattern.MatchString(optionValue) + case "--pids-limit": + valid = positivePattern.MatchString(optionValue) + default: + return nil, fmt.Errorf("option %q is unsupported", option) + } + if !valid { + return nil, fmt.Errorf("option %q has invalid value %q", option, optionValue) + } + } + return args, nil +} + +func validCPUSet(value string) bool { + if !cpusetPattern.MatchString(value) { + return false + } + for item := range strings.SplitSeq(value, ",") { + bounds := strings.Split(item, "-") + if len(bounds) == 1 { + continue + } + first, firstErr := strconv.Atoi(bounds[0]) + last, lastErr := strconv.Atoi(bounds[1]) + if firstErr != nil || lastErr != nil || first > last { + return false + } + } + return true +} + +// ValidateJobVolume accepts a named volume mounted at an absolute container +// path. Host bind mounts and runner-owned workspace/runtime targets are denied. +func ValidateJobVolume(value string) error { + if value == "" || len(value) > 4096 || hasASCIIControl(value) { + return fmt.Errorf("volume is empty, too long, or contains a control character") + } + parts := strings.Split(value, ":") + if len(parts) < 2 || len(parts) > 3 || !volumeNamePattern.MatchString(parts[0]) { + return fmt.Errorf("volume must be NAME:/absolute/container/path[:ro|rw]") + } + target := parts[1] + if !strings.HasPrefix(target, "/") || target != path.Clean(target) || target == "/" { + return fmt.Errorf("volume target %q must be a clean absolute path", target) + } + for _, reserved := range []string{"/__w", "/__buildkite-gha"} { + if target == reserved || strings.HasPrefix(target, reserved+"/") { + return fmt.Errorf("volume target %q overlaps a runner-owned path", target) + } + } + if len(parts) == 3 && parts[2] != "ro" && parts[2] != "rw" { + return fmt.Errorf("volume mode %q is unsupported", parts[2]) + } + return nil +} + +// ValidateJobVolumes applies the bounded list contract used by plans and the +// runtime boundary. +func ValidateJobVolumes(values []string) error { + if len(values) > MaxJobVolumes { + return fmt.Errorf("more than %d volumes", MaxJobVolumes) + } + seen := map[string]bool{} + for _, value := range values { + if seen[value] { + return fmt.Errorf("volume %q is repeated", value) + } + seen[value] = true + if err := ValidateJobVolume(value); err != nil { + return fmt.Errorf("invalid volume %q: %w", value, err) + } + } + return nil +} + +// JobVolumeName returns the source name from a validated job volume. +func JobVolumeName(value string) string { + name, _, _ := strings.Cut(value, ":") + return name +} + +func hasASCIIControl(value string) bool { + return strings.ContainsFunc(value, func(r rune) bool { return r < 0x20 || r == 0x7f }) +} + +// ArgumentList matches the argument splitting used by the pinned +// actions/runner ProcessStartInfo.Arguments path. Single quotes are ordinary +// characters; double quotes group arguments; backslashes only escape quotes. +func ArgumentList(value string) []string { + var args []string + for i := 0; i < len(value); { + for i < len(value) && (value[i] == ' ' || value[i] == '\t') { + i++ + } + if i == len(value) { + break + } + var arg strings.Builder + quoted := false + for i < len(value) { + if !quoted && (value[i] == ' ' || value[i] == '\t') { + break + } + backslashes := 0 + for i < len(value) && value[i] == '\\' { + backslashes++ + i++ + } + copyCharacter := true + if i < len(value) && value[i] == '"' { + if backslashes%2 == 0 { + if quoted && i+1 < len(value) && value[i+1] == '"' { + i++ + } else { + copyCharacter = false + quoted = !quoted + } + } + backslashes /= 2 + } + arg.WriteString(strings.Repeat("\\", backslashes)) + if i == len(value) || !quoted && (value[i] == ' ' || value[i] == '\t') { + break + } + if copyCharacter { + arg.WriteByte(value[i]) + } + i++ + } + args = append(args, arg.String()) + } + return args +} diff --git a/internal/containerpolicy/policy_test.go b/internal/containerpolicy/policy_test.go new file mode 100644 index 00000000..a312bdce --- /dev/null +++ b/internal/containerpolicy/policy_test.go @@ -0,0 +1,44 @@ +package containerpolicy + +import ( + "slices" + "testing" +) + +func TestJobOptionsAcceptedSyntax(t *testing.T) { + value := "--cpus=.5 --cpuset-cpus 0-2,4 -m 512m --memory-reservation=256M --memory-swap 1g --pids-limit=128 --shm-size 64m" + want := []string{"--cpus=.5", "--cpuset-cpus", "0-2,4", "-m", "512m", "--memory-reservation=256M", "--memory-swap", "1g", "--pids-limit=128", "--shm-size", "64m"} + got, err := JobOptions(value) + if err != nil || !slices.Equal(got, want) { + t.Fatalf("JobOptions(%q) = %#v, %v; want %#v", value, got, err, want) + } +} + +func TestJobOptionsRejectsInvalidSyntax(t *testing.T) { + for _, value := range []string{ + "--cpus 0", "--cpus NaN", "--cpuset-cpus 3-1", "--memory -1", "--memory 1t", + "--pids-limit 0", "--shm-size", "--cpus 1 --cpus=2", "--memory 1g extra", + } { + t.Run(value, func(t *testing.T) { + if _, err := JobOptions(value); err == nil { + t.Fatal("JobOptions() accepted invalid syntax") + } + }) + } +} + +func TestValidateJobVolume(t *testing.T) { + for _, value := range []string{"cache:/cache", "cache.v1:/var/cache:ro", "CACHE_1:/data:rw"} { + if err := ValidateJobVolume(value); err != nil { + t.Errorf("ValidateJobVolume(%q) = %v", value, err) + } + } + for _, value := range []string{"/tmp:/data", "cache:data", "cache:/", "cache:/data/../other", "cache:/__w", "cache:/__buildkite-gha/runtime", "cache:/data:z"} { + if err := ValidateJobVolume(value); err == nil { + t.Errorf("ValidateJobVolume(%q) accepted invalid volume", value) + } + } + if err := ValidateJobVolumes([]string{"cache:/one", "cache:/one"}); err == nil { + t.Error("ValidateJobVolumes() accepted a repeated volume") + } +} diff --git a/internal/plan/plan.go b/internal/plan/plan.go index 103df623..2415434b 100644 --- a/internal/plan/plan.go +++ b/internal/plan/plan.go @@ -15,6 +15,7 @@ import ( "github.com/buildkite/buildkite-gha/internal/action/metadata" "github.com/buildkite/buildkite-gha/internal/action/source" + "github.com/buildkite/buildkite-gha/internal/containerpolicy" "github.com/buildkite/buildkite-gha/internal/expression" ) @@ -254,9 +255,11 @@ type Step struct { } type Container struct { - Image string `json:"image"` - Env map[string]string `json:"env,omitempty"` - Ports []string `json:"ports,omitempty"` + Image string `json:"image"` + Env map[string]string `json:"env,omitempty"` + Ports []string `json:"ports,omitempty"` + Volumes []string `json:"volumes,omitempty"` + Options string `json:"options,omitempty"` } type ContainerCredentials struct { @@ -567,7 +570,7 @@ func (job Job) Validate() error { return fmt.Errorf("job containers and services require network capability") } if job.Container != nil { - if err := validateContainer(job.Container.Image, job.Container.Env, job.Container.Ports); err != nil { + if err := validateContainer(*job.Container); err != nil { return fmt.Errorf("job container: %w", err) } } @@ -1201,18 +1204,18 @@ func compareNeedOutput(left, right NeedOutput) int { return strings.Compare(left.Output, right.Output) } -func validateContainer(image string, env map[string]string, ports []string) error { - if !ValidContainerImageReference(image) { +func validateContainer(container Container) error { + if !ValidContainerImageReference(container.Image) { return fmt.Errorf("invalid image reference") } - if err := validateContainerImageEnv(image, env); err != nil { + if err := validateContainerImageEnv(container.Image, container.Env); err != nil { return err } - if len(ports) > 128 { + if len(container.Ports) > 128 { return fmt.Errorf("more than 128 ports") } seen := map[string]bool{} - for _, port := range ports { + for _, port := range container.Ports { if seen[port] { return fmt.Errorf("repeated port %q", port) } @@ -1221,6 +1224,12 @@ func validateContainer(image string, env map[string]string, ports []string) erro return fmt.Errorf("invalid port %q", port) } } + if err := containerpolicy.ValidateJobVolumes(container.Volumes); err != nil { + return err + } + if _, err := containerpolicy.JobOptions(container.Options); err != nil { + return fmt.Errorf("invalid options: %w", err) + } return nil } diff --git a/internal/plan/plan_test.go b/internal/plan/plan_test.go index 919723c0..f6100704 100644 --- a/internal/plan/plan_test.go +++ b/internal/plan/plan_test.go @@ -924,7 +924,7 @@ func validateJobPlanSchema(t *testing.T, encoded []byte) { func TestContainerContract(t *testing.T) { job := validJob() job.RequiredCapabilities = []string{"docker", "network"} - job.Container = &Container{Image: "node:24", Env: map[string]string{"NODE_ENV": "test"}, Ports: []string{"8080"}} + job.Container = &Container{Image: "node:24", Env: map[string]string{"NODE_ENV": "test"}, Ports: []string{"8080"}, Volumes: []string{"cache:/cache:ro"}, Options: "--cpus 2"} job.Services = map[string]ServiceContainer{"database": { Image: "postgres:16", Credentials: &ContainerCredentials{Username: "user", Password: "password"}, @@ -957,7 +957,7 @@ func TestContainerContract(t *testing.T) { if err != nil { t.Fatal(err) } - want := `{"container":{"image":"node:24","env":{"NODE_ENV":"test"},"ports":["8080"]},"services":{"database":{"image":"postgres:16","credentials":{"username":"user","password":"password"},"env":{"POSTGRES_DB":"app"},"ports":["5432:5432"],"volumes":["database:/data"],"options":"--health-retries 5","command":"postgres -c fsync=off","entrypoint":"docker-entrypoint.sh"}}}` + want := `{"container":{"image":"node:24","env":{"NODE_ENV":"test"},"ports":["8080"],"volumes":["cache:/cache:ro"],"options":"--cpus 2"},"services":{"database":{"image":"postgres:16","credentials":{"username":"user","password":"password"},"env":{"POSTGRES_DB":"app"},"ports":["5432:5432"],"volumes":["database:/data"],"options":"--health-retries 5","command":"postgres -c fsync=off","entrypoint":"docker-entrypoint.sh"}}}` if string(wire) != want { t.Fatalf("encoded containers = %s, want %s", wire, want) } @@ -969,7 +969,7 @@ func TestContainerModelFields(t *testing.T) { typeOf reflect.Type fields []string }{ - {name: "job", typeOf: reflect.TypeFor[Container](), fields: []string{"Image:image", "Env:env,omitempty", "Ports:ports,omitempty"}}, + {name: "job", typeOf: reflect.TypeFor[Container](), fields: []string{"Image:image", "Env:env,omitempty", "Ports:ports,omitempty", "Volumes:volumes,omitempty", "Options:options,omitempty"}}, {name: "service", typeOf: reflect.TypeFor[ServiceContainer](), fields: []string{"Image:image", "Credentials:credentials,omitempty", "Env:env,omitempty", "Ports:ports,omitempty", "Volumes:volumes,omitempty", "Options:options,omitempty", "Command:command,omitempty", "Entrypoint:entrypoint,omitempty"}}, } for _, test := range tests { @@ -985,6 +985,27 @@ func TestContainerModelFields(t *testing.T) { } } +func TestJobContainerPlanRejectsUnsafeOptionsAndVolumes(t *testing.T) { + for name, container := range map[string]Container{ + "privileged": {Image: "node:24", Options: "--privileged"}, + "network": {Image: "node:24", Options: "--network=host"}, + "unbounded memory": {Image: "node:24", Options: "--memory-swap -1"}, + "repeated option": {Image: "node:24", Options: "--cpus 1 --cpus=2"}, + "bind mount": {Image: "node:24", Volumes: []string{"/tmp:/data"}}, + "runtime overlap": {Image: "node:24", Volumes: []string{"cache:/__buildkite-gha/runtime"}}, + "unsupported volume": {Image: "node:24", Volumes: []string{"cache:/data:z"}}, + } { + t.Run(name, func(t *testing.T) { + job := validJob() + job.RequiredCapabilities = []string{"docker", "network"} + job.Container = &container + if err := job.Validate(); err == nil { + t.Fatal("Validate() accepted unsafe job container control") + } + }) + } +} + func TestPrerequisiteOutputProjectionContract(t *testing.T) { digest := "sha256:" + strings.Repeat("4", 64) job := validJob() @@ -1198,7 +1219,7 @@ func TestContainerPortGrammarMatchesSchema(t *testing.T) { {"08", false}, {"+80", false}, {"80/udp/tcp", false}, } { t.Run(test.port, func(t *testing.T) { - goValid := validateContainer("node:24", nil, []string{test.port}) == nil + goValid := validateContainer(Container{Image: "node:24", Ports: []string{test.port}}) == nil job := validJob() job.RequiredCapabilities = []string{"docker", "network"} job.Container = &Container{Image: "node:24", Ports: []string{test.port}} diff --git a/internal/runtime/containers.go b/internal/runtime/containers.go index b0dfbaf1..69cce70c 100644 --- a/internal/runtime/containers.go +++ b/internal/runtime/containers.go @@ -18,6 +18,7 @@ import ( "sync" "time" + "github.com/buildkite/buildkite-gha/internal/containerpolicy" "github.com/buildkite/buildkite-gha/internal/expression" "github.com/buildkite/buildkite-gha/internal/plan" ) @@ -88,6 +89,9 @@ func (r Runner) startJobContainerOrdered(ctx context.Context, processor *command if err := validateEnvironmentNames(spec.Env); err != nil { return nil, fmt.Errorf("job container environment: %w", err) } + if err := containerpolicy.ValidateJobVolumes(spec.Volumes); err != nil { + return nil, fmt.Errorf("job container: %w", err) + } } for serviceID, service := range services { if err := validateEnvironmentNames(service.Env); err != nil { @@ -166,12 +170,20 @@ func (r Runner) startJobContainerOrdered(ctx context.Context, processor *command } } workflowFailure = true - if len(services) != 0 { + if len(services) != 0 || spec != nil && len(spec.Volumes) != 0 { volumes, volumeErr := boundedDockerOutput(ctx, env, docker, "volume", "ls", "--quiet") if volumeErr != nil { return nil, fmt.Errorf("snapshot Docker volumes: %w", volumeErr) } b.existingVolumes = lineSet(volumes) + if spec != nil { + for _, volume := range spec.Volumes { + name := containerpolicy.JobVolumeName(volume) + if b.existingVolumes[name] { + return nil, fmt.Errorf("job container volume %q already exists", name) + } + } + } } if spec != nil { if err = r.pullContainerImage(ctx, processor, env, docker, spec.Image); err != nil { @@ -309,14 +321,27 @@ func (r Runner) startJobContainerOrdered(ctx context.Context, processor *command } args = append(args, "--mount", mount) } + options, optionErr := containerpolicy.JobOptions(spec.Options) + if optionErr != nil { + return nil, fmt.Errorf("job container options: %w", optionErr) + } + args = append(args, options...) for _, name := range sortedKeys(spec.Env) { args = append(args, "--env", name+"="+spec.Env[name]) } args = appendPublishedPorts(args, spec.Ports) + for _, volume := range spec.Volumes { + args = append(args, "--volume", volume) + } args = append(args, spec.Image, "-c", "while :; do sleep 3600; done") if _, err = boundedDockerOutput(ctx, env, docker, args...); err != nil { return nil, fmt.Errorf("create job container: %w", err) } + if len(spec.Volumes) != 0 { + if err = b.trackContainerVolumes(ctx, "job container", b.container); err != nil { + return nil, err + } + } if _, err = boundedDockerOutput(ctx, env, docker, "start", b.container); err != nil { return nil, fmt.Errorf("start job container: %w", err) } @@ -439,10 +464,14 @@ func (b *jobContainerBackend) reconcileCreatedService(ctx context.Context, index } func (b *jobContainerBackend) trackServiceVolumes(ctx context.Context, serviceID, reference string) error { + return b.trackContainerVolumes(ctx, fmt.Sprintf("service %q", serviceID), reference) +} + +func (b *jobContainerBackend) trackContainerVolumes(ctx context.Context, subject, reference string) error { const format = `{{range .Mounts}}{{if eq .Type "volume"}}{{println .Name}}{{end}}{{end}}` output, err := boundedDockerOutput(ctx, b.env, b.docker, "inspect", "--format", format, reference) if err != nil { - return fmt.Errorf("inspect service %q volumes: %w", serviceID, err) + return fmt.Errorf("inspect %s volumes: %w", subject, err) } for volume := range lineSet(output) { if !b.existingVolumes[volume] && !slices.Contains(b.ownedVolumes, volume) { @@ -465,49 +494,7 @@ func validateServiceOptions(options []string) error { // actions/runner ProcessStartInfo.Arguments path. Single quotes are ordinary // characters; double quotes group arguments; backslashes only escape quotes. func dockerArgumentList(value string) ([]string, error) { - var args []string - for i := 0; i < len(value); { - for i < len(value) && (value[i] == ' ' || value[i] == '\t') { - i++ - } - if i == len(value) { - break - } - var arg strings.Builder - quoted := false - for i < len(value) { - if !quoted && (value[i] == ' ' || value[i] == '\t') { - break - } - backslashes := 0 - for i < len(value) && value[i] == '\\' { - backslashes++ - i++ - } - copyCharacter := true - if i < len(value) && value[i] == '"' { - if backslashes%2 == 0 { - if quoted && i+1 < len(value) && value[i+1] == '"' { - i++ - } else { - copyCharacter = false - quoted = !quoted - } - } - backslashes /= 2 - } - arg.WriteString(strings.Repeat("\\", backslashes)) - if i == len(value) || !quoted && (value[i] == ' ' || value[i] == '\t') { - break - } - if copyCharacter { - arg.WriteByte(value[i]) - } - i++ - } - args = append(args, arg.String()) - } - return args, nil + return containerpolicy.ArgumentList(value), nil } var dockerPortLine = regexp.MustCompile(`^([0-9]+)/([A-Za-z0-9]+) -> (?:[^:]+|\[[^]]+\]):([0-9]+)$`) diff --git a/internal/runtime/containers_test.go b/internal/runtime/containers_test.go index 1b3068ea..5eb52052 100644 --- a/internal/runtime/containers_test.go +++ b/internal/runtime/containers_test.go @@ -30,6 +30,7 @@ import ( actionintegration "github.com/buildkite/buildkite-gha/internal/action/integration" "github.com/buildkite/buildkite-gha/internal/action/source" "github.com/buildkite/buildkite-gha/internal/compiler" + "github.com/buildkite/buildkite-gha/internal/containerpolicy" "github.com/buildkite/buildkite-gha/internal/expression" "github.com/buildkite/buildkite-gha/internal/plan" ) @@ -872,6 +873,77 @@ func TestRunJobContainerServicesLifecycleAndArguments(t *testing.T) { } } +func TestRunJobContainerOptionsAndVolumesExactArguments(t *testing.T) { + f := newJobDocker(t, "") + w := t.TempDir() + j := jobContainerPlan(t, w, nil) + j.Container.Env = map[string]string{"Z": "last", "A": "first"} + j.Container.Ports = []string{"8080"} + j.Container.Volumes = []string{"cache:/cache:ro"} + j.Container.Options = "--cpus 1.5 --memory=2g --shm-size 64m" + j.Container.Image = "node:24" + if _, err := (Runner{Docker: f.path, RuntimeExecutable: os.Args[0]}).RunJob(t.Context(), j, w); err != nil { + t.Fatal(err) + } + runtimeExecutable, err := filepath.Abs(os.Args[0]) + if err != nil { + t.Fatal(err) + } + for _, call := range f.calls(t) { + if len(call.Args) == 0 || call.Args[0] != "create" || !slices.Contains(call.Args, "node:24") { + continue + } + want := []string{ + "create", "--name", call.Args[2], "--label", "com.buildkite.gha=true", "--label", call.Args[6], "--network", call.Args[8], + "--mount", "type=bind,source=" + w + ",target=" + jobContainerWorkspace, + "--mount", call.Args[12], + "--mount", "type=bind,source=" + runtimeExecutable + ",target=" + jobContainerRuntime + ",readonly", + "--workdir", jobContainerWorkspace, "--entrypoint", "sh", + "--cpus", "1.5", "--memory=2g", "--shm-size", "64m", + "--env", "A=first", "--env", "Z=last", "--publish", "8080", "--volume", "cache:/cache:ro", + "node:24", "-c", "while :; do sleep 3600; done", + } + if !slices.Equal(call.Args, want) { + t.Fatalf("job container create argv = %#v\nwant = %#v", call.Args, want) + } + removed, readErr := os.ReadFile(filepath.Join(f.root, "removed-volumes")) + if readErr != nil || strings.TrimSpace(string(removed)) != "cache" { + t.Fatalf("removed job volumes = %q, %v", removed, readErr) + } + return + } + t.Fatal("job container create call not found") +} + +func TestJobContainerOptionsRejectDockerAuthority(t *testing.T) { + for _, options := range []string{"--privileged", "--network host", "--volume /:/host", "--cap-add SYS_ADMIN", "--device /dev/kvm", "--pid=host", "--entrypoint sh"} { + t.Run(options, func(t *testing.T) { + if _, err := containerpolicy.JobOptions(options); err == nil { + t.Fatal("JobOptions() accepted Docker authority") + } + }) + } +} + +func TestJobContainerRejectsPreexistingNamedVolume(t *testing.T) { + f := newJobDocker(t, "") + if err := os.WriteFile(filepath.Join(f.root, "existing-volumes"), []byte("cache\n"), 0o600); err != nil { + t.Fatal(err) + } + _, err := (Runner{Docker: f.path, RuntimeExecutable: os.Args[0]}).startJobContainer( + t.Context(), newCommandProcessor(io.Discard, io.Discard), t.TempDir(), t.TempDir(), + &plan.Container{Image: "node:24", Volumes: []string{"cache:/cache"}}, nil, + ) + if err == nil || !strings.Contains(err.Error(), `job container volume "cache" already exists`) { + t.Fatalf("startJobContainer() error = %v", err) + } + for _, call := range f.calls(t) { + if len(call.Args) != 0 && call.Args[0] == "create" { + t.Fatalf("pre-existing volume reached docker create: %#v", call.Args) + } + } +} + func TestRunServiceContainerAutoRemoveCleanup(t *testing.T) { f := newJobDocker(t, "service-auto-remove") b, err := (Runner{Docker: f.path}).startJobContainer(t.Context(), newCommandProcessor(io.Discard, io.Discard), t.TempDir(), t.TempDir(), nil, map[string]plan.ServiceContainer{"database": {Image: "postgres:16", Options: "--rm"}}) diff --git a/internal/workflow/model.go b/internal/workflow/model.go index d5ec557f..5ec283b5 100644 --- a/internal/workflow/model.go +++ b/internal/workflow/model.go @@ -152,10 +152,12 @@ type Job struct { // Container is the statically owned subset of a GitHub Actions container. type Container struct { - Image string `json:"image"` - Env map[string]string `json:"env,omitempty"` - Ports []string `json:"ports,omitempty"` - Span Span `json:"span"` + Image string `json:"image"` + Env map[string]string `json:"env,omitempty"` + Ports []string `json:"ports,omitempty"` + Volumes []string `json:"volumes,omitempty"` + Options string `json:"options,omitempty"` + Span Span `json:"span"` } // Service is a named service container. Services retain workflow declaration diff --git a/internal/workflow/parse.go b/internal/workflow/parse.go index 94f983be..432c9cc6 100644 --- a/internal/workflow/parse.go +++ b/internal/workflow/parse.go @@ -11,6 +11,7 @@ import ( "strings" actionsource "github.com/buildkite/buildkite-gha/internal/action/source" + "github.com/buildkite/buildkite-gha/internal/containerpolicy" "github.com/buildkite/buildkite-gha/internal/expression" "github.com/buildkite/buildkite-gha/internal/plan" "github.com/rhysd/actionlint" @@ -55,7 +56,7 @@ func Parse(path string, source []byte) (*Workflow, error) { if err := yaml.Unmarshal(source, &document); err != nil { return nil, fmt.Errorf("%s: parse workflow YAML: %w", path, err) } - serviceContainers, containerDiagnostics, err := validateRawContainers(path, &document) + rawContainers, containerDiagnostics, err := validateRawContainers(path, &document) if err != nil { return nil, err } @@ -191,7 +192,7 @@ func Parse(path string, source []byte) (*Workflow, error) { } sort.Strings(ids) for _, id := range ids { - job, err := adaptJob(path, parsed.Jobs[id], scalars, concurrency.Steps, serviceContainers) + job, err := adaptJob(path, parsed.Jobs[id], scalars, concurrency.Steps, rawContainers) if err != nil { return nil, err } @@ -332,7 +333,7 @@ var containerPortPattern = regexp.MustCompile(`^(?:[1-9][0-9]{0,3}|[1-5][0-9]{4} // actionlint normalizes maps (including service IDs), so the raw tree is also // the authoritative source for diagnostics. func validateRawContainers(path string, document *yaml.Node) (map[string]rawServiceContainer, []expectedActionlintDiagnostic, error) { - serviceContainers := map[string]rawServiceContainer{} + rawContainers := map[string]rawServiceContainer{} var diagnostics []expectedActionlintDiagnostic root := document if root.Kind == yaml.DocumentNode && len(root.Content) != 0 { @@ -340,15 +341,17 @@ func validateRawContainers(path string, document *yaml.Node) (map[string]rawServ } jobs := mappingValue(root, "jobs") if jobs == nil || jobs.Kind != yaml.MappingNode { - return serviceContainers, diagnostics, nil + return rawContainers, diagnostics, nil } for i := 0; i+1 < len(jobs.Content); i += 2 { jobID := strings.ToLower(jobs.Content[i].Value) job := jobs.Content[i+1] if container := mappingValue(job, "container"); container != nil { - if _, _, err := validateRawContainer(path, container, false); err != nil { + extra, _, err := validateRawContainer(path, container, false) + if err != nil { return nil, nil, err } + rawContainers[jobID+"\x00"] = extra } services := mappingValue(job, "services") if services == nil || services.Kind != yaml.MappingNode { @@ -367,11 +370,11 @@ func validateRawContainers(path string, document *yaml.Node) (map[string]rawServ return nil, nil, err } extra.Order = j / 2 - serviceContainers[jobID+"\x00"+name.Value] = extra + rawContainers[jobID+"\x00"+name.Value] = extra diagnostics = append(diagnostics, expected...) } } - return serviceContainers, diagnostics, nil + return rawContainers, diagnostics, nil } func rawError(path string, node *yaml.Node, message string) error { @@ -423,7 +426,7 @@ func validateRawContainer(path string, node *yaml.Node, service bool) (rawServic image := node if node.Kind == yaml.MappingNode { image = mappingValue(node, "image") - controls := []string{"credentials", "volumes", "options", "command", "entrypoint"} + controls := []string{"credentials", "command", "entrypoint"} if service { controls = nil } @@ -514,27 +517,55 @@ func validateRawContainer(path string, node *yaml.Node, service bool) (rawServic return extra, nil, rawError(path, port, "invalid or repeated container port") } seen[port.Value] = true - if service { - extra.Ports = append(extra.Ports, port.Value) - } + extra.Ports = append(extra.Ports, port.Value) } } - if service && node.Kind == yaml.MappingNode { + if node.Kind == yaml.MappingNode { volumes := mappingValue(node, "volumes") if volumes != nil { - if volumes.Kind != yaml.SequenceNode || len(volumes.Content) > 128 { - return extra, nil, rawError(path, volumes, "invalid service container volumes") + limit := 128 + subject := "service container" + if !service { + limit = containerpolicy.MaxJobVolumes + subject = "container" } + if volumes.Kind != yaml.SequenceNode || len(volumes.Content) > limit { + return extra, nil, rawError(path, volumes, "invalid "+subject+" volumes") + } + seen := map[string]bool{} for _, volume := range volumes.Content { - if volume.Kind != yaml.ScalarNode || len(volume.Value) > 4096 || strings.ContainsAny(volume.Value, "\x00\r\n") { - return extra, nil, rawError(path, volume, "invalid service container volume") + if volume.Kind != yaml.ScalarNode || len(volume.Value) > 4096 || strings.ContainsAny(volume.Value, "\x00\r\n") || seen[volume.Value] { + return extra, nil, rawError(path, volume, "invalid or repeated "+subject+" volume") + } + if !service { + if strings.Contains(volume.Value, "${{") { + return extra, nil, rawError(path, volume, "expression-valued container volume is unsupported") + } + if err := containerpolicy.ValidateJobVolume(volume.Value); err != nil { + return extra, nil, rawError(path, volume, "invalid container volume: "+err.Error()) + } } + seen[volume.Value] = true extra.Volumes = append(extra.Volumes, volume.Value) } } if options := mappingValue(node, "options"); options != nil { - if options.Kind != yaml.ScalarNode || len(options.Value) > 65536 || strings.ContainsAny(options.Value, "\x00\r\n") { - return extra, nil, rawError(path, options, "invalid service container options") + limit := 65536 + subject := "service container" + if !service { + limit = 4096 + subject = "container" + } + if options.Kind != yaml.ScalarNode || len(options.Value) > limit || strings.ContainsAny(options.Value, "\x00\r\n") { + return extra, nil, rawError(path, options, "invalid "+subject+" options") + } + if !service { + if strings.Contains(options.Value, "${{") { + return extra, nil, rawError(path, options, "expression-valued container options are unsupported") + } + if _, err := containerpolicy.JobOptions(options.Value); err != nil { + return extra, nil, rawError(path, options, "invalid container options: "+err.Error()) + } } extra.Options = options.Value } @@ -542,7 +573,7 @@ func validateRawContainer(path string, node *yaml.Node, service bool) (rawServic return extra, diagnostics, nil } -func adaptJob(path string, in *actionlint.Job, scalars map[Position]any, concurrency map[Position]stepConcurrency, serviceContainers map[string]rawServiceContainer) (Job, error) { +func adaptJob(path string, in *actionlint.Job, scalars map[Position]any, concurrency map[Position]stepConcurrency, rawContainers map[string]rawServiceContainer) (Job, error) { out := Job{ID: in.ID.Value, Span: pointSpan(in.Pos)} if in.Environment != nil { return Job{}, locatedError(path, in.Environment.Pos, fmt.Sprintf("job %q", in.ID.Value), "GitHub environments and environment secrets are unsupported") @@ -593,7 +624,7 @@ func adaptJob(path string, in *actionlint.Job, scalars map[Position]any, concurr out.IfSpan = spanFrom(in.If.Pos, in.If.Value) } if in.Container != nil { - container, err := adaptContainer(path, in.ID.Value, in.Container) + container, err := adaptContainer(path, in.ID.Value, in.Container, rawContainers[strings.ToLower(in.ID.Value)+"\x00"]) if err != nil { return Job{}, err } @@ -611,8 +642,8 @@ func adaptJob(path string, in *actionlint.Job, scalars map[Position]any, concurr names = append(names, name) } sort.Slice(names, func(i, j int) bool { - left := serviceContainers[strings.ToLower(in.ID.Value)+"\x00"+names[i]].Order - right := serviceContainers[strings.ToLower(in.ID.Value)+"\x00"+names[j]].Order + left := rawContainers[strings.ToLower(in.ID.Value)+"\x00"+names[i]].Order + right := rawContainers[strings.ToLower(in.ID.Value)+"\x00"+names[j]].Order if left != right { return left < right } @@ -623,7 +654,7 @@ func adaptJob(path string, in *actionlint.Job, scalars map[Position]any, concurr if service == nil || service.Name == nil || service.Container == nil || !serviceIDPattern.MatchString(service.Name.Value) { return Job{}, locatedError(path, in.Services.Pos, in.ID.Value, fmt.Sprintf("invalid service ID %q", name)) } - container, err := adaptServiceContainer(path, in.ID.Value, service.Container, serviceContainers[strings.ToLower(in.ID.Value)+"\x00"+service.Name.Value]) + container, err := adaptServiceContainer(path, in.ID.Value, service.Container, rawContainers[strings.ToLower(in.ID.Value)+"\x00"+service.Name.Value]) if err != nil { return Job{}, err } @@ -936,19 +967,13 @@ func adaptEnv(in *actionlint.Env) map[string]string { var serviceIDPattern = regexp.MustCompile(`^[a-z_][a-z0-9_-]*$`) -func adaptContainer(path, jobID string, in *actionlint.Container) (Container, error) { +func adaptContainer(path, jobID string, in *actionlint.Container, raw rawServiceContainer) (Container, error) { if in.Image == nil || strings.TrimSpace(in.Image.Value) == "" { return Container{}, locatedError(path, in.Pos, jobID, "container image must be non-empty") } if in.Credentials != nil { return Container{}, locatedError(path, in.Credentials.Pos, jobID, "container credentials are unsupported") } - if len(in.Volumes) != 0 { - return Container{}, locatedError(path, in.Volumes[0].Pos, jobID, "container volumes are unsupported") - } - if in.Options != nil { - return Container{}, locatedError(path, in.Options.Pos, jobID, "container options are unsupported") - } if in.Env != nil && in.Env.Expression != nil { return Container{}, locatedError(path, in.Env.Expression.Pos, jobID, "expression-valued container env is unsupported") } @@ -959,12 +984,11 @@ func adaptContainer(path, jobID string, in *actionlint.Container) (Container, er } } } - out := Container{Image: in.Image.Value, Env: adaptEnv(in.Env), Span: pointSpan(in.Pos)} - for _, port := range in.Ports { - if port.ContainsExpression() { - return Container{}, locatedError(path, port.Pos, jobID, "expression-valued container port is unsupported") - } - out.Ports = append(out.Ports, port.Value) + // actionlint v1.7.12 assigns volume nodes to Container.Ports. Keep the raw + // owned fields authoritative until that dependency behavior changes. + out := Container{ + Image: in.Image.Value, Env: adaptEnv(in.Env), Ports: raw.Ports, + Volumes: raw.Volumes, Options: raw.Options, Span: pointSpan(in.Pos), } return out, nil } diff --git a/internal/workflow/parse_test.go b/internal/workflow/parse_test.go index cc6aba4b..28b79aba 100644 --- a/internal/workflow/parse_test.go +++ b/internal/workflow/parse_test.go @@ -4,6 +4,7 @@ import ( "os" "path/filepath" "reflect" + "slices" "strings" "testing" ) @@ -251,13 +252,13 @@ func TestParseRejectsJobPermissionAliases(t *testing.T) { } func TestParseOwnsLiteralContainersInDeclarationOrder(t *testing.T) { - source := []byte("on: push\njobs:\n test:\n runs-on: ubuntu-latest\n container:\n image: node:24\n env: {NODE_ENV: test}\n ports: [8080]\n services:\n zed: {image: redis:7}\n alpha: {image: 'registry.example:5000/team/postgres:16', ports: ['5432:5432']}\n steps:\n - run: true\n") + source := []byte("on: push\njobs:\n test:\n runs-on: ubuntu-latest\n container:\n image: node:24\n env: {NODE_ENV: test}\n ports: [8080]\n volumes: ['cache:/cache:ro']\n options: --cpus 2 --memory=1g\n services:\n zed: {image: redis:7}\n alpha: {image: 'registry.example:5000/team/postgres:16', ports: ['5432:5432']}\n steps:\n - run: true\n") parsed, err := Parse("containers.yml", source) if err != nil { t.Fatal(err) } job := parsed.Jobs[0] - if job.Container == nil || job.Container.Image != "node:24" || job.Container.Env["NODE_ENV"] != "test" || len(job.Services) != 2 || job.Services[0].Name != "zed" || job.Services[1].Name != "alpha" || job.Services[1].Container.Image != "registry.example:5000/team/postgres:16" { + if job.Container == nil || job.Container.Image != "node:24" || job.Container.Env["NODE_ENV"] != "test" || !slices.Equal(job.Container.Volumes, []string{"cache:/cache:ro"}) || job.Container.Options != "--cpus 2 --memory=1g" || len(job.Services) != 2 || job.Services[0].Name != "zed" || job.Services[1].Name != "alpha" || job.Services[1].Container.Image != "registry.example:5000/team/postgres:16" { t.Fatalf("owned containers = %#v / %#v", job.Container, job.Services) } } @@ -346,8 +347,8 @@ func TestParseRejectsExpressionValuedServiceContainerEnvironment(t *testing.T) { func TestParseRejectsUnsupportedContainerControls(t *testing.T) { for name, body := range map[string]string{ "credentials": "credentials: {username: me, password: secret}", - "volumes": "volumes: ['/tmp:/tmp']", - "options": "options: --privileged", + "command": "command: sleep 1", + "entrypoint": "entrypoint: sh", } { t.Run(name, func(t *testing.T) { source := []byte("on: push\njobs:\n test:\n runs-on: ubuntu-latest\n container:\n image: node:24\n " + body + "\n steps:\n - run: true\n") @@ -358,6 +359,26 @@ func TestParseRejectsUnsupportedContainerControls(t *testing.T) { } } +func TestParseRejectsUnsafeJobContainerOptionsAndVolumes(t *testing.T) { + for name, body := range map[string]string{ + "privileged option": "options: --privileged", + "network option": "options: --network host", + "entrypoint option": "options: --entrypoint sh", + "option expression": "options: --cpus ${{ matrix.cpus }}", + "host bind": "volumes: ['/tmp:/data']", + "workspace overlap": "volumes: ['cache:/__w/repo']", + "volume expression": "volumes: ['${{ matrix.name }}:/data']", + "unsupported volume mode": "volumes: ['cache:/data:z']", + } { + t.Run(name, func(t *testing.T) { + source := []byte("on: push\njobs:\n test:\n runs-on: ubuntu-latest\n container:\n image: node:24\n " + body + "\n steps: [{run: true}]\n") + if _, err := Parse("containers.yml", source); err == nil { + t.Fatal("Parse() accepted unsafe job container control") + } + }) + } +} + func TestContainerValidationIsScopedAndSourceLocated(t *testing.T) { unrelated := []byte("on: push\njobs:\n test:\n runs-on: ubuntu-latest\n steps:\n - uses: owner/action@v1\n with: {image: node:24, options: --privileged}\n") if _, err := Parse("scoped.yml", unrelated); err != nil { diff --git a/schemas/job-plan.schema.json b/schemas/job-plan.schema.json index e582fd35..8d6595b1 100644 --- a/schemas/job-plan.schema.json +++ b/schemas/job-plan.schema.json @@ -117,7 +117,7 @@ {"if": {"properties": {"source": {"const": "github"}}, "required": ["source"]}, "then": {"required": ["repository", "requested_ref", "commit"]}} ] }, - "container": {"type": "object", "additionalProperties": false, "required": ["image"], "properties": {"image": {"$ref": "#/$defs/image"}, "env": {"$ref": "#/$defs/containerEnv"}, "ports": {"type": "array", "maxItems": 128, "uniqueItems": true, "items": {"type": "string", "pattern": "^(?:[1-9][0-9]{0,3}|[1-5][0-9]{4}|6[0-4][0-9]{3}|65[0-4][0-9]{2}|655[0-2][0-9]|6553[0-5])(?::(?:[1-9][0-9]{0,3}|[1-5][0-9]{4}|6[0-4][0-9]{3}|65[0-4][0-9]{2}|655[0-2][0-9]|6553[0-5]))?(?:/(?:tcp|udp))?$"}}}}, + "container": {"type": "object", "additionalProperties": false, "required": ["image"], "properties": {"image": {"$ref": "#/$defs/image"}, "env": {"$ref": "#/$defs/containerEnv"}, "ports": {"type": "array", "maxItems": 128, "uniqueItems": true, "items": {"type": "string", "pattern": "^(?:[1-9][0-9]{0,3}|[1-5][0-9]{4}|6[0-4][0-9]{3}|65[0-4][0-9]{2}|655[0-2][0-9]|6553[0-5])(?::(?:[1-9][0-9]{0,3}|[1-5][0-9]{4}|6[0-4][0-9]{3}|65[0-4][0-9]{2}|655[0-2][0-9]|6553[0-5]))?(?:/(?:tcp|udp))?$"}}, "volumes": {"type": "array", "maxItems": 32, "uniqueItems": true, "items": {"type": "string", "minLength": 3, "maxLength": 4096, "pattern": "^[^\\x00-\\x1f\\x7f]+$"}}, "options": {"type": "string", "maxLength": 4096, "pattern": "^[^\\x00-\\x1f\\x7f\\\"]*$"}}}, "serviceContainer": {"type": "object", "additionalProperties": false, "required": ["image"], "properties": {"image": {"type": "string", "minLength": 1, "maxLength": 512}, "credentials": {"type": "object", "additionalProperties": false, "required": ["username", "password"], "properties": {"username": {"type": "string", "maxLength": 65536}, "password": {"type": "string", "maxLength": 65536}}}, "env": {"$ref": "#/$defs/containerEnv"}, "ports": {"type": "array", "maxItems": 128, "uniqueItems": true, "items": {"type": "string", "minLength": 1, "maxLength": 4096, "pattern": "^[^\\x00-\\x1f\\x7f]+$"}}, "volumes": {"type": "array", "maxItems": 128, "uniqueItems": true, "items": {"type": "string", "minLength": 1, "maxLength": 4096, "pattern": "^[^\\x00-\\x1f\\x7f]+$"}}, "options": {"type": "string", "maxLength": 65536}, "command": {"type": "string", "maxLength": 65536}, "entrypoint": {"type": "string", "maxLength": 4096}}}, "containerEnv": {"type": "object", "maxProperties": 256, "propertyNames": {"pattern": "^[A-Za-z_][A-Za-z0-9_]{0,254}$"}, "additionalProperties": {"type": "string", "maxLength": 65536}}, "image": {"type": "string", "minLength": 1, "maxLength": 512, "pattern": "^(?:(?:[a-z0-9]+(?:[._-][a-z0-9]+)*|\\[[0-9a-f:]+\\])(?::(?:[1-9][0-9]{0,3}|[1-5][0-9]{4}|6[0-4][0-9]{3}|65[0-4][0-9]{2}|655[0-2][0-9]|6553[0-5]))?/)?[a-z0-9]+(?:[._-][a-z0-9]+)*(?:/[a-z0-9]+(?:[._-][a-z0-9]+)*)*(?::[A-Za-z0-9_][A-Za-z0-9_.-]{0,127})?(?:@sha256:[0-9a-f]{64})?$"}, From dd79972c9756d344fcc8676941b29aaa496735e1 Mon Sep 17 00:00:00 2001 From: Amp Date: Sat, 22 Aug 2026 07:08:48 +0000 Subject: [PATCH 02/15] Clean volumes after ambiguous container creation Amp-Thread-ID: https://ampcode.com/threads/T-01a02836-fd56-769a-b44a-a7412431fd1d Co-authored-by: Lachlan Donald --- internal/runtime/containers.go | 17 ++++++++++++++++- internal/runtime/containers_test.go | 15 +++++++++++++++ 2 files changed, 31 insertions(+), 1 deletion(-) diff --git a/internal/runtime/containers.go b/internal/runtime/containers.go index 69cce70c..378399d8 100644 --- a/internal/runtime/containers.go +++ b/internal/runtime/containers.go @@ -335,7 +335,17 @@ func (r Runner) startJobContainerOrdered(ctx context.Context, processor *command } args = append(args, spec.Image, "-c", "while :; do sleep 3600; done") if _, err = boundedDockerOutput(ctx, env, docker, args...); err != nil { - return nil, fmt.Errorf("create job container: %w", err) + createErr := err + exists, queryErr := b.jobContainerExists(ctx) + if queryErr != nil { + createErr = errors.Join(createErr, fmt.Errorf("reconcile job container create: %w", queryErr)) + } + if exists && len(spec.Volumes) != 0 { + if trackErr := b.trackContainerVolumes(ctx, "job container", b.container); trackErr != nil { + createErr = errors.Join(createErr, trackErr) + } + } + return nil, fmt.Errorf("create job container: %w", createErr) } if len(spec.Volumes) != 0 { if err = b.trackContainerVolumes(ctx, "job container", b.container); err != nil { @@ -868,6 +878,11 @@ func (b *jobContainerBackend) serviceContainerExists(ctx context.Context, id str return strings.TrimSpace(out) != "", err } +func (b *jobContainerBackend) jobContainerExists(ctx context.Context) (bool, error) { + out, err := boundedDockerOutput(ctx, b.env, b.docker, "ps", "--all", "--quiet", "--filter", "label="+b.owner, "--filter", "name=^/"+b.container+"$") + return strings.TrimSpace(out) != "", err +} + func removeDockerConfig(path string) error { if err := os.RemoveAll(path); err != nil { return fmt.Errorf("remove private Docker configuration: %w", err) diff --git a/internal/runtime/containers_test.go b/internal/runtime/containers_test.go index 5eb52052..3ec91498 100644 --- a/internal/runtime/containers_test.go +++ b/internal/runtime/containers_test.go @@ -1468,6 +1468,21 @@ func TestRunJobContainerLaterServiceCreateFailureCleansExactServices(t *testing. } } +func TestRunJobContainerAmbiguousCreateFailureCleansNamedVolume(t *testing.T) { + f := newJobDocker(t, "fail-create") + _, err := (Runner{Docker: f.path, RuntimeExecutable: os.Args[0]}).startJobContainer( + t.Context(), newCommandProcessor(io.Discard, io.Discard), t.TempDir(), t.TempDir(), + &plan.Container{Image: "alpine", Volumes: []string{"cache:/cache"}}, nil, + ) + if err == nil || !strings.Contains(err.Error(), "create job container") { + t.Fatalf("startJobContainer() error = %v", err) + } + removed, readErr := os.ReadFile(filepath.Join(f.root, "removed-volumes")) + if readErr != nil || strings.TrimSpace(string(removed)) != "cache" { + t.Fatalf("removed volumes = %q, %v", removed, readErr) + } +} + func TestRunJobContainerServiceReadinessCancellationCleansEverything(t *testing.T) { t.Parallel() From 824245583b73273a4ee1128a32e1b603a8c36321 Mon Sep 17 00:00:00 2001 From: Amp Date: Sat, 22 Aug 2026 07:20:20 +0000 Subject: [PATCH 03/15] Reject duplicate job container volume targets Amp-Thread-ID: https://ampcode.com/threads/T-01a02836-fd56-769a-b44a-a7412431fd1d Co-authored-by: Lachlan Donald --- docs/compatibility.md | 2 +- internal/containerpolicy/policy.go | 13 +++++++++++++ internal/containerpolicy/policy_test.go | 3 +++ internal/workflow/parse.go | 6 ++++++ internal/workflow/parse_test.go | 1 + 5 files changed, 24 insertions(+), 1 deletion(-) diff --git a/docs/compatibility.md b/docs/compatibility.md index b25073be..a19d8161 100644 --- a/docs/compatibility.md +++ b/docs/compatibility.md @@ -648,7 +648,7 @@ services: Job containers support `image`, `env`, `ports`, and a bounded subset of `volumes` and `options`. Services support `image`, `credentials`, `env`, `ports`, `volumes`, `options`, `command`, and `entrypoint`. - Job container images can use compile-time `github`, `inputs`, `vars`, `strategy`, and `matrix` values. The complete image must resolve to a non-empty string and a valid image reference during compilation. Secrets, `needs`, step outputs, and whole or dynamic contexts are unsupported. -- Job container volumes accept `NAME:/absolute/container/path`, optionally followed by `:ro` or `:rw`. Volume names contain up to 128 letters, digits, periods, underscores, or hyphens and start with a letter or digit. A job can define 32 unique volumes. Host bind mounts, expressions, and targets under `/__w` or `/__buildkite-gha` are unsupported. +- Job container volumes accept `NAME:/absolute/container/path`, optionally followed by `:ro` or `:rw`. Volume names contain up to 128 letters, digits, periods, underscores, or hyphens and start with a letter or digit. A job can define 32 unique volumes with unique target paths. Host bind mounts, expressions, and targets under `/__w` or `/__buildkite-gha` are unsupported. - Job container options accept `--cpus`, `--cpuset-cpus`, `--memory` (or `-m`), `--memory-reservation`, `--memory-swap`, `--pids-limit`, and `--shm-size`. Use `--flag value` or `--flag=value`, without quotes or expressions. Each option can appear once. Other Docker options are unsupported because they can override runner-owned lifecycle, isolation, networking, mounts, or privileges. - Service fields can use compile-time `github`, `inputs`, `vars`, `strategy`, and `matrix` values or runtime `needs` outputs. An empty evaluated image skips the service. - A complete non-credential service map can use `${{ fromJSON(needs..outputs.) }}`. Declare credentials statically so the compiler can prove their secret authority. diff --git a/internal/containerpolicy/policy.go b/internal/containerpolicy/policy.go index 33fa54f2..365cc6f6 100644 --- a/internal/containerpolicy/policy.go +++ b/internal/containerpolicy/policy.go @@ -119,6 +119,7 @@ func ValidateJobVolumes(values []string) error { return fmt.Errorf("more than %d volumes", MaxJobVolumes) } seen := map[string]bool{} + seenTargets := map[string]bool{} for _, value := range values { if seen[value] { return fmt.Errorf("volume %q is repeated", value) @@ -127,6 +128,11 @@ func ValidateJobVolumes(values []string) error { if err := ValidateJobVolume(value); err != nil { return fmt.Errorf("invalid volume %q: %w", value, err) } + target := JobVolumeTarget(value) + if seenTargets[target] { + return fmt.Errorf("volume target %q is repeated", target) + } + seenTargets[target] = true } return nil } @@ -137,6 +143,13 @@ func JobVolumeName(value string) string { return name } +// JobVolumeTarget returns the target path from a validated job volume. +func JobVolumeTarget(value string) string { + _, remainder, _ := strings.Cut(value, ":") + target, _, _ := strings.Cut(remainder, ":") + return target +} + func hasASCIIControl(value string) bool { return strings.ContainsFunc(value, func(r rune) bool { return r < 0x20 || r == 0x7f }) } diff --git a/internal/containerpolicy/policy_test.go b/internal/containerpolicy/policy_test.go index a312bdce..0ba7dcf8 100644 --- a/internal/containerpolicy/policy_test.go +++ b/internal/containerpolicy/policy_test.go @@ -41,4 +41,7 @@ func TestValidateJobVolume(t *testing.T) { if err := ValidateJobVolumes([]string{"cache:/one", "cache:/one"}); err == nil { t.Error("ValidateJobVolumes() accepted a repeated volume") } + if err := ValidateJobVolumes([]string{"one:/cache", "two:/cache"}); err == nil { + t.Error("ValidateJobVolumes() accepted a repeated target") + } } diff --git a/internal/workflow/parse.go b/internal/workflow/parse.go index 432c9cc6..48b717b4 100644 --- a/internal/workflow/parse.go +++ b/internal/workflow/parse.go @@ -533,6 +533,7 @@ func validateRawContainer(path string, node *yaml.Node, service bool) (rawServic return extra, nil, rawError(path, volumes, "invalid "+subject+" volumes") } seen := map[string]bool{} + seenTargets := map[string]bool{} for _, volume := range volumes.Content { if volume.Kind != yaml.ScalarNode || len(volume.Value) > 4096 || strings.ContainsAny(volume.Value, "\x00\r\n") || seen[volume.Value] { return extra, nil, rawError(path, volume, "invalid or repeated "+subject+" volume") @@ -544,6 +545,11 @@ func validateRawContainer(path string, node *yaml.Node, service bool) (rawServic if err := containerpolicy.ValidateJobVolume(volume.Value); err != nil { return extra, nil, rawError(path, volume, "invalid container volume: "+err.Error()) } + target := containerpolicy.JobVolumeTarget(volume.Value) + if seenTargets[target] { + return extra, nil, rawError(path, volume, "repeated container volume target "+target) + } + seenTargets[target] = true } seen[volume.Value] = true extra.Volumes = append(extra.Volumes, volume.Value) diff --git a/internal/workflow/parse_test.go b/internal/workflow/parse_test.go index 28b79aba..582f8ff1 100644 --- a/internal/workflow/parse_test.go +++ b/internal/workflow/parse_test.go @@ -366,6 +366,7 @@ func TestParseRejectsUnsafeJobContainerOptionsAndVolumes(t *testing.T) { "entrypoint option": "options: --entrypoint sh", "option expression": "options: --cpus ${{ matrix.cpus }}", "host bind": "volumes: ['/tmp:/data']", + "duplicate target": "volumes: ['one:/data', 'two:/data']", "workspace overlap": "volumes: ['cache:/__w/repo']", "volume expression": "volumes: ['${{ matrix.name }}:/data']", "unsupported volume mode": "volumes: ['cache:/data:z']", From ca3d86a835a0179d5829123f22183fdc0b2b5919 Mon Sep 17 00:00:00 2001 From: Amp Date: Sat, 22 Aug 2026 07:31:09 +0000 Subject: [PATCH 04/15] Reject unsupported short job volume names Amp-Thread-ID: https://ampcode.com/threads/T-01a02836-fd56-769a-b44a-a7412431fd1d Co-authored-by: Lachlan Donald --- docs/compatibility.md | 2 +- internal/containerpolicy/policy.go | 2 +- internal/containerpolicy/policy_test.go | 2 +- internal/plan/plan_test.go | 1 + internal/workflow/parse_test.go | 1 + 5 files changed, 5 insertions(+), 3 deletions(-) diff --git a/docs/compatibility.md b/docs/compatibility.md index a19d8161..85b38287 100644 --- a/docs/compatibility.md +++ b/docs/compatibility.md @@ -648,7 +648,7 @@ services: Job containers support `image`, `env`, `ports`, and a bounded subset of `volumes` and `options`. Services support `image`, `credentials`, `env`, `ports`, `volumes`, `options`, `command`, and `entrypoint`. - Job container images can use compile-time `github`, `inputs`, `vars`, `strategy`, and `matrix` values. The complete image must resolve to a non-empty string and a valid image reference during compilation. Secrets, `needs`, step outputs, and whole or dynamic contexts are unsupported. -- Job container volumes accept `NAME:/absolute/container/path`, optionally followed by `:ro` or `:rw`. Volume names contain up to 128 letters, digits, periods, underscores, or hyphens and start with a letter or digit. A job can define 32 unique volumes with unique target paths. Host bind mounts, expressions, and targets under `/__w` or `/__buildkite-gha` are unsupported. +- Job container volumes accept `NAME:/absolute/container/path`, optionally followed by `:ro` or `:rw`. Volume names contain 2 to 128 letters, digits, periods, underscores, or hyphens and start with a letter or digit. A job can define 32 unique volumes with unique target paths. Host bind mounts, expressions, and targets under `/__w` or `/__buildkite-gha` are unsupported. - Job container options accept `--cpus`, `--cpuset-cpus`, `--memory` (or `-m`), `--memory-reservation`, `--memory-swap`, `--pids-limit`, and `--shm-size`. Use `--flag value` or `--flag=value`, without quotes or expressions. Each option can appear once. Other Docker options are unsupported because they can override runner-owned lifecycle, isolation, networking, mounts, or privileges. - Service fields can use compile-time `github`, `inputs`, `vars`, `strategy`, and `matrix` values or runtime `needs` outputs. An empty evaluated image skips the service. - A complete non-credential service map can use `${{ fromJSON(needs..outputs.) }}`. Declare credentials statically so the compiler can prove their secret authority. diff --git a/internal/containerpolicy/policy.go b/internal/containerpolicy/policy.go index 365cc6f6..6482a5f5 100644 --- a/internal/containerpolicy/policy.go +++ b/internal/containerpolicy/policy.go @@ -13,7 +13,7 @@ import ( const MaxJobVolumes = 32 var ( - volumeNamePattern = regexp.MustCompile(`^[A-Za-z0-9][A-Za-z0-9_.-]{0,127}$`) + volumeNamePattern = regexp.MustCompile(`^[A-Za-z0-9][A-Za-z0-9_.-]{1,127}$`) sizePattern = regexp.MustCompile(`^[1-9][0-9]*[bBkKmMgG]?$`) cpusPattern = regexp.MustCompile(`^(?:[0-9]+(?:\.[0-9]+)?|\.[0-9]+)$`) cpusetPattern = regexp.MustCompile(`^[0-9]+(?:-[0-9]+)?(?:,[0-9]+(?:-[0-9]+)?)*$`) diff --git a/internal/containerpolicy/policy_test.go b/internal/containerpolicy/policy_test.go index 0ba7dcf8..75875a28 100644 --- a/internal/containerpolicy/policy_test.go +++ b/internal/containerpolicy/policy_test.go @@ -33,7 +33,7 @@ func TestValidateJobVolume(t *testing.T) { t.Errorf("ValidateJobVolume(%q) = %v", value, err) } } - for _, value := range []string{"/tmp:/data", "cache:data", "cache:/", "cache:/data/../other", "cache:/__w", "cache:/__buildkite-gha/runtime", "cache:/data:z"} { + for _, value := range []string{"v:/data", "/tmp:/data", "cache:data", "cache:/", "cache:/data/../other", "cache:/__w", "cache:/__buildkite-gha/runtime", "cache:/data:z"} { if err := ValidateJobVolume(value); err == nil { t.Errorf("ValidateJobVolume(%q) accepted invalid volume", value) } diff --git a/internal/plan/plan_test.go b/internal/plan/plan_test.go index f6100704..10e56fea 100644 --- a/internal/plan/plan_test.go +++ b/internal/plan/plan_test.go @@ -992,6 +992,7 @@ func TestJobContainerPlanRejectsUnsafeOptionsAndVolumes(t *testing.T) { "unbounded memory": {Image: "node:24", Options: "--memory-swap -1"}, "repeated option": {Image: "node:24", Options: "--cpus 1 --cpus=2"}, "bind mount": {Image: "node:24", Volumes: []string{"/tmp:/data"}}, + "short volume name": {Image: "node:24", Volumes: []string{"v:/data"}}, "runtime overlap": {Image: "node:24", Volumes: []string{"cache:/__buildkite-gha/runtime"}}, "unsupported volume": {Image: "node:24", Volumes: []string{"cache:/data:z"}}, } { diff --git a/internal/workflow/parse_test.go b/internal/workflow/parse_test.go index 582f8ff1..7fbbf5c7 100644 --- a/internal/workflow/parse_test.go +++ b/internal/workflow/parse_test.go @@ -366,6 +366,7 @@ func TestParseRejectsUnsafeJobContainerOptionsAndVolumes(t *testing.T) { "entrypoint option": "options: --entrypoint sh", "option expression": "options: --cpus ${{ matrix.cpus }}", "host bind": "volumes: ['/tmp:/data']", + "short volume name": "volumes: ['v:/data']", "duplicate target": "volumes: ['one:/data', 'two:/data']", "workspace overlap": "volumes: ['cache:/__w/repo']", "volume expression": "volumes: ['${{ matrix.name }}:/data']", From beeebc69823596b4c515264e95dce115dd1ec7ea Mon Sep 17 00:00:00 2001 From: Amp Date: Sat, 22 Aug 2026 07:42:46 +0000 Subject: [PATCH 05/15] Reconcile job volumes after setup cancellation Amp-Thread-ID: https://ampcode.com/threads/T-01a02836-fd56-769a-b44a-a7412431fd1d Co-authored-by: Lachlan Donald --- internal/runtime/containers.go | 6 +++-- internal/runtime/containers_test.go | 36 +++++++++++++++++++++++++++++ 2 files changed, 40 insertions(+), 2 deletions(-) diff --git a/internal/runtime/containers.go b/internal/runtime/containers.go index 378399d8..9933928a 100644 --- a/internal/runtime/containers.go +++ b/internal/runtime/containers.go @@ -336,15 +336,17 @@ func (r Runner) startJobContainerOrdered(ctx context.Context, processor *command args = append(args, spec.Image, "-c", "while :; do sleep 3600; done") if _, err = boundedDockerOutput(ctx, env, docker, args...); err != nil { createErr := err - exists, queryErr := b.jobContainerExists(ctx) + reconcileCtx, cancelReconcile := context.WithTimeout(context.WithoutCancel(ctx), r.cleanupTimeout()) + exists, queryErr := b.jobContainerExists(reconcileCtx) if queryErr != nil { createErr = errors.Join(createErr, fmt.Errorf("reconcile job container create: %w", queryErr)) } if exists && len(spec.Volumes) != 0 { - if trackErr := b.trackContainerVolumes(ctx, "job container", b.container); trackErr != nil { + if trackErr := b.trackContainerVolumes(reconcileCtx, "job container", b.container); trackErr != nil { createErr = errors.Join(createErr, trackErr) } } + cancelReconcile() return nil, fmt.Errorf("create job container: %w", createErr) } if len(spec.Volumes) != 0 { diff --git a/internal/runtime/containers_test.go b/internal/runtime/containers_test.go index 3ec91498..2afe5c8b 100644 --- a/internal/runtime/containers_test.go +++ b/internal/runtime/containers_test.go @@ -279,6 +279,9 @@ func TestJobContainerFakeDockerProcess(t *testing.T) { if scenario == "fail-create" { os.Exit(42) } + if scenario == "block-job-create" && !strings.HasPrefix(name, "buildkite-gha-service-") { + select {} + } fmt.Print("docker-id-" + name) os.Exit(0) case "start": @@ -1483,6 +1486,39 @@ func TestRunJobContainerAmbiguousCreateFailureCleansNamedVolume(t *testing.T) { } } +func TestRunJobContainerCreateCancellationCleansNamedVolume(t *testing.T) { + f := newJobDocker(t, "block-job-create") + w, tmp := t.TempDir(), t.TempDir() + ctx, cancel := context.WithCancel(t.Context()) + done := make(chan error, 1) + go func() { + _, err := (Runner{Docker: f.path, RuntimeExecutable: os.Args[0]}).startJobContainer( + ctx, newCommandProcessor(io.Discard, io.Discard), w, tmp, + &plan.Container{Image: "alpine", Volumes: []string{"cache:/cache"}}, nil, + ) + done <- err + }() + deadline := time.Now().Add(10 * time.Second) + for { + if _, err := os.Stat(filepath.Join(f.root, "container")); err == nil { + break + } + if time.Now().After(deadline) { + cancel() + t.Fatal("job container create did not start") + } + time.Sleep(10 * time.Millisecond) + } + cancel() + if err := <-done; err == nil || !strings.Contains(err.Error(), "create job container") { + t.Fatalf("startJobContainer() error = %v", err) + } + removed, readErr := os.ReadFile(filepath.Join(f.root, "removed-volumes")) + if readErr != nil || strings.TrimSpace(string(removed)) != "cache" { + t.Fatalf("removed volumes = %q, %v", removed, readErr) + } +} + func TestRunJobContainerServiceReadinessCancellationCleansEverything(t *testing.T) { t.Parallel() From 8242ff48b089d95be7818c2b277cec5e9a8a68b8 Mon Sep 17 00:00:00 2001 From: Amp Date: Sat, 22 Aug 2026 07:53:55 +0000 Subject: [PATCH 06/15] Preserve container ownership edge cases Amp-Thread-ID: https://ampcode.com/threads/T-01a02836-fd56-769a-b44a-a7412431fd1d Co-authored-by: Lachlan Donald --- internal/runtime/containers.go | 8 +++++++- internal/runtime/containers_test.go | 16 ++++++++++++++++ internal/workflow/parse.go | 2 +- internal/workflow/parse_test.go | 12 ++++++++++++ 4 files changed, 36 insertions(+), 2 deletions(-) diff --git a/internal/runtime/containers.go b/internal/runtime/containers.go index 9933928a..d4e1ece3 100644 --- a/internal/runtime/containers.go +++ b/internal/runtime/containers.go @@ -350,7 +350,7 @@ func (r Runner) startJobContainerOrdered(ctx context.Context, processor *command return nil, fmt.Errorf("create job container: %w", createErr) } if len(spec.Volumes) != 0 { - if err = b.trackContainerVolumes(ctx, "job container", b.container); err != nil { + if err = b.trackJobContainerVolumes(ctx); err != nil { return nil, err } } @@ -479,6 +479,12 @@ func (b *jobContainerBackend) trackServiceVolumes(ctx context.Context, serviceID return b.trackContainerVolumes(ctx, fmt.Sprintf("service %q", serviceID), reference) } +func (b *jobContainerBackend) trackJobContainerVolumes(parent context.Context) error { + ctx, cancel := context.WithTimeout(context.WithoutCancel(parent), b.runner.cleanupTimeout()) + defer cancel() + return b.trackContainerVolumes(ctx, "job container", b.container) +} + func (b *jobContainerBackend) trackContainerVolumes(ctx context.Context, subject, reference string) error { const format = `{{range .Mounts}}{{if eq .Type "volume"}}{{println .Name}}{{end}}{{end}}` output, err := boundedDockerOutput(ctx, b.env, b.docker, "inspect", "--format", format, reference) diff --git a/internal/runtime/containers_test.go b/internal/runtime/containers_test.go index 2afe5c8b..fe26feda 100644 --- a/internal/runtime/containers_test.go +++ b/internal/runtime/containers_test.go @@ -1519,6 +1519,22 @@ func TestRunJobContainerCreateCancellationCleansNamedVolume(t *testing.T) { } } +func TestTrackJobContainerVolumesSurvivesSetupCancellation(t *testing.T) { + f := newJobDocker(t, "") + if err := os.WriteFile(filepath.Join(f.root, "container"), nil, 0o600); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(f.root, "volumes-job"), []byte("cache\n"), 0o600); err != nil { + t.Fatal(err) + } + ctx, cancel := context.WithCancel(t.Context()) + cancel() + b := &jobContainerBackend{runner: Runner{Docker: f.path}, docker: f.path, env: map[string]string{"DOCKER_CONFIG": t.TempDir()}, container: "job", existingVolumes: map[string]bool{}} + if err := b.trackJobContainerVolumes(ctx); err != nil || !slices.Equal(b.ownedVolumes, []string{"cache"}) { + t.Fatalf("trackJobContainerVolumes() volumes = %#v, error = %v", b.ownedVolumes, err) + } +} + func TestRunJobContainerServiceReadinessCancellationCleansEverything(t *testing.T) { t.Parallel() diff --git a/internal/workflow/parse.go b/internal/workflow/parse.go index 48b717b4..791e5f77 100644 --- a/internal/workflow/parse.go +++ b/internal/workflow/parse.go @@ -344,7 +344,7 @@ func validateRawContainers(path string, document *yaml.Node) (map[string]rawServ return rawContainers, diagnostics, nil } for i := 0; i+1 < len(jobs.Content); i += 2 { - jobID := strings.ToLower(jobs.Content[i].Value) + jobID := strings.ToLower(resolveAlias(jobs.Content[i]).Value) job := jobs.Content[i+1] if container := mappingValue(job, "container"); container != nil { extra, _, err := validateRawContainer(path, container, false) diff --git a/internal/workflow/parse_test.go b/internal/workflow/parse_test.go index 7fbbf5c7..22bf36d4 100644 --- a/internal/workflow/parse_test.go +++ b/internal/workflow/parse_test.go @@ -574,6 +574,18 @@ func TestParseContainerShortForms(t *testing.T) { } } +func TestParseContainerUnderAliasedJobID(t *testing.T) { + source := []byte("on: push\nenv: {ID: &job-id test}\njobs:\n *job-id:\n runs-on: ubuntu-latest\n container:\n image: node:24\n ports: [8080]\n volumes: ['cache:/cache']\n options: --cpus 2\n steps: [{run: true}]\n") + parsed, err := Parse("alias.yml", source) + if err != nil { + t.Fatal(err) + } + container := parsed.Jobs[0].Container + if container == nil || !slices.Equal(container.Ports, []string{"8080"}) || !slices.Equal(container.Volumes, []string{"cache:/cache"}) || container.Options != "--cpus 2" { + t.Fatalf("container = %#v", container) + } +} + func TestParseRetainsSequentialRuntimeControls(t *testing.T) { source := []byte("name: runtime\non: push\njobs:\n test:\n runs-on: ubuntu-latest\n if: always()\n continue-on-error: true\n timeout-minutes: 5\n steps:\n - run: echo ok\n if: success()\n timeout-minutes: 2\n continue-on-error: true\n") parsed, err := Parse("workflow.yml", source) From 3a5e5982b6199fd5c4d26b57e01a36985c92c866 Mon Sep 17 00:00:00 2001 From: Amp Date: Sat, 22 Aug 2026 09:07:54 +0000 Subject: [PATCH 07/15] Match GitHub job container controls Amp-Thread-ID: https://ampcode.com/threads/T-01a02836-fd56-769a-b44a-a7412431fd1d Co-authored-by: Lachlan Donald --- docs/compatibility.md | 8 +- docs/security.md | 9 +- internal/compiler/bundle_test.go | 6 +- internal/compiler/compiler_test.go | 4 +- internal/containerpolicy/policy.go | 137 +++++++----------------- internal/containerpolicy/policy_test.go | 35 ++++-- internal/plan/plan_test.go | 24 +++-- internal/runtime/containers.go | 102 +++++++++++++----- internal/runtime/containers_test.go | 95 +++++++++++----- internal/workflow/parse.go | 8 +- internal/workflow/parse_test.go | 20 ++-- 11 files changed, 246 insertions(+), 202 deletions(-) diff --git a/docs/compatibility.md b/docs/compatibility.md index 85b38287..1a48b5ba 100644 --- a/docs/compatibility.md +++ b/docs/compatibility.md @@ -645,11 +645,11 @@ services: --health-retries 10 ``` -Job containers support `image`, `env`, `ports`, and a bounded subset of `volumes` and `options`. Services support `image`, `credentials`, `env`, `ports`, `volumes`, `options`, `command`, and `entrypoint`. +Job containers support `image`, `env`, `ports`, `volumes`, and `options`. Services support `image`, `credentials`, `env`, `ports`, `volumes`, `options`, `command`, and `entrypoint`. - Job container images can use compile-time `github`, `inputs`, `vars`, `strategy`, and `matrix` values. The complete image must resolve to a non-empty string and a valid image reference during compilation. Secrets, `needs`, step outputs, and whole or dynamic contexts are unsupported. -- Job container volumes accept `NAME:/absolute/container/path`, optionally followed by `:ro` or `:rw`. Volume names contain 2 to 128 letters, digits, periods, underscores, or hyphens and start with a letter or digit. A job can define 32 unique volumes with unique target paths. Host bind mounts, expressions, and targets under `/__w` or `/__buildkite-gha` are unsupported. -- Job container options accept `--cpus`, `--cpuset-cpus`, `--memory` (or `-m`), `--memory-reservation`, `--memory-swap`, `--pids-limit`, and `--shm-size`. Use `--flag value` or `--flag=value`, without quotes or expressions. Each option can appear once. Other Docker options are unsupported because they can override runner-owned lifecycle, isolation, networking, mounts, or privileges. +- Job container volumes accept `DESTINATION[:ro|rw]` for an anonymous volume or `SOURCE:DESTINATION[:ro|rw]` for a named volume or bind mount. `DESTINATION` must be absolute. `SOURCE` must be a Docker volume name or absolute host path. A job can define 128 unique declarations. Expressions are unsupported. +- Job container options pass through to `docker create`, except `--network`, `--net`, and `--entrypoint`, including their `--flag=value` forms. Options split into arguments without a shell. Double quotes group arguments; single quotes are ordinary characters. Expressions, line breaks, NUL bytes, and values over 65,536 bytes are unsupported. - Service fields can use compile-time `github`, `inputs`, `vars`, `strategy`, and `matrix` values or runtime `needs` outputs. An empty evaluated image skips the service. - A complete non-credential service map can use `${{ fromJSON(needs..outputs.) }}`. Declare credentials statically so the compiler can prove their secret authority. - Credentials accept direct values and `github`, `vars`, `secrets`, or `env` expressions. Passwords pass to `docker login` through standard input. Authentication uses a private per-job Docker configuration and never reads ambient Docker credentials. @@ -663,7 +663,7 @@ Each job uses a private Docker bridge network. Container jobs reach services by A service with a Docker health check must become healthy before steps run. A service without one is ready after it starts. Failures include bounded status, health, port, and log diagnostics. -Cleanup removes the job container, emits masked and bounded service logs, then removes services in declaration order, the network, newly created job and service volumes, and private Docker configuration. A job container volume name that existed before the job is rejected rather than attached. Remaining owned resources fail the job. Docker resources are not a security or resource-isolation boundary: the hosted queue must isolate the whole job and enforce host CPU, memory, disk, and network limits. See the [security model](security.md#isolate-the-whole-job). +Cleanup removes the job container, emits masked and bounded service logs, then removes services in declaration order, the network, volumes created during the job, and private Docker configuration. Pre-existing named volumes can be attached but are not removed. Remaining owned resources fail the job. Docker resources are not a security or resource-isolation boundary: the hosted queue must isolate the whole job and enforce host CPU, memory, disk, and network limits. See the [security model](security.md#isolate-the-whole-job). macOS jobs reject containers, services, Docker actions, and Docker capability. diff --git a/docs/security.md b/docs/security.md index fbdf631f..11cfa6be 100644 --- a/docs/security.md +++ b/docs/security.md @@ -45,10 +45,11 @@ Run untrusted jobs on a queue with: - a clean environment for every job - host-level CPU, memory, disk, and network limits -Job container options are limited to resource controls. Job container volumes -accept named Docker volumes, not host bind mounts, and cannot replace the -runner-owned workspace or runtime mounts. Service options can grant privileges, -mount host paths, and publish ports. The private Docker network, ownership +Job and service container options can grant privileges, mount host paths, +publish ports, and override Docker settings. Job container options cannot +override the runner-owned network or entrypoint, but other Docker create +options pass through. Job container volumes accept named volumes, anonymous +volumes, and absolute host bind mounts. The private Docker network, ownership labels, and cleanup checks reduce accidental residue. They do not contain hostile code. diff --git a/internal/compiler/bundle_test.go b/internal/compiler/bundle_test.go index c44799c6..cf3d2c11 100644 --- a/internal/compiler/bundle_test.go +++ b/internal/compiler/bundle_test.go @@ -70,8 +70,8 @@ jobs: image: [node:24, node:25] container: image: ${{ matrix.image }} - volumes: [cache:/cache:ro] - options: --cpus 2 + volumes: [cache:/cache:ro, /anonymous, /srv/data:/data] + options: --privileged --label "description=two words" steps: [{run: true}] `) event := readFile(t, smokePath("events", "push.json")) @@ -88,7 +88,7 @@ jobs: } for i, image := range []string{"node:24", "node:25"} { container := first.Plans[i].Job.Container - if container == nil || container.Image != image || !slices.Equal(container.Volumes, []string{"cache:/cache:ro"}) || container.Options != "--cpus 2" || bytes.Contains(first.Plans[i].Contents, []byte("${{")) { + if container == nil || container.Image != image || !slices.Equal(container.Volumes, []string{"cache:/cache:ro", "/anonymous", "/srv/data:/data"}) || container.Options != `--privileged --label "description=two words"` || bytes.Contains(first.Plans[i].Contents, []byte("${{")) { t.Fatalf("plan %d container = %#v", i, first.Plans[i].Job.Container) } } diff --git a/internal/compiler/compiler_test.go b/internal/compiler/compiler_test.go index 9ea2f19d..87319d93 100644 --- a/internal/compiler/compiler_test.go +++ b/internal/compiler/compiler_test.go @@ -4173,12 +4173,12 @@ func readFile(t *testing.T, path string) []byte { } func TestCompilePlansEmitV8ForContainers(t *testing.T) { - workflowSource := []byte("on: push\njobs:\n test:\n runs-on: ubuntu-latest\n container:\n image: node:24\n volumes: ['cache:/cache:ro']\n options: --cpus=2 --memory 1g\n services:\n redis: {image: redis:7}\n steps:\n - run: true\n") + workflowSource := []byte("on: push\njobs:\n test:\n runs-on: ubuntu-latest\n container:\n image: node:24\n volumes: ['cache:/cache:ro', '/anonymous', '/srv/data:/data']\n options: --privileged --label \"description=two words\"\n services:\n redis: {image: redis:7}\n steps:\n - run: true\n") plans, err := compileUntrustedPlans("containers.yml", workflowSource, readFile(t, smokePath("events", "push.json")), "0.0.0-test", "sha256:"+strings.Repeat("1", 64), "gha-untrusted") if err != nil { t.Fatal(err) } - if len(plans) != 1 || plans[0].Schema != plan.Schema || plans[0].Container == nil || !slices.Equal(plans[0].Container.Volumes, []string{"cache:/cache:ro"}) || plans[0].Container.Options != "--cpus=2 --memory 1g" || len(plans[0].Services) != 1 || !slices.Equal(plans[0].RequiredCapabilities, []string{"docker", "network"}) { + if len(plans) != 1 || plans[0].Schema != plan.Schema || plans[0].Container == nil || !slices.Equal(plans[0].Container.Volumes, []string{"cache:/cache:ro", "/anonymous", "/srv/data:/data"}) || plans[0].Container.Options != `--privileged --label "description=two words"` || len(plans[0].Services) != 1 || !slices.Equal(plans[0].RequiredCapabilities, []string{"docker", "network"}) { t.Fatalf("container plan = %#v", plans) } } diff --git a/internal/containerpolicy/policy.go b/internal/containerpolicy/policy.go index 6482a5f5..ecf3d1d4 100644 --- a/internal/containerpolicy/policy.go +++ b/internal/containerpolicy/policy.go @@ -6,108 +6,68 @@ import ( "fmt" "path" "regexp" - "strconv" "strings" ) -const MaxJobVolumes = 32 +const MaxJobVolumes = 128 +const MaxJobOptionsLength = 65536 -var ( - volumeNamePattern = regexp.MustCompile(`^[A-Za-z0-9][A-Za-z0-9_.-]{1,127}$`) - sizePattern = regexp.MustCompile(`^[1-9][0-9]*[bBkKmMgG]?$`) - cpusPattern = regexp.MustCompile(`^(?:[0-9]+(?:\.[0-9]+)?|\.[0-9]+)$`) - cpusetPattern = regexp.MustCompile(`^[0-9]+(?:-[0-9]+)?(?:,[0-9]+(?:-[0-9]+)?)*$`) - positivePattern = regexp.MustCompile(`^[1-9][0-9]*$`) -) +var volumeNamePattern = regexp.MustCompile(`^[A-Za-z0-9][A-Za-z0-9_.-]*$`) // JobOptions splits options using the GitHub runner's argument rules and -// accepts only bounded resource controls. It returns the exact Docker argv. +// rejects the network and entrypoint overrides that GitHub does not support. +// It returns the exact Docker argv without invoking a shell. func JobOptions(value string) ([]string, error) { - if len(value) > 4096 || hasASCIIControl(value) || strings.Contains(value, "\"") { - return nil, fmt.Errorf("options exceed 4096 bytes or contain a control or quote character") + if len(value) > MaxJobOptionsLength || strings.ContainsAny(value, "\x00\r\n") { + return nil, fmt.Errorf("options exceed %d bytes or contain a control character", MaxJobOptionsLength) } args := ArgumentList(value) - if len(args) > 16 { - return nil, fmt.Errorf("options contain more than 16 arguments") - } - seen := map[string]bool{} - for i := 0; i < len(args); i++ { - option, optionValue, inline := strings.Cut(args[i], "=") - if !inline { - if i+1 == len(args) { - return nil, fmt.Errorf("option %q requires a value", option) + for _, arg := range args { + for _, unsupported := range []string{"--network", "--net", "--entrypoint"} { + if arg == unsupported || strings.HasPrefix(arg, unsupported+"=") { + return nil, fmt.Errorf("option %q is unsupported", arg) } - i++ - optionValue = args[i] - } - canonical := option - if canonical == "-m" { - canonical = "--memory" - } - if seen[canonical] { - return nil, fmt.Errorf("option %q is repeated", option) - } - seen[canonical] = true - valid := false - switch canonical { - case "--cpus": - cpus, err := strconv.ParseFloat(optionValue, 64) - valid = cpusPattern.MatchString(optionValue) && err == nil && cpus > 0 - case "--cpuset-cpus": - valid = validCPUSet(optionValue) - case "--memory", "--memory-reservation", "--memory-swap", "--shm-size": - valid = sizePattern.MatchString(optionValue) - case "--pids-limit": - valid = positivePattern.MatchString(optionValue) - default: - return nil, fmt.Errorf("option %q is unsupported", option) - } - if !valid { - return nil, fmt.Errorf("option %q has invalid value %q", option, optionValue) } } return args, nil } -func validCPUSet(value string) bool { - if !cpusetPattern.MatchString(value) { - return false - } - for item := range strings.SplitSeq(value, ",") { - bounds := strings.Split(item, "-") - if len(bounds) == 1 { - continue - } - first, firstErr := strconv.Atoi(bounds[0]) - last, lastErr := strconv.Atoi(bounds[1]) - if firstErr != nil || lastErr != nil || first > last { - return false - } - } - return true -} - -// ValidateJobVolume accepts a named volume mounted at an absolute container -// path. Host bind mounts and runner-owned workspace/runtime targets are denied. +// ValidateJobVolume accepts GitHub's named, anonymous, and absolute host-bind +// volume syntax with an absolute container destination. func ValidateJobVolume(value string) error { if value == "" || len(value) > 4096 || hasASCIIControl(value) { return fmt.Errorf("volume is empty, too long, or contains a control character") } parts := strings.Split(value, ":") - if len(parts) < 2 || len(parts) > 3 || !volumeNamePattern.MatchString(parts[0]) { - return fmt.Errorf("volume must be NAME:/absolute/container/path[:ro|rw]") + var source, target, mode string + switch len(parts) { + case 1: + target = parts[0] + case 2: + if parts[1] == "ro" || parts[1] == "rw" { + target, mode = parts[0], parts[1] + } else { + source, target = parts[0], parts[1] + if source == "" { + return fmt.Errorf("volume source is empty") + } + } + case 3: + source, target, mode = parts[0], parts[1], parts[2] + if source == "" { + return fmt.Errorf("volume source is empty") + } + default: + return fmt.Errorf("volume must be DESTINATION[:ro|rw] or SOURCE:DESTINATION[:ro|rw]") } - target := parts[1] - if !strings.HasPrefix(target, "/") || target != path.Clean(target) || target == "/" { - return fmt.Errorf("volume target %q must be a clean absolute path", target) + if source != "" && !path.IsAbs(source) && !volumeNamePattern.MatchString(source) { + return fmt.Errorf("volume source %q must be a name or absolute host path", source) } - for _, reserved := range []string{"/__w", "/__buildkite-gha"} { - if target == reserved || strings.HasPrefix(target, reserved+"/") { - return fmt.Errorf("volume target %q overlaps a runner-owned path", target) - } + if !path.IsAbs(target) { + return fmt.Errorf("volume target %q must be an absolute path", target) } - if len(parts) == 3 && parts[2] != "ro" && parts[2] != "rw" { - return fmt.Errorf("volume mode %q is unsupported", parts[2]) + if mode != "" && mode != "ro" && mode != "rw" { + return fmt.Errorf("volume mode %q is unsupported", mode) } return nil } @@ -119,7 +79,6 @@ func ValidateJobVolumes(values []string) error { return fmt.Errorf("more than %d volumes", MaxJobVolumes) } seen := map[string]bool{} - seenTargets := map[string]bool{} for _, value := range values { if seen[value] { return fmt.Errorf("volume %q is repeated", value) @@ -128,28 +87,10 @@ func ValidateJobVolumes(values []string) error { if err := ValidateJobVolume(value); err != nil { return fmt.Errorf("invalid volume %q: %w", value, err) } - target := JobVolumeTarget(value) - if seenTargets[target] { - return fmt.Errorf("volume target %q is repeated", target) - } - seenTargets[target] = true } return nil } -// JobVolumeName returns the source name from a validated job volume. -func JobVolumeName(value string) string { - name, _, _ := strings.Cut(value, ":") - return name -} - -// JobVolumeTarget returns the target path from a validated job volume. -func JobVolumeTarget(value string) string { - _, remainder, _ := strings.Cut(value, ":") - target, _, _ := strings.Cut(remainder, ":") - return target -} - func hasASCIIControl(value string) bool { return strings.ContainsFunc(value, func(r rune) bool { return r < 0x20 || r == 0x7f }) } diff --git a/internal/containerpolicy/policy_test.go b/internal/containerpolicy/policy_test.go index 75875a28..1bf28147 100644 --- a/internal/containerpolicy/policy_test.go +++ b/internal/containerpolicy/policy_test.go @@ -2,38 +2,53 @@ package containerpolicy import ( "slices" + "strings" "testing" ) func TestJobOptionsAcceptedSyntax(t *testing.T) { - value := "--cpus=.5 --cpuset-cpus 0-2,4 -m 512m --memory-reservation=256M --memory-swap 1g --pids-limit=128 --shm-size 64m" - want := []string{"--cpus=.5", "--cpuset-cpus", "0-2,4", "-m", "512m", "--memory-reservation=256M", "--memory-swap", "1g", "--pids-limit=128", "--shm-size", "64m"} + value := `--privileged --user 1000 --label "description=two words" --mount type=tmpfs,dst=/tmp --name=custom` + want := []string{"--privileged", "--user", "1000", "--label", "description=two words", "--mount", "type=tmpfs,dst=/tmp", "--name=custom"} got, err := JobOptions(value) if err != nil || !slices.Equal(got, want) { t.Fatalf("JobOptions(%q) = %#v, %v; want %#v", value, got, err, want) } } -func TestJobOptionsRejectsInvalidSyntax(t *testing.T) { +func TestJobOptionsRejectsUnsupportedOverrides(t *testing.T) { for _, value := range []string{ - "--cpus 0", "--cpus NaN", "--cpuset-cpus 3-1", "--memory -1", "--memory 1t", - "--pids-limit 0", "--shm-size", "--cpus 1 --cpus=2", "--memory 1g extra", + "--network host", "--network=host", "--net host", "--net=host", + "--entrypoint sh", "--entrypoint=sh", } { t.Run(value, func(t *testing.T) { if _, err := JobOptions(value); err == nil { - t.Fatal("JobOptions() accepted invalid syntax") + t.Fatal("JobOptions() accepted an unsupported override") } }) } } +func TestJobOptionsInputBounds(t *testing.T) { + if _, err := JobOptions(strings.Repeat("x", MaxJobOptionsLength)); err != nil { + t.Fatalf("JobOptions() rejected the maximum input length: %v", err) + } + for _, value := range []string{strings.Repeat("x", MaxJobOptionsLength+1), "--label=a\n--privileged", "--label=a\x00b"} { + if _, err := JobOptions(value); err == nil { + t.Fatal("JobOptions() accepted out-of-bounds input") + } + } +} + func TestValidateJobVolume(t *testing.T) { - for _, value := range []string{"cache:/cache", "cache.v1:/var/cache:ro", "CACHE_1:/data:rw"} { + for _, value := range []string{ + "v:/data", "cache:/cache", "cache.v1:/var/cache:ro", "CACHE_1:/data:rw", + "/anonymous", "/anonymous:ro", "/srv/cache:/cache", "/srv/cache:/cache:rw", + } { if err := ValidateJobVolume(value); err != nil { t.Errorf("ValidateJobVolume(%q) = %v", value, err) } } - for _, value := range []string{"v:/data", "/tmp:/data", "cache:data", "cache:/", "cache:/data/../other", "cache:/__w", "cache:/__buildkite-gha/runtime", "cache:/data:z"} { + for _, value := range []string{"-bad:/data", "relative/path:/data", "cache:data", "cache:/data:z", ":/data"} { if err := ValidateJobVolume(value); err == nil { t.Errorf("ValidateJobVolume(%q) accepted invalid volume", value) } @@ -41,7 +56,7 @@ func TestValidateJobVolume(t *testing.T) { if err := ValidateJobVolumes([]string{"cache:/one", "cache:/one"}); err == nil { t.Error("ValidateJobVolumes() accepted a repeated volume") } - if err := ValidateJobVolumes([]string{"one:/cache", "two:/cache"}); err == nil { - t.Error("ValidateJobVolumes() accepted a repeated target") + if err := ValidateJobVolumes([]string{"one:/cache", "two:/cache"}); err != nil { + t.Errorf("ValidateJobVolumes() rejected Docker-owned duplicate target validation: %v", err) } } diff --git a/internal/plan/plan_test.go b/internal/plan/plan_test.go index 10e56fea..d8b2466b 100644 --- a/internal/plan/plan_test.go +++ b/internal/plan/plan_test.go @@ -985,15 +985,10 @@ func TestContainerModelFields(t *testing.T) { } } -func TestJobContainerPlanRejectsUnsafeOptionsAndVolumes(t *testing.T) { +func TestJobContainerPlanRejectsUnsupportedOptionsAndVolumes(t *testing.T) { for name, container := range map[string]Container{ - "privileged": {Image: "node:24", Options: "--privileged"}, "network": {Image: "node:24", Options: "--network=host"}, - "unbounded memory": {Image: "node:24", Options: "--memory-swap -1"}, - "repeated option": {Image: "node:24", Options: "--cpus 1 --cpus=2"}, - "bind mount": {Image: "node:24", Volumes: []string{"/tmp:/data"}}, - "short volume name": {Image: "node:24", Volumes: []string{"v:/data"}}, - "runtime overlap": {Image: "node:24", Volumes: []string{"cache:/__buildkite-gha/runtime"}}, + "entrypoint": {Image: "node:24", Options: "--entrypoint sh"}, "unsupported volume": {Image: "node:24", Volumes: []string{"cache:/data:z"}}, } { t.Run(name, func(t *testing.T) { @@ -1001,12 +996,25 @@ func TestJobContainerPlanRejectsUnsafeOptionsAndVolumes(t *testing.T) { job.RequiredCapabilities = []string{"docker", "network"} job.Container = &container if err := job.Validate(); err == nil { - t.Fatal("Validate() accepted unsafe job container control") + t.Fatal("Validate() accepted unsupported job container control") } }) } } +func TestJobContainerPlanAcceptsGitHubOptionsAndVolumes(t *testing.T) { + job := validJob() + job.RequiredCapabilities = []string{"docker", "network"} + job.Container = &Container{ + Image: "node:24", + Options: `--privileged --label "description=two words" --memory-swap -1`, + Volumes: []string{"v:/data", "/anonymous", "/tmp:/host", "one:/same", "two:/same", "cache:/__buildkite-gha/runtime"}, + } + if err := job.Validate(); err != nil { + t.Fatalf("Validate() rejected GitHub-compatible job container controls: %v", err) + } +} + func TestPrerequisiteOutputProjectionContract(t *testing.T) { digest := "sha256:" + strings.Repeat("4", 64) job := validJob() diff --git a/internal/runtime/containers.go b/internal/runtime/containers.go index d4e1ece3..774e26ce 100644 --- a/internal/runtime/containers.go +++ b/internal/runtime/containers.go @@ -170,20 +170,12 @@ func (r Runner) startJobContainerOrdered(ctx context.Context, processor *command } } workflowFailure = true - if len(services) != 0 || spec != nil && len(spec.Volumes) != 0 { + if len(services) != 0 || spec != nil { volumes, volumeErr := boundedDockerOutput(ctx, env, docker, "volume", "ls", "--quiet") if volumeErr != nil { return nil, fmt.Errorf("snapshot Docker volumes: %w", volumeErr) } b.existingVolumes = lineSet(volumes) - if spec != nil { - for _, volume := range spec.Volumes { - name := containerpolicy.JobVolumeName(volume) - if b.existingVolumes[name] { - return nil, fmt.Errorf("job container volume %q already exists", name) - } - } - } } if spec != nil { if err = r.pullContainerImage(ctx, processor, env, docker, spec.Image); err != nil { @@ -334,25 +326,34 @@ func (r Runner) startJobContainerOrdered(ctx context.Context, processor *command args = append(args, "--volume", volume) } args = append(args, spec.Image, "-c", "while :; do sleep 3600; done") - if _, err = boundedDockerOutput(ctx, env, docker, args...); err != nil { - createErr := err + created, createErr := boundedDockerOutput(ctx, env, docker, args...) + reference := strings.TrimSpace(created) + if reference != "" { + b.container = reference + } + if createErr != nil { reconcileCtx, cancelReconcile := context.WithTimeout(context.WithoutCancel(ctx), r.cleanupTimeout()) - exists, queryErr := b.jobContainerExists(reconcileCtx) - if queryErr != nil { - createErr = errors.Join(createErr, fmt.Errorf("reconcile job container create: %w", queryErr)) + if reference == "" { + reference, err = b.reconcileCreatedJob(reconcileCtx) + if err != nil { + createErr = errors.Join(createErr, err) + } else if reference != "" { + b.container = reference + } } - if exists && len(spec.Volumes) != 0 { + if reference != "" { if trackErr := b.trackContainerVolumes(reconcileCtx, "job container", b.container); trackErr != nil { createErr = errors.Join(createErr, trackErr) } } + if trackErr := b.trackCreatedVolumes(reconcileCtx); trackErr != nil { + createErr = errors.Join(createErr, trackErr) + } cancelReconcile() return nil, fmt.Errorf("create job container: %w", createErr) } - if len(spec.Volumes) != 0 { - if err = b.trackJobContainerVolumes(ctx); err != nil { - return nil, err - } + if err = b.trackJobContainerVolumes(ctx); err != nil { + return nil, err } if _, err = boundedDockerOutput(ctx, env, docker, "start", b.container); err != nil { return nil, fmt.Errorf("start job container: %w", err) @@ -475,6 +476,33 @@ func (b *jobContainerBackend) reconcileCreatedService(ctx context.Context, index return nil } +func (b *jobContainerBackend) reconcileCreatedJob(ctx context.Context) (string, error) { + output, err := boundedDockerOutput(ctx, b.env, b.docker, "ps", "--all", "--quiet", "--no-trunc", "--filter", "label="+b.owner) + if err != nil { + return "", fmt.Errorf("reconcile ambiguous job container create: %w", err) + } + known := map[string]bool{} + for _, service := range b.services { + if service.created { + known[service.name] = true + } + } + var unmatched []string + for reference := range lineSet(output) { + if !known[reference] { + unmatched = append(unmatched, reference) + } + } + slices.Sort(unmatched) + if len(unmatched) > 1 { + return "", fmt.Errorf("reconcile ambiguous job container create: found %d new owned containers", len(unmatched)) + } + if len(unmatched) == 1 { + return unmatched[0], nil + } + return "", nil +} + func (b *jobContainerBackend) trackServiceVolumes(ctx context.Context, serviceID, reference string) error { return b.trackContainerVolumes(ctx, fmt.Sprintf("service %q", serviceID), reference) } @@ -482,7 +510,11 @@ func (b *jobContainerBackend) trackServiceVolumes(ctx context.Context, serviceID func (b *jobContainerBackend) trackJobContainerVolumes(parent context.Context) error { ctx, cancel := context.WithTimeout(context.WithoutCancel(parent), b.runner.cleanupTimeout()) defer cancel() - return b.trackContainerVolumes(ctx, "job container", b.container) + inspectErr := b.trackContainerVolumes(ctx, "job container", b.container) + if inspectErr == nil { + return nil + } + return errors.Join(inspectErr, b.trackCreatedVolumes(ctx)) } func (b *jobContainerBackend) trackContainerVolumes(ctx context.Context, subject, reference string) error { @@ -491,11 +523,30 @@ func (b *jobContainerBackend) trackContainerVolumes(ctx context.Context, subject if err != nil { return fmt.Errorf("inspect %s volumes: %w", subject, err) } + volumes := make([]string, 0) + for volume := range lineSet(output) { + if !b.existingVolumes[volume] && !slices.Contains(b.ownedVolumes, volume) { + volumes = append(volumes, volume) + } + } + slices.Sort(volumes) + b.ownedVolumes = append(b.ownedVolumes, volumes...) + return nil +} + +func (b *jobContainerBackend) trackCreatedVolumes(ctx context.Context) error { + output, err := boundedDockerOutput(ctx, b.env, b.docker, "volume", "ls", "--quiet") + if err != nil { + return fmt.Errorf("reconcile created Docker volumes: %w", err) + } + volumes := make([]string, 0) for volume := range lineSet(output) { if !b.existingVolumes[volume] && !slices.Contains(b.ownedVolumes, volume) { - b.ownedVolumes = append(b.ownedVolumes, volume) + volumes = append(volumes, volume) } } + slices.Sort(volumes) + b.ownedVolumes = append(b.ownedVolumes, volumes...) return nil } @@ -796,12 +847,12 @@ func (b *jobContainerBackend) cleanup(parent context.Context) error { var out string var queryErr error if b.container != "" { - out, queryErr = boundedDockerOutput(ctx, b.env, b.docker, "ps", "--all", "--quiet", "--filter", "label="+b.owner, "--filter", "name=^/"+b.container+"$") + out, queryErr = boundedDockerOutput(ctx, b.env, b.docker, "ps", "--all", "--quiet", "--filter", "id="+b.container) if queryErr != nil { err = errors.Join(err, fmt.Errorf("query job container: %w", queryErr)) } if queryErr != nil || strings.TrimSpace(out) != "" { - _, e := boundedDockerOutput(ctx, b.env, b.docker, "rm", "--force", "--volumes", b.container) + _, e := boundedDockerOutput(ctx, b.env, b.docker, "rm", "--force", b.container) if e != nil { err = errors.Join(err, fmt.Errorf("remove job container: %w", e)) } @@ -886,11 +937,6 @@ func (b *jobContainerBackend) serviceContainerExists(ctx context.Context, id str return strings.TrimSpace(out) != "", err } -func (b *jobContainerBackend) jobContainerExists(ctx context.Context) (bool, error) { - out, err := boundedDockerOutput(ctx, b.env, b.docker, "ps", "--all", "--quiet", "--filter", "label="+b.owner, "--filter", "name=^/"+b.container+"$") - return strings.TrimSpace(out) != "", err -} - func removeDockerConfig(path string) error { if err := os.RemoveAll(path); err != nil { return fmt.Errorf("remove private Docker configuration: %w", err) diff --git a/internal/runtime/containers_test.go b/internal/runtime/containers_test.go index fe26feda..f7ad7a43 100644 --- a/internal/runtime/containers_test.go +++ b/internal/runtime/containers_test.go @@ -230,12 +230,17 @@ func TestJobContainerFakeDockerProcess(t *testing.T) { if args[i] == "--name" { name = args[i+1] } + if strings.HasPrefix(args[i], "--name=") { + name = strings.TrimPrefix(args[i], "--name=") + } if args[i] == "--publish" { publications = append(publications, args[i+1]) } if args[i] == "--volume" || args[i] == "-v" { parts := strings.Split(args[i+1], ":") - if len(parts) > 1 && parts[0] != "" && !filepath.IsAbs(parts[0]) { + if len(parts) == 1 || len(parts) == 2 && (parts[1] == "ro" || parts[1] == "rw") { + volumes = append(volumes, "anonymous-volume") + } else if parts[0] != "" && !filepath.IsAbs(parts[0]) { volumes = append(volumes, parts[0]) } } @@ -265,6 +270,9 @@ func TestJobContainerFakeDockerProcess(t *testing.T) { } if len(volumes) != 0 { _ = os.WriteFile(filepath.Join(root, "volumes-"+name), []byte(strings.Join(volumes, "\n")), 0o600) + vf, _ := os.OpenFile(filepath.Join(root, "current-volumes"), os.O_CREATE|os.O_APPEND|os.O_WRONLY, 0o600) + _, _ = vf.WriteString(strings.Join(volumes, "\n") + "\n") + _ = vf.Close() } if scenario == "fail-later-service-create" && strings.HasPrefix(name, "buildkite-gha-service-") { counter := filepath.Join(root, "service-create-count") @@ -352,7 +360,7 @@ func TestJobContainerFakeDockerProcess(t *testing.T) { fmt.Print("diagnostic sibling-secret\n") os.Exit(0) case "ps": - if scenario == "query-fail" && strings.Contains(strings.Join(args, " "), "name=") { + if scenario == "query-fail" && (strings.Contains(strings.Join(args, " "), "name=") || strings.Contains(strings.Join(args, " "), "id=docker-id-buildkite-gha-job-")) { os.Exit(43) } joined := strings.Join(args, " ") @@ -474,6 +482,15 @@ func TestJobContainerFakeDockerProcess(t *testing.T) { if data, err := os.ReadFile(filepath.Join(root, "existing-volumes")); err == nil { fmt.Print(string(data)) } + if data, err := os.ReadFile(filepath.Join(root, "current-volumes")); err == nil { + removed, _ := os.ReadFile(filepath.Join(root, "removed-volumes")) + removedSet := lineSet(string(removed)) + for volume := range lineSet(string(data)) { + if !removedSet[volume] { + fmt.Println(volume) + } + } + } if scenario == "volume-leftover" { if data, err := os.ReadFile(filepath.Join(root, "removed-volumes")); err == nil { fmt.Print(string(data)) @@ -648,10 +665,10 @@ func TestRunJobContainerLifecycleAndEnvironment(t *testing.T) { t.Fatalf("workspace mode %o", m.Mode().Perm()) } calls := f.calls(t) - if len(calls) != 13 { + if len(calls) != 15 { t.Fatalf("calls=%d: %#v", len(calls), calls) } - want := []string{"pull", "network create", "create", "start", "exec", "exec", "exec", "ps", "rm", "network ls", "network rm", "ps", "network ls"} + want := []string{"volume ls", "pull", "network create", "create", "inspect", "start", "exec", "exec", "exec", "ps", "rm", "network ls", "network rm", "ps", "network ls"} for i, c := range calls { if c.ConfigMode != 0o700 || c.ConfigEntries != 0 || c.Host != nil || c.Context != nil || c.Builder != nil || c.Kit != nil { t.Fatalf("private env call %d: %#v", i, c) @@ -660,7 +677,7 @@ func TestRunJobContainerLifecycleAndEnvironment(t *testing.T) { t.Fatalf("call %d=%q want %q", i, c.Args, want[i]) } } - create := strings.Join(calls[2].Args, " ") + create := strings.Join(calls[3].Args, " ") for _, s := range []string{"target=" + jobContainerWorkspace, "target=" + jobContainerTemp, "target=" + jobContainerRuntime + ",readonly", "--workdir " + jobContainerWorkspace} { if !strings.Contains(create, s) { t.Errorf("create missing %q", s) @@ -870,7 +887,7 @@ func TestRunJobContainerServicesLifecycleAndArguments(t *testing.T) { // network, and verification. joined := fmt.Sprint(calls) arm, zrm := strings.Index(joined, "rm --force --volumes docker-id-"+creates[0].Args[2]), strings.Index(joined, "rm --force --volumes docker-id-"+creates[1].Args[2]) - jobrm, netrm := strings.Index(joined, "rm --force --volumes "+creates[2].Args[2]), strings.Index(joined, "network rm") + jobrm, netrm := strings.Index(joined, "rm --force docker-id-"+creates[2].Args[2]), strings.Index(joined, "network rm") if jobrm < 0 || jobrm >= arm || arm >= zrm || zrm >= netrm { t.Fatalf("cleanup order: %s", joined) } @@ -882,8 +899,8 @@ func TestRunJobContainerOptionsAndVolumesExactArguments(t *testing.T) { j := jobContainerPlan(t, w, nil) j.Container.Env = map[string]string{"Z": "last", "A": "first"} j.Container.Ports = []string{"8080"} - j.Container.Volumes = []string{"cache:/cache:ro"} - j.Container.Options = "--cpus 1.5 --memory=2g --shm-size 64m" + j.Container.Volumes = []string{"cache:/cache:ro", "/anonymous", w + ":/host-workspace:ro"} + j.Container.Options = `--privileged --label "description=two words" --mount type=tmpfs,dst=/scratch --volume option-cache:/option` j.Container.Image = "node:24" if _, err := (Runner{Docker: f.path, RuntimeExecutable: os.Args[0]}).RunJob(t.Context(), j, w); err != nil { t.Fatal(err) @@ -902,15 +919,16 @@ func TestRunJobContainerOptionsAndVolumesExactArguments(t *testing.T) { "--mount", call.Args[12], "--mount", "type=bind,source=" + runtimeExecutable + ",target=" + jobContainerRuntime + ",readonly", "--workdir", jobContainerWorkspace, "--entrypoint", "sh", - "--cpus", "1.5", "--memory=2g", "--shm-size", "64m", - "--env", "A=first", "--env", "Z=last", "--publish", "8080", "--volume", "cache:/cache:ro", + "--privileged", "--label", "description=two words", "--mount", "type=tmpfs,dst=/scratch", "--volume", "option-cache:/option", + "--env", "A=first", "--env", "Z=last", "--publish", "8080", + "--volume", "cache:/cache:ro", "--volume", "/anonymous", "--volume", w + ":/host-workspace:ro", "node:24", "-c", "while :; do sleep 3600; done", } if !slices.Equal(call.Args, want) { t.Fatalf("job container create argv = %#v\nwant = %#v", call.Args, want) } removed, readErr := os.ReadFile(filepath.Join(f.root, "removed-volumes")) - if readErr != nil || strings.TrimSpace(string(removed)) != "cache" { + if readErr != nil || strings.TrimSpace(string(removed)) != "anonymous-volume\ncache\noption-cache" { t.Fatalf("removed job volumes = %q, %v", removed, readErr) } return @@ -918,32 +936,51 @@ func TestRunJobContainerOptionsAndVolumesExactArguments(t *testing.T) { t.Fatal("job container create call not found") } -func TestJobContainerOptionsRejectDockerAuthority(t *testing.T) { - for _, options := range []string{"--privileged", "--network host", "--volume /:/host", "--cap-add SYS_ADMIN", "--device /dev/kvm", "--pid=host", "--entrypoint sh"} { +func TestRunJobContainerOptionNameUsesCreatedReference(t *testing.T) { + f := newJobDocker(t, "") + b, err := (Runner{Docker: f.path, RuntimeExecutable: os.Args[0]}).startJobContainer( + t.Context(), newCommandProcessor(io.Discard, io.Discard), t.TempDir(), t.TempDir(), + &plan.Container{Image: "node:24", Options: "--name custom-job"}, nil, + ) + if err != nil { + t.Fatal(err) + } + if err := b.cleanup(t.Context()); err != nil { + t.Fatal(err) + } + calls := f.calls(t) + if jobDockerCallIndex(calls, "start", "docker-id-custom-job") < 0 || jobDockerCallIndex(calls, "rm", "--force", "docker-id-custom-job") < 0 { + t.Fatalf("custom job reference was not used for lifecycle: %#v", calls) + } +} + +func TestJobContainerOptionsRejectRunnerOwnedOverrides(t *testing.T) { + for _, options := range []string{"--network host", "--network=host", "--net host", "--net=host", "--entrypoint sh", "--entrypoint=sh"} { t.Run(options, func(t *testing.T) { if _, err := containerpolicy.JobOptions(options); err == nil { - t.Fatal("JobOptions() accepted Docker authority") + t.Fatal("JobOptions() accepted a runner-owned override") } }) } } -func TestJobContainerRejectsPreexistingNamedVolume(t *testing.T) { +func TestJobContainerUsesButDoesNotRemovePreexistingNamedVolume(t *testing.T) { f := newJobDocker(t, "") if err := os.WriteFile(filepath.Join(f.root, "existing-volumes"), []byte("cache\n"), 0o600); err != nil { t.Fatal(err) } - _, err := (Runner{Docker: f.path, RuntimeExecutable: os.Args[0]}).startJobContainer( + b, err := (Runner{Docker: f.path, RuntimeExecutable: os.Args[0]}).startJobContainer( t.Context(), newCommandProcessor(io.Discard, io.Discard), t.TempDir(), t.TempDir(), &plan.Container{Image: "node:24", Volumes: []string{"cache:/cache"}}, nil, ) - if err == nil || !strings.Contains(err.Error(), `job container volume "cache" already exists`) { + if err != nil { t.Fatalf("startJobContainer() error = %v", err) } - for _, call := range f.calls(t) { - if len(call.Args) != 0 && call.Args[0] == "create" { - t.Fatalf("pre-existing volume reached docker create: %#v", call.Args) - } + if err := b.cleanup(t.Context()); err != nil { + t.Fatal(err) + } + if _, err := os.Stat(filepath.Join(f.root, "removed-volumes")); !errors.Is(err, os.ErrNotExist) { + t.Fatal("cleanup took ownership of a pre-existing named volume") } } @@ -1486,7 +1523,7 @@ func TestRunJobContainerAmbiguousCreateFailureCleansNamedVolume(t *testing.T) { } } -func TestRunJobContainerCreateCancellationCleansNamedVolume(t *testing.T) { +func TestRunJobContainerCreateCancellationCleansAnonymousVolume(t *testing.T) { f := newJobDocker(t, "block-job-create") w, tmp := t.TempDir(), t.TempDir() ctx, cancel := context.WithCancel(t.Context()) @@ -1494,7 +1531,7 @@ func TestRunJobContainerCreateCancellationCleansNamedVolume(t *testing.T) { go func() { _, err := (Runner{Docker: f.path, RuntimeExecutable: os.Args[0]}).startJobContainer( ctx, newCommandProcessor(io.Discard, io.Discard), w, tmp, - &plan.Container{Image: "alpine", Volumes: []string{"cache:/cache"}}, nil, + &plan.Container{Image: "alpine", Volumes: []string{"/cache"}}, nil, ) done <- err }() @@ -1514,7 +1551,7 @@ func TestRunJobContainerCreateCancellationCleansNamedVolume(t *testing.T) { t.Fatalf("startJobContainer() error = %v", err) } removed, readErr := os.ReadFile(filepath.Join(f.root, "removed-volumes")) - if readErr != nil || strings.TrimSpace(string(removed)) != "cache" { + if readErr != nil || strings.TrimSpace(string(removed)) != "anonymous-volume" { t.Fatalf("removed volumes = %q, %v", removed, readErr) } } @@ -1680,10 +1717,10 @@ func TestRunJobContainerSetupFailuresCleanOwnedResources(t *testing.T) { removedContainer, removedNetwork := false, false for _, c := range calls { joined := strings.Join(c.Args, " ") - if strings.HasPrefix(joined, "rm ") && !strings.Contains(joined, "buildkite-gha-job-") { + if strings.HasPrefix(joined, "rm ") && !strings.Contains(joined, "buildkite-gha-job-") && !strings.Contains(joined, "job-container-id") { t.Fatalf("unowned removal %q", joined) } - removedContainer = removedContainer || strings.HasPrefix(joined, "rm --force --volumes buildkite-gha-job-") + removedContainer = removedContainer || strings.HasPrefix(joined, "rm --force docker-id-buildkite-gha-job-") || joined == "rm --force job-container-id" if strings.HasPrefix(joined, "network rm") && !strings.Contains(joined, "buildkite-gha-network-") { t.Fatalf("unowned removal %q", joined) } @@ -1712,7 +1749,7 @@ func TestRunJobContainerCleanupQueryFailureStillRemovesExactResources(t *testing t.Fatalf("error=%v", err) } joined := fmt.Sprint(f.calls(t)) - if !strings.Contains(joined, "rm --force --volumes buildkite-gha-job-") || !strings.Contains(joined, "network rm buildkite-gha-network-") { + if !strings.Contains(joined, "rm --force docker-id-buildkite-gha-job-") || !strings.Contains(joined, "network rm buildkite-gha-network-") { t.Fatalf("cleanup %s", joined) } } @@ -1970,7 +2007,7 @@ func TestRunJobContainerNodeProbeFailureCleansOwnedResources(t *testing.T) { } calls := f.calls(t) joined := fmt.Sprint(calls) - if !strings.Contains(joined, "rm --force --volumes buildkite-gha-job-") || !strings.Contains(joined, "network rm buildkite-gha-network-") { + if !strings.Contains(joined, "rm --force docker-id-buildkite-gha-job-") || !strings.Contains(joined, "network rm buildkite-gha-network-") { t.Fatalf("owned resources not cleaned: %s", joined) } for _, call := range calls { @@ -2141,7 +2178,7 @@ func TestRunJobContainerReadOnlyMountProbeFailureCleansOwnedResources(t *testing } calls := f.calls(t) joined := fmt.Sprint(calls) - if !strings.Contains(joined, "rm --force --volumes buildkite-gha-job-") || !strings.Contains(joined, "network rm buildkite-gha-network-") { + if !strings.Contains(joined, "rm --force docker-id-buildkite-gha-job-") || !strings.Contains(joined, "network rm buildkite-gha-network-") { t.Fatalf("owned resources not cleaned: %s", joined) } if len(calls) == 0 { diff --git a/internal/workflow/parse.go b/internal/workflow/parse.go index 791e5f77..977cddd6 100644 --- a/internal/workflow/parse.go +++ b/internal/workflow/parse.go @@ -533,7 +533,6 @@ func validateRawContainer(path string, node *yaml.Node, service bool) (rawServic return extra, nil, rawError(path, volumes, "invalid "+subject+" volumes") } seen := map[string]bool{} - seenTargets := map[string]bool{} for _, volume := range volumes.Content { if volume.Kind != yaml.ScalarNode || len(volume.Value) > 4096 || strings.ContainsAny(volume.Value, "\x00\r\n") || seen[volume.Value] { return extra, nil, rawError(path, volume, "invalid or repeated "+subject+" volume") @@ -545,11 +544,6 @@ func validateRawContainer(path string, node *yaml.Node, service bool) (rawServic if err := containerpolicy.ValidateJobVolume(volume.Value); err != nil { return extra, nil, rawError(path, volume, "invalid container volume: "+err.Error()) } - target := containerpolicy.JobVolumeTarget(volume.Value) - if seenTargets[target] { - return extra, nil, rawError(path, volume, "repeated container volume target "+target) - } - seenTargets[target] = true } seen[volume.Value] = true extra.Volumes = append(extra.Volumes, volume.Value) @@ -559,7 +553,7 @@ func validateRawContainer(path string, node *yaml.Node, service bool) (rawServic limit := 65536 subject := "service container" if !service { - limit = 4096 + limit = containerpolicy.MaxJobOptionsLength subject = "container" } if options.Kind != yaml.ScalarNode || len(options.Value) > limit || strings.ContainsAny(options.Value, "\x00\r\n") { diff --git a/internal/workflow/parse_test.go b/internal/workflow/parse_test.go index 22bf36d4..2bb4d149 100644 --- a/internal/workflow/parse_test.go +++ b/internal/workflow/parse_test.go @@ -252,13 +252,13 @@ func TestParseRejectsJobPermissionAliases(t *testing.T) { } func TestParseOwnsLiteralContainersInDeclarationOrder(t *testing.T) { - source := []byte("on: push\njobs:\n test:\n runs-on: ubuntu-latest\n container:\n image: node:24\n env: {NODE_ENV: test}\n ports: [8080]\n volumes: ['cache:/cache:ro']\n options: --cpus 2 --memory=1g\n services:\n zed: {image: redis:7}\n alpha: {image: 'registry.example:5000/team/postgres:16', ports: ['5432:5432']}\n steps:\n - run: true\n") + source := []byte("on: push\njobs:\n test:\n runs-on: ubuntu-latest\n container:\n image: node:24\n env: {NODE_ENV: test}\n ports: [8080]\n volumes: ['cache:/cache:ro', '/anonymous', '/srv/data:/data']\n options: --privileged --label \"description=two words\"\n services:\n zed: {image: redis:7}\n alpha: {image: 'registry.example:5000/team/postgres:16', ports: ['5432:5432']}\n steps:\n - run: true\n") parsed, err := Parse("containers.yml", source) if err != nil { t.Fatal(err) } job := parsed.Jobs[0] - if job.Container == nil || job.Container.Image != "node:24" || job.Container.Env["NODE_ENV"] != "test" || !slices.Equal(job.Container.Volumes, []string{"cache:/cache:ro"}) || job.Container.Options != "--cpus 2 --memory=1g" || len(job.Services) != 2 || job.Services[0].Name != "zed" || job.Services[1].Name != "alpha" || job.Services[1].Container.Image != "registry.example:5000/team/postgres:16" { + if job.Container == nil || job.Container.Image != "node:24" || job.Container.Env["NODE_ENV"] != "test" || !slices.Equal(job.Container.Volumes, []string{"cache:/cache:ro", "/anonymous", "/srv/data:/data"}) || job.Container.Options != `--privileged --label "description=two words"` || len(job.Services) != 2 || job.Services[0].Name != "zed" || job.Services[1].Name != "alpha" || job.Services[1].Container.Image != "registry.example:5000/team/postgres:16" { t.Fatalf("owned containers = %#v / %#v", job.Container, job.Services) } } @@ -359,28 +359,30 @@ func TestParseRejectsUnsupportedContainerControls(t *testing.T) { } } -func TestParseRejectsUnsafeJobContainerOptionsAndVolumes(t *testing.T) { +func TestParseRejectsUnsupportedJobContainerOptionsAndVolumes(t *testing.T) { for name, body := range map[string]string{ - "privileged option": "options: --privileged", "network option": "options: --network host", "entrypoint option": "options: --entrypoint sh", "option expression": "options: --cpus ${{ matrix.cpus }}", - "host bind": "volumes: ['/tmp:/data']", - "short volume name": "volumes: ['v:/data']", - "duplicate target": "volumes: ['one:/data', 'two:/data']", - "workspace overlap": "volumes: ['cache:/__w/repo']", "volume expression": "volumes: ['${{ matrix.name }}:/data']", "unsupported volume mode": "volumes: ['cache:/data:z']", } { t.Run(name, func(t *testing.T) { source := []byte("on: push\njobs:\n test:\n runs-on: ubuntu-latest\n container:\n image: node:24\n " + body + "\n steps: [{run: true}]\n") if _, err := Parse("containers.yml", source); err == nil { - t.Fatal("Parse() accepted unsafe job container control") + t.Fatal("Parse() accepted unsupported job container control") } }) } } +func TestParseAcceptsGitHubJobContainerOptionsAndVolumes(t *testing.T) { + source := []byte("on: push\njobs:\n test:\n runs-on: ubuntu-latest\n container:\n image: node:24\n options: --privileged --cap-add SYS_ADMIN\n volumes: ['v:/data', '/anonymous', '/tmp:/host', 'one:/same', 'two:/same', 'cache:/__w/repo']\n steps: [{run: true}]\n") + if _, err := Parse("containers.yml", source); err != nil { + t.Fatalf("Parse() rejected GitHub-compatible job container controls: %v", err) + } +} + func TestContainerValidationIsScopedAndSourceLocated(t *testing.T) { unrelated := []byte("on: push\njobs:\n test:\n runs-on: ubuntu-latest\n steps:\n - uses: owner/action@v1\n with: {image: node:24, options: --privileged}\n") if _, err := Parse("scoped.yml", unrelated); err != nil { From 51be8800f5de32f9419449a8920e4b471fb35135 Mon Sep 17 00:00:00 2001 From: Amp Date: Sat, 22 Aug 2026 09:16:12 +0000 Subject: [PATCH 08/15] Align job container plan schema Amp-Thread-ID: https://ampcode.com/threads/T-01a02836-fd56-769a-b44a-a7412431fd1d Co-authored-by: Lachlan Donald --- internal/compiler/compiler_test.go | 1 + schemas/job-plan.schema.json | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/internal/compiler/compiler_test.go b/internal/compiler/compiler_test.go index 87319d93..58569d64 100644 --- a/internal/compiler/compiler_test.go +++ b/internal/compiler/compiler_test.go @@ -4181,6 +4181,7 @@ func TestCompilePlansEmitV8ForContainers(t *testing.T) { if len(plans) != 1 || plans[0].Schema != plan.Schema || plans[0].Container == nil || !slices.Equal(plans[0].Container.Volumes, []string{"cache:/cache:ro", "/anonymous", "/srv/data:/data"}) || plans[0].Container.Options != `--privileged --label "description=two words"` || len(plans[0].Services) != 1 || !slices.Equal(plans[0].RequiredCapabilities, []string{"docker", "network"}) { t.Fatalf("container plan = %#v", plans) } + validateCompiledPlansAgainstSchema(t, plans) } func TestCompilePlansResolveJobContainerImageExpressions(t *testing.T) { diff --git a/schemas/job-plan.schema.json b/schemas/job-plan.schema.json index 8d6595b1..3bfb7a8d 100644 --- a/schemas/job-plan.schema.json +++ b/schemas/job-plan.schema.json @@ -117,7 +117,7 @@ {"if": {"properties": {"source": {"const": "github"}}, "required": ["source"]}, "then": {"required": ["repository", "requested_ref", "commit"]}} ] }, - "container": {"type": "object", "additionalProperties": false, "required": ["image"], "properties": {"image": {"$ref": "#/$defs/image"}, "env": {"$ref": "#/$defs/containerEnv"}, "ports": {"type": "array", "maxItems": 128, "uniqueItems": true, "items": {"type": "string", "pattern": "^(?:[1-9][0-9]{0,3}|[1-5][0-9]{4}|6[0-4][0-9]{3}|65[0-4][0-9]{2}|655[0-2][0-9]|6553[0-5])(?::(?:[1-9][0-9]{0,3}|[1-5][0-9]{4}|6[0-4][0-9]{3}|65[0-4][0-9]{2}|655[0-2][0-9]|6553[0-5]))?(?:/(?:tcp|udp))?$"}}, "volumes": {"type": "array", "maxItems": 32, "uniqueItems": true, "items": {"type": "string", "minLength": 3, "maxLength": 4096, "pattern": "^[^\\x00-\\x1f\\x7f]+$"}}, "options": {"type": "string", "maxLength": 4096, "pattern": "^[^\\x00-\\x1f\\x7f\\\"]*$"}}}, + "container": {"type": "object", "additionalProperties": false, "required": ["image"], "properties": {"image": {"$ref": "#/$defs/image"}, "env": {"$ref": "#/$defs/containerEnv"}, "ports": {"type": "array", "maxItems": 128, "uniqueItems": true, "items": {"type": "string", "pattern": "^(?:[1-9][0-9]{0,3}|[1-5][0-9]{4}|6[0-4][0-9]{3}|65[0-4][0-9]{2}|655[0-2][0-9]|6553[0-5])(?::(?:[1-9][0-9]{0,3}|[1-5][0-9]{4}|6[0-4][0-9]{3}|65[0-4][0-9]{2}|655[0-2][0-9]|6553[0-5]))?(?:/(?:tcp|udp))?$"}}, "volumes": {"type": "array", "maxItems": 128, "uniqueItems": true, "items": {"type": "string", "minLength": 1, "maxLength": 4096, "pattern": "^[^\\x00-\\x1f\\x7f]+$"}}, "options": {"type": "string", "maxLength": 65536, "pattern": "^[^\\x00\\r\\n]*$"}}}, "serviceContainer": {"type": "object", "additionalProperties": false, "required": ["image"], "properties": {"image": {"type": "string", "minLength": 1, "maxLength": 512}, "credentials": {"type": "object", "additionalProperties": false, "required": ["username", "password"], "properties": {"username": {"type": "string", "maxLength": 65536}, "password": {"type": "string", "maxLength": 65536}}}, "env": {"$ref": "#/$defs/containerEnv"}, "ports": {"type": "array", "maxItems": 128, "uniqueItems": true, "items": {"type": "string", "minLength": 1, "maxLength": 4096, "pattern": "^[^\\x00-\\x1f\\x7f]+$"}}, "volumes": {"type": "array", "maxItems": 128, "uniqueItems": true, "items": {"type": "string", "minLength": 1, "maxLength": 4096, "pattern": "^[^\\x00-\\x1f\\x7f]+$"}}, "options": {"type": "string", "maxLength": 65536}, "command": {"type": "string", "maxLength": 65536}, "entrypoint": {"type": "string", "maxLength": 4096}}}, "containerEnv": {"type": "object", "maxProperties": 256, "propertyNames": {"pattern": "^[A-Za-z_][A-Za-z0-9_]{0,254}$"}, "additionalProperties": {"type": "string", "maxLength": 65536}}, "image": {"type": "string", "minLength": 1, "maxLength": 512, "pattern": "^(?:(?:[a-z0-9]+(?:[._-][a-z0-9]+)*|\\[[0-9a-f:]+\\])(?::(?:[1-9][0-9]{0,3}|[1-5][0-9]{4}|6[0-4][0-9]{3}|65[0-4][0-9]{2}|655[0-2][0-9]|6553[0-5]))?/)?[a-z0-9]+(?:[._-][a-z0-9]+)*(?:/[a-z0-9]+(?:[._-][a-z0-9]+)*)*(?::[A-Za-z0-9_][A-Za-z0-9_.-]{0,127})?(?:@sha256:[0-9a-f]{64})?$"}, From 77a101a641aa3355d287b983e727b3f77d1d9c4a Mon Sep 17 00:00:00 2001 From: Amp Date: Sat, 22 Aug 2026 09:27:44 +0000 Subject: [PATCH 09/15] Retry ambiguous job container cleanup Amp-Thread-ID: https://ampcode.com/threads/T-01a02836-fd56-769a-b44a-a7412431fd1d Co-authored-by: Lachlan Donald --- internal/runtime/containers.go | 30 ++++++++++++++++++++-------- internal/runtime/containers_test.go | 31 ++++++++++++++++++++++++++++- 2 files changed, 52 insertions(+), 9 deletions(-) diff --git a/internal/runtime/containers.go b/internal/runtime/containers.go index 774e26ce..c27d369c 100644 --- a/internal/runtime/containers.go +++ b/internal/runtime/containers.go @@ -49,6 +49,7 @@ type jobContainerBackend struct { env map[string]string config string owner, container, network string + containerCreated bool services []serviceContainer workspace, temp string imagePATH string @@ -330,6 +331,7 @@ func (r Runner) startJobContainerOrdered(ctx context.Context, processor *command reference := strings.TrimSpace(created) if reference != "" { b.container = reference + b.containerCreated = true } if createErr != nil { reconcileCtx, cancelReconcile := context.WithTimeout(context.WithoutCancel(ctx), r.cleanupTimeout()) @@ -339,6 +341,7 @@ func (r Runner) startJobContainerOrdered(ctx context.Context, processor *command createErr = errors.Join(createErr, err) } else if reference != "" { b.container = reference + b.containerCreated = true } } if reference != "" { @@ -847,14 +850,25 @@ func (b *jobContainerBackend) cleanup(parent context.Context) error { var out string var queryErr error if b.container != "" { - out, queryErr = boundedDockerOutput(ctx, b.env, b.docker, "ps", "--all", "--quiet", "--filter", "id="+b.container) - if queryErr != nil { - err = errors.Join(err, fmt.Errorf("query job container: %w", queryErr)) - } - if queryErr != nil || strings.TrimSpace(out) != "" { - _, e := boundedDockerOutput(ctx, b.env, b.docker, "rm", "--force", b.container) - if e != nil { - err = errors.Join(err, fmt.Errorf("remove job container: %w", e)) + if !b.containerCreated { + reference, reconcileErr := b.reconcileCreatedJob(ctx) + if reconcileErr != nil { + err = errors.Join(err, reconcileErr) + } else if reference != "" { + b.container = reference + b.containerCreated = true + } + } + if b.containerCreated { + out, queryErr = boundedDockerOutput(ctx, b.env, b.docker, "ps", "--all", "--quiet", "--filter", "id="+b.container) + if queryErr != nil { + err = errors.Join(err, fmt.Errorf("query job container: %w", queryErr)) + } + if queryErr != nil || strings.TrimSpace(out) != "" { + _, e := boundedDockerOutput(ctx, b.env, b.docker, "rm", "--force", b.container) + if e != nil { + err = errors.Join(err, fmt.Errorf("remove job container: %w", e)) + } } } } diff --git a/internal/runtime/containers_test.go b/internal/runtime/containers_test.go index f7ad7a43..1dc18d0a 100644 --- a/internal/runtime/containers_test.go +++ b/internal/runtime/containers_test.go @@ -284,7 +284,7 @@ func TestJobContainerFakeDockerProcess(t *testing.T) { os.Exit(42) } } - if scenario == "fail-create" { + if scenario == "fail-create" || scenario == "fail-create-reconcile-once" { os.Exit(42) } if scenario == "block-job-create" && !strings.HasPrefix(name, "buildkite-gha-service-") { @@ -360,6 +360,13 @@ func TestJobContainerFakeDockerProcess(t *testing.T) { fmt.Print("diagnostic sibling-secret\n") os.Exit(0) case "ps": + if scenario == "fail-create-reconcile-once" && slices.Contains(args, "--no-trunc") { + marker := filepath.Join(root, "failed-job-reconcile") + if _, err := os.Stat(marker); errors.Is(err, os.ErrNotExist) { + _ = os.WriteFile(marker, nil, 0o600) + os.Exit(43) + } + } if scenario == "query-fail" && (strings.Contains(strings.Join(args, " "), "name=") || strings.Contains(strings.Join(args, " "), "id=docker-id-buildkite-gha-job-")) { os.Exit(43) } @@ -1523,6 +1530,28 @@ func TestRunJobContainerAmbiguousCreateFailureCleansNamedVolume(t *testing.T) { } } +func TestRunJobContainerCleanupRetriesAmbiguousCreateReconciliation(t *testing.T) { + f := newJobDocker(t, "fail-create-reconcile-once") + _, err := (Runner{Docker: f.path, RuntimeExecutable: os.Args[0]}).startJobContainer( + t.Context(), newCommandProcessor(io.Discard, io.Discard), t.TempDir(), t.TempDir(), + &plan.Container{Image: "alpine"}, nil, + ) + if err == nil || !strings.Contains(err.Error(), "create job container") { + t.Fatalf("startJobContainer() error = %v", err) + } + reconciles := 0 + removed := false + for _, call := range f.calls(t) { + if len(call.Args) != 0 && call.Args[0] == "ps" && slices.Contains(call.Args, "--no-trunc") { + reconciles++ + } + removed = removed || slices.Equal(call.Args, []string{"rm", "--force", "job-container-id"}) + } + if reconciles != 2 || !removed { + t.Fatalf("ambiguous create cleanup calls = %#v", f.calls(t)) + } +} + func TestRunJobContainerCreateCancellationCleansAnonymousVolume(t *testing.T) { f := newJobDocker(t, "block-job-create") w, tmp := t.TempDir(), t.TempDir() From d65ec6fa874fddc06b78eead4da94b37cd275eec Mon Sep 17 00:00:00 2001 From: Amp Date: Sat, 22 Aug 2026 09:41:44 +0000 Subject: [PATCH 10/15] Retry ambiguous volume cleanup Amp-Thread-ID: https://ampcode.com/threads/T-01a02836-fd56-769a-b44a-a7412431fd1d Co-authored-by: Lachlan Donald --- internal/runtime/containers.go | 3 +++ internal/runtime/containers_test.go | 14 ++++++++++++-- 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/internal/runtime/containers.go b/internal/runtime/containers.go index c27d369c..bafd5ec1 100644 --- a/internal/runtime/containers.go +++ b/internal/runtime/containers.go @@ -851,6 +851,9 @@ func (b *jobContainerBackend) cleanup(parent context.Context) error { var queryErr error if b.container != "" { if !b.containerCreated { + if trackErr := b.trackCreatedVolumes(ctx); trackErr != nil { + err = errors.Join(err, trackErr) + } reference, reconcileErr := b.reconcileCreatedJob(ctx) if reconcileErr != nil { err = errors.Join(err, reconcileErr) diff --git a/internal/runtime/containers_test.go b/internal/runtime/containers_test.go index 1dc18d0a..734aba04 100644 --- a/internal/runtime/containers_test.go +++ b/internal/runtime/containers_test.go @@ -486,6 +486,15 @@ func TestJobContainerFakeDockerProcess(t *testing.T) { _ = os.Remove(network) os.Exit(0) case "volume-ls": + if scenario == "fail-create-reconcile-once" { + if _, err := os.Stat(filepath.Join(root, "current-volumes")); err == nil { + marker := filepath.Join(root, "failed-volume-reconcile") + if _, err := os.Stat(marker); errors.Is(err, os.ErrNotExist) { + _ = os.WriteFile(marker, nil, 0o600) + os.Exit(43) + } + } + } if data, err := os.ReadFile(filepath.Join(root, "existing-volumes")); err == nil { fmt.Print(string(data)) } @@ -1534,7 +1543,7 @@ func TestRunJobContainerCleanupRetriesAmbiguousCreateReconciliation(t *testing.T f := newJobDocker(t, "fail-create-reconcile-once") _, err := (Runner{Docker: f.path, RuntimeExecutable: os.Args[0]}).startJobContainer( t.Context(), newCommandProcessor(io.Discard, io.Discard), t.TempDir(), t.TempDir(), - &plan.Container{Image: "alpine"}, nil, + &plan.Container{Image: "alpine", Volumes: []string{"cache:/cache"}}, nil, ) if err == nil || !strings.Contains(err.Error(), "create job container") { t.Fatalf("startJobContainer() error = %v", err) @@ -1547,7 +1556,8 @@ func TestRunJobContainerCleanupRetriesAmbiguousCreateReconciliation(t *testing.T } removed = removed || slices.Equal(call.Args, []string{"rm", "--force", "job-container-id"}) } - if reconciles != 2 || !removed { + removedVolumes, readErr := os.ReadFile(filepath.Join(f.root, "removed-volumes")) + if reconciles != 2 || !removed || readErr != nil || strings.TrimSpace(string(removedVolumes)) != "cache" { t.Fatalf("ambiguous create cleanup calls = %#v", f.calls(t)) } } From 330ca5c404543b71ea6ef785e9f47ca04deaebca Mon Sep 17 00:00:00 2001 From: Amp Date: Sat, 22 Aug 2026 09:50:52 +0000 Subject: [PATCH 11/15] Track volume ownership reconciliation Amp-Thread-ID: https://ampcode.com/threads/T-01a02836-fd56-769a-b44a-a7412431fd1d Co-authored-by: Lachlan Donald --- internal/runtime/containers.go | 10 +++++++++- internal/runtime/containers_test.go | 24 +++++++++++++++++++++++- 2 files changed, 32 insertions(+), 2 deletions(-) diff --git a/internal/runtime/containers.go b/internal/runtime/containers.go index bafd5ec1..da18e3a0 100644 --- a/internal/runtime/containers.go +++ b/internal/runtime/containers.go @@ -59,6 +59,7 @@ type jobContainerBackend struct { servicePorts map[string]expression.ServiceContext existingVolumes map[string]bool ownedVolumes []string + volumesTracked bool } func privateDocker(r Runner) (string, string, map[string]string, error) { @@ -351,6 +352,8 @@ func (r Runner) startJobContainerOrdered(ctx context.Context, processor *command } if trackErr := b.trackCreatedVolumes(reconcileCtx); trackErr != nil { createErr = errors.Join(createErr, trackErr) + } else { + b.volumesTracked = true } cancelReconcile() return nil, fmt.Errorf("create job container: %w", createErr) @@ -358,6 +361,7 @@ func (r Runner) startJobContainerOrdered(ctx context.Context, processor *command if err = b.trackJobContainerVolumes(ctx); err != nil { return nil, err } + b.volumesTracked = true if _, err = boundedDockerOutput(ctx, env, docker, "start", b.container); err != nil { return nil, fmt.Errorf("start job container: %w", err) } @@ -850,10 +854,14 @@ func (b *jobContainerBackend) cleanup(parent context.Context) error { var out string var queryErr error if b.container != "" { - if !b.containerCreated { + if !b.volumesTracked { if trackErr := b.trackCreatedVolumes(ctx); trackErr != nil { err = errors.Join(err, trackErr) + } else { + b.volumesTracked = true } + } + if !b.containerCreated { reference, reconcileErr := b.reconcileCreatedJob(ctx) if reconcileErr != nil { err = errors.Join(err, reconcileErr) diff --git a/internal/runtime/containers_test.go b/internal/runtime/containers_test.go index 734aba04..6a847312 100644 --- a/internal/runtime/containers_test.go +++ b/internal/runtime/containers_test.go @@ -331,6 +331,13 @@ func TestJobContainerFakeDockerProcess(t *testing.T) { os.Exit(1) } if strings.Contains(strings.Join(args, " "), ".Mounts") { + if scenario == "fail-job-volume-tracking-once" { + marker := filepath.Join(root, "failed-volume-inspect") + if _, err := os.Stat(marker); errors.Is(err, os.ErrNotExist) { + _ = os.WriteFile(marker, nil, 0o600) + os.Exit(43) + } + } data, _ := os.ReadFile(filepath.Join(root, "volumes-"+name)) fmt.Print(string(data)) os.Exit(0) @@ -486,7 +493,7 @@ func TestJobContainerFakeDockerProcess(t *testing.T) { _ = os.Remove(network) os.Exit(0) case "volume-ls": - if scenario == "fail-create-reconcile-once" { + if scenario == "fail-create-reconcile-once" || scenario == "fail-job-volume-tracking-once" { if _, err := os.Stat(filepath.Join(root, "current-volumes")); err == nil { marker := filepath.Join(root, "failed-volume-reconcile") if _, err := os.Stat(marker); errors.Is(err, os.ErrNotExist) { @@ -1562,6 +1569,21 @@ func TestRunJobContainerCleanupRetriesAmbiguousCreateReconciliation(t *testing.T } } +func TestRunJobContainerCleanupRetriesSuccessfulCreateVolumeTracking(t *testing.T) { + f := newJobDocker(t, "fail-job-volume-tracking-once") + _, err := (Runner{Docker: f.path, RuntimeExecutable: os.Args[0]}).startJobContainer( + t.Context(), newCommandProcessor(io.Discard, io.Discard), t.TempDir(), t.TempDir(), + &plan.Container{Image: "alpine", Volumes: []string{"cache:/cache"}}, nil, + ) + if err == nil || !strings.Contains(err.Error(), "inspect job container volumes") { + t.Fatalf("startJobContainer() error = %v", err) + } + removedVolumes, readErr := os.ReadFile(filepath.Join(f.root, "removed-volumes")) + if readErr != nil || strings.TrimSpace(string(removedVolumes)) != "cache" { + t.Fatalf("removed volumes = %q, %v", removedVolumes, readErr) + } +} + func TestRunJobContainerCreateCancellationCleansAnonymousVolume(t *testing.T) { f := newJobDocker(t, "block-job-create") w, tmp := t.TempDir(), t.TempDir() From cab308ba122af1cdb53bd9ab53391b8b7a39b208 Mon Sep 17 00:00:00 2001 From: Amp Date: Sat, 22 Aug 2026 10:00:22 +0000 Subject: [PATCH 12/15] Guard Docker volume ownership baseline Amp-Thread-ID: https://ampcode.com/threads/T-01a02836-fd56-769a-b44a-a7412431fd1d Co-authored-by: Lachlan Donald --- docs/compatibility.md | 2 +- internal/containerpolicy/policy.go | 12 ++++-------- internal/containerpolicy/policy_test.go | 4 ++-- internal/runtime/containers.go | 4 +++- internal/runtime/containers_test.go | 22 ++++++++++++++++++++++ 5 files changed, 32 insertions(+), 12 deletions(-) diff --git a/docs/compatibility.md b/docs/compatibility.md index 1a48b5ba..b37ecb11 100644 --- a/docs/compatibility.md +++ b/docs/compatibility.md @@ -648,7 +648,7 @@ services: Job containers support `image`, `env`, `ports`, `volumes`, and `options`. Services support `image`, `credentials`, `env`, `ports`, `volumes`, `options`, `command`, and `entrypoint`. - Job container images can use compile-time `github`, `inputs`, `vars`, `strategy`, and `matrix` values. The complete image must resolve to a non-empty string and a valid image reference during compilation. Secrets, `needs`, step outputs, and whole or dynamic contexts are unsupported. -- Job container volumes accept `DESTINATION[:ro|rw]` for an anonymous volume or `SOURCE:DESTINATION[:ro|rw]` for a named volume or bind mount. `DESTINATION` must be absolute. `SOURCE` must be a Docker volume name or absolute host path. A job can define 128 unique declarations. Expressions are unsupported. +- Job container volumes accept `DESTINATION` for an anonymous volume or `SOURCE:DESTINATION[:ro|rw]` for a named volume or bind mount. `DESTINATION` must be absolute. `SOURCE` must be a Docker volume name or absolute host path. A job can define 128 unique declarations. Expressions are unsupported. - Job container options pass through to `docker create`, except `--network`, `--net`, and `--entrypoint`, including their `--flag=value` forms. Options split into arguments without a shell. Double quotes group arguments; single quotes are ordinary characters. Expressions, line breaks, NUL bytes, and values over 65,536 bytes are unsupported. - Service fields can use compile-time `github`, `inputs`, `vars`, `strategy`, and `matrix` values or runtime `needs` outputs. An empty evaluated image skips the service. - A complete non-credential service map can use `${{ fromJSON(needs..outputs.) }}`. Declare credentials statically so the compiler can prove their secret authority. diff --git a/internal/containerpolicy/policy.go b/internal/containerpolicy/policy.go index ecf3d1d4..024905e5 100644 --- a/internal/containerpolicy/policy.go +++ b/internal/containerpolicy/policy.go @@ -44,13 +44,9 @@ func ValidateJobVolume(value string) error { case 1: target = parts[0] case 2: - if parts[1] == "ro" || parts[1] == "rw" { - target, mode = parts[0], parts[1] - } else { - source, target = parts[0], parts[1] - if source == "" { - return fmt.Errorf("volume source is empty") - } + source, target = parts[0], parts[1] + if source == "" { + return fmt.Errorf("volume source is empty") } case 3: source, target, mode = parts[0], parts[1], parts[2] @@ -58,7 +54,7 @@ func ValidateJobVolume(value string) error { return fmt.Errorf("volume source is empty") } default: - return fmt.Errorf("volume must be DESTINATION[:ro|rw] or SOURCE:DESTINATION[:ro|rw]") + return fmt.Errorf("volume must be DESTINATION or SOURCE:DESTINATION[:ro|rw]") } if source != "" && !path.IsAbs(source) && !volumeNamePattern.MatchString(source) { return fmt.Errorf("volume source %q must be a name or absolute host path", source) diff --git a/internal/containerpolicy/policy_test.go b/internal/containerpolicy/policy_test.go index 1bf28147..bf62b9ec 100644 --- a/internal/containerpolicy/policy_test.go +++ b/internal/containerpolicy/policy_test.go @@ -42,13 +42,13 @@ func TestJobOptionsInputBounds(t *testing.T) { func TestValidateJobVolume(t *testing.T) { for _, value := range []string{ "v:/data", "cache:/cache", "cache.v1:/var/cache:ro", "CACHE_1:/data:rw", - "/anonymous", "/anonymous:ro", "/srv/cache:/cache", "/srv/cache:/cache:rw", + "/anonymous", "/srv/cache:/cache", "/srv/cache:/cache:rw", } { if err := ValidateJobVolume(value); err != nil { t.Errorf("ValidateJobVolume(%q) = %v", value, err) } } - for _, value := range []string{"-bad:/data", "relative/path:/data", "cache:data", "cache:/data:z", ":/data"} { + for _, value := range []string{"-bad:/data", "relative/path:/data", "cache:data", "cache:/data:z", ":/data", "/anonymous:ro"} { if err := ValidateJobVolume(value); err == nil { t.Errorf("ValidateJobVolume(%q) accepted invalid volume", value) } diff --git a/internal/runtime/containers.go b/internal/runtime/containers.go index da18e3a0..b8dc6464 100644 --- a/internal/runtime/containers.go +++ b/internal/runtime/containers.go @@ -58,6 +58,7 @@ type jobContainerBackend struct { probedNodes map[string]bool servicePorts map[string]expression.ServiceContext existingVolumes map[string]bool + volumeBaselineCaptured bool ownedVolumes []string volumesTracked bool } @@ -178,6 +179,7 @@ func (r Runner) startJobContainerOrdered(ctx context.Context, processor *command return nil, fmt.Errorf("snapshot Docker volumes: %w", volumeErr) } b.existingVolumes = lineSet(volumes) + b.volumeBaselineCaptured = true } if spec != nil { if err = r.pullContainerImage(ctx, processor, env, docker, spec.Image); err != nil { @@ -854,7 +856,7 @@ func (b *jobContainerBackend) cleanup(parent context.Context) error { var out string var queryErr error if b.container != "" { - if !b.volumesTracked { + if b.volumeBaselineCaptured && !b.volumesTracked { if trackErr := b.trackCreatedVolumes(ctx); trackErr != nil { err = errors.Join(err, trackErr) } else { diff --git a/internal/runtime/containers_test.go b/internal/runtime/containers_test.go index 6a847312..d7e4d92a 100644 --- a/internal/runtime/containers_test.go +++ b/internal/runtime/containers_test.go @@ -493,6 +493,9 @@ func TestJobContainerFakeDockerProcess(t *testing.T) { _ = os.Remove(network) os.Exit(0) case "volume-ls": + if scenario == "fail-volume-snapshot" { + os.Exit(43) + } if scenario == "fail-create-reconcile-once" || scenario == "fail-job-volume-tracking-once" { if _, err := os.Stat(filepath.Join(root, "current-volumes")); err == nil { marker := filepath.Join(root, "failed-volume-reconcile") @@ -1584,6 +1587,25 @@ func TestRunJobContainerCleanupRetriesSuccessfulCreateVolumeTracking(t *testing. } } +func TestRunJobContainerFailedVolumeSnapshotDoesNotClaimHostVolumes(t *testing.T) { + f := newJobDocker(t, "fail-volume-snapshot") + if err := os.WriteFile(filepath.Join(f.root, "existing-volumes"), []byte("shared\n"), 0o600); err != nil { + t.Fatal(err) + } + _, err := (Runner{Docker: f.path, RuntimeExecutable: os.Args[0]}).startJobContainer( + t.Context(), newCommandProcessor(io.Discard, io.Discard), t.TempDir(), t.TempDir(), + &plan.Container{Image: "alpine", Volumes: []string{"cache:/cache"}}, nil, + ) + if err == nil || !strings.Contains(err.Error(), "snapshot Docker volumes") { + t.Fatalf("startJobContainer() error = %v", err) + } + for _, call := range f.calls(t) { + if len(call.Args) >= 2 && call.Args[0] == "volume" && call.Args[1] == "rm" { + t.Fatalf("failed baseline cleanup claimed host volumes: %#v", f.calls(t)) + } + } +} + func TestRunJobContainerCreateCancellationCleansAnonymousVolume(t *testing.T) { f := newJobDocker(t, "block-job-create") w, tmp := t.TempDir(), t.TempDir() From 0f12a7bb04b037c39c5f43daa74360f7577599cd Mon Sep 17 00:00:00 2001 From: Amp Date: Mon, 24 Aug 2026 02:32:41 +0000 Subject: [PATCH 13/15] Simplify container create reconciliation Amp-Thread-ID: https://ampcode.com/threads/T-01a02836-fd56-769a-b44a-a7412431fd1d Co-authored-by: Lachlan Donald --- internal/containerpolicy/policy_test.go | 22 ++++++++++ internal/runtime/containers.go | 58 ++++++++++--------------- internal/runtime/containers_test.go | 23 ---------- 3 files changed, 45 insertions(+), 58 deletions(-) diff --git a/internal/containerpolicy/policy_test.go b/internal/containerpolicy/policy_test.go index bf62b9ec..493de589 100644 --- a/internal/containerpolicy/policy_test.go +++ b/internal/containerpolicy/policy_test.go @@ -39,6 +39,28 @@ func TestJobOptionsInputBounds(t *testing.T) { } } +func TestArgumentList(t *testing.T) { + t.Parallel() + for _, test := range []struct { + name, value string + want []string + }{ + {name: "empty argument", value: `--env ""`, want: []string{"--env", ""}}, + {name: "double quotes", value: `--health-cmd "pg_isready -U postgres"`, want: []string{"--health-cmd", "pg_isready -U postgres"}}, + {name: "single quotes are literal", value: `--label 'two words'`, want: []string{"--label", "'two", "words'"}}, + {name: "escaped quote", value: `one\"two`, want: []string{`one"two`}}, + {name: "unmatched quote", value: `"two words`, want: []string{"two words"}}, + {name: "newline is literal", value: "one\ntwo", want: []string{"one\ntwo"}}, + {name: "consecutive quotes", value: `"one""two"`, want: []string{`one"two`}}, + } { + t.Run(test.name, func(t *testing.T) { + if got := ArgumentList(test.value); !slices.Equal(got, test.want) { + t.Fatalf("ArgumentList(%q) = %#v; want %#v", test.value, got, test.want) + } + }) + } +} + func TestValidateJobVolume(t *testing.T) { for _, value := range []string{ "v:/data", "cache:/cache", "cache.v1:/var/cache:ro", "CACHE_1:/data:rw", diff --git a/internal/runtime/containers.go b/internal/runtime/containers.go index b8dc6464..6fa278ff 100644 --- a/internal/runtime/containers.go +++ b/internal/runtime/containers.go @@ -241,10 +241,7 @@ func (r Runner) startJobContainerOrdered(ctx context.Context, processor *command b.services = append(b.services, serviceContainer{id: serviceID, name: name}) serviceArgs := []string{"create", "--name", name, "--label", "com.buildkite.gha=true", "--label", b.owner, "--network", b.network, "--network-alias", serviceID} serviceArgs = appendPublishedPorts(serviceArgs, service.Ports) - options, optionErr := dockerArgumentList(service.Options) - if optionErr != nil { - return nil, fmt.Errorf("parse service %q options: %w", serviceID, optionErr) - } + options := containerpolicy.ArgumentList(service.Options) if err = validateServiceOptions(options); err != nil { return nil, fmt.Errorf("service %q options: %w", serviceID, err) } @@ -259,11 +256,7 @@ func (r Runner) startJobContainerOrdered(ctx context.Context, processor *command serviceArgs = append(serviceArgs, "--entrypoint", service.Entrypoint) } serviceArgs = append(serviceArgs, service.Image) - command, commandErr := dockerArgumentList(service.Command) - if commandErr != nil { - return nil, fmt.Errorf("parse service %q command: %w", serviceID, commandErr) - } - serviceArgs = append(serviceArgs, command...) + serviceArgs = append(serviceArgs, containerpolicy.ArgumentList(service.Command)...) created, createErr := boundedDockerOutput(ctx, env, docker, serviceArgs...) if reference := strings.TrimSpace(created); reference != "" { b.services[len(b.services)-1].name = reference @@ -338,25 +331,7 @@ func (r Runner) startJobContainerOrdered(ctx context.Context, processor *command } if createErr != nil { reconcileCtx, cancelReconcile := context.WithTimeout(context.WithoutCancel(ctx), r.cleanupTimeout()) - if reference == "" { - reference, err = b.reconcileCreatedJob(reconcileCtx) - if err != nil { - createErr = errors.Join(createErr, err) - } else if reference != "" { - b.container = reference - b.containerCreated = true - } - } - if reference != "" { - if trackErr := b.trackContainerVolumes(reconcileCtx, "job container", b.container); trackErr != nil { - createErr = errors.Join(createErr, trackErr) - } - } - if trackErr := b.trackCreatedVolumes(reconcileCtx); trackErr != nil { - createErr = errors.Join(createErr, trackErr) - } else { - b.volumesTracked = true - } + createErr = errors.Join(createErr, b.reconcileFailedJobCreate(reconcileCtx)) cancelReconcile() return nil, fmt.Errorf("create job container: %w", createErr) } @@ -512,6 +487,26 @@ func (b *jobContainerBackend) reconcileCreatedJob(ctx context.Context) (string, return "", nil } +func (b *jobContainerBackend) reconcileFailedJobCreate(ctx context.Context) error { + var err error + if !b.containerCreated { + reference, reconcileErr := b.reconcileCreatedJob(ctx) + err = errors.Join(err, reconcileErr) + if reference != "" { + b.container = reference + b.containerCreated = true + } + } + if b.containerCreated { + err = errors.Join(err, b.trackContainerVolumes(ctx, "job container", b.container)) + } + trackErr := b.trackCreatedVolumes(ctx) + if trackErr == nil { + b.volumesTracked = true + } + return errors.Join(err, trackErr) +} + func (b *jobContainerBackend) trackServiceVolumes(ctx context.Context, serviceID, reference string) error { return b.trackContainerVolumes(ctx, fmt.Sprintf("service %q", serviceID), reference) } @@ -568,13 +563,6 @@ func validateServiceOptions(options []string) error { return nil } -// dockerArgumentList matches the argument splitting used by the pinned -// actions/runner ProcessStartInfo.Arguments path. Single quotes are ordinary -// characters; double quotes group arguments; backslashes only escape quotes. -func dockerArgumentList(value string) ([]string, error) { - return containerpolicy.ArgumentList(value), nil -} - var dockerPortLine = regexp.MustCompile(`^([0-9]+)/([A-Za-z0-9]+) -> (?:[^:]+|\[[^]]+\]):([0-9]+)$`) func (b *jobContainerBackend) readServicePorts(ctx context.Context, id, name string, _ []string) (map[string]string, error) { diff --git a/internal/runtime/containers_test.go b/internal/runtime/containers_test.go index d7e4d92a..9bdb2e00 100644 --- a/internal/runtime/containers_test.go +++ b/internal/runtime/containers_test.go @@ -1071,29 +1071,6 @@ func TestRunServiceContainerAutoRemoveBetweenQueryAndStop(t *testing.T) { } } -func TestDockerArgumentListMatchesRunnerQuoting(t *testing.T) { - t.Parallel() - for _, test := range []struct { - name, value string - want []string - }{ - {name: "empty", value: `--env ""`, want: []string{"--env", ""}}, - {name: "double quotes", value: `--health-cmd "pg_isready -U postgres"`, want: []string{"--health-cmd", "pg_isready -U postgres"}}, - {name: "single quotes literal", value: `--label 'two words'`, want: []string{"--label", "'two", "words'"}}, - {name: "escaped quote", value: `one\"two`, want: []string{`one"two`}}, - {name: "unmatched quote", value: `"two words`, want: []string{"two words"}}, - {name: "newline literal", value: "one\ntwo", want: []string{"one\ntwo"}}, - {name: "consecutive quotes", value: `"one""two"`, want: []string{`one"two`}}, - } { - t.Run(test.name, func(t *testing.T) { - got, err := dockerArgumentList(test.value) - if err != nil || !slices.Equal(got, test.want) { - t.Fatalf("dockerArgumentList(%q) = %#v, %v; want %#v", test.value, got, err, test.want) - } - }) - } -} - func TestServiceOptionsRejectOnlyNetworkOverride(t *testing.T) { t.Parallel() for _, test := range []struct { From 79a8dfff9bba97207ce5273b691177c5e9388308 Mon Sep 17 00:00:00 2001 From: Amp Date: Mon, 24 Aug 2026 05:29:55 +0000 Subject: [PATCH 14/15] Integrate job containers with workflow programs Amp-Thread-ID: https://ampcode.com/threads/T-01a02836-fd56-769a-b44a-a7412431fd1d Co-authored-by: Lachlan Donald --- internal/compiler/plan_builder.go | 9 +++++++-- internal/compiler/workflow_program.go | 8 +++++--- internal/program/program.go | 8 +++++--- 3 files changed, 17 insertions(+), 8 deletions(-) diff --git a/internal/compiler/plan_builder.go b/internal/compiler/plan_builder.go index 4dd4af47..ab4105f9 100644 --- a/internal/compiler/plan_builder.go +++ b/internal/compiler/plan_builder.go @@ -304,8 +304,10 @@ func (b planBuilder) reducePlanInstanceEventExpressions(instance JobInstance) (J if instance.Container != nil { container := *instance.Container - if container.Image, err = reduceTemplate(container.Image); err != nil { - return JobInstance{}, err + for _, field := range []*string{&container.Image, &container.Options} { + if *field, err = reduceTemplate(*field); err != nil { + return JobInstance{}, err + } } if container.Env, err = reduceMap(container.Env); err != nil { return JobInstance{}, err @@ -313,6 +315,9 @@ func (b planBuilder) reducePlanInstanceEventExpressions(instance JobInstance) (J if container.Ports, err = reduceSlice(container.Ports); err != nil { return JobInstance{}, err } + if container.Volumes, err = reduceSlice(container.Volumes); err != nil { + return JobInstance{}, err + } instance.Container = &container } diff --git a/internal/compiler/workflow_program.go b/internal/compiler/workflow_program.go index 398fa92a..26f553b5 100644 --- a/internal/compiler/workflow_program.go +++ b/internal/compiler/workflow_program.go @@ -31,9 +31,11 @@ func lowerWorkflowProgram(instance JobInstance) program.Program { } if instance.Container != nil { result.Job.Container = &program.Container{ - Image: workflowSite(instance.Container.Image, program.SurfaceRuntimeTemplate, program.ResultString, jobLocation, "job.container.image"), - Env: workflowBindings(instance.Container.Env, program.SurfaceRuntimeTemplate, jobLocation, "job.container.env", program.PurposeExpression), - Ports: workflowSites(instance.Container.Ports, program.SurfaceRuntimeTemplate, program.ResultString, jobLocation, "job.container.ports"), + Image: workflowSite(instance.Container.Image, program.SurfaceRuntimeTemplate, program.ResultString, jobLocation, "job.container.image"), + Env: workflowBindings(instance.Container.Env, program.SurfaceRuntimeTemplate, jobLocation, "job.container.env", program.PurposeExpression), + Ports: workflowSites(instance.Container.Ports, program.SurfaceRuntimeTemplate, program.ResultString, jobLocation, "job.container.ports"), + Volumes: workflowSites(instance.Container.Volumes, program.SurfaceRuntimeTemplate, program.ResultString, jobLocation, "job.container.volumes"), + Options: workflowSite(instance.Container.Options, program.SurfaceRuntimeTemplate, program.ResultString, jobLocation, "job.container.options"), } } if len(instance.Services) != 0 { diff --git a/internal/program/program.go b/internal/program/program.go index 519a0334..fde9f549 100644 --- a/internal/program/program.go +++ b/internal/program/program.go @@ -88,9 +88,11 @@ type Defaults struct { } type Container struct { - Image Site - Env []Binding - Ports []Site + Image Site + Env []Binding + Ports []Site + Volumes []Site + Options Site } type ContainerCredentials struct { From 327e2420179c86a0065e176e11d60b5742de7c03 Mon Sep 17 00:00:00 2001 From: Amp Date: Mon, 24 Aug 2026 05:38:24 +0000 Subject: [PATCH 15/15] Inventory job container program fields Amp-Thread-ID: https://ampcode.com/threads/T-01a02836-fd56-769a-b44a-a7412431fd1d Co-authored-by: Lachlan Donald --- internal/compiler/workflow_program_test.go | 5 ++++- internal/program/program.go | 10 ++++++---- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/internal/compiler/workflow_program_test.go b/internal/compiler/workflow_program_test.go index d76bfa43..284764a1 100644 --- a/internal/compiler/workflow_program_test.go +++ b/internal/compiler/workflow_program_test.go @@ -19,7 +19,8 @@ func TestWorkflowProgramInventoriesEveryExecutionField(t *testing.T) { DefaultShell: "job-shell", DefaultWorkingDirectory: "job-directory", Container: &workflow.Container{ - Image: "container-image", Env: map[string]string{"C": "container-env"}, Ports: []string{"container-port"}, Span: span, + Image: "container-image", Env: map[string]string{"C": "container-env"}, Ports: []string{"container-port"}, + Volumes: []string{"container-volume"}, Options: "container-options", Span: span, }, Services: []workflow.Service{{Name: "db", Container: workflow.ServiceContainer{ Image: "service-image", Env: map[string]string{"S": "service-env"}, Ports: []string{"service-port"}, Volumes: []string{"service-volume"}, @@ -53,6 +54,8 @@ func TestWorkflowProgramInventoriesEveryExecutionField(t *testing.T) { "job.container.image|runtime-template|string|expression", "job.container.env.C|runtime-template|string|expression", "job.container.ports[0]|runtime-template|string|expression", + "job.container.volumes[0]|runtime-template|string|expression", + "job.container.options|runtime-template|string|expression", "job.services.db.image|runtime-template|string|expression", "job.services.db.credentials.username|service-credential|string|expression", "job.services.db.credentials.password|service-credential|string|expression", diff --git a/internal/program/program.go b/internal/program/program.go index fde9f549..3cfea7dc 100644 --- a/internal/program/program.go +++ b/internal/program/program.go @@ -251,12 +251,14 @@ func visitContainer(container Container, visit func(Site) error) error { if err := visitBindings(container.Env, visit); err != nil { return err } - for _, port := range container.Ports { - if err := visitSite(port, visit); err != nil { - return err + for _, sites := range [][]Site{container.Ports, container.Volumes} { + for _, site := range sites { + if err := visitSite(site, visit); err != nil { + return err + } } } - return nil + return visitSite(container.Options, visit) } func visitServiceContainer(container ServiceContainer, visit func(Site) error) error {