Clearer Custom Task Reference Interface

[XinruZhang]

Feature Request

Request to have an additive dedicated field for referencing Custom Task, instead of using generally named fields apiVersion + kind + name under taskRef for it.

- name: sample-custom-task-new
  taskRef:
    customTask: "wait.testing.tekton.dev/v1beta1/Wait"
    name: "wait-task" # optional, depends on how custom tasks are implemented.

In this way, we create a new field customTask under taskRef for referencing Custom Task (just like we have a dedicated field resolver for Remote Resources).

- name: custom-task
 taskRef:
   customTask: "wait.testing.tekton.dev/v1beta1/Wait"
   name: "wait-task"

- taskRef:
   resolver: git
   params:
   - name: url
     value: https://github.com/tektoncd/catalog.git
   - name: revision
     value: main
   - name: pathInRepo
     value: task/git-clone/0.6/git-clone.yaml

- name: regular-task
 taskRef:
   name: "sample-task"

Use Case

Say I'm a new comer to Tekton, the first thing I knew about taskRef is it refers to a Task and will be scheduled as a TaskRun.

When I want to use the powerful Custom Task, I realized I need to use the following genaric syntax under taskRef to specify a CustomTask reference.

What's even more weird is, I'm not allowed to use taskRef.apiVersion when specifying a regular Task, which seems like an inconsistent API design.

# the current way of referencing a custom task
- name: custom-task 
  taskRef:
    apiVersion: "wait.testing.tekton.dev/v1beta1"
    kind: "Wait"
    name: "wait-task"
# illegal usage
- name: regular-task
  taskRef:
    apiVersion: "tekton.dev/v1beta1"  # illegal, PipelineRun controller ceates a CustomRun/Run for this taskRef instead of a TaskRun.
    kind: Task # optional
    name: "regular-task"

It would be clearer if we have a dedicated field to refer to CustomTasks under taskRef.

[vdemeester]

@XinruZhang I am not sure I completely agree with this statement. The apiVersion + kind + name is a very well known construct in Kubernetes types

[XinruZhang]

Thank you for the comment @vdemeester. Sorry for the confusion that I might have introduced here 🙃

My main point is it is confusing for a new Tekton user that using taskRef.apiVersion + taskRef.kind + taskRef.name to refer to a Custom Task. I updated the issue description, added the Use Case section to describe where the confusion is for a Tekton User, and why it is inconsistent, hope this helps with clarifying my idea.

Since I'm proposing a new field here, we can consider makeing this API field more generic (remove the implied Kubernetes-ism) as suggested in this conformance design principle to improve our API abstraction.

[XinruZhang]

@tektoncd/core-collaborators PTAL :)

[lbernick]

