Move validation from `pipelinespec` to `pipelinerunspec` when propagating parameters #5291

chitrangpatel · 2022-08-09T15:30:46Z

Changes

Prior to this, propagating parameters only skipped webhook validation for params defined in script. As a result, when users tried to propagate params to other fields like args or command, etc. an webhook validation was raised. This PR address issue #5141. This PR addresses validations in pipelinespec and pipelinerunspec. The changes for validations in taskspec and taskrunspec are in a separate PR.

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

Move parameter validation from `pipelinespec` to `pipelinerunspec` when propagating parameters

chitrangpatel · 2022-08-09T15:31:19Z

/kind bug

tekton-robot · 2022-08-09T15:34:59Z

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.4%	99.4%	0.0
pkg/reconciler/pipelinerun/pipelinerun.go	86.0%	86.0%	0.0

chitrangpatel · 2022-08-09T15:44:02Z

/hold Wait for PR to be merged first

chitrangpatel · 2022-08-12T13:56:33Z

/hold cancel

tekton-robot · 2022-08-12T14:03:23Z

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.4%	99.4%	0.0
pkg/reconciler/pipelinerun/pipelinerun.go	85.7%	85.7%	0.0

jerop

thanks @chitrangpatel!

pkg/apis/pipeline/v1beta1/pipelinerun_validation.go

pkg/apis/pipeline/v1beta1/pipelinerun_validation_test.go

tekton-robot · 2022-08-12T14:23:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jerop

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:

~~OWNERS~~ [jerop]

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

tekton-robot · 2022-08-12T14:49:15Z

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.4%	99.4%	0.0
pkg/reconciler/pipelinerun/pipelinerun.go	85.7%	85.7%	0.0

chitrangpatel · 2022-08-12T15:14:34Z

/retest timeout flake again 🙁

tekton-robot · 2022-08-12T15:14:35Z

@chitrangpatel: The /retest command does not accept any targets.
The following commands are available to trigger required jobs:

/test pull-tekton-pipeline-alpha-integration-tests
/test pull-tekton-pipeline-build-tests
/test pull-tekton-pipeline-integration-tests
/test tekton-pipeline-unit-tests

The following commands are available to trigger optional jobs:

/test pull-tekton-pipeline-go-coverage

Use /test all to run all jobs.

In response to this:

/retest timeout flake again 🙁

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.

jerop · 2022-08-12T15:18:54Z

/retest

@chitrangpatel /retest won't work if you have something else in the same line

abayer · 2022-08-12T15:34:07Z

pkg/apis/pipeline/v1/pipeline_validation.go

