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 <bircni@icloud.com>
Reviewed-on: https://gitea.com/gitea/runner/pulls/1122
Reviewed-by: bircni <bircni@icloud.com>
Co-authored-by: silverwind <me@silverwind.io>

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

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)
+}

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,
 	}
 }
 

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()

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
 }

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
+					}
 				}
 			})
 		}

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()

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.

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 {

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