From 3926b660ce553af933020cda279d3bf70a1df954 Mon Sep 17 00:00:00 2001 From: Vincent Demeester Date: Wed, 12 May 2021 11:31:38 +0200 Subject: [PATCH] =?UTF-8?q?Skip=20*heavy*=20validation=20on=20deletion=20?= =?UTF-8?q?=F0=9F=99=83?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When deleting an object, we don't need to pursue all the validation that we do at creation. It reduces the work to be done as part of the validation *and* allows invalid version of the resource (from previous versions for example) to be deleted safely. Signed-off-by: Vincent Demeester --- .../v1alpha1/cluster_task_validation.go | 3 + .../pipeline/v1alpha1/condition_validation.go | 3 + .../pipeline/v1alpha1/pipeline_validation.go | 3 + .../v1alpha1/pipelinerun_validation.go | 3 + pkg/apis/pipeline/v1alpha1/run_validation.go | 3 + pkg/apis/pipeline/v1alpha1/task_validation.go | 3 + .../pipeline/v1alpha1/taskrun_validation.go | 3 + .../v1beta1/cluster_task_validation.go | 3 + .../pipeline/v1beta1/pipeline_validation.go | 3 + .../v1beta1/pipeline_validation_test.go | 6 ++ .../v1beta1/pipelinerun_validation.go | 4 ++ .../v1beta1/pipelinerun_validation_test.go | 61 ++++++++++++------- pkg/apis/pipeline/v1beta1/task_validation.go | 3 + .../pipeline/v1beta1/task_validation_test.go | 26 ++++++++ .../pipeline/v1beta1/taskrun_validation.go | 3 + .../v1beta1/taskrun_validation_test.go | 49 ++++++++++----- .../v1alpha1/pipelineresource_validation.go | 4 +- 17 files changed, 146 insertions(+), 37 deletions(-) diff --git a/pkg/apis/pipeline/v1alpha1/cluster_task_validation.go b/pkg/apis/pipeline/v1alpha1/cluster_task_validation.go index b90a7f3b978..eaf1c04bc16 100644 --- a/pkg/apis/pipeline/v1alpha1/cluster_task_validation.go +++ b/pkg/apis/pipeline/v1alpha1/cluster_task_validation.go @@ -29,5 +29,8 @@ func (t *ClusterTask) Validate(ctx context.Context) *apis.FieldError { if err := validate.ObjectMetadata(t.GetObjectMeta()); err != nil { return err.ViaField("metadata") } + if apis.IsInDelete(ctx) { + return nil + } return t.Spec.Validate(ctx) } diff --git a/pkg/apis/pipeline/v1alpha1/condition_validation.go b/pkg/apis/pipeline/v1alpha1/condition_validation.go index dbafbba3a61..1aed47511cb 100644 --- a/pkg/apis/pipeline/v1alpha1/condition_validation.go +++ b/pkg/apis/pipeline/v1alpha1/condition_validation.go @@ -32,6 +32,9 @@ func (c Condition) Validate(ctx context.Context) *apis.FieldError { if err := validate.ObjectMetadata(c.GetObjectMeta()); err != nil { return err.ViaField("metadata") } + if apis.IsInDelete(ctx) { + return nil + } return c.Spec.Validate(ctx).ViaField("Spec") } diff --git a/pkg/apis/pipeline/v1alpha1/pipeline_validation.go b/pkg/apis/pipeline/v1alpha1/pipeline_validation.go index 4e56b98e12d..40308bbce06 100644 --- a/pkg/apis/pipeline/v1alpha1/pipeline_validation.go +++ b/pkg/apis/pipeline/v1alpha1/pipeline_validation.go @@ -39,6 +39,9 @@ func (p *Pipeline) Validate(ctx context.Context) *apis.FieldError { if err := validate.ObjectMetadata(p.GetObjectMeta()); err != nil { return err.ViaField("metadata") } + if apis.IsInDelete(ctx) { + return nil + } return p.Spec.Validate(ctx) } diff --git a/pkg/apis/pipeline/v1alpha1/pipelinerun_validation.go b/pkg/apis/pipeline/v1alpha1/pipelinerun_validation.go index 249e0465449..f6396cb4337 100644 --- a/pkg/apis/pipeline/v1alpha1/pipelinerun_validation.go +++ b/pkg/apis/pipeline/v1alpha1/pipelinerun_validation.go @@ -32,6 +32,9 @@ func (pr *PipelineRun) Validate(ctx context.Context) *apis.FieldError { if err := validate.ObjectMetadata(pr.GetObjectMeta()).ViaField("metadata"); err != nil { return err } + if apis.IsInDelete(ctx) { + return nil + } return pr.Spec.Validate(ctx) } diff --git a/pkg/apis/pipeline/v1alpha1/run_validation.go b/pkg/apis/pipeline/v1alpha1/run_validation.go index 2a617e4a554..fe011f08b0f 100644 --- a/pkg/apis/pipeline/v1alpha1/run_validation.go +++ b/pkg/apis/pipeline/v1alpha1/run_validation.go @@ -31,6 +31,9 @@ func (r *Run) Validate(ctx context.Context) *apis.FieldError { if err := validate.ObjectMetadata(r.GetObjectMeta()).ViaField("metadata"); err != nil { return err } + if apis.IsInDelete(ctx) { + return nil + } return r.Spec.Validate(ctx) } diff --git a/pkg/apis/pipeline/v1alpha1/task_validation.go b/pkg/apis/pipeline/v1alpha1/task_validation.go index 549cc53ee1e..e0a3b3dadfa 100644 --- a/pkg/apis/pipeline/v1alpha1/task_validation.go +++ b/pkg/apis/pipeline/v1alpha1/task_validation.go @@ -37,6 +37,9 @@ func (t *Task) Validate(ctx context.Context) *apis.FieldError { if err := validate.ObjectMetadata(t.GetObjectMeta()); err != nil { return err.ViaField("metadata") } + if apis.IsInDelete(ctx) { + return nil + } return t.Spec.Validate(ctx) } diff --git a/pkg/apis/pipeline/v1alpha1/taskrun_validation.go b/pkg/apis/pipeline/v1alpha1/taskrun_validation.go index b1f2cace2d3..6ef2668c118 100644 --- a/pkg/apis/pipeline/v1alpha1/taskrun_validation.go +++ b/pkg/apis/pipeline/v1alpha1/taskrun_validation.go @@ -34,6 +34,9 @@ func (tr *TaskRun) Validate(ctx context.Context) *apis.FieldError { if err := validate.ObjectMetadata(tr.GetObjectMeta()).ViaField("metadata"); err != nil { return err } + if apis.IsInDelete(ctx) { + return nil + } return tr.Spec.Validate(ctx) } diff --git a/pkg/apis/pipeline/v1beta1/cluster_task_validation.go b/pkg/apis/pipeline/v1beta1/cluster_task_validation.go index cd602e91ac2..3cdb3fca90c 100644 --- a/pkg/apis/pipeline/v1beta1/cluster_task_validation.go +++ b/pkg/apis/pipeline/v1beta1/cluster_task_validation.go @@ -27,5 +27,8 @@ var _ apis.Validatable = (*ClusterTask)(nil) func (t *ClusterTask) Validate(ctx context.Context) *apis.FieldError { errs := validate.ObjectMetadata(t.GetObjectMeta()).ViaField("metadata") + if apis.IsInDelete(ctx) { + return nil + } return errs.Also(t.Spec.Validate(apis.WithinSpec(ctx)).ViaField("spec")) } diff --git a/pkg/apis/pipeline/v1beta1/pipeline_validation.go b/pkg/apis/pipeline/v1beta1/pipeline_validation.go index 6eb248c8afe..da5a37cb08f 100644 --- a/pkg/apis/pipeline/v1beta1/pipeline_validation.go +++ b/pkg/apis/pipeline/v1beta1/pipeline_validation.go @@ -36,6 +36,9 @@ var _ apis.Validatable = (*Pipeline)(nil) // that any references resources exist, that is done at run time. func (p *Pipeline) Validate(ctx context.Context) *apis.FieldError { errs := validate.ObjectMetadata(p.GetObjectMeta()).ViaField("metadata") + if apis.IsInDelete(ctx) { + return nil + } return errs.Also(p.Spec.Validate(apis.WithinSpec(ctx)).ViaField("spec")) } diff --git a/pkg/apis/pipeline/v1beta1/pipeline_validation_test.go b/pkg/apis/pipeline/v1beta1/pipeline_validation_test.go index fb39e31b189..8639375e005 100644 --- a/pkg/apis/pipeline/v1beta1/pipeline_validation_test.go +++ b/pkg/apis/pipeline/v1beta1/pipeline_validation_test.go @@ -132,6 +132,12 @@ func TestPipeline_Validate_Success(t *testing.T) { }}, }, }, + }, { + name: "do not validate spec on delete", + p: &Pipeline{ + ObjectMeta: metav1.ObjectMeta{Name: "pipeline"}, + }, + wc: apis.WithinDelete, }} for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { diff --git a/pkg/apis/pipeline/v1beta1/pipelinerun_validation.go b/pkg/apis/pipeline/v1beta1/pipelinerun_validation.go index 97061b3e7df..446241c9798 100644 --- a/pkg/apis/pipeline/v1beta1/pipelinerun_validation.go +++ b/pkg/apis/pipeline/v1beta1/pipelinerun_validation.go @@ -32,6 +32,10 @@ var _ apis.Validatable = (*PipelineRun)(nil) func (pr *PipelineRun) Validate(ctx context.Context) *apis.FieldError { errs := validate.ObjectMetadata(pr.GetObjectMeta()).ViaField("metadata") + if apis.IsInDelete(ctx) { + return nil + } + if pr.IsPending() && pr.HasStarted() { errs = errs.Also(apis.ErrInvalidValue("PipelineRun cannot be Pending after it is started", "spec.status")) } diff --git a/pkg/apis/pipeline/v1beta1/pipelinerun_validation_test.go b/pkg/apis/pipeline/v1beta1/pipelinerun_validation_test.go index ae0fa5108c9..bac3739825d 100644 --- a/pkg/apis/pipeline/v1beta1/pipelinerun_validation_test.go +++ b/pkg/apis/pipeline/v1beta1/pipelinerun_validation_test.go @@ -136,31 +136,29 @@ func TestPipelineRun_Invalid(t *testing.T) { }, want: apis.ErrInvalidValue("invalid bundle reference (could not parse reference: not a valid reference)", "spec.pipelineref.bundle"), wc: enableTektonOCIBundles(t), - }, - { - name: "pipelinerun pending while running", - pr: v1beta1.PipelineRun{ - ObjectMeta: metav1.ObjectMeta{ - Name: "pipelinerunname", - }, - Spec: v1beta1.PipelineRunSpec{ - Status: v1beta1.PipelineRunSpecStatusPending, - PipelineRef: &v1beta1.PipelineRef{ - Name: "prname", - }, - }, - Status: v1beta1.PipelineRunStatus{ - PipelineRunStatusFields: v1beta1.PipelineRunStatusFields{ - StartTime: &metav1.Time{time.Now()}, - }, + }, { + name: "pipelinerun pending while running", + pr: v1beta1.PipelineRun{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pipelinerunname", + }, + Spec: v1beta1.PipelineRunSpec{ + Status: v1beta1.PipelineRunSpecStatusPending, + PipelineRef: &v1beta1.PipelineRef{ + Name: "prname", }, }, - want: &apis.FieldError{ - Message: "invalid value: PipelineRun cannot be Pending after it is started", - Paths: []string{"spec.status"}, + Status: v1beta1.PipelineRunStatus{ + PipelineRunStatusFields: v1beta1.PipelineRunStatusFields{ + StartTime: &metav1.Time{time.Now()}, + }, }, }, - } + want: &apis.FieldError{ + Message: "invalid value: PipelineRun cannot be Pending after it is started", + Paths: []string{"spec.status"}, + }, + }} for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { @@ -180,6 +178,7 @@ func TestPipelineRun_Validate(t *testing.T) { tests := []struct { name string pr v1beta1.PipelineRun + wc func(context.Context) context.Context }{{ name: "normal case", pr: v1beta1.PipelineRun{ @@ -256,11 +255,29 @@ func TestPipelineRun_Validate(t *testing.T) { }, }, }, + }, { + name: "do not validate spec on delete", + pr: v1beta1.PipelineRun{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pipelinelinename", + }, + Spec: v1beta1.PipelineRunSpec{ + PipelineRef: &v1beta1.PipelineRef{ + Name: "prname", + }, + Timeout: &metav1.Duration{Duration: -48 * time.Hour}, + }, + }, + wc: apis.WithinDelete, }} for _, ts := range tests { t.Run(ts.name, func(t *testing.T) { - if err := ts.pr.Validate(context.Background()); err != nil { + ctx := context.Background() + if ts.wc != nil { + ctx = ts.wc(ctx) + } + if err := ts.pr.Validate(ctx); err != nil { t.Error(err) } }) diff --git a/pkg/apis/pipeline/v1beta1/task_validation.go b/pkg/apis/pipeline/v1beta1/task_validation.go index 16846842a92..0ca71ff5883 100644 --- a/pkg/apis/pipeline/v1beta1/task_validation.go +++ b/pkg/apis/pipeline/v1beta1/task_validation.go @@ -36,6 +36,9 @@ var _ apis.Validatable = (*Task)(nil) func (t *Task) Validate(ctx context.Context) *apis.FieldError { errs := validate.ObjectMetadata(t.GetObjectMeta()).ViaField("metadata") + if apis.IsInDelete(ctx) { + return nil + } return errs.Also(t.Spec.Validate(apis.WithinSpec(ctx)).ViaField("spec")) } diff --git a/pkg/apis/pipeline/v1beta1/task_validation_test.go b/pkg/apis/pipeline/v1beta1/task_validation_test.go index e0c4e7ff1f5..93adc6945c0 100644 --- a/pkg/apis/pipeline/v1beta1/task_validation_test.go +++ b/pkg/apis/pipeline/v1beta1/task_validation_test.go @@ -56,6 +56,32 @@ var invalidSteps = []v1beta1.Step{{Container: corev1.Container{ Image: "myimage", }}} +func TestTaskValidate(t *testing.T) { + tests := []struct { + name string + t *v1beta1.Task + wc func(context.Context) context.Context + }{{ + name: "do not validate spec on delete", + t: &v1beta1.Task{ + ObjectMeta: metav1.ObjectMeta{Name: "task"}, + }, + wc: apis.WithinDelete, + }} + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctx := context.Background() + if tt.wc != nil { + ctx = tt.wc(ctx) + } + err := tt.t.Validate(ctx) + if err != nil { + t.Errorf("Task.Validate() returned error for valid Task: %v", err) + } + }) + } +} + func TestTaskSpecValidate(t *testing.T) { type fields struct { Params []v1beta1.ParamSpec diff --git a/pkg/apis/pipeline/v1beta1/taskrun_validation.go b/pkg/apis/pipeline/v1beta1/taskrun_validation.go index 52dbcecb247..b2d6bf50bc4 100644 --- a/pkg/apis/pipeline/v1beta1/taskrun_validation.go +++ b/pkg/apis/pipeline/v1beta1/taskrun_validation.go @@ -33,6 +33,9 @@ var _ apis.Validatable = (*TaskRun)(nil) // Validate taskrun func (tr *TaskRun) Validate(ctx context.Context) *apis.FieldError { errs := validate.ObjectMetadata(tr.GetObjectMeta()).ViaField("metadata") + if apis.IsInDelete(ctx) { + return nil + } return errs.Also(tr.Spec.Validate(apis.WithinSpec(ctx)).ViaField("spec")) } diff --git a/pkg/apis/pipeline/v1beta1/taskrun_validation_test.go b/pkg/apis/pipeline/v1beta1/taskrun_validation_test.go index b1a630a49b0..e0fbd3a59a1 100644 --- a/pkg/apis/pipeline/v1beta1/taskrun_validation_test.go +++ b/pkg/apis/pipeline/v1beta1/taskrun_validation_test.go @@ -32,18 +32,18 @@ import ( func TestTaskRun_Invalidate(t *testing.T) { tests := []struct { - name string - task *v1beta1.TaskRun - want *apis.FieldError + name string + taskRun *v1beta1.TaskRun + want *apis.FieldError }{{ - name: "invalid taskspec", - task: &v1beta1.TaskRun{}, + name: "invalid taskspec", + taskRun: &v1beta1.TaskRun{}, want: apis.ErrMissingField("spec.taskref.name", "spec.taskspec").Also( apis.ErrGeneric(`invalid resource name "": must be a valid DNS label`, "metadata.name")), }} for _, ts := range tests { t.Run(ts.name, func(t *testing.T) { - err := ts.task.Validate(context.Background()) + err := ts.taskRun.Validate(context.Background()) if d := cmp.Diff(err.Error(), ts.want.Error()); d != "" { t.Error(diff.PrintWantGot(d)) } @@ -52,16 +52,37 @@ func TestTaskRun_Invalidate(t *testing.T) { } func TestTaskRun_Validate(t *testing.T) { - tr := &v1beta1.TaskRun{ - ObjectMeta: metav1.ObjectMeta{ - Name: "taskrname", + tests := []struct { + name string + taskRun *v1beta1.TaskRun + wc func(context.Context) context.Context + }{{ + name: "simple taskref", + taskRun: &v1beta1.TaskRun{ + ObjectMeta: metav1.ObjectMeta{ + Name: "taskrname", + }, + Spec: v1beta1.TaskRunSpec{ + TaskRef: &v1beta1.TaskRef{Name: "taskrefname"}, + }, }, - Spec: v1beta1.TaskRunSpec{ - TaskRef: &v1beta1.TaskRef{Name: "taskrefname"}, + }, { + name: "do not validate spec on delete", + taskRun: &v1beta1.TaskRun{ + ObjectMeta: metav1.ObjectMeta{Name: "taskrname"}, }, - } - if err := tr.Validate(context.Background()); err != nil { - t.Errorf("TaskRun.Validate() error = %v", err) + wc: apis.WithinDelete, + }} + for _, ts := range tests { + t.Run(ts.name, func(t *testing.T) { + ctx := context.Background() + if ts.wc != nil { + ctx = ts.wc(ctx) + } + if err := ts.taskRun.Validate(ctx); err != nil { + t.Errorf("TaskRun.Validate() error = %v", err) + } + }) } } diff --git a/pkg/apis/resource/v1alpha1/pipelineresource_validation.go b/pkg/apis/resource/v1alpha1/pipelineresource_validation.go index c1c217acd0a..a7b21401744 100644 --- a/pkg/apis/resource/v1alpha1/pipelineresource_validation.go +++ b/pkg/apis/resource/v1alpha1/pipelineresource_validation.go @@ -34,7 +34,9 @@ func (r *PipelineResource) Validate(ctx context.Context) *apis.FieldError { if err := validate.ObjectMetadata(r.GetObjectMeta()); err != nil { return err.ViaField("metadata") } - + if apis.IsInDelete(ctx) { + return nil + } return r.Spec.Validate(ctx) }