From baf5070ebe1ab9faf604af30dbc995ff86f3562e Mon Sep 17 00:00:00 2001 From: Nikola Jokic Date: Wed, 9 Sep 2026 11:49:11 +0200 Subject: [PATCH 1/2] Detect listener pod drift by comparing the pod, not a hash The listener pod controller decided whether to delete and rebuild the pod by comparing an actions.github.com/integrity-hash annotation that the resource builder stamped onto the desired pod. The hash was recomputed from scratch on every reconcile out of the pod spec plus the hashes of the listener, the config secret, the service account, the role, the role binding and the metrics config, which meant the answer to "did the pod change?" depended on a pile of bookkeeping that had to stay in sync by hand. Compare the two pods directly instead. The comparison is Semantic.DeepDerivative rather than DeepEqual because the live pod carries a large number of fields the desired pod never sets: nodeName, dnsPolicy, schedulerName, securityContext, the default tolerations, the kube-api-access projected volume and its mount, defaulted protocol and imagePullPolicy, and so on. DeepEqual would report drift on every reconcile of every healthy listener and spin in a delete/create loop, and pre-populating the defaults does not help since nodeName is scheduler-assigned and the access-token volume has a generated name. The price of DeepDerivative is that it ignores empty values on the desired side, so field removal is invisible. That is harmless for anything sourced from the user-facing template, because the AutoscalingRunnerSet controller replaces the listener wholesale when its spec changes, but container ports come from the --listener-metrics-addr controller flag instead of from any resource, so port removal is checked separately by length. The config secret needs its own signal. The pod mounts it as a volume and references it by name, so the pod spec is byte-identical before and after a config change, while the listener parses config.json once at startup. Without something extra, a new scale set URL, TLS certificate, metrics configuration or scaler tuning would never reach the running listener. The desired pod therefore carries the secret's resource version as an annotation and drift is detected by comparing it. An empty annotation on the live pod is ignored so that pods created before this annotation existed are not all recreated on controller upgrade. The integrity-hash constant and its remaining writers are left in place; this change only removes the listener pod reader. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../autoscalinglistener_controller.go | 2 +- controllers/actions.github.com/constants.go | 7 + controllers/actions.github.com/helpers.go | 97 +++++ .../actions.github.com/helpers_bench_test.go | 19 + .../helpers_listener_test.go | 331 ++++++++++++++++++ .../actions.github.com/resourcebuilder.go | 52 +-- 6 files changed, 461 insertions(+), 47 deletions(-) create mode 100644 controllers/actions.github.com/helpers.go create mode 100644 controllers/actions.github.com/helpers_bench_test.go create mode 100644 controllers/actions.github.com/helpers_listener_test.go diff --git a/controllers/actions.github.com/autoscalinglistener_controller.go b/controllers/actions.github.com/autoscalinglistener_controller.go index 497a294abb..662a9a1adf 100644 --- a/controllers/actions.github.com/autoscalinglistener_controller.go +++ b/controllers/actions.github.com/autoscalinglistener_controller.go @@ -467,7 +467,7 @@ func (r *AutoscalingListenerReconciler) Reconcile(ctx context.Context, req ctrl. return ctrl.Result{}, err } - shouldReCreate := desiredPod.Annotations[annotationKeyIntegrityHash] != listenerPod.Annotations[annotationKeyIntegrityHash] + shouldReCreate := listenerPodSpecRequiresRecreation(&listenerPod, desiredPod) if shouldReCreate { log.Info("Listener pod dependency changed, recreating listener pod") if err := r.deleteListenerPod(ctx, &autoscalingListener, &listenerPod, log); err != nil { diff --git a/controllers/actions.github.com/constants.go b/controllers/actions.github.com/constants.go index 588c95beb7..48c78e4b72 100644 --- a/controllers/actions.github.com/constants.go +++ b/controllers/actions.github.com/constants.go @@ -50,6 +50,13 @@ const ( AnnotationKeyGitHubRunnerGroupName = "actions.github.com/runner-group-name" AnnotationKeyGitHubRunnerScaleSetName = "actions.github.com/runner-scale-set-name" AnnotationKeyPatchID = "actions.github.com/patch-id" + // AnnotationKeyListenerConfigResourceVersion records the resource version of + // the listener config secret the listener pod was created from. The pod + // mounts that secret and parses it once at startup, so a change to its + // contents only takes effect after a restart. Nothing about the change is + // visible in the pod spec, which references the secret by name, so the + // resource version is carried on the pod to make the drift observable. + AnnotationKeyListenerConfigResourceVersion = "actions.github.com/listener-config-resource-version" ) // Labels applied to listener roles diff --git a/controllers/actions.github.com/helpers.go b/controllers/actions.github.com/helpers.go new file mode 100644 index 0000000000..59ba87782b --- /dev/null +++ b/controllers/actions.github.com/helpers.go @@ -0,0 +1,97 @@ +package actionsgithubcom + +import ( + corev1 "k8s.io/api/core/v1" + apiequality "k8s.io/apimachinery/pkg/api/equality" +) + +// listenerPodSpecRequiresRecreation reports whether the live listener pod must be +// deleted and rebuilt to match the desired spec. +// +// The config secret is checked first. The pod mounts it as a volume and the +// listener parses it once at startup, so a change to the scale set URL, the TLS +// certificate, the metrics configuration or the scaler tuning only reaches the +// listener after a restart. None of that is visible in the pod spec, which +// references the secret by name and is byte-identical before and after, so the +// desired pod carries the secret's resource version as an annotation and drift +// is detected by comparing it. An empty annotation on the live pod is ignored: +// pods created before this annotation existed would otherwise all be recreated +// on controller upgrade, and the next legitimate config change recreates them +// anyway. +// +// The resource version also moves when only the secret's labels or annotations +// change, which does not affect the listener. That is accepted rather than +// worked around by hashing the secret data: those fields come from the +// AutoscalingListener spec, and a change to that spec already makes the +// AutoscalingRunnerSet controller replace the listener wholesale. +// +// DeepDerivative, not DeepEqual: the live pod carries a large number of fields +// the desired pod never sets, written by the API server and by admission +// (nodeName, dnsPolicy, schedulerName, securityContext, enableServiceLinks, +// the default tolerations, the kube-api-access-* projected volume and its mount, +// terminationMessagePath, imagePullPolicy, secret defaultMode, ...). DeepEqual +// would therefore report drift on every reconcile of every healthy listener and +// spin in a delete/create loop. It cannot be made to work by pre-populating the +// defaults either, since nodeName is scheduler-assigned and the access-token +// volume has a generated name. See TestListenerPodSpecRequiresRecreation. +// +// The cost of DeepDerivative is that it ignores empty values on the desired side, +// so a field being *removed* is invisible to it. For everything sourced from the +// user-facing template that is harmless: the AutoscalingRunnerSet controller +// compares the whole AutoscalingListener spec with cmp.Equal and deletes the +// listener outright, which takes the pod with it. Container ports are the +// exception, because they come from the --listener-metrics-addr controller flag +// rather than from any resource, so disabling metrics would otherwise leave the +// port on the pod forever. Comparing port length is enough: additions and value +// changes are already caught by DeepDerivative, only removal is blind. The +// contents are deliberately not compared, because the API server defaults +// protocol to TCP and that would reintroduce the delete/create loop. +func listenerPodSpecRequiresRecreation(current, desired *corev1.Pod) bool { + if current == nil || desired == nil { + return current != desired + } + + if listenerConfigChanged(current, desired) { + return true + } + + if listenerContainerPortsRemoved(current, desired) { + return true + } + + return !apiequality.Semantic.DeepDerivative(desired.Spec, current.Spec) +} + +func listenerConfigChanged(current, desired *corev1.Pod) bool { + currentVersion := current.Annotations[AnnotationKeyListenerConfigResourceVersion] + desiredVersion := desired.Annotations[AnnotationKeyListenerConfigResourceVersion] + if currentVersion == "" || desiredVersion == "" { + return false + } + return currentVersion != desiredVersion +} + +func listenerContainerPortsRemoved(current, desired *corev1.Pod) bool { + for i := range desired.Spec.Containers { + desiredContainer := &desired.Spec.Containers[i] + currentContainer := findContainerByName(current.Spec.Containers, desiredContainer.Name) + if currentContainer == nil { + // A container the live pod does not have at all is drift that + // DeepDerivative already reports; nothing to decide here. + continue + } + if len(desiredContainer.Ports) < len(currentContainer.Ports) { + return true + } + } + return false +} + +func findContainerByName(containers []corev1.Container, name string) *corev1.Container { + for i := range containers { + if containers[i].Name == name { + return &containers[i] + } + } + return nil +} diff --git a/controllers/actions.github.com/helpers_bench_test.go b/controllers/actions.github.com/helpers_bench_test.go new file mode 100644 index 0000000000..95cc9ca895 --- /dev/null +++ b/controllers/actions.github.com/helpers_bench_test.go @@ -0,0 +1,19 @@ +package actionsgithubcom + +import ( + "testing" +) + +// BenchmarkListenerPodSpecRequiresRecreation measures the listener pod drift +// check, which also runs on every AutoscalingListener reconcile. +func BenchmarkListenerPodSpecRequiresRecreation(b *testing.B) { + desired := desiredListenerPod() + live := livePodFromDesired(desired) + b.ReportAllocs() + b.ResetTimer() + for range b.N { + if listenerPodSpecRequiresRecreation(live, desired) { + b.Fatal("expected no recreation") + } + } +} diff --git a/controllers/actions.github.com/helpers_listener_test.go b/controllers/actions.github.com/helpers_listener_test.go new file mode 100644 index 0000000000..ecd4044680 --- /dev/null +++ b/controllers/actions.github.com/helpers_listener_test.go @@ -0,0 +1,331 @@ +package actionsgithubcom + +import ( + "testing" + + "github.com/actions/actions-runner-controller/apis/actions.github.com/v1alpha1" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/utils/ptr" +) + +// desiredListenerPod mirrors the shape produced by newScaleSetListenerPod: only +// the handful of fields the builder actually sets. +func desiredListenerPod() *corev1.Pod { + grace := int64(60) + return &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{Name: "listener", Namespace: "arc-systems"}, + Spec: corev1.PodSpec{ + ServiceAccountName: "listener", + NodeSelector: map[string]string{"kubernetes.io/os": "linux"}, + Containers: []corev1.Container{{ + Name: autoscalingListenerContainerName, + Image: "ghcr.io/actions/arc:0.1.0", + Command: []string{"/ghalistener"}, + Env: []corev1.EnvVar{ + {Name: "LISTENER_CONFIG_PATH", Value: "/etc/gha-listener/config.json"}, + }, + Ports: []corev1.ContainerPort{{ContainerPort: 8080}}, + VolumeMounts: []corev1.VolumeMount{ + {Name: "listener-config", MountPath: "/etc/gha-listener", ReadOnly: true}, + }, + }}, + Volumes: []corev1.Volume{{ + Name: "listener-config", + VolumeSource: corev1.VolumeSource{ + Secret: &corev1.SecretVolumeSource{SecretName: "listener-config"}, + }, + }}, + RestartPolicy: corev1.RestartPolicyNever, + TerminationGracePeriodSeconds: &grace, + }, + } +} + +// livePodFromDesired approximates what the API server and admission return after +// the desired pod is created: every field the controller set, plus the defaults +// and injections it did not. +func livePodFromDesired(desired *corev1.Pod) *corev1.Pod { + live := desired.DeepCopy() + defaultMode := int32(420) + enableServiceLinks := true + tolerationSeconds := int64(300) + + live.Spec.NodeName = "node-1" + live.Spec.DNSPolicy = corev1.DNSClusterFirst + live.Spec.SchedulerName = "default-scheduler" + live.Spec.SecurityContext = &corev1.PodSecurityContext{} + live.Spec.DeprecatedServiceAccount = desired.Spec.ServiceAccountName + live.Spec.EnableServiceLinks = &enableServiceLinks + live.Spec.PreemptionPolicy = ptr.To(corev1.PreemptLowerPriority) + live.Spec.Priority = ptr.To(int32(0)) + live.Spec.Tolerations = []corev1.Toleration{ + {Key: "node.kubernetes.io/not-ready", Operator: corev1.TolerationOpExists, + Effect: corev1.TaintEffectNoExecute, TolerationSeconds: &tolerationSeconds}, + {Key: "node.kubernetes.io/unreachable", Operator: corev1.TolerationOpExists, + Effect: corev1.TaintEffectNoExecute, TolerationSeconds: &tolerationSeconds}, + } + live.Spec.Volumes[0].Secret.DefaultMode = &defaultMode + live.Spec.Volumes = append(live.Spec.Volumes, corev1.Volume{ + Name: "kube-api-access-x7f2k", + VolumeSource: corev1.VolumeSource{ + Projected: &corev1.ProjectedVolumeSource{DefaultMode: &defaultMode}, + }, + }) + live.Spec.Containers[0].TerminationMessagePath = corev1.TerminationMessagePathDefault + live.Spec.Containers[0].TerminationMessagePolicy = corev1.TerminationMessageReadFile + live.Spec.Containers[0].ImagePullPolicy = corev1.PullIfNotPresent + live.Spec.Containers[0].Ports[0].Protocol = corev1.ProtocolTCP + live.Spec.Containers[0].VolumeMounts = append(live.Spec.Containers[0].VolumeMounts, corev1.VolumeMount{ + Name: "kube-api-access-x7f2k", MountPath: "/var/run/secrets/kubernetes.io/serviceaccount", ReadOnly: true, + }) + return live +} + +// TestListenerPodSpecRequiresRecreation_SteadyState is the guard rail against +// swapping DeepDerivative for DeepEqual. +// +// A healthy listener pod carries many API-server defaults and admission +// injections that the desired pod never sets. If this comparison ever becomes +// strict, the controller deletes and recreates the pod on every reconcile, the +// listener never lives long enough to poll jobs, and every scale set in the +// cluster stops functioning. This test must keep passing. +func TestListenerPodSpecRequiresRecreation_SteadyState(t *testing.T) { + desired := desiredListenerPod() + live := livePodFromDesired(desired) + + assert.False(t, listenerPodSpecRequiresRecreation(live, desired), + "a healthy pod carrying only API-server defaults must never be recreated; "+ + "if this fails, the comparison became too strict and will spin in a delete/create loop") +} + +func TestListenerPodSpecRequiresRecreation(t *testing.T) { + tests := map[string]struct { + mutateDesired func(*corev1.Pod) + want bool + why string + }{ + "identical": { + mutateDesired: func(*corev1.Pod) {}, + want: false, + why: "no drift", + }, + "image changed": { + mutateDesired: func(p *corev1.Pod) { p.Spec.Containers[0].Image = "ghcr.io/actions/arc:0.2.0" }, + want: true, + why: "an upgraded listener image must roll the pod", + }, + "metrics port removed": { + mutateDesired: func(p *corev1.Pod) { p.Spec.Containers[0].Ports = nil }, + want: true, + why: "disabling --listener-metrics-addr must remove the port from the running pod", + }, + "metrics port value changed": { + mutateDesired: func(p *corev1.Pod) { p.Spec.Containers[0].Ports[0].ContainerPort = 9090 }, + want: true, + why: "changing the metrics port must roll the pod", + }, + "metrics port added": { + mutateDesired: func(p *corev1.Pod) { + p.Spec.Containers[0].Ports = append(p.Spec.Containers[0].Ports, + corev1.ContainerPort{ContainerPort: 9090}) + }, + want: true, + why: "an added port must roll the pod", + }, + "env var added": { + mutateDesired: func(p *corev1.Pod) { + p.Spec.Containers[0].Env = append(p.Spec.Containers[0].Env, + corev1.EnvVar{Name: "HTTP_PROXY", Value: "http://proxy:8080"}) + }, + want: true, + why: "proxy configuration must roll the pod", + }, + "env value changed": { + mutateDesired: func(p *corev1.Pod) { p.Spec.Containers[0].Env[0].Value = "/etc/other/config.json" }, + want: true, + why: "a changed env value must roll the pod", + }, + "config secret name changed": { + mutateDesired: func(p *corev1.Pod) { p.Spec.Volumes[0].Secret.SecretName = "other-config" }, + want: true, + why: "pointing at a different config secret must roll the pod", + }, + "service account changed": { + mutateDesired: func(p *corev1.Pod) { p.Spec.ServiceAccountName = "other-sa" }, + want: true, + why: "a changed service account must roll the pod", + }, + "nodeSelector key added": { + mutateDesired: func(p *corev1.Pod) { p.Spec.NodeSelector["pool"] = "listeners" }, + want: true, + why: "an added nodeSelector key must roll the pod", + }, + } + + for name, tc := range tests { + t.Run(name, func(t *testing.T) { + desired := desiredListenerPod() + live := livePodFromDesired(desired) + tc.mutateDesired(desired) + + assert.Equal(t, tc.want, listenerPodSpecRequiresRecreation(live, desired), tc.why) + }) + } + + t.Run("nil handling", func(t *testing.T) { + desired := desiredListenerPod() + assert.False(t, listenerPodSpecRequiresRecreation(nil, nil)) + assert.True(t, listenerPodSpecRequiresRecreation(nil, desired)) + assert.True(t, listenerPodSpecRequiresRecreation(desired, nil)) + }) +} + +// TestListenerPodSpecRequiresRecreation_KnownDeepDerivativeLimits documents, +// rather than asserts away, the removals DeepDerivative cannot see. These are +// all sourced from the user-facing listener template, so they are handled +// upstream: the AutoscalingRunnerSet controller compares the whole +// AutoscalingListener spec with cmp.Equal and deletes the listener, which +// deletes the pod. If that upstream behaviour ever changes to a derivative +// comparison, these become real bugs. +func TestListenerPodSpecRequiresRecreation_KnownDeepDerivativeLimits(t *testing.T) { + tests := map[string]func(*corev1.Pod){ + "all tolerations removed": func(p *corev1.Pod) { p.Spec.Tolerations = nil }, + "trailing volume removed": func(p *corev1.Pod) { + p.Spec.Volumes = p.Spec.Volumes[:len(p.Spec.Volumes)-1] + }, + "nodeSelector emptied": func(p *corev1.Pod) { p.Spec.NodeSelector = nil }, + "grace period removed": func(p *corev1.Pod) { p.Spec.TerminationGracePeriodSeconds = nil }, + } + + for name, mutate := range tests { + t.Run(name, func(t *testing.T) { + desired := desiredListenerPod() + live := livePodFromDesired(desired) + mutate(desired) + + assert.False(t, listenerPodSpecRequiresRecreation(live, desired), + "documented DeepDerivative limitation: removal is invisible here and is "+ + "instead handled by AutoscalingListener re-creation upstream") + }) + } +} + +// TestListenerPodSpecRequiresRecreation_MetricsToggleUsesRealBuilder exercises +// the drift check against genuine newScaleSetListenerPod output rather than a +// hand-written fixture, closing the gap between the fixture above and reality. +// +// metricsConfig is the one pod-spec input that does not come from any resource: +// it is derived from the --listener-metrics-addr controller flag. Because +// nothing in the AutoscalingListener spec changes when an operator disables +// metrics, the AutoscalingListener is not re-created and this comparison is the +// only thing that can remove the stale port from the running pod. +func TestListenerPodSpecRequiresRecreation_MetricsToggleUsesRealBuilder(t *testing.T) { + autoscalingRunnerSet := v1alpha1.AutoscalingRunnerSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-scale-set", + Namespace: "test-ns", + Labels: map[string]string{ + LabelKeyKubernetesPartOf: labelValueKubernetesPartOf, + LabelKeyKubernetesVersion: "0.2.0", + }, + Annotations: map[string]string{ + runnerScaleSetIDAnnotationKey: "1", + AnnotationKeyGitHubRunnerGroupName: "test-group", + AnnotationKeyGitHubRunnerScaleSetName: "test-scale-set", + }, + }, + Spec: v1alpha1.AutoscalingRunnerSetSpec{GitHubConfigUrl: "https://github.com/org/repo"}, + } + + cache := NewResourceCache() + b := ResourceBuilder{ResourceCache: &cache} + ephemeralRunnerSet, err := b.newEphemeralRunnerSet(&autoscalingRunnerSet) + require.NoError(t, err) + listener, err := b.newAutoscalingListener(&autoscalingRunnerSet, ephemeralRunnerSet, autoscalingRunnerSet.Namespace, "test:latest", nil) + require.NoError(t, err) + sa, err := b.newScaleSetListenerServiceAccount(listener) + require.NoError(t, err) + role := b.newScaleSetListenerRole(listener) + roleBinding := b.newScaleSetListenerRoleBinding(listener, role, sa) + + build := func(metrics *listenerMetricsServerConfig) *corev1.Pod { + // The cache keys on the listener object, not on metricsConfig, so it must + // be bypassed to build both variants. + b.ResourceCache.listenerPod.Delete(listener) + pod, err := b.newScaleSetListenerPod(listener, &corev1.Secret{}, sa, role, roleBinding, metrics) + require.NoError(t, err) + return pod + } + + withMetrics := build(&listenerMetricsServerConfig{addr: ":8080", endpoint: "/metrics"}) + withoutMetrics := build(nil) + + require.NotEmpty(t, withMetrics.Spec.Containers[0].Ports, + "builder should expose a container port when metrics are enabled") + require.Empty(t, withoutMetrics.Spec.Containers[0].Ports, + "builder should expose no container port when metrics are disabled") + + assert.True(t, listenerPodSpecRequiresRecreation(withMetrics, withoutMetrics), + "disabling --listener-metrics-addr must recreate a pod that still has the metrics port") + assert.True(t, listenerPodSpecRequiresRecreation(withoutMetrics, withMetrics), + "enabling --listener-metrics-addr must recreate a pod that has no metrics port") + assert.False(t, listenerPodSpecRequiresRecreation(withMetrics, withMetrics), + "an unchanged metrics configuration must not recreate the pod") +} + +// The listener pod mounts its config as a secret volume and parses it once at +// startup, so a change to the secret contents is invisible in the pod spec but +// still requires a restart to take effect. +func TestListenerPodSpecRequiresRecreation_ConfigSecretChanged(t *testing.T) { + tests := map[string]struct { + liveVersion string + desiredVersion string + want bool + why string + }{ + "unchanged": { + liveVersion: "100", + desiredVersion: "100", + want: false, + why: "the config the listener is running is still the desired one", + }, + "changed": { + liveVersion: "100", + desiredVersion: "101", + want: true, + why: "the listener only reads its config at startup, so it must be restarted", + }, + "missing on live pod": { + liveVersion: "", + desiredVersion: "101", + want: false, + why: "pods predating the annotation must not all be recreated on controller upgrade", + }, + } + + for name, tt := range tests { + t.Run(name, func(t *testing.T) { + desired := desiredListenerPod() + live := livePodFromDesired(desired) + + setListenerConfigVersion(live, tt.liveVersion) + setListenerConfigVersion(desired, tt.desiredVersion) + + assert.Equal(t, tt.want, listenerPodSpecRequiresRecreation(live, desired), tt.why) + }) + } +} + +func setListenerConfigVersion(pod *corev1.Pod, version string) { + if version == "" { + delete(pod.Annotations, AnnotationKeyListenerConfigResourceVersion) + return + } + if pod.Annotations == nil { + pod.Annotations = map[string]string{} + } + pod.Annotations[AnnotationKeyListenerConfigResourceVersion] = version +} diff --git a/controllers/actions.github.com/resourcebuilder.go b/controllers/actions.github.com/resourcebuilder.go index 961f7b9821..c78bc200fb 100644 --- a/controllers/actions.github.com/resourcebuilder.go +++ b/controllers/actions.github.com/resourcebuilder.go @@ -472,24 +472,16 @@ func (b *ResourceBuilder) newScaleSetListenerPod( Kind: "Pod", }, ObjectMeta: metav1.ObjectMeta{ - Name: autoscalingListener.Name, - Namespace: autoscalingListener.Namespace, - Labels: labels, - Annotations: make(map[string]string), + Name: autoscalingListener.Name, + Namespace: autoscalingListener.Namespace, + Labels: labels, + Annotations: map[string]string{ + AnnotationKeyListenerConfigResourceVersion: podConfig.ResourceVersion, + }, }, Spec: podSpec, } - newRunnerScaleSetListenerPod.Annotations[annotationKeyIntegrityHash] = scaleSetListenerPodIntegrity( - newRunnerScaleSetListenerPod, - autoscalingListener, - podConfig, - serviceAccount, - role, - roleBinding, - metricsConfig, - ) - if err := b.setControllerReference(autoscalingListener, newRunnerScaleSetListenerPod); err != nil { return nil, fmt.Errorf("failed to set controller reference for listener pod: %w", err) } @@ -502,38 +494,6 @@ func (b *ResourceBuilder) newScaleSetListenerPod( return newRunnerScaleSetListenerPod, nil } -func scaleSetListenerPodIntegrity( - pod *corev1.Pod, - autoscalingListener *v1alpha1.AutoscalingListener, - podConfig *corev1.Secret, - serviceAccount *corev1.ServiceAccount, - role *rbacv1.Role, - roleBinding *rbacv1.RoleBinding, - metricsConfig *listenerMetricsServerConfig, -) string { - type data struct { - ListenerPodSpec *corev1.PodSpec `json:"listenerPodSpec,omitempty"` - AutoscalingListenerIntegrityHash string `json:"autoscalingListenerIntegrityHash"` - ConfigSecretIntegrityHash string `json:"configSecretIntegrityHash"` - ServiceAccountIntegrityHash string `json:"serviceAccountIntegrityHash"` - RoleIntegrityHash string `json:"roleIntegrityHash"` - RoleBindingIntegrityHash string `json:"roleBindingIntegrityHash"` - MetricsConfig *listenerMetricsServerConfig `json:"metricsConfig,omitempty"` - } - - d := data{ - ListenerPodSpec: &pod.Spec, - AutoscalingListenerIntegrityHash: autoscalingListener.Annotations[annotationKeyIntegrityHash], - ConfigSecretIntegrityHash: podConfig.Annotations[annotationKeyIntegrityHash], - ServiceAccountIntegrityHash: serviceAccount.Annotations[annotationKeyIntegrityHash], - RoleIntegrityHash: role.Annotations[annotationKeyIntegrityHash], - RoleBindingIntegrityHash: roleBinding.Annotations[annotationKeyIntegrityHash], - MetricsConfig: metricsConfig, - } - - return hash.ComputeTemplateHash(&d) -} - func mergeListenerPodWithTemplate(pod *corev1.Pod, tmpl *corev1.PodTemplateSpec) { if pod.Annotations == nil { pod.Annotations = make(map[string]string) From 2c9c33f4cde8cb11db7e20b6c76f24d90dfce0ee Mon Sep 17 00:00:00 2001 From: Nikola Jokic Date: Thu, 10 Sep 2026 09:54:03 +0200 Subject: [PATCH 2/2] Recreate listener pods that predate the config version annotation Ignoring an absent annotation on the live pod was meant to spare every existing listener a restart when the controller is upgraded, but it quietly loses a configuration update. The reconcile that declines to recreate the pod carries on to the annotation merge a few lines further down and patches the desired annotations onto the live pod, so the resource version of a config secret the running listener never read is recorded as though it had been applied. Every later comparison then matches, and the listener serves the configuration it parsed at startup with nothing left to correct it: a scale set URL, TLS certificate or scaler setting changed around the time of an upgrade would simply never take effect. Treat the missing annotation as drift instead and accept the one-time rollout. It costs little, because deleting a listener does not disturb the runners it has already started, and an upgrade usually changes the listener image and so recreates the pod regardless. The desired side is still allowed to be empty, since there is nothing to compare against then, but that only happens in tests as the desired pod is always built from a secret read back from the API server. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- controllers/actions.github.com/helpers.go | 23 ++++++++++++++----- .../helpers_listener_test.go | 8 ++++++- 2 files changed, 24 insertions(+), 7 deletions(-) diff --git a/controllers/actions.github.com/helpers.go b/controllers/actions.github.com/helpers.go index 59ba87782b..9f2a22c224 100644 --- a/controllers/actions.github.com/helpers.go +++ b/controllers/actions.github.com/helpers.go @@ -14,9 +14,17 @@ import ( // listener after a restart. None of that is visible in the pod spec, which // references the secret by name and is byte-identical before and after, so the // desired pod carries the secret's resource version as an annotation and drift -// is detected by comparing it. An empty annotation on the live pod is ignored: -// pods created before this annotation existed would otherwise all be recreated -// on controller upgrade, and the next legitimate config change recreates them +// is detected by comparing it. +// +// A pod that predates this annotation counts as drift and is recreated once, on +// the first reconcile after the controller is upgraded. Ignoring it instead is +// tempting, to avoid that rollout, but it loses updates: the reconcile that +// declines to recreate the pod goes on to patch the desired annotations onto it, +// so a config change that landed while the old controller was running is +// recorded as already applied and the listener keeps serving the configuration +// it parsed at startup, with nothing to ever correct it. The rollout is cheap by +// comparison - deleting a listener does not disturb the runners it started, and +// an upgrade normally changes the listener image and so recreates the pod // anyway. // // The resource version also moves when only the secret's labels or annotations @@ -63,12 +71,15 @@ func listenerPodSpecRequiresRecreation(current, desired *corev1.Pod) bool { } func listenerConfigChanged(current, desired *corev1.Pod) bool { - currentVersion := current.Annotations[AnnotationKeyListenerConfigResourceVersion] desiredVersion := desired.Annotations[AnnotationKeyListenerConfigResourceVersion] - if currentVersion == "" || desiredVersion == "" { + if desiredVersion == "" { + // Nothing to compare against, so there is nothing this check can say. + // The desired pod is always built from a secret read back from the API + // server, so this only happens in tests. return false } - return currentVersion != desiredVersion + + return current.Annotations[AnnotationKeyListenerConfigResourceVersion] != desiredVersion } func listenerContainerPortsRemoved(current, desired *corev1.Pod) bool { diff --git a/controllers/actions.github.com/helpers_listener_test.go b/controllers/actions.github.com/helpers_listener_test.go index ecd4044680..8712b46923 100644 --- a/controllers/actions.github.com/helpers_listener_test.go +++ b/controllers/actions.github.com/helpers_listener_test.go @@ -301,8 +301,14 @@ func TestListenerPodSpecRequiresRecreation_ConfigSecretChanged(t *testing.T) { "missing on live pod": { liveVersion: "", desiredVersion: "101", + want: true, + why: "a pod predating the annotation is recreated once, because otherwise the reconcile that spares it patches the annotation on and the config change is lost for good", + }, + "missing on desired pod": { + liveVersion: "100", + desiredVersion: "", want: false, - why: "pods predating the annotation must not all be recreated on controller upgrade", + why: "there is nothing to compare against, so this check must not claim drift", }, }