fix: misc bug fixed and improvements (#1253)

1. Host mode is selected only through `runs-on` again, as in upstream act.
1. A workflow volume that `valid_volumes` drops no longer displaces the runner's own socket, workspace, tool cache or `/var/run/act` mount, so `/var/run/docker.sock:/var/run/docker.sock` works on hosts whose daemon socket lives elsewhere.
1. `docker://` is stripped from job and service images, as on GitHub.
1. Each created service's log is printed at job end, and service log dumps no longer run workflow commands, as on GitHub.
1. A `--privileged` dropped because privileged mode is off now logs a warning.
1. README: the proxy variables change per step through `env:` or for the rest of a job through `$GITHUB_ENV`, `container.env` does not override them.

Reviewed-on: https://gitea.com/gitea/runner/pulls/1253
Reviewed-by: Zettat123 <39446+zettat123@noreply.gitea.com>
Co-authored-by: silverwind <me@silverwind.io>
This commit is contained in:
silverwind
2026-09-28 05:53:41 +00:00
committed by bircni
parent 990ebaa2ac
commit 2b599fae7f
7 changed files with 117 additions and 50 deletions

View File

@@ -298,7 +298,7 @@ These hosts are added to `no_proxy` for jobs, so they are always reached directl
Gitea is not added. Add it to `no_proxy` yourself if it should be reached directly.
To change a value for one job, set it in a step's `env:` or in the job's `container.env`. Setting it at workflow or job level has no effect. To change it for the whole runner, set it in `runner.envs`. A `no_proxy` set there is added to the list above instead of replacing it.
For one step, set it in the step's `env:`, for the rest of a job, write it to `$GITHUB_ENV`. Setting it at workflow or job level or in `container.env` has no effect. To change it for the whole runner, set it in `runner.envs`. A `no_proxy` set there is added to the list above instead of replacing it.
Images are pulled by the Docker daemon, which needs its own proxy setting. In the `dind` images the daemon runs in the same container and reads the variables above. For any other daemon, see [the Docker documentation](https://docs.docker.com/engine/daemon/proxy/). The runner logs a warning at startup if it has a proxy and the daemon does not.

View File

@@ -593,7 +593,8 @@ func (cr *containerReference) mergeContainerConfigs(ctx context.Context, config
// If the `privileged` config has been disabled, `copts.privileged` need to be forced to false,
// even if the user specifies `--privileged` in the options string.
if !hostConfig.Privileged {
if !hostConfig.Privileged && copts.privileged {
logger.Warnf("container option %q is not allowed when privileged mode is disabled and will be ignored", "--privileged")
copts.privileged = false
}
@@ -629,6 +630,7 @@ func (cr *containerReference) mergeContainerConfigs(ctx context.Context, config
logger.Debugf("Custom container.HostConfig from options ==> %+v", containerConfig.HostConfig)
cr.sanitizeConfig(ctx, nil, containerConfig.HostConfig) // a dropped volume must not displace the runner's own
overlayVolumes(hostConfig, containerConfig.HostConfig)
binds := hostConfig.Binds
mounts := hostConfig.Mounts
@@ -1261,6 +1263,12 @@ func (cr *containerReference) sanitizeConfig(ctx context.Context, config *contai
return config, hostConfig
}
func SanitizeBinds(ctx context.Context, validVolumes, binds []string) []string {
cr := &containerReference{input: &NewContainerInput{ValidVolumes: validVolumes}}
_, hostConfig := cr.sanitizeConfig(ctx, nil, &container.HostConfig{Binds: binds})
return hostConfig.Binds
}
// bindTarget returns the container path a bind mounts onto, empty if it cannot be parsed.
func bindTarget(bind string) string {
parsed, err := loader.ParseVolume(bind)

View File

@@ -682,6 +682,7 @@ func mergeOptions(t *testing.T, runnerOptions, workflowOptions string, privilege
WorkflowOptions: workflowOptions,
NetworkMode: "bridge",
UsernsMode: "private",
ValidVolumes: []string{"**"},
}}
_, hostConfig, err := cr.mergeContainerConfigs(common.WithLogger(context.Background(), logger), &container.Config{}, &container.HostConfig{
@@ -844,8 +845,10 @@ func TestMergeContainerConfigsVolumesReplaceRunnerMounts(t *testing.T) {
ctx := common.WithLogger(context.Background(), logger)
cr := &containerReference{
input: &NewContainerInput{
NetworkMode: "bridge",
RunnerOptions: "--volume /host/tools:/opt/hostedtoolcache",
NetworkMode: "bridge",
RunnerOptions: "--volume /host/tools:/opt/hostedtoolcache",
WorkflowOptions: "--volume /unapproved:/var/run/docker.sock",
ValidVolumes: []string{"/host/tools"},
},
}
@@ -867,6 +870,7 @@ func TestMergeContainerConfigsWarnsOnlyAboutOptionsThatWereGiven(t *testing.T) {
assert.Zero(t, warnings("--volume /host/tools:/opt/hostedtoolcache", ""))
assert.Zero(t, warnings("", "--shm-size 1g"))
assert.Equal(t, 1, warnings("--network host", ""))
assert.Equal(t, 1, warnings("", "--privileged"))
}
func TestMergeContainerConfigsKeepsNetworkAliasesFromOptions(t *testing.T) {

View File

@@ -107,6 +107,10 @@ func RemoveOrphanNetworks(_ context.Context, _, _ string, _ time.Time) error {
return nil
}
func SanitizeBinds(_ context.Context, _, binds []string) []string {
return binds
}
func CreateJobVolumes(ctx context.Context, runnerUUID string, volumeNames []string) error {
return nil
}

View File

@@ -447,7 +447,7 @@ func newStepContainer(ctx context.Context, step step, image string, cmd, entrypo
envList = append(envList, rc.runnerEnv(ctx)...)
binds, mounts := rc.GetBindsAndMounts()
binds, mounts := rc.GetBindsAndMounts(ctx)
networkMode := "container:" + rc.jobContainerName()
if rc.IsHostEnv() {
networkMode = "default"

View File

@@ -64,7 +64,7 @@ type RunContext struct {
ExprEval *expressionEvaluator
JobContainer container.ExecutionsEnvironment
serviceContainers []*serviceContainer
containerSpec model.ContainerSpec // container:, resolved once with platformImage, zero without a job container
containerSpec model.ContainerSpec // container:, resolved once with platformImage, zero without a container image
JobName string
ActionPath string
Parent *RunContext
@@ -113,6 +113,7 @@ type serviceContainer struct {
name string
image string
container container.ExecutionsEnvironment
created bool
logsDumped bool
info *container.Info // last poll, the source of the `job.services` entry
}
@@ -334,12 +335,12 @@ func splitVolumes(specs []string) ([]string, map[string]string, map[string]bool)
}
// Returns the binds and mounts for the container, resolving paths as appopriate
func (rc *RunContext) GetBindsAndMounts() ([]string, map[string]string) {
func (rc *RunContext) GetBindsAndMounts(ctx context.Context) ([]string, map[string]string) {
name := rc.jobContainerName()
ext := container.LinuxContainerEnvironmentExtensions{}
// the runner's own mounts below yield to the targets the job claims
binds, mounts, claimed := splitVolumes(rc.containerSpec.Volumes)
binds, mounts, claimed := splitVolumes(container.SanitizeBinds(ctx, rc.validVolumes(), rc.containerSpec.Volumes))
if rc.containerDaemonSocket() != "-" && !claimed["/var/run/docker.sock"] {
binds = append(binds, rc.jobDockerSocket()+":/var/run/docker.sock")
@@ -497,10 +498,12 @@ func (rc *RunContext) startJobContainer() common.Executor {
if err := model.DecodeEvaluated("job services", rc.Run.Job().RawServices, rc.ExprEval.shared(ctx).EvaluateYamlNode, &services); err != nil {
return err
}
for serviceID, spec := range services {
for _, serviceID := range slices.Sorted(maps0.Keys(services)) {
spec := services[serviceID]
if spec == nil {
return fmt.Errorf("service %s has no container definition", serviceID)
}
spec.Image = strings.TrimPrefix(spec.Image, "docker://")
// GitHub compatibility: skip services whose image evaluates to an
// empty string, enabling conditional services via expressions
if spec.Image == "" {
@@ -537,6 +540,7 @@ func (rc *RunContext) startJobContainer() common.Executor {
}
serviceContainerName := createContainerName(rc.jobContainerName(), serviceID)
serviceLog := common.NewLineWriter(rawLogLine(ctx)) // raw, so a service's output cannot set the job's env, outputs or state
c := newContainer(&container.NewContainerInput{
Name: serviceContainerName,
Image: spec.Image,
@@ -547,8 +551,8 @@ func (rc *RunContext) startJobContainer() common.Executor {
Env: envs,
Mounts: serviceMounts,
Binds: serviceBinds,
Stdout: logWriter,
Stderr: logWriter,
Stdout: serviceLog,
Stderr: serviceLog,
Privileged: rc.Config.Privileged,
UsernsMode: rc.Config.UsernsMode,
Platform: rc.Config.ContainerArchitecture,
@@ -602,7 +606,7 @@ func (rc *RunContext) startJobContainer() common.Executor {
return err
}
rc.startDockerProxy(ctx)
containerInput.Binds, containerInput.Mounts = rc.GetBindsAndMounts()
containerInput.Binds, containerInput.Mounts = rc.GetBindsAndMounts(ctx)
containerInput.ValidVolumes = rc.validVolumes()
rc.jobNetworkName = networkName
@@ -633,16 +637,21 @@ func (rc *RunContext) startJobContainer() common.Executor {
}
func (rc *RunContext) commandLogWriter(ctx context.Context) io.Writer {
return common.NewLineWriter(rc.commandHandler(ctx), rawLogLine(ctx))
}
func rawLogLine(ctx context.Context) common.LineHandler {
rawLogger := common.Logger(ctx).WithField(rawOutputField, true)
return common.NewLineWriter(rc.commandHandler(ctx), func(line string) bool {
return func(line string) bool {
rawLogger.Infof("%s", line)
return true
})
}
}
func (rc *RunContext) cleanupJobResources(networkName string, createAndDeleteNetwork, preclean bool) common.Executor {
return func(ctx context.Context) error {
logger := common.Logger(ctx)
rc.printServiceLogs(ctx)
errs := []error{rc.closeDockerProxy(ctx)}
if rc.JobContainer != nil {
errs = append(errs, rc.JobContainer.Remove()(ctx))
@@ -789,6 +798,7 @@ func (rc *RunContext) startServiceContainers() common.Executor {
for _, svc := range rc.serviceContainers {
execs = append(execs, common.NewPipelineExecutor(
svc.container.Create(rc.Config.ContainerCapAdd, rc.Config.ContainerCapDrop),
func(context.Context) error { svc.created = true; return nil },
svc.container.Start(false),
))
}
@@ -927,6 +937,26 @@ func (svc *serviceContainer) dumpLogs(ctx context.Context) {
}
}
const serviceLogsTimeout = 15 * time.Second // cleanup gets one minute in total
func (rc *RunContext) printServiceLogs(ctx context.Context) {
ctx, cancel := context.WithTimeout(ctx, serviceLogsTimeout)
defer cancel()
rawLogger := common.Logger(ctx).WithField(rawOutputField, true)
for _, svc := range rc.serviceContainers {
if !svc.created || svc.logsDumped {
continue
}
rawLogger.Infof("::group::Print service container logs: %s", EscapeCommandData(svc.name))
svc.dumpLogs(ctx)
rawLogger.Infof("::endgroup::")
if ctx.Err() != nil {
common.Logger(ctx).Warnf("Timed out printing the service logs after %s", serviceLogsTimeout)
return
}
}
}
// inspect also records the state for the `job.services` context.
func (svc *serviceContainer) inspect(ctx context.Context) (*container.Info, error) {
info, err := svc.container.Inspect(ctx)
@@ -1039,7 +1069,7 @@ func (rc *RunContext) cleanupFailedStart(ctx context.Context) {
}
func (rc *RunContext) IsHostEnv() bool {
return strings.EqualFold(rc.platformImage, "-self-hosted")
return rc.containerSpec.Image == "" && strings.EqualFold(rc.platformImage, "-self-hosted")
}
func (rc *RunContext) stopContainer() common.Executor {
@@ -1162,12 +1192,13 @@ func (rc *RunContext) runsOnPlatformNames(ctx context.Context) []string {
// resolvePlatformImage evaluates the job's container once for every consumer, ignoring one without an image as GitHub does.
func (rc *RunContext) resolvePlatformImage(ctx context.Context) error {
withoutEnv, _ := splitContainerEnv(rc.Run.Job().RawContainer)
var spec *model.ContainerSpec
var spec model.ContainerSpec
if err := model.DecodeEvaluated("container", withoutEnv, rc.ExprEval.shared(ctx).EvaluateYamlNode, &spec); err != nil {
return err
}
if spec != nil && spec.Image != "" {
rc.containerSpec, rc.platformImage = *spec, spec.Image
spec.Image = strings.TrimPrefix(spec.Image, "docker://")
if spec.Image != "" {
rc.containerSpec, rc.platformImage = spec, spec.Image
return nil
}
image, err := rc.runsOnImage(ctx)

View File

@@ -32,6 +32,7 @@ import (
"github.com/docker/cli/cli/compose/loader"
"github.com/moby/moby/api/types/volume"
log "github.com/sirupsen/logrus"
logrustest "github.com/sirupsen/logrus/hooks/test"
assert "github.com/stretchr/testify/assert"
"github.com/stretchr/testify/mock"
require "github.com/stretchr/testify/require"
@@ -342,12 +343,12 @@ jobs:
require.Nil(t, redis.Cmd)
}
func TestStartJobContainerEvaluatesContainersOnce(t *testing.T) {
func TestStartJobContainerEvaluatesContainersOnceAndTrimsDockerPrefix(t *testing.T) {
inputs := startJobContainerInputs(t, `
jobs:
job:
container: ${{ fromJSON('{"image":"node:20","options":"--label ${{ github.job }}","credentials":{"username":"user","password":"${{ vars.TITLE }}"}}') }}
services: ${{ fromJSON('{"redis":{"image":"redis:latest","volumes":["data:/data"],"env":{"TITLE":"${{ github.job }}"},"credentials":{"username":"user","password":"${{ github.job }}"},"entrypoint":"/entry.sh","command":"redis-server --port 6380"}}') }}
container: ${{ fromJSON('{"image":"docker://node:20","options":"--label ${{ github.job }}","credentials":{"username":"user","password":"${{ vars.TITLE }}"}}') }}
services: ${{ fromJSON('{"redis":{"image":"docker://redis:latest","volumes":["data:/data"],"env":{"TITLE":"${{ github.job }}"},"credentials":{"username":"user","password":"${{ github.job }}"},"entrypoint":"/entry.sh","command":"redis-server --port 6380"}}') }}
`, &Config{})
redis, job := inputs[0], inputs[1]
@@ -412,17 +413,14 @@ jobs:
steps: []
`, &Config{ProxyEnv: map[string]string{"http_proxy": "http://proxy:3128", "no_proxy": "internal.example"}})
env := map[string][]string{}
for _, in := range inputs {
env[in.Image] = in.Env
}
db, redis := inputs[0], inputs[1]
require.Contains(t, env["redis:latest"], "http_proxy=http://proxy:3128")
require.Contains(t, env["redis:latest"], "no_proxy=internal.example")
require.Contains(t, redis.Env, "http_proxy=http://proxy:3128")
require.Contains(t, redis.Env, "no_proxy=internal.example")
// the service's own env wins over what the runner injected, without dropping the rest
require.Contains(t, env["postgres:latest"], "no_proxy=db-only.example")
require.NotContains(t, env["postgres:latest"], "no_proxy=internal.example")
require.Contains(t, env["postgres:latest"], "http_proxy=http://proxy:3128")
require.Contains(t, db.Env, "no_proxy=db-only.example")
require.NotContains(t, db.Env, "no_proxy=internal.example")
require.Contains(t, db.Env, "http_proxy=http://proxy:3128")
}
// act builds Dockerfile actions through the API, which does not pre-populate the proxy
@@ -483,7 +481,7 @@ func TestRunContext_GetBindsAndMounts(t *testing.T) {
config := testcase.rc.Config
config.Workdir = testcase.name
config.BindWorkdir = bindWorkDir
gotbind, gotmount := rctemplate.GetBindsAndMounts()
gotbind, gotmount := rctemplate.GetBindsAndMounts(t.Context())
// Name binds/mounts are either/or
if config.BindWorkdir {
@@ -533,7 +531,8 @@ func TestRunContext_GetBindsAndMounts(t *testing.T) {
},
},
Config: &Config{
BindWorkdir: false,
BindWorkdir: false,
ValidVolumes: []string{"**"},
Secrets: map[string]string{
"MAME": "/host/mame/roms",
},
@@ -544,7 +543,7 @@ func TestRunContext_GetBindsAndMounts(t *testing.T) {
rc.ExprEval = rc.NewExpressionEvaluator(context.Background())
require.NoError(t, rc.resolvePlatformImage(context.Background()))
gotbind, gotmount := rc.GetBindsAndMounts()
gotbind, gotmount := rc.GetBindsAndMounts(t.Context())
assert.Contains(t, gotbind, "/host/mame/roms:/root/.mame/roms:ro")
assert.NotContains(t, gotbind, "${{ secrets.MAME }}")
assert.NotContains(t, gotmount, "${{ secrets.MAME }}")
@@ -562,12 +561,13 @@ func TestRunContext_GetBindsAndMounts(t *testing.T) {
Config: &Config{
BindWorkdir: false,
SharedToolCache: true, // so OverridesToolCache has a mount to displace
ValidVolumes: []string{"**"},
},
containerSpec: model.ContainerSpec{Volumes: testcase.volumes},
}
rc.Run.JobID = "job1"
gotbind, gotmount := rc.GetBindsAndMounts()
gotbind, gotmount := rc.GetBindsAndMounts(t.Context())
if len(testcase.wantbind) > 0 {
assert.Contains(t, gotbind, testcase.wantbind)
@@ -602,11 +602,12 @@ func TestRunContext_GetBindsAndMounts(t *testing.T) {
Config: &Config{},
}
_, gotmount := rc.GetBindsAndMounts()
_, gotmount := rc.GetBindsAndMounts(t.Context())
assert.NotContains(t, gotmount, sharedToolCacheVolume)
rc.Config.SharedToolCache = true
_, gotmount = rc.GetBindsAndMounts()
rc.containerSpec.Volumes = []string{"/unapproved:" + container.DefaultToolCache}
_, gotmount = rc.GetBindsAndMounts(t.Context())
assert.Equal(t, container.DefaultToolCache, gotmount[sharedToolCacheVolume])
})
@@ -616,18 +617,18 @@ func TestRunContext_GetBindsAndMounts(t *testing.T) {
}
rc := &RunContext{
Run: &model.Run{JobID: "job1", Workflow: &model.Workflow{Jobs: map[string]*model.Job{"job1": {}}}},
Config: &Config{BindWorkdir: true, Workdir: "/workspace/1/owner/repo", PresetGitHubContext: &model.GithubContext{}},
Config: &Config{BindWorkdir: true, Workdir: "/workspace/1/owner/repo", PresetGitHubContext: &model.GithubContext{}, ValidVolumes: []string{"claimed"}},
}
gotbind, _ := rc.GetBindsAndMounts()
gotbind, _ := rc.GetBindsAndMounts(t.Context())
assert.True(t, slices.ContainsFunc(gotbind, func(bind string) bool { return strings.HasPrefix(bind, "/workspace/1:/workspace/1") }), gotbind)
rc.Config.BindWorkdir = false
_, gotmount := rc.GetBindsAndMounts()
_, gotmount := rc.GetBindsAndMounts(t.Context())
assert.Equal(t, "/workspace/1", gotmount[rc.jobContainerName()])
rc.containerSpec.Volumes = []string{"claimed:/workspace/1"}
_, gotmount = rc.GetBindsAndMounts()
_, gotmount = rc.GetBindsAndMounts(t.Context())
assert.Equal(t, "/workspace/1/owner/repo", gotmount[rc.jobContainerName()])
})
}
@@ -1272,12 +1273,13 @@ func TestRunContext_cleanupFailedStart(t *testing.T) {
}
for _, testcase := range []struct {
name, health string
pullError error
name, health string
pullError, startError error
}{
{name: "healthy", health: container.HealthHealthy},
{name: "unhealthy", health: container.HealthUnhealthy},
{name: "pull failure", pullError: errors.New("pull failed")},
{name: "start failure", startError: errors.New("start failed")},
} {
t.Run(testcase.name, func(t *testing.T) {
var operations []string
@@ -1291,7 +1293,7 @@ func TestRunContext_cleanupFailedStart(t *testing.T) {
for name, instance := range map[string]*containerMock{"job": job, "service": service} {
instance.On("Pull", true).Return(record(name+".Pull", map[string]error{"job": testcase.pullError}[name]))
instance.On("Create", mock.Anything, mock.Anything).Return(record(name+".Create", nil))
instance.On("Start", false).Return(record(name+".Start", nil))
instance.On("Start", false).Return(record(name+".Start", map[string]error{"service": testcase.startError}[name]))
instance.On("Remove").Return(record(name+".Remove", nil))
instance.On("Close").Return(record(name+".Close", nil))
}
@@ -1302,10 +1304,14 @@ func TestRunContext_cleanupFailedStart(t *testing.T) {
service.On("Inspect", mock.Anything).Return(&container.Info{State: "running", Health: health}, nil).
Run(func(mock.Arguments) { operations = append(operations, "service.Inspect") }).Once()
}
service.On("DumpLogs", mock.Anything).Return(nil).
Run(func(mock.Arguments) { operations = append(operations, "service.DumpLogs") })
stdouts := map[string]io.Writer{}
service.On("DumpLogs", mock.Anything).Return(nil).Run(func(mock.Arguments) {
operations = append(operations, "service.DumpLogs")
fmt.Fprintln(stdouts["postgres:latest"], "::add-mask::leak")
})
origNewContainer := newContainer
newContainer = func(input *container.NewContainerInput) container.ExecutionsEnvironment {
stdouts[input.Image] = input.Stdout
return map[string]*containerMock{"postgres:latest": service, "node:20": job}[input.Image]
}
t.Cleanup(func() { newContainer = origNewContainer })
@@ -1316,22 +1322,30 @@ func TestRunContext_cleanupFailedStart(t *testing.T) {
Run: &model.Run{JobID: "job", Workflow: workflow},
platformImage: "node:20",
}
ctx := common.WithDryrun(t.Context(), true)
logger, hook := logrustest.NewNullLogger()
ctx := common.WithLogger(common.WithDryrun(t.Context(), true), logger)
rc.ExprEval = rc.NewExpressionEvaluator(ctx)
err = rc.startContainer().Then(rc.stopContainer()).Finally(rc.closeContainer())(ctx)
want := "job.Remove service.Remove service.Close service.Pull job.Pull"
if testcase.pullError != nil {
switch {
case testcase.pullError != nil:
require.ErrorIs(t, err, testcase.pullError)
} else {
want += " service.Create service.Start service.Inspect job.Create job.Start job.Inspect job.Copy service.Inspect"
case testcase.startError != nil:
require.ErrorIs(t, err, testcase.startError)
want += " service.Create service.Start service.DumpLogs"
default:
want += " service.Create service.Start service.Inspect job.Create job.Start job.Inspect job.Copy service.Inspect service.DumpLogs"
if testcase.health == container.HealthUnhealthy {
require.ErrorContains(t, err, "the service 'postgres' is unhealthy")
want += " service.DumpLogs"
} else {
require.NoError(t, err)
}
}
assert.Equal(t, strings.Fields(want+" job.Remove service.Remove service.Close job.Close"), operations)
assert.Empty(t, rc.Masks)
assert.Equal(t, testcase.pullError == nil && testcase.health != container.HealthUnhealthy, slices.ContainsFunc(hook.AllEntries(), func(entry *log.Entry) bool {
return strings.HasPrefix(entry.Message, "::group::Print service container logs")
}))
})
}
@@ -1593,6 +1607,12 @@ func createRunsOnRunContext(t *testing.T, runsOn string) *RunContext {
})
}
func TestRunContextContainerImageCannotSelectHost(t *testing.T) {
rc := createIfTestRunContext(map[string]*model.Job{"job1": createJob(t, "container: -self-hosted", "")})
require.NoError(t, rc.resolvePlatformImage(t.Context()))
assert.False(t, rc.IsHostEnv())
}
func TestRunContextImageOS(t *testing.T) {
ctx := context.Background()