fix(taskrun): block taskrun spec updates once the taskrun has started

l-qing · tekton-robot · commit d6a2cdbaefa1 · 2024-07-25T13:49:41.000+01:00
fix #8148 Typically, the spec field of a TaskRun resource is not allowed to be updated after creation, such as the `timeout` configuration. Only a few fields, such as `status` `statusMessage`, are allowed to be updated.
diff --git a/pkg/apis/pipeline/v1/taskrun_validation.go b/pkg/apis/pipeline/v1/taskrun_validation.go
@@ -26,6 +26,7 @@ import (
 	"github.com/tektoncd/pipeline/pkg/apis/validate"
 	admissionregistrationv1 "k8s.io/api/admissionregistration/v1"
 	corev1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/api/equality"
 	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/utils/strings/slices"
 	"knative.dev/pkg/apis"
@@ -50,6 +51,9 @@ func (tr *TaskRun) Validate(ctx context.Context) *apis.FieldError {
 
 // Validate taskrun spec
 func (ts *TaskRunSpec) Validate(ctx context.Context) (errs *apis.FieldError) {
+	// Validate the spec changes
+	errs = errs.Also(ts.ValidateUpdate(ctx))
+
 	// Must have exactly one of taskRef and taskSpec.
 	if ts.TaskRef == nil && ts.TaskSpec == nil {
 		errs = errs.Also(apis.ErrMissingOneOf("taskRef", "taskSpec"))
@@ -118,6 +122,34 @@ func (ts *TaskRunSpec) Validate(ctx context.Context) (errs *apis.FieldError) {
 	return errs
 }
 
+// ValidateUpdate validates the update of a TaskRunSpec
+func (ts *TaskRunSpec) ValidateUpdate(ctx context.Context) (errs *apis.FieldError) {
+	if !apis.IsInUpdate(ctx) {
+		return
+	}
+	oldObj, ok := apis.GetBaseline(ctx).(*TaskRun)
+	if !ok || oldObj == nil {
+		return
+	}
+	old := &oldObj.Spec
+
+	// If already in the done state, the spec cannot be modified.
+	// Otherwise, only the status, statusMessage field can be modified.
+	tips := "Once the TaskRun is complete, no updates are allowed"
+	if !oldObj.IsDone() {
+		old = old.DeepCopy()
+		old.Status = ts.Status
+		old.StatusMessage = ts.StatusMessage
+		tips = "Once the TaskRun has started, only status and statusMessage updates are allowed"
+	}
+
+	if !equality.Semantic.DeepEqual(old, ts) {
+		errs = errs.Also(apis.ErrInvalidValue(tips, ""))
+	}
+
+	return
+}
+
 // validateInlineParameters validates that any parameters called in the
 // Task spec are declared in the TaskRun.
 // This is crucial for propagated parameters because the parameters could
diff --git a/pkg/apis/pipeline/v1/taskrun_validation_test.go b/pkg/apis/pipeline/v1/taskrun_validation_test.go
@@ -22,6 +22,7 @@ import (
 	"time"
 
 	"github.com/google/go-cmp/cmp"
+	"github.com/google/go-cmp/cmp/cmpopts"
 	"github.com/tektoncd/pipeline/pkg/apis/config"
 	cfgtesting "github.com/tektoncd/pipeline/pkg/apis/config/testing"
 	"github.com/tektoncd/pipeline/pkg/apis/pipeline/pod"
@@ -31,6 +32,7 @@ import (
 	corev1resources "k8s.io/apimachinery/pkg/api/resource"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"knative.dev/pkg/apis"
+	duckv1 "knative.dev/pkg/apis/duck/v1"
 )
 
 func TestTaskRun_Invalidate(t *testing.T) {
@@ -968,3 +970,148 @@ func TestTaskRunSpec_Validate(t *testing.T) {
 		})
 	}
 }
+
+func TestTaskRunSpec_ValidateUpdate(t *testing.T) {
+	tests := []struct {
+		name            string
+		isCreate        bool
+		isUpdate        bool
+		baselineTaskRun *v1.TaskRun
+		taskRun         *v1.TaskRun
+		expectedError   apis.FieldError
+	}{
+		{
+			name: "is create ctx",
+			taskRun: &v1.TaskRun{
+				Spec: v1.TaskRunSpec{},
+			},
+			isCreate:      true,
+			isUpdate:      false,
+			expectedError: apis.FieldError{},
+		}, {
+			name: "is update ctx, no changes",
+			baselineTaskRun: &v1.TaskRun{
+				Spec: v1.TaskRunSpec{
+					Status: "TaskRunCancelled",
+				},
+			},
+			taskRun: &v1.TaskRun{
+				Spec: v1.TaskRunSpec{
+					Status: "TaskRunCancelled",
+				},
+			},
+			isCreate:      false,
+			isUpdate:      true,
+			expectedError: apis.FieldError{},
+		}, {
+			name:            "is update ctx, baseline is nil, skip validation",
+			baselineTaskRun: nil,
+			taskRun: &v1.TaskRun{
+				Spec: v1.TaskRunSpec{
+					Timeout: &metav1.Duration{Duration: 1},
+				},
+			},
+			isCreate:      false,
+			isUpdate:      true,
+			expectedError: apis.FieldError{},
+		}, {
+			name: "is update ctx, baseline is unknown, only status changes",
+			baselineTaskRun: &v1.TaskRun{
+				Spec: v1.TaskRunSpec{
+					Status:        "",
+					StatusMessage: "",
+				},
+				Status: v1.TaskRunStatus{
+					Status: duckv1.Status{
+						Conditions: duckv1.Conditions{
+							{Type: apis.ConditionSucceeded, Status: corev1.ConditionUnknown},
+						},
+					},
+				},
+			},
+			taskRun: &v1.TaskRun{
+				Spec: v1.TaskRunSpec{
+					Status:        "TaskRunCancelled",
+					StatusMessage: "TaskRun is cancelled",
+				},
+			},
+			isCreate:      false,
+			isUpdate:      true,
+			expectedError: apis.FieldError{},
+		}, {
+			name: "is update ctx, baseline is unknown, status and timeout changes",
+			baselineTaskRun: &v1.TaskRun{
+				Spec: v1.TaskRunSpec{
+					Status:        "",
+					StatusMessage: "",
+					Timeout:       &metav1.Duration{Duration: 0},
+				},
+				Status: v1.TaskRunStatus{
+					Status: duckv1.Status{
+						Conditions: duckv1.Conditions{
+							{Type: apis.ConditionSucceeded, Status: corev1.ConditionUnknown},
+						},
+					},
+				},
+			},
+			taskRun: &v1.TaskRun{
+				Spec: v1.TaskRunSpec{
+					Status:        "TaskRunCancelled",
+					StatusMessage: "TaskRun is cancelled",
+					Timeout:       &metav1.Duration{Duration: 1},
+				},
+			},
+			isCreate: false,
+			isUpdate: true,
+			expectedError: apis.FieldError{
+				Message: `invalid value: Once the TaskRun has started, only status and statusMessage updates are allowed`,
+				Paths:   []string{""},
+			},
+		}, {
+			name: "is update ctx, baseline is done, status changes",
+			baselineTaskRun: &v1.TaskRun{
+				Spec: v1.TaskRunSpec{
+					Status: "",
+				},
+				Status: v1.TaskRunStatus{
+					Status: duckv1.Status{
+						Conditions: duckv1.Conditions{
+							{Type: apis.ConditionSucceeded, Status: corev1.ConditionTrue},
+						},
+					},
+				},
+			},
+			taskRun: &v1.TaskRun{
+				Spec: v1.TaskRunSpec{
+					Status: "TaskRunCancelled",
+				},
+			},
+			isCreate: false,
+			isUpdate: true,
+			expectedError: apis.FieldError{
+				Message: `invalid value: Once the TaskRun is complete, no updates are allowed`,
+				Paths:   []string{""},
+			},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			ctx := config.ToContext(context.Background(), &config.Config{
+				FeatureFlags: &config.FeatureFlags{},
+				Defaults:     &config.Defaults{},
+			})
+			if tt.isCreate {
+				ctx = apis.WithinCreate(ctx)
+			}
+			if tt.isUpdate {
+				ctx = apis.WithinUpdate(ctx, tt.baselineTaskRun)
+			}
+			tr := tt.taskRun
+			err := tr.Spec.ValidateUpdate(ctx)
+			if d := cmp.Diff(tt.expectedError.Error(), err.Error(), cmpopts.IgnoreUnexported(apis.FieldError{})); d != "" {
+				t.Errorf("TaskRunSpec.ValidateUpdate() errors diff %s", diff.PrintWantGot(d))
+			}
+		})
+	}
+}
diff --git a/pkg/apis/pipeline/v1beta1/taskrun_validation.go b/pkg/apis/pipeline/v1beta1/taskrun_validation.go
@@ -26,6 +26,7 @@ import (
 	"github.com/tektoncd/pipeline/pkg/apis/validate"
 	admissionregistrationv1 "k8s.io/api/admissionregistration/v1"
 	corev1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/api/equality"
 	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/utils/strings/slices"
 	"knative.dev/pkg/apis"
@@ -50,6 +51,9 @@ func (tr *TaskRun) Validate(ctx context.Context) *apis.FieldError {
 
 // Validate taskrun spec
 func (ts *TaskRunSpec) Validate(ctx context.Context) (errs *apis.FieldError) {
+	// Validate the spec changes
+	errs = errs.Also(ts.ValidateUpdate(ctx))
+
 	// Must have exactly one of taskRef and taskSpec.
 	if ts.TaskRef == nil && ts.TaskSpec == nil {
 		errs = errs.Also(apis.ErrMissingOneOf("taskRef", "taskSpec"))
@@ -118,6 +122,34 @@ func (ts *TaskRunSpec) Validate(ctx context.Context) (errs *apis.FieldError) {
 	return errs
 }
 
+// ValidateUpdate validates the update of a TaskRunSpec
+func (ts *TaskRunSpec) ValidateUpdate(ctx context.Context) (errs *apis.FieldError) {
+	if !apis.IsInUpdate(ctx) {
+		return
+	}
+	oldObj, ok := apis.GetBaseline(ctx).(*TaskRun)
+	if !ok || oldObj == nil {
+		return
+	}
+	old := &oldObj.Spec
+
+	// If already in the done state, the spec cannot be modified.
+	// Otherwise, only the status, statusMessage field can be modified.
+	tips := "Once the TaskRun is complete, no updates are allowed"
+	if !oldObj.IsDone() {
+		old = old.DeepCopy()
+		old.Status = ts.Status
+		old.StatusMessage = ts.StatusMessage
+		tips = "Once the TaskRun has started, only status and statusMessage updates are allowed"
+	}
+
+	if !equality.Semantic.DeepEqual(old, ts) {
+		errs = errs.Also(apis.ErrInvalidValue(tips, ""))
+	}
+
+	return
+}
+
 // validateInlineParameters validates that any parameters called in the
 // Task spec are declared in the TaskRun.
 // This is crucial for propagated parameters because the parameters could
diff --git a/pkg/apis/pipeline/v1beta1/taskrun_validation_test.go b/pkg/apis/pipeline/v1beta1/taskrun_validation_test.go
