diff --git a/apis/actions.github.com/v1alpha1/autoscalingrunnerset_types.go b/apis/actions.github.com/v1alpha1/autoscalingrunnerset_types.go index 7435e88c..f268d874 100644 --- a/apis/actions.github.com/v1alpha1/autoscalingrunnerset_types.go +++ b/apis/actions.github.com/v1alpha1/autoscalingrunnerset_types.go @@ -330,20 +330,6 @@ const ( AutoscalingRunnerSetPhaseOutdated AutoscalingRunnerSetPhase = "Outdated" ) -func (ars *AutoscalingRunnerSet) Hash() string { - type data struct { - Spec *AutoscalingRunnerSetSpec - Labels map[string]string - } - - d := &data{ - Spec: ars.Spec.DeepCopy(), - Labels: ars.Labels, - } - - return hash.ComputeTemplateHash(d) -} - func (ars *AutoscalingRunnerSet) ListenerSpecHash() string { arsSpec := ars.Spec.DeepCopy() spec := arsSpec diff --git a/charts/gha-runner-scale-set-controller-experimental/crds/actions.github.com_autoscalinglisteners.yaml b/charts/gha-runner-scale-set-controller-experimental/crds/actions.github.com_autoscalinglisteners.yaml index 92652986..84e243f9 100644 --- a/charts/gha-runner-scale-set-controller-experimental/crds/actions.github.com_autoscalinglisteners.yaml +++ b/charts/gha-runner-scale-set-controller-experimental/crds/actions.github.com_autoscalinglisteners.yaml @@ -8802,16 +8802,6 @@ spec: It is used to identify which vault integration should be used to resolve secrets. type: string type: object - required: - - autoscalingRunnerSetName - - autoscalingRunnerSetNamespace - - ephemeralRunnerSetName - - githubConfigSecret - - githubConfigUrl - - image - - maxRunners - - minRunners - - runnerScaleSetId type: object status: description: AutoscalingListenerStatus defines the observed state of AutoscalingListener diff --git a/charts/gha-runner-scale-set-controller-experimental/crds/actions.github.com_autoscalingrunnersets.yaml b/charts/gha-runner-scale-set-controller-experimental/crds/actions.github.com_autoscalingrunnersets.yaml index 083c3d17..d236b1a9 100644 --- a/charts/gha-runner-scale-set-controller-experimental/crds/actions.github.com_autoscalingrunnersets.yaml +++ b/charts/gha-runner-scale-set-controller-experimental/crds/actions.github.com_autoscalingrunnersets.yaml @@ -16541,10 +16541,6 @@ spec: It is used to identify which vault integration should be used to resolve secrets. type: string type: object - required: - - githubConfigSecret - - githubConfigUrl - - template type: object status: description: AutoscalingRunnerSetStatus defines the observed state of AutoscalingRunnerSet diff --git a/charts/gha-runner-scale-set-controller/crds/actions.github.com_autoscalinglisteners.yaml b/charts/gha-runner-scale-set-controller/crds/actions.github.com_autoscalinglisteners.yaml index 92652986..84e243f9 100644 --- a/charts/gha-runner-scale-set-controller/crds/actions.github.com_autoscalinglisteners.yaml +++ b/charts/gha-runner-scale-set-controller/crds/actions.github.com_autoscalinglisteners.yaml @@ -8802,16 +8802,6 @@ spec: It is used to identify which vault integration should be used to resolve secrets. type: string type: object - required: - - autoscalingRunnerSetName - - autoscalingRunnerSetNamespace - - ephemeralRunnerSetName - - githubConfigSecret - - githubConfigUrl - - image - - maxRunners - - minRunners - - runnerScaleSetId type: object status: description: AutoscalingListenerStatus defines the observed state of AutoscalingListener diff --git a/charts/gha-runner-scale-set-controller/crds/actions.github.com_autoscalingrunnersets.yaml b/charts/gha-runner-scale-set-controller/crds/actions.github.com_autoscalingrunnersets.yaml index 083c3d17..d236b1a9 100644 --- a/charts/gha-runner-scale-set-controller/crds/actions.github.com_autoscalingrunnersets.yaml +++ b/charts/gha-runner-scale-set-controller/crds/actions.github.com_autoscalingrunnersets.yaml @@ -16541,10 +16541,6 @@ spec: It is used to identify which vault integration should be used to resolve secrets. type: string type: object - required: - - githubConfigSecret - - githubConfigUrl - - template type: object status: description: AutoscalingRunnerSetStatus defines the observed state of AutoscalingRunnerSet diff --git a/config/crd/bases/actions.github.com_autoscalinglisteners.yaml b/config/crd/bases/actions.github.com_autoscalinglisteners.yaml index 92652986..84e243f9 100644 --- a/config/crd/bases/actions.github.com_autoscalinglisteners.yaml +++ b/config/crd/bases/actions.github.com_autoscalinglisteners.yaml @@ -8802,16 +8802,6 @@ spec: It is used to identify which vault integration should be used to resolve secrets. type: string type: object - required: - - autoscalingRunnerSetName - - autoscalingRunnerSetNamespace - - ephemeralRunnerSetName - - githubConfigSecret - - githubConfigUrl - - image - - maxRunners - - minRunners - - runnerScaleSetId type: object status: description: AutoscalingListenerStatus defines the observed state of AutoscalingListener diff --git a/config/crd/bases/actions.github.com_autoscalingrunnersets.yaml b/config/crd/bases/actions.github.com_autoscalingrunnersets.yaml index 083c3d17..d236b1a9 100644 --- a/config/crd/bases/actions.github.com_autoscalingrunnersets.yaml +++ b/config/crd/bases/actions.github.com_autoscalingrunnersets.yaml @@ -16541,10 +16541,6 @@ spec: It is used to identify which vault integration should be used to resolve secrets. type: string type: object - required: - - githubConfigSecret - - githubConfigUrl - - template type: object status: description: AutoscalingRunnerSetStatus defines the observed state of AutoscalingRunnerSet diff --git a/controllers/actions.github.com/autoscalinglistener_controller.go b/controllers/actions.github.com/autoscalinglistener_controller.go index 3b61a3ba..431bdbdd 100644 --- a/controllers/actions.github.com/autoscalinglistener_controller.go +++ b/controllers/actions.github.com/autoscalinglistener_controller.go @@ -163,21 +163,22 @@ func (r *AutoscalingListenerReconciler) Reconcile(ctx context.Context, req ctrl. return ctrl.Result{}, err } + updatedServiceAccount := serviceAccount.DeepCopy() + var shouldUpdate bool desiredLabels := r.filterAndMergeLabels(serviceAccount.Labels, desiredServiceAccount.Labels) - labelsModified := !maps.Equal(serviceAccount.Labels, desiredLabels) + if !maps.Equal(serviceAccount.Labels, desiredLabels) { + updatedServiceAccount.Labels = desiredLabels + shouldUpdate = true + } desiredAnnotations := r.filterAndMergeAnnotations(serviceAccount.Annotations, desiredServiceAccount.Annotations) - annotationsModified := !maps.Equal(serviceAccount.Annotations, desiredAnnotations) - if labelsModified || annotationsModified { - updatedServiceAccount := serviceAccount.DeepCopy() - if labelsModified { - updatedServiceAccount.Labels = desiredLabels - } - if annotationsModified { - updatedServiceAccount.Annotations = desiredAnnotations - } + if !r.annotationsEqual(serviceAccount.Annotations, desiredAnnotations) { + updatedServiceAccount.Annotations = desiredAnnotations + shouldUpdate = true + } + if shouldUpdate { log.Info("Updating listener service account") - if err := r.Patch(ctx, updatedServiceAccount, client.MergeFrom(&serviceAccount)); err != nil { + if err := r.Update(ctx, updatedServiceAccount); err != nil { log.Error(err, "Failed to update listener service account") return ctrl.Result{}, err } @@ -206,24 +207,25 @@ func (r *AutoscalingListenerReconciler) Reconcile(ctx context.Context, req ctrl. switch { case err == nil: desiredRole := r.newScaleSetListenerRole(&autoscalingListener) + updatedRole := listenerRole.DeepCopy() + var shouldUpdate bool desiredLabels := r.filterAndMergeLabels(listenerRole.Labels, desiredRole.Labels) - labelsModified := !maps.Equal(listenerRole.Labels, desiredLabels) + if !maps.Equal(listenerRole.Labels, desiredLabels) { + updatedRole.Labels = desiredLabels + shouldUpdate = true + } desiredAnnotations := r.filterAndMergeAnnotations(listenerRole.Annotations, desiredRole.Annotations) - annotationsModified := !maps.Equal(listenerRole.Annotations, desiredAnnotations) - rulesModified := !reflect.DeepEqual(listenerRole.Rules, desiredRole.Rules) - if labelsModified || annotationsModified || rulesModified { - updatedRole := listenerRole.DeepCopy() - if labelsModified { - updatedRole.Labels = desiredLabels - } - if annotationsModified { - updatedRole.Annotations = desiredAnnotations - } - if rulesModified { - updatedRole.Rules = desiredRole.Rules - } + if !r.annotationsEqual(listenerRole.Annotations, desiredAnnotations) { + updatedRole.Annotations = desiredAnnotations + shouldUpdate = true + } + if !reflect.DeepEqual(listenerRole.Rules, desiredRole.Rules) { + updatedRole.Rules = desiredRole.Rules + shouldUpdate = true + } + if shouldUpdate { log.Info("Updating listener role") - if err := r.Patch(ctx, updatedRole, client.MergeFrom(&listenerRole)); err != nil { + if err := r.Update(ctx, updatedRole); err != nil { log.Error(err, "Failed to update listener role") return ctrl.Result{}, err } @@ -248,20 +250,21 @@ func (r *AutoscalingListenerReconciler) Reconcile(ctx context.Context, req ctrl. &listenerRole, &serviceAccount, ) + updatedRoleBinding := listenerRoleBinding.DeepCopy() + var shouldUpdate bool desiredLabels := r.filterAndMergeLabels(listenerRoleBinding.Labels, desiredRoleBinding.Labels) - labelsModified := !maps.Equal(listenerRoleBinding.Labels, desiredLabels) + if !maps.Equal(listenerRoleBinding.Labels, desiredLabels) { + updatedRoleBinding.Labels = desiredLabels + shouldUpdate = true + } desiredAnnotations := r.filterAndMergeAnnotations(listenerRoleBinding.Annotations, desiredRoleBinding.Annotations) - annotationsModified := !maps.Equal(listenerRoleBinding.Annotations, desiredAnnotations) - if labelsModified || annotationsModified { - updatedRoleBinding := listenerRoleBinding.DeepCopy() - if labelsModified { - updatedRoleBinding.Labels = desiredLabels - } - if annotationsModified { - updatedRoleBinding.Annotations = desiredAnnotations - } + if !r.annotationsEqual(listenerRoleBinding.Annotations, desiredAnnotations) { + updatedRoleBinding.Annotations = desiredAnnotations + shouldUpdate = true + } + if shouldUpdate { log.Info("Updating listener role binding") - if err := r.Patch(ctx, updatedRoleBinding, client.MergeFrom(&listenerRoleBinding)); err != nil { + if err := r.Update(ctx, updatedRoleBinding); err != nil { log.Error(err, "Failed to update listener role binding") return ctrl.Result{}, err } @@ -303,20 +306,21 @@ func (r *AutoscalingListenerReconciler) Reconcile(ctx context.Context, req ctrl. log.Error(err, "Failed to build desired listener proxy secret") return ctrl.Result{}, err } + updatedProxySecret := proxySecret.DeepCopy() + var shouldUpdate bool desiredLabels := r.filterAndMergeLabels(proxySecret.Labels, desiredListenerProxy.Labels) - labelsModified := !maps.Equal(proxySecret.Labels, desiredLabels) + if !maps.Equal(proxySecret.Labels, desiredLabels) { + updatedProxySecret.Labels = desiredLabels + shouldUpdate = true + } desiredAnnotations := r.filterAndMergeAnnotations(proxySecret.Annotations, desiredListenerProxy.Annotations) - annotationsModified := !maps.Equal(proxySecret.Annotations, desiredAnnotations) - if labelsModified || annotationsModified { - updatedProxySecret := proxySecret.DeepCopy() - if labelsModified { - updatedProxySecret.Labels = desiredLabels - } - if annotationsModified { - updatedProxySecret.Annotations = desiredAnnotations - } + if !r.annotationsEqual(proxySecret.Annotations, desiredAnnotations) { + updatedProxySecret.Annotations = desiredAnnotations + shouldUpdate = true + } + if shouldUpdate { log.Info("Updating listener proxy secret") - if err := r.Patch(ctx, updatedProxySecret, client.MergeFrom(&proxySecret)); err != nil { + if err := r.Update(ctx, updatedProxySecret); err != nil { log.Error(err, "Failed to update listener proxy secret") return ctrl.Result{}, err } @@ -389,21 +393,22 @@ func (r *AutoscalingListenerReconciler) Reconcile(ctx context.Context, req ctrl. if err != nil { return ctrl.Result{}, fmt.Errorf("failed to build listener config secret: %w", err) } + updatedSecret := listenerConfigSecret.DeepCopy() + var shouldUpdate bool desiredLabels := r.filterAndMergeLabels(listenerConfigSecret.Labels, desiredSecret.Labels) - labelsModified := !maps.Equal(listenerConfigSecret.Labels, desiredLabels) + if !maps.Equal(listenerConfigSecret.Labels, desiredLabels) { + updatedSecret.Labels = desiredLabels + shouldUpdate = true + } desiredAnnotations := r.filterAndMergeAnnotations(listenerConfigSecret.Annotations, desiredSecret.Annotations) - annotationsModified := !maps.Equal(listenerConfigSecret.Annotations, desiredAnnotations) + if !r.annotationsEqual(listenerConfigSecret.Annotations, desiredAnnotations) { + updatedSecret.Annotations = desiredAnnotations + shouldUpdate = true + } - if labelsModified || annotationsModified { - updatedSecret := listenerConfigSecret.DeepCopy() - if labelsModified { - updatedSecret.Labels = desiredLabels - } - if annotationsModified { - updatedSecret.Annotations = desiredAnnotations - } + if shouldUpdate { log.Info("Updating listener config secret", "namespace", updatedSecret.Namespace, "name", updatedSecret.Name) - if err := r.Patch(ctx, updatedSecret, client.MergeFrom(&listenerConfigSecret)); err != nil { + if err := r.Update(ctx, updatedSecret); err != nil { return ctrl.Result{}, fmt.Errorf("failed to update listener config secret: %w", err) } return ctrl.Result{Requeue: true}, nil @@ -462,9 +467,17 @@ func (r *AutoscalingListenerReconciler) Reconcile(ctx context.Context, req ctrl. return ctrl.Result{}, err } - shouldReCreate := desiredPod.Annotations[AnnotationKeyIntegrityHash] != listenerPod.Annotations[AnnotationKeyIntegrityHash] - if shouldReCreate { - log.Info("Listener pod dependency changed, recreating listener pod") + if desiredPod.Annotations[AnnotationKeyIntegrityHash] != listenerPod.Annotations[AnnotationKeyIntegrityHash] { + // Since the pod is controlled by a pod controller, we tag the pod with integrity hash. + // If the integrity hash is changed, that means the new spec is different. Keep in mind, the tagged hash + // is created by hashing only the fields this controller sets. + log.Info( + "Listener pod dependency changed, recreating listener pod", + "desiredSpec", + mustJSON(desiredPod.Spec), + "currentSpec", + mustJSON(listenerPod.Spec), + ) if err := r.deleteListenerPod(ctx, &autoscalingListener, &listenerPod, log); err != nil { return ctrl.Result{}, err } @@ -473,21 +486,22 @@ func (r *AutoscalingListenerReconciler) Reconcile(ctx context.Context, req ctrl. return ctrl.Result{}, nil } + updatedPod := listenerPod.DeepCopy() + var shouldUpdate bool desiredLabels := r.filterAndMergeLabels(listenerPod.Labels, desiredPod.Labels) - labelsModified := !maps.Equal(listenerPod.Labels, desiredLabels) + if !maps.Equal(listenerPod.Labels, desiredLabels) { + updatedPod.Labels = desiredLabels + shouldUpdate = true + } desiredAnnotations := r.filterAndMergeAnnotations(listenerPod.Annotations, desiredPod.Annotations) - annotationsModified := !maps.Equal(listenerPod.Annotations, desiredAnnotations) + if !r.annotationsEqual(listenerPod.Annotations, desiredAnnotations) { + updatedPod.Annotations = desiredAnnotations + shouldUpdate = true + } - if labelsModified || annotationsModified { - updatedPod := listenerPod.DeepCopy() - if labelsModified { - updatedPod.Labels = desiredLabels - } - if annotationsModified { - updatedPod.Annotations = desiredAnnotations - } + if shouldUpdate { log.Info("Updating listener pod", "namespace", updatedPod.Namespace, "name", updatedPod.Name) - if err := r.Patch(ctx, updatedPod, client.MergeFrom(&listenerPod)); err != nil { + if err := r.Update(ctx, updatedPod); err != nil { log.Error(err, "Unable to update listener pod", "namespace", updatedPod.Namespace, "name", updatedPod.Name) return ctrl.Result{}, err } @@ -513,7 +527,11 @@ func (r *AutoscalingListenerReconciler) Reconcile(ctx context.Context, req ctrl. return ctrl.Result{}, err } - log.Info("Creating listener pod", "namespace", desiredPod.Namespace, "name", desiredPod.Name) + log.Info( + "Creating listener pod", + "namespace", desiredPod.Namespace, + "name", desiredPod.Name, + ) if err := r.Create(ctx, desiredPod); err != nil { log.Error(err, "Unable to create listener pod", "namespace", desiredPod.Namespace, "name", desiredPod.Name) return ctrl.Result{}, err diff --git a/controllers/actions.github.com/autoscalingrunnerset_controller.go b/controllers/actions.github.com/autoscalingrunnerset_controller.go index 1e1e2bb4..74356ca0 100644 --- a/controllers/actions.github.com/autoscalingrunnerset_controller.go +++ b/controllers/actions.github.com/autoscalingrunnerset_controller.go @@ -142,13 +142,11 @@ func (r *AutoscalingRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl } // Something has changed, we need to re-apply the pending phase and change hash annotation to trigger the update of runner scale set and listener. - if targetHash := autoscalingRunnerSet.Hash(); autoscalingRunnerSet.Annotations[AnnotationKeyIntegrityHash] != targetHash { - // TODO: apply the version label + if targetHash := autoscalingRunnerSetIntegrityHash(&autoscalingRunnerSet); autoscalingRunnerSet.Annotations[AnnotationKeyIntegrityHash] != targetHash { original := autoscalingRunnerSet.DeepCopy() if autoscalingRunnerSet.Annotations == nil { autoscalingRunnerSet.Annotations = map[string]string{} } - autoscalingRunnerSet.Annotations[AnnotationKeyIntegrityHash] = targetHash if err := r.Patch(ctx, &autoscalingRunnerSet, client.MergeFrom(original)); err != nil { log.Error(err, "Failed to update autoscaling runner set with new change hash and pending phase") return ctrl.Result{}, err @@ -291,7 +289,8 @@ func (r *AutoscalingRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl return ctrl.Result{}, nil } - if ephemeralRunnerSet.Annotations[AnnotationKeyIntegrityHash] != desired.Annotations[AnnotationKeyIntegrityHash] { + integrityDiff := desired.Annotations[AnnotationKeyIntegrityHash] != ephemeralRunnerSetIntegrityHash(&ephemeralRunnerSet) + if integrityDiff { original := ephemeralRunnerSet.DeepCopy() ephemeralRunnerSet.Spec.EphemeralRunnerMetadata = desired.Spec.EphemeralRunnerMetadata ephemeralRunnerSet.Spec.EphemeralRunnerSpec = desired.Spec.EphemeralRunnerSpec @@ -310,7 +309,7 @@ func (r *AutoscalingRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl ephemeralRunnerMetadataModified := !cmp.Equal(ephemeralRunnerSet.Spec.EphemeralRunnerMetadata, desired.Spec.EphemeralRunnerMetadata) ephemeralRunnerLabelsModified := !maps.Equal(ephemeralRunnerSet.Labels, desired.Labels) - ephemeralRunnerAnnotationsModified := !maps.Equal(ephemeralRunnerSet.Annotations, desired.Annotations) + ephemeralRunnerAnnotationsModified := !r.annotationsEqual(ephemeralRunnerSet.Annotations, desired.Annotations) if ephemeralRunnerLabelsModified || ephemeralRunnerAnnotationsModified || ephemeralRunnerMetadataModified { original := ephemeralRunnerSet.DeepCopy() @@ -358,14 +357,17 @@ func (r *AutoscalingRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl } if !cmp.Equal(listener.Spec, desired.Spec) || - !cmp.Equal(listener.Labels, desired.Labels) || - !cmp.Equal(listener.Annotations, desired.Annotations) { - log.Info("Deleting AutoscalingListener to re-create with updated spec") - if err := r.Delete(ctx, &listener); err != nil { - log.Error(err, "Failed to delete AutoscalingListener for re-creation") + !maps.Equal(listener.Labels, desired.Labels) || + !r.annotationsEqual(listener.Annotations, desired.Annotations) { + log.Info("Updating listener") + listener.Spec = desired.Spec + listener.Annotations = r.filterAndMergeAnnotations(listener.Annotations, desired.Annotations) + listener.Labels = r.filterAndMergeLabels(listener.Labels, desired.Labels) + if err := r.Update(ctx, &listener); err != nil { + log.Error(err, "Failed to update AutoscalingListener with new spec") return ctrl.Result{}, err } - log.Info("Deleted AutoscalingListener, will re-create on next reconcile") + log.Info("Successfully updated AutoscalingListener with new spec") return ctrl.Result{}, nil } } diff --git a/controllers/actions.github.com/autoscalingrunnerset_controller_test.go b/controllers/actions.github.com/autoscalingrunnerset_controller_test.go index d6783271..8b950e47 100644 --- a/controllers/actions.github.com/autoscalingrunnerset_controller_test.go +++ b/controllers/actions.github.com/autoscalingrunnerset_controller_test.go @@ -461,7 +461,14 @@ var _ = Describe("Test AutoScalingRunnerSet controller", Ordered, func() { listener := new(v1alpha1.AutoscalingListener) Eventually( func() error { - return k8sClient.Get(ctx, client.ObjectKey{Name: scaleSetListenerName(autoscalingRunnerSet), Namespace: autoscalingRunnerSet.Namespace}, listener) + return k8sClient.Get( + ctx, + client.ObjectKey{ + Name: scaleSetListenerName(autoscalingRunnerSet), + Namespace: autoscalingRunnerSet.Namespace, + }, + listener, + ) }, autoscalingRunnerSetTestTimeout, autoscalingRunnerSetTestInterval, @@ -472,13 +479,21 @@ var _ = Describe("Test AutoScalingRunnerSet controller", Ordered, func() { runnerSet := new(v1alpha1.EphemeralRunnerSet) Eventually( func() error { - return k8sClient.Get(ctx, client.ObjectKey{Name: autoscalingRunnerSet.Name, Namespace: autoscalingRunnerSet.Namespace}, runnerSet) + return k8sClient.Get( + ctx, + client.ObjectKey{ + Name: autoscalingRunnerSet.Name, + Namespace: autoscalingRunnerSet.Namespace, + }, + runnerSet, + ) }, autoscalingRunnerSetTestTimeout, autoscalingRunnerSetTestInterval, ).Should(Succeed(), "EphemeralRunnerSet should be created") originalRunnerSetUID := runnerSet.UID originalRunnerSetHash := runnerSet.Annotations[AnnotationKeyIntegrityHash] + originalResourceVersion := runnerSet.ResourceVersion patched := autoscalingRunnerSet.DeepCopy() patched.Spec.Template.Spec.Containers[0].Image = "ghcr.io/actions/runner:updated" @@ -492,7 +507,8 @@ var _ = Describe("Test AutoScalingRunnerSet controller", Ordered, func() { g.Expect(err).NotTo(HaveOccurred(), "failed to get EphemeralRunnerSet") g.Expect(current.UID).To(Equal(originalRunnerSetUID), "EphemeralRunnerSet should be updated in place") g.Expect(current.Spec.EphemeralRunnerSpec.PodTemplateSpec.Spec.Containers[0].Image).To(Equal("ghcr.io/actions/runner:updated")) - g.Expect(current.Annotations[AnnotationKeyIntegrityHash]).NotTo(Equal(originalRunnerSetHash), "EphemeralRunnerSet spec hash should change") + g.Expect(current.Annotations[AnnotationKeyIntegrityHash]).To(Equal(originalRunnerSetHash), "EphemeralRunnerSet hash integrity key should not be modified") + g.Expect(current.ResourceVersion).NotTo(Equal(originalResourceVersion), "EphemeralRunnerSet ResourceVersion should change after update") }, autoscalingRunnerSetTestTimeout, autoscalingRunnerSetTestInterval, @@ -504,34 +520,50 @@ var _ = Describe("Test AutoScalingRunnerSet controller", Ordered, func() { err := k8sClient.Get(ctx, client.ObjectKey{Name: scaleSetListenerName(autoscalingRunnerSet), Namespace: autoscalingRunnerSet.Namespace}, current) g.Expect(err).NotTo(HaveOccurred(), "failed to get Listener") g.Expect(current.UID).To(Equal(originalListenerUID), "Listener should not be recreated") - g.Expect(current.ResourceVersion).To(Equal(originalListenerResourceVersion), "Listener should not be updated") + g.Expect(current.ResourceVersion).To(Equal(originalListenerResourceVersion), "Listener ResourceVersion should not change after update") }, - time.Second*5, + autoscalingRunnerSetTestTimeout, autoscalingRunnerSetTestInterval, ).Should(Succeed()) }) - It("recreates only the Listener when max runners changes", func() { + It("Updates only the Listener when max runners changes", func() { listener := new(v1alpha1.AutoscalingListener) Eventually( func() error { - return k8sClient.Get(ctx, client.ObjectKey{Name: scaleSetListenerName(autoscalingRunnerSet), Namespace: autoscalingRunnerSet.Namespace}, listener) + return k8sClient.Get( + ctx, + client.ObjectKey{ + Name: scaleSetListenerName(autoscalingRunnerSet), + Namespace: autoscalingRunnerSet.Namespace, + }, + listener, + ) }, autoscalingRunnerSetTestTimeout, autoscalingRunnerSetTestInterval, ).Should(Succeed(), "Listener should be created") originalListenerUID := listener.UID + originalListenerResourceVersion := listener.ResourceVersion + originalListenerIntegrityHash := listener.Annotations[AnnotationKeyIntegrityHash] runnerSet := new(v1alpha1.EphemeralRunnerSet) Eventually( func() error { - return k8sClient.Get(ctx, client.ObjectKey{Name: autoscalingRunnerSet.Name, Namespace: autoscalingRunnerSet.Namespace}, runnerSet) + return k8sClient.Get( + ctx, + client.ObjectKey{ + Name: autoscalingRunnerSet.Name, + Namespace: autoscalingRunnerSet.Namespace, + }, + runnerSet, + ) }, autoscalingRunnerSetTestTimeout, autoscalingRunnerSetTestInterval, ).Should(Succeed(), "EphemeralRunnerSet should be created") - originalRunnerSetUID := runnerSet.UID - originalRunnerSetHash := runnerSet.Annotations[AnnotationKeyIntegrityHash] + originalERSRunnerSetUID := runnerSet.UID + originalERSResourceVersion := runnerSet.ResourceVersion patched := autoscalingRunnerSet.DeepCopy() max := 20 @@ -544,8 +576,10 @@ var _ = Describe("Test AutoScalingRunnerSet controller", Ordered, func() { current := new(v1alpha1.AutoscalingListener) err := k8sClient.Get(ctx, client.ObjectKey{Name: scaleSetListenerName(autoscalingRunnerSet), Namespace: autoscalingRunnerSet.Namespace}, current) g.Expect(err).NotTo(HaveOccurred(), "failed to get Listener") - g.Expect(current.UID).NotTo(Equal(originalListenerUID), "Listener should be recreated") + g.Expect(current.UID).To(Equal(originalListenerUID), "Listener should be updated") + g.Expect(current.Annotations[AnnotationKeyIntegrityHash]).To(Equal(originalListenerIntegrityHash), "Listener hash integrity key should not be modified") g.Expect(current.Spec.MaxRunners).To(Equal(max)) + g.Expect(current.ResourceVersion).NotTo(Equal(originalListenerResourceVersion), "Listener ResourceVersion should change after update") }, autoscalingRunnerSetTestTimeout, autoscalingRunnerSetTestInterval, @@ -556,8 +590,8 @@ var _ = Describe("Test AutoScalingRunnerSet controller", Ordered, func() { current := new(v1alpha1.EphemeralRunnerSet) err := k8sClient.Get(ctx, client.ObjectKey{Name: autoscalingRunnerSet.Name, Namespace: autoscalingRunnerSet.Namespace}, current) g.Expect(err).NotTo(HaveOccurred(), "failed to get EphemeralRunnerSet") - g.Expect(current.UID).To(Equal(originalRunnerSetUID), "EphemeralRunnerSet should not be recreated") - g.Expect(current.Annotations[AnnotationKeyIntegrityHash]).To(Equal(originalRunnerSetHash), "EphemeralRunnerSet spec should not change") + g.Expect(current.UID).To(Equal(originalERSRunnerSetUID), "EphemeralRunnerSet should not be recreated") + g.Expect(current.ResourceVersion).To(Equal(originalERSResourceVersion), "EphemeralRunnerSet spec should not change") }, time.Second*5, autoscalingRunnerSetTestInterval, @@ -568,7 +602,14 @@ var _ = Describe("Test AutoScalingRunnerSet controller", Ordered, func() { runnerSet := new(v1alpha1.EphemeralRunnerSet) Eventually( func() (string, error) { - err := k8sClient.Get(ctx, client.ObjectKey{Name: autoscalingRunnerSet.Name, Namespace: autoscalingRunnerSet.Namespace}, runnerSet) + err := k8sClient.Get( + ctx, + client.ObjectKey{ + Name: autoscalingRunnerSet.Name, + Namespace: autoscalingRunnerSet.Namespace, + }, + runnerSet, + ) if err != nil { return "", err } @@ -586,7 +627,14 @@ var _ = Describe("Test AutoScalingRunnerSet controller", Ordered, func() { Eventually( func() (string, error) { current := new(v1alpha1.EphemeralRunnerSet) - err := k8sClient.Get(ctx, client.ObjectKey{Name: autoscalingRunnerSet.Name, Namespace: autoscalingRunnerSet.Namespace}, current) + err := k8sClient.Get( + ctx, + client.ObjectKey{ + Name: autoscalingRunnerSet.Name, + Namespace: autoscalingRunnerSet.Namespace, + }, + current, + ) if err != nil { return "", err } @@ -601,7 +649,14 @@ var _ = Describe("Test AutoScalingRunnerSet controller", Ordered, func() { runnerSet := new(v1alpha1.EphemeralRunnerSet) Eventually( func() (string, error) { - err := k8sClient.Get(ctx, client.ObjectKey{Name: autoscalingRunnerSet.Name, Namespace: autoscalingRunnerSet.Namespace}, runnerSet) + err := k8sClient.Get( + ctx, + client.ObjectKey{ + Name: autoscalingRunnerSet.Name, + Namespace: autoscalingRunnerSet.Namespace, + }, + runnerSet, + ) if err != nil { return "", err } @@ -614,6 +669,8 @@ var _ = Describe("Test AutoScalingRunnerSet controller", Ordered, func() { patched := autoscalingRunnerSet.DeepCopy() patched.Spec.EphemeralRunnerSetMetadata.Annotations["arc.test/metadata-annotation"] = "updated" patched.Spec.EphemeralRunnerSetMetadata.Annotations["arc.test/new-metadata-annotation"] = "added" + originalERSIntegrityHash := runnerSet.Annotations[AnnotationKeyIntegrityHash] + patched.Spec.EphemeralRunnerSetMetadata.Annotations[AnnotationKeyIntegrityHash] = "must-not-be-modified" err := k8sClient.Patch(ctx, patched, client.MergeFrom(autoscalingRunnerSet)) Expect(err).NotTo(HaveOccurred(), "failed to patch AutoScalingRunnerSet EphemeralRunnerSet metadata") @@ -624,6 +681,7 @@ var _ = Describe("Test AutoScalingRunnerSet controller", Ordered, func() { g.Expect(err).NotTo(HaveOccurred(), "failed to get EphemeralRunnerSet") g.Expect(current.Annotations["arc.test/metadata-annotation"]).To(Equal("updated")) g.Expect(current.Annotations["arc.test/new-metadata-annotation"]).To(Equal("added")) + g.Expect(current.Annotations[AnnotationKeyIntegrityHash]).To(Equal(originalERSIntegrityHash), "EphemeralRunnerSet hash integrity key should not be modified") }, autoscalingRunnerSetTestTimeout, autoscalingRunnerSetTestInterval, @@ -634,7 +692,14 @@ var _ = Describe("Test AutoScalingRunnerSet controller", Ordered, func() { runnerSet := new(v1alpha1.EphemeralRunnerSet) Eventually( func(g Gomega) { - err := k8sClient.Get(ctx, client.ObjectKey{Name: autoscalingRunnerSet.Name, Namespace: autoscalingRunnerSet.Namespace}, runnerSet) + err := k8sClient.Get( + ctx, + client.ObjectKey{ + Name: autoscalingRunnerSet.Name, + Namespace: autoscalingRunnerSet.Namespace, + }, + runnerSet, + ) g.Expect(err).NotTo(HaveOccurred(), "failed to get EphemeralRunnerSet") g.Expect(runnerSet.Spec.EphemeralRunnerMetadata).NotTo(BeNil()) g.Expect(runnerSet.Spec.EphemeralRunnerMetadata.Labels["arc.test/runner-metadata-label"]).To(Equal("initial")) @@ -719,22 +784,38 @@ var _ = Describe("Test AutoScalingRunnerSet controller", Ordered, func() { listener := new(v1alpha1.AutoscalingListener) Eventually( func() error { - return k8sClient.Get(ctx, client.ObjectKey{Name: scaleSetListenerName(autoscalingRunnerSet), Namespace: autoscalingRunnerSet.Namespace}, listener) + return k8sClient.Get( + ctx, + client.ObjectKey{ + Name: scaleSetListenerName(autoscalingRunnerSet), + Namespace: autoscalingRunnerSet.Namespace, + }, + listener, + ) }, autoscalingRunnerSetTestTimeout, autoscalingRunnerSetTestInterval, ).Should(Succeed(), "Listener should be created") originalListenerUID := listener.UID + originalListenerIntegrityHash := listener.Annotations[AnnotationKeyIntegrityHash] runnerSet := new(v1alpha1.EphemeralRunnerSet) Eventually( func() error { - return k8sClient.Get(ctx, client.ObjectKey{Name: autoscalingRunnerSet.Name, Namespace: autoscalingRunnerSet.Namespace}, runnerSet) + return k8sClient.Get( + ctx, + client.ObjectKey{ + Name: autoscalingRunnerSet.Name, + Namespace: autoscalingRunnerSet.Namespace, + }, + runnerSet, + ) }, autoscalingRunnerSetTestTimeout, autoscalingRunnerSetTestInterval, ).Should(Succeed(), "EphemeralRunnerSet should be created") - originalRunnerSetUID := runnerSet.UID + originalEphemeralRunnerSetUID := runnerSet.UID + originalEphemeralRunnerSetIntegrityHash := runnerSet.Annotations[AnnotationKeyIntegrityHash] patched := autoscalingRunnerSet.DeepCopy() patched.Spec.GitHubConfigSecret = updatedSecret.Name @@ -757,8 +838,9 @@ var _ = Describe("Test AutoScalingRunnerSet controller", Ordered, func() { current := new(v1alpha1.EphemeralRunnerSet) err := k8sClient.Get(ctx, client.ObjectKey{Name: autoscalingRunnerSet.Name, Namespace: autoscalingRunnerSet.Namespace}, current) g.Expect(err).NotTo(HaveOccurred(), "failed to get EphemeralRunnerSet") - g.Expect(current.UID).To(Equal(originalRunnerSetUID), "EphemeralRunnerSet should be updated in place") + g.Expect(current.UID).To(Equal(originalEphemeralRunnerSetUID), "EphemeralRunnerSet should be updated in place") g.Expect(current.Spec.EphemeralRunnerSpec.GitHubConfigSecret).To(Equal(updatedSecret.Name)) + g.Expect(current.Annotations[AnnotationKeyIntegrityHash]).To(Equal(originalEphemeralRunnerSetIntegrityHash), "EphemeralRunnerSet hash integrity key should not be modified") }, autoscalingRunnerSetTestTimeout, autoscalingRunnerSetTestInterval, @@ -769,8 +851,9 @@ var _ = Describe("Test AutoScalingRunnerSet controller", Ordered, func() { current := new(v1alpha1.AutoscalingListener) err := k8sClient.Get(ctx, client.ObjectKey{Name: scaleSetListenerName(autoscalingRunnerSet), Namespace: autoscalingRunnerSet.Namespace}, current) g.Expect(err).NotTo(HaveOccurred(), "failed to get Listener") - g.Expect(current.UID).NotTo(Equal(originalListenerUID), "Listener should be recreated") - g.Expect(current.Spec.GitHubConfigSecret).To(Equal(updatedSecret.Name)) + g.Expect(current.UID).To(Equal(originalListenerUID), "Listener should be updated in place") + g.Expect(updatedSecret.Name).To(Equal(current.Spec.GitHubConfigSecret)) + g.Expect(current.Annotations[AnnotationKeyIntegrityHash]).To(Equal(originalListenerIntegrityHash), "Listener hash integrity key should not be modified") }, autoscalingRunnerSetTestTimeout, autoscalingRunnerSetTestInterval, @@ -1099,10 +1182,11 @@ var _ = Describe("Test AutoscalingController creation failures", Ordered, func() }, }, Spec: v1alpha1.AutoscalingRunnerSetSpec{ - GitHubConfigUrl: "https://github.com/owner/repo", - MaxRunners: &max, - MinRunners: &min, - RunnerGroup: "testgroup", + GitHubConfigUrl: "https://github.com/owner/repo", + GitHubConfigSecret: "secret1", + MaxRunners: &max, + MinRunners: &min, + RunnerGroup: "testgroup", Template: corev1.PodTemplateSpec{ Spec: corev1.PodSpec{ Containers: []corev1.Container{ diff --git a/controllers/actions.github.com/resourcebuilder.go b/controllers/actions.github.com/resourcebuilder.go index bdce614a..e08be51a 100644 --- a/controllers/actions.github.com/resourcebuilder.go +++ b/controllers/actions.github.com/resourcebuilder.go @@ -115,6 +115,10 @@ func (b *ResourceBuilder) setControllerReference(owner client.Object, object cli return ctrl.SetControllerReference(owner, object, b.Scheme) } +func autoscalingRunnerSetIntegrityHash(ars *v1alpha1.AutoscalingRunnerSet) string { + return hash.ComputeTemplateHash(&ars.Spec) +} + 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]) if err != nil { @@ -1144,3 +1148,20 @@ func (b *ResourceBuilder) filterAndMergeAnnotations(base, overwrite map[string]s return result } + +// compareAnnotations compares two maps of annotations, ignoring the integrity hash annotation. +func (b *ResourceBuilder) annotationsEqual(m1, m2 map[string]string) bool { + if len(m1) != len(m2) { + return false + } + + for k, v1 := range m1 { + if k == AnnotationKeyIntegrityHash { + continue + } + if v2, ok := m2[k]; !ok || v1 != v2 { + return false + } + } + return true +} diff --git a/controllers/actions.github.com/utils.go b/controllers/actions.github.com/utils.go index a77b24ba..da87eb7b 100644 --- a/controllers/actions.github.com/utils.go +++ b/controllers/actions.github.com/utils.go @@ -1,6 +1,8 @@ package actionsgithubcom import ( + "encoding/json" + "k8s.io/apimachinery/pkg/util/rand" ) @@ -25,3 +27,11 @@ func RandStringRunes(n int) string { } return string(b) } + +func mustJSON(v any) string { + val, err := json.Marshal(v) + if err != nil { + panic(err) + } + return string(val) +}