-
Notifications
You must be signed in to change notification settings - Fork 221
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
Set the HOME env variable to be consistent with the used home directory #1296
base: main
Are you sure you want to change the base?
Conversation
…ot root Signed-off-by: Dawid Kulikowski <git@dawidkulikowski.pl>
Build succeeded. ✔️ unit-test SUCCESS in 9m 28s |
@debarshiray @HarryMichal I was wondering if you could take a look at this PR? It's very straight forward. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My apologies for not responding sooner, @daw1012345 I don't think I fully understand the actual user facing problem here.
I saw your screenshot in containers/podman#18492. The first time the container is started, Podman creates the user and the podman inspect
JSON shows HOME
to be something like /home/foo
. On subsequent starts, the user is already there and the podman inspect
JSON shows HOME
to to be /root
. I got this bit, and I agree that it's odd for it to change it's value like that, and thanks for fixing that in Podman.
However, if you enter
the container, HOME
is always set to /home/foo
.
Are you saying that the tools look at the podman inspect
JSON for the value of HOME
to place the files? I wonder if some of these tools can be made to look at something like:
toolbox run sh -c 'echo $HOME'
I also didn't understand some of the phrases in containers/podman#18492 How is HOME
different from the home folder of the user executing the podman command? I left a more verbose comment on the pull request.
@@ -426,6 +426,7 @@ func createContainer(container, image, release, authFile string, showCommandToEn | |||
"--volume", homeDirMountArg, | |||
"--volume", toolboxPathMountArg, | |||
"--volume", runtimeDirectoryMountArg, | |||
"--env", fmt.Sprintf("HOME=%s", currentUser.HomeDir), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: could you please move this further up between --dns
and --env
?
I suppose this will bake HOME
into the podman inspect
JSON as /home/foo
? Just like we do for TOOLBOX_PATH
and XDG_RUNTIME_DIR
?
If so, then we need to remember that there are two processes with two different UIDs that are started by Toolbx inside the containers. The first is the entry point of the container that runs as root:root
and the other is the process running as the user's UID that the user actually interacts with. Currently, the entry point has HOME
set to /home/foo
on the first start, and then /root
for subsequent starts.
I am worried that baking HOME
to /home/foo
will make the entry point (running as root:root
) always use it as it's home directory. If the entry point ends up creating some files in its home directory as a side-effect, these will pollute the user's home directory too, and, potentially, even conflict with the user's own files.
This isn't a huge problem because this is already the case for the first start, so, I suppose things are working alright at the moment. However, it will entrench it a bit more.
Since we are already trying to clean up this weird oddity with the HOME
environment variable, I think we should also take the step to ensure that the entry point running as root:root
has HOME
set to the correct value for root
.
Sorry for the confusion @debarshiray, in the original PR I was confused about what the intended behavior was. In the end, the bug was that the HOME environment variable was being set to the home of the user starting/creating the container while the intended behavior was to set it to the home of the user specified with the
Yes, this is the case, most notably with the VSCode Dev Containers extension. Specifically, it seems that internally it runs
As you mentioned, this wasn't an issue before while this bug was present in podman. However, I can investigate if there are any issues that may arise. |
After having a quick look, it seems that the entry point doesn't use the HOME env var or even the directory of the current user. In other words, I don't think there's anything to fix in the entrypoint. |
@debarshiray Any updates? My fixes in podman were released with v4.6.0. |
@debarshiray can you take a look at this PR again? |
That change could actually be misleading for the VSCode dev containers extension as it would mean that the vscode server install ends up in the HOME folder, thus being shared across every toolbox user-wide. Then it won't be possible to have different extensions installed depending on the toolbox attached in VSCode, etc. @lbssousa do you agree on that take ? I think the actual right location for the .vscode-server in the toolbox could be |
@nicocti However, I do agree that separation of extensions between toolbox containers should be preserved. Both vscode-for-toolbox and toolbox-vscode address this in one way or another. In the latest version of vscode-for-toolbox, the VSCode server gets installed on a podman volume to achieve this goal. |
Note: I did fix my previous comment
I'm not sure to understand, for me it's a feature I don't want to remove ! On my side I've made some tests and I don't understand where that |
I'm not saying the separation of vscode-servers is an issue, but currently it happens as a result of an issue (if vscode launches at all - it has not worked for me out of the box mainly because of permission issues, e.g. trying to write to
The issue is that toolbox does not set the HOME variable, which causes podman to infer the home directory from the user specified in the |
Thanks for the clarification. What I've been doing on my side is rebuilding the toolbox image with my tools + a fix for vscode to install the server: FROM registry.fedoraproject.org/fedora-toolbox:38
RUN mkdir /root/.vscode-server && chmod o+x /root && chown -R ${USER_UID}:${USER_UID} /root/.vscode-server Then, I install {
"dev.containers.dockerPath": "/app/tools/podman/bin/podman-remote",
"dev.containers.dockerComposePath": "podman-compose"
} I'm then able to attach my projects to my running toolbox. What I don't get is why does the dev containers extensions always try to install vscode-server to the /root folder (even when using microsoft default devcontainers with those settings : https://code.visualstudio.com/remote/advancedcontainers/docker-options#_podman). Isn't there a divergent behavior between docker and podman regarding how the HOME env is defined ? I don't have docker running on my current machine, but will try asap. |
I have created a small project that makes the integration of vscode and toolbox smooth. It essentially automates a lot of the steps that you mentioned, however it does not need for the containers to be modified at all. If you're interested, you can check it out here. The latest version also provides a workaround for this issue since it hasn't been merged for months. It works great on my machine, but I'd always appreciate it if more people tried it out.
This is because of $HOME - VSCode reads it to decide where to place the files. It is not an issue with podman - it makes the logical decision to set HOME=/root because toolbox sets the |
As per containers/podman#18492 , in podman the HOME environment variable defaults to the home of the caller on first container start. Upon a restart, it defaults to the home of the user specified with
--user
which is root in the case of toolbox. If the linked PR is merged, the default will be defined by--user
and will be used unless the HOME env variable is explicitly defined.This PR explicitly sets the HOME env variable to be consistent with the home directory used by toolbox. Certain tools such as VSCode Dev Containers extract information about the home directory via this variable.