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

Removed root processes #216

Merged
merged 39 commits into from
Feb 18, 2024

Conversation

Dashboy1998
Copy link
Contributor

@Dashboy1998 Dashboy1998 commented Jan 30, 2024

Context

Choices

  • Keeping the ability to still run as root as to not break anything. Ideally we would later change the image so PUID/PGID is not used and --user must be used.

Test instructions

Important

You must set user to NUMBERICAL_UID:NUMBERICAL_GID
To find your UID run id -u & to find your GID run id -g. In the examples they are listed as 1000:1000.
If you wish to run it as a different UID/GID this can by done by changing the ownership: chown UID:GID palworld/
or by changing the permissions for all other: chmod o=rwx palworld/

  1. Try to start the container as root:root, It should work
  2. Try to start the container as root:steam, Given error
  3. Try to start the container as steam:root, Given error
  4. Start container with 1000:1000 (This is the same as steam:steam)
  5. Verify rcon-cli is working: docker exec palworld-server rcon-cli showplayers
  6. Verify backup works: docker exec palworld-server backup
  7. Verify no processes are running as root: docker exec -t palworld-server top -n 1
  8. Verify supercronic is running: docker exec -t palworld-server bash -c "top -n 1 | grep --color supercronic"

Checklist before requesting a review

  • I have performed a self-review of my code
  • I've added documentation about this change to the README.
  • I've not introduced breaking changes.

@Dashboy1998 Dashboy1998 changed the title WIP: Removed root processes Removed root processes Jan 30, 2024
@Dashboy1998 Dashboy1998 marked this pull request as ready for review January 30, 2024 04:10
@Dashboy1998 Dashboy1998 marked this pull request as draft January 30, 2024 04:12
@Dashboy1998 Dashboy1998 changed the title Removed root processes WIP: Removed root processes Jan 30, 2024
@Dashboy1998 Dashboy1998 changed the title WIP: Removed root processes Removed root processes Jan 30, 2024
@Dashboy1998 Dashboy1998 marked this pull request as ready for review January 30, 2024 14:32
@Dashboy1998 Dashboy1998 marked this pull request as draft January 31, 2024 05:37
@Dashboy1998
Copy link
Contributor Author

Still in progress.

@Dashboy1998 Dashboy1998 marked this pull request as ready for review February 15, 2024 05:55
@thijsvanloef
Copy link
Owner

Would it be possible to port these changes to a true rootless container? Like the one https://github.com/CM2Walki/steamcmd offers?

@Dashboy1998
Copy link
Contributor Author

Dashboy1998 commented Feb 16, 2024

Would it be possible to port these changes to a true rootless container? Like the one https://github.com/CM2Walki/steamcmd offers?

Yes.

Now if we do that we have two options.

  1. Only start as steam (1000:1000) however this means the files that are created are owned by 1000:1000 in your binds so you must make sure 1000 has permission in the bind.
  2. Allow them to start as any nonroot user. Just need to remove the su and all root specific tasks.

Now the big change by switching to a rootless container would be everyone would need to change from PUID/PGID to user: UID:GID. The solution I made is backward compatible, although probably best to abandon it now rather than have technical debt moving forward.

@Dashboy1998
Copy link
Contributor Author

@thijsvanloef There is no advantage to switching from cm2network/steamcmd:root to cm2network/steamcmd:steam as we would have to change to root in the dockerfile to install packages then switch back to steam.

If you want to move forward on dropping PUID/PGUID and running with user: 0:0 then let me know and I can implement that.

@thijsvanloef
Copy link
Owner

There is no advantage to switching from cm2network/steamcmd:root to cm2network/steamcmd:steam as we would have to change to root in the dockerfile to install packages then switch back to steam.

Yeah I believe that is why I opened this repo with the base image with the :root tag.

I want to keep the PUID/PGUID as that keeps it backwards compatible, and close to the linuxserver.io standard.
I would much rather keep the PUID PGID and drop the user: 0:0 from the examples as that confuses people

@thijsvanloef thijsvanloef merged commit 16c52bf into thijsvanloef:main Feb 18, 2024
9 checks passed
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.

None yet

2 participants