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

v0.24.0: broken bash variable evaluation #3935

Closed
r0bj opened this issue May 10, 2021 · 6 comments · Fixed by #3938
Closed

v0.24.0: broken bash variable evaluation #3935

r0bj opened this issue May 10, 2021 · 6 comments · Fixed by #3938
Labels
kind/bug Categorizes issue or PR as related to a bug.

Comments

@r0bj
Copy link

r0bj commented May 10, 2021

Expected Behavior

bash variable evaluation works fine.

Actual Behavior

bash variable evaluation is broken.

Steps to Reproduce the Problem

taskrun.yaml:

---
apiVersion: tekton.dev/v1beta1
kind: TaskRun
metadata:
  name: test
spec:
  taskSpec:
    steps:
    - name: test
      image: bash:5.1.8
      script: |
        #!/usr/bin/env bash
        set -xe

        var1=var1_value
        var2=var1
        echo $(eval echo \$$var2)

Taskrun result:

+ var1=var1_value
+ var2=var1
++ eval echo '$15var1'
+++ echo 5var1
+ echo 5var1
5var1

Note: Output should be var1_value instead of 5var1.

On tekton pipelines before version v0.24.0 it works as expected:

+ var1=var1_value
+ var2=var1
++ eval echo '$var1'
+++ echo var1_value
+ echo var1_value
var1_value

Additional Info

  • Kubernetes version:

    Output of kubectl version:

Client Version: version.Info{Major:"1", Minor:"20+", GitVersion:"v1.20.4-dirty", GitCommit:"e87da0bd6e03ec3fea7933c4b5263d151aafd07c", GitTreeState:"dirty", BuildDate:"2021-03-15T09:58:13Z", GoVersion:"go1.16.2", Compiler:"gc", Platform:"darwin/amd64"}
Server Version: version.Info{Major:"1", Minor:"20", GitVersion:"v1.20.4", GitCommit:"e87da0bd6e03ec3fea7933c4b5263d151aafd07c", GitTreeState:"clean", BuildDate:"2021-02-18T16:03:00Z", GoVersion:"go1.15.8", Compiler:"gc", Platform:"linux/amd64"}
  • Tekton Pipeline version:
Client version: 0.18.0
Pipeline version: v0.24.0
Triggers version: v0.13.0
@r0bj r0bj added the kind/bug Categorizes issue or PR as related to a bug. label May 10, 2021
@vdemeester
Copy link
Member

/cc @sbwsg
This probably relates to #3888

@ghost
Copy link

ghost commented May 11, 2021

Could you post the generated pod?

Edit: nm, I'll recreate the issue on my side.

@ghost
Copy link

ghost commented May 11, 2021

The script gets expanded to:

    - args:
      - -c
      - |
        tmpfile="/tekton/scripts/script-0-tqc48"
        touch ${tmpfile} && chmod +x ${tmpfile}
        cat > ${tmpfile} << 'script-heredoc-randomly-generated-vf6hm'
        #!/usr/bin/env bash
        set -xe
        var1=var1_value
        var2=var1
        echo $(eval echo \$$$$var2)

        script-heredoc-randomly-generated-vf6hm
      command:
      - sh

This is the new script behaviour, expected. Kubernetes rewrites $$ as $ so we've doubled-up the number of dollar signs to try and prevent losing characters when k8s eventually rewrites our script.

But then the output is:

 (main) λ k logs -f test-pod-sq7x2
+ var1=var1_value
+ var2=var1
++ eval echo '$16var1'
+++ echo 6var1
+ echo 6var1
6var1

Sigh.

OK I'll make a revert commit and reopen #3871

@ghost
Copy link

ghost commented May 11, 2021

For someone who doesn't know the infinite reaches of bash scripting, what is eval echo \$$$$var2? Why so many dollars and a backslash, is this related to something in the way eval works?

Never mind. The 4 dollar signs are tekton. This is so confusing.

@ghost
Copy link

ghost commented May 11, 2021

For posterity's sake, here's how the script is rendered prior to 0.24.0:

    - args:
      - -c
      - |
        tmpfile="/tekton/scripts/script-0-fblm6"
        touch ${tmpfile} && chmod +x ${tmpfile}
        cat > ${tmpfile} << 'script-heredoc-randomly-generated-wz24z'
        #!/usr/bin/env bash
        set -xe
        var1=var1_value
        var2=var1
        echo $(eval echo \$$var2)

        script-heredoc-randomly-generated-wz24z
      command:
      - sh

Note \$$var2. See the resulting bash output:

 (tags/v0.23.0) λ k logs -f test-pod-kr8pg
+ var1=var1_value
+ var2=var1
++ eval echo '$var1'
+++ echo var1_value
+ echo var1_value
var1_value

Kubernetes converted the two dollar signs to a single dollar sign and we somehow lost the backslash.

@ghost
Copy link

ghost commented May 11, 2021

Bit more investigation: Kubernetes does replace instances of two dollar signs, "$$", with a single dollar sign, "$" but it also attempts to replace instances of "$(X)" with the contents of env var X. In this case k8s sees $( from $(eval echo \$$var2) and assumes that "eval echo $$var2" is a variable name. Of course it doesn't have an env var with that name so it simply passes the whole string, verbatim, through to the container.

Long story short the original "fix" was too naive and we should revert this change.

tekton-robot pushed a commit that referenced this issue May 11, 2021
This reverts commit 9a9f896.

Attempting to fix instances of "$$" introduced a new bug in the way
bash scripts are interpreted: #3935
afrittoli pushed a commit to afrittoli/pipeline that referenced this issue May 12, 2021
This reverts commit 9a9f896.

Attempting to fix instances of "$$" introduced a new bug in the way
bash scripts are interpreted: tektoncd#3935
tekton-robot pushed a commit that referenced this issue May 12, 2021
This reverts commit 9a9f896.

Attempting to fix instances of "$$" introduced a new bug in the way
bash scripts are interpreted: #3935
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.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants