ich777 / docker-steamcmd-server

Simple Dockerfile that installs steamcmd and a selected game server
119 stars 95 forks source link

ensure group is created in Dockerfile #19

Closed PrivatePuffin closed 2 years ago

PrivatePuffin commented 2 years ago

This ensures a group is created inside the container before the entrypoint is triggered, as some container engines might trigger a "group not found" error assigning the GID to said group.

This also makes sure the group GID is modified to ${GID} on the entrypoint as well, while also creating the ${GROUP} variable for dynamic group assignment.

ich777 commented 2 years ago

As already discussed here f0ea49b I have to look into this and report back.

I run Docker also on a Debian machine and changing the GID in my Docker run command with the environment variable --env 'GID=1000' gives me no issue whatsoever.

PrivatePuffin commented 2 years ago

As already discussed here f0ea49b I have to look into this and report back.

I run Docker also on a Debian machine and changing the GID in my Docker run command with the environment variable --env 'GID=1000' gives me no issue whatsoever.

Like I explained: Not all container engines share GID<->GroupName linkage from the host. You need to ensure this when building a good container.

The only thing this does is ensure this exists, on systems where it already does it shouldn't cause any issues.


You are putting too much effort into this IMHO, it just:

PrivatePuffin commented 2 years ago

Sidenote @ich777 Your FUNDING.md isn't working, you might need to enable the checkbox in the Github settings for the repo as well ;-)

ich777 commented 2 years ago

As said, I will look through this since this means some serious work for me because this is not the only branch that needs to be changed and this is only a project that I do in my free time...

I completely understand what this does but the container was in the beginning only designed mainly for Unraid and I'm curious to look into Kubernetes, I hope you understand this. Please give me a few days.

Sidenote @ich777 Your FUNDING.md isn't working, you might need to enable the checkbox in the Github settings for the repo as well ;-)

Really? This is really strange because I've received already a few Donations... :D Have to look into this too.

Anyways thank you for the report and PR, I'm very thankful for every PR...

PrivatePuffin commented 2 years ago

As said, I will look through this since this means some serious work for me because this is not the only branch that needs to be changed and this is only a project that I do in my free time...

You should link to the base image instead of redoing the complete image for every game ;-)

I completely understand what this does but the container was in the beginning only designed mainly for Unraid and

I understand, but even on normal docker this will not work if the GID does not have a group assigned on the host. Which is also (a bit vaguely I admit) explained to unraid users here: https://forums.unraid.net/topic/84160-should-i-create-a-new-uid-for-each-docker/

Even unraid users will hit error here if they don't create a group with that GID beforehand on the unraid system. So this actually also solves the same bug in Unraid, you just won't get the bug with your default GID, but that does not mean it isn't present on Unraid!

I'm curious to look into Kubernetes, I hope you understand this. Please give me a few days.

Please don't, this as NOTHING to do with kubernetes at all. (and will cause needless delays) Even with normal docker this will throw an error if the GID does not have a groupname assigned beforehand.

I have worked with this solution on other projects before, that needed to support multiple/normal docker systems.

ich777 commented 2 years ago

You should link to the base image instead of redoing the complete image for every game ;-)

This is exactly what I'm doing...

I understand, but even on normal docker this will not work if the GID does not have a group assigned on the host.

Yes, that's also clear to me and that is something that the end user should make sure. TBH you are the first one that reports this issue and I thought this was never become an issue at all if everything is setup correctly on the host.

Even unraid users will hit error here if they don't create a group with that GID beforehand on the unraid system. So this actually also solves the same bug in Unraid, you just won't get the bug with your default GID, but that does not mean it isn't present on Unraid!

Not really because this is the default group and you can of course change the GID to another user but why would anyone change the GID to a user that doesn't exist anyways?

I have worked with this solution on other projects before, that needed to support multiple/normal docker systems.

TBH I really like your PR but it will also work if everything is set up like it should be.

PrivatePuffin commented 2 years ago

Not really because this is the default group and you can of course change the GID to another user but why would anyone change the GID to a user that doesn't exist anyways?

