Tekton converts double dollar symbols to one

[MartinKanters]

Expected Behavior

When the script element in a Tekton Task definition has double dollar symbols, it should remain as two dollar symbols.

Actual Behavior

When the script element in a Tekton Task definition has double dollar symbols, the two symbols are reduced to one dollar symbol.

Steps to Reproduce the Problem

1. Apply the Sample TaskRun manifest defined at the bottom of this description
2. See the output log of Tekton. (also at the bottom)
3. I have verified that it is not a bug in the output log itself.

Additional Info

• Kubernetes version:

  Output of kubectl version:

  Client Version: version.Info{Major:"1", Minor:"20", GitVersion:"v1.20.4", 
  GitCommit:"e87da0bd6e03ec3fea7933c4b5263d151aafd07c", GitTreeState:"clean", BuildDate:"2021-02-22T19:05:30Z", 
  GoVersion:"go1.15.8", Compiler:"gc", Platform:"linux/amd64"}
  Server Version: version.Info{Major:"1", Minor:"19", GitVersion:"v1.19.7", 
  GitCommit:"14f897abdc7b57f0850da68bd5959c9ee14ce2fe", GitTreeState:"clean", BuildDate:"2021-01-22T17:29:38Z", 
  GoVersion:"go1.15.5", Compiler:"gc", Platform:"linux/amd64"}
  

• Tekton Pipeline version:

  Output of tkn version or kubectl get pods -n tekton-pipelines -l app=tekton-pipelines-controller -o=jsonpath='{.items[0].metadata.labels.version}'

  v0.22.0

---

Sample TaskRun:

apiVersion: tekton.dev/v1beta1
kind: TaskRun
metadata:
  generateName: dollar-dollar-taskrun-
spec:
  taskSpec:
    steps:
      - name: dollar-dollar-demo
        image: alpine:latest
        script: |
          echo 'two dollar signs: $$'
          echo 'four dollar signs: $$$$'
          echo 'two dollar signs from an environment variable: ' $DOLLAR_DOLLAR_ENV
        env:
          - name: DOLLAR_DOLLAR_ENV
            value: '$$'

Generated output (The lines prefixed with + are the result of the set -x which is added implicitly by Tekton):

+ echo 'two dollar signs: $'
two dollar signs: $
+ echo 'four dollar signs: $$'
four dollar signs: $$
+ echo 'two dollar signs from an environment variable: ' '$'
two dollar signs from an environment variable:  $

[ghost]

I see the same output as you. Checking the pod I can see that Tekton is passing the dollar signs through to the shell script exactly as written in the TaskRun:

$ k get pod dollar-dollar-taskrun-hwfrk-pod-x527m -o jsonpath="{range .spec.initContainers[]}{.args}"

["-c","tmpfile=\"/tekton/scripts/script-0-wwkmn\"\ntouch ${tmpfile} \u0026\u0026 chmod +x ${tmpfile}\ncat \u003e ${tmpfile} \u003c\u003c 'script-heredoc-randomly-generated-n8gwj'\n#!/bin/sh\nset -xe\necho 'two dollar signs: $$'\necho 'four dollar signs: $$$$'\necho 'two dollar signs from an environment variable: ' $DOLLAR_DOLLAR_ENV\n\nscript-heredoc-randomly-generated-n8gwj\n"]

and the env var:

k get pod dollar-dollar-taskrun-hwfrk-pod-x527m -o jsonpath="{range .spec.containers[]}{.env}"    
[{"name":"DOLLAR_DOLLAR_ENV","value":"$$"}]