@@ -38,6 +39,7 @@ func (p *Pipeline) Validate(ctx context.Context) *apis.FieldError {
 	if apis.IsInDelete(ctx) {
 		return nil
 	}
+	ctx = config.SetValidateParameterVariablesAndWorkspaces(ctx, true)


I should have mentioned this on the other PR, but this name is confusing to me. Just based on the name alone, it's not really clear what changes when you set this. It feels like it's saying "ok, validate parameters and workspaces" in general, which seems like something you'd never actually want to turn off. Could we get this a more clear name to better signify exactly what it's doing?

Yes, that's right. How about I have a flag that is defaulted to true and we only call it when we intend to make it false?

e.g.

we could have config.DoNotValidateParameterVariablesAndWorkspaces(ctx) which would set it the underlying context field to false.

We also have a function config.ValidateParameterVariablesAndWorkspaces(ctx) which returns a true or false based on the value of the context field. This function is called by task_validation to decide whether to validate parameters and workspaces. We can have it return true by default.

Does that look like a good solution?

I'd really like the name to be more clear than ValidateParameterVariablesAndWorkspaces - that name is just too vague. What does it relate to? Which parameter variables and workspaces? If we're just using it for parameter propagation cases, it feels like it would make more sense to have the name be something regarding parameter propagation validation, i.e., why we're setting this flag, not what the result of setting this flag is.

I see what you mean. Its trying to skip validation when it does not contain the full information. e.g. When propagating parameters, a user could provide the params at the task run or the pipeline run level and directly call them in the steps. However, when we validate the underlying taskSpec or pipelineSpec, we don't have these params declared there and so the web hook would throw an error. This flag allows us to specify where to skip validation of params due to lack of information.

I could call the it SkipValidationDueToPropagatedParametersAndWorkspaces (a bit of a mouthful, but ok I guess)?

That would definitely be better, yeah.

I will make the appropriate changes.

@abayer done! Thanks for the suggestion.

tekton-robot · 2022-08-12T17:45:53Z

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.4%	99.4%	0.0

…ting parameters Prior to this, propagating parameters only skipped webhook validation for params defined in script. As a result, when users tried to propagate params to other fields like `args` or `command`, etc. an webhook validation was raised. This PR address this issue.

tekton-robot · 2022-08-12T17:58:17Z

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.4%	99.4%	0.0
pkg/reconciler/pipelinerun/pipelinerun.go	85.8%	85.8%	0.0

abayer · 2022-08-12T18:21:34Z

/retest

abayer · 2022-08-15T14:19:34Z

/lgtm

JeromeJu · 2022-08-29T15:20:29Z

It seems that some ArrayOrString params in https://github.com/tektoncd/pipeline/blob/main/pkg/apis/pipeline/v1beta1/pipelinerun_validation_test.go/#L130 should be updated to ParamValue.

cc @Yongxuanzhang @chitrangpatel Could you please help confirm here?

chitrangpatel · 2022-08-29T15:22:52Z

I agree. I don't recall which PR was merged first. There is backwards compatibility which is why the tests work. However, we should make it ParamValue. I can do that in one shot in a separate PR.

JeromeJu · 2022-08-29T15:23:27Z

Thanks @chitrangpatel

tekton-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Aug 9, 2022

tekton-robot requested review from abayer and jerop August 9, 2022 15:30

tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 9, 2022

tekton-robot added the kind/bug Categorizes issue or PR as related to a bug. label Aug 9, 2022

chitrangpatel mentioned this pull request Aug 9, 2022

Move parameter validation from taskspec to taskrunspec when propagating parameters #5143

Merged

7 tasks

tekton-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 9, 2022

chitrangpatel marked this pull request as draft August 9, 2022 15:44

tekton-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 9, 2022

chitrangpatel force-pushed the Bugfix-5141-validating-propagated-params-pipelines branch from 91e9714 to 1280803 Compare August 12, 2022 02:44

chitrangpatel marked this pull request as ready for review August 12, 2022 13:56

tekton-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 12, 2022

tekton-robot requested review from dlorenc and pritidesai August 12, 2022 13:56

tekton-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 12, 2022

jerop approved these changes Aug 12, 2022

View reviewed changes

pkg/apis/pipeline/v1beta1/pipelinerun_validation.go Outdated Show resolved Hide resolved

pkg/apis/pipeline/v1beta1/pipelinerun_validation_test.go Outdated Show resolved Hide resolved

pkg/apis/pipeline/v1beta1/pipelinerun_validation_test.go Outdated Show resolved Hide resolved

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 12, 2022

chitrangpatel force-pushed the Bugfix-5141-validating-propagated-params-pipelines branch from 1280803 to 14f4901 Compare August 12, 2022 14:45

abayer reviewed Aug 12, 2022

View reviewed changes

chitrangpatel force-pushed the Bugfix-5141-validating-propagated-params-pipelines branch from 14f4901 to d529a7b Compare August 12, 2022 17:41

chitrangpatel force-pushed the Bugfix-5141-validating-propagated-params-pipelines branch from d529a7b to 5f503b5 Compare August 12, 2022 17:53

tekton-robot assigned abayer Aug 15, 2022

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 15, 2022

tekton-robot merged commit 06a2887 into tektoncd:main Aug 15, 2022

jerop mentioned this pull request Aug 15, 2022

TEP0107 - Parameter propagation doesn't work for taskSpec-steps-args #5141

Closed

JeromeJu mentioned this pull request Aug 16, 2022

Add V1 TaskRun Golang structs #5264

Merged

5 tasks

chitrangpatel mentioned this pull request Aug 24, 2022

Propagating object params #5371

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move validation from `pipelinespec` to `pipelinerunspec` when propagating parameters #5291

Move validation from `pipelinespec` to `pipelinerunspec` when propagating parameters #5291

chitrangpatel commented Aug 9, 2022 •

edited by jerop

Loading

chitrangpatel commented Aug 9, 2022

tekton-robot commented Aug 9, 2022

chitrangpatel commented Aug 9, 2022

chitrangpatel commented Aug 12, 2022

tekton-robot commented Aug 12, 2022

jerop left a comment

tekton-robot commented Aug 12, 2022

tekton-robot commented Aug 12, 2022

chitrangpatel commented Aug 12, 2022

tekton-robot commented Aug 12, 2022

jerop commented Aug 12, 2022 •

edited

Loading

abayer Aug 12, 2022

chitrangpatel Aug 12, 2022

abayer Aug 12, 2022

chitrangpatel Aug 12, 2022

abayer Aug 12, 2022

chitrangpatel Aug 12, 2022

chitrangpatel Aug 12, 2022

tekton-robot commented Aug 12, 2022

tekton-robot commented Aug 12, 2022

abayer commented Aug 12, 2022

abayer commented Aug 15, 2022

JeromeJu commented Aug 29, 2022

chitrangpatel commented Aug 29, 2022

JeromeJu commented Aug 29, 2022

Move validation from pipelinespec to pipelinerunspec when propagating parameters #5291

Move validation from pipelinespec to pipelinerunspec when propagating parameters #5291

Conversation

chitrangpatel commented Aug 9, 2022 • edited by jerop Loading

Changes

Submitter Checklist

Release Notes

chitrangpatel commented Aug 9, 2022

tekton-robot commented Aug 9, 2022

chitrangpatel commented Aug 9, 2022

chitrangpatel commented Aug 12, 2022

tekton-robot commented Aug 12, 2022

jerop left a comment

Choose a reason for hiding this comment

tekton-robot commented Aug 12, 2022

tekton-robot commented Aug 12, 2022

chitrangpatel commented Aug 12, 2022

tekton-robot commented Aug 12, 2022

jerop commented Aug 12, 2022 • edited Loading

abayer Aug 12, 2022

Choose a reason for hiding this comment

chitrangpatel Aug 12, 2022

Choose a reason for hiding this comment

abayer Aug 12, 2022

Choose a reason for hiding this comment

chitrangpatel Aug 12, 2022

Choose a reason for hiding this comment

abayer Aug 12, 2022

Choose a reason for hiding this comment

chitrangpatel Aug 12, 2022

Choose a reason for hiding this comment

chitrangpatel Aug 12, 2022

Choose a reason for hiding this comment

tekton-robot commented Aug 12, 2022

tekton-robot commented Aug 12, 2022

abayer commented Aug 12, 2022

abayer commented Aug 15, 2022

JeromeJu commented Aug 29, 2022

chitrangpatel commented Aug 29, 2022

JeromeJu commented Aug 29, 2022

Move validation from `pipelinespec` to `pipelinerunspec` when propagating parameters #5291

Move validation from `pipelinespec` to `pipelinerunspec` when propagating parameters #5291

chitrangpatel commented Aug 9, 2022 •

edited by jerop

Loading

jerop commented Aug 12, 2022 •

edited

Loading