Fix EphemeralRunnerSet metadata drift check comparing against the wrong value (#4634)

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

controllers/actions.github.com/autoscalingrunnerset_controller.go

@@ -310,14 +310,20 @@ func (r *AutoscalingRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl
 			return ctrl.Result{}, nil
 		}
 
+		// Merge rather than overwrite so annotations/labels applied by other
+		// controllers or users are preserved. Compare against the merge result so
+		// foreign keys do not make this permanently report "modified".
+		desiredLabels := r.filterAndMergeLabels(ephemeralRunnerSet.Labels, desired.Labels)
+		desiredAnnotations := r.mergeAnnotations(ephemeralRunnerSet.Annotations, desired.Annotations)
+
 		ephemeralRunnerMetadataModified := !cmp.Equal(ephemeralRunnerSet.Spec.EphemeralRunnerMetadata, desired.Spec.EphemeralRunnerMetadata)
-		ephemeralRunnerLabelsModified := !maps.Equal(ephemeralRunnerSet.Labels, desired.Labels)
-		ephemeralRunnerAnnotationsModified := !maps.Equal(ephemeralRunnerSet.Annotations, desired.Annotations)
+		ephemeralRunnerLabelsModified := !maps.Equal(ephemeralRunnerSet.Labels, desiredLabels)
+		ephemeralRunnerAnnotationsModified := !maps.Equal(ephemeralRunnerSet.Annotations, desiredAnnotations)
 
 		if ephemeralRunnerLabelsModified || ephemeralRunnerAnnotationsModified || ephemeralRunnerMetadataModified {
 			original := ephemeralRunnerSet.DeepCopy()
-			ephemeralRunnerSet.Labels = r.filterAndMergeLabels(ephemeralRunnerSet.Labels, desired.Labels)
-			ephemeralRunnerSet.Annotations = r.mergeAnnotations(ephemeralRunnerSet.Annotations, desired.Annotations)
+			ephemeralRunnerSet.Labels = desiredLabels
+			ephemeralRunnerSet.Annotations = desiredAnnotations
 			ephemeralRunnerSet.Spec.EphemeralRunnerMetadata = desired.Spec.EphemeralRunnerMetadata
 			log.Info("Updating ephemeral runner set metadata to match desired labels and annotations")
 			if err := r.Patch(ctx, &ephemeralRunnerSet, client.MergeFrom(original)); err != nil {

controllers/actions.github.com/autoscalingrunnerset_controller_test.go

@@ -665,6 +665,75 @@ var _ = Describe("Test AutoScalingRunnerSet controller", Ordered, func() {
 			).Should(Succeed(), "EphemeralRunnerSet should be patched with annotation-only metadata drift")
 		})
 
+		It("preserves foreign annotations and labels on the EphemeralRunnerSet", func() {
+			runnerSet := new(v1alpha1.EphemeralRunnerSet)
+			Eventually(
+				func() (string, error) {
+					err := k8sClient.Get(ctx, client.ObjectKey{Name: autoscalingRunnerSet.Name, Namespace: autoscalingRunnerSet.Namespace}, runnerSet)
+					if err != nil {
+						return "", err
+					}
+					return runnerSet.Annotations["arc.test/metadata-annotation"], nil
+				},
+				autoscalingRunnerSetTestTimeout,
+				autoscalingRunnerSetTestInterval,
+			).Should(Equal("initial"), "EphemeralRunnerSet should start with the predefined annotation")
+
+			// Simulate a third party (admission webhook, another controller, a user)
+			// adding metadata the AutoscalingRunnerSet knows nothing about.
+			foreign := runnerSet.DeepCopy()
+			foreign.Annotations["thirdparty.example.com/injected"] = "keep-me"
+			foreign.Labels["thirdparty.example.com/injected"] = "keep-me"
+			err := k8sClient.Patch(ctx, foreign, client.MergeFrom(runnerSet))
+			Expect(err).NotTo(HaveOccurred(), "failed to inject foreign metadata on EphemeralRunnerSet")
+
+			// Force the controller through the metadata reconciliation path.
+			patched := autoscalingRunnerSet.DeepCopy()
+			patched.Spec.EphemeralRunnerSetMetadata.Annotations["arc.test/metadata-annotation"] = "updated"
+			err = k8sClient.Patch(ctx, patched, client.MergeFrom(autoscalingRunnerSet))
+			Expect(err).NotTo(HaveOccurred(), "failed to patch AutoScalingRunnerSet EphemeralRunnerSet metadata")
+
+			Eventually(
+				func(g Gomega) {
+					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.Annotations["arc.test/metadata-annotation"]).To(Equal("updated"))
+				},
+				autoscalingRunnerSetTestTimeout,
+				autoscalingRunnerSetTestInterval,
+			).Should(Succeed(), "desired annotation should still propagate")
+
+			// The foreign keys must survive, and the controller must be able to get
+			// past the metadata block. Before the fix the comparison was made
+			// against the unmerged desired metadata, so a foreign key made the
+			// block report "modified" on every reconcile and return early, which
+			// meant nothing after it — including listener reconciliation — ever
+			// ran again. Deleting the listener makes that stall observable.
+			listener := new(v1alpha1.AutoscalingListener)
+			Eventually(
+				func() error {
+					return k8sClient.Get(ctx, client.ObjectKey{Name: scaleSetListenerName(autoscalingRunnerSet), Namespace: autoscalingRunnerSet.Namespace}, listener)
+				},
+				autoscalingRunnerSetTestTimeout,
+				autoscalingRunnerSetTestInterval,
+			).Should(Succeed(), "listener should exist before deletion")
+			Expect(k8sClient.Delete(ctx, listener)).To(Succeed())
+			Eventually(
+				func(g Gomega) {
+					recreated := new(v1alpha1.AutoscalingListener)
+					g.Expect(k8sClient.Get(ctx, client.ObjectKey{Name: scaleSetListenerName(autoscalingRunnerSet), Namespace: autoscalingRunnerSet.Namespace}, recreated)).To(Succeed())
+
+					current := new(v1alpha1.EphemeralRunnerSet)
+					g.Expect(k8sClient.Get(ctx, client.ObjectKey{Name: autoscalingRunnerSet.Name, Namespace: autoscalingRunnerSet.Namespace}, current)).To(Succeed())
+					g.Expect(current.Annotations).To(HaveKeyWithValue("thirdparty.example.com/injected", "keep-me"), "foreign annotation must not be stripped")
+					g.Expect(current.Labels).To(HaveKeyWithValue("thirdparty.example.com/injected", "keep-me"), "foreign label must not be stripped")
+				},
+				autoscalingRunnerSetTestTimeout,
+				autoscalingRunnerSetTestInterval,
+			).Should(Succeed(), "reconciliation must converge past the metadata block instead of looping on it")
+		})
+
 		It("updates EphemeralRunnerSet runner metadata when only EphemeralRunner metadata changes", func() {
 			runnerSet := new(v1alpha1.EphemeralRunnerSet)
 			Eventually(