Custom task without api version return validation error #6505

Yongxuanzhang · 2023-04-06T21:31:12Z

Changes

This commit closes #6459. For a customtask reference in a pipelinetask, if the Kind is non-empty and not "Task" or "ClusterTask", then it should be a Custom task and api version should be set. If not then validation webhook should return error. TaskRun's taskRef validation will be handled separately in #6557

/kind bug

Signed-off-by: Yongxuan Zhang yongxuanzhang@google.com

Submitter Checklist

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

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
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

Custom task without api version returns validation error

tekton-robot · 2023-04-06T21:37: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/apis/pipeline/v1/pipeline_types.go	97.1%	97.1%	0.0
pkg/apis/pipeline/v1beta1/pipeline_types.go	95.9%	95.9%	0.1

tekton-robot · 2023-04-06T21:38:41Z

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_types.go	97.1%	97.1%	0.0
pkg/apis/pipeline/v1beta1/pipeline_types.go	95.9%	95.9%	0.1

lbernick

We'll need to modify the isCustomTask functions introduced in #6447.

Also (nit) there are some typos in the PR title/description where custom is spelled "custome", please fix.

pkg/apis/pipeline/v1/pipeline_types.go

tekton-robot · 2023-04-10T03:48:34Z

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_types.go	97.1%	97.2%	0.1
pkg/apis/pipeline/v1beta1/pipeline_types.go	95.9%	96.0%	0.1

tekton-robot · 2023-04-10T03:50:55Z

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_types.go	97.1%	97.2%	0.1
pkg/apis/pipeline/v1beta1/pipeline_types.go	95.9%	96.0%	0.1

lbernick · 2023-04-10T13:39:06Z

pkg/apis/pipeline/v1/pipeline_types.go

