A Particular Param Syntax Makes Git Resolver Stop Working

[XinruZhang]

Expected Behavior

Tekton controller can successfully schedule the TaskRun when using the following Param syntax for Git Resolver

apiVersion: tekton.dev/v1beta1
kind: TaskRun
metadata:
  generateName: "tr-git-resolver-"
spec:
  workspaces:
    - name: output
      emptyDir: {}
  params:
    - name: url
      value: https://github.com/tektoncd/catalog.git
    - name: revision
      value: main
  taskRef:
    resolver: git
    params:
      - name: url
        value:
          type: string
          stringVal: https://github.com/tektoncd/catalog.git
      - name: revision
        value:
          type: string
          stringVal: main
      - name: pathInRepo
        value:
          type: string
          stringVal: task/git-clone/0.8/git-clone.yaml

Actual Behavior

It fails to schedule the TaskRun, error message:

message: 'failed to get task: error requesting remote resource: error getting
      "Git" "default/git-22641305a9f3dd8efe7d2f99769499f0": error opening file "":
      cannot open directory: /'

Steps to Reproduce the Problem

1. Submit the YAML file
2. Describe the created TaskRun.

Additional Info

Environment

enable-api-fields=alpha

• Kubernetes version:

  Output of kubectl version:

This version information is deprecated and will be replaced with the output from kubectl version --short.  Use --output=yaml|json to get the full version.
Client Version: version.Info{Major:"1", Minor:"24", GitVersion:"v1.24.2", GitCommit:"f66044f4361b9f1f96f0053dd46cb7dce5e990a8", GitTreeState:"clean", BuildDate:"2022-06-15T14:22:29Z", GoVersion:"go1.18.3", Compiler:"gc", Platform:"darwin/amd64"}
Kustomize Version: v4.5.4
Server Version: version.Info{Major:"1", Minor:"24", GitVersion:"v1.24.9-gke.3200", GitCommit:"92ea556d4e7418d0e7b5db1ee576a73f8fc47e91", GitTreeState:"clean", BuildDate:"2023-01-20T09:29:29Z", GoVersion:"go1.18.9b7", Compiler:"gc", Platform:"linux/amd64"}

• Tekton Pipeline version:

  Output of tkn version

Client version: dev
Pipeline version: v0.45.0

Error Logs in Tekton Controller Pod

{"severity":"error","timestamp":"2023-03-16T15:27:53.401Z","logger":"tekton-pipelines-controller","caller":"taskrun/taskrun.go:361","message":"Failed to determine Task spec to use for taskrun tr-git-resolver-1cfq2b: failed to get task: error requesting remote resource: error getting \"Git\" \"default/git-5f3b816bda119ef32529173400b6f525\": error opening file \"\": cannot open directory: /","commit":"d1b63d4","knative.dev/controller":"git.luolix.top.tektoncd.pipeline.pkg.reconciler.taskrun.Reconciler","knative.dev/kind":"tekton.dev.TaskRun","knative.dev/traceid":"e1f2c547-30e2-4b27-8fa2-3214ad8a11c4","knative.dev/key":"default/tr-git-resolver-1cfq2b","stacktrace":"github.com/tektoncd/pipeline/pkg/reconciler/taskrun.(*Reconciler).prepare\n\tgit.luolix.top/tektoncd/pipeline/pkg/reconciler/taskrun/taskrun.go:361\ngit.luolix.top/tektoncd/pipeline/pkg/reconciler/taskrun.(*Reconciler).ReconcileKind\n\tgit.luolix.top/tektoncd/pipeline/pkg/reconciler/taskrun/taskrun.go:181\ngit.luolix.top/tektoncd/pipeline/pkg/client/injection/reconciler/pipeline/v1beta1/taskrun.(*reconcilerImpl).Reconcile\n\tgit.luolix.top/tektoncd/pipeline/pkg/client/injection/reconciler/pipeline/v1beta1/taskrun/reconciler.go:235\nknative.dev/pkg/controller.(*Impl).processNextWorkItem\n\tknative.dev/pkg@v0.0.0-20221123011842-b78020c16606/controller/controller.go:542\nknative.dev/pkg/controller.(*Impl).RunContext.func3\n\tknative.dev/pkg@v0.0.0-20221123011842-b78020c16606/controller/controller.go:491"}

Valid Syntax

The following syntax works:

apiVersion: tekton.dev/v1beta1
kind: TaskRun
metadata:
  generateName: "tr-git-resolver-valid"
spec:
  workspaces:
    - name: output
      emptyDir: {}
  params:
    - name: url
      value: https://github.com/tektoncd/catalog.git
    - name: revision
      value: main
  taskRef:
    resolver: git
    params:
      - name: url
        value: https://github.com/tektoncd/catalog.git
      - name: revision
        value: main
      - name: pathInRepo
        value: task/git-clone/0.8/git-clone.yaml

[Yongxuanzhang]

We have discussed offline that this is not a supported syntax, shall we close this issue?

