Validate PipelineRun Parameters #2719

jerop · 2020-05-29T21:33:30Z

Changes

While working on allowing PipelineRuns to provide extra parameters (#2513), we found that providing extra parameters was unintentionally allowed. That's because, currently, there's no validation that all parameters expected by the Pipeline is provided by the PipelineRun.

In this PR, we add validation for PipelineRun parameters by generating a list of provided parameters then iterating through the expected parameters to ensure they are in the list of provided parameters. Note that parameters which have default values specified in Pipeline are not required to be provided by PipelineRun.

In the validation, we still allow PipelineRuns to provide extra parameters. If we disallow PipelineRuns from providing extra parameters, the Pipelines would fail. As a result, systems that autogenerate PipelineRuns would need to look at each pipeline to see what parameters they need so it can provide only the required parameters. That means users would have to resort to more complex designs to solve this issue, as further described in (#2513).

By allowing PipelineRuns to provide extra parameters, we make the process of autogenerating of PipelineRuns simpler.

Fixes #2708.

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

Includes tests (if functionality changed/added)
Includes docs (if user facing)
Commit messages follow commit message best practices

See the contribution guide for more details.

Double check this list of stuff that's easy to miss:

If you are adding a new binary/image to the cmd dir, please update
the release Task to build and release this image.

Reviewer Notes

If API changes are included, additive changes must be approved by at least two OWNERS and backwards incompatible changes must be approved by more than 50% of the OWNERS, and they must first be added in a backwards compatible way.

Release Notes

- PipelineRun parameters are validated to ensure that all the parameters required by the Pipeline are provided by the PipelineRun.

- PipelineRun parameters validation allows PipelineRuns to provide extra parameters in addition to the required parameters.

- Warning: backwards incompatible change that will force users to pass in default parameter values or provide the required parameters.

tekton-robot · 2020-05-29T21:33:54Z

This PR cannot be merged: expecting exactly one kind/ label

Available kind/ labels are:

kind/bug: Categorizes issue or PR as related to a bug.
kind/flake: Categorizes issue or PR as related to a flakey test
kind/cleanup: Categorizes issue or PR as related to cleaning up code, process, or technical debt.
kind/design: Categorizes issue or PR as related to design.
kind/documentation: Categorizes issue or PR as related to documentation.
kind/feature: Categorizes issue or PR as related to a new feature.
kind/misc: Categorizes issue or PR as a miscellaneuous one.

tekton-robot · 2020-05-29T21:35:27Z

This PR cannot be merged: expecting exactly one kind/ label

Available kind/ labels are:

kind/bug: Categorizes issue or PR as related to a bug.
kind/flake: Categorizes issue or PR as related to a flakey test
kind/cleanup: Categorizes issue or PR as related to cleaning up code, process, or technical debt.
kind/design: Categorizes issue or PR as related to design.
kind/documentation: Categorizes issue or PR as related to documentation.
kind/feature: Categorizes issue or PR as related to a new feature.
kind/misc: Categorizes issue or PR as a miscellaneuous one.

tekton-robot · 2020-05-29T21:36: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/reconciler/pipelinerun/pipelinerun.go	75.3%	75.3%	-0.0

tekton-robot · 2020-05-29T21:41:43Z

This PR cannot be merged: expecting exactly one kind/ label

Available kind/ labels are:

kind/bug: Categorizes issue or PR as related to a bug.
kind/flake: Categorizes issue or PR as related to a flakey test
kind/cleanup: Categorizes issue or PR as related to cleaning up code, process, or technical debt.
kind/design: Categorizes issue or PR as related to design.
kind/documentation: Categorizes issue or PR as related to documentation.
kind/feature: Categorizes issue or PR as related to a new feature.
kind/misc: Categorizes issue or PR as a miscellaneuous one.

tekton-robot · 2020-05-29T21:42:38Z

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/reconciler/pipelinerun/pipelinerun.go	75.3%	75.3%	-0.0

tekton-robot · 2020-05-29T21:57:20Z

This PR cannot be merged: expecting exactly one kind/ label

Available kind/ labels are:

kind/bug: Categorizes issue or PR as related to a bug.
kind/flake: Categorizes issue or PR as related to a flakey test
kind/cleanup: Categorizes issue or PR as related to cleaning up code, process, or technical debt.
kind/design: Categorizes issue or PR as related to design.
kind/documentation: Categorizes issue or PR as related to documentation.
kind/feature: Categorizes issue or PR as related to a new feature.
kind/misc: Categorizes issue or PR as a miscellaneuous one.

tekton-robot · 2020-05-29T21:58: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/reconciler/pipelinerun/pipelinerun.go	75.3%	75.3%	-0.0

tekton-robot · 2020-05-29T22:08:33Z

This PR cannot be merged: expecting exactly one kind/ label

Available kind/ labels are:

kind/bug: Categorizes issue or PR as related to a bug.
kind/flake: Categorizes issue or PR as related to a flakey test
kind/cleanup: Categorizes issue or PR as related to cleaning up code, process, or technical debt.
kind/design: Categorizes issue or PR as related to design.
kind/documentation: Categorizes issue or PR as related to documentation.
kind/feature: Categorizes issue or PR as related to a new feature.
kind/misc: Categorizes issue or PR as a miscellaneuous one.

tekton-robot · 2020-05-29T22:10: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/reconciler/pipelinerun/pipelinerun.go	75.3%	75.5%	0.2

tekton-robot · 2020-05-29T22:11:52Z

This PR cannot be merged: expecting exactly one kind/ label

Available kind/ labels are:

kind/bug: Categorizes issue or PR as related to a bug.
kind/flake: Categorizes issue or PR as related to a flakey test
kind/cleanup: Categorizes issue or PR as related to cleaning up code, process, or technical debt.
kind/design: Categorizes issue or PR as related to design.
kind/documentation: Categorizes issue or PR as related to documentation.
kind/feature: Categorizes issue or PR as related to a new feature.
kind/misc: Categorizes issue or PR as a miscellaneuous one.

tekton-robot · 2020-05-29T22:13:32Z

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/reconciler/pipelinerun/pipelinerun.go	75.3%	75.5%	0.2

tekton-robot · 2020-05-29T22:14:17Z

This PR cannot be merged: expecting exactly one kind/ label

Available kind/ labels are:

kind/bug: Categorizes issue or PR as related to a bug.
kind/flake: Categorizes issue or PR as related to a flakey test
kind/cleanup: Categorizes issue or PR as related to cleaning up code, process, or technical debt.
kind/design: Categorizes issue or PR as related to design.
kind/documentation: Categorizes issue or PR as related to documentation.
kind/feature: Categorizes issue or PR as related to a new feature.
kind/misc: Categorizes issue or PR as a miscellaneuous one.

tekton-robot · 2020-05-29T22:16:29Z

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/reconciler/pipelinerun/pipelinerun.go	75.3%	75.5%	0.2

tekton-robot · 2020-05-31T15:56:08Z

This PR cannot be merged: expecting exactly one kind/ label

Available kind/ labels are:

kind/bug: Categorizes issue or PR as related to a bug.
kind/flake: Categorizes issue or PR as related to a flakey test
kind/cleanup: Categorizes issue or PR as related to cleaning up code, process, or technical debt.
kind/design: Categorizes issue or PR as related to design.
kind/documentation: Categorizes issue or PR as related to documentation.
kind/feature: Categorizes issue or PR as related to a new feature.
kind/misc: Categorizes issue or PR as a miscellaneuous one.

tekton-robot · 2020-05-31T15:58:35Z

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/reconciler/pipelinerun/pipelinerun.go	75.3%	75.5%	0.2

tekton-robot · 2020-06-01T13:09:40Z

This PR cannot be merged: expecting exactly one kind/ label

Available kind/ labels are:

kind/bug: Categorizes issue or PR as related to a bug.
kind/flake: Categorizes issue or PR as related to a flakey test
kind/cleanup: Categorizes issue or PR as related to cleaning up code, process, or technical debt.
kind/design: Categorizes issue or PR as related to design.
kind/documentation: Categorizes issue or PR as related to documentation.
kind/feature: Categorizes issue or PR as related to a new feature.
kind/misc: Categorizes issue or PR as a miscellaneuous one.

tekton-robot · 2020-06-01T13:12:00Z

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/reconciler/pipelinerun/pipelinerun.go	75.3%	75.5%	0.2

tekton-robot · 2020-06-09T16:27:46Z

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/reconciler/pipelinerun/pipelinerun.go	81.7%	81.9%	0.2

tekton-robot · 2020-06-09T16:44: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/reconciler/pipelinerun/pipelinerun.go	81.7%	81.9%	0.2

tekton-robot · 2020-06-09T16:47:46Z

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/reconciler/pipelinerun/pipelinerun.go	81.7%	81.9%	0.2

jerop · 2020-06-09T17:01:35Z

thanks for the review @vdemeester and @pritidesai!

made the changes and the test case with default params is here

ghost

Fantastic, this is going to make writing test infra much easier.

pritidesai

looks great, thanks @jerop for addressing all the comments 🙏

vdemeester

/lgtm

/hold
I just want a last look from @tektoncd/core-maintainers. This will force task to setup default value for parameters if the parameters can be an empty string.

# […]
spec:
  params:
  - name: extraOptions
    type: string
  steps:
  - name: foooooo
    image: bash
    script: |
      ls -l ${extraOptions}

Without specifying the parameter extraOptions, the above task would succeed before this change and "fail" after. We will need a pass on the catalog to add default field where needed.

tekton-robot · 2020-06-10T09:21:25Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pritidesai, sbwsg, 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:

~~OWNERS~~ [sbwsg,vdemeester]

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

pkg/reconciler/pipelinerun/pipelinerun.go

bobcatfish · 2020-06-15T14:53:40Z

@vdemeester if anyone was relying on this for Pipelines + PipelineRuns we definitely want to warn them, so I think it makes sense to put a warning in the release notes.

I think the catalog shouldn't need any updates b/c this PR is only updating Pipelines + PipelinesRuns, and it looks like Tasks + TaskRuns already have this validation, for example this Task:

apiVersion: tekton.dev/v1beta1
kind: Task
metadata:
  name: hello
spec:
  params:
  - name: extraOptions
    type: string
  steps:
    - name: hello
      image: ubuntu
      command:
        - echo
      args:
        - "Hello World $(params.extraOptions)"

With this TaskRun:

apiVersion: tekton.dev/v1beta1
kind: TaskRun
metadata:
  generateName: hello-
spec:
  taskRef:
    name: hello

The TaskRun fails with:

invalid input params: missing values for these params which have no default values: [extraOptions]

While working on allowing PipelineRuns to provide extra parameters (tektoncd#2513), we found that providing extra parameters was unintentionally allowed. That's because, currently, there's no validation that all parameters expected by the Pipeline is provided by the PipelineRun. In this PR, we add validation for PipelineRun parameters by generating a list of provided parameters then iterating through the required parameters to ensure they are in the list of provided parameters. Note that parameters which have default values specified in Pipeline are not required. In the validation, we still allow PipelineRuns to provide extra parameters. If we disallow PipelineRuns from provides extra parameters, the Pipelines would fail. As a result, systems that autogenerate PipelineRuns need to lookat each pipeline to see what parameters they need so it can provide only the required parameters. That means users would have to resort to more complex designs to solve this issue, as further described in (tektoncd#2513). By allowing PipelineRuns to provide extra parameters, we make the process of autogenerating of PipelineRuns simpler. Fixes tektoncd#2708.

bobcatfish · 2020-06-15T14:55:40Z

/lgtm

tekton-robot · 2020-06-15T14:57: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/reconciler/pipelinerun/pipelinerun.go	82.0%	82.1%	0.1

jerop · 2020-06-15T20:06:30Z

/retest

vdemeester · 2020-06-16T14:10:50Z

@vdemeester if anyone was relying on this for Pipelines + PipelineRuns we definitely want to warn them, so I think it makes sense to put a warning in the release notes.

I think the catalog shouldn't need any updates b/c this PR is only updating Pipelines + PipelinesRuns, and it looks like Tasks + TaskRuns already have this validation, for example this Task:
apiVersion: tekton.dev/v1beta1
kind: Task
metadata:
  name: hello
spec:
  params:
  - name: extraOptions
    type: string
  steps:
    - name: hello
      image: ubuntu
      command:
        - echo
      args:
        - "Hello World $(params.extraOptions)"
With this TaskRun:
apiVersion: tekton.dev/v1beta1
kind: TaskRun
metadata:
  generateName: hello-
spec:
  taskRef:
    name: hello
The TaskRun fails with:
invalid input params: missing values for these params which have no default values: [extraOptions]

Ahh good then ! It's a less scaring change 🙃
/hold cancel

tekton-robot requested review from bobcatfish and dibyom May 29, 2020 21:33

tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 29, 2020

jerop changed the title ~~Validate PipelineRun parameters~~ Validate PipelineRun Parameters May 29, 2020

jerop force-pushed the pipeline-validation branch from 0697022 to bcf1c72 Compare May 29, 2020 21:39

jerop force-pushed the pipeline-validation branch from bcf1c72 to 4e0550c Compare May 29, 2020 21:56

jerop force-pushed the pipeline-validation branch from 4e0550c to 8347846 Compare May 29, 2020 22:08

jerop force-pushed the pipeline-validation branch from 8347846 to 13759ed Compare May 29, 2020 22:11

jerop force-pushed the pipeline-validation branch from 13759ed to b0a3bac Compare May 29, 2020 22:13

jerop marked this pull request as draft May 31, 2020 15:42

jerop force-pushed the pipeline-validation branch from b0a3bac to 9fc4144 Compare May 31, 2020 15:55

jerop force-pushed the pipeline-validation branch from 9fc4144 to 264032d Compare June 1, 2020 13:09

jerop force-pushed the pipeline-validation branch from 264032d to 5b8b6d2 Compare June 1, 2020 14:42

tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Jun 9, 2020

jerop force-pushed the pipeline-validation branch from 25120c4 to 7eab836 Compare June 9, 2020 16:41

jerop force-pushed the pipeline-validation branch from 7eab836 to 582f848 Compare June 9, 2020 16:45

jerop requested review from vdemeester and pritidesai June 9, 2020 17:01

ghost approved these changes Jun 9, 2020

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 9, 2020

pritidesai approved these changes Jun 9, 2020

View reviewed changes

vdemeester approved these changes Jun 10, 2020

View reviewed changes

tekton-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Jun 10, 2020

bobcatfish reviewed Jun 15, 2020

View reviewed changes

pkg/reconciler/pipelinerun/pipelinerun.go Outdated Show resolved Hide resolved

jerop force-pushed the pipeline-validation branch from 582f848 to 47f39ac Compare June 15, 2020 14:54

tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Jun 15, 2020

tekton-robot assigned bobcatfish Jun 15, 2020

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

tekton-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 16, 2020

tekton-robot merged commit db66eca into tektoncd:master Jun 16, 2020

jerop deleted the pipeline-validation branch September 30, 2020 15:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate PipelineRun Parameters #2719

Validate PipelineRun Parameters #2719

jerop commented May 29, 2020 •

edited

Loading

tekton-robot commented May 29, 2020

tekton-robot commented May 29, 2020

tekton-robot commented May 29, 2020

tekton-robot commented May 29, 2020

tekton-robot commented May 29, 2020

tekton-robot commented May 29, 2020

tekton-robot commented May 29, 2020

tekton-robot commented May 29, 2020

tekton-robot commented May 29, 2020

tekton-robot commented May 29, 2020

tekton-robot commented May 29, 2020

tekton-robot commented May 29, 2020

tekton-robot commented May 29, 2020

tekton-robot commented May 31, 2020

tekton-robot commented May 31, 2020

tekton-robot commented Jun 1, 2020

tekton-robot commented Jun 1, 2020

tekton-robot commented Jun 9, 2020

tekton-robot commented Jun 9, 2020

tekton-robot commented Jun 9, 2020

jerop commented Jun 9, 2020

ghost left a comment

pritidesai left a comment

vdemeester left a comment •

edited

Loading

tekton-robot commented Jun 10, 2020

bobcatfish commented Jun 15, 2020

bobcatfish commented Jun 15, 2020

tekton-robot commented Jun 15, 2020

jerop commented Jun 15, 2020

vdemeester commented Jun 16, 2020

Validate PipelineRun Parameters #2719

Validate PipelineRun Parameters #2719

Conversation

jerop commented May 29, 2020 • edited Loading

Changes

Submitter Checklist

Reviewer Notes

Release Notes

tekton-robot commented May 29, 2020

tekton-robot commented May 29, 2020

tekton-robot commented May 29, 2020

tekton-robot commented May 29, 2020

tekton-robot commented May 29, 2020

tekton-robot commented May 29, 2020

tekton-robot commented May 29, 2020

tekton-robot commented May 29, 2020

tekton-robot commented May 29, 2020

tekton-robot commented May 29, 2020

tekton-robot commented May 29, 2020

tekton-robot commented May 29, 2020

tekton-robot commented May 29, 2020

tekton-robot commented May 31, 2020

tekton-robot commented May 31, 2020

tekton-robot commented Jun 1, 2020

tekton-robot commented Jun 1, 2020

tekton-robot commented Jun 9, 2020

tekton-robot commented Jun 9, 2020

tekton-robot commented Jun 9, 2020

jerop commented Jun 9, 2020

ghost left a comment

Choose a reason for hiding this comment

pritidesai left a comment

Choose a reason for hiding this comment

vdemeester left a comment • edited Loading

Choose a reason for hiding this comment

tekton-robot commented Jun 10, 2020

bobcatfish commented Jun 15, 2020

bobcatfish commented Jun 15, 2020

tekton-robot commented Jun 15, 2020

jerop commented Jun 15, 2020

vdemeester commented Jun 16, 2020

jerop commented May 29, 2020 •

edited

Loading

vdemeester left a comment •

edited

Loading