Handling parameter interpolation in fields not designed for it

[skaegi]

When authoring Tekton resources that use parameter interpolation we can get in trouble when either the value type is not a string or if there is additional validation like for #1514 . For now I'm just pointing out the problem but it would be good if we could somehow defer validation in Tasks and Pipelines until after parameter substitution occurred.

[skaegi]

Two approaches...

1. parse selected fields of the Task and Pipeline spec identified as supporting interpolation into interface{} or similar until after fields have been interpolated or perhaps not until they are translated into a runtime object.
2. introduce a new field to Task and Pipeline called patches that lets you target a path and provide a value that will be added at that location. (semantics of JSONPatch "add"). For example...

spec:
  params:
    - name: my-boolean-param
       default: true
  patches:
    - path: somePlace.thatNeeds.ABoolean
      value: $(params.my-boolean-param)
  somePlace:
    thatNeeds:
      ABoolean: false # if the value is $(params.my-boolean-param) we will fail because until interpolated it is a string

[skaegi]

I'm looking at -- https://github.com/kubernetes-sigs/kustomize/blob/master/docs/plugins/builtins.md#field-name-patchesStrategicMerge -- to see if it might be an applicable approach. One difference is that the patch would always be inlined (and parameterized) instead of in a separate file.

But the basic idea is to provide specOverlays that are applied to the spec. In that way only the specOverlay would need to be flexibly typed (or maybe just a long string?!)

Also see... https://github.com/kubernetes/kubernetes/blob/release-1.1/docs/devel/api-conventions.md#patch-operations ... as that's the intent to some degree ... e.g. to "patch" the Task/Pipeline

[skaegi]

So... to construct an example...

apiVersion: tekton.dev/v1alpha1
kind: Task
metadata:
  name: echo-hello-world
spec:
  steps:
    - name: echo
      image: ubuntu
      command:
        - echo
      args:
        - "hello world"

Could be written as...

apiVersion: tekton.dev/v1alpha1
kind: Task
metadata:
  name: echo-hello-world
spec:
  patchesStrategicMerge:
    spec:
      steps:
        - name: echo
           args:
             - "hello strategic merge world"
  steps:
    - name: echo
      image: ubuntu
      command:
        - echo

or with params as...

apiVersion: tekton.dev/v1alpha1
kind: Task
metadata:
  name: echo-hello-world
spec:
  params:
    - name: helloworld
      default: "hello strategic merge world"
  patchesStrategicMerge:
    spec:
      steps:
        - name: echo
           args:
             - $(params.helloworld)
  steps:
    - name: echo
      image: ubuntu
      command:
        - echo

---

Going back to the volume case in #1514 the Task could be re-written "safely" by appending a patchesStrategicMerge...

apiVersion: tekton.dev/v1alpha1
kind: Task
metadata:
  name: buildah
spec:
  inputs:
    params:
      - name: BUILDER_IMAGE
        type: string
        description: The location of the buildah builder image.
        default: quay.io/buildah/stable
      - name: DOCKERFILE
        type: string
        description: Name of the dockerfile relative to the context directory.
        default: Dockerfile
      - name: CONTEXT
        type: string
        description: Path to the dockerfile directory relative to the root of the repository.
        default: .
      - name: FORMAT
        type: string
        description: one of oci, v2s1, v2s2.
        default: v2s1
      - name: TLSVERIFY
        type: string
        description: Verify the TLS on the registry endpoint (for push/pull to a non-TLS registry)
        default: "true"
      - name: MEMORY
        type: string
        description: Size of /var/lib/containers in G units.
        default: 5G
    resources:
      - name: source
        type: git

  outputs:
    resources:
      - name: image
        type: image

  steps:
    - name: build
      image: $(inputs.params.BUILDER_IMAGE)
      workingDir: /workspace/source/$(inputs.params.CONTEXT)
      # See: https://github.com/containers/buildah/issues/1481#issuecomment-480629425
      command: ['buildah', 'bud', '--isolation', 'chroot', '--tls-verify=$(inputs.params.TLSVERIFY)', '-f', '$(inputs.params.DOCKERFILE)', '-t', '$(outputs.resources.image.url)', '.']
      volumeMounts:
        - name: varlibcontainers
          mountPath: /var/lib/containers

    - name: images
      image: $(inputs.params.BUILDER_IMAGE)
      workingDir: /workspace/source
      command: ['buildah', 'images', '--all', '--format', '{{.Digest}} {{.Size}} {{.CreatedAt}} {{.Name}}']
      volumeMounts:
        - name: varlibcontainers
          mountPath: /var/lib/containers

    - name: push
      image: $(inputs.params.BUILDER_IMAGE)
      workingDir: /workspace/source
      command: ['buildah', 'push', '--debug', '--format', '$(inputs.params.FORMAT)', '--tls-verify=$(inputs.params.TLSVERIFY)', '$(outputs.resources.image.url)', 'docker://$(outputs.resources.image.url)']
      volumeMounts:
        - name: varlibcontainers
          mountPath: /var/lib/containers
  patchesStrategicMerge:
    spec:
      volumes:
        - name: varlibcontainers
          emptyDir:
            sizeLimit: $(inputs.params.MEMORY)
            medium: Memory

[skaegi]

In #1393 we've been looking at adding support for types other than strings to params. We'll hit a similar problem if we put a value like $(inputs.params.someBoolean) in a field that expects a bool. For a concrete example of where that might be useful is in parameterizing security context for a particular Step. https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.16/#securitycontext-v1-core

