From 197f6c1fd7fcae2a0c4314485286af3fffec387c Mon Sep 17 00:00:00 2001 From: Yusuke Kuoka Date: Mon, 25 Apr 2022 01:02:23 +0000 Subject: [PATCH] feat: Support arbitrarily setting `privileged: true` for runner container Resolves #1282 --- README.md | 23 +++++++++++++ controllers/new_runner_pod_test.go | 5 ++- controllers/runner_controller.go | 52 +++++++++++++++++++++++------- 3 files changed, 66 insertions(+), 14 deletions(-) diff --git a/README.md b/README.md index 0a7ecc5125..e580007450 100644 --- a/README.md +++ b/README.md @@ -435,6 +435,17 @@ spec: requests: cpu: "2.0" memory: "4Gi" + # This is an advanced configuration. Don't touch it unless you know what you're doing. + securityContext: + # Usually, the runner container's privileged field is derived from dockerdWithinRunnerContainer. + # But in the case where you need to run privileged job steps even if you don't use docker/don't need dockerd within the runner container, + # just specified `privileged: true` like this. + # See https://github.com/actions-runner-controller/actions-runner-controller/issues/1282 + # Do note that specifying `privileged: false` while using dind is very likely to fail, even if you use some vm-based container runtimes + # like firecracker and kata. Basically they run containers within dedicated micro vms and so + # it's more like you can use `privileged: true` safer with those runtimes. + # + # privileged: true - name: docker resources: limits: @@ -1125,6 +1136,18 @@ spec: # This must match the name of a RuntimeClass resource available on the cluster. # More info: https://kubernetes.io/docs/concepts/containers/runtime-class runtimeClassName: "runc" + # This is an advanced configuration. Don't touch it unless you know what you're doing. + containers: + - name: runner + # Usually, the runner container's privileged field is derived from dockerdWithinRunnerContainer. + # But in the case where you need to run privileged job steps even if you don't use docker/don't need dockerd within the runner container, + # just specified `privileged: true` like this. + # See https://github.com/actions-runner-controller/actions-runner-controller/issues/1282 + # Do note that specifying `privileged: false` while using dind is very likely to fail, even if you use some vm-based container runtimes + # like firecracker and kata. Basically they run containers within dedicated micro vms and so + # it's more like you can use `privileged: true` safer with those runtimes. + # + # privileged: true ``` ### Custom Volume mounts diff --git a/controllers/new_runner_pod_test.go b/controllers/new_runner_pod_test.go index 84b4767b07..dc36822d9b 100644 --- a/controllers/new_runner_pod_test.go +++ b/controllers/new_runner_pod_test.go @@ -400,8 +400,7 @@ func TestNewRunnerPod(t *testing.T) { DockerEnabled: boolPtr(false), }, want: newTestPod(dockerDisabled, func(p *corev1.Pod) { - // TODO - // p.Spec.Containers[0].SecurityContext.Privileged = boolPtr(true) + p.Spec.Containers[0].SecurityContext.Privileged = boolPtr(true) }), }, } @@ -904,7 +903,7 @@ func TestNewRunnerPodFromRunnerController(t *testing.T) { }, want: newTestPod(dockerDisabled, func(p *corev1.Pod) { - // p.Spec.Containers[0].SecurityContext.Privileged = boolPtr(true) + p.Spec.Containers[0].SecurityContext.Privileged = boolPtr(true) }), }, } diff --git a/controllers/runner_controller.go b/controllers/runner_controller.go index 6cb9a9cbd1..a274079bb0 100644 --- a/controllers/runner_controller.go +++ b/controllers/runner_controller.go @@ -349,25 +349,52 @@ func (r *RunnerReconciler) newPod(runner v1alpha1.Runner) (corev1.Pod, error) { if len(runner.Spec.Containers) == 0 { template.Spec.Containers = append(template.Spec.Containers, corev1.Container{ - Name: "runner", - ImagePullPolicy: runner.Spec.ImagePullPolicy, - EnvFrom: runner.Spec.EnvFrom, - Env: runner.Spec.Env, - Resources: runner.Spec.Resources, + Name: "runner", }) if (runner.Spec.DockerEnabled == nil || *runner.Spec.DockerEnabled) && (runner.Spec.DockerdWithinRunnerContainer == nil || !*runner.Spec.DockerdWithinRunnerContainer) { template.Spec.Containers = append(template.Spec.Containers, corev1.Container{ - Name: "docker", - VolumeMounts: runner.Spec.DockerVolumeMounts, - Resources: runner.Spec.DockerdContainerResources, - Env: runner.Spec.DockerEnv, + Name: "docker", }) } } else { template.Spec.Containers = runner.Spec.Containers } + for i, c := range template.Spec.Containers { + switch c.Name { + case "runner": + if c.ImagePullPolicy == "" { + template.Spec.Containers[i].ImagePullPolicy = runner.Spec.ImagePullPolicy + } + if len(c.EnvFrom) == 0 { + template.Spec.Containers[i].EnvFrom = runner.Spec.EnvFrom + } + if len(c.Env) == 0 { + template.Spec.Containers[i].Env = runner.Spec.Env + } + if len(c.Resources.Requests) == 0 { + template.Spec.Containers[i].Resources.Requests = runner.Spec.Resources.Requests + } + if len(c.Resources.Limits) == 0 { + template.Spec.Containers[i].Resources.Limits = runner.Spec.Resources.Limits + } + case "docker": + if len(c.VolumeMounts) == 0 { + template.Spec.Containers[i].VolumeMounts = runner.Spec.DockerVolumeMounts + } + if len(c.Resources.Limits) == 0 { + template.Spec.Containers[i].Resources.Limits = runner.Spec.DockerdContainerResources.Limits + } + if len(c.Resources.Requests) == 0 { + template.Spec.Containers[i].Resources.Requests = runner.Spec.DockerdContainerResources.Requests + } + if len(c.Env) == 0 { + template.Spec.Containers[i].Env = runner.Spec.DockerEnv + } + } + } + template.Spec.SecurityContext = runner.Spec.SecurityContext template.Spec.EnableServiceLinks = runner.Spec.EnableServiceLinks @@ -624,8 +651,11 @@ func newRunnerPod(runnerName string, template corev1.Pod, runnerSpec v1alpha1.Ru if runnerContainer.SecurityContext == nil { runnerContainer.SecurityContext = &corev1.SecurityContext{} } - // Runner need to run privileged if it contains DinD - runnerContainer.SecurityContext.Privileged = &dockerdInRunnerPrivileged + + if runnerContainer.SecurityContext.Privileged == nil { + // Runner need to run privileged if it contains DinD + runnerContainer.SecurityContext.Privileged = &dockerdInRunnerPrivileged + } pod := template.DeepCopy()