From d9f4d655453b5ab34e0c763330edef54a7b94c3b Mon Sep 17 00:00:00 2001 From: silverwind Date: Wed, 26 Aug 2026 15:55:27 +0000 Subject: [PATCH] fix: honor volumes declared on service containers (#1186) Service containers were built without a volume policy, so every bind and mount they declared was dropped, whatever `valid_volumes` allowed. GitHub passes a service's declared volumes straight to `docker create`, so a workflow that mounts into a service silently did nothing here. Services now get the configured policy, but not `validVolumes()`, which would also hand them the docker daemon socket that GitHub mounts only into the job container. ### What changes for users On the default `valid_volumes: []` nothing changes: a service's volumes are still dropped, now with a warning rather than in silence. Once `valid_volumes` is configured, a service's declared volumes are honored under it instead of discarded, which is what that setting already documents. No workflow that worked before stops working, and a service can reach no volume the policy does not already allow the job container, so this is not a breaking change. --------- Co-authored-by: bircni Reviewed-on: https://gitea.com/gitea/runner/pulls/1186 Reviewed-by: bircni Co-authored-by: silverwind --- act/container/docker_run.go | 6 +++ act/container/docker_run_test.go | 3 +- act/runner/run_context.go | 19 ++------ act/runner/run_context_test.go | 65 ++++++++++++++----------- internal/pkg/config/config.example.yaml | 2 +- 5 files changed, 51 insertions(+), 44 deletions(-) diff --git a/act/container/docker_run.go b/act/container/docker_run.go index d3e86e40..21ed49d1 100644 --- a/act/container/docker_run.go +++ b/act/container/docker_run.go @@ -1221,6 +1221,12 @@ func (cr *containerReference) sanitizeConfig(ctx context.Context, config *contai } hostConfig.Mounts = sanitizedMounts } else { + for _, bind := range hostConfig.Binds { + logger.Warnf("[%s] is not a valid volume, will be ignored", bind) + } + for _, mt := range hostConfig.Mounts { + logger.Warnf("[%s] is not a valid volume, will be ignored", mt.Source) + } hostConfig.Binds = []string{} hostConfig.Mounts = []mount.Mount{} } diff --git a/act/container/docker_run_test.go b/act/container/docker_run_test.go index 3998c8a4..dc274055 100644 --- a/act/container/docker_run_test.go +++ b/act/container/docker_run_test.go @@ -570,7 +570,7 @@ func TestCheckVolumes(t *testing.T) { } for _, tc := range testCases { t.Run(tc.desc, func(t *testing.T) { - logger, _ := test.NewNullLogger() + logger, hook := test.NewNullLogger() ctx := common.WithLogger(context.Background(), logger) cr := &containerReference{ input: &NewContainerInput{ @@ -579,6 +579,7 @@ func TestCheckVolumes(t *testing.T) { } _, hostConf := cr.sanitizeConfig(ctx, &container.Config{}, &container.HostConfig{Binds: tc.binds}) assert.Equal(t, tc.expectedBinds, hostConf.Binds) + assert.Len(t, hook.AllEntries(), len(tc.binds)-len(tc.expectedBinds)) // every drop is warned about }) } } diff --git a/act/runner/run_context.go b/act/runner/run_context.go index c08b6a07..c6b400ef 100644 --- a/act/runner/run_context.go +++ b/act/runner/run_context.go @@ -235,9 +235,8 @@ func (rc *RunContext) containerDaemonSocket() string { const sharedToolCacheVolume = "act-toolcache" // mounted only when the tool cache is shared -// validVolumes returns the volumes allowed on this job's containers: the configured base -// plus the volumes the runner mounts automatically. It derives a fresh slice every call and -// never mutates the shared Config (see containerDaemonSocket). +// validVolumes returns what the job and action containers may mount, the configured base plus +// the runner's own volumes. Fresh slice per call, the shared Config is never mutated. func (rc *RunContext) validVolumes() []string { name := rc.jobContainerName() volumes := slices.Clone(rc.Config.ValidVolumes) @@ -283,7 +282,7 @@ func splitVolumes(specs []string) ([]string, map[string]string, 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 + binds = append(binds, spec) // unclassifiable, sanitizeConfig warns and drops it continue } targets[parsed.Target] = true @@ -493,7 +492,7 @@ func (rc *RunContext) startJobContainer() common.Executor { for _, volume := range spec.Volumes { interpolatedVolumes = append(interpolatedVolumes, rc.ExprEval.Interpolate(ctx, volume)) } - serviceBinds, serviceMounts := rc.GetServiceBindsAndMounts(interpolatedVolumes) + serviceBinds, serviceMounts, _ := splitVolumes(interpolatedVolumes) interpolatedPorts := make([]string, 0, len(spec.Ports)) for _, port := range spec.Ports { @@ -526,6 +525,7 @@ func (rc *RunContext) startJobContainer() common.Executor { NetworkAliases: []string{serviceID}, ExposedPorts: exposedPorts, PortBindings: portBindings, + ValidVolumes: rc.Config.ValidVolumes, // not validVolumes(), a service gets no docker socket AllocatePTY: rc.Config.AllocatePTY, }) rc.serviceContainers = append(rc.serviceContainers, &serviceContainer{name: serviceID, image: serviceImage, container: c}) @@ -1587,12 +1587,3 @@ func (rc *RunContext) interpolateCredentials(ctx context.Context, credentials ma return username, password, nil } - -// 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, mounts, claimed := splitVolumes(svcVolumes) - if daemonSocket := rc.containerDaemonSocket(); daemonSocket != "-" && !claimed["/var/run/docker.sock"] { - binds = append(binds, getDockerDaemonSocketMountPath(daemonSocket)+":/var/run/docker.sock") - } - return binds, mounts -} diff --git a/act/runner/run_context_test.go b/act/runner/run_context_test.go index 969c3f2b..77e8e313 100644 --- a/act/runner/run_context_test.go +++ b/act/runner/run_context_test.go @@ -311,6 +311,23 @@ jobs: require.Equal(t, [2]string{"", ""}, credentials["redis:latest"]) } +func TestStartJobContainerGivesServicesTheirVolumes(t *testing.T) { + redis := startJobContainerInputs(t, ` +jobs: + job: + services: + redis: + image: redis:latest + volumes: + - data:/data +`, &Config{ValidVolumes: []string{"data"}})[0] + + require.Equal(t, "redis:latest", redis.Image) // services are built before the job container + require.Equal(t, []string{"data"}, redis.ValidVolumes) + require.Equal(t, map[string]string{"data": "/data"}, redis.Mounts) + require.Empty(t, redis.Binds) // the docker socket is the job container's alone +} + // Only the workflow's options may be stripped later, so the two sources have to reach the // container apart from each other. func TestStartJobContainerKeepsRunnerOptionsApartFromWorkflowOptions(t *testing.T) { @@ -520,37 +537,29 @@ func TestRunContext_GetBindsAndMounts(t *testing.T) { rc.Run.JobID = "job1" rc.Run.Workflow.Jobs = map[string]*model.Job{"job1": job} - 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 + gotbind, gotmount := rc.GetBindsAndMounts() - 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 - } + // 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/internal/pkg/config/config.example.yaml b/internal/pkg/config/config.example.yaml index 1b394b02..34575127 100644 --- a/internal/pkg/config/config.example.yaml +++ b/internal/pkg/config/config.example.yaml @@ -241,7 +241,7 @@ container: #valid_volumes: [] # Overrides the docker client host with the specified one. # If it's empty, runner will find an available docker host automatically. - # If it's "-", runner will find an available docker host automatically, but the docker host won't be mounted to the job containers and service containers. + # If it's "-", runner will find an available docker host automatically, but won't mount it into the job and action containers. # If it's not empty or "-", the specified docker host will be used. An error will be returned if it doesn't work. #docker_host: "" # Pull docker image(s) even if already present.