From 4aff897d2fda7457e07881a08a58fce0c46851f6 Mon Sep 17 00:00:00 2001
From: Kewei Yang <cug.ykw@gmail.com>
Date: Mon, 9 Oct 2023 20:48:32 +0800
Subject: [PATCH] fix: panic may occur when calculating the final task timeout
 waiting time

---
 pkg/reconciler/pipelinerun/pipelinerun.go     |  2 +-
 .../pipelinerun/pipelinerun_test.go           | 85 +++++++++++++++----
 2 files changed, 69 insertions(+), 18 deletions(-)

diff --git a/pkg/reconciler/pipelinerun/pipelinerun.go b/pkg/reconciler/pipelinerun/pipelinerun.go
index 15fb97aa1c6..de8eef0d2a7 100644
--- a/pkg/reconciler/pipelinerun/pipelinerun.go
+++ b/pkg/reconciler/pipelinerun/pipelinerun.go
@@ -276,7 +276,7 @@ func (c *Reconciler) ReconcileKind(ctx context.Context, pr *v1.PipelineRun) pkgr
 		waitTime := pr.PipelineTimeout(ctx) - elapsed
 		if pr.Status.FinallyStartTime == nil && pr.TasksTimeout() != nil {
 			waitTime = pr.TasksTimeout().Duration - elapsed
-		} else if pr.FinallyTimeout() != nil {
+		} else if pr.Status.FinallyStartTime != nil && pr.FinallyTimeout() != nil {
 			finallyWaitTime := pr.FinallyTimeout().Duration - c.Clock.Since(pr.Status.FinallyStartTime.Time)
 			if finallyWaitTime < waitTime {
 				waitTime = finallyWaitTime
diff --git a/pkg/reconciler/pipelinerun/pipelinerun_test.go b/pkg/reconciler/pipelinerun/pipelinerun_test.go
index f61d0cd0e5d..68338d5cab8 100644
--- a/pkg/reconciler/pipelinerun/pipelinerun_test.go
+++ b/pkg/reconciler/pipelinerun/pipelinerun_test.go
@@ -2691,7 +2691,7 @@ spec:
 `)}
 	prName := "test-pipeline-run-with-timeout"
 	ts := []*v1.Task{simpleHelloWorldTask}
-	trs := []*v1.TaskRun{
+	oneFinishedTRs := []*v1.TaskRun{
 		getTaskRun(
 			t,
 			"test-pipeline-run-with-timeout-hello-world",
@@ -2708,12 +2708,26 @@ spec:
     name: hello-world
     kind: Task
 `)}
+	oneStartedTRs := []*v1.TaskRun{
+		getTaskRun(
+			t,
+			"test-pipeline-run-with-timeout-hello-world",
+			prName,
+			ps[0].Name,
+			"hello-world",
+			corev1.ConditionUnknown,
+		),
+	}
 
 	tcs := []struct {
-		name string
-		pr   *v1.PipelineRun
+		name                    string
+		trs                     []*v1.TaskRun
+		pr                      *v1.PipelineRun
+		wantFinallyTimeout      bool
+		wantOneFinalTaskSkipped bool
 	}{{
 		name: "finally timeout specified",
+		trs:  oneFinishedTRs,
 		pr: parse.MustParseV1PipelineRun(t, fmt.Sprintf(`
 metadata:
   name: %s
@@ -2746,8 +2760,11 @@ status:
         status: "Unknown"
         type: Succeeded
 `, prName)),
+		wantFinallyTimeout:      true,
+		wantOneFinalTaskSkipped: true,
 	}, {
 		name: "finally timeout calculated",
+		trs:  oneFinishedTRs,
 		pr: parse.MustParseV1PipelineRun(t, fmt.Sprintf(`
 metadata:
   name: %s
@@ -2781,13 +2798,43 @@ status:
         status: "Unknown"
         type: Succeeded
 `, prName)),
+		wantFinallyTimeout:      true,
+		wantOneFinalTaskSkipped: true,
+	}, {
+		name: "finally timeout specified and pipeline timeout set to 0",
+		trs:  oneStartedTRs,
+		pr: parse.MustParseV1PipelineRun(t, fmt.Sprintf(`
+metadata:
+  name: %s
+  namespace: foo
+spec:
+  pipelineRef:
+    name: test-pipeline-with-finally
+  timeouts:
+    finally: 15m
+    pipeline: 0s
+status:
+  startTime: "2021-12-31T23:40:00Z"
+  childReferences:
+  - name: test-pipeline-run-with-timeout-hello-world
+    apiVersion: tekton.dev/v1
+    kind: TaskRun
+    pipelineTaskName: task1
+    status:
+      conditions:
+      - lastTransitionTime: null
+        status: Unknown
+        type: Succeeded
+`, prName)),
+		wantFinallyTimeout:      false,
+		wantOneFinalTaskSkipped: false,
 	}}
 	for _, tc := range tcs {
 		d := test.Data{
 			PipelineRuns: []*v1.PipelineRun{tc.pr},
 			Pipelines:    ps,
 			Tasks:        ts,
-			TaskRuns:     trs,
+			TaskRuns:     tc.trs,
 		}
 		prt := newPipelineRunTest(t, d)
 		defer prt.Cancel()
@@ -2807,22 +2854,26 @@ status:
 		}
 
 		// Check that there is a skipped task for the expected reason
