feat(verify/docker): apply runArgs whitelist to verify test containers#10068
feat(verify/docker): apply runArgs whitelist to verify test containers#10068bogdannazarenko wants to merge 5 commits intoGoogleContainerTools:mainfrom
Conversation
Introduces ParseRunArgs in pkg/skaffold/actions/docker/runargs.go that accepts a docker-run-style flag list using a conservative whitelist (--network, -v/--volume, -e/--env, --user, --add-host, --tmpfs, --privileged, --cap-add, --cap-drop). Unknown flags return an error so users fail fast rather than have settings silently dropped. Only the '--flag=value' form is supported to keep the parser unambiguous. Two small helpers project the parsed result onto the existing docker API types: ApplyToContainerConfig writes User and appends Env onto container.Config; ApplyToHostConfig overlays NetworkMode, Binds, ExtraHosts, Tmpfs, Privileged, CapAdd and CapDrop onto a container.HostConfig. To make the overlay available at run time, ContainerCreateOpts in pkg/skaffold/docker/image.go gains an optional HostConfigApply hook that is invoked after the default HostConfig is constructed. The Task parses the owning action's runArgs once in createTasks, stores it on the Task, and wires both helpers through the existing config paths. No runtime change occurs when runArgs is absent: the overlay methods are no-ops on a nil receiver.
Covers:
- nil and empty input return nil, nil
- every supported flag parses into the expected field, including
repeated -v/--volume, -e/--env, --cap-add, --cap-drop
- unknown bare flags (e.g. --rm) are rejected with a clear message
- non-flag bare values are rejected with the space-separated hint
- empty/whitespace entries are skipped
- ApplyToContainerConfig is a no-op on nil receiver or nil cfg
- ApplyToContainerConfig sets User and appends Env preserving existing
- ApplyToHostConfig is a no-op on nil receiver or nil hc
- ApplyToHostConfig overrides NetworkMode only when provided, appends
Binds / ExtraHosts / CapAdd / CapDrop, unions Tmpfs, and honours
Privileged=true
Adds a new 'Passing Docker run flags with runArgs' subsection under the local execution-mode docs covering the whitelist, the accepted --flag=value form, and a security warning about bind mounts and privileged mode. Includes a worked example that mounts the host's ADC credentials into a google/cloud-sdk container. Introduces an integration example at integration/examples/custom-actions-runargs/ demonstrating a hardened action (non-root user, dropped cap) and a gcloud credential-passthrough action. Extends TestExec_LocalActions with an 'action with runArgs overlay' case that asserts --user=1000:1000 pins uid=1000 inside the container and -e=SENTINEL=from-runargs reaches the process env.
Extends the schema-level `LocalVerifier` with an optional `runArgs`
list and plumbs it through the local docker verifier so users can
reuse the same conservative whitelist already supported on
`customActions.*.executionMode.local.runArgs`:
verify:
- name: smoke
executionMode:
local:
useLocalImages: true
runArgs:
- --network=host
- -v=/var/run/docker.sock:/var/run/docker.sock
- --user=1000:1000
container:
image: myorg/smoke:latest
Implementation
* schema: LocalVerifier gains `RunArgs []string` with the same godoc
whitelist as custom-actions.
* verify/docker: createAndRunContainer parses the per-test-case runArgs
via actionsdocker.ParseRunArgs; an unknown flag is surfaced as a
typed per-test error instead of being silently dropped. The parsed
result overlays the container config (User, extra Env) after the
schema-sourced env and --verify-env-file values so it keeps the same
precedence as custom-actions, and exposes ApplyToHostConfig via the
existing ContainerCreateOpts.HostConfigApply hook for NetworkMode,
Binds, ExtraHosts, Tmpfs, Privileged, CapAdd, CapDrop.
No runtime change occurs when runArgs is absent: both overlay methods
are no-ops on a nil `*RunArgs` receiver.
… error
Extends fakeDockerDaemon with a RunOpts capture slice so tests can
assert on the ContainerCreateOpts passed to Run. Adds Test_RunArgs
covering:
* nil runArgs: HostConfigApply is still a safe method value on a
nil *RunArgs receiver and leaves the HostConfig untouched.
* parsed runArgs overlay the expected container.Config fields
(User, Env) and HostConfig fields (NetworkMode, Binds, CapAdd,
Privileged).
* unknown --gpus=all flag fails the test case rather than being
silently ignored.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the runArgs field for local custom actions and verifiers, allowing users to specify a whitelisted set of Docker run flags such as volumes, environment variables, and network modes. The implementation includes a new parsing utility, schema updates, and integration into the Docker execution environment. Feedback focuses on improving error message clarity for invalid flag formats and using pointers for boolean flags to support explicit false overrides.
| case "--cap-drop": | ||
| out.CapDrop = append(out.CapDrop, val) | ||
| default: | ||
| return nil, fmt.Errorf("runArgs[%d] %q: unsupported flag %q (allowed: --network, -v/--volume, -e/--env, --user, --add-host, --tmpfs, --privileged, --cap-add, --cap-drop)", i, raw, key) |
There was a problem hiding this comment.
The error message here can be slightly misleading when the user provides a supported flag but with an invalid format (e.g., --privileged=foo). Since --privileged is listed in the allowed section of the error message, but it's not a case in the switch key (because it's handled separately before the Cut), the user might be confused as to why it's reported as "unsupported". Consider refining the error message or the logic to distinguish between truly unknown flags and invalid usage of supported ones.
| } | ||
| } | ||
| if r.Privileged { | ||
| hc.Privileged = true |
There was a problem hiding this comment.
The current implementation of ApplyToHostConfig only sets hc.Privileged = true if r.Privileged is true. If r.Privileged is false (which is the default for a bool), it does nothing. This means that if a future version of Skaffold or a different caller were to enable privileged mode by default, passing --privileged=false in runArgs would not be able to override it back to false. While not an issue with the current defaults, using a pointer (*bool) in the RunArgs struct would allow for a more robust overlay that can explicitly set the value to false.
1 participant
Summary
Follow-up to #10066 (custom-actions runArgs). Extends the
LocalVerifierschema with an optionalrunArgslist and plumbs it throughpkg/skaffold/verify/dockersoskaffold verifycontainers get the same conservative whitelist already supported oncustomActions.*.executionMode.local.runArgs.Why
The roadmap note on PR #10066 explicitly called this out as a deferred follow-up:
This PR is that follow-up. It's intentionally small — one new schema field and one call site.
Changes
LocalVerifiergainsRunArgs []stringwith the same godoc whitelist (--network,-v/--volume,-e/--env,--user,--add-host,--tmpfs,--privileged,--cap-add,--cap-drop).createAndRunContainerparses the per-test-caserunArgsviaactionsdocker.ParseRunArgs. Unknown flags fail the test case instead of being silently dropped. The parsed result overlays the container config (User, extra Env) after the schema-sourced env and--verify-env-filevalues, and exposesApplyToHostConfigvia the existingContainerCreateOpts.HostConfigApplyhook.fakeDockerDaemongains aRunOptscapture slice; newTest_RunArgscovers the nil, happy-path overlay, and unknown-flag cases.No runtime change occurs when
runArgsis absent — overlays are no-ops on a nil*RunArgsreceiver.Stacked on #10066
This branch is based on #10066's head, so the PR diff includes the three phase-2 commits. Once #10066 merges, I'll rebase onto
mainand the diff will collapse to the two commits introduced here:No schema version bump
Keeps
latestatskaffold/v4beta14per the maintainer-drivenhack/new-version.shworkflow.Tests
make quicktestis clean aside from the pre-existingTestNewEnvClient/DOCKER_HOST_not_setandTestNewRunnerfailures (environment-dependent, reproduce onmain) andTestParseExamples/custom-actions-runargs(pre-existing on the phase-2 branch this stacks on; the example YAML declaresskaffold/v4beta15which doesn't exist on HEAD yet — will resolve with the maintainer'shack/new-version.shbump).