diff --git a/pkg/apis/pipeline/v1/pipeline_validation.go b/pkg/apis/pipeline/v1/pipeline_validation.go index d7304db85bd..190a7a36cb3 100644 --- a/pkg/apis/pipeline/v1/pipeline_validation.go +++ b/pkg/apis/pipeline/v1/pipeline_validation.go @@ -66,6 +66,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.ValidateBetaFeaturesEnabledForParamArrayIndexing(ctx)) // Validate the pipeline's workspaces. errs = errs.Also(validatePipelineWorkspacesDeclarations(ps.Workspaces)) errs = errs.Also(validatePipelineWorkspacesUsage(ctx, ps.Workspaces, ps.Tasks).ViaField("tasks")) @@ -689,11 +690,8 @@ func validateResultsFromMatrixedPipelineTasksNotConsumed(tasks []PipelineTask, f // ValidateParamArrayIndex validates if the param reference to an array param is out of bound. // error is returned when the array indexing reference is out of bound of the array param // e.g. if a param reference of $(params.array-param[2]) and the array param is of length 2. +// TODO(#6616): Move this functionality to the reconciler, as it is only used there func (ps *PipelineSpec) ValidateParamArrayIndex(ctx context.Context, params Params) error { - if !config.CheckAlphaOrBetaAPIFields(ctx) { - return nil - } - // Collect all array params lengths arrayParamsLengths := ps.Params.extractParamArrayLengths() for k, v := range params.extractParamArrayLengths() { @@ -704,6 +702,20 @@ func (ps *PipelineSpec) ValidateParamArrayIndex(ctx context.Context, params Para return validateOutofBoundArrayParams(arrayIndexParamRefs, arrayParamsLengths) } +// ValidateBetaFeaturesEnabledForParamArrayIndexing validates that "enable-api-fields" is set to "alpha" or "beta" if the pipeline spec +// contains indexing references to array params. +// This can be removed when array param indexing is moved to "stable". +func (ps *PipelineSpec) ValidateBetaFeaturesEnabledForParamArrayIndexing(ctx context.Context) (errs *apis.FieldError) { + if config.CheckAlphaOrBetaAPIFields(ctx) { + return nil + } + arrayParamIndexingRefs := ps.GetIndexingReferencesToArrayParams() + if len(arrayParamIndexingRefs) == 0 { + return nil + } + return apis.ErrGeneric(fmt.Sprintf("cannot index into array parameters when 'enable-api-fields' is 'stable', but found indexing references: %s", arrayParamIndexingRefs)) +} + // GetIndexingReferencesToArrayParams returns all strings referencing indices of PipelineRun array parameters // from parameters, workspaces, and when expressions defined in the Pipeline's Tasks and Finally Tasks. // For example, if a Task in the Pipeline has a parameter with a value "$(params.array-param-name[1])", diff --git a/pkg/apis/pipeline/v1/pipeline_validation_test.go b/pkg/apis/pipeline/v1/pipeline_validation_test.go index 23fde61aa23..c4e23b05a41 100644 --- a/pkg/apis/pipeline/v1/pipeline_validation_test.go +++ b/pkg/apis/pipeline/v1/pipeline_validation_test.go @@ -3463,11 +3463,40 @@ func TestValidateParamArrayIndex_valid(t *testing.T) { } } +func TestPipelineWithBetaFields(t *testing.T) { + tts := []struct { + name string + spec PipelineSpec + }{{ + name: "array indexing", + 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"}, + }}, + }, + }} + for _, tt := range tts { + t.Run(tt.name, func(t *testing.T) { + if err := tt.spec.Validate(context.Background()); err == nil { + t.Errorf("no error when using beta field when `enable-api-fields` is stable") + } + + ctx := config.EnableBetaAPIFields(context.Background()) + if err := tt.spec.Validate(ctx); err != nil { + t.Errorf("unexpected error when using beta field: %s", err) + } + }) + } +} + func TestValidateParamArrayIndex_invalid(t *testing.T) { - ctx := context.Background() - cfg := config.FromContextOrDefaults(ctx) - cfg.FeatureFlags.EnableAPIFields = config.BetaAPIFields - ctx = config.ToContext(ctx, cfg) for _, tt := range []struct { name string original PipelineSpec @@ -3684,7 +3713,7 @@ func TestValidateParamArrayIndex_invalid(t *testing.T) { tt := tt // capture range variable t.Run(tt.name, func(t *testing.T) { t.Parallel() - err := tt.original.ValidateParamArrayIndex(ctx, tt.params) + err := tt.original.ValidateParamArrayIndex(context.Background(), tt.params) if d := cmp.Diff(tt.expected.Error(), err.Error()); d != "" { t.Errorf("ValidateParamArrayIndex() errors diff %s", diff.PrintWantGot(d)) } diff --git a/pkg/apis/pipeline/v1/task_validation.go b/pkg/apis/pipeline/v1/task_validation.go index e75d4143773..25fe4430b26 100644 --- a/pkg/apis/pipeline/v1/task_validation.go +++ b/pkg/apis/pipeline/v1/task_validation.go @@ -96,6 +96,7 @@ 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.ValidateBetaFeaturesEnabledForParamArrayIndexing(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")) @@ -575,16 +576,27 @@ func isParamRefs(s string) bool { return strings.HasPrefix(s, "$("+ParamsPrefix) } +// ValidateBetaFeaturesEnabledForParamArrayIndexing validates that "enable-api-fields" is set to "alpha" or "beta" if the task spec +// contains indexing references to array params. +// This can be removed when array param indexing is moved to "stable". +func (ts *TaskSpec) ValidateBetaFeaturesEnabledForParamArrayIndexing(ctx context.Context) *apis.FieldError { + if config.CheckAlphaOrBetaAPIFields(ctx) { + return nil + } + arrayIndexParamRefs := ts.GetIndexingReferencesToArrayParams() + if len(arrayIndexParamRefs) == 0 { + return nil + } + return apis.ErrGeneric(fmt.Sprintf("cannot index into array parameters when 'enable-api-fields' is 'stable', but found indexing references: %s", arrayIndexParamRefs)) +} + // ValidateParamArrayIndex validates if the param reference to an array param is out of bound. // error is returned when the array indexing reference is out of bound of the array param // e.g. if a param reference of $(params.array-param[2]) and the array param is of length 2. // - `trParams` are params from taskrun. // - `taskSpec` contains params declarations. +// TODO(#6616): Move this functionality to the reconciler, as it is only used there func (ts *TaskSpec) ValidateParamArrayIndex(ctx context.Context, params Params) error { - if !config.CheckAlphaOrBetaAPIFields(ctx) { - return nil - } - // Collect all array params lengths arrayParamsLengths := ts.Params.extractParamArrayLengths() for k, v := range params.extractParamArrayLengths() { diff --git a/pkg/apis/pipeline/v1/task_validation_test.go b/pkg/apis/pipeline/v1/task_validation_test.go index 81a60d992c0..d52e1ec9051 100644 --- a/pkg/apis/pipeline/v1/task_validation_test.go +++ b/pkg/apis/pipeline/v1/task_validation_test.go @@ -1918,8 +1918,7 @@ func TestValidateParamArrayIndex(t *testing.T) { } for _, tc := range tcs { t.Run(tc.name, func(t *testing.T) { - ctx := config.ToContext(context.Background(), &config.Config{FeatureFlags: &config.FeatureFlags{EnableAPIFields: "beta"}}) - err := tc.taskspec.ValidateParamArrayIndex(ctx, tc.params) + err := tc.taskspec.ValidateParamArrayIndex(context.Background(), tc.params) if d := cmp.Diff(tc.expectedError.Error(), err.Error()); d != "" { t.Errorf("validateParamArrayIndex() errors diff %s", diff.PrintWantGot(d)) } @@ -1927,6 +1926,37 @@ func TestValidateParamArrayIndex(t *testing.T) { } } +func TestTaskSpecBetaFields(t *testing.T) { + tests := []struct { + name string + spec v1.TaskSpec + }{{ + name: "array param indexing", + spec: v1.TaskSpec{ + Params: []v1.ParamSpec{{Name: "foo", Type: v1.ParamTypeArray}}, + Steps: []v1.Step{{ + Name: "my-step", + Image: "my-image", + Script: ` + #!/usr/bin/env bash + echo $(params.foo[1])`, + }}, + }, + }} + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if err := tt.spec.Validate(context.Background()); err == nil { + t.Errorf("no error when using beta field when `enable-api-fields` is stable") + } + + ctx := config.EnableBetaAPIFields(context.Background()) + if err := tt.spec.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/pipeline_validation.go b/pkg/apis/pipeline/v1beta1/pipeline_validation.go index 9c7c2ae36d9..2112858b13c 100644 --- a/pkg/apis/pipeline/v1beta1/pipeline_validation.go +++ b/pkg/apis/pipeline/v1beta1/pipeline_validation.go @@ -713,6 +713,7 @@ func validateResultsFromMatrixedPipelineTasksNotConsumed(tasks []PipelineTask, f // ValidateParamArrayIndex validates if the param reference to an array param is out of bound. // error is returned when the array indexing reference is out of bound of the array param // e.g. if a param reference of $(params.array-param[2]) and the array param is of length 2. +// TODO(#6616): Move this functionality to the reconciler, as it is only used there func (ps *PipelineSpec) ValidateParamArrayIndex(ctx context.Context, params Params) error { // Collect all array params lengths arrayParamsLengths := ps.Params.extractParamArrayLengths() diff --git a/pkg/apis/pipeline/v1beta1/task_validation.go b/pkg/apis/pipeline/v1beta1/task_validation.go index 769809f6d68..41768dbf9f8 100644 --- a/pkg/apis/pipeline/v1beta1/task_validation.go +++ b/pkg/apis/pipeline/v1beta1/task_validation.go @@ -584,6 +584,7 @@ func isParamRefs(s string) bool { // e.g. if a param reference of $(params.array-param[2]) and the array param is of length 2. // - `trParams` are params from taskrun. // - `taskSpec` contains params declarations. +// TODO(#6616): Move this functionality to the reconciler, as it is only used there func (ts *TaskSpec) ValidateParamArrayIndex(ctx context.Context, params Params) error { // Collect all array params lengths arrayParamsLengths := ts.Params.extractParamArrayLengths() diff --git a/pkg/reconciler/pipelinerun/resources/pipelineref.go b/pkg/reconciler/pipelinerun/resources/pipelineref.go index b905fab9d05..68793084f59 100644 --- a/pkg/reconciler/pipelinerun/resources/pipelineref.go +++ b/pkg/reconciler/pipelinerun/resources/pipelineref.go @@ -158,6 +158,14 @@ func readRuntimeObjectAsPipeline(ctx context.Context, obj runtime.Object, k8s ku return obj, nil case *v1.Pipeline: // TODO(#6356): Support V1 Task verification + + // Ensure that "enable-api-fields" is set to "alpha" or "beta" if the referenced Pipeline is using array param indexing. + // This check ensures that remote Pipelines are validated the same way as local Pipelines. + // TODO(#6616): Move all Pipeline spec validation from reconciler to here + err := obj.Spec.ValidateBetaFeaturesEnabledForParamArrayIndexing(ctx) + if err != nil { + return nil, fmt.Errorf("remote Pipeline validation failed for object %s: %w", obj.GetName(), err) + } t := &v1beta1.Pipeline{ TypeMeta: metav1.TypeMeta{ Kind: "Pipeline", diff --git a/pkg/reconciler/taskrun/resources/taskref.go b/pkg/reconciler/taskrun/resources/taskref.go index 95a0730f1de..53cdfe1ba46 100644 --- a/pkg/reconciler/taskrun/resources/taskref.go +++ b/pkg/reconciler/taskrun/resources/taskref.go @@ -181,6 +181,14 @@ func readRuntimeObjectAsTask(ctx context.Context, obj runtime.Object, k8s kubern return convertClusterTaskToTask(*obj), nil case *v1.Task: // TODO(#6356): Support V1 Task verification + + // Ensure that "enable-api-fields" is set to "alpha" or "beta" if the referenced Task is using array param indexing. + // This check ensures that remote Tasks are validated the same way as local Tasks. + // TODO(#6616): Move all Task spec validation from reconciler to here + err := obj.Spec.ValidateBetaFeaturesEnabledForParamArrayIndexing(ctx) + if err != nil { + return nil, fmt.Errorf("remote Task validation failed for object %s: %w", obj.GetName(), err) + } t := &v1beta1.Task{ TypeMeta: metav1.TypeMeta{ Kind: "Task",