add feature gate validation for param array indexing #6280

Yongxuanzhang · 2023-03-02T22:17:10Z

Changes

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.

/kind bug

closes #6102

Signed-off-by: Yongxuan Zhang [email protected]

Submitter Checklist

As the author of this PR, please check off the items in this checklist:

Has Docs included if any changes are user facing
Has Tests included if any functionality added or changed
Follows the commit message standard
Meets the Tekton contributor standards (including
functionality, content, code)
Has a kind label. You can add one by adding a comment on this PR that contains /kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tep
Release notes block below has been updated with any user facing changes (API changes, bug fixes, changes requiring upgrade notices or deprecation warnings)
Release notes contains the string "action required" if the change requires additional action from users switching to the new release

Release Notes

Add feature gate check for param array indexing, the enable-api-fields needs to be set to alpha or beta

tekton-robot · 2023-03-02T22:17:17Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign imjasonh after the PR has been reviewed.
You can assign the PR to them by writing /assign @imjasonh in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

pkg/apis/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

tekton-robot · 2023-03-02T22:22:51Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1/pipeline_validation.go	98.1%	98.6%	0.5
pkg/apis/pipeline/v1/task_validation.go	96.9%	97.3%	0.4
pkg/apis/pipeline/v1beta1/pipeline_validation.go	98.5%	98.9%	0.4
pkg/apis/pipeline/v1beta1/task_validation.go	97.1%	97.4%	0.4

tekton-robot · 2023-03-02T22:25:17Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1/pipeline_validation.go	98.1%	98.6%	0.5
pkg/apis/pipeline/v1/task_validation.go	96.9%	97.3%	0.4
pkg/apis/pipeline/v1beta1/pipeline_validation.go	98.5%	98.9%	0.4
pkg/apis/pipeline/v1beta1/task_validation.go	97.1%	97.4%	0.4

lbernick · 2023-03-03T16:07:15Z

pkg/apis/pipeline/v1/pipeline_validation_test.go

 	},
 	} {
 		tt := tt // capture range variable
+		if tt.apiFields == config.AlphaAPIFields {


you can use this helper function: https://github.com/tektoncd/pipeline/blob/main/pkg/apis/config/feature_flags.go#L351

Yeah I noticed that function, but it is not an exported function, do you suggest we export it?

yeah I think it would be fine to export!

lbernick · 2023-03-03T16:07:24Z

pkg/apis/pipeline/v1/pipeline_validation_test.go

+			}},
+		},
+		params:    []Param{{Name: "second-param", Value: *NewStructuredValues("second-value", "second-value-again")}},
+		apiFields: config.AlphaAPIFields,


can you also add a test for beta api fields?

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?

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?

pkg/apis/pipeline/v1/pipeline_validation_test.go

lbernick · 2023-03-03T16:08:22Z

pkg/apis/pipeline/v1/pipeline_validation.go

@@ -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(`indexing into array params: %v require "enable-api-fields" feature gate to be "alpha" or "beta"`, arrayIndexParamRefs)


Suggested change

return fmt.Errorf(`indexing into array params: %v require "enable-api-fields" feature gate to be "alpha" or "beta"`, arrayIndexParamRefs)

return fmt.Errorf(`indexing into array params: %v requires "enable-api-fields" feature gate to be "alpha" or "beta"`, arrayIndexParamRefs)

This will be a list of params in %v why change it to requires ?

because "requires" refers to "indexing" not to "array params"

maybe invalid parameter expression %s: indexing into array params requires "enable-api-fields" feature gate to be "alpha" or "beta"

lbernick · 2023-03-03T16:09:55Z

pkg/apis/pipeline/v1beta1/pipeline_validation.go

@@ -581,5 +577,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(`indexing into array params: %v require "enable-api-fields" feature gate to be "alpha" or "beta"`, arrayIndexParamRefs)


Suggested change

return fmt.Errorf(`indexing into array params: %v require "enable-api-fields" feature gate to be "alpha" or "beta"`, arrayIndexParamRefs)

return fmt.Errorf(`indexing into array params: %v requires "enable-api-fields" feature gate to be "alpha" or "beta"`, arrayIndexParamRefs)

lbernick · 2023-03-03T16:13:01Z

pkg/apis/pipeline/v1beta1/pipeline_validation.go

