Skip to content

Commit

Permalink
add feature gate validation for param array indexing
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@Yongxuanzhang
Yongxuanzhang committed Mar 10, 2023
1 parent 67abc4a commit 1ecb863
Show file tree
Hide file tree
Showing 9 changed files with 335 additions and 98 deletions.
9 changes: 5 additions & 4 deletions pkg/apis/config/feature_flags.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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,
})
Expand Down
9 changes: 5 additions & 4 deletions pkg/apis/pipeline/v1/pipeline_validation.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down Expand Up @@ -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)
}
145 changes: 108 additions & 37 deletions pkg/apis/pipeline/v1/pipeline_validation_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3271,14 +3271,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{
Expand All @@ -3294,7 +3291,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{
Expand All @@ -3310,7 +3308,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{
Expand All @@ -3325,7 +3324,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{
Expand All @@ -3343,6 +3343,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{
Expand All @@ -3362,7 +3363,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{
Expand All @@ -3388,7 +3390,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{
Expand All @@ -3412,6 +3415,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{
Expand All @@ -3433,10 +3437,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)
Expand All @@ -3448,15 +3471,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{
Expand All @@ -3472,8 +3492,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{
Expand All @@ -3489,8 +3510,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{
Expand All @@ -3505,8 +3527,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{
Expand All @@ -3524,7 +3547,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{
Expand All @@ -3544,7 +3568,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{
Expand All @@ -3564,8 +3589,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{
Expand All @@ -3591,8 +3617,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.second-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.first-param[3]) $(params.second-param[2]) $(params.second-param[3])]"),
}, {
name: "parameter references with bracket notation and special characters reference out of bound",
original: PipelineSpec{
Expand All @@ -3616,7 +3643,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{
Expand All @@ -3638,13 +3666,56 @@ 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))
Expand Down
10 changes: 5 additions & 5 deletions pkg/apis/pipeline/v1/task_validation.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand All @@ -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)
}
Loading

0 comments on commit 1ecb863

Please sign in to comment.