Skip to content
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

Add PUID and PGID to palworld docker image. #16

Merged
merged 3 commits into from
Jan 21, 2024
Merged

Add PUID and PGID to palworld docker image. #16

merged 3 commits into from
Jan 21, 2024

Conversation

fryfrog
Copy link
Contributor

@fryfrog fryfrog commented Jan 21, 2024

This is pretty simple. It checks that the PUID and PGID aren't 0 (root) and then just sets the steam user and group to them. The default is 1000:1000 to match the current behavior.

Switched to bash for the [[ ]] style if.

Should resolve #15.

@fryfrog
Copy link
Contributor Author

fryfrog commented Jan 21, 2024

Oops, forgot the readme and example compose file!

@thijsvanloef thijsvanloef self-assigned this Jan 21, 2024
@thijsvanloef thijsvanloef merged commit dd87782 into thijsvanloef:main Jan 21, 2024
3 checks passed
@thijsvanloef
Copy link
Owner

Thank you @fryfrog very helpful :)

@MoergJ
Copy link

MoergJ commented Jan 24, 2024

I would like to revisit this, as this means that the main process of the container can only be started as root, to be able to usermod and groupmod.
The main question seems to be, which one of the following is better:

  1. Change file permissions to allow the ID of the user running the process in the container to write to the volume
  2. Change the user ID to match the file permissions on the volume

If backup systems are the reason for this change, I would question these backup solutions, as an external system, that just copies files, can (in most cases) not be sure that those files are not currently being accessed or written to.

Please let me know, what your opinion on this is, maybe I'm missing something.

@fryfrog
Copy link
Contributor Author

fryfrog commented Jan 24, 2024

I think you are saying it'd be better to run the whole container as one user via Docker's own --user uid:gid right? I would totally agree w/ that, but it'd require quite a bit of change and of course you wouldn't be able to do anything that needs to be done as root.

I would make a new github issue and discuss it, maybe put up a pull request that does it?

I'm a big fan of not running containers as root, but they're pretty rare.

@fryfrog
Copy link
Contributor Author

fryfrog commented Jan 24, 2024

And to be clear, before this change it was hard coded doing things as either root or uid:gid 1000:1000 which was the steam user inside the container. This change did not make it run as root (in fact it does less as root).

@MoergJ
Copy link

MoergJ commented Jan 24, 2024

My proposal wouldn't even work with --user uid:gid as the user steam has a fixed uid and gid in the base image. Changing this would only be possible during build time.

@fryfrog
Copy link
Contributor Author

fryfrog commented Jan 24, 2024

