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, }