diff --git a/pkg/apis/pipeline/v1/pipelinerun_validation.go b/pkg/apis/pipeline/v1/pipelinerun_validation.go index 37c977197b0..16330aa2153 100644 --- a/pkg/apis/pipeline/v1/pipelinerun_validation.go +++ b/pkg/apis/pipeline/v1/pipelinerun_validation.go @@ -27,6 +27,7 @@ import ( "github.com/tektoncd/pipeline/pkg/apis/validate" "github.com/tektoncd/pipeline/pkg/internal/resultref" admissionregistrationv1 "k8s.io/api/admissionregistration/v1" + "k8s.io/apimachinery/pkg/api/equality" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "knative.dev/pkg/apis" "knative.dev/pkg/webhook/resourcesemantics" @@ -55,6 +56,9 @@ func (pr *PipelineRun) Validate(ctx context.Context) *apis.FieldError { // Validate pipelinerun spec func (ps *PipelineRunSpec) Validate(ctx context.Context) (errs *apis.FieldError) { + // Validate the spec changes + errs = errs.Also(ps.ValidateUpdate(ctx)) + // Must have exactly one of pipelineRef and pipelineSpec. if ps.PipelineRef == nil && ps.PipelineSpec == nil { errs = errs.Also(apis.ErrMissingOneOf("pipelineRef", "pipelineSpec")) @@ -124,6 +128,31 @@ func (ps *PipelineRunSpec) Validate(ctx context.Context) (errs *apis.FieldError) return errs } +// ValidateUpdate validates the update of a PipelineRunSpec +func (ps *PipelineRunSpec) ValidateUpdate(ctx context.Context) (errs *apis.FieldError) { + if !apis.IsInUpdate(ctx) { + return + } + oldObj, ok := apis.GetBaseline(ctx).(*PipelineRun) + if !ok || oldObj == nil { + return + } + old := &oldObj.Spec + + // If already in the done state, the spec cannot be modified. Otherwise, only the status field can be modified. + tips := "Once the PipelineRun is complete, no updates are allowed" + if !oldObj.IsDone() { + old = old.DeepCopy() + old.Status = ps.Status + tips = "Once the PipelineRun has started, only status updates are allowed" + } + if !equality.Semantic.DeepEqual(old, ps) { + errs = errs.Also(apis.ErrInvalidValue(tips, "")) + } + + return +} + func (ps *PipelineRunSpec) validatePipelineRunParameters(ctx context.Context) (errs *apis.FieldError) { if len(ps.Params) == 0 { return errs diff --git a/pkg/apis/pipeline/v1/pipelinerun_validation_test.go b/pkg/apis/pipeline/v1/pipelinerun_validation_test.go index 2962ddf6c81..42a38ed7c3f 100644 --- a/pkg/apis/pipeline/v1/pipelinerun_validation_test.go +++ b/pkg/apis/pipeline/v1/pipelinerun_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 TestPipelineRun_Invalid(t *testing.T) { @@ -1511,3 +1513,180 @@ func TestPipelineRunSpecBetaFeatures(t *testing.T) { }) } } + +func TestPipelineRunSpec_ValidateUpdate(t *testing.T) { + tests := []struct { + name string + isCreate bool + isUpdate bool + baselinePipelineRun *v1.PipelineRun + pipelineRun *v1.PipelineRun + expectedError apis.FieldError + }{ + { + name: "is create ctx", + pipelineRun: &v1.PipelineRun{ + Spec: v1.PipelineRunSpec{}, + }, + isCreate: true, + isUpdate: false, + expectedError: apis.FieldError{}, + }, { + name: "is update ctx, no changes", + baselinePipelineRun: &v1.PipelineRun{ + Spec: v1.PipelineRunSpec{ + Status: "", + }, + }, + pipelineRun: &v1.PipelineRun{ + Spec: v1.PipelineRunSpec{ + Status: "", + }, + }, + isCreate: false, + isUpdate: true, + expectedError: apis.FieldError{}, + }, { + name: "is update ctx, baseline is nil, skip validation", + baselinePipelineRun: nil, + pipelineRun: &v1.PipelineRun{ + Spec: v1.PipelineRunSpec{ + Timeouts: &v1.TimeoutFields{ + Pipeline: &metav1.Duration{Duration: 1}, + }, + }, + }, + isCreate: false, + isUpdate: true, + expectedError: apis.FieldError{}, + }, { + name: "is update ctx, baseline is unknown, status changes from Empty to Cancelled", + baselinePipelineRun: &v1.PipelineRun{ + Spec: v1.PipelineRunSpec{ + Status: "", + }, + Status: v1.PipelineRunStatus{ + Status: duckv1.Status{ + Conditions: duckv1.Conditions{ + {Type: apis.ConditionSucceeded, Status: corev1.ConditionUnknown}, + }, + }, + }, + }, + pipelineRun: &v1.PipelineRun{ + Spec: v1.PipelineRunSpec{ + Status: "Cancelled", + }, + }, + isCreate: false, + isUpdate: true, + expectedError: apis.FieldError{}, + }, { + name: "is update ctx, baseline is unknown, timeouts changes", + baselinePipelineRun: &v1.PipelineRun{ + Spec: v1.PipelineRunSpec{ + Status: "", + Timeouts: &v1.TimeoutFields{ + Pipeline: &metav1.Duration{Duration: 0}, + }, + }, + Status: v1.PipelineRunStatus{ + Status: duckv1.Status{ + Conditions: duckv1.Conditions{ + {Type: apis.ConditionSucceeded, Status: corev1.ConditionUnknown}, + }, + }, + }, + }, + pipelineRun: &v1.PipelineRun{ + Spec: v1.PipelineRunSpec{ + Timeouts: &v1.TimeoutFields{ + Pipeline: &metav1.Duration{Duration: 1}, + }, + }, + }, + isCreate: false, + isUpdate: true, + expectedError: apis.FieldError{ + Message: `invalid value: Once the PipelineRun has started, only status updates are allowed`, + Paths: []string{""}, + }, + }, { + name: "is update ctx, baseline is unknown, status changes from PipelineRunPending to Empty, and timeouts changes", + baselinePipelineRun: &v1.PipelineRun{ + Spec: v1.PipelineRunSpec{ + Status: "PipelineRunPending", + Timeouts: &v1.TimeoutFields{ + Pipeline: &metav1.Duration{Duration: 0}, + }, + }, + Status: v1.PipelineRunStatus{ + Status: duckv1.Status{ + Conditions: duckv1.Conditions{ + {Type: apis.ConditionSucceeded, Status: corev1.ConditionUnknown}, + }, + }, + }, + }, + pipelineRun: &v1.PipelineRun{ + Spec: v1.PipelineRunSpec{ + Status: "", + Timeouts: &v1.TimeoutFields{ + Pipeline: &metav1.Duration{Duration: 1}, + }, + }, + }, + isCreate: false, + isUpdate: true, + expectedError: apis.FieldError{ + Message: `invalid value: Once the PipelineRun has started, only status updates are allowed`, + Paths: []string{""}, + }, + }, { + name: "is update ctx, baseline is done, status changes", + baselinePipelineRun: &v1.PipelineRun{ + Spec: v1.PipelineRunSpec{ + Status: "PipelineRunPending", + }, + Status: v1.PipelineRunStatus{ + Status: duckv1.Status{ + Conditions: duckv1.Conditions{ + {Type: apis.ConditionSucceeded, Status: corev1.ConditionTrue}, + }, + }, + }, + }, + pipelineRun: &v1.PipelineRun{ + Spec: v1.PipelineRunSpec{ + Status: "TaskRunCancelled", + }, + }, + isCreate: false, + isUpdate: true, + expectedError: apis.FieldError{ + Message: `invalid value: Once the PipelineRun 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.baselinePipelineRun) + } + pr := tt.pipelineRun + err := pr.Spec.ValidateUpdate(ctx) + if d := cmp.Diff(tt.expectedError.Error(), err.Error(), cmpopts.IgnoreUnexported(apis.FieldError{})); d != "" { + t.Errorf("PipelineRunSpec.ValidateUpdate() errors diff %s", diff.PrintWantGot(d)) + } + }) + } +} diff --git a/pkg/apis/pipeline/v1beta1/pipelinerun_validation.go b/pkg/apis/pipeline/v1beta1/pipelinerun_validation.go index 31fe4fb7281..834c7493df4 100644 --- a/pkg/apis/pipeline/v1beta1/pipelinerun_validation.go +++ b/pkg/apis/pipeline/v1beta1/pipelinerun_validation.go @@ -26,6 +26,7 @@ import ( "github.com/tektoncd/pipeline/pkg/apis/validate" "github.com/tektoncd/pipeline/pkg/internal/resultref" admissionregistrationv1 "k8s.io/api/admissionregistration/v1" + "k8s.io/apimachinery/pkg/api/equality" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/utils/strings/slices" @@ -60,6 +61,9 @@ func (pr *PipelineRun) Validate(ctx context.Context) *apis.FieldError { // Validate pipelinerun spec func (ps *PipelineRunSpec) Validate(ctx context.Context) (errs *apis.FieldError) { + // Validate the spec changes + errs = errs.Also(ps.ValidateUpdate(ctx)) + // Must have exactly one of pipelineRef and pipelineSpec. if ps.PipelineRef == nil && ps.PipelineSpec == nil { errs = errs.Also(apis.ErrMissingOneOf("pipelineRef", "pipelineSpec")) @@ -145,6 +149,31 @@ func (ps *PipelineRunSpec) Validate(ctx context.Context) (errs *apis.FieldError) return errs } +// ValidateUpdate validates the update of a PipelineRunSpec +func (ps *PipelineRunSpec) ValidateUpdate(ctx context.Context) (errs *apis.FieldError) { + if !apis.IsInUpdate(ctx) { + return + } + oldObj, ok := apis.GetBaseline(ctx).(*PipelineRun) + if !ok || oldObj == nil { + return + } + old := &oldObj.Spec + + // If already in the done state, the spec cannot be modified. Otherwise, only the status field can be modified. + tips := "Once the PipelineRun is complete, no updates are allowed" + if !oldObj.IsDone() { + old = old.DeepCopy() + old.Status = ps.Status + tips = "Once the PipelineRun has started, only status updates are allowed" + } + if !equality.Semantic.DeepEqual(old, ps) { + errs = errs.Also(apis.ErrInvalidValue(tips, "")) + } + + return +} + func (ps *PipelineRunSpec) validatePipelineRunParameters(ctx context.Context) (errs *apis.FieldError) { if len(ps.Params) == 0 { return errs diff --git a/pkg/apis/pipeline/v1beta1/pipelinerun_validation_test.go b/pkg/apis/pipeline/v1beta1/pipelinerun_validation_test.go index 2d8fb19833e..1d7199fddee 100644 --- a/pkg/apis/pipeline/v1beta1/pipelinerun_validation_test.go +++ b/pkg/apis/pipeline/v1beta1/pipelinerun_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 TestPipelineRun_Invalid(t *testing.T) { @@ -1692,3 +1694,179 @@ func TestPipelineRunSpecBetaFeatures(t *testing.T) { }) } } +func TestPipelineRunSpec_ValidateUpdate(t *testing.T) { + tests := []struct { + name string + isCreate bool + isUpdate bool + baselinePipelineRun *v1beta1.PipelineRun + pipelineRun *v1beta1.PipelineRun + expectedError apis.FieldError + }{ + { + name: "is create ctx", + pipelineRun: &v1beta1.PipelineRun{ + Spec: v1beta1.PipelineRunSpec{}, + }, + isCreate: true, + isUpdate: false, + expectedError: apis.FieldError{}, + }, { + name: "is update ctx, no changes", + baselinePipelineRun: &v1beta1.PipelineRun{ + Spec: v1beta1.PipelineRunSpec{ + Status: "", + }, + }, + pipelineRun: &v1beta1.PipelineRun{ + Spec: v1beta1.PipelineRunSpec{ + Status: "", + }, + }, + isCreate: false, + isUpdate: true, + expectedError: apis.FieldError{}, + }, { + name: "is update ctx, baseline is nil, skip validation", + baselinePipelineRun: nil, + pipelineRun: &v1beta1.PipelineRun{ + Spec: v1beta1.PipelineRunSpec{ + Timeouts: &v1beta1.TimeoutFields{ + Pipeline: &metav1.Duration{Duration: 1}, + }, + }, + }, + isCreate: false, + isUpdate: true, + expectedError: apis.FieldError{}, + }, { + name: "is update ctx, baseline is unknown, status changes from Empty to Cancelled", + baselinePipelineRun: &v1beta1.PipelineRun{ + Spec: v1beta1.PipelineRunSpec{ + Status: "", + }, + Status: v1beta1.PipelineRunStatus{ + Status: duckv1.Status{ + Conditions: duckv1.Conditions{ + {Type: apis.ConditionSucceeded, Status: corev1.ConditionUnknown}, + }, + }, + }, + }, + pipelineRun: &v1beta1.PipelineRun{ + Spec: v1beta1.PipelineRunSpec{ + Status: "Cancelled", + }, + }, + isCreate: false, + isUpdate: true, + expectedError: apis.FieldError{}, + }, { + name: "is update ctx, baseline is unknown, timeouts changes", + baselinePipelineRun: &v1beta1.PipelineRun{ + Spec: v1beta1.PipelineRunSpec{ + Status: "", + Timeouts: &v1beta1.TimeoutFields{ + Pipeline: &metav1.Duration{Duration: 0}, + }, + }, + Status: v1beta1.PipelineRunStatus{ + Status: duckv1.Status{ + Conditions: duckv1.Conditions{ + {Type: apis.ConditionSucceeded, Status: corev1.ConditionUnknown}, + }, + }, + }, + }, + pipelineRun: &v1beta1.PipelineRun{ + Spec: v1beta1.PipelineRunSpec{ + Timeouts: &v1beta1.TimeoutFields{ + Pipeline: &metav1.Duration{Duration: 1}, + }, + }, + }, + isCreate: false, + isUpdate: true, + expectedError: apis.FieldError{ + Message: `invalid value: Once the PipelineRun has started, only status updates are allowed`, + Paths: []string{""}, + }, + }, { + name: "is update ctx, baseline is unknown, status changes from PipelineRunPending to Empty, and timeouts changes", + baselinePipelineRun: &v1beta1.PipelineRun{ + Spec: v1beta1.PipelineRunSpec{ + Status: "PipelineRunPending", + Timeouts: &v1beta1.TimeoutFields{ + Pipeline: &metav1.Duration{Duration: 0}, + }, + }, + Status: v1beta1.PipelineRunStatus{ + Status: duckv1.Status{ + Conditions: duckv1.Conditions{ + {Type: apis.ConditionSucceeded, Status: corev1.ConditionUnknown}, + }, + }, + }, + }, + pipelineRun: &v1beta1.PipelineRun{ + Spec: v1beta1.PipelineRunSpec{ + Status: "", + Timeouts: &v1beta1.TimeoutFields{ + Pipeline: &metav1.Duration{Duration: 1}, + }, + }, + }, + isCreate: false, + isUpdate: true, + expectedError: apis.FieldError{ + Message: `invalid value: Once the PipelineRun has started, only status updates are allowed`, + Paths: []string{""}, + }, + }, { + name: "is update ctx, baseline is done, status changes", + baselinePipelineRun: &v1beta1.PipelineRun{ + Spec: v1beta1.PipelineRunSpec{ + Status: "PipelineRunPending", + }, + Status: v1beta1.PipelineRunStatus{ + Status: duckv1.Status{ + Conditions: duckv1.Conditions{ + {Type: apis.ConditionSucceeded, Status: corev1.ConditionTrue}, + }, + }, + }, + }, + pipelineRun: &v1beta1.PipelineRun{ + Spec: v1beta1.PipelineRunSpec{ + Status: "TaskRunCancelled", + }, + }, + isCreate: false, + isUpdate: true, + expectedError: apis.FieldError{ + Message: `invalid value: Once the PipelineRun 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.baselinePipelineRun) + } + pr := tt.pipelineRun + err := pr.Spec.ValidateUpdate(ctx) + if d := cmp.Diff(tt.expectedError.Error(), err.Error(), cmpopts.IgnoreUnexported(apis.FieldError{})); d != "" { + t.Errorf("PipelineRunSpec.ValidateUpdate() errors diff %s", diff.PrintWantGot(d)) + } + }) + } +}