From 43f1cd0dac0c221aad3362338266649cd8c230cb Mon Sep 17 00:00:00 2001 From: Nikola Jokic Date: Thu, 15 May 2025 10:56:34 +0200 Subject: [PATCH] Refactor resource naming removing unnecessary calculations (#4076) --- .../autoscalinglistener_controller.go | 22 ++--- .../autoscalinglistener_controller_test.go | 99 ++----------------- .../actions.github.com/resourcebuilder.go | 32 +----- 3 files changed, 24 insertions(+), 129 deletions(-) diff --git a/controllers/actions.github.com/autoscalinglistener_controller.go b/controllers/actions.github.com/autoscalinglistener_controller.go index 70b215e8..cecfdfbf 100644 --- a/controllers/actions.github.com/autoscalinglistener_controller.go +++ b/controllers/actions.github.com/autoscalinglistener_controller.go @@ -139,9 +139,9 @@ func (r *AutoscalingListenerReconciler) Reconcile(ctx context.Context, req ctrl. // Create a mirror secret in the same namespace as the AutoscalingListener mirrorSecret := new(corev1.Secret) - if err := r.Get(ctx, types.NamespacedName{Namespace: autoscalingListener.Namespace, Name: scaleSetListenerSecretMirrorName(autoscalingListener)}, mirrorSecret); err != nil { + if err := r.Get(ctx, types.NamespacedName{Namespace: autoscalingListener.Namespace, Name: autoscalingListener.Name}, mirrorSecret); err != nil { if !kerrors.IsNotFound(err) { - log.Error(err, "Unable to get listener secret mirror", "namespace", autoscalingListener.Namespace, "name", scaleSetListenerSecretMirrorName(autoscalingListener)) + log.Error(err, "Unable to get listener secret mirror", "namespace", autoscalingListener.Namespace, "name", autoscalingListener.Name) return ctrl.Result{}, err } @@ -160,9 +160,9 @@ func (r *AutoscalingListenerReconciler) Reconcile(ctx context.Context, req ctrl. // Make sure the runner scale set listener service account is created for the listener pod in the controller namespace serviceAccount := new(corev1.ServiceAccount) - if err := r.Get(ctx, types.NamespacedName{Namespace: autoscalingListener.Namespace, Name: scaleSetListenerServiceAccountName(autoscalingListener)}, serviceAccount); err != nil { + if err := r.Get(ctx, types.NamespacedName{Namespace: autoscalingListener.Namespace, Name: autoscalingListener.Name}, serviceAccount); err != nil { if !kerrors.IsNotFound(err) { - log.Error(err, "Unable to get listener service accounts", "namespace", autoscalingListener.Namespace, "name", scaleSetListenerServiceAccountName(autoscalingListener)) + log.Error(err, "Unable to get listener service accounts", "namespace", autoscalingListener.Namespace, "name", autoscalingListener.Name) return ctrl.Result{}, err } @@ -175,9 +175,9 @@ func (r *AutoscalingListenerReconciler) Reconcile(ctx context.Context, req ctrl. // Make sure the runner scale set listener role is created in the AutoscalingRunnerSet namespace listenerRole := new(rbacv1.Role) - if err := r.Get(ctx, types.NamespacedName{Namespace: autoscalingListener.Spec.AutoscalingRunnerSetNamespace, Name: scaleSetListenerRoleName(autoscalingListener)}, listenerRole); err != nil { + if err := r.Get(ctx, types.NamespacedName{Namespace: autoscalingListener.Spec.AutoscalingRunnerSetNamespace, Name: autoscalingListener.Name}, listenerRole); err != nil { if !kerrors.IsNotFound(err) { - log.Error(err, "Unable to get listener role", "namespace", autoscalingListener.Spec.AutoscalingRunnerSetNamespace, "name", scaleSetListenerRoleName(autoscalingListener)) + log.Error(err, "Unable to get listener role", "namespace", autoscalingListener.Spec.AutoscalingRunnerSetNamespace, "name", autoscalingListener.Name) return ctrl.Result{}, err } @@ -197,9 +197,9 @@ func (r *AutoscalingListenerReconciler) Reconcile(ctx context.Context, req ctrl. // Make sure the runner scale set listener role binding is created listenerRoleBinding := new(rbacv1.RoleBinding) - if err := r.Get(ctx, types.NamespacedName{Namespace: autoscalingListener.Spec.AutoscalingRunnerSetNamespace, Name: scaleSetListenerRoleName(autoscalingListener)}, listenerRoleBinding); err != nil { + if err := r.Get(ctx, types.NamespacedName{Namespace: autoscalingListener.Spec.AutoscalingRunnerSetNamespace, Name: autoscalingListener.Name}, listenerRoleBinding); err != nil { if !kerrors.IsNotFound(err) { - log.Error(err, "Unable to get listener role binding", "namespace", autoscalingListener.Spec.AutoscalingRunnerSetNamespace, "name", scaleSetListenerRoleName(autoscalingListener)) + log.Error(err, "Unable to get listener role binding", "namespace", autoscalingListener.Spec.AutoscalingRunnerSetNamespace, "name", autoscalingListener.Name) return ctrl.Result{}, err } @@ -330,7 +330,7 @@ func (r *AutoscalingListenerReconciler) cleanupResources(ctx context.Context, au } listenerRoleBinding := new(rbacv1.RoleBinding) - err = r.Get(ctx, types.NamespacedName{Namespace: autoscalingListener.Spec.AutoscalingRunnerSetNamespace, Name: scaleSetListenerRoleName(autoscalingListener)}, listenerRoleBinding) + err = r.Get(ctx, types.NamespacedName{Namespace: autoscalingListener.Spec.AutoscalingRunnerSetNamespace, Name: autoscalingListener.Name}, listenerRoleBinding) switch { case err == nil: if listenerRoleBinding.DeletionTimestamp.IsZero() { @@ -346,7 +346,7 @@ func (r *AutoscalingListenerReconciler) cleanupResources(ctx context.Context, au logger.Info("Listener role binding is deleted") listenerRole := new(rbacv1.Role) - err = r.Get(ctx, types.NamespacedName{Namespace: autoscalingListener.Spec.AutoscalingRunnerSetNamespace, Name: scaleSetListenerRoleName(autoscalingListener)}, listenerRole) + err = r.Get(ctx, types.NamespacedName{Namespace: autoscalingListener.Spec.AutoscalingRunnerSetNamespace, Name: autoscalingListener.Name}, listenerRole) switch { case err == nil: if listenerRole.DeletionTimestamp.IsZero() { @@ -363,7 +363,7 @@ func (r *AutoscalingListenerReconciler) cleanupResources(ctx context.Context, au logger.Info("Cleaning up the listener service account") listenerSa := new(corev1.ServiceAccount) - err = r.Get(ctx, types.NamespacedName{Name: scaleSetListenerServiceAccountName(autoscalingListener), Namespace: autoscalingListener.Namespace}, listenerSa) + err = r.Get(ctx, types.NamespacedName{Name: autoscalingListener.Name, Namespace: autoscalingListener.Namespace}, listenerSa) switch { case err == nil: if listenerSa.DeletionTimestamp.IsZero() { diff --git a/controllers/actions.github.com/autoscalinglistener_controller_test.go b/controllers/actions.github.com/autoscalinglistener_controller_test.go index 69b7978c..5ce5ee48 100644 --- a/controllers/actions.github.com/autoscalinglistener_controller_test.go +++ b/controllers/actions.github.com/autoscalinglistener_controller_test.go @@ -134,37 +134,25 @@ var _ = Describe("Test AutoScalingListener controller", func() { autoscalingListenerTestTimeout, autoscalingListenerTestInterval).Should(BeEquivalentTo(autoscalingListenerFinalizerName), "AutoScalingListener should have a finalizer") - // Check if secret is created - mirrorSecret := new(corev1.Secret) - Eventually( - func() (string, error) { - err := k8sClient.Get(ctx, client.ObjectKey{Name: scaleSetListenerSecretMirrorName(autoscalingListener), Namespace: autoscalingListener.Namespace}, mirrorSecret) - if err != nil { - return "", err - } - return string(mirrorSecret.Data["github_token"]), nil - }, - autoscalingListenerTestTimeout, - autoscalingListenerTestInterval).Should(BeEquivalentTo(autoscalingListenerTestGitHubToken), "Mirror secret should be created") - // Check if service account is created serviceAccount := new(corev1.ServiceAccount) Eventually( func() (string, error) { - err := k8sClient.Get(ctx, client.ObjectKey{Name: scaleSetListenerServiceAccountName(autoscalingListener), Namespace: autoscalingListener.Namespace}, serviceAccount) + err := k8sClient.Get(ctx, client.ObjectKey{Name: autoscalingListener.Name, Namespace: autoscalingListener.Namespace}, serviceAccount) if err != nil { return "", err } return serviceAccount.Name, nil }, autoscalingListenerTestTimeout, - autoscalingListenerTestInterval).Should(BeEquivalentTo(scaleSetListenerServiceAccountName(autoscalingListener)), "Service account should be created") + autoscalingListenerTestInterval, + ).Should(BeEquivalentTo(autoscalingListener.Name), "Service account should be created") // Check if role is created role := new(rbacv1.Role) Eventually( func() ([]rbacv1.PolicyRule, error) { - err := k8sClient.Get(ctx, client.ObjectKey{Name: scaleSetListenerRoleName(autoscalingListener), Namespace: autoscalingListener.Spec.AutoscalingRunnerSetNamespace}, role) + err := k8sClient.Get(ctx, client.ObjectKey{Name: autoscalingListener.Name, Namespace: autoscalingListener.Spec.AutoscalingRunnerSetNamespace}, role) if err != nil { return nil, err } @@ -178,7 +166,7 @@ var _ = Describe("Test AutoScalingListener controller", func() { roleBinding := new(rbacv1.RoleBinding) Eventually( func() (string, error) { - err := k8sClient.Get(ctx, client.ObjectKey{Name: scaleSetListenerRoleName(autoscalingListener), Namespace: autoscalingListener.Spec.AutoscalingRunnerSetNamespace}, roleBinding) + err := k8sClient.Get(ctx, client.ObjectKey{Name: autoscalingListener.Name, Namespace: autoscalingListener.Spec.AutoscalingRunnerSetNamespace}, roleBinding) if err != nil { return "", err } @@ -186,7 +174,7 @@ var _ = Describe("Test AutoScalingListener controller", func() { return roleBinding.RoleRef.Name, nil }, autoscalingListenerTestTimeout, - autoscalingListenerTestInterval).Should(BeEquivalentTo(scaleSetListenerRoleName(autoscalingListener)), "Rolebinding should be created") + autoscalingListenerTestInterval).Should(BeEquivalentTo(autoscalingListener.Name), "Rolebinding should be created") // Check if pod is created pod := new(corev1.Pod) @@ -248,7 +236,7 @@ var _ = Describe("Test AutoScalingListener controller", func() { Eventually( func() bool { roleBinding := new(rbacv1.RoleBinding) - err := k8sClient.Get(ctx, client.ObjectKey{Name: scaleSetListenerRoleName(autoscalingListener), Namespace: autoscalingListener.Spec.AutoscalingRunnerSetNamespace}, roleBinding) + err := k8sClient.Get(ctx, client.ObjectKey{Name: autoscalingListener.Name, Namespace: autoscalingListener.Spec.AutoscalingRunnerSetNamespace}, roleBinding) return kerrors.IsNotFound(err) }, autoscalingListenerTestTimeout, @@ -259,7 +247,7 @@ var _ = Describe("Test AutoScalingListener controller", func() { Eventually( func() bool { role := new(rbacv1.Role) - err := k8sClient.Get(ctx, client.ObjectKey{Name: scaleSetListenerRoleName(autoscalingListener), Namespace: autoscalingListener.Spec.AutoscalingRunnerSetNamespace}, role) + err := k8sClient.Get(ctx, client.ObjectKey{Name: autoscalingListener.Name, Namespace: autoscalingListener.Spec.AutoscalingRunnerSetNamespace}, role) return kerrors.IsNotFound(err) }, autoscalingListenerTestTimeout, @@ -340,7 +328,7 @@ var _ = Describe("Test AutoScalingListener controller", func() { role := new(rbacv1.Role) Eventually( func() ([]rbacv1.PolicyRule, error) { - err := k8sClient.Get(ctx, client.ObjectKey{Name: scaleSetListenerRoleName(autoscalingListener), Namespace: autoscalingListener.Spec.AutoscalingRunnerSetNamespace}, role) + err := k8sClient.Get(ctx, client.ObjectKey{Name: autoscalingListener.Name, Namespace: autoscalingListener.Spec.AutoscalingRunnerSetNamespace}, role) if err != nil { return nil, err } @@ -397,75 +385,6 @@ var _ = Describe("Test AutoScalingListener controller", func() { autoscalingListenerTestInterval, ).ShouldNot(BeEquivalentTo(oldPodUID), "Pod should be re-created") }) - - It("It should update mirror secrets to match secret used by AutoScalingRunnerSet", func() { - // Waiting for the pod is created - pod := new(corev1.Pod) - Eventually( - func() (string, error) { - err := k8sClient.Get(ctx, client.ObjectKey{Name: autoscalingListener.Name, Namespace: autoscalingListener.Namespace}, pod) - if err != nil { - return "", err - } - - return pod.Name, nil - }, - autoscalingListenerTestTimeout, - autoscalingListenerTestInterval).Should(BeEquivalentTo(autoscalingListener.Name), "Pod should be created") - - // Update the secret - updatedSecret := configSecret.DeepCopy() - updatedSecret.Data["github_token"] = []byte(autoscalingListenerTestGitHubToken + "_updated") - err := k8sClient.Update(ctx, updatedSecret) - Expect(err).NotTo(HaveOccurred(), "failed to update test secret") - - updatedPod := pod.DeepCopy() - // Ignore status running and consult the container state - updatedPod.Status.Phase = corev1.PodRunning - updatedPod.Status.ContainerStatuses = []corev1.ContainerStatus{ - { - Name: autoscalingListenerContainerName, - State: corev1.ContainerState{ - Terminated: &corev1.ContainerStateTerminated{ - ExitCode: 1, - }, - }, - }, - } - err = k8sClient.Status().Update(ctx, updatedPod) - Expect(err).NotTo(HaveOccurred(), "failed to update test pod to failed") - - // Check if mirror secret is updated with right data - mirrorSecret := new(corev1.Secret) - Eventually( - func() (map[string][]byte, error) { - err := k8sClient.Get(ctx, client.ObjectKey{Name: scaleSetListenerSecretMirrorName(autoscalingListener), Namespace: autoscalingListener.Namespace}, mirrorSecret) - if err != nil { - return nil, err - } - - return mirrorSecret.Data, nil - }, - autoscalingListenerTestTimeout, - autoscalingListenerTestInterval).Should(BeEquivalentTo(updatedSecret.Data), "Mirror secret should be updated") - - // Check if we re-created a new pod - Eventually( - func() error { - latestPod := new(corev1.Pod) - err := k8sClient.Get(ctx, client.ObjectKey{Name: autoscalingListener.Name, Namespace: autoscalingListener.Namespace}, latestPod) - if err != nil { - return err - } - if latestPod.UID == pod.UID { - return fmt.Errorf("Pod should be recreated") - } - - return nil - }, - autoscalingListenerTestTimeout, - autoscalingListenerTestInterval).Should(Succeed(), "Pod should be recreated") - }) }) }) diff --git a/controllers/actions.github.com/resourcebuilder.go b/controllers/actions.github.com/resourcebuilder.go index cd08b3a8..0726b82d 100644 --- a/controllers/actions.github.com/resourcebuilder.go +++ b/controllers/actions.github.com/resourcebuilder.go @@ -429,7 +429,7 @@ func mergeListenerContainer(base, from *corev1.Container) { func (b *ResourceBuilder) newScaleSetListenerServiceAccount(autoscalingListener *v1alpha1.AutoscalingListener) *corev1.ServiceAccount { return &corev1.ServiceAccount{ ObjectMeta: metav1.ObjectMeta{ - Name: scaleSetListenerServiceAccountName(autoscalingListener), + Name: autoscalingListener.Name, Namespace: autoscalingListener.Namespace, Labels: b.mergeLabels(autoscalingListener.Labels, map[string]string{ LabelKeyGitHubScaleSetNamespace: autoscalingListener.Spec.AutoscalingRunnerSetNamespace, @@ -444,7 +444,7 @@ func (b *ResourceBuilder) newScaleSetListenerRole(autoscalingListener *v1alpha1. rulesHash := hash.ComputeTemplateHash(&rules) newRole := &rbacv1.Role{ ObjectMeta: metav1.ObjectMeta{ - Name: scaleSetListenerRoleName(autoscalingListener), + Name: autoscalingListener.Name, Namespace: autoscalingListener.Spec.AutoscalingRunnerSetNamespace, Labels: b.mergeLabels(autoscalingListener.Labels, map[string]string{ LabelKeyGitHubScaleSetNamespace: autoscalingListener.Spec.AutoscalingRunnerSetNamespace, @@ -478,7 +478,7 @@ func (b *ResourceBuilder) newScaleSetListenerRoleBinding(autoscalingListener *v1 newRoleBinding := &rbacv1.RoleBinding{ ObjectMeta: metav1.ObjectMeta{ - Name: scaleSetListenerRoleName(autoscalingListener), + Name: autoscalingListener.Name, Namespace: autoscalingListener.Spec.AutoscalingRunnerSetNamespace, Labels: b.mergeLabels(autoscalingListener.Labels, map[string]string{ LabelKeyGitHubScaleSetNamespace: autoscalingListener.Spec.AutoscalingRunnerSetNamespace, @@ -501,7 +501,7 @@ func (b *ResourceBuilder) newScaleSetListenerSecretMirror(autoscalingListener *v newListenerSecret := &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ - Name: scaleSetListenerSecretMirrorName(autoscalingListener), + Name: autoscalingListener.Name, Namespace: autoscalingListener.Namespace, Labels: b.mergeLabels(autoscalingListener.Labels, map[string]string{ LabelKeyGitHubScaleSetNamespace: autoscalingListener.Spec.AutoscalingRunnerSetNamespace, @@ -713,30 +713,6 @@ func scaleSetListenerName(autoscalingRunnerSet *v1alpha1.AutoscalingRunnerSet) s return fmt.Sprintf("%v-%v-listener", autoscalingRunnerSet.Name, namespaceHash) } -func scaleSetListenerServiceAccountName(autoscalingListener *v1alpha1.AutoscalingListener) string { - namespaceHash := hash.FNVHashString(autoscalingListener.Spec.AutoscalingRunnerSetNamespace) - if len(namespaceHash) > 8 { - namespaceHash = namespaceHash[:8] - } - return fmt.Sprintf("%v-%v-listener", autoscalingListener.Spec.AutoscalingRunnerSetName, namespaceHash) -} - -func scaleSetListenerRoleName(autoscalingListener *v1alpha1.AutoscalingListener) string { - namespaceHash := hash.FNVHashString(autoscalingListener.Spec.AutoscalingRunnerSetNamespace) - if len(namespaceHash) > 8 { - namespaceHash = namespaceHash[:8] - } - return fmt.Sprintf("%v-%v-listener", autoscalingListener.Spec.AutoscalingRunnerSetName, namespaceHash) -} - -func scaleSetListenerSecretMirrorName(autoscalingListener *v1alpha1.AutoscalingListener) string { - namespaceHash := hash.FNVHashString(autoscalingListener.Spec.AutoscalingRunnerSetNamespace) - if len(namespaceHash) > 8 { - namespaceHash = namespaceHash[:8] - } - return fmt.Sprintf("%v-%v-listener", autoscalingListener.Spec.AutoscalingRunnerSetName, namespaceHash) -} - func proxyListenerSecretName(autoscalingListener *v1alpha1.AutoscalingListener) string { namespaceHash := hash.FNVHashString(autoscalingListener.Spec.AutoscalingRunnerSetNamespace) if len(namespaceHash) > 8 {