Merge pull request tektoncd#109 from katanomi/feat/upgrade-tekton-to-…

…v0.56.1

feat: upgrade tekton to v0.56.1

docs/pipelineruns.md

@@ -1424,6 +1424,8 @@ If you set the timeout to 0, the `PipelineRun` fails immediately upon encounteri
 
 > :warning: ** `timeout` is deprecated and will be removed in future versions. Consider using `timeouts` instead.
 
+> :note: An internal detail of the `PipelineRun` and `TaskRun` reconcilers in the Tekton controller is that it will requeue a `PipelineRun` or `TaskRun` for re-evaluation, versus waiting for the next update, under certain conditions.  The wait time for that re-queueing is the elapsed time subtracted from the timeout; however, if the timeout is set to '0', that calculation produces a negative number, and the new reconciliation event will fire immediately, which can impact overall performance, which is counter to the intent of wait time calculation.  So instead, the reconcilers will use the configured global timeout as the wait time when the associated timeout has been set to '0'.
+
 ## `PipelineRun` status
 
 ### The `status` field

docs/taskruns.md

@@ -771,6 +771,8 @@ a different global default timeout value using the `default-timeout-minutes` fie
 all `TaskRuns` that do not have a timeout set will have no timeout and will run until it completes successfully
 or fails from an error.
 
+> :note: An internal detail of the `PipelineRun` and `TaskRun` reconcilers in the Tekton controller is that it will requeue a `PipelineRun` or `TaskRun` for re-evaluation, versus waiting for the next update, under certain conditions.  The wait time for that re-queueing is the elapsed time subtracted from the timeout; however, if the timeout is set to '0', that calculation produces a negative number, and the new reconciliation event will fire immediately, which can impact overall performance, which is counter to the intent of wait time calculation.  So instead, the reconcilers will use the configured global timeout as the wait time when the associated timeout has been set to '0'.
+
 ### Specifying `ServiceAccount` credentials
 
 You can execute the `Task` in your `TaskRun` with a specific set of credentials by

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/pipelinerun/pipelinerun.go

@@ -25,6 +25,7 @@ import (
 	"reflect"
 	"regexp"
 	"strings"
+	"time"
 
 	"github.com/hashicorp/go-multierror"
 	"github.com/tektoncd/pipeline/pkg/apis/config"
@@ -273,9 +274,20 @@ func (c *Reconciler) ReconcileKind(ctx context.Context, pr *v1.PipelineRun) pkgr
 		// Compute the time since the task started.
 		elapsed := c.Clock.Since(pr.Status.StartTime.Time)
 		// Snooze this resource until the appropriate timeout has elapsed.
-		waitTime := pr.PipelineTimeout(ctx) - elapsed
-		if pr.Status.FinallyStartTime == nil && pr.TasksTimeout() != nil {
+		// but if the timeout has been disabled by setting timeout to 0, we
+		// do not want to subtract from 0, because a negative wait time will
+		// result in the requeue happening essentially immediately
+		timeout := pr.PipelineTimeout(ctx)
+		taskTimeout := pr.TasksTimeout()
+		waitTime := timeout - elapsed
+		if timeout == config.NoTimeoutDuration {
+			waitTime = time.Duration(config.FromContextOrDefaults(ctx).Defaults.DefaultTimeoutMinutes) * time.Minute
+		}
+		if pr.Status.FinallyStartTime == nil && taskTimeout != nil {
 			waitTime = pr.TasksTimeout().Duration - elapsed
+			if taskTimeout.Duration == config.NoTimeoutDuration {
+				waitTime = time.Duration(config.FromContextOrDefaults(ctx).Defaults.DefaultTimeoutMinutes) * time.Minute
+			}
 		} else if pr.Status.FinallyStartTime != nil && pr.FinallyTimeout() != nil {
 			finallyWaitTime := pr.FinallyTimeout().Duration - c.Clock.Since(pr.Status.FinallyStartTime.Time)
 			if finallyWaitTime < waitTime {
@@ -848,6 +860,14 @@ func (c *Reconciler) runNextSchedulableTask(ctx context.Context, pr *v1.Pipeline
 				continue
 			}
 			resources.ApplyTaskResults(resources.PipelineRunState{rpt}, resolvedResultRefs)
+
+			if err := rpt.EvaluateCEL(); err != nil {
+				logger.Errorf("Final task %q is not executed, due to error evaluating CEL %s: %v", rpt.PipelineTask.Name, pr.Name, err)
+				pr.Status.MarkFailed(string(v1.PipelineRunReasonCELEvaluationFailed),
+					"Error evaluating CEL %s: %w", pr.Name, pipelineErrors.WrapUserError(err))
+				return controller.NewPermanentError(err)
+			}
+
 			nextRpts = append(nextRpts, rpt)
 		}
 	}

pkg/reconciler/pipelinerun/pipelinerun_test.go

@@ -2486,6 +2486,96 @@ spec:
 	}
 }
 
