diff --git a/pkg/apis/pipeline/v1beta1/pipeline_types_test.go b/pkg/apis/pipeline/v1beta1/pipeline_types_test.go index 6e4f7372048..4d6a8831735 100644 --- a/pkg/apis/pipeline/v1beta1/pipeline_types_test.go +++ b/pkg/apis/pipeline/v1beta1/pipeline_types_test.go @@ -221,7 +221,7 @@ func TestPipelineTask_ValidateRegularTask_Success(t *testing.T) { tests := []struct { name string tasks PipelineTask - enableAPIFields bool + enableAPIFields string enableBundles bool }{{ name: "pipeline task - valid taskRef name", @@ -240,11 +240,13 @@ func TestPipelineTask_ValidateRegularTask_Success(t *testing.T) { tasks: PipelineTask{ TaskRef: &TaskRef{ResolverRef: ResolverRef{Resolver: "bar"}}, }, + enableAPIFields: "beta", }, { name: "pipeline task - use of params", tasks: PipelineTask{ TaskRef: &TaskRef{ResolverRef: ResolverRef{Resolver: "bar", Params: Params{}}}, }, + enableAPIFields: "beta", }, { name: "pipeline task - use of bundle with the feature flag set", tasks: PipelineTask{ @@ -259,9 +261,7 @@ func TestPipelineTask_ValidateRegularTask_Success(t *testing.T) { cfg := &config.Config{ FeatureFlags: &config.FeatureFlags{}, } - if tt.enableAPIFields { - cfg.FeatureFlags.EnableAPIFields = config.AlphaAPIFields - } + cfg.FeatureFlags.EnableAPIFields = tt.enableAPIFields if tt.enableBundles { cfg.FeatureFlags.EnableTektonOCIBundles = true } diff --git a/pkg/apis/pipeline/v1beta1/pipeline_validation.go b/pkg/apis/pipeline/v1beta1/pipeline_validation.go index 6411d3f09d4..ea01650d206 100644 --- a/pkg/apis/pipeline/v1beta1/pipeline_validation.go +++ b/pkg/apis/pipeline/v1beta1/pipeline_validation.go @@ -52,6 +52,7 @@ func (p *Pipeline) Validate(ctx context.Context) *apis.FieldError { // we do not support propagated parameters and workspaces. // Validate that all params and workspaces it uses are declared. errs = errs.Also(p.Spec.validatePipelineParameterUsage(ctx).ViaField("spec")) + errs = errs.Also(p.Spec.ValidateBetaFields(ctx)) return errs.Also(p.Spec.validatePipelineWorkspacesUsage().ViaField("spec")) } @@ -87,6 +88,60 @@ 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 +// have "enable-api-fields" set to "alpha" or "beta". +func (ps *PipelineSpec) ValidateBetaFields(ctx context.Context) *apis.FieldError { + var errs *apis.FieldError + // Object parameters + for i, p := range ps.Params { + if p.Type == ParamTypeObject { + errs = errs.Also(version.ValidateEnabledAPIFields(ctx, "object type parameter", config.BetaAPIFields).ViaFieldIndex("params", i)) + } + } + // Indexing into array parameters + arrayParamIndexingRefs := ps.GetIndexingReferencesToArrayParams() + if len(arrayParamIndexingRefs) != 0 && !config.CheckAlphaOrBetaAPIFields(ctx) { + errs = errs.Also(apis.ErrGeneric(fmt.Sprintf("cannot index into array parameters when 'enable-api-fields' is 'stable', but found indexing references: %s", arrayParamIndexingRefs))) + } + // array and object results + for i, result := range ps.Results { + switch result.Type { + case ResultsTypeObject: + errs = errs.Also(version.ValidateEnabledAPIFields(ctx, "object results", config.BetaAPIFields).ViaFieldIndex("results", i)) + case ResultsTypeArray: + errs = errs.Also(version.ValidateEnabledAPIFields(ctx, "array results", config.BetaAPIFields).ViaFieldIndex("results", i)) + case ResultsTypeString: + default: + } + } + for i, pt := range ps.Tasks { + errs = errs.Also(pt.validateBetaFields(ctx).ViaFieldIndex("tasks", i)) + } + for i, pt := range ps.Finally { + errs = errs.Also(pt.validateBetaFields(ctx).ViaFieldIndex("tasks", i)) + } + + return errs +} + +// validateBetaFields returns an error if the PipelineTask uses beta features but does not +// have "enable-api-fields" set to "alpha" or "beta". +func (pt *PipelineTask) validateBetaFields(ctx context.Context) *apis.FieldError { + var errs *apis.FieldError + if pt.TaskRef != nil { + // Resolvers + if pt.TaskRef.Resolver != "" { + errs = errs.Also(version.ValidateEnabledAPIFields(ctx, "taskref.resolver", config.BetaAPIFields)) + } + if len(pt.TaskRef.Params) > 0 { + errs = errs.Also(version.ValidateEnabledAPIFields(ctx, "taskref.params", config.BetaAPIFields)) + } + } else if pt.TaskSpec != nil { + errs = errs.Also(pt.TaskSpec.ValidateBetaFields(ctx)) + } + return errs +} + // ValidatePipelineTasks ensures that pipeline tasks has unique label, pipeline tasks has specified one of // taskRef or taskSpec, and in case of a pipeline task with taskRef, it has a reference to a valid task (task name) func ValidatePipelineTasks(ctx context.Context, tasks []PipelineTask, finalTasks []PipelineTask) *apis.FieldError { diff --git a/pkg/apis/pipeline/v1beta1/pipeline_validation_test.go b/pkg/apis/pipeline/v1beta1/pipeline_validation_test.go index a2d691937c8..d08f01748f1 100644 --- a/pkg/apis/pipeline/v1beta1/pipeline_validation_test.go +++ b/pkg/apis/pipeline/v1beta1/pipeline_validation_test.go @@ -3409,6 +3409,206 @@ func enableFeatures(t *testing.T, features []string) func(context.Context) conte } } +func TestPipelineWithBetaFields(t *testing.T) { + tts := []struct { + name string + spec PipelineSpec + }{{ + name: "array indexing in Tasks", + spec: PipelineSpec{ + Params: []ParamSpec{ + {Name: "first-param", Type: ParamTypeArray, Default: NewStructuredValues("default-value", "default-value-again")}, + }, + Tasks: []PipelineTask{{ + Name: "foo", + Params: Params{ + {Name: "first-task-first-param", Value: *NewStructuredValues("$(params.first-param[0])")}, + }, + TaskRef: &TaskRef{Name: "foo"}, + }}, + }, + }, { + name: "array indexing in Finally", + spec: PipelineSpec{ + Params: []ParamSpec{ + {Name: "first-param", Type: ParamTypeArray, Default: NewStructuredValues("default-value", "default-value-again")}, + }, + Tasks: []PipelineTask{{ + Name: "foo", + TaskRef: &TaskRef{Name: "foo"}, + }}, + Finally: []PipelineTask{{ + Name: "bar", + Params: Params{ + {Name: "first-task-first-param", Value: *NewStructuredValues("$(params.first-param[0])")}, + }, + TaskRef: &TaskRef{Name: "bar"}, + }}, + }, + }, { + name: "pipeline tasks - use of resolver without the feature flag set", + spec: PipelineSpec{ + Tasks: []PipelineTask{{ + Name: "uses-resolver", + TaskRef: &TaskRef{ResolverRef: ResolverRef{Resolver: "bar"}}, + }}, + }, + }, { + name: "pipeline tasks - use of resolver params without the feature flag set", + spec: PipelineSpec{ + Tasks: []PipelineTask{{ + Name: "uses-resolver-params", + TaskRef: &TaskRef{ResolverRef: ResolverRef{Resolver: "bar", Params: Params{{}}}}, + }}, + }, + }, { + name: "finally tasks - use of resolver without the feature flag set", + spec: PipelineSpec{ + Tasks: []PipelineTask{{ + Name: "valid-pipeline-task", + TaskRef: &TaskRef{Name: "foo-task"}, + }}, + Finally: []PipelineTask{{ + Name: "uses-resolver", + TaskRef: &TaskRef{ResolverRef: ResolverRef{Resolver: "bar"}}, + }}, + }, + }, { + name: "finally tasks - use of resolver params without the feature flag set", + spec: PipelineSpec{ + Tasks: []PipelineTask{{ + Name: "valid-pipeline-task", + TaskRef: &TaskRef{Name: "foo-task"}, + }}, + Finally: []PipelineTask{{ + Name: "uses-resolver-params", + TaskRef: &TaskRef{ResolverRef: ResolverRef{Resolver: "bar", Params: Params{{}}}}, + }}, + }, + }, { + name: "object params", + spec: PipelineSpec{ + Params: []ParamSpec{ + {Name: "first-param", Type: ParamTypeObject, Properties: map[string]PropertySpec{}}, + }, + Tasks: []PipelineTask{{ + Name: "foo", + TaskRef: &TaskRef{Name: "foo"}, + }}, + }, + }, { + name: "object params in Tasks", + spec: PipelineSpec{ + Tasks: []PipelineTask{{ + Name: "valid-pipeline-task", + TaskSpec: &EmbeddedTask{TaskSpec: TaskSpec{ + Steps: []Step{{Image: "busybox", Script: "echo hello"}}, + Params: []ParamSpec{{Name: "my-object-param", Type: ParamTypeObject, Properties: map[string]PropertySpec{}}}, + }}, + }}, + }, + }, { + name: "object params in Finally", + spec: PipelineSpec{ + Tasks: []PipelineTask{{ + Name: "foo", + TaskRef: &TaskRef{Name: "foo"}, + }}, + Finally: []PipelineTask{{ + Name: "valid-finally-task", + TaskSpec: &EmbeddedTask{TaskSpec: TaskSpec{ + Steps: []Step{{Image: "busybox", Script: "echo hello"}}, + Params: []ParamSpec{{Name: "my-object-param", Type: ParamTypeObject, Properties: map[string]PropertySpec{}}}, + }}, + }}, + }, + }, { + name: "array results", + spec: PipelineSpec{ + Tasks: []PipelineTask{{ + Name: "valid-pipeline-task", + TaskRef: &TaskRef{Name: "foo-task"}, + }}, + Results: []PipelineResult{{Name: "my-array-result", Type: ResultsTypeArray, Value: *NewStructuredValues("$(tasks.valid-pipeline-task.results.foo[*])")}}, + }, + }, { + name: "array results in Tasks", + spec: PipelineSpec{ + Tasks: []PipelineTask{{ + Name: "valid-pipeline-task", + TaskSpec: &EmbeddedTask{TaskSpec: TaskSpec{ + Steps: []Step{{Image: "busybox", Script: "echo hello"}}, + Results: []TaskResult{{Name: "my-array-result", Type: ResultsTypeArray}}, + }}, + }}, + }, + }, { + name: "array results in Finally", + spec: PipelineSpec{ + Tasks: []PipelineTask{{ + Name: "valid-pipeline-task", + TaskRef: &TaskRef{Name: "foo-task"}, + }}, + Finally: []PipelineTask{{ + Name: "valid-finally-task", + TaskSpec: &EmbeddedTask{TaskSpec: TaskSpec{ + Steps: []Step{{Image: "busybox", Script: "echo hello"}}, + Results: []TaskResult{{Name: "my-array-result", Type: ResultsTypeArray}}, + }}, + }}, + }, + }, { + name: "object results", + spec: PipelineSpec{ + Tasks: []PipelineTask{{ + Name: "valid-pipeline-task", + TaskRef: &TaskRef{Name: "foo-task"}, + }}, + Results: []PipelineResult{{Name: "my-object-result", Type: ResultsTypeObject, Value: *NewStructuredValues("$(tasks.valid-pipeline-task.results.foo[*])")}}, + }, + }, { + name: "object results in Tasks", + spec: PipelineSpec{ + Tasks: []PipelineTask{{ + Name: "valid-pipeline-task", + TaskSpec: &EmbeddedTask{TaskSpec: TaskSpec{ + Steps: []Step{{Image: "busybox", Script: "echo hello"}}, + Results: []TaskResult{{Name: "my-object-result", Type: ResultsTypeObject, Properties: map[string]PropertySpec{}}}, + }}, + }}, + }, + }, { + name: "object results in Finally", + spec: PipelineSpec{ + Tasks: []PipelineTask{{ + Name: "valid-pipeline-task", + TaskRef: &TaskRef{Name: "foo-task"}, + }}, + Finally: []PipelineTask{{ + Name: "valid-finally-task", + TaskSpec: &EmbeddedTask{TaskSpec: TaskSpec{ + Steps: []Step{{Image: "busybox", Script: "echo hello"}}, + Results: []TaskResult{{Name: "my-object-result", Type: ResultsTypeObject, Properties: map[string]PropertySpec{}}}, + }}, + }}, + }, + }} + for _, tt := range tts { + t.Run(tt.name, func(t *testing.T) { + pipeline := Pipeline{ObjectMeta: metav1.ObjectMeta{Name: "foo"}, Spec: tt.spec} + ctx := config.EnableStableAPIFields(context.Background()) + if err := pipeline.Validate(ctx); err == nil { + t.Errorf("no error when using beta field when `enable-api-fields` is stable") + } + + ctx = config.EnableBetaAPIFields(context.Background()) + if err := pipeline.Validate(ctx); err != nil { + t.Errorf("unexpected error when using beta field: %s", err) + } + }) + } +} + func TestGetIndexingReferencesToArrayParams(t *testing.T) { for _, tt := range []struct { name string diff --git a/pkg/apis/pipeline/v1beta1/task_validation.go b/pkg/apis/pipeline/v1beta1/task_validation.go index e9e6d2832df..c48bc13c68c 100644 --- a/pkg/apis/pipeline/v1beta1/task_validation.go +++ b/pkg/apis/pipeline/v1beta1/task_validation.go @@ -65,6 +65,7 @@ func (t *Task) Validate(ctx context.Context) *apis.FieldError { errs = errs.Also(t.Spec.Validate(apis.WithinSpec(ctx)).ViaField("spec")) // When a Task is created directly, instead of declared inline in a TaskRun or PipelineRun, // we do not support propagated parameters. Validate that all params it uses are declared. + errs = errs.Also(t.Spec.ValidateBetaFields(ctx)) return errs.Also(ValidateUsageOfDeclaredParameters(ctx, t.Spec.Steps, t.Spec.Params).ViaField("spec")) } @@ -99,6 +100,35 @@ func (ts *TaskSpec) Validate(ctx context.Context) (errs *apis.FieldError) { return errs } +// 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 { + var errs *apis.FieldError + // Object parameters + for i, p := range ts.Params { + if p.Type == ParamTypeObject { + errs = errs.Also(version.ValidateEnabledAPIFields(ctx, "object type parameter", config.BetaAPIFields).ViaFieldIndex("params", i)) + } + } + // Indexing into array parameters + arrayIndexParamRefs := ts.GetIndexingReferencesToArrayParams() + if len(arrayIndexParamRefs) != 0 && !config.CheckAlphaOrBetaAPIFields(ctx) { + errs = errs.Also(apis.ErrGeneric(fmt.Sprintf("cannot index into array parameters when 'enable-api-fields' is 'stable', but found indexing references: %s", arrayIndexParamRefs))) + } + // Array and object results + for i, result := range ts.Results { + switch result.Type { + case ResultsTypeObject: + errs = errs.Also(version.ValidateEnabledAPIFields(ctx, "object results", config.BetaAPIFields).ViaFieldIndex("results", i)) + case ResultsTypeArray: + errs = errs.Also(version.ValidateEnabledAPIFields(ctx, "array results", config.BetaAPIFields).ViaFieldIndex("results", i)) + case ResultsTypeString: + default: + } + } + return errs +} + // ValidateUsageOfDeclaredParameters validates that all parameters referenced in the Task are declared by the Task. func ValidateUsageOfDeclaredParameters(ctx context.Context, steps []Step, params ParamSpecs) *apis.FieldError { var errs *apis.FieldError diff --git a/pkg/apis/pipeline/v1beta1/task_validation_test.go b/pkg/apis/pipeline/v1beta1/task_validation_test.go index 3272a8739e4..1aab13785c0 100644 --- a/pkg/apis/pipeline/v1beta1/task_validation_test.go +++ b/pkg/apis/pipeline/v1beta1/task_validation_test.go @@ -1686,6 +1686,76 @@ func TestIncompatibleAPIVersions(t *testing.T) { } } +func TestTaskBetaFields(t *testing.T) { + tests := []struct { + name string + spec v1beta1.TaskSpec + }{{ + name: "array param indexing", + spec: v1beta1.TaskSpec{ + Params: []v1beta1.ParamSpec{{Name: "foo", Type: v1beta1.ParamTypeArray}}, + Steps: []v1beta1.Step{{ + Name: "my-step", + Image: "my-image", + Script: ` + #!/usr/bin/env bash + echo $(params.foo[1])`, + }}, + }, + }, { + name: "object params", + spec: v1beta1.TaskSpec{ + Params: []v1beta1.ParamSpec{{Name: "foo", Type: v1beta1.ParamTypeObject, Properties: map[string]v1beta1.PropertySpec{"bar": {Type: v1beta1.ParamTypeString}}}}, + Steps: []v1beta1.Step{{ + Name: "my-step", + Image: "my-image", + Script: ` + #!/usr/bin/env bash + echo $(params.foo.bar)`, + }}, + }, + }, { + name: "array results", + spec: v1beta1.TaskSpec{ + Results: []v1beta1.TaskResult{{Name: "array-result", Type: v1beta1.ResultsTypeArray}}, + Steps: []v1beta1.Step{{ + Name: "my-step", + Image: "my-image", + Script: ` + #!/usr/bin/env bash + echo -n "[\"hello\",\"world\"]" | tee $(results.array-result.path)`, + }}, + }, + }, { + name: "object results", + spec: v1beta1.TaskSpec{ + Results: []v1beta1.TaskResult{{Name: "object-result", Type: v1beta1.ResultsTypeObject, + Properties: map[string]v1beta1.PropertySpec{}}}, + Steps: []v1beta1.Step{{ + Name: "my-step", + Image: "my-image", + Script: ` + #!/usr/bin/env bash + echo -n "{\"hello\":\"world\"}" | tee $(results.object-result.path)`, + }}, + }, + }} + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctx := config.EnableStableAPIFields(context.Background()) + task := v1beta1.Task{ObjectMeta: metav1.ObjectMeta{Name: "foo"}, Spec: tt.spec} + if err := task.Validate(ctx); err == nil { + t.Errorf("no error when using beta field when `enable-api-fields` is stable") + } + + ctx = config.EnableBetaAPIFields(context.Background()) + if err := task.Validate(ctx); err != nil { + t.Errorf("unexpected error when using beta field: %s", err) + } + }) + } +} + func TestGetArrayIndexParamRefs(t *testing.T) { stepsReferences := []string{} for i := 10; i <= 26; i++ { diff --git a/pkg/apis/pipeline/v1beta1/taskref_validation.go b/pkg/apis/pipeline/v1beta1/taskref_validation.go index 4e42b7aa2e2..eddf7d8a780 100644 --- a/pkg/apis/pipeline/v1beta1/taskref_validation.go +++ b/pkg/apis/pipeline/v1beta1/taskref_validation.go @@ -21,6 +21,8 @@ import ( "strings" "github.com/google/go-containerregistry/pkg/name" + "github.com/tektoncd/pipeline/pkg/apis/config" + "github.com/tektoncd/pipeline/pkg/apis/version" "k8s.io/apimachinery/pkg/util/validation" "knative.dev/pkg/apis" ) @@ -35,6 +37,7 @@ func (ref *TaskRef) Validate(ctx context.Context) (errs *apis.FieldError) { switch { case ref.Resolver != "" || ref.Params != nil: if ref.Resolver != "" { + errs = errs.Also(version.ValidateEnabledAPIFields(ctx, "resolver", config.BetaAPIFields).ViaField("resolver")) if ref.Name != "" { errs = errs.Also(apis.ErrMultipleOneOf("name", "resolver")) } @@ -43,6 +46,7 @@ func (ref *TaskRef) Validate(ctx context.Context) (errs *apis.FieldError) { } } if ref.Params != nil { + errs = errs.Also(version.ValidateEnabledAPIFields(ctx, "resolver", config.BetaAPIFields).ViaField("resolver")) if ref.Name != "" { errs = errs.Also(apis.ErrMultipleOneOf("name", "params")) } diff --git a/pkg/reconciler/pipelinerun/resources/pipelineref_test.go b/pkg/reconciler/pipelinerun/resources/pipelineref_test.go index 681b0587e9b..7a35e385db0 100644 --- a/pkg/reconciler/pipelinerun/resources/pipelineref_test.go +++ b/pkg/reconciler/pipelinerun/resources/pipelineref_test.go @@ -285,9 +285,11 @@ func TestGetPipelineFunc_RemoteResolution(t *testing.T) { pipelineRef := &v1.PipelineRef{ResolverRef: v1.ResolverRef{Resolver: "git"}} testcases := []struct { - name string - pipelineYAML string - wantPipeline *v1.Pipeline + name string + pipelineYAML string + enableAPIFields string + wantPipeline *v1.Pipeline + wantErr bool }{{ name: "v1beta1 pipeline", pipelineYAML: strings.Join([]string{ diff --git a/pkg/reconciler/taskrun/resources/taskref.go b/pkg/reconciler/taskrun/resources/taskref.go index 3b72d3c3a67..958517f12cb 100644 --- a/pkg/reconciler/taskrun/resources/taskref.go +++ b/pkg/reconciler/taskrun/resources/taskref.go @@ -155,6 +155,9 @@ func readRuntimeObjectAsTask(ctx context.Context, obj runtime.Object, k8s kubern case *v1beta1.Task: // Verify the Task once we fetch from the remote resolution, mutating, validation and conversion of the task should happen after the verification, since signatures are based on the remote task contents vr := trustedresources.VerifyResource(ctx, obj, k8s, refSource, verificationPolicies) + if err := obj.Spec.ValidateBetaFields(ctx); err != nil { + return nil, nil, fmt.Errorf("invalid Task %s: %w", obj.GetName(), err) + } t := &v1.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 53fc64ec897..82d5c03db2c 100644 --- a/pkg/reconciler/taskrun/resources/taskref_test.go +++ b/pkg/reconciler/taskrun/resources/taskref_test.go @@ -455,7 +455,7 @@ func TestGetTaskFunc_RemoteResolution(t *testing.T) { "apiVersion: tekton.dev/v1beta1", taskYAMLStringWithBetaFeatures, }, "\n"), - wantTask: parse.MustParseV1Task(t, taskYAMLStringWithBetaFeatures), + wantErr: true, }, { name: "v1beta1 cluster task", taskYAML: strings.Join([]string{