From 0192861155a075db357987e5111af33ff0738940 Mon Sep 17 00:00:00 2001 From: silverwind Date: Wed, 29 Jul 2026 21:32:28 +0000 Subject: [PATCH] fix: allow relocating the tool cache and mounting over runner paths (#1122) 1. Docker rejects two mounts on one target, so a `container.volumes:` or `--volume` aimed at `/opt/hostedtoolcache` failed the job with `Duplicate mount point`. Job and service volumes now displace the mount on the same path, and `name:/target:ro` no longer mounts read-write at the literal path `/target:ro`. 1. Setting `RUNNER_TOOL_CACHE` only changed what the variable said, the cache stayed where it was, so tools writing to it landed outside the mount and `${{ runner.tool_cache }}` disagreed with the variable. It now relocates the cache. Leaving it unset behaves as before. 1. Unknown `config.yaml` keys now warn instead of being dropped without a trace. Fixes https://gitea.com/gitea/runner/issues/813 --------- Co-authored-by: bircni Reviewed-on: https://gitea.com/gitea/runner/pulls/1122 Reviewed-by: bircni Co-authored-by: silverwind --- act/container/docker_run.go | 31 +++- act/container/docker_run_test.go | 20 +++ .../linux_container_environment_extensions.go | 5 +- act/runner/action.go | 5 +- act/runner/run_context.go | 136 +++++++++++------- act/runner/run_context_test.go | 41 +++++- act/runner/step_docker.go | 5 +- internal/pkg/config/config.example.yaml | 6 +- internal/pkg/config/config.go | 17 +++ internal/pkg/config/config_test.go | 16 +++ 10 files changed, 210 insertions(+), 72 deletions(-) diff --git a/act/container/docker_run.go b/act/container/docker_run.go index cd10668c..0e3a1616 100644 --- a/act/container/docker_run.go +++ b/act/container/docker_run.go @@ -471,8 +471,7 @@ func (cr *containerReference) mergeContainerConfigs(ctx context.Context, config logger.Debugf("Custom container.HostConfig from options ==> %+v", containerConfig.HostConfig) - hostConfig.Binds = append(hostConfig.Binds, containerConfig.HostConfig.Binds...) - hostConfig.Mounts = append(hostConfig.Mounts, containerConfig.HostConfig.Mounts...) + overlayVolumes(hostConfig, containerConfig.HostConfig) binds := hostConfig.Binds mounts := hostConfig.Mounts networkMode := hostConfig.NetworkMode @@ -1108,6 +1107,34 @@ func (cr *containerReference) sanitizeConfig(ctx context.Context, config *contai return config, hostConfig } +// 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) + if err != nil { + return "" + } + return parsed.Target +} + +// overlayVolumes appends src's volumes to dst, dropping the dst ones they mount over. Docker +// rejects two mounts on one target, so the volumes declared last have to win. +func overlayVolumes(dst, src *container.HostConfig) { + claimed := map[string]bool{} + for _, bind := range src.Binds { + if target := bindTarget(bind); target != "" { + claimed[target] = true + } + } + for _, mt := range src.Mounts { + claimed[mt.Target] = true + } + + dst.Binds = append(slices.DeleteFunc(slices.Clone(dst.Binds), + func(bind string) bool { return claimed[bindTarget(bind)] }), src.Binds...) + dst.Mounts = append(slices.DeleteFunc(slices.Clone(dst.Mounts), + func(mt mount.Mount) bool { return claimed[mt.Target] }), src.Mounts...) +} + type validVolumeMatcher struct { allowAll bool named []glob.Glob diff --git a/act/container/docker_run_test.go b/act/container/docker_run_test.go index 57c180fa..8f362a88 100644 --- a/act/container/docker_run_test.go +++ b/act/container/docker_run_test.go @@ -23,6 +23,7 @@ import ( cerrdefs "github.com/containerd/errdefs" "github.com/moby/moby/api/pkg/stdcopy" "github.com/moby/moby/api/types/container" + "github.com/moby/moby/api/types/mount" mobyclient "github.com/moby/moby/client" "github.com/sirupsen/logrus/hooks/test" "github.com/stretchr/testify/assert" @@ -660,3 +661,22 @@ func TestCheckVolumesRejectsEscapingHostPaths(t *testing.T) { }) assert.Empty(t, hostConf.Binds) } + +func TestMergeContainerConfigsVolumesReplaceRunnerMounts(t *testing.T) { + logger, _ := test.NewNullLogger() + ctx := common.WithLogger(context.Background(), logger) + cr := &containerReference{ + input: &NewContainerInput{ + NetworkMode: "bridge", + Options: "--volume /host/tools:/opt/hostedtoolcache", + }, + } + + _, hostConf, err := cr.mergeContainerConfigs(ctx, &container.Config{}, &container.HostConfig{ + Binds: []string{"/var/run/docker.sock:/var/run/docker.sock"}, + Mounts: []mount.Mount{{Type: mount.TypeVolume, Source: "act-toolcache", Target: "/opt/hostedtoolcache"}}, + }) + require.NoError(t, err) + assert.Equal(t, []string{"/var/run/docker.sock:/var/run/docker.sock", "/host/tools:/opt/hostedtoolcache"}, hostConf.Binds) + assert.Empty(t, hostConf.Mounts) +} diff --git a/act/container/linux_container_environment_extensions.go b/act/container/linux_container_environment_extensions.go index d896f2b3..32ed8e75 100644 --- a/act/container/linux_container_environment_extensions.go +++ b/act/container/linux_container_environment_extensions.go @@ -66,12 +66,15 @@ func (*LinuxContainerEnvironmentExtensions) JoinPathVariable(paths ...string) st return strings.Join(paths, ":") } +// DefaultToolCache is where the runner mounts the tool cache inside job containers. +const DefaultToolCache = "/opt/hostedtoolcache" + func (*LinuxContainerEnvironmentExtensions) GetRunnerContext(ctx context.Context) map[string]any { return map[string]any{ "os": "Linux", "arch": RunnerArch(ctx), "temp": "/tmp", - "tool_cache": "/opt/hostedtoolcache", + "tool_cache": DefaultToolCache, } } diff --git a/act/runner/action.go b/act/runner/action.go index 700b1f29..c8f7a141 100644 --- a/act/runner/action.go +++ b/act/runner/action.go @@ -472,10 +472,7 @@ func newStepContainer(ctx context.Context, step step, image string, cmd, entrypo envList = append(envList, fmt.Sprintf("%s=%s", k, v)) } - envList = append(envList, fmt.Sprintf("%s=%s", "RUNNER_TOOL_CACHE", "/opt/hostedtoolcache")) - envList = append(envList, fmt.Sprintf("%s=%s", "RUNNER_OS", "Linux")) - envList = append(envList, fmt.Sprintf("%s=%s", "RUNNER_ARCH", container.RunnerArch(ctx))) - envList = append(envList, fmt.Sprintf("%s=%s", "RUNNER_TEMP", "/tmp")) + envList = append(envList, rc.runnerEnv(ctx)...) binds, mounts := rc.GetBindsAndMounts() networkMode := "container:" + rc.jobContainerName() diff --git a/act/runner/run_context.go b/act/runner/run_context.go index 6cfea8ab..777c51af 100644 --- a/act/runner/run_context.go +++ b/act/runner/run_context.go @@ -30,7 +30,9 @@ import ( "gitea.com/gitea/runner/act/exprparser" "gitea.com/gitea/runner/act/model" + "github.com/docker/cli/cli/compose/loader" "github.com/docker/go-connections/nat" + "github.com/moby/moby/api/types/mount" "github.com/opencontainers/selinux/go-selinux" ) @@ -204,52 +206,93 @@ func (rc *RunContext) validVolumes() []string { getDockerDaemonSocketMountPath(rc.containerDaemonSocket())) } +// toolCache returns the tool cache path the job sees, relocatable through RUNNER_TOOL_CACHE. +func (rc *RunContext) toolCache(fallback string) string { + if path := rc.GetEnv()["RUNNER_TOOL_CACHE"]; path != "" { + return path + } + return fallback +} + +// runnerEnv returns a container's RUNNER_* variables, derived from the values runner.tool_cache +// and friends report so the two cannot drift apart. +func (rc *RunContext) runnerEnv(ctx context.Context) []string { + ext := container.LinuxContainerEnvironmentExtensions{} + runnerContext := ext.GetRunnerContext(ctx) + runnerContext["tool_cache"] = rc.toolCache(container.DefaultToolCache) + + env := make([]string, 0, len(runnerContext)) + for key, value := range runnerContext { + env = append(env, fmt.Sprintf("RUNNER_%s=%s", strings.ToUpper(key), value)) + } + slices.Sort(env) + return env +} + +// splitVolumes routes volume specs into binds and a source:target mount map, and returns the +// container paths they mount onto. Only a plain source:target volume fits the map, everything +// else (anonymous volumes, host binds, mount options) stays a bind. +func splitVolumes(specs []string) ([]string, map[string]string, map[string]bool) { + binds := []string{} + mounts := map[string]string{} + targets := map[string]bool{} + + for _, spec := range specs { + parsed, err := loader.ParseVolume(spec) + if err != nil { + binds = append(binds, spec) // let Docker report the malformed spec + continue + } + targets[parsed.Target] = true + if parsed.Type == string(mount.TypeVolume) && parsed.Source != "" && !parsed.ReadOnly { + mounts[parsed.Source] = parsed.Target + } else { + binds = append(binds, spec) + } + } + return binds, mounts, targets +} + // Returns the binds and mounts for the container, resolving paths as appopriate func (rc *RunContext) GetBindsAndMounts() ([]string, map[string]string) { name := rc.jobContainerName() - - binds := []string{} - if daemonSocket := rc.containerDaemonSocket(); daemonSocket != "-" { - daemonPath := getDockerDaemonSocketMountPath(daemonSocket) - binds = append(binds, fmt.Sprintf("%s:%s", daemonPath, "/var/run/docker.sock")) - } - ext := container.LinuxContainerEnvironmentExtensions{} - mounts := map[string]string{ - "act-toolcache": "/opt/hostedtoolcache", - name + "-env": ext.GetActPath(), - } - + var volumes []string if job := rc.Run.Job(); job != nil { if container := job.Container(); container != nil { for _, v := range container.Volumes { if rc.ExprEval != nil { v = rc.ExprEval.Interpolate(context.Background(), v) } - if !strings.Contains(v, ":") || filepath.IsAbs(v) { - // Bind anonymous volume or host file. - binds = append(binds, v) - } else { - // Mount existing volume. - paths := strings.SplitN(v, ":", 2) - mounts[paths[0]] = paths[1] - } + volumes = append(volumes, v) } } } + // the runner's own mounts below yield to the targets the job claims + binds, mounts, claimed := splitVolumes(volumes) - if rc.Config.BindWorkdir { - bindModifiers := "" - if runtime.GOOS == "darwin" { - bindModifiers = ":delegated" + if daemonSocket := rc.containerDaemonSocket(); daemonSocket != "-" && !claimed["/var/run/docker.sock"] { + binds = append(binds, getDockerDaemonSocketMountPath(daemonSocket)+":/var/run/docker.sock") + } + if toolCache := rc.toolCache(container.DefaultToolCache); !claimed[toolCache] { + mounts["act-toolcache"] = toolCache + } + mounts[name+"-env"] = ext.GetActPath() // runner-internal, never overridable + + if workdir := ext.ToContainerPath(rc.Config.Workdir); !claimed[workdir] { + if rc.Config.BindWorkdir { + bindModifiers := "" + if runtime.GOOS == "darwin" { + bindModifiers = ":delegated" + } + if selinux.GetEnabled() { + bindModifiers = ":z" + } + binds = append(binds, fmt.Sprintf("%s:%s%s", rc.Config.Workdir, workdir, bindModifiers)) + } else { + mounts[name] = workdir } - if selinux.GetEnabled() { - bindModifiers = ":z" - } - binds = append(binds, fmt.Sprintf("%s:%s%s", rc.Config.Workdir, ext.ToContainerPath(rc.Config.Workdir), bindModifiers)) - } else { - mounts[name] = ext.ToContainerPath(rc.Config.Workdir) } return binds, mounts @@ -283,7 +326,10 @@ func (rc *RunContext) startHostEnvironment() common.Executor { if err := os.MkdirAll(runnerTmp, 0o777); err != nil { return err } - toolCache := filepath.Join(cacheDir, "tool_cache") + toolCache := rc.toolCache(filepath.Join(cacheDir, "tool_cache")) + if err := os.MkdirAll(toolCache, 0o777); err != nil { + return err + } rc.JobContainer = &container.HostEnvironment{ Path: path, TmpDir: runnerTmp, @@ -368,10 +414,7 @@ func (rc *RunContext) startJobContainer() common.Executor { envList := make([]string, 0) - envList = append(envList, fmt.Sprintf("%s=%s", "RUNNER_TOOL_CACHE", "/opt/hostedtoolcache")) - envList = append(envList, fmt.Sprintf("%s=%s", "RUNNER_OS", "Linux")) - envList = append(envList, fmt.Sprintf("%s=%s", "RUNNER_ARCH", container.RunnerArch(ctx))) - envList = append(envList, fmt.Sprintf("%s=%s", "RUNNER_TEMP", "/tmp")) + envList = append(envList, rc.runnerEnv(ctx)...) envList = append(envList, fmt.Sprintf("%s=%s", "LANG", "C.UTF-8")) // Use same locale as GitHub Actions ext := container.LinuxContainerEnvironmentExtensions{} @@ -992,6 +1035,8 @@ func (rc *RunContext) getRunnerContext(ctx context.Context) map[string]any { runnerContext := map[string]any{} if rc.JobContainer != nil { maps0.Copy(runnerContext, rc.JobContainer.GetRunnerContext(ctx)) + defaultToolCache, _ := runnerContext["tool_cache"].(string) + runnerContext["tool_cache"] = rc.toolCache(defaultToolCache) } runnerContext["name"] = rc.Config.RunnerName runnerContext["environment"] = "self-hosted" @@ -1363,24 +1408,9 @@ func (rc *RunContext) handleServiceCredentials(ctx context.Context, creds map[st // GetServiceBindsAndMounts returns the binds and mounts for the service container, resolving paths as appopriate func (rc *RunContext) GetServiceBindsAndMounts(svcVolumes []string) ([]string, map[string]string) { - binds := []string{} - if daemonSocket := rc.containerDaemonSocket(); daemonSocket != "-" { - daemonPath := getDockerDaemonSocketMountPath(daemonSocket) - binds = append(binds, fmt.Sprintf("%s:%s", daemonPath, "/var/run/docker.sock")) + binds, mounts, claimed := splitVolumes(svcVolumes) + if daemonSocket := rc.containerDaemonSocket(); daemonSocket != "-" && !claimed["/var/run/docker.sock"] { + binds = append(binds, getDockerDaemonSocketMountPath(daemonSocket)+":/var/run/docker.sock") } - - mounts := map[string]string{} - - for _, v := range svcVolumes { - if !strings.Contains(v, ":") || filepath.IsAbs(v) { - // Bind anonymous volume or host file. - binds = append(binds, v) - } else { - // Mount existing volume. - paths := strings.SplitN(v, ":", 2) - mounts[paths[0]] = paths[1] - } - } - return binds, mounts } diff --git a/act/runner/run_context_test.go b/act/runner/run_context_test.go index 078d62bc..8c72cdb2 100644 --- a/act/runner/run_context_test.go +++ b/act/runner/run_context_test.go @@ -19,6 +19,7 @@ import ( "gitea.com/gitea/runner/act/exprparser" "gitea.com/gitea/runner/act/model" + "github.com/docker/cli/cli/compose/loader" log "github.com/sirupsen/logrus" assert "github.com/stretchr/testify/assert" require "github.com/stretchr/testify/require" @@ -363,6 +364,10 @@ func TestRunContext_GetBindsAndMounts(t *testing.T) { {"BindAnonymousVolume", []string{"/volume"}, "/volume", map[string]string{}}, {"BindHostFile", []string{"/path/to/file/on/host:/volume"}, "/path/to/file/on/host:/volume", map[string]string{}}, {"MountExistingVolume", []string{"volume-id:/volume"}, "", map[string]string{"volume-id": "/volume"}}, + {"MountExistingVolumeReadOnly", []string{"volume-id:/volume:ro"}, "volume-id:/volume:ro", map[string]string{}}, + {"BindRelativeHostPath", []string{"./relative:/volume"}, "./relative:/volume", map[string]string{}}, + {"OverridesToolCache", []string{"/host/tools:/opt/hostedtoolcache"}, "/host/tools:/opt/hostedtoolcache", map[string]string{}}, + {"OverridesDockerSocket", []string{"/host/docker.sock:/var/run/docker.sock"}, "/host/docker.sock:/var/run/docker.sock", map[string]string{}}, } t.Run("InterpolatedContainerVolumes", func(t *testing.T) { @@ -418,15 +423,37 @@ func TestRunContext_GetBindsAndMounts(t *testing.T) { rc.Run.JobID = "job1" rc.Run.Workflow.Jobs = map[string]*model.Job{"job1": job} - gotbind, gotmount := rc.GetBindsAndMounts() + jobBinds, jobMounts := rc.GetBindsAndMounts() + svcBinds, svcMounts := rc.GetServiceBindsAndMounts(testcase.volumes) + // job and service containers classify volumes alike, only their own mounts differ + for _, got := range []struct { + binds []string + mounts map[string]string + }{{jobBinds, jobMounts}, {svcBinds, svcMounts}} { + gotbind, gotmount := got.binds, got.mounts - if len(testcase.wantbind) > 0 { - assert.Contains(t, gotbind, testcase.wantbind) - } + if len(testcase.wantbind) > 0 { + assert.Contains(t, gotbind, testcase.wantbind) + } - for k, v := range testcase.wantmount { - assert.Contains(t, gotmount, k) - assert.Equal(t, gotmount[k], v) + for k, v := range testcase.wantmount { + assert.Contains(t, gotmount, k) + assert.Equal(t, gotmount[k], v) + } + + // Docker rejects a container with two mounts on one target, so the job's own + // volumes must displace the runner's rather than pile up next to them. + targets := map[string]bool{} + for _, bind := range gotbind { + parsed, err := loader.ParseVolume(bind) + require.NoError(t, err) + assert.NotContains(t, targets, parsed.Target, "%s mounts an already mounted target", bind) + targets[parsed.Target] = true + } + for source, target := range gotmount { + assert.NotContains(t, targets, target, "%s mounts an already mounted target", source) + targets[target] = true + } } }) } diff --git a/act/runner/step_docker.go b/act/runner/step_docker.go index ecdf26f9..a0b9edc6 100644 --- a/act/runner/step_docker.go +++ b/act/runner/step_docker.go @@ -110,10 +110,7 @@ func (sd *stepDocker) newStepContainer(ctx context.Context, image string, cmd, e envList = append(envList, fmt.Sprintf("%s=%s", k, v)) } - envList = append(envList, fmt.Sprintf("%s=%s", "RUNNER_TOOL_CACHE", "/opt/hostedtoolcache")) - envList = append(envList, fmt.Sprintf("%s=%s", "RUNNER_OS", "Linux")) - envList = append(envList, fmt.Sprintf("%s=%s", "RUNNER_ARCH", container.RunnerArch(ctx))) - envList = append(envList, fmt.Sprintf("%s=%s", "RUNNER_TEMP", "/tmp")) + envList = append(envList, rc.runnerEnv(ctx)...) binds, mounts := rc.GetBindsAndMounts() networkMode := "container:" + rc.jobContainerName() diff --git a/internal/pkg/config/config.example.yaml b/internal/pkg/config/config.example.yaml index 4a30c53e..3486db29 100644 --- a/internal/pkg/config/config.example.yaml +++ b/internal/pkg/config/config.example.yaml @@ -167,7 +167,11 @@ container: enable_ipv6: false # Omit to use Docker's default (IPv6 disabled). Enabling it requires dockerd started with --ipv6. # Whether to use privileged mode or not when launching task containers (privileged mode is required for Docker-in-Docker). privileged: false - # Any other options to be used when the container is started (e.g., --add-host=my.gitea.url:host-gateway). + # Any other options to be used when the container is started, for example: + # options: --add-host=my.gitea.url:host-gateway + # A volume declared here replaces the one the runner mounts on the same container path, so the + # tool cache can be kept on the host. Its source must also be allowed by valid_volumes below: + # options: --volume /host/toolcache:/opt/hostedtoolcache options: # The parent directory of a job's working directory. # NOTE: There is no need to add the first '/' of the path as runner will add it automatically. diff --git a/internal/pkg/config/config.go b/internal/pkg/config/config.go index b6295bda..7e2fe29a 100644 --- a/internal/pkg/config/config.go +++ b/internal/pkg/config/config.go @@ -4,6 +4,7 @@ package config import ( + "bytes" "errors" "fmt" "maps" @@ -143,6 +144,7 @@ func LoadDefault(file string) (*Config, error) { if err := yaml.Unmarshal(content, cfg); err != nil { return nil, fmt.Errorf("parse config file %q: %w", file, err) } + warnUnknownKeys(file, content) definedRunnerKeys, err = definedRunnerConfigKeys(content) if err != nil { return nil, fmt.Errorf("parse config file %q for defaults metadata: %w", file, err) @@ -288,6 +290,21 @@ func LoadDefault(file string) (*Config, error) { return cfg, nil } +// warnUnknownKeys reports keys the config does not define, which are otherwise ignored +// without a trace. It only warns, so a config carrying keys from another runner version +// still loads. +func warnUnknownKeys(file string, content []byte) { + decoder := yaml.NewDecoder(bytes.NewReader(content)) + decoder.KnownFields(true) + + var typeErr *yaml.TypeError + if err := decoder.Decode(&Config{}); errors.As(err, &typeErr) { + for _, message := range typeErr.Errors { + log.Warnf("config file %q: %s, it will be ignored", file, message) + } + } +} + func definedRunnerConfigKeys(content []byte) (map[string]bool, error) { var root yaml.Node if err := yaml.Unmarshal(content, &root); err != nil { diff --git a/internal/pkg/config/config_test.go b/internal/pkg/config/config_test.go index 04a100ab..39da42fd 100644 --- a/internal/pkg/config/config_test.go +++ b/internal/pkg/config/config_test.go @@ -9,6 +9,7 @@ import ( "testing" "time" + "github.com/sirupsen/logrus/hooks/test" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -197,6 +198,21 @@ func TestLoadDefault_MalformedYAMLReturnsParseError(t *testing.T) { assert.NotContains(t, err.Error(), "defaults metadata") } +func TestLoadDefault_WarnsOnUnknownKeysButStillLoads(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "config.yaml") + require.NoError(t, os.WriteFile(path, []byte("container:\n volumes:\n - /host:/ctr\n privileged: true\n"), 0o600)) + + hook := test.NewGlobal() + defer hook.Reset() + + cfg, err := LoadDefault(path) + require.NoError(t, err) + assert.True(t, cfg.Container.Privileged) + require.Len(t, hook.Entries, 1) + assert.Contains(t, hook.LastEntry().Message, "field volumes not found") +} + func TestContainerNetworkCreateOptions(t *testing.T) { // Verify that the enable_ipv4/enable_ipv6 YAML keys unmarshal into the *bool fields, // distinguishing an explicit true/false from an omitted key (nil). A nil here is