Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unable to use array parameter substitution in pipeline #5149

Closed
pietervincken opened this issue Jul 18, 2022 · 9 comments · Fixed by #5162
Closed

Unable to use array parameter substitution in pipeline #5149

pietervincken opened this issue Jul 18, 2022 · 9 comments · Fixed by #5162
Assignees
Labels
kind/bug Categorizes issue or PR as related to a bug. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now.

Comments

@pietervincken
Copy link

Expected Behavior

Array parameters can be used in order to pass lists to pipelines and tasks. Since v0.36.0 this behaviour seems broken and prevents the usage of array parameters in pipelines.

Actual Behavior

When using array parameter substitution the following error is thrown:

invalid input params for task task-name: param types don't match the user-specified type: [array-variable-name]

Steps to Reproduce the Problem

  1. Create a pipeline that uses an array parameter and passes it down as described here
  2. Create a task that uses parameter substitution as described here
  3. Perform a pipeline, with or without a value for the array, even with a default in the pipeline, it will fail.

Additional Info

  • Kubernetes version:

    Output of kubectl version:

Server Version: version.Info{Major:"1", Minor:"23", GitVersion:"v1.23.5", GitCommit:"5a97ee6d15525f6e4a1c2646bf1dfd2ebd5220b5", GitTreeState:"clean", BuildDate:"2022-06-15T04:26:33Z", GoVersion:"go1.17.8", Compiler:"gc", Platform:"linux/amd64"}
  • Tekton Pipeline version:

Pipeline version: v0.36.0 and v0.37.2 (confirmed on both)

Potentially related:

Pipeline example

apiVersion: tekton.dev/v1beta1
kind: Pipeline
metadata:
  name: test-pipeline
  labels:
    name: test-pipeline
spec:
  params:
    - name: additional-parameters
      type: array
      default: []

  tasks:
    - name: test-pipeline
      taskRef:
        name: test-pipelines
      params:
        - name: additional-parameters
          value: ["$(params.additional-parameters[*])"]

Pipeline run

apiVersion: tekton.dev/v1beta1
kind: PipelineRun
metadata:
  name: test-pipeline-run
spec:
  pipelineRef:
    name: test-pipeline
  params:
    - name: additional-parameters
      value:
        - "test"
@pietervincken pietervincken added the kind/bug Categorizes issue or PR as related to a bug. label Jul 18, 2022
@lbernick lbernick added the priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. label Jul 18, 2022
@Yongxuanzhang
Copy link
Member

/assign @Yongxuanzhang

@Yongxuanzhang
Copy link
Member

Yongxuanzhang commented Jul 18, 2022

Hi @pietervincken, thanks for reporting this issue!
Do you mind providing a full working example of this case?
This is the one I'm using from the example you provided and added the missing task. But it can use the array params in the task under the 2 versions. Please let me know if I miss anything!

kind: Task
apiVersion: tekton.dev/v1beta1
metadata:
  name: test-task
spec:
  params:
    - name: additional-parameters
      default: []
  steps:
    - name: echo
      image: bash:latest
      args: ["echo", "$(params.additional-parameters[*])"]
---
apiVersion: tekton.dev/v1beta1
kind: Pipeline
metadata:
  name: test-pipeline
  labels:
    name: test-pipeline
spec:
  params:
    - name: additional-parameters
      type: array
      default: []
  tasks:
    - name: test-task
      taskRef:
        name: test-task
      params:
        - name: additional-parameters
          value: ["$(params.additional-parameters[*])"]
---
apiVersion: tekton.dev/v1beta1
kind: PipelineRun
metadata:
  name: test-pipeline-run
spec:
  pipelineRef:
    name: test-pipeline
  params:
    - name: additional-parameters
      value:
        - "array1"
        - "array2"

@Yongxuanzhang
Copy link
Member

Maybe this can work for new created resources but not for existing resources?
Like if you have pipeline, pipelinerun, task existing in cluster before tekton v0.36, then update tekton will cause this error?

@pietervincken
Copy link
Author

pietervincken commented Jul 18, 2022

Hi @Yongxuanzhang

I used the exact same example as you did, but without the actual params in the pipelinerun. That results in the issue. If the additional-parameters isn't specified, it should default to []. I get the error invalid input params for task array-issue-task: param types don't match the user-specified type: [additional-parameters]

@pietervincken
Copy link
Author

