Skip to content

Commit 3e0bc3f

Browse files
mumoshumumoshu
authored
Fix docker.sock permission error for non-dind Ubuntu 20.04 runners since v0.27.2 (#2499)
#2490 has been happening since v0.27.2 for non-dind runners based on Ubuntu 20.04 runner images. It does not affect Ubuntu 22.04 non-dind runners(i.e. runners with dockerd sidecars) and Ubuntu 20.04/22.04 dind runners(i.e. runners without dockerd sidecars). However, presuming many folks are still using Ubuntu 20.04 runners and non-dind runners, we should fix it. This change tries to fix it by defaulting to the docker group id 1001 used by Ubuntu 20.04 runners, and use gid 121 for Ubuntu 22.04 runners. We use the image tag to see which Ubuntu version the runner is based on. The algorithm is so simple- we assume it's Ubuntu-22.04-based if the image tag contains "22.04". This might be a breaking change for folks who have already upgraded to Ubuntu 22.04 runners using their own custom runner images. Note again; we rely on the image tag to detect Ubuntu 22.04 runner images and use the proper docker gid- Folks using our official Ubuntu 22.04 runner images are not affected. It is a breaking change anyway, so I have added a remedy- ARC got a new flag, `--docker-gid`, which defaults to `1001` but can be set to `121` or whatever gid the operator/admin likes. This can be set to `--docker-gid=121`, for example, if you are using your own custom runner image based on Ubuntu 22.04 and the image tag does not contain "22.04". Fixes #2490
1 parent ba1ac09 commit 3e0bc3f

File tree

5 files changed

+150
-66
lines changed

5 files changed

+150
-66
lines changed

‎controllers/actions.summerwind.net/integration_test.go‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -105,12 +105,14 @@ func SetupIntegrationTest(ctx2 context.Context) *testEnvironment {
105105
Log: logf.Log,
106106
Recorder: mgr.GetEventRecorderFor("runnerreplicaset-controller"),
107107
GitHubClient: multiClient,
108-
RunnerImage: "example/runner:test",
109-
DockerImage: "example/docker:test",
110108
Name: controllerName("runner"),
111109
RegistrationRecheckInterval: time.Millisecond * 100,
112110
RegistrationRecheckJitter: time.Millisecond * 10,
113111
UnregistrationRetryDelay: 1 * time.Second,
112+
RunnerPodDefaults: RunnerPodDefaults{
113+
RunnerImage: "example/runner:test",
114+
DockerImage: "example/docker:test",
115+
},
114116
}
115117
err = runnerController.SetupWithManager(mgr)
116118
Expect(err).NotTo(HaveOccurred(), "failed to setup runner controller")

‎controllers/actions.summerwind.net/new_runner_pod_test.go‎

Lines changed: 79 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,21 @@ import (
1515
"sigs.k8s.io/controller-runtime/pkg/client"
1616
)
1717

18+
func newRunnerPod(template corev1.Pod, runnerSpec arcv1alpha1.RunnerConfig, githubBaseURL string, d RunnerPodDefaults) (corev1.Pod, error) {
19+
return newRunnerPodWithContainerMode("", template, runnerSpec, githubBaseURL, d)
20+
}
21+
22+
func setEnv(c *corev1.Container, name, value string) {
23+
for j := range c.Env {
24+
e := &c.Env[j]
25+
26+
if e.Name == name {
27+
e.Value = value
28+
return
29+
}
30+
}
31+
}
32+
1833
func newWorkGenericEphemeralVolume(t *testing.T, storageReq string) corev1.Volume {
1934
GBs, err := resource.ParseQuantity(storageReq)
2035
if err != nil {
@@ -171,7 +186,7 @@ func TestNewRunnerPod(t *testing.T) {
171186
Env: []corev1.EnvVar{
172187
{
173188
Name: "DOCKER_GROUP_GID",
174-
Value: "121",
189+
Value: "1234",
175190
},
176191
},
177192
VolumeMounts: []corev1.VolumeMount{
@@ -397,6 +412,50 @@ func TestNewRunnerPod(t *testing.T) {
397412
config: arcv1alpha1.RunnerConfig{},
398413
want: newTestPod(base, nil),
399414
},
415+
{
416+
description: "it should respect DOCKER_GROUP_GID of the dockerd sidecar container",
417+
template: corev1.Pod{
418+
Spec: corev1.PodSpec{
419+
Containers: []corev1.Container{
420+
{
421+
Name: "docker",
422+
Env: []corev1.EnvVar{
423+
{
424+
Name: "DOCKER_GROUP_GID",
425+
Value: "2345",
426+
},
427+
},
428+
},
429+
},
430+
},
431+
},
432+
config: arcv1alpha1.RunnerConfig{},
433+
want: newTestPod(base, func(p *corev1.Pod) {
434+
setEnv(&p.Spec.Containers[1], "DOCKER_GROUP_GID", "2345")
435+
}),
436+
},
437+
{
438+
description: "it should add DOCKER_GROUP_GID=1001 to the dockerd sidecar container for Ubuntu 20.04 runners",
439+
template: corev1.Pod{},
440+
config: arcv1alpha1.RunnerConfig{
441+
Image: "ghcr.io/summerwind/actions-runner:ubuntu-20.04-20210726-1",
442+
},
443+
want: newTestPod(base, func(p *corev1.Pod) {
444+
setEnv(&p.Spec.Containers[1], "DOCKER_GROUP_GID", "1001")
445+
p.Spec.Containers[0].Image = "ghcr.io/summerwind/actions-runner:ubuntu-20.04-20210726-1"
446+
}),
447+
},
448+
{
449+
description: "it should add DOCKER_GROUP_GID=121 to the dockerd sidecar container for Ubuntu 22.04 runners",
450+
template: corev1.Pod{},
451+
config: arcv1alpha1.RunnerConfig{
452+
Image: "ghcr.io/summerwind/actions-runner:ubuntu-22.04-20210726-1",
453+
},
454+
want: newTestPod(base, func(p *corev1.Pod) {
455+
setEnv(&p.Spec.Containers[1], "DOCKER_GROUP_GID", "121")
456+
p.Spec.Containers[0].Image = "ghcr.io/summerwind/actions-runner:ubuntu-22.04-20210726-1"
457+
}),
458+
},
400459
{
401460
description: "dockerdWithinRunnerContainer=true should set privileged=true and omit the dind sidecar container",
402461
template: corev1.Pod{},
@@ -552,7 +611,14 @@ func TestNewRunnerPod(t *testing.T) {
552611
for i := range testcases {
553612
tc := testcases[i]
554613
t.Run(tc.description, func(t *testing.T) {
555-
got, err := newRunnerPod(tc.template, tc.config, defaultRunnerImage, defaultRunnerImagePullSecrets, defaultDockerImage, defaultDockerRegistryMirror, githubBaseURL, false)
614+
got, err := newRunnerPod(tc.template, tc.config, githubBaseURL, RunnerPodDefaults{
615+
RunnerImage: defaultRunnerImage,
616+
RunnerImagePullSecrets: defaultRunnerImagePullSecrets,
617+
DockerImage: defaultDockerImage,
618+
DockerRegistryMirror: defaultDockerRegistryMirror,
619+
DockerGID: "1234",
620+
UseRunnerStatusUpdateHook: false,
621+
})
556622
require.NoError(t, err)
557623
require.Equal(t, tc.want, got)
558624
})
@@ -713,7 +779,7 @@ func TestNewRunnerPodFromRunnerController(t *testing.T) {
713779
Env: []corev1.EnvVar{
714780
{
715781
Name: "DOCKER_GROUP_GID",
716-
Value: "121",
782+
Value: "1234",
717783
},
718784
},
719785
VolumeMounts: []corev1.VolumeMount{
@@ -1171,6 +1237,7 @@ func TestNewRunnerPodFromRunnerController(t *testing.T) {
11711237
defaultRunnerImage = "default-runner-image"
11721238
defaultRunnerImagePullSecrets = []string{}
11731239
defaultDockerImage = "default-docker-image"
1240+
defaultDockerGID = "1234"
11741241
defaultDockerRegistryMirror = ""
11751242
githubBaseURL = "api.github.com"
11761243
)
@@ -1190,12 +1257,15 @@ func TestNewRunnerPodFromRunnerController(t *testing.T) {
11901257

11911258
t.Run(tc.description, func(t *testing.T) {
11921259
r := &RunnerReconciler{
1193-
RunnerImage: defaultRunnerImage,
1194-
RunnerImagePullSecrets: defaultRunnerImagePullSecrets,
1195-
DockerImage: defaultDockerImage,
1196-
DockerRegistryMirror: defaultDockerRegistryMirror,
1197-
GitHubClient: multiClient,
1198-
Scheme: scheme,
1260+
GitHubClient: multiClient,
1261+
Scheme: scheme,
1262+
RunnerPodDefaults: RunnerPodDefaults{
1263+
RunnerImage: defaultRunnerImage,
1264+
RunnerImagePullSecrets: defaultRunnerImagePullSecrets,
1265+
DockerImage: defaultDockerImage,
1266+
DockerRegistryMirror: defaultDockerRegistryMirror,
1267+
DockerGID: defaultDockerGID,
1268+
},
11991269
}
12001270
got, err := r.newPod(tc.runner)
12011271
require.NoError(t, err)

‎controllers/actions.summerwind.net/runner_controller.go‎

Lines changed: 39 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -68,15 +68,24 @@ type RunnerReconciler struct {
6868
Recorder record.EventRecorder
6969
Scheme *runtime.Scheme
7070
GitHubClient *MultiGitHubClient
71-
RunnerImage string
72-
RunnerImagePullSecrets []string
73-
DockerImage string
74-
DockerRegistryMirror string
7571
Name string
7672
RegistrationRecheckInterval time.Duration
7773
RegistrationRecheckJitter time.Duration
78-
UseRunnerStatusUpdateHook bool
7974
UnregistrationRetryDelay time.Duration
75+
76+
RunnerPodDefaults RunnerPodDefaults
77+
}
78+
79+
type RunnerPodDefaults struct {
80+
RunnerImage string
81+
RunnerImagePullSecrets []string
82+
DockerImage string
83+
DockerRegistryMirror string
84+
// The default Docker group ID to use for the dockerd sidecar container.
85+
// Ubuntu 20.04 runner images assumes 1001 and the 22.04 variant assumes 121 by default.
86+
DockerGID string
87+
88+
UseRunnerStatusUpdateHook bool
8089
}
8190

8291
// +kubebuilder:rbac:groups=actions.summerwind.dev,resources=runners,verbs=get;list;watch;create;update;patch;delete
@@ -145,7 +154,7 @@ func (r *RunnerReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr
145154

146155
ready := runnerPodReady(&pod)
147156

148-
if (runner.Status.Phase != phase || runner.Status.Ready != ready) && !r.UseRunnerStatusUpdateHook || runner.Status.Phase == "" && r.UseRunnerStatusUpdateHook {
157+
if (runner.Status.Phase != phase || runner.Status.Ready != ready) && !r.RunnerPodDefaults.UseRunnerStatusUpdateHook || runner.Status.Phase == "" && r.RunnerPodDefaults.UseRunnerStatusUpdateHook {
149158
if pod.Status.Phase == corev1.PodRunning {
150159
// Seeing this message, you can expect the runner to become `Running` soon.
151160
log.V(1).Info(
@@ -292,7 +301,7 @@ func (r *RunnerReconciler) processRunnerCreation(ctx context.Context, runner v1a
292301
return ctrl.Result{}, err
293302
}
294303

295-
needsServiceAccount := runner.Spec.ServiceAccountName == "" && (r.UseRunnerStatusUpdateHook || runner.Spec.ContainerMode == "kubernetes")
304+
needsServiceAccount := runner.Spec.ServiceAccountName == "" && (r.RunnerPodDefaults.UseRunnerStatusUpdateHook || runner.Spec.ContainerMode == "kubernetes")
296305
if needsServiceAccount {
297306
serviceAccount := &corev1.ServiceAccount{
298307
ObjectMeta: metav1.ObjectMeta{
@@ -306,7 +315,7 @@ func (r *RunnerReconciler) processRunnerCreation(ctx context.Context, runner v1a
306315

307316
rules := []rbacv1.PolicyRule{}
308317

309-
if r.UseRunnerStatusUpdateHook {
318+
if r.RunnerPodDefaults.UseRunnerStatusUpdateHook {
310319
rules = append(rules, []rbacv1.PolicyRule{
311320
{
312321
APIGroups: []string{"actions.summerwind.dev"},
@@ -583,7 +592,7 @@ func (r *RunnerReconciler) newPod(runner v1alpha1.Runner) (corev1.Pod, error) {
583592
}
584593
}
585594

586-
pod, err := newRunnerPodWithContainerMode(runner.Spec.ContainerMode, template, runner.Spec.RunnerConfig, r.RunnerImage, r.RunnerImagePullSecrets, r.DockerImage, r.DockerRegistryMirror, ghc.GithubBaseURL, r.UseRunnerStatusUpdateHook)
595+
pod, err := newRunnerPodWithContainerMode(runner.Spec.ContainerMode, template, runner.Spec.RunnerConfig, ghc.GithubBaseURL, r.RunnerPodDefaults)
587596
if err != nil {
588597
return pod, err
589598
}
@@ -634,7 +643,7 @@ func (r *RunnerReconciler) newPod(runner v1alpha1.Runner) (corev1.Pod, error) {
634643

635644
if runnerSpec.ServiceAccountName != "" {
636645
pod.Spec.ServiceAccountName = runnerSpec.ServiceAccountName
637-
} else if r.UseRunnerStatusUpdateHook || runner.Spec.ContainerMode == "kubernetes" {
646+
} else if r.RunnerPodDefaults.UseRunnerStatusUpdateHook || runner.Spec.ContainerMode == "kubernetes" {
638647
pod.Spec.ServiceAccountName = runner.ObjectMeta.Name
639648
}
640649

@@ -754,13 +763,19 @@ func runnerHookEnvs(pod *corev1.Pod) ([]corev1.EnvVar, error) {
754763
}, nil
755764
}
756765

757-
func newRunnerPodWithContainerMode(containerMode string, template corev1.Pod, runnerSpec v1alpha1.RunnerConfig, defaultRunnerImage string, defaultRunnerImagePullSecrets []string, defaultDockerImage, defaultDockerRegistryMirror string, githubBaseURL string, useRunnerStatusUpdateHook bool) (corev1.Pod, error) {
766+
func newRunnerPodWithContainerMode(containerMode string, template corev1.Pod, runnerSpec v1alpha1.RunnerConfig, githubBaseURL string, d RunnerPodDefaults) (corev1.Pod, error) {
758767
var (
759768
privileged bool = true
760769
dockerdInRunner bool = runnerSpec.DockerdWithinRunnerContainer != nil && *runnerSpec.DockerdWithinRunnerContainer
761770
dockerEnabled bool = runnerSpec.DockerEnabled == nil || *runnerSpec.DockerEnabled
762771
ephemeral bool = runnerSpec.Ephemeral == nil || *runnerSpec.Ephemeral
763772
dockerdInRunnerPrivileged bool = dockerdInRunner
773+
774+
defaultRunnerImage = d.RunnerImage
775+
defaultRunnerImagePullSecrets = d.RunnerImagePullSecrets
776+
defaultDockerImage = d.DockerImage
777+
defaultDockerRegistryMirror = d.DockerRegistryMirror
778+
useRunnerStatusUpdateHook = d.UseRunnerStatusUpdateHook
764779
)
765780

766781
if containerMode == "kubernetes" {
@@ -1013,10 +1028,22 @@ func newRunnerPodWithContainerMode(containerMode string, template corev1.Pod, ru
10131028
// for actions-runner-controller) so typically should not need to be
10141029
// overridden
10151030
if ok, _ := envVarPresent("DOCKER_GROUP_GID", dockerdContainer.Env); !ok {
1031+
gid := d.DockerGID
1032+
// We default to gid 121 for Ubuntu 22.04 images
1033+
// See below for more details
1034+
// - https://github.com/actions/actions-runner-controller/issues/2490#issuecomment-1501561923
1035+
// - https://github.com/actions/actions-runner-controller/blob/8869ad28bb5a1daaedefe0e988571fe1fb36addd/runner/actions-runner.ubuntu-20.04.dockerfile#L14
1036+
// - https://github.com/actions/actions-runner-controller/blob/8869ad28bb5a1daaedefe0e988571fe1fb36addd/runner/actions-runner.ubuntu-22.04.dockerfile#L12
1037+
if strings.Contains(runnerContainer.Image, "22.04") {
1038+
gid = "121"
1039+
} else if strings.Contains(runnerContainer.Image, "20.04") {
1040+
gid = "1001"
1041+
}
1042+
10161043
dockerdContainer.Env = append(dockerdContainer.Env,
10171044
corev1.EnvVar{
10181045
Name: "DOCKER_GROUP_GID",
1019-
Value: "121",
1046+
Value: gid,
10201047
})
10211048
}
10221049
dockerdContainer.Args = append(dockerdContainer.Args, "--group=$(DOCKER_GROUP_GID)")
@@ -1240,10 +1267,6 @@ func newRunnerPodWithContainerMode(containerMode string, template corev1.Pod, ru
12401267
return *pod, nil
12411268
}
12421269

1243-
func newRunnerPod(template corev1.Pod, runnerSpec v1alpha1.RunnerConfig, defaultRunnerImage string, defaultRunnerImagePullSecrets []string, defaultDockerImage, defaultDockerRegistryMirror string, githubBaseURL string, useRunnerStatusUpdateHookEphemeralRole bool) (corev1.Pod, error) {
1244-
return newRunnerPodWithContainerMode("", template, runnerSpec, defaultRunnerImage, defaultRunnerImagePullSecrets, defaultDockerImage, defaultDockerRegistryMirror, githubBaseURL, useRunnerStatusUpdateHookEphemeralRole)
1245-
}
1246-
12471270
func (r *RunnerReconciler) SetupWithManager(mgr ctrl.Manager) error {
12481271
name := "runner-controller"
12491272
if r.Name != "" {

‎controllers/actions.summerwind.net/runnerset_controller.go‎

Lines changed: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -45,13 +45,10 @@ type RunnerSetReconciler struct {
4545
Recorder record.EventRecorder
4646
Scheme *runtime.Scheme
4747

48-
CommonRunnerLabels []string
49-
GitHubClient *MultiGitHubClient
50-
RunnerImage string
51-
RunnerImagePullSecrets []string
52-
DockerImage string
53-
DockerRegistryMirror string
54-
UseRunnerStatusUpdateHook bool
48+
CommonRunnerLabels []string
49+
GitHubClient *MultiGitHubClient
50+
51+
RunnerPodDefaults RunnerPodDefaults
5552
}
5653

5754
// +kubebuilder:rbac:groups=actions.summerwind.dev,resources=runnersets,verbs=get;list;watch;create;update;patch;delete
@@ -231,7 +228,7 @@ func (r *RunnerSetReconciler) newStatefulSet(ctx context.Context, runnerSet *v1a
231228

232229
githubBaseURL := ghc.GithubBaseURL
233230

234-
pod, err := newRunnerPodWithContainerMode(runnerSet.Spec.RunnerConfig.ContainerMode, template, runnerSet.Spec.RunnerConfig, r.RunnerImage, r.RunnerImagePullSecrets, r.DockerImage, r.DockerRegistryMirror, githubBaseURL, r.UseRunnerStatusUpdateHook)
231+
pod, err := newRunnerPodWithContainerMode(runnerSet.Spec.RunnerConfig.ContainerMode, template, runnerSet.Spec.RunnerConfig, githubBaseURL, r.RunnerPodDefaults)
235232
if err != nil {
236233
return nil, err
237234
}

0 commit comments

Comments
 (0)