Update PipelineSpec Task validation to prevents errors at runtime #1358

vtereso · 2019-09-26T16:15:23Z

Changes

Currently, if step names are omitted, a duplicate empty value collides in the Pipeline validation. Also, if a Task name uses forbidden characters, pods will fail to be created since it is appended to the name. If the name is omitted (in a single step, which would pass validation) we would get <PipelineRunName>--<Rand>-<ConditionalRefName>-<Rand> for a condition pod and <PipelineRunName>--<Rand> otherwise, where the double hyphen imply the name should be specified. Further, in the catalog and elsewhere it seems to be a well documented practice to include the name.

This PR adds validation to PipelineSpec Task name and TaskRef name validation to ensure that the name are valid Kubernetes names (revokes empty values).

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

Names of Task instances within Pipelines are now validated: they must not be empty, and they must only contain valid characters (https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names).

tekton-robot · 2019-09-26T16:18:37Z

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1alpha1/pipeline_validation.go	97.8%	96.8%	-1.0

vtereso · 2019-09-26T16:30:17Z

Not sure where within https://github.com/tektoncd/pipeline/blob/master/docs/pipelines.md to update or if it is necessary?

tekton-robot · 2019-09-26T16:33:06Z

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1alpha1/pipeline_validation.go	97.8%	97.8%	0.0

tekton-robot · 2019-09-26T16:35:06Z

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1alpha1/pipeline_validation.go	97.8%	97.8%	0.0

bobcatfish

Not sure where within https://github.com/tektoncd/pipeline/blob/master/docs/pipelines.md to update or if it is necessary?

Sounds worth mentioning to me, maybe at:
https://github.com/tektoncd/pipeline/blob/master/docs/pipelines.md#pipeline-tasks ?

Adding hold for the docs but otherwise looks good!

/hold

/lgtm
/approve

