Remove validation of parameter variables after substitution #4835

Aleromerog · 2022-05-04T18:30:51Z

Changes

This commit removes validation for fields containing a parameter variable
after the parameter value has already been substituted in TaskRun and PipelineRun.
This is done to allow parameter values to contain literal strings like $(params.a); for example, in #4690.

Removing validation of parameters variables after substitution for PipelineRun is out of the scope of
this PR, since it needs more discussion/analysis

Note: Passing $(params.x) are not longer being validated after substitution. However, in order to work as expected referring to #4690 you need to scape special characters to avoid problems with bash or zsh, like the following:

'\{\"key\": \"\$$params.a$\"\}'
or
"{'key': '$(params.a)'}"

to get the expected output:

{"key": "$(params.a)"}

Submitter Checklist

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

Docs included if any changes are user facing
Tests included if any functionality added or changed
Follows the commit message standard
Meets the Tekton contributor standards (including
functionality, content, code)
Release notes block below has been filled in
(if there are no user facing changes, use release note "NONE")

Release Notes

NONE

tekton-robot · 2022-05-04T18:33:11Z

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/taskrun/taskrun.go	79.9%	79.5%	-0.4

Aleromerog · 2022-05-04T18:34:13Z

/ok-to-test

tekton-robot · 2022-05-04T18:37: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/reconciler/taskrun/taskrun.go	79.9%	79.5%	-0.4

ywluogg · 2022-05-04T18:44:38Z

/assign @ywluogg

ywluogg

Can you add more tests to reflect the changes?

bendory · 2022-05-04T19:29:15Z

nit: please fix typo in title -- "declared", not "decalred" :-)

lbernick

thanks @Aleromerog!
A few comments:

This change should likely be made for PipelineRuns as well
Take a look at our commit message standards; there's a great example commit message there you can use. Here's a rewording I might suggest:

Title: Remove validation of parameter variables after substitution
Message: This commit removes validation for fields containing a parameter variable
after the parameter value has already been substituted.
This is done to allow parameter values to contain literal strings like $(params.foo); for example, in #4690. <etc>

pkg/reconciler/taskrun/taskrun.go

Yongxuanzhang · 2022-05-04T22:18:41Z

pkg/apis/pipeline/v1beta1/task_validation.go

