TEP-0023: Implicit Parameter and Resource Mapping in the Pipeline specification

[Peaorl]

TEP-0023 introduces implicit parameter and resource mapping in the Pipeline specification.
As per requests #3050 and #1484.

Please provided feedback on the proposal.

The algorithms in the Design section may need some tweaking as I implement it in the code.

Also see the [DISCUSSION] section.
I think it would be nice to support omitting the from parameter for PipelineTask input resources too.

[tekton-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign sthaha
You can assign the PR to them by writing /assign @sthaha in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• teps/OWNERS

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

[Peaorl]

/cc @afrittoli

[ghost]

nice visualization of the change.

[ghost]

Maybe the User Story could be:

As a Pipeline author I want the parameters and resources in a Pipeline Spec to be automatically handed to Tasks that share the parameter or resource names so that my Pipelines are easier to consume and it becomes harder to make mistakes editing the YAML in future.

[ghost]

To add even more possible features... we could implicitly map named workspaces in a Pipeline to named workspaces in Tasks:

# PipelineSpec
spec:
  workspaces:
  - name: source
  - name: output
  tasks:
  - taskRef: name: lint
    workspaces:
    - name: source
      workspace: source
  - taskRef: name: unit-test
    workspaces:
    - name: source
      workspace: source
  - taskRef: name: build
    workspaces:
    - name: source
      workspace: source
    - name: output
      workspace: output
  # below just shows one without a matching workspace name for reference
  - taskRef: name: integration-test
    workspaces:
    - name: binary-to-test
      workspace: output

Would become...

# PipelineSpec
spec:
  workspaces:
  - name: source
  - name: output
  tasks:
  - taskRef:
      name: lint
  - taskRef:
      name: unit-test
  - taskRef:
      name: build
  - taskRef:
      name: integration-test
    workspaces:
    - name: binary-to-test
      workspace: output

[vdemeester]

Few questions:

• Any reason for supporting PipelineResources in this proposal ? We aren't decide yet what we should do with them, skipping them initially would reduce the scope of work, especially if this works ends up being removed (if we remove PipelineResources as we know them now).
• Is it a on or off feature ? aka can I pass some parameters using the params: field and except the rest to be passed implicitly ?
• How do we handle same argument "name" with different types ?
• How will apply handle this ? Let's assume I have an "implicit" pipeline. I create it and it get mutated to be explicit (so more parameters, …). I change some pipeline parameter and re-apply it. apply will merge the two (be default), and thus will append PipelineTask parameter to the one that got added by mutation — and will likely fail because some parameters are not given (if the do not have a default value).
• Would this be enable through a feature-flag ?

I am all for ease the pain of users when writing pipeline and tasks. But I wonder where should this be tackled still — I don't have strong opinion on this though. The pipeline type definitions have been very explicit by design, and I wonder if it's a good or bad thing if we change that.
As of today, the proposed behaviour could be done by an external component – an external mutating webhook, a tool (generator, cli, ui, …), a higher level construct (https://github.com/shipwright-io/ at some level, …).
I am not against exploring this though, but I would rather wait to do this after the v1 API — and/or have this behaviour opt-in (aka the user have to flip a switch in the feature flag configmap).

[vdemeester]

Any consumer need to support this indeed, the one we "know" (tkn, dashboard, triggers, vscode tekton plugin, dashboard, openshift console, …) and the one we don't even know 🙃

[vdemeester]

Those are big caveats imo.

[vdemeester]

We would need more detail on this aliasing of parameters. Would this be another field in the API ? Or a field on the ParamSpec ? Is this expected that users would map the param they know are different ?

[Peaorl]

If we add this, it would make sense to add it in the Pipeline's spec.params field and thus altering the ParamSpec. Modifying the ParamSpec would mean that both Pipeline and Task authors can specify aliases for parameters.

To restrict this feature to Pipeline authors, who'd have to know the different parameter names, we would have to separate the ParamSpec type for Pipeline and Task types.
Alternatively, we could avoid separating the ParamSpec if the webhook rejects Tasks with alias fields.

[vdemeester]

It would be slower because it would need to resolve the task reference before being able to validate.
Resolving means querying the kubernetes API or (when tekton bundle is in) pull an OCI image (if we use bundle in referencing the task(s)).

I definitely do not expect those slowdown to be negligible, or at least, I only have my intuiton that it might or might not be nigligible. See here for potential "slowdown" when getting the task definition (using bundles).

[adambkaplan]

If extracting data for the task requires an OCI image to be pulled, it will require non-negligible amounts of time and will fail for reasons outside of Tekton's control.

[vdemeester]

This would most likely be a problem on the validation side (at least compared to the current behavior). Applying the same pipeline definition (kubectl apply -f mypipeline.yaml) will result in an error because both taskRef and taskSpec are specified.

• If we remove that validation, which one of taskSpec or taskRef has precedence ?
• Do we overidde the taskSpec with the content of the Task referenced ? But then, the object is not what the user sent, and will always be updated on apply.
• Do we take taskSpec and ignore taskRef ? But then, if the Task referenced changed, it will be ignored.

[vdemeester]

What does this mean ? What "duplication" are we talking about ? (in code, in execution, …)

Small quote, sometimes, duplication is better than the wrong abstraction 😛

[vdemeester]

I think drawbacks need more details.
In addition, the potential slowdown in the webhook should be considered as a drawback, imo.

[bobcatfish]

Spoke with @Peaorl today and this is on hold for now. Some feedback came up in API working group to address; closing this in the meantime!

[wlynch]

I'm curious about exploring this proposal again - I ended up coming to a similar idea this week.

I think we may want to scope this to just parameters for the time being - unlike PipelineResources and Workspaces, parameters have a useful property where unused definitions can't affect the behavior of the Task/Pipeline.

Instead of trying to match parameters, what if we overlay them and created a merged view (e.g. don't error if there isn't a 1:1 mapping - let extra params pass through)? As in this proposal, innermost parameter definitions take precedence if there are params with the same name (i.e. plumb through anything that is missing, else use the existing values/behavior). Only embedded resources should be able to use this - you shouldn't be able to create a standalone Task in the catalog / OCI registry that relies on implicit parameters, since that would be ambiguous.

Example

User Input:

apiVersion: tekton.dev/v1beta1
kind: TaskRun
metadata:
  generateName: hello-
spec:
  params:
    - name: message
      value: "hello world!"
    - name: a
      value: asdf
  taskSpec:
    # There are no explicit params defined here. They are derived from the TaskRun above.
    steps:
    - name: default
      image: ubuntu
      script: |
        echo $(params.message)

Post-Webhook Resolution (i.e. what the controller sees):

spec:
  params:
  - name: message
    value: hello world!
  - name: a
    value: asdf
  serviceAccountName: default
  taskSpec:
    params:
    - name: message
      type: string
    # It doesn't matter that there is an extra param here -
    # this won't be replaced / resolved to anything in the steps.
    - name: a
      type: string
    steps:
    - image: ubuntu
      name: default
      resources: {}
      script: |
        echo $(params.message)

To the pipeline controller this would still look like the explicitly defined, typed schema it has always been using, but users can take advantage the shortcut to help make authoring a bit more streamlined.

I think this would address some of the ambiguity concerns @vdemeester raised, and can be done in a way that does not change the behavior of the existing API and configs.