pietervincken commented Jul 18, 2022

Using

apiVersion: tekton.dev/v1beta1
kind: PipelineRun
metadata:
  name: test-pipeline-run
spec:
  pipelineRef:
    name: test-pipeline
  params:
    - name: additional-parameters
      value: []

or

apiVersion: tekton.dev/v1beta1
kind: PipelineRun
metadata:
  name: test-pipeline-run
spec:
  pipelineRef:
    name: test-pipeline

triggers the behavior

@Yongxuanzhang
Copy link
Member

Using

apiVersion: tekton.dev/v1beta1
kind: PipelineRun
metadata:
  name: test-pipeline-run
spec:
  pipelineRef:
    name: test-pipeline
  params:
    - name: additional-parameters
      value: []

or

apiVersion: tekton.dev/v1beta1
kind: PipelineRun
metadata:
  name: test-pipeline-run
spec:
  pipelineRef:
    name: test-pipeline

triggers the behavior

Oh I see, thanks! Looks like something wrong with the default value type

@Yongxuanzhang
Copy link
Member

Yongxuanzhang commented Jul 18, 2022

This is related to #4786 and also the following PR since we changed the behaviour of UnmarshalJSON for ArrayOrString, previously it will by default set the type to array if the value is not a string.
This might be a problem since we support object now.

So when passing an empty array as param from pipelinerun -> pipeline -> task
The value to be unmarshalled will be [] -> null -> null

Before that commit the null will be treated as array type, but now we think it is string.

chuangw6 added a commit to chuangw6/pipeline that referenced this issue Jul 18, 2022
Prior to this change, if there is only one element in an array that is
a reference to an empty array, the original array becomes nil after
replacement, but it should be an empty array instead of nil.

Fixes tektoncd#5149
@chuangw6
Copy link
Member

chuangw6 commented Jul 18, 2022

Hi @pietervincken ,
Thanks for reporting the issue.
After digging into the issue with @Yongxuanzhang, we found that it was a bug in the original code.

The reason of this bug is that if an array contains a single element that is a reference to an empty array, the original array would become nil after repacement whereas it is expected to be an empty array. This will cause a series of downstream unexpected behaviour and cause the validation to fail.

With #5162 , your example should work now.

@chuangw6
Copy link
Member

/assign

tekton-robot pushed a commit that referenced this issue Jul 18, 2022
Prior to this change, if there is only one element in an array that is
a reference to an empty array, the original array becomes nil after
replacement, but it should be an empty array instead of nil.

Fixes #5149
vdemeester added a commit to vdemeester/tektoncd-pipeline that referenced this issue Aug 30, 2022
Prior to this change, if there is only one element in an array that is
a reference to an empty array, the original array becomes nil after
replacement, but it should be an empty array instead of nil.

Fixes tektoncd#5149

Signed-off-by: Vincent Demeester <[email protected]>
tekton-robot pushed a commit that referenced this issue Aug 30, 2022
Prior to this change, if there is only one element in an array that is
a reference to an empty array, the original array becomes nil after
replacement, but it should be an empty array instead of nil.

Fixes #5149

Signed-off-by: Vincent Demeester <[email protected]>
chengjingtao pushed a commit to katanomi/pipeline that referenced this issue Sep 2, 2022
Prior to this change, if there is only one element in an array that is
a reference to an empty array, the original array becomes nil after
replacement, but it should be an empty array instead of nil.

Fixes tektoncd#5149
chengjingtao pushed a commit to katanomi/pipeline that referenced this issue Sep 2, 2022
Prior to this change, if there is only one element in an array that is
a reference to an empty array, the original array becomes nil after
replacement, but it should be an empty array instead of nil.

Fixes tektoncd#5149

Signed-off-by: Vincent Demeester <[email protected]>
tekton-robot pushed a commit to tekton-robot/pipeline that referenced this issue Sep 6, 2022
Prior to this change, if there is only one element in an array that is
a reference to an empty array, the original array becomes nil after
replacement, but it should be an empty array instead of nil.

Fixes tektoncd#5149

Signed-off-by: Vincent Demeester <[email protected]>
tekton-robot pushed a commit that referenced this issue Sep 7, 2022
Prior to this change, if there is only one element in an array that is
a reference to an empty array, the original array becomes nil after
replacement, but it should be an empty array instead of nil.

Fixes #5149

Signed-off-by: Vincent Demeester <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants