From c4297d25bb9f85a59d47e1b7b6ce134a557ca10a Mon Sep 17 00:00:00 2001 From: Nikola Jokic Date: Fri, 3 Feb 2023 17:27:31 +0100 Subject: [PATCH] Avoid deleting scale set if annotation is not parsable or if it does not exist (#2239) --- .../autoscalingrunnerset_controller.go | 7 +- .../autoscalingrunnerset_controller_test.go | 190 +++++++++++++++--- github/actions/fake/client.go | 7 + 3 files changed, 179 insertions(+), 25 deletions(-) diff --git a/controllers/actions.github.com/autoscalingrunnerset_controller.go b/controllers/actions.github.com/autoscalingrunnerset_controller.go index 391ca966..db0a4f9d 100644 --- a/controllers/actions.github.com/autoscalingrunnerset_controller.go +++ b/controllers/actions.github.com/autoscalingrunnerset_controller.go @@ -412,8 +412,11 @@ func (r *AutoscalingRunnerSetReconciler) deleteRunnerScaleSet(ctx context.Contex logger.Info("Deleting the runner scale set from Actions service") runnerScaleSetId, err := strconv.Atoi(autoscalingRunnerSet.Annotations[runnerScaleSetIdKey]) if err != nil { - logger.Error(err, "Failed to parse runner scale set ID") - return err + // If the annotation is not set correctly, or if it does not exist, we are going to get stuck in a loop trying to parse the scale set id. + // If the configuration is invalid (secret does not exist for example), we never get to the point to create runner set. But then, manual cleanup + // would get stuck finalizing the resource trying to parse annotation indefinitely + logger.Info("autoscaling runner set does not have annotation describing scale set id. Skip deletion", "err", err.Error()) + return nil } actionsClient, err := r.actionsClientFor(ctx, autoscalingRunnerSet) diff --git a/controllers/actions.github.com/autoscalingrunnerset_controller_test.go b/controllers/actions.github.com/autoscalingrunnerset_controller_test.go index 911b8c4d..65bebe8c 100644 --- a/controllers/actions.github.com/autoscalingrunnerset_controller_test.go +++ b/controllers/actions.github.com/autoscalingrunnerset_controller_test.go @@ -8,6 +8,7 @@ import ( corev1 "k8s.io/api/core/v1" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" logf "sigs.k8s.io/controller-runtime/pkg/log" . "github.com/onsi/ginkgo/v2" @@ -15,7 +16,7 @@ import ( "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - actionsv1alpha1 "github.com/actions/actions-runner-controller/apis/actions.github.com/v1alpha1" + "github.com/actions/actions-runner-controller/apis/actions.github.com/v1alpha1" "github.com/actions/actions-runner-controller/github/actions/fake" ) @@ -29,7 +30,7 @@ var _ = Describe("Test AutoScalingRunnerSet controller", func() { var ctx context.Context var cancel context.CancelFunc autoscalingNS := new(corev1.Namespace) - autoscalingRunnerSet := new(actionsv1alpha1.AutoscalingRunnerSet) + autoscalingRunnerSet := new(v1alpha1.AutoscalingRunnerSet) configSecret := new(corev1.Secret) BeforeEach(func() { @@ -73,12 +74,12 @@ var _ = Describe("Test AutoScalingRunnerSet controller", func() { min := 1 max := 10 - autoscalingRunnerSet = &actionsv1alpha1.AutoscalingRunnerSet{ + autoscalingRunnerSet = &v1alpha1.AutoscalingRunnerSet{ ObjectMeta: metav1.ObjectMeta{ Name: "test-asrs", Namespace: autoscalingNS.Name, }, - Spec: actionsv1alpha1.AutoscalingRunnerSetSpec{ + Spec: v1alpha1.AutoscalingRunnerSetSpec{ GitHubConfigUrl: "https://github.com/owner/repo", GitHubConfigSecret: configSecret.Name, MaxRunners: &max, @@ -118,7 +119,7 @@ var _ = Describe("Test AutoScalingRunnerSet controller", func() { Context("When creating a new AutoScalingRunnerSet", func() { It("It should create/add all required resources for a new AutoScalingRunnerSet (finalizer, runnerscaleset, ephemeralrunnerset, listener)", func() { // Check if finalizer is added - created := new(actionsv1alpha1.AutoscalingRunnerSet) + created := new(v1alpha1.AutoscalingRunnerSet) Eventually( func() (string, error) { err := k8sClient.Get(ctx, client.ObjectKey{Name: autoscalingRunnerSet.Name, Namespace: autoscalingRunnerSet.Namespace}, created) @@ -157,7 +158,7 @@ var _ = Describe("Test AutoScalingRunnerSet controller", func() { // Check if ephemeral runner set is created Eventually( func() (int, error) { - runnerSetList := new(actionsv1alpha1.EphemeralRunnerSetList) + runnerSetList := new(v1alpha1.EphemeralRunnerSetList) err := k8sClient.List(ctx, runnerSetList, client.InNamespace(autoscalingRunnerSet.Namespace)) if err != nil { return 0, err @@ -171,13 +172,13 @@ var _ = Describe("Test AutoScalingRunnerSet controller", func() { // Check if listener is created Eventually( func() error { - return k8sClient.Get(ctx, client.ObjectKey{Name: scaleSetListenerName(autoscalingRunnerSet), Namespace: autoscalingRunnerSet.Namespace}, new(actionsv1alpha1.AutoscalingListener)) + return k8sClient.Get(ctx, client.ObjectKey{Name: scaleSetListenerName(autoscalingRunnerSet), Namespace: autoscalingRunnerSet.Namespace}, new(v1alpha1.AutoscalingListener)) }, autoscalingRunnerSetTestTimeout, autoscalingRunnerSetTestInterval).Should(Succeed(), "Listener should be created") // Check if status is updated - runnerSetList := new(actionsv1alpha1.EphemeralRunnerSetList) + runnerSetList := new(v1alpha1.EphemeralRunnerSetList) err := k8sClient.List(ctx, runnerSetList, client.InNamespace(autoscalingRunnerSet.Namespace)) Expect(err).NotTo(HaveOccurred(), "failed to list EphemeralRunnerSet") Expect(len(runnerSetList.Items)).To(BeEquivalentTo(1), "Only one EphemeralRunnerSet should be created") @@ -189,7 +190,7 @@ var _ = Describe("Test AutoScalingRunnerSet controller", func() { Eventually( func() (int, error) { - updated := new(actionsv1alpha1.AutoscalingRunnerSet) + updated := new(v1alpha1.AutoscalingRunnerSet) err := k8sClient.Get(ctx, client.ObjectKey{Name: autoscalingRunnerSet.Name, Namespace: autoscalingRunnerSet.Namespace}, updated) if err != nil { return 0, fmt.Errorf("failed to get AutoScalingRunnerSet: %w", err) @@ -206,7 +207,7 @@ var _ = Describe("Test AutoScalingRunnerSet controller", func() { // Wait till the listener is created Eventually( func() error { - return k8sClient.Get(ctx, client.ObjectKey{Name: scaleSetListenerName(autoscalingRunnerSet), Namespace: autoscalingRunnerSet.Namespace}, new(actionsv1alpha1.AutoscalingListener)) + return k8sClient.Get(ctx, client.ObjectKey{Name: scaleSetListenerName(autoscalingRunnerSet), Namespace: autoscalingRunnerSet.Namespace}, new(v1alpha1.AutoscalingListener)) }, autoscalingRunnerSetTestTimeout, autoscalingRunnerSetTestInterval).Should(Succeed(), "Listener should be created") @@ -218,7 +219,7 @@ var _ = Describe("Test AutoScalingRunnerSet controller", func() { // Check if the listener is deleted Eventually( func() error { - err := k8sClient.Get(ctx, client.ObjectKey{Name: scaleSetListenerName(autoscalingRunnerSet), Namespace: autoscalingRunnerSet.Namespace}, new(actionsv1alpha1.AutoscalingListener)) + err := k8sClient.Get(ctx, client.ObjectKey{Name: scaleSetListenerName(autoscalingRunnerSet), Namespace: autoscalingRunnerSet.Namespace}, new(v1alpha1.AutoscalingListener)) if err != nil && errors.IsNotFound(err) { return nil } @@ -231,7 +232,7 @@ var _ = Describe("Test AutoScalingRunnerSet controller", func() { // Check if all the EphemeralRunnerSet is deleted Eventually( func() error { - runnerSetList := new(actionsv1alpha1.EphemeralRunnerSetList) + runnerSetList := new(v1alpha1.EphemeralRunnerSetList) err := k8sClient.List(ctx, runnerSetList, client.InNamespace(autoscalingRunnerSet.Namespace)) if err != nil { return err @@ -249,7 +250,7 @@ var _ = Describe("Test AutoScalingRunnerSet controller", func() { // Check if the AutoScalingRunnerSet is deleted Eventually( func() error { - err := k8sClient.Get(ctx, client.ObjectKey{Name: autoscalingRunnerSet.Name, Namespace: autoscalingRunnerSet.Namespace}, new(actionsv1alpha1.AutoscalingRunnerSet)) + err := k8sClient.Get(ctx, client.ObjectKey{Name: autoscalingRunnerSet.Name, Namespace: autoscalingRunnerSet.Namespace}, new(v1alpha1.AutoscalingRunnerSet)) if err != nil && errors.IsNotFound(err) { return nil } @@ -264,7 +265,7 @@ var _ = Describe("Test AutoScalingRunnerSet controller", func() { Context("When updating a new AutoScalingRunnerSet", func() { It("It should re-create EphemeralRunnerSet and Listener as needed when updating AutoScalingRunnerSet", func() { // Wait till the listener is created - listener := new(actionsv1alpha1.AutoscalingListener) + listener := new(v1alpha1.AutoscalingListener) Eventually( func() error { return k8sClient.Get(ctx, client.ObjectKey{Name: scaleSetListenerName(autoscalingRunnerSet), Namespace: autoscalingRunnerSet.Namespace}, listener) @@ -272,7 +273,7 @@ var _ = Describe("Test AutoScalingRunnerSet controller", func() { autoscalingRunnerSetTestTimeout, autoscalingRunnerSetTestInterval).Should(Succeed(), "Listener should be created") - runnerSetList := new(actionsv1alpha1.EphemeralRunnerSetList) + runnerSetList := new(v1alpha1.EphemeralRunnerSetList) err := k8sClient.List(ctx, runnerSetList, client.InNamespace(autoscalingRunnerSet.Namespace)) Expect(err).NotTo(HaveOccurred(), "failed to list EphemeralRunnerSet") Expect(len(runnerSetList.Items)).To(Equal(1), "There should be 1 EphemeralRunnerSet") @@ -289,7 +290,7 @@ var _ = Describe("Test AutoScalingRunnerSet controller", func() { // We should create a new EphemeralRunnerSet and delete the old one, eventually, we will have only one EphemeralRunnerSet Eventually( func() (string, error) { - runnerSetList := new(actionsv1alpha1.EphemeralRunnerSetList) + runnerSetList := new(v1alpha1.EphemeralRunnerSetList) err := k8sClient.List(ctx, runnerSetList, client.InNamespace(autoscalingRunnerSet.Namespace)) if err != nil { return "", err @@ -307,7 +308,7 @@ var _ = Describe("Test AutoScalingRunnerSet controller", func() { // We should create a new listener Eventually( func() (string, error) { - listener := new(actionsv1alpha1.AutoscalingListener) + listener := new(v1alpha1.AutoscalingListener) err := k8sClient.Get(ctx, client.ObjectKey{Name: scaleSetListenerName(autoscalingRunnerSet), Namespace: autoscalingRunnerSet.Namespace}, listener) if err != nil { return "", err @@ -320,13 +321,13 @@ var _ = Describe("Test AutoScalingRunnerSet controller", func() { // Only update the Spec for the AutoScalingListener // This should trigger re-creation of the Listener only - runnerSetList = new(actionsv1alpha1.EphemeralRunnerSetList) + runnerSetList = new(v1alpha1.EphemeralRunnerSetList) err = k8sClient.List(ctx, runnerSetList, client.InNamespace(autoscalingRunnerSet.Namespace)) Expect(err).NotTo(HaveOccurred(), "failed to list EphemeralRunnerSet") Expect(len(runnerSetList.Items)).To(Equal(1), "There should be 1 EphemeralRunnerSet") runnerSet = runnerSetList.Items[0] - listener = new(actionsv1alpha1.AutoscalingListener) + listener = new(v1alpha1.AutoscalingListener) err = k8sClient.Get(ctx, client.ObjectKey{Name: scaleSetListenerName(autoscalingRunnerSet), Namespace: autoscalingRunnerSet.Namespace}, listener) Expect(err).NotTo(HaveOccurred(), "failed to get Listener") @@ -339,7 +340,7 @@ var _ = Describe("Test AutoScalingRunnerSet controller", func() { // We should not re-create a new EphemeralRunnerSet Consistently( func() (string, error) { - runnerSetList := new(actionsv1alpha1.EphemeralRunnerSetList) + runnerSetList := new(v1alpha1.EphemeralRunnerSetList) err := k8sClient.List(ctx, runnerSetList, client.InNamespace(autoscalingRunnerSet.Namespace)) if err != nil { return "", err @@ -357,7 +358,7 @@ var _ = Describe("Test AutoScalingRunnerSet controller", func() { // We should only re-create a new listener Eventually( func() (string, error) { - listener := new(actionsv1alpha1.AutoscalingListener) + listener := new(v1alpha1.AutoscalingListener) err := k8sClient.Get(ctx, client.ObjectKey{Name: scaleSetListenerName(autoscalingRunnerSet), Namespace: autoscalingRunnerSet.Namespace}, listener) if err != nil { return "", err @@ -370,11 +371,11 @@ var _ = Describe("Test AutoScalingRunnerSet controller", func() { }) It("It should update RunnerScaleSet's runner group on service when it changes", func() { - updated := new(actionsv1alpha1.AutoscalingRunnerSet) + updated := new(v1alpha1.AutoscalingRunnerSet) // Wait till the listener is created Eventually( func() error { - return k8sClient.Get(ctx, client.ObjectKey{Name: scaleSetListenerName(autoscalingRunnerSet), Namespace: autoscalingRunnerSet.Namespace}, new(actionsv1alpha1.AutoscalingListener)) + return k8sClient.Get(ctx, client.ObjectKey{Name: scaleSetListenerName(autoscalingRunnerSet), Namespace: autoscalingRunnerSet.Namespace}, new(v1alpha1.AutoscalingListener)) }, autoscalingRunnerSetTestTimeout, autoscalingRunnerSetTestInterval).Should(Succeed(), "Listener should be created") @@ -426,3 +427,146 @@ var _ = Describe("Test AutoScalingRunnerSet controller", func() { }) }) }) + +var _ = Describe("Test AutoscalingController creation failures", func() { + Context("When autoscaling runner set creation fails on the client", func() { + var ctx context.Context + var cancel context.CancelFunc + autoscalingNS := new(corev1.Namespace) + configSecret := new(corev1.Secret) + + BeforeEach(func() { + ctx, cancel = context.WithCancel(context.TODO()) + autoscalingNS = &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{Name: "testns-autoscaling" + RandStringRunes(5)}, + } + + err := k8sClient.Create(ctx, autoscalingNS) + Expect(err).NotTo(HaveOccurred(), "failed to create test namespace for AutoScalingRunnerSet") + + configSecret = &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "github-config-secret", + Namespace: autoscalingNS.Name, + }, + Data: map[string][]byte{ + "github_token": []byte(autoscalingRunnerSetTestGitHubToken), + }, + } + + err = k8sClient.Create(ctx, configSecret) + Expect(err).NotTo(HaveOccurred(), "failed to create config secret") + + mgr, err := ctrl.NewManager(cfg, ctrl.Options{}) + Expect(err).NotTo(HaveOccurred(), "failed to create manager") + + controller := &AutoscalingRunnerSetReconciler{ + Client: mgr.GetClient(), + Scheme: mgr.GetScheme(), + Log: logf.Log, + ControllerNamespace: autoscalingNS.Name, + DefaultRunnerScaleSetListenerImage: "ghcr.io/actions/arc", + ActionsClient: fake.NewMultiClient(), + } + err = controller.SetupWithManager(mgr) + Expect(err).NotTo(HaveOccurred(), "failed to setup controller") + + go func() { + defer GinkgoRecover() + + err := mgr.Start(ctx) + Expect(err).NotTo(HaveOccurred(), "failed to start manager") + }() + }) + + AfterEach(func() { + defer cancel() + + err := k8sClient.Delete(ctx, autoscalingNS) + Expect(err).NotTo(HaveOccurred(), "failed to delete test namespace for AutoScalingRunnerSet") + }) + + It("It should be able to clean up if annotation related to scale set id does not exist", func() { + min := 1 + max := 10 + autoscalingRunnerSet := &v1alpha1.AutoscalingRunnerSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-asrs", + Namespace: autoscalingNS.Name, + }, + Spec: v1alpha1.AutoscalingRunnerSetSpec{ + GitHubConfigUrl: "https://github.com/owner/repo", + MaxRunners: &max, + MinRunners: &min, + RunnerGroup: "testgroup", + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + { + Name: "runner", + Image: "ghcr.io/actions/runner", + }, + }, + }, + }, + }, + } + + err := k8sClient.Create(ctx, autoscalingRunnerSet) + Expect(err).NotTo(HaveOccurred(), "failed to create AutoScalingRunnerSet") + + // wait for the finalizer to be added + ars := new(v1alpha1.AutoscalingRunnerSet) + Eventually( + func() (string, error) { + err := k8sClient.Get(ctx, client.ObjectKey{Name: autoscalingRunnerSet.Name, Namespace: autoscalingRunnerSet.Namespace}, ars) + if err != nil { + return "", err + } + if len(ars.Finalizers) == 0 { + return "", nil + } + return ars.Finalizers[0], nil + }, + autoscalingRunnerSetTestTimeout, + autoscalingRunnerSetTestInterval, + ).Should(BeEquivalentTo(autoscalingRunnerSetFinalizerName), "AutoScalingRunnerSet should have a finalizer") + + ars.ObjectMeta.Annotations = make(map[string]string) + err = k8sClient.Update(ctx, ars) + Expect(err).NotTo(HaveOccurred(), "Update autoscaling runner set without annotation should be successful") + + Eventually( + func() (bool, error) { + err := k8sClient.Get(ctx, client.ObjectKey{Name: autoscalingRunnerSet.Name, Namespace: autoscalingRunnerSet.Namespace}, ars) + if err != nil { + return false, err + } + return len(ars.ObjectMeta.Annotations) == 0, nil + }, + autoscalingRunnerSetTestTimeout, + autoscalingRunnerSetTestInterval, + ).Should(BeEquivalentTo(true), "Autoscaling runner set should be updated with empty annotations") + + err = k8sClient.Delete(ctx, ars) + Expect(err).NotTo(HaveOccurred(), "Delete autoscaling runner set should be successful") + + Eventually( + func() (bool, error) { + updated := new(v1alpha1.AutoscalingRunnerSet) + err := k8sClient.Get(ctx, client.ObjectKey{Name: autoscalingRunnerSet.Name, Namespace: autoscalingRunnerSet.Namespace}, updated) + if err == nil { + return false, nil + } + if !errors.IsNotFound(err) { + return false, err + } + + return !controllerutil.ContainsFinalizer(updated, autoscalingRunnerSetFinalizerName), nil + }, + autoscalingRunnerSetTestTimeout, + autoscalingRunnerSetTestInterval, + ).Should(BeEquivalentTo(true), "Finalizer and resource should eventually be deleted") + }) + }) +}) diff --git a/github/actions/fake/client.go b/github/actions/fake/client.go index 29321b84..5d7e22b7 100644 --- a/github/actions/fake/client.go +++ b/github/actions/fake/client.go @@ -31,6 +31,13 @@ func WithGetRunner(runner *actions.RunnerReference, err error) Option { } } +func WithCreateRunnerScaleSet(scaleSet *actions.RunnerScaleSet, err error) Option { + return func(f *FakeClient) { + f.createRunnerScaleSetResult.RunnerScaleSet = scaleSet + f.createRunnerScaleSetResult.err = err + } +} + var defaultRunnerScaleSet = &actions.RunnerScaleSet{ Id: 1, Name: "testset",