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

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

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

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

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

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.