From d6a2cdbaefa1472064840419fe2a72a845610980 Mon Sep 17 00:00:00 2001 From: qingliu Date: Wed, 24 Jul 2024 14:52:14 +0800 Subject: [PATCH] fix(taskrun): block taskrun spec updates once the taskrun has started 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. --- pkg/apis/pipeline/v1/taskrun_validation.go | 32 ++++ .../pipeline/v1/taskrun_validation_test.go | 147 ++++++++++++++++++ .../pipeline/v1beta1/taskrun_validation.go | 32 ++++ .../v1beta1/taskrun_validation_test.go | 147 ++++++++++++++++++ 4 files changed, 358 insertions(+) diff --git a/pkg/apis/pipeline/v1/taskrun_validation.go b/pkg/apis/pipeline/v1/taskrun_validation.go index ef4dcb5b6f5..cfdd423e996 100644 --- 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 index 4241e27fe86..1b0bca9d825 100644 --- 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 index bc0a2639875..a3f37caeee9 100644 --- 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 index f594f420401..0afe02f0372 100644 --- a/pkg/apis/pipeline/v1beta1/taskrun_validation_test.go +++ b/pkg/apis/pipeline/v1beta1/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" pod "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 EnableForbiddenEnv(ctx context.Context) context.Context { @@ -939,3 +941,148 @@ func TestTaskRunSpec_Validate(t *testing.T) { }) } } + +func TestTaskRunSpec_ValidateUpdate(t *testing.T) { + tests := []struct { + name string + isCreate bool + isUpdate bool + baselineTaskRun *v1beta1.TaskRun + taskRun *v1beta1.TaskRun + expectedError apis.FieldError + }{ + { + name: "is create ctx", + taskRun: &v1beta1.TaskRun{ + Spec: v1beta1.TaskRunSpec{}, + }, + isCreate: true, + isUpdate: false, + expectedError: apis.FieldError{}, + }, { + name: "is update ctx, no changes", + baselineTaskRun: &v1beta1.TaskRun{ + Spec: v1beta1.TaskRunSpec{ + Status: "TaskRunCancelled", + }, + }, + taskRun: &v1beta1.TaskRun{ + Spec: v1beta1.TaskRunSpec{ + Status: "TaskRunCancelled", + }, + }, + isCreate: false, + isUpdate: true, + expectedError: apis.FieldError{}, + }, { + name: "is update ctx, baseline is nil, skip validation", + baselineTaskRun: nil, + taskRun: &v1beta1.TaskRun{ + Spec: v1beta1.TaskRunSpec{ + Timeout: &metav1.Duration{Duration: 1}, + }, + }, + isCreate: false, + isUpdate: true, + expectedError: apis.FieldError{}, + }, { + name: "is update ctx, baseline is unknown, only status changes", + baselineTaskRun: &v1beta1.TaskRun{ + Spec: v1beta1.TaskRunSpec{ + Status: "", + StatusMessage: "", + }, + Status: v1beta1.TaskRunStatus{ + Status: duckv1.Status{ + Conditions: duckv1.Conditions{ + {Type: apis.ConditionSucceeded, Status: corev1.ConditionUnknown}, + }, + }, + }, + }, + taskRun: &v1beta1.TaskRun{ + Spec: v1beta1.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: &v1beta1.TaskRun{ + Spec: v1beta1.TaskRunSpec{ + Status: "", + StatusMessage: "", + Timeout: &metav1.Duration{Duration: 0}, + }, + Status: v1beta1.TaskRunStatus{ + Status: duckv1.Status{ + Conditions: duckv1.Conditions{ + {Type: apis.ConditionSucceeded, Status: corev1.ConditionUnknown}, + }, + }, + }, + }, + taskRun: &v1beta1.TaskRun{ + Spec: v1beta1.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: &v1beta1.TaskRun{ + Spec: v1beta1.TaskRunSpec{ + Status: "", + }, + Status: v1beta1.TaskRunStatus{ + Status: duckv1.Status{ + Conditions: duckv1.Conditions{ + {Type: apis.ConditionSucceeded, Status: corev1.ConditionTrue}, + }, + }, + }, + }, + taskRun: &v1beta1.TaskRun{ + Spec: v1beta1.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)) + } + }) + } +}