Even though apiVersion and kind are technically kubernetes-isms, I think the spirit of the design principle on conformance is to not introduce fields that are only able to run on k8s as opposed to other platforms. I'm not convinced that apiVersion and kind detract from conformance goals because it's extremely likely that custom task users would need to specify this info regardless of platform (I'm assuming that's why it's encoded in the "customTask" syntax in the example above).

Also, we can make this an additive change, but it introduces redundancy into the API. I'd imagine we'd consider deprecating the existing syntax at some point. I'm not sure there's a strong enough reason to do this.

Maybe to lessen confusion, we could allow the "illegal usage" example above? i.e.

apiVersion: tekton.dev/v1beta1
kind: TaskRun
spec:
- name: regular-task
  taskRef:
    apiVersion: "tekton.dev/v1"  # Create a v1 TaskRun instead of a v1beta1 TaskRun (the current behavior)
    kind: Task # optional, defaults to Task as it does currently
    name: "regular-task"

[XinruZhang]

Thank you for the comment @lbernick!

It's extremely likely that custom task users would need to specify this info regardless of platform

An interesting idea emerged when I discussed this with @vdemeester offline, APIVersion + Kind is not necessarily required when referring to a Custom Task, the two fields are often used for two purposes:

1. Filtering Events for CustomRun/Run:

• CEL Custom Task Controller
• TaskLoop Custom Task Controller
• PipelineLoop Custom Task Controller
• Pip Custom Task Controller

For the filtering purpose, we actually don't have to use APIVersion + Kind, for example using "pipelineLoop" to filter all the Kubernetes Event on v1alpha.Run has the same effect.

2. Get the target CRD resource back and make modifications:

• PipelineInPod Custom Task Controller

For this purpose, the info "APIVersion" and "Kind" is important for those custom task controllers who take care of those CRDs defined within CustomRun/Run. But they don't have to be specified by end users by using the pipeline.spec.tasks[].taskRef or run.spec.refor customRun.spec.customRef. Instead, each individual custom task controller can decide what CRD they want to manage within the controller, as long as end users specify which Custom Task Controller they want to use.

For example, a custom task user can specify:

taskRef:
  customTask: "PipelineLoop"
  name: "my-pipeline-loop"

And in the PipelineLoop controller, it can only reconcile resources where their

• apiVersion=="pipelineloop.dev/v1beta1"
• kind=="PipelineLoop"
• name=="taskRef.name"

I think the spirit of the design principle on conformance is to not introduce fields that are only able to run on k8s as opposed to other platforms

I partially agree with this statement, for the conformance policy, the functional capability of being platform-ignorant is indeed important, but the naming itself is also important.

[XinruZhang]

Moving the discussion with @pritidesai here in the issue: the main idea that @pritidesai brings is:

The pipeline author decided to choose the Tekton Task specifications as Custom Task. I do not think we should restrict or try to interpret the values specified in those fields. It's very well documented that those specifications (taskRef.apiVersion and taskRef.kind) creates customRun/Run.

A pipeline author could also decide to specify apiVersion: "tekton.dev/v1beta1" and kind: Pipeline. It's user's decision to specify any kind of resource as a customRun/Run.

[lbernick]

Chatted offline with @XinruZhang and we agreed that it's not necessarily the fact that "apiVersion" and "kind" are "kubernetes-isms" that makes them confusing-- what's actually confusing is that custom run controllers reconcile custom run objects, and whether they reconcile objects with the specified api version and kind is entirely up to the controller. We're overloading "kind" to mean both the custom run controller that should reconcile the object and the type of object that the custom run controller can optionally reconcile. Similarly, we're overloading "apiVersion" to mean both the version of the custom spec (where api group is not clearly meaningful) and the api group + version of the object that the controller can optionally reconcile.

It's also confusing that name is required for Tasks but optional for custom Tasks. If the custom task controller supports reconciling objects of the same name and api version, it's supposed to look up the object with that name and use its spec, and if the user doesn't specify a name, the custom task controller may support reconciling w/ an empty spec.

Imagine a custom run controller must continue to reconcile CustomRuns, but can do so however it wants to (i.e. it's not necessarily required to reconcile objects of the same api version and kind as it filters for). The following syntax would be clearer IMO. (I'm not proposing this syntax, I'm just trying to brainstorm what it would look like if we didn't overload apiversion/kind in this way.)

taskSpec:
  customRunController: "PipelineLoopFancyController"
  customSpecAPIVersion: "v1"
  customSpec:  # Anything in here is entirely up to the controller.
     pipelineLoopNameToFetchFromCluster: "my-pipeline-loop-crd"

This signifies that the custom run would be reconciled by the "PipelineLoopFancyController", using its v1 version of the custom spec. This specific custom run controller can either accept a custom spec inline, or it can look up whatever type of objects it reconciles. In this example, it would look up whatever type of object it reconciles, named "my-pipeline-loop-crd", and use that object's spec. (We probably wouldn't allow custom task references in taskref if we did this.)

I'm not sure what syntax I'd want for a reference to a custom task in an ideal world, but wanted to hear others' thoughts on this (maybe nobody else finds this confusing?)

[pritidesai]

I haven't gotten chance to review the entire thread here, pinging @Tomcli so that Tommy can chime in, thanks!

[Tomcli]

I think @XinruZhang's proposal on the customTask can work for us on the pipelineloop because the custom task just need to be aware where the custom apiVersion and kind are defined to filter all the customRuns/Runs events and to get the pipelineloop template when it's used as a taskRef. In this case we can get the ApiVersion and Kind from the customTask string by splitting the last slash and have some regular expression validation to make sure both ApiVersion and Kind are defined in the customTask field.

[tekton-robot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale with a justification.
Stale issues rot after an additional 30d of inactivity and eventually close.
If this issue is safe to close now please do so with /close with a justification.
If this issue should be exempted, mark the issue as frozen with /lifecycle frozen with a justification.

/lifecycle stale

Send feedback to tektoncd/plumbing.

[tekton-robot]

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten with a justification.
Rotten issues close after an additional 30d of inactivity.
If this issue is safe to close now please do so with /close with a justification.
If this issue should be exempted, mark the issue as frozen with /lifecycle frozen with a justification.

/lifecycle rotten

Send feedback to tektoncd/plumbing.

[tekton-robot]

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen with a justification.
Mark the issue as fresh with /remove-lifecycle rotten with a justification.
If this issue should be exempted, mark the issue as frozen with /lifecycle frozen with a justification.

/close

Send feedback to tektoncd/plumbing.

[tekton-robot]

@tekton-robot: Closing this issue.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen with a justification.
Mark the issue as fresh with /remove-lifecycle rotten with a justification.
If this issue should be exempted, mark the issue as frozen with /lifecycle frozen with a justification.

/close

Send feedback to tektoncd/plumbing.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.