Exclude stopped injected sidecars from TaskRun status

fixes #7640 In #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 <[email protected]>
tektoncd · Feb 8, 2024 · 7ae602c · 7ae602c
1 parent f1bf59c
commit 7ae602c
Show file tree

Hide file tree

Showing 4 changed files with 45 additions and 8 deletions.
diff --git a/pkg/pod/entrypoint.go b/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) }

diff --git a/pkg/pod/status.go b/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)
 		}
 	}

diff --git a/pkg/reconciler/taskrun/taskrun.go b/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

diff --git a/pkg/reconciler/taskrun/taskrun_test.go b/pkg/reconciler/taskrun/taskrun_test.go
@@ -5379,20 +5379,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{
@@ -5406,6 +5422,10 @@ status:
 					Name:  "step-do-something",
 					Image: "my-step-image",
 				},
+				{
+					Name:  "sidecar1",
+					Image: "image-id",
+				},
 				{
 					Name:  "injected-sidecar",
 					Image: "some-image",
@@ -5419,6 +5439,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{}},
@@ -5430,7 +5454,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)
@@ -5450,14 +5474,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) {