+func TestReconcileWithTimeoutDisabled(t *testing.T) {
+	testCases := []struct {
+		name    string
+		timeout time.Duration
+	}{
+		{
+			name:    "pipeline timeout is 24h",
+			timeout: 24 * time.Hour,
+		},
+		{
+			name:    "pipeline timeout is way longer than 24h",
+			timeout: 360 * time.Hour,
+		},
+	}
+
+	for _, tc := range testCases {
+		startTime := time.Date(2022, time.January, 1, 0, 0, 0, 0, time.UTC).Add(-3 * tc.timeout)
+		t.Run(tc.name, func(t *testing.T) {
+			ps := []*v1.Pipeline{parse.MustParseV1Pipeline(t, `
+metadata:
+  name: test-pipeline
+  namespace: foo
+spec:
+  tasks:
+  - name: hello-world-1
+    taskRef:
+      name: hello-world
+  - name: hello-world-2
+    taskRef:
+      name: hello-world
+`)}
+			prs := []*v1.PipelineRun{parse.MustParseV1PipelineRun(t, `
+metadata:
+  name: test-pipeline-run-with-timeout-disabled
+  namespace: foo
+spec:
+  pipelineRef:
+    name: test-pipeline
+  taskRunTemplate:
+    serviceAccountName: test-sa
+  timeouts:
+    pipeline: 0h0m0s
+status:
+  startTime: "2021-12-30T00:00:00Z"
+`)}
+			ts := []*v1.Task{simpleHelloWorldTask}
+
+			trs := []*v1.TaskRun{mustParseTaskRunWithObjectMeta(t, taskRunObjectMeta("test-pipeline-run-with-timeout-hello-world-1", "foo", "test-pipeline-run-with-timeout-disabled",
+				"test-pipeline", "hello-world-1", false), `
+spec:
+  serviceAccountName: test-sa
+  taskRef:
+    name: hello-world
+    kind: Task
+`)}
+			start := metav1.NewTime(startTime)
+			prs[0].Status.StartTime = &start
+
+			d := test.Data{
+				PipelineRuns: prs,
+				Pipelines:    ps,
+				Tasks:        ts,
+				TaskRuns:     trs,
+			}
+			prt := newPipelineRunTest(t, d)
+			defer prt.Cancel()
+
+			c := prt.TestAssets.Controller
+			clients := prt.TestAssets.Clients
+			reconcileError := c.Reconciler.Reconcile(prt.TestAssets.Ctx, "foo/test-pipeline-run-with-timeout-disabled")
+			if reconcileError == nil {
+				t.Errorf("expected error, but got nil")
+			}
+			if isRequeueError, requeueDuration := controller.IsRequeueKey(reconcileError); !isRequeueError {
+				t.Errorf("Expected requeue error, but got: %s", reconcileError.Error())
+			} else if requeueDuration < 0 {
+				t.Errorf("Expected a positive requeue duration but got %s", requeueDuration.String())
+			}
+			prt.Test.Logf("Getting reconciled run")
+			reconciledRun, err := clients.Pipeline.TektonV1().PipelineRuns("foo").Get(prt.TestAssets.Ctx, "test-pipeline-run-with-timeout-disabled", metav1.GetOptions{})
+			if err != nil {
+				prt.Test.Errorf("Somehow had error getting reconciled run out of fake client: %s", err)
+			}
+			if reconciledRun.Status.GetCondition(apis.ConditionSucceeded).Reason == "PipelineRunTimeout" {
+				t.Errorf("Expected PipelineRun to not be timed out, but it is timed out")
+			}
+		})
+	}
+}
+
 func TestReconcileWithTimeoutForALongTimeAndEtcdLimit_Pipeline(t *testing.T) {
 	timeout := 12 * time.Hour
 	testCases := []struct {
@@ -4553,6 +4643,174 @@ spec:
 	}
 }
 
