Skip to content

fix: don't use adduser and addgroup for docker images #3344

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

Merged
merged 4 commits into from
Aug 1, 2022

Conversation

deansheather
Copy link
Member

Adds back armv7 and arm64 images. Changes the Dockerfile to avoid using RUN and adds comments explaining that RUN should never be used.

Uses a docker cp to copy the /etc/passwd and /etc/group files out of the base image (which works on multi-arch using docker create), adds the coder user/group lines and then copies them back in during the real image build.

Fixes #3337

@deansheather deansheather requested review from kylecarbs and Emyrk August 1, 2022 18:02
docker cp "$temp_container_id":/etc/passwd ./passwd
docker rm "$temp_container_id"

echo "coder:x:1000:coder" >> ./group
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we just run the proper commands in the container here instead then copy the files? Seems like it could be simpler.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can't run commands in the container because it's a different architecture. All of the binaries like sh, echo, useradd, groupadd etc. are for whatever architecture the image is.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could just check in the /etc/passwd and /etc/group files to the repo, but then they won't update if alpine changes anything, so I opted for this instead.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I mean you can run them in this container instead of doing the echo

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

e.g. docker exec $temp_container_id adduser

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That won't work since the container is for a different architecture. Docker lets you create containers for different architectures but they won't start because of exec format errors. I'm taking advantage of that here to copy the files out of the image for the correct arch.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I 🤦🤦🤦🤦🤦🤦🤦🤦 so hard when I realized how wrong I was.

Copy link
Member

@kylecarbs kylecarbs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TY for fixing this so quickly 🥳🥳🥳

Copy link
Member

@Emyrk Emyrk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool way around this

@deansheather deansheather enabled auto-merge (squash) August 1, 2022 18:53
@deansheather deansheather merged commit 66a5b0f into main Aug 1, 2022
@deansheather deansheather deleted the docker-adduser-multiarch branch August 1, 2022 19:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add arm64 and armv7 Docker images
3 participants