As a Linux Dev-ops engineer, one works with nameless GID's all the time. It's not unheard of at all.

ich777 commented 2 years ago

As a Linux Dev-ops engineer, one works with nameless GID's all the time. It's not unheard of at all.

You have to give me a few days to change this.

PrivatePuffin commented 2 years ago

As a Linux Dev-ops engineer, one works with nameless GID's all the time. It's not unheard of at all.

You have to give me a few days to change this.

Awesome! Meanwhile I'll continue to port your work to Helm from the Helm side of things :)

ich777 commented 2 years ago

Helm is the "template" kind of thing or better the package manager for Kubernetes or am I wrong? If you need any icons for the containers you can find them here

PrivatePuffin commented 2 years ago

Helm is the "template" kind of thing or better the package manager for Kubernetes or am I wrong? If you need any icons for the containers you can find them here

Both partly. But I really don't need much help when it comes to images and such, my work i'm currently working on is more complicated than i'm able to discuss here.

PrivatePuffin commented 2 years ago

@ich777 Small update: The same problem also exists on containers like chromium from you as well it looks like :)

ich777 commented 2 years ago

@Ornias1993 yes I know, this is the case for all of my containers.

PrivatePuffin commented 2 years ago

Did some more work on finding bugs here... Found another related bug: If a user uses a read-only user.sh script, it currently is not executed because chmod is hard-failing. Latest commit just ignores that and continues execution.

No idea why it was hard-failing in the user-script if and soft failing in the other portions though...

PrivatePuffin commented 2 years ago

@ich777 I've managed to lower the amount of code considerably, while maintaining the effect we need here :)

ich777 commented 2 years ago

If a user uses a read-only user.sh script, it currently is not executed because chmod is hard-failing. Latest commit just ignores that and continues execution.

Would it be a viable option to copy the user.sh script to lets say start-user.sh and then chmod and execute it like:

echo "---Checking for optional scripts---"
if [ -f /opt/scripts/user.sh ]; then
    echo "---Found optional script, executing---"
    cp /opt/scripts/user.sh /opt/scripts/start-user.sh
    chmod -f +x /opt/scripts/start-user.sh
    /opt/scripts/start-user.sh
else
    echo "---No optional script found, continuing---"
fi

I thought about this about a year ago because I've seen this in the log from someone but everything was working because correctly because the permissions where set correctly. I know this is a little bit more work but I have to change the container anyways... :) What do you think about that?

@ich777 I've managed to lower the amount of code considerably, while maintaining the effect we need here :)

I'm really liking this change and will merge it ASAP short question before I do that, is it also viable to redirect the output to /dev/null and only display non errors from groupmod to avoid confusion for existing users with a change like groupmod -g ${GID} ${USER} 2>/dev/null? From what I see it is now checking if the group exists with this command and if not create it with the name steam and then it assigns the group to the steam user.

Just to let you know I will update the branches accordingly which already had the permissions change in place ASAP too, I will also have some time on Monday 18th and will update and test even more containers that doesn't had the changes yet.

Thank you for everything you are doing @Ornias1993! :)

PrivatePuffin commented 2 years ago

If a user uses a read-only user.sh script, it currently is not executed because chmod is hard-failing. Latest commit just ignores that and continues execution.

Would it be a viable option to copy the user.sh script to lets say start-user.sh and then chmod and execute it like:

echo "---Checking for optional scripts---"
if [ -f /opt/scripts/user.sh ]; then
    echo "---Found optional script, executing---"
    cp /opt/scripts/user.sh /opt/scripts/start-user.sh
    chmod -f +x /opt/scripts/start-user.sh
    /opt/scripts/start-user.sh
else
  echo "---No optional script found, continuing---"
fi

I thought about this about a year ago because I've seen this in the log from someone but everything was working because correctly because the permissions where set correctly. I know this is a little bit more work but I have to change the container anyways... :) What do you think about that?

Yes this would be prefered if you want to keep the '+x'. The best would be to keep user.sh in another folder like: /opt/custom/userscript.sh

Then copy it to /opt/scripts/ This would also prevent errors in the chown later in the code, which trigger when a script inside /opt/scripts is readonly

