Avoid listener pod recreation for prepended sidecars (#4685)

controllers/actions.github.com/helpers.go

@@ -131,6 +131,11 @@ func ephemeralRunnerSetOutdatedForAppliedRevision(ephemeralRunnerSet *v1alpha1.E
 // defaults either, since nodeName is scheduler-assigned and the access-token
 // volume has a generated name. See TestListenerPodSpecRequiresRecreation.
 //
+// Regular containers are matched by name separately: DeepDerivative compares
+// slices by position, so an admission-injected sidecar before the listener would
+// otherwise look like drift. Extra live containers are ignored, while every
+// desired container must still match. Init-container ordering remains significant.
+//
 // 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
@@ -151,11 +156,13 @@ func listenerPodSpecRequiresRecreation(current, desired *corev1.Pod) bool {
 		return true
 	}
 
-	if listenerContainerPortsRemoved(current, desired) {
+	if listenerContainersChanged(current, desired) {
 		return true
 	}
 
-	return !apiequality.Semantic.DeepDerivative(desired.Spec, current.Spec)
+	currentSpec, desiredSpec := current.Spec, desired.Spec
+	currentSpec.Containers, desiredSpec.Containers = nil, nil
+	return !apiequality.Semantic.DeepDerivative(desiredSpec, currentSpec)
 }
 
 func listenerConfigChanged(current, desired *corev1.Pod) bool {
@@ -170,18 +177,19 @@ func listenerConfigChanged(current, desired *corev1.Pod) bool {
 	return current.Annotations[AnnotationKeyListenerConfigResourceVersion] != desiredVersion
 }
 
-func listenerContainerPortsRemoved(current, desired *corev1.Pod) bool {
+func listenerContainersChanged(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
+			return true
 		}
 		if len(desiredContainer.Ports) < len(currentContainer.Ports) {
 			return true
 		}
+		if !apiequality.Semantic.DeepDerivative(*desiredContainer, *currentContainer) {
+			return true
+		}
 	}
 	return false
 }

controllers/actions.github.com/helpers_listener_test.go

@@ -167,11 +167,22 @@ func TestListenerPodSpecRequiresRecreation(t *testing.T) {
 
 	for name, tc := range tests {
 		t.Run(name, func(t *testing.T) {
-			desired := desiredListenerPod()
-			live := livePodFromDesired(desired)
-			tc.mutateDesired(desired)
+			for _, position := range []string{"no sidecar", "prepended sidecar", "appended sidecar"} {
+				t.Run(position, func(t *testing.T) {
+					desired := desiredListenerPod()
+					live := livePodFromDesired(desired)
+					sidecar := corev1.Container{Name: "injected", Image: "sidecar:latest"}
+					switch position {
+					case "prepended sidecar":
+						live.Spec.Containers = append([]corev1.Container{sidecar}, live.Spec.Containers...)
+					case "appended sidecar":
+						live.Spec.Containers = append(live.Spec.Containers, sidecar)
+					}
+					tc.mutateDesired(desired)
 
-			assert.Equal(t, tc.want, listenerPodSpecRequiresRecreation(live, desired), tc.why)
+					assert.Equal(t, tc.want, listenerPodSpecRequiresRecreation(live, desired), tc.why)
+				})
+			}
 		})
 	}
 
@@ -183,6 +194,117 @@ func TestListenerPodSpecRequiresRecreation(t *testing.T) {
 	})
 }
 
+func TestListenerPodSpecRequiresRecreation_Containers(t *testing.T) {
+	injected := corev1.Container{Name: "injected", Image: "sidecar:latest"}
+	tests := map[string]struct {
+		mutateLive func(*corev1.Pod)
+		want       bool
+	}{
+		"unchanged": {
+			mutateLive: func(*corev1.Pod) {},
+		},
+		"injected before desired containers": {
+			mutateLive: func(p *corev1.Pod) {
+				p.Spec.Containers = append([]corev1.Container{injected}, p.Spec.Containers...)
+			},
+		},
+		"injected between desired containers": {
+			mutateLive: func(p *corev1.Pod) {
+				p.Spec.Containers = []corev1.Container{p.Spec.Containers[0], injected, p.Spec.Containers[1]}
+			},
+		},
+		"injected after desired containers": {
+			mutateLive: func(p *corev1.Pod) {
+				p.Spec.Containers = append(p.Spec.Containers, injected)
+			},
+		},
+		"desired containers reordered": {
+			mutateLive: func(p *corev1.Pod) {
+				p.Spec.Containers[0], p.Spec.Containers[1] = p.Spec.Containers[1], p.Spec.Containers[0]
+			},
+		},
+		"desired containers reordered with injection": {
+			mutateLive: func(p *corev1.Pod) {
+				p.Spec.Containers = []corev1.Container{injected, p.Spec.Containers[1], p.Spec.Containers[0]}
+			},
+		},
+		"missing listener": {
+			mutateLive: func(p *corev1.Pod) {
+				p.Spec.Containers = p.Spec.Containers[1:]
+			},
+			want: true,
+		},
+		"missing template sidecar": {
+			mutateLive: func(p *corev1.Pod) {
+				p.Spec.Containers = []corev1.Container{p.Spec.Containers[0], injected}
+			},
+			want: true,
+		},
+		"missing all containers": {
+			mutateLive: func(p *corev1.Pod) {
+				p.Spec.Containers = nil
+			},
+			want: true,
+		},
+		"renamed listener": {
+			mutateLive: func(p *corev1.Pod) {
+				p.Spec.Containers[0].Name = "not-listener"
+			},
+			want: true,
+		},
+		"template sidecar image changed": {
+			mutateLive: func(p *corev1.Pod) {
+				p.Spec.Containers[1].Image = "sidecar:old"
+			},
+			want: true,
+		},
+		"template sidecar defaulted": {
+			mutateLive: func(p *corev1.Pod) {
+				p.Spec.Containers[1].ImagePullPolicy = corev1.PullIfNotPresent
+				p.Spec.Containers[1].TerminationMessagePath = corev1.TerminationMessagePathDefault
+			},
+		},
+		"template sidecar command reordered": {
+			mutateLive: func(p *corev1.Pod) {
+				p.Spec.Containers[1].Command = []string{"60", "sleep"}
+			},
+			want: true,
+		},
+		"init containers reordered": {
+			mutateLive: func(p *corev1.Pod) {
+				p.Spec.InitContainers[0], p.Spec.InitContainers[1] = p.Spec.InitContainers[1], p.Spec.InitContainers[0]
+			},
+			want: true,
+		},
+		"init container image changed": {
+			mutateLive: func(p *corev1.Pod) {
+				p.Spec.InitContainers[0].Image = "init:old"
+			},
+			want: true,
+		},
+	}
+
+	for name, tc := range tests {
+		t.Run(name, func(t *testing.T) {
+			desired := desiredListenerPod()
+			desired.Spec.Containers = append(desired.Spec.Containers, corev1.Container{
+				Name: "template-sidecar", Image: "sidecar:latest", Command: []string{"sleep", "60"},
+			})
+			desired.Spec.InitContainers = []corev1.Container{
+				{Name: "first", Image: "init:latest"},
+				{Name: "second", Image: "init:latest"},
+			}
+			live := livePodFromDesired(desired)
+			tc.mutateLive(live)
+			originalDesired, originalLive := desired.DeepCopy(), live.DeepCopy()
+
+			assert.Equal(t, tc.want, listenerPodSpecRequiresRecreation(live, desired))
+			assert.Equal(t, originalDesired, desired, "comparison must not mutate the cached desired pod")
+			assert.Equal(t, originalLive, live, "comparison must not mutate the live pod")
+		})
+	}
+}
+
 // 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