Do not panic if Timeout is nil

`TaskRun` and `PipelineRun` should have a default Timeout set by the webhook. That said, if the webhook didn't do its work, or the object where created before those webhook defaults, it could be nil and makes the controller panicking. Signed-off-by: Vincent Demeester <[email protected]> (cherry picked from commit 4cd1d52)
tektoncd · Jul 16, 2019 · ef4611b · ef4611b
1 parent 40d0594
commit ef4611b
Show file tree

Hide file tree

Showing 7 changed files with 82 additions and 7 deletions.
diff --git a/pkg/reconciler/timeout_handler.go b/pkg/reconciler/timeout_handler.go
@@ -7,6 +7,7 @@ import (
 
 	"time"
 
+	"github.com/tektoncd/pipeline/pkg/apis/config"
 	"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1"
 	clientset "github.com/tektoncd/pipeline/pkg/client/clientset/versioned"
 	"go.uber.org/zap"
@@ -204,14 +205,26 @@ func (t *TimeoutSet) checkTaskRunTimeouts(namespace string, pipelineclientset cl
 // 1. Stop signal, 2. TaskRun to complete or 3. Taskrun to time out, which is
 // determined by checking if the tr's timeout has occurred since the startTime
 func (t *TimeoutSet) WaitTaskRun(tr *v1alpha1.TaskRun, startTime *metav1.Time) {
-	t.waitRun(tr, tr.Spec.Timeout.Duration, startTime, t.taskRunCallbackFunc)
+	var timeout time.Duration
+	if tr.Spec.Timeout == nil {
+		timeout = config.DefaultTimeoutMinutes * time.Minute
+	} else {
+		timeout = tr.Spec.Timeout.Duration
+	}
+	t.waitRun(tr, timeout, startTime, t.taskRunCallbackFunc)
 }
 
 // WaitPipelineRun function creates a blocking function for pipelinerun to wait for
 // 1. Stop signal, 2. pipelinerun to complete or 3. pipelinerun to time out which is
 // determined by checking if the tr's timeout has occurred since the startTime
 func (t *TimeoutSet) WaitPipelineRun(pr *v1alpha1.PipelineRun, startTime *metav1.Time) {
-	t.waitRun(pr, pr.Spec.Timeout.Duration, startTime, t.pipelineRunCallbackFunc)
+	var timeout time.Duration
+	if pr.Spec.Timeout == nil {
+		timeout = config.DefaultTimeoutMinutes * time.Minute
+	} else {
+		timeout = pr.Spec.Timeout.Duration
+	}
+	t.waitRun(pr, timeout, startTime, t.pipelineRunCallbackFunc)
 }
 
 func (t *TimeoutSet) waitRun(runObj StatusKey, timeout time.Duration, startTime *metav1.Time, callback func(interface{})) {

diff --git a/pkg/reconciler/timeout_handler_test.go b/pkg/reconciler/timeout_handler_test.go
@@ -43,6 +43,15 @@ func TestTaskRunCheckTimeouts(t *testing.T) {
 		tb.TaskRunStartTime(time.Now()),
 	))
 
+	taskRunRunningNilTimeout := tb.TaskRun("test-taskrun-running-nil-timeout", testNs, tb.TaskRunSpec(
+		tb.TaskRunTaskRef(simpleTask.Name, tb.TaskRefAPIVersion("a1")),
+		tb.TaskRunNilTimeout,
+	), tb.TaskRunStatus(tb.Condition(apis.Condition{
+		Type:   apis.ConditionSucceeded,
+		Status: corev1.ConditionUnknown}),
+		tb.TaskRunStartTime(time.Now()),
+	))
+
 	taskRunDone := tb.TaskRun("test-taskrun-completed", testNs, tb.TaskRunSpec(
 		tb.TaskRunTaskRef(simpleTask.Name, tb.TaskRefAPIVersion("a1")),
 		tb.TaskRunTimeout(config.DefaultTimeoutMinutes*time.Minute),
@@ -61,7 +70,7 @@ func TestTaskRunCheckTimeouts(t *testing.T) {
 	))
 
 	d := test.Data{
-		TaskRuns: []*v1alpha1.TaskRun{taskRunTimedout, taskRunRunning, taskRunDone, taskRunCancelled},
+		TaskRuns: []*v1alpha1.TaskRun{taskRunTimedout, taskRunRunning, taskRunDone, taskRunCancelled, taskRunRunningNilTimeout},
 		Tasks:    []*v1alpha1.Task{simpleTask},
 		Namespaces: []*corev1.Namespace{{
 			ObjectMeta: metav1.ObjectMeta{
@@ -96,6 +105,10 @@ func TestTaskRunCheckTimeouts(t *testing.T) {
 		name:           "running",
 		taskRun:        taskRunRunning,
 		expectCallback: false,
+	}, {
+		name:           "running-with-nil-timeout",
+		taskRun:        taskRunRunningNilTimeout,
+		expectCallback: false,
 	}, {
 		name:           "completed",
 		taskRun:        taskRunDone,
@@ -150,6 +163,16 @@ func TestPipelinRunCheckTimeouts(t *testing.T) {
 			tb.PipelineRunStartTime(time.Now()),
 		),
 	)
+	prRunningNilTimeout := tb.PipelineRun("test-pipeline-running-nil-timeout", testNs,
+		tb.PipelineRunSpec("test-pipeline",
+			tb.PipelineRunNilTimeout,
+		),
+		tb.PipelineRunStatus(tb.PipelineRunStatusCondition(apis.Condition{
+			Type:   apis.ConditionSucceeded,
+			Status: corev1.ConditionUnknown}),
+			tb.PipelineRunStartTime(time.Now()),
+		),
+	)
 	prDone := tb.PipelineRun("test-pipeline-done", testNs,
 		tb.PipelineRunSpec("test-pipeline",
 			tb.PipelineRunTimeout(config.DefaultTimeoutMinutes*time.Minute),
@@ -170,7 +193,7 @@ func TestPipelinRunCheckTimeouts(t *testing.T) {
 		),
 	)
 	d := test.Data{
-		PipelineRuns: []*v1alpha1.PipelineRun{prTimeout, prRunning, prDone, prCancelled},
+		PipelineRuns: []*v1alpha1.PipelineRun{prTimeout, prRunning, prDone, prCancelled, prRunningNilTimeout},
 		Pipelines:    []*v1alpha1.Pipeline{simplePipeline},
 		Tasks:        []*v1alpha1.Task{ts},
 		Namespaces: []*corev1.Namespace{{
@@ -201,6 +224,10 @@ func TestPipelinRunCheckTimeouts(t *testing.T) {
 		name:           "pr-running",
 		pr:             prRunning,
 		expectCallback: false,
+	}, {
+		name:           "pr-running-nil-timeout",
+		pr:             prRunningNilTimeout,
+		expectCallback: false,
 	}, {
 		name:           "pr-timedout",
 		pr:             prTimeout,

diff --git a/pkg/reconciler/v1alpha1/pipelinerun/pipelinerun.go b/pkg/reconciler/v1alpha1/pipelinerun/pipelinerun.go
@@ -26,6 +26,7 @@ import (
 	"github.com/knative/pkg/configmap"
 	"github.com/knative/pkg/controller"
 	"github.com/knative/pkg/tracker"
+
 	"github.com/tektoncd/pipeline/pkg/apis/pipeline"
 	"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1"
 	artifacts "github.com/tektoncd/pipeline/pkg/artifacts"
@@ -441,6 +442,8 @@ func (c *Reconciler) updateTaskRunsStatusDirectly(pr *v1alpha1.PipelineRun) erro
 func (c *Reconciler) createTaskRun(logger *zap.SugaredLogger, rprt *resources.ResolvedPipelineRunTask, pr *v1alpha1.PipelineRun, storageBasePath string) (*v1alpha1.TaskRun, error) {
 	var taskRunTimeout = &metav1.Duration{Duration: 0 * time.Second}
 
+	// If the value of the timeout is 0 for any resource, there is no timeout.
+	// It is impossible for pr.Spec.Timeout to be nil, since SetDefault always assigns it with a value.
 	if pr.Spec.Timeout != nil {
 		pTimeoutTime := pr.Status.StartTime.Add(pr.Spec.Timeout.Duration)
 		if time.Now().After(pTimeoutTime) {

diff --git a/pkg/reconciler/v1alpha1/taskrun/taskrun.go b/pkg/reconciler/v1alpha1/taskrun/taskrun.go
@@ -26,6 +26,7 @@ import (
 	"github.com/knative/pkg/apis"
 	"github.com/knative/pkg/controller"
 	"github.com/knative/pkg/tracker"
+	"github.com/tektoncd/pipeline/pkg/apis/config"
 	"github.com/tektoncd/pipeline/pkg/apis/pipeline"
 	"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1"
 	informers "github.com/tektoncd/pipeline/pkg/client/informers/externalversions/pipeline/v1alpha1"
@@ -259,6 +260,9 @@ func (c *Reconciler) reconcile(ctx context.Context, tr *v1alpha1.TaskRun) error
 		tr.ObjectMeta.Annotations[key] = value
 	}
 
+	if tr.Spec.Timeout == nil {
+		tr.Spec.Timeout = &metav1.Duration{Duration: config.DefaultTimeoutMinutes * time.Minute}
+	}
 	// Check if the TaskRun has timed out; if it is, this will set its status
 	// accordingly.
 	if timedOut, err := c.checkTimeout(tr, taskSpec, c.KubeClientSet.CoreV1().Pods(tr.Namespace).Delete); err != nil {

diff --git a/pkg/reconciler/v1alpha1/taskrun/taskrun_test.go b/pkg/reconciler/v1alpha1/taskrun/taskrun_test.go
@@ -1371,8 +1371,8 @@ func TestReconcilePodUpdateStatus(t *testing.T) {
 		t.Fatalf("Unexpected error fetching taskrun: %v", err)
 	}
 	if d := cmp.Diff(newTr.Status.GetCondition(apis.ConditionSucceeded), &apis.Condition{
-		Type:   apis.ConditionSucceeded,
-		Status: corev1.ConditionTrue,
+		Type:    apis.ConditionSucceeded,
+		Status:  corev1.ConditionTrue,
 		Reason:  status.ReasonSucceeded,
 		Message: "All Steps have completed executing",
 	}, ignoreLastTransitionTime); d != "" {
@@ -1519,6 +1519,24 @@ func TestReconcileTimeouts(t *testing.T) {
 				Message: `TaskRun "test-taskrun-default-timeout-60-minutes" failed to finish within "1h0m0s"`,
 			},
 		},
+		{
+			taskRun: tb.TaskRun("test-taskrun-nil-timeout-default-60-minutes", "foo",
+				tb.TaskRunSpec(
+					tb.TaskRunTaskRef(simpleTask.Name),
+					tb.TaskRunNilTimeout,
+				),
+				tb.TaskRunStatus(tb.Condition(apis.Condition{
+					Type:   apis.ConditionSucceeded,
+					Status: corev1.ConditionUnknown}),
+					tb.TaskRunStartTime(time.Now().Add(-61*time.Minute)))),
+
+			expectedStatus: &apis.Condition{
+				Type:    apis.ConditionSucceeded,
+				Status:  corev1.ConditionFalse,
+				Reason:  "TaskRunTimeout",
+				Message: `TaskRun "test-taskrun-nil-timeout-default-60-minutes" failed to finish within "1h0m0s"`,
+			},
+		},
 	}
 
 	for _, tc := range testcases {

diff --git a/test/builder/pipeline.go b/test/builder/pipeline.go
@@ -333,13 +333,18 @@ func PipelineRunParam(name, value string) PipelineRunSpecOp {
 	}
 }
 
-// PipelineRunTimeout sets the timeout to the PipelineSpec.
+// PipelineRunTimeout sets the timeout to the PipelineRunSpec.
 func PipelineRunTimeout(duration time.Duration) PipelineRunSpecOp {
 	return func(prs *v1alpha1.PipelineRunSpec) {
 		prs.Timeout = &metav1.Duration{Duration: duration}
 	}
 }
 
+// PipelineRunNilTimeout sets the timeout to nil on the PipelineRunSpec
+func PipelineRunNilTimeout(prs *v1alpha1.PipelineRunSpec) {
+	prs.Timeout = nil
+}
+
 // PipelineRunNodeSelector sets the Node selector to the PipelineSpec.
 func PipelineRunNodeSelector(values map[string]string) PipelineRunSpecOp {
 	return func(prs *v1alpha1.PipelineRunSpec) {

diff --git a/test/builder/task.go b/test/builder/task.go
@@ -349,6 +349,11 @@ func TaskRunTimeout(d time.Duration) TaskRunSpecOp {
 	}
 }
 
+// TaskRunNilTimeout sets the timeout duration to nil on the TaskRunSpec.
+func TaskRunNilTimeout(spec *v1alpha1.TaskRunSpec) {
+	spec.Timeout = nil
+}
+
 // TaskRunNodeSelector sets the NodeSelector to the PipelineSpec.
 func TaskRunNodeSelector(values map[string]string) TaskRunSpecOp {
 	return func(spec *v1alpha1.TaskRunSpec) {