Reject embedded `Task`s with `APIVersion` and/or `Kind` specified #5018

abayer · 2022-06-22T16:00:54Z

Changes

If custom tasks are enabled and either of PipelineTask.TaskSpec.APIVersion or PipelineTask.TaskSpec.Kind are specified, the PipelineTask will be treated as a custom task. This is still the case if an actual embedded Task is specified in PipelineTask.TaskSpec.TaskSpec, so we should be failing validation for those cases.

/kind bug

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

Fail PipelineTask validation if a normal, non-custom embedded task is specified along with `apiVersion` and/or `kind`

abayer · 2022-06-22T16:01:10Z

/assign @wlynch @vdemeester @lbernick @jerop

tekton-robot · 2022-06-22T16:02:40Z

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/pipeline_types.go	96.8%	96.9%	0.1

abayer · 2022-06-22T16:03:28Z

pkg/apis/pipeline/v1beta1/pipeline_types.go

+
+	// Reject cases where APIVersion and/or Kind are specified alongside an embedded Task.
+	// We determine if this is an embedded Task by checking of TaskSpec.TaskSpec.Steps has items.
+	if pt.TaskSpec != nil && len(pt.TaskSpec.TaskSpec.Steps) > 0 {


If anyone has a better way to determine whether the PipelineTask is, in fact, an embedded Task, I'd love to hear it. =)

We could at least extract this in a function on PipelineTask, so that if we change the way to determine it, it won't change that code (and would make that code more readable ? 🙃 )

We require today that kind is specified if apiVersion is, but we don't check the content of kind and apiVersion.

Should we determine if it's an embedded Task or an embedded custom task based on the content of kind and apiVersion? We could say that (tekton.dev/*, Task) means it's an embedded Task and should be validated accordingly, else it's an embedded custom task and we cannot validate further.

Once we implement PIP, we can add Pipeline to the list types that we validate.

@vdemeester Good call, doing so.

@afrittoli I thought about that, but decided that it was better to be consistent, especially since Pipeline won't be in pt.TaskSpec.TaskSpec as an inline type.

Also, the current behavior, with custom tasks enabled, with apiVersion/kind and TaskSpec.TaskSpec populated results in a Run getting created and never doing anything, so we're not breaking any existing use cases by being strict. And supporting Task in the TypeMeta would mean changes to reconciler logic to not treat those cases as custom tasks, which doesn't feel like it's worth doing.

vdemeester · 2022-06-23T08:31:43Z

pkg/apis/pipeline/v1beta1/pipeline_types.go

+
+	// Reject cases where APIVersion and/or Kind are specified alongside an embedded Task.
+	// We determine if this is an embedded Task by checking of TaskSpec.TaskSpec.Steps has items.
+	if pt.TaskSpec != nil && len(pt.TaskSpec.TaskSpec.Steps) > 0 {


We could at least extract this in a function on PipelineTask, so that if we change the way to determine it, it won't change that code (and would make that code more readable ? 🙃 )

tekton-robot · 2022-06-23T13:03:41Z

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/pipeline_types.go	96.8%	97.0%	0.1

abayer · 2022-06-23T14:35:25Z

/retest

lbernick

thanks @abayer! Could you document that the api version of a task embedded within a pipeline should match the api version of the pipeline?

lbernick · 2022-06-27T19:48:30Z

pkg/apis/pipeline/v1beta1/pipeline_types.go

+func (pt PipelineTask) validateEmbeddedOrType() (errs *apis.FieldError) {
+	// Reject cases where APIVersion and/or Kind are specified alongside an embedded Task.
+	// We determine if this is an embedded Task by checking of TaskSpec.TaskSpec.Steps has items.
+	if pt.TaskSpec != nil && len(pt.TaskSpec.TaskSpec.Steps) > 0 {


why is checking if there are steps necessary? can we just check if pt.TaskSpec.TaskSpec != nil?

pt.TaskSpec.TaskSpec is just an inline TaskSpec, not a pointer, so this was the best I could come up with.

abayer · 2022-06-27T21:26:06Z

@lbernick Not sure about what to document in that regard - what this change is doing is making specifying apiVersion and/or kind and a populated pt.TaskSpec.TaskSpec invalid. So assuming this goes in, you won't be able to specify a Task's apiVersion in a PipelineTask at all.

lbernick · 2022-06-28T12:58:18Z

Not sure about what to document in that regard - what this change is doing is making specifying apiVersion and/or kind and a populated pt.TaskSpec.TaskSpec invalid. So assuming this goes in, you won't be able to specify a Task's apiVersion in a PipelineTask at all.

Ah yeah I misspoke-- we could say that embedded Tasks within a Pipeline will create TaskRuns with the same API version as the pipeline

abayer · 2022-06-28T13:26:51Z

@lbernick 👍 - in practice, that's obviously just clarifying what's always been the case, but still worth having. =)

fixes tektoncd#4543 If custom tasks are enabled and either of `PipelineTask.TaskSpec.APIVersion` or `PipelineTask.TaskSpec.Kind` are specified, the `PipelineTask` will be treated as a custom task. This is still the case if an actual embedded `Task` is specified in `PipelineTask.TaskSpec.TaskSpec`, so we should be failing validation for those cases. Signed-off-by: Andrew Bayer <[email protected]>

tekton-robot · 2022-06-28T13:31:33Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

lbernick · 2022-06-28T13:33:24Z

/lgtm

tekton-robot · 2022-06-28T13:35: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/pipeline_types.go	96.8%	97.0%	0.1

tekton-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/bug Categorizes issue or PR as related to a bug. labels Jun 22, 2022

tekton-robot requested review from jerop and lbernick June 22, 2022 16:01

tekton-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jun 22, 2022

tekton-robot assigned jerop, lbernick, vdemeester and wlynch Jun 22, 2022

abayer commented Jun 22, 2022

View reviewed changes

vdemeester approved these changes Jun 23, 2022

View reviewed changes

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

abayer force-pushed the reject-kind-apiversion-in-embedded-taskspec branch from 695dee7 to c4e3812 Compare June 23, 2022 13:00

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

lbernick reviewed Jun 27, 2022

View reviewed changes

abayer force-pushed the reject-kind-apiversion-in-embedded-taskspec branch from c4e3812 to 71aa0c6 Compare June 28, 2022 13:31

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

tekton-robot merged commit 88ea861 into tektoncd:main Jun 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reject embedded `Task`s with `APIVersion` and/or `Kind` specified #5018

Reject embedded `Task`s with `APIVersion` and/or `Kind` specified #5018

abayer commented Jun 22, 2022

abayer commented Jun 22, 2022

tekton-robot commented Jun 22, 2022

abayer Jun 22, 2022

vdemeester Jun 23, 2022

afrittoli Jun 23, 2022

abayer Jun 23, 2022

abayer Jun 23, 2022 •

edited

Loading

vdemeester Jun 23, 2022

tekton-robot commented Jun 23, 2022

abayer commented Jun 23, 2022

lbernick left a comment

lbernick Jun 27, 2022

abayer Jun 27, 2022

abayer commented Jun 27, 2022

lbernick commented Jun 28, 2022

abayer commented Jun 28, 2022

tekton-robot commented Jun 28, 2022

lbernick commented Jun 28, 2022

tekton-robot commented Jun 28, 2022

Reject embedded Tasks with APIVersion and/or Kind specified #5018

Reject embedded Tasks with APIVersion and/or Kind specified #5018

Conversation

abayer commented Jun 22, 2022

Changes

Submitter Checklist

Release Notes

abayer commented Jun 22, 2022

tekton-robot commented Jun 22, 2022

abayer Jun 22, 2022

Choose a reason for hiding this comment

vdemeester Jun 23, 2022

Choose a reason for hiding this comment

afrittoli Jun 23, 2022

Choose a reason for hiding this comment

abayer Jun 23, 2022

Choose a reason for hiding this comment

abayer Jun 23, 2022 • edited Loading

Choose a reason for hiding this comment

vdemeester Jun 23, 2022

Choose a reason for hiding this comment

tekton-robot commented Jun 23, 2022

abayer commented Jun 23, 2022

lbernick left a comment

Choose a reason for hiding this comment

lbernick Jun 27, 2022

Choose a reason for hiding this comment

abayer Jun 27, 2022

Choose a reason for hiding this comment

abayer commented Jun 27, 2022

lbernick commented Jun 28, 2022

abayer commented Jun 28, 2022

tekton-robot commented Jun 28, 2022

lbernick commented Jun 28, 2022

tekton-robot commented Jun 28, 2022

Reject embedded `Task`s with `APIVersion` and/or `Kind` specified #5018

Reject embedded `Task`s with `APIVersion` and/or `Kind` specified #5018

abayer Jun 23, 2022 •

edited

Loading