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

@@ -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) }

pkg/pod/entrypoint_test.go

@@ -168,32 +168,39 @@ 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 {
 		desc           string
 		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)

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 {