-		if len(reconciledRun.Status.SkippedTasks) != 1 {
-			t.Errorf("expected one skipped task, found %d", len(reconciledRun.Status.SkippedTasks))
-		} else if reconciledRun.Status.SkippedTasks[0].Reason != v1.FinallyTimedOutSkip {
-			t.Errorf("expected skipped reason to be '%s', but was '%s", v1.FinallyTimedOutSkip, reconciledRun.Status.SkippedTasks[0].Reason)
+		if tc.wantOneFinalTaskSkipped {
+			if len(reconciledRun.Status.SkippedTasks) != 1 {
+				t.Errorf("expected one skipped task, found %d", len(reconciledRun.Status.SkippedTasks))
+			} else if reconciledRun.Status.SkippedTasks[0].Reason != v1.FinallyTimedOutSkip {
+				t.Errorf("expected skipped reason to be '%s', but was '%s", v1.FinallyTimedOutSkip, reconciledRun.Status.SkippedTasks[0].Reason)
+			}
 		}
 
-		updatedTaskRun, err := clients.Pipeline.TektonV1().TaskRuns("foo").Get(context.Background(), trs[1].Name, metav1.GetOptions{})
-		if err != nil {
-			t.Fatalf("error getting updated TaskRun: %#v", err)
-		}
+		if tc.wantFinallyTimeout {
+			updatedTaskRun, err := clients.Pipeline.TektonV1().TaskRuns("foo").Get(context.Background(), tc.trs[1].Name, metav1.GetOptions{})
+			if err != nil {
+				t.Fatalf("error getting updated TaskRun: %#v", err)
+			}
 
-		if updatedTaskRun.Spec.Status != v1.TaskRunSpecStatusCancelled {
-			t.Errorf("expected existing TaskRun Spec.Status to be set to %s, but was %s", v1.TaskRunSpecStatusCancelled, updatedTaskRun.Spec.Status)
-		}
-		if updatedTaskRun.Spec.StatusMessage != v1.TaskRunCancelledByPipelineTimeoutMsg {
-			t.Errorf("expected existing TaskRun Spec.StatusMessage to be set to %s, but was %s", v1.TaskRunCancelledByPipelineTimeoutMsg, updatedTaskRun.Spec.StatusMessage)
+			if updatedTaskRun.Spec.Status != v1.TaskRunSpecStatusCancelled {
+				t.Errorf("expected existing TaskRun Spec.Status to be set to %s, but was %s", v1.TaskRunSpecStatusCancelled, updatedTaskRun.Spec.Status)
+			}
+			if updatedTaskRun.Spec.StatusMessage != v1.TaskRunCancelledByPipelineTimeoutMsg {
+				t.Errorf("expected existing TaskRun Spec.StatusMessage to be set to %s, but was %s", v1.TaskRunCancelledByPipelineTimeoutMsg, updatedTaskRun.Spec.StatusMessage)
+			}
 		}
 	}
 }