fix(executor): add executable permission to staged script

[vatine]

Fixes #3496

Motivation

As-was, staged scripts end up with the permission 0644 (rw-r--r--) which is less than useful for shell scripts. With this modification in place, the scripts are executable.

Modifications

Changed file permissions for the staged script.

Verification

Built a docker image of argoexec, changed the workflow-controller flags to use the private image and verified that pod logs no longer had a /argo/staging/script: permission denied in them

[vatine]

I must say that I am not entirely sure how the test failures relate to this change. They seem to relate to completely different things.

[agilgur5]

I must say that I am not entirely sure how the test failures relate to this change. They seem to relate to completely different things.

Hmm seems to be the same one just mentioned on Slack in #12713. Indeed seems unrelated

[agilgur5]

verified that pod logs no longer had a /argo/staging/script: permission denied in them

interestingly I've not seen this error before -- I wonder why it just popped up for you and hasn't popped up before for others?

[vatine]

Even more interestingly, I get the permission error for some images, but not for others (for the ones where this change was required, I also needed to include "shebang" lines listing an interpreter). I have not managed to nail down what the actual difference between these are.

I can have a look at making the permission/mode value variable and setting it to 0o755 only for scripts.

[agilgur5]

Even more interestingly, I get the permission error for some images, but not for others (for the ones where this change was required, I also needed to include "shebang" lines listing an interpreter)

This lines up with what I'm suspecting, which is that this might be a shell form vs. exec form instance or something similar. Like if you have a script without executable permissions, you can still run it with, e.g. bash my-script.sh, but . my-script.sh will fail.

Looking at the code, the path to the script source file is passed as an arg. I'm not entirely sure if command is set elsewhere, so it may very well depend on your container image's default cmd -- I think that would add up.

[vatine]

Certainly, the images that I can recall failing are on private repos (and, thus, as detailed in #11159 , the workflow controller cannot read the command from the image when constructing the pod), I have tried both bash and bash -c as the configured command in the configmap, with identical results. I can check what the pod command setting is. What I do know is that with this executable permission, it works. In all cases where this has failed, it is also not the first step in a workflow.

[vatine]

This is the command and args from the relevant pod:

$ kubectl get pod -n argo <pod name> -o json | jq '.spec.containers[1] | (.command,.args)'
[
  "/var/run/argo/argoexec",
  "emissary",
  "--loglevel",
  "debug",
  "--log-format",
  "text",
  "--"
]
[
  "/argo/staging/script"
]

I still cannot explain why it works in some cases and fail in others.

[agilgur5]

I still cannot explain why it works in some cases and fail in others.

Sorry for the delayed response; I wanted to take some time to see if we could root cause this and have been pretty overloaded. Thanks for looking more into this.

I have tried both bash and bash -c as the configured command in the configmap, with identical results.

This is surprising... based on this and your main container's command, it almost seems like it's not picking that command up from your configmap? If it works in some but not others, I might guess some images are not specified? 🤔

In all cases where this has failed, it is also not the first step in a workflow.

This shouldn't matter, it has no impact on the command.

What I do know is that with this executable permission, it works.

Based on my analysis in #11159 (comment), since we don't know the mode of the command beforehand, I think this might still be a correct change; some commands will need this permission.
Since it's a permission though, I want to be pretty careful to not extend it in a way that could cause a vuln. So far, most threat models I've thought of are not particularly impactful since the script is user input anyway, but I'm not sure I've covered all the bases.

[vatine]

This is what Argo puts in the pod's args: and command: (note this probably has some relevance to #11159, as I see no actual trace of the "command" or "entrypoint" in the argoexec call; for those where t just seems to work, can see a terminal "/bin/sh" on the argoexec command line).

$ kubectl get pods -n argo pr-run-e-e-tes-202404191144-docker-1590034149 -o json | jq '.spec.containers[1] | .command, .args'
[
  "/var/run/argo/argoexec",
  "emissary",
  "--loglevel",
  "info",
  "--log-format",
  "text",
  "--"
]
[
  "/argo/staging/script"
]

This is with the configmap configured to have that image mapped to:

cmd:
  - /bin/bash

[agilgur5]

Based on my analysis in #11159 (comment), since we don't know the mode of the command beforehand, I think this might still be a correct change; some commands will need this permission.

Apparently, per #13549, there was a separate issue filed exclusively for the script exec perms in #3496. Given both of those and the above, this is probably worthwhile to have in its own right, regardless of the Emissary nuances

[agilgur5]

Thanks for finding, fixing, and iterating on this again!

[agilgur5]

I have tried both bash and bash -c as the configured command in the configmap, with identical results.

This is surprising... based on this and your main container's command, it almost seems like it's not picking that command up from your configmap?

This is with the configmap configured to have that image mapped to:

cmd:
  - /bin/bash

I found the problem here. cmd in Docker is equivalent to args in k8s. There's no example of this in the docs, but you could set entrypoint instead in your images config.

The bug is that for script templates, cmd seems to be accidentally ignored. The entrypoint + cmd lookup only sets args if they are not already set in the container spec. And for script templates, the ScriptSourcePath, /argo/staging/script currently, is appended to the args just before Pod creation -> entrypoint lookup occurs.

This is fixable by changing the ordering, but may be a breaking change for script templates, as existing ones would unexpectedly have their cmd added.

For clarity, the full command + args for script templates would end up as:

[
  "/var/run/argo/argoexec",
  "emissary",
  "--loglevel",
  "info",
  "--log-format",
  "text",
  "--",
  "<k8s command OR, if not present, docker entrypoint>"
],
[
  "<k8s args OR, if not present, docker cmd>",
  "/argo/staging/script"
]

where cmd is currently ignored.