-
+	notCustomTaskKinds := map[TaskKind]bool{
+		"":                 true,
+		NamespacedTaskKind: true,


@jerop can you take a look here? I'm not sure if validation runs before or after defaulting. If the user sets "kind" = Task, didn't we decide that should be a custom task?

if user sets kind to task:

if apiversion is non-empty --> custom task

if apiversion is empty --> task (because this is what we default to, we add kind but leave apiversion as empty)

Do you or @Yongxuanzhang know whether defaulting runs before or after validation? (I couldn't find the answer here or here). If validation runs before defaulting, using kind = "Task" would be treated here as a Task when it's actually a custom task. But if defaulting runs first, using kind = "Task" is correctly treated here as a Task. My guess is the logic is correct since e2e tests are passing, but want to double check.

pkg/apis/pipeline/v1/pipeline_types.go

jerop · 2023-04-11T20:52:53Z

pkg/apis/pipeline/v1/pipeline_validation_test.go

@@ -123,6 +123,27 @@ func TestPipeline_Validate_Failure(t *testing.T) {
 			Message: `expected at least one, got none`,
 			Paths:   []string{"spec.description", "spec.params", "spec.resources", "spec.tasks", "spec.workspaces"},
 		},
+	}, {


@Yongxuanzhang please add test cases where kind is set to:

"Task"

ClusterTask"

""

Then add 3 more test cases that are exactly like above, but then APIVersion is also set

jerop

please update the docs and add release notes 🙏🏾

tekton-robot · 2023-04-19T03:26: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/apis/pipeline/v1/pipeline_validation.go	99.4%	99.4%	0.0
pkg/apis/pipeline/v1beta1/pipeline_validation.go	99.1%	99.1%	0.0

tekton-robot · 2023-04-19T03:27:17Z

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.4%	99.4%	0.0
pkg/apis/pipeline/v1beta1/pipeline_validation.go	99.1%	99.1%	0.0

pkg/apis/pipeline/v1/pipeline_validation.go

lbernick · 2023-04-19T13:49:56Z

pkg/apis/pipeline/v1/pipeline_validation.go

+	case pt.TaskRef != nil && !notCustomTaskKinds[pt.TaskRef.Kind]:
+		errs = errs.Also(pt.validateCustomTask())
 	case pt.TaskRef != nil && pt.TaskRef.APIVersion != "":


This might be more readable as:

case pt.TaskRef != nil: if pt.TaskRef.APIVersion != "" || !notCustomTaskKinds[pt.TaskRef.Kind] { pt.validateCustomTask() }

(Also could the double negative !notCustomTaskKinds be made into a positive?)

It's not so easy to merge these 2, there are some cases in this switch control, it seems not add readability with this

pkg/apis/pipeline/v1/pipeline_types.go

tekton-robot · 2023-04-19T17:15:45Z

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/apis/pipeline/v1beta1/pipeline_validation.go	99.1%	99.1%	0.0

tekton-robot · 2023-04-19T17:16: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.4%	99.4%	0.0
pkg/apis/pipeline/v1beta1/pipeline_validation.go	99.1%	99.1%	0.0

Yongxuanzhang · 2023-04-19T19:31:32Z

/retest

jerop · 2023-04-19T21:30:28Z

pkg/apis/pipeline/v1/pipeline_validation.go

+	taskKinds := map[TaskKind]bool{
+		"":                 true,
+		NamespacedTaskKind: true,
+		"ClusterTask":      true,


we don't have cluster tasks in v1

Suggested change

"ClusterTask": true,

I think this section of our docs is wrong: https://github.com/tektoncd/pipeline/blob/main/docs/tasks.md#task-vs-clustertask ("There is no v1 API specification for ClusterTask but a v1beta1 clustertask can still be referenced in a v1 pipeline.")

I think this might not have been correct: #6587

pkg/apis/pipeline/v1beta1/pipeline_validation.go

pkg/apis/pipeline/v1beta1/pipeline_validation_test.go

jerop · 2023-04-19T21:36:30Z

pkg/apis/pipeline/v1beta1/pipeline_validation_test.go

+		p: &Pipeline{
+			ObjectMeta: metav1.ObjectMeta{Name: "pipeline"},
+			Spec: PipelineSpec{
+				Tasks: []PipelineTask{{Name: "foo", TaskRef: &TaskRef{Kind: "Example", Name: ""}}},


what happens if there was apiversion but no kind?

It's tested in another test (pipelinetask validate), I will move these cases there.

jerop · 2023-04-19T21:36:41Z

pkg/apis/pipeline/v1beta1/pipeline_validation_test.go

+			Spec: PipelineSpec{
+				Tasks: []PipelineTask{{Name: "foo", TaskSpec: &EmbeddedTask{
+					TypeMeta: runtime.TypeMeta{
+						Kind: "Example",


what happens if there was apiversion but no kind?

jerop

please add docs and release notes

tekton-robot · 2023-04-20T15:24:05Z

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/apis/pipeline/v1beta1/pipeline_validation.go	99.1%	99.1%	0.0

tekton-robot · 2023-04-20T15:24:09Z

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.4%	99.4%	0.0
pkg/apis/pipeline/v1beta1/pipeline_validation.go	99.1%	99.1%	0.0

tekton-robot · 2023-04-20T15:47:27Z

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/apis/pipeline/v1beta1/pipeline_validation.go	99.1%	99.1%	0.0

tekton-robot · 2023-04-20T15:48: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.4%	99.4%	0.0
pkg/apis/pipeline/v1beta1/pipeline_validation.go	99.1%	99.1%	0.0

tekton-robot · 2023-04-20T20:35:34Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lbernick

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]

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

jerop

/lgtm

This commit closes tektoncd#6459. For a customtask reference in a pipelinetask, if the Kind is non-empty and not "Task" or "ClusterTask", then it should be a Custom task and api version should be set. If not then validation webhook should return error. TaskRun's taskRef validation will be handled separately in tektoncd#6557 Signed-off-by: Yongxuan Zhang yongxuanzhang@google.com

tekton-robot · 2023-04-25T14:40: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.4%	99.4%	0.0
pkg/apis/pipeline/v1beta1/pipeline_validation.go	99.1%	99.1%	0.0

tekton-robot · 2023-04-25T14:42:16Z

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.4%	99.4%	0.0
pkg/apis/pipeline/v1beta1/pipeline_validation.go	99.1%	99.1%	0.0

jerop

/lgtm

tekton-robot added kind/bug Categorizes issue or PR as related to a bug. release-note-none Denotes a PR that doesnt merit a release note. labels Apr 6, 2023

tekton-robot requested review from afrittoli and pritidesai April 6, 2023 21:31

tekton-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Apr 6, 2023

lbernick suggested changes Apr 7, 2023

View reviewed changes

pkg/apis/pipeline/v1/pipeline_types.go Outdated Show resolved Hide resolved

Yongxuanzhang force-pushed the fix-6459 branch from 74b3efa to b2a8c35 Compare April 10, 2023 03:43

Yongxuanzhang changed the title ~~Custome task without api version return validation error~~ Custom task without api version return validation error Apr 10, 2023

lbernick reviewed Apr 10, 2023

View reviewed changes

jerop reviewed Apr 11, 2023

View reviewed changes

jerop added this to the Pipelines v0.47 milestone Apr 11, 2023

jerop assigned jerop and lbernick Apr 12, 2023

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

lbernick modified the milestones: Pipelines v0.47, Pipelines v0.48 Apr 18, 2023

Yongxuanzhang mentioned this pull request Apr 19, 2023

Taskrun's taskref setting kind but without apiversion should be considered as validation error #6557

Open

Yongxuanzhang force-pushed the fix-6459 branch from b2a8c35 to 3d2f600 Compare April 19, 2023 03:20

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

lbernick reviewed Apr 19, 2023

View reviewed changes

Yongxuanzhang force-pushed the fix-6459 branch from 3d2f600 to fbd1da9 Compare April 19, 2023 17:08

jerop reviewed Apr 19, 2023

View reviewed changes

tekton-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesnt merit a release note. labels Apr 20, 2023

Yongxuanzhang force-pushed the fix-6459 branch from c1151b2 to d3fc30d Compare April 20, 2023 15:41

Yongxuanzhang requested review from jerop and lbernick April 20, 2023 15:59

lbernick approved these changes Apr 20, 2023

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 20, 2023

jerop reviewed Apr 25, 2023

View reviewed changes

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

Yongxuanzhang force-pushed the fix-6459 branch from d3fc30d to bd39031 Compare April 25, 2023 14:34

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

jerop reviewed Apr 25, 2023

View reviewed changes

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

tekton-robot merged commit 7d6cd81 into tektoncd:main Apr 25, 2023

This was referenced Apr 26, 2023

inconsistent ClusterTask references in V1 #6587

Closed

Allow references to ClusterTasks in v1 Pipeline Tasks #6588

Merged

jerop mentioned this pull request May 23, 2023

clarify in docs to not use apiVersion for taskRef for non-customtask #6704

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Custom task without api version return validation error #6505

Custom task without api version return validation error #6505

Yongxuanzhang commented Apr 6, 2023 •

edited

Loading

tekton-robot commented Apr 6, 2023

tekton-robot commented Apr 6, 2023

lbernick left a comment

tekton-robot commented Apr 10, 2023

tekton-robot commented Apr 10, 2023

lbernick Apr 10, 2023

jerop Apr 11, 2023

lbernick Apr 20, 2023

jerop Apr 11, 2023

jerop left a comment •

edited

Loading

tekton-robot commented Apr 19, 2023

tekton-robot commented Apr 19, 2023

lbernick Apr 19, 2023

Yongxuanzhang Apr 19, 2023

tekton-robot commented Apr 19, 2023

tekton-robot commented Apr 19, 2023

Yongxuanzhang commented Apr 19, 2023

jerop Apr 19, 2023

lbernick Apr 20, 2023

lbernick Apr 26, 2023

jerop Apr 19, 2023

Yongxuanzhang Apr 20, 2023 •

edited

Loading

jerop Apr 19, 2023

jerop left a comment

tekton-robot commented Apr 20, 2023

tekton-robot commented Apr 20, 2023

tekton-robot commented Apr 20, 2023

tekton-robot commented Apr 20, 2023

tekton-robot commented Apr 20, 2023

jerop left a comment

tekton-robot commented Apr 25, 2023

tekton-robot commented Apr 25, 2023

jerop left a comment

Custom task without api version return validation error #6505

Custom task without api version return validation error #6505

Conversation

Yongxuanzhang commented Apr 6, 2023 • edited Loading

Changes

Submitter Checklist

Release Notes

tekton-robot commented Apr 6, 2023

tekton-robot commented Apr 6, 2023

lbernick left a comment

Choose a reason for hiding this comment

tekton-robot commented Apr 10, 2023

tekton-robot commented Apr 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerop left a comment • edited Loading

Choose a reason for hiding this comment

tekton-robot commented Apr 19, 2023

tekton-robot commented Apr 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tekton-robot commented Apr 19, 2023

tekton-robot commented Apr 19, 2023

Yongxuanzhang commented Apr 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yongxuanzhang Apr 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerop left a comment

Choose a reason for hiding this comment

tekton-robot commented Apr 20, 2023

tekton-robot commented Apr 20, 2023

tekton-robot commented Apr 20, 2023

tekton-robot commented Apr 20, 2023

tekton-robot commented Apr 20, 2023

jerop left a comment

Choose a reason for hiding this comment

tekton-robot commented Apr 25, 2023

tekton-robot commented Apr 25, 2023

jerop left a comment

Choose a reason for hiding this comment

Yongxuanzhang commented Apr 6, 2023 •

edited

Loading

jerop left a comment •

edited

Loading

Yongxuanzhang Apr 20, 2023 •

edited

Loading