exec: Fix incorrect HOME and USER env variables for tasks that have user set#25859
exec: Fix incorrect HOME and USER env variables for tasks that have user set#25859pkazmierczak merged 8 commits intomainfrom
HOME and USER env variables for tasks that have user set#25859Conversation
tgross
left a comment
tgross
left a comment
There was a problem hiding this comment.
LGTM. It looks like we're only setting these environment variables on Linux... should we be setting them on other Unix platforms as well?
yeah I got a little confused in the |
| execCmd, allocDir := testExecCmd.command, testExecCmd.allocDir | ||
| execCmd.Cmd = "/bin/bash" | ||
| execCmd.Args = []string{"-c", "echo $USER"} | ||
| execCmd.User = "runner" |
There was a problem hiding this comment.
This test is always going to fail anywhere except GitHub runners, right? Is there a testutil flag we can set to skip it?
Co-authored-by: Tim Gross <tgross@hashicorp.com>
|
Mind updating our docs to mention this behavior? Unrelated https://developer.hashicorp.com/nomad/docs/job-specification/task#user incldues the weird phrase I think the most important docs would be to add |
… custom `user` setting into release/1.9.x (#25887) * no-op commit due to failed cherry-picking * docs: emphasize HOME and USER env vars for tasks that use custom `user` setting (#25879) In #25859 we fixed the task environment variables to account for user field setting. This PR follows up with documentation adjustments. --------- Co-authored-by: temp <temp@hashicorp.com> Co-authored-by: Piotr Kazmierczak <470696+pkazmierczak@users.noreply.github.com>
… custom `user` setting into release/1.8.x (#25888) * no-op commit due to failed cherry-picking * docs: emphasize HOME and USER env vars for tasks that use custom `user` setting (#25879) In #25859 we fixed the task environment variables to account for user field setting. This PR follows up with documentation adjustments. --------- Co-authored-by: temp <temp@hashicorp.com> Co-authored-by: Piotr Kazmierczak <470696+pkazmierczak@users.noreply.github.com>
| // Override HOME and USER environment variables | ||
| cmd.Env = append(cmd.Env, fmt.Sprintf("USER=%s", u.Username)) | ||
| cmd.Env = append(cmd.Env, fmt.Sprintf("HOME=%s", u.HomeDir)) |
There was a problem hiding this comment.
There are could be a lot of variables missing. Also this could depend on the system where this is run.
@jbardin , @pkazmierczak
I expect that something like su - <username> -c "<command>" will be run and let the system to decide which variables should be adjusted.
There was a problem hiding this comment.
I suspect, if HomeDir will have spaces the code above will not work. Probably it should be "HOME=\"%s\"".
There was a problem hiding this comment.
I expect that something like su - -c "" will be run and let the system to decide which variables should be adjusted.
The exec family of task drivers does not invoke a subshell to execute the task command. Docker, podman, systemd units, etc operate the same way to avoid security issues and crossplatform issues. Tasks which expect shell semantics should invoke the shell themselves. An su/subshell task driver would be an interesting experiment, but would be backward incompatible with the exec family.
I suspect, if HomeDir will have spaces the code above will not work. Probably it should be "HOME="%s"".
Quoting is a shell feature, not inherent to how UNIX encodes environment variables. Quotes should not be necessary even if the values contain spaces or additional equal signs. See: https://go.dev/play/p/FiV8FXiaNPr
Also please don't ping engineers in comments. jbardin hasn't worked on Nomad in years and is quite busy with other things. Commenting on PRs is welcome, but the author should be expected to receive a notification with being explicitly pinged.
jrasell
added
backport/ent/1.8.x+ent
|
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
5 participants
Fixes #25854