@@ -79,7 +79,7 @@ func validateResults(ctx context.Context, results []TaskResult) (errs *apis.Fiel

 // a mount path which conflicts with any other declared workspaces, with the explicitly
 // declared volume mounts, or with the stepTemplate. The names must also be unique.
-func validateDeclaredWorkspaces(workspaces []WorkspaceDeclaration, steps []Step, stepTemplate *StepTemplate) (errs *apis.FieldError) {
+func ValidateDeclaredWorkspaces(workspaces []WorkspaceDeclaration, steps []Step, stepTemplate *StepTemplate) (errs *apis.FieldError) {


Exported func's comments should start with their name, see this and other functions for reference. Same with other functions.

If not you will fail the build check

tekton-robot · 2022-05-10T20:46: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/v1beta1/task_validation.go	97.0%	97.1%	0.1

tekton-robot · 2022-05-11T15:52:39Z

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/config/contexts.go	Do not exist	0.0%
pkg/apis/pipeline/v1beta1/task_validation.go	97.0%	97.1%	0.1

tekton-robot · 2022-05-11T16:29: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/config/contexts.go	Do not exist	0.0%
pkg/apis/pipeline/v1beta1/task_validation.go	97.0%	97.1%	0.1

lbernick

Thanks Alejandro this is looking good!

Using the example from the linked issue but with a Pipeline instead of a Task:

apiVersion: tekton.dev/v1beta1
kind: Pipeline
metadata:
  name: some-pipeline
  namespace: my-ns
spec:
  params:
  - name: input
  tasks:
  - name: foo
    taskSpec:
    params:
    - name: input
      value: $(params.input)
    steps:
    - name: print
      image: alpine
      script: |
        echo $(params.input)
---
apiVersion: tekton.dev/v1beta1
kind: PipelineRun
metadata:
  name: some-pipeline-run-2
  namespace: my-ns
spec:
  pipelineRef:
    name: some-pipeline
  serviceAccountName: tekton-triggers-admin
  params:
    - name: input
      value: '{"key": "$(params.a)"}'

we may run into a similar issue when validating the pipelinespec.

(not sure if this is the case or not but it's worth looking into and adding a test case for this or a note about this in your commit message/PR description)

lbernick · 2022-05-12T15:47:15Z

pkg/apis/pipeline/v1beta1/task_validation.go

@@ -48,6 +48,15 @@ func (ts *TaskSpec) Validate(ctx context.Context) (errs *apis.FieldError) {
 	if len(ts.Steps) == 0 {
 		errs = errs.Also(apis.ErrMissingField("steps"))
 	}
+
+	if config.IsSubstituted(ctx) {
+		// Validate the task's workspaces only.


What's the reason to validate only a Task's workspaces, and not include any of the other non-parameter-related validation steps? (genuinely asking, not suggesting this should necessarily be changed)

Also, I think in the linked issue, if the value passed for the parameter was '{"key": "$(workspaces.a)"}' instead of '{"key": "$(params.a)"}' (or similar) we'll run into a similar issue with workspace validation-- but maybe out of scope for this PR

I think that the initial reason that a second validation were added was from #3786. And it ended up calling to validate all again instead of just workspaces

pkg/apis/config/contexts.go

pkg/apis/pipeline/v1beta1/task_validation_test.go

Aleromerog · 2022-05-12T17:13:51Z

Thanks Alejandro this is looking good!

Using the example from the linked issue but with a Pipeline instead of a Task:
apiVersion: tekton.dev/v1beta1
kind: Pipeline
metadata:
  name: some-pipeline
  namespace: my-ns
spec:
  params:
  - name: input
  tasks:
  - name: foo
    taskSpec:
    params:
    - name: input
      value: $(params.input)
    steps:
    - name: print
      image: alpine
      script: |
        echo $(params.input)
---
apiVersion: tekton.dev/v1beta1
kind: PipelineRun
metadata:
  name: some-pipeline-run-2
  namespace: my-ns
spec:
  pipelineRef:
    name: some-pipeline
  serviceAccountName: tekton-triggers-admin
  params:
    - name: input
      value: '{"key": "$(params.a)"}'
we may run into a similar issue when validating the pipelinespec.

(not sure if this is the case or not but it's worth looking into and adding a test case for this or a note about this in your commit message/PR description)

we may run into a similar issue when validating the pipelinespec.

(not sure if this is the case or not but it's worth looking into and adding a test case for this or a note about this in your commit message/PR description)

Thanks for the feedback!

I changed Pipeline to have same behaviour (to validate only workspaces on the second validation) but apparently there were some test cases that were failing because they were waiting to validate the entire thing. Maybe we can create other issue and discuss further about adding it to pipelines as well if necessary.

tekton-robot · 2022-05-12T17:30:49Z

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/config/contexts.go	Do not exist	0.0%
pkg/apis/pipeline/v1beta1/task_validation.go	97.6%	97.7%	0.0

lbernick · 2022-05-12T17:59:36Z

I changed Pipeline to have same behaviour (to validate only workspaces on the second validation) but apparently there were some test cases that were failing because they were waiting to validate the entire thing. Maybe we can create other issue and discuss further about adding it to pipelines as well if necessary.

Sounds good; in this case, I would just write up your findings in a comment on #4690, and update the commit message + PR description to indicate that tackling this on PipelineRun is out of scope for this commit/PR

This commit removes validation for fields containing a parameter variable after the parameter value has already been substituted in TaskRun and PipelineRun. This is done to allow parameter values to contain literal strings like $(params.a); for example, in tektoncd#4690.

tekton-robot · 2022-05-12T20:12:28Z

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/config/contexts.go	Do not exist	0.0%
pkg/apis/pipeline/v1beta1/task_validation.go	97.6%	97.7%	0.0

vdemeester

/lgtm

tekton-robot · 2022-05-16T15:22:14Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lbernick, 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~~ [lbernick,vdemeester]

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

Aleromerog · 2022-05-16T17:39:48Z

/retest

tekton-robot added the release-note-none Denotes a PR that doesnt merit a release note. label May 4, 2022

tekton-robot requested review from bobcatfish and imjasonh May 4, 2022 18:30

tekton-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label May 4, 2022

tekton-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label May 4, 2022

tekton-robot assigned ywluogg May 4, 2022

ywluogg reviewed May 4, 2022

View reviewed changes

lbernick reviewed May 4, 2022

View reviewed changes

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

Aleromerog changed the title ~~Validate only decalred worskpaces and usages at TaskRun~~ Validate only declared workspaces and usages at TaskRun May 4, 2022

Yongxuanzhang reviewed May 4, 2022

View reviewed changes

vdemeester mentioned this pull request May 5, 2022

If I pass something that can be interpreted as a variable on a task input it will break the task with a non-existent variable error #4690

Closed

Aleromerog force-pushed the main branch from d3b6dbd to 0c4df57 Compare May 10, 2022 14:29

tekton-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 10, 2022

Aleromerog force-pushed the main branch from 0c4df57 to 765867c Compare May 10, 2022 14:41

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

Aleromerog force-pushed the main branch from 765867c to ccfcb9a Compare May 10, 2022 15:59

imjasonh removed their request for review May 10, 2022 16:10

Aleromerog force-pushed the main branch from ccfcb9a to 7134a77 Compare May 10, 2022 19:14

tekton-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 10, 2022

Aleromerog force-pushed the main branch from 7134a77 to 061d8e1 Compare May 11, 2022 15:50

tekton-robot removed the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 11, 2022

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

Aleromerog force-pushed the main branch from 061d8e1 to b9e9fd0 Compare May 11, 2022 16:23

Aleromerog changed the title ~~Validate only declared workspaces and usages at TaskRun~~ Remove validation of parameter variables after substitution May 12, 2022

lbernick reviewed May 12, 2022

View reviewed changes

Aleromerog force-pushed the main branch from b9e9fd0 to 54b911b Compare May 12, 2022 17:28

Aleromerog force-pushed the main branch from 54b911b to da8486a Compare May 12, 2022 20:08

lbernick approved these changes May 13, 2022

View reviewed changes

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

vdemeester approved these changes May 16, 2022

View reviewed changes

tekton-robot assigned vdemeester May 16, 2022

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

tekton-robot merged commit c938ee5 into tektoncd:main May 16, 2022

ywluogg mentioned this pull request Jun 22, 2022

Add ywluogg into Pipeline reviewer roles tektoncd/community#737

Merged

lbernick mentioned this pull request May 16, 2023

Cleanup: Remove "substituted context" task validation #6671

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove validation of parameter variables after substitution #4835

Remove validation of parameter variables after substitution #4835

Aleromerog commented May 4, 2022 •

edited

Loading

tekton-robot commented May 4, 2022

Aleromerog commented May 4, 2022

tekton-robot commented May 4, 2022

ywluogg commented May 4, 2022

ywluogg left a comment

bendory commented May 4, 2022

lbernick left a comment

Yongxuanzhang May 4, 2022

tekton-robot commented May 10, 2022

tekton-robot commented May 11, 2022

tekton-robot commented May 11, 2022

lbernick left a comment

lbernick May 12, 2022

lbernick May 12, 2022

Aleromerog May 12, 2022

Aleromerog commented May 12, 2022

tekton-robot commented May 12, 2022

lbernick commented May 12, 2022

tekton-robot commented May 12, 2022

vdemeester left a comment

tekton-robot commented May 16, 2022

Aleromerog commented May 16, 2022

Remove validation of parameter variables after substitution #4835

Remove validation of parameter variables after substitution #4835

Conversation

Aleromerog commented May 4, 2022 • edited Loading

Changes

Submitter Checklist

Release Notes

tekton-robot commented May 4, 2022

Aleromerog commented May 4, 2022

tekton-robot commented May 4, 2022

ywluogg commented May 4, 2022

ywluogg left a comment

Choose a reason for hiding this comment

bendory commented May 4, 2022

lbernick left a comment

Choose a reason for hiding this comment

Yongxuanzhang May 4, 2022

Choose a reason for hiding this comment

tekton-robot commented May 10, 2022

tekton-robot commented May 11, 2022

tekton-robot commented May 11, 2022

lbernick left a comment

Choose a reason for hiding this comment

lbernick May 12, 2022

Choose a reason for hiding this comment

lbernick May 12, 2022

Choose a reason for hiding this comment

Aleromerog May 12, 2022

Choose a reason for hiding this comment

Aleromerog commented May 12, 2022

tekton-robot commented May 12, 2022

lbernick commented May 12, 2022

tekton-robot commented May 12, 2022

vdemeester left a comment

Choose a reason for hiding this comment

tekton-robot commented May 16, 2022

Aleromerog commented May 16, 2022

Aleromerog commented May 4, 2022 •

edited

Loading