[XinruZhang]

SGTM

/close

[tekton-robot]

@XinruZhang: Closing this issue.

In response to this:

SGTM

/close

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.

[lbernick]

If this syntax isn't supported, we should deny pipelineruns and taskruns that use it. This error message is confusing-- it doesn't tell users what they can do to fix the problem. (I just ran into the same issue using this syntax in pipeline tasks.)

[Yongxuanzhang]

If this syntax isn't supported, we should deny pipelineruns and taskruns that use it. This error message is confusing-- it doesn't tell users what they can do to fix the problem. (I just ran into the same issue using this syntax in pipeline tasks.)

I think we already have docs&examples about how to use params correctly.
I wonder why did you run into this issue? Did you refer to any docs/examples? If so maybe we should fix them first.

The logic is in this function:

pipeline/pkg/apis/pipeline/v1beta1/param_types.go

Line 385 in 8541ed1

func (paramValues *ParamValue) UnmarshalJSON(value []byte) error {

We overwrite the UnmarshalJSON to support string, array and object types for the same value. What you're writing here:

      - name: url
        value:
          type: string
          stringVal: https://github.com/tektoncd/catalog.git

is the object syntax. The object has 2 keys "type" and "stringVal". I think it may be hard to "tell users what to do" from code

[lbernick]

I think we already have docs&examples about how to use params correctly. I wonder why did you run into this issue? Did you refer to any docs/examples? If so maybe we should fix them first.

I was trying to reproduce an error encountered by an internal user. However, it looks like our API spec lists this syntax as "required" to support in task defaults which seems inconsistent with saying this syntax is unsupported? (I found this via rg stringVal -g '*.md'.)

What you're writing here:

      - name: url
        value:
          type: string
          stringVal: https://github.com/tektoncd/catalog.git

is the object syntax. The object has 2 keys "type" and "stringVal". I think it may be hard to "tell users what to do" from code

🤯 This is super confusing

The specific error I got was param types don't match the user-specified type: [url revision] (from here), which was confusing because I thought I had specified string params and the task required string params. At the very least, we should add more detail to this error message.

Re: the git resolver, I'm not sure where this went wrong, but it seems like if it receives an object param called "pathInRepo" instead of a string param called "pathInRepo" it treats the "pathInRepo" as "/" which seems very user unfriendly

[Yongxuanzhang]

Oh I see. But why referring to the api-spec doc rather than https://github.com/tektoncd/pipeline/blob/main/docs/tasks.md#specifying-parameters?

The error is same, because you're passing an "object" to the "value", the "type" will be inferred as "object" and hence fail validation.

[Yongxuanzhang]

But I agree we could improve the error messages

[lbernick]

Oh I see. But why referring to the api-spec doc rather than https://github.com/tektoncd/pipeline/blob/main/docs/tasks.md#specifying-parameters?

I didn't refer to any docs to generate this example (it was reproducing an issue reported internally), but it's super important that our API spec reflects the syntax we want vendors to be required to support.

But I agree we could improve the error messages

I think there are four AIs:

• Fix/clarify API spec docs
• Clarify regular docs: it might be worth it to create a "parameters" doc that explains the different syntax for defining params, including defaults, and providing values for the params, so you can see all the syntax in one place, and link to this doc from pipeline(run)s.md and task(run)s.md
• Fix the git resolver (not sure exactly what happened here)
• Improve the error messages in validate_taskrun.go

[Yongxuanzhang]

Oh I see. But why referring to the api-spec doc rather than https://github.com/tektoncd/pipeline/blob/main/docs/tasks.md#specifying-parameters?

I didn't refer to any docs to generate this example (it was reproducing an issue reported internally), but it's super important that our API spec reflects the syntax we want vendors to be required to support.

But I agree we could improve the error messages

I think there are four AIs:

• Fix/clarify API spec docs
• Clarify regular docs: it might be worth it to create a "parameters" doc that explains the different syntax for defining params, including defaults, and providing values for the params, so you can see all the syntax in one place, and link to this doc from pipeline(run)s.md and task(run)s.md
• Fix the git resolver (not sure exactly what happened here)
• Improve the error messages in validate_taskrun.go

1. I think this may be a design issue from params. We define those fields but we don't allow users to use them in yamls, but vendors need to support those fields, that's the reason we listed them in api-doc?
2. That makes sense!
3. Maybe need to improve the git resolve error message,
4. Makes sense, need to consider all other places as well

[dibyom]

The API spec showing stringVal and arrayVal as required is confusing. It seems to be leaking our implementation details around how we implement string, array, and object params instead of the actual api field value. IMO, the API spec is incorrect (due to us using a custom unmarshaller), and should reflect the actual end user API i.e. we should document the type and value fields instead of objectVal, stringVal etc.

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

[afrittoli]

/remove-lifecycle stale this issue has not been solved as far as I know.
#6445 helped a lot with docs, but the OpenAPI bit is still broken.

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