@@ -540,10 +540,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) {


not in scope for this PR, but this function looks redundant with this one: https://github.com/tektoncd/pipeline/blob/main/pkg/apis/version/version_validation.go#L30

It would be good to remove CheckAlphaOrBetaAPIFields

CheckAlphaOrBetaAPIFields is equal to

ValidateEnabledAPIFields(ctx,"","alpha") || ValidateEnabledAPIFields(ctx,"","alpha")

So I think using one function to check alpha or beta is better? And ValidateEnabledAPIFields is mainly used for validation webhook.

I agree that one function would be better! CheckAlphaOrBetaAPIFields is equivalent to ValidateEnabledAPIFields(ctx, "", "beta"), which validates that the flag is set to beta stability or lower.

tekton-robot · 2023-03-03T18:14:40Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1/pipeline_validation.go	98.1%	98.6%	0.5
pkg/apis/pipeline/v1/task_validation.go	96.9%	97.3%	0.4
pkg/apis/pipeline/v1beta1/pipeline_validation.go	98.5%	98.9%	0.4
pkg/apis/pipeline/v1beta1/task_validation.go	97.1%	97.4%	0.4

tekton-robot · 2023-03-03T18:16:18Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1/pipeline_validation.go	98.1%	98.6%	0.5
pkg/apis/pipeline/v1/task_validation.go	96.9%	97.3%	0.4
pkg/apis/pipeline/v1beta1/pipeline_validation.go	98.5%	98.9%	0.4
pkg/apis/pipeline/v1beta1/task_validation.go	97.1%	97.4%	0.4

tekton-robot · 2023-03-10T20:38:52Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1/pipeline_validation.go	99.0%	99.5%	0.5
pkg/apis/pipeline/v1/task_validation.go	96.9%	97.3%	0.4
pkg/apis/pipeline/v1beta1/pipeline_validation.go	99.0%	99.5%	0.5
pkg/apis/pipeline/v1beta1/task_validation.go	96.9%	97.3%	0.4

tekton-robot · 2023-03-10T20:41:30Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1/pipeline_validation.go	99.0%	99.5%	0.5
pkg/apis/pipeline/v1/task_validation.go	96.9%	97.3%	0.4
pkg/apis/pipeline/v1beta1/pipeline_validation.go	99.0%	99.5%	0.5
pkg/apis/pipeline/v1beta1/task_validation.go	96.9%	97.3%	0.4

tekton-robot · 2023-03-10T20:43:33Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1/pipeline_validation.go	99.0%	99.5%	0.5
pkg/apis/pipeline/v1/task_validation.go	96.9%	97.3%	0.4
pkg/apis/pipeline/v1beta1/pipeline_validation.go	99.0%	99.5%	0.5
pkg/apis/pipeline/v1beta1/task_validation.go	96.9%	97.3%	0.4

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 [email protected]

tekton-robot · 2023-03-10T20:52:47Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1/pipeline_validation.go	99.0%	99.5%	0.5
pkg/apis/pipeline/v1/task_validation.go	96.9%	97.3%	0.4
pkg/apis/pipeline/v1beta1/pipeline_validation.go	99.0%	99.5%	0.5
pkg/apis/pipeline/v1beta1/task_validation.go	96.9%	97.3%	0.4

tekton-robot · 2023-03-10T20:53:40Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1/pipeline_validation.go	99.0%	99.5%	0.5
pkg/apis/pipeline/v1/task_validation.go	96.9%	97.3%	0.4
pkg/apis/pipeline/v1beta1/pipeline_validation.go	99.0%	99.5%	0.5
pkg/apis/pipeline/v1beta1/task_validation.go	96.9%	97.3%	0.4

QuanZhang-William · 2023-03-22T17:55:51Z

/assign

tekton-robot · 2023-03-22T22:52:49Z

@Yongxuanzhang: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

QuanZhang-William · 2023-03-23T19:51:54Z

pkg/apis/config/feature_flags.go

@@ -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 {


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)

QuanZhang-William · 2023-03-23T19:53:23Z

pkg/apis/pipeline/v1/pipeline_validation_test.go

+		name      string
+		original  PipelineSpec
+		params    []Param
+		apiFields string


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 🤔

tekton-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/bug Categorizes issue or PR as related to a bug. labels Mar 2, 2023

tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 2, 2023

tekton-robot requested review from afrittoli and lbernick March 2, 2023 22:17

Yongxuanzhang mentioned this pull request Mar 2, 2023

add feature gate check for param array indexing #6120

Closed

7 tasks

lbernick reviewed Mar 3, 2023

View reviewed changes

Yongxuanzhang force-pushed the feature-gate-check-for-array-indexing branch from 4c941dd to a95d40a Compare March 3, 2023 18:08

Yongxuanzhang requested review from lbernick and removed request for afrittoli March 3, 2023 18:42

Yongxuanzhang force-pushed the feature-gate-check-for-array-indexing branch from a95d40a to 68b412b Compare March 10, 2023 20:06

tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 10, 2023

Yongxuanzhang force-pushed the feature-gate-check-for-array-indexing branch 2 times, most recently from 1ecb863 to 13afb11 Compare March 10, 2023 20:31

tekton-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 10, 2023

Yongxuanzhang force-pushed the feature-gate-check-for-array-indexing branch from 13afb11 to 80b2502 Compare March 10, 2023 20:35

Yongxuanzhang force-pushed the feature-gate-check-for-array-indexing branch from 80b2502 to 24c4db0 Compare March 10, 2023 20:43

Yongxuanzhang force-pushed the feature-gate-check-for-array-indexing branch from 24c4db0 to 2e24d75 Compare March 10, 2023 20:46

tekton-robot assigned QuanZhang-William Mar 22, 2023

lbernick self-assigned this Mar 22, 2023

tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 22, 2023

QuanZhang-William reviewed Mar 23, 2023

View reviewed changes

Yongxuanzhang closed this May 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add feature gate validation for param array indexing #6280

add feature gate validation for param array indexing #6280

Yongxuanzhang commented Mar 2, 2023 •

edited

Loading

tekton-robot commented Mar 2, 2023

tekton-robot commented Mar 2, 2023

tekton-robot commented Mar 2, 2023

lbernick Mar 3, 2023

Yongxuanzhang Mar 3, 2023 •

edited

Loading

lbernick Mar 3, 2023

lbernick Mar 3, 2023

Yongxuanzhang Mar 3, 2023

lbernick Mar 3, 2023

lbernick Mar 3, 2023

Yongxuanzhang Mar 3, 2023

lbernick Mar 3, 2023

lbernick Mar 3, 2023

lbernick Mar 3, 2023

lbernick Mar 3, 2023

Yongxuanzhang Mar 3, 2023

lbernick Mar 3, 2023

tekton-robot commented Mar 3, 2023

tekton-robot commented Mar 3, 2023

tekton-robot commented Mar 10, 2023

tekton-robot commented Mar 10, 2023

tekton-robot commented Mar 10, 2023

tekton-robot commented Mar 10, 2023

tekton-robot commented Mar 10, 2023

QuanZhang-William commented Mar 22, 2023

tekton-robot commented Mar 22, 2023

QuanZhang-William Mar 23, 2023

QuanZhang-William Mar 23, 2023

	return fmt.Errorf(`indexing into array params: %v require "enable-api-fields" feature gate to be "alpha" or "beta"`, arrayIndexParamRefs)
	return fmt.Errorf(`indexing into array params: %v requires "enable-api-fields" feature gate to be "alpha" or "beta"`, arrayIndexParamRefs)

add feature gate validation for param array indexing #6280

add feature gate validation for param array indexing #6280

Conversation

Yongxuanzhang commented Mar 2, 2023 • edited Loading

Changes

Submitter Checklist

Release Notes

tekton-robot commented Mar 2, 2023

tekton-robot commented Mar 2, 2023

tekton-robot commented Mar 2, 2023

Choose a reason for hiding this comment

Yongxuanzhang Mar 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tekton-robot commented Mar 3, 2023

tekton-robot commented Mar 3, 2023

tekton-robot commented Mar 10, 2023

tekton-robot commented Mar 10, 2023

tekton-robot commented Mar 10, 2023

tekton-robot commented Mar 10, 2023

tekton-robot commented Mar 10, 2023

QuanZhang-William commented Mar 22, 2023

tekton-robot commented Mar 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yongxuanzhang commented Mar 2, 2023 •

edited

Loading

Yongxuanzhang Mar 3, 2023 •

edited

Loading