-
Notifications
You must be signed in to change notification settings - Fork 52
Build package with local UID/GID and fast-retrieve app/package information #168
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
Conversation
Signed-off-by: Gigon Bae <gbae@nvidia.com>
Yep, I requested Docker SDK be used at the start of the project. |
docker_build_cmd = ["docker", "build", "-f", docker_file_path, "-t", tag, temp_dir] | ||
docker_build_cmd = f'''docker build -f "{docker_file_path}" -t {tag} "{temp_dir}"''' | ||
if sys.platform != "win32": | ||
docker_build_cmd += """ --build-arg MONAI_UID=$(id -u) --build-arg MONAI_GID=$(id -g)""" |
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.
Hmm, the user ID and group of the current logged-on user is used to as MONAI_UID in the Docker that can potentially be run on any other server or in another logged-on user's env. Does not sound right to me.
General security principal is that you never do this, instead, a service account needs to be defined, and at deployment time, the account is configured/assigned to the app. It will be important to declare the intensions in this code.
This will work if the same user creates the MAP and then runs the MAP in the same env. What happens if this MAP is run by any other user (say 1000 creates it, 1003 runs it, and 1003 could be in a completely different set of groups)?
if sys.platform == "win32": | ||
cmd = f'docker run --rm -a STDOUT -a STDERR -v "{info_dir}":/var/run/monai/export/config {map_name}' | ||
else: | ||
cmd = f"""docker_id=$(docker create {map_name}) |
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.
This whole set of commands can be encapsulated in a method, and Docker SDK used to make it more robust and easier to handle exception.
In any case, It is a good change; don't run
docker when one only needs to copy files from within the Docker image.
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.
The fix correctly gets the current UID/GID to create Docker image, and the image will run fine with the current user running it, but what happens if anther use runs the MAP?
MAP is created with the default UID/GID (1000) by default.
However, if the user has UID which is not 1000 such as 1001, the local input/output folder is mounted with the host's input folder permission (1001) by App Runner and causes the following message when MAP is executed.
This patch builds a Docker image with local UID/GID to resolve the issue.
Also, it retrieves app/package information with the faster method if the OS is not Windows.
(copying app.json/pkg.json through
docker cp
command is faster than launching container to download config file).The bug is confirmed and tested by creating AWS EC2 instance:
Due to large base image size, only 1.3GB was left after exercise the Tutorial 1 (examples/apps/simple_imaging_app):
This patch is verified in the AWS EC2 instance by using the following test package:
Resolves #165
P.S. It might be better to use 'docker' client library instead of executing docker with shell command. We will address the issue later.