diff --git a/pkg/apis/pipeline/v1/pipeline_validation.go b/pkg/apis/pipeline/v1/pipeline_validation.go index 0fb1fe3ef2d..f11f818e4a9 100644 --- a/pkg/apis/pipeline/v1/pipeline_validation.go +++ b/pkg/apis/pipeline/v1/pipeline_validation.go @@ -70,7 +70,7 @@ func (ps *PipelineSpec) Validate(ctx context.Context) (errs *apis.FieldError) { errs = errs.Also(validatePipelineContextVariables(ps.Tasks).ViaField("tasks")) errs = errs.Also(validatePipelineContextVariables(ps.Finally).ViaField("finally")) errs = errs.Also(validateExecutionStatusVariables(ps.Tasks, ps.Finally)) - errs = errs.Also(ps.validateBetaFields(ctx)) + errs = errs.Also(ps.ValidateBetaFields(ctx)) // Validate the pipeline's workspaces. errs = errs.Also(validatePipelineWorkspacesDeclarations(ps.Workspaces)) // Validate the pipeline's results @@ -84,9 +84,9 @@ func (ps *PipelineSpec) Validate(ctx context.Context) (errs *apis.FieldError) { return errs } -// validateBetaFields returns an error if the Pipeline spec uses beta features but does not +// ValidateBetaFields returns an error if the Pipeline spec uses beta features but does not // have "enable-api-fields" set to "alpha" or "beta". -func (ps *PipelineSpec) validateBetaFields(ctx context.Context) *apis.FieldError { +func (ps *PipelineSpec) ValidateBetaFields(ctx context.Context) *apis.FieldError { var errs *apis.FieldError // Object parameters for i, p := range ps.Params { @@ -133,7 +133,7 @@ func (pt *PipelineTask) validateBetaFields(ctx context.Context) *apis.FieldError errs = errs.Also(version.ValidateEnabledAPIFields(ctx, "taskref.params", config.BetaAPIFields)) } } else if pt.TaskSpec != nil { - errs = errs.Also(pt.TaskSpec.validateBetaFields(ctx)) + errs = errs.Also(pt.TaskSpec.ValidateBetaFields(ctx)) } return errs } diff --git a/pkg/apis/pipeline/v1/task_validation.go b/pkg/apis/pipeline/v1/task_validation.go index da95dc04cd9..ecd2d6125d1 100644 --- a/pkg/apis/pipeline/v1/task_validation.go +++ b/pkg/apis/pipeline/v1/task_validation.go @@ -90,16 +90,16 @@ func (ts *TaskSpec) Validate(ctx context.Context) (errs *apis.FieldError) { errs = errs.Also(validateSidecarNames(ts.Sidecars)) errs = errs.Also(ValidateParameterTypes(ctx, ts.Params).ViaField("params")) errs = errs.Also(ValidateParameterVariables(ctx, ts.Steps, ts.Params)) - errs = errs.Also(ts.validateBetaFields(ctx)) + errs = errs.Also(ts.ValidateBetaFields(ctx)) errs = errs.Also(validateTaskContextVariables(ctx, ts.Steps)) errs = errs.Also(validateTaskResultsVariables(ctx, ts.Steps, ts.Results)) errs = errs.Also(validateResults(ctx, ts.Results).ViaField("results")) return errs } -// validateBetaFields returns an error if the Task spec uses beta features but does not +// ValidateBetaFields returns an error if the Task spec uses beta features but does not // have "enable-api-fields" set to "alpha" or "beta". -func (ts *TaskSpec) validateBetaFields(ctx context.Context) *apis.FieldError { +func (ts *TaskSpec) ValidateBetaFields(ctx context.Context) *apis.FieldError { var errs *apis.FieldError // Object parameters for i, p := range ts.Params { diff --git a/pkg/reconciler/pipelinerun/resources/pipelineref.go b/pkg/reconciler/pipelinerun/resources/pipelineref.go index dd0aa4362fd..53957956dd1 100644 --- a/pkg/reconciler/pipelinerun/resources/pipelineref.go +++ b/pkg/reconciler/pipelinerun/resources/pipelineref.go @@ -154,13 +154,18 @@ func readRuntimeObjectAsPipeline(ctx context.Context, obj runtime.Object, k8s ku vr := trustedresources.VerifyPipeline(ctx, obj, k8s, refSource, verificationPolicies) if vr.VerificationResultType == trustedresources.VerificationError { if vr.Err != nil { - return nil, fmt.Errorf("remote Pipeline verification failed for object %s: %w", obj.GetName(), vr.Err) + return nil, fmt.Errorf("Pipeline verification failed for object %s: %w", obj.GetName(), vr.Err) } - return nil, fmt.Errorf("remote Pipeline verification failed for object %s", obj.GetName()) + return nil, fmt.Errorf("Pipeline verification failed for object %s", obj.GetName()) } return obj, nil case *v1.Pipeline: // TODO(#6356): Support V1 Task verification + // Validation of beta fields must happen before the V1 Pipeline is converted into the storage version of the API. + // TODO(#6592): Decouple API versioning from feature versioning + if err := obj.Spec.ValidateBetaFields(ctx); err != nil { + return nil, fmt.Errorf("invalid Pipeline %s: %w", obj.GetName(), err) + } t := &v1beta1.Pipeline{ TypeMeta: metav1.TypeMeta{ Kind: "Pipeline", diff --git a/pkg/reconciler/pipelinerun/resources/pipelineref_test.go b/pkg/reconciler/pipelinerun/resources/pipelineref_test.go index ac0b3d8f767..363b816f37e 100644 --- a/pkg/reconciler/pipelinerun/resources/pipelineref_test.go +++ b/pkg/reconciler/pipelinerun/resources/pipelineref_test.go @@ -291,12 +291,13 @@ func TestGetPipelineFunc_RemoteResolution(t *testing.T) { ctx := context.Background() cfg := config.FromContextOrDefaults(ctx) ctx = config.ToContext(ctx, cfg) - pipeline := parse.MustParseV1beta1Pipeline(t, pipelineYAMLString) pipelineRef := &v1beta1.PipelineRef{ResolverRef: v1beta1.ResolverRef{Resolver: "git"}} testcases := []struct { name string pipelineYAML string + wantPipeline *v1beta1.Pipeline + wantErr bool }{{ name: "v1beta1 pipeline", pipelineYAML: strings.Join([]string{ @@ -304,6 +305,15 @@ func TestGetPipelineFunc_RemoteResolution(t *testing.T) { "apiVersion: tekton.dev/v1beta1", pipelineYAMLString, }, "\n"), + wantPipeline: parse.MustParseV1beta1Pipeline(t, pipelineYAMLString), + }, { + name: "v1beta1 pipeline with beta features", + pipelineYAML: strings.Join([]string{ + "kind: Pipeline", + "apiVersion: tekton.dev/v1beta1", + pipelineYAMLStringWithBetaFeatures, + }, "\n"), + wantPipeline: parse.MustParseV1beta1Pipeline(t, pipelineYAMLStringWithBetaFeatures), }, { name: "v1 pipeline", pipelineYAML: strings.Join([]string{ @@ -311,6 +321,16 @@ func TestGetPipelineFunc_RemoteResolution(t *testing.T) { "apiVersion: tekton.dev/v1", pipelineYAMLString, }, "\n"), + wantPipeline: parse.MustParseV1beta1Pipeline(t, pipelineYAMLString), + }, { + name: "v1 pipeline with beta features", + pipelineYAML: strings.Join([]string{ + "kind: Pipeline", + "apiVersion: tekton.dev/v1", + pipelineYAMLStringWithBetaFeatures, + }, "\n"), + wantPipeline: nil, + wantErr: true, }} for _, tc := range testcases { t.Run(tc.name, func(t *testing.T) { @@ -325,16 +345,22 @@ func TestGetPipelineFunc_RemoteResolution(t *testing.T) { }, nil /*VerificationPolicies*/) resolvedPipeline, resolvedRefSource, err := fn(ctx, pipelineRef.Name) - if err != nil { - t.Fatalf("failed to call pipelinefn: %s", err.Error()) - } - - if diff := cmp.Diff(pipeline, resolvedPipeline); diff != "" { - t.Error(diff) - } - - if d := cmp.Diff(sampleRefSource, resolvedRefSource); d != "" { - t.Errorf("refSources did not match: %s", diff.PrintWantGot(d)) + if tc.wantErr { + if err == nil { + t.Errorf("expected an error when calling pipelinefunc but got none") + } + } else { + if err != nil { + t.Fatalf("failed to call pipelinefn: %s", err.Error()) + } + + if diff := cmp.Diff(tc.wantPipeline, resolvedPipeline); diff != "" { + t.Error(diff) + } + + if d := cmp.Diff(sampleRefSource, resolvedRefSource); d != "" { + t.Errorf("refSources did not match: %s", diff.PrintWantGot(d)) + } } }) } @@ -900,3 +926,16 @@ spec: script: | echo "hello world!" ` + +var pipelineYAMLStringWithBetaFeatures = ` +metadata: + name: foo +spec: + tasks: + - name: task1 + taskRef: + resolver: git + params: + - name: name + value: test-task +` diff --git a/pkg/reconciler/taskrun/resources/taskref.go b/pkg/reconciler/taskrun/resources/taskref.go index c56cc4d4e06..d425d67875d 100644 --- a/pkg/reconciler/taskrun/resources/taskref.go +++ b/pkg/reconciler/taskrun/resources/taskref.go @@ -175,15 +175,20 @@ func readRuntimeObjectAsTask(ctx context.Context, obj runtime.Object, k8s kubern vr := trustedresources.VerifyTask(ctx, obj, k8s, refSource, verificationPolicies) if vr.VerificationResultType == trustedresources.VerificationError { if vr.Err != nil { - return nil, fmt.Errorf("remote Task verification failed for object %s: %w", obj.GetName(), vr.Err) + return nil, fmt.Errorf("Task verification failed for object %s: %w", obj.GetName(), vr.Err) } - return nil, fmt.Errorf("remote Task verification failed for object %s", obj.GetName()) + return nil, fmt.Errorf("Task verification failed for object %s", obj.GetName()) } return obj, nil case *v1beta1.ClusterTask: return convertClusterTaskToTask(*obj), nil case *v1.Task: // TODO(#6356): Support V1 Task verification + // Validation of beta fields must happen before the V1 Task is converted into the storage version of the API. + // TODO(#6592): Decouple API versioning from feature versioning + if err := obj.Spec.ValidateBetaFields(ctx); err != nil { + return nil, fmt.Errorf("invalid Task %s: %w", obj.GetName(), err) + } t := &v1beta1.Task{ TypeMeta: metav1.TypeMeta{ Kind: "Task", diff --git a/pkg/reconciler/taskrun/resources/taskref_test.go b/pkg/reconciler/taskrun/resources/taskref_test.go index 5dbcccdb2d3..cc61ac2fc2f 100644 --- a/pkg/reconciler/taskrun/resources/taskref_test.go +++ b/pkg/reconciler/taskrun/resources/taskref_test.go @@ -551,12 +551,13 @@ func TestGetTaskFunc_RemoteResolution(t *testing.T) { ctx := context.Background() cfg := config.FromContextOrDefaults(ctx) ctx = config.ToContext(ctx, cfg) - task := parse.MustParseV1beta1Task(t, taskYAMLString) taskRef := &v1beta1.TaskRef{ResolverRef: v1beta1.ResolverRef{Resolver: "git"}} testcases := []struct { name string taskYAML string + wantTask *v1beta1.Task + wantErr bool }{{ name: "v1beta1 task", taskYAML: strings.Join([]string{ @@ -564,6 +565,15 @@ func TestGetTaskFunc_RemoteResolution(t *testing.T) { "apiVersion: tekton.dev/v1beta1", taskYAMLString, }, "\n"), + wantTask: parse.MustParseV1beta1Task(t, taskYAMLString), + }, { + name: "v1beta1 task with beta features", + taskYAML: strings.Join([]string{ + "kind: Task", + "apiVersion: tekton.dev/v1beta1", + taskYAMLStringWithBetaFeatures, + }, "\n"), + wantTask: parse.MustParseV1beta1Task(t, taskYAMLStringWithBetaFeatures), }, { name: "v1 task", taskYAML: strings.Join([]string{ @@ -571,6 +581,15 @@ func TestGetTaskFunc_RemoteResolution(t *testing.T) { "apiVersion: tekton.dev/v1", taskYAMLString, }, "\n"), + wantTask: parse.MustParseV1beta1Task(t, taskYAMLString), + }, { + name: "v1 task with beta features", + taskYAML: strings.Join([]string{ + "kind: Task", + "apiVersion: tekton.dev/v1", + taskYAMLStringWithBetaFeatures, + }, "\n"), + wantErr: true, }} for _, tc := range testcases { t.Run(tc.name, func(t *testing.T) { @@ -586,16 +605,22 @@ func TestGetTaskFunc_RemoteResolution(t *testing.T) { fn := resources.GetTaskFunc(ctx, nil, nil, requester, tr, tr.Spec.TaskRef, "", "default", "default", nil /*VerificationPolicies*/) resolvedTask, resolvedRefSource, err := fn(ctx, taskRef.Name) - if err != nil { - t.Fatalf("failed to call pipelinefn: %s", err.Error()) - } - - if d := cmp.Diff(sampleRefSource, resolvedRefSource); d != "" { - t.Errorf("refSources did not match: %s", diff.PrintWantGot(d)) - } - - if d := cmp.Diff(task, resolvedTask); d != "" { - t.Errorf("resolvedTask did not match: %s", diff.PrintWantGot(d)) + if tc.wantErr { + if err == nil { + t.Fatalf("expected an error when calling taskfunc but got none") + } + } else { + if err != nil { + t.Fatalf("failed to call taskfn: %s", err.Error()) + } + + if d := cmp.Diff(sampleRefSource, resolvedRefSource); d != "" { + t.Errorf("refSources did not match: %s", diff.PrintWantGot(d)) + } + + if d := cmp.Diff(tc.wantTask, resolvedTask); d != "" { + t.Errorf("resolvedTask did not match: %s", diff.PrintWantGot(d)) + } } }) } @@ -1078,3 +1103,17 @@ spec: script: | echo "hello world!" ` + +var taskYAMLStringWithBetaFeatures = ` +metadata: + name: foo +spec: + steps: + - name: step1 + image: ubuntu + script: | + echo "hello world!" + results: + - name: array-result + type: array +`