Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add feature gate validation for param array indexing #6280

Closed
wants to merge 1 commit into from
Closed

add feature gate validation for param array indexing #6280

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions pkg/apis/config/feature_flags.go
View file
Open in desktop
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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like this function and the above wrapper functions are only used for testing purpose. We may want to refactor this part of code to the testing package like: https://github.com/tektoncd/pipeline/blob/main/pkg/apis/config/testing/defaults.go

This gets more necessary when we expose this function, as the value set here is not validated at all. The risk is mitigated if this is in testing package, as production code should not reference it. WDYT?

(But this can be addressed in separate PR)

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
Open in desktop
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)
}
149 changes: 111 additions & 38 deletions pkg/apis/pipeline/v1/pipeline_validation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not related to this PR. I'm not sure why we call it 'apiFields' from beginning instead of apiField (not only here but also other places), "apiFields" looks like a slice to me 🤔

}{{
name: "single parameter",
original: PipelineSpec{
Expand All @@ -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{
Expand All @@ -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{
Expand All @@ -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{
Expand All @@ -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{
Expand All @@ -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{
Expand All @@ -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{
Expand All @@ -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{
Expand All @@ -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,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you also add a test for beta api fields?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

beta api fields is a default value in this test function, that's why I didn't add a specific case for that. Do you think we should add that one?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm I'm not really seeing where the default is set to beta, can you point me to it?
I think it would be better to make it explicit-- what if someone changes the default?

},
} {
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 @@ -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{
Expand All @@ -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{
Expand All @@ -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{
Expand All @@ -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{
Expand All @@ -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{
Expand All @@ -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{
Expand All @@ -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{
Expand Down Expand Up @@ -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{
Expand All @@ -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{
Expand All @@ -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{
Expand All @@ -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))
Expand Down
10 changes: 5 additions & 5 deletions pkg/apis/pipeline/v1/task_validation.go
View file
Open in desktop
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