From 79ff8945d351d545a840281ef20f5a2946b40f7e Mon Sep 17 00:00:00 2001 From: Nikola Jokic Date: Thu, 24 Sep 2026 00:01:00 +0200 Subject: [PATCH] Create runner pod before recording identity (#4681) --- .../ephemeralrunner_controller.go | 32 +-- .../ephemeralrunner_creation_order_test.go | 183 ++++++++++++++++++ 2 files changed, 204 insertions(+), 11 deletions(-) create mode 100644 controllers/actions.github.com/ephemeralrunner_creation_order_test.go diff --git a/controllers/actions.github.com/ephemeralrunner_controller.go b/controllers/actions.github.com/ephemeralrunner_controller.go index 8c5fb738..15a42f8d 100644 --- a/controllers/actions.github.com/ephemeralrunner_controller.go +++ b/controllers/actions.github.com/ephemeralrunner_controller.go @@ -305,8 +305,11 @@ func (r *EphemeralRunnerReconciler) Reconcile(ctx context.Context, req ctrl.Requ } } + var ( + initialRunnerID int + initialRunnerName string + ) if ephemeralRunner.Status.RunnerID == 0 { - log.Info("Updating ephemeral runner status with runnerId and runnerName") runnerID, err := strconv.Atoi(string(secret.Data["runnerId"])) if err != nil { log.Error(err, "Runner config secret is corrupted: missing runnerId") @@ -317,16 +320,8 @@ func (r *EphemeralRunnerReconciler) Reconcile(ctx context.Context, req ctrl.Requ log.Info("Corrupted runner config secret has been deleted") return ctrl.Result{RequeueAfter: 500 * time.Millisecond}, nil } - - runnerName := string(secret.Data["runnerName"]) - original := ephemeralRunner.DeepCopy() - ephemeralRunner.Status.RunnerID = runnerID - ephemeralRunner.Status.RunnerName = runnerName - - if err := r.Status().Patch(ctx, &ephemeralRunner, client.MergeFrom(original)); err != nil { - return ctrl.Result{}, fmt.Errorf("failed to update runner status for RunnerId/RunnerName: %w", err) - } - log.Info("Updated ephemeral runner status with runnerId and runnerName") + initialRunnerID = runnerID + initialRunnerName = string(secret.Data["runnerName"]) } if len(ephemeralRunner.Status.Failures) > maxFailures { @@ -415,6 +410,21 @@ func (r *EphemeralRunnerReconciler) Reconcile(ctx context.Context, req ctrl.Requ } } + // Validation above keeps malformed JIT secrets from reaching a Pod. The Pod + // only needs the valid secret, so publish the registration identity after + // the Pod exists. A retry can recover both fields from that secret. + if ephemeralRunner.Status.RunnerID == 0 { + log.Info("Updating ephemeral runner status with runnerId and runnerName") + original := ephemeralRunner.DeepCopy() + ephemeralRunner.Status.RunnerID = initialRunnerID + ephemeralRunner.Status.RunnerName = initialRunnerName + + if err := r.Status().Patch(ctx, &ephemeralRunner, client.MergeFrom(original)); err != nil { + return ctrl.Result{}, fmt.Errorf("failed to update runner status for RunnerId/RunnerName: %w", err) + } + log.Info("Updated ephemeral runner status with runnerId and runnerName") + } + cs := runnerContainerStatus(pod) switch { case pod.Status.Phase == corev1.PodFailed: // All containers are stopped diff --git a/controllers/actions.github.com/ephemeralrunner_creation_order_test.go b/controllers/actions.github.com/ephemeralrunner_creation_order_test.go new file mode 100644 index 00000000..338a664e --- /dev/null +++ b/controllers/actions.github.com/ephemeralrunner_creation_order_test.go @@ -0,0 +1,183 @@ +/* +Copyright 2020 The actions-runner-controller authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package actionsgithubcom + +import ( + "context" + "errors" + "testing" + "time" + + "github.com/actions/actions-runner-controller/apis/actions.github.com/v1alpha1" + "github.com/go-logr/logr" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + kerrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + ctrlfake "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" +) + +func TestReconcileDefersRunnerIdentityUntilPodExists(t *testing.T) { + ctx := context.Background() + key := types.NamespacedName{Namespace: "default", Name: "test-runner"} + + scheme := runtime.NewScheme() + require.NoError(t, corev1.AddToScheme(scheme)) + require.NoError(t, v1alpha1.AddToScheme(scheme)) + + runner := &v1alpha1.EphemeralRunner{ + ObjectMeta: metav1.ObjectMeta{ + Name: key.Name, + Namespace: key.Namespace, + Finalizers: []string{ephemeralRunnerFinalizerName, ephemeralRunnerActionsFinalizerName}, + }, + Spec: v1alpha1.EphemeralRunnerSpec{ + PodTemplateSpec: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{{Name: v1alpha1.EphemeralRunnerContainerName}}, + }, + }, + }, + } + secret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: key.Name, Namespace: key.Namespace}, + Data: map[string][]byte{ + jitTokenKey: []byte("jit-token"), + "runnerId": []byte("7"), + "runnerName": []byte("test-runner"), + }, + } + + failStatusPatch := true + statusPatchAttempts := 0 + c := ctrlfake.NewClientBuilder(). + WithScheme(scheme). + WithObjects(runner, secret). + WithStatusSubresource(&v1alpha1.EphemeralRunner{}). + WithInterceptorFuncs(interceptor.Funcs{ + SubResourcePatch: func(ctx context.Context, clt client.Client, subResourceName string, obj client.Object, patch client.Patch, opts ...client.SubResourcePatchOption) error { + if _, ok := obj.(*v1alpha1.EphemeralRunner); ok { + statusPatchAttempts++ + if failStatusPatch { + failStatusPatch = false + return errors.New("transient status patch failure") + } + } + return clt.Status().Patch(ctx, obj, patch, opts...) + }, + }). + Build() + + newReconciler := func() *EphemeralRunnerReconciler { + return &EphemeralRunnerReconciler{ + Client: c, + Log: logr.Discard(), + Scheme: scheme, + ResourceBuilder: ResourceBuilder{ + Scheme: scheme, + }, + } + } + podCount := func() int { + t.Helper() + + var pods corev1.PodList + require.NoError(t, c.List(ctx, &pods, client.InNamespace(key.Namespace))) + return len(pods.Items) + } + getRunner := func() *v1alpha1.EphemeralRunner { + t.Helper() + + got := new(v1alpha1.EphemeralRunner) + require.NoError(t, c.Get(ctx, key, got)) + return got + } + + // The first pass creates the Pod but deliberately leaves status unwritten. + _, err := newReconciler().Reconcile(ctx, ctrl.Request{NamespacedName: key}) + require.NoError(t, err) + assert.Equal(t, 1, podCount()) + assert.Zero(t, getRunner().Status.RunnerID) + assert.Empty(t, getRunner().Status.RunnerName) + assert.Zero(t, statusPatchAttempts, "the runner identity must not delay Pod creation") + + // This is the same state after a controller crash following Pod creation: + // the next reconcile finds the Pod and restores the identity from the JIT + // secret. A transient patch failure returns an error for reconciliation to + // retry without creating another Pod. + _, err = newReconciler().Reconcile(ctx, ctrl.Request{NamespacedName: key}) + require.ErrorContains(t, err, "failed to update runner status for RunnerId/RunnerName") + assert.Equal(t, 1, podCount()) + assert.Zero(t, getRunner().Status.RunnerID) + assert.Empty(t, getRunner().Status.RunnerName) + + _, err = newReconciler().Reconcile(ctx, ctrl.Request{NamespacedName: key}) + require.NoError(t, err) + assert.Equal(t, 1, podCount()) + assert.Equal(t, 2, statusPatchAttempts) + assert.Equal(t, 7, getRunner().Status.RunnerID) + assert.Equal(t, "test-runner", getRunner().Status.RunnerName) +} + +func TestReconcileRejectsMalformedJITSecretBeforeCreatingPod(t *testing.T) { + ctx := context.Background() + key := types.NamespacedName{Namespace: "default", Name: "test-runner"} + + scheme := runtime.NewScheme() + require.NoError(t, corev1.AddToScheme(scheme)) + require.NoError(t, v1alpha1.AddToScheme(scheme)) + + runner := newExampleRunner(key.Name, key.Namespace, "github-config-secret") + runner.Finalizers = []string{ephemeralRunnerFinalizerName, ephemeralRunnerActionsFinalizerName} + secret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: key.Name, Namespace: key.Namespace}, + Data: map[string][]byte{ + jitTokenKey: []byte("jit-token"), + "runnerName": []byte(key.Name), + }, + } + c := ctrlfake.NewClientBuilder(). + WithScheme(scheme). + WithObjects(runner, secret). + WithStatusSubresource(&v1alpha1.EphemeralRunner{}). + Build() + + reconciler := &EphemeralRunnerReconciler{ + Client: c, + Log: logr.Discard(), + Scheme: scheme, + ResourceBuilder: ResourceBuilder{ + Scheme: scheme, + }, + } + + result, err := reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: key}) + require.NoError(t, err) + assert.Equal(t, 500*time.Millisecond, result.RequeueAfter) + + err = c.Get(ctx, key, new(corev1.Pod)) + assert.True(t, kerrors.IsNotFound(err), "a malformed JIT Secret must never produce a Pod") + + err = c.Get(ctx, key, new(corev1.Secret)) + assert.True(t, kerrors.IsNotFound(err), "the malformed JIT Secret must be deleted") +}