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

If I pass something that can be interpreted as a variable on a task input it will break the task with a non-existent variable error #4690

Closed
viniagostini opened this issue Mar 18, 2022 · 11 comments
Assignees
Labels
kind/feature Categorizes issue or PR as related to a new feature. lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. priority/backlog Higher priority than priority/awaiting-more-evidence.

Comments

@viniagostini
Copy link

Expected Behavior

print the literal {"key": "$(params.a)"}

Actual Behavior

CouldntGetTask error
failed to create task run pod "some-task-run-2": non-existent variable in "echo {\"key\": \"$(params.a)\"}\n": steps[0].script. Maybe missing or invalid Task my-ns/some-task

Steps to Reproduce the Problem

apiVersion: tekton.dev/v1beta1
kind: Task
metadata:
  name: some-task
  namespace: my-ns
spec:
  params:
    - name: input
  steps:
    - name: print
      image: alpine
      script: |
        echo $(params.input)

---

apiVersion: tekton.dev/v1beta1
kind: TaskRun
metadata:
  name: some-task-run-2
  namespace: my-ns
spec:
  taskRef:
    name: some-task
  serviceAccountName: tekton-triggers-admin
  params:
    - name: input
      value: '{"key": "$(params.a)"}'

I ran into this problem trying to pass a TaskRun cloud event payload as a param to another Task. For now, I have a workaround using CEL on the EventListener to replace every $(params. for an empty string like so

- name: "overlays"
   value:
     - key: cePayload
        expression: body.marshalJSON().replace('$(params.','')

This works fine for me now but I see some unexpected behaviors happening in the future, so it would be nice if the task could escape or not try to interpret the values on the params.

Additional Info

  • Kubernetes version:

    Output of kubectl version:

Client Version: version.Info{Major:"1", Minor:"23", GitVersion:"v1.23.3", GitCommit:"816c97ab8cff8a1c72eccca1026f7820e93e0d25", GitTreeState:"clean", BuildDate:"2022-01-25T21:17:57Z", GoVersion:"go1.17.6", Compiler:"gc", Platform:"darwin/amd64"}
Server Version: version.Info{Major:"1", Minor:"21+", GitVersion:"v1.21.5-eks-bc4871b", GitCommit:"5236faf39f1b7a7dabea8df12726f25608131aa9", GitTreeState:"clean", BuildDate:"2021-10-29T23:32:16Z", GoVersion:"go1.16.8", Compiler:"gc", Platform:"linux/amd64"}
  • Tekton Pipeline version:
Client version: 0.23.0
Pipeline version: v0.28.2
Triggers version: v0.18.0
Dashboard version: v0.24.1
@viniagostini viniagostini added the kind/bug Categorizes issue or PR as related to a bug. label Mar 18, 2022
@dibyom dibyom added kind/feature Categorizes issue or PR as related to a new feature. priority/backlog Higher priority than priority/awaiting-more-evidence. and removed kind/bug Categorizes issue or PR as related to a bug. labels Mar 21, 2022
@Aleromerog
Copy link
Contributor

/assign @Aleromerog

@Aleromerog
Copy link
Contributor

Aleromerog commented Apr 26, 2022

After some attempts of not evaluating strings inside quotes, we realized there may be cases when a user would want variables inside quotes to be treated as variables. So there are a couple of options for this:

  1. We use something besides " or ' to indicate we should not escape e.g. $(params.&lt;&gt;) or $$(params.<>)
  2. We use "", or ' but use a feature flag at the controller level to decided which behaviour to use
  3. We use an annotation on the task/taskrun to decide which behaviour to use
  4. Evaluate the string at a task scope level with only the variables on that scope

Maybe option 1 is a quick and easy fix, but I'd like to have some feedback on this.
@vdemeester wdyt?

@vdemeester
Copy link
Member

For me, the easiest fix would be to not validate (nor interpolate) things in Params of a Task. But with embedded Task, it seems it could be a problem. I think the approach in #4835 (comment) would work fine as well.

Aleromerog added a commit to Aleromerog/pipeline that referenced this issue May 10, 2022
This commit removes validation for fields containing a parameter variable
after the parameter value has already been substituted in TaskRun and PipelineRun.
This is done to allow parameter values to contain literal strings like $(params.a); for example, in tektoncd#4690.
Aleromerog added a commit to Aleromerog/pipeline that referenced this issue May 10, 2022
This commit removes validation for fields containing a parameter variable
after the parameter value has already been substituted in TaskRun and PipelineRun.
This is done to allow parameter values to contain literal strings like $(params.a); for example, in tektoncd#4690.
Aleromerog added a commit to Aleromerog/pipeline that referenced this issue May 10, 2022
This commit removes validation for fields containing a parameter variable
after the parameter value has already been substituted in TaskRun and PipelineRun.
This is done to allow parameter values to contain literal strings like $(params.a); for example, in tektoncd#4690.
Aleromerog added a commit to Aleromerog/pipeline that referenced this issue May 10, 2022
This commit removes validation for fields containing a parameter variable
after the parameter value has already been substituted in TaskRun and PipelineRun.
This is done to allow parameter values to contain literal strings like $(params.a); for example, in tektoncd#4690.
Aleromerog added a commit to Aleromerog/pipeline that referenced this issue May 11, 2022
This commit removes validation for fields containing a parameter variable
after the parameter value has already been substituted in TaskRun and PipelineRun.
This is done to allow parameter values to contain literal strings like $(params.a); for example, in tektoncd#4690.
Aleromerog added a commit to Aleromerog/pipeline that referenced this issue May 11, 2022
This commit removes validation for fields containing a parameter variable
after the parameter value has already been substituted in TaskRun and PipelineRun.
This is done to allow parameter values to contain literal strings like $(params.a); for example, in tektoncd#4690.
Aleromerog added a commit to Aleromerog/pipeline that referenced this issue May 12, 2022
This commit removes validation for fields containing a parameter variable
after the parameter value has already been substituted in TaskRun and PipelineRun.
This is done to allow parameter values to contain literal strings like $(params.a); for example, in tektoncd#4690.
Aleromerog added a commit to Aleromerog/pipeline that referenced this issue May 12, 2022
This commit removes validation for fields containing a parameter variable
after the parameter value has already been substituted in TaskRun and PipelineRun.
This is done to allow parameter values to contain literal strings like $(params.a); for example, in tektoncd#4690.
@Aleromerog
Copy link
Contributor

Aleromerog commented May 13, 2022

I've addressed this bug for TaskRuns, but it still needs to be addressed for PipelineRuns. Here's an example PipelineRun where this error appears:

apiVersion: tekton.dev/v1beta1
kind: Pipeline
metadata:
  name: some-pipeline
  namespace: my-ns
spec:
  params:
  - name: input
  tasks:
  - name: foo
    taskSpec:
    params:
    - name: input
      value: $(params.input)
    steps:
    - name: print
      image: alpine
      script: |
        echo $(params.input)
---
apiVersion: tekton.dev/v1beta1
kind: PipelineRun
metadata:
  name: some-pipeline-run-2
  namespace: my-ns
spec:
  pipelineRef:
    name: some-pipeline
  serviceAccountName: tekton-triggers-admin
  params:
    - name: input
      value: '{"key": "$(params.a)"}'

tekton-robot pushed a commit that referenced this issue May 16, 2022
This commit removes validation for fields containing a parameter variable
after the parameter value has already been substituted in TaskRun and PipelineRun.
This is done to allow parameter values to contain literal strings like $(params.a); for example, in #4690.
yannickhilber pushed a commit to yannickhilber/pipeline that referenced this issue May 17, 2022
This commit removes validation for fields containing a parameter variable
after the parameter value has already been substituted in TaskRun and PipelineRun.
This is done to allow parameter values to contain literal strings like $(params.a); for example, in tektoncd#4690.
@Aleromerog
Copy link
Contributor

Aleromerog commented May 24, 2022

Merged a PR for this.

@jerop
Copy link
Member

jerop commented May 24, 2022

Thanks @Aleromerog!

@jerop jerop closed this as completed May 24, 2022
@lbernick
Copy link
Member

I was able to reproduce this bug on main. It looks like it was reintroduced by #4891

@lbernick lbernick reopened this May 16, 2023
@tekton-robot
Copy link
Collaborator

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 tekton-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Aug 14, 2023
@tekton-robot
Copy link
Collaborator

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 tekton-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 13, 2023
@tekton-robot tekton-robot added the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Sep 13, 2023
@tekton-robot
Copy link
Collaborator

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
Copy link
Collaborator

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature. lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. priority/backlog Higher priority than priority/awaiting-more-evidence.
Projects
None yet
Development

No branches or pull requests

7 participants