From 163ec59f2ccb05dc0aa804de40ac5eb85cd3c7ce Mon Sep 17 00:00:00 2001 From: Lee Bernick Date: Thu, 25 May 2023 18:18:14 -0400 Subject: [PATCH] Add validation for beta features in v1 remote Tasks/Pipelines Currently, when a user creates a v1 Task or Pipeline using beta features on their cluster, we validate that "enable-api-fields" is set to "alpha" or "beta", and if not, reject the Task or Pipeline. However, no such validation is done for v1 remote Tasks or Pipelines. This is because we validate remote Task and Pipeline specs in the reconciler after converting them to v1beta1. This commit adds the same validation to remote v1 Tasks and Pipelines that we are already performing for local v1 Tasks and Pipelines. --- pkg/apis/pipeline/v1/pipeline_validation.go | 8 +-- pkg/apis/pipeline/v1/task_validation.go | 6 +- .../pipelinerun/resources/pipelineref.go | 9 ++- .../pipelinerun/resources/pipelineref_test.go | 61 +++++++++++++++---- pkg/reconciler/taskrun/resources/taskref.go | 9 ++- .../taskrun/resources/taskref_test.go | 61 +++++++++++++++---- 6 files changed, 121 insertions(+), 33 deletions(-) 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 +`