The way the Step script block works Tekton writes a temp shell script with the contents of the block and then executes it in your Step's container. I modified the test yaml to print out the contents of that shell script:

        script: |
          for f in /tekton/scripts/* ; do
            cat $f
          done

And it looks like it's the script initContainer writing the temp file with the missing dollar signs:

[dollar-dollar-demo] + cat /tekton/scripts/script-0-n57jq
# [ ... ] snip
[dollar-dollar-demo] #!/bin/sh
[dollar-dollar-demo] set -xe
[dollar-dollar-demo] echo 'two dollar signs: $'
[dollar-dollar-demo] echo 'four dollar signs: $$'
[dollar-dollar-demo] echo 'two dollar signs from an environment variable: ' $DOLLAR_DOLLAR_ENV
[dollar-dollar-demo] for f in /tekton/scripts/* ; do
[dollar-dollar-demo]   cat $f
[dollar-dollar-demo] done
[dollar-dollar-demo]

So it looks like we need to figure out a way to write that shell script tempfile that doesn't do this. I'm not entirely sure what that is yet.

[MartinKanters]

Thanks for your research, it gave me some nice insights in the workings of Tekton.

I've been trying to replicate the behavior locally with Docker. I suspected the shell image that Tekton uses in the initContainer messes up the '$$'. I've found the code that creates the tempfile. I also think I found the shell image that is used in the initContainer.

But trying it locally gives:

$ docker run -i --entrypoint "" gcr.io/distroless/base@sha256:92720b2305d7315b5426aec19f8651e9e04222991f877cae71f40b3141d2f07e cat > '/tmp/t' << 'EOF'
test $$ dollar dollar
EOF
cat /tmp/t
test $$ dollar dollar

Any ideas?

[ghost]

Ah, after a bit of digging I think this is how Kubernetes treats $$ in the args and env fields of a pod. From https://v1-18.docs.kubernetes.io/docs/reference/generated/kubernetes-api/v1.18/#container-v1-core

The $(VAR_NAME) syntax can be escaped with a double $$, ie: $$(VAR_NAME). Escaped references will never be expanded, regardless of whether the variable exists or not.

And from https://v1-18.docs.kubernetes.io/docs/reference/generated/kubernetes-api/v1.18/#envvar-v1-core :

The $(VAR_NAME) syntax can be escaped with a double $$, ie: $$(VAR_NAME).

[ghost]

So I guess kubernetes is seeing the double dollar and, assuming that it is a variable escape, replaces it with a single dollar.

I am having a really hard time finding the k8s source for this variable substitution though! I can't find the exact lines of go that perform the replacement to confirm this behaviour.

[MartinKanters]

Nice find!

I'm pretty confident this is the source of the expansion. It is used here.
As I haven't set up a Go dev environment, I've copied the code from expand.go in a online Go playground. I've verified there that the double dollars will get reduced to one dollar. You can find it here: https://replit.com/join/btveakky-martinkanters.
Please see line 15 and run the command. If you cannot access it, here's a screencap:

[ghost]

Yeah, confirmed I see the same behaviour running your demo. I'm pretty well convinced now that this is indeed the source of the problem.

So Tekton might be able to work around this by injecting the script a different way. We could place it in an annotation on the Pod and then project that via downward API, for example. This way we bypass k8s' $$ replacement. I am going to try a proof-of-concept to see if that would work. Also paging @imjasonh here to see if he has any thoughts on best approaches to tackle this.

[imjasonh]

This is indeed surprising. Nice investigation work getting to the root cause so quickly! 👍

I don't have a better idea than injecting via the downward API.

[MartinKanters]

I've created a bug report on Kubernetes. But I can understand that waiting for this to be resolved is not the way to go for Tekton.

[ghost]

A workaround for this behaviour has been merged and should be released as part of 0.24 this week.

[ghost]

/reopen

I'm going to have to revert this change. The new workaround breaks some bash scripts: #3935

[tekton-robot]

@sbwsg: Reopened this issue.

In response to this:

/reopen

I'm going to have to revert this change. The new workaround breaks some bash scripts: #3935

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.

[ghost]

Copy/pasting the findings from #3935:

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.

[MartinKanters]

Sucks that the simple proposed solution was not enough.. I'm disappointed by Kubernetes' variable handling. $(a b c) should never be expanded to a variable imo.

Anyhow, I don't know too much about the implementation, but instead of finding workarounds for putting plaintext scripts into the containers, can't we first base64 decode it and encode it as first step in the container? Not sure if we have access to the base64 utility in that container, but something like this should prevent any data-tampering by Kubernetes I think.

[ghost]

can't we first base64 decode it and encode it as first step in the container?

Nice! I'll give this a try.

[ghost]

This works quite nicely (branch with changes over here) and doesn't mangle bash scripts but there are definitely trade-offs:

1. The Shell Image passed to the controller with the -shellImage flag must contain the base64 program. Busybox has this by default but if users are overriding this themselves then I'm unsure we can make that same guarantee. This is also kinda true for the existing implementation except the image only has to have the cat program.
2. Tiny scripts can be up to 300% bigger, but as scripts get longer the max increase gets closer to 133%.
3. The Pod's initContainer now show scripts being populated like this:

    initContainers:
    - args:
      - -c
      - |
        tmpfile="/tekton/scripts/script-0-f8fmf"
        touch ${tmpfile} && chmod +x ${tmpfile}
        base64 -d > ${tmpfile} << 'script-heredoc-randomly-generated-q8h4s'
        IyEvYmluL3NoCnNldCAteGUKZWNobyAibm8gc2hlYmFuZyI=
        script-heredoc-randomly-generated-q8h4s

We could work around trade-off (1) (base64 being required in -shellImage) by having the entrypoint perform the base64 decode instead of doing it in the init container.

I don't think there are any great workarounds for trade-offs (2) and (3) though.

I dunno, what do other folks think?

[MartinKanters]

Nicely done, @sbwsg . To me, the benefit (ensuring data integrity and no dollar wickedness) outweighs the trade-offs. Sure, scripts will increase in size (trade-off 2), but I believe we found out that Kubernetes allows a lot of data in this args field, right?
Debugging issues will become harder (trade-off 3), but it should be very clear to a user that it is base64 encoded. The format and code around it suggests it. trade-off 1, I'm not sure what the use-case is of overriding the shell image (I'm not that hardcore a user), but most images should have base64 enabled by default.

I doubt it, but might there be any performance concern perhaps around base64 encoding and decoding with bigger scripts? I think it should be very quick, but haven't tested it.

One other tiny, tiny benefit is that the heredoc can be reduced to something simple, as long as it does not contain any base64-allowed character ;)

[ghost]

Very good point about the heredoc, I will update the branch to take advantage of this!

Agreed on 2 & 3. There are ways we could increase the available script size, for example by splitting multiple scripts across multiple init containers writing them. And visual inspection of the scripts in the pod has always been quite difficult so base64 encoding doesn't feel like a big drop in usability (to me).

Requiring base64 in the shellImage strikes me as the biggest hurdle at the moment. I'm not able to make any strong assertions about who is or is not overriding it. I think that RedHat, for example, use a different shellImage for OpenShift Pipelines users and I expect that several other orgs do similar. This would be a breaking change I feel. One additional wrinkle is that Task Sidecars can also use script: fields and they don't use the entrypoint binary, so decoding in entrypoint wouldn't solve all instances. Will keep thinking about this!

I have, for the time being, created a PR to at least document the problem on our side: #3939

[MartinKanters]

Nice job on the docs!

I'm thinking of another idea.. I'm not sure what the use-case of using a custom initContainer image is, so perhaps this will sound ignorant. We could base64 decode the script into a file in a new initContainer (with a fixed image, containing base64) and make sure the original initContainer can get to it by using a volume mount.

Then again, I'm not sure what the use would be of the 'original' initContainer, if the first one can already fully prepare the script.
Also, I haven't ever tried using multiple initContainers with a shared volume between them, but reading the docs gave me the impression that it should be possible.

[ghost]

I've got a workaround going which no longer requires base64 in the shellImage. We update the entrypoint binary to include a decode-script subcommand and use that in the place-scripts initContainer. Works very nicely, thanks for the suggestion @imjasonh. Also updated with fixed heredoc as suggested, cheers @MartinKanters !

Changes visible at: main...sbwsg:fix-dollar-signs-in-scripts-with-base64

Need to update and add unit tests, integration, dev docs, etc... Next week.