Encode scripts as base 64 to avoid k8s mangling "$$" #3963
Conversation
|
The following is the coverage report on the affected files.
|
|
/kind bug |
|
The following is the coverage report on the affected files.
|
|
/test tekton-pipeline-unit-tests |
|
The following is the coverage report on the affected files.
|
|
TODO:
|
|
The following is the coverage report on the affected files.
|
| # Test that bash scripts with variable evaluations work as expected | ||
| - name: bash-variable-evaluations | ||
| image: bash:5.1.8 | ||
| workingDir: /workspace |
There was a problem hiding this comment.
Is this workspace needed here? (also for the v1beta1 version)
There was a problem hiding this comment.
Ah good catch, it isn't needed when running as root. I think I was playing around with different runAsUser settings and added this for non-root. Removed from both examples, cheers!
|
The following is the coverage report on the affected files.
|
|
/test pull-tekton-pipeline-alpha-integration-tests |
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
/test pull-tekton-pipeline-build-tests |
|
/test pull-tekton-pipeline-integration-tests |
| } | ||
| return SubcommandSuccessful{message: fmt.Sprintf("Decoded script %s", src)} | ||
| } | ||
| default: |
There was a problem hiding this comment.
does this mean invalid args are ignored? (or does this just stop it from processing something that doesnt look like a subcommand?)
There was a problem hiding this comment.
Yeah, just trying to capture the fact that the default operation is to "do nothing, return nil".
|
ugh this was a surprisingly tricky one!!! ps. thanks for the FABULOUS commit message 😍 !!!
🙃
feels like these could potentially benefit from some test coverage, what do you think? im thinking especially subcommands just lemme know if you feel its not worth it, i noticed you included unit tests for decode script so maybe you felt it wasnt worth it to maintain unit tests for these other tiny files - i think it's the indexing into the |
There was a problem hiding this comment.
Ah yeah good point, I added tests for decodeScript because I wanted to try out the new fstest built-in package, unfortunately it only lets you mock a read-only filesystem so I didn't bother writing additional new tests for cp and subcommands as they both write. I'll add some for these using a TempDir.
| } | ||
| return SubcommandSuccessful{message: fmt.Sprintf("Decoded script %s", src)} | ||
| } | ||
| default: |
There was a problem hiding this comment.
Yeah, just trying to capture the fact that the default operation is to "do nothing, return nil".
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
Pretty neat @sbwsg and thanks for the great commit message, did base-122 was investigated for size? This blog post details it, https://blog.kevinalbs.com/base122 and the go library from the same author has as well some details https://github.com/kevinAlbs/Base122 I was wondering if the argument had a size limit, but it seems that k8/container runtime is limited by what the linux kernel is providing, which is pretty much unlimited as the system provide it, from the manpage https://linux.die.net/man/2/execve : |
|
Cheers @chmouel I hadn't considered using a different encoding like Base 122! My only hesitation is that the design and code are controlled by a single person it looks like, and it sounds from the readme that the author still considers the design a work-in-progress. Given that we're not exposing the encoding as part of our API we could always iterate on it and try out better / more compact representations over time if users complain about size or other issues. |
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
| set -xe | ||
| var1=var1_value | ||
| var2=var1 | ||
| echo $(eval echo \$$var2) > tmpfile |
|
/lgtm thanks @sbwsg , let's hope this fix our variables encoding issues 🤞🏻 |
|
oops. I'm missing a file truncate somewhere in the switch back to the |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: afrittoli, vdemeester The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest |
|
OK, decoded script files should be correctly truncated now. With my previous change the script file was ending up as the decoded script + the tail of the base64 string. |
|
The following is the coverage report on the affected files.
|
| @@ -0,0 +1,30 @@ | |||
| package subcommands | |||
There was a problem hiding this comment.
I love this refactoring ❤️ awesome work @sbwsg 👏
| @@ -0,0 +1,30 @@ | |||
| package subcommands | |||
There was a problem hiding this comment.
Missing copyright license headers 😜
There was a problem hiding this comment.
👍 thanks @pritidesai ; added the license headers.
Kubernetes replaces instances of "$$" in container args fields with "$". This can muck up the contents of script fields because scripts are passed into a TaskRun Pod as an arg to an init container. Prior to this commit we [tried to prevent the replacement](#3888) from happening by: 1. putting scripts into annotations on a pod and projecting them using downward API - con: the max size of the annotations map is capped to ~250kB. The aggregate size of all scripts in a single Task therefore becomes constrained by this. Any other systems using annotations will reduce the available headroom. Backwards incompatible. 2. replacing instances of "$$" in scripts with "$$$$" for k8s to then process back to "$$" - con: k8s doesn't actually process _all_ instances of "$$". For example, if you write an arg with format "echo $(eval \$$foo)" then k8s will see the first "$(", assume it's a variable reference, and pass it through verbatim. So user's scripts with bash variable become broken by tekton's new replacement. Backwards incompatible. This commit takes a third approach, proposed by @MartinKanters, encoding scripts as base64 in the controller and then having them decoded in the init container. This bypasses Kubernetes' args processing completely because dollar signs aren't used in base64 encodings. It also doesn't introduce a backwards-incompatible limit to the aggregate script size. And it doesn't mangle existing bash scripts with variable replacements. The most noticeable trade-offs we now make are: 1. Tiny scripts can be up to 300% bigger, but as scripts get longer the max increase gets closer to 133%. 2. Also the TaskRun's `initContainer` YAML is a bit less human readable: ``` initContainers: - args: - -c - | tmpfile="/tekton/scripts/script-0-f8fmf" touch ${tmpfile} && chmod +x ${tmpfile} cat > ${tmpfile} << '_EOF_' IyEvYmluL3NoCnNldCAteGUKZWNobyAibm8gc2hlYmFuZyI= _EOF_ /tekton/tools/entrypoint decode-script "${tmpfile}" ``` The entrypoint is extended to decode base64 files so that the `shellImage` (which is used to write scripts to disk for Step containers) is not required to package a `base64` binary.
|
The following is the coverage report on the affected files.
|
|
/test pull-tekton-pipeline-integration-tests |
|
/test pull-tekton-pipeline-alpha-integration-tests |
| initContainer.Args[1] += fmt.Sprintf(`tmpfile="%s" | ||
| touch ${tmpfile} && chmod +x ${tmpfile} | ||
| cat > ${tmpfile} << '%s' | ||
| %s | ||
| %s | ||
| /tekton/tools/entrypoint decode-script "${tmpfile}" |
There was a problem hiding this comment.
NIT: can we use something more meaningful here instead of tmpfile, may be a script file or a encoded script file? looks odd to see a tmpfile in pod description:
kubectl get pod/dollar-dollar-taskrun-lztnb-pod-tvs2h -o json | jq .spec.initContainers
...
{
"args": [
"-c",
"tmpfile=\"/tekton/scripts/script-0-ck986\"\ntouch ${tmpfile} && chmod +x ${tmpfile}\ncat > ${tmpfile} << '_EOF_'\nIyEvYmluL3NoCnNldCAteGUKZWNobyAndHdvIGRvbGxhciBzaWduczogJCQnCmVjaG8gJ2ZvdXIgZG9sbGFyIHNpZ25zOiAkJCQkJwplY2hvICd0d28gZG9sbGFyIHNpZ25zIGZyb20gYW4gZW52aXJvbm1lbnQgdmFyaWFibGU6ICcgJERPTExBUl9ET0xMQVJfRU5WCg==\n_EOF_\n/tekton/tools/entrypoint decode-script \"${tmpfile}\"\n"
...
It's a very strong disadvantage but there is no way around to deal with such special characters. thanks @sbwsg /lgtm |
Changes
Fixes #3871
Kubernetes replaces instances of "$$" in container args fields with "$". This can muck up the contents of script fields because scripts are passed into a TaskRun Pod as an arg to an init container. In particular,
$$is used for the current process id in bash scripts.Prior to this commit we tried to prevent the replacement from happening by:
putting scripts into annotations on a pod and projecting them using downward
API
replacing instances of "$$" in scripts with "$$$$" for k8s to then process
back to "$$"
This commit takes a third approach, proposed by @MartinKanters, encoding scripts as base64 in the controller and then having them decoded in the init container. This bypasses Kubernetes' args processing completely because dollar
signs aren't used in the default base64 alphabet.
This approach avoids introducing a backwards-incompatible limit to the aggregate script size and doesn't mangle existing bash scripts with variable replacements.
The most noticeable trade-offs we now make are:
initContainerYAML is a bit less human readable:The entrypoint is extended to decode base64 files so that the
shellImage(which is used to write scripts to disk for Step containers) is not required
to package a
base64binary.Submitter Checklist
As the author of this PR, please check off the items in this checklist:
functionality, content, code)
Release Notes