Exclude stopped injected sidecars from TaskRun status

fixes tektoncd#7640

In tektoncd#5565, we started stopping injected sidecars with `nop` in the same way we stop explicitly defined sidecar containers. `MakeTaskRunStatus` was updated to only include explicitly defined sidecars in the `TaskRun` status, rather than just any container in the `TaskRun` pod that doesn't start with `step-`, so while the pod is running, the injected sidecar doesn't show up in the status. However, once the pod has completed and the sidecars are stopped, if the `TaskRun`'s spec contains a sidecar, `updateStoppedSidecarStatus` will be called, and that function's logic for what containers to include in `TaskRun.Status.Sidecars` is still including everything but `step-`-prefixed containers. That should be updated to behave in the same way as `MakeTaskRunStatus`.

Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>

pkg/pod/entrypoint.go

@@ -338,9 +338,9 @@ func IsSidecarStatusRunning(tr *v1.TaskRun) bool {
 // represents a step.
 func IsContainerStep(name string) bool { return strings.HasPrefix(name, stepPrefix) }
 
-// isContainerSidecar returns true if the container name indicates that it
+// IsContainerSidecar returns true if the container name indicates that it
 // represents a sidecar.
-func isContainerSidecar(name string) bool { return strings.HasPrefix(name, sidecarPrefix) }
+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) }

pkg/pod/status.go

@@ -147,7 +147,7 @@ func MakeTaskRunStatus(ctx context.Context, logger *zap.SugaredLogger, tr v1.Tas
  for _, s := range pod.Status.ContainerStatuses {
  if IsContainerStep(s.Name) {
  stepStatuses = append(stepStatuses, s)
- } else if isContainerSidecar(s.Name) {
+ } else if IsContainerSidecar(s.Name) {
  sidecarStatuses = append(sidecarStatuses, s)
  }
  }

pkg/reconciler/taskrun/taskrun.go

@@ -914,7 +914,7 @@ func isPodAdmissionFailed(err error) bool {
 func updateStoppedSidecarStatus(pod *corev1.Pod, tr *v1.TaskRun) error {
  tr.Status.Sidecars = []v1.SidecarState{}
  for _, s := range pod.Status.ContainerStatuses {
- if !podconvert.IsContainerStep(s.Name) {
+ if podconvert.IsContainerSidecar(s.Name) {
  var sidecarState corev1.ContainerState
  if s.LastTerminationState.Terminated != nil {
  // Sidecar has successfully by terminated by nop image

pkg/reconciler/taskrun/taskrun_test.go

@@ -5367,20 +5367,36 @@ status:
 }
 
 func TestStopSidecars_WithInjectedSidecarsNoTaskSpecSidecars(t *testing.T) {
+ sidecarTask := &v1.Task{
+ ObjectMeta: objectMeta("test-task-injected-sidecar", "foo"),
+ Spec: v1.TaskSpec{
+ Steps: []v1.Step{simpleStep},
+ Sidecars: []v1.Sidecar{{
+ Name: "sidecar1",
+ Image: "image-id",
+ }},
+ },
+ }
+
  taskRun := parse.MustParseV1TaskRun(t, `
 metadata:
  name: test-taskrun-injected-sidecars
  namespace: foo
 spec:
  taskRef:
- name: test-task
+ name: test-task-injected-sidecar
 status:
  podName: test-taskrun-injected-sidecars-pod
  conditions:
  - message: Build succeeded
  reason: Build succeeded
  status: "True"
  type: Succeeded
+ sidecars:
+ - name: sidecar1
+ container: sidecar-sidecar1
+ running:
+ startedAt: "2000-01-01T01:01:01Z"
 `)
 
  pod := &corev1.Pod{
@@ -5394,6 +5410,10 @@ status:
  Name: "step-do-something",
  Image: "my-step-image",
  },
+ {
+ Name: "sidecar1",
+ Image: "image-id",
+ },
  {
  Name: "injected-sidecar",
  Image: "some-image",
@@ -5407,6 +5427,10 @@ status:
  Name: "step-do-something",
  State: corev1.ContainerState{Terminated: &corev1.ContainerStateTerminated{}},
  },
+ {
+ Name: "sidecar-sidecar1",
+ State: corev1.ContainerState{Running: &corev1.ContainerStateRunning{}},
+ },
  {
  Name: "injected-sidecar",
  State: corev1.ContainerState{Running: &corev1.ContainerStateRunning{}},
@@ -5418,7 +5442,7 @@ status:
  d := test.Data{
  Pods: []*corev1.Pod{pod},
  TaskRuns: []*v1.TaskRun{taskRun},
- Tasks: []*v1.Task{simpleTask},
+ Tasks: []*v1.Task{sidecarTask},
  }
 
  testAssets, cancel := getTaskRunController(t, d)
@@ -5438,14 +5462,27 @@ status:
  t.Fatalf("error retrieving pod: %s", err)
  }
 
- if len(retrievedPod.Spec.Containers) != 2 {
+ if len(retrievedPod.Spec.Containers) != 3 {
  t.Fatalf("expected pod with two containers")
  }
 
  // check that injected sidecar is replaced with nop image
- if d := cmp.Diff(images.NopImage, retrievedPod.Spec.Containers[1].Image); d != "" {
+ if d := cmp.Diff(images.NopImage, retrievedPod.Spec.Containers[2].Image); d != "" {
  t.Errorf("expected injected sidecar image to be replaced with nop image %s", diff.PrintWantGot(d))
  }
+
+ // Get the updated TaskRun.
+ reconciledRun, err := clients.Pipeline.TektonV1().TaskRuns(taskRun.Namespace).Get(testAssets.Ctx, taskRun.Name, metav1.GetOptions{})
+ if err != nil {
+ t.Fatalf("Error getting updated TaskRun after reconcile: %v", err)
+ }
+
+ // Verify that the injected sidecar isn't present in the TaskRun's status.
+ for _, sc := range reconciledRun.Status.Sidecars {
+ if sc.Container == "injected-sidecar" {
+ t.Errorf("expected not to find injected-sidecar in TaskRun status, but found %v", sc)
+ }
+ }
 }
 
 func Test_validateTaskSpecRequestResources_ValidResources(t *testing.T) {