diff --git a/docs/compatibility.md b/docs/compatibility.md index 94a7c6ab..b37ecb11 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`, `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` 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. -- 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, 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 7e66bced..11cfa6be 100644 --- a/docs/security.md +++ b/docs/security.md @@ -45,9 +45,13 @@ 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 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. 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..cf3d2c11 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, /anonymous, /srv/data:/data] + options: --privileged --label "description=two words" 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", "/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.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..58569d64 100644 --- a/internal/compiler/compiler_test.go +++ b/internal/compiler/compiler_test.go @@ -4173,14 +4173,15 @@ 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', '/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 || 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) } + validateCompiledPlansAgainstSchema(t, plans) } func TestCompilePlansResolveJobContainerImageExpressions(t *testing.T) { diff --git a/internal/compiler/plan_builder.go b/internal/compiler/plan_builder.go index d51a30d9..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 } @@ -631,9 +636,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/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/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/containerpolicy/policy.go b/internal/containerpolicy/policy.go new file mode 100644 index 00000000..024905e5 --- /dev/null +++ b/internal/containerpolicy/policy.go @@ -0,0 +1,141 @@ +// Package containerpolicy owns the job-container Docker syntax that workflows +// may control. +package containerpolicy + +import ( + "fmt" + "path" + "regexp" + "strings" +) + +const MaxJobVolumes = 128 +const MaxJobOptionsLength = 65536 + +var volumeNamePattern = regexp.MustCompile(`^[A-Za-z0-9][A-Za-z0-9_.-]*$`) + +// JobOptions splits options using the GitHub runner's argument rules and +// 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) > MaxJobOptionsLength || strings.ContainsAny(value, "\x00\r\n") { + return nil, fmt.Errorf("options exceed %d bytes or contain a control character", MaxJobOptionsLength) + } + args := ArgumentList(value) + 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) + } + } + } + return args, nil +} + +// 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, ":") + var source, target, mode string + switch len(parts) { + case 1: + target = parts[0] + case 2: + 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 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) + } + if !path.IsAbs(target) { + return fmt.Errorf("volume target %q must be an absolute path", target) + } + if mode != "" && mode != "ro" && mode != "rw" { + return fmt.Errorf("volume mode %q is unsupported", mode) + } + 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 +} + +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..493de589 --- /dev/null +++ b/internal/containerpolicy/policy_test.go @@ -0,0 +1,84 @@ +package containerpolicy + +import ( + "slices" + "strings" + "testing" +) + +func TestJobOptionsAcceptedSyntax(t *testing.T) { + 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 TestJobOptionsRejectsUnsupportedOverrides(t *testing.T) { + for _, value := range []string{ + "--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 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 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", + "/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", "/anonymous:ro"} { + 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") + } + 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.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..d8b2466b 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,36 @@ func TestContainerModelFields(t *testing.T) { } } +func TestJobContainerPlanRejectsUnsupportedOptionsAndVolumes(t *testing.T) { + for name, container := range map[string]Container{ + "network": {Image: "node:24", Options: "--network=host"}, + "entrypoint": {Image: "node:24", Options: "--entrypoint sh"}, + "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 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() @@ -1198,7 +1228,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/program/program.go b/internal/program/program.go index 519a0334..3cfea7dc 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 { @@ -249,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 { diff --git a/internal/runtime/containers.go b/internal/runtime/containers.go index b0dfbaf1..6fa278ff 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" ) @@ -48,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 @@ -56,7 +58,9 @@ type jobContainerBackend struct { probedNodes map[string]bool servicePorts map[string]expression.ServiceContext existingVolumes map[string]bool + volumeBaselineCaptured bool ownedVolumes []string + volumesTracked bool } func privateDocker(r Runner) (string, string, map[string]string, error) { @@ -88,6 +92,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 +173,13 @@ func (r Runner) startJobContainerOrdered(ctx context.Context, processor *command } } workflowFailure = true - if len(services) != 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) + b.volumeBaselineCaptured = true } if spec != nil { if err = r.pullContainerImage(ctx, processor, env, docker, spec.Image); err != nil { @@ -233,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) } @@ -251,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 @@ -309,14 +310,35 @@ 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) + created, createErr := boundedDockerOutput(ctx, env, docker, args...) + reference := strings.TrimSpace(created) + if reference != "" { + b.container = reference + b.containerCreated = true + } + if createErr != nil { + reconcileCtx, cancelReconcile := context.WithTimeout(context.WithoutCancel(ctx), r.cleanupTimeout()) + createErr = errors.Join(createErr, b.reconcileFailedJobCreate(reconcileCtx)) + cancelReconcile() + return nil, fmt.Errorf("create job container: %w", createErr) + } + 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) } @@ -438,76 +460,107 @@ 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) 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) +} + +func (b *jobContainerBackend) trackJobContainerVolumes(parent context.Context) error { + ctx, cancel := context.WithTimeout(context.WithoutCancel(parent), b.runner.cleanupTimeout()) + defer cancel() + 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 { 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) } + 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 } -func validateServiceOptions(options []string) error { - for _, option := range options { - if option == "--network" || option == "--net" || strings.HasPrefix(option, "--network=") || strings.HasPrefix(option, "--net=") { - return fmt.Errorf("network override %q is unsupported", option) +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) { + volumes = append(volumes, volume) } } + slices.Sort(volumes) + b.ownedVolumes = append(b.ownedVolumes, volumes...) 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) { - 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++ +func validateServiceOptions(options []string) error { + for _, option := range options { + if option == "--network" || option == "--net" || strings.HasPrefix(option, "--network=") || strings.HasPrefix(option, "--net=") { + return fmt.Errorf("network override %q is unsupported", option) } - args = append(args, arg.String()) } - return args, nil + return nil } var dockerPortLine = regexp.MustCompile(`^([0-9]+)/([A-Za-z0-9]+) -> (?:[^:]+|\[[^]]+\]):([0-9]+)$`) @@ -791,14 +844,32 @@ 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+"$") - 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) - if e != nil { - err = errors.Join(err, fmt.Errorf("remove job container: %w", e)) + if b.volumeBaselineCaptured && !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) + } 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 1b3068ea..9bdb2e00 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" ) @@ -229,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]) } } @@ -264,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") @@ -275,9 +284,12 @@ 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-") { + select {} + } fmt.Print("docker-id-" + name) os.Exit(0) case "start": @@ -319,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) @@ -348,7 +367,14 @@ 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 == "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) } joined := strings.Join(args, " ") @@ -467,9 +493,30 @@ 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") + 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)) } + 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)) @@ -644,10 +691,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) @@ -656,7 +703,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) @@ -866,12 +913,103 @@ 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) } } +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", "/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) + } + 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", + "--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)) != "anonymous-volume\ncache\noption-cache" { + t.Fatalf("removed job volumes = %q, %v", removed, readErr) + } + return + } + t.Fatal("job container create call not found") +} + +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 a runner-owned override") + } + }) + } +} + +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) + } + 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 { + t.Fatalf("startJobContainer() error = %v", err) + } + 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") + } +} + 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"}}) @@ -933,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 { @@ -1396,6 +1511,127 @@ 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 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", Volumes: []string{"cache:/cache"}}, 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"}) + } + 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)) + } +} + +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 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() + 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"}}, 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)) != "anonymous-volume" { + t.Fatalf("removed volumes = %q, %v", removed, readErr) + } +} + +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() @@ -1541,10 +1777,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) } @@ -1573,7 +1809,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) } } @@ -1831,7 +2067,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 { @@ -2002,7 +2238,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/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..977cddd6 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) + jobID := strings.ToLower(resolveAlias(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 = containerpolicy.MaxJobOptionsLength + 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..2bb4d149 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', '/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" || 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) } } @@ -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,30 @@ func TestParseRejectsUnsupportedContainerControls(t *testing.T) { } } +func TestParseRejectsUnsupportedJobContainerOptionsAndVolumes(t *testing.T) { + for name, body := range map[string]string{ + "network option": "options: --network host", + "entrypoint option": "options: --entrypoint sh", + "option expression": "options: --cpus ${{ matrix.cpus }}", + "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 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 { @@ -551,6 +576,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) diff --git a/schemas/job-plan.schema.json b/schemas/job-plan.schema.json index e582fd35..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))?$"}}}}, + "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})?$"},