From 3cefcac57dd06bbb7df88bfde89205c456df344e Mon Sep 17 00:00:00 2001 From: Lee Bernick Date: Fri, 12 May 2023 08:45:25 -0400 Subject: [PATCH] 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 | 20 ++++++++-- .../pipeline/v1/pipeline_validation_test.go | 39 ++++++++++++++++--- pkg/apis/pipeline/v1/task_validation.go | 20 ++++++++-- pkg/apis/pipeline/v1/task_validation_test.go | 34 +++++++++++++++- .../pipeline/v1beta1/pipeline_validation.go | 1 + pkg/apis/pipeline/v1beta1/task_validation.go | 1 + .../pipelinerun/resources/pipelineref.go | 8 ++++ pkg/reconciler/taskrun/resources/taskref.go | 8 ++++ 8 files changed, 116 insertions(+), 15 deletions(-) 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",