Skip to content

Commit 43f1cd0

Browse files
nikola-jokicnikola-jokic
authored
Refactor resource naming removing unnecessary calculations (#4076)
1 parent 389d842 commit 43f1cd0

File tree

3 files changed

+24
-129
lines changed

3 files changed

+24
-129
lines changed

‎controllers/actions.github.com/autoscalinglistener_controller.go‎

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -139,9 +139,9 @@ func (r *AutoscalingListenerReconciler) Reconcile(ctx context.Context, req ctrl.
139139

140140
// Create a mirror secret in the same namespace as the AutoscalingListener
141141
mirrorSecret := new(corev1.Secret)
142-
if err := r.Get(ctx, types.NamespacedName{Namespace: autoscalingListener.Namespace, Name: scaleSetListenerSecretMirrorName(autoscalingListener)}, mirrorSecret); err != nil {
142+
if err := r.Get(ctx, types.NamespacedName{Namespace: autoscalingListener.Namespace, Name: autoscalingListener.Name}, mirrorSecret); err != nil {
143143
if !kerrors.IsNotFound(err) {
144-
log.Error(err, "Unable to get listener secret mirror", "namespace", autoscalingListener.Namespace, "name", scaleSetListenerSecretMirrorName(autoscalingListener))
144+
log.Error(err, "Unable to get listener secret mirror", "namespace", autoscalingListener.Namespace, "name", autoscalingListener.Name)
145145
return ctrl.Result{}, err
146146
}
147147

@@ -160,9 +160,9 @@ func (r *AutoscalingListenerReconciler) Reconcile(ctx context.Context, req ctrl.
160160

161161
// Make sure the runner scale set listener service account is created for the listener pod in the controller namespace
162162
serviceAccount := new(corev1.ServiceAccount)
163-
if err := r.Get(ctx, types.NamespacedName{Namespace: autoscalingListener.Namespace, Name: scaleSetListenerServiceAccountName(autoscalingListener)}, serviceAccount); err != nil {
163+
if err := r.Get(ctx, types.NamespacedName{Namespace: autoscalingListener.Namespace, Name: autoscalingListener.Name}, serviceAccount); err != nil {
164164
if !kerrors.IsNotFound(err) {
165-
log.Error(err, "Unable to get listener service accounts", "namespace", autoscalingListener.Namespace, "name", scaleSetListenerServiceAccountName(autoscalingListener))
165+
log.Error(err, "Unable to get listener service accounts", "namespace", autoscalingListener.Namespace, "name", autoscalingListener.Name)
166166
return ctrl.Result{}, err
167167
}
168168

@@ -175,9 +175,9 @@ func (r *AutoscalingListenerReconciler) Reconcile(ctx context.Context, req ctrl.
175175

176176
// Make sure the runner scale set listener role is created in the AutoscalingRunnerSet namespace
177177
listenerRole := new(rbacv1.Role)
178-
if err := r.Get(ctx, types.NamespacedName{Namespace: autoscalingListener.Spec.AutoscalingRunnerSetNamespace, Name: scaleSetListenerRoleName(autoscalingListener)}, listenerRole); err != nil {
178+
if err := r.Get(ctx, types.NamespacedName{Namespace: autoscalingListener.Spec.AutoscalingRunnerSetNamespace, Name: autoscalingListener.Name}, listenerRole); err != nil {
179179
if !kerrors.IsNotFound(err) {
180-
log.Error(err, "Unable to get listener role", "namespace", autoscalingListener.Spec.AutoscalingRunnerSetNamespace, "name", scaleSetListenerRoleName(autoscalingListener))
180+
log.Error(err, "Unable to get listener role", "namespace", autoscalingListener.Spec.AutoscalingRunnerSetNamespace, "name", autoscalingListener.Name)
181181
return ctrl.Result{}, err
182182
}
183183

@@ -197,9 +197,9 @@ func (r *AutoscalingListenerReconciler) Reconcile(ctx context.Context, req ctrl.
197197

198198
// Make sure the runner scale set listener role binding is created
199199
listenerRoleBinding := new(rbacv1.RoleBinding)
200-
if err := r.Get(ctx, types.NamespacedName{Namespace: autoscalingListener.Spec.AutoscalingRunnerSetNamespace, Name: scaleSetListenerRoleName(autoscalingListener)}, listenerRoleBinding); err != nil {
200+
if err := r.Get(ctx, types.NamespacedName{Namespace: autoscalingListener.Spec.AutoscalingRunnerSetNamespace, Name: autoscalingListener.Name}, listenerRoleBinding); err != nil {
201201
if !kerrors.IsNotFound(err) {
202-
log.Error(err, "Unable to get listener role binding", "namespace", autoscalingListener.Spec.AutoscalingRunnerSetNamespace, "name", scaleSetListenerRoleName(autoscalingListener))
202+
log.Error(err, "Unable to get listener role binding", "namespace", autoscalingListener.Spec.AutoscalingRunnerSetNamespace, "name", autoscalingListener.Name)
203203
return ctrl.Result{}, err
204204
}
205205

@@ -330,7 +330,7 @@ func (r *AutoscalingListenerReconciler) cleanupResources(ctx context.Context, au
330330
}
331331

332332
listenerRoleBinding := new(rbacv1.RoleBinding)
333-
err = r.Get(ctx, types.NamespacedName{Namespace: autoscalingListener.Spec.AutoscalingRunnerSetNamespace, Name: scaleSetListenerRoleName(autoscalingListener)}, listenerRoleBinding)
333+
err = r.Get(ctx, types.NamespacedName{Namespace: autoscalingListener.Spec.AutoscalingRunnerSetNamespace, Name: autoscalingListener.Name}, listenerRoleBinding)
334334
switch {
335335
case err == nil:
336336
if listenerRoleBinding.DeletionTimestamp.IsZero() {
@@ -346,7 +346,7 @@ func (r *AutoscalingListenerReconciler) cleanupResources(ctx context.Context, au
346346
logger.Info("Listener role binding is deleted")
347347

348348
listenerRole := new(rbacv1.Role)
349-
err = r.Get(ctx, types.NamespacedName{Namespace: autoscalingListener.Spec.AutoscalingRunnerSetNamespace, Name: scaleSetListenerRoleName(autoscalingListener)}, listenerRole)
349+
err = r.Get(ctx, types.NamespacedName{Namespace: autoscalingListener.Spec.AutoscalingRunnerSetNamespace, Name: autoscalingListener.Name}, listenerRole)
350350
switch {
351351
case err == nil:
352352
if listenerRole.DeletionTimestamp.IsZero() {
@@ -363,7 +363,7 @@ func (r *AutoscalingListenerReconciler) cleanupResources(ctx context.Context, au
363363

364364
logger.Info("Cleaning up the listener service account")
365365
listenerSa := new(corev1.ServiceAccount)
366-
err = r.Get(ctx, types.NamespacedName{Name: scaleSetListenerServiceAccountName(autoscalingListener), Namespace: autoscalingListener.Namespace}, listenerSa)
366+
err = r.Get(ctx, types.NamespacedName{Name: autoscalingListener.Name, Namespace: autoscalingListener.Namespace}, listenerSa)
367367
switch {
368368
case err == nil:
369369
if listenerSa.DeletionTimestamp.IsZero() {

‎controllers/actions.github.com/autoscalinglistener_controller_test.go‎

Lines changed: 9 additions & 90 deletions
Original file line numberDiff line numberDiff line change
@@ -134,37 +134,25 @@ var _ = Describe("Test AutoScalingListener controller", func() {
134134
autoscalingListenerTestTimeout,
135135
autoscalingListenerTestInterval).Should(BeEquivalentTo(autoscalingListenerFinalizerName), "AutoScalingListener should have a finalizer")
136136

137-
// Check if secret is created
138-
mirrorSecret := new(corev1.Secret)
139-
Eventually(
140-
func() (string, error) {
141-
err := k8sClient.Get(ctx, client.ObjectKey{Name: scaleSetListenerSecretMirrorName(autoscalingListener), Namespace: autoscalingListener.Namespace}, mirrorSecret)
142-
if err != nil {
143-
return "", err
144-
}
145-
return string(mirrorSecret.Data["github_token"]), nil
146-
},
147-
autoscalingListenerTestTimeout,
148-
autoscalingListenerTestInterval).Should(BeEquivalentTo(autoscalingListenerTestGitHubToken), "Mirror secret should be created")
149-
150137
// Check if service account is created
151138
serviceAccount := new(corev1.ServiceAccount)
152139
Eventually(
153140
func() (string, error) {
154-
err := k8sClient.Get(ctx, client.ObjectKey{Name: scaleSetListenerServiceAccountName(autoscalingListener), Namespace: autoscalingListener.Namespace}, serviceAccount)
141+
err := k8sClient.Get(ctx, client.ObjectKey{Name: autoscalingListener.Name, Namespace: autoscalingListener.Namespace}, serviceAccount)
155142
if err != nil {
156143
return "", err
157144
}
158145
return serviceAccount.Name, nil
159146
},
160147
autoscalingListenerTestTimeout,
161-
autoscalingListenerTestInterval).Should(BeEquivalentTo(scaleSetListenerServiceAccountName(autoscalingListener)), "Service account should be created")
148+
autoscalingListenerTestInterval,
149+
).Should(BeEquivalentTo(autoscalingListener.Name), "Service account should be created")
162150

163151
// Check if role is created
164152
role := new(rbacv1.Role)
165153
Eventually(
166154
func() ([]rbacv1.PolicyRule, error) {
167-
err := k8sClient.Get(ctx, client.ObjectKey{Name: scaleSetListenerRoleName(autoscalingListener), Namespace: autoscalingListener.Spec.AutoscalingRunnerSetNamespace}, role)
155+
err := k8sClient.Get(ctx, client.ObjectKey{Name: autoscalingListener.Name, Namespace: autoscalingListener.Spec.AutoscalingRunnerSetNamespace}, role)
168156
if err != nil {
169157
return nil, err
170158
}
@@ -178,15 +166,15 @@ var _ = Describe("Test AutoScalingListener controller", func() {
178166
roleBinding := new(rbacv1.RoleBinding)
179167
Eventually(
180168
func() (string, error) {
181-
err := k8sClient.Get(ctx, client.ObjectKey{Name: scaleSetListenerRoleName(autoscalingListener), Namespace: autoscalingListener.Spec.AutoscalingRunnerSetNamespace}, roleBinding)
169+
err := k8sClient.Get(ctx, client.ObjectKey{Name: autoscalingListener.Name, Namespace: autoscalingListener.Spec.AutoscalingRunnerSetNamespace}, roleBinding)
182170
if err != nil {
183171
return "", err
184172
}
185173

186174
return roleBinding.RoleRef.Name, nil
187175
},
188176
autoscalingListenerTestTimeout,
189-
autoscalingListenerTestInterval).Should(BeEquivalentTo(scaleSetListenerRoleName(autoscalingListener)), "Rolebinding should be created")
177+
autoscalingListenerTestInterval).Should(BeEquivalentTo(autoscalingListener.Name), "Rolebinding should be created")
190178

191179
// Check if pod is created
192180
pod := new(corev1.Pod)
@@ -248,7 +236,7 @@ var _ = Describe("Test AutoScalingListener controller", func() {
248236
Eventually(
249237
func() bool {
250238
roleBinding := new(rbacv1.RoleBinding)
251-
err := k8sClient.Get(ctx, client.ObjectKey{Name: scaleSetListenerRoleName(autoscalingListener), Namespace: autoscalingListener.Spec.AutoscalingRunnerSetNamespace}, roleBinding)
239+
err := k8sClient.Get(ctx, client.ObjectKey{Name: autoscalingListener.Name, Namespace: autoscalingListener.Spec.AutoscalingRunnerSetNamespace}, roleBinding)
252240
return kerrors.IsNotFound(err)
253241
},
254242
autoscalingListenerTestTimeout,
@@ -259,7 +247,7 @@ var _ = Describe("Test AutoScalingListener controller", func() {
259247
Eventually(
260248
func() bool {
261249
role := new(rbacv1.Role)
262-
err := k8sClient.Get(ctx, client.ObjectKey{Name: scaleSetListenerRoleName(autoscalingListener), Namespace: autoscalingListener.Spec.AutoscalingRunnerSetNamespace}, role)
250+
err := k8sClient.Get(ctx, client.ObjectKey{Name: autoscalingListener.Name, Namespace: autoscalingListener.Spec.AutoscalingRunnerSetNamespace}, role)
263251
return kerrors.IsNotFound(err)
264252
},
265253
autoscalingListenerTestTimeout,
@@ -340,7 +328,7 @@ var _ = Describe("Test AutoScalingListener controller", func() {
340328
role := new(rbacv1.Role)
341329
Eventually(
342330
func() ([]rbacv1.PolicyRule, error) {
343-
err := k8sClient.Get(ctx, client.ObjectKey{Name: scaleSetListenerRoleName(autoscalingListener), Namespace: autoscalingListener.Spec.AutoscalingRunnerSetNamespace}, role)
331+
err := k8sClient.Get(ctx, client.ObjectKey{Name: autoscalingListener.Name, Namespace: autoscalingListener.Spec.AutoscalingRunnerSetNamespace}, role)
344332
if err != nil {
345333
return nil, err
346334
}
@@ -397,75 +385,6 @@ var _ = Describe("Test AutoScalingListener controller", func() {
397385
autoscalingListenerTestInterval,
398386
).ShouldNot(BeEquivalentTo(oldPodUID), "Pod should be re-created")
399387
})
400-
401-
It("It should update mirror secrets to match secret used by AutoScalingRunnerSet", func() {
402-
// Waiting for the pod is created
403-
pod := new(corev1.Pod)
404-
Eventually(
405-
func() (string, error) {
406-
err := k8sClient.Get(ctx, client.ObjectKey{Name: autoscalingListener.Name, Namespace: autoscalingListener.Namespace}, pod)
407-
if err != nil {
408-
return "", err
409-
}
410-
411-
return pod.Name, nil
412-
},
413-
autoscalingListenerTestTimeout,
414-
autoscalingListenerTestInterval).Should(BeEquivalentTo(autoscalingListener.Name), "Pod should be created")
415-
416-
// Update the secret
417-
updatedSecret := configSecret.DeepCopy()
418-
updatedSecret.Data["github_token"] = []byte(autoscalingListenerTestGitHubToken + "_updated")
419-
err := k8sClient.Update(ctx, updatedSecret)
420-
Expect(err).NotTo(HaveOccurred(), "failed to update test secret")
421-
422-
updatedPod := pod.DeepCopy()
423-
// Ignore status running and consult the container state
424-
updatedPod.Status.Phase = corev1.PodRunning
425-
updatedPod.Status.ContainerStatuses = []corev1.ContainerStatus{
426-
{
427-
Name: autoscalingListenerContainerName,
428-
State: corev1.ContainerState{
429-
Terminated: &corev1.ContainerStateTerminated{
430-
ExitCode: 1,
431-
},
432-
},
433-
},
434-
}
435-
err = k8sClient.Status().Update(ctx, updatedPod)
436-
Expect(err).NotTo(HaveOccurred(), "failed to update test pod to failed")
437-
438-
// Check if mirror secret is updated with right data
439-
mirrorSecret := new(corev1.Secret)
440-
Eventually(
441-
func() (map[string][]byte, error) {
442-
err := k8sClient.Get(ctx, client.ObjectKey{Name: scaleSetListenerSecretMirrorName(autoscalingListener), Namespace: autoscalingListener.Namespace}, mirrorSecret)
443-
if err != nil {
444-
return nil, err
445-
}
446-
447-
return mirrorSecret.Data, nil
448-
},
449-
autoscalingListenerTestTimeout,
450-
autoscalingListenerTestInterval).Should(BeEquivalentTo(updatedSecret.Data), "Mirror secret should be updated")
451-
452-
// Check if we re-created a new pod
453-
Eventually(
454-
func() error {
455-
latestPod := new(corev1.Pod)
456-
err := k8sClient.Get(ctx, client.ObjectKey{Name: autoscalingListener.Name, Namespace: autoscalingListener.Namespace}, latestPod)
457-
if err != nil {
458-
return err
459-
}
460-
if latestPod.UID == pod.UID {
461-
return fmt.Errorf("Pod should be recreated")
462-
}
463-
464-
return nil
465-
},
466-
autoscalingListenerTestTimeout,
467-
autoscalingListenerTestInterval).Should(Succeed(), "Pod should be recreated")
468-
})
469388
})
470389
})
471390

‎controllers/actions.github.com/resourcebuilder.go‎

Lines changed: 4 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -429,7 +429,7 @@ func mergeListenerContainer(base, from *corev1.Container) {
429429
func (b *ResourceBuilder) newScaleSetListenerServiceAccount(autoscalingListener *v1alpha1.AutoscalingListener) *corev1.ServiceAccount {
430430
return &corev1.ServiceAccount{
431431
ObjectMeta: metav1.ObjectMeta{
432-
Name: scaleSetListenerServiceAccountName(autoscalingListener),
432+
Name: autoscalingListener.Name,
433433
Namespace: autoscalingListener.Namespace,
434434
Labels: b.mergeLabels(autoscalingListener.Labels, map[string]string{
435435
LabelKeyGitHubScaleSetNamespace: autoscalingListener.Spec.AutoscalingRunnerSetNamespace,
@@ -444,7 +444,7 @@ func (b *ResourceBuilder) newScaleSetListenerRole(autoscalingListener *v1alpha1.
444444
rulesHash := hash.ComputeTemplateHash(&rules)
445445
newRole := &rbacv1.Role{
446446
ObjectMeta: metav1.ObjectMeta{
447-
Name: scaleSetListenerRoleName(autoscalingListener),
447+
Name: autoscalingListener.Name,
448448
Namespace: autoscalingListener.Spec.AutoscalingRunnerSetNamespace,
449449
Labels: b.mergeLabels(autoscalingListener.Labels, map[string]string{
450450
LabelKeyGitHubScaleSetNamespace: autoscalingListener.Spec.AutoscalingRunnerSetNamespace,
@@ -478,7 +478,7 @@ func (b *ResourceBuilder) newScaleSetListenerRoleBinding(autoscalingListener *v1
478478

479479
newRoleBinding := &rbacv1.RoleBinding{
480480
ObjectMeta: metav1.ObjectMeta{
481-
Name: scaleSetListenerRoleName(autoscalingListener),
481+
Name: autoscalingListener.Name,
482482
Namespace: autoscalingListener.Spec.AutoscalingRunnerSetNamespace,
483483
Labels: b.mergeLabels(autoscalingListener.Labels, map[string]string{
484484
LabelKeyGitHubScaleSetNamespace: autoscalingListener.Spec.AutoscalingRunnerSetNamespace,
@@ -501,7 +501,7 @@ func (b *ResourceBuilder) newScaleSetListenerSecretMirror(autoscalingListener *v
501501

502502
newListenerSecret := &corev1.Secret{
503503
ObjectMeta: metav1.ObjectMeta{
504-
Name: scaleSetListenerSecretMirrorName(autoscalingListener),
504+
Name: autoscalingListener.Name,
505505
Namespace: autoscalingListener.Namespace,
506506
Labels: b.mergeLabels(autoscalingListener.Labels, map[string]string{
507507
LabelKeyGitHubScaleSetNamespace: autoscalingListener.Spec.AutoscalingRunnerSetNamespace,
@@ -713,30 +713,6 @@ func scaleSetListenerName(autoscalingRunnerSet *v1alpha1.AutoscalingRunnerSet) s
713713
return fmt.Sprintf("%v-%v-listener", autoscalingRunnerSet.Name, namespaceHash)
714714
}
715715

716-
func scaleSetListenerServiceAccountName(autoscalingListener *v1alpha1.AutoscalingListener) string {
717-
namespaceHash := hash.FNVHashString(autoscalingListener.Spec.AutoscalingRunnerSetNamespace)
718-
if len(namespaceHash) > 8 {
719-
namespaceHash = namespaceHash[:8]
720-
}
721-
return fmt.Sprintf("%v-%v-listener", autoscalingListener.Spec.AutoscalingRunnerSetName, namespaceHash)
722-
}
723-
724-
func scaleSetListenerRoleName(autoscalingListener *v1alpha1.AutoscalingListener) string {
725-
namespaceHash := hash.FNVHashString(autoscalingListener.Spec.AutoscalingRunnerSetNamespace)
726-
if len(namespaceHash) > 8 {
727-
namespaceHash = namespaceHash[:8]
728-
}
729-
return fmt.Sprintf("%v-%v-listener", autoscalingListener.Spec.AutoscalingRunnerSetName, namespaceHash)
730-
}
731-
732-
func scaleSetListenerSecretMirrorName(autoscalingListener *v1alpha1.AutoscalingListener) string {
733-
namespaceHash := hash.FNVHashString(autoscalingListener.Spec.AutoscalingRunnerSetNamespace)
734-
if len(namespaceHash) > 8 {
735-
namespaceHash = namespaceHash[:8]
736-
}
737-
return fmt.Sprintf("%v-%v-listener", autoscalingListener.Spec.AutoscalingRunnerSetName, namespaceHash)
738-
}
739-
740716
func proxyListenerSecretName(autoscalingListener *v1alpha1.AutoscalingListener) string {
741717
namespaceHash := hash.FNVHashString(autoscalingListener.Spec.AutoscalingRunnerSetNamespace)
742718
if len(namespaceHash) > 8 {

0 commit comments

Comments
 (0)