(I'm also gonna edit your release notes to include this change!)

bobcatfish · 2019-09-27T18:53:42Z

pkg/apis/pipeline/v1alpha1/pipeline_validation.go

@@ -23,6 +23,7 @@ import (
 	"github.com/tektoncd/pipeline/pkg/list"
 	"golang.org/x/xerrors"
 	"k8s.io/apimachinery/pkg/api/equality"
+	"k8s.io/apimachinery/pkg/util/validation"


ooo what a handy library!

bobcatfish · 2019-09-27T18:57:48Z

Maybe docs can link to https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names

tekton-robot · 2019-09-27T19:52:12Z

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1alpha1/pipeline_validation.go	97.8%	97.9%	0.1

vtereso · 2019-09-27T19:53:27Z

Just realized that the TaskRef names weren't being checked either so I just added this as well.

bobcatfish · 2019-09-27T20:13:04Z

Thanks @vtereso !!

/lgtm

vdemeester

/lgtm

afrittoli

Nice work, thank you!
I have a minor comment on the errors, let me know what you think about it.

pkg/apis/pipeline/v1alpha1/pipeline_validation.go

afrittoli · 2019-09-30T09:30:26Z

pkg/apis/pipeline/v1alpha1/pipeline_validation.go

+		}
+		// TaskRef name must be a valid k8s name
+		if errSlice := validation.IsQualifiedName(t.TaskRef.Name); len(errSlice) != 0 {
+			return apis.ErrInvalidValue(errSlice[0], "spec.tasks.taskRef")


vdemeester

/lgtm

I kinda agree with @afrittoli but I think we can do that in a follow-up. Sounds good @afrittoli @vtereso ?

vtereso · 2019-10-04T13:40:39Z

It was an easy change so I returned all the errors

tekton-robot · 2019-10-04T13:41:39Z

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1alpha1/pipeline_validation.go	97.8%	97.9%	0.1

tekton-robot · 2019-10-04T13:44:09Z

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1alpha1/pipeline_validation.go	97.8%	97.9%	0.1

afrittoli

/lgtm

afrittoli · 2019-10-04T14:06:03Z

/test pull-tekton-pipeline-integration-tests

afrittoli · 2019-10-04T14:07:41Z

/hold cancel

tekton-robot · 2019-10-04T14:52:08Z

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1alpha1/pipeline_validation.go	97.8%	97.9%	0.1

vdemeester

/lgtm

vdemeester · 2019-10-07T12:19:38Z

/lgtm cancel
@bobcatfish @vtereso needs cla/google to be fixed for this to be mergeable 😓

… errors at runtime

tekton-robot · 2019-10-07T13:25:15Z

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1alpha1/pipeline_validation.go	97.8%	97.9%	0.1

vtereso · 2019-10-07T13:38:10Z

The cla is 🆗

vdemeester

/lgtm

tekton-robot · 2019-10-08T07:46:42Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

googlebot added the cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit label Sep 26, 2019

tekton-robot requested review from dlorenc and imjasonh September 26, 2019 16:15

tekton-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Sep 26, 2019

vtereso force-pushed the TaskNameValidation branch from 8b3a7f1 to b4b93cf Compare September 26, 2019 16:29

tekton-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Sep 26, 2019

vtereso force-pushed the TaskNameValidation branch from b4b93cf to 12445d6 Compare September 26, 2019 16:32

bobcatfish reviewed Sep 27, 2019

View reviewed changes

tekton-robot assigned bobcatfish Sep 27, 2019

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. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Sep 27, 2019

vtereso force-pushed the TaskNameValidation branch from 12445d6 to 9eb296c Compare September 27, 2019 19:50

tekton-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 27, 2019

vtereso changed the title ~~Update PipelineSpec Task name validation to prevents errors at runtime~~ Update PipelineSpec Task validation to prevents errors at runtime Sep 27, 2019

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 27, 2019

vdemeester approved these changes Sep 30, 2019

View reviewed changes

tekton-robot assigned vdemeester Sep 30, 2019

afrittoli reviewed Sep 30, 2019

View reviewed changes

vdemeester approved these changes Oct 4, 2019

View reviewed changes

vtereso force-pushed the TaskNameValidation branch from 9eb296c to eb57389 Compare October 4, 2019 13:39

tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Oct 4, 2019

vtereso force-pushed the TaskNameValidation branch from eb57389 to 27c91b6 Compare October 4, 2019 13:41

afrittoli reviewed Oct 4, 2019

View reviewed changes

tekton-robot assigned afrittoli Oct 4, 2019

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 4, 2019

tekton-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 4, 2019

vtereso force-pushed the TaskNameValidation branch from 27c91b6 to efc93cd Compare October 4, 2019 14:49

tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Oct 4, 2019

vdemeester approved these changes Oct 7, 2019

View reviewed changes

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 7, 2019

tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Oct 7, 2019

Update PipelineSpec Task name and TaskRef name validation to prevents…

167dd54

… errors at runtime

vtereso force-pushed the TaskNameValidation branch from efc93cd to 167dd54 Compare October 7, 2019 13:23

vdemeester approved these changes Oct 8, 2019

View reviewed changes

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 8, 2019

tekton-robot merged commit c722343 into tektoncd:master Oct 8, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update PipelineSpec Task validation to prevents errors at runtime #1358

Update PipelineSpec Task validation to prevents errors at runtime #1358

vtereso commented Sep 26, 2019 •

edited

Loading

tekton-robot commented Sep 26, 2019

vtereso commented Sep 26, 2019

tekton-robot commented Sep 26, 2019

tekton-robot commented Sep 26, 2019

bobcatfish left a comment

bobcatfish Sep 27, 2019

bobcatfish commented Sep 27, 2019

tekton-robot commented Sep 27, 2019

vtereso commented Sep 27, 2019

bobcatfish commented Sep 27, 2019

vdemeester left a comment

afrittoli left a comment

afrittoli Sep 30, 2019

vdemeester left a comment

vtereso commented Oct 4, 2019

tekton-robot commented Oct 4, 2019

tekton-robot commented Oct 4, 2019

afrittoli left a comment

afrittoli commented Oct 4, 2019

afrittoli commented Oct 4, 2019

tekton-robot commented Oct 4, 2019

vdemeester left a comment

vdemeester commented Oct 7, 2019

tekton-robot commented Oct 7, 2019

vtereso commented Oct 7, 2019

vdemeester left a comment

tekton-robot commented Oct 8, 2019

Update PipelineSpec Task validation to prevents errors at runtime #1358

Update PipelineSpec Task validation to prevents errors at runtime #1358

Conversation

vtereso commented Sep 26, 2019 • edited Loading

Changes

Submitter Checklist

Reviewer Notes

Release Notes

tekton-robot commented Sep 26, 2019

vtereso commented Sep 26, 2019

tekton-robot commented Sep 26, 2019

tekton-robot commented Sep 26, 2019

bobcatfish left a comment

Choose a reason for hiding this comment

bobcatfish Sep 27, 2019

Choose a reason for hiding this comment

bobcatfish commented Sep 27, 2019

tekton-robot commented Sep 27, 2019

vtereso commented Sep 27, 2019

bobcatfish commented Sep 27, 2019

vdemeester left a comment

Choose a reason for hiding this comment

afrittoli left a comment

Choose a reason for hiding this comment

afrittoli Sep 30, 2019

Choose a reason for hiding this comment

vdemeester left a comment

Choose a reason for hiding this comment

vtereso commented Oct 4, 2019

tekton-robot commented Oct 4, 2019

tekton-robot commented Oct 4, 2019

afrittoli left a comment

Choose a reason for hiding this comment

afrittoli commented Oct 4, 2019

afrittoli commented Oct 4, 2019

tekton-robot commented Oct 4, 2019

vdemeester left a comment

Choose a reason for hiding this comment

vdemeester commented Oct 7, 2019

tekton-robot commented Oct 7, 2019

vtereso commented Oct 7, 2019

vdemeester left a comment

Choose a reason for hiding this comment

tekton-robot commented Oct 8, 2019

vtereso commented Sep 26, 2019 •

edited

Loading