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
Update Dockerfile. Update docker base images. #330
Conversation
6ad7131
to
dd96e9c
Compare
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.
I like that this adds the ability to set the port to a non-default value for folks who need that, and I'm glad we're hopefully resolving the permissions issues in a cross-platform way! I have a few questions and suggestions re: the implementation
Dockerfile
Outdated
libffi-dev \ | ||
&& mkdir /stuff && cd /stuff || exit 1 \ | ||
&& git clone https://github.com/ncopa/su-exec.git su-exec-clone \ | ||
&& cd su-exec-clone || exit 1 \ |
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.
I'd prefer we use gosu instead of su-exec
. I recognize it's larger, but they publish built binaries for their releases, which we could use rather than pulling in all the build machinery, cloning, making, etc. I'm also personally more familiar with gosu
, and it seems to be the more commonly used of the two (also easier for most of us to introspect go dependencies than C)
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.
In general I agree, but this would also mean to:
- Install
Go
into the docker image or - Use a
Go
based docker image and installpython
.
This also means:
- The docker image gets bigger.
- There will be an additional system dependency.
This is basically why I decided against gosu
in the first place.
Just a few notes to the current implementation (coming with this PR):
- The runtime docker image is based on the
base
image, which does not contain the build dependencies - it is purepython:3.8-alpine3.12
. - The
builder
image (which also buildssu-exec
) is cleaned at the very end and the build dependencies are removed as well.
If you like, I can still implement gosu
.
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.
Do you need go to run a go binary? I thought they could run without golang installed on the system. gosu
publishes binary releases, so I figured we might be able to use that.
If that's not the case, no need to bother with gosu
!
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.
I didn't know - I tested it and it works without installing Go
itself. Great. Changed 👍
Some changes:
- I replaced
su-exec
withgosu
. - I added the signature check (for
gosu
) according to the recommendation from https://github.com/tianon/gosu/blob/master/INSTALL.md. - I restructured the docker layers to separate the single build stages and their system dependencies. Also this separates the data within those intermediary docker images, even makes them smaller. - This only affects the docker image build.
Could you please have another look @mplanchard ?
Thanks a lot.
else | ||
# | ||
echo "Using custom CMD: $@" | ||
fi |
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.
I'm a little concerned that this update could break people's existing commands. If someone were relying on packages
being provided by default, and were adding extra commands on top of that, it seems like this might break, unless I'm missing something?
e.g. if someone were running docker run pypiserver -- log-file log.out
?
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.
I tested a little bit. So, as far as I can tell, the above command docker run pypiserver --log-file log.out
won't work in the current implementation, because the packages
directory is not added by default as soon as docker gets additional parameters.
Error: while trying to list root(/home/pypiserver/packages): [Errno 2] No such file or directory: '/home/pypiserver/packages'
Ok, there is a default packages
path: /home/pypiserver/packages
, but it does not exist in the docker container.
The default packages
path is added because of
CMD ["packages"]
However, this only works in case no additional docker commands are given when the container is started.
The only option, that's added every time is the port, because - in the original implementation - it is part of the ENTRYPOINT
:
ENTRYPOINT ["pypi-server", "-p", "8080"]
The given command in the Dockerfile's CMD
will be overwritten when doing
docker run pypiserver --log-file log.out
The command should be
docker run pypiserver --log-file log.out packages
to make it work properly.
I think the new implementation (contained in this PR) behaves in exactly the same way the original implementation does.
Of course it is possible to always add the default path, but I think, this will get confusing and can, in the end, actually affect user's commands.
What do you think?
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.
Thanks for digging in on that! I think you're right. Any specification of additional arguments on the current dockerfile would override the default packages
argument, so I think this will behave exactly like it did before.
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.
I'm good w/this going in either as-is or with gosu
! Let me know what you find w/that
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.
Thanks again for this! I'll get it merged today.
Also just noting that I've created a zulip to help organize community discussions, feature ideas, etc. Feel free to join if you'd like. |
@stephen-dexda pointed out in #341 that our update in #330 changed the `chown` operation to apply to the entire `/data` directory, rather than just `/data/packages`. For anyone who was previously relying on a workflow like mounting a read-only secrets directory into `/data` to host authentication information, this broke their workflow. This fix sets `entrypoint.sh` to only `chown` `/data/packages`, which should ensure that the permissions issues resolved by #330 (e.g. #309) remain fixed, while also fixing the issue in #341.
This PR udpates the Dockerfile.
Docker base images are updated.
The main change is the introduction of an
entrypoint
script, which sets the permission on the package directry/data/packages
.In the previous version of the
Dockerfile
this was done before declaring the same directory as dockerVOLUME
, which switched ownership back toroot
.This behaviour caused: #309
To still run
pypiserver
as user and notroot
, the toolsu-exec
was introduced and used withinentrypoint.sh
.