Skip to content

Commit

Permalink
Fix docker.sock permission error for non-dind Ubuntu 20.04 runners si…
Browse files Browse the repository at this point in the history 
…nce v0.27.2 (actions#2499)

actions#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 actions#2490
  • Loading branch information
@mumoshu
mumoshu committed Apr 17, 2023
1 parent ba1ac09 commit 3e0bc3f
Show file tree
Hide file tree
Showing 5 changed files with 150 additions and 66 deletions.
6 changes: 4 additions & 2 deletions controllers/actions.summerwind.net/integration_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -105,12 +105,14 @@ func SetupIntegrationTest(ctx2 context.Context) *testEnvironment {
Log: logf.Log,
Recorder: mgr.GetEventRecorderFor("runnerreplicaset-controller"),
GitHubClient: multiClient,
RunnerImage: "example/runner:test",
DockerImage: "example/docker:test",
Name: controllerName("runner"),
RegistrationRecheckInterval: time.Millisecond * 100,
RegistrationRecheckJitter: time.Millisecond * 10,
UnregistrationRetryDelay: 1 * time.Second,
RunnerPodDefaults: RunnerPodDefaults{
RunnerImage: "example/runner:test",
DockerImage: "example/docker:test",
},
}
err = runnerController.SetupWithManager(mgr)
Expect(err).NotTo(HaveOccurred(), "failed to setup runner controller")
Expand Down
88 changes: 79 additions & 9 deletions controllers/actions.summerwind.net/new_runner_pod_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,21 @@ import (
"sigs.k8s.io/controller-runtime/pkg/client"
)

func newRunnerPod(template corev1.Pod, runnerSpec arcv1alpha1.RunnerConfig, githubBaseURL string, d RunnerPodDefaults) (corev1.Pod, error) {
return newRunnerPodWithContainerMode("", template, runnerSpec, githubBaseURL, d)
}

func setEnv(c *corev1.Container, name, value string) {
for j := range c.Env {
e := &c.Env[j]

if e.Name == name {
e.Value = value
return
}
}
}

func newWorkGenericEphemeralVolume(t *testing.T, storageReq string) corev1.Volume {
GBs, err := resource.ParseQuantity(storageReq)
if err != nil {
Expand Down Expand Up @@ -171,7 +186,7 @@ func TestNewRunnerPod(t *testing.T) {
Env: []corev1.EnvVar{
{
Name: "DOCKER_GROUP_GID",
Value: "121",
Value: "1234",
},
},
VolumeMounts: []corev1.VolumeMount{
Expand Down Expand Up @@ -397,6 +412,50 @@ func TestNewRunnerPod(t *testing.T) {
config: arcv1alpha1.RunnerConfig{},
want: newTestPod(base, nil),
},
{
description: "it should respect DOCKER_GROUP_GID of the dockerd sidecar container",
template: corev1.Pod{
Spec: corev1.PodSpec{
Containers: []corev1.Container{
{
Name: "docker",
Env: []corev1.EnvVar{
{
Name: "DOCKER_GROUP_GID",
Value: "2345",
},
},
},
},
},
},
config: arcv1alpha1.RunnerConfig{},
want: newTestPod(base, func(p *corev1.Pod) {
setEnv(&p.Spec.Containers[1], "DOCKER_GROUP_GID", "2345")
}),
},
{
description: "it should add DOCKER_GROUP_GID=1001 to the dockerd sidecar container for Ubuntu 20.04 runners",
template: corev1.Pod{},
config: arcv1alpha1.RunnerConfig{
Image: "ghcr.io/summerwind/actions-runner:ubuntu-20.04-20210726-1",
},
want: newTestPod(base, func(p *corev1.Pod) {
setEnv(&p.Spec.Containers[1], "DOCKER_GROUP_GID", "1001")
p.Spec.Containers[0].Image = "ghcr.io/summerwind/actions-runner:ubuntu-20.04-20210726-1"
}),
},
{
description: "it should add DOCKER_GROUP_GID=121 to the dockerd sidecar container for Ubuntu 22.04 runners",
template: corev1.Pod{},
config: arcv1alpha1.RunnerConfig{
Image: "ghcr.io/summerwind/actions-runner:ubuntu-22.04-20210726-1",
},
want: newTestPod(base, func(p *corev1.Pod) {
setEnv(&p.Spec.Containers[1], "DOCKER_GROUP_GID", "121")
p.Spec.Containers[0].Image = "ghcr.io/summerwind/actions-runner:ubuntu-22.04-20210726-1"
}),
},
{
description: "dockerdWithinRunnerContainer=true should set privileged=true and omit the dind sidecar container",
template: corev1.Pod{},
Expand Down Expand Up @@ -552,7 +611,14 @@ func TestNewRunnerPod(t *testing.T) {
for i := range testcases {
tc := testcases[i]
t.Run(tc.description, func(t *testing.T) {
got, err := newRunnerPod(tc.template, tc.config, defaultRunnerImage, defaultRunnerImagePullSecrets, defaultDockerImage, defaultDockerRegistryMirror, githubBaseURL, false)
got, err := newRunnerPod(tc.template, tc.config, githubBaseURL, RunnerPodDefaults{
RunnerImage: defaultRunnerImage,
RunnerImagePullSecrets: defaultRunnerImagePullSecrets,
DockerImage: defaultDockerImage,
DockerRegistryMirror: defaultDockerRegistryMirror,
DockerGID: "1234",
UseRunnerStatusUpdateHook: false,
})
require.NoError(t, err)
require.Equal(t, tc.want, got)
})
Expand Down Expand Up @@ -713,7 +779,7 @@ func TestNewRunnerPodFromRunnerController(t *testing.T) {
Env: []corev1.EnvVar{
{
Name: "DOCKER_GROUP_GID",
Value: "121",
Value: "1234",
},
},
VolumeMounts: []corev1.VolumeMount{
Expand Down Expand Up @@ -1171,6 +1237,7 @@ func TestNewRunnerPodFromRunnerController(t *testing.T) {
defaultRunnerImage = "default-runner-image"
defaultRunnerImagePullSecrets = []string{}
defaultDockerImage = "default-docker-image"
defaultDockerGID = "1234"
defaultDockerRegistryMirror = ""
githubBaseURL = "api.github.com"
)
Expand All @@ -1190,12 +1257,15 @@ func TestNewRunnerPodFromRunnerController(t *testing.T) {

t.Run(tc.description, func(t *testing.T) {
r := &RunnerReconciler{
RunnerImage: defaultRunnerImage,
RunnerImagePullSecrets: defaultRunnerImagePullSecrets,
DockerImage: defaultDockerImage,
DockerRegistryMirror: defaultDockerRegistryMirror,
GitHubClient: multiClient,
Scheme: scheme,
GitHubClient: multiClient,
Scheme: scheme,
RunnerPodDefaults: RunnerPodDefaults{
RunnerImage: defaultRunnerImage,
RunnerImagePullSecrets: defaultRunnerImagePullSecrets,
DockerImage: defaultDockerImage,
DockerRegistryMirror: defaultDockerRegistryMirror,
DockerGID: defaultDockerGID,
},
}
got, err := r.newPod(tc.runner)
require.NoError(t, err)
Expand Down
55 changes: 39 additions & 16 deletions controllers/actions.summerwind.net/runner_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -68,15 +68,24 @@ type RunnerReconciler struct {
Recorder record.EventRecorder
Scheme *runtime.Scheme
GitHubClient *MultiGitHubClient
RunnerImage string
RunnerImagePullSecrets []string
DockerImage string
DockerRegistryMirror string
Name string
RegistrationRecheckInterval time.Duration
RegistrationRecheckJitter time.Duration
UseRunnerStatusUpdateHook bool
UnregistrationRetryDelay time.Duration

RunnerPodDefaults RunnerPodDefaults
}

type RunnerPodDefaults struct {
RunnerImage string
RunnerImagePullSecrets []string
DockerImage string
DockerRegistryMirror string
// The default Docker group ID to use for the dockerd sidecar container.
// Ubuntu 20.04 runner images assumes 1001 and the 22.04 variant assumes 121 by default.
DockerGID string

UseRunnerStatusUpdateHook bool
}

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

ready := runnerPodReady(&pod)

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

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

rules := []rbacv1.PolicyRule{}

if r.UseRunnerStatusUpdateHook {
if r.RunnerPodDefaults.UseRunnerStatusUpdateHook {
rules = append(rules, []rbacv1.PolicyRule{
{
APIGroups: []string{"actions.summerwind.dev"},
Expand Down Expand Up @@ -583,7 +592,7 @@ func (r *RunnerReconciler) newPod(runner v1alpha1.Runner) (corev1.Pod, error) {
}
}

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

if runnerSpec.ServiceAccountName != "" {
pod.Spec.ServiceAccountName = runnerSpec.ServiceAccountName
} else if r.UseRunnerStatusUpdateHook || runner.Spec.ContainerMode == "kubernetes" {
} else if r.RunnerPodDefaults.UseRunnerStatusUpdateHook || runner.Spec.ContainerMode == "kubernetes" {
pod.Spec.ServiceAccountName = runner.ObjectMeta.Name
}

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

func newRunnerPodWithContainerMode(containerMode string, template corev1.Pod, runnerSpec v1alpha1.RunnerConfig, defaultRunnerImage string, defaultRunnerImagePullSecrets []string, defaultDockerImage, defaultDockerRegistryMirror string, githubBaseURL string, useRunnerStatusUpdateHook bool) (corev1.Pod, error) {
func newRunnerPodWithContainerMode(containerMode string, template corev1.Pod, runnerSpec v1alpha1.RunnerConfig, githubBaseURL string, d RunnerPodDefaults) (corev1.Pod, error) {
var (
privileged bool = true
dockerdInRunner bool = runnerSpec.DockerdWithinRunnerContainer != nil && *runnerSpec.DockerdWithinRunnerContainer
dockerEnabled bool = runnerSpec.DockerEnabled == nil || *runnerSpec.DockerEnabled
ephemeral bool = runnerSpec.Ephemeral == nil || *runnerSpec.Ephemeral
dockerdInRunnerPrivileged bool = dockerdInRunner

defaultRunnerImage = d.RunnerImage
defaultRunnerImagePullSecrets = d.RunnerImagePullSecrets
defaultDockerImage = d.DockerImage
defaultDockerRegistryMirror = d.DockerRegistryMirror
useRunnerStatusUpdateHook = d.UseRunnerStatusUpdateHook
)

if containerMode == "kubernetes" {
Expand Down Expand Up @@ -1013,10 +1028,22 @@ func newRunnerPodWithContainerMode(containerMode string, template corev1.Pod, ru
// for actions-runner-controller) so typically should not need to be
// overridden
if ok, _ := envVarPresent("DOCKER_GROUP_GID", dockerdContainer.Env); !ok {
gid := d.DockerGID
// We default to gid 121 for Ubuntu 22.04 images
// See below for more details
// - https://github.com/actions/actions-runner-controller/issues/2490#issuecomment-1501561923
// - https://github.com/actions/actions-runner-controller/blob/8869ad28bb5a1daaedefe0e988571fe1fb36addd/runner/actions-runner.ubuntu-20.04.dockerfile#L14
// - https://github.com/actions/actions-runner-controller/blob/8869ad28bb5a1daaedefe0e988571fe1fb36addd/runner/actions-runner.ubuntu-22.04.dockerfile#L12
if strings.Contains(runnerContainer.Image, "22.04") {
gid = "121"
} else if strings.Contains(runnerContainer.Image, "20.04") {
gid = "1001"
}

dockerdContainer.Env = append(dockerdContainer.Env,
corev1.EnvVar{
Name: "DOCKER_GROUP_GID",
Value: "121",
Value: gid,
})
}
dockerdContainer.Args = append(dockerdContainer.Args, "--group=$(DOCKER_GROUP_GID)")
Expand Down Expand Up @@ -1240,10 +1267,6 @@ func newRunnerPodWithContainerMode(containerMode string, template corev1.Pod, ru
return *pod, nil
}

func newRunnerPod(template corev1.Pod, runnerSpec v1alpha1.RunnerConfig, defaultRunnerImage string, defaultRunnerImagePullSecrets []string, defaultDockerImage, defaultDockerRegistryMirror string, githubBaseURL string, useRunnerStatusUpdateHookEphemeralRole bool) (corev1.Pod, error) {
return newRunnerPodWithContainerMode("", template, runnerSpec, defaultRunnerImage, defaultRunnerImagePullSecrets, defaultDockerImage, defaultDockerRegistryMirror, githubBaseURL, useRunnerStatusUpdateHookEphemeralRole)
}

func (r *RunnerReconciler) SetupWithManager(mgr ctrl.Manager) error {
name := "runner-controller"
if r.Name != "" {
Expand Down
13 changes: 5 additions & 8 deletions controllers/actions.summerwind.net/runnerset_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,13 +45,10 @@ type RunnerSetReconciler struct {
Recorder record.EventRecorder
Scheme *runtime.Scheme

CommonRunnerLabels []string
GitHubClient *MultiGitHubClient
RunnerImage string
RunnerImagePullSecrets []string
DockerImage string
DockerRegistryMirror string
UseRunnerStatusUpdateHook bool
CommonRunnerLabels []string
GitHubClient *MultiGitHubClient

RunnerPodDefaults RunnerPodDefaults
}

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

githubBaseURL := ghc.GithubBaseURL

pod, err := newRunnerPodWithContainerMode(runnerSet.Spec.RunnerConfig.ContainerMode, template, runnerSet.Spec.RunnerConfig, r.RunnerImage, r.RunnerImagePullSecrets, r.DockerImage, r.DockerRegistryMirror, githubBaseURL, r.UseRunnerStatusUpdateHook)
pod, err := newRunnerPodWithContainerMode(runnerSet.Spec.RunnerConfig.ContainerMode, template, runnerSet.Spec.RunnerConfig, githubBaseURL, r.RunnerPodDefaults)
if err != nil {
return nil, err
}
Expand Down
Loading

0 comments on commit 3e0bc3f

Please sign in to comment.