Validate beta features only when v1 Tasks and Pipelines are defined #6701

lbernick · 2023-05-22T19:41:58Z

Currently, validation differs between api versions: when beta features are used in v1 APIs,
"enable-api-fields" must be set to "alpha" or "beta", but when beta features are used in beta APIs,
"enable-api-fields" may be set to "alpha", "beta", or "stable".

We also validate the specs of referenced Tasks or Pipelines in the TaskRun/PipelineRun reconciler.
This presents a problem when referencing a Task or Pipeline declared locally, since the Task or Pipeline may be converted into a different API version when it's stored.

This commit moves validation for beta features to apply only to Tasks and Pipelines when they are created or updated,
and not called when a Task spec or Pipeline spec is validated.

This commit will allow us to swap the storage version of our API without user-facing impact.
Separately, we plan to decouple feature versioning from API versioning, as it is a better long-term solution (#6592).

This commit contains no expected functional changes, since the TaskRun and PipelineRun reconcilers do not
currently validate that"enable-api-fields" is set to "alpha" or "beta" when beta features are used in
referenced Tasks or Pipelines.

This validation will be added for v1 remote Tasks and Pipelines in a separate commit (#6725).

Closes #6616.
/kind misc

Submitter Checklist

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

n/a Has Docs if any changes are user facing, including updates to minimum requirements e.g. Kubernetes version bumps
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
n/a Release notes block below has been updated with any user facing changes (API changes, bug fixes, changes requiring upgrade notices or deprecation warnings). See some examples of good release notes.
n/a Release notes contains the string "action required" if the change requires additional action from users switching to the new release

Release Notes

NONE

tekton-robot · 2023-05-22T19:47:06Z

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.7%	99.7%	0.0
pkg/apis/pipeline/v1/task_validation.go	97.3%	97.4%	0.1

tekton-robot · 2023-05-22T19:51:53Z

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.7%	99.7%	0.0
pkg/apis/pipeline/v1/task_validation.go	97.3%	97.4%	0.1

tekton-robot · 2023-05-23T13:17:55Z

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.7%	99.7%	0.0
pkg/apis/pipeline/v1/task_validation.go	97.3%	97.3%	0.1

tekton-robot · 2023-05-23T13:20:01Z

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.7%	99.7%	0.0
pkg/apis/pipeline/v1/task_validation.go	97.3%	97.3%	0.1

tekton-robot · 2023-05-23T13:31:44Z

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.7%	99.7%	0.0
pkg/apis/pipeline/v1/task_validation.go	97.3%	97.3%	0.1

tekton-robot · 2023-05-23T13:35:03Z

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.7%	99.7%	0.0
pkg/apis/pipeline/v1/task_validation.go	97.3%	97.3%	0.1

JeromeJu · 2023-05-23T13:36:14Z

/assign

vdemeester

Couldn't we use the context (ctx) to kno when to validate and when to not validate (as there is already information in there about if it's at creation, or update, etc..) ?

One of the shortcomings of this is, if the webhook is not ready/registered, one might be able to create something that is not valid (aka uses beta fields while the configuration is set to stable) and get it executed. This, however, can only happen explicitely (user removes the webhook/admission controller registration) or at installation (in a very short period of time where CRDs are there but the webhook has not fully started…).

lbernick · 2023-05-23T15:41:27Z

chatted offline with @vdemeester, but this PR doesn't actually change anything from the current implementation. Right now, we don't do any validation in the reconciler for beta fields. We do call taskSpec.Validate and pipelineSpec.Validate in the reconciler, but the fetched pipeline or task is treated as v1beta1, so no validation of beta features is done:

pipeline/pkg/reconciler/pipelinerun/pipelinerun.go

Line 448 in dffb5f4

if err := pipelineSpec.Validate(ctx); err != nil {

We could discuss future improvements guarding against the webhook being disabled or not ready, but this PR doesn't change our current validation behavior, it just makes it possible for us to swap to v1 as our storage version without breaking v1beta1 pipelines using beta features.

We could also choose to add beta features validation for v1 remote tasks here:

pipeline/pkg/reconciler/pipelinerun/resources/pipelineref.go

Lines 163 to 173 in dffb5f4

    
           // TODO(#6356): Support V1 Task verification 
        
           t := &v1beta1.Pipeline{ 
        
           	TypeMeta: metav1.TypeMeta{ 
        
           		Kind:       "Pipeline", 
        
           		APIVersion: "tekton.dev/v1beta1", 
        
           	}, 
        
           } 
        
           if err := t.ConvertFrom(ctx, obj); err != nil { 
        
           	return nil, fmt.Errorf("failed to convert obj %s into Pipeline", obj.GetObjectKind().GroupVersionKind().String()) 
        
           } 
        
           return t, nil

Note that this would be a change from what we're doing today.

vdemeester · 2023-05-23T15:45:34Z

I am a bit neutral on this, I don't really like that approach too much, but I don't hate it either, and it does unblock switching the storage version. As a side note, I don't really see a big rush to switch the storage version (as discussed during the API WG) but I also don't see too much reason to block that either.

I would love to have opinion of others @tektoncd/core-maintainers 👼🏼

lbernick · 2023-05-23T15:54:27Z

@vdemeester could you elaborate on your concerns with this proposal or any problems you think we might run into? We could definitely brainstorm ways to make the reconciler more resilient to issues with the webhook validation being accidentally skipped, but I don't think that should block this PR.

tekton-robot · 2023-05-24T16:43: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	99.7%	99.7%	0.0
pkg/apis/pipeline/v1/task_validation.go	97.3%	97.3%	0.1

tekton-robot · 2023-05-24T16:45:31Z

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.7%	99.7%	0.0
pkg/apis/pipeline/v1/task_validation.go	97.3%	97.3%	0.1

lbernick · 2023-05-24T17:37:58Z

known flake: #6672
/retest

dibyom · 2023-05-24T18:59:08Z

hey @lbernick this is neat and generally I like the approach. I have two questions -

post v1 swap, a user has a v1beta1 pipeline that contains object params that they are referencing in a v1beta1 pipelinerun (with the default values of enable-api-fields set) - will this pipeline work or fail validation?
today i.e. pre v1 swap, if I reference a v1 pipeline with beta fields via remote resolution, does that trigger a failed validation as well?

lbernick · 2023-05-24T19:07:06Z

hey @lbernick this is neat and generally I like the approach. I have two questions -

post v1 swap, a user has a v1beta1 pipeline that contains object params that they are referencing in a v1beta1 pipelinerun (with the default values of enable-api-fields set) - will this pipeline work or fail validation?

In the reconciler, we only call pipelineSpec.Validate, not pipelineRunSpec.Validate. We do not currently validate that enable-api-fields is set to "beta" for beta features in the reconciler, and this commit means we will still not do so even after swapping to v1. Therefore, this example should work.

today i.e. pre v1 swap, if I reference a v1 pipeline with beta fields via remote resolution, does that trigger a failed validation as well?

It doesn't. This is an example of how validation for remote Pipelines differs from validation of local Pipelines. We should probably add this validation, but I don't think that should block this PR.

tekton-robot · 2023-05-25T14:13:24Z

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.7%	99.7%	0.0
pkg/apis/pipeline/v1/task_validation.go	97.3%	97.3%	0.1

dibyom

Code LGTM, but let's wait till the pipeline wg next week to merge to align on the discussions around alternatives to this and why we need this PR - I wrote up my understanding of the summary here: #6592 (comment)

dibyom

Code LGTM, but let's wait till the pipeline wg next week to merge to align on the discussions around alternatives to this and why we need this PR - I wrote up my understanding of the summary here: #6592 (comment)

lbernick · 2023-05-26T20:26:14Z

/hold cancel
removing the hold just to avoid confusion-- I wouldn't want people to avoid reviewing since there's a hold on it. @JeromeJu if you want to avoid this getting merged you can approve instead of lgtm!

tekton-robot · 2023-05-26T20:32:31Z

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.7%	99.7%	0.0
pkg/apis/pipeline/v1/task_validation.go	97.3%	97.3%	0.0

tekton-robot · 2023-05-26T20:33:56Z

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.7%	99.7%	0.0
pkg/apis/pipeline/v1/task_validation.go	97.3%	97.3%	0.0

tekton-robot · 2023-05-26T21:12:55Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dibyom, JeromeJu, vdemeester

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~pkg/apis/OWNERS~~ [dibyom,vdemeester]

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

pritidesai · 2023-05-26T21:27:20Z

pkg/apis/pipeline/v1/pipeline_validation.go

@@ -70,7 +76,6 @@ func (ps *PipelineSpec) Validate(ctx context.Context) (errs *apis.FieldError) {
 	errs = errs.Also(validatePipelineContextVariables(ps.Tasks).ViaField("tasks"))
 	errs = errs.Also(validatePipelineContextVariables(ps.Finally).ViaField("finally"))
 	errs = errs.Also(validateExecutionStatusVariables(ps.Tasks, ps.Finally))
-	errs = errs.Also(ps.ValidateBetaFields(ctx))


just to make it clear for the reviewers, this PR is moving this validation from func (ps *PipelineSpec) Validate() to func (p *Pipeline) Validate() such that the validation ValidateBetaFields is done during pipeline creation time compared to pipelineRun.

pkg/apis/pipeline/v1/task_validation_test.go

Currently, validation differs between api versions: when beta features are used in v1 APIs, "enable-api-fields" must be set to "alpha" or "beta", but when beta features are used in beta APIs, "enable-api-fields" may be set to "alpha", "beta", or "stable". We also validate the specs of referenced Tasks or Pipelines in the TaskRun/PipelineRun reconciler. This presents a problem when referencing a Task or Pipeline declared locally, since the Task or Pipeline may be converted into a different API version when it's stored. This commit moves validation for beta features to apply only to Tasks and Pipelines when they are created or updated, and not called when a Task spec or Pipeline spec is validated. This commit will allow us to swap the storage version of our API without user-facing impact. Separately, we plan to decouple feature versioning from API versioning, as it is a better long-term solution. This commit contains no expected functional changes, since the TaskRun and PipelineRun reconcilers do not currently validate that"enable-api-fields" is set to "alpha" or "beta" when beta features are used in referenced Tasks or Pipelines. This validation will be added for v1 remote Tasks and Pipelines in a separate commit.

lbernick · 2023-05-30T18:04:27Z

Code LGTM, but let's wait till the pipeline wg next week to merge to align on the discussions around alternatives to this and why we need this PR - I wrote up my understanding of the summary here: #6592 (comment)

discussion at Pipelines WG today-- no concerns re: merging this PR

JeromeJu · 2023-05-30T18:07:04Z

/lgtm
as discussed in the WG and the issue itself

tekton-robot · 2023-05-30T18:09:04Z

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.7%	99.7%	0.0
pkg/apis/pipeline/v1/task_validation.go	97.3%	97.3%	0.0

tekton-robot · 2023-05-30T18:11:07Z

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.7%	99.7%	0.0
pkg/apis/pipeline/v1/task_validation.go	97.3%	97.3%	0.0

lbernick · 2023-05-30T18:44:51Z

known flake: #6672
This flake is marked fixed, but it was fixed only a few hours ago so maybe the fix has not been picked up by this PR yet? I'm not sure exactly how our merge automation works
/retest

tekton-robot · 2023-05-30T19:11:43Z

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.7%	99.7%	0.0
pkg/apis/pipeline/v1/task_validation.go	97.3%	97.3%	0.0

tekton-robot · 2023-05-30T19:14:04Z

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.7%	99.7%	0.0
pkg/apis/pipeline/v1/task_validation.go	97.3%	97.3%	0.0

Yongxuanzhang · 2023-05-30T19:17:28Z

known flake: #6672 This flake is marked fixed, but it was fixed only a few hours ago so maybe the fix has not been picked up by this PR yet? I'm not sure exactly how our merge automation works /retest

If that fix works, and the PR has rebased from the main. Then we shouldn’t encounter the flaky again？
Or maybe I'm wrong... We should keep an eye on tests and reopen it if it fixed

tekton-robot added kind/misc Categorizes issue or PR as a miscellaneuous one. release-note-none Denotes a PR that doesnt merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 22, 2023

tekton-robot requested review from bobcatfish and imjasonh May 22, 2023 19:42

lbernick mentioned this pull request May 22, 2023

Versioned validation of referenced Pipelines/Tasks #6616

Closed

lbernick force-pushed the validate branch from 6235ebe to 9830cf7 Compare May 23, 2023 13:12

lbernick force-pushed the validate branch from 9830cf7 to 3485ffb Compare May 23, 2023 13:25

tekton-robot assigned JeromeJu May 23, 2023

vdemeester reviewed May 23, 2023

View reviewed changes

lbernick force-pushed the validate branch from 3485ffb to 7ad93df Compare May 24, 2023 16:38

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

lbernick force-pushed the validate branch from 7ad93df to 8aeba51 Compare May 25, 2023 14:07

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

dibyom approved these changes May 26, 2023

View reviewed changes

lbernick force-pushed the validate branch from 6d6720b to 17da549 Compare May 26, 2023 20:25

tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label May 26, 2023

tekton-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 26, 2023

JeromeJu approved these changes May 26, 2023

View reviewed changes

pritidesai reviewed May 26, 2023

View reviewed changes

lbernick added this to the Pipelines v0.49 milestone May 30, 2023

Yongxuanzhang reviewed May 30, 2023

View reviewed changes

pkg/apis/pipeline/v1/task_validation_test.go Show resolved Hide resolved

lbernick force-pushed the validate branch from 17da549 to 412fbce Compare May 30, 2023 18:03

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label May 30, 2023

lbernick closed this May 30, 2023

lbernick reopened this May 30, 2023

tekton-robot merged commit 14c26f8 into tektoncd:main May 30, 2023

lbernick mentioned this pull request Jun 8, 2023

How can feature flags in the provenance better reflect the execution of the pipeline run? #6797

Open

JeromeJu mentioned this pull request Aug 29, 2023

Beta features for inline Task and Pipeline not validated #7077

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate beta features only when v1 Tasks and Pipelines are defined #6701

Validate beta features only when v1 Tasks and Pipelines are defined #6701

lbernick commented May 22, 2023 •

edited

Loading

tekton-robot commented May 22, 2023

tekton-robot commented May 22, 2023

tekton-robot commented May 23, 2023

tekton-robot commented May 23, 2023

tekton-robot commented May 23, 2023

tekton-robot commented May 23, 2023

JeromeJu commented May 23, 2023

vdemeester left a comment

lbernick commented May 23, 2023 •

edited

Loading

vdemeester commented May 23, 2023

lbernick commented May 23, 2023

tekton-robot commented May 24, 2023

tekton-robot commented May 24, 2023

lbernick commented May 24, 2023

dibyom commented May 24, 2023

lbernick commented May 24, 2023

tekton-robot commented May 25, 2023

dibyom left a comment

dibyom left a comment

lbernick commented May 26, 2023

tekton-robot commented May 26, 2023

tekton-robot commented May 26, 2023

tekton-robot commented May 26, 2023

pritidesai May 26, 2023

lbernick commented May 30, 2023

JeromeJu commented May 30, 2023

tekton-robot commented May 30, 2023

tekton-robot commented May 30, 2023

lbernick commented May 30, 2023

tekton-robot commented May 30, 2023

tekton-robot commented May 30, 2023

Yongxuanzhang commented May 30, 2023 •

edited

Loading

Validate beta features only when v1 Tasks and Pipelines are defined #6701

Validate beta features only when v1 Tasks and Pipelines are defined #6701

Conversation

lbernick commented May 22, 2023 • edited Loading

Submitter Checklist

Release Notes

tekton-robot commented May 22, 2023

tekton-robot commented May 22, 2023

tekton-robot commented May 23, 2023

tekton-robot commented May 23, 2023

tekton-robot commented May 23, 2023

tekton-robot commented May 23, 2023

JeromeJu commented May 23, 2023

vdemeester left a comment

Choose a reason for hiding this comment

lbernick commented May 23, 2023 • edited Loading

vdemeester commented May 23, 2023

lbernick commented May 23, 2023

tekton-robot commented May 24, 2023

tekton-robot commented May 24, 2023

lbernick commented May 24, 2023

dibyom commented May 24, 2023

lbernick commented May 24, 2023

tekton-robot commented May 25, 2023

dibyom left a comment

Choose a reason for hiding this comment

dibyom left a comment

Choose a reason for hiding this comment

lbernick commented May 26, 2023

tekton-robot commented May 26, 2023

tekton-robot commented May 26, 2023

tekton-robot commented May 26, 2023

pritidesai May 26, 2023

Choose a reason for hiding this comment

lbernick commented May 30, 2023

JeromeJu commented May 30, 2023

tekton-robot commented May 30, 2023

tekton-robot commented May 30, 2023

lbernick commented May 30, 2023

tekton-robot commented May 30, 2023

tekton-robot commented May 30, 2023

Yongxuanzhang commented May 30, 2023 • edited Loading

lbernick commented May 22, 2023 •

edited

Loading

lbernick commented May 23, 2023 •

edited

Loading

Yongxuanzhang commented May 30, 2023 •

edited

Loading