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.

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

ValidateParamArrayIndex for taskSpec should check beta flag. #6607

Closed
Yongxuanzhang opened this issue May 1, 2023 · 3 comments · Fixed by #6613
Closed

ValidateParamArrayIndex for taskSpec should check beta flag. #6607

Yongxuanzhang opened this issue May 1, 2023 · 3 comments · Fixed by #6613
Assignees
Labels
kind/bug Categorizes issue or PR as related to a bug.

Comments

@Yongxuanzhang
Copy link
Member

Yongxuanzhang commented May 1, 2023

Expected Behavior

func (ts *TaskSpec) ValidateParamArrayIndex(ctx context.Context, params Params) error {
cfg := config.FromContextOrDefaults(ctx)
if cfg.FeatureFlags.EnableAPIFields != config.AlphaAPIFields {
return nil
}

should check beta flag instead of alpha flag.

Thanks @dibyom for catching this bug.

Actual Behavior

taskSpec's ValidateParamArrayIndex checks alpha flag.

Additional Info

Array indexing (Tep 76) is promoted by #6103, yet the feature flag check for taskSpec's ValidateParamArrayIndex is not updated. The resulting consequence is that the validation is skipped for taskSpec when this feature is used and the api feature flag is set to beta

  • Kubernetes version:

    Output of kubectl version:

(paste your output here)
  • Tekton Pipeline version:

Version since v0.46

@Yongxuanzhang Yongxuanzhang added the kind/bug Categorizes issue or PR as related to a bug. label May 1, 2023
@Yongxuanzhang
Copy link
Member Author

/assign

Yongxuanzhang added a commit to Yongxuanzhang/pipeline that referenced this issue May 2, 2023
This commit closes tektoncd#6607. TaskSpec's member function
ValidateParamArrayIndex checks alpha feature flag, but array indexing is
promoted to beta feature and we should check beta flag instead. This
commit fixes this issue.

Signed-off-by: Yongxuan Zhang [email protected]
Yongxuanzhang added a commit to Yongxuanzhang/pipeline that referenced this issue May 3, 2023
This commit closes tektoncd#6607. V1 TaskSpec's member function
ValidateParamArrayIndex checks alpha feature flag, but array indexing is
promoted to beta feature and we should check beta flag instead. And for
v1beta1 the beta is on by default so we don't check the beta flag. This
commit fixes this issue.

Signed-off-by: Yongxuan Zhang [email protected]
Yongxuanzhang added a commit to Yongxuanzhang/pipeline that referenced this issue May 3, 2023
This commit closes tektoncd#6607. V1 TaskSpec's member function
ValidateParamArrayIndex checks alpha feature flag, but array indexing is
promoted to beta feature and we should check beta flag instead. And for
v1beta1 the beta is on by default so we don't check the beta flag. This
commit fixes this issue.

Signed-off-by: Yongxuan Zhang [email protected]
@lbernick
Copy link
Member

lbernick commented May 3, 2023

This is a bit more complicated; see #6616 for more detail.

Here's what I think the logic should look like:

  • If a task uses indexing into array params, we should validate in the webhook that "enable-api-fields" is set to a value that permits using this feature
  • If we cannot validate in the webhook since the task is e.g. stored in git, we should validate in the reconciler that "enable-api-fields" is set to a value that permits using this feature before converting the task to v1beta1 (or honestly we could skip this validation)
  • if the task's array param indexes are out of bounds based on the params provided, we should validate this in the reconciler, separately from the value of "enable-api-fields"

@dibyom
Copy link
Member

dibyom commented May 3, 2023

If we cannot validate in the webhook since the task is e.g. stored in git, we should validate in the reconciler that "enable-api-fields" is set to a value that permits using this feature before converting the task to v1beta1 (or honestly we could skip this validation)

wouldn't skipping the validation result in referenced pipelines and in cluster pipelines being validated differently?

Yongxuanzhang added a commit to Yongxuanzhang/pipeline that referenced this issue May 9, 2023
This commit closes tektoncd#6607. V1 TaskSpec's member function
ValidateParamArrayIndex checks alpha feature flag, but array indexing is
promoted to beta feature and we should check beta flag instead. And for
v1beta1 the beta is on by default so we don't check the beta flag. This
commit fixes this issue and also update doc.

Signed-off-by: Yongxuan Zhang [email protected]
tekton-robot pushed a commit that referenced this issue May 9, 2023
This commit closes #6607. V1 TaskSpec's member function
ValidateParamArrayIndex checks alpha feature flag, but array indexing is
promoted to beta feature and we should check beta flag instead. And for
v1beta1 the beta is on by default so we don't check the beta flag. This
commit fixes this issue and also update doc.

Signed-off-by: Yongxuan Zhang [email protected]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants