-
Notifications
You must be signed in to change notification settings - Fork 336
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
Implement UpdateRemoteUserID handling #1287
base: main
Are you sure you want to change the base?
Implement UpdateRemoteUserID handling #1287
Conversation
@aacebedo Thanks for the PR! Let's run the test suite real quick and see if it works out. As for your PR I think scoping it only to linux, as per the spec, would already limit the potential blast radius. I'll still need to test the change with a number of provider/workspace combinations |
57c95a3
to
b07caec
Compare
fixed the lint issue |
b07caec
to
f6f5d0f
Compare
f6f5d0f
to
ea68d64
Compare
ea68d64
to
4f419f7
Compare
If everything is OK can we merge? |
4f419f7
to
f46464d
Compare
rebased on main and test E2E tests. It is now working |
@aacebedo I haven't forgotten this PR, I'm still thinking through it and weigh against other options. Thanks for the work you've put in, bear with me :) |
Thanks! I have some users who are facing the issue and it is very annoying for them. Do you have an idea of an ETA? Can I help? |
@aacebedo I've tested this locally but did not get the result I thought I would. Using the following devcontainer.json and Dockerfile, I toggled
When updateRemoteUserUID is true
When updateRemoteUserUID is false
Should the false entry have dropped the extra info? Also what is the current issue you are facing? When I set this to false, I thought I would have permission issues but can still mount my workspace |
Hi @bkneis You printed the /etc/passwd of the container right? I'll check For the issue I am facing, it is described in #1284. Ensure that the UID GID of the user in the container is not the same as the one on your host. |
@aacebedo Thanks for clarifying. Yes these two files were from the container. My host user has UID 1000 so I was expecting a different UID when setting to true. I am not using podman, but I don't think this should make a difference for this test? As it should still parse my /etc/passwd and check the UID? Just want to make sure the PR is working as expected before merging |
@bkneis I tried your example with my branch and did not see this issue, in my case the /etc/passwd file is correct disregarding the value updateRemoteUserUID (it is not truncated). Can you recheck please (I used your example)? |
@bkneis could you give me more details about the commands to reproduce? |
@aacebedo I just retested with your branch but could not reproduce my issue. Toggling
when the UID is the same, which is expected. So it must have been finger trouble my end |
Great so we can pass the tests on it and merge it then? |
This PR is a tentative implementation of the UpdateRemoteUserID handling.
The reference implementation uses an additional dockerfile (https://github.com/devcontainers/cli/blob/main/scripts/updateUID.Dockerfile) to create a new image using the container image created previously.
I don't think this approach works well with prebuilding feature of devpod. So I decided to amend the
container after it has been created.
In some case, this modification may make the same uid gid to be used twice for 2 users and 2 groups but in the end it shall not impact the way the container work in the end.
This also solves #1284.