Skip to content

Refactor resource naming removing unnecessary calculations #4076

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
nikola-jokic merged 3 commits into from
May 15, 2025
Merged

Refactor resource naming removing unnecessary calculations #4076

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ba93864
change listener name hash to include config url and runner group
nikola-jokic May 7, 2025
259a4ff
Merge branch 'master' into nikola-jokic/remove-mirror-secret-change-l…
nikola-jokic May 7, 2025
828c401
revert the naming
nikola-jokic May 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 11 additions & 11 deletions controllers/actions.github.com/autoscalinglistener_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand All @@ -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
}

Expand All @@ -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
}

Expand All @@ -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
}

Expand Down Expand Up @@ -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() {
Expand All @@ -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() {
Expand All @@ -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() {
Expand Down
99 changes: 9 additions & 90 deletions controllers/actions.github.com/autoscalinglistener_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand All @@ -178,15 +166,15 @@ 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
}

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)
Expand Down Expand Up @@ -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,
Expand All @@ -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,
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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")
})
})
})

Expand Down
32 changes: 4 additions & 28 deletions controllers/actions.github.com/resourcebuilder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Copy link

Copilot AI May 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing the resource name to autoscalingListener.Name may risk collisions if autoscalingListener.Name does not already include the necessary config URL and runner group information. Consider ensuring that the name is properly derived or unique across clusters.

Copilot uses AI. Check for mistakes.
Namespace: autoscalingListener.Namespace,
Labels: b.mergeLabels(autoscalingListener.Labels, map[string]string{
LabelKeyGitHubScaleSetNamespace: autoscalingListener.Spec.AutoscalingRunnerSetNamespace,
Expand All @@ -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,
Expand Down Expand Up @@ -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,
Expand All @@ -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,
Expand Down Expand Up @@ -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 {
Expand Down
Loading