From 04fc509d7ddb27d176162654527da532c6652235 Mon Sep 17 00:00:00 2001 From: Jason Hall Date: Wed, 4 Dec 2019 16:19:10 -0500 Subject: [PATCH] Handle injected sidecars more correctly Before this change, sidecars that are injected into the Pod by external Mutating Admission Controllers would not be guaranteed to be Ready before starting the first step, and would not be correctly stopped when steps finish. This is because we only considered containers with names starting with "sidecar-" as sidecars, and injected sidecars might have another name. Instead, with this change, we look for any *non-step* container, which must (currently) indicate a sidecar container, when waiting for or stopping sidecars. An added test covers this new behavior, which would have failed before this change. --- pkg/pod/entrypoint.go | 15 ++++++++---- pkg/pod/entrypoint_test.go | 47 +++++++++++++++++++++++++------------- pkg/pod/status.go | 6 ++++- 3 files changed, 47 insertions(+), 21 deletions(-) diff --git a/pkg/pod/entrypoint.go b/pkg/pod/entrypoint.go index 2efbcc0b7ea..1dbf25258da 100644 --- a/pkg/pod/entrypoint.go +++ b/pkg/pod/entrypoint.go @@ -169,7 +169,11 @@ func StopSidecars(nopImage string, kubeclient kubernetes.Interface, pod corev1.P updated := false if newPod.Status.Phase == corev1.PodRunning { for _, s := range newPod.Status.ContainerStatuses { - if isContainerSidecar(s.Name) && s.State.Running != nil { + // Stop any running container that isn't a step. + // An injected sidecar container might not have the + // "sidecar-" prefix, so we can't just look for that + // prefix. + if !isContainerStep(s.Name) && s.State.Running != nil { for j, c := range newPod.Spec.Containers { if c.Name == s.Name && c.Image != nopImage { updated = true @@ -187,14 +191,17 @@ func StopSidecars(nopImage string, kubeclient kubernetes.Interface, pod corev1.P return nil } -// isContainerStep returns true if the container name indicates that it represents a step. +// isContainerStep returns true if the container name indicates that it +// represents a step. func isContainerStep(name string) bool { return strings.HasPrefix(name, stepPrefix) } -// isContainerSidecar returns true if the container name indicates that it represents a sidecar. +// isContainerSidecar returns true if the container name indicates that it +// represents a sidecar. func isContainerSidecar(name string) bool { return strings.HasPrefix(name, sidecarPrefix) } // trimStepPrefix returns the container name, stripped of its step prefix. func trimStepPrefix(name string) string { return strings.TrimPrefix(name, stepPrefix) } -// trimSidecarPrefix returns the container name, stripped of its sidecar prefix. +// trimSidecarPrefix returns the container name, stripped of its sidecar +// prefix. func trimSidecarPrefix(name string) string { return strings.TrimPrefix(name, sidecarPrefix) } diff --git a/pkg/pod/entrypoint_test.go b/pkg/pod/entrypoint_test.go index e627aedfe7f..3b22b4aad86 100644 --- a/pkg/pod/entrypoint_test.go +++ b/pkg/pod/entrypoint_test.go @@ -168,18 +168,25 @@ func TestStopSidecars(t *testing.T) { Image: "foo", } sidecarContainer := corev1.Container{ - Name: sidecarPrefix + "my-sidecar", - Image: "original-image", - Command: []string{"my", "command"}, - Args: []string{"my", "args"}, - Env: []corev1.EnvVar{{Name: "FOO", Value: "bar"}}, + Name: sidecarPrefix + "my-sidecar", + Image: "original-image", } stoppedSidecarContainer := corev1.Container{ - Name: sidecarContainer.Name, - Image: nopImage, - Command: []string{"my", "command"}, - Args: []string{"my", "args"}, - Env: []corev1.EnvVar{{Name: "FOO", Value: "bar"}}, + Name: sidecarContainer.Name, + Image: nopImage, + } + + // This is a container that doesn't start with the "sidecar-" prefix, + // which indicates it was injected into the Pod by a Mutating Webhook + // Admission Controller. Injected sidecars should also be stopped if + // they're running. + injectedSidecar := corev1.Container{ + Name: "injected", + Image: "original-injected-image", + } + stoppedInjectedSidecar := corev1.Container{ + Name: injectedSidecar.Name, + Image: nopImage, } for _, c := range []struct { @@ -187,13 +194,13 @@ func TestStopSidecars(t *testing.T) { pod corev1.Pod wantContainers []corev1.Container }{{ - desc: "Running sidecar should be stopped", + desc: "Running sidecars (incl injected) should be stopped", pod: corev1.Pod{ ObjectMeta: metav1.ObjectMeta{ Name: "test-pod", }, Spec: corev1.PodSpec{ - Containers: []corev1.Container{stepContainer, sidecarContainer}, + Containers: []corev1.Container{stepContainer, sidecarContainer, injectedSidecar}, }, Status: corev1.PodStatus{ Phase: corev1.PodRunning, @@ -203,10 +210,14 @@ func TestStopSidecars(t *testing.T) { Name: sidecarContainer.Name, // Sidecar is running. State: corev1.ContainerState{Running: &corev1.ContainerStateRunning{StartedAt: metav1.NewTime(time.Now())}}, + }, { + Name: injectedSidecar.Name, + // Injected sidecar is running. + State: corev1.ContainerState{Running: &corev1.ContainerStateRunning{StartedAt: metav1.NewTime(time.Now())}}, }}, }, }, - wantContainers: []corev1.Container{stepContainer, stoppedSidecarContainer}, + wantContainers: []corev1.Container{stepContainer, stoppedSidecarContainer, stoppedInjectedSidecar}, }, { desc: "Pending Pod should not be updated", pod: corev1.Pod{ @@ -221,13 +232,13 @@ func TestStopSidecars(t *testing.T) { }, wantContainers: []corev1.Container{stepContainer, sidecarContainer}, }, { - desc: "Non-Running sidecar should not be stopped", + desc: "Non-Running sidecars should not be stopped", pod: corev1.Pod{ ObjectMeta: metav1.ObjectMeta{ Name: "test-pod", }, Spec: corev1.PodSpec{ - Containers: []corev1.Container{stepContainer, sidecarContainer}, + Containers: []corev1.Container{stepContainer, sidecarContainer, injectedSidecar}, }, Status: corev1.PodStatus{ Phase: corev1.PodRunning, @@ -237,10 +248,14 @@ func TestStopSidecars(t *testing.T) { Name: sidecarContainer.Name, // Sidecar is waiting. State: corev1.ContainerState{Waiting: &corev1.ContainerStateWaiting{}}, + }, { + Name: injectedSidecar.Name, + // Injected sidecar is waiting. + State: corev1.ContainerState{Waiting: &corev1.ContainerStateWaiting{}}, }}, }, }, - wantContainers: []corev1.Container{stepContainer, sidecarContainer}, + wantContainers: []corev1.Container{stepContainer, sidecarContainer, injectedSidecar}, }} { t.Run(c.desc, func(t *testing.T) { kubeclient := fakek8s.NewSimpleClientset(&c.pod) diff --git a/pkg/pod/status.go b/pkg/pod/status.go index b1639502631..c04044549d0 100644 --- a/pkg/pod/status.go +++ b/pkg/pod/status.go @@ -71,7 +71,11 @@ func SidecarsReady(podStatus corev1.PodStatus) bool { return false } for _, s := range podStatus.ContainerStatuses { - if !isContainerSidecar(s.Name) { + // If the step indicates that it's a step, skip it. + // An injected sidecar might not have the "sidecar-" prefix, so + // we can't just look for that prefix, we need to look at any + // non-step container. + if isContainerStep(s.Name) { continue } if s.State.Running != nil && s.Ready {