@ich777 I've managed to lower the amount of code considerably, while maintaining the effect we need here :)

I'm really liking this change and will merge it ASAP short question before I do that, is it also viable to redirect the output to /dev/null and only display non errors from groupmod to avoid confusion for existing users with a change like groupmod -g ${GID} ${USER} 2>/dev/null? From what I see it is now checking if the group exists with this command and if not create it with the name steam and then it assigns the group to the steam user.

Currently no group is being created, because the 'steam' group is made together with the steam user in the dockerfile. I came to this conclusion during my testing :)

groupmod only changes (hence mod) the already existing steam group, to use the GID as defined in ${GID}. So the problem was just that ${GID} was not yet linked to the right group and that's all :)

It should also not throw any errors as well :)

PrivatePuffin commented 2 years ago

@ich777 I've added code to ensure both the old /opt/scripts/user.sh and new /opt/custom/user.sh, get copied to /opt/scripts/start-user.sh

This ensures there are no breaking changes for current users, because users will hate that happening!

PrivatePuffin commented 2 years ago

I've shrunk the code a bit and made the echo-logging a bit clearer :)

ich777 commented 2 years ago

The best would be to keep user.sh in another folder like: /opt/custom/userscript.sh

Actually completely agreed. Since many users have custom scripts already added to their containers your additions are welcome and I will recommend from when I changed it in my containers that it should be mounted to /opt/custom/user.sh

It should also not throw any errors as well :)

Sure but I think it will tell in the logs that this group already exists or am I wrong? Just because the additional line, I know the users from my containers and they are really finicky when it comes to things like that... :D

This ensures there are no breaking changes for current users, because users will hate that happening!

Correct... :)

PrivatePuffin commented 2 years ago

Sure but I think it will tell in the logs that this group already exists or am I wrong? Just because the additional line, I know the users from my containers and they are really finicky when it comes to things like that... :D

It's not creating a group, it's changing it's GID. Like I said: This is groupmod nor groupadd.

ich777 commented 2 years ago

It's not creating a group, it's changing it's GID. Like I said: This is groupmod nor groupadd.

Sure thing, but I've now tested it and I get this:

root@e4a8dda95449:/# groupmod -g ${GID} ${USER} 
groupmod: GID '100' already exists
PrivatePuffin commented 2 years ago

It's not creating a group, it's changing it's GID. Like I said: This is groupmod nor groupadd.

Sure thing, but I've now tested it and I get this:

root@e4a8dda95449:/# groupmod -g ${GID} ${USER} 
groupmod: GID '100' already exists

Ahh the GID specifically, yes we need to hide that one indeed. But we can just dev null all the output from it as it's just an extra catch.

PrivatePuffin commented 2 years ago

Fixed, also prevented it from failing the script on failure in case that might happen to some.

ich777 commented 2 years ago

Fixed, also prevented it from failing the script on failure in case that might happen to some.

Thank you! I hope I have time to merge this today evening when I get home from work and change all the other SteamCMD containers which had the previous chmod change in it and rebuilt them.

ich777 commented 2 years ago

@Ornias1993 just a quick update, changed all branches where the changes for chown was made and containers are already rebuilt.

Will change all my other containers too but this will take some time, also sent you a little sponsor money for your work here!

PrivatePuffin commented 2 years ago

Awesome, thanks a lot :) With a bit of luck all you games should be available as TrueNAS SCALE Apps as well as Unraid Apps by the end of summer 👍

ich777 commented 2 years ago

as well as Unraid Apps

They are already available as Unraid apps... Click & Click

I started creating these container and I mainly create these containers for Unraid.

PrivatePuffin commented 2 years ago

as well as Unraid Apps

They are already available as Unraid apps... Click & Click

I started creating these container and I mainly create these containers for Unraid.

That was what I was saying: SCALE Apps as well as your own current Unraid Apps.

ich777 commented 2 years ago

SCALE Apps as well as your own current Unraid Apps.

Just wrote that to make 100% sure you are not working on Unraid templates too because it was not 100% clear to me that you mean my existing templates. :) Thank you again.