From 2e24d7598828b0e3fd004d18ab6dda9d833ca53d Mon Sep 17 00:00:00 2001 From: Yongxuan Zhang Date: Thu, 2 Mar 2023 22:14:29 +0000 Subject: [PATCH] add feature gate validation for param array indexing Before this commit, if alpha or beta feature gate is not enabled, the array indexing params will not be added to string replacements, thus will lead to non-existent variable error. This is confusing to users and doesn't provide correct guidance. This commit adds the check to the array indexing validation. Signed-off-by: Yongxuan Zhang yongxuanzhang@google.com --- pkg/apis/config/feature_flags.go | 9 +- pkg/apis/pipeline/v1/pipeline_validation.go | 9 +- .../pipeline/v1/pipeline_validation_test.go | 149 +++++++++++++----- pkg/apis/pipeline/v1/task_validation.go | 10 +- pkg/apis/pipeline/v1/task_validation_test.go | 44 +++++- .../pipeline/v1beta1/pipeline_validation.go | 9 +- .../v1beta1/pipeline_validation_test.go | 149 +++++++++++++----- pkg/apis/pipeline/v1beta1/task_validation.go | 10 +- .../pipeline/v1beta1/task_validation_test.go | 44 +++++- 9 files changed, 333 insertions(+), 100 deletions(-) diff --git a/pkg/apis/config/feature_flags.go b/pkg/apis/config/feature_flags.go index 305200b7fe9..bcbfcdc4c1d 100644 --- a/pkg/apis/config/feature_flags.go +++ b/pkg/apis/config/feature_flags.go @@ -315,17 +315,17 @@ func NewFeatureFlagsFromConfigMap(config *corev1.ConfigMap) (*FeatureFlags, erro // EnableAlphaAPIFields enables alpha features in an existing context (for use in testing) func EnableAlphaAPIFields(ctx context.Context) context.Context { - return setEnableAPIFields(ctx, AlphaAPIFields) + return SetEnableAPIFields(ctx, AlphaAPIFields) } // EnableBetaAPIFields enables beta features in an existing context (for use in testing) func EnableBetaAPIFields(ctx context.Context) context.Context { - return setEnableAPIFields(ctx, BetaAPIFields) + return SetEnableAPIFields(ctx, BetaAPIFields) } // EnableStableAPIFields enables stable features in an existing context (for use in testing) func EnableStableAPIFields(ctx context.Context) context.Context { - return setEnableAPIFields(ctx, StableAPIFields) + return SetEnableAPIFields(ctx, StableAPIFields) } // CheckEnforceResourceVerificationMode returns true if the ResourceVerificationMode is EnforceResourceVerificationMode @@ -348,7 +348,8 @@ func CheckAlphaOrBetaAPIFields(ctx context.Context) bool { return cfg.FeatureFlags.EnableAPIFields == AlphaAPIFields || cfg.FeatureFlags.EnableAPIFields == BetaAPIFields } -func setEnableAPIFields(ctx context.Context, want string) context.Context { +// SetEnableAPIFields sets the config "enable-api-fields" to the "want" value +func SetEnableAPIFields(ctx context.Context, want string) context.Context { featureFlags, _ := NewFeatureFlagsFromMap(map[string]string{ "enable-api-fields": want, }) diff --git a/pkg/apis/pipeline/v1/pipeline_validation.go b/pkg/apis/pipeline/v1/pipeline_validation.go index 1c8ab05e6df..50569b24514 100644 --- a/pkg/apis/pipeline/v1/pipeline_validation.go +++ b/pkg/apis/pipeline/v1/pipeline_validation.go @@ -429,10 +429,6 @@ func validateResultsFromMatrixedPipelineTasksNotConsumed(tasks []PipelineTask, f // 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. 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() { @@ -470,5 +466,10 @@ func (ps *PipelineSpec) ValidateParamArrayIndex(ctx context.Context, params Para arrayIndexParamRefs = append(arrayIndexParamRefs, extractArrayIndexingParamRefs(p)...) } + // if there are array indexing param references, the api feature gate needs to be set to `alpha` or `beta` + if len(arrayIndexParamRefs) > 0 && !config.CheckAlphaOrBetaAPIFields(ctx) { + return fmt.Errorf(`invalid parameter expression %s: indexing into array params requires "enable-api-fields" feature gate to be "alpha" or "beta"`, arrayIndexParamRefs) + } + return validateOutofBoundArrayParams(arrayIndexParamRefs, arrayParamsLengths) } diff --git a/pkg/apis/pipeline/v1/pipeline_validation_test.go b/pkg/apis/pipeline/v1/pipeline_validation_test.go index a18e4f6025c..c3cdde68144 100644 --- a/pkg/apis/pipeline/v1/pipeline_validation_test.go +++ b/pkg/apis/pipeline/v1/pipeline_validation_test.go @@ -3339,14 +3339,11 @@ func getTaskSpec() TaskSpec { } func TestValidateParamArrayIndex_valid(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 - params []Param + name string + original PipelineSpec + params []Param + apiFields string }{{ name: "single parameter", original: PipelineSpec{ @@ -3362,7 +3359,8 @@ func TestValidateParamArrayIndex_valid(t *testing.T) { }, }}, }, - params: []Param{{Name: "second-param", Value: *NewStructuredValues("second-value", "second-value-again")}}, + params: []Param{{Name: "second-param", Value: *NewStructuredValues("second-value", "second-value-again")}}, + apiFields: config.BetaAPIFields, }, { name: "single parameter with when expression", original: PipelineSpec{ @@ -3378,7 +3376,8 @@ func TestValidateParamArrayIndex_valid(t *testing.T) { }}, }}, }, - params: []Param{{Name: "second-param", Value: *NewStructuredValues("second-value", "second-value-again")}}, + params: []Param{{Name: "second-param", Value: *NewStructuredValues("second-value", "second-value-again")}}, + apiFields: config.BetaAPIFields, }, { name: "pipeline parameter nested inside task parameter", original: PipelineSpec{ @@ -3393,7 +3392,8 @@ func TestValidateParamArrayIndex_valid(t *testing.T) { }, }}, }, - params: nil, // no parameter values. + params: nil, // no parameter values. + apiFields: config.BetaAPIFields, }, { name: "array parameter", original: PipelineSpec{ @@ -3411,6 +3411,7 @@ func TestValidateParamArrayIndex_valid(t *testing.T) { params: []Param{ {Name: "second-param", Value: *NewStructuredValues("second-value", "array")}, }, + apiFields: config.BetaAPIFields, }, { name: "parameter evaluation with final tasks", original: PipelineSpec{ @@ -3430,7 +3431,8 @@ func TestValidateParamArrayIndex_valid(t *testing.T) { }}, }}, }, - params: []Param{{Name: "second-param", Value: *NewStructuredValues("second-value", "second-value-again")}}, + params: []Param{{Name: "second-param", Value: *NewStructuredValues("second-value", "second-value-again")}}, + apiFields: config.BetaAPIFields, }, { name: "parameter evaluation with both tasks and final tasks", original: PipelineSpec{ @@ -3456,7 +3458,8 @@ func TestValidateParamArrayIndex_valid(t *testing.T) { }}, }}, }, - params: []Param{{Name: "second-param", Value: *NewStructuredValues("second-value", "second-value-again")}}, + params: []Param{{Name: "second-param", Value: *NewStructuredValues("second-value", "second-value-again")}}, + apiFields: config.BetaAPIFields, }, { name: "parameter references with bracket notation and special characters", original: PipelineSpec{ @@ -3480,6 +3483,7 @@ func TestValidateParamArrayIndex_valid(t *testing.T) { {Name: "second/param", Value: *NewStructuredValues("second-value", "second-value-again")}, {Name: "fourth/param", Value: *NewStructuredValues("fourth-value", "fourth-value-again")}, }, + apiFields: config.BetaAPIFields, }, { name: "single parameter in workspace subpath", original: PipelineSpec{ @@ -3501,10 +3505,29 @@ func TestValidateParamArrayIndex_valid(t *testing.T) { }, }}, }, - params: []Param{{Name: "second-param", Value: *NewStructuredValues("second-value", "second-value-again")}}, + params: []Param{{Name: "second-param", Value: *NewStructuredValues("second-value", "second-value-again")}}, + apiFields: config.BetaAPIFields, + }, { + name: "validation with alpha gate", + original: PipelineSpec{ + Params: []ParamSpec{ + {Name: "first-param", Type: ParamTypeArray, Default: NewStructuredValues("default-value", "default-value-again")}, + {Name: "second-param", Type: ParamTypeString}, + }, + Tasks: []PipelineTask{{ + Params: []Param{ + {Name: "first-task-first-param", Value: *NewStructuredValues("$(params.first-param[1])")}, + {Name: "first-task-second-param", Value: *NewStructuredValues("$(params.second-param[0])")}, + {Name: "first-task-third-param", Value: *NewStructuredValues("static value")}, + }, + }}, + }, + params: []Param{{Name: "second-param", Value: *NewStructuredValues("second-value", "second-value-again")}}, + apiFields: config.AlphaAPIFields, }, } { tt := tt // capture range variable + ctx := config.SetEnableAPIFields(context.Background(), tt.apiFields) t.Run(tt.name, func(t *testing.T) { t.Parallel() err := tt.original.ValidateParamArrayIndex(ctx, tt.params) @@ -3516,15 +3539,12 @@ func TestValidateParamArrayIndex_valid(t *testing.T) { } 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 - params []Param - expected error + name string + original PipelineSpec + params []Param + apiFields string + expected error }{{ name: "single parameter reference out of bound", original: PipelineSpec{ @@ -3540,8 +3560,9 @@ func TestValidateParamArrayIndex_invalid(t *testing.T) { }, }}, }, - params: []Param{{Name: "second-param", Value: *NewStructuredValues("second-value", "second-value-again")}}, - expected: fmt.Errorf("non-existent param references:[$(params.first-param[2]) $(params.second-param[2])]"), + params: []Param{{Name: "second-param", Value: *NewStructuredValues("second-value", "second-value-again")}}, + apiFields: config.BetaAPIFields, + expected: fmt.Errorf("non-existent param references:[$(params.first-param[2]) $(params.second-param[2])]"), }, { name: "single parameter reference with when expression out of bound", original: PipelineSpec{ @@ -3557,8 +3578,9 @@ func TestValidateParamArrayIndex_invalid(t *testing.T) { }}, }}, }, - params: []Param{{Name: "second-param", Value: *NewStructuredValues("second-value", "second-value-again")}}, - expected: fmt.Errorf("non-existent param references:[$(params.first-param[2]) $(params.second-param[2])]"), + params: []Param{{Name: "second-param", Value: *NewStructuredValues("second-value", "second-value-again")}}, + apiFields: config.BetaAPIFields, + expected: fmt.Errorf("non-existent param references:[$(params.first-param[2]) $(params.second-param[2])]"), }, { name: "pipeline parameter reference nested inside task parameter out of bound", original: PipelineSpec{ @@ -3573,8 +3595,9 @@ func TestValidateParamArrayIndex_invalid(t *testing.T) { }, }}, }, - params: nil, // no parameter values. - expected: fmt.Errorf("non-existent param references:[$(params.first-param[2]) $(params.second-param[2])]"), + params: nil, // no parameter values. + apiFields: config.BetaAPIFields, + expected: fmt.Errorf("non-existent param references:[$(params.first-param[2]) $(params.second-param[2])]"), }, { name: "array parameter reference out of bound", original: PipelineSpec{ @@ -3592,7 +3615,8 @@ func TestValidateParamArrayIndex_invalid(t *testing.T) { params: []Param{ {Name: "second-param", Value: *NewStructuredValues("second-value", "array")}, }, - expected: fmt.Errorf("non-existent param references:[$(params.first-param[3]) $(params.second-param[4])]"), + apiFields: config.BetaAPIFields, + expected: fmt.Errorf("non-existent param references:[$(params.first-param[3]) $(params.second-param[4])]"), }, { name: "object parameter reference out of bound", original: PipelineSpec{ @@ -3612,7 +3636,8 @@ func TestValidateParamArrayIndex_invalid(t *testing.T) { params: []Param{ {Name: "second-param", Value: *NewStructuredValues("second-value", "array")}, }, - expected: fmt.Errorf("non-existent param references:[$(params.first-param[4]) $(params.second-param[4])]"), + apiFields: config.BetaAPIFields, + expected: fmt.Errorf("non-existent param references:[$(params.first-param[4]) $(params.second-param[4])]"), }, { name: "parameter evaluation with final tasks reference out of bound", original: PipelineSpec{ @@ -3632,8 +3657,9 @@ func TestValidateParamArrayIndex_invalid(t *testing.T) { }}, }}, }, - params: []Param{{Name: "second-param", Value: *NewStructuredValues("second-value", "second-value-again")}}, - expected: fmt.Errorf("non-existent param references:[$(params.first-param[2]) $(params.second-param[2])]"), + params: []Param{{Name: "second-param", Value: *NewStructuredValues("second-value", "second-value-again")}}, + apiFields: config.BetaAPIFields, + expected: fmt.Errorf("non-existent param references:[$(params.first-param[2]) $(params.second-param[2])]"), }, { name: "parameter evaluation with both tasks and final tasks reference out of bound", original: PipelineSpec{ @@ -3664,8 +3690,9 @@ func TestValidateParamArrayIndex_invalid(t *testing.T) { }}, }}, }, - params: []Param{{Name: "second-param", Value: *NewStructuredValues("second-value", "second-value-again")}}, - expected: fmt.Errorf("non-existent param references:[$(params.first-param[2]) $(params.first-param[3]) $(params.first-param[4]) $(params.second-param[2]) $(params.second-param[3]) $(params.second-param[4])]"), + params: []Param{{Name: "second-param", Value: *NewStructuredValues("second-value", "second-value-again")}}, + apiFields: config.BetaAPIFields, + expected: fmt.Errorf("non-existent param references:[$(params.first-param[2]) $(params.first-param[3]) $(params.first-param[4]) $(params.second-param[2]) $(params.second-param[3]) $(params.second-param[4])]"), }, { name: "parameter in matrix reference out of bound", original: PipelineSpec{ @@ -3682,8 +3709,9 @@ func TestValidateParamArrayIndex_invalid(t *testing.T) { }, }}, }, - params: []Param{{Name: "second-param", Value: *NewStructuredValues("second-value", "second-value-again")}}, - expected: fmt.Errorf("non-existent param references:[$(params.first-param[2])]"), + params: []Param{{Name: "second-param", Value: *NewStructuredValues("second-value", "second-value-again")}}, + apiFields: config.BetaAPIFields, + expected: fmt.Errorf("non-existent param references:[$(params.first-param[2])]"), }, { name: "parameter references with bracket notation and special characters reference out of bound", original: PipelineSpec{ @@ -3707,7 +3735,8 @@ func TestValidateParamArrayIndex_invalid(t *testing.T) { {Name: "second/param", Value: *NewStructuredValues("second-value", "second-value-again")}, {Name: "fourth/param", Value: *NewStructuredValues("fourth-value", "fourth-value-again")}, }, - expected: fmt.Errorf("non-existent param references:[$(params[\"first.param\"][2]) $(params[\"second/param\"][2]) $(params['fourth/param'][2]) $(params['third.param'][2])]"), + apiFields: config.BetaAPIFields, + expected: fmt.Errorf("non-existent param references:[$(params[\"first.param\"][2]) $(params[\"second/param\"][2]) $(params['fourth/param'][2]) $(params['third.param'][2])]"), }, { name: "single parameter in workspace subpath reference out of bound", original: PipelineSpec{ @@ -3729,13 +3758,57 @@ func TestValidateParamArrayIndex_invalid(t *testing.T) { }, }}, }, - params: []Param{{Name: "second-param", Value: *NewStructuredValues("second-value", "second-value-again")}}, - expected: fmt.Errorf("non-existent param references:[$(params.first-param[2]) $(params.second-param[3])]"), + params: []Param{{Name: "second-param", Value: *NewStructuredValues("second-value", "second-value-again")}}, + apiFields: config.BetaAPIFields, + expected: fmt.Errorf("non-existent param references:[$(params.first-param[2]) $(params.second-param[3])]"), + }, { + name: "validation alpha feature gate", + original: PipelineSpec{ + Params: []ParamSpec{ + {Name: "first-param", Type: ParamTypeArray, Default: NewStructuredValues("default-value", "default-value-again")}, + {Name: "second-param", Type: ParamTypeArray}, + }, + Tasks: []PipelineTask{{ + Params: []Param{ + {Name: "first-task-first-param", Value: *NewStructuredValues("$(params.first-param[2])")}, + {Name: "first-task-second-param", Value: *NewStructuredValues("static value")}, + }, + Workspaces: []WorkspacePipelineTaskBinding{ + { + Name: "first-workspace", + Workspace: "first-workspace", + SubPath: "$(params.second-param[3])", + }, + }, + }}, + }, + params: []Param{{Name: "second-param", Value: *NewStructuredValues("second-value", "second-value-again")}}, + apiFields: config.AlphaAPIFields, + expected: fmt.Errorf("non-existent param references:[$(params.first-param[2]) $(params.second-param[3])]"), + }, { + name: "stable gate not allowed", + original: PipelineSpec{ + Params: []ParamSpec{ + {Name: "first-param", Type: ParamTypeArray, Default: NewStructuredValues("default-value", "default-value-again")}, + {Name: "second-param", Type: ParamTypeString}, + }, + Tasks: []PipelineTask{{ + Params: []Param{ + {Name: "first-task-first-param", Value: *NewStructuredValues("$(params.first-param[2])")}, + {Name: "first-task-second-param", Value: *NewStructuredValues("$(params.second-param[2])")}, + {Name: "first-task-third-param", Value: *NewStructuredValues("static value")}, + }, + }}, + }, + params: []Param{{Name: "second-param", Value: *NewStructuredValues("second-value", "second-value-again")}}, + apiFields: config.StableAPIFields, + expected: fmt.Errorf(`invalid parameter expression %s: indexing into array params requires "enable-api-fields" feature gate to be "alpha" or "beta"`, []string{"$(params.first-param[2])", "$(params.second-param[2])"}), }, } { tt := tt // capture range variable t.Run(tt.name, func(t *testing.T) { t.Parallel() + ctx := config.SetEnableAPIFields(context.Background(), tt.apiFields) err := tt.original.ValidateParamArrayIndex(ctx, 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 fdf30e52dcb..94b291d78d6 100644 --- a/pkg/apis/pipeline/v1/task_validation.go +++ b/pkg/apis/pipeline/v1/task_validation.go @@ -607,11 +607,6 @@ func isParamRefs(s string) bool { // - `trParams` are params from taskrun. // - `taskSpec` contains params declarations. func (ts *TaskSpec) ValidateParamArrayIndex(ctx context.Context, params Params) error { - cfg := config.FromContextOrDefaults(ctx) - if cfg.FeatureFlags.EnableAPIFields != config.AlphaAPIFields { - return nil - } - // Collect all array params lengths arrayParamsLengths := ts.Params.extractParamArrayLengths() for k, v := range params.extractParamArrayLengths() { @@ -634,5 +629,10 @@ func (ts *TaskSpec) ValidateParamArrayIndex(ctx context.Context, params Params) arrayIndexParamRefs = append(arrayIndexParamRefs, extractArrayIndexingParamRefs(p)...) } + // if there are array indexing param references, the api feature gate needs to be set to `alpha` or `beta` + if len(arrayIndexParamRefs) > 0 && !config.CheckAlphaOrBetaAPIFields(ctx) { + return fmt.Errorf(`invalid parameter expression %s: indexing into array params requires "enable-api-fields" feature gate to be "alpha" or "beta"`, arrayIndexParamRefs) + } + return validateOutofBoundArrayParams(arrayIndexParamRefs, arrayParamsLengths) } diff --git a/pkg/apis/pipeline/v1/task_validation_test.go b/pkg/apis/pipeline/v1/task_validation_test.go index 5fa4d763f19..33dc4c6b09e 100644 --- a/pkg/apis/pipeline/v1/task_validation_test.go +++ b/pkg/apis/pipeline/v1/task_validation_test.go @@ -1745,6 +1745,7 @@ func TestValidateParamArrayIndex(t *testing.T) { name string params []v1.Param taskspec *v1.TaskSpec + apiFields string expectedError error }{{ name: "steps reference invalid", @@ -1801,6 +1802,7 @@ func TestValidateParamArrayIndex(t *testing.T) { }}, }}, }, + apiFields: config.BetaAPIFields, expectedError: fmt.Errorf("non-existent param references:[%v]", strings.Join(stepsInvalidReferences, " ")), }, { name: "stepTemplate reference invalid", @@ -1817,6 +1819,7 @@ func TestValidateParamArrayIndex(t *testing.T) { Image: "$(params.array-params[3])", }, }, + apiFields: config.BetaAPIFields, expectedError: fmt.Errorf("non-existent param references:[%v]", "$(params.array-params[3])"), }, { name: "volumes reference invalid", @@ -1879,6 +1882,7 @@ func TestValidateParamArrayIndex(t *testing.T) { }, }, }, + apiFields: config.BetaAPIFields, expectedError: fmt.Errorf("non-existent param references:[%v]", strings.Join(volumesInvalidReferences, " ")), }, { name: "workspaces reference invalid", @@ -1895,6 +1899,7 @@ func TestValidateParamArrayIndex(t *testing.T) { MountPath: "$(params.array-params[3])", }}, }, + apiFields: config.BetaAPIFields, expectedError: fmt.Errorf("non-existent param references:[%v]", "$(params.array-params[3])"), }, { name: "sidecar reference invalid", @@ -1912,12 +1917,49 @@ func TestValidateParamArrayIndex(t *testing.T) { }, }, }, + apiFields: config.BetaAPIFields, + expectedError: fmt.Errorf("non-existent param references:[%v]", "$(params.array-params[3])"), + }, { + name: "stable gate not allowed", + params: []v1.Param{{ + Name: "array-params", + Value: *v1.NewStructuredValues("bar", "foo"), + }}, + taskspec: &v1.TaskSpec{ + Params: []v1.ParamSpec{{ + Name: "array-params", + Default: v1.NewStructuredValues("bar", "foo"), + }}, + Sidecars: []v1.Sidecar{{ + Script: "$(params.array-params[3])", + }, + }, + }, + apiFields: config.StableAPIFields, + expectedError: fmt.Errorf(`invalid parameter expression %s: indexing into array params requires "enable-api-fields" feature gate to be "alpha" or "beta"`, []string{"$(params.array-params[3])"}), + }, { + name: "alpha gate allowed", + params: []v1.Param{{ + Name: "array-params", + Value: *v1.NewStructuredValues("bar", "foo"), + }}, + taskspec: &v1.TaskSpec{ + Params: []v1.ParamSpec{{ + Name: "array-params", + Default: v1.NewStructuredValues("bar", "foo"), + }}, + Sidecars: []v1.Sidecar{{ + Script: "$(params.array-params[3])", + }, + }, + }, + apiFields: config.AlphaAPIFields, expectedError: fmt.Errorf("non-existent param references:[%v]", "$(params.array-params[3])"), }, } for _, tc := range tcs { t.Run(tc.name, func(t *testing.T) { - ctx := config.ToContext(context.Background(), &config.Config{FeatureFlags: &config.FeatureFlags{EnableAPIFields: "alpha"}}) + ctx := config.SetEnableAPIFields(context.Background(), tc.apiFields) err := tc.taskspec.ValidateParamArrayIndex(ctx, tc.params) if d := cmp.Diff(tc.expectedError.Error(), err.Error()); d != "" { t.Errorf("validateParamArrayIndex() errors diff %s", diff.PrintWantGot(d)) diff --git a/pkg/apis/pipeline/v1beta1/pipeline_validation.go b/pkg/apis/pipeline/v1beta1/pipeline_validation.go index c2456a51480..1143c7f550f 100644 --- a/pkg/apis/pipeline/v1beta1/pipeline_validation.go +++ b/pkg/apis/pipeline/v1beta1/pipeline_validation.go @@ -430,10 +430,6 @@ func validateResultsFromMatrixedPipelineTasksNotConsumed(tasks []PipelineTask, f // 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. 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() { @@ -471,5 +467,10 @@ func (ps *PipelineSpec) ValidateParamArrayIndex(ctx context.Context, params Para arrayIndexParamRefs = append(arrayIndexParamRefs, extractArrayIndexingParamRefs(p)...) } + // if there are array indexing param references, the api feature gate needs to be set to `alpha` or `beta` + if len(arrayIndexParamRefs) > 0 && !config.CheckAlphaOrBetaAPIFields(ctx) { + return fmt.Errorf(`invalid parameter expression %s: indexing into array params requires "enable-api-fields" feature gate to be "alpha" or "beta"`, arrayIndexParamRefs) + } + return validateOutofBoundArrayParams(arrayIndexParamRefs, arrayParamsLengths) } diff --git a/pkg/apis/pipeline/v1beta1/pipeline_validation_test.go b/pkg/apis/pipeline/v1beta1/pipeline_validation_test.go index bb4e2e5ab5c..c2d39958265 100644 --- a/pkg/apis/pipeline/v1beta1/pipeline_validation_test.go +++ b/pkg/apis/pipeline/v1beta1/pipeline_validation_test.go @@ -3358,14 +3358,11 @@ func enableFeatures(t *testing.T, features []string) func(context.Context) conte } func TestValidateParamArrayIndex_valid(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 - params []Param + name string + original PipelineSpec + params []Param + apiFields string }{{ name: "single parameter", original: PipelineSpec{ @@ -3381,7 +3378,8 @@ func TestValidateParamArrayIndex_valid(t *testing.T) { }, }}, }, - params: []Param{{Name: "second-param", Value: *NewStructuredValues("second-value", "second-value-again")}}, + params: []Param{{Name: "second-param", Value: *NewStructuredValues("second-value", "second-value-again")}}, + apiFields: config.BetaAPIFields, }, { name: "single parameter with when expression", original: PipelineSpec{ @@ -3397,7 +3395,8 @@ func TestValidateParamArrayIndex_valid(t *testing.T) { }}, }}, }, - params: []Param{{Name: "second-param", Value: *NewStructuredValues("second-value", "second-value-again")}}, + params: []Param{{Name: "second-param", Value: *NewStructuredValues("second-value", "second-value-again")}}, + apiFields: config.BetaAPIFields, }, { name: "pipeline parameter nested inside task parameter", original: PipelineSpec{ @@ -3412,7 +3411,8 @@ func TestValidateParamArrayIndex_valid(t *testing.T) { }, }}, }, - params: nil, // no parameter values. + params: nil, // no parameter values. + apiFields: config.BetaAPIFields, }, { name: "array parameter", original: PipelineSpec{ @@ -3430,6 +3430,7 @@ func TestValidateParamArrayIndex_valid(t *testing.T) { params: []Param{ {Name: "second-param", Value: *NewStructuredValues("second-value", "array")}, }, + apiFields: config.BetaAPIFields, }, { name: "parameter evaluation with final tasks", original: PipelineSpec{ @@ -3449,7 +3450,8 @@ func TestValidateParamArrayIndex_valid(t *testing.T) { }}, }}, }, - params: []Param{{Name: "second-param", Value: *NewStructuredValues("second-value", "second-value-again")}}, + params: []Param{{Name: "second-param", Value: *NewStructuredValues("second-value", "second-value-again")}}, + apiFields: config.BetaAPIFields, }, { name: "parameter evaluation with both tasks and final tasks", original: PipelineSpec{ @@ -3475,7 +3477,8 @@ func TestValidateParamArrayIndex_valid(t *testing.T) { }}, }}, }, - params: []Param{{Name: "second-param", Value: *NewStructuredValues("second-value", "second-value-again")}}, + params: []Param{{Name: "second-param", Value: *NewStructuredValues("second-value", "second-value-again")}}, + apiFields: config.BetaAPIFields, }, { name: "parameter references with bracket notation and special characters", original: PipelineSpec{ @@ -3499,6 +3502,7 @@ func TestValidateParamArrayIndex_valid(t *testing.T) { {Name: "second/param", Value: *NewStructuredValues("second-value", "second-value-again")}, {Name: "fourth/param", Value: *NewStructuredValues("fourth-value", "fourth-value-again")}, }, + apiFields: config.BetaAPIFields, }, { name: "single parameter in workspace subpath", original: PipelineSpec{ @@ -3520,10 +3524,29 @@ func TestValidateParamArrayIndex_valid(t *testing.T) { }, }}, }, - params: []Param{{Name: "second-param", Value: *NewStructuredValues("second-value", "second-value-again")}}, + params: []Param{{Name: "second-param", Value: *NewStructuredValues("second-value", "second-value-again")}}, + apiFields: config.BetaAPIFields, + }, { + name: "validation with alpha gate", + original: PipelineSpec{ + Params: []ParamSpec{ + {Name: "first-param", Type: ParamTypeArray, Default: NewStructuredValues("default-value", "default-value-again")}, + {Name: "second-param", Type: ParamTypeString}, + }, + Tasks: []PipelineTask{{ + Params: []Param{ + {Name: "first-task-first-param", Value: *NewStructuredValues("$(params.first-param[1])")}, + {Name: "first-task-second-param", Value: *NewStructuredValues("$(params.second-param[0])")}, + {Name: "first-task-third-param", Value: *NewStructuredValues("static value")}, + }, + }}, + }, + params: []Param{{Name: "second-param", Value: *NewStructuredValues("second-value", "second-value-again")}}, + apiFields: config.AlphaAPIFields, }, } { tt := tt // capture range variable + ctx := config.SetEnableAPIFields(context.Background(), tt.apiFields) t.Run(tt.name, func(t *testing.T) { t.Parallel() err := tt.original.ValidateParamArrayIndex(ctx, tt.params) @@ -3535,15 +3558,12 @@ func TestValidateParamArrayIndex_valid(t *testing.T) { } 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 - params []Param - expected error + name string + original PipelineSpec + params []Param + apiFields string + expected error }{{ name: "single parameter reference out of bound", original: PipelineSpec{ @@ -3559,8 +3579,9 @@ func TestValidateParamArrayIndex_invalid(t *testing.T) { }, }}, }, - params: []Param{{Name: "second-param", Value: *NewStructuredValues("second-value", "second-value-again")}}, - expected: fmt.Errorf("non-existent param references:[$(params.first-param[2]) $(params.second-param[2])]"), + params: []Param{{Name: "second-param", Value: *NewStructuredValues("second-value", "second-value-again")}}, + apiFields: config.BetaAPIFields, + expected: fmt.Errorf("non-existent param references:[$(params.first-param[2]) $(params.second-param[2])]"), }, { name: "single parameter reference with when expression out of bound", original: PipelineSpec{ @@ -3576,8 +3597,9 @@ func TestValidateParamArrayIndex_invalid(t *testing.T) { }}, }}, }, - params: []Param{{Name: "second-param", Value: *NewStructuredValues("second-value", "second-value-again")}}, - expected: fmt.Errorf("non-existent param references:[$(params.first-param[2]) $(params.second-param[2])]"), + params: []Param{{Name: "second-param", Value: *NewStructuredValues("second-value", "second-value-again")}}, + apiFields: config.BetaAPIFields, + expected: fmt.Errorf("non-existent param references:[$(params.first-param[2]) $(params.second-param[2])]"), }, { name: "pipeline parameter reference nested inside task parameter out of bound", original: PipelineSpec{ @@ -3592,8 +3614,9 @@ func TestValidateParamArrayIndex_invalid(t *testing.T) { }, }}, }, - params: nil, // no parameter values. - expected: fmt.Errorf("non-existent param references:[$(params.first-param[2]) $(params.second-param[2])]"), + params: nil, // no parameter values. + apiFields: config.BetaAPIFields, + expected: fmt.Errorf("non-existent param references:[$(params.first-param[2]) $(params.second-param[2])]"), }, { name: "array parameter reference out of bound", original: PipelineSpec{ @@ -3611,7 +3634,8 @@ func TestValidateParamArrayIndex_invalid(t *testing.T) { params: []Param{ {Name: "second-param", Value: *NewStructuredValues("second-value", "array")}, }, - expected: fmt.Errorf("non-existent param references:[$(params.first-param[3]) $(params.second-param[4])]"), + apiFields: config.BetaAPIFields, + expected: fmt.Errorf("non-existent param references:[$(params.first-param[3]) $(params.second-param[4])]"), }, { name: "object parameter reference out of bound", original: PipelineSpec{ @@ -3631,7 +3655,8 @@ func TestValidateParamArrayIndex_invalid(t *testing.T) { params: []Param{ {Name: "second-param", Value: *NewStructuredValues("second-value", "array")}, }, - expected: fmt.Errorf("non-existent param references:[$(params.first-param[4]) $(params.second-param[4])]"), + apiFields: config.BetaAPIFields, + expected: fmt.Errorf("non-existent param references:[$(params.first-param[4]) $(params.second-param[4])]"), }, { name: "parameter evaluation with final tasks reference out of bound", original: PipelineSpec{ @@ -3651,8 +3676,9 @@ func TestValidateParamArrayIndex_invalid(t *testing.T) { }}, }}, }, - params: []Param{{Name: "second-param", Value: *NewStructuredValues("second-value", "second-value-again")}}, - expected: fmt.Errorf("non-existent param references:[$(params.first-param[2]) $(params.second-param[2])]"), + params: []Param{{Name: "second-param", Value: *NewStructuredValues("second-value", "second-value-again")}}, + apiFields: config.BetaAPIFields, + expected: fmt.Errorf("non-existent param references:[$(params.first-param[2]) $(params.second-param[2])]"), }, { name: "parameter evaluation with both tasks and final tasks reference out of bound", original: PipelineSpec{ @@ -3683,8 +3709,9 @@ func TestValidateParamArrayIndex_invalid(t *testing.T) { }}, }}, }, - params: []Param{{Name: "second-param", Value: *NewStructuredValues("second-value", "second-value-again")}}, - expected: fmt.Errorf("non-existent param references:[$(params.first-param[2]) $(params.first-param[3]) $(params.first-param[4]) $(params.second-param[2]) $(params.second-param[3]) $(params.second-param[4])]"), + params: []Param{{Name: "second-param", Value: *NewStructuredValues("second-value", "second-value-again")}}, + apiFields: config.BetaAPIFields, + expected: fmt.Errorf("non-existent param references:[$(params.first-param[2]) $(params.first-param[3]) $(params.first-param[4]) $(params.second-param[2]) $(params.second-param[3]) $(params.second-param[4])]"), }, { name: "parameter in matrix reference out of bound", original: PipelineSpec{ @@ -3701,8 +3728,9 @@ func TestValidateParamArrayIndex_invalid(t *testing.T) { }, }}, }, - params: []Param{{Name: "second-param", Value: *NewStructuredValues("second-value", "second-value-again")}}, - expected: fmt.Errorf("non-existent param references:[$(params.first-param[2])]"), + params: []Param{{Name: "second-param", Value: *NewStructuredValues("second-value", "second-value-again")}}, + apiFields: config.BetaAPIFields, + expected: fmt.Errorf("non-existent param references:[$(params.first-param[2])]"), }, { name: "parameter references with bracket notation and special characters reference out of bound", original: PipelineSpec{ @@ -3726,7 +3754,8 @@ func TestValidateParamArrayIndex_invalid(t *testing.T) { {Name: "second/param", Value: *NewStructuredValues("second-value", "second-value-again")}, {Name: "fourth/param", Value: *NewStructuredValues("fourth-value", "fourth-value-again")}, }, - expected: fmt.Errorf("non-existent param references:[$(params[\"first.param\"][2]) $(params[\"second/param\"][2]) $(params['fourth/param'][2]) $(params['third.param'][2])]"), + apiFields: config.BetaAPIFields, + expected: fmt.Errorf("non-existent param references:[$(params[\"first.param\"][2]) $(params[\"second/param\"][2]) $(params['fourth/param'][2]) $(params['third.param'][2])]"), }, { name: "single parameter in workspace subpath reference out of bound", original: PipelineSpec{ @@ -3748,13 +3777,57 @@ func TestValidateParamArrayIndex_invalid(t *testing.T) { }, }}, }, - params: []Param{{Name: "second-param", Value: *NewStructuredValues("second-value", "second-value-again")}}, - expected: fmt.Errorf("non-existent param references:[$(params.first-param[2]) $(params.second-param[3])]"), + params: []Param{{Name: "second-param", Value: *NewStructuredValues("second-value", "second-value-again")}}, + apiFields: config.BetaAPIFields, + expected: fmt.Errorf("non-existent param references:[$(params.first-param[2]) $(params.second-param[3])]"), + }, { + name: "validation alpha feature gate", + original: PipelineSpec{ + Params: []ParamSpec{ + {Name: "first-param", Type: ParamTypeArray, Default: NewStructuredValues("default-value", "default-value-again")}, + {Name: "second-param", Type: ParamTypeArray}, + }, + Tasks: []PipelineTask{{ + Params: []Param{ + {Name: "first-task-first-param", Value: *NewStructuredValues("$(params.first-param[2])")}, + {Name: "first-task-second-param", Value: *NewStructuredValues("static value")}, + }, + Workspaces: []WorkspacePipelineTaskBinding{ + { + Name: "first-workspace", + Workspace: "first-workspace", + SubPath: "$(params.second-param[3])", + }, + }, + }}, + }, + params: []Param{{Name: "second-param", Value: *NewStructuredValues("second-value", "second-value-again")}}, + apiFields: config.AlphaAPIFields, + expected: fmt.Errorf("non-existent param references:[$(params.first-param[2]) $(params.second-param[3])]"), + }, { + name: "stable gate not allowed", + original: PipelineSpec{ + Params: []ParamSpec{ + {Name: "first-param", Type: ParamTypeArray, Default: NewStructuredValues("default-value", "default-value-again")}, + {Name: "second-param", Type: ParamTypeString}, + }, + Tasks: []PipelineTask{{ + Params: []Param{ + {Name: "first-task-first-param", Value: *NewStructuredValues("$(params.first-param[2])")}, + {Name: "first-task-second-param", Value: *NewStructuredValues("$(params.second-param[2])")}, + {Name: "first-task-third-param", Value: *NewStructuredValues("static value")}, + }, + }}, + }, + params: []Param{{Name: "second-param", Value: *NewStructuredValues("second-value", "second-value-again")}}, + apiFields: config.StableAPIFields, + expected: fmt.Errorf(`invalid parameter expression %s: indexing into array params requires "enable-api-fields" feature gate to be "alpha" or "beta"`, []string{"$(params.first-param[2])", "$(params.second-param[2])"}), }, } { tt := tt // capture range variable t.Run(tt.name, func(t *testing.T) { t.Parallel() + ctx := config.SetEnableAPIFields(context.Background(), tt.apiFields) err := tt.original.ValidateParamArrayIndex(ctx, 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/v1beta1/task_validation.go b/pkg/apis/pipeline/v1beta1/task_validation.go index 1486c38aaa6..ee28b926ff0 100644 --- a/pkg/apis/pipeline/v1beta1/task_validation.go +++ b/pkg/apis/pipeline/v1beta1/task_validation.go @@ -607,11 +607,6 @@ func isParamRefs(s string) bool { // - `trParams` are params from taskrun. // - `taskSpec` contains params declarations. func (ts *TaskSpec) ValidateParamArrayIndex(ctx context.Context, params Params) error { - cfg := config.FromContextOrDefaults(ctx) - if cfg.FeatureFlags.EnableAPIFields != config.AlphaAPIFields { - return nil - } - // Collect all array params lengths arrayParamsLengths := ts.Params.extractParamArrayLengths() for k, v := range params.extractParamArrayLengths() { @@ -634,5 +629,10 @@ func (ts *TaskSpec) ValidateParamArrayIndex(ctx context.Context, params Params) arrayIndexParamRefs = append(arrayIndexParamRefs, extractArrayIndexingParamRefs(p)...) } + // if there are array indexing param references, the api feature gate needs to be set to `alpha` or `beta` + if len(arrayIndexParamRefs) > 0 && !config.CheckAlphaOrBetaAPIFields(ctx) { + return fmt.Errorf(`invalid parameter expression %s: indexing into array params requires "enable-api-fields" feature gate to be "alpha" or "beta"`, arrayIndexParamRefs) + } + return validateOutofBoundArrayParams(arrayIndexParamRefs, arrayParamsLengths) } diff --git a/pkg/apis/pipeline/v1beta1/task_validation_test.go b/pkg/apis/pipeline/v1beta1/task_validation_test.go index 982655e2540..8824c56542e 100644 --- a/pkg/apis/pipeline/v1beta1/task_validation_test.go +++ b/pkg/apis/pipeline/v1beta1/task_validation_test.go @@ -1748,6 +1748,7 @@ func TestValidateParamArrayIndex(t *testing.T) { name string params []v1beta1.Param taskspec *v1beta1.TaskSpec + apiFields string expectedError error }{{ name: "steps reference invalid", @@ -1804,6 +1805,7 @@ func TestValidateParamArrayIndex(t *testing.T) { }}, }}, }, + apiFields: config.BetaAPIFields, expectedError: fmt.Errorf("non-existent param references:[%v]", strings.Join(stepsInvalidReferences, " ")), }, { name: "stepTemplate reference invalid", @@ -1820,6 +1822,7 @@ func TestValidateParamArrayIndex(t *testing.T) { Image: "$(params.array-params[3])", }, }, + apiFields: config.BetaAPIFields, expectedError: fmt.Errorf("non-existent param references:[%v]", "$(params.array-params[3])"), }, { name: "volumes reference invalid", @@ -1882,6 +1885,7 @@ func TestValidateParamArrayIndex(t *testing.T) { }, }, }, + apiFields: config.BetaAPIFields, expectedError: fmt.Errorf("non-existent param references:[%v]", strings.Join(volumesInvalidReferences, " ")), }, { name: "workspaces reference invalid", @@ -1898,6 +1902,7 @@ func TestValidateParamArrayIndex(t *testing.T) { MountPath: "$(params.array-params[3])", }}, }, + apiFields: config.BetaAPIFields, expectedError: fmt.Errorf("non-existent param references:[%v]", "$(params.array-params[3])"), }, { name: "sidecar reference invalid", @@ -1915,12 +1920,49 @@ func TestValidateParamArrayIndex(t *testing.T) { }, }, }, + apiFields: config.BetaAPIFields, + expectedError: fmt.Errorf("non-existent param references:[%v]", "$(params.array-params[3])"), + }, { + name: "stable gate not allowed", + params: []v1beta1.Param{{ + Name: "array-params", + Value: *v1beta1.NewStructuredValues("bar", "foo"), + }}, + taskspec: &v1beta1.TaskSpec{ + Params: []v1beta1.ParamSpec{{ + Name: "array-params", + Default: v1beta1.NewStructuredValues("bar", "foo"), + }}, + Sidecars: []v1beta1.Sidecar{{ + Script: "$(params.array-params[3])", + }, + }, + }, + apiFields: config.StableAPIFields, + expectedError: fmt.Errorf(`invalid parameter expression %s: indexing into array params requires "enable-api-fields" feature gate to be "alpha" or "beta"`, []string{"$(params.array-params[3])"}), + }, { + name: "alpha gate allowed", + params: []v1beta1.Param{{ + Name: "array-params", + Value: *v1beta1.NewStructuredValues("bar", "foo"), + }}, + taskspec: &v1beta1.TaskSpec{ + Params: []v1beta1.ParamSpec{{ + Name: "array-params", + Default: v1beta1.NewStructuredValues("bar", "foo"), + }}, + Sidecars: []v1beta1.Sidecar{{ + Script: "$(params.array-params[3])", + }, + }, + }, + apiFields: config.AlphaAPIFields, expectedError: fmt.Errorf("non-existent param references:[%v]", "$(params.array-params[3])"), }, } for _, tc := range tcs { t.Run(tc.name, func(t *testing.T) { - ctx := config.ToContext(context.Background(), &config.Config{FeatureFlags: &config.FeatureFlags{EnableAPIFields: "alpha"}}) + ctx := config.SetEnableAPIFields(context.Background(), tc.apiFields) err := tc.taskspec.ValidateParamArrayIndex(ctx, tc.params) if d := cmp.Diff(tc.expectedError.Error(), err.Error()); d != "" { t.Errorf("validateParamArrayIndex() errors diff %s", diff.PrintWantGot(d))