Split array param indexing validation between reconciler and webhook

There are two components to validation of array param indexing:

1. Validating that beta features are enabled.
2. Validating that array indexes are in bounds.

Prior to this commit, both of these forms of validation were done in the PipelineRun reconciler.
However, the first form of validation differs between the v1beta1 and the v1 API version.
In addition, array index bounds checking was skipped if beta features were disabled.
This meant that if "enable-api-fields" was set to "stable", a user could create a beta Task/Pipeline
using array parameter indexing, and we would not validate whether indexes were in bounds, potentially
leading to a confusing error message or reconciler panic.

This commit moves the first form of validation to take place only in the webhook,
leaving validation for bounds checking in the reconciler. Bounds checking is performed regardless
of what feature flags are enabled.

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])",

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))
 			}

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() {

pkg/apis/pipeline/v1/task_validation_test.go

@@ -1918,15 +1918,45 @@ 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))
 			}
 		})
 	}
 }
 
+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++ {

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()

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()