Always call podconvert.StopSidecars to handle injected sidecars

Injected sidecars are only cleaned up if sidecars are defined in the
task spec because of a check that exits early if the TaskRun does not
include any sidecars. This breaks users that are running in environments
with injected sidecars, but are not defining sidecars in their TaskRuns.

This change will remove the early exit condition when there are no sidecars
defined on the TaskRun Spec. This will cause the podconvert.StopSidecars
method to run every time, properly cleaning up sidecars the TaskRun doesn't
know about.

Resolves #4731

pkg/reconciler/taskrun/taskrun.go

@@ -253,11 +253,6 @@ func (c *Reconciler) stopSidecars(ctx context.Context, tr *v1beta1.TaskRun) erro
 		return nil
 	}
 
-	// do not continue if the TaskSpec had no sidecars
-	if tr.Status.TaskSpec != nil && len(tr.Status.TaskSpec.Sidecars) == 0 && len(tr.Status.Sidecars) == 0 {
-		return nil
-	}
-
 	// do not continue if the TaskRun was canceled or timed out as this caused the pod to be deleted in failTaskRun
 	condition := tr.Status.GetCondition(apis.ConditionSucceeded)
 	if condition != nil {
@@ -267,11 +262,6 @@ func (c *Reconciler) stopSidecars(ctx context.Context, tr *v1beta1.TaskRun) erro
 		}
 	}
 
-	// do not continue if there are no Running sidecars.
-	if !podconvert.IsSidecarStatusRunning(tr) {
-		return nil
-	}
-
 	pod, err := podconvert.StopSidecars(ctx, c.Images.NopImage, c.KubeClientSet, tr.Namespace, tr.Status.PodName)
 	if err == nil {
 		// Check if any SidecarStatuses are still shown as Running after stopping

pkg/reconciler/taskrun/taskrun_test.go

@@ -4078,65 +4078,85 @@ status:
 	}
 }
 
-func TestStopSidecars_NoClientGetPodForTaskSpecWithoutRunningSidecars(t *testing.T) {
-	for _, tc := range []struct {
-		desc string
-		tr   *v1beta1.TaskRun
-	}{{
-		desc: "no sidecars",
-		tr: parse.MustParseV1beta1TaskRun(t, `
-metadata:
-  name: test-taskrun
-  namespace: foo
-status:
-  conditions:
-  - status: "True"
-    type: Succeeded
-  podName: test-taskrun-pod
-  startTime: "2000-01-01T01:01:01Z"
-`),
-	}, {
-		desc: "sidecars are terminated",
-		tr: parse.MustParseV1beta1TaskRun(t, `
+func TestStopSidecars_WithInjectedSidecarsNoTaskSpecSidecars(t *testing.T) {
+	taskRun := parse.MustParseV1beta1TaskRun(t, `
 metadata:
-  name: test-taskrun
+  name: test-taskrun-injected-sidecars
   namespace: foo
+spec:
+  taskRef:
+    name: test-task
 status:
+  podName: test-taskrun-injected-sidecars-pod
   conditions:
-  - status: "True"
+  - message: Build succeeded
+    reason: Build succeeded
+    status: "True"
     type: Succeeded
-  podName: test-taskrun-pod
-  sidecars:
-  - terminated:
-      exitCode: 0
-      finishedAt: "2000-01-01T01:01:01Z"
-      startedAt: "2000-01-01T01:01:01Z"
-  startTime: "2000-01-01T01:01:01Z"
-`),
-	}} {
-		t.Run(tc.desc, func(t *testing.T) {
-			d := test.Data{
-				TaskRuns: []*v1beta1.TaskRun{tc.tr},
-			}
+`)
 
-			testAssets, cancel := getTaskRunController(t, d)
-			defer cancel()
-			c := testAssets.Controller
-			clients := testAssets.Clients
-			reconcileErr := c.Reconciler.Reconcile(testAssets.Ctx, getRunName(tc.tr))
+	pod := &corev1.Pod{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "test-taskrun-injected-sidecars-pod",
+			Namespace: "foo",
+		},
+		Spec: corev1.PodSpec{
+			Containers: []corev1.Container{
+				{
+					Name:  "step-do-something",
+					Image: "my-step-image",
+				},
+				{
+					Name:  "injected-sidecar",
+					Image: "some-image",
+				},
+			},
+		},
+		Status: corev1.PodStatus{
+			Phase: corev1.PodRunning,
+			ContainerStatuses: []corev1.ContainerStatus{
+				{
+					Name:  "step-do-something",
+					State: v1.ContainerState{Terminated: &corev1.ContainerStateTerminated{}},
+				},
+				{
+					Name:  "injected-sidecar",
+					State: v1.ContainerState{Running: &corev1.ContainerStateRunning{}},
+				},
+			},
+		},
+	}
 
-			// We do not expect an error
-			if reconcileErr != nil {
-				t.Errorf("Expected no error to be returned by reconciler: %v", reconcileErr)
-			}
+	d := test.Data{
+		Pods:     []*corev1.Pod{pod},
+		TaskRuns: []*v1beta1.TaskRun{taskRun},
+		Tasks:    []*v1beta1.Task{simpleTask},
+	}
 
-			// Verify that the pod was not retrieved
-			for _, action := range clients.Kube.Actions() {
-				if action.Matches("get", "pods") {
-					t.Errorf("expected the pod not to be retrieved because the TaskRun has no sidecars")
-				}
-			}
-		})
+	testAssets, cancel := getTaskRunController(t, d)
+	defer cancel()
+	c := testAssets.Controller
+	clients := testAssets.Clients
+
+	reconcileErr := c.Reconciler.Reconcile(testAssets.Ctx, getRunName(taskRun))
+
+	// We do not expect an error
+	if reconcileErr != nil {
+		t.Errorf("Expected no error to be returned by reconciler: %v", reconcileErr)
+	}
+
+	retrievedPod, err := clients.Kube.CoreV1().Pods(pod.Namespace).Get(testAssets.Ctx, pod.Name, metav1.GetOptions{})
+	if err != nil {
+		t.Errorf("error retrieving pod: %s", err)
+	}
+
+	if len(retrievedPod.Spec.Containers) < 2 {
+		t.Errorf("expected pod with two containers")
+	}
+
+	//check that injected sidecar is replaced with nop image
+	if d := cmp.Diff(retrievedPod.Spec.Containers[1].Image, images.NopImage); d != "" {
+		t.Errorf("expected injected sidecar image to be replaced with nop image %s", diff.PrintWantGot(d))
 	}
 }