diff --git a/controllers/actions.github.com/autoscalinglistener_controller_test.go b/controllers/actions.github.com/autoscalinglistener_controller_test.go index 407dd12a..36150eb9 100644 --- a/controllers/actions.github.com/autoscalinglistener_controller_test.go +++ b/controllers/actions.github.com/autoscalinglistener_controller_test.go @@ -26,9 +26,8 @@ import ( ) const ( - autoscalingListenerTestTimeout = time.Second * 20 - autoscalingListenerTestInterval = time.Millisecond * 250 - autoscalingListenerTestGitHubToken = "gh_token" + autoscalingListenerTestTimeout = time.Second * 20 + autoscalingListenerTestInterval = time.Millisecond * 250 ) var _ = Describe("Test AutoScalingListener controller", func() { diff --git a/controllers/actions.github.com/autoscalingrunnerset_controller.go b/controllers/actions.github.com/autoscalingrunnerset_controller.go index 1a7a83b9..bcab0c01 100644 --- a/controllers/actions.github.com/autoscalingrunnerset_controller.go +++ b/controllers/actions.github.com/autoscalingrunnerset_controller.go @@ -48,7 +48,7 @@ const ( annotationKeyValuesHash = "actions.github.com/values-hash" autoscalingRunnerSetFinalizerName = "autoscalingrunnerset.actions.github.com/finalizer" - runnerScaleSetIdAnnotationKey = "runner-scale-set-id" + runnerScaleSetIDAnnotationKey = "runner-scale-set-id" ) type UpdateStrategy string @@ -180,14 +180,14 @@ func (r *AutoscalingRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl return ctrl.Result{}, nil } - scaleSetIdRaw, ok := autoscalingRunnerSet.Annotations[runnerScaleSetIdAnnotationKey] + scaleSetIDRaw, ok := autoscalingRunnerSet.Annotations[runnerScaleSetIDAnnotationKey] if !ok { // Need to create a new runner scale set on Actions service log.Info("Runner scale set id annotation does not exist. Creating a new runner scale set.") return r.createRunnerScaleSet(ctx, autoscalingRunnerSet, log) } - if id, err := strconv.Atoi(scaleSetIdRaw); err != nil || id <= 0 { + if id, err := strconv.Atoi(scaleSetIDRaw); err != nil || id <= 0 { log.Info("Runner scale set id annotation is not an id, or is <= 0. Creating a new runner scale set.") // something modified the scaleSetId. Try to create one return r.createRunnerScaleSet(ctx, autoscalingRunnerSet, log) @@ -403,7 +403,7 @@ func (r *AutoscalingRunnerSetReconciler) createRunnerScaleSet(ctx context.Contex return ctrl.Result{}, err } - runnerGroupId := 1 + runnerGroupID := 1 if len(autoscalingRunnerSet.Spec.RunnerGroup) > 0 { runnerGroup, err := actionsClient.GetRunnerGroupByName(ctx, autoscalingRunnerSet.Spec.RunnerGroup) if err != nil { @@ -411,14 +411,14 @@ func (r *AutoscalingRunnerSetReconciler) createRunnerScaleSet(ctx context.Contex return ctrl.Result{}, err } - runnerGroupId = int(runnerGroup.ID) + runnerGroupID = int(runnerGroup.ID) } - runnerScaleSet, err := actionsClient.GetRunnerScaleSet(ctx, runnerGroupId, autoscalingRunnerSet.Spec.RunnerScaleSetName) + runnerScaleSet, err := actionsClient.GetRunnerScaleSet(ctx, runnerGroupID, autoscalingRunnerSet.Spec.RunnerScaleSetName) if err != nil { logger.Error(err, "Failed to get runner scale set from Actions service", "runnerGroupId", - strconv.Itoa(runnerGroupId), + strconv.Itoa(runnerGroupID), "runnerScaleSetName", autoscalingRunnerSet.Spec.RunnerScaleSetName) return ctrl.Result{}, err @@ -429,7 +429,7 @@ func (r *AutoscalingRunnerSetReconciler) createRunnerScaleSet(ctx context.Contex ctx, &actions.RunnerScaleSet{ Name: autoscalingRunnerSet.Spec.RunnerScaleSetName, - RunnerGroupId: runnerGroupId, + RunnerGroupId: runnerGroupID, Labels: []actions.Label{ { Name: autoscalingRunnerSet.Spec.RunnerScaleSetName, @@ -466,7 +466,7 @@ func (r *AutoscalingRunnerSetReconciler) createRunnerScaleSet(ctx context.Contex logger.Info("Adding runner scale set ID, name and runner group name as an annotation and url labels") if err = patch(ctx, r.Client, autoscalingRunnerSet, func(obj *v1alpha1.AutoscalingRunnerSet) { obj.Annotations[AnnotationKeyGitHubRunnerScaleSetName] = runnerScaleSet.Name - obj.Annotations[runnerScaleSetIdAnnotationKey] = strconv.Itoa(runnerScaleSet.Id) + obj.Annotations[runnerScaleSetIDAnnotationKey] = strconv.Itoa(runnerScaleSet.Id) obj.Annotations[AnnotationKeyGitHubRunnerGroupName] = runnerScaleSet.RunnerGroupName if err := applyGitHubURLLabels(obj.Spec.GitHubConfigUrl, obj.Labels); err != nil { // should never happen logger.Error(err, "Failed to apply GitHub URL labels") @@ -484,7 +484,7 @@ func (r *AutoscalingRunnerSetReconciler) createRunnerScaleSet(ctx context.Contex } func (r *AutoscalingRunnerSetReconciler) updateRunnerScaleSetRunnerGroup(ctx context.Context, autoscalingRunnerSet *v1alpha1.AutoscalingRunnerSet, logger logr.Logger) (ctrl.Result, error) { - runnerScaleSetId, err := strconv.Atoi(autoscalingRunnerSet.Annotations[runnerScaleSetIdAnnotationKey]) + runnerScaleSetID, err := strconv.Atoi(autoscalingRunnerSet.Annotations[runnerScaleSetIDAnnotationKey]) if err != nil { logger.Error(err, "Failed to parse runner scale set ID") return ctrl.Result{}, err @@ -496,7 +496,7 @@ func (r *AutoscalingRunnerSetReconciler) updateRunnerScaleSetRunnerGroup(ctx con return ctrl.Result{}, err } - runnerGroupId := 1 + runnerGroupID := 1 if len(autoscalingRunnerSet.Spec.RunnerGroup) > 0 { runnerGroup, err := actionsClient.GetRunnerGroupByName(ctx, autoscalingRunnerSet.Spec.RunnerGroup) if err != nil { @@ -504,12 +504,12 @@ func (r *AutoscalingRunnerSetReconciler) updateRunnerScaleSetRunnerGroup(ctx con return ctrl.Result{}, err } - runnerGroupId = int(runnerGroup.ID) + runnerGroupID = int(runnerGroup.ID) } - updatedRunnerScaleSet, err := actionsClient.UpdateRunnerScaleSet(ctx, runnerScaleSetId, &actions.RunnerScaleSet{RunnerGroupId: runnerGroupId}) + updatedRunnerScaleSet, err := actionsClient.UpdateRunnerScaleSet(ctx, runnerScaleSetID, &actions.RunnerScaleSet{RunnerGroupId: runnerGroupID}) if err != nil { - logger.Error(err, "Failed to update runner scale set", "runnerScaleSetId", runnerScaleSetId) + logger.Error(err, "Failed to update runner scale set", "runnerScaleSetId", runnerScaleSetID) return ctrl.Result{}, err } @@ -527,7 +527,7 @@ func (r *AutoscalingRunnerSetReconciler) updateRunnerScaleSetRunnerGroup(ctx con } func (r *AutoscalingRunnerSetReconciler) updateRunnerScaleSetName(ctx context.Context, autoscalingRunnerSet *v1alpha1.AutoscalingRunnerSet, logger logr.Logger) (ctrl.Result, error) { - runnerScaleSetId, err := strconv.Atoi(autoscalingRunnerSet.Annotations[runnerScaleSetIdAnnotationKey]) + runnerScaleSetID, err := strconv.Atoi(autoscalingRunnerSet.Annotations[runnerScaleSetIDAnnotationKey]) if err != nil { logger.Error(err, "Failed to parse runner scale set ID") return ctrl.Result{}, err @@ -544,9 +544,9 @@ func (r *AutoscalingRunnerSetReconciler) updateRunnerScaleSetName(ctx context.Co return ctrl.Result{}, err } - updatedRunnerScaleSet, err := actionsClient.UpdateRunnerScaleSet(ctx, runnerScaleSetId, &actions.RunnerScaleSet{Name: autoscalingRunnerSet.Spec.RunnerScaleSetName}) + updatedRunnerScaleSet, err := actionsClient.UpdateRunnerScaleSet(ctx, runnerScaleSetID, &actions.RunnerScaleSet{Name: autoscalingRunnerSet.Spec.RunnerScaleSetName}) if err != nil { - logger.Error(err, "Failed to update runner scale set", "runnerScaleSetId", runnerScaleSetId) + logger.Error(err, "Failed to update runner scale set", "runnerScaleSetId", runnerScaleSetID) return ctrl.Result{}, err } @@ -563,7 +563,7 @@ func (r *AutoscalingRunnerSetReconciler) updateRunnerScaleSetName(ctx context.Co } func (r *AutoscalingRunnerSetReconciler) deleteRunnerScaleSet(ctx context.Context, autoscalingRunnerSet *v1alpha1.AutoscalingRunnerSet, logger logr.Logger) error { - scaleSetId, ok := autoscalingRunnerSet.Annotations[runnerScaleSetIdAnnotationKey] + scaleSetID, ok := autoscalingRunnerSet.Annotations[runnerScaleSetIDAnnotationKey] if !ok { // Annotation not being present can occur in 3 scenarios // 1. Scale set is never created. @@ -580,7 +580,7 @@ func (r *AutoscalingRunnerSetReconciler) deleteRunnerScaleSet(ctx context.Contex return nil } logger.Info("Deleting the runner scale set from Actions service") - runnerScaleSetId, err := strconv.Atoi(scaleSetId) + runnerScaleSetID, err := strconv.Atoi(scaleSetID) if err != nil { // If the annotation is not set correctly, 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 got to the point to create runner set. @@ -595,17 +595,17 @@ func (r *AutoscalingRunnerSetReconciler) deleteRunnerScaleSet(ctx context.Contex return err } - err = actionsClient.DeleteRunnerScaleSet(ctx, runnerScaleSetId) + err = actionsClient.DeleteRunnerScaleSet(ctx, runnerScaleSetID) if err != nil { - logger.Error(err, "Failed to delete runner scale set", "runnerScaleSetId", runnerScaleSetId) + logger.Error(err, "Failed to delete runner scale set", "runnerScaleSetId", runnerScaleSetID) return err } err = patch(ctx, r.Client, autoscalingRunnerSet, func(obj *v1alpha1.AutoscalingRunnerSet) { - delete(obj.Annotations, runnerScaleSetIdAnnotationKey) + delete(obj.Annotations, runnerScaleSetIDAnnotationKey) }) if err != nil { - logger.Error(err, "Failed to patch autoscaling runner set with annotation removed", "annotation", runnerScaleSetIdAnnotationKey) + logger.Error(err, "Failed to patch autoscaling runner set with annotation removed", "annotation", runnerScaleSetIDAnnotationKey) return err } @@ -1006,6 +1006,7 @@ func (c *autoscalingRunnerSetFinalizerDependencyCleaner) removeManagerRoleFinali // NOTE: if this is logic should be used for other resources, // consider using generics + type EphemeralRunnerSets struct { list *v1alpha1.EphemeralRunnerSetList sorted bool diff --git a/controllers/actions.github.com/autoscalingrunnerset_controller_test.go b/controllers/actions.github.com/autoscalingrunnerset_controller_test.go index 7411b6b9..620f3ab5 100644 --- a/controllers/actions.github.com/autoscalingrunnerset_controller_test.go +++ b/controllers/actions.github.com/autoscalingrunnerset_controller_test.go @@ -34,9 +34,8 @@ import ( ) const ( - autoscalingRunnerSetTestTimeout = time.Second * 20 - autoscalingRunnerSetTestInterval = time.Millisecond * 250 - autoscalingRunnerSetTestGitHubToken = "gh_token" + autoscalingRunnerSetTestTimeout = time.Second * 20 + autoscalingRunnerSetTestInterval = time.Millisecond * 250 ) var _ = Describe("Test AutoScalingRunnerSet controller", Ordered, func() { @@ -141,7 +140,7 @@ var _ = Describe("Test AutoScalingRunnerSet controller", Ordered, func() { return "", err } - if _, ok := created.Annotations[runnerScaleSetIdAnnotationKey]; !ok { + if _, ok := created.Annotations[runnerScaleSetIDAnnotationKey]; !ok { return "", nil } @@ -149,7 +148,7 @@ var _ = Describe("Test AutoScalingRunnerSet controller", Ordered, func() { return "", nil } - return fmt.Sprintf("%s_%s", created.Annotations[runnerScaleSetIdAnnotationKey], created.Annotations[AnnotationKeyGitHubRunnerGroupName]), nil + return fmt.Sprintf("%s_%s", created.Annotations[runnerScaleSetIDAnnotationKey], created.Annotations[AnnotationKeyGitHubRunnerGroupName]), nil }, autoscalingRunnerSetTestTimeout, autoscalingRunnerSetTestInterval).Should(BeEquivalentTo("1_testgroup"), "RunnerScaleSet should be created/fetched and update the AutoScalingRunnerSet's annotation") diff --git a/controllers/actions.github.com/constants.go b/controllers/actions.github.com/constants.go index cbf3309c..6619a504 100644 --- a/controllers/actions.github.com/constants.go +++ b/controllers/actions.github.com/constants.go @@ -36,7 +36,8 @@ const ( LabelKeyGitHubRepository = "actions.github.com/repository" ) -// Finalizer used to protect resources from deletion while AutoscalingRunnerSet is running +// AutoscalingRunnerSetCleanupFinalizerName is a finalizer used to protect resources +// from deletion while AutoscalingRunnerSet is running const AutoscalingRunnerSetCleanupFinalizerName = "actions.github.com/cleanup-protection" const ( diff --git a/controllers/actions.github.com/ephemeralrunner_controller.go b/controllers/actions.github.com/ephemeralrunner_controller.go index ec67b833..80a012be 100644 --- a/controllers/actions.github.com/ephemeralrunner_controller.go +++ b/controllers/actions.github.com/ephemeralrunner_controller.go @@ -689,7 +689,7 @@ func (r *EphemeralRunnerReconciler) createPod(ctx context.Context, runner *v1alp } log.Info("Creating new pod for ephemeral runner") - newPod := r.newEphemeralRunnerPod(ctx, runner, secret, envs...) + newPod := r.newEphemeralRunnerPod(runner, secret, envs...) if err := ctrl.SetControllerReference(runner, newPod, r.Scheme); err != nil { log.Error(err, "Failed to set controller reference to a new pod") diff --git a/controllers/actions.github.com/helpers_test.go b/controllers/actions.github.com/helpers_test.go index c3c0ef6b..f8f9e810 100644 --- a/controllers/actions.github.com/helpers_test.go +++ b/controllers/actions.github.com/helpers_test.go @@ -42,11 +42,11 @@ func createNamespace(t ginkgo.GinkgoTInterface, client client.Client) (*corev1.N ObjectMeta: metav1.ObjectMeta{Name: "testns-autoscaling" + RandStringRunes(5)}, } - err := k8sClient.Create(context.Background(), ns) + err := client.Create(context.Background(), ns) require.NoError(t, err) t.Cleanup(func() { - err := k8sClient.Delete(context.Background(), ns) + err := client.Delete(context.Background(), ns) require.NoError(t, err) }) diff --git a/controllers/actions.github.com/resourcebuilder.go b/controllers/actions.github.com/resourcebuilder.go index bf188d6d..98b894a6 100644 --- a/controllers/actions.github.com/resourcebuilder.go +++ b/controllers/actions.github.com/resourcebuilder.go @@ -2,7 +2,6 @@ package actionsgithubcom import ( "bytes" - "context" "encoding/json" "fmt" "maps" @@ -83,7 +82,7 @@ func boolPtr(v bool) *bool { } func (b *ResourceBuilder) newAutoScalingListener(autoscalingRunnerSet *v1alpha1.AutoscalingRunnerSet, ephemeralRunnerSet *v1alpha1.EphemeralRunnerSet, namespace, image string, imagePullSecrets []corev1.LocalObjectReference) (*v1alpha1.AutoscalingListener, error) { - runnerScaleSetId, err := strconv.Atoi(autoscalingRunnerSet.Annotations[runnerScaleSetIdAnnotationKey]) + runnerScaleSetID, err := strconv.Atoi(autoscalingRunnerSet.Annotations[runnerScaleSetIDAnnotationKey]) if err != nil { return nil, err } @@ -125,7 +124,7 @@ func (b *ResourceBuilder) newAutoScalingListener(autoscalingRunnerSet *v1alpha1. GitHubConfigUrl: autoscalingRunnerSet.Spec.GitHubConfigUrl, GitHubConfigSecret: autoscalingRunnerSet.Spec.GitHubConfigSecret, VaultConfig: autoscalingRunnerSet.VaultConfig(), - RunnerScaleSetId: runnerScaleSetId, + RunnerScaleSetId: runnerScaleSetID, AutoscalingRunnerSetNamespace: autoscalingRunnerSet.Namespace, AutoscalingRunnerSetName: autoscalingRunnerSet.Name, EphemeralRunnerSetName: ephemeralRunnerSet.Name, @@ -496,7 +495,7 @@ func (b *ResourceBuilder) newScaleSetListenerRoleBinding(autoscalingListener *v1 } func (b *ResourceBuilder) newEphemeralRunnerSet(autoscalingRunnerSet *v1alpha1.AutoscalingRunnerSet) (*v1alpha1.EphemeralRunnerSet, error) { - runnerScaleSetId, err := strconv.Atoi(autoscalingRunnerSet.Annotations[runnerScaleSetIdAnnotationKey]) + runnerScaleSetID, err := strconv.Atoi(autoscalingRunnerSet.Annotations[runnerScaleSetIDAnnotationKey]) if err != nil { return nil, err } @@ -541,7 +540,7 @@ func (b *ResourceBuilder) newEphemeralRunnerSet(autoscalingRunnerSet *v1alpha1.A Spec: v1alpha1.EphemeralRunnerSetSpec{ Replicas: 0, EphemeralRunnerSpec: v1alpha1.EphemeralRunnerSpec{ - RunnerScaleSetId: runnerScaleSetId, + RunnerScaleSetId: runnerScaleSetID, GitHubConfigUrl: autoscalingRunnerSet.Spec.GitHubConfigUrl, GitHubConfigSecret: autoscalingRunnerSet.Spec.GitHubConfigSecret, Proxy: autoscalingRunnerSet.Spec.Proxy, @@ -556,20 +555,12 @@ func (b *ResourceBuilder) newEphemeralRunnerSet(autoscalingRunnerSet *v1alpha1.A } func (b *ResourceBuilder) newEphemeralRunner(ephemeralRunnerSet *v1alpha1.EphemeralRunnerSet) *v1alpha1.EphemeralRunner { - labels := make(map[string]string) - for k, v := range ephemeralRunnerSet.Labels { - if k == LabelKeyKubernetesComponent { - labels[k] = "runner" - } else { - labels[k] = v - } - } - - annotations := make(map[string]string) - for key, val := range ephemeralRunnerSet.Annotations { - annotations[key] = val - } + labels := make(map[string]string, len(ephemeralRunnerSet.Labels)) + maps.Copy(labels, ephemeralRunnerSet.Labels) + labels[LabelKeyKubernetesComponent] = "runner" + annotations := make(map[string]string, len(ephemeralRunnerSet.Annotations)+1) + maps.Copy(annotations, ephemeralRunnerSet.Annotations) annotations[AnnotationKeyPatchID] = strconv.Itoa(ephemeralRunnerSet.Spec.PatchID) return &v1alpha1.EphemeralRunner{ ObjectMeta: metav1.ObjectMeta{ @@ -596,27 +587,17 @@ func (b *ResourceBuilder) newEphemeralRunner(ephemeralRunnerSet *v1alpha1.Epheme } } -func (b *ResourceBuilder) newEphemeralRunnerPod(ctx context.Context, runner *v1alpha1.EphemeralRunner, secret *corev1.Secret, envs ...corev1.EnvVar) *corev1.Pod { +func (b *ResourceBuilder) newEphemeralRunnerPod(runner *v1alpha1.EphemeralRunner, secret *corev1.Secret, envs ...corev1.EnvVar) *corev1.Pod { var newPod corev1.Pod - labels := map[string]string{} - annotations := map[string]string{} + annotations := make(map[string]string, len(runner.Annotations)+len(runner.Spec.Annotations)) + maps.Copy(annotations, runner.Annotations) + maps.Copy(annotations, runner.Spec.Annotations) - for k, v := range runner.Labels { - labels[k] = v - } - for k, v := range runner.Spec.Labels { - labels[k] = v - } + labels := make(map[string]string, len(runner.Labels)+len(runner.Spec.Labels)+2) + maps.Copy(labels, runner.Labels) + maps.Copy(labels, runner.Spec.Labels) labels["actions-ephemeral-runner"] = string(corev1.ConditionTrue) - - for k, v := range runner.Annotations { - annotations[k] = v - } - for k, v := range runner.Spec.Annotations { - annotations[k] = v - } - labels[LabelKeyPodTemplateHash] = hash.FNVHashStringObjects( FilterLabels(labels, LabelKeyRunnerTemplateHash), annotations, diff --git a/controllers/actions.github.com/resourcebuilder_test.go b/controllers/actions.github.com/resourcebuilder_test.go index 12e50533..b4c11466 100644 --- a/controllers/actions.github.com/resourcebuilder_test.go +++ b/controllers/actions.github.com/resourcebuilder_test.go @@ -1,7 +1,6 @@ package actionsgithubcom import ( - "context" "fmt" "strings" "testing" @@ -28,7 +27,7 @@ func TestLabelPropagation(t *testing.T) { "directly.excluded.org/arbitrary": "not-excluded-value", }, Annotations: map[string]string{ - runnerScaleSetIdAnnotationKey: "1", + runnerScaleSetIDAnnotationKey: "1", AnnotationKeyGitHubRunnerGroupName: "test-group", AnnotationKeyGitHubRunnerScaleSetName: "test-scale-set", }, @@ -104,7 +103,7 @@ func TestLabelPropagation(t *testing.T) { Name: "test", }, } - pod := b.newEphemeralRunnerPod(context.TODO(), ephemeralRunner, runnerSecret) + pod := b.newEphemeralRunnerPod(ephemeralRunner, runnerSecret) for key := range ephemeralRunner.Labels { assert.Equal(t, ephemeralRunner.Labels[key], pod.Labels[key]) } @@ -124,7 +123,7 @@ func TestGitHubURLTrimLabelValues(t *testing.T) { LabelKeyKubernetesVersion: "0.2.0", }, Annotations: map[string]string{ - runnerScaleSetIdAnnotationKey: "1", + runnerScaleSetIDAnnotationKey: "1", AnnotationKeyGitHubRunnerGroupName: "test-group", AnnotationKeyGitHubRunnerScaleSetName: "test-scale-set", }, @@ -190,7 +189,7 @@ func TestOwnershipRelationships(t *testing.T) { LabelKeyKubernetesVersion: "0.2.0", }, Annotations: map[string]string{ - runnerScaleSetIdAnnotationKey: "1", + runnerScaleSetIDAnnotationKey: "1", AnnotationKeyGitHubRunnerGroupName: "test-group", AnnotationKeyGitHubRunnerScaleSetName: "test-scale-set", annotationKeyValuesHash: "test-hash", @@ -233,7 +232,7 @@ func TestOwnershipRelationships(t *testing.T) { Name: "test-secret", }, } - pod := b.newEphemeralRunnerPod(context.TODO(), ephemeralRunner, runnerSecret) + pod := b.newEphemeralRunnerPod(ephemeralRunner, runnerSecret) // Test EphemeralRunnerPod ownership require.Len(t, pod.OwnerReferences, 1, "EphemeralRunnerPod should have exactly one owner reference")