+func TestReconcileWithFinalTasksCELWhenExpressions(t *testing.T) {
+	ps := []*v1.Pipeline{parse.MustParseV1Pipeline(t, `
+metadata:
+  name: test-pipeline
+  namespace: foo
+spec:
+  params:
+  - name: run
+    type: string
+  tasks:
+  - name: a-task
+    taskRef:
+      name: a-task
+  - name: b-task
+    taskRef:
+      name: b-task
+  finally:
+  - name: f-c-task
+    taskRef:
+      name: f-c-task
+    when:
+    - cel: "'$(tasks.a-task.results.aResult)' == 'aResultValue'"
+    - cel: "'$(params.run)'=='yes'"
+    - cel: "'$(tasks.a-task.status)' == 'Succeeded'"
+  - name: f-d-task
+    taskRef:
+      name: f-d-task
+    when:
+    - cel: "'$(tasks.b-task.status)' == 'Succeeded'"
+`)}
+	prs := []*v1.PipelineRun{parse.MustParseV1PipelineRun(t, `
+metadata:
+  name: test-pipeline-run-different-final-task-when
+  namespace: foo
+spec:
+  params:
+  - name: run
+    value: "yes"
+  pipelineRef:
+    name: test-pipeline
+  taskRunTemplate:
+    serviceAccountName: test-sa-0
+`)}
+	ts := []*v1.Task{
+		{ObjectMeta: baseObjectMeta("a-task", "foo")},
+		{ObjectMeta: baseObjectMeta("b-task", "foo")},
+		{ObjectMeta: baseObjectMeta("f-c-task", "foo")},
+		{ObjectMeta: baseObjectMeta("f-d-task", "foo")},
+	}
+	trs := []*v1.TaskRun{mustParseTaskRunWithObjectMeta(t,
+		taskRunObjectMeta("test-pipeline-run-different-final-task-when-a-task-xxyyy", "foo", "test-pipeline-run-different-final-task-when",
+			"test-pipeline", "a-task", true),
+		`
+spec:
+  serviceAccountName: test-sa
+  taskRef:
+    name: hello-world
+  timeout: 1h0m0s
+status:
+  conditions:
+  - status: "True"
+    type: Succeeded
+  results:
+  - name: aResult
+    value: aResultValue
+`), mustParseTaskRunWithObjectMeta(t,
+		taskRunObjectMeta("test-pipeline-run-different-final-task-when-b-task-xxyyy", "foo", "test-pipeline-run-different-final-task-when",
+			"test-pipeline", "b-task", true),
+		`
+spec:
+  serviceAccountName: test-sa
+  taskRef:
+    name: hello-world
+  timeout: 1h0m0s
+status:
+  conditions:
+  - status: "False"
+    type: Succeeded
+`)}
+	cms := []*corev1.ConfigMap{
+		{
+			ObjectMeta: metav1.ObjectMeta{Name: config.GetFeatureFlagsConfigName(), Namespace: system.Namespace()},
+			Data: map[string]string{
+				"enable-cel-in-whenexpression": "true",
+			},
+		},
+	}
+	d := test.Data{
+		PipelineRuns: prs,
+		Pipelines:    ps,
+		Tasks:        ts,
+		TaskRuns:     trs,
+		ConfigMaps:   cms,
+	}
+	prt := newPipelineRunTest(t, d)
+	defer prt.Cancel()
+
+	wantEvents := []string{
+		"Normal Started",
+		"Normal Running Tasks Completed: 2 \\(Failed: 1, Cancelled 0\\), Incomplete: 1, Skipped: 1",
+	}
+	pipelineRun, clients := prt.reconcileRun("foo", "test-pipeline-run-different-final-task-when", wantEvents, false)
+
+	expectedTaskRunName := "test-pipeline-run-different-final-task-when-f-c-task"
+	expectedTaskRun := mustParseTaskRunWithObjectMeta(t,
+		taskRunObjectMeta(expectedTaskRunName, "foo", "test-pipeline-run-different-final-task-when", "test-pipeline", "f-c-task", true),
+		`
+spec:
+  serviceAccountName: test-sa-0
+  taskRef:
+    name: f-c-task
+    kind: Task
+`)
+	expectedTaskRun.Labels[pipeline.MemberOfLabelKey] = v1.PipelineFinallyTasks
+	// Check that the expected TaskRun was created
+	actual, err := clients.Pipeline.TektonV1().TaskRuns("foo").List(prt.TestAssets.Ctx, metav1.ListOptions{
+		LabelSelector: "tekton.dev/pipelineTask=f-c-task,tekton.dev/pipelineRun=test-pipeline-run-different-final-task-when",
+		Limit:         1,
+	})
+
+	if err != nil {
+		t.Fatalf("Failure to list TaskRun's %s", err)
+	}
+	if len(actual.Items) != 1 {
+		t.Fatalf("Expected 1 TaskRuns got %d", len(actual.Items))
+	}
+	actualTaskRun := actual.Items[0]
+	if d := cmp.Diff(expectedTaskRun, &actualTaskRun, ignoreResourceVersion, ignoreTypeMeta); d != "" {
+		t.Errorf("expected to see TaskRun %v created. Diff %s", expectedTaskRunName, diff.PrintWantGot(d))
+	}
+
+	expectedWhenExpressionsInTaskRun := []v1.WhenExpression{{
+		CEL: "'aResultValue' == 'aResultValue'",
+	}, {
+		CEL: "'yes'=='yes'",
+	}, {
+		CEL: "'Succeeded' == 'Succeeded'",
+	}}
+	verifyTaskRunStatusesWhenExpressions(t, pipelineRun.Status, expectedTaskRunName, expectedWhenExpressionsInTaskRun)
+
+	actualSkippedTasks := pipelineRun.Status.SkippedTasks
+	expectedSkippedTasks := []v1.SkippedTask{{
+		Name:   "f-d-task",
+		Reason: v1.WhenExpressionsSkip,
+		WhenExpressions: v1.WhenExpressions{{
+			CEL: "'Failed' == 'Succeeded'",
+		}},
+	}}
+	if d := cmp.Diff(expectedSkippedTasks, actualSkippedTasks); d != "" {
+		t.Errorf("expected to find Skipped Tasks %v. Diff %s", expectedSkippedTasks, diff.PrintWantGot(d))
+	}
+
+	skippedTasks := []string{"f-d-task"}
+	for _, skippedTask := range skippedTasks {
+		labelSelector := fmt.Sprintf("tekton.dev/pipelineTask=%s,tekton.dev/pipelineRun=test-pipeline-run-different-final-task-when", skippedTask)
+		actualSkippedTask, err := clients.Pipeline.TektonV1().TaskRuns("foo").List(prt.TestAssets.Ctx, metav1.ListOptions{
+			LabelSelector: labelSelector,
+			Limit:         1,
+		})
+		if err != nil {
+			t.Fatalf("Failure to list TaskRun's %s", err)
+		}
+		if len(actualSkippedTask.Items) != 0 {
+			t.Fatalf("Expected 0 TaskRuns got %d", len(actualSkippedTask.Items))
+		}
+	}
+}
+
 func TestReconcile_InvalidCELWhenExpressions(t *testing.T) {
 	ps := []*v1.Pipeline{parse.MustParseV1Pipeline(t, `
 metadata:

pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go

@@ -76,9 +76,8 @@ type ResolvedPipelineTask struct {
 // EvaluateCEL evaluate the CEL expressions, and store the evaluated results in EvaluatedCEL
 func (t *ResolvedPipelineTask) EvaluateCEL() error {
 	if t.PipelineTask != nil {
-		if len(t.EvaluatedCEL) == 0 {
-			t.EvaluatedCEL = make(map[string]bool)
-		}
+		// Each call to this function will reset this field to prevent additional CELs.
+		t.EvaluatedCEL = make(map[string]bool)
 		for _, we := range t.PipelineTask.When {
 			if we.CEL == "" {
 				continue

pkg/reconciler/taskrun/resources/taskref_test.go

@@ -228,6 +228,12 @@ func TestLocalTaskRef(t *testing.T) {
 				&v1beta1.ClusterTask{
 					ObjectMeta: metav1.ObjectMeta{
 						Name: "cluster-task",
+						Annotations: map[string]string{
+							"foo": "bar",
+						},
+						Labels: map[string]string{
+							"foo": "bar",
+						},
 					},
 				},
 				&v1beta1.ClusterTask{
@@ -247,6 +253,12 @@ func TestLocalTaskRef(t *testing.T) {
 				},
 				ObjectMeta: metav1.ObjectMeta{
 					Name: "cluster-task",
+					Annotations: map[string]string{
+						"foo": "bar",
+					},
+					Labels: map[string]string{
+						"foo": "bar",
+					},
 				},
 			},
 			wantErr: nil,