From f22def86e0404c353b7fc29788491a5d90d10185 Mon Sep 17 00:00:00 2001 From: Nikola Jokic Date: Wed, 9 Sep 2026 10:41:31 +0200 Subject: [PATCH] Recreate the listener pod when its config secret changes Replacing the integrity hash with a pod spec comparison lost the one signal the spec cannot carry. The listener mounts its config as a secret volume and 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. The pod references the secret by name, so the spec is byte-identical before and after and the pod was never recreated. The desired pod now carries the config secret's resource version as an annotation, which is free to read and moves exactly when the secret is written. An empty annotation on the live pod is ignored so that pods created by an older controller are not all recreated on upgrade. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- controllers/actions.github.com/constants.go | 7 +++ controllers/actions.github.com/helpers.go | 30 +++++++++++ .../helpers_listener_test.go | 54 +++++++++++++++++++ .../actions.github.com/resourcebuilder.go | 10 ++-- 4 files changed, 97 insertions(+), 4 deletions(-) diff --git a/controllers/actions.github.com/constants.go b/controllers/actions.github.com/constants.go index 3c523e9d..af05eaf7 100644 --- a/controllers/actions.github.com/constants.go +++ b/controllers/actions.github.com/constants.go @@ -56,6 +56,13 @@ const ( // runner spec from one that reported it against a spec that has since been // updated. AnnotationKeyActionableRevision = "actions.github.com/actionable-revision" + // 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 index e96b0621..27c26613 100644 --- a/controllers/actions.github.com/helpers.go +++ b/controllers/actions.github.com/helpers.go @@ -66,6 +66,23 @@ func ephemeralRunnerSetOutdatedForAppliedRevision(ephemeralRunnerSet *v1alpha1.E // 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, @@ -92,6 +109,10 @@ func listenerPodSpecRequiresRecreation(current, desired *corev1.Pod) bool { return current != desired } + if listenerConfigChanged(current, desired) { + return true + } + if listenerContainerPortsRemoved(current, desired) { return true } @@ -99,6 +120,15 @@ func listenerPodSpecRequiresRecreation(current, desired *corev1.Pod) bool { 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] diff --git a/controllers/actions.github.com/helpers_listener_test.go b/controllers/actions.github.com/helpers_listener_test.go index 89384a03..ecd40446 100644 --- a/controllers/actions.github.com/helpers_listener_test.go +++ b/controllers/actions.github.com/helpers_listener_test.go @@ -275,3 +275,57 @@ func TestListenerPodSpecRequiresRecreation_MetricsToggleUsesRealBuilder(t *testi 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 9a6776d3..ea092aab 100644 --- a/controllers/actions.github.com/resourcebuilder.go +++ b/controllers/actions.github.com/resourcebuilder.go @@ -447,10 +447,12 @@ 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, }