diff --git a/controllers/actions.github.com/helpers.go b/controllers/actions.github.com/helpers.go index a0699682..12aa76d5 100644 --- a/controllers/actions.github.com/helpers.go +++ b/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 } diff --git a/controllers/actions.github.com/helpers_listener_test.go b/controllers/actions.github.com/helpers_listener_test.go index 8712b469..13e8e461 100644 --- a/controllers/actions.github.com/helpers_listener_test.go +++ b/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