Sure, that is what I mean right? Making this a non-root docker image would require a pretty big re-jiggering of the image. It should be possible, it just means that any chown or chmod needs to be done by the user outside the image. It really only does the two things, runs steam command to get the game and then runs steam command to run the game. Both of which are now literally hard coded to steam user, but obviously if you re-work everything to just run as the user it runs as (and probably error out if you accidentally do it as root.

All possible, just... makes setting up the image a little harder and more technical for people.

@MoergJ
Copy link

MoergJ commented Jan 24, 2024

That's why I dropped the whole change-uid stuff again. For me UID 1000 is perfectly fine and I have no need to change it (because I change the file permissions on the mounted volume). My main goal is to have an Image that contains a USER steam before the ENTRYPOINT that's basically it. This way no process in the running container is spawned by root.

@fryfrog
Copy link
Contributor Author

fryfrog commented Jan 24, 2024

If you set PUID and PGID to 1000, you have literally the same previous behavior. I'm not understanding something about what you want.

@MoergJ
Copy link

MoergJ commented Jan 25, 2024

From an application perspective, the behaviour is the same. The main difference is this:
The currently provided image

root@palworld-server-866c5db9b5-fvz6q:/home/steam/server# ps aux
USER         PID %CPU %MEM    VSZ   RSS TTY      STAT START   TIME COMMAND
root           1  0.0  0.0   3896  3076 ?        Ss   14:06   0:00 /bin/bash /home/steam/server/init.sh
root         119  0.0  0.0   3896  3052 ?        S    14:08   0:00 /bin/bash ./start.sh
root         187  0.0  0.0   4160  3400 pts/0    Ss   14:08   0:00 bash
root         199  0.0  0.0   6200  3508 ?        S    14:08   0:00 su steam -c ./PalServer.sh -queryport=27015
steam        200  0.0  0.0   2480   512 ?        Ss   14:08   0:00 sh -c ./PalServer.sh -queryport=27015
steam        201  0.0  0.0   2480   516 ?        S    14:08   0:00 /bin/sh ./PalServer.sh -queryport=27015
steam        208 13.6  0.8 5102408 1179044 ?     Sl   14:08   0:14 /palworld/Pal/Binaries/Linux/PalServer-Linux-Test Pal
root         269  0.0  0.0   6756  2856 pts/0    R+   14:10   0:00 ps aux

Mine, with removed usermod and groupmod, switched to USER steam during build time

steam@palworld-server-58bc74cf78-6jfp4:~/server$ ps aux
USER         PID %CPU %MEM    VSZ   RSS TTY      STAT START   TIME COMMAND
steam          1  0.0  0.0   3896  2904 ?        Ss   Jan24   0:00 /bin/bash /home/steam/server/init.sh
steam         99  0.0  0.0   3896  2940 ?        S    Jan24   0:00 /bin/bash ./start.sh
steam        170  0.0  0.0   2480   584 ?        S    Jan24   0:00 /bin/sh ./PalServer.sh -queryport=27015
steam        177 28.9  2.5 8969332 3301520 ?     Sl   Jan24 326:33 /palworld/Pal/Binaries/Linux/PalServer-Linux-Test Pal
steam        627  0.0  0.0   4160  3448 pts/0    Ss   14:07   0:00 bash
steam        634  0.0  0.0   6756  3016 pts/0    R+   14:07   0:00 ps aux

In the currently published image, PID 1 and immediate children are run as root. This is what I am addressing and changing in my fork, to not have any processes in the container run as root.

@fryfrog
Copy link
Contributor Author

fryfrog commented Jan 25, 2024

Ah, I got ya! That's what you'd get from a proper --user uid:gid designed image too. You've just hard coded it.

@MoergJ
Copy link

MoergJ commented Jan 25, 2024

Exactly, but with --user uid:gid (different than 1000:1000) the already existing files in the image are not writable, so might not be applicable in this case.

@fryfrog
Copy link
Contributor Author

fryfrog commented Jan 25, 2024

Yeah, exactly! A proper --user uid:gid docker image, the admin is the one that has to get ownership and permissions setup properly. Its really one of the biggest reasons a lot of containers run as root and then drop down using the PUID and PGID for the app. It lets them make sure that stuff is setup properly.

I bet you could make an image that works in both ways by simply checking if the uid/gid is 0 at startup. If it isn't, just don't run any commands that need to run as root and you're set.

@MoergJ
Copy link

MoergJ commented Jan 25, 2024

I'm sure we could. I personally don't yet understand the need to change from 1000:1000 in the first place. The permissions only become an issue when sharing a filesytem between host processes and the container. I would always prefer volumes over bind mounts. With volumes, this problem does not exist, or is easily solvable by changing the volumes file permissions.

I think I will maintain my fork for myself for the time being, until this is something that finds it's way back here 😃

Thanks @fryfrog for the discussion, I appreciate your effort!

@fryfrog
Copy link
Contributor Author

fryfrog commented Jan 25, 2024

I personally don't yet understand the need to change from 1000:1000 in the first place.

Really? This one is dead easy to explain. My personal user has the uid:gid 1000:1000 on all my hosts because it was the first one to be created. Why should anyone have to architect their system uid/gids around one docker image that hard coded it to 1000:1000?

MusclePr pushed a commit to MusclePr/palworld-server-docker that referenced this pull request Jun 19, 2024
Add PUID and PGID to palworld docker image.
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 environment variables to change User ID and Group ID
3 participants