[skaegi]

More thoughts on using a flexible parameterized type...

When we unmarshal Tasks and Pipeline we do so directly into the target types we need at "run" time. For example each Step in a Task is unmarshaled directly into a corev1.Container. I'd propose we instead unmarshal into a more flexible intermediary "template" or "parameterized" type that would let us support the above parameterization cases.

To offer the same validation and type guarantees, after unmarshaling into the intermediary type we would perform a Resolve against the Task/Pipeline "default" parameter values, checking that we can unmarshal the result into the target "run" type. For example, "step" json is unmarshaled into a ResolveableStep that validates that when "resolved" can be unmarshaled into a Step.

The TaskRun/PipelineRun "reconciler" logic would be much the same, but use the Resolve operation with their param over-rides to produce the target "resolved
types they need at runtime.

[vdemeester]

When we unmarshal Tasks and Pipeline we do so directly into the target types we need at "run" time. For example each Step in a Task is unmarshaled directly into a corev1.Container. I'd propose we instead unmarshal into a more flexible intermediary "template" or "parameterized" type that would let us support the above parameterization cases.

To offer the same validation and type guarantees, after unmarshaling into the intermediary type we would perform a Resolve against the Task/Pipeline "default" parameter values, checking that we can unmarshal the result into the target "run" type. For example, "step" json is unmarshaled into a ResolveableStep that validates that when "resolved" can be unmarshaled into a Step.

This is more or less what we did for the support of composefile in the docker cli (see here). In order to support interpolation almost everywhere and support type, we do unmarshall into a boosted map, do the interpolation, and re-unmarshall it into the real struct.

[skaegi]

Excellent thanks for the reference...

[dibyom]

I see this is part of Pipelines1.0/beta milestone. Is that still the case @skaegi ?

[vdemeester]

I think it should be, at least for 1.0 (so after the beta api but before calling tekton ga)

[tekton-robot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
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.

/lifecycle stale

Send feedback to tektoncd/plumbing.

[tekton-robot]

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

/close

Send feedback to tektoncd/plumbing.

[tekton-robot]

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

/lifecycle rotten

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.
Mark the issue as fresh with /remove-lifecycle rotten.

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

[vdemeester]

/remove-lifecycle rotten
/remove-lifecycle stale
/reopen

[tekton-robot]

@vdemeester: Reopened this issue.

In response to this:

/remove-lifecycle rotten
/remove-lifecycle stale
/reopen

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.

[michaelsauter]

Ping :)

Any way to move this forward? Or is this a won't-fix? I hope it is not as especially task resource constraints (also as noted in #4080) are really crucial to have reusable tasks.

[vdemeester]

/reopen
Indeed, we still need to fix that, and somehow I missed the pings.. Sorry 🙏🏼

[tekton-robot]

@vdemeester: Reopened this issue.

In response to this:

/reopen
Indeed, we still need to fix that, and somehow I missed the pings.. Sorry 🙏🏼

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.

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

[michaelsauter]

I think this issue is super important. Without it, tasks that build applications cannot be reused very well because the resource consumption (memory) cannot be known by the task author.

[vdemeester]

/remove-lifecycle stale
Note that on resource consumption there is a TEP around it : tektoncd/community#560

[lbernick]

/assign

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

[faust64]

I see tektoncd/community#560 was merged. Now I'm trying to make it work.
My use case involved overriding resource limits/requests ( #1548 ), and I came up with the following pipelinerun:

apiVersion: tekton.dev/v1beta1
kind: PipelineRun
spec:
  taskRunSpecs:
  - pipelineTaskName: build
    stepOverrides:
    - name: build
      resources:
        limits:
          cpu: 200m
          memory: 768Mi
        requests:
          cpu: 100m
          memory: 512Mi
  pipelineRef:
    name: docker-build
...

Trying to create this, I get the following error:

Error from server (BadRequest): error when creating "test":
admission webhook "validation.webhook.pipeline.tekton.dev" denied the request: validation failed:
must not set the field(s): spec.taskRunSpecs[0].stepOverrides"

Am I doing it wrong?
The docs I could find, regarding stepOverrides, mostly gives TaskRuns-based samples.
Then again, docs mention "PipelineTaskRunSpec may also contain StepOverrides" ( https://tekton.dev/docs/pipelines/pipelineruns/#specifying-taskrunspecs )

Checking other issues in this repository, I found the following #4653
Pointing to another PR, merged in March: #4598
I did upgrade Tekton Pipelines to 0.37.0 earlier today. Didn't help.
Going through release notes, I find a mention regarding overrides, related to taskruns, in 0.34.0.
What's the status for this? Anything left TODO, or should this be doable already? Or soon to be released?

[lbernick]

Hi @faust64, have you tried enabling alpha features?

I'll update the error message to be more clear that this is what is needed.

[faust64]

Indeed, I was missing this.
I can create my pipelinerun, after editing my feature-flags configmap.
resource requests/limits match those configured in my stepOverrides.
Thanks a lot @lbernick !

[lbernick]

Great, thanks! Error message is updated in #5045

[lbernick]

Our resource requirements api has been addressed in a few TEPs, and I think this issue doesn't have other clear use cases right now, so I'm going to close it. If there are specific fields of Tasks that do need parameterization but don